Skip to content

Catalog: keep the data-products fixtures out of the volumes landing chunk - #5259

Merged
nl0 merged 1 commit into
masterfrom
stack/5217-1-data-products-lazy-adapter
Aug 31, 2026
Merged

Catalog: keep the data-products fixtures out of the volumes landing chunk#5259
nl0 merged 1 commit into
masterfrom
stack/5217-1-data-products-lazy-adapter

Conversation

@nl0

@nl0 nl0 commented Aug 31, 2026

Copy link
Copy Markdown
Member

Description

With the data-products preview off, every deployment still shipped the preview's
33 KB demo fixture corpus in the JavaScript a browser downloads for the volumes
landing. model/DataProducts/hooks is imported there through useProducts, and
it imported ./fixtureAdapter statically, so the fixtures rode into that chunk
for customers who never open a data product.

This reaches the adapter through a dynamic import instead, in the shape
Tabulator.tsx already uses for ConfigEditor. The corpus gets its own
on-demand chunk; the resource keys and the hook signatures do not change.

useAdapter goes with it: it returned the adapter synchronously, which a lazy
adapter cannot do, and it had no callers.

Verification

cd catalog
npx vitest run app/model/DataProducts app/containers/DataProducts   # 132 pass
npx tsc --noEmit                                                    # clean
npx oxlint app                                                      # clean
npm run build

After a production build the fixture corpus (acme_cohort_2024 and its siblings)
appears in exactly one async chunk — 13 KB minified, referenced from the runtime's
chunk map — and in none of the three entrypoint assets (runtime, the vendor
chunk, app).

Position in the stack

PR 1 of 9, based on master. This is the split of #5217 the review asked for
in f27,
which named the DataProducts extraction as the one worth making first: it is the
only app/model-layer change in that PR, the only [Changed], and it depends on
nothing else in the set.

TODO

  • Unit tests — covered by the existing DataProducts specs; the change is a
    call-shape change behind unchanged hook signatures
  • Security: Confirm that this change meets security best practices and does not violate the security model
  • Open: Confirm that this change doesn't break the Open variant
  • Changelog entry

🤖 Generated with Claude Code

@codecov

codecov Bot commented Aug 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 8.33333% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 34.92%. Comparing base (fe9bda5) to head (1906251).

Files with missing lines Patch % Lines
catalog/app/model/DataProducts/hooks.ts 8.33% 7 Missing and 4 partials ⚠️

❗ There is a different number of reports uploaded between BASE (fe9bda5) and HEAD (1906251). Click for more details.

HEAD has 25 uploads less than BASE
Flag BASE (fe9bda5) HEAD (1906251)
lambda 14 0
py-shared 1 0
api-python 10 0
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #5259       +/-   ##
===========================================
- Coverage   55.20%   34.92%   -20.28%     
===========================================
  Files         872      741      -131     
  Lines       36766    23802    -12964     
  Branches     6429     6429               
===========================================
- Hits        20298     8314    -11984     
+ Misses      14724    13744      -980     
  Partials     1744     1744               
Flag Coverage Δ
api-python ?
catalog 34.92% <8.33%> (+<0.01%) ⬆️
lambda ?
py-shared ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nl0
nl0 marked this pull request as ready for review August 31, 2026 14:01
@nl0

nl0 commented Aug 31, 2026

Copy link
Copy Markdown
Member Author

Local code review — findings and dispositions

Ran the repo-local review over this layer (master..stack/5217-1-data-products-lazy-adapter) alongside Greptile's. Greptile reviewed the previous tip and reported no actionable findings (5/5, no inline threads), so there was nothing to reply to there. Three findings came out of the local pass; all three accepted and fixed in 3481302, 9234c1e and 3eac3bd.

1. Accepted — the retry generation made the resource key impure

ResourceCache.keyFor recomputes key(input) on release as well as on claim, and Release throws when it lands on a key nobody claimed. This branch read adapterGeneration inside key, so the key moved underneath an already-mounted reader: once the retry window elapsed, unmounting threw Release: entry does not exist out of an effect cleanup, and the entry that was claimed kept claimed: 1, which CleanUp never evicts — so an outage leaked one dead entry per resource per window for as long as the tab stayed open.

Fixed by carrying the generation in each resource's input, so a reader releases the generation it claimed and key is a pure function of its input. The new test in hooks.spec.tsx fails with exactly Release: entry does not exist without the change.

