fix(audit-sprint-5a): modal Phase 2 — 4 window.confirm migrations - #168
Closed
ejay-dev wants to merge 1 commit into
Closed
fix(audit-sprint-5a): modal Phase 2 — 4 window.confirm migrations#168ejay-dev wants to merge 1 commit into
ejay-dev wants to merge 1 commit into
Conversation
Builds on Sprint 4c's AlertDialog primitive. Replaces 4 of the 7
remaining browser confirm() callers. Each migration also gives the
surface focus trap, ESC, aria-modal, scroll lock — none of which
the browser confirm provided.
Migrated
- app/admin/sessions/page.tsx — revoke session. Confirms with the
user's display name, not just "Revoke this session?".
- components/policies/policy-editor.tsx — publish gate. Shows the
policy title + version in the dialog; the previous browser
confirm only said "Are you sure?".
- components/team/role-cell.tsx — owner-involving role change.
Promotes/demotes get a tailored title. Refactored handleChoose
into applyRoleChange + dialog opener so the optimistic-update
logic stays in one place.
- components/vault/evidence-file-actions.tsx — evidence delete +
bonus: replaced two window.alert() calls with sonner toast.error
(one in openFile error path, one in delete-failure path).
Deferred to Sprint 5b/Phase 3
- app/admin/components/member-management-actions.tsx — uses
window.prompt for the removal reason then window.confirm.
Needs a Dialog with a textarea (not AlertDialog), so it lands
in the form-modal phase.
- app/admin/components/billing-action-buttons.tsx,
app/admin/components/org-action-buttons.tsx,
components/control-plane/admin-command-center.tsx — also use
window.confirm. All take a confirmText prop pattern and follow
a similar refactor; bundled into the next Phase-2 PR.
Validation
- tsc -p tsconfig.typecheck.json: clean
- eslint: 0 errors, 18 warnings (baseline)
- jest: 5319/5334 pass (no test changes)
Built on top of fix/audit-sprint-4c-modal-primitives; merge that
first so the AlertDialog primitive is on main.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR continues the modal migration by replacing selected browser confirmation/alert flows with the shared AlertDialog and toast primitives introduced in the prior modal-primitives work.
Changes:
- Migrates admin session revocation, policy publishing, owner role changes, and evidence deletion to
AlertDialog. - Replaces evidence-file browser alerts with sonner toast errors.
- Refactors role-change handling so owner-involving changes are staged before confirmation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
app/admin/sessions/page.tsx |
Adds pending revoke state and AlertDialog confirmation for session revocation. |
components/policies/policy-editor.tsx |
Adds AlertDialog confirmation around policy publishing. |
components/team/role-cell.tsx |
Adds owner role-change confirmation flow using AlertDialog. |
components/vault/evidence-file-actions.tsx |
Adds delete confirmation dialog and toast-based error reporting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+109
to
+110
| in the organisation. The current version | ||
| ({policy.version}) becomes the active one. |
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
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.
Summary
Builds on Sprint 4c's AlertDialog primitive (PR #167). Replaces 4 of the 7 remaining browser
confirm()callers. Each migration also gains focus trap, ESC handler, aria-modal, and scroll lock — none of which the browser confirm provided.Migrated
confirm('Revoke this session?')confirm('Are you sure?')for publishwindow.confirmfor owner-involving role changeshandleChooseso optimistic-update logic is one pathwindow.confirmfor delete +window.alertfor errorstoast.error(Sprint 4c's sonner Toaster)Deferred to next Phase 2 PR / Phase 3
app/admin/components/member-management-actions.tsx— useswindow.promptfor removal reason thenwindow.confirm. Needs a Dialog with textarea (not AlertDialog), so it lands in form-modal phase.app/admin/components/billing-action-buttons.tsx,app/admin/components/org-action-buttons.tsx,components/control-plane/admin-command-center.tsx— alsowindow.confirm. All take aconfirmTextprop pattern with similar refactor.Validation
npm run type-checkcleannpm run lint0 errors, 18 warnings (baseline)npx jest5319/5334 pass (no test changes — primitives covered by Sprint 4c's wrappers)Test plan
/admin/sessions, click "Revoke" — AlertDialog appears with the user's name, ESC dismisses, focus traps/app/policies/[id]/edit, click Publish — AlertDialog appears with policy title + version🤖 Generated with Claude Code