Ergonomic update with escape and space keys - #4707
Conversation
escape allow to unselect while deleteing many items, no action on space keypress othgerwise the number for items to be deleted is empty
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe PR adds shared modal-aware panel shortcut handling, applies it across gallery and face-recognition views, adjusts delete-dialog focus behavior, changes Escape selection handling, and documents shortcuts in English and French. ChangesShortcut coordination
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 unsupported.)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
resources/js/v8/components/forms/gallery-dialogs/DeleteDialog.vue (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the Vue component-library guideline for v8.
The v8 entry point registers
@nuxt/ui/vue-plugin, and v8 components useUModal. KeepUModalhere and document the v8 exception to the PrimeVue guideline.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 97bff0b8-8df0-4f04-a5e3-96b01ba36ae2
📒 Files selected for processing (19)
docs/specs/3-reference/keyboard-shortcuts.mdresources/js/stores/ModalsState.tsresources/js/v8/components/forms/gallery-dialogs/DeleteDialog.vueresources/js/v8/components/gallery/photoModule/FaceOverlay.vueresources/js/v8/components/headers/AlbumsHeader.vueresources/js/v8/components/headers/TimelineHeader.vueresources/js/v8/composables/usePanelShortcuts.tsresources/js/v8/views/Statistics.vueresources/js/v8/views/face-recog/FaceClusters.vueresources/js/v8/views/face-recog/PersonDetail.vueresources/js/v8/views/gallery-panels/Album.vueresources/js/v8/views/gallery-panels/Albums.vueresources/js/v8/views/gallery-panels/Favourites.vueresources/js/v8/views/gallery-panels/Flow.vueresources/js/v8/views/gallery-panels/Frame.vueresources/js/v8/views/gallery-panels/Map.vueresources/js/v8/views/gallery-panels/Search.vueresources/js/v8/views/gallery-panels/Tag.vueresources/js/v8/views/gallery-panels/Timeline.vue
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| is_modal_open(state): boolean { | ||
| return ( | ||
| state.is_login_open || | ||
| state.is_webauthn_open || | ||
| state.is_metrics_open || | ||
| state.is_upload_visible || | ||
| state.is_camera_capture_visible || | ||
| state.is_create_album_visible || | ||
| state.is_create_tag_album_visible || | ||
| state.is_create_person_album_visible || | ||
| state.is_album_edit_open || | ||
| state.is_photo_edit_open || | ||
| state.is_rename_visible || | ||
| state.is_move_visible || | ||
| state.is_delete_visible || | ||
| state.is_merge_album_visible || | ||
| state.is_share_album_visible || | ||
| state.is_embed_code_visible || | ||
| state.is_import_from_link_open || | ||
| state.is_import_from_dropbox_open || | ||
| state.is_import_from_server_open || | ||
| state.is_tag_visible || | ||
| state.is_license_visible || | ||
| state.is_copy_visible || | ||
| state.is_apply_renamer_visible || | ||
| state.is_watermark_confirm_visible || | ||
| state.is_keybindings_help_open |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
printf '%s\n' '--- scoped repository guidance ---'
find /tmp/coderabbit-repo-knowledge/lycheeorg-lychee-cc271fdf -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- frontend conventions ---'
sed -n '1,240p' docs/specs/3-reference/coding-conventions.md
printf '%s\n' '--- target symbols and files ---'
rg -n -C 5 'is_modal_open|definePanelShortcuts|isAssignmentOpen|is_download_album_visible|Escape|KeyP|key.*p|modal' \
resources/js/stores/ModalsState.ts \
resources/js/v8/components/gallery/photoModule/FaceOverlay.vue \
resources/js/v8/views/gallery-panels/Albums.vue \
resources/js/v8 \
resources/js/storesRepository: LycheeOrg/Lychee
Length of output: 50373
🏁 Script executed:
printf '%s\n' '--- relevant repository guidance files ---'
find /tmp/coderabbit-repo-knowledge/lycheeorg-lychee-cc271fdf -maxdepth 2 -type f -name '*.md' \
| grep -Ei 'frontend|vue|modal|gallery|shortcut|coding|learn'
printf '%s\n' '--- ModalsState ---'
cat -n resources/js/stores/ModalsState.ts | sed -n '1,150p'
printf '%s\n' '--- panel shortcut composable ---'
fd -i 'usePanelShortcuts' resources/js
for f in $(fd -i 'usePanelShortcuts' resources/js); do
echo "### $f"
cat -n "$f"
done
printf '%s\n' '--- FaceOverlay relevant section ---'
cat -n resources/js/v8/components/gallery/photoModule/FaceOverlay.vue | sed -n '1,150p'
printf '%s\n' '--- Albums shortcut and state sections ---'
cat -n resources/js/v8/views/gallery-panels/Albums.vue | sed -n '250,375p'
printf '%s\n' '--- modal implementations ---'
fd -i 'DownloadAlbum|FaceAssignment|Assignment' resources/js/v8Repository: LycheeOrg/Lychee
Length of output: 20193
🏁 Script executed:
printf '%s\n' '--- remaining is_modal_open consumers and shortcut registrations ---'
rg -n -C 4 'is_modal_open|definePanelShortcuts|defineShortcuts' resources/js --glob '*.ts' --glob '*.vue'
printf '%s\n' '--- Face assignment modal ---'
cat -n resources/js/v8/components/modals/faceRecog/FaceAssignmentModal.vue | sed -n '1,240p'
printf '%s\n' '--- Download album modal ---'
cat -n resources/js/v8/components/modals/DownloadAlbum.vue | sed -n '1,240p'
printf '%s\n' '--- frontend dependency contract ---'
rg -n -C 3 '"`@nuxt/ui`"|nuxt/ui|defineShortcuts' package.json package-lock.json pnpm-lock.yaml yarn.lock 2>/dev/null || trueRepository: LycheeOrg/Lychee
Length of output: 43048
🌐 Web query:
Nuxt UI 4.11.0 defineShortcuts reactive computed config empty object documentation
💡 Result:
In Nuxt UI 4.11.0, the defineShortcuts composable accepts a MaybeRef configuration object, allowing you to pass a ref or a computed property to update shortcuts reactively [1][2]. When you provide a reactive source (like a computed property) to defineShortcuts, the composable tracks changes to that object [1][2]. If you want to disable or remove shortcuts dynamically, you can set the corresponding key in your configuration object to false, null, or undefined [1][3][4]. Regarding your query about an "empty object": 1. If the reactive source resolves to an empty object {}, all existing keyboard shortcuts previously defined by that specific defineShortcuts call will effectively be disabled or ignored because the configuration no longer maps any keys to handlers [1][5][2]. 2. The internal implementation uses a computed property that iterates over Object.entries(toValue(config)) [5]. If the object is empty, no shortcut logic is executed or registered for that reactive instance [5]. Best Practices: - Reactive Updates: To swap shortcuts, use a computed property that returns a full configuration object based on your application state, rather than mutating an object in place [2]. - Disabling: To conditionally enable or disable individual shortcuts, explicitly set them to false or use the whenever option (if available in your specific version/variant) to add conditional constraints to the handler [1][6][7]. - Cleanup: defineShortcuts returns a function that removes the registered event listeners [1][2]. If you are replacing shortcuts entirely, you may want to ensure you are not creating redundant listeners if the composable is called multiple times rather than updated reactively [7].
Citations:
- 1: https://ui.nuxt.com/docs/composables/define-shortcuts
- 2: https://mintlify.wiki/nuxt/ui/composables/use-shortcuts
- 3: https://app.unpkg.com/@nuxt/ui@4.11.0/files/dist/runtime/composables/defineShortcuts.d.ts
- 4: https://cdn.jsdelivr.net/npm/@nuxt/ui@4.6.0/dist/runtime/composables/defineShortcuts.d.ts
- 5: https://github.com/nuxt/ui/blob/90a94fb1/src/runtime/composables/defineShortcuts.ts
- 6: https://ui2.nuxt.com/getting-started/shortcuts
- 7: GitHub issue 693 in nuxt/ui (link omitted to avoid creating a cross-reference)
🏁 Script executed:
printf '%s\n' '--- scoped learnings for frontend and modals ---'
sed -n '1,220p' /tmp/coderabbit-repo-knowledge/lycheeorg-lychee-cc271fdf/learnings/resources-js-v8.md
sed -n '1,220p' /tmp/coderabbit-repo-knowledge/lycheeorg-lychee-cc271fdf/learnings/resources-js.md
printf '%s\n' '--- exact diff summary for the reviewed paths ---'
git diff --stat -- resources/js/stores/ModalsState.ts resources/js/v8/components/gallery/photoModule/FaceOverlay.vue resources/js/v8/views/gallery-panels/Albums.vue
git diff -- resources/js/stores/ModalsState.ts resources/js/v8/components/gallery/photoModule/FaceOverlay.vue resources/js/v8/views/gallery-panels/Albums.vue | sed -n '1,260p'Repository: LycheeOrg/Lychee
Length of output: 1097
Track locally owned dialogs in the shared modal state.
definePanelShortcuts disables panel shortcuts only when togglableStore.is_modal_open is true. The local isAssignmentOpen and is_download_album_visible refs are not included, so panel shortcuts remain active while either UModal is open. In FaceOverlay.vue, P can also unmount the assignment modal through its parent v-if.
Bind both dialog states to the shared modal state. Add regression tests for Escape in both dialogs and P in the face-assignment dialog.
📍 Affects 3 files
resources/js/stores/ModalsState.ts#L87-L113(this comment)resources/js/v8/components/gallery/photoModule/FaceOverlay.vue#L85-L85resources/js/v8/views/gallery-panels/Albums.vue#L349-L349
There was a problem hiding this comment.
Add is_download_album_visible to the useTogglablesStateStore and reuse it instead of the refs:
const is_download_album_visible = ref(false);
const {
is_share_album_visible,
is_download_album_visible, // < HERE
toggleDelete,
toggleMergeAlbum,
toggleMove,
toggleRename,
toggleShareAlbum,
toggleEmbedCode,
toggleTag,
toggleLicense,
toggleCopy,
toggleUpload,
toggleApplyRenamer,
is_apply_renamer_visible,
toggleWatermarkConfirm,
is_watermark_confirm_visible,
} = useGalleryModals(togglableStore);
We could also do the same with isAssignmentOpen that would be more clean.
There was a problem hiding this comment.
@ildyria, agreed. Use useTogglablesStateStore for both dialog visibility states.
This change will make is_modal_open cover the download dialog and the face-assignment dialog. It will also remove the component-local visibility refs.
- Add
is_download_album_visibleand a face-assignment visibility field toresources/js/stores/ModalsState.ts. - Include both fields in
is_modal_open. - Replace the local refs in the affected components with store-backed state.
- Add regression coverage for shortcut suppression while each dialog is open.
✏️ Learnings added
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 4707
File: resources/js/stores/ModalsState.ts:87-113
Timestamp: 2026-09-03T21:04:39.467Z
Learning: In the Lychee Vue frontend, dialog visibility that must disable `definePanelShortcuts` should use fields from `useTogglablesStateStore` and be included in its `is_modal_open` getter. Component-local dialog refs do not participate in shared shortcut gating.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
- 📌 Create a pull request with these changes
You are interacting with an AI system.
ildyria
left a comment
ildyria
left a comment
There was a problem hiding this comment.
That is 99% there.
Just the person selection issue.
2 participants
escape close the modal, not the background page
escape allow to unselect
while deleteing many items, no action on space keypress othgerwise the number for items to be deleted is empty
Summary by CodeRabbit
New Features
Bug Fixes