-
Notifications
You must be signed in to change notification settings - Fork 73
feat: version 3 #609
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: version 3 #609
Conversation
WalkthroughThis pull request introduces significant changes across the CI workflow, configuration files, API modules, tests, and type definitions. A new CI step has been added for running type checks and various editor and build configuration adjustments are made via VS Code settings, tsconfig, and Vite. A large number of legacy API files and tests have been removed and replaced with a new modular API structure (with dedicated directories for config, consent, custom-map, ecommerce, event, etc.) alongside updated core modules and composables for tracking, settings, and consent. New TypeScript type definitions and tests have been added to support these improvements. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant A as Vue App
participant B as bootstrap()
participant Q as query()
participant O as optIn()
U->>A: Trigger enableCookies()
A->>B: Call bootstrap()
B-->>A: Return gtag instance
A->>Q: Call query("event", "consent", {…})
Q-->>A: Consent event sent
A->>O: Call optIn()
Assessment against linked issues
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (21)
.github/workflows/deployment.yml (1)
28-28: Remove trailing whitespace.There is trailing whitespace on line 28 that should be removed.
- +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 28-28: trailing spaces
(trailing-spaces)
src/api/set.test.ts (1)
7-8: Fix grammar in test description.The test description has a grammatical error.
- it("should the set command", () => { + it("should use the set command", () => {src/api/refund.ts (1)
3-15: Consider documenting the RefundParams typeThe type definition is well-structured, but adding JSDoc comments would help developers understand the purpose of each field and how they relate to Google Analytics refund tracking.
+/** + * Parameters for tracking a refund event + * @see https://developers.google.com/analytics/devguides/collection/ga4/refunds + */ type RefundParams = | { + /** The transaction ID of the refunded transaction */ transaction_id?: string; + /** The monetary value of the refund */ value?: number; + /** The currency code */ currency?: string; + /** The items that were refunded */ items?: Array<{ id?: string; name?: string; quantity?: number; price?: number; }>; } | Gtag.CustomParams;src/api/purchase.test.ts (1)
1-16: Test correctly verifies the purchase event APIThe test properly verifies that the
purchasefunction delegates to thequeryfunction with the correct parameters.Consider adding tests for additional scenarios in the future, such as:
- Multiple purchase parameters
- Different transaction types
- Edge cases with missing parameters
src/api/query.test.ts (1)
1-20: Test looks good but could use setup/teardown for improved isolationThe test correctly verifies that the
queryfunction passes events to the gtag library by checking the window's data layer. For improved test robustness, consider:
- Adding setup/teardown to clean the data layer between tests
- Mocking the window object instead of using the actual global window
+ beforeEach(() => { + // Reset dataLayer before each test + const { dataLayerName } = getSettings(); + window[dataLayerName] = []; + });This will prevent potential test interference if more test cases are added in the future.
src/settings.test.ts (1)
3-28: Tests look good but could benefit from better isolationThe tests correctly verify the default settings configuration and update functionality. For improved test isolation, consider:
- Adding a reset function to restore default settings between tests
- Verifying that unspecified settings remain unchanged when updating
+ beforeEach(() => { + // Reset to default settings before each test + resetSettings(); + });Also, consider expanding the second test to verify that other settings remain unchanged:
expect(getSettings()).toEqual( expect.objectContaining({ tagId: "UA-1234567", gtagName: "foo", + // Verify other settings remain unchanged + resourceUrl: "https://www.googletagmanager.com/gtag/js", + dataLayerName: "dataLayer", }), );src/api/config.test.ts (1)
5-40: Tests look good but need improved test isolationThe tests correctly verify that the
configfunction works as expected with both single and multiple accounts. For improved test isolation:
- Reset settings between tests
- Reset mock call history before each test
+ beforeEach(() => { + resetSettings(); + vi.clearAllMocks(); + });This ensures that each test starts with a clean state and makes verification more reliable.
src/api/query.ts (1)
6-11: Consider adding JSDoc comments.The function would benefit from documentation explaining its purpose, parameters, and return value.
+/** + * Executes a gtag command with the provided arguments + * @param args - The arguments to pass to the gtag function + */ export default function query(...args: QueryParams) { const { dataLayerName, gtagName } = getSettings(); if (isServer()) { return; } }src/index.ts (1)
6-13: Consider stronger typing for the global Window interface.While the biome-ignore comments are necessary to bypass linting, consider using more specific types than
any | any[]for better type safety and intellisense support.declare global { interface Window { // biome-ignore lint/suspicious/noExplicitAny: - [gtag: Settings["gtagName"]]: any | any[]; + [gtag: Settings["gtagName"]]: (...args: any[]) => void | any[]; // biome-ignore lint/suspicious/noExplicitAny: - [dataLayer: Settings["dataLayerName"]]: any | any[]; + [dataLayer: Settings["dataLayerName"]]: any[][] | undefined; } }src/add-configuration.test.ts (1)
60-75: LGTM on the linker test.This test correctly verifies the linker configuration is properly set before initializing the tracking.
Consider adding a test case for when
tagIdis not provided to verify the early return behavior.src/add-router.tracking.test.ts (1)
1-56: Well-structured tests for router tracking functionalityThe tests cover both initial tracking (when router is ready) and tracking after navigation, which is comprehensive. The use of
expect.objectContaining()for partial matching in assertions is a good practice.However, there's a discrepancy in naming - the file is named
add-router.tracking.test.ts(with a dot) while it imports fromadd-router-tracking(with a hyphen).Consider updating the file name to follow the same naming convention as the module it's testing:
- src/add-router.tracking.test.ts + src/add-router-tracking.test.tssrc/api/screenview.ts (2)
12-24: Type handling looks robust, but could be improvedThe function correctly handles different parameter types with proper type guards. However, the type assertion
as stringon line 20 could be avoided with better type narrowing.- template.screen_name = (params.name ?? params.path) as string; + template.screen_name = params.name ? String(params.name) : String(params.path);
25-27: Unnecessary optional chainingSince
templateis always defined (initialized on line 15), the optional chaining intemplate?.app_nameis unnecessary.- if (pageTracker?.appName && template?.app_name === undefined) { + if (pageTracker?.appName && template.app_name === undefined) {src/track-route.ts (1)
6-16: Redundant optional chaining in exclusion checkThere's an unnecessary optional chaining in the exclude check on line 13, since
pageTracker?.excludeis already checked on line 9.- return pageTracker.exclude?.some(({ name, path } = {}) => { + return pageTracker.exclude.some(({ name, path } = {}) => {src/utils.ts (2)
36-41: Consider enhancing error handling for script loadingWhile the basic error handling works, it would be helpful to provide more detailed error information when script loading fails.
- script.onerror = reject; + script.onerror = (event) => { + reject(new Error(`Failed to load script from ${url}: ${event}`)); + };
5-41: Consider handling duplicate script injectionsThe current implementation doesn't check if a script with the same URL is already loaded or being loaded, which could lead to duplicate injections.
export async function injectScript( url: string, options?: { preconnectOrigin?: string; defer?: boolean; }, ): Promise<void> { return new Promise((resolve, reject) => { if (isServer()) { return; } + // Check if script is already loaded or loading + const existingScript = document.querySelector(`script[src="${url}"]`); + if (existingScript) { + // If the script is already loaded, resolve immediately + if (existingScript.hasAttribute('data-loaded')) { + resolve(); + return; + } + + // If the script is still loading, wait for it + existingScript.addEventListener('load', () => resolve()); + existingScript.addEventListener('error', reject); + return; + } const head = document.head; const script = document.createElement("script"); script.async = true; script.src = url; + script.setAttribute('data-loading', 'true'); if (options?.defer) { script.defer = true; } if (options?.preconnectOrigin) { const link = document.createElement("link"); link.href = options.preconnectOrigin; link.rel = "preconnect"; head.appendChild(link); } head.appendChild(script); - script.onload = () => resolve(); + script.onload = () => { + script.setAttribute('data-loaded', 'true'); + script.removeAttribute('data-loading'); + resolve(); + }; script.onerror = reject; }); }src/api/pageview.ts (1)
21-56: Consider improving type safety for the Route interfaceThe implementation handles different input types well, but there's a type assertion on line 38 (
params.name as string) that could be avoided with a more precise type definition for the Route interface.Consider updating the Route interface in
src/types.tsto explicitly define thenameproperty as a string or string | undefined to avoid the need for type assertion:// In src/types.ts export interface Route { path: string; fullPath: string; + name: string | undefined; // other properties... }src/track-route.test.ts (3)
55-67: Consider adding URL origin verification for page_locationThe test properly validates pageview tracking, but it assumes that
page_locationwill always be "http://localhost:3000/about". In a real browser environment, the origin might differ.Consider making the test more robust by using a more generic verification:
expect(query).toHaveBeenCalledWith("event", "page_view", { page_path: "/about", - page_location: "http://localhost:3000/about", + page_location: expect.stringContaining("/about"), page_title: "about", send_page_view: true, });
69-99: Well-structured exclusion testsThe tests for route exclusion properly verify that tracking doesn't happen for excluded routes, testing both path and name-based exclusions.
Consider adding a test for combined exclusion criteria (both path and name) to ensure the exclusion logic handles multiple conditions properly.
181-201: Consider adding test for error handlingThe test suite covers the happy path thoroughly but doesn't test error scenarios.
Consider adding tests for error handling, such as:
- What happens when trackRoute is called with an invalid route
- What happens when a template function throws an error
- How errors in callbacks are handled
This would provide more confidence in the robustness of the tracking functionality.
src/settings.ts (1)
49-50: Consider using a more robust state management approachThe current implementation uses a mutable variable to store settings, which could potentially lead to unexpected behavior if accessed or modified directly.
Consider using a more robust approach such as:
- Using a private variable with getter/setter methods
- Implementing a reactive store (like Vue's reactive)
- Using a singleton pattern with controlled access
-let settings: Settings = { ...defaultSettings }; +const settingsState = { + _settings: { ...defaultSettings } as Settings, + get settings(): Settings { + return { ...this._settings }; + }, + reset(): void { + this._settings = { ...defaultSettings }; + }, + update(configParams: Partial<Settings>): void { + this._settings = { ...this._settings, ...configParams }; + } +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsontest/__snapshots__/options.spec.js.snapis excluded by!**/*.snaptest/__snapshots__/utils.spec.js.snapis excluded by!**/*.snap
📒 Files selected for processing (97)
.github/workflows/deployment.yml(1 hunks).vscode/settings.json(1 hunks)biome.json(1 hunks)package.json(3 hunks)src/add-configuration.js(0 hunks)src/add-configuration.test.ts(1 hunks)src/add-configuration.ts(1 hunks)src/add-gtag.test.ts(1 hunks)src/add-gtag.ts(1 hunks)src/add-router-tracking.ts(1 hunks)src/add-router.tracking.test.ts(1 hunks)src/add-routes-tracker.js(0 hunks)src/api/config.js(0 hunks)src/api/config.test.ts(1 hunks)src/api/config.ts(1 hunks)src/api/custom-map.js(0 hunks)src/api/custom-map.test.ts(1 hunks)src/api/custom-map.ts(1 hunks)src/api/disable.js(0 hunks)src/api/event.js(0 hunks)src/api/event.test.ts(1 hunks)src/api/event.ts(1 hunks)src/api/exception.js(0 hunks)src/api/exception.test.ts(1 hunks)src/api/exception.ts(1 hunks)src/api/index.js(0 hunks)src/api/index.ts(1 hunks)src/api/linker.js(0 hunks)src/api/linker.test.ts(1 hunks)src/api/linker.ts(1 hunks)src/api/opt-in.js(0 hunks)src/api/opt-out.js(0 hunks)src/api/pageview.js(0 hunks)src/api/pageview.test.ts(1 hunks)src/api/pageview.ts(1 hunks)src/api/purchase.js(0 hunks)src/api/purchase.test.ts(1 hunks)src/api/purchase.ts(1 hunks)src/api/query.js(0 hunks)src/api/query.test.ts(1 hunks)src/api/query.ts(1 hunks)src/api/refund.js(0 hunks)src/api/refund.test.ts(1 hunks)src/api/refund.ts(1 hunks)src/api/screenview.js(0 hunks)src/api/screenview.test.ts(1 hunks)src/api/screenview.ts(1 hunks)src/api/set.js(0 hunks)src/api/set.test.ts(1 hunks)src/api/set.ts(1 hunks)src/api/time.js(0 hunks)src/api/time.test.ts(1 hunks)src/api/time.ts(1 hunks)src/attach-api.js(0 hunks)src/bootstrap.js(0 hunks)src/index.js(0 hunks)src/index.test.ts(1 hunks)src/index.ts(1 hunks)src/options.js(0 hunks)src/register-globals.js(0 hunks)src/router.js(0 hunks)src/settings.test.ts(1 hunks)src/settings.ts(1 hunks)src/track-route.test.ts(1 hunks)src/track-route.ts(1 hunks)src/track.js(0 hunks)src/types.ts(1 hunks)src/utils.js(0 hunks)src/utils.test.ts(1 hunks)src/utils.ts(1 hunks)test/add-configuration.spec.js(0 hunks)test/add-routes-tracker.spec.js(0 hunks)test/api/config.spec.js(0 hunks)test/api/custom-map.spec.js(0 hunks)test/api/disable.spec.js(0 hunks)test/api/event.spec.js(0 hunks)test/api/exception.spec.js(0 hunks)test/api/linker.spec.js(0 hunks)test/api/opt-in.spec.js(0 hunks)test/api/opt-out.spec.js(0 hunks)test/api/pageview.spec.js(0 hunks)test/api/purchase.spec.js(0 hunks)test/api/query.spec.js(0 hunks)test/api/refund.spec.js(0 hunks)test/api/screenview.spec.js(0 hunks)test/api/set.spec.js(0 hunks)test/api/time.spec.js(0 hunks)test/basic.spec.js(0 hunks)test/bootstrap.spec.js(0 hunks)test/exports.spec.js(0 hunks)test/options.spec.js(0 hunks)test/register-globals.spec.js(0 hunks)test/track.spec.js(0 hunks)test/utils.spec.js(0 hunks)tsconfig.json(1 hunks)vite.config.js(1 hunks)vue-gtag.d.ts(0 hunks)
💤 Files with no reviewable changes (51)
- test/api/purchase.spec.js
- src/api/set.js
- src/api/query.js
- test/api/set.spec.js
- src/api/time.js
- src/api/refund.js
- src/api/linker.js
- src/api/purchase.js
- src/api/opt-out.js
- test/api/opt-in.spec.js
- src/add-routes-tracker.js
- src/api/opt-in.js
- src/attach-api.js
- test/api/pageview.spec.js
- src/add-configuration.js
- test/api/refund.spec.js
- test/api/config.spec.js
- test/api/linker.spec.js
- src/api/config.js
- src/api/event.js
- test/add-configuration.spec.js
- test/api/opt-out.spec.js
- src/router.js
- src/api/custom-map.js
- test/api/custom-map.spec.js
- src/api/exception.js
- src/register-globals.js
- test/api/exception.spec.js
- src/bootstrap.js
- test/bootstrap.spec.js
- test/basic.spec.js
- src/api/disable.js
- test/api/time.spec.js
- test/exports.spec.js
- src/api/screenview.js
- test/register-globals.spec.js
- test/track.spec.js
- src/api/pageview.js
- test/api/screenview.spec.js
- test/utils.spec.js
- test/options.spec.js
- vue-gtag.d.ts
- src/api/index.js
- test/api/event.spec.js
- src/options.js
- test/api/query.spec.js
- test/api/disable.spec.js
- src/utils.js
- test/add-routes-tracker.spec.js
- src/track.js
- src/index.js
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/deployment.yml
[error] 28-28: trailing spaces
(trailing-spaces)
🔇 Additional comments (106)
.vscode/settings.json (1)
7-10: Great formatting standardization!These new editor settings ensure consistent code formatting across the project. Setting a tab size of 2 spaces, using spaces instead of tabs, and disabling automatic indentation detection will help maintain uniform code style for all contributors.
tsconfig.json (1)
1-45:❓ Verification inconclusive
Well-structured TypeScript configuration with one potential conflict
The TypeScript configuration is comprehensive and follows best practices with strict type checking, proper module resolution, and path aliases.
However, there appears to be a contradiction between
noEmit: true(line 14), which prevents TypeScript from generating output files, and the declaration settings (lines 33-35) that specify generating declaration files. This might cause unexpected behavior during builds.Consider clarifying how these settings should work together. If you intend to generate declaration files during the build process, you might need to:
- Remove
noEmit: truefor production builds- Use conditional tsconfig settings between development and production
- Or rely on the vite-plugin-dts (as configured in vite.config.js) to handle type generation separately
🏁 Script executed:
#!/bin/bash # Check if there are any npm scripts that might modify the tsconfig settings during build grep -r "tsc" package.jsonLength of output: 102
Clarify Declaration Generation vs. NoEmit Usage
The TypeScript configuration is well-structured and enforces strict type safety. However, there's an intentional setup where the configuration enables declaration file generation (via
"declaration": true,"declarationDir", and"outDir"), yet the"noEmit": trueoption prevents TypeScript from producing any output. The package.json scripts (usingtsc --noEmit) confirm that type checking is performed without actual file emission—indicating that declaration generation is likely managed by a tool likevite-plugin-dts(configured in vite.config.js).Action items:
- Verify Intent: Confirm that the current strategy—to rely on
vite-plugin-dts(or a similar tool) for generating declaration files instead of TypeScript’s own emission—is intentional.- Document Rationale: If this design is intended, add a comment in the tsconfig.json or in your project documentation explaining why declarations are configured yet TypeScript is prevented from emitting files. This will help future maintainers understand the separation of type-checking and declaration generation.
- Consider Conditional Configurations: Alternatively, if you expect tsc to generate declarations during production builds, consider using conditional tsconfig settings or removing
"noEmit": truein the production configuration.vite.config.js (6)
3-3: Good addition of TypeScript declaration file generationAdding the vite-plugin-dts plugin is appropriate for a TypeScript project to ensure proper type definitions are generated for consumers.
19-19: Correct Vue global assignmentChanging the Vue global from lowercase to capitalized (
Vueinstead ofvue) is correct as this matches Vue's actual global object name.
23-23: Good choice of minifierUsing terser for minification is a solid choice for optimizing the build output while maintaining compatibility.
25-30: Proper TypeScript declaration configurationThe DTS plugin is configured well to generate type declarations and make them accessible through the package's entry point.
34-37: Enhanced test configurationAdding mock reset and coverage reporting improves test isolation and provides better visibility into test coverage, which is valuable for maintaining code quality.
9-12:❓ Verification inconclusive
Moving to TypeScript and dropping older module formats
The switch from JavaScript to TypeScript for the entry point aligns with the project's migration to TypeScript. However, note that reducing support to only ES modules (dropping UMD and CommonJS) might affect compatibility with some environments. Ensure this is an intentional breaking change aligned with your version 3 strategy.
🏁 Script executed:
#!/bin/bash # Check if there are any references to UMD or CJS usage in docs or issues grep -r "umd\|cjs\|commonjs" --include="*.md" --include="*.js" .Length of output: 66
Subject: Verify ES Module-Only Config Change for V3 Migration
The updated configuration switches the entry file to TypeScript and now supports only ES modules. While the grep check did not reveal references to UMD/CommonJS in markdown or JavaScript files, this output is inconclusive. Please manually verify that no documentation, issues, or external dependencies rely on legacy module formats, ensuring that this intentional breaking change fully aligns with your version 3 strategy.
- Location:
vite.config.js, lines 9–12- Key Change: Dropped UMD/CommonJS support—only ES modules are provided.
- Action: Confirm through manual review or further investigation that no parts of the project or its consumers depend on UMD or CommonJS builds.
src/api/time.ts (1)
1-5: Clean implementation of timing event trackingThis function follows good practices by being focused, simple, and well-typed. The
Gtag.EventParamstype annotation ensures type safety for consumers of this API.src/api/purchase.ts (1)
1-5: Clean implementation of the purchase tracking function.The function properly wraps the event function to track purchase events with Google Analytics, following a consistent pattern established in the API design.
src/api/exception.ts (2)
3-8: Well-defined type union for exception parameters.Good use of a union type to provide flexibility in how exception parameters can be specified, allowing either a simple object with optional properties or the full Gtag.CustomParams.
10-12: Clean implementation of the exception tracking function.The function correctly wraps the event function to track exception events with Google Analytics.
src/api/set.ts (2)
3-3: Good use of type alias for better code readability.Using a type alias for the set command parameters improves code readability and maintainability.
5-7: Clean implementation of the set function using rest parameters.The function correctly uses rest parameters to accept variable arguments and passes them to the query function with the "set" command.
src/api/custom-map.ts (2)
3-3: Appropriate type alias for custom map parameters.The type alias helps with code readability while maintaining the connection to the underlying Gtag types.
5-9: Clean implementation of the custom map configuration.The function properly wraps the parameters in an object with the custom_map property before passing to the config function.
.github/workflows/deployment.yml (1)
28-30: Great addition of type checking to the CI workflow.Adding a typecheck step to the CI workflow is a valuable improvement that will help catch type-related issues early in the development process. This aligns well with the TypeScript implementations mentioned in the PR summary.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 28-28: trailing spaces
(trailing-spaces)
src/api/exception.test.ts (1)
1-18: Well-structured unit test for the exception function.The test properly verifies that the exception function forwards the correct parameters to the query function with the "exception" event type. Good use of mocking to isolate the test from dependencies.
src/api/set.test.ts (1)
1-14: Good unit test structure for the set function.The test correctly verifies that the set function forwards parameters to the query function. The mocking approach effectively isolates the test from external dependencies.
src/api/time.test.ts (1)
1-16: Well-implemented unit test for the time function.This test effectively verifies that the time function correctly triggers the "timing_complete" event with the provided parameters. Good use of mocking to isolate the test functionality.
src/api/linker.test.ts (1)
1-16: Well-structured test with good isolationThis test properly verifies that the linker function correctly passes parameters to the query function. The test structure follows good practices by:
- Mocking dependencies for isolation
- Using clear and descriptive test names
- Verifying the exact arguments passed to the mocked function
src/api/refund.ts (2)
17-19: Function implementation looks goodThe implementation correctly calls the event function with the appropriate parameters.
15-15:❓ Verification inconclusive
Verify Gtag type availability
The code references
Gtag.CustomParamswithout an explicit import. Ensure this type is globally available or properly imported.
🏁 Script executed:
#!/bin/bash # Check if Gtag type is defined somewhere in the project echo "Checking for Gtag type definition..." rg -l "type\s+Gtag|interface\s+Gtag|namespace\s+Gtag|declare\s+namespace\s+Gtag" --type tsLength of output: 175
Attention: Verify Gtag Global Definition
The code still references
Gtag.CustomParamswithout an explicit import. Our search through the project's TypeScript files did not reveal any in-repository definition (e.g., via a type, interface, or namespace declaration). This suggests you may be relying on a globally provided type—perhaps from an external package such as@types/gtag.js—or an ambient declaration.
- Action Required: Please verify that the global type for
Gtagis indeed available in your project (for example, via an installation of the appropriate type definitions). If not, add the necessary import and/or declare the type explicitly to prevent potential type errors.src/api/linker.ts (2)
3-8: Well-defined type with appropriate optionalityThe
LinkerParamstype correctly defines which fields are required vs optional. The use of a string union type forurl_positionis a good practice for restricting values to valid options.
10-12: Function implementation is concise and effectiveThe linker function has a clear purpose and delegates to the query function appropriately.
src/api/refund.test.ts (2)
1-5:❓ Verification inconclusive
Check import alignment with implementation
The test imports
querydirectly while the implementation inrefund.tsuses theeventfunction. This suggests there might be a mismatch in how the mocking is set up.
🏁 Script executed:
#!/bin/bash # Check how event.ts is implemented echo "Checking event.ts implementation..." cat src/api/event.ts || echo "File not found"Length of output: 614
Mismatch in Mocking Setup for Refund Tests
The implementation in
src/api/event.tsconfirms that theeventfunction is used to triggerquerycalls. However, insrc/api/refund.test.ts, the test currently imports and mocksquerydirectly. Sincerefund.ts(as indicated in the review comment) uses theeventfunction rather than callingquerydirectly, please verify that the mocking in the test aligns with the actual call paths. Adjust the mocks (or imports) accordingly to cover the real implementation flow.
6-16:❓ Verification inconclusive
Ensure assertion matches implementation
The test expects
queryto be called with("event", "refund", ...), but the implementation callsevent("refund", ...). This discrepancy should be resolved by either:
- Updating the test to match the implementation
- Verifying that the
eventfunction internally callsqueryas expected
🏁 Script executed:
#!/bin/bash # Check how the event function is implemented echo "Checking how event function calls query..." rg -A 5 "function event\(" --type ts src/apiLength of output: 489
Mismatch between test assertion and implementation of refund event
The test is asserting that
queryis called with("event", "refund", { transaction_id: "transaction_id_value" }), yet therefundfunction callsevent("refund", { transaction_id: "transaction_id_value" }). The current implementation ofeventinsrc/api/event.ts(as seen from the snippet) does not clearly show an invocation ofquerywith those parameters.Please consider one of the following actions:
- Update the test: Change the expectation to verify that
eventis called with the appropriate arguments, if that is the intended behavior.- Enhance the implementation: Ensure that the
eventfunction internally callsquerywith the correct parameters if the test’s expectation is correct.Review the implementations in both
src/api/refund.test.ts(lines 6–16) andsrc/api/event.tsto align the behavior and the tests accordingly.src/add-router-tracking.ts (1)
1-20: Clean implementation of router tracking!The
addRouterTrackingfunction is well-structured and follows the single responsibility principle. It correctly:
- Retrieves settings and checks for router existence
- Waits for the router to be ready with
await router.isReady()- Tracks the initial route
- Sets up an event listener for subsequent route changes
The early return pattern when router is absent is a good practice.
src/utils.test.ts (1)
4-47: Well-structured test suite for injectScriptThe test coverage is comprehensive, testing:
- Basic script tag creation
- Script with defer attribute
- Link element for domain preconnect
Good job using
afterEachto clean up the DOM between tests.src/api/event.ts (2)
4-5: Type definitions for event parameters look goodThe TypeScript type definitions correctly leverage the Gtag types for proper type safety.
7-17: Well-implemented event handling functionThe function correctly:
- Retrieves settings
- Handles the variable arguments pattern
- Applies conditional logic for the
send_toparameter- Calls the underlying query function
The conditional logic on line 12 ensures that
send_tois only set when there are additional accounts and when it hasn't been explicitly defined by the caller.src/api/custom-map.test.ts (1)
7-23: Well-structured test with good isolation practicesThe test correctly verifies the
customMapfunction behavior and follows good practices:
- Uses
beforeEachto reset settings between tests- Clearly sets up the test state before executing the function
- Makes specific assertions about the expected behavior
This approach ensures good test isolation and maintainability.
biome.json (2)
11-11: Well-structured file inclusion for TypeScript.The updated include pattern now specifically targets TypeScript files, the TypeScript configuration file, and global type declarations, which is more precise than including all files in the src directory.
15-16: Good formatting consistency.Adding the indentWidth property ensures consistent indentation (2 spaces) across the codebase, which is a standard practice in many JavaScript/TypeScript projects.
src/api/config.ts (2)
5-5: Good type definition using Gtag types.Using TypeScript's ability to extract parts of existing types via
Tail<Gtag.GtagCommands["config"]>is an excellent way to maintain type safety while reusing the Google Analytics type definitions.
7-23: Well-structured implementation with proper error handling.The function correctly:
- Uses TypeScript rest parameters with proper typing
- Gets settings and handles the case when tagId is not available
- Processes the main tag configuration
- Handles additional accounts when present
The early returns pattern helps maintain flat code structure and readability.
src/types.ts (3)
4-7: Good type re-exporting practice.Re-exporting types from vue-router with the alias
Routeinstead of the more verboseRouteLocationNormalizedGenericimproves code readability throughout the codebase.
9-9: Excellent utility type implementation.The
Tail<T>type using conditional types and inference is a great example of advanced TypeScript usage to extract all elements except the first from a tuple type.
11-11: Clear union type for tracking parameters.Creating a union type
PageTrackerParamshelps unify the handling of different tracking event types, making the API more consistent and easier to use.src/api/pageview.test.ts (4)
6-6: Proper test isolation with mocking.Mocking the query function ensures tests focus on the pageview functionality without side effects from external dependencies.
11-25: Well-structured test setup.The test setup creates a realistic router configuration with base path, routes, and navigation. Mocking the router's isReady method ensures tests don't depend on actual router initialization.
27-113: Comprehensive test coverage for pageview functionality.These tests thoroughly cover:
- Different ways to call the pageview function (path string, route object)
- Various configuration options (useRouteFullPath)
- Default parameters (page_location, send_page_view)
- Title tracking and custom parameters
This extensive test suite ensures the pageview functionality works correctly in all scenarios.
115-147: Good testing of pageTracker integration.Testing the pageTracker configuration separately in its own describe block is a clean approach. The tests verify both standard path tracking and router base path integration.
src/api/query.ts (3)
1-3: LGTM on the imports.The imports correctly pull in the necessary dependencies for the module.
4-4: Good use of type inference fromGtag.Gtag.Using
Parameters<Gtag.Gtag>is a clean approach to derive the parameter types from the gtag function signature.
13-21:❓ Verification inconclusive
Verify gtag integration pattern.
The implementation follows standard gtag.js integration patterns. However, consider the following:
- Add type safety for
window[dataLayerName]to ensure it's an array- Add error handling for edge cases where window properties aren't set correctly
🏁 Script executed:
#!/bin/bash # Check how other popular GA libraries type the data layer rg -A 3 "dataLayer\s*=" --glob="*.ts"Length of output: 510
Attention: gtag Integration Pattern Review
The implementation follows the standard gtag.js integration approach. However, please consider the following improvements:
- Type Safety: Ensure that
window[dataLayerName]is explicitly typed as an array (e.g., using a check likeArray.isArray(window[dataLayerName])before assigning a default of[]).- Error Handling: Add safeguards to handle edge cases where the window properties (such as
dataLayerNameorgtagName) might not be set correctly.The output from searching the test files shows that other parts of the code (as seen in tests like in
src/add-gtag.test.ts) use the string"dataLayer"without additional type validation. This suggests that the current pattern is standard; however, the proposed enhancements would improve robustness.src/index.ts (4)
1-5: LGTM on the imports.The imports correctly pull in the necessary dependencies for the module.
15-16: Good use of TypeScript utility types.The use of
Partial<Settings>combined withRequired<Pick<Settings, "tagId">>creates a clear type that requires tagId while making other settings optional.
18-25: LGTM on the Vue plugin implementation.The implementation follows the Vue plugin pattern correctly by:
- Setting up the necessary configuration
- Returning a function that modifies the Vue app instance
- Adding the API to Vue's global properties
27-27: Good export pattern.Re-exporting from the API module is a clean way to provide a unified interface.
src/add-configuration.test.ts (3)
1-9: LGTM on the test setup.The test imports, mocking, and date fixing are properly set up. Using
mockdateensures consistent test results regardless of when they run.
10-24: LGTM on the basic configuration test.This test correctly verifies that:
- The JS command is called with the expected date
- The config command is called with the correct tagId and default settings
26-58: LGTM on the multiple domains test.This test thoroughly verifies the correct behavior with multiple accounts, ensuring:
- The main tag is configured first without a group
- Additional accounts are configured with appropriate settings
- Different configurations are correctly merged with defaults
src/add-configuration.ts (5)
1-4: LGTM on the imports.The imports correctly pull in the necessary dependencies for the module.
5-10: Good implementation of default configuration merging.The
mergeDefaultsfunction ensures consistent default behavior while preserving custom configurations. Settingsend_page_view: falseas the default aligns with best practices for GTM/GA implementations.
12-24: LGTM on function setup and early return.The function correctly:
- Extracts settings with appropriate destructuring
- Returns early if no tagId is provided, preventing errors
25-31: LGTM on linker and main tag configuration.The implementation correctly:
- Sets up the linker if provided in settings
- Initializes the GTM library with the current date
- Configures the main tag with merged default settings
32-46: LGTM on additional accounts configuration.The implementation correctly:
- Returns early if no additional accounts are provided
- Iterates through each account
- Configures each with the appropriate tag ID
- Merges default settings with account-specific settings and group name
src/add-gtag.ts (1)
6-38: Well-implemented Google Tag Manager integration functionThe
addGtagfunction follows a clear and logical flow:
- Retrieves settings
- Guards against missing tagId
- Sets up configuration
- Conditionally adds router tracking
- Handles script injection with proper error handling
The use of optional chaining for callbacks and the async/await pattern makes the code clean and maintainable.
src/index.test.ts (1)
1-37: LGTM: Concise and focused plugin installation testThe test properly verifies that:
- The plugin is installed correctly on a Vue app
- The
addGtagfunction is called during installation- The
$gtagglobal property is added with all expected methodsThe snapshot test is a good way to ensure all expected methods are present.
src/api/screenview.test.ts (1)
1-71: Comprehensive tests for screenview functionalityThe tests cover all important scenarios for the screenview functionality:
- Tracking with a string parameter
- Including app_name when configured
- Using route name from a route object
- Falling back to path when route has no name
The test setup is clean and the assertions verify the correct parameters are being passed.
src/api/screenview.ts (3)
1-4: Imports look appropriateThe imports correctly bring in the necessary dependencies for the screenview functionality.
5-10: Well-defined TypeScript typesThe type definitions are clear and appropriate. Using optional properties for
app_nameandscreen_namein theScreenviewtype and creating a union type forScreenviewParamsfollows TypeScript best practices.
29-30: Query implementation looks correctThe function correctly calls
querywith the event type "screen_view" and the constructed template.src/api/event.test.ts (5)
1-6: Test setup is correctly implementedThe imports and mock setup provide the necessary foundation for testing the event function.
7-14: Basic event functionality testThis test correctly verifies that the event function passes parameters to the query function.
16-27: Default group name testComprehensive test verifying that when additionalAccounts are present, the default group name is added.
29-41: Custom group name testWell-structured test that verifies custom group names are properly used when specified in settings.
43-58: Send_to property preservation testGood test ensuring that an explicitly provided send_to property takes precedence over the default behavior.
src/track-route.ts (5)
1-5: Imports are appropriateThe file correctly imports the necessary tracking functions, settings, and type definitions.
18-26: Early return pattern is well implementedThe function correctly gets settings and checks for exclusion before proceeding with tracking.
27-34: Template initialization is well-handledThe code correctly handles both function and object templates.
36-46: Conditional tracking based on settingsThe function properly differentiates between screenview and pageview tracking based on settings.
48-49: Lifecycle hook for tracking completionThe function properly calls the optional
onAfterTrackcallback after tracking is complete.src/utils.ts (4)
1-3: Server detection function is implemented correctlyThe
isServerfunction correctly checks for the absence of browser-specific globals.
5-16: Well-structured script injection function signatureThe function signature with typed parameters and options is clear and follows TypeScript best practices.
12-25: Early return pattern and script configurationThe function correctly handles server-side execution and configures the script element with the appropriate attributes.
27-34: Preconnect optimization is a good performance enhancementAdding preconnect functionality can improve load performance for external scripts.
src/api/index.ts (1)
1-12: Well-structured API export patternThis index file provides a clear, centralized export point for all API modules. The pattern of named exports with default imports is consistent and follows modern JavaScript/TypeScript practices.
src/add-gtag.test.ts (9)
8-12: Good test setup with appropriate mocksThe test file properly mocks all dependencies needed for isolated testing of the
addGtagfunction.
13-15: Good test organization with proper setupUsing
beforeEach(resetSettings)ensures each test starts with a clean state, which is a good testing practice.
16-33: Well-structured test for default gtag.js library downloadThis test thoroughly verifies that the correct URL is constructed and that
injectScriptis called with the appropriate parameters.
35-53: Good test coverage for custom resource URLThis test correctly verifies the behavior when a custom resource URL is provided, ensuring flexibility in the implementation.
55-66: Callback verification is well-testedThis test correctly verifies that the onReady callback is executed when the plugin is ready.
68-81: Error handling test is thoroughThe test properly mocks a rejection in
injectScriptand verifies that the error callback is triggered, which is essential for robust error handling.
83-91: Initial configuration test is concise and clearThis test verifies that
addConfigurationis called, ensuring proper initialization.
93-110: Router tracking test is comprehensiveThis test creates a real router instance and verifies that both
addRouterTrackingandaddConfigurationare called when router settings are provided.
112-116: Edge case handling is well-testedThis test verifies that
addConfigurationis not called when thetagIdis missing, ensuring the function handles this edge case appropriately.package.json (3)
39-45: Good package exports configurationThe exports configuration follows modern package.json practices and clearly defines the entry points for the package.
46-48: Simplified files arrayThe files array has been simplified to only include the "dist" directory, which is cleaner and more maintainable.
58-66: Good TypeScript dependencies addedThe addition of TypeScript-related dependencies like
@types/gtag.js,@types/node, andvite-plugin-dtssupports the transition to TypeScript.src/api/pageview.ts (3)
5-12: Clear type definitions that enhance code maintainabilityThe type definitions for
Pageview,PageviewParams, andPageviewOptionsare well-structured and provide good type safety for the function parameters.
14-19: Well-implemented path normalization helperThe
getPathWithBasefunction correctly handles the normalization of base and path components, ensuring proper path construction regardless of trailing/leading slashes.
47-53: Good default value handlingThe function correctly checks for
undefinedvalues rather than falsy values, which is a good TypeScript practice and prevents unexpected behavior with valid falsy values.src/track-route.test.ts (5)
1-7: Well organized test imports and mocking setupThe imports are well organized, and the API query module is properly mocked. This ensures isolation of the test from actual network calls.
8-34: Good test suite setup with proper router configurationThe test setup creates a clean environment for each test by resetting settings and creating a new router with two routes. The router's
isReadymethod is properly mocked to avoid waiting for actual router initialization.
35-53: Comprehensive screenview tracking testThe test properly validates that screenview tracking works as expected, with all expected parameters being passed to the query function.
101-135: Thorough callback testingThe tests correctly verify that both before and after tracking callbacks are invoked with the current route.
137-179: Comprehensive template testingTests for both static templates and function-based templates are properly implemented and verify that the correct parameters are passed to the query function.
src/settings.ts (7)
1-3: Good type importsImporting specific types from other modules shows good modular design and type safety practices.
4-9: Well-defined ConfigParams typeThe ConfigParams union type provides a clear interface for configuration parameters that can be passed to Gtag.
10-13: Flexible PageTrackerTemplate typeThe PageTrackerTemplate type allows for both static and dynamic template definitions, providing good flexibility for developers.
14-22: Comprehensive PageTracker type definitionThe PageTracker type includes all necessary configurations for route tracking, including exclusion options and callbacks.
24-38: Well-structured Settings typeThe Settings type provides a comprehensive configuration interface with sensible optional and required properties.
40-47: Properly defined defaultSettingsThe defaultSettings object is correctly marked as Readonly to prevent accidental modifications, and it provides sensible defaults for required properties.
51-55: Simple and effective settings access functionsThe getSettings and resetSettings functions provide a clean API for accessing and resetting settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/api/ecommerce.ts (1)
1-12: Looks good, but JSDoc could be more comprehensive.The implementation is clean and follows good TypeScript practices with proper typing. The function correctly delegates to the generic
eventfunction.However, the JSDoc comment specifically references "refund" events while the function handles all e-commerce events. Consider expanding the documentation to cover the broader scope of e-commerce events.
/** - * @see https://developers.google.com/analytics/devguides/collection/ga4/reference/events?client_type=gtag#refund + * E-commerce event tracking for GA4 + * @see https://developers.google.com/analytics/devguides/collection/ga4/reference/events?client_type=gtag#ecommerce */src/api/event.ts (3)
5-5: Consider moving EventParams type to the types module.The
EventParamstype is defined inline, which is inconsistent with the pattern used forEventNamesthat's imported from a dedicated types file. For better code organization and maintainability, consider moving this type definition to thetypes/event.tsfile.
7-15: Function modifies input parameters directly.The function directly modifies the
paramsobject by assigning toparams.send_to. This mutation could lead to unexpected behavior for callers if they use the same object elsewhere.export default function event(name: EventNames, params: EventParams) { const { groupName, additionalAccounts } = getSettings(); + const eventParams = { ...params }; if (params.send_to === undefined && additionalAccounts?.length) { - params.send_to = groupName; + eventParams.send_to = groupName; } - query("event", name, params); + query("event", name, eventParams); }
10-12: Conditional check could be more explicit.The conditional check
additionalAccounts?.lengthworks but could be more explicit about the intent to check if the array has elements.- if (params.send_to === undefined && additionalAccounts?.length) { + if (params.send_to === undefined && additionalAccounts && additionalAccounts.length > 0) {src/api/ecommerce.test.ts (1)
6-16: Test coverage could be expanded.The current test covers only the "refund" event type with minimal parameters. Consider adding tests for:
- Other e-commerce event types (purchase, view_item, etc.)
- More complex parameter scenarios (items array, additional properties)
- Edge cases and error handling
This would provide more comprehensive validation of the functionality.
describe("ecommerce", () => { it("should use the ecommerce event", () => { ecommerce("refund", { transaction_id: "transaction_id_value", }); expect(query).toHaveBeenCalledWith("event", "refund", { transaction_id: "transaction_id_value", }); }); + + it("should handle purchase events with items", () => { + ecommerce("purchase", { + transaction_id: "transaction_id_value", + currency: "USD", + value: 123.45, + items: [ + { + item_id: "item1", + item_name: "Test Item", + price: 123.45, + quantity: 1 + } + ] + }); + + expect(query).toHaveBeenCalledWith("event", "purchase", { + transaction_id: "transaction_id_value", + currency: "USD", + value: 123.45, + items: [ + { + item_id: "item1", + item_name: "Test Item", + price: 123.45, + quantity: 1 + } + ] + }); + }); });src/types.ts (1)
13-16: Consider stricter typing for CustomParamsWhile the
anytype is sometimes necessary, consider if you could use a more specific type likeunknownor a union of expected types to improve type safety while still allowing flexibility.export type CustomParams = { // biome-ignore lint/suspicious/noExplicitAny: - [key: string]: any; + [key: string]: string | number | boolean | null | undefined | object; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/api/ecommerce.test.ts(1 hunks)src/api/ecommerce.ts(1 hunks)src/api/event.ts(1 hunks)src/api/index.ts(1 hunks)src/api/set.test.ts(1 hunks)src/index.test.ts(1 hunks)src/types.ts(1 hunks)src/types/ecommerce.ts(1 hunks)src/types/event.ts(1 hunks)vite.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/api/set.test.ts
- src/index.test.ts
🔇 Additional comments (14)
vite.config.js (6)
3-3: Good addition of the DTS plugin for TypeScript support.Adding the
vite-plugin-dtsplugin is appropriate for generating TypeScript declaration files, which is essential for the TypeScript migration mentioned in the PR objectives.
9-9: Correctly updated entry point to TypeScript.The change from a JavaScript to TypeScript entry point aligns with the TypeScript rewrite mentioned in the PR objectives.
19-19: Standard Vue global name correction.Changing from lowercase "vue" to capitalized "Vue" follows standard conventions for global variable naming in the Vue ecosystem.
24-29: Good TypeScript integration configuration.The DTS plugin configuration is appropriate for generating TypeScript declaration files:
insertTypesEntry: trueproperly adds types entry to package.jsonrollupTypes: trueensures bundling of type definitionsThis will help TypeScript users get proper type support when using the library.
33-36: Improved test configuration with coverage reporting.Adding
mockReset: trueand coverage reporting with text and HTML outputs enhances the testing framework capabilities and provides better feedback on test coverage.
11-12:✅ Verification successful
Verify impact of formats reduction to ESM-only.
The formats have been simplified to only include "es" (ESM), removing "umd" and "cjs" formats that may have been present before. While this aligns with the "esm-only build" objective, ensure this change won't break backward compatibility for users relying on UMD or CommonJS.
🏁 Script executed:
#!/bin/bash # Check if the package.json specifies module type and if there's any documentation about the format change cat package.json | grep -E '"type"|"main"|"module"' grep -r "require\(.*vue-gtag" --include="*.js" .Length of output: 199
Below is an updated verification step to gather the necessary information. It appears the previous grep command failed due to improper escaping of the parentheses. To verify if any CommonJS (i.e.
require("vue-gtag") usage still exists in the repo (which would indicate users expecting UMD/CJS support), please run the following shell script:Once you review the output of this command:
- If there are no hits, it indicates that there’s no evidence of legacy usage in the codebase, so the removal of UMD/CJS formats should not affect any consumers.
- If there are matches, then please verify if these consumers need continued support for UMD or CommonJS builds.
After this verification, you can update the review comment accordingly.
🏁 Script executed:
#!/bin/bash # Search for CommonJS usage of vue-gtag within JavaScript files using ripgrep rg 'require\("vue-gtag' .Length of output: 27
ESM-Only Build Confirmed – No Legacy Usage Found
- The package configuration shows
"type": "module"and the main export is set to the ESM file ("./dist/vue-gtag.js").- A search for CommonJS usage (
require("vue-gtag") did not return any results, indicating there’s no legacy dependency on UMD or CommonJS formats.Based on these findings, the reduction to an ESM-only build should not affect backward compatibility.
src/types.ts (4)
1-7: Good imports and exports from vue-routerThe file correctly imports specific types from internal modules and re-exports types from vue-router with appropriate naming. This provides a centralized type system for the rest of the application.
9-9: Well-designed utility type for tuple manipulationThe
Tail<T>type utility is correctly implemented to extract all elements of a tuple except the first one, which is useful for parameter handling in function calls.
11-11: Good union type for page tracking parametersDefining
PageTrackerParamsas a union ofPageview | Screenviewcreates a clean API for page tracking functions that can handle both types of views.
18-18: Good pattern for extensible parametersThe
ParamsWithCustom<T>type provides a clean way to allow both strictly typed parameters and custom parameters, which is valuable for a library that needs to be flexible.src/api/index.ts (1)
1-11: Well-organized API exportsThe barrel file pattern used here provides a clean, centralized way to export all API modules. This makes imports more manageable throughout the codebase and follows best practices for module organization.
src/types/ecommerce.ts (3)
1-15: Comprehensive ecommerce event typesThe
EcommerceEventNametype provides a complete set of GA4 ecommerce event names, which ensures type safety when tracking these events. This will help prevent typos and provide autocomplete suggestions.
17-92: Excellent documentation and typing for ecommerce itemsThe
EcommerceItemtype is well-documented with comprehensive JSDoc comments that explain each field's purpose. The type definition closely follows GA4 ecommerce parameters specifications, which will help ensure proper tracking implementation.
94-131: Well-structured ecommerce parameters with clear documentationThe
EcommerceParamstype properly defines all the necessary fields for ecommerce event tracking with detailed JSDoc comments. The cross-references to item-level parameters (with explanations of precedence) are particularly helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tsconfig.json (1)
29-33: Declaration Files & Output Directories:
Declaration file generation is enabled and configured to output to"./dist". However, since"noEmit": trueis specified earlier, TypeScript won’t produce any output by default. Make sure this is intentional—if you intend to generate declaration files, you might need to adjust the build process or add"emitDeclarationOnly": truein a dedicated build configuration.src/utils.ts (3)
5-15: Improve Promise handling for server-side executionThe early return in the server-side case doesn't properly resolve the Promise.
export async function injectScript( url: string, options?: { preconnectOrigin?: boolean; defer?: boolean; }, ): Promise<void> { return new Promise((resolve, reject) => { if (isServer()) { - return; + return resolve(); }
27-36: Add error handling for URL parsing during preconnectWhen creating a preconnect link, the code assumes the URL is valid and can be parsed by the URL constructor, which could throw an error with invalid URLs.
if (options?.preconnectOrigin) { const link = document.createElement("link"); + try { const resource = new URL(url); link.href = resource.origin; link.rel = "preconnect"; head.appendChild(link); + } catch (error) { + console.warn("Failed to parse URL for preconnect:", error); + } }
38-42: Consider adding cleanup functionalityThe script elements added to the DOM will remain there permanently. For components or modules that might be mounted and unmounted multiple times, consider adding a way to clean up these elements.
src/settings.ts (2)
47-56: Consider using environment variables for the resource URLThe default Google Tag Manager URL is hardcoded. For greater flexibility, consider making this configurable via environment variables.
const defaultSettings: Readonly<Settings> = { resource: { - url: "https://www.googletagmanager.com/gtag/js", + url: import.meta.env?.VITE_GTAG_URL || "https://www.googletagmanager.com/gtag/js", preconnect: false, deferred: false, }, // ... };
58-58: Consider using a more immutable approach for settings managementThe use of a mutable variable for settings could lead to unexpected behavior in certain scenarios, especially in concurrent environments.
You might consider a more functional approach that doesn't rely on a mutable global variable, perhaps using a settings manager class or a reactive state system depending on your framework.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
package.json(4 hunks)src/add-gtag.test.ts(1 hunks)src/add-gtag.ts(1 hunks)src/api/ecommerce.ts(1 hunks)src/api/query.ts(1 hunks)src/index.ts(1 hunks)src/settings.test.ts(1 hunks)src/settings.ts(1 hunks)src/types/event.ts(1 hunks)src/utils.test.ts(1 hunks)src/utils.ts(1 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/utils.test.ts
- src/api/ecommerce.ts
- src/settings.test.ts
- src/api/query.ts
- src/types/event.ts
- src/add-gtag.ts
- src/add-gtag.test.ts
- src/index.ts
- package.json
🔇 Additional comments (13)
tsconfig.json (7)
1-8: Compiler Options & Target Declaration:
The configuration correctly targets ES2020 and sets the module system to ESNext. Using"useDefineForClassFields": trueensures that class field semantics are aligned with the latest ECMAScript specifications. The specified libraries ("ES2020","DOM", and"DOM.Iterable") are appropriate for modern web applications.
9-15: Bundler Mode Configuration:
The settings under the "Bundler mode" comment are well defined."moduleResolution": "bundler", along with enabling TypeScript extensions for imports, JSON module resolution, and isolated modules, supports an ESM-only build effectively. Note that"noEmit": truemeans TypeScript will only perform type checking without producing output, so verify that this aligns with your build and release strategy (e.g., if a separate tool is used for emitting declarations).
16-21: Strict Type Checking Enforcement:
Enabling"strict","noUnusedLocals","noUnusedParameters", and"noFallthroughCasesInSwitch"promotes robust and error-free code. These options are recommended to catch potential issues early during development.
22-25: Vue-Specific JSX Settings:
The"jsx": "preserve"and"jsxImportSource": "vue"settings are correctly configured to support Vue’s JSX transformation. Ensure that your Vue version supports these options or consider updating if needed.
26-29: Path Aliases Configuration:
Setting"baseUrl": "."helps with resolving modules relative to the project root. This simple aliasing works well if your directory structure is straightforward. If needed, you might later extend this with additional aliases for deeper directory structures.
34-38: Additional Checks and Type Definitions:
The options"forceConsistentCasingInFileNames": trueand"esModuleInterop": trueare standard best practices to avoid common module and casing issues. Including types for"@types/gtag.js","@types/node", and"vitest/globals"ensures that your project has access to the necessary global type declarations for these libraries.
40-42: File Inclusion and Exclusion Settings:
The"include"and"exclude"settings target the TypeScript source (.tsand.d.ts) and properly exclude"node_modules"and"dist". This configuration helps keep the compilation focused on your source files.src/utils.ts (3)
1-3: Good server detection implementationThe
isServerutility function correctly checks for bothwindowanddocumentto determine server-side rendering, which is more robust than just checking for one of them.
49-51: Good null check and array check in isObject functionThe
isObjectfunction correctly checks for null, type, and also ensures the value is not an array, which is important for accurate deep merging.
53-73: Robust deep merge implementationThe
deepMergefunction is well-implemented:
- Creates a new object rather than mutating the target
- Uses
Object.prototype.hasOwnProperty.callfor security- Correctly handles nested objects with recursive merging
- Uses proper TypeScript generics for type safety
This is good for handling nested configuration settings.
src/settings.ts (3)
1-4: Good organization with explicit importsThe imports are clearly organized, importing types from relevant modules and the
deepMergeutility from the utils file we just reviewed.
5-45: Well-structured type definitions with appropriate optionalityThe type definitions are comprehensive and provide good structure for the configuration:
ConfigParamsuses the union of Gtag typesPageTrackerTemplateallows for both static params and dynamic functionsSettingsclearly marks which properties are optional vs requiredThis will help catch configuration errors at compile time.
66-68: Good implementation using deep mergeYou've addressed the previous review comment by using the
deepMergeutility function from utils.ts, which ensures that nested objects are properly merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/add-gtag.test.ts (2)
20-37: Consider adding call count assertions.The test verifies that
injectScriptis called with the correct URL parameters, which is good. For additional precision, consider adding an assertion for the number of times it was called.expect(utils.injectScript).toHaveBeenCalledWith(url, { preconnectOrigin: preconnect, defer: false, }); + expect(utils.injectScript).toHaveBeenCalledTimes(1);
117-121: Consider additional verification for missing tagId case.While the test correctly verifies that
addConfigurationis not called when the tagId is missing, it would be beneficial to also check thatinjectScriptis not called in this scenario.it("should not bootstrap gtag if tagId is missing", async () => { await addGtag(); expect(addConfiguration).not.toHaveBeenCalled(); + expect(utils.injectScript).not.toHaveBeenCalled(); });src/settings.ts (2)
49-58: Consider revisiting default resource values.The default resource config sets
preconnectanddeferredtofalse. If users typically benefit from loading analytics scripts proactively, consider setting them totrueor exposing that as a more visible config option. This might improve performance for many use cases.
60-70: Guard against concurrency or SSR usage.
settingsis a mutable global variable. If your library is used in server-side rendering or concurrent environments, shared mutable state could lead to requests overriding each other’s configurations. Consider externalizing or scoping these settings per request or instance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package.json(4 hunks)src/add-gtag.test.ts(1 hunks)src/add-gtag.ts(1 hunks)src/settings.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/add-gtag.ts
- package.json
🔇 Additional comments (12)
src/add-gtag.test.ts (10)
1-16: Imports and mocks setup looks good.The imports and mocks are well-organized. The module setup properly isolates the testing of
addGtagfrom its dependencies.
17-19: Good test setup with proper isolation.Using
beforeEach(resetSettings)ensures that each test starts with a clean state, which is essential for test isolation.
39-58: Test for custom resource URL is comprehensive.The test correctly verifies that a custom resource URL is used when specified in the settings.
60-71: Callback test is well-structured.The test for the
onReadycallback properly verifies that the callback is fired when the plugin is ready.
73-86: Error handling test is thorough.Good job on testing the error case by mocking
injectScriptto reject with an error and verifying that the error callback is called.
88-96: Initial configuration test is clear.The test correctly verifies that
addConfigurationis called after invokingaddGtag.
98-115: Router tracking test is comprehensive.The test properly creates a router instance and verifies that router tracking is initialized when a router is provided in the settings.
123-132: Custom script test is well-implemented.The test correctly verifies that the script is not injected when
useCustomScriptis set to true.
134-146: OnReady with custom script test is thorough.The test correctly verifies that the
onReadycallback is fired even when using a custom script loader.
1-147: Overall test coverage is excellent.The test suite comprehensively covers the functionality of the
addGtagfunction, including script loading, configuration, router tracking, error handling, and callback execution. The tests are well-organized and provide good coverage of various scenarios.src/settings.ts (2)
5-47: Well-structured type definitions.Your type definitions for
ConfigParams,PageTrackerTemplate,PageTracker,Resource, andSettingsare clearly laid out. They enhance readability and help ensure type safety across the project. Great job!
1-3:✅ Verification successful
Confirm presence of Gtag type definitions.
You're referencing Gtag.* types, such as
Gtag.ControlParamsandGtag.EventParams. Please ensure they are installed or declared in your type definitions (e.g., via@types/gtag.js) to avoid potential TypeScript errors.If necessary, you can run a script to confirm whether
@types/gtag.js(or comparable definitions) are declared in your package.json:
🏁 Script executed:
#!/bin/bash rg -A 3 '"@types/gtag' package.jsonLength of output: 37
I'm not seeing any references to
"@types/gtag"in your package.json, which raises the possibility that the type definitions forGtagmight be declared locally or via another dependency. To gather more clarity on this, please run the following shell script to search the repository for any custom type declarations forGtag:Once you review the output, please verify if the necessary
Gtag.*type definitions (such asGtag.ControlParamsandGtag.EventParams) are present. If they’re not found, you may need to install@types/gtag.jsor declare them manually to avoid potential TypeScript errors.
🏁 Script executed:
#!/bin/bash # Search for any declaration of the Gtag namespace within the repository, which would confirm local or custom type definitions. rg -n "declare namespace Gtag" .Length of output: 77
Gtag type definitions confirmed
The custom declaration for the
Gtagnamespace is present insrc/gtag.d.ts(line 2). This confirms thatGtag.ControlParams,Gtag.EventParams, and other related types are properly defined. No further changes are needed regarding the inclusion of@types/gtag.jsin your package.json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/settings.ts (1)
32-48: Consider freezing certain properties inSettingsif immutability is desired.While this design works, you could optionally use
Readonlytypes for properties likeresourceorpageTrackerif you want to prevent accidental mutations. That said, the current approach is functional enough if mutability is an intentional choice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/api/config.ts(1 hunks)src/api/pageview.ts(1 hunks)src/api/screenview.ts(1 hunks)src/settings.ts(1 hunks)src/track-route.ts(1 hunks)src/types/page-tracker.ts(1 hunks)src/types/router.ts(1 hunks)src/types/utils.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/types/page-tracker.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/api/config.ts
- src/track-route.ts
- src/api/screenview.ts
🔇 Additional comments (11)
src/settings.ts (5)
1-4: Imports look consistent.All imported types and the
deepMergeutility are utilized properly. Good job keeping imports scoped and clear.
6-30: Type definitions appear comprehensive.The union types for
ConfigParamsandPageTrackerTemplateencapsulate their possible shapes, andPageTrackerandResourcefields are clearly documented. This structure helps maintain clarity for multiple usage scenarios.
50-60: Default settings are well-defined.Providing default values for key fields promotes consistency and reduces boilerplate.
61-67: Settings accessors are straightforward and clean.The
getSettingsandresetSettingsfunctions are intuitive: they promote strong maintainability by exposing clear read/reset behavior for global settings.
69-71: Usage ofdeepMergeensures nested objects are handled correctly.The decision to leverage a deep merge function addresses the issue of nested object updates that would be lost with a shallow merge.
src/types/utils.ts (1)
1-1: LGTM! Good utility type for tuple manipulation.The
Tail<T>type is a well-implemented utility type that extracts all elements except the first from a tuple type. This pattern is useful when working with function arguments or implementing functional programming concepts in TypeScript.src/types/router.ts (1)
1-4: LGTM! Clean type exports from vue-router.This is a clean approach to re-exporting and renaming types from vue-router, which centralizes router type imports and simplifies the type names for use throughout the application.
src/api/pageview.ts (4)
5-7: LGTM! Clear and meaningful type definitions.These types provide a flexible API for the pageview function, allowing users to pass a simple string path, a Route object, or a full Gtag config object.
9-12: LGTM! Well-defined options interface.The options interface provides good flexibility for controlling how the page path is constructed.
14-19: LGTM! Path normalization helper function.This utility function handles path normalization correctly, ensuring that the base and path are properly joined regardless of whether they include trailing/leading slashes.
47-53: LGTM! Good defaults for page tracking.Setting default values for
page_locationandsend_page_viewensures consistent tracking behavior even when these values aren't explicitly provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/api/opt.ts (1)
3-11: Consider adding type safety for window property accessThe dynamic property access on the window object could benefit from better TypeScript type safety.
+// Add a type declaration to improve type safety +declare global { + interface Window { + [key: `ga-disable-${string}`]: boolean | undefined; + } +} export function optOut(tagId?: TagId) { const { tagId: settingsTagId } = getSettings(); window[`ga-disable-${tagId ?? settingsTagId}`] = true; } export function optIn(tagId?: TagId) { const { tagId: settingsTagId } = getSettings(); window[`ga-disable-${tagId ?? settingsTagId}`] = undefined; }src/api/opt.test.ts (1)
4-43: Consider adding tests for edge casesWhile the current tests cover the basic functionality well, consider adding tests for:
- Invalid tag IDs
- Behavior when settings are missing or incomplete
- Multiple consecutive calls to optIn/optOut
it("should handle missing settings gracefully", () => { // Test with empty or undefined settings updateSettings({}); optOut(); // Verify behavior }); it("should handle consecutive calls", () => { updateSettings({ tagId: "UA-1" }); optOut(); optOut(); // Call again expect(window["ga-disable-UA-1"]).toBe(true); optIn(); optIn(); // Call again expect(window["ga-disable-UA-1"]).toBeUndefined(); });src/api/index.ts (1)
12-12: Different export style for opt moduleWhile other modules use default exports, the opt module uses named exports. This is not necessarily a problem, but it creates an inconsistency in the API design.
Consider either:
- Changing opt.ts to use a default export for consistency, or
- Documenting why this module uses a different export style
- export { optIn, optOut } from "./opt"; + export { default as opt } from "./opt";This would require changing opt.ts to:
export default { optIn, optOut };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
src/api/index.ts(1 hunks)src/api/opt.test.ts(1 hunks)src/api/opt.ts(1 hunks)src/index.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.test.ts
🔇 Additional comments (6)
src/api/opt.ts (2)
1-6: Implement optOut function correctly to disable Google Analytics trackingThe implementation follows Google's standard approach for disabling tracking via the
window["ga-disable-{tag}"]property. The function properly handles both explicit tag IDs and fallback to settings.
8-11: Implement optIn function correctly to re-enable trackingThe implementation correctly sets the window property to
undefinedto re-enable tracking. This approach follows Google Analytics conventions for opting users back into tracking.src/api/opt.test.ts (3)
4-17: Tests for optOut functionality look goodThe test properly validates that when
optOut()is called, the appropriate window property is set to true, correctly disabling tracking for the configured tag ID.
19-28: Tests for optIn functionality look goodThe test correctly verifies that after opting out and then opting in, the tracking property is reset to undefined, re-enabling tracking.
30-43: Tests for custom tagId functionality look goodThe nested test suite properly verifies the behavior with explicit tagId parameters for both optOut and optIn functions.
src/api/index.ts (1)
1-11: Well-organized exports for API modulesThe index file provides a clean, centralized point of access for all API functionality, making imports easier in consuming code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/api/opt.ts (1)
6-6: Consider adding SSR safety check.Direct window access could cause issues during server-side rendering if window is undefined.
- window[`ga-disable-${tagId ?? settingsTagId}`] = value; + if (typeof window !== 'undefined') { + window[`ga-disable-${tagId ?? settingsTagId}`] = value; + }src/api/opt.test.ts (1)
47-73: Consider adding a test for mixed account handling.Add a test case where additionalAccounts are configured but a specific tagId is provided to the opt functions, verifying that only the specified account is affected.
it("should only opt out the specified account when tagId is provided", () => { optOut("UA-1"); expect(window["ga-disable-UA-1"]).toBe(true); expect(window["ga-disable-UA-2"]).toBeUndefined(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/api/opt.test.ts(1 hunks)src/api/opt.ts(1 hunks)
🔇 Additional comments (8)
src/api/opt.ts (4)
3-15: Well-structured helper function for property assignment.The
assignPropertyfunction appropriately handles both single tagId and additional accounts scenarios.
8-10: Verify the early return condition.The function returns early if there are no additional accounts OR a specific tagId was provided. Make sure this is the intended behavior.
17-25: Well-documented optOut implementation.The function correctly implements Google Analytics opt-out mechanism by setting the
ga-disable-{tagId}property to true.
27-35: Correct implementation of optIn functionality.The function properly uses
undefinedrather thanfalseto enable tracking, which aligns with Google Analytics documentation.src/api/opt.test.ts (4)
4-9: Good test setup with proper state reset.The beforeEach hook correctly resets settings and window properties before each test, preventing test pollution.
11-30: Comprehensive tests for basic opt functionality.Tests cover both optOut and optIn behavior with the default tagId from settings.
32-45: Good testing of custom tagId scenario.Tests verify that opt functions work correctly when a specific tagId is provided.
47-73: Well-structured tests for additional accounts.Tests confirm that all accounts are properly opted in/out when using additional accounts configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/add-router.tracking.test.ts (2)
1-1: Consider filename consistency.The filename
add-router.tracking.test.tsuses a dot, while the imported module isadd-router-tracking. Consider standardizing to either hyphen-based or dot-based naming for consistency across the codebase.
13-130: Consider adding tests for edge cases.The current tests cover the main scenarios well, but consider adding tests for these edge cases:
- When
pageTracker.routeris not set (which should trigger an early return inaddRouterTracking)- Error handling if
router.isReady()rejectsit("should not track when router is not set", async () => { updateSettings({ pageTracker: {} }); await addRouterTracking(); expect(trackRoute).not.toHaveBeenCalled(); }); it("should handle router ready rejection", async () => { const router = createRouter({ history: createWebHistory(), routes, }); vi.spyOn(router, "isReady").mockRejectedValue(new Error("Router not ready")); updateSettings({ pageTracker: { router } }); await expect(addRouterTracking()).rejects.toThrow("Router not ready"); expect(trackRoute).not.toHaveBeenCalled(); });src/types/settings.ts (1)
15-32: Fix typos in JSDoc commentsThere are two typos in the comment for
skipSamePath:- /** Avoid traking route changes triggerd by the same path */ + /** Avoid tracking route changes triggered by the same path */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
package.json(4 hunks)src/add-configuration.ts(1 hunks)src/add-router-tracking.ts(1 hunks)src/add-router.tracking.test.ts(1 hunks)src/api/opt.ts(1 hunks)src/index.ts(1 hunks)src/settings.ts(1 hunks)src/types/settings.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/api/opt.ts
- src/add-configuration.ts
- src/add-router-tracking.ts
- src/index.ts
- package.json
🔇 Additional comments (13)
src/add-router.tracking.test.ts (3)
1-131: Well-structured test suite for router tracking functionality.The tests comprehensively cover the main router tracking scenarios and demonstrate good testing practices with proper isolation and mocking. The implementation aligns well with the PR objective of rewriting the codebase in TypeScript.
57-92: Good coverage of identical paths tracking.This test correctly verifies that identical paths are tracked by default, which ensures that the
skipSamePathoption works as intended when enabled. The test assertions are precise and thorough.
94-129: Thorough testing of the skipSamePath option.The test properly verifies that when
skipSamePathis enabled, identical consecutive paths are not tracked multiple times. This is an important configuration option that impacts user analytics data quality.src/types/settings.ts (5)
1-4: Good choice of type imports for settingsThe imports are clean and specific, pulling in only the necessary types from other modules.
5-9: Clean type definition for ConfigParamsThe union type effectively aggregates the various Gtag parameter types, making the configuration API more flexible.
11-13: Well-designed PageTrackerTemplate typeThis type allows for both static parameter objects and dynamic functions that generate parameters based on routes, providing flexibility in how tracking is configured.
34-41: Well-structured Resource type with clear defaultsThe Resource type is cleanly defined with JSDoc comments indicating default values, which helps developers understand the configuration options.
43-70: Comprehensive Settings type with good documentationThe Settings type is well-structured with thorough JSDoc comments. The optional/required properties are appropriately marked, allowing for flexible configuration while ensuring essential properties are provided.
src/settings.ts (5)
1-3: Good imports with specific type referencesThe imports are clean and specific, pulling in only what's needed for the module.
4-13: Well-defined default settings with sensible valuesThe default settings are defined as a readonly object, which is a good practice to prevent accidental modification.
15-15: Settings initialization looks goodProperly initializes the settings with the default values.
17-25: Excellent implementation of deepMerge in updateSettingsThe implementation now uses
deepMergeinstead of a shallow merge, which correctly handles nested object properties as suggested in the previous review.
15-25:❓ Verification inconclusive
Consider implications of mutable global state
The current implementation uses a mutable global
settingsvariable. While common for configuration management, this approach can create challenges for testing and potential race conditions in concurrent scenarios.Consider alternatives:
- A more immutable approach returning new settings objects
- A configuration context pattern if using React
- At minimum, ensuring thread safety for concurrent access
🏁 Script executed:
#!/bin/bash # Check for concurrent access to settings in the codebase # Look for direct assignments to settings variable echo "Checking for direct assignments to settings variable..." rg "settings\s*=" --glob "*.ts" --glob "*.js" -A 1 -B 1 # Look for potential concurrent access patterns echo "Checking for potential concurrent access patterns..." rg "updateSettings" --glob "*.ts" --glob "*.js" -A 3 -B 3 # Look for async operations that might access settings echo "Checking for async operations that might access settings..." rg "(setTimeout|setInterval|Promise).+getSettings" --glob "*.ts" --glob "*.js"Length of output: 16586
Action Required: Revisit Global Settings Mutation Pattern
The current design uses a mutable global variable for settings (with direct assignments in both
resetSettingsandupdateSettings). Our verification across the codebase confirms that this pattern is consistently used (as seen in multiple test files) and no asynchronous access togetSettingswas detected. However, the mutable state design still bears potential risks:
- While the synchronous usage today keeps race conditions at bay, future asynchronous or concurrent operations might expose thread-safety issues.
- Consider refactoring to return new settings objects (an immutable approach) to mitigate these risks.
- If using a UI framework like React, adopting a context-based pattern could provide a more robust configuration management solution.
Please verify that current usage aligns with the expected application context and address potential concurrency concerns if the operational profile changes in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/types/settings.ts (2)
24-25: Fix typo in JSDoc comment.There's a typo in the comment: "traking" should be "tracking".
- /** Avoid traking route changes triggerd by the same path */ + /** Avoid tracking route changes triggered by the same path */
57-58: Consider making resource optional with internal defaults.The
resourceproperty is currently required, but since your JSDoc comments indicate default values for its properties, you might want to make the entireresourceproperty optional and handle defaults internally.- /** gtag.js loader configuration */ - resource: Resource; + /** gtag.js loader configuration */ + resource?: Resource;src/utils.ts (1)
58-78: Recursive deep merge implementationThe deepMerge function correctly implements a recursive object merge strategy, properly handling nested objects.
Consider adding protection against circular references and maximum recursion depth to prevent potential stack overflow errors with very deeply nested objects:
+ const MAX_DEPTH = 100; + + export function deepMerge<T extends DeepMergeable, U extends DeepMergeable>( + target: T, + source: U, + depth = 0, + seen = new WeakMap() + ): T & U { + if (depth > MAX_DEPTH) { + console.warn('Maximum deepMerge depth exceeded'); + return { ...target, ...source } as T & U; + } + + // Detect circular references + if (isObject(source)) { + if (seen.has(source)) { + return seen.get(source); + } + seen.set(source, null); + } const output: DeepMergeable = { ...target }; for (const key in source) { if (Object.prototype.hasOwnProperty.call(source, key)) { const sourceValue = source[key]; const targetValue = target[key]; if (isObject(sourceValue) && isObject(targetValue)) { - output[key] = deepMerge(targetValue, sourceValue); + output[key] = deepMerge(targetValue, sourceValue, depth + 1, seen); } else { output[key] = sourceValue; } } } + if (isObject(source)) { + seen.set(source, output); + } + return output as T & U; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/add-gtag.test.ts(1 hunks)src/add-gtag.ts(1 hunks)src/types/settings.ts(1 hunks)src/utils.test.ts(1 hunks)src/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/utils.test.ts
- src/add-gtag.ts
🔇 Additional comments (23)
src/types/settings.ts (6)
50-75: Review required vs. optional properties in Settings type.In the
Settingstype,resource,dataLayerName, andgroupNameare required properties whiletagIdis optional. Please verify if this is intentional. Typically, the tag ID would be required for Google Analytics configuration, while other properties might be optional with default values.Also, consider providing sensible defaults for the required properties to make the library easier to use.
1-4: The imports are well-organized and clearly indicate dependencies.The file properly imports the necessary types from related modules.
11-14: Good flexibility with PageTrackerTemplate type.The union type allows for both static and dynamic generation of page tracking parameters, which provides good flexibility to users.
15-32: Well-documented PageTracker type with comprehensive options.The PageTracker type includes a good set of configuration options with clear JSDoc comments. The callbacks for before and after tracking are particularly useful for advanced use cases.
34-46: Well-structured Resource type with security considerations.The Resource type includes important properties like
noncefor Content Security Policy, which is a good security practice.
5-9:✅ Verification successful
Check Gtag namespace import.
The
Gtagnamespace is used in theConfigParamstype but doesn't appear to be imported anywhere in this file. Make sure the namespace is properly defined or imported in your project.
🏁 Script executed:
#!/bin/bash # Check for Gtag namespace definition echo "Searching for Gtag namespace definition" rg "namespace Gtag" --type ts echo -e "\nSearching for Gtag type imports" rg "import.+Gtag" --type tsLength of output: 476
Gtag namespace is properly defined globally
The
Gtagnamespace is declared in the ambient type file (src/gtag.d.ts), so an explicit import insrc/types/settings.tsisn’t necessary. The namespace declaration is being picked up by the TypeScript compiler, which makes the current usage inConfigParamsvalid.src/add-gtag.test.ts (11)
1-16: Nicely structured test file setupThe imports and mocks are properly organized. Using
vi.mockwithimportActualfor the utils module ensures you can mock specific functions while keeping the real implementation of others.
17-19: Good test organization with proper test setupUsing
beforeEach(resetSettings)ensures each test starts with a clean state, which is essential for test isolation.
20-37: Well-implemented test for basic gtag.js library loadingThis test verifies that the library is downloaded with the correct URL parameters when using default configuration.
39-58: Properly tests custom resource configurationThe test correctly verifies that a custom resource URL is respected when specified in settings.
60-71: Callback validation is properly testedThe test verifies that the onReady callback is executed when the plugin is loaded.
73-86: Good error handling testThis test correctly mocks a rejection scenario and verifies that the onError callback is triggered when script loading fails.
88-96: Confirms initial configuration is calledThe test verifies that addConfiguration is called during initialization, which is important for establishing the base configuration.
98-115: Router tracking initialization is properly testedThis test ensures that router tracking is initialized when a router instance is provided in the settings.
117-121: Appropriate validation for missing tagIdThe test confirms that addConfiguration is not called when tagId is missing, which prevents invalid setup.
123-146: Tests for custom script scenarios are comprehensiveThese tests verify both that the script is not injected when useCustomScript is true and that the onReady callback is still triggered in this scenario.
148-165: Security feature tested properlyThe test verifies that the nonce attribute is correctly passed to the injectScript function when specified in settings, which is important for Content Security Policy compliance.
src/utils.ts (6)
1-3: Simple and effective server detectionThe isServer function provides a straightforward way to check if code is running in a server environment.
5-16: Well-typed script injection functionThe function signature with TypeScript typing is clear and provides good documentation of the available options.
18-31: Script element creation is well implementedThe code properly creates and configures a script element with the necessary attributes based on the provided options.
32-42: Resource preconnection is a good performance optimizationCreating a preconnect link for the script's origin can improve loading performance, especially on slower connections.
43-47: Script loading events are properly handledThe code correctly attaches onload and onerror handlers to resolve or reject the promise.
50-56: Good type definition for deepMergeThe DeepMergeable type and isObject helper function are well implemented for the deepMerge functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/add-router-tracking.test.ts (1)
13-92: Consider adding a test for the early return case.The test suite is comprehensive for the happy path scenarios, but missing a test for when
pageTracker.routeris undefined, which would trigger the early return in theaddRouterTrackingfunction.it("should do nothing if router is not configured", async () => { updateSettings({ pageTracker: { router: undefined } }); await addRouterTracking(); expect(trackRoute).not.toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/add-router-tracking.test.ts(1 hunks)src/add-router-tracking.ts(1 hunks)src/settings.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/add-router-tracking.ts
🧰 Additional context used
🧬 Code Definitions (2)
src/add-router-tracking.test.ts (2)
src/settings.ts (2)
resetSettings(210-212)updateSettings(214-216)src/add-router-tracking.ts (1)
addRouterTracking(4-24)
src/settings.ts (4)
src/api/pageview.ts (1)
Pageview(5-5)src/api/screenview.ts (1)
Screenview(4-7)src/api/linker.ts (1)
LinkerParams(3-8)src/utils.ts (1)
deepMerge(70-90)
🔇 Additional comments (12)
src/settings.ts (7)
1-12: Well-structured imports and type aliasesGood job on organizing imports by specific types rather than entire modules, and creating type aliases to centralize Vue Router dependencies. This approach makes the codebase more maintainable if you need to change router implementations in the future.
14-63: Well-documented PageTracker types with flexible configuration optionsThe PageTracker types are thoroughly documented with JSDoc comments. I particularly like the flexibility provided by allowing both object literals and functions for templates and exclude options, which enhances customization possibilities for users.
65-98: Comprehensive hook system with clear documentationThe hooks implementation provides a robust event system with clear documentation about trigger timing and parameters. This will be valuable for users who need to perform custom actions at specific points in the tracking lifecycle.
100-123: Good resource configuration with sensible defaultsThe Resource type provides good flexibility for script loading configuration with clear documentation and sensible defaults. The addition of CSP nonce support is particularly helpful for security-conscious applications.
127-195: Comprehensive Settings type with proper documentationThe Settings type provides a complete configuration API with well-documented options. The distinction between PluginSettings and Settings, making tagId required in the former, is a good design decision to ensure proper initialization.
197-202: Proper use of readonly for default settingsGood job marking defaultSettings as Readonly to prevent accidental modifications. This ensures the defaults remain consistent throughout the application lifecycle.
214-216: Improved settings update with deep mergeGreat implementation of updateSettings using deepMerge rather than a shallow merge. This ensures that nested configuration objects are properly merged instead of being overwritten.
src/add-router-tracking.test.ts (5)
1-7: Well-structured test setup with proper mocking.The imports and mocking setup are clean and appropriate for the tests. Mocking
trackRouteis the right approach for unit testing the router tracking functionality in isolation.
8-12: Good minimal route definition for testing.The routes array provides a simple and adequate setup for testing router functionality without unnecessary complexity.
13-34: First test case correctly verifies initial tracking.This test effectively verifies that the router tracking initializes correctly by checking that
trackRouteis called with the initial route path once the router is ready. Good use of mocking for theisReadymethod.
35-55: Second test case properly validates tracking after navigation.The test correctly verifies that route changes trigger tracking by pushing to a new route and checking that
trackRouteis called with the new path. The use oftoHaveBeenNthCalledWithensures the proper sequence is verified.
57-91: Third test case correctly verifies duplicate path handling.This test properly validates that identical consecutive paths don't trigger multiple tracking events, which aligns with the implementation in
add-router-tracking.tsthat skips tracking when paths are identical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/create-gtag.ts (2)
37-38: Clarify the naming ofCreateGtagReturn.Although
CreateGtagReturnis self-descriptive as a function that installs on Vue’sApp, consider naming it more explicitly, e.g.,InstallFunctionorVuePluginInstallFn, so it’s immediately clear to collaborators when reading usage contexts.
39-46: Handle potential issues withupdateSettingsbefore script injection.
createGtagupdates the settings and immediately callsaddGtag()without handling a scenario wheretagIdmight be missing or incorrect. SincePluginSettingsrequirestagId, this is likely safe. Still, consider adding a runtime check fortagIdif you expect dynamic usage or user-provided configuration, to provide earlier error reporting.src/composable/use-gtag-with-consent.test.ts (2)
29-43: Validate concurrency in asynchronous acceptance.Observing
acceptAll()callscreateGtaginternally and then reloads the page. Consider whether more granular checks are needed to confirm that the script is fully initialized before reloading, especially in real production usage. The test otherwise is correctly verifying the acceptance flow.
69-79: Edge case: rejecting after consenting.Currently, the test ensures
rejectAllcallsconsentDeniedAlland doesn’t reload. A nice extension might be verifying the behavior if the user previously provided some form of consent. Although it might be safe, consider adding an extra test to confirm an edge scenario, e.g., partial acceptance before rejection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package.json(4 hunks)src/composable/use-gtag-with-consent.test.ts(1 hunks)src/composable/use-gtag-with-consent.ts(1 hunks)src/create-gtag.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/composable/use-gtag-with-consent.ts
🧰 Additional context used
🧬 Code Definitions (2)
src/composable/use-gtag-with-consent.test.ts (2)
src/settings.ts (2)
resetSettings(210-212)updateSettings(214-216)src/composable/use-gtag-with-consent.ts (1)
useGtagWithConsent(17-55)
src/create-gtag.ts (1)
src/settings.ts (2)
PluginSettings(194-195)updateSettings(214-216)
🔇 Additional comments (8)
src/create-gtag.ts (3)
18-35: Check for possible extension or customization in theapiconst.The
apiobject centralizes multiple functionality modules. This design is flexible, but if additional custom methods or relevant hooks need to be exposed later, confirm that updates here maintain backward compatibility. The approach is clean and consistent otherwise.
50-54: Useful module augmentation for Vue.Explicitly extending
ComponentCustomPropertiesfor$gtagis a solid approach, ensuring typed usage in Vue. This is good for plugin-based patterns and keeps code strongly typed across the app.
1-16:❓ Verification inconclusive
Ensure async call alignment if needed.
Currently,
addGtagis imported, but we do not explicitly verify whetheraddGtag()is synchronous or asynchronous. IfaddGtagis async, consider usingawaitor explicit error handling here to prevent potential race conditions and to make sure the script is fully loaded before proceeding.Could you confirm whether
addGtagreturns a promise or a synchronous operation? If it's async, here’s a sample script to search for its definition and see whether it is declared as async:
🏁 Script executed:
#!/bin/bash ast-grep --pattern $'function addGtag($_) async {\n $$$\n}'Length of output: 59
Below is a new shell script to further verify whether
addGtagis declared as an asynchronous function. Please run these commands and share the output so we can confirm if any async patterns (for a regular function or an arrow function) exist insrc/add-gtag.ts:#!/bin/bash echo "Checking for async declaration (regular function) in src/add-gtag.ts:" rg -n "async function addGtag" src/add-gtag.ts echo "" echo "Checking for async declaration (arrow function) in src/add-gtag.ts:" rg -n "const addGtag\s*=\s*async" src/add-gtag.tsBased on the output:
- If one of these searches returns a match, it indicates that
addGtagis asynchronous and should be invoked withawait(along with proper error handling) to prevent race conditions.- If no async declaration is found, then the current synchronous usage is likely correct.
Please run these scripts and then manually verify the results before proceeding.
src/composable/use-gtag-with-consent.test.ts (5)
1-12: Mocking strategy is well-defined.Mocking consent-related APIs,
addGtag, andsettingsensures isolation of side effects. This is a good practice for test stability and clarity.
17-27: Environment setup is clear.Resetting settings and stubbing
location.reloadinbeforeEach/afterEachblocks ensures each test starts fresh. This helps avoid cross-test contamination and is a best practice.
45-67: Test coverage for custom consent props is beneficial.The logic for partial acceptance using distinct property values (
ad_storage,ad_user_data) is validated. This ensures the code can set custom consent states. Great job covering this scenario in detail.
81-86: Verifying no call is made when no consent is found.Good coverage for the case where no
_gacookie is set. The test ensureshasConsentis false and that the analytics script is never added. This aligns with consent prerequisites.
88-99: Assertion clarity for existing consent.Checking that
hasConsent.valueistruewhen_gaexists, along with the subsequent calls, is a thorough approach. It’s well aligned with your defaultconsentMode: "denied"logic and ensures code correctness for returning users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 11
🧹 Nitpick comments (30)
src/api/time/time.ts (1)
1-6: LGTM! Well-implemented timing event function.The
timefunction correctly triggers Google Analytics timing events by calling theeventfunction with "timing_complete" as the event name.-export default function time(params: GtagEventParams) { +export function time(params: GtagEventParams) {Consider using a named export instead of a default export to maintain consistency with other modules in the codebase (like
customMapin custom-map.ts).src/core/add-router-tracking.ts (1)
13-15: Consider error handling for router readinessThe function waits for the router to be ready but doesn't include error handling. If
router.isReady()rejects, it could cause unhandled promise rejections.- await router.isReady(); + try { + await router.isReady(); + } catch (error) { + console.error("Failed to initialize router tracking:", error); + return; + }src/api/linker/linker.ts (1)
10-12: Consider adding JSDoc comments for better developer experienceThe implementation is clean and straightforward, but adding JSDoc comments would improve documentation.
+/** + * Configure cross-domain measurement by specifying which domains to link. + * @param params - Configuration options for the linker + */ export function linker(params: LinkerParams) { query("set", "linker", params); }src/api/time/time.test.ts (1)
1-16: Test effectively validates the time function behaviorThe test correctly validates that the
timefunction forwards parameters to the appropriate event type.However, unlike the custom-map test, this test doesn't call
resetSettingsin abeforeEachhook. Consider adding it for consistency with other test files if the function uses global settings.src/api/exception/exception.test.ts (1)
1-18: Test effectively validates the exception function behaviorThe test correctly validates that the
exceptionfunction forwards parameters to the appropriate event type.As with the time test, consider adding
resetSettingsin abeforeEachhook for consistency with other test files if the function uses global settings.src/api/event/event.ts (1)
12-20: Consider creating a copy of the params object to avoid side effects.The function currently modifies the
paramsobject directly when setting thesend_toproperty. This could lead to unexpected behavior if the caller expects the original object to remain unchanged.export function event(name: GtagEventNames, params: EventParams) { const { groupName, additionalAccounts } = getSettings(); - if (params.send_to === undefined && additionalAccounts?.length) { - params.send_to = groupName; - } + const eventParams = { ...params }; + + if (eventParams.send_to === undefined && additionalAccounts?.length) { + eventParams.send_to = groupName; + } - query("event", name, params); + query("event", name, eventParams); }src/api/config/config.ts (1)
5-21: Well structured config implementation!The function effectively configures tracking for both the main tagId and any additional accounts. The early returns are a good practice for avoiding deep nesting.
Consider adding a brief JSDoc comment to document this function's purpose, parameters, and behavior - especially since it's an important part of the tracking architecture.
+/** + * Configures Google Analytics tracking with the specified parameters + * @param params - Configuration parameters to apply to the tracking ID(s) + */ export function config(params: GtagConfig) { const { tagId, additionalAccounts } = getSettings(); if (!tagId) { return; } query("config", tagId, params); if (!additionalAccounts) { return; } for (const account of additionalAccounts) { query("config", account.tagId, params); } }src/core/settings.test.ts (1)
3-26: Good test coverage for settings functionalityThe tests properly verify both retrieving default settings and updating them with new values. The use of
expect.objectContainingis appropriate for partial object matching.Consider adding a test for the
resetSettingsfunction to ensure settings are properly restored to defaults after modification.import { getSettings, updateSettings, resetSettings } from "./settings"; describe("config", () => { it("should return the configuration", () => { expect(getSettings()).toEqual({ resource: { url: "https://www.googletagmanager.com/gtag/js" }, dataLayerName: "dataLayer", gtagName: "gtag", groupName: "default", }); }); it("should update the default configuration", () => { updateSettings({ tagId: "UA-1234567", gtagName: "foo", }); expect(getSettings()).toEqual( expect.objectContaining({ tagId: "UA-1234567", gtagName: "foo", }), ); }); + it("should reset configuration to defaults", () => { + // First update settings + updateSettings({ + tagId: "UA-1234567", + gtagName: "foo", + }); + + // Then reset + resetSettings(); + + // Verify defaults are restored + expect(getSettings()).toEqual({ + resource: { url: "https://www.googletagmanager.com/gtag/js" }, + dataLayerName: "dataLayer", + gtagName: "gtag", + groupName: "default", + }); + }); });src/core/add-router-tracking.test.ts (1)
13-92: Comprehensive router tracking testsThe tests cover all key behaviors: initial route tracking, navigation tracking, and skipping duplicate routes. The setup with mock routes and spying on router methods is well implemented.
However, there's repetitive code in the router creation and setup. Consider extracting this into a helper function to improve test maintainability.
describe("add-router-tracking", () => { beforeEach(resetSettings); + + function setupRouter() { + const router = createRouter({ + history: createWebHistory(), + routes, + }); + + vi.spyOn(router, "isReady").mockResolvedValue(); + return router; + } it("should track once the router is ready", async () => { - const router = createRouter({ - history: createWebHistory(), - routes, - }); - - vi.spyOn(router, "isReady").mockResolvedValue(); + const router = setupRouter(); updateSettings({ pageTracker: { router } }); await addRouterTracking(); expect(trackRoute).toHaveBeenCalledWith( expect.objectContaining({ path: "/", }), ); }); it("should track after route change", async () => { - const router = createRouter({ - history: createWebHistory(), - routes, - }); - - vi.spyOn(router, "isReady").mockResolvedValue(); + const router = setupRouter(); updateSettings({ pageTracker: { router } }); await addRouterTracking(); await router.push("/about"); expect(trackRoute).toHaveBeenNthCalledWith( 2, expect.objectContaining({ path: "/about", }), ); }); it("should skip tracking identical paths", async () => { - const router = createRouter({ - history: createWebHistory(), - routes, - }); - - vi.spyOn(router, "isReady").mockResolvedValue(); + const router = setupRouter(); updateSettings({ pageTracker: { router, }, }); await addRouterTracking(); await router.push("/about"); await router.push("/about"); expect(trackRoute).toHaveBeenNthCalledWith( 1, expect.objectContaining({ path: "/", }), ); expect(trackRoute).toHaveBeenNthCalledWith( 2, expect.objectContaining({ path: "/about", }), ); expect(trackRoute).toHaveBeenCalledTimes(2); }); });src/api/event/event.test.ts (1)
7-57: Good test coverage for event functionalityThe tests effectively verify the core behaviors of the event function, including proper handling of the
send_toproperty across different scenarios.To improve test maintainability, consider using a
beforeEachhook to reset settings before each test, as you've done in other test files. This would prevent potential test interdependencies.import { updateSettings, resetSettings } from "../../core/settings"; import { query } from "../query"; import { event } from "./event"; vi.mock("../query"); describe("event", () => { + beforeEach(resetSettings); + it("should query the event command", () => { event("screen_view", { screen_name: "about" }); expect(query).toHaveBeenCalledWith("event", "screen_view", { screen_name: "about", }); }); it("should add a group name", () => { updateSettings({ additionalAccounts: [{ tagId: "UA-1" }, { tagId: "UA-2" }], }); event("screen_view", { screen_name: "about" }); expect(query).toHaveBeenCalledWith("event", "screen_view", { screen_name: "about", send_to: "default", }); }); it("should use a custom group name", () => { updateSettings({ additionalAccounts: [{ tagId: "UA-1" }, { tagId: "UA-2" }], groupName: "custom_group_name", }); event("screen_view", { screen_name: "about" }); expect(query).toHaveBeenCalledWith("event", "screen_view", { screen_name: "about", send_to: "custom_group_name", }); }); it("should use the send_to property if already set", () => { updateSettings({ additionalAccounts: [{ tagId: "UA-1" }, { tagId: "UA-2" }], }); event("screen_view", { screen_name: "about", send_to: "my_custom_send_to", }); expect(query).toHaveBeenCalledWith("event", "screen_view", { screen_name: "about", send_to: "my_custom_send_to", }); }); });src/core/track-route.test.ts (1)
201-221: Consider adding a test for combined custom template and screenview.The test for custom template with screenview is good, but consider adding another test that uses a function template with screenview to fully cover all permutations.
it("should track screen views using a custom template", async () => { updateSettings({ pageTracker: { router, useScreenview: true, template: { screen_name: "about", }, }, }); await router.isReady(); await router.push("/"); trackRoute(router.currentRoute.value); expect(query).toHaveBeenCalledWith("event", "screen_view", { screen_name: "about", }); }); + +it("should track screen views using a custom template function", async () => { + updateSettings({ + appName: "MyApp", + pageTracker: { + router, + useScreenview: true, + template: (to) => ({ + screen_name: `${to.name}_custom`, + }), + }, + }); + + await router.isReady(); + await router.push("/about"); + + trackRoute(router.currentRoute.value); + + expect(query).toHaveBeenCalledWith("event", "screen_view", { + screen_name: "about_custom", + app_name: "MyApp", + }); +});src/api/opt/opt.test.ts (1)
4-9: Consider adding error case tests.The tests cover the main functionality well, but consider adding tests for error cases, such as invalid tag IDs or edge cases.
beforeEach(() => { resetSettings(); window["ga-disable-UA-1"] = undefined; window["ga-disable-UA-2"] = undefined; }); + +it("should handle invalid tag IDs gracefully", () => { + // Test with empty string + optOut(""); + expect(window["ga-disable-"]).toBeUndefined(); + + // Test with null value (assuming TypeScript allows this test) + optOut(null as any); + expect(window["ga-disable-null"]).toBeUndefined(); +});src/api/query/query.ts (3)
5-12: Consider more specific type for Window interface extension.While the dynamic indexing with
[key: string]works, it might be more type-safe to limit this to known property patterns for tracking (like 'ga-disable-*' properties).declare global { interface Window { // biome-ignore lint/suspicious/noExplicitAny: - [key: string]: any | any[]; + [key: `${string}`]: any | any[]; + // More specific for tracking properties + [key: `ga-disable-${string}`]?: boolean; + [key: `${string}DataLayer`]?: any[]; } }
14-30: Avoid redefining window[gtagName] function on every query call.The current implementation redefines the global gtag function on every call to
query, which is inefficient for repeated calls. Consider defining this function once during initialization.export function query(...args: QueryParams) { const { dataLayerName, gtagName } = getSettings(); if (isServer()) { return; } window[dataLayerName] = window[dataLayerName] || []; - // biome-ignore lint/complexity/useArrowFunction: - window[gtagName] = function () { - // biome-ignore lint/style/noArguments: - window[dataLayerName].push(arguments); - }; + // Only define the function if it doesn't exist yet + if (typeof window[gtagName] !== 'function') { + // biome-ignore lint/complexity/useArrowFunction: + window[gtagName] = function () { + // biome-ignore lint/style/noArguments: + window[dataLayerName].push(arguments); + }; + } window[gtagName](...args); }
17-19: Consider providing more context for isServer early returns.The early return when running on the server is good, but consider adding a debug log or comment to explain why tracking is skipped on the server.
if (isServer()) { + // Skip tracking on server-side as window object is not available return; }src/api/screenview/screenview.ts (2)
11-29: Improve type safety in screenview function.The function handles different parameter types, but the type checking could be improved to avoid potential runtime errors.
export function screenview(params: ScreenviewParams) { const { appName } = getSettings(); let template: Screenview = {}; if (typeof params === "string") { template.screen_name = params; - } else if ("path" in params) { - template.screen_name = (params.name ?? params.path) as string; + } else if ("path" in params && params.path !== undefined) { + // More explicit check for Route type + const routeName = params.name !== undefined ? String(params.name) : undefined; + template.screen_name = routeName ?? params.path; } else { + // Must be Screenview type template = params; } - if (appName && template?.app_name === undefined) { + // template is never undefined here, so no need for optional chaining + if (appName && template.app_name === undefined) { template.app_name = appName; } query("event", "screen_view", template); }
11-29: Consider adding validation for screen_name.The function doesn't validate that a screen_name exists before sending the event, which could lead to tracking events with missing data.
export function screenview(params: ScreenviewParams) { const { appName } = getSettings(); let template: Screenview = {}; if (typeof params === "string") { template.screen_name = params; } else if ("path" in params) { template.screen_name = (params.name ?? params.path) as string; } else { template = params; } if (appName && template?.app_name === undefined) { template.app_name = appName; } + // Validate that we have a screen name before sending + if (!template.screen_name) { + console.warn('screenview called without a screen_name'); + return; + } + query("event", "screen_view", template); }src/core/track-route.ts (1)
16-18: Consider simplifying the route exclusion checkThe array check could be more concise and avoid potential issues with destructuring undefined objects.
- return pageTracker.exclude?.some(({ name, path } = {}) => { - return (name && name === route.name) || (path && path === route.path); - }); + return pageTracker.exclude?.some((exclusion) => { + return (exclusion.name && exclusion.name === route.name) || + (exclusion.path && exclusion.path === route.path); + });src/api/consent/consent.ts (1)
1-24: Consider adding individual consent type managementThe current implementation only provides all-or-nothing consent management. Consider adding functions to manage individual consent types for more granular control.
import type { GtagConsentArg, GtagConsentParams } from "../../types/gtag"; import { query } from "../query"; export function consent(consentArg: GtagConsentArg, params: GtagConsentParams) { query("consent", consentArg, params); } export function consentGrantedAll(mode: GtagConsentArg = "default") { consent(mode, { ad_user_data: "granted", ad_personalization: "granted", ad_storage: "granted", analytics_storage: "granted", }); } export function consentDeniedAll(mode: GtagConsentArg = "default") { consent(mode, { ad_user_data: "denied", ad_personalization: "denied", ad_storage: "denied", analytics_storage: "denied", }); } + +// Individual consent type management functions +export function setAdUserDataConsent( + status: "granted" | "denied", + mode: GtagConsentArg = "default" +) { + consent(mode, { ad_user_data: status }); +} + +export function setAdPersonalizationConsent( + status: "granted" | "denied", + mode: GtagConsentArg = "default" +) { + consent(mode, { ad_personalization: status }); +} + +export function setAdStorageConsent( + status: "granted" | "denied", + mode: GtagConsentArg = "default" +) { + consent(mode, { ad_storage: status }); +} + +export function setAnalyticsStorageConsent( + status: "granted" | "denied", + mode: GtagConsentArg = "default" +) { + consent(mode, { analytics_storage: status }); +}src/core/add-configuration.test.ts (1)
13-25: Consider parameterizing similar test casesThe test structure is duplicated across several test cases. Consider using a parameterized test approach to reduce duplication.
- it("should query with initial configuration", () => { - updateSettings({ - tagId: "UA-12345678", - }); - - addConfiguration(); - - expect(query).toHaveBeenNthCalledWith(1, "js", new Date()); - expect(query).toHaveBeenNthCalledWith(2, "config", "UA-12345678", { - send_page_view: false, - anonymize_ip: true, - }); - }); + const testCases = [ + { + name: "initial configuration", + settings: { tagId: "UA-12345678" }, + expectedCalls: [ + { nth: 1, args: ["js", new Date()] }, + { + nth: 2, + args: ["config", "UA-12345678", { + send_page_view: false, + anonymize_ip: true, + }] + } + ] + }, + // Add more test cases here + ]; + + testCases.forEach(({ name, settings, expectedCalls }) => { + it(`should query with ${name}`, () => { + updateSettings(settings); + addConfiguration(); + expectedCalls.forEach(({ nth, args }) => { + expect(query).toHaveBeenNthCalledWith(nth, ...args); + }); + }); + });src/core/add-gtag.ts (2)
13-17: Consider changing the order of operationsThe configuration and router tracking are set up before checking if Gtag already exists. Consider checking for Gtag existence first to avoid unnecessary configuration if the script is already loaded.
export default async function addGtag(): Promise<void> { const { resource, dataLayerName, tagId, pageTracker, hooks } = getSettings(); if (!tagId) { return; } + if (hasGtag()) { + hooks?.["script:loaded"]?.(); + addConfiguration(); + + if (pageTracker?.router) { + addRouterTracking(); + } + return; + } + addConfiguration(); if (pageTracker?.router) { addRouterTracking(); } - if (hasGtag()) { - hooks?.["script:loaded"]?.(); - return; - } - try { await injectScript(`${resource.url}?id=${tagId}&l=${dataLayerName}`, { preconnect: resource.preconnect, defer: resource.defer, nonce: resource.nonce, }); hooks?.["script:loaded"]?.(); } catch (error) { hooks?.["script:error"]?.(error); } }
32-34: Consider adding more specific error handlingThe current error handling doesn't distinguish between different types of errors that could occur during script injection. Consider adding more specific error handling or logging.
try { await injectScript(`${resource.url}?id=${tagId}&l=${dataLayerName}`, { preconnect: resource.preconnect, defer: resource.defer, nonce: resource.nonce, }); hooks?.["script:loaded"]?.(); } catch (error) { + console.error("Failed to load Google Analytics script:", error); hooks?.["script:error"]?.(error); }src/api/consent/consent.test.ts (1)
1-62: Test coverage looks good but could be enhanced with edge cases.The test suite effectively verifies the core functionality of consent management functions, confirming that each function properly calls the query method with the expected parameters. The mocking approach is appropriate.
I would suggest adding a few more test cases:
- Test error handling scenarios
- Edge cases with partial consent parameters
- Verify behavior with invalid consent argument values
Add tests for edge cases:
it("should handle partial consent parameters", () => { consent("update", { analytics_storage: "granted", // Only specifying one parameter }); expect(query).toHaveBeenCalledWith("consent", "update", { analytics_storage: "granted", }); }); it("should handle invalid consent arguments gracefully", () => { // @ts-expect-error: Testing with invalid argument consent("invalid_argument", { ad_storage: "granted", }); expect(query).toHaveBeenCalledWith("consent", "invalid_argument", { ad_storage: "granted", }); });src/api/screenview/screenview.test.ts (3)
1-26: Good test setup but consider mock restoration.The test setup is comprehensive, but you should reset your mocks between tests to ensure test isolation.
Add cleanup after each test:
beforeEach(async () => { resetSettings(); router = createRouter({ history: createWebHistory("/base-path"), routes: [ { path: "/no-name", component: { template: "<div />" } }, { path: "/about", name: "about", component: { template: "<div />" } }, ], }); vi.spyOn(router, "isReady").mockResolvedValue(); await router.isReady(); await router.push({ name: "about", query: { id: 1 }, hash: "#title" }); }); + afterEach(() => { + vi.resetAllMocks(); + });
28-46: Add negative test case for invalid screen name parameter.The tests cover the happy path scenarios but lack tests for invalid parameter handling.
Add a test for invalid screen name parameter:
it("should handle undefined screen name gracefully", () => { // @ts-expect-error Testing invalid parameter screenview(undefined); // Expect query not to be called or called with default values expect(query).toHaveBeenCalledWith("event", "screen_view", expect.any(Object)); });
48-84: Consider testing error scenarios in route tracking.The
pageTrackertests are good but should also test error scenarios to ensure robustness.Add a test for route tracking failures:
it("should handle route tracking errors gracefully", async () => { // Mock router to throw an error const originalRoute = router.currentRoute; Object.defineProperty(router, 'currentRoute', { get: () => { throw new Error('Route error'); } }); try { // This should not throw screenview(router); // Expect query not to be called because of the error expect(query).not.toHaveBeenCalled(); } finally { // Restore router Object.defineProperty(router, 'currentRoute', { get: () => originalRoute.value }); } });src/composables/use-consent/use-consent.ts (1)
19-20: Make cookie name a configurable constant.The GA cookie name is hardcoded, which could become problematic if the cookie name changes in the future or if a different implementation uses a different name.
Move the cookie name to a configurable constant:
-const GA_COOKIE_VALUE = "_ga"; +/** + * Default cookie name for Google Analytics + * @type {string} + */ +const DEFAULT_GA_COOKIE_VALUE = "_ga";Then update the usage to allow for configuration through settings:
const hasConsent = ref<boolean>( - isServer() ? false : document.cookie.includes(GA_COOKIE_VALUE), + isServer() ? false : document.cookie.includes(settings.gaCookieName || DEFAULT_GA_COOKIE_VALUE), );src/api/pageview/pageview.ts (2)
9-14: Consider edge case in path normalization.The
getPathWithBasefunction handles most path combinations well, but has an edge case when both base and path are "/", resulting in "//".Consider this improved implementation:
function getPathWithBase(path: string, base: string): string { const normalizedBase = base.endsWith("/") ? base : `${base}/`; const normalizedPath = path.startsWith("/") ? path.substring(1) : path; - return `${normalizedBase}${normalizedPath}`; + // Avoid double slash when normalized path is empty + return normalizedPath ? `${normalizedBase}${normalizedPath}` : normalizedBase; }
21-37: Type casting concern in route name handling.The code assumes
params.nameis a string when present, but there's no type check before casting.Consider adding a more explicit type check:
template = { - ...(params.name ? { page_title: params.name as string } : {}), + ...(params.name && typeof params.name === 'string' ? { page_title: params.name } : {}), page_path: pageTracker?.useRouterBasePath ? getPathWithBase(path, base) : path, };src/types/gtag.ts (1)
46-46: Refine the callback parameter type.
Usinganyincallback?: (field?: string | GtagCustomParams) => anymight reduce type safety. Consider narrowing the return type or usingunknownfor improved clarity.- callback?: (field?: string | GtagCustomParams) => any, + callback?: (field?: string | GtagCustomParams) => unknown,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (59)
biome.json(1 hunks)package.json(3 hunks)src/api/config/config.test.ts(1 hunks)src/api/config/config.ts(1 hunks)src/api/config/index.ts(1 hunks)src/api/consent/consent.test.ts(1 hunks)src/api/consent/consent.ts(1 hunks)src/api/consent/index.ts(1 hunks)src/api/custom-map/custom-map.test.ts(1 hunks)src/api/custom-map/custom-map.ts(1 hunks)src/api/custom-map/index.ts(1 hunks)src/api/ecommerce/ecommerce.test.ts(1 hunks)src/api/ecommerce/ecommerce.ts(1 hunks)src/api/ecommerce/index.ts(1 hunks)src/api/event/event.test.ts(1 hunks)src/api/event/event.ts(1 hunks)src/api/event/index.ts(1 hunks)src/api/exception/exception.test.ts(1 hunks)src/api/exception/exception.ts(1 hunks)src/api/exception/index.ts(1 hunks)src/api/index.ts(1 hunks)src/api/linker/index.ts(1 hunks)src/api/linker/linker.test.ts(1 hunks)src/api/linker/linker.ts(1 hunks)src/api/opt/index.ts(1 hunks)src/api/opt/opt.test.ts(1 hunks)src/api/opt/opt.ts(1 hunks)src/api/pageview/index.ts(1 hunks)src/api/pageview/pageview.test.ts(1 hunks)src/api/pageview/pageview.ts(1 hunks)src/api/query/index.ts(1 hunks)src/api/query/query.test.ts(1 hunks)src/api/query/query.ts(1 hunks)src/api/screenview/index.ts(1 hunks)src/api/screenview/screenview.test.ts(1 hunks)src/api/screenview/screenview.ts(1 hunks)src/api/set/index.ts(1 hunks)src/api/set/set.test.ts(1 hunks)src/api/set/set.ts(1 hunks)src/api/time/index.ts(1 hunks)src/api/time/time.test.ts(1 hunks)src/api/time/time.ts(1 hunks)src/composables/index.ts(1 hunks)src/composables/use-consent/index.ts(1 hunks)src/composables/use-consent/use-consent.test.ts(1 hunks)src/composables/use-consent/use-consent.ts(1 hunks)src/core/add-configuration.test.ts(1 hunks)src/core/add-configuration.ts(1 hunks)src/core/add-gtag.test.ts(1 hunks)src/core/add-gtag.ts(1 hunks)src/core/add-router-tracking.test.ts(1 hunks)src/core/add-router-tracking.ts(1 hunks)src/core/settings.test.ts(1 hunks)src/core/settings.ts(1 hunks)src/core/track-route.test.ts(1 hunks)src/core/track-route.ts(1 hunks)src/index.test.ts(1 hunks)src/index.ts(1 hunks)src/types/gtag.ts(1 hunks)
✅ Files skipped from review due to trivial changes (16)
- src/api/ecommerce/index.ts
- src/api/event/index.ts
- src/api/set/index.ts
- src/api/linker/index.ts
- src/api/pageview/index.ts
- src/api/time/index.ts
- src/api/query/index.ts
- src/api/config/index.ts
- src/composables/index.ts
- src/api/consent/index.ts
- src/composables/use-consent/index.ts
- src/api/custom-map/index.ts
- src/api/opt/index.ts
- src/api/screenview/index.ts
- src/api/exception/index.ts
- src/api/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- biome.json
- src/index.test.ts
🧰 Additional context used
🧬 Code Definitions (35)
src/api/custom-map/custom-map.ts (1)
src/api/config/config.ts (1)
config(5-21)
src/api/config/config.test.ts (2)
src/api/config/config.ts (1)
config(5-21)src/api/query/query.ts (1)
query(14-30)
src/api/time/time.ts (1)
src/api/event/event.ts (1)
event(12-20)
src/api/ecommerce/ecommerce.test.ts (2)
src/api/ecommerce/ecommerce.ts (1)
ecommerce(7-12)src/api/query/query.ts (1)
query(14-30)
src/api/set/set.test.ts (2)
src/api/set/set.ts (1)
set(6-8)src/api/query/query.ts (1)
query(14-30)
src/api/custom-map/custom-map.test.ts (2)
src/api/custom-map/custom-map.ts (1)
customMap(4-8)src/api/query/query.ts (1)
query(14-30)
src/api/exception/exception.ts (1)
src/api/event/event.ts (1)
event(12-20)
src/api/event/event.test.ts (2)
src/api/event/event.ts (1)
event(12-20)src/api/query/query.ts (1)
query(14-30)
src/api/config/config.ts (1)
src/api/query/query.ts (1)
query(14-30)
src/api/set/set.ts (1)
src/api/query/query.ts (1)
query(14-30)
src/core/add-router-tracking.test.ts (3)
src/core/settings.ts (2)
resetSettings(210-212)updateSettings(214-216)src/core/add-router-tracking.ts (1)
addRouterTracking(4-24)src/core/track-route.ts (1)
trackRoute(21-52)
src/api/event/event.ts (1)
src/api/query/query.ts (1)
query(14-30)
src/api/linker/linker.ts (1)
src/api/query/query.ts (1)
query(14-30)
src/core/settings.test.ts (1)
src/core/settings.ts (2)
getSettings(206-208)updateSettings(214-216)
src/api/screenview/screenview.test.ts (2)
src/api/screenview/screenview.ts (1)
screenview(11-29)src/api/query/query.ts (1)
query(14-30)
src/api/time/time.test.ts (2)
src/api/time/time.ts (1)
time(4-6)src/api/query/query.ts (1)
query(14-30)
src/api/ecommerce/ecommerce.ts (1)
src/api/event/event.ts (1)
event(12-20)
src/core/track-route.ts (3)
src/core/settings.ts (3)
Route(12-12)getSettings(206-208)PageTrackerParams(14-14)src/api/screenview/screenview.ts (1)
screenview(11-29)src/api/pageview/pageview.ts (1)
pageview(16-48)
src/index.ts (2)
src/core/settings.ts (2)
PluginSettings(194-195)updateSettings(214-216)src/core/add-gtag.ts (1)
addGtag(6-35)
src/api/linker/linker.test.ts (2)
src/api/linker/linker.ts (1)
linker(10-12)src/api/query/query.ts (1)
query(14-30)
src/api/screenview/screenview.ts (1)
src/api/query/query.ts (1)
query(14-30)
src/api/query/query.test.ts (1)
src/api/query/query.ts (1)
query(14-30)
src/composables/use-consent/use-consent.ts (1)
src/index.ts (1)
createGtag(16-23)
src/composables/use-consent/use-consent.test.ts (1)
src/composables/use-consent/use-consent.ts (1)
useConsent(21-57)
src/core/add-router-tracking.ts (2)
src/core/settings.ts (1)
getSettings(206-208)src/core/track-route.ts (1)
trackRoute(21-52)
src/api/pageview/pageview.test.ts (2)
src/api/pageview/pageview.ts (1)
pageview(16-48)src/api/query/query.ts (1)
query(14-30)
src/core/add-configuration.test.ts (3)
src/core/settings.ts (2)
resetSettings(210-212)updateSettings(214-216)src/core/add-configuration.ts (1)
addConfiguration(15-59)src/api/query/query.ts (1)
query(14-30)
src/api/exception/exception.test.ts (2)
src/api/exception/exception.ts (1)
exception(11-13)src/api/query/query.ts (1)
query(14-30)
src/api/opt/opt.test.ts (1)
src/api/opt/opt.ts (2)
optOut(23-25)optIn(33-35)
src/core/add-configuration.ts (4)
src/core/settings.ts (1)
getSettings(206-208)src/api/consent/consent.ts (2)
consentGrantedAll(8-15)consentDeniedAll(17-24)src/api/linker/linker.ts (1)
linker(10-12)src/api/query/query.ts (1)
query(14-30)
src/core/add-gtag.ts (3)
src/core/settings.ts (1)
getSettings(206-208)src/core/add-configuration.ts (1)
addConfiguration(15-59)src/core/add-router-tracking.ts (1)
addRouterTracking(4-24)
src/core/add-gtag.test.ts (4)
src/core/settings.ts (2)
resetSettings(210-212)updateSettings(214-216)src/core/add-gtag.ts (1)
addGtag(6-35)src/core/add-configuration.ts (1)
addConfiguration(15-59)src/core/add-router-tracking.ts (1)
addRouterTracking(4-24)
src/api/consent/consent.test.ts (2)
src/api/consent/consent.ts (3)
consent(4-6)consentGrantedAll(8-15)consentDeniedAll(17-24)src/api/query/query.ts (1)
query(14-30)
src/api/consent/consent.ts (1)
src/api/query/query.ts (1)
query(14-30)
src/api/pageview/pageview.ts (1)
src/api/query/query.ts (1)
query(14-30)
🔇 Additional comments (89)
src/api/custom-map/custom-map.ts (1)
1-8: LGTM! Well-implemented Google Analytics custom dimensions mapping.The
customMapfunction is a clean wrapper around theconfigfunction that properly sets up custom dimension mapping in Google Analytics. The implementation correctly passes the custom parameters to the underlying configuration system.src/api/set/set.test.ts (1)
1-14: LGTM! Well-structured and focused test.The test correctly verifies that the
setfunction passes its parameters to thequeryfunction with "set" as the command name, which is the expected behavior.src/api/exception/exception.ts (1)
1-13: LGTM! Well-implemented exception tracking function with good type definitions.The
exceptionfunction correctly triggers Google Analytics exception events with appropriate parameters. The type definition forExceptionParamsprovides flexibility by accepting either specific exception properties or custom parameters.src/api/set/set.ts (1)
1-8: Well-structured type-safe implementation of thesetcommandThe
setfunction provides a clean wrapper around the query mechanism, with proper TypeScript typing using theGtagCommands["set"]type. This maintains type safety while allowing for variable arguments.src/api/ecommerce/ecommerce.ts (1)
1-12: Clean implementation with proper type safetyThe
ecommercefunction provides a well-typed wrapper around theeventfunction, specifically for ecommerce events. The type definitions ensure type safety while maintaining a clean API.src/core/add-router-tracking.ts (1)
4-24: Well-implemented router tracking setupThe
addRouterTrackingfunction correctly sets up tracking for both the initial route and subsequent navigation. The early return when no router is available is good defensive programming, and the check to avoid tracking when paths haven't changed is an excellent optimization.src/api/custom-map/custom-map.test.ts (1)
1-23: Well-structured test with good isolation practicesThe test correctly validates that the
customMapfunction passes the appropriate parameters to the query function. UsingbeforeEach(resetSettings)ensures proper test isolation.src/api/linker/linker.ts (1)
3-8: Well-defined type with appropriate constraintsThe
LinkerParamstype is well-structured with clear optional properties and a precise union type forurl_position.src/api/query/query.test.ts (1)
4-15: Well-structured test for the query function.This test appropriately verifies that the
queryfunction correctly passes events to the gtag library by checking the arguments pushed to the data layer.src/api/config/config.test.ts (2)
7-18: Comprehensive test for single account configuration.The test correctly verifies that the
configfunction passes the appropriate parameters to thequeryfunction when a single tag ID is configured.
20-39: Good test coverage for multiple account configurations.This test properly validates that the
configfunction handles multiple Google Analytics accounts by callingqueryfor each configured account with the correct parameters.src/api/linker/linker.test.ts (1)
6-15: Test adequately verifies linker functionality.The test ensures that the
linkerfunction correctly passes the domains to the Google Analytics linker feature via thequeryfunction.src/core/track-route.test.ts (6)
1-33: Well-structured test setup and initialization.The test setup is comprehensive, with proper router initialization, route definitions, and mock configurations. The
beforeEachhook ensures a clean testing environment for each test case.
35-53: Good implementation of screenview tracking test.This test effectively verifies that screenviews are tracked correctly with the appropriate parameters, including screen_name and app_name.
55-67: Well-implemented pageview tracking test.The test appropriately verifies that pageviews include all required parameters: page_path, page_location, page_title, and send_page_view flag.
69-115: Comprehensive route exclusion testing.These tests thoroughly cover route exclusions using different approaches (path, name, and custom function), ensuring flexibility in configuration.
117-155: Thorough testing of tracking hooks.The hook tests properly verify that callbacks are fired at the right moments in the tracking flow, enabling custom integration logic.
157-199: Well-implemented custom template testing.These tests thoroughly validate both static and dynamic template configuration options, ensuring flexibility in customizing tracking data.
src/api/opt/opt.test.ts (4)
1-10: Well-structured test setup and initialization.The test setup properly resets settings and clears window properties before each test, ensuring a clean testing environment.
11-30: Good tests for basic opt-out and opt-in functionality.These tests effectively verify the core functionality of opting out and opting back in for analytics tracking.
32-45: Good tests for custom tagId handling.These tests properly verify that custom tag IDs work correctly with the opt-out and opt-in functions.
47-73: Thorough testing of additional account handling.The tests for additional accounts verify that both the primary account and additional accounts are correctly opted out and in.
src/api/screenview/screenview.ts (1)
4-9: Well-defined and clear type definitions.The type definitions are clear and concise, providing a good structure for screen view data.
src/index.ts (3)
1-5: Imports look well-structured and consistent.
Usingimport typeforAppaligns with modern TypeScript best practices, minimizing runtime overhead to type-only imports.
8-12: Vue module augmentation is correctly scoped.
Defining$gtaginComponentCustomPropertiesensures that the Gtag API is readily available in templates and components without additional typings.
25-26: Re-exports enhance discoverability.
Re-exporting from./api/indexand./composables/indexmakes your library’s interface more intuitive for consumers.src/core/add-configuration.ts (2)
7-13: mergeDefaults function is concise and clear.
Merging default settings with user overrides in a single expression enhances readability and maintainability.
15-59: Ensure robust handling for unrecognizedconsentModevalues and missingtagIdinadditionalAccounts.
consentModeonly checks"granted"or"denied", ignoring other potential values.- Loop through
additionalAccountsassumes each has atagId. Consider graceful handling or validation for missing IDs.Would you like a script to search for usage of
consentModeor check that all additional accounts supply a validtagId?src/api/pageview/pageview.test.ts (8)
1-7: Mocking strategy is straightforward.
Mockingqueryensures the test can verify calls to the analytics layer.
8-37: Basic page path tracking tests are clear and well-implemented.
These tests help confirm that passing a string path triggers the correctpage_pathproperty in the event.
39-49: Route-based page path handling is validated.
This test confirms that extracting the path fromrouter.currentRoute.valueis handled as expected.
51-61: Ensures defaultpage_location.
Verifying that the test environment’s URL is included is a thorough approach to confirm defaults are appended.
63-73: Verifies defaultsend_page_viewsetting.
Ensuringsend_page_view = trueby default covers a common usage scenario without further configuration.
75-85: Validatespage_titlefrom route naming.
Confirming route name mapping topage_titleensures consistent analytics naming.
87-99: Confirming custom parameters.
This test ensures advanced usage scenarios (e.g., custom location or disablingsend_page_view) are correctly handled.
101-176:pageTrackertests thoroughly validate advanced routing scenarios.
The suite covers base paths, full paths, and togglingsendPageView, demonstrating robust coverage for route-based analytics.src/core/track-route.ts (3)
5-19: Well-structured route exclusion logicThe
isRouteExcludedfunction appropriately handles multiple exclusion scenarios:
- No exclusions defined
- Function-based exclusion
- Array-based exclusions by name or path
21-52: Well-implemented route tracking with flexible template handlingThe
trackRoutefunction effectively:
- Respects route exclusions
- Handles hook callbacks before and after tracking
- Supports both static and function-based templates
- Conditionally uses either screenview or pageview tracking
40-49: Robust parameter selection for analytics callsThe code correctly handles different parameter sources for both screenview and pageview calls, checking if the template has the expected properties before falling back to using the route directly.
src/api/consent/consent.ts (3)
4-6: Clear wrapper function for consent managementThe
consentfunction provides a clean interface to the underlying query mechanism, making the consent API more intuitive to use.
8-15: Convenient helper for granting all consent typesThe
consentGrantedAllfunction provides a useful shorthand for the common operation of granting all consent types, with flexibility to override the default mode.
17-24: Convenient helper for denying all consent typesThe
consentDeniedAllfunction provides a useful shorthand for the common operation of denying all consent types, with flexibility to override the default mode.src/core/add-configuration.test.ts (5)
1-26: Well-structured initial configuration testThe test properly mocks dependencies, sets up a fixed date for deterministic testing, and verifies that the query function is called with the expected parameters.
27-63: Comprehensive test for multiple domain configurationThe test thoroughly verifies that multiple domains are configured correctly with their respective settings, ensuring that:
- The base configuration is applied
- Group names are correctly added
- Custom configurations override defaults
65-79: Well-designed linker testThe test verifies that the linker is correctly set up when specified in the configuration.
81-113: Good test coverage for hooksThe tests properly verify that both pre and post hooks are called at the appropriate times relative to the query operation.
115-151: Thorough consent mode testingThe tests verify that the consent settings are correctly applied for both granted and denied modes, including checking the order of operations.
src/core/add-gtag.ts (2)
6-11: Good early return pattern with tag ID checkThe function properly handles the case where no tag ID is provided by returning early, preventing unnecessary operations.
24-34: Well-implemented script injection with error handlingThe code effectively:
- Uses async/await for script injection
- Provides necessary parameters from settings
- Handles success with appropriate hook
- Catches and reports errors through hooks
src/composables/use-consent/use-consent.test.ts (7)
1-17: Proper test setup with mocks and imports.The test file correctly imports and mocks all necessary dependencies. The mocking strategy is appropriate, especially using
vi.importActualto retain the actual implementation while overriding specific functions.
18-32: Well-structured test setup and teardown hooks.The test suite follows best practices with
beforeEachto reset settings and stub the global location object for each test. TheafterEachhook ensures all mocks are reset, preventing test pollution.
33-47: LGTM: Comprehensive test for acceptAll behavior.This test properly verifies that the
acceptAllmethod:
- Calls
consentGrantedAllwith the correct parameter- Reloads the page
- Updates settings with the expected configuration
- Calls
addGtagto load the tracking script
49-71: LGTM: Thorough test for acceptCustom behavior.The test correctly verifies custom consent handling, checking that:
- The custom consent parameters are properly passed to the consent function
- The page is reloaded
- Settings are updated correctly
- The tracking script is loaded
73-83: Good test coverage for rejectAll behavior.This test correctly verifies that
rejectAll:
- Calls
consentDeniedAllwith the correct parameter- Doesn't reload the page (important difference from accept methods)
- Doesn't call
addGtag(should not load tracking when rejected)
85-90: LGTM: Proper initial consent state verification.This test correctly verifies that the initial consent state is
falsewhen no cookie is present, and thataddGtagis not called when consent is not granted.
92-102: LGTM: Proper cookie-based consent detection.The test correctly simulates a scenario with an existing GA cookie, verifying that:
- The
hasConsentvalue is correctly identified astrue- Settings are updated with the expected parameters
- The tracking script is loaded automatically
src/core/add-gtag.test.ts (11)
1-16: Good mock setup with proper module imports.The test file correctly imports dependencies and mocks necessary components. Using
vi.importActualallows you to maintain the original implementation while overriding specific functions, which is a good practice.
17-22: Proper test initialization.The
beforeEachhook correctly cleans up the DOM and resets settings before each test, ensuring test isolation.
23-36: LGTM: Basic library load test.The test correctly verifies that the standard Google Tag Manager script is loaded with the proper URL format, including the tag ID and data layer name.
38-56: Good edge case handling for existing script.This test correctly verifies the optimization that prevents loading the script multiple times if it's already in the DOM, and ensures the callback is still triggered.
58-74: LGTM: Custom resource URL test.The test properly verifies that a custom URL can be used for loading the Google Tag Manager script, which is crucial for specialized implementations.
76-89: LGTM: Callback execution test.This test correctly verifies that the success callback is fired when the script is loaded successfully.
91-106: LGTM: Error handling test.The test properly verifies error handling by mocking a rejected promise and ensuring the error callback is called.
108-116: LGTM: Configuration initialization test.This test correctly verifies that the initial configuration is called when the Google Tag Manager script is loaded.
118-135: Good integration test with Vue Router.The test properly sets up a Vue Router instance and verifies that router tracking is initialized. This ensures that page navigation events will be tracked correctly.
137-141: LGTM: Missing tagId handling test.The test verifies the important validation that prevents initialization when no tag ID is provided, which would cause errors in Google Analytics.
143-159: Comprehensive script attribute tests.The tests thoroughly verify that script attributes (preconnect, defer, nonce) are correctly passed to the script injection function, which is important for:
- Performance optimization (preconnect)
- Non-blocking script loading (defer)
- Content Security Policy compliance (nonce)
Also applies to: 161-177, 179-195
src/api/pageview/pageview.ts (3)
1-8: LGTM: Clean type definitions with proper imports.The type definitions are clear and well-structured, properly leveraging imported types.
39-45: LGTM: Good default handling.The function properly sets default values for
page_locationandsend_page_viewbased on either window location or user settings.
47-48: LGTM: Proper event dispatch.The function correctly uses the
queryfunction to send the page view event with the constructed template.src/core/settings.ts (9)
1-13: LGTM: Clean Vue Router type imports and reexports.The code properly imports types from Vue Router and re-exports them with clearer names, which improves code readability.
14-23: LGTM: Well-designed type definitions for page tracker parameters.These type definitions provide flexibility for configuring page tracking, allowing both direct parameters and function-based parameters.
24-63: Excellent PageTracker type with comprehensive JSDoc documentation.The PageTracker type is well-structured with thorough JSDoc comments explaining each option. This approach provides excellent developer experience and self-documentation.
65-98: LGTM: Well-designed hooks interface with clear documentation.The hooks interface is well-designed, providing clear lifecycle events for router tracking and script loading. The JSDoc comments clearly explain the purpose and parameters of each hook.
100-123: LGTM: Comprehensive resource configuration options.The Resource type includes all necessary options for script loading, including URL, preconnect, defer, and nonce support, with clear JSDoc documentation.
125-192: Excellent Settings interface with comprehensive documentation.The Settings type is well-structured and thoroughly documented. It covers all aspects of Google Analytics configuration, from tag IDs to router tracking to consent modes.
194-195: LGTM: Good type constraint for plugin settings.The PluginSettings type properly requires the tagId to be present while making other settings optional, which is an excellent practice for API design.
197-202: LGTM: Sensible default settings with readonly modifier.The default settings object uses the Readonly type to prevent accidental modification, which is a good practice for configuration objects.
204-216: LGTM: Clean settings management functions.The settings management functions are straightforward and follow good practices:
getSettingsreturns the current settingsresetSettingsrestores defaultsupdateSettingsuses deep merge to properly handle nested objectspackage.json (6)
5-5: Confirm version bump.
Bumping to3.0.0-beta.16looks intentional and aligns with the upcoming major release changes. Just ensure that existing consumers are adequately informed about any breaking changes in the next stable release.Could you verify the impact of this version bump on existing consumers, especially those still on 2.x versions?
18-20: Good consolidation of the scripts.
Switching frombiome checktobiome lintresolves the previously reported missing "check" script. The additionaltypecheckandtest:covscripts improve project maintenance.Also applies to: 22-22
39-44: ESM-friendly exports.
Defining an"exports"field for ESM usage is a valuable step. This ensures better module resolution for consumers using ESM.
45-49: Retaining main, module, and dist references.
Maintaining these fields is practical for compatibility with bundlers and older Node versions while also listing"dist"under"files"to ensure a clean package distribution.
56-57: Peer dependencies updated for Vue 3 ecosystem.
Upgradingvueto^3.5.13and addingvue-routeras a peer dependency ensures clarity for library consumers. Confirm that users on older Vue 3 versions can still integrate or receive a suitable peer dependency warning.Could you confirm if you have tested or validated compatibility with Vue 3 versions earlier than 3.5.x?
61-62: New development dependencies align with the TS refactor.
Adding@types/node, coverage tools, minification, and DTS plugins is consistent with the migration to a TypeScript + Vite build. Also, providingvuein devDependencies simultaneously withvuein peerDependencies is typical for library development, ensuring local testing.Also applies to: 66-66, 68-68, 70-70, 72-76
src/types/gtag.ts (4)
1-5: Well-structured union types for configuration.
CombiningGtagControlParams,GtagEventParams,GtagConfigParams, andGtagCustomParamsin a union is an elegant way to capture varied parameter possibilities without bloating the function signatures.
58-62: [Duplicate] Strengthen return type for theGtagfunction.
This replicates a previously raised comment: ifgtag("get", ...)or other commands produce data, consider returning that data or a promise rather than strictlyvoid.
94-135: Comprehensive enumeration of event names.
Defining separate event sets for e-commerce (GtagEcommerceEventNames) and general analytics (GtagGeneralEventNames) leads to better type safety, reducing accidental typos or invalid event names.
324-348: Consent mode coverage looks robust.
ProvidingGtagConsentModeandGtagConsentParamsfor ad personalization, analytics, and storage strikes a good balance between flexibility and typed constraints. This should align well with future expansions to the consent APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/api/event/event.ts (1)
15-17: Consider handling the potential edge case of undefined params.While this implementation works correctly when params is an object, it could be made more robust by adding a null check before accessing the
params.send_toproperty.- if (params.send_to === undefined && additionalAccounts?.length) { + if (params?.send_to === undefined && additionalAccounts?.length) { params.send_to = groupName; }src/index.test.ts (2)
6-6: Consider using the vi.mock method with inline factory function for more control.The current mock setup is good, but you could make it more explicit about what's being mocked and potentially add mock implementations.
-vi.mock("./core/add-gtag"); +vi.mock("./core/add-gtag", () => ({ + default: vi.fn().mockResolvedValue(undefined), +}));
8-22: Consider adding test cases for error scenarios and custom configurations.The current tests cover the happy path, but adding tests for error handling and custom configurations would improve test coverage.
Consider adding tests for:
- When
addGtagthrows an error- When custom configuration options are provided
- Integration with a router for automatic tracking
src/api/query/query.ts (1)
7-12: Consider restricting the global window interface extension.The current global window interface extension allows any property to be added with any type, which can lead to type safety issues. Consider narrowing this to only the specific properties needed for this module.
declare global { interface Window { - // biome-ignore lint/suspicious/noExplicitAny: - [key: string]: any | any[]; + [dataLayerName: string]: any[]; + [gtagName: string]: (...args: any[]) => void; } }src/core/add-configuration.ts (1)
32-36: Consider handling custom consent settings.The current implementation only supports granted/denied for all consent types. Consider adding support for more granular consent settings where users can selectively enable specific consent types.
if (consentMode === "granted") { consentGrantedAll(); } else if (consentMode === "denied") { consentDeniedAll(); + } else if (typeof consentMode === "object") { + consent("default", consentMode); }src/api/consent/consent.ts (1)
8-24: Consider adding support for individual consent preferences.The current implementation only supports granting or denying all consent types at once. Consider adding helper functions for managing individual consent types for more granular control.
+export function consentUpdate(mode: GtagConsentArg = "default", updates: Partial<GtagConsentParams>) { + // Get current consent status or start with all denied + const currentConsent: GtagConsentParams = { + ad_user_data: "denied", + ad_personalization: "denied", + ad_storage: "denied", + analytics_storage: "denied", + // Override with updates + ...updates + }; + + consent(mode, currentConsent); +} +// Helper for analytics only +export function consentAnalyticsOnly(mode: GtagConsentArg = "default") { + consent(mode, { + ad_user_data: "denied", + ad_personalization: "denied", + ad_storage: "denied", + analytics_storage: "granted", + }); +}src/core/add-configuration.test.ts (1)
10-153: Consider adding a missingtagIdtest scenario.All current tests appear thorough. However, testing the branch where
tagIdis undefined would ensure that no calls toqueryoccur in that case and further strengthen coverage.src/core/add-gtag.ts (1)
6-35: Optional SSR safeguard consideration.This function always attempts to inject or configure gtag on invocation. If server-side rendering fans out requests or calls this function incidentally, you might consider adding an SSR check (e.g., an
isServer()guard) before script injection to avoid potential errors. Otherwise, this looks good.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
src/api/config/config.test.ts(1 hunks)src/api/config/config.ts(1 hunks)src/api/consent/consent.test.ts(1 hunks)src/api/consent/consent.ts(1 hunks)src/api/custom-map/custom-map.test.ts(1 hunks)src/api/custom-map/custom-map.ts(1 hunks)src/api/ecommerce/ecommerce.test.ts(1 hunks)src/api/ecommerce/ecommerce.ts(1 hunks)src/api/event/event.test.ts(1 hunks)src/api/event/event.ts(1 hunks)src/api/exception/exception.test.ts(1 hunks)src/api/exception/exception.ts(1 hunks)src/api/linker/linker.test.ts(1 hunks)src/api/linker/linker.ts(1 hunks)src/api/opt/opt.test.ts(1 hunks)src/api/opt/opt.ts(1 hunks)src/api/pageview/pageview.test.ts(1 hunks)src/api/pageview/pageview.ts(1 hunks)src/api/query/query.test.ts(1 hunks)src/api/query/query.ts(1 hunks)src/api/screenview/screenview.test.ts(1 hunks)src/api/screenview/screenview.ts(1 hunks)src/api/set/set.test.ts(1 hunks)src/api/set/set.ts(1 hunks)src/api/time/time.test.ts(1 hunks)src/composables/use-consent/use-consent.test.ts(1 hunks)src/composables/use-consent/use-consent.ts(1 hunks)src/core/add-configuration.test.ts(1 hunks)src/core/add-configuration.ts(1 hunks)src/core/add-gtag.test.ts(1 hunks)src/core/add-gtag.ts(1 hunks)src/core/add-router-tracking.test.ts(1 hunks)src/core/add-router-tracking.ts(1 hunks)src/core/settings.ts(1 hunks)src/core/track-route.test.ts(1 hunks)src/core/track-route.ts(1 hunks)src/index.test.ts(1 hunks)src/index.ts(1 hunks)tsconfig.json(1 hunks)vite.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (26)
- src/api/exception/exception.ts
- src/api/custom-map/custom-map.ts
- src/api/config/config.test.ts
- src/api/set/set.ts
- src/api/linker/linker.test.ts
- src/api/time/time.test.ts
- src/core/add-router-tracking.ts
- src/api/set/set.test.ts
- src/api/query/query.test.ts
- tsconfig.json
- src/core/add-router-tracking.test.ts
- src/api/linker/linker.ts
- src/api/exception/exception.test.ts
- src/api/ecommerce/ecommerce.test.ts
- src/api/config/config.ts
- src/api/consent/consent.test.ts
- src/index.ts
- src/core/track-route.ts
- src/composables/use-consent/use-consent.ts
- src/api/screenview/screenview.ts
- src/api/opt/opt.test.ts
- src/core/add-gtag.test.ts
- src/api/ecommerce/ecommerce.ts
- src/api/custom-map/custom-map.test.ts
- src/api/pageview/pageview.ts
- src/composables/use-consent/use-consent.test.ts
🧰 Additional context used
🧬 Code Definitions (11)
src/index.test.ts (2)
src/index.ts (1)
createGtag(16-23)src/core/add-gtag.ts (1)
addGtag(6-35)
src/api/query/query.ts (1)
src/core/settings.ts (1)
getSettings(206-208)
src/api/opt/opt.ts (1)
src/core/settings.ts (2)
TagId(125-125)getSettings(206-208)
src/api/screenview/screenview.test.ts (3)
src/core/settings.ts (2)
resetSettings(210-212)updateSettings(214-216)src/api/screenview/screenview.ts (1)
screenview(11-29)src/api/query/query.ts (1)
query(14-30)
src/core/add-gtag.ts (3)
src/core/settings.ts (1)
getSettings(206-208)src/core/add-configuration.ts (1)
addConfiguration(15-59)src/core/add-router-tracking.ts (1)
addRouterTracking(4-24)
src/core/settings.ts (3)
src/api/pageview/pageview.ts (1)
Pageview(5-5)src/api/screenview/screenview.ts (1)
Screenview(4-7)src/api/linker/linker.ts (1)
LinkerParams(3-8)
src/api/event/event.ts (2)
src/core/settings.ts (1)
getSettings(206-208)src/api/query/query.ts (1)
query(14-30)
src/api/event/event.test.ts (3)
src/api/event/event.ts (1)
event(12-20)src/api/query/query.ts (1)
query(14-30)src/core/settings.ts (1)
updateSettings(214-216)
src/core/add-configuration.test.ts (3)
src/core/settings.ts (2)
resetSettings(210-212)updateSettings(214-216)src/core/add-configuration.ts (1)
addConfiguration(15-59)src/api/query/query.ts (1)
query(14-30)
src/api/consent/consent.ts (1)
src/api/query/query.ts (1)
query(14-30)
src/core/add-configuration.ts (4)
src/core/settings.ts (1)
getSettings(206-208)src/api/consent/consent.ts (2)
consentGrantedAll(8-15)consentDeniedAll(17-24)src/api/linker/linker.ts (1)
linker(10-12)src/api/query/query.ts (1)
query(14-30)
🔇 Additional comments (43)
src/api/event/event.ts (1)
1-20: The event function implementation is solid and follows the pattern of other Google Analytics tracking functions.The function correctly handles the logic for determining when to set the
send_toproperty based on additional accounts, which is important for sending events to the right GA properties. The implementation is concise and focused on a single responsibility.src/index.test.ts (1)
1-22: Tests adequately cover the plugin initialization and global property installation.The test suite verifies that:
- The plugin installation calls
addGtag- The global
$gtagproperty is correctly set to the APIThis ensures the Vue plugin works as expected when integrated with applications.
src/core/track-route.test.ts (5)
1-221: Comprehensive test suite covering all aspects of route tracking functionality.The test suite thoroughly verifies the behavior of the
trackRoutefunction under various configurations:
- Screen view tracking
- Page view tracking
- Route exclusion using different methods
- Hook callbacks
- Custom templates
This provides excellent coverage for the tracking functionality.
35-53: Test for screenview tracking is well-structured.This test correctly verifies that when
useScreenviewis enabled, the tracking system sends a screen_view event with the appropriate parameters.
55-67: Test for pageview tracking is well-structured.This test correctly verifies that when screen views are not used (default), the tracking system sends a page_view event with the appropriate parameters including path, location, and title.
69-115: Route exclusion tests cover all supported exclusion methods.Great job testing all three exclusion methods:
- Exclusion by path
- Exclusion by route name
- Exclusion using a custom function
This ensures routes can be properly excluded from tracking when needed.
157-220: Custom template tests are thorough and cover both object and function templates.The tests verify that:
- Static object templates work for page views
- Function-based templates work for page views
- Templates work with screen views
This ensures the template system is flexible and works as expected.
src/api/event/event.test.ts (3)
1-58: Good test coverage for the event function with different configurations.The test suite effectively verifies:
- Basic event tracking functionality
- Group name handling with additional accounts
- Custom group name usage
- Preservation of existing send_to property
This ensures the event tracking system works correctly with various configurations.
16-27: The test for group name handling is correct and necessary.This test verifies an important feature - when multiple tracking accounts are configured, the system correctly adds the group name to the
send_toparameter to ensure events are sent to the appropriate account.
43-57: The test for preserving existing send_to property is important.This test ensures that if a developer explicitly sets a
send_toproperty, the system respects that choice and doesn't override it. This is crucial for allowing manual control when needed.src/api/query/query.ts (1)
1-5: LGTM! Import and type definitions are well-organized.The imports and type definitions are properly organized, using path aliases and relevant type imports.
src/core/add-configuration.ts (5)
7-13: Good implementation of mergeDefaults with sensible defaults.The function correctly preserves user-defined configuration while providing sensible defaults for
send_page_viewandanonymize_ip.
15-28: Function correctly handles missing tagId case.The function properly checks if the tagId is present and exits early if it's not, which prevents unnecessary operations.
42-43: LGTM! Proper initialization of Google Analytics.The code correctly initializes Google Analytics with the standard pattern of first calling "js" with the current date, followed by "config" with the tag ID and configuration.
45-56: LGTM! Proper handling of additional accounts.The function correctly handles multiple Google Analytics accounts, ensuring each one gets its own configuration call with appropriate settings.
30-31: Safe hook execution pattern.The code safely checks for hooks before execution with the optional chaining operator, preventing errors if hooks are undefined.
Also applies to: 58-59
src/api/pageview/pageview.test.ts (4)
6-26: LGTM! Proper test setup with router mock.The test file correctly sets up the testing environment by:
- Mocking the query function
- Resetting settings before each test
- Creating a router with a base path
- Waiting for the router to be ready
- Navigating to a test route
This provides a consistent testing environment for all test cases.
27-85: LGTM! Comprehensive basic pageview testing.The tests thoroughly verify basic pageview functionality:
- Tracking a page path as a string
- Tracking using a route object
- Verifying default properties like page_location and page_title
- Ensuring send_page_view is true by default
All tests have clear assertions that verify the expected behavior.
87-99: LGTM! Testing custom parameters.The test correctly verifies that custom parameters can override the defaults, ensuring flexibility in the API.
101-176: LGTM! Thorough testing of pageTracker options.The tests comprehensively verify that the pageTracker configuration options:
- Work with the router reference
- Support using the full route path with query params and hash
- Support using the router's base path
- Allow changing the default send_page_view value
These tests ensure that all configuration options are working as expected.
src/api/consent/consent.ts (3)
1-6: LGTM! Simple and clear consent function.The
consentfunction correctly passes the consent arguments to the query function, following Google's consent API pattern.
8-15: LGTM! Convenient helper for granting all consent.The
consentGrantedAllfunction provides a useful shorthand for granting all consent types at once, with a sensible default mode.
17-24: LGTM! Convenient helper for denying all consent.The
consentDeniedAllfunction provides a useful shorthand for denying all consent types at once, with a sensible default mode.src/api/opt/opt.ts (1)
1-48: All SSR checks and property assignments look solid.Your implementation correctly addresses previous feedback by utilizing
isServer()checks and deleting the property when enabling tracking. No further issues found.src/api/screenview/screenview.test.ts (4)
1-7: Good module organization and testing setup.The imports and mocking are well-structured, properly separating the query functionality for isolated testing.
8-26: Well-structured test environment with clean router setup.The beforeEach setup correctly initializes the router with predefined routes and properly mocks the router's readiness. This ensures a consistent state for each test case.
28-46: Comprehensive test cases for basic screenview functionality.These tests correctly verify both string-based and object-based parameter handling, ensuring the screenview function properly tracks events with the expected parameters.
48-84: Good coverage of router integration scenarios.The nested describe block thoroughly tests pageTracker integration, including:
- Application name inclusion
- Route name tracking
- Fallback to path when no name is available
This ensures the screenview function works correctly with different route configurations.
vite.config.js (5)
3-3: Good addition of TypeScript declaration generation.The vite-plugin-dts inclusion supports the TypeScript migration by automatically generating declaration files.
20-26: External dependencies properly updated.The configuration correctly adds vue-router as an external dependency and updates the globals mapping.
30-35: TypeScript integration well configured.The DTS plugin is properly configured with appropriate options for type generation.
39-42: Enhanced test configuration.The addition of mockReset and coverage reporting will improve test reliability and provide better insights into test coverage.
14-18:❓ Verification inconclusive
Verify impact of build format changes on existing users.
The build configuration has been significantly modified:
- Entry point changed from JS to TS
- Formats reduced to only ES modules (removed UMD and CommonJS)
- Static filename instead of format-based naming
These changes align with modern practices but may affect compatibility. Please ensure this is documented in the migration guide mentioned in the PR objectives.
🏁 Script executed:
#!/bin/bash # Check for any imports of this library using require() which would be affected rg -g "*.{js,ts,vue}" "require\(['\"]vue-gtag['\"]" || echo "No CommonJS imports found"Length of output: 118
ACTION: Confirm and Document Build Configuration Changes
The updated build configuration in
vite.config.jsnow:
- Changes the entry point from a JavaScript file to a TypeScript file.
- Restricts the output format to only ES modules (removing UMD/CommonJS).
- Uses a static filename (
vue-gtag) rather than a format-based naming convention.A scan for CommonJS imports (using
require('vue-gtag')) returned no results, indicating that within our codebase, there’s no direct dependency on the previously available CommonJS build. However, these changes remain breaking in nature and could affect users consuming the library via UMD or other non-ES methods.Next Steps:
- Documentation: Please ensure that these breaking changes are clearly documented in the migration guide referenced in the PR objectives.
- User Impact: Verify externally (beyond internal scans) that end users are not relying on the removed build formats.
src/core/settings.ts (10)
1-10: Well-organized imports with clear type references.The imports are properly structured and use explicit types from their respective modules.
11-13: Good practice using type aliases for external dependencies.Creating aliases for the vue-router types improves code readability and provides flexibility if the underlying implementation changes.
14-23: Clear type definitions for page tracking.The PageTrackerParams, PageTrackerTemplate, and PageTrackerExclude types are well-defined and provide flexibility for different tracking scenarios.
24-63: Comprehensive PageTracker configuration with excellent documentation.The PageTracker type provides a robust set of options for configuring page tracking behavior, with clear JSDoc comments explaining each property.
65-98: Well-structured hooks with clear documentation.The Hooks type provides a comprehensive set of lifecycle events with proper TypeScript function signatures and descriptive comments.
100-123: Resource configuration with security considerations.The Resource type includes important security features like nonce support for Content Security Policy compliance.
125-192: Comprehensive Settings type with clear documentation.The Settings type provides a complete configuration surface with well-documented options. The JSDoc comments explain each property clearly.
194-195: Strong type constraints for plugin settings.The PluginSettings type ensures that the tagId is required while making other settings optional, enforcing proper initialization.
197-204: Sensible default settings with appropriate immutability.The defaultSettings object uses Readonly to prevent accidental modification, and provides reasonable defaults for required properties.
206-216: Clean settings management API.The getter and update functions provide a simple and intuitive interface for managing settings, with proper type safety.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/core/settings.ts (2)
127-203: Well-structured settings type.
The comprehensiveSettingsinterface covers essential analytics configuration points (resource URLs, dataLayer, hooks, etc.). The doc comments are highly informative. Consider adding JSDoc for any optional fields that might need special handling (e.g., clarifying defaults).
216-229: Efficient state management withgetSettings,resetSettings, andupdateSettings.
Merging configuration viadeepMergeis a powerful approach for complex objects. Ensure that deeply nested arrays or objects are handled as intended (e.g., overwriting vs. merging). A dedicated test can prevent regressions.src/core/add-gtag.ts (1)
24-35: Robust error handling on script injection.
Catching and propagating the error tohooks?.["script:error"]helps surface failures to the consumer. A fallback or retry might be valuable if script loading is critical.src/composables/use-consent/use-consent.ts (3)
17-19: Consider a more robust method for consent detection.The current implementation uses a simple
includes()check on document.cookie, which might lead to false positives if another cookie contains the substring "_ga". A more precise approach would check for the exact cookie name.const hasConsent = ref<boolean>( - isServer() ? false : document.cookie.includes(GA_COOKIE_VALUE), + isServer() ? false : !!document.cookie + .split('; ') + .find(row => row.startsWith(`${GA_COOKIE_VALUE}=`)) );
27-29: Add error handling to rejectAll method.The
rejectAllmethod lacks error handling, which could cause silent failures if the consent operation encounters issues.const rejectAll = () => { - consentDeniedAll("update"); + try { + consentDeniedAll("update"); + } catch (error) { + console.error("Failed to reject all consent:", error); + } };
16-47: Consider adding a way to update consent status without page reloads.The current implementation doesn't provide a way to update the
hasConsentref when the cookie status changes during the application lifecycle without reloading the page. This could lead to inconsistencies between the actual consent state and what's reflected in the application.You could add a method to check the current consent status and update the ref accordingly:
export function useConsent(): UseWithConsentReturn { const hasConsent = ref<boolean>( isServer() ? false : document.cookie.includes(GA_COOKIE_VALUE), ); + // Function to update the current consent status + const updateConsentStatus = () => { + if (!isServer()) { + hasConsent.value = document.cookie.includes(GA_COOKIE_VALUE); + } + }; + + // Consider using this instead of page reloads + // and potentially set up a cookie change listener if needed // Rest of the implementation... return { hasConsent, acceptAll, rejectAll, acceptCustom, + updateConsentStatus, }; }Additionally, consider implementing a cookie change listener or integrating with a cookie consent manager if your application uses one.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/composables/use-consent/use-consent.test.ts(1 hunks)src/composables/use-consent/use-consent.ts(1 hunks)src/core/add-gtag.test.ts(1 hunks)src/core/add-gtag.ts(1 hunks)src/core/settings.test.ts(1 hunks)src/core/settings.ts(1 hunks)src/index.test.ts(1 hunks)src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/settings.test.ts
- src/composables/use-consent/use-consent.test.ts
- src/core/add-gtag.test.ts
🧰 Additional context used
🧬 Code Definitions (5)
src/core/add-gtag.ts (3)
src/core/settings.ts (1)
getSettings(218-220)src/core/add-configuration.ts (1)
addConfiguration(15-59)src/core/add-router-tracking.ts (1)
addRouterTracking(4-24)
src/index.test.ts (3)
src/core/settings.ts (1)
resetSettings(222-224)src/index.ts (2)
createGtag(30-38)addGtag(43-43)src/core/add-gtag.ts (1)
addGtag(6-35)
src/composables/use-consent/use-consent.ts (2)
src/core/add-gtag.ts (1)
addGtag(6-35)src/api/consent/consent.ts (3)
consentGrantedAll(8-15)consentDeniedAll(17-24)consent(4-6)
src/index.ts (2)
src/core/settings.ts (3)
getSettings(218-220)PluginSettings(205-206)updateSettings(226-228)src/core/add-gtag.ts (1)
addGtag(6-35)
src/core/settings.ts (3)
src/api/pageview/pageview.ts (1)
Pageview(5-5)src/api/screenview/screenview.ts (1)
Screenview(4-7)src/api/linker/linker.ts (1)
LinkerParams(3-8)
🔇 Additional comments (13)
src/core/settings.ts (2)
1-9: Imports are clear and organized.
All imported types and utilities appear relevant and properly referenced, ensuring good modular separation.
208-215: Default settings are concise and safe.
Storing a small set of essential defaults indefaultSettingshelps ensure stable behavior in the absence of user overrides. Good practice!src/index.test.ts (3)
9-16: Clear testing of plugin installation.
EnsuringaddGtagis called upon plugin creation (wheninitModeis not "manual") verifies correct default behavior. The test is concise and informative.
17-25: Manual installation test is comprehensive.
The sequence checks thataddGtagisn't invoked initially, then called explicitly viainitialize(), which matches theinitMode: "manual"specification.
27-34: Verifying$gtagglobal property.
The test ensures that$gtagis properly attached toapp.config.globalProperties, confirming the plugin's integration is as expected.src/core/add-gtag.ts (2)
6-11: Graceful handling of missing tag ID.
Early return whentagIdis absent prevents unnecessary script injection or router tracking, which is a good defensive measure.
19-23: Double-injection prevention logic is clear.
By checkinghasGtag()before injecting the script, you avoid reloading or overwriting an already loaded gtag instance. This is a good practice for idempotency.src/index.ts (3)
1-2: Re-exporting API and theaddGtagfunction enhances discoverability.
By centralizing exports, you make it easier for consumers to import what they need. Nicely organized.
20-28: Conditional gtag initialization withhandleGtag().
initMode === "manual"check ensures the library does not auto-load if the user explicitly wants to invokeaddGtag. This is a nifty separation of concerns.
30-38: [Duplicate suggestion about awaitingaddGtag]- handleGtag(); + void handleGtag(); // or await handleGtag() in an async contextIf subsequent operations depend on gtag being fully loaded, consider awaiting for reliability. Since this mirrors a prior reviewer comment, marking as duplicate.
src/composables/use-consent/use-consent.ts (3)
21-25: Avoid page reloads and add error handling.Reloading the page in the
acceptAllmethod disrupts the user experience and loses application state. Additionally, there's no error handling for the asynchronous operations.const acceptAll = async () => { - await addGtag(); - consentGrantedAll("update"); - window.location.reload(); + try { + await addGtag(); + consentGrantedAll("update"); + // If reload is absolutely necessary, add a comment explaining why + // window.location.reload(); + } catch (error) { + console.error("Failed to accept all consent:", error); + } };
31-35: Avoid page reloads and add error handling for acceptCustom.Similar to
acceptAll, theacceptCustommethod unnecessarily reloads the page and lacks error handling.const acceptCustom = async (params: GtagConsentParams) => { - await addGtag(); - consent("update", params); - window.location.reload(); + try { + await addGtag(); + consent("update", params); + // If reload is absolutely necessary, add a comment explaining why + // window.location.reload(); + } catch (error) { + console.error("Failed to accept custom consent:", error); + } };
37-39: Add error handling around initial addGtag call.The initial call to
addGtagwhenhasConsent.valueis true lacks error handling, which could lead to unhandled promise rejections.if (hasConsent.value) { - addGtag(); + // Initialize Google Analytics if consent is already granted + addGtag().catch(error => { + console.error("Failed to initialize Google Analytics:", error); + }); }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
45-47: Potential Redundancy with Legacy Fields.
The file still includes"main","module", and"types"fields even though an"exports"field is defined. While these may be kept for backward compatibility, it might be useful to add an inline comment clarifying their purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package.json(3 hunks)
🔇 Additional comments (14)
package.json (14)
5-5: Version Bump Update.
The version field was updated to"3.0.0-beta.19", which correctly reflects the major release bump per the PR objectives.
18-18: Updated Lint Command.
The"lint"script is now set to"biome lint", replacing an older check command. This better aligns with the current tooling.
19-19: Enhanced Build Process Validation.
The"build"script now runs"tsc --noEmit && vite build", ensuring that type errors are caught before executing the build step. This extra check improves build robustness.
20-20: Addition of Dedicated Type Check.
Introducing the"typecheck"script ("tsc --noEmit") provides an isolated way to verify type integrity. This is a valuable addition for maintaining type safety.
22-22: New Test Coverage Script.
The"test:cov"script ("vitest run --coverage") facilitates runtime test coverage analysis, supporting ongoing quality assurance efforts.
24-24: Revised Prepublish Process.
The"prepublishOnly"script has been updated to:"npm run lint && npm run test:once && npm run build"This change replaces the earlier use of
"yarn check"with"npm run lint", ensuring consistency with the rest of the npm-based scripts. Please verify that switching to npm here is intentional.
39-44: Modernized Exports Configuration.
The introduction of the"exports"field now explicitly defines the import path and types file:"exports": { ".": { "types": "./dist/vue-gtag.d.ts", "import": "./dist/vue-gtag.js" } }This modern approach improves module resolution for consumers.
49-49: Streamlined Files Inclusion.
Limiting the"files"array to just"dist"ensures that only the necessary build artifacts are published, reducing package bloat.
56-57: Updated Peer Dependencies.
The peer dependencies now require:
"vue": "^3.5.13""vue-router": "^4.5.0"These updates align with the latest framework versions and intended compatibility improvements.
61-62: Enhanced Dev Dependency Versions.
Adding"@types/node": "^22.13.5"and"@vitest/coverage-v8": "^3.0.5"ensures that the development environment is up-to-date and leverages current type definitions and test coverage tools.
66-66: Inclusion of Terser for Code Minification.
The addition of"terser": "^5.39.0"will help optimize and minify the code during builds. Confirm that this version meets your performance and compatibility requirements.
68-68: Introducing Vite Plugin for DTS Generation.
The new dev dependency"vite-plugin-dts": "^4.5.0"enhances type declaration generation, improving the development experience and maintaining robust type information.
70-70: Aligned Vue Version in Dev Dependencies.
Updating"vue"in the dev dependencies to"^3.5.13"aligns it with the peer dependency, avoiding potential version conflicts during development.
73-77: Optional Peer Dependency Configuration.
The newly added"peerDependenciesMeta"field marks"vue-router"as optional:"peerDependenciesMeta": { "vue-router": { "optional": true } }This configuration provides flexibility for users who may not need routing capabilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (12)
src/core/add-router-tracking/add-router-tracking.ts (1)
4-24: Good implementation of router tracking with proper flow control.The implementation correctly handles router tracking by:
- Checking if router is available
- Waiting for router to be ready before tracking the initial route
- Setting up tracking for future route changes
- Avoiding duplicate tracking when path hasn't changed
Consider adding JSDoc documentation to explain the purpose and behavior of this function, particularly noting that it automatically integrates with Vue Router:
+/** + * Sets up automatic route tracking with Vue Router. + * + * This function retrieves router configuration from settings, + * tracks the initial route, and sets up tracking for all subsequent + * route changes, while avoiding duplicate tracking for the same path. + * + * @returns A promise that resolves when router tracking is set up + */ export async function addRouterTracking(): Promise<void> {src/core/settings/settings.test.ts (1)
3-27: Test coverage is good but could benefit from better test isolation.The tests correctly verify both the default configuration and the update functionality.
Consider resetting the settings after the second test to ensure test isolation and prevent potential side effects in other tests:
it("should update the default configuration", () => { updateSettings({ tagId: "UA-1234567", gtagName: "foo", }); expect(getSettings()).toEqual( expect.objectContaining({ tagId: "UA-1234567", gtagName: "foo", }), ); + + // Reset settings after test to ensure isolation + resetSettings(); });src/core/add-configuration/add-configuration.ts (1)
15-59: Comprehensive configuration implementation but missing return type and error handling.The function correctly handles all aspects of configuration including consent mode, linker options, and additional accounts.
Consider these improvements:
- Add a return type annotation to the function
- Add JSDoc documentation to explain the function's purpose
- Implement error handling for potential failures during configuration
/** * Initializes and configures Google Analytics tracking based on current settings. * * Sets up consent mode, linker for cross-domain tracking, and configures * both the main tag ID and any additional accounts. * * @throws Potential errors from the underlying query functions are not handled * @returns void */ -export function addConfiguration() { +export function addConfiguration(): void {Additionally, consider wrapping the query calls in try/catch blocks to handle potential errors:
- query("js", new Date()); - query("config", tagId, mergeDefaults(config)); + try { + query("js", new Date()); + query("config", tagId, mergeDefaults(config)); + } catch (error) { + console.error("Failed to initialize Google Analytics:", error); + hooks?.["config:init:error"]?.(error); + }src/core/settings/settings.ts (1)
12-24: Well-structured settings module with appropriate immutability patterns.The implementation correctly handles settings management with getters, setters, and reset functionality. The use of deep merge for updates is appropriate for handling nested configuration objects.
Consider adding JSDoc documentation to clarify the purpose of each function:
+/** + * Returns the current settings configuration. + * @returns The current settings object + */ export function getSettings(): Settings { return settings; } +/** + * Resets all settings to their default values. + */ export function resetSettings(): void { settings = { ...defaultSettings }; } +/** + * Updates the current settings by deeply merging the provided configuration. + * @param configParams Partial settings to merge with current configuration + */ export function updateSettings(configParams: Partial<Settings>): void { settings = deepMerge(settings, configParams); }src/core/add-gtag/add-gtag.ts (3)
20-22: The function should maintain its Promise chain correctlyWhen gtag is already loaded, the function returns early but still needs to return a resolved Promise to maintain consistent async behavior.
if (hasGtag()) { hooks?.["script:loaded"]?.(); - return; + return Promise.resolve(); }
31-31: Consider awaiting hooks if they might be asyncThe script loaded hook might be asynchronous, but it's not being awaited. If the hook performs asynchronous operations, they might not complete before the function returns.
- hooks?.["script:loaded"]?.(); + await hooks?.["script:loaded"]?.();
33-33: Consider awaiting error hooks if they might be asyncSimilar to the loaded hook, the error hook might be asynchronous, but it's not being awaited.
- hooks?.["script:error"]?.(error); + await hooks?.["script:error"]?.(error);src/core/track-route/track-route.ts (4)
20-22: Simplify the optional chaining in array methodsThe optional chaining on
pageTracker.exclude?.some()is redundant since we've already checked for!pageTracker?.excludeon line 12.- return pageTracker.exclude?.some(({ name, path } = {}) => { + return pageTracker.exclude.some(({ name, path } = {}) => { return (name && name === route.name) || (path && path === route.path); });
32-32: Consider awaiting hooks if they might be asyncIf the
router:track:beforehook might perform async operations, you should await it to ensure they complete before continuing with tracking.- hooks?.["router:track:before"]?.(route); + await hooks?.["router:track:before"]?.(route);
55-55: Consider awaiting hooks if they might be asyncSimilar to the before hook, if the
router:track:afterhook might perform async operations, you should await it.- hooks?.["router:track:after"]?.(route); + await hooks?.["router:track:after"]?.(route);
43-53: Consider using type guards for cleaner conditional logicThe current approach of using
inoperator for type checking works, but type guards could make the code more readable and type-safe.You could define type guards:
function isScreenviewParams(params: any): params is Screenview { return params && 'screen_name' in params; } function isPageviewParams(params: any): params is Pageview { return params && 'page_path' in params; }Then use them in your conditional logic:
if (pageTracker?.useScreenview) { - const screenviewParams = - template && "screen_name" in template ? template : route; + const screenviewParams = template && isScreenviewParams(template) + ? template + : route; screenview(screenviewParams); } else { - const pageviewParams = - template && "page_path" in template ? template : route; + const pageviewParams = template && isPageviewParams(template) + ? template + : route; pageview(pageviewParams); }src/core/add-configuration/add-configuration.test.ts (1)
115-150: Consider additional scenarios for partial consent modes.
The current tests thoroughly cover the “granted” and “denied” modes. If future requirements include partially granted consent (e.g., analytics only), consider adding tests to cover those scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
src/core/add-configuration/add-configuration.test.ts(1 hunks)src/core/add-configuration/add-configuration.ts(1 hunks)src/core/add-configuration/index.ts(1 hunks)src/core/add-gtag/add-gtag.test.ts(1 hunks)src/core/add-gtag/add-gtag.ts(1 hunks)src/core/add-gtag/index.ts(1 hunks)src/core/add-router-tracking/add-router-tracking.test.ts(1 hunks)src/core/add-router-tracking/add-router-tracking.ts(1 hunks)src/core/add-router-tracking/index.ts(1 hunks)src/core/settings/index.ts(1 hunks)src/core/settings/settings.test.ts(1 hunks)src/core/settings/settings.ts(1 hunks)src/core/settings/types.ts(1 hunks)src/core/track-route/index.ts(1 hunks)src/core/track-route/track-route.test.ts(1 hunks)src/core/track-route/track-route.ts(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- src/core/add-gtag/index.ts
- src/core/track-route/index.ts
- src/core/add-router-tracking/index.ts
- src/core/add-configuration/index.ts
- src/core/settings/index.ts
🧰 Additional context used
🧬 Code Definitions (8)
src/core/add-router-tracking/add-router-tracking.ts (2)
src/core/settings/settings.ts (1)
getSettings(14-16)src/core/track-route/track-route.ts (1)
trackRoute(25-56)
src/core/add-configuration/add-configuration.ts (1)
src/core/settings/settings.ts (1)
getSettings(14-16)
src/core/add-gtag/add-gtag.ts (3)
src/core/settings/settings.ts (1)
getSettings(14-16)src/core/add-configuration/add-configuration.ts (1)
addConfiguration(15-59)src/core/add-router-tracking/add-router-tracking.ts (1)
addRouterTracking(4-24)
src/core/settings/settings.test.ts (1)
src/core/settings/settings.ts (2)
getSettings(14-16)updateSettings(22-24)
src/core/add-configuration/add-configuration.test.ts (2)
src/core/settings/settings.ts (2)
resetSettings(18-20)updateSettings(22-24)src/core/add-configuration/add-configuration.ts (1)
addConfiguration(15-59)
src/core/settings/settings.ts (1)
src/core/settings/types.ts (1)
Settings(126-202)
src/core/track-route/track-route.ts (2)
src/core/settings/types.ts (2)
Route(11-11)PageTrackerParams(13-13)src/core/settings/settings.ts (1)
getSettings(14-16)
src/core/add-gtag/add-gtag.test.ts (4)
src/core/settings/settings.ts (2)
resetSettings(18-20)updateSettings(22-24)src/core/add-gtag/add-gtag.ts (1)
addGtag(6-35)src/core/add-configuration/add-configuration.ts (1)
addConfiguration(15-59)src/core/add-router-tracking/add-router-tracking.ts (1)
addRouterTracking(4-24)
🔇 Additional comments (12)
src/core/add-configuration/add-configuration.ts (1)
7-13: Good pattern for setting sensible defaults while preserving custom configuration.The
mergeDefaultsfunction correctly provides default values while allowing custom configuration to override them.src/core/settings/settings.ts (1)
4-10: Good practice using a readonly constant for default settings.The default settings are properly defined as a readonly constant with sensible default values.
src/core/add-router-tracking/add-router-tracking.test.ts (1)
1-92: Test coverage looks comprehensive and well-structuredThe test suite effectively covers the three main scenarios for router tracking:
- Initial tracking when the router is ready
- Tracking after a route change
- Skipping tracking for identical paths
The mocking approach and assertions are clear and verify the expected behavior.
src/core/add-gtag/add-gtag.test.ts (1)
1-196: Test suite is thorough and comprehensiveThe test suite covers all major scenarios including:
- Basic library download
- Avoiding duplicate downloads
- Custom resource handling
- Callback execution
- Configuration initialization
- Router tracking initialization
- Missing tag ID handling
- Script attribute verification (preconnect, defer, nonce)
src/core/add-configuration/add-configuration.test.ts (2)
1-9: Great setup for the test suite.
All the initial imports, mocking ofquery, and date handling withmockdateare well-structured and straightforward. No changes needed.
10-112: Solid coverage for configuration behaviors and hooks.
These tests comprehensively validate multiple scenarios—initial configuration, additional accounts, and linker parameters—while also verifying hooks forconfig:init:beforeandconfig:init:after. They look well-structured and logically sound.src/core/track-route/track-route.test.ts (4)
1-34: Good test environment setup.
The imports, router initialization, and mocking ofquerycreate a clear foundation for verifying route tracking.
35-67: Thorough screenview and pageview coverage.
These tests demonstrate correct event parameters for both screenview and pageview tracking. The logic appears accurate.
69-115: Robust route exclusion tests.
Exclusions based on path, name, and custom functions work as intended. No issues identified.
117-220: Flexible handling of callbacks and custom templates.
Tracking hooks (before,after) and custom templates (both static and function-based) are thoroughly covered. This ensures a wide range of tracking customizations.src/core/settings/types.ts (2)
1-97: Well-defined type structures for page tracking and hooks.
ThePageTrackerandHookstypes clearly capture the configurable parameters and user-defined callbacks. All fields appear logically grouped.
99-206: Comprehensive settings definition.
TheSettingsandPluginSettingstypes provide a robust backbone for the analytics configuration, including multi-account setups, consent modes, and plugin extensibility. This is clean and maintainable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/core/create-gtag/create-gtag.test.ts (1)
1-30: Good test coverage, consider adding more edge casesThe test file has a good structure and basic coverage for the main functionality of
createGtag. The three test cases validate the core behaviors: initialization, Vue integration, and manual mode skipping initialization.Consider adding these additional test cases for more comprehensive coverage:
- Error handling when
tagIdis missing or invalid- Testing with various configuration options (e.g., different
initModevalues)- Testing the behavior when called multiple times (idempotence)
it("should not initialize gtag when tagId is missing", () => { createGtag({}); expect(addGtag).not.toHaveBeenCalled(); }); it("should support different initMode values", () => { createGtag({ tagId: "UA-123456789", initMode: "onLoad" }); expect(addGtag).toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/core/add-gtag/add-gtag.ts(1 hunks)src/core/create-gtag/create-gtag.test.ts(1 hunks)src/core/create-gtag/create-gtag.ts(1 hunks)src/core/create-gtag/index.ts(1 hunks)src/core/index.ts(1 hunks)src/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/core/create-gtag/index.ts
- src/core/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/index.ts
- src/core/add-gtag/add-gtag.ts
🧰 Additional context used
🧬 Code Definitions (1)
src/core/create-gtag/create-gtag.ts (1)
src/core/add-gtag/add-gtag.ts (1)
addGtag(10-39)
Issue fixed or features implemented:
closes #607
closes #535
closes #533
closes #393
closes #615
closes #589
closes #334
closes #377
closes #566
closes #526
Summary by CodeRabbit
New Features
Chores