test: add tests for split screen, bottom drawer, and scroll sync#381
Merged
test: add tests for split screen, bottom drawer, and scroll sync#381
Conversation
Add component/hook-level tests for LocationProvider URL parsing, SplitScreenProvider state transitions, ScrollSync publisher/consumer behavior, ZoomSyncBridge scale propagation, BottomDrawer expand/collapse/swipe gestures, SidebarOverlay open/close/ escape dismiss, and Sidebar split tab visibility rules. Set up vitest with jsdom environment, @testing-library/react, and polyfills for DOMMatrix, ResizeObserver, matchMedia, and localStorage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for graypaper-reader ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExtracts LocationProvider helpers into utils, adds Vitest + React Testing Library tests and jsdom test setup/polyfills, updates test devDependencies, and adjusts LocationProvider to import the new utilities; no public API or runtime exports changed. Changes
Sequence Diagram(s)(omitted) 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 docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Visual Regression Test Report ✅ PassedGithub run id: 23794854889 🔗 Artifacts: Download |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
vitest.setup.ts (1)
5-7: ClearlocalStoragein globalafterEachto avoid cross-test state leakage.Several new tests persist values (e.g., theme); clearing storage centrally keeps suites order-independent.
💡 Suggested patch
afterEach(() => { cleanup(); + if (typeof window !== "undefined" && window.localStorage) { + window.localStorage.clear(); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vitest.setup.ts` around lines 5 - 7, The global afterEach currently calls cleanup() but doesn't clear persisted browser state; update the global teardown in the vitest setup by adding a localStorage.clear() (or sessionStorage.clear() if needed) alongside the existing afterEach cleanup so tests don't leak theme or other values; locate the afterEach callback that invokes cleanup() and append a call to clear storage (e.g., localStorage.clear()) to run after every test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/LocationProvider/LocationProvider.spec.ts`:
- Around line 12-31: The spec currently re-implements the private parsing logic
(extractSearchParams) inline which makes tests validate duplicated code instead
of the production implementation; replace the duplicated implementations in
LocationProvider.spec.ts (both occurrences around the current
extractSearchParams blocks) by moving the parsing/resolution logic into a small
shared module (e.g., export a parseHash or extractSearchParams function) that is
imported and used by LocationProvider.tsx and by this spec so tests run the
exact production code path; update imports in LocationProvider.tsx and
LocationProvider.spec.ts to use the new shared function and remove the local
copies.
In `@src/components/Sidebar/Sidebar.spec.tsx`:
- Around line 21-28: TypeScript complains because mockUseBreakpoint is declared
as a no-arg function but you call it with spread args inside the vi.mock
wrapper; either make mockUseBreakpoint accept variadic args (e.g., change its
declaration to vi.fn((...args: unknown[]) => false) or vi.fn<any, unknown[]>())
or stop forwarding args in the wrapper by replacing useBreakpoint: (...args:
unknown[]) => mockUseBreakpoint(...args) with useBreakpoint: () =>
mockUseBreakpoint(); update the mock implementation in the vi.mock block
(references: mockUseBreakpoint, useBreakpoint, vi.mock) accordingly so the types
align and the TS2556 error disappears.
---
Nitpick comments:
In `@vitest.setup.ts`:
- Around line 5-7: The global afterEach currently calls cleanup() but doesn't
clear persisted browser state; update the global teardown in the vitest setup by
adding a localStorage.clear() (or sessionStorage.clear() if needed) alongside
the existing afterEach cleanup so tests don't leak theme or other values; locate
the afterEach callback that invokes cleanup() and append a call to clear storage
(e.g., localStorage.clear()) to run after every test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 703cca56-3b15-4daa-a328-40480640a828
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.jsonsrc/components/BottomDrawer/BottomDrawer.spec.tsxsrc/components/LocationProvider/LocationProvider.spec.tssrc/components/Sidebar/Sidebar.spec.tsxsrc/components/SidebarOverlay/SidebarOverlay.spec.tsxsrc/components/SplitScreenProvider/SplitScreenProvider.spec.tsxsrc/components/ZoomSync/ZoomSync.spec.tsxsrc/hooks/useScrollSync.spec.tsxvitest.config.tsvitest.setup.ts
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/Sidebar/Sidebar.spec.tsx (2)
122-130: RedundantlocalStorage.removeItemcall.Line 123 removes
"gp-tab"from localStorage, butbeforeEach(line 62) already does this before every test. This duplication doesn't cause harm but adds noise.🧹 Remove redundant cleanup
it("defaults to outline tab", () => { - localStorage.removeItem("gp-tab"); const ctx = createSplitContext(); renderSidebar(ctx);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Sidebar/Sidebar.spec.tsx` around lines 122 - 130, The test "defaults to outline tab" in Sidebar.spec.tsx contains a redundant localStorage.removeItem("gp-tab") call; remove that line from the it("defaults to outline tab") test body since the beforeEach cleanup already clears "gp-tab" for every test (keep the rest of the test, e.g., createSplitContext(), renderSidebar(ctx), and the assertion on screen.getByTestId("tab-outline")).
13-18: CallingonSearchFinishedduring render is a React anti-pattern.The Search mock invokes
onSearchFinished(false)synchronously during render, which triggers a side effect (and potentially state updates) during the render phase. This can cause React warnings and unpredictable test behavior.♻️ Proposed fix: defer the callback or use a wrapper
vi.mock("../Search/Search", () => ({ - Search: ({ onSearchFinished }: { onSearchFinished: (v: boolean) => void }) => { - onSearchFinished(false); - return <div data-testid="mock-search">Search</div>; - }, + Search: ({ onSearchFinished }: { onSearchFinished: (v: boolean) => void }) => { + React.useEffect(() => { + onSearchFinished(false); + }, [onSearchFinished]); + return <div data-testid="mock-search">Search</div>; + }, }));You'll need to add
import React from "react"at the top, or alternatively use a simpler approach if the callback isn't needed for these tests:vi.mock("../Search/Search", () => ({ - Search: ({ onSearchFinished }: { onSearchFinished: (v: boolean) => void }) => { - onSearchFinished(false); - return <div data-testid="mock-search">Search</div>; - }, + Search: () => <div data-testid="mock-search">Search</div>, }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Sidebar/Sidebar.spec.tsx` around lines 13 - 18, The mock Search component currently calls the onSearchFinished prop synchronously during render (Search mock, prop onSearchFinished), which is an anti-pattern; fix it by importing React and deferring the callback so it runs after render (e.g., wrap the call in a useEffect inside the mock component or call it inside a setTimeout 0), ensuring the mock still returns the <div data-testid="mock-search"> but triggers onSearchFinished(false) only after mount to avoid side effects during render.
🤖 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/Sidebar/Sidebar.spec.tsx`:
- Around line 122-130: The test "defaults to outline tab" in Sidebar.spec.tsx
contains a redundant localStorage.removeItem("gp-tab") call; remove that line
from the it("defaults to outline tab") test body since the beforeEach cleanup
already clears "gp-tab" for every test (keep the rest of the test, e.g.,
createSplitContext(), renderSidebar(ctx), and the assertion on
screen.getByTestId("tab-outline")).
- Around line 13-18: The mock Search component currently calls the
onSearchFinished prop synchronously during render (Search mock, prop
onSearchFinished), which is an anti-pattern; fix it by importing React and
deferring the callback so it runs after render (e.g., wrap the call in a
useEffect inside the mock component or call it inside a setTimeout 0), ensuring
the mock still returns the <div data-testid="mock-search"> but triggers
onSearchFinished(false) only after mount to avoid side effects during render.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 635fa690-82df-4019-9553-8f384406fe33
📒 Files selected for processing (1)
src/components/Sidebar/Sidebar.spec.tsx
…red utils Tests now import the real production functions instead of re-implementing them locally, ensuring tests verify actual shipped behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/LocationProvider/utils/resolveFullVersion.ts (1)
14-16: Match againstVersionInfo.hash, not the record key.The helper's contract guarantees
versionsvalues carry ahash, but it doesn't guarantee the record key is the same string. Matching on keys bakes in the current storage shape and makes this utility easier to break later if metadata gets re-keyed.♻️ Proposed refactor
const byHash = - Object.keys(metadata.versions).find((version) => version.startsWith(shortVersion)) ?? + Object.values(metadata.versions).find((version) => version.hash.startsWith(shortVersion))?.hash ?? (metadata.nightly?.hash.startsWith(shortVersion) ? metadata.nightly.hash : null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/LocationProvider/utils/resolveFullVersion.ts` around lines 14 - 16, The current byHash lookup matches shortVersion against the record key (Object.keys(metadata.versions)), which is fragile; change it to match against the VersionInfo.hash property instead (e.g., iterate Object.values(metadata.versions) or Object.entries and check value.hash.startsWith(shortVersion)) and return the matching value.hash; keep the nightly fallback (metadata.nightly?.hash.startsWith(shortVersion) ? metadata.nightly.hash : null) unchanged so resolveFullVersion and any callers use the actual VersionInfo.hash rather than the map key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/LocationProvider/LocationProvider.spec.ts`:
- Around line 74-76: The two specs around resolveFullVersion are insufficiently
strict: update the "resolves 'nightly' literal (case-insensitive concept)" test
to call resolveFullVersion with a different-cased literal (e.g., "NightLy")
against the same metadata to assert case-insensitivity; and update the "prefers
hash prefix over friendly name" test to include a conflicting friendly-name
entry in the metadata (so there is both a friendly-name match and a hash-prefix
match) and assert that resolveFullVersion returns the hash-prefixed version
rather than the friendly name. Reference resolveFullVersion and the metadata
test fixture when making these changes so the tests actually exercise case
normalization and conflict resolution.
In `@src/components/LocationProvider/utils/extractSearchParams.ts`:
- Around line 5-23: The hash parser is brittle: query keys can overwrite the
path-derived rest, values with '=' are truncated, and malformed percent-escapes
can throw during decode; update extractSearchParams to only allow setting the
known search keys (v, search, section, split) and never overwrite rest, split
each "key=value" at the first '=' (so values may contain '='), and wrap
decodeURIComponent in a try/catch fallback (e.g., use the raw value or a safe
decode) to avoid throwing on malformed escapes before calling handleHashChange;
locate the parsing loop that iterates searchParams.split("&"), change its
splitting logic to use indexOf to separate key and value, check key against the
allowed set (exclude rest), and perform safe decoding before assigning into
result.
---
Nitpick comments:
In `@src/components/LocationProvider/utils/resolveFullVersion.ts`:
- Around line 14-16: The current byHash lookup matches shortVersion against the
record key (Object.keys(metadata.versions)), which is fragile; change it to
match against the VersionInfo.hash property instead (e.g., iterate
Object.values(metadata.versions) or Object.entries and check
value.hash.startsWith(shortVersion)) and return the matching value.hash; keep
the nightly fallback (metadata.nightly?.hash.startsWith(shortVersion) ?
metadata.nightly.hash : null) unchanged so resolveFullVersion and any callers
use the actual VersionInfo.hash rather than the map key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d5038b6-3a98-496b-a6a1-0a4c811e3a78
⛔ Files ignored due to path filters (5)
tools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/homepage-outline-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-dropdown-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-dropdown-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-selected-on-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/split-screen.spec.ts-snapshots/split-view-opened-light-mode-linux.pngis excluded by!**/*.png
📒 Files selected for processing (4)
src/components/LocationProvider/LocationProvider.spec.tssrc/components/LocationProvider/LocationProvider.tsxsrc/components/LocationProvider/utils/extractSearchParams.tssrc/components/LocationProvider/utils/resolveFullVersion.ts
…red utils - extractSearchParams: prevent `rest` overwrite via query params, split at first `=` to preserve values containing `=`, wrap decodeURIComponent in try/catch for malformed percent-escapes - resolveFullVersion: match against VersionInfo.hash property instead of record keys - Tests: case-insensitive "NightLy" literal, conflicting hash-prefix vs friendly-name, rest overwrite prevention, malformed escape handling Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/LocationProvider/utils/resolveFullVersion.ts (1)
1-9: Prefer reusing canonical metadata types instead of redefining local shapes.
VersionInfo/ResolveVersionInputduplicate existing metadata contracts and can drift fromIMetadata/IVersionInfo. Consider deriving this input type from the shared source type to keep refactors safe.Proposed refactor
-interface VersionInfo { - hash: string; - name?: string; -} - -interface ResolveVersionInput { - versions: Record<string, VersionInfo>; - nightly?: { hash: string } | null; -} +import type { IMetadata } from "../../MetadataProvider/MetadataProvider"; + +type ResolveVersionInput = Pick<IMetadata, "versions" | "nightly">;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/LocationProvider/utils/resolveFullVersion.ts` around lines 1 - 9, The local types VersionInfo and ResolveVersionInput duplicate shared metadata shapes; update resolveFullVersion to reuse the canonical types (e.g., import IVersionInfo and IMetadata or the existing shared types) instead of redefining VersionInfo/ResolveVersionInput so they stay in sync with metadata contracts; replace the local interfaces with the imported types in the function signature and any usages within resolveFullVersion to derive nightly and versions from the shared type definitions (keep the same property names like hash/name to preserve logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/LocationProvider/utils/resolveFullVersion.ts`:
- Around line 14-17: The prefix lookup in resolveFullVersion.ts currently picks
the first hash that startsWith(shortVersion) (byHash) which can silently choose
the wrong version when multiple hashes share the same prefix; change the logic
in the function that computes byHash to collect all matching hashes from
Object.values(metadata.versions).map(v => v.hash) and also include
metadata.nightly.hash if it startsWith(shortVersion), then if matches.length ===
1 return that single hash, otherwise (matches.length === 0 or >1) return null to
explicitly handle no-match and ambiguous-match cases; update any early-return
that used byHash to use this unique-match behavior instead.
---
Nitpick comments:
In `@src/components/LocationProvider/utils/resolveFullVersion.ts`:
- Around line 1-9: The local types VersionInfo and ResolveVersionInput duplicate
shared metadata shapes; update resolveFullVersion to reuse the canonical types
(e.g., import IVersionInfo and IMetadata or the existing shared types) instead
of redefining VersionInfo/ResolveVersionInput so they stay in sync with metadata
contracts; replace the local interfaces with the imported types in the function
signature and any usages within resolveFullVersion to derive nightly and
versions from the shared type definitions (keep the same property names like
hash/name to preserve logic).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f33bc62e-3290-49fc-a266-87a0dc7981be
⛔ Files ignored due to path filters (2)
tools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/homepage-outline-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-dropdown-light-mode-linux.pngis excluded by!**/*.png
📒 Files selected for processing (3)
src/components/LocationProvider/LocationProvider.spec.tssrc/components/LocationProvider/utils/extractSearchParams.tssrc/components/LocationProvider/utils/resolveFullVersion.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/LocationProvider/LocationProvider.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/LocationProvider/utils/extractSearchParams.ts
When multiple version hashes share the same prefix, return null instead of silently picking the first match. This prevents resolving to the wrong version on short/ambiguous prefixes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/LocationProvider/utils/resolveFullVersion.ts`:
- Around line 14-19: The current logic builds hashMatches from metadata.versions
and metadata.nightly.hash but does not deduplicate identical hashes, so
duplicate identical entries can cause an ambiguous-length branch; update the
resolution in resolveFullVersion.ts to remove duplicate hashes (e.g., via a Set
or unique filter) after building hashMatches and before the length checks so
that if all matching hashes are identical you return that single hash; reference
the variables/values involved (hashMatches, metadata.versions,
metadata.nightly?.hash, shortVersion) and then perform the existing length
checks on the deduplicated array.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 26ae5ff4-34d6-4bb9-9ebb-4eb9b617f70a
⛔ Files ignored due to path filters (1)
tools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-selected-on-light-mode-linux.pngis excluded by!**/*.png
📒 Files selected for processing (2)
src/components/LocationProvider/LocationProvider.spec.tssrc/components/LocationProvider/utils/resolveFullVersion.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/LocationProvider/LocationProvider.spec.ts
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
extractSearchParams) and version resolution logicisSyncingrefisSplitActiveandisNarrow, tab persistence, activateSplit on click@testing-library/react, and polyfills (DOMMatrix, ResizeObserver, matchMedia, localStorage)Closes #373
Test plan
npx vitest run)npm run qa)🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores
Refactor