feat: add database migration system for schema changes#423
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a transactional Firestore migration runner with CLI forward/rollback paths and dynamic imports, implements a migration to backfill/remove ChangesUnresolved merge conflict (BLOCKING)
Firestore migration infrastructure and first migration
Sequence DiagramsequenceDiagram
participant CLI as migrate.ts
participant Firestore
participant MigrationModule
CLI->>Firestore: getAppliedMigrations()
Firestore-->>CLI: applied[]
CLI->>CLI: list and sort migration files
CLI->>Firestore: claimMigration(name) [transaction]
Firestore-->>CLI: claimed=true
CLI->>MigrationModule: import migration module
CLI->>MigrationModule: migration.up(db) / migration.down(db)
MigrationModule->>Firestore: read/write batches
MigrationModule-->>CLI: complete
CLI->>Firestore: unclaimMigration(name) [transaction] (on rollback or failure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@roshankumar0036singh All 5 checks passing... Ready to merge |
|
@rahul616sama package.json has an unresolved merge conflict here |
|
flip the sequence here await to |
|
On migrate.ts — missing down() |
|
@rahul616sama On 001_add_feedbackRequestSent_to_events.ts — intorudce batching as it is slow for writing oen at ta time |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
2-45:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winResolve merge markers;
package.jsonis currently invalid JSON and blocks install/CI.
<<<<<<<,=======,>>>>>>>markers (Line 2, Line 15, Line 45) make this file unparsable. This will break root install/lockfile update and any tooling depending on root dependencies.Proposed reconciliation (remove conflict markers, keep merged content)
{ -<<<<<<< HEAD - "scripts": { - "build": "tsc -p cloud-functions/tsconfig.json", - "test:rules": "firebase emulators:exec --only firestore \"jest tests/firestore.rules.test.ts --runInBand\"" - }, - "devDependencies": { - "`@firebase/rules-unit-testing`": "^5.0.1", - "`@types/jest`": "^30.0.0", - "`@types/node`": "^25.8.0", - "firebase-tools": "^15.18.0", - "jest": "^30.4.2", - "ts-jest": "^29.4.11" - } -======= "scripts": { "build": "tsc -p cloud-functions/tsconfig.json", "test:rules": "firebase emulators:exec --only firestore \"jest tests/firestore.rules.test.ts --runInBand\"", "prepare": "husky" @@ "lint-staged": { @@ } ->>>>>>> upstream/main }🤖 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 `@package.json` around lines 2 - 45, The package.json contains unresolved merge markers (<<<<<<<, =======, >>>>>>>) making it invalid; remove those markers and reconcile the contents by keeping the merged block that includes the complete "scripts" (build, test:rules, prepare), full "devDependencies" (including eslint, husky, lint-staged, prettier), the "dependencies" section (expo-clipboard), and the "lint-staged" configuration—ensuring keys "scripts", "devDependencies", "dependencies", and "lint-staged" are present exactly once and the file is valid JSON.
🤖 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 `@cloud-functions/migrations/001_add_feedbackRequestSent_to_events.ts`:
- Around line 13-25: The migration currently updates documents one-by-one using
eventDoc.ref.update inside the for-loop (iterating eventsSnap.docs) which is
slow and risky; change it to use Firestore BulkWriter (or WriteBatch in
500-write chunks) to perform batched writes: create a BulkWriter instance (or
accumulate WriteBatch objects up to 500 writes), call update on eventDoc.ref for
documents missing feedbackRequestSent, and ensure you await writer.close() (or
commit all batches) at the end; keep the same predicate
(data.feedbackRequestSent === undefined) and preserve the console log behavior
for updated docs (use writer callbacks or log after successful commits).
In `@cloud-functions/migrations/migrate.ts`:
- Around line 56-57: The migration name is being added twice because
applied.push(file) runs locally and markAsDone(file, applied) also appends
migrationName before saving; remove the local duplicate by deleting the
applied.push(file) call (or alternatively change markAsDone to accept the
already-updated applied array and not append internally). Locate the call to
applied.push(file) and either remove it so markAsDone(file, applied) is the sole
appender, or update markAsDone(file, applied) to set the applied array instead
of pushing, ensuring only one append occurs.
- Around line 42-57: The migration runner currently does a non-atomic
check/update (applied includes -> migration.up -> applied.push -> markAsDone) so
concurrent processes can both run the same migration or re-apply side effects if
markAsDone fails; replace that flow for each file by implementing a
per-migration Firestore claim using a transaction: attempt to atomically
create/claim a migration doc (e.g., set a "claimedBy"/"claimedAt" field) inside
a transaction and abort if the doc already indicates claimed/completed, then run
migration.up(db) only after the claim succeeds, and finally update the migration
doc in a second transaction (or within the same transaction if possible) to mark
it completed (set "done"/"appliedAt" and remove claim), ensuring you do not use
DocumentReference.create() and instead use
transaction.get/transaction.set/transaction.update to guarantee atomicity and
avoid the known firebase-admin create() retry/hang issue; ensure failures during
mark-as-done do not leave side effects untracked by using the claim/complete
state to retry or compensate.
---
Outside diff comments:
In `@package.json`:
- Around line 2-45: The package.json contains unresolved merge markers (<<<<<<<,
=======, >>>>>>>) making it invalid; remove those markers and reconcile the
contents by keeping the merged block that includes the complete "scripts"
(build, test:rules, prepare), full "devDependencies" (including eslint, husky,
lint-staged, prettier), the "dependencies" section (expo-clipboard), and the
"lint-staged" configuration—ensuring keys "scripts", "devDependencies",
"dependencies", and "lint-staged" are present exactly once and the file is valid
JSON.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e0e0ccd-9edc-4057-b463-c3f347f42b85
📒 Files selected for processing (4)
cloud-functions/migrations/001_add_feedbackRequestSent_to_events.tscloud-functions/migrations/migrate.tscloud-functions/package.jsonpackage.json
|
Fixed package.json merge conflict Ready for review |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
2-46:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winResolve the Git merge conflict before merge.
The file contains unresolved Git merge conflict markers (
<<<<<<< HEAD,=======,>>>>>>> upstream/main) that make it invalid JSON. This blocks the PR from merging and will break any npm operations, including theupdate-lockfile.ymlworkflow that runsnpm install --package-lock-onlywhenpackage.jsonchanges.Resolution guidance:
- Manually edit this file to remove the conflict markers
- Choose which version of
scriptsanddevDependenciesto keep (likely theupstream/mainside, which includes the fuller tooling setup with husky, lint-staged, prettier, and eslint)- Ensure the final JSON is valid and includes the
@types/nodeversion update to^25.9.1✅ Recommended resolution
Accept the
upstream/mainside (remove HEAD section and conflict markers):{ -<<<<<<< HEAD - "scripts": { - "build": "tsc -p cloud-functions/tsconfig.json", - "test:rules": "firebase emulators:exec --only firestore \"jest tests/firestore.rules.test.ts --runInBand\"" - }, - "devDependencies": { - "`@firebase/rules-unit-testing`": "^5.0.1", - "`@types/jest`": "^30.0.0", - "`@types/node`": "^25.8.0", - "firebase-tools": "^15.18.0", - "jest": "^30.4.2", - "ts-jest": "^29.4.11" - } -======= "scripts": { "build": "tsc -p cloud-functions/tsconfig.json", "test:rules": "firebase emulators:exec --only firestore \"jest tests/firestore.rules.test.ts --runInBand\"", "prepare": "husky" }, "devDependencies": { "`@firebase/rules-unit-testing`": "^5.0.1", "`@types/jest`": "^30.0.0", "`@types/node`": "^25.9.1", "eslint": "^10.4.0", "firebase-tools": "^15.18.0", "husky": "^9.1.7", "jest": "^30.4.2", "lint-staged": "^17.0.5", "prettier": "^3.8.3", "ts-jest": "^29.4.11" }, "dependencies": { "expo-clipboard": "^56.0.3" }, "lint-staged": { "*.{js,jsx,ts,tsx}": [ "eslint --fix", "prettier --write" ], "*.{json,md,yml,yaml}": [ "prettier --write" ] } ->>>>>>> upstream/main }🤖 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 `@package.json` around lines 2 - 46, This file has unresolved Git conflict markers; remove the conflict block and produce a single valid JSON object merging the intended fields: keep the upstream/main side for "scripts" (including "prepare": "husky"), the fuller "devDependencies" (including eslint, husky, lint-staged, prettier and `@types/node` ^25.9.1), add the "dependencies" and "lint-staged" sections, and ensure "scripts" still contains "build" and "test:rules"; then validate the resulting package.json parses as JSON (no <<<<<<<, =======, >>>>>>> markers) and run a quick npm install to verify the lockfile workflow will succeed.
🧹 Nitpick comments (1)
cloud-functions/migrations/001_add_feedbackRequestSent_to_events.ts (1)
38-56: 💤 Low valueConsider:
down()removes the field from all events, not just those modified byup().The
up()function only addsfeedbackRequestSentto documents where it's undefined, butdown()unconditionally removes the field from all events. If any event had this field set before the migration (e.g., manually), the rollback would delete that data.For this specific new field this is likely fine. For future migrations with potentially existing data, consider tracking modified document IDs or using a more surgical rollback approach.
🤖 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 `@cloud-functions/migrations/001_add_feedbackRequestSent_to_events.ts` around lines 38 - 56, The down() migration unconditionally deletes feedbackRequestSent from all events; change it to only remove the field from the documents that were modified by up() by tracking which docs were changed (e.g., record modified document IDs when up() sets feedbackRequestSent or write a marker collection/document listing those IDs) and then have down() read that list and delete feedbackRequestSent only for those specific event docs (reference functions: up(), down(), field name feedbackRequestSent, and the events collection).
🤖 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 `@cloud-functions/migrations/migrate.ts`:
- Around line 64-74: The migration runner currently claims a migration via
claimMigration(file) then runs migration.up(db) but never releases the claim on
failure; wrap the import + await migration.up(db) call in a try/catch and if any
error is thrown call unclaimMigration(file) (or the appropriate function that
removes the file from the applied/claimed list), log or rethrow the error so the
process exits/fails as before, and ensure the claim is only kept when
migration.up completes successfully (references: claimMigration, migration.up,
and unclaimMigration).
---
Outside diff comments:
In `@package.json`:
- Around line 2-46: This file has unresolved Git conflict markers; remove the
conflict block and produce a single valid JSON object merging the intended
fields: keep the upstream/main side for "scripts" (including "prepare":
"husky"), the fuller "devDependencies" (including eslint, husky, lint-staged,
prettier and `@types/node` ^25.9.1), add the "dependencies" and "lint-staged"
sections, and ensure "scripts" still contains "build" and "test:rules"; then
validate the resulting package.json parses as JSON (no <<<<<<<, =======, >>>>>>>
markers) and run a quick npm install to verify the lockfile workflow will
succeed.
---
Nitpick comments:
In `@cloud-functions/migrations/001_add_feedbackRequestSent_to_events.ts`:
- Around line 38-56: The down() migration unconditionally deletes
feedbackRequestSent from all events; change it to only remove the field from the
documents that were modified by up() by tracking which docs were changed (e.g.,
record modified document IDs when up() sets feedbackRequestSent or write a
marker collection/document listing those IDs) and then have down() read that
list and delete feedbackRequestSent only for those specific event docs
(reference functions: up(), down(), field name feedbackRequestSent, and the
events collection).
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ad9715d-e7ed-40ba-81f9-981e7e9ff4da
📒 Files selected for processing (3)
cloud-functions/migrations/001_add_feedbackRequestSent_to_events.tscloud-functions/migrations/migrate.tspackage.json
|
|
@roshankumar0036singh ready for review |
roshankumar0036singh
merged commit 4577e6f
into
roshankumar0036singh:main
2 participants
Closes #339
What changed
cloud-functions/migrations/directorycloud-functions/migrations/migrate.ts— a migration runner that finds and runs all pending migrations in ordercloud-functions/migrations/001_add_feedbackRequestSent_to_events.ts— sample migration that addsfeedbackRequestSent: falseto events missing the fieldnpm run migratescript tocloud-functions/package.jsonHow it works
npm run migratefrom thecloud-functions/folder.tsfiles in themigrations/folder sorted by namemigrations/applied) to see which ones already ranSummary by CodeRabbit
Bug Fixes
New Features