diff --git a/UX_REVIEW_2026-07-27.md b/UX_REVIEW_2026-07-27.md index 12a82a8..dda27fc 100644 --- a/UX_REVIEW_2026-07-27.md +++ b/UX_REVIEW_2026-07-27.md @@ -531,6 +531,99 @@ whichever half is missing. Accept, without needing the rail; hovering a rail card glows its span and vice versa. +### 7 — DONE (seventh session). The rail isn't a screen-size fact; it's a mode. + +The item says to confirm first and fix whichever half is missing. Confirming +first is what mattered, because the interesting defect wasn't either half. + +**Settled first: the contradiction items 4 and 5 left behind.** The fifth +session measured rail cards at 1517px; the sixth found no rail at all at the +same width and wrote down a margin of 258. Both were right. The editor is a +fixed 720px column centred in the pane, and the doc-list sidebar is 280px, so +at 1517px the right margin is **258 with the sidebar open and 406 without** — +either side of the rail's 348 threshold. What moves between them is +distraction-free mode, which engages *on its own* the moment the editor takes +focus. So the rail is not a property of her screen. It appears when she starts +writing and disappears when she stops, and both sessions had simply caught it +in different states. + +**The bug that fell out of that.** `recomputeRail` was triggered by a +ResizeObserver on the wrapper, a window `resize`, or a change to the suggestion +set. Entering or leaving distraction-free mode is none of the three: the +wrapper is a fixed 720px column, so re-centring it changes its *position* and +never its *size*, and a ResizeObserver reports only size. `railEnabled` therefore +kept whatever value it last had. + +Leaving distraction-free with the rail up is the bad direction, and it is not +subtle — measured in Chrome at 1517×810: the 300px column stayed mounted in the +266px margin the restored sidebar left behind, **overhanging the viewport by +66px**, cards clipped mid-sentence ("use "an": "a…"), and the page grew a +horizontal scrollbar it never has otherwise. The other direction is only a loss: +she starts typing, the margin opens to 406, and no rail arrives. Both persisted +indefinitely — dispatching a lone `resize` event was enough to correct either, +which is what proved the measurement was the only thing missing. + +**Implemented:** + +- `EditorCore.tsx` — the ResizeObserver now watches `.petal-scrollport` as well + as the wrapper. The scrollport spans the pane, so it resizes whenever the + chrome around the editor does; the wrapper, being fixed-width, never does. It + is the element the sticky-pin code already reaches for, so it needed no new + handle, and unlike threading `focusMode` down as a prop it also covers any + future chrome that moves the editor. +- `railFit.ts` — `RAIL_MIN_MARGIN` and `railFitsBeside` lifted out of the + measurement callback. A bare `>=` doesn't need a name; this one earns it, + because the number picks between two entirely different suggestion surfaces + and the margin it reads moves for reasons unrelated to window size. +- `EditorCore.tsx` — **clicking a highlight now opens the anchored card even + when the rail is up**, which is the item's own acceptance criterion and was + previously false by design. Hover still defers to the rail: an unbidden + floating card next to a margin card saying the same thing is noise, and that + earlier reasoning was about hover and still holds. A click isn't. The rail card + glows instead of expanding, so the suggestion is never open in two places, and + a click-opened card keeps its glow after the pointer leaves (it closes on a + click away) so the margin and the open card don't disagree about what she's + reading. + +**Measured, not estimated.** The item guessed ~400px of eye travel from +underline to rail card. At 1517px in distraction-free mode the real distance +from the first flagged span's right edge to its card is **651px**. After the +change the card lands 6px under the word. + +**Verified in a real browser at the review's own 1517×810**, driving the local +build with the rule pack from item 3b so no model or VPN was involved: + +- *Rail follows the mode, with no resize event anywhere.* Click into the prose → + sidebar collapses, margin 406, rail mounts with its cards, no overflow. Escape + → sidebar restores, margin 258, rail unmounts, no overflow, no horizontal + scroll. Re-focus → it comes back. Re-run after the `railFit` extraction. +- *Click with the rail up.* Popover opens flush under "a apple" (6px gap, left + edges aligned), carrying the type pill, the diff, the full explanation, Ask + Petal, Accept and Dismiss; it fits the viewport; exactly one rail card glows + and **none is expanded**. +- *Accept from that popover.* Text became "an apple", the popover closed, the + rail went 6 cards → 5, and the other four kept their id, position and wording + — item 2's stability holding under a path it hadn't been exercised on. +- *The two halves the item asked about were already fine.* Span hover lights its + rail card, card hover lights its span (both directions, checked via the + `-active` classes). And with the rail off, clicking an underline already gave + an anchored popover — richer than the item's "one-line reason + more", since + it carries the whole explanation and Ask Petal. Nothing to build there. + +**Deliberately not done:** no "more" affordance linking the popover to a rail +card. The item imagined the popover as a teaser for the rail's fuller version; +there is no fuller version — both surfaces render the same explanation, and the +popover has Ask Petal too. Adding a control that expands a second copy of what +she is already reading would be the redundancy the hover rule exists to avoid. + +Coverage: `railFit.test.ts` pins the threshold to the margins actually measured +in Chrome — 406 fits, 258 and the mid-animation 266 don't, the bound is +inclusive, 1920-with-sidebar fits, narrow windows never do. **The observer wiring +itself has no unit test and can't have a useful one**: jsdom has no layout, so +every `getBoundingClientRect()` is zero, `railFitsBeside(0, 0)` is false, and the +rail branch is unreachable there. That half is browser-verified only, and is +written down as such rather than covered by a test that would pass regardless. + ## 8. Smaller items (each small, do opportunistically) - **Accept All per category.** Five tense fixes = five clicks today. Add @@ -611,6 +704,16 @@ renders invisibly, and at her actual viewport the rail is disabled — the inlin hover card is what she sees, which inverts item 7's premise. Untouched: 6, 7, 8, item 3's incremental half.)* +*(Seventh session: item 7 done — see the subsection under it. Two things there +are worth carrying forward. First, **the rail is a mode, not a screen size**: +distraction-free engages by itself on editor focus and moves the margin across +the rail's threshold, so "does she see the rail?" has no fixed answer at a given +width — items 4 and 5 disagreed only because they caught it in different states. +Second, the layout invariant that bit here is the same shape as the one item 4 +recorded: **a fixed-width column that gets re-centred changes position without +changing size**, and neither a ResizeObserver on it nor a window resize will say +so. Untouched: 6, 8, item 3's incremental half.)* + **Migration 0015 on the live database.** It rebuilds the suggestions table, so unlike 0014 it could have dropped her rows. Backed up first — and the backup had to be the whole WAL set (`petal.db`, `-wal`, `-shm` in @@ -625,14 +728,20 @@ CHECK, and every existing row still carrying the label she has already read (the seven `clarity` rows include the mislabelled Chinese one — by design, only new findings get the new type; if you want that card relabelled, edit the sentence). -**Suggested next (sixth session onward):** items 6, 7, 8 are all untouched -and all small; item 5's re-scoped Translate card type is the cheapest -*visible* win (see its Status note — the span is already detected and -already rendered in English, it's only mislabeled as Clarity), and -`internal/suggestions/translate.go` already exists — read it before -designing a new type. Item 3's incremental-surfacing half now has the -chunking it was waiting on, but still needs streaming, which the current -`/check` response shape doesn't do. +**Suggested next (seventh session onward):** **item 6** is the obvious pick — +it is the last small one, it is self-contained (prompt the answer path to reply +in both pair languages, then let the answer area grow to ~50vh instead of a +~100px scrollbox), and item 7 just made the surface it lives on more prominent: +Ask Petal now opens inside a card anchored at the word in *both* layouts, so a +cramped English-only answer is more visible than when the review was written. +Item 8's four are all still small and independent; **dismissal persistence** is +the one with real value now that item 2 gives suggestions stable identity across +checks. Item 3's incremental-surfacing half still needs streaming, which the +current `/check` response shape doesn't do — it remains the largest of what's +left. + +*(Superseded, kept for the reading list: the sixth session recommended item 5's +Translate card type, which is now done and live.)* *The advice below was written for the second session and is kept for its reading list, not its recommendation: item 3b is done and deployed.* It was diff --git a/web/src/components/Editor/EditorCore.tsx b/web/src/components/Editor/EditorCore.tsx index 0a5e317..26448bb 100644 --- a/web/src/components/Editor/EditorCore.tsx +++ b/web/src/components/Editor/EditorCore.tsx @@ -19,6 +19,7 @@ import { useCallback, useEffect, useLayoutEffect, useRef, useState } from 'react import { Toolbar } from '../Toolbar/Toolbar' import { SuggestionCard } from './SuggestionCard' import { SuggestionRail, type RailItem } from './SuggestionRail' +import { railFitsBeside } from './railFit' import { SuggestionHighlight, setSuggestions, setActiveSuggestion, findRange } from './SuggestionHighlight' import { SpellCheck, setSpellChecker, wordAt } from './SpellCheck' import { MisspellCard } from './MisspellCard' @@ -425,9 +426,7 @@ export function EditorCore({ const wrapper = wrapperRef.current if (!wrapper) return const wrapRect = wrapper.getBoundingClientRect() - // Need room for the 300px column + its 32px gutter (see .petal-rail), plus - // a little breathing space to the viewport edge. - setRailEnabled(window.innerWidth - wrapRect.right >= 348) + setRailEnabled(railFitsBeside(window.innerWidth, wrapRect.right)) const seen = new Set() const items: RailItem[] = [] wrapper.querySelectorAll('.petal-suggestion[data-suggestion-id]').forEach((el) => { @@ -448,11 +447,24 @@ export function EditorCore({ // Re-anchor when the suggestion set changes (after the decorations repaint), // and keep the rail in sync with viewport/editor width changes (room + reflow). + // + // The scrollport is observed as well as the wrapper, and it is not redundant: + // the wrapper is a fixed 720px column, so entering or leaving distraction-free + // mode *moves* it (the pane re-centres) without ever changing its size. A + // ResizeObserver on the wrapper alone reports nothing, no window resize fires, + // and `railEnabled` keeps whatever value it had — leaving the 300px rail + // rendered into the 266px margin a restored sidebar leaves behind, cards + // clipped mid-sentence and the page scrolling sideways. The scrollport spans + // the pane, so it resizes whenever the chrome around the editor does. useEffect(() => { recomputeRail() const wrapper = wrapperRef.current + const port = wrapper?.closest('.petal-scrollport') const ro = wrapper ? new ResizeObserver(() => recomputeRail()) : null - if (wrapper && ro) ro.observe(wrapper) + if (wrapper && ro) { + ro.observe(wrapper) + if (port) ro.observe(port) + } window.addEventListener('resize', recomputeRail) return () => { ro?.disconnect() @@ -590,12 +602,15 @@ export function EditorCore({ (e: React.MouseEvent) => { if (!(e.target as HTMLElement).closest('.petal-suggestion')) return if (railEnabled) { - setActiveId(null) + // A click-opened card outlives the pointer (it closes on a click away), + // so its rail card keeps the glow — otherwise the open card and the + // margin stop agreeing about which suggestion is being read. + if (!hover) setActiveId(null) return } scheduleClose() }, - [scheduleClose, railEnabled], + [scheduleClose, railEnabled, hover], ) const keepOpen = useCallback(() => clearTimeout(closeTimer.current), []) @@ -685,14 +700,14 @@ export function EditorCore({ if (suggestionEl) { const id = suggestionEl.getAttribute('data-suggestion-id') if (id) { - // With the rail open, a tap emphasizes and expands its margin card - // instead of opening a floating one. - if (railEnabled) { - setActiveId(id) - setRailExpandedId(id) - } else { - openCardFor(id, suggestionEl) - } + // Clicking a highlight always opens the card at the word. Hover still + // defers to the rail (see handleMouseOver) — an unbidden floating card + // beside a margin card that already says the same thing is noise. But a + // click is her asking to deal with *this* word, and answering it 650px + // away in the periphery is the gesture item 7 is about. The rail card + // glows rather than expanding, so the suggestion is never open twice. + setActiveId(id) + openCardFor(id, suggestionEl) } return } @@ -1210,7 +1225,7 @@ export function EditorCore({ onAdd={addMisspellingToDict} /> )} - {hover && !railEnabled && ( + {hover && ( { + it('fits in distraction-free mode, where the pane spans the window', () => { + // 1517px window, sidebar collapsed: the 720px column centres at left 391, + // so its right edge is 1111 and 406px of margin remain. + expect(railFitsBeside(1517, 1111)).toBe(true) + }) + + it('does not fit with the document list open at the same window size', () => { + // Same window, 280px sidebar in flow: the column re-centres to right 1259 and + // the margin falls to 258 — the measurement item 5 reported. This is the case + // that must return false; rendering the rail here overhangs the viewport. + expect(railFitsBeside(1517, 1259)).toBe(false) + }) + + it('rejects the mid-animation width too, not just the settled one', () => { + // The sidebar animates over 280ms, so the recompute can land on an + // intermediate margin (266 was observed one frame in). Anything under the + // threshold has to read as "no rail", or the column flickers back in. + expect(railFitsBeside(1517, 1251)).toBe(false) + }) + + it('treats the threshold as inclusive', () => { + expect(railFitsBeside(1000, 1000 - RAIL_MIN_MARGIN)).toBe(true) + expect(railFitsBeside(1000, 1000 - RAIL_MIN_MARGIN + 1)).toBe(false) + }) + + it('has room to spare on a wide desktop', () => { + // 1920px maximised, sidebar open: margin 468. + expect(railFitsBeside(1920, 1452)).toBe(true) + }) + + it('never fits on a narrow window, whatever the column does', () => { + expect(railFitsBeside(900, 810)).toBe(false) + expect(railFitsBeside(768, 744)).toBe(false) + }) +}) diff --git a/web/src/components/Editor/railFit.ts b/web/src/components/Editor/railFit.ts new file mode 100644 index 0000000..f0ef424 --- /dev/null +++ b/web/src/components/Editor/railFit.ts @@ -0,0 +1,19 @@ +// Whether the margin rail has room to sit beside the editor. +// +// The rail is a 300px column with a 32px gutter (see `.petal-rail` in index.css); +// RAIL_MIN_MARGIN adds a little breathing space to the viewport edge. Below it the +// editor falls back to the inline card anchored under the word. +// +// This is a bare comparison, but it earns a name: the number decides which of two +// entirely different suggestion surfaces she gets, and the margin it measures moves +// for reasons that have nothing to do with the window size. The editor is a fixed +// 720px column centred in the pane, so collapsing the 280px sidebar (distraction-free +// mode) re-centres it and changes this margin by 140px without resizing anything. +// See the ResizeObserver in EditorCore for the other half of that story. +export const RAIL_MIN_MARGIN = 348 + +// `wrapperRight` and `innerWidth` are both viewport coordinates — i.e. exactly +// `wrapper.getBoundingClientRect().right` and `window.innerWidth`. +export function railFitsBeside(innerWidth: number, wrapperRight: number): boolean { + return innerWidth - wrapperRight >= RAIL_MIN_MARGIN +}