Canonicalize workspace roots for session visibility#158
Open
YuMS wants to merge 6 commits into
Open
Conversation
Review Summary by QodoCanonicalize workspace roots for consistent session visibility
WalkthroughsDescription• Canonicalize workspace roots and thread cwd values through realpath • Ensures sessions remain visible with symlinked workspace paths • Exports canonicalization functions for workspace roots state • Adds comprehensive test coverage and manual regression tests Diagramflowchart LR
A["Thread/List Response"] -->|canonicalizeThreadListResponseForRead| B["Canonicalized CWD Values"]
C["Workspace Roots State"] -->|canonicalizeWorkspaceRootsStateForRead| D["Canonicalized Paths"]
B --> E["Sessions Visible Regardless of Symlink"]
D --> E
File Changes1. src/server/codexAppServerBridge.ts
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR resolves symlinked workspace-root paths and thread cwd values to canonical real paths via an async realpath resolver, applies the canonicalization to thread/list RPC results and the workspace-roots-state API, and adds tests plus a manual test plan. ChangesSymlink canonicalization for workspace roots
Sequence DiagramsequenceDiagram
participant Client
participant callRpcWithArchiveRecovery
participant canonicalizeThreadListResponseForRead
participant readWorkspaceRootsState
participant canonicalizeWorkspaceRootsStateForRead
participant realpath
Client->>callRpcWithArchiveRecovery: thread/list RPC call
callRpcWithArchiveRecovery->>canonicalizeThreadListResponseForRead: raw response with symlinked cwd
canonicalizeThreadListResponseForRead->>realpath: resolve cwd paths
realpath-->>canonicalizeThreadListResponseForRead: canonical paths
canonicalizeThreadListResponseForRead-->>callRpcWithArchiveRecovery: canonicalized thread list
callRpcWithArchiveRecovery-->>Client: thread list with real paths
Client->>readWorkspaceRootsState: /codex-api/workspace-roots-state request
readWorkspaceRootsState->>canonicalizeWorkspaceRootsStateForRead: persisted workspace roots state
canonicalizeWorkspaceRootsStateForRead->>realpath: resolve order, active, label paths
realpath-->>canonicalizeWorkspaceRootsStateForRead: canonical paths
canonicalizeWorkspaceRootsStateForRead-->>readWorkspaceRootsState: canonicalized state
readWorkspaceRootsState-->>Client: workspace roots with real paths
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/server/codexAppServerBridge.archive.test.ts`:
- Around line 135-183: The test file is missing the closing braces for the inner
"it('reuses cwd realpath results within one thread list response'...)" test and
the outer describe("canonicalizeThreadListResponseForRead", ...) block; add the
missing "})" to close the failing it block and then add another "})" to close
the describe block so the next describe can start cleanly. Locate the describe
block named "canonicalizeThreadListResponseForRead" and the two it blocks inside
it and append the necessary closing braces in order to properly terminate the it
and then the describe.
In `@src/server/codexAppServerBridge.ts`:
- Around line 4385-4391: The state is being canonicalized only on read but not
before persisting, so writeWorkspaceRootsState() receives raw symlink paths
(nextState) and the file can contain mixed entries; update
persistWorkspaceRoot() and the handler that serves PUT
/codex-api/workspace-roots-state to canonicalize the outgoing state before
calling writeWorkspaceRootsState()—e.g., call
canonicalizeWorkspaceRootsStateForRead(nextState) (or extract a small
canonicalizeForWrite helper) and pass its result into writeWorkspaceRootsState()
instead of nextState so the on-disk file stores canonical paths and avoids
reintroducing duplicates.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f1813ad-67c6-4045-92ea-aeeb994d4ab0
📒 Files selected for processing (3)
src/server/codexAppServerBridge.archive.test.tssrc/server/codexAppServerBridge.tstests.md
Comment on lines
+4385
to
+4391
| return await canonicalizeWorkspaceRootsStateForRead({ | ||
| order: normalizeStringArray(payload['electron-saved-workspace-roots']), | ||
| labels: normalizeStringRecord(payload['electron-workspace-root-labels']), | ||
| active: normalizeStringArray(payload['active-workspace-roots']), | ||
| projectOrder: normalizeStringArray(payload['project-order']), | ||
| remoteProjects: normalizeRemoteProjects(payload['remote-projects']), | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Canonicalize before persisting, not only when reading.
This normalizes the API response, but new roots written through persistWorkspaceRoot() and PUT /codex-api/workspace-roots-state still hit disk as raw symlink paths because writeWorkspaceRootsState() stores nextState unchanged. That leaves the state file with mixed canonical/non-canonical entries and can reintroduce duplicates for any code path that reads the file directly.
Suggested direction
async function writeWorkspaceRootsState(nextState: WorkspaceRootsState): Promise<void> {
+ const canonicalState = await canonicalizeWorkspaceRootsStateForRead(nextState)
const statePath = getCodexGlobalStatePath()
let payload: Record<string, unknown> = {}
try {
const raw = await readFile(statePath, 'utf8')
payload = asRecord(JSON.parse(raw)) ?? {}
} catch {
payload = {}
}
- payload['electron-saved-workspace-roots'] = normalizeStringArray(nextState.order)
- payload['electron-workspace-root-labels'] = normalizeStringRecord(nextState.labels)
- payload['active-workspace-roots'] = normalizeStringArray(nextState.active)
- payload['project-order'] = normalizeStringArray(nextState.projectOrder)
+ payload['electron-saved-workspace-roots'] = normalizeStringArray(canonicalState.order)
+ payload['electron-workspace-root-labels'] = normalizeStringRecord(canonicalState.labels)
+ payload['active-workspace-roots'] = normalizeStringArray(canonicalState.active)
+ payload['project-order'] = normalizeStringArray(canonicalState.projectOrder)
await writeFile(statePath, JSON.stringify(payload), 'utf8')
}🤖 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 `@src/server/codexAppServerBridge.ts` around lines 4385 - 4391, The state is
being canonicalized only on read but not before persisting, so
writeWorkspaceRootsState() receives raw symlink paths (nextState) and the file
can contain mixed entries; update persistWorkspaceRoot() and the handler that
serves PUT /codex-api/workspace-roots-state to canonicalize the outgoing state
before calling writeWorkspaceRootsState()—e.g., call
canonicalizeWorkspaceRootsStateForRead(nextState) (or extract a small
canonicalizeForWrite helper) and pass its result into writeWorkspaceRootsState()
instead of nextState so the on-disk file stores canonical paths and avoids
reintroducing duplicates.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Author
|
@friuns2 Could you help take a look at this PR? |
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
canonicalize saved workspace roots with local realpath
canonicalize thread/list cwd values before sidebar filtering
keep sessions visible when symlink and target paths are mixed
add regression tests and manual test coverage
Verification
Summary by CodeRabbit
Bug Fixes
Tests
Documentation