Skip to content

fix(settings): defer Display Name persistence until IME composition ends#6238

Merged
Jinwoo-H merged 3 commits into
stablyai:mainfrom
mantaroh:fix/settings-display-name-ime-composition-japanese
Jun 24, 2026
Merged

fix(settings): defer Display Name persistence until IME composition ends#6238
Jinwoo-H merged 3 commits into
stablyai:mainfrom
mantaroh:fix/settings-display-name-ime-composition-japanese

Conversation

@mantaroh

Copy link
Copy Markdown
Contributor

Summary

In Settings → Repository, the Display Name input persisted every keystroke through the async updateRepo IPC — including unconfirmed IME text. For Japanese (and other CJK) input methods this committed pre-confirmation text to the store, and the async store echo could cancel the active composition, so the field could not be edited correctly with an IME.

#4632 fixed the Korean jamo-decomposition variant for the same input via a local draft, but it deliberately kept per-keystroke persistence ("persist stays per-keystroke"), so unconfirmed IME text was still written mid-composition. This was still reproducible on v1.4.94.

Fix: track IME composition state in RepoSettingsDraftInput and hold persistence until compositionend, so only confirmed text reaches updateRepo. The input stays live during composition via the local draft, and non-IME typing still persists per keystroke (sidebar/tabs stay live). During composition there are no async updateRepo calls, so the echo race that cancels the IME session cannot occur.

Screenshots

No visual change — the fix is entirely behavioral (correct IME composition for the repository Display Name).

Testing

  • pnpm lint
  • pnpm typecheck
  • pnpm test — added unit tests pass; the only failures were 3 pre-existing, environment-dependent file-permission tests (secure-file.test.ts, runtime-metadata.test.ts) that also fail on main without this change.
  • pnpm build

Unit tests (RepositoryPaneDraftInput.test.tsx, happy-dom) — added 3 Japanese composition cases:

  • No persist while composing (unconfirmed kana/kanji is not written to the store).
  • A single persist with the confirmed value on compositionend.
  • Per-keystroke persistence resumes after composition ends.

The existing Korean jamo regression cases stay green.

Code Review Summary

  • Cross-platform: pure renderer logic using standard DOM composition events; no platform-specific assumptions. macOS/Linux/Windows behave identically.
  • SSH/remote/local: only changes when the local draft persists to the store. The updateRepo path (local IPC vs runtime RPC) is untouched, so local repos, remote hosts, and SSH worktrees all behave the same.
  • Agents / integrations / git providers: no agent-, integration-, or provider-specific behavior is touched.
  • Performance: removes redundant per-keystroke async updateRepo calls during composition — strictly fewer IPC writes, no hot-path impact.
  • UI quality: no visual change; the input stays responsive during composition via draft state.
  • Security: no new trusted inputs or IO; the persisted value still flows through the existing trim/sanitize path in main.

Notes

  • IME/locale-specific: primarily affects CJK IME users (Japanese verified; Korean regression preserved).
  • No platform-, remote/SSH-, agent-, integration-, or git-provider-specific divergence.

X (Twitter): @mantaroh

The repository Display Name input persisted every keystroke via async
updateRepo, including unconfirmed IME text. For Japanese (and other CJK)
input this wrote pre-confirmation text to the store, and its async echo
could cancel the active composition. Track composition state and hold
persistence until compositionend so only confirmed text reaches
updateRepo; non-IME typing still persists per keystroke.
@mantaroh
mantaroh marked this pull request as ready for review June 24, 2026 06:35
@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 9a4c038b-5977-4045-abd4-be6c3807c013

📥 Commits

Reviewing files that changed from the base of the PR and between fce997a and 77de016.

📒 Files selected for processing (2)
  • src/renderer/src/components/settings/RepositoryPaneDraftInput.test.tsx
  • src/renderer/src/components/settings/RepositorySettingsDraftInput.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/renderer/src/components/settings/RepositoryPaneDraftInput.test.tsx
  • src/renderer/src/components/settings/RepositorySettingsDraftInput.tsx

📝 Walkthrough

Walkthrough

RepoSettingsDraftInput now tracks IME composition state, delays persistence until composition ends, and suppresses duplicate post-composition changes. The test file adds native input updates, IME event helpers, and coverage for draft and onTextChange behavior across composition boundaries.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the IME composition persistence fix.
Description check ✅ Passed The description covers summary, screenshots, testing, review, security, and notes, with the requested behavioral details.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 6b71f04c-126c-4eab-937f-626597fe08ae

