fix: repair quick capture search and keyboard ux#144
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThe changes introduce live-search functionality by adding Changes
Sequence DiagramsequenceDiagram
actor User
participant QCInputField as QuickCaptureInputField
participant QCViewModel as QuickCaptureViewModel
participant DB as Database
User->>QCInputField: Edits text
QCInputField->>QCInputField: controlTextDidChange
QCInputField->>QCViewModel: onTextChange callback
QCViewModel->>QCViewModel: handleInputChange(text)
QCViewModel->>QCViewModel: Update inputText
alt In Search Mode
QCViewModel->>QCViewModel: submitSearch()
QCViewModel->>DB: Query with inputText
DB-->>QCViewModel: Return results
QCViewModel->>QCViewModel: Update results & selectedResultID
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
|
@codex review |
|
@greptileai review |
✅ Actions performedReview triggered.
|
|
The PR already exists and has been reviewed by other bots (Macroscope and CodeRabbit). Since I can't update the PR body (it's not agent-managed), let me create a final comprehensive summary of my review findings. BugBot Review Complete ✅I've completed a thorough bug review of PR #144 "fix: repair quick capture search and keyboard ux". Here's what I found: Overall Assessment: APPROVED ✅The PR is safe to merge. All changes are well-implemented with good test coverage. Key Findings:✅ What Works Well:
Minor Polish Items (Optional):
Documents Created:
Recommendation:Merge now - Core functionality is solid and well-tested. Optional follow-up work can address performance optimization if needed.   |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Co-authored-by: Etan Heyman <EtanHey@users.noreply.github.com>
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@brain-bar/Sources/BrainBar/QuickCapturePanel.swift`:
- Around line 338-342: Remove the redundant direct binding write in
controlTextDidChange: instead of setting parent.text there, let
parent.onTextChange(_) drive updates so handleInputChange remains the single
path that updates inputText; keep the existing guard in handleInputChange to
avoid re-entrancy and ensure controlTextDidChange only calls
parent.onTextChange(textField.stringValue) (referencing controlTextDidChange,
parent.text, parent.onTextChange, handleInputChange, and inputText).
- Around line 150-157: The handler currently calls submitSearch() on every
keystroke (in handleInputChange), causing synchronous DB queries; add a debounce
by storing a cancellable Task (e.g., a property like searchTask) and when
handleInputChange(_:) runs cancel any existing task, then create a new Task that
awaits a short sleep (150–300ms) and then calls submitSearch() only if mode ==
.search and inputText still matches newValue; ensure you update inputText
immediately as you do now, cancel the previous Task before creating a new one,
and clean up/cancel searchTask when the view deinitializes or mode changes.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: bb6abb08-78de-4416-ae05-46cf242e23b7
📒 Files selected for processing (2)
brain-bar/Sources/BrainBar/QuickCapturePanel.swiftbrain-bar/Tests/BrainBarTests/QuickCapturePanelTests.swift
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-03-18T00:12:08.774Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 87
File: brain-bar/Sources/BrainBar/BrainBarServer.swift:118-129
Timestamp: 2026-03-18T00:12:08.774Z
Learning: In Swift files under brain-bar/Sources/BrainBar, enforce that when a critical dependency like the database is nil due to startup ordering (socket before DB), any tool handler that accesses the database must throw an explicit error (e.g., ToolError.noDatabase) instead of returning a default/empty value. Do not allow silent defaults (e.g., guard let db else { return ... }). Flag patterns that silently return defaults when db is nil, as this masks startup timing issues. This guidance applies broadly to similar Swift files in the BrainBar module, not just this one location.
Applied to files:
brain-bar/Sources/BrainBar/QuickCapturePanel.swift
📚 Learning: 2026-03-29T18:45:40.988Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 133
File: brain-bar/Sources/BrainBar/BrainDatabase.swift:0-0
Timestamp: 2026-03-29T18:45:40.988Z
Learning: In the BrainBar module’s Swift database layer (notably BrainDatabase.swift), ensure that the `search()` function’s `unreadOnly=true` path orders results by the delivery frontier cursor so the watermark `maxRowID` stays contiguous. Specifically, when `unreadOnly` is enabled, the query must include `ORDER BY c.rowid ASC` (e.g., via `let orderByClause = unreadOnly ? "c.rowid ASC" : "f.rank"`). Do not replace the unread-only ordering with relevance-based sorting (e.g., `f.rank`) unconditionally or for the unread-only path, as it can introduce gaps in the watermark and incorrectly mark unseen rows as delivered. Flag any future change to the `ORDER BY` clause in this function that makes relevance sorting apply to the unread-only case.
Applied to files:
brain-bar/Sources/BrainBar/QuickCapturePanel.swift
🔇 Additional comments (8)
brain-bar/Sources/BrainBar/QuickCapturePanel.swift (6)
273-283: LGTM!The reordering correctly captures the row data before switching modes, preventing any state interference from the mode change clearing results. This ensures
inputTextis populated with the selected result's title.
408-413: LGTM!The borderless style mask is appropriate for this floating utility panel and aligns with the PR objective to remove extra panel chrome.
539-550: LGTM!Adding
.focusable(false)to mode buttons and their container correctly excludes them from the keyboard focus loop, ensuring Tab remains dedicated to mode toggling as intended by the PR.
687-690: LGTM!The
.focusable(false)on the button style ensures keyboard navigation stays on the text field.
742-748: LGTM!Marking result rows and the scroll view as non-focusable keeps keyboard interaction centered on the input field while allowing mouse interactions on results.
573-578: LGTM!Padding adjustments align with the PR objective to refine the panel layout.
brain-bar/Tests/BrainBarTests/QuickCapturePanelTests.swift (2)
70-92: LGTM!This test comprehensively verifies the live search behavior: text update, results population, auto-selection of first result, and idle feedback state. Good coverage of the new
handleInputChangefunctionality.
196-210: LGTM!This test validates the capture-mode return behavior: successful storage, confirmation flash trigger, input clearing, and database persistence. Good end-to-end coverage of the
handleInputReturnpath.
| func handleInputChange(_ newValue: String) { | ||
| if inputText != newValue { | ||
| inputText = newValue | ||
| } | ||
|
|
||
| guard mode == .search else { return } | ||
| submitSearch() | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider debouncing search queries to avoid excessive database calls.
Calling submitSearch() on every keystroke can cause performance issues during rapid typing, especially with larger databases. Each call performs a synchronous database query on the main thread.
Consider adding a small debounce (e.g., 150-300ms) using Task with cancellation:
♻️ Example debounce approach
+ private var searchTask: Task<Void, Never>?
+
func handleInputChange(_ newValue: String) {
- if inputText != newValue {
- inputText = newValue
- }
-
+ inputText = newValue
guard mode == .search else { return }
- submitSearch()
+ searchTask?.cancel()
+ searchTask = Task {
+ try? await Task.sleep(nanoseconds: 150_000_000)
+ guard !Task.isCancelled else { return }
+ submitSearch()
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@brain-bar/Sources/BrainBar/QuickCapturePanel.swift` around lines 150 - 157,
The handler currently calls submitSearch() on every keystroke (in
handleInputChange), causing synchronous DB queries; add a debounce by storing a
cancellable Task (e.g., a property like searchTask) and when
handleInputChange(_:) runs cancel any existing task, then create a new Task that
awaits a short sleep (150–300ms) and then calls submitSearch() only if mode ==
.search and inputText still matches newValue; ensure you update inputText
immediately as you do now, cancel the previous Task before creating a new one,
and clean up/cancel searchTask when the view deinitializes or mode changes.
| func controlTextDidChange(_ notification: Notification) { | ||
| guard let textField = notification.object as? NSTextField else { return } | ||
| parent.text = textField.stringValue | ||
| parent.onTextChange(textField.stringValue) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Dual text update is redundant but functional.
Line 340 updates parent.text (the binding), and line 341 calls onTextChange which calls handleInputChange, which also updates inputText. The guard in handleInputChange prevents a double-write, but the design could be simplified by removing line 340 and letting handleInputChange be the sole path for updating inputText.
♻️ Optional simplification
func controlTextDidChange(_ notification: Notification) {
guard let textField = notification.object as? NSTextField else { return }
- parent.text = textField.stringValue
parent.onTextChange(textField.stringValue)
}📝 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.
| func controlTextDidChange(_ notification: Notification) { | |
| guard let textField = notification.object as? NSTextField else { return } | |
| parent.text = textField.stringValue | |
| parent.onTextChange(textField.stringValue) | |
| } | |
| func controlTextDidChange(_ notification: Notification) { | |
| guard let textField = notification.object as? NSTextField else { return } | |
| parent.onTextChange(textField.stringValue) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@brain-bar/Sources/BrainBar/QuickCapturePanel.swift` around lines 338 - 342,
Remove the redundant direct binding write in controlTextDidChange: instead of
setting parent.text there, let parent.onTextChange(_) drive updates so
handleInputChange remains the single path that updates inputText; keep the
existing guard in handleInputChange to avoid re-entrancy and ensure
controlTextDidChange only calls parent.onTextChange(textField.stringValue)
(referencing controlTextDidChange, parent.text, parent.onTextChange,
handleInputChange, and inputText).
Co-authored-by: Etan Heyman <EtanHey@users.noreply.github.com>
2 participants
Summary
Test plan
swift test --package-path brain-barswift test --package-path brain-bar --filter QuickCapturePanelTestspytestcurrently fails outside this Swift change set intests/test_enrichment_controller.pyNote
Fix quick capture search live updates and keyboard focus traversal
QuickCaptureViewModel.handleInputChangeto trigger search immediately on every keystroke while in search mode, replacing deferred submission.applySelectedSearchResultto switch to capture mode before populating the input field, ensuring prior search state is cleared correctly.focusable(false), keeping keyboard shortcuts intact.QuickCapturePanelController.onTextChangecallback toQuickCaptureInputFieldso text changes propagate to the view model via thecontrolTextDidChangedelegate.Macroscope summarized 2cac340.
Summary by CodeRabbit
New Features
Style
Tests