In toggle mode, tap any one shortcut key (e.g. Fn or Space from Fn+Space) to end dictation — not the full combo#225
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR changes shortcut callbacks to return whether an input was consumed and adds toggle component lifecycle events. Hotkey handling, session control, and test harness wiring are updated to use the new boolean flow and the new consume-only action. ChangesToggle component events and hotkey consumption pipeline
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Sources/AppState.swift (1)
2805-2812: ⚡ Quick winMove
guard let selfbefore the sleep to avoid redundant delay.Currently, if
selfisnilat line 2808, the code uses the fallback value, sleeps for 100ms, and only then returnsnilat line 2813. Moving the guard earlier would exit immediately without the unnecessary delay.♻️ Suggested refactor
contextCaptureTask = Task { [weak self] in + guard let self else { return nil } do { - // Delay context collection to let the OS accessibility tree update after a consecutive paste. - // Fetch the delay from AppState using safe weak reference fallback - let delay = self?.contextCaptureDelayNanoseconds ?? 100_000_000 - try await Task.sleep(nanoseconds: delay) + try await Task.sleep(nanoseconds: self.contextCaptureDelayNanoseconds) } catch { return nil } - guard let self else { return nil } let context = await self.contextService.collectContext()🤖 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 `@Sources/AppState.swift` around lines 2805 - 2812, Move the nil-check for the weak self before sleeping to avoid an unnecessary delay when self is gone: add a guard let self = self else { return nil } at the start of this block, then read self.contextCaptureDelayNanoseconds and call try await Task.sleep(nanoseconds: delay); ensure you reference the same symbol names (contextCaptureDelayNanoseconds and Task.sleep) and remove reliance on the fallback literal before the guard so the method returns immediately when self is nil.Sources/HotkeyManager.swift (1)
62-66: 💤 Low valueConsider short-circuiting the callback loop for efficiency.
The loop invokes
onShortcutEventfor every emitted event without short-circuiting onceshouldConsumeis set totrue. If callbacks have side effects or if consumption can be determined early, consider breaking after the firsttruereturn.However, if you need to invoke the callback for all events for observability or other reasons, this is fine as-is.
⚡ Optional optimization if early-exit is desired
var shouldConsume = false for event in result.emittedEvents { if onShortcutEvent?(event) == true { shouldConsume = true + break } }🤖 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 `@Sources/HotkeyManager.swift` around lines 62 - 66, The loop over result.emittedEvents calls the onShortcutEvent callback for every event even after shouldConsume is set true; modify the loop in HotkeyManager to short-circuit by breaking out once onShortcutEvent(event) returns true and shouldConsume is set, so stop invoking further callbacks and set shouldConsume accordingly (refer to onShortcutEvent, result.emittedEvents, and shouldConsume).
🤖 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.
Nitpick comments:
In `@Sources/AppState.swift`:
- Around line 2805-2812: Move the nil-check for the weak self before sleeping to
avoid an unnecessary delay when self is gone: add a guard let self = self else {
return nil } at the start of this block, then read
self.contextCaptureDelayNanoseconds and call try await Task.sleep(nanoseconds:
delay); ensure you reference the same symbol names
(contextCaptureDelayNanoseconds and Task.sleep) and remove reliance on the
fallback literal before the guard so the method returns immediately when self is
nil.
In `@Sources/HotkeyManager.swift`:
- Around line 62-66: The loop over result.emittedEvents calls the
onShortcutEvent callback for every event even after shouldConsume is set true;
modify the loop in HotkeyManager to short-circuit by breaking out once
onShortcutEvent(event) returns true and shouldConsume is set, so stop invoking
further callbacks and set shouldConsume accordingly (refer to onShortcutEvent,
result.emittedEvents, and shouldConsume).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7890ff29-d1fb-4fe5-92f7-de19c22b3a0a
📒 Files selected for processing (7)
Sources/AppState.swiftSources/HotkeyManager.swiftSources/SetupTestHotkeyHarness.swiftSources/SetupView.swiftSources/ShortcutCore/DictationShortcutSessionController.swiftSources/ShortcutCore/ShortcutMatcher.swiftSources/ShortcutCore/ShortcutModels.swift
…tap to toggle mode Modified files: - Sources/AppState.swift: Implement .consumeOnly to block trailing characters, and refactor handleShortcutEvent into a @discardableResult Bool so consumed key events stop propagating. - Sources/ShortcutCore/DictationShortcutSessionController.swift: Add ignoreToggleUntilComponentDeactivation to consume stop-sequence events. - Sources/ShortcutCore/ShortcutMatcher.swift: Detect toggle component inputs, emit stateless events, and safely unwrap modifiers. - Sources/ShortcutCore/ShortcutModels.swift: Add toggleComponentInputReceived event. - Sources/HotkeyManager.swift: Update onShortcutEvent to loop through multiple emitted events. - Sources/SetupTestHotkeyHarness.swift: Handle .consumeOnly. - Sources/SetupView.swift: Handle .consumeOnly.
6237ef1 to
f985f31
Compare
pianoandglass
changed the title
|
Rebased onto the latest It's intentionally self-contained: scoped to compile standalone against @coderabbitai review |
|
✅ Action performedReview finished.
|
1 participant
Description
In Tap to Toggle mode, the dictation shortcut is a combination of two or more keys (e.g.
Fn+Space, orFn+Control+Space) that starts dictation with a single tap — no holding required. This PR changes how you end that dictation.Before → After
Fn+Spaceshortcut, ending dictation required pressing the wholeFn+Spacecombo again, or clicking the on-screen Stop button.Fn+Spaceshortcut, tapping eitherFnorSpaceon its own ends the dictation and sends it for transcription. This works for any multi-key combination (e.g.Fn+Control+Space→ any one of the three keys ends it). PressingSpaceto stop does not type a stray space into your text field.Changed Files
Sources/AppState.swift.consumeOnlyaction to swallow trailing shortcut keys so they don't leak into the active text field.Sources/ShortcutCore/DictationShortcutSessionController.swiftignoreToggleUntilComponentDeactivationflag to correctly track and consume stop-sequence events. Implemented the.consumeOnlyaction to prevent trailing keys from leaking.Sources/ShortcutCore/ShortcutMatcher.swifttoggleComponentInputReceivedevents. Refactored modifier extraction to use safeif letbinding instead of force unwrapping.Sources/ShortcutCore/ShortcutModels.swifttoggleComponentInputReceivedevent to theShortcutEventenumeration.Sources/HotkeyManager.swiftonShortcutEventhandler to evaluate multiple emitted events per loop and correctly flag when a key press must be consumed by the OS.Sources/SetupTestHotkeyHarness.swift.consumeOnlyaction.Sources/SetupView.swift.consumeOnlyaction in the shortcut action switch statement.Technical Details: Consuming Trailing Keys and Edge Cases
Handling global multi-key shortcuts requires carefully consuming keyboard events so they do not accidentally leak into the active text editor. The logic handles the following edge cases:
Fn+Space), they might releaseFnbeforeSpace. This triggers atoggleComponentInputReceivedevent. The session controller intercepts this stateless event and returns.consumeOnly, which tells macOS to completely swallow the keystroke, preventing a stray space from being typed into the text box.toggleComponentDeactivated). It only resets the ignore state when the user completely lifts their fingers off the keys, preventing accidental double-triggers.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes