Conversation
- Multi-select in the OAuth2 app form, backed by /api/v2/auth/scopes. - Form holds the selection as an array and joins it on submit, so the page views keep passing the form payload straight to the API. - Picker disables while the catalog is loading or errored; the other fields stay editable and the app's existing scopes are preserved.
|
/coder-agents-review |
|
Chat: Review in progress (15/15 reviewers complete) | View chat deep-review v0.9.0 | Round 1 | Last posted: Round 1, 7 findings (4 P3, 2 Nit, 1 Note), COMMENT. Review Finding inventoryFinding inventory - PR #29360Findings
Round logRound 1Netero-only first pass: no findings, tsc/biome/vitest all pass. Panel of 14 (Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Nami, ging-ts, ging-react, Gon, Leorio, Komugi, Chopper, Robin, Meruem + Meruem wildcard). No P0-P2 confirmed after cross-check. 4 P3, 2 Nit, 1 Note. Reviewed against 34ba077..6bfdd88. Cross-check notes:
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, well-scoped UI half of the scope allowlist. The core contract holds under scrutiny: the split/join round-trip, the catalog-error path that refuses to clear an existing allowlist, and the disabled-only-the-picker design all traced correctly and the three vitest cases pass. Test density is high (199 test / 97 production lines) and the safety invariant (a catalog outage cannot silently drop an allowlist) is pinned by both a story and a specific-assertion test. Netero's mechanical pass, ging-ts, ging-react, Komugi, and Robin found nothing.
No P0-P2 after cross-check. 4 P3, 2 Nit, 1 Note. Nothing blocks; the P3s are worth addressing before merge because this code has no follow-up owner.
Themes: one query-cache mismatch where the comment claims immutability the code does not implement (CRF-1, three reviewers converged), one empty-state message that states a false state on a reachable interaction (CRF-2, reproduced by Hisoka), the catalog-error UX offering no retry short of a full reload (CRF-3), and a stated preservation guarantee whose realistic trigger is untested (CRF-4).
As Bisky put it: "every stone I picked up is real ... nothing fake, nothing unrunnable, nothing fragile." The findings below are edges on a genuinely solid change.
site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppForm.tsx:229
Note [CRF-7] On a catalog load failure during create, submit stays enabled and produces an unrestricted app. (Meruem Note, Hisoka Note)
submitDisabled(line 102-103) does not includescopesQuery.isError, so when the catalog fails to load the picker is disabled and empty while the submit button stays active. On the create path (appundefined) the initial scope is[], so an admin who cannot see the catalog can still create an app, and it is created with no allowlist (unrestricted).
The error text makes the failure visible and an empty allowlist is a documented valid state, so this is defensible fail-open behavior, not a regression (without this feature all apps are already unrestricted). Flagging it as a human decision: if an allowlist should be settable before an app is created, block submit while scopesQuery.isError on the create path; otherwise this is fine as-is.
🤖
🤖 This review was automatically generated with Coder Agents.
- Spread disabledRefetchOptions so a fetched catalog is kept for the session, matching the comment that says it never changes. - A failed fetch still retries on the next mount.
…load - Without it the picker stayed disabled until a full page reload, which also drops any unsaved edits on the form. - Test covers retry followed by a successful selection and submit.
- The create page sends the form output as a POST and the edit page as a PUT, so the callback type is now the intersection of both request types instead of the POST type alone. - Trim the scope catalog fixture comment.
- The catalog loads, omits one of the app's stored scopes, and a name-only save still sends that scope back. - The existing coverage only exercised a catalog that failed to load.
- "All scopes selected" also showed when a typed search matched nothing, or when the catalog was empty. "No matching scopes" is true in every case the dropdown is empty.
…cope-allowlist' into plat-475-2-form # Conflicts: # site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.stories.tsx
1 participant
Adds a scope picker to the OAuth2 app form. UI half of PLAT-475; the API half is #29313.
/api/v2/auth/scopes.scopestring on submit. The page views are unchanged.scope: "", which changes nothing.Depends on #29313. Rebase onto
mainafter it merges.