Skip to content

Canonicalize workspace roots for session visibility#158

Open
YuMS wants to merge 8 commits into
friuns2:mainfrom
YuMS:feature/canonicalize-workspace-roots
Open

Canonicalize workspace roots for session visibility#158
YuMS wants to merge 8 commits into
friuns2:mainfrom
YuMS:feature/canonicalize-workspace-roots

Conversation

@YuMS
Copy link
Copy Markdown

@YuMS YuMS commented May 11, 2026

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

npx vitest run src/server/codexAppServerBridge.archive.test.ts
npx tsc -p tsconfig.server.json --noEmit
npm run build:frontend
git diff --check
PROFILE_BASE_URL=http://127.0.0.1:4173 PROFILE_WAIT_MS=7000 node scripts/profile-browser-runtime.cjs

Summary by CodeRabbit

  • Bug Fixes

    • Normalize symlinked workspace root paths so sessions, project filters, labels, thread CWDs, and persisted workspace-root state remain consistent and readable.
  • Tests

    • Added tests validating workspace-root canonicalization, thread-list CWD normalization, reuse of path-resolution results, and persisted canonical state.
  • Documentation

    • Added a manual verification procedure for symlinked workspace-root behavior.

Review Change Stack

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

Review Summary by Qodo

Canonicalize workspace roots for consistent session visibility

✨ Enhancement 🧪 Tests

Grey Divider

Walkthroughs

Description
• 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
Diagram
flowchart 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
Loading

Grey Divider

File Changes

1. src/server/codexAppServerBridge.ts ✨ Enhancement +76/-5

Add path canonicalization for workspace roots and threads

• Added realpath import from node:fs/promises for path canonicalization
• Exported WorkspaceRootsState type for external use
• Modified callRpcWithArchiveRecovery to canonicalize thread/list responses
• Implemented canonicalizeWorkspaceRootsStateForRead function to realpath workspace roots, labels,
 and project orders
• Implemented canonicalizeThreadCwdRecord and canonicalizeThreadListResponseForRead to
 canonicalize thread cwd values
• Updated readWorkspaceRootsState to apply canonicalization when reading persisted state

src/server/codexAppServerBridge.ts


2. src/server/codexAppServerBridge.archive.test.ts 🧪 Tests +58/-1

Add canonicalization function tests

• Added imports for canonicalizeThreadListResponseForRead and
 canonicalizeWorkspaceRootsStateForRead
• Added test suite for canonicalizeWorkspaceRootsStateForRead verifying symlink paths are
 realpathd
• Added test suite for canonicalizeThreadListResponseForRead verifying thread cwd values are
 canonicalized
• Tests verify that mixed symlink and canonical paths are normalized consistently

src/server/codexAppServerBridge.archive.test.ts


3. tests.md 📝 Documentation +30/-0

Add manual regression test for symlink workspace roots

• Added manual regression test section "Sidebar sessions survive symlinked workspace roots"
• Documents prerequisites, test steps, and expected results for symlink path handling
• Verifies sessions are visible regardless of whether recorded through symlink or canonical path
• Includes verification of API response returning canonical real paths
• Tests both light and dark theme rendering

tests.md


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

qodo-free-for-open-source-projects Bot commented May 11, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Action required

1. Label overwrite race ✓ Resolved 🐞 Bug ≡ Correctness
Description
canonicalizeWorkspaceRootsStateForRead concurrently assigns canonicalized label keys into a shared
object; if multiple original keys (e.g., symlink + target) realpath to the same canonical key, the
winning label becomes dependent on async completion order and can flip between reads. This is
user-visible because the sidebar hydrates display names from rootsState.labels.
Code

src/server/codexAppServerBridge.ts[R3905-3909]

+  const labels: Record<string, string> = { ...state.labels }
+  await Promise.all(Object.entries(state.labels).map(async ([key, label]) => {
+    const canonicalKey = await canonicalizeWorkspaceRootPath(key, pathRealpath)
+    labels[canonicalKey] = label
+  }))
Evidence
The server currently copies labels then concurrently assigns canonical keys, which can collide when
both canonical and symlink forms exist; the state writer can introduce such mixed keys, and the UI
consumes labels to set visible project display names.

src/server/codexAppServerBridge.ts[3896-3909]
src/server/codexAppServerBridge.ts[4006-4022]
src/composables/useDesktopState.ts[3853-3865]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`canonicalizeWorkspaceRootsStateForRead` builds `labels` by copying `state.labels` and then `Promise.all`-writing `labels[canonicalKey] = label` concurrently. If two different keys canonicalize to the same `canonicalKey` (common when both symlink and target were saved), the final label is nondeterministic (depends on which `realpath` resolves last).
## Issue Context
This function is used on every `/codex-api/workspace-roots-state` read, and the UI iterates `rootsState.labels` to hydrate project display names.
## Fix Focus Areas
- src/server/codexAppServerBridge.ts[3896-3917]
## Suggested fix approach
- Build a **new** `labels` map deterministically instead of mutating a shared object from concurrent tasks.
- Make collision policy explicit, e.g.:
- Prefer an entry whose original key is already canonical (`key === canonicalKey`) over a symlink-derived key.
- Or process entries sequentially in stable key order.
- Consider dropping non-canonical keys from the returned `labels` to avoid returning both forms.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. No realpath memoization ✓ Resolved 🐞 Bug ➹ Performance
Description
canonicalizeThreadListResponseForRead calls realpath once per thread item (per absolute cwd) without
memoizing identical cwd strings, so a thread/list page with many sessions from the same project
triggers redundant filesystem syscalls. This runs on every thread/list RPC and thread/list is
paginated (50/100 items per page) and repeatedly requested during background loading.
Code

src/server/codexAppServerBridge.ts[R3935-3940]

+  const record = asRecord(payload)
+  if (!record || !Array.isArray(record.data)) return payload
+  return {
+    ...record,
+    data: await Promise.all(record.data.map((item) => canonicalizeThreadCwdRecord(item, pathRealpath))),
+  }
Evidence
The server now canonicalizes thread/list results and does a per-item async mapping; thread/list is
called with up to 100 items per page and paginated in the UI, so redundant per-item realpath calls
can accumulate across pages.

src/server/codexAppServerBridge.ts[1225-1234]
src/server/codexAppServerBridge.ts[3920-3940]
src/api/codexGateway.ts[688-704]
src/composables/useDesktopState.ts[4080-4091]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`canonicalizeThreadListResponseForRead` maps over every `record.data` entry and calls `canonicalizeWorkspaceRootPath` (which calls `realpath`) per item. If multiple items share the same `cwd` string, we perform redundant `realpath` calls.
## Issue Context
The server applies this transformation for every `thread/list` RPC. The client calls `thread/list` with limits up to 100 and performs background pagination.
## Fix Focus Areas
- src/server/codexAppServerBridge.ts[3920-3941]
- src/server/codexAppServerBridge.ts[1225-1234]
## Suggested fix approach
- Add a per-invocation cache in `canonicalizeThreadListResponseForRead`, e.g. `Map<string, Promise<string>>`, keyed by the original `cwd` string.
- Resolve each unique `cwd` at most once, then reuse the cached promise for subsequent items.
- Keep the existing behavior of skipping non-absolute values and swallowing `realpath` failures.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread src/server/codexAppServerBridge.ts Outdated
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 13, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 28a32473-2318-41a3-b9c2-42feb12c5ecd

📥 Commits

Reviewing files that changed from the base of the PR and between 98afb9d and 57d56b9.

📒 Files selected for processing (3)
  • src/server/codexAppServerBridge.archive.test.ts
  • src/server/codexAppServerBridge.ts
  • tests.md
✅ Files skipped from review due to trivial changes (1)
  • tests.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/server/codexAppServerBridge.archive.test.ts
  • src/server/codexAppServerBridge.ts

📝 Walkthrough

Walkthrough

Resolve symlinked workspace-root paths and thread cwd values to canonical real paths via an injected async realpath resolver; apply canonicalization to thread/list RPC results and the workspace-roots-state API; add tests and a manual verification plan.

Changes

Symlink canonicalization for workspace roots

