fix: replace image popover with image preview dialog#965
Open
priscila-moneo wants to merge 4 commits into
Open
fix: replace image popover with image preview dialog#965priscila-moneo wants to merge 4 commits into
priscila-moneo wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughAdds a shared ImagePreviewCell and isImageUrl utility, refactors PreviewModal to handle image load failures with a broken-image placeholder, updates pages to use the shared preview component and requests image metadata, and adds/updates unit tests and i18n. ChangesImage Preview Centralization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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: 3
🧹 Nitpick comments (1)
src/components/__tests__/image-preview-cell.test.js (1)
40-54: ⚡ Quick winAdd a regression test for malformed URL-encoded filenames.
Given the filename derivation path, please add a case with an invalid encoded segment (e.g.,
%E0%A4%A) to ensure the component doesn’t crash and still renders a fallback filename.🤖 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/__tests__/image-preview-cell.test.js` around lines 40 - 54, Add a new test in src/components/__tests__/image-preview-cell.test.js that renders <ImagePreviewCell> without a fileName but with an imageUrl whose last path segment contains a malformed percent-encoding (e.g., "%E0%A4%A"), then simulate the click on the preview button and assert the component does not crash and shows a filename/fallback; specifically, reuse the existing test pattern (userEvent.setup(), render(<ImagePreviewCell imageUrl={badUrl} />), await user.click(screen.getByRole("button", { name: title }))) and assert the UI contains either a safe fallback or the raw segment using a tolerant matcher (for example expect(screen.getByText(/sponsor_banner\.png|%E0%A4%A/)).toBeInTheDocument()), so the test verifies resilience of ImagePreviewCell's filename extraction logic.
🤖 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/image-preview-cell.js`:
- Around line 26-27: The filename decoding step for resolvedFileName can throw
on malformed percent-encodings; wrap the decodeURIComponent call that uses
imageUrl.split("/").pop().split("?")[0] in a safe guard: try to decode and on
any exception fall back to the raw segment (or a sanitized fallback like an
empty string); update the expression that assigns resolvedFileName (refer to
resolvedFileName, imageUrl) to use this safe decode logic so render-time errors
are avoided.
In `@src/pages/sponsors-global/form-templates/form-template-item-list-page.js`:
- Around line 209-213: The render callback currently destructures file_url from
row.images[0] which can throw if the first array element is nullish; update the
render implementation used for the column to read the URL via optional chaining
(e.g., access row.images?.[0]?.file_url) before calling isImageUrl, and only
return <ImagePreviewCell imageUrl={url} /> when url is a valid string and
isImageUrl(url) is true; keep the checks around row.images length/existence and
use the same symbols (render, row.images, isImageUrl, ImagePreviewCell) so the
change is localized to that render function.
In `@src/utils/methods.js`:
- Around line 637-641: getFileUploadAllowedExtensions currently returns raw
tokens without trimming, so inputs like "jpg, png, " produce entries with
whitespace; update the function (getFileUploadAllowedExtensions) to normalize
tokens by mapping each element (whether coming from an array or from
String(ext).split(",")) through .toString()/String(...) then .trim(), and then
filter(Boolean) to remove empty strings—i.e., convert ext to an array, trim
every item, and filter out falsy values before returning.
---
Nitpick comments:
In `@src/components/__tests__/image-preview-cell.test.js`:
- Around line 40-54: Add a new test in
src/components/__tests__/image-preview-cell.test.js that renders
<ImagePreviewCell> without a fileName but with an imageUrl whose last path
segment contains a malformed percent-encoding (e.g., "%E0%A4%A"), then simulate
the click on the preview button and assert the component does not crash and
shows a filename/fallback; specifically, reuse the existing test pattern
(userEvent.setup(), render(<ImagePreviewCell imageUrl={badUrl} />), await
user.click(screen.getByRole("button", { name: title }))) and assert the UI
contains either a safe fallback or the raw segment using a tolerant matcher (for
example
expect(screen.getByText(/sponsor_banner\.png|%E0%A4%A/)).toBeInTheDocument()),
so the test verifies resilience of ImagePreviewCell's filename extraction logic.
🪄 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: 737d8ee1-de60-4ff6-92d2-78089fe1cd24
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
src/components/__tests__/image-preview-cell.test.jssrc/components/image-preview-cell.jssrc/components/mui/PreviewModal/index.jsxsrc/components/mui/__tests__/preview-modal.test.jssrc/i18n/en.jsonsrc/pages/sponsors-global/form-templates/form-template-item-list-page.jssrc/pages/sponsors-global/inventory/__tests__/inventory-list-page.test.jssrc/pages/sponsors-global/inventory/inventory-list-page.jssrc/utils/__tests__/methods.test.jssrc/utils/methods.js
💤 Files with no reviewable changes (1)
- src/pages/sponsors-global/inventory/tests/inventory-list-page.test.js
|
|
||
| useEffect(() => { | ||
| if (open) setImageError(false); | ||
| }, [open, url]); |
There was a problem hiding this comment.
url shouldn't be a dependecy here
| ) : null | ||
| render: (row) => { | ||
| const url = row.images?.[0]?.file_url ?? row.images?.[0]?.file_path; | ||
| if (!url || !isImageUrl(url)) return null; |
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/pages/sponsors-global/inventory/inventory-list-page.js (1)
172-180:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
file_pathfallback inconsistency.Lines 148-152, 147-151, and 162-166 in the other files changed by this PR all use the pattern
row.images?.[0]?.file_url ?? row.images?.[0]?.file_pathto fall back tofile_pathwhenfile_urlis unavailable. This file only checksfile_url, which may prevent images from displaying whenfile_urlis missing butfile_pathexists.🔧 Suggested fix to align with other files
render: (row) => { const hasImages = Array.isArray(row.images) && row.images.length > 0; - const imageUrl = row.images?.[0]?.file_url; + const imageUrl = row.images?.[0]?.file_url ?? row.images?.[0]?.file_path; const itemName = row.name; if (!hasImages || !imageUrl) return null;🤖 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/pages/sponsors-global/inventory/inventory-list-page.js` around lines 172 - 180, The render function for the image cell currently only reads row.images?.[0]?.file_url which can be undefined even when row.images?.[0]?.file_path exists; update the imageUrl assignment in the render (the variable used when rendering ImagePreviewCell) to use a fallback: imageUrl = row.images?.[0]?.file_url ?? row.images?.[0]?.file_path, and adjust the hasImages/imageUrl existence check so the ImagePreviewCell is rendered when either URL or file_path is present while keeping itemName unchanged.
🧹 Nitpick comments (1)
src/pages/sponsors-global/inventory/inventory-list-page.js (1)
173-177: 💤 Low valueRedundant
hasImagescheck.The
hasImagescheck is unnecessary becauseimageUrlalready uses optional chaining (row.images?.[0]?.file_url). Ifrow.imagesis not an array or is empty,imageUrlwill beundefined, which is caught by the!imageUrlcondition.♻️ Optional simplification
render: (row) => { - const hasImages = Array.isArray(row.images) && row.images.length > 0; const imageUrl = row.images?.[0]?.file_url ?? row.images?.[0]?.file_path; const itemName = row.name; - if (!hasImages || !imageUrl) return null; + if (!imageUrl) return null; return <ImagePreviewCell imageUrl={imageUrl} itemName={itemName} />;🤖 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/pages/sponsors-global/inventory/inventory-list-page.js` around lines 173 - 177, The hasImages boolean is redundant because imageUrl already uses optional chaining; remove the hasImages declaration and simplify the guard to only check imageUrl (i.e., drop the Array.isArray(row.images) && row.images.length > 0 check and keep the existing imageUrl logic). Locate the block using variables hasImages, imageUrl, and itemName in inventory-list-page.js and delete the hasImages line and the combined if (!hasImages || !imageUrl) guard, replacing it with a single if (!imageUrl) return null to preserve behavior.
🤖 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/pages/sponsors-global/inventory/inventory-list-page.js`:
- Around line 172-180: The render function for the image cell currently only
reads row.images?.[0]?.file_url which can be undefined even when
row.images?.[0]?.file_path exists; update the imageUrl assignment in the render
(the variable used when rendering ImagePreviewCell) to use a fallback: imageUrl
= row.images?.[0]?.file_url ?? row.images?.[0]?.file_path, and adjust the
hasImages/imageUrl existence check so the ImagePreviewCell is rendered when
either URL or file_path is present while keeping itemName unchanged.
---
Nitpick comments:
In `@src/pages/sponsors-global/inventory/inventory-list-page.js`:
- Around line 173-177: The hasImages boolean is redundant because imageUrl
already uses optional chaining; remove the hasImages declaration and simplify
the guard to only check imageUrl (i.e., drop the Array.isArray(row.images) &&
row.images.length > 0 check and keep the existing imageUrl logic). Locate the
block using variables hasImages, imageUrl, and itemName in
inventory-list-page.js and delete the hasImages line and the combined if
(!hasImages || !imageUrl) guard, replacing it with a single if (!imageUrl)
return null to preserve behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01456fa8-7e55-4f51-b543-2d94a737e62f
📒 Files selected for processing (9)
src/app.jssrc/components/image-preview-cell.jssrc/components/mui/PreviewModal/index.jsxsrc/pages/sponsors-global/form-templates/add-form-template-item-popup.jssrc/pages/sponsors-global/form-templates/form-template-item-list-page.jssrc/pages/sponsors-global/inventory/inventory-list-page.jssrc/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-add-item-from-inventory-popup.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/components/manage-items/sponsor-form-item-from-inventory.jssrc/utils/methods.js
🚧 Files skipped from review as they are similar to previous changes (4)
- src/utils/methods.js
- src/pages/sponsors-global/form-templates/form-template-item-list-page.js
- src/components/image-preview-cell.js
- src/components/mui/PreviewModal/index.jsx
santipalenque
approved these changes
Jun 8, 2026
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
priscila-moneo
force-pushed
the
fix/form-template-grid-image-preview
branch
2 times, most recently
from
4cea787 to
ce3f134
Compare
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
priscila-moneo
force-pushed
the
fix/form-template-grid-image-preview
branch
from
ce3f134 to
e2edcf0
Compare
smarcet
reviewed
Jun 9, 2026
| </Tooltip> | ||
| ) : null | ||
| render: (row) => { | ||
| const url = row.images?.[0]?.file_url; |
There was a problem hiding this comment.
Inconsistent URL resolution compared to the other migrated consumers. add-form-template-item-popup.js and both sponsor pages use file_url ?? file_path as a fallback, but this file only reads file_url. If any inventory item has file_path set but not file_url, those list rows will silently show no preview icon while the popup pages will show one for the same item.
Consider applying the same fallback here:
const url = row.images?.[0]?.file_url ?? row.images?.[0]?.file_path;Or, if file_url is always guaranteed to be populated for inventory images, remove the ?? file_path fallback from the popup consumers to make the intent explicit.
smarcet
reviewed
Jun 9, 2026
| <ImageIcon fontSize="small" /> | ||
| </IconButton> | ||
|
|
||
| <PreviewModal |
There was a problem hiding this comment.
PreviewModal is always mounted here — every row in the table keeps a PreviewModal instance in the React tree even when it's never been opened. The project convention (popup-dialog-pattern rule) is to use conditional rendering so dialogs are mounted only when open.
{open && (
<PreviewModal
title={itemName || T.translate("preview_modal.title")}
open
onClose={() => setOpen(false)}
url={imageUrl}
filename={resolvedFileName}
uploadDate={uploadDate}
/>
)}MUI Dialog with open=false doesn't add DOM nodes (no keepMounted), so the practical impact is React tree overhead — not stale state — but for large tables this mounts N dialog instances unnecessarily.
smarcet
requested changes
Jun 9, 2026
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@priscila-moneo please review comments
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
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/86ba8twjm
2nd commit:
Extended the ImagePreviewCell migration from the first commit to the remaining sponsor form item pages that had the same pattern.
Summary by CodeRabbit