This repository was archived by the owner on Jun 9, 2026. It is now read-only.
Repository navigation
change migration endpoint to CLI - #103
Merged
Merged
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR replaces the HTTP migration endpoint with a CLI-driven migration system: adds a Nest-powered CLI, migration contracts, contexts, logging/output helpers, a tracked/dry-run migration runtime, idempotency test harness, migrations registry, and removes the MigrationController and ConfigMigrationService. Credentials are loaded from disk for org targeting. ChangesCLI-driven migration system
Sequence DiagramsequenceDiagram
actor User
participant CLI as ndb-admin CLI
participant NestApp as NestJS App
participant CredSvc as Credentials Service
participant CouchDB as CouchDB (Org 1..N)
User->>CLI: npm run cli migrate run latest-config-formats --org staging
CLI->>NestApp: bootstrap AppModule (with services)
NestApp->>CredSvc: load credentials.json
CredSvc-->>NestApp: SystemCredentials[]
CLI->>CLI: resolve target orgs (filter by --org)
CLI->>CouchDB: check connectivity (GET /app)
CouchDB-->>CLI: reachability per org
CLI->>User: display connectivity status
loop For each reachable org
CLI->>CouchDB: DRY-RUN: execute migration (reads + validate, no writes)
CouchDB-->>CLI: migration result & write-stats
CLI->>User: print preview (dry-run)
end
CLI->>User: prompt for confirmation (--yes to skip)
User-->>CLI: confirm
loop For each reachable org
CLI->>CouchDB: REAL RUN: execute migration (reads + validate + writes)
CouchDB-->>CLI: migration result & write-stats
CLI->>User: print final outcomes
end
CLI->>User: print summary and exit with computed code
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes 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 |
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (9)
src/migration/tracked-migration-context.ts (1)
25-31: ⚡ Quick winUse
unknowninstead ofanyfor caught exceptions.Line 28 uses
e: anyin the catch block. Per coding guidelines, preferunknownoverany. Additionally, safely access the error message:- } catch (e: any) { - throw new Error(`JSON validation failed: ${e.message}`); + } catch (e: unknown) { + const message = e instanceof Error ? e.message : String(e); + throw new Error(`JSON validation failed: ${message}`); }As per coding guidelines, "Prefer type inference when obvious, avoid
anytype - useunknownwhen type is uncertain" and "Leverage TypeScript's type system, avoidanywhere possible".🤖 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 `@src/migration/tracked-migration-context.ts` around lines 25 - 31, In validateJson, change the catch clause to type the caught error as unknown (catch (e: unknown)) and safely extract a message for the thrown Error (e.g., check if e is an object with a string message or fallback to String(e) or a default) so you avoid using any while still producing a useful error text when throwing new Error(`JSON validation failed: ...`); update validateJson to perform the appropriate type guard/formatting for e before accessing message.src/migration/testing/migration-idempotency.harness.spec.ts (1)
47-50: 💤 Low valueClarify why 'failed' status is acceptable for idempotency.
The idempotency assertion allows both
'no-change'and'failed'statuses to pass. If a migration fails consistently on every run, it technically satisfies the "no further changes" criterion, but this may mask real issues. Consider whether migrations that fail should be treated separately or require explicit handling.🤖 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 `@src/migration/testing/migration-idempotency.harness.spec.ts` around lines 47 - 50, The test currently asserts that result.secondRunResult.status can be either 'no-change' or 'failed', which masks real failures; change the assertion in migration-idempotency.harness.spec.ts to require 'no-change' only, and if you want to allow consistent failures add an explicit separate check: verify that result.secondRunResult.status === 'failed' only when a new flag (e.g., allowConsistentFailure) is set, and in that case also assert the failure is identical to the first run by comparing result.secondRunResult.error (or result.firstRunResult.error) to ensure the failure is consistent rather than transient.src/migration/config-migration/latest-config-formats.migration.ts (2)
50-53: 💤 Low valueConsider adding verbose logging for actual writes.
Line 52 logs a verbose message only during dry-run, but there's no equivalent verbose log when the migration actually writes. For consistency and observability, consider logging document changes in both modes.
📋 Suggested logging enhancement
ctx.log.info(' Config requires migration'); if (ctx.dryRun) { ctx.log.verbose(' ~ Config:CONFIG_ENTITY (migration would apply)'); + } else { + ctx.log.verbose(' ✓ Config:CONFIG_ENTITY (applying migration)'); }🤖 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 `@src/migration/config-migration/latest-config-formats.migration.ts` around lines 50 - 53, The migration currently emits a verbose message only in dry-run mode (using ctx.log.verbose when ctx.dryRun) but omits equivalent verbose logging when it actually writes; update the migration so that after applying the write you log the same detailed information in non-dryRun mode (use the same ctx.log.verbose call pattern as the dry-run branch and include identifying context like 'CONFIG_ENTITY' and the document/change summary), ensuring the write path (the block that performs the migration/commit) calls ctx.log.verbose with the same message so both dry-run and real runs provide consistent observability.
20-20: ⚡ Quick winReplace
anywithunknownfor better type safety.Lines 20 and 31 use
anytype for config documents. TypeScript guidelines recommend usingunknownwhen the type is uncertain, then narrowing with type guards or assertions as needed.🔒 Proposed type safety improvement
async run(ctx): Promise<MigrationResult> { - let config: any; + let config: unknown; try { config = await ctx.couchdb.get(CONFIG_DOC_PATH); } catch {- let newConfig: any = JSON.parse(JSON.stringify(config), (key, value) => { + let newConfig: unknown = JSON.parse(JSON.stringify(config), (key, value) => { for (const migration of ndbCoreConfigMigrations) {As per coding guidelines, "Prefer type inference when obvious, avoid
anytype - useunknownwhen type is uncertain".🤖 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 `@src/migration/config-migration/latest-config-formats.migration.ts` at line 20, Replace the loose any type on the variable named config with unknown (change "let config: any" to "let config: unknown") and then narrow or assert its shape immediately before any use — e.g., use type guards, typeof checks, or explicit casts where the code reads properties from config (ensure the checks occur before places that previously assumed config structure, including the other occurrence on line 31). Update any downstream code that indexed into config to first validate the expected keys/types or perform a safe cast so the compiler no longer requires any.src/cli.ts (2)
104-109: ⚡ Quick winUse
unknowninstead ofanyfor caught errors.Line 104 uses
anytype for the caught error. TypeScript guidelines recommendunknownfor uncertain types.🔒 Proposed type safety improvement
try { const result = await migration.run(ctx); return { result, writeStats: ctx.getWriteStats() }; - } catch (e: any) { + } catch (e: unknown) { return { - result: failedMigrationResult(e.message), + result: failedMigrationResult(e instanceof Error ? e.message : String(e)), writeStats: ctx.getWriteStats(), }; }As per coding guidelines, "avoid
anytype - useunknownwhen type is uncertain".🤖 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 `@src/cli.ts` around lines 104 - 109, Change the catch parameter from `any` to `unknown` in the migration error handling block and safely extract a message before calling `failedMigrationResult`; e.g., inside the catch of the code that currently returns `{ result: failedMigrationResult(e.message), writeStats: ctx.getWriteStats() }`, narrow `e` (e.g. `e instanceof Error ? e.message : String(e)`) or otherwise convert it to a string, then pass that safe message into `failedMigrationResult` while leaving `ctx.getWriteStats()` unchanged.
158-175: 💤 Low valueConsider the impact of disabling NestJS logging.
Line 162 sets
logger: false, which suppresses all NestJS bootstrap and service logs. While this keeps CLI output clean, it may make debugging initialization issues difficult. Consider whether warning/error level logs should remain enabled.🤖 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 `@src/cli.ts` around lines 158 - 175, The CLI currently disables all NestJS logs in withApp by passing logger: false to NestFactory.createApplicationContext(AppModule), which hides bootstrap and runtime errors; change the logger option to preserve at least warning/error output (e.g., set logger to ['error','warn'] or a LoggerOptions object) so initialization and service errors from AppModule, CredentialsService, and CouchdbService are still emitted while keeping verbose info/debug suppressed.src/migration/testing/migration-idempotency.harness.ts (3)
163-172: 💤 Low valueConsider using structuredClone for deep copying.
The harness uses
JSON.parse(JSON.stringify(...))for deep cloning, which has limitations (functions, undefined, symbols, Dates are not properly handled). While adequate for typical migration data, consider usingstructuredClone()for more robust copying if available in your Node.js version (v17+).♻️ Alternative using structuredClone
- const store1: DocStore = JSON.parse(JSON.stringify(initialDocs)); + const store1: DocStore = structuredClone(initialDocs); const ctx1 = buildInMemoryContext(store1, false); const firstRunResult = await migration.run(ctx1); - const stateAfterFirstRun: DocStore = JSON.parse(JSON.stringify(store1)); + const stateAfterFirstRun: DocStore = structuredClone(store1); // Second run — same store state as left after first run - const store2: DocStore = JSON.parse(JSON.stringify(stateAfterFirstRun)); + const store2: DocStore = structuredClone(stateAfterFirstRun); const ctx2 = buildInMemoryContext(store2, false); const secondRunResult = await migration.run(ctx2); - const stateAfterSecondRun: DocStore = JSON.parse(JSON.stringify(store2)); + const stateAfterSecondRun: DocStore = structuredClone(store2);🤖 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 `@src/migration/testing/migration-idempotency.harness.ts` around lines 163 - 172, Replace brittle JSON.parse(JSON.stringify(...)) deep-clones with structuredClone where possible: call structuredClone(initialDocs) for store1 and structuredClone(stateAfterFirstRun) for store2 (and similarly for stateAfterFirstRun/stateAfterSecondRun captures) before passing to buildInMemoryContext and migration.run; if structuredClone is not available at runtime, fall back to a safe alternative (e.g., lodash.cloneDeep or a small helper) so cloning correctly preserves Dates, undefined, symbols, and functions.
43-97: 💤 Low valueStub CouchDB doesn't handle
_revfields.The stub doesn't validate or update
_revfields that are typically required in CouchDB operations for conflict detection. While this simplifies the test harness, be aware that migrations relying on revision handling won't be properly tested by this harness.🤖 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 `@src/migration/testing/migration-idempotency.harness.ts` around lines 43 - 97, The stub CouchDB in buildStubCouchdb must simulate _rev handling: update put, putAll and post (the jest.fn implementations) to validate incoming _rev when replacing an existing document (throw an error with response.status 409 on mismatch), generate or increment a _rev (e.g., "1-..." -> "2-...") when creating/updating, and store the updated _rev in the stored document; also ensure get and getAll return the document including its _rev so tests relying on revision-based conflict detection behave like CouchDB.
96-96: ⚡ Quick winReplace double type cast with a proper stub interface.
The
as unknown as Couchdbdouble cast bypasses TypeScript's type system. While the stub currently implements all methods needed by migrations (get,put,putAll,getAll,post,find), the double cast will silently mask any future missing methods. This violates the principle of leveraging TypeScript's type system.Consider defining a
CouchdbStubinterface with only the methods the harness implements, then cast to that instead:interface CouchdbStub { get: (...args: any[]) => Promise<any>; put: (...args: any[]) => Promise<any>; putAll: (...args: any[]) => Promise<any>; getAll: (...args: any[]) => Promise<any>; post: (...args: any[]) => Promise<any>; find: (...args: any[]) => Promise<any>; } return { /* stub object */ } as CouchdbStub;This makes type expectations explicit and allows TypeScript to catch inconsistencies.
🤖 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 `@src/migration/testing/migration-idempotency.harness.ts` at line 96, Replace the double cast "as unknown as Couchdb" with a dedicated stub interface and cast to that: define an interface (e.g., CouchdbStub) declaring the exact methods your harness returns (get, put, putAll, getAll, post, find each returning Promise<any>), then return the stub object as CouchdbStub instead of using the unknown double cast so TypeScript will enforce the stub implements those methods (locate the return that currently uses "as unknown as Couchdb" and update the cast and add the new interface).
🤖 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 `@src/cli.ts`:
- Around line 179-195: The helper resolveOrgs currently calls process.exit(2);
change it to signal failure instead of exiting: have resolveOrgs (which uses
CredentialsService and OrgRunner.filterOrgs) throw a specific, identifiable
error (e.g., an Error instance with a distinctive message or a small ExitError
type containing code=2) or return a sentinel (null) when no orgs match; remove
the process.exit call. Update the command action callers (the functions wrapped
by withApp) to catch that error/sentinel and map it to process exit code 2 (or
return 2) so cleanup in withApp can run and exit behavior is centralized.
In `@src/migration/config-migration/ndb-core-config-migrations.ts`:
- Around line 179-181: The type guard using typeof configPart === 'object'
incorrectly treats null as an object; update the check in the migration logic
that inspects defaultValue/configPart so it only returns when configPart is a
non-null object (i.e., add configPart !== null) — locate the branch around the
if (typeof configPart === 'object') return configPart; and change it to
explicitly exclude null so null defaultValue gets wrapped into the migration
format (or add a clarifying comment if retaining current behavior is
intentional).
In `@src/migration/migrations.ts`:
- Line 4: The CONFIG_DOC_PATH constant currently contains unencoded special
characters which will be placed directly into CouchDB HTTP URLs; update the
constant CONFIG_DOC_PATH to use a percent-encoded document path (e.g., change
'/app/Config:CONFIG_ENTITY' to '/app/Config%3ACONFIG_ENTITY') or, alternatively,
replace the document id with a safe form without special characters (e.g.,
'/app/Config_CONFIG_ENTITY' or '/app/Config-CONFIG_ENTITY') and update any code
that references CONFIG_DOC_PATH accordingly so URLs sent to CouchDB are valid.
In `@src/migration/README.md`:
- Around line 163-175: Update the README.md fenced directory diagram to include
a language identifier so the linter stops flagging MD040: change the opening
fence from ``` to ```text (i.e., in the README.md code block that starts with
``` and shows the src/migration/ tree) so the block becomes ```text and the rest
of the diagram remains unchanged.
In `@src/migration/testing/migration-idempotency.harness.spec.ts`:
- Around line 19-36: The seed key in seedForMigration does not match the
normalized path produced when CONFIG_DOC_PATH is passed through key(), causing
lookups to miss; update the seed key in seedForMigration from
'app/Config:CONFIG_ENTITY' to '/app/Config:CONFIG_ENTITY' (or alternatively make
CONFIG_DOC_PATH match the comment by removing the leading slash) so the
stub.get() lookup and key() normalization produce the same document key; check
seedForMigration, CONFIG_DOC_PATH, and the key() normalization logic to ensure
both use the same leading-slash convention.
In `@src/migration/testing/migration-idempotency.harness.ts`:
- Line 128: Replace the catch parameter type from any to unknown in the catch
clause (the caught variable named e) and then narrow its type before use; for
example, inside the catch of the idempotency test harness
(migration-idempotency.harness.ts) check whether e is an instance of Error to
access message/stack, otherwise convert/serialize e (e.g., String(e) or
JSON.stringify where safe) before logging or rethrowing, ensuring all usages of
e are type-safe after narrowing.
- Line 53: The variable `err` is currently declared with the `any` type (`const
err: any = new Error(\`Not found: ${k}\`)`); change it to a proper error type
(e.g., `Error`) or `unknown` with subsequent type narrowing so you do not use
`any`. Update the declaration of `err` in the same scope where `k` is available
(the `err` binding) to `const err: Error = new Error(\`Not found: ${k}\`)` or
`const err: unknown = new Error(...)` and add a type guard before using error
properties so TypeScript knows its shape. Ensure any downstream handling
(logging, throw) uses the narrowed/typed `err` instead of `any`.
- Line 67: The fallback for document id uses Math.random() which makes tests
non-deterministic; replace that by introducing a deterministic scoped counter
(e.g., a module/function-scoped variable like deterministicIdCounter) and use it
when computing the id instead of Math.random(), i.e. change the expression that
sets the id (the line with const id: string = doc._id ?? doc.id ??
String(Math.random());) to use the counter fallback (incrementing the counter
each time) or alternatively fail fast by throwing if neither doc._id nor doc.id
is present to enforce tests supply IDs.
---
Nitpick comments:
In `@src/cli.ts`:
- Around line 104-109: Change the catch parameter from `any` to `unknown` in the
migration error handling block and safely extract a message before calling
`failedMigrationResult`; e.g., inside the catch of the code that currently
returns `{ result: failedMigrationResult(e.message), writeStats:
ctx.getWriteStats() }`, narrow `e` (e.g. `e instanceof Error ? e.message :
String(e)`) or otherwise convert it to a string, then pass that safe message
into `failedMigrationResult` while leaving `ctx.getWriteStats()` unchanged.
- Around line 158-175: The CLI currently disables all NestJS logs in withApp by
passing logger: false to NestFactory.createApplicationContext(AppModule), which
hides bootstrap and runtime errors; change the logger option to preserve at
least warning/error output (e.g., set logger to ['error','warn'] or a
LoggerOptions object) so initialization and service errors from AppModule,
CredentialsService, and CouchdbService are still emitted while keeping verbose
info/debug suppressed.
In `@src/migration/config-migration/latest-config-formats.migration.ts`:
- Around line 50-53: The migration currently emits a verbose message only in
dry-run mode (using ctx.log.verbose when ctx.dryRun) but omits equivalent
verbose logging when it actually writes; update the migration so that after
applying the write you log the same detailed information in non-dryRun mode (use
the same ctx.log.verbose call pattern as the dry-run branch and include
identifying context like 'CONFIG_ENTITY' and the document/change summary),
ensuring the write path (the block that performs the migration/commit) calls
ctx.log.verbose with the same message so both dry-run and real runs provide
consistent observability.
- Line 20: Replace the loose any type on the variable named config with unknown
(change "let config: any" to "let config: unknown") and then narrow or assert
its shape immediately before any use — e.g., use type guards, typeof checks, or
explicit casts where the code reads properties from config (ensure the checks
occur before places that previously assumed config structure, including the
other occurrence on line 31). Update any downstream code that indexed into
config to first validate the expected keys/types or perform a safe cast so the
compiler no longer requires any.
In `@src/migration/testing/migration-idempotency.harness.spec.ts`:
- Around line 47-50: The test currently asserts that
result.secondRunResult.status can be either 'no-change' or 'failed', which masks
real failures; change the assertion in migration-idempotency.harness.spec.ts to
require 'no-change' only, and if you want to allow consistent failures add an
explicit separate check: verify that result.secondRunResult.status === 'failed'
only when a new flag (e.g., allowConsistentFailure) is set, and in that case
also assert the failure is identical to the first run by comparing
result.secondRunResult.error (or result.firstRunResult.error) to ensure the
failure is consistent rather than transient.
In `@src/migration/testing/migration-idempotency.harness.ts`:
- Around line 163-172: Replace brittle JSON.parse(JSON.stringify(...))
deep-clones with structuredClone where possible: call
structuredClone(initialDocs) for store1 and structuredClone(stateAfterFirstRun)
for store2 (and similarly for stateAfterFirstRun/stateAfterSecondRun captures)
before passing to buildInMemoryContext and migration.run; if structuredClone is
not available at runtime, fall back to a safe alternative (e.g.,
lodash.cloneDeep or a small helper) so cloning correctly preserves Dates,
undefined, symbols, and functions.
- Around line 43-97: The stub CouchDB in buildStubCouchdb must simulate _rev
handling: update put, putAll and post (the jest.fn implementations) to validate
incoming _rev when replacing an existing document (throw an error with
response.status 409 on mismatch), generate or increment a _rev (e.g., "1-..." ->
"2-...") when creating/updating, and store the updated _rev in the stored
document; also ensure get and getAll return the document including its _rev so
tests relying on revision-based conflict detection behave like CouchDB.
- Line 96: Replace the double cast "as unknown as Couchdb" with a dedicated stub
interface and cast to that: define an interface (e.g., CouchdbStub) declaring
the exact methods your harness returns (get, put, putAll, getAll, post, find
each returning Promise<any>), then return the stub object as CouchdbStub instead
of using the unknown double cast so TypeScript will enforce the stub implements
those methods (locate the return that currently uses "as unknown as Couchdb" and
update the cast and add the new interface).
In `@src/migration/tracked-migration-context.ts`:
- Around line 25-31: In validateJson, change the catch clause to type the caught
error as unknown (catch (e: unknown)) and safely extract a message for the
thrown Error (e.g., check if e is an object with a string message or fallback to
String(e) or a default) so you avoid using any while still producing a useful
error text when throwing new Error(`JSON validation failed: ...`); update
validateJson to perform the appropriate type guard/formatting for e before
accessing message.
🪄 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: 3e8ea3fd-00a6-40ce-aae5-1512b393c73b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
README.mdpackage.jsonsrc/app.module.tssrc/cli.tssrc/cli/org-output.tssrc/cli/org-runner.tssrc/credentials/credentials.service.tssrc/migration/README.mdsrc/migration/config-migration/config-migration.service.spec.tssrc/migration/config-migration/latest-config-formats.migration.tssrc/migration/config-migration/migrate-add-entity-attributes.tssrc/migration/config-migration/ndb-core-config-migrations.tssrc/migration/console-logger.tssrc/migration/migration-definition.tssrc/migration/migration-output.tssrc/migration/migration.controller.spec.tssrc/migration/migration.controller.tssrc/migration/migrations.tssrc/migration/testing/migration-idempotency.harness.spec.tssrc/migration/testing/migration-idempotency.harness.tssrc/migration/tracked-migration-context.ts
💤 Files with no reviewable changes (4)
- src/migration/config-migration/config-migration.service.spec.ts
- src/migration/migration.controller.spec.ts
- src/migration/config-migration/migrate-add-entity-attributes.ts
- src/migration/migration.controller.ts
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/migration/tracked-migration-context.ts (1)
67-106: ⚡ Quick winType the error in the catch block.
Line 74 uses an empty
catchblock without a type annotation, which implicitly types the error asany. While the error is not used in this case, it's better to explicitly type it asunknownfor consistency.🔧 Proposed fix
let oldData: unknown; try { oldData = await this.couchdb.get(path, db); - } catch { + } catch (_e: unknown) { this.log.verbose(`+ ${path} (new document)`); return; }As per coding guidelines: "Use
unknownwhen type is uncertain".🤖 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 `@src/migration/tracked-migration-context.ts` around lines 67 - 106, The catch block in method logDiff should explicitly type the caught error as unknown instead of leaving it implicit; update the catch clause in tracked-migration-context.ts inside the logDiff method (the try { oldData = await this.couchdb.get(path, db); } catch { ... }) to use catch (err: unknown) and keep the existing logic (logging the new-document message and returning) unchanged so the behavior remains the same while satisfying the typing guideline.
🤖 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 `@src/credentials/credentials.service.ts`:
- Around line 22-34: Wrap the file read and JSON.parse in a try-catch inside the
credentials loader (the block that calls this.resolveCredentialsPath(),
readFileSync(...) and JSON.parse(...)) to catch I/O and syntax errors, log or
throw a descriptive error that includes the credentialsPath and original error
message, and avoid crashing silently; ensure you rethrow or throw a controlled
Error so callers can handle initialization failures. Also make property access
consistent by replacing c['name'] with dot notation (c.name) when constructing
the returned object, and keep use of this.DEFAULT_DOMAIN in the URL fallback
logic unchanged.
In `@src/migration/config-migration/latest-config-formats.migration.ts`:
- Around line 21-32: Replace the use of any for the migration variables by
declaring config and newConfig as unknown: change the type of config (the result
of ctx.couchdb.get(CONFIG_DOC_PATH)) to unknown and similarly newConfig to
unknown before doing the JSON parse/transform; after you call
ctx.validateJson(newConfig) narrow or cast newConfig to the expected validated
type (or perform an explicit type guard) so subsequent code uses a safe typed
value. Ensure references to config, newConfig, and the ctx.couchdb.get(...) call
are updated accordingly and avoid leaving any other any-typed variables in this
migration path.
- Around line 22-29: The catch currently treats all errors from
ctx.couchdb.get(CONFIG_DOC_PATH) as "Config document not found"; update the
error handling in latest-config-formats.migration.ts to detect the specific 404
Not Found case (check the caught error's status/statusCode as done in
statistics.service.ts) and only return the { changed:false, status:'failed',
warnings:['Config document not found'] } for 404; for any other error,
log/report it (so Sentry can capture the original error) and rethrow (or return
an appropriate failure that preserves the original error), referencing the
ctx.couchdb.get call and the surrounding try/catch where you implement the
conditional handling.
In `@src/migration/tracked-migration-context.ts`:
- Around line 43-65: The catch block in the async method put currently uses an
untyped exception (catch (e)); change it to catch (e: unknown) to follow TS
guidelines. In the put method where you call this.couchdb.put(...) inside
try/catch, update the catch signature to catch (e: unknown) and keep the
existing error handling (this.failed++ and rethrow) or wrap/transform the error
before throwing if needed, ensuring TypeScript compiles without an implicit any
for the caught exception.
- Around line 27-33: In validateJson replace the catch clause using catch (e:
any) with catch (e: unknown) and narrow e before accessing message: check if e
is an instance of Error (or has a message string) and use that message,
otherwise coerce e to a string (e.g., String(e)) when constructing the thrown
Error; update the Error creation in validateJson to use the narrowed message so
no any is used.
---
Nitpick comments:
In `@src/migration/tracked-migration-context.ts`:
- Around line 67-106: The catch block in method logDiff should explicitly type
the caught error as unknown instead of leaving it implicit; update the catch
clause in tracked-migration-context.ts inside the logDiff method (the try {
oldData = await this.couchdb.get(path, db); } catch { ... }) to use catch (err:
unknown) and keep the existing logic (logging the new-document message and
returning) unchanged so the behavior remains the same while satisfying the
typing guideline.
🪄 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: 8b0e732a-5149-436b-ab0d-90487dd927c5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
.gitignoreREADME.mdpackage.jsonsrc/assets/credentials.jsonsrc/cli.tssrc/couchdb/couchdb.service.spec.tssrc/couchdb/couchdb.service.tssrc/credentials/credentials.service.tssrc/migration/README.mdsrc/migration/config-migration/latest-config-formats.migration.tssrc/migration/config-migration/ndb-core-config-migrations.tssrc/migration/console-logger.tssrc/migration/migration-output.tssrc/migration/testing/migration-idempotency.harness.tssrc/migration/tracked-migration-context.ts
💤 Files with no reviewable changes (1)
- src/assets/credentials.json
✅ Files skipped from review due to trivial changes (6)
- src/couchdb/couchdb.service.spec.ts
- src/couchdb/couchdb.service.ts
- .gitignore
- src/migration/console-logger.ts
- src/migration/README.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- src/migration/migration-output.ts
- src/migration/testing/migration-idempotency.harness.ts
- src/migration/config-migration/ndb-core-config-migrations.ts
- package.json
- src/cli.ts
sleidig
force-pushed
the
feat/migration-cli
branch
from
May 12, 2026 14:44
e9db069 to
7c99736
Compare
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Summary by CodeRabbit
New Features
Documentation
Chores
Tests