fix(admin): name the access lists, and stop confirming presses that did nothing - #213
Merged
Conversation
…id nothing The watched-lists page identified rows by `#4001` whenever the list had never been read, even though the catalog knew its name the whole time. `getWatchedListViews` now LEFT JOINs `accessListCatalog` and COALESCEs the catalog name over the snapshot, matching the precedence `watchedListName` already used for audit rows — so the log and the page cannot disagree about what a list is called. Two presses also confirmed work they had not done. `addWatch` and `removeWatch` are idempotent, and both actions read that as success: adding a list another admin had just added answered "List added to the watchlist", and removing a row another tab had already removed answered "removed from the watchlist". Neither wrote an audit row, so the notice was the only account of the press, and it credited this admin with someone else's act. Both services now report whether they changed anything. The add redirects, so its correction is wording alone (`?done=watch-already`); the remove does not, so it returns a `warn` outcome. A refused re-press was silent. `useSubmitGuard` blocks the second press while the first is in flight, and `removeWatchAction` deliberately does not redirect, so the page did not move and nothing was said. `StopWatching` moved into its own client component to wire `onRefused` — a server component cannot hand a function to a client one. The add control was rebuilt as a field group above the list rather than an aside on the section head, since it acts on the collection and not on any row, and the page adopted `.page__head`/`.page__lede`, which let the duplicate `.lede` rule leave `globals.css`. `monitorSentence`'s "normal" case gained a real sentence that names the holder and says what the page compares, without claiming reads are healthy — it cannot know that. Both new e2e tests were mutation-proved: reverting either fix fails exactly the test that covers it.
📝 WalkthroughWalkthroughThe access-list watch flow now reports successful and no-op changes, uses catalog names when snapshots are absent, validates the add-list form, and separates page header content from watch controls. Unit and E2E tests cover the updated behavior. ChangesAccess-list watch flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Admin as Access-list page
participant Action as addWatchAction
participant Service as addWatch
participant DB as Watchlist database
participant Notice as doneNotice
Admin->>Action: Submit catalog selection
Action->>Service: Add access-list watch
Service->>DB: Insert watch row
DB-->>Service: Inserted or duplicate
Service-->>Action: Return state-change result
Action->>Notice: Redirect with outcome marker
Notice-->>Admin: Render success or already-watched notice
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/admin/access-lists/actions.ts`:
- Line 41: Replace the existing parseId usage in the access-list actions with
the shared Zod schema, validating formData.get("accessListId") before invoking
addWatch and removeWatch. Reuse the parsed ID for both service calls and
preserve the existing action behavior after validation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7be20d16-9fc8-488f-96fb-d2555b1cd592
📒 Files selected for processing (9)
e2e/access-lists.spec.tssrc/app/admin/access-lists/actions.tssrc/app/admin/access-lists/page.tsxsrc/app/admin/access-lists/stop-watching.tsxsrc/app/admin/access-lists/view.tssrc/app/globals.csssrc/services/access-lists.tstests/access-lists-service.test.tstests/access-lists-view.test.ts
| const { accountId: actor } = await requireAdminAction(); | ||
| const accessListId = parseId(formData.get("accessListId")); | ||
| await addWatch(getDb(), accessListId, actor); | ||
| const added = await addWatch(getDb(), accessListId, actor); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the local action structure and existing Zod conventions.
ast-grep outline src/app/admin/access-lists/actions.ts --items all
rg -n -C 3 --glob '*.{ts,tsx}' 'from "zod"|z\.(object|coerce|number)' src
fd -HI '^package\.json$' -x sh -c 'echo "--- $1"; jq ".dependencies.zod // .devDependencies.zod" "$1"' sh {}Repository: guarzo/authGD
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- actions.ts ---'
cat -n src/app/admin/access-lists/actions.ts
printf '%s\n' '--- package versions ---'
jq '{dependencies,devDependencies}' package.json | jq 'with_entries(.value |= with_entries(select(.key == "zod" or .key == "next")))'
printf '%s\n' '--- application Zod imports and schemas ---'
rg -n --glob 'src/**/*.{ts,tsx}' 'import \{ z \} from "zod"|Schema|parse\(' src | head -120Repository: guarzo/authGD
Length of output: 13331
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- actions.ts ---'
cat -n src/app/admin/access-lists/actions.ts
printf '%s\n' '--- package versions ---'
jq '{dependencies,devDependencies}' package.json |
jq 'with_entries(.value |= with_entries(select(.key == "zod" or .key == "next")))'
printf '%s\n' '--- application Zod imports and schemas ---'
rg -n --glob 'src/**/*.{ts,tsx}' \
'import \{ z \} from "zod"|Schema|parse\(' src | head -120Repository: guarzo/authGD
Length of output: 13331
Validate accessListId with Zod before calling the service.
At src/app/admin/access-lists/actions.ts:40 and :85, replace parseId with a shared Zod schema. Parse formData.get("accessListId") before addWatch and removeWatch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/admin/access-lists/actions.ts` at line 41, Replace the existing
parseId usage in the access-list actions with the shared Zod schema, validating
formData.get("accessListId") before invoking addWatch and removeWatch. Reuse the
parsed ID for both service calls and preserve the existing action behavior after
validation.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Screenshots in the request showed two things wrong with
/admin/access-lists: rows identified by a number the admin has no use for, and an add control parked as an aside on a section head. Fixing the first turned up a third problem underneath it — two controls that confirmed work they had not done.The name
getWatchedListViewsread names off the drift snapshot, which only exists once the list has been read. A list added to the watchlist a minute ago rendered as#4001— whileaccessListCatalog, the table the<select>is built from, held its name the whole time.It now LEFT JOINs the catalog and COALESCEs catalog name over snapshot name. That ordering is not arbitrary: it matches what
watchedListName()already does for audit rows (src/services/access-lists.ts:93-104), so the audit log and the page cannot disagree about what a list is called.The two presses that lied
addWatchusesonConflictDoNothing;removeWatchdeletes by id. Both are idempotent, and both actions read "no error" as "it worked":Neither press wrote an audit row, so the notice was the only account of what happened, and it credited this admin with an act that had already happened without them.
Neither is reachable by clicking alone — the
<select>never offers a watched list, and a removed row takes its button with it. Both are reachable exactly the way a real admin reaches them: the page renders, the table changes underneath it, and the press lands against a world that has moved.Both services now return whether they changed anything. The two actions differ in how they can say so:
addWatchActionredirects, andConfirmNoticecarries a sentence and no tone, so its correction is wording alone —?done=watch-already→ "That list was already on the watchlist. Nothing was added."removeWatchActiondeliberately does not redirect (a redirect resets theDisclosurestate), so it returns awarnActionOutcomeand gets both the words and the tone.I did not add a
tonechannel toConfirmNoticeto make these symmetric — it is shared with/accountand/admin/sync, and that is a wider change than this task asked for.The silent refusal
useSubmitGuardrefuses a second press while the first is in flight. Where the first press navigates, silence is right. Here it is not:removeWatchActiondoes not redirect, so the page does not move and the admin watches a press do nothing. Worse, the whole region shares oneConfirmingForm, so the button refused is frequently a different row's than the one in flight.StopWatchingmoved out ofpage.tsxinto its own"use client"file to wireonRefused. That is the whole reason for the new file:onRefusedis a function, and a server component cannot hand a function to a client one.Layout
The add control became a field group above the list rather than an aside on the section head — it acts on the collection, not on any row, and "Add to watchlist" is the thing an admin does repeatedly, where "Designate as holder" is done once. The
<select>isrequiredwith a disabled placeholder, which also fixes a real defect: an untouched submit used to POST with noaccessListIdand throwinvalid_id.The page adopted
.page__head/.page__lede, which let the duplicate.lederule leaveglobals.css.monitorSentence's"normal"case gained a real sentence naming the holder and stating what the page compares — deliberately without claiming reads are healthy, sincemonitorStatenever sees areadStatusand warn rows render directly beneath that sentence.Verification
Both new e2e tests were mutation-proved: forcing
addWatchActionto always emitdone=watchand deleting theif (!removed)branch failed exactly those two tests and nothing else (13 passed). Both mutations were reversed and the restore verified against the diff.code-reviewerran clean — admin guard on all four actions, audit writes confined to the branches that actually change state, enqueue-don't-execute boundary untouched, no orphaned comment claims.Where to look
src/services/access-lists.ts— the two return-value changes are the substance; everything else follows from them. The docblocks there argue why "idempotent" and "confirmed as done" are different claims, which is the point of the whole change.Summary by CodeRabbit
New Features
Bug Fixes