🎨 Palette: Add keyboard shortcuts for main search input - #368
Conversation
Co-authored-by: corebrimtech <175357468+corebrimtech@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughPR adds keyboard-driven search focus (Ctrl/⌘+K and /) with form-input-aware safety checks. Updates search input with ref, hint label, and clear-and-refocus behavior. Refactors filter pipeline and empty-state messaging, and reformats UI components for consistency. ChangesKeyboard-driven search focus and dashboard UI updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/app/page.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/components/ui/input.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4410d2efe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ((e.ctrlKey || e.metaKey) && e.key === "k") { | ||
| e.preventDefault(); | ||
| searchInputRef.current?.focus(); |
There was a problem hiding this comment.
When focus is already inside a text field, this branch still calls preventDefault() for Ctrl/Cmd+K because the active-element guard is only applied to the / shortcut below. On macOS, Ctrl+K is a standard text-editing shortcut to delete to the end of the line, and rich text inputs commonly use Cmd/Ctrl+K for link insertion, so users typing in the search field or another input on the page lose those shortcuts. Apply the same input/textarea/contenteditable guard before swallowing this key combination.
Useful? React with 👍 / 👎.
![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)
src/app/page.tsx (1)
445-451: 💤 Low valueConsider a platform-aware keyboard hint.
The hint currently displays "⌘K" (Mac-specific), which may be unfamiliar to Windows and Linux users who would press "Ctrl+K". Consider detecting the platform and showing "Ctrl+K" on non-Mac systems, or displaying both shortcuts for clarity.
Additionally, the "/" shortcut is not mentioned in the hint, which may reduce discoverability.
💡 Example platform-aware hint
{!searchQuery && ( <div className="absolute right-4 top-1/2 transform -translate-y-1/2 pointer-events-none text-slate-400 opacity-50 flex items-center gap-1"> <kbd className="hidden sm:inline-flex items-center gap-1 rounded border border-slate-200 bg-slate-100 px-1.5 font-mono text-[10px] font-medium text-slate-500 dark:border-slate-700 dark:bg-slate-800 dark:text-slate-400"> {typeof navigator !== 'undefined' && navigator.platform.toLowerCase().includes('mac') ? '⌘K' : 'Ctrl+K'} </kbd> </div> )}Or show both shortcuts:
<kbd ...>⌘K / Ctrl+K</kbd>🤖 Prompt for 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. In `@src/app/page.tsx` around lines 445 - 451, The keyboard hint currently hardcodes "⌘K" in the JSX block that renders when !searchQuery (the kbd inside the conditional), which is Mac-specific and omits the "/" hint; update the rendering logic in that conditional to be platform-aware or show both shortcuts and include "/" for discoverability: detect platform via typeof navigator !== 'undefined' and navigator.platform (or use a helper isMac) to choose between "⌘K" and "Ctrl+K", or render a combined label like "⌘K / Ctrl+K", and append " / " + "/" (or a separate small kbd) to also display the "/" shortcut so non-mac and keyboard-only users see both options; keep the same styling and replace the inner text of the existing kbd element (in the same JSX block) rather than adding a new element.
🤖 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 `@src/app/page.tsx`:
- Line 82: Replace the attribute check on the active element with the DOM
property that reflects the computed editable state: instead of using
document.activeElement?.hasAttribute("contenteditable") use the element's
isContentEditable property (i.e., document.activeElement?.isContentEditable) so
elements with contenteditable="false" are treated correctly; update the
conditional in page.tsx where document.activeElement is tested to negate
isContentEditable (e.g., !document.activeElement?.isContentEditable) and keep
the optional chaining as present.
- Around line 74-77: The Cmd/Ctrl+K key handler currently always calls
e.preventDefault() and focuses searchInputRef.current, which can steal input;
update the handler (the block that checks (e.ctrlKey || e.metaKey) && e.key ===
"k") to first inspect document.activeElement and only call e.preventDefault()
and searchInputRef.current?.focus() when the active element is not an input,
textarea, or contenteditable (same pattern used by the "/" handler); reference
document.activeElement, searchInputRef, and the Ctrl/Cmd+K key-check in your
change.
---
Nitpick comments:
In `@src/app/page.tsx`:
- Around line 445-451: The keyboard hint currently hardcodes "⌘K" in the JSX
block that renders when !searchQuery (the kbd inside the conditional), which is
Mac-specific and omits the "/" hint; update the rendering logic in that
conditional to be platform-aware or show both shortcuts and include "/" for
discoverability: detect platform via typeof navigator !== 'undefined' and
navigator.platform (or use a helper isMac) to choose between "⌘K" and "Ctrl+K",
or render a combined label like "⌘K / Ctrl+K", and append " / " + "/" (or a
separate small kbd) to also display the "/" shortcut so non-mac and
keyboard-only users see both options; keep the same styling and replace the
inner text of the existing kbd element (in the same JSX block) rather than
adding a new element.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9e3d145-61ff-408c-94ab-5c7251909c02
📒 Files selected for processing (3)
.Jules/palette.mdsrc/app/page.tsxsrc/components/ui/input.tsx
| if ((e.ctrlKey || e.metaKey) && e.key === "k") { | ||
| e.preventDefault(); | ||
| searchInputRef.current?.focus(); | ||
| } |
There was a problem hiding this comment.
Critical: Cmd/Ctrl+K doesn't check activeElement before preventing default.
The Cmd/Ctrl+K handler always prevents default and focuses the search input, even when the user is typing in another input field, textarea, or contenteditable element. This violates the documented pattern in .Jules/palette.md (lines 27-29) and can steal keystrokes from modal dialogs, forms, and other interactive elements.
The "/" handler (lines 78-86) correctly checks document.activeElement before preventing default—apply the same pattern to Cmd/Ctrl+K.
🛡️ Proposed fix to add activeElement check
const handleKeyDown = (e: KeyboardEvent) => {
- if ((e.ctrlKey || e.metaKey) && e.key === "k") {
+ if (
+ (e.ctrlKey || e.metaKey) &&
+ e.key === "k" &&
+ document.activeElement?.tagName !== "INPUT" &&
+ document.activeElement?.tagName !== "TEXTAREA" &&
+ !document.activeElement?.isContentEditable
+ ) {
e.preventDefault();
searchInputRef.current?.focus();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ((e.ctrlKey || e.metaKey) && e.key === "k") { | |
| e.preventDefault(); | |
| searchInputRef.current?.focus(); | |
| } | |
| if ( | |
| (e.ctrlKey || e.metaKey) && | |
| e.key === "k" && | |
| document.activeElement?.tagName !== "INPUT" && | |
| document.activeElement?.tagName !== "TEXTAREA" && | |
| !document.activeElement?.isContentEditable | |
| ) { | |
| e.preventDefault(); | |
| searchInputRef.current?.focus(); | |
| } |
🤖 Prompt for 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.
In `@src/app/page.tsx` around lines 74 - 77, The Cmd/Ctrl+K key handler currently
always calls e.preventDefault() and focuses searchInputRef.current, which can
steal input; update the handler (the block that checks (e.ctrlKey || e.metaKey)
&& e.key === "k") to first inspect document.activeElement and only call
e.preventDefault() and searchInputRef.current?.focus() when the active element
is not an input, textarea, or contenteditable (same pattern used by the "/"
handler); reference document.activeElement, searchInputRef, and the Ctrl/Cmd+K
key-check in your change.
Source: Coding guidelines
| e.key === "/" && | ||
| document.activeElement?.tagName !== "INPUT" && | ||
| document.activeElement?.tagName !== "TEXTAREA" && | ||
| !document.activeElement?.hasAttribute("contenteditable") |
There was a problem hiding this comment.
Use isContentEditable instead of hasAttribute("contenteditable").
hasAttribute("contenteditable") checks only for the attribute's existence, not its value. Elements with contenteditable="false" would incorrectly pass this check and allow the shortcut to steal focus.
🔧 Proposed fix
if (
e.key === "/" &&
document.activeElement?.tagName !== "INPUT" &&
document.activeElement?.tagName !== "TEXTAREA" &&
- !document.activeElement?.hasAttribute("contenteditable")
+ !document.activeElement?.isContentEditable
) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| !document.activeElement?.hasAttribute("contenteditable") | |
| if ( | |
| e.key === "/" && | |
| document.activeElement?.tagName !== "INPUT" && | |
| document.activeElement?.tagName !== "TEXTAREA" && | |
| !document.activeElement?.isContentEditable | |
| ) { |
🤖 Prompt for 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.
In `@src/app/page.tsx` at line 82, Replace the attribute check on the active
element with the DOM property that reflects the computed editable state: instead
of using document.activeElement?.hasAttribute("contenteditable") use the
element's isContentEditable property (i.e.,
document.activeElement?.isContentEditable) so elements with
contenteditable="false" are treated correctly; update the conditional in
page.tsx where document.activeElement is tested to negate isContentEditable
(e.g., !document.activeElement?.isContentEditable) and keep the optional
chaining as present.
1 participant
💡 What: Added a global keyboard shortcut (
Cmd+K/Ctrl+Kand/) to instantly focus the main search input on the dashboard. Also added a visual<kbd>⌘K</kbd>hint inside the input when it is empty.🎯 Why: Power users frequently rely on keyboard shortcuts to navigate data-heavy applications. Adding a quick way to focus the search bar without reaching for the mouse significantly speeds up the workflow. The visual hint improves discoverability, and clicking the clear (
X) button now intelligently refocuses the input so the user can immediately type a new query.📸 Before/After:
⌘Kon the right side. Pressing/orCmd/Ctrl+Kinstantly focuses the input, allowing immediate typing.♿ Accessibility:
document.activeElement. This ensures that if the user is already typing in an input or textarea, the shortcut is gracefully bypassed, preventing the accidental swallowing of standard text keystrokes (like typing a slash in a different form field).<kbd>element utilizes semantic HTML for screen readers and high-contrast visual queues.Testing:
PR created automatically by Jules for task 4959276174044128335 started by @corebrimtech
Summary by CodeRabbit
Release Notes
New Features
Style