Add restore-to-message arrow on user messages#3596
Conversation
|
@BugBot run |
![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 a feature to restore the app's codebase and database to the state prior to a specific user message, creating a new chat with the preceding messages. Key changes include the implementation of the restoreToMessageVersion IPC handler, extraction of the core revert logic into revertCodebaseToVersion, UI updates in ChatMessage to trigger the restore, and a new E2E test. Feedback highlights three main areas for improvement: resolving an unhandled promise rejection in revertCodebaseToVersion due to an unawaited promise, performing the codebase revert before database insertion to avoid orphaned chats on failure, and disabling all restore buttons globally during a restore operation to prevent concurrent requests.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
github-actions
Bot
added
the
needs-human:review-issue
![dyad-assistant[bot]](https://avatars.githubusercontent.com/in/3027874?s=60&v=4){kind=small}
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Well-structured PR that adds a "restore to message" feature with good refactoring of the existing Issues Summary
🟡 MEDIUM — No chat-app ownership check in restoreToMessageVersion The 🟡 MEDIUM — Stale branch causes version field conflict with main The PR changes 🟢 Low Priority Notes (2 items)
Generated by Dyadbot persona-based code review |
|
@BugBot run |
🤖 Claude Code Review SummaryPR Confidence: 4/5All five reviewer-raised concerns were addressed with scoped code changes; unit tests (1723), lint, and typecheck all pass. Not 5/5 only because the new restore-to-message flow is exercised by E2E (not unit) tests, which were not run in this pass. Unresolved Threads
No unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions — the principles were clear enough for every decision this run (Principle #6 cleanly covered the only UX judgment call). 🤖 Generated by Claude Code |
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Issues Summary
MEDIUM: Missing chat list cache invalidation -- The 🟢 Low Priority Notes (5 items)
What looks good
Generated by Dyadbot persona-based code review |
|
@BugBot run |
🤖 Claude Code Review SummaryPR Confidence: 5/5All review threads from trusted authors are resolved; the only outstanding comment was a missing chat-list cache invalidation, now fixed, and lint/type-check pass. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions — the principles were clear enough for every decision in this run. 🤖 Generated by Claude Code |
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge This PR adds a well-implemented "restore to message" feature that lets users fork their chat and revert their app's codebase to the state before a specific user message. The implementation is solid across all layers:
Issues Summary
🟢 Low Priority Notes (3 items)
Generated by Dyadbot persona-based code review |
|
@BugBot run |
🤖 Claude Code Review SummaryPR Confidence: 4/5All review comments are addressed; changes are minimal, lint-clean, and type-check passes — only knocked from 5 because the e2e suite ( Unresolved Threads
Resolved Threads
Product Principle SuggestionsNo suggestions — the existing principles (#4 and #6) gave clear guidance for both decisions. 🤖 Generated by Claude Code |
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Well-structured PR that adds a "restore to message" feature with an undo arrow on user messages. The feature forks the chat (and optionally reverts the codebase) to the state before a given message. The implementation is solid: proper The refactoring of Issues Summary
🟢 Low Priority Notes (5 items)
Generated by Dyadbot persona-based code review |
🎭 Playwright Test Results❌ Some tests failed
Summary: 353 passed, 9 failed, 1 flaky, 7 skipped Failed Tests🍎 macOS
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/cloud_sandbox.spec.ts \
e2e-tests/engine.spec.ts \
e2e-tests/local_agent_advanced.spec.ts \
e2e-tests/mcp_oauth.spec.ts \
e2e-tests/mcp.spec.ts
|
|
@BugBot run |
🤖 Claude Code Review SummaryPR Confidence: 5/5All six review threads were valid, addressable feedback (or a factually-inapplicable concern); each was fixed or explained, the branch typechecks, and lint/format pass. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions 🤖 Generated by Claude Code |
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge ✅ No issues found by persona-based review. Review NotesScope: 78 files changed (1049+/147-). ~65 are aria snapshot updates adding the new "Restore to this point" button. The core changes span 7 source files introducing a new IPC handler, React hook mutation, UI component, and 3 E2E tests. All diffs were reviewed in full. Architecture & Correctness:
Security / IPC Boundary:
Database:
UX:
Testing:
🟢 Low Priority Notes (4 items)
Generated by Dyadbot persona-based code review |
🎭 Playwright Test Results❌ Some tests failed
Summary: 354 passed, 8 failed, 7 skipped Failed Tests🍎 macOS
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/engine.spec.ts \
e2e-tests/local_agent_advanced.spec.ts \
e2e-tests/mcp_oauth.spec.ts \
e2e-tests/mcp.spec.ts |
- version_handlers: attach .catch() to the un-awaited preserve-branch deletion so its async rejection is handled instead of becoming an unhandled promise rejection (the surrounding try/catch could not catch it) - restoreToMessageVersion: revert the codebase before creating the new chat so a failed revert no longer leaves an orphaned, partially-created chat - restoreToMessageVersion: add a chat.appId === appId ownership check (defense in depth) so a mismatched (appId, chatId) can't act on the wrong app - ChatMessage/useVersions: disable all restore buttons while any restore-to-message is pending (via useIsMutating) to prevent concurrent restores; only the initiating button shows the spinner - package-lock.json: drop the unintended 1.2.0-beta.1 -> 1.3.0 version bump so this feature PR no longer touches the release version Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Invalidate chats query cache after restoreToMessageVersion creates a new chat, so the sidebar chat list refreshes immediately instead of the new chat appearing as a ghost entry until the next refresh Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Return a distinct response shape (no newChatId) when no version can be determined for restore-to-message, so the renderer stays on the current chat instead of "navigating" to the same chat (misleading no-op) - Carry over the original chat's title (with "(restored)" suffix) to the forked chat so it's identifiable in the sidebar instead of untitled Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add the new "Restore to this point" button to user-message snapshots across 70 specs. Also update restore_to_message.spec.ts to expect 3 restore buttons instead of 2, since importing the "minimal" fixture auto-generates an AI_RULES.md user message before the test's prompts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Remove duplicate error toast on restore failure: drop meta.showErrorToast from restoreToMessageMutation since handleRestoreToMessage already shows the error (and also covers the cancelStream call) - Wrap forked chat + messages inserts in a transaction so a failed messages insert can't leave an orphaned "(restored)" chat - Walk forward past intervening user messages when resolving the target commit hash so consecutive user messages don't skip the responding assistant Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Disable all version-modifying buttons (message restore arrows and the version-pane revert button) while any restore-to-message or revert-version operation is in flight, via a unified isAnyVersionMutationPending flag - Show a compound 'Code restored, but: ...' toast when a restore-to-message reverts the code but a secondary step fails, so the partial success is clear Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- version_handlers.ts: move successMessage "(including database)" inside the try block so it only reports success when the Neon restore actually succeeded - version_handlers.ts: fall back to a bare "(restored)" title for forks of untitled chats so they remain identifiable in the sidebar Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Add focus-visible:opacity-100 to the restore-to-message button so keyboard-only users can see it when focused - Make the pre-restore auto-commit message more descriptive so the preserved uncommitted changes are easy to identify in version history Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Don't carry over usingFreeAgentModeQuota when forking a chat on restore, to avoid double-counting free-agent quota - Bail out with a warning instead of creating an empty '(restored)' chat when restoring to the first message - Strip existing '(restored)' suffix before appending to prevent title pile-up on repeated restores - Run restore onSuccess query invalidations concurrently so navigation isn't delayed - Hide restore button on cancelled user prompts - E2E: verify the original chat is left intact after a restore Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Restoring to the first message now forks an empty chat at version 1 instead of erroring with "Cannot restore before the first message". - The per-message restore button is always visible (no longer hover-only). - The restore dialog now offers two vertical options: "Restore code & fork chat" (previous behavior) and "Fork chat only" (forks the chat without reverting the codebase/database). Fork-only anchors the new chat's initialCommitHash to the current HEAD. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Restore the libc: ["glibc"] block that was inadvertently removed on this branch; it's unrelated to this PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The revertVersion handler no longer inlines the Neon restore and Supabase re-deploy logic — upstream extracted it into revertCodebaseToVersion(), which the handler already calls. The rebase had re-introduced the old inline copy, referencing now-const successMessage/warningMessage and an undefined currentCommitHash. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Use getDatabaseRestoreWarning in revertCodebaseToVersion's Neon catch so the retention-window explanation is preserved for both the revert and restore-to-message flows; fix the copy-pasted log message - Add AlertDialogDescription to the restore confirmation dialog explaining the consequences of each action (fixes a11y warning) - Make cancelStream await the in-flight stream handler's completion so a cancelled turn's partial file writes settle before a subsequent revert - Document that the copied-message field list must stay in sync with the messages table schema Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Clear streamCompletions map in the before-quit handler to match the established stream-cleanup pattern - Resolve the stream completion promise before deleting it from the map so a post-abort reader still observes a settled promise - Validate the resolved targetCommitHash exists (via assertVersionExists) before restoring the codebase, returning a user-friendly warning instead of an opaque git error for stale/GC'd hashes - Add a compile-time exhaustiveness guard so new messages columns can't be silently dropped when forking a chat - Constrain restoreToMessage chatId/messageId to positive integers Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@BugBot run |
| | "role" | ||
| | "content" | ||
| | "approvalState" | ||
| | "sourceCommitHash" |
There was a problem hiding this comment.
🟡 MEDIUM
Fork-only mode uses hardcoded "main" ref instead of current branch HEAD
When restoreCodebase is false (fork-only mode), forkInitialCommitHash is computed via getCurrentCommitHash({ path: appPath, ref: "main" }). If the user's app is on a non-main branch, this captures main's commit hash rather than the current branch HEAD. Since initialCommitHash tracks the code state when a chat started, the forked chat would have an incorrect baseline, making "changes since chat started" diffs wrong. The regular createChat handler (chat_handlers.ts:41) correctly uses getCurrentCommitHash without a ref (defaulting to HEAD), and the existing revertCodebaseToVersion helper also resolves the current branch dynamically.
💡 Suggestion: Replace ref: "main" with ref: "HEAD" (or omit the ref argument entirely to use the default) so the fork-only chat records the current branch HEAD: await getCurrentCommitHash({ path: appPath }).catch(() => targetCommitHash).
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge This PR adds a well-implemented "restore to message" feature with two modes: revert code + fork chat, or fork chat only. The implementation is solid across the stack: proper Zod-validated IPC contract, defense-in-depth validation (chat-app ownership check), compile-time schema drift guard for message copying, DB transaction for atomic chat+messages creation, and stream completion tracking to prevent races with cancelled streams. Issues Summary
MEDIUM detail: When 🟢 Low Priority Notes (3 items)
Positive observations:
Generated by Dyadbot persona-based code review |
🎭 Playwright Test Results❌ Some tests failed
Summary: 185 passed, 4 failed, 1 flaky, 6 skipped Failed Tests🍎 macOS
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/local_agent_advanced.spec.ts \
e2e-tests/mcp_oauth.spec.ts
|
In fork-only mode (restoreCodebase=false), forkInitialCommitHash was computed with ref: "main", capturing main's HEAD rather than the current branch HEAD. This gave forked chats an incorrect baseline, making "changes since chat started" diffs wrong on non-main branches. Omit the ref so getCurrentCommitHash defaults to HEAD, matching the createChat handler. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@BugBot run |
1 similar comment
|
@BugBot run |
2 participants
No description provided.