One correction to the first write-up of this finding: the throw does not reach the root error boundary and does not blank the catalog. Checked with a boundary wrapped around a surviving sibling — the boundary is not triggered and the sibling stays mounted. It surfaces as an uncaught error, so the cost is a Sentry event per occurrence plus the leak. Worth fixing, but not the catalog-blanking failure unavailableAdapter guards against.

2. Accepted — the retry was per-resource, not per-window

Resources only share adapterPromise while one import is in flight, so each consumer reading during an outage drove its own failure, its own retry and its own Sentry report — the doc comment's "one retry per window" was not true. Measured against a deliberately broken chunk with three mounted readers over five simulated minutes:

imports Sentry reports
as pushed 25 25
timer + subscription (rejected) 118 118
shipped 3 1

The middle row is why the generation is advanced lazily on read rather than on a timer. A timer has to notify its readers to be worth anything, and notifying them re-renders, which re-reads, which retries the import, which fails, which schedules another. Read on the way past, the retry still arrives with the next read, which is what the window promises. The report is deduplicated per generation, and a successful load clears the pending window so the generation stops moving.

3. Accepted — bundling.spec.ts could not see the import it exists to catch

The graph walk anchored its pattern to the whole import declaration, so it could not cross newlines and missed every multi-line import/export … from. It also followed only ./ specifiers. Both holes admit the exact regression the file guards:

  • a multi-line import {⏎ fixtureAdapter,⏎} from './fixtureAdapter' in hooks.ts left both assertions green with the fixture corpus statically reachable from the barrel again;
  • ./capabilities — re-exported multi-line from the barrel — was already invisible to the walk, so that subtree was never checked at all.

Fixed by matching the from clause on its own (no dynamic import() has one) and counting an absolute model/DataProducts/… self-reference as an edge. The walk is now pinned directly, since its failure mode is to under-report edges silently rather than to error. Verified by injecting each form into hooks.ts: the old spec caught neither, the new one fails on both.

Also swept

Comments that restate the code beside them rather than carrying a constraint the code cannot show: the stand-in's list-versus-product aside (ProductResult draws it more precisely), "loud rather than lying" next to the throw it describes, Unavailable's account of its own ink and missing remedy line, and two test comments restating their assertions. Left dense otherwise — this module runs 49–69% comment lines on master, so a blanket trim here would read as an outlier rather than as restraint.

Checked and left alone

  • unavailableAdapter.getProduct throwing — genuinely unreachable behind the isUnavailable check; kept as an assertion rather than softened.
  • supportsBrowsing/supportsFetching with the stand-in — correctly routes to the existing NOT_FOUND path, so no call site needs a new branch.
  • useAdapter removal — no remaining callers, and supportsRequests is still exercised through the adapter port directly, so it is not a dead export.
  • Detail's early return — no hooks follow it, and Empty accepts title plus children.
  • A claim from the local pass that the absolute model/DataProducts/fixtures style is used "in several containers": checked, and every absolute importer is a .spec file, which never ships in a bundle. The hole in the walk was real regardless, so the hardening stands on the multi-line case.

Verification

cd catalog && npx vitest run app/model/DataProducts app/containers/DataProducts
npx tsc --noEmit
npx oxfmt --check ./app && npx oxlint ./app

140 tests pass; typecheck, format and lint clean. Both new pins were checked against the unfixed code, not just observed green.

Layers 2–9 were rebased onto this chain in order and force-pushed with --force-with-lease; each PR's changed-file count still matches its own layer.

`model/DataProducts/hooks` is on the volumes landing's import path through
`useProducts`, so its static `fixtureAdapter` import put the whole 36K demo
fixture corpus in the chunk every deployment downloads there -- including the
ones with the `data-products` preview off, which never read it.

Reach the adapter through a dynamic import instead, in the shape
`Tabulator.tsx` already uses for `ConfigEditor`. The corpus gets its own
on-demand chunk; the resource keys and the hook signatures do not change.

`useAdapter` goes with it: it returned the adapter synchronously, which a lazy
adapter cannot do, and it had no callers.
@nl0
nl0 force-pushed the stack/5217-1-data-products-lazy-adapter branch from 3eac3bd to 1906251 Compare August 31, 2026 17:08
@nl0

nl0 commented Aug 31, 2026

Copy link
Copy Markdown
Member Author

