-
Notifications
You must be signed in to change notification settings - Fork 4
fix: show stack trace in error page #137
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
Conversation
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.
Pull request overview
This pull request adds functionality to display stack traces on the error page to aid debugging during development. The changes include modifying the ErrorPage component to accept and display stack traces from route errors, refactoring error handling logic to extract the route error outside the custom hook, and updating tests to verify stack trace display.
Changes:
- Modified
ErrorPagecomponent to display stack traces when available - Refactored
useRouteErrorDerivedMessageto accept route error as parameter instead of callinguseRouteErrorinternally - Updated test to verify stack trace rendering
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/components/ErrorPage/ErrorPage.tsx | Added stack trace display functionality, created new interfaces for error types, and refactored error handling logic |
| src/components/ErrorPage/tests/ErrorPage.test.tsx | Updated test to verify stack trace display and changed mock implementation from throwing to returning an error |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <Image className="mb-3" src={ErrorIllustration} fluid alt="Something went wrong error page image" /> | ||
| <h2>{intl.formatMessage(errorPageMessages.errorHeader)}</h2> | ||
| {message && (<p className="mb-0">{message}</p>)} | ||
| {stack && (<p className="mb-0">{stack}</p>)} |
Copilot
AI
Jan 14, 2026
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.
Stack traces typically contain newline characters and can be quite long. Rendering the entire stack trace as plain text within a paragraph tag may result in poor formatting and readability. Consider wrapping the stack trace in a <pre> or <code> element, adding CSS classes for proper formatting (e.g., monospace font, overflow handling), or truncating/collapsing long stack traces.
| {stack && (<p className="mb-0">{stack}</p>)} | |
| {stack && (<pre className="mb-0">{stack}</pre>)} |
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b8fd9dc to
e2055bc
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interface UnknownError { | ||
| stack?: string; | ||
| } |
Copilot
AI
Jan 15, 2026
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.
The interface name 'UnknownError' is misleading since it only contains a stack property. This interface is used to type route errors that may or may not be Error objects. Consider renaming to 'RouteErrorWithStack' or 'ErrorLike' to better reflect its purpose.
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interface UnknownError { | ||
| stack?: string; | ||
| } |
Copilot
AI
Jan 15, 2026
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.
The UnknownError interface only includes the stack property, but the routeError is cast to this type and then passed to getErrorMessage which expects properties like status, statusText, data, and message. This type casting could mask type safety issues. Consider making UnknownError extend or include all properties that might be needed, or use a union type that includes Error and other potential error types.
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.
Just a comment on switching the HTML tag to pre from p
| <Image className="mb-3" src={ErrorIllustration} fluid alt="Something went wrong error page image" /> | ||
| <h2>{intl.formatMessage(errorPageMessages.errorHeader)}</h2> | ||
| {message && (<p className="mb-0">{message}</p>)} | ||
| {stack && (<p className="mb-0">{stack}</p>)} |
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 would follow this suggestion from co-pilot to allow the stacktrace to be formatted correctly.
https://github.com/edx/frontend-app-enterprise-checkout/pull/137/changes#r2692239949
It is also what is currently done in the learner portal.
https://github.com/openedx/frontend-app-learner-portal-enterprise/blob/0a20f3252fcfdae585b453e31856ffd36029890e/src/components/app/AppErrorBoundary.tsx#L196C39-L196C42
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/components/ErrorPage/tests/ErrorPage.test.tsx:129
- Consider adding a test case to verify that the stack trace is not displayed when the error object doesn't have a stack property (e.g., when routeError is a Response object, string, or other type). This ensures the optional chaining on line 109 of ErrorPage.tsx works correctly and doesn't attempt to display undefined values.
it('displays default error message when useRouteError returns an unknown type', () => {
(useRouteError as jest.Mock).mockReturnValue(12345);
renderComponent();
validateText("We're sorry, something went wrong");
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const ErrorPage = ({ message }: ErrorPageProps) => { | ||
| const derivedErrorMessage = useRouteErrorDerivedMessage(); | ||
| const routeError = useRouteError() as UnknownError; |
Copilot
AI
Jan 16, 2026
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.
The type assertion as UnknownError on line 101 is unsafe because useRouteError() can return various types (Error, Response, string, number, etc.). The UnknownError interface only has an optional stack property, which means accessing routeError?.stack might incorrectly assume a stack exists. Consider using a type guard to check if the error is an Error instance before accessing the stack property.
| const getRouteErrorDerivedMessage = (routeError: UnknownError): string | undefined => { | ||
| try { | ||
| const routeError = useRouteError(); | ||
| return routeError ? getErrorMessage(routeError) : undefined; | ||
| } catch { | ||
| return undefined; | ||
| } | ||
| }; |
Copilot
AI
Jan 16, 2026
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.
The refactoring of useRouteErrorDerivedMessage to getRouteErrorDerivedMessage with an explicit parameter is good for testability. However, the try-catch block in this function may now be unnecessary since the error handling is moved upstream. The catch block will never be reached in normal circumstances because getErrorMessage doesn't throw errors. Consider removing the try-catch or adding a comment explaining what errors it's intended to catch.
| it('displays error message and stack trace from useRouteError if it is an Error object', () => { | ||
| (useRouteError as jest.Mock).mockImplementation(() => { | ||
| const error = new Error('Hook failed'); | ||
| error.stack = 'Insert stack trace here'; | ||
| return error; | ||
| }); | ||
| renderComponent(); | ||
| validateText("We're sorry, something went wrong"); | ||
| validateText('Hook failed'); | ||
| validateText('Insert stack trace here'); | ||
| }); |
Copilot
AI
Jan 16, 2026
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.
The new test case validates that both the error message and stack trace are displayed, but it doesn't verify that they are rendered in separate pre elements as expected. Consider checking that the stack trace is actually displayed in its own element separate from the error message to ensure the UI structure is correct.
| validateText('500 Internal Server Error'); | ||
| }); |
Copilot
AI
Jan 16, 2026
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.
The removed test case 'handles useRouteError throwing an error' was testing a scenario where useRouteError itself throws during execution. With the refactored code, if useRouteError throws on line 101, it will not be caught since the try-catch is now only in getRouteErrorDerivedMessage around getErrorMessage, not around the useRouteError call. Consider either restoring this test case or adding error boundary handling for the case where useRouteError might throw.
| }; | ||
|
|
||
| const useRouteErrorDerivedMessage = (): string | undefined => { | ||
| const getRouteErrorDerivedMessage = (routeError: UnknownError): string | undefined => { |
Copilot
AI
Jan 16, 2026
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.
The parameter type for getRouteErrorDerivedMessage is UnknownError, but it passes this to getErrorMessage which expects unknown. This type mismatch creates unnecessary type narrowing. Since getErrorMessage is designed to handle any unknown type and has its own type guards, getRouteErrorDerivedMessage should accept unknown as well rather than UnknownError.
| const getRouteErrorDerivedMessage = (routeError: UnknownError): string | undefined => { | |
| const getRouteErrorDerivedMessage = (routeError: unknown): string | undefined => { |
Jira Ticket: ENT-11060
This pull request enhances the
ErrorPagecomponent to display additional error details, specifically the stack trace, when available. It also updates the error handling logic and associated tests to support this new functionality.Stage Devstack Testing Instructions
git apply diff.txt(sample diff.txt)npm run start:stage