fix(settings): normalize icon refresh rate onto a 1–30 fps grid - #929
Conversation
Align the slider, URI handler, and live SCK capture floor so stored intervals only ever hold Off or 1/n seconds for n in 1…30. Snap URI writes before Defaults persistence, clamp live-loop sleep, and document the discrete grid. Default remains 0.25 s (~4 fps); SkyLight offscreen stays at 1/s. Signed-off-by: Camille Guillory <camille.guillory@gmail.com>
github-actions
Bot
added
docs
|
Labels: All convention checks passed. ✅ |
📝 WalkthroughWalkthroughThe PR normalizes icon refresh intervals to reciprocal FPS values from 1–30 FPS. It updates runtime throttling, settings assignment, slider limits, URI handling, tests, and URI documentation. ChangesIcon refresh interval normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsURIHandler
participant AdvancedSettings
participant MenuBarItemImageCache
participant SettingsPersistence
SettingsURIHandler->>AdvancedSettings: normalize iconRefreshInterval
AdvancedSettings->>SettingsPersistence: persist normalized value
AdvancedSettings->>MenuBarItemImageCache: provide supported interval
MenuBarItemImageCache->>MenuBarItemImageCache: clamp refresh-loop sleep
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Actionable comments posted: 1
🤖 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 `@Thaw/Settings/Models/AdvancedSettings.swift`:
- Around line 119-125: Update the iconRefreshInterval didSet logic around
normalizedIconRefreshInterval so normalization does not return before
Defaults.set executes. Persist the normalized value whenever it changes, while
retaining the oldValue guard, and add assertions covering Defaults loading and
profile application to verify the normalized interval is stored.
🪄 Autofix
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 Plus
Run ID: b178c7d4-a1d8-4bb7-a61b-e96ed3c9dbf0
📒 Files selected for processing (9)
Thaw/MenuBar/MenuBarItems/MenuBarItemImageCache.swiftThaw/Settings/Models/AdvancedSettings.swiftThaw/Settings/SettingsPanes/MenuBarLayoutSettingsPane.swiftThaw/Utilities/SettingsURIHandler.swiftThawTests/Settings/Models/AdvancedSettingsTests.swiftThawTests/Settings/Models/IconRefreshIntervalNormalizationTests.swiftThawTests/Settings/Models/ProfileDecodingDefaultsTests.swiftThawTests/Settings/URI/SettingsURIHandlerApplyTests.swiftdocs/URI_SCHEMES.md
diazdesandi
left a comment
diazdesandi
left a comment
There was a problem hiding this comment.
- Bug: the snapping path never writes to Defaults (
AdvancedSettings.swift:112).
if iconRefreshInterval != normalized {
iconRefreshInterval = normalized
return // nothing after this runs
}Swift does not re-enter a property observer when you assign to the property from inside its own didSet. The assignment only replaces the stored value. That return is not handing off to a nested didSet; it drops the write.
I checked this with a standalone repro of the same shape (swiftc, both -Onone and -O): assigning 3.5 leaves an in-memory 1.0 and zero Defaults.set calls.
So any off-grid write updates memory and leaves Defaults stale. That hits profile apply (Profile.swift:157) and the external-change path (AdvancedSettings.swift:346). Apply a profile carrying 2.5, relaunch, and you get whatever was in Defaults before. The URI path is safe only because it writes Defaults itself first.
Delete the return and fall through with the already-normalized value:
didSet {
let normalized = Self.normalizedIconRefreshInterval(iconRefreshInterval)
if iconRefreshInterval != normalized {
iconRefreshInterval = normalized
}
guard oldValue != iconRefreshInterval else { return }
Defaults.set(iconRefreshInterval, forKey: .iconRefreshInterval)
}Then 3.5 persists as 1.0, 0.26 persists as 0.25, and 0.25 writes nothing.
The tests miss this because they only assert the in-memory value (ProfileDecodingDefaultsTests.swift:327, AdvancedSettingsTests.swift:114). Adding Defaults.double(forKey: .iconRefreshInterval) == 1.0 to the profile-apply test would have caught it.
- Design: this reverts the capture rate limit from
8962b4be.MenuBarItemImageCache.swift:252goes from 250 ms to 1/30 s. That constant landed on July 27 in "perf(image-cache): rate-limit the on-screen capture path," and the comment being deleted says why: 30 composite captures per second is enough to pin a core.
Making the slider ceiling and the engine floor agree is the right goal. Raising the engine to 30 fps is one way; capping the slider at 4 fps is the other, and that one keeps the guard. Your own follow-up note says you may need to put the floor back. If 30 fps raises that risk, move the slider instead of the engine. If 30 fps really is wanted, that seems worth a word with whoever wrote 8962b4be, not a quiet revert inside a normalization change.
- Label: the breaking-change box says No, but the URI range narrowed.
SettingsURIHandler.swift:115goes from(0, 5)to(0, 1). An existingthaw://…?iconRefreshInterval=2.5used to store2.5and now stores1.0. Small blast radius; just tick the box.
What's good: the normalization function is well tested, and the 1 to 30 round trip is the right shape of test. Snapping in the URI handler before the Defaults write, instead of relying on a live observer, is the correct instinct. The Task.sleep fix stands on its own: Int(interval * 1000) truncating to a zero-length sleep was a real spin risk. Docs also quietly fix the stale default of 0.1.
Ensure off-grid values are written after normalization, including Defaults load and profile application paths. Signed-off-by: Camille Guillory <camille.guillory@gmail.com>
|
Addressed review:
|
|
@diazdesandi The requested persistence fix and tests are now in |
2 participants
Summary
Normalize icon refresh intervals so the slider, URI handler, live SCK capture floor, and Defaults all agree on one discrete grid: Off, or
1/nseconds for integernin 1…30.Scope: Icon refresh rate path only (settings + image-cache sleep/floor + URI/docs/tests).
Linked issue (required)
Closes: N/A
PR Type
Area
Does this PR introduce a breaking change?
thaw://values above 1 second now clamp to 1 second, and positive values snap to the documented 1–30 fps grid.What is the new behavior?
MenuBarItemImageCache.maxIconRefreshRate(30 fps); stored intervals snap onto the grid inAdvancedSettings.didSetand the normalized value is persisted.iconRefreshIntervalclamps to 0…1 s, then snaps onto the same grid before writing Defaults (so persistence does not depend on an observer).minIconRefreshIntervalso a sub-ms value cannot spin the main actor.0.25s (~4 fps). Users may opt into 30 fps; the slider warns that higher values use more CPU. SkyLight offscreen capture stays at 1/s.Addresses review on #928/#929: DCO sign-off, URI-side normalization, normalized Defaults persistence, explicit breaking-change disclosure. The 30 fps SCK ceiling is intentional; the existing UI warning exposes its CPU tradeoff.
PR Checklist
swiftformat .to keep the code style consistent.xcodebuild test …orswift test --package-path MenuBarModel.developmentbranch.Test commands run:
sudo xcodebuild -license). CI runs the suite.IconRefreshIntervalNormalizationTests, URI grid persistence, Defaults-load persistence, profile-apply persistence, and related model expectations.Follow-up work
Summary by CodeRabbit
Improvements
Documentation
0–1second range, frame-rate snapping, and default interval.