Skip to content

Commit 6ddb427

Browse files
committed
Bugfix: Full view ".." row no longer hides behind header
- `FullList`'s scroll model dropped the spurious `+ headerHeight` translation between `scrollTop` and the spacer's scroll offset: `spacerScrollTop = scrollTop` and `scrollToIndex` writes `getScrollToPosition`'s result straight to `scrollContainer.scrollTop`. The earlier model shifted-then-clamped, collapsing `scrollTop ∈ [0, headerHeight]` to one spacer state, so PageDown ×2 → PageUp ×2 landed at `scrollTop === headerHeight` — row 0 fully under the sticky column header, cursor included - New Playwright regression spec `full-cursor-page-nav.spec.ts` walks the exact repro (PageDown ×2 → PageUp ×2 on a 120-file fixture) and pins `cursor.top >= header.bottom`, cursor inside the viewport, and `scrollTop === 0` at the top of the list. Confirmed failing on the un-fixed binary (`cursor.top=78` vs `header.bottom=100`, 22 px = headerHeight at scale 1) and green after the fix - Updated the `Decision` note in `views/CLAUDE.md` to describe the simpler `scrollTop === spacerScrollTop` model and link the regression spec
1 parent f3f4508 commit 6ddb427

3 files changed

Lines changed: 242 additions & 15 deletions

File tree

apps/desktop/src/lib/file-explorer/views/CLAUDE.md

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -80,13 +80,21 @@ not as a sibling above. **Why**: when the user has "Always show scrollbars" set
8080
non-overlay scrollbars steal a ~15 px gutter from the scroll container. A sibling header rendering at the wrapper's full
8181
width then misaligned with the data rows below. Moving the header inside makes it share the row content width
8282
automatically (and therefore the scrollbar gutter), so columns line up at every scrollbar mode without JS measurement.
83-
The trade-off is virtual-scroll math: row positions are now `headerHeight` pixels into the scrollable content, so
84-
`FullList` derives `spacerScrollTop = max(0, scrollTop - headerHeight)` and
85-
`rowAreaHeight = containerHeight - headerHeight` and feeds those into `calculateVirtualWindow` / `getScrollToPosition` /
86-
`firstVisibleGlobalIndex` / `lastVisibleGlobalIndex` / `getVisibleItemsCountUtil`. `scrollToIndex` adds `headerHeight`
87-
back when writing to `scrollContainer.scrollTop`. A11y: the listbox role moves off `.full-list` (now a generic scroll
88-
container) onto a `.listbox-region` inner wrapper around `.virtual-spacer` so the sticky header isn't a direct child of
89-
the listbox (would violate `aria-required-children`).
83+
Virtual-scroll math: the spacer follows the header in natural flow, so the spacer's content origin (row 0) sits
84+
`headerHeight` pixels into the unscrolled document. The sticky header always covers the first `headerHeight` pixels of
85+
the viewport once any scroll has happened, so the effective row area is `containerHeight - headerHeight`. Critically,
86+
`scrollTop` and the spacer's scroll offset are the same number — no translation needed. `FullList` therefore derives
87+
`spacerScrollTop = scrollTop` and `rowAreaHeight = containerHeight - headerHeight` and feeds those into
88+
`calculateVirtualWindow` / `getScrollToPosition` / `firstVisibleGlobalIndex` / `lastVisibleGlobalIndex` /
89+
`getVisibleItemsCountUtil`. `scrollToIndex` writes `getScrollToPosition`'s result straight to
90+
`scrollContainer.scrollTop`. A11y: the listbox role moves off `.full-list` (now a generic scroll container) onto a
91+
`.listbox-region` inner wrapper around `.virtual-spacer` so the sticky header isn't a direct child of the listbox (would
92+
violate `aria-required-children`).
93+
94+
The earlier version of this model shifted scrollTop by `headerHeight` and lossy-clamped at zero, which made
95+
`scrollTop ∈ [0, headerHeight]` collapse to the same spacer state. PageDown × 2 → PageUp × 2 then landed at
96+
`scrollTop === headerHeight`, hiding row 0 (including the `..` cursor) under the sticky header. See
97+
`test/e2e-playwright/full-cursor-page-nav.spec.ts` for the pinned regression.
9098

9199
**Decision**: Virtual scroll in frontend, data in backend **Why**: Sending 50k entries over IPC = 17.4MB, ~4s transfer.
92100
Virtual scroll fetches only visible ~50 items on demand. Backend-driven caching eliminates serialization overhead.

