fix: add image preview and download link for upload input v3#257
Open
tomrndom wants to merge 4 commits into
Open
fix: add image preview and download link for upload input v3#257tomrndom wants to merge 4 commits into
tomrndom wants to merge 4 commits into
Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
📝 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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
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