feat: allow preview for media types #9052
Conversation
dosubot
bot
added
the
size:L
WalkthroughWalkthroughThe recent changes enhance attachment handling across the application by improving file download logic and HTTP response management. Key updates include more precise parameters for MIME types and file previews, streamlined signed URL generation, and clearer function naming. These modifications collectively create a more efficient and user-friendly file management system. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
packages/nocodb/src/helpers/attachmentHelpers.ts (1)
5-19: Consider adding comments for clarity.Adding comments to explain the logic can improve readability and maintainability.
+ // Destructure mimetype and path from args const { mimetype, path } = args; if (mimetype) { + // Check if the mimetype is in the list of previewable types return previewableMimeTypes.some((type) => mimetype.includes(type)); } else if (path) { + // Extract the file extension from the path const ext = path.split('.').pop(); if (ext) { + // Get the MIME type from the extension and check if it's previewable const mimeType = mime.getType(ext); return previewableMimeTypes.some((type) => mimeType?.includes(type)); } } + // Return false if neither mimetype nor path is provided return false;packages/nocodb/src/controllers/attachments-secure.controller.ts (1)
78-99: Consider logging the error in the catch block.Logging the error can help with debugging and provide more context when an error occurs.
} catch (e) { + console.error(`Error in fileReadv3: ${e.message}`, e); res.status(404).send('Not found'); }packages/nocodb/src/modules/jobs/jobs/data-export/data-export.processor.ts (1)
98-108: Consider adding comments for clarity.Adding comments to explain the logic can improve readability and maintainability.
// if url is not defined, it is local attachment if (!url) { + // Generate a signed URL for the local attachment url = await PresignedUrl.getSignedUrl({ path: path.join(destPath.replace('nc/uploads/', '')), expireSeconds: 3 * 60 * 60, // 3 hours filename: `${model.title} (${getViewTitle(view)}).csv`, preview: false, mimetype: 'text/csv', }); } else { + // Generate a signed URL for the external attachment url = await PresignedUrl.getSignedUrl({ path: decodeURI(new URL(url).pathname), expireSeconds: 3 * 60 * 60, // 3 hours filename: `${model.title} (${getViewTitle(view)}).csv`, preview: false, mimetype: 'text/csv', }); }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- packages/nocodb/src/controllers/attachments-secure.controller.ts (1 hunks)
- packages/nocodb/src/controllers/attachments.controller.ts (4 hunks)
- packages/nocodb/src/db/BaseModelSqlv2.ts (3 hunks)
- packages/nocodb/src/helpers/attachmentHelpers.ts (1 hunks)
- packages/nocodb/src/models/PresignedUrl.ts (4 hunks)
- packages/nocodb/src/modules/jobs/jobs/data-export/data-export.processor.ts (1 hunks)
- packages/nocodb/src/plugins/s3/S3.ts (1 hunks)
- packages/nocodb/src/services/attachments.service.ts (2 hunks)
Additional comments not posted (24)
packages/nocodb/src/controllers/attachments.controller.ts (4)
25-25: Import statement looks good.The
isPreviewAllowedhelper function is correctly imported from~/helpers/attachmentHelpers.
77-77: Usage ofisPreviewAllowedlooks good.The function is correctly used to determine if a file can be previewed based on its mimetype and path.
114-114: Usage ofisPreviewAllowedlooks good.The function is correctly used to determine if a file can be previewed based on its mimetype and path.
139-162: Handling of query parameters looks good.The query parameters
ResponseContentTypeandResponseContentDispositionare correctly extracted and used to set the response headers.packages/nocodb/src/models/PresignedUrl.ts (6)
6-6: Import statement looks good.The
isPreviewAllowedhelper function is correctly imported from~/helpers/attachmentHelpers.
95-96: Addition ofpreviewandmimetypeparameters looks good.The parameters are correctly added to the
getSignedUrlmethod, enhancing its flexibility.
102-106: Destructuring ofpreviewandmimetypeparameters looks good.The destructuring is correctly implemented and aligns with the changes described in the summary.
108-110: Usage ofisPreviewAllowedlooks good.The function is correctly used to set the
previewvariable based on thepathandmimetype.
145-149: Addition ofpreviewandmimetypeparameters in storage adapter looks good.The parameters are correctly added to the
getSignedUrlmethod of the storage adapter, enhancing its flexibility.
161-183: Handling ofResponseContentDispositionandResponseContentTypeheaders looks good.The headers are correctly set based on the
previewandmimetypeparameters.packages/nocodb/src/plugins/s3/S3.ts (4)
110-118: Addition ofoptionsparameter looks good.The parameter is correctly added to the
getSignedUrlmethod, enhancing its flexibility.
119-120: Destructuring offilename,preview, andmimetypeproperties looks good.The destructuring is correctly implemented and aligns with the changes described in the summary.
124-129: Handling ofResponseContentDispositionandResponseContentTypeheaders looks good.The headers are correctly set based on the
previewandmimetypeproperties.
Line range hint
161-183: Handling ofResponseContentDispositionandResponseContentTypeheaders in fallback URL generation looks good.The headers are correctly set based on the
previewandmimetypeproperties in the fallback URL generation.packages/nocodb/src/services/attachments.service.ts (4)
92-93: Ensure thepreviewparameter is handled correctly ingetSignedUrl.The
previewparameter is passed astrue. Verify that thegetSignedUrlmethod correctly handles this parameter to enable preview functionality.
93-93: Ensure themimetypeparameter is handled correctly ingetSignedUrl.The
mimetypeparameter is passed as the attachment's mimetype. Verify that thegetSignedUrlmethod correctly handles this parameter to support different MIME types.
86-87: Ensure thepreviewparameter is handled correctly ingetSignedUrl.The
previewparameter is passed astrue. Verify that thegetSignedUrlmethod correctly handles this parameter to enable preview functionality.Verification successful
Ensure the
previewparameter is handled correctly ingetSignedUrl.The
previewparameter is correctly handled in thegetSignedUrlmethod. The method includes thepreviewparameter in its options and processes it accordingly.
packages/nocodb/src/services/attachments.service.ts:
- The
getSignedUrlmethod includes thepreviewparameter in its options.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `getSignedUrl` method correctly handles the `preview` parameter. # Test: Search for the `getSignedUrl` method definition. Expect: The method should handle the `preview` parameter. rg --type typescript -A 10 $'getSignedUrl'Length of output: 76
Script:
#!/bin/bash # Description: Verify that the `getSignedUrl` method correctly handles the `preview` parameter. # Locate all TypeScript files and search for the `getSignedUrl` method definition within those files. fd --extension ts --exec rg 'getSignedUrl' -A 10Length of output: 5389
87-87: Ensure themimetypeparameter is handled correctly ingetSignedUrl.The
mimetypeparameter is passed as the attachment's mimetype. Verify that thegetSignedUrlmethod correctly handles this parameter to support different MIME types.Verification successful
Ensure the
mimetypeparameter is handled correctly ingetSignedUrl.The
mimetypeparameter is passed to thegetSignedUrlmethod in multiple instances. The method definitions in both thePresignedUrlandS3classes include handling for themimetypeparameter, ensuring it supports different MIME types.
PresignedUrl class:
public static async getSignedUrl( param: { path: string; expireSeconds?: number; filename?: string; preview?: boolean; mimetype?: string; }, ncMeta = Noco.ncMeta, ) { // Implementation details... if (typeof (storageAdapter as any).getSignedUrl === 'function') { tempUrl = await (storageAdapter as any).getSignedUrl( path, expiresInSeconds, { filename, preview, mimetype, }, ); // Additional implementation details... } }S3 class:
public async getSignedUrl( key, expiresInSeconds = 7200, options?: { filename?: string; preview?: boolean; mimetype?: string; }, ) { const { filename, preview, mimetype } = options || {}; // Implementation details... return getSignedUrl(this.s3Client, command, { expiresIn: expiresInSeconds, }); }Both implementations correctly handle the
mimetypeparameter, ensuring support for different MIME types.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `getSignedUrl` method correctly handles the `mimetype` parameter. # Test: Search for the `getSignedUrl` method definition. Expect: The method should handle the `mimetype` parameter. rg --type typescript -A 10 $'getSignedUrl'Length of output: 76
Script:
#!/bin/bash # Description: Verify that the `getSignedUrl` method correctly handles the `mimetype` parameter. # Find all TypeScript files and search for the `getSignedUrl` method definition. fd --extension ts --exec rg 'getSignedUrl' -A 10Length of output: 5389
packages/nocodb/src/db/BaseModelSqlv2.ts (6)
7215-7217: Ensurepreviewproperty is necessary.The addition of the
previewproperty to thePresignedUrl.getSignedUrlmethod enhances the functionality, but verify if this is required for all attachment types.
7216-7217: Checkmimetypeandfilenameproperties.The inclusion of
mimetypeandfilenameproperties provides more context for the attachments. Ensure these properties are correctly populated and used downstream.
7226-7228: Ensurepreviewproperty is necessary.The addition of the
previewproperty to thePresignedUrl.getSignedUrlmethod enhances the functionality, but verify if this is required for all attachment types.
7227-7228: Checkmimetypeandfilenameproperties.The inclusion of
mimetypeandfilenameproperties provides more context for the attachments. Ensure these properties are correctly populated and used downstream.
7238-7240: Ensurepreviewproperty is necessary.The addition of the
previewproperty to thePresignedUrl.getSignedUrlmethod enhances the functionality, but verify if this is required for all attachment types.
7239-7240: Checkmimetypeandfilenameproperties.The inclusion of
mimetypeandfilenameproperties provides more context for the attachments. Ensure these properties are correctly populated and used downstream.
o1lab
force-pushed
the
nc-feat/preview-and-download
branch
from
a2c2475 to
52bc217
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- packages/nocodb/src/controllers/attachments-secure.controller.ts (1 hunks)
- packages/nocodb/src/controllers/attachments.controller.ts (4 hunks)
- packages/nocodb/src/db/BaseModelSqlv2.ts (3 hunks)
- packages/nocodb/src/helpers/attachmentHelpers.ts (1 hunks)
- packages/nocodb/src/models/PresignedUrl.ts (5 hunks)
- packages/nocodb/src/modules/jobs/jobs/data-export/data-export.processor.ts (1 hunks)
- packages/nocodb/src/plugins/s3/S3.ts (1 hunks)
- packages/nocodb/src/services/attachments.service.ts (2 hunks)
Files skipped from review as they are similar to previous changes (7)
- packages/nocodb/src/controllers/attachments-secure.controller.ts
- packages/nocodb/src/controllers/attachments.controller.ts
- packages/nocodb/src/db/BaseModelSqlv2.ts
- packages/nocodb/src/helpers/attachmentHelpers.ts
- packages/nocodb/src/modules/jobs/jobs/data-export/data-export.processor.ts
- packages/nocodb/src/plugins/s3/S3.ts
- packages/nocodb/src/services/attachments.service.ts
Additional context used
Biome
packages/nocodb/src/models/PresignedUrl.ts
[error] 174-174: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Additional comments not posted (5)
packages/nocodb/src/models/PresignedUrl.ts (5)
6-6: LGTM!The import statement for
isPreviewAllowedappears correct.
95-96: LGTM!The
previewandmimetypeparameters are correctly added to thegetSignedUrlmethod.
102-108: LGTM!The destructuring and handling of
previewandmimetypeparameters are correctly implemented.
127-139: LGTM!The conditional logic for setting
ResponseContentDispositionbased onpreviewis correctly implemented.
141-143: LGTM!The inclusion of
mimetypeinResponseContentTypeis correctly implemented.
o1lab
force-pushed
the
nc-feat/preview-and-download
branch
from
52bc217 to
1bd898b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- packages/nc-gui/components/cell/attachment/Modal.vue (3 hunks)
- packages/nc-gui/components/cell/attachment/index.vue (4 hunks)
- packages/nc-gui/components/cell/attachment/utils.ts (6 hunks)
- packages/nc-gui/composables/useAttachment.ts (1 hunks)
- packages/nocodb/src/controllers/attachments-secure.controller.ts (1 hunks)
- packages/nocodb/src/controllers/attachments.controller.ts (4 hunks)
- packages/nocodb/src/db/BaseModelSqlv2.ts (3 hunks)
- packages/nocodb/src/helpers/attachmentHelpers.ts (1 hunks)
- packages/nocodb/src/models/PresignedUrl.ts (5 hunks)
- packages/nocodb/src/modules/jobs/jobs/data-export/data-export.processor.ts (1 hunks)
- packages/nocodb/src/plugins/s3/S3.ts (1 hunks)
- packages/nocodb/src/services/attachments.service.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- packages/nc-gui/components/cell/attachment/Modal.vue
Files skipped from review as they are similar to previous changes (7)
- packages/nocodb/src/controllers/attachments-secure.controller.ts
- packages/nocodb/src/controllers/attachments.controller.ts
- packages/nocodb/src/db/BaseModelSqlv2.ts
- packages/nocodb/src/helpers/attachmentHelpers.ts
- packages/nocodb/src/modules/jobs/jobs/data-export/data-export.processor.ts
- packages/nocodb/src/plugins/s3/S3.ts
- packages/nocodb/src/services/attachments.service.ts
Additional context used
Biome
packages/nocodb/src/models/PresignedUrl.ts
[error] 174-174: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Additional comments not posted (8)
packages/nc-gui/composables/useAttachment.ts (1)
23-23: Verify the necessity ofmode: 'no-cors'in the fetch request.Using
mode: 'no-cors'allows cross-origin requests without requiring the server to explicitly allow them through CORS headers. This can have security implications and may affect the fetch response, as it restricts the response type toopaque. Ensure that this change is necessary and does not introduce security vulnerabilities.packages/nocodb/src/models/PresignedUrl.ts (2)
95-96: LGTM! The new parameters enhance flexibility.The introduction of
previewandmimetypeparameters in the constructor improves the flexibility of thePresignedUrlclass, allowing for conditional handling of URL generation based on these parameters.
108-110: LGTM! The changes improve signed URL generation.The logic for handling
previewandmimetypeparameters enhances the functionality of the signed URL generation process, allowing for better control over content types and preview options.Also applies to: 127-143
packages/nc-gui/components/cell/attachment/index.vue (1)
47-47: LGTM! The changes improve control flow and resource management.The introduction of the
modalRenderedvariable enhances the control flow and resource management by ensuring that the modal is rendered only when necessary, preventing unnecessary rendering.Also applies to: 128-128, 162-162, 357-357
packages/nc-gui/components/cell/attachment/utils.ts (4)
324-328: EnsuredownloadFilefunction is implemented for bulk downloads.The
bulkDownloadAttachmentsfunction depends on thedownloadFilefunction, which is currently a placeholder. EnsuredownloadFileis implemented to avoid errors.
330-340: Verify URL construction and error handling indownloadAttachment.Ensure that the
downloadAttachmentfunction correctly constructs the URL and handles errors appropriately.Verification successful
Verify URL construction and error handling in
downloadAttachment.The
downloadAttachmentfunction correctly constructs the URL and handles errors appropriately. It checks for necessary values and displays an error message if any are missing.
packages/nc-gui/components/cell/attachment/utils.ts: URL construction and error handling indownloadAttachment.packages/nc-gui/components/cell/attachment/Modal.vue: Usage ofdownloadAttachment.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the URL construction and error handling in the `downloadAttachment` function. # Test: Search for the usage of `downloadAttachment`. Expect: Correct URL construction and error handling. rg --type js --type ts 'downloadAttachment' -A 5Length of output: 3743
38-38: Verify the usage ofmodalRendered.Ensure that the
modalRenderedstate variable is used correctly and consistently throughout the file.Verification successful
Verify the lifecycle and usage of
modalRenderedinindex.vue.Ensure that
modalRenderedis used correctly within the lifecycle methods and other relevant parts ofindex.vue.
Verify the initialization and management of
modalRenderedin the component setup and lifecycle.Ensure that
modalRenderedis correctly initialized and managed within the component's setup and lifecycle.
The usage of
modalRenderedis consistent and correct.The
modalRenderedvariable is properly initialized in thesetupfunction and used correctly within the component.
modalRenderedis imported fromuseProvideAttachmentCell.- It is used to control the rendering of the
LazyCellAttachmentModalcomponent.- The variable is set to
truein specific conditions, ensuring correct behavior.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `modalRendered` in the codebase. # Test: Search for the usage of `modalRendered`. Expect: Consistent and correct usage. rg --type js --type ts 'modalRendered' -A 5Length of output: 2865
Script:
#!/bin/bash # Description: Verify the lifecycle and usage of `modalRendered` in `index.vue`. # Test: Search for the lifecycle methods and other relevant parts where `modalRendered` is used. rg 'modalRendered|mounted|created|beforeMount|beforeUpdate|updated|beforeDestroy|destroyed' packages/nc-gui/components/cell/attachment/index.vue -A 10Length of output: 1293
Script:
#!/bin/bash # Description: Verify the initialization and management of `modalRendered` in the component setup and lifecycle. # Test: Search for the component setup and other relevant parts where `modalRendered` might be managed. rg 'setup|modalRendered' packages/nc-gui/components/cell/attachment/index.vue -A 20Length of output: 2831
13-15: Verify the usage of$apiandbaseURL.Ensure that
$apiandbaseURLare correctly initialized and used throughout the file for constructing URLs dynamically.Verification successful
Verify the usage of
$apiandbaseURL.The
$apiandbaseURLvariables are used extensively in API calls and configurations throughout the codebase. Ensure that these variables are correctly initialized and consistently used in the following locations:
packages/nc-gui/components/cell/attachment/utils.tspackages/nc-gui/components/smartsheet/details/Api.vuepackages/nc-gui/components/smartsheet/ApiSnippet.vuepackages/nc-gui/components/dlg/QuickImport.vuepackages/nc-gui/components/dlg/AirtableImport.vuepackages/nc-gui/composables/useApi/index.tsVerify that the initialization and usage of these variables align with the expected configurations and API endpoints.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `$api` and `baseURL` in the codebase. # Test: Search for the usage of `$api` and `baseURL`. Expect: Consistent and correct usage. rg --type js --type ts '$api' -A 5 rg --type js --type ts 'baseURL' -A 5Length of output: 315124
o1lab
force-pushed
the
nc-feat/preview-and-download
branch
from
1bd898b to
04b15b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- packages/nc-gui/components/cell/attachment/Modal.vue (3 hunks)
- packages/nc-gui/components/cell/attachment/index.vue (4 hunks)
- packages/nc-gui/components/cell/attachment/utils.ts (6 hunks)
- packages/nc-gui/composables/useAttachment.ts (1 hunks)
- packages/nocodb/src/controllers/attachments-secure.controller.ts (3 hunks)
- packages/nocodb/src/controllers/attachments.controller.ts (4 hunks)
- packages/nocodb/src/db/BaseModelSqlv2.ts (3 hunks)
- packages/nocodb/src/helpers/attachmentHelpers.ts (1 hunks)
- packages/nocodb/src/models/PresignedUrl.ts (5 hunks)
- packages/nocodb/src/modules/jobs/jobs/data-export/data-export.processor.ts (1 hunks)
- packages/nocodb/src/plugins/s3/S3.ts (1 hunks)
- packages/nocodb/src/services/attachments.service.ts (2 hunks)
Files skipped from review as they are similar to previous changes (10)
- packages/nc-gui/components/cell/attachment/Modal.vue
- packages/nc-gui/components/cell/attachment/index.vue
- packages/nc-gui/components/cell/attachment/utils.ts
- packages/nc-gui/composables/useAttachment.ts
- packages/nocodb/src/controllers/attachments-secure.controller.ts
- packages/nocodb/src/db/BaseModelSqlv2.ts
- packages/nocodb/src/helpers/attachmentHelpers.ts
- packages/nocodb/src/modules/jobs/jobs/data-export/data-export.processor.ts
- packages/nocodb/src/plugins/s3/S3.ts
- packages/nocodb/src/services/attachments.service.ts
Additional context used
Biome
packages/nocodb/src/models/PresignedUrl.ts
[error] 174-174: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Additional comments not posted (16)
packages/nocodb/src/models/PresignedUrl.ts (9)
6-6: Import statement approved.The import statement for
isPreviewAllowedis correct and necessary for the new functionality.
95-96: New parameters approved.The addition of
previewandmimetypeparameters in thegetSignedUrlmethod is appropriate for the new feature requirements.
102-106: Destructuring approved.The destructuring of
expireSeconds,filename, andmimetypeparameters from theparamobject is necessary and correctly implemented.
108-110: Logic for determiningpreviewapproved.The logic for determining the
previewvariable using theisPreviewAllowedfunction is correctly implemented.
123-148: Construction ofpathParametersobject approved.The construction of the
pathParametersobject to includeResponseContentDispositionandResponseContentTypeis correctly implemented and necessary for handling the content disposition and type in the URL.
141-143: Addition ofResponseContentTypeheader approved.The inclusion of the
ResponseContentTypeheader in thepathParametersobject based on themimetypeparameter is correctly implemented.
127-139: Addition ofResponseContentDispositionheader approved.The inclusion of the
ResponseContentDispositionheader in thepathParametersobject based on thepreviewandfilenameparameters is correctly implemented.
145-149: Construction ofcachePathapproved.The construction of the
cachePathby appending query parameters to the path is correctly implemented and necessary for caching the URL with the appropriate parameters.
Line range hint
223-241: New parameters ingetSignedUrlcalls approved.The addition of
previewandmimetypeparameters in thegetSignedUrlcalls is appropriate for the new feature requirements.packages/nocodb/src/controllers/attachments.controller.ts (7)
25-25: Import statement approved.The import statement for
isPreviewAllowedis correct and necessary for the new functionality.
Line range hint
86-90: Usage ofisPreviewAllowedapproved.The usage of the
isPreviewAllowedfunction to check if a file can be previewed is correctly implemented.
Line range hint
123-127: Usage ofisPreviewAllowedapproved.The usage of the
isPreviewAllowedfunction to check if a file can be previewed is correctly implemented.
148-156: New variables approved.The addition of
queryResponseContentTypeandqueryResponseContentDispositionvariables improves the flexibility of managing response headers.
163-164: Setting ofContent-Typeheader approved.Setting the
Content-Typeheader based on thequeryResponseContentTypevariable is correctly implemented.
167-169: Setting ofContent-Dispositionheader approved.Setting the
Content-Dispositionheader based on thequeryResponseContentDispositionvariable is correctly implemented.
177-241: New methoddownloadAttachmentapproved.The addition of the
downloadAttachmentmethod to theAttachmentsControllerclass is correctly implemented and includes necessary guards for API rate limiting and access control.
|
Uffizzi Preview |
o1lab
force-pushed
the
nc-feat/preview-and-download
branch
from
04b15b3 to
1bde1f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- packages/nc-gui/components/cell/attachment/Carousel.vue (2 hunks)
- packages/nc-gui/components/cell/attachment/Modal.vue (3 hunks)
- packages/nc-gui/components/cell/attachment/index.vue (4 hunks)
- packages/nc-gui/components/cell/attachment/utils.ts (6 hunks)
- packages/nc-gui/composables/useAttachment.ts (1 hunks)
- packages/nocodb/src/controllers/attachments-secure.controller.ts (3 hunks)
- packages/nocodb/src/controllers/attachments.controller.ts (4 hunks)
- packages/nocodb/src/db/BaseModelSqlv2.ts (3 hunks)
- packages/nocodb/src/helpers/attachmentHelpers.ts (1 hunks)
- packages/nocodb/src/models/PresignedUrl.ts (5 hunks)
- packages/nocodb/src/modules/jobs/jobs/data-export/data-export.processor.ts (1 hunks)
- packages/nocodb/src/plugins/s3/S3.ts (1 hunks)
- packages/nocodb/src/services/attachments.service.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- packages/nc-gui/components/cell/attachment/Carousel.vue
Files skipped from review as they are similar to previous changes (10)
- packages/nc-gui/components/cell/attachment/Modal.vue
- packages/nc-gui/components/cell/attachment/index.vue
- packages/nc-gui/components/cell/attachment/utils.ts
- packages/nocodb/src/controllers/attachments-secure.controller.ts
- packages/nocodb/src/controllers/attachments.controller.ts
- packages/nocodb/src/db/BaseModelSqlv2.ts
- packages/nocodb/src/helpers/attachmentHelpers.ts
- packages/nocodb/src/modules/jobs/jobs/data-export/data-export.processor.ts
- packages/nocodb/src/plugins/s3/S3.ts
- packages/nocodb/src/services/attachments.service.ts
Additional context used
Biome
packages/nocodb/src/models/PresignedUrl.ts
[error] 174-174: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Additional comments not posted (8)
packages/nc-gui/composables/useAttachment.ts (3)
Line range hint
3-12:
LGTM!The function
getPossibleAttachmentSrcis well-implemented.
23-23: Potential security issue withmode: 'no-cors'.Using
mode: 'no-cors'in thefetchrequest can lead to security issues and unexpected behavior. It is generally not recommended to use this mode unless absolutely necessary.Please ensure that this change is necessary and does not introduce security vulnerabilities.
Line range hint
30-32:
LGTM!The function
openAttachmentis well-implemented.packages/nocodb/src/models/PresignedUrl.ts (5)
6-6: LGTM!The import statement for
isPreviewAllowedis correct.
95-96: LGTM!The addition of
previewandmimetypeparameters enhances the functionality of thegetSignedUrlmethod.
102-109: LGTM!The destructuring of
expireSeconds,filename, andmimetypeparameters and the conditional handling of thepreviewvariable are well-implemented.
123-148: LGTM!The construction of
pathParametersbased on thepreviewandfilenameparameters is well-implemented.
151-151: LGTM!The retrieval of the URL from the cache is well-implemented.
o1lab
force-pushed
the
nc-feat/preview-and-download
branch
from
1bde1f0 to
5783a56
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- packages/nc-gui/components/cell/attachment/Carousel.vue (2 hunks)
- packages/nc-gui/components/cell/attachment/Modal.vue (3 hunks)
- packages/nc-gui/components/cell/attachment/index.vue (4 hunks)
- packages/nc-gui/components/cell/attachment/utils.ts (6 hunks)
- packages/nc-gui/composables/useAttachment.ts (1 hunks)
- packages/nocodb/src/controllers/attachments-secure.controller.ts (3 hunks)
- packages/nocodb/src/controllers/attachments.controller.ts (4 hunks)
- packages/nocodb/src/db/BaseModelSqlv2.ts (3 hunks)
- packages/nocodb/src/helpers/attachmentHelpers.ts (1 hunks)
- packages/nocodb/src/models/PresignedUrl.ts (5 hunks)
- packages/nocodb/src/modules/jobs/jobs/data-export/data-export.processor.ts (1 hunks)
- packages/nocodb/src/plugins/s3/S3.ts (1 hunks)
- packages/nocodb/src/services/attachments.service.ts (2 hunks)
Files skipped from review as they are similar to previous changes (11)
- packages/nc-gui/components/cell/attachment/Carousel.vue
- packages/nc-gui/components/cell/attachment/Modal.vue
- packages/nc-gui/components/cell/attachment/index.vue
- packages/nc-gui/components/cell/attachment/utils.ts
- packages/nc-gui/composables/useAttachment.ts
- packages/nocodb/src/controllers/attachments-secure.controller.ts
- packages/nocodb/src/db/BaseModelSqlv2.ts
- packages/nocodb/src/helpers/attachmentHelpers.ts
- packages/nocodb/src/modules/jobs/jobs/data-export/data-export.processor.ts
- packages/nocodb/src/plugins/s3/S3.ts
- packages/nocodb/src/services/attachments.service.ts
Additional context used
Biome
packages/nocodb/src/models/PresignedUrl.ts
[error] 174-174: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Additional comments not posted (10)
packages/nocodb/src/models/PresignedUrl.ts (5)
6-6: Import statement looks good.The
isPreviewAllowedfunction is imported correctly.
95-96: New parameters added togetSignedUrlmethod.The new parameters
previewandmimetypeare correctly added to the method signature.
102-106: Destructuring of new parameters looks good.The new parameters
previewandmimetypeare correctly destructured from theparamobject.
108-110: Logic for determiningpreviewvariable looks good.The
previewvariable is correctly determined using theisPreviewAllowedfunction.
127-143: Usage ofpreviewandmimetypeinpathParameterslooks good.The
previewandmimetypeparameters are correctly used to setResponseContentDispositionandResponseContentTypeinpathParameters.packages/nocodb/src/controllers/attachments.controller.ts (5)
25-25: Import statement looks good.The
isPreviewAllowedfunction is imported correctly.
86-86: Usage ofisPreviewAllowedinfileReadmethod looks good.The
isPreviewAllowedfunction is correctly used to determine if a file can be previewed.
123-123: Usage ofisPreviewAllowedinfileReadv2method looks good.The
isPreviewAllowedfunction is correctly used to determine if a file can be previewed.
148-170: Handling of query parameters infileReadv3method looks good.The new variables
queryResponseContentTypeandqueryResponseContentDispositionare correctly introduced to handle query parameters.
177-242: New methoddownloadAttachmentlooks good.The new method
downloadAttachmentis correctly implemented and follows best practices.
o1lab
force-pushed
the
nc-feat/preview-and-download
branch
from
5783a56 to
0922c5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- packages/nc-gui/components/cell/attachment/Carousel.vue (2 hunks)
- packages/nc-gui/components/cell/attachment/Modal.vue (3 hunks)
- packages/nc-gui/components/cell/attachment/index.vue (4 hunks)
- packages/nc-gui/components/cell/attachment/utils.ts (6 hunks)
- packages/nc-gui/composables/useAttachment.ts (1 hunks)
- packages/nocodb/src/controllers/attachments-secure.controller.ts (3 hunks)
- packages/nocodb/src/controllers/attachments.controller.ts (4 hunks)
- packages/nocodb/src/controllers/public-datas.controller.ts (3 hunks)
- packages/nocodb/src/db/BaseModelSqlv2.ts (3 hunks)
- packages/nocodb/src/helpers/attachmentHelpers.ts (1 hunks)
- packages/nocodb/src/models/PresignedUrl.ts (5 hunks)
- packages/nocodb/src/modules/jobs/jobs/data-export/data-export.processor.ts (1 hunks)
- packages/nocodb/src/plugins/s3/S3.ts (1 hunks)
- packages/nocodb/src/services/attachments.service.ts (2 hunks)
- packages/nocodb/src/services/public-datas.service.ts (1 hunks)
Files skipped from review as they are similar to previous changes (11)
- packages/nc-gui/components/cell/attachment/Carousel.vue
- packages/nc-gui/components/cell/attachment/Modal.vue
- packages/nc-gui/components/cell/attachment/index.vue
- packages/nc-gui/components/cell/attachment/utils.ts
- packages/nocodb/src/controllers/attachments-secure.controller.ts
- packages/nocodb/src/controllers/attachments.controller.ts
- packages/nocodb/src/db/BaseModelSqlv2.ts
- packages/nocodb/src/helpers/attachmentHelpers.ts
- packages/nocodb/src/modules/jobs/jobs/data-export/data-export.processor.ts
- packages/nocodb/src/plugins/s3/S3.ts
- packages/nocodb/src/services/attachments.service.ts
Additional context used
Biome
packages/nocodb/src/models/PresignedUrl.ts
[error] 174-174: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Additional comments not posted (9)
packages/nc-gui/composables/useAttachment.ts (1)
23-23: Verify the impact of usingmode: 'no-cors'in the fetch request.Adding
mode: 'no-cors'can lead to unexpected behavior by allowing cross-origin requests without requiring the server to explicitly permit them. Ensure this change does not introduce security vulnerabilities or unintended side effects.packages/nocodb/src/models/PresignedUrl.ts (5)
95-96: New parameterspreviewandmimetypeadded.The
previewparameter allows conditional handling of URL generation based on whether previews are permitted. Themimetypeparameter specifies the content type of the response.
102-106: Destructuring new parameters ingetSignedUrl.The destructuring of
expireSeconds,filename, and the new parametersmimetypeandpreviewfrom theparamobject enhances readability and maintainability.
108-110: Conditional handling ofpreviewparameter.The
previewvariable is determined using theisPreviewAllowedfunction, which checks if previews can be generated for the specifiedpathandmimetype. This ensures that previews are only allowed when permitted.
127-139: Conditional construction ofResponseContentDisposition.The
ResponseContentDispositionis set toinlinewhenpreviewis true; otherwise, it defaults toattachment. The filename is conditionally appended to the disposition string based on the presence of thefilenameparameter.
141-143: Inclusion ofmimetypeinResponseContentType.If the
mimetypeis provided, it is included in theResponseContentTypeof the URL parameters. This enhances the flexibility of the signed URL generation process.packages/nocodb/src/controllers/public-datas.controller.ts (2)
25-27: New dependencyAttachmentsServiceadded to the constructor.The new dependency allows the controller to handle attachment-related functionalities, expanding its responsibilities.
201-237: New methoddownloadPublicAttachmentadded.The method handles downloading attachments associated with a public shared view. It validates the existence of the specified column and record, and retrieves the attachment using the
attachmentsService.packages/nocodb/src/services/public-datas.service.ts (1)
786-787: LGTM!The function correctly returns the retrieved row data.
| const view = await View.getByUUID(context, sharedViewUuid); | ||
|
|
||
| if (!view) NcError.viewNotFound(sharedViewUuid); |
There was a problem hiding this comment.
Handle potential null return from getByUUID.
The getByUUID method might return null if the view is not found. Ensure that NcError.viewNotFound is called correctly.
const view = await View.getByUUID(context, sharedViewUuid);
if (!view) {
throw NcError.viewNotFound(sharedViewUuid);
}
o1lab
force-pushed
the
nc-feat/preview-and-download
branch
from
0922c5c to
057f1aa
Compare
dosubot
bot
added
size:XL
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
packages/nc-gui/composables/useSharedView.ts (1)
369-369: Document the new function in the export statement.Ensure that the new function
fetchSharedViewAttachmentis properly documented in the module's documentation.return { sharedView, loadSharedView, meta, nestedFilters, fetchSharedViewData, fetchSharedViewActiveDate, fetchSharedCalendarViewData, fetchSharedViewGroupedData, fetchAggregatedData, fetchBulkAggregatedData, fetchSharedViewAttachment, paginationData, sorts, exportFile, formColumns, allowCSVDownload, } + /** + * Fetches an attachment for a shared view. + * @param {string} columnId - The ID of the column. + * @param {string} rowId - The ID of the row. + * @param {string} urlOrPath - The URL or path of the attachment. + * @returns {Promise<any>} - The attachment data. + */
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
packages/nocodb/src/schema/swagger.jsonis excluded by!**/*.json
Files selected for processing (16)
- packages/nc-gui/components/cell/attachment/Carousel.vue (2 hunks)
- packages/nc-gui/components/cell/attachment/Modal.vue (3 hunks)
- packages/nc-gui/components/cell/attachment/index.vue (4 hunks)
- packages/nc-gui/components/cell/attachment/utils.ts (6 hunks)
- packages/nc-gui/composables/useAttachment.ts (1 hunks)
- packages/nc-gui/composables/useSharedView.ts (2 hunks)
- packages/nocodb/src/controllers/attachments-secure.controller.ts (3 hunks)
- packages/nocodb/src/controllers/attachments.controller.ts (4 hunks)
- packages/nocodb/src/controllers/public-datas.controller.ts (3 hunks)
- packages/nocodb/src/db/BaseModelSqlv2.ts (3 hunks)
- packages/nocodb/src/helpers/attachmentHelpers.ts (1 hunks)
- packages/nocodb/src/models/PresignedUrl.ts (5 hunks)
- packages/nocodb/src/modules/jobs/jobs/data-export/data-export.processor.ts (1 hunks)
- packages/nocodb/src/plugins/s3/S3.ts (1 hunks)
- packages/nocodb/src/services/attachments.service.ts (2 hunks)
- packages/nocodb/src/services/public-datas.service.ts (1 hunks)
Files skipped from review as they are similar to previous changes (12)
- packages/nc-gui/components/cell/attachment/Carousel.vue
- packages/nc-gui/components/cell/attachment/Modal.vue
- packages/nc-gui/components/cell/attachment/index.vue
- packages/nc-gui/components/cell/attachment/utils.ts
- packages/nocodb/src/controllers/attachments-secure.controller.ts
- packages/nocodb/src/controllers/attachments.controller.ts
- packages/nocodb/src/controllers/public-datas.controller.ts
- packages/nocodb/src/db/BaseModelSqlv2.ts
- packages/nocodb/src/helpers/attachmentHelpers.ts
- packages/nocodb/src/modules/jobs/jobs/data-export/data-export.processor.ts
- packages/nocodb/src/plugins/s3/S3.ts
- packages/nocodb/src/services/public-datas.service.ts
Additional context used
Biome
packages/nocodb/src/models/PresignedUrl.ts
[error] 174-174: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Additional comments not posted (7)
packages/nc-gui/composables/useAttachment.ts (1)
23-23: Caution: Usingmode: 'no-cors'can lead to security issues and unexpected behavior.The
mode: 'no-cors'option in the fetch request allows cross-origin requests without requiring the server to explicitly allow them through CORS headers. This could lead to security issues or unexpected behavior, as the response will be opaque and you won't be able to read the response body or headers.Consider revisiting the need for
mode: 'no-cors'and ensure it aligns with the security policies of your application.packages/nocodb/src/models/PresignedUrl.ts (3)
95-96: Ensure proper validation for new parameters.The new
previewandmimetypeparameters should be validated to ensure they meet the expected criteria. This helps prevent potential issues with invalid or unexpected values.Also applies to: 102-110
127-143: Ensure consistent handling ofResponseContentDispositionandResponseContentType.The logic for setting
ResponseContentDispositionandResponseContentTypeshould be consistent and clear. Ensure that these headers are correctly set based on thepreviewandmimetypeparameters.
172-175: Avoid usingthisin a static context.Using
thisin a static context can be confusing. Use the class name instead.- await this.add({ + await PresignedUrl.add({Tools
Biome
[error] 174-174: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
packages/nocodb/src/services/attachments.service.ts (3)
86-87: Ensure proper handling ofpreviewandmimetypeparameters.The new
previewandmimetypeparameters should be handled consistently and validated to ensure they meet the expected criteria.Also applies to: 92-93
219-259: Ensure proper handling ofpreviewandmimetypeparameters.The new
previewandmimetypeparameters should be handled consistently and validated to ensure they meet the expected criteria.
241-259: Ensure consistent handling ofpreviewandmimetypeparameters.The logic for generating signed URLs and handling the new
previewandmimetypeparameters should be consistent and clear.
mertmit
force-pushed
the
nc-feat/preview-and-download
branch
from
057f1aa to
a61d878
Compare
Signed-off-by: mertmit <mertmit99@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
packages/nocodb/src/models/PresignedUrl.ts (1)
112-114: Simplify thepreviewassignment.The assignment of
previewcan be simplified by directly using the result ofisPreviewAllowed.- const preview = param.preview - ? isPreviewAllowed({ path, mimetype }) - : false; + const preview = isPreviewAllowed({ path, mimetype });
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
packages/nocodb/src/schema/swagger.jsonis excluded by!**/*.json
Files selected for processing (16)
- packages/nc-gui/components/cell/attachment/Carousel.vue (2 hunks)
- packages/nc-gui/components/cell/attachment/Modal.vue (3 hunks)
- packages/nc-gui/components/cell/attachment/index.vue (4 hunks)
- packages/nc-gui/components/cell/attachment/utils.ts (6 hunks)
- packages/nc-gui/composables/useAttachment.ts (1 hunks)
- packages/nc-gui/composables/useSharedView.ts (2 hunks)
- packages/nocodb/src/controllers/attachments-secure.controller.ts (3 hunks)
- packages/nocodb/src/controllers/attachments.controller.ts (4 hunks)
- packages/nocodb/src/controllers/public-datas.controller.ts (3 hunks)
- packages/nocodb/src/db/BaseModelSqlv2.ts (2 hunks)
- packages/nocodb/src/helpers/attachmentHelpers.ts (1 hunks)
- packages/nocodb/src/models/PresignedUrl.ts (6 hunks)
- packages/nocodb/src/modules/jobs/jobs/data-export/data-export.processor.ts (1 hunks)
- packages/nocodb/src/plugins/s3/S3.ts (1 hunks)
- packages/nocodb/src/services/attachments.service.ts (2 hunks)
- packages/nocodb/src/services/public-datas.service.ts (1 hunks)
Files skipped from review due to trivial changes (2)
- packages/nc-gui/components/cell/attachment/Modal.vue
- packages/nocodb/src/helpers/attachmentHelpers.ts
Files skipped from review as they are similar to previous changes (11)
- packages/nc-gui/components/cell/attachment/Carousel.vue
- packages/nc-gui/components/cell/attachment/index.vue
- packages/nc-gui/components/cell/attachment/utils.ts
- packages/nc-gui/composables/useSharedView.ts
- packages/nocodb/src/controllers/attachments-secure.controller.ts
- packages/nocodb/src/controllers/attachments.controller.ts
- packages/nocodb/src/db/BaseModelSqlv2.ts
- packages/nocodb/src/modules/jobs/jobs/data-export/data-export.processor.ts
- packages/nocodb/src/plugins/s3/S3.ts
- packages/nocodb/src/services/attachments.service.ts
- packages/nocodb/src/services/public-datas.service.ts
Additional context used
Biome
packages/nocodb/src/models/PresignedUrl.ts
[error] 178-178: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Additional comments not posted (6)
packages/nc-gui/composables/useAttachment.ts (1)
23-23: Verify the necessity ofmode: 'no-cors'.Using
mode: 'no-cors'can bypass CORS restrictions but might lead to security vulnerabilities. Ensure that this is necessary and that the implications are well-understood.packages/nocodb/src/models/PresignedUrl.ts (2)
6-6: EnsureisPreviewAllowedis imported correctly.Verify that the
isPreviewAllowedfunction is correctly implemented and imported from~/helpers/attachmentHelpers.
95-96: Review new parameters ingetSignedUrlmethod.The new
previewandmimetypeparameters should be validated to ensure they are used correctly within the method.packages/nocodb/src/controllers/public-datas.controller.ts (3)
18-18: EnsureAttachmentsServiceis imported correctly.Verify that the
AttachmentsServiceis correctly implemented and imported from~/services/attachments.service.
26-26: Verify the integration ofAttachmentsService.Ensure that the integration of
AttachmentsServicein the constructor does not introduce any issues and that it is used correctly throughout the class.
200-237: Review new route for downloading attachments.The new route for downloading attachments should be thoroughly tested to ensure it handles all edge cases and potential errors correctly.
| filename, | ||
| pathParameters, | ||
| ); | ||
| await this.add({ |
There was a problem hiding this comment.
Avoid using this in a static context.
Using this in a static context can be confusing. Use the class name instead.
- await this.add({
+ await PresignedUrl.add({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.
| await this.add({ | |
| await PresignedUrl.add({ |
Tools
Biome
[error] 178-178: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Change Summary
Change type