fix(nc-gui): form view / attachment / currency issues #10168
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces modifications across several Vue components in the Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/nc-gui/components/cell/attachment/Modal.vue (2)
23-23: Use consistent naming to enhance clarity.
The addition ofrenameFileInlineis clear but ensure it aligns with the naming convention used for the other functions in this file (e.g.,onDrop,removeFile).
132-132: Nice toggle for new attachment modal.
The@click="isNewAttachmentModalOpen = true"pattern is straightforward. Keep in mind to handle closing states as well (if needed).packages/nc-gui/components/smartsheet/column/CurrencyOptions.vue (2)
55-55: Consider whethershallowRefis necessary here.
Using ashallowReffor aMapis fine if you only need reactivity when the reference itself changes (e.g., a completely new map). If the map’s internal changes also need to trigger reactivity, consider using arefwith a reactive approach.
65-68: Optional chaining is safe, but consider edge cases.
This logic handles missing map entries gracefully using optional chaining. However, if there's any possibility of an empty or non-string input, you might validate or trim the input to avoid unexpected search behavior. Overall, looks good for normal usage of currency filtering.packages/nc-gui/components/cell/attachment/utils.ts (2)
300-308: **Consider unifying rename logic. **
renameFileInlineandrenameFileare conceptually similar, only differing in user interaction (dialog vs. direct update). Factor out shared logic to reduce code duplication and make it easier to maintain.
354-410: **Robust bulk download implementation with room for improvement. **
This newbulkDownloadAttachmentsmethod handles multiple and single-file scenarios well. A few notes:
- If one of the files fails to download, the process continues for the rest without explicit user feedback about partial failures. Consider clarifying the behavior or introducing partial-failure notices.
- The function is large; some parts (e.g., error checks) could be extracted for better readability and testability.
- The usage of
window.open(zipURL, '_self')might impede user experience if the browser blocks pop-ups. Consider a user-triggered download approach if feasible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/nc-gui/components/cell/DateTimePicker.vue(1 hunks)packages/nc-gui/components/cell/attachment/AttachFile.vue(2 hunks)packages/nc-gui/components/cell/attachment/Carousel.vue(2 hunks)packages/nc-gui/components/cell/attachment/Modal.vue(9 hunks)packages/nc-gui/components/cell/attachment/UploadProviders/Camera.vue(2 hunks)packages/nc-gui/components/cell/attachment/UploadProviders/Local.vue(3 hunks)packages/nc-gui/components/cell/attachment/utils.ts(6 hunks)packages/nc-gui/components/smartsheet/column/CurrencyOptions.vue(3 hunks)
🔇 Additional comments (34)
packages/nc-gui/components/cell/attachment/AttachFile.vue (2)
8-9: Use ofisMobileModefor responsive layouts looks good.
This addition effectively introduces a device-responsive adaptation, ensuring a better user experience on mobile devices.
51-53: Consider verifying horizontal scrolling usability.
WhenisMobileModeis true,overflow-x-scrollcan cause horizontal scrolling within the menu. Verify that this does not introduce unwanted scrollbars at smaller breakpoints. Otherwise, you may want to conditionally constrain or hide scrollbars.packages/nc-gui/components/cell/attachment/UploadProviders/Camera.vue (2)
11-12: Good consistency with the mobile-first approach.
IntroducingisMobileModehere aligns with the other components, improving the overall mobile responsiveness.
111-112: Progressive enhancement for smaller devices.
Using a narrower canvas width for mobile devices is a sensible approach. Just confirm that the image capture functionality remains consistent (e.g., correct aspect ratio) when the video dimensions differ from the canvas width.packages/nc-gui/components/cell/DateTimePicker.vue (1)
497-497: Revisiting thereadonlycondition.
RemovingisMobileModefrom:readonlyallows mobile users to edit date/time fields ifisColDisabledis false. Ensure that this won’t inadvertently allow editing in contexts where your UI previously intended to be read-only on mobile devices.packages/nc-gui/components/cell/attachment/Modal.vue (13)
64-65: Good addition for new attachment modal.
IntroducingisNewAttachmentModalOpenhelps to clearly manage state for opening/closing the modal. Keep it consistent with your naming pattern (e.g.,modalVisible).
66-69: Successful introduction of rename tracking state.
isRenamingFile,renameFileIdx, andnewTitleeffectively separate concerns for renaming. This design choice helps readability and simpler debugging.
70-71: Confirm presence of multiple input boxes.
inputBoxis assigned viaref(), then accessed asinputBox.value[0]in subsequent code. Ensure you won’t have multiple input references or unintended array usage.Consider using a single-element ref or multiple named refs if needed.
72-81: Neat approach to start the rename process.
The function sets up all relevant state and focuses on the input box.
82-87: Reset function is straightforward.
handleResetFileRenameensures all reactive states are cleared. This keeps the rename flow clean.
88-99: Handle rename with robust error handling.
handleFileRenameproperly guards early withif (!isRenamingFile.value) return, and throws an error in case of rename failure. Nicely done!
111-120: Enhanced layout for bulk download.
The new flex container and extra button for bulk downloading attachments improve the user’s workflow. Ensure the button remains discoverable in smaller viewports.
167-167: Checkbox usage for selection.
Toggling thecheckedproperty helps users select attachments. Confirm that you handle edge cases (e.g., selecting none or all attachments).
204-204: Inline rename on double-click.
CallinghandleFileRenameStart(i)during@dblclick.stopis a smooth UX approach, allowing quick rename actions.
207-224: Conditional rendering of tooltip vs. inline edit.
Rendering an<input>only whenisRenamingFile && renameFileIdx === iprevents conflicts. However, check that truncation logic underNcTooltipdoesn’t interfere with user input or display issues.
244-244: Renaming button triggers rename flow properly.
This nicely aligns with the double-click rename mechanism. Confirm consistent usage of both rename triggers for user-friendly flows.
256-256: Consistent file removal.
removeFile(i)remains a clear, index-based approach. If implementing reordering, confirm indices remain accurate upon item removal.
272-273: New attachment component.
<LazyCellAttachmentAttachFile>being conditionally displayed byisNewAttachmentModalOpenis a good modular approach. Ensure any required on-close logic reverts state properly.packages/nc-gui/components/smartsheet/column/CurrencyOptions.vue (1)
79-81: Confirm handling of potential duplicate currency locales.
Ifl.valuecan be duplicated, repeated calls to.set()will overwrite the map entry. Ensure that reusing the latest entry is the intended behavior. If this is a non-issue, everything looks good here.packages/nc-gui/components/cell/attachment/UploadProviders/Local.vue (5)
9-9: Leverage ofisMobileModefor responsive design is good.Importing
isMobileModefromuseGlobal()cleanly integrates device awareness into this component. This is a straightforward and effective approach to apply mobile-specific styles or behaviors.
10-10: No concerns on this spacing line.
121-129: Consider text semantics to maintain accessibility.Using
<span>instead of<h1>leads to more semantic consistency here, since the text is supplemental rather than a top-level heading. This change helps screen readers prioritize relevant content.
140-140: Verify grid layout on smaller devices.By setting a fixed grid with 4 columns (
grid-cols-4), check if the layout might cause unintended overflow on narrow screens. Consider making the grid responsive by adding breakpoints or using auto-fill if needed.
146-146: Great use of conditional sizes for the file icon.Toggling the icon size based on
isMobileModeprovides a clear visual adjustment on smaller screens. This logic is simple and improves usability for mobile users.packages/nc-gui/components/cell/attachment/utils.ts (5)
4-4: **New import for ZIP file downloads. **
This import introduces thedownloadZipfunction to implement bulk attachment downloads as a ZIP file, which is a welcome enhancement for user convenience.
285-285: **Cloning the attachment reference. **
Creating a new object with the spread operator helps avoid side effects on the original reference. This is a good practice for reliably updating reactive data.
349-349: **Legacy download method. **
bulkDownloadAttachmentsLegacyremains in the code. If this approach is deprecated, consider removing or marking it as such to avoid confusion down the line.
498-498: **Exporting inline rename. **
ExposingrenameFileInlinefor use in other components is consistent with the earlier function addition and ensures consistent naming. Looks good.
271-271: 🛠️ Refactor suggestion**Potential out-of-bounds risk. **
Usingattachments.value[idx]!relies on the assumption thatidxis valid. Consider adding a guard check or verifyingidxis within range to avoid runtime errors.Would you like me to generate a quick script to confirm that no out-of-range index is ever used for
attachments.valuethroughout the codebase?packages/nc-gui/components/cell/attachment/Carousel.vue (5)
199-199: **Consistentmimetypeusage for image checks. **
Usingitem.mimetypeinstead ofmimeTypealigns with the rest of the codebase, ensuring uniform property naming for attachment type checks.
209-209: **Consistentmimetypeusage for video checks. **
Switching toitem.mimetypekeeps the naming consistent across different file-type checks.
211-211: **Dynamic prop binding for video MIME type. **
The:mime-type="item.mimetype"syntax is helpful for clarity, keeping the logic consistent with the rest of the code.
217-217: **Consistentmimetypeusage for PDF checks. **
As with images and videos, usingmimetypeavoids confusion from diverging property names.
223-223: **Consistentmimetypeusage for office file checks. **
This final rename toitem.mimetypecompletes the uniform approach for all file-type checks in the carousel.
f6b7e2d to
42ed5b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/nc-gui/components/cell/attachment/Modal.vue (3)
88-99: Enhance error handling and validation in file rename operationWhile the error handling is present, there are a few improvements that could be made:
- Add validation for the new title (e.g., check for invalid characters, max length)
- Provide more specific error messages to help users understand what went wrong
- Consider not throwing the error after showing the error message
Here's a suggested improvement:
const handleFileRename = async () => { if (!isRenamingFile.value) return if (newTitle.value) { + const isValid = /^[^<>:"/\\|?*\x00-\x1F]+$/.test(newTitle.value) + if (!isValid) { + message.error('File name contains invalid characters') + return + } + try { await renameFileInline(renameFileIdx.value, newTitle.value) handleResetFileRename() } catch (e) { - message.error('Error while renaming file') - throw e + message.error(e.message || 'Error while renaming file') + console.error('File rename error:', e) } } }
115-120: Consider debouncing the bulk download actionThe bulk download button could benefit from debouncing to prevent accidental double-clicks that might trigger multiple downloads.
<NcButton :disabled="!selectedVisibleItems.some((v) => !!v)" type="secondary" size="small" - @click="bulkDownloadAttachments" + @click="_.debounce(bulkDownloadAttachments, 300)" >
167-167: Improve checkbox transition and file name interactionThe checkbox opacity transition and file name interaction could be enhanced:
- The checkbox opacity transition could be smoother
- The file name container could benefit from a subtle hover effect
-class="nc-attachment-checkbox absolute top-2 left-2 group-hover:(opacity-100) opacity-0 z-50" +class="nc-attachment-checkbox absolute top-2 left-2 group-hover:(opacity-100) opacity-0 z-50 transition-opacity duration-200" -class="flex w-full text-[12px] items-center text-gray-700 cursor-default h-5" +class="flex w-full text-[12px] items-center text-gray-700 cursor-default h-5 hover:bg-gray-50 rounded"Also applies to: 205-235
packages/nc-gui/components/cell/attachment/utils.ts (2)
271-272: Add explicit null check for safer array accessUsing non-null assertion (
!) might hide potential runtime errors. Consider adding an explicit null check.- attachments.value[idx]!.title = title + if (!attachments.value[idx]) { + console.error('Attachment not found at index:', idx) + return + } + attachments.value[idx].title = title updateModelValue(attachments.value)
350-411: Enhance bulk download implementation for better UX and reliabilityWhile the implementation is functional, consider these improvements:
- Use more user-friendly error messages instead of generic "Failed to download file"
- Add loading state during download process
- Ensure URL cleanup happens even if error occurs
- Add progress indicator for large downloads
async function bulkDownloadAttachments() { + const loadingKey = 'bulk-download' + try { + message.loading({ content: 'Preparing files for download...', key: loadingKey }) const items: AttachmentType[] = selectedVisibleItems.value .map((v, i) => (v ? visibleItems.value[i] : undefined)) .filter(Boolean) if (items.length === 0) return if (items.length === 1) { return downloadAttachment(items[0]!) } if (!meta.value || !column.value) return const modelId = meta.value.id const columnId = column.value.id const rowId = extractPkFromRow(unref(row).row, meta.value.columns!) if (!modelId || !columnId || !rowId) { - console.error('Missing modelId, columnId or rowId') - message.error('Failed to download file') + throw new Error('Required identifiers are missing') } + let downloadedCount = 0 const filePromises = items.map(async (item) => { const src = item.url || item.path if (!src) { - console.error('Missing src') - message.error('Failed to download file') + console.error(`Missing source URL for file: ${item.title}`) return undefined } const apiPromise = isPublic.value ? () => fetchSharedViewAttachment(columnId!, rowId!, src) : () => $api.dbDataTableRow.attachmentDownload(modelId!, columnId!, rowId!, { urlOrPath: src, }) const res = await apiPromise() if (!res) { - console.error('Invalid response') - message.error('Failed to download file') + console.error(`Failed to fetch file: ${item.title}`) return undefined } let file: Response if (res.path) { file = await fetch(`${baseURL}/${res.path}`) } else if (res.url) { file = await fetch(`${res.url}`) } else { - console.error('Invalid blob response') - message.error('Failed to download file') + console.error(`Invalid response format for file: ${item.title}`) return undefined } + downloadedCount++ + message.loading({ + content: `Preparing files for download (${downloadedCount}/${items.length})...`, + key: loadingKey, + }) return file }) const files = (await Promise.all(filePromises)).filter(Boolean) as Response[] + if (files.length === 0) { + throw new Error('No files could be prepared for download') + } + message.loading({ content: 'Creating zip file...', key: loadingKey }) const zipBlob = await downloadZip(files).blob() const zipURL = URL.createObjectURL(zipBlob) - try { - window.open(zipURL, '_self') - } catch (e) { - console.error('Error opening blob window', e) - message.error('Failed to download file') - return undefined - } finally { - setTimeout(() => URL.revokeObjectURL(zipURL), 1000) - } + window.open(zipURL, '_self') + message.success({ content: 'Download started!', key: loadingKey }) + } catch (e) { + console.error('Bulk download error:', e) + message.error({ + content: `Download failed: ${e instanceof Error ? e.message : 'Unknown error'}`, + key: loadingKey, + }) + } finally { + if (zipURL) { + setTimeout(() => URL.revokeObjectURL(zipURL), 1000) + } + } }packages/nc-gui/components/cell/DatePicker.vue (1)
Line range hint
338-347: Consider enhancing accessibility attributes.The input field could benefit from additional ARIA attributes to improve screen reader support.
Consider adding these accessibility enhancements:
<input ref="datePickerRef" type="text" :value="localState?.format(dateFormat) ?? ''" :placeholder="placeholder" class="nc-date-input border-none outline-none !text-current bg-transparent !focus:(border-none outline-none ring-transparent)" :readonly="readOnly" + :aria-label="t('general.datePicker')" + :aria-invalid="isDateInvalid" + role="textbox" + aria-haspopup="true" @blur="onBlur" @focus="onFocus" @keydown="handleKeydown($event, open)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/nc-gui/components/cell/DatePicker.vue(2 hunks)packages/nc-gui/components/cell/DateTimePicker.vue(3 hunks)packages/nc-gui/components/cell/TimePicker.vue(2 hunks)packages/nc-gui/components/cell/YearPicker.vue(2 hunks)packages/nc-gui/components/cell/attachment/AttachFile.vue(2 hunks)packages/nc-gui/components/cell/attachment/Carousel.vue(4 hunks)packages/nc-gui/components/cell/attachment/Modal.vue(10 hunks)packages/nc-gui/components/cell/attachment/UploadProviders/Camera.vue(2 hunks)packages/nc-gui/components/cell/attachment/UploadProviders/Local.vue(3 hunks)packages/nc-gui/components/cell/attachment/utils.ts(6 hunks)packages/nc-gui/components/smartsheet/column/CurrencyOptions.vue(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/nc-gui/components/cell/attachment/UploadProviders/Local.vue
- packages/nc-gui/components/cell/attachment/AttachFile.vue
- packages/nc-gui/components/cell/attachment/UploadProviders/Camera.vue
- packages/nc-gui/components/cell/DateTimePicker.vue
- packages/nc-gui/components/smartsheet/column/CurrencyOptions.vue
- packages/nc-gui/components/cell/attachment/Carousel.vue
🔇 Additional comments (9)
packages/nc-gui/components/cell/TimePicker.vue (2)
16-16: Removal ofisMobileModereference looks sound.Confirm that no other parts of the UI rely on
isMobileModefor read-only logic.
342-342: Consistent usage of read-only property.Switching to
:readonly="readOnly"cleans up the logic, but be sure that mobile-specific requirements are still met elsewhere.packages/nc-gui/components/cell/attachment/Modal.vue (2)
66-70: LGTM: Clean state management setupThe reactive references for managing file renaming state are well-organized and follow Vue's best practices.
298-298: LGTM: Consistent attachment item sizingThe fixed height ensures consistent layout across different attachment types.
packages/nc-gui/components/cell/YearPicker.vue (2)
14-14: Consistent removal ofisMobileMode.
RemovingisMobileModeusage fromuseGlobal()is consistent with the overall refactor across Date and Time Picker components. This simplification streamlines the code and aligns well with the new approach of relying solely on thereadOnlystate for determining the input’s read-only status.
301-301: Simplified conditions for:readonly.
By depending only onreadOnly, you ensure a clearer, more predictable UX. Previously, mixingisMobileModecould introduce complex conditionals. This refined approach should make maintenance and enhancements easier going forward.packages/nc-gui/components/cell/attachment/utils.ts (1)
300-308: LGTM! Clean implementation of inline renamingThe implementation correctly handles both the attachment title update and selected file state management.
packages/nc-gui/components/cell/DatePicker.vue (2)
17-17: Verify the impact of removing isMobileMode.The removal of
isMobileModefromuseGlobal()appears to be part of a larger refactoring effort across multiple components.Let's analyze the impact across the codebase:
339-339: Verify mobile functionality after removing mobile-specific handling.The simplified
readonlybinding might affect the component's behavior on mobile devices. Previously, the mobile mode likely triggered the native date picker, which provides a better touch interface.Please test the following scenarios on mobile devices:
- Touch interaction with the date picker
- Native date picker functionality
- Keyboard behavior when using external keyboard on mobile
| @@ -1,6 +1,7 @@ | |||
| import type { AttachmentReqType, AttachmentType } from 'nocodb-sdk' | |||
| import { populateUniqueFileName } from 'nocodb-sdk' | |||
| import DOMPurify from 'isomorphic-dompurify' | |||
| import { downloadZip } from 'client-zip' | |||
There was a problem hiding this comment.
💡 Codebase verification
Missing dependency: client-zip package is not installed
The client-zip package is not listed in any of the project's package.json files, including the main package.json and the package.json files in the packages directory. This dependency needs to be added to packages/nc-gui/package.json since it's being used in the GUI component.
- Add
client-zipas a dependency inpackages/nc-gui/package.json - Run package manager install command to install the dependency
🔗 Analysis chain
Verify client-zip package dependency
Please ensure the client-zip package is listed in the project's dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if client-zip is properly listed in package.json
cat package.json | jq '.dependencies["client-zip"] // .devDependencies["client-zip"]'
Length of output: 92
Script:
#!/bin/bash
# Check if client-zip is listed in any package.json files in the repository
fd package.json --exec sh -c 'echo "=== {} ==="; cat {} | jq ".dependencies[\"client-zip\"] // .devDependencies[\"client-zip\"]"'
Length of output: 636
42ed5b0 to
428eee6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (18)
packages/nc-gui/components/cell/attachment/Modal.vue (10)
66-68: Use consistent naming for renaming states.Currently, you have
isRenamingFile,renameFileIdx, andnewTitle. Ensure variable names clearly convey intent and match the domain. For instance, renamingrenameFileIdxtorenamingFileIndexor something more descriptive can help with readability.
72-80: Improve accessibility for file rename input focus.In
handleFileRenameStart, consider adding checks to ensure the element is present before callingfocus()andselect(). This prevents runtime errors in edge cases where the element might not be rendered yet or if the ref is null.nextTick().then(() => { - inputBox.value[0].focus() - inputBox.value[0].select() + if (inputBox.value?.[0]) { + inputBox.value[0].focus() + inputBox.value[0].select() + } })
82-86: Gracefully handle pending rename states.
handleResetFileRenameclears the renaming state. Consider whether the user might need to confirm cancellation or be warned of unsaved changes if they've already typed something. This is especially relevant in multi-step or synchronous rename flows.
88-99: Enhance error feedback.In
handleFileRename, you currently display a generic error message “Error while renaming file”. Including the original error message or other relevant details can help users or developers troubleshoot rename failures more quickly.
111-120: Consider grouping these button sets for more robust UI structure.There are multiple download and attach file buttons in this header. Maintaining consistent spacing, grouping, or theming can improve user experience, especially in smaller breakpoints or for accessibility.
132-132: Provide user feedback upon opening the new attachment modal.When setting
isNewAttachmentModalOpen = true, consider whether the user should be notified or guided (e.g., focusing on key UI elements, clarifying instructions, etc.).
167-167: Improve discoverability of multi-select usage.A tooltip or short textual guide could inform users that they can select multiple attachments for bulk actions. This helps avoid confusion if the user is unaware of the multi-select feature.
205-230: Avoid forcing rename mode for every file on double-click.
@dblclick.stop="handleFileRenameStart(i)"is an intuitive approach, but consider user feedback for accidental double-clicks on attachments. Possibly confirm user intent or allow turning off inline rename if needed.
233-235: Harmonize rename vs. remove UI states.The rename and remove actions share a small area. Consider spacing or a slight layout tweak to reduce accidental clicks and keep the UI visually balanced.
298-298: Consistent application of SCSS utility classes.In the
.nc-attachment-modal .nc-attachment-itemblock, ensure that tailwind/utility classes are consistently used. Some classes may conflict with others. Confirm the intention (height fixed to 200px, etc.) aligns with other design components.packages/nc-gui/components/cell/attachment/utils.ts (8)
4-4: Confirm usage offflateZip.
zip as fflateZipis imported. Verify that all references tozipor archive creation in the code are updated to usefflateZip. Consider consistent naming to avoid confusion (e.g.,import { zip as fflateZip } from 'fflate'→import { zip as createZip } from 'fflate').
271-271: Use optional chaining.
attachments.value[idx]!forcibly asserts non-nullness. Consider employing optional chaining to avoid potential runtime errors ifidxis out of bounds. This fosters safer code.- attachments.value[idx]!.title = title + attachments.value[idx]?.title = title
300-309: Document the inline rename approach.
renameFileInlinedirectly modifiesattachments.value[idx]and optionally updatesselectedFile. Add comments explaining the difference between inline rename and the modal-based approach, clarifying potential side effects or concurrency issues.
350-358: Prompt user on zero or single attachments.When
items.length === 0oritems.length === 1, the logic early-returns for either skipping or downloading a single file. Consider confirming user intent if the user meant to bulk-download but only ended up with one selected item, or indicating that only one file was downloaded. This can reduce user confusion.
371-377: Improve error messaging for missing src.When
!src, you log an error and show “Failed to download file.” Possibly improve the message to clarify that a valid file source is required for the download.
379-385: Extract repeated fetch logic into a helper.
apiPromiseuses eitherfetchSharedViewAttachment(...)or$api.dbDataTableRow.attachmentDownload(...). Merging the repeated logic into a single method could simplify code and centralize error handling for multiple scenarios.
418-421: Structure the zip object for clarity.The zip object is being built from a loop-based approach. For large numbers of files, consider using a more descriptive structure or a specialized class to store file metadata. This can ease debugging if conflicts arise with repeated file names.
426-454: Consider streaming for large file sets.If users might bulk-download large sets of attachments, consider streaming the zip creation or using a web worker. Large in-memory assemblies could degrade performance or cause memory constraints in certain environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/nc-gui/components/cell/DatePicker.vue(2 hunks)packages/nc-gui/components/cell/DateTimePicker.vue(3 hunks)packages/nc-gui/components/cell/TimePicker.vue(2 hunks)packages/nc-gui/components/cell/YearPicker.vue(2 hunks)packages/nc-gui/components/cell/attachment/AttachFile.vue(2 hunks)packages/nc-gui/components/cell/attachment/Carousel.vue(4 hunks)packages/nc-gui/components/cell/attachment/Modal.vue(10 hunks)packages/nc-gui/components/cell/attachment/UploadProviders/Camera.vue(2 hunks)packages/nc-gui/components/cell/attachment/UploadProviders/Local.vue(3 hunks)packages/nc-gui/components/cell/attachment/utils.ts(6 hunks)packages/nc-gui/components/smartsheet/column/CurrencyOptions.vue(2 hunks)packages/nc-gui/nuxt.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/nc-gui/components/cell/DateTimePicker.vue
- packages/nc-gui/components/cell/attachment/Carousel.vue
- packages/nc-gui/components/cell/attachment/UploadProviders/Camera.vue
- packages/nc-gui/components/cell/DatePicker.vue
- packages/nc-gui/components/cell/TimePicker.vue
- packages/nc-gui/components/cell/YearPicker.vue
- packages/nc-gui/components/cell/attachment/UploadProviders/Local.vue
- packages/nc-gui/components/cell/attachment/AttachFile.vue
- packages/nc-gui/components/smartsheet/column/CurrencyOptions.vue
🔇 Additional comments (4)
packages/nc-gui/components/cell/attachment/Modal.vue (2)
64-65: Clarify the attachment modal logic.
isNewAttachmentModalOpenis introduced but it’s not immediately clear if this should also handle form validation or additional actions when toggling the attachment modal. Consider verifying any interplay betweenisNewAttachmentModalOpenandisRenamingFileto prevent conflicting states or UI confusion.
70-70: Confirm the input reference type.
inputBoxis a ref, but you’re accessing it as an array at[0]. Verify thatinputBox.valueis always an array and that indexing is valid in all usage scenarios (e.g., line 77).packages/nc-gui/components/cell/attachment/utils.ts (2)
285-285: Ensure correct property references onselectedFile.When you clone an attachment into
selectedFile.value = { ...attachment }, verify that all relevant attachment fields remain intact (such as mimetype, url, etc.). This code duplicates the object without further deep copying nested properties.
543-543: Ensure concurrency handling of rename & bulk downloads.
renameFileInlineis exported alongsidebulkDownloadAttachments. Think about concurrency: if a user renames an item while a bulk download is in progress, confirm that the correct file name is included in the final archive. Potential race conditions may arise if the rename occurs mid-download.
| @@ -236,6 +236,7 @@ export default defineNuxtConfig({ | |||
| 'deep-object-diff', | |||
| 'embla-carousel-vue', | |||
| 'emoji-mart-vue-fast/src', | |||
| 'fflate', | |||
💡 Codebase verification
fflate dependency is missing from the project
The library fflate is not declared in any of the project's package.json files (neither in dependencies nor devDependencies) and is also not found in the lockfiles. Since it's included in optimizeDeps.include in nuxt.config.ts, it needs to be properly installed and declared to avoid build failures.
- Add
fflateto the dependencies inpackages/nc-gui/package.json - Run package manager install command to update lockfile
🔗 Analysis chain
Verify that 'fflate' is properly declared in dependencies
The library 'fflate' was added to optimizeDeps.include. Please ensure that 'fflate' is installed and declared in the project's dependencies (e.g., in packages/nc-gui/package.json) to avoid build failures.
Run the following script to confirm:
Let me verify if the package is installed by checking the lockfile as well.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if 'fflate' is properly listed in any package.json file
fd package.json --exec sh -c 'echo "=== {} ==="; cat {} | jq ".dependencies[\"fflate\"] // .devDependencies[\"fflate\"]"'
Length of output: 628
Script:
#!/bin/bash
# Check if 'fflate' is listed in package-lock.json or yarn.lock
echo "=== Checking package-lock.json ==="
fd package-lock.json --exec sh -c 'cat {} | jq ".[\"dependencies\"][\"fflate\"] // .[\"packages\"][\"node_modules/fflate\"]"'
echo -e "\n=== Checking yarn.lock ==="
fd yarn.lock --exec grep -A 5 "^fflate@"
Length of output: 328
| if (!modelId || !columnId || !rowId) { | ||
| console.error('Missing modelId, columnId or rowId') | ||
| message.error('Failed to download file') | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Graceful handle for missing resource IDs.
You log an error and show a message if modelId, columnId, or rowId is missing. Consider adding logic to request the user re-check their row or column definitions or redirect them if the data is unavailable.
Uffizzi Ephemeral Environment Deploying☁️ https://app.uffizzi.com/github.com/nocodb/nocodb/pull/10168 ⚙️ Updating now by workflow run 12599636662. What is Uffizzi? Learn more! |
428eee6 to
ec92595
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/nc-gui/components/cell/attachment/Modal.vue (1)
205-235: Enhance keyboard accessibility for inline renaming.Consider adding keyboard shortcuts for common actions:
Enterto start editing (in addition to double-click)Tabto move between files<div class="flex w-full text-[12px] items-center text-gray-700 cursor-default h-5" :class="{ truncate: !isRenamingFile }" - @dblclick.stop="handleFileRenameStart(i)" + @dblclick.stop="handleFileRenameStart(i)" + @keydown.enter.stop="handleFileRenameStart(i)" + tabindex="0" >packages/nc-gui/components/cell/attachment/utils.ts (1)
350-454: Improve bulk download implementation organization and error handling.While the implementation is robust, consider these improvements:
- Extract zip creation logic into a separate function
- Provide more specific error messages
+ async function createZipFile(filesData: { name: string; data: Uint8Array }[]): Promise<Blob> { + const zip: Record<string, Uint8Array> = {} + filesData.forEach(({ name, data }) => { + zip[name] = data + }) + + const zipData = await new Promise<Uint8Array>((resolve, reject) => { + fflateZip(zip, (err, data) => { + if (err) reject(err) + else resolve(data) + }) + }) + + return new Blob([zipData], { type: 'application/zip' }) + } async function bulkDownloadAttachments() { // ... existing validation code ... try { - // Create a zip object - const zip: Record<string, Uint8Array> = {} - // Add files to zip object - filesData.forEach(({ name, data }) => { - zip[name] = data - }) - - const zipData = await new Promise<Uint8Array>((resolve, reject) => { - fflateZip(zip, (err, data) => { - if (err) reject(err) - else resolve(data) - }) - }) - - const blob = new Blob([zipData], { type: 'application/zip' }) + const blob = await createZipFile(filesData) const zipURL = URL.createObjectURL(blob) try { window.open(zipURL, '_self') } catch (e) { - console.error('Error opening blob window', e) - message.error('Failed to download file') + console.error('Error opening download window:', e) + message.error('Failed to initiate download. Please try again.') return undefined } finally { setTimeout(() => URL.revokeObjectURL(zipURL), 1000) } } catch (e) { - console.error('Error creating zip file', e) - message.error('Failed to create zip file') + console.error('Error creating zip archive:', e) + message.error('Failed to create zip archive. Please try again or download files individually.') } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/nc-gui/components/cell/DatePicker.vue(2 hunks)packages/nc-gui/components/cell/DateTimePicker.vue(3 hunks)packages/nc-gui/components/cell/TimePicker.vue(2 hunks)packages/nc-gui/components/cell/YearPicker.vue(2 hunks)packages/nc-gui/components/cell/attachment/AttachFile.vue(2 hunks)packages/nc-gui/components/cell/attachment/Carousel.vue(4 hunks)packages/nc-gui/components/cell/attachment/Modal.vue(10 hunks)packages/nc-gui/components/cell/attachment/UploadProviders/Camera.vue(2 hunks)packages/nc-gui/components/cell/attachment/UploadProviders/Local.vue(3 hunks)packages/nc-gui/components/cell/attachment/utils.ts(6 hunks)packages/nc-gui/components/smartsheet/column/CurrencyOptions.vue(2 hunks)packages/nc-gui/nuxt.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/nc-gui/components/cell/DatePicker.vue
- packages/nc-gui/components/cell/attachment/UploadProviders/Camera.vue
- packages/nc-gui/components/cell/attachment/AttachFile.vue
- packages/nc-gui/components/cell/DateTimePicker.vue
- packages/nc-gui/nuxt.config.ts
- packages/nc-gui/components/smartsheet/column/CurrencyOptions.vue
- packages/nc-gui/components/cell/TimePicker.vue
- packages/nc-gui/components/cell/YearPicker.vue
- packages/nc-gui/components/cell/attachment/UploadProviders/Local.vue
- packages/nc-gui/components/cell/attachment/Carousel.vue
🔇 Additional comments (4)
packages/nc-gui/components/cell/attachment/Modal.vue (3)
64-99: Well-structured file renaming implementation!The new reactive refs and methods for handling file renaming follow Vue.js composition API best practices. The implementation includes proper state management and error handling.
Line range hint
111-132: Clean UI organization for file actions!The bulk download functionality is well-integrated with proper button state management and clear visual hierarchy.
Line range hint
287-311: Clean and consistent styling implementation!The styles follow Tailwind conventions and provide a good responsive layout for attachments.
packages/nc-gui/components/cell/attachment/utils.ts (1)
364-367: Graceful handle for missing resource IDs.You log an error and show a message if
modelId,columnId, orrowIdis missing. Consider adding logic to request the user re-check their row or column definitions or redirect them if the data is unavailable.
| attachments.value[idx]!.title = title | ||
| updateModelValue(attachments.value) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for safer attachment updates.
The non-null assertion operator might lead to runtime errors if the attachment at the given index is undefined.
- attachments.value[idx]!.title = title
+ if (!attachments.value[idx]) {
+ console.error('Attachment not found at index:', idx)
+ return
+ }
+ attachments.value[idx].title = title
updateModelValue(attachments.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.
| attachments.value[idx]!.title = title | |
| updateModelValue(attachments.value) | |
| } | |
| if (!attachments.value[idx]) { | |
| console.error('Attachment not found at index:', idx) | |
| return | |
| } | |
| attachments.value[idx].title = title | |
| updateModelValue(attachments.value) |
3c0b535 to
a19df89
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/nc-gui/components/cell/attachment/Modal.vue (3)
72-99: Add input validation for the new file titleWhile the file renaming handlers are well-structured, consider adding validation for the new title to prevent empty or invalid filenames.
const handleFileRename = async () => { if (!isRenamingFile.value) return - if (newTitle.value) { + const trimmedTitle = newTitle.value.trim() + if (!trimmedTitle) { + message.error('File name cannot be empty') + return + } + if (!/^[^<>:"/\\|?*]+$/.test(trimmedTitle)) { + message.error('File name contains invalid characters') + return + } + try { + await renameFileInline(renameFileIdx.value, trimmedTitle) + handleResetFileRename() + } catch (e) { message.error('Error while renaming file') throw e } }
115-120: Add loading state for bulk downloadConsider adding a loading state to prevent multiple clicks during the download process and provide better user feedback.
<NcButton :disabled="!selectedVisibleItems.some((v) => !!v)" type="secondary" size="small" + :loading="isDownloading" @click="bulkDownloadAttachments" >
205-235: Enhance accessibility for file renamingThe inline renaming UI needs accessibility improvements for screen readers.
<div class="flex w-full text-[12px] items-center text-gray-700 cursor-default h-5" :class="{ truncate: !isRenamingFile }" + role="button" + :aria-label="isRenamingFile ? 'Press Enter to save or Escape to cancel' : `Double click to rename ${item.title}`" @dblclick.stop="handleFileRenameStart(i)" >packages/nc-gui/components/cell/attachment/utils.ts (1)
350-454: Add progress tracking for bulk downloadsConsider implementing progress tracking for better user feedback during large downloads.
async function bulkDownloadAttachments() { + const progress = ref(0) + const total = selectedVisibleItems.value.filter(Boolean).length const items: AttachmentType[] = selectedVisibleItems.value .map((v, i) => (v ? visibleItems.value[i] : undefined)) .filter(Boolean) if (items.length === 0) return if (items.length === 1) { return downloadAttachment(items[0]!) } // ... existing code ... for (const item of items) { // ... existing download code ... + progress.value++ + emit('download-progress', { progress: progress.value, total }) } // ... rest of the function ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/nc-gui/components/cell/DatePicker.vue(2 hunks)packages/nc-gui/components/cell/DateTimePicker.vue(3 hunks)packages/nc-gui/components/cell/TimePicker.vue(2 hunks)packages/nc-gui/components/cell/YearPicker.vue(2 hunks)packages/nc-gui/components/cell/attachment/AttachFile.vue(2 hunks)packages/nc-gui/components/cell/attachment/Carousel.vue(4 hunks)packages/nc-gui/components/cell/attachment/Modal.vue(10 hunks)packages/nc-gui/components/cell/attachment/UploadProviders/Camera.vue(2 hunks)packages/nc-gui/components/cell/attachment/UploadProviders/Local.vue(3 hunks)packages/nc-gui/components/cell/attachment/utils.ts(6 hunks)packages/nc-gui/components/smartsheet/column/CurrencyOptions.vue(2 hunks)packages/nc-gui/nuxt.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/nc-gui/components/cell/DatePicker.vue
- packages/nc-gui/components/cell/attachment/UploadProviders/Local.vue
- packages/nc-gui/components/cell/attachment/UploadProviders/Camera.vue
- packages/nc-gui/components/cell/DateTimePicker.vue
- packages/nc-gui/components/cell/YearPicker.vue
- packages/nc-gui/nuxt.config.ts
- packages/nc-gui/components/cell/attachment/AttachFile.vue
- packages/nc-gui/components/smartsheet/column/CurrencyOptions.vue
- packages/nc-gui/components/cell/TimePicker.vue
- packages/nc-gui/components/cell/attachment/Carousel.vue
🔇 Additional comments (3)
packages/nc-gui/components/cell/attachment/Modal.vue (1)
64-70: LGTM: Well-structured state managementThe new reactive references are well-organized and follow clear naming conventions for managing modal and file renaming states.
packages/nc-gui/components/cell/attachment/utils.ts (2)
271-273: Add null check for safer attachment updates.The non-null assertion operator might lead to runtime errors if the attachment at the given index is undefined.
300-308: LGTM: Clean implementation of inline renamingThe renameFileInline function is well-implemented with proper handling of the selected file state.
Change Summary
Change type
fix: (bug fix for the user, not a fix to a build script)