Repository navigation
refactor(renderer): fold 13 sidebar, hook and chat readers off the deprecated owner resolver, and count the adapter as a focus read - #30478
Conversation
…-page readers off the deprecated owner resolver Thirteen read-routing callers (#4, #5, #10-#20 of the round-3 caller list) read the owner through runtimeEnvironmentIdWithLegacyFallbackUnlessTwin. A workspace two hosts publish is no longer searched on either host. The focus-read ratchet now counts the legacy-fallback adapter, which reads the focused runtime inside an object literal its discovery skipped; the five F1 entries that blind spot pruned come back. Deprecated importers 53 -> 40.
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
The description clearly explains the scope, behavior changes, rationale, testing, and checklist status. However, the required Linked Issue section does not contain an actual issue link, and results fo… | Add an actual linked issue under “Fixes #” or provide the required maintainer rationale. Report whether pnpm lint, pnpm typecheck, and pnpm build passed or will be covered by CI. |
| Docstring Coverage | |
Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 23 files. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly identifies the main refactor: migrating renderer readers from the deprecated owner resolver and adding the adapter to focus-read tracking. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Description check
-
Explanation
The description clearly explains the scope, behavior changes, rationale, testing, and checklist status. However, the required Linked Issue section does not contain an actual issue link, and results for lint, typecheck, and build are not reported.
Resolution
Add an actual linked issue under “Fixes #” or provide the required maintainer rationale. Report whether pnpm lint, pnpm typecheck, and pnpm build passed or will be covered by CI.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 23 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
![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
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
fb894b31-6d2c-4a78-9dc5-1d83d9e3faf0
📒 Files selected for processing: 25
config/deprecated-runtime-owner-importers-baseline.txtconfig/focus-setting-read-baseline.txtconfig/scripts/check-focus-read-ratchet.mjsconfig/scripts/check-focus-read-ratchet.test.mjssrc/renderer/src/components/browser-pane/assemble-chrome/browser-deferred-lifecycle.test.tsxsrc/renderer/src/components/browser-pane/workspace-doc/workspace-doc-page-pane.test.tsxsrc/renderer/src/components/browser-pane/workspace-doc/workspace-doc-page-pane.tsxsrc/renderer/src/components/native-chat/native-chat-file-link.tssrc/renderer/src/components/native-chat/native-chat-http-link-source-owner.test.tssrc/renderer/src/components/native-chat/native-chat-http-link-source-owner.tssrc/renderer/src/components/right-sidebar/FileExplorer.tsxsrc/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-controller-state.tsxsrc/renderer/src/components/right-sidebar/file-explorer-row-context-menu.tsxsrc/renderer/src/components/right-sidebar/git-status-file-watch-refresh.tssrc/renderer/src/components/right-sidebar/source-control-entry-context-menu.test.tsxsrc/renderer/src/components/right-sidebar/source-control/listing/entry-context-menu.tsxsrc/renderer/src/components/right-sidebar/useFileSearchRunner.test.tsxsrc/renderer/src/components/right-sidebar/useFileSearchRunner.tssrc/renderer/src/components/right-sidebar/useFileSearchScope.tssrc/renderer/src/components/terminal-pane/terminal-input-host-platform.tssrc/renderer/src/hooks/editor-external-watch-targets.tssrc/renderer/src/hooks/ipc-events/browser-state-capture-paint-hold.test.tssrc/renderer/src/hooks/ipc-events/browser-state-ipc-bridge.tssrc/renderer/src/hooks/ipc-events/browser-state-open-link-profile.test.tssrc/renderer/src/lib/worktree-runtime-owner.ts
💤 Files with no reviewable changes: 1
- config/deprecated-runtime-owner-importers-baseline.txt
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.
| const ownerIsTwin = useAppStore( | ||
| (state) => | ||
| !!activeWorktreeId && | ||
| resolveOwner(state, { workspaceId: activeWorktreeId }).kind === 'ambiguous' | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '65,225p' src/renderer/src/components/right-sidebar/useFileSearchRunner.ts
sed -n '1,120p' src/renderer/src/components/right-sidebar/useFileSearchScope.tsRepository: stablyai/orca
Length of output: 7639
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- runner ---'
nl -ba src/renderer/src/components/right-sidebar/useFileSearchRunner.ts | sed -n '1,240p'
printf '%s\n' '--- search state symbols ---'
rg -n -F --glob '*.ts' --glob '*.tsx' \
-e 'updateActiveSearchState' \
-e 'fileSearchStateByWorktree' \
-e 'seedRequestId' \
-e 'executeSearch(' \
src/renderer/src | sed -n '1,240p'
printf '%s\n' '--- relevant state declarations ---'
rg -n -F --glob '*.ts' --glob '*.tsx' \
-e 'FileSearchState' \
-e 'fileSearchStateByWorktree:' \
-e 'setFileSearch' \
src/renderer/src | sed -n '1,200p'Repository: stablyai/orca
Length of output: 34644
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- panel ---'
nl -ba src/renderer/src/components/right-sidebar/useFileSearchPanel.ts | sed -n '1,340p'
printf '%s\n' '--- state action ---'
nl -ba src/renderer/src/store/slices/editor/actions/file-search-actions.ts | sed -n '1,135p'
printf '%s\n' '--- lifecycle tests ---'
nl -ba src/renderer/src/components/right-sidebar/useFileSearchPanel.lifecycle.test.tsx | sed -n '1,220p'
printf '%s\n' '--- runner tests around cancellation and twin ---'
nl -ba src/renderer/src/components/right-sidebar/useFileSearchRunner.test.tsx | sed -n '300,430p'Repository: stablyai/orca
Length of output: 32583
Cancel the active search when ownership becomes ambiguous.
An in-flight search can finish after ownerIsTwin becomes true. Existing results remain current, so the scope hook does not cancel the search. Its completion can then publish visible results for a twin workspace, violating the no-results behavior.
This is a narrow stale-results issue. The inspected click path preserves the saved result owner, so no wrong-owner action is established.
🐛 Suggested fix
useEffect(
() => cancelPendingSearch,
[cancelPendingSearch, worktreePath, executionHostId, runtimeEnvironmentId]
)
+
+ useEffect(() => {
+ if (!ownerIsTwin) {
+ return
+ }
+ cancelPendingSearch()
+ updateActiveSearchState({ results: null, resultOwner: null, error: null })
+ }, [ownerIsTwin, cancelPendingSearch, updateActiveSearchState])
return { executeSearch, cancelPendingSearch }
|
Codex (gpt-6.1-sol, high) on 36aee92: no findings. pr.yml 39186155917 success; ssh-orcad Docker lane (e2e.yml dispatch, all tests/e2e/ssh-orcad-* specs) 39186109625 success on the exact head. Local static job: 0 failures. Units: 2,240 files single-worker (right-sidebar, lib, hooks, native-chat, terminal-pane, browser-pane). |
ELI5
Thirteen places in the right sidebar, the editor watcher, native chat links and the doc-page preview asked an old helper "which server owns this workspace?". That helper is being retired. They now ask the replacement, which gives the same answer everywhere except for a few shapes the old one got wrong.
What Changed
runtimeEnvironmentIdWithLegacyFallbackUnlessTwininstead of the deprecatedgetRuntimeEnvironmentIdForWorktree(callers Add Ctrl+Click to open links in terminal #4, Fix PTY listener leak, use local restty with selection support #5 and Add card gap and quick-create in worktree dialog #10–P2-xs: we should add a “add new repo” button at the end of the “select repo” dropdown #20 in the round-3 classification).resolveOwnerWithLegacyFallbackbecause its focused-runtime answer is returned inside an object literal. It is now a seed, so the adapter and every wrapper that returns its result count as focus reads. The five entries that refactor(renderer): fold four lib callers off the deprecated runtime-owner resolver #30458 pruned through that blind spot come back, annotated; F8 removes them when it deletes the legacy branch.runtimeEnvironmentIdWithLegacyFallbackUnlessTwinaccepts a null workspace id (no active workspace) and returns null, as the deprecated helper did.Why
The deprecated resolver collapses "two hosts publish this" into "local" and guesses with the focused server. The replacement keeps both as distinct answers. Using the "unless twin" wrapper here keeps each reader's old meaning for a twin ("no server"), so only the rows the design marks as deliberate change. File search was the one reader where "no server" still meant "show results from some host", so it refuses instead. Counting the adapter in the focus ratchet stops the folds from looking like focus reads were removed when they were only hidden.
Linked Issue
Round 3 of the SSH/remote hardening (S6(b) F2+F3). No single issue.
Visual Proof
N/A: routing-only change with no new UI; the only visible difference is that file search shows nothing for a workspace published by two hosts.
Testing
New: file search does not run for a twin workspace (fails before); focus ratchet discovers a wrapper around the adapter.
pnpm testsingle worker oversrc/renderer/src/components/right-sidebar,src/renderer/src/lib,src/renderer/src/hooks,components/native-chat,components/terminal-pane,components/browser-paneand the ratchet tests.Full local copy of the pr.yml static job.
ssh-orcad Docker lane dispatched on the exact head (link in a comment).
Live verification: unit-tested only. The changes are read-routing folds whose live paths the ssh-orcad lane covers.
I manually tested these changes locally
Automated tests added/updated, or explained why not below
AI Disclosure
Review
Codex (
gpt-6.1-sol, high): no findings.Agent skill upstream boundary
Notes
SSH: the deliberate changes are exactly the twin, rival-server and stale-server-stamp rows of the round-3 acceptance table. Wire: none. Cross-platform: none.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)