Skip to content

fix(platform): protect IME composition in commit fields - #4242

Merged
yannickmonney merged 1 commit into
mainfrom
fix/ime-enter-commit-fields
Oct 9, 2026
Merged

yannickmonney merged 1 commit into
mainfrom
fix/ime-enter-commit-fields

Conversation

@yannickmonney

@yannickmonney yannickmonney commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

IME candidate-confirming Enter and Escape now leave unfinished text alone in sent-message editing, task titles, label rename/create, label search and agent answers. A shared native/mirrored composition guard also protects Dialog, Popover and both ResponsiveDialog variants at the document-capture dismissal boundary, including closed and retained mobile sessions.

Ordinary Escape also cancels a dirty task title without saving it. Previously the draft reset was queued before a synchronous blur, and that blur saved the old edited closure. A synchronous per-focus settlement guard cancels first; subsequent Enter or Tab edits still commit once. No layout, strings, API or database changes.

Current head: 705723cdce0ce030039cb403f9fb545d6ae28616, tree a85e8e7be1617bc8051004d1bc4006869f6b5a0e. It preserves the original 19-file IME work through 8b1cc48c3623d02fe13e036a62e6e9e66a1190bb and adds the narrow Escape regression repair.

Validation on the current title repair:

  • Focused task component suites: 54 passed. The two new synchronous-blur regressions fail against the original production bytes with one unwanted save each.
  • Actual installed Chromium component execution: original code fails all four desktop/mobile cancellation cases; repaired code passes all four. Escape writes nothing, closes with opener focus restored, reopening retains the original title, and the next edit commits exactly once through Enter or Tab. Transport is mocked; 23 unrelated browser cases were intentionally not run.
  • Scoped type-aware lint/type-check, formatting, manual-register gate and normal commit hooks pass. Local SAST reports zero findings. React Doctor reports no errors and one existing complexity warning in the unchanged parent component.
  • Distinct source review accepts the narrow four-file repair and directly checks the causal browser receipts. This does not replace whole-feature acceptance or fresh CI.

Earlier exact-source IME proof remains historical: at 125d475266e001f2d76b94610d5a3b26b47fa723, the author reported 329 targeted passes with a separately disclosed folded-panel timeout followed by a passing standalone recheck. The native independent review is retained at #4242 (review) . Those old-source results are not claimed as fresh execution on the current head. The main integration at 8b1cc48 retained all 19 feature paths; the title repair leaves 16 of those blobs unchanged and narrowly extends the title source, its existing access test and the manual register.

The source/readiness assessment is complete and the PR is Ready for normal exact-head hosted CI. Required hosted gates and the final head/main/protection/verdict read remain pending before merge. Real OS IME, live backend persistence, the full browser suite and performance acceptance are not established by this focused repair. Installed Vitest 4.1.11 / browser 4.1.10 and extensionless-import warnings are retained. No CI rerun, cancellation, manual dispatch, merge or deployment was performed for this publication.

Closes #4210

Current-main rebase

Replayed the previously accepted source 74a551087c2cee091a1ad746db799c3ae84f886d onto main d1373d84cd56972501403f62145ec52e6f65d44a, including the merged shared CI repair in #4625. The accepted feature payload and all current-main changes are preserved in one atomic commit. Configured commit and conflict checks pass; earlier behavioral proof remains recorded above. All seven native required checks and full merge-group validation remain required for this new source.

Maintenance replay: preserves the accepted feature payload on current main 7d178ca. Includes the merged #4649 Knip cleanup and the exact independently accepted one-line shared CLI inventory repair from #4654 (252f0df), which is still pending native merge on main. The fixed suite inventory keeps its discovery and source/compiled phase guards. Existing feature proof is retained; no fresh full-feature/full-workspace test or hosted-green claim. Native required checks remain mandatory.

@yannickmonney yannickmonney left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Independent verdict: ACCEPT

