Skip to content

fix: prevent Cmd+K from opening duplicate search dialogs#158

Merged
rsbh merged 1 commit into
mainfrom
fix/duplicate-search-dialog
Jul 9, 2026
Merged

fix: prevent Cmd+K from opening duplicate search dialogs#158
rsbh merged 1 commit into
mainfrom
fix/duplicate-search-dialog

Conversation

@rsbh

@rsbh rsbh commented Jul 9, 2026

Copy link
Copy Markdown
Member

Problem

Pressing Cmd/Ctrl+K opened two stacked search dialogs in the default theme.

The Search component bundled the trigger button, the Command.Dialog, and the Cmd+K keydown listener in one component. The default theme mounts <Search /> twice (mobile header + sidebar header), so each instance registered its own listener and opened its own dialog.

Fix

Split search.tsx into three parts:

  • SearchProvider — small context holding the shared open state
  • Search — trigger-only icon button, opens the dialog via context
  • SearchDialog — the single Command.Dialog with the single Cmd+K listener and query logic

SearchProvider + SearchDialog are mounted once in App.tsx (gated by config.search?.enabled), so themes only render trigger buttons wherever they want. The paper theme's hidden-trigger workaround (display: none trigger just to host the dialog) is removed.

Verification

Headless Chrome against the dev server, both themes:

  • Default theme (basic example): Cmd+K opens exactly 1 dialog (was 2), typing filters, Cmd+K toggles closed, both trigger buttons open the same dialog, no console errors
  • Paper theme (versioned example): Cmd+K opens 1 dialog, toggle works
  • Typecheck and biome lint clean

🤖 Generated with Claude Code

The Search component bundled trigger, dialog, and the Cmd+K keydown
listener together. The default theme mounts it twice (mobile header and
sidebar header), so each keypress opened two stacked dialogs.

Split it into SearchProvider (shared open state), Search (trigger-only
button), and SearchDialog (single dialog + single keydown listener).
The provider and dialog are mounted once in App, so themes only render
trigger buttons. Paper theme's hidden-trigger workaround is removed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@vercel

vercel Bot commented Jul 9, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
chronicle Ready Ready Preview, Comment Jul 9, 2026 7:51am

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0a01f607-564a-40b3-8a94-78967ec59d4f

📥 Commits

Reviewing files that changed from the base of the PR and between f1c3a05 and 5aeb27e.

📒 Files selected for processing (4)
  • packages/chronicle/src/components/ui/search.tsx
  • packages/chronicle/src/server/App.tsx
  • packages/chronicle/src/themes/paper/Layout.module.css
  • packages/chronicle/src/themes/paper/Layout.tsx
💤 Files with no reviewable changes (2)
  • packages/chronicle/src/themes/paper/Layout.tsx
  • packages/chronicle/src/themes/paper/Layout.module.css

📝 Walkthrough

Summary by CodeRabbit

  • New Features
    • Search is now available through a shared app-level state, keeping the search dialog consistent across the site.
  • Bug Fixes
    • Improved full-width page layout spacing so content no longer shifts unexpectedly in the paper theme.
  • Refactor
    • Simplified the search interface structure and moved the search trigger out of the page layout for cleaner behavior.

Walkthrough

Search dialog state is refactored from local component state into a shared React context via a new SearchProvider and useSearch hook. App.tsx wraps content in SearchProvider and renders SearchDialog conditionally. Paper theme Layout.tsx removes its local Search trigger, and .contentFull CSS is adjusted.

Changes

Search UI context refactor

Layer / File(s) Summary
SearchContext, SearchProvider, useSearch
packages/chronicle/src/components/ui/search.tsx
Adds SearchContext/SearchProvider/useSearch to centralize open/setOpen state; Search toggles the dialog via context instead of local state; SearchDialog reads open/setOpen from context and renders input, loading/empty states, suggestions, and results accordingly.
App.tsx provider wiring
packages/chronicle/src/server/App.tsx
Imports SearchDialog and SearchProvider, wraps RootHead and routed content in SearchProvider, and conditionally renders SearchDialog when config.search?.enabled is true.
Paper theme layout cleanup and CSS fix
packages/chronicle/src/themes/paper/Layout.tsx, packages/chronicle/src/themes/paper/Layout.module.css
Removes the local Search import and its conditional trigger render from the layout, and sets .contentFull margin-left to 0.

Estimated code review effort: 2 (Simple) | ~12 minutes

Possibly related PRs

  • raystack/chronicle#8: Builds directly on the earlier search feature that this PR refactors into a context-based implementation.
  • raystack/chronicle#45: Both PRs touch search.tsx and the paper theme's mounting/integration of the Search component.
  • raystack/chronicle#92: Both PRs modify the search dialog's open state handling in the same file.

Suggested reviewers: rohilsurana, rohanchkrabrty, paanSinghCoder

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: preventing duplicate search dialogs from Cmd+K in the default theme.
Description check ✅ Passed The description accurately explains the duplicate-dialog bug, the context-based fix, and the theme/layout changes.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ 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 fix/duplicate-search-dialog

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.

@rsbh rsbh merged commit 0138884 into main Jul 9, 2026
7 of 9 checks passed
@rsbh rsbh deleted the fix/duplicate-search-dialog branch July 9, 2026 08:50
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.

2 participants