fix(audit-sprint-7c): modal Phase 3 — 2 form modals → Dialog primitive - #177
Closed
ejay-dev wants to merge 1 commit into
Closed
fix(audit-sprint-7c): modal Phase 3 — 2 form modals → Dialog primitive#177ejay-dev wants to merge 1 commit into
ejay-dev wants to merge 1 commit into
Conversation
Modal-audit agent flagged 7 form modals for migration to the Sprint 4c
Dialog primitive. This PR migrates 2 of the cleanest ones as proof; the
other 5 have heavier opinionated styling (white-bg invite modal,
gradient-header vault uploads, etc) that warrant per-modal styling
decisions and screenshot review — deferred to follow-up PRs.
Migrated
- components/integrations/integration-config-dialog.tsx (~30 LoC
net trim): swap fixed inset-0 + bg-slate-950/70 wrapper for
Dialog/DialogContent. Strip "Integration Config" cyan eyebrow and
rounded-3xl outer chrome per the stored enterprise-aesthetic
preference. Gain focus trap, ESC, aria-modal, scroll lock.
- components/registers/add-certification-modal.tsx (~30 LoC net
trim): same migration. Strip the blue→indigo→cyan gradient header
badge + submit-button gradient + rounded-[2rem] surface. UI now
matches the rest of the new primitives — neutral slate dark
surface, no chrome.
Visual change is real
Both modals lose their bespoke gradient/rounded styling in favour of
the neutral Dialog defaults. This is intentional per the stored
"enterprise aesthetic over AI feel" preference and the modal-audit
finding that flagged both as cyan-glow offenders. After-merge
screenshot review recommended.
Deferred to follow-up PRs (heavier per-modal decisions)
- components/team/invite-modal.tsx (white-bg modal, rounded-[2rem]) —
can't mechanically swap to dark Dialog default without a deeper
light/dark decision
- components/team/invite-button.tsx (overlapping concern with above)
- components/vault/upload-artifact-modal.tsx (file-upload complexity +
duplicated overlay block the agent flagged as a bug)
- components/vault/credential-inspector-modal.tsx
- components/compliance-system/plan-activation-flow.tsx (state-machine
flow, not a single-screen form)
Built on top of fix/audit-sprint-4c-modal-primitives (PR #167) for the
Dialog primitive.
Validation
- tsc -p tsconfig.typecheck.json: clean
- eslint: 0 errors, 18 warnings (baseline)
- jest: 5319/5334 pass (no test changes)
Test plan
- Browse /app/settings/integrations, click any Connect button →
Dialog opens centered with focus on first field, ESC dismisses,
no cyan eyebrow
- Browse /app/registers (staff register), click Record Certification
→ same Dialog UX, no gradient chrome
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 migrates two form modals from custom “fixed inset-0” overlays to the shared Radix-based Dialog primitive introduced in the modal primitives work, aligning these surfaces with consistent a11y behavior and neutral styling.
Changes:
- Migrated
AddCertificationModaltoDialog/DialogContent/DialogHeader/DialogFooter, removing bespoke gradient/chrome. - Migrated
IntegrationConfigDialogto the sharedDialogprimitive and simplified the header/close affordances. - Added audit header comments describing the migration context.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| components/registers/add-certification-modal.tsx | Replaced ad-hoc overlay modal with shared Dialog and updated layout/styling. |
| components/integrations/integration-config-dialog.tsx | Replaced ad-hoc overlay modal with shared Dialog, reorganized content into header/body/footer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Modal-audit agent flagged 7 form modals for migration. This PR ships 2 of the cleanest as proof; the other 5 have heavier opinionated styling that warrants per-modal decisions and screenshot review — deferred.
Migrated
fixed inset-0 bg-slate-950/70, "Integration Config" cyan eyebrow,rounded-3xlrounded-[2rem]Both gain: focus trap, ESC, aria-modal, scroll lock, sonner-mounted z-index from Sprint 6b.
Visual change is real
Both modals lose their bespoke gradient/rounded chrome in favour of the neutral Dialog defaults. This is intentional per the stored "enterprise aesthetic over AI feel" preference and the modal-audit finding that explicitly flagged both as cyan-glow offenders. After-merge screenshot review recommended.
Deferred to follow-up PRs (heavier per-modal decisions)
components/team/invite-modal.tsx— white-bg modal needs a deeper light/dark decision before mechanical swapcomponents/team/invite-button.tsx— overlaps with invite-modalcomponents/vault/upload-artifact-modal.tsx— file-upload complexity + a duplicated overlay block the agent flagged as a bugcomponents/vault/credential-inspector-modal.tsxcomponents/compliance-system/plan-activation-flow.tsx— state-machine flow, not a single-screen formValidation
npm run type-checkcleannpm run lint0 errors, 18 warnings (baseline)npx jest5319/5334 pass (no test changes)Test plan
/app/settings/integrations→ click Connect on any integration → Dialog opens centered, focus on first field, ESC dismisses, no cyan eyebrow/app/registers→ click Record Certification → same Dialog UX, no gradient chrome🤖 Generated with Claude Code