fix: add image preview and download link for upload input v3#257
Merged
Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUploaded-file rows now show linked image thumbnails (ProgressiveImg) with a local ChangesUploaded file thumbnail and download rendering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/inputs/upload-input-v3/index.js`:
- Around line 386-387: The preview/download anchors currently render with
href={src} even when src is empty, producing broken links; update the render
logic in the UploadInputV3 component (the JSX that outputs the <a href={src}
...> preview and download anchors) to conditionally render those <a> elements
only when src is truthy, and otherwise render a non-interactive fallback (e.g.,
a <span> or icon button with no href and appropriate aria-disabled) so the UI
isn’t clickable when no URL exists; apply this gate to both the preview anchor
and the download anchor render paths that reference the src variable.
- Around line 390-391: The onError handler for the image fallback currently sets
e.target.src = file_icon but leaves the same handler in place, risking an
infinite loop if the fallback also fails; update the onError callback used on
the image element (the onError prop in this component) to first remove or
nullify the handler (e.g., e.target.onerror = null or e.currentTarget.onerror =
null) and then set the fallback src, and optionally guard by checking if
e.target.src is already the fallback before assigning to avoid reassigning the
same value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95d83902-9a42-48cc-8c43-de00acf18754
📒 Files selected for processing (1)
src/components/inputs/upload-input-v3/index.js
Comment on lines
+386
to
+387
| <a href={src} target="_blank" rel="noreferrer" title="Preview file"> | ||
| <img |
There was a problem hiding this comment.
Avoid rendering preview/download anchors when URL is missing.
Line 386 and Line 399 can render <a> with an empty/undefined href, which creates broken interactive UI (click does nothing). Gate anchors on src and render plain text/icon when absent.
Proposed fix
-<a href={src} target="_blank" rel="noreferrer" title="Preview file">
- <img
- src={previewSrc}
- alt={filename}
- onError={(e) => { e.target.src = file_icon; }}
- style={{ width: 70, height: 70, objectFit: 'contain', display: 'block', borderRadius: 4 }}
- />
-</a>
+{src ? (
+ <a href={src} target="_blank" rel="noreferrer" title="Preview file">
+ <img
+ src={previewSrc}
+ alt={filename}
+ onError={(e) => { e.currentTarget.onerror = null; e.currentTarget.src = file_icon; }}
+ style={{ width: 70, height: 70, objectFit: 'contain', display: 'block', borderRadius: 4 }}
+ />
+ </a>
+) : (
+ <img
+ src={file_icon}
+ alt={filename}
+ style={{ width: 70, height: 70, objectFit: 'contain', display: 'block', borderRadius: 4 }}
+ />
+)}
@@
-<Typography
- component="a"
- href={src}
- target="_blank"
- rel="noreferrer"
- download
+<Typography
+ component={src ? "a" : "span"}
+ href={src || undefined}
+ target={src ? "_blank" : undefined}
+ rel={src ? "noreferrer" : undefined}
+ download={Boolean(src)}Also applies to: 398-403
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/inputs/upload-input-v3/index.js` around lines 386 - 387, The
preview/download anchors currently render with href={src} even when src is
empty, producing broken links; update the render logic in the UploadInputV3
component (the JSX that outputs the <a href={src} ...> preview and download
anchors) to conditionally render those <a> elements only when src is truthy, and
otherwise render a non-interactive fallback (e.g., a <span> or icon button with
no href and appropriate aria-disabled) so the UI isn’t clickable when no URL
exists; apply this gate to both the preview anchor and the download anchor
render paths that reference the src variable.
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
smarcet
reviewed
May 27, 2026
| let src = file?.private_url || file?.public_url; | ||
| if (src === '#') src = file?.public_url; | ||
| // custom replace for dropbox case ( download vs raw) | ||
| const previewSrc = src ? src.replace("?dl=0", "?raw=1") : filename; |
Collaborator
There was a problem hiding this comment.
@tomrndom this logic is not correct u are asumming that files are always an image, file can have any ext please do check v2 implementation that use https://github.com/OpenStackweb/openstack-uicore-foundation/blob/main/src/components/progressive-img/index.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js (1)
327-333: ⚡ Quick winStrengthen fallback assertion to avoid false positives.
The current check passes for any non-original
src(including empty/invalid). Assert the expected fallback asset value instead.Suggested test tightening
test('preview image falls back to file_icon on load error', () => { const files = [{ filename: 'document.pdf', size: 102400, public_url: 'https://cdn.example.com/document.pdf' }]; render(<UploadInputV3 {...defaultProps} value={files} />); const img = screen.getByRole('img', { name: 'document.pdf' }); fireEvent.error(img); - expect(img).not.toHaveAttribute('src', 'https://cdn.example.com/document.pdf'); + expect(img.getAttribute('src')).toContain('fileMock'); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js` around lines 327 - 333, The test "preview image falls back to file_icon on load error" currently asserts the src is not the original URL which can yield false positives; update it to assert the exact fallback asset is used after error: after firing fireEvent.error(img) assert that img has the expected fallback src (the component's file_icon asset) by checking expect(img).toHaveAttribute('src', '<expected file_icon path or variable>') or compare against the imported/defined file_icon constant used by UploadInputV3 so the assertion explicitly verifies the fallback image is rendered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js`:
- Around line 327-333: The test "preview image falls back to file_icon on load
error" currently asserts the src is not the original URL which can yield false
positives; update it to assert the exact fallback asset is used after error:
after firing fireEvent.error(img) assert that img has the expected fallback src
(the component's file_icon asset) by checking expect(img).toHaveAttribute('src',
'<expected file_icon path or variable>') or compare against the imported/defined
file_icon constant used by UploadInputV3 so the assertion explicitly verifies
the fallback image is rendered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9cc56f35-7ec0-4294-8f40-a0c9bb669ec3
📒 Files selected for processing (3)
package.jsonsrc/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.jssrc/components/inputs/upload-input-v3/index.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/inputs/upload-input-v3/index.js
Contributor
santipalenque
left a comment
santipalenque
left a comment
There was a problem hiding this comment.
other than seba's comment this looks good to me
smarcet
reviewed
Jun 1, 2026
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/i18n/en.json (1)
142-149:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix invalid JSON in
src/i18n/en.json(breaks i18n loading)
jqreports a parse error at line 150, column 15 (Expected separator between values), consistent withupload_input_v3missing its closing}and trailing comma before the"grid_filter"section.🐛 Proposed fix
"upload_input_v3": { "no_post_url": "No Post URL", "upload_disabled": "Upload has been disabled by administrators.", "click_upload": "Click to upload", "drag_and_drop": "or drag and drop", "see_preview": "See Preview", "preview_file": "Preview file", "complete": "Complete" - "grid_filter": { + }, + "grid_filter": {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/i18n/en.json` around lines 142 - 149, The JSON for the "upload_input_v3" object is malformed (missing its closing brace and the trailing comma before the next "grid_filter" section), causing i18n load errors; fix by closing the "upload_input_v3" object with a } and adding the required comma after that object so the subsequent "grid_filter" key is a valid sibling, ensuring the file becomes valid JSON (locate the "upload_input_v3" block in src/i18n/en.json to apply the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/i18n/en.json`:
- Around line 142-149: The JSON for the "upload_input_v3" object is malformed
(missing its closing brace and the trailing comma before the next "grid_filter"
section), causing i18n load errors; fix by closing the "upload_input_v3" object
with a } and adding the required comma after that object so the subsequent
"grid_filter" key is a valid sibling, ensuring the file becomes valid JSON
(locate the "upload_input_v3" block in src/i18n/en.json to apply the change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0248b2cd-0545-4a0b-94ce-36d252963731
📒 Files selected for processing (2)
package.jsonsrc/i18n/en.json
✅ Files skipped from review due to trivial changes (1)
- package.json
smarcet
force-pushed
the
fix/upload-input-v3-preview-download
branch
from
da2977c to
8efd84c
Compare
smarcet
added a commit
that referenced
this pull request
Jun 1, 2026
* fix: add image preview and download link for upload input v3 Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> * fix: add unit test cases for preview image and download link Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> * fix: adjust route for fileMock at jest config Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> * fix: change preview component, fix src, adjust tests Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> * fix: add literals to i18n file Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> * fix: adjust test file to use i18n file Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> --------- Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com> Co-authored-by: sebastian marcet <smarcet@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref: https://app.clickup.com/t/86b9axe92
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation