Creation modal quality of life changes #467
Conversation
WalkthroughThis pull request introduces several enhancements to frontend components, focusing on modal interactions, form management, and keyboard shortcut handling. The changes primarily involve updating component props, modifying CSS classes, and refining modal focus and interaction behaviors. Key modifications include adding a Changes
Assessment against linked issues
Possibly related PRs
Suggested Reviewers
Security Recommendations
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🔇 Additional comments (1)
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/Base/Modal.vue (1)
58-62: Add rate limiting to click outside handlerThe click outside handler could be triggered rapidly, potentially causing race conditions.
Consider debouncing the handler:
if (props.clickOutsideToClose) { + const debouncedClose = useDebounceFn(() => { + close(); + }, 300); onClickOutside(modalBox, () => { - close(); + debouncedClose(); }); }frontend/layouts/default.vue (1)
203-208: Sanitize shortcut keys before useThe current implementation directly uses shortcut keys without sanitization. While the risk is low since shortcuts are defined internally, it's good practice to sanitize all user inputs.
Apply this diff to add sanitization:
if (option?.shortcut) { - const shortcutKeycode = option.shortcut.replace(/([0-9])/, "digit$&"); + // Sanitize shortcut key to only allow specific characters + const sanitizedShortcut = option.shortcut.replace(/[^0-9!@#]/, ""); + if (sanitizedShortcut !== option.shortcut) { + console.error('Invalid shortcut key'); + return; + } + const shortcutKeycode = sanitizedShortcut.replace(/([0-9])/, "digit$&"); whenever(keys[shortcutKeycode], () => { if (activeElement.value?.tagName !== "INPUT") { option.action(); } }); }
🛑 Comments failed to post (3)
frontend/components/Base/Modal.vue (1)
44-47: 💡 Codebase verification
⚠️ Potential issue🔒 Add event listener cleanup to prevent security vulnerabilities
The click outside handler needs proper cleanup to prevent:
- Memory leaks affecting application performance
- Potential information disclosure if modal content remains accessible
- Resource exhaustion from accumulated event listeners
+ const stopClickOutside = ref(); if (props.clickOutsideToClose) { - onClickOutside(modalBox, () => { + stopClickOutside.value = onClickOutside(modalBox, () => { close(); }); } + onUnmounted(() => { + if (stopClickOutside.value) { + stopClickOutside.value(); + } + });🔗 Analysis chain
Ensure proper cleanup of click outside handler
The click outside handler is added conditionally but there's no cleanup when the component is unmounted.
Add cleanup to prevent memory leaks and potential security issues:
+ const stopClickOutside = ref(); if (props.clickOutsideToClose) { - onClickOutside(modalBox, () => { + stopClickOutside.value = onClickOutside(modalBox, () => { close(); }); } + onUnmounted(() => { + if (stopClickOutside.value) { + stopClickOutside.value(); + } + });🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find onClickOutside implementation and usage ast-grep --pattern 'const onClickOutside = $_' # Search for similar modal cleanup patterns rg "onClickOutside.*modal" -A 5 -B 5 # Find unmounted cleanup patterns ast-grep --pattern 'onUnmounted(() => { $$$ })'Length of output: 4153
Script:
#!/bin/bash # Search for onClickOutside implementation in composables fd "use.*\.ts$" --exec cat {} \; # Look for vueuse imports as it might be from there rg "@vueuse/.*onClickOutside" -A 2 -B 2Length of output: 26563
frontend/components/Location/CreateModal.vue (1)
98-103:
⚠️ Potential issueValidate route parameters to prevent path traversal
The location ID from route parameters needs validation to prevent potential security issues.
Add validation:
const locationId = computed(() => { if (route.fullPath.includes("/location/")) { - return route.params.id; + const id = route.params.id; + // Ensure id is a valid UUID or your expected format + const validIdPattern = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; + return validIdPattern.test(id) ? id : null; } return null; });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const locationId = computed(() => { if (route.fullPath.includes("/location/")) { const id = route.params.id; // Ensure id is a valid UUID or your expected format const validIdPattern = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; return validIdPattern.test(id) ? id : null; } return null; });frontend/components/Item/CreateModal.vue (1)
144-163:
⚠️ Potential issueValidate route parameters before use
The current implementation directly uses route parameters without validation. This could potentially lead to XSS attacks if the route parameters contain malicious content.
Apply this diff to add validation:
if (locationId.value) { + // Validate locationId is a valid UUID + if (!/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(locationId.value)) { + console.error('Invalid location ID format'); + return; + } const found = locations.value.find(l => l.id === locationId.value); if (found) { form.location = found; } } if (labelId.value) { + // Validate labelId is a valid UUID + if (!/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(labelId.value)) { + console.error('Invalid label ID format'); + return; + } form.labels = labels.value.filter(l => l.id === labelId.value); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.watch( () => modal.value, (open) => { if (open) { useTimeoutFn(() => { focused.value = true }, 50); if (locationId.value) { // Validate locationId is a valid UUID if (!/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(locationId.value)) { console.error('Invalid location ID format'); return; } const found = locations.value.find(l => l.id === locationId.value); if (found) { form.location = found; } } if (labelId.value) { // Validate labelId is a valid UUID if (!/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(labelId.value)) { console.error('Invalid label ID format'); return; } form.labels = labels.value.filter(l => l.id === labelId.value); } } else { focused.value = false; } }
|
Linting needs to be run, the labels dropdown seems to get hidden on small screens |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/components/global/QuickMenu/Input.vue (1)
Line range hint
1-116: Consider additional security measures for the quick menu component.While reviewing the component, I noticed some potential security considerations:
- Input validation: Consider sanitizing user input before using it in the filtering logic to prevent potential XSS attacks through action text.
- Action execution safeguards: The quick select feature (single-character shortcuts) could benefit from an additional confirmation step for destructive actions.
- Rate limiting: Consider adding debounce to prevent rapid-fire action execution.
Here's a suggested implementation for input sanitization:
const filteredActions = computed(() => { - const searchTerm = inputValue.value.toLowerCase(); + const searchTerm = sanitizeInput(inputValue.value.toLowerCase()); return (props.actions || []).filter(action => { return action.text.toLowerCase().includes(searchTerm) || action.shortcut?.includes(searchTerm); }); }); + function sanitizeInput(input: string): string { + // Remove potentially dangerous characters while preserving legitimate search terms + return input.replace(/[<>'"]/g, ''); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/components/Item/CreateModal.vue(2 hunks)frontend/components/global/QuickMenu/Input.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/components/Item/CreateModal.vue
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: Frontend and End-to-End Tests / Integration Tests
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (1)
frontend/components/global/QuickMenu/Input.vue (1)
84-85: LGTM! The focus handling change improves security.The simplified focus handling logic that only clears input on blur is better than automatically revealing actions, as it reduces the risk of unintended action execution.
Blackshadow8910
force-pushed
the
creation-autofill
branch
from
026fbae to
224b669
Compare
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: 2
🔭 Outside diff range comments (1)
frontend/components/Location/CreateModal.vue (1)
Line range hint
108-143: Enhance security and error handling in create function.The current implementation has several security and reliability concerns:
- No rate limiting on form submission
- Basic error handling
- Unconstrained navigation
Consider implementing:
- Rate limiting for form submissions
- More detailed error handling
- Navigation validation
+ const SUBMIT_COOLDOWN = 1000; // 1 second + let lastSubmitTime = 0; + async function create(close = true) { if (loading.value) { toast.error("Already creating a location"); return; } + + // Rate limiting + const now = Date.now(); + if (now - lastSubmitTime < SUBMIT_COOLDOWN) { + toast.error("Please wait before submitting again"); + return; + } + lastSubmitTime = now; + loading.value = true; if (shift.value) { close = false; } const { data, error } = await api.locations.create({ name: form.name, description: form.description, parentId: form.parent ? form.parent.id : null, }); if (error) { loading.value = false; - toast.error("Couldn't create location"); + // More detailed error handling + toast.error(`Failed to create location: ${error.message || 'Unknown error'}`); + return; } if (data) { + // Validate data.id before navigation + if (!data.id || typeof data.id !== 'string') { + toast.error("Invalid response from server"); + loading.value = false; + return; + } toast.success("Location created"); } reset(); if (close) { modal.value = false; - navigateTo(`/location/${data.id}`); + // Safe navigation with validated id + if (data?.id) { + navigateTo(`/location/${encodeURIComponent(data.id)}`); + } } }
🛑 Comments failed to post (2)
frontend/components/Location/CreateModal.vue (2)
100-105:
⚠️ Potential issueEnhance route parameter validation for security.
The current implementation directly uses route parameters without proper validation, which could lead to security vulnerabilities.
Consider this safer implementation:
const locationId = computed(() => { if (route.fullPath.includes("/location/")) { - return route.params.id; + const id = route.params.id; + // Validate id format (assuming UUID format) + return typeof id === 'string' && /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(id) + ? id + : null; } return null; });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const locationId = computed(() => { if (route.fullPath.includes("/location/")) { const id = route.params.id; // Validate id format (assuming UUID format) return typeof id === 'string' && /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(id) ? id : null; } return null; });
62-79: 🛠️ Refactor suggestion
Improve timing and cleanup in watch handler.
The current implementation has several potential issues:
- The 50ms timeout is arbitrary and might not be reliable across different devices
- Missing cleanup for the timeout on component unmount
- Potential race condition between timeout and location finding
Consider this improved implementation:
+ const timeoutRef = ref<number | null>(null); + watch( () => modal.value, open => { if (open) { - useTimeoutFn(() => { - focused.value = true; - }, 50); + // Clear any existing timeout + if (timeoutRef.value) clearTimeout(timeoutRef.value); + // Set new timeout and store reference + timeoutRef.value = setTimeout(() => { + focused.value = true; + timeoutRef.value = null; + }, 50); if (locationId.value) { const found = locations.value.find(l => l.id === locationId.value); if (found) { form.parent = found; } } } else { focused.value = false; + // Clear timeout on modal close + if (timeoutRef.value) { + clearTimeout(timeoutRef.value); + timeoutRef.value = null; + } } } ); + + // Cleanup on component unmount + onBeforeUnmount(() => { + if (timeoutRef.value) { + clearTimeout(timeoutRef.value); + timeoutRef.value = null; + } + });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const timeoutRef = ref<number | null>(null); watch( () => modal.value, open => { if (open) { // Clear any existing timeout if (timeoutRef.value) clearTimeout(timeoutRef.value); // Set new timeout and store reference timeoutRef.value = setTimeout(() => { focused.value = true; timeoutRef.value = null; }, 50); if (locationId.value) { const found = locations.value.find(l => l.id === locationId.value); if (found) { form.parent = found; } } } else { focused.value = false; // Clear timeout on modal close if (timeoutRef.value) { clearTimeout(timeoutRef.value); timeoutRef.value = null; } } } ); // Cleanup on component unmount onBeforeUnmount(() => { if (timeoutRef.value) { clearTimeout(timeoutRef.value); timeoutRef.value = null; } });
|
Sorry for the broken Docker builds, we're working on sorting that out. However, the linting issues should be correct. |
What type of PR is this?
What this PR does / why we need it:
This PR does the following:
I think this layout for the modals is a little more consistent and efficient, since having the labels on top makes it possible to tab to the create button. Any other opinions on this would be appreciated though.
Which issue(s) this PR fixes:
Fixes #411
Summary by CodeRabbit
Release Notes
New Features
Improvements
Style Updates
User Experience