Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe ChangesKeyboard shortcut test execution
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~3 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change only moves the affected keyboard shortcut tests to the main actor and does not alter production behavior, so merge risk is minimal. 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 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 |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
All contributors have signed the CLA ✍️ ✅ |
`shortcutConfigParsingRoundTripsSpaceKey` calls `resolvedKeyCode()`, which by default reads the current keyboard layout through Text Input Sources. Those calls must run on the main thread. `KeyboardShortcutSpaceKeyTests` is not main-actor isolated, so Swift Testing ran it on a background thread, where the layout lookup trapped and took the test host down with every test running beside it. Isolate the suite to the main actor, like the other suites that call `resolvedKeyCode()`.
49d9644 to
d215846
Compare
|
You actually had this one first! You opened it on Sep 14 and we ended up landing the same fix days later without spotting your PR, which is on us. Closing since main has it now, but the credit is yours. Thank you for tracking it down :) |
|
@ejc3 one more note, since these deserve a proper thank you: a bunch of your Sep 14 fixes (the Space key layout crash, the sidebar drag recursion, the offscreen blur reset crash, the window route recursion, the digit shortcut test) got redone on main about a week later because we did not look at your PRs first. You found and fixed them before we did. That is on us, and we are going to check contributor PRs before starting on a bug from now on. Thank you for all of this, seriously! |
Summary
KeyboardShortcutSpaceKeyTests.shortcutConfigParsingRoundTripsSpaceKeycan crash the unit-test host. The crash report shows aSIGTRAPon a background thread insideKeyboardLayout.characterFromInputSource, called fromShortcutStroke.resolvedKeyCode()at line 60 of the test.resolvedKeyCode()reads the current keyboard layout through Text Input Sources by default, and those calls must run on the main thread. This suite is not main-actor isolated, so Swift Testing runs it on a background thread. Every other suite that callsresolvedKeyCode()is@MainActor.The suite is now
@MainActor.Testing
4638e5b1eaplus the compile fixes in Fix the compile errors that keep main and its unit tests from building #12584, throughscripts/ci/run-app-host-xcodebuild.shin 12 batches the way CI runs app-host tests, a crash report shows this trap on a background thread fromshortcutConfigParsingRoundTripsSpaceKey, and the host died.KeyboardShortcutSpaceKeyTestsran and passed twice in batch 1, with no layout-lookup crash report.Demo Video
Not applicable. The change prevents a crash and has no visible effect.
Review Trigger (Copy/Paste as PR comment)
Checklist
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Runs
KeyboardShortcutSpaceKeyTestson@MainActorsoresolvedKeyCode()reads Text Input Sources on the main thread instead of Swift Testing’s background thread, preventing the unit-test host from crashing during space-key shortcut parsing.Written for commit d215846. Summary will update on new commits.
Summary by CodeRabbit