Repository navigation
Recover wiped keyboard shortcuts: decode legacy StoredShortcut format (#5422) - #5423
austinywang wants to merge 2 commits into
Conversation
Decoding a pre-0.64.11 flat shortcut JSON into the new nested StoredShortcut throws keyNotFound('first'), so the SettingCodable path returns nil and the binding reverts to its default. This test captures that and fails until a legacy decoder is added.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a custom StoredShortcut.init(from:) that decodes the current nested first/second shape and falls back to the pre-0.64.11 flat shape (top-level key/command/.../chord* fields). The user's data is still present in UserDefaults under the same shortcut.<action> key, so this recovers it non-destructively; encoding stays the nested format. Fixes the regression where updating to v0.64.11+ silently reverted every customized keyboard shortcut to its default. Closes #5422 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughStoredShortcut implements a custom Codable decoder that supports both a new nested ChangesBackward-compatible shortcut decoder
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (16 passed)
✨ 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 |
Greptile SummaryAdds a custom
Confidence Score: 4/5Safe to merge; the decoder logic is correct, the fallback path is well-guarded, and the regression tests cover the real pre-0.64.11 JSON fixtures end-to-end. The custom decoder correctly distinguishes new from legacy JSON and the private LegacyFlatShortcut type is tightly scoped. The only forward-looking concern is that legacy data in UserDefaults is never rewritten to the new format, so the fallback decoder silently becomes a permanent dependency — a future cleanup that removes it would reproduce the original regression for any user who has not re-saved their shortcuts through the UI. Both changed files are straightforward; StoredShortcut.swift warrants a second look around the migration-write question before the legacy decoder is ever removed. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["StoredShortcut.init(from: decoder)"] --> B["container.decodeIfPresent(.first)"]
B -- "first key present & valid ShortcutStroke" --> C["Decode .second via decodeIfPresent (new nested format)"]
B -- "first key absent or null" --> D["LegacyFlatShortcut(from: decoder)"]
D --> E["decode 'key' (required discriminator)"]
E -- "key present" --> F["Build firstStroke from key/command/shift/option/control/keyCode"]
E -- "key absent → throws" --> G["Caller catches → falls back to action default binding"]
F --> H["decodeIfPresent chordKey"]
H -- "chordKey present & non-empty" --> I["Build secondStroke from chord* fields"]
H -- "chordKey absent or empty" --> J["secondStroke = nil"]
I --> K["StoredShortcut with chord"]
J --> L["StoredShortcut single-stroke or unbound"]
C --> M["StoredShortcut (new format)"]
Reviews (1): Last reviewed commit: "Recover legacy flat StoredShortcut bindi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if let first = try container.decodeIfPresent(ShortcutStroke.self, forKey: .first) { | ||
| self.first = first | ||
| self.second = try container.decodeIfPresent(ShortcutStroke.self, forKey: .second) | ||
| return | ||
| } | ||
| let legacy = try LegacyFlatShortcut(from: decoder) | ||
| self.first = legacy.firstStroke | ||
| self.second = legacy.secondStroke |
There was a problem hiding this comment.
Because decodeFromUserDefaults / decodeFromJSON both call try? and discard errors, and encodeForUserDefaults / encodeForJSON are only invoked when a setting is explicitly saved by the user, the legacy flat JSON will remain in UserDefaults indefinitely for users who never re-open and re-save their shortcuts. The legacy decoder will silently stay load-bearing forever; a future PR that removes it (reasonable cleanup) would silently revert those users' shortcuts again. Consider either (a) writing the decoded StoredShortcut back in the new format after a successful legacy decode, or (b) adding a doc comment on LegacyFlatShortcut that explicitly states it must be kept until a coordinated migration is shipped.
| @Test func newNestedFormatStillRoundTrips() throws { | ||
| // The legacy fallback must not regress the current nested format. | ||
| let original = StoredShortcut( | ||
| first: ShortcutStroke(key: "p", command: true, shift: true, keyCode: 35), | ||
| second: ShortcutStroke(key: "k", keyCode: 40) | ||
| ) | ||
| let data = try JSONEncoder().encode(original) | ||
| #expect(try JSONDecoder().decode(StoredShortcut.self, from: data) == original) | ||
| } |
There was a problem hiding this comment.
decodeFromJSON path not explicitly exercised
The PR description says "This one decoder fixes every read path: KeyboardShortcutSettingsLookup, and the package SettingCodable decodeFromUserDefaults / decodeFromJSON." The suite tests decodeFromUserDefaults directly but not decodeFromJSON. Both ultimately use JSONDecoder().decode(StoredShortcut.self, …), so the coverage is implicit, but a thin explicit test for decodeFromJSON with a legacy fixture would confirm the JSONSerialization → JSONDecoder round-trip and guard against a future refactor that diverges the two paths.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Packages/CmuxSettings/Sources/CmuxSettings/Values/StoredShortcut.swift`:
- Around line 32-38: Add DocC callouts to the public initializer
StoredShortcut.init(from:) so it documents the decoder parameter and possible
thrown errors; update the doc comment above `public init(from decoder: any
Decoder) throws` to include a `- Parameter decoder:` description explaining the
decoder input and a `- Throws:` description listing the conditions/errors that
can be thrown during decoding (e.g., invalid shape or missing required fields),
ensuring the public symbol meets the package documentation guideline.
- Around line 84-126: Move the private struct LegacyFlatShortcut (and its
init(from:) decoder logic that references ShortcutStroke) into a new file named
LegacyFlatShortcut.swift and remove the file-private visibility so the type is
internal (i.e., drop the leading "private") so StoredShortcut's init(from:) can
still decode using LegacyFlatShortcut; keep the same CodingKeys, Decodable
conformance, and behavior unchanged and run a build to ensure ShortcutStroke is
visible to the new file.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 34277bb8-217e-42aa-b417-4a83beb74e10
📒 Files selected for processing (2)
Packages/CmuxSettings/Sources/CmuxSettings/Values/StoredShortcut.swiftPackages/CmuxSettings/Tests/CmuxSettingsTests/StoredShortcutLegacyDecodingTests.swift
| /// Decodes the current nested shape and transparently recovers the legacy | ||
| /// flat shape persisted by cmux ≤ 0.64.10 (top-level `key` / `command` / … | ||
| /// / `chord*` fields). Without this, every shortcut a user customized | ||
| /// before the move to nested ``ShortcutStroke``s fails to decode and | ||
| /// silently reverts to its default. | ||
| /// See https://github.com/manaflow-ai/cmux/issues/5422. | ||
| public init(from decoder: any Decoder) throws { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Complete DocC callouts for the new public decoder initializer.
public init(from:) has a good summary, but this package rule requires full DocC callouts (- Parameter decoder: and - Throws: at minimum).
Suggested doc update
/// Decodes the current nested shape and transparently recovers the legacy
/// flat shape persisted by cmux ≤ 0.64.10 (top-level `key` / `command` / …
/// / `chord*` fields). Without this, every shortcut a user customized
/// before the move to nested ``ShortcutStroke``s fails to decode and
/// silently reverts to its default.
/// See https://github.com/manaflow-ai/cmux/issues/5422.
+/// - Parameter decoder: The decoder containing either nested or legacy-flat shortcut data.
+/// - Throws: A decoding error when neither supported shape can be decoded.
public init(from decoder: any Decoder) throws {As per coding guidelines: “Every public symbol in any new Swift package under Packages/ must be documented … with parameter/returns/throws callouts.”
🤖 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 `@Packages/CmuxSettings/Sources/CmuxSettings/Values/StoredShortcut.swift`
around lines 32 - 38, Add DocC callouts to the public initializer
StoredShortcut.init(from:) so it documents the decoder parameter and possible
thrown errors; update the doc comment above `public init(from decoder: any
Decoder) throws` to include a `- Parameter decoder:` description explaining the
decoder input and a `- Throws:` description listing the conditions/errors that
can be thrown during decoding (e.g., invalid shape or missing required fields),
ensuring the public symbol meets the package documentation guideline.
| /// The pre-0.64.11 on-disk shape of ``StoredShortcut``: the primary stroke's | ||
| /// fields flat at the top level plus optional `chord*` fields for a second | ||
| /// stroke. Decoded only as a fallback by ``StoredShortcut/init(from:)`` so | ||
| /// bindings persisted before the move to nested ``ShortcutStroke``s survive. | ||
| /// See https://github.com/manaflow-ai/cmux/issues/5422. | ||
| private struct LegacyFlatShortcut: Decodable { | ||
| let firstStroke: ShortcutStroke | ||
| let secondStroke: ShortcutStroke? | ||
|
|
||
| private enum CodingKeys: String, CodingKey { | ||
| case key, command, shift, option, control, keyCode | ||
| case chordKey, chordCommand, chordShift, chordOption, chordControl, chordKeyCode | ||
| } | ||
|
|
||
| init(from decoder: any Decoder) throws { | ||
| let c = try decoder.container(keyedBy: CodingKeys.self) | ||
| // `key` is the legacy discriminator: every legacy value has it (an | ||
| // unbound binding is `key == ""`). Its absence means the payload is | ||
| // neither the new nor the legacy shape, so decoding throws and the | ||
| // caller falls back to the action's default binding. | ||
| let key = try c.decode(String.self, forKey: .key) | ||
| firstStroke = ShortcutStroke( | ||
| key: key, | ||
| command: try c.decodeIfPresent(Bool.self, forKey: .command) ?? false, | ||
| shift: try c.decodeIfPresent(Bool.self, forKey: .shift) ?? false, | ||
| option: try c.decodeIfPresent(Bool.self, forKey: .option) ?? false, | ||
| control: try c.decodeIfPresent(Bool.self, forKey: .control) ?? false, | ||
| keyCode: try c.decodeIfPresent(UInt16.self, forKey: .keyCode) | ||
| ) | ||
| if let chordKey = try c.decodeIfPresent(String.self, forKey: .chordKey), !chordKey.isEmpty { | ||
| secondStroke = ShortcutStroke( | ||
| key: chordKey, | ||
| command: try c.decodeIfPresent(Bool.self, forKey: .chordCommand) ?? false, | ||
| shift: try c.decodeIfPresent(Bool.self, forKey: .chordShift) ?? false, | ||
| option: try c.decodeIfPresent(Bool.self, forKey: .chordOption) ?? false, | ||
| control: try c.decodeIfPresent(Bool.self, forKey: .chordControl) ?? false, | ||
| keyCode: try c.decodeIfPresent(UInt16.self, forKey: .chordKeyCode) | ||
| ) | ||
| } else { | ||
| secondStroke = nil | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Extract LegacyFlatShortcut into its own file.
This introduces a second meaningful type in a Packages/**/*.swift file; please move it to LegacyFlatShortcut.swift (or an equivalently type-named file) to keep package boundaries and navigation consistent.
As per coding guidelines: Packages/**/*.swift: “One major type per file; each struct, class, enum, actor, or protocol … with meaningful body lives in its own file named after the type.”
🤖 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 `@Packages/CmuxSettings/Sources/CmuxSettings/Values/StoredShortcut.swift`
around lines 84 - 126, Move the private struct LegacyFlatShortcut (and its
init(from:) decoder logic that references ShortcutStroke) into a new file named
LegacyFlatShortcut.swift and remove the file-private visibility so the type is
internal (i.e., drop the leading "private") so StoredShortcut's init(from:) can
still decode using LegacyFlatShortcut; keep the same CodingKeys, Decodable
conformance, and behavior unchanged and run a build to ensure ShortcutStroke is
visible to the new file.
CodeRabbit now posts non-blocking comment reviews (request_changes_workflow=false, #5538).
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Fixes #5422.
Updating to v0.64.11+ silently reverted every customized keyboard shortcut to its default. The Settings SPM reimplement (#4975) changed
StoredShortcut's persisted JSON from a flat shape to nestedfirst/secondstrokes, with synthesizedCodableand no legacy decoder. The storage key (shortcut.<action>inUserDefaults) is unchanged, so the new build reads each user's existing flat JSON, fails to decode it (keyNotFound("first")), swallows the error, and falls back to the default.Fix
A custom
StoredShortcut.init(from:)that decodes the nested shape and falls back to the legacy flat shape (mapping the flatkey/command/…/keyCodetofirstandchord*tosecond). The user's data is still inUserDefaultsunder the same key, so this recovers it non-destructively — no migration write, and encoding stays the nested format. A missingkey(neither shape) still throws so genuinely unrecognized data falls back to the default.This one decoder fixes every read path:
KeyboardShortcutSettingsLookup, and the packageSettingCodabledecodeFromUserDefaults/decodeFromJSON.Two-commit red → green
keyNotFound("first"), so theSettingCodablepath returns nil. Red.Verified locally with
swift test --package-path Packages/CmuxSettings(4 legacy tests fail on commit 1, all 44 pass on commit 2).The general guard going forward: a round-trip / legacy-decode test for every
Codabletype persisted inUserDefaultsorcmux.json, so a future serialized-shape change can't silently wipe user data.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Cursor Bugbot is generating a summary for commit ac32c3a. Configure here.
Summary by cubic
Restores users’ customized keyboard shortcuts by decoding the legacy flat
StoredShortcutformat, so upgrades to v0.64.11+ no longer reset bindings to defaults. Data is recovered on read from the sameUserDefaultskeys; encoding stays in the new nested shape.StoredShortcut.init(from:)that decodes the nestedfirst/secondstrokes and falls back to the legacy flatkey/command/.../chord*fields; throws only if neither shape matches.KeyboardShortcutSettingsLookupandSettingCodabledecode paths.Written for commit ac32c3a. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests