Skip to content

Release hardening: five catalog fixes from the master-vs-26.7.4 review - #5217

Closed
QuiltSimon wants to merge 18 commits into
masterfrom
fix/release-hardening-catalog
Closed

Release hardening: five catalog fixes from the master-vs-26.7.4 review#5217
QuiltSimon wants to merge 18 commits into
masterfrom
fix/release-hardening-catalog

Conversation

@QuiltSimon

@QuiltSimon QuiltSimon commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Fixes the five live, unflagged catalog findings from tonight's hardening review of master (6167dd82) vs the 26.7.4 pin (eda3016f).

Fixes

  1. Athena: ui.athena.defaultWorkgroup honored again. The workspace re-home dropped BucketPreferences from the query console, so the preference silently died — customers landed on the alphabetically-first workgroup. Every legacy /b/:bucket/queries URL redirects here with ?bucket= set; that bucket's ui.athena now threads back into the model, restoring exactly the scope the preference applied to before. New Athena.spec.tsx pins the wiring (2/3 cases fail against the unfixed source).
  2. Search: Order-by control no longer vanishes mid-typing. The server-side filter path passed the post-filter facet count as totalAvailable; now passes the pre-filter count like the other three paths.
  3. Search: Order-by select has an accessible name. aria-labelledby moved from inputProps (lands on the aria-hidden native input) to SelectDisplayProps. Spec written first and watched fail.
  4. QuerySelect: accessible name + no false "Custom". labelId wires the label to the focusable node; the display is blank (not "Custom") when error is set, since Athena nulls the value on a failed load. Snapshots gain the label id; useId is now pinned deterministically in the spec.
  5. Admin: the disabled Enabled switch explains itself. Cause-specific tooltip (self vs service user) on a live wrapper span; the roles dialog gates on isService directly instead of trusting the registry to couple it to isRoleAssignmentDisabled. Verified by mutation: flipping the branch fails the spec.
  6. Data products: fixture corpus out of the flag-off bundle. hooks.ts statically imported the fixture adapter, dragging 33KB of demo product records into chunks every flag-off customer downloads. The adapter now loads via dynamic import. Verified against a production build: fixture strings appear in exactly one async chunk, zero in app/runtime/vendor entries, not preloaded by index.html.

Verification

  • Full suite: 173 files, 1620 passed, 1 skipped (TZ=UTC), up 8 tests from baseline.
  • tsc --noEmit clean; oxfmt/oxlint clean via lint-staged.
  • Every behavioral fix has a test that was watched to fail against the unfixed source (spec-first, stash-revert, or mutation).

Dismissed in writing

  • No component test for fix 2 (totalAvailable): the internal render-prop components have no test seam short of a full GQL+model harness; the module's convention is pure-function specs. The invariant is now stated at the call site and in AvailablePackagesMetaFiltersGroup's prop doc.

🤖 Generated with Claude Code

Greptile Summary

This PR hardens several Catalog behaviors across Athena preferences, search ordering, query accessibility, administrative controls, and data-product bundle loading.

  • Restores bucket-scoped Athena workgroup preferences.
  • Stabilizes search ordering controls and supplies accessible select labels.
  • Corrects QuerySelect labeling and error-state display.
  • Adds explanatory tooltips for disabled administrative switches.
  • Lazily loads the data-product fixture adapter to keep fixtures out of initial bundles.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
catalog/app/containers/Admin/UsersAndRoles/Users.tsx Adds cause-specific disabled-control tooltips and documents the service-user role-selector gate; the previously reported registry-refusal scenario is not reachable.
catalog/app/containers/Queries/Athena/Athena.tsx Restores bucket-scoped preference loading before mounting the Athena model.
catalog/app/containers/Queries/Athena/model/state.tsx Threads optional Athena preferences into workgroup selection.
catalog/app/containers/Queries/QuerySelect.tsx Associates the select with its visible label and avoids displaying “Custom” during load failures.
catalog/app/containers/Search/Layout/PackageFilters.tsx Places the ordering control’s accessible-name attribute on the focusable select display.
catalog/app/containers/Search/model.ts Uses the pre-filter package count so ordering availability remains stable during server-side filtering.
catalog/app/model/DataProducts/hooks.ts Replaces the static fixture-adapter dependency with a cached dynamic import across resource reads.

Reviews (2): Last reviewed commit: "fix(admin): explain the disabled Admin s..." | Re-trigger Greptile


Update 2026-08-26: retargeted to master, /code-review complete

Base changed dev → master per release direction; the branch was rebased and the search fixes re-authored against #5222 (which replaced the two-axis ordering controls with one "Sort by" select after this PR was cut — both bugs survived the rewrite verbatim). Sergey's two commits are preserved on the rebased branch.

/code-review on the final state returned 10 findings — 6 fixed in bc73eb966, 4 dismissed:

Fixed: the roles dialog's live Save beside a frozen selector (one readOnly expression now gates title/selector/actions); the Admin column's missing isService guard; loadAdapter latching a rejected chunk import permanently; totalAvailable inverting the vanishing-control bug on the truncated path (max(initial, available)); useAdapter deleted (no callers, and a suspending adapter read is the wrong shape for gating an affordance); CHANGELOG entries added.

Dismissed in writing:

  1. localStorage outranks defaultWorkgroup — the same precedence exists verbatim in 26.7.4 (Bucket/Queries/Athena/model/requests.ts:127 at eda3016f); this PR restores the preference to the code path, and reordering decade-old precedence for returning users is a product decision, not hardening.
  2. Prefs fetch gates the scoped console / remount on scope change — the gate is the old per-bucket behavior restored (old Wrapper matched _ → Placeholder identically); workgroup switches preserve ?bucket= (Workgroups.tsx:40), so the remount fires only on genuine global↔scoped navigation.
  3. Duplicate ?bucket= extraction — two commented call sites; extract a useBucketScope() when a third appears.
  4. Dead no-reason branch in EditableSwitch — the type-level fix forces call-site restructuring that doesn't belong in a hardening PR; the branch is defensive for future callers and spec-pinned.

