Repository navigation
test(ime): install option-as-alt right before the dead-key dispatch - #14800
Merged
Merged
Conversation
testOptionDeadKeyUsesGhosttyTranslationInsteadOfStartingComposition swapped a macos-option-as-alt = true config into GhosttyApp before creating its surface, then pumped the run loop for up to 5 s waiting for the surface. A configuration reload queued earlier in the shared test host (appearance sync, theme, settings) could run during that pump: #14789's failing log reads the theme and user config files mid-test, before the surface's io starts. The reload replaced the app config, so the surface was created without option-as-alt, Option was never stripped, and the assertion saw Option set. The config is now installed after the surface exists, on both the app and the live surface, with no run-loop turn before the synchronous keyDown dispatch, and restored right after it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe dead-key input test helper now supports temporarily applying an Option-as-Alt configuration to a live surface. The regression test calls the helper directly and retains its existing expected results. ChangesDead-Key Input Tests
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Merge Risk: ⚪ Minimal · up to This change is confined to regression-test configuration setup and restoration; no concrete test-workflow or product regression is established. It is ready for normal checks. 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ Finishing Touches 💡 1📝 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 |
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Merge receipt for |
rustybret
pushed a commit
to rustybret/bmux
that referenced
this pull request
Sep 26, 2026
cc90659 test: a window with no restorable workspaces is dropped from the snapshot (manaflow-ai#14801) 9b10f7c Merge pull request manaflow-ai#14756 from manaflow-ai/12956-ssh-auth-followup-main fb665a0 test(ime): install option-as-alt right before the dead-key dispatch (manaflow-ai#14800) 9d459e3 fix(fork): an access-time update no longer discards a fresh fork validation (manaflow-ai#14799) 39e2c3a fix(ssh): share one route check across concurrent cmux ssh opens c344ce9 test: cover concurrent cmux ssh opens sharing one route check 9ff9017 fix(ssh): report OpenSSH failures from cmux ssh instead of a Cloud VM error 311797d test: cover cmux ssh failing fast on refused and unreachable hosts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DeadKeyCompositionRegressionTests.testOptionDeadKeyUsesGhosttyTranslationInsteadOfStartingCompositionfailed on #14789 with "An explicitly claimed Option side must show AppKit Ghostty's translated event": Option was still set on the interpreted events.Root cause
The test did three things in order:
macos-option-as-alt = trueconfig intoGhosttyApp.shared.TerminalSurface.swapConfigForTestingdoes not propagate to surfaces, so the surface takes whatever app config is current when it is created. A configuration reload queued earlier in the shared test host can run during that pump; appearance sync, theme and settings changes all queue one, and reloads wait at a font-work barrier. The reload replaces the app config with a freshly loaded one that has no option-as-alt.The failing log shows exactly that. Between the test starting and the surface's io thread spawning
/usr/bin/login, the app readsthemes/Apple System Colors Lightand the userconfig.ghostty, then finalizes a new config. The surface was created from that reloaded config, soghostty_surface_key_translation_modskept Option.Change
The test now installs the config after the surface exists, immediately before the synchronous
keyDowndispatch, with no run-loop turn in between:KeyboardLayout.textInputEventreads, and the live surface viaghostty_surface_update_config, insidesuppressGhosttyReloadActions.Test-only; no production change.
Test plan
DeadKeyCompositionRegressionTestsgreen🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes a flaky IME dead-key regression test by installing
macos-option-as-altafter the surface exists instead of before creating it. The config swap previously happened during a run-loop pump where a queued configuration reload from the shared test host could overwrite the app config, leaving the surface without option-as-alt. The test now updates both the app and the live surface config immediately before the synchronous key dispatch and restores both right after, while the surface is still alive (liveSurfaceis a raw pointer).Test-only; no production change.
Written for commit 009eca7. Summary will update on new commits.
Summary by CodeRabbit