Skip to content

Show the bucket name on stacks without the beta setting - #5235

Open
QuiltSimon wants to merge 8 commits into
masterfrom
fix/bucket-name-ungated
Open

Show the bucket name on stacks without the beta setting#5235
QuiltSimon wants to merge 8 commits into
masterfrom
fix/bucket-name-ungated

Conversation

@QuiltSimon

@QuiltSimon QuiltSimon commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Found reviewing the 26.8.0 candidate.

Before the shell re-home (#5189), NavBar/Controls.tsx rendered s3://<bucket> unconditionally on every bucket page, for every stack. At head the name lives only in the new header card, and BucketLayout gates that whole card behind settings?.beta. With beta off — the default — a bucket's Packages and Workflows tabs render just the tab strip: Overview / Files / Packages / Workflows, with the bucket identified nowhere on the page. Overview happens to name it in its own content, which is probably why this survived review.

The name is wayfinding, not a beta feature.

The fix

Header bundles the name with useStats and useTabulatorTables, so ungating it wholesale would add two data fetches to every bucket page on every stack. Instead the name and its adjacent admin-settings link are split out as an exported BucketTitle, and BucketLayout renders Header when beta is on, BucketTitle when it is off. The stats row, the tables stat and the Create-package button stay behind beta exactly as before.

BucketTitle calls neither stats hook, so the ungated path costs no queries — pinned by a test asserting both mocks go uncalled.

Tests

  • TZ=UTC npx vitest run app/containers/Bucket/Header.spec.tsx — 12 passed (9 pre-existing untouched, 3 new)
  • TZ=UTC npx vitest run app/containers/Bucket — 347 passed across 48 files
  • npx tsc --noEmit -p . — clean

Honest limit on coverage: the new tests pin BucketTitle's own behavior (renders the name, renders no stats, issues no queries), and Header still renders the name. Neither exercises BucketLayout's beta branch itself — that is the line that actually regressed, and it has no test harness here. It is verifiable by reading the diff in Bucket.tsx, and worth a reviewer's eye rather than trusting the green suite.

Notes

  • The Overview/Header.tsx and Overview/v2/Header.tsx files are unrelated components with the same name; untouched.
  • Ships with its catalog/CHANGELOG.md entry.

🤖 Generated with Claude Code

Greptile Summary

This PR restores the bucket name and adjacent administrator settings link on bucket pages when beta features are disabled, while keeping statistics, tables, create-package controls, and their queries beta-gated.

  • Extracts the existing title and administrator link into a query-free BucketTitle component.
  • Selects the full header for beta stacks and the title-only header otherwise.
  • Adds focused rendering and query-isolation tests plus a changelog entry.

Confidence Score: 5/5

The PR appears safe to merge with no concrete correctness, security, or compatibility failures identified.

The extracted title preserves the existing administrator gate and route target, the layout supplies its required providers, and the non-beta branch avoids both statistics query paths as intended.

Important Files Changed

Filename Overview
catalog/app/containers/Bucket/Bucket.tsx Selects the full or title-only bucket header according to the beta setting while always retaining bucket wayfinding.
catalog/app/containers/Bucket/Header.tsx Extracts the existing title and admin settings link without changing their authorization or routing behavior.
catalog/app/containers/Bucket/Header.spec.tsx Verifies title rendering and confirms the title-only component invokes neither statistics query path.
catalog/CHANGELOG.md Documents restoration of bucket wayfinding on non-beta stacks.

Reviews (1): Last reviewed commit: "Show the bucket name on stacks without t..." | Re-trigger Greptile

Before the shell re-home, NavBar/Controls rendered `s3://<bucket>` on every
bucket page for every stack. At head the name lives only in the new header
card, which BucketLayout gates behind `settings?.beta` — so on a default
stack the Packages and Workflows tabs render just the tab strip, with the
bucket identified nowhere on the page.

Split the name and its admin settings link out as BucketTitle and render
that when beta is off. It deliberately does not call useStats or
useTabulatorTables, so the ungated path adds no queries; the stats row and
Create-package button stay behind beta.

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

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 78.57143% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 34.95%. Comparing base (fe9bda5) to head (35a02d6).

Files with missing lines Patch % Lines
catalog/app/containers/Bucket/Overview/Header.tsx 0.00% 2 Missing ⚠️
...atalog/app/containers/Bucket/Overview/Overview.tsx 0.00% 1 Missing ⚠️

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

HEAD has 25 uploads less than BASE
Flag BASE (fe9bda5) HEAD (35a02d6)
lambda 14 0
py-shared 1 0
api-python 10 0
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #5235       +/-   ##
===========================================
- Coverage   55.20%   34.95%   -20.26%     
===========================================
  Files         872      741      -131     
  Lines       36766    23801    -12965     
  Branches     6429     6425        -4     
===========================================
- Hits        20298     8320    -11978     
+ Misses      14724    13737      -987     
  Partials     1744     1744               
Flag Coverage Δ
api-python ?
catalog 34.95% <78.57%> (+0.02%) ⬆️
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.

smkohnstamm and others added 5 commits August 27, 2026 11:02
Review found the first pass double-rendered on the tab it did not check.
Overview v1 (the non-beta Overview) has always drawn its own h5 bucket
name and admin gear in its hero, so ungating the header card above the
tabs put both on screen twice. The v2 Overview already made this split
when the header moved above the tabs; v1 never did.

The hero now carries the description and stats only, and the
out-of-stack fallback drops its name too. Header takes `withStats`
instead of the layout choosing between two components, which also makes
the gate itself testable — the line that regressed had no harness. A
long name ellipsizes rather than overflowing the card, and the admin
link is covered both ways now that the redux mock is controllable.

Co-Authored-By: Claude <noreply@anthropic.com>
oxfmt runs first in lint:app, so the multi-line description conditional
(a leftover shape from the deleted M.Box wrapper) would have failed CI
before oxlint or the tests ran.

The stats row's top margin separated it from the name above it. With the
name gone, a bucket with no description had nothing there, leaving a
strip of hero background above the stats — apply the margin only when a
description precedes them.

Co-Authored-By: Claude <noreply@anthropic.com>
minWidth: 0 sat on .title, not on the h5, so the flex item kept
min-width: auto and could not shrink past min-content — nothing for
overflow: hidden to clip, and the changelog claim did not hold.

.top's position: relative was the containing block for the absolutely
positioned settings link that moved above the tabs; no positioned
descendant remains.

Co-Authored-By: Claude <noreply@anthropic.com>
The BucketTitle block's afterEach reset cleanup and isAdmin but not the
two mocks its final test calls mockClear on, so a describe appended below
would read undefined from useTabulatorTables. Harmless only because this
block is currently last in the file.

Co-Authored-By: Claude <noreply@anthropic.com>
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.

3 participants