test: replace hash scroll source snapshot by Astro-Han · Pull Request #532 · Astro-Han/pawwork

Astro-Han · 2026-05-11T02:21:27Z

Summary

Replaced the brittle source-string assertion for the already-rendered hash-scroll path with a behavior test.
Split useSessionHashScroll into a small router wrapper plus createSessionHashScroll core so the behavior test can run inside the full Bun unit suite without @solidjs/router module-cache leakage.
Kept the user-facing hash-scroll behavior unchanged.

Why

#528 tracks a Windows advisory failure caused by a test reading source text and matching an LF-only implementation snippet. Normalizing CRLF would only hide the platform symptom while keeping the test tied to source formatting.

This PR moves the tested behavior behind a router-free core and verifies the contract directly: an already-rendered hash target scrolls to the message, marks it active, marks the hash target, and does not emit duplicate fallback navigation.

Related Issue

Closes #528.

Human Review Status

Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.

Review Focus

Confirm useSessionHashScroll remains only a router wrapper around createSessionHashScroll.
Confirm the core extraction is a logic move, not a behavior change.
Confirm the new behavior test covers the already-rendered hash target path and duplicate-navigation suppression.

Risk Notes

Low. This is a test-debt PR with one internal extraction. The main risk is accidental behavior drift during the move; that is covered by app full unit tests, app typecheck, the targeted behavior test, and a temporary red check for duplicate navigation.

CodeRabbit suggested changing the expected navigation callback count from one to zero. That was not applied because the single onMessageNavigation call is the target-message intent used by use-session-timeline-interaction to cancel bottom-follow lock. The regression guard is about preventing a second fallback notification after direct already-rendered scrolling.

How To Verify

Focused hash-scroll unit: 7 passed, 0 failed
Temporary red check: adding a duplicate fallback onMessageNavigation call makes the new behavior test fail
App full unit: 1043 passed, 0 failed
App typecheck: passed
Diff check: no whitespace errors
Windows advisory: `unit-windows-app` passed; full advisory failed on unrelated `unit-windows-opencode-session` timeout (`cancel records MessageAbortedError on interrupted process`)

Commands run:

cd packages/app && bun test --preload ./happydom.ts ./src/pages/session/use-session-hash-scroll.test.ts
bun --cwd packages/app test:unit
bun --cwd packages/app typecheck
git diff --check
gh workflow run windows-advisory.yml --ref codex/i528-hash-scroll-test

Screenshots or Recordings

None. No visible UI changes.

Checklist

coderabbitai · 2026-05-11T02:21:40Z

📝 Walkthrough

Walkthrough

This PR refactors session message navigation logic by extracting hash scroll implementation from the useSessionHashScroll hook into a reusable createSessionHashScroll factory, updates the hook to delegate to it, and replaces source-code snapshot tests with behavior-based DOM verification to fix Windows CI flakiness.

Changes

Session Hash Scroll Core Extraction & Hook Refactoring

Layer / File(s)	Summary
Type Contract `packages/app/src/pages/session/use-session-hash-scroll-core.ts`	New `SessionHashScrollInput` type defines the contract for session identity, readiness flags, visible message access, history loading, pending message state, hash targets, auto-scroll controls, scroller/anchor helpers, and optional callbacks for navigation and hash clearing events.
Core Setup & State `packages/app/src/pages/session/use-session-hash-scroll-core.ts`	`createSessionHashScroll` initializes memoized maps for message lookup by id and index, internal mutable state for pending/clearing tracking, and a frame-based queue mechanism to coalesce scroll operations.
Hash & Scroll Operations `packages/app/src/pages/session/use-session-hash-scroll-core.ts`	Implements `clearMessageHash` (removes hash, consumes pending state, fires callback), `updateHash` (syncs URL), `scrollToElement` (offsets for sticky title), `seek` (bounded retry on missing elements), `scrollToMessage` (marks navigation and target), and `applyHash` (resolves hash to message id/element with empty-hash and fallback logic).
Reactive Effects & Lifecycle `packages/app/src/pages/session/use-session-hash-scroll-core.ts`	Three `createEffect` handlers re-apply hash on hash/readiness changes, resolve pending or hash-derived messages, and load history when target is absent; lifecycle sets manual scroll restoration on mount and cancels pending frames on cleanup; returns public API.
Hook Wrapper `packages/app/src/pages/session/use-session-hash-scroll.ts`	`useSessionHashScroll` refactored from 219 lines of inline implementation to 3 lines: imports core module and delegates to `createSessionHashScroll(input, location, navigate)`.
Test Updates `packages/app/src/pages/session/use-session-hash-scroll.test.ts`	Imports now reference core module; replaces source-code `toContain()` snapshot assertion with DOM-based Solid integration test mounting `createSessionHashScroll`, simulating already-rendered message, and verifying single navigation without duplicate fallback.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Astro-Han/pawwork#286: Modifies session hash scroll behavior and related navigation tests.
Astro-Han/pawwork#402: Modifies session hash scroll API and callback contracts.
Astro-Han/pawwork#498: Changes useSessionHashScroll signature and wiring with callback updates.

