fix: add inventory item image preview#819
Merged
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughAdded three i18n keys and a reusable exported InventoryImagePreviewCell component (uses MUI Popover for a 220×220 preview with load-failure fallback and click-to-open). Replaced inline image rendering in the inventory list, updated useEffect deps, and added tests for preview behavior and accessibility. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant InventoryPage as InventoryPage
participant Popover as Popover (Preview)
participant Browser as Browser
User->>InventoryPage: hover or focus preview button
InventoryPage->>Popover: open with imageUrl, set size 220x220
Popover->>User: render image or fallback text (onError)
User->>InventoryPage: click preview/button
InventoryPage->>Browser: window.open(imageUrl, "_blank", "noopener,noreferrer")
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/sponsors-global/inventory/inventory-list-page.js`:
- Around line 171-213: The render function currently dereferences row.images
without guarding for null/undefined; update the render lambda (the render: (row)
=> ...) to first check that row.images is an array (e.g. row.images &&
row.images.length > 0) before accessing row.images[0] and
row.images[0].file_url, and use optional chaining where helpful
(row.images?.[0]?.file_url) for both the Tooltip src/alt and the IconButton
onClick so the UI won't throw when the relationship is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d96d6c6-8f89-4ba5-927b-78ce911f2f19
📒 Files selected for processing (2)
src/i18n/en.jsonsrc/pages/sponsors-global/inventory/inventory-list-page.js
priscila-moneo
force-pushed
the
fix/add-inventory-item-image-preview
branch
from
26a11d4 to
dbe5ae4
Compare
martinquiroga-exo
requested changes
Mar 10, 2026
martinquiroga-exo
left a comment
martinquiroga-exo
left a comment
There was a problem hiding this comment.
@priscila-moneo please see comments.
| if (!hasImages || !imageUrl) return null; | ||
|
|
||
| return ( | ||
| <Tooltip |
There was a problem hiding this comment.
For this use case we should use Popover instead of Tooltip
| overflow: "hidden" | ||
| }} | ||
| > | ||
| <Box |
There was a problem hiding this comment.
This will cause the browser to eagerly load the images. We want preview images to be lazy loaded, conditionally mount them using Popover
| > | ||
| <Box | ||
| component="img" | ||
| src={imageUrl} |
There was a problem hiding this comment.
There is no fallback we could be rendering a broken preview if the image load fails for instance. Currently the code prevents rendering when the URL is missing but not if the link is broken or there is a network issue, etc.
priscila-moneo
force-pushed
the
fix/add-inventory-item-image-preview
branch
from
dbe5ae4 to
e3a0014
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/sponsors-global/inventory/inventory-list-page.js`:
- Around line 177-179: The useEffect is causing duplicate dispatches because
handlers (onPageChange/onSort/onSearch/onFilter) already call getInventoryItems
and the effect also re-dispatches when mapped props change; remove the duplicate
by changing the useEffect to run only once on mount (useEffect(..., [])) and
call getInventoryItems with the current mapped page prop instead of the
hard-coded 1 (use getInventoryItems(term, page, perPage, order, orderDir,
hideArchived)), or alternatively delete this useEffect entirely and keep the
handler-driven calls—ensure you update the effect (or remove it) so it no longer
depends on getInventoryItems, term, perPage, order, orderDir, hideArchived to
avoid double requests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e84f382-ab5a-49e1-a3bd-a2026cf7e17a
📒 Files selected for processing (3)
src/i18n/en.jsonsrc/pages/sponsors-global/inventory/__tests__/inventory-list-page.test.jssrc/pages/sponsors-global/inventory/inventory-list-page.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/i18n/en.json
priscila-moneo
force-pushed
the
fix/add-inventory-item-image-preview
branch
from
e3a0014 to
77bca5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pages/sponsors-global/inventory/inventory-list-page.js (1)
177-185:⚠️ Potential issue | 🟡 MinorPreserve the stored page on mount.
Line 180 still hard-resets the first fetch to page 1. If this screen is remounted after paging through the list, the Redux-backed list state is discarded and users get bounced back to the first page.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors-global/inventory/inventory-list-page.js` around lines 177 - 185, The useEffect currently forces the first fetch to DEFAULT_CURRENT_PAGE which resets Redux-backed paging on remount; change the call to getInventoryItems(...) to use the stored page value (e.g., currentPage / inventory.currentPage from props or selector) instead of DEFAULT_CURRENT_PAGE, falling back to DEFAULT_CURRENT_PAGE only if that stored page is undefined/null; update the useEffect dependencies if needed so it reads the stored page value on mount and does not always reset to 1.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/sponsors-global/inventory/inventory-list-page.js`:
- Around line 57-79: The accessible label (previewAlt) used on the IconButton is
generic and doesn't communicate the click action that opens the asset in a new
tab; update the component to accept the item name (e.g., itemName prop) and
build an action-oriented aria-label (replace previewAlt on the IconButton) that
includes the item name and mentions the new-tab behavior (for example: "Open
image preview for {itemName} in a new tab"), while keeping the existing
hover/focus handlers (openPreview, closePreview) and the onClick
window.open(imageUrl, "_blank", "noopener,noreferrer") behavior unchanged.
---
Duplicate comments:
In `@src/pages/sponsors-global/inventory/inventory-list-page.js`:
- Around line 177-185: The useEffect currently forces the first fetch to
DEFAULT_CURRENT_PAGE which resets Redux-backed paging on remount; change the
call to getInventoryItems(...) to use the stored page value (e.g., currentPage /
inventory.currentPage from props or selector) instead of DEFAULT_CURRENT_PAGE,
falling back to DEFAULT_CURRENT_PAGE only if that stored page is undefined/null;
update the useEffect dependencies if needed so it reads the stored page value on
mount and does not always reset to 1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34b1fa9c-e171-46f6-9313-77fd4548810a
📒 Files selected for processing (3)
src/i18n/en.jsonsrc/pages/sponsors-global/inventory/__tests__/inventory-list-page.test.jssrc/pages/sponsors-global/inventory/inventory-list-page.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/sponsors-global/inventory/tests/inventory-list-page.test.js
priscila-moneo
force-pushed
the
fix/add-inventory-item-image-preview
branch
from
77bca5a to
fd56ed7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/sponsors-global/inventory/inventory-list-page.js`:
- Around line 59-63: The previewAlt variable and the IconButton label are
reusing the action-oriented translation key, causing the <img> to have an
imperative alt; instead, use two separate translation keys and values: keep
"inventory_item_list.image_preview_action_alt" (with itemName) for the
IconButton's aria-label/label and switch the <img>'s alt to a descriptive key
like "inventory_item_list.image_preview_alt" (also accept itemName if needed for
context); update the code references around previewAlt, the IconButton props,
and the <img> alt so the button announces the action and the image has a
descriptive alt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 064c253d-ed5e-4d98-898e-a3a53fdb0a01
📒 Files selected for processing (3)
src/i18n/en.jsonsrc/pages/sponsors-global/inventory/__tests__/inventory-list-page.test.jssrc/pages/sponsors-global/inventory/inventory-list-page.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/sponsors-global/inventory/tests/inventory-list-page.test.js
- src/i18n/en.json
priscila-moneo
force-pushed
the
fix/add-inventory-item-image-preview
branch
from
fd56ed7 to
2910cef
Compare
priscila-moneo
force-pushed
the
fix/add-inventory-item-image-preview
branch
from
2910cef to
b1112c3
Compare
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/86b883qz9
Summary by CodeRabbit
New Features
Improvements
Tests