You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Split out of #2466, where this was ~25% of the diff and had nothing to do with the mobile scroll bug that PR is about.
What it guards
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 throws. That is what makes it worth a test rather than a code review note.
The case that prompted it, from #2466 — the space layout wrote id={SPACE_TABS_ANCHOR} with the constant living in space-tabs.tsx:
An element with no id until hydration, so a fragment link had nothing to scroll to on a cold load. In the browser it looked perfect and every test passed.
How
Walks the server render graph from every layout / page / loading / route / template / error / not-found / opengraph-image, and fails on any non-component value taken from a client module — named, default, namespace or re-exported.
Two things that took a measurement rather than a guess:
Type-only edges are excluded, and that is load-bearing. Following every from edge reports four modules that do not exist at runtime: the sole route into core/blocks/data/filters.ts is an import type in core/chat/edit-types.ts, and counting it walks on into the sync store.
import() and bare imports are followed, and reach nothing today. Both are recorded as traversal edges, and both are dormant: the files calling import() are client modules reaching for next/dynamic, and every bare import in the graph resolves to CSS or a package. That is measured, not assumed — deleting either branch leaves every corpus assertion green, which is why they are covered by callReferences fixtures instead.
Following all edge kinds rather than named imports alone takes the guarded graph from 401 to 407 modules. The six are default-entity-page.tsx, post-entity-page.tsx, entity-page-header.tsx, fetch-entity-page-data.ts, core/id/create-id.ts and partials/activity/activity-layout.tsx — reached by default or namespace import, and invisible to a named-imports-only walk.
What it finds today
Listed rather than fixed, each read by hand. The fix for all of them is the same shape — move the value into a module with no directive and import it from both sides.
where
what
status
partials/bounties/bounty-board-skeleton.tsx
BOARD_GRID_CLASS, BOARD_CARD_HEIGHT_PX
live — app/bounties/loading.tsx is server-rendered and puts the class straight into a className, so the grid has no grid during the loading flash
core/responses/entity-response.ts
getChecked
a server caller would throw — the better failure of the two
core/blocks/data/read-block-media-dimensions.ts
NO_BLOCK_MEDIA_DIMENSIONS
landmine — would return a reference in place of its empty-dimensions object; nothing calls it outside its own test
core/bounties/config.ts
useFeatureFlag
inert — only ever called from useBountiesEnabled, a client hook
The bounties one is a real bug and I have deliberately not fixed it here.BOARD_CARD_HEIGHT_PX derives from AVAILABLE_CARD_HEIGHT_PX in a second client module, so the fix relocates layout constants across two features and wants someone who can look at the bounties board while doing it. Happy to take it if it is not already spoken for.
The list is itself checked for staleness, so an entry that gets fixed cannot sit here quietly re-permitting the same import later.
Verification
Planted export * as Probe2 from '~/core/state/feature-flags' in the space layout: unreported by the named-imports-only version, caught by this one. Same for reverting the anchor import that motivated it.
Sibling of async-components-in-client-boundaries.test.ts, which guards the other direction of the same boundary — async components rendered from client files. Same failure mode: correct-looking UI, wrong side of the line.
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.
…hips, dead space (GEO-2974) (#2466)
* fix(profile): stop the Activity gallery moving the reader, and unstack its bottom row (GEO-2974)
**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) <noreply@anthropic.com>
* feat(profile): report what the Activity gallery is doing on the device it is doing it on (GEO-2974)
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) <noreply@anthropic.com>
* refactor: move the autoplay work out of this PR (GEO-2974)
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) <noreply@anthropic.com>
* Revert the Activity gallery's height floor (GEO-2974)
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) <noreply@anthropic.com>
* fix(profile): preserve mobile activity scroll position
* fix(profile): show the mobile activity reserve on mobile (GEO-2974)
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.
* fix(profile): widen the activity claim card so its pills sit side by side (GEO-2974)
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).
* feat(profile): stop nesting the mobile activity gallery inside a card (GEO-2974)
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.
* fix(profile): stop the activity reserve dragging the reader back up (GEO-2974)
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.
* feat(profile): land See all on the tab bar rather than the page top (GEO-2974)
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.
* refactor(profile): tighten the activity tests and the comments around them
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.
* fix(profile): keep the tab anchor off the client boundary, and let the swap end
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.
* revert(debates): leave the gated autoplay thresholds alone
Backed out of ee27426. 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.
* docs(profile): say what the anchor fix did and did not fix
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.
* fix(profile): watch every input the height reserve depends on
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.
* docs(profile): name the browser the viewport fix is for
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.
* fix(profile): a viewport that shrinks and grows back must not end the swap
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.
* fix(profile): put the reader back when a growing viewport clamps them
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.
* test: move the server/client boundary guard to its own PR
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.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Worth keeping, and I'd merge it. Of the three you split out, this is the one that earns its place most clearly.
I checked it rather than read it:
Passes on current master — 4 tests, 463ms.
It actually fires. I planted import { MOBILE_LAYOUT_MAX_WIDTH_PX } from '~/core/hooks/use-is-mobile-layout' into app/space/[id]/(space)/layout.tsx and it failed with exactly the offender:
A guard nobody has watched fail is decoration. This one isn't.
The self-checks are the part I'd have asked for if they weren't there — asserting the walk found files, that there are >20 seeds and >100 graph modules means it can't go green on an empty run after a directory move. That's the failure mode this kind of test usually dies of, silently.
Two notes, neither a blocker:
looksLikeComponent is a heuristic with a blind spot. PascalCase-but-not-SCREAMING_CASE means a client module exporting const DefaultConfig = {...} or const Foo = 3 passes straight through. That's fine as a trade — the alternative is resolving types — but it's worth a line in the doc block saying so, because the test reads as exhaustive and the next person will assume coverage it doesn't have.
Merging this doesn't fix the bounties bug, it pins it.BOARD_GRID_CLASS going through app/bounties/loading.tsx is a real live defect and the KNOWN list makes it permanent-looking rather than urgent. Worth a ticket so it doesn't just live in a set literal — happy for you to take it as you offered.
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 what makes it worth a test. A profile branch had the space
layout write `id={SPACE_TABS_ANCHOR}` with the constant living in a client
module, and the deployed HTML came back carrying `{"id":"$56"}`: an element with
no id until hydration, and a fragment link with nothing to scroll to. It looked
right in the browser and every test passed.
This walks the server render graph from every layout, page, loading state and
route handler, and fails on any non-component value taken from a client module —
named, default, namespace or re-exported. Type-only edges are excluded, which is
load-bearing rather than tidy: 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.
Five cases exist today. They are listed with what each one does and what fixing
it would take, rather than fixed here: one is live (`app/bounties/loading.tsx`
renders `className={BOARD_GRID_CLASS}` from a client module), one would throw,
one is a landmine, one is inert. The list is checked for staleness too, so an
entry that gets fixed cannot sit here re-permitting the same import later.
Sibling of `async-components-in-client-boundaries.test.ts`, which guards the
other direction of the same boundary.
…pelled
Review found the hole: capitalisation alone let `const DefaultConfig = {...}`
through, and a client module exporting a capitalised value is exactly the case
this test exists for. Planted one and it sailed past.
Three questions now, cheapest first: is it capitalised, is it used here as a
component — rendered as `<Name>` or handed on as `render={Name}` — and does the
source declare it as a type the compiler erases. Capitalised and neither of the
last two is a value wearing a component's name.
Measured against the tree rather than reasoned about: of 163 capitalised imports
from client modules in the server graph, 161 are rendered as JSX where they are
imported, and the other two are `export type` imported without the `type`
keyword — `Tabs` from `editor-provider` and `Feature` from `use-place-search`.
So the type check is not defensive, it is the difference between this landing
clean and reporting two files that do nothing at runtime.
The prop form is for a case the tree does not hold yet — a server module taking
a client component and passing it on without rendering it — because the failure
would otherwise be a false accusation, and a guard that cries wolf gets deleted.
Proved all three by planting them: the value is caught, the rendered component
and the passed-on component are not.
Doc block now says what is still uncovered, which the review asked for: a
component neither rendered nor passed on here would be reported, and a
capitalised value re-exported from a barrel is let through, since a re-export
has no use site to read.
Thanks — both notes addressed, and the first one turned out to be closable rather than just documentable.
looksLikeComponent blind spot. You were right that const DefaultConfig = {...} sailed straight through; I planted one and confirmed it. Rather than only document it, I used the technique your sibling test already uses — read the use site. It now asks three questions: is the name capitalised, is it used here as a component (<Name> or render={Name}), and does the source declare it as a type the compiler erases.
Measured before writing it, because the type check is not defensive padding: of 163 capitalised imports from client modules in the server graph, 161 are rendered as JSX where they are imported, and the other two are export type imported without the type keyword — Tabs from editor-provider and Feature from use-place-search. Without that check the guard reports two files that do nothing at runtime.
Proved on three planted cases: the capitalised value is caught, a rendered client component is not, and a client component passed on as render={Foo} without being rendered here is not. That last form is in there for a case the tree does not hold yet, because a false accusation is how a guard like this gets deleted.
The doc block now says what is still uncovered, which is smaller but not empty: a component this file neither renders nor passes on would be reported, and a capitalised value re-exported from a barrel is let through, since a re-export has no use site to read.
The bounties bug — taken, not ticketed.#2515, stacked on this branch so the diff is the bounties change alone. You are right that KNOWN makes it look permanent, so it does not sit there: the two entries come off the list in that PR, and this test's own staleness check is what forces them to.
The fix needed more than moving one constant — BOARD_CARD_HEIGHT_PXisAVAILABLE_CARD_HEIGHT_PX from bounty-card.tsx, another client module, so the three sibling card heights move to a server-safe module together. Both original modules re-export what they declared, so no existing importer or test mock changes. And the guard now protects it: point the skeleton's import back at board-bounty-card and it fails with exactly that pair.
One thing I have not done: watched the loading state render. The mechanism is confirmed, the visual symptom I am taking on reasoning — flagged in that PR for someone who can open the board.
Seven findings, all of them real, and six were the same mistake in different
places: a regex standing in where parsing was needed.
`[\s\S]*?` does not stop at the end of a declaration. In `app/layout.tsx` the
one loose matcher started on a bare `import 'katex/…css';`, walked past two
statements and captured a later specifier — three overruns in one small file, and
every statement it swallowed was skipped. It would equally start on an
`export const` and run until it found a `from` dozens of lines away. There is a
matcher per declaration shape now, each bounded to its own, and the traversal and
the detection read from the same set.
That set also grew. `import Default, * as Namespace from './client'` matched
neither of the old two patterns and both bindings went through. `import()` and
bare imports are followed rather than assumed harmless — the assumption held when
I measured it, but nothing kept it true, and a module reachable only through
`await import()` was unguarded. `atoms` was missing from the roots entirely,
which fails twice over: never loaded, and every `~/atoms/…` specifier resolving
to null.
Classification moves to the declaration. Asking the use site let
`config={DefaultConfig}` prove an object was a component, and had nothing at all
to say about a re-export. Reading how the client module declares the export
answers both: 144 of the tree's capitalised imports are `export function`, 17 an
arrow or memo/forwardRef, 2 an `export type` imported without the keyword, and
nothing is unclassifiable — so `unknown` is reported rather than waved through.
Re-exports keep their alias, so `export { default as Button }` stops being
reported and `export { DefaultConfig }` starts.
The allowlist is keyed by client module as well as importer and name. Without it
an entry went on authorising the same name after the import was repointed at a
different client module — debt licensing a new fault.
Every one of the seven was planted before and after: caught by the new version,
missed by the old, and the two that should stay silent do. The per-root assertion
earned its place immediately by catching `styles` being added to the roots when
it holds nothing but a test.
Server graph: 377 modules two rounds ago, 402 last round, 407 now.
…the boundary
`app/bounties/loading.tsx` is a Server Component and `BountyBoardSkeleton` puts
`BOARD_GRID_CLASS` straight into a `className`. It was declared in
`board-bounty-card.tsx`, which is `'use client'`, and every export of a client
module is a client reference on the server rather than the value it looks like —
so the loading grid rendered with no grid at all until hydration replaced it.
`BOARD_CARD_HEIGHT_PX` went the same way into a `style`.
Both now live in `board-layout.ts`, which carries no directive. That needed the
card heights to come along, because `BOARD_CARD_HEIGHT_PX` is
`AVAILABLE_CARD_HEIGHT_PX` and that was declared in `bounty-card.tsx`, also a
client module — so the three sibling heights move to `bounty-card-layout.ts`
together rather than being split across two files. Both original modules
re-export what they used to declare, so every existing importer is untouched.
Found by the guard in #2478, which has the pair listed as known debt. They come
off that list here, and its staleness check is what makes sure they had to.
Four more findings, all four real and all four in the classifier from the round
before. That is the finding: this was the third round of patching the same
regexes, and every question they were failing at — what is a directive, what is a
class, what does an identifier resolve to, what does a parenthesis contain — is a
question about syntax that the compiler already answers.
So it parses now. `ts.createSourceFile` over 1,600 files, top-level statements
only, and the classifier reads declarations rather than the text near them.
Each of the four, planted before and after:
- `'use client'` behind a licence header. The regex saw the first token only, so a
real client module went unrecognised — and that is worse than unchecked,
because it joins the server graph and everything it exports stops being an
offence anywhere. Proved exactly that: with the header, the old version missed
the planted import and started reporting the module's own imports instead.
- `export default SuggestedFormats`, a component declared above and exported by
name, in a real file. Reported as unclassifiable before, silent now — the
initialiser is followed through an identifier, and through parentheses, `as`
and `satisfies` too.
- `export class GeoChatRequestError extends Error`, also a real file. A capital
letter made it a component; heritage decides it now, so only React's base
classes qualify.
- `export const DefaultConfig = ({ enabled: true })` and `export const
ButtonStyles = cva('x')`. A leading bracket and a call both read as components
before. A call is a value unless it is `memo` or `forwardRef`.
A fifth test pins the classifier's shape against the tree: of the capitalised
exports the server graph takes from client modules, 100+ are components, exactly
2 are `export type` imported without the keyword, and none is a value or
unreadable. A change that started calling components values moves that before it
reaches the allowlist.
Costs 2.3s against 0.5s, nearly all of it loading the compiler.
One thing still read from the text: `import('literal')`, because finding those in
the tree means walking every node of every file and a computed specifier resolves
to nothing anyway.
…the boundary
`app/bounties/loading.tsx` is a Server Component and `BountyBoardSkeleton` puts
`BOARD_GRID_CLASS` straight into a `className`. It was declared in
`board-bounty-card.tsx`, which is `'use client'`, and every export of a client
module is a client reference on the server rather than the value it looks like —
so the loading grid rendered with no grid at all until hydration replaced it.
`BOARD_CARD_HEIGHT_PX` went the same way into a `style`.
Both now live in `board-layout.ts`, which carries no directive. That needed the
card heights to come along, because `BOARD_CARD_HEIGHT_PX` is
`AVAILABLE_CARD_HEIGHT_PX` and that was declared in `bounty-card.tsx`, also a
client module — so the three sibling heights move to `bounty-card-layout.ts`
together rather than being split across two files. Both original modules
re-export what they used to declare, so every existing importer is untouched.
Found by the guard in #2478, which has the pair listed as known debt. They come
off that list here, and its staleness check is what makes sure they had to.
Five findings. Four implemented as asked, one implemented the other way round
because measuring says the suggested fix fails the build on master.
Seeded: the metadata routes. `robots`, `sitemap`, `manifest`, `icon`,
`apple-icon` and `twitter-image` are modules Next runs on the server as much as
a page is, and `app/robots.ts` is already in this tree, so anything reachable
only through it sat outside the walk. Planted a value import there — silent
before, caught now.
A directive is a string literal. `ts.isStringLiteralLike` also accepts a
no-substitution template literal, so a file opening `` `use client` `` was taken
for a client module; planted one and the old version raised a false offence
against it. Module specifiers get the same stricter predicate, where a template
literal is not legal either.
A tagged template is a value. `gql`, `css` and `sql` produce data, and
`styled.div` — the tag that would produce a component — is used nowhere in this
repo; its only mention was the comment justifying the exemption. `export const
SpaceQuery = gql\`…\`` goes through before and is caught now.
Dynamic imports are read from the tree. The regex matched inside comments, and
would have matched `type T = import('./types').T`; planted the comment case and
the old version followed it into a module nothing imports. A `CallExpression`
whose callee is the `import` keyword cannot be either. 86ms for the walk, and
nothing in this file reads source text any more.
The exception is capitalised functions. Review asked for evidence of
renderability, or `unknown` without it. Of the 520 capitalised functions client
modules export, 505 contain JSX and 15 do not — and all 15 are components that
render nothing and only run effects. Six are imported by the server graph today,
so that rule would accuse `SpaceRedirect`,
`PersonalProfileSuggestedTaskSync` and `PersonalProfileBioStarterMerge` of being
values, and a guard that accuses real components is one somebody deletes. The
evidence runs the other way instead: every `return` handing back an object,
array, string or number makes it a value, which catches
`function BuildOptions() { return {}; }` — the case review named — and leaves the
effect-only components alone.
…the boundary
`app/bounties/loading.tsx` is a Server Component and `BountyBoardSkeleton` puts
`BOARD_GRID_CLASS` straight into a `className`. It was declared in
`board-bounty-card.tsx`, which is `'use client'`, and every export of a client
module is a client reference on the server rather than the value it looks like —
so the loading grid rendered with no grid at all until hydration replaced it.
`BOARD_CARD_HEIGHT_PX` went the same way into a `style`.
Both now live in `board-layout.ts`, which carries no directive. That needed the
card heights to come along, because `BOARD_CARD_HEIGHT_PX` is
`AVAILABLE_CARD_HEIGHT_PX` and that was declared in `bounty-card.tsx`, also a
client module — so the three sibling heights move to `bounty-card-layout.ts`
together rather than being split across two files. Both original modules
re-export what they used to declare, so every existing importer is untouched.
Found by the guard in #2478, which has the pair listed as known debt. They come
off that list here, and its staleness check is what makes sure they had to.
It was added to the set two rounds ago and the comment above it still listed
`memo` and `forwardRef` — the small version of the mistake this file keeps
making, a list extended in one place and described in another.
No behaviour change, so nothing to prove: the set was already right.
…the boundary
`app/bounties/loading.tsx` is a Server Component and `BountyBoardSkeleton` puts
`BOARD_GRID_CLASS` straight into a `className`. It was declared in
`board-bounty-card.tsx`, which is `'use client'`, and every export of a client
module is a client reference on the server rather than the value it looks like —
so the loading grid rendered with no grid at all until hydration replaced it.
`BOARD_CARD_HEIGHT_PX` went the same way into a `style`.
Both now live in `board-layout.ts`, which carries no directive. That needed the
card heights to come along, because `BOARD_CARD_HEIGHT_PX` is
`AVAILABLE_CARD_HEIGHT_PX` and that was declared in `bounty-card.tsx`, also a
client module — so the three sibling heights move to `bounty-card-layout.ts`
together rather than being split across two files. Both original modules
re-export what they used to declare, so every existing importer is untouched.
Found by the guard in #2478, which has the pair listed as known debt. They come
off that list here, and its staleness check is what makes sure they had to.
No committed fixture exercises this dynamic-import extraction path, and the PR states that no current server-graph import() resolves to local source. Consequently, removing this branch can leave every corpus assertion green while restoring the exact traversal gap it is meant to close. Add a focused callReferences fixture for string and no-substitution-template dynamic imports.
Handle export assignments as namespace client bindings
This fallback is not limited to a bare side-effect call. For example, export default require('./client') and export = require('./client') reach it, produce a reference with no exported name, and are then skipped by clientBindings(), silently missing a default/namespace client-value re-export. Record an ExportAssignment as a consumed namespace binding before the side-effect fallback.
Update description to reflect bare import and dynamic import traversal
The PR description says that bare imports and import() are not followed, but this implementation records bare imports as traversal edges (and callReferences does the same for dynamic imports). Update the description to match the intended behavior so reviewers and future maintainers do not rely on the obsolete limitation.
`export default require('./client')` fell through to the side-effect fallback
and produced a reference with no binding, which `clientBindings` then skips — a
barrel could hand a whole client module on and the guard would report nothing.
An `ExportAssignment` now records the module object, the way `const ns =
require(…)` already did.
The other two are the same problem as the fix I shipped for the entry-point
anchor: logic whose only evidence was a throwaway probe I ran once. Deleting the
dynamic-import branch, or reading a backtick `use client` as a directive, left
every corpus assertion green, because nothing in this tree exercises either
path. `callReferences` and `isClientModule` now have fixtures, so the next
person to simplify them finds out from the suite instead of from production.
Each fixture group was checked by breaking the code it covers: nine mutations,
nine failures, none of them in the corpus assertions.
Review round on 2f5a374c9 — no open findings, three "previously missed". All three were right. Fixed in 615c6b0.
Handle export assignments as namespace client bindings (line 167) — correct, and it is the one with teeth. export default require('./client') has its call parented by an ExportAssignment, which fell past every branch to the side-effect fallback, so the reference carried no exported name and clientBindings skipped it. A barrel written that way could hand a whole client module to a server module and the guard would report nothing. An ExportAssignment now records the module object, the way const ns = require(…) already did:
Two notes on the fix as suggested. It named export = require() and export default require() separately; one ts.isExportAssignment covers both, because TypeScript represents them as the same node and only isExportEquals separates them. And I checked whether export { x } from needed the same treatment — it does not, and cannot: a call is never the direct child of an ExportDeclaration. A first pass of mine had that check in, and it was dead code.
Enumerating the class — a CommonJS re-export that hands the whole module object on, rather than the one flagged line. The AST shapes are ExportAssignment (both forms, now handled) and module.exports = require('./x'), whose parent is a plain BinaryExpression. I have not added the module.exports branch, and the reason is the same one I gave for declining the argumentExpression guard two rounds ago: there are zero module.exports, exports. and export = occurrences in the five walked roots, and the whole tree contains exactly one require( — markdown-adapter reaching the tiptap extensions, already covered. A branch for a shape this walk cannot encounter reads as if it could, and the next person has to work out that it cannot. export default require() is the one of the three that can genuinely compile and run here, and that is the one now handled.
Add fixtures covering dynamic import extraction (line 100) — right, and it is precisely the critique I have been applying to my own fixes in this PR, turned back on me. Deleting the import() branch leaves every corpus assertion green, because no server-graph import() resolves to local source. My only evidence for that branch was a throwaway probe I ran once.
Enumerated across the file: of the fourteen module-scope functions, three had committed fixtures (SERVER_ENTRY, verdictFor, exportKindsOf), and the classifiers are covered thoroughly throughexportKindsOf. The real gap was two functions, both of which I changed in later rounds on probe evidence alone — callReferences/requireBindings, and isClientModule, where reading a backtick `use client` as a directive would move a server module into the client set and silently stop the walk at it. Both now have describe blocks: 22 new tests, 99 total.
Proved by breaking each one. Nine mutations, nine failures, none of them in the corpus assertions — which is the finding restated as evidence:
export-assignment removed 2 failed
dynamic-import edge removed 2 failed
isStringLiteralLike -> isStringLiteral 1 failed (no-substitution-template specifier)
directive: isStringLiteral -> ...Like 1 failed (`use client` read as a directive)
directive prologue: return -> continue 1 failed ('use client' after an import)
local falls back to exported name 1 failed
destructure propertyName dropped 1 failed
namespace require removed 1 failed
computed key treated as a name 2 failed
One fixture has nothing to mutate and is a characterisation test rather than a guard: type T = import('./panel').T produces no CallExpression at all, so the erasure it relies on is a property of reading the tree instead of the text.
Update description to reflect bare import and dynamic import traversal (line 793) — correct, the description was stale for three rounds. Fixed above. Re-measuring while I was in there caught a second stale number in the same paragraph: the all-edge-kinds delta was 401 → 407 modules today, not 377 → 402, and the six are now listed by name.
No behaviour change to anything you have on a device — this PR adds a test file and touches no app code.
Another round: no. Same recommendation as last round, and this round is evidence for it rather than against. The three findings were all real, all in the "prove it, don't assert it" class, and none of them changed what the guard reports on this tree — the corpus is unmoved at 407 modules and the same four known offences. What the reviewer has left to give is process critique of the test file, which it has now given and I have taken. The two findings before these were factually wrong about the installed TypeScript, which is the signal I said I would stop on.
…the boundary
`app/bounties/loading.tsx` is a Server Component and `BountyBoardSkeleton` puts
`BOARD_GRID_CLASS` straight into a `className`. It was declared in
`board-bounty-card.tsx`, which is `'use client'`, and every export of a client
module is a client reference on the server rather than the value it looks like —
so the loading grid rendered with no grid at all until hydration replaced it.
`BOARD_CARD_HEIGHT_PX` went the same way into a `style`.
Both now live in `board-layout.ts`, which carries no directive. That needed the
card heights to come along, because `BOARD_CARD_HEIGHT_PX` is
`AVAILABLE_CARD_HEIGHT_PX` and that was declared in `bounty-card.tsx`, also a
client module — so the three sibling heights move to `bounty-card-layout.ts`
together rather than being split across two files. Both original modules
re-export what they used to declare, so every existing importer is untouched.
Found by the guard in #2478, which has the pair listed as known debt. They come
off that list here, and its staleness check is what makes sure they had to.
This bare-import edge is not covered by the callReferences fixtures, because those only exercise dynamic import() and require(). The current server graph's bare imports resolve only to CSS or packages, so removing this branch leaves the corpus assertions green and reopens the documented silent traversal gap. Extract the static reference collector so a fixture can assert import './helper' (and the empty import/export forms) produces an edge.
Clarify classifier rule for definitely non-renderable returns
This description contradicts the implemented classifier and its fixtures: returned arrays, strings, and numbers are valid React output and are intentionally classified as components below. Describe the rule in terms of definitely non-renderable return values so future changes do not restore the already-corrected primitive-return behavior.
Types and values are separate declaration spaces, so one name can legally hold
both, and `export const SortOrder = {…}` followed by `export type SortOrder = …`
is the Effect idiom this tree uses seventeen times — in both orders, because
nothing makes you pick one. The erased half was recorded with an unconditional
`set`, so whichever came last won, and `erased` is the single kind `verdictFor`
waves through. A client value written that way crossed the boundary with nothing
reported: not a wrong answer, which is the failure this guard exists to prevent.
Recording an erased declaration now leaves a runtime one of the same name alone.
Only in that direction — a name with a runtime declaration has a runtime export,
so a type never makes it erased, while the reverse order was already right. Both
branches that can collide take the guard: types and interfaces, and `declare`,
where `export function X` merging with `export declare namespace X` is legal.
The third erased write keeps its plain `set`, because an explicit type-only
clause has to be able to outrank an `export *` of the same name.
`staticReferences` moves out of the suite to module scope for the same reason
`callReferences` got fixtures last commit: inside the closure it was only ever
exercised against this tree, where every bare import resolves to CSS or a
package, so deleting that edge left every corpus assertion green. Its own doc
block claimed the opposite of what it did.
And `classifyFunction`'s comment listed arrays, strings and numbers as values,
which is the behaviour an earlier round corrected — React renders all three. It
now defers to `returnsAValue` rather than keeping a second copy of the list,
which is the mistake this file has made four times.
Thirteen mutations, thirteen failures.
Round on 615c6b01e — one open finding and two "previously missed". All three right; fixed in 6d827c6. The open one is answered on its thread (#discussion_r4099679833) — declaration merging let an erased type overwrite the runtime value it shares a name with, which is live in this tree 17 times.
Cover static bare-import references in callReferences fixtures (line 804) — correct, and it is my own last round's fix applied one function further. I closed the "proved by a probe, not by a fixture" gap for callReferences and isClientModule and missed references(), for the reason the finding names: it was a closure inside the suite, so there was nowhere to put a fixture. Deleting the bare-import edge leaves every corpus assertion green, because every bare import in this tree resolves to CSS or a package — which the PR description says out loud, so this was documented and untested at the same time.
Extracted to module scope as staticReferences, along with the Reference type, and references(file) is now the memoising wrapper. Mechanical: the body was already pure apart from the cache lookup, and the corpus is unmoved at 407 modules and the same four known offences.
Fixtures for every edge it records and every edge it must not, since the type-only exclusion is the load-bearing half:
import './x'; edge, no binding (dormant in this tree)
import {} from './x'; edge, no binding (dormant)
export {} from './x'; edge, no binding (dormant)
import type { A } from './x'; nothing
import { type A } from './x'; nothing
export type { A } from './x'; nothing
export { type A } from './x'; nothing
import Panel, * as Everything default + namespace, one statement
export { Inner as Outer } from both names kept
export * from './x' starReexport
export * as Ns from './x' namespace
Clarify classifier rule for definitely non-renderable returns (line 356) — right, and it is the same shape as the lazy comment two rounds ago, except worse: the comment did not merely omit something, it described the behaviour an earlier round corrected. It listed arrays, strings and numbers as values; the classifier treats all three as components because React renders them, and the fixtures assert it. Someone trusting the comment over the code would have reintroduced the bug.
Fixed by deleting the list rather than correcting it. returnsAValue already documents what is unrenderable, correctly, twenty lines down, and classifyFunction's comment now defers to it. Two lists that had to be kept level is the mistake this file has made four separate times; the fix for the fourth one was to stop having two lists, so keeping a third in a comment was not defensible.
Thirteen mutations across the three fixes, thirteen failures:
erased clobbers a merged value 1 failed
type/interface not recorded at all 4 failed
declare not recorded 3 failed
type-only element uses the guard 1 failed
bare import edge removed 2 failed
empty import list edge removed 1 failed
empty export list edge removed 1 failed
type-only import followed 3 failed
type-only specifier followed 2 failed
type-only re-export followed 1 failed
default import dropped 1 failed
re-export alias dropped 1 failed
star re-export recorded as a namespace 1 failed
77 → 99 → 113 tests. No app code touched, so nothing you have on a device moves.
Another round: I have changed my mind — yes, one more. I said no twice, on the grounds that the reviewer had started costing me correctness. This round contradicts that: the declaration-merging finding is a genuine silent-guard hole with 17 live instances of the shape that triggers it, and it is the kind I would not have found by rereading, because it needs someone to ask "what else can legally share this name?". The two previously-missed findings were also right, and both were incomplete applications of fixes I had made myself one round earlier. Nothing this round was wrong, which is the opposite of the signal I stopped on.
The specific thing I want a look at is the code I just moved: staticReferences changed scope, and exportKindsOf gained a helper that makes one write conditional. If the next round comes back with nothing on either, I would merge on that.
…the boundary
`app/bounties/loading.tsx` is a Server Component and `BountyBoardSkeleton` puts
`BOARD_GRID_CLASS` straight into a `className`. It was declared in
`board-bounty-card.tsx`, which is `'use client'`, and every export of a client
module is a client reference on the server rather than the value it looks like —
so the loading grid rendered with no grid at all until hydration replaced it.
`BOARD_CARD_HEIGHT_PX` went the same way into a `style`.
Both now live in `board-layout.ts`, which carries no directive. That needed the
card heights to come along, because `BOARD_CARD_HEIGHT_PX` is
`AVAILABLE_CARD_HEIGHT_PX` and that was declared in `bounty-card.tsx`, also a
client module — so the three sibling heights move to `bounty-card-layout.ts`
together rather than being split across two files. Both original modules
re-export what they used to declare, so every existing importer is untouched.
Found by the guard in #2478, which has the pair listed as known debt. They come
off that list here, and its staleness check is what makes sure they had to.
…ssing entries
`export { Foo } from './values'` beside `export * from './components'` resolves
`Foo` to `./values`, whichever order the two are written in — that is the
language's rule, not a preference. Stars were applied in source order, so an
explicit export written above one read as whatever the star happened to hold: a
component, and waved through. The reverse order worked by accident.
Stars now run in a second pass and fill only the names nothing else claimed, so
a declaration, a named re-export, a namespace re-export and a type-only clause
all outrank them from either side. This is the hole I described in the last round
and left as a follow-up; it is a silent one, so it belongs here.
`SERVER_ENTRY` was also short by four conventions, read off `FILE_TYPES` and
`HTTP_ACCESS_FALLBACKS` in the installed Next 16.2.0 rather than from memory:
`global-error`, `global-not-found`, `forbidden`, `unauthorized`. Next loads all
four itself, so nothing has to import them, and a convention missing from the
seed list is a subtree the walk never enters. `app/global-error.tsx` is in this
tree today — it says `use client`, so it is excluded as a client module either
way and the graph is unmoved at 407, which is why the pattern is tested directly
rather than through the corpus.
Eight mutations, eight failures.
export * precedence was source-order dependent. An explicit export written above a star read as whatever the star held. This is the hole I described at the end of the last round and left as a follow-up; the reviewer generalised it past the type-only half I had found to the runtime shape, which is the worse one. Stars now run in a second pass and fill only unclaimed names. Enumerating "what outranks a star" rather than the flagged pairing caught a third shape neither of us named — a local declaration above a star, which is the most likely of them to appear in a real barrel. Three of six orderings were wrong before; all six have fixtures.
SERVER_ENTRY was short by four conventions, not three. Read off FILE_TYPES/HTTP_ACCESS_FALLBACKS in the installed Next 16.2.0: global-error is the one the review missed, and the only one of the four that exists here — app/global-error.tsx, which nothing imports. It says use client, so the graph is unmoved at 407 either way; the pattern is tested directly rather than through the corpus, because a fix with nothing to fail is a shape I have already shipped once on this PR.
One thing reported and not fixed, detailed on the precedence thread: clientBindings judges every name a star's target exports, so a module that claims one of those names explicitly would be reported for a binding it does not actually have. Over-reporting rather than silent, needs the claimed names plumbed from staticReferences, and zero modules here have a star plus an explicit clause. Say the word if it should go in here.
Another round: yes, one more, and then I stop regardless. Last round I said I wanted eyes on the two things I had just moved; the reviewer instead found the thing I had deferred, which is a better outcome and the second round running that produced only real findings. But I want to be honest about the pattern rather than keep extending: what these rounds are now finding is the guard's own edge semantics — which export form wins, which file Next loads — and each fix has left the corpus at 407 modules and the same four offences. That is worth having and it is also asymptotic.
So: one round on 91e4ac6, because the star restructure is the largest single change I have made to this file and it moved the write that every other branch now depends on. If it comes back clean, or with findings about hypotheticals rather than about this tree, I merge.
…the boundary
`app/bounties/loading.tsx` is a Server Component and `BountyBoardSkeleton` puts
`BOARD_GRID_CLASS` straight into a `className`. It was declared in
`board-bounty-card.tsx`, which is `'use client'`, and every export of a client
module is a client reference on the server rather than the value it looks like —
so the loading grid rendered with no grid at all until hydration replaced it.
`BOARD_CARD_HEIGHT_PX` went the same way into a `style`.
Both now live in `board-layout.ts`, which carries no directive. That needed the
card heights to come along, because `BOARD_CARD_HEIGHT_PX` is
`AVAILABLE_CARD_HEIGHT_PX` and that was declared in `bounty-card.tsx`, also a
client module — so the three sibling heights move to `bounty-card-layout.ts`
together rather than being split across two files. Both original modules
re-export what they used to declare, so every existing importer is untouched.
Found by the guard in #2478, which has the pair listed as known debt. They come
off that list here, and its staleness check is what makes sure they had to.
Dropping a computed access to a bindingless edge makes const value = require('./client')[key] invisible: traversal stops at the client module, and clientBindings skips this reference because it has neither namespace nor exported. Since this expression definitely reads some client export, classify it as an unknown/namespace binding so it is reported rather than silently bypassing the guard.
Track erased names from export type star re-exports
Skipping export type * entirely loses all erased export names from the map. A 'use client' barrel using this syntax can be consumed with a normal import used only in type positions; kindsFor(target).get(name) then returns unknown and reports an offence even though TypeScript erases the binding. This syntax already exists at core/utils/diff/index.ts:1; propagate the star's names as erased and add a fixture for a client barrel.
Two directions of the same mistake — a reference the walk could not resolve to
one export was thrown away, and a name the walk never recorded was treated as
suspicious.
`require('./client')[key]` definitely reads an export and cannot say which. That
is the position a namespace binding is in, and a namespace is reported, but this
became a bare edge instead — and `clientBindings` skips a reference with neither
a name nor `namespace`, so the read was invisible. It now reports, along with the
two destructuring forms of the same thing: `{ [key]: v }`, and `{ ...rest }`,
which takes every export not named above it and was claiming an export literally
called `rest`. An array destructure stays a bare edge, because `x[0]` is not a
named export of anything, and `const {} = require(…)` gets its edge back — it
binds nothing and still loads the module, the way `import {}` does, and returning
no reference at all dropped the traversal with it.
`export type * from './x'` was skipped outright, so every name it carries was
missing from the map — and a name this cannot find reads as `unknown`, which is
reported. A client barrel written that way had its erased bindings accused of
being client values. Its names are now recorded as erased, and `export type * as
Ns` with them, which was the same omission one branch over. An unresolved
type-only star records no wildcard sentinel: that sentinel exists because an
unresolved star might be hiding a client value, and a type-only one carries
nothing at runtime.
`core/utils/diff/index.ts` is the tree's one `export type *`. It is a server
module, so nothing moves today: 407 modules, the same five offences.
Nine mutations, nine failures, one test each.
Round on 91e4ac648 — no open findings, two "previously missed". Both right, both fixed in 822cf81. Suppressed findings have no threads, so the replies are here.
They are two directions of one mistake: a reference the walk could not resolve to a single export was thrown away, and a name the walk never recorded was treated as suspicious.
Classify computed bindingless access as unknown client binding (line 140) — correct, and the argument that settles it is consistency rather than the finding's own wording. require('./client')[key] definitely reads an export and cannot say which. That is exactly the position a namespace binding is in, and this file already reports those unconditionally — "a namespace has no one export to read, and every property taken off it is a reference". Dropping the computed case to a bare edge made the same uncertainty silent two lines apart, and clientBindings skips a reference with neither a name nor namespace.
This reverses a fixture I committed last round, which asserted the bare-edge behaviour. It failed the moment the fix went in, which is the only reason I can say the fix does something.
Enumerating the class — every place requireBindings gives up on naming an export:
require('./x')[key] now reported as [computed]
const { [key]: v } = require('./x') now reported as { [computed] } (was dropped)
const { A, ...rest } = require('./x') A resolved, rest reported (claimed an export called `rest`)
const {} = require('./x') bare edge (was NO edge at all)
const [first] = require('./x') bare edge, unchanged
Two of those the finding did not name and are worse than the one it did. { ...rest } takes every export not named above it and was recorded as a named binding called rest — not silence, a wrong answer. And an all-computed destructure returned an empty array, which dropped the traversal edge along with the binding, so the client module was never even visited.
The array destructure is the one I left alone: x[0] is not a named export of anything, so a bare edge is the honest answer rather than a gap. Pinned by a mutation that routes array patterns through the object path — it then invents an export named first, and the fixture fails.
Track erased names from export type * re-exports (line 784) — also correct, and you are right that it is in the tree: core/utils/diff/index.ts:1, the only one.
grep -rn "export type \*" app atoms core design-system partials
core/utils/diff/index.ts:1:export type * from './types';
The statement was skipped outright, so every name it carries was missing from the map — and a name this cannot find reads as unknown, which is an offence. So the failure mode is the false-positive one: a client barrel written this way has its erased bindings accused of being client values, over an import TypeScript deletes.
Enumerating the class — erased export forms recorded as nothing rather than as erased. There were two, and export type * as Ns is the one the finding did not name:
export type * from './x' names recorded as erased (were absent -> unknown -> reported)
export type * as Ns from './x' Ns recorded as erased (was absent -> unknown -> reported)
One thing I did not carry across: an unresolved type-only star records no * sentinel. That sentinel exists because an unresolved value star might be hiding a client value, and silence from a guard is the failure it exists to prevent — but a type-only star carries nothing at runtime, so there is nothing for it to hide, and setting the sentinel there would invent an offence out of a type import. Mutating it to set the sentinel unconditionally fails a fixture written for that.
What changes on this tree: nothing.core/utils/diff/index.ts has no use client, and the tree's only require( is markdown-adapter's static property read. Measured rather than assumed: 407 modules, 5 offences, before and after, same as the last three rounds.
Nine mutations, nine failures, each caught by exactly one test. 125 → 133 tests. No app code touched.
Another round: no, and this one is a stopping rule rather than a judgement call. Last round I said one more and then I stop regardless, because the star restructure was the biggest change I had made. That round came back with no open findings on it, which is the answer I was asking for.
What the two suppressed findings have in common is the shape I said I would watch for: both are about syntax nobody here writes — one require read under a computed key, one client barrel using export type * — and both left the corpus at 407 modules and the same five offences, as has every fix for the last four rounds. They were worth taking, and the class enumeration around each found more than the finding did. But four rounds of that is the asymptote, and the reviewer is now finding the guard's semantics rather than the guard's bugs. Merging.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out of #2466, where this was ~25% of the diff and had nothing to do with the mobile scroll bug that PR is about.
What it guards
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 throws. That is what makes it worth a test rather than a code review note.
The case that prompted it, from #2466 — the space layout wrote
id={SPACE_TABS_ANCHOR}with the constant living inspace-tabs.tsx:An element with no id until hydration, so a fragment link had nothing to scroll to on a cold load. In the browser it looked perfect and every test passed.
How
Walks the server render graph from every
layout/page/loading/route/template/error/not-found/opengraph-image, and fails on any non-component value taken from a client module — named, default, namespace or re-exported.Two things that took a measurement rather than a guess:
fromedge reports four modules that do not exist at runtime: the sole route intocore/blocks/data/filters.tsis animport typeincore/chat/edit-types.ts, and counting it walks on into the sync store.import()and bare imports are followed, and reach nothing today. Both are recorded as traversal edges, and both are dormant: the files callingimport()are client modules reaching fornext/dynamic, and every bare import in the graph resolves to CSS or a package. That is measured, not assumed — deleting either branch leaves every corpus assertion green, which is why they are covered bycallReferencesfixtures instead.Following all edge kinds rather than named imports alone takes the guarded graph from 401 to 407 modules. The six are
default-entity-page.tsx,post-entity-page.tsx,entity-page-header.tsx,fetch-entity-page-data.ts,core/id/create-id.tsandpartials/activity/activity-layout.tsx— reached by default or namespace import, and invisible to a named-imports-only walk.What it finds today
Listed rather than fixed, each read by hand. The fix for all of them is the same shape — move the value into a module with no directive and import it from both sides.
partials/bounties/bounty-board-skeleton.tsxBOARD_GRID_CLASS,BOARD_CARD_HEIGHT_PXapp/bounties/loading.tsxis server-rendered and puts the class straight into aclassName, so the grid has no grid during the loading flashcore/responses/entity-response.tsgetCheckedcore/blocks/data/read-block-media-dimensions.tsNO_BLOCK_MEDIA_DIMENSIONScore/bounties/config.tsuseFeatureFlaguseBountiesEnabled, a client hookThe bounties one is a real bug and I have deliberately not fixed it here.
BOARD_CARD_HEIGHT_PXderives fromAVAILABLE_CARD_HEIGHT_PXin a second client module, so the fix relocates layout constants across two features and wants someone who can look at the bounties board while doing it. Happy to take it if it is not already spoken for.The list is itself checked for staleness, so an entry that gets fixed cannot sit here quietly re-permitting the same import later.
Verification
Planted
export * as Probe2 from '~/core/state/feature-flags'in the space layout: unreported by the named-imports-only version, caught by this one. Same for reverting the anchor import that motivated it.Sibling of
async-components-in-client-boundaries.test.ts, which guards the other direction of the same boundary — async components rendered from client files. Same failure mode: correct-looking UI, wrong side of the line.