Reviewed exact head 125d475266e001f2d76b94610d5a3b26b47fa723, confirmed at start and immediately before posting. Reviewer: TALE-567, agent 579ebc6d-a30d-43a6-b3fb-a340b6a22eae, run 44dbd1a3-ec65-4eff-b525-249418117c91; distinct from implementation agent dce9fe1e-9749-4181-8df8-464a4176ed56, run fcd0cbda-6cb3-49ac-a8e3-518da4323ce8. This is an independent agent acceptance record, not a human approval; the shared GitHub principal is also the PR author, so it is recorded as a COMMENT review rather than a GitHub APPROVED state.

No actionable findings against TALE-550 / #4210.

  • All six fields use the shared composition hook before their key actions. Tests cover the composition-event mirror, native isComposing, Safari 229, completed Enter and ordinary-key/blur controls. Label picker covers both selection and creation. Task title preserves its existing blur commit path.
  • Composer extraction retains its Enter/Shift+Enter and mid-composition paste guards; the complete 57-test composer suite passes. Expired mirrors clear on blur, detachment and closed sessions rather than suppressing a later interaction.
  • Real Dialog, Popover and desktop/mobile ResponsiveDialog guard document-capture Escape. Closed retained mobile drawer and detached-target cases pass, as do existing desktop/mobile focus-restoration and overlay-wrapper tests. active={open} reaches both task-title presentations.
  • Chat rename and ChatSurface production files are untouched. In-memory git merge-tree --write-tree with #4223 head f94581c971fe721dd9ec3d53644843a741a783df succeeds without conflicts; its attachment-retention changes remain separate. This is static composition evidence, not a combined browser run.

Independently observed proof

All tests use Node 24.21.0, targeted jsdom suites and one worker per invocation:

  • Six field/composer suites: 131 passed.
  • ChatSurface importer: 65 passed.
  • Task-modal draft/limits/repeat importers: 35 passed.
  • Hook, real overlays and adjacent UI/wrapper suites: 66 passed.
  • 297 tests passed in 17 suites total.
  • The five PR field-regression suites transplanted unchanged into an isolated worktree of main base 8a580fcc2e453c068a5119a37663777032aa7259: 26 failed / 48 passed, assertion failures reproducing every field. Breakdown: question 4, message edit 3, task title 3, label rename/create 8, picker selection/create 8. The six production field files and three changed overlay files are byte-identical between that main base and fetched origin/main 67c724a54d851803b7c6204bc13241f527e9a6a1.
  • Scoped oxlint --type-aware --type-check over all 17 changed TS files: exit 0. Scoped oxfmt --check, git diff --check and manual-reference gate: pass.

Initial Bun-hosted test execution hit an environment/runtime Zod import failure in two suites; rerunning the unchanged tests under Node passed. The first isolated red run hit Vite's external-font filesystem restriction; allowing the existing dependency directory in a review-only config produced the assertion-based red proof. Neither was treated as a product failure or as regression proof.

Unrun / handoff

No browser, real OS IME, E2E, container, full platform suite, whole-workspace tsc, full check or SAST was run. jsdom does not establish native Japanese/Chinese/Korean IME behavior, browser focus/layout fidelity or visual acceptance. Disk stayed above the 20 GiB floor (203 GiB at start, 197 GiB later).

CI snapshot still has five queued Candidate source checks; no green CI claim. No rerun/cancel, push, merge or card move. Acceptance is code/test acceptance for this exact head, not merge readiness. Author keeps CI ownership; TALE-359 owns subsequent merge-readiness reconciliation. Full logs and review report are in TALE-567's delivery box.

@yannickmonney

Copy link
Copy Markdown
Contributor Author

Updated with main, register rows only · TALE-621 merge lane · agent #2 3f9fdcee, run 2164d882

  • New head: 8b1cc48c3623d02fe13e036a62e6e9e66a1190bb. It is a merge commit (Merge remote-tracking branch 'origin/main' into fix/ime-enter-commit-fields) with two parents: the reviewed head 125d475266e001f2d76b94610d5a3b26b47fa723 and main ffa15e019c3b6453bfb82dc44231abef2b8be0de. I pushed it as a fast-forward at 09:19:07Z, so no history was rewritten.
  • The conflict was only in services/platform/tests/manual/reference/automation.md.
    • Every main row is kept, and this PR's three rows are unchanged.
    • I kept the three rows together as one block and placed it beside the existing tasks rows (after the TASK-A2 row) instead of at the top of the table, where every queued PR inserts its row.
  • My local checks:
  • Verdicts, re-read for the new head: the PR, TALE-550 and TALE-359 carry one verdict: TALE-567's ACCEPT (review 5404438267, at 125d4752). The PR body also cites the author's own pre-review. Neither raises a blocking finding, and the PR has no review threads. The new head changes no reviewed line except the rows' position.
  • CI: the old head's checks don't count for this head; one of them, Build sandbox-runtime, had ended cancelled. The new head's five workflows were queued at 09:19Z behind about 190 other queued runs: Checks 37191766975, Build 37191766965, E2E 37191766941, SAST 37191766944 and Commitlint 37191766950. CLI is path-filtered and doesn't run for this PR. I haven't rerun or cancelled anything.
  • Not merged. The merge waits for green CI at this exact head.

@yannickmonney

Copy link
Copy Markdown
Contributor Author

Blocking current-head finding for TALE-550 / PR #4242 at 8b1cc48: ordinary Escape can save a dirty task title instead of cancelling it.

In task-modal.tsx, EditableTitle explicitly promises commit on blur/Enter and revert on Escape. Its ordinary Escape handler calls setDraft(value) and immediately blurs; onBlur invokes commit from the same render, which still closes over the edited draft. This current PR adds the IME guard but retains that ordinary Escape path. Existing tests cover composing Escape and Enter/blur, not dirty ordinary Escape with zero mutation.

Observed product evidence: #4083 hosted run https://github.com/tale-project/tale/actions/runs/37189059988 , artifact 11299468224 (SHA256 67960267aa8d355125c46f38dc4e0766ae69bc567974f42ce43a9f6ae6682a0b), functional-baseline-50.json records title cancellation producing writes:1 and changed:true, followed by restoration of the synthetic title. The a9 baseline and 8a9 candidate modal source are byte-identical; the current #4242 path was independently source-compared but not executed in this audit. This is inherited behavior exposed by a real browser, not a #4083-specific regression or a reason to weaken its oracle.

