feat: enhance Google Sheets event handling to include new block colum… #8577
Conversation
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
…ns in headers This update ensures that when a new block is added to a journey, its corresponding column is included in the header during the first event submission. The logic has been adjusted to rebuild the header dynamically, preventing data loss for new blocks. Additionally, tests have been added to verify this functionality.
Contributor
WalkthroughDetects dynamic block-derived keys in incoming events, resolves the corresponding journey block by longest-prefix and normalized label, conditionally augments the Google Sheets header when a connected block is first seen, rebuilds column mappings, and writes aligned header and row data to all relevant synced sheets. Changes
Sequence Diagram(s)sequenceDiagram
participant Event as Incoming Event
participant Utils as appendEventToGoogleSheets
participant Blocks as Journey Blocks Service
participant Sheets as Google Sheets API
Event->>Utils: call appendEventToGoogleSheets(event)
Utils->>Utils: parse keys, detect dynamicKey patterns
alt dynamicKey not present in header
Utils->>Blocks: fetch/inspect journey blocks (resolve by longest-prefix, normalize label)
Blocks-->>Utils: block info (id, type, connected)
alt block is connected
Utils->>Utils: build updatedBlockHeaders/updatedColumns/updatedFinalHeader
Utils->>Sheets: update header range with updatedFinalHeader
end
end
Utils->>Utils: map event values into updatedFinalHeader positions
Utils->>Sheets: write aligned data row(s) to all synced sheets
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 7fa4f9f
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (1)
apis/api-journeys-modern/src/schema/event/utils.ts (1)
355-423: Potential duplicate/misparsed dynamic columns (normalization + prefix collision).
- The block-id parse is order-dependent: if a block id is a prefix of another id,
dynamicKey.startsWith(prefix)can match the wrong block (Line 370-385).- You normalize labels for historical headers (Line 324-333) but don’t normalize
eventLabelwhen augmenting (Line 374-382). IfdynamicKeydiffers only by whitespace, you can end up with duplicate headers and misaligned writes.Proposed fix (deterministic match + normalized key used for both headerMap and rowMap)
const safe = (value: string | number | null | undefined): string => value == null ? '' : String(value) const visitorId = safe(row[0]) const createdAt = safe(row[1]) - const dynamicKey = safe(row[5]) + const dynamicKey = safe(row[5]) const dynamicValue = safe(row[6]) + let keyForRow = dynamicKey + // Ensure the current event's block is included in the header calculation. // This handles the case where a new block is added to the journey and this // is the first event for that block - without this, the header wouldn't // include the new block's column and the data would be lost. - if (dynamicKey !== '' && !headerMap.has(dynamicKey)) { - // Parse blockId and label from the dynamicKey format: "${blockId}-${label}" - // Block IDs can contain dashes, so we need to check against actual block IDs - // to find the correct split point - for (const block of journeyBlocks) { - const prefix = `${block.id}-` - if (dynamicKey.startsWith(prefix)) { - const eventBlockId = block.id - const eventLabel = dynamicKey.substring(prefix.length) - - // Only add if this block is connected - if (connectedBlockIds.has(eventBlockId)) { - headerMap.set(dynamicKey, { - blockId: eventBlockId, - label: eventLabel - }) - } - break - } - } - } + if (dynamicKey !== '') { + // Longest-prefix match to avoid prefix collisions (order-independent) + const matchedBlock = journeyBlocks + .filter((b) => dynamicKey.startsWith(`${b.id}-`)) + .sort((a, b) => b.id.length - a.id.length)[0] + + if (matchedBlock != null) { + const prefix = `${matchedBlock.id}-` + const rawLabel = dynamicKey.substring(prefix.length) + const normalizedLabel = rawLabel.replace(/\s+/g, ' ').trim() + const normalizedKey = `${matchedBlock.id}-${normalizedLabel}` + keyForRow = normalizedKey + + if (!headerMap.has(normalizedKey) && connectedBlockIds.has(matchedBlock.id)) { + headerMap.set(normalizedKey, { blockId: matchedBlock.id, label: normalizedLabel }) + } + } + } // Rebuild normalizedBlockHeaders with the potentially updated headerMap const updatedBlockHeaders = Array.from(headerMap.values()) const updatedColumns = buildJourneyExportColumns({ baseColumns, blockHeaders: updatedBlockHeaders, journeyBlocks, orderIndex }) const updatedFinalHeader = updatedColumns.map((column) => column.key) @@ const rowMap: Record<string, string> = {} if (visitorId !== '') rowMap.visitorId = visitorId if (createdAt !== '') rowMap.date = createdAt - if (dynamicKey !== '' && dynamicValue !== '') { - rowMap[dynamicKey] = dynamicValue + if (keyForRow !== '' && dynamicValue !== '') { + rowMap[keyForRow] = dynamicValue }
🧹 Nitpick comments (1)
apis/api-journeys-modern/src/schema/event/utils.spec.ts (1)
661-753: Test is a solid regression guard; consider de-brittling the “length === 3” assertions.If
buildJourneyExportColumnsever adds more base/derived columns for these blocks,toHaveLength(3)(Line 743, 749) will fail even if the behavior is still correct. Prefer asserting:
- header contains the new column, and
- the written row has the poll value at the index of that column (derived from the updated header).
📜 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 (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} : 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-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-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-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-517)
⏰ 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, 1/3)
- GitHub Check: test (22, 3/3)
- GitHub Check: test (22, 2/3)
- GitHub Check: lint (22)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
apis/api-journeys-modern/src/schema/event/utils.ts (1)
388-406: Consistent use of recomputed header/columns is good.Using
updatedColumnsfor header generation (Line 404-410) andupdatedFinalHeaderfor alignment + range sizing (Line 421-434) fixes the “new column gets dropped” class of bugs.Also applies to: 421-434
…s sync This update enhances the event handling logic to ensure that when block IDs have prefix overlaps, the longest matching block ID is used. Additionally, whitespace in dynamic labels is normalized to match existing headers, preventing duplicate columns. Tests have been added to verify these functionalities.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apis/api-journeys-modern/src/schema/event/utils.ts (1)
362-394: LGTM! Longest-prefix matching and whitespace normalization implemented correctly.The logic correctly:
- Uses longest-prefix matching to avoid block ID collisions (e.g., "poll" vs "poll-extended")
- Normalizes whitespace consistently with existing header normalization (line 324)
- Only adds connected blocks to the header
- Handles the edge case where no block matches (via the
matchedBlock != nullcheck)♻️ Optional: Extract longest-prefix matching into a helper function
For better testability and reusability, consider extracting the longest-prefix matching logic:
function findLongestPrefixMatch( dynamicKey: string, blocks: Array<{ id: string }> ): { blockId: string; label: string } | null { const matchedBlock = blocks .filter((b) => dynamicKey.startsWith(`${b.id}-`)) .sort((a, b) => b.id.length - a.id.length)[0] if (matchedBlock == null) return null const prefix = `${matchedBlock.id}-` const rawLabel = dynamicKey.substring(prefix.length) const normalizedLabel = rawLabel.replace(/\s+/g, ' ').trim() return { blockId: matchedBlock.id, label: normalizedLabel } }Then use it:
if (dynamicKey !== '') { - const matchedBlock = journeyBlocks - .filter((b) => dynamicKey.startsWith(`${b.id}-`)) - .sort((a, b) => b.id.length - a.id.length)[0] - - if (matchedBlock != null) { - const prefix = `${matchedBlock.id}-` - const rawLabel = dynamicKey.substring(prefix.length) - const normalizedLabel = rawLabel.replace(/\s+/g, ' ').trim() - const normalizedKey = `${matchedBlock.id}-${normalizedLabel}` + const match = findLongestPrefixMatch(dynamicKey, journeyBlocks) + + if (match != null) { + const normalizedKey = `${match.blockId}-${match.label}` keyForRow = normalizedKey if ( !headerMap.has(normalizedKey) && - connectedBlockIds.has(matchedBlock.id) + connectedBlockIds.has(match.blockId) ) { headerMap.set(normalizedKey, { - blockId: matchedBlock.id, - label: normalizedLabel + blockId: match.blockId, + label: match.label }) } } }apis/api-journeys-modern/src/schema/event/utils.spec.ts (1)
661-931: Optional: Consider additional edge case tests.While the existing tests comprehensively cover the main functionality, consider adding tests for edge cases:
- Dynamic key with no matching block ID (should be handled gracefully)
- Dynamic key with empty label part (e.g., "block-id-")
- Dynamic key for a disconnected block (should not be added to header)
💡 Example edge case test
it('should handle dynamic key with no matching block gracefully', async () => { const mockSync = { id: 'sync-id', journeyId: 'journey-id', teamId: 'team-id', spreadsheetId: 'spreadsheet-id', sheetName: 'Sheet1', deletedAt: null } prismaMock.googleSheetsSync.findMany.mockResolvedValue([mockSync] as any) prismaMock.journey.findUnique.mockResolvedValue({ blocks: [ { id: 'existing-block', typename: 'RadioQuestionBlock', parentBlockId: null, parentOrder: 0, nextBlockId: null, action: null, content: null, deletedAt: null } ] } as any) prismaMock.event.findMany.mockResolvedValue([]) mockGetTeamGoogleAccessToken.mockResolvedValue({ accessToken: 'access-token' }) mockEnsureSheet.mockResolvedValue(undefined) mockReadValues .mockResolvedValueOnce([['Visitor ID', 'Date']]) .mockResolvedValueOnce([]) // Dynamic key for a non-existent block await appendEventToGoogleSheets({ journeyId: 'journey-id', teamId: 'team-id', row: [ 'visitor-id', '2024-01-01T00:00:00.000Z', '', '', '', 'non-existent-block-Label', // No block matches this prefix 'Value' ] }) // Should not crash, and should write base columns only expect(mockWriteValues).toHaveBeenCalled() const writeCall = mockWriteValues.mock.calls[0][0] expect(writeCall.values[0]).toHaveLength(2) // Only base columns })
📜 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 (3)
📚 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-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 (2)
apis/api-journeys-modern/src/schema/event/utils.spec.ts (1)
apis/api-journeys-modern/src/schema/event/utils.ts (1)
appendEventToGoogleSheets(256-525)
apis/api-journeys-modern/src/schema/event/utils.ts (2)
apis/api-journeys-modern/src/schema/journeyVisitor/export/headings.ts (2)
buildJourneyExportColumns(106-128)buildHeaderRows(167-203)apis/api-journeys-modern/src/lib/google/sheets.ts (1)
columnIndexToA1(200-209)
⏰ 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: lint (22)
- GitHub Check: test (22, 3/3)
- GitHub Check: test (22, 1/3)
- GitHub Check: test (22, 2/3)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
apis/api-journeys-modern/src/schema/event/utils.ts (2)
396-404: LGTM! Header rebuild correctly incorporates dynamic blocks.The code properly:
- Rebuilds block headers from the potentially updated
headerMap- Regenerates columns using
buildJourneyExportColumnswith correct ordering- Creates the final header array for data alignment
This ensures new blocks discovered at runtime are included in the header before data is written.
413-413: LGTM! Consistent use of updated header throughout.All references correctly use:
updatedColumnsfor header row generation (line 413)keyForRow(normalized key) for data mapping (lines 425-426)updatedFinalHeaderfor data alignment and range calculations (lines 429-431, 441)This ensures data is correctly aligned with the dynamically augmented header.
Also applies to: 425-427, 429-431, 441-442
apis/api-journeys-modern/src/schema/event/utils.spec.ts (3)
661-755: LGTM! Comprehensive test for longest-prefix matching.This test correctly verifies:
- Blocks with overlapping prefix IDs ("poll" and "poll-extended")
- The dynamicKey "poll-extended-My Label" matches "poll-extended", not "poll"
- Data is written to the correct column (index 2)
The test setup with realistic block hierarchy (StepBlock → CardBlock → RadioQuestionBlocks) ensures the connectivity filter works as expected.
757-837: LGTM! Whitespace normalization test validates duplicate prevention.This test correctly verifies:
- Labels with different whitespace ("My Label" vs "My Label") normalize to the same key
- No duplicate columns are created
- Data is written to the existing column (index 2)
The test includes an existing event with a normalized label, then sends a new event with extra whitespace, ensuring the normalization logic prevents duplication.
839-931: LGTM! Critical test verifies the main bug fix.This test thoroughly validates the PR objective:
- A new RadioQuestionBlock is added to the journey
- No prior events exist for this block
- First event submission dynamically adds the block column to the header
- Header is updated with the new column (length increases from 2 to 3)
- Data is written to the correct column position
The assertions comprehensively verify:
- Header update occurred (
mockUpdateRangeValuescalled)- Header length and content are correct (lines 921-922)
- Data row has correct length and values (lines 927-930)
|
I see you added the "on stage" label, I'll get this merged to the stage branch! |
tanflem
approved these changes
Jan 12, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
…ns in headers
This update ensures that when a new block is added to a journey, its corresponding column is included in the header during the first event submission. The logic has been adjusted to rebuild the header dynamically, preventing data loss for new blocks. Additionally, tests have been added to verify this functionality.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.