feat: enhance appendEventToGoogleSheets to prevent value duplication …#8584
Conversation
…and support semicolon-separated appending
WalkthroughappendEventToGoogleSheets and journey export header logic now deduplicate headers by blockId, choose deterministic first labels (ordered by createdAt), resolve target columns per row dynamically, and merge existing/new cell values with semicolon-separated deduplication. Three unit tests were added for merge/dedupe/column-resolution behaviors. Changes
Sequence Diagram(s)(Skipped — changes are internal header/merge logic and do not introduce a new multi-component sequential flow.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
View your CI Pipeline Execution ↗ for commit 1ddd925
☁️ Nx Cloud last updated this comment at |
![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)
apis/api-journeys-modern/src/schema/event/utils.spec.ts (2)
610-618: Test assertions don't verify the actual merged value.The test verifies that
updateRangeValuesis called with values containing'visitor-id', but doesn't assert that the merged value actually contains'Option A; Option B'. This weakens the test's ability to catch regressions in the merge logic.♻️ Suggested improvement to verify merged values
// Should call updateRangeValues with merged values (semicolon separated) expect(mockUpdateRangeValues).toHaveBeenCalledWith( expect.objectContaining({ values: expect.arrayContaining([ - expect.arrayContaining(['visitor-id']) + expect.arrayContaining(['Option A; Option B']) ]) }) )Alternatively, capture the call arguments and assert the specific cell value:
const updateCall = mockUpdateRangeValues.mock.calls.find( (call) => call[0].range.includes('A2') ) expect(updateCall[0].values[0]).toContain('Option A; Option B')
659-661: Test doesn't verify the actual deduplication result.The test confirms that
updateRangeValuesis called, but doesn't assert that the value remains'Option A; Option B'without duplicating'Option A'. Consider adding an assertion to verify the merged row content.♻️ Suggested improvement
// Should call updateRangeValues without duplicating 'Option A' - expect(mockUpdateRangeValues).toHaveBeenCalled() + expect(mockUpdateRangeValues).toHaveBeenCalledWith( + expect.objectContaining({ + values: [expect.arrayContaining(['Option A; Option B'])] + }) + ) expect(mockWriteValues).not.toHaveBeenCalled()
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apis/api-journeys-modern/src/schema/event/utils.spec.tsapis/api-journeys-modern/src/schema/event/utils.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/base.mdc)
**/*.{ts,tsx,js,jsx}: Use early returns whenever possible to make the code more readable.
Use descriptive variable and function/const names.
Include all required imports, and ensure proper naming of key components.
Files:
apis/api-journeys-modern/src/schema/event/utils.spec.tsapis/api-journeys-modern/src/schema/event/utils.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/base.mdc)
Define a type if possible.
Files:
apis/api-journeys-modern/src/schema/event/utils.spec.tsapis/api-journeys-modern/src/schema/event/utils.ts
🧠 Learnings (7)
📚 Learning: 2025-12-19T04:58:24.460Z
Learnt from: CR
Repo: JesusFilm/core PR: 0
File: apps/watch/AGENTS.md:0-0
Timestamp: 2025-12-19T04:58:24.460Z
Learning: Applies to apps/watch/src/**/*.spec.{ts,tsx} : Co-locate React Testing Library specs under `*.spec.ts(x)` and mock network traffic with MSW handlers in `apps/watch/test`.
Applied to files:
apis/api-journeys-modern/src/schema/event/utils.spec.ts
📚 Learning: 2025-12-19T04:58:24.460Z
Learnt from: CR
Repo: JesusFilm/core PR: 0
File: apps/watch/AGENTS.md:0-0
Timestamp: 2025-12-19T04:58:24.460Z
Learning: Applies to apps/watch/src/**/*.spec.{ts,tsx} : Enclose SWR-based hooks in `TestSWRConfig` (`apps/watch/test/TestSWRConfig.tsx`) to isolate cache state between assertions in tests.
Applied to files:
apis/api-journeys-modern/src/schema/event/utils.spec.ts
📚 Learning: 2025-12-19T04:58:24.460Z
Learnt from: CR
Repo: JesusFilm/core PR: 0
File: apps/watch/AGENTS.md:0-0
Timestamp: 2025-12-19T04:58:24.460Z
Learning: Applies to apps/watch/src/**/*.spec.{ts,tsx} : Use the shared Jest setup in `apps/watch/setupTests.tsx` which already boots MSW, Next router mock, and has a longer async timeout.
Applied to files:
apis/api-journeys-modern/src/schema/event/utils.spec.ts
📚 Learning: 2025-09-29T23:03:36.840Z
Learnt from: mikeallisonJS
Repo: JesusFilm/core PR: 7629
File: apis/api-journeys-modern/src/schema/event/utils.ts:43-60
Timestamp: 2025-09-29T23:03:36.840Z
Learning: In the JesusFilm/core repository, do not recommend using Prisma's `upsert` operation for `JourneyVisitor` creation in `apis/api-journeys-modern/src/schema/event/utils.ts` as it is not race condition safe for this use case. The current `findUnique` then `create` pattern is the preferred approach.
Applied to files:
apis/api-journeys-modern/src/schema/event/utils.spec.ts
📚 Learning: 2025-12-19T19:18:43.790Z
Learnt from: CR
Repo: JesusFilm/core PR: 0
File: apps/resources/AGENTS.md:0-0
Timestamp: 2025-12-19T19:18:43.790Z
Learning: Applies to apps/resources/**/*.spec.ts?(x) : Enclose SWR-based hooks in `TestSWRConfig` (`apps/resources/test/TestSWRConfig.tsx`) to isolate cache state between assertions
Applied to files:
apis/api-journeys-modern/src/schema/event/utils.spec.ts
📚 Learning: 2025-12-19T19:18:43.790Z
Learnt from: CR
Repo: JesusFilm/core PR: 0
File: apps/resources/AGENTS.md:0-0
Timestamp: 2025-12-19T19:18:43.790Z
Learning: Applies to apps/resources/**/*.spec.ts?(x) : Co-locate React Testing Library specs under `*.spec.ts(x)` and mock network traffic with MSW handlers in `apps/resources/test`
Applied to files:
apis/api-journeys-modern/src/schema/event/utils.spec.ts
📚 Learning: 2025-11-16T21:30:53.412Z
Learnt from: Kneesal
Repo: JesusFilm/core PR: 8309
File: apps/journeys-admin/setupTests.tsx:47-51
Timestamp: 2025-11-16T21:30:53.412Z
Learning: In apps/journeys-admin/setupTests.tsx, the `document.clearImmediate` mock is required for tests involving the mux upload provider, as an underlying library uses clearImmediate for timeout management to prevent race conditions and unnecessary polling calls.
Applied to files:
apis/api-journeys-modern/src/schema/event/utils.spec.ts
🧬 Code graph analysis (1)
apis/api-journeys-modern/src/schema/event/utils.spec.ts (1)
apis/api-journeys-modern/src/schema/event/utils.ts (1)
appendEventToGoogleSheets(256-499)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test (22, 2/3)
- GitHub Check: test (22, 3/3)
- GitHub Check: test (22, 1/3)
- GitHub Check: lint (22)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
apis/api-journeys-modern/src/schema/event/utils.ts (1)
457-468: LGTM! Merge logic correctly handles deduplication and semicolon-separated appending.The implementation properly:
- Preserves existing values when new value is empty
- Uses new value when existing is empty
- Avoids duplicates by checking if the trimmed new value exists in the split parts
- Appends with consistent "; " separator
One minor consideration: if a value itself contains "; " as part of its content, it would be incorrectly split. This is likely an acceptable trade-off for this use case since event values typically don't contain semicolons.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apis/api-journeys-modern/src/schema/event/utils.spec.ts (2)
610-618: Weak assertion does not verify semicolon merging behavior.The test description says it "should append new values with semicolon" but the assertion only checks that
valuescontains'visitor-id'. This would pass even if the merging logic was broken. Consider capturing the call arguments and asserting the actual merged value contains'Option A; Option B'.💡 Suggested stronger assertion
- // Should call updateRangeValues with merged values (semicolon separated) - expect(mockUpdateRangeValues).toHaveBeenCalledWith( - expect.objectContaining({ - values: expect.arrayContaining([ - expect.arrayContaining(['visitor-id']) - ]) - }) - ) + // Should call updateRangeValues with merged values (semicolon separated) + expect(mockUpdateRangeValues).toHaveBeenCalled() + const updateCall = mockUpdateRangeValues.mock.calls.find( + (call) => call[0].range?.includes('A2') + ) + expect(updateCall).toBeDefined() + const mergedValues = updateCall?.[0].values[0] + expect(mergedValues).toContain('Option A; Option B')
660-664: Assertion does not verify deduplication behavior.The test intends to verify that duplicates are not added, but only asserts that
updateRangeValueswas called. To properly validate deduplication, assert that the merged value remains'Option A; Option B'(not'Option A; Option B; Option A').💡 Suggested stronger assertion
- // Should call updateRangeValues without duplicating 'Option A' - expect(mockUpdateRangeValues).toHaveBeenCalled() - expect(mockWriteValues).not.toHaveBeenCalled() + // Should call updateRangeValues without duplicating 'Option A' + expect(mockUpdateRangeValues).toHaveBeenCalled() + const updateCall = mockUpdateRangeValues.mock.calls.find( + (call) => call[0].range?.includes('A2') + ) + expect(updateCall).toBeDefined() + const mergedValues = updateCall?.[0].values[0] + // Verify 'Option A' appears only once (no duplication) + expect(mergedValues).toContain('Option A; Option B') + expect(mergedValues).not.toContain('Option A; Option B; Option A') + expect(mockWriteValues).not.toHaveBeenCalled()
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apis/api-journeys-modern/src/schema/event/utils.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/base.mdc)
**/*.{ts,tsx,js,jsx}: Use early returns whenever possible to make the code more readable.
Use descriptive variable and function/const names.
Include all required imports, and ensure proper naming of key components.
Files:
apis/api-journeys-modern/src/schema/event/utils.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/base.mdc)
Define a type if possible.
Files:
apis/api-journeys-modern/src/schema/event/utils.spec.ts
🧠 Learnings (4)
📚 Learning: 2025-12-19T04:58:24.460Z
Learnt from: CR
Repo: JesusFilm/core PR: 0
File: apps/watch/AGENTS.md:0-0
Timestamp: 2025-12-19T04:58:24.460Z
Learning: Applies to apps/watch/src/**/*.spec.{ts,tsx} : Enclose SWR-based hooks in `TestSWRConfig` (`apps/watch/test/TestSWRConfig.tsx`) to isolate cache state between assertions in tests.
Applied to files:
apis/api-journeys-modern/src/schema/event/utils.spec.ts
📚 Learning: 2025-12-19T04:58:24.460Z
Learnt from: CR
Repo: JesusFilm/core PR: 0
File: apps/watch/AGENTS.md:0-0
Timestamp: 2025-12-19T04:58:24.460Z
Learning: Applies to apps/watch/src/**/*.spec.{ts,tsx} : Co-locate React Testing Library specs under `*.spec.ts(x)` and mock network traffic with MSW handlers in `apps/watch/test`.
Applied to files:
apis/api-journeys-modern/src/schema/event/utils.spec.ts
📚 Learning: 2025-09-29T23:03:36.840Z
Learnt from: mikeallisonJS
Repo: JesusFilm/core PR: 7629
File: apis/api-journeys-modern/src/schema/event/utils.ts:43-60
Timestamp: 2025-09-29T23:03:36.840Z
Learning: In the JesusFilm/core repository, do not recommend using Prisma's `upsert` operation for `JourneyVisitor` creation in `apis/api-journeys-modern/src/schema/event/utils.ts` as it is not race condition safe for this use case. The current `findUnique` then `create` pattern is the preferred approach.
Applied to files:
apis/api-journeys-modern/src/schema/event/utils.spec.ts
📚 Learning: 2025-11-16T21:30:53.412Z
Learnt from: Kneesal
Repo: JesusFilm/core PR: 8309
File: apps/journeys-admin/setupTests.tsx:47-51
Timestamp: 2025-11-16T21:30:53.412Z
Learning: In apps/journeys-admin/setupTests.tsx, the `document.clearImmediate` mock is required for tests involving the mux upload provider, as an underlying library uses clearImmediate for timeout management to prevent race conditions and unnecessary polling calls.
Applied to files:
apis/api-journeys-modern/src/schema/event/utils.spec.ts
🧬 Code graph analysis (1)
apis/api-journeys-modern/src/schema/event/utils.spec.ts (1)
apis/api-journeys-modern/src/schema/event/utils.ts (1)
appendEventToGoogleSheets(256-499)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (22, 3/3)
- GitHub Check: lint (22)
- GitHub Check: Analyze (javascript)
|
I see you added the "on stage" label, I'll get this merged to the stage branch! |
…eys based on blockId, preventing incorrect value placements
|
Merge conflict attempting to merge this into stage. Please fix manually. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apis/api-journeys-modern/src/schema/event/utils.ts`:
- Around line 371-390: The extractor extractBlockIdFromKey currently only
detects keys like "blockId-..." and returns null for a key that is exactly a
blockId, causing resolvedBlockId to be null and resolvedColumnKey to fall back
to the raw blockId and be dropped; update extractBlockIdFromKey to also treat an
exact match (key === col.blockId) as a valid match (i.e., check col.blockId !=
null && (key === col.blockId || key.startsWith(`${col.blockId}-`))), so
resolvedBlockId will be set correctly and resolvedColumnKey (which uses
columns.find(col => col.blockId === resolvedBlockId)?.key) will resolve to the
correct column key and preserve the value with finalHeader.
🧹 Nitpick comments (3)
apis/api-journeys-modern/src/schema/event/utils.ts (1)
478-488: Optional: make semicolon splitting more tolerant.If existing data was written with
";"(no space) or inconsistent spacing, the current split on"; "can miss duplicates. Consider splitting on";"and trimming to be robust.♻️ Optional tweak
- const existingParts = existingValue.split('; ').map((s) => s.trim()) + const existingParts = existingValue + .split(';') + .map((s) => s.trim()) + .filter(Boolean)apis/api-journeys-modern/src/schema/event/utils.spec.ts (2)
572-619: Consider asserting the actual semicolon-merged value.Right now the test only checks that
updateRangeValuesis called (Line 610+). To lock behavior, assert the merged cell contains"Option A; Option B".
621-664: Consider asserting that duplicates are not introduced.This test could explicitly assert the merged cell remains
"Option A; Option B"(and not"Option A; Option B; Option A"), rather than only checking that an update happened.
|
Merge conflict attempting to merge this into stage. Please fix manually. |
…upport exact and prefix matches for blockId
|
Merge conflict attempting to merge this into stage. Please fix manually. |
…nsure new blocks are included and prevent duplicate column keys
|
Merge conflict attempting to merge this into stage. Please fix manually. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apis/api-journeys-modern/src/schema/event/utils.ts`:
- Around line 509-519: The dedup logic in the mergedRow computation
(alignedRow.map) only splits existingValue into existingParts so multi-item new
values duplicate; change the merge to split both existingValue and value by ';',
trim parts, create a Set to union them (preserving original order if possible),
then join with '; ' and return that combined string (update the block that
defines existingParts and the final return to perform split-new, merge-unique,
and rejoin); keep the early returns for empty value or empty existingValue and
reference the alignedRow, existingRow, existingParts, and mergedRow identifiers
when making the change.
…en-google-sync-and-standard-data-2
…om existing and new inputs, improving data integrity
…en-google-sync-and-standard-data-2
…mns by blockId, ensuring only the first label is retained for each block
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apis/api-journeys-modern/src/schema/journeyVisitor/journeyVisitor.spec.ts (1)
376-441: Don’t drop values when labels change for the same blockId.
With blockId-only columns, events with different labels should still land in the same column; the current expectation (“Submit” only) would mask data loss. Please update the expected output (and implementation) to merge both values (e.g., “Submit; Cancel”), or explicitly confirm that dropping is intended.🛠️ Suggested expectation update
- '"Date","Button Click"\n"2024-01-01","Submit"\n' + '"Date","Button Click"\n"2024-01-01","Submit; Cancel"\n'apis/api-journeys-modern/src/schema/journeyVisitor/journeyVisitor.ts (1)
332-349: Header dedupe now causes label-changed events to be dropped.
Headers are blockId-only, but row keys are still${blockId}-${label}ingetJourneyVisitors, so values from label changes won’t match any column key and get omitted. Map events to the canonical column key for the blockId (or move to blockId-only keys) so all values land in the single column.🛠️ One way to align row keys with blockId-only headers
-async function* getJourneyVisitors( - journeyId: string, - eventWhere: Prisma.EventWhereInput, - timezone: string, - batchSize: number = 1000 -): AsyncGenerator<JourneyVisitorExportRow> { +async function* getJourneyVisitors( + journeyId: string, + eventWhere: Prisma.EventWhereInput, + timezone: string, + blockIdToKey: Map<string, string>, + batchSize: number = 1000 +): AsyncGenerator<JourneyVisitorExportRow> { @@ - if (event.blockId) { + if (event.blockId) { const normalizedLabel = (event.label ?? '') .replace(/\s+/g, ' ') .trim() - const key = `${event.blockId}-${normalizedLabel}` + const key = + blockIdToKey.get(event.blockId) ?? + `${event.blockId}-${normalizedLabel}` @@ - for await (const row of getJourneyVisitors( - journeyId, - eventWhere, - userTimezone - )) { + const blockIdToKey = new Map( + columns + .filter((c) => c.blockId != null) + .map((c) => [c.blockId as string, c.key]) + ) + for await (const row of getJourneyVisitors( + journeyId, + eventWhere, + userTimezone, + blockIdToKey + )) {apis/api-journeys-modern/src/schema/journeyVisitor/journeyVisitorExportToGoogleSheet.mutation.ts (1)
303-322: Same blockId-only header mismatch here (label-changed events dropped).
Headers are deduped by blockId, but rows are still keyed by${blockId}-${label}ingetJourneyVisitors, so events with changed labels won’t land in any column. Please route all events to the canonical column key for their blockId (or switch to blockId-only keys) to avoid data loss.🛠️ Align row keys with blockId-only headers
-async function* getJourneyVisitors( - journeyId: string, - eventWhere: Prisma.EventWhereInput, - timezone: string, - batchSize: number = 1000 -): AsyncGenerator<JourneyVisitorExportRow> { +async function* getJourneyVisitors( + journeyId: string, + eventWhere: Prisma.EventWhereInput, + timezone: string, + blockIdToKey: Map<string, string>, + batchSize: number = 1000 +): AsyncGenerator<JourneyVisitorExportRow> { @@ - if (event.blockId) { + if (event.blockId) { const normalizedLabel = (event.label ?? '') .replace(/\s+/g, ' ') .trim() - const key = `${event.blockId}-${normalizedLabel}` + const key = + blockIdToKey.get(event.blockId) ?? + `${event.blockId}-${normalizedLabel}` @@ - for await (const row of getJourneyVisitors( - journeyId, - eventWhere, - userTimezone - )) { + const blockIdToKey = new Map( + columns + .filter((c) => c.blockId != null) + .map((c) => [c.blockId as string, c.key]) + ) + for await (const row of getJourneyVisitors( + journeyId, + eventWhere, + userTimezone, + blockIdToKey + )) {
…en-google-sync-and-standard-data-2
…el retrieval Updated journeyVisitor and related exports to include an orderBy clause, ensuring labels are consistently retrieved based on creation date. This change enhances data integrity by retaining the first label per blockId across various export functions.
…en-google-sync-and-standard-data-2
mikeallisonJS
deleted the
mikeallison/nes-1142-inconsistent-data-between-google-sync-and-standard-data-2
branch
2 participants
…and support semicolon-separated appending
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.