updated toast logic and action buttons logic - #67
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Dashboard/shared/InactiveMenuModal.jsx (1)
119-134: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOther rows' buttons look enabled but silently no-op while a different item is uploading.
handleAddPhotoClickblocks any click whileuploadingItemIdis truthy (Line 18), but the per-rowdisabled(Line 121) only checksuploadingItemId === item.id, not other items. So while item A uploads, item B's "Add Photo" button still renders as active/clickable, yet clicking it silently does nothing due to the handler guard — confusing UX.🐛 Suggested fix — disable non-uploading rows too while any upload is active
<button onClick={() => handleAddPhotoClick(item.id)} - disabled={uploadingItemId === item.id || uploadedItemId === item.id} + disabled={ + uploadingItemId === item.id || + uploadedItemId === item.id || + (uploadingItemId !== null && uploadingItemId !== item.id) + } className="flex items-center gap-2 px-4 py-2 bg-orange-50 hover:bg-orange-100 text-orange-600 rounded-xl text-[13px] font-bold transition-colors shadow-sm disabled:opacity-75 disabled:cursor-not-allowed" >🤖 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/Dashboard/shared/InactiveMenuModal.jsx` around lines 119 - 134, The per-row “Add Photo” state in InactiveMenuModal is inconsistent with the guard in handleAddPhotoClick: only the currently uploading item is disabled, while other rows still look clickable even though the click is ignored. Update the button disabled logic for this row to also cover the case where any upload is in progress, and keep the label/state behavior aligned with uploadingItemId and uploadedItemId so inactive rows don’t appear enabled during another item’s upload.
🧹 Nitpick comments (2)
src/components/Dashboard/InventoryAlerts.jsx (1)
42-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated, hardcoded threshold copy.
The "100g / 100ml" threshold text is duplicated verbatim in two places and hardcoded as UI copy, disconnected from whatever logic actually computes
data.lowStock. If the real threshold changes, these strings won't reflect it and could mislead users.Consider extracting a shared constant/component (e.g.
LOW_STOCK_THRESHOLD_TEXTor a small<ThresholdHint />) so both locations stay in sync, and ideally derive the values from the same source used to computelowStock.Also applies to: 86-88
🤖 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/Dashboard/InventoryAlerts.jsx` around lines 42 - 45, The low-stock threshold copy in InventoryAlerts is duplicated and hardcoded, so update the threshold hint and the other matching display to use a shared source of truth instead of literal “100g / 100ml” text. Extract a reusable constant or small component such as LOW_STOCK_THRESHOLD_TEXT or ThresholdHint, and have both the hint and any lowStock-related UI reference the same value used by the logic that computes data.lowStock.src/utils/toastUtils.js (1)
41-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate timer-cancellation logic across
success/error/info.The same 4-line "cancel pending timer if present" block is repeated verbatim in three methods. Extracting a small helper would reduce duplication and the chance of the three copies drifting apart.
♻️ Suggested consolidation
+const cancelPendingDismissal = (id) => { + if (id !== undefined && pendingDismissals.has(id)) { + clearTimeout(pendingDismissals.get(id)); + pendingDismissals.delete(id); + } +}; + export const toast = { ... success: (message, options = {}) => { - const targetId = options.id; - if (targetId !== undefined && pendingDismissals.has(targetId)) { - clearTimeout(pendingDismissals.get(targetId)); - pendingDismissals.delete(targetId); - } + cancelPendingDismissal(options.id); return sonnerToast.success(message, { duration: 6000, ...options }); },Additionally, note that many callers (e.g.
useIngredients.js,useMenuItems.js) explicitly passduration: 6000totoast.loading(...)calls, which per this file's own docstring is ignored by Sonner for loading variants — that's harmless dead weight but worth cleaning up opportunistically.🤖 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/utils/toastUtils.js` around lines 41 - 68, The `success`, `error`, and `info` methods in `toastUtils` repeat the same pending-dismissal cleanup, so extract that shared logic into a small helper and call it from each method. Keep the helper responsible for checking `options.id`, clearing any timer in `pendingDismissals`, and deleting the entry, then reuse it in `sonnerToast.success`, `sonnerToast.error`, and `sonnerToast.info` to keep the behavior identical and avoid drift.
🤖 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/utils/toastUtils.js`:
- Around line 21-39: The forced 6-second dismiss in toast.loading is too
aggressive and can hide long-running loading toasts before success/error
arrives. Update the pendingDismissals timeout logic in toastUtils.js so the max
lifetime is decoupled from the visible loading duration, either by removing the
unconditional forced dismiss or replacing it with a much longer ceiling, while
keeping the existing id-based replacement behavior in toast.loading intact.
---
Outside diff comments:
In `@src/components/Dashboard/shared/InactiveMenuModal.jsx`:
- Around line 119-134: The per-row “Add Photo” state in InactiveMenuModal is
inconsistent with the guard in handleAddPhotoClick: only the currently uploading
item is disabled, while other rows still look clickable even though the click is
ignored. Update the button disabled logic for this row to also cover the case
where any upload is in progress, and keep the label/state behavior aligned with
uploadingItemId and uploadedItemId so inactive rows don’t appear enabled during
another item’s upload.
---
Nitpick comments:
In `@src/components/Dashboard/InventoryAlerts.jsx`:
- Around line 42-45: The low-stock threshold copy in InventoryAlerts is
duplicated and hardcoded, so update the threshold hint and the other matching
display to use a shared source of truth instead of literal “100g / 100ml” text.
Extract a reusable constant or small component such as LOW_STOCK_THRESHOLD_TEXT
or ThresholdHint, and have both the hint and any lowStock-related UI reference
the same value used by the logic that computes data.lowStock.
In `@src/utils/toastUtils.js`:
- Around line 41-68: The `success`, `error`, and `info` methods in `toastUtils`
repeat the same pending-dismissal cleanup, so extract that shared logic into a
small helper and call it from each method. Keep the helper responsible for
checking `options.id`, clearing any timer in `pendingDismissals`, and deleting
the entry, then reuse it in `sonnerToast.success`, `sonnerToast.error`, and
`sonnerToast.info` to keep the behavior identical and avoid drift.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 936c0b1d-0d65-458d-a645-a73ac68195c3
📒 Files selected for processing (16)
src/Layout/AppLayout.jsxsrc/components/Dashboard/ChefMenuView.jsxsrc/components/Dashboard/DashboardHeader.jsxsrc/components/Dashboard/IngredientsView.jsxsrc/components/Dashboard/InventoryAlerts.jsxsrc/components/Dashboard/MenuManagementView.jsxsrc/components/Dashboard/RecipeBuilderView.jsxsrc/components/Dashboard/shared/ConfirmModal.jsxsrc/components/Dashboard/shared/InactiveMenuModal.jsxsrc/components/Dashboard/shared/IngredientModal.jsxsrc/hooks/dashboard/useIngredients.jssrc/hooks/dashboard/useKitchenOrders.jssrc/hooks/dashboard/useMenuItems.jssrc/hooks/dashboard/useMenuUploads.jssrc/hooks/dashboard/useOrders.jssrc/utils/toastUtils.js
| export const toast = { | ||
| loading: (message, options = {}) => { | ||
| const id = sonnerToast.loading(message, options); | ||
| const targetId = options.id !== undefined ? options.id : id; | ||
|
|
||
| // Cancel any existing timer for this ID (e.g., progress-update calls with same id) | ||
| if (pendingDismissals.has(targetId)) { | ||
| clearTimeout(pendingDismissals.get(targetId)); | ||
| } | ||
|
|
||
| // Set a max lifetime of 6 seconds so loading toasts don't stay on screen forever | ||
| const timerId = setTimeout(() => { | ||
| sonnerToast.dismiss(targetId); | ||
| pendingDismissals.delete(targetId); | ||
| }, 6000); | ||
|
|
||
| pendingDismissals.set(targetId, timerId); | ||
| return id; | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Forced 6s dismiss can hide the loading toast mid-operation.
The forced-dismiss timer starts the moment loading() is called and fires unconditionally at 6000ms unless something (a progress update, success, or error) resets/cancels it in the meantime. Most consumers of this wrapper (useUpdateIngredientStock, useUpdateOrderStatus, kitchen status update, useDeleteMenuItem, useSaveRecipe, and useUpdateMenuItem/useCreateMenuItem without an image) issue a single loading() call with no intermediate reset. Any of these network calls that take longer than 6 seconds (slow API, cold start, throttled network) will have their loading toast silently dismissed while the mutation is still in flight, then a new toast will pop back up when success/error eventually fires with the reused id — producing a confusing "toast disappears, then reappears out of nowhere" UX. Even progress-tracked uploads (useUploadIngredients, useUploadMenu) are only protected during active onUploadProgress ticks; a gap during server-side processing after the upload completes has the same exposure.
Consider decoupling the forced max-lifetime from the visible duration (e.g., a much larger ceiling like 30–60s, or no forced dismiss at all — since success/error already correctly replace the loading toast via id).
💡 Suggested direction
- // Set a max lifetime of 6 seconds so loading toasts don't stay on screen forever
- const timerId = setTimeout(() => {
+ // Set a generous max lifetime so loading toasts don't stay on screen forever,
+ // while giving slow/no-progress operations enough headroom to complete normally.
+ const timerId = setTimeout(() => {
sonnerToast.dismiss(targetId);
pendingDismissals.delete(targetId);
- }, 6000);
+ }, options.maxLifetime ?? 30000);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const toast = { | |
| loading: (message, options = {}) => { | |
| const id = sonnerToast.loading(message, options); | |
| const targetId = options.id !== undefined ? options.id : id; | |
| // Cancel any existing timer for this ID (e.g., progress-update calls with same id) | |
| if (pendingDismissals.has(targetId)) { | |
| clearTimeout(pendingDismissals.get(targetId)); | |
| } | |
| // Set a max lifetime of 6 seconds so loading toasts don't stay on screen forever | |
| const timerId = setTimeout(() => { | |
| sonnerToast.dismiss(targetId); | |
| pendingDismissals.delete(targetId); | |
| }, 6000); | |
| pendingDismissals.set(targetId, timerId); | |
| return id; | |
| }, | |
| export const toast = { | |
| loading: (message, options = {}) => { | |
| const id = sonnerToast.loading(message, options); | |
| const targetId = options.id !== undefined ? options.id : id; | |
| // Cancel any existing timer for this ID (e.g., progress-update calls with same id) | |
| if (pendingDismissals.has(targetId)) { | |
| clearTimeout(pendingDismissals.get(targetId)); | |
| } | |
| // Set a generous max lifetime so loading toasts don't stay on screen forever, | |
| // while giving slow/no-progress operations enough headroom to complete normally. | |
| const timerId = setTimeout(() => { | |
| sonnerToast.dismiss(targetId); | |
| pendingDismissals.delete(targetId); | |
| }, options.maxLifetime ?? 30000); | |
| pendingDismissals.set(targetId, timerId); | |
| return id; | |
| }, |
🤖 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/utils/toastUtils.js` around lines 21 - 39, The forced 6-second dismiss in
toast.loading is too aggressive and can hide long-running loading toasts before
success/error arrives. Update the pendingDismissals timeout logic in
toastUtils.js so the max lifetime is decoupled from the visible loading
duration, either by removing the unconditional forced dismiss or replacing it
with a much longer ceiling, while keeping the existing id-based replacement
behavior in toast.loading intact.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes