fix(admin/spa): drop redundant abort guard and add Modal a11y#650
fix(admin/spa): drop redundant abort guard and add Modal a11y#650
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 58 minutes and 16 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the Modal component with accessibility features, including ARIA roles, focus trapping, and focus restoration. It also simplifies error handling in useApiQuery by removing redundant abort signal checks. A significant issue was identified in the Modal component's focus management logic: because the effect is dependent on the busy state, focus is reset to the first element whenever the modal's busy status changes, which disrupts the user experience during form submissions or other asynchronous actions.
| useEffect(() => { | ||
| if (!open) return; | ||
|
|
||
| previouslyFocusedRef.current = (document.activeElement as HTMLElement | null) ?? null; | ||
| // Focus the first focusable element so keyboard users land | ||
| // inside the dialog instead of the page underneath. Run on the | ||
| // next tick so the dialog DOM exists and any autofocus has had | ||
| // a chance to settle. | ||
| queueMicrotask(() => focusFirstFocusable(dialogRef.current)); | ||
|
|
||
| const onKey = (e: KeyboardEvent) => { | ||
| if (e.key === "Escape" && !busy) onClose(); | ||
| if (e.key === "Escape" && !busy) { | ||
| onClose(); | ||
| return; | ||
| } | ||
| if (e.key !== "Tab") return; | ||
| const root = dialogRef.current; | ||
| if (!root) return; | ||
| const focusables = focusableElements(root); | ||
| if (focusables.length === 0) { | ||
| // Empty dialog: keep Tab from leaving via the page. | ||
| e.preventDefault(); | ||
| return; | ||
| } | ||
| const first = focusables[0]; | ||
| const last = focusables[focusables.length - 1]; | ||
| const active = document.activeElement; | ||
| if (e.shiftKey && active === first) { | ||
| e.preventDefault(); | ||
| last.focus(); | ||
| } else if (!e.shiftKey && active === last) { | ||
| e.preventDefault(); | ||
| first.focus(); | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener("keydown", onKey); | ||
| return () => window.removeEventListener("keydown", onKey); | ||
| return () => { | ||
| window.removeEventListener("keydown", onKey); | ||
| // Restore focus to whoever opened the dialog. Guard against | ||
| // the trigger being unmounted (e.g. the dialog deleted the | ||
| // row whose button opened it) by checking isConnected. | ||
| const restore = previouslyFocusedRef.current; | ||
| previouslyFocusedRef.current = null; | ||
| if (restore && restore.isConnected) { | ||
| restore.focus(); | ||
| } | ||
| }; | ||
| }, [open, busy, onClose]); |
There was a problem hiding this comment.
The focus management logic is currently bundled with the busy and onClose dependencies. This causes the focus to be reset to the first element and the "previously focused" element to be captured again every time the modal's busy state changes (e.g., during a save operation). This results in a poor user experience where focus jumps back to the start of the form unexpectedly and causes focus flicker for screen readers.
Focus capture and restoration should be tied strictly to the open state transition, while the keyboard listener can remain reactive to busy and onClose changes.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b36cfed7b4
ℹ️ 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".
| return () => { | ||
| window.removeEventListener("keydown", onKey); | ||
| // Restore focus to whoever opened the dialog. Guard against | ||
| // the trigger being unmounted (e.g. the dialog deleted the | ||
| // row whose button opened it) by checking isConnected. | ||
| const restore = previouslyFocusedRef.current; | ||
| previouslyFocusedRef.current = null; | ||
| if (restore && restore.isConnected) { | ||
| restore.focus(); |
There was a problem hiding this comment.
The effect cleanup restores focus unconditionally, but this cleanup also runs whenever dependencies change while the modal is still open (not just on close). In this codebase, onClose is passed inline in modal callers and busy can toggle during actions, so re-renders while open will run this block and move focus out of the dialog mid-interaction, breaking keyboard flow and screen-reader context. Focus restoration should be gated to the open→closed transition (or split into a dedicated close-only effect).
Useful? React with 👍 / 👎.
|
@claude review |
|
Claude finished @bootjp's task in 2m 11s —— View job Code Review
|
bootjp
force-pushed
the
feat/admin-dashboard-spa
branch
from
6349276 to
e3b9555
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@claude review 685f410 addresses the Modal focus-management bug from your previous review. The single
The dist/ placeholder |
|
Claude finished @bootjp's task in 1m 28s —— View job Code Review
|
Two follow-ups from the review of #649: 1. useApi.ts: removed the `if (ctrl.signal.aborted) return;` check inside the catch handler. The cleanup function sets `cancelled = true` *before* calling `ctrl.abort()`, so any abort path is already covered by the preceding `cancelled` check. The second guard was harmless belt-and-suspenders dead code; the comment now records why one check is enough. 2. Modal.tsx: added the WAI-ARIA dialog contract (`role="dialog"`, `aria-modal="true"`, `aria-labelledby` pointing at the title) and a focus trap: - On open: store the previously-focused element, then focus the first focusable inside the dialog so keyboard users do not stay stranded on the page underneath. Empty dialogs fall back to focusing the dialog container itself with tabindex=-1. - Tab / Shift+Tab cycle within the dialog instead of escaping to the page underneath. - On close: restore focus to whoever opened the dialog (skipped when that element has been unmounted, e.g. the dialog deleted the row whose button opened it). - Escape and backdrop click stay wired through the existing `busy` gate so a half-applied operation cannot be dismissed. Implementation is vanilla DOM querying — focus-trap-react / react-focus-lock would have added 5–7 KB gzip for what is a ~40 line vanilla solution. Bundle delta from this PR is +0.4 KB gzip (60.23 -> 60.59 KB). Verified with `npm run build` and `npm run lint` (tsc --strict --noUnusedLocals --noUncheckedSideEffectImports), and `go build ./...`. Stacked on top of #649 (feat/admin-dashboard-spa); merge that one first, then this rebases cleanly onto main.
Claude review on b36cfed caught a real focus-management bug: the single useEffect in Modal.tsx mixed two invariants — focus capture/restore (open) and the keyboard handler (open + busy + onClose) — into one effect with deps [open, busy, onClose]. Concrete failure: user clicks Save, parent flips busy=true. React runs the cleanup, which removes the keydown listener AND restores focus to the previously-focused element (the trigger button, outside the dialog). The effect then re-runs, captures the trigger button as the NEW restore target, and snaps focus back. Net: focus flickers out and back on every busy toggle, previouslyFocusedRef gets clobbered with the wrong element, and screen readers announce the dialog as briefly "exited" mid-operation. Fix: split into two effects. - Focus capture/restore — deps [open] only. Runs once on open, once on close. previouslyFocusedRef stays valid for the whole dialog lifetime. - Keyboard handler — deps [open, busy, onClose]. Re-binds when any of those change; only observable side effect is one window listener swap. All three automated reviewers (Claude, Gemini, Codex) flagged this. The split is the standard React idiom — each invariant owns its own state slice. The dist/ placeholder index.html is intentionally NOT regenerated (per .gitignore: assets are built at deploy time, not committed).
bootjp
force-pushed
the
feat/admin-dashboard-spa-followups
branch
from
685f410 to
086480c
Compare
|
Claude finished @bootjp's task in 1m 22s —— View job Code Review
|
Per `docs/design/README.md`'s lifecycle convention, the admin dashboard design doc is now "partial": - **P1** (DynamoDB CRUD + AdminForward) — shipped via #634, #635, #644, #648 - **P2** (S3 buckets list/create/delete/ACL + DescribeTable) — shipped via #658, with #669 + #673 in flight - **P3** (React SPA + embed) — shipped via #649, #650 - **P4** (TLS / role / CSRF / operator docs) — TLS, role, CSRF are already live in P1; operator docs in #674 Independent of the in-flight slice 2 PRs (#669/#673) and the docs PR (#674) — this rename only reflects what is already on main today, plus an "Implementation status" table mapping each phase to the PR it landed in. ## What this PR changes - `git mv` the design doc from `2026_04_24_proposed_admin_dashboard.md` to `2026_04_24_partial_admin_dashboard.md` so its history follows - Add an "Implementation status" header table indexing each phase to the PRs that landed it - List the outstanding open items so future readers know what is still owed against the original proposal: - AdminForward acceptance criterion 5 (rolling-upgrade compat flag) — deferred - S3 object browser — explicitly out of scope per Section 2 Non-goals - TLS cert hot-reload — restart-to-rotate is the documented model When the rolling-upgrade flag lands, the doc gets renamed once more to `2026_04_24_implemented_admin_dashboard.md` per the README's lifecycle convention.
## Summary Phase **3.B** of [`docs/design/2026_04_24_partial_sqs_compatible_adapter.md`](https://github.com/bootjp/elastickv/blob/main/docs/design/2026_04_24_partial_sqs_compatible_adapter.md) §16.4. Adds the AWS SQS **query protocol** (form-encoded request, XML response) alongside the existing JSON protocol on the same listener — older `aws-sdk-java` v1, `boto < 1.34`, and AWS CLI clients can now talk to elastickv without modification. Detection happens per-request via `Content-Type` / `X-Amz-Target` / `Action` presence; no flag, no separate port. See the new proposal doc committed alongside. This is an **architectural proof PR**: dispatch + decoding + encoding + error envelope + the `*Core` refactor pattern are all in place, with **three verbs** wired end-to-end as concrete proof. Each follow-up verb is a parser + response struct + one switch arm — no further design work needed. ### Verbs in this PR | Verb | Why it's in the proof set | |---|---| | `CreateQueue` | Exercises the `Attribute.N.{Name,Value}` indexed-collection parser. | | `ListQueues` | Exercises the repeated-element XML response shape. | | `GetQueueUrl` | Exercises the `<ErrorResponse>` envelope path via `QueueDoesNotExist`. | Every other verb returns a structured **501 `NotImplementedYet`** XML envelope so operators see the gap explicitly. `SendMessage` / `ReceiveMessage` / `DeleteMessage` are the highest-priority follow-ups (they need the same `*Core` refactor on the FIFO send loop). ### Key design points - **No new listener / no flag.** `pickSqsProtocol(*http.Request)` decides per request. JSON and Query share the SQS port and the SigV4 path. - **Wire-format-free cores.** `createQueue` / `listQueues` / `getQueueUrl` are now `decode → core → encode` with `core(ctx, in) (out, error)`. The JSON wrappers are unchanged in behavior; existing JSON tests pass without modification. - **DoS protection inherited.** Body read is bounded by the same `sqsMaxRequestBodyBytes` the JSON path uses. - **SigV4 unchanged.** The signed canonical request includes the form-encoded body, so the existing SigV4 middleware verifies query requests without code changes. - **Error parity.** `<Code>` reuses the existing `sqsErr*` constants. HTTP status mirrors what the JSON path returns, so SDK retry classifiers work across protocols. - **Cyclomatic budget honoured.** `handle()` was refactored to extract `handleQueryProtocol` — `cyclop ≤ 10` per project rules, no `//nolint`. ### Known limitation (design §11.4) `proxyToLeader`'s error writer always emits the JSON envelope, so a query-protocol client hitting a follower during a leader flip sees one JSON error before retry lands on the new leader. Follow-up PR threads the detected protocol onto the request context so the proxy emits matching XML. ## Test plan - [x] `go build ./...` — clean - [x] `go test -count=1 -race -run "TestSQS|QueryProtocol|TestPickSqs|TestCollectIndexedKV|TestWriteSQSQueryError" ./adapter/` — passes - [x] `golangci-lint run ./adapter/...` — `0 issues.` - [x] `pickSqsProtocol` table tests cover documented edge cases (header precedence, charset suffix, GET with Action, missing Action, nil request). - [x] `collectIndexedKVPairs` tests cover happy path, orphan Name, empty input, unrelated prefix. - [x] End-to-end via the in-process listener: CreateQueue / ListQueues / GetQueueUrl round-trip on the query side. - [x] **Cross-protocol parity**: a queue created via Query is visible via JSON `GetQueueUrl` with the same URL. - [x] Error envelope: 4xx maps to `<Type>Sender</Type>`, 5xx to `<Type>Receiver</Type>`, namespace pinned, `x-amzn-ErrorType` header set. - [x] Unknown verb returns 501 with the `NotImplementedYet` XML envelope. - [x] Missing `Action` parameter returns 400 (per design §3). ## Self-review (5 lenses) 1. **Data loss** — Wire-format change only. Cores are byte-for-byte identical to the previous handler bodies; no Raft / OCC / MVCC code is touched. 2. **Concurrency** — No new shared state. Detection is request-local. Body parsing is bounded. 3. **Performance** — One additional `Content-Type` string compare per request on the dispatch hot path. Negligible. 4. **Data consistency** — `*Core` returns the same business-logic outputs as before; the JSON tests are the regression net for parity. Cross-protocol parity test pins behaviour. 5. **Test coverage** — 10 new test cases cover detection, parsing, envelope shape, and three end-to-end verbs. Existing `TestSQS*` race suite passes on the refactor. ## Stacking This PR is **independent** — branched from current `main` (which has #638 + #649 merged). It does not depend on PR #650 / PR #659. Merge whenever ready.
) ## Summary **Replaces #659**, which has unresolvable conflicts now that main has moved on (PR #649 squashed into main; PR #658 added the S3 admin endpoints; the Approximate counters implementation now lives directly in `adapter/sqs_catalog.go` via `scanApproxCounters`). Rather than a multi-day rebase, this PR re-applies the unique SQS admin code on a fresh branch off current main. What survived from #659: - `adapter/sqs_admin.go` — `SQSServer.AdminListQueues` / `AdminDescribeQueue` / `AdminDeleteQueue` (SigV4-bypass entrypoints, same shape as `AdminListTables` / `AdminListBuckets`). - `internal/admin/sqs_handler.go` — HTTP handler for `/admin/api/v1/sqs/queues{,/{name}}` with role re-evaluation on DELETE. - `web/admin/src/pages/SqsList.tsx` / `SqsDetail.tsx` — SPA pages for the queues view + delete confirmation. What changed during the re-apply: - `AdminQueueCounters` is now `int64` (matches `sqsApproxCounters` from main; bridge does no width conversion). - `AdminDescribeQueue` calls main's `scanApproxCounters` instead of the duplicate `computeApproxCounters` from the old branch — same numeric output, single implementation. - Dropped the `CountersTruncated` field; main's counter type doesn't expose truncation. SPA's "truncated" pill came out with it. - `apiRouteTable.dispatch` refactored to extract `resourceHandlerFor` so the dispatcher stays under cyclop=10 as new resources land. ## Backend - Re-evaluates the principal's role against the live `MapRoleStore` on every `DELETE` so a downgraded key cannot keep mutating with a still-valid JWT (Codex P1 pattern from earlier admin PRs). - `admin.QueuesSource` is **opt-in**: deployments without `--sqsAddress` leave `/admin/api/v1/sqs/*` off the wire; the SPA renders a soft "endpoint pending" notice on the 404, mirroring the Tables / Buckets `nil` contract. - The bridge in `main_admin.go` (`sqsQueuesBridge`, `convertAdminQueueSummary`, `translateAdminQueuesError`) keeps `internal/admin` free of the heavy adapter dependency tree, same architectural pattern as Dynamo and S3. ## Frontend - **/sqs** queue list with refresh + per-row link to detail. - **/sqs/:name** detail showing FIFO badge, counters card (Visible / In-flight / Delayed), raw attributes table, and a Delete confirmation `Modal` gated by `RequireFullAccess`. - `api/client.ts` gains `listQueues` / `describeQueue` / `deleteQueue` with the same `AbortSignal` pattern used for `cluster` / `dynamo` / `s3` reads. - Layout nav adds an SQS tab between DynamoDB and S3. ## Out of scope (recorded in `docs/design/2026_04_24_proposed_sqs_compatible_adapter.md` Section 14, deferred per the SQS partial doc §16.2) - **PurgeQueue from the SPA** — the underlying `purgeQueueWithRetry` adapter method is on main; the admin entrypoint is a trivial follow-up. - **Send / Peek / CreateQueue from the SPA** — each needs its own SigV4-bypass adapter entrypoint and form UX; deferred to keep this PR focused. ## Test plan - [x] `go build ./...` — clean - [x] `go test -race ./internal/admin/...` — passes - [x] `go test -race -run TestSQS ./adapter/` — passes - [x] `go test -run TestStartAdmin .` — passes - [x] `golangci-lint run ./adapter/... ./internal/admin/... ./...` — `0 issues.`, no `//nolint` - [x] `cd web/admin && npm run build` — 49 modules, 199 KB JS / 61 KB gzip + 14.7 KB CSS - [ ] Manual smoke (after PR lands): start a node with `--sqsAddress :4566 --adminEnabled --adminAllowInsecureDevCookie`, create a queue, send a few messages, hit `/admin/sqs/<name>` → counters match `GetQueueAttributes("All")`, Delete dialog returns to list. ## Self-review (5 lenses) 1. **Data loss** — Delete reuses the existing `deleteQueueWithRetry` OCC path; counters are read-only. No new write paths. 2. **Concurrency** — Per-request leader check on Delete; counters scan uses one snapshot read TS. 3. **Performance** — Counters bounded by main's existing `sqsApproxCounterScanLimit`; admin reads are cheap point lookups + one bounded scan. 4. **Data consistency** — `AdminDescribeQueue` and SigV4 `GetQueueAttributes` both call `scanApproxCounters` at a fresh `nextTxnReadTS`, so a single point in time produces the same counters via either surface. 5. **Test coverage** — Existing admin / SQS race suites stay green via the new `nil` Queues argument added to `startAdminServer` call sites; the new bridge is exercised by the cross-package build itself. ## Stacking This PR is **independent** — branched from current `main` (which has the merged versions of #649 / #658 / #650 / counter implementation). Closing #659 in favour of this clean rewrite.
Per docs/design/README.md's lifecycle convention. The original P1–P4 plan has fully shipped: - P1 (admin skeleton + Dynamo + AdminForward) — #634/#635/#644/#648 - P2 (S3 endpoints incl. write paths and AdminForward integration) — #658 / #669 / #673 / #695 (TOCTOU safety net) - P3 (React SPA + embed) — #649 / #650 - P4 (TLS / role / CSRF / operator doc / deployment runbook / scripts/rolling-update.sh admin support) — #674 / #669 / #678 The AdminDeleteBucket TOCTOU caught during PR #669 review (the last "in-flight" item that kept the doc at _partial_) is fully resolved by the safety-net design landed in #695. What changed: - git mv 2026_04_24_partial_admin_dashboard.md → 2026_04_24_implemented_admin_dashboard.md (history follows the rename) - Header Status line: "Partial" → "Implemented", explanation updated to reflect the post-fix state and the rationale for promotion. - "Last updated" bumped to 2026-04-28 with the rename trigger. - Section heading "Outstanding open items" → "Out-of-scope follow-ups" — the remaining three entries (criterion 5, object browser, TLS hot-reload) are not in-flight work; they are deferred-at-design or Non-goal items. The TOCTOU bullet is removed (resolved) and replaced with a one-line cross-link to the safety-net design + admin_deployment.md §4.6 contract. - Removed the closing "rename trigger" sentence — we just did the rename. - Status table: P2 row now lists #695 alongside #658/#669/#673 so a future reader can find the TOCTOU fix from the index. - Cross-references updated everywhere the old filename appeared: docs/admin.md (header link + Cross-references) docs/admin_deployment.md (header link + final cross-ref) docs/design/2026_04_28_proposed_admin_delete_bucket_safety_net.md (Background section pointer) internal/admin/config.go (Section 7.1 reference comment) No code changes other than the comment-only filename refresh in config.go.
## Summary Promote the admin dashboard design doc from `_partial_` → `_implemented_` per `docs/design/README.md`'s lifecycle convention. PR #695 landed the TOCTOU safety-net fix (the last in-flight item that kept the doc at `_partial_`), so the original P1–P4 plan is now fully shipped: | Phase | Landed via | |---|---| | P1 (admin skeleton + Dynamo + AdminForward) | #634 / #635 / #644 / #648 | | P2 (S3 endpoints + writes + AdminForward S3 + TOCTOU fix) | #658 / #669 / #673 / **#695** | | P3 (React SPA + embed) | #649 / #650 | | P4 (TLS / role / CSRF / operator doc / runbook / script wiring) | #674 / #669 / #678 | The remaining three items in the doc move from "Outstanding open items" (in-flight) to **"Out-of-scope follow-ups"** (deferred-at-design or Non-goal): - AdminForward criterion 5 — rolling-upgrade flag, deferred behind a cluster-version bump that doesn't exist yet - S3 object browser — Non-goal per §2.2 - TLS cert hot-reload — out of scope per `docs/admin.md` ## Changes - `git mv` partial → implemented (history follows the rename) - Status line / Last-updated / status-table / Out-of-scope section content reflects the promotion - Cross-references updated in all 4 referencing files: `docs/admin.md`, `docs/admin_deployment.md`, `docs/design/2026_04_28_proposed_admin_delete_bucket_safety_net.md`, `internal/admin/config.go` (comment-only) ## Test plan - [x] No code changes other than a comment-only filename refresh in `config.go` - [x] `go build ./...` passes - [x] `golangci-lint run ./internal/admin/...` — 0 issues - [x] `grep -rn "2026_04_24_partial\|2026_04_24_proposed_admin"` returns nothing — no stale references
…FO) (#664) ## Summary **Docs-only PR.** Two design proposals for the remaining Phase 3 SQS items per [`docs/design/2026_04_24_proposed_sqs_compatible_adapter.md`](https://github.com/bootjp/elastickv/blob/main/docs/design/2026_04_24_proposed_sqs_compatible_adapter.md) Section 14 (Phase 3 bullets). Both items were explicitly called out as needing separate design docs before any implementation work; this PR lands those proposals so the implementation PRs have a reviewed architecture to build on. ### 3.C — Per-queue throttling and tenant fairness ([proposal](docs/design/2026_04_26_proposed_sqs_per_queue_throttling.md)) Per-queue token-bucket throttling configured on queue meta (no separate keyspace), evaluated at the SQS adapter layer on the leader (no Raft per request), surfaced as the AWS `Throttling` error envelope so SDK retry/backoff just works. Key decisions: - **Default-off**. Existing queues are unaffected; operators opt in per queue via `SetQueueAttributes`. - **Per-action buckets** (Send / Receive / Default) so a slow consumer cannot pin the producer. - **Per-leader buckets, no replication**. Worst case on failover: one extra burst on the new leader. Acceptable per AWS-equivalent behaviour at region failover boundaries; replicating would cost a Raft commit per `SendMessage`. - **Batch verbs charge by entry count**, not call count, with all-or-nothing rejection (matches AWS). - **Admin-only configuration plane**. Standard SQS clients see `InvalidAttributeName` on the `Throttle*` attributes (matches AWS behaviour for unknown attributes); the data-plane enforcement runs for everyone. ### 3.D — Split-queue FIFO (high-throughput FIFO) ([proposal](docs/design/2026_04_26_proposed_sqs_split_queue_fifo.md)) Per-`MessageGroupId` hash partitioning across multiple Raft groups, mirroring AWS High Throughput FIFO. Within-group ordering preserved; across-group throughput scales with the partition count. Key decisions: - **Existing single-partition FIFO queues stay byte-identical** (`PartitionCount = 0` path is the legacy layout; no migration runs implicitly). - **Power-of-two partition counts only** (1, 2, 4, 8, 16, 32) so the routing step is `hash & (N-1)` and future offline rebuilds stay tractable. - **Partition count is immutable after first SendMessage**. Live re-partitioning would break ordering for in-flight messages of every group whose hash bucket changed; out of scope. - **Multi-PR rollout plan** with an explicit "gate of no return" called out at PR 5 (the data-plane PR). PRs 1–4 are reversible no-ops on data layout; once a partitioned FIFO holds real data, rollback means draining and recreating the queue. - **FNV-1a hash** (deterministic across processes / Go versions / architectures). Risk of attacker-controlled `MessageGroupId` pinning all traffic to one partition is documented and accepted (the feature is for cooperative operators). ## Test plan - [x] Markdown renders correctly on GitHub (manually previewed). - [x] Cross-references resolve (the partial-doc rename in PR #659 is in flight; both proposals reference the partial filename — they will resolve once #659 merges, otherwise resolve to the current `_proposed_` filename for a 1-character mismatch that is fine to leave for now). - [x] No code changes; CI is irrelevant beyond the markdown lint pass. ## Self-review This is a docs-only PR; the 5-lens self-review collapses to: 1. **Data loss / Concurrency / Performance / Consistency** — N/A, no code touched. 2. **Test coverage** — N/A, no code touched. The proposals themselves include a Testing Strategy section (§6 / §9) so the implementation PRs have explicit acceptance criteria. ## Stacking This PR is **independent** of #650, #659, and #662. Branched from current `main`. Merge whenever ready — landing the proposal docs early lets reviewers comment on the architecture before the implementation PRs go up. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added detailed designs for optional per-queue token-bucket throttling (rules, batch semantics, AWS-shaped Throttling errors with Retry-After, metrics, rollout default-off) and for HT‑FIFO “split-queue” partitioning (routing, immutability, gating/rollout). * **New Features** * Optional queue-level send/receive throttling with batch-aware charging and Retry-After; HT‑FIFO partitioning with deterministic partition routing and immutable partition attributes. * **Tests** * Extensive unit and integration tests for throttling, HT‑FIFO partitioning, attribute validation, immutability, idempotency, and cache invalidation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 participant
Summary
Two follow-ups from the #649 review that were intentionally deferred to keep that PR focused on the functional bugs.
Stacked on
feat/admin-dashboard-spa(#649). Once #649 merges, this rebases cleanly ontomain.1.
useApi.ts— drop the redundant abort guardThe
catchhandler had two checks back-to-back:The cleanup function sets
cancelled = truebefore callingctrl.abort(), so any abort path is already covered by the first check. Dropped the second guard and recorded why one check is enough in a comment.2.
Modal.tsx— full WAI-ARIA dialog contract + focus trapAdded:
role="dialog",aria-modal="true",aria-labelledbyon the dialog root (usesuseId()for the title id).tabindex=-1.Vanilla DOM implementation —
focus-trap-react/react-focus-lockwould have been 5–7 KB gzip for what is a ~40 line solution.Bundle delta: +0.4 KB gzip (60.23 → 60.59 KB JS, 14.55 → 14.57 KB CSS).
Test plan
cd web/admin && npm install && npm run buildcd web/admin && npm run lint—tsc --strictgo build ./...busy) and focus returns to the trigger button.