-
Notifications
You must be signed in to change notification settings - Fork 84
fix incorrect route #6828
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 incorrect route #6828
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
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.
Greptile Overview
Greptile Summary
Fixed an incorrect redirect route that was causing users to end up on the home page instead of the monitor page after adding all assets from a system in the Action Center.
Key Changes:
- Changed redirect from string concatenation
${ACTION_CENTER_ROUTE}/${monitorId}to proper Next.js router object withpathnameandqueryparameters - Imported
ACTION_CENTER_WEBSITE_MONITOR_ROUTEconstant to use the correct route pattern - Added proper URL encoding for the
monitorIdquery parameter to match Next.js conventions
The previous route was constructing /data-discovery/action-center/${monitorId} which doesn't correspond to any existing page. The correct route is /data-discovery/action-center/website/[monitorId] which is now properly used via the router object pattern.
Confidence Score: 5/5
- This PR is safe to merge with minimal risk - it's a straightforward bug fix
- The change fixes a clear routing bug where an invalid URL pattern was being used. The fix uses the proper Next.js router pattern with pathname and query parameters, follows existing conventions in the codebase (using the ACTION_CENTER_WEBSITE_MONITOR_ROUTE constant), and includes proper URL encoding. The change is minimal, focused, and addresses the exact issue described in the PR description
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| clients/admin-ui/src/pages/data-discovery/action-center/website/[monitorId]/[systemId]/index.tsx | 5/5 | Fixed redirect route to use correct pathname with query params instead of invalid string concatenation |
Sequence Diagram
sequenceDiagram
participant User
participant MonitorResultAssets as MonitorResultAssets Page
participant Query as useGetDiscoveredSystemAggregateQuery
participant Router as Next.js Router
participant MonitorPage as Website Monitor Page
User->>MonitorResultAssets: Navigate to system assets page
MonitorResultAssets->>Query: Fetch system results for monitorId + systemId
Query-->>MonitorResultAssets: Return systemResults
alt systemResults.items.length === 0
MonitorResultAssets->>Router: router.push({ pathname, query: { monitorId } })
Router->>MonitorPage: Redirect to /action-center/website/[monitorId]
MonitorPage-->>User: Display monitor page
else systemResults.items.length > 0
MonitorResultAssets-->>User: Display DiscoveredAssetsTable
end
1 file reviewed, no comments
lucanovera
left a comment
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.


Ticket ENG-1754
Description Of Changes
Routes got changed, this is the one that slipped through the cracks
Code Changes
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works