Skip to content

fix(settings): unblock recorder keys and refresh update availability - #34

Merged
scttbnsn merged 2 commits into
dev/m2from
fix/keyboard-and-update-menu
Oct 31, 2026
Merged

scttbnsn merged 2 commits into
dev/m2from
fix/keyboard-and-update-menu

Conversation

@scttbnsn

@scttbnsn scttbnsn commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

The shortcut recorder swallowed Tab, Return and Space while recording, trapping keyboard users in the capture button. Those keys now leave recording and reach the sheet controls, including Shift-Tab. Modified combinations remain available for shortcuts.

Sparkle availability changes now notify the model so an open menu updates Check for Updates without waiting for a timer or reopening. Settings also uses the current app activation API.

Validation: both regression cases failed before the fixes; 21 targeted tests and all 380 Swift tests pass. Full scripts/verify.sh passes, including release packaging, 19 website tests, strict formatting and plist checks. Local Qlty reports no issues.

Summary by CodeRabbit

  • New Features
    • The update menu now refreshes when update availability changes, keeping its status current.
    • While recording a shortcut, unmodified Tab, Return, and Space now stop recording and pass through to the settings window.
  • Bug Fixes
    • Update availability changes now trigger the app model to refresh its displayed state.

@scttbnsn

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 51 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 80fd0a97-46b8-44cf-821d-2ce08792f6ff
Commits

Reviewing files that changed from the base of the PR and between f6b0de6 and 00caae0.

Files selected for processing: 3
  • Sources/DilatedUI/Settings/ShortcutsSettings.swift
  • Sources/DilatedUI/Updates/AppUpdater.swift
  • Tests/DilatedUITests/AppUpdaterTests.swift
Walkthrough

The changes forward updater availability notifications to the model, allow selected keys to pass through during shortcut recording, and change how the settings window activates the app.

Changes: Updater Menu Refresh

Layer / File(s) Summary
Forward updater availability changes
Sources/DilatedUI/Updates/AppUpdater.swift, Sources/DilatedUI/AppModel.swift, Tests/DilatedUITests/MenuTests.swift
AppUpdater calls its change callback when canCheckForUpdates changes. AppModel forwards the notification to onChange, and the menu test uses that callback to refresh the existing menu.

Shortcut Recording

Layer / File(s) Summary
Pass through selected recorder keys
Sources/DilatedUI/Settings/ShortcutsSettings.swift, Tests/DilatedUITests/ShortcutPreferencesTests.swift
Tab, Return, and Space pass through during recording when Command, Control, and Option are absent. The tests cover these keys with modifier combinations and verify that an unmodified A does not pass through.

Settings Window Activation

Layer / File(s) Summary
Use default app activation
Sources/DilatedUI/Settings/SettingsWindow.swift
SettingsWindow.present() calls NSApp.activate() without explicitly setting ignoringOtherApps.

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

Change: Bug fix

Possibly related PRs

Suggested reviewers: biggest-littlest

Merge Risk: Minimal · previous review f6b0d

The reviewed changes present no established merge-blocking risk beyond normal validation.

Pre-merge checks: Passed 4 · Failed 1

Failed 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%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title names two changes that both appear in the changeset: recorder keys (Tab, Return, Space) now pass through while recording in ShortcutsSettings.swift, and update availability refreshes the men…
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.
Full details: Title check

Explanation

The title names two changes that both appear in the changeset: recorder keys (Tab, Return, Space) now pass through while recording in ShortcutsSettings.swift, and update availability refreshes the menu via the new AppUpdater onChange callback and AppModel forwarding. The title also uses the conventional fix(settings) prefix. It covers both main changes and omits the smaller Settings activation API change, which is expected.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 6 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
Generate docstrings
  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@coderabbitai

coderabbitai Bot commented Oct 31, 2026 •

Copy link
Copy Markdown
Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Additional comments: 2
Sources/DilatedUI/Settings/ShortcutsSettings.swift: 1

15-15: Trivial · Maintainability & Code Quality · Low value

Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Use isDisjoint(with:) for the modifier check.

SwiftLint reports intersection(...).isEmpty on this line. Replace it with the equivalent isDisjoint(with:) check.

Proposed fix
-      && modifiers.intersection([.command, .control, .option]).isEmpty
+      && modifiers.isDisjoint(with: [.command, .control, .option])