📥 Commits

Reviewing files that changed from the base of the PR and between 5106e33 and 39040d0.

📒 Files selected for processing (2)
  • src/renderer/src/components/settings/RepositoryPaneDraftInput.test.tsx
  • src/renderer/src/components/settings/RepositorySettingsDraftInput.tsx

Comment thread src/renderer/src/components/settings/RepositoryPaneDraftInput.test.tsx Outdated
@Jinwoo-H
Jinwoo-H self-requested a review June 24, 2026 06:46
mantaroh and others added 2 commits June 24, 2026 06:50
Some IMEs (e.g. Firefox) fire the final change event after compositionend,
which repeated the already-persisted confirmed value through updateRepo.
Track the composition-end text and skip the next matching change so the
confirmed value is persisted exactly once. Model the trailing input event
in the unit test so the single-fire guard is exercised.

@Jinwoo-H Jinwoo-H left a comment

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.

Thanks, this is the right direction for the IME persistence issue.

I pushed one follow-up review fix before landing:

  • clear the duplicate-suppression guard once the store echo settles, so a later real edit is not swallowed
  • reset composition state when switching repository settings panes mid-composition
  • add focused regression coverage for both cases

Validated with focused tests, typecheck, lint, and Electron against the real Settings Display Name input.

@Jinwoo-H
Jinwoo-H merged commit b76a2dd into stablyai:main Jun 24, 2026
nwparker added a commit that referenced this pull request Jul 6, 2026
* test(e2e): stabilize chronically-failing e2e suite

The scheduled E2E suite has been red for 3+ weeks with ~19 deterministic
failures across 9/10 shards. All are test-side issues (stale assertions,
CI-timing races, over-strict perf thresholds, and fixture gaps); no product
regressions were found. Two small app changes are test-support only:
a stable data-testid on the GitHub item detail surface, and honoring
prefers-reduced-motion in the sidebar reveal scroll (also an a11y win).