Trimmed to the minimal change

Ruled over-built on read: too much machinery, tests and comments for a change
that does not reach a production code path. Rebuilt from master as a single
commit.

commits files diff
before 8 7 +541 / −50
after 1 3 +24 / −25

hooks.ts is now 209 lines against master's 210 — the fix is net negative.
The whole PR is the static import becoming a thunk, each fetch reaching it
through .then, and useAdapter going away because a lazy adapter cannot be
returned synchronously.

What came out

  • The retry/generation machineryRETRY_ADAPTER_AFTER, adapterGeneration,
    retryAt, reportedGeneration, currentGeneration(), and the generation
    threaded through all six resource keys and all six hooks. This was the bulk of
    it: indirection whose only purpose was to make a failed chunk fetch retryable
    through a cache with no eviction.
  • unavailableAdapter and isUnavailable, the stand-in that produced the
    empty answers the generation existed to expire.
  • ProductResult and its ripple: useProduct returns DataProduct | null
    again, so Detail.tsx and DataProducts.spec.tsx revert to master entirely.
  • bundling.spec.ts (128 lines) and hooks.spec.tsx (167 lines) — 295 lines
    carrying 8 test cases, all of them guarding the machinery above rather than the
    bundling change.
  • The Sentry/console reporting path, and the doc comments that narrated the
    machinery rather than constraining it.
  • The [Removed] useAdapter changelog line: an unused internal hook is not a
    user-facing change, and the remaining [Changed] entry is reworded to drop the
    failure-reporting claim that no longer holds.

The trade this makes — worth a maintainer's eye

The deleted machinery guarded a real state: if the adapter chunk cannot be
fetched, the read now rejects, and utils/ResourceCache latches a rejection and
rethrows it on every later read without evicting the entry.

I am letting that stand, for three reasons:

  1. With the preview off, enabled: false resolves to [] without calling the
    loader, so the chunk is never fetched. The failure is unreachable on the path
    this PR is about.
  2. With the preview on, this is the same exposure every other dynamic import
    in the catalog already carries — Bucket, Admin, the preview renderers,
    ConfigEditor — none of which have retry or stand-in handling. Adding it here
    only would be inconsistent, not safer.
  3. Making a latched ResourceCache entry recoverable is the underlying fix
    (previously recorded as f10), and it belongs in utils/ResourceCache
    rather than in one of its callers.

If you would rather the containment ship with the bundling change, say so and it
comes back as its own PR on top — but it should not ride in as a side effect of
moving an import.

The earlier local-review comment above is now moot

All three of its accepted findings governed code that no longer exists: the
impure resource key (finding 1) and the per-resource retry (finding 2) were
properties of the generation machinery, and finding 3 hardened bundling.spec.ts.
Nothing to carry forward. Greptile's summary in the body described the previous
tip for the same reason and was dropped; the PR is re-triggerable if a fresh pass
is wanted.

Verification

cd catalog
npx vitest run app/model/DataProducts app/containers/DataProducts   # 132 pass
npx tsc --noEmit                                                    # clean
npx oxlint app                                                      # clean
npm run build

The bundling claim is checked against the build rather than asserted in a spec:
after a production build the fixture corpus appears in exactly one async chunk
(13 KB minified, referenced from the runtime's chunk map) and in none of the three
entrypoint assets. The import() is the only edge into fixtureAdapter, which is
the only edge into fixtures.

Stack

Layers 2–9 were rebased onto this chain in order and force-pushed with
--force-with-lease. The old-tip-to-new-tip delta is byte-identical to this
layer's own before/after delta, so nothing above changed except by inheriting the
trim; each PR's base and changed-file count still match its own layer, every
layer typechecks, its own specs pass, and the stack tip is green at
181 files / 1716 tests.

@nl0
nl0 enabled auto-merge August 31, 2026 18:53
@nl0
nl0 disabled auto-merge August 31, 2026 18:53
@nl0 nl0 changed the title Catalog: keep the data-products fixtures out of the volumes landing chunk (5217 stack 1/9) Catalog: keep the data-products fixtures out of the volumes landing chunk Aug 31, 2026
@nl0
nl0 merged commit 41a0243 into master Aug 31, 2026
43 of 45 checks passed
@nl0
nl0 deleted the stack/5217-1-data-products-lazy-adapter branch August 31, 2026 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant