Surface thread title regeneration failures instead of silently completing - #198
Conversation
…ting Title regeneration resolved its generator from the global text-generation instance setting, and ProviderCommandReactor's catchCause converted any generation failure into a *successful* completion carrying no title. The user saw the spinner appear and clear with the title unchanged and no error. The fork's Amp/Copilot/Gemini CLI/Droid text-generation shapes fail unconditionally, so for them the action was a guaranteed no-op, but the same path also swallowed transient CLI and network failures everywhere. Carry the failure through instead: - ThreadTitleRegeneration gains an optional `error`. The record now encodes two states — pending while `error` is null, failed once stamped — reusing the field clients already read rather than adding a parallel one. - thread.title.regeneration.complete gains an optional `error`, mutually exclusive with `title`. The reactor passes TextGenerationError.detail through; the decider keeps the record and stamps the reason on failure and still clears it on success. A new request or a manual rename clears it through the existing paths. - Startup only clears *pending* regenerations now: a recorded failure is a finished result, not an interrupted one. - Persisted via a new title_regeneration_error column so the projection, shell snapshots, and replay stay consistent. - SidebarV2 routes its pending checks through shared helpers (a failed record no longer spins or disables the menu item), toasts the reason on a live failure, and offers "Retry regenerate title" afterwards. The toast diff reports transitions only, so reloading never replays old failures. Fixes #197
github-actions
Bot
added
vouch:trusted
|
Warning Review limit reached
Next review available in: 45 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughTitle regeneration failures now carry explicit error details through contracts, server completion handling, persistence, and client state. The sidebar displays failure toasts and offers retry actions. Migration and regression tests cover storage, failure detection, and retry behavior. ChangesTitle regeneration error flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SidebarV2
participant ProviderCommandReactor
participant decider
participant ProjectionThreads
SidebarV2->>ProviderCommandReactor: request title regeneration
ProviderCommandReactor->>decider: complete with generated title or error
decider->>ProjectionThreads: persist regeneration state
ProjectionThreads-->>SidebarV2: return thread error state
SidebarV2->>SidebarV2: show failure toast or retry label
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 606fb87cbc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const nextTitleRegeneration = | ||
| command.error !== undefined && pending != null | ||
| ? { requestId: pending.requestId, startedAt: pending.startedAt, error: command.error } |
There was a problem hiding this comment.
When an upgraded server reports a generation failure to an older web or desktop client, retaining a non-null titleRegeneration record makes that client treat the request as permanently in flight: the pre-change SidebarV2 computes pending state with thread.titleRegeneration != null, showing the spinner and disabling regeneration. Because error is optional, the old schema still decodes this record without signaling incompatibility. Represent the failed state without overloading the existing pending field, or negotiate support before emitting it.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/server/src/orchestration/decider.ts (1)
686-695: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: move the
nextTitleRegenerationcomputation inside therequestIsCurrentbranch.
nextTitleRegenerationis computed unconditionally, but it is only used whenrequestIsCurrentis true (line 707). This is not a bug — the discarded value never reaches the payload for a stale completion — but computing it inline only when needed would make the "stale completions cannot affect the current pending record" invariant more obvious from the code structure alone.🤖 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 `@apps/server/src/orchestration/decider.ts` around lines 686 - 695, Move the nextTitleRegeneration computation into the requestIsCurrent branch where it is consumed, keeping stale completion handling from constructing an unused value. Preserve the existing failed-completion record and successful/no-op clearing behavior for current requests.packages/contracts/src/orchestration.ts (1)
897-901: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider enforcing "title xor error" at the schema level.
The comment states
titleanderrorare mutually exclusive onThreadTitleRegenerationCompleteCommand, but the schema allows both fields to be set simultaneously. Today onlyProviderCommandReactor.tsproduces this command, and it never sets both, so the invariant holds by construction. A schema-level refinement would make the contract self-enforcing against a future producer that violates it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/contracts/src/orchestration.ts` around lines 897 - 901, Update the ThreadTitleRegenerationCompleteCommand schema to enforce that exactly one of title or error is present, using a schema-level refinement while preserving the existing field types and optionality. Ensure commands containing both fields are rejected, matching the documented mutual-exclusion invariant.apps/server/src/persistence/Migrations/036_ProjectionThreadTitleRegenerationError.test.ts (2)
19-23: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the nullable
TEXTcontract.The current assertion checks only the column name. It would pass if the column had the wrong type or a
NOT NULLconstraint. SelecttypeandnotnullfromPRAGMA table_infoand asserttype === "TEXT"andnotnull === 0.Suggested assertion
- const columns = yield* sql<{ readonly name: string }>` + const columns = yield* sql<{ + readonly name: string; + readonly type: string; + readonly notnull: number; + }>` PRAGMA table_info(projection_threads) `; - const names = new Set(columns.map((column) => column.name)); - assert.ok(names.has("title_regeneration_error")); + const errorColumn = columns.find( + (column) => column.name === "title_regeneration_error", + ); + assert.ok(errorColumn); + assert.strictEqual(errorColumn.type, "TEXT"); + assert.strictEqual(errorColumn.notnull, 0);🤖 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 `@apps/server/src/persistence/Migrations/036_ProjectionThreadTitleRegenerationError.test.ts` around lines 19 - 23, Update the migration test’s PRAGMA table_info query to select each column’s type and notnull metadata, then locate title_regeneration_error and assert its type is "TEXT" and notnull is 0, preserving the existing column-presence assertion.
27-39: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake the idempotence test exercise the column guard.
The second
runMigrations({ toMigrationInclusive: 39 })call skips migration 39 because the migration ledger already records it. It does not execute thePRAGMAcheck or theALTER TABLEbranch.In a fresh database, run through migration 38, add the column manually, then run migration 39 and assert that exactly one column exists.
🤖 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 `@apps/server/src/persistence/Migrations/036_ProjectionThreadTitleRegenerationError.test.ts` around lines 27 - 39, Update the idempotence test around runMigrations so the database first migrates only through 38, then manually adds title_regeneration_error to projection_threads, and finally runs migration 39. Keep the existing PRAGMA assertion verifying exactly one matching column, ensuring migration 39’s column-existence guard is actually exercised.
🤖 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.
Nitpick comments:
In `@apps/server/src/orchestration/decider.ts`:
- Around line 686-695: Move the nextTitleRegeneration computation into the
requestIsCurrent branch where it is consumed, keeping stale completion handling
from constructing an unused value. Preserve the existing failed-completion
record and successful/no-op clearing behavior for current requests.
In
`@apps/server/src/persistence/Migrations/036_ProjectionThreadTitleRegenerationError.test.ts`:
- Around line 19-23: Update the migration test’s PRAGMA table_info query to
select each column’s type and notnull metadata, then locate
title_regeneration_error and assert its type is "TEXT" and notnull is 0,
preserving the existing column-presence assertion.
- Around line 27-39: Update the idempotence test around runMigrations so the
database first migrates only through 38, then manually adds
title_regeneration_error to projection_threads, and finally runs migration 39.
Keep the existing PRAGMA assertion verifying exactly one matching column,
ensuring migration 39’s column-existence guard is actually exercised.
In `@packages/contracts/src/orchestration.ts`:
- Around line 897-901: Update the ThreadTitleRegenerationCompleteCommand schema
to enforce that exactly one of title or error is present, using a schema-level
refinement while preserving the existing field types and optionality. Ensure
commands containing both fields are rejected, matching the documented
mutual-exclusion invariant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6feeafd9-9f07-4d0d-a362-8ca351aba3a5
📒 Files selected for processing (15)
apps/server/src/orchestration/Layers/ProjectionPipeline.tsapps/server/src/orchestration/Layers/ProjectionSnapshotQuery.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.test.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.tsapps/server/src/orchestration/decider.tsapps/server/src/persistence/Layers/ProjectionThreads.tsapps/server/src/persistence/Migrations.tsapps/server/src/persistence/Migrations/036_ProjectionThreadTitleRegenerationError.test.tsapps/server/src/persistence/Migrations/036_ProjectionThreadTitleRegenerationError.tsapps/server/src/persistence/Services/ProjectionThreads.tsapps/web/src/components/SidebarV2.tsxpackages/client-runtime/package.jsonpackages/client-runtime/src/state/threadTitleRegeneration.test.tspackages/client-runtime/src/state/threadTitleRegeneration.tspackages/contracts/src/orchestration.ts
Review found two problems with recording the failure inside the pending `titleRegeneration` record. Older web clients read "regeneration in flight" as `titleRegeneration != null`, so a failure stored there left them showing "Regenerating…" with the action disabled indefinitely. Move the failure to a sibling `titleRegenerationFailure` field: every completion now clears the pending record exactly as a success always did, so an old client sees a failure the way it sees a success and simply ignores the field it cannot read. This also restores the startup sweep to clearing all pending records, since a pending record once again means only "interrupted". The reason was also invisible after a reload — the toast fires on a live transition only, and the menu item said nothing beyond "Retry". The context-menu label now carries the reason, collapsed to one clause, which is stateless and survives reloads without re-toasting. Migration 036 grows to the three failure columns and is renamed to match.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09c0c9744a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _tag: "Failed", | ||
| // The completion payload requires a non-empty reason; a provider | ||
| // that fails without one still has to produce a visible error. | ||
| error: error.detail.trim() || TITLE_REGENERATION_FALLBACK_ERROR, |
There was a problem hiding this comment.
When a provider exits with verbose output, this stores the entire TextGenerationError.detail without a size limit. For example, CodexTextGeneration.ts:239-248 builds the detail from the complete stderr/stdout stream; the value is then persisted in the event and projection and included in every shell snapshot until the failure is cleared, so one noisy failure can permanently inflate SQLite rows and WebSocket payloads. Normalize or truncate the user-facing reason before dispatching the completion.
AGENTS.md reference: AGENTS.md:L56-L62
Useful? React with 👍 / 👎.
| snoozedUntil: row.snoozedUntil, | ||
| snoozedAt: row.snoozedAt, | ||
| titleRegeneration: mapTitleRegeneration(row), | ||
| titleRegenerationFailure: mapTitleRegenerationFailure(row), |
There was a problem hiding this comment.
When ProjectionSnapshotQuery.test.ts runs, both getSnapshot() and getShellSnapshot() now return titleRegenerationFailure: null because this mapping always materializes the field, but the exact assert.deepEqual expectations beginning at lines 283 and 398 omit it. Those existing backend tests therefore fail even for threads without a regeneration failure; update the expected fixtures and cover the non-null projection path.
AGENTS.md reference: AGENTS.md:L29-L33
Useful? React with 👍 / 👎.
Review found the new tests passed vacuously. The shared-layer idiom used elsewhere in this directory hands every case in a block the same in-memory database, so once the first case migrated to the head id the migrator skipped every later runMigrations call and the bodies under test never ran again. Removing all three PRAGMA guards from migration 40 still left the suite green, even though an unguarded 40 dies with "duplicate column name" on every fresh install. Provide a fresh database per case and assert against raw PRAGMA rows rather than a Set, which cannot express a duplicate column. Confirmed by negative control: with the guards removed the fresh-install and pending-columns cases now fail with MigrationError, and the stale-39 repair case still passes; with the guards restored all three pass. The 036 test added in #198 had the same flaw and is fixed the same way.
Round three established that the test runner works after all — the `vp` on PATH resolves to a build that fails at collection, but `npx vp test run` runs everything. That surfaced a genuine regression from #198: adding titleRegenerationFailure to the hydration query left ProjectionSnapshotQuery's expected thread and shell objects one key short, so that suite has been red on main since the merge. Add the field to both expectations. Full suites now pass: 2613 in apps/server + packages, 1724 in apps/web. Also corrected two comments that claimed more than they deliver. SQLite rejects a duplicate ADD COLUMN outright, so the array-over-Set note in migrationTestSupport overstated what a count assertion can catch; and both shapes of migration 39 reached main in the same merge, so no released build ever carried the earlier one — only machines that ran the PR branch mid-review are affected.
…expectations `titleRegenerationFailure` was added to the thread read model in #198, but the two `assert.deepEqual` expectations in ProjectionSnapshotQuery.test.ts were not updated, so `Test` has been failing on `main` since that merge.
1 participant
Fixes #197.
Problem
regenerateThreadTitleresolves its generator from the globaltextGenerationModelSelectionsetting, and theEffect.catchCauseinProviderCommandReactor.tsconverted any generation failure into a successful completion carrying no title. The user picked "Regenerate title", the spinner appeared and cleared, and the title was unchanged with no error.The fork's
AmpTextGeneration,CopilotTextGeneration,GeminiCliTextGeneration, and the inline Droid stub fail every operation unconditionally, so for those instances the action was a guaranteed no-op — which is how this surfaced. The same path also swallows transient CLI and network failures for the drivers that do implement generation, so this is not fork-specific and is worth reporting upstream.Approach
Carry the failure through, per the issue's suggested fix.
ThreadTitleRegenerationgains an optionalerror, so the existing record encodes two states: pending whileerroris null, failed once the server stamps a reason. This reuses the field clients already read (onemapTitleRegenerationhelper, one shell field) instead of adding a paralleltitleRegenerationErroracross the eight thread/shell mapping sites.A transient event-only signal was considered and rejected: the sidebar is fed by the shell stream, which coalesces to the latest event per aggregate and re-reads projected state, so a one-shot notice event would be dropped under load. The failure has to be state to reach the sidebar reliably.
Changes
Contracts
ThreadTitleRegeneration.error(optional, nullable).thread.title.regeneration.completegains an optionalerror, mutually exclusive withtitle.Server
ProviderCommandReactor—TextGenerationError.detailis carried into the completion instead of being dropped; an empty detail falls back to a generic message, since the payload requires a non-empty reason. Unexpected defects also complete as failures rather than as silent successes.decider— a failed completion keeps the record and stamps the reason; success still clears it. A new regeneration request or a manual rename clears it through the existing paths.036_ProjectionThreadTitleRegenerationErroraddstitle_regeneration_error, wired throughProjectionThreads(service + layer),ProjectionPipeline, and all fourProjectionSnapshotQueryselects, so projection, shell snapshots, and replay stay consistent.Client
@t3tools/client-runtime/state/thread-title-regeneration:isTitleRegenerationPending,titleRegenerationError, andcollectTitleRegenerationFailures. The last reports transitions only — a failure already present the first time a thread is observed is recorded silently, so a page reload never replays old errors as new toasts, while a repeat failure still reports because each request carries its own id.SidebarV2— pending checks route through the helper (a failed record no longer spins or disables the menu item), a live failure raises an error toast naming the thread and the reason, and the menu item reads "Retry regenerate title" afterwards.Verification
vp run typecheckandvp checkpass (0 errors; only pre-existing warnings).Tests are written but were not executed.
vp testis broken repo-wide on this machine — a trivial two-line smoke test fails at collection withTypeError: Cannot read properties of undefined (reading 'config'), and the full server suite fails 213/213 suites identically. Reproduced on a clean tree before these changes and again after a freshvp install, so it is pre-existing toolchain breakage rather than a regression here. Tests added/updated:ProviderCommandReactor.test.ts— the reason is recorded on failure (the existing "clears title regeneration state when generation fails" case asserted the old silent behavior and was updated), and a retry clears it.036_ProjectionThreadTitleRegenerationError.test.ts— column added, idempotent.threadTitleRegeneration.test.ts— predicates and the failure-transition diff.The integrated browser pass was not run: provoking a real failure needs one of the stub-text-generation providers configured as the global text-generation instance with its CLI installed, and seeding
state.sqlitedirectly would show the menu-label change but by design would not fire the toast, which only fires on a live transition.Not addressed
Gating the action up front. As the issue notes, deriving
capabilities.threadTitleRegenerationfrom the configured instance does not work — the descriptor is built once at startup while the text-generation instance is a runtime setting. Filtering the settings picker by a static per-provider-kindsupportsTextGenerationcapability would prevent selecting an unsupported instance at all, but that introduces a new capability concept and belongs in its own change.Summary by CodeRabbit
New Features
Bug Fixes