From e31f65f54fe3438b096f81a116232c7e526eebb9 Mon Sep 17 00:00:00 2001 From: Minsu Lee <1964421+amondnet@users.noreply.github.com> Date: Fri, 5 Jun 2026 05:11:56 +0900 Subject: [PATCH] fix(settings): prevent IME composition cancellation in repository Display Name and Worktree Location inputs (#4632) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(settings): prevent IME composition cancellation in repository Display Name and Worktree Location inputs Controlled inputs bound directly to the zustand store caused IME composition to be cancelled on every keystroke because React re-rendered from the stale store value before the async IPC round-trip completed. On slow typing this yielded bare jamo decomposition (가나다 → ㄱㅏㄴㅏㄷㅏ); on fast typing characters were silently dropped. Introduce RepoSettingsDraftInput, a wrapper that keeps keystrokes in local draft state so the input is always synchronously controlled. The store is still written on every keystroke in the background so the sidebar stays live. Draft resets only when the active repo changes, so async IPC echoes cannot clobber newer draft text. Apply RepoSettingsDraftInput to both the Display Name and the Worktree Location inputs (identical store-bound binding, same root cause). Also link the Display Name Label to its input via htmlFor/id for a11y and test-locator clarity. Tests added: - RepositoryPaneDraftInput.test.tsx: unit tests (happy-dom) — draft survives stale store re-render and stale IPC echo; resets on repo switch; persists on every keystroke. - tests/e2e/settings-display-name-ime.spec.ts: Playwright e2e driving Blink's real IME pipeline via CDP Input.imeSetComposition / Input.insertText with an adaptive 2-set Korean IME emulator. Fails with 'ㄱㅏㄴㅏㄷㅏ' on the pre-fix code; passes on the fix. * fix(settings): preserve external repo setting updates Co-authored-by: Orca --------- Co-authored-by: Jinwoo-H Co-authored-by: Orca --- .../components/settings/RepositoryPane.tsx | 84 ++++++++-- .../RepositoryPaneDraftInput.test.tsx | 112 +++++++++++++ tests/e2e/settings-display-name-ime.spec.ts | 155 ++++++++++++++++++ 3 files changed, 336 insertions(+), 15 deletions(-) create mode 100644 src/renderer/src/components/settings/RepositoryPaneDraftInput.test.tsx create mode 100644 tests/e2e/settings-display-name-ime.spec.ts diff --git a/src/renderer/src/components/settings/RepositoryPane.tsx b/src/renderer/src/components/settings/RepositoryPane.tsx index 9999b74ea..cf4199749 100644 --- a/src/renderer/src/components/settings/RepositoryPane.tsx +++ b/src/renderer/src/components/settings/RepositoryPane.tsx @@ -1,4 +1,4 @@ -import { useCallback, useRef, useState } from 'react' +import { useCallback, useEffect, useRef, useState } from 'react' import type { OrcaHooks, Repo, RepoHookSettings } from '../../../../shared/types' import { getRepoKindLabel, isFolderRepo } from '../../../../shared/repo-kind' import { Button } from '../ui/button' @@ -31,6 +31,61 @@ type RepositoryPaneProps = { removeProject: (repoId: string) => void } +type RepoTextDraft = { repoId: string; text: string } + +// Why: updateRepo persists via async IPC before the store value updates, so a +// store-controlled input resets mid-IME-composition (Hangul decomposes into +// jamo). Keep keystrokes in local draft state; persist stays per-keystroke. +export function RepoSettingsDraftInput({ + repoId, + storeValue, + onTextChange, + ...inputProps +}: { + repoId: string + storeValue: string + onTextChange: (text: string) => void +} & Omit, 'value' | 'onChange'>): React.JSX.Element { + const [draft, setDraft] = useState({ repoId, text: storeValue }) + const pendingStoreEchoesRef = useRef([]) + + useEffect(() => { + setDraft((current) => { + if (current.repoId !== repoId) { + pendingStoreEchoesRef.current = [] + return { repoId, text: storeValue } + } + if (storeValue === current.text) { + pendingStoreEchoesRef.current = [] + return current + } + const pendingEchoIndex = pendingStoreEchoesRef.current.indexOf(storeValue) + if (pendingEchoIndex !== -1) { + // Why: queued updateRepo calls can echo older input text after newer + // keystrokes; accepting that echo re-cancels active IME composition. + pendingStoreEchoesRef.current.splice(0, pendingEchoIndex + 1) + return current + } + pendingStoreEchoesRef.current = [] + return { repoId, text: storeValue } + }) + }, [repoId, storeValue]) + + const text = draft.repoId === repoId ? draft.text : storeValue + return ( + { + const nextText = e.target.value + pendingStoreEchoesRef.current.push(nextText) + setDraft({ repoId, text: nextText }) + onTextChange(nextText) + }} + /> + ) +} + export function matchesRepositoryIdentitySearch(query: string, repo: Repo): boolean { const normalizedQuery = normalizeSettingsSearchQuery(query) if (!normalizedQuery) { @@ -215,14 +270,14 @@ export function RepositoryPane({ className="space-y-2" forceVisible={forceFullPaneForRepoMatch} > - - - updateRepo(repo.id, { - displayName: e.target.value - }) - } + + updateRepo(repo.id, { displayName: text })} className="h-9 text-sm" /> @@ -292,13 +347,12 @@ export function RepositoryPane({ ) : null} - - updateRepo(repo.id, { - worktreeBasePath: e.target.value.trim() ? e.target.value : undefined - }) + onTextChange={(text) => + updateRepo(repo.id, { worktreeBasePath: text.trim() ? text : undefined }) } className="h-9 text-sm" /> diff --git a/src/renderer/src/components/settings/RepositoryPaneDraftInput.test.tsx b/src/renderer/src/components/settings/RepositoryPaneDraftInput.test.tsx new file mode 100644 index 000000000..e97134a7d --- /dev/null +++ b/src/renderer/src/components/settings/RepositoryPaneDraftInput.test.tsx @@ -0,0 +1,112 @@ +// @vitest-environment happy-dom + +import React, { act } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { RepoSettingsDraftInput } from './RepositoryPane' + +let container: HTMLDivElement +let root: Root + +beforeEach(() => { + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) +}) + +afterEach(() => { + act(() => { + root.unmount() + }) + container.remove() +}) + +function render(props: { + repoId: string + storeValue: string + onTextChange: (text: string) => void +}): void { + act(() => { + root.render(React.createElement(RepoSettingsDraftInput, props)) + }) +} + +function getInput(): HTMLInputElement { + const input = container.querySelector('input') + if (!input) { + throw new Error('input not rendered') + } + return input +} + +function typeText(text: string): void { + act(() => { + const input = getInput() + // Why: React reads controlled-input changes via the native value setter; + // assigning input.value directly is swallowed by React's value tracking. + const setValue = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, 'value')?.set + setValue?.call(input, text) + input.dispatchEvent(new Event('input', { bubbles: true })) + }) +} + +describe('RepoSettingsDraftInput', () => { + it('keeps draft text while the store still holds the previous value (IME regression)', () => { + const onTextChange = vi.fn() + render({ repoId: 'repo-1', storeValue: '가', onTextChange }) + + typeText('가나') + + // Why: updateRepo persists via async IPC, so the store re-renders the pane + // with the stale value first. Reverting the input here is what aborted the + // Hangul IME composition (가나다 → ㄱㅏㄴㅏㄷㅏ). + render({ repoId: 'repo-1', storeValue: '가', onTextChange }) + + expect(getInput().value).toBe('가나') + expect(onTextChange).toHaveBeenCalledWith('가나') + }) + + it('keeps draft text when a stale store echo arrives after newer keystrokes', () => { + const onTextChange = vi.fn() + render({ repoId: 'repo-1', storeValue: '', onTextChange }) + + typeText('가') + typeText('가나') + + // Stale repos:changed echo of the first keystroke. + render({ repoId: 'repo-1', storeValue: '가', onTextChange }) + + expect(getInput().value).toBe('가나') + }) + + it('accepts same-repo store changes that did not come from the input draft', () => { + const onTextChange = vi.fn() + render({ repoId: 'repo-1', storeValue: '../custom-worktrees', onTextChange }) + + render({ repoId: 'repo-1', storeValue: '', onTextChange }) + + expect(getInput().value).toBe('') + }) + + it('resets the draft when the pane switches repos', () => { + const onTextChange = vi.fn() + render({ repoId: 'repo-1', storeValue: 'Repo One', onTextChange }) + + typeText('Renamed') + + render({ repoId: 'repo-2', storeValue: 'Repo Two', onTextChange }) + + expect(getInput().value).toBe('Repo Two') + }) + + it('persists every keystroke through onTextChange', () => { + const onTextChange = vi.fn() + render({ repoId: 'repo-1', storeValue: '', onTextChange }) + + typeText('a') + typeText('ab') + + expect(onTextChange).toHaveBeenNthCalledWith(1, 'a') + expect(onTextChange).toHaveBeenNthCalledWith(2, 'ab') + }) +}) diff --git a/tests/e2e/settings-display-name-ime.spec.ts b/tests/e2e/settings-display-name-ime.spec.ts new file mode 100644 index 000000000..d00579109 --- /dev/null +++ b/tests/e2e/settings-display-name-ime.spec.ts @@ -0,0 +1,155 @@ +/** + * Regression test for Hangul IME composition in the repository Display Name + * setting (jamo decomposition: typing 가나다 produced ㄱㅏㄴㅏㄷㅏ). + * + * Why CDP: Playwright's keyboard API cannot drive IME composition. The CDP + * `Input.imeSetComposition` / `Input.insertText` commands go through Blink's + * real composition pipeline, so a controlled-input value reset mid-composition + * cancels the composition exactly like a real OS IME session. + */ +import type { CDPSession, Locator, Page } from '@stablyai/playwright-test' +import { test, expect } from './helpers/orca-app' +import { getStoreState, waitForSessionReady } from './helpers/store' +import type { Repo } from '../../src/shared/types' + +async function openRepoSettings(page: Page, repoId: string): Promise { + await page.evaluate((repoId) => { + const state = window.__store!.getState() + state.openSettingsTarget({ pane: 'repo', repoId }) + state.openSettingsPage() + }, repoId) + await expect(page.getByPlaceholder('Search settings')).toBeVisible({ timeout: 10_000 }) + // Why: first-run announcements can cover the settings pane on fresh profiles. + const maybeLaterButton = page.getByRole('button', { name: 'Maybe Later' }) + if (await maybeLaterButton.isVisible({ timeout: 1_000 }).catch(() => false)) { + await maybeLaterButton.click() + } +} + +/** + * Minimal 2-set Korean IME combination table for the key sequence ㄱㅏㄴㅏㄷㅏ. + * Returns the next composition text, plus the syllable to commit when the new + * key cannot join the pending composition (간 + ㅏ → commit 가, compose 나). + */ +function combineJamo(pending: string, key: string): { commit?: string; compose: string } { + const joins: Record = { + 'ㄱ+ㅏ': { compose: '가' }, + '가+ㄴ': { compose: '간' }, + '간+ㅏ': { commit: '가', compose: '나' }, + '나+ㄷ': { compose: '낟' }, + '낟+ㅏ': { commit: '나', compose: '다' } + } + if (!pending) { + return { compose: key } + } + // Why: like a real IME, a non-joinable key commits the pending text and + // starts a fresh composition with just the new key. + return joins[`${pending}+${key}`] ?? { commit: pending, compose: key } +} + +/** + * Emulates a real 2-set Korean IME typing 가나다 slowly (keys: ㄱㅏㄴㅏㄷㅏ). + * + * Adaptive on purpose: a real OS IME is cancelled (ImeCancelComposition) when + * the page rewrites the text backing its composition — exactly what a + * controlled React input does when its store echo is async. That browser→IME + * channel is not observable from page JS (no compositionend fires), so the + * emulator detects the same condition directly: an input event whose value the + * page clobbered right afterwards. After a clobber the IME restarts from an + * empty state on the next key, which is what turned 가나다 into ㄱㅏㄴㅏㄷㅏ. + * + * The per-key delay models slow human typing: it gives the async updateRepo + * store echo time to land between keystrokes, which is exactly the condition + * that produced the full jamo decomposition. + */ +async function typeHangulGanadaSlowly( + session: CDPSession, + page: Page, + input: Locator +): Promise { + await input.evaluate((el) => { + const w = window as unknown as { __imeClobbered?: boolean } + w.__imeClobbered = false + el.addEventListener('input', () => { + const seen = (el as HTMLInputElement).value + // Why: React restores the controlled value after the input event's + // dispatch but later than its microtasks, so poll on a macrotask. + setTimeout(() => { + if ((el as HTMLInputElement).value !== seen) { + w.__imeClobbered = true + } + }, 0) + }) + }) + const takeClobbered = (): Promise => + page.evaluate(() => { + const w = window as unknown as { __imeClobbered?: boolean } + const clobbered = w.__imeClobbered === true + w.__imeClobbered = false + return clobbered + }) + + let pending = '' + for (const key of ['ㄱ', 'ㅏ', 'ㄴ', 'ㅏ', 'ㄷ', 'ㅏ']) { + if (await takeClobbered()) { + pending = '' + } + const { commit, compose } = combineJamo(pending, key) + if (commit) { + await session.send('Input.insertText', { text: commit }) + } + await session.send('Input.imeSetComposition', { + text: compose, + selectionStart: compose.length, + selectionEnd: compose.length + }) + pending = compose + // Slow typing: let the async store echo land before the next key. + await page.waitForTimeout(200) + } + // A clobbered final composition was already committed by the page's own + // echo; only a still-live composition needs an explicit IME commit. + if (!(await takeClobbered())) { + await session.send('Input.insertText', { text: pending }) + } +} + +test.describe('Repository Display Name IME composition', () => { + test('keeps Hangul syllables composed while typing slowly', async ({ orcaPage }) => { + await waitForSessionReady(orcaPage) + + const repos = await getStoreState(orcaPage, 'repos') + expect(repos.length).toBeGreaterThan(0) + const repo = repos[0] + + await openRepoSettings(orcaPage, repo.id) + + const repoSection = orcaPage.locator(`[data-settings-section="repo-${repo.id}"]`) + const displayNameInput = repoSection.getByLabel('Display Name') + await expect(displayNameInput).toHaveValue(repo.displayName) + + // Clear and wait for the async store echo to settle before composing. + await displayNameInput.click() + await displayNameInput.fill('') + await expect(displayNameInput).toHaveValue('') + + const session = await orcaPage.context().newCDPSession(orcaPage) + await typeHangulGanadaSlowly(session, orcaPage, displayNameInput) + + // Why: with the store-bound controlled input, the async updateRepo echo + // reset the field mid-composition, aborting the IME session per keystroke + // and committing bare jamo (ㄱㅏㄴㅏㄷㅏ) instead of syllables. + await expect(displayNameInput).toHaveValue('가나다') + + // The per-keystroke persist still reaches the store. + await expect + .poll( + async () => { + const current = await getStoreState(orcaPage, 'repos') + return current.find((entry) => entry.id === repo.id)?.displayName + }, + { timeout: 5_000, message: 'display name did not persist to the store' } + ) + .toBe('가나다') + }) +})