Keep migration progress and completion visible - #834
Conversation
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe migration now emits progress for every processed commit, coalesces rendering, flushes pending updates on completion or failure, and uses a shared durable report formatter for CLI output and success-state UI. ChangesMigration visibility
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
Release Preflight
If this PR is from a |
Release Preflight
If this PR is from a |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
scripts/v18-to-v19/V18MigrationApp.ts (1)
57-78: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
progress.flush()is unguarded where it can override the real migration outcome. In both files,flush()'s reporter performs external I/O (emit(...)orprocess.stderr.write(...)) with no error boundary, and it sits inside control flow that also determines/propagates success or failure — the shared fix is to isolate flush() failures from the reported outcome.
scripts/v18-to-v19/V18MigrationApp.ts#L57-L78: wrap bothprogress.flush()calls (Lines 67 and 71) in their own try/catch so a flush failure after a successfulrunV18ToV19Migrationcannot be caught and reported asstatus: 'failed'.scripts/v18-to-v19/migrate.ts#L79-L87: wrap thefinallyblock'sprogress.flush()(Line 86) in its own try/catch so a flush failure cannot replace/mask the real migration error thrown from thetry.🤖 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 `@scripts/v18-to-v19/V18MigrationApp.ts` around lines 57 - 78, Isolate progress.flush() failures from migration outcomes by wrapping both flush calls in V18MigrationApp.run (scripts/v18-to-v19/V18MigrationApp.ts, lines 57-78) in independent try/catch blocks, preserving the success or failure result from runV18ToV19Migration; also wrap the finally-block flush in scripts/v18-to-v19/migrate.ts, lines 79-87, so it cannot mask the original migration error.
🤖 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 `@scripts/v18-to-v19/migrate.ts`:
- Around line 79-87: Protect the progress.flush() call in the finally block
surrounding runV18ToV19Migration so a flush failure cannot replace the original
migration error. Catch and handle any flush exception without rethrowing it,
while preserving the existing migration result and error propagation behavior.
---
Outside diff comments:
In `@scripts/v18-to-v19/V18MigrationApp.ts`:
- Around line 57-78: Isolate progress.flush() failures from migration outcomes
by wrapping both flush calls in V18MigrationApp.run
(scripts/v18-to-v19/V18MigrationApp.ts, lines 57-78) in independent try/catch
blocks, preserving the success or failure result from runV18ToV19Migration; also
wrap the finally-block flush in scripts/v18-to-v19/migrate.ts, lines 79-87, so
it cannot mask the original migration error.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 907e821b-b7cc-4b38-b8b6-d015a5bdfe70
📒 Files selected for processing (13)
scripts/v18-to-v19/V18MigrationApp.tsscripts/v18-to-v19/V18MigrationAppSurface.tsscripts/v18-to-v19/V18MigrationPlan.tsscripts/v18-to-v19/V18MigrationProgress.tsscripts/v18-to-v19/V18MigrationProgressCoalescer.tsscripts/v18-to-v19/V18MigrationReport.tsscripts/v18-to-v19/V18WriterChainRewriter.tsscripts/v18-to-v19/migrate.tstest/unit/scripts/v18-migration-app.test.tstest/unit/scripts/v18-migration-cli.test.tstest/unit/scripts/v18-migration-progress.test.tstest/unit/scripts/v18-to-v19-scratch.test.tstest/unit/scripts/v18-to-v19-writer-chain.test.ts
💤 Files with no reviewable changes (1)
- scripts/v18-to-v19/V18MigrationProgress.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: preflight
- GitHub Check: test-bun
- GitHub Check: test-deno
- GitHub Check: coverage-threshold
- GitHub Check: test-node (22)
- GitHub Check: type-firewall-lint
- GitHub Check: type-firewall-generated-sdk
- GitHub Check: v19 base/head performance
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Do not use direct imports fromsrc/infrastructure/**insrc/domain/**orsrc/ports/**; depend on a port instead.
Do not use direct Node built-ins insrc/domain/**orsrc/ports/**; use a port instead.
Files:
scripts/v18-to-v19/V18MigrationAppSurface.tstest/unit/scripts/v18-migration-cli.test.tsscripts/v18-to-v19/V18MigrationReport.tstest/unit/scripts/v18-migration-app.test.tstest/unit/scripts/v18-migration-progress.test.tsscripts/v18-to-v19/V18MigrationPlan.tsscripts/v18-to-v19/V18MigrationApp.tstest/unit/scripts/v18-to-v19-writer-chain.test.tsscripts/v18-to-v19/migrate.tsscripts/v18-to-v19/V18WriterChainRewriter.tsscripts/v18-to-v19/V18MigrationProgressCoalescer.tstest/unit/scripts/v18-to-v19-scratch.test.ts
🪛 ast-grep (0.45.0)
test/unit/scripts/v18-migration-cli.test.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🔇 Additional comments (11)
scripts/v18-to-v19/V18MigrationProgressCoalescer.ts (1)
14-91: LGTM!test/unit/scripts/v18-migration-progress.test.ts (1)
1-90: LGTM!scripts/v18-to-v19/V18MigrationPlan.ts (1)
23-24: LGTM!Also applies to: 194-200
scripts/v18-to-v19/V18WriterChainRewriter.ts (1)
1-8: LGTM!Also applies to: 22-34, 52-58, 68-68, 77-83, 106-121, 131-131, 154-154
test/unit/scripts/v18-to-v19-scratch.test.ts (1)
18-18: LGTM!Also applies to: 160-196, 225-229
test/unit/scripts/v18-to-v19-writer-chain.test.ts (1)
16-24: LGTM!Also applies to: 51-74, 114-118
scripts/v18-to-v19/V18MigrationAppSurface.ts (1)
175-189: LGTM!scripts/v18-to-v19/migrate.ts (1)
19-20: LGTM!Also applies to: 33-33, 56-59, 88-98, 120-134, 136-184
scripts/v18-to-v19/V18MigrationReport.ts (1)
1-21: LGTM!test/unit/scripts/v18-migration-app.test.ts (1)
5-5: LGTM!Also applies to: 30-47, 194-220
test/unit/scripts/v18-migration-cli.test.ts (1)
5-8: LGTM!Also applies to: 60-94
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/v18-to-v19/V18MigrationProgressCoalescer.ts (1)
91-95: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winContain reporter failures from scheduled rendering.
At Line 94, a reporter exception escapes the timer callback, potentially terminating the process outside the migration’s error handling.
flushBestEffort()only protects terminal flushing.Proposed fix
this.#timer = null; if (this.#pending !== null) { - this.#deliverPending(); + try { + this.#deliverPending(); + } catch { + // Progress rendering must not replace the migration outcome. + } this.#scheduleWindow(); }🤖 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 `@scripts/v18-to-v19/V18MigrationProgressCoalescer.ts` around lines 91 - 95, Update the scheduled callback in V18MigrationProgressCoalescer’s timer flow to catch and contain exceptions from `#deliverPending`, routing failures through the migration’s existing error-handling mechanism instead of allowing them to escape the timer. Keep `#scheduleWindow` behavior intact and preserve flushBestEffort’s separate terminal-flush protection.
🤖 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.
Outside diff comments:
In `@scripts/v18-to-v19/V18MigrationProgressCoalescer.ts`:
- Around line 91-95: Update the scheduled callback in
V18MigrationProgressCoalescer’s timer flow to catch and contain exceptions from
`#deliverPending`, routing failures through the migration’s existing
error-handling mechanism instead of allowing them to escape the timer. Keep
`#scheduleWindow` behavior intact and preserve flushBestEffort’s separate
terminal-flush protection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e8bd3571-9519-438b-b3bb-0057c554b85b
📒 Files selected for processing (4)
scripts/v18-to-v19/V18MigrationApp.tsscripts/v18-to-v19/V18MigrationProgressCoalescer.tsscripts/v18-to-v19/migrate.tstest/unit/scripts/v18-migration-progress.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: coverage-threshold
- GitHub Check: test-node (22)
- GitHub Check: test-deno
- GitHub Check: type-firewall-lint
- GitHub Check: typecheck-test-advisory
- GitHub Check: test-bun
- GitHub Check: type-firewall-generated-sdk
- GitHub Check: type-firewall-semgrep
- GitHub Check: type-firewall-types
- GitHub Check: v19 base/head performance
- GitHub Check: preflight
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Do not use direct imports fromsrc/infrastructure/**insrc/domain/**orsrc/ports/**; depend on a port instead.
Do not use direct Node built-ins insrc/domain/**orsrc/ports/**; use a port instead.
Files:
test/unit/scripts/v18-migration-progress.test.tsscripts/v18-to-v19/V18MigrationProgressCoalescer.tsscripts/v18-to-v19/V18MigrationApp.tsscripts/v18-to-v19/migrate.ts
🔇 Additional comments (4)
scripts/v18-to-v19/V18MigrationProgressCoalescer.ts (1)
54-60: LGTM!test/unit/scripts/v18-migration-progress.test.ts (1)
11-23: LGTM!Also applies to: 38-53, 55-90
scripts/v18-to-v19/V18MigrationApp.ts (1)
54-71: LGTM!scripts/v18-to-v19/migrate.ts (1)
56-97: LGTM!
Release Preflight
If this PR is from a |
Release Preflight
If this PR is from a |
1 participant
Closes #833.
Publishes inventory and rewrite progress for every commit, coalesces terminal rendering to a bounded cadence with boundary/final flushing, and leaves an explicit success, authority-change, and recovery report after TUI teardown.