fix(ensadmin): preserve connection param only on sidebar navigation#1840
fix(ensadmin): preserve connection param only on sidebar navigation#1840
Conversation
…on query param, preventing page-specific params like name from leaking between routes.
… query param, preventing page-specific params like name from leaking between routes
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
🦋 Changeset detectedLatest commit: d013230 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughThis PR refactors query parameter handling in the ensadmin sidebar navigation. It removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes the root cause of query param leakage during sidebar navigation by rewriting Key changes:
The approach is clean and correct: Confidence Score: 5/5Safe to merge — the change is a well-scoped fix with no new logic introduced and no remaining P0/P1 issues. The refactor is straightforward: a complex, fragile special-case in No files require special attention. Important Files Changed
Reviews (1): Last reviewed commit: "lint" | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
This PR fixes ENSAdmin sidebar navigation so that only the globally-relevant connection query param is preserved across route changes, preventing page-specific params (e.g. name) from leaking between pages. It also removes the downstream workaround hook that was previously clearing URL params at the layout level.
Changes:
- Update sidebar link building to forward only
connectioninstead of all current search params. - Remove
UseClearUrlParamsusage from the app layout and delete the hook. - Add a changeset to publish the behavior change as a patch.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| apps/ensadmin/src/components/nav-main.tsx | Restricts preserved query params on sidebar navigation to connection only. |
| apps/ensadmin/src/app/layout.tsx | Removes the layout-level URL param cleanup hook usage. |
| apps/ensadmin/src/hooks/use-clear-url-params.tsx | Deletes the now-unneeded hook implementation. |
| .changeset/curvy-fans-begin.md | Records the ENSAdmin patch change for release/versioning. |
Comments suppressed due to low confidence (2)
apps/ensadmin/src/components/nav-main.tsx:48
- There is already a centralized helper for preserving the
connectionquery param (useRawConnectionUrlParam().retainCurrentRawConnectionUrlParam) and it also avoids edge cases around existing query strings / hash fragments. Reusing that helper here (or otherwise centralizing theconnectionparam key) would prevent duplication and keepconnectionURL handling consistent across the app.
const separator = url.includes("?") ? "&" : "?";
return `${url}${separator}connection=${encodeURIComponent(connection)}`;
}
return url;
};
apps/ensadmin/src/components/nav-main.tsx:51
appendQueryParamsno longer appends arbitrary query params (it now only preserves theconnectionparam). Renaming it to something likeappendConnectionParam/retainConnectionParamwould make the intent clearer and reduce confusion for future edits.
const separator = url.includes("?") ? "&" : "?";
return `${url}${separator}connection=${encodeURIComponent(connection)}`;
}
return url;
};
const isActive = (url: UrlString): boolean => {
const urlPathname = url.split("?")[0];
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@notrab Looks good. Shared 1 small suggestions please feel welcome to merge when ready 👍
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
connectionquery param, preventing page-specific params likenamefrom leaking between routes.UseClearUrlParamshook that was working around this issue downstream cc @Y3drkWhy
vitalik.ethon/inspect/recordsand then clicking "Names" would open/name?name=vitalik.ethinstead of the empty explore page.The
UseClearUrlParamshook (added in #1453) was a band-aid for this imo. Fixing the root cause innav-main.tsxmakes it unnecessary.Testing
connectionwhen navigating between routes that usename(e.g./inspect/records→/name,/name=>/status).Notes for Reviewer (Optional)
The original
appendQueryParamsinnav-main.tsxforwarded all search params to sidebar links and only had a special case for strippingname/strategywhen leaving/inspector.The new version explicitly forwards only
connection, which is the only globally-relevant param.Pre-Review Checklist (Blocking)