Repository navigation
docs(ime): codify desktop composition regression checks - #17172
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Merge Risk: 🔵 Low · up to This documentation-only PR adds IME regression requirements without changing runtime behavior. It is mergeable with owner follow-up to clarify bounded state and cleanup rules, cover every attachment path and duplicate settlement, and confirm local PTY, preview-terminal, and folder-workspace verification; these gaps could leave future fixes under-specified but do not create a current production behavior change. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) Full details: Description checkExplanation The description is complete in substance. It explains the change, purpose, linked issues, visual-proof exception, verification, AI use, scope, and review considerations. It provides equivalent testing and review information, although it does not use the exact 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. Comment |
Greptile SummaryThe PR adds a durable IME regression checklist and permits that reference document through the repository’s documentation ignore policy.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| .gitignore | Allowlists the new IME checklist while preserving the existing documentation ignore policy. |
| docs/reference/ime-regression-checklist.md | Adds the IME regression contracts and now defines all remote-evidence states with explicit host-owned evidence requirements. |
Reviews (9): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…ression-checklist
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/reference/ime-regression-checklist.md (1)
36-40: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake all required native contexts explicit.
The native matrix covers macOS, Windows, and Linux/SSH. It does not explicitly require native evidence for local PTYs, preview terminals, or folder workspaces. Lines 62-63 require paired tests, but those tests are not native evidence. Add explicit rows or context columns for every environment named in the PR objective.
🧹 Nitpick comments (2)
docs/reference/ime-regression-checklist.md (2)
23-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the composition settlement event explicitly.
The heading and attachment section use “settlement” for asynchronous attachment resolution. These lines use the same term for the
compositionend/blurtransition. State that only the first composition-settlement event adopts the browser DOM and flushes the queue. Attachment completions must only enqueue while composition is active.
60-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine the per-composition bound.
The gate requires bounded state and work, but it defines no limit or overflow behavior. Add a measurable bound and a deterministic cancellation or rejection rule. Reviewers must be able to verify this requirement without inferring the limit from implementation details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5c5e709f-f518-42d4-9e5c-78208f09c08f
📒 Files selected for processing (1)
docs/reference/ime-regression-checklist.md
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/reference/ime-regression-checklist.md (2)
25-27: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover all declared attachment producers.
The contract names local selection, pasted-image temp saves, and SSH uploads. The cases below exercise only SSH uploads. Add local-selection and pasted-image cases with the same ordering, duplicate, disable, and blur assertions.
29-30: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest duplicate settlement separately from duplicate paths.
Returning one path twice checks duplicate payload values. It does not check the duplicate-settlement failure described in the issue ledger, where the same completion is processed twice and stale DOM is re-adopted. Deliver one completion twice and assert that DOM adoption and queue flushing occur once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a3f091a-7345-4962-859c-d5549bbc86bc
📒 Files selected for processing (1)
docs/reference/ime-regression-checklist.md
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.
|
|
||
| Every transient collection and ownership tracker must have an explicit lifetime and bound: | ||
|
|
||
| - Native Chat uses `NATIVE_FILE_DROP_MAX_PATHS` (`256`). If a resolved completion would cross the cap, the whole batch is rejected atomically and the overflow notice remains visible through settlement; accepted paths keep order and duplicates. The queue is cleared before re-entry and on disable or pane-owner remount. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Make attachment work owner-scoped and bounded.
Clearing the queue does not invalidate an in-flight completion. A local, pasted-image, or SSH result can resolve after re-entry and enter the new owner’s queue. Define a bound for pending attachment work and discard results from older composition or pane-owner generations before enqueueing.
Proposed contract update
- The queue is cleared before re-entry and on disable or pane-owner remount.
+ Pending attachment work and queued semantic paths are bounded per owner.
+ Results from an older composition or pane-owner generation are discarded.
+ The queue is cleared before re-entry and on disable or pane-owner remount.| Every transient collection and ownership tracker must have an explicit lifetime and bound: | ||
|
|
||
| - Native Chat uses `NATIVE_FILE_DROP_MAX_PATHS` (`256`). If a resolved completion would cross the cap, the whole batch is rejected atomically and the overflow notice remains visible through settlement; accepted paths keep order and duplicates. The queue is cleared before re-entry and on disable or pane-owner remount. | ||
| - The terminal placeholder mask tracks one scalar `activeSessionId` because xterm renders one composition view. A newer start supersedes an older one, a stale end cannot clear the latest owner, and blur or disposal clears it. The composition route keeps its per-ID reference-counted map intentionally for transport ownership; it is not replaced by the scalar. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include compositionend in placeholder-mask cleanup.
Line 20 lists blur and disposal, but not normal compositionend. The issue ledger requires end, blur, and disposal to clear ownership, class, and listeners. Add compositionend here so a completed composition cannot leave the placeholder mask active.
Proposed contract update
- a stale end cannot clear the latest owner, and blur or disposal clears it.
+ a stale end cannot clear the latest owner, and compositionend, blur, or disposal clears it.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - The terminal placeholder mask tracks one scalar `activeSessionId` because xterm renders one composition view. A newer start supersedes an older one, a stale end cannot clear the latest owner, and blur or disposal clears it. The composition route keeps its per-ID reference-counted map intentionally for transport ownership; it is not replaced by the scalar. | |
| - The terminal placeholder mask tracks one scalar `activeSessionId` because xterm renders one composition view. A newer start supersedes an older one, a stale end cannot clear the latest owner, and compositionend, blur, or disposal clears it. The composition route keeps its per-ID reference-counted map intentionally for transport ownership; it is not replaced by the scalar. |
|
|
||
| - Native Chat uses `NATIVE_FILE_DROP_MAX_PATHS` (`256`). If a resolved completion would cross the cap, the whole batch is rejected atomically and the overflow notice remains visible through settlement; accepted paths keep order and duplicates. The queue is cleared before re-entry and on disable or pane-owner remount. | ||
| - The terminal placeholder mask tracks one scalar `activeSessionId` because xterm renders one composition view. A newer start supersedes an older one, a stale end cannot clear the latest owner, and blur or disposal clears it. The composition route keeps its per-ID reference-counted map intentionally for transport ownership; it is not replaced by the scalar. | ||
| - Typing diagnostics cap pending and ignored echo candidates at `MAX_PENDING_ECHO_CANDIDATES` (`64`), cap pending user-input signals, drain timed-out candidates, and clear all series on pane detach. Overflow becomes an explicitly ambiguous burst rather than an arbitrary attribution. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Specify the pending user-input signal bound.
Line 17 requires an explicit bound for every transient collection. MAX_PENDING_ECHO_CANDIDATES bounds echo candidates, but pending user-input signals has no limit, identifier, or overflow behavior. Define those values and require the same ambiguous-burst handling at overflow.
|
Fresh CI on head |
* docs(ime): codify desktop composition regression checks * docs(ime): define remote verification verdicts * docs(ime): narrow placeholder masking invariant * docs(ime): require final-cell caret containment * docs(ime): cover async attachment settlement * docs(ime): correct semantic placeholder contract * docs(ime): record bounded ownership contracts
ELI5
This gives future IME fixes one durable checklist: what each regression means, which component owns provisional text, how async attachments must settle, what evidence is required, and what must be checked on macOS, Windows, and Linux/SSH.
What changed
live/unverifiable/exitedvocabulary for remote hosts.Why
Synthetic regression tests and native input methods prove different things. Capturing both layers prevents a future fix from solving one machine's symptom while regressing browser, renderer, PTY, remote-host, attachment, or ordinary mid-line behavior.
Linked issues
Refs #16911
Refs #16949
Refs #16950
Refs #17104
Visual proof
N/A — documentation-only change with no rendered Orca UI surface.
Exact-head verification
Published head:
0f5d797d0f4831093e65e3e7df6b8e7ce388d2b3mainnormally before verification.pnpm exec oxfmt --check docs/reference/ime-regression-checklist.md: passed.pnpm tc: passed.pnpm run check:code-quality:changed: passed; no changed JavaScript or TypeScript.git diff --check: passed.git ls-files --error-unmatch docs/reference/ime-regression-checklist.md: passed.origin/main: one.gitignoreallowlist and one reference document; no runtime code.Readiness and code elegance
docs/reference/structure and adds no parallel checklist file.AI disclosure
AI-assisted editing and review were used. All claims are tied to the committed document and exact-head gates above.
Agent skill upstream boundary
Notes
Implementation PRs:
This PR is technically independent but should merge after those code PRs so its issue ledger describes landed behavior.
Checklist
33293935966completed cleanly: 16 successful, 10 skipped, 0 failed or pending (includingverify); the prior failures were the shared happy-dom baseline, not documentation assertions.