-
Notifications
You must be signed in to change notification settings - Fork 15
fix(ensadmin): remove name param when not on /name route
#1453
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
Open
Y3drk
wants to merge
4
commits into
main
Choose a base branch
from
y3drk/fix/issue-1435
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+74
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 9581785 The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…gardless of route.
…/fix/issue-1435
…/fix/issue-1435
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fix(ensadmin): remove
nameparam when not on/namerouteReviewer Focus (Read This First)
What reviewers should focus on
next/routerelements and global hooks, my confidence being even worse when they're combined. Even though I read up on Next.js's docs and studied our other hooks, I fear that this fix might not be performance-efficient.The way I understand it, the proposed implementation should be called only when the URL changes and not affect the loading of the content, but I'd really value a verification on that part.
Problem & Motivation
Why this exists
?name=valueparam is added to the URL, it stays there permanently, no matter where we are in the app, and we want to limit this param only to the/namerouteWhat Changed (Concrete)
What actually changed
Changes are limited to
apps/ensadmin.UseClearUrlParams(@apps/ensadmin/src/hooks/use-clear-url-params.tsx)apps/ensadmin/src/app/layout.tsx:66) and spec'ed it to only removenameparam when not on/nameDue to that, on every route change (either by UI or manually editing the URL), the page's URL will be checked for redundant parameters, and any unwanted ones will be cleared.
Design & Planning
How this approach was chosen
Path to current solution:
nameparameter, but classified it as a bad idea due to the possible need to handle other parameters in the future and the intuition that such solutions are against every reusability-related good practice possible.Considered alternatives:
useRawConnectionUrlParamis currently used → calling it separately on all of our pages insideensadmin/src/appSelf-Review
What you caught yourself
UseClearUrlParams) but cannot think of anything betterCross-Codebase Alignment
Related code you checked
useRawConnectionUrlParam&useConnectionsLibraryqueryParams,pathParams,next/navigationapps/ensadmin/src/hooks/use-connection-url-param.tsx,apps/ensadmin/src/hooks/use-connections-library.tsx,apps/ensadmin/src/app/name/page.tsxDownstream & Consumer Impact
Who this affects and how
nameparameter provided for the /connection route?" should not be popping up anymore).apps/ensadmin/src/hooks/use-clear-url-params.tsxTesting Evidence
How this was validated
networktab in dev tools. Everything seems fine.If this is wrong, what breaks first:
CypressorPlaywright(a meaningful overhead to introduce, well-known issue for the entire ENSAdmin) → essentially we're limited to manual testing for the UIknow gaps)Scope Reductions
What you intentionally didn't do
Risk Analysis
How this could go wrong
useClearUrlParamshook will be called exactly once per route change. This affects both the logic (clearing up redundant params) and efficiency of the App.nameparam not being deletedconnectionparam being deletedUseClearUrlParamshook fromapps/ensadmin/src/app/layout.tsx(will simply rollback to the state before this PR)Pre-Review Checklist (Blocking)