Skip to content

fix(desktop): support IME composition and reduce task noise#7537

Closed
waffensam wants to merge 2 commits into
BasedHardware:mainfrom
waffensam:codex/desktop-task-ime-fixes
Closed

fix(desktop): support IME composition and reduce task noise#7537
waffensam wants to merge 2 commits into
BasedHardware:mainfrom
waffensam:codex/desktop-task-ime-fixes

Conversation

@waffensam
Copy link
Copy Markdown
Contributor

Summary:

  • add IME marked-text tracking to the shared macOS chat text editor so composing text does not show placeholders or submit on Return
  • hide chat and floating-bar placeholders while IME composition is active
  • tighten task extraction prompt rules for public/group channels and add a prompt regression test

Tests:

  • git diff --check origin/main..HEAD
  • xcrun swift build -c debug --package-path Desktop
  • xcrun swift test --package-path Desktop --filter TaskAssistantPromptTests

Notes:

  • Swift build emits existing upstream warnings unrelated to this change.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 29, 2026

Greptile Summary

This PR fixes IME (Input Method Editor) composition handling in the macOS desktop chat editor and reduces noisy task extraction from public/group channels. The IME fix prevents placeholder text from flashing during composition and stops Return from submitting while text is still being composed; the task extraction change adds a prompt guard that suppresses task creation when the user is merely observing a public or community channel.

  • OmiTextEditor.swift: introduces OmiNSTextView (an NSTextView subclass) that overrides setMarkedText, unmarkText, and insertText to track composition state; the coordinator deduplicates state changes with lastMarkedTextState and dispatches them asynchronously to avoid mid-layout SwiftUI mutations; updateNSView also skips programmatic string assignment during marked-text to prevent clobbering the IME buffer.
  • AskAIInputView / ChatInputView: each adds a @State var hasMarkedText wired to OmiTextEditor.onMarkedTextChange, gating placeholder visibility on both the text being empty and composition being inactive.
  • TaskAssistantSettings.swift / TaskAssistantPromptTests.swift: adds a CRITICAL FOR PUBLIC/GROUP CHANNELS block to the extraction prompt and a new XCTest that guards against accidental deletion of the key phrases.

Confidence Score: 4/5

Safe to merge; the IME tracking logic is sound, text binding is guarded during composition, and the new prompt rules are additive.

All five changed files are low-risk: the IME state machine correctly deduplicates callbacks, the updateNSView guard prevents string clobbering during composition, and the placeholder gating in both input views is straightforward. The only notable gap is the test file, which checks string presence rather than exercising any extraction behavior, leaving prompt regressions that preserve the exact words but change their context undetected.

desktop/Desktop/Tests/TaskAssistantPromptTests.swift — the test assertions only verify substring presence and would not catch structural rearrangements or semantic regressions in the prompt.

Important Files Changed

Filename Overview
desktop/Desktop/Sources/Theme/OmiTextEditor.swift Core change: introduces OmiNSTextView subclass to track IME composition state via setMarkedText/unmarkText/insertText overrides; coordinator deduplicates callbacks and dispatches to main thread; updateNSView now skips string assignment during marked text and always syncs state after refreshing the closure. Logic is correct.
desktop/Desktop/Sources/FloatingControlBar/AskAIInputView.swift Adds hasMarkedText @State and gates placeholder visibility on it; wires onMarkedTextChange into OmiTextEditor. Straightforward and correct.
desktop/Desktop/Sources/MainWindow/Components/ChatInputView.swift Same pattern as AskAIInputView: adds hasMarkedText state and gates placeholder on it; supplies onMarkedTextChange callback. No issues.
desktop/Desktop/Sources/ProactiveAssistants/Assistants/TaskExtraction/TaskAssistantSettings.swift Adds a CRITICAL FOR PUBLIC/GROUP CHANNELS block to the analysis prompt that filters tasks unless the user is directly mentioned, in a DM, or an active participant. Prompt logic is reasonable; behavioural correctness depends on the LLM respecting it.
desktop/Desktop/Tests/TaskAssistantPromptTests.swift New test file; only asserts that four specific substrings are present in the prompt string. It is a textual-regression guard, not a behavioral test — no actual extraction logic is exercised.

Sequence Diagram

sequenceDiagram
    participant User
    participant OmiNSTextView
    participant Coordinator
    participant SwiftUI

    User->>OmiNSTextView: starts IME (e.g. Japanese input)
    OmiNSTextView->>OmiNSTextView: setMarkedText(_:selectedRange:replacementRange:)
    OmiNSTextView->>Coordinator: onMarkedTextStatusChange(true)
    Coordinator->>Coordinator: "updateMarkedTextState(true), lastMarkedTextState = true"
    Coordinator-->>SwiftUI: DispatchQueue.main.async → onMarkedTextChange(true)
    SwiftUI-->>SwiftUI: "hasMarkedText = true → placeholder hidden"

    User->>OmiNSTextView: presses Enter (during composition)
    OmiNSTextView->>Coordinator: doCommandBy(insertNewline:)
    Coordinator-->>OmiNSTextView: "hasMarkedText() == true → return false (IME handles it)"

    User->>OmiNSTextView: confirms IME selection
    OmiNSTextView->>OmiNSTextView: insertText(_:replacementRange:)
    OmiNSTextView->>Coordinator: onMarkedTextStatusChange(false)
    Coordinator->>Coordinator: "updateMarkedTextState(false), lastMarkedTextState = false"
    Coordinator-->>SwiftUI: DispatchQueue.main.async → onMarkedTextChange(false)
    SwiftUI-->>SwiftUI: "hasMarkedText = false → placeholder shown (if text empty)"

    Note over SwiftUI,OmiNSTextView: updateNSView guards text assignment via !textView.hasMarkedText()
Loading

Reviews (1): Last reviewed commit: "fix(desktop): reduce group-channel task ..." | Re-trigger Greptile

Comment on lines +9 to +13
XCTAssertTrue(prompt.contains("CRITICAL FOR PUBLIC/GROUP CHANNELS"))
XCTAssertTrue(prompt.contains("visible evidence shows the user is directly involved"))
XCTAssertTrue(prompt.contains("call no_task_found"))
XCTAssertTrue(prompt.contains("questions posted to the community at large"))
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Shallow string-presence assertions

The test only verifies that four literal substrings exist inside defaultAnalysisPrompt. It acts as a deletion guard but cannot catch prompt regressions where the wording is reordered, split across lines, or rephrased while losing semantic meaning — all of those would still pass. If the intent is a prompt-regression test, consider also asserting the key logical conditions in the right order (e.g. that the @mention bullet appears before the DM bullet, or that no_task_found follows the public-channel guard block), which would catch structural rearrangements that break the LLM's interpretation.

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!

@waffensam
Copy link
Copy Markdown
Contributor Author

Superseded by split desktop bugfix PRs: #7539 for IME composition and #7538 for group chat task false positives.

@waffensam waffensam closed this May 29, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Hey @waffensam 👋

Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request.

After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:

  • Project standards — Ensuring consistency across the codebase
  • User needs — Making sure changes align with what our users need
  • Code best practices — Maintaining code quality and maintainability
  • Project direction — Keeping aligned with our roadmap and vision

Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out.

Thank you for being part of the Omi community! 💜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant