Repository navigation
fix(ui): restore prompt rail scroll tracking #4417
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
981ebb5
c153875
3b34ec0
198137f
6af2c7e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,6 +131,102 @@ function notifyTranscriptScrolled(page: Page): Promise<void> { | |
| }); | ||
| } | ||
|
|
||
| interface ActivePromptRailSnapshot { | ||
| currentIds: string[]; | ||
| expectedId: string | null; | ||
| sourceTurnId: string | null; | ||
| } | ||
|
|
||
| async function activePromptRailSnapshot(page: Page): Promise<ActivePromptRailSnapshot> { | ||
| return page.evaluate(async ({ promptCount }) => { | ||
| const root = document.querySelector<HTMLElement>('[data-chat-scroll-container="true"]'); | ||
| if (!root) throw new Error('the chat scroll container is missing'); | ||
| const ticks = [...document.querySelectorAll<HTMLElement>('.maka-prompt-rail-tick')]; | ||
| const currentIds = ticks | ||
| .filter((tick) => tick.getAttribute('aria-current') === 'true') | ||
| .map((tick) => tick.dataset.promptTurnId ?? ''); | ||
| const rootBounds = root.getBoundingClientRect(); | ||
| const atEnd = root.scrollHeight - root.scrollTop - root.clientHeight <= 2; | ||
| const turns = [...root.querySelectorAll<HTMLElement>('[data-transcript-turn-id]')] | ||
| .map((turn) => ({ | ||
| element: turn, | ||
| id: turn.dataset.transcriptTurnId ?? '', | ||
| index: Number(turn.dataset.transcriptTurnId?.split('-').at(-1)) - 1, | ||
| })) | ||
| .filter((turn) => turn.id.length > 0 && Number.isFinite(turn.index)); | ||
| const readingBandTurns = turns | ||
| .filter(({ element }) => { | ||
| const bounds = element.getBoundingClientRect(); | ||
| return bounds.bottom > rootBounds.top | ||
| && bounds.top < rootBounds.top + rootBounds.height * 0.34; | ||
| }) | ||
| .sort((left, right) => left.index - right.index); | ||
| const scrollportTurns = turns | ||
| .filter(({ element }) => { | ||
| const bounds = element.getBoundingClientRect(); | ||
| return bounds.bottom > rootBounds.top && bounds.top < rootBounds.bottom; | ||
| }) | ||
| .sort((left, right) => left.index - right.index); | ||
| const sourceTurn = atEnd | ||
| ? turns.reduce<typeof turns[number] | null>( | ||
| (latest, turn) => latest === null || turn.index > latest.index ? turn : latest, | ||
| null, | ||
| ) | ||
| : readingBandTurns[0] ?? scrollportTurns[0] ?? null; | ||
| const expectedRailIndex = sourceTurn === null || ticks.length === 0 | ||
| ? null | ||
| : Math.round( | ||
| sourceTurn.index * (ticks.length - 1) / (promptCount - 1), | ||
| ); | ||
| const expectedId = expectedRailIndex === null | ||
| ? null | ||
| : ticks[expectedRailIndex]?.dataset.promptTurnId ?? null; | ||
| return { | ||
| currentIds, | ||
| expectedId, | ||
| sourceTurnId: sourceTurn?.id ?? null, | ||
| }; | ||
| }, { promptCount: PROMPT_RAIL_PROMPT_COUNT }); | ||
| } | ||
|
|
||
| async function expectPromptRailMatchesReadingPosition(page: Page): Promise<void> { | ||
| let lastSnapshot: ActivePromptRailSnapshot | null = null; | ||
| try { | ||
| await expect.poll(async () => { | ||
| lastSnapshot = await activePromptRailSnapshot(page); | ||
| return lastSnapshot.expectedId !== null | ||
| && lastSnapshot.currentIds.length === 1 | ||
| && lastSnapshot.currentIds[0] === lastSnapshot.expectedId; | ||
| }, { message: 'the one current tick maps from the Turn being read' }).toBe(true); | ||
| } catch { | ||
| throw new Error(`the prompt rail did not settle on the reading position: ${JSON.stringify(lastSnapshot)}`); | ||
| } | ||
| const snapshot = await activePromptRailSnapshot(page); | ||
| expect(snapshot.expectedId, `no visible Turn in ${JSON.stringify(snapshot)}`).not.toBeNull(); | ||
| expect(snapshot.currentIds).toEqual([snapshot.expectedId]); | ||
| } | ||
|
|
||
| async function scrollTranscriptThroughHistory(page: Page): Promise<void> { | ||
| for (let pageIndex = 0; pageIndex < PROMPT_RAIL_PROMPT_COUNT; pageIndex += 1) { | ||
| const firstBefore = await page.locator('[data-turn-id]').first().getAttribute('data-turn-id'); | ||
| if (firstBefore === 'turn-prompt-rail-1') return; | ||
| await page.evaluate(() => { | ||
| const root = document.querySelector<HTMLElement>('[data-chat-scroll-container="true"]'); | ||
| if (!root) throw new Error('the chat scroll container is missing'); | ||
| root.scrollTop = 0; | ||
| root.dispatchEvent(new WheelEvent('wheel', { deltaY: -120, bubbles: true })); | ||
| root.dispatchEvent(new Event('scroll')); | ||
| }); | ||
| await expect.poll(async () => | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 — this waits on a history page with the default 10 s while the sibling spec gives the same action 20 s.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aligned to 20 seconds in c153875. The history-load poll now reports its own timeout before the nested rail-position assertion runs. |
||
| page.locator('[data-turn-id]').first().getAttribute('data-turn-id'), { | ||
| message: `history loads before ${firstBefore}`, | ||
| }).not.toBe(firstBefore); | ||
| await waitForPaintedFrames(page); | ||
| await expectPromptRailMatchesReadingPosition(page); | ||
| } | ||
| throw new Error('the first prompt did not enter the active transcript range'); | ||
| } | ||
|
|
||
| test('every tick paints a bar with a real box', async ({ promptRailWindow: page }) => { | ||
| // Measured over ALL ticks, not a sample: a helper that skips what it cannot | ||
| // evaluate creates its blind spot exactly where a regression lives. | ||
|
|
@@ -270,6 +366,59 @@ test('the first click of a session lands on its prompt and holds', async ({ | |
| expect(settled?.tickIsCurrent).toBe(true); | ||
| }); | ||
|
|
||
| test('manual transcript scrolling keeps exactly the visible prompt current', async ({ | ||
| promptRailWindow: page, | ||
| }) => { | ||
| await page.setViewportSize({ width: 1_000, height: 700 }); | ||
| await scrollTranscriptTo(page, 'bottom'); | ||
| await expectPromptRailMatchesReadingPosition(page); | ||
| await expect(page.locator('.maka-prompt-rail-tick[aria-current="true"]')).toHaveCount(1); | ||
| await expect(page.locator('.maka-prompt-rail-tick').last()).toHaveAttribute( | ||
| 'aria-current', | ||
| 'true', | ||
| ); | ||
|
|
||
| await page.evaluate(() => { | ||
| const rail = document.querySelector('.maka-prompt-rail'); | ||
| if (!rail) throw new Error('the prompt rail is missing'); | ||
| const counts: number[] = []; | ||
| const record = () => counts.push( | ||
| rail.querySelectorAll('.maka-prompt-rail-tick[aria-current="true"]').length, | ||
| ); | ||
| const observer = new MutationObserver(record); | ||
| observer.observe(rail, { | ||
| attributes: true, | ||
| subtree: true, | ||
| attributeFilter: ['aria-current'], | ||
| }); | ||
| record(); | ||
| Object.assign(window, { | ||
| __makaPromptRailCurrentCounts: counts, | ||
| __makaPromptRailCurrentObserver: observer, | ||
| }); | ||
| }); | ||
|
|
||
| await scrollTranscriptThroughHistory(page); | ||
| await scrollTranscriptTo(page, 'top'); | ||
| await expectPromptRailMatchesReadingPosition(page); | ||
| await expect(page.locator('.maka-prompt-rail-tick[aria-current="true"]')).toHaveCount(1); | ||
| await expect(page.locator('.maka-prompt-rail-tick').first()).toHaveAttribute( | ||
| 'aria-current', | ||
| 'true', | ||
| ); | ||
|
|
||
| const currentCounts = await page.evaluate(() => { | ||
| const state = window as Window & { | ||
| __makaPromptRailCurrentCounts?: number[]; | ||
| __makaPromptRailCurrentObserver?: MutationObserver; | ||
| }; | ||
| state.__makaPromptRailCurrentObserver?.disconnect(); | ||
| return state.__makaPromptRailCurrentCounts ?? []; | ||
| }); | ||
| expect(currentCounts.length).toBeGreaterThan(1); | ||
| expect(currentCounts.every((count) => count === 1), currentCounts.join(',')).toBe(true); | ||
| }); | ||
|
|
||
| test('active transcript Turns keep stable DOM identities while scrolling', async ({ | ||
| promptRailWindow: page, | ||
| }) => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,8 @@ import { | |
| mergePromptAnchorRailTurns, | ||
| observeActivePromptRailVisibility, | ||
| PromptAnchorRail, | ||
| selectPromptRailActiveTurn, | ||
| selectPromptRailTickForMountedTurn, | ||
| type PromptRailFrameScheduler, | ||
| } from '../prompt-anchor-rail.js'; | ||
|
|
||
|
|
@@ -226,6 +228,120 @@ test('a jump gives the transcript back the moment the reader touches it', () => | |
| harness.restore(); | ||
| }); | ||
|
|
||
| const turnIndexById = new Map([ | ||
| ['turn-1', 0], | ||
| ['turn-2', 1], | ||
| ['turn-3', 2], | ||
| ['turn-4', 3], | ||
| ]); | ||
|
|
||
| test('the transcript tail selects the latest mounted Turn', () => { | ||
| assert.equal(selectPromptRailActiveTurn({ | ||
| atEnd: true, | ||
| mountedTurnIds: ['turn-3', 'turn-1', 'turn-4'], | ||
| readingBandTurnIds: ['turn-1'], | ||
| scrollportTurnIds: ['turn-1', 'turn-3'], | ||
| turnIndexById, | ||
| }), 'turn-4'); | ||
| }); | ||
|
|
||
| test('the reading band selects its earliest Turn by transcript order', () => { | ||
| assert.equal(selectPromptRailActiveTurn({ | ||
| atEnd: false, | ||
| mountedTurnIds: ['turn-1', 'turn-2', 'turn-3', 'turn-4'], | ||
| readingBandTurnIds: ['turn-4', 'turn-2', 'turn-3'], | ||
| scrollportTurnIds: ['turn-1', 'turn-2', 'turn-3', 'turn-4'], | ||
| turnIndexById, | ||
| }), 'turn-2'); | ||
| }); | ||
|
|
||
| test('an empty reading band falls back to the earliest Turn in the scrollport', () => { | ||
| assert.equal(selectPromptRailActiveTurn({ | ||
| atEnd: false, | ||
| mountedTurnIds: ['turn-1', 'turn-2', 'turn-3', 'turn-4'], | ||
| readingBandTurnIds: [], | ||
| scrollportTurnIds: ['turn-4', 'turn-3'], | ||
| turnIndexById, | ||
| }), 'turn-3'); | ||
| }); | ||
|
|
||
| test('no eligible Turn leaves the selection unresolved', () => { | ||
| assert.equal(selectPromptRailActiveTurn({ | ||
| atEnd: false, | ||
| mountedTurnIds: ['unknown-turn'], | ||
| readingBandTurnIds: [], | ||
| scrollportTurnIds: [], | ||
| turnIndexById, | ||
| }), null); | ||
| }); | ||
|
|
||
| test('an unsampled mounted Turn maps through the two nearest durable landmarks', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3 — this case describes a scenario the component cannot reach.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This scenario is reachable because the component does not receive a complete 120-entry prompt index: Desktop requests at most 64 landmarks. |
||
| const railTurns = Array.from({ length: 64 }, (_, railIndex) => { | ||
| const turnIndex = Math.round(railIndex * 119 / 63); | ||
| return { turnId: `turn-${turnIndex + 1}`, label: '', sequence: turnIndex * 2 }; | ||
| }); | ||
| assert.equal(selectPromptRailTickForMountedTurn({ | ||
| activeTurnId: 'turn-66', | ||
| mountedTurnIds: ['turn-66', 'turn-67', 'turn-68', 'turn-69'], | ||
| railTurns, | ||
| previousRailTurnId: 'turn-67', | ||
| }), 'turn-65'); | ||
| }); | ||
|
|
||
| test('uneven sequence gaps choose a nearby real landmark instead of a linear tick position', () => { | ||
| const railTurns = [ | ||
| { turnId: 'turn-a', label: '', sequence: 0 }, | ||
| { turnId: 'turn-b', label: '', sequence: 10 }, | ||
| { turnId: 'turn-c', label: '', sequence: 1_000 }, | ||
| { turnId: 'turn-d', label: '', sequence: 1_010 }, | ||
| ]; | ||
| assert.equal(selectPromptRailTickForMountedTurn({ | ||
| activeTurnId: 'active', | ||
| mountedTurnIds: ['turn-b', 'active', 'turn-c'], | ||
| railTurns, | ||
| previousRailTurnId: 'turn-b', | ||
| }), 'turn-c'); | ||
| }); | ||
|
|
||
| test('one-sided sequence extrapolation cannot skip past the adjacent tick', () => { | ||
| const railTurns = [ | ||
| { turnId: 'turn-a', label: '', sequence: 0 }, | ||
| { turnId: 'turn-b', label: '', sequence: 10 }, | ||
| { turnId: 'turn-c', label: '', sequence: 1_000 }, | ||
| { turnId: 'turn-d', label: '', sequence: 1_010 }, | ||
| ]; | ||
| assert.equal(selectPromptRailTickForMountedTurn({ | ||
| activeTurnId: 'active', | ||
| mountedTurnIds: ['active', 'turn-b', 'turn-c'], | ||
| railTurns, | ||
| previousRailTurnId: 'turn-d', | ||
| }), 'turn-a'); | ||
| }); | ||
|
|
||
| test('a window without a sampled wrapper preserves its previous current tick', () => { | ||
| assert.equal(selectPromptRailTickForMountedTurn({ | ||
| activeTurnId: 'active', | ||
| mountedTurnIds: ['active', 'neighbor'], | ||
| railTurns: [ | ||
| { turnId: 'turn-a', label: '', sequence: 0 }, | ||
| { turnId: 'turn-b', label: '', sequence: 10 }, | ||
| ], | ||
| previousRailTurnId: 'turn-a', | ||
| }), 'turn-a'); | ||
| }); | ||
|
|
||
| test('a window without landmarks replaces a stale current with a current rail tick', () => { | ||
| assert.equal(selectPromptRailTickForMountedTurn({ | ||
| activeTurnId: 'active', | ||
| mountedTurnIds: ['active'], | ||
| railTurns: [ | ||
| { turnId: 'turn-a', label: '', sequence: 0 }, | ||
| { turnId: 'turn-b', label: '', sequence: 10 }, | ||
| ], | ||
| previousRailTurnId: 'stale-turn', | ||
| }), 'turn-a'); | ||
| }); | ||
|
|
||
| test('keeps the active tick visible when the rail viewport resizes', () => { | ||
| let railBox = box(0, 600); | ||
| let tickBox = box(570, 590); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3 — this restates the production rule, so the mid-scroll assertion cannot disagree with it.
L151-179 re-implement tail / band / scrollport-fallback and the
round(i × 63 / 119)sampling, and L25 redeclaresMAX_PROMPT_RAIL_TICKS. I checked the sampling against L400-416 across all 120 indexes and it agrees today, but the moment the production sampling changes this goes red for no product reason, and while it agrees it only proves the implementation matches a copy of itself.The assertions that do carry weight are L375-379 (bottom → exactly the last tick), L404-408 (paged to Turn 1 → exactly the first tick), and the
MutationObservercardinality check. Those three cover #4415 and fail onmain. I'd keep them, drop the rule copy, and for the middle assert something weaker but independent: the current tick's index is within one of the topmost visible Turn's sampled index. Export the sampling function and the tick constant fromprompt-anchor-rail.tsxso the spec imports them instead of restating them.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am keeping the exact mid-scroll assertion because #4415 requires the current prompt to follow manual scrolling throughout the transcript; endpoint and cardinality assertions cannot catch an interior mapping regression.
The source Turn in the test is derived independently from rendered DOM geometry. Production derives it from IntersectionObserver membership and maps an unsampled mounted Turn through sequence anchors. The fixture has uniform sequences, so the test can compute the expected rendered tick without calling the production helper. Importing the production mapping would make this oracle less independent, while weakening it would leave the core regression uncovered.