Skip to content

vscode: align backlog issue-preview placement with builder-terminal count-then-pick pattern (remove focus side-effect) #1074

Description

@amrmelsayed

Problem

Three different "where does this open?" strategies are scattered across Codev's VS Code surfaces:

  1. Issue preview (view-issue.ts:173-174): force-focus group 1, then markdown.showPreviewToSide which resolves ViewColumn.Beside relative to the focused group.
  2. Builder / shell terminals (terminal-manager.ts:478-485): count groups, target ViewColumn.Two only if group 2 exists, else ViewColumn.One (the Bugfix #804 fix).
  3. Codev Markdown Preview for specs/plans/reviews (extension.ts:804): passes vscode.ViewColumn.Beside directly.

The issue preview strategy is the most fragile of the three because it depends on a focus side-effect rather than reading layout state directly:

  • It works only because the focus call happens first; if any future caller bypasses the focus step, Beside resolves to whatever group is active and previews chain into groups 3 / 4 / 5 (the exact failure mode the comment block in view-issue.ts:166-172 documents).
  • It steals focus to group 1 as a side effect, pulling the user away from wherever they were sitting (e.g. a builder terminal in group 2).
  • If group 1 has been closed and focusFirstEditorGroup resolves to the leftmost surviving group (not group 1 by ordinal), the preview lands "beside that," which may not be where the user expects.

The builder terminal strategy (hasSecondGroup = tabGroups.all.length >= 2; location = hasSecondGroup ? { viewColumn: Two } : { viewColumn: One }) is the cleaner of the two patterns: explicit, deterministic, no focus side-effect.

Proposal

Align the issue preview with the builder pattern. Replace the two-step focus-and-Beside dance in view-issue.ts:173-174 with an explicit count-then-pick:

// Before (today)
await vscode.commands.executeCommand('workbench.action.focusFirstEditorGroup');
await vscode.commands.executeCommand('markdown.showPreviewToSide', uri);

// After (this issue)
const hasSecondGroup = vscode.window.tabGroups.all.length >= 2;
const viewColumn = hasSecondGroup ? vscode.ViewColumn.Two : vscode.ViewColumn.One;
await vscode.commands.executeCommand('markdown.showPreview', uri, viewColumn, /* preserveFocus */ true);

markdown.showPreview (not markdown.showPreviewToSide) is the built-in command that accepts an explicit viewColumn argument. preserveFocus: true keeps focus where the user clicked instead of stealing it to the preview.

This gives the same user-visible behavior in the common case (preview lands in group 2 when a second group exists, otherwise group 1) without the focus side-effect, without depending on Beside semantics, and without the cross-surface inconsistency.

Plan-gate decisions

  1. Single shared helper or inline at each call site? Three call sites today (issue preview, builder/shell terminal, Codev Markdown Preview) could share a pickSiblingGroup(): vscode.ViewColumn helper. Lean: inline the same count-then-pick at the issue-preview call site for v1; extract to a helper if a fourth call site lands. Avoids a premature abstraction for a 3-line pattern.
  2. markdown.showPreview vs markdown.showPreviewToSide. The former takes an explicit viewColumn; the latter is Beside-only. Lean: switch to showPreview so the placement is explicit and testable.
  3. preserveFocus. Today the focus is stolen to group 1 first then handed to the preview tab. Lean: pass preserveFocus: true so the user stays where they clicked. Alternative: focus the preview after opening (today's behavior). Lean: preserve focus, matches builder-terminal terminal.show(!focus) convention (focus defaults to false).

Acceptance criteria

  • commands/view-issue.ts no longer calls workbench.action.focusFirstEditorGroup.
  • commands/view-issue.ts uses markdown.showPreview with an explicit viewColumn derived from vscode.window.tabGroups.all.length.
  • When a second editor group exists, the preview opens in group 2 (same as today's common case).
  • When only one editor group exists, the preview opens in group 1 (same as today's single-column case).
  • Focus is preserved on the click origin (the backlog row / sidebar), not stolen to the preview.
  • Repeat-clicking a backlog row while the preview is focused does not chain new groups (group 3, 4, 5...). Today's hack prevents this via the focus-to-group-1 side effect; the new explicit placement prevents it by construction.
  • Unit test for the viewColumn decision logic (mocked tabGroups.all.length returns 1 or 2; assert the expected ViewColumn).
  • No regression to the existing cache / refresh / close-cleanup behavior in view-issue.ts (the IssueContentProvider, OverviewCache.onDidChange subscription, onDidCloseTextDocument cleanup all stay).

Out of scope

Protocol selection

Small, well-scoped mechanical change with no design ambiguity beyond the three plan-gate items above (all locked leans). Roughly 20-40 LOC + one unit test.

  • BUGFIX is a poor fit (this is a quality / cross-surface-consistency improvement, not a fix for a user-visible bug).
  • AIR is the natural fit: small feature change, fully described in this issue, no spec/plan ceremony.

Related

Metadata

Metadata

Assignees

Labels

area/vscodeArea: VS Code extension

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions