Skip to content

Commit 0655dc0

Browse files
committed
Bugfix: File viewer word wrap no longer cuts off the file after ~60 lines
With word wrap on, files under 1 MB whose FIRST line was short (a README starting with `# Cmdr`, a log starting with a date) rendered blank space after roughly line 60, and scrolling to the bottom showed nothing: the end of the file was unreachable. Root cause, two stacked inaccuracies in the pretext height map (`viewer-line-heights.svelte.ts`): - `viewer-text-width.svelte.ts` measured the wrap width off the first `.line-text` span. That span is a flex item with no `flex-grow`, so it shrink-wraps to its own content: `# Cmdr` measures ~44px. Pretext then wrapped EVERY line at 44px, inflating the height map ~7x (12,348px spacer for ~1,800px of content on Cmdr's own README). The virtual scroll mapped positions through the inflated map while the DOM rendered real 18px rows, so the render window ended around line 60 and the rest of the scroll range was blank. Fix: derive the available width from the row geometry instead (scroll container `clientWidth` minus `.line` padding minus the gutter), which is also correct in no-wrap mode where the row itself is as wide as the widest line. - Pretext reports height 0 for empty lines, while the DOM renders every `.line` row at least one line tall (the gutter number keeps the row open). On a README with 35 blank lines the map under-counted by ~630px. Fix: clamp each line's height to `getLineHeight()` in `buildPrefixSum`. Tests (both seen red on the broken code first): - `viewer-text-width.svelte.test.ts`: the tracker returns the available row width (736px), not the first line's text width (44px), plus pure `computeAvailableTextWidth` cases. - `viewer-wordwrap-scroll.spec.ts` (E2E): with wrap on, scrolling to the bottom of a short-first-line file shows the last line, sampled across the height map's async ready flip. Verified live on the original README repro: spacer now matches the content, the last line lands exactly at the viewport bottom, and mid-file scrolling leaves no blank viewport.
1 parent 3ee8759 commit 0655dc0

6 files changed

Lines changed: 310 additions & 14 deletions

File tree

apps/desktop/src/routes/viewer/CLAUDE.md

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ FE primitives live at [`src/lib/file-viewer/CLAUDE.md`](../../lib/file-viewer/CL
1414
| `viewer-scroll.svelte.ts` | Virtual scroll composable: line cache, fetch debounce, scroll compression, effects |
1515
| `viewer-search.svelte.ts` | Search composable: start/poll/cancel/navigate, match highlighting, debounce, `useRegex` and `caseSensitive` toggles, regex-error projection |
1616
| `viewer-line-heights.svelte.ts` | Height map for accurate word-wrap scrolling via pretext (FullLoad files only) |
17-
| `viewer-text-width.svelte.ts` | `ResizeObserver`-driven tracker for the rendered `.line-text` width |
17+
| `viewer-text-width.svelte.ts` | `ResizeObserver`-driven tracker for the width available to line text (scroll container minus gutter and `.line` padding) |
1818
| `viewer-indexing-poll.ts` | Periodic `viewer_get_status` poll while the backend builds a line index |
1919
| `viewer-keyboard.ts` | Pure key helpers (`handleNavigationKey` / `handleToggleKey` / `handleSearchToggleKey` / `handleTailToggleKey`) plus `createViewerKeyboard`, the page's full keydown router (modifier shortcuts, Escape ladder, ⌘A, bare-key dispatch) |
2020
| `selection.svelte.ts` | Selection model: state + pure helpers (normalise, in-range, segment bounds, byte estimator) |
@@ -180,6 +180,16 @@ either duplicate work or risk a different result.
180180
- `runHeightMapInitEffect` guards with `if (heightMap.ready) return` to avoid re-preparing when only `textWidth`
181181
changes. Width-only changes are handled by `runHeightMapReflowEffect` via `reflow()` (instant) instead of re-running
182182
the async `prepareLines` pipeline. Without this guard, both effects would race on width changes.
183+
- **The height map's wrap width comes from the row geometry, never from a `.line-text` span.**
184+
`viewer-text-width.svelte.ts` computes it as the scroll container's `clientWidth` minus the `.line` padding and the
185+
gutter (`.line-number` width + margin). `.line-text` is a flex item with no `flex-grow`, so it shrink-wraps to its own
186+
content: measuring it on a file whose first line is short ("# Cmdr", ~44px) once fed a 44px wrap width to pretext,
187+
inflating the height map ~7x (blank space below ~line 60, end of the file unreachable). The `.line` row is no better:
188+
in no-wrap mode the `.lines-container` is `max-content`, so the row is as wide as the widest line. Pinned by
189+
`viewer-text-width.svelte.test.ts` and `viewer-wordwrap-scroll.spec.ts` (E2E).
190+
- **Pretext reports height 0 for empty lines; `buildPrefixSum` clamps each line to `getLineHeight()`.** The DOM renders
191+
every `.line` row at least one line tall (the gutter number keeps the row open), so without the clamp the height map
192+
under-counts by one row per empty line and the scroll mapping drifts on files with many blank lines.
183193
- `closeWindow()`'s `setTimeout(() => …, 0)` before `currentWindow.close()` is load-bearing — not decoration. Calling
184194
`close()` synchronously from inside a webview event handler runs webkit2gtk's destruction on the same GTK main-loop
185195
tick, stalling other webviews' IPC for an undefined duration. The settings page (`routes/settings/+page.svelte`'s

apps/desktop/src/routes/viewer/viewer-line-heights.svelte.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -147,11 +147,15 @@ export function createLineHeightMap() {
147147
if (!currentLayoutFn) return
148148
const n = preparedTexts.length
149149
const sums = new Float64Array(n + 1)
150+
const minHeight = getLineHeight()
150151
let acc = 0
151152
for (let i = 0; i < n; i++) {
152153
sums[i] = acc
153-
const result = currentLayoutFn(preparedTexts[i], maxWidth, getLineHeight())
154-
acc += result.height
154+
const result = currentLayoutFn(preparedTexts[i], maxWidth, minHeight)
155+
// Pretext reports height 0 for empty lines, but the DOM renders every
156+
// `.line` row at least one line tall (the gutter number keeps the row
157+
// open). Clamp so the prefix sum matches what's actually on screen.
158+
acc += Math.max(result.height, minHeight)
155159
}
156160
sums[n] = acc
157161
cumHeight = sums
Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
import { describe, it, expect, beforeEach, afterEach } from 'vitest'
2+
3+
import { computeAvailableTextWidth, createTextWidthTracker } from './viewer-text-width.svelte'
4+
5+
/** Stub rAF so tests can flush the tracker's deferred measurements synchronously. */
6+
let rafQueue: FrameRequestCallback[] = []
7+
let originalRaf: typeof requestAnimationFrame
8+
9+
function flushRaf(): void {
10+
const queue = rafQueue
11+
rafQueue = []
12+
for (const cb of queue) cb(performance.now())
13+
}
14+
15+
beforeEach(() => {
16+
originalRaf = globalThis.requestAnimationFrame
17+
globalThis.requestAnimationFrame = (cb: FrameRequestCallback): number => {
18+
rafQueue.push(cb)
19+
return rafQueue.length
20+
}
21+
})
22+
23+
afterEach(() => {
24+
globalThis.requestAnimationFrame = originalRaf
25+
rafQueue = []
26+
document.body.innerHTML = ''
27+
})
28+
29+
function makeRect(width: number): DOMRect {
30+
return {
31+
width,
32+
height: 18,
33+
top: 0,
34+
left: 0,
35+
right: width,
36+
bottom: 18,
37+
x: 0,
38+
y: 0,
39+
toJSON: () => ({}),
40+
}
41+
}
42+
43+
/**
44+
* Builds the viewer's content DOM with a SHORT first line ("# Cmdr", 44px of
45+
* text) inside a much wider viewport. Mirrors the regression case: the wrap
46+
* width must come from the row geometry, never from the first line's own
47+
* shrink-wrapped text span.
48+
*/
49+
function makeViewerDom({
50+
contentClientWidth,
51+
gutterWidth,
52+
firstLineTextWidth,
53+
}: {
54+
contentClientWidth: number
55+
gutterWidth: number
56+
firstLineTextWidth: number
57+
}): HTMLDivElement {
58+
const content = document.createElement('div')
59+
content.className = 'file-content'
60+
// happy-dom doesn't lay anything out; stub the scroll container's client width.
61+
Object.defineProperty(content, 'clientWidth', { value: contentClientWidth, configurable: true })
62+
63+
const line = document.createElement('div')
64+
line.className = 'line'
65+
line.style.paddingLeft = '8px'
66+
line.style.paddingRight = '8px'
67+
68+
const lineNumber = document.createElement('span')
69+
lineNumber.className = 'line-number'
70+
lineNumber.style.marginRight = '8px'
71+
lineNumber.getBoundingClientRect = () => makeRect(gutterWidth)
72+
73+
const lineText = document.createElement('span')
74+
lineText.className = 'line-text'
75+
lineText.textContent = '# Cmdr'
76+
lineText.getBoundingClientRect = () => makeRect(firstLineTextWidth)
77+
78+
line.append(lineNumber, lineText)
79+
content.appendChild(line)
80+
document.body.appendChild(content)
81+
return content
82+
}
83+
84+
describe('computeAvailableTextWidth', () => {
85+
it('subtracts line padding and the gutter from the container width', () => {
86+
expect(
87+
computeAvailableTextWidth({
88+
contentClientWidth: 800,
89+
linePaddingLeft: 8,
90+
linePaddingRight: 8,
91+
gutterOuterWidth: 48,
92+
}),
93+
).toBe(736)
94+
})
95+
96+
it('clamps to 0 when the gutter eats the whole width', () => {
97+
expect(
98+
computeAvailableTextWidth({
99+
contentClientWidth: 40,
100+
linePaddingLeft: 8,
101+
linePaddingRight: 8,
102+
gutterOuterWidth: 48,
103+
}),
104+
).toBe(0)
105+
})
106+
})
107+
108+
describe('createTextWidthTracker', () => {
109+
it('measures the available row width, not the first line text span (which shrink-wraps to its content)', () => {
110+
const content = makeViewerDom({ contentClientWidth: 800, gutterWidth: 40, firstLineTextWidth: 44 })
111+
const tracker = createTextWidthTracker({
112+
getContentRef: () => content,
113+
getVisibleLinesKey: () => 1,
114+
})
115+
116+
tracker.runVisibleLinesEffect()
117+
flushRaf()
118+
119+
// 800 (scroll container) - 8 - 8 (line padding) - 40 (gutter) - 8 (gutter margin) = 736.
120+
// Pre-fix this returned 44 (the width of "# Cmdr"), which made the word-wrap
121+
// height map wrap every line at 44px and inflate the scroll height ~7x.
122+
expect(tracker.textWidth).toBe(736)
123+
})
124+
})

apps/desktop/src/routes/viewer/viewer-text-width.svelte.ts

Lines changed: 44 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,17 @@
1-
/** Tracks the rendered width of a `.line-text` element inside the given content ref.
2-
* Used by the viewer's height-map logic to wrap lines correctly at the current width.
1+
/** Tracks the width available for line text inside the given content ref: the
2+
* scroll container's client width minus the `.line` row padding and the gutter
3+
* (line number + its margin). Used by the viewer's height-map logic to wrap
4+
* lines at the same width the browser wraps them.
5+
*
6+
* Never measure the `.line-text` span itself: it's a flex item with no
7+
* `flex-grow`, so it shrink-wraps to its own content. A file whose first
8+
* rendered line is short ("# Cmdr") would yield a ~44px "available width",
9+
* making the height map wrap every line at 44px and inflate the scroll
10+
* height ~7x (blank space below ~line 60, end of the file unreachable).
11+
* The `.line` row can't be measured either: in no-wrap mode the
12+
* `.lines-container` is `max-content`, so the row is as wide as the WIDEST
13+
* line. Deriving the width from the scroll container keeps the measurement
14+
* correct in both wrap modes.
315
*
416
* Exposed effects mirror the composable pattern used by `viewer-scroll.svelte.ts`:
517
* the page component wires them as `$effect`s so reactivity is preserved. */
@@ -9,13 +21,37 @@ interface TextWidthDeps {
921
getVisibleLinesKey: () => unknown
1022
}
1123

24+
/**
25+
* Width available for a line's text: the scroll container's content area minus
26+
* the `.line` row padding and the gutter (the `.line-number` outer width plus
27+
* its right margin). Clamped to 0 for degenerate inputs.
28+
*/
29+
export function computeAvailableTextWidth(args: {
30+
contentClientWidth: number
31+
linePaddingLeft: number
32+
linePaddingRight: number
33+
gutterOuterWidth: number
34+
}): number {
35+
const { contentClientWidth, linePaddingLeft, linePaddingRight, gutterOuterWidth } = args
36+
return Math.max(0, contentClientWidth - linePaddingLeft - linePaddingRight - gutterOuterWidth)
37+
}
38+
1239
export function createTextWidthTracker(deps: TextWidthDeps) {
1340
let textWidth = $state(0)
1441

1542
function measure(el: HTMLElement) {
16-
const lineText = el.querySelector('.line-text')
17-
if (!lineText) return
18-
const w = lineText.getBoundingClientRect().width
43+
const line = el.querySelector('.line')
44+
const lineNumber = el.querySelector('.line-number')
45+
if (!(line instanceof HTMLElement) || !(lineNumber instanceof HTMLElement)) return
46+
const lineStyle = getComputedStyle(line)
47+
const lineNumberStyle = getComputedStyle(lineNumber)
48+
const w = computeAvailableTextWidth({
49+
contentClientWidth: el.clientWidth,
50+
linePaddingLeft: Number.parseFloat(lineStyle.paddingLeft) || 0,
51+
linePaddingRight: Number.parseFloat(lineStyle.paddingRight) || 0,
52+
gutterOuterWidth:
53+
lineNumber.getBoundingClientRect().width + (Number.parseFloat(lineNumberStyle.marginRight) || 0),
54+
})
1955
if (w > 0 && Math.abs(w - textWidth) > 1) {
2056
textWidth = w
2157
}
@@ -41,18 +77,15 @@ export function createTextWidthTracker(deps: TextWidthDeps) {
4177
}
4278

4379
/** Re-measures when visible lines first appear: `ResizeObserver` won't fire if the
44-
* container size didn't change but the inner `.line-text` element just became
45-
* present in the DOM. */
80+
* container size didn't change but the inner `.line` row just became present in
81+
* the DOM. */
4682
function runVisibleLinesEffect() {
4783
void deps.getVisibleLinesKey()
4884
if (textWidth > 0) return
4985
requestAnimationFrame(() => {
5086
const ref = deps.getContentRef()
5187
if (!ref) return
52-
const lineText = ref.querySelector('.line-text')
53-
if (!lineText) return
54-
const w = lineText.getBoundingClientRect().width
55-
if (w > 0) textWidth = w
88+
measure(ref)
5689
})
5790
}
5891

apps/desktop/test/e2e-playwright/CLAUDE.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,7 @@ details.
153153
| `viewer-encoding-picker.spec.ts` | 1 test: switching encoding from UTF-16 LE to UTF-8 on a 6 MB file keeps the viewport interactive during the background rebuild. |
154154
| `viewer-tail.spec.ts` | 1 test: enabling tail mode auto-extends the viewport when the file grows on disk. Uses `fs.appendFile` to drive the FSEvents pipeline end to end. `test.describe.configure({ timeout: 30000 })` to absorb FSEvents debounce + BE coalescer. |
155155
| `viewer-wordwrap-persistence.spec.ts` | 1 test: word wrap toggled in one viewer session is on in the next. Exercises the restricted-window settings path (typed commands + main-window bridge) under the real capability ACL, which vitest can't reproduce. Polls the instance's `settings.json` (requires `CMDR_DATA_DIR`) so the reopen never races the 500 ms save debounce. |
156+
| `viewer-wordwrap-scroll.spec.ts` | 1 test: with word wrap on, scrolling to the bottom of a short-first-line file shows the last line. Regression pin for the height-map wrap width: measuring the first `.line-text` span (shrink-wrapped, ~44px for "# Cmdr") once inflated the scroll height ~7x and made the end of the file unreachable blank space. |
156157
| `mtp.spec.ts` | MTP E2E tests: volume selection, navigation, file ops, large file transfer via virtual device. Uses `e2e-shared/mcp-client.ts` (MCP client helper) and `e2e-shared/mtp-fixtures.ts` (MTP fixtures). Requires `virtual-mtp` feature. |
157158
| `mtp-conflicts.spec.ts` | MTP conflict resolution: cross-volume move (MTP↔local) and same-volume move (MTP→MTP) with overwrite/skip policies. Requires `virtual-mtp` feature. |
158159
| `mtp-same-volume-move-merge.spec.ts` | Same-volume rename-merge fast path: MTP→MTP folder move auto-merges (no folder prompt), a clashing file inside follows the file policy (prompts under Stop), and a dest-only file survives. MTP-backed (uses the real `move_within_same_volume` rename-merge), so it lives on the dedicated MTP shard. Requires `virtual-mtp` feature. |

0 commit comments

Comments
 (0)