web/admin: Objects tab on S3Detail (Phase 5) - #816
Conversation
Wires the Phase-3b /admin/api/v1/s3/buckets/{name}/objects HTTP
surface into the SPA's bucket-detail page. Driven by §4.x of
docs/design/2026_05_22_proposed_admin_data_browser.md.
Capabilities (gated on session role):
read_only — list + download
full — additionally upload + delete
UI:
- New "Objects" card on S3Detail with a paginated table (page size
100, mirroring the server's adminListObjectsDefaultMaxKeys).
- Breadcrumb navigation: clicking a path segment drops back to that
prefix. Clicking "(root)" returns to the bucket root.
- Folders surfaced via the server's CommonPrefixes (delimiter fixed
to "/"). Folder rows render with a 📁 prefix and clicking drills
into the prefix.
- Object rows open a detail modal showing size / content-type / ETag
/ last-modified / storage-class plus Download and Delete buttons.
- Upload: hidden <input type="file"> triggered by an "Upload" button.
The picked file uploads to `{prefix}/{file.name}` with its native
Content-Type. The 100 MiB server cap surfaces as a 413 from the
bridge.
- Delete uses a two-stage confirm (button row collapses to "Delete
this object?") so a click cannot accidentally destroy bytes.
API client (web/admin/src/api/client.ts):
- AdminObject / AdminObjectListing / AdminListObjectsOptions type
definitions mirror the Go handler.
- listObjects (JSON) / deleteObject (JSON) reuse the existing
apiFetch path with the CSRF header.
- putObject / downloadObjectURL bypass apiFetch because they carry
raw bytes (application/octet-stream):
* putObject does a manual fetch() so the body can be a Blob; the
CSRF header is still attached.
* downloadObjectURL returns the URL for a plain <a download> link
so the browser streams the response and honours the server's
Content-Disposition: attachment header.
- base64UrlEncodeBytes builds the {key-b64url} segment via
TextEncoder → btoa → url-safe alphabet, padding stripped.
Self-review (5 passes):
1. Data loss — no SPA-side write path bypasses the server's
validation; the 100 MiB cap is enforced at both the
http.MaxBytesReader and the adapter ErrAdminUploadTooLarge
sentinel layers.
2. Concurrency — Modal busy gates Esc / backdrop dismissal so a
half-completed save cannot be orphaned; upload busy gates the
Upload button.
3. Performance — List paginates at 100; cursor stack lets Refresh
reload the current page without restarting the scan. Folder
navigation runs a fresh listObjects under the new prefix
rather than client-side filtering.
4. Consistency — Prefix change resets to page 0. Refresh after
upload reloads the current page so the operator sees their own
write immediately.
5. Test coverage — Manual exercise of dev server only. Vitest
harness for the SPA does not yet exist; encoding helpers will
get property tests when that lands.
npm run lint (tsc -b --noEmit) — passes
npm run build (tsc -b && vite build) — passes (bundle output is
.gitignored; internal/admin/dist/index.html placeholder
unchanged in this commit)Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 38 minutes and 21 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bootjp
commented
May 23, 2026
@claude review Phase 5: S3 Objects tab. Stacked on PR #814 (Phase 3b).
npm run lint / npm run build pass. Backend tests unchanged. |
Claude finished @bootjp's task in 3m 13s —— View job Code Review: Phase 5 — S3 Objects TabOverall the implementation is clean. The breadcrumb prefix navigation, two-stage delete confirm, and upload-then-refresh flow are solid. A few issues worth addressing before merge. Medium — Race condition in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:388dd46c8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| useEffect(() => { | ||
| setCursorStack([]); | ||
| void loadPage(undefined, prefix); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
This effect starts a new listObjects call whenever bucket or prefix changes, but it does not cancel or ignore prior in-flight requests. If an operator navigates folders quickly (or switches buckets), an older response can resolve last and overwrite state with stale rows for the previous prefix, so the table and object actions no longer match the current breadcrumb context.
Useful? React with 👍 / 👎.
| setCursorStack((s) => [...s, next]); | ||
| void loadPage(next, prefix); |
There was a problem hiding this comment.
onNextPage appends next_continuation_token to cursorStack before the page fetch completes. On a transient API failure, the UI still shows the old page but internal cursor state has already moved forward; subsequent Refresh/post-mutation reloads use the wrong token and can jump to a different page unexpectedly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive S3 object management interface, allowing users to list, upload, download, and delete objects within buckets. Key additions include new API client methods, TypeScript interfaces for S3 metadata, and the S3ObjectsTab component for the admin dashboard. The review feedback suggests enhancing the reliability of the UI by implementing request cancellation using AbortController to prevent race conditions during navigation and broadening the success criteria for object uploads to support standard S3-compatible responses.
| useEffect(() => { | ||
| setCursorStack([]); | ||
| void loadPage(undefined, prefix); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [bucket, prefix]); |
There was a problem hiding this comment.
The useEffect hook triggers a page load whenever the bucket or prefix changes, but it lacks a cleanup mechanism to abort in-flight requests. This can lead to race conditions where a slower response from a previous prefix/bucket overwrites the state of the current one. Consider using an AbortController to cancel stale requests.
| useEffect(()=>{ | |
| setCursorStack([]); | |
| voidloadPage(undefined,prefix); | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| },[bucket,prefix]); | |
| useEffect(()=>{ | |
| constcontroller=newAbortController(); | |
| setCursorStack([]); | |
| voidloadPage(undefined,prefix,controller.signal); | |
| return()=>controller.abort(); | |
| },[bucket,prefix]); |
| // without scanning from the start. | ||
| const [cursorStack, setCursorStack] = useState<string[]>([]); | ||
| const loadPage = async (cursor: string | undefined, p: string) => { |
There was a problem hiding this comment.
The loadPage function should accept an AbortSignal to allow the caller (like useEffect) to cancel the request if the component unmounts or the parameters change.
| constloadPage=async(cursor: string|undefined,p: string)=>{ | |
| constloadPage=async(cursor: string|undefined,p: string,signal?: AbortSignal)=>{ |
| const result = await api.listObjects(bucket, { | ||
| prefix: p, | ||
| delimiter: DELIMITER, | ||
| continuation_token: cursor, | ||
| max_keys: PAGE_SIZE, | ||
| }); |
There was a problem hiding this comment.
Pass the signal to the api.listObjects call to ensure the network request is actually aborted when the controller is triggered.
| constresult=awaitapi.listObjects(bucket,{ | |
| prefix: p, | |
| delimiter: DELIMITER, | |
| continuation_token: cursor, | |
| max_keys: PAGE_SIZE, | |
| }); | |
| constresult=awaitapi.listObjects(bucket,{ | |
| prefix: p, | |
| delimiter: DELIMITER, | |
| continuation_token: cursor, | |
| max_keys: PAGE_SIZE, | |
| },signal); |
| `${apiBase}/s3/buckets/${encodeURIComponent(bucket)}/objects/${base64UrlEncodeBytes(key)}`, | ||
| { method: "PUT", body, headers, credentials: "same-origin" }, | ||
| ); | ||
| if (res.status === 204) return; |
There was a problem hiding this comment.
The putObject method only treats HTTP 204 as a success. While the current Go backend implementation returns 204, standard S3-compatible APIs may return 200 OK for successful PUT operations. It is safer to check res.ok or handle 200 explicitly to avoid unexpected ApiError throws when the body is empty but the status is 200.
| if(res.status===204)return; | |
| if(res.ok)return; |
…x putObject (PR #816 r1) Three reviews on PR #816 (1 Codex P1, 1 Codex P2, 1 Gemini high echoing the P1, 1 Gemini medium). 1 Codex P1 / Gemini high - cancel stale list requests on prefix/bucket changes. The previous loadPage had no cancellation, so an older slower listObjects could resolve last and overwrite state with stale rows for the previous prefix. Follow-up download/delete actions on those rows would then mutate the wrong resource - the bucket/key identity comes from the rendered row. Fix: listAbortRef tracks the in-flight controller. Every loadPage() aborts the predecessor; a generation-token check (listAbortRef !== ctrl) covers the race where a response lands before abort() is processed. AbortError on the catch path is suppressed so a rapid Next-page click sequence doesn't show aborted mid-navigation. Unmount effect aborts the last controller too. 2) Codex P2 - advance cursorStack only after fetch succeeds. The old onNextPage pushed the new continuation token before the fetch resolved. On a transient API failure the UI still showed the old page while internal cursor state had moved forward, so Refresh / post-mutation reloads used the wrong token and jumped to an unrelated page. Fix: loadPage now returns a success boolean. onNextPage awaits the result and only pushes the new cursor when the fetch resolved successfully. Refresh + post-mutation reloads stay aligned with the actual rendered page. 3) Gemini medium - putObject treated only 204 as success. Now treats any 2xx as success via res.ok. The current Go handler returns 204 but a future change that returns 200 with the metadata body would otherwise misclassify as a non-JSON error. Self-review (5 passes): Data loss: closes a write-to-wrong-bucket-or-key vector on the follow-up delete/download path. Closes a cursor-drift vector on the pagination state machine. Concurrency: AbortController + generation-token covers both abort-fired and abort-late races. Performance: aborted requests waste bytes but the alternative is operator-visible incorrectness. Consistency: same pattern as the Phase-4 DynamoItemsTab r1 fix (PR #815 r1) so future audits find one shared shape. Test coverage: no SPA test harness yet; manual exercise of the dev server confirms rapid folder navigation behaves correctly and a forced-fail Next-page leaves cursorStack intact. npm run lint (tsc) - passes npm run build (vite) - passes (dist/index.html placeholder unchanged in this commit) EOF )
bootjp
commented
May 23, 2026
@claude review Round 1 addresses 4 findings:
Same race-condition shape as the Phase-4 DynamoItemsTab r1 fix on PR #815 r1. npm run lint / npm run build pass. |
Claude finished @bootjp's task in 2m 34s —— View job Code Review: Phase 5 — S3 Objects Tab (Round 1 Review)
Round 1 fixes — all three targets verified correct1. Stale list races ( The implementation is clean and covers both abort scenarios:
One minor note: 2. Cursor advance only on success — ✅
3.
Remaining items from original review (not addressed in r1)These were all Low / Nit priority in the original review, none block merge. Low — Objects tab still gated on {detail.data&&<S3ObjectsTabbucket={name}/>}A 503 during a Raft leader flip still hides the tab silently. The detail error block on lines 71-73 already renders the error text in the metadata card, so operators will see something — but they lose access to list/download even though those calls would succeed. Acceptable as-is if the current priority is keeping the surface area small. Low — Download anchor + session expiry ( An Nit — Breadcrumb {segments.map((seg,idx)=>{consttarget=segments.slice(0,idx+1).join("/")+"/";return<spankey={idx}...>
Nit — Disabled current breadcrumb segment is visually identical to clickable ancestors ( SummaryRound 1 is correct. All three targeted findings are properly fixed. The abort pattern (ref + generation token + unmount cleanup) is the same shape used in Phase-4 DynamoItemsTab, which is good for future auditability. The four remaining items are all low/nit and were already known before r1. Ready to merge after #814 lands. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:927117bcbc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| useEffect(() => { | ||
| setCursorStack([]); | ||
| void loadPage(undefined, prefix); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
When bucket changes, this effect immediately calls loadPage(undefined, prefix) but keeps the previous bucket’s prefix in state, so navigating from bucket-a/photos/ to bucket-b can open bucket-b scoped to photos/ instead of root. In React Router, param changes typically reuse the same component instance, so this stale prefix is realistic and can hide root objects (or cause uploads/deletes to target an unintended subpath) until the user manually clicks (root).
Useful? React with 👍 / 👎.
…ath (PR #816 r2) Codex P2 on PR #816. React Router param changes reuse the same S3DetailPage instance, so when the operator navigates from `/s3/bucket-a` (sitting at prefix "photos/") to `/s3/bucket-b`, the prefix state stayed at "photos/". Result: - bucket-b's list call ran against prefix="photos/" instead of root, hiding root-level objects until the operator manually clicked "(root)"; - subsequent upload would write to bucket-b/photos/{file} instead of bucket-b/{file}; - subsequent delete would target the wrong subpath if the row happened to render anything. Fix: new useEffect with deps [bucket] dispatches setPrefix("") on every bucket change. The existing [bucket, prefix] effect re-fires on the next render with the fresh empty prefix; the brief loadPage(undefined, oldPrefix) that runs from this render's commit is cancelled by listAbortRef before any rows land in state. Tested manually: navigating /s3/foo → /s3/bar via the bucket list now opens bar at root regardless of how deep the operator had drilled in foo. npm run lint — passes npm run build — passes
bootjp
commented
May 23, 2026
@claude review Round 2 addresses Codex P2 (bucket-change keeps stale prefix). New effect with deps [bucket] resets prefix to "" on every bucket change; AbortController cancels the brief loadPage that fires from the same commit before any rows land. |
Claude finished @bootjp's task in 2m 8s —— View job Code Review: Phase 5 — S3 Objects Tab (Round 2 Review)
Round 2 fix — verified correct ✅
useEffect(()=>{setPrefix("");},[bucket]);The execution sequence when
The two-layer protection is correct: the One cosmetic note (not a bug): On Render 1, Remaining open items (unchanged from prior rounds — all Low/Nit, none block merge)Low — Objects tab still gated on {detail.data&&<S3ObjectsTabbucket={name}/>}A 503 during a Raft leader flip hides the tab silently. Unchanged from r1. The error text in the metadata card gives operators a signal, but they lose list/download even though those calls would succeed independently. Acceptable as-is. Low — Download anchor + session expiry (
Nit — Breadcrumb {segments.map((seg,idx)=>{consttarget=segments.slice(0,idx+1).join("/")+"/";return<spankey={idx}...>
Nit — Disabled current breadcrumb segment is visually identical to clickable ancestors ( No change from r0. SummaryRound 2 is correct. The bucket-change stale-prefix race (Codex P2) is properly fixed via the two-effect pattern ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:d40ac0acef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| setLoadError(err instanceof ApiError ? `${err.code}: ${err.message || err.code}` : String(err)); | ||
| return false; |
There was a problem hiding this comment.
When loadPage hits a non-abort error, it sets loadError but leaves page untouched, so the previous bucket/prefix rows remain visible and clickable under the new context. After a route change (e.g., /s3/a to /s3/b) followed by a transient list failure, operators can still open stale rows and trigger download/delete against the current bucket with an old key, which can operate on the wrong object if that key exists in both buckets. Clearing page (and related cursor state) on error avoids cross-context actions from stale data.
Useful? React with 👍 / 👎.
Codex P1 on PR #816 r2. The previous loadPage error path set loadError but left `page` populated with the prior bucket / prefix's rows. After /s3/a -> /s3/b followed by a transient list failure, bucket-a's rows stayed visible under the bucket-b context — clicking Download or Delete on those rows then operated against the wrong bucket with an old key, which can hit an existing object if the key happens to collide. Fix: on non-abort error, loadPage now calls setPage(null) and setCursorStack([]). The table renders the empty/error state, no stale rows remain clickable, and a subsequent Refresh cannot reuse a continuation token that no longer addresses anything under the new context. This is a fail-closed semantic — better to surface 'no data' under an error banner than to silently keep cross-context rows in play. Caller audit (semantic-change rule): loadPage is private to S3ObjectsTab; the five callers (init effect, onNextPage, onRefresh, onFilePicked post-upload, onDelete post-delete) all treat the return value as advisory and don't rely on `page` staying populated through a failure. onNextPage already short-circuits on ok=false; the others void-cast the result. No external consumer. Self-review (5 passes): Data loss: closes a write-to-wrong-bucket-or-key vector on the post-failure click path. No write-path semantics changed. Concurrency: no shared state introduced beyond the existing page / cursorStack setters. Performance: one extra setState pair on the error path. Consistency: same fail-closed-on-error shape the rest of the SPA error surfaces follow (no half-rendered table). Test coverage: manual exercise confirms a forced 500 from listObjects collapses the rows and shows the error banner; a subsequent Refresh on the new context loads the right rows. npm run lint - passes npm run build - passes (dist/index.html placeholder unchanged in this commit)
bootjp
commented
May 23, 2026
@claude review Round 3 addresses Codex P1 (stale rows after fetch failure remain clickable under new bucket context).
Caller audit: |
Claude finished @bootjp's task in 2m 18s —— View job Code Review: Phase 5 — S3 Objects Tab (Round 3)
Round 3 fix — verified correct ✅
setPage(null);setCursorStack([]);The placement is correct: both abort guards ( Caller audit for the five call sites (all confirmed safe):
One subtlety worth noting: Confirmed still correct from prior rounds
Remaining open items (unchanged from r2 — all Low/Nit, none block merge)Low — Objects tab still gated on {detail.data&&<S3ObjectsTabbucket={name}/>}A 503 during a Raft leader flip still hides the Objects tab silently. The metadata-card error text gives operators some signal, but list/download would succeed independently. Unchanged from r1. Low — Download anchor + session expiry (
Nit — Breadcrumb {segments.map((seg,idx)=>{consttarget=segments.slice(0,idx+1).join("/")+"/";return<spankey={idx}...>
Nit — Disabled current breadcrumb segment visually identical to clickable ancestors ( Low (pre-existing, not introduced by r3) — Detail modal doesn't reset on bucket change If the operator has a detail modal open for an object in SummaryRound 3 is correct and minimal. The two-line addition ( |
…y (PR #816 r4) Claude review on PR #816 r3 caught one remaining cross-bucket state leak and one breadcrumb nit. 1) Detail modal didn't reset on bucket change. Same React Router reuse-the-same-component-instance class that r2 fixed for prefix: if the operator had an object detail modal open for an object in bucket-a and then navigated to /s3/bucket-b, the modal stayed open referencing the stale bucket-a key. Clicking Delete then called api.deleteObject(bucketB, staleBucketAKey) - which can hit an existing object if the key happens to collide. Fix: the existing useEffect([bucket]) that already reset prefix now also clears detail / confirmDelete / detailError. Same timing rationale - the listAbortRef + the [bucket, prefix] effect's loadPage call together ensure the post-render state is the bucket-b root view with no open modal. 2) Breadcrumb React key was the loop index; swapped to the segment target ("foo/bar/" form). idx is a stable React key when the list neither shrinks nor reorders, but the breadcrumb DOES shrink when the operator drops to a shallower depth - keying on target lets React reuse the right span across renders without churning unrelated DOM. Caller audit (semantic-change rule): - No semantic changes to any function. setDetail / setConfirmDelete / setDetailError are existing useState setters; adding them to the bucket-change effect is a new call site, not a contract change. The signatures, return values, and error vocabulary of every function in S3ObjectsTab are unchanged. Self-review (5 passes): Data loss - closes the modal-bound write-to-wrong-bucket vector (delete + future edit). Same fail-closed shape as the r2 prefix reset. Concurrency - the effect adds three setState calls; React batches them into one re-render. Performance - negligible (three setState calls on a relatively rare bucket-change event). Consistency - the modal close mirrors the existing prefix / cursorStack resets so future audits find one shared pattern. Test coverage - manual exercise confirms the modal closes on /s3/a -> /s3/b navigation, including the case where the modal was in the confirm-delete state. npm run lint - passes npm run build - passes
bootjp
commented
May 23, 2026
@claude review Round 4 addresses the two items from your r3 review:
The other r3-noted Low items (Objects tab gated on |
Claude encountered an error —— View job I'll analyze this and get back to you. |
bootjp
commented
May 23, 2026
@claude review (Re-pinging: the previous review run errored with 'Failed to compute SHA' on S3ObjectsTab.tsx — appears to be a CI git infra issue, not a code one. The r4 commit |
Claude finished @bootjp's task in 2m 7s —— View job Code Review: Phase 5 — S3 Objects Tab (Round 4)
Round 4 fixes — both verified correct ✅1. Detail modal reset on bucket change — ✅
useEffect(()=>{setPrefix("");setDetail(null);setConfirmDelete(false);setDetailError(null);},[bucket]);The fix is correctly placed and covers all three modal-related state slices. The timing argument from r2 still holds: the The three new additions specifically close the cross-bucket delete vector identified in r3: if an operator had a detail modal open for 2. Breadcrumb
<spankey={target}className="flex items-center gap-1">
Confirmed correct from prior rounds
Remaining items (unchanged from r3 — all Low/pre-existing, none block merge)Low — Objects tab still gated on {detail.data&&<S3ObjectsTabbucket={name}/>}A 503 during a Raft leader flip hides the Objects tab silently even though Low — Download anchor + session expiry ( An SummaryRound 4 is correct. Both targeted items (detail modal cross-bucket leak + breadcrumb key stability) are properly fixed with minimal, well-placed changes. The four-round fix progression (abort races r1, cursor-advance r1, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:290e945565
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await api.deleteObject(bucket, detail.key); | ||
| setDetail(null); | ||
| setConfirmDelete(false); | ||
| void loadPage(cursorStack[cursorStack.length - 1], prefix); |
There was a problem hiding this comment.
After a successful delete this reload uses the current continuation token, which can strand the UI on an empty tail page even when earlier pages still contain objects (e.g., 101 objects, delete the only item on page 2). In that case objectCount becomes 0, the tab shows the "bucket is empty" state, and there is no previous-page control to recover within the tab because refresh reuses the same token. This should fall back to an earlier cursor (or page 1) when the post-delete page comes back empty.
Useful? React with 👍 / 👎.
…r5) Codex P2 on PR #816 r4. The onDelete success path reloaded with the same continuation token, which can strand the UI on an empty tail page even when earlier pages still have content. Example: 101 objects, page size 100, operator drills to page 2 (1 object), deletes it. The reload uses the same cursor → scans from after the deleted item → returns empty → tab shows 'bucket is empty' even though page 1 still has 100 objects. Fix: before issuing the reload, check whether the current page would become empty after the local delete (objects.length <= 1 && common_prefixes.length == 0). When true AND the operator is past page 0 (cursorStack non-empty), reset the cursor stack and reload from page 0 so the operator lands on a non-empty view. The common case — page has many entries — keeps the operator's position via the existing cursorStack top. At page 0 the cursor is already undefined and the empty state is unambiguous, so no fallback is needed there. Caller audit (semantic-change rule): onDelete: the only caller is the ObjectDetailModal's Delete button (line 339 in the same file). The semantic change is the reload cursor: defaults to currentCursor (cursorStack top), falls back to undefined under the empty-page condition. No external consumer; the page/cursorStack state both stay internal to S3ObjectsTab. loadPage: the new call site uses (undefined, prefix) — same signature, no behavioural surprise. The cursorStack reset matches the [bucket] effect's reset pattern (Codex P2 fix from r2 already established the 'cursor stack consistency with page state' invariant). Self-review: Data loss: none. Display-state fix; the deleted object is gone either way. Concurrency: setCursorStack + loadPage run in the same handler; no race with parallel updates. Performance: one extra setState call on the empty-page delete path. Consistency: matches r3's fail-closed-on-error shape (clear stale state, show fresh canonical view). Test coverage: SPA test harness absent; manual exercise confirms 101-object scenario shows page 1 after deleting the only page-2 object. npm run lint - passes npm run build - passes
bootjp
commented
May 23, 2026
@claude review Round 5 addresses Codex P2 (delete that empties current page leaves UI on a misleading 'empty bucket' state when earlier pages still have content).
Caller audit: |
Claude finished @bootjp's task in 1m 55s —— View job Code Review: Phase 5 — S3 Objects Tab (Round 5)
Round 5 fix — verified correct ✅
constwouldEmptyPage=(page?.objects?.length??0)<=1&&(page?.common_prefixes?.length??0)===0;if(wouldEmptyPage&&cursorStack.length>0){setCursorStack([]);voidloadPage(undefined,prefix);}else{voidloadPage(cursorStack[cursorStack.length-1],prefix);}The logic is sound. Walking through the key cases: Scenario 1 — targeted case (101 objects, page 2 has 1 object, delete it): Scenario 2 — common case (page has many entries): Scenario 3 — page has 1 object + 3 folders, delete the object: Scenario 4 — page 0 with only 1 object, delete it: Scenario 5 — multi-page bucket, page 2 has 1 object + Implementation notes:
Confirmed correct from prior rounds
Remaining items (unchanged from r4 — all Low/pre-existing, none block merge)Low — Objects tab still gated on {detail.data&&<S3ObjectsTabbucket={name}/>}A 503 during a Raft leader flip hides the Objects tab silently even though Low — Download anchor + session expiry
SummaryRound 5 is correct and minimal. The |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:3da10e1f9a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| {(objectCount > 0 || folderCount > 0) && ( | ||
| <table className="table"> |
There was a problem hiding this comment.
This table keeps rendering page rows even when loading is true, so after changing bucket/prefix the previous context’s rows remain clickable until the new listObjects call finishes. In that window, clicking a stale row opens the modal under the new bucket prop, and Download/Delete can target the wrong bucket/key combination if names overlap. Guarding row rendering during refreshes (or clearing page at request start for context changes) avoids cross-context actions from stale data.
Useful? React with 👍 / 👎.
…sor-on-success (PR #815 r3) Three findings from Claude review on PR #815 r2. 1) Codex P1 (post-r1) - modal not reset on table change. The parent DynamoDetail page uses useApiQuery, which doesn't clear data on dep change, so DynamoItemsTab stays mounted continuously across /dynamo/tables/A -> /dynamo/tables/B navigation. Without an explicit reset, the table prop flips to B but modalKey (set by openView) still holds keyFromA. onSave then calls api.putItem(B, keyFromA, body), silently overwriting a colliding key in table B. Fix: the [table, pageSize] useEffect now inlines the same reset closeModalForce performs (setModalMode/Item/Key/Error all to null). Inlined rather than calling closeModalForce because that helper is declared later in the file - the effect runs at runtime via closure but the inline keeps the reset list visible at the call site. Same cross-context class as the S3 ObjectsTab detail-modal- on-bucket-change fix on PR #816 r4. 2) Codex P2 - React key collision for binary primary keys. describePrimaryKey renders binary attributes as '<binary ~NB>'. Two distinct binary values with the same byte length produce the same string; React sees duplicate keys; reconciliation is undefined and rows can bind to wrong onOpen handlers (cross-row click hijack). Fix: React key is now encodeAdminItemKey(extractPrimaryKey(item, partitionKey, sortKey) ?? {}). encodeAdminItemKey returns the full base64url-encoded JSON of the key map - binary bytes encode verbatim, collision-free within a scan page. 3) Minor - comment promised cursor-advance after success, code advanced optimistically. Now matches: loadPage returns a success boolean (mirrors the S3 ObjectsTab pattern from PR #816 r1), onNextPage awaits and only calls setCurrentCursor on ok=true. A transient Next failure now keeps currentCursor at the prior page; Refresh re-fetches the visible page, not the failed one. Caller audit (semantic-change rule): loadPage: signature changed void -> Promise<boolean>. Five callers - init effect, onNextPage, onRefresh, post-write reload, post-delete reload. Only onNextPage uses the new return value; the other four void-cast or ignore. Adding a return value is strictly additive for the void callers. React key change: internal to the JSX render block; no external consumers. useEffect modal reset: state-setter sequencing; no public API change. Self-review: Data loss: closes a write-to-wrong-table vector on modal save AND a cross-row click-hijack vector on duplicate- binary-key pages. Both real-world failure modes. Concurrency: cursor advance now matches S3 tab - both advance only on success. Performance: encodeAdminItemKey per row is one extra base64+JSON encode (~10-100us); negligible vs. the scan latency. Consistency: DynamoItemsTab now matches S3ObjectsTab on all three patterns (success-only cursor advance, modal reset on parent context change, collision-free row keys). Test coverage: SPA test harness absent; manual exercise confirms /dynamo/A -> /dynamo/B closes any open modal, and a binary-keyed table renders distinct rows for each key. npm run lint - passes npm run build - passes
…d (PR #816 r6) Codex P1 on PR #816 r5. The bucket / prefix change effect set listAbortRef to cancel in-flight requests but left the page state populated, so during the listObjects load window the previous context's rows remained visible and clickable. A click in that window opens the detail modal carrying the old bucket-a object key while the bucket prop has already flipped to bucket-b — Download / Delete then target (bucket=b, key=staleKeyFromA), a cross-bucket data-integrity vector when key paths overlap. Fix: the [bucket, prefix] useEffect now calls setPage(null) synchronously before issuing loadPage. Stale rows disappear the instant the context changes; the table renders the loading state during the new fetch. Side effects: - Refresh / Next / post-write reloads are unaffected; they bypass this effect and call loadPage directly. - Initial mount is unaffected (page is already null). - A bucket / prefix change shows 'Loading...' instead of the previous context's rows for the network round-trip duration; this is the intended fail-closed UX. Caller audit (semantic-change rule): loadPage signature unchanged. The useEffect adds one setPage(null) call before the existing setCursorStack + loadPage sequence. Page-state semantics unchanged: null still means 'no data loaded'; setPage(result) on success still populates rows. The only new behaviour is that the null window now spans the full context-change → loadPage- resolve duration instead of just up to loadPage's first setState. No external consumer of the page state. Same fail-closed shape the prefix-reset (r2) and modal- reset (r4) effects established. Future context changes that touch bucket/prefix automatically get the same stale-row protection. npm run lint - passes npm run build - passes
bootjp
commented
May 23, 2026
@claude review Round 6 addresses Codex P1 (stale rows clickable during context-change loading window). The Caller audit: |
Claude finished @bootjp's task in 2m 15s —— View job Code Review: Phase 5 — S3 Objects Tab (Round 6)
Round 6 fix — verified correct ✅
useEffect(()=>{setPage(null);// <-- new in r6setCursorStack([]);voidloadPage(undefined,prefix);// eslint-disable-next-line react-hooks/exhaustive-deps},[bucket,prefix]);Execution sequence is correct. All four synchronous setters in the effect body (and at the start of
Because all four setters execute synchronously before any The cross-bucket click vector is fully closed. Without r6, the window between a bucket change and Refresh / Next / post-write call sites are unaffected. Those call Initial mount is a no-op. Interaction with the Confirmed correct from all prior rounds
Remaining items (pre-existing, out of scope — confirmed in trigger comment)Low — Objects tab still gated on {detail.data&&<S3ObjectsTabbucket={name}/>}Pre-existing pattern shared with the rest of the SPA; not introduced by this PR. Low — Download anchor + session expiry
SummaryRound 6 is correct and minimal. The one-line addition (
Ready to merge after #814 lands. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:f01525f097
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // would 400 before reaching the storage layer. | ||
| const key = prefix + file.name; | ||
| await api.putObject(bucket, key, file, file.type || "application/octet-stream"); | ||
| void loadPage(cursorStack[cursorStack.length - 1], prefix); |
There was a problem hiding this comment.
After await api.putObject(...), this callback reloads using the prefix and cursorStack captured when the file was selected, not the current view context. If an operator navigates to another prefix or bucket while a long upload is in flight, the completion handler can repopulate page with the old listing, leaving rows that no longer match the breadcrumb/route and enabling follow-up download/delete actions against unintended keys. Reload should use the latest bucket/prefix/cursor (or skip if context changed) before mutating list state.
Useful? React with 👍 / 👎.
…mount (PR #815 r4) Codex P2 on PR #815 r3. useApiQuery does NOT clear data on dep change, so a /dynamo/tables/A -> /dynamo/tables/B navigation left the DynamoDetail page rendering with the OLD detail.data (name=A, partition_key/sort_key from A's schema) for the brief window until the new describe response landed. During that window: <DynamoItemsTab table={name=B} partitionKey={detail.data.partition_key from A} sortKey={detail.data.sort_key from A} /> The tab dispatched a scan against table B but the extractPrimaryKey calls inside used A's partition/sort key names — wrong attributes extracted, edit/delete derivation failed (or, worse, targeted unrelated columns). The r3 internal modal reset (DynamoItemsTab.tsx [table, pageSize] effect) handled the case where the tab itself stayed mounted across table changes, but the props were still stale. This fix moves the boundary one level up: the parent gates the mount on detail.data.name === name so the tab never sees a table/schema mismatch. The key={name} prop forces a fresh component instance on route change so the tab's internal state also resets cleanly. Caller audit (semantic-change rule): No function signatures changed. The gating condition narrows the render predicate; any consumer downstream of the tab is unaffected (the tab still receives consistent table/key props or doesn't mount at all). The key={name} remount means the tab's useState slices reset on navigation rather than carrying across — same fail-closed shape as the [bucket] effect in S3ObjectsTab. Self-review: Data loss: closes the wrong-table edit/delete vector during the describe-loading window. Concurrency: no shared state changes. Performance: one extra prop comparison per render; remount discards the tab's in-flight scan via the existing AbortController cleanup effect. Consistency: matches the S3DetailPage pattern (where S3ObjectsTab is already mounted via {detail.data && ...}) once that gets the same name-match guard. Filing the parallel s3 fix as a separate one-liner on the #816 branch since the same useApiQuery race applies there. Test coverage: SPA test harness absent; manual exercise confirms /dynamo/A -> /dynamo/B no longer renders A's item rows under B's table prop. npm run lint - passes npm run build - passes
…ey remount (PR #816 r7) Parallel of the PR #815 r4 fix on the Dynamo side. useApiQuery keeps the previous bucket's detail.data around while a new fetch is in flight, so a /s3/A -> /s3/B navigation briefly mounts S3ObjectsTab with bucket=B while the describe response for B is still loading. During that window: - The tab's own [bucket] effect (added in r2/r4) resets prefix, modal state, and cursor. - The [bucket, prefix] effect (added r6) clears page to hide stale rows during the load. Those internal protections all fire on the bucket prop change. But the parent S3DetailPage was still rendering the tab with detail.data from the OLD bucket - the tab's internal protections re-fire correctly, but for one render cycle the operator could observe stale-context content under the new route. Gating on bucket_name === route name eliminates that window entirely; key={name} forces a clean component instance on navigation so the tab's AbortController cleanup cancels any in-flight scan. Caller audit (semantic-change rule): No function signatures changed. The gating condition narrows the render predicate. key={name} swap remounts the tab cleanly on route change; the tab's existing cleanup effect handles the in-flight abort. No external consumer downstream. npm run lint - passes npm run build - passes
bootjp
commented
May 23, 2026
Claude finished @bootjp's task in 2m 34s —— View job Code Review: Phase 5 — S3 Objects Tab (Round 7)
Round 7 fix — verified correct ✅
// Before{detail.data&&<S3ObjectsTabbucket={name}/>}// After{detail.data&&detail.data.bucket_name===name&&(<S3ObjectsTabkey={name}bucket={name}/>)}The bug being closed:
Why Once this guard is in place, the Objects tab is simply not rendered during the stale-data window. Why With the gate in place, the tab is unmounted during the B-loading window and remounted when B's describe arrives. The Interaction with the r2/r4 With Interaction with r6 The r6 Parallel to PR #815 r4: The shape is identical to the DynamoDB side ( Nit — Multi-line comment in JSX
{/* Tab suppressed during describe-loading window; key forces clean remount on route change */}<S3ObjectsTabkey={name}bucket={name}/>Not a blocker — the commit message covers it thoroughly. Confirmed correct from all prior rounds
Remaining items (pre-existing, out of scope)Low — Objects tab hidden on non-404 describe error (
Low — Download anchor + session expiry (
SummaryRound 7 is correct and complete. The |
…(PR #815 r5) Codex P1 on PR #815 r4 caught the page-size-change case my r4 parent-level key remount didn't cover. The table-switch case is handled by DynamoDetail's key={name} remount (full unmount + remount, page is freshly null on first render), but pageSize changes keep the same component mounted; without an explicit setPage(null) in the [table, pageSize] effect, the previous page-size's rows stay visible during the new scan's loading window. A click in that window opens the modal with rows that don't match the page-size context the operator just switched to. Fix: add setPage(null) to the effect alongside the existing modal/cursor resets. Mirrors the S3 ObjectsTab [bucket, prefix] effect from PR #816 r6. Caller audit: no signature changes. setPage(null) is one new synchronous setter before the existing loadPage(undefined) call. The component still treats page=null as 'no data' uniformly. npm run lint - passes npm run build - passes
…igation (PR #816 r8) Codex P1 on PR #816 r7. onFilePicked is an async closure that can outlive a folder/bucket navigation when the upload is large. The previous code: await api.putObject(bucket, key, file, ...) void loadPage(cursorStack[cursorStack.length - 1], prefix) uses closure-captured prefix/cursorStack — so a navigation during the upload would have the completion handler reload the OLD prefix's listing under the NEW context. The bucket case is mostly covered by the parent's key={name} remount (unmount cancels via the AbortController), but prefix changes within the same bucket keep the component mounted. Fix: 1. Snapshot uploadBucket / uploadPrefix / uploadCursor at click time (defines the upload's intent). 2. contextRef updates on every render to hold the LATEST bucket/prefix — the standard React-async-callback ref pattern; closures see stale state, refs see current. 3. After the upload resolves, compare snapshot to current via contextRef. If matched, reload normally. If diverged, skip the reload AND the error surface — the [bucket, prefix] effect has already kicked off the right scan for the operator's new context, and surfacing an error for an upload they walked away from would attach to an unrelated screen. The successful PUT against (uploadBucket, uploadPrefix+key) is durable regardless — only the listing refresh is gated. Caller audit: onFilePicked is only called from the hidden <input type='file'> onChange (file picker). No external consumer; the closure semantics shift is internal. contextRef pattern is the same as the listAbortRef / scanAbortRef pattern used elsewhere in the file — refs updated on every render to let async callbacks see current state. No new abstraction introduced. npm run lint - passes npm run build - passes
bootjp
commented
May 23, 2026
@claude review Round 8 addresses Codex P1: post-upload reload using closure-captured prefix/cursorStack.
contextRef is the standard ref-on-every-render pattern; mirrors the listAbortRef / scanAbortRef shape elsewhere in the file. |
Claude finished @bootjp's task in 2m 2s —— View job Code Review: Phase 5 — S3 Objects Tab (Round 8)
Round 8 fix — verified correct ✅
The
constcontextRef=useRef({ bucket, prefix });contextRef.current={ bucket, prefix };The assignment on every render (outside Snapshot at click time: constuploadBucket=bucket;constuploadPrefix=prefix;constuploadCursor=cursorStack[cursorStack.length-1];Three variables locked to the upload's intended context. Post-success gate: constcur=contextRef.current;if(uploadBucket===cur.bucket&&uploadPrefix===cur.prefix){voidloadPage(uploadCursor,uploadPrefix);}If the operator navigated prefix mid-flight (same bucket, different folder), Post-error gate — same check: constcur=contextRef.current;if(uploadBucket===cur.bucket&&uploadPrefix===cur.prefix){setUploadError(...);}Correct: silently dropping the error banner when the operator is on a different screen is the right UX choice. The upload either succeeded (durable on the server) or failed (the operator can retry from the correct context). An error banner attached to an unrelated screen would be confusing. ✅ Bucket-change case (belt-and-braces):
One nit — comment style (pre-existing pattern, not r8-specific)
The comments in r8 (and throughout the file from prior rounds) reference PR rounds and Codex issue numbers inline ( Confirmed correct from all prior rounds
Remaining items (pre-existing, out of scope)Low — Objects tab hidden on non-404 describe error ( A 503 during a Raft leader flip hides the Objects tab silently even though list/download would succeed independently. Pre-existing pattern; the r7 gate ( Low — Download anchor + session expiry (
SummaryRound 8 is correct. The |
…#818 r1) Two style notes from Gemini medium x2 / Claude bot on PR #818. 1) CLAUDE.md violation: the inline comment referenced PRs #813/#814/#815/#816 and 'today' — transient context that rots and doesn't belong in source comments. Per CLAUDE.md: 'Don't reference the current task, fix, or callers ... those belong in the PR description'. The commit message and PR description still carry the cross- PR diagnostic context for git archeology. 2) Eventually deadline now derives from initialTTL (initialTTL + 3*time.Second) rather than a hardcoded 5s. If a future TTL adjustment lands, the assertion window stays valid without a parallel update. Trimmed comment focuses on the technical justification: - WHY 2s TTL (SET-ack latency on 3-node Raft under -race) - WHY ttl+3s deadline (poll cadence + Raft-replicated DEL latency on expiry) No behavior change to the test. initialTTL=2s + deadline=5s (2+3) matches the previous 2s + 5s. Tested locally with -race -count=2 (7.4s wall) — passes both runs. go test -race -count=2 -timeout=120s ./adapter/... in scope: passes golangci-lint: clean
…I flake (#818) ## Summary Fixes the recurring `TestRedis_ExpiredKey_BecomesInvisible` CI flake that has been blocking unrelated admin PRs (#813, #814, #815, #816) all day. ## Root cause The test sets a key with `PX 200` (200ms TTL) then immediately reads it back: ```go require.NoError(t, rdb.Do(ctx, "SET", "expiry:short", "v", "PX", "200").Err()) got, err := rdb.Get(ctx, "expiry:short").Result() require.NoError(t, err) // ← fails: redis.Nil require.Equal(t, "v", got, "key must be visible before expiry") ``` The `SET` goes through a 3-node Raft cluster: client → leader proposes → quorum apply → leader applies → response. Under `-race` on CI runners this round-trip can take 100–250ms. The TTL clock starts at the moment the leader applies, so by the time `SET` returns OK on a slow CI runner, the key may have <50ms of TTL left. The immediately-following `GET` then loses the race and hits `redis.Nil`. That this is the bug — not anything in any of #813-#816 — is locked in by the cross-PR failure pattern: the flake fired today on PRs touching only `internal/admin/` and `web/admin/`, with zero code overlap with `adapter/`. ## Fix `PX 200 → PX 2000`. 2s is comfortably past the worst observed SET-ack latency (~250ms on CI) while keeping the test under a 7s wall-clock budget (2s TTL + 5s `Eventually` deadline, raised from 1s). The test's intent — verify expired keys become invisible to subsequent GETs — is preserved exactly. Only the timing constants change; the assertions are unchanged. ## Self-review (5 passes) 1. **Data loss** — none. Test-only constants. 2. **Concurrency** — closes a wall-clock vs. Raft-apply-latency race in the test fixture. 3. **Performance** — test now takes ~3s instead of ~250ms locally; still well under the 30s package timeout. 4. **Consistency** — the broader test still asserts both halves (visible before expiry, invisible after expiry) of the TTL contract. 5. **Test coverage** — no change to coverage; same assertions, friendlier timing. ## Test plan - [x] `go test -race -count=3 -timeout=180s -run TestRedis_ExpiredKey_BecomesInvisible ./adapter/` — passes 3/3 (10.792s) - [x] `golangci-lint --config=.golangci.yaml run` — clean - [ ] CI ## Context Discovered while running the PR review loop on #813-#817. The Claude bot's Round 8 review on #813 marked the PR as "Ready to merge once CI passes" — only the Redis flake was blocking. This fix unblocks #813 (and the parallel admin PRs) without touching any admin code. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Improved reliability of the Redis TTL compatibility test by adjusting timing parameters to reduce intermittent test failures. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/bootjp/elastickv/pull/818?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
](https://app.coderabbit.ai/change-stack/bootjp/elastickv/pull/818?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
…ix CI flake (#819) ## Summary Fixes the recurring `TestSQSServer_Throttle_NoOpSetQueueAttributesPreservesBucket` CI flake that has been hitting the same admin PR series (#813, #815, #816) that #818 just unblocked. ## Root cause The test sequence: 1. `SetQueueAttributes` (capacity=10, refill=1/sec) 2. **10 drain sends** (drain the bucket) 3. Sanity-check send → expects 400 (drained) 4. `SetQueueAttributes` with identical values (no-op) 5. **Post-no-op send → expects 400** ← intermittently returns 200 Each step goes through a real Raft propose+apply. Under `-race` on slow CI runners each round-trip takes 100–250ms. Total wall-clock from start to step 5 reaches 1.5–2.5s. At **1 token/sec** refill, by step 5 the bucket has accumulated 1+ tokens and the send returns 200 — falsely indicating a no-op-invalidate-bypass regression. The test's intent — verify that a no-op `SetQueueAttributes` does not reset bucket state — is independent of the refill rate. Slowing the refill removes the race without changing what's tested. ## Fix `refill 1/sec → 0.01/sec` (1 token per 100s). Even a 10s test window can't accumulate to a whole token. The throttle config validator (`adapter/sqs_catalog.go:163` `SendRefillPerSecond float64`) accepts fractional values. `0.01 != 0` so `IsEmpty` returns false and the throttle path is still exercised — only the refill cadence changes. ## Self-review (5 passes) 1. **Data loss** — none; test-only constant. 2. **Concurrency** — closes a wall-clock vs. refill-rate race in the test fixture (same shape as the Redis TTL fix in #818). 3. **Performance** — no runtime change; the test still completes in ~1–2s. 4. **Consistency** — the test still verifies the no-op-preserves-bucket invariant identically. The refill rate isn't load-bearing for the assertion. 5. **Test coverage** — unchanged. Same drain → sanity → no-op → recheck sequence; only the refill cadence differs. ## Test plan - [x] `go test -race -count=2 -timeout=120s -run TestSQSServer_Throttle_NoOpSetQueueAttributesPreservesBucket ./adapter/` — passes both (2.1s) - [x] `golangci-lint --config=.golangci.yaml run` — clean - [ ] CI ## Context Second flake-fix PR today (after #818 for Redis TTL). The admin PR series (#813, #815, #816) keeps hitting unrelated adapter-package flakes because the admin code lives in `internal/admin/` / `web/admin/` but `go test ./...` runs the whole tree. Each fix is small and scoped.
Resolves conflict in web/admin/src/api/client.ts: keep both helpers from the parallel PRs. - base64UrlEncodeBytes (from this branch, #816): takes a string, used internally to build S3 object key URL segments. - encodeAdminItemKey (from main via #815): takes a primary-key attribute map, used by the Dynamo items methods + exported for DynamoItemsTab's scan-cursor encoder. Refactored encodeAdminItemKey to delegate to base64UrlEncodeBytes (JSON-stringify the map, then byte-encode) — both helpers share the same wire shape (unpadded base64-url alphabet), so the only difference is the input type. One implementation, two entry points.
bootjp
commented
May 23, 2026
Resolved the client.ts conflict after #815 merged. Kept both helpers:
Refactored npm run lint / npm run build pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:5c0b19d2b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (uploadBucket === cur.bucket && uploadPrefix === cur.prefix) { | ||
| void loadPage(uploadCursor, uploadPrefix); |
There was a problem hiding this comment.
After a successful upload, the tab always refreshes using uploadCursor, which was captured when the file was selected. If the user stays in the same bucket/prefix but navigates to a different page while the upload is in flight, the completion handler will jump the listing back to the old page and overwrite the user’s newer pagination state. This is a stale-context regression similar to the bucket/prefix race, but scoped to in-prefix pagination.
Useful? React with 👍 / 👎.
…fix (PR #816 r9) Codex P2 on PR #816 r8. The r8 contextRef pattern guarded the post-upload reload on bucket/prefix mismatch but missed the in-prefix pagination case. If the operator clicked Next while the upload was in flight, contextRef.current.bucket and .prefix still matched (same bucket/prefix), so the gate fell through and loadPage was called with the captured uploadCursor — jumping the listing back to the upload's origin page and clobbering the operator's later pagination state. Fix: track currentCursor in contextRef alongside bucket/prefix, and add the equality check for all three axes. The post-upload reload now only fires when the operator is still on the exact (bucket, prefix, page) the upload targeted. Same fail-closed shape as r8 for bucket/prefix: a successful PUT stays durable on the server; the listing refresh is purely a UX nicety and skipping it under contention is the right call. The error-surface gate (catch path) gets the same three-axis check for symmetry — an upload error on a page the operator has left wouldn't make sense to attach. Caller audit: onFilePicked is only invoked from the file-input onChange. contextRef.current.cursor is updated on every render via the existing assignment line; the new field is additive. No external consumer of contextRef. npm run lint - passes npm run build - passes
bootjp
commented
May 23, 2026
@claude review Round 9 addresses Codex P2: r8's contextRef gated the post-upload reload on bucket/prefix mismatch but missed the in-prefix pagination case. If the operator clicked Next mid-upload, the gate fell through and Fix: track cursor in contextRef alongside bucket/prefix; gate on all three axes. Both success and error paths use the same three-axis check. Same fail-closed shape as r8 — successful PUT stays durable, listing refresh is purely UX. Caller audit: onFilePicked is the only consumer; contextRef field is additive. |
Claude finished @bootjp's task in 1m 43s —— View job Code Review: Phase 5 — S3 Objects Tab (Round 9)
Round 9 fix — verified correct ✅
constcontextRef=useRef({ bucket, prefix,cursor: ""asstring|undefined});contextRef.current={ bucket, prefix,cursor: cursorStack[cursorStack.length-1]};The Three-axis gate (success path): constcur=contextRef.current;if(uploadBucket===cur.bucket&&uploadPrefix===cur.prefix&&uploadCursor===cur.cursor){voidloadPage(uploadCursor,uploadPrefix);}Walking through the scenarios:
The The error path has the identical three-axis guard. No asymmetry between success and error paths. ✅ One nit — initial cursor value
constcontextRef=useRef({ bucket, prefix,cursor: ""asstring|undefined});The initial value uses constcontextRef=useRef<{bucket: string;prefix: string;cursor: string|undefined}>({ bucket, prefix,cursor: undefined});Not a blocker. Confirmed correct from all prior rounds
Remaining items (pre-existing, out of scope)Low — Objects tab hidden on non-404 describe error ( A 503 during a Raft leader flip hides the Objects tab silently even though Low — Download anchor + session expiry (
SummaryRound 9 is correct. The |
Uh oh!
There was an error while loading. Please reload this page.
## Summary **Phase 6** — close the loop on the Admin Data Browser rollout: - `git mv docs/design/2026_05_22_proposed_admin_data_browser.md → _implemented_…` - Status: `Proposed` → `Implemented` - Add an Implementation record table at the top of the doc with one row per PR plus the in-flight design adjustments that came out of PR review - Rollout plan table grows an Outcome column with the PR number for each phase ## Sequencing This PR should merge **last**, after Phases 2a (#805 merged), 2b (#811 merged), 3a (#813), 3b (#814), 4 (#815), 5 (#816) have all landed. The doc rename codifies the completion, so jumping it ahead would be misleading. ## Why two commits The first commit (`82d9387d`) carried the `git mv` but missed the content edits because they happened after the `git add`. The second commit (`bb7b77be`) adds the content. They're kept separate per CLAUDE.md's "never amend, prefer new commits" rule — both are tracked in the same PR. ## What changed in the doc Implementation record table: | Phase | What landed | PR | |-------|-------------|----| | 2a | DynamoDB item adapter RPCs | #805 | | 2b | S3 object adapter RPCs | #811 | | 3a | DynamoDB item HTTP + bridge | #813 | | 3b | S3 object HTTP + bridge | #814 | | 4 | SPA DynamoDetail Items tab | #815 | | 5 | SPA S3Detail Objects tab + Upload | #816 | | 6 | This rename | #817 | Notable in-flight design adjustments documented: - Empty `L` / `M` Dynamo attributes preserved via custom `MarshalJSON` (Gemini medium, Codex P1 on #813) - Path-segment validation decodes via `url.PathUnescape` before validating; URL-encoded table names work, decoded `/` / `..` still rejected (Codex P1 on #813) - The HTTP layer cannot reject body-declares-extra-key-columns; principled fix tracked separately (Codex P2 on #813) - SPA bundle output stays `.gitignored`; CI rebuilds from source ## Test plan - [x] `git mv` preserves history (rename detection 100% similarity on the first commit) - [x] No backend touched - [x] `go test -count=1 ./...` — passes (no code changes) - [ ] CI ## Phase plan (final state) - Phase 2a: merged (#805) - Phase 2b: merged (#811) - Phase 3a: open (#813) — to merge before this - Phase 3b: open (#814) — to merge before this - Phase 4: open (#815) — to merge before this - Phase 5: open (#816) — to merge before this - **Phase 6: this PR — to merge LAST** <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated admin data browser implementation documentation with shipped phases and PR references. * Added implementation record detailing completed phases and notable design adjustments. * Enhanced rollout plan with outcome tracking for each completed phase. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/bootjp/elastickv/pull/817?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
](https://app.coderabbit.ai/change-stack/bootjp/elastickv/pull/817?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
Summary
Phase 5 of
docs/design/2026_05_22_proposed_admin_data_browser.md— adds the Objects tab to the S3 bucket detail page so operators can list / download / upload / delete objects through the admin SPA. Consumes the Phase-3b HTTP surface from PR #814.Stacked on
This branch is built on
feat/admin-http-s3-objects(PR #814). Merge order: #814 → this PR. CI compiles cleanly here because the Phase-3b server endpoints live in the parent branch.Capabilities
read_onlyfullUI
S3Detailwith a paginated table (page size 100, mirroring the server default).CommonPrefixes(delimiter fixed to/). Folder rows render with a 📁 prefix and clicking drills into them.downloadattr) + Delete (full role).<input type="file">triggered by an Upload button. File uploads to{prefix}/{file.name}with its native Content-Type.API client
web/admin/src/api/client.tsgrows:AdminObject/AdminObjectListing/AdminListObjectsOptionstypes.listObjects/deleteObject(JSON viaapiFetch).putObject/downloadObjectURL(bypassapiFetchbecause they carry raw bytes).putObjectdoes a manualfetch()so the body can be aBlob; CSRF header still attached.downloadObjectURLreturns the URL for an<a download>link so the browser streams the response and honours the server'sContent-Dispositionheader.base64UrlEncodeBytes: TextEncoder → btoa → url-safe alphabet, padding stripped.Server-side rules honoured
http.MaxBytesReaderand the adapterErrAdminUploadTooLargesentinel layers — both surface as 413.listObjectsunder the new prefix rather than client-side filtering — keeps the page bounded at 100 entries regardless of bucket size.Self-review (5 passes)
busyblocks dismissal during save. Upload button disabled while in flight.Test plan
npm run lint(tsc) — passesnpm run build(vite) — passesgo test -count=1 ./internal/admin/...— passes (no backend changes)Phase plan
_proposed_→_implemented_