analytics: combine copy feature tracking events into one#708
Draft
ambergristle wants to merge 5 commits intovikejs:mainfrom
Draft
analytics: combine copy feature tracking events into one#708ambergristle wants to merge 5 commits intovikejs:mainfrom
copy feature tracking events into one#708ambergristle wants to merge 5 commits intovikejs:mainfrom
Conversation
📝 WalkthroughWalkthroughCLI hook file-ignore regex was commented out; tracking consolidated into a single Changes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🧹 Nitpick comments (1)
packages/cli/index.ts (1)
435-435: Remove commented-out dead code instead of leaving it in place.The
reIgnoreFileregex was unused here because thewalkutility from@batijs/buildalready applies this same filter internally (seepackages/build/src/index.ts:53). Rather than leaving this as a comment, delete the line entirely to keep the codebase clean.🧹 Proposed fix
async function retrieveHooks(hooks: Map<BoilerplateDefWithConfig, string>): Promise<Map<"after", Hook[]>> { - // const reIgnoreFile = /^(chunk-|asset-|#)/gi; const map = new Map<"after", Hook[]>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/index.ts` at line 435, Remove the dead commented-out regex declaration by deleting the line containing the commented `reIgnoreFile` variable in packages/cli/index.ts; this unused `// const reIgnoreFile = /^(chunk-|asset-|#)/gi;` should be removed because the `walk` utility in `@batijs/build` already handles this filtering, so no other changes are required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@website/components/InputGroup.tsx`:
- Around line 45-48: The reduce over selectedFeaturesFlags() (currently
producing const flags) overwrites flags when multiple selections share the same
category; change the accumulator so each category maps to an array of flags
instead of a single string: in the reduce callback for selectedFeaturesFlags()
check if agg[category] exists, push the current flag into the array or
initialize it with [flag], and return agg (so functions/components consuming
flags will get arrays per category rather than single values); update any
downstream usage of flags to handle arrays (or normalize to strings where
appropriate).
In `@website/components/Store.tsx`:
- Around line 32-35: selectedFeaturesFlags() currently maps features to objects
({ flag, category }) which breaks consumers like InputGroup.tsx that expect
strings and do `--${flag}`; change the mapping in selectedFeaturesFlags (the map
call shown) to return the flag string (e.g., f.flag) so consumers still receive
an array of flag string values, or alternatively update InputGroup.tsx to read
the flag property (e.g., use f.flag) if you prefer returning objects — pick one
approach and make all callers consistent (adjust selectedFeaturesFlags or
InputGroup.tsx references accordingly).
---
Nitpick comments:
In `@packages/cli/index.ts`:
- Line 435: Remove the dead commented-out regex declaration by deleting the line
containing the commented `reIgnoreFile` variable in packages/cli/index.ts; this
unused `// const reIgnoreFile = /^(chunk-|asset-|#)/gi;` should be removed
because the `walk` utility in `@batijs/build` already handles this filtering, so
no other changes are required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 963199ef-4ad1-402d-87fc-2abf81505797
📒 Files selected for processing (4)
packages/cli/index.tswebsite/components/InputGroup.tsxwebsite/components/Store.tsxwebsite/lib/track.ts
ambergristle
changed the title
copy feature tracking events into one
There was a problem hiding this comment.
🧹 Nitpick comments (1)
website/lib/track.ts (1)
35-64: RefactorformatFeatureFlagsto the two-phase aggregation pattern.Line 40 uses a truthy check (
if (data[category])), and the mixedstring | string[]accumulator requires a final type assertion. A two-phaseRecord<string, string[]>→Record<string, string>transform is simpler and more robust.♻️ Proposed refactor
export function formatFeatureFlags(selectedFlags: FeatureFlag[]): Record<string, string> { - const categoriesMultiple = new Set<string>(); - - const flags = selectedFlags.reduce( - (data, { flag, category }) => { - if (data[category]) { - if (Array.isArray(data[category])) { - data[category].push(flag); - } else { - // Track all keys converted to arrays - categoriesMultiple.add(category); - data[category] = [data[category], flag]; - } - } else { - data[category] = flag; - } - return data; - }, - {} as Record<string, string | string[]>, - ); - - // Recombine all arrays into strings - categoriesMultiple.forEach((category) => { - if (Array.isArray(flags[category])) { - flags[category] = flags[category].sort().join(":"); - } - }); - - return flags as Record<string, string>; + const grouped = selectedFlags.reduce<Record<string, string[]>>((agg, { flag, category }) => { + (agg[category] ??= []).push(flag); + return agg; + }, {}); + + return Object.fromEntries( + Object.entries(grouped).map(([category, values]) => [ + category, + values.length > 1 ? [...values].sort().join(":") : values[0], + ]), + ); }Based on learnings: “recommended implementation is a two-phase approach: reduce into
Record<string, string[]>, then convert viaObject.fromEntries, joining only multi-value keys with sorted:.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/lib/track.ts` around lines 35 - 64, The current formatFeatureFlags uses a mixed accumulator (string | string[]) and a truthy check causing type assertions; refactor it to a two-phase approach: in formatFeatureFlags first reduce selectedFlags into a Record<string, string[]> (always push flags into arrays for each category), then convert that record into Record<string, string> by mapping each entry to a sorted joined string (join with ":"), using Object.fromEntries or equivalent; remove the categoriesMultiple Set and the mixed-type logic so no final type assertion is needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@website/lib/track.ts`:
- Around line 35-64: The current formatFeatureFlags uses a mixed accumulator
(string | string[]) and a truthy check causing type assertions; refactor it to a
two-phase approach: in formatFeatureFlags first reduce selectedFlags into a
Record<string, string[]> (always push flags into arrays for each category), then
convert that record into Record<string, string> by mapping each entry to a
sorted joined string (join with ":"), using Object.fromEntries or equivalent;
remove the categoriesMultiple Set and the mixed-type logic so no final type
assertion is needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4942932a-8df0-417b-a632-6fbfd86ff9f8
📒 Files selected for processing (2)
website/components/InputGroup.tsxwebsite/lib/track.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- website/components/InputGroup.tsx
- add specificity to event name - combine all feature events into a single tracking event with object payload, allowing for simpler analytics
- group multiple flags in arrays, then join with semicolon - will surface analytics data points as combinations
ambergristle
force-pushed
the
simplify-tracking
branch
from
af90841 to
52ffeb0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/cli/index.ts (1)
479-479: Consider removing dead code instead of commenting it out.The
reIgnoreFileregex is never used withinretrieveHooksor anywhere else in this function's scope. Commenting out unused code adds noise—if it's no longer needed, delete it entirely.Additionally, this change appears unrelated to the PR's main objective of consolidating tracking events. Was this intended to be part of this PR?
🧹 Suggested fix: remove instead of comment
async function retrieveHooks(hooks: Map<BoilerplateDefWithConfig, string>): Promise<Map<"after", Hook[]>> { - // const reIgnoreFile = /^(chunk-|asset-|#)/gi; const map = new Map<"after", Hook[]>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/index.ts` at line 479, The commented-out regex declaration reIgnoreFile is dead code inside the retrieveHooks scope; remove the commented line entirely (delete the line containing "// const reIgnoreFile = /^(chunk-|asset-|#)/gi;") rather than leaving it commented so the function only contains active code and to avoid unrelated noise in this PR.website/lib/track.ts (1)
35-64: Consider simplifying with the two-phase approach.The logic is correct and produces the expected colon-joined sorted strings for multi-select categories. However, the hybrid approach (keeping singles as strings, tracking array conversions in a Set) is more complex than necessary.
The two-phase approach from your earlier discussion would be cleaner:
♻️ Simpler two-phase implementation
export function formatFeatureFlags(selectedFlags: FeatureFlag[]): Record<string, string> { - const categoriesMultiple = new Set<string>(); - - const flags = selectedFlags.reduce( - (data, { flag, category }) => { - if (data[category]) { - if (Array.isArray(data[category])) { - data[category].push(flag); - } else { - // Track all keys converted to arrays - categoriesMultiple.add(category); - data[category] = [data[category], flag]; - } - } else { - data[category] = flag; - } - return data; - }, - {} as Record<string, string | string[]>, - ); - - // Recombine all arrays into strings - categoriesMultiple.forEach((category) => { - if (Array.isArray(flags[category])) { - flags[category] = flags[category].sort().join(":"); - } - }); - - return flags as Record<string, string>; + // Phase 1: Group flags by category into arrays + const grouped = selectedFlags.reduce( + (acc, { flag, category }) => { + (acc[category] ??= []).push(flag); + return acc; + }, + {} as Record<string, string[]>, + ); + + // Phase 2: Convert arrays to strings (join multi-value with ":") + return Object.fromEntries( + Object.entries(grouped).map(([k, v]) => + [k, v.length > 1 ? [...v].sort().join(":") : v[0]] + ), + ); }Based on learnings: the two-phase approach "avoids the split/sort/join overhead on every collision compared to operating on strings during the reduce."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/lib/track.ts` around lines 35 - 64, formatFeatureFlags uses a hybrid "string-or-array" reduce and a categoriesMultiple Set which is more complex than necessary; refactor formatFeatureFlags to use a two-phase approach: during the reduce always accumulate flags[category] as string[] (use selectedFlags.reduce to push into flags[category] arrays, removing categoriesMultiple and the string-branch logic), then after the reduce iterate the keys of flags to sort each array and join with ":" to produce the final Record<string,string>; reference the existing identifiers selectedFlags, flags, category and remove the special-case handling that treats singles as strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/cli/index.ts`:
- Line 479: The commented-out regex declaration reIgnoreFile is dead code inside
the retrieveHooks scope; remove the commented line entirely (delete the line
containing "// const reIgnoreFile = /^(chunk-|asset-|#)/gi;") rather than
leaving it commented so the function only contains active code and to avoid
unrelated noise in this PR.
In `@website/lib/track.ts`:
- Around line 35-64: formatFeatureFlags uses a hybrid "string-or-array" reduce
and a categoriesMultiple Set which is more complex than necessary; refactor
formatFeatureFlags to use a two-phase approach: during the reduce always
accumulate flags[category] as string[] (use selectedFlags.reduce to push into
flags[category] arrays, removing categoriesMultiple and the string-branch
logic), then after the reduce iterate the keys of flags to sort each array and
join with ":" to produce the final Record<string,string>; reference the existing
identifiers selectedFlags, flags, category and remove the special-case handling
that treats singles as strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b9cc2e17-75b7-4b14-9315-bd3cebd738d7
📒 Files selected for processing (4)
packages/cli/index.tswebsite/components/InputGroup.tsxwebsite/components/Store.tsxwebsite/lib/track.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- website/components/Store.tsx
- website/components/InputGroup.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
copyevents #702Summary by CodeRabbit
Refactor
New Features
Chores