chore: Add new data migration solution for safe migrations#39578
Open
chore: Add new data migration solution for safe migrations#39578
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: b2d735e The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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:
WalkthroughAdds a versioned data-migration framework with registration, ordered execution, distributed locking, migration records, downgrade/manual-reversion checks, generation tooling, models/typings, startup integration, and comprehensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Startup as Server Startup
participant Runner as Migration Runner
participant Locks as SystemLocks
participant DB as DataMigrations DB
participant Migration as Migration.run()
Startup->>Runner: runDataMigrations()
Runner->>Runner: compute commit/version hash & sort migrations
Runner->>Locks: acquireLock('data_migrations')
alt lock acquired
Locks-->>Runner: {acquired: true, record}
Runner->>DB: read migration records
Runner->>Runner: determine upgrade vs downgrade
alt downgrade && manual reversion required
Runner->>Startup: show error box / halt
else proceed
loop for each eligible migration by order
Runner->>Migration: migration.run()
alt success
Migration-->>Runner: success
Runner->>DB: upsert completed record
else failure
Migration-->>Runner: error
Runner->>DB: update failed record (increment runCount)
alt fatal
Runner->>Startup: exit process
else
Runner->>Startup: show warning (retry later)
end
end
end
Runner->>Locks: releaseLock('data_migrations', { lastVersion })
Locks-->>Runner: released
end
else lock acquisition failed
Locks-->>Runner: {acquired: false}
Runner->>Startup: show fatal error & exit
end
Runner-->>Startup: complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
Introduces a new framework for managing data migrations, including versioning, ordering, and safe execution of changes. It integrates with the app's startup process and includes new models and templates.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/.scripts/make-data-migration.ts">
<violation number="1" location="apps/meteor/.scripts/make-data-migration.ts:61">
P2: Return a non-zero process exit code when migration generation fails; logging alone makes failures appear successful.</violation>
</file>
<file name="packages/core-typings/src/ISystemLock.ts">
<violation number="1" location="packages/core-typings/src/ISystemLock.ts:14">
P2: This union is redundant because `ISystemLockMigration` already extends `ISystemLockBase`, so `ISystemLock` effectively collapses to `ISystemLockBase` and loses the intended migration-specific typing.</violation>
</file>
<file name="apps/meteor/server/lib/dataMigrations.ts">
<violation number="1" location="apps/meteor/server/lib/dataMigrations.ts:74">
P1: Global `currentAttempt` is never reset between invocations. If `runDataMigrations()` is called multiple times (e.g., in tests), subsequent calls will have exhausted retry attempts. Reset the counter at the start of `acquireLock()` or pass it as a parameter.</violation>
</file>
<file name="packages/models/src/models/SystemLocks.ts">
<violation number="1" location="packages/models/src/models/SystemLocks.ts:16">
P1: The default 5-minute stale-lock window can let a second instance steal the migration lock while the first instance is still running long migrations, causing concurrent migration execution.
(Based on your team's feedback about concurrency and race-condition risks.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| extraData?: { lastVersion?: string }; | ||
| } | ||
|
|
||
| export type ISystemLock = ISystemLockBase | ISystemLockMigration; |
Contributor
There was a problem hiding this comment.
P2: This union is redundant because ISystemLockMigration already extends ISystemLockBase, so ISystemLock effectively collapses to ISystemLockBase and loses the intended migration-specific typing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core-typings/src/ISystemLock.ts, line 14:
<comment>This union is redundant because `ISystemLockMigration` already extends `ISystemLockBase`, so `ISystemLock` effectively collapses to `ISystemLockBase` and loses the intended migration-specific typing.</comment>
<file context>
@@ -0,0 +1,14 @@
+ extraData?: { lastVersion?: string };
+}
+
+export type ISystemLock = ISystemLockBase | ISystemLockMigration;
</file context>
coderabbitai
Bot
added
the
type: feature
![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: 7
🧹 Nitpick comments (3)
apps/meteor/.scripts/data-migration.template (1)
10-12: Replace empty comment placeholder with a meaningful placeholder or TODO.The empty comment
//on line 11 should be replaced. Consider using a throw statement or a more explicit placeholder that guides developers.Suggested improvement
async run() { - // + throw new Error('TODO: Implement migration logic'); },As per coding guidelines: "Avoid code comments in the implementation" for
**/*.{ts,tsx,js}files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/.scripts/data-migration.template` around lines 10 - 12, The empty placeholder inside the async run() method should be replaced with an explicit TODO or a fail-fast statement to guide future implementers; update the run() function in the template to either throw a clear Error (e.g., throw new Error('Migration run() not implemented: add migration logic here')) or include a descriptive TODO comment string (e.g., /** TODO: implement migration steps for this template */) so the placeholder is explicit and actionable rather than just "//".apps/meteor/server/startup/dataMigrations/index.ts (1)
1-2: Consider the coding guideline on comments.The coding guidelines specify to avoid code comments in TypeScript/JavaScript implementation files. However, since this file serves as a documentation placeholder for the migration import hub (with actual migrations added by the generator script), these instructional comments may be acceptable as developer documentation rather than inline code comments.
If strict adherence is preferred, consider moving this guidance to a README or the PR description instead.
As per coding guidelines: "Avoid code comments in the implementation" for
**/*.{ts,tsx,js}files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/startup/dataMigrations/index.ts` around lines 1 - 2, This file currently contains developer-facing instructional comments about adding data migrations; to comply with the guideline to avoid comments in implementation files, move the instructions into a README or CONTRIBUTING note (or the repository PR template) and replace the inline comments in apps/meteor/server/startup/dataMigrations/index.ts with either an explanatory JSDoc-style header that is allowed by policy or an empty file with a brief one-line export stub if required by the importer; reference the generator command "npm run data-migration:add <id> <description>" in the new README/CONTRIBUTING so contributors can find the workflow without implementation-file comments.apps/meteor/.scripts/make-data-migration.ts (1)
36-58: Drop the inline narration.These comments only restate the next statement and make a small script noisier than it needs to be.
As per coding guidelines, "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/.scripts/make-data-migration.ts` around lines 36 - 58, The file contains inline explanatory comments that simply restate the next statement (e.g., the comments before the writeFileSync and readFileSync calls and the "generate new migration file", "get contents of index.ts to append new migration", "append migration import to index file" notes); remove these redundant comments and keep only meaningful comments if needed, then ensure functions/variables like existingWithId, getNextOrder, padOrder, renderFile, writeFileSync, readFileSync, indexFile, data, and fileName remain unchanged and the script flow (checking existingWithId, computing order/fileName, rendering via renderFile, writing new migration and updating index.ts) is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/.scripts/make-data-migration.ts`:
- Around line 23-25: The id passed into main is used directly in file paths and
an import which allows path-traversal or invalid TS if it contains slashes,
dots, quotes or newlines; enforce a strict slug (e.g. allow only [a-z0-9_-] and
reject or normalize anything else) at the start of main before any filesystem or
import usage (function main). Reject the call or exit with an error if id fails
the regex, or sanitize it to a safe filename, and then use the
validated/sanitized value for the generated filename
(server/startup/dataMigrations/${id}.ts) and the dynamic import string
(import("./server/startup/dataMigrations/${id}")) as well as for every other
interpolation site in this file.
- Around line 23-31: Update the main migration generation flow to fail the
process on validation or I/O errors instead of just logging and returning: in
the main(id: string, description: string) function, when id or description is
whitespace-only, set process.exitCode = 1 (or throw) after logging; likewise,
when detecting a duplicate migration id (the duplicate-id check surrounding the
generation logic) or when template rendering / file write operations (the code
paths handling template creation and fs.writeFileSync or write operations) fail,
propagate the error or set process.exitCode = 1 so the CLI exits non-zero.
Ensure any top-level caller either catches and logs the error then sets
process.exitCode = 1 or lets the thrown error bubble to the process so
CI/automation sees the failure.
In `@apps/meteor/server/lib/dataMigrations.ts`:
- Around line 76-97: The downgrade rerun key needs to be based on the
version/hash being downgraded from, not the target/currentHash; update the
migration record and gate logic so downgrades compare against a stored "from"
hash. Add a field (e.g. lastRunFromHash) to IDataMigrationRecord and ensure code
that records migration runs sets lastRunFromHash = the source version/hash when
executing a downgrade; then change shouldRun to, when !isUpgrade (downgrade),
compare record.lastRunFromHash !== currentHashFromSource (use the new
lastRunFromHash) instead of comparing record.lastRunHash to currentHash, while
leaving the upgrade path using record.lastRunHash vs currentHash and keeping
existing checks for migration.direction and migration.strategy in shouldRun.
- Around line 71-75: The module-scoped currentAttempt causes retry state to leak
across runs and the check uses <= which allows one extra retry; make the retry
counter local and bounded per call by moving currentAttempt into acquireLock()
(or into runDataMigrations() and pass it into acquireLock()) so each invocation
starts at 0, use a strict < maxAttempts loop/condition instead of <=, and
preserve existing LOCK_KEY, maxAttempts, retryInterval names when updating
acquireLock() and runDataMigrations() signatures/usages.
- Around line 156-170: The process is exiting while a data_migrations lock
remains held; in checkManualReversions() (and the fatal-migration path that
calls showErrorBox) ensure you explicitly release the lock on the
data_migrations collection before calling process.exit(1) (or instead throw an
error so the existing finally block that unlocks runs); update the code paths
that call showErrorBox + process.exit (including the fatal-migration branch) to
call the unlock/release function used when acquiring the lock (or rework to
throw) so the lock is always cleared before termination.
In `@apps/meteor/server/startup/migrations/xrun.ts`:
- Around line 6-9: The import '../dataMigrations' unconditionally loads and
executes the current data-migration set, causing head data mutations even when
migrateDatabase() is targeted to an older MIGRATION_VERSION; remove or stop the
side-effect import and instead gate running runDataMigrations so it only
executes when starting up to the full latest schema (e.g., invoke
runDataMigrations conditionally from the migrateDatabase/onServerVersionChange
flow or after confirming target == 'latest'), and ensure you reference and call
runDataMigrations only when onServerVersionChange/migrateDatabase indicate a
latest startup; update/remove the top-level import '../dataMigrations' and move
its invocation into the conditional logic around migrateDatabase,
onServerVersionChange, runDataMigrations.
In `@packages/models/src/models/SystemLocks.ts`:
- Around line 16-33: The lock implementation in acquireLock/ releaseLock (used
by runDataMigrations) is vulnerable to lease stealing because lockedAt is only
set once and staleThreshold can cause another node to acquire the same _id while
the original holder is still running; change the scheme to include an owner
token (e.g., lockOwner or token) stored with the lock record, set token in
acquireLock, and require token match in releaseLock before clearing; implement
periodic lease renewal/heartbeat that updates lockedAt only when the token
matches to extend ownership during long-running migrations, and ensure
findOneAndUpdate/queries use both _id and token where appropriate to avoid
releasing or renewing locks held by others.
---
Nitpick comments:
In `@apps/meteor/.scripts/data-migration.template`:
- Around line 10-12: The empty placeholder inside the async run() method should
be replaced with an explicit TODO or a fail-fast statement to guide future
implementers; update the run() function in the template to either throw a clear
Error (e.g., throw new Error('Migration run() not implemented: add migration
logic here')) or include a descriptive TODO comment string (e.g., /** TODO:
implement migration steps for this template */) so the placeholder is explicit
and actionable rather than just "//".
In `@apps/meteor/.scripts/make-data-migration.ts`:
- Around line 36-58: The file contains inline explanatory comments that simply
restate the next statement (e.g., the comments before the writeFileSync and
readFileSync calls and the "generate new migration file", "get contents of
index.ts to append new migration", "append migration import to index file"
notes); remove these redundant comments and keep only meaningful comments if
needed, then ensure functions/variables like existingWithId, getNextOrder,
padOrder, renderFile, writeFileSync, readFileSync, indexFile, data, and fileName
remain unchanged and the script flow (checking existingWithId, computing
order/fileName, rendering via renderFile, writing new migration and updating
index.ts) is preserved.
In `@apps/meteor/server/startup/dataMigrations/index.ts`:
- Around line 1-2: This file currently contains developer-facing instructional
comments about adding data migrations; to comply with the guideline to avoid
comments in implementation files, move the instructions into a README or
CONTRIBUTING note (or the repository PR template) and replace the inline
comments in apps/meteor/server/startup/dataMigrations/index.ts with either an
explanatory JSDoc-style header that is allowed by policy or an empty file with a
brief one-line export stub if required by the importer; reference the generator
command "npm run data-migration:add <id> <description>" in the new
README/CONTRIBUTING so contributors can find the workflow without
implementation-file comments.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 193b7202-abe4-49d5-8559-f69436bb910a
📒 Files selected for processing (20)
.changeset/quick-toys-tap.mdapps/meteor/.scripts/data-migration.templateapps/meteor/.scripts/make-data-migration.tsapps/meteor/jest.config.tsapps/meteor/package.jsonapps/meteor/server/lib/dataMigrations.spec.tsapps/meteor/server/lib/dataMigrations.tsapps/meteor/server/models.tsapps/meteor/server/startup/dataMigrations/index.tsapps/meteor/server/startup/migrations/xrun.tspackages/core-typings/src/ISystemLock.tspackages/core-typings/src/index.tspackages/core-typings/src/migrations/IDataMigrationRecord.tspackages/model-typings/src/index.tspackages/model-typings/src/models/IDataMigrationsModel.tspackages/model-typings/src/models/ISystemLocksModel.tspackages/models/src/index.tspackages/models/src/modelClasses.tspackages/models/src/models/DataMigrations.tspackages/models/src/models/SystemLocks.ts
📜 Review details
⏰ 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). (6)
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 📦 Meteor Build (coverage)
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/jest.config.tspackages/models/src/modelClasses.tspackages/core-typings/src/migrations/IDataMigrationRecord.tsapps/meteor/server/startup/dataMigrations/index.tspackages/models/src/models/DataMigrations.tspackages/models/src/index.tspackages/model-typings/src/models/ISystemLocksModel.tspackages/models/src/models/SystemLocks.tspackages/core-typings/src/ISystemLock.tspackages/model-typings/src/models/IDataMigrationsModel.tsapps/meteor/server/lib/dataMigrations.tsapps/meteor/server/startup/migrations/xrun.tsapps/meteor/server/lib/dataMigrations.spec.tspackages/core-typings/src/index.tspackages/model-typings/src/index.tsapps/meteor/server/models.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/server/lib/dataMigrations.spec.ts
🧠 Learnings (26)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/jest.config.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/jest.config.tsapps/meteor/server/startup/dataMigrations/index.tsapps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/jest.config.tsapps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/jest.config.tsapps/meteor/server/lib/dataMigrations.tsapps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/jest.config.tsapps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/jest.config.tsapps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/jest.config.tsapps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/jest.config.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/jest.config.tsapps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/jest.config.tsapps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.spec.ts : Use `.spec.ts` extension for test files (e.g., `login.spec.ts`)
Applied to files:
apps/meteor/jest.config.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Store commonly used locators in variables/constants for reuse
Applied to files:
apps/meteor/jest.config.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/jest.config.tspackages/models/src/modelClasses.tspackages/core-typings/src/migrations/IDataMigrationRecord.tsapps/meteor/server/startup/dataMigrations/index.tspackages/models/src/models/DataMigrations.tspackages/models/src/index.tspackages/model-typings/src/models/ISystemLocksModel.tspackages/models/src/models/SystemLocks.tspackages/core-typings/src/ISystemLock.tspackages/model-typings/src/models/IDataMigrationsModel.tsapps/meteor/server/lib/dataMigrations.tsapps/meteor/server/startup/migrations/xrun.tsapps/meteor/server/lib/dataMigrations.spec.tspackages/core-typings/src/index.tspackages/model-typings/src/index.tsapps/meteor/server/models.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/jest.config.tspackages/models/src/modelClasses.tspackages/core-typings/src/migrations/IDataMigrationRecord.tsapps/meteor/server/startup/dataMigrations/index.tspackages/models/src/models/DataMigrations.tspackages/models/src/index.tspackages/model-typings/src/models/ISystemLocksModel.tspackages/models/src/models/SystemLocks.tspackages/core-typings/src/ISystemLock.tspackages/model-typings/src/models/IDataMigrationsModel.tsapps/meteor/server/lib/dataMigrations.tsapps/meteor/server/startup/migrations/xrun.tsapps/meteor/server/lib/dataMigrations.spec.tspackages/core-typings/src/index.tspackages/model-typings/src/index.tsapps/meteor/server/models.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.{ts,tsx,js} : Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Applied to files:
apps/meteor/package.json
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/server/startup/dataMigrations/index.tsapps/meteor/server/models.ts
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/server/startup/dataMigrations/index.ts
📚 Learning: 2026-03-04T14:16:49.202Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39304
File: packages/ui-contexts/src/ActionManagerContext.ts:26-26
Timestamp: 2026-03-04T14:16:49.202Z
Learning: In `packages/ui-contexts/src/ActionManagerContext.ts` (TypeScript, RocketChat/Rocket.Chat), the `disposeView` method in `IActionManager` uses an intentionally explicit union `UiKit.ModalView['id'] | UiKit.BannerView['viewId'] | UiKit.ContextualBarView['id']` to document which view types are accepted, even though all constituents resolve to the same primitive. The inline `// eslint-disable-next-line typescript-eslint/no-duplicate-type-constituents` comment is intentional and should not be flagged or removed.
Applied to files:
packages/core-typings/src/ISystemLock.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
apps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
.changeset/quick-toys-tap.md
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
.changeset/quick-toys-tap.md
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Applied to files:
.changeset/quick-toys-tap.md
🔇 Additional comments (7)
apps/meteor/jest.config.ts (1)
52-52: LGTM!The test match path follows the established pattern for server-side test files in this configuration.
packages/models/src/modelClasses.ts (1)
69-70: LGTM!The new exports for
DataMigrationsandSystemLocksfollow the established pattern in this file. The model registration is handled inapps/meteor/server/models.ts(per the context snippets showingregisterModel('IDataMigrationsModel', ...)calls at lines 135-136).packages/model-typings/src/index.ts (1)
76-77: LGTM!The new type exports follow the established pattern and are logically grouped near the existing
IMigrationsModelexport.apps/meteor/package.json (1)
44-44: LGTM!The new
data-migration:addscript follows the same pattern as the existingmigration:addscript, usingts-node-transpile-onlywith--skip-projectflag consistently..changeset/quick-toys-tap.md (1)
1-10: LGTM!The changeset correctly documents the scope of changes across the affected packages with an appropriate patch-level version bump and clear description.
packages/models/src/index.ts (2)
76-77: LGTM!The type imports for the new model interfaces are correctly added.
205-206: LGTM!The proxified model exports use string literals that match the
registerModel()calls inapps/meteor/server/models.ts:135-136. The models will be properly resolved at runtime.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #39578 +/- ##
===========================================
- Coverage 70.94% 70.87% -0.08%
===========================================
Files 3198 3207 +9
Lines 113370 114120 +750
Branches 20506 20744 +238
===========================================
+ Hits 80432 80882 +450
- Misses 30890 31190 +300
Partials 2048 2048
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/.scripts/make-data-migration.ts">
<violation number="1" location="apps/meteor/.scripts/make-data-migration.ts:47">
P2: Set a non-zero exit code when the migration id already exists; otherwise duplicate-id failures still look successful to CI or wrapper scripts.</violation>
</file>
<file name="packages/models/src/models/SystemLocks.ts">
<violation number="1" location="packages/models/src/models/SystemLocks.ts:52">
P1: Renewing the lock by `_id` alone is not ownership-safe. After a stale lock is reacquired by another instance, the previous holder can still refresh the new holder's lease, which undermines exclusivity for data migrations.
(Based on your team's feedback about concurrency.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/meteor/server/lib/dataMigrations.ts (3)
46-58: Consider validatingorderand detecting duplicate ids.The function validates
id,description, andrun(), but doesn't validate thatorderis a positive number or check for duplicate migration ids. Two migrations with the sameid(different object references) or sameordercould cause unexpected behavior.Proposed validation improvements
export function addDataMigration(migration: IDataMigration): void { if (!migration.id) { throw new Error('Data migration id is required'); } if (!migration.description) { throw new Error('Data migration description is required'); } if (!migration.run) { throw new Error('Data migration run() is required'); } + if (typeof migration.order !== 'number' || migration.order < 0) { + throw new Error('Data migration order must be a non-negative number'); + } + + const existingWithSameId = Array.from(dataMigrations).find((m) => m.id === migration.id); + if (existingWithSameId) { + throw new Error(`Data migration with id "${migration.id}" already registered`); + } dataMigrations.add(migration); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/lib/dataMigrations.ts` around lines 46 - 58, addDataMigration currently checks id, description and run but does not validate migration.order or guard against duplicate ids/orders; update addDataMigration to (1) ensure migration.order exists and is a finite positive integer (or non-negative number per project convention) and throw a clear Error if not, (2) check the existing dataMigrations collection for any entry with the same migration.id and throw an Error for duplicate ids, and (3) optionally check for conflicting migration.order values and throw or warn; reference the addDataMigration function, the migration.id and migration.order properties, and the dataMigrations collection when implementing these validations.
146-155: Minor type safety concern withas anycast.The query at line 151 uses
as anyto bypass TypeScript checking. This could mask type errors if theIDataMigrationRecordtype changes.Consider defining a proper filter type or ensuring the model's type definitions support this query shape.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/lib/dataMigrations.ts` around lines 146 - 155, The query in checkManualReversions uses an unsafe cast (`as any`) on DataMigrations.find; replace it with a properly typed filter for the collection (e.g., use the collection's Filter<IDataMigrationRecord> type or Partial/IQuery type that matches IDataMigrationRecord) so the object { order: { $gt: maxRegisteredOrder }, status: 'completed', requiresManualReversion: true } is passed with the correct type; update the signature or import the appropriate Filter type from your Mongo driver and apply it to the DataMigrations.find call to remove the `as any` cast and preserve type-safety.
255-297: Consider extracting common record update logic.The success (lines 255-274) and failure (lines 280-297) update operations share significant structure. Extracting a helper function could reduce duplication and make the code easier to maintain.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/lib/dataMigrations.ts` around lines 255 - 297, Extract the shared update logic around DataMigrations.updateOne into a helper (e.g., updateMigrationRecord) that accepts recordId, status ('completed' | 'failed'), errorMsg (optional), migration and currentHash; build the common $set payload (lastRunAt, lastRunHash=currentHash, order=migration.order, requiresManualReversion and manualReversionInstructions when present) and always $inc runCount, then conditionally add $unset: { lastError: 1 } for 'completed' or $set: { lastError: errorMsg } for 'failed'; replace the two DataMigrations.updateOne blocks in the try and catch with calls to this helper (keeping upsert: true) and preserve the log.startup call in the success path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/server/lib/dataMigrations.ts`:
- Around line 299-305: When a fatal migration occurs inside the migration
handler (migration.fatal branch) you must explicitly release the data_migrations
lock before calling process.exit(1); update the migration failure path so it
calls the existing lock-release routine (the function that clears the
"data_migrations" lock used by this module) — and await it if it's async —
immediately after showWarningBox and before process.exit(1). Ensure you
reference the same lock cleanup function used in the finally block so the lock
is cleared deterministically even on fatal failures.
---
Nitpick comments:
In `@apps/meteor/server/lib/dataMigrations.ts`:
- Around line 46-58: addDataMigration currently checks id, description and run
but does not validate migration.order or guard against duplicate ids/orders;
update addDataMigration to (1) ensure migration.order exists and is a finite
positive integer (or non-negative number per project convention) and throw a
clear Error if not, (2) check the existing dataMigrations collection for any
entry with the same migration.id and throw an Error for duplicate ids, and (3)
optionally check for conflicting migration.order values and throw or warn;
reference the addDataMigration function, the migration.id and migration.order
properties, and the dataMigrations collection when implementing these
validations.
- Around line 146-155: The query in checkManualReversions uses an unsafe cast
(`as any`) on DataMigrations.find; replace it with a properly typed filter for
the collection (e.g., use the collection's Filter<IDataMigrationRecord> type or
Partial/IQuery type that matches IDataMigrationRecord) so the object { order: {
$gt: maxRegisteredOrder }, status: 'completed', requiresManualReversion: true }
is passed with the correct type; update the signature or import the appropriate
Filter type from your Mongo driver and apply it to the DataMigrations.find call
to remove the `as any` cast and preserve type-safety.
- Around line 255-297: Extract the shared update logic around
DataMigrations.updateOne into a helper (e.g., updateMigrationRecord) that
accepts recordId, status ('completed' | 'failed'), errorMsg (optional),
migration and currentHash; build the common $set payload (lastRunAt,
lastRunHash=currentHash, order=migration.order, requiresManualReversion and
manualReversionInstructions when present) and always $inc runCount, then
conditionally add $unset: { lastError: 1 } for 'completed' or $set: { lastError:
errorMsg } for 'failed'; replace the two DataMigrations.updateOne blocks in the
try and catch with calls to this helper (keeping upsert: true) and preserve the
log.startup call in the success path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ab0158c-2c66-4993-8ad7-5d0699505784
📒 Files selected for processing (1)
apps/meteor/server/lib/dataMigrations.ts
📜 Review details
⏰ 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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/server/lib/dataMigrations.ts
🧠 Learnings (4)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/server/lib/dataMigrations.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/server/lib/dataMigrations.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/server/lib/dataMigrations.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/server/lib/dataMigrations.ts
🔇 Additional comments (5)
apps/meteor/server/lib/dataMigrations.ts (5)
1-44: LGTM!The imports, type definitions, and helper utilities are well-structured. The discriminated union for
IDataMigrationproperly enforces thatmanualReversionInstructionsis required whenrequiresManualReversionistrue. ThegetRecordIdfunction correctly zero-pads the order for lexicographic sorting.
60-69: LGTM!The
semver.coerceapproach handles various version string formats gracefully, and defaulting tofalsewhen parsing fails is a safe choice to avoid incorrectly triggering downgrade logic.
71-74: Keep lock retry state local to one migration run.
currentAttemptis module-scoped and the check at line 116 uses<= maxAttemptswhich allows 31 attempts (0 through 30 inclusive) instead of the intended 30. WhilecurrentAttemptis reset at line 188, the recursive pattern with module-scoped state is fragile. Converting to an iterative loop with a local counter would be more robust.Also applies to: 116-127
76-98: LGTM!The
shouldRunlogic correctly handles:
- Direction filtering (upgrade-only, downgrade-only, or both)
- First-time execution (no record)
- Retry after failure
oncevsevery-upgradestrategies
187-226: LGTM!The initialization logic properly:
- Resets retry counter
- Validates commit hash availability
- Sorts migrations deterministically
- Releases lock when skipping due to same version
- Detects downgrades via both version comparison and migration order
coderabbitai
Bot
added
type: chore
and removed
type: feature
Contributor
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/server/lib/dataMigrations.ts">
<violation number="1" location="apps/meteor/server/lib/dataMigrations.ts:309">
P1: Ensure the fatal migration path still exits if releasing the lock fails.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+309
to
+310
| await releaseLock(); | ||
| process.exit(1); |
Contributor
There was a problem hiding this comment.
P1: Ensure the fatal migration path still exits if releasing the lock fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/lib/dataMigrations.ts, line 309:
<comment>Ensure the fatal migration path still exits if releasing the lock fails.</comment>
<file context>
@@ -301,6 +306,7 @@ export async function runDataMigrations(): Promise<void> {
'DATA MIGRATION FAILED (FATAL)',
[`Migration "${recordId}" failed:`, errorMsg, '', 'Server cannot continue.'].join('\n'),
);
+ await releaseLock();
process.exit(1);
}
</file context>
Suggested change
| await releaseLock(); | |
| process.exit(1); | |
| try { | |
| await releaseLock(); | |
| } finally { | |
| process.exit(1); | |
| } |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/meteor/server/lib/dataMigrations.spec.ts (1)
262-278: Add a regression test for “failed migration retries on next startup (same version)”.Current failure coverage only checks the first run. A second
runDataMigrations()invocation in the same version should be asserted to execute failed migrations again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/lib/dataMigrations.spec.ts` around lines 262 - 278, Extend the test for failing migrations to assert retries on subsequent startup by calling runDataMigrations() a second time after the first run, and assert the migration's run function (runFn) is invoked again and that mockUpdateOne and mockShowWarningBox are called for the retry; specifically, in the test using addDataMigration(createMigration({ id: 'failing-migration', order: 107, run: runFn })), after the initial await runDataMigrations() add a second await runDataMigrations() and add expectations like expect(runFn).toHaveBeenCalledTimes(2) and that mockUpdateOne and mockShowWarningBox were called for the retry (still containing status: 'failed' and lastError: 'Something broke').apps/meteor/.scripts/make-data-migration.ts (1)
46-46: Remove inline implementation comments in this script.Please drop these inline comments to stay aligned with repository implementation style for TS/JS files.
As per coding guidelines: "Avoid code comments in the implementation".
Also applies to: 59-59, 62-62, 68-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/.scripts/make-data-migration.ts` at line 46, Remove the inline implementation comments in the make-data-migration script (e.g. the "// check if migration id already exists" line and the other inline comments at the mentioned locations) so the code follows the repository style of avoiding implementation comments; simply delete those comment lines in the script (no code changes needed) and keep surrounding logic and identifiers intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/.scripts/make-data-migration.ts`:
- Around line 38-42: The generated migration file is currently vulnerable to
invalid syntax when description contains quotes, backslashes, or newlines;
before embedding the description into the file template (the description
variable used in make-data-migration.ts and the similar use at the other spot
around lines 57-60), escape it or serialize it so it becomes a safe string
literal: e.g. replace backslashes with \\\\, single quotes with \\' (or prefer
JSON.stringify(description) to produce a valid quoted string), and normalize
newlines to escaped `\n` sequences so the rendered
server/startup/dataMigrations/*.ts file always contains a valid string literal.
In `@apps/meteor/server/lib/dataMigrations.ts`:
- Around line 225-231: The current short-circuit uses lastVersion alone and
prevents retrying failed/pending migrations; modify the logic so we only skip
when the previous run for Info.version completed successfully. Concretely: stop
writing extraData.lastVersion in the finally path on failure (only set it on
successful completion), and change the guard that reads lockRecord/extraData
(the check using lastVersion and isSystemLockMigration at the top of the snippet
and the analogous checks around lines 313–321) to require a success/completed
flag (e.g. extraData.migrationStatus === 'completed' or similar) in addition to
lastVersion === Info.version before returning and calling releaseLock; ensure
releaseLock behavior remains correct when allowing retries after failures.
---
Nitpick comments:
In `@apps/meteor/.scripts/make-data-migration.ts`:
- Line 46: Remove the inline implementation comments in the make-data-migration
script (e.g. the "// check if migration id already exists" line and the other
inline comments at the mentioned locations) so the code follows the repository
style of avoiding implementation comments; simply delete those comment lines in
the script (no code changes needed) and keep surrounding logic and identifiers
intact.
In `@apps/meteor/server/lib/dataMigrations.spec.ts`:
- Around line 262-278: Extend the test for failing migrations to assert retries
on subsequent startup by calling runDataMigrations() a second time after the
first run, and assert the migration's run function (runFn) is invoked again and
that mockUpdateOne and mockShowWarningBox are called for the retry;
specifically, in the test using addDataMigration(createMigration({ id:
'failing-migration', order: 107, run: runFn })), after the initial await
runDataMigrations() add a second await runDataMigrations() and add expectations
like expect(runFn).toHaveBeenCalledTimes(2) and that mockUpdateOne and
mockShowWarningBox were called for the retry (still containing status: 'failed'
and lastError: 'Something broke').
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f88be8d0-67ad-415b-8bf9-7cc48ac5adba
📒 Files selected for processing (6)
apps/meteor/.scripts/make-data-migration.tsapps/meteor/server/lib/dataMigrations.spec.tsapps/meteor/server/lib/dataMigrations.tspackages/core-typings/src/ISystemLock.tspackages/model-typings/src/models/ISystemLocksModel.tspackages/models/src/models/SystemLocks.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/models/src/models/SystemLocks.ts
📜 Review details
⏰ 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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/server/lib/dataMigrations.tspackages/model-typings/src/models/ISystemLocksModel.tspackages/core-typings/src/ISystemLock.tsapps/meteor/server/lib/dataMigrations.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/server/lib/dataMigrations.spec.ts
🧠 Learnings (18)
📚 Learning: 2026-03-11T18:15:53.272Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/app/api/server/v1/rooms.ts:294-296
Timestamp: 2026-03-11T18:15:53.272Z
Learning: In Rocket.Chat's `rooms.mediaConfirm/:rid/:fileId` endpoint (apps/meteor/app/api/server/v1/rooms.ts), updating `file.name` from `bodyParams.fileName` without updating `file.path` is intentionally safe. The file path is keyed by `_id` (not by filename), so the stored path remains valid regardless of a rename. `file.name` only affects the display name in the message attachment; do not flag this as a path-divergence issue.
Applied to files:
apps/meteor/.scripts/make-data-migration.ts
📚 Learning: 2026-03-03T11:11:48.541Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
Applied to files:
apps/meteor/.scripts/make-data-migration.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/server/lib/dataMigrations.tsapps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/server/lib/dataMigrations.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/server/lib/dataMigrations.tspackages/model-typings/src/models/ISystemLocksModel.tspackages/core-typings/src/ISystemLock.tsapps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/server/lib/dataMigrations.tspackages/model-typings/src/models/ISystemLocksModel.tspackages/core-typings/src/ISystemLock.tsapps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2026-03-04T14:16:49.202Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39304
File: packages/ui-contexts/src/ActionManagerContext.ts:26-26
Timestamp: 2026-03-04T14:16:49.202Z
Learning: In `packages/ui-contexts/src/ActionManagerContext.ts` (TypeScript, RocketChat/Rocket.Chat), the `disposeView` method in `IActionManager` uses an intentionally explicit union `UiKit.ModalView['id'] | UiKit.BannerView['viewId'] | UiKit.ContextualBarView['id']` to document which view types are accepted, even though all constituents resolve to the same primitive. The inline `// eslint-disable-next-line typescript-eslint/no-duplicate-type-constituents` comment is intentional and should not be flagged or removed.
Applied to files:
packages/core-typings/src/ISystemLock.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
apps/meteor/server/lib/dataMigrations.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/server/lib/dataMigrations.spec.ts
🔇 Additional comments (2)
packages/core-typings/src/ISystemLock.ts (1)
10-15: Lock typing specialization looks good.The
_id: 'data_migrations'specialization andextraData.lastVersionshape align well with migration lock metadata usage.packages/model-typings/src/models/ISystemLocksModel.ts (1)
5-14: Model contract is consistent with the raw implementation.
acquireLock/renewLockThreshold/releaseLocksignatures align cleanly withSystemLocksRawusage.
Comment on lines
+38
to
+42
| if (!description.trim()) { | ||
| console.error('2nd param must be a non-empty description'); | ||
| process.exitCode = 1; | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Escape description before rendering the migration file.
A description containing ', \, or line breaks can produce invalid generated code in server/startup/dataMigrations/*.ts.
Proposed hardening
function main(id: string, description: string): void {
const normalizedId = id.trim();
+ const normalizedDescription = description.trim();
if (!normalizedId) {
console.error('1st param must be a non-empty migration id');
process.exitCode = 1;
return;
}
@@
- if (!description.trim()) {
+ if (!normalizedDescription) {
console.error('2nd param must be a non-empty description');
process.exitCode = 1;
return;
}
@@
- renderFile('./.scripts/data-migration.template', { id: normalizedId, description, order })
+ const escapedDescription = normalizedDescription
+ .replace(/\\/g, '\\\\')
+ .replace(/'/g, "\\'")
+ .replace(/\r?\n/g, '\\n');
+
+ renderFile('./.scripts/data-migration.template', { id: normalizedId, description: escapedDescription, order })Also applies to: 57-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/.scripts/make-data-migration.ts` around lines 38 - 42, The
generated migration file is currently vulnerable to invalid syntax when
description contains quotes, backslashes, or newlines; before embedding the
description into the file template (the description variable used in
make-data-migration.ts and the similar use at the other spot around lines
57-60), escape it or serialize it so it becomes a safe string literal: e.g.
replace backslashes with \\\\, single quotes with \\' (or prefer
JSON.stringify(description) to produce a valid quoted string), and normalize
newlines to escaped `\n` sequences so the rendered
server/startup/dataMigrations/*.ts file always contains a valid string literal.
Comment on lines
+225
to
+231
| const lastVersion = (lockRecord && isSystemLockMigration(lockRecord) && lockRecord.extraData?.lastVersion) || undefined; | ||
|
|
||
| if (lastVersion === Info.version) { | ||
| log.startup('Data migrations already ran for this version. Skipping.'); | ||
| await releaseLock(); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Same-version short-circuit prevents retrying failed/pending migrations.
After a non-fatal failure, finally writes lastVersion, and the next startup on the same version exits early at Line 227-231, so failed migrations are not retried despite the warning text.
Suggested guard refinement
const lastVersion = (lockRecord && isSystemLockMigration(lockRecord) && lockRecord.extraData?.lastVersion) || undefined;
+ const hasFailedRecords = records.some((r) => r.status === 'failed');
+ const hasUnseenRegisteredMigrations = ordered.some((m) => !recordMap.has(getRecordId(m)));
- if (lastVersion === Info.version) {
+ if (lastVersion === Info.version && !hasFailedRecords && !hasUnseenRegisteredMigrations) {
log.startup('Data migrations already ran for this version. Skipping.');
await releaseLock();
return;
}Also applies to: 313-316, 321-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/server/lib/dataMigrations.ts` around lines 225 - 231, The current
short-circuit uses lastVersion alone and prevents retrying failed/pending
migrations; modify the logic so we only skip when the previous run for
Info.version completed successfully. Concretely: stop writing
extraData.lastVersion in the finally path on failure (only set it on successful
completion), and change the guard that reads lockRecord/extraData (the check
using lastVersion and isSystemLockMigration at the top of the snippet and the
analogous checks around lines 313–321) to require a success/completed flag (e.g.
extraData.migrationStatus === 'completed' or similar) in addition to lastVersion
=== Info.version before returning and calling releaseLock; ensure releaseLock
behavior remains correct when allowing retries after failures.
KevLehman
reviewed
Mar 16, 2026
Co-authored-by: Kevin Aleman <kaleman960@gmail.com>
sampaiodiego
requested changes
Mar 19, 2026
Member
sampaiodiego
left a comment
sampaiodiego
left a comment
There was a problem hiding this comment.
queries and updates should be moved to their respective model classes
| export class DataMigrationsRaw extends BaseRaw<IDataMigrationRecord> implements IDataMigrationsModel { | ||
| constructor(db: Db) { | ||
| super(db, 'data_migrations', undefined, { | ||
| collectionNameResolver(name) { |
Member
There was a problem hiding this comment.
collectionNameResolver should be used only if we don't want to use the prefix rocketchat_ on collection names, is this the case here?
Member
Author
There was a problem hiding this comment.
Current migration collection doesn't have the rocketchat_ prefix so I followed that. Do you think we should add the prefix @sampaiodiego ?
| } | ||
|
|
||
| async function checkManualReversions(maxRegisteredOrder: number, releaseLock: LockReleaseFunction): Promise<void> { | ||
| const requireManualReversion = await DataMigrations.find({ |
Member
There was a problem hiding this comment.
queries should be written on the model class
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
4 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.
https://rocketchat.atlassian.net/browse/CORE-1850
This pull request introduces a new framework for managing "data migrations" in the Meteor app, allowing for versioned, ordered, and safely executed changes to persistent data. It adds a dedicated system for registering, running, and tracking data migrations, including locking and manual reversion handling, and integrates this system with the app's startup process. The implementation includes new database models, migration templates, and scripts for generating migration files.
Data Migration Framework Implementation
dataMigrationssystem, including theaddDataMigrationregistration function andrunDataMigrationsexecution logic, with support for migration ordering, manual reversion, locking, and error handling (server/lib/dataMigrations.ts,core-typings/src/migrations/IDataMigrationRecord.ts,core-typings/src/ISystemLock.ts). [1] [2] [3]DataMigrationsRaw,SystemLocksRaw, and their typings and proxies (packages/models/src/models/DataMigrations.ts,packages/models/src/models/SystemLocks.ts,model-typings/src/models/IDataMigrationsModel.ts,model-typings/src/models/ISystemLocksModel.ts,models/src/index.ts,models/src/modelClasses.ts). [1] [2] [3] [4] [5] [6] [7]Developer Tooling and Integration
make-data-migration.ts) and template for generating new migration files, with automatic ordering and index updating; exposed as an npm command (package.json,.scripts/data-migration.template,.scripts/make-data-migration.ts). [1] [2] [3]server/startup/migrations/xrun.ts). [1] [2]server/models.ts). [1] [2]Documentation and Guidance
server/startup/dataMigrations/index.ts).These changes establish a robust, maintainable, and safe process for evolving the app's data schema and contents, supporting both upgrades and downgrades with clear tracking and error handling.
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit