Fix modal action button click handling#1785
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds typed close handling, Escape-key listener, and route-change watcher to DialogV2; normalizes link Changes
Sequence DiagramsequenceDiagram
actor User
participant DialogV2 as DialogV2
participant Handler as handleButtonClick
participant Store as dialogv2 Store
participant Router as Browser/Router
User->>DialogV2: click button or anchor (or press Escape)
DialogV2->>Handler: handleButtonClick(button, event)
Handler->>Handler: normalizeRel -> safeButton
Handler->>Handler: compute hasModifier / isLeftClick / isModifiedLinkClick
alt disabled
Handler-->>DialogV2: ignore click
else modified or non-left click for href
Handler->>Handler: event.preventDefault()
Handler->>Store: close(button with skipNavigation=true)
else allowed href or non-href
Handler->>Store: close(button)
end
rect rgba(120,180,240,0.5)
Store->>Store: evaluate preventClose / dialogCanceled
alt allow close
Store->>Store: showDialog = false
opt button.href present and not skipNavigation
Store->>Router: openButtonHref(button) -> navigate (router.push or window.open)
end
else preventClose
Store-->>DialogV2: remain open
end
end
alt Escape pressed or route change
DialogV2->>Store: close(undefined) or close due to route watcher
end
DialogV2->>DialogV2: onUnmounted -> stopRouteWatch, remove Escape listener
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stores/dialogv2.ts (1)
30-34:⚠️ Potential issue | 🟠 MajorMark passive dismissals as canceled and clear stale button metadata.
The no-button path only hides the dialog.
onDialogDismiss()will therefore resolvefalsefor overlay/Escape/close-icon dismissals, andlastButtonRolecan carry over from the previous dialog.💡 Proposed fix
const openDialog = (options: DialogV2Options) => { dialogOptions.value = options showDialog.value = true dialogCanceled.value = false + lastButtonRole.value = '' } @@ - // Modal dismissed without a button action (overlay, escape, close icon) + // Modal dismissed without a button action (overlay, escape, close icon) + dialogCanceled.value = true + lastButtonRole.value = '' showDialog.value = false }Also applies to: 84-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/dialogv2.ts` around lines 30 - 34, openDialog currently only shows the dialog and resets dialogCanceled but doesn't clear stale button metadata or ensure passive dismissals are marked canceled; update openDialog (and the symmetrical close/dismiss path where showDialog is set to false) to explicitly set dialogCanceled.value = true for passive dismissals and clear lastButtonRole/any last button metadata when opening a new dialog by resetting lastButtonRole (and any related lastButton* state) to null/undefined so a previous button role cannot leak into onDialogDismiss() results; locate and change the openDialog function and the dismiss/close code path that hides the dialog (the code that sets showDialog.value = false) to perform these resets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/DialogV2.vue`:
- Around line 20-29: handleButtonClick currently prevents default for every
button.href which breaks modified clicks; change it so event.preventDefault() is
only called when the click should open in the same tab: i.e., when button.href
is set AND (event is undefined/programmatic OR the event is a MouseEvent with
left-button (event.button === 0) and no modifier keys (event.metaKey,
event.ctrlKey, event.shiftKey, event.altKey) ); otherwise do not call
preventDefault so Cmd/Ctrl/Shift/middle-clicks keep normal browser behavior;
update the logic inside handleButtonClick (and any places using
DialogV2Button/close) to implement this check.
In `@src/stores/dialogv2.ts`:
- Around line 43-47: The _blank branch currently calls window.open without
ensuring noopener is set; modify the block that checks button.target ===
'_blank' so it always supplies a third-argument feature string to window.open
containing "noopener" (and "noreferrer" if desired) by normalizing button.rel
into a comma-separated list, deduplicating any existing entries, and joining
them with commas (e.g., build a variable like relFeatures from button.rel and
"noopener" then call window.open(button.href, button.target, relFeatures));
ensure both branches call window.open with that relFeatures string rather than
omitting the third argument.
---
Outside diff comments:
In `@src/stores/dialogv2.ts`:
- Around line 30-34: openDialog currently only shows the dialog and resets
dialogCanceled but doesn't clear stale button metadata or ensure passive
dismissals are marked canceled; update openDialog (and the symmetrical
close/dismiss path where showDialog is set to false) to explicitly set
dialogCanceled.value = true for passive dismissals and clear lastButtonRole/any
last button metadata when opening a new dialog by resetting lastButtonRole (and
any related lastButton* state) to null/undefined so a previous button role
cannot leak into onDialogDismiss() results; locate and change the openDialog
function and the dismiss/close code path that hides the dialog (the code that
sets showDialog.value = false) to perform these resets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5d19fc4-dac1-48a1-905f-68987d0d2d18
📒 Files selected for processing (2)
src/components/DialogV2.vuesrc/stores/dialogv2.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/stores/dialogv2.ts (1)
77-83: Consider consolidating the duplicatepreventClosechecks.The
!button.preventClosecondition is evaluated twice. Nesting the href check would be cleaner:♻️ Proposed simplification
- if (!button.preventClose) { - showDialog.value = false - } - - if (!button.preventClose && button.href) - openButtonHref(button) + if (!button.preventClose) { + showDialog.value = false + if (button.href) + openButtonHref(button) + } return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/dialogv2.ts` around lines 77 - 83, Consolidate the duplicate checks by combining the two !button.preventClose branches into one: if (!button.preventClose) { showDialog.value = false; if (button.href) openButtonHref(button); } then return; Update the block that currently checks !button.preventClose twice (the code touching showDialog.value and openButtonHref) to this single nested check to avoid redundant evaluation.src/components/DialogV2.vue (3)
50-55: Event listener not cleaned up on unmount.The
keydownlistener is added globally but never removed. If this component is ever unmounted and remounted, listeners will accumulate.♻️ Proposed fix using onUnmounted
+import { onMounted, onUnmounted, watch } from 'vue' -import { onMounted, watch } from 'vue' +let escapeHandler: ((event: KeyboardEvent) => void) | null = null + onMounted(() => { // Close dialog on route change watch(route, () => { if (dialogStore.showDialog) { dialogStore.closeDialog() } }) // Close dialog on Escape key - addEventListener('keydown', (event: KeyboardEvent) => { + escapeHandler = (event: KeyboardEvent) => { if (event.key === 'Escape' && dialogStore.showDialog && !dialogStore.dialogOptions?.preventAccidentalClose) { dialogStore.closeDialog() } - }) + } + addEventListener('keydown', escapeHandler) +}) + +onUnmounted(() => { + if (escapeHandler) { + removeEventListener('keydown', escapeHandler) + escapeHandler = null + } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DialogV2.vue` around lines 50 - 55, The global keydown listener added with addEventListener in DialogV2.vue is never removed, causing handler accumulation on remount; wrap the listener registration in the component setup and register a named handler (e.g., onKeydown or handleEscape) and call removeEventListener for that same function inside an onUnmounted hook, keeping the existing condition using dialogStore.showDialog and dialogStore.dialogOptions?.preventAccidentalClose and ensuring dialogStore.closeDialog() is invoked when Escape is detected.
118-120: Redundantcursor-not-allowedclass in both button and anchor.
cursor-not-allowedappears twice when disabled (once standalone, once in the compound rule). Can be simplified:♻️ Proposed cleanup
'!cursor-pointer': !button.disabled, - 'cursor-not-allowed': button.disabled, - 'opacity-70 cursor-not-allowed pointer-events-none': button.disabled, + 'opacity-70 cursor-not-allowed': button.disabled, + 'pointer-events-none': button.disabled,Note: For the
<button>element,pointer-events-nonemay not be needed since:disabledhandles click prevention natively.Also applies to: 139-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DialogV2.vue` around lines 118 - 120, The class binding for the disabled control in DialogV2.vue redundantly applies 'cursor-not-allowed' twice and adds 'pointer-events-none' where native :disabled already prevents clicks; update the class object that references button.disabled so it only includes 'cursor-not-allowed' once (remove the standalone or the one in the compound rule) and remove 'pointer-events-none' for actual <button> elements (keep pointer-events-none only for anchors if used); adjust the same pattern where button.disabled is used later (the similar block around the other occurrence) so both locations use the simplified class list.
128-146: Addaria-disabledfor accessibility on disabled anchor buttons.Disabled anchors lack semantic indication for assistive technologies. While
pointer-events-noneprevents clicks visually, screen readers need explicit notification.♿ Proposed accessibility fix
<a v-else :href="button.href" :target="button.target" :rel="button.rel" + :aria-disabled="button.disabled || undefined" + :tabindex="button.disabled ? -1 : undefined" :class="{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DialogV2.vue` around lines 128 - 146, The anchor rendering in DialogV2.vue should expose disabled state to assistive tech: when rendering the <a> for a button with button.disabled true, add an aria-disabled attribute bound to the boolean (e.g., :aria-disabled="button.disabled") and remove it when false; also ensure the anchor becomes unfocusable when disabled by adding :tabindex="button.disabled ? -1 : undefined" and keep the existing click guard in handleButtonClick to no-op when button.disabled is true. Update the <a> element binding for aria-disabled and tabindex and confirm handleButtonClick checks button.disabled before acting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/DialogV2.vue`:
- Around line 50-55: The global keydown listener added with addEventListener in
DialogV2.vue is never removed, causing handler accumulation on remount; wrap the
listener registration in the component setup and register a named handler (e.g.,
onKeydown or handleEscape) and call removeEventListener for that same function
inside an onUnmounted hook, keeping the existing condition using
dialogStore.showDialog and dialogStore.dialogOptions?.preventAccidentalClose and
ensuring dialogStore.closeDialog() is invoked when Escape is detected.
- Around line 118-120: The class binding for the disabled control in
DialogV2.vue redundantly applies 'cursor-not-allowed' twice and adds
'pointer-events-none' where native :disabled already prevents clicks; update the
class object that references button.disabled so it only includes
'cursor-not-allowed' once (remove the standalone or the one in the compound
rule) and remove 'pointer-events-none' for actual <button> elements (keep
pointer-events-none only for anchors if used); adjust the same pattern where
button.disabled is used later (the similar block around the other occurrence) so
both locations use the simplified class list.
- Around line 128-146: The anchor rendering in DialogV2.vue should expose
disabled state to assistive tech: when rendering the <a> for a button with
button.disabled true, add an aria-disabled attribute bound to the boolean (e.g.,
:aria-disabled="button.disabled") and remove it when false; also ensure the
anchor becomes unfocusable when disabled by adding :tabindex="button.disabled ?
-1 : undefined" and keep the existing click guard in handleButtonClick to no-op
when button.disabled is true. Update the <a> element binding for aria-disabled
and tabindex and confirm handleButtonClick checks button.disabled before acting.
In `@src/stores/dialogv2.ts`:
- Around line 77-83: Consolidate the duplicate checks by combining the two
!button.preventClose branches into one: if (!button.preventClose) {
showDialog.value = false; if (button.href) openButtonHref(button); } then
return; Update the block that currently checks !button.preventClose twice (the
code touching showDialog.value and openButtonHref) to this single nested check
to avoid redundant evaluation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a69ca97-554a-4437-9cb4-e1aadab69ed7
📒 Files selected for processing (2)
src/components/DialogV2.vuesrc/stores/dialogv2.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/DialogV2.vue`:
- Line 4: Reorder the Vue imports so type-only imports come after value imports
to satisfy the perfectionist/sort-imports rule: move the type import
"WatchStopHandle" (import type { WatchStopHandle } from 'vue') below any
non-type imports from 'vue' in DialogV2.vue (or combine value and type imports
into a single statement with the value members first and "type { WatchStopHandle
}" after), then run the frontend linter fixer (bun lint:fix) before committing.
In `@src/stores/dialogv2.ts`:
- Line 49: The lint error comes from the arrow function used inside
relTokens.some; change the arrow callback from using parentheses around the
single parameter to the idiomatic form without them (i.e., replace (token) =>
token.toLowerCase() === 'noreferrer' with token => token.toLowerCase() ===
'noreferrer') so the style/arrow-parens rule is satisfied in the
relTokens.some(...) expression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0c15fc6-9bfb-4894-bbf9-938b6dfcf083
📒 Files selected for processing (2)
src/components/DialogV2.vuesrc/stores/dialogv2.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/DialogV2.vue`:
- Around line 31-35: The early return for modified clicks (isModifiedLinkClick)
skips close(button) and the window.open hardening; change the branch so that for
modified link clicks you still call close(button) and enforce noopener when
button.target === '_blank' (add 'noopener' to button.rel if missing) or
otherwise run the same window.open hardening logic before returning; reference
the hasModifier/isModifiedLinkClick check and ensure close(button),
button.target, button.rel and the window.open hardening code paths are executed
for that case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00da4828-35c1-47da-beb9-110836a1eb14
📒 Files selected for processing (2)
src/components/DialogV2.vuesrc/stores/dialogv2.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/stores/dialogv2.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
messages/id.json (1)
1114-1114: Optional consistency polish for mixed-language terms.Consider localizing
updateandweb appto keep Indonesian terminology more consistent in this sentence.✏️ Suggested wording adjustment
- "onboarding-cli-where-why": "Buka terminal di proyek Capacitor Anda (folder yang berisi package.json) dan jalankan perintah ini. Langkah onboarding sederhana ini biasanya memakan waktu 2 hingga 10 menit dan memungkinkan Anda menguji update pertama di aplikasi Anda dengan Capgo Updater. Kami menggunakan CLI (bukan web app) karena CLI memeriksa kode native, mendeteksi detail aplikasi secara otomatis, dan menerapkan pengaturan paling aman.", + "onboarding-cli-where-why": "Buka terminal di proyek Capacitor Anda (folder yang berisi package.json) dan jalankan perintah ini. Langkah onboarding sederhana ini biasanya memakan waktu 2 hingga 10 menit dan memungkinkan Anda menguji pembaruan pertama di aplikasi Anda dengan Capgo Updater. Kami menggunakan CLI (bukan aplikasi web) karena CLI memeriksa kode native, mendeteksi detail aplikasi secara otomatis, dan menerapkan pengaturan paling aman.",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@messages/id.json` at line 1114, The string for key "onboarding-cli-where-why" mixes English terms "update" and "web app"; please revise the value to consistently use Indonesian equivalents (e.g., replace "update" with "pembaruan" and "web app" with "aplikasi web") while preserving the original meaning and punctuation so the JSON key "onboarding-cli-where-why" remains unchanged.src/components/DialogV2.vue (1)
13-21: Consider centralizingrelnormalization in one shared utility.
normalizeRelhere and the_blankfeature normalization in the store now encode related rules in two places; extracting one shared helper would reduce drift risk over time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DialogV2.vue` around lines 13 - 21, The rel normalization logic is duplicated between the normalizeRel function in DialogV2.vue and the _blank handling in the store; extract this into a shared utility (e.g., create a helper function normalizeRel in a common utils module) and replace both the local normalizeRel and the store's inline _blank normalization to call that shared helper (update imports and usages for normalizeRel in DialogV2.vue and the store code to use the new centralized function).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@messages/es.json`:
- Line 893: Replace the awkward time phrasing for the "init-capgo-in-your-a"
message key: change the string value "Instala Capgo en tu aplicación Capacitor
en 2 a 10 minutos" to the more natural Spanish "Instala Capgo en tu aplicación
Capacitor entre 2 y 10 minutos" so it matches the phrasing used elsewhere (e.g.,
line 1114) and improves readability.
In `@messages/ru.json`:
- Line 1114: The phrasing for the "onboarding-cli-where-why" message is
grammatically awkward due to the feminine pronoun "она" referring to CLI; update
the string to use a neutral construction such as "CLI проверяет нативный код"
(e.g., replace "потому что она проверяет нативный код" with "потому что CLI
проверяет нативный код") and ensure any following verbs/phrases agree (e.g.,
keep "автоматически определяет детали приложения и применяет самый безопасный
вариант настройки") so the whole sentence reads naturally in Russian.
In `@src/stores/dialogv2.ts`:
- Around line 47-53: The code currently passes arbitrary tokens from button.rel
into window.open's features via relFeatures (relTokens, relSet, relFeatures),
which can trigger popup behavior; instead only allow the safe tokens 'noopener'
and 'noreferrer' and drop any other rel values before calling window.open.
Change the logic around relTokens/relSet to explicitly build a list containing
only 'noopener' and, if present in the original rel (case-insensitive),
'noreferrer', then join that list into relFeatures and pass it to
window.open(button.href, button.target, relFeatures).
---
Nitpick comments:
In `@messages/id.json`:
- Line 1114: The string for key "onboarding-cli-where-why" mixes English terms
"update" and "web app"; please revise the value to consistently use Indonesian
equivalents (e.g., replace "update" with "pembaruan" and "web app" with
"aplikasi web") while preserving the original meaning and punctuation so the
JSON key "onboarding-cli-where-why" remains unchanged.
In `@src/components/DialogV2.vue`:
- Around line 13-21: The rel normalization logic is duplicated between the
normalizeRel function in DialogV2.vue and the _blank handling in the store;
extract this into a shared utility (e.g., create a helper function normalizeRel
in a common utils module) and replace both the local normalizeRel and the
store's inline _blank normalization to call that shared helper (update imports
and usages for normalizeRel in DialogV2.vue and the store code to use the new
centralized function).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 652ae220-6084-481f-8843-a7f985a30a5f
📒 Files selected for processing (17)
messages/de.jsonmessages/en.jsonmessages/es.jsonmessages/fr.jsonmessages/hi.jsonmessages/id.jsonmessages/it.jsonmessages/ja.jsonmessages/ko.jsonmessages/pl.jsonmessages/pt-br.jsonmessages/ru.jsonmessages/tr.jsonmessages/vi.jsonmessages/zh-cn.jsonsrc/components/DialogV2.vuesrc/stores/dialogv2.ts
✅ Files skipped from review due to trivial changes (2)
- messages/vi.json
- messages/de.json
|
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
1 participant
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 (AI generated)
DialogV2footer actions to handle click events consistently so links and buttons can trigger expected dialog behavior.window.openoptions for_blanknavigation to always includenoopener.hreflink interactions when modifier keys are used.Motivation (AI generated)
Users reported the deployment-status modal action button was not behaving correctly and needed a safe interaction model that still supports browser-native link semantics.
Business Impact (AI generated)
This restores reliable modal action behavior while preserving expected navigation patterns, preventing users from being blocked or redirected unexpectedly.
Test Plan (AI generated)
bun lintScreenshots
Checklist (AI generated)
Summary by CodeRabbit
New Features
Bug Fixes
Style
Localization