refactor(catalog): add bucket context and S3 config factory#4924
refactor(catalog): add bucket context and S3 config factory#4924drernie wants to merge 1 commit into
Conversation
Introduce a route-level bucket context for repeated bucket param reads and thread bucket config into the existing S3 factory. The motivation is repeated route parsing and region/presign inconsistency risk across bucket read paths.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4924 +/- ##
==========================================
- Coverage 46.55% 46.54% -0.01%
==========================================
Files 832 833 +1
Lines 34120 34168 +48
Branches 5833 5839 +6
==========================================
+ Hits 15884 15904 +20
- Misses 16237 16264 +27
- Partials 1999 2000 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| export function BucketContextProvider({ | ||
| bucket: bucketProp, | ||
| config = {}, |
There was a problem hiding this comment.
config = {} creates a brand-new object literal on every call to BucketContextProvider. That reference then flows into BucketContextValueProvider's useMemo([name, config]) dependency array, so the memoized value is recreated on every render even when neither the bucket name nor the config has actually changed. Every consumer of the outer context (e.g. the BucketInner subtree in Bucket.tsx, test wrappers that omit config) will re-render unnecessarily on each parent re-render.
Replace the inline default with a stable module-level constant: const EMPTY_CONFIG: BucketConfig = {}, and use that as the default value.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| export default function Bucket() { | ||
| return ( | ||
| <BucketContextProvider> | ||
| <BucketInner /> | ||
| </BucketContextProvider> | ||
| ) |
There was a problem hiding this comment.
config: {} to components that will later get the region-aware inner provider
Bucket() mounts an outer BucketContextProvider (no props → config = {}) so that BucketInner can call useBucketStrict(). Then, only after useBucketExistence resolves Ok, BucketReady mounts an inner BucketContextProvider with the cached region. Components that sit between the two providers — currently BucketPreferences.Provider, MetaTitle, and BucketNav.BucketNav — would receive the empty-config outer context if they ever call useBucketContext().config for S3 construction. None of them do today, but the silent divergence between the two context layers is easy to miss when adding new features inside BucketLayout but outside the Ok branch.
1 participant
Current pain
Bucket-route components repeatedly read
useParams().bucket, and S3 clients are created without a single bucket-config-aware path. That makes region/presign behavior easy to drift as more bucket-specific configuration is introduced.Changes
BucketContextProvideranduseBucketContext()for the bucket route subtree.containers/Bucket/Bucket.tsx, reusing existing bucket existence/cache plumbing without adding a fetch.containers/Bucket/*withuseBucketContext()where they read the route bucket.useS3Factory()/useS3()path to accept bucket config and preserve the old region-string/default behavior.useBucketContext().config.Bucket context inventory
Direct bucket route reads were replaced in overview, dir/file, package tree/revisions/compare, queries, workflows, and package-dialog/file-picker paths.
Bucket.tsxremains the route owner and provider mount point.S3 construction inventory
No separate S3 factory was added. Existing
utils/AWS/S3.jsxremains the construction point; bucket subtreeAWS.S3.use()call sites now pass bucket context config.Verification
npm run lintnpm run test:only -- app/containers/Bucket/context.spec.tsx app/containers/Bucket/Queries/Athena/model/state.spec.tsx app/containers/Bucket/PackageDialog/State/schema.spec.ts app/containers/Bucket/PackageDialog/State/params.spec.ts app/containers/Bucket/Dir/Toolbar/CreatePackage/useSuccessors.spec.ts --no-cache— 37 passedNODE_OPTIONS="--localstorage-file=/tmp/quilt-catalog-vitest-localstorage.json" npm testcurrently fails in unrelated suites under Node 26/npm 11:app/utils/spreadsheets/spreadsheets.spec.ts: date expectations off by one day for ODS/FODS/CSV casesapp/containers/Bucket/Queries/Athena/model/requests.spec.ts: twouseQueryRunasync timing tests time outPre-merge stack verification
Not run yet. Before merge: workflow-dispatch an interim catalog image for this branch, pin the immutable tag on the shared test stack, and record the test-stack URL/image tag here.
Greptile Summary
This PR centralizes bucket route reads into a
BucketContextProvider/useBucketContext()hook pair, replacing scattereduseParams().bucketcalls across the bucket subtree and routing all S3 client construction through a bucket-config-awareuseS3(bucketConfig)path.context.tsx): introducesBucketContextProvider(auto-reads:bucketfrom route params or accepts explicitbucket+configprops) anduseBucketContext()consumed by ~20 files previously doing directuseParamsreads.S3.jsx):useS3/ the internal factory now accept aBucketConfigobject (or legacy region string) so that bucket-specific region is forwarded to the cached client; backward-compatible with callers that pass nothing.Bucket.tsx: an outer route-based provider letsBucketInnercalluseBucketStrict(), while a separate inner provider (mounted afteruseBucketExistenceresolves) supplies the cached region config to the actual page components.Confidence Score: 4/5
The refactor is mechanically correct and well-tested; the two-provider nesting in Bucket.tsx is a design choice that works today but leaves a latent trap for future components added in the wrong layer.
The migration is broad (30+ files) but each change is a straightforward substitution. The main fragility is that
config = {}inBucketContextProvidercreates a new object reference on every render, defeating theuseMemoinBucketContextValueProviderand causing unnecessary context re-renders for all consumers of the outer provider. The double-provider pattern is functional but requires contributors to understand which layer they're adding code to.catalog/app/containers/Bucket/context.tsx (config default stability) and catalog/app/containers/Bucket/Bucket.tsx (two-layer provider structure).
Important Files Changed
config = {}default creates an unstable object reference on each render, defeating the useMemo inside BucketContextValueProvider.Comments Outside Diff (1)
catalog/app/utils/AWS/S3.jsx, line 154-156 (link)useS3Factorystill documents the old(region?) => S3Clientsignature. The factory returned now accepts abucketConfigargument that can be a region string, aBucketConfigobject, orundefined, so the comment should be updated to avoid misleading callers who consult it.Reviews (1): Last reviewed commit: "refactor(catalog): add bucket context an..." | Re-trigger Greptile