Full suite after everything: 175 files, 1632 passed, 1 skipped (TZ=UTC).

@codecov

codecov Bot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 78.65169% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.32%. Comparing base (213766a) to head (491639d).

Files with missing lines Patch % Lines
catalog/app/model/DataProducts/hooks.ts 59.25% 9 Missing and 2 partials ⚠️
...talog/app/containers/Admin/UsersAndRoles/Users.tsx 80.64% 4 Missing and 2 partials ⚠️
catalog/app/containers/Queries/Athena/Athena.tsx 90.90% 1 Missing ⚠️
catalog/app/containers/Search/model.ts 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5217      +/-   ##
==========================================
+ Coverage   54.55%   55.32%   +0.77%     
==========================================
  Files         871      871              
  Lines       36448    36502      +54     
  Branches     6438     6446       +8     
==========================================
+ Hits        19883    20196     +313     
+ Misses      14807    14557     -250     
+ Partials     1758     1749       -9     
Flag Coverage Δ
api-python 93.82% <ø> (ø)
catalog 35.68% <78.65%> (+1.23%) ⬆️
lambda 85.37% <ø> (ø)
py-shared 98.02% <ø> (ø)

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.

@sir-sigurd

Copy link
Copy Markdown
Member

Greptile's P1 doesn't hold — the "registry then refuses" step is false on every registry version.

On current registry code isRoleAssignmentDisabled is defined to include service users, so isService=true implies isRoleAssignmentDisabled=true (pinned by a test). The decoupled state described here never reaches the catalog.

On older registries the two flags were decoupled, but there the role-assignment mutations gate on isRoleAssignmentDisabled alone with no service-user check — they accept the submission. So there is no refusal and no "Something went wrong" on either vintage.

The inconsistency it points at is real, though: the dialog title and actions still key off isRoleAssignmentDisabled alone while the selector now also gates on isService. That yields at most an enabled no-op Save, in a state no registry produces — so I left the logic alone and corrected the comment above it, which claimed the gate suppresses that Save.

I also pushed the same disabledReason treatment to the Admin column, which had kept the unexplained disabled switch that fix 5 removes from Enabled — with tests at the column's own getDisplay, watched red first.

@greptileai please review the new head.

smkohnstamm and others added 7 commits August 26, 2026 09:03
…sole

The workspace re-home dropped BucketPreferences from the query console, so
useWorkgroup's preferences param went dead and customers with
ui.athena.defaultWorkgroup landed on the alphabetically-first workgroup.
Every legacy /b/:bucket/queries URL redirects with ?bucket= set — resolve
that bucket's preferences and thread ui.athena back into the model, exactly
the scope the preference applied to before the re-home.

Co-Authored-By: Claude <noreply@anthropic.com>
Two fixes to the metadata-ordering controls:

- The server-side filter path passed the post-filter facet count as
  totalAvailable, so typing in "Find metadata" on a truncated-facet stack
  made the Order-by control vanish once matches dropped below the offer
  threshold. Pass the pre-filter count, matching the other three paths and
  the documented invariant.

- The Order-by select's aria-labelledby sat in inputProps, which MUI spreads
  onto the aria-hidden native input — the focusable node had no accessible
  name. Moved to SelectDisplayProps. Spec renders the block and asserts both
  selects name their focusable node; the Order-by case fails without the fix.

Co-Authored-By: Claude <noreply@anthropic.com>
The InputLabel had no id and the Select no labelId, so the focusable
role=button node had no accessible name — a screen reader announced
"Custom, button" with no hint it is the query selector.

Athena also passes value={null} for the error state, so the field read
"Custom" (asserting a hand-written query is loaded) beside a red helper
saying the load failed. Render blank when error is set.

Snapshots gain the label id; useId is Math.random-based, so the spec pins
it deterministically now that an id renders unconditionally.

Co-Authored-By: Claude <noreply@anthropic.com>
…ice locally

The disabled branch of EditableSwitch dropped the tooltip entirely, so the
service-user and this-is-you cases rendered as an identical dead control with
no cause. Wrap the disabled switch in a cause-specific tooltip (a disabled
element fires no events, so it needs a live wrapper span).

Also gate the roles dialog on isService directly instead of trusting the
registry to couple isRoleAssignmentDisabled to it — a registry that reports
isService alone would otherwise offer a Save it then refuses.

Co-Authored-By: Claude <noreply@anthropic.com>
hooks.ts statically imported fixtureAdapter, which imports the 33KB fixture
corpus. The volumes landing calls useProducts, so every flag-off customer
downloaded and evaluated demo product records ("Clinical Cohort 2024",
"acme_cohort_2024") on first load of /, readable in devtools.

Load the adapter through a dynamic import so webpack splits it out; the
flag-off resource keys resolve to [] without consulting it, so the chunk is
never fetched. useAdapter keeps its synchronous contract by suspending on
the same load.

Verified against a production build: fixture strings appear in exactly one
async chunk, absent from app/runtime/vendor entries and not preloaded by
index.html.

Co-Authored-By: Claude <noreply@anthropic.com>
The comment claimed the local `|| isService` stops the dialog offering a Save
the registry then refuses. It doesn't: the dialog title and actions still read
isRoleAssignmentDisabled alone, so only the selector is gated.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Enabled column now says why its switch is dead; the Admin column beside it
kept the unexplained one. Same treatment, with the three causes the catalog can
actually tell apart: yourself, a service user, an SSO-managed account.

The tests render the column's own getDisplay rather than EditableSwitch in
isolation — a reason passed at one call site and not its sibling is exactly what
this fixes, so only the call site proves it. Watched red against the unfixed
source.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@QuiltSimon
QuiltSimon force-pushed the fix/release-hardening-catalog branch from d189402 to 2beda22 Compare August 26, 2026 14:12
@QuiltSimon
QuiltSimon changed the base branch from dev to master August 26, 2026 14:12
@QuiltSimon

Copy link
Copy Markdown
Contributor Author

@sir-sigurd I'm re-reviewing this based on your comments, thanks for your review. I'll repost it to you once I've done a /code-review for us to merge into Master.

- EditRoles: one readOnly expression for title, selector, and actions — the
  half-applied isService gate left a live Save beside a frozen selector.
- Admin column: same isService guard as the Enabled column and roles dialog.
- loadAdapter: never latch a rejected chunk import; retry on next read.
- totalAvailable (server path): max(initial, available) — initial alone
  withheld the control when a text query out-returned the truncated list.
- Drop useAdapter/AdapterResource: no callers, and a suspending adapter read
  is the wrong shape for gating an affordance.
- CHANGELOG entries for the user-visible fixes.

Co-Authored-By: Claude <noreply@anthropic.com>
@QuiltSimon
QuiltSimon requested a review from sir-sigurd August 26, 2026 14:49
smkohnstamm and others added 10 commits August 26, 2026 10:45
…ng-catalog

# Conflicts:
#	catalog/CHANGELOG.md
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n isService

Derive each switch column's `disabled` from its reason so the guard and the
explanation cannot drift, and apply the dialog's read-only pair to RoleDisplay.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Make the offer monotonic per mount and withhold it while nothing is displayed,
so every call site answers visibility through one helper.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ject

The parsed preferences are rebuilt every provider render, re-firing the
workgroup effect throughout the probe fan-out.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…directs

Only the root shape promoted the bucket, so a legacy workgroup or execution URL
landed in the console unscoped and lost the bucket's ui.athena preferences.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@nl0 nl0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review pass — #5217

Reviewed at commit 491639d4cc90. This is a projection of the review case; the case is the record.

This is an advisory review pass (pass 7) over the change as it stands at 491639d4cc902f0afa7723213ae24f6b9a89ef8f — a comment rather than a verdict: while the posture on this case is advisory the pass may only COMMENT, and a converged, blocker-free gate means this pass is fit to post, never that the pull request is fine. It was made from that commit's own diff; the case's related set parsed to zero references again this pass, so no sibling pull request was read alongside it, and eleven lanes ran over the whole diff — ten in the reviewing lineage plus one second-lineage read from outside it.
Ten findings are carried below against a rank cap of ten: six are anchored as inline comments, and four are named in the body instead — three because the site each one is about lies between hunks or on a line this diff does not carry, and one because its ledger entry never recorded a usable locus. The cap cut eight more, which stay on the case's ledger rather than being dropped: three test-coverage gaps on the change's own stated claims, one undisclosed rider, one entry re-classed to the conventions band, one credit, and two whose earlier park reasons had expired and were re-ranked on merit and cut again. This pass also records a depth limit of its own: 894 added lines across 22 files and six unrelated subsystems is past what one pass reads to depth, and the production-build chunk claim, the MUI v4 internals behind the two accessibility fixes, the ?bucket= scope's interaction with ?table= tabulator deep links, and the registry-side coupling the admin guards assume were not gone into.

What ran. diff-correctness (claude/opus-5+sonnet-5), removed-behavior (claude/opus-5+sonnet-5), cross-file (claude/opus-5+sonnet-5), cleanup (claude/opus-5+sonnet-5), altitude (claude/opus-5+sonnet-5), conventions (claude/opus-5+sonnet-5), scope (claude/opus-5+sonnet-5), optionality (claude/opus-5+sonnet-5), intent (claude/opus-5+sonnet-5), description-conformance (claude/opus-5+sonnet-5), second-lineage (codex/gpt-5.6-sol)

Posted below. 6 finding(s) as inline comments: f12 (substantive), f27 (substantive), f30 (substantive), f31 (minor), f36 (minor), f45 (minor).

Open, not anchored here. These stand open on the case and could not be anchored as inline comments at this commit — a review comment can only sit on a line the diff of this pull request actually carries, and a finding whose site is elsewhere is stated here with its locus rather than dropped:

  • f9 (substantive, second-lineage) — span-crosses-hunks at locus catalog/app/model/DataProducts/hooks.ts:60-159: FAILURE SCENARIO: if the first useProducts(true) load hits a transient chunk failure, the import handler resolves to unavailableAdapter and ResourceCache stores [] under the true key; later product-list reads return that cached empty result without calling loadAdapter, even after the chunk becomes available, and the per-id resources similarly cache fallback misses. The adapterPromise = null reset therefore does not restore the feature within the session.
  • f16 (substantive, second-lineage) — span-crosses-hunks at locus catalog/app/containers/Queries/Athena/model/requests.ts:207-216: FAILURE SCENARIO: with stored workgroup 'deleted', bucket default 'team' and accessible workgroups 'alpha' and 'team', the || selects 'deleted'; after that fails validation the code chooses 'alpha' rather than trying 'team' — so the ui.athena.defaultWorkgroup preference this PR exists to restore is silently skipped whenever a stale localStorage value is present.
  • f33 (substantive, removed-behavior) — no-locus at locus : requests.ts:208 preserves storage.getWorkgroup() || defaultWorkgroup exactly, so the preference is consulted only when localStorage is empty; Workgroups.tsx:52 writes storage.setWorkgroup on every workgroup switch, so any user who has ever changed workgroup still never sees that bucket's ui.athena.defaultWorkgroup — narrower than the CHANGELOG's 'honors that bucket's ui.athena.defaultWorkgroup again'. requests.spec.ts:1263 exercises the preference only with the storage mock returning nothing, and the new Athena.spec.tsx asserts only that the value is threaded into Model.Provider, so no test pins the precedence either way. FILED AS A WIDENING by the lane: it is ground outside the delta the lane was scoped to grade.
  • f34 (substantive, diff-correctness) — line-not-in-diff at locus catalog/app/containers/Queries/Athena/model/requests.ts:201-212: listIncludes lowercases both sides, but setData stores the CALLER'S casing instead of the canonical name already present in workgroups.data.list. FAILURE SCENARIO: an admin writes ui.athena.defaultWorkgroup: Analytics-Prod while the Athena workgroup is named analytics-prod — line 209 matches, line 210 sets workgroup.data='Analytics-Prod', and that string is passed straight to athena.listNamedQueries({WorkGroup}) at line 281 and startQueryExecution({WorkGroup}) at line 925, which AWS rejects with InvalidRequestException. The console therefore ERRORS instead of honoring the default, and the same hole applies to a mis-cased :workgroup URL segment at line 202. This is the case-insensitive match introduced by this diff meeting an AWS API that is case-sensitive.

Parked. Real findings this pass is not the pass for — non-gating, still on the case:

  • f2 (substantive, scope) — below the cap at pass 6 — ranked 13 of 18; scope-and-intent band, substantive after this round's regrade on six independent lane reports. Parked at the lowest cost of any cut in this set: its locus and its scenario are both delivered inside f31, which posts and names Queries.tsx:80 verbatim ('clicks the Athena tab (Queries.tsx:80 links to urls.queriesAthena() with no search)'). The reader therefore sees the unfixed navigation site; what waits is the scope lane's framing of it as the fix stopping short.
  • f3 (substantive, intent) — below the cap at pass 6 — ranked 14 of 18; scope-and-intent band. Re-parked with BOTH loci of its rule-class now on the entry, which is why it was un-parked this round: body.md:6 (fix 2 claims 'passes the pre-filter count', ships Math.max(initial.length, available.length), whose own comment at model.ts:1302 says neither count is the pre-filter total) AND body.md:9 (fix 5 claims the roles dialog gates 'on isService directly instead of trusting the registry to couple it to isRoleAssignmentDisabled', while Users.tsx:660 and :876 both ship 'user.isRoleAssignmentDisabled || user.isService' — an OR alongside, not a replacement). Both verified by me at this head. Author-owned in substance; the second locus had no home before this park cause.
  • f4 (substantive, intent) — below the cap at pass 1 — ranked 13 of 28; scope-and-intent band. CONFIRMED: the fix claimed for one path reaches all four through AvailablePackagesMetaFiltersGroup.
  • f5 (substantive, altitude) — below the cap at pass 1 — ranked 23 of 28; altitude band. CONFIRMED: ordering visibility patched twice instead of at the count the server already answers.
  • f6 (minor, scope) — below the cap at pass 1 — ranked 19 of 28; scope-and-intent band, minor: a test-seam gap, and the diff creates the very seam it says it lacks.
  • f7 (substantive, intent) — below the cap at pass 1 — ranked 14 of 28; scope-and-intent band. CONFIRMED: the dismissal's stated substitute for a test is a prop doc this same diff falsifies.
  • f8 (minor, altitude) — below the cap at pass 1 — ranked 27 of 28; altitude band, minor after this round's regrade. Reopened from a refutation that did not reach it, then ranked and parked — visible, which is the whole point of the reopen.
  • f10 (substantive, altitude) — below the cap at pass 1 — ranked 24 of 28; altitude band. CONFIRMED: ResourceCache's latched-error defect worked around in a third consumer; the correctness half of the same mechanism posts as f9.
  • f11 (substantive, intent) — below the cap at pass 6 — ranked 12 of 18; top of the scope-and-intent band, which correctness outranks. CONFIRMED and substantive: fix 6 is stated as a bundling change while a new silent-degradation state ships with it. The cost of parking it is reduced, not zero: its correctness halves DO post as f45 (getProduct reports a transport failure as a missing product, CONFIRMED at r2) and f9 (the fallback value is cached, so the retry does not restore the feature), so the reader gets the mechanism even though the undisclosed-claim finding waits.
  • f13 (substantive, intent) — below the cap at pass 1 — ranked 11 of 28, first out; top of the scope-and-intent band, which correctness outranks. CONFIRMED and substantive: legacy /b/:bucket/queries/athena deep links land somewhere new and no claim or CHANGELOG line says so. First finding to post at pass 2 if the top ten close.
  • f17 (substantive, scope) — below the cap at pass 1 — ranked 12 of 28; scope-and-intent band. CONFIRMED, and the only entry citing the repository's own .github/pull_request_template.md rule; two documents (docs/Catalog/Preferences.md:97,145 and Athena/spec.md:32,60,87) still describe the pre-change behavior.
  • f18 (substantive, altitude) — below the cap at pass 1 — ranked 25 of 28; altitude band. CONFIRMED, and it names an unfixed next case (Athena/Workgroups.tsx:64-66) against WCAG 2.1 SC 4.1.2.
  • f19 (substantive, altitude) — below the cap at pass 1 — ranked 26 of 28; altitude band. CONFIRMED: a preference object flattened at the consumer to dodge an unmemoized shared provider.
  • f21 (minor, scope) — below the cap at pass 1 — ranked 20 of 28; scope-and-intent band, minor: the breadcrumb half of the fix has no test.
  • f22 (substantive, description-conformance) — below the cap at pass 1 — ranked 17 of 28; author-owned. CONFIRMED: the need is pitched at review-process altitude and cites two bare commit hashes rather than a linked upstream.
  • f23 (minor, description-conformance) — below the cap at pass 1 — ranked 21 of 28; author-owned, minor: Verification reports outcomes rather than a check a reviewer can run.
  • f24 (substantive, description-conformance) — below the cap at pass 7 — ranked 18 of 18; author-owned. Un-parked THIS round because f39's accepted deviation rests on this finding having been 'delivered to the author', which seven passes of parking have made false. Re-ranked and cut: the ranking is bands and concreteness, and a one-line missing link in the Links section does not outrank ten live defects however much leverage it carries. SO THE RULING RECORDS THE CONSEQUENCE INSTEAD OF HIDING IT, and the next pass owes one of the two: either f24 takes a slot, or f39 is reopened, because an acceptance conditioned on a delivery the cap keeps denying is not an acceptance — it is the review deciding, silently and every pass, that it will never ask for the one thing that would let it read the cross-repository seam. Nothing in the code changes this: only the author can add the link, and only the review can ask.
  • f26 (substantive, description-conformance) — below the cap at pass 1 — ranked 16 of 28; author-owned, delivered not withheld (pr-review.md Ruled 2026-08-27). CONFIRMED here on reopen against the mechanical checker; privacy hits 0, so nothing needed scrubbing and the kind's top-of-set rule for a private reference does not fire.
  • f28 (minor, cleanup) — below the cap at pass 1 — ranked 28 of 28; cleanup, minor: a new test reinvents an empty facet tree the model already exports.
  • f29 (substantive, conventions) — below the cap at pass 6 — ranked 16 of 18; head of the cleanup/conventions/altitude band, which the ranking places below correctness and below scope-and-intent. Un-parked and re-examined this round on genuinely harder evidence, and the entry now carries it: catalog/CLAUDE.md:14-16 BINDS DESIGN.md onto code changes ('UI changes conform to it; when a change genuinely needs the contract to move, update DESIGN.md in the same diff'), DESIGN.md:245-248 states the Focus Ring Rule and :401-402 restates it as a Do, style.js:68-71 implements it for MuiButtonBase ONLY, the span at Users.tsx:815 carries no className, and six existing sites set the ring by hand. All five quotations verified verbatim by me at this head. I record the tension the pass-1 park already named and did not resolve: this is the strongest citation in the set and a present-tense accessibility defect on an element THIS diff adds inside a unit whose fix 5 is itself an accessibility fix, yet the band ordering is what it is and correctness took all ten slots. It is re-parked on rank, not on merit.
  • f32 (minor, cross-file) — below the cap at pass 7 — ranked 17 of 18. Un-parked THIS round because the previous park's own condition ('first to post at the next pass') came due, re-ranked on its merits, and cut again — and the promise is retired rather than repeated, because a park cause that schedules a future post is a commitment the ranking cannot honour in advance. The rank is honest and it is the band rule doing the work: this is the correctness band's ONLY non-CONFIRMED entry, graded PLAUSIBLE because the registry computing isService/isAdminAssignmentDisabled is outside this repository and this pass's related set parsed to zero references, so nothing this pass could run would confirm or refute it. Every one of the ten above it is CONFIRMED. It re-enters the ranking the moment the registry side becomes readable — i.e. the moment f24 is delivered and a related unit comes down — and that dependency is now recorded on f24 as well as here.
  • f41 (substantive, cleanup) — below the cap at pass 6 — ranked 17 of 18; cleanup/conventions/altitude band. CONFIRMED at r2 byte-for-byte (Athena.tsx:446-449 added by this diff is identical to the pre-existing TabulatorTables.tsx:129-132, and a repo-wide grep for get('bucket') finds only those two), but the failure it names is drift risk rather than a present defect, which puts it below f29's present-tense one on concreteness. Its deeper form posts nowhere either: the altitude entry for the same ground, f1, was refuted at r1 and stands settled.
  • f42 (minor, altitude) — below the cap at pass 6 — ranked 18 of 18, last; cleanup/conventions/altitude band. PLAUSIBLE, and the verification is explicit that the trigger DOES NOT EXIST YET — ElasticSearch.tsx passes no error prop and its query-load failures route to a standalone Lab.Alert, so the behavior is correct today and only a future change wiring an error signal into that QuerySelect would break it. Lowest concreteness in the whole surviving set. The verification also caught a citation slip in the return (the ElasticSearch QuerySelect block is at :268-274 with value= at :272, not :270); the entry's own locus, Athena.tsx:151, is correct.
  • f44 (minor, description-conformance) — below the cap at pass 6 — ranked 15 of 18; scope-and-intent band, minor, AUTHOR-OWNED. CONFIRMED at r2 against the head: catalog/CHANGELOG.md:24 states the execution/breadcrumb link scope-preservation and :25 states the withhold-on-zero-results rule, and a grep of the whole body for 'breadcrumb', 'execution', 'withheld', 'matches nothing', 'empty list', 'no results', 'zero results' returns nothing. Capped, not withheld: the 2026-08-27 ruling makes author-owned findings content that ranks and caps with every other, and this one loses to correctness on rank alone. It is visible in the body's parked list and carried to the next pass.
  • f46 (substantive, removed-behavior) — below the cap at pass 7 — ranked 11 of 18, first out; head of the scope-and-intent band, which the confirmed correctness band outranks and which took all ten slots this round. Admitted at substantive THIS round as a widening (see admit_rationale) rather than deleted, and CONFIRMED by the verifier with its mutation named. The cost of the cut is real and I state it: this is the test gap on the unit's HEADLINE fix, and its sibling f21 (the breadcrumb half) has been parked since pass 1, so the whole 'the restored seam is pinned by nothing' shape now waits in the parked list. What reduces the cost to something short of a deletion: the defect the missing test would catch DOES post, twice over, as f33 and f16 — the reader is told the preference does not actually reach the resolver, which is the failure the absent test would have caught.
  • f47 (minor, cross-file) — below the cap at pass 7 — ranked 15 of 18. RE-CLASSED BY THIS RULING, and that is why it is cut rather than filling slot ten: the cross-file lane filed it in the correctness band, but nothing misbehaves at this head — the code is right and the two comments (model.ts:1079-1083 and PackageFilters.tsx:237-239, both untouched by this diff) are stale about it. That is a conventions/cleanup defect, which the ranking places below scope-and-intent, not a correctness one. The verifier CONFIRMED the contradiction verbatim AND established what caps its weight: model.spec.ts:444-451 pins the displayed>0 clause the stale comments invite a maintainer to delete, so the invited regression fails the suite. Recorded plainly: no CONFIRMED correctness defect was dropped by this cap, and this entry is the one the reader might mistake for one.
  • f49 (substantive, scope) — below the cap at pass 7 — ranked 12 of 18; scope-and-intent band. CONFIRMED at this head (EditRoles is unexported, Users.spec.tsx never renders it, so the dialog half of CHANGELOG.md:27 is uncovered while the identical readOnly at Users.tsx:876 is). Cut because it is a coverage gap on a fix whose CODE the verifier found correct — reverting Users.tsx:660 is the scenario, not anything the head does — and every entry above it names a defect live at this head. Third member of the same rule-class as f46 and f21; if the next pass has room, that class posts as one entry with three loci rather than three entries.
  • f50 (minor, scope) — below the cap at pass 7 — ranked 13 of 18; scope-and-intent band, minor. CONFIRMED: hooks.spec.tsx covers only the chunk-load degradation and nothing pins fixtureAdapter out of the flag-off entry chunks. It is cut below its own siblings f46 and f49 because its scenario needs a future edit (a static import or a barrel re-export) before anything breaks, where theirs need only a revert. Note what this leaves standing from f37's pass-1 rationale: claim 6's production-build verification still rests on the author's word alone and no artifact in the repository re-checks it.
  • f51 (minor, optionality) — below the cap at pass 7 — ranked 16 of 18. A CREDIT, not a defect: the optionality lane returned in finding shape the fact that narrowing useWorkgroup's third parameter to defaultWorkgroup?: string removes utils/BucketPreferences from the AWS-call layer entirely, a door the change claims and genuinely opens (requests.spec.ts:1269 already drives the hook with a bare string). CONFIRMED. Cut because the cap exists to protect author attention for what the author owes an answer on, and a credit owes none — never because it is untrue. Carried forward so the next pass can post it if the correctness band ever clears; it is the only entry on this ledger that says something went right.
  • f54 (minor, intent) — below the cap at pass 7 — ranked 14 of 18; scope-and-intent band, minor, AUTHOR-OWNED. CONFIRMED as the undisclosed-rider question (useAdapter deleted from hooks.ts and the barrel at index.ts:35, surfacing only in the description's Update list and never in the CHANGELOG), and distinct from the settled f20, which was refuted on whether supportsRequests is still reachable rather than on whether its removal was disclosed. Capped, not withheld — the 2026-08-27 ruling makes author-owned findings content that ranks and caps with every other — and it loses on rank to nine live defects and the split.

Dispositions since round 3.

  • f48 — declined by verifier-lane: REFUTED on the claim as filed: three of the five cited call sites — components/Layout/ContentBar.tsx, Home/Buckets/Buckets.jsx and website/pages/Landing/FrontDoor/ExampleQueries.tsx — are MUI ButtonBase descendants (IconButton / clickable Chip, per their own comments), so they already receive the theme's .Mui-focusVisible ring and are not instances of the hand-rolled pattern the candidate standard names. The verifier did confirm the two parts that stand — the new span at Users.tsx:815 carries no focus styling at all, and an exhaustive grep for focusRing|FocusRing|useFocusVisible|focus-ring across catalog/app finds only Sidebar.tsx's file-private const — but a candidate standard whose evidence set is 40% wrong is not the standard to put to the maintainer. The narrower true statement (no shared carrier exists; Users.tsx:815 has none) is already carried by the parked f29.
  • f52 — deviation by verdict: ACCEPTED as the norm's compensated deviation — 'an intent or standards question the ruling cannot settle, answered by escalation to the maintainer's gate rather than by a guess' — and as the intent lane's own charter, which makes an intent question it cannot settle an escalation and NEVER a blocker. I checked it is genuinely unsettleable at this seat rather than judgment being dodged: the direction it would have to be ruled against does not exist in this checkout (queryRedirects.jsx:8-11 states the re-home's premise 'the bucket is not a home for the consoles anymore' as a comment, not a direction node; Athena/spec.md models no ?bucket= state; body.md carries zero linked issues and this pass's related set parsed to zero references, so no upstream came down). Both readings are live and neither is refutable here — the diff's (a per-bucket preference needs a bucket in scope) and the alternative the description never weighs (a workspace-level default leaving the console bucket-free). Settled here rather than left open precisely so it cannot be ranked or posted as a finding against the author: it routes to the maintainer's gate with f40's two pass-1 escalations, and the pen-holder owes it a line in the receipt. Its checkable consequences are NOT deferred with it — they stand on their own evidence as f30, f31, f2 and f17.
  • f53 — declined by verifier-lane: REFUTED: the CHANGELOG's phrase is a near-verbatim paraphrase of the code's OWN comment at Users.tsx:657-659 — 'a live Save the registry refuses submits unchanged values and reports success' — so 'refuses' means the silent no-op the comment describes, not an error the entry never claims. And the premise that the live-Save path was unreachable is false at the diff: pre-fix the dialog title, RoleSelect.nonAssignable and the Save DialogActions branch (Users.tsx:702-732, Save at :730) all gated on user.isRoleAssignmentDisabled ALONE, so a Save WAS offered for isService && !isRoleAssignmentDisabled — the exact state this PR's own new spec case ['a service user', {isRoleAssignmentDisabled: false, isService: true}] exercises.

Measures. passes/rounds run 3; open blockers 0, substantive 7, minor 3; parked 30; settled 14 of 54; re-graded 4 (verdict: 4).

Posture: advisory. This review is advisory — it is a comment, not a merge gate, and it was rendered for a human GO before it reached this pull request. A reply on any comment is read and dispositioned at the next pass; a settled finding is not re-litigated without a named cause.

Comment on lines +17 to +20
function useScopeSearch() {
const { bucket } = useParams()
const { search } = useLocation()
return mkSearch({ bucket, ...parseSearch(search, true) })

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f12 · substantive · second-lineage

FAILURE SCENARIO: visiting /b/source/queries/athena/primary?bucket=other redirects to /queries/athena/primary?bucket=other, so the console loads other's preferences instead of source's; the root redirect at :44-51 has the same spread order. The route's own bucket is the authoritative one and the incoming query string wins over it.

Subject: a query-string bucket overrides the authoritative bucket from the legacy route · anchored at 491639d · a reply here is read and dispositioned at the next pass.

Comment thread catalog/CHANGELOG.md
Comment on lines +24 to +28
- [Fixed] Athena Queries: a console opened with a `?bucket=` scope — which every legacy per-bucket queries URL redirects to — honors that bucket's `ui.athena.defaultWorkgroup` again, and keeps that scope when you follow an execution or breadcrumb link inside the console; the workspace re-home had silently dropped the preference, landing customers on the alphabetically-first workgroup ([#5217](https://github.com/quiltdata/quilt/pull/5217))
- [Fixed] Search sidebar: the "Sort by" control no longer disappears while typing in "Find metadata" on stacks with truncated facet lists, it is withheld when the query matches nothing rather than offering to sort an empty list, and it has an accessible name — its label used to land on a hidden input, reading as an unnamed button to assistive tech ([#5217](https://github.com/quiltdata/quilt/pull/5217))
- [Fixed] Queries: the query selector announces its label to assistive tech, and no longer claims "Custom" is loaded while its helper text reports the query failed to load ([#5217](https://github.com/quiltdata/quilt/pull/5217))
- [Fixed] Admin Users: a disabled Enabled or Admin switch explains why on hover or keyboard focus — "you can't deactivate yourself" and "managed by the stack" used to render as the same mute, dead control — and the roles dialog and Role column read as read-only end to end for service users rather than offering a Save the registry refuses ([#5217](https://github.com/quiltdata/quilt/pull/5217))
- [Changed] With the `data-products` preview off, the demo fixture data no longer ships in the bundles a browser downloads on the volumes landing; the adapter behind the preview loads only when the feature is on ([#5217](https://github.com/quiltdata/quilt/pull/5217))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f27 · substantive · scope

Five subsystems sharing no code and no call path: A queryRedirects.jsx+spec plus Athena/{Athena,History,model/state,model/requests}; B Search/{model,Layout/PackageFilters}; C Queries/QuerySelect+snapshot; D Admin/UsersAndRoles/Users; E model/DataProducts/{hooks,index}. The DataProducts extraction is the one worth making first: the only [Changed] in the set, the only app/model layer change, and the only fix whose stated verification (a production build's chunk contents) leaves no artifact in the repo a reviewer or a later bisect can re-check. FAILURE SCENARIO: one regression post-release makes git revert of #5217 take the other four fixes with it, and all five CHANGELOG entries cite #5217, so any partial revert requires hand-editing catalog/CHANGELOG.md and cherry-picking hunks out of a 22-file commit under release pressure.

Subject: six independent fixes gathered as one unit — split candidate · anchored at 491639d · a reply here is read and dispositioned at the next pass.

Comment on lines +456 to +460
)
return (
<Model.Provider>
<AthenaContainer />
</Model.Provider>
<BucketPreferences.Provider bucket={bucket}>
<ScopedWrapper />
</BucketPreferences.Provider>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f30 · substantive · removed-behavior

The deleted single <Model.Provider></Model.Provider> return kept the console OUTSIDE any BucketPreferences.Provider, which is what made the create-package Files panel inert (Inputs/Files.tsx:96-117 read the Ctx default Result.Init() and returned null); nothing re-establishes that inertness. FAILURE SCENARIO: on /queries/athena/?bucket= — where every legacy /b//queries/athena/... URL and containers/Bucket/Header.tsx:95 now land — run a query over manifest entries, click 'Create package' (the Files panel renders for the first time), then 'Add files from bucket': S3FilePicker.Dialog mounts BucketSelect, whose RRDom.useParams<{bucket}>() + invariant(currentBucket, ...) at PackageDialog/Inputs/Files/S3FilePicker.tsx:122-123 THROWS because /queries/athena/:workgroup has no :bucket param, unwinding to the only boundary in the tree (app.tsx:128) and replacing the whole catalog screen.

Subject: the console moving inside BucketPreferences.Provider un-gates a package-dialog path that needs a :bucket route param · anchored at 491639d · a reply here is read and dispositioned at the next pass.

Comment on lines +451 to +455
if (!bucket)
return (
<Model.Provider>
<AthenaContainer />
</Model.Provider>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f31 · minor · removed-behavior

The deleted unconditional return gave /queries/athena* one Model.Provider mount for any query string; the new if (!bucket) early return makes the two cases structurally different trees and nothing re-establishes the stable mount. FAILURE SCENARIO: with elasticsearch-queries on, a user editing SQL at /queries/athena/primary?bucket=b clicks the 'Athena' tab (Queries.tsx:80 links to urls.queriesAthena() with no search), the branch flips, Model.Provider unmounts, and the editor text held in useQueryBody's state (model/requests.ts:816) plus the selected catalog/database are DESTROYED; before this change the same click kept the provider mounted. This is a distinct consequence from the scope-dropping itself: unsaved work is lost, not just a preference.

Subject: the console's mount identity now depends on the presence of ?bucket=, destroying in-progress editor state · anchored at 491639d · a reply here is read and dispositioned at the next pass.

Comment on lines +1366 to +1367
const maxSeen = React.useRef(0)
maxSeen.current = Math.max(maxSeen.current, totalAvailable)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f36 · minor · diff-correctness

maxSeen.current = Math.max(...) mutates a ref in the RENDER BODY, and it latches against any drop in totalAvailable rather than only against the 'Find metadata' narrowing it was written for. CITED STANDARD: React docs, 'Referencing values with refs' — 'Do not write or read ref.current during rendering'. FAILURE SCENARIO: on the no-filtering path (available.length 1..9, filtering: Disabled()) a package-search change from 9 available metadata fields to 3 that lands with NO fetching phase (urql cache hit, so AvailablePackagesMetaFiltersReady keeps rendering the same Group element type and the instance is not remounted) leaves maxSeen=9 and keeps 'Sort by' offered over a 3-field list — which the replaced totalAvailable >= FACET_ORDERING_THRESHOLD withheld. Note this is the OPPOSITE direction from the remount-reset hole: the latch persists when it should drop.

Subject: useOrderingOffered writes a ref during render, and its stickiness is not scoped to the filter box · anchored at 491639d · a reply here is read and dispositioned at the next pass.

// an adapter that cannot browse or fetch, so no call site needs a new branch.
const unavailableAdapter: DataProductAdapter = {
listProducts: async () => [],
getProduct: async () => null,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f45 · minor · diff-correctness

Returning null for a chunk-fetch failure is indistinguishable from a genuine miss, and the one caller treats null as drift and navigates away: a deploy rotates chunk hashes while a tab is open, the user opens a data-product deep link, import('./fixtureAdapter') 404s, useProduct returns null, and Detail.tsx:330 redirects to the volume grid as if the product had been deleted — no error, no retry affordance. The comment's claim that these 'match what the port already returns' holds for listContents/fetchEntry but not for getProduct, where the port has no 'cannot look' shape.

Subject: unavailableAdapter.getProduct reports a transport failure as 'product does not exist' · anchored at 491639d · a reply here is read and dispositioned at the next pass.

@nl0

nl0 commented Aug 31, 2026

Copy link
Copy Markdown
Member

5217 stack: chain restored after #5259 merged

#5259 (unit 1) merged, so the rest of the chain has been rebased onto master and force-pushed. Recording what changed and how it was verified, since this moved eight branches at once.

Why the force-pushes. #5259 was squash-merged, so unit 1 landed on master under a new SHA while unit 2's branch still carried the original commit. That is why #5260's diff had been showing model/DataProducts/hooks.ts and index.ts, which are not its layer. Rebasing onto master dropped that commit as already-applied (matched by patch id), and #5260 is now four files: the changelog entry, QuerySelect.tsx, its spec, and the snapshot. GitHub had already retargeted #5260 to master; that is confirmed.

Per-PR verification — each PR shows exactly its own layer.

PR base files
#5260 master 4
#5261 5217-2 3
#5262 5217-3 4
#5263 5217-4 3
#5264 5217-5 3
#5265 5217-6 5
#5266 5217-7 3
#5267 5217-8 15

Layer content was checked by patch id rather than by eye: each layer's own diff was fingerprinted before and after, and all seven cascaded layers came through byte-identical. The chain is contiguous — every layer contains its parent's tip — so reviewing a layer against its stack base still works. As a whole-stack check, the tree at the top of the stack is identical to what it was before the rebase: master gained exactly the squash of unit 1, which the stack already contained, so the rebase introduced no content drift at all. Nothing needed conflict resolution.

Native stack. The chain is registered as a GitHub stack, and it survived the rebase intact — it tracks the new head SHAs with master as its base. Linking the existing PRs was a no-op that reported the stack already current, so no pull request was recreated and no base branch was moved. Bases, heads and descriptions were captured before and after and are unchanged by that step.

Caveats. Nothing failed to convert. Two things worth knowing: the stack was already registered before this rebase, so the linking step confirmed rather than created it; and whether merging one PR auto-retargets the next is not something this exercise demonstrated, so it should not be assumed until a merge actually shows it. Merge bottom-up.

@nl0

nl0 commented Sep 1, 2026

Copy link
Copy Markdown
Member

superseded by the stack of atomic prs ^

@nl0 nl0 closed this Sep 1, 2026
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.

4 participants