Skip to content

fix(session): preserve live comments across daemon-driven reloads#278

Open
mvanhorn wants to merge 1 commit intomodem-dev:mainfrom
mvanhorn:fix/212-reload-preserves-live-comments
Open

fix(session): preserve live comments across daemon-driven reloads#278
mvanhorn wants to merge 1 commit intomodem-dev:mainfrom
mvanhorn:fix/212-reload-preserves-live-comments

Conversation

@mvanhorn
Copy link
Copy Markdown

Summary

hunk session reload (the daemon-driven reload path) was wiping the in-memory live-comment state. After this PR, daemon reloads preserve live comments the same way the r keystroke and watch-mode auto-reload already do.

Why this matters

Per #212, after an agent annotates the diff with hunk-level comments, calling hunk session reload (from a second terminal or from another agent message on the same session) reloaded the diff and dropped every annotation the user was actively reviewing. The maintainer confirmed the behavior in the issue comment: "Thanks for sharing. This is definitely a problem."

The root cause is in src/hunk-session/bridge.ts:70: the reload_session handler forwarded sourcePath but not resetApp. AppHost.tsx:63 treats undefined resetApp as truthy (options?.resetApp !== false is true), which increments appVersion, remounts <App />, and discards the React state in useReviewController -- including liveCommentsByFileId. The interactive r reload and watch-mode reload both pass resetApp: false (see App.tsx:354), so they preserve state; only the daemon path didn't.

Changes

  • src/hunk-session/bridge.ts: the reload_session handler now passes resetApp: false alongside sourcePath, matching the contract App.tsx:354 already uses. The handler type now also accepts resetApp so future opt-in-to-hard-reset is a one-line addition.
  • src/hunk-session/bridge.test.ts: extended the existing dispatch test with an exact-match assertion via toHaveBeenCalledWith({ resetApp: false, sourcePath: "/repo" }) -- exact match so accidental regression to the old undefined-passing behavior is caught.
  • src/ui/AppHost.interactions.test.tsx: new higher-level test that writes a live comment, dispatches a reload_session after rewriting the right-hand file, and asserts both the new content and the original review note appear in the rendered frame.

Testing

  • bun run typecheck -- clean
  • bun run format:check -- clean
  • bun run lint -- clean (0 warnings)
  • bun test ./src/hunk-session/bridge.test.ts ./src/ui/AppHost.interactions.test.tsx -- 45 pass, 0 fail
  • Manual smoke: start hunk diff in one terminal, write a live comment via the session bridge, run hunk session reload from a second terminal -- the comment persists across the reload.

Residual risk: an agent that explicitly wanted a hard reset on reload_session loses that capability. The handler type now accepts resetApp, so an explicit opt-in wire flag is a follow-up if anyone needs it.

Fixes #212

AI was used for assistance.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 10, 2026

Greptile Summary

This PR fixes a bug where hunk session reload (the daemon-driven path) dropped all in-memory live comments by failing to pass resetApp: false to AppHost, causing the entire <App /> component to remount and discard React state. The fix aligns the daemon reload path with the existing r-keystroke and watch-mode reload paths, which already used resetApp: false.

  • src/hunk-session/bridge.ts: the reload_session handler now explicitly passes resetApp: false, preventing AppHost from bumping appVersion and remounting <App /> on daemon reloads.
  • src/hunk-session/bridge.test.ts: existing dispatch test upgraded with an exact-match toHaveBeenCalledWith assertion, catching any future regression to the old undefined-passing behavior.
  • src/ui/AppHost.interactions.test.tsx: new integration test writes a live comment, rewrites the right-hand file, fires reload_session, and asserts both the updated diff content and the original annotation survive.

Confidence Score: 5/5

Safe to merge — the change is a one-line targeted fix to an established, tested code path with no side effects on unrelated behaviour.

The daemon reload path now matches the behavior of the interactive r reload and watch-mode reload, which already passed resetApp: false. The fix is minimal, the root cause is clearly traced in AppHost.tsx, and the new tests cover both the unit contract and the end-to-end comment-preservation flow. No new surfaces are opened, and no existing callers are broken.

No files require special attention.

Important Files Changed

Filename Overview
src/hunk-session/bridge.ts One-line fix: adds resetApp: false to the reload_session handler call, aligning the daemon path with interactive reload; handler type updated to accept the new field.
src/hunk-session/bridge.test.ts Exact-match assertion added to the reload handler test; also adds sourcePath to the dispatch input so the full options object is validated.
src/ui/AppHost.interactions.test.tsx New end-to-end interaction test and a dispatchCommand helper added to the mock client; test covers the full write-comment → reload → assert-both cycle.

Sequence Diagram

sequenceDiagram
    participant Daemon as Daemon / Agent
    participant Bridge as bridge.ts (createHunkSessionBridge)
    participant AppHost as AppHost.tsx (reloadSession)
    participant App as App component

    Daemon->>Bridge: "dispatchCommand(reload_session, { sourcePath })"
    Note over Bridge: OLD: forwarded only sourcePath<br/>NEW: also passes resetApp: false
    Bridge->>AppHost: "reloadSession(nextInput, { resetApp: false, sourcePath })"
    AppHost->>AppHost: setActiveBootstrap(nextBootstrap)
    alt "resetApp !== false (OLD behaviour)"
        AppHost->>App: setAppVersion(v+1) full remount, state lost
    else "resetApp === false (NEW behaviour)"
        AppHost-->>App: no remount, liveCommentsByFileId preserved
    end
    AppHost-->>Daemon: ReloadedSessionResult
Loading

Reviews (1): Last reviewed commit: "fix(session): preserve live comments acr..." | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reloading diff wipes agent review annotations

1 participant