Repository navigation
Conversation
|
@knu is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
To use Codex here, create a Codex account and connect to github. |
📝 WalkthroughWalkthroughWhen marked/preedit IME text is active, committed text chunks are forwarded via a new helper that builds explicit Ghostty key events; the nil-text fallback is suppressed during marked text. A new XCTest verifies Control‑J forwards only the committed segment while leaving remaining marked text. ChangesCommitted IME Text Routing
Sequence Diagram(s)sequenceDiagram
participant KeyDown as keyDown Handler
participant Accum as accumulatedText Iteration
participant Helper as sendCommittedIMETextToSurface
participant Ghostty as Ghostty Surface
KeyDown->>Accum: iterate accumulated text chunks
Accum->>Accum: check markedTextBefore
Accum->>Helper: if chunk committed and marked, forward committed text
Helper->>Ghostty: send ghostty_input_key_s (text, composing=false)
Ghostty->>Ghostty: handle input
Accum->>KeyDown: continue processing
Possibly related PRs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (15 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 |
Contributor
Greptile SummaryThis PR fixes two independent bugs: (1) CJK IME-committed text was being dropped and the trigger shortcut (e.g., Ctrl+J) was leaking to the terminal when marked text was active; (2) the legacy Ghostty config was not loaded when the new config file was entirely absent (only loaded when it was present but empty, which was inconsistent with
Confidence Score: 5/5Safe to merge — both the IME text routing and config legacy-loading fixes are well-scoped, paired with updated tests that cover the corrected behavior, and make no structural changes to the surrounding key-event pipeline. The IME fix correctly threads committed text through a synthetic zero-keycode event, re-inserts the trigger key into the keyUp-suppression set before issuing No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[keyDown with Ctrl+J\nwhile IME preedit active] --> B{shouldSuppressGhosttyKeyForwardingAfterIMEHandling?}
B -- yes --> C[imeConsumedKeyUps.insert\nreturn]
B -- no --> D[imeConsumedKeyUps.remove keyCode\nbuild keyEvent]
D --> E{accumulatedText non-empty?}
E -- yes --> F[for each text in accumulatedText]
F --> G{shouldSendText?}
G -- yes + markedTextBefore --> H[imeConsumedKeyUps.insert keyCode\nsendCommittedIMETextToSurface\nkeycode=0 mods=NONE\ncontinue]
G -- yes + NOT markedTextBefore --> I[send keyEvent with text\nnormal path]
G -- no + NOT markedTextBefore --> J[send keyEvent text=nil\nno-text fallback]
G -- no + markedTextBefore --> K[skip — do nothing]
H --> L{more texts?}
I --> L
J --> L
K --> L
L -- yes --> F
L -- no --> M{shouldSendCommittedIMEConfirmKey?\nKorean + Enter + empty preedit}
M -- yes --> N[send Enter key to terminal]
M -- no --> O[done]
E -- no --> P[normal single-key send path]
Q[keyUp with Ctrl+J] --> R{imeConsumedKeyUps contains keyCode?}
R -- yes --> S[remove + suppress keyUp]
R -- no --> T[forward keyUp to Ghostty]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[keyDown with Ctrl+J\nwhile IME preedit active] --> B{shouldSuppressGhosttyKeyForwardingAfterIMEHandling?}
B -- yes --> C[imeConsumedKeyUps.insert\nreturn]
B -- no --> D[imeConsumedKeyUps.remove keyCode\nbuild keyEvent]
D --> E{accumulatedText non-empty?}
E -- yes --> F[for each text in accumulatedText]
F --> G{shouldSendText?}
G -- yes + markedTextBefore --> H[imeConsumedKeyUps.insert keyCode\nsendCommittedIMETextToSurface\nkeycode=0 mods=NONE\ncontinue]
G -- yes + NOT markedTextBefore --> I[send keyEvent with text\nnormal path]
G -- no + NOT markedTextBefore --> J[send keyEvent text=nil\nno-text fallback]
G -- no + markedTextBefore --> K[skip — do nothing]
H --> L{more texts?}
I --> L
J --> L
K --> L
L -- yes --> F
L -- no --> M{shouldSendCommittedIMEConfirmKey?\nKorean + Enter + empty preedit}
M -- yes --> N[send Enter key to terminal]
M -- no --> O[done]
E -- no --> P[normal single-key send path]
Q[keyUp with Ctrl+J] --> R{imeConsumedKeyUps contains keyCode?}
R -- yes --> S[remove + suppress keyUp]
R -- no --> T[forward keyUp to Ghostty]
Reviews (4): Last reviewed commit: "Load legacy Ghostty config when new file..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
knu
force-pushed
the
fix-cjk-ime-marked-commit
branch
from
May 28, 2026 04:53
aefbcae to
0a1ddc8
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/GhosttyTerminalView.swift (2)
3093-3099:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat a missing
config.ghosttyas a legacy-config fallback case.
fileSize(_:)returnsnilwhenconfig.ghosttyis absent, but this helper only returnstruefor0. That skips loading the legacyconfigfile for legacy-only installations, which contradicts the contract documented inloadLegacyGhosttyConfigIfNeeded.🐛 Proposed fix
static func shouldLoadLegacyGhosttyConfig( newConfigFileSize: Int?, legacyConfigFileSize: Int? ) -> Bool { guard let legacyConfigFileSize, legacyConfigFileSize > 0 else { return false } - return newConfigFileSize == 0 + guard let newConfigFileSize else { return true } + return newConfigFileSize == 0 }🤖 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 `@Sources/GhosttyTerminalView.swift` around lines 3093 - 3099, The helper shouldLoadLegacyGhosttyConfig(newConfigFileSize:legacyConfigFileSize:) currently only returns true when newConfigFileSize == 0, but fileSize(_:) yields nil when config.ghostty is missing; update the logic to treat a missing newConfigFileSize (nil) as a fallback as well. Keep the existing guard that legacyConfigFileSize must be non-nil and > 0, then return true if newConfigFileSize is nil or equals 0 so legacy config is loaded for legacy-only installations.
1725-3819: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract these additions out of
GhosttyTerminalView.swift.This file is already well past the repo’s size limit, and this change adds several hundred more lines across unrelated concerns: config loading, render/tick instrumentation, agent-hibernation tracking, terminal-surface helpers, and menu/clipboard actions. Please split the new helpers/types into dedicated files before merge.
As per coding guidelines,
**/{Sources,CLI,Packages,cmuxTests,cmuxUITests}/**/*.swift: “Do not add more than 250 lines to an existing production Swift file that is already over 800 lines, unless an extraction exception is met...” and “Do not mix UI rendering, state ownership, persistence, networking, parsing, subprocess/socket protocol, and platform bridge code in one Swift file.”Also applies to: 4912-7444, 8337-11017
🤖 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 `@Sources/GhosttyTerminalView.swift` around lines 1725 - 3819, The PR adds several hundred lines of unrelated helpers and types into GhosttyTerminalView.swift (config loading, CJK font logic, appearance sync, tick/logging, clipboard/transfer helpers) which violates the file-size and separation guidelines; extract these additions into focused new files. Move related groups into separate files (e.g. CJK font helpers -> GhosttyCJKFontFallback.swift containing cjkFontMappings, autoInjectedCJKFontMappings, resolvedInjectedCJKFontName, configuredCTFont, discoveredCTFont, fontContainsGlyphs, normalizedFontName, cjkCoverageSampleCharactersByRange, sharedCJKRanges, japaneseRanges; config parsing/scan helpers -> GhosttyConfigScan.swift with parsedConfigEntry, applyConfigFileDirective, scanFontConfigFile, scanAppearanceConfigFile, userFontConfigSummary, userAppearanceConfigSummary, loadedGhosttyConfigScanPaths, loadedCJKScanPaths, configFileSize, shouldIncludeLegacyGhosttyConfigInScanPaths, shouldLoadLegacyGhosttyConfig; appearance/runtime sync -> GhosttyAppearanceSync.swift with appearanceSynchronizationPlan, runtimeColorSchemeSynchronizationDecision, runtimeColorSchemeSynchronizationDecision, ghosttyRuntimeColorScheme, terminalRuntimeColorSchemePreference, runtimeColorSchemeForConfigLoad, shouldReloadConfigurationForAppearanceChange, shouldApplyDefaultBackgroundUpdate, synchronizeGhosttyRuntimeColorScheme overloads; background/log and scroll lag utilities -> GhosttyBackgroundAndLag.swift with DefaultBackgroundValues, defaultBackgroundValues, resolvedAppearanceValue, shouldCaptureScrollLagEvent, markScrollActivity/endScrollSession, and related properties moved to appropriate classes), update GhosttyTerminalView to import/use these new symbols (e.g. call UserFontConfigSummary via userFontConfigSummary(), call GhosttyConfig.load(...) etc.), and ensure visibility (fileprivate/internal) and tests are updated; keep all behavioral logic identical and run formatting/build to confirm no ABI or symbol name changes.
🤖 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.
Outside diff comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 3093-3099: The helper
shouldLoadLegacyGhosttyConfig(newConfigFileSize:legacyConfigFileSize:) currently
only returns true when newConfigFileSize == 0, but fileSize(_:) yields nil when
config.ghostty is missing; update the logic to treat a missing newConfigFileSize
(nil) as a fallback as well. Keep the existing guard that legacyConfigFileSize
must be non-nil and > 0, then return true if newConfigFileSize is nil or equals
0 so legacy config is loaded for legacy-only installations.
- Around line 1725-3819: The PR adds several hundred lines of unrelated helpers
and types into GhosttyTerminalView.swift (config loading, CJK font logic,
appearance sync, tick/logging, clipboard/transfer helpers) which violates the
file-size and separation guidelines; extract these additions into focused new
files. Move related groups into separate files (e.g. CJK font helpers ->
GhosttyCJKFontFallback.swift containing cjkFontMappings,
autoInjectedCJKFontMappings, resolvedInjectedCJKFontName, configuredCTFont,
discoveredCTFont, fontContainsGlyphs, normalizedFontName,
cjkCoverageSampleCharactersByRange, sharedCJKRanges, japaneseRanges; config
parsing/scan helpers -> GhosttyConfigScan.swift with parsedConfigEntry,
applyConfigFileDirective, scanFontConfigFile, scanAppearanceConfigFile,
userFontConfigSummary, userAppearanceConfigSummary,
loadedGhosttyConfigScanPaths, loadedCJKScanPaths, configFileSize,
shouldIncludeLegacyGhosttyConfigInScanPaths, shouldLoadLegacyGhosttyConfig;
appearance/runtime sync -> GhosttyAppearanceSync.swift with
appearanceSynchronizationPlan, runtimeColorSchemeSynchronizationDecision,
runtimeColorSchemeSynchronizationDecision, ghosttyRuntimeColorScheme,
terminalRuntimeColorSchemePreference, runtimeColorSchemeForConfigLoad,
shouldReloadConfigurationForAppearanceChange,
shouldApplyDefaultBackgroundUpdate, synchronizeGhosttyRuntimeColorScheme
overloads; background/log and scroll lag utilities ->
GhosttyBackgroundAndLag.swift with DefaultBackgroundValues,
defaultBackgroundValues, resolvedAppearanceValue, shouldCaptureScrollLagEvent,
markScrollActivity/endScrollSession, and related properties moved to appropriate
classes), update GhosttyTerminalView to import/use these new symbols (e.g. call
UserFontConfigSummary via userFontConfigSummary(), call GhosttyConfig.load(...)
etc.), and ensure visibility (fileprivate/internal) and tests are updated; keep
all behavioral logic identical and run formatting/build to confirm no ABI or
symbol name changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 05071dad-6962-4d40-8af0-c9962909f623
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/CJKIMEMarkedSelectionTests.swift
💤 Files with no reviewable changes (1)
- cmuxTests/CJKIMEMarkedSelectionTests.swift
Author
|
Rebased. |
Send the text committed by IME interpretation when marked text is active, instead of dropping it and forwarding only the original key event to the terminal. Covers Japanese IME commit paths triggered by control-key shortcuts, including both full and partial preedit commits.
Mark the original key event as IME-consumed when marked text commits through the synthetic text path, so the paired keyUp is not forwarded as an unmatched release. Extend the CJK IME regression test to cover the keyUp after a control-key shortcut commit.
knu
force-pushed
the
fix-cjk-ime-marked-commit
branch
from
June 23, 2026 12:07
0a1ddc8 to
1dc3a1f
Compare
Author
|
Updated. |
teamleaderleo
added
bug
Collaborator
|
This is covered on current main: IME committed text and trigger-key suppression are in |
Collaborator
|
The shipped implementation is on current main at 5eda9315. Thank you for the report :) |
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
2 participants
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
Send the text committed by IME interpretation when marked text is active, instead of dropping it and forwarding only the original key event to the terminal.
Covers Japanese IME commit paths triggered by control-key shortcuts, including both full and partial preedit commits.
Testing
Included in this PR.
Demo Video
For UI or behavior changes, include a short demo video (GitHub upload, Loom, or other direct link).
Before:
Committed text is unexpectedly lost, and the trigger shortcut (Ctrl+J) leaks to the terminal.
After:
Committed text is properly sent to the terminal, and the trigger shortcut (Ctrl+J) is consumed in IME.
Review Trigger (Copy/Paste as PR comment)
Checklist
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes CJK IME commit handling so committed text is sent to the terminal and the trigger shortcut is fully consumed (down and up). Also restores legacy config fallback: load legacy
configwhenconfig.ghosttyis missing or empty.keyCode0, no mods) when marked text is active; mark the trigger as IME-consumed so keyDown/keyUp don’t leak to the terminal.configifconfig.ghosttyis absent or empty and the legacy file is non-empty; update discovery logic and tests.Written for commit 1dc3a1f. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests