[bugfix] Preserve IME composition in data_editor NumberColumn (#16129) - #16165
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
✅ PR preview is ready!
|
✅ E2E test count change is within normal rangeThis PR has added 3 E2E test cases
|
The bundled `@glideapps/glide-data-grid` overlay editor treated the Enter that confirms a CJK IME composition as a cell-commit, closing the editor after the first composed digit and making multi-digit input via a Japanese/Korean IME impossible in `st.data_editor` NumberColumn cells. Patch the vendored library to bail out of the overlay's onKeyDown handler when `nativeEvent.isComposing` is true (or `keyCode` is 229), mirroring the guard Streamlit already uses for `isEnterKeyPressed` in `inputUtils.ts` and the fix previously applied to `st.chat_input` (#6988). Adds an e2e regression test that dispatches a composing Enter and verifies the cell overlay stays open, then confirms a normal Enter still commits.
c8870a6 to
e0c3f42
Compare
sfc-gh-dbyttow
added
change:bugfix
github-actions
Bot
removed
the
ai-review
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR fixes a bug (#16129) where CJK IME composition in st.data_editor NumberColumn cells was broken. The @glideapps/glide-data-grid overlay editor's onKeyDown handler treated the Enter key that confirms an IME composition (isComposing=true / keyCode=229) as a cell-commit, closing the editor after the first composed digit.
The fix applies a Yarn patch to @glideapps/glide-data-grid@6.0.4-alpha24 that bails out of the overlay editor's onKeyDown when an IME composition is in progress. An E2E regression test validates the fix.
Changed files:
frontend/.yarn/patches/@glideapps-glide-data-grid-npm-6.0.4-alpha24-9c1a40557b.patch(new) — the patchfrontend/package.json— registers the patch viaresolutionsfrontend/yarn.lock— lockfile update for the patched resolutione2e_playwright/st_data_editor_editing_test.py— new regression testNOTICES— updated license notices
Code Quality
Both reviewers agree the implementation is clean and minimal:
- The patch adds an early return at the top of
onKeyDownusing the standard IME detection pattern (event.nativeEvent?.isComposing === true || event.keyCode === 229), applied to both ESM and CJS builds. - The resolution entry in
package.jsonfollows the same pattern as the existingcolor2kpatch. - The E2E test reuses existing helpers and follows established patterns in the test file.
- No code-structure or maintainability issues identified.
Test Coverage
The new E2E test test_number_cell_editor_ignores_ime_composing_enter provides solid coverage:
- Verifies composing Enter (
isComposing: true,keyCode: 229) does not close the overlay editor. - Verifies regular Enter does still commit and close the editor.
Both reviewers agree that E2E coverage is the appropriate level for a fix in a vendored/patched third-party library, where frontend unit tests (Vitest/RTL) cannot easily reach the patched code path.
Backwards Compatibility
No breaking changes. Both reviewers confirm:
- Public Streamlit interfaces are unchanged.
- Non-IME Enter commit behavior is explicitly preserved.
- The change is limited to handling IME composition keydown events inside the data editor overlay.
Security & Risk
No security concerns identified by either reviewer. The change:
- Does not touch WebSocket/connection handling, authentication, session management, file serving, or cross-origin behavior.
- Does not introduce new dynamic code execution surfaces.
- Uses a local Yarn patch with no new external network behavior.
Overall risk is low and localized to keyboard event handling during composition.
External test recommendation
- Recommend external_test: No
- Triggered categories: None
- Evidence:
- The patch modifies in-browser keydown handling within the overlay editor only.
- The E2E test uses synthetic DOM events in a local app interaction.
package.json/yarn.lockchanges are resolution/lock metadata only — no routing, auth, embedding, or cross-origin changes.
- Suggested external_test focus areas: None required.
- Confidence: High
- Assumptions and gaps: Assessment assumes no runtime-side changes outside this diff and no external-host-specific key event translation differences beyond standard browser behavior.
Accessibility
No accessibility regressions. The change is an improvement for CJK input method users — IME composition events are now properly handled so multi-character input can be completed without premature cell commits.
Readability
Both reviewers found no readability issues in the changed code:
- The patch comments are clear with a traceability reference to
streamlit/streamlit#16129. - The test docstring effectively explains the regression scenario and intent.
- Test naming and inline comments clearly communicate what is being validated.
PR title/description: The title is clear, concise, and follows conventions. One reviewer optionally suggested aligning the PR body with the .github/pull_request_template.md headings for consistency — this is a stylistic preference, not blocking.
Recommendations
-
Consider upstream contribution (follow-up, not blocking): The IME fix is broadly useful. Consider contributing it upstream to
@glideapps/glide-data-gridso the patch can eventually be dropped. -
Optional: align PR body with repo template (not blocking): The PR body could use the standard
Describe your changes/GitHub Issue Link/Testing Planheadings for searchability.
Verdict
APPROVED: Both reviewers unanimously approve. The fix is targeted, well-tested, backwards compatible, and introduces no security or architecture risks. The Yarn patch follows established patterns and the E2E test provides solid regression coverage.
Reviewer agreement
| Aspect | claude-4.6-opus-high-thinking | gpt-5.3-codex-high |
|---|---|---|
| Verdict | APPROVED | APPROVED |
| Code quality | No issues | No issues |
| Test coverage | Sufficient (E2E) | Sufficient (E2E) |
| Security | No concerns | No concerns |
| External test | Not needed | Not needed |
| Backwards compat | No breaking changes | No breaking changes |
All expected reviewers completed their reviews successfully.
Consolidated review by claude-4.6-opus-high-thinking. Individual reviews from: claude-4.6-opus-high-thinking, gpt-5.3-codex-high.
This review also includes 1 inline comment(s) on specific code lines.
| + // (e.g. Enter that confirms a CJK IME candidate). Committing the | ||
| + // cell here would close the editor after the first composed | ||
| + // character. See streamlit/streamlit#16129. | ||
| + if (event.nativeEvent?.isComposing === true || event.keyCode === 229) { |
There was a problem hiding this comment.
thought: The existing inputUtils.isEnterKeyPressed uses only isComposing without the keyCode === 229 fallback. Including the keyCode === 229 guard here is a reasonable extra safeguard for older browsers — just noting the slight divergence from the existing Streamlit pattern. Both approaches are correct.
There was a problem hiding this comment.
Thanks — good catch on the divergence. I added the keyCode === 229 fallback here specifically because the overlay editor is a vendored third-party path where we cannot rely on inputUtils.isEnterKeyPressed running (this is inside the patched glide-data-grid code, not Streamlit-owned event handling), and older Safari/legacy Edge still surface composition Enter as keyCode 229 with isComposing=false. Keeping the belt-and-suspenders guard here is intentional; the Streamlit-owned isEnterKeyPressed path continues to use the isComposing-only pattern.
|
| Filename | Overview |
|---|---|
| frontend/.yarn/patches/@glideapps-glide-data-grid-npm-6.0.4-alpha24-9c1a40557b.patch | Adds equivalent IME composition guards to the package's CommonJS and ESM overlay editor builds. |
| frontend/package.json | Resolves Glide Data Grid 6.0.4-alpha24 to the new local Yarn patch. |
| frontend/yarn.lock | Locks the patched Glide Data Grid artifact and its dependency metadata. |
| e2e_playwright/st_data_editor_editing_test.py | Covers composing Enter and verifies that normal Enter still commits the NumberColumn editor. |
| NOTICES | Adds notices for dependencies brought into the generated patched package resolution. |
Reviews (1): Last reviewed commit: "Regenerate NOTICES for glide-data-grid p..." | Re-trigger Greptile
sfc-gh-lmasuch
added
the
ai-final-review
github-actions
Bot
removed
the
ai-final-review
There was a problem hiding this comment.
Summary
Fixes #16129: an st.data_editor NumberColumn cell would commit and close its overlay editor on the Enter that confirms a CJK IME composition, making multi-digit input via a Japanese/Korean IME impossible.
The fix is a yarn patch on the vendored @glideapps/glide-data-grid@6.0.4-alpha24 dependency. The overlay editor's onKeyDown now returns early when the event is part of an in-progress IME composition (event.nativeEvent?.isComposing === true || event.keyCode === 229), mirroring the guard Streamlit already uses in frontend/lib/src/util/inputUtils.ts (isEnterKeyPressed) and the earlier st.chat_input fix (#6988). The patch is wired via the existing resolutions mechanism in frontend/package.json (same as the color2k patch), with corresponding frontend/yarn.lock and NOTICES updates. A regression e2e test is added in e2e_playwright/st_data_editor_editing_test.py.
All three reviewers (gpt-5.4-xhigh, gemini-3.1-pro, claude-opus-4-8-thinking-xhigh) independently approved the change; the only differences were non-blocking suggestions on PR metadata and test fidelity, covered below.
Code Quality
Strong consensus across all reviewers: the change is minimal, well-scoped, and follows established repo conventions.
- Placing the guard at the top of the overlay editor's
onKeyDownshort-circuits Enter/Escape/Tab handling for every composing keystroke, which is the correct layer since IME composition only happens while the overlay input is focused. - The guard matches the established Streamlit convention (
inputUtils.isEnterKeyPressed), including the legacykeyCode === 229fallback for browsers/platforms that don't setisComposingreliably. - Two reviewers explicitly noted that guarding at the shared overlay-editor layer is slightly broader than the
NumberColumn-specific symptom, and all agreed this broader scope is correct — a composing Enter should not commit any overlay-backed editor, matching browser IME semantics. - Patch wiring follows the
color2kprecedent: theresolutionskey matches the consumer descriptor, and the lock entry is regenerated with a patch checksum. TheNOTICESadditions (react-number-format,@linaria/react) are auto-generated license attribution for packages already transitive underglide-data-grid, not newly introduced dependencies.
Test Coverage
Unanimous agreement that coverage is adequate for the change size. A dedicated e2e regression test (test_number_cell_editor_ignores_ime_composing_enter) is added to the existing data-editor test file, following the E2E guidance to extend existing files rather than create new ones. It includes both the positive assertion (a plain Enter still commits/closes the cell) and the anti-regression negative assertion (a composing Enter must not close the overlay), satisfying the "must NOT happen" guideline.
No frontend unit test is expected here, since the change lives entirely in a vendored dependency patch rather than Streamlit-owned source; the e2e test is the most direct regression protection and is effectively the authoritative runtime verification that the patch is applied. Note that make check alone does not run e2e tests, so the CI e2e run must pass to confirm the patch takes effect at runtime. See inline comments for optional test-fidelity improvements (multi-digit reproduction and covering the keyCode === 229 fallback branch).
Backwards Compatibility
Fully backwards compatible, per all reviewers. This is a pure bugfix with no public API, protobuf, or config change. The only behavioral delta is that keystrokes during an active IME composition no longer commit/close the cell editor — the correct behavior for all column types (it also improves CJK text entry in text cells). Non-composing Enter behavior is unchanged.
Security & Risk
No security concerns identified by any reviewer. The change is a client-side keyboard-handling guard in a vendored grid component; it does not touch WebSocket/session handling, endpoints, auth, uploads, asset serving, cookies/XSRF, CORS, security headers, iframe/postMessage, or introduce runtime JS execution. No new runtime dependencies are added (the resolutions entry re-registers the already-present glide-data-grid version with a patch). Regression risk is low and contained to grid cell editing.
The primary residual risk (raised by two reviewers) is maintenance: because the fix lives in a vendored yarn patch pinned to the exact version @glideapps/glide-data-grid@npm:6.0.4-alpha24, a future dependency bump could silently make the patch a no-op with no error. See Recommendations.
External test recommendation
- Recommend
external_test: No (unanimous across all three reviewers) - Triggered categories: None
- Evidence:
frontend/.yarn/patches/@glideapps-glide-data-grid-npm-6.0.4-alpha24-9c1a40557b.patch: client-sideonKeyDownguard in the grid overlay editor; browser-native IME behavior, identical in top-level and embedded contexts.frontend/package.json/frontend/yarn.lock/NOTICES: dependency-patch wiring and license attribution only; no hosting, routing, or cross-origin behavior changes.e2e_playwright/st_data_editor_editing_test.py: local UI keyboard interaction test; no routing, auth, transport, embedding, asset, storage, or header dependency.
- Suggested
external_testfocus areas: None — no externally-hosted or embedded boundary behavior is affected. - Confidence: High
- Assumptions and gaps: Assumes IME composition and keyboard event handling behave identically across top-level and iframe-hosted deployments, which holds for browser-native input handling. Reviews were diff-based; the e2e run is the runtime confirmation.
Accessibility
Positive impact, per all reviewers. The fix makes IME-based (CJK) numeric input in st.data_editor actually usable, which is an accessibility and internationalization improvement for keyboard input. No focus, ARIA, or semantic-HTML behavior is changed, and no a11y regressions are introduced.
Readability
- Code comments / docstrings / naming — Consensus that these are clear and exemplary. The patch comment leads with intent ("Ignore keys that are part of an in-progress IME composition") and references
streamlit/streamlit#16129; the e2e docstring and the test nametest_number_cell_editor_ignores_ime_composing_enterstand on their own. No code rewrites required. A minor comment-wording nitpick was raised by one reviewer but is not worth acting on given the comments are already clear. - PR title & description — Reviewers disagreed here (non-blocking):
- Location: title
[bugfix] Preserve IME composition in data_editor NumberColumn (#16129). One reviewer found it acceptable; two suggested improvement. One recommended dropping the trailing issue number and using the documented[fix] Descriptionform (e.g.[fix] Preserve IME composition in data_editor NumberColumn); another suggested foregrounding the user-visible symptom. - Location: description. It uses custom headings (
## Summary,## Repro,## Fix,## Verification) rather than the standard Streamlit template sections (## Describe your changes,## GitHub Issue Link,## Testing Plan) — confirmed against the current PR body. Two reviewers recommended aligning with the template and tightening the (accurate but mechanical) prose; one considered it fine as-is. - Resolution: These are style/consistency suggestions with no functional impact. Aligning the description with the standard template is a reasonable, low-effort improvement, but it is explicitly non-blocking.
- Location: title
Recommendations
- Track upstreaming the fix. The
resolutionskey is pinned to the exact version@glideapps/glide-data-grid@npm:6.0.4-alpha24; a future dependency bump will silently make the patch a no-op without any error. File/link an upstream issue and add a reminder to re-apply or drop the patch on the nextglide-data-gridupgrade (same maintenance caveat as the existingcolor2kpatch). (Raised by 2 of 3 reviewers.) - Ensure the new e2e test runs in CI for this PR, since it is the only check that verifies the vendored patch is actually applied at runtime.
- Optional: align the PR title/description with the standard Streamlit template. (Non-blocking; 2 of 3 reviewers.)
- Optional: see inline comments for test-fidelity improvements (reproduce the multi-digit scenario and cover the
keyCode === 229fallback branch).
Verdict
APPROVED: A minimal, correct, well-documented bugfix that follows established repo patterns (patch via resolutions, IME guard mirroring inputUtils) and adds an appropriate regression e2e test; all three reviewers approved, and only non-blocking maintenance, PR-metadata, and test-fidelity suggestions remain.
This is a consolidated automated AI review produced by claude-opus-4-8-thinking-xhigh, synthesizing reviews from gpt-5.4-xhigh, gemini-3.1-pro, and claude-opus-4-8-thinking-xhigh. All three expected reviews were present and accounted for. Please verify the feedback and use your judgment.
This review also includes 2 inline comment(s) on specific code lines.
|
|
||
| click_on_cell(cell_editor, 1, 0, double_click=True, column_width="medium") | ||
| cell_overlay = get_open_cell_overlay(app) | ||
| input_field = cell_overlay.locator(".gdg-input") |
There was a problem hiding this comment.
suggestion: #16129 is about multi-digit IME input, but this test dispatches a composing Enter into an empty edit. Consider filling a partial value first (e.g. input_field.fill("12")) and asserting it survives the composing Enter (expect(input_field).to_have_value("12")) to reproduce the reported scenario more faithfully. Note also that keyCode/which passed to the KeyboardEvent constructor are ignored by Chromium (read back as 0), so this only exercises the isComposing === true branch of the guard, not the keyCode === 229 fallback.
| which: 229, | ||
| bubbles: true, | ||
| cancelable: true, | ||
| isComposing: true, |
There was a problem hiding this comment.
suggestion: Consider adding a second synthetic keydown with keyCode=229 and isComposing=false so the test also protects the keyCode === 229 fallback branch relied on by some browser/IME combinations.
The performance-workflow flake (Enter after fill committing the previous value) is a test-harness timing artifact: Playwright's atomic fill+Enter can outrun React's re-render, so glide's deferred overlay commit closes over the stale value. Real typing never hits this sub-frame window. Fix it in the shared number-cell e2e helper by yielding one event-loop task after fill so the re-render flushes before Enter, and revert the glide-data-grid patch hunk that forced commits to use the latest value. Only the IME composing-Enter guard from #16165 remains in the patch, keeping the vendored patch minimal and avoiding further divergence from upstream.
3 participants
Summary
Fixes #16129.
The bundled
@glideapps/glide-data-gridoverlay editor treated the Enter that confirms a CJK IME composition (fired withKeyboardEvent.isComposing === true/keyCode === 229) as a cell-commit, closing thest.data_editorNumberColumn cell editor after the first composed digit and making multi-digit input via a Japanese/Korean IME impossible.Repro
Open an
st.data_editorNumberColumn cell, switch to a Japanese/Korean IME, type multiple digits — the editor commits and closes on the composition-Enter before the composed value is finalised.Fix
frontend/.yarn/patches/@glideapps-glide-data-grid-npm-6.0.4-alpha24-9c1a40557b.patch: bail out of the overlay editor'sonKeyDownwhenevent.nativeEvent.isComposing === true(orevent.keyCode === 229). Mirrors the guard Streamlit already applies ininputUtils.isEnterKeyPressedand the fix previously landed forst.chat_inputin st.chat_input element does not behave as expected for Japanese input #6988. Patch is applied via the existing yarnresolutionspattern (same mechanism used forcolor2k).frontend/package.json/frontend/yarn.lock: register the patch viaresolutions.e2e_playwright/st_data_editor_editing_test.py: addtest_number_cell_editor_ignores_ime_composing_enter, which dispatches a composing Enter into the open cell overlay and asserts the editor stays visible, then confirms a plain Enter still commits.Verification
make checkpassesNote
Low Risk
Narrow keyboard-handling change in a vendored dependency patch; normal Enter commit behavior is covered by the new e2e test.
Overview
Fixes #16129:
st.data_editorNumberColumn cells closed on the Enter that confirms a CJK IME composition, so users could not enter multi-digit numbers with a Japanese/Korean IME.A yarn patch on
@glideapps/glide-data-gridmakes the overlay editor’sonKeyDownreturn early whennativeEvent.isComposingis true orkeyCode === 229, so composition Enter no longer commits the cell (aligned with Streamlit’sinputUtils.isEnterKeyPressed/st.chat_input). The patch is wired throughfrontend/package.jsonresolutions andyarn.lock; NOTICES picks up related transitive license entries.Playwright adds
test_number_cell_editor_ignores_ime_composing_enter, which dispatches a composing Enter and asserts the overlay stays open until a normal Enter commits.Reviewed by Cursor Bugbot for commit e0c3f42. Bugbot is set up for automated code reviews on this repo. Configure here.