feat(site): move template allowlist to ai settings - #26615
Conversation
6692bdf to
4a0d153
Compare
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 3 | Last posted: Round 3, 17 findings (1 P2, 5 P3, 2 P4, 6 Nit, 3 Note), APPROVE. Review Finding inventoryFinding inventoryFindings
Round logRound 1Panel: Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Gon, Leorio, Nami, Ging-react, Robin, Zoro, Komugi, Chopper, Meruem. 5 P3, 1 P4, 5 Nit, 2 Note. Reviewed against ecfff8a..4a0d153aa762. Round 2CRF-1 through CRF-5, CRF-7 through CRF-13 addressed. CRF-6 acknowledged. Panel: Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Nami, Chopper, Komugi, Kite. 1 P2, 1 P4, 1 Nit, 1 Note new. 1 dropped. Reviewed against ecfff8a..87f12f9. Round 3CRF-14 through CRF-17 addressed. Panel: Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Komugi, Meruem. 0 new findings. All prior findings closed. Reviewed against ecfff8a..bc28ae4. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean move with a solid UI redesign. The page relocation, route redirect, and sidebar consistency are all well executed. Story coverage improved from 1 story to 5, and the mutation lifecycle is correctly gated by isPending through the refetch cycle. The fixture data in stories is thoughtful, with realistic template names and developer counts.
Severity summary: 5 P3, 1 P4, 5 Nit, 2 Note.
The P3 findings cluster around two themes: (1) error handling diverges from the AI Settings sibling convention by discarding error details and merging failure modes, and (2) hand-rolling UI patterns that existing primitives (Command, TableEmpty) already provide. The story coverage gap on the Retry button wiring is also worth closing.
"The button becomes costume jewelry: looks right, does nothing." (Bisky, on the LoadError story)
site/src/pages/AISettingsPage/TemplatesPage/TemplatesPage.tsx:33
P3 [CRF-2] Load error merges two independent failure modes. Boolean(templatesQuery.error || allowlistQuery.error) collapses two distinct API failures into one hasError boolean, and the view shows "Failed to load template data." If the allowlist endpoint returns a 500 but templates succeeds, the admin investigates the wrong thing.
Pass separate error props or the actual error objects so the view can say "Failed to load allowlist configuration" vs "Failed to load templates" depending on which query errored. The sibling pages use ErrorAlert with the real error object. (Leorio P3, Zoro P3)
🤖
🤖 This review was automatically generated with Coder Agents.
4a0d153 to
4e4e82c
Compare
Fixed by preserving the specific query errors in the page view. The templates page now passes separate template and allowlist errors through to |
4e4e82c to
c909e2f
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
R1 fixes are solid. All 12 code fixes verified by the panel: Command primitives, ErrorAlert with separate query errors, TableEmpty, story interactions, and cleanups all landed correctly. CRF-6 (no optimistic updates) accepted as following sibling convention.
R2 severity summary: 1 P2, 1 P4, 1 Nit, 1 Note.
The P2 is a correctness regression: the old code resolved template IDs against live templates before saving, naturally dropping IDs for deleted templates. The new code saves raw IDs from the server, which means phantom IDs for deleted templates silently persist in the allowlist. The UI shows "No restrictions set" while the server enforces a non-empty allowlist.
"The old
AgentSettingsTemplatesPageViewresolved IDs againstoptionsByIDintoserverSelection, then saved fromcurrentSelection, which naturally excluded IDs with no matching template. The new code skips this resolution step for saves." (Mafuuu, tracing the regression)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All findings resolved. 3 rounds, 18 findings (1 P2, 5 P3, 2 P4, 5 Nit, 2 Note, 1 accepted, 1 dropped, 1 not posted). Every code fix verified by the panel.
The P2 (phantom template IDs) was the critical catch: the redesign from batch-save to immediate-save dropped the ID resolution step that filtered out deleted templates. The fix derives save payloads from the resolved template list, and a dedicated PhantomTemplateIDs story covers both add and remove paths.
The overall arc is solid: feature moved to AI Settings, UI redesigned with project primitives (Command, TableEmpty, ErrorAlert), error handling split per query, story coverage at 8 stories with 7 play functions, and no stale references left behind.
"I tried to build a case against this change and couldn't." (Pariston, R3)
🤖 This review was automatically generated with Coder Agents.
Documentation CheckUpdates Needed
Automated review via Coder Agents |
2 participants
Move the Coder Agents template allowlist from Agents settings into AI Settings and redirect the old
/agents/settings/templatesroute to/ai/settings/templates.Redesign the page as an allowlist table with an add-template picker, empty unrestricted state, and per-row remove action. The Agents settings sidebar now links out to AI Settings for Templates, matching the existing Providers and Models links.
Note
🤖 This PR was written by Coder Agent on behalf of Danielle Maywood