fix: replace image popover with image preview dialog#965
Open
priscila-moneo wants to merge 3 commits into
Open
fix: replace image popover with image preview dialog#965priscila-moneo wants to merge 3 commits into
priscila-moneo wants to merge 3 commits into
Conversation
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
|
Looking for one thing? Review this PR in Change Stack to search files, summaries, diffs, and code without losing your place. 📝 WalkthroughWalkthroughThis PR introduces a shared 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
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
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
1 participant
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
New Features
Refactor
Tests