fix(ui-core): the gallery's ResizeObserver is attached through a callback ref (#159) - #167
Merged
Merged
Conversation
…back ref (#159) The asset gallery rendered one tile per row at every viewport width, for the life of the screen. Resizing the window did nothing. The rows were laid out correctly — `flex w-full`, 1239px wide — and each held exactly one tile. `useColumns` initialised `columns` to 1 and attached its `ResizeObserver` in an effect that began `if (element === null) return`. The scroller `scrollRef` points at is rendered **inside `<Async>`'s children render-prop**, so on mount `<Async>` is still showing skeleton rows and `ref.current` is null: the effect took the early return. Both dependencies were stable — `ref` is a `RefObject`, `measure` a `useCallback` over `[ref]` — so it never ran again once the real element mounted. The arithmetic was never wrong. `Math.floor((1239 + 12) / (160 + 12))` is 7. Only the measurement never happened, which is why every test agreed with it. A ref object mutating is invisible to React; a **callback ref** is not — React calls it with the node on attach and with `null` on detach, so an effect keyed on that state re-runs at exactly the two moments that matter. `useVirtualizer` now reads the same state value through `getScrollElement`, so the two halves of the screen cannot disagree about which element they are measuring. The contrast inside the file was the tell all along: rows virtualised correctly the whole time columns did not, because `getScrollElement` is a callback that gets re-read. **The guard is in a browser, and it has to be.** jsdom reports every element as 0x0, so the virtualizer renders no rows there and `useColumns` falls back to one column — which the old docstring called "correct-but-slow rather than wrong" and the unit tests then asserted as if it were the intended layout. That is this milestone's root pattern: a claim verified against itself. So `cycle/cycle.spec.ts` measures the real thing at two viewport widths and asserts the rendered count agrees with what fits, is more than one when several fit, and follows the window when it narrows. Verified by mutation, both halves independently: restoring the empty dependency array renders 1 where 3 fit, and dropping the observer while keeping the callback ref passes the wide case and fails the re-flow. What survives in jsdom is only what is honest there: `columnsFor` is exported and its arithmetic pinned, and one test states that the one-column fallback is reached because the observer is genuinely absent. Frontend only: no Python, no migration, `openapi.json` and the generated client byte-identical, `FORMAT_VERSION` still 12. Closes #159
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #159. Third of the
0.0.1-beta.2defect fixes.The defect
The asset gallery rendered one tile per row at every viewport width, for the life of the screen, and resizing the window did not change it. Six 160-px thumbnails occupied a single 160-px column of a 1239-px pane.
useColumnsattached itsResizeObserverin an effect that beganif (element === null) return. The scrollerscrollRefpoints at is rendered inside<Async>'s children render-prop, so on mount<Async>is showing skeleton rows andref.currentisnull— the effect took the early return. Both dependencies were stable (refis aRefObject,measureauseCallbackover[ref]), so it never ran again once the real element arrived.columnsstayed at its initial1.The arithmetic was never wrong —
columnsFor(1239)is 7 and always was. Only the measurement never happened, which is exactly why a suite full of assertions never saw it.The fix
A ref object mutating is invisible to React. A callback ref is not: React calls it with the node on attach and with
nullon detach, so an effect keyed on that state re-runs by construction at the two moments that matter.useVirtualizernow reads the same state value throughgetScrollElement, so the two halves of the screen cannot disagree about which element they are measuring. The contrast inside the file was the tell all along — rows virtualised correctly the whole time columns did not, becausegetScrollElementis a callback that gets re-read.The guard, and why it cannot be a unit test
jsdom reports every element as 0×0. The virtualizer therefore renders no rows at all there,
useColumnsfalls back to one column, and the screen's own docstring called that "correct-but-slow rather than wrong" — sogallery.test.tsxwas running in precisely the state the bug produced and passing. That is this milestone's root pattern: a claim verified against itself.So the column count is asserted in a real browser, in
frontend/app/cycle/cycle.spec.ts(#59's real-server config, #48's harness — no new runner):Verified by mutation, both halves independently:
[]deps (the original shape)toBeGreaterThan(1)toBeLessThan(3)What stays in jsdom is only what is honest there:
columnsForis exported and its arithmetic pinned (includingcolumnsFor(0) === 1), and one test assertsglobalThis.ResizeObserveris genuinelyundefinedbefore checkingdata-columns="1"— so the fallback path stays a stated fallback instead of quietly hiding the next regression.Ledger
openapi.jsonand the generated client byte-identical;FORMAT_VERSIONstill 12.browser cycle (chromium)already runs this spec.Checks run locally
pnpm --filter @visionset/app cycle— 1 passedpnpm test— 711 annotator + 113 ui-core vitest (was 111), 22 node:testpnpm -r build,pnpm -r lint,pnpm -r typecheck— clean