Migrate Text Editing to rust backend#4316
Conversation
|
@Annonnymmousss I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
Code Review
This pull request refactors the text tool to handle text editing, cursor movement, selection, and history (undo/redo) directly in the Rust backend instead of relying on a frontend HTML text input overlay. It introduces double-click and triple-click support, cursor blinking, and text selection rendering using the Parley layout library. The review feedback highlights critical areas for improvement, including supporting internationalization (IMEs) by avoiding direct keydown-to-text mapping, handling non-invertible transforms to prevent NaN-induced panics, and safely accessing the global window object to avoid crashes in non-browser environments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
11 issues found across 17 files
Confidence score: 2/5
- In
frontend/src/utility-functions/input.ts, the newkeydownpath callspreventDefault()but only forwards simple unmodified printable keys, so arrows/delete/shortcuts and IME-like flows can be dropped and text editing can feel broken as-is — only suppress native handling when a full replacement path exists, or forward the missing key classes before merging. - In
editor/src/messages/input_mapper/input_mappings.rs, the new mappings are inconsistent (document-level vertical selection modifiers don’t mirror non-selection behavior, andAccel+Backspacemaps to full-line delete on non-macOS), which can cause unexpected or destructive editing actions for users — align selection/non-selection modifier mappings and restore platform-expected word-delete behavior before merge. - In
editor/src/messages/tool/tool_messages/text_tool.rs,DoubleClickstarts a transaction that is never ended andCommitTextcan silently exit whenget_text_idisNone, risking stuck transaction state and silent text-loss/confusing UX — ensure every edit exit path emitsEndTransactionand report/recover when the text layer disappears. - In
editor/src/messages/tool/tool_messages/text_tool.rsandnode-graph/nodes/text/src/text_context.rs, non-invertible transforms can propagate NaN positions that bypass clamping, which can lead to invalid hit-test/cursor coordinates and unstable text interactions — add finite-value guards and early fallback behavior when matrix inversion or coordinate validity fails.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
|
|
||
| fn spawn_blink_timer(tick: u64, responses: &mut VecDeque<Message>) { | ||
| responses.add(FutureMessage::Await { |
There was a problem hiding this comment.
you can do this
responses.add(async {});There was a problem hiding this comment.
Oh okayy. Thanks.
There was a problem hiding this comment.
@TrueDoctor We could likely add a general api for doing this, remember when we talked about that when working on the future message handler? This is also kinda bad for tests. And in general a foot gun that easily leads to race conditions (kinda funny in a serial message passing systems).
There was a problem hiding this comment.
This looks like a very wrong approach. I would prefer if we hooked this into animation frame message loop but this can't stay as is
No description provided.