Keep this with the existing TALE-550 author (Codex #4) and repair after its live TALE-450 acceptance work; do not interrupt that frozen fixture or add a parallel title writer. Add a causal focused test using actual focus, a dirty title, ordinary Escape and synchronous blur: no save/request, original title restored, existing focus/modal Escape behavior retained. Keep Enter/normal blur committing once and preserve all current IME start/end/229 controls. Reuse the established inline-edit cancellation pattern across any actual shared helper consumers; do not replace strict assertions with a delay.

Preserve prior exact-source tests as history, then provide fresh target execution, real isolated browser behavior, normal exact-head CI and distinct independent review on the repaired head. The prior TALE-567 acceptance does not close this newly observed finding. The merge lane must remove #4242 from ready work until this finding is independently closed. No acceptance benchmark rerun, source-baseline change, extra heavy/browser stack or deployment is authorized by this feedback. Root retains #4083 coordination.

@yannickmonney
yannickmonney marked this pull request as draft October 4, 2026 09:53
@yannickmonney

Copy link
Copy Markdown
Contributor Author

Root takes the bounded ordinary-Escape title repair on existing draft #4242 at8b1cc48c3623d02fe13e036a62e6e9e66a1190bb into a local isolated worktree. Fresh10:21:20Z guard:550Todo/C4, sourcefcd0cbda settled/no retry;450d46a9d45 settled/no retry;550pendingReviewnull/liveautomationnull. Preserve original C4 assignment/history and all IME behavior. The card is held Backlog with an explicit external reservation so no second author is dispatched while the normal manager's startup recovery is pending. Root will hand back only after causal focus+dirtyEscape tests, ordinaryEnter/blur andcomposition controls, distinct review and source publication; runtime/merge proof remains separate. No native agent start, schedule edit or deployment.

@yannickmonney

Copy link
Copy Markdown
Contributor Author

Independent source review: title Escape cancellation

ACCEPT the narrow repair at705723cdce0ce030039cb403f9fb545d6ae28616, treea85e8e7be1617bc8051004d1bc4006869f6b5a0e, parent8b1cc48c3623d02fe13e036a62e6e9e66a1190bb. Root read all4changedfiles and surrounding EditableTitle/ThreadRenameInput reuse. Manifestd66f30508116284145ccd4bd3daa73578ed1492b12525da6c18bb825b2838e81 independently matches4Gitblobs and21retained receipts.

Escape marks cancellation synchronously before setDraft and nativeblur, avoiding the stale draft closure's write. Per-focus reset admits subsequent edits; onBlur still clears composition state before settlement guard, and completed Enter/ordinary Tab blur commit once. Composition guard remains before either keyboard action. This is the same existing settlement-ref concept used in ThreadRenameInput, with reset because EditableTitle persists between focus sessions. No source blocker found in this bounded diff. Original IME behavior/source changes remain intact.

Observed author evidence was directly read:54focusedUI passes; two actual old-source jsdom failures; installed Chromium old-source4fail versus repaired4pass on desktop/mobile and Enter/Tab follow-on, real native key/overlay/focus/reopen interactions with explicit mutation I/O mocks. Root reviewed the bounded runtime plan before those runs. The tests reproduce the original stale-closure cancellation write instead of manually ordering separate blur events. Browser ownership/source-restoration/stop receipts retained; no backend persistence/native OS IME/performance claim. Existing full IME feature acceptance remains separate, as do fresh hosted checks and main composition. This source ACCEPT authorizes ordinary source publication only; PR stays draft until all feature gates close. No merge or deployment acceptance.

@yannickmonney
yannickmonney marked this pull request as ready for review October 4, 2026 10:53
@yannickmonney

Copy link
Copy Markdown
Contributor Author

PR #4242 is now Ready for normal exact-head CI at 705723cdce0ce030039cb403f9fb545d6ae28616 (tree a85e8e7be1617bc8051004d1bc4006869f6b5a0e). Fresh pre-transition guard confirmed OPEN/draft, exact head and current main ffa15e019c3b6453bfb82dc44231abef2b8be0de; that main is an ancestor and the clean composition equals the candidate tree.

The ordinary-Escape title-save blocker is closed by the narrow repair, two old-source jsdom failures versus 54 current focused passes, and four original-code Chromium failures versus four repaired passes on desktop/mobile with no cancellation write and one subsequent Enter/Tab save. Distinct current-source acceptance is retained here.

The original IME review at 125d4752 remains historical exact-source evidence: 297 independently observed jsdom passes/17 suites and 26 main-based assertion failures. All 18 original source/test/package blobs survive unchanged through the main integration; the current title addition has its own fresh proof. The task and independent review contract did not require an OS-IME or full local suite, and this receipt does not claim either. No other blocking source finding or review thread remains in the fresh readback.

Full current-head hosted CI, its actual execution/cache/retry evidence and the final live head/main/protection/verdict read are still required before a normal protected merge. The Ready event triggers ordinary workflows; no manual rerun/cancellation/dispatch was performed. TALE-550 remains Backlog with the same C4 owner and external root reservation, preventing duplicate authorship until explicit root handback or completion. No merge or deployment is claimed.

@yannickmonney
yannickmonney force-pushed the fix/ime-enter-commit-fields branch 2 times, most recently from f3305c8 to b1a80b3 Compare October 9, 2026 03:56
@yannickmonney
yannickmonney force-pushed the fix/ime-enter-commit-fields branch 3 times, most recently from 74a5510 to 7411ce5 Compare October 9, 2026 14:05
@yannickmonney
yannickmonney force-pushed the fix/ime-enter-commit-fields branch from 7411ce5 to eac9a7f Compare October 9, 2026 15:00
@yannickmonney
yannickmonney added this pull request to the merge queue Oct 9, 2026
@yannickmonney
yannickmonney merged commit 8333dbb into main Oct 9, 2026
64 checks passed
@yannickmonney
yannickmonney deleted the fix/ime-enter-commit-fields branch October 9, 2026 22:14
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.

Bug: six more Enter-to-commit fields save unfinished IME text

1 participant