feat: add command to fix cross-team visitor issues in project configu…#8905
Open
mikeallisonJS wants to merge 6 commits intomainfrom
Open
feat: add command to fix cross-team visitor issues in project configu…#8905mikeallisonJS wants to merge 6 commits intomainfrom
mikeallisonJS wants to merge 6 commits intomainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds a Prisma maintenance script that identifies and fixes Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 508e27c
☁️ 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.ts`:
- Around line 86-100: The visitor is created outside the transaction
(db.visitor.create) so if the later transaction that creates JourneyVisitor
records fails you'll leave orphan Visitor rows; move the visitor creation into
an interactive Prisma transaction using db.$transaction(async (tx) => { ... })
so you can call await tx.visitor.create(...) and then use the returned
visitor.id in the subsequent tx.journeyVisitor.create/delete/update calls
(instead of db.journeyVisitor.*), ensuring all operations succeed or roll back
together.
🪄 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: 4db1f23a-274e-4b10-ad8b-6ed9d35e0fd9
📒 Files selected for processing (3)
apis/api-journeys-modern/project.jsonapis/api-journeys-modern/src/scripts/fix-cross-team-visitors.spec.tsapis/api-journeys-modern/src/scripts/fix-cross-team-visitors.ts
apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.spec.ts (1)
14-23: Consider adding a type annotation for theoverridesparameter.The helper function would be more type-safe with an explicit type for
overrides:- const makeMismatchedRecord = (overrides = {}) => ({ + const makeMismatchedRecord = (overrides: Partial<MismatchedRecord> = {}) => ({You'll need to import or locally define the
MismatchedRecordtype from the implementation file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.spec.ts` around lines 14 - 23, The helper function makeMismatchedRecord should add an explicit type annotation for the overrides parameter to improve type-safety: import (or locally define) the concrete record type (e.g., MismatchedRecord or the actual interface used by the implementation) and change the signature to accept overrides: Partial<MismatchedRecord> (or the appropriate Partial of the record type), so callers can only override known fields like journeyVisitorId, journeyId, wrongVisitorId, etc.; update the import to bring that type from the implementation file and use it in makeMismatchedRecord's parameter and return typing.apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.ts (1)
220-225: New JourneyVisitor is created without historical aggregate data.The new
JourneyVisitoris created with onlyjourneyIdandvisitorId, so aggregate fields (activityCount,duration, timestamps) reset to defaults. The historical data from the deletedwrongJVis not transferred.If preserving these aggregates matters, consider copying them:
♻️ Optional: Preserve aggregate fields on reassign
This would require fetching
wrongJVbefore deletion (similar to the merge path) and copying its fields to the new record. If the current behavior is intentional (fresh start), this can be ignored.+ const wrongJV = await db.journeyVisitor.findUnique({ + where: { id: record.journeyVisitorId } + }) + await db.$transaction([ db.event.updateMany({ where: { journeyId: record.journeyId, visitorId: record.wrongVisitorId }, data: { visitorId: correctVisitor.id } }), db.journeyVisitor.delete({ where: { id: record.journeyVisitorId } }), db.journeyVisitor.create({ - data: { - journeyId: record.journeyId, - visitorId: correctVisitor.id - } + data: { + journeyId: record.journeyId, + visitorId: correctVisitor.id, + activityCount: wrongJV?.activityCount ?? 0, + duration: wrongJV?.duration ?? 0, + lastStepViewedAt: wrongJV?.lastStepViewedAt, + // ... other fields + } }) ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.ts` around lines 220 - 225, The new JourneyVisitor created via db.journeyVisitor.create only sets journeyId and visitorId and thus loses historical aggregate fields from the deleted wrongJV; fetch the existing wrongJV (the record referenced earlier as wrongJV) before you delete it and copy its aggregate fields (e.g., activityCount, duration, firstSeen/lastSeen or other timestamp fields) into the data object passed to db.journeyVisitor.create (or merge/sum them if appropriate) so the historical aggregates are preserved when creating the new JourneyVisitor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.spec.ts`:
- Around line 14-23: The helper function makeMismatchedRecord should add an
explicit type annotation for the overrides parameter to improve type-safety:
import (or locally define) the concrete record type (e.g., MismatchedRecord or
the actual interface used by the implementation) and change the signature to
accept overrides: Partial<MismatchedRecord> (or the appropriate Partial of the
record type), so callers can only override known fields like journeyVisitorId,
journeyId, wrongVisitorId, etc.; update the import to bring that type from the
implementation file and use it in makeMismatchedRecord's parameter and return
typing.
In `@apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.ts`:
- Around line 220-225: The new JourneyVisitor created via
db.journeyVisitor.create only sets journeyId and visitorId and thus loses
historical aggregate fields from the deleted wrongJV; fetch the existing wrongJV
(the record referenced earlier as wrongJV) before you delete it and copy its
aggregate fields (e.g., activityCount, duration, firstSeen/lastSeen or other
timestamp fields) into the data object passed to db.journeyVisitor.create (or
merge/sum them if appropriate) so the historical aggregates are preserved when
creating the new JourneyVisitor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3883cf05-96ca-429c-b1b2-e28827c60e06
📒 Files selected for processing (2)
apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.spec.tsapis/api-journeys-modern/src/scripts/fix-cross-team-visitors.ts
… fields and handle missing JourneyVisitor cases
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.ts (1)
349-361: Don't let--applyreport success after per-record exceptions.The loop converts thrown errors into
skipped, andmain()only exits non-zero for uncaught failures. That makes a partial repair look successful to the Nx target/CI even when some rows were not fixed. Consider tracking whether the catch path ran and setting a non-zero exit code after the summary in apply mode.Also applies to: 387-409
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.ts` around lines 349 - 361, The catch block swallows per-record exceptions by marking them 'skipped' and the script's main() only exits non-zero for uncaught failures, so add a boolean flag (e.g., hadPerRecordErrors) that's set true inside both catch paths where you push a skipped result (referencing record.journeyVisitorId, record.wrongVisitorId, results array), then after the summary/when apply mode is active (the --apply branch in main()), check that flag and call process.exit(1) to ensure the CLI/CI sees a non-zero exit code when any record-level errors occurred; do the same for the other catch region (lines ~387-409) so all per-record exceptions are tracked.apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.spec.ts (2)
10-12: Reset the shared Prisma mock implementations between tests.
jest.clearAllMocks()only clears call history. It leavesmockResolvedValue/mockImplementationintact, so helpers like the interactive$transactionstub on Line 298 can leak into later cases and make the suite order-dependent. Use the deep-mock reset helper forprismaMockhere, orjest.resetAllMocks()if that's the local pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.spec.ts` around lines 10 - 12, The beforeEach currently only calls jest.clearAllMocks(), which leaves mockResolvedValue/mockImplementation on the shared prismaMock and allows the interactive $transaction stub (see $transaction stub around line 298) to leak between tests; update the beforeEach to reset deep mocks instead—either replace jest.clearAllMocks() with jest.resetAllMocks() or call the prismaMock deep-reset helper (e.g., prismaMock.$resetMocks() or whatever reset method your prisma mock exposes) so all Prisma mock implementations are cleared between tests.
14-23: Type the fixture factory.
overrides = {}and the inferred return type let the fixture drift from the production record shape without a compile-time failure. Deriving the type fromfixMismatchedRecord's second parameter keeps these tests honest.As per coding guidelines, "Define a type if possible."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.spec.ts` around lines 14 - 23, The fixture factory makeMismatchedRecord is untyped and can drift from the real record shape; update its signature to derive types from the production helper by using the second parameter type of fixMismatchedRecord for the overrides (e.g. overrides: Parameters<typeof fixMismatchedRecord>[1]) and, if needed, annotate the returned object with the corresponding production record type (e.g. ReturnType or the same Parameters[...] type) so TypeScript enforces shape compatibility with fixMismatchedRecord.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.ts`:
- Around line 76-145: The code reads snapshots like wrongJV (and
existingCorrectJV) and event counts before starting the write transaction, which
can cause stale data to be applied; fix by moving all reads that determine
merge/create payloads into the same transaction used for writes: use
tx.journeyVisitor.findUnique/findFirst (instead of db.journeyVisitor.findUnique)
and tx.event.count (instead of db.event.count) inside the async tx callback,
recompute the merged counters/last* fields from those tx-scoped reads, then
perform tx.visitor.create, tx.event.updateMany, tx.journeyVisitor.delete and
tx.journeyVisitor.create using the freshly read tx variables (apply same change
for the other blocks referencing existingCorrectJV and wrongJV in the other
ranges).
---
Nitpick comments:
In `@apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.spec.ts`:
- Around line 10-12: The beforeEach currently only calls jest.clearAllMocks(),
which leaves mockResolvedValue/mockImplementation on the shared prismaMock and
allows the interactive $transaction stub (see $transaction stub around line 298)
to leak between tests; update the beforeEach to reset deep mocks instead—either
replace jest.clearAllMocks() with jest.resetAllMocks() or call the prismaMock
deep-reset helper (e.g., prismaMock.$resetMocks() or whatever reset method your
prisma mock exposes) so all Prisma mock implementations are cleared between
tests.
- Around line 14-23: The fixture factory makeMismatchedRecord is untyped and can
drift from the real record shape; update its signature to derive types from the
production helper by using the second parameter type of fixMismatchedRecord for
the overrides (e.g. overrides: Parameters<typeof fixMismatchedRecord>[1]) and,
if needed, annotate the returned object with the corresponding production record
type (e.g. ReturnType or the same Parameters[...] type) so TypeScript enforces
shape compatibility with fixMismatchedRecord.
In `@apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.ts`:
- Around line 349-361: The catch block swallows per-record exceptions by marking
them 'skipped' and the script's main() only exits non-zero for uncaught
failures, so add a boolean flag (e.g., hadPerRecordErrors) that's set true
inside both catch paths where you push a skipped result (referencing
record.journeyVisitorId, record.wrongVisitorId, results array), then after the
summary/when apply mode is active (the --apply branch in main()), check that
flag and call process.exit(1) to ensure the CLI/CI sees a non-zero exit code
when any record-level errors occurred; do the same for the other catch region
(lines ~387-409) so all per-record exceptions are tracked.
🪄 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: c044ec9d-d680-4c7e-b91e-f28e31045479
📒 Files selected for processing (2)
apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.spec.tsapis/api-journeys-modern/src/scripts/fix-cross-team-visitors.ts
apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.ts
Outdated
Show resolved
Hide resolved
…d maintain functionality
…missing JourneyVisitors
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.ts (1)
118-173:⚠️ Potential issue | 🟠 MajorMove the reads that drive the merge inside the transaction.
correctJV,wrongJV, andeventCountare all snapshotted before the write transaction and then reused to decide whichlast*fields survive. If new activity lands between those reads and the update, this can overwrite newer aggregate state on the destination row, and the precomputedeventCountcan still leak into the summary when the function falls back toskipped. Re-read theJourneyVisitorrows inside an interactive$transaction(async (tx) => ...)and seteventsUpdatedfrom the returnedupdateMany().count.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.ts` around lines 118 - 173, The reads driving the merge (correctJV, wrongJV, and eventCount) must be moved inside an interactive transaction so the update uses a consistent snapshot and you can derive eventsUpdated from the actual update result; wrap the logic in db.$transaction(async (tx) => { ... }) and inside use tx.journeyVisitor.findUnique(...) to re-read correctJV and wrongJV and tx.event.updateMany(...) for the event move, capture the returned count from updateMany() and assign result.eventsUpdated from that count, then call tx.journeyVisitor.update(...) with buildMergeData(wrongJV, correctJV) and tx.journeyVisitor.delete(...) — avoid using pre-fetched correctJV/wrongJV/eventCount from before the transaction.
🧹 Nitpick comments (1)
apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.spec.ts (1)
10-12: Reset mock implementations between tests.
prismaMockis shared across the file, andjest.clearAllMocks()only clears call history. AnymockResolvedValueormockRejectedValueleft behind by one case will still be active in the next one, so this suite becomes order-dependent as it grows.jest.resetAllMocks()or explicit resets onprismaMockwill keep each case isolated.♻️ Small fix
beforeEach(() => { - jest.clearAllMocks() + jest.resetAllMocks() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.spec.ts` around lines 10 - 12, The beforeEach currently calls jest.clearAllMocks(), which only clears call history and leaves mock implementations like mockResolvedValue on the shared prismaMock; change this to reset mock implementations between tests by using jest.resetAllMocks() in the beforeEach (or explicitly resetting prismaMock mocks) so prismaMock's mockResolvedValue/mockRejectedValue do not leak between tests; update the beforeEach that references jest.clearAllMocks() accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.ts`:
- Around line 111-131: The code currently returns early and marks records
skipped when the destination Visitor (correctVisitor) or its JourneyVisitor
(correctJV) is missing; instead, update the logic in the repair flow so that
when correctVisitor is null you create the missing Visitor using data from the
source visitor (use record/sourceVisitor fields) and set result.correctVisitorId
to the new id, and when correctJV is null you create the missing JourneyVisitor
row (use db.journeyVisitor.create) linking record.journeyId and
result.correctVisitorId within the same transaction used by this script; locate
and modify the blocks around correctVisitor, result, correctJV, and the
db.journeyVisitor.findUnique call to perform creation rather than returning a
skipped result.
---
Duplicate comments:
In `@apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.ts`:
- Around line 118-173: The reads driving the merge (correctJV, wrongJV, and
eventCount) must be moved inside an interactive transaction so the update uses a
consistent snapshot and you can derive eventsUpdated from the actual update
result; wrap the logic in db.$transaction(async (tx) => { ... }) and inside use
tx.journeyVisitor.findUnique(...) to re-read correctJV and wrongJV and
tx.event.updateMany(...) for the event move, capture the returned count from
updateMany() and assign result.eventsUpdated from that count, then call
tx.journeyVisitor.update(...) with buildMergeData(wrongJV, correctJV) and
tx.journeyVisitor.delete(...) — avoid using pre-fetched
correctJV/wrongJV/eventCount from before the transaction.
---
Nitpick comments:
In `@apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.spec.ts`:
- Around line 10-12: The beforeEach currently calls jest.clearAllMocks(), which
only clears call history and leaves mock implementations like mockResolvedValue
on the shared prismaMock; change this to reset mock implementations between
tests by using jest.resetAllMocks() in the beforeEach (or explicitly resetting
prismaMock mocks) so prismaMock's mockResolvedValue/mockRejectedValue do not
leak between tests; update the beforeEach that references jest.clearAllMocks()
accordingly.
🪄 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: d9fea40a-7ef4-4db0-b569-173b17c9e038
📒 Files selected for processing (2)
apis/api-journeys-modern/src/scripts/fix-cross-team-visitors.spec.tsapis/api-journeys-modern/src/scripts/fix-cross-team-visitors.ts
tataihono
approved these changes
Mar 24, 2026
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
2 participants
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.
…ration
Summary by CodeRabbit
New Features
Tests