feat: Add error boundaries to layout components and introduce RouteEr… #9384
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements route-level error boundaries to prevent errors in individual routes from crashing the entire application. Previously, a single route error would make the whole app unresponsive, requiring a full page reload. Now errors are isolated to specific routes, allowing users to navigate to other sections while the problematic route displays an error message with retry functionality.
Key Changes:
- Route-level error boundaries automatically reset when users navigate away from errored routes
- Clear error messages with retry functionality for better user experience
- Error boundaries wrap all route components across Console and MyAccount applications
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| features/admin.core.v1/components/route-error-boundary.tsx | New route-level error boundary component for Console app with auto-reset on navigation |
| features/admin.core.v1/components/protected-route.tsx | Wraps protected route components with RouteErrorBoundary |
| apps/myaccount/src/components/shared/route-error-boundary.tsx | New route-level error boundary component for MyAccount app |
| apps/myaccount/src/components/shared/protected-route.tsx | Wraps protected route components with RouteErrorBoundary |
| apps/myaccount/src/layouts/default.tsx | Adds ErrorBoundary wrapper to route components |
| apps/myaccount/src/layouts/app.tsx | Adds ErrorBoundary wrapper to route components |
| apps/console/src/layouts/full-screen-layout.tsx | Adds ErrorBoundary wrapper with auth callback handling |
| apps/console/src/layouts/default-layout.tsx | Adds ErrorBoundary wrapper with auth callback handling |
| apps/console/src/layouts/dashboard-layout.tsx | Adds ErrorBoundary wrapper with auth callback handling |
| apps/console/src/layouts/app-layout.tsx | Adds ErrorBoundary wrapper with auth callback handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
There are some ESLint and Typecheck failures. Please fix them. |
|
Thank you for the detailed review and suggestions! You're right, I'll address the ESLint and type-check failures. Regarding the component implementation, that's an excellent point. I'll refactor both RouteErrorBoundary components to be functional components that wrap the existing ErrorBoundary from @wso2is/react-components as you suggested. This will improve code reuse and consistency. |
|
Any update on this team? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9384 +/- ##
==========================================
+ Coverage 48.75% 55.88% +7.12%
==========================================
Files 42 42
Lines 1003 1020 +17
Branches 250 231 -19
==========================================
+ Hits 489 570 +81
+ Misses 514 450 -64
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Hi @ngsanthosh, There are a few more lint issues remaining. https://github.com/wso2/identity-apps/actions/runs/18883760205/job/54325447917?pr=9384 Can you take a look and update the PR? |
|
Hey @ngsanthosh, Will you be able to address the lint issues within today? |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds route-scoped ErrorBoundary usage across console, myaccount, and admin.core; introduces utilities Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Router
participant RouteBoundary as RouteErrorBoundary / ErrorBoundary
participant RouteComp as RouteComponent
participant EventLog as EventPublisher
participant Fallback as Broken/Generic Fallback UI
Note over Router,RouteBoundary: Per-route ErrorBoundary keyed by location.pathname
User->>Router: Navigate to route
Router->>RouteBoundary: Render wrapped route
RouteBoundary->>RouteComp: Attempt render
alt Component throws (runtime or chunk-load)
RouteComp-->>RouteBoundary: Error thrown
RouteBoundary->>EventLog: Publish route error (includes routeName)
RouteBoundary->>Fallback: Render localized fallback with retry
Note right of RouteBoundary: onChunkLoadError stores\n`appHomePath` → sessionStorage
Fallback-->>User: Offer retry / refresh
else Success
RouteComp-->>User: Normal UI
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/myaccount/src/layouts/app.tsx (1)
54-71: Prefer<Trans>here to preserve rich translationsSwitching the subtitle array to
<Trans>(like in the shared RouteErrorBoundary) keeps translators free to add markup or pluralization logic without later refactors.Apply this diff to stay consistent:
- const genericErrorSubtitles: string[] = [ - t("myAccount:placeholders.genericError.subtitles.0"), - t("myAccount:placeholders.genericError.subtitles.1") - ]; + const genericErrorSubtitles: ReactNode[] = [ + ( + <Trans + key="subtitle-0" + i18nKey="myAccount:placeholders.genericError.subtitles.0" + /> + ), + ( + <Trans + key="subtitle-1" + i18nKey="myAccount:placeholders.genericError.subtitles.1" + /> + ) + ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/console/src/layouts/app-layout.tsx(4 hunks)apps/console/src/layouts/dashboard-layout.tsx(1 hunks)apps/console/src/layouts/default-layout.tsx(1 hunks)apps/console/src/layouts/full-screen-layout.tsx(1 hunks)apps/myaccount/src/components/shared/protected-route.tsx(3 hunks)apps/myaccount/src/components/shared/route-error-boundary.tsx(1 hunks)apps/myaccount/src/layouts/app.tsx(4 hunks)apps/myaccount/src/layouts/default.tsx(5 hunks)features/admin.core.v1/components/protected-route.tsx(3 hunks)features/admin.core.v1/components/route-error-boundary.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
apps/myaccount/src/components/shared/route-error-boundary.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
apps/myaccount/src/layouts/default.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
features/admin.core.v1/components/route-error-boundary.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
apps/console/src/layouts/default-layout.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
apps/console/src/layouts/app-layout.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
apps/console/src/layouts/full-screen-layout.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
apps/console/src/layouts/dashboard-layout.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
apps/myaccount/src/layouts/app.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
🔇 Additional comments (7)
apps/myaccount/src/layouts/default.tsx (4)
22-49: LGTM! Imports are well-organized.All new imports are necessary for the error boundary implementation and properly organized within their respective groups.
61-61: LGTM! Standard translation hook usage.
102-109: ****The
ProtectedRoutecomponent already includes internal error boundary handling. The component wraps rendered content withRouteErrorBoundaryin both conditional branches (lines 84-88 and 91-95 ofprotected-route.tsx), so protected routes are properly isolated from unhandled errors. No additional error boundary wrapper is needed at the call site indefault.tsx.Likely an incorrect or invalid review comment.
115-135: No issues found - implementation is correct.The ErrorBoundary implementation in default.tsx is properly structured:
AppUtils.onChunkLoadError - Verified as properly implemented in
apps/myaccount/src/utils/app-utils.ts. It correctly dispatches theAppConstants.CHUNK_LOAD_ERROR_EVENTfor chunk loading failures.Translation keys - The
myAccount:placeholders.genericError.*pattern is consistent with other confirmed working translation keys throughout the codebase (e.g.,myAccount:placeholders.loginError.*,myAccount:placeholders.accessDeniedError.*). Translations are loaded from the@wso2is/i18npackage, which is properly initialized in the app.apps/myaccount/src/components/shared/route-error-boundary.tsx (1)
47-54: Telemetry publishing is on pointCapturing both the component stack and the friendly route name gives us actionable diagnostics without extra work later. Nicely done.
apps/myaccount/src/components/shared/protected-route.tsx (1)
81-93: Nice guard-railWrapping both guarded code paths in RouteErrorBoundary keeps the surface consistent while still short-circuiting to the access-denied route when needed.
features/admin.core.v1/components/route-error-boundary.tsx (1)
47-54: Good use of unified telemetry payloadMirroring the event payload shape between apps will make downstream aggregation much simpler. Looks great.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/console/src/layouts/default-layout.tsx (1)
127-181: Factor out the shared broken-page fallback.
handleRouteChunkError,brokenPageSubtitles, andbrokenPageFallbackare now duplicated across every layout updated in this PR (default, full-screen, dashboard, app). Centralizing them in a small helper/hook (e.g.,useRouteErrorFallback) would keep the markup, translations, and storage side-effect in one place and reduce the risk of these drifting out of sync during future tweaks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/console/src/layouts/app-layout.tsx(4 hunks)apps/console/src/layouts/dashboard-layout.tsx(3 hunks)apps/console/src/layouts/default-layout.tsx(3 hunks)apps/console/src/layouts/full-screen-layout.tsx(3 hunks)apps/myaccount/src/layouts/app.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/myaccount/src/layouts/app.tsx
- apps/console/src/layouts/dashboard-layout.tsx
- apps/console/src/layouts/app-layout.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
apps/console/src/layouts/default-layout.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
apps/console/src/layouts/full-screen-layout.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
🔇 Additional comments (1)
apps/console/src/layouts/full-screen-layout.tsx (1)
154-169: Thanks for keying the per-route boundary.Deriving the key from
renderProps.locationguarantees this boundary remounts on every navigation variant, so users no longer get stuck on the fallback for sibling URLs. Nicely addresses the earlier navigation trap.
|
@JayaShakthi97 I have cleared the lint issues. Kindly check and do the needful :) Thanks. |
|
@DonOmalVindula I have made necessary changes, Kindly check. @brionmario Could you please ask someone from team to check this PR and do the needful. Thanks in advance. |
|
@DonOmalVindula Any update? its been very long |
Upon testing your change locally, I noticed that the side panel collapse/expand functionality is not working as expected. Please see the attachment below. Screen.Recording.2025-11-22.at.15.47.22.movIt should behave as follows (Current behavior in the master branch) Uploading Screen Recording 2025-11-22 at 15.52.26.mov… Please fix this error and let us know. Also, better to include steps to reproduce an erroneous behavior so we can see your implemented error boundary in action. |
ngsanthosh
force-pushed
the
patch_handle_sww_err_message
branch
from
1066164 to
d238cb1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/console/src/layouts/app-layout.tsx (1)
55-76: Extract duplicated error handling logic to a shared utility.This is identical to the code in
full-screen-layout.tsxanddefault-layout.tsx. Please consolidate into a shared utility as suggested in the review comment forfull-screen-layout.tsx.apps/console/src/layouts/default-layout.tsx (1)
127-148: Extract duplicated error handling logic to a shared utility.This is identical to the code in
full-screen-layout.tsxandapp-layout.tsx. Please consolidate into a shared utility as suggested in the review comment forfull-screen-layout.tsx.
🧹 Nitpick comments (3)
apps/console/src/layouts/app-layout.tsx (1)
115-118: Consider using consistent locationKey construction.The locationKey is constructed using string concatenation with
+operators, whilefull-screen-layout.tsxanddefault-layout.tsxusearray.join("::"). Both approaches work, but consistency improves maintainability.Apply this diff for consistency:
- const locationKey: string = renderProps.location.key - ?? `${ renderProps.location.pathname }::` - + `${ renderProps.location.search }::` - + `${ renderProps.location.hash }`; + const locationKey: string = renderProps.location.key + ?? [ + renderProps.location.pathname, + renderProps.location.search, + renderProps.location.hash + ].join("::");apps/myaccount/src/components/shared/protected-route.tsx (1)
25-26: RouteErrorBoundary integration in ProtectedRoute looks correctWrapping the resolved
ComponentwithRouteErrorBoundaryin both “no scope” and “has required scope” branches preserves the existing auth/redirect behavior while adding per‑route error isolation and a route name for logging. No functional issues stand out here.If you want to trim duplication later, you could extract a small helper like
renderWithBoundary(props)used by both branches, but that’s purely cosmetic.Also applies to: 81-85, 89-93
apps/myaccount/src/components/shared/route-error-boundary.tsx (1)
27-101: Consistent RouteErrorBoundary for MyAccount; consider sharing common logicThis implementation mirrors the Console
RouteErrorBoundarywith MyAccount‑specific i18n keys and illustration, which is good for consistency and meets the route‑level isolation/logging goals.Given how similar the two components are, you might later consider factoring out a shared helper (e.g., a small factory or common hook for
handleError+renderFallback) to keep Console/MyAccount behavior in sync when you tweak copy or analytics. Also, the same note about confirming theErrorBoundaryprop names (fallback,handleError,onChunkLoadError) from my Console comment applies here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/console/src/layouts/app-layout.tsx(4 hunks)apps/console/src/layouts/dashboard-layout.tsx(3 hunks)apps/console/src/layouts/default-layout.tsx(3 hunks)apps/console/src/layouts/full-screen-layout.tsx(3 hunks)apps/myaccount/src/components/shared/protected-route.tsx(3 hunks)apps/myaccount/src/components/shared/route-error-boundary.tsx(1 hunks)apps/myaccount/src/layouts/app.tsx(4 hunks)features/admin.core.v1/components/protected-route.tsx(3 hunks)features/admin.core.v1/components/route-error-boundary.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- features/admin.core.v1/components/protected-route.tsx
- apps/console/src/layouts/dashboard-layout.tsx
- apps/myaccount/src/layouts/app.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
apps/myaccount/src/components/shared/route-error-boundary.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
apps/console/src/layouts/default-layout.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
apps/console/src/layouts/full-screen-layout.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
apps/console/src/layouts/app-layout.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
features/admin.core.v1/components/route-error-boundary.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
🔇 Additional comments (4)
apps/console/src/layouts/full-screen-layout.tsx (1)
154-170: LocationKey correctly ensures ErrorBoundary remounts on navigation.The
locationKeyderived fromrenderProps.locationensures the ErrorBoundary remounts when users navigate to different routes, preventing them from staying stuck on the fallback. This addresses the concern from previous reviews.Minor note: Consider adding error logging in
handleRouteChunkErrorto aid debugging:const handleRouteChunkError = (_error: Error, _errorInfo: React.ErrorInfo): void => { + console.error("Route chunk load error:", _error); sessionStorage.setItem("auth_callback_url_console", appHomePath); };apps/console/src/layouts/default-layout.tsx (1)
168-184: LocationKey implementation correctly resets ErrorBoundary on navigation.The stable
locationKeyderived fromrenderProps.locationensures the ErrorBoundary remounts when navigating between different routes, preventing users from remaining stuck on the error fallback. This properly addresses the concern raised in previous reviews.features/admin.core.v1/components/route-error-boundary.tsx (2)
27-54: RouteErrorBoundary implementation and logging look solidThe props shape, event publishing (including
componentStack,message,name, androute), and the localized UI all align well with the route-level isolation and observability goals. I don’t see functional issues in this part.
56-101: The review comment is based on incorrect assumptions about the ErrorBoundary API and should be dismissed.Verification confirms the code is correct:
Prop names are verified: Both
handleErrorandonChunkLoadErrormatch the ErrorBoundaryProps interface exactly. They are not silently ignored; they're properly called incomponentDidCatch().Fallback API is element-based, not callback-based: The
fallbackprop acceptsReact.ReactNode(a rendered element), not a callback function. The ErrorBoundary'srender()method returnsfallbackdirectly without any error/reset parameters. PassingrenderFallback()(immediately invoked to return JSX) is the correct usage pattern.No resetError callback exists: The suggestion to wire
handleRetryto aresetErrorparameter is invalid because ErrorBoundary doesn't support that pattern. The current implementation with full-page refresh viaCommonUtils.refreshPage()is the intended behavior.Likely an incorrect or invalid review comment.
…odule - Create error-boundary-utils.tsx with createRouteErrorHandler() and createBrokenPageFallback() functions - Extract sessionStorage error handler logic shared across layouts - Extract broken page fallback UI component logic shared across layouts - Update default-layout.tsx, app-layout.tsx, and full-screen-layout.tsx to use shared utilities - Remove duplicated error handling code and unused imports from all three layouts - Improves code reuse and maintainability across console layouts
ngsanthosh
force-pushed
the
patch_handle_sww_err_message
branch
from
d238cb1 to
d62d2ee
Compare
|
Hi @DonOmalVindula, Thank you for testing and catching the side panel issue! I've identified and fixed the problem. Root CauseThe ErrorBoundary components were using a location-based key that was causing unnecessary remounts, which interfered with the side panel's state management. Fix AppliedI've updated the ErrorBoundary key strategy to use
Steps to Reproduce Error Boundary BehaviorTo see the error boundaries in action:
The latest commit contains these fixes. Kindly review and do the needful |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
features/admin.core.v1/components/protected-route.tsx (1)
24-24: ProtectedRoute now correctly delegates to RouteErrorBoundaryWrapping the protected component with
RouteErrorBoundarypreserves the existing auth/redirect flow and cleanly scopes route-level failures. PassingrouteName={ renderProps.match?.path }should be fine for logging; if you ever need URL-level granularity in logs or metrics, consider also (or instead) usingrenderProps.location.pathname.Please verify at least one failing route still allows navigating to other protected routes when an error is thrown inside
Component, and that route-level errors are logged with the expected route identifier.Also applies to: 60-67
apps/console/src/layouts/dashboard-layout.tsx (1)
197-219: Consolidate dashboard error handler/fallback with shared utilitiesThe new
handleRouteChunkErrorandbrokenPageFallbacklook correct and align with other layouts: they storeappHomePathinsessionStorageon errors and show a broken-page placeholder with a refresh action. Reusing the shared helpers from@wso2is/admin.core.v1/utils/error-boundary-utils(as done infull-screen-layout.tsxanddefault-layout.tsx) would remove duplication and centralize future changes:
const handleRouteChunkError = createRouteErrorHandler(config.deployment.appHomePath);const brokenPageFallback = createBrokenPageFallback(t);Also, if there is any chance
config.deploymentmight be transiently undefined when an error occurs, consider guarding the property access insidehandleRouteChunkErrorto avoid the handler itself throwing.Please double‑check that:
config.deployment.appHomePathis always defined by the timeDashboardLayoutcan render and handle errors, andhandleRouteChunkErrorandbrokenPageFallbackremain consistent with the implementations inerror-boundary-utils(or switch this file to those helpers).Also applies to: 471-473
apps/myaccount/src/layouts/app.tsx (1)
28-28: Generic MyAccount fallback and per-route ErrorBoundaries are wired correctly; consider centralizing if reused elsewhereThe new
genericErrorFallbackuses localized generic-error copy and a refresh action, and it’s now applied both:
- as the global
fallbackon the outerErrorBoundary, and- via per-route
ErrorBoundarywrappers keyed byrenderProps.location.pathnamefor non-protected routes,which should give users a clear “something went wrong” UI and automatic reset on navigation.
If other MyAccount surfaces (e.g., a shared
RouteErrorBoundarycomponent) also need the same generic error placeholder, you could extractgenericErrorFallbackcreation into a small helper to keep the behavior and wording centralized.Please:
- Confirm the
myAccount:placeholders.genericError.*translation keys andgenericErrorillustration are present in all supported locales, and- Manually test a failing non-protected route to ensure only that route shows the generic error UI while the rest of the app remains usable and recovers on navigation.
Also applies to: 54-71, 82-85, 104-118
features/admin.core.v1/utils/error-boundary-utils.tsx (1)
24-36: Minor polish forcreateRouteErrorHandlerdocs and robustness.Implementation is fine, but a couple of small cleanups could help:
- The JSDoc refers to a “route chunk error handler”, but the function is a generic error handler (used for any boundary error). Consider updating the wording so future readers don’t assume it’s chunk‑error–specific.
- If these utilities might ever run in a non‑browser context (SSR/tests), you could optionally guard the
sessionStoragecall, e.g. checktypeof window !== "undefined"before writing, and/or centralize"auth_callback_url_console"as a shared constant if one already exists elsewhere.features/admin.core.v1/components/route-error-boundary.tsx (1)
47-91: Route error logging and fallback look good; consider reusing shared fallback util.The logging payload (name, message, componentStack, route) is compact and non‑PII, and the broken‑page
EmptyPlaceholder+ refresh action aligns with the new UX.There is, however, quite a bit of overlap with
createBrokenPageFallbackinutils/error-boundary-utils.tsx(same illustration and i18n keys). To keep the broken‑page UI consistent and reduce maintenance overhead, consider:
- Either delegating to
createBrokenPageFallback(t)here, or- Documenting that this boundary intentionally uses a slightly richer
Trans‑based variant so that future edits keep both in sync.This is purely a maintainability/consistency tweak; behavior is already correct.
apps/console/src/layouts/app-layout.tsx (1)
86-101: Use a more specific key for per-routeErrorBoundaryto reset on query-string changes.Keying the inner
ErrorBoundarybyrenderProps.location.pathnamegives you reset on path changes, but not when only the query string changes (same path, different?foo=bar), which can still be a “route change” from a user’s perspective.To make error recovery more robust across all navigation, consider including
location.keyor the search part as in the earlier pattern:- <ErrorBoundary - key={ renderProps.location.pathname } + <ErrorBoundary + key={ + renderProps.location.key + ?? `${ renderProps.location.pathname }${ renderProps.location.search ?? "" }` + } onChunkLoadError={ AppUtils.onChunkLoadError } handleError={ handleRouteChunkError } fallback={ brokenPageFallback } >This keeps the current behavior but also guarantees the boundary remounts when only search parameters change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/console/src/layouts/app-layout.tsx(4 hunks)apps/console/src/layouts/dashboard-layout.tsx(3 hunks)apps/console/src/layouts/default-layout.tsx(4 hunks)apps/console/src/layouts/full-screen-layout.tsx(4 hunks)apps/myaccount/src/components/shared/protected-route.tsx(3 hunks)apps/myaccount/src/components/shared/route-error-boundary.tsx(1 hunks)apps/myaccount/src/layouts/app.tsx(4 hunks)apps/myaccount/src/layouts/default.tsx(5 hunks)features/admin.core.v1/components/protected-route.tsx(3 hunks)features/admin.core.v1/components/route-error-boundary.tsx(1 hunks)features/admin.core.v1/utils/error-boundary-utils.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/myaccount/src/components/shared/route-error-boundary.tsx
- apps/myaccount/src/layouts/default.tsx
🧰 Additional context used
🧬 Code graph analysis (7)
apps/console/src/layouts/app-layout.tsx (1)
features/admin.core.v1/utils/error-boundary-utils.tsx (2)
createRouteErrorHandler(30-36)createBrokenPageFallback(44-65)
apps/console/src/layouts/full-screen-layout.tsx (1)
features/admin.core.v1/utils/error-boundary-utils.tsx (2)
createRouteErrorHandler(30-36)createBrokenPageFallback(44-65)
features/admin.core.v1/components/route-error-boundary.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
apps/myaccount/src/layouts/app.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
features/admin.core.v1/utils/error-boundary-utils.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
apps/console/src/layouts/dashboard-layout.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
apps/console/src/layouts/default-layout.tsx (1)
features/admin.core.v1/utils/error-boundary-utils.tsx (2)
createRouteErrorHandler(30-36)createBrokenPageFallback(44-65)
🔇 Additional comments (6)
apps/myaccount/src/components/shared/protected-route.tsx (3)
2-2: LGTM!Copyright year update is appropriate.
25-25: LGTM!The import is correctly structured and aligns with the PR's objective to introduce route-level error boundaries.
81-93: Add key prop to RouteErrorBoundary for proper error reset on route changes. Verify side panel regression impact.React Error Boundaries should reset when navigating between routes, and using a key prop (e.g., key={location.pathname}) ensures the boundary instance remounts and errors don't persist. The current code reuses the same boundary instance, preventing automatic reset.
Issue 1: Missing key prop
Both RouteErrorBoundary instances (lines 81-86 and 89-93) lack a key prop. Add
key={props.match?.path}to reset error state on route navigation:if (!route?.scope) { return ( - <RouteErrorBoundary routeName={ props.match?.path }> + <RouteErrorBoundary key={ props.match?.path } routeName={ props.match?.path }> <Component { ...props } /> </RouteErrorBoundary> ); } if (scopes?.includes(route?.scope)) { return ( - <RouteErrorBoundary routeName={ props.match?.path }> + <RouteErrorBoundary key={ props.match?.path } routeName={ props.match?.path }> <Component { ...props } /> </RouteErrorBoundary> );Issue 2: Verify reported side panel regression
The reviewer reported that side panel collapse/expand functionality broke after these changes. Please confirm this regression and verify the error boundary wrapping does not interfere with event handling or component lifecycle in the side panel.
Optional: Reduce code duplication
Lines 81-86 and 89-93 are identical. Extract the wrapping logic to a helper function for maintainability.
apps/console/src/layouts/dashboard-layout.tsx (1)
242-257: Per-route ErrorBoundary wiring here matches the desired isolation semanticsWrapping non-protected routes in an
ErrorBoundarykeyed byrenderProps.location.pathnamewithonChunkLoadError,handleRouteChunkError, andbrokenPageFallbackgives each URL its own error state and ensures navigating to a different path remounts the boundary and allows recovery.Please verify with a test route that throws during render that:
- Only that route’s content area shows the broken-page fallback while the header and side panel remain usable, and
- Navigating to a different route clears the error and renders normally.
apps/console/src/layouts/full-screen-layout.tsx (1)
24-32: Full-screen layout’s shared error handling and per-route boundaries look solidThis layout cleanly reuses
createRouteErrorHandler(appHomePath)andcreateBrokenPageFallback(t)and wires them into both:
- the per-route
ErrorBoundarykeyed byrenderProps.location.pathname, and- the top-level
ErrorBoundaryaround the<Switch>,which should give consistent broken-page UX and ensure route-level errors reset on navigation.
Please confirm via manual testing that:
- A failure in a full-screen route shows the expected broken-page placeholder with the refresh action, and
- Navigating to another full-screen route (different pathname) clears the error and renders successfully.
Also applies to: 109-112, 128-143, 181-182
apps/console/src/layouts/default-layout.tsx (1)
34-40: Default layout’s integration with shared error-boundary utilities is correct and consistentUsing
createRouteErrorHandler(appHomePath)andcreateBrokenPageFallback(t)here, then applying them to both:
- the per-route
ErrorBoundarykeyed byrenderProps.location.pathname, and- the outer
ErrorBoundarywrapping the layout content,keeps error handling DRY and aligned with the other console layouts while providing clear, retryable fallbacks.
Please verify that:
- Non-protected default-layout routes now show the broken-page fallback (with refresh) when they error, without affecting other sections, and
- The back-to-home behavior (
appHomePath) used in both navigation and error handler is correct for this layout.Also applies to: 128-131, 145-160, 223-224
|
Any update? @DonOmalVindula |
|
https://drive.google.com/file/d/1ap2l_Dp84bHTK7_Kr0gkv1ZWPWG6puzO/view?usp=sharing Above is a screen recording of the changes done. Kindly check and verify @DonOmalVindula |
|
wso2 team any update on this? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const createRouteErrorHandler = ( | ||
| appHomePath: string | ||
| ): ((_error: Error, _errorInfo: React.ErrorInfo) => void) => { | ||
| return (_error: Error, _errorInfo: React.ErrorInfo): void => { | ||
| sessionStorage.setItem("auth_callback_url_console", appHomePath); | ||
| }; | ||
| }; |
There was a problem hiding this comment.
The function name createRouteErrorHandler suggests it handles route errors, but it specifically stores the app home path in sessionStorage. A more descriptive name like createRouteChunkErrorHandler or createSessionStorageErrorHandler would better convey its specific purpose.
| @@ -1,5 +1,5 @@ | |||
| /** | |||
| * Copyright (c) 2020, WSO2 LLC. (https://www.wso2.com). All Rights Reserved. | |||
| * Copyright (c) 2025, WSO2 LLC. (https://www.wso2.com). | |||
There was a problem hiding this comment.
The copyright year was changed from 2020 to 2025, removing the original year. Consider using a range format like "2020-2025" to preserve the original copyright year while indicating updates, which is a common practice for maintaining copyright history.
| * Copyright (c) 2025, WSO2 LLC. (https://www.wso2.com). | |
| * Copyright (c) 2020-2025, WSO2 LLC. (https://www.wso2.com). |
| @@ -1,5 +1,5 @@ | |||
| /** | |||
| * Copyright (c) 2019, WSO2 LLC. (https://www.wso2.com). All Rights Reserved. | |||
| * Copyright (c) 2025, WSO2 LLC. (https://www.wso2.com). | |||
There was a problem hiding this comment.
The copyright year was changed from 2019 to 2025, removing the original year. Consider using a range format like "2019-2025" to preserve the original copyright year while indicating updates, which is a common practice for maintaining copyright history.
| * Copyright (c) 2025, WSO2 LLC. (https://www.wso2.com). | |
| * Copyright (c) 2019-2025, WSO2 LLC. (https://www.wso2.com). |
| <ErrorBoundary | ||
| onChunkLoadError={ AppUtils.onChunkLoadError } | ||
| fallback={ ( | ||
| <EmptyPlaceholder | ||
| action={ ( | ||
| <LinkButton onClick={ () => CommonUtils.refreshPage() }> | ||
| { t("myAccount:placeholders.genericError.action") } | ||
| </LinkButton> | ||
| ) } | ||
| image={ getEmptyPlaceholderIllustrations().genericError } | ||
| imageSize="tiny" | ||
| subtitle={ [ | ||
| t("myAccount:placeholders.genericError.subtitles.0"), | ||
| t("myAccount:placeholders.genericError.subtitles.1") | ||
| ] } | ||
| title={ t("myAccount:placeholders.genericError.title") } | ||
| /> | ||
| ) } | ||
| > |
There was a problem hiding this comment.
The ErrorBoundary component is missing the key prop that's used in other layouts (like app.tsx line 111) to reset the error boundary state when navigating between routes. Without this key prop based on the location pathname, the error boundary won't reset when users navigate away from an errored route, which defeats one of the main purposes of this PR.
| const brokenPageSubtitles: string[] = [ | ||
| t("console:common.placeholders.brokenPage.subtitles.0"), | ||
| t("console:common.placeholders.brokenPage.subtitles.1") | ||
| ]; | ||
|
|
||
| const brokenPageFallback: ReactNode = ( | ||
| <EmptyPlaceholder | ||
| action={ ( | ||
| <LinkButton onClick={ () => CommonUtils.refreshPage() }> | ||
| { t("console:common.placeholders.brokenPage.action") } | ||
| </LinkButton> | ||
| ) } | ||
| image={ getEmptyPlaceholderIllustrations().brokenPage } | ||
| imageSize="tiny" | ||
| subtitle={ brokenPageSubtitles } | ||
| title={ t("console:common.placeholders.brokenPage.title") } | ||
| /> | ||
| ); |
There was a problem hiding this comment.
The brokenPageFallback is manually constructed here, while other console layouts use the createBrokenPageFallback helper function from error-boundary-utils. This code duplication makes maintenance more difficult. Consider using createBrokenPageFallback for consistency across all layouts.
There was a problem hiding this comment.
+1 for this suggestion. Let's use a common function here
| ? Component | ||
| ? <Component { ...renderProps } /> | ||
| ? ( | ||
| <RouteErrorBoundary routeName={ renderProps.match?.path }> |
There was a problem hiding this comment.
The RouteErrorBoundary is missing a key prop to reset the error boundary state when navigating between routes. Without a key based on the route path (e.g., key={renderProps.match?.path} or key={renderProps.location.pathname}), the error boundary will maintain its error state even when users navigate to different routes. This contradicts the PR's goal of automatically resetting error state on route changes.
| <RouteErrorBoundary routeName={ renderProps.match?.path }> | |
| <RouteErrorBoundary | |
| key={ renderProps.location.pathname } | |
| routeName={ renderProps.match?.path }> |
| <RouteErrorBoundary routeName={ props.match?.path }> | ||
| <Component { ...props } /> | ||
| </RouteErrorBoundary> |
There was a problem hiding this comment.
The RouteErrorBoundary is missing a key prop to reset the error boundary state when navigating between routes. Without a key based on the route path (e.g., key={props.match?.path} or key={props.location.pathname}), the error boundary will maintain its error state even when users navigate to different routes. This contradicts the PR's goal of automatically resetting error state on route changes.
There was a problem hiding this comment.
+1 for this suggestion. Let's implement this
| /** | ||
| * Route-level Error Boundary that isolates errors to specific routes. | ||
| * | ||
| * @param props - Props for the component. | ||
| * @returns Route error boundary component. | ||
| */ | ||
| const ROUTE_ERROR_EVENT_ID: string = "route-error-boundary"; | ||
|
|
There was a problem hiding this comment.
The constant ROUTE_ERROR_EVENT_ID should be declared before the component's JSDoc comment, not between the JSDoc and the component declaration. This improves code readability and follows conventional constant declaration patterns.
| /** | |
| * Route-level Error Boundary that isolates errors to specific routes. | |
| * | |
| * @param props - Props for the component. | |
| * @returns Route error boundary component. | |
| */ | |
| const ROUTE_ERROR_EVENT_ID: string = "route-error-boundary"; | |
| const ROUTE_ERROR_EVENT_ID: string = "route-error-boundary"; | |
| /** | |
| * Route-level Error Boundary that isolates errors to specific routes. | |
| * | |
| * @param props - Props for the component. | |
| * @returns Route error boundary component. | |
| */ |
| }); | ||
| const appHomePath: string = useSelector((state: AppState) => state.config.deployment.appHomePath); | ||
|
|
||
| const handleRouteChunkError = createRouteErrorHandler(appHomePath); |
There was a problem hiding this comment.
Let's add types for const values as suggested in the linter
Hi @ngsanthosh, Sorry about the delay. The previous bug is now resolved. But now I'm seeing a different bug where the state of the sidepanel resets when i click on a navigation item. The buggy behaviour is shown below Screen.Recording.2025-12-14.at.14.49.31.movWhere as in the correct behaviour, the sidepanel should persist it's state when clicking on navigation items as shown below. Please fix it. Screen.Recording.2025-12-14.at.14.49.10.movAdditionally, you have to fix the following.
|
- Add type annotations for handleRouteChunkError const values - Use shared utilities createRouteErrorHandler and createBrokenPageFallback in dashboard-layout - Fix copyright years to use range format (2019-2025, 2020-2025) - Move ROUTE_ERROR_EVENT_ID constant before JSDoc comment - Remove key prop from ErrorBoundary to fix sidepanel state reset bug - Fix import order in error-boundary-utils.tsx - Remove unused imports in default-layout.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
apps/myaccount/src/components/shared/protected-route.tsx (1)
81-93: Missingkeyprop on bothRouteErrorBoundaryusages.Both instances of
RouteErrorBoundary(lines 82 and 90) lack akeyprop. This was flagged in previous reviews and acknowledged for implementation. Without it, error state persists across route changes.if (!route?.scope) { return ( - <RouteErrorBoundary routeName={ props.match?.path }> + <RouteErrorBoundary + key={ props.location.pathname } + routeName={ props.match?.path } + > <Component { ...props } /> </RouteErrorBoundary> ); } if (scopes?.includes(route?.scope)) { return ( - <RouteErrorBoundary routeName={ props.match?.path }> + <RouteErrorBoundary + key={ props.location.pathname } + routeName={ props.match?.path } + > <Component { ...props } /> </RouteErrorBoundary> );apps/console/src/layouts/full-screen-layout.tsx (1)
128-142: Per-routeErrorBoundarymissingkeyprop for navigation reset.The
ErrorBoundarywrapping route components (line 134) lacks akeyprop. For routes with dynamic segments (e.g.,/view/:id), the boundary won't reset when navigating between different IDs, leaving users stuck on the error fallback.Previous reviews indicated this was addressed, but the key prop appears to be missing from the current code.
return ( <ErrorBoundary + key={ renderProps.location.pathname } onChunkLoadError={ AppUtils.onChunkLoadError } handleError={ handleRouteChunkError } fallback={ brokenPageFallback } > <route.component { ...renderProps } /> </ErrorBoundary> );apps/console/src/layouts/default-layout.tsx (1)
145-159: Per-routeErrorBoundarymissingkeyprop.Same issue as other layouts: the per-route
ErrorBoundary(line 151) needs akeyprop to reset error state when navigating between different URLs matching the same route pattern.return ( <ErrorBoundary + key={ renderProps.location.pathname } onChunkLoadError={ AppUtils.onChunkLoadError } handleError={ handleRouteChunkError } fallback={ brokenPageFallback } > <route.component { ...renderProps } /> </ErrorBoundary> );apps/console/src/layouts/app-layout.tsx (1)
88-102: Per-routeErrorBoundarymissingkeyprop.The
ErrorBoundarywrapping route components (line 94) needs akeyprop to ensure proper reset on navigation.return ( <ErrorBoundary + key={ renderProps.location.pathname } onChunkLoadError={ AppUtils.onChunkLoadError } handleError={ handleRouteChunkError } fallback={ brokenPageFallback } > <route.component { ...renderProps } /> </ErrorBoundary> );
🧹 Nitpick comments (2)
features/admin.core.v1/components/route-error-boundary.tsx (1)
40-104: LGTM! Clean implementation of route-level error boundary.The component correctly:
- Publishes error events with route context for debugging
- Provides localized fallback UI with retry action
- Handles chunk load errors via
AppUtils.onChunkLoadErrorNote: This component is nearly identical to
apps/myaccount/src/components/shared/route-error-boundary.tsx. Consider extracting a shared base component or factory function in a future refactor to reduce duplication across apps.apps/myaccount/src/layouts/app.tsx (1)
54-71: Consider using shared error boundary utilities for consistency.The console app layouts use
createBrokenPageFallbackfromerror-boundary-utils.tsx, while myaccount defines the fallback inline. Consider creating a similar utility for myaccount or extracting a shared module to maintain consistency and reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/console/src/layouts/app-layout.tsx(4 hunks)apps/console/src/layouts/dashboard-layout.tsx(5 hunks)apps/console/src/layouts/default-layout.tsx(4 hunks)apps/console/src/layouts/full-screen-layout.tsx(4 hunks)apps/myaccount/src/components/shared/protected-route.tsx(3 hunks)apps/myaccount/src/components/shared/route-error-boundary.tsx(1 hunks)apps/myaccount/src/layouts/app.tsx(4 hunks)features/admin.core.v1/components/protected-route.tsx(3 hunks)features/admin.core.v1/components/route-error-boundary.tsx(1 hunks)features/admin.core.v1/utils/error-boundary-utils.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/console/src/layouts/dashboard-layout.tsx
- features/admin.core.v1/utils/error-boundary-utils.tsx
🧰 Additional context used
🧬 Code graph analysis (6)
features/admin.core.v1/components/route-error-boundary.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
apps/myaccount/src/components/shared/route-error-boundary.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
apps/console/src/layouts/app-layout.tsx (1)
features/admin.core.v1/utils/error-boundary-utils.tsx (2)
createRouteErrorHandler(30-36)createBrokenPageFallback(44-65)
apps/myaccount/src/layouts/app.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
apps/console/src/layouts/default-layout.tsx (1)
features/admin.core.v1/utils/error-boundary-utils.tsx (2)
createRouteErrorHandler(30-36)createBrokenPageFallback(44-65)
apps/console/src/layouts/full-screen-layout.tsx (1)
features/admin.core.v1/utils/error-boundary-utils.tsx (2)
createRouteErrorHandler(30-36)createBrokenPageFallback(44-65)
🔇 Additional comments (4)
apps/console/src/layouts/full-screen-layout.tsx (1)
109-111: Good adoption of shared error boundary utilities.Clean use of
createRouteErrorHandlerandcreateBrokenPageFallbackutilities, eliminating duplicated error handling logic previously inlined in this layout.apps/console/src/layouts/default-layout.tsx (1)
128-130: Consistent use of shared error handling utilities.The layout correctly uses the centralized
createRouteErrorHandlerandcreateBrokenPageFallbackutilities.apps/myaccount/src/components/shared/route-error-boundary.tsx (1)
40-104: LGTM! Well-structured route error boundary for MyAccount.The implementation correctly:
- Publishes structured error events with route context
- Provides localized fallback UI using MyAccount i18n namespace
- Includes retry functionality and chunk load error handling
As noted for the admin.core version, this component shares significant logic with
features/admin.core.v1/components/route-error-boundary.tsx. A future refactor could extract shared logic while keeping app-specific i18n keys.apps/console/src/layouts/app-layout.tsx (1)
52-54: Good use of centralized error handling utilities.Clean integration of
createRouteErrorHandlerandcreateBrokenPageFallbackutilities.
Added the key prop using the current pathname to ErrorBoundary and RouteErrorBoundary components across multiple layout and route files. This ensures proper remounting of error boundaries when the route changes, improving error handling and component lifecycle management.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/myaccount/src/layouts/default.tsx (1)
115-136: MissinghandleErrorprop for error logging consistency.The ErrorBoundary is missing the
handleErrorprop that other layouts include for error tracking. This means errors in this layout won't trigger the session storage update for auth callback URL, making post-error recovery behavior inconsistent with Console layouts.Additionally, consider using a shared utility pattern similar to Console's
createBrokenPageFallbackandcreateRouteErrorHandlerto reduce duplication and ensure consistent error handling across MyAccount layouts.🔎 Suggested fix
+import { AppUtils } from "../utils"; + +// Inside the component, before the return: +const handleRouteError = (_error: Error, _errorInfo: React.ErrorInfo): void => { + // Add error logging or session storage handling as needed + console.error("Route error:", _error); +}; <ErrorBoundary key={ renderProps.location.pathname } onChunkLoadError={ AppUtils.onChunkLoadError } + handleError={ handleRouteError } fallback={ ( <EmptyPlaceholder
🧹 Nitpick comments (4)
apps/myaccount/src/layouts/default.tsx (1)
102-109: Protected routes lack per-route error boundary coverage.The
ProtectedRoutecomponent renders directly without an ErrorBoundary wrapper, unlike non-protected routes. If a protected route's component throws an error, it won't be isolated at the route level and could affect the entire layout.Consider wrapping ProtectedRoute rendering with similar error boundary handling for consistent error isolation.
apps/myaccount/src/layouts/app.tsx (1)
82-85: Global ErrorBoundary missinghandleErrorprop.The global ErrorBoundary lacks a
handleErrorcallback for error logging/tracking. While the fallback UI works, adding error handling would improve debugging and consistency with Console layouts.🔎 Suggested addition
+const handleRouteError = (_error: Error, _errorInfo: React.ErrorInfo): void => { + // Log error or update session storage for auth recovery + console.error("Route error:", _error); +}; <ErrorBoundary onChunkLoadError={ AppUtils.onChunkLoadError } + handleError={ handleRouteError } fallback={ genericErrorFallback } >apps/console/src/layouts/default-layout.tsx (1)
27-27: Minor: Extraneous empty line in imports.There's an empty line between the ProtectedRoute import and AppConstants import that appears to be a formatting artifact.
apps/myaccount/src/components/shared/protected-route.tsx (1)
81-99: Consider extracting the RouteErrorBoundary wrapper to reduce duplication.Both conditional branches (lines 81-88 and 92-99) contain identical
RouteErrorBoundarywrappers. This duplication can be eliminated by extracting the wrapper logic.🔎 Suggested refactor to eliminate duplication
const resolveComponents = (props: RouteComponentProps<any>): ReactElement => { const scopes: string[] = allowedScopes?.split(" "); + + const wrapWithErrorBoundary = (component: ReactElement): ReactElement => ( + <RouteErrorBoundary + key={ props.location.pathname } + routeName={ props.match?.path } + > + { component } + </RouteErrorBoundary> + ); if (!route?.scope) { - return ( - <RouteErrorBoundary - key={ props.location.pathname } - routeName={ props.match?.path } - > - <Component { ...props } /> - </RouteErrorBoundary> - ); + return wrapWithErrorBoundary(<Component { ...props } />); } if (scopes?.includes(route?.scope)) { - return ( - <RouteErrorBoundary - key={ props.location.pathname } - routeName={ props.match?.path } - > - <Component { ...props } /> - </RouteErrorBoundary> - ); + return wrapWithErrorBoundary(<Component { ...props } />); } else { return <Redirect to={ AppConstants.getPaths().get("ACCESS_DENIED_ERROR") } />; } };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/console/src/layouts/app-layout.tsx(4 hunks)apps/console/src/layouts/dashboard-layout.tsx(5 hunks)apps/console/src/layouts/default-layout.tsx(4 hunks)apps/console/src/layouts/full-screen-layout.tsx(4 hunks)apps/myaccount/src/components/shared/protected-route.tsx(3 hunks)apps/myaccount/src/layouts/app.tsx(4 hunks)apps/myaccount/src/layouts/default.tsx(5 hunks)features/admin.core.v1/components/protected-route.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
apps/myaccount/src/layouts/default.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
apps/console/src/layouts/dashboard-layout.tsx (1)
features/admin.core.v1/utils/error-boundary-utils.tsx (2)
createRouteErrorHandler(30-36)createBrokenPageFallback(44-65)
apps/myaccount/src/layouts/app.tsx (1)
features/admin.core.v1/configs/ui.ts (1)
getEmptyPlaceholderIllustrations(417-432)
apps/console/src/layouts/app-layout.tsx (1)
features/admin.core.v1/utils/error-boundary-utils.tsx (2)
createRouteErrorHandler(30-36)createBrokenPageFallback(44-65)
apps/console/src/layouts/default-layout.tsx (1)
features/admin.core.v1/utils/error-boundary-utils.tsx (2)
createRouteErrorHandler(30-36)createBrokenPageFallback(44-65)
apps/console/src/layouts/full-screen-layout.tsx (1)
features/admin.core.v1/utils/error-boundary-utils.tsx (2)
createRouteErrorHandler(30-36)createBrokenPageFallback(44-65)
🔇 Additional comments (15)
features/admin.core.v1/components/protected-route.tsx (2)
2-2: LGTM! Copyright header updated correctly.The copyright year range format preserves the original year while reflecting updates, which is the standard practice.
63-70: Error boundary implementation looks solid; verify side-panel state behavior.The RouteErrorBoundary wrapper with
key={renderProps.location.pathname}correctly implements automatic error-state reset on route changes, which aligns with the PR objectives. The approach properly isolates errors to individual routes.However, based on the PR comment history, reviewers have reported side-panel state resets when navigating between routes. Please verify that the side-panel state persists correctly across navigation and that the error boundary remounting doesn't inadvertently affect unrelated UI state.
To verify the side-panel behavior, manually test the following scenarios:
- Open the application and expand/collapse the side panel
- Navigate between different routes using the side panel menu items
- Confirm the side panel expand/collapse state persists across navigation
- Trigger an error in one route (e.g., simulate a chunk load error)
- Verify the error boundary catches it and the side panel remains functional
- Navigate to a different route and confirm the error state resets
If the side-panel state resets unexpectedly, consider whether:
- The side panel component's state is managed outside the error boundary scope
- A different key strategy (e.g., keying only on route pattern rather than full pathname) might be needed to balance error reset with state persistence
apps/console/src/layouts/dashboard-layout.tsx (3)
195-198: Good use of shared error handling utilities.The implementation correctly uses
createRouteErrorHandlerandcreateBrokenPageFallbackfrom the shared utilities, addressing previous review feedback about code duplication across layouts.
222-240: Per-route ErrorBoundary implementation looks correct.The ErrorBoundary is properly keyed by
renderProps.location.pathnamefor reset on navigation, includeshandleErrorfor error tracking, and uses the shared fallback. The guard for null component is also appropriate.
449-461: Global ErrorBoundary correctly configured.The top-level ErrorBoundary uses the same shared utilities for consistent error handling at both route and layout levels.
apps/console/src/layouts/full-screen-layout.tsx (2)
109-111: Shared error handling utilities correctly applied.The implementation properly uses the extracted utilities for consistent error handling across layouts.
128-142: Per-route ErrorBoundary correctly implemented.The boundary is properly keyed by pathname for navigation reset, includes all required props (
onChunkLoadError,handleError,fallback), and guards against null components.apps/myaccount/src/layouts/app.tsx (2)
54-71: Inline error fallback definition is acceptable but could be shared.The genericErrorFallback is well-structured with proper i18n support. For future maintainability, consider extracting this pattern to a shared utility similar to Console's
createBrokenPageFallbackfor MyAccount-specific error handling.
109-118: Per-route ErrorBoundary properly keyed but missinghandleError.The key prop using
renderProps.location.pathnameensures error state resets on navigation. However, addinghandleErrorwould provide consistent error tracking.apps/console/src/layouts/app-layout.tsx (2)
52-54: Correct use of shared error handling utilities.The implementation properly leverages
createRouteErrorHandlerandcreateBrokenPageFallbackfor consistent error handling.
94-102: Per-route ErrorBoundary correctly configured.All required props are present:
keyfor navigation reset,onChunkLoadError,handleError, andfallback. The implementation aligns with other Console layouts.apps/console/src/layouts/default-layout.tsx (2)
128-130: Shared error handling utilities correctly applied.The implementation uses
createRouteErrorHandlerandcreateBrokenPageFallbackconsistently with other Console layouts.
150-160: Per-route ErrorBoundary implementation is correct.The boundary is properly keyed by pathname, includes all required props, and guards against null components. This addresses the previous review feedback about ensuring error state reset on navigation.
apps/myaccount/src/components/shared/protected-route.tsx (2)
2-2: Copyright year format correctly updated.The copyright now uses the "2019-2025" range format, properly preserving the original year while indicating updates. This addresses the previous review feedback.
25-25: LGTM: RouteErrorBoundary import is correct.The import statement properly references the new route-level error boundary component.
| return ( | ||
| <RouteErrorBoundary | ||
| key={ props.location.pathname } | ||
| routeName={ props.match?.path } | ||
| > | ||
| <Component { ...props } /> | ||
| </RouteErrorBoundary> | ||
| ); | ||
| } | ||
|
|
||
| if (scopes?.includes(route?.scope)) { | ||
| return <Component { ...props } />; | ||
| return ( | ||
| <RouteErrorBoundary | ||
| key={ props.location.pathname } | ||
| routeName={ props.match?.path } | ||
| > | ||
| <Component { ...props } /> | ||
| </RouteErrorBoundary> | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for protected-route.tsx and understand its structure
cat -n apps/myaccount/src/components/shared/protected-route.tsx | head -110Repository: wso2/identity-apps
Length of output: 4896
🏁 Script executed:
#!/bin/bash
# Check the protected-route.tsx file structure
git ls-files apps/myaccount/src/components/shared/protected-route.tsx
# Read the file to understand full context
wc -l apps/myaccount/src/components/shared/protected-route.tsxRepository: wso2/identity-apps
Length of output: 179
Error boundary pathname key causes unnecessary remounting on every route change.
Using key={props.location.pathname} on RouteErrorBoundary will remount the component tree on every route change, causing the component to be recreated from scratch, including any state in the tree below it. This is a documented anti-pattern—error boundaries shouldn't remount on every URL change to avoid unwanted side-effects and performance issues.
Since the codebase uses Redux (line 23), global state is already protected. However, if any local component state (panels, modals, form inputs) exists within the rendered Component, it will reset on route navigation. Consider these alternatives:
- Remove the
keyprop entirely—error boundaries don't need pathname-based resets - If error reset is needed, use conditional logic in the error boundary (e.g., reset only after
componentDidCatch) - Manage UI state (panel visibility, etc.) via Redux or Context outside the error boundary
🤖 Prompt for AI Agents
In apps/myaccount/src/components/shared/protected-route.tsx around lines 81 to
99, the RouteErrorBoundary is given key={props.location.pathname} which forces a
full remount on every route change and clears local component state; remove the
key prop from both RouteErrorBoundary usages (i.e., stop using
props.location.pathname as a key) so the error boundary and its children are not
recreated on navigation; if you still need to reset the error boundary on
certain conditions, implement reset logic inside the boundary (or use a stable
identifier, not the pathname) rather than keying by the full URL.
4 participants
Related Issues
Purpose
This PR implements route-level error boundaries to isolate errors to specific routes/features in both the Console and MyAccount applications. Previously, when an error occurred in any route, the entire application would become unresponsive due to only having app-level error boundaries. Users had to reload the entire page to recover.
What this fixes:
Before: Error in one route → Entire app unresponsive → Requires full page reload
After: Error in one route → Only that route shows error → Other routes remain accessible
Related PRs
Checklist
Security checks
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.Technical Details
New Components:
features/admin.core.v1/components/route-error-boundary.tsx- Error boundary for console routesapps/myaccount/src/components/shared/route-error-boundary.tsx- Error boundary for myaccount routesModified Files:
Key Features:
Summary by CodeRabbit
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.