From cc66489ca4cf47cb6cfecf89df066271e9af6afb Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Sat, 19 Sep 2026 12:15:51 -0700 Subject: [PATCH 01/19] fix(profile): stop the Activity gallery moving the reader, and unstack its bottom row (GEO-2974) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Switching Debates to Claims threw the reader up the page.** Measured on an iPhone 13 viewport against the live site: the document goes 1198px to 874px with the reader standing 389px down it, and 874px can only scroll to 210px — so the browser puts them there, 179px above where they were, settling at 121px. No scroll position survives that, because the page genuinely is shorter: a claim card is not as tall as a debate card. So the gallery holds its *height* instead, and lets go only once dropping the floor would not move anybody. The release condition is the whole fix, and a timer is not it. The first attempt lifted the floor after two frames and changed nothing, because the incoming cards keep growing for about a second and a half as their own queries land — 196px, then 217px, then 254px — so the floor was always gone before the document stopped moving. `canReleaseHeldHeight` asks the only question that matters instead: is the page still tall enough underneath this reader without it. Verified end to end — scrollY now holds at 389 through a switch in both directions, and the floor comes off the moment they scroll up, leaving them where they are. **The debater's name ran underneath the "Winner?" button.** The identity row was `absolute bottom-3 left-4` with no right bound and the vote button `absolute right-4 bottom-3` on top of it. There is room at the width a feed card gives a tile and none at the 312px a gallery card does, so on a phone the position chip read "Ag…" and "Disagr…" under a pill. They are one flex row now, which cannot overlap at any width. This affects every narrow rendering of the player, not just the gallery. **A card no longer stretches to the tallest in the row.** `items-stretch` gave short cards a border reaching far below their own content; `items-start` lets it hug what is in it. **Autoplay in a gated row asks an easier question.** Not reproduced headlessly — driving the scroller programmatically, the centred card played on all four swipes — so this is reasoning rather than a fix to a measured fault: `DebateExploreFeedCard` requires 0.6 of itself on screen to start, which exists to stop a feed of cards all playing at once. `DebatePlaybackGate` already prevents that by naming one card, so in a gated surface the ratio test is a second, stricter gate that can only subtract — a chosen card whose ratio sits in the dead band never starts, and tapping is the only way out. Gated cards now use 0.25/0.1, which is the question they actually need answered: am I on screen. Co-Authored-By: Claude Opus 5 (1M context) --- .../debates/browse/debate-feed-player.tsx | 68 +++++--- .../web/core/debates/debate-playback-gate.tsx | 18 ++ .../explore/debate-explore-feed-card.tsx | 26 ++- .../profile/activity-held-height.test.ts | 63 +++++++ .../profile/profile-activity-section.tsx | 158 ++++++++++++++++-- 5 files changed, 285 insertions(+), 48 deletions(-) create mode 100644 apps/web/partials/profile/activity-held-height.test.ts diff --git a/apps/web/core/debates/browse/debate-feed-player.tsx b/apps/web/core/debates/browse/debate-feed-player.tsx index 4d18c52596..c9ae733b55 100644 --- a/apps/web/core/debates/browse/debate-feed-player.tsx +++ b/apps/web/core/debates/browse/debate-feed-player.tsx @@ -318,35 +318,49 @@ function DebaterVideo({ )} - {/* Debater identity: avatar + name + position, opens their personal space in the side panel. */} - + {participant && ( - - {participant.position_label} - + votes.castVote(participant)} + /> )} - - - {participant && ( - votes.castVote(participant)} - /> - )} + {scrubber &&
{scrubber}
} diff --git a/apps/web/core/debates/debate-playback-gate.tsx b/apps/web/core/debates/debate-playback-gate.tsx index a3069d758a..88fb8a3e68 100644 --- a/apps/web/core/debates/debate-playback-gate.tsx +++ b/apps/web/core/debates/debate-playback-gate.tsx @@ -37,3 +37,21 @@ export function useDebatePlaybackAllowed(debateId: string): boolean { if (allowed === undefined) return true; return allowed !== null && ID.equals(allowed, debateId); } + +/** + * Whether a gate is deciding at all, which changes what a card's own judgement + * is *for*. + * + * Ungated, a card's intersection ratio answers "should I be the one playing" — + * it is the only thing standing between a feed of cards and all of them playing + * at once, so it is strict on purpose: 0.6 to start. + * + * Gated, that question is already answered, and a second stricter test can only + * subtract. A card the gate has chosen but whose ratio sits in the dead band + * never starts, and tapping is the reader's only way out — which is what a + * 520px card in a 664px viewport spends much of its time doing. So a gated card + * asks the easier question instead: am I on screen at all. + */ +export function useIsDebatePlaybackGated(): boolean { + return React.useContext(DebatePlaybackContext) !== undefined; +} diff --git a/apps/web/partials/explore/debate-explore-feed-card.tsx b/apps/web/partials/explore/debate-explore-feed-card.tsx index 8684352fe0..e0f6e2815d 100644 --- a/apps/web/partials/explore/debate-explore-feed-card.tsx +++ b/apps/web/partials/explore/debate-explore-feed-card.tsx @@ -7,7 +7,7 @@ import { DebateClaimsPanel } from '~/core/debates/browse/debate-claims-panel'; import { DebateFeedPlayer } from '~/core/debates/browse/debate-feed-player'; import { DebateShareDialog } from '~/core/debates/browse/share-dialog'; import { useDebateShareAction } from '~/core/debates/browse/use-debate-share-action'; -import { useDebatePlaybackAllowed } from '~/core/debates/debate-playback-gate'; +import { useDebatePlaybackAllowed, useIsDebatePlaybackGated } from '~/core/debates/debate-playback-gate'; import { useDebate, useDebateMedia } from '~/core/debates/hooks'; import { hasProcessedVideo, isWatchableDebate } from '~/core/debates/playback-utils'; import { useDebateTranscriptClaims } from '~/core/debates/use-debate-transcript-claims'; @@ -34,6 +34,16 @@ import { SpaceThumb } from './space-thumb'; const ACTIVATE_RATIO = 0.6; const DEACTIVATE_RATIO = 0.4; +/** + * The same hysteresis, asked of a card that is not competing for the turn. + * + * Where a gate names the one card allowed to play, this card's ratio only has to + * say whether it is worth playing *to* — so it is a visibility test rather than + * a contest, and the thresholds are what "on screen at all" means. + */ +const GATED_ACTIVATE_RATIO = 0.25; +const GATED_DEACTIVATE_RATIO = 0.1; + type DebateExploreFeedCardProps = { item: ExploreFeedItem; /** Hide the space thumbnail + space-name link in the meta row (same semantics as ExploreFeedCard). */ @@ -86,6 +96,12 @@ export function DebateExploreFeedCard({ // already was strictly between them. The lower edge is inclusive so that the observer's // report at the 0.4 threshold deactivates rather than landing ambiguously inside the band — // a ratio reported exactly at a threshold is the normal case, not an edge case. + // Which pair applies depends on whether anything else is deciding — see + // `useIsDebatePlaybackGated`. + const isGated = useIsDebatePlaybackGated(); + const activateAt = isGated ? GATED_ACTIVATE_RATIO : ACTIVATE_RATIO; + const deactivateAt = isGated ? GATED_DEACTIVATE_RATIO : DEACTIVATE_RATIO; + const [active, setActive] = React.useState(false); React.useEffect(() => { if (!container) return; @@ -94,17 +110,17 @@ export function DebateExploreFeedCard({ for (const entry of entries) { setActive(current => { if (!entry.isIntersecting) return false; - if (entry.intersectionRatio >= ACTIVATE_RATIO) return true; - if (entry.intersectionRatio <= DEACTIVATE_RATIO) return false; + if (entry.intersectionRatio >= activateAt) return true; + if (entry.intersectionRatio <= deactivateAt) return false; return current; }); } }, - { threshold: [DEACTIVATE_RATIO, ACTIVATE_RATIO] } + { threshold: [deactivateAt, activateAt] } ); observer.observe(container); return () => observer.disconnect(); - }, [container]); + }, [activateAt, container, deactivateAt]); // A veto, not a replacement: where a surface holds playback to one debate — // a row of cards, all of them fully on screen at once — this says whether it diff --git a/apps/web/partials/profile/activity-held-height.test.ts b/apps/web/partials/profile/activity-held-height.test.ts new file mode 100644 index 0000000000..3da3b6371f --- /dev/null +++ b/apps/web/partials/profile/activity-held-height.test.ts @@ -0,0 +1,63 @@ +import { describe, expect, it } from 'vitest'; + +import { canReleaseHeldHeight } from './profile-activity-section'; + +/** + * When the Activity gallery may stop holding its height (GEO-2974). + * + * Switching Debates to Claims swaps tall cards for short ones, which is + * legitimate — a claim card really is shorter. What is not is doing it while + * somebody is standing below where the page would then end. Measured on an + * iPhone 13 against the live site: the document goes 1198 to 874 with the reader + * 389 down it, and 874 can only scroll to 210, so the browser puts them there. + * + * No scroll position survives that, so the gallery holds its height instead and + * this decides when to let go. The numbers below are that measurement. + */ +const AT_THE_BOTTOM = { + heldHeight: 520, + contentHeight: 254, + documentHeight: 1198, + viewportHeight: 664, + scrollY: 389, +}; + +describe('canReleaseHeldHeight', () => { + /** + * The reader is 389 down a page that would end at 268 without the floor + * (1198 - 266 padding - 664 viewport). Letting go moves them 121px. + */ + it('holds while dropping the floor would move the reader', () => { + expect(canReleaseHeldHeight(AT_THE_BOTTOM)).toBe(false); + }); + + it('lets go once they have scrolled up out of the way', () => { + expect(canReleaseHeldHeight({ ...AT_THE_BOTTOM, scrollY: 120 })).toBe(true); + }); + + /** + * The ordinary way out: the incoming content grew past the floor, so the floor + * is adding nothing and there is nothing to lose by dropping it. + */ + it('lets go once the content is taller than the floor', () => { + expect(canReleaseHeldHeight({ ...AT_THE_BOTTOM, contentHeight: 600 })).toBe(true); + }); + + it('lets go when the content exactly matches the floor', () => { + expect(canReleaseHeldHeight({ ...AT_THE_BOTTOM, contentHeight: 520 })).toBe(true); + }); + + /** The boundary itself is safe: the reader lands exactly at the new bottom. */ + it('lets go at the exact offset where nothing moves', () => { + expect(canReleaseHeldHeight({ ...AT_THE_BOTTOM, scrollY: 268 })).toBe(true); + expect(canReleaseHeldHeight({ ...AT_THE_BOTTOM, scrollY: 269 })).toBe(false); + }); + + /** + * A tall enough page absorbs the loss. The reader is in the middle of a long + * document, so 266px off the bottom is not their problem. + */ + it('lets go on a page with room to spare below', () => { + expect(canReleaseHeldHeight({ ...AT_THE_BOTTOM, documentHeight: 4000 })).toBe(true); + }); +}); diff --git a/apps/web/partials/profile/profile-activity-section.tsx b/apps/web/partials/profile/profile-activity-section.tsx index dfe1241375..137a2b7833 100644 --- a/apps/web/partials/profile/profile-activity-section.tsx +++ b/apps/web/partials/profile/profile-activity-section.tsx @@ -88,6 +88,8 @@ export function ProfileActivitySection({ kinds }: { kinds: ActivityKind[] }) { // cannot shift the selection out from under them. const selected = available.find(kind => kind.key === selectedKey) ?? available[0]; + const { galleryRef, contentRef, heldHeight, holdHeight } = useHeldHeight(selected?.key); + // Nothing at all rather than an empty card. A heading over a blank space reads // as a page that failed to load, and most accounts have never been in a debate. if (kinds.some(kind => kind.isLoading) || available.length === 0 || !selected) return null; @@ -111,7 +113,11 @@ export function ProfileActivitySection({ kinds }: { kinds: ActivityKind[] }) { key={kind.key} type="button" aria-pressed={isSelected} - onClick={() => setSelectedKey(kind.key)} + onClick={() => { + // Before the swap, so there is a height to hold. + holdHeight(); + setSelectedKey(kind.key); + }} className={cx( 'inline-flex items-center gap-1.5 rounded-full border px-3 py-1 text-smallButton transition-colors', isSelected @@ -130,20 +136,38 @@ export function ProfileActivitySection({ kinds }: { kinds: ActivityKind[] }) { )} - {selected.isError && selected.rows.length === 0 ? ( - /* - * No retry here on purpose. This card is a summary; the tab its count - * links to holds the authoritative list and offers the retry, so a - * second control here would be a second thing to keep in step. - */ -

Couldn’t load {selected.label.toLowerCase()}.

- ) : ( - - )} + {/* + * The gallery keeps the height it had while the kinds are swapped. + * + * Measured on a phone: Debates puts the document at 1198px with the + * reader 389px down it; the moment Claims is picked it is 874px, which is + * shorter than where they were standing, so the browser clamps the scroll + * and throws them 179px up the page. A claim card really is shorter than a + * debate card, so the collapse is legitimate — what is not is doing it + * underneath somebody. + * + * So the swap happens at the old height and the height is released + * afterwards, by which point the reader is looking at the new cards rather + * than being moved past them. + */} +
+
+ {selected.isError && selected.rows.length === 0 ? ( + /* + * No retry here on purpose. This card is a summary; the tab its count + * links to holds the authoritative list and offers the retry, so a + * second control here would be a second thing to keep in step. + */ +

Couldn’t load {selected.label.toLowerCase()}.

+ ) : ( + + )} +
+
{shown.map(row => ( @@ -216,6 +240,108 @@ function ActivityGallery({ ); } +/** + * Whether the floor can come off without moving the reader. + * + * Two ways out, and the first is the ordinary one: the incoming content has + * grown past the floor, so the floor is adding nothing. Otherwise it is adding + * `padding`, and dropping it takes that much off the bottom of the page — safe + * only while the reader is above where the page would then end. + * + * A pure function because it is the whole rule, and because the alternative was + * a timer: an earlier version lifted the floor after two frames, which is a + * guess about when the content settles rather than an answer about whether it is + * safe. The incoming cards grew for about a second and a half. + */ +export function canReleaseHeldHeight({ + heldHeight, + contentHeight, + documentHeight, + viewportHeight, + scrollY, +}: { + heldHeight: number; + contentHeight: number; + documentHeight: number; + viewportHeight: number; + scrollY: number; +}): boolean { + const padding = heldHeight - contentHeight; + if (padding <= 0) return true; + + return scrollY <= documentHeight - padding - viewportHeight; +} + +/** + * Keeps a swapped region from collapsing out from under the reader. + * + * A claim card really is shorter than a debate card, so the region genuinely + * shrinks — what is not acceptable is doing it while somebody is standing below + * the new bottom of the page. Measured on a phone: Debates puts the document at + * 1198px with the reader 389px down it, and picking Claims takes it to 874px, + * whose furthest scroll is 210px. The browser has nowhere to put them but 179px + * up the page. + * + * There is no scroll position that survives that, so this holds the *height* + * instead: the region is floored at what it measured when the swap was asked + * for, and the floor comes off only once dropping it would not move anybody. + * + * **The release condition is the whole point**, and a timer is not it. An + * earlier version lifted the floor after two frames, which measured well and + * fixed nothing: the incoming cards keep growing for about a second and a half + * as their own queries land (196px, then 217px, then 254px), so the floor was + * always gone long before the document stopped moving. Instead this asks the + * only question that matters — is the page still tall enough underneath this + * reader without the floor — and keeps asking until the answer is yes. + */ +function useHeldHeight(selectedKey: string | undefined) { + /** The region that carries the floor. */ + const galleryRef = React.useRef(null); + /** The content inside it, which keeps its natural height so it can be measured. */ + const contentRef = React.useRef(null); + const [heldHeight, setHeldHeight] = React.useState(null); + + const holdHeight = React.useCallback(() => { + const height = contentRef.current?.getBoundingClientRect().height; + if (height) setHeldHeight(height); + }, []); + + React.useEffect(() => { + if (heldHeight === null) return; + + const check = () => { + const content = contentRef.current; + + const release = + !content || + canReleaseHeldHeight({ + heldHeight, + contentHeight: content.getBoundingClientRect().height, + documentHeight: document.documentElement.scrollHeight, + viewportHeight: window.innerHeight, + scrollY: window.scrollY, + }); + + if (release) setHeldHeight(null); + }; + + // Polled *and* on scroll: the content settles on its own schedule, and the + // reader scrolling up is the other way the answer turns yes. + const interval = window.setInterval(check, 250); + window.addEventListener('scroll', check, { passive: true }); + check(); + + return () => { + window.clearInterval(interval); + window.removeEventListener('scroll', check); + }; + // Re-armed by the key, so a second switch before the first released still + // measures and releases rather than being swallowed by the held value. + }, [heldHeight, selectedKey]); + + return { galleryRef, contentRef, heldHeight, holdHeight }; +} + /** * Which card is nearest the middle of the row. * From af98219038e28c336b83f217dfbc1932f214c02a Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Sat, 19 Sep 2026 12:56:46 -0700 Subject: [PATCH 02/19] feat(profile): report what the Activity gallery is doing on the device it is doing it on (GEO-2974) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three faults were reported from a phone and none of the three reproduces headlessly. That is worth stating precisely, because it is the finding: * Autoplay. Chromium played the centred card on all four swipes. WebKit — the engine both Safari and Chrome run on iOS — reported `paused: false`, the clock advancing, `muted: true`, `playsInline: true`, and `play()` resolving. * The page moving when the kinds are swapped. Held at 389 through a switch in both directions, and the floor released on scroll-up without moving anybody. * The gallery losing its place when a claim opens. Held at 315 with the panel open, over identical content — 6 cards, same widths. The gallery-reset probe did reproduce until I looked at what it was clicking: it took the *first* card's heading, which at that scroll offset is off-screen to the left, so the browser scrolled it into view. Correct behaviour, and identical to the report from the outside. Clicking the card actually on screen changes nothing. Worth recording as a warning about this kind of probe. So the things causing these are the things a headless engine does not have — iOS media policy, Low Power Mode, a toolbar that resizes the viewport as you scroll, real touch momentum — and guessing at fixes for symptoms nobody can reproduce is how three plausible changes get shipped and none of them help. This asks the phone instead. Behind a feature flag, the Activity card reports the scroll offsets, its own height, and each video's real state, plus how the last `play()` settled — the prototype is patched once so a refusal the app swallowed is still visible. The autoplay report is specific enough to name what to look for: tap once and a play button appears, tap again and it plays, which says the app believed it was already playing while the element was not. If the readout shows `PAUSED` while the card offers no play button, the initial `play()` was refused and the refusal never reached the UI. If it shows `playing` with `t` frozen, the element is running and not decoding. Those are different faults with different fixes, and one screenshot from the phone separates them. Co-Authored-By: Claude Opus 5 (1M context) --- apps/web/core/state/feature-flags.test.tsx | 3 + apps/web/core/state/feature-flags.ts | 11 ++ .../feature-flags-dialog.test.tsx | 1 + .../partials/profile/activity-diagnostics.tsx | 148 ++++++++++++++++++ .../profile/profile-activity-section.tsx | 5 + 5 files changed, 168 insertions(+) create mode 100644 apps/web/partials/profile/activity-diagnostics.tsx diff --git a/apps/web/core/state/feature-flags.test.tsx b/apps/web/core/state/feature-flags.test.tsx index 88a1f68ae7..9a891b3510 100644 --- a/apps/web/core/state/feature-flags.test.tsx +++ b/apps/web/core/state/feature-flags.test.tsx @@ -28,6 +28,7 @@ describe('feature flags', () => { expect(defaultFeatureFlags.exploreSidePanel).toBe(false); expect(defaultFeatureFlags.bountiesTab).toBe(true); expect(normalizeFeatureFlags(null)).toEqual({ + activityDiagnostics: false, debugDebatesPage: false, debateDebugging: false, debateFormatSelector: false, @@ -41,6 +42,7 @@ describe('feature flags', () => { // reaching the dialog would render a checkbox for a flag nothing reads. it('drops the retired claims-and-debates flags that are still in storage', () => { expect(normalizeFeatureFlags({ questionsTab: true, debatesTab: true, debateDebugging: true })).toEqual({ + activityDiagnostics: false, debugDebatesPage: false, debateDebugging: true, debateFormatSelector: false, @@ -65,6 +67,7 @@ describe('feature flags', () => { // serialize in is incidental — it follows the definition list, and pinning it here would fail // on a reordering that changes nothing a reader could notice. expect(JSON.parse(window.localStorage.getItem(featureFlagsStorageKey) ?? 'null')).toEqual({ + activityDiagnostics: false, debugDebatesPage: true, debateDebugging: true, debateFormatSelector: true, diff --git a/apps/web/core/state/feature-flags.ts b/apps/web/core/state/feature-flags.ts index 0864d6f925..329240b5e4 100644 --- a/apps/web/core/state/feature-flags.ts +++ b/apps/web/core/state/feature-flags.ts @@ -20,6 +20,13 @@ export const featureFlagDefinitions = [ description: 'Allow the first matched debater to choose a format before accepting.', enabledByDefault: false, }, + { + id: 'activityDiagnostics', + label: 'Activity gallery diagnostics', + description: + "Show what the profile's Activity gallery is doing — scroll offsets, its height, and each video's real playback state. For faults that only happen on a phone (GEO-2974).", + enabledByDefault: false, + }, { id: 'debugDebatesPage', label: 'Debates debug tab per space', @@ -111,6 +118,10 @@ export function useDebugDebatesPageEnabled() { return useFeatureFlag('debugDebatesPage'); } +export function useActivityDiagnosticsEnabled() { + return useFeatureFlag('activityDiagnostics'); +} + /** * Read *and* write, for the flags dialog. Deliberately not hydration-gated like * {@link useFeatureFlag}: the dialog's contents are inside a Radix `Root` that is closed until a diff --git a/apps/web/partials/feature-flags/feature-flags-dialog.test.tsx b/apps/web/partials/feature-flags/feature-flags-dialog.test.tsx index 0e2ceba8ee..f3480b47e6 100644 --- a/apps/web/partials/feature-flags/feature-flags-dialog.test.tsx +++ b/apps/web/partials/feature-flags/feature-flags-dialog.test.tsx @@ -57,6 +57,7 @@ describe('FeatureFlagsDialog', () => { await waitFor(() => { // Values, not key order — see the note in `feature-flags.test.ts`. expect(JSON.parse(window.localStorage.getItem(featureFlagsStorageKey) ?? 'null')).toEqual({ + activityDiagnostics: false, debugDebatesPage: true, debateDebugging: true, debateFormatSelector: true, diff --git a/apps/web/partials/profile/activity-diagnostics.tsx b/apps/web/partials/profile/activity-diagnostics.tsx new file mode 100644 index 0000000000..3df8299549 --- /dev/null +++ b/apps/web/partials/profile/activity-diagnostics.tsx @@ -0,0 +1,148 @@ +'use client'; + +import * as React from 'react'; + +/** + * What the Activity gallery is actually doing, on the device it is doing it on + * (GEO-2974). + * + * Three faults were reported from a phone — autoplay that needs two taps, the + * page moving when the kinds are swapped, and the gallery losing its place when + * a claim opens. None of the three reproduces headlessly: Chromium played every + * card on every swipe, WebKit reported `paused: false` with the clock advancing + * and `play()` resolving, and both kept their scroll positions through a switch + * and through opening the panel. + * + * That is not evidence the faults are imagined. It is evidence that the things + * causing them are the things headless engines do not have — iOS media policy, + * Low Power Mode, a toolbar that resizes the viewport as you scroll, real touch + * momentum. So rather than keep guessing at fixes for symptoms nobody can + * reproduce, this reports the state that would tell them apart, on the phone + * where they happen. + * + * **What to look for, for the autoplay report specifically.** The reported + * sequence — tap once and a play button appears, tap again and it plays — says + * the app believed it was already playing while the element was not: the first + * tap *paused* something. So the row to read is `paused` against `app`. If + * `paused: true` shows while the card claims to be playing, the initial `play()` + * was refused and the refusal never reached the UI, and the fix is to make the + * player's visible state follow the element rather than the intention. If + * `paused: false` with `t` frozen, the element is running and not decoding, + * which is a different fix entirely. + * + * Behind a flag rather than a query param so it survives the navigation into a + * side panel and back, which is one of the things being measured. + */ +type VideoState = { + paused: boolean; + muted: boolean; + inline: boolean; + ready: number; + t: number; + err: number | null; +}; + +type Snapshot = { + pageY: number; + galleryX: number | null; + galleryH: number | null; + docH: number; + viewportH: number; + videos: VideoState[]; + lastPlay: string | null; +}; + +/** Set by the patch below, so a refusal the app swallowed is still visible. */ +declare global { + var __geoLastPlayOutcome: string | null | undefined; +} + +/** + * Records how the last `play()` resolved. + * + * A rejected `play()` is the single most likely explanation for the reported + * two-tap sequence, and it is invisible from the outside — the promise is + * usually handed to a `.catch` that sets state the card may not render. This + * patches the prototype once and writes the outcome where the readout can see + * it, without changing what any caller receives. + */ +function recordPlayOutcomes() { + const proto = HTMLMediaElement.prototype as HTMLMediaElement & { __geoPatched?: boolean }; + if (proto.__geoPatched) return; + proto.__geoPatched = true; + + const original = proto.play; + proto.play = function patched(this: HTMLMediaElement) { + const result = original.call(this); + + if (result && typeof result.then === 'function') { + result.then( + () => { + globalThis.__geoLastPlayOutcome = 'resolved'; + }, + (error: unknown) => { + const name = error instanceof Error ? `${error.name}: ${error.message}` : String(error); + globalThis.__geoLastPlayOutcome = `REJECTED ${name}`; + } + ); + } + + return result; + }; +} + +export function ActivityDiagnostics({ galleryRef }: { galleryRef: React.RefObject }) { + const [snapshot, setSnapshot] = React.useState(null); + + React.useEffect(() => { + recordPlayOutcomes(); + + const read = () => { + const scroller = galleryRef.current?.querySelector('[data-activity-card]')?.parentElement ?? null; + + setSnapshot({ + pageY: Math.round(window.scrollY), + galleryX: scroller ? Math.round(scroller.scrollLeft) : null, + galleryH: galleryRef.current ? Math.round(galleryRef.current.getBoundingClientRect().height) : null, + docH: document.documentElement.scrollHeight, + viewportH: window.innerHeight, + videos: [...document.querySelectorAll('video')].slice(0, 2).map(video => ({ + paused: video.paused, + muted: video.muted, + inline: video.playsInline, + ready: video.readyState, + t: +video.currentTime.toFixed(1), + err: video.error?.code ?? null, + })), + lastPlay: globalThis.__geoLastPlayOutcome ?? null, + }); + }; + + read(); + const interval = window.setInterval(read, 500); + + return () => window.clearInterval(interval); + }, [galleryRef]); + + if (!snapshot) return null; + + return ( +
+
+ pageY {snapshot.pageY} · doc {snapshot.docH} · vh {snapshot.viewportH} · maxY{' '} + {snapshot.docH - snapshot.viewportH} +
+
+ galleryX {snapshot.galleryX ?? '—'} · galleryH {snapshot.galleryH ?? '—'} +
+ {snapshot.videos.map((video, index) => ( +
+ v{index} {video.paused ? 'PAUSED' : 'playing'} · t {video.t} · ready {video.ready} ·{' '} + {video.muted ? 'muted' : 'audible'} · {video.inline ? 'inline' : 'NOT-INLINE'} + {video.err === null ? '' : ` · err ${video.err}`} +
+ ))} +
play(): {snapshot.lastPlay ?? 'not called yet'}
+
+ ); +} diff --git a/apps/web/partials/profile/profile-activity-section.tsx b/apps/web/partials/profile/profile-activity-section.tsx index 137a2b7833..932e4d23a8 100644 --- a/apps/web/partials/profile/profile-activity-section.tsx +++ b/apps/web/partials/profile/profile-activity-section.tsx @@ -9,6 +9,7 @@ import { DebatePlaybackGate } from '~/core/debates/debate-playback-gate'; import { type ExploreFeedRow, toExploreFeedItem } from '~/core/explore/explore-card-item'; import { type SpaceLabel, spaceLabel, useSpaceLabels } from '~/core/hooks/use-space-labels'; import type { ClaimResponse } from '~/core/profile/use-person-positions'; +import { useActivityDiagnosticsEnabled } from '~/core/state/feature-flags'; import { normId } from '~/core/utils/norm-id'; import { RightArrowLongSmall } from '~/design-system/icons/right-arrow-long-small'; @@ -16,6 +17,7 @@ import { PrefetchLink as Link } from '~/design-system/prefetch-link'; import { ExploreFeedCard } from '~/partials/explore/explore-feed-card'; +import { ActivityDiagnostics } from './activity-diagnostics'; import { GalleryClaimCard } from './gallery-claim-card'; /** How many cards a gallery holds before the reader is sent to the tab. */ @@ -89,6 +91,7 @@ export function ProfileActivitySection({ kinds }: { kinds: ActivityKind[] }) { const selected = available.find(kind => kind.key === selectedKey) ?? available[0]; const { galleryRef, contentRef, heldHeight, holdHeight } = useHeldHeight(selected?.key); + const showDiagnostics = useActivityDiagnosticsEnabled(); // Nothing at all rather than an empty card. A heading over a blank space reads // as a page that failed to load, and most accounts have never been in a debate. @@ -169,6 +172,8 @@ export function ProfileActivitySection({ kinds }: { kinds: ActivityKind[] }) {
+ {showDiagnostics && } + Date: Sat, 19 Sep 2026 13:08:43 -0700 Subject: [PATCH 03/19] refactor: move the autoplay work out of this PR (GEO-2974) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Autoplay turns out not to be a gallery fault at all — it fails on the Explore feed too, where there is no playback gate — so the threshold change here was aimed at the wrong thing, and the diagnostic belongs with the investigation rather than with the layout fixes. What is left is what was measured and fixed here: the height the gallery holds so a kind switch cannot move the reader, the bottom row of a debate tile that overlapped its own vote button at narrow widths, and the stretch that gave short cards a border reaching past their content. Co-Authored-By: Claude Opus 5 (1M context) --- .../web/core/debates/debate-playback-gate.tsx | 18 --- apps/web/core/state/feature-flags.test.tsx | 3 - apps/web/core/state/feature-flags.ts | 11 -- .../explore/debate-explore-feed-card.tsx | 26 +-- .../feature-flags-dialog.test.tsx | 1 - .../partials/profile/activity-diagnostics.tsx | 148 ------------------ .../profile/profile-activity-section.tsx | 5 - 7 files changed, 5 insertions(+), 207 deletions(-) delete mode 100644 apps/web/partials/profile/activity-diagnostics.tsx diff --git a/apps/web/core/debates/debate-playback-gate.tsx b/apps/web/core/debates/debate-playback-gate.tsx index 88fb8a3e68..a3069d758a 100644 --- a/apps/web/core/debates/debate-playback-gate.tsx +++ b/apps/web/core/debates/debate-playback-gate.tsx @@ -37,21 +37,3 @@ export function useDebatePlaybackAllowed(debateId: string): boolean { if (allowed === undefined) return true; return allowed !== null && ID.equals(allowed, debateId); } - -/** - * Whether a gate is deciding at all, which changes what a card's own judgement - * is *for*. - * - * Ungated, a card's intersection ratio answers "should I be the one playing" — - * it is the only thing standing between a feed of cards and all of them playing - * at once, so it is strict on purpose: 0.6 to start. - * - * Gated, that question is already answered, and a second stricter test can only - * subtract. A card the gate has chosen but whose ratio sits in the dead band - * never starts, and tapping is the reader's only way out — which is what a - * 520px card in a 664px viewport spends much of its time doing. So a gated card - * asks the easier question instead: am I on screen at all. - */ -export function useIsDebatePlaybackGated(): boolean { - return React.useContext(DebatePlaybackContext) !== undefined; -} diff --git a/apps/web/core/state/feature-flags.test.tsx b/apps/web/core/state/feature-flags.test.tsx index 9a891b3510..88a1f68ae7 100644 --- a/apps/web/core/state/feature-flags.test.tsx +++ b/apps/web/core/state/feature-flags.test.tsx @@ -28,7 +28,6 @@ describe('feature flags', () => { expect(defaultFeatureFlags.exploreSidePanel).toBe(false); expect(defaultFeatureFlags.bountiesTab).toBe(true); expect(normalizeFeatureFlags(null)).toEqual({ - activityDiagnostics: false, debugDebatesPage: false, debateDebugging: false, debateFormatSelector: false, @@ -42,7 +41,6 @@ describe('feature flags', () => { // reaching the dialog would render a checkbox for a flag nothing reads. it('drops the retired claims-and-debates flags that are still in storage', () => { expect(normalizeFeatureFlags({ questionsTab: true, debatesTab: true, debateDebugging: true })).toEqual({ - activityDiagnostics: false, debugDebatesPage: false, debateDebugging: true, debateFormatSelector: false, @@ -67,7 +65,6 @@ describe('feature flags', () => { // serialize in is incidental — it follows the definition list, and pinning it here would fail // on a reordering that changes nothing a reader could notice. expect(JSON.parse(window.localStorage.getItem(featureFlagsStorageKey) ?? 'null')).toEqual({ - activityDiagnostics: false, debugDebatesPage: true, debateDebugging: true, debateFormatSelector: true, diff --git a/apps/web/core/state/feature-flags.ts b/apps/web/core/state/feature-flags.ts index 329240b5e4..0864d6f925 100644 --- a/apps/web/core/state/feature-flags.ts +++ b/apps/web/core/state/feature-flags.ts @@ -20,13 +20,6 @@ export const featureFlagDefinitions = [ description: 'Allow the first matched debater to choose a format before accepting.', enabledByDefault: false, }, - { - id: 'activityDiagnostics', - label: 'Activity gallery diagnostics', - description: - "Show what the profile's Activity gallery is doing — scroll offsets, its height, and each video's real playback state. For faults that only happen on a phone (GEO-2974).", - enabledByDefault: false, - }, { id: 'debugDebatesPage', label: 'Debates debug tab per space', @@ -118,10 +111,6 @@ export function useDebugDebatesPageEnabled() { return useFeatureFlag('debugDebatesPage'); } -export function useActivityDiagnosticsEnabled() { - return useFeatureFlag('activityDiagnostics'); -} - /** * Read *and* write, for the flags dialog. Deliberately not hydration-gated like * {@link useFeatureFlag}: the dialog's contents are inside a Radix `Root` that is closed until a diff --git a/apps/web/partials/explore/debate-explore-feed-card.tsx b/apps/web/partials/explore/debate-explore-feed-card.tsx index e0f6e2815d..8684352fe0 100644 --- a/apps/web/partials/explore/debate-explore-feed-card.tsx +++ b/apps/web/partials/explore/debate-explore-feed-card.tsx @@ -7,7 +7,7 @@ import { DebateClaimsPanel } from '~/core/debates/browse/debate-claims-panel'; import { DebateFeedPlayer } from '~/core/debates/browse/debate-feed-player'; import { DebateShareDialog } from '~/core/debates/browse/share-dialog'; import { useDebateShareAction } from '~/core/debates/browse/use-debate-share-action'; -import { useDebatePlaybackAllowed, useIsDebatePlaybackGated } from '~/core/debates/debate-playback-gate'; +import { useDebatePlaybackAllowed } from '~/core/debates/debate-playback-gate'; import { useDebate, useDebateMedia } from '~/core/debates/hooks'; import { hasProcessedVideo, isWatchableDebate } from '~/core/debates/playback-utils'; import { useDebateTranscriptClaims } from '~/core/debates/use-debate-transcript-claims'; @@ -34,16 +34,6 @@ import { SpaceThumb } from './space-thumb'; const ACTIVATE_RATIO = 0.6; const DEACTIVATE_RATIO = 0.4; -/** - * The same hysteresis, asked of a card that is not competing for the turn. - * - * Where a gate names the one card allowed to play, this card's ratio only has to - * say whether it is worth playing *to* — so it is a visibility test rather than - * a contest, and the thresholds are what "on screen at all" means. - */ -const GATED_ACTIVATE_RATIO = 0.25; -const GATED_DEACTIVATE_RATIO = 0.1; - type DebateExploreFeedCardProps = { item: ExploreFeedItem; /** Hide the space thumbnail + space-name link in the meta row (same semantics as ExploreFeedCard). */ @@ -96,12 +86,6 @@ export function DebateExploreFeedCard({ // already was strictly between them. The lower edge is inclusive so that the observer's // report at the 0.4 threshold deactivates rather than landing ambiguously inside the band — // a ratio reported exactly at a threshold is the normal case, not an edge case. - // Which pair applies depends on whether anything else is deciding — see - // `useIsDebatePlaybackGated`. - const isGated = useIsDebatePlaybackGated(); - const activateAt = isGated ? GATED_ACTIVATE_RATIO : ACTIVATE_RATIO; - const deactivateAt = isGated ? GATED_DEACTIVATE_RATIO : DEACTIVATE_RATIO; - const [active, setActive] = React.useState(false); React.useEffect(() => { if (!container) return; @@ -110,17 +94,17 @@ export function DebateExploreFeedCard({ for (const entry of entries) { setActive(current => { if (!entry.isIntersecting) return false; - if (entry.intersectionRatio >= activateAt) return true; - if (entry.intersectionRatio <= deactivateAt) return false; + if (entry.intersectionRatio >= ACTIVATE_RATIO) return true; + if (entry.intersectionRatio <= DEACTIVATE_RATIO) return false; return current; }); } }, - { threshold: [deactivateAt, activateAt] } + { threshold: [DEACTIVATE_RATIO, ACTIVATE_RATIO] } ); observer.observe(container); return () => observer.disconnect(); - }, [activateAt, container, deactivateAt]); + }, [container]); // A veto, not a replacement: where a surface holds playback to one debate — // a row of cards, all of them fully on screen at once — this says whether it diff --git a/apps/web/partials/feature-flags/feature-flags-dialog.test.tsx b/apps/web/partials/feature-flags/feature-flags-dialog.test.tsx index f3480b47e6..0e2ceba8ee 100644 --- a/apps/web/partials/feature-flags/feature-flags-dialog.test.tsx +++ b/apps/web/partials/feature-flags/feature-flags-dialog.test.tsx @@ -57,7 +57,6 @@ describe('FeatureFlagsDialog', () => { await waitFor(() => { // Values, not key order — see the note in `feature-flags.test.ts`. expect(JSON.parse(window.localStorage.getItem(featureFlagsStorageKey) ?? 'null')).toEqual({ - activityDiagnostics: false, debugDebatesPage: true, debateDebugging: true, debateFormatSelector: true, diff --git a/apps/web/partials/profile/activity-diagnostics.tsx b/apps/web/partials/profile/activity-diagnostics.tsx deleted file mode 100644 index 3df8299549..0000000000 --- a/apps/web/partials/profile/activity-diagnostics.tsx +++ /dev/null @@ -1,148 +0,0 @@ -'use client'; - -import * as React from 'react'; - -/** - * What the Activity gallery is actually doing, on the device it is doing it on - * (GEO-2974). - * - * Three faults were reported from a phone — autoplay that needs two taps, the - * page moving when the kinds are swapped, and the gallery losing its place when - * a claim opens. None of the three reproduces headlessly: Chromium played every - * card on every swipe, WebKit reported `paused: false` with the clock advancing - * and `play()` resolving, and both kept their scroll positions through a switch - * and through opening the panel. - * - * That is not evidence the faults are imagined. It is evidence that the things - * causing them are the things headless engines do not have — iOS media policy, - * Low Power Mode, a toolbar that resizes the viewport as you scroll, real touch - * momentum. So rather than keep guessing at fixes for symptoms nobody can - * reproduce, this reports the state that would tell them apart, on the phone - * where they happen. - * - * **What to look for, for the autoplay report specifically.** The reported - * sequence — tap once and a play button appears, tap again and it plays — says - * the app believed it was already playing while the element was not: the first - * tap *paused* something. So the row to read is `paused` against `app`. If - * `paused: true` shows while the card claims to be playing, the initial `play()` - * was refused and the refusal never reached the UI, and the fix is to make the - * player's visible state follow the element rather than the intention. If - * `paused: false` with `t` frozen, the element is running and not decoding, - * which is a different fix entirely. - * - * Behind a flag rather than a query param so it survives the navigation into a - * side panel and back, which is one of the things being measured. - */ -type VideoState = { - paused: boolean; - muted: boolean; - inline: boolean; - ready: number; - t: number; - err: number | null; -}; - -type Snapshot = { - pageY: number; - galleryX: number | null; - galleryH: number | null; - docH: number; - viewportH: number; - videos: VideoState[]; - lastPlay: string | null; -}; - -/** Set by the patch below, so a refusal the app swallowed is still visible. */ -declare global { - var __geoLastPlayOutcome: string | null | undefined; -} - -/** - * Records how the last `play()` resolved. - * - * A rejected `play()` is the single most likely explanation for the reported - * two-tap sequence, and it is invisible from the outside — the promise is - * usually handed to a `.catch` that sets state the card may not render. This - * patches the prototype once and writes the outcome where the readout can see - * it, without changing what any caller receives. - */ -function recordPlayOutcomes() { - const proto = HTMLMediaElement.prototype as HTMLMediaElement & { __geoPatched?: boolean }; - if (proto.__geoPatched) return; - proto.__geoPatched = true; - - const original = proto.play; - proto.play = function patched(this: HTMLMediaElement) { - const result = original.call(this); - - if (result && typeof result.then === 'function') { - result.then( - () => { - globalThis.__geoLastPlayOutcome = 'resolved'; - }, - (error: unknown) => { - const name = error instanceof Error ? `${error.name}: ${error.message}` : String(error); - globalThis.__geoLastPlayOutcome = `REJECTED ${name}`; - } - ); - } - - return result; - }; -} - -export function ActivityDiagnostics({ galleryRef }: { galleryRef: React.RefObject }) { - const [snapshot, setSnapshot] = React.useState(null); - - React.useEffect(() => { - recordPlayOutcomes(); - - const read = () => { - const scroller = galleryRef.current?.querySelector('[data-activity-card]')?.parentElement ?? null; - - setSnapshot({ - pageY: Math.round(window.scrollY), - galleryX: scroller ? Math.round(scroller.scrollLeft) : null, - galleryH: galleryRef.current ? Math.round(galleryRef.current.getBoundingClientRect().height) : null, - docH: document.documentElement.scrollHeight, - viewportH: window.innerHeight, - videos: [...document.querySelectorAll('video')].slice(0, 2).map(video => ({ - paused: video.paused, - muted: video.muted, - inline: video.playsInline, - ready: video.readyState, - t: +video.currentTime.toFixed(1), - err: video.error?.code ?? null, - })), - lastPlay: globalThis.__geoLastPlayOutcome ?? null, - }); - }; - - read(); - const interval = window.setInterval(read, 500); - - return () => window.clearInterval(interval); - }, [galleryRef]); - - if (!snapshot) return null; - - return ( -
-
- pageY {snapshot.pageY} · doc {snapshot.docH} · vh {snapshot.viewportH} · maxY{' '} - {snapshot.docH - snapshot.viewportH} -
-
- galleryX {snapshot.galleryX ?? '—'} · galleryH {snapshot.galleryH ?? '—'} -
- {snapshot.videos.map((video, index) => ( -
- v{index} {video.paused ? 'PAUSED' : 'playing'} · t {video.t} · ready {video.ready} ·{' '} - {video.muted ? 'muted' : 'audible'} · {video.inline ? 'inline' : 'NOT-INLINE'} - {video.err === null ? '' : ` · err ${video.err}`} -
- ))} -
play(): {snapshot.lastPlay ?? 'not called yet'}
-
- ); -} diff --git a/apps/web/partials/profile/profile-activity-section.tsx b/apps/web/partials/profile/profile-activity-section.tsx index 932e4d23a8..137a2b7833 100644 --- a/apps/web/partials/profile/profile-activity-section.tsx +++ b/apps/web/partials/profile/profile-activity-section.tsx @@ -9,7 +9,6 @@ import { DebatePlaybackGate } from '~/core/debates/debate-playback-gate'; import { type ExploreFeedRow, toExploreFeedItem } from '~/core/explore/explore-card-item'; import { type SpaceLabel, spaceLabel, useSpaceLabels } from '~/core/hooks/use-space-labels'; import type { ClaimResponse } from '~/core/profile/use-person-positions'; -import { useActivityDiagnosticsEnabled } from '~/core/state/feature-flags'; import { normId } from '~/core/utils/norm-id'; import { RightArrowLongSmall } from '~/design-system/icons/right-arrow-long-small'; @@ -17,7 +16,6 @@ import { PrefetchLink as Link } from '~/design-system/prefetch-link'; import { ExploreFeedCard } from '~/partials/explore/explore-feed-card'; -import { ActivityDiagnostics } from './activity-diagnostics'; import { GalleryClaimCard } from './gallery-claim-card'; /** How many cards a gallery holds before the reader is sent to the tab. */ @@ -91,7 +89,6 @@ export function ProfileActivitySection({ kinds }: { kinds: ActivityKind[] }) { const selected = available.find(kind => kind.key === selectedKey) ?? available[0]; const { galleryRef, contentRef, heldHeight, holdHeight } = useHeldHeight(selected?.key); - const showDiagnostics = useActivityDiagnosticsEnabled(); // Nothing at all rather than an empty card. A heading over a blank space reads // as a page that failed to load, and most accounts have never been in a debate. @@ -172,8 +169,6 @@ export function ProfileActivitySection({ kinds }: { kinds: ActivityKind[] }) { - {showDiagnostics && } - Date: Sat, 19 Sep 2026 13:58:50 -0700 Subject: [PATCH 04/19] Revert the Activity gallery's height floor (GEO-2974) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It traded one visible fault for a worse one. Holding the region at its pre-switch height stops the document shrinking under the reader, but the release condition — only once dropping the floor would not move anybody — is false for exactly the reader it was written for, the one standing near the bottom of the page looking at Activity. So the floor stays on, and a claims gallery sits in a debate-sized hole: 196px of cards in a 678px box on desktop. Reported with a screenshot, and it is the right call. A transient jump is worse than nothing; permanent dead space on every switch is worse than the jump. The two changes that did work stay: the tile's bottom row no longer overlaps its own vote button at narrow widths, and `items-start` keeps a short card's border against its own content. The jump itself is unfixed and I am not going to guess at it a third time. The numbers say why it is hard: on a phone a debate card is 474px and a claim card 254px, so switching takes ~270px out of a page whose reader is often inside that margin of the bottom. No scroll position survives that, which leaves either reserving the space — this, rejected — or making the two kinds close enough in height that there is nothing to reserve. The second is the real fix and belongs with the mobile layout work rather than bolted onto it. Co-Authored-By: Claude Opus 5 (1M context) --- .../profile/activity-held-height.test.ts | 63 ------- .../profile/profile-activity-section.tsx | 157 ++---------------- 2 files changed, 15 insertions(+), 205 deletions(-) delete mode 100644 apps/web/partials/profile/activity-held-height.test.ts diff --git a/apps/web/partials/profile/activity-held-height.test.ts b/apps/web/partials/profile/activity-held-height.test.ts deleted file mode 100644 index 3da3b6371f..0000000000 --- a/apps/web/partials/profile/activity-held-height.test.ts +++ /dev/null @@ -1,63 +0,0 @@ -import { describe, expect, it } from 'vitest'; - -import { canReleaseHeldHeight } from './profile-activity-section'; - -/** - * When the Activity gallery may stop holding its height (GEO-2974). - * - * Switching Debates to Claims swaps tall cards for short ones, which is - * legitimate — a claim card really is shorter. What is not is doing it while - * somebody is standing below where the page would then end. Measured on an - * iPhone 13 against the live site: the document goes 1198 to 874 with the reader - * 389 down it, and 874 can only scroll to 210, so the browser puts them there. - * - * No scroll position survives that, so the gallery holds its height instead and - * this decides when to let go. The numbers below are that measurement. - */ -const AT_THE_BOTTOM = { - heldHeight: 520, - contentHeight: 254, - documentHeight: 1198, - viewportHeight: 664, - scrollY: 389, -}; - -describe('canReleaseHeldHeight', () => { - /** - * The reader is 389 down a page that would end at 268 without the floor - * (1198 - 266 padding - 664 viewport). Letting go moves them 121px. - */ - it('holds while dropping the floor would move the reader', () => { - expect(canReleaseHeldHeight(AT_THE_BOTTOM)).toBe(false); - }); - - it('lets go once they have scrolled up out of the way', () => { - expect(canReleaseHeldHeight({ ...AT_THE_BOTTOM, scrollY: 120 })).toBe(true); - }); - - /** - * The ordinary way out: the incoming content grew past the floor, so the floor - * is adding nothing and there is nothing to lose by dropping it. - */ - it('lets go once the content is taller than the floor', () => { - expect(canReleaseHeldHeight({ ...AT_THE_BOTTOM, contentHeight: 600 })).toBe(true); - }); - - it('lets go when the content exactly matches the floor', () => { - expect(canReleaseHeldHeight({ ...AT_THE_BOTTOM, contentHeight: 520 })).toBe(true); - }); - - /** The boundary itself is safe: the reader lands exactly at the new bottom. */ - it('lets go at the exact offset where nothing moves', () => { - expect(canReleaseHeldHeight({ ...AT_THE_BOTTOM, scrollY: 268 })).toBe(true); - expect(canReleaseHeldHeight({ ...AT_THE_BOTTOM, scrollY: 269 })).toBe(false); - }); - - /** - * A tall enough page absorbs the loss. The reader is in the middle of a long - * document, so 266px off the bottom is not their problem. - */ - it('lets go on a page with room to spare below', () => { - expect(canReleaseHeldHeight({ ...AT_THE_BOTTOM, documentHeight: 4000 })).toBe(true); - }); -}); diff --git a/apps/web/partials/profile/profile-activity-section.tsx b/apps/web/partials/profile/profile-activity-section.tsx index 137a2b7833..8f04195abe 100644 --- a/apps/web/partials/profile/profile-activity-section.tsx +++ b/apps/web/partials/profile/profile-activity-section.tsx @@ -88,8 +88,6 @@ export function ProfileActivitySection({ kinds }: { kinds: ActivityKind[] }) { // cannot shift the selection out from under them. const selected = available.find(kind => kind.key === selectedKey) ?? available[0]; - const { galleryRef, contentRef, heldHeight, holdHeight } = useHeldHeight(selected?.key); - // Nothing at all rather than an empty card. A heading over a blank space reads // as a page that failed to load, and most accounts have never been in a debate. if (kinds.some(kind => kind.isLoading) || available.length === 0 || !selected) return null; @@ -113,11 +111,7 @@ export function ProfileActivitySection({ kinds }: { kinds: ActivityKind[] }) { key={kind.key} type="button" aria-pressed={isSelected} - onClick={() => { - // Before the swap, so there is a height to hold. - holdHeight(); - setSelectedKey(kind.key); - }} + onClick={() => setSelectedKey(kind.key)} className={cx( 'inline-flex items-center gap-1.5 rounded-full border px-3 py-1 text-smallButton transition-colors', isSelected @@ -136,39 +130,20 @@ export function ProfileActivitySection({ kinds }: { kinds: ActivityKind[] }) { )} - {/* - * The gallery keeps the height it had while the kinds are swapped. - * - * Measured on a phone: Debates puts the document at 1198px with the - * reader 389px down it; the moment Claims is picked it is 874px, which is - * shorter than where they were standing, so the browser clamps the scroll - * and throws them 179px up the page. A claim card really is shorter than a - * debate card, so the collapse is legitimate — what is not is doing it - * underneath somebody. - * - * So the swap happens at the old height and the height is released - * afterwards, by which point the reader is looking at the new cards rather - * than being moved past them. - */} -
-
- {selected.isError && selected.rows.length === 0 ? ( - /* - * No retry here on purpose. This card is a summary; the tab its count - * links to holds the authoritative list and offers the retry, so a - * second control here would be a second thing to keep in step. - */ -

Couldn’t load {selected.label.toLowerCase()}.

- ) : ( - - )} -
-
- + {selected.isError && selected.rows.length === 0 ? ( + /* + * No retry here on purpose. This card is a summary; the tab its count + * links to holds the authoritative list and offers the retry, so a + * second control here would be a second thing to keep in step. + */ +

Couldn’t load {selected.label.toLowerCase()}.

+ ) : ( + + )} (null); - /** The content inside it, which keeps its natural height so it can be measured. */ - const contentRef = React.useRef(null); - const [heldHeight, setHeldHeight] = React.useState(null); - - const holdHeight = React.useCallback(() => { - const height = contentRef.current?.getBoundingClientRect().height; - if (height) setHeldHeight(height); - }, []); - - React.useEffect(() => { - if (heldHeight === null) return; - - const check = () => { - const content = contentRef.current; - - const release = - !content || - canReleaseHeldHeight({ - heldHeight, - contentHeight: content.getBoundingClientRect().height, - documentHeight: document.documentElement.scrollHeight, - viewportHeight: window.innerHeight, - scrollY: window.scrollY, - }); - - if (release) setHeldHeight(null); - }; - - // Polled *and* on scroll: the content settles on its own schedule, and the - // reader scrolling up is the other way the answer turns yes. - const interval = window.setInterval(check, 250); - window.addEventListener('scroll', check, { passive: true }); - check(); - - return () => { - window.clearInterval(interval); - window.removeEventListener('scroll', check); - }; - // Re-armed by the key, so a second switch before the first released still - // measures and releases rather than being swallowed by the held value. - }, [heldHeight, selectedKey]); - - return { galleryRef, contentRef, heldHeight, holdHeight }; -} - /** * Which card is nearest the middle of the row. * From f2b735f5c4e011c801bacfacfa5a20b28f880aa8 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Sat, 19 Sep 2026 14:20:12 -0700 Subject: [PATCH 05/19] fix(profile): preserve mobile activity scroll position --- .../profile/profile-activity-section.test.tsx | 82 ++++++- .../profile/profile-activity-section.tsx | 226 +++++++++++++----- 2 files changed, 251 insertions(+), 57 deletions(-) diff --git a/apps/web/partials/profile/profile-activity-section.test.tsx b/apps/web/partials/profile/profile-activity-section.test.tsx index b5d2342a98..b907ea27c1 100644 --- a/apps/web/partials/profile/profile-activity-section.test.tsx +++ b/apps/web/partials/profile/profile-activity-section.test.tsx @@ -1,5 +1,5 @@ import '@testing-library/jest-dom/vitest'; -import { cleanup, render, screen } from '@testing-library/react'; +import { cleanup, fireEvent, render, screen } from '@testing-library/react'; import * as React from 'react'; @@ -47,6 +47,48 @@ const kind = (over: Partial[ ...over, }); +const rect = (width: number, height: number): DOMRect => ({ + x: 0, + y: 0, + top: 0, + right: width, + bottom: height, + left: 0, + width, + height, + toJSON: () => ({}), +}); + +/** A 390×600 mobile viewport sitting 400px down a synthetic profile page. */ +function mockMobileActivityGeometry(pageHeightWithoutActivity: number) { + const originalRect = HTMLElement.prototype.getBoundingClientRect; + vi.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockImplementation(function (this: HTMLElement) { + if ('activitySection' in this.dataset) { + const debatesSelected = + this.querySelector('button[aria-pressed="true"]')?.textContent?.includes('Debates'); + return rect(390, debatesSelected ? 500 : 250); + } + + if ('activityScrollReserve' in this.dataset) { + return rect(390, Number.parseFloat(this.style.height) || 0); + } + + return originalRect.call(this); + }); + + vi.spyOn(window, 'scrollY', 'get').mockReturnValue(400); + vi.spyOn(window, 'innerHeight', 'get').mockReturnValue(600); + vi.spyOn(document.documentElement, 'scrollHeight', 'get').mockImplementation(() => { + const section = document.querySelector('[data-activity-section]'); + const reserve = document.querySelector('[data-activity-scroll-reserve]'); + return ( + pageHeightWithoutActivity + + (section?.getBoundingClientRect().height ?? 0) + + (reserve?.getBoundingClientRect().height ?? 0) + ); + }); +} + /** * What the Activity card says when half of it did not arrive (GEO-2859). * @@ -56,7 +98,10 @@ const kind = (over: Partial[ * no positions, while the rail beside it counted 208. */ describe('ProfileActivitySection', () => { - afterEach(cleanup); + afterEach(() => { + cleanup(); + vi.restoreAllMocks(); + }); it('renders nothing when both kinds are genuinely empty', () => { // Most accounts have never been in a debate; a heading over blank space @@ -102,4 +147,37 @@ describe('ProfileActivitySection', () => { expect(screen.getByRole('button', { name: /Debates/ })).toHaveTextContent('—'); }); + + it('reserves the lost mobile document height while switching between kinds', () => { + mockMobileActivityGeometry(600); + + const { container } = render( + + ); + const reserve = container.querySelector('[data-activity-scroll-reserve]'); + + expect(reserve).toHaveStyle({ height: '0px' }); + + fireEvent.click(screen.getByRole('button', { name: /Claims/ })); + expect(reserve).toHaveStyle({ height: '150px' }); + + fireEvent.click(screen.getByRole('button', { name: /Debates/ })); + expect(reserve).toHaveStyle({ height: '0px' }); + + fireEvent.click(screen.getByRole('button', { name: /Claims/ })); + expect(reserve).toHaveStyle({ height: '150px' }); + }); + + it('adds no reserve when content below Activity already preserves the scroll range', () => { + mockMobileActivityGeometry(900); + + const { container } = render( + + ); + const reserve = container.querySelector('[data-activity-scroll-reserve]'); + + fireEvent.click(screen.getByRole('button', { name: /Claims/ })); + + expect(reserve).toHaveStyle({ height: '0px' }); + }); }); diff --git a/apps/web/partials/profile/profile-activity-section.tsx b/apps/web/partials/profile/profile-activity-section.tsx index 8f04195abe..f9773376bb 100644 --- a/apps/web/partials/profile/profile-activity-section.tsx +++ b/apps/web/partials/profile/profile-activity-section.tsx @@ -88,73 +88,189 @@ export function ProfileActivitySection({ kinds }: { kinds: ActivityKind[] }) { // cannot shift the selection out from under them. const selected = available.find(kind => kind.key === selectedKey) ?? available[0]; + const { sectionRef, reserveRef, prepareSwitch } = useMobileActivityHeightReserve(selected?.key); + // Nothing at all rather than an empty card. A heading over a blank space reads // as a page that failed to load, and most accounts have never been in a debate. if (kinds.some(kind => kind.isLoading) || available.length === 0 || !selected) return null; return ( -
- {/* The toggles sit to the right of the heading, and wrap below it rather +
+
+ {/* The toggles sit to the right of the heading, and wrap below it rather than squeezing into it on a narrow screen. */} -
-

Activity

+
+

Activity

- {/* Only when there is a choice to make. One pill on its own is a label + {/* Only when there is a choice to make. One pill on its own is a label dressed up as a control. */} - {available.length > 1 && ( -
- {available.map(kind => { - const isSelected = kind.key === selected.key; - - return ( - - ); - })} -
+ {available.length > 1 && ( +
+ {available.map(kind => { + const isSelected = kind.key === selected.key; + + return ( + + ); + })} +
+ )} +
+ + {selected.isError && selected.rows.length === 0 ? ( + /* + * No retry here on purpose. This card is a summary; the tab its count + * links to holds the authoritative list and offers the retry, so a + * second control here would be a second thing to keep in step. + */ +

Couldn’t load {selected.label.toLowerCase()}.

+ ) : ( + )} -
- - {selected.isError && selected.rows.length === 0 ? ( - /* - * No retry here on purpose. This card is a summary; the tab its count - * links to holds the authoritative list and offers the retry, so a - * second control here would be a second thing to keep in step. - */ -

Couldn’t load {selected.label.toLowerCase()}.

- ) : ( - - )} - - {selected.seeAllLabel} - - -
+ + {selected.seeAllLabel} + + +
+ + {/* + * On narrow screens this supplies only the document height missing below + * the current viewport. The reserve sits outside the card, so Claims + * stays compact while switching away from the taller Debates view cannot + * clamp the viewport upward. Profiles with content below Activity need no + * reserve at all, and desktop keeps its natural layout. + */} +
+
); } +/** + * Preserve the mobile scroll range while Activity views of different heights + * are swapped. There is no scroll position to restore when the new document is + * shorter than the viewport's old bottom; keeping only that missing height in + * the document is what prevents the browser from clamping `scrollY`. + * + * The reserve is a sibling of the card rather than a `min-height` on it. That + * leaves the selected gallery and its footer at their natural height instead + * of putting a short Claims row inside a debate-sized white card. + */ +function useMobileActivityHeightReserve(selectedKey: string | undefined) { + const sectionRef = React.useRef(null); + const reserveRef = React.useRef(null); + const swapRef = React.useRef<{ + width: number; + sectionHeight: number; + naturalDocumentHeight: number; + viewportBottom: number; + } | null>(null); + + const prepareSwitch = React.useCallback(() => { + const section = sectionRef.current; + const reserve = reserveRef.current; + if (!section || !reserve) return; + + const { width, height } = section.getBoundingClientRect(); + const currentReserve = reserve.getBoundingClientRect().height; + swapRef.current = { + width, + sectionHeight: height, + naturalDocumentHeight: document.documentElement.scrollHeight - currentReserve, + viewportBottom: window.scrollY + window.innerHeight, + }; + + // Deliberately over-reserve before the swap. The layout effect replaces + // this with the exact missing scroll range before the browser paints the + // new view. + reserve.style.height = `${height}px`; + }, []); + + React.useLayoutEffect(() => { + const section = sectionRef.current; + const reserve = reserveRef.current; + if (!section || !reserve) return; + + const sync = () => { + const { width, height } = section.getBoundingClientRect(); + const swap = swapRef.current; + + // A new layout width (rotation, resized side panel, breakpoint change) + // has different card wrapping. Drop the old calculation; the next tab + // switch will establish one for the new layout. + if (!swap || Math.abs(swap.width - width) > 1) { + swapRef.current = null; + reserve.style.height = '0px'; + return; + } + + const naturalDocumentHeight = swap.naturalDocumentHeight - swap.sectionHeight + height; + const missingScrollRange = Math.max(0, swap.viewportBottom - naturalDocumentHeight); + + reserve.style.height = `${missingScrollRange}px`; + }; + + sync(); + + // Claim cards grow as their queries land. Shrink the reserve by the same + // amount so the overall document height stays steady rather than drifting. + if (typeof ResizeObserver === 'undefined') return; + const observer = new ResizeObserver(sync); + observer.observe(section); + const onScroll = () => { + const swap = swapRef.current; + if (!swap) return; + + // Once the reader moves up, do not retain space they no longer need. + swap.viewportBottom = Math.min(swap.viewportBottom, window.scrollY + window.innerHeight); + sync(); + }; + window.addEventListener('scroll', onScroll, { passive: true }); + + return () => { + observer.disconnect(); + window.removeEventListener('scroll', onScroll); + }; + }, [selectedKey]); + + return { sectionRef, reserveRef, prepareSwitch }; +} + function ActivityGallery({ rows, responseByClaimId, From 94c4ac8218a7b53fedc56ea9812a56486d666d72 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Sat, 19 Sep 2026 15:55:40 -0700 Subject: [PATCH 06/19] fix(profile): show the mobile activity reserve on mobile (GEO-2974) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The height reserve added for this bug has never run. Its class was `md:hidden`, and the breakpoints here are desktop-first — `md` is `@media (max-width: 767px)`, see styles.css — so it was switched off on exactly the phones it exists for. `display: none` reserves no height, so the mechanism was inert on the only viewports that needed it, which is why the page still jumped on the deployed build. Measured on Susan Winter's profile at 390×664, before this change: before scrollY 414 scrollHeight 1198 reserve 0 after scrollY 268 scrollHeight 932 reserve 0 Δ -146 The reserve's computed style there was `display: none` while its inline height was being set to 632px and then 0px — the arithmetic was running and being discarded. With the breakpoint corrected: before scrollY 414 scrollHeight 1078 reserve 204 after scrollY 414 scrollHeight 1078 reserve 146 Δ 0 The reserve shrinks as the claim cards grow, holding the document at the height the reader's position needs rather than drifting. Two further changes, both from what the traces showed: The position is now restored synchronously if a shrink beats the reserve to it. The reserve goes into the document a frame before React swaps the view, but the gallery can paint empty while its queries land, and a document shorter than the reserve can cover takes the reader with it. Putting them back inside the layout effect means they never see it. The release-on-scroll path no longer collapses the reserve when the correction above is what moved the page. It treated any upward scroll as the reader choosing to move, so the clamp's own scroll event released the very height that would have prevented it. Three of the new tests fail against the old implementation. The ones already here could not: they mock geometry, JSDOM applies no Tailwind, and so a reserve the browser was hiding measured and asserted perfectly while doing nothing on a phone. The added assertions pin the breakpoint itself, which is the only trace of that mistake that survives into JSDOM, and cover the reported shape — a profile with nothing below Activity, where the reserve is the only thing between the reader and the top of the page. --- .../profile/profile-activity-section.test.tsx | 95 ++++++++++++++++++- .../profile/profile-activity-section.tsx | 74 +++++++++++---- 2 files changed, 147 insertions(+), 22 deletions(-) diff --git a/apps/web/partials/profile/profile-activity-section.test.tsx b/apps/web/partials/profile/profile-activity-section.test.tsx index b907ea27c1..9b8206d79d 100644 --- a/apps/web/partials/profile/profile-activity-section.test.tsx +++ b/apps/web/partials/profile/profile-activity-section.test.tsx @@ -76,7 +76,13 @@ function mockMobileActivityGeometry(pageHeightWithoutActivity: number) { return originalRect.call(this); }); - vi.spyOn(window, 'scrollY', 'get').mockReturnValue(400); + const scroll: { y: number; moveAfterFirstRead?: number } = { y: 400 }; + let reads = 0; + vi.spyOn(window, 'scrollY', 'get').mockImplementation(() => { + reads += 1; + if (scroll.moveAfterFirstRead !== undefined && reads > 1) return scroll.moveAfterFirstRead; + return scroll.y; + }); vi.spyOn(window, 'innerHeight', 'get').mockReturnValue(600); vi.spyOn(document.documentElement, 'scrollHeight', 'get').mockImplementation(() => { const section = document.querySelector('[data-activity-section]'); @@ -87,6 +93,10 @@ function mockMobileActivityGeometry(pageHeightWithoutActivity: number) { (reserve?.getBoundingClientRect().height ?? 0) ); }); + + // Returned so a test can stand in for the browser moving the reader — there is no real layout + // here to clamp a scroll position, and the recovery path exists for exactly that case. + return scroll; } /** @@ -168,6 +178,89 @@ describe('ProfileActivitySection', () => { expect(reserve).toHaveStyle({ height: '150px' }); }); + /** + * The reserve has to be *displayed* on the screens it exists for, which is the one thing the tests + * above could not see: they mock geometry, JSDOM applies no Tailwind, and so a reserve that the + * browser was hiding still measured and asserted perfectly while doing nothing on a real phone. + * + * The breakpoints here are desktop-first — `md` is `@media (max-width: 767px)`, see styles.css — so + * `md:hidden` hides an element *on mobile*. This element carried exactly that, which made the whole + * mechanism inert on the only viewports it was written for (GEO-2974). Asserting the class is + * crude, but it is the only trace of the mistake that survives into JSDOM. + */ + it('keeps the reserve displayed at the mobile breakpoint, where it is the only thing holding the page', () => { + mockMobileActivityGeometry(600); + + const { container } = render( + + ); + const reserve = container.querySelector('[data-activity-scroll-reserve]'); + + // `md:hidden` would switch it off below 768px, which is every phone. + expect(reserve?.className).not.toMatch(/(^|\s)md:hidden(\s|$)/); + // And it has to be on at that width rather than merely not off. + expect(reserve?.className).toMatch(/(^|\s)md:block(\s|$)/); + }); + + /** + * Susan Winter's profile, which is where this was reported: an Activity card with nothing below it, + * so the document barely exceeds the viewport and the card's own height is the entire scroll range. + * Switching to the shorter view takes more height out of the page than the page has to spare. + */ + it('holds the whole missing range on a profile with nothing below Activity', () => { + // 40px of page besides the card: a name and an avatar, no sections after it. + mockMobileActivityGeometry(40); + + const { container } = render( + + ); + const reserve = container.querySelector('[data-activity-scroll-reserve]'); + + // Nothing to hold before a switch: the page is however tall it is. + expect(reserve).toHaveStyle({ height: '0px' }); + + fireEvent.click(screen.getByRole('button', { name: /Claims/ })); + + // Claims: 40 + 250 = 290 natural, and the reader's viewport bottom is at 1000. + expect(reserve).toHaveStyle({ height: '710px' }); + + // And it has to be on screen to mean anything. On this profile the reserve is the only thing + // between the reader and the top of the page, so a height it is not allowed to render is the + // same as no fix at all — which is how this shipped once already. + expect(reserve?.className).toMatch(/(^|\s)md:block(\s|$)/); + expect(reserve?.className).not.toMatch(/(^|\s)md:hidden(\s|$)/); + }); + + /** + * The reserve is in the document a frame before React swaps the view, but the gallery can paint + * empty while its queries land, and a document shorter than the reserve can cover takes the reader + * with it. Where that happens they are put back, before the browser paints. + */ + it('puts the reader back when a shrink beat the reserve to it', () => { + const scroll = mockMobileActivityGeometry(600); + const scrollTo = vi.spyOn(window, 'scrollTo').mockImplementation(() => {}); + + render(); + + // The switch reads the position once on the way in; by the time the effect looks again the + // browser has moved the reader, which is the ordering this recovery exists for. + scroll.moveAfterFirstRead = 150; + fireEvent.click(screen.getByRole('button', { name: /Claims/ })); + + expect(scrollTo).toHaveBeenCalledWith(0, 400); + }); + + it('leaves the reader alone when the reserve did its job', () => { + mockMobileActivityGeometry(600); + const scrollTo = vi.spyOn(window, 'scrollTo').mockImplementation(() => {}); + + render(); + + fireEvent.click(screen.getByRole('button', { name: /Claims/ })); + + expect(scrollTo).not.toHaveBeenCalled(); + }); + it('adds no reserve when content below Activity already preserves the scroll range', () => { mockMobileActivityGeometry(900); diff --git a/apps/web/partials/profile/profile-activity-section.tsx b/apps/web/partials/profile/profile-activity-section.tsx index f9773376bb..396d91cda9 100644 --- a/apps/web/partials/profile/profile-activity-section.tsx +++ b/apps/web/partials/profile/profile-activity-section.tsx @@ -175,21 +175,33 @@ export function ProfileActivitySection({ kinds }: { kinds: ActivityKind[] }) { * stays compact while switching away from the taller Debates view cannot * clamp the viewport upward. Profiles with content below Activity need no * reserve at all, and desktop keeps its natural layout. + * + * `hidden md:block`, not `md:hidden`: the breakpoints here are desktop-first + * (`md` is `max-width: 767px`, see styles.css), so `md:hidden` hid this on + * exactly the phones it exists for — `display: none` reserves no height, and + * the fix was inert on the only screens that needed it. */} -
+
); } /** - * Preserve the mobile scroll range while Activity views of different heights - * are swapped. There is no scroll position to restore when the new document is - * shorter than the viewport's old bottom; keeping only that missing height in - * the document is what prevents the browser from clamping `scrollY`. + * Keep the reader where they were while Activity views of different heights are swapped. + * + * Switching to a shorter view makes the document shorter, and a document that no longer reaches the + * reader's viewport bottom has no scroll position to hold them at — the browser moves them up. On a + * profile with content below Activity there is other height to absorb that; on a short one, like a + * person with only an Activity card, there is none, and the page jumps to the top. + * + * Two things hold the position, in order of precedence: * - * The reserve is a sibling of the card rather than a `min-height` on it. That - * leaves the selected gallery and its footer at their natural height instead - * of putting a short Claims row inside a debate-sized white card. + * 1. A sibling reserve supplies exactly the document height missing below the viewport, so the + * shorter view never shortens the scrollable range. It is a sibling rather than a `min-height` + * on the card, which keeps a three-row Claims gallery from sitting inside a debate-sized box. + * 2. If the position is lost anyway — the gallery can paint empty for a frame before its queries + * land, which shortens the document below even the reserve's reach — it is put back before the + * browser paints. */ function useMobileActivityHeightReserve(selectedKey: string | undefined) { const sectionRef = React.useRef(null); @@ -198,8 +210,11 @@ function useMobileActivityHeightReserve(selectedKey: string | undefined) { width: number; sectionHeight: number; naturalDocumentHeight: number; - viewportBottom: number; + scrollY: number; } | null>(null); + // Set while this hook is the one moving the page, so its own correction is not mistaken below for + // the reader choosing to scroll. + const restoringRef = React.useRef(false); const prepareSwitch = React.useCallback(() => { const section = sectionRef.current; @@ -212,12 +227,13 @@ function useMobileActivityHeightReserve(selectedKey: string | undefined) { width, sectionHeight: height, naturalDocumentHeight: document.documentElement.scrollHeight - currentReserve, - viewportBottom: window.scrollY + window.innerHeight, + scrollY: window.scrollY, }; - // Deliberately over-reserve before the swap. The layout effect replaces - // this with the exact missing scroll range before the browser paints the - // new view. + // Hold the outgoing view's whole height before React replaces it. Waiting for the layout effect + // would leave a window where the document is short and the position is already gone; over- + // reserving now costs nothing, because the effect below replaces it with the exact figure before + // anything is painted. reserve.style.height = `${height}px`; }, []); @@ -230,9 +246,9 @@ function useMobileActivityHeightReserve(selectedKey: string | undefined) { const { width, height } = section.getBoundingClientRect(); const swap = swapRef.current; - // A new layout width (rotation, resized side panel, breakpoint change) - // has different card wrapping. Drop the old calculation; the next tab - // switch will establish one for the new layout. + // A new layout width (rotation, resized side panel, breakpoint change) has different card + // wrapping. Drop the old calculation; the next tab switch will establish one for the new + // layout. if (!swap || Math.abs(swap.width - width) > 1) { swapRef.current = null; reserve.style.height = '0px'; @@ -240,24 +256,40 @@ function useMobileActivityHeightReserve(selectedKey: string | undefined) { } const naturalDocumentHeight = swap.naturalDocumentHeight - swap.sectionHeight + height; - const missingScrollRange = Math.max(0, swap.viewportBottom - naturalDocumentHeight); + const missingScrollRange = Math.max(0, swap.scrollY + window.innerHeight - naturalDocumentHeight); reserve.style.height = `${missingScrollRange}px`; + + // The reserve is in the document now, so the position asked for is reachable again. Anything + // that already moved the reader — a frame painted before the reserve was in place — is undone + // here, synchronously, so they never see it. + if (Math.abs(window.scrollY - swap.scrollY) > 1) { + restoringRef.current = true; + window.scrollTo(0, swap.scrollY); + } }; sync(); - // Claim cards grow as their queries land. Shrink the reserve by the same - // amount so the overall document height stays steady rather than drifting. if (typeof ResizeObserver === 'undefined') return; + // Claim cards grow as their queries land. Shrink the reserve by the same amount so the overall + // document height stays steady rather than drifting. const observer = new ResizeObserver(sync); observer.observe(section); + const onScroll = () => { + // The correction above, arriving as a scroll event. Treating it as the reader moving up would + // release the very height that made the correction possible. + if (restoringRef.current) { + restoringRef.current = false; + return; + } + const swap = swapRef.current; if (!swap) return; - // Once the reader moves up, do not retain space they no longer need. - swap.viewportBottom = Math.min(swap.viewportBottom, window.scrollY + window.innerHeight); + // Once the reader moves up of their own accord, stop holding space they no longer need. + swap.scrollY = Math.min(swap.scrollY, window.scrollY); sync(); }; window.addEventListener('scroll', onScroll, { passive: true }); From 4876473aff9319e655b49ed469a2535eea7f4266 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Sat, 19 Sep 2026 16:15:00 -0700 Subject: [PATCH 07/19] fix(profile): widen the activity claim card so its pills sit side by side (GEO-2974) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agree and Disagree were stacking on a phone. The card was `80cqw`, which came to 283px on a 390px viewport, and the card's own padding took 26px off that — leaving the pill row 257px, under the 272px `claim-pills-wide` needs to fit both labels whole. So the row fell back to one column, which is the intended behaviour for a genuinely narrow card and the wrong answer for this one. At 88% the card is 311px, the pill row gets 285px, and the pills sit side by side: before card 282.88 pill row 256.88 grid-template-columns: 256.875px after card 311.16 pill row 285.16 grid-template-columns: 138.578px 138.578px A percentage rather than a pixel floor, so the card still cannot grow wider than the space it is in: phones narrower than this one keep stacking, which is the container query doing its job rather than a card overflowing the screen. The trade is the sliver of the next card, which goes from 39px to 10px — still enough to show the gallery scrolls sideways. Scroll position across tab switches is unaffected: six switches at 390×664 on the widened card hold at 0px, with the reserve absorbing the larger swing (182px rather than 146px). --- apps/web/partials/profile/profile-activity-section.tsx | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/apps/web/partials/profile/profile-activity-section.tsx b/apps/web/partials/profile/profile-activity-section.tsx index 396d91cda9..8b7b2f1ed9 100644 --- a/apps/web/partials/profile/profile-activity-section.tsx +++ b/apps/web/partials/profile/profile-activity-section.tsx @@ -469,7 +469,13 @@ function GalleryCard({ // be three times wider, so `80vw` there is not 80% of anything the reader // can see. The scroller establishes the container this measures — see // `ActivityGallery`. - 'w-[min(420px,80cqw)] shrink-0 snap-start', + // 88%, not 80%: at 80 the card came to 283px on a 390px phone, which left the pill row + // 257px once the card's own padding was off it — under the 272px that `claim-pills-wide` + // needs to put Agree and Disagree side by side, so they stacked. 88 gives the row 285px + // and keeps a sliver of the next card in view, which is what says the gallery scrolls. + // Narrower phones still stack, which is the container query doing its job rather than a + // card growing wider than the screen it is on. + 'w-[min(420px,88cqw)] shrink-0 snap-start', // The lobby card brings its own outline; the feed's card does not, and // draws a rule underneath itself to separate it from the next card // *down* — which in a row is a line under nothing. From 2f0ed06ad13849a7e04c715330401cee38b04e0b Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Sat, 19 Sep 2026 16:34:44 -0700 Subject: [PATCH 08/19] feat(profile): stop nesting the mobile activity gallery inside a card (GEO-2974) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A bordered panel holding bordered cards spends two gutters and two rules on saying "these belong together", which the heading already says. On a 390px phone that was most of what the claim pills were short of: 105px of the screen, 27%, went to chrome before the buttons. On mobile the section stops being a card. The heading and its rule stay, the box around them goes, and the gallery bleeds out through the app shell's gutter so the next card is clipped by the screen edge rather than by a panel — which is also what makes it read as a carousel. Desktop is untouched and keeps the card. The point is not the pixels so much as what they buy. Widening the card inside the panel had the pills and the next-card peek competing for the same space: card pill row peek boxed, 80cqw 283 257 39 pills stacked boxed, 88cqw 311 285 10 pills fit, peek nearly gone unwrapped, 84cqw 313 287 44 both The bleed is on the `@container` rather than the scroller, because `cqw` measures the container: bleeding only what scrolls gives the reader more to look at without giving the cards more to size against, which left the pill row 0.7px over its threshold. Scroll position across tab switches is unchanged — six switches at 390×664 hold at 0px, the reserve absorbing 184px. --- .../profile/profile-activity-section.tsx | 34 ++++++++++++------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/apps/web/partials/profile/profile-activity-section.tsx b/apps/web/partials/profile/profile-activity-section.tsx index 8b7b2f1ed9..90564945de 100644 --- a/apps/web/partials/profile/profile-activity-section.tsx +++ b/apps/web/partials/profile/profile-activity-section.tsx @@ -99,11 +99,18 @@ export function ProfileActivitySection({ kinds }: { kinds: ActivityKind[] }) {
{/* The toggles sit to the right of the heading, and wrap below it rather than squeezing into it on a narrow screen. */} -
+

Activity

{/* Only when there is a choice to make. One pill on its own is a label @@ -341,12 +348,17 @@ function ActivityGallery({ element whose overflow is the whole point is a bad trade for one class. The wrapper is the width the reader actually sees, which is what the cards want to measure — see `GalleryCard`. */} -
+ {/* The bleed is on the container rather than the scroller, because `cqw` below measures this + element: widening only what scrolls would give the reader more to look at without giving the + cards any more to size against. Out through the app shell's own gutter (`2ch`, see the + layout) on a phone, so a card can use the full width and the one behind it is cut off by the + screen edge rather than by a panel. */} +
- + {shown.map(row => ( Date: Sat, 19 Sep 2026 16:56:36 -0700 Subject: [PATCH 09/19] fix(profile): stop the activity reserve dragging the reader back up (GEO-2974) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scrolling down after a tab switch put the reader straight back where they were. My doing, in the commit that added the position recovery: sizing the reserve and correcting the position were the same function, and that function ran on every scroll event. Scrolling down leaves the held position where it is, so every scroll down was followed by a correction back to it — the page reading as though it refused to move. Measured at 390×664, scrolling down after switching to Claims, against what the page can actually reach: before asked 300 → landed 200 (reachable 546) asked 400 → landed 200 (reachable 546) after asked 300 → landed 300 (reachable 546) asked 400 → landed 400 (reachable 546) The two jobs are separate now. `sizeReserve` only sizes; it is what runs on scroll and on resize. The correction runs once, in the layout effect for the swap it belongs to, which is all it was ever for — a frame painted before the reserve was in the document. And the scroll listener is armed a frame late, so the swap's own scroll events are not read as the reader moving, which is what the flag it replaces was trying to do. `swap.scrollY` is now `holdY`. It was never a reading of where the page is, it is the position being held, and the two being spelled the same is most of how this got written. The test that covers it needed a `ResizeObserver` first. JSDOM has none, so the hook was taking its no-observer path and never attaching the scroll listener — every assertion about scrolling passed for that reason rather than on merit. With one stubbed, the new test fails against the code this replaces. --- .../profile/profile-activity-section.test.tsx | 65 ++++++++++++++++++- .../profile/profile-activity-section.tsx | 65 ++++++++++--------- 2 files changed, 98 insertions(+), 32 deletions(-) diff --git a/apps/web/partials/profile/profile-activity-section.test.tsx b/apps/web/partials/profile/profile-activity-section.test.tsx index 9b8206d79d..2889737cab 100644 --- a/apps/web/partials/profile/profile-activity-section.test.tsx +++ b/apps/web/partials/profile/profile-activity-section.test.tsx @@ -1,5 +1,5 @@ import '@testing-library/jest-dom/vitest'; -import { cleanup, fireEvent, render, screen } from '@testing-library/react'; +import { act, cleanup, fireEvent, render, screen } from '@testing-library/react'; import * as React from 'react'; @@ -59,8 +59,22 @@ const rect = (width: number, height: number): DOMRect => ({ toJSON: () => ({}), }); -/** A 390×600 mobile viewport sitting 400px down a synthetic profile page. */ +/** + * A 390×600 mobile viewport sitting 400px down a synthetic profile page. + * + * Includes a `ResizeObserver`, which JSDOM has none of. Without one the hook takes its + * no-observer path and never attaches the scroll listener — so anything asserted about scrolling + * passed for the wrong reason, whatever the code did. + */ function mockMobileActivityGeometry(pageHeightWithoutActivity: number) { + class TestResizeObserver { + constructor(private readonly callback: ResizeObserverCallback) {} + observe() {} + unobserve() {} + disconnect() {} + } + vi.stubGlobal('ResizeObserver', TestResizeObserver); + const originalRect = HTMLElement.prototype.getBoundingClientRect; vi.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockImplementation(function (this: HTMLElement) { if ('activitySection' in this.dataset) { @@ -250,6 +264,53 @@ describe('ProfileActivitySection', () => { expect(scrollTo).toHaveBeenCalledWith(0, 400); }); + /** + * The reader has to be able to scroll afterwards. Sizing the reserve and correcting the position + * were the same function once, and that function ran on every scroll — so scrolling down, which + * leaves the held position where it was, corrected the reader straight back to it. The page read + * as refusing to move (GEO-2974). + */ + it('lets the reader scroll down after a switch', async () => { + const scroll = mockMobileActivityGeometry(600); + const scrollTo = vi.spyOn(window, 'scrollTo').mockImplementation(() => {}); + + render(); + fireEvent.click(screen.getByRole('button', { name: /Claims/ })); + scrollTo.mockClear(); + + // The scroll listener is armed a frame after the swap, so that the swap's own events are not + // read as the reader moving. Wait for it, then scroll down. + await act(async () => { + await new Promise(resolve => requestAnimationFrame(() => resolve(null))); + }); + + scroll.y = 700; + fireEvent.scroll(window); + + expect(scrollTo).not.toHaveBeenCalled(); + }); + + it('stops holding height the reader has scrolled back above', async () => { + const scroll = mockMobileActivityGeometry(600); + const { container } = render( + + ); + const reserve = container.querySelector('[data-activity-scroll-reserve]'); + + fireEvent.click(screen.getByRole('button', { name: /Claims/ })); + expect(reserve).toHaveStyle({ height: '150px' }); + + await act(async () => { + await new Promise(resolve => requestAnimationFrame(() => resolve(null))); + }); + + // Back up to the top: nothing below the viewport needs holding any more. + scroll.y = 0; + fireEvent.scroll(window); + + expect(reserve).toHaveStyle({ height: '0px' }); + }); + it('leaves the reader alone when the reserve did its job', () => { mockMobileActivityGeometry(600); const scrollTo = vi.spyOn(window, 'scrollTo').mockImplementation(() => {}); diff --git a/apps/web/partials/profile/profile-activity-section.tsx b/apps/web/partials/profile/profile-activity-section.tsx index 90564945de..9375c68e6d 100644 --- a/apps/web/partials/profile/profile-activity-section.tsx +++ b/apps/web/partials/profile/profile-activity-section.tsx @@ -217,12 +217,9 @@ function useMobileActivityHeightReserve(selectedKey: string | undefined) { width: number; sectionHeight: number; naturalDocumentHeight: number; - scrollY: number; + /** The position being held for the reader, which only ever moves up. */ + holdY: number; } | null>(null); - // Set while this hook is the one moving the page, so its own correction is not mistaken below for - // the reader choosing to scroll. - const restoringRef = React.useRef(false); - const prepareSwitch = React.useCallback(() => { const section = sectionRef.current; const reserve = reserveRef.current; @@ -234,7 +231,7 @@ function useMobileActivityHeightReserve(selectedKey: string | undefined) { width, sectionHeight: height, naturalDocumentHeight: document.documentElement.scrollHeight - currentReserve, - scrollY: window.scrollY, + holdY: window.scrollY, }; // Hold the outgoing view's whole height before React replaces it. Waiting for the layout effect @@ -249,7 +246,10 @@ function useMobileActivityHeightReserve(selectedKey: string | undefined) { const reserve = reserveRef.current; if (!section || !reserve) return; - const sync = () => { + // Sizing only. Nothing here moves the reader: this runs on every scroll, and a function that + // both sizes the reserve and corrects the position will correct it every time they scroll — + // which reads as the page refusing to move (GEO-2974). + const sizeReserve = () => { const { width, height } = section.getBoundingClientRect(); const swap = swapRef.current; @@ -263,45 +263,50 @@ function useMobileActivityHeightReserve(selectedKey: string | undefined) { } const naturalDocumentHeight = swap.naturalDocumentHeight - swap.sectionHeight + height; - const missingScrollRange = Math.max(0, swap.scrollY + window.innerHeight - naturalDocumentHeight); - - reserve.style.height = `${missingScrollRange}px`; - - // The reserve is in the document now, so the position asked for is reachable again. Anything - // that already moved the reader — a frame painted before the reserve was in place — is undone - // here, synchronously, so they never see it. - if (Math.abs(window.scrollY - swap.scrollY) > 1) { - restoringRef.current = true; - window.scrollTo(0, swap.scrollY); - } + reserve.style.height = `${Math.max(0, swap.holdY + window.innerHeight - naturalDocumentHeight)}px`; }; - sync(); + sizeReserve(); + + // The reserve is in the document now, so the position asked for is reachable again. If a frame + // painted before it was — the gallery can paint empty while its queries land — the reader is put + // back here, once, before the browser paints. Once, because this is a correction for the swap + // that just happened and not a rule about where the page may be scrolled to. + const swap = swapRef.current; + if (swap && Math.abs(window.scrollY - swap.holdY) > 1) { + window.scrollTo(0, swap.holdY); + } if (typeof ResizeObserver === 'undefined') return; // Claim cards grow as their queries land. Shrink the reserve by the same amount so the overall // document height stays steady rather than drifting. - const observer = new ResizeObserver(sync); + const observer = new ResizeObserver(sizeReserve); observer.observe(section); + // Armed a frame late, so the scroll events belonging to the swap itself — the correction above, + // and any clamp it was correcting — are not read as the reader choosing to move. + let armed = false; + const arm = requestAnimationFrame(() => { + armed = true; + }); + const onScroll = () => { - // The correction above, arriving as a scroll event. Treating it as the reader moving up would - // release the very height that made the correction possible. - if (restoringRef.current) { - restoringRef.current = false; - return; - } + if (!armed) return; - const swap = swapRef.current; - if (!swap) return; + const current = swapRef.current; + if (!current) return; // Once the reader moves up of their own accord, stop holding space they no longer need. - swap.scrollY = Math.min(swap.scrollY, window.scrollY); - sync(); + // Moving down needs nothing held and nothing released. + if (window.scrollY < current.holdY) { + current.holdY = window.scrollY; + sizeReserve(); + } }; window.addEventListener('scroll', onScroll, { passive: true }); return () => { + cancelAnimationFrame(arm); observer.disconnect(); window.removeEventListener('scroll', onScroll); }; From 0a74302ff296fa6f2440630873f5b8cdec4a8714 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Sat, 19 Sep 2026 17:23:25 -0700 Subject: [PATCH 10/19] feat(profile): land See all on the tab bar rather than the page top (GEO-2974) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tapping "See all debates" from the Activity card put the reader at the top of the profile: a phone screenful of cover, avatar, name, roles and bio, none of which is what they clicked for. The tab row is the one thing worth seeing on arrival — it says which tab the link sent them to — so the link now carries a fragment that puts that row under the navbar, with the list starting right below it. A fragment rather than a scroll written by hand because of when each one runs: the router applies a fragment after the destination renders, which is the first moment the page is tall enough to hold the position, while scrolling on click runs against the page being left and lands short on a profile barely a screen tall. --- apps/web/app/space/[id]/(space)/layout.tsx | 31 ++++++++++++------- .../profile/profile-activity-section.test.tsx | 25 +++++++++++++++ .../profile/profile-activity-section.tsx | 11 ++++++- apps/web/partials/space-page/space-tabs.tsx | 21 +++++++++++++ 4 files changed, 75 insertions(+), 13 deletions(-) diff --git a/apps/web/app/space/[id]/(space)/layout.tsx b/apps/web/app/space/[id]/(space)/layout.tsx index cd18b661ff..4dd481867c 100644 --- a/apps/web/app/space/[id]/(space)/layout.tsx +++ b/apps/web/app/space/[id]/(space)/layout.tsx @@ -35,7 +35,7 @@ import { AddDataPanel } from '~/partials/space-page/add-data-panel'; import { SpaceEditors } from '~/partials/space-page/space-editors'; import { SpaceMembers } from '~/partials/space-page/space-members'; import { SpacePageMetadataHeader } from '~/partials/space-page/space-metadata-header'; -import { SpaceTabs } from '~/partials/space-page/space-tabs'; +import { SPACE_TABS_ANCHOR, SpaceTabs } from '~/partials/space-page/space-tabs'; import type { PersonRecordCounts } from '~/partials/space-page/space-tabs'; import { cachedFetchEntitiesBatch, cachedFetchEntityPage } from '../../(entity)/[id]/[entityId]/cached-fetch-entity'; @@ -209,17 +209,24 @@ export default async function Layout(props0: LayoutProps) { ) : null} - - - + {/* + * The tab bar is a link target, so a link can send the reader to the tabs rather than + * to the top of the page — see `withSpaceTabsAnchor`. `scroll-mt-14` clears the sticky + * navbar above it, which would otherwise cover the row the link exists to show. + */} +
+ + + +
diff --git a/apps/web/partials/profile/profile-activity-section.test.tsx b/apps/web/partials/profile/profile-activity-section.test.tsx index 2889737cab..9c537f3689 100644 --- a/apps/web/partials/profile/profile-activity-section.test.tsx +++ b/apps/web/partials/profile/profile-activity-section.test.tsx @@ -172,6 +172,31 @@ describe('ProfileActivitySection', () => { expect(screen.getByRole('button', { name: /Debates/ })).toHaveTextContent('—'); }); + it('sends See all to the tab bar rather than the top of the page', () => { + render( + + ); + + // Without the fragment the reader lands at the top of the profile — a screenful of cover, + // avatar, name, roles and bio — rather than on the list they clicked for. + expect(screen.getByRole('link', { name: /See all debates/ })).toHaveAttribute( + 'href', + '/space/s/debates#space-tabs' + ); + + fireEvent.click(screen.getByRole('button', { name: /Claims/ })); + + expect(screen.getByRole('link', { name: /See all claims/ })).toHaveAttribute( + 'href', + '/space/s/positions#space-tabs' + ); + }); + it('reserves the lost mobile document height while switching between kinds', () => { mockMobileActivityGeometry(600); diff --git a/apps/web/partials/profile/profile-activity-section.tsx b/apps/web/partials/profile/profile-activity-section.tsx index 9375c68e6d..f7aab0e880 100644 --- a/apps/web/partials/profile/profile-activity-section.tsx +++ b/apps/web/partials/profile/profile-activity-section.tsx @@ -15,6 +15,7 @@ import { RightArrowLongSmall } from '~/design-system/icons/right-arrow-long-smal import { PrefetchLink as Link } from '~/design-system/prefetch-link'; import { ExploreFeedCard } from '~/partials/explore/explore-feed-card'; +import { withSpaceTabsAnchor } from '~/partials/space-page/space-tabs'; import { GalleryClaimCard } from './gallery-claim-card'; @@ -167,8 +168,16 @@ export function ProfileActivitySection({ kinds }: { kinds: ActivityKind[] }) { personName={selected.personName} /> )} + {/* + * Lands on the tab bar, not the page top. + * + * A navigation lands at the top of the page, which on a phone is a screenful of cover, + * avatar, name, roles and bio — none of it what "See all debates" was clicked for. The + * fragment puts the tab row under the navbar instead, so the list opens at the top of the + * screen with the underlined tab above it saying where the reader has been sent. + */} {selected.seeAllLabel} diff --git a/apps/web/partials/space-page/space-tabs.tsx b/apps/web/partials/space-page/space-tabs.tsx index 49cee6f077..39adda184e 100644 --- a/apps/web/partials/space-page/space-tabs.tsx +++ b/apps/web/partials/space-page/space-tabs.tsx @@ -41,6 +41,27 @@ type BuiltSpaceTab = { /** The record routes on a profile. Reachable only by their own tab — see the dedupe below. */ const PERSON_TAB_LABELS = ['Debates', 'Positions', 'Proposals', 'About'] as const; +/** + * The tab bar's own id, so a link can land on the tabs instead of the top of the page. + * + * A fragment rather than a scroll written by hand, because of when each one runs. The router + * applies a fragment once the destination has rendered, which is the first moment the page is tall + * enough to hold the position; scrolling on click instead runs against the page being left, and a + * profile whose Overview is barely a screen tall has nowhere to put the reader, so they land short. + * + * It does not survive a cold load of the link — the lists render on the client, so the document is + * still one screen tall when the browser looks for the fragment. That leaves the reader at the top + * of the profile, which is where a cold load leaves them anyway. + * + * Whoever renders the bar owns the id — see the space layout. + */ +export const SPACE_TABS_ANCHOR = 'space-tabs'; + +/** `href` with the fragment that lands the reader on the tab bar rather than the page top. */ +export function withSpaceTabsAnchor(href: string) { + return `${href}#${SPACE_TABS_ANCHOR}`; +} + /** * The About tab, defined once for both paths that draw it. * From 1613abb8a4be27513cc668f6d3ab15d11a9ec843 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Sat, 19 Sep 2026 17:44:46 -0700 Subject: [PATCH 11/19] refactor(profile): tighten the activity tests and the comments around them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review pass over the branch. The reserve tests each rendered the same two kinds and reached for the same element, and two of them spelled out the same wait for the armed scroll listener; both are one helper now. The short-profile test re-asserted the two classes the test above it exists for, which is the one place that claim belongs. Added the case the suite was missing. The sizing path has two callers — a scroll back up and the section resizing as claim cards load — and only the first was covered, so folding the position correction back into the sizing function was a regression the tests would have waved through. The stub now hands back the observer's callback so a test can stand in for the cards growing. Comments corrected where the code had moved under them: the card the reserve sits outside of is gone on the screens it exists for, the cards are 84cqw rather than 80vw, and the tab bar's scroll margin now says which navbar it is clearing. --- apps/web/app/space/[id]/(space)/layout.tsx | 8 +- .../profile/profile-activity-section.test.tsx | 118 +++++++++++------- .../profile/profile-activity-section.tsx | 48 +++---- 3 files changed, 106 insertions(+), 68 deletions(-) diff --git a/apps/web/app/space/[id]/(space)/layout.tsx b/apps/web/app/space/[id]/(space)/layout.tsx index 4dd481867c..c90315c0b9 100644 --- a/apps/web/app/space/[id]/(space)/layout.tsx +++ b/apps/web/app/space/[id]/(space)/layout.tsx @@ -211,8 +211,12 @@ export default async function Layout(props0: LayoutProps) { {/* * The tab bar is a link target, so a link can send the reader to the tabs rather than - * to the top of the page — see `withSpaceTabsAnchor`. `scroll-mt-14` clears the sticky - * navbar above it, which would otherwise cover the row the link exists to show. + * to the top of the page — see `withSpaceTabsAnchor`. + * + * The margin clears the navbar, which is `sticky top-0 h-11` and would otherwise cover + * the row the link exists to show — the same 44px the hub panel and the side rail sit + * below as `top-11`. `scroll-mt-14` is that plus 12px, so the tabs land under the + * navbar rather than welded to it. */}
diff --git a/apps/web/partials/profile/profile-activity-section.test.tsx b/apps/web/partials/profile/profile-activity-section.test.tsx index 9c537f3689..554b3792c9 100644 --- a/apps/web/partials/profile/profile-activity-section.test.tsx +++ b/apps/web/partials/profile/profile-activity-section.test.tsx @@ -67,8 +67,11 @@ const rect = (width: number, height: number): DOMRect => ({ * passed for the wrong reason, whatever the code did. */ function mockMobileActivityGeometry(pageHeightWithoutActivity: number) { + let notifyResize: (() => void) | null = null; class TestResizeObserver { - constructor(private readonly callback: ResizeObserverCallback) {} + constructor(callback: ResizeObserverCallback) { + notifyResize = () => callback([], this as unknown as ResizeObserver); + } observe() {} unobserve() {} disconnect() {} @@ -108,9 +111,40 @@ function mockMobileActivityGeometry(pageHeightWithoutActivity: number) { ); }); - // Returned so a test can stand in for the browser moving the reader — there is no real layout - // here to clamp a scroll position, and the recovery path exists for exactly that case. - return scroll; + return { + // Stands in for the browser moving the reader: there is no real layout here to clamp a scroll + // position, and the recovery path exists for exactly that case. + scroll, + // And for the claim cards growing as their queries land, which is the other way the sizing + // path runs after a switch. + sectionResized: () => notifyResize?.(), + }; +} + +/** + * The card with both kinds, and the reserve element the tests below assert on. + * + * Every reserve test wants the same two kinds — a tall Debates view and a short Claims one — since + * what they are about is the switch between them, not what is in either. + */ +function renderActivity() { + const { container } = render( + + ); + + return { reserve: container.querySelector('[data-activity-scroll-reserve]') }; +} + +/** + * Wait for the scroll listener to arm. + * + * It arms a frame after a swap, so the swap's own scroll events are not read as the reader moving — + * a test that scrolls straight after a switch is testing the disarmed frame and nothing else. + */ +function armScrollListener() { + return act(async () => { + await new Promise(resolve => requestAnimationFrame(() => resolve(null))); + }); } /** @@ -200,10 +234,7 @@ describe('ProfileActivitySection', () => { it('reserves the lost mobile document height while switching between kinds', () => { mockMobileActivityGeometry(600); - const { container } = render( - - ); - const reserve = container.querySelector('[data-activity-scroll-reserve]'); + const { reserve } = renderActivity(); expect(reserve).toHaveStyle({ height: '0px' }); @@ -230,10 +261,7 @@ describe('ProfileActivitySection', () => { it('keeps the reserve displayed at the mobile breakpoint, where it is the only thing holding the page', () => { mockMobileActivityGeometry(600); - const { container } = render( - - ); - const reserve = container.querySelector('[data-activity-scroll-reserve]'); + const { reserve } = renderActivity(); // `md:hidden` would switch it off below 768px, which is every phone. expect(reserve?.className).not.toMatch(/(^|\s)md:hidden(\s|$)/); @@ -250,10 +278,7 @@ describe('ProfileActivitySection', () => { // 40px of page besides the card: a name and an avatar, no sections after it. mockMobileActivityGeometry(40); - const { container } = render( - - ); - const reserve = container.querySelector('[data-activity-scroll-reserve]'); + const { reserve } = renderActivity(); // Nothing to hold before a switch: the page is however tall it is. expect(reserve).toHaveStyle({ height: '0px' }); @@ -261,13 +286,8 @@ describe('ProfileActivitySection', () => { fireEvent.click(screen.getByRole('button', { name: /Claims/ })); // Claims: 40 + 250 = 290 natural, and the reader's viewport bottom is at 1000. + // That it is also allowed to render at this width is the test above. expect(reserve).toHaveStyle({ height: '710px' }); - - // And it has to be on screen to mean anything. On this profile the reserve is the only thing - // between the reader and the top of the page, so a height it is not allowed to render is the - // same as no fix at all — which is how this shipped once already. - expect(reserve?.className).toMatch(/(^|\s)md:block(\s|$)/); - expect(reserve?.className).not.toMatch(/(^|\s)md:hidden(\s|$)/); }); /** @@ -276,10 +296,10 @@ describe('ProfileActivitySection', () => { * with it. Where that happens they are put back, before the browser paints. */ it('puts the reader back when a shrink beat the reserve to it', () => { - const scroll = mockMobileActivityGeometry(600); + const { scroll } = mockMobileActivityGeometry(600); const scrollTo = vi.spyOn(window, 'scrollTo').mockImplementation(() => {}); - render(); + renderActivity(); // The switch reads the position once on the way in; by the time the effect looks again the // browser has moved the reader, which is the ordering this recovery exists for. @@ -296,18 +316,14 @@ describe('ProfileActivitySection', () => { * as refusing to move (GEO-2974). */ it('lets the reader scroll down after a switch', async () => { - const scroll = mockMobileActivityGeometry(600); + const { scroll } = mockMobileActivityGeometry(600); const scrollTo = vi.spyOn(window, 'scrollTo').mockImplementation(() => {}); - render(); + renderActivity(); fireEvent.click(screen.getByRole('button', { name: /Claims/ })); scrollTo.mockClear(); - // The scroll listener is armed a frame after the swap, so that the swap's own events are not - // read as the reader moving. Wait for it, then scroll down. - await act(async () => { - await new Promise(resolve => requestAnimationFrame(() => resolve(null))); - }); + await armScrollListener(); scroll.y = 700; fireEvent.scroll(window); @@ -316,18 +332,13 @@ describe('ProfileActivitySection', () => { }); it('stops holding height the reader has scrolled back above', async () => { - const scroll = mockMobileActivityGeometry(600); - const { container } = render( - - ); - const reserve = container.querySelector('[data-activity-scroll-reserve]'); + const { scroll } = mockMobileActivityGeometry(600); + const { reserve } = renderActivity(); fireEvent.click(screen.getByRole('button', { name: /Claims/ })); expect(reserve).toHaveStyle({ height: '150px' }); - await act(async () => { - await new Promise(resolve => requestAnimationFrame(() => resolve(null))); - }); + await armScrollListener(); // Back up to the top: nothing below the viewport needs holding any more. scroll.y = 0; @@ -336,11 +347,33 @@ describe('ProfileActivitySection', () => { expect(reserve).toHaveStyle({ height: '0px' }); }); + /** + * The other caller of the sizing path. A claim card grows when its queries land, which resizes + * the section — and resizing the reserve is the whole response to that. Moving the reader is not, + * wherever they have got to by then: the correction belongs to the swap that asked for it. + */ + it('leaves the reader alone when the cards grow after a switch', async () => { + const { scroll, sectionResized } = mockMobileActivityGeometry(600); + const scrollTo = vi.spyOn(window, 'scrollTo').mockImplementation(() => {}); + + renderActivity(); + fireEvent.click(screen.getByRole('button', { name: /Claims/ })); + scrollTo.mockClear(); + await armScrollListener(); + + scroll.y = 700; + act(() => { + sectionResized(); + }); + + expect(scrollTo).not.toHaveBeenCalled(); + }); + it('leaves the reader alone when the reserve did its job', () => { mockMobileActivityGeometry(600); const scrollTo = vi.spyOn(window, 'scrollTo').mockImplementation(() => {}); - render(); + renderActivity(); fireEvent.click(screen.getByRole('button', { name: /Claims/ })); @@ -350,10 +383,7 @@ describe('ProfileActivitySection', () => { it('adds no reserve when content below Activity already preserves the scroll range', () => { mockMobileActivityGeometry(900); - const { container } = render( - - ); - const reserve = container.querySelector('[data-activity-scroll-reserve]'); + const { reserve } = renderActivity(); fireEvent.click(screen.getByRole('button', { name: /Claims/ })); diff --git a/apps/web/partials/profile/profile-activity-section.tsx b/apps/web/partials/profile/profile-activity-section.tsx index f7aab0e880..df1b6fdde8 100644 --- a/apps/web/partials/profile/profile-activity-section.tsx +++ b/apps/web/partials/profile/profile-activity-section.tsx @@ -187,7 +187,7 @@ export function ProfileActivitySection({ kinds }: { kinds: ActivityKind[] }) { {/* * On narrow screens this supplies only the document height missing below - * the current viewport. The reserve sits outside the card, so Claims + * the current viewport. The reserve sits outside the section, so Claims * stays compact while switching away from the taller Debates view cannot * clamp the viewport upward. Profiles with content below Activity need no * reserve at all, and desktop keeps its natural layout. @@ -214,7 +214,7 @@ export function ProfileActivitySection({ kinds }: { kinds: ActivityKind[] }) { * * 1. A sibling reserve supplies exactly the document height missing below the viewport, so the * shorter view never shortens the scrollable range. It is a sibling rather than a `min-height` - * on the card, which keeps a three-row Claims gallery from sitting inside a debate-sized box. + * on the section, which keeps a three-row Claims gallery from sitting inside a debate-sized box. * 2. If the position is lost anyway — the gallery can paint empty for a frame before its queries * land, which shortens the document below even the reserve's reach — it is put back before the * browser paints. @@ -349,25 +349,29 @@ function ActivityGallery({ // start. The gate names the one nearest the middle. {/* - * `snap-x` so a flick lands on a card rather than between two. + * The wrapper, not the scroller, carries both of these. * - * The gap at either end is a spacer element rather than padding on the - * scroller: a scroll container's trailing padding is dropped by every - * browser that matters, so `p-4` gave 16px on the left and nothing on the - * right. Spacers are honoured on both sides, and `scroll-px` keeps a - * snapped card off the edge it lands against. + * `@container`, because the container types imply `contain: inline-size`, and containing the + * element whose overflow is the whole point is a bad trade for one class. The wrapper is also + * the width the reader actually sees, which is what the cards want to measure — `cqw` below + * reads this element, so widening only what scrolls would give the reader more to look at + * without giving the cards any more to size against. + * + * The bleed takes it out through the app shell's own gutter on a phone, so a card can use the + * full width and the one behind it is cut off by the screen edge rather than by a panel. `2ch` + * is the shell's figure (`2xl:px-[2ch]` in `app/entry.tsx`) and the two have to stay equal, or + * the gallery hangs off the side of the document and every profile scrolls sideways. `md` is + * inside `2xl` in a desktop-first scale, so the gutter is always there to cancel. */} - {/* `@container` on a wrapper rather than on the scroller itself: the - container types imply `contain: inline-size`, and containing the - element whose overflow is the whole point is a bad trade for one class. - The wrapper is the width the reader actually sees, which is what the - cards want to measure — see `GalleryCard`. */} - {/* The bleed is on the container rather than the scroller, because `cqw` below measures this - element: widening only what scrolls would give the reader more to look at without giving the - cards any more to size against. Out through the app shell's own gutter (`2ch`, see the - layout) on a phone, so a card can use the full width and the one behind it is cut off by the - screen edge rather than by a panel. */}
+ {/* + * `snap-x` so a flick lands on a card rather than between two. + * + * The gap at either end is a spacer element rather than padding on the scroller: a scroll + * container's trailing padding is dropped by every browser that matters, so `p-4` gave 16px + * on the left and nothing on the right. Spacers are honoured on both sides, and `scroll-px` + * keeps a snapped card off the edge it lands against. + */}
Date: Sat, 19 Sep 2026 18:00:18 -0700 Subject: [PATCH 12/19] fix(profile): keep the tab anchor off the client boundary, and let the swap end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things from review. The space layout is a Server Component and read `SPACE_TABS_ANCHOR` out of a 'use client' module, so what it wrote into the id was a client reference, not the string: the flight payload came back as {"id":"$56","className":"scroll-mt-14"} and the element had no id until hydration resolved it — which is why a cold load of /space/…/debates#space-tabs had nothing to scroll to. The anchor and the href that points at it now live in a module with no directive on it. A guard test walks the server render graph for the same mistake anywhere else; five pre-existing cases are listed there with what each one does. The height reserve never let go. Once the cards had grown and the reserve reached zero the swap stayed armed, so a shrink long afterwards would size a reserve from a position the reader had left and hand them blank space to scroll into. Zero is the safe moment to drop it, because nothing is being held at zero. And a gated card now starts playing on sight. 0.6 of a card exists to stop a stack of them playing at once, which is what the gate is for wherever there is one; kept that high under a gate it only subtracts, and the Activity row shares one vertical ratio across every card, so a row half off the screen leaves the chosen card inside its own dead band. This was described in the PR and never written. --- apps/web/app/space/[id]/(space)/layout.tsx | 3 +- .../debates/debate-playback-gate.test.tsx | 36 +++- .../web/core/debates/debate-playback-gate.tsx | 13 ++ ...client-values-in-server-boundaries.test.ts | 185 ++++++++++++++++++ .../explore/debate-explore-feed-card.tsx | 36 +++- .../profile/profile-activity-section.test.tsx | 34 +++- .../profile/profile-activity-section.tsx | 34 +++- .../partials/space-page/space-tabs-anchor.ts | 27 +++ apps/web/partials/space-page/space-tabs.tsx | 21 -- 9 files changed, 348 insertions(+), 41 deletions(-) create mode 100644 apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts create mode 100644 apps/web/partials/space-page/space-tabs-anchor.ts diff --git a/apps/web/app/space/[id]/(space)/layout.tsx b/apps/web/app/space/[id]/(space)/layout.tsx index c90315c0b9..bfc2787655 100644 --- a/apps/web/app/space/[id]/(space)/layout.tsx +++ b/apps/web/app/space/[id]/(space)/layout.tsx @@ -35,8 +35,9 @@ import { AddDataPanel } from '~/partials/space-page/add-data-panel'; import { SpaceEditors } from '~/partials/space-page/space-editors'; import { SpaceMembers } from '~/partials/space-page/space-members'; import { SpacePageMetadataHeader } from '~/partials/space-page/space-metadata-header'; -import { SPACE_TABS_ANCHOR, SpaceTabs } from '~/partials/space-page/space-tabs'; +import { SpaceTabs } from '~/partials/space-page/space-tabs'; import type { PersonRecordCounts } from '~/partials/space-page/space-tabs'; +import { SPACE_TABS_ANCHOR } from '~/partials/space-page/space-tabs-anchor'; import { cachedFetchEntitiesBatch, cachedFetchEntityPage } from '../../(entity)/[id]/[entityId]/cached-fetch-entity'; import { cachedFetchSpace } from '../cached-fetch-space'; diff --git a/apps/web/core/debates/debate-playback-gate.test.tsx b/apps/web/core/debates/debate-playback-gate.test.tsx index 82260c0afd..616d914bd3 100644 --- a/apps/web/core/debates/debate-playback-gate.test.tsx +++ b/apps/web/core/debates/debate-playback-gate.test.tsx @@ -3,12 +3,16 @@ import { cleanup, render, screen } from '@testing-library/react'; import { afterEach, describe, expect, it } from 'vitest'; -import { DebatePlaybackGate, useDebatePlaybackAllowed } from './debate-playback-gate'; +import { DebatePlaybackGate, useDebatePlaybackAllowed, useIsDebatePlaybackGated } from './debate-playback-gate'; function Probe({ id }: { id: string }) { return {useDebatePlaybackAllowed(id) ? 'allowed' : 'held'}; } +function GatedProbe() { + return {useIsDebatePlaybackGated() ? 'gated' : 'ungated'}; +} + const A = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'; const B = 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb'; @@ -51,6 +55,36 @@ describe('DebatePlaybackGate', () => { expect(screen.getByTestId(A)).toHaveTextContent('held'); }); + /** + * A card asks this to decide how much of itself has to be on screen before it plays. Under a + * gate the answer is "any of it" — the gate has already chosen one card, so the card's own + * stricter ratio can only keep the chosen one silent. + */ + it('tells a card whether a gate is arbitrating', () => { + render(); + expect(screen.getByTestId('gated')).toHaveTextContent('ungated'); + + cleanup(); + + render( + + + + ); + expect(screen.getByTestId('gated')).toHaveTextContent('gated'); + }); + + it('is gated even while the gallery has chosen nobody', () => { + // `allowedId={null}` is a gate holding everything, not the absence of one. + render( + + + + ); + + expect(screen.getByTestId('gated')).toHaveTextContent('gated'); + }); + it('matches ids however they are spelled', () => { // Ids reach this from two directions — a relation and a card — and one of // them may carry dashes. Comparing them raw would hold the very card the diff --git a/apps/web/core/debates/debate-playback-gate.tsx b/apps/web/core/debates/debate-playback-gate.tsx index a3069d758a..87b87e3832 100644 --- a/apps/web/core/debates/debate-playback-gate.tsx +++ b/apps/web/core/debates/debate-playback-gate.tsx @@ -30,6 +30,19 @@ export function DebatePlaybackGate({ allowedId, children }: { allowedId: string return {children}; } +/** + * Whether a surface is holding playback to one debate at all. + * + * A card decides for itself whether enough of it is on screen to play, and that judgement is + * calibrated for a stack of cards where nothing else arbitrates. Under a gate it is a second, + * stricter arbiter that can only subtract — so a card the gate has chosen can sit in its own dead + * band and never start, with tapping the reader's only way out. Knowing a gate is in force lets a + * card ask the question it actually needs answered: is any of me on screen. + */ +export function useIsDebatePlaybackGated(): boolean { + return React.useContext(DebatePlaybackContext) !== undefined; +} + /** Whether this debate may play. True wherever no gate is in force. */ export function useDebatePlaybackAllowed(debateId: string): boolean { const allowed = React.useContext(DebatePlaybackContext); diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts new file mode 100644 index 0000000000..d3d076c75b --- /dev/null +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -0,0 +1,185 @@ +// This walks the source tree with `fs` and never touches the DOM. +// @vitest-environment node +import { existsSync, readFileSync, readdirSync } from 'node:fs'; +import path from 'node:path'; +import { describe, expect, it } from 'vitest'; + +/** + * Every export of a `'use client'` module is a *client reference* on the server, not the value it + * looks like. A Server Component that reads one gets an opaque placeholder: React writes it into + * the flight payload as `"$56"`, and the real value only appears once the client resolves the + * reference during hydration. + * + * Nothing fails. That is the whole problem. The space layout wrote `id={SPACE_TABS_ANCHOR}` where + * the constant lived in `space-tabs.tsx`, and the server HTML came back as + * `["$","div",null,{"id":"$56","className":"scroll-mt-14"}]` — an element with no id until + * hydration, so `/space/…/debates#space-tabs` had nothing to scroll to on a cold load. In the + * browser it looked perfect (GEO-2974). + * + * The sibling of this test guards the other direction — async components rendered from client + * files. Same failure mode: correct-looking UI, wrong boundary. + * + * What this cannot see: whether the value is ever *read* while rendering on the server. A client + * hook imported next to a server-safe constant and only ever called from a client component is + * inert. So the allowlist below is not a list of things that are fine — it is a list of things + * checked by hand, each with what was found. + */ + +const ROOT = path.resolve(__dirname, '..', '..'); +const SOURCE_DIRS = ['app', 'core', 'partials', 'design-system']; + +/** The files Next renders on the server by definition. Everything they reach is the server graph. */ +const SERVER_ENTRY = /\/(layout|page|template|default|loading|error|not-found|route|opengraph-image)\.tsx?$/; + +const NAMED_IMPORT_BLOCK = /^import\s+(?!type\s)(?:[A-Za-z_$][\w$]*\s*,\s*)?\{([^}]*)\}\s+from\s+['"]([^'"]+)['"]/gm; + +/** + * Pre-existing, each one read before being listed. None of them is this PR's, and all of them are + * the same latent shape — a client reference standing where a value is expected. + * + * - `bounty-board-skeleton` is the live one: `app/bounties/loading.tsx` is server-rendered and puts + * `BOARD_GRID_CLASS` straight into a `className`, exactly the bug above. Worth its own fix. + * - `read-block-media-dimensions` would return a client reference in place of its empty-dimensions + * object; nothing calls it outside its test today, so it is a landmine rather than a fault. + * - `entity-response` calls `getChecked` while deriving a response kind; server callers would throw + * rather than render something wrong. + * - `bounties/config` is inert: `useFeatureFlag` is only ever called from `useBountiesEnabled`, + * which is a client hook. The module is in the server graph for `bountiesEnabledForNetwork`. + */ +const KNOWN = new Set([ + 'partials/bounties/bounty-board-skeleton.tsx -> BOARD_CARD_HEIGHT_PX', + 'partials/bounties/bounty-board-skeleton.tsx -> BOARD_GRID_CLASS', + 'core/blocks/data/read-block-media-dimensions.ts -> NO_BLOCK_MEDIA_DIMENSIONS', + 'core/responses/entity-response.ts -> getChecked', + 'core/bounties/config.ts -> useFeatureFlag', +]); + +function sourceFiles(): string[] { + const found: string[] = []; + const walk = (dir: string) => { + for (const entry of readdirSync(path.join(ROOT, dir), { withFileTypes: true })) { + const rel = path.join(dir, entry.name); + if (entry.isDirectory()) { + if (entry.name !== 'node_modules') walk(rel); + } else if (/\.tsx?$/.test(entry.name) && !/\.test\.tsx?$/.test(entry.name)) { + found.push(rel); + } + } + }; + for (const dir of SOURCE_DIRS) walk(dir); + return found; +} + +function isClientFile(contents: string): boolean { + return /^\s*['"]use client['"]/.test(contents); +} + +function resolveImport(specifier: string, importingFile: string): string | null { + let absolute: string; + + if (specifier.startsWith('~/')) { + absolute = path.join(ROOT, specifier.slice(2)); + } else if (specifier.startsWith('.')) { + absolute = path.resolve(ROOT, path.dirname(importingFile), specifier); + } else { + return null; + } + + const relative = path.relative(ROOT, absolute); + for (const candidate of [`${relative}.tsx`, `${relative}.ts`, path.join(relative, 'index.tsx')]) { + if (!SOURCE_DIRS.some(dir => candidate.startsWith(`${dir}${path.sep}`))) continue; + if (existsSync(path.join(ROOT, candidate))) return candidate; + } + return null; +} + +/** `A` or `A as B` inside a named import block, skipping inline `type` specifiers. */ +function parseNamedBindings(block: string): string[] { + return block + .split(',') + .map(entry => entry.trim()) + .filter(entry => entry.length > 0 && !entry.startsWith('type ')) + .map(entry => entry.split(/\s+as\s+/)[0].trim()) + .filter(Boolean); +} + +/** + * Components are the one export a Server Component may take from a client module — that is what the + * boundary is for. PascalCase stands in for "component", with SCREAMING_CASE excluded, since + * `BOARD_GRID_CLASS` passes a naive capital-letter test while being a string. + */ +function looksLikeComponent(name: string): boolean { + return /^[A-Z]/.test(name) && name !== name.toUpperCase(); +} + +describe('server components take only components from client modules', () => { + const files = sourceFiles(); + const contentsByFile = new Map(files.map(file => [file, readFileSync(path.join(ROOT, file), 'utf8')])); + const clientFiles = new Set([...contentsByFile].filter(([, c]) => isClientFile(c)).map(([f]) => f)); + + it('walks a source tree that actually has files in it', () => { + // Guards against a silently empty run if the layout moves. + expect(files.length).toBeGreaterThan(50); + expect(clientFiles.size).toBeGreaterThan(50); + }); + + /** Every non-client module reachable from a server entry point, which is where this can bite. */ + const serverGraph = new Set(); + const queue = files.filter(file => SERVER_ENTRY.test(file.split(path.sep).join('/')) && !clientFiles.has(file)); + + // The seeds are the layouts, pages and loading states. If this is empty the walk above drifted. + expect(queue.length).toBeGreaterThan(20); + + while (queue.length > 0) { + const file = queue.pop()!; + if (serverGraph.has(file) || clientFiles.has(file)) continue; + serverGraph.add(file); + + for (const match of (contentsByFile.get(file) ?? '').matchAll(NAMED_IMPORT_BLOCK)) { + const target = resolveImport(match[2], file); + if (target && !clientFiles.has(target) && !serverGraph.has(target)) queue.push(target); + } + } + + it('reaches a server graph worth checking', () => { + expect(serverGraph.size).toBeGreaterThan(100); + }); + + it('finds no non-component value taken from a "use client" module', () => { + const offences: string[] = []; + + for (const file of serverGraph) { + for (const match of contentsByFile.get(file)!.matchAll(NAMED_IMPORT_BLOCK)) { + const [, block, specifier] = match; + const target = resolveImport(specifier, file); + if (!target || !clientFiles.has(target)) continue; + + for (const name of parseNamedBindings(block)) { + if (looksLikeComponent(name)) continue; + const offence = `${file.split(path.sep).join('/')} -> ${name}`; + if (!KNOWN.has(offence)) offences.push(`${offence} (from ${target.split(path.sep).join('/')})`); + } + } + } + + expect(offences).toEqual([]); + }); + + it('keeps the known list honest', () => { + // An entry that no longer matches anything has been fixed, and leaving it here would quietly + // re-permit the same import later. + const live = new Set(); + + for (const file of serverGraph) { + for (const match of contentsByFile.get(file)!.matchAll(NAMED_IMPORT_BLOCK)) { + const target = resolveImport(match[2], file); + if (!target || !clientFiles.has(target)) continue; + for (const name of parseNamedBindings(match[1])) { + if (!looksLikeComponent(name)) live.add(`${file.split(path.sep).join('/')} -> ${name}`); + } + } + } + + expect([...KNOWN].filter(entry => !live.has(entry))).toEqual([]); + }); +}); diff --git a/apps/web/partials/explore/debate-explore-feed-card.tsx b/apps/web/partials/explore/debate-explore-feed-card.tsx index 8684352fe0..19e834bdfd 100644 --- a/apps/web/partials/explore/debate-explore-feed-card.tsx +++ b/apps/web/partials/explore/debate-explore-feed-card.tsx @@ -7,7 +7,7 @@ import { DebateClaimsPanel } from '~/core/debates/browse/debate-claims-panel'; import { DebateFeedPlayer } from '~/core/debates/browse/debate-feed-player'; import { DebateShareDialog } from '~/core/debates/browse/share-dialog'; import { useDebateShareAction } from '~/core/debates/browse/use-debate-share-action'; -import { useDebatePlaybackAllowed } from '~/core/debates/debate-playback-gate'; +import { useDebatePlaybackAllowed, useIsDebatePlaybackGated } from '~/core/debates/debate-playback-gate'; import { useDebate, useDebateMedia } from '~/core/debates/hooks'; import { hasProcessedVideo, isWatchableDebate } from '~/core/debates/playback-utils'; import { useDebateTranscriptClaims } from '~/core/debates/use-debate-transcript-claims'; @@ -34,6 +34,19 @@ import { SpaceThumb } from './space-thumb'; const ACTIVATE_RATIO = 0.6; const DEACTIVATE_RATIO = 0.4; +/** + * The same pair where a gate has already picked the one card allowed to play. + * + * 0.6 exists to stop a stack of cards all playing at once, which is the gate's job wherever there + * is one. Kept that high under a gate it can only subtract: the profile's Activity row shares one + * vertical ratio across every card in it, so a row sitting half off the bottom of the screen puts + * the chosen card at 0.5 — inside the dead band, holding whatever it was, which after a tab switch + * is "not playing". Low enough to start on sight, with the hysteresis kept so a card resting near + * the edge does not toggle. Playback is muted, so starting early costs the reader nothing. + */ +const GATED_ACTIVATE_RATIO = 0.25; +const GATED_DEACTIVATE_RATIO = 0.1; + type DebateExploreFeedCardProps = { item: ExploreFeedItem; /** Hide the space thumbnail + space-name link in the meta row (same semantics as ExploreFeedCard). */ @@ -82,10 +95,15 @@ export function DebateExploreFeedCard({ // screen at once, and nothing else holds a card active. Each toggle starts or interrupts a // playback attempt, which is what made scrolling feel glitchy (GEO-2895). // - // Now: reach 0.6 to activate, fall back to 0.4 to give it up, and hold whatever the card - // already was strictly between them. The lower edge is inclusive so that the observer's - // report at the 0.4 threshold deactivates rather than landing ambiguously inside the band — - // a ratio reported exactly at a threshold is the normal case, not an edge case. + // Now: reach the activation ratio to activate, fall back to the deactivation one to give it up, + // and hold whatever the card already was strictly between them. The lower edge is inclusive so + // that the observer's report at that threshold deactivates rather than landing ambiguously + // inside the band — a ratio reported exactly at a threshold is the normal case, not an edge + // case. Which pair applies depends on whether a gate has already chosen one card; see above. + const gated = useIsDebatePlaybackGated(); + const activateRatio = gated ? GATED_ACTIVATE_RATIO : ACTIVATE_RATIO; + const deactivateRatio = gated ? GATED_DEACTIVATE_RATIO : DEACTIVATE_RATIO; + const [active, setActive] = React.useState(false); React.useEffect(() => { if (!container) return; @@ -94,17 +112,17 @@ export function DebateExploreFeedCard({ for (const entry of entries) { setActive(current => { if (!entry.isIntersecting) return false; - if (entry.intersectionRatio >= ACTIVATE_RATIO) return true; - if (entry.intersectionRatio <= DEACTIVATE_RATIO) return false; + if (entry.intersectionRatio >= activateRatio) return true; + if (entry.intersectionRatio <= deactivateRatio) return false; return current; }); } }, - { threshold: [DEACTIVATE_RATIO, ACTIVATE_RATIO] } + { threshold: [deactivateRatio, activateRatio] } ); observer.observe(container); return () => observer.disconnect(); - }, [container]); + }, [container, activateRatio, deactivateRatio]); // A veto, not a replacement: where a surface holds playback to one debate — // a row of cards, all of them fully on screen at once — this says whether it diff --git a/apps/web/partials/profile/profile-activity-section.test.tsx b/apps/web/partials/profile/profile-activity-section.test.tsx index 554b3792c9..a5ae0cb6bb 100644 --- a/apps/web/partials/profile/profile-activity-section.test.tsx +++ b/apps/web/partials/profile/profile-activity-section.test.tsx @@ -78,12 +78,14 @@ function mockMobileActivityGeometry(pageHeightWithoutActivity: number) { } vi.stubGlobal('ResizeObserver', TestResizeObserver); + const sectionHeight = { debates: 500, claims: 250 }; + const originalRect = HTMLElement.prototype.getBoundingClientRect; vi.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockImplementation(function (this: HTMLElement) { if ('activitySection' in this.dataset) { const debatesSelected = this.querySelector('button[aria-pressed="true"]')?.textContent?.includes('Debates'); - return rect(390, debatesSelected ? 500 : 250); + return rect(390, debatesSelected ? sectionHeight.debates : sectionHeight.claims); } if ('activityScrollReserve' in this.dataset) { @@ -116,8 +118,9 @@ function mockMobileActivityGeometry(pageHeightWithoutActivity: number) { // position, and the recovery path exists for exactly that case. scroll, // And for the claim cards growing as their queries land, which is the other way the sizing - // path runs after a switch. + // path runs after a switch. Move `sectionHeight` first; the observer reads it. sectionResized: () => notifyResize?.(), + sectionHeight, }; } @@ -347,6 +350,33 @@ describe('ProfileActivitySection', () => { expect(reserve).toHaveStyle({ height: '0px' }); }); + /** + * The swap is over once the page can hold the reader without help, and it has to actually end. + * Left armed, `holdY` outlives the switch it belongs to: a shrink long afterwards would size a + * reserve from a position the reader left, and hand them a screen of blank space to scroll into. + */ + it('stops holding once the page is tall enough on its own', () => { + const { sectionHeight, sectionResized } = mockMobileActivityGeometry(600); + const { reserve } = renderActivity(); + + fireEvent.click(screen.getByRole('button', { name: /Claims/ })); + expect(reserve).toHaveStyle({ height: '150px' }); + + // The claim cards finish loading, and the page is long enough on its own. + sectionHeight.claims = 500; + act(() => { + sectionResized(); + }); + expect(reserve).toHaveStyle({ height: '0px' }); + + // Whatever shrinks the section after that is not this switch's business. + sectionHeight.claims = 250; + act(() => { + sectionResized(); + }); + expect(reserve).toHaveStyle({ height: '0px' }); + }); + /** * The other caller of the sizing path. A claim card grows when its queries land, which resizes * the section — and resizing the reserve is the whole response to that. Moving the reader is not, diff --git a/apps/web/partials/profile/profile-activity-section.tsx b/apps/web/partials/profile/profile-activity-section.tsx index df1b6fdde8..ee108019ac 100644 --- a/apps/web/partials/profile/profile-activity-section.tsx +++ b/apps/web/partials/profile/profile-activity-section.tsx @@ -15,7 +15,7 @@ import { RightArrowLongSmall } from '~/design-system/icons/right-arrow-long-smal import { PrefetchLink as Link } from '~/design-system/prefetch-link'; import { ExploreFeedCard } from '~/partials/explore/explore-feed-card'; -import { withSpaceTabsAnchor } from '~/partials/space-page/space-tabs'; +import { withSpaceTabsAnchor } from '~/partials/space-page/space-tabs-anchor'; import { GalleryClaimCard } from './gallery-claim-card'; @@ -268,11 +268,30 @@ function useMobileActivityHeightReserve(selectedKey: string | undefined) { if (!swap || Math.abs(swap.width - width) > 1) { swapRef.current = null; reserve.style.height = '0px'; - return; + return 0; } const naturalDocumentHeight = swap.naturalDocumentHeight - swap.sectionHeight + height; - reserve.style.height = `${Math.max(0, swap.holdY + window.innerHeight - naturalDocumentHeight)}px`; + const held = Math.max(0, swap.holdY + window.innerHeight - naturalDocumentHeight); + reserve.style.height = `${held}px`; + + return held; + }; + + /** + * Size, and let the swap go once the page no longer needs it. + * + * A swap that outlives its own settling is state waiting to be wrong: the cards grow, the + * reserve reaches zero, and `holdY` sits there for as long as the reader stays on this tab — + * so a shrink an hour later would conjure height back out of a position they left behind, and + * leave them scrolling into blank space. + * + * Zero is the safe moment to let go precisely because nothing is being held at it, so dropping + * the swap cannot move anybody. Not on the first sizing though — that one runs before the + * correction below, and the correction needs the swap it belongs to. + */ + const sizeAndSettle = () => { + if (sizeReserve() === 0) swapRef.current = null; }; sizeReserve(); @@ -289,7 +308,7 @@ function useMobileActivityHeightReserve(selectedKey: string | undefined) { if (typeof ResizeObserver === 'undefined') return; // Claim cards grow as their queries land. Shrink the reserve by the same amount so the overall // document height stays steady rather than drifting. - const observer = new ResizeObserver(sizeReserve); + const observer = new ResizeObserver(sizeAndSettle); observer.observe(section); // Armed a frame late, so the scroll events belonging to the swap itself — the correction above, @@ -305,11 +324,12 @@ function useMobileActivityHeightReserve(selectedKey: string | undefined) { const current = swapRef.current; if (!current) return; - // Once the reader moves up of their own accord, stop holding space they no longer need. - // Moving down needs nothing held and nothing released. + // Once the reader moves up of their own accord, stop holding space they no longer need — + // and back at the top there is nothing left to hold, so the swap goes with it. Moving down + // needs nothing held and nothing released. if (window.scrollY < current.holdY) { current.holdY = window.scrollY; - sizeReserve(); + sizeAndSettle(); } }; window.addEventListener('scroll', onScroll, { passive: true }); diff --git a/apps/web/partials/space-page/space-tabs-anchor.ts b/apps/web/partials/space-page/space-tabs-anchor.ts new file mode 100644 index 0000000000..34a33edda9 --- /dev/null +++ b/apps/web/partials/space-page/space-tabs-anchor.ts @@ -0,0 +1,27 @@ +/** + * The tab bar's link target. + * + * Its own module, with no `'use client'`, because the space layout is a Server Component and reads + * {@link SPACE_TABS_ANCHOR} to write a DOM `id`. Every export of a client module is a client + * reference on the server, so when this lived in `space-tabs.tsx` the layout rendered + * `{"id":"$56"}` into the flight payload instead of the string — the id only existed once hydration + * resolved the reference, which is no use to a browser looking for a fragment on a cold load. + * + * The id and the href that points at it belong together: they are one contract with two halves, and + * the point of naming them once is that the two cannot drift. + */ + +/** The id on the element wrapping the tab bar — see the space layout. */ +export const SPACE_TABS_ANCHOR = 'space-tabs'; + +/** + * `href` with the fragment that lands the reader on the tab bar rather than the page top. + * + * A fragment rather than a scroll written by hand, because of when each one runs. The router + * applies a fragment once the destination has rendered, which is the first moment the page is tall + * enough to hold the position; scrolling on click instead runs against the page being left, and a + * profile whose Overview is barely a screen tall has nowhere to put the reader, so they land short. + */ +export function withSpaceTabsAnchor(href: string) { + return `${href}#${SPACE_TABS_ANCHOR}`; +} diff --git a/apps/web/partials/space-page/space-tabs.tsx b/apps/web/partials/space-page/space-tabs.tsx index 39adda184e..49cee6f077 100644 --- a/apps/web/partials/space-page/space-tabs.tsx +++ b/apps/web/partials/space-page/space-tabs.tsx @@ -41,27 +41,6 @@ type BuiltSpaceTab = { /** The record routes on a profile. Reachable only by their own tab — see the dedupe below. */ const PERSON_TAB_LABELS = ['Debates', 'Positions', 'Proposals', 'About'] as const; -/** - * The tab bar's own id, so a link can land on the tabs instead of the top of the page. - * - * A fragment rather than a scroll written by hand, because of when each one runs. The router - * applies a fragment once the destination has rendered, which is the first moment the page is tall - * enough to hold the position; scrolling on click instead runs against the page being left, and a - * profile whose Overview is barely a screen tall has nowhere to put the reader, so they land short. - * - * It does not survive a cold load of the link — the lists render on the client, so the document is - * still one screen tall when the browser looks for the fragment. That leaves the reader at the top - * of the profile, which is where a cold load leaves them anyway. - * - * Whoever renders the bar owns the id — see the space layout. - */ -export const SPACE_TABS_ANCHOR = 'space-tabs'; - -/** `href` with the fragment that lands the reader on the tab bar rather than the page top. */ -export function withSpaceTabsAnchor(href: string) { - return `${href}#${SPACE_TABS_ANCHOR}`; -} - /** * The About tab, defined once for both paths that draw it. * From cb31e9b607b68507ccd35bf91be44ae90eab5347 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Sat, 19 Sep 2026 18:05:45 -0700 Subject: [PATCH 13/19] revert(debates): leave the gated autoplay thresholds alone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Backed out of ee2742637. The reason I changed them was that an earlier revision of the PR description said they had changed, which is a reason to fix the description. There is no observed fault behind it. The autoplay report was never reproduced, this surface has been tested on the preview through several rounds at 0.6/0.4 with nobody reporting a card that would not start, and the dead band the change was aimed at needs the Activity row to be straddling the bottom of the screen — where the reader is looking at something else and a paused video is arguably right. 0.25 would also start a card when a quarter of it is showing, which is closer to the complaint in the ticket than away from it. `useIsDebatePlaybackGated` goes with it: nothing else asks the question. --- .../debates/debate-playback-gate.test.tsx | 36 +------------------ .../web/core/debates/debate-playback-gate.tsx | 13 ------- .../explore/debate-explore-feed-card.tsx | 36 +++++-------------- 3 files changed, 10 insertions(+), 75 deletions(-) diff --git a/apps/web/core/debates/debate-playback-gate.test.tsx b/apps/web/core/debates/debate-playback-gate.test.tsx index 616d914bd3..82260c0afd 100644 --- a/apps/web/core/debates/debate-playback-gate.test.tsx +++ b/apps/web/core/debates/debate-playback-gate.test.tsx @@ -3,16 +3,12 @@ import { cleanup, render, screen } from '@testing-library/react'; import { afterEach, describe, expect, it } from 'vitest'; -import { DebatePlaybackGate, useDebatePlaybackAllowed, useIsDebatePlaybackGated } from './debate-playback-gate'; +import { DebatePlaybackGate, useDebatePlaybackAllowed } from './debate-playback-gate'; function Probe({ id }: { id: string }) { return {useDebatePlaybackAllowed(id) ? 'allowed' : 'held'}; } -function GatedProbe() { - return {useIsDebatePlaybackGated() ? 'gated' : 'ungated'}; -} - const A = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'; const B = 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb'; @@ -55,36 +51,6 @@ describe('DebatePlaybackGate', () => { expect(screen.getByTestId(A)).toHaveTextContent('held'); }); - /** - * A card asks this to decide how much of itself has to be on screen before it plays. Under a - * gate the answer is "any of it" — the gate has already chosen one card, so the card's own - * stricter ratio can only keep the chosen one silent. - */ - it('tells a card whether a gate is arbitrating', () => { - render(); - expect(screen.getByTestId('gated')).toHaveTextContent('ungated'); - - cleanup(); - - render( - - - - ); - expect(screen.getByTestId('gated')).toHaveTextContent('gated'); - }); - - it('is gated even while the gallery has chosen nobody', () => { - // `allowedId={null}` is a gate holding everything, not the absence of one. - render( - - - - ); - - expect(screen.getByTestId('gated')).toHaveTextContent('gated'); - }); - it('matches ids however they are spelled', () => { // Ids reach this from two directions — a relation and a card — and one of // them may carry dashes. Comparing them raw would hold the very card the diff --git a/apps/web/core/debates/debate-playback-gate.tsx b/apps/web/core/debates/debate-playback-gate.tsx index 87b87e3832..a3069d758a 100644 --- a/apps/web/core/debates/debate-playback-gate.tsx +++ b/apps/web/core/debates/debate-playback-gate.tsx @@ -30,19 +30,6 @@ export function DebatePlaybackGate({ allowedId, children }: { allowedId: string return {children}; } -/** - * Whether a surface is holding playback to one debate at all. - * - * A card decides for itself whether enough of it is on screen to play, and that judgement is - * calibrated for a stack of cards where nothing else arbitrates. Under a gate it is a second, - * stricter arbiter that can only subtract — so a card the gate has chosen can sit in its own dead - * band and never start, with tapping the reader's only way out. Knowing a gate is in force lets a - * card ask the question it actually needs answered: is any of me on screen. - */ -export function useIsDebatePlaybackGated(): boolean { - return React.useContext(DebatePlaybackContext) !== undefined; -} - /** Whether this debate may play. True wherever no gate is in force. */ export function useDebatePlaybackAllowed(debateId: string): boolean { const allowed = React.useContext(DebatePlaybackContext); diff --git a/apps/web/partials/explore/debate-explore-feed-card.tsx b/apps/web/partials/explore/debate-explore-feed-card.tsx index 19e834bdfd..8684352fe0 100644 --- a/apps/web/partials/explore/debate-explore-feed-card.tsx +++ b/apps/web/partials/explore/debate-explore-feed-card.tsx @@ -7,7 +7,7 @@ import { DebateClaimsPanel } from '~/core/debates/browse/debate-claims-panel'; import { DebateFeedPlayer } from '~/core/debates/browse/debate-feed-player'; import { DebateShareDialog } from '~/core/debates/browse/share-dialog'; import { useDebateShareAction } from '~/core/debates/browse/use-debate-share-action'; -import { useDebatePlaybackAllowed, useIsDebatePlaybackGated } from '~/core/debates/debate-playback-gate'; +import { useDebatePlaybackAllowed } from '~/core/debates/debate-playback-gate'; import { useDebate, useDebateMedia } from '~/core/debates/hooks'; import { hasProcessedVideo, isWatchableDebate } from '~/core/debates/playback-utils'; import { useDebateTranscriptClaims } from '~/core/debates/use-debate-transcript-claims'; @@ -34,19 +34,6 @@ import { SpaceThumb } from './space-thumb'; const ACTIVATE_RATIO = 0.6; const DEACTIVATE_RATIO = 0.4; -/** - * The same pair where a gate has already picked the one card allowed to play. - * - * 0.6 exists to stop a stack of cards all playing at once, which is the gate's job wherever there - * is one. Kept that high under a gate it can only subtract: the profile's Activity row shares one - * vertical ratio across every card in it, so a row sitting half off the bottom of the screen puts - * the chosen card at 0.5 — inside the dead band, holding whatever it was, which after a tab switch - * is "not playing". Low enough to start on sight, with the hysteresis kept so a card resting near - * the edge does not toggle. Playback is muted, so starting early costs the reader nothing. - */ -const GATED_ACTIVATE_RATIO = 0.25; -const GATED_DEACTIVATE_RATIO = 0.1; - type DebateExploreFeedCardProps = { item: ExploreFeedItem; /** Hide the space thumbnail + space-name link in the meta row (same semantics as ExploreFeedCard). */ @@ -95,15 +82,10 @@ export function DebateExploreFeedCard({ // screen at once, and nothing else holds a card active. Each toggle starts or interrupts a // playback attempt, which is what made scrolling feel glitchy (GEO-2895). // - // Now: reach the activation ratio to activate, fall back to the deactivation one to give it up, - // and hold whatever the card already was strictly between them. The lower edge is inclusive so - // that the observer's report at that threshold deactivates rather than landing ambiguously - // inside the band — a ratio reported exactly at a threshold is the normal case, not an edge - // case. Which pair applies depends on whether a gate has already chosen one card; see above. - const gated = useIsDebatePlaybackGated(); - const activateRatio = gated ? GATED_ACTIVATE_RATIO : ACTIVATE_RATIO; - const deactivateRatio = gated ? GATED_DEACTIVATE_RATIO : DEACTIVATE_RATIO; - + // Now: reach 0.6 to activate, fall back to 0.4 to give it up, and hold whatever the card + // already was strictly between them. The lower edge is inclusive so that the observer's + // report at the 0.4 threshold deactivates rather than landing ambiguously inside the band — + // a ratio reported exactly at a threshold is the normal case, not an edge case. const [active, setActive] = React.useState(false); React.useEffect(() => { if (!container) return; @@ -112,17 +94,17 @@ export function DebateExploreFeedCard({ for (const entry of entries) { setActive(current => { if (!entry.isIntersecting) return false; - if (entry.intersectionRatio >= activateRatio) return true; - if (entry.intersectionRatio <= deactivateRatio) return false; + if (entry.intersectionRatio >= ACTIVATE_RATIO) return true; + if (entry.intersectionRatio <= DEACTIVATE_RATIO) return false; return current; }); } }, - { threshold: [deactivateRatio, activateRatio] } + { threshold: [DEACTIVATE_RATIO, ACTIVATE_RATIO] } ); observer.observe(container); return () => observer.disconnect(); - }, [container, activateRatio, deactivateRatio]); + }, [container]); // A veto, not a replacement: where a surface holds playback to one debate — // a row of cards, all of them fully on screen at once — this says whether it From c8e6e6c5e0b1d3059534f85a6b756deb72a3ec64 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Sat, 19 Sep 2026 18:22:06 -0700 Subject: [PATCH 14/19] docs(profile): say what the anchor fix did and did not fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verified on the deployed build: the server now renders id="space-tabs" rather than the client reference. A cold load of the anchored link still lands at the top, and not for that reason — the page streams, so half a second in the element is not in the document when the browser goes looking for the fragment. --- apps/web/partials/space-page/space-tabs-anchor.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/apps/web/partials/space-page/space-tabs-anchor.ts b/apps/web/partials/space-page/space-tabs-anchor.ts index 34a33edda9..a5858d170c 100644 --- a/apps/web/partials/space-page/space-tabs-anchor.ts +++ b/apps/web/partials/space-page/space-tabs-anchor.ts @@ -4,8 +4,13 @@ * Its own module, with no `'use client'`, because the space layout is a Server Component and reads * {@link SPACE_TABS_ANCHOR} to write a DOM `id`. Every export of a client module is a client * reference on the server, so when this lived in `space-tabs.tsx` the layout rendered - * `{"id":"$56"}` into the flight payload instead of the string — the id only existed once hydration - * resolved the reference, which is no use to a browser looking for a fragment on a cold load. + * `{"id":"$56"}` into the flight payload instead of the string, and the id only existed once + * hydration resolved the reference. + * + * That is fixed, and a cold load still lands at the top of the page — the two are separate. The + * page streams: half a second in, the document is one empty viewport and this element is not in it + * yet, so the browser looks for the fragment, finds nothing, and does not look again. The fragment + * is for the click, which the router applies after the destination has rendered. * * The id and the href that points at it belong together: they are one contract with two halves, and * the point of naming them once is that the two cannot drift. From f0f4749ff39c26e92aa77a709ae7a97852026696 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Sat, 19 Sep 2026 18:45:48 -0700 Subject: [PATCH 15/19] fix(profile): watch every input the height reserve depends on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reserve is a sum of four things and only two of them were being watched. `window.innerHeight` is read live but nothing recomputed when it changed, and on a phone it changes on its own: the browser chrome collapses as the reader scrolls and returns when they stop. A taller viewport needs more height held below it, so the reader could be clamped upward by exactly the height of a hidden URL bar. A `resize` listener closes it — `resize` rather than `visualViewport`, because `innerHeight` is the figure the sum uses and the two do not always agree. The document height was worse: measured once at the switch and adjusted by the section's own delta, so anything else on the page moving afterwards — a cover image landing above Activity, the rail settling — left it wrong. It is measured when it is needed now, minus whatever the reserve is currently contributing, which is the same arithmetic without the memory. Two fields leave the swap with it. The boundary guard walked named imports only, so a quarter of the server graph was unguarded — `default-entity-page` and `post-entity-page` among them, reached by default import. It follows default, namespace and re-export edges now, and reports offences in all of those shapes. Type-only edges are excluded, which is load-bearing: the sole route into `core/blocks/data/filters.ts` is an `import type`, and counting it walks into the sync store and reports three modules TypeScript erases before anything runs. --- ...client-values-in-server-boundaries.test.ts | 101 ++++++++++++++---- .../profile/profile-activity-section.test.tsx | 57 +++++++++- .../profile/profile-activity-section.tsx | 28 +++-- 3 files changed, 150 insertions(+), 36 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index d3d076c75b..e416430923 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -31,8 +31,31 @@ const SOURCE_DIRS = ['app', 'core', 'partials', 'design-system']; /** The files Next renders on the server by definition. Everything they reach is the server graph. */ const SERVER_ENTRY = /\/(layout|page|template|default|loading|error|not-found|route|opengraph-image)\.tsx?$/; +/** + * Every way one module reaches another *at runtime*, because a traversal that follows only one of + * them walks a smaller graph than the server actually renders and quietly stops guarding the rest + * of it. The tree uses all of these: ~9800 named imports, ~340 default, ~870 namespace, 53 + * re-exports. + * + * `import type` is excluded, and it matters: the only route into `core/blocks/data/filters.ts` is a + * type import from `core/chat/edit-types.ts`, so counting it walks into the sync store and reports + * three modules that TypeScript erases before anything runs. + */ +const MODULE_EDGE = /^(?:import|export)\s+(?!type\s)[\s\S]*?from\s+['"]([^'"]+)['"]/gm; + +/** `import { a, b as c } from '…'`, with or without a default binding in front. */ const NAMED_IMPORT_BLOCK = /^import\s+(?!type\s)(?:[A-Za-z_$][\w$]*\s*,\s*)?\{([^}]*)\}\s+from\s+['"]([^'"]+)['"]/gm; +/** `import Local from '…'`, ignoring the `import type` and `import * as` forms. */ +const DEFAULT_IMPORT = /^import\s+(?!type\s)([A-Za-z_$][\w$]*)\s*(?:,\s*\{[^}]*\})?\s+from\s+['"]([^'"]+)['"]/gm; + +/** `import * as Local from '…'`, where every property read is a client reference. */ +const NAMESPACE_IMPORT = /^import\s+\*\s+as\s+([A-Za-z_$][\w$]*)\s+from\s+['"]([^'"]+)['"]/gm; + +/** `export { a } from '…'` and `export * from '…'`, which hand a client reference straight on. */ +const NAMED_REEXPORT = /^export\s+(?!type\s)\{([^}]*)\}\s+from\s+['"]([^'"]+)['"]/gm; +const STAR_REEXPORT = /^export\s+\*\s+from\s+['"]([^'"]+)['"]/gm; + /** * Pre-existing, each one read before being listed. None of them is this PR's, and all of them are * the same latent shape — a client reference standing where a value is expected. @@ -86,7 +109,12 @@ function resolveImport(specifier: string, importingFile: string): string | null } const relative = path.relative(ROOT, absolute); - for (const candidate of [`${relative}.tsx`, `${relative}.ts`, path.join(relative, 'index.tsx')]) { + for (const candidate of [ + `${relative}.tsx`, + `${relative}.ts`, + path.join(relative, 'index.tsx'), + path.join(relative, 'index.ts'), + ]) { if (!SOURCE_DIRS.some(dir => candidate.startsWith(`${dir}${path.sep}`))) continue; if (existsSync(path.join(ROOT, candidate))) return candidate; } @@ -135,8 +163,8 @@ describe('server components take only components from client modules', () => { if (serverGraph.has(file) || clientFiles.has(file)) continue; serverGraph.add(file); - for (const match of (contentsByFile.get(file) ?? '').matchAll(NAMED_IMPORT_BLOCK)) { - const target = resolveImport(match[2], file); + for (const match of (contentsByFile.get(file) ?? '').matchAll(MODULE_EDGE)) { + const target = resolveImport(match[1], file); if (target && !clientFiles.has(target) && !serverGraph.has(target)) queue.push(target); } } @@ -145,22 +173,59 @@ describe('server components take only components from client modules', () => { expect(serverGraph.size).toBeGreaterThan(100); }); - it('finds no non-component value taken from a "use client" module', () => { - const offences: string[] = []; - + /** + * Every value a server-graph module takes from a client module, in any of the shapes it can + * arrive in, as `[offence, source]` pairs. A default binding is judged by its local name — the + * only signal a default import carries — and a namespace binding is reported whole, since every + * property read off it is a client reference. + */ + function* clientValuesInServerGraph(): Generator<[string, string]> { for (const file of serverGraph) { - for (const match of contentsByFile.get(file)!.matchAll(NAMED_IMPORT_BLOCK)) { - const [, block, specifier] = match; + const contents = contentsByFile.get(file)!; + const from = file.split(path.sep).join('/'); + + const fromClientModule = (specifier: string) => { const target = resolveImport(specifier, file); - if (!target || !clientFiles.has(target)) continue; + return target && clientFiles.has(target) ? target.split(path.sep).join('/') : null; + }; + + for (const [, block, specifier] of contents.matchAll(NAMED_IMPORT_BLOCK)) { + const target = fromClientModule(specifier); + if (!target) continue; + for (const name of parseNamedBindings(block)) { + if (!looksLikeComponent(name)) yield [`${from} -> ${name}`, target]; + } + } + for (const [, local, specifier] of contents.matchAll(DEFAULT_IMPORT)) { + const target = fromClientModule(specifier); + if (target && !looksLikeComponent(local)) yield [`${from} -> default as ${local}`, target]; + } + + for (const [, local, specifier] of contents.matchAll(NAMESPACE_IMPORT)) { + const target = fromClientModule(specifier); + if (target) yield [`${from} -> * as ${local}`, target]; + } + + for (const [, block, specifier] of contents.matchAll(NAMED_REEXPORT)) { + const target = fromClientModule(specifier); + if (!target) continue; for (const name of parseNamedBindings(block)) { - if (looksLikeComponent(name)) continue; - const offence = `${file.split(path.sep).join('/')} -> ${name}`; - if (!KNOWN.has(offence)) offences.push(`${offence} (from ${target.split(path.sep).join('/')})`); + if (!looksLikeComponent(name)) yield [`${from} -> re-exports ${name}`, target]; } } + + for (const [, specifier] of contents.matchAll(STAR_REEXPORT)) { + const target = fromClientModule(specifier); + if (target) yield [`${from} -> re-exports *`, target]; + } } + } + + it('finds no non-component value taken from a "use client" module', () => { + const offences = [...clientValuesInServerGraph()] + .filter(([offence]) => !KNOWN.has(offence)) + .map(([offence, target]) => `${offence} (from ${target})`); expect(offences).toEqual([]); }); @@ -168,17 +233,7 @@ describe('server components take only components from client modules', () => { it('keeps the known list honest', () => { // An entry that no longer matches anything has been fixed, and leaving it here would quietly // re-permit the same import later. - const live = new Set(); - - for (const file of serverGraph) { - for (const match of contentsByFile.get(file)!.matchAll(NAMED_IMPORT_BLOCK)) { - const target = resolveImport(match[2], file); - if (!target || !clientFiles.has(target)) continue; - for (const name of parseNamedBindings(match[1])) { - if (!looksLikeComponent(name)) live.add(`${file.split(path.sep).join('/')} -> ${name}`); - } - } - } + const live = new Set([...clientValuesInServerGraph()].map(([offence]) => offence)); expect([...KNOWN].filter(entry => !live.has(entry))).toEqual([]); }); diff --git a/apps/web/partials/profile/profile-activity-section.test.tsx b/apps/web/partials/profile/profile-activity-section.test.tsx index a5ae0cb6bb..4eccaaf3d0 100644 --- a/apps/web/partials/profile/profile-activity-section.test.tsx +++ b/apps/web/partials/profile/profile-activity-section.test.tsx @@ -102,12 +102,15 @@ function mockMobileActivityGeometry(pageHeightWithoutActivity: number) { if (scroll.moveAfterFirstRead !== undefined && reads > 1) return scroll.moveAfterFirstRead; return scroll.y; }); - vi.spyOn(window, 'innerHeight', 'get').mockReturnValue(600); + const viewport = { height: 600 }; + vi.spyOn(window, 'innerHeight', 'get').mockImplementation(() => viewport.height); + + const page = { withoutActivity: pageHeightWithoutActivity }; vi.spyOn(document.documentElement, 'scrollHeight', 'get').mockImplementation(() => { const section = document.querySelector('[data-activity-section]'); const reserve = document.querySelector('[data-activity-scroll-reserve]'); return ( - pageHeightWithoutActivity + + page.withoutActivity + (section?.getBoundingClientRect().height ?? 0) + (reserve?.getBoundingClientRect().height ?? 0) ); @@ -121,6 +124,10 @@ function mockMobileActivityGeometry(pageHeightWithoutActivity: number) { // path runs after a switch. Move `sectionHeight` first; the observer reads it. sectionResized: () => notifyResize?.(), sectionHeight, + // The viewport, which a phone changes on its own as its chrome collapses, and the rest of the + // page, which goes on loading after the switch. + viewport, + page, }; } @@ -350,6 +357,52 @@ describe('ProfileActivitySection', () => { expect(reserve).toHaveStyle({ height: '0px' }); }); + /** + * A phone changes its own viewport height: the browser chrome collapses as the reader scrolls and + * comes back when they stop, with nothing on the page moving. The held height is measured against + * that viewport, so a taller one needs more below it — and before this was watched, the reader + * could be clamped upward by exactly the height of a hidden URL bar. + */ + it('re-sizes the reserve when the viewport height changes', () => { + const { viewport } = mockMobileActivityGeometry(600); + const { reserve } = renderActivity(); + + fireEvent.click(screen.getByRole('button', { name: /Claims/ })); + expect(reserve).toHaveStyle({ height: '150px' }); + + // 600 → 700 of viewport. Held at 400, the page now has to reach 1100 rather than 1000, against + // a natural 850. + viewport.height = 700; + fireEvent.resize(window); + + expect(reserve).toHaveStyle({ height: '250px' }); + }); + + /** + * The section is not the only thing on the page that moves after a switch. A cover image landing + * above Activity changes the document height without changing the section at all, so a natural + * height remembered from the switch is wrong — and wrong in the direction that leaves the reader + * scrolling into space the page no longer needs. + */ + it('measures the rest of the page rather than remembering it', () => { + const { page, sectionHeight, sectionResized } = mockMobileActivityGeometry(600); + const { reserve } = renderActivity(); + + fireEvent.click(screen.getByRole('button', { name: /Claims/ })); + expect(reserve).toHaveStyle({ height: '150px' }); + + // The cards grow, and the cover lands above them in the same frame. + page.withoutActivity = 700; + sectionHeight.claims = 300; + act(() => { + sectionResized(); + }); + + // 700 + 300 reaches the reader's 1000 exactly. Carried from the switch instead, the page above + // is still believed to be 600 and 100px would be held for no one. + expect(reserve).toHaveStyle({ height: '0px' }); + }); + /** * The swap is over once the page can hold the reader without help, and it has to actually end. * Left armed, `holdY` outlives the switch it belongs to: a shrink long afterwards would size a diff --git a/apps/web/partials/profile/profile-activity-section.tsx b/apps/web/partials/profile/profile-activity-section.tsx index ee108019ac..aa3efd5fd8 100644 --- a/apps/web/partials/profile/profile-activity-section.tsx +++ b/apps/web/partials/profile/profile-activity-section.tsx @@ -223,9 +223,8 @@ function useMobileActivityHeightReserve(selectedKey: string | undefined) { const sectionRef = React.useRef(null); const reserveRef = React.useRef(null); const swapRef = React.useRef<{ + /** The layout this was calculated for. A different one invalidates it — see `sizeReserve`. */ width: number; - sectionHeight: number; - naturalDocumentHeight: number; /** The position being held for the reader, which only ever moves up. */ holdY: number; } | null>(null); @@ -235,13 +234,7 @@ function useMobileActivityHeightReserve(selectedKey: string | undefined) { if (!section || !reserve) return; const { width, height } = section.getBoundingClientRect(); - const currentReserve = reserve.getBoundingClientRect().height; - swapRef.current = { - width, - sectionHeight: height, - naturalDocumentHeight: document.documentElement.scrollHeight - currentReserve, - holdY: window.scrollY, - }; + swapRef.current = { width, holdY: window.scrollY }; // Hold the outgoing view's whole height before React replaces it. Waiting for the layout effect // would leave a window where the document is short and the position is already gone; over- @@ -259,7 +252,7 @@ function useMobileActivityHeightReserve(selectedKey: string | undefined) { // both sizes the reserve and corrects the position will correct it every time they scroll — // which reads as the page refusing to move (GEO-2974). const sizeReserve = () => { - const { width, height } = section.getBoundingClientRect(); + const { width } = section.getBoundingClientRect(); const swap = swapRef.current; // A new layout width (rotation, resized side panel, breakpoint change) has different card @@ -271,7 +264,11 @@ function useMobileActivityHeightReserve(selectedKey: string | undefined) { return 0; } - const naturalDocumentHeight = swap.naturalDocumentHeight - swap.sectionHeight + height; + // Measured now rather than carried from the switch. The page keeps moving afterwards — the + // cards grow as their queries land, a cover image arrives above, the rail settles — and a + // figure taken once is wrong for every one of those. Subtracting what the reserve is + // currently contributing is what makes this the height the page would have without it. + const naturalDocumentHeight = document.documentElement.scrollHeight - reserve.getBoundingClientRect().height; const held = Math.max(0, swap.holdY + window.innerHeight - naturalDocumentHeight); reserve.style.height = `${held}px`; @@ -334,10 +331,19 @@ function useMobileActivityHeightReserve(selectedKey: string | undefined) { }; window.addEventListener('scroll', onScroll, { passive: true }); + // `held` is measured against the viewport, and on a phone the viewport changes without anything + // else on the page moving: the browser chrome collapses as the reader scrolls and comes back + // when they stop. A taller viewport needs more held below it, and nothing here was watching — + // so the reader could be clamped upward by exactly the height of a hidden URL bar. `resize` and + // not `visualViewport`, because `window.innerHeight` is the figure the sum above uses and the + // two do not always agree. + window.addEventListener('resize', sizeAndSettle); + return () => { cancelAnimationFrame(arm); observer.disconnect(); window.removeEventListener('scroll', onScroll); + window.removeEventListener('resize', sizeAndSettle); }; }, [selectedKey]); From 1f8c05f179e9a006725a6cbc1f0ec4c42d488478 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Sat, 19 Sep 2026 18:46:36 -0700 Subject: [PATCH 16/19] docs(profile): name the browser the viewport fix is for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit iOS Safari moves `innerHeight` when its URL bar collapses, which is the device this was reported from. Chrome on Android pins the layout viewport to its largest size, so nothing moves there — worth saying, or testing it on Android reads as the fix not working. --- .../partials/profile/profile-activity-section.tsx | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/apps/web/partials/profile/profile-activity-section.tsx b/apps/web/partials/profile/profile-activity-section.tsx index aa3efd5fd8..38be7c0c93 100644 --- a/apps/web/partials/profile/profile-activity-section.tsx +++ b/apps/web/partials/profile/profile-activity-section.tsx @@ -332,11 +332,15 @@ function useMobileActivityHeightReserve(selectedKey: string | undefined) { window.addEventListener('scroll', onScroll, { passive: true }); // `held` is measured against the viewport, and on a phone the viewport changes without anything - // else on the page moving: the browser chrome collapses as the reader scrolls and comes back - // when they stop. A taller viewport needs more held below it, and nothing here was watching — - // so the reader could be clamped upward by exactly the height of a hidden URL bar. `resize` and - // not `visualViewport`, because `window.innerHeight` is the figure the sum above uses and the - // two do not always agree. + // else on the page moving: iOS Safari grows `innerHeight` when its URL bar collapses under a + // scroll and shrinks it back when the reader stops, firing `resize` both ways. A taller + // viewport needs more held below it, and nothing here was watching — so the reader could be + // clamped upward by exactly the height of a hidden URL bar. That is the device this was + // reported from; Chrome on Android pins its layout viewport to the largest size instead, so + // `innerHeight` never moves there and there is nothing to react to. + // + // `resize` rather than `visualViewport`, because `window.innerHeight` is the figure the sum + // above uses and the two do not always agree. window.addEventListener('resize', sizeAndSettle); return () => { From f8140170ad3e0a02cff976b3af705408af710cc3 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Sat, 19 Sep 2026 19:01:35 -0700 Subject: [PATCH 17/19] fix(profile): a viewport that shrinks and grows back must not end the swap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Settling on zero is right for changes that do not come back. Cards growing and the reader scrolling up both leave the page needing less than it did and go on needing less. A viewport is not like that: on iOS the URL bar returning takes height away and hiding it gives the height back, so settling on the shrink retired the swap during the half of the cycle where nothing was needed, and left nothing to rebuild the reserve on the half where it was. Resizes size now; they do not settle. The other three callers are unchanged, each of them monotone. The boundary guard missed `export * as Name from`, which is the common form here — 11 files against 4 for the bare `export *` it did match — so a server barrel could re-export a whole client namespace unnoticed. Planted one to prove it, and it goes unreported before this and is caught after. `default as Local` inside a named block is judged by the local name too, since `default` says nothing. `import()` and bare imports still are not followed. Neither reaches a source module from the server graph today, which is now checked rather than assumed, and the note says so. --- ...client-values-in-server-boundaries.test.ts | 31 ++++++++++++++++--- .../profile/profile-activity-section.test.tsx | 25 +++++++++++++++ .../profile/profile-activity-section.tsx | 13 ++++++-- 3 files changed, 61 insertions(+), 8 deletions(-) diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts index e416430923..bcfadb8189 100644 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts @@ -23,6 +23,11 @@ import { describe, expect, it } from 'vitest'; * hook imported next to a server-safe constant and only ever called from a client component is * inert. So the allowlist below is not a list of things that are fine — it is a list of things * checked by hand, each with what was found. + * + * Nor does it follow `import()` or a bare `import './x'`. Neither reaches a source module from the + * server graph today — checked, not assumed: the 85 files calling `import()` are client modules + * reaching for `next/dynamic`, and every bare import in the graph resolves to CSS or a package. + * Worth adding the day either stops being true. */ const ROOT = path.resolve(__dirname, '..', '..'); @@ -54,7 +59,14 @@ const NAMESPACE_IMPORT = /^import\s+\*\s+as\s+([A-Za-z_$][\w$]*)\s+from\s+['"]([ /** `export { a } from '…'` and `export * from '…'`, which hand a client reference straight on. */ const NAMED_REEXPORT = /^export\s+(?!type\s)\{([^}]*)\}\s+from\s+['"]([^'"]+)['"]/gm; -const STAR_REEXPORT = /^export\s+\*\s+from\s+['"]([^'"]+)['"]/gm; + +/** + * `export * from '…'` and `export * as Name from '…'`. + * + * The named form is the common one here — 11 files against 4 — so a matcher that only knew the + * bare `export *` was blind to most of the barrels in the tree. + */ +const STAR_REEXPORT = /^export\s+\*\s+(?:as\s+([A-Za-z_$][\w$]*)\s+)?from\s+['"]([^'"]+)['"]/gm; /** * Pre-existing, each one read before being listed. None of them is this PR's, and all of them are @@ -121,13 +133,22 @@ function resolveImport(specifier: string, importingFile: string): string | null return null; } -/** `A` or `A as B` inside a named import block, skipping inline `type` specifiers. */ +/** + * `A` or `A as B` inside a named import block, skipping inline `type` specifiers. + * + * The exported name is what identifies the export, so `foo as bar` is judged as `foo`. `default as + * Bar` is the exception: `default` says nothing about what it is, so the local name is the only + * signal there — the same reasoning as a default import. + */ function parseNamedBindings(block: string): string[] { return block .split(',') .map(entry => entry.trim()) .filter(entry => entry.length > 0 && !entry.startsWith('type ')) - .map(entry => entry.split(/\s+as\s+/)[0].trim()) + .map(entry => { + const [exported, local] = entry.split(/\s+as\s+/).map(part => part.trim()); + return exported === 'default' && local ? local : exported; + }) .filter(Boolean); } @@ -215,9 +236,9 @@ describe('server components take only components from client modules', () => { } } - for (const [, specifier] of contents.matchAll(STAR_REEXPORT)) { + for (const [, namespace, specifier] of contents.matchAll(STAR_REEXPORT)) { const target = fromClientModule(specifier); - if (target) yield [`${from} -> re-exports *`, target]; + if (target) yield [`${from} -> re-exports ${namespace ? `* as ${namespace}` : '*'}`, target]; } } } diff --git a/apps/web/partials/profile/profile-activity-section.test.tsx b/apps/web/partials/profile/profile-activity-section.test.tsx index 4eccaaf3d0..89c51f6b45 100644 --- a/apps/web/partials/profile/profile-activity-section.test.tsx +++ b/apps/web/partials/profile/profile-activity-section.test.tsx @@ -403,6 +403,31 @@ describe('ProfileActivitySection', () => { expect(reserve).toHaveStyle({ height: '0px' }); }); + /** + * A viewport that shrinks comes back. On iOS the URL bar returning takes height away and hiding + * it again gives the height back, so a shrink that happens to need nothing held must not retire + * the swap — there would be nothing left to rebuild the reserve when the height returns, and the + * reader would be clamped by the difference. + */ + it('keeps holding across a viewport that shrinks and grows back', () => { + const { viewport } = mockMobileActivityGeometry(600); + const { reserve } = renderActivity(); + + fireEvent.click(screen.getByRole('button', { name: /Claims/ })); + expect(reserve).toHaveStyle({ height: '150px' }); + + // The URL bar comes back, far enough that 400 + 450 is exactly the natural 850 and nothing + // needs holding — which is a fact about this instant, not about the switch being over. + viewport.height = 450; + fireEvent.resize(window); + expect(reserve).toHaveStyle({ height: '0px' }); + + // It hides again. + viewport.height = 700; + fireEvent.resize(window); + expect(reserve).toHaveStyle({ height: '250px' }); + }); + /** * The swap is over once the page can hold the reader without help, and it has to actually end. * Left armed, `holdY` outlives the switch it belongs to: a shrink long afterwards would size a diff --git a/apps/web/partials/profile/profile-activity-section.tsx b/apps/web/partials/profile/profile-activity-section.tsx index 38be7c0c93..5ebb92256e 100644 --- a/apps/web/partials/profile/profile-activity-section.tsx +++ b/apps/web/partials/profile/profile-activity-section.tsx @@ -286,6 +286,12 @@ function useMobileActivityHeightReserve(selectedKey: string | undefined) { * Zero is the safe moment to let go precisely because nothing is being held at it, so dropping * the swap cannot move anybody. Not on the first sizing though — that one runs before the * correction below, and the correction needs the swap it belongs to. + * + * Only for changes that do not come back. Cards growing and the reader scrolling up both leave + * the page needing less than it did, and go on needing less. A viewport is not like that: it + * shrinks and grows again as the URL bar returns and hides, so settling on a shrink would + * retire the swap during the half of that cycle where nothing is needed, and leave nothing to + * rebuild the reserve on the half where it is. Resizes size, and do not settle. */ const sizeAndSettle = () => { if (sizeReserve() === 0) swapRef.current = null; @@ -340,14 +346,15 @@ function useMobileActivityHeightReserve(selectedKey: string | undefined) { // `innerHeight` never moves there and there is nothing to react to. // // `resize` rather than `visualViewport`, because `window.innerHeight` is the figure the sum - // above uses and the two do not always agree. - window.addEventListener('resize', sizeAndSettle); + // above uses and the two do not always agree. And `sizeReserve` rather than `sizeAndSettle`: + // see there for why a reversible change must not retire the swap. + window.addEventListener('resize', sizeReserve); return () => { cancelAnimationFrame(arm); observer.disconnect(); window.removeEventListener('scroll', onScroll); - window.removeEventListener('resize', sizeAndSettle); + window.removeEventListener('resize', sizeReserve); }; }, [selectedKey]); From a89d7dba3bb535c51137c532d474fc32fc042fe3 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Sat, 19 Sep 2026 19:56:06 -0700 Subject: [PATCH 18/19] fix(profile): put the reader back when a growing viewport clamps them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sizing the reserve back up after the viewport grows returns the scroll range but not the reader, and the reader has already gone: while the reserve holds anything it sizes the document so `holdY` is exactly the furthest the page can scroll — that is what holding a position means — so 100px more viewport is 100px less maximum and the browser clamps by the difference. By construction, on every URL-bar transition, not occasionally. The resize path now does what the swap itself does: size, then restore, once. Synchronously inside the resize handler, which is the guard against the clamp's own scroll event — it is dispatched afterwards, so `onScroll` reads the restored position and has nothing to mistake for the reader moving up. The test harness mocked `scrollTo` as a no-op, so every assertion about where the reader ends up was made against a page that never moved. It moves the mocked position now, which is what lets the new test watch the clamp, the restore and the scroll event that follows it. --- .../profile/profile-activity-section.test.tsx | 49 ++++++++++++++++--- .../profile/profile-activity-section.tsx | 31 ++++++++++-- 2 files changed, 68 insertions(+), 12 deletions(-) diff --git a/apps/web/partials/profile/profile-activity-section.test.tsx b/apps/web/partials/profile/profile-activity-section.test.tsx index 89c51f6b45..c94bcb53fa 100644 --- a/apps/web/partials/profile/profile-activity-section.test.tsx +++ b/apps/web/partials/profile/profile-activity-section.test.tsx @@ -116,7 +116,15 @@ function mockMobileActivityGeometry(pageHeightWithoutActivity: number) { ); }); + // Moves the mocked position, the way a real one does. Mocked as a no-op it silently turned + // every "and then the reader is back at 400" into a page still sitting where it was, which is + // how a viewport resize could lose the reader with the tests all passing. + const scrollTo = vi.spyOn(window, 'scrollTo').mockImplementation(((_x: number, y: number) => { + scroll.y = y; + }) as typeof window.scrollTo); + return { + scrollTo, // Stands in for the browser moving the reader: there is no real layout here to clamp a scroll // position, and the recovery path exists for exactly that case. scroll, @@ -306,8 +314,7 @@ describe('ProfileActivitySection', () => { * with it. Where that happens they are put back, before the browser paints. */ it('puts the reader back when a shrink beat the reserve to it', () => { - const { scroll } = mockMobileActivityGeometry(600); - const scrollTo = vi.spyOn(window, 'scrollTo').mockImplementation(() => {}); + const { scroll, scrollTo } = mockMobileActivityGeometry(600); renderActivity(); @@ -326,8 +333,7 @@ describe('ProfileActivitySection', () => { * as refusing to move (GEO-2974). */ it('lets the reader scroll down after a switch', async () => { - const { scroll } = mockMobileActivityGeometry(600); - const scrollTo = vi.spyOn(window, 'scrollTo').mockImplementation(() => {}); + const { scroll, scrollTo } = mockMobileActivityGeometry(600); renderActivity(); fireEvent.click(screen.getByRole('button', { name: /Claims/ })); @@ -403,6 +409,35 @@ describe('ProfileActivitySection', () => { expect(reserve).toHaveStyle({ height: '0px' }); }); + /** + * A growing viewport moves the reader before this hook hears about it, and by construction rather + * than by chance: while the reserve holds anything it sizes the document so `holdY` is exactly the + * furthest the page can scroll, so 100px more viewport is 100px less maximum, every time. Sizing + * the reserve back up returns the range but not the reader. + */ + it('puts the reader back when a growing viewport clamps them', () => { + const { scroll, viewport, scrollTo } = mockMobileActivityGeometry(600); + const { reserve } = renderActivity(); + + fireEvent.click(screen.getByRole('button', { name: /Claims/ })); + expect(reserve).toHaveStyle({ height: '150px' }); + scrollTo.mockClear(); + + // The URL bar hides. The document is 1000 tall, so the furthest it can scroll drops from 400 to + // 300 and the browser takes the reader with it before the resize handler runs. + viewport.height = 700; + scroll.y = 300; + fireEvent.resize(window); + + expect(reserve).toHaveStyle({ height: '250px' }); + expect(scrollTo).toHaveBeenCalledWith(0, 400); + + // And the clamp arrives as a scroll event afterwards. Read as the reader moving up it would + // lower the hold to 300 and shrink the reserve to 150, undoing the restore that just happened. + fireEvent.scroll(window); + expect(reserve).toHaveStyle({ height: '250px' }); + }); + /** * A viewport that shrinks comes back. On iOS the URL bar returning takes height away and hiding * it again gives the height back, so a shrink that happens to need nothing held must not retire @@ -461,8 +496,7 @@ describe('ProfileActivitySection', () => { * wherever they have got to by then: the correction belongs to the swap that asked for it. */ it('leaves the reader alone when the cards grow after a switch', async () => { - const { scroll, sectionResized } = mockMobileActivityGeometry(600); - const scrollTo = vi.spyOn(window, 'scrollTo').mockImplementation(() => {}); + const { scroll, sectionResized, scrollTo } = mockMobileActivityGeometry(600); renderActivity(); fireEvent.click(screen.getByRole('button', { name: /Claims/ })); @@ -478,8 +512,7 @@ describe('ProfileActivitySection', () => { }); it('leaves the reader alone when the reserve did its job', () => { - mockMobileActivityGeometry(600); - const scrollTo = vi.spyOn(window, 'scrollTo').mockImplementation(() => {}); + const { scrollTo } = mockMobileActivityGeometry(600); renderActivity(); diff --git a/apps/web/partials/profile/profile-activity-section.tsx b/apps/web/partials/profile/profile-activity-section.tsx index 5ebb92256e..016f6ddf14 100644 --- a/apps/web/partials/profile/profile-activity-section.tsx +++ b/apps/web/partials/profile/profile-activity-section.tsx @@ -346,15 +346,38 @@ function useMobileActivityHeightReserve(selectedKey: string | undefined) { // `innerHeight` never moves there and there is nothing to react to. // // `resize` rather than `visualViewport`, because `window.innerHeight` is the figure the sum - // above uses and the two do not always agree. And `sizeReserve` rather than `sizeAndSettle`: - // see there for why a reversible change must not retire the swap. - window.addEventListener('resize', sizeReserve); + // above uses and the two do not always agree. And it sizes without settling: see + // `sizeAndSettle` for why a reversible change must not retire the swap. + // + // Sizing alone is not enough, because a growing viewport has already moved the reader by the + // time this runs. While the reserve holds anything, it sizes the document so that `holdY` is + // *exactly* the furthest the page can scroll — that is what holding the position means — so a + // viewport 100px taller drops the maximum by 100 and the browser takes the reader with it, + // every time rather than occasionally. Restoring afterwards is the same one-shot correction the + // swap itself gets, for the same reason. + const onViewportResize = () => { + const swap = swapRef.current; + if (!swap) { + sizeReserve(); + return; + } + + // Synchronously, inside the resize handler, and that ordering is the whole guard. The clamp + // also arrives as a scroll event, which is dispatched after this runs — so by the time + // `onScroll` reads the position it is the restored one, and there is nothing there for it to + // mistake for the reader moving up. + const target = swap.holdY; + sizeReserve(); + + if (Math.abs(window.scrollY - target) > 1) window.scrollTo(0, target); + }; + window.addEventListener('resize', onViewportResize); return () => { cancelAnimationFrame(arm); observer.disconnect(); window.removeEventListener('scroll', onScroll); - window.removeEventListener('resize', sizeReserve); + window.removeEventListener('resize', onViewportResize); }; }, [selectedKey]); From c546fe653c7ec5c636dd67d69ccec45cda765546 Mon Sep 17 00:00:00 2001 From: Preston Mantel Date: Sat, 19 Sep 2026 20:00:22 -0700 Subject: [PATCH 19/19] test: move the server/client boundary guard to its own PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It guards a tree-wide concern and was a quarter of this diff, which is a lot of review attention spent away from the mobile scroll bug this PR is about — two rounds of it, in the end. The anchor fix that found it stays here; the guard and the four pre-existing cases it lists go to #2478, where they can be judged on their own. --- ...client-values-in-server-boundaries.test.ts | 261 ------------------ 1 file changed, 261 deletions(-) delete mode 100644 apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts diff --git a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts b/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts deleted file mode 100644 index bcfadb8189..0000000000 --- a/apps/web/partials/entity-page/client-values-in-server-boundaries.test.ts +++ /dev/null @@ -1,261 +0,0 @@ -// This walks the source tree with `fs` and never touches the DOM. -// @vitest-environment node -import { existsSync, readFileSync, readdirSync } from 'node:fs'; -import path from 'node:path'; -import { describe, expect, it } from 'vitest'; - -/** - * Every export of a `'use client'` module is a *client reference* on the server, not the value it - * looks like. A Server Component that reads one gets an opaque placeholder: React writes it into - * the flight payload as `"$56"`, and the real value only appears once the client resolves the - * reference during hydration. - * - * Nothing fails. That is the whole problem. The space layout wrote `id={SPACE_TABS_ANCHOR}` where - * the constant lived in `space-tabs.tsx`, and the server HTML came back as - * `["$","div",null,{"id":"$56","className":"scroll-mt-14"}]` — an element with no id until - * hydration, so `/space/…/debates#space-tabs` had nothing to scroll to on a cold load. In the - * browser it looked perfect (GEO-2974). - * - * The sibling of this test guards the other direction — async components rendered from client - * files. Same failure mode: correct-looking UI, wrong boundary. - * - * What this cannot see: whether the value is ever *read* while rendering on the server. A client - * hook imported next to a server-safe constant and only ever called from a client component is - * inert. So the allowlist below is not a list of things that are fine — it is a list of things - * checked by hand, each with what was found. - * - * Nor does it follow `import()` or a bare `import './x'`. Neither reaches a source module from the - * server graph today — checked, not assumed: the 85 files calling `import()` are client modules - * reaching for `next/dynamic`, and every bare import in the graph resolves to CSS or a package. - * Worth adding the day either stops being true. - */ - -const ROOT = path.resolve(__dirname, '..', '..'); -const SOURCE_DIRS = ['app', 'core', 'partials', 'design-system']; - -/** The files Next renders on the server by definition. Everything they reach is the server graph. */ -const SERVER_ENTRY = /\/(layout|page|template|default|loading|error|not-found|route|opengraph-image)\.tsx?$/; - -/** - * Every way one module reaches another *at runtime*, because a traversal that follows only one of - * them walks a smaller graph than the server actually renders and quietly stops guarding the rest - * of it. The tree uses all of these: ~9800 named imports, ~340 default, ~870 namespace, 53 - * re-exports. - * - * `import type` is excluded, and it matters: the only route into `core/blocks/data/filters.ts` is a - * type import from `core/chat/edit-types.ts`, so counting it walks into the sync store and reports - * three modules that TypeScript erases before anything runs. - */ -const MODULE_EDGE = /^(?:import|export)\s+(?!type\s)[\s\S]*?from\s+['"]([^'"]+)['"]/gm; - -/** `import { a, b as c } from '…'`, with or without a default binding in front. */ -const NAMED_IMPORT_BLOCK = /^import\s+(?!type\s)(?:[A-Za-z_$][\w$]*\s*,\s*)?\{([^}]*)\}\s+from\s+['"]([^'"]+)['"]/gm; - -/** `import Local from '…'`, ignoring the `import type` and `import * as` forms. */ -const DEFAULT_IMPORT = /^import\s+(?!type\s)([A-Za-z_$][\w$]*)\s*(?:,\s*\{[^}]*\})?\s+from\s+['"]([^'"]+)['"]/gm; - -/** `import * as Local from '…'`, where every property read is a client reference. */ -const NAMESPACE_IMPORT = /^import\s+\*\s+as\s+([A-Za-z_$][\w$]*)\s+from\s+['"]([^'"]+)['"]/gm; - -/** `export { a } from '…'` and `export * from '…'`, which hand a client reference straight on. */ -const NAMED_REEXPORT = /^export\s+(?!type\s)\{([^}]*)\}\s+from\s+['"]([^'"]+)['"]/gm; - -/** - * `export * from '…'` and `export * as Name from '…'`. - * - * The named form is the common one here — 11 files against 4 — so a matcher that only knew the - * bare `export *` was blind to most of the barrels in the tree. - */ -const STAR_REEXPORT = /^export\s+\*\s+(?:as\s+([A-Za-z_$][\w$]*)\s+)?from\s+['"]([^'"]+)['"]/gm; - -/** - * Pre-existing, each one read before being listed. None of them is this PR's, and all of them are - * the same latent shape — a client reference standing where a value is expected. - * - * - `bounty-board-skeleton` is the live one: `app/bounties/loading.tsx` is server-rendered and puts - * `BOARD_GRID_CLASS` straight into a `className`, exactly the bug above. Worth its own fix. - * - `read-block-media-dimensions` would return a client reference in place of its empty-dimensions - * object; nothing calls it outside its test today, so it is a landmine rather than a fault. - * - `entity-response` calls `getChecked` while deriving a response kind; server callers would throw - * rather than render something wrong. - * - `bounties/config` is inert: `useFeatureFlag` is only ever called from `useBountiesEnabled`, - * which is a client hook. The module is in the server graph for `bountiesEnabledForNetwork`. - */ -const KNOWN = new Set([ - 'partials/bounties/bounty-board-skeleton.tsx -> BOARD_CARD_HEIGHT_PX', - 'partials/bounties/bounty-board-skeleton.tsx -> BOARD_GRID_CLASS', - 'core/blocks/data/read-block-media-dimensions.ts -> NO_BLOCK_MEDIA_DIMENSIONS', - 'core/responses/entity-response.ts -> getChecked', - 'core/bounties/config.ts -> useFeatureFlag', -]); - -function sourceFiles(): string[] { - const found: string[] = []; - const walk = (dir: string) => { - for (const entry of readdirSync(path.join(ROOT, dir), { withFileTypes: true })) { - const rel = path.join(dir, entry.name); - if (entry.isDirectory()) { - if (entry.name !== 'node_modules') walk(rel); - } else if (/\.tsx?$/.test(entry.name) && !/\.test\.tsx?$/.test(entry.name)) { - found.push(rel); - } - } - }; - for (const dir of SOURCE_DIRS) walk(dir); - return found; -} - -function isClientFile(contents: string): boolean { - return /^\s*['"]use client['"]/.test(contents); -} - -function resolveImport(specifier: string, importingFile: string): string | null { - let absolute: string; - - if (specifier.startsWith('~/')) { - absolute = path.join(ROOT, specifier.slice(2)); - } else if (specifier.startsWith('.')) { - absolute = path.resolve(ROOT, path.dirname(importingFile), specifier); - } else { - return null; - } - - const relative = path.relative(ROOT, absolute); - for (const candidate of [ - `${relative}.tsx`, - `${relative}.ts`, - path.join(relative, 'index.tsx'), - path.join(relative, 'index.ts'), - ]) { - if (!SOURCE_DIRS.some(dir => candidate.startsWith(`${dir}${path.sep}`))) continue; - if (existsSync(path.join(ROOT, candidate))) return candidate; - } - return null; -} - -/** - * `A` or `A as B` inside a named import block, skipping inline `type` specifiers. - * - * The exported name is what identifies the export, so `foo as bar` is judged as `foo`. `default as - * Bar` is the exception: `default` says nothing about what it is, so the local name is the only - * signal there — the same reasoning as a default import. - */ -function parseNamedBindings(block: string): string[] { - return block - .split(',') - .map(entry => entry.trim()) - .filter(entry => entry.length > 0 && !entry.startsWith('type ')) - .map(entry => { - const [exported, local] = entry.split(/\s+as\s+/).map(part => part.trim()); - return exported === 'default' && local ? local : exported; - }) - .filter(Boolean); -} - -/** - * Components are the one export a Server Component may take from a client module — that is what the - * boundary is for. PascalCase stands in for "component", with SCREAMING_CASE excluded, since - * `BOARD_GRID_CLASS` passes a naive capital-letter test while being a string. - */ -function looksLikeComponent(name: string): boolean { - return /^[A-Z]/.test(name) && name !== name.toUpperCase(); -} - -describe('server components take only components from client modules', () => { - const files = sourceFiles(); - const contentsByFile = new Map(files.map(file => [file, readFileSync(path.join(ROOT, file), 'utf8')])); - const clientFiles = new Set([...contentsByFile].filter(([, c]) => isClientFile(c)).map(([f]) => f)); - - it('walks a source tree that actually has files in it', () => { - // Guards against a silently empty run if the layout moves. - expect(files.length).toBeGreaterThan(50); - expect(clientFiles.size).toBeGreaterThan(50); - }); - - /** Every non-client module reachable from a server entry point, which is where this can bite. */ - const serverGraph = new Set(); - const queue = files.filter(file => SERVER_ENTRY.test(file.split(path.sep).join('/')) && !clientFiles.has(file)); - - // The seeds are the layouts, pages and loading states. If this is empty the walk above drifted. - expect(queue.length).toBeGreaterThan(20); - - while (queue.length > 0) { - const file = queue.pop()!; - if (serverGraph.has(file) || clientFiles.has(file)) continue; - serverGraph.add(file); - - for (const match of (contentsByFile.get(file) ?? '').matchAll(MODULE_EDGE)) { - const target = resolveImport(match[1], file); - if (target && !clientFiles.has(target) && !serverGraph.has(target)) queue.push(target); - } - } - - it('reaches a server graph worth checking', () => { - expect(serverGraph.size).toBeGreaterThan(100); - }); - - /** - * Every value a server-graph module takes from a client module, in any of the shapes it can - * arrive in, as `[offence, source]` pairs. A default binding is judged by its local name — the - * only signal a default import carries — and a namespace binding is reported whole, since every - * property read off it is a client reference. - */ - function* clientValuesInServerGraph(): Generator<[string, string]> { - for (const file of serverGraph) { - const contents = contentsByFile.get(file)!; - const from = file.split(path.sep).join('/'); - - const fromClientModule = (specifier: string) => { - const target = resolveImport(specifier, file); - return target && clientFiles.has(target) ? target.split(path.sep).join('/') : null; - }; - - for (const [, block, specifier] of contents.matchAll(NAMED_IMPORT_BLOCK)) { - const target = fromClientModule(specifier); - if (!target) continue; - for (const name of parseNamedBindings(block)) { - if (!looksLikeComponent(name)) yield [`${from} -> ${name}`, target]; - } - } - - for (const [, local, specifier] of contents.matchAll(DEFAULT_IMPORT)) { - const target = fromClientModule(specifier); - if (target && !looksLikeComponent(local)) yield [`${from} -> default as ${local}`, target]; - } - - for (const [, local, specifier] of contents.matchAll(NAMESPACE_IMPORT)) { - const target = fromClientModule(specifier); - if (target) yield [`${from} -> * as ${local}`, target]; - } - - for (const [, block, specifier] of contents.matchAll(NAMED_REEXPORT)) { - const target = fromClientModule(specifier); - if (!target) continue; - for (const name of parseNamedBindings(block)) { - if (!looksLikeComponent(name)) yield [`${from} -> re-exports ${name}`, target]; - } - } - - for (const [, namespace, specifier] of contents.matchAll(STAR_REEXPORT)) { - const target = fromClientModule(specifier); - if (target) yield [`${from} -> re-exports ${namespace ? `* as ${namespace}` : '*'}`, target]; - } - } - } - - it('finds no non-component value taken from a "use client" module', () => { - const offences = [...clientValuesInServerGraph()] - .filter(([offence]) => !KNOWN.has(offence)) - .map(([offence, target]) => `${offence} (from ${target})`); - - expect(offences).toEqual([]); - }); - - it('keeps the known list honest', () => { - // An entry that no longer matches anything has been fixed, and leaving it here would quietly - // re-permit the same import later. - const live = new Set([...clientValuesInServerGraph()].map(([offence]) => offence)); - - expect([...KNOWN].filter(entry => !live.has(entry))).toEqual([]); - }); -});