fix(app): refresh review diffs on git state updates - #896
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughNormalize watcher file paths, reject missing files, and apply an explicit ChangesVCS Refresh Event Filtering
FileWatcher: VCS-aware filtering
InstanceStore: detect repo init and reload
Session UI & Review State
CI Labeler Update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/pages/session/use-session-vcs-refresh.test.ts, packages/app/src/pages/session/use-session-vcs-refresh.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 updates the VCS refresh logic to allow specific Git metadata changes, such as index and branch updates, to trigger a session refresh while ensuring cross-platform path compatibility. The reviewer suggested expanding the monitored Git files to include remote branches, tags, and transient operation heads like MERGE_HEAD to ensure the UI remains synchronized during various Git operations.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/app/src/pages/session/use-session-vcs-refresh.test.ts (1)
5-30: ⚡ Quick winExpand regression coverage for all newly allowlisted Git metadata paths.
This test now covers refs and object-store exclusion, but it still misses explicit assertions for the other allowlisted refresh triggers (
.git/HEAD,.git/packed-refs,.git/logs/HEAD,.git/worktrees/...).Suggested test additions
test("refreshes for rescan, source updates, and git state updates", () => { @@ expect( isFileWatcherVcsRefreshEvent({ type: "file.watcher.updated", properties: { file: ".git/refs/heads/feature/test", event: "change" }, }), ).toBe(true) + expect( + isFileWatcherVcsRefreshEvent({ + type: "file.watcher.updated", + properties: { file: ".git/HEAD", event: "change" }, + }), + ).toBe(true) + expect( + isFileWatcherVcsRefreshEvent({ + type: "file.watcher.updated", + properties: { file: ".git/packed-refs", event: "change" }, + }), + ).toBe(true) + expect( + isFileWatcherVcsRefreshEvent({ + type: "file.watcher.updated", + properties: { file: ".git/logs/HEAD", event: "change" }, + }), + ).toBe(true) + expect( + isFileWatcherVcsRefreshEvent({ + type: "file.watcher.updated", + properties: { file: ".git/worktrees/wt1/HEAD", event: "change" }, + }), + ).toBe(true) expect( isFileWatcherVcsRefreshEvent({ type: "file.watcher.updated", properties: { file: ".git/objects/aa/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", event: "change" }, }),🤖 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-vcs-refresh.test.ts` around lines 5 - 30, The test for isFileWatcherVcsRefreshEvent is missing explicit assertions for several newly allowlisted Git metadata paths; update the "refreshes for rescan, source updates, and git state updates" test to include additional expect(...) calls that assert true for events whose properties.file is ".git/HEAD", ".git/packed-refs", ".git/logs/HEAD", and a worktree path such as ".git/worktrees/my-worktree/gitdir", and continue to assert false for object-store paths (e.g., ".git/objects/..."); locate the test and add these expect(...) assertions using the same pattern as the existing file.watcher.updated checks against isFileWatcherVcsRefreshEvent.
🤖 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.
Nitpick comments:
In `@packages/app/src/pages/session/use-session-vcs-refresh.test.ts`:
- Around line 5-30: The test for isFileWatcherVcsRefreshEvent is missing
explicit assertions for several newly allowlisted Git metadata paths; update the
"refreshes for rescan, source updates, and git state updates" test to include
additional expect(...) calls that assert true for events whose properties.file
is ".git/HEAD", ".git/packed-refs", ".git/logs/HEAD", and a worktree path such
as ".git/worktrees/my-worktree/gitdir", and continue to assert false for
object-store paths (e.g., ".git/objects/..."); locate the test and add these
expect(...) assertions using the same pattern as the existing
file.watcher.updated checks against isFileWatcherVcsRefreshEvent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6706cb5f-594e-48b0-9387-49d9bab74eb7
📒 Files selected for processing (2)
packages/app/src/pages/session/use-session-vcs-refresh.test.tspackages/app/src/pages/session/use-session-vcs-refresh.ts
Perf delta summaryComparator: pass
|
github-actions
Bot
added
the
harness
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/opencode/src/project/instance-store.ts (1)
18-19: ⚡ Quick winPrefer Effect services over synchronous Node.js APIs.
The
hasGitMarkerhelper uses synchronousfs.existsSync,path.join, andpath.dirnamedirectly. Consider refactoring to useFileSystem.FileSystemandPath.Pathservices for consistency with the codebase patterns and better error handling through Effect's error channel.♻️ Example refactor using Effect services
-import fs from "node:fs" -import path from "node:path" +// FileSystem and Path are already available via yield* in Effect.gen context-function hasGitMarker(directory: string) { - let current = directory - while (true) { - if (fs.existsSync(path.join(current, ".git"))) return true - const parent = path.dirname(current) - if (parent === current) return false - current = parent - } -} +function hasGitMarker(directory: string): Effect.Effect<boolean, unknown, Filesystem> { + return Effect.gen(function* () { + const filesystem = yield* Filesystem + let current = directory + while (true) { + const gitPath = yield* filesystem.path.join(current, ".git") + const exists = yield* filesystem.exists(gitPath) + if (exists) return true + const parent = yield* filesystem.path.dirname(current) + if (parent === current) return false + current = parent + } + }) +}Then update the call site at line 228:
- if (exit.value.project.vcs !== "git" && hasGitMarker(directory)) { + const gitMarkerExists = yield* hasGitMarker(directory) + if (exit.value.project.vcs !== "git" && gitMarkerExists) {As per coding guidelines: "Prefer
FileSystem.FileSysteminstead of rawfs/promisesfor effectful file I/O in Effect services" and "PreferPath.Path,Config,Clock, andDateTimeservices when those concerns are already inside Effect code".Also applies to: 76-84
🤖 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/project/instance-store.ts` around lines 18 - 19, The helper hasGitMarker currently uses synchronous Node APIs (fs.existsSync, path.join, path.dirname); refactor it to use the Effect services FileSystem.FileSystem and Path.Path instead: replace direct fs/path calls inside hasGitMarker (and related helpers referenced around lines 76–84) with effectful operations from the FileSystem and Path services (e.g., use service-provided exists/existsSync equivalents and path join/dirname operations via Path.Path), return an Effect that fails on I/O errors rather than throwing, and update the call site(s) that invoke hasGitMarker to run the resulting Effect within the existing Effect runtime (keeping function name hasGitMarker and preserving its semantics of detecting .git markers).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/file/watcher.ts`:
- Around line 189-191: The current createCallback
(ParcelWatcher.SubscribeCallback) restores the instance context on every (err,
evts) call; instead define the handler function once inside createCallback and
bind the instance context once using Instance.bind (e.g., bind(ctx)) when
returning the callback, then remove any per-invocation context restoration
inside the handler; this change should be made in the createCallback function so
the returned ParcelWatcher.SubscribeCallback is already bound to the correct
ctx.
---
Nitpick comments:
In `@packages/opencode/src/project/instance-store.ts`:
- Around line 18-19: The helper hasGitMarker currently uses synchronous Node
APIs (fs.existsSync, path.join, path.dirname); refactor it to use the Effect
services FileSystem.FileSystem and Path.Path instead: replace direct fs/path
calls inside hasGitMarker (and related helpers referenced around lines 76–84)
with effectful operations from the FileSystem and Path services (e.g., use
service-provided exists/existsSync equivalents and path join/dirname operations
via Path.Path), return an Effect that fails on I/O errors rather than throwing,
and update the call site(s) that invoke hasGitMarker to run the resulting Effect
within the existing Effect runtime (keeping function name hasGitMarker and
preserving its semantics of detecting .git markers).
🪄 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: 72f60678-68f0-4ae6-8728-4e2454d7aefd
📒 Files selected for processing (4)
packages/opencode/src/file/watcher.tspackages/opencode/src/project/instance-store.tspackages/opencode/test/file/watcher.test.tspackages/opencode/test/project/instance-store.test.ts
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
Refresh the session Review panel's cached VCS diffs when Git state metadata changes.
Also align the backend Git watcher publish boundary with the frontend VCS refresh filter so staged, unstaged, and branch diff invalidation events are actually produced.
Also refresh cached project metadata when a directory becomes a Git repo after it was already opened, so Review does not stay stuck in the non-Git mode list.
A follow-up fixes two Review mode regressions found in live retesting:
Last turnnow requests the latest user turn instead of the whole-session aggregate, and selecting or reopening a VCS-backed Review mode forces a fresh diff load instead of reusing a prior empty cache.Also restore workflow PR
tasklabeling so the PR triage workflow contract matches the live labeler config.Why
Fixes #895.
The right-side Review panel could show stale data for
Branch changesbecause the VCS refresh path ignored Git state updates. The frontend filter originally ignored every.git/update, and the backend watcher only published.git/HEADfrom the Git directory subscription. That meant updates to.git/index, branch refs, and packed refs could fail to invalidate cached staged, unstaged, or branch diffs.A live test also found a separate transition bug: if a directory was opened before it was a Git repo and later became one via shell commands, the cached instance context kept
project.vcsempty. The Review mode selector then only showedLast turnand hidUnstaged,Staged, andBranch, even though the filesystem was now a Git repo.A second live retest found that
Last turnwas still backed by the full session aggregate, andBranch changescould keep a stale empty result after the branch changed. The Review panel now fetches turn diffs with the latest user message ID, and VCS modes force-refresh when selected or when the Review panel is reopened.The watcher path is now narrow on both sides:
.git/HEAD,.git/index,.git/packed-refs, and.git/refsvisible to the watcher;HEAD,index,packed-refs,refs/heads/**, andrefs/remotes/**from the Git directory;Cached non-Git instances now re-check the directory when a
.gitmarker appears and reload once the project is confirmed as Git.A review follow-up also covers remote branch refs, because branch diffs are computed from the default branch ref via
merge-base.The CI follow-up fixes a pre-existing workflow contract drift surfaced by
unit-opencode: workflow PRs are expected to receive bothciandtasklabels, but.github/labeler.ymlno longer had thetaskworkflow rule.Related Issue
Fixes #895
Human Review Status
Pending
Review Focus
Please focus on the Git watcher event boundary across
packages/opencode/src/file/watcher.tsandpackages/app/src/pages/session/use-session-vcs-refresh.ts, the cached project transition inpackages/opencode/src/project/instance-store.ts, and the Review mode state inpackages/app/src/pages/session/use-session-review-state.ts. The expected behavior is: a directory opened as non-Git can become Git later, Review should expose VCS modes, Git metadata changes should refresh staged, unstaged, or branch diffs while noisy Git internals remain filtered,Last turnshould only show the latest user turn, and selectingBranch changesshould not reuse a prior empty cache.For the CI follow-up, please check that the restored
tasklabeler rule only applies to workflow files and matches the existing contract test.Risk Notes
No copy or layout changed. This does change visible Review panel data behavior, so I launched the dev Electron app from this branch for live retesting and verified the backend VCS branch diff against the same
/Users/yuhan/PawWorkrepo state.This PR touches Git metadata path filtering in the backend watcher and frontend refresh consumer. The intended path surface is narrow: POSIX native watcher events are covered locally, Windows-style watcher paths are covered in the frontend normalization test, and backend Windows behavior uses
path.relative/path.septhrough Node's platform path implementation.This PR also touches cached project reload behavior for directories that transition from non-Git to Git. The reload check is limited to cached non-Git instances where a
.gitmarker exists; already-Git projects keep the existing fast path.This PR also touches GitHub labeler automation. The intended behavior is narrow: workflow-only PRs get
taskin addition toci, while priority remains owned by the priority triage script.No docs, release notes, dependencies, permissions, credentials, deletion behavior, generated content, or local file changes are relevant.
The main residual risk is platform-specific watcher path shape variation that is not reproduced by the native macOS watcher test. The filtering is intentionally limited to VCS state paths instead of all
.git/updates.How To Verify
Screenshots or Recordings
Not applicable. This change updates refresh invalidation behavior, cached project metadata, and GitHub labeler configuration; it does not change visible UI or copy.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores