feat(modmail): migration script - #325
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdded a one-off legacy ModMail migration with dry-run, live, and verification modes. The migration maps legacy records into the current schema, validates conflicts, remaps relationships, reports results, and supports migrated DM-origin threads whose channels may be unavailable. ChangesLegacy ModMail migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant migrateLegacyModmail
participant LegacyDatabase
participant TargetDatabase
Operator->>migrateLegacyModmail: Select migration or verification mode
migrateLegacyModmail->>LegacyDatabase: Read and validate legacy ModMail data
migrateLegacyModmail->>TargetDatabase: Write mapped records or compare migrated data
TargetDatabase-->>migrateLegacyModmail: Return migration or verification results
migrateLegacyModmail-->>Operator: Print statistics and exit status
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/private/db/src/scripts/migrateLegacyModmail.ts (2)
999-1005: 🩺 Stability & Availability | 🔵 TrivialConfirm the target's transaction timeouts before the live run.
The whole migration runs inside one target transaction while the script round-trips to the legacy database between writes. If the target enforces
idle_in_transaction_session_timeoutorstatement_timeout, a largeThreadMessagetable can abort the run part way through. Consider setting both to0for this session at the start of the transaction, and record the expected wall-clock duration in the cutover runbook.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/private/db/src/scripts/migrateLegacyModmail.ts` around lines 999 - 1005, At the start of the target transaction callback in the live migration path, configure the transaction session with both idle_in_transaction_session_timeout and statement_timeout set to 0 before calling runMigration; preserve the dry-run rollback behavior. Also document the migration’s expected wall-clock duration in the cutover runbook.
863-876: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlso report target threads that have no legacy counterpart.
The loop iterates
legacyPerThreadonly. A migrated thread present in the target but absent from legacy is never reported. That case arises after a partially applied run, which is exactly when an operator reaches for--verify. Add a reverse pass overtargetPerThread.♻️ Proposed addition
const mismatched: string[] = []; for (const [modThreadId, expected] of legacyPerThread) { const actual = targetPerThread.get(modThreadId); if (actual !== expected) { mismatched.push(`${modThreadId} (legacy=${expected} target=${actual ?? 'missing'})`); } } + + for (const modThreadId of targetPerThread.keys()) { + if (!legacyPerThread.has(modThreadId)) { + mismatched.push(`${modThreadId} (legacy=missing target=${targetPerThread.get(modThreadId)})`); + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/private/db/src/scripts/migrateLegacyModmail.ts` around lines 863 - 876, Extend the verification logic around legacyPerThread and targetPerThread to also iterate targetPerThread and append any thread IDs absent from legacyPerThread to mismatched, including the target message count in the diagnostic. Keep the existing mismatch reporting and success behavior, ensuring extra target-only threads make verification fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/roadmap/06-modmail-port.md`:
- Line 122: Update the GuildSettings-to-guild_settings mapping entry to state
that mod_forum_id is never written by the migration script for any guild, rather
than instructing operators to confirm the legacy channel type during the run.
Reference item 1 for the rationale and required manual handling of forum-channel
setup.
In `@packages/private/db/src/scripts/migrateLegacyModmail.ts`:
- Around line 659-661: Guard the collidingSnippets query in
packages/private/db/src/scripts/migrateLegacyModmail.ts:659-661 by moving it
inside the existing guildIds.length > 0 block. At
packages/private/db/src/scripts/migrateLegacyModmail.ts:751-756, guard the
guildIds query/use at line 731 similarly and short-circuit with
report('snippet_updates', 0, 0) when both migratedLegacyIds and
migratedTargetIds are empty.
---
Nitpick comments:
In `@packages/private/db/src/scripts/migrateLegacyModmail.ts`:
- Around line 999-1005: At the start of the target transaction callback in the
live migration path, configure the transaction session with both
idle_in_transaction_session_timeout and statement_timeout set to 0 before
calling runMigration; preserve the dry-run rollback behavior. Also document the
migration’s expected wall-clock duration in the cutover runbook.
- Around line 863-876: Extend the verification logic around legacyPerThread and
targetPerThread to also iterate targetPerThread and append any thread IDs absent
from legacyPerThread to mismatched, including the target message count in the
diagnostic. Keep the existing mismatch reporting and success behavior, ensuring
extra target-only threads make verification fail.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 58f1c620-f9ef-42d7-aa9c-26fe300bf7b8
📒 Files selected for processing (6)
docs/roadmap/01-architecture.mddocs/roadmap/06-modmail-port.mdpackage.jsonpackages/private/db/src/scripts/migrateLegacyModmail.tspackages/private/db/tsconfig.jsonservices/api/src/routes/modmail/threads/util.ts
Code reviewFound 3 high-signal issues (validated independently against the actual source): 1.
|
1 participant
Closes #157