From 0651ff97e5bc1d70c0202b32186df1500e343bed Mon Sep 17 00:00:00 2001 From: Gary Basin Date: Thu, 16 Jul 2026 17:44:14 -0400 Subject: [PATCH] refactor(web): the markup card takes the measured clamp, and the fold gutter gets a name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EntryQuoteCard hand-rolled the last unmeasured clamp in the app: a static `max-h` box that always offered "Show all changes" and always drew a fade, whether or not anything was actually hidden below the cut. It also sat one `position: static` away from #544 — CriticMarkupView emits an `sr-only` span, which under a static clamp anchors above the clip, escapes it, and inflates the scroll height. Adopting the clamp primitive fixes both: the control and the fade now appear only when the diff is measured to overflow, and the box is `relative` so the clip actually holds. The change count never lived on the button alone, so nothing is lost when it hides — the header still reads "N changes". Taking a second caller forced two latent bugs in the primitive into the open: - It measured through a ref in an effect keyed on [canClamp, expanded], so a caller mounting its clamped element on a later render attached the node silently and was never measured at all. EntryQuoteCard renders only once its markup fetch resolves, so its toggle could never have appeared. The node is state now, and the effect runs when it lands. - ClampBoundary was rebuilt on every render, making it a new component type each time and remounting everything clamped inside it — a whole message body — on every render. It is memoized on `nested` now. Both are pinned by tests that were A/B'd against the broken code first. WorkFold's gutter pair is unchanged, just named and explained: the two values are derived from each other (59 = 43 + 16) and silently drift apart when only one is edited. SessionCapabilitiesPopover is deliberately left alone; it truncates by item count, not by measured height, and has no clamp to unify. --- .../web/src/components/ClampedBlock.test.tsx | 52 ++++++++ surface/web/src/components/ClampedBlock.tsx | 119 +++++++++++++----- .../src/components/EntryQuoteCard.test.tsx | 70 +++++++++-- surface/web/src/components/EntryQuoteCard.tsx | 43 +++++-- surface/web/src/sessions/WorkFold.tsx | 17 ++- 5 files changed, 245 insertions(+), 56 deletions(-) diff --git a/surface/web/src/components/ClampedBlock.test.tsx b/surface/web/src/components/ClampedBlock.test.tsx index 09de5904..deb25e0b 100644 --- a/surface/web/src/components/ClampedBlock.test.tsx +++ b/surface/web/src/components/ClampedBlock.test.tsx @@ -1,6 +1,7 @@ // @vitest-environment jsdom import { act, cleanup, fireEvent, render, screen } from '@testing-library/react'; +import { useEffect } from 'react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { ClampedBlock } from './ClampedBlock'; @@ -118,3 +119,54 @@ describe('ClampedBlock', () => { expect(observers).toHaveLength(1); }); }); + +describe('ClampedBlock boundary identity', () => { + it('does not remount clamped content when the clamp re-renders', () => { + let mounts = 0; + function Child() { + useEffect(() => { + mounts += 1; + }, []); + return Child; + } + + const { rerender } = render( + + + , + ); + expect(mounts).toBe(1); + + rerender( + + + , + ); + + expect(mounts).toBe(1); + }); +}); + +describe('clamp containing blocks', () => { + // #544: `overflow: hidden` only clips descendants whose containing block runs + // through the clamping box. A `position: absolute` descendant (Tailwind's + // `sr-only`, which GFM footnotes and CriticMarkupView both emit) otherwise + // anchors to a positioned ancestor ABOVE the clamp, escapes the clip, and + // drags the scroll height into thousands of px of blank space. Every clamp in + // the app that can hold rendered markup must therefore be `relative`. + it('every collapsed clamp class in the app declares position: relative', async () => { + const sources = import.meta.glob('/src/**/*.tsx', { as: 'raw', eager: true }) as Record; + const offenders: string[] = []; + for (const [path, source] of Object.entries(sources)) { + if (path.includes('ClampedBlock')) continue; + // Matches both the primitive's prop and hand-rolled clamp class strings. + const clampClasses = source.match(/(?:collapsedClassName=")([^"]*)"/g) ?? []; + for (const match of clampClasses) { + const value = match.slice('collapsedClassName="'.length, -1); + const clips = value.includes('overflow-hidden') || value.includes('line-clamp-'); + if (clips && !value.split(/\s+/).includes('relative')) offenders.push(`${path}: ${value}`); + } + } + expect(offenders).toEqual([]); + }); +}); diff --git a/surface/web/src/components/ClampedBlock.tsx b/surface/web/src/components/ClampedBlock.tsx index ca606aa7..f61307bd 100644 --- a/surface/web/src/components/ClampedBlock.tsx +++ b/surface/web/src/components/ClampedBlock.tsx @@ -1,4 +1,12 @@ -import { createContext, useContext, useLayoutEffect, useRef, useState, type ReactNode } from 'react'; +import { + createContext, + useCallback, + useContext, + useLayoutEffect, + useState, + type ReactNode, + type RefCallback, +} from 'react'; const ClampContext = createContext(false); @@ -7,6 +15,79 @@ function classes(...values: Array): string | undefin return value || undefined; } +export interface Clamp { + /** Attach to the element that carries `collapsedClassName`. */ + contentRef: RefCallback; + /** True while the content is actually being clipped. */ + clamped: boolean; + expanded: boolean; + /** Only true once the content is measured to exceed the clamp. */ + overflows: boolean; + toggle: () => void; + /** Nesting guard for children — see ClampedBlock. */ + ClampBoundary: ({ children }: { children: ReactNode }) => ReactNode; +} + +/** + * The measuring half of ClampedBlock, for callers that must place the toggle + * themselves (EntryQuoteCard puts it in an action row beside Apply). Prefer + * ClampedBlock when the toggle can simply follow the content. + */ +export function useClamp(enabled = true): Clamp { + const insideClamp = useContext(ClampContext); + const canClamp = enabled && !insideClamp; + // The node is state, not a ref, so callers may mount the clamped element on a + // later render than the hook — EntryQuoteCard renders it only once its markup + // fetch resolves. A ref would attach silently, leaving this effect keyed on + // deps that never change, so the content was never measured at all. + const [element, setElement] = useState(null); + const [expanded, setExpanded] = useState(false); + const [overflows, setOverflows] = useState(false); + + // Only measure while clamped. Expanded content does not overflow, so measuring + // it would incorrectly remove the control that lets the user collapse it. + useLayoutEffect(() => { + if (!element || !canClamp || expanded) return; + + const measure = () => setOverflows(element.scrollHeight > element.clientHeight + 1); + measure(); + + // jsdom and older browser shells do not provide ResizeObserver. + if (typeof ResizeObserver === 'undefined') return; + const observer = new ResizeObserver(measure); + observer.observe(element); + return () => observer.disconnect(); + }, [canClamp, element, expanded]); + + const nested = insideClamp || canClamp; + // Memoized because React keys elements by component identity: a boundary + // rebuilt each render would be a new type every time, remounting everything + // clamped inside it (a whole message body) on every render. + const ClampBoundary = useCallback( + ({ children }: { children: ReactNode }) => {children}, + [nested], + ); + + return { + contentRef: setElement, + clamped: canClamp && !expanded, + expanded, + overflows: canClamp && overflows, + toggle: () => setExpanded((value) => !value), + ClampBoundary, + }; +} + +/** + * A block that clips its content and offers a toggle only once it measurably + * overflows. The clamp is published on context, so a ClampedBlock rendered + * inside another one declines to clamp: nesting two clips made the outer one + * bite only after the inner was released, so "Show more" SHRANK the text. + * + * `collapsedClassName` must carry `relative` whenever the content can hold an + * absolutely positioned descendant — `overflow: hidden` only clips descendants + * whose containing block runs through this box. + */ export function ClampedBlock({ children, collapsedClassName, @@ -37,30 +118,7 @@ export function ClampedBlock({ overflowingClassName?: string; toggleClassName?: string; }) { - const insideClamp = useContext(ClampContext); - const canClamp = enabled && !insideClamp; - const contentRef = useRef(null); - const [expanded, setExpanded] = useState(false); - const [overflows, setOverflows] = useState(false); - - // Only measure while clamped. Expanded content does not overflow, so measuring - // it would incorrectly remove the control that lets the user collapse it. - useLayoutEffect(() => { - const element = contentRef.current; - if (!element || !canClamp || expanded) return; - - const measure = () => setOverflows(element.scrollHeight > element.clientHeight + 1); - measure(); - - // jsdom and older browser shells do not provide ResizeObserver. - if (typeof ResizeObserver === 'undefined') return; - const observer = new ResizeObserver(measure); - observer.observe(element); - return () => observer.disconnect(); - }, [canClamp, expanded]); - - const clamped = canClamp && !expanded; - const showToggle = canClamp && overflows; + const { contentRef, clamped, expanded, overflows, toggle, ClampBoundary } = useClamp(enabled); return ( <> @@ -72,15 +130,10 @@ export function ClampedBlock({ clamped && overflows && overflowingClassName, )} > - {children} + {children} - {showToggle ? ( - ) : null} diff --git a/surface/web/src/components/EntryQuoteCard.test.tsx b/surface/web/src/components/EntryQuoteCard.test.tsx index 9c79851a..a05bca77 100644 --- a/surface/web/src/components/EntryQuoteCard.test.tsx +++ b/surface/web/src/components/EntryQuoteCard.test.tsx @@ -1,12 +1,38 @@ // @vitest-environment jsdom -import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { act, cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { EntryInlineChip, EntryQuoteCard } from './EntryQuoteCard'; import { clearEntryResolveCacheForTests, type ResolvedEntryQuote } from '../lib/entryLinks'; const resolveEntryMock = vi.hoisted(() => vi.fn()); +const observers: MockResizeObserver[] = []; + +// The clamp measures once on mount and then re-measures through a ResizeObserver, +// which jsdom does not implement. Markup arrives after the mount measurement, so +// growing the card only reaches the clamp through the observer. +class MockResizeObserver { + constructor(private readonly callback: ResizeObserverCallback) { + observers.push(this); + } + + observe(_target: Element) {} + + disconnect() {} + + trigger() { + this.callback([], this as unknown as ResizeObserver); + } +} + +function setMetrics(element: Element, scrollHeight: number, clientHeight: number) { + Object.defineProperties(element, { + scrollHeight: { configurable: true, value: scrollHeight }, + clientHeight: { configurable: true, value: clientHeight }, + }); +} + vi.mock('../api', () => ({ api: { resolveEntry: resolveEntryMock, @@ -37,6 +63,8 @@ function entry(overrides: Partial = {}): ResolvedEntryQuote beforeEach(() => { clearEntryResolveCacheForTests(); resolveEntryMock.mockReset(); + observers.length = 0; + vi.stubGlobal('ResizeObserver', MockResizeObserver); }); afterEach(() => { @@ -103,17 +131,16 @@ describe('entry links', () => { expect(screen.getByRole('link').getAttribute('href')).toBe('/e/evt_404'); }); - it('upgrades small markup artifacts into a tracked-changes card with clamp and expand', async () => { - const headers = new Headers({ - 'Content-Type': 'text/markdown; charset=utf-8', - 'Content-Length': '180', - 'X-Artifact-Seq': '4', - }); + function renderMarkupCard() { vi.stubGlobal( 'fetch', vi.fn().mockResolvedValue({ ok: true, - headers, + headers: new Headers({ + 'Content-Type': 'text/markdown; charset=utf-8', + 'Content-Length': '180', + 'X-Artifact-Seq': '4', + }), text: vi .fn() .mockResolvedValue( @@ -130,7 +157,7 @@ describe('entry links', () => { }), ); - render( + return render( { })} />, ); + } + + it('upgrades small markup artifacts into a tracked-changes card', async () => { + renderMarkupCard(); expect(screen.getAllByText('memo.md').length).toBeGreaterThan(0); expect(await screen.findByText('Edited memo')).toBeTruthy(); expect(screen.getByText('markup')).toBeTruthy(); - expect(screen.getByText('Show all changes (2)')).toBeTruthy(); expect(screen.getByText('old').className).toContain('atrium-critic-view-del'); expect(screen.getByText('new').className).toContain('atrium-critic-view-ins'); + // The toggle is measured, not assumed: a diff this short leaves nothing + // hidden, so neither a control nor a fade is offered for it. + expect(screen.queryByRole('button', { name: /Show all changes/ })).toBeNull(); + expect(screen.queryByTestId('markup-clamp-fade')).toBeNull(); + }); + + it('measures the clamp even though the markup mounts after the fetch resolves', async () => { + renderMarkupCard(); + await screen.findByText('Edited memo'); + + const clamped = screen.getByText('old').closest('.overflow-hidden'); + expect(clamped).toBeTruthy(); + setMetrics(clamped!, 600, 314); + act(() => { + for (const observer of observers) observer.trigger(); + }); + expect(screen.getByTestId('markup-clamp-fade')).toBeTruthy(); fireEvent.click(screen.getByRole('button', { name: 'Show all changes (2)' })); expect(screen.getByRole('button', { name: 'Show fewer changes' })).toBeTruthy(); + // Expanding releases the clip and the fade, so the whole diff is reachable. + expect(screen.getByText('old').closest('.overflow-hidden')).toBeNull(); + expect(screen.queryByTestId('markup-clamp-fade')).toBeNull(); }); it('leaves non-markup artifact cards on the excerpt renderer', async () => { diff --git a/surface/web/src/components/EntryQuoteCard.tsx b/surface/web/src/components/EntryQuoteCard.tsx index 963082a6..b6012442 100644 --- a/surface/web/src/components/EntryQuoteCard.tsx +++ b/surface/web/src/components/EntryQuoteCard.tsx @@ -10,6 +10,7 @@ import { resolveEntryQuote, type ResolvedEntryQuote } from '../lib/entryLinks'; import { attachmentMetaToPreviewFile } from '../lib/previewFiles'; import { LEGACY_UNFURL_COLLAPSED_STORAGE_KEY, readWithLegacy, UNFURL_COLLAPSED_STORAGE_KEY } from '../storageKeys'; import { ApplyMarkupMenu } from './ApplyMarkupMenu'; +import { useClamp } from './ClampedBlock'; import { CriticMarkupView } from './CriticMarkupView'; import { FileIcon } from './icons'; import { Lightbox, type PreviewFile } from './media'; @@ -461,7 +462,9 @@ export function EntryQuoteCard({ const contextApply = useContext(EntryQuoteApplyContext); const effectiveApplyContext = applyContext === undefined ? contextApply : applyContext; const markup = useMarkupArtifact(entry); - const [expanded, setExpanded] = useState(false); + // The toggle lives in the action row beside Apply, so this takes the clamp's + // measuring half and places the control itself. + const changes = useClamp(); const storageKey = messageEventId != null ? `${messageEventId}:${entry.handle}` : null; const [collapsed, setCollapsed] = useState( () => storageKey != null && collapsedUnfurlStorageKeys().includes(storageKey), @@ -520,24 +523,42 @@ export function EntryQuoteCard({
-
- +
+ + +
- {!expanded ? ( + {/* Measured, so the fade means what it looks like: a diff that already + fits used to be veiled as though something were hidden below it. */} + {changes.clamped && changes.overflows ? (
- + {changes.overflows ? ( + + ) : ( + + )} {effectiveApplyContext ? (