Studio: IME / multilingual composer regression test + RTL dir="auto"#5485
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves support for right-to-left (RTL) languages by adding the dir="auto" attribute to composer inputs in the frontend. It also introduces a comprehensive Playwright test suite, playwright_chat_ime_i18n.py, designed to catch regressions in IME composition and multilingual text handling. Review feedback suggests refactoring event listener attachments to ensure they persist after page recovery in tests, adhering to PEP 8 by removing spaces around keyword argument assignments, and avoiding silent exception handling in console log collection.
| page, | ||
| ctx, | ||
| default_timeout_ms = 60_000, | ||
| info = lambda m: print(f"[ime] recovery: {m}", flush = True), | ||
| ) | ||
| if form_err is not None: |
There was a problem hiding this comment.
When recover_or_replace_page replaces the page object (e.g., after a crash or navigation failure), the event listeners for pageerror and console attached at lines 164 and 175 are lost. This means subsequent errors on the recovered page will not be captured. Consider refactoring the listener attachment into a helper function and calling it both after initial page creation and after recovery.
References
- Centralize recurring or complex logical checks into a single helper function and reuse it across the codebase to ensure consistency and simplify maintenance.
| NEW = os.environ["STUDIO_NEW_PW"] | ||
| ART_DIR = os.environ.get("PW_ART_DIR", "logs/playwright_ime") | ||
| ART = Path(ART_DIR) | ||
| ART.mkdir(parents = True, exist_ok = True) |
There was a problem hiding this comment.
Adhere to the PEP 8 style guide by avoiding spaces around the = sign for keyword arguments. This is a consistent pattern throughout the file that should be corrected for better adherence to Python standards.
| ART.mkdir(parents = True, exist_ok = True) | |
| ART.mkdir(parents=True, exist_ok=True) |
References
- Avoid spaces around the = sign when used to indicate a keyword argument. (link)
- Prioritize consistency with established coding patterns within a module.
| except Exception: | ||
| return |
There was a problem hiding this comment.
Avoid using broad, silent exception handlers. Instead, log the exception (even at a debug level) to aid in future debugging if the console message property access fails.
| except Exception: | |
| return | |
| except Exception as e: | |
| info(f"Error collecting console message: {e}") | |
| return |
References
- Avoid using broad, silent exception handlers like
except Exception: pass. Instead, log the exception, even if at a debug level, to aid in future debugging.
danielhanchen
added
the
auto-addresses-issue
|
This PR appears to address open issue(s). The duplicate detector matched the following open issues with HIGH confidence:
If this PR fixes any of them, consider adding |
danielhanchen
force-pushed
the
studio/ime-i18n-regression
branch
from
052dd41 to
61dab76
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61dab767d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "unsloth[intelgputorch271]" | ||
| ] | ||
| intelgputorch291 = [ | ||
| intelgputorch210 = [ |
There was a problem hiding this comment.
This hunk drops all of the Intel XPU optional-dependency groups for torch 2.7.1, 2.9.1, 2.11.0, and 2.12.0 (plus their intel-gpu-* aliases), even though the change is only about Studio IME coverage. After this, users installing those advertised variants such as unsloth[intel-gpu-torch291] or unsloth[intel-gpu-torch2120] will no longer get the pinned torch/triton/torchvision XPU wheels and will fall back to the base package, breaking those Intel configurations.
Useful? React with 👍 / 👎.
danielhanchen
added
auto-approved
|
Auto-review verdict: Approved Adds dir="auto" to the three Studio chat composer textareas so Arabic/Hebrew/Persian/Urdu input flows right-to-left, and wires a model-free Playwright smoke (multilingual paste + stuck-IME composition repro for issue #5318) into the Studio UI CI job. Useful because it both fixes a user-visible RTL bug and locks down the previously-fixed stuck-composition regression that backend/frontend CI both missed. Reason: All real review findings fixed; PR adds correct RTL bidi support and a regression smoke for a real upstream IME bug, with no remaining defects. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba4f1c0918
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| - name: Drive IME + multilingual paste regression with Playwright | ||
| env: | ||
| BASE_URL: http://127.0.0.1:18896 | ||
| STUDIO_OLD_PW: ${{ env.STUDIO_IME_OLD_PW }} |
There was a problem hiding this comment.
The IME smoke script only reads BASE_URL and STUDIO_NEW_PW, but this step still injects STUDIO_OLD_PW; the newly added guard test (test_ime_workflow_step_does_not_set_studio_old_pw) explicitly asserts that this variable is absent, so the test suite now fails immediately on this mismatch. Dropping this env var is required to keep the new regression checks green.
Useful? React with 👍 / 👎.
| NEW="CIIme-$(python -c 'import secrets; print(secrets.token_urlsafe(16))')" | ||
| echo "::add-mask::$OLD" | ||
| echo "::add-mask::$NEW" | ||
| echo "STUDIO_IME_OLD_PW=$OLD" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
This workflow still exports STUDIO_IME_OLD_PW even though the new IME path no longer consumes it, and the new test (test_ime_pass_password_step_does_not_export_old_pw) now fails because of this exact line. Keeping the obsolete export breaks the regression test contract introduced in the same commit.
Useful? React with 👍 / 👎.
danielhanchen
force-pushed
the
studio/ime-i18n-regression
branch
from
f89359d to
f4be9b6
Compare
|
Force-pushed a clean rebuild on top of current main. The previous tip had drifted across iterations and ended up reverting #4931 (intelgputorch271 extras), #5477 (studio/frontend dep cleanup), and #5478 (deterministic dep-removal check), because the staging working copy was based on a snapshot of main that predated those merges. The final diff now stops at the five files this PR actually owns:
No source edits beyond the dir="auto" lines and the new CI step. |
Adds dir="auto" to the main, edit, and compare chat composers so RTL scripts (Arabic, Hebrew, Persian, Urdu) flow right to left without forcing the rest of the UI into RTL. Wires a model-free Playwright smoke (multilingual paste round trip across 31 scripts + a stuck-IME composition repro for issue #5318 / PR #5327) into the Studio UI CI job as a third Studio boot, plus a pure-Python static-guard test that locks down dir="auto" on all three composers and the minimal env contract for the smoke.
danielhanchen
force-pushed
the
studio/ime-i18n-regression
branch
from
f4be9b6 to
a8772e1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8772e185a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| page = recover_or_replace_page( | ||
| page, | ||
| ctx, | ||
| default_timeout_ms = 60_000, | ||
| info = lambda m: print(f"[ime] recovery: {m}", flush = True), |
There was a problem hiding this comment.
If the first composer wait times out or the renderer/page is replaced, this recovery call does not pass goto_url=BASE, so recover_or_replace_page can return a fresh blank page (or leave the page on a stale route) and the second attempt just waits for the textarea in the wrong document. The existing chat and extra UI scripts use goto_url=BASE for this exact mount-recovery path; without it the new IME smoke can fail instead of recovering from the transient mount/renderer issues it is trying to tolerate.
Useful? React with 👍 / 👎.
1 participant
Summary
tests/studio/playwright_chat_ime_i18n.py: a Playwright smoke that exercises the chat composer textarea across 31 scripts (covering more than 90 percent of the world's population by speaker count) and reproduces the issue [Bug] Japanese IME activation makes the chat input field unable to accept any text #5318 stuck-composition pattern..github/workflows/studio-ui-smoke.ymlas a third Studio boot on port 18896.dir="auto"to the main chat composer, the inline edit composer, and the compare-mode composer so RTL scripts (Arabic, Hebrew, Persian, Urdu) flow right to left without forcing the whole UI into RTL.Why
@assistant-ui/reactfloated from0.12.19to0.12.28(Fix Studio chat history and attachments with newer assistant-ui #5296) and surfaced a library bug; the existing chat / extra UI smokes did not exercise composition events.dir="auto"is a one-line opt-in to Unicode bidi auto-detection. Without it, typing Arabic / Hebrew / Persian / Urdu shows correct characters but the cursor, punctuation, and line wrap flow left to right, which is jarring for RTL users.What the smoke checks
dir="auto".你好.compositionstartevents with no matchingcompositionend, then verify the field still acceptsabcand a follow-up keystroked, and that the Send button is not stuck disabled.Cost
Model-free on purpose, since the bug surface is the React composer rather than inference. About 60 seconds of wall clock on a warm runner, plus the third Studio boot (which is fast because the install is already warm from the earlier steps in the same job).
Verified locally
main(HEAD with fix: harden Studio IME composer sends #5327):ascii=OK paste=31/31 normal_composition=OK stuck_recovery=OK, zero page errors, zero console errors.b65a7450): paste 31/31 still passes (paste path is unaffected), normal composition step FAILS withreadback '' missing '你好'exactly as expected for the regression we want to catch.Test plan
ubuntu-latest.tests/studio/playwright_chat_ime_i18n.pystep produces screenshots underlogs/playwright_ime/.mainbuilds.