Checkpointing & Diffs - #64
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds per-turn checkpoint diffs and revert flows: new diff types and derivation, UI to view/revert diffs (lazy-loaded DiffPanel, per-message controls), client/store persistence and WS/provider RPCs for listing/diffing/reverting, and a filesystem-backed server checkpoint store with capture/diff/restore APIs. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Browser UI
participant Store as Client Store
participant SessionLogic as Session Logic
participant WS as WebSocket
participant ProviderMgr as Server ProviderManager
participant FSStore as FilesystemCheckpointStore
User->>UI: Click "View diff" on message
UI->>SessionLogic: deriveTurnDiffSummaries(thread.events)
SessionLogic-->>UI: turnDiffSummaries[]
UI->>Store: dispatch(OPEN_DIFF(threadId, turnId?, filePath?))
Store-->>UI: state updated (diffOpen, targets)
UI->>WS: providers.getCheckpointDiff(params) (if needed)
WS->>ProviderMgr: getCheckpointDiff(params)
ProviderMgr->>FSStore: diffCheckpoints(cwd, fromTurn,toTurn)
FSStore-->>ProviderMgr: unified diff text
ProviderMgr-->>WS: diff result
WS-->>UI: diff text
UI->>UI: DiffPanel renders patch viewer / file list / turn selector
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Contributor
Add checkpointing and diffs across server, contracts, and web UI to list, diff, and revert provider checkpoints, and render turn diffs on demandImplement provider checkpoint RPCs ( 📍Where to StartStart with the provider RPC surface and flow in Macroscope summarized e29a770. |
Greptile SummaryThis PR replaces the placeholder diff panel with a fully functional per-turn diff viewer, adds new store state for targeting a specific thread/turn/file, and surfaces changed-file metadata directly on assistant timeline messages. The overall design is clean and consistent with existing patterns in the codebase. Key changes:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Server as Codex App Server
participant Store as Zustand Store
participant ChatView as ChatView
participant DiffPanel as DiffPanel (lazy)
Server->>Store: APPLY_EVENT (item/started, fileChange)
Server->>Store: APPLY_EVENT (item/completed, fileChange)
Server->>Store: APPLY_EVENT (turn/diff/updated)
Server->>Store: APPLY_EVENT (item/completed, agentMessage)
Server->>Store: APPLY_EVENT (turn/completed)
Note over Store: events prepended newest-first
ChatView->>ChatView: deriveTurnDiffSummaries(events)<br/>[useMemo on activeThread.events]
ChatView->>ChatView: turnDiffSummaryByAssistantMessageId<br/>[useMemo on turnDiffSummaries]
ChatView->>ChatView: Render "Changed files" block<br/>below assistant message
alt User clicks "View diff" or file chip
ChatView->>Store: OPEN_DIFF { threadId, turnId, filePath }
Store->>Store: diffOpen=true, diffThreadId, diffTurnId, diffFilePath set
Store->>DiffPanel: Lazy load + mount
DiffPanel->>DiffPanel: deriveTurnDiffSummaries(events)<br/>[separate useMemo — duplicate computation]
DiffPanel->>DiffPanel: Resolve selectedTurn & selectedFile<br/>from stored targets
DiffPanel->>DiffPanel: Render PatchDiff with selectedPatch
end
alt User selects different turn in DiffPanel
DiffPanel->>Store: SET_DIFF_TARGET { threadId, turnId, filePath }
end
alt User closes DiffPanel
DiffPanel->>Store: CLOSE_DIFF
Store->>Store: diffOpen=false (targets preserved)
end
alt User deletes thread whose diffThreadId matches
Store->>Store: DELETE_THREAD clears diffOpen,<br/>diffThreadId, diffTurnId, diffFilePath
end
Last reviewed commit: f889da1 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/session-logic.ts`:
- Around line 549-669: deriveTurnDiffSummaries processes events in reverse
(newest first) but currently overwrites newer data with older events; update the
merge logic so first-seen values are preserved and only fill missing fields: in
deriveTurnDiffSummaries change assignments to summary.unifiedDiff and
summary.assistantMessageId to set only when they are undefined (e.g., if
(summary.unifiedDiff === undefined) summary.unifiedDiff = diff), and when
merging per-file entries from parseFileChangeEntriesFromEvent ensure that if an
existing file entry exists you only set kind or diff when existing.kind or
existing.diff are undefined (do not overwrite non-undefined values); apply the
same “only fill missing” behavior where turn status or other fields are merged
so newer values remain.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
juliusmarminge
force-pushed
the
codething/ecb6d645
branch
from
February 18, 2026 07:55
f889da1 to
4a686f8
Compare
juliusmarminge
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
apps/server/src/providerManager.test.ts (1)
152-180: Misleading variable name for combined mock object.The variable
codexon line 180 contains bothcodexmethods andfilesystemCheckpointStore, but onlycodexproperties are assigned to it. ThefilesystemCheckpointStoreis accessed via a separateinternalsvariable. This is correct but the type cast structure is a bit confusing.Consider splitting the type casts more clearly or renaming for clarity in future tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providerManager.test.ts` around lines 152 - 180, The test's cast on the manager is misleading because the single variable named `codex` is given a composite type that includes both `codex` methods and `filesystemCheckpointStore` but you only assign the codex methods to it; split the casts and variables for clarity by extracting two separate typed views from `manager` (e.g., `const codex = (manager as unknown as { codex: { ... } }).codex;` and `const internals = (manager as unknown as { filesystemCheckpointStore: { ... } }).filesystemCheckpointStore;`) or rename the existing variable to `internals` where appropriate so `codex` only contains codex methods and `internals` contains `filesystemCheckpointStore`; update usages of `codex`/`internals` accordingly in the test (references: ProviderManager, codex, filesystemCheckpointStore, internals, hasSession, readThread, rollbackThread, listSessions, isGitRepository, ensureRootCheckpoint).apps/web/src/store.ts (1)
253-259: Minor: Prefer spread syntax overObject.assignfor consistency.The rest of the codebase uses spread syntax (
{ ...summary, key: value }). UsingObject.assignhere is functionally equivalent but inconsistent with the surrounding code style.♻️ Suggested refactor
return sorted.map((summary) => typeof summary.checkpointTurnCount === "number" ? summary - : Object.assign({}, summary, { - checkpointTurnCount: inferredTurnCountByTurnId[summary.turnId], - }), + : { + ...summary, + checkpointTurnCount: inferredTurnCountByTurnId[summary.turnId], + }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/store.ts` around lines 253 - 259, Replace the Object.assign usage inside the return of the sorted.map callback with spread syntax to match project style: where the code currently uses Object.assign({}, summary, { checkpointTurnCount: inferredTurnCountByTurnId[summary.turnId] }), change it to use { ...summary, checkpointTurnCount: inferredTurnCountByTurnId[summary.turnId] } so the map callback that checks typeof summary.checkpointTurnCount returns a spread-updated summary instead of Object.assign.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/filesystemCheckpointStore.ts`:
- Around line 164-173: The diffCheckpoints call currently returns result.stdout
from this.runGit without checking result.stdoutTruncated so large diffs can be
silently truncated; update the code in the method that calls this.runGit (and
the similar usages around the 244-278 region) to inspect the returned result
object for stdoutTruncated and handle it explicitly—either throw a descriptive
error (e.g., "git diff output truncated") or return a structured response
indicating truncation (e.g., { truncated: true, partialOutput: result.stdout })
so callers can fail fast or retry; ensure you reference the runGit return shape
and propagate the truncation signal instead of blindly returning result.stdout.
In `@apps/server/src/providerManager.ts`:
- Around line 302-372: revertToCheckpoint currently proceeds with a thread
rollback even when getOrInitializeFilesystemCheckpointCwd returns null, risking
desync between workspace files and conversation state; update revertToCheckpoint
to fail fast: after computing input and currentTurnCount, if input.turnCount > 0
ensure checkpointCwd is non-null and that
filesystemCheckpointStore.hasCheckpoint(...) returns true (throw a clear Error
if not) before calling this.codex.rollbackThread or
filesystemCheckpointStore.restoreCheckpoint, and only perform the
rollback/restore sequence once filesystem availability is validated; reference
functions: revertToCheckpoint, getOrInitializeFilesystemCheckpointCwd,
filesystemCheckpointStore.hasCheckpoint,
filesystemCheckpointStore.restoreCheckpoint, and this.codex.rollbackThread.
In `@apps/web/src/components/ChatView.tsx`:
- Around line 316-335: The current Promise.all call in the checkpoint hydration
(iterating requestedSummaries and calling api.providers.getCheckpointDiff using
activeSessionId and inferredCheckpointTurnCountByTurnId) will reject the whole
batch if any request fails; change the batching to use Promise.allSettled (or
wrap each await in a try/catch) so you collect only fulfilled entries, filter to
result.status === "fulfilled" and extract the [turnId, diff] tuples, then
dispatch the SET_THREAD_TURN_CHECKPOINT_DIFFS action with Object.fromEntries of
those fulfilled entries (still honoring the cancelled check and using
activeThreadId) so partial successes are applied instead of being dropped.
In `@apps/web/src/components/DiffPanel.tsx`:
- Around line 99-103: The effect that runs on thread change currently resets
checkpoint diff and error but misses clearing the loading flag; update the
useEffect watching activeThread?.id (the one that calls setCheckpointDiffByKey
and setCheckpointDiffError) to also reset the loading state by calling the
setter for isLoadingCheckpointDiff (e.g., setIsLoadingCheckpointDiff(false)) so
any in-flight fetch spinner is cleared when switching threads.
In `@apps/web/src/session-logic.ts`:
- Around line 443-607: deriveTurnDiffSummaries currently only records
completedAt/status/assistantMessageId and therefore drops diff data; update
deriveTurnDiffSummaries (and the MutableTurnDiffSummary shape created in
ensureSummary) to capture and merge diff payloads from events with method
"turn/diff/updated" and from item payloads that represent file-change data so
summaries always include files and unifiedDiff. Specifically: add unifiedDiff?:
string and files?: TurnDiffFileChange[] to MutableTurnDiffSummary, and in the
event reduction loop handle event.method === "turn/diff/updated" by extracting
event.payload.unifiedDiff and event.payload.files (or file change items) and
merging them into summary.unifiedDiff and summary.files (prefer newer/non-empty
values and append/merge file entries rather than overwrite blindly); also detect
file-change item payloads inside "item/..." events (use asObject/asString
helpers and the existing item parsing used for assistantMessageId) and merge
those file changes into the same summary.files/unifiedDiff so the derived
TurnDiffSummary pushed at the end contains populated files and unifiedDiff
fields.
---
Duplicate comments:
In `@apps/web/src/session-logic.ts`:
- Around line 574-593: The loop over ordered (newest-first via
eventTurnId/ensureSummary) is overwriting newer summary fields with older
events; change assignments for summary.completedAt, summary.status and
summary.assistantMessageId so they only set when the field is not already
populated (e.g., check if summary.completedAt/status/assistantMessageId is falsy
before assigning) while keeping the existing event.method branches
(turn/completed and item/completed) and use normalizeDetail/asString as
currently used to compute values.
---
Nitpick comments:
In `@apps/server/src/providerManager.test.ts`:
- Around line 152-180: The test's cast on the manager is misleading because the
single variable named `codex` is given a composite type that includes both
`codex` methods and `filesystemCheckpointStore` but you only assign the codex
methods to it; split the casts and variables for clarity by extracting two
separate typed views from `manager` (e.g., `const codex = (manager as unknown as
{ codex: { ... } }).codex;` and `const internals = (manager as unknown as {
filesystemCheckpointStore: { ... } }).filesystemCheckpointStore;`) or rename the
existing variable to `internals` where appropriate so `codex` only contains
codex methods and `internals` contains `filesystemCheckpointStore`; update
usages of `codex`/`internals` accordingly in the test (references:
ProviderManager, codex, filesystemCheckpointStore, internals, hasSession,
readThread, rollbackThread, listSessions, isGitRepository,
ensureRootCheckpoint).
In `@apps/web/src/store.ts`:
- Around line 253-259: Replace the Object.assign usage inside the return of the
sorted.map callback with spread syntax to match project style: where the code
currently uses Object.assign({}, summary, { checkpointTurnCount:
inferredTurnCountByTurnId[summary.turnId] }), change it to use { ...summary,
checkpointTurnCount: inferredTurnCountByTurnId[summary.turnId] } so the map
callback that checks typeof summary.checkpointTurnCount returns a spread-updated
summary instead of Object.assign.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/components/ChatView.tsx (1)
1861-1911:⚠️ Potential issue | 🟠 MajorAdd an accessible label for the icon-only revert button.
Screen readers get no accessible name here. Addaria-label(and optionallytitle) for the Undo icon button.♿ Proposed fix
<Button type="button" size="xs" variant="outline" disabled={isRevertingCheckpoint || isWorking} onClick={() => onRevertUserMessage(timelineEntry.message.id)} + aria-label="Revert agent work before this message" > <Undo2Icon className="size-3" /> </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ChatView.tsx` around lines 1861 - 1911, The Undo icon-only Button in the ChatView component lacks an accessible name; update the Button rendered when canRevertAgentWork is true (the Button wrapping Undo2Icon) to include an aria-label (e.g., aria-label="Revert agent work" or similar) and optionally a title attribute so screen readers and tooltip users receive a clear description; ensure this change is applied to the Button that uses onClick={() => onRevertUserMessage(timelineEntry.message.id)} and keep existing disabled, type, size, and variant props unchanged.
🧹 Nitpick comments (3)
apps/web/src/components/DiffPanel.tsx (2)
172-249: Complex fallback chain — consider adding inline documentation.The
selectedPatchderivation has multiple layers of fallback logic (checkpoint diff → unifiedDiff → file patches → conversation deduplication → reversed summaries). While the logic appears correct, a brief inline comment explaining the priority order would aid future maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/DiffPanel.tsx` around lines 172 - 249, The fallback chain in selectedPatch (inside the useMemo and the nested helper patchForSummary) is complex and needs a concise inline comment describing the priority order; add a short top-of-block comment above the patchForSummary definition summarizing the resolution sequence (1. selectedTurnCheckpointDiff, 2. per-turn checkpoint diff via patchForSummary using checkpointDiffByKey, 3. summary.unifiedDiff, 4. aggregated file patches, 5. conversationCheckpointDiff, 6. deduplicated latest patch per file path, 7. reversed summaries fallback) and note that patchForSummary itself prefers checkpoint diffs then unifiedDiff then file patches—this will make future maintenance easier without changing logic in selectedPatch, patchForSummary, turnDiffSummaries, inferredCheckpointTurnCountByTurnId, checkpointDiffByKey, or selectedTurnCheckpointDiff.
280-300: Add runtime validation for the checkpoint diff response using the Zod schema frompackages/contracts.The
result.difffromapi.providers.getCheckpointDiff()is used directly without validation. AproviderGetCheckpointDiffResultSchemaexists inpackages/contracts/src/provider.tsand should be used to validate the response, as per coding guidelines requiring Zod schemas frompackages/contractsfor shared type contracts in theapps/directory.Consider validating the response with
providerGetCheckpointDiffResultSchema.parse(result)before usingresult.diff. This pattern should also be applied inChatView.tsxwhere the same method is called.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/DiffPanel.tsx` around lines 280 - 300, Validate the API response from api.providers.getCheckpointDiff using the Zod schema providerGetCheckpointDiffResultSchema from packages/contracts before using result.diff: wrap the existing .then handler in a try/catch (or validate synchronously) and call providerGetCheckpointDiffResultSchema.parse(result); if parse succeeds, proceed to call setCheckpointDiffByKey and dispatch the SET_THREAD_TURN_CHECKPOINT_DIFFS update (referencing setCheckpointDiffByKey and the dispatch block in DiffPanel.tsx); if parse fails, log/handle the schema error and do not apply the invalid diff. Apply the same parse-and-guard pattern to the identical getCheckpointDiff usage in ChatView.tsx.apps/web/src/components/ui/toggle.tsx (1)
1-46: Consider placing this shared Toggle primitive inpackages/ui.
If apps/web is expected to source shared UI primitives frompackages/ui, move this component there and re-export it for app usage.Based on learnings: Use packages/ui components - import UI components from the packages/ui package.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ui/toggle.tsx` around lines 1 - 46, The Toggle component (Toggle function, toggleVariants const, and its dependency TogglePrimitive and cn) is a shared UI primitive that should be moved from apps/web to the monorepo UI package: create a new file in packages/ui (e.g., export path index) containing the Toggle component, toggleVariants, and necessary imports (TogglePrimitive, cn, cva, VariantProps) and update the packages/ui barrel to re-export them; then update all imports in apps/web to import { Toggle, toggleVariants } from "packages/ui" (or the package name) and ensure the packages/ui package build/tsconfig/exports are updated so the new component is published to the consuming app.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/DiffPanel.tsx`:
- Around line 410-422: The onValueChange handler for ToggleGroup can receive an
empty array, making value[0] undefined and producing an invalid DiffRenderMode;
update the handler in the ToggleGroup (the onValueChange that calls
setDiffRenderMode) to guard against empty selection by checking if value.length
> 0 and only calling setDiffRenderMode((value[0] as DiffRenderMode)) when
present, otherwise keep the current diffRenderMode (or set a safe default),
ensuring ToggleGroup/Toggle behavior remains stable.
In `@apps/web/src/components/ui/toggle-group.tsx`:
- Around line 52-74: The Toggle component always prefers context values so
per-item prop overrides are ignored; update the resolution in Toggle (use
ToggleGroupContext and incoming props) to be prop-first, e.g. compute
resolvedVariant/resolvedSize by preferring the local prop (variant, size) and
falling back to context.variant/context.size, and then use those resolved values
when passing data-size, data-variant, size and variant to ToggleComponent;
reference the Toggle function, ToggleGroupContext, and
resolvedVariant/resolvedSize when making the change.
---
Outside diff comments:
In `@apps/web/src/components/ChatView.tsx`:
- Around line 1861-1911: The Undo icon-only Button in the ChatView component
lacks an accessible name; update the Button rendered when canRevertAgentWork is
true (the Button wrapping Undo2Icon) to include an aria-label (e.g.,
aria-label="Revert agent work" or similar) and optionally a title attribute so
screen readers and tooltip users receive a clear description; ensure this change
is applied to the Button that uses onClick={() =>
onRevertUserMessage(timelineEntry.message.id)} and keep existing disabled, type,
size, and variant props unchanged.
---
Duplicate comments:
In `@apps/web/src/components/ChatView.tsx`:
- Around line 299-359: The current Promise.all call drops all diffs if any
request fails; replace Promise.all with Promise.allSettled over
requestedSummaries.map(async (summary) => { ...
api.providers.getCheckpointDiff(...) ... return [summary.turnId, result.diff] as
const; }), then in the .then handler iterate settled results to collect only
those with status "fulfilled" (extract their value entries), build
checkpointDiffByTurnId from those entries and dispatch the existing
"SET_THREAD_TURN_CHECKPOINT_DIFFS" action only if there are any successful
entries; keep the cancelled guard, the .catch/.finally cleanup that deletes keys
from checkpointDiffRequestsRef.current, and preserve the cancelled return in the
cleanup function.
In `@apps/web/src/components/DiffPanel.tsx`:
- Around line 100-103: The effect that resets checkpoint state on thread change
(useEffect watching activeThread?.id) currently calls setCheckpointDiffByKey({})
and setCheckpointDiffError(null) but doesn't clear the loading flag; add a call
to setIsLoadingCheckpointDiff(false) inside that effect so any in-flight fetch
won't leave the spinner stuck; update the useEffect that references
setCheckpointDiffByKey, setCheckpointDiffError, and activeThread?.id to also
call setIsLoadingCheckpointDiff(false).
---
Nitpick comments:
In `@apps/web/src/components/DiffPanel.tsx`:
- Around line 172-249: The fallback chain in selectedPatch (inside the useMemo
and the nested helper patchForSummary) is complex and needs a concise inline
comment describing the priority order; add a short top-of-block comment above
the patchForSummary definition summarizing the resolution sequence (1.
selectedTurnCheckpointDiff, 2. per-turn checkpoint diff via patchForSummary
using checkpointDiffByKey, 3. summary.unifiedDiff, 4. aggregated file patches,
5. conversationCheckpointDiff, 6. deduplicated latest patch per file path, 7.
reversed summaries fallback) and note that patchForSummary itself prefers
checkpoint diffs then unifiedDiff then file patches—this will make future
maintenance easier without changing logic in selectedPatch, patchForSummary,
turnDiffSummaries, inferredCheckpointTurnCountByTurnId, checkpointDiffByKey, or
selectedTurnCheckpointDiff.
- Around line 280-300: Validate the API response from
api.providers.getCheckpointDiff using the Zod schema
providerGetCheckpointDiffResultSchema from packages/contracts before using
result.diff: wrap the existing .then handler in a try/catch (or validate
synchronously) and call providerGetCheckpointDiffResultSchema.parse(result); if
parse succeeds, proceed to call setCheckpointDiffByKey and dispatch the
SET_THREAD_TURN_CHECKPOINT_DIFFS update (referencing setCheckpointDiffByKey and
the dispatch block in DiffPanel.tsx); if parse fails, log/handle the schema
error and do not apply the invalid diff. Apply the same parse-and-guard pattern
to the identical getCheckpointDiff usage in ChatView.tsx.
In `@apps/web/src/components/ui/toggle.tsx`:
- Around line 1-46: The Toggle component (Toggle function, toggleVariants const,
and its dependency TogglePrimitive and cn) is a shared UI primitive that should
be moved from apps/web to the monorepo UI package: create a new file in
packages/ui (e.g., export path index) containing the Toggle component,
toggleVariants, and necessary imports (TogglePrimitive, cn, cva, VariantProps)
and update the packages/ui barrel to re-export them; then update all imports in
apps/web to import { Toggle, toggleVariants } from "packages/ui" (or the package
name) and ensure the packages/ui package build/tsconfig/exports are updated so
the new component is published to the consuming app.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/components/ChatView.tsx (1)
1946-2046: Consider extracting the inline IIFE for improved readability.The nested IIFE pattern
(() => {...})()for rendering the turn diff summary works but impacts readability. Consider extracting this to a helper component or moving the logic outside the JSX.♻️ Suggested extraction
+const TurnDiffSummarySection = memo(function TurnDiffSummarySection({ + turnSummary, + onOpenTurnDiff, +}: { + turnSummary: TurnDiffSummary; + onOpenTurnDiff: (turnId: string, filePath?: string) => void; +}) { + const isCheckpointDiffLoading = + !turnSummary.checkpointDiffLoaded && turnSummary.files.length === 0; + const summaryStat = useMemo(() => { + if (turnSummary.unifiedDiff) { + return countDiffStat(turnSummary.unifiedDiff); + } + return turnSummary.files.reduce( + (acc, file) => { + const next = + typeof file.additions === "number" && typeof file.deletions === "number" + ? { additions: file.additions, deletions: file.deletions } + : file.diff + ? countDiffStat(file.diff) + : null; + if (!next) return acc; + return { + additions: acc.additions + next.additions, + deletions: acc.deletions + next.deletions, + }; + }, + { additions: 0, deletions: 0 }, + ); + }, [turnSummary.unifiedDiff, turnSummary.files]); + // ... rest of component +});Then use
<TurnDiffSummarySection turnSummary={...} onOpenTurnDiff={...} />in the render.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ChatView.tsx` around lines 1946 - 2046, Extract the large inline IIFE rendering the turn diff UI into a new React component (e.g., TurnDiffSummarySection) that accepts props {turnSummary, onOpenTurnDiff}, moving logic that references turnDiffSummaryByAssistantMessageId, countDiffStat and the local computed values (isCheckpointDiffLoading, summaryStat, changedFileCountLabel) into that component; then replace the IIFE in ChatView.tsx with a simple <TurnDiffSummarySection turnSummary={turnSummary} onOpenTurnDiff={onOpenTurnDiff} /> (or null when no turnSummary) to improve readability and keep behavior identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/web/src/components/ChatView.tsx`:
- Around line 320-347: The batch fetch for checkpoint diffs uses Promise.all
which rejects the whole batch on any single getCheckpointDiff failure; change
the logic in the block that iterates requestedSummaries to use
Promise.allSettled over the array of getCheckpointDiff promises, then build
entries only from the fulfilled results (mapping each settled result back to its
summary.turnId), call dispatch with SET_THREAD_TURN_CHECKPOINT_DIFFS using
Object.fromEntries of the successful pairs, and preserve the existing cleanup
that deletes entries from checkpointDiffRequestsRef.current for each summary;
ensure errors from rejected results are ignored per-summary rather than aborting
the whole operation.
---
Nitpick comments:
In `@apps/web/src/components/ChatView.tsx`:
- Around line 1946-2046: Extract the large inline IIFE rendering the turn diff
UI into a new React component (e.g., TurnDiffSummarySection) that accepts props
{turnSummary, onOpenTurnDiff}, moving logic that references
turnDiffSummaryByAssistantMessageId, countDiffStat and the local computed values
(isCheckpointDiffLoading, summaryStat, changedFileCountLabel) into that
component; then replace the IIFE in ChatView.tsx with a simple
<TurnDiffSummarySection turnSummary={turnSummary}
onOpenTurnDiff={onOpenTurnDiff} /> (or null when no turnSummary) to improve
readability and keep behavior identical.
This comment has been minimized.
This comment has been minimized.
- add provider/WS contracts and server RPC handlers for listing/reverting checkpoints - read and rollback Codex thread turns in the app server manager - add ChatView checkpoint menu + store reducer support to revert local thread state - expand backend, websocket, store, and contract tests for checkpoint flows
- add `FilesystemCheckpointStore` to capture, restore, and prune per-turn git refs - wire checkpoint init/capture into `ProviderManager` with per-session locking and error events - require checkpoint presence before rollback and restore workspace state after revert - add tests for checkpoint store behavior and provider rollback integration
- Replace placeholder diff sidebar with real turn/file patch rendering - Link assistant messages to changed-file summaries and open targeted diffs - Persist diff selection state in the store and cover parsing/reducer behavior with tests - Lazy-load `DiffPanel` to reduce initial bundle cost
- add server support to diff filesystem checkpoints, including root/HEAD fallbacks and lazy checkpoint initialization - wire new `providers.getCheckpointDiff` WebSocket route through provider manager and contracts - update web app to load per-turn diffs from checkpoints and improve diff panel behavior on narrow screens
- add reusable `ui/toggle` and `ui/toggle-group` components built on Base UI - switch diff view mode controls and chat header diff toggle to use the new toggle primitives
- Replace "Revert agent work" button text with `Undo2Icon` in `ChatView` - Keep existing revert behavior and disabled/loading states unchanged
- Add a confirmation dialog before checkpoint revert - Warn that newer messages and turn diffs will be discarded - Cancel revert when the user does not confirm
- add shared provider checkpoint diff query options and keys - refactor `ChatView` and `DiffPanel` to fetch diffs via React Query - persist selected-turn checkpoint diffs back into thread state for reuse
juliusmarminge
force-pushed
the
codething/ecb6d645
branch
from
February 18, 2026 09:12
65d6d4c to
8307207
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/src/components/DiffPanel.tsx (1)
70-418: Consider splitting DiffPanel into focused subcomponents.
It now mixes data fetching, selection controls, and patch rendering in one unit; extracting header/selector and patch viewport pieces will improve maintainability.
Based on learnings: Applies to **/*.tsx : Extract large React components into multiple subcomponents with granular functionality. Co-locate subcomponents in the same file as the main component. Avoid hoisting callbacks too high up the component tree; prefer colocating logic close to JSX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/DiffPanel.tsx` around lines 70 - 418, DiffPanel is doing too much — mix of data fetching, selection header, and patch rendering — so extract focused subcomponents in the same file: create a Header/Selector subcomponent (e.g., DiffPanelHeader) that receives turnDiffSummaries, selectedTurnId, selectTurn, selectWholeConversation, diffRenderMode, and setDiffRenderMode (keep shouldUseDragRegion logic nearby) and render the turn buttons and ToggleGroup; and create a PatchViewport subcomponent (e.g., DiffPanelViewport) that receives patchViewportRef, renderablePatch, renderableFiles, selectedFilePath, isLoadingCheckpointDiff, checkpointDiffError and contains the scroll-into-view useEffect and the FileDiff rendering. Move any memoized logic or callbacks that are only used by a subcomponent (for example the renderableFiles memo or the selectTurn/selectWholeConversation callbacks) into that subcomponent to colocate logic with JSX while keeping data-fetching and state (activeThread, queries, selectedPatch) in DiffPanel; wire props between DiffPanel and the two new subcomponents and keep all components in the same file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/ChatView.tsx`:
- Around line 162-163: Replace the manual pending boolean pattern
(isRevertingCheckpoint / setIsRevertingCheckpoint) with React transition-safe
updates: use useTransition to get startTransition and isPending (or adopt
useActionState for async submits), and ensure any state updates or dispatch()
calls that occur after an await are wrapped inside startTransition (or invoked
via useActionState) so they are treated as transition updates; specifically,
update the revert-checkpoint flow that currently toggles
setIsRevertingCheckpoint around the async call to instead call
startTransition(() => { /* dispatch or set state updates that must be deferred
*/ }) for all post-await updates, and apply the same change to the other async
pending handlers that manage pending state and dispatches (the blocks
referencing setIsRevertingCheckpoint/selectedEffort/dispatch and the async
handlers in the other pending-state regions).
In `@apps/web/src/lib/providerReactQuery.ts`:
- Around line 28-50: hasValidRange currently only checks types and allows
negative or reversed ranges; update the validation to ensure fromTurnCount and
toTurnCount are numbers, non-negative, and fromTurnCount <= toTurnCount (for
example: const hasValidRange = typeof input.fromTurnCount === "number" && typeof
input.toTurnCount === "number" && input.fromTurnCount >= 0 &&
input.fromTurnCount <= input.toTurnCount). Use this updated hasValidRange in the
enabled flag and in queryFn pre-checks (replace the existing typeof re-checks)
and throw a clear error (e.g., "Checkpoint diff range is invalid or out of
order.") before calling api.providers.getCheckpointDiff with sessionId,
fromTurnCount, toTurnCount.
---
Duplicate comments:
In `@apps/server/src/filesystemCheckpointStore.ts`:
- Around line 178-187: diffCheckpoints currently returns result.stdout from
runGit which uses outputMode: "truncate", so large diffs can be silently
truncated; update diffCheckpoints (and callers if needed) to check
result.stdoutTruncated (or equivalent flag returned by runGit) and either throw
a descriptive error (e.g., "git diff output truncated") or return a structured
response {stdout, truncated: true} so callers can handle truncation; locate
runGit and diffCheckpoints by name in filesystemCheckpointStore.ts and ensure
the change preserves existing behavior for non-truncated results while making
truncation explicit.
In `@apps/server/src/providerManager.ts`:
- Around line 302-372: The revertToCheckpoint flow allows thread rollback when
checkpointCwd is null, leaving files out of sync; update revertToCheckpoint so
that after computing input and checkpointCwd (via
getOrInitializeFilesystemCheckpointCwd) you fail fast when checkpointCwd is null
and input.turnCount > 0 by throwing an error indicating filesystem checkpoints
are unavailable for the requested turn; ensure this check occurs before calling
rollbackThread so filesystem restoration
(filesystemCheckpointStore.restoreCheckpoint / pruneAfterTurn) isn't skipped;
reference functions/vars: revertToCheckpoint, checkpointCwd,
getOrInitializeFilesystemCheckpointCwd, filesystemCheckpointStore,
rollbackThread, and input.turnCount.
In `@apps/web/src/components/DiffPanel.tsx`:
- Around line 349-354: The onValueChange handler for ToggleGroup can receive an
empty array causing value[0] to be undefined; update the handler used with
ToggleGroup (the onValueChange callback that calls setDiffRenderMode) to guard
against empty selections by checking value.length and only calling
setDiffRenderMode with value[0] when present, otherwise keep the existing
diffRenderMode (or a safe default) to avoid setting an invalid mode.
---
Nitpick comments:
In `@apps/web/src/components/DiffPanel.tsx`:
- Around line 70-418: DiffPanel is doing too much — mix of data fetching,
selection header, and patch rendering — so extract focused subcomponents in the
same file: create a Header/Selector subcomponent (e.g., DiffPanelHeader) that
receives turnDiffSummaries, selectedTurnId, selectTurn, selectWholeConversation,
diffRenderMode, and setDiffRenderMode (keep shouldUseDragRegion logic nearby)
and render the turn buttons and ToggleGroup; and create a PatchViewport
subcomponent (e.g., DiffPanelViewport) that receives patchViewportRef,
renderablePatch, renderableFiles, selectedFilePath, isLoadingCheckpointDiff,
checkpointDiffError and contains the scroll-into-view useEffect and the FileDiff
rendering. Move any memoized logic or callbacks that are only used by a
subcomponent (for example the renderableFiles memo or the
selectTurn/selectWholeConversation callbacks) into that subcomponent to colocate
logic with JSX while keeping data-fetching and state (activeThread, queries,
selectedPatch) in DiffPanel; wire props between DiffPanel and the two new
subcomponents and keep all components in the same file.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: codex <codex@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
- avoid git reset when repository has no HEAD\n- use React Query isLoading for checkpoint diff spinner\n- remove unreachable provider manager guard\n- add no-HEAD restore regression test\n\nCo-authored-by: codex <codex@users.noreply.github.com>
| : (t.codexThreadId ?? eventThreadId ?? null), | ||
| error: event.kind === "error" && event.message ? event.message : t.error, | ||
| session: t.session ? evolveSession(t.session, event) : t.session, | ||
| messages: applyEventToMessages(t.messages, event, activeAssistantItemRef), |
Contributor
There was a problem hiding this comment.
🟢 Low
src/store.ts:945 Mutating activeAssistantItemRef.current inside the reducer violates React's purity requirements—reducers may run multiple times in Strict Mode or concurrent rendering. Consider returning the updated ref value from applyEventToMessages and storing it in state, or move the ref mutation to an effect.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/web/src/store.ts around line 945:
Mutating `activeAssistantItemRef.current` inside the reducer violates React's purity requirements—reducers may run multiple times in Strict Mode or concurrent rendering. Consider returning the updated ref value from `applyEventToMessages` and storing it in state, or move the ref mutation to an effect.
Evidence trail:
Viewed `apps/web/src/store.ts:932-952` at `832eb42` showing reducer calls `applyEventToMessages(..., activeAssistantItemRef)`. Viewed `apps/web/src/session-logic.ts:856-957` at `832eb42` showing `activeAssistantItemRef.current = ...` mutations inside `applyEventToMessages`.
This comment has been minimized.
This comment has been minimized.
- Remove `checkpointDiffLoaded` from hydration and persistence paths - Update persistence schema tests to assert loaded flags are cleared
| files: z.array(persistedTurnDiffFileChangeSchema), | ||
| assistantMessageId: z.string().min(1).optional(), | ||
| checkpointTurnCount: z.number().int().min(0).optional(), | ||
| checkpointDiffLoaded: z.boolean().optional(), |
Contributor
There was a problem hiding this comment.
🟢 Low
src/persistenceSchema.ts:64 checkpointDiffLoaded is defined in the schema but never mapped in hydrateThread (lines 294-308) or toPersistedState (lines 389-403). Consider adding the mapping logic for this field, or removing it from the schema if unused.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/web/src/persistenceSchema.ts around line 64:
`checkpointDiffLoaded` is defined in the schema but never mapped in `hydrateThread` (lines 294-308) or `toPersistedState` (lines 389-403). Consider adding the mapping logic for this field, or removing it from the schema if unused.
Evidence trail:
Viewed `apps/web/src/persistenceSchema.ts` around persistedTurnDiffSummarySchema definition and `hydrateThread`/`toPersistedState` mappings at commit `a2e4c71` (lines ~50-110, ~300-360, ~360-400).
| cacheScope: `turn:${target.turnId}`, | ||
| }), | ||
| ), | ||
| }); |
Contributor
There was a problem hiding this comment.
Checkpoint hydration queries fire even when diff panel closed
Medium Severity
checkpointDiffHydrationTargets generates fetch targets for every turn summary without checkpointDiffLoaded, and useQueries fires all of them — regardless of whether the diff panel is open. After persistence hydration strips checkpointDiffLoaded, every summary qualifies, triggering a burst of checkpoint diff API calls (each with retry: 8) for data the user isn't viewing. For threads with many turns, this generates significant unnecessary network traffic and server load.
| } | ||
|
|
||
| return byPath; | ||
| } |
Contributor
There was a problem hiding this comment.
Exported splitUnifiedDiffByFile has no external consumers
Low Severity
splitUnifiedDiffByFile is exported but never imported outside session-logic.ts. It is only called internally by deriveTurnDiffFilesFromUnifiedDiff in the same file. The unnecessary export enlarges the public API surface and may mislead future developers into thinking it's a stable, externally consumed utility.
This comment has been minimized.
This comment has been minimized.
- Wrap `DiffPanel` in `WorkerPoolContextProvider` and add a dedicated diffs worker - Use content-based patch cache keys to stabilize `parsePatchFiles` caching - Add tests for cache key stability, whitespace normalization, and content changes
Comment on lines
+396
to
+397
| ...(typeof file.additions === "number" ? { additions: file.additions } : {}), | ||
| ...(typeof file.deletions === "number" ? { deletions: file.deletions } : {}), |
Contributor
There was a problem hiding this comment.
🟢 Low
src/persistenceSchema.ts:396 Consider adding Number.isFinite() and Number.isInteger() guards for additions, deletions, and checkpointTurnCount before serializing. Currently, NaN/Infinity/floats would serialize but fail validation on rehydration, causing the entire turnDiffSummaries array to be discarded due to .catch([]).
- ...(typeof file.additions === "number" ? { additions: file.additions } : {}),
- ...(typeof file.deletions === "number" ? { deletions: file.deletions } : {}),
+ ...(Number.isInteger(file.additions) ? { additions: file.additions } : {}),
+ ...(Number.isInteger(file.deletions) ? { deletions: file.deletions } : {}),🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/web/src/persistenceSchema.ts around lines 396-397:
Consider adding `Number.isFinite()` and `Number.isInteger()` guards for `additions`, `deletions`, and `checkpointTurnCount` before serializing. Currently, `NaN`/`Infinity`/floats would serialize but fail validation on rehydration, causing the entire `turnDiffSummaries` array to be discarded due to `.catch([])`.
Evidence trail:
Viewed `apps/web/src/persistenceSchema.ts:33-88` (persistedTurnDiffFileChangeSchema, persistedTurnDiffSummarySchema, turnDiffSummaries .catch([])) at commit `ba46a1b9`. Viewed `apps/web/src/persistenceSchema.ts:372-405` (toPersistedState mapping with typeof number checks) at commit `ba46a1b9`.
| mediaQueryList.addEventListener("change", handleChange); | ||
| return () => { | ||
| mediaQueryList.removeEventListener("change", handleChange); | ||
| }; |
Contributor
There was a problem hiding this comment.
MediaQuery listener breaks in older browsers
Medium Severity
useMediaQuery uses MediaQueryList.addEventListener("change", ...), which is not supported in some older Safari/WebKit environments where only addListener exists. In those runtimes the hook throws during effect setup, breaking App layout rendering when the diff sheet logic mounts.
This comment has been minimized.
This comment has been minimized.
- Keep the diff viewer alive when hidden to avoid remount churn - Share diff worker pool setup via a dedicated provider - Add `keepMounted` support to `SheetPopup` for sheet-mode persistence
- upgrade `@pierre/diffs` to `^1.1.0-beta.16` - switch file diff list to `Virtualizer` for smoother large patch rendering - adjust diff panel container/layout handling and theme typing
This comment has been minimized.
This comment has been minimized.
Member
Author
- Restore null CWD in sessionOverrides for thread/start and thread/resume so the Codex app server can distinguish 'no override' from an explicit path (e.g., to reuse a resumed thread's original CWD). - Use backreference in diff --git fallback regex to correctly parse paths containing ' b/' (e.g., binary diffs without +++ b/ lines). - Add test exercising the git header fallback for binary diffs with ' b/' in the path. Applied via @cursor push command
| patchViewportRef.current.querySelectorAll<HTMLElement>("[data-diff-file-path]"), | ||
| ).find((element) => element.dataset.diffFilePath === selectedFilePath); | ||
| target?.scrollIntoView({ block: "nearest" }); | ||
| }, [selectedFilePath, renderableFiles]); |
Contributor
There was a problem hiding this comment.
Scroll-to-file silently fails with virtualized rendering
Medium Severity
The useEffect that scrolls to selectedFilePath uses querySelectorAll("[data-diff-file-path]") on the viewport ref to find a DOM element and call scrollIntoView. However, the file diffs are rendered inside a Virtualizer that likely only mounts elements within the visible viewport plus the configured intersectionObserverMargin (1200px). For diffs with many files, the target element may not exist in the DOM if it falls outside the virtualization window, causing the scroll to silently fail and leaving the user unable to navigate to the file they clicked.
Additional Locations (1)
Contributor
There was a problem hiding this comment.
Bugbot Autofix determined this is a false positive.
The @pierre/diffs Virtualizer renders all children to the DOM as normal React elements; virtualization only controls the internal rendering of each FileDiff component via IntersectionObserver visibility, so the wrapper divs with data-diff-file-path are always queryable.
| ) | ||
| .map(([, patch]) => patch) | ||
| .join("\n\n"); | ||
| } |
Contributor
There was a problem hiding this comment.
Conversation fallback shows incremental diffs not cumulative
Medium Severity
In the selectedPatch memo's "All turns" fallback path, latestPatchByPath keeps only the most recent turn's per-file diff for each path. Because per-turn diffs are incremental (relative to the previous turn, not the original state), the combined view can be misleading when a file is modified across multiple turns — the user sees only the latest turn's delta rather than the cumulative change from the conversation start. This persists until the full conversation checkpoint diff loads.
| `Filesystem checkpoint is unavailable for turn ${input.turnCount} in thread ${beforeSnapshot.threadId}.`, | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Redundant truthiness check on already-validated variable
Low Severity
The guard if (checkpointCwd && input.turnCount > 0) at line 323 includes a redundant check on checkpointCwd. This variable is guaranteed to be truthy at that point because lines 310–313 already throw if getOrInitializeFilesystemCheckpointCwd returned a falsy value. The checkpointCwd && portion is dead code that adds confusion about whether a null path could reach this block.
Contributor
|
Bugbot Autofix prepared fixes for 2 of the 3 bugs found in the latest run.
Or push these changes by commenting: Preview (db3b9e0807)diff --git a/apps/server/src/providerManager.ts b/apps/server/src/providerManager.ts
--- a/apps/server/src/providerManager.ts
+++ b/apps/server/src/providerManager.ts
@@ -320,7 +320,7 @@
);
}
- if (checkpointCwd && input.turnCount > 0) {
+ if (input.turnCount > 0) {
const hasCheckpoint = await this.filesystemCheckpointStore.hasCheckpoint({
cwd: checkpointCwd,
threadId: beforeSnapshot.threadId,
diff --git a/apps/web/src/components/DiffPanel.tsx b/apps/web/src/components/DiffPanel.tsx
--- a/apps/web/src/components/DiffPanel.tsx
+++ b/apps/web/src/components/DiffPanel.tsx
@@ -241,29 +241,8 @@
}
// Fallback when a conversation checkpoint diff isn't available yet:
- // keep one patch per file path (latest change wins) so files aren't duplicated.
- const latestPatchByPath = new Map<string, string>();
- for (const summary of turnDiffSummaries) {
- for (const file of summary.files) {
- if (latestPatchByPath.has(file.path)) {
- continue;
- }
- const patch = file.diff?.trim();
- if (!patch) {
- continue;
- }
- latestPatchByPath.set(file.path, patch);
- }
- }
- if (latestPatchByPath.size > 0) {
- return Array.from(latestPatchByPath.entries())
- .toSorted(([leftPath], [rightPath]) =>
- leftPath.localeCompare(rightPath, undefined, { numeric: true, sensitivity: "base" }),
- )
- .map(([, patch]) => patch)
- .join("\n\n");
- }
-
+ // show each turn's diff (oldest first) via patchForSummary, which prefers
+ // cached checkpoint diffs when available.
const patches = turnDiffSummaries
.toReversed()
.map((summary) => patchForSummary(summary)?.trim()) |
aorwall
added a commit
to aorwall/t3code
that referenced
this pull request
Aug 12, 2026
…uracy (pingdotgg#64) * fix: close three fork gaps — Effect rules, stale unions, register accuracy Satisfy the two Effect rules the fork was suppressing with `@effect-diagnostics-next-line`: - `globalFetch` in the orval-generated Moatless client is answered where apps/web already answers the same rule for the same reason: a package-level `diagnosticSeverity` override in tsconfig. `HttpClient` would put an Effect runtime and a layer between a generated call and the request it makes — a per-package decision, not a per-line one. - `globalErrorInEffectCatch` is answered by making the channel tagged. The two failures a Moatless query can carry were already distinct `_tag` classes; `MoatlessError` names them as a union, `MoatlessUnexpectedError` gives the defensive branch somewhere tagged to land, and `asMoatlessError` replaces the local `toError`. Readers are unaffected: all extend `Error`. Drop the `UnsupportedMethodError` union member from eleven methods the deployed backend serves and never refuses — the seven `terminal.*`, `subscribeTerminalEvents`, `subscribeTerminalMetadata`, `git.runStackedAction`, `git.resolvePullRequest`. Verified: deployed serverVersion 0.0.31 matches the checkout, `main` dispatches all eleven with no `unsupported_exit` in their arms, and terminals work in the deployed app. `vcs.switchRef` and `git.preparePullRequestThread` keep the member — their arm still returns `unsupported_exit` on a real branch, so a client that dropped it would fail to decode a refusal it will receive. Reconcile docs/fork/gaps.md: correct the capabilities entry (the backend does report a subset; three thread-lifecycle booleans and two contract-registered keys are what is missing), rewrite the union entry to the two that remain, strike the Effect-rules entry, and add a Moatless entry for the subagent identity the Agents surface folds on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(fork): a verified procedure for testing the web client on Moatless The bundled-server-and-pairing skills describe a stack this fork's web client does not talk to, so an agent following them set up the wrong thing. test-moatless-web is the fork-owned replacement for the web case: proxy target, single-origin mode, and Moatless cookie sign-in, each checked against a running backend. The two upstream skills keep everything but a scope note routing web work here. Rewriting them would be a standing conflict on docs upstream still maintains, and buys nothing the note does not. Mobile stays open rather than guessed: whether that client can reach a Moatless backend is unverified, so its note says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
deriveTurnDiffSummaries, combiningfileChangeitems andturn/diff/updatedpayloads into file-level summariesDiffPanelthat supports turn selection, file selection, and rendered patches using@pierre/diffsthreadId,turnId,filePath) and clear targets when threads are removedSuspenseto reduce initial render costTesting
apps/web/src/session-logic.test.ts: verifies per-turn diff aggregation, assistant message linkage, and file extraction from unified diffsapps/web/src/store.test.ts: verifiesOPEN_DIFFstores explicit turn/file targets and existing reducer behavior remains intactNote
Medium Risk
Touches core session lifecycle and introduces git-based filesystem operations plus new provider RPCs, which could impact correctness/performance and workspace state if edge cases slip through, though changes are well-covered by tests.
Overview
Adds server-side checkpointing for Codex sessions:
CodexAppServerManagercan nowthread/readandthread/rollback, andProviderManagerexposes new RPCs to list checkpoints, diff checkpoint ranges, and revert to a checkpoint while capturing/pruning git-backed filesystem snapshots via the newFilesystemCheckpointStore.Replaces the web app’s diff placeholder with a functional, lazy-loaded diff UX: derives per-turn diff summaries from provider events, hydrates checkpoint diffs via React Query, renders patches with
@pierre/diffs(split/stacked, file/turn selection, responsive sheet vs inline), persists diff targeting state, and adds timeline actions to view diffs or revert to earlier checkpoints. Extensive unit tests added across server, web state/persistence, and diff derivation/queries.Written by Cursor Bugbot for commit e29a770. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Tests