Suggested labels

ci, P2, app, flaky-test, task

Poem

🐰 A hash in the scroll, so smooth and so clean,
Core logic extracted, reusable and lean,
Tests now behave, no snapshots to break,
Windows and Linux both get the same cake! 🎂

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately reflects the main change: replacing a source snapshot assertion with a behavior test for hash scroll functionality.
Linked Issues check	✅ Passed	Code changes fully meet `#528` objectives: brittle source assertion replaced with behavior test, router-free core helper created, and onMessageNavigation behavior preserved without duplicate calls.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to `#528`: test behavior replacement, core helper extraction, and wrapper preservation. No unrelated refactors, dependency changes, or file modifications outside stated scope.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check	✅ Passed	The pull request provides a comprehensive description with all required template sections completed, including summary, why, related issue, human review status, review focus, risk notes, verification steps, and a completed checklist.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch codex/i528-hash-scroll-test

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

gemini-code-assist

Code Review

This pull request refactors the session hash scrolling logic by extracting the core functionality into a new file, use-session-hash-scroll-core.ts, decoupling it from the router implementation now handled in use-session-hash-scroll.ts. The update includes a new comprehensive test case for hash navigation. I have no feedback to provide as there were no review comments.

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/app/src/pages/session/use-session-hash-scroll-core.ts`:
- Around line 97-100: The handler currently always invokes
input.onMessageNavigation in scrollToMessage even when the target was already
resolved by applyHash; change the logic to only call input.onMessageNavigation
(and input.setActiveMessage) when the message is not already the current one by
checking input.currentMessageId() !== message.id. Apply the same conditional
suppression in the analogous block referenced around lines 133-139 so both
places only fire input.onMessageNavigation when switching to a different
message.

In `@packages/app/src/pages/session/use-session-hash-scroll.test.ts`:
- Around line 124-127: The test currently asserts that navigationCalls contains
["msg_2"], but the desired behavior is that direct hash scrolling for an
already-rendered target must not trigger onMessageNavigation; update the
assertion for navigationCalls (in use-session-hash-scroll.test.ts) to expect no
navigation calls (e.g., expect(navigationCalls).toEqual([]) or
expect(navigationCalls).toHaveLength(0)) so the test fails if fallback
navigation is reintroduced; keep the other assertions (scrollPositions,
activeMessages, markedTargets) unchanged.

🪄 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 Plus

Run ID: b1c74790-cde7-4993-bf39-cc02fc7eee57

📥 Commits

Reviewing files that changed from the base of the PR and between 4d1d21e and 86b8191.

📒 Files selected for processing (3)

packages/app/src/pages/session/use-session-hash-scroll-core.ts
packages/app/src/pages/session/use-session-hash-scroll.test.ts
packages/app/src/pages/session/use-session-hash-scroll.ts

Astro-Han · 2026-05-11T02:26:10Z

CodeRabbit review threads resolved without changes. Both suggestions interpret the desired behavior as zero onMessageNavigation calls for an already-rendered hash target, but this path still intentionally emits one target-message notification so use-session-timeline-interaction can cancel bottom-follow lock and record the navigation intent. The #528 fix is to replace the source-string guard with a behavior test that catches duplicate fallback navigation; the new test expects exactly one call, and a temporary red check confirmed it fails when a second fallback notification is reintroduced.

test: replace hash scroll source snapshot

86b8191

gemini-code-assist Bot reviewed May 11, 2026

View reviewed changes

coderabbitai Bot reviewed May 11, 2026

View reviewed changes

Comment thread packages/app/src/pages/session/use-session-hash-scroll-core.ts

Comment thread packages/app/src/pages/session/use-session-hash-scroll.test.ts

Astro-Han added task Narrow execution, audit, spike, migration, tracking, or upstream follow-up work app Application behavior and product flows P3 Low priority flaky-test Non-deterministic test failure windows Windows-specific labels May 11, 2026

Astro-Han mentioned this pull request May 11, 2026

[Task] Clean up release-gate E2E test debt from v2026.5.11 #529

Closed

Astro-Han merged commit abce058 into dev May 11, 2026
30 of 31 checks passed

Astro-Han deleted the codex/i528-hash-scroll-test branch May 11, 2026 02:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: replace hash scroll source snapshot#532

test: replace hash scroll source snapshot#532
Astro-Han merged 1 commit into
devfrom
codex/i528-hash-scroll-test

Astro-Han commented May 11, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented May 11, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Poem

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Astro-Han commented May 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

Astro-Han commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why

Related Issue

Human Review Status

Review Focus

Risk Notes

How To Verify

Screenshots or Recordings

Checklist

Uh oh!

coderabbitai Bot commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Poem

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Astro-Han commented May 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Astro-Han commented May 11, 2026 •

edited

Loading

coderabbitai Bot commented May 11, 2026 •

edited

Loading