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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 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 Merge Risk: 🟡 Moderate · up to Escape handling can still trigger background actions while dialogs are open and can navigate away instead of clearing selections in the person view, creating unintended user actions. The PR should not be considered merge-ready until these bounded shortcut conflicts are fixed or explicitly accepted by the owner. 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
|
|
||
| // Keybindings | ||
| defineShortcuts({ | ||
| definePanelShortcuts({ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clear the local batch selection before navigating away.
definePanelShortcuts detects only togglableStore.selectedPhotosIds and togglableStore.selectedAlbumsIds. PersonDetail.vue stores its batch selection in local selectedPhotoIds, so Escape does not use the shared selection-clearing path. When batch mode has selected photos and no photo is open, Escape still executes the fallback navigation to people instead of clearing the selection. Handle the local selection before the navigation branch, or extend the composable to accept a local selection and clear callback.
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