Added keyboard accessible shortcut menu for create modals #457
Conversation
WalkthroughThis pull request introduces enhancements to the frontend components, primarily focusing on adding a new Quick Menu feature. The changes include modifications to the Changes
Sequence DiagramsequenceDiagram
participant User
participant QuickMenuModal
participant QuickMenuInput
participant Actions
User->>QuickMenuModal: Open Modal (Ctrl + Backquote)
QuickMenuModal->>QuickMenuInput: Focus Input
User->>QuickMenuInput: Type Action
QuickMenuInput->>Actions: Filter Available Actions
User->>QuickMenuInput: Select Action
QuickMenuInput->>QuickMenuModal: Invoke Selected Action
QuickMenuModal->>User: Close Modal
Possibly related PRs
Suggested reviewers
Security Recommendations
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🔇 Additional comments (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 3
🧹 Nitpick comments (2)
frontend/components/global/QuickMenu/Input.vue (1)
96-103: Optimize action filtering performanceThe current filtering implementation could be optimized:
Consider:
const filteredActions = computed(() => { + const searchTerm = inputValue.value.toLowerCase(); return (props.actions || []).filter(action => { return ( - action.text.toLowerCase().includes(inputValue.value.toLowerCase()) || - action.shortcut?.includes(inputValue.value.toLowerCase()) + action.text.toLowerCase().includes(searchTerm) || + action.shortcut?.toLowerCase().includes(searchTerm) ); }); });frontend/layouts/default.vue (1)
245-261: Enhance action handling security and maintainabilityThe current implementation of quick menu actions has several areas for improvement:
- Consider using an action registry pattern instead of direct modal manipulation
- Add input validation for shortcuts
- Implement proper action logging for security auditing
Example implementation:
const actionRegistry = { registerAction(action: QuickMenuAction) { // Validate shortcut if (action.shortcut && !/^[1-9]$/.test(action.shortcut)) { throw new Error('Invalid shortcut'); } // Log action registration console.log(`Registering action: ${action.text}`); return action; } }; const quickMenuActions = ref([ actionRegistry.registerAction({ text: "Create Item", action: () => (modals.item = true), shortcut: "1", }), // ... other actions ]);
🛑 Comments failed to post (3)
frontend/components/global/QuickMenu/Modal.vue (1)
62-65: 🛠️ Refactor suggestion
Review timeout usage in action invocation
The 100ms timeout in
invokeActioncould lead to race conditions or user experience issues:
- Multiple rapid actions might queue up
- Actions might execute in unexpected order
Consider using a more robust approach:
- useTimeoutFn(action.action, 100).start(); + await nextTick(); + action.action();Committable suggestion skipped: line range outside the PR's diff.
frontend/components/global/QuickMenu/Input.vue (1)
87-94: 🛠️ Refactor suggestion
Enhance keyboard shortcut security and accessibility
The current keyboard shortcut implementation could be improved:
- No prevention of shortcut conflicts
- Missing ARIA labels for accessibility
- No rate limiting on shortcut triggers
Consider:
- Adding a shortcut registry to prevent conflicts
- Implementing proper ARIA labels
- Adding rate limiting for shortcut triggers
frontend/layouts/default.vue (1)
236-243: 🛠️ Refactor suggestion
Improve modal management security
The current implementation of closing all modals when opening the quick menu could lead to data loss if users have unsaved changes.
Consider:
- Adding confirmation dialogs for unsaved changes
- Implementing a modal stack
- Adding proper focus management when switching between modals
There was a problem hiding this comment.
Other things to consider:
Would be nice to have a Ctrl + ` by the create button to help new users spot the shortcut, maybe hide it on mobile but that isnt very necesarry.
Having hotkeys to directly open the create items, create labels and create create locations could be nice though not a must have.
Having hotkeys be customisable might be nice but not really a must have.
Translation keys need to added.
| </button> | ||
| </li> | ||
| </ul> | ||
| <span class="text-base-300">Use number keys to quick select.</span> |
There was a problem hiding this comment.
This text is currently kinda unreadable, I think it would be better if it was just the normal text colour?
It also gets blocked off by the dropdown so moving it to be above would make it more readable.
| class="input input-bordered w-full" | ||
| @input="inputValue = $event.target.value" | ||
| ></ComboboxInput> | ||
| <ComboboxOptions class="card dropdown-content absolute w-full rounded-lg border border-base-300 bg-base-100"> |
There was a problem hiding this comment.
Might be worth adding mt-2 as currently it looks a bit off and that matches what we do in Autocomplete2.tsx
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Nitpick comments (5)
frontend/components/global/QuickMenu/Modal.vue (2)
35-37: Consider increasing the focus timeout.The 50ms timeout for focusing the input might be too short on slower devices. Consider increasing it to at least 100ms for better reliability.
- }, 50).start; + }, 100).start;
50-57: Protect against potential race conditions.The watch on selectedAction could trigger multiple times if the user rapidly selects different actions. Consider adding a guard:
watch(selectedAction, action => { - if (action) invokeAction(action); + if (action && modal.value) invokeAction(action); });frontend/components/global/QuickMenu/Input.vue (1)
8-10: Consider adjusting max-height for better UX.The current max-height of 48 units might cut off items in the dropdown. Consider:
- Using a viewport-relative height (e.g., max-h-[80vh])
- Adding a scrollbar indicator
- class="card dropdown-content absolute max-h-48 w-full overflow-y-scroll rounded-lg border border-base-300 bg-base-100" + class="card dropdown-content absolute max-h-[80vh] w-full overflow-y-auto rounded-lg border border-base-300 bg-base-100 scrollbar-thin scrollbar-track-base-200 scrollbar-thumb-base-300"frontend/layouts/default.vue (2)
259-266: Consider centralizing modal state management.The current implementation manually closes each modal. Consider using a centralized approach:
+ const closeAllModals = () => { + Object.keys(modals).forEach(key => { + if (key !== 'quickMenu') modals[key] = false; + }); + }; whenever(quickMenuShortcut, () => { modals.quickMenu = true; - modals.item = false; - modals.location = false; - modals.label = false; - modals.import = false; + closeAllModals(); });
268-302: Consider adding keyboard shortcuts for navigation actions.The navigation actions currently have empty shortcuts. Consider adding number shortcuts (4-9) for quick navigation:
nav.map((v, index) => { return { text: computed(() => `${t("global.navigate")}: ${v.name.value}`), action: () => { navigateTo(v.to); }, - shortcut: "", + shortcut: `${index + 4}`, }; })
🛑 Comments failed to post (1)
frontend/components/global/QuickMenu/Input.vue (1)
89-96:
⚠️ Potential issueEnhance input handling security.
The current implementation directly compares user input with shortcuts without sanitization. Consider:
- Sanitizing input to prevent XSS
- Making shortcut comparison case-insensitive
- Adding input debouncing
+ const sanitizeInput = (input: string) => input.replace(/[<>]/g, ''); + const debouncedInput = useDebounce(inputValue, 150); watch(inputValue, (val, oldVal) => { if (!oldVal) { + const sanitizedVal = sanitizeInput(val); - const action = props.actions?.find(v => v.shortcut === val); + const action = props.actions?.find(v => v.shortcut?.toLowerCase() === sanitizedVal.toLowerCase()); if (action) { emit("quickSelect", action); } } });Committable suggestion skipped: line range outside the PR's diff.
What type of PR is this?
What this PR does / why we need it:
This PR adds a command palette like modal menu that can be brought up with Ctrl+`, and allows for faster access to the other create modals for keyboard only use. Other commands can be added later but I can't think of anything, so for now its just a keyboard shortcut to those menus. The shortcut should also probably be displayed somewhere.
I also changed
BaseModal.vueto allow clicking off of the modal to close it and made the close button optional, since I think these additions benefit this feature.Which issue(s) this PR fixes:
#16
Summary by CodeRabbit
Release Notes
New Features
Improvements
UI Changes