The rail is a mode, not a screen size
Item 7 said to confirm before building, and confirming is what mattered. The rail's 348px threshold is measured against a fixed 720px column centred in the pane. The doc-list sidebar is 280px, so at her 1517px viewport the right margin is 258 with it open and 406 without — either side of the threshold. What moves between them is distraction-free mode, which engages on its own when the editor takes focus. The rail therefore appears when she starts writing and disappears when she stops; items 4 and 5 disagreed about whether it exists at 1517px only because they caught it in different states. Re-centring a fixed-width column changes its position and not its size, so the wrapper's ResizeObserver reported nothing and no window resize fired. railEnabled kept whatever value it last had. Leaving distraction-free with the rail up left a 300px column in a 266px margin: overhanging the viewport by 66px, cards clipped mid-sentence, the page scrolling sideways. Entering it with the rail down opened 406px of margin and put nothing in it. Both persisted until something else happened to resize the window. Observe the scrollport too — it spans the pane, so it resizes whenever the chrome around the editor does. That covers any future chrome that moves the editor, which threading focusMode down as a prop would not. Clicking a highlight now opens the anchored card even when the rail is up. That is the item's own acceptance criterion and was previously false by design; the measured distance from the first flagged span to its rail card is 651px, not the ~400 the review guessed. Hover still defers to the rail, since the reasoning against an unbidden second card was about hover and still holds — but a click is her asking to deal with that word. The rail card glows instead of expanding, so nothing is ever open twice. Verified in Chrome at the review's own 1517x810, driving the rule pack from item 3b so no model was involved: the rail follows the mode in both directions with no resize event anywhere; the popover lands 6px under the word with the full explanation, Ask Petal, Accept and Dismiss; accepting from it applied the edit and took the rail 6 cards to 5, leaving the rest with their ids, positions and wording intact. railFit.test.ts pins the threshold to the margins actually measured. The observer wiring has no unit test and can't have a useful one: jsdom has no layout, so every rect is zero and the rail branch is unreachable there. That half is browser-verified only, and the doc says so.
This commit is contained in:
prosolis
+117
-8
@@ -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
|
||||
|
||||
@@ -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<string>()
|
||||
const items: RailItem[] = []
|
||||
wrapper.querySelectorAll<HTMLElement>('.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,15 +700,15 @@ 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) {
|
||||
// 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)
|
||||
setRailExpandedId(id)
|
||||
} else {
|
||||
openCardFor(id, suggestionEl)
|
||||
}
|
||||
}
|
||||
return
|
||||
}
|
||||
if (!editor) return
|
||||
@@ -1210,7 +1225,7 @@ export function EditorCore({
|
||||
onAdd={addMisspellingToDict}
|
||||
/>
|
||||
)}
|
||||
{hover && !railEnabled && (
|
||||
{hover && (
|
||||
<SuggestionCard
|
||||
suggestion={hover.suggestion}
|
||||
style={{ top: hover.top, left: hover.left }}
|
||||
|
||||
@@ -0,0 +1,42 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import { RAIL_MIN_MARGIN, railFitsBeside } from './railFit'
|
||||
|
||||
// The numbers below are not invented: they were measured in Chrome at the UX
|
||||
// review's own 1517x810 viewport, with the 280px sidebar open and closed. They
|
||||
// are here so the two layouts stay distinguishable if the constant is ever tuned.
|
||||
describe('railFitsBeside', () => {
|
||||
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)
|
||||
})
|
||||
})
|
||||
@@ -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
|
||||
}
|
||||
Reference in New Issue
Block a user