apps/desktop/src/lib/file-explorer/views/FullList.svelte

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -295,13 +295,18 @@
295295
let scrollContainer: HTMLDivElement | undefined = $state()
296296
let containerHeight = $state(0)
297297
let scrollTop = $state(0)
298-
// The header is a `position: sticky` element inside the scroll container,
299-
// so the row area starts `headerHeight` pixels into the scrollable content.
300-
// All virtual-scroll math operates in row-area-local coords: subtract the
301-
// header offset from `scrollTop` / `containerHeight` before passing through.
298+
// The header is `position: sticky; top: 0` and always covers the first
299+
// `headerHeight` pixels of the viewport once any scroll has happened, so
300+
// the effective row area is shorter than the container by that much. The
301+
// spacer is the header's next-sibling in natural flow, so `scrollTop`
302+
// already IS the spacer's scroll offset — no `- headerHeight` shift.
303+
// (The previous model shifted then clamped at 0, which collapsed
304+
// `scrollTop ∈ [0, headerHeight]` to a single spacer state and let the
305+
// "top of list" canonical scrollTop land at `headerHeight`, hiding row 0
306+
// under the sticky header.)
302307
let headerHeight = $state(0)
303308
const rowAreaHeight = $derived(Math.max(0, containerHeight - headerHeight))
304-
const spacerScrollTop = $derived(Math.max(0, scrollTop - headerHeight))
309+
const spacerScrollTop = $derived(scrollTop)
305310
306311
// ==== Virtual scrolling derived calculations ====
307312
const virtualWindow = $derived(
@@ -598,11 +603,12 @@
598603
// Exported for parent to call when arrow keys change cursor position
599604
export function scrollToIndex(index: number) {
600605
if (!scrollContainer) return
601-
// `getScrollToPosition` works in row-area coords. Translate back to the
602-
// scroll container's coordinate space by adding the sticky-header offset.
606+
// `getScrollToPosition` returns the spacer's required scroll offset in
607+
// row-area coords. Since `scrollTop === spacerScrollTop` (see the
608+
// sticky-header model note above), it's also the container's scrollTop.
603609
const spacerPos = getScrollToPosition(index, rowHeight, spacerScrollTop, rowAreaHeight)
604610
if (spacerPos !== undefined) {
605-
const newScrollTop = spacerPos + headerHeight
611+
const newScrollTop = spacerPos
606612
scrollContainer.scrollTop = newScrollTop
607613
// Also update state directly to trigger reactive chain immediately
608614
// (scroll events may be batched or delayed by the browser)
Lines changed: 213 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,213 @@
1+
/**
2+
* E2E regression test for Full view sticky-header cursor visibility.
3+
*
4+
* Repro: in Full view of a folder with enough entries to scroll past two pages,
5+
* press PageDown twice then PageUp twice. The cursor lands back on ".." (index
6+
* 0). Before the fix, scrollTop ended at exactly `headerHeight`, which placed
7+
* row 0 fully under the sticky column header — invisible, cursor included.
8+
*
9+
* The fix removes a spurious `+ headerHeight` translation in `FullList.svelte`:
10+
* the spacer's scroll offset is `scrollTop` directly, not `scrollTop -
11+
* headerHeight` clamped at 0. This test pins both the visual behavior and the
12+
* underlying scrollTop so the regression can't sneak back in.
13+
*/
14+
15+
import fs from 'fs'
16+
import path from 'path'
17+
import { test, expect } from './fixtures.js'
18+
import { ensureAppReady, executeViaCommandPalette, getFixtureRoot, pollUntil } from './helpers.js'
19+
20+
/** Subdir under the shared fixture root holding the files for this suite. */
21+
const FIXTURE_SUBDIR = 'full-page-nav-fixtures'
22+
23+
const FULL_LIST_SCROLL = '.file-pane.is-focused .full-list-container .full-list'
24+
const FULL_LIST_HEADER = '.file-pane.is-focused .full-list-container .header-row'
25+
const CURSOR_ENTRY = '.file-pane.is-focused .full-list-container .file-entry.is-under-cursor'
26+
27+
/** Sub-pixel rounding tolerance for getBoundingClientRect comparisons. */
28+
const PIXEL_TOLERANCE = 1
29+
30+
interface Rect {
31+
left: number
32+
top: number
33+
right: number
34+
bottom: number
35+
width: number
36+
height: number
37+
}
38+
39+
/**
40+
* Builds a 120-file fixture. 120 is enough to scroll past two PageDown jumps
41+
* at any sensible window height, and small enough that `recreateFixtures()`
42+
* elsewhere doesn't recreate it.
43+
*/
44+
function ensureFullPageNavFixture(): string {
45+
const fixtureRoot = getFixtureRoot()
46+
const dir = path.join(fixtureRoot, FIXTURE_SUBDIR)
47+
if (fs.existsSync(dir) && fs.readdirSync(dir).length >= 120) {
48+
return dir
49+
}
50+
fs.rmSync(dir, { recursive: true, force: true })
51+
fs.mkdirSync(dir, { recursive: true })
52+
for (let i = 0; i < 120; i++) {
53+
fs.writeFileSync(path.join(dir, `file-${String(i).padStart(3, '0')}.txt`), 'x')
54+
}
55+
return dir
56+
}
57+
58+
/** Disable CSS transitions so width/scroll animations don't race the reads. */
59+
async function disableTransitions(tauriPage: Parameters<typeof ensureAppReady>[0]): Promise<void> {
60+
await tauriPage.evaluate(`(function () {
61+
var existing = document.getElementById('cmdr-e2e-no-transitions');
62+
if (existing) return;
63+
var s = document.createElement('style');
64+
s.id = 'cmdr-e2e-no-transitions';
65+
s.textContent = '*, *::before, *::after { transition: none !important; animation: none !important; }';
66+
document.head.appendChild(s);
67+
})()`)
68+
}
69+
70+
/** Navigate the focused pane to a path via the mcp-nav-to-path Tauri event. */
71+
async function navigateFocusedPaneTo(
72+
tauriPage: Parameters<typeof ensureAppReady>[0],
73+
paneId: 'left' | 'right',
74+
targetPath: string,
75+
): Promise<void> {
76+
await tauriPage.evaluate(`(function () {
77+
var invoke = window.__TAURI_INTERNALS__.invoke;
78+
invoke('plugin:event|emit', {
79+
event: 'mcp-nav-to-path',
80+
payload: { pane: ${JSON.stringify(paneId)}, path: ${JSON.stringify(targetPath)} }
81+
});
82+
})()`)
83+
}
84+
85+
/** Read three rects + the scroll container's scrollTop in one round-trip. */
86+
async function readState(tauriPage: Parameters<typeof ensureAppReady>[0]): Promise<{
87+
cursor: Rect | null
88+
header: Rect | null
89+
scrollContainer: Rect | null
90+
scrollTop: number
91+
cursorName: string
92+
}> {
93+
return tauriPage.evaluate<{
94+
cursor: Rect | null
95+
header: Rect | null
96+
scrollContainer: Rect | null
97+
scrollTop: number
98+
cursorName: string
99+
}>(`(function () {
100+
function toRect(el) {
101+
if (!el) return null;
102+
var r = el.getBoundingClientRect();
103+
return { left: r.left, top: r.top, right: r.right, bottom: r.bottom, width: r.width, height: r.height };
104+
}
105+
var cursor = document.querySelector(${JSON.stringify(CURSOR_ENTRY)});
106+
var header = document.querySelector(${JSON.stringify(FULL_LIST_HEADER)});
107+
var scroll = document.querySelector(${JSON.stringify(FULL_LIST_SCROLL)});
108+
return {
109+
cursor: toRect(cursor),
110+
header: toRect(header),
111+
scrollContainer: toRect(scroll),
112+
scrollTop: scroll ? scroll.scrollTop : -1,
113+
cursorName: cursor ? (cursor.getAttribute('data-filename') || '') : '',
114+
};
115+
})()`)
116+
}
117+
118+
/** Read the current cursor row's filename. */
119+
async function getCursorName(tauriPage: Parameters<typeof ensureAppReady>[0]): Promise<string> {
120+
return tauriPage.evaluate<string>(
121+
`(function () { var e = document.querySelector(${JSON.stringify(CURSOR_ENTRY)}); return e ? (e.getAttribute('data-filename') || '') : ''; })()`,
122+
)
123+
}
124+
125+
/**
126+
* Press a key on the focused pane and wait until the cursor row's filename
127+
* actually changes — proves the keystroke was processed.
128+
*/
129+
async function pressAndWaitCursorChange(
130+
tauriPage: Parameters<typeof ensureAppReady>[0],
131+
key: string,
132+
expectedName?: string,
133+
): Promise<void> {
134+
const before = await getCursorName(tauriPage)
135+
await tauriPage.keyboard.press(key)
136+
await pollUntil(
137+
tauriPage,
138+
async () => {
139+
const name = await getCursorName(tauriPage)
140+
if (expectedName !== undefined) return name === expectedName
141+
return name !== before
142+
},
143+
2000,
144+
)
145+
}
146+
147+
test.describe('Full view sticky-header cursor visibility', () => {
148+
test.beforeAll(() => {
149+
ensureFullPageNavFixture()
150+
})
151+
152+
test('cursor on ".." stays visible after PageDown ×2, PageUp ×2', async ({ tauriPage }) => {
153+
await ensureAppReady(tauriPage)
154+
await disableTransitions(tauriPage)
155+
156+
const fixtureDir = path.join(getFixtureRoot(), FIXTURE_SUBDIR)
157+
await navigateFocusedPaneTo(tauriPage, 'left', fixtureDir)
158+
await pollUntil(
159+
tauriPage,
160+
async () =>
161+
tauriPage.evaluate<boolean>(`!!document.querySelector('.file-pane.is-focused [data-filename="file-000.txt"]')`),
162+
5000,
163+
)
164+
165+
// Switch to Full view via command palette.
166+
await executeViaCommandPalette(tauriPage, 'Full view')
167+
await pollUntil(tauriPage, async () => tauriPage.isVisible(FULL_LIST_HEADER), 5000)
168+
169+
// Cursor starts on "..". Confirm before running the repro.
170+
await pollUntil(tauriPage, async () => (await getCursorName(tauriPage)) === '..', 3000)
171+
172+
// PageDown × 2 — cursor walks two pages down into the listing.
173+
await pressAndWaitCursorChange(tauriPage, 'PageDown')
174+
await pressAndWaitCursorChange(tauriPage, 'PageDown')
175+
176+
// PageUp × 2 — cursor comes back to "..".
177+
await pressAndWaitCursorChange(tauriPage, 'PageUp')
178+
await pressAndWaitCursorChange(tauriPage, 'PageUp', '..')
179+
180+
// Sample state. The interesting reads:
181+
// - cursor row exists and is on "..",
182+
// - cursor row's top is at or below the header's bottom (not hidden under it),
183+
// - cursor row's bottom is at or above the scroll container's bottom (still in viewport),
184+
// - scrollTop is 0 (canonical "top of list" state).
185+
const state = await readState(tauriPage)
186+
187+
expect(state.cursor, 'cursor row should be rendered').not.toBeNull()
188+
expect(state.header, 'header row should be rendered').not.toBeNull()
189+
expect(state.scrollContainer, 'scroll container should be rendered').not.toBeNull()
190+
if (!state.cursor || !state.header || !state.scrollContainer) return
191+
192+
expect(state.cursorName, 'cursor should be back on ".."').toBe('..')
193+
194+
// Primary assertion: the cursor row top sits at or below the header's bottom.
195+
// Before the fix, cursor.top was ~0 px above the viewport (under the sticky header).
196+
expect(
197+
state.cursor.top,
198+
`cursor.top (${String(state.cursor.top)}) must be >= header.bottom (${String(state.header.bottom)}) — row is hidden under the sticky header`,
199+
).toBeGreaterThanOrEqual(state.header.bottom - PIXEL_TOLERANCE)
200+
201+
// The cursor row must also be inside the scroll container's viewport.
202+
expect(
203+
state.cursor.bottom,
204+
`cursor.bottom (${String(state.cursor.bottom)}) must be <= scrollContainer.bottom (${String(state.scrollContainer.bottom)})`,
205+
).toBeLessThanOrEqual(state.scrollContainer.bottom + PIXEL_TOLERANCE)
206+
207+
// Underlying invariant: at the top of the list, scrollTop is exactly 0.
208+
// Before the fix this was `headerHeight` (~22 px at scale 1), which is what
209+
// covered row 0. Pinning it at 0 documents the canonical state and gives
210+
// a clean failure message if the math regresses.
211+
expect(state.scrollTop, `scrollTop must be 0 at top of list, got ${String(state.scrollTop)}`).toBe(0)
212+
})
213+
})

0 commit comments

Comments
 (0)