fix: error boundary #5003
fix: error boundary #5003
Conversation
alter-eggo
force-pushed
the
fix/error-boundary
branch
from
b51b984
to
3bc8157
Compare
|
added stack trace |
There was a problem hiding this comment.
@pete-watters just checking if you touched this at all with your work? Is there an open issue to revisit this? I'll check.
|
There is this issue, outdated and can be closed here? #2921 |
|
Thanks for fixing this @alter-eggo . Can you please check if we have an issue before doing stuff like this? We shouldn't have PRs with no issue. There is a task for this, with a design in the |
| const body = 'An error occurred in the app.'; | ||
| const errorPayload = error.message; | ||
|
|
||
| return ( |
There was a problem hiding this comment.
Can you please refactor this so it's a dumb component, receiving some props it needs and located in the ui/components folder?
We should also add a storybook story for it so we can see how it looks
alter-eggo
force-pushed
the
fix/error-boundary
branch
from
3bc8157
to
f7a5a0e
Compare
|
Warning Rate Limit Exceeded@alter-eggo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 49 minutes and 49 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent changes involve significant updates to error handling, UI components, and state management within the application. Key modifications include the removal of the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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
Outside diff range and nitpick comments (3)
src/app/app.tsx (1)
Line range hint
39-39: Avoid usinganyfor type definitions to ensure type safety and better developer experience.- localConfig: localConfig as any, + localConfig: localConfig as LocalConfigType, // Define or import `LocalConfigType` appropriatelysrc/app/ui/utils/prism.tsx (1)
Line range hint
16-16: Replaceanywith specific types in props definitions to enhance type safety and maintainability.- [otherProp: string]: any; + [otherProp: string]: SpecificType; // Replace `SpecificType` with the actual expected typeAlso applies to: 24-24, 32-32, 39-39
src/app/features/stacks-transaction-request/contract-deploy-details/contract-deploy-details.tsx (1)
Line range hint
38-38: Replaceanywith a specific type for theprismprop to ensure type safety and better integration with TypeScript.- prism={Prism as any} + prism={Prism as PrismType} // Define or import `PrismType` appropriately
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- src/app/app.tsx (2 hunks)
- src/app/features/errors/app-error-boundary.tsx (1 hunks)
- src/app/features/stacks-transaction-request/contract-deploy-details/contract-deploy-details.tsx (1 hunks)
- src/app/routes/app-routes.tsx (2 hunks)
- src/app/store/ui/ui.hooks.ts (1 hunks)
- src/app/store/ui/ui.ts (1 hunks)
- src/app/ui/utils/prism.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
- src/app/store/ui/ui.hooks.ts
Additional Context Used
Biome (10)
src/app/app.tsx (1)
39-39: Unexpected any. Specify a different type.
src/app/features/errors/app-error-boundary.tsx (1)
77-77: Unexpected any. Specify a different type.
src/app/features/stacks-transaction-request/contract-deploy-details/contract-deploy-details.tsx (2)
38-38: Unexpected any. Specify a different type.
2-3: Some named imports are only used as types.
src/app/ui/utils/prism.tsx (6)
16-16: Unexpected any. Specify a different type.
24-24: Unexpected any. Specify a different type.
32-32: Unexpected any. Specify a different type.
39-39: Unexpected any. Specify a different type.
1-1: All these imports are only used as types.
2-3: All these imports are only used as types.
Additional comments not posted (6)
src/app/store/ui/ui.ts (1)
Line range hint
1-9: The existing implementation ofloadingStateusingatomFamilyappears to be correct and well-implemented.src/app/app.tsx (1)
44-44: Simplified rendering logic inConnectedAppby directly renderingAppRoutesand conditionallyDevtoolsenhances clarity and performance.src/app/ui/utils/prism.tsx (1)
92-92: Setting the default text color to 'unset' in thethemeconfiguration allows for more flexible theming. Ensure this change aligns with the overall design and theming strategy.src/app/features/stacks-transaction-request/contract-deploy-details/contract-deploy-details.tsx (1)
34-34: Adding acolorprop toCodeBlockenhances visual clarity. Ensure the chosen color (#fff) aligns with the design guidelines.src/app/features/errors/app-error-boundary.tsx (1)
16-101: The implementation ofRouterErrorBoundaryis robust, providing detailed error information and user-friendly interaction options. Ensure all new hooks and components are thoroughly tested.src/app/routes/app-routes.tsx (1)
84-263: Integration ofRouterErrorBoundaryin the routing configuration enhances error handling capabilities. Ensure thorough testing, especially in scenarios where routing errors are expected to occur.
| my="space.05" | ||
| mx="space.05" | ||
| p="space.04" | ||
| prism={Prism as any} |
There was a problem hiding this comment.
Replace any with a specific type for the prism prop to ensure type safety and better integration with TypeScript.
- prism={Prism as any}
+ prism={Prism as PrismType} // Define or import `PrismType` appropriatelyCommittable suggestion was skipped due low confidence.
| const error = useRouteError() as Error; | ||
| const errorObj = JSON.parse(JSON.stringify(error, Object.getOwnPropertyNames(error))); |
There was a problem hiding this comment.
This looks sketchy. Let's do some runtime checking.
Something like?
if (!isError) return children; or nav home?
And why the stringify/parse? I have no idea why this is happening, and nor might the next person reading this code.
There was a problem hiding this comment.
hm, I added this a while ago, not recall now why. removed
There was a problem hiding this comment.
not sure if there could be !isError as it's error boundary. but even if it could -> better to show at least "Unknown error" msg.
if we nav home and error happened on home screen - there will be just blank screen
alter-eggo
force-pushed
the
fix/error-boundary
branch
from
f7a5a0e
to
a24c6de
Compare
alter-eggo
force-pushed
the
fix/error-boundary
branch
from
0faee13
to
d48f4c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
src/app/app.tsx (1)
Line range hint
39-39: Replaceanywith a more specific type to enhance type safety.- localConfig: localConfig as any, + localConfig: localConfig as LocalConfigType, // Assuming LocalConfigType is defined appropriatelysrc/app/common/clarity-prism.ts (1)
Line range hint
55-61: Replace string concatenation with template literals in regex patterns to improve readability and maintainability.- pattern: RegExp(par + '(?:or|and|xor|not|begin|let|if|ok|err|unwrap\\!|unwrap-err\\!|unwrap-panic|unwrap-err-panic|match|try\\!|asserts\\!|map-get\\?|var-get|contract-map-get\\?|get|tuple|define-public|define-private|define-constant|define-map|define-data-var|define-fungible-token|define-non-fungible-token|define-read-only)' + space), + pattern: RegExp(`${par}(?:or|and|xor|not|begin|let|if|ok|err|unwrap\\!|unwrap-err\\!|unwrap-panic|unwrap-err-panic|match|try\\!|asserts\\!|map-get\\?|var-get|contract-map-get\\?|get|tuple|define-public|define-private|define-constant|define-map|define-data-var|define-fungible-token|define-non-fungible-token|define-read-only)${space}`),Also applies to: 66-66, 71-75, 81-84, 90-92, 97-97
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- src/app/app.tsx (2 hunks)
- src/app/common/clarity-prism.ts (1 hunks)
- src/app/features/errors/app-error-boundary.tsx (1 hunks)
- src/app/features/stacks-transaction-request/contract-deploy-details/contract-deploy-details.tsx (1 hunks)
- src/app/routes/app-routes.tsx (2 hunks)
- src/app/store/ui/ui.hooks.ts (1 hunks)
- src/app/store/ui/ui.ts (1 hunks)
- src/app/ui/components/highlighter.tsx (2 hunks)
- src/app/ui/utils/prism.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/app/routes/app-routes.tsx
- src/app/store/ui/ui.hooks.ts
- src/app/store/ui/ui.ts
Additional Context Used
Biome (20)
src/app/app.tsx (1)
39-39: Unexpected any. Specify a different type.
src/app/common/clarity-prism.ts (7)
7-7: Unexpected any. Specify a different type.
55-61: Template literals are preferred over string concatenation.
66-66: Template literals are preferred over string concatenation.
71-75: Template literals are preferred over string concatenation.
81-84: Template literals are preferred over string concatenation.
90-92: Template literals are preferred over string concatenation.
97-97: Template literals are preferred over string concatenation.
src/app/features/stacks-transaction-request/contract-deploy-details/contract-deploy-details.tsx (2)
38-38: Unexpected any. Specify a different type.
2-3: Some named imports are only used as types.
src/app/ui/components/highlighter.tsx (4)
143-143: Unexpected any. Specify a different type.
7-14: Some named imports are only used as types.
35-35: Avoid using the index of an array as key property in an element.
118-118: Avoid using the index of an array as key property in an element.
src/app/ui/utils/prism.tsx (6)
16-16: Unexpected any. Specify a different type.
24-24: Unexpected any. Specify a different type.
32-32: Unexpected any. Specify a different type.
39-39: Unexpected any. Specify a different type.
1-1: All these imports are only used as types.
2-3: All these imports are only used as types.
Additional comments not posted (6)
src/app/app.tsx (1)
44-44: Simplified rendering hierarchy by directly usingAppRoutesenhances clarity and maintainability.src/app/ui/utils/prism.tsx (1)
92-92: Using 'unset' for the color property enhances theming flexibility by allowing inheritance from parent elements.src/app/common/clarity-prism.ts (1)
3-3: Exporting thePrismTypeinterface promotes reusability and modular design.src/app/features/stacks-transaction-request/contract-deploy-details/contract-deploy-details.tsx (1)
34-34: Adding acolorprop with the value#fffto theCodeBlockcomponent ensures text visibility against varying backgrounds.src/app/features/errors/app-error-boundary.tsx (1)
51-120: TheRouterErrorBoundarycomponent enhances error handling with user-friendly messages and functionality for copying the error. Good implementation of user interaction features.src/app/ui/components/highlighter.tsx (1)
137-137: Explicitly typing theprismprop asPrismTypeenhances type safety and clarity in theHighlightercomponent.
alter-eggo
force-pushed
the
fix/error-boundary
branch
from
d48f4c8
to
91c981b
Compare
I seems current app's error boundary doesn't work. I decided not to try fix that legacy code, and just added new component for this. I believe it's ok for now, still way better than default error screen:
Summary by CodeRabbit
New Features
RouterErrorBoundaryfor enhanced error handling in routes.Improvements
CodeBlockcomponent in contract deploy details to include a new color prop.Removals
AppErrorBoundaryfrom the rendering hierarchy.useErrorStackTraceStatefunction anderrorStackTraceStateconstant to simplify state management.Style
themeconstant from'#fff'to'unset'.These changes aim to improve error handling, enhance the user interface, and streamline the codebase.