Layer / File(s) Summary
Type export and fs import
src/server/codexAppServerBridge.ts
Export WorkspaceRootsState and import realpath from Node.js fs/promises to enable canonicalization plumbing.
Canonicalization utilities
src/server/codexAppServerBridge.ts
Add canonicalizeWorkspaceRootsState, canonicalizeWorkspaceRootsStateForRead, and canonicalizeThreadListResponseForRead that resolve and rewrite workspace-root paths (order, active, project-order, labels) and per-thread cwd, using an injectable pathRealpath and per-call caching.
RPC & workspace-roots persistence integration
src/server/codexAppServerBridge.ts
Post-process only thread/list responses via canonicalizeThreadListResponseForRead in callRpcWithArchiveRecovery; have readWorkspaceRootsState/writeWorkspaceRootsState return and persist canonicalized WorkspaceRootsState fields.
Tests and manual verification
src/server/codexAppServerBridge.archive.test.ts, tests.md
Import and test canonicalizeWorkspaceRootsStateForRead and canonicalizeThreadListResponseForRead (symlink → canonical path rewriting, label deduplication, caching per-response); add manual test plan "Sidebar sessions survive symlinked workspace roots".

Sequence Diagram

sequenceDiagram
  participant Client
  participant callRpcWithArchiveRecovery
  participant canonicalizeThreadListResponseForRead
  participant readWorkspaceRootsState
  participant canonicalizeWorkspaceRootsStateForRead
  participant realpath

  Client->>callRpcWithArchiveRecovery: thread/list RPC call
  callRpcWithArchiveRecovery->>canonicalizeThreadListResponseForRead: raw response with cwd values
  canonicalizeThreadListResponseForRead->>realpath: resolve cwd paths (with cache)
  realpath-->>canonicalizeThreadListResponseForRead: canonical paths
  canonicalizeThreadListResponseForRead-->>callRpcWithArchiveRecovery: canonicalized thread list
  callRpcWithArchiveRecovery-->>Client: thread list with canonical cwd

  Client->>readWorkspaceRootsState: /codex-api/workspace-roots-state request
  readWorkspaceRootsState->>canonicalizeWorkspaceRootsStateForRead: persisted workspace roots payload
  canonicalizeWorkspaceRootsStateForRead->>realpath: resolve order/active/label paths
  realpath-->>canonicalizeWorkspaceRootsStateForRead: canonical paths
  canonicalizeWorkspaceRootsStateForRead-->>readWorkspaceRootsState: canonicalized state
  readWorkspaceRootsState-->>Client: workspace roots with canonical paths
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hop through links and tangled trails,
Realpath hums and straightens all the trails.
Roots align, labels fold to one true name,
Threads return home tidy — none the same.
Hooray — the sidebar sleeps without a fright!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Canonicalize workspace roots for session visibility' directly aligns with the main objective of canonicalizing saved workspace roots to keep sessions visible when symlink and target paths are mixed.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.

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.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 1c9dacd and 5a33063.

📒 Files selected for processing (3)
  • src/server/codexAppServerBridge.archive.test.ts
  • src/server/codexAppServerBridge.ts
  • tests.md

Comment thread src/server/codexAppServerBridge.archive.test.ts
Comment thread src/server/codexAppServerBridge.ts Outdated
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@YuMS
Copy link
Copy Markdown
Author

YuMS commented May 13, 2026

@friuns2 Could you help take a look at this PR?

@friuns2
Copy link
Copy Markdown
Owner

friuns2 commented May 15, 2026

I think the canonicalize-on-write gap is still real in the current head.

readWorkspaceRootsState() now canonicalizes on read, but writeWorkspaceRootsState() still persists nextState verbatim, and persistWorkspaceRoot() still inserts the raw normalizedRoot into order / active / projectOrder / labels.

That means mixed symlink + canonical paths can still be written back to disk and then only cleaned up on read, so duplicates can be reintroduced at the persistence layer.

I’d recommend canonicalizing before every write as well, either by:

  1. canonicalizing the outgoing state before writeWorkspaceRootsState(...), or
  2. making writeWorkspaceRootsState(...) itself canonicalize its input.

That would make the storage format stable instead of only normalizing the read view.

@YuMS
Copy link
Copy Markdown
Author

YuMS commented May 16, 2026

@friuns2 Noted. I chose the simpler approach: writeWorkspaceRootsState() now canonicalizes its input state directly. That means persistWorkspaceRoot(), PUT /codex-api/workspace-roots-state, and any future write paths all persist the same stable canonical format without duplicating canonicalization logic at each call site.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants