Skip to content

fix(desktop): improve file navigation, go to line and word wrap - #62543

Open
colthreepv wants to merge 2 commits into
anomalyco:devfrom
colthreepv:fix/go-to-codeline
Open

colthreepv wants to merge 2 commits into
anomalyco:devfrom
colthreepv:fix/go-to-codeline

Conversation

@colthreepv

@colthreepv colthreepv commented Nov 7, 2026 •

Copy link
Copy Markdown

Issue for this PR

Closes #13430
Closes #16781

Both were auto-closed for inactivity. File links now open successfully, but their line destinations still need accurate scrolling; the file-viewer wrap toggle requested in #16781 is also missing.

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

  • Center a linked line or short range using rendered row bounds instead of a fixed 24px-per-line estimate. For virtualized files, bring the target into the render window first and correct its position as wrapped rows finish measuring. Tall ranges align at their first line.
  • Let a new link destination override saved tab scroll; release navigation when the user interacts or leaves the view.
  • Add a word-wrap icon toggle to the file viewer on desktop and web. Its client-local preference persists across conversations. The existing mobile-diff setting and chat wrapping are unchanged.

The toggle uses the real viewer's wrap/scroll options. Recreating the code component on a wrap change avoids stale virtual-row measurements. The Codicons artwork includes CC BY 4.0 attribution.

No feature flag: this is a navigation correction and an explicit opt-in control for an existing rendering option; default wrapping stays unchanged. The new toolbar affordance is included here for design review while this PR is a draft.

How did you verify your code works?

  • bun run check passed (existing repository warnings remain).
  • bun run lint:changed passed.
  • Link-parsing and scroll-machine tests: 54 passed.
  • Built-app regression measures actual row positions and checks exact range highlighting, wrap on/off, and tall-range alignment.
  • Navigation and existing file-reopen/scroll-preservation cases: 10/10 runs passed with five workers and no retries. The navigation regression also passed after replacing the text button with the icon.

Visual

<svg xmlns="http://www.w3.org/2000/svg" width="240" height="72" viewBox="0 0 240 72">
  <style>@keyframes reveal{0%,100%{opacity:.2}40%,75%{opacity:1}}.target{animation:reveal 3s ease-in-out infinite}@media(prefers-reduced-motion:reduce){.target{animation:none}}</style>
  <g fill="none" stroke="currentColor">
    <rect x="9" y="7" width="222" height="58" rx="5" opacity=".25"/>
    <path d="M21 20h105M21 52h105" opacity=".25"/>
    <rect class="target" x="17" y="28" width="152" height="16" rx="2" fill="currentColor" fill-opacity=".12" stroke-opacity=".4"/>
    <path d="M21 36h132"/>
    <path d="M187 25h23a5 5 0 0 1 0 10h-17m4-4-4 4 4 4M187 45h15" stroke-linecap="round" stroke-linejoin="round"/>
  </g>
</svg>

Screenshots / recordings

Before/after captures are still to be attached before marking this draft ready for review.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

Added by the opencode bot (AI-generated).

Related: #58037

@colthreepv

Copy link
Copy Markdown
Author

Verification update for commits dcc5b7c796 and 7c0954e04a.

Automated checks

  • bun run check: passed, all 34 tasks successful; 550 existing repository warnings and no errors.
  • bun run lint:changed: passed.
  • Link parsing and scroll-machine unit tests: 54 passed.
  • Built-app navigation and file-reopen/scroll-preservation regressions: 10/10 runs passed with five workers and no retries.
  • The targeted built-app navigation regression also passed after replacing the text wrap button with the Codicons icon.
  • The browser regression checks actual rendered-row geometry, exact range highlighting, wrap on/off, and tall-range alignment, rather than only checking that a file tab opens.

Live desktop checks

Exercised the development desktop app through Windows desktop automation (computer-use), using real clicks, wheel scrolling, tab changes, and pane resizing. Links targeted this branch's packages/gui-plugins/src/file/view.tsx through absolute file URLs.

Scenario Observed result
First file opening at line 300 Line highlighted and visually centered.
New destination at line 500 with the file already open New destination reached and visually centered.
Short range 300–305 Exactly those six lines highlighted and visually centered.
Tall range 200–400 View starts at line 200 rather than trying to center the whole range.
Manual scrolling after following a link User can scroll away without being pulled back to the linked destination.
Switch to another file, then return through the original tab Manual position retained, with approximately line 258 at the top.
Narrow viewer with wrap enabled Long lines visibly wrap; following the short-range link still centers the rendered range.
Narrow viewer with wrap disabled Long lines stay on one row; following the short-range link still centers the range.

Live positioning checks above were visual observations; precise geometry assertions are covered by the browser regression. No app/server restart or code changes were needed during the live checks.

Before/after screenshots or a recording are still pending; this PR remains a draft.

@colthreepv
colthreepv marked this pull request as ready for review November 7, 2026 10:12

@opencodejr opencodejr Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This works as described for wrapped files. On the base, a link to long.txt#L100 lands about 6,850px from its line. With this change the line is centred and the range is highlighted. I checked this with the new e2e case on both checkouts and with screenshots, ran bun test src/file in gui-plugins (195 pass) and type-checked gui-plugins and session-ui with no errors. I did not test the path for virtualized files (over 500 KB).

Public API: none of the server, SDK, config or CLI changes. The new word-wrap value in the GUI plugin IconName and the optional FileSearchHandle.reveal are both additions, so existing callers keep working.

Concerns:

  • The third argument of the viewer store is a from migration source, not a name (see the inline comment).
  • A link target is never released after it lands, so later layout changes keep snapping the view back to it (inline).
  • Toggling wrap reapplies a scroll offset measured under the other mode, so the view lands on a different line (inline).
  • #58037 (open) also replaces the (line - 4) * 24 estimate in renderer.tsx and adds a review.spec.ts case for wrapped files. The two overlap and will conflict; this one also handles virtualized files and adds the toggle. It would help to coordinate with that PR, or note that this one supersedes it.

The before/after screenshots below come from the e2e run. Design review of the new toolbar row is still up to the team.

Related: #58037

Before

After


export default Plugin.define({
stores: {
viewer: Store.global(Schema.Struct({ wrap: Schema.Boolean }), { wrap: true }, "viewer"),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The third argument of Store.global is from, the older storage keys to import a value from once. It is not the store's name; the key in stores already names it (plugin.<id>.viewer). Passing "viewer" makes the host try to import from a bare app key viewer, which never shipped. packages/gui-plugins/AGENTS.md asks not to add a migration for a key that never shipped. Can the argument be dropped?

setHasScroller(true)

// Wrapped virtual rows reconcile their heights after drawing. Re-measure an active link target after that layout.
const observer = new ResizeObserver(() => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Once a link target has been centred, it stays active until the user presses a pointer or key, or the view hides. Until then, every resize of the content (this observer) and every scroll event (line 219) re-centres on it. The view keeps snapping back to the link long after it landed, for example when an agent edit rereads the file, a comment annotation opens, or the panel is resized by dragging its handle (that pointerdown is outside the scroller). Could the target be released once a measurement lands on the same position again, or after the first rendered pass of a virtualized file, rather than staying active indefinitely?

aria-pressed={ctx.stores.viewer.value.wrap}
icon={<Icon name="word-wrap" size="normal" />}
onClick={() => {
scrollSync.interacted()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

interacted() clears the target, and the keyed Show then recreates the viewer. Its onRendered restores the saved y, which is a pixel offset measured with the other wrap mode. Wrapped and unwrapped rows differ a lot in height, so toggling wrap moves you to a different part of the file. Could you record the top visible line before the toggle and reveal it after the remount, instead of restoring the old pixel offset?

This branch has not been deployed

No deployments
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.

[FEATURE]: Toggle Word Wrap in File Viewer (Desktop) [FEATURE]:[Web]: Clickable file:line references in messages (jump to file and line)

1 participant