fix(todo): prevent stuck status from stale metadata and parts shadowing backend - #637
Merged
Merged
Conversation
The websearch tool was using Effect.orDie at the tool execution level, which converted recoverable errors (like quota exceeded, invalid API key, network issues) into unrecoverable defects. This caused the generic 'Tool execution aborted' message instead of meaningful error messages. The outer Tool.define already wraps execution with Effect.orDie (tool.ts:124), so the inner orDie was redundant and harmful - it prevented the McpExa error catch handler from properly propagating user-friendly error messages. Fixes Astro-Han#612
## Problem Session UI experienced typing lag and scroll stuttering, especially with 50+ messages. Root causes identified in Astro-Han#615: 1. No virtualization - all messages rendered in DOM 2. Excessive createMemo calls per message (276+ reactive calls) 3. Unthrottled scroll events updating thumb position every frame 4. Inefficient content-visibility without proper sizing ## Changes ### Virtual Scrolling (message-timeline.tsx) - Replace <For> with virtua/solid <VList> - Only render visible messages, reducing DOM nodes from O(n) to O(1) - Remove redundant content-visibility CSS (handled by virtualizer) - Add getKey for stable item identity ### Throttled Scroll Events (scroll-view.tsx) - Throttle onScroll handler with requestAnimationFrame - Reduce CPU usage during scrolling by batching updates - Prevent redundant thumb position calculations ### Memo Optimization (message-part.tsx) - Add custom equals functions to createMemo for Map comparisons - Avoid unnecessary re-renders when Map contents haven't changed - Reduce reactive computation overhead ## Testing - All 1063 unit tests pass - No breaking changes to public API Fixes Astro-Han#615
…adowing backend - Replace strict isValidTodo batch rejection with normalizeTodo that fills missing fields (status/priority) with sensible defaults. Prevents entire metadata arrays from being discarded when a single todo is partially malformed, which previously caused fallback to stale tool input. - Let backend terminal state override stale active parts in selectSessionTodoDockSnapshot. When backend todos are all terminal but parts still show active, backend is fresher and should win. - Add tests covering: - partial metadata normalization - multi-part competition with invalid metadata - backend terminal overriding stale active parts - empty/non-array metadata fallback Fixes #TBD
github-actions
Bot
added
app
Contributor
📝 WalkthroughWalkthroughThis PR fixes the bug where todo status gets stuck on stale parts when the backend has completed terminal updates. It introduces a ChangesBackend Terminal Precedence & Clear Active Parts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/pages/session/session-status-extractors.test.ts, packages/app/src/pages/session/session-status-extractors.ts, packages/app/src/pages/session/todos/todo-source.test.ts, packages/app/src/pages/session/todos/todo-source.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces todo metadata normalization to handle malformed data with sensible defaults and updates the todo source logic to prefer terminal backend states over stale active parts, preventing the UI from getting stuck. It also optimizes the ScrollView component by throttling scroll events. Feedback suggests extending the backend-preference logic to selectSessionTodoDataSnapshot for UI consistency and implementing cleanup and untrack() for the scroll throttling logic to align with SolidJS best practices.
github-actions
Bot
added
the
harness
![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: 1
🧹 Nitpick comments (1)
packages/app/src/pages/session/todos/todo-source.ts (1)
19-27: ⚡ Quick winClarify type signature to match actual usage.
The function signature declares both parameters as
SessionTodoItem[], but it's called at line 39 withsourceBackend(which isTodo[]frominput.backend) andsourceParts(which isSessionTodoItem[]). While TypeScript permits this due to structural compatibility, the signature is misleading. Consider either:
- Updating the signature to
(backend: Todo[], parts: SessionTodoItem[]), or- Making it generic:
<T extends { id?: string; content: string }>(backend: T[], parts: T[]).This improves clarity for future maintainers.
🤖 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/app/src/pages/session/todos/todo-source.ts` around lines 19 - 27, The signature of sameTodoList is misleading because it currently types both parameters as SessionTodoItem[] but is invoked with sourceBackend (Todo[] from input.backend) and sourceParts (SessionTodoItem[]); update the signature to reflect real usage—for example change sameTodoList(backend: Todo[], parts: SessionTodoItem[]) or make it generic like function sameTodoList<T extends { id?: string; content: string }>(backend: T[], parts: SessionTodoItem[] | T[])—and adjust any imports/types (Todo, SessionTodoItem) referenced where sameTodoList is defined/used (e.g., calls from input.backend, sourceBackend, sourceParts) so the types align and intent is clear.
🤖 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 `@packages/opencode/src/server/instance/session.ts`:
- Around line 143-151: The route currently runs validator(...) before the E2E
feature gate so malformed requests can leak the route; move the
e2eSessionRoutesEnabled() check to run before validation (i.e., as a
pre-validation middleware) so it returns c.notFound() for all requests when
disabled. Concretely, ensure the e2eSessionRoutesEnabled() guard executes before
the validator(...) middleware (reference validator, z.object({ sessionID:
SessionID.zod, todos: z.array(Todo.Input) }) and the async handler using
c.notFound()), or add a small pre-validator wrapper that performs the
e2eSessionRoutesEnabled() check and calls c.notFound() when false.
---
Nitpick comments:
In `@packages/app/src/pages/session/todos/todo-source.ts`:
- Around line 19-27: The signature of sameTodoList is misleading because it
currently types both parameters as SessionTodoItem[] but is invoked with
sourceBackend (Todo[] from input.backend) and sourceParts (SessionTodoItem[]);
update the signature to reflect real usage—for example change
sameTodoList(backend: Todo[], parts: SessionTodoItem[]) or make it generic like
function sameTodoList<T extends { id?: string; content: string }>(backend: T[],
parts: SessionTodoItem[] | T[])—and adjust any imports/types (Todo,
SessionTodoItem) referenced where sameTodoList is defined/used (e.g., calls from
input.backend, sourceBackend, sourceParts) so the types align and intent is
clear.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2871d8d5-0a24-4c4c-89d3-e8c341507cc8
📒 Files selected for processing (14)
packages/app/e2e/session/session-composer-dock.spec.tspackages/app/src/components/session/session-status-panel.tsxpackages/app/src/components/session/session-status-summary.tsxpackages/app/src/context/global-sync.tsxpackages/app/src/context/global-sync/bootstrap.tspackages/app/src/context/global-sync/event-reducer.test.tspackages/app/src/context/global-sync/event-reducer.tspackages/app/src/pages/session/session-status-extractors.test.tspackages/app/src/pages/session/session-todos.test.tspackages/app/src/pages/session/todos/todo-source.test.tspackages/app/src/pages/session/todos/todo-source.tspackages/app/src/pages/session/todos/use-session-todos.tspackages/opencode/src/server/instance/middleware.tspackages/opencode/src/server/instance/session.ts
✅ Files skipped from review due to trivial changes (1)
- packages/app/src/context/global-sync/bootstrap.ts
13 tasks
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
2 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.
Summary
Fixes #636: the UI could keep showing stale
in_progresstodos from oldtodowriteparts after backend todo state had already reachedcompleted/ terminal, or after a live backend todo update explicitly cleared the todo list.Changes
todowriteparts for both primary and fallback sources.globalSync.data.session_todo[sessionID]so it sees backend todo updates instead of only message parts.status.Test Coverage
bun --cwd packages/app test src/context/global-sync.test.ts src/pages/session/todos/todo-source.test.ts src/pages/session/session-todos.test.tsbun --cwd packages/opencode test test/server/session-e2e-routes.test.tsbun --cwd packages/app typecheckbun --cwd packages/opencode typecheckPLAYWRIGHT_PORT=3317 bun --cwd packages/app test:e2e --project=chromium --grep "backend (terminal update|todo update is empty)" --reporter=line --workers=1git diff --check