refactor(tracker): simplify config value parsing#519
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR refactors
Confidence Score: 4/5Safe to merge; the refactor is a straightforward extraction of two helpers with no logic changes for the common paths. The only behavioural difference is how empty URL query-param values are handled, which is a rare edge case. The packages/tracker/src/core/utils.ts — specifically the URL-param parsing loop that now calls Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[getTrackerConfig] --> B{window defined?}
B -- No --> C[return clientId: '']
B -- Yes --> D[Spread globalConfig into config]
D --> E{script element found?}
E -- No --> J[return config]
E -- Yes --> F[Iterate data-* attributes]
F --> G{key === skipPatterns\nor maskPatterns?}
G -- Yes --> H[parseJsonAttribute\nJSON.parse or fallback to array]
G -- No --> I[parseConfigValue\ntrue / false / number / string]
H --> F
I --> F
F --> K[Parse script src URL params]
K --> L[parseConfigValue for each param]
L --> J
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[getTrackerConfig] --> B{window defined?}
B -- No --> C[return clientId: '']
B -- Yes --> D[Spread globalConfig into config]
D --> E{script element found?}
E -- No --> J[return config]
E -- Yes --> F[Iterate data-* attributes]
F --> G{key === skipPatterns\nor maskPatterns?}
G -- Yes --> H[parseJsonAttribute\nJSON.parse or fallback to array]
G -- No --> I[parseConfigValue\ntrue / false / number / string]
H --> F
I --> F
F --> K[Parse script src URL params]
K --> L[parseConfigValue for each param]
L --> J
Reviews (1): Last reviewed commit: "refactor(tracker): simplify config value..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function parseConfigValue(value: string): boolean | number | string { | ||
| if (value === "true" || value === "") { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
parseConfigValue is applied to URL query-param values for the first time in this PR. In the old code, a ?key= param (empty value) would fall through to the final else branch and be kept as the string "". Now it returns true. If any consumer ever passes an empty param value expecting a string "", they'll silently get a boolean true instead. Consider guarding the empty-string → true coercion behind a flag, or at least documenting the intentional alignment.
| function parseConfigValue(value: string): boolean | number | string { | |
| if (value === "true" || value === "") { | |
| return true; | |
| } | |
| function parseConfigValue( | |
| value: string, | |
| treatEmptyAsTrue = false, | |
| ): boolean | number | string { | |
| if (value === "true" || (treatEmptyAsTrue && value === "")) { | |
| return true; | |
| } |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Confidence score: 3/5
- In
packages/tracker/src/core/utils.ts, URL-param coercion now treats empty values astrue, so cases likeclientId=can become a boolean and break tracker initialization at runtime; merging as-is risks a user-facing regression for integrations that pass empty query params — preserve empty-string values while keeping boolean-attribute behavior, and add a regression test forclientId=before merging.
Architecture diagram
sequenceDiagram
participant Init as Script Initializer
participant GetConfig as getTrackerConfig()
participant Global as window.databuddyConfig
participant Script as <script> element
participant URL as script.src URL
participant ParseValue as parseConfigValue()
participant ParseJSON as parseJsonAttribute()
participant Config as config object
Note over Init,Config: Config parsing with new shared coercion
Init->>GetConfig: getTrackerConfig()
GetConfig->>Global: read global config
Global-->>GetConfig: globalConfig record
GetConfig->>Config: config = { clientId: "", ...globalConfig }
alt Script element exists
GetConfig->>Script: iterate attributes
loop For each attribute
alt name starts with "data-"
Script->>GetConfig: attr.name, attr.value
GetConfig->>GetConfig: extract key (camelCase)
alt key is "skipPatterns" or "maskPatterns"
GetConfig->>ParseJSON: parseJsonAttribute(value)
ParseJSON->>ParseJSON: try JSON.parse
alt success
ParseJSON-->>GetConfig: parsed array
else fail
ParseJSON-->>GetConfig: []
end
GetConfig->>Config: config[key] = result
else scalar attribute
GetConfig->>ParseValue: parseConfigValue(value)
alt value === "true" or ""
ParseValue-->>GetConfig: true
else value === "false"
ParseValue-->>GetConfig: false
else /^\d+$/.test(value)
ParseValue-->>GetConfig: Number(value)
else
ParseValue-->>GetConfig: value as string
end
GetConfig->>Config: config[key] = result
end
end
end
end
alt Script has valid src URL
GetConfig->>URL: new URL(script.src)
URL->>URL: extract search params
loop For each search param
URL->>GetConfig: key, value
GetConfig->>ParseValue: parseConfigValue(value)
ParseValue-->>GetConfig: coerced value
GetConfig->>Config: config[key] = coerced value
end
end
GetConfig-->>Init: merged TrackerOptions config
Note over Init,Config: Precedence: global < data-* < URL params
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
c04fe29 to
31e9067
Compare
|
Subagent review caught one behavior regression before merge: empty URL query params were being coerced like empty |
|
The latest updates on your projects. Learn more about Unkey Deploy
|
1 participant
Summary
anywrites from tracker config parsingdata-*attributes and script URL query paramsVerification
bunx ultracite check packages/tracker/src/core/utils.tscd packages/tracker && bun run check-typescd packages/tracker && bun run testbun run check-typesbun run lintbun run testAI disclosure
Summary by cubic
Simplifies tracker config parsing by centralizing value coercion and removing unsafe
anywrites. Shared parsers handle booleans, numbers, and JSON for bothdata-*attributes and script URL params, preserving empty strings in URL params and treating emptydata-*values as true, with precedence unchanged.parseConfigValueandparseJsonAttributefor consistent coercion.anymaps; tightened config typing.Written for commit 31e9067. Summary will update on new commits.