feat: age-gate UI — admin review queue and per-repo config panel#452
feat: age-gate UI — admin review queue and per-repo config panel#452RajatGarga wants to merge 2 commits into
Conversation
Missing linked issueThis PR does not reference a tracking issue in its body. Every PR must link to an issue in this repository so we can trace work back to a planned change. How to fix
Accepted keywords (case-insensitive, any tense): Policy reference: see the PR template. Maintainer bypass: apply the |
brandonrc
left a comment
brandonrc
left a comment
There was a problem hiding this comment.
Accessibility review (WCAG 2.2 AA) — age-gate page + per-repo AgeGateSettings panel. This is the richest a11y surface of the v1.2.0 cache PRs; several real gaps in the new dialog, tables, and async flows.
Good baseline first: real Tabs/TabsTrigger (roles correct), AgeGateSettings uses proper <Label htmlFor> for the Switch and number Input, the settings section is wrapped in <section aria-labelledby="settings-age-gate-heading">, and the approve/reject Dialog uses shadcn Dialog (focus trap + Esc + DialogTitle/Description) so dialog focus management is handled by the primitive.
Issues:
-
Approve/Reject confirm button loses its accessible name while pending (4.1.2 Name, Role, Value). In the dialog footer the action button renders only
<Loader2 className="animate-spin" />whenisActioning— an icon-only button with no text and noaria-label. While the request is in flight the button announces as just "button". Addaria-label(e.g. "Approving...") or keep visually-hidden text alongside the spinner. The spinner icon should also bearia-hidden. -
Async approve/reject result is toast-only — no live region (4.1.3 Status Messages).
toast.success("Package version approved")andmutationErrorToastare the only feedback; sonner toasts are not reliably announced by AT, and the dialog closes on success so focus returns with no spoken confirmation. Add anaria-live="polite"status region for the outcome, consistent with theupstream-auth-statuslive-region pattern in #459. Same applies to AgeGateSettings save (toast.success("Age gate settings saved")). -
DataTable actions column has an empty, unnamed header cell (1.3.1 Info and Relationships / 4.1.2).
{ id: "actions", header: "" }renders a<th>(TableHead) with no text content, so the column has no accessible name. Provide a non-empty header that is visually hidden, e.g.header: "Actions"rendered in ansr-onlyspan, rather than an empty string. (The shared DataTable renderscol.headerdirectly into TableHead, so ansr-onlywrapper would need supporting there or pass a node.) -
Approve / Reject row buttons rely on row context for meaning (1.3.1 / 2.4.6). "Approve" / "Reject" buttons repeat on every pending row with no per-row distinguishing accessible name. A screen-reader user navigating by buttons hears "Approve, Reject, Approve, Reject..." with no idea which package. Add
aria-labellikeApprove {package_name}@{package_version}to each. (The repo's own #459 e2e suite asserts this kind of per-row actions naming, e.g. "repository actions for ...".) -
Reject is destructive but commits from a dialog with only an optional reason — acceptable since it is already a two-step (dialog) flow; no change needed, just confirming the destructive-confirmation bar is met by the dialog.
-
Minor — StatusBadge conveys status via color + text. Text is present (good, not color-only), so 1.4.1 is satisfied; no action.
-
Minor — AgeGateSettings "Minimum age (days)" Input has native min/max but no inline validation/error association; if out-of-range values are possible client-side, mirror the #459 aria-describedby+role=alert pattern. Currently relies on native number constraints, so low priority.
Items 1-4 are real AT barriers in new UI and should be addressed before merge. Since this is an external fork PR (@RajatGarga), happy to point at #459 as the concrete reference implementation for the error-association and live-region patterns.
brandonrc
left a comment
brandonrc
left a comment
There was a problem hiding this comment.
Thanks for the contribution, this is a well-structured feature. The admin queue page, per-repo config panel, webhook event additions, and sidebar entry hang together coherently, the components follow the existing shadcn/TanStack-Query patterns, the apiFetch usage for the not-yet-in-SDK endpoints matches how the rest of this codebase bridges SDK lag, and you included both unit and Playwright coverage. A few things need addressing before merge.
🔴 Broken import path will fail the build (age-gate-settings.tsx:7)
import type { Repository } from "@/types/repository"; — there is no src/types/repository.ts in this repo; the Repository type is exported from @/types (see repo-settings-tab.tsx:12, which imports Repository from @/types, and your own age-gate-settings.test.tsx, which correctly imports from @/types). As written, tsc/Next build will fail to resolve the module. Change to import type { Repository } from "@/types";.
🟡 History tab: client-side filtering breaks pagination counts (age-gate/page.tsx:189-191, 366-368)
historyItems = historyData?.items.filter((item) => item.status !== "pending") strips pending rows from the page client-side, but the DataTable is handed total={historyData?.pagination?.total} — the unfiltered server total. So the page count and the visible row count disagree: a page can show fewer rows than pageSize (pending rows silently removed), and total over-counts. The history query also sends no status filter, so the server returns every status and you filter on the client, which means pending items consume page slots and paginate inconsistently. Prefer filtering server-side, e.g. pass a status filter for "reviewed" states (or status in approved,rejected if the API supports it) and drop the client-side .filter, so items and pagination.total describe the same set.
🟡 updateRepoConfig sends min_age_days even when the input is empty/NaN (age-gate-settings.tsx:634, 582-586)
onChange={(e) => setOverrides((o) => ({ ...o, minAgeDays: Number(e.target.value) }))} — clearing the number input yields Number("") === 0, and the Save handler submits min_age_days: minAgeDays directly. Backend min is presumably >= 1 (the input sets min={1} but that is not enforced on submit). Consider validating minAgeDays >= 1 before enabling Save, mirroring how #450 gates its TTL save, so operators get an inline error rather than a 400.
💭 requested_at field is declared but unused; last_requested_at is what the table renders
The AgeGateReview interface carries both requested_at and last_requested_at. The table uses last_requested_at (non-null) for "Last requested". Just confirm the backend actually returns last_requested_at as non-nullable — formatDate will render "Invalid Date" if it is ever absent, since unlike upstream_published_at it has no — fallback.
💭 approve/reject always send body: JSON.stringify({ reason }) where reason may be undefined
JSON.stringify({ reason: undefined }) produces "{}", which is fine, but worth confirming the backend treats a missing reason and reason: null identically. Minor.
Nice touches worth calling out: the enabled: !!user?.is_admin && activeTab === "history" gate to avoid fetching history until the tab is opened, the admin-access EmptyState guard, and reusing pendingColumns to derive historyColumns.
Note (not a defect): this edits repo-settings-tab.tsx, which #450 also modifies (#450 adds a Proxy Cache section to the same render tree; you add an AgeGateSettings section). Both insert <section> blocks plus a <Separator /> in the same area, so expect a conflict. Flag merge order to whoever sequences the v1.2.0 wave.
brandonrc
left a comment
brandonrc
left a comment
There was a problem hiding this comment.
Security/API review (comment-only). External fork PR, reviewed with extra scrutiny on authz and injection.
Authorization (review queue + config): correct and defense-in-depth.
- The page lives under the
(admin)route group, whoselayout.tsxwraps children inRequireAdmin(redirects non-admins to/error/403). - The page additionally guards with
if (!user?.is_admin)(renders an "Admin access required" empty state) and gates bothlistReviewsqueries onenabled: !!user?.is_admin, so no admin API call fires for a non-admin. - The sidebar entry sits in
operationsItems, which is rendered only inside the{isAdmin && ...}block inapp-sidebar.tsx, so it is not exposed to non-admins. - All review endpoints hit
/api/v1/admin/age-gate/...; client gating is purely UX and the server admin routes remain the real boundary. No client-side privilege assumption is load-bearing.
The per-repo AgeGateSettings panel writes via PUT /api/v1/repositories/{key}/age-gate; it does not self-gate on admin, so confirm the backend authorizes that route at repo-admin level. The page-level age-gate review queue is admin-gated; this per-repo config panel is rendered within the repository settings surface and must rely on server-side authz. Flagging for confirmation, not blocking on the web diff.
XSS / injection: clean.
- No
dangerouslySetInnerHTML/innerHTML/evalanywhere in the diff (verified across all 5 PRs). - All submission/review content (
package_name,package_version,repository_key,review_reason,reviewed_by) renders through React JSX, which auto-escapes. A malicious upstream package name like<img src=x onerror=...>renders as inert text in both the table and the confirm dialog. - API client encodes the repo key with
encodeURIComponent; review IDs go into the path unencoded but are server-generated UUIDs, not user input. Low risk, but considerencodeURIComponent(id)onapprove/reject/getReviewfor defense in depth in case the ID shape ever changes.
Minor input-validation gap (non-blocking): min_age_days uses Number(e.target.value) with no integer/range clamp before send. The input has min={1} max={3650} but those are advisory; a user can type a non-integer or out-of-range value and the Save button stays enabled, pushing it to the backend. The backend must validate (compare to the cache-TTL PR #450, which added explicit client-side range + integer checks and disables Save). Recommend mirroring that pattern: reject non-integer / out-of-range and disable Save, rather than relying solely on the backend 400.
No blocking issues. Two follow-ups to confirm: (1) backend authz on the per-repo PUT, (2) min_age_days client-side validation parity with PR #450.
…dation, encode review IDs - page.tsx: per-row aria-label on Approve/Reject buttons, aria-live status region, server-side history filter (status=approved,rejected param), aria-hidden spinner icon - age-gate-settings.tsx: fix build-breaking import (types/repository → types), min_age_days 1–3650 validation with inline error, disabled Save on invalid, live status region - data-table.tsx: srOnlyHeader column option renders screen-reader-only header text - age-gate.ts: encodeURIComponent on review IDs in getReview/approve/reject URLs
Review response — all items addressedFixes in commit
|
2 participants
Summary
Adds the web frontend for the age-gate quality gate feature (requires artifact-keeper/artifact-keeper#1403 to be merged first):
/age-gate): lists pending/approved/rejected package version reviews with approve and reject actions.AgeGateSettingscomponent lets admins enable the gate and set the minimum age threshold per repository (npm and PyPI remote repos only).age_gate_*event types wired into the webhook subscription UI andWebhookEventtype union.Discussion: https://github.com/orgs/artifact-keeper/discussions/1402
Test Checklist
npm run test)e2e/suites/interactions/operations/age-gate.spec.ts)UI Changes