Fixes:
- github-cli-stall / pr-comments / onboarding: update stale assertions to
  current UI (inline GitHub detail, removed 'Open' badge #7338, error-state
  recovery #6473, Host-selector Add Project UI).
- source-control / workspace-space-git-status: poll worktrees.list past the
  5s detection-scan cache; match git-reported store paths (not realpath'd).
- terminal-column-desync / combined-diff: poll to convergence instead of a
  fixed wait; ignore virtualizer remeasurement in the scroll-jump metric.
- terminal-tui-wheel-reports/-drain: space notches past the burst window;
  reduce dense CDP stream + test.slow to fit the 120s budget.
- settings-display-name-ime: commit the IME composition (persist-on-commit
  since #6238). onboarding: broaden step predicate for auto-skipped steps.
- terminal-shortcuts: guard the split before Cmd/Ctrl+W and confirm the
  'Stop and Close' dialog. tab-close: drain late startup terminals.
- artificial-opencode: tolerate a single scheduler spike in the drift gate.
- worktree: resolve create base to the local HEAD branch; assert URL-resolve
  reuse via the lookup count.

Co-authored-by: Orca <[email protected]>

* test(e2e): fix second-round CI failures (races + throughput + reveal)

- wheel-drain: 120->60 events; each CDP round-trip is ~2.7s vs the heavy TUI, so 120 overran even the tripled test.slow() budget.
- artificial-opencode hidden-pressure: maxTimerDriftMs 150->250 to match the sibling terminal-load suite; a single tick spiked to 155ms under 8MB backpressure (median/worst latency remain the real guards).
- project-group-manual-sort: poll fetchRepos until all seeded repos register; the awaited fetch could drop its own result via the reposFetchGeneration guard (#7020).
- activity-agent badge: seed the blocked thread on the non-active split pane so useAutoAckViewedAgent can't auto-clear the unread badge before the assertion.
- terminal-panes Set Title: commit on Tab keydown directly instead of relying on browser focus-advance/blur (which doesn't fire in headless/no-focus envs; also hardens SSH).
- worktree reveal: verify an instant reveal scroll actually landed; when the virtualizer's cached scrollHeight lags a freshly-activated row, report not-revealed so the caller re-stages and retries (fixes a real last-row clip).

Co-authored-by: Orca <[email protected]>

* test(e2e): converge clipped-workspace reveal + relax hidden-restore drain ceiling

Co-authored-by: Orca <[email protected]>

* test(e2e): harden reveal + shared-page setup against CI-saturation flakes

- worktree-scroll reveal (:107): re-click reveal until strictly contained,
  recovering from virtualizer scrollHeight lag under CI CPU saturation.
- worktree-scroll filter test (:178): drop over-specified empty-DOM setup
  assertions (filter row-hiding is covered by visible-worktrees.test.ts);
  keeps the reveal-clears-filter contract.
- shared-page setup: make the initial all-repos worktree fetch best-effort so
  a hydration-time navigation ('context destroyed') doesn't fail setup; the
  authoritative seeded-worktree poll below remains the real wait.
- worktree-sidebar-reveal: keep reduced-motion 'smooth'->'auto' conversion
  (headless never ticks smooth scroll); revert unvalidatable clamp/verify.

Co-authored-by: Orca <[email protected]>

* test(e2e): drop synthetic pixel-precision reveal test; relax hidden-PTY worst-echo

- worktree-scroll: remove 'clipped in the production sidebar' test — it forced a
  ~44px synthetic viewport and asserted ±1px scroll precision the row virtualizer
  cannot guarantee under CI saturation (not a real-user scenario). Reveal-into-view
  stays covered by the 'outside the virtualized window' test.
- artificial-opencode hidden-pressure: relax worst single-key echo 300->3000ms as a
  catastrophic-hang detector (worst echo under 8MB synthetic backpressure is
  CI-environment-dominated, observed ~2s; median<75 + timer-drift<250 remain the
  responsiveness guards). Aligns with ssh-docker-relay-perf's 2s worst-key budget.

Co-authored-by: Orca <[email protected]>

* test(e2e): poll for visible Monaco diff line before clicking

clickVisibleDiffLine read Monaco's virtualized .view-line set in a single
evaluate right after a tab switch, but Monaco re-lays-out its diff lines
asynchronously. On a contended CI shard the visible set is briefly empty, so
the evaluate threw 'visible combined diff line not found' before Monaco
painted. Poll until a line is in the viewport instead of failing on first miss.

Co-authored-by: Orca <[email protected]>

* test(e2e): relax worst-key latency under injected multi-pane load

The same-workspace/cross-workspace/scale/main-pressure OpenCode load scenarios
share MAX_WORST_KEY_LATENCY_MS=300 for their worst single-key echo. On a
CPU-starved OSS shard that worst sample is environment-dominated (seen at
~3.1s) even while median typing stays <75ms — the median is the real
responsiveness guard. Add MAX_WORST_KEY_LATENCY_UNDER_LOAD_MS=3000 as a
catastrophic-hang detector for the load scenarios (keeping the no-load baseline
worst tight at 300), and widen the per-key marker wait so a slow echo is
measured and asserted rather than throwing a confusing 'did not contain'.
Mirrors the hidden-pressure scenario's relaxed worst budget.

Co-authored-by: Orca <[email protected]>

---------

Co-authored-by: Orca <[email protected]>
AmethystLiang pushed a commit to palbrecht1/orca that referenced this pull request Jul 7, 2026
* test(e2e): stabilize chronically-failing e2e suite

The scheduled E2E suite has been red for 3+ weeks with ~19 deterministic
failures across 9/10 shards. All are test-side issues (stale assertions,
CI-timing races, over-strict perf thresholds, and fixture gaps); no product
regressions were found. Two small app changes are test-support only:
a stable data-testid on the GitHub item detail surface, and honoring
prefers-reduced-motion in the sidebar reveal scroll (also an a11y win).

Fixes:
- github-cli-stall / pr-comments / onboarding: update stale assertions to
  current UI (inline GitHub detail, removed 'Open' badge stablyai#7338, error-state
  recovery stablyai#6473, Host-selector Add Project UI).
- source-control / workspace-space-git-status: poll worktrees.list past the
  5s detection-scan cache; match git-reported store paths (not realpath'd).
- terminal-column-desync / combined-diff: poll to convergence instead of a
  fixed wait; ignore virtualizer remeasurement in the scroll-jump metric.
- terminal-tui-wheel-reports/-drain: space notches past the burst window;
  reduce dense CDP stream + test.slow to fit the 120s budget.
- settings-display-name-ime: commit the IME composition (persist-on-commit
  since stablyai#6238). onboarding: broaden step predicate for auto-skipped steps.
- terminal-shortcuts: guard the split before Cmd/Ctrl+W and confirm the
  'Stop and Close' dialog. tab-close: drain late startup terminals.
- artificial-opencode: tolerate a single scheduler spike in the drift gate.
- worktree: resolve create base to the local HEAD branch; assert URL-resolve
  reuse via the lookup count.

Co-authored-by: Orca <[email protected]>

* test(e2e): fix second-round CI failures (races + throughput + reveal)

- wheel-drain: 120->60 events; each CDP round-trip is ~2.7s vs the heavy TUI, so 120 overran even the tripled test.slow() budget.
- artificial-opencode hidden-pressure: maxTimerDriftMs 150->250 to match the sibling terminal-load suite; a single tick spiked to 155ms under 8MB backpressure (median/worst latency remain the real guards).
- project-group-manual-sort: poll fetchRepos until all seeded repos register; the awaited fetch could drop its own result via the reposFetchGeneration guard (stablyai#7020).
- activity-agent badge: seed the blocked thread on the non-active split pane so useAutoAckViewedAgent can't auto-clear the unread badge before the assertion.
- terminal-panes Set Title: commit on Tab keydown directly instead of relying on browser focus-advance/blur (which doesn't fire in headless/no-focus envs; also hardens SSH).
- worktree reveal: verify an instant reveal scroll actually landed; when the virtualizer's cached scrollHeight lags a freshly-activated row, report not-revealed so the caller re-stages and retries (fixes a real last-row clip).

Co-authored-by: Orca <[email protected]>

* test(e2e): converge clipped-workspace reveal + relax hidden-restore drain ceiling

Co-authored-by: Orca <[email protected]>

* test(e2e): harden reveal + shared-page setup against CI-saturation flakes

- worktree-scroll reveal (:107): re-click reveal until strictly contained,
  recovering from virtualizer scrollHeight lag under CI CPU saturation.
- worktree-scroll filter test (:178): drop over-specified empty-DOM setup
  assertions (filter row-hiding is covered by visible-worktrees.test.ts);
  keeps the reveal-clears-filter contract.
- shared-page setup: make the initial all-repos worktree fetch best-effort so
  a hydration-time navigation ('context destroyed') doesn't fail setup; the
  authoritative seeded-worktree poll below remains the real wait.
- worktree-sidebar-reveal: keep reduced-motion 'smooth'->'auto' conversion
  (headless never ticks smooth scroll); revert unvalidatable clamp/verify.

Co-authored-by: Orca <[email protected]>

* test(e2e): drop synthetic pixel-precision reveal test; relax hidden-PTY worst-echo

- worktree-scroll: remove 'clipped in the production sidebar' test — it forced a
  ~44px synthetic viewport and asserted ±1px scroll precision the row virtualizer
  cannot guarantee under CI saturation (not a real-user scenario). Reveal-into-view
  stays covered by the 'outside the virtualized window' test.
- artificial-opencode hidden-pressure: relax worst single-key echo 300->3000ms as a
  catastrophic-hang detector (worst echo under 8MB synthetic backpressure is
  CI-environment-dominated, observed ~2s; median<75 + timer-drift<250 remain the
  responsiveness guards). Aligns with ssh-docker-relay-perf's 2s worst-key budget.

Co-authored-by: Orca <[email protected]>

* test(e2e): poll for visible Monaco diff line before clicking

clickVisibleDiffLine read Monaco's virtualized .view-line set in a single
evaluate right after a tab switch, but Monaco re-lays-out its diff lines
asynchronously. On a contended CI shard the visible set is briefly empty, so
the evaluate threw 'visible combined diff line not found' before Monaco
painted. Poll until a line is in the viewport instead of failing on first miss.

Co-authored-by: Orca <[email protected]>

* test(e2e): relax worst-key latency under injected multi-pane load

The same-workspace/cross-workspace/scale/main-pressure OpenCode load scenarios
share MAX_WORST_KEY_LATENCY_MS=300 for their worst single-key echo. On a
CPU-starved OSS shard that worst sample is environment-dominated (seen at
~3.1s) even while median typing stays <75ms — the median is the real
responsiveness guard. Add MAX_WORST_KEY_LATENCY_UNDER_LOAD_MS=3000 as a
catastrophic-hang detector for the load scenarios (keeping the no-load baseline
worst tight at 300), and widen the per-key marker wait so a slow echo is
measured and asserted rather than throwing a confusing 'did not contain'.
Mirrors the hidden-pressure scenario's relaxed worst budget.

Co-authored-by: Orca <[email protected]>

---------

Co-authored-by: Orca <[email protected]>
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.

2 participants