Skip to content

fix(search): stabilize search modal height to stop jitter (#54) - #61

Merged
DavidNic11 merged 3 commits into
mainfrom
fix/search-modal-jitter-54
Sep 29, 2026
Merged

DavidNic11 merged 3 commits into
mainfrom
fix/search-modal-jitter-54

Conversation

@k5s-bot

@k5s-bot k5s-bot Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #54 — "Search is super jittery".

The search (⌘K) modal was sized entirely by its content and centered in the viewport. Because the results list grows and shrinks with the number of matches, every keypress that changed the result count resized the panel and re-centered it vertically, producing the jittery experience described in the issue.

Fix

Give the results list a fixed height (min(60vh, 420px)) instead of letting it flex to content:

  • Short result sets (or "no results") keep a stable minimum height so the modal no longer twitches.
  • Long result sets scroll inside that defined height rather than growing the modal.
  • The panel's size and position now stay constant while typing.
  • A subtle fade-in smooths the one-time transition when results first appear.

The production change is limited to CSS in src/lib/components/search/SearchModal.svelte; behavior and markup are unchanged. The empty (pre-typing) state stays compact as before. A Playwright regression test in e2e/search.spec.ts locks the fix in (see Verification).

Before / After

Before, the panel's top edge and height jumped on each keypress as the result count changed (e.g. top at ~y122 for many results vs ~y325 for one). After, all states — many results, one result, and no results — occupy the exact same box, with long lists scrolling internally.

Verification

  • Reproduced the jitter and verified the fix in the running app (localStorage backend) with Playwright screenshots across empty / many-results / one-result / no-results states.
  • Added an automated regression guard (e2e/search.spec.ts → "search modal keeps a stable size and position across result counts (Search is super jittery #54)"): it opens the ⌘K modal and asserts .search-panel's top (y) and height are identical within 1px across many-results, one-result, and no-results queries — so reverting the fixed .results-container height back to a content-driven size fails CI. Passes locally (8/8 search specs, local project); CI exercises it in both local and api.
  • pnpm check (svelte-check): 0 errors, 0 warnings, after syncing the branch with main.

Maintainers: apply the ai-review label to this PR to request an automated code review, or the ai-triage label to have review feedback addressed and the branch brought back in sync with its base. (The automation cannot apply either label to its own PR, so a human needs to add it.)

The search modal was sized purely by its content and centered in the
viewport, so every keypress that changed the number of results resized
the panel and re-centered it — producing a "super jittery" experience.

Give the results list a fixed height (min(60vh, 420px)) so the panel
stays the same size and position regardless of how many results match:
short result sets keep a stable minimum height and long ones scroll
inside the defined height instead of growing the modal. A subtle
fade-in smooths the transition when results first appear.

Fixes #54

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DaPMmpPcYsu1VXEB3FZBJh
@k5s-bot k5s-bot Bot mentioned this pull request Sep 27, 2026
@k5s-bot

k5s-bot Bot commented Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Starting work on this now.

@k5s-bot

k5s-bot Bot commented Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Watch live or take over the session here: https://claude.ai/code/session_01GN7ANwuCs5KANkSgf9rCjG

@k5s-bot k5s-bot Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct diagnosis and a minimal, well-scoped fix. Giving .results-container a fixed height: min(60vh, 420px) removes the content-driven resize/re-center that produced the per-keypress jitter in #54, and the change is genuinely isolated to this component — the full-page /search route (src/routes/search/+page.svelte) uses a separate .results-area class, so it's unaffected, and the pre-typing empty state still uses .search-hint and stays compact. The flex interaction with the panel's max-height: 70vh is fine (results-container can still shrink via default flex-shrink on very short viewports). CI is green. Approving — one non-blocking note inline about the lack of a regression guard.

Comment thread src/lib/components/search/SearchModal.svelte
@k5s-bot

k5s-bot Bot commented Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

Review posted successfully.

Summary

I reviewed PR #61 ("fix(search): stabilize search modal height to stop jitter") on imaustink/glyph.

Verdict: Approve — no code changes made or pushed (review only).

What the PR does: A 10-line, CSS-only change to src/lib/components/search/SearchModal.svelte that replaces flex: 1 on .results-container with a fixed height: min(60vh, 420px) plus a one-time fade-in.

Assessment:

  • Correctness — Sound. The diagnosis is right: the content-sized, viewport-centered panel re-sized and re-centered on every keypress as result count changed. A fixed height (not min-height) is the correct choice to fully stop the jitter for all result counts; long lists scroll internally. Verified the flex interaction with the panel's max-height: 70vh is safe.
  • Scope/isolation — Clean. Svelte-scoped styles; the full-page /search route uses a separate .results-area class and is unaffected. The pre-typing empty state (.search-hint) stays compact. Markup/behavior unchanged.
  • CI — All checks green (frontend, e2e-local, e2e-api, api, collab, helm, builds).
  • Tests — No automated regression guard was added. I left one inline, non-blocking comment at SearchModal.svelte:206 with a copy-pasteable agent prompt to add a Playwright test asserting .search-panel's bounding box is stable across many/one/no-results states.

I considered but deliberately skipped flagging the missing prefers-reduced-motion guard on the …


✅ Opened pull request: imaustink/glyph#61

@k5s-bot k5s-bot Bot removed the ai-review label Sep 28, 2026
@k5s-bot

k5s-bot Bot commented Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Starting work on this now.

@k5s-bot

k5s-bot Bot commented Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Watch live or take over the session here: https://claude.ai/code/session_01LWwpdWQ6ovTfq4m7f7f9dG

Asserts the search panel's top (y) and height stay identical across
many-results, one-result, and no-results states, so a future revert of
the fixed results-container height back to a content-driven size would
fail CI.
@k5s-bot

k5s-bot Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Triage follow-up

Picked this PR back up under ai-triage. Here's what I changed and why.

Addressed feedback

  • @k5s-bot's inline note at SearchModal.svelte:206 ("ships without an automated regression guard"): added a Playwright test to e2e/search.spec.ts — "search modal keeps a stable size and position across result counts (Search is super jittery #54)". It seeds several pages sharing a token, opens the ⌘K modal via the existing openSearchModal helper, then captures .search-panel's bounding box for a many-results, one-result, and no-results query and asserts the panel's top (y) and height are identical within 1px across all three. Reverting .results-container back to a content-driven size (e.g. flex: 1) changes those boxes and fails the test — exactly the regression the note called out. Verified green locally (8/8 search specs, local project); CI runs it in both local and api. The review comment is resolved.

Branch sync

  • Merged origin/main into the branch (was 1 ahead / 3 behind). It merged cleanly with no conflicts — picked merge over rebase because the branch is already pushed and rebasing would need a force-push. pnpm check (svelte-check) is still 0 errors / 0 warnings post-merge.

PR description

  • Updated so it no longer claims the change is "limited to CSS" (it now also adds the e2e guard) and the Verification section documents the new test and the sync.

Deliberately not done

  • prefers-reduced-motion guard on the 140ms results fade-in. The review explicitly considered this and chose not to flag it as non-blocking, so I left it out to keep the change in scope. Easy to add later if you'd like the fade suppressed for reduced-motion users.

CI was still running the checks on the merge/test commit when I wrapped up — the pre-existing checks were all green on the prior head, and everything I could verify locally passes.

@k5s-bot

k5s-bot Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Done. PR #61 is ready to merge — all review feedback addressed, branch synced, and 0 unresolved threads.

Summary

PR: #61 (state: OPEN, MERGEABLE, head d0cef22)

The PR already had two approvals (k5s-bot + imaustink) and all checks green. The only outstanding feedback was one non-blocking inline review comment asking for an automated regression guard. Here's what I did:

  1. Addressed the review comment (SearchModal.svelte:206 — "ships without an automated regression guard"). Added a Playwright test to e2e/search.spec.ts that opens the ⌘K modal and asserts .search-panel's top (y) and height are identical within 1px across many-results, one-result, and no-results queries. A revert of the fixed .results-container height back to flex: 1 would move those boxes and fail the test. Verified locally: 8/8 search specs pass (local project). Replied to the comment and resolved the thread.

  2. Synced with base. Branch was 1 ahead / 3 behind main. Merged origin/main in (chose merge over rebase since the branch is pushed and rebase would need a forbidden force-push) — clean, no conflicts. pnpm check (svelte-check) remains 0 errors / 0 warnings post-merge.

  3. Updated the PR description so it no longer claims the change is "limited to CSS" and documents the new regression test in Verification.

  4. Posted a summary comment on the PR.

Deliberately not done: I left the 140…


✅ Opened pull request: imaustink/glyph#61

@k5s-bot k5s-bot Bot removed the ai-triage label Sep 29, 2026
@DavidNic11
DavidNic11 merged commit 731d18d into main Sep 29, 2026
9 checks passed
@DavidNic11
DavidNic11 deleted the fix/search-modal-jitter-54 branch September 29, 2026 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Search is super jittery

3 participants