Source: Linters/SAST tools

Sources/DilatedUI/Updates/AppUpdater.swift-118-118: 1

118-118: Minor · Functional Correctness · Quick win

Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Preserve the pre-start canCheckForUpdates state.

With an available idle driver, canCheckForUpdates is true before startup because !started is true, even when driverCanCheck is false. startDriver() sets started = true before it calls refreshDriver(). Line [118] therefore records the already-false post-start value as previous, so the true-to-false transition does not call onChange. An existing availability consumer can keep an enabled update item, and checkForUpdates() then exits at its canCheckForUpdates guard. Capture the value before setting started, or pass that pre-start value into refreshDriver().


Review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: edd80b09-e5a0-479c-a230-dea23203fa99
Commits

Reviewing files that changed from the base of the PR and between 528e4e0 and f6b0de6.

Files selected for processing: 6
  • Sources/DilatedUI/AppModel.swift
  • Sources/DilatedUI/Settings/SettingsWindow.swift
  • Sources/DilatedUI/Settings/ShortcutsSettings.swift
  • Sources/DilatedUI/Updates/AppUpdater.swift
  • Tests/DilatedUITests/MenuTests.swift
  • Tests/DilatedUITests/ShortcutPreferencesTests.swift

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

@scttbnsn

Copy link
Copy Markdown
Contributor Author

Fixed both comments in 00caae0. startDriver now captures availability before startup changes it, and the regression test checks that an existing consumer gets the true-to-false notification. That test failed before the fix; all 22 targeted tests pass. The modifier check now uses isDisjoint(with:).

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 31, 2026 •

Copy link
Copy Markdown
Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@biggest-littlest biggest-littlest left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The startup regression reproduces the missed notification and passes with the fix. The modifier cleanup preserves the tested shortcut behavior.

@scttbnsn
scttbnsn merged commit b9b73ab into dev/m2 Oct 31, 2026
3 of 4 checks passed
scttbnsn added a commit that referenced this pull request Oct 31, 2026
* test(drive-alive): wait on events instead of wall-clock deadlines (#32)

* test(drive-alive): wait on events instead of wall-clock deadlines

The suite is MainActor, so a test sleeping toward a 5 s deadline could wake after a busy main thread and fail before the controller's queued cycle ran. The waits now await the pulse, schedule and write events directly, with a one-minute time limit.

* test(drive-alive): let a waiting signal end on cancellation

Signal.wait() now throws CancellationError when its task is cancelled, so a test timeout ends a never-fired wait instead of hanging the run.

* fix(settings): save duration presets on focus loss and keep the field in sync (#33)

* fix(settings): save duration presets on focus loss and keep the field in sync

The field only saved on Return and was never re-synced, so clicking away lost edits and it showed unsorted or stale text after a save or reset. It now commits on Return or focus loss through AppModel.applyPresets and re-renders from the saved presets.

* fix(settings): report preset save failures and clear stale preset errors

applyPresets now builds one settings value and writes it once, so a failed save returns false and the field keeps the typed text. A successful apply, or leaving the field with no edits, clears a leftover presets validation message.

* fix(settings): unblock recorder keys and refresh update availability (#34)

* fix(settings): unblock recorder keys and refresh update availability

* fix(updates): notify menu consumers when startup disables checks

* fix(settings): loop glyph previews and improve native QA (#35)

* feat(build): verify dev app launch and stream bounded logs

* fix(triggers): identify actions by rule and condition for accessibility

* fix(settings): loop glyph previews through every state

* fix(build): normalize the validated dev app path

* fix(lifecycle): preserve helper state and unblock prepared quits (#36)

* fix(updates): cancel Settings sheets after package preparation

* fix(helper): discard superseded power readbacks

* build(release): prepare 1.0.0-beta.6, build 11 (#37)

* build(release): prepare 1.0.0-beta.6, build 11

* docs(release): match acceptance wording to the current schema

* fix(settings): preserve preset drafts and clear retry errors (#39)

* fix(settings): clear preset save errors after a successful retry

* fix(settings): keep preset drafts when glass appearance changes

* fix(settings): preserve identical errors from other settings saves
@scttbnsn
scttbnsn deleted the fix/keyboard-and-update-menu branch November 6, 2026 12:42
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