Skip to content

Fix/vscode diff preview edge cases - #1734

Open
Piyushrathoree wants to merge 4 commits into
Nano-Collective:mainfrom
Piyushrathoree:fix/vscode-diff-preview-edge-cases
Open

Piyushrathoree wants to merge 4 commits into
Nano-Collective:mainfrom
Piyushrathoree:fix/vscode-diff-preview-edge-cases

Conversation

@Piyushrathoree

Copy link
Copy Markdown
Contributor

Fixes #1726

Description

The VS Code diff preview had three edge cases:

  1. Collisions: pending diffs were keyed by toolCallId. When one tool call reported several diff blocks, earlier blocks were overwritten in DiffManager.
  2. Focus stealing: clicking a diff card in chat ran workbench.action.terminal.focus, which moved focus to an open terminal.
  3. Stale state: diffs and their virtual documents were never cleared, so they outlived new chats, session resumes, and agent restarts.

Changes:

  • Pending diffs are keyed by diffChangeId(toolCallId, filePath), a shared helper used by both the producer and the "Open Diff" click.
  • The card records its file path and sends it with showDiff, so the click opens the right file.
  • Diffs opened from chat pass preserveTerminalFocus = false, so the terminal is not focused. The default is unchanged for the CLI path.
  • DiffManager.clearPendingChanges() closes the diff editors and drops the records. It runs on New Chat, resume, and each ACP handshake (onConnectionReady).

Limitation: a tool call that reports several diffs still shows one card, for the first file. The other diffs are stored without collisions, but the UI does not show them yet.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Changeset

  • Added a changeset describing this change for the changelog

Testing

Automated Tests

  • New features include passing tests in .spec.ts files (plugins/vscode/src/diff-manager.spec.ts, 8 tests)
  • All existing tests pass (pnpm test:all) — not run in full for this change
  • Tests cover both success and error scenarios (no-op clear, missing change, multi-diff, focus on/off)
  • VS Code specs pass (pnpm run test:ava 'plugins/vscode/src/*.spec.ts', 180 tests)
  • VS Code type check passes (pnpm run test:types:vscode)

Manual Testing

  • Extension Development Host: diff card click opens the diff, and focus stays out of the terminal
  • Extension Development Host: New Chat, resume session, and agent restart each close the old diff tab
  • Tested with Ollama
  • Tested with OpenRouter
  • Tested with OpenAI-compatible API
  • Tested MCP integration (if applicable)

Checklist

  • This was for an open issue, I was assigned to it
  • Code follows project style guidelines
  • Self-review completed
  • Documentation updated (if needed)
  • No breaking changes (or clearly documented)
  • Appropriate logging added using structured logging (see CONTRIBUTING.md)

… diffs

- Key pending diffs by toolCallId and file path so multiple diff blocks
  under one tool call no longer overwrite each other.
- Diffs opened from the chat no longer move focus to the terminal.
- Clear pending diffs and their editors on new chat, resume, and ACP
  reconnect.

Fixes Nano-Collective#1726
@github-actions github-actions Bot added the area:vscode VS Code extension and host integration label Oct 12, 2026
@github-actions

Copy link
Copy Markdown
Contributor

nc-review: needs work — 1 blocking, 2 important

@Piyushrathoree — there is a blocking item below.

The PR correctly fixes all three issues from #1726 (collision keying, focus stealing, missing session cleanup), the production logic is sound and matches the issue text, and the new spec file genuinely exercises each branch it targets. It is blocked, however, by a problematic changeset: the file attributes a VS Code-only fix to @nanocollective/nanocoder, declares a minor bump for a bug fix, and the body still reads 'patch bump' — the CHANGELOG entry that ships will be misattributed, the wrong severity, and largely content-free.

🔴 blocking · changeset · .changeset/itchy-banks-burn.md:2

The changeset declares "@nanocollective/nanocoder": minor for a pure bug fix in plugins/vscode/. Three problems compound:

  1. Wrong package: .changeset/config.json lists nanocoder-vscode under ignore, so by design the VS Code extension is not versioned from this repo. Every changed file in this PR lives under plugins/vscode/, so attributing the change to the main @nanocollective/nanocoder package will leak a VS Code bug-fix line into the main CLI's CHANGELOG.md and trigger a minor version bump on the wrong package. The project rubric notes: a wrong name passes the file-presence check and then breaks release-prepare on every subsequent push to main.
  2. Wrong bump type: The linked issue is labelled bug, the PR's own checklist marks it as a bug fix, and the changeset body itself reads 'patch bump' — so the frontmatter should be patch, not minor. CONTRIBUTING.md (and issue [Bug] Diff preview edge cases: collisions when notifications share tool IDs, terminal focus stealing, and missing session cleanup #1726) both call for patch for bug-fix-only changes.
  3. Empty body: The body is a single throwaway sentence and does not describe the user-facing behaviour for CHANGELOG.md consumers, contrary to CONTRIBUTING's 'the markdown body you write IS the changelog entry, verbatim.'

Fix by either (a) dropping the changeset entirely since the change is entirely inside the ignored nanocoder-vscode package and produces no CLI-facing change worth a release note, or (b) rewriting to attribute correctly and use patch with a substantive one-paragraph summary.

🟠 important · tests · plugins/vscode/src/diff-manager.spec.ts:92

The showDiff from chat does not move focus to the terminal test only covers the existing-files branch (originalContent !== ''). The same preserveTerminalFocus guard was added to the new-file branch in diff-manager.ts around the former line 139, and that branch is never exercised. A regression that flipped the guard back to the old if (activeTerminal) form in the new-file path would still pass this suite. Add a originalContent: '' case to the 'showDiff from chat does not move focus to the terminal' test (or a separate test) so both branches are guarded.

🟠 important · tests · plugins/vscode/src/diff-manager.spec.ts:124

clearPendingChanges drops every change and notifies listeners asserts notified > before. Because addPendingChange also notifies, before is whatever the count was after the loop, and the assertion depends on how many notifications happened during the adds. Two addPendingChange calls guarantee notified >= 2, but the test is only correct if every one of those notifications fired before before is captured — it happens to work today, but the assertion is more accidental than intended and would silently pass even if clearPendingChanges stopped calling notifyChanges() (as long as two add notifications had already occurred). Tighten to t.is(notified, 3) (2 adds + 1 clear) or capture the count immediately before the await.


🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional

Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.

Remove the itchy-banks-burn changeset: every change in this PR is inside
the ignored nanocoder-vscode package, so it has no CLI release note.

Add focus tests for the new-file branch of showDiff, which the existing
existing-file test did not exercise. Assert that clearPendingChanges
notifies exactly once instead of relying on the add-time count.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:needs-work nc-review found blocking findings area:vscode VS Code extension and host integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Diff preview edge cases: collisions when notifications share tool IDs, terminal focus stealing, and missing session cleanup

1 participant