-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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: correct redirection causing intermediate 404 #7400
base: main
Are you sure you want to change the base?
Conversation
Welcome to the SigNoz community! Thank you for your first pull request and making this project better. 🤗 |
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.
❌ Changes requested. Reviewed everything up to e08fac3 in 2 minutes and 4 seconds
More details
- Looked at
23
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. frontend/src/AppRoutes/index.tsx:63
- Draft comment:
The condition for 'shouldShowNotFound' is inverted. Currently it uses 'isFetchingUser || isFetchingLicenses || isFetchingFeatureFlags', which is true while data is still loading. This will cause the NotFound route to render during the async fetches (leading to an intermediate 404). It should instead check that all fetches are complete, e.g. '!isFetchingUser && !isFetchingLicenses && !isFetchingFeatureFlags'. - Reason this comment was not posted:
Marked as duplicate.
2. frontend/src/AppRoutes/index.tsx:316
- Draft comment:
The NotFound route is conditionally rendered based on 'shouldShowNotFound'. Due to the inverted logic, it may render prematurely. Once the condition is corrected, the NotFound route should only be available when fetches are complete and no route matches. - Reason this comment was not posted:
Marked as duplicate.
3. frontend/src/AppRoutes/index.tsx:274
- Draft comment:
Typographical errors in the comments: 'calls fails' should be 'calls fail' and 'indefinitive loading' should be 'indefinite loading'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_ddUO0hlKwSrkbl6b
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Summary
Intermediate 404 page on visiting deployment URL
Added state canShowNotFound .This state update enables the display of a "Not Found" page () when necessary.
Related Issues / PR's
#7384
Screenshots
Affected Areas and Manually Tested Areas
Important
Fixes intermediate 404 error by checking async fetching states before rendering NotFound component in
index.tsx
.isFetchingUser
,isFetchingLicenses
, andisFetchingFeatureFlags
inApp()
inindex.tsx
.shouldShowNotFound
condition to determine ifNotFound
component should be rendered.<Route path="*" component={NotFound} />
to render only ifshouldShowNotFound
is true inindex.tsx
.This description was created by
for e08fac3. It will automatically update as commits are pushed.