Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughReplaces single-query-param resolution with support for multiple query params: script reads Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as User
participant Page as Web Page
participant Web as web/generic.ts
participant Tag as Script Tag (DOM)
participant Script as script/base.js
participant URL as Window.location
Note over Web,Tag: Web layer may set data-query-param and/or data-query-params
User->>Page: Load page
Page->>Web: Render Analytics component (props)
alt props.queryParams provided
Web->>Tag: set data-query-params = JSON.stringify(props.queryParams)
end
opt props.queryParam provided
Web->>Tag: set data-query-param = props.queryParam
end
Page->>Script: Execute analytics script
Script->>Tag: Read data-query-params / data-query-param
Script->>Script: Compute QUERY_PARAMS (parse, fallback, dedupe, default ['via'])
Script->>URL: Scan query string for first matching param in QUERY_PARAMS
alt Found
Script->>Script: QUERY_PARAM_VALUE = first found value
else None
Script->>Script: QUERY_PARAM_VALUE = null
end
Script->>Window: Expose analytics surface (includes QUERY_PARAMS and qm)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/web/src/generic.ts (1)
82-84: LGTM! Consider documenting the precedence when both properties are set.The code correctly sets
data-query-paramswhenprops.queryParamsis provided. However, since bothqueryParamandqueryParamscan be set simultaneously (lines 78-80 and 82-84), and the script layer gives precedence todata-query-param, it may be helpful to document this behavior to avoid user confusion during the migration from the deprecatedqueryParamtoqueryParams.packages/web/src/types.ts (1)
146-146: Consider enhancing the deprecation notice with precedence details.The deprecation message correctly directs users to
queryParams, but could benefit from clarifying that if bothqueryParamandqueryParamsare set simultaneously,queryParamtakes precedence (based on the script layer's implementation).Apply this diff to improve the deprecation notice:
- * @deprecated Use queryParams instead + * @deprecated Use queryParams instead. Note: if both are set, queryParam takes precedence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/script/src/base.js(2 hunks)packages/web/src/generic.ts(1 hunks)packages/web/src/types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/web/src/generic.ts (1)
packages/script/src/base.js (1)
script(3-3)
🔇 Additional comments (4)
packages/script/src/base.js (3)
66-90: LGTM! Clear fallback hierarchy and good error handling.The resolution logic correctly implements the priority:
data-query-param→data-query-params→ default['via'], with deduplication viaSet. The warning on parse failure is appropriate.
92-102: LGTM! First-match strategy is correct.The loop returns the first query parameter with a non-null value, providing a sensible fallback mechanism across multiple candidates.
306-306: No internal references to_dubAnalytics.pwere found; verify downstream consumers handle the change from a string to an array.packages/web/src/types.ts (1)
150-155: LGTM! Clear documentation of the new property.The
queryParamsproperty is well-documented with an appropriate default value and clear usage examples from the existingqueryParamdocumentation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/script/src/base.js (3)
73-87: Clarify behavior when both attributes are present.The
else iflogic means onlydata-query-paramis processed when bothdata-query-paramanddata-query-paramsare present. While the web layer types deprecatequeryParamin favor ofqueryParams, the script should handle edge cases where both attributes exist.Consider documenting this behavior or handling both attributes by merging them instead of mutual exclusion.
Apply this diff to merge both attributes when present:
- if (queryParam) { + if (queryParam && !queryParams) { resolvedQueryParams = [queryParam, ...resolvedQueryParams]; - } else if (queryParams) { + } else if (queryParams && !queryParam) { try { resolvedQueryParams = [ ...JSON.parse(queryParams), ...resolvedQueryParams, ]; } catch (error) { console.warn( '[dubAnalytics] Failed to parse data-query-params.', error, ); } + } else if (queryParam && queryParams) { + try { + resolvedQueryParams = [ + queryParam, + ...JSON.parse(queryParams), + ...resolvedQueryParams, + ]; + } catch (error) { + console.warn( + '[dubAnalytics] Failed to parse data-query-params.', + error, + ); + resolvedQueryParams = [queryParam, ...resolvedQueryParams]; + } }Alternatively, if mutual exclusivity is intentional, add a warning when both are present to guide developers toward the new API.
82-85: Include the failed value in the error message.When JSON parsing fails, logging the attempted value would aid debugging.
Apply this diff:
} catch (error) { console.warn( - '[dubAnalytics] Failed to parse data-query-params.', + `[dubAnalytics] Failed to parse data-query-params: "${queryParams}".`, error, ); }
92-102: Optimize redundantparams.get()calls.The current logic calls
params.get(param)twice per iteration—once to check and once to return. Store the result to avoid redundant lookups.Apply this diff:
const QUERY_PARAM_VALUE = (() => { const params = new URLSearchParams(location.search); for (const param of QUERY_PARAMS) { - if (params.get(param)) { - return params.get(param); + const value = params.get(param); + if (value) { + return value; } } return null; })();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/script/src/base.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/script/src/base.js (2)
packages/script/src/extensions/outbound-domains.js (1)
cookieManager(3-8)packages/script/src/extensions/site-visit.js (1)
cookieManager(3-10)
🔇 Additional comments (1)
packages/script/src/base.js (1)
298-308: Confirm external dependencies on removed window._dubAnalytics properties
Internal search shows no references to the removedp,m, oroexports. If any external integrations or published extensions rely on these properties, update them accordingly; otherwise the removal is safe.
|
@coderabbitai final review, any breaking changes? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 210 🏁 Script executed: Length of output: 3754 @devkiran: Here's the final review focusing on breaking changes:
|
2 participants
Summary by CodeRabbit
New Features
Deprecations
Documentation
Bug Fixes
Chores