fix: preserve ?search and ?section through URL normalization redirect#418
Merged
fix: preserve ?search and ?section through URL normalization redirect#418
Conversation
…rect When opening a URL like /#/?search=foo§ion=bar (no version segment), the LocationProvider redirects to /#/<latest-hash>?v=<name> and the resulting hashchange used to fire a second handleHashChange that read undefined for both fields and wiped state before Search/Outline could consume it. The redirect now opts in to keeping ?search/?section in the rewritten URL via locationParamsToHash's new includeSearchSection flag, so the second pass reads the original values and state survives. Subsequent navigation goes through the default path and still drops these fields, matching the existing "consume once" semantics. Adds e2e coverage in tools/snapshot-tests/tests/url-params.spec.ts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for graypaper-reader ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (4)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR modifies URL parameter handling in LocationProvider to preserve Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/LocationProvider/LocationProvider.tsx (1)
86-114: Refactor duplicated canonical redirect block into a local helper.The redirect-hash construction appears twice with identical logic. Extracting it avoids branch drift in future edits.
♻️ Suggested deduplication
const handleHashChange = useCallback(() => { const { rest: newHash, search, section, split } = extractSearchParams(window.location.hash); const resolvedSplit = split ? (resolveFullVersion(split) ?? undefined) : undefined; + const rewriteCanonicalHash = (version: string) => { + const redirectHash = locationParamsToHash({ version, search, section, split: resolvedSplit }, metadata, { + includeSearchSection: true, + }); + window.location.hash = redirectHash; + }; if (!newHash.startsWith(SEGMENT_SEPARATOR)) { const version = metadata.latest; setLocationParams((params) => ({ ...params, version, search, section, split: resolvedSplit, })); - const redirectHash = locationParamsToHash({ version, search, section, split: resolvedSplit }, metadata, { - includeSearchSection: true, - }); - window.location.hash = redirectHash; + rewriteCanonicalHash(version); return; } @@ if (!fullVersion) { const version = metadata.latest; setLocationParams((params) => ({ ...params, version, search, section, split: resolvedSplit, })); - const redirectHash = locationParamsToHash({ version, search, section, split: resolvedSplit }, metadata, { - includeSearchSection: true, - }); - window.location.hash = redirectHash; + rewriteCanonicalHash(version); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/LocationProvider/LocationProvider.tsx` around lines 86 - 114, There are two identical blocks that build a canonical redirect hash and set state/window.location.hash; extract them into a local helper (e.g., redirectToCanonical) inside LocationProvider.tsx that accepts a version (or uses metadata.latest when needed), calls setLocationParams with { version, search, section, split: resolvedSplit }, constructs the redirectHash via locationParamsToHash(..., metadata, { includeSearchSection: true }), and sets window.location.hash = redirectHash; then replace both duplicated blocks with a call to this helper (use resolveFullVersion when computing the version before calling it).tools/snapshot-tests/tests/url-params.spec.ts (1)
11-15: Avoid duplicated selector literals ingetScrollTop.Reuse
PDF_SCROLL_CONTAINERviapage.evaluatearg to prevent selector drift.🧩 Small maintainability tweak
async function getScrollTop(page: import("@playwright/test").Page) { - return await page.evaluate(() => { - const viewer = document.querySelector(".pdfViewer"); + return await page.evaluate((selector) => { + const viewer = document.querySelector(selector); return viewer?.parentElement?.scrollTop ?? 0; - }); + }, PDF_SCROLL_CONTAINER); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/snapshot-tests/tests/url-params.spec.ts` around lines 11 - 15, The getScrollTop function duplicates the ".pdfViewer" selector literal; change it to accept/use the shared PDF_SCROLL_CONTAINER constant by passing it into page.evaluate as an argument (e.g., call page.evaluate((sel) => { const viewer = document.querySelector(sel); ... }, PDF_SCROLL_CONTAINER)) so the selector is not hard-coded inside the evaluated function; update getScrollTop to reference PDF_SCROLL_CONTAINER and remove the inline string literal to prevent selector drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/LocationProvider/LocationProvider.tsx`:
- Around line 86-114: There are two identical blocks that build a canonical
redirect hash and set state/window.location.hash; extract them into a local
helper (e.g., redirectToCanonical) inside LocationProvider.tsx that accepts a
version (or uses metadata.latest when needed), calls setLocationParams with {
version, search, section, split: resolvedSplit }, constructs the redirectHash
via locationParamsToHash(..., metadata, { includeSearchSection: true }), and
sets window.location.hash = redirectHash; then replace both duplicated blocks
with a call to this helper (use resolveFullVersion when computing the version
before calling it).
In `@tools/snapshot-tests/tests/url-params.spec.ts`:
- Around line 11-15: The getScrollTop function duplicates the ".pdfViewer"
selector literal; change it to accept/use the shared PDF_SCROLL_CONTAINER
constant by passing it into page.evaluate as an argument (e.g., call
page.evaluate((sel) => { const viewer = document.querySelector(sel); ... },
PDF_SCROLL_CONTAINER)) so the selector is not hard-coded inside the evaluated
function; update getScrollTop to reference PDF_SCROLL_CONTAINER and remove the
inline string literal to prevent selector drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 902779ec-e270-4085-8dba-f5f13a013912
📒 Files selected for processing (3)
src/components/LocationProvider/LocationProvider.tsxsrc/components/LocationProvider/utils/locationParamsToHash.tstools/snapshot-tests/tests/url-params.spec.ts
Contributor
Visual Regression Test Report ✅ PassedGithub run id: 24995292162 🔗 Artifacts: Download |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
/#/?search=foo§ion=bar(no version segment), theLocationProviderredirect to/#/<hash>?v=<name>was stripping?search/?sectionand a secondhashchangeimmediately wiped them from state beforeSearchandOutlinecould consume them. The redirect now opts in to keeping those fields via a newincludeSearchSectionflag onlocationParamsToHash, so the rewritten URL still carries them and the second pass restores the same state.Version,SelectionProvider,NoteManager,SplitScreenProvider) keep using the default serialization, so subsequent navigation still drops?search/?sectionand preserves the existing "consume once" semantics — version changes and manual navigation to a plain hash correctly clear them.tools/snapshot-tests/tests/url-params.spec.tscovers the original bug for both URL forms (with/without a version segment), the combined search+section case, and the regression scenario where a follow-up hash change must drop the fields.Test plan
npm test(unit tests, 96 passing)npx biome cicleannpx playwright test url-params.spec.ts(10/10 across light + dark mode)/#/?search=protocol§ion=Headerand confirm input is filled and the viewer scrolls; then change version and confirm the URL no longer carries those params🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests