Switch layouts to use shadcn #507
Conversation
WalkthroughThis PR replaces the legacy notification system based on Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant QMM as QuickMenuModal
participant DH as useDialogHotkey
participant DP as DialogProvider
U->>QMM: Presses Ctrl+Backquote (keyboard shortcut)
QMM->>DH: Triggers keydown event
DH->>DP: Calls openDialog("quick-menu")
DP-->>QMM: Updates dialog state (open)
U->>QMM: Selects a quick action
QMM->>QMM: Executes associated action
U->>DP: Initiates closeDialog (dialog close)
sequenceDiagram
participant C as Component
participant T as vue-sonner (toast)
C->>T: Calls toast.error()/toast.success()
T-->>C: Displays notification on UI
Possibly related PRs
Suggested reviewers
Security Recommendations
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: 2
🔭 Outside diff range comments (16)
frontend/pages/assets/[id].vue (1)
15-19: Sanitize error messages to prevent information leakage.Avoid exposing raw API error details in toast messages. Consider using generic error messages for users.
- toast.error("Failed to load asset"); + toast.error("Unable to process request. Please try again later.");frontend/components/Label/CreateModal.vue (2)
94-99: Add input validation and sanitize error messages.The label creation endpoint needs proper input validation and error message sanitization.
- const { error, data } = await api.labels.create(form); + // Validate input + if (!form.name.trim() || form.name.length > 255) { + toast.error("Invalid label name"); + loading.value = false; + return; + } + + if (form.description && form.description.length > 255) { + toast.error("Description too long"); + loading.value = false; + return; + } + + const { error, data } = await api.labels.create(form); if (error) { - toast.error("Couldn't create label"); + toast.error("Unable to create label. Please try again later."); loading.value = false; return; }
104-107: Validate navigation target to prevent open redirect.Ensure the label ID is validated before navigation to prevent potential redirect attacks.
if (close) { modal.value = false; - navigateTo(`/label/${data.id}`); + if (typeof data.id === 'string' && /^[a-zA-Z0-9-]+$/.test(data.id)) { + navigateTo(`/label/${data.id}`); + } else { + toast.error("Invalid response. Please refresh the page."); + } }frontend/components/App/ImportDialog.vue (2)
30-30: Enhance file upload security measures.The current file upload validation only checks file extensions. Consider implementing additional security measures:
-<input ref="importRef" type="file" class="hidden" accept=".csv,.tsv" @change="setFile" /> +<input + ref="importRef" + type="file" + class="hidden" + accept=".csv,.tsv" + @change="setFile" + maxsize="5242880" <!-- 5MB limit --> +/>
75-82: Add file validation checks before upload.The setFile function should validate the file content and size before allowing upload.
function setFile(e: Event) { const result = e.target as HTMLInputElement; if (!result.files || result.files.length === 0) { return; } + const file = result.files[0]; + + // Check file size (5MB limit) + if (file.size > 5242880) { + toast.error("File size exceeds 5MB limit"); + return; + } + + // Validate file type + if (!['text/csv', 'text/tab-separated-values'].includes(file.type)) { + toast.error("Invalid file type"); + return; + } importCsv.value = result.files[0]; }frontend/components/Form/Multiselect.vue (1)
106-119: Sanitize user input before API calls.The createAndAdd function should sanitize the input name before sending it to the API.
async function createAndAdd(name: string) { + // Sanitize input + const sanitizedName = name.trim().replace(/[<>]/g, ''); + + // Validate input length + if (sanitizedName.length < 1 || sanitizedName.length > 50) { + toast.error('Label name must be between 1 and 50 characters'); + return; + } + const { error, data } = await api.labels.create({ - name, + name: sanitizedName, color: "", // Future! description: "", });frontend/components/Location/CreateModal.vue (1)
107-138: Implement rate limiting and input sanitization.The create function needs protection against rapid submissions and input sanitization.
+const COOLDOWN_PERIOD = 2000; // 2 seconds +let lastSubmissionTime = 0; + async function create(close = true) { if (loading.value) { toast.error("Already creating a location"); return; } + + // Rate limiting + const now = Date.now(); + if (now - lastSubmissionTime < COOLDOWN_PERIOD) { + toast.error("Please wait before submitting again"); + return; + } + lastSubmissionTime = now; + + // Sanitize inputs + const sanitizedName = form.name.trim().replace(/[<>]/g, ''); + const sanitizedDesc = form.description.trim().replace(/[<>]/g, ''); + loading.value = true; if (shift.value) { close = false; } const { data, error } = await api.locations.create({ - name: form.name, - description: form.description, + name: sanitizedName, + description: sanitizedDesc, parentId: form.parent ? form.parent.id : null, });frontend/components/Maintenance/EditModal.vue (1)
56-75: Enhance input validation for maintenance entries.The createEntry function needs proper validation for cost and date inputs.
async function createEntry() { if (!entry.itemId) { return; } + + // Validate cost + const costValue = parseFloat(entry.cost); + if (isNaN(costValue) || costValue < 0) { + toast.error("Invalid cost value"); + return; + } + + // Validate dates + if (entry.scheduledDate && entry.completedDate) { + if (new Date(entry.scheduledDate) < new Date(entry.completedDate)) { + toast.error("Scheduled date cannot be earlier than completed date"); + return; + } + } + + // Sanitize inputs + const sanitizedName = entry.name.trim().replace(/[<>]/g, ''); + const sanitizedDesc = entry.description.trim().replace(/[<>]/g, ''); + const { error } = await api.items.maintenance.create(entry.itemId, { - name: entry.name, + name: sanitizedName, completedDate: entry.completedDate ?? "", scheduledDate: entry.scheduledDate ?? "", - description: entry.description, - cost: parseFloat(entry.cost) ? entry.cost : "0", + description: sanitizedDesc, + cost: costValue.toString(), });frontend/pages/index.vue (2)
81-87: Enhance email validation security.The current email validation using a simple regex pattern might allow some edge cases. Consider using a more robust email validation library.
-const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; +import isEmail from 'validator/lib/isEmail'; + +if (!isEmail(email.value)) {
117-132: Enhance login security measures.The login function should implement additional security measures.
- Add rate limiting for failed login attempts
- Implement CAPTCHA for repeated failed attempts
- Consider adding 2FA support
- Add proper CSRF protection
frontend/layouts/default.vue (1)
368-371: Secure token handling in URL.The invitation token is exposed in the URL, which could be logged in browser history or server logs.
Consider:
- Using POST requests with tokens in the body
- Implementing short-lived tokens
- Adding token revocation capabilities
frontend/pages/reports/label-generator.vue (1)
184-195: Enhance QR code URL security.The QR code URL generation needs additional security measures.
- Validate and sanitize input parameters
- Consider using signed URLs to prevent tampering
- Add rate limiting for QR code generation
- Implement URL expiration for generated QR codes
frontend/pages/profile.vue (3)
172-191: Enhance password change security.The password change functionality needs additional security measures.
- Add password complexity validation
- Implement password history check
- Add rate limiting for password change attempts
- Consider adding re-authentication before password change
118-136: Strengthen account deletion security.The account deletion process should have additional safeguards.
- Require password confirmation
- Implement cool-down period
- Add email confirmation
- Consider two-step verification for deletion
290-303: Secure notifier testing.The notifier testing endpoint needs protection against abuse.
- Add rate limiting for test attempts
- Validate and sanitize URL input
- Implement timeout for test requests
- Consider adding CORS protection
frontend/pages/item/[id]/index/edit.vue (1)
434-448: Add confirmation and audit logging for item deletion.The item deletion operation should include additional security measures and audit logging.
async function deleteItem() { + // Add audit logging + const auditLog = { + action: 'delete_item', + itemId: itemId.value, + timestamp: new Date().toISOString(), + userId: api.getCurrentUser().id + }; + const confirmed = await confirm.open("Are you sure you want to delete this item?"); if (!confirmed.data) { return; } + // Add additional confirmation for sensitive items + if (item.value?.isProtected) { + const adminConfirmed = await confirm.open({ + title: "Admin Confirmation Required", + message: "This is a protected item. Please enter your admin password to confirm deletion.", + input: "password" + }); + if (!adminConfirmed.data) { + return; + } + } const { error } = await api.items.delete(itemId.value); if (error) { toast.error("Failed to delete item"); + auditLog.status = 'failed'; + auditLog.error = error.message; + await api.audit.log(auditLog); return; } toast.success("Item deleted"); + auditLog.status = 'success'; + await api.audit.log(auditLog); navigateTo("/home"); }
🧹 Nitpick comments (12)
frontend/components/ui/sonner/Sonner.vue (1)
8-8: Track the TODO comment for daisyui removal.The comment indicates a pending cleanup task. Consider creating a tracking issue.
Would you like me to create an issue to track the daisyui removal task?
frontend/pages/label/[id].vue (1)
28-45: Consider adding rate limiting for delete operations.The delete operation could be vulnerable to rapid repeated requests. Consider implementing rate limiting or adding a cooldown period between delete operations.
frontend/pages/location/[id].vue (1)
35-51: Add validation for cascading delete operations.The location delete operation affects child items. Consider:
- Adding a count of affected items in the confirmation message
- Implementing a soft delete option
- Validating user permissions for nested items
frontend/components/Item/CreateModal.vue (2)
203-213: Enhance file upload security measures.The photo upload functionality should include:
- Server-side file type validation
- File size limits
- Malware scanning for uploaded files
- Secure file storage configuration
25-31: Strengthen file input restrictions.Consider adding:
- Maximum file size attribute
- More specific MIME type validation
<input id="photo" class="hidden" type="file" + maxlength="10485760" accept="image/png,image/jpeg,image/gif,image/avif,image/webp" + data-max-size="10485760" @change="previewImage" />frontend/pages/tools.vue (2)
128-145: Add rate limiting and logging for administrative operations.The ensureAssetIDs operation should:
- Include rate limiting for batch operations
- Add audit logging for tracking changes
- Implement progressive processing for large datasets
185-202: Enhance security for photo management operations.Consider adding:
- Validation of photo file integrity
- Checksum verification
- Audit trail for photo changes
frontend/pages/index.vue (1)
89-94: Add rate limiting for registration attempts.The registration endpoint should be protected against brute force attacks.
Consider implementing rate limiting on the API side and showing appropriate error messages to users when limits are exceeded.
frontend/pages/items.vue (2)
2-2: Consider adding error tracking metadata.While the toast import is correctly implemented, consider enhancing error tracking by including metadata with the toast notifications for better debugging and monitoring.
import { toast } from "vue-sonner"; +import { useErrorTracking } from "~/composables/use-error-tracking";
283-287: Enhance error handling with specific error messages.The error handling could be more informative to help users understand what went wrong.
if (error) { resetItems(); - toast.error("Failed to search items"); + toast.error(`Failed to search items: ${error.message || 'Unknown error'}`); return; }frontend/pages/item/[id]/index.vue (2)
2-2: Consider implementing a centralized error handler.While the toast import is correct, consider implementing a centralized error handler to standardize error messages and tracking.
import { toast } from "vue-sonner"; +import { useErrorHandler } from "~/composables/use-error-handler"; + +const errorHandler = useErrorHandler();
28-34: Enhance error handling with retry mechanism.Consider adding a retry mechanism for failed item fetches to improve user experience.
if (error) { - toast.error("Failed to load item"); + toast.error(`Failed to load item: ${error.message}`); + // Add retry mechanism + const retryCount = 3; + for (let i = 0; i < retryCount; i++) { + const { data: retryData, error: retryError } = await api.items.get(itemId.value); + if (!retryError) { + return retryData; + } + } navigateTo("/home"); return; }
🛑 Comments failed to post (2)
frontend/components/ui/shortcut/Shortcut.vue (1)
20-26: 💡 Codebase verification
Sanitize user-supplied class names for security
The
classNameprop infrontend/components/ui/shortcut/Shortcut.vueis directly bound to the element's classes without any sanitization or validation. Even though Vue provides XSS protection for markup, unsanitized class names can still lead to potential CSS injection or styling manipulation issues. Consider applying a whitelist filter or regex validation to ensure that only expected, safe class names are allowed.
- File:
frontend/components/ui/shortcut/Shortcut.vue- Issue: The
classNameprop is used without sanitization, which may allow unintended or malicious CSS classes.Always ensure user input is properly validated to reduce the risk of security bypasses.
🔗 Analysis chain
Consider sanitizing user-provided className prop.
While Vue provides XSS protection by default, it's good practice to validate/sanitize user-provided class names.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for className prop usage across the codebase rg -A 2 "className.*Shortcut"Length of output: 31
Script:
#!/bin/bash # Print the contents of Shortcut.vue to inspect the definition and usage of the className prop. cat frontend/components/ui/shortcut/Shortcut.vueLength of output: 739
frontend/pages/assets/[id].vue (1)
11-11:
⚠️ Potential issueAdd input validation for asset ID.
The asset ID from route params should be validated before use to prevent potential injection attacks.
- const assetId = computed<string>(() => route.params.id as string); + const assetId = computed<string>(() => { + const id = route.params.id as string; + if (!/^[a-zA-Z0-9-]+$/.test(id)) { + toast.error("Invalid asset ID format"); + navigateTo("/home"); + return ""; + } + return id; + });📝 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 assetId = computed<string>(() => { const id = route.params.id as string; if (!/^[a-zA-Z0-9-]+$/.test(id)) { toast.error("Invalid asset ID format"); navigateTo("/home"); return ""; } return id; });
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: 4
🔭 Outside diff range comments (1)
frontend/layouts/default.vue (1)
106-111: Security: Consider limiting version information exposure.Exposing detailed version and build information in the UI could help attackers identify vulnerable versions. Consider showing this information only to administrators.
- <a - href="https://github.com/sysadminsmedia/homebox/releases/tag/{{ status.build.version }}" - target="_blank" - > - {{ $t("global.version", { version: status.build.version }) }} ~ - {{ $t("global.build", { build: status.build.commit }) }}</a + <a v-if="isAdmin" + href="https://github.com/sysadminsmedia/homebox/releases/tag/{{ status.build.version }}" + target="_blank" + > + {{ $t("global.version", { version: status.build.version }) }}</a
🧹 Nitpick comments (9)
frontend/components/ui/dialog/DialogTrigger.vue (1)
1-11: LGTM! Consider enhancing accessibility.The implementation is clean and type-safe. Consider adding ARIA attributes for better screen reader support.
<DialogTrigger v-bind="props"> + <div role="button" aria-haspopup="dialog"> <slot /> + </div> </DialogTrigger>frontend/components/ui/dialog/Dialog.vue (1)
1-14: Consider adding security features to the dialog system.While the implementation is correct, consider enhancing security by:
- Adding focus trapping to prevent keyboard navigation outside the dialog
- Implementing click-outside protection for sensitive dialogs
- Adding escape key handling with confirmation for sensitive operations
<script setup lang="ts"> import { DialogRoot, type DialogRootEmits, type DialogRootProps, useForwardPropsEmits } from 'radix-vue' +import { useFocusTrap } from '@vueuse/integrations/useFocusTrap' +import { ref } from 'vue' const props = defineProps<DialogRootProps>() const emits = defineEmits<DialogRootEmits>() const forwarded = useForwardPropsEmits(props, emits) +const dialogRef = ref(null) +useFocusTrap(dialogRef) </script> <template> - <DialogRoot v-bind="forwarded"> + <DialogRoot v-bind="forwarded" ref="dialogRef"> <slot /> </DialogRoot> </template>frontend/components/ui/command/CommandGroup.vue (1)
22-22: Consider simplifying complex CSS selectors.The inline CSS selectors are complex and could be simplified for better maintainability. Consider moving these to a separate CSS module.
- :class="cn('overflow-hidden p-1 text-foreground [&_[cmdk-group-heading]]:px-2 [&_[cmdk-group-heading]]:py-1.5 [&_[cmdk-group-heading]]:text-xs [&_[cmdk-group-heading]]:font-medium [&_[cmdk-group-heading]]:text-muted-foreground', props.class)" + :class="cn('command-group', props.class)"Then in a separate CSS module:
.command-group { @apply overflow-hidden p-1 text-foreground; } .command-group [cmdk-group-heading] { @apply px-2 py-1.5 text-xs font-medium text-muted-foreground; }frontend/components/ui/dialog/DialogScrollContent.vue (1)
50-55: Enhance close button accessibility.The close button implementation is good with the sr-only text, but could be improved further.
Consider adding these accessibility enhancements:
<DialogClose - class="absolute top-3 right-3 p-0.5 transition-colors rounded-md hover:bg-secondary" + class="absolute top-3 right-3 p-0.5 transition-colors rounded-md hover:bg-secondary focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-primary" + aria-label="Close dialog" > <X class="w-4 h-4" /> <span class="sr-only">Close</span> </DialogClose>frontend/components/global/QuickMenu/Modal.vue (2)
59-59: Localize user-facing text.The text "No results found." is currently hard-coded in English. For consistency and inclusivity, consider translating it using the
tfunction fromvue-i18n—similar to how other text is handled.- <CommandEmpty>No results found.</CommandEmpty> + <CommandEmpty>{{ t('global.noResultsFound') }}</CommandEmpty>
44-57: Validate user-supplied actions for security.The logic captures keyboard events and executes actions from
props.actions. If these actions involve routing or manipulating data based on user input, confirm they're vetted to prevent malicious code or unintended behavior.Would you like help adding a security layer or client-side validation to ensure safe handling of user-supplied commands?
frontend/components/ui/command/Command.vue (1)
8-9: Confirm defaultopenstate.Defaulting
opentotruemay unintentionally expose the command palette as soon as the component mounts. Ensure this is the desired behavior. Otherwise, consider defaulting it tofalse.withDefaults(defineProps<ComboboxRootProps & { class?: HTMLAttributes['class'] }>(), { - open: true, + open: false, modelValue: '', })frontend/components/ui/command/CommandDialog.vue (1)
13-21: Consider adding ARIA attributes for better accessibility.While the Dialog component likely handles basic accessibility, consider adding custom ARIA labels or descriptions for better screen reader support.
- <Dialog v-bind="forwarded"> + <Dialog v-bind="forwarded" aria-label="Command menu" aria-description="Use this menu to access commands and navigate the application">frontend/components/ui/command/CommandInput.vue (1)
29-29: Review auto-focus behavior.The
auto-focusattribute might affect user experience, especially for screen reader users. Consider making this configurable.- auto-focus + :auto-focus="preferences?.autoFocusEnabled ?? true"
🛑 Comments failed to post (4)
frontend/components/ui/dialog/DialogContent.vue (2)
42-47: 🛠️ Refactor suggestion
Enhance close button security and accessibility.
The close button implementation needs additional security and accessibility improvements.
Apply these security and accessibility enhancements:
<DialogClose - class="absolute right-4 top-4 rounded-sm opacity-70 ring-offset-background transition-opacity hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:pointer-events-none data-[state=open]:bg-accent data-[state=open]:text-muted-foreground" + class="absolute right-4 top-4 rounded-sm opacity-70 ring-offset-background transition-opacity hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:pointer-events-none data-[state=open]:bg-accent data-[state=open]:text-muted-foreground" + aria-label="Close dialog" + @keydown.esc.prevent="$emit('close')" + role="button" + tabindex="0" > <X class="w-4 h-4" /> <span class="sr-only">Close</span> </DialogClose>📝 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.<DialogClose class="absolute right-4 top-4 rounded-sm opacity-70 ring-offset-background transition-opacity hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:pointer-events-none data-[state=open]:bg-accent data-[state=open]:text-muted-foreground" aria-label="Close dialog" @keydown.esc.prevent="$emit('close')" role="button" tabindex="0" > <X class="w-4 h-4" /> <span class="sr-only">Close</span> </DialogClose>
29-31: 🛠️ Refactor suggestion
Add security attributes to overlay.
The overlay should prevent any potential clickjacking attempts.
Add security attributes:
<DialogOverlay - class="fixed inset-0 z-50 bg-black/80 data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0" + class="fixed inset-0 z-50 bg-black/80 data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0" + style="pointer-events: auto;" + @click.self="$emit('close')" />📝 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.<DialogOverlay class="fixed inset-0 z-50 bg-black/80 data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0" style="pointer-events: auto;" @click.self="$emit('close')" />frontend/components/global/QuickMenu/Modal.vue (1)
52-52:
⚠️ Potential issueFix reactive assignment for
open.You're assigning
open = false;even thoughopenis declared as a Vue ref. This won't update the reactive state. Instead, useopen.value = false;to correctly toggle the ref's value.Suggested fix:
- open = false; + open.value = false;Also applies to: 67-67, 84-84
frontend/components/ui/command/CommandInput.vue (1)
28-31: 💡 Codebase verification
Action: Add explicit input sanitization to CommandInput.vue
Our investigation did not uncover any sanitization or validation logic applied to the input within CommandInput.vue. While other parts of the frontend (e.g., Markdown rendering) make use of DOMPurify, this component relies on a ComboboxInput that doesn’t appear to implement its own sanitization. It is recommended to explicitly sanitize or validate the user input to mitigate potential XSS risks.
🔗 Analysis chain
Consider input sanitization and validation.
The ComboboxInput component should sanitize user input to prevent XSS attacks. Consider adding input validation and sanitization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing input sanitization in the codebase rg -A 5 "sanitize|escape|validate" --type vueLength of output: 75
Script:
#!/bin/bash # Add a vue file type and search for potential input sanitization, escaping, or validation references in Vue files. rg --type-add='vue:*.vue' -A 5 "sanitize|escape|validate"Length of output: 58756
…sage in default layout
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
🧹 Nitpick comments (1)
frontend/locales/en.json (1)
97-98: New 'no_results' Localization Entry for Quick MenuThe addition of
"no_results": "No results found."in the"quick_menu"section enriches the user feedback when no quick menu items are available. This is a useful enhancement for improving UX consistency. Please verify that its phrasing aligns with similar keys (e.g.,"no_results": "No Items Found"in the items section) and ensure that when rendered, the string is properly escaped to prevent any potential XSS risks when applied in an HTML context.
🛑 Comments failed to post (2)
frontend/components/global/QuickMenu/Modal.vue (2)
29-41: 🛠️ Refactor suggestion
Add rate limiting for keyboard shortcut actions.
The keyboard shortcut handling could be vulnerable to rapid-fire execution. Consider adding debouncing and rate limiting to prevent potential DoS-like behavior.
+ import { useThrottleFn } from '@vueuse/core'; + - function handleOpenChange() { + const handleOpenChange = useThrottleFn(() => { open.value = !open.value; - } + }, 300);📝 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.import { useThrottleFn } from '@vueuse/core'; const open = ref(false); const { t } = useI18n(); const keys = useMagicKeys(); const CtrlBackquote = keys.control_Backquote; const handleOpenChange = useThrottleFn(() => { open.value = !open.value; }, 300); watch(CtrlBackquote, v => { if (v) handleOpenChange(); });
44-94: 🛠️ Refactor suggestion
Enhance security for text display and action execution.
The template directly displays user-provided text and executes actions without validation. Consider these security enhancements:
- Sanitize text content before display
- Validate actions before execution
- Add error boundaries for action execution
- {{ create.text }} + {{ sanitizeText(create.text) }}- action.action(); + try { + if (validateAction(action)) { + action.action(); + } + } catch (error) { + console.error('Action execution failed:', error); + }Committable suggestion skipped: line range outside the PR's diff.
|
To Fix:
|
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: 4
🔭 Outside diff range comments (1)
frontend/components/Form/TextField.vue (1)
8-14: Server-side validation for input length is missingThe current API contract file only contains JSDoc annotations (e.g.,
@maxLengthand@minLength) for documenting the expected string lengths. These annotations are not active runtime validations, leaving a gap in the server-side enforcement of input lengths.
- The annotations in
frontend/lib/api/types/data-contracts.tsprovide documentation but do not perform any actual validation.- No explicit server-side validation logic was found that checks the input length before processing.
Security Recommendation: Implement robust runtime validations on the server to ensure that input lengths are strictly enforced. This prevents potential malicious input that could bypass client-side checks.
🔗 Analysis chain
Add server-side validation for input length.
While client-side length validation is implemented, ensure server-side validation is also in place to prevent malicious input.
Also applies to: 32-34
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if server-side validation exists for text input rg -A 5 'validate.*length|maxLength|minLength' --type tsLength of output: 3669
🧹 Nitpick comments (15)
frontend/components/ui/alert-dialog/AlertDialogFooter.vue (1)
1-21: Security Recommendation: Add input validation for dynamic classes.Since this is an alert dialog component that might handle sensitive information:
- Consider adding runtime validation for the class prop to prevent injection of malicious classes
- Implement Content Security Policy (CSP) headers to restrict inline styles
- Document security requirements for implementers
frontend/components/ui/alert-dialog/AlertDialogCancel.vue (2)
1-6: LGTM! Clean and well-structured imports.The imports are properly organized and typed. The use of radix-vue for the base dialog component is a good choice for accessibility and security.
Consider documenting the security implications of using radix-vue's AlertDialogCancel component, particularly its role in preventing accidental destructive actions.
7-14: Props handling looks secure but could benefit from validation.The props destructuring and delegation is clean, but consider adding runtime validation for enhanced security.
Consider adding prop validation to ensure type safety at runtime:
+import { PropType } from 'vue' + const props = defineProps<AlertDialogCancelProps & { class?: HTMLAttributes['class'] }>() + +const validateProps = (props: typeof props) => { + if (props.class && typeof props.class !== 'string' && !Array.isArray(props.class)) { + console.warn('AlertDialogCancel: class prop must be a string or array') + return false + } + return true +}frontend/components/App/QuickMenuModal.vue (3)
21-27: Consider adding runtime type validation for actions prop.While TypeScript provides compile-time type safety, consider adding runtime validation for the actions array to prevent potential injection of malicious actions.
+ import { z } from 'zod'; + + const ActionSchema = z.union([ + z.object({ + text: z.string(), + action: z.function(), + type: z.literal('navigate') + }), + z.object({ + text: z.string(), + action: z.function(), + shortcut: z.string(), + type: z.literal('create') + }) + ]); + const props = defineProps({ actions: { type: Array as PropType<QuickMenuAction[]>, required: false, default: () => [], + validator: (value: QuickMenuAction[]) => { + try { + z.array(ActionSchema).parse(value); + return true; + } catch { + return false; + } + } }, });
32-41: Consider rate limiting keyboard shortcuts.The keyboard shortcut handler could be triggered rapidly, potentially causing performance issues or unintended behavior.
+ import { debounce } from '@vueuse/core'; + const keys = useMagicKeys(); const CtrlBackquote = keys.control_Backquote; - function handleOpenChange() { + const handleOpenChange = debounce(() => { open.value = !open.value; - } + }, 200);
61-74: Implement action execution timeout.Both create and navigate action executions should have a timeout to prevent potential hanging of the UI if an action takes too long.
+ const executeAction = async (action: () => void) => { + const timeoutPromise = new Promise((_, reject) => { + setTimeout(() => reject(new Error('Action timeout')), 5000); + }); + + try { + await Promise.race([action(), timeoutPromise]); + } catch (error) { + console.error('Action execution failed:', error); + // Optionally show error toast + } + }; // In template, update both create and navigate action handlers: @select=" async () => { open = false; - create.action(); + await executeAction(create.action); } "Also applies to: 78-91
frontend/components/ui/alert-dialog/AlertDialogTrigger.vue (1)
1-5: LGTM! Consider prop validation for enhanced security.The TypeScript props definition provides good type safety. However, for additional security, consider adding runtime validation for critical props that could affect the dialog's behavior.
<script setup lang="ts"> import { AlertDialogTrigger, type AlertDialogTriggerProps } from 'radix-vue' -const props = defineProps<AlertDialogTriggerProps>() +const props = withDefaults(defineProps<AlertDialogTriggerProps>(), { + // Add default values and validation here + asChild: false, +}) </script>frontend/components/App/OutdatedModal.vue (4)
2-20: Ensure consistent external link security.Notably,
rel="noopener"is present, which is good practice. Consider also adding"noreferrer"to fortify external link protection when pointing to GitHub releases.
60-60: Consider removing console logging in production.This console output can clutter production logs or inadvertently reveal app signals. Remove or guard it using environment checks.
68-70: Notify or prompt users about data storage.Users might not realize the dismissal is remembered. Providing a short notice (e.g., “We’re storing your version preference in localStorage.”) can be more transparent and user-friendly.
2-70: Security Recommendation: Review Access Control & CSP.For improved security across modals and dialogs, ensure a robust Content Security Policy (CSP) is in place to mitigate XSS vectors. Also review user access control so that outdated version prompts do not reveal sensitive version data to unauthorized users.
frontend/components/ui/alert-dialog/AlertDialogHeader.vue (1)
1-16: Security Recommendation: Consistent Security Policies Project-Wide.Remember to ensure that new UI components—like this header—comply with your application’s security standards for sanitizing any user-generated content that may appear in the slot.
frontend/components/ui/alert-dialog/AlertDialog.vue (1)
1-14: Security Recommendation: Centralize critical dialogs.When adopting new dialogs, consider a central configuration for security-related props (e.g., ARIA roles, keyboard traps, etc.) to avoid accidental misconfigurations across different components.
frontend/components/Location/CreateModal.vue (1)
73-75: Consider using Vue's focus directives.Instead of manual focus management with timeouts, consider using Vue's built-in v-focus directive or create a custom directive.
+ const vFocus = { + mounted: (el) => el.focus() + } + + // Register directive in component + defineDirectives({ + focus: vFocus + })Also applies to: 84-84
frontend/components/ui/alert-dialog/AlertDialogTitle.vue (1)
15-22: Consider enhancing accessibility with ARIA attributes.While the implementation is clean, consider adding ARIA attributes to improve accessibility for screen readers.
<AlertDialogTitle v-bind="delegatedProps" :class="cn('text-lg font-semibold', props.class)" + role="heading" + aria-level="2" > <slot /> </AlertDialogTitle>
🛑 Comments failed to post (4)
frontend/components/App/QuickMenuModal.vue (1)
48-57: 🛠️ Refactor suggestion
Sanitize keyboard input before processing.
The keydown event handler directly processes user input without sanitization. While the risk is low due to the limited scope of keyboard events, it's good practice to validate input.
@keydown=" (e: KeyboardEvent) => { + // Validate key length to prevent buffer overflow attempts + if (e.key.length > 20) return; + + // Only allow alphanumeric and common special characters + if (!/^[a-zA-Z0-9\-_+]$/.test(e.key)) return; + const action = props.actions.filter(item => 'shortcut' in item).find(item => item.shortcut === e.key); if (action) { open = false; action.action(); } } "📝 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.@keydown=" (e: KeyboardEvent) => { // Validate key length to prevent buffer overflow attempts if (e.key.length > 20) return; // Only allow alphanumeric and common special characters if (!/^[a-zA-Z0-9\-_+]$/.test(e.key)) return; const action = props.actions.filter(item => 'shortcut' in item).find(item => item.shortcut === e.key); if (action) { open = false; action.action(); } } " />frontend/components/ModalConfirm.vue (1)
23-28:
⚠️ Potential issueRemove console.log statement and consider security implications.
- The console.log statement should be removed as it could leak modal state information in production.
- Consider implementing rate limiting for confirmation actions if they trigger sensitive operations.
- watch( - () => isRevealed.value, - open => { - console.log("isRevealed", open); - } - );📝 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.frontend/components/Label/CreateModal.vue (2)
76-85: 🛠️ Refactor suggestion
Restore focus management for accessibility.
The commented-out focus management code should be restored or replaced with an alternative solution to maintain accessibility.
- // watch( - // () => modal.value, - // open => { - // if (open) - // useTimeoutFn(() => { - // focused.value = true; - // }, 50); - // else focused.value = false; - // } - // ); + watch( + () => modal.value, + (open) => { + if (open) { + nextTick(() => { + focused.value = true; + }); + } else { + focused.value = false; + } + } + );📝 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) { nextTick(() => { focused.value = true; }); } else { focused.value = false; } } );
8-38:
⚠️ Potential issueAdd CSRF protection to form submission.
The form submission lacks CSRF protection, which could lead to cross-site request forgery attacks.
Add the CSRF token to the form:
- <form @submit.prevent="create()"> + <form @submit.prevent="create()" :data-csrf="csrfToken">📝 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.<form @submit.prevent="create()" :data-csrf="csrfToken"> <FormTextField ref="locationNameRef" v-model="form.name" :trigger-focus="focused" :autofocus="true" :label="$t('components.label.create_modal.label_name')" :max-length="255" :min-length="1" /> <FormTextArea v-model="form.description" :label="$t('components.label.create_modal.label_description')" :max-length="255" /> <div class="modal-action"> <div class="flex justify-center"> <BaseButton class="rounded-r-none" :loading="loading" type="submit"> {{ $t("global.create") }} </BaseButton> <div class="dropdown dropdown-top"> <label tabindex="0" class="btn rounded-l-none rounded-r-xl"> <MdiChevronDown class="size-5" /> </label> <ul tabindex="0" class="dropdown-content menu rounded-box right-0 w-64 bg-base-100 p-2 shadow"> <li> <button type="button" @click="create(false)">{{ $t("global.create_and_add") }}</button> </li> </ul> </div> </div> </div> </form>
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
🔭 Outside diff range comments (2)
frontend/layouts/default.vue (2)
310-313: Enhance logout security.The logout implementation should clear sensitive data and invalidate the session.
async function logout() { + // Clear sensitive data from stores + await Promise.all([ + labelStore.$reset(), + locationStore.$reset(), + ]); + // Clear local storage + localStorage.clear(); + // Clear session storage + sessionStorage.clear(); await authCtx.logout(api); + // Force reload to clear Vue's internal state + window.location.href = '/'; - navigateTo("/"); }
214-223: Add rate limiting for keyboard shortcuts.Implement rate limiting for keyboard shortcuts to prevent potential DoS attacks from rapid key presses.
dropdown.forEach(option => { if (option?.shortcut) { const shortcutKeycode = option.shortcut.replace(/([0-9])/, "digit$&"); + const throttle = useThrottleFn(() => { + if (activeElement.value?.tagName !== "INPUT") { + openDialog(option.dialogId); + } + }, 500); whenever(keys[shortcutKeycode], () => { - if (activeElement.value?.tagName !== "INPUT") { - openDialog(option.dialogId); - } + throttle(); }); } });
🧹 Nitpick comments (4)
frontend/components/App/QuickMenuModal.vue (2)
33-38: Consider implementing rate limiting for keyboard events.The keyboard event handling could benefit from rate limiting to prevent potential DoS through rapid keyboard events.
const keys = useMagicKeys(); const CtrlBackquote = keys.control_Backquote; +const lastTriggerTime = ref(0); +const THROTTLE_MS = 300; + watch(CtrlBackquote, v => { - if (v) openDialog("quick-menu"); + if (v) { + const now = Date.now(); + if (now - lastTriggerTime.value >= THROTTLE_MS) { + openDialog("quick-menu"); + lastTriggerTime.value = now; + } + } });
43-53: Add input validation for keyboard events.The keydown handler directly uses user input without validation. While the risk is low due to keyboard event constraints, it's good practice to add validation.
<CommandInput :placeholder="t('components.quick_menu.shortcut_hint')" @keydown=" (e: KeyboardEvent) => { + // Validate key length to prevent buffer overflow attempts + if (e.key.length > 1) return; + const item = props.actions.filter(item => 'shortcut' in item).find(item => item.shortcut === e.key); if (item) { openDialog(item.dialogId); } } " />frontend/components/Label/CreateModal.vue (1)
70-79: Consider removing commented watch block.The commented watch block for modal focus management should be either implemented or removed.
- // watch( - // () => modal.value, - // open => { - // if (open) - // useTimeoutFn(() => { - // focused.value = true; - // }, 50); - // else focused.value = false; - // } - // );frontend/components/Item/CreateModal.vue (1)
207-217: Add error retry mechanism for photo upload.The photo upload error handling could be improved with a retry mechanism for better user experience.
if (form.photo) { + const maxRetries = 3; + let retryCount = 0; + while (retryCount < maxRetries) { const { error } = await api.items.attachments.add(data.id, form.photo, form.photo.name, AttachmentTypes.Photo); if (error) { + retryCount++; + if (retryCount === maxRetries) { loading.value = false; toast.error("Failed to upload Photo"); return; + } + await new Promise(resolve => setTimeout(resolve, 1000 * retryCount)); + continue; } + break; + } toast.success("Photo uploaded"); }
🛑 Comments failed to post (3)
frontend/components/App/QuickMenuModal.vue (1)
73-85:
⚠️ Potential issueAdd URL validation before navigation.
The navigation handler should validate URLs before navigation to prevent potential open redirect vulnerabilities.
+const isValidInternalUrl = (url: string) => { + return url.startsWith('/') && !url.startsWith('//'); +}; + <CommandItem v-for="(navigate, i) in props.actions.filter(item => item.type === 'navigate')" :key="navigate.text" :value="`global.navigate_${i + 1}`" @select=" () => { closeDialog('quick-menu'); - navigateTo(navigate.href); + if (isValidInternalUrl(navigate.href)) { + navigateTo(navigate.href); + } else { + console.error('Invalid navigation URL detected'); + } } " >📝 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 isValidInternalUrl = (url: string) => { return url.startsWith('/') && !url.startsWith('//'); }; <CommandItem v-for="(navigate, i) in props.actions.filter(item => item.type === 'navigate')" :key="navigate.text" :value="`global.navigate_${i + 1}`" @select=" () => { closeDialog('quick-menu'); if (isValidInternalUrl(navigate.href)) { navigateTo(navigate.href); } else { console.error('Invalid navigation URL detected'); } } " > {{ navigate.text }} </CommandItem>frontend/app.vue (1)
19-19:
⚠️ Potential issueRemove console.log in production.
Console logging should be removed in production as it could leak sensitive information.
-console.log("running in app"); +if (process.env.NODE_ENV !== 'production') { + console.log("running in app"); +}📝 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.if (process.env.NODE_ENV !== 'production') { console.log("running in app"); }frontend/components/Label/CreateModal.vue (1)
8-17: 💡 Codebase verification
Input Sanitization for Label Name is Missing – Security Risk
Based on our search, there is no evidence of input sanitization (e.g., using DOMPurify or similar libraries) for the label name in
frontend/components/Label/CreateModal.vue. While sanitization utilities are present in other parts of the codebase (e.g., infrontend/components/global/Markdown.vue), they are not applied here. This leaves the form input vulnerable to potential XSS attacks. Please add appropriate sanitization before processing or storing user input.
- Location to address:
frontend/components/Label/CreateModal.vue(Lines 8-17)- Recommendation: Integrate an XSS prevention library (e.g., DOMPurify) to sanitize
form.nameinput on submission.🔗 Analysis chain
Add input sanitization for label name.
The form input for label name should be sanitized to prevent XSS attacks.
Run this script to check for XSS prevention in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for XSS prevention utilities rg -l "v-safe-html|DOMPurify|sanitize"Length of output: 105
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: 4
🔭 Outside diff range comments (5)
frontend/components/Item/CreateModal.vue (1)
197-218: Strengthen API security measuresThe API interaction needs additional security controls:
- Validate API responses thoroughly
- Implement rate limiting for create operations
- Use generic error messages to prevent information leakage
- Add request timeout
- const { error, data } = await api.items.create(out); + const { error, data } = await Promise.race([ + api.items.create(out), + new Promise((_, reject) => + setTimeout(() => reject(new Error('Request timeout')), 30000) + ) + ]); loading.value = false; if (error) { loading.value = false; - toast.error("Couldn't create item"); + toast.error("Operation failed. Please try again later."); return; }frontend/layouts/default.vue (4)
310-313: Add CSRF protection to logout operation.The logout operation should be protected against CSRF attacks.
Ensure the API call includes CSRF tokens:
async function logout() { + const csrfToken = document.querySelector('meta[name="csrf-token"]')?.getAttribute('content'); await authCtx.logout(api, { + headers: { + 'X-CSRF-Token': csrfToken + } }); navigateTo("/"); }
113-115: Sanitize version information display.The version and build information is displayed without sanitization, which could be a security risk if compromised.
Apply this diff to sanitize the output:
- {{ $t("global.version", { version: status.build.version }) }} ~ - {{ $t("global.build", { build: status.build.commit }) }}</a + {{ $t("global.version", { version: DOMPurify.sanitize(status.build.version) }) }} ~ + {{ $t("global.build", { build: DOMPurify.sanitize(status.build.commit) }) }}</a
214-223: Add rate limiting to keyboard shortcuts.The keyboard shortcut handling should be rate-limited to prevent abuse.
Apply this diff to add debouncing:
+ const debouncedOpenDialog = useDebounceFn((dialogId: string) => { + openDialog(dialogId); + }, 300); dropdown.forEach(option => { if (option?.shortcut) { const shortcutKeycode = option.shortcut.replace(/([0-9])/, "digit$&"); whenever(keys[shortcutKeycode], () => { if (activeElement.value?.tagName !== "INPUT") { - openDialog(option.dialogId); + debouncedOpenDialog(option.dialogId); } }); } });
173-177: Add error handling for status API call.The status API call lacks proper error handling, which could lead to unhandled exceptions.
Apply this diff to add error handling:
const { data: status } = useAsyncData(async () => { - const { data } = await pubApi.status(); - return data; + try { + const { data } = await pubApi.status(); + return data; + } catch (error) { + console.error('Failed to fetch status:', error); + return null; + } });
🧹 Nitpick comments (1)
frontend/components/Label/Selector.vue (1)
34-35: Convert TODOs to tracked issues.The TODO comments should be converted to tracked issues for better visibility and follow-up.
Would you like me to create issues for:
- Setting cursor position when radix-vue 2 is released
- Replacing the "no item found" text and framework placeholder?
🛑 Comments failed to post (4)
frontend/components/Item/CreateModal.vue (1)
28-34:
⚠️ Potential issueEnhance file upload security measures
The current file upload implementation needs additional security controls:
- Add server-side file type validation
- Implement file size limits
- Consider adding antivirus scanning for uploaded files
<input id="photo" class="hidden" type="file" accept="image/png,image/jpeg,image/gif,image/avif,image/webp" + max-size="5242880" @change="previewImage" />Committable suggestion skipped: line range outside the PR's diff.
frontend/components/Label/Selector.vue (3)
26-26: 🛠️ Refactor suggestion
Remove commented-out code.
The commented-out code should either be implemented or removed to maintain code cleanliness.
Apply this diff to remove the commented code:
- // ...frameworks.filter(i => !modelValue.value.includes(i.label)), - // const filterFunction = (list: string[], search: string) => { - // return list; - // }; - // :filter-function="filterFunction"Also applies to: 29-32
66-73: 🛠️ Refactor suggestion
Add type safety to event handling.
The event handling lacks proper TypeScript type definitions, which could lead to runtime errors.
Apply this diff to add type safety:
- @select.prevent=" - ev => { - if (typeof ev.detail.value === 'string') { - searchTerm = ''; - modelValue.push(ev.detail.value); - } - } - " + @select.prevent=" + (ev: { detail: { value: unknown } }) => { + if (typeof ev.detail.value === 'string') { + searchTerm = ''; + modelValue.push(ev.detail.value); + } + } + "
25-28:
⚠️ Potential issueSanitize user input in search term.
The
searchTermvalue is directly used in the UI without sanitization, which could potentially lead to XSS attacks if the input contains malicious HTML/JavaScript.Apply this diff to sanitize the input:
const filteredFrameworks = computed(() => [ // ...frameworks.filter(i => !modelValue.value.includes(i.label)), - { value: "create-item", label: `Create ${searchTerm.value}` }, + { value: "create-item", label: `Create ${DOMPurify.sanitize(searchTerm.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.const filteredFrameworks = computed(() => [ // ...frameworks.filter(i => !modelValue.value.includes(i.label)), { value: "create-item", label: `Create ${DOMPurify.sanitize(searchTerm.value)}` }, ]);
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
🛑 Comments failed to post (3)
frontend/components/ui/textarea/Textarea.vue (1)
6-6:
⚠️ Potential issueFix undefined variable usage.
The
useTextareaAutosizefunction is used but not imported, which will cause a runtime error.Add the import at the top of the script:
import { useVModel } from '@vueuse/core' +import { useTextareaAutosize } from '@vueuse/core'frontend/components/Form/TextArea.vue (2)
37-38: 💡 Verification agent
🧩 Analysis chain
Verify component import paths.
The import paths using
~alias might cause issues in different environments.Consider using absolute paths or properly configured aliases:
-import { Label } from "~/components/ui/label"; -import { Textarea } from "~/components/ui/textarea"; +import { Label } from "@/components/ui/label"; +import { Textarea } from "@/components/ui/textarea";
🏁 Script executed:
#!/bin/bash # Verify the component import paths rg -l "^import.*from ['\"]~/components" frontend/Length of output: 93
Component Import Path Alias Inconsistency – Update All References
The diff in frontend/components/Form/TextArea.vue suggests switching from the tilde (
) alias to the at (@) alias. However, our verification shows that in addition to the TextArea.vue file, there's at least one occurrence in frontend/composables/use-preferences.ts that still uses the "" alias. This inconsistency may lead to issues in environments where the tilde alias isn’t properly configured.• Update all component imports consistently (e.g., replacing "~/components" with "@/components") to ensure compatibility across environments.
• Double-check your module bundler (e.g., Vite, webpack) alias configurations to make sure they align with the new alias.
• As a security recommendation, verify that your alias configuration does not inadvertently expose internal modules or lead to unexpected path resolution issues during bundling.
69-70: 🛠️ Refactor suggestion
Enhance ID generation security.
Using
useId()without proper initialization could lead to predictable IDs. Consider using a more secure ID generation method.Replace the current ID generation with a more secure version:
-const id = useId(); +const id = `textarea-${useId()}-${Math.random().toString(36).slice(2, 11)}`;📝 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 id = `textarea-${useId()}-${Math.random().toString(36).slice(2, 11)}`; const value = useVModel(props, "modelValue");
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
🛑 Comments failed to post (2)
frontend/components/Location/Selector.vue (2)
120-138:
⚠️ Potential issueFix import order per pipeline failures.
Move the
lucide-vue-nextimport before component imports to resolve the pipeline error.+import { Check, ChevronsUpDown } from 'lucide-vue-next' import { Button } from '~/components/ui/button' import { Command, CommandEmpty, CommandGroup, CommandInput, CommandItem, CommandList } from '~/components/ui/command' -import { Check, ChevronsUpDown } from 'lucide-vue-next'📝 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.import { Check, ChevronsUpDown } from 'lucide-vue-next' import { Button } from '~/components/ui/button' import { Command, CommandEmpty, CommandGroup, CommandInput, CommandItem, CommandList } from '~/components/ui/command' import { Label } from '~/components/ui/label' import { Popover, PopoverContent, PopoverTrigger, } from '~/components/ui/popover' import { cn } from '~/lib/utils' import type { LocationSummary } from "~~/lib/api/types/data-contracts" import { useFlatLocations } from "~~/composables/use-location-helpers"🧰 Tools
🪛 GitHub Actions: Pull Request CI
[warning] 120-120: Replace
import·{·Button·}·from·'~/components/ui/button'with··import·{·Button·}·from·"~/components/ui/button";
[warning] 121-121: Replace
import·{⏎··Command,⏎··CommandEmpty,⏎··CommandGroup,⏎··CommandInput,⏎··CommandItem,⏎··CommandList⏎}·from·'~/components/ui/command'with··import·{·Command,·CommandEmpty,·CommandGroup,·CommandInput,·CommandItem,·CommandList·}·from·"~/components/ui/command";
[warning] 129-129: Replace
import·{·Label·}·from·'~/components/ui/label'with··import·{·Label·}·from·"~/components/ui/label";
[warning] 130-130: Replace
import·{⏎··Popover,⏎··PopoverContent,⏎··PopoverTrigger,⏎}·from·'~/components/ui/popover'with··import·{·Popover,·PopoverContent,·PopoverTrigger·}·from·"~/components/ui/popover";
[warning] 135-135: Replace
import·{·cn·}·from·'~/lib/utils'with··import·{·cn·}·from·"~/lib/utils";
[warning] 136-136: Replace
import·{·Check,·ChevronsUpDown·}·from·'lucide-vue-next'with··import·{·Check,·ChevronsUpDown·}·from·"lucide-vue-next";
[error] 136-136:
lucide-vue-nextimport should occur before import of~/components/ui/button
[warning] 137-137: Replace
import·type·{·LocationSummary·}·from·"~~/lib/api/types/data-contracts"with··import·type·{·LocationSummary·}·from·"~~/lib/api/types/data-contracts";
[warning] 138-138: Replace
import·{·useFlatLocations·}·from·"~~/composables/use-location-helpers"with··import·{·useFlatLocations·}·from·"~~/composables/use-location-helpers";
71-115: 🛠️ Refactor suggestion
Improve accessibility for keyboard navigation.
Add proper ARIA labels and keyboard navigation support:
<Popover v-model:open="open"> <PopoverTrigger as-child> <Button :id="id" variant="outline" role="combobox" :aria-expanded="open" + :aria-controls="`${id}-popup`" + :aria-haspopup="true" class="w-full justify-between" >📝 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.<Popover v-model:open="open"> <PopoverTrigger as-child> <Button :id="id" variant="outline" role="combobox" :aria-expanded="open" :aria-controls="`${id}-popup`" :aria-haspopup="true" class="w-full justify-between" > {{ value ? value.name : $t('components.location.selector.select_location') }} <ChevronsUpDown class="ml-2 h-4 w-4 shrink-0 opacity-50" /> </Button> </PopoverTrigger> <PopoverContent class="w-[--radix-popper-anchor-width] p-0"> <Command v-model="search"> <CommandInput :placeholder="$t('components.location.selector.search_location')" /> <CommandEmpty>{{ $t('components.location.selector.no_location_found') }}</CommandEmpty> <CommandList> <CommandGroup> <CommandItem v-for="location in locations" :key="location.id" :value="location.name" @select="selectLocation(location as unknown as LocationSummary)" > <Check :class="cn( 'mr-2 h-4 w-4', value?.id === location.id ? 'opacity-100' : 'opacity-0', )" /> <div> <div class="flex w-full"> {{ location.name }} </div> <div v-if="location.name !== location.treeString" class="mt-1 text-xs text-muted-foreground"> {{ location.treeString }} </div> </div> </CommandItem> </CommandGroup> </CommandList> </Command> </PopoverContent> </Popover>🧰 Tools
🪛 GitHub Actions: Pull Request CI
[warning] 73-73: Replace
⏎··········:id="id"⏎··········variant="outline"⏎··········role="combobox"⏎··········:aria-expanded="open"⏎··········class="w-full·justify-between"⏎········with·:id="id"·variant="outline"·role="combobox"·:aria-expanded="open"·class="w-full·justify-between"
[warning] 80-80: Replace
'components.location.selector.select_location'with "components.location.selector.select_location"
[warning] 81-81: Classnames 'h-4, w-4' could be replaced by the 'size-4' shorthand!
[warning] 87-87: Replace
'components.location.selector.no_location_found'with "components.location.selector.no_location_found"
[warning] 96-96: Replace
⏎··················:class="cn(⏎····················'mr-2·h-4·w-4',⏎····················value?.id·===·location.id·?·'opacity-100'·:·'opacity-0',⏎··················)with·:class="cn('mr-2·h-4·w-4',·value?.id·===·location.id·?·'opacity-100'·:·'opacity-0')
Deploying homebox-docs with
|
| Latest commit: |
5d96072
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://54395a50.homebox-docs.pages.dev |
| Branch Preview URL: | https://tonya-shadcn-layout.homebox-docs.pages.dev |
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
🧹 Nitpick comments (2)
frontend/components/Label/Selector.vue (2)
41-47: Implement maximum tag limit.There's currently no limit on the number of tags that can be added, which could lead to performance issues or potential DoS vectors if abused.
Add a reasonable maximum limit:
-<TagsInput v-model="modelValue" class="w-80 gap-0 px-0"> +<TagsInput + v-model="modelValue" + class="w-80 gap-0 px-0" + :disabled="modelValue.length >= 10" +>
13-19: Consider moving frameworks array to a configuration file.Hardcoding the frameworks array in the component makes it difficult to maintain and update. Consider moving it to a separate configuration file for better maintainability.
Create a new configuration file (e.g.,
config/frameworks.ts):export const frameworks = [ { value: "next.js", label: "Next.js" }, { value: "sveltekit", label: "SvelteKit" }, { value: "nuxt", label: "Nuxt" }, { value: "remix", label: "Remix" }, { value: "astro", label: "Astro" }, ] as const; export type Framework = typeof frameworks[number]['value'];
🛑 Comments failed to post (2)
frontend/components/Label/Selector.vue (2)
31-33: 🛠️ Refactor suggestion
Implement proper filtering function.
The current filter function returns the unmodified list without any actual filtering logic, which could lead to performance issues with large datasets.
Consider implementing proper filtering:
const filterFunction = (list: string[], search: string) => { - return list; + const sanitizedSearch = search.toLowerCase(); + return list.filter(item => item.toLowerCase().includes(sanitizedSearch)); };📝 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 filterFunction = (list: string[], search: string) => { const sanitizedSearch = search.toLowerCase(); return list.filter(item => item.toLowerCase().includes(sanitizedSearch)); };
69-75:
⚠️ Potential issueAdd input validation for tag selection.
The current implementation directly pushes user input to the model without validation, which could lead to XSS vulnerabilities if the values are later used in unsafe contexts.
Add validation before adding new tags:
@select.prevent=" ev => { if (typeof ev.detail.value === 'string') { + const value = ev.detail.value.trim(); + if (!value || value.length > 50) return; // Add reasonable length limit searchTerm = ''; - modelValue.push(ev.detail.value); + modelValue.push(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.ev => { if (typeof ev.detail.value === 'string') { const value = ev.detail.value.trim(); if (!value || value.length > 50) return; // Add reasonable length limit searchTerm = ''; modelValue.push(value); } } "
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: 5
🔭 Outside diff range comments (3)
frontend/components/Label/CreateModal.vue (1)
83-108: Fix potential race condition in loading state management.There's a small window between checking and setting the loading state where multiple submissions could occur.
Consider using a mutex or atomic operation. Here's a suggested fix:
async function create(close = true) { - if (loading.value) { + if (loading.value || !loading.value.compareAndSet(false, true)) { toast.error("Already creating a label"); return; } - loading.value = true;frontend/components/Item/CreateModal.vue (1)
190-219: Enhance error handling and rollback mechanism.The current implementation handles errors but could benefit from a more robust error handling strategy.
Consider these improvements:
- Implement rollback mechanism if photo upload fails
- Add retry logic for failed uploads
- Provide more specific error messages
if (error) { loading.value = false; - toast.error("Couldn't create item"); + toast.error(`Failed to create item: ${error.message}`); return; }frontend/components/Location/Selector.vue (1)
1-63: Remove commented out code.The old implementation should be removed rather than commented out. Git history will preserve this code if needed for future reference.
-<!-- <template> - <FormAutocomplete2 - v-if="locations" - v-model="value" - :items="locations" - display="name" - :label="$t('components.location.selector.parent_location')" - > - <template #display="{ item, selected, active }"> - <div> - <div class="flex w-full"> - {{ cast(item.value).name }} - <span - v-if="selected" - :class="['absolute inset-y-0 right-0 flex items-center pr-4', active ? 'text-white' : 'text-primary']" - > - <MdiCheck class="size-5" aria-hidden="true" /> - </span> - </div> - <div v-if="cast(item.value).name != cast(item.value).treeString" class="mt-1 text-xs"> - {{ cast(item.value).treeString }} - </div> - </div> - </template> - </FormAutocomplete2> -</template> - -<script lang="ts" setup> - import type { FlatTreeItem } from "~~/composables/use-location-helpers"; - import { useFlatLocations } from "~~/composables/use-location-helpers"; - import type { LocationSummary } from "~~/lib/api/types/data-contracts"; - import MdiCheck from "~icons/mdi/check"; - - type Props = { - modelValue?: LocationSummary | null; - }; - - // Cast the type of the item to a FlatTreeItem so we can get type "safety" in the template - // Note that this does not actually change the type of the item, it just tells the compiler - // that the type is FlatTreeItem. We must keep this in sync with the type of the items - function cast(value: any): FlatTreeItem { - return value as FlatTreeItem; - } - - const props = defineProps<Props>(); - const value = useVModel(props, "modelValue"); - - const locations = useFlatLocations(); - const form = ref({ - parent: null as LocationSummary | null, - search: "", - }); - - // Whenever parent goes from value to null reset search - watch( - () => value.value, - () => { - if (!value.value) { - form.value.search = ""; - } - } - ); -</script> -->
🧹 Nitpick comments (5)
frontend/components/Label/Selector.vue (2)
13-24: Consider moving frameworks to a configuration file and add input validation.
- Move the frameworks array to a separate configuration file for better maintainability.
- Add input validation for the modelValue to prevent XSS attacks.
Example validation implementation:
+const validateInput = (value: string) => { + // Prevent XSS by allowing only alphanumeric characters, dots, and hyphens + return /^[a-zA-Z0-9.-]+$/.test(value); +}; const modelValue = ref<string[]>([]); +watch(modelValue, (newValue) => { + modelValue.value = newValue.filter(validateInput); +});
69-70: Consider using CSS variables for sensitive styling information.The inline CSS classes could expose styling information. Consider moving them to CSS variables for better security and maintainability.
- position="popper" - class="mt-2 w-[--radix-popper-anchor-width] rounded-md border bg-popover text-popover-foreground shadow-md outline-none data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2" + position="popper" + :class="$style.commandList"Add to your styles:
<style module> .commandList { composes: mt-2 w-[--radix-popper-anchor-width] rounded-md border bg-popover text-popover-foreground shadow-md outline-none from global; composes: data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 from global; composes: data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 from global; composes: data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2 from global; } </style>frontend/components/Location/Selector.vue (3)
88-88: Improve type safety in CommandItem select handler.The type cast using
as unknown as LocationSummaryis a code smell that indicates potential type safety issues.Consider updating the locations type or using a type guard instead of force casting:
-@select="selectLocation(location as unknown as LocationSummary)" +@select="selectLocation(location)"
131-138: Rename selectLocation for clarity and improve toggle behavior.The function name could be more descriptive, and the toggle behavior could be simplified.
-function selectLocation(location: LocationSummary) { - if (value.value?.id !== location.id) { - value.value = location; - } else { - value.value = null; - } - open.value = false; -} +function toggleLocationSelection(location: LocationSummary) { + value.value = value.value?.id === location.id ? null : location; + open.value = false; +}
141-148: Simplify watch function.The watch function can be more concise by directly watching the nullish state.
-watch( - () => value.value, - () => { - if (!value.value) { - search.value = ""; - } - } -); +watch( + () => !value.value, + (isNull) => isNull && (search.value = "") +);
🛑 Comments failed to post (5)
frontend/components/Item/CreateModal.vue (1)
28-36:
⚠️ Potential issueImplement file upload security measures.
The current file upload implementation needs additional security measures to prevent malicious file uploads.
Consider implementing these security measures:
- Add file size validation
- Implement server-side MIME type verification
- Add antivirus scanning for uploaded files
<Input id="image-create-photo" class="w-full" type="file" accept="image/png,image/jpeg,image/gif,image/avif,image/webp" + :max-size="5242880" @change="previewImage" >gay</Input>Also, remove the hardcoded text "gay" from the Input component as it appears to be unintentional.
Committable suggestion skipped: line range outside the PR's diff.
frontend/components/Label/Selector.vue (3)
36-42: 🛠️ Refactor suggestion
Remove or implement the filterFunction.
The current filterFunction is a pass-through function that doesn't provide any value. Either remove it or implement proper filtering logic.
Also, consider creating issues for the TODO items to track their implementation.
Would you like me to create issues for the TODO items?
78-85: 🛠️ Refactor suggestion
Improve type safety in event handler.
The event handler lacks proper TypeScript type checking and could be more robust.
- @select.prevent=" - ev => { - if (typeof ev.detail.value === 'string') { - searchTerm = ''; - modelValue.push(ev.detail.value); - } - } - " + @select.prevent=" + (ev: { detail: { value: unknown } }) => { + const value = ev.detail.value; + if (typeof value === 'string' && validateInput(value)) { + searchTerm = ''; + modelValue.push(value); + } + } + "Committable suggestion skipped: line range outside the PR's diff.
25-35:
⚠️ Potential issueAdd input sanitization to search term filtering.
The current implementation lacks input sanitization for the search term, which could potentially lead to XSS vulnerabilities.
const filteredFrameworks = computed(() => { + const sanitizedSearchTerm = searchTerm.value.replace(/[<>]/g, ''); const filtered = frameworks.filter(i => { - return i.label.toLocaleLowerCase().includes(searchTerm.value.toLocaleLowerCase()); + return i.label.toLocaleLowerCase().includes(sanitizedSearchTerm.toLocaleLowerCase()); }); - if (searchTerm.value.trim() !== "") { + if (sanitizedSearchTerm.trim() !== "") { filtered.push({ value: "create-item", label: `Create ${searchTerm.value}` }); } return filtered; });📝 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 filteredFrameworks = computed(() => { + const sanitizedSearchTerm = searchTerm.value.replace(/[<>]/g, ''); const filtered = frameworks.filter(i => { - return i.label.toLocaleLowerCase().includes(searchTerm.value.toLocaleLowerCase()); + return i.label.toLocaleLowerCase().includes(sanitizedSearchTerm.toLocaleLowerCase()); }); - if (searchTerm.value.trim() !== "") { + if (sanitizedSearchTerm.trim() !== "") { filtered.push({ value: "create-item", label: `Create ${searchTerm.value}` }); } return filtered; });frontend/components/Location/Selector.vue (1)
93-93: 💡 Verification agent
🧩 Analysis chain
Sanitize displayed location data.
Location names and tree strings are directly rendered in the template. Consider sanitizing this data to prevent XSS attacks.
Also applies to: 96-96
🏁 Script executed:
#!/bin/bash # Check for potential XSS vulnerabilities in location data handling # Search for direct rendering of location data without sanitization rg -A 2 '{{.*location\.(name|treeString).*}}'Length of output: 158
Below is a revised verification script with a corrected regex to ensure we properly detect unsanitized location data rendering. Once executed, please manually verify whether the output shows direct mustache bindings for properties like
location.nameorlocation.treeStringwithout sanitization.Security: Sanitize Displayed Location Data
- The component renders location attributes (e.g.,
location.name) directly, which might lead to XSS if the content is malicious.- Please ensure that any dynamic content injected into the template is correctly sanitized or escaped prior to rendering.
🏁 Script executed:
#!/bin/bash # Verify unsanitized rendering of location properties in Selector.vue echo "Searching for unsanitized location.name bindings:" rg -n "\{\{\s*location\.name\s*\}\}" frontend/components/Location/Selector.vue echo "Searching for unsanitized location.treeString bindings (if present):" rg -n "\{\{\s*location\.treeString\s*\}\}" frontend/components/Location/Selector.vueLength of output: 637
Security: Sanitize Displayed Location Data
Issue Identified: The component renders
location.name(line 93) andlocation.treeString(line 96) directly in the template. While Vue’s default interpolation escapes HTML, it’s important to ensure that no unsanitized or unexpected HTML is introduced through these properties—especially if any of this data might eventually be rendered using directives that bypass Vue's built-in escaping (e.g.,v-html) or if the data flow changes in the future.Recommendation:
• Validate and sanitize any data that originates from user input before it reaches the template.
• Consider using a library like DOMPurify to enforce strict sanitation if there’s any risk of unsanitized HTML slipping through.
• Regularly review your data flow to ensure that changes in the code do not expose potential XSS vectors.
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: 5
🧹 Nitpick comments (4)
frontend/pages/testing.vue (1)
14-16: Consider reducing toast duration for security and UX.The 100-second toast duration could lead to UI clutter and potentially mask important system notifications. Consider reducing this to a more standard duration (3-5 seconds).
- duration: 100000, + duration: 5000,frontend/components/Item/CreateModal.vue (2)
197-203: Enhance error handling for item creation.The error handling could be more specific to help users understand and resolve issues:
const { error, data } = await api.items.create(out); loading.value = false; if (error) { loading.value = false; - toast.error("Couldn't create item"); + toast.error(`Failed to create item: ${error.message || 'Unknown error'}`); return; }
208-218: Add retry mechanism for photo upload failures.Consider implementing a retry mechanism for photo uploads to handle temporary network issues.
Would you like me to provide an implementation of a retry mechanism with exponential backoff?
frontend/components/Label/Selector.vue (1)
19-35: Strengthen props validation for better security.The current props validation is basic. Consider adding more robust validation:
const props = defineProps({ modelValue: { type: Array as () => string[], default: null, + validator: (value: string[] | null) => { + if (value === null) return true; + return value.every(id => typeof id === 'string' && id.length > 0); + } }, labels: { type: Array as () => LabelOut[], required: true, + validator: (value: LabelOut[]) => { + return value.every(label => + typeof label.id === 'string' && + typeof label.name === 'string' + ); + } }, });
🛑 Comments failed to post (5)
frontend/components/Item/CreateModal.vue (2)
83-83:
⚠️ Potential issueRemove unused type import.
The
LabelOuttype is imported but never used, as flagged by the linter.-import type { ItemCreate, LabelOut, LocationOut } from "~~/lib/api/types/data-contracts"; +import type { ItemCreate, LocationOut } from "~~/lib/api/types/data-contracts";📝 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.import type { ItemCreate, LocationOut } from "~~/lib/api/types/data-contracts";🧰 Tools
🪛 GitHub Actions: Pull Request CI
[error] 83-83: 'LabelOut' is defined but never used @typescript-eslint/no-unused-vars
28-34: 🛠️ Refactor suggestion
Enhance file upload security measures.
The file upload implementation could benefit from additional security measures:
- Add file size validation
- Implement server-side MIME type verification
- Consider adding virus scanning for uploaded files
<Input id="image-create-photo" class="w-full" type="file" + :maxSize="5242880" // 5MB limit accept="image/png,image/jpeg,image/gif,image/avif,image/webp" @change="previewImage" />📝 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.<Input id="image-create-photo" class="w-full" type="file" :maxSize="5242880" // 5MB limit accept="image/png,image/jpeg,image/gif,image/avif,image/webp" @change="previewImage" />frontend/components/Label/Selector.vue (3)
78-78:
⚠️ Potential issueRemove debug output to prevent data exposure.
The template directly displays the modelValue, which could expose sensitive information in production.
- {{ modelValue }}📝 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.
57-74: 🛠️ Refactor suggestion
Enhance error handling for better security feedback.
The current error handling is basic and might expose sensitive information. Consider implementing more detailed error handling:
const createAndAdd = async (name: string) => { + if (!name || name.trim().length === 0) { + toast.error("Label name cannot be empty"); + return; + } const { error, data } = await api.labels.create({ name, color: "", // Future! description: "", }); if (error) { - toast.error("Couldn't create label"); + const safeMessage = error.status === 403 ? "Permission denied" : + error.status === 409 ? "Label already exists" : + "Failed to create label"; + toast.error(safeMessage); return; }📝 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 createAndAdd = async (name: string) => { + if (!name || name.trim().length === 0) { + toast.error("Label name cannot be empty"); + return; + } const { error, data } = await api.labels.create({ name, color: "", // Future! description: "", }); if (error) { - toast.error("Couldn't create label"); + const safeMessage = error.status === 403 ? "Permission denied" : + error.status === 409 ? "Label already exists" : + "Failed to create label"; + toast.error(safeMessage); return; } toast.success("Label created"); modelValue.value.push(data.id); }; // TODO: when radix-vue 2 is release use hook to set cursor to end when label is added with click </script>
121-132: 🛠️ Refactor suggestion
Secure event handling in CommandItem select.
The select handler directly processes user input without validation. Consider adding input validation:
@select.prevent=" async ev => { if (typeof ev.detail.value === 'string') { + const value = ev.detail.value.trim(); + if (!value) return; if (ev.detail.value === 'create-item') { - await createAndAdd(searchTerm); + await createAndAdd(searchTerm.trim()); } else { + if (!props.labels.some(l => l.id === value)) return; modelValue.push(ev.detail.value); } searchTerm = ''; } } "Committable suggestion skipped: line range outside the PR's diff.
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: 4
🧹 Nitpick comments (1)
frontend/components/Label/Selector.vue (1)
1-148: Security Recommendations for Production DeploymentTo ensure secure deployment, implement the following security measures:
- Add Content Security Policy (CSP) headers
- Implement proper CSRF protection for API calls
- Add rate limiting for label creation
- Sanitize all user inputs
- Remove debug logging
- Add proper error boundaries
- Implement proper access control
Would you like me to provide detailed implementation guidance for any of these security measures?
🛑 Comments failed to post (4)
frontend/components/Label/Selector.vue (4)
85-85:
⚠️ Potential issueValidate display value access.
The display value access assumes the label will always be found. Add null checks to prevent potential security issues from undefined access.
- :display-value="v => props.labels.find(l => l.id === v)!.name" + :display-value="v => props.labels.find(l => l.id === v)?.name ?? 'Unknown Label'"📝 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.:display-value="v => props.labels.find(l => l.id === v)?.name ?? 'Unknown Label'"
126-126:
⚠️ Potential issueRemove console.log from production code.
Console logging can expose sensitive information in production. Remove or replace with proper logging.
- console.log(ev); + // Use proper logging service if needed📝 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.// Use proper logging service if needed
42-44:
⚠️ Potential issueSanitize user input before search.
The search functionality directly uses user input without sanitization. Consider implementing input sanitization to prevent XSS attacks.
- return i.label.toLocaleLowerCase().includes(searchTerm.value.toLocaleLowerCase()); + const sanitizedSearch = DOMPurify.sanitize(searchTerm.value); + return i.label.toLocaleLowerCase().includes(sanitizedSearch.toLocaleLowerCase());📝 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..filter(i => { + const sanitizedSearch = DOMPurify.sanitize(searchTerm.value); - return i.label.toLocaleLowerCase().includes(searchTerm.value.toLocaleLowerCase()); + return i.label.toLocaleLowerCase().includes(sanitizedSearch.toLocaleLowerCase()); })
57-71:
⚠️ Potential issueEnhance security for label creation.
The label creation process needs additional security measures:
- Input validation for the label name
- Rate limiting to prevent abuse
- CSRF protection for the API call
const createAndAdd = async (name: string) => { + if (!name || name.length > 100) { + toast.error("Invalid label name"); + return; + } + + const sanitizedName = DOMPurify.sanitize(name); const { error, data } = await api.labels.create({ - name, + name: sanitizedName, color: "", // Future! description: "", });📝 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 createAndAdd = async (name: string) => { if (!name || name.length > 100) { toast.error("Invalid label name"); return; } const sanitizedName = DOMPurify.sanitize(name); const { error, data } = await api.labels.create({ name: sanitizedName, color: "", // Future! description: "", }); if (error) { toast.error("Couldn't create label"); return; } toast.success("Label created"); modelValue.value.push(data.id); };
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: 6
🧹 Nitpick comments (11)
frontend/components/ui/sidebar/Sidebar.vue (3)
34-34: Consider z-index security implications.While adding
z-40improves the visual hierarchy, high z-index values could potentially be exploited in overlay-based attacks if malicious content can be injected. Consider implementing a maximum z-index policy across the application to prevent z-index-based UI manipulation.
11-15: Enhance props validation for security.The current props validation is minimal. Consider adding runtime validation to ensure props values can't be manipulated to cause unwanted behavior:
+ import { z } from 'zod' + + const sidebarPropsSchema = z.object({ + side: z.enum(['left', 'right']).default('left'), + variant: z.enum(['sidebar', 'floating', 'inset']).default('sidebar'), + collapsible: z.enum(['none', 'offcanvas', 'icon']).default('offcanvas'), + }) + const props = withDefaults(defineProps<SidebarProps>(), { side: "left", variant: "sidebar", collapsible: "offcanvas", }); + + // Validate props at runtime + sidebarPropsSchema.parse(props)
24-24: Review attribute binding security.The direct binding of
$attrscould potentially expose internal properties. Consider implementing an allowlist of safe attributes:+ const safeAttrs = computed(() => { + const allowed = ['id', 'class', 'data-testid'] + return Object.fromEntries( + Object.entries($attrs).filter(([key]) => allowed.includes(key)) + ) + }) - v-bind="$attrs" + v-bind="safeAttrs"Also applies to: 80-80
frontend/components/ui/sheet/SheetContent.vue (3)
40-42: Verify z-index hierarchy for security.The z-index change from 50 to 40 could affect the modal stacking order. Ensure this doesn't create opportunities for clickjacking, especially when multiple dialogs or alerts are open.
Consider implementing a z-index management system to prevent UI-based security issues:
- Document z-index ranges for different component types
- Ensure critical UI elements (like security alerts) always appear on top
- Implement guards against overlapping interactive elements
43-51: Implement additional security measures for the dialog.While the current implementation looks good, consider enhancing security with these measures:
- Add
aria-modal="true"to prevent screen reader access to background content- Consider adding a backdrop click handler with confirmation for sensitive operations
- Implement focus trapping to prevent keyboard navigation outside the modal
1-54: Security Review: Overall ImplementationThe implementation looks secure with proper prop typing and safe template binding. However, here are some security recommendations:
- Consider adding Content Security Policy (CSP) headers to restrict inline styles
- Implement rate limiting for modal operations to prevent UI-based DoS
- Add event logging for security-sensitive modal operations
- Consider adding a timeout for modals containing sensitive information
frontend/components/Location/LegacySelector.vue (1)
48-62: Consider adding input validation for search field.The state management implementation is clean and secure. However, consider adding input validation for the search field to prevent potential injection attacks.
Add validation when setting the search value:
watch( () => value.value, () => { if (!value.value) { - form.value.search = ""; + form.value.search = validateSearchInput(""); } } ); + function validateSearchInput(input: string): string { + // Remove any potentially dangerous characters + return input.replace(/[<>'"]/g, ''); + }frontend/components/Location/Selector.vue (2)
16-16: Add aria-label to CommandInput for better accessibility.The search input should have an aria-label for screen readers.
- <CommandInput :placeholder="$t('components.location.selector.search_location')" /> + <CommandInput + :placeholder="$t('components.location.selector.search_location')" + :aria-label="$t('components.location.selector.search_location')" + />
61-63: Add debounce to search input for better performance.Consider debouncing the search input to prevent excessive re-renders when typing quickly.
const open = ref(false); - const search = ref(""); + const search = ref(""); + const debouncedSearch = refDebounced(search, 300); const id = useId();You'll need to add the following composable:
// composables/use-debounced-ref.ts import { ref, customRef } from 'vue' export function refDebounced<T>(value: T, delay = 200) { let timeout: NodeJS.Timeout return customRef((track, trigger) => { return { get() { track() return value }, set(newValue: T) { clearTimeout(timeout) timeout = setTimeout(() => { value = newValue trigger() }, delay) } } }) }frontend/components/Item/CreateModal.vue (2)
197-203: Improve error handling specificity.The error handling is generic and doesn't provide specific feedback about what went wrong. This could make troubleshooting difficult for users and developers.
Enhance error handling:
const { error, data } = await api.items.create(out); loading.value = false; if (error) { loading.value = false; - toast.error("Couldn't create item"); + toast.error(`Failed to create item: ${error.message || 'Unknown error'}`); return; }
229-232: Add loading state for navigation.The navigation after item creation doesn't account for potential network delays, which could lead to a race condition.
Add loading state for navigation:
if (close) { closeDialog("create-item"); + loading.value = true; navigateTo(`/item/${data.id}`); + loading.value = false; }
🛑 Comments failed to post (6)
frontend/components/Location/LegacySelector.vue (2)
11-13:
⚠️ Potential issueEnsure proper content sanitization for location names.
The direct rendering of
cast(item.value).namecould potentially lead to XSS vulnerabilities if the location names contain malicious HTML/JavaScript content.Consider using Vue's built-in content sanitization or implement a sanitization function:
- {{ cast(item.value).name }} + {{ sanitizeContent(cast(item.value).name) }}Add the sanitization function to your script section:
function sanitizeContent(content: string): string { // Use a library like DOMPurify or implement custom sanitization return DOMPurify.sanitize(content); }
38-43:
⚠️ Potential issueImprove type safety in casting function.
The current implementation using
anytype bypasses TypeScript's type checking mechanisms, which could lead to runtime errors and potential security vulnerabilities if malicious data is passed through.Consider implementing a type guard instead of direct casting:
- function cast(value: any): FlatTreeItem { - return value as FlatTreeItem; + function cast(value: unknown): FlatTreeItem { + if (!isFlatTreeItem(value)) { + throw new Error('Invalid FlatTreeItem'); + } + return value; + } + + function isFlatTreeItem(value: unknown): value is FlatTreeItem { + return ( + typeof value === 'object' && + value !== null && + 'name' in value && + 'treeString' in 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.// Cast the type of the item to a FlatTreeItem so we can get type "safety" in the template // Note that this does not actually change the type of the item, it just tells the compiler // that the type is FlatTreeItem. We must keep this in sync with the type of the items function cast(value: unknown): FlatTreeItem { if (!isFlatTreeItem(value)) { throw new Error('Invalid FlatTreeItem'); } return value; } function isFlatTreeItem(value: unknown): value is FlatTreeItem { return ( typeof value === 'object' && value !== null && 'name' in value && 'treeString' in value ); }frontend/components/Location/Selector.vue (2)
29-29:
⚠️ Potential issueSanitize location data to prevent XSS attacks.
Location names and tree strings are rendered directly in the template. While Vue automatically escapes HTML in templates, it's a good practice to explicitly sanitize user-provided data.
- {{ location.name }} + {{ $sanitize(location.name) }}- {{ location.treeString }} + {{ $sanitize(location.treeString) }}You'll need to add a sanitization utility. Consider using DOMPurify:
// lib/utils.ts import DOMPurify from 'dompurify'; export const sanitize = (str: string) => DOMPurify.sanitize(str); // Then register it as a global property in your Vue app app.config.globalProperties.$sanitize = sanitize;Also applies to: 32-32
67-74: 🛠️ Refactor suggestion
Add error handling for location updates.
The location selection logic should handle potential errors when updating the model value.
function selectLocation(location: LocationSummary) { + try { if (value.value?.id !== location.id) { value.value = location; } else { value.value = null; } open.value = false; + } catch (error) { + console.error('Failed to update location:', error); + toast.error($t('components.location.selector.update_failed')); + } }📝 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.function selectLocation(location: LocationSummary) { try { if (value.value?.id !== location.id) { value.value = location; } else { value.value = null; } open.value = false; } catch (error) { console.error('Failed to update location:', error); toast.error($t('components.location.selector.update_failed')); } }frontend/components/Item/CreateModal.vue (2)
153-155: 🛠️ Refactor suggestion
Re-enable focus management for better accessibility.
The focus management code is commented out, which could affect keyboard navigation and screen reader compatibility.
Uncomment and update the focus management code:
-// useTimeoutFn(() => { -// focused.value = true; -// }, 50); +useTimeoutFn(() => { + focused.value = true; + nameInput.value?.focus(); +}, 50); -// focused.value = false; +focused.value = false;Also applies to: 168-168
31-32:
⚠️ Potential issueAdd file size validation and sanitization for image uploads.
The file input accepts multiple image formats without size validation, which could lead to potential security risks:
- Large file uploads could cause DoS
- Malicious files could exploit image processing vulnerabilities
Add file size validation and sanitize uploaded images:
<Input id="image-create-photo" class="w-full" type="file" + :max-size="5242880" accept="image/png,image/jpeg,image/gif,image/avif,image/webp;capture=camera" @change="previewImage" />Add validation in the
previewImagefunction:function previewImage(event: Event) { const input = event.target as HTMLInputElement; if (input.files && input.files.length > 0) { + const file = input.files[0]; + if (file.size > 5242880) { + toast.error("File size must be less than 5MB"); + return; + } const reader = new FileReader();📝 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.<!-- Updated Input component --> <Input id="image-create-photo" class="w-full" type="file" :max-size="5242880" accept="image/png,image/jpeg,image/gif,image/avif,image/webp;capture=camera" @change="previewImage" /> <!-- Updated previewImage function --> function previewImage(event: Event) { const input = event.target as HTMLInputElement; if (input.files && input.files.length > 0) { const file = input.files[0]; if (file.size > 5242880) { toast.error("File size must be less than 5MB"); return; } const reader = new FileReader(); // Additional file processing logic... } }
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
🔭 Outside diff range comments (1)
frontend/components/Item/CreateModal.vue (1)
173-233: 🛠️ Refactor suggestionImprove error handling and transaction safety.
Consider these improvements:
- Use a transaction-like approach for item and photo creation
- Add specific error details in toast messages
- Consider cleanup on photo upload failure
async function create(close = true) { if (!form.location) { + toast.error("Location is required"); return; } if (loading.value) { toast.error("Already creating an item"); return; } loading.value = true; if (shift.value) { close = false; } + try { const out: ItemCreate = { parentId: null, name: form.name, description: form.description, locationId: form.location.id as string, labelIds: form.labels, }; const { error, data } = await api.items.create(out); if (error) { throw new Error(error.message); } if (form.photo) { const { error: photoError } = await api.items.attachments.add( data.id, form.photo, form.photo.name, AttachmentTypes.Photo ); if (photoError) { // Attempt to clean up the created item + await api.items.delete(data.id); throw new Error(photoError.message); } } toast.success("Item created successfully"); resetForm(); if (close) { closeDialog("create-item"); navigateTo(`/item/${data.id}`); } + } catch (error) { + toast.error(`Error: ${error.message}`); + } finally { loading.value = false; + } }
🧹 Nitpick comments (1)
frontend/components/Label/CreateModal.vue (1)
83-108: Enhance error handling and input validation.Consider these improvements:
- Add input validation before the API call
- Include specific error details from the API response in the error toast
- Consider sanitizing user input for security
async function create(close = true) { if (loading.value) { toast.error("Already creating a label"); return; } + // Validate input + if (!form.name.trim()) { + toast.error("Label name is required"); + return; + } loading.value = true; if (shift.value) { close = false; } const { error, data } = await api.labels.create(form); if (error) { - toast.error("Couldn't create label"); + toast.error(`Couldn't create label: ${error.message}`); loading.value = false; return; }
🛑 Comments failed to post (1)
frontend/components/Item/CreateModal.vue (1)
28-34:
⚠️ Potential issueAdd file upload restrictions for security.
The file input should include:
- Maximum file size limit
- Client-side image validation
- File type validation beyond just the accept attribute
<Input id="image-create-photo" class="w-full" type="file" + data-max-size="5242880" accept="image/png,image/jpeg,image/gif,image/avif,image/webp;capture=camera" - @change="previewImage" + @change="validateAndPreviewImage" />Add this validation function:
function validateAndPreviewImage(event: Event) { const input = event.target as HTMLInputElement; if (!input.files?.length) return; const file = input.files[0]; const maxSize = Number(input.dataset.maxSize) || 5242880; // 5MB default if (file.size > maxSize) { toast.error("File size exceeds 5MB limit"); input.value = ''; return; } if (!file.type.match('image.*')) { toast.error("Only image files are allowed"); input.value = ''; return; } previewImage(event); }
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: 7
🧹 Nitpick comments (3)
frontend/components/Label/Selector.vue (1)
41-41: Fix Tailwind CSS classnames order.The pipeline failure indicates an issue with the order of Tailwind CSS classnames.
Consider using a Tailwind CSS class sorter to maintain consistent ordering.
🧰 Tools
🪛 GitHub Actions: Pull Request CI
[warning] 41-41: Invalid Tailwind CSS classnames order tailwindcss/classnames-order
frontend/components/Location/Selector.vue (2)
31-31: Fix Tailwind class order.The class order violates the Tailwind CSS style guide.
Apply this diff to fix the class order:
- <div v-if="location.name !== location.treeString" class="text-muted-foreground mt-1 text-xs"> + <div v-if="location.name !== location.treeString" class="mt-1 text-xs text-muted-foreground">🧰 Tools
🪛 GitHub Actions: Pull Request CI
[warning] 31-31: Invalid Tailwind CSS classnames order tailwindcss/classnames-order
55-57: Add input validation for LocationSummary.The LocationSummary type is used without validation. Consider adding runtime validation to ensure data integrity.
Add validation using a schema validation library like Zod:
import { z } from 'zod'; const LocationSummarySchema = z.object({ id: z.string(), name: z.string(), treeString: z.string() }); // Add validation in selectLocation function function validateLocation(location: unknown): LocationSummary { return LocationSummarySchema.parse(location); }
🛑 Comments failed to post (7)
frontend/components/Label/Selector.vue (5)
10-10:
⚠️ Potential issueRemove non-null assertion operator for safer type checking.
The non-null assertion operator (
!) in the display-value prop could lead to runtime errors if a label is not found.-:display-value="v => props.labels.find(l => l.id === v)!.name" +:display-value="v => props.labels.find(l => l.id === v)?.name ?? 'Unknown Label'"📝 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.:display-value="v => props.labels.find(l => l.id === v)?.name ?? 'Unknown Label'"
51-51:
⚠️ Potential issueRemove console.log statement from production code.
Console logging in production code could potentially expose sensitive information.
-console.log(ev);📝 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.
95-98:
⚠️ Potential issueInitialize modelValue with an empty array instead of null.
Using null as the default value could lead to runtime errors when accessing array methods.
modelValue: { type: Array as () => string[], - default: null, + default: () => [], },📝 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.modelValue: { type: Array as () => string[], default: () => [], },
111-113:
⚠️ Potential issueImplement input sanitization for fuzzy search.
The fuzzysort library operates directly on user input. Consider sanitizing the search term to prevent potential XSS attacks.
+const sanitizeInput = (input: string) => input.replace(/[<>]/g, ''); const filtered = fuzzysort - .go(searchTerm.value, props.labels, { key: "name", all: true }) + .go(sanitizeInput(searchTerm.value), props.labels, { key: "name", all: true })📝 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 sanitizeInput = (input: string) => input.replace(/[<>]/g, ''); const filtered = fuzzysort .go(sanitizeInput(searchTerm.value), props.labels, { key: "name", all: true }) .map(l => ({
126-141:
⚠️ Potential issueImplement rate limiting and input validation for label creation.
The label creation endpoint needs protection against potential abuse:
- Add rate limiting to prevent rapid creation of labels
- Validate label name length and content
- Consider adding CSRF protection if not already present
const createAndAdd = async (name: string) => { + if (!name || name.length > 50 || !/^[a-zA-Z0-9\s-_]+$/.test(name)) { + toast.error("Invalid label name"); + return; + } const { error, data } = await api.labels.create({ name, color: "", // Future! description: "", });📝 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 createAndAdd = async (name: string) => { if (!name || name.length > 50 || !/^[a-zA-Z0-9\s-_]+$/.test(name)) { toast.error("Invalid label name"); return; } const { error, data } = await api.labels.create({ name, color: "", // Future! description: "", }); if (error) { toast.error("Couldn't create label"); return; } toast.success("Label created"); modelValue.value.push(data.id); };frontend/components/Location/Selector.vue (2)
29-30:
⚠️ Potential issueSanitize location data display.
The location name and treeString are rendered directly in the template. While Vue automatically escapes HTML content, it's a good practice to explicitly sanitize user-provided data.
Apply this diff to add sanitization:
- {{ location.name }} + {{ $sanitize(location.name) }} - {{ location.treeString }} + {{ $sanitize(location.treeString) }}Note: You'll need to implement or import a sanitization function.
Also applies to: 32-33
77-81: 🛠️ Refactor suggestion
Add search input protection.
The search functionality lacks rate limiting and input validation, which could lead to performance issues or DoS attacks.
Add debouncing and input validation:
+ import { debounce } from 'lodash-es'; + const MAX_SEARCH_LENGTH = 100; const filteredLocations = computed(() => { + if (search.value.length > MAX_SEARCH_LENGTH) { + search.value = search.value.slice(0, MAX_SEARCH_LENGTH); + } const filtered = fuzzysort.go(search.value, locations.value, { key: "name", all: true }).map(i => i.obj); return filtered; }); + // Add debounced search + const debouncedSearch = debounce((value: string) => { + search.value = value; + }, 300);📝 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.import { debounce } from 'lodash-es'; const MAX_SEARCH_LENGTH = 100; const filteredLocations = computed(() => { if (search.value.length > MAX_SEARCH_LENGTH) { search.value = search.value.slice(0, MAX_SEARCH_LENGTH); } const filtered = fuzzysort.go(search.value, locations.value, { key: "name", all: true }).map(i => i.obj); return filtered; }); // Add debounced search const debouncedSearch = debounce((value: string) => { search.value = value; }, 300);
… helps with issues with ui not working with larger labels, as it is only enforced on the frontend could be easily bypassed but thats a them problem
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: 8
🔭 Outside diff range comments (1)
frontend/components/Label/CreateModal.vue (1)
83-91: 🛠️ Refactor suggestionStrengthen input validation in the create function.
The length validation is good, but consider adding more robust validation.
async function create(close = true) { if (loading.value) { toast.error("Already creating a label"); return; } + // Validate for empty or whitespace-only input + if (!form.name.trim()) { + toast.error("Label name cannot be empty"); + return; + } + // Validate for special characters + if (!/^[a-zA-Z0-9\s-_]+$/.test(form.name)) { + toast.error("Label name contains invalid characters"); + return; + } if (form.name.length > 50) { toast.error("Label name must not be longer than 50 characters"); return; }
🧹 Nitpick comments (1)
frontend/components/Label/CreateModal.vue (1)
23-30: Add loading state visual feedback.The button's disabled state is set correctly, but users need visual feedback during the loading state.
<ButtonGroup> - <Button :disabled="loading" type="submit">{{ $t("global.create") }}</Button> + <Button :disabled="loading" type="submit"> + <Spinner v-if="loading" class="mr-2 h-4 w-4" /> + {{ $t("global.create") }} + </Button> <Button variant="outline" :disabled="loading" type="button" @click="create(false)">{{ $t("global.create_and_add") }}</Button> </ButtonGroup>
🛑 Comments failed to post (8)
frontend/components/Label/Selector.vue (5)
51-51:
⚠️ Potential issueRemove console.log and fix create-item functionality.
The code contains a console.log statement and a TODO comment about broken create-item functionality.
-console.log(ev); -// TODO: this breaks everything, fix create-itemPlease fix the create-item functionality and remove the console.log statement to prevent potential information leakage in production.
Also applies to: 53-53
95-98:
⚠️ Potential issueFix modelValue prop type and default value mismatch.
The modelValue prop's default value should be an empty array to match its type definition.
modelValue: { type: Array as () => string[], - default: null, + default: () => [], },📝 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.modelValue: { type: Array as () => string[], default: () => [], },
10-10:
⚠️ Potential issueRemove non-null assertion operator for safer type checking.
The display-value prop uses a non-null assertion (
!) which could lead to runtime errors if a label is not found.-:display-value="v => props.labels.find(l => l.id === v)!.name" +:display-value="v => props.labels.find(l => l.id === v)?.name ?? 'Unknown'"📝 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.:display-value="v => props.labels.find(l => l.id === v)?.name ?? 'Unknown'"
41-41:
⚠️ Potential issueFix Tailwind CSS class order.
The pipeline reports an invalid order of Tailwind CSS classes.
-class="bg-popover text-popover-foreground data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2 mt-2 w-[--radix-popper-anchor-width] rounded-md border shadow-md outline-none" +class="mt-2 w-[--radix-popper-anchor-width] rounded-md border bg-popover text-popover-foreground shadow-md outline-none data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2"📝 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.class="mt-2 w-[--radix-popper-anchor-width] rounded-md border bg-popover text-popover-foreground shadow-md outline-none data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2"🧰 Tools
🪛 GitHub Actions: Pull Request CI
[warning] 41-41: Invalid Tailwind CSS classnames order tailwindcss/classnames-order
126-144:
⚠️ Potential issueEnhance security measures for label creation.
The label creation logic needs additional security measures:
- Input sanitization for the label name
- Rate limiting for label creation
- Validation for empty color and description fields
const createAndAdd = async (name: string) => { + // Sanitize input + name = name.trim(); + if (!/^[a-zA-Z0-9\s-_]+$/.test(name)) { + toast.error("Label name contains invalid characters"); + return; + } if (name.length > 50) { toast.error("Label name must not be longer than 50 characters"); return; } + if (name.length === 0) { + toast.error("Label name cannot be empty"); + return; + } const { error, data } = await api.labels.create({ name, - color: "", // Future! - description: "", + color: "#000000", // Default color + description: "Created via label selector", });📝 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 createAndAdd = async (name: string) => { // Sanitize input name = name.trim(); if (!/^[a-zA-Z0-9\s-_]+$/.test(name)) { toast.error("Label name contains invalid characters"); return; } if (name.length > 50) { toast.error("Label name must not be longer than 50 characters"); return; } if (name.length === 0) { toast.error("Label name cannot be empty"); return; } const { error, data } = await api.labels.create({ name, color: "#000000", // Default color description: "Created via label selector", }); if (error) { toast.error("Couldn't create label"); return; } toast.success("Label created"); modelValue.value.push(data.id); };frontend/components/Label/CreateModal.vue (3)
8-22:
⚠️ Potential issueEnhance input validation for better security.
While the form includes length validation, consider adding these security measures:
- Input sanitization to prevent XSS attacks
- Content-type validation to ensure only text is submitted
- Rate limiting for form submissions
Add input sanitization before form submission:
<form class="flex flex-col gap-2" @submit.prevent="create()"> <FormTextField ref="locationNameRef" v-model="form.name" :trigger-focus="focused" :autofocus="true" :label="$t('components.label.create_modal.label_name')" :max-length="50" :min-length="1" + @input="sanitizeInput" /> <FormTextArea v-model="form.description" :label="$t('components.label.create_modal.label_description')" :max-length="255" + @input="sanitizeInput" />Add the sanitization function:
function sanitizeInput(event: Event) { const input = event.target as HTMLInputElement; input.value = input.value.replace(/[<>]/g, ''); }
109-112:
⚠️ Potential issueSecure the navigation flow.
The current navigation implementation might be vulnerable to open redirect if the
data.idis manipulated.if (close) { closeDialog("create-label"); - navigateTo(`/label/${data.id}`); + // Validate id format before navigation + if (typeof data.id === 'number' && Number.isInteger(data.id) && data.id > 0) { + navigateTo(`/label/${data.id}`); + } else { + toast.error("Invalid label ID received"); + } }📝 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.if (close) { closeDialog("create-label"); // Validate id format before navigation if (typeof data.id === 'number' && Number.isInteger(data.id) && data.id > 0) { navigateTo(`/label/${data.id}`); } else { toast.error("Invalid label ID received"); } }
68-77: 🛠️ Refactor suggestion
Re-enable focus management for accessibility.
The commented-out watch function affects keyboard accessibility. Consider re-implementing it using the new Dialog component's API.
+onMounted(() => { + watch( + () => isDialogOpen("create-label"), + (open) => { + if (open) { + useTimeoutFn(() => { + focused.value = true; + }, 50); + } else { + focused.value = false; + } + } + ); +});📝 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.onMounted(() => { watch( () => isDialogOpen("create-label"), (open) => { if (open) { useTimeoutFn(() => { focused.value = true; }, 50); } else { focused.value = false; } } ); });
There was a problem hiding this comment.
You'll want to setup validation restrictions on length in the Go code as well, otherwise someone using the API could make labels stupid long.
You'll probably have to run a db migration as well after changing this. Should be task db:migrate -- <Name> if I'm remembering correctly.
In regards to alerts, we should probably try to at least have some color for success vs error. Just as a general visual indicator.
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
🧹 Nitpick comments (2)
frontend/pages/testing.vue (2)
12-19: Adjust toast duration for testing.The test toast duration is set to 100 seconds (100,000ms), which is unusually long and might not represent real-world usage.
- duration: 100000, + duration: 5000, // 5 seconds is a more typical duration
27-30: Enhance keyboard shortcut accessibility.Consider adding aria-label to improve screen reader support for the keyboard shortcut instructions.
- <span class="flex items-center gap-1 text-sm"> + <span class="flex items-center gap-1 text-sm" aria-label="Keyboard shortcut instructions">
🛑 Comments failed to post (2)
frontend/pages/testing.vue (2)
14-17:
⚠️ Potential issueAdd error handling for toast notifications.
Implement try-catch blocks around toast notifications to handle potential failures gracefully.
@click=" + try { toast.success('This is a test', { duration: 100000, }) + } catch (error) { + console.error('Failed to show notification:', error); + } "Also applies to: 21-24
20-26: 🛠️ Refactor suggestion
Remove or implement handler for empty button.
Button 3 lacks an onClick handler and appears to be a placeholder.
- <Button>Button 3</Button> + <Button @click="toast.info('Button 3 clicked')">Button 3</Button>📝 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.<ButtonGroup> <Button @click="toast.success('Success')">Success</Button> <Button variant="outline" @click="toast.info('Info')">Info</Button> <Button variant="destructive" @click="toast.warning('Warning')">Warning</Button> <Button variant="destructive" @click="toast.error('Error')">Error</Button> <Button @click="toast.info('Button 3 clicked')">Button 3</Button> </ButtonGroup>
Would changing the db not cause issues if someone already has a longer name? Might be better to either have the api error? Also added coloured alerts. |
|
We'll have to test the DB thing, I know that in full databases it can be changed to be smaller without too many issues. SQLite however is probably going to be a PITA. |
What type of PR is this?
What this PR does / why we need it:
This continues the work from #492 and aims to switch the layout to be purely shadcn (excluding colours).
Changes
before
after
before
after
Screencast from 2025-02-02 12-42-15.webm
Not sure if we want to keep the colours for error and success?
before
after
Summary by CodeRabbit
Release Notes
New Features
• Users now receive consistent toast notifications for errors and successes.
• Dialogs and modals have been updated for clearer alerts, improved accessibility, and smoother interactions.
• A quick-access menu with keyboard shortcuts has been added to speed up navigation and actions.
• Additional UI elements—including refined badges, labels, and tag inputs—enhance overall presentation.
• A new tags input interface allows users to select frameworks easily.
• The introduction of a popover component improves location selection functionality.
• A new alert dialog structure enhances user feedback during critical actions.
• New components such as
ButtonGroup,Label,Textarea,Popover,TagsInput,DialogProvider,Sonner,LegacySelector, andAlertDialoghave been introduced to enhance UI flexibility.Refactor
• Legacy components were replaced with modern, cohesive implementations for a more unified experience.
• The notification handling has been transitioned from a custom notifier to a centralized toast notification system.
• The structure of various modal and dialog components has been improved for better organization and usability.
Chores
• Dependency updates and cleanup of outdated notifier utilities improve internal efficiency.
• Removed unused components and streamlined the codebase for better maintainability.
• Updated localization strings to enhance user guidance and interface clarity.