fix/improved error handling in blocks#436
Conversation
…back via toast notifications
… feature/blocks_improved-error-handling
WalkthroughAdded try/catch error handling and destructive localized toast notifications via a global labels context across multiple client-side block components; one server-side file received a non-functional whitespace change. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
… feature/blocks_improved-error-handling
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/blocks/invoice-list/src/frontend/InvoiceList.client.tsx (1)
51-63: Error handling improves user feedback.The try/catch block successfully prevents crashes and provides user feedback via toast, which aligns with the PR objectives.
Consider these optional enhancements:
- Error logging: Log caught errors to aid debugging and monitoring (e.g.,
console.error('Invoice filter failed:', _error)).- Specific error messages: Differentiate between error types (network failures, auth issues, server errors) to provide more actionable feedback to users.
packages/blocks/notification-details/src/frontend/NotificationDetails.client.tsx (1)
49-56: Consider removing label strings from the dependency array.The
labels.errors.requestError.contentandlabels.errors.requestError.titledependencies could trigger unnecessary effect re-runs if the labels object is recreated on each render. Since these values are only referenced in the error handler (not in the effect's core logic), they don't need to be dependencies.Apply this diff to optimize the dependency array:
}, [ component.accessToken, component.id, - labels.errors.requestError.content, - labels.errors.requestError.title, locale, notification.status.value, ]);packages/blocks/order-list/src/frontend/OrderList.client.tsx (1)
60-71: Error handling inhandleFilteris robust; consider deduping toast configWrapping the fetch in
try/catchand surfacing a destructive toast with localizedlabels.errors.requestErrorgreatly improves resilience and UX compared to a hard failure.If you want to DRY things up (since
handleResetuses the same toast), you could extract a tiny helper in this component:const showRequestErrorToast = () => toast({ variant: 'destructive', title: labels.errors.requestError.title, description: labels.errors.requestError.content, });and call
showRequestErrorToast()from both handlers.packages/blocks/service-list/src/frontend/ServiceList.client.tsx (1)
8-11: Consistent localized toast handling for fetch errors; small reuse opportunityThe new error handling in
handleFilterandhandleResetcorrectly prevents the block from crashing and surfaces a localized destructive toast usinglabels.errors.requestError. Logic and control flow look sound.To reduce duplication and keep future changes to the error copy/toast options in one place, you could extract a tiny local helper like
showRequestErrorToast()and reuse it in both handlers (and potentially other blocks following the same pattern).Also applies to: 35-50, 56-66
packages/blocks/category/src/frontend/Category.client.tsx (1)
6-9: Toast-based pagination error handling looks good; consider minor UX polishWrapping
sdk.blocks.getCategoryArticlesin a try/catch and showing a localized destructive toast vialabels.errors.requestErroris a solid improvement and keeps state untouched on failures.Unrelated to this PR’s main goal but worth considering:
Pagination(Line 119–124) still hasdisabled={false}. Wiring that toisPending—as some other blocks do—would prevent duplicate clicks while a fetch is in flight and keep UX consistent across blocks.Also applies to: 35-63
packages/blocks/ticket-list/src/frontend/TicketList.client.tsx (1)
10-13: Robust ticket-list error handling; duplication of toast snippet is acceptable but extractableThe added try/catch in
handleFilterandhandleResetcorrectly guardssdk.blocks.getTicketListand surfaces a localized destructive toast vialabels.errors.requestError, while preserving existing state on failure. This is exactly the behavior described in the PR summary.The toast configuration is duplicated across both handlers; if this pattern continues to spread, consider a small helper (even just within this module) to centralize the “request error” toast shape.
Also applies to: 35-68, 72-84
packages/blocks/article-search/src/frontend/ArticleSearch.client.tsx (1)
7-10: Article search error handling is solid; 60s toast duration may be too aggressiveThe new try/catch around
sdk.blocks.searchArticlesis well done: failures no longer crash the block, stale suggestions are cleared withsetSuggestions([]), and a localized destructive toast is shown vialabels.errors.requestError.The only UX concern is the explicit
duration: 60000(Lines 51–56). A 60‑second destructive toast on every failed keystroke‑driven search can easily become noisy, especially on flaky networks. You might want to either:
- Rely on the default toast duration used elsewhere in the app, or
- Use a shorter/custom duration specifically for search suggestions, possibly gated/debounced for repeated failures.
Also applies to: 32-58
packages/blocks/order-details/src/frontend/OrderDetails.client.tsx (1)
14-17: Order details now fail gracefully; consider minor typing and reuse improvementsThe new try/catch blocks in
handleFilterandhandleResetcorrectly wrapsdk.blocks.getOrderDetails, preserving existing items/filters on failure and surfacing a localized destructive toast vialabels.errors.requestError. That aligns well with the PR objective of avoiding block‑level crashes.If you touch this again, two small cleanups could help:
- Narrow the
handleFilterparameter fromPartial<any>toPartial<Request.GetOrderDetailsBlockQuery>for better type safety.- Extract the repeated toast configuration into a tiny helper to keep the “request failed” UX consistent and easier to adjust.
Also applies to: 106-142, 146-165
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/blocks/article-search/src/frontend/ArticleSearch.client.tsx(2 hunks)packages/blocks/article-search/src/frontend/ArticleSearch.server.tsx(1 hunks)packages/blocks/category/src/frontend/Category.client.tsx(3 hunks)packages/blocks/invoice-list/src/frontend/InvoiceList.client.tsx(1 hunks)packages/blocks/notification-details/src/frontend/NotificationDetails.client.tsx(2 hunks)packages/blocks/notification-list/src/frontend/NotificationList.client.tsx(3 hunks)packages/blocks/order-details/src/frontend/OrderDetails.client.tsx(3 hunks)packages/blocks/order-list/src/frontend/OrderList.client.tsx(3 hunks)packages/blocks/orders-summary/src/frontend/OrdersSummary.client.tsx(3 hunks)packages/blocks/service-list/src/frontend/ServiceList.client.tsx(2 hunks)packages/blocks/ticket-list/src/frontend/TicketList.client.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
packages/blocks/article-search/src/frontend/ArticleSearch.client.tsx (3)
packages/ui/src/providers/GlobalProvider/GlobalProvider.tsx (1)
useGlobalContext(81-81)packages/blocks/article-search/src/api-harmonization/article-search.model.ts (1)
ArticleList(14-16)packages/ui/src/hooks/use-toast.ts (1)
toast(189-189)
packages/blocks/category/src/frontend/Category.client.tsx (2)
packages/ui/src/providers/GlobalProvider/GlobalProvider.tsx (1)
useGlobalContext(81-81)packages/ui/src/hooks/use-toast.ts (1)
toast(189-189)
packages/blocks/service-list/src/frontend/ServiceList.client.tsx (2)
packages/ui/src/providers/GlobalProvider/GlobalProvider.tsx (1)
useGlobalContext(81-81)packages/ui/src/hooks/use-toast.ts (1)
toast(189-189)
packages/blocks/notification-details/src/frontend/NotificationDetails.client.tsx (2)
packages/ui/src/providers/GlobalProvider/GlobalProvider.tsx (1)
useGlobalContext(81-81)packages/ui/src/hooks/use-toast.ts (1)
toast(189-189)
packages/blocks/order-list/src/frontend/OrderList.client.tsx (2)
packages/ui/src/providers/GlobalProvider/GlobalProvider.tsx (1)
useGlobalContext(81-81)packages/ui/src/hooks/use-toast.ts (1)
toast(189-189)
packages/blocks/invoice-list/src/frontend/InvoiceList.client.tsx (1)
packages/ui/src/hooks/use-toast.ts (1)
toast(189-189)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy-preview
🔇 Additional comments (8)
packages/blocks/article-search/src/frontend/ArticleSearch.server.tsx (1)
27-27: Non-functional formatting change.This blank line addition improves readability but has no impact on functionality.
packages/blocks/invoice-list/src/frontend/InvoiceList.client.tsx (1)
69-80: Consistent error handling pattern.The error handling here mirrors the approach in
handleFilter(lines 51-63), maintaining consistency across the component. The same optional enhancements suggested forhandleFilterapply here: consider adding error logging and more specific error messages for improved debugging and user experience.packages/blocks/notification-details/src/frontend/NotificationDetails.client.tsx (1)
7-9: Imports and error handling pattern are correct.The try/catch implementation with localized toast notifications is clean and aligns with the project-wide error handling approach. The
labels.errors.requestErrorstructure is guaranteed by the type system (CMS.Model.AppConfig.Labels), so no runtime risk of undefined values. The pattern is consistent across other blocks in the codebase.packages/blocks/order-list/src/frontend/OrderList.client.tsx (3)
9-12: Toast and GlobalProvider imports look correctThe new
toastanduseGlobalContextimports are consistent with the referenced modules and their later usage in this component.
37-37: UsinguseGlobalContextfor localized labels is appropriateAccessing
labelsfromuseGlobalContextkeeps error messaging aligned with global i18n and with other blocks in this PR; no issues here.
77-87:handleReseterror handling matcheshandleFilterReset logic now fails gracefully: on request failure, state is left unchanged and the user sees the same localized destructive toast as in
handleFilter. This keeps behavior consistent across both interactions.packages/blocks/notification-list/src/frontend/NotificationList.client.tsx (1)
11-14: Notification list error handling aligns well with the new pattern
handleFilterandhandleResetnow correctly guardsdk.blocks.getNotificationListwith try/catch and report failures via a localized destructive toast (labels.errors.requestError). This prevents the block from breaking while keeping previously loaded notifications visible.Implementation is consistent with the other blocks in this PR and doesn’t introduce new correctness issues.
Also applies to: 38-38, 57-72, 75-92
packages/blocks/orders-summary/src/frontend/OrdersSummary.client.tsx (1)
60-112: The 'week' range case is dead code that doesn't execute in the current implementationThe switch statement includes a
'week'case (line 90–92), but the CMS data never providestype: 'week'—onlytype: 'day'andtype: 'month'. So while the logic is indeed identical to the'day'case, this branch is unreachable based on current CMS configuration.If
'week'type is intended for future use, note that the current logic would be incorrect: subtractingrangedays whenrangerepresents weeks (e.g.,value: 1, type: 'week') would produce too narrow a window. Usesubtract(range, 'weeks')instead.Additionally, the
setRangecall on line 104 uses a destructive assertion (!) that will throw if the configuration is inconsistent. A guarded fallback (e.g.,const found = component.ranges?.find(...); if (found) setRange(found)) would make the code more resilient to data mismatches.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
… feature/blocks_improved-error-handling
…ross multiple blocks
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/major-chefs-cough.md (1)
14-14: Consider a more descriptive summary for release notes.The summary "improved error handling" is accurate but generic. Consider expanding it to highlight the user-facing benefit, e.g., "improved error handling with toast notifications on API failures" or "graceful error messages on data fetch failures".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.changeset/major-chefs-cough.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-14T14:21:20.382Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 357
File: .changeset/cold-showers-camp.md:1-9
Timestamp: 2025-11-14T14:21:20.382Z
Learning: In the o2sdev/openselfservice repository, new packages should always be added with a 'major' version bump in changesets to start at version 1.0.0, not 'minor'.
Applied to files:
.changeset/major-chefs-cough.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy-preview
🔇 Additional comments (1)
.changeset/major-chefs-cough.md (1)
1-12: Changeset structure and version strategy are appropriate.All packages correctly marked as 'minor' for backward-compatible error handling improvements. The list of 10 packages aligns with the blocks receiving enhanced error handling across the PR.
What does this PR do?
Key Changes
How to test
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.