Skip to content

Add the rules that decide which editor tabs are the same file (phase 1b-1, unused) - #31272

Merged
nwparker merged 5 commits into
mainfrom
nwparker/editor-buffer-phase-1b-1
Oct 21, 2026
Merged

nwparker merged 5 commits into
mainfrom
nwparker/editor-buffer-phase-1b-1

Conversation

@nwparker

@nwparker nwparker commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​296 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​296
Prod 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​146 $\color{#cf222e}{\Huge{\mathbf{−}}}$​1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​145

ELI5

Later in this series, editor tabs that show the same file in different workspaces will share one record of their unsaved edits. Deciding "is this the same file?" has to be exactly right: if two different files are treated as one, edits from one could be written into the other. This PR adds only that decision, as a small module with thorough tests. Nothing uses it yet, so nothing changes for users.

What Changed

  • New src/renderer/src/store/slices/editor/buffers/editor-buffer-identity.ts: computes the key that will decide which tabs share a buffer record (environment, host, runtime revision, path). Rules:
    • Case is never folded in file names (case-insensitive file systems can't be proven from the client).
    • Windows paths: separators and the drive letter are normalized only for paths proven to be Windows (drive letter or \\), or // paths on a host known to be Windows. On Linux/SSH, //repo/a\b.ts and //repo/a/b.ts stay different files.
    • UNC: only the server name is case-folded, matching how editor models are shared today.
    • WSL: \\wsl$\… and \\wsl.localhost\… are treated as the same file only on a local Windows client, using a tagged key that can't collide with a real share named wsl.
    • SSH: keyed by the endpoint the tab captured when it opened, never the connection's current endpoint, so a target edited to point at another machine can't merge files across machines. Missing endpoint → never shares.
    • Non-edit tabs, unresolved owners and retired runtimes never share.
  • editor-model-owner.ts: splits resolveEditorModelOwner out of getEditorModelOwnerKey (pure refactor; output unchanged).

Why

This is step 1b-1 of moving to one editor buffer per file. Landing the identity rules first, unused, lets them be reviewed in isolation. Two independent reviews (GPT and Opus) found four ways an earlier draft could merge different files or split files that share today (WSL vs a real \\wsl share, POSIX // paths, SSH retargeting, UNC server case); all are fixed and covered by tests.

A change to the editor model URI for remote files (from draft #30714) was split out of this PR: it is a user-visible fix in its own right (remote .tsx files currently parse as plain TypeScript, and imports can resolve to the local machine's file at the same path) and will come as its own PR.

Linked Issue

Follow-up to #31251.

Visual Proof

N/A: no behaviour change; the module is not used yet.

Testing

  • 27 identity tests (exact case, drive letters/separators, WSL folding on Windows only, POSIX // paths, UNC server case on local and remote Windows hosts, SSH captured endpoint across retarget and reconnect, runtime revision continuation and retirement quarantine).

  • Editor slice and editor-model suites pass (484 tests); pnpm tc, check:code-quality:changed, oxfmt --check, oxlint clean.

  • Reviewed by GPT (three rounds) and Opus.

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

AI Disclosure

Review

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance

The renderer does not yet have each SSH tab's captured endpoint; supplying it is part of the later switch PR. Until then the module would never share SSH tabs.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

nwparker and others added 3 commits October 20, 2026 16:27
Adds `editorBufferIdentityOf`, not consumed yet: [environment, host, pairing root, path key] with
exact case, normalized Windows separators and drive letter, WSL alias folding only for a local
Windows client, SSH keyed with its runtime environment, and a detached record id for retired hosts.
Splits `resolveEditorModelOwner` out of the owner-key function; the owner key is unchanged.

Co-Authored-By: Claude <noreply@anthropic.com>
…hase 1b-1 review)

- A folded WSL alias is a tagged tuple, so a real `\\wsl\<distro>` share can't collide with it.
- `//` is ambiguous between POSIX and UNC, so it folds only for a known-Windows (local) host;
  drive and `\\` paths still fold separators and the drive letter on any host.
- A proven UNC path folds its server name, matching the Monaco URI authority today.
- SSH identity includes the target's endpoint incarnation: a target id edited to point at another
  machine gets a new key, a reconnect keeps it; an unknown endpoint never shares.
- Documents the live pairing resolution and per-event retirement ids.

Co-Authored-By: Claude <noreply@anthropic.com>
…ified host path flavor

- The SSH endpoint is the one the tab captured at open, passed in by the caller; a live lookup
  would match an old tab to whatever machine the target id names now. Missing means never share.
- Remote hosts accept a verified path flavor (`windows` | `posix` | `unknown`), so a Windows host's
  `//SERVER/share` folds like `\\SERVER\share`, as the Monaco URI authority does today.

Co-Authored-By: Claude <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 8 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 4e5f9c25-c946-45b7-b3c0-b93fdd556d47
📥 Commits

Reviewing files that changed from the base of the PR and between 786d4b9 and fd08847.

📒 Files selected for processing: 3
  • src/renderer/src/components/editor/editor-model-owner.ts
  • src/renderer/src/store/slices/editor/buffers/editor-buffer-identity.test.ts
  • src/renderer/src/store/slices/editor/buffers/editor-buffer-identity.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 29cc0b91-28f9-407b-8cf4-cc1f848fe316
📥 Commits

Reviewing files that changed from the base of the PR and between 6e973fa and edc4cff.

📒 Files selected for processing: 3
  • src/renderer/src/components/editor/editor-model-owner.ts
  • src/renderer/src/store/slices/editor/buffers/editor-buffer-identity.test.ts
  • src/renderer/src/store/slices/editor/buffers/editor-buffer-identity.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

📝 Walkthrough

The change adds resolveEditorModelOwner and updates getEditorModelOwnerKey to use its resolved owner. It also adds utilities to create editor buffer identities from environment, host, runtime pairing, SSH endpoint, and normalized path data. The utilities handle unresolved or non-edit tabs, retired environments, and detached record IDs. New tests cover path normalization, host and ownership cases, and runtime pairing revisions.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to edc4c

This change adds groundwork for deciding when editor tabs refer to the same file, but nothing uses it yet. The existing editor owner key behaves the same as before, so users should see no change. No merge-blocking risk was found.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly identifies the main change: adding rules to determine when editor tabs represent the same file. The phase and unused status provide useful scope context.
Description check Passed The description is detailed and covers the change, rationale, testing, visual proof, scope, and checklist. It does not provide an actual linked issue URL as required by the template; it only reference…
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.
Full details: Description check

Explanation

The description is detailed and covers the change, rationale, testing, visual proof, scope, and checklist. It does not provide an actual linked issue URL as required by the template; it only references #31251, and the full validation command checklist remains unchecked.

✨ 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.

❤️ Share

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

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.

1 participant