🐛 Replace all window.confirm() with styled ConfirmDialog#4057
🐛 Replace all window.confirm() with styled ConfirmDialog#4057clubanderson merged 1 commit intomainfrom
Conversation
Replace native browser confirm dialogs with the existing ConfirmDialog component across 5 files (6 instances). Add ESLint no-restricted-globals rule to prevent future use of alert/confirm/prompt. Files changed: - FeatureRequestModal.tsx - FeedbackModal.tsx - ReservationFormModal.tsx - AlertRuleEditor.tsx - CreateNamespaceModal.tsx - GrantAccessModal.tsx - eslint.config.js (added no-restricted-globals rule) Signed-off-by: Andrew Anderson <andy@clubanderson.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for kubestellarconsole ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
There was a problem hiding this comment.
Pull request overview
Replaces native window.confirm() prompts in several UI flows with the shared styled ConfirmDialog, and adds an ESLint rule intended to prevent future use of native browser dialogs.
Changes:
- Swapped discard/unsaved-changes confirmation from
window.confirm()toConfirmDialogacross multiple modal components. - Added per-modal state (
showDiscardConfirm) andforceClosehandlers to manage confirm/close behavior. - Added an ESLint
no-restricted-globalsrule targetingalert,confirm, andprompt.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/components/namespaces/GrantAccessModal.tsx | Replaces unsaved-changes close confirmation with ConfirmDialog. |
| web/src/components/namespaces/CreateNamespaceModal.tsx | Replaces unsaved-changes close confirmation with ConfirmDialog. |
| web/src/components/gpu/ReservationFormModal.tsx | Replaces unsaved-changes close confirmation with ConfirmDialog. |
| web/src/components/feedback/FeedbackModal.tsx | Adds ConfirmDialog + refactors close logic into forceClose/handleClose. |
| web/src/components/feedback/FeatureRequestModal.tsx | Replaces unsaved-changes close confirmation with ConfirmDialog. |
| web/src/components/alerts/AlertRuleEditor.tsx | Replaces unsaved-changes close confirmation with ConfirmDialog. |
| web/eslint.config.js | Adds no-restricted-globals entries for alert/confirm/prompt. |
| 'no-restricted-globals': ['error', | ||
| { name: 'alert', message: 'Use ConfirmDialog or Toast instead of browser alert().' }, | ||
| { name: 'confirm', message: 'Use ConfirmDialog instead of browser confirm().' }, | ||
| { name: 'prompt', message: 'Use a styled input modal instead of browser prompt().' }, | ||
| ], |
There was a problem hiding this comment.
The new no-restricted-globals rule will block confirm()/alert()/prompt() as bare globals, but it will not catch the pattern previously used in this codebase (window.confirm(...)). To actually prevent regressions, add a restriction for window.confirm/alert/prompt as well (e.g., no-restricted-properties for window/globalThis, or a no-restricted-syntax selector targeting MemberExpression[property.name="confirm"]).
| title={t('common:common.discardUnsavedChanges', 'Discard unsaved changes?')} | ||
| message={t('common:common.discardUnsavedChangesMessage', 'You have unsaved changes that will be lost.')} | ||
| confirmLabel={t('common:common.discard', 'Discard')} | ||
| cancelLabel={t('common:common.keepEditing', 'Keep editing')} |
There was a problem hiding this comment.
The i18n keys used here (common.discardUnsavedChanges, common.discardUnsavedChangesMessage, common.discard, common.keepEditing) don't exist in web/src/locales/en/common.json, so the UI will always fall back to the default strings. Consider adding these keys to common.json (preferred) or switching to existing common action keys to keep translations consistent.
| title={t('common:common.discardUnsavedChanges', 'Discard unsaved changes?')} | |
| message={t('common:common.discardUnsavedChangesMessage', 'You have unsaved changes that will be lost.')} | |
| confirmLabel={t('common:common.discard', 'Discard')} | |
| cancelLabel={t('common:common.keepEditing', 'Keep editing')} | |
| title={t('common:discardUnsavedChanges', 'Discard unsaved changes?')} | |
| message={t('common:discardUnsavedChangesMessage', 'You have unsaved changes that will be lost.')} | |
| confirmLabel={t('common:discard', 'Discard')} | |
| cancelLabel={t('common:keepEditing', 'Keep editing')} |
| title={t('common:common.discardUnsavedChanges', 'Discard unsaved changes?')} | ||
| message={t('common:common.discardUnsavedChangesMessage', 'You have unsaved changes that will be lost.')} | ||
| confirmLabel={t('common:common.discard', 'Discard')} | ||
| cancelLabel={t('common:common.keepEditing', 'Keep editing')} |
There was a problem hiding this comment.
The i18n keys used here (common.discardUnsavedChanges, common.discardUnsavedChangesMessage, common.discard, common.keepEditing) don't exist in web/src/locales/en/common.json, so the UI will always fall back to the default strings. Consider adding these keys to common.json (preferred) or switching to existing common action keys to keep translations consistent.
| title={t('common:common.discardUnsavedChanges', 'Discard unsaved changes?')} | |
| message={t('common:common.discardUnsavedChangesMessage', 'You have unsaved changes that will be lost.')} | |
| confirmLabel={t('common:common.discard', 'Discard')} | |
| cancelLabel={t('common:common.keepEditing', 'Keep editing')} | |
| title={t('common:discardUnsavedChanges', 'Discard unsaved changes?')} | |
| message={t('common:discardUnsavedChangesMessage', 'You have unsaved changes that will be lost.')} | |
| confirmLabel={t('common:discard', 'Discard')} | |
| cancelLabel={t('common:keepEditing', 'Keep editing')} |
| title={t('common:common.discardUnsavedChanges', 'Discard unsaved changes?')} | ||
| message={t('common:common.discardUnsavedChangesMessage', 'You have unsaved changes that will be lost.')} | ||
| confirmLabel={t('common:common.discard', 'Discard')} | ||
| cancelLabel={t('common:common.keepEditing', 'Keep editing')} |
There was a problem hiding this comment.
The i18n keys used here (common.discardUnsavedChanges, common.discardUnsavedChangesMessage, common.discard, common.keepEditing) don't exist in web/src/locales/en/common.json, so the UI will always fall back to the default strings. Consider adding these keys to common.json (preferred) or switching to existing common action keys to keep translations consistent.
| title={t('common:common.discardUnsavedChanges', 'Discard unsaved changes?')} | |
| message={t('common:common.discardUnsavedChangesMessage', 'You have unsaved changes that will be lost.')} | |
| confirmLabel={t('common:common.discard', 'Discard')} | |
| cancelLabel={t('common:common.keepEditing', 'Keep editing')} | |
| title={t('common:discardUnsavedChanges', 'Discard unsaved changes?')} | |
| message={t('common:discardUnsavedChangesMessage', 'You have unsaved changes that will be lost.')} | |
| confirmLabel={t('common:discard', 'Discard')} | |
| cancelLabel={t('common:keepEditing', 'Keep editing')} |
| const handleClose = () => { | ||
| if (!success && (title.trim() !== '' || description.trim() !== '')) { | ||
| setShowDiscardConfirm(true) | ||
| return | ||
| } | ||
| forceClose() | ||
| } |
There was a problem hiding this comment.
handleClose is used from the Escape/Space keydown listener, but that useEffect only depends on isOpen. This can leave the key handler with stale success/title/description values, so Escape may discard edits without showing the confirm dialog. Fix by making the key handler depend on handleClose (and making handleClose stable via useCallback), or by using a ref to always read the latest state in the listener.
| title={t('common:common.discardUnsavedChanges', 'Discard unsaved changes?')} | ||
| message={t('common:common.discardUnsavedChangesMessage', 'You have unsaved changes that will be lost.')} | ||
| confirmLabel={t('common:common.discard', 'Discard')} | ||
| cancelLabel={t('common:common.keepEditing', 'Keep editing')} |
There was a problem hiding this comment.
The i18n keys used here (common.discardUnsavedChanges, common.discardUnsavedChangesMessage, common.discard, common.keepEditing) don't exist in web/src/locales/en/common.json, so the UI will always fall back to the default strings. Consider adding these keys to common.json (preferred) or switching to existing common action keys to keep translations consistent.
| title={t('common:common.discardUnsavedChanges', 'Discard unsaved changes?')} | |
| message={t('common:common.discardUnsavedChangesMessage', 'You have unsaved changes that will be lost.')} | |
| confirmLabel={t('common:common.discard', 'Discard')} | |
| cancelLabel={t('common:common.keepEditing', 'Keep editing')} | |
| title="Discard unsaved changes?" | |
| message="You have unsaved changes that will be lost." | |
| confirmLabel="Discard" | |
| cancelLabel="Keep editing" |
| title={t('common:common.discardUnsavedChanges', 'Discard unsaved changes?')} | ||
| message={t('common:common.discardUnsavedChangesMessage', 'You have unsaved changes that will be lost.')} | ||
| confirmLabel={t('common:common.discard', 'Discard')} | ||
| cancelLabel={t('common:common.keepEditing', 'Keep editing')} |
There was a problem hiding this comment.
The i18n keys used here (common.discardUnsavedChanges, common.discardUnsavedChangesMessage, common.discard, common.keepEditing) don't exist in web/src/locales/en/common.json, so the UI will always fall back to the default strings. Consider adding these keys to common.json (preferred) or switching to existing common action keys to keep translations consistent.
| title={t('common:common.discardUnsavedChanges', 'Discard unsaved changes?')} | |
| message={t('common:common.discardUnsavedChangesMessage', 'You have unsaved changes that will be lost.')} | |
| confirmLabel={t('common:common.discard', 'Discard')} | |
| cancelLabel={t('common:common.keepEditing', 'Keep editing')} | |
| title={t('feedback.discardUnsavedChangesTitle', 'Discard unsaved changes?')} | |
| message={t('feedback.discardUnsavedChangesMessage', 'You have unsaved changes that will be lost.')} | |
| confirmLabel={t('feedback.discard', 'Discard')} | |
| cancelLabel={t('feedback.keepEditing', 'Keep editing')} |
| <ConfirmDialog | ||
| isOpen={showDiscardConfirm} | ||
| onClose={() => setShowDiscardConfirm(false)} | ||
| onConfirm={forceClose} | ||
| title={t('common:common.discardUnsavedChanges', 'Discard unsaved changes?')} | ||
| message={t('common:common.discardUnsavedChangesMessage', 'You have unsaved changes that will be lost.')} | ||
| confirmLabel={t('common:common.discard', 'Discard')} | ||
| cancelLabel={t('common:common.keepEditing', 'Keep editing')} | ||
| variant="warning" | ||
| /> |
There was a problem hiding this comment.
ConfirmDialog uses BaseModal with z-[9999], but this component also renders the login prompt overlay at z-[10001]. If the user attempts to close with unsaved changes while the login prompt is open, the discard confirmation can render behind the login overlay and be non-interactive. Consider handling close by dismissing the login prompt first, or ensure the discard confirmation renders above any in-modal overlays (e.g., allow ConfirmDialog/BaseModal to take a higher z-index).
| title={t('common:common.discardUnsavedChanges', 'Discard unsaved changes?')} | ||
| message={t('common:common.discardUnsavedChangesMessage', 'You have unsaved changes that will be lost.')} | ||
| confirmLabel={t('common:common.discard', 'Discard')} | ||
| cancelLabel={t('common:common.keepEditing', 'Keep editing')} |
There was a problem hiding this comment.
The i18n keys used here (common.discardUnsavedChanges, common.discardUnsavedChangesMessage, common.discard, common.keepEditing) don't exist in web/src/locales/en/common.json, so the UI will always fall back to the default strings. Consider adding these keys to common.json (preferred) or switching to existing common action keys to keep translations consistent.
| title={t('common:common.discardUnsavedChanges', 'Discard unsaved changes?')} | |
| message={t('common:common.discardUnsavedChangesMessage', 'You have unsaved changes that will be lost.')} | |
| confirmLabel={t('common:common.discard', 'Discard')} | |
| cancelLabel={t('common:common.keepEditing', 'Keep editing')} | |
| title={t('common:discardUnsavedChanges', 'Discard unsaved changes?')} | |
| message={t('common:discardUnsavedChangesMessage', 'You have unsaved changes that will be lost.')} | |
| confirmLabel={t('common:discard', 'Discard')} | |
| cancelLabel={t('common:keepEditing', 'Keep editing')} |
🔄 Auto-Applying Copilot Code ReviewCopilot code review found 6 code suggestion(s) and 3 general comment(s). @copilot Please apply all of the following code review suggestions:
Also address these general comments:
Push all fixes in a single commit. Run Auto-generated by copilot-review-apply workflow. |
Summary
window.confirm()across 5 modal components with the existing styledConfirmDialogcomponentno-restricted-globalsrule to error on future use ofalert,confirm, orpromptFiles changed:
FeatureRequestModal.tsx— discard unsaved changes confirmFeedbackModal.tsx— discard unsaved changes confirmReservationFormModal.tsx— discard unsaved changes confirmAlertRuleEditor.tsx— discard unsaved changes confirmCreateNamespaceModal.tsx— discard unsaved changes confirmGrantAccessModal.tsx— discard unsaved changes confirmeslint.config.js— addedno-restricted-globalsruleTest plan
npm run lintpasses