-
Notifications
You must be signed in to change notification settings - Fork 140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: error boundary #5003
fix: error boundary #5003
Conversation
b51b984
to
3bc8157
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check #3986 to make sure we solve this fully
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm changing this from Request changes as we checked with design and it's pending their response.
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Outside diff range and nitpick comments (3)
src/app/app.tsx (1)
Line range hint
39-39
: Avoid usingany
for 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
: Replaceany
with 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
: Replaceany
with a specific type for theprism
prop 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 ofloadingState
usingatomFamily
appears to be correct and well-implemented.src/app/app.tsx (1)
44-44
: Simplified rendering logic inConnectedApp
by directly renderingAppRoutes
and conditionallyDevtools
enhances clarity and performance.src/app/ui/utils/prism.tsx (1)
92-92
: Setting the default text color to 'unset' in thetheme
configuration 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 acolor
prop toCodeBlock
enhances 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 ofRouterErrorBoundary
is 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 ofRouterErrorBoundary
in 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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` appropriately
Committable suggestion was skipped due low confidence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Left some optional comments here and there
const error = useRouteError() as Error; | ||
const errorObj = JSON.parse(JSON.stringify(error, Object.getOwnPropertyNames(error))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, I added this a while ago, not recall now why. removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
f7a5a0e
to
a24c6de
Compare
0faee13
to
d48f4c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
src/app/app.tsx (1)
Line range hint
39-39
: Replaceany
with 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 usingAppRoutes
enhances 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 thePrismType
interface promotes reusability and modular design.src/app/features/stacks-transaction-request/contract-deploy-details/contract-deploy-details.tsx (1)
34-34
: Adding acolor
prop with the value#fff
to theCodeBlock
component ensures text visibility against varying backgrounds.src/app/features/errors/app-error-boundary.tsx (1)
51-120
: TheRouterErrorBoundary
component 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 theprism
prop asPrismType
enhances type safety and clarity in theHighlighter
component.
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
RouterErrorBoundary
for enhanced error handling in routes.Improvements
CodeBlock
component in contract deploy details to include a new color prop.Removals
AppErrorBoundary
from the rendering hierarchy.useErrorStackTraceState
function anderrorStackTraceState
constant to simplify state management.Style
theme
constant from'#fff'
to'unset'
.These changes aim to improve error handling, enhance the user interface, and streamline the codebase.