fix(app): derive review changes from agent edits - #819
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThis PR migrates session diff storage from arrays of snapshot file diffs to a new turn-change aggregation model using a discriminated union type. It replaces ChangesTurn-change aggregation and state migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
github-actions
Bot
added
app
![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/components/prompt-input/comment-routing.ts, packages/app/src/context/global-sync/bootstrap.test.ts, packages/app/src/context/global-sync/child-store.ts, packages/app/src/context/global-sync/event-reducer.test.ts, packages/app/src/context/global-sync/event-reducer.ts, packages/app/src/context/global-sync/session-cache.test.ts, packages/app/src/context/global-sync/session-cache.ts, packages/app/src/context/global-sync/types.ts, packages/app/src/context/sync.tsx, packages/app/src/pages/session.tsx, packages/app/src/pages/session/session-turn-changes.tsx, packages/app/src/pages/session/use-session-review-panel.tsx, packages/app/src/pages/session/use-session-review-state.ts, packages/app/src/pages/session/use-session-timeline-data.test.ts, packages/app/src/pages/session/use-session-timeline-data.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 refactors session change tracking by introducing a turn-based aggregation system and a new table to track uncaptured shell activity. The review identifies a critical performance bottleneck caused by an N+1 query pattern during session-level aggregation and a regression where file changes are concatenated rather than consolidated per path. Additionally, the feedback points out fragile parsing logic in the revert summary heuristic and a redundant database query in the turn aggregation implementation.
Perf delta summaryComparator: pass
|
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/src/server/instance/session.ts (1)
685-707:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign route payload with declared aggregate schema.
Line 685 declares
TurnChange.AggregateSchema, but Line 707 can returnnull(result ?? null). That breaks the API contract on the “no data” path and can break typed clients.🤖 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/opencode/src/server/instance/session.ts` around lines 685 - 707, The route declares TurnChange.AggregateSchema but currently returns null on "no data" (result ?? null), breaking the contract; update the handler that calls TurnChange.Service.get (inside AppRuntime.runPromise) to never return null: either change the response to return a proper AggregateSchema-compliant payload when found and return an appropriate HTTP error (e.g., 404) or use c.json(result) only when result is defined and call c.notFound()/c.status(404).json(...) when undefined; do not return null unless you also update TurnChange.AggregateSchema to allow nulls.
🧹 Nitpick comments (3)
packages/app/src/pages/session/use-session-timeline-data.ts (2)
144-149: ⚡ Quick winEffect lacks guard against repeated fetch requests.
This effect will call
sync.session.diff(id)every time any tracked dependency changes whileturn_change_aggregate[id]remainsundefined. If the async fetch is slow or fails silently, this could trigger multiple in-flight requests.Consider adding a loading guard or tracking in-flight requests to prevent redundant calls.
♻️ Proposed guard pattern
+ const diffFetching = new Set<string>() + createEffect(() => { const id = input.routeSessionID() if (!id) return if (input.sync.data.turn_change_aggregate[id] !== undefined) return + if (diffFetching.has(id)) return + diffFetching.add(id) - void input.sync.session.diff(id) + void input.sync.session.diff(id).finally(() => diffFetching.delete(id)) })🤖 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/use-session-timeline-data.ts` around lines 144 - 149, The effect calling createEffect currently triggers input.sync.session.diff(id) repeatedly while input.sync.data.turn_change_aggregate[id] is undefined; add a guard to track in-flight fetches (e.g., a Set or Boolean map) so you only call input.sync.session.diff(id) if there is not already a pending request for that id, mark the id as in-flight before calling diff and clear it on success/failure, and reference the existing symbols createEffect, input.routeSessionID(), input.sync.data.turn_change_aggregate[id], and input.sync.session.diff(id) when implementing the guard.
41-53: ⚡ Quick winDuplicate
aggregateFilesimplementation across files.The
aggregateFileshelper is defined identically in bothuse-session-timeline-data.ts(lines 41-53) anduse-session-review-state.ts(lines 22-34). Consider extracting this to a shared utility to maintain consistency and reduce duplication.♻️ Proposed extraction
Create a shared helper, e.g., in
@/utils/aggregate-files.ts:import type { SessionDiffResponse, SnapshotFileDiff } from "`@opencode-ai/sdk/v2/client`" export function aggregateFiles(aggregate: SessionDiffResponse | undefined): SnapshotFileDiff[] { if (!aggregate) return [] if (aggregate.kind === "empty" || aggregate.kind === "uncaptured") return [] return aggregate.files .filter((file) => file.restoreState === "applied") .map((file) => ({ file: file.openPath ?? file.path, patch: file.patch ?? "", additions: file.additions ?? 0, deletions: file.deletions ?? 0, status: file.status, })) }Then import from both files.
Also applies to: 22-34
🤖 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/use-session-timeline-data.ts` around lines 41 - 53, The aggregateFiles function is duplicated; extract it into a single exported utility named aggregateFiles that accepts a SessionDiffResponse | undefined and returns SnapshotFileDiff[], then replace the local copies in both hooks with imports of that utility; ensure the new utility imports the types SessionDiffResponse and SnapshotFileDiff from the SDK and preserves the existing behavior (return [] for falsy/empty/uncaptured, filter restoreState === "applied", and map file/openPath, patch, additions, deletions, status with defaults).packages/app/src/pages/session/use-session-timeline-data.test.ts (1)
30-31: 💤 Low valueSimplify the
capturedAggregatehelper type annotation.The type
SessionDiffResponse & { kind: "captured" }is narrowed but the function just returns the input. A cleaner approach:-const capturedAggregate = (files: SessionDiffResponse & { kind: "captured" }) => files +const capturedAggregate = (input: { kind: "captured"; sessionID: string; files: ReturnType<typeof appliedAggregateFile>[] }): SessionDiffResponse => inputOr simply inline the object literals in tests since the helper doesn't add much value.
🤖 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/use-session-timeline-data.test.ts` around lines 30 - 31, The helper capturedAggregate unnecessarily annotates its parameter as SessionDiffResponse & { kind: "captured" } while merely returning the input; replace it with a simpler identity signature (e.g., const capturedAggregate = <T extends { kind: "captured" }>(x: T) => x) or remove the helper and inline the object literals in tests where used to keep types inferred and reduce verbosity; update usages of capturedAggregate accordingly (refer to the capturedAggregate helper in use-session-timeline-data.test.ts).
🤖 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/app/src/pages/session/use-session-review-state.ts`:
- Around line 278-292: The createEffect callback is silently swallowing errors
and can compare against a stale captured scope; update the handler in
createEffect to (1) replace the empty .catch(() => {}) with an error handler
that logs or reports the error (include the caught error object and context like
sessionID and scope) instead of swallowing it, and (2) ensure the effect
re-triggers when executionScope changes or avoid stale comparisons by not
capturing scope once — either add input.executionScope() as a dependency to the
effect or re-read executionScope() inside the .then() before calling
shouldApplyExecutionResult; reference the createEffect, scope variable,
input.executionScope(), input.sdk.createClient(...).session.diff, and
shouldApplyExecutionResult symbols when making the change.
In `@packages/opencode/src/session/revert.ts`:
- Around line 49-57: The extraction using lastIndexOf(" b/") in revert.ts misses
quoted diff headers; update the parsing in the function containing
currentFile/files/line (where lastIndexOf(" b/") is used) to use a regex that
matches both quoted and unquoted `b/` paths (e.g. capture group for the path
after b/ allowing an optional surrounding double quote), strip any trailing
quote, trim, then set currentFile and add to files as before; ensure you replace
the lastIndexOf(" b/") branch with this regex-based extraction so paths with
spaces wrapped in quotes are correctly included.
In `@packages/opencode/src/session/turn-change.ts`:
- Around line 924-931: The mutedFiles list can contain duplicate entries for the
same file path because mutedRows may include multiple restore rows for one path;
before mapping/returning mutedFiles (constructed from mutedRows, using
toDisplay, openPath, restoreState and checked against appliedPaths), deduplicate
by row.data.path (or by the resulting displayFile.path) so capturedAggregate
emits only one AggregateFile per path—update the mutedFiles construction to
filter or reduce duplicates (e.g., use a Map or Set keyed by path) prior to the
final .map/.filter step.
In `@packages/opencode/test/session/session-artifacts.test.ts`:
- Around line 95-99: You recorded an uncaptured turn with
TurnChange.recordUncaptured({ sessionID: uncaptured.id, messageID: assistant })
but never finalized it, so the test may not exercise the uncaptured-aggregate
path; after creating the uncaptured change call TurnChange.finalize(...) for
that session/message (using the same sessionID and messageID returned by
makeAssistant) before asserting SessionSummary.artifacts({ sessionID:
uncaptured.id }) to ensure the uncaptured turn is finalized and the
empty-artifacts path is actually tested.
In `@packages/opencode/test/tool/bash.test.ts`:
- Around line 541-543: The test currently sets command to a PowerShell
Set-Content or a fallback `printf` for non-PowerShell shells, which fails when
the active shell is cmd.exe (no printf); change the non-PowerShell branch to
detect cmd-style shells and use `echo hello > <file>` for cmd.exe and keep
`printf 'hello\n' > <file>` for POSIX shells. Locate the assignment to `command`
(the `PS.has(sh()) ? ... : ...` expression) and update the fallback to inspect
sh() or the shell indicator and choose `echo` for cmd and `printf` for others,
using the existing `quote(target.replaceAll("\\", "/"))` and proper escaping.
Ensure the new branch preserves newline behavior and works cross-platform in the
test.
---
Outside diff comments:
In `@packages/opencode/src/server/instance/session.ts`:
- Around line 685-707: The route declares TurnChange.AggregateSchema but
currently returns null on "no data" (result ?? null), breaking the contract;
update the handler that calls TurnChange.Service.get (inside
AppRuntime.runPromise) to never return null: either change the response to
return a proper AggregateSchema-compliant payload when found and return an
appropriate HTTP error (e.g., 404) or use c.json(result) only when result is
defined and call c.notFound()/c.status(404).json(...) when undefined; do not
return null unless you also update TurnChange.AggregateSchema to allow nulls.
---
Nitpick comments:
In `@packages/app/src/pages/session/use-session-timeline-data.test.ts`:
- Around line 30-31: The helper capturedAggregate unnecessarily annotates its
parameter as SessionDiffResponse & { kind: "captured" } while merely returning
the input; replace it with a simpler identity signature (e.g., const
capturedAggregate = <T extends { kind: "captured" }>(x: T) => x) or remove the
helper and inline the object literals in tests where used to keep types inferred
and reduce verbosity; update usages of capturedAggregate accordingly (refer to
the capturedAggregate helper in use-session-timeline-data.test.ts).
In `@packages/app/src/pages/session/use-session-timeline-data.ts`:
- Around line 144-149: The effect calling createEffect currently triggers
input.sync.session.diff(id) repeatedly while
input.sync.data.turn_change_aggregate[id] is undefined; add a guard to track
in-flight fetches (e.g., a Set or Boolean map) so you only call
input.sync.session.diff(id) if there is not already a pending request for that
id, mark the id as in-flight before calling diff and clear it on
success/failure, and reference the existing symbols createEffect,
input.routeSessionID(), input.sync.data.turn_change_aggregate[id], and
input.sync.session.diff(id) when implementing the guard.
- Around line 41-53: The aggregateFiles function is duplicated; extract it into
a single exported utility named aggregateFiles that accepts a
SessionDiffResponse | undefined and returns SnapshotFileDiff[], then replace the
local copies in both hooks with imports of that utility; ensure the new utility
imports the types SessionDiffResponse and SnapshotFileDiff from the SDK and
preserves the existing behavior (return [] for falsy/empty/uncaptured, filter
restoreState === "applied", and map file/openPath, patch, additions, deletions,
status with defaults).
🪄 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: 69896e87-76a5-4fbd-8c43-dfc810931108
⛔ Files ignored due to path filters (1)
packages/sdk/js/src/v2/gen/types.gen.tsis excluded by!**/gen/**
📒 Files selected for processing (51)
packages/app/e2e/inputs/select-review-filter.spec.tspackages/app/e2e/session/session-review.spec.tspackages/app/e2e/snap/session-turn-changes.snap.tspackages/app/src/components/prompt-input/comment-routing.tspackages/app/src/context/global-sync/bootstrap.test.tspackages/app/src/context/global-sync/child-store.tspackages/app/src/context/global-sync/event-reducer.test.tspackages/app/src/context/global-sync/event-reducer.tspackages/app/src/context/global-sync/session-cache.test.tspackages/app/src/context/global-sync/session-cache.tspackages/app/src/context/global-sync/types.tspackages/app/src/context/sync.tsxpackages/app/src/pages/session.tsxpackages/app/src/pages/session/session-turn-changes.tsxpackages/app/src/pages/session/use-session-review-panel.tsxpackages/app/src/pages/session/use-session-review-state.tspackages/app/src/pages/session/use-session-timeline-data.test.tspackages/app/src/pages/session/use-session-timeline-data.tspackages/opencode/migration/20260521052709_turn_change_uncaptured/migration.sqlpackages/opencode/migration/20260521052709_turn_change_uncaptured/snapshot.jsonpackages/opencode/src/server/instance/session.tspackages/opencode/src/session/export.tspackages/opencode/src/session/message-v2.tspackages/opencode/src/session/revert.tspackages/opencode/src/session/session.sql.tspackages/opencode/src/session/session.tspackages/opencode/src/session/summary.tspackages/opencode/src/session/turn-change.tspackages/opencode/src/share/share-next.tspackages/opencode/src/tool/bash-write-heuristic.tspackages/opencode/src/tool/bash.tspackages/opencode/test/server/turn-change-aggregate-routes.test.tspackages/opencode/test/session/compaction.test.tspackages/opencode/test/session/id-monotonicity.test.tspackages/opencode/test/session/processor-effect.test.tspackages/opencode/test/session/processor-rate-limit.test.tspackages/opencode/test/session/prompt-effect.test.tspackages/opencode/test/session/session-artifacts.test.tspackages/opencode/test/session/snapshot-tool-race.test.tspackages/opencode/test/session/turn-change-aggregate.test.tspackages/opencode/test/share/share-next.test.tspackages/opencode/test/tool/bash-write-heuristic.test.tspackages/opencode/test/tool/bash.test.tspackages/ui/src/components/session-turn-changes-panel.tsxpackages/ui/src/components/session-turn-changes.tspackages/ui/src/components/session-turn-turn-changes.test.tspackages/ui/src/components/session-turn.csspackages/ui/src/components/timeline-playground.stories.tsxpackages/ui/src/context/data.tsxpackages/ui/src/i18n/en.tspackages/ui/src/i18n/zh.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
1 participant
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
expected_outputs.Self-check
Allowed legacy/PR-C-owned matches remain:
SessionSummary.summarize: compatibility no-op inpackages/opencode/src/session/summary.ts.session_diff: legacy storage/session read path inpackages/opencode/src/session/session.tsandpackages/opencode/src/storage/storage.tsfor PR C cleanup.summary.diffs: storage import/migration compatibility only.Expected new symbols are present:
TurnChangeUncapturedTable: schema + turn-change service callers.TurnChangeInvalidated: event definition, turn-change/revert publishers, share watcher.aggregateTurnUnion/aggregateSessionFromTurns: service, summary/server/share callers.isLikelyWriteCommand: bash heuristic + bash tool integration.Verification
bun test test/session/turn-change.test.ts test/session/turn-change-aggregate.test.ts test/tool/bash-write-heuristic.test.ts test/session/revert.test.ts test/session/session-artifacts.test.ts test/share/share-next.test.ts test/session/export.test.ts --timeout 30000— pass, 144 tests.bun test --preload ./happydom.ts src/context/global-sync/session-cache.test.ts src/context/global-sync/event-reducer.test.ts src/context/global-sync/bootstrap.test.ts src/pages/session/use-session-timeline-data.test.ts src/pages/session/use-session-review-panel.test.tsx src/pages/session/use-session-review-state.test.ts— pass, 72 tests.bun test src/components/session-turn-turn-changes.test.ts && bun run typecheckinpackages/ui— pass.bun run typecheckat repo root — pass.git diff --check— pass.bun run test:e2e -- e2e/session/session-review.spec.ts— pass with 3 existing/legacy review-comment cases skipped after Last Turn rendering moved to the timeline surface.Visual / smoke
session-review.spec.tswalked the changed Last Turn / review surface in Chromium and verified aggregate rows/actions. The plannedsession-turn-changesnap target does not exist in this repo; no new snap target was added in this PR.session_aggregateshould be accepted by review or mitigated by dual-publish / coordinated SaaS schema update before merge if required.Residual risk
session.revert.summarywhile an active revert exists.computeDiff,Session.Event.Diff,session_diff, and summary diff compatibility once AB lands.Summary by CodeRabbit
New Features
Improvements