Merged
Conversation
…tion - Added a new section on multi-tenancy, detailing host-based tenant routing and entry points. - Included specifics on tenant domains, host classes, routing behavior, and local development setup. - Clarified the role of `proxy.ts` in managing Supabase auth and tenant routing.
- Added a new text editing module with core functionalities including text insertion, deletion, and cursor navigation. - Introduced a `SimpleLayoutEngine` for deterministic text layout in tests, supporting line metrics and cursor positioning. - Created a minimal plain-text editor example using `winit` and `Skia`. - Updated dependencies in `Cargo.toml` to include `arboard` for clipboard functionality and other necessary libraries. - Enhanced documentation for text editing features and layout options.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
WalkthroughAdds a new workspace crate Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant App as TextEditorApp
participant Editor as apply_command()
participant Layout as TextLayoutEngine
participant History as EditHistory
User->>App: Input (keyboard/mouse/IME)
App->>App: Build EditingCommand
App->>Editor: apply_command(state, command, Layout)
alt pure edit
Editor->>Editor: Apply pure edit (insert/delete/etc.)
else layout-dependent
Editor->>Layout: Query line_metrics / position_at_point / caret_rect
Layout-->>Editor: Metrics / positions
Editor->>Editor: Apply layout-aware edit
end
Editor-->>App: New TextEditorState
App->>History: push(old_state, edit_kind)
History->>History: Merge if mergeable & within timeout
App->>Layout: Rebuild metrics
App->>App: Render frame (text, caret, selection)
App->>User: Display updated editor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
…ands - Introduced `EditHistory` to manage undo and redo operations with merge capabilities for consecutive edits. - Added new editing commands: `BackspaceWord`, `DeleteWord`, `BackspaceLine`, and `DeleteLine` for improved text manipulation. - Updated `apply_command` function to handle new commands and integrate with the history management. - Enhanced tests to cover undo/redo scenarios and command classification for better reliability.
- Updated the `cursor_baseline_y` function to simplify the calculation of the cursor's baseline, removing unnecessary extrapolation for trailing newlines. - Introduced `ime_suppress_next_key` flag in `TextEditorApp` to prevent double input when an IME commit occurs, ensuring smoother text input handling.
- Updated the `update_preedit` method to always set `preedit` to `Some(text)`, ensuring consistent preedit state. - Modified key event handling to allow text insertion for Enter, Space, and Tab keys only when `preedit` is None, improving user experience. - Added logic to clear empty preedit states after IME events, preventing input issues during text editing.
- Updated key event handling to utilize `PhysicalKey` and `KeyCode` for improved accuracy in detecting key presses. - Simplified logic for handling text insertion and command execution based on physical key codes, enhancing user experience during text editing. - Ensured consistent behavior for key commands such as select all, copy, cut, and paste.
…t logic - Introduced `floor_char_boundary` and `ceil_char_boundary` functions to ensure safe adjustments of raw byte offsets in multi-byte text. - Updated cursor movement commands to skip whitespace when navigating between words, enhancing text editing experience. - Enhanced tests to validate new boundary functions and cursor movement behavior, ensuring robustness in text manipulation.
- Added checks for empty line metrics before accessing line indices to prevent potential panics during text editing operations. - Improved the logic for draining text based on cursor position, ensuring more robust handling of text deletions in the editor.
- Moved `SkiaLayoutEngine` implementation to a dedicated module for better structure and maintainability. - Introduced `CaretRect` struct to encapsulate caret geometry, improving clarity in caret positioning logic. - Updated `TextLayoutEngine` trait to include `caret_rect_at` method, enhancing caret management across layout engines. - Refined cursor movement logic to ensure consistent behavior when navigating through text and lines.
…bilities - Added a new `grida-text-edit` crate to provide a platform-agnostic text editing engine. - Implemented core functionalities including text insertion, deletion, and cursor navigation. - Integrated `SkiaLayoutEngine` for advanced text layout and rendering. - Established a minimal plain-text editor example using `winit` and Skia for demonstration. - Updated `Cargo.toml` to include necessary dependencies for the new module.
![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: fce0bc74d8
ℹ️ 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".
Comment on lines
+118
to
+121
| self.undo_stack.push(HistoryEntry { | ||
| state: current.clone(), | ||
| kind: entry.kind, | ||
| timestamp: Instant::now(), |
There was a problem hiding this comment.
When redo restores a state, it pushes current back onto undo_stack with the same mergeable kind and a fresh timestamp, so the next push of the same edit kind can merge across the redo boundary. In practice, redoing a typing run and then typing again within the timeout can skip recording the post-redo state, so one undo jumps too far (e.g., from abcd back to the pre-run state instead of abc).
Useful? React with 👍 / 👎.
Comment on lines
+197
to
+199
| let mut provider = TypefaceFontProvider::new(); | ||
| provider.register_typeface(tf, Some(family)); | ||
| self.font_collection.set_asset_font_manager(Some(provider.into())); |
There was a problem hiding this comment.
add_font_bytes allocates a new TypefaceFontProvider on each call and immediately installs it as the asset font manager, which replaces all fonts registered earlier via this API. Hosts that register multiple families for fallback will silently lose earlier families and only retain the last one, causing incorrect shaping/fallback on mixed-script text.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
crates/grida-text-edit/examples/wd_text_editor.rs (1)
70-97: Consider moving this windowing example tocrates/grida-dev.The
grida-text-editcrate's core library (insrc/) is platform-agnostic, as documented inskia_layout.rs. The example atexamples/wd_text_editor.rsintroduceswinit/glutincoupling; relocating it togrida-devwould align with the established pattern (which already hosts similar windowed examples likebench_cache_picture.rs).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-text-edit/examples/wd_text_editor.rs` around lines 70 - 97, Move the windowing example out of the grida-text-edit crate and into the grida-dev crate: identify the example file (examples/wd_text_editor.rs) and any symbols that couple it to platform/windowing (imports like glutin_winit::DisplayBuilder, glutin::{config::ConfigTemplateBuilder, context::ContextAttributesBuilder}, winit::event_loop::EventLoop and winit::window::Window/WindowAttributes, and skia_safe GPU surface wrappers such as backend_render_targets/wrap_backend_render_target) and relocate the whole example file into the grida-dev crate’s examples directory, update its Cargo.toml entry if needed, and remove or replace the moved example from grida-text-edit so the core crate remains platform-agnostic.crates/grida-text-edit/src/lib.rs (1)
731-736: Avoid duplicating line-index logic across two public helpers.
line_index_for_offset_utf8duplicateslayout::line_index_for_offset. Delegating to one implementation reduces drift risk.♻️ Proposed refactor
pub fn line_index_for_offset_utf8(metrics: &[LineMetrics], utf8_offset: usize) -> usize { - metrics - .iter() - .position(|lm| utf8_offset < lm.end_index) - .unwrap_or(metrics.len().saturating_sub(1)) + line_index_for_offset(metrics, utf8_offset) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-text-edit/src/lib.rs` around lines 731 - 736, The function line_index_for_offset_utf8 duplicates the logic in layout::line_index_for_offset; replace its body with a direct call/delegation to layout::line_index_for_offset (passing the same metrics slice and utf8_offset) so there is a single source of truth for computing the line index for a UTF-8 offset; ensure the public signature (pub fn line_index_for_offset_utf8(metrics: &[LineMetrics], utf8_offset: usize) -> usize) and return behavior remain unchanged and import or qualify layout::line_index_for_offset as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/grida-text-edit/Cargo.toml`:
- Line 4: Update the crate manifest's edition field in Cargo.toml from "2021" to
"2024": locate the edition = "2021" entry and change it to edition = "2024" so
the new crate uses the Rust 2024 edition as required by repo policy.
In `@crates/grida-text-edit/src/layout.rs`:
- Around line 20-23: is_empty_line currently treats any 1-byte range as empty
(end_index - start_index <= 1), misclassifying single-character lines like "a";
change is_empty_line to accept the full text (e.g., pub fn is_empty_line(&self,
text: &str) -> bool) and return true only when the slice corresponding to
start_index..end_index is exactly the newline terminator (e.g., length == 1 and
that byte/char == '\n' or substring == "\n"); update all callsites of
is_empty_line to pass the text argument.
In `@crates/grida-text-edit/src/lib.rs`:
- Around line 214-216: The with_cursor constructor currently accepts cursor
without validation which can later cause panics; update the function (pub fn
with_cursor) to normalize the provided cursor like set_cursor_and_anchor does by
calling snap_grapheme_boundary on the text before storing it, ensuring the
returned Self stores a clamped/valid cursor (and anchor: None) so downstream
methods like drain() and selected_text() cannot panic on invalid offsets.
In `@crates/grida-text-edit/src/simple_layout.rs`:
- Around line 193-205: The selection geometry uses chars().count() which
miscounts multi-codepoint graphemes; replace those counts in the x_lo and x_hi
calculations to use grapheme counts (e.g.
UnicodeSegmentation::graphemes(true).count()) on the same slices (the before
variables derived from text[lm.start_index..overlap_lo] and
text[lm.start_index..overlap_hi] and for the full line_content) and multiply
that grapheme count by self.char_width; also add the necessary use/import for
UnicodeSegmentation so the compilation sees the grapheme method.
In `@crates/grida-text-edit/src/skia_layout.rs`:
- Around line 194-200: The add_font_bytes function currently creates a new
TypefaceFontProvider and calls font_collection.set_asset_font_manager on each
call, which discards previously registered fonts; fix this by adding a cached
collection field (e.g., Vec<(String, Vec<u8>)> or similar) to the struct to
store each (family, bytes) registration, push the new font into that cache
inside add_font_bytes, then rebuild a single TypefaceFontProvider from all
cached entries (iterating the cache and calling provider.register_typeface for
each (family, bytes) pair) and call
self.font_collection.set_asset_font_manager(Some(provider.into())) once with the
rebuilt provider so all fonts are preserved across calls; keep using
FontMgr::new() and loader.new_from_data per cached bytes when constructing
typefaces.
In `@crates/grida-text-edit/src/tests.rs`:
- Around line 57-58: Update the test comment that incorrectly calls 👍🏽 a
"2-codepoint ZWJ sequence" — locate the TextEditorState::with_cursor("a👍🏽b",
9) test in tests.rs and change the comment to correctly describe 👍🏽 as an
emoji plus skin-tone modifier grapheme cluster (e.g., "👍🏽 is an emoji +
skin-tone modifier grapheme cluster (8 UTF-8 bytes),") so the terminology is
accurate.
In `@docs/wg/feat-text-editing/index.md`:
- Around line 200-203: The three multi-click bullet lines repeat "select the
..." phrasing; simplify to a consistent, concise form by rewording each bullet
to remove redundancy—e.g., for "k = 1 (single click)" use "place the caret at p
(collapsed selection)"; for "k = 2 (double click)" use "select the word
containing p (R_word(p))"; for "k = 3 (triple click)" use "select the line
containing p (R_line(p))"; and for "k = 4 (quadruple click)" use "select the
entire editable value (document range)"; update the bullets for k = 1..4
accordingly to improve scanability.
In `@editor/AGENTS.md`:
- Line 26: There is a typo in the docs: replace the misspelled middleware symbol
"TanantMiddleware.routeProxyRequest" with the correct
"TenantMiddleware.routeProxyRequest" in the routing flow description so the
README accurately references the middleware function name.
---
Nitpick comments:
In `@crates/grida-text-edit/examples/wd_text_editor.rs`:
- Around line 70-97: Move the windowing example out of the grida-text-edit crate
and into the grida-dev crate: identify the example file
(examples/wd_text_editor.rs) and any symbols that couple it to
platform/windowing (imports like glutin_winit::DisplayBuilder,
glutin::{config::ConfigTemplateBuilder, context::ContextAttributesBuilder},
winit::event_loop::EventLoop and winit::window::Window/WindowAttributes, and
skia_safe GPU surface wrappers such as
backend_render_targets/wrap_backend_render_target) and relocate the whole
example file into the grida-dev crate’s examples directory, update its
Cargo.toml entry if needed, and remove or replace the moved example from
grida-text-edit so the core crate remains platform-agnostic.
In `@crates/grida-text-edit/src/lib.rs`:
- Around line 731-736: The function line_index_for_offset_utf8 duplicates the
logic in layout::line_index_for_offset; replace its body with a direct
call/delegation to layout::line_index_for_offset (passing the same metrics slice
and utf8_offset) so there is a single source of truth for computing the line
index for a UTF-8 offset; ensure the public signature (pub fn
line_index_for_offset_utf8(metrics: &[LineMetrics], utf8_offset: usize) ->
usize) and return behavior remain unchanged and import or qualify
layout::line_index_for_offset as needed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.tomlcrates/grida-dev/Cargo.tomlcrates/grida-text-edit/Cargo.tomlcrates/grida-text-edit/examples/wd_text_editor.rscrates/grida-text-edit/src/history.rscrates/grida-text-edit/src/layout.rscrates/grida-text-edit/src/lib.rscrates/grida-text-edit/src/simple_layout.rscrates/grida-text-edit/src/skia_layout.rscrates/grida-text-edit/src/tests.rsdocs/wg/feat-text-editing/index.mdeditor/AGENTS.md
Comment on lines
+20
to
+23
| /// Returns `true` when the line contains no glyph content (only a newline terminator). | ||
| pub fn is_empty_line(&self) -> bool { | ||
| self.end_index.saturating_sub(self.start_index) <= 1 | ||
| } |
There was a problem hiding this comment.
is_empty_line misclassifies one-byte content lines as empty.
At Line 22, end_index - start_index <= 1 marks lines like "a" as empty. This breaks cursor/selection behavior on single-character ASCII lines.
🔧 Proposed fix (and update callsites to pass text)
impl LineMetrics {
/// Returns `true` when the line contains no glyph content (only a newline terminator).
- pub fn is_empty_line(&self) -> bool {
- self.end_index.saturating_sub(self.start_index) <= 1
+ pub fn is_empty_line(&self, text: &str) -> bool {
+ if self.start_index >= self.end_index {
+ return true;
+ }
+ self.end_index == self.start_index + 1
+ && text.as_bytes().get(self.start_index) == Some(&b'\n')
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-text-edit/src/layout.rs` around lines 20 - 23, is_empty_line
currently treats any 1-byte range as empty (end_index - start_index <= 1),
misclassifying single-character lines like "a"; change is_empty_line to accept
the full text (e.g., pub fn is_empty_line(&self, text: &str) -> bool) and return
true only when the slice corresponding to start_index..end_index is exactly the
newline terminator (e.g., length == 1 and that byte/char == '\n' or substring ==
"\n"); update all callsites of is_empty_line to pass the text argument.
Comment on lines
+193
to
+205
| let x_lo = if overlap_lo <= lm.start_index { | ||
| 0.0 | ||
| } else { | ||
| let before = &text[lm.start_index..overlap_lo]; | ||
| before.chars().count() as f32 * self.char_width | ||
| }; | ||
|
|
||
| let x_hi = if overlap_hi >= content_end { | ||
| line_content.chars().count() as f32 * self.char_width | ||
| } else { | ||
| let before = &text[lm.start_index..overlap_hi]; | ||
| before.chars().count() as f32 * self.char_width | ||
| }; |
There was a problem hiding this comment.
Use grapheme counts for selection widths to match caret/hit-testing behavior.
Line 197, Line 201, and Line 204 use chars().count(), but this engine’s cursor math is grapheme-based. Multi-codepoint graphemes will produce misaligned selection geometry.
🔧 Proposed fix
let x_lo = if overlap_lo <= lm.start_index {
0.0
} else {
let before = &text[lm.start_index..overlap_lo];
- before.chars().count() as f32 * self.char_width
+ before.graphemes(true).count() as f32 * self.char_width
};
let x_hi = if overlap_hi >= content_end {
- line_content.chars().count() as f32 * self.char_width
+ line_content.graphemes(true).count() as f32 * self.char_width
} else {
let before = &text[lm.start_index..overlap_hi];
- before.chars().count() as f32 * self.char_width
+ before.graphemes(true).count() as f32 * self.char_width
};📝 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.
Suggested change
| let x_lo = if overlap_lo <= lm.start_index { | |
| 0.0 | |
| } else { | |
| let before = &text[lm.start_index..overlap_lo]; | |
| before.chars().count() as f32 * self.char_width | |
| }; | |
| let x_hi = if overlap_hi >= content_end { | |
| line_content.chars().count() as f32 * self.char_width | |
| } else { | |
| let before = &text[lm.start_index..overlap_hi]; | |
| before.chars().count() as f32 * self.char_width | |
| }; | |
| let x_lo = if overlap_lo <= lm.start_index { | |
| 0.0 | |
| } else { | |
| let before = &text[lm.start_index..overlap_lo]; | |
| before.graphemes(true).count() as f32 * self.char_width | |
| }; | |
| let x_hi = if overlap_hi >= content_end { | |
| line_content.graphemes(true).count() as f32 * self.char_width | |
| } else { | |
| let before = &text[lm.start_index..overlap_hi]; | |
| before.graphemes(true).count() as f32 * self.char_width | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-text-edit/src/simple_layout.rs` around lines 193 - 205, The
selection geometry uses chars().count() which miscounts multi-codepoint
graphemes; replace those counts in the x_lo and x_hi calculations to use
grapheme counts (e.g. UnicodeSegmentation::graphemes(true).count()) on the same
slices (the before variables derived from text[lm.start_index..overlap_lo] and
text[lm.start_index..overlap_hi] and for the full line_content) and multiply
that grapheme count by self.char_width; also add the necessary use/import for
UnicodeSegmentation so the compilation sees the grapheme method.
Comment on lines
+194
to
+200
| pub fn add_font_bytes(&mut self, family: &str, bytes: &[u8]) { | ||
| let loader = FontMgr::new(); | ||
| if let Some(tf) = loader.new_from_data(bytes, None) { | ||
| let mut provider = TypefaceFontProvider::new(); | ||
| provider.register_typeface(tf, Some(family)); | ||
| self.font_collection.set_asset_font_manager(Some(provider.into())); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's verify the file exists and examine the code around lines 194-200
wc -l crates/grida-text-edit/src/skia_layout.rsRepository: gridaco/grida
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Examine the code at lines 194-200 and surrounding context
sed -n '180,220p' crates/grida-text-edit/src/skia_layout.rs | cat -nRepository: gridaco/grida
Length of output: 1885
🏁 Script executed:
#!/bin/bash
# Look at the struct definition to understand the data structure
sed -n '1,100p' crates/grida-text-edit/src/skia_layout.rs | cat -nRepository: gridaco/grida
Length of output: 4313
🏁 Script executed:
#!/bin/bash
# Search for all usages of add_font_bytes to understand how it's called
rg -n 'add_font_bytes' crates/grida-text-edit/src/Repository: gridaco/grida
Length of output: 302
🏁 Script executed:
#!/bin/bash
# Check the entire SkiaLayoutEngine struct definition
rg -n 'struct SkiaLayoutEngine' -A 20 crates/grida-text-edit/src/skia_layout.rsRepository: gridaco/grida
Length of output: 868
add_font_bytes replaces the asset manager on each call, losing previously registered fonts.
When called multiple times, each invocation creates a new TypefaceFontProvider and replaces the asset manager via set_asset_font_manager, discarding any prior font registrations. To preserve multi-family font fallback, cache font registrations and rebuild the provider with all accumulated fonts on each call.
Suggested approach
Add a field to cache registered fonts and rebuild the provider with all accumulated registrations:
pub struct SkiaLayoutEngine {
pub font_collection: FontCollection,
pub paragraph: Option<Paragraph>,
pub layout_width: f32,
pub layout_height: f32,
pub font_size: f32,
pub config: TextConfig,
cached_text: String,
+ registered_fonts: Vec<(String, Vec<u8>)>,
}
pub fn new_with_config(layout_width: f32, layout_height: f32, config: TextConfig) -> Self {
let mut fc = FontCollection::new();
fc.set_default_font_manager(FontMgr::new(), None);
let font_size = config.font_size;
Self {
font_collection: fc,
paragraph: None,
layout_width,
layout_height,
font_size,
config,
cached_text: String::new(),
+ registered_fonts: Vec::new(),
}
}
pub fn add_font_bytes(&mut self, family: &str, bytes: &[u8]) {
+ self.registered_fonts.push((family.to_string(), bytes.to_vec()));
let loader = FontMgr::new();
+ let mut provider = TypefaceFontProvider::new();
+ for (fam, data) in &self.registered_fonts {
+ if let Some(tf) = loader.new_from_data(data.as_slice(), None) {
+ provider.register_typeface(tf, Some(fam.as_str()));
+ }
+ }
+ self.font_collection.set_asset_font_manager(Some(provider.into()));
- if let Some(tf) = loader.new_from_data(bytes, None) {
- let mut provider = TypefaceFontProvider::new();
- provider.register_typeface(tf, Some(family));
- self.font_collection.set_asset_font_manager(Some(provider.into()));
- }
self.paragraph = None;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-text-edit/src/skia_layout.rs` around lines 194 - 200, The
add_font_bytes function currently creates a new TypefaceFontProvider and calls
font_collection.set_asset_font_manager on each call, which discards previously
registered fonts; fix this by adding a cached collection field (e.g.,
Vec<(String, Vec<u8>)> or similar) to the struct to store each (family, bytes)
registration, push the new font into that cache inside add_font_bytes, then
rebuild a single TypefaceFontProvider from all cached entries (iterating the
cache and calling provider.register_typeface for each (family, bytes) pair) and
call self.font_collection.set_asset_font_manager(Some(provider.into())) once
with the rebuilt provider so all fonts are preserved across calls; keep using
FontMgr::new() and loader.new_from_data per cached bytes when constructing
typefaces.
Comment on lines
+57
to
+58
| // 👍🏽 is a 2-codepoint ZWJ sequence (8 UTF-8 bytes). | ||
| let s = TextEditorState::with_cursor("a👍🏽b", 9); // after 👍🏽 |
There was a problem hiding this comment.
Fix the emoji terminology in the test comment.
👍🏽 is an emoji + skin-tone modifier grapheme cluster, not a ZWJ sequence.
📝 Suggested comment fix
- // 👍🏽 is a 2-codepoint ZWJ sequence (8 UTF-8 bytes).
+ // 👍🏽 is a 2-codepoint emoji+modifier grapheme cluster (8 UTF-8 bytes).📝 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.
Suggested change
| // 👍🏽 is a 2-codepoint ZWJ sequence (8 UTF-8 bytes). | |
| let s = TextEditorState::with_cursor("a👍🏽b", 9); // after 👍🏽 | |
| // 👍🏽 is a 2-codepoint emoji+modifier grapheme cluster (8 UTF-8 bytes). | |
| let s = TextEditorState::with_cursor("a👍🏽b", 9); // after 👍🏽 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-text-edit/src/tests.rs` around lines 57 - 58, Update the test
comment that incorrectly calls 👍🏽 a "2-codepoint ZWJ sequence" — locate the
TextEditorState::with_cursor("a👍🏽b", 9) test in tests.rs and change the
comment to correctly describe 👍🏽 as an emoji plus skin-tone modifier grapheme
cluster (e.g., "👍🏽 is an emoji + skin-tone modifier grapheme cluster (8 UTF-8
bytes),") so the terminology is accurate.
Comment on lines
+200
to
+203
| - **k = 1 (single click)**: place the caret at `p` (a collapsed selection). | ||
| - **k = 2 (double click)**: select the **word** containing `p`, i.e. the range `R_word(p)`. | ||
| - **k = 3 (triple click)**: select the **line** containing `p`, i.e. the range `R_line(p)`. | ||
| - **k = 4 (quadruple click)**: select the entire editable value (document range). |
There was a problem hiding this comment.
Reduce repetitive phrasing in the multi-click bullets.
The three consecutive lines read repetitively; a small reword keeps the section easier to scan.
✍️ Proposed wording tweak
-- **k = 2 (double click)**: select the **word** containing `p`, i.e. the range `R_word(p)`.
-- **k = 3 (triple click)**: select the **line** containing `p`, i.e. the range `R_line(p)`.
-- **k = 4 (quadruple click)**: select the entire editable value (document range).
+- **k = 2 (double click)**: word-select at `p`, i.e. `R_word(p)`.
+- **k = 3 (triple click)**: line-select at `p`, i.e. `R_line(p)`.
+- **k = 4 (quadruple click)**: select the entire editable value (document range).🧰 Tools
🪛 LanguageTool
[style] ~202-~202: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ng p, i.e. the range R_word(p). - k = 3 (triple click): select the **line...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~203-~203: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ng p, i.e. the range R_line(p). - k = 4 (quadruple click): select the ent...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/wg/feat-text-editing/index.md` around lines 200 - 203, The three
multi-click bullet lines repeat "select the ..." phrasing; simplify to a
consistent, concise form by rewording each bullet to remove redundancy—e.g., for
"k = 1 (single click)" use "place the caret at p (collapsed selection)"; for "k
= 2 (double click)" use "select the word containing p (R_word(p))"; for "k = 3
(triple click)" use "select the line containing p (R_line(p))"; and for "k = 4
(quadruple click)" use "select the entire editable value (document range)";
update the bullets for k = 1..4 accordingly to improve scanability.
- Updated references from `TanantMiddleware` to `TenantMiddleware` in `AGENTS.md`, `proxy.ts`, and `middleware.ts` for consistency and accuracy. - Ensured proper naming conventions are followed across the codebase to prevent potential confusion.
…e line checks - Modified the `is_empty_line` method in `LineMetrics` to take a `text` parameter, ensuring it accurately checks for empty lines based on the provided text. - Updated all relevant calls to `is_empty_line` across the codebase to pass the necessary text context, enhancing the robustness of line metrics handling during text editing operations.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/grida-text-edit/src/simple_layout.rs (1)
193-205:⚠️ Potential issue | 🟠 Major
chars().count()miscounts multi-codepoint graphemes in selection geometry, misaligning withcaret_rect_atwhich uses grapheme counts.Lines 197, 201, and 204 use
chars().count()while all other geometry (caret_rect_atat line 131,position_at_pointat line 106) usesgraphemes(true).count(). Multi-codepoint sequences (accented characters, emoji, Hangul jamo) will produce incorrectx_lo/x_hioffsets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-text-edit/src/simple_layout.rs` around lines 193 - 205, The selection geometry uses chars().count() which miscounts multi-codepoint graphemes and desyncs with caret_rect_at and position_at_point that use graphemes(true).count(); update the x_lo and x_hi calculations in simple_layout.rs to use before.graphemes(true).count() (or text[...].graphemes(true).count()) instead of chars().count() for the three occurrences so x offsets align with caret_rect_at and position_at_point.crates/grida-text-edit/src/skia_layout.rs (1)
194-202:⚠️ Potential issue | 🟠 Major
add_font_bytesstill replaces the asset manager on each call, discarding previously registered fonts.Each invocation constructs a fresh
TypefaceFontProvider, registers only the new typeface, and overwrites the font manager — earlier font registrations are lost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-text-edit/src/skia_layout.rs` around lines 194 - 202, add_font_bytes currently creates a new TypefaceFontProvider each call and overwrites the font manager, losing prior fonts; fix it by reusing/merging a persistent provider instead of replacing it: retrieve or create a single TypefaceFontProvider (store it on the struct, e.g., self.typeface_provider: Option<TypefaceFontProvider> or attempt to get the existing manager from self.font_collection and downcast/convert it), then call register_typeface(tf, Some(family)) on that provider and set_asset_font_manager(Some(provider.into())) while keeping the provider stored for future calls; keep the existing self.paragraph = None behavior.
🧹 Nitpick comments (4)
crates/grida-text-edit/src/skia_layout.rs (2)
1-8: Redundantself as skia_safealias in theusestatement.
skia_safeis already the crate's canonical name;self as skia_safeis a no-op that clippy flags asuseless_rename.🧹 Suggested cleanup
-use skia_safe::{ - self as skia_safe, - textlayout::{ +use skia_safe::{ + textlayout::{ FontCollection, Paragraph, ParagraphBuilder, ParagraphStyle, RectHeightStyle, RectWidthStyle, TextStyle, TypefaceFontProvider, }, Color, FontMgr, Point, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-text-edit/src/skia_layout.rs` around lines 1 - 8, The top-level use imports include a redundant alias "self as skia_safe" which is a no-op and flagged by clippy; update the import so it directly uses skia_safe without the "self as skia_safe" rename (locate the use statement that mentions skia_safe and textlayout::{...} and remove the "self as skia_safe," part), keeping the rest of the imported items (FontCollection, Paragraph, ParagraphBuilder, ParagraphStyle, RectHeightStyle, RectWidthStyle, TextStyle, TypefaceFontProvider, Color, FontMgr, Point) unchanged.
247-262: Redundant guardif last_end <= text.len()is always true.
last_endis derived from already-validatedend_indexvalues (each clamped totext.len()on line 232), so the innerifat line 250 is always entered and the outerlast_end < text.len()branch is unreachable after Skia's loop. The double condition obscures intent.🧹 Simplified form
if !text.is_empty() && text.ends_with('\n') { - let last_end = result.last().map_or(0, |lm| lm.end_index); - if last_end < text.len() || result.last().map_or(true, |lm| lm.start_index < lm.end_index) { - if last_end <= text.len() { - let last = result.last().unwrap(); - let line_height = last.ascent + last.descent; - result.push(LineMetrics { - start_index: text.len(), - end_index: text.len(), - baseline: last.baseline + line_height, - ascent: last.ascent, - descent: last.descent, - }); - } - } + if result.last().map_or(true, |lm| lm.start_index < lm.end_index) { + let last = result.last().unwrap(); + let line_height = last.ascent + last.descent; + result.push(LineMetrics { + start_index: text.len(), + end_index: text.len(), + baseline: last.baseline + line_height, + ascent: last.ascent, + descent: last.descent, + }); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-text-edit/src/skia_layout.rs` around lines 247 - 262, The nested check `if last_end <= text.len()` is redundant; simplify the branch that handles trailing newline by removing that inner guard and directly using the last line metrics safely — replace the inner block with an if let Some(last) = result.last() { let line_height = last.ascent + last.descent; result.push(LineMetrics { start_index: text.len(), end_index: text.len(), baseline: last.baseline + line_height, ascent: last.ascent, descent: last.descent, }); } so you no longer rely on an always-true `last_end <= text.len()` and avoid unwrap() on `result.last()`; keep the outer condition using `last_end` and `text.ends_with('\n')` as-is.crates/grida-text-edit/src/lib.rs (2)
733-738:line_index_for_offset_utf8duplicatesline_index_for_offsetfromlayout.rs.Both have identical bodies;
layout.rsalready exportsline_index_for_offset(re-exported on line 11 of this file). The second definition inlib.rsis dead weight and a DRY violation.♻️ Suggested cleanup
Remove the
line_index_for_offset_utf8definition inlib.rsand replace all internal call sites with the already-re-exportedline_index_for_offset.-pub fn line_index_for_offset_utf8(metrics: &[LineMetrics], utf8_offset: usize) -> usize { - metrics - .iter() - .position(|lm| utf8_offset < lm.end_index) - .unwrap_or(metrics.len().saturating_sub(1)) -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-text-edit/src/lib.rs` around lines 733 - 738, Remove the redundant function definition pub fn line_index_for_offset_utf8(...) in lib.rs and replace all internal call sites that reference line_index_for_offset_utf8 with the already-exported line_index_for_offset from layout.rs; ensure any modules using the old name import or reference line_index_for_offset (the re-export is already available) and run tests to confirm no remaining references to line_index_for_offset_utf8 remain.
151-166: Redundantif pos > 0guard inprev_word_segment_startis dead code.Line 160's
if pos > 0is always true: execution only reaches it whenpos > 0(the early-return at line 152 eliminates thepos == 0case) andseg_start >= pos(the return at line 156-158 eliminatesseg_start < pos). The dead branch and its surrounding comment are noise.🧹 Simplified form
// pos is exactly at a segment boundary — step back into the previous one - if pos > 0 { - let safe = floor_char_boundary(text, pos.saturating_sub(1)); - let (prev_start, _) = word_segment_at(text, safe); - return prev_start; - } - 0 + let safe = floor_char_boundary(text, pos.saturating_sub(1)); + let (prev_start, _) = word_segment_at(text, safe); + prev_start🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-text-edit/src/lib.rs` around lines 151 - 166, The guard `if pos > 0` in function prev_word_segment_start is redundant because the function already returned when pos == 0 and when seg_start < pos; remove that dead branch and its comment and simplify the logic to unconditionally compute safe = floor_char_boundary(text, pos.saturating_sub(1)) and then call word_segment_at(text, safe) to return prev_start; keep using the existing helpers word_segment_at and floor_char_boundary and preserve the early-return at the top.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/grida-text-edit/src/skia_layout.rs`:
- Around line 272-281: The loop in skia_layout.rs is prematurely returning for
lines with one UTF-16 unit because the check uses
lm.end_index.saturating_sub(lm.start_index) <= 1; change this guard to == 0 so
only truly empty (newline-only) runs trigger the early return using
utf16_to_utf8_offset(text, lm.start_index).min(text.len()), and allow
single-character runs to fall through so cursor placement can be computed using
the x coordinate; update the condition near the loop over metrics and the
references to lm.start_index/lm.end_index accordingly.
---
Duplicate comments:
In `@crates/grida-text-edit/src/simple_layout.rs`:
- Around line 193-205: The selection geometry uses chars().count() which
miscounts multi-codepoint graphemes and desyncs with caret_rect_at and
position_at_point that use graphemes(true).count(); update the x_lo and x_hi
calculations in simple_layout.rs to use before.graphemes(true).count() (or
text[...].graphemes(true).count()) instead of chars().count() for the three
occurrences so x offsets align with caret_rect_at and position_at_point.
In `@crates/grida-text-edit/src/skia_layout.rs`:
- Around line 194-202: add_font_bytes currently creates a new
TypefaceFontProvider each call and overwrites the font manager, losing prior
fonts; fix it by reusing/merging a persistent provider instead of replacing it:
retrieve or create a single TypefaceFontProvider (store it on the struct, e.g.,
self.typeface_provider: Option<TypefaceFontProvider> or attempt to get the
existing manager from self.font_collection and downcast/convert it), then call
register_typeface(tf, Some(family)) on that provider and
set_asset_font_manager(Some(provider.into())) while keeping the provider stored
for future calls; keep the existing self.paragraph = None behavior.
---
Nitpick comments:
In `@crates/grida-text-edit/src/lib.rs`:
- Around line 733-738: Remove the redundant function definition pub fn
line_index_for_offset_utf8(...) in lib.rs and replace all internal call sites
that reference line_index_for_offset_utf8 with the already-exported
line_index_for_offset from layout.rs; ensure any modules using the old name
import or reference line_index_for_offset (the re-export is already available)
and run tests to confirm no remaining references to line_index_for_offset_utf8
remain.
- Around line 151-166: The guard `if pos > 0` in function
prev_word_segment_start is redundant because the function already returned when
pos == 0 and when seg_start < pos; remove that dead branch and its comment and
simplify the logic to unconditionally compute safe = floor_char_boundary(text,
pos.saturating_sub(1)) and then call word_segment_at(text, safe) to return
prev_start; keep using the existing helpers word_segment_at and
floor_char_boundary and preserve the early-return at the top.
In `@crates/grida-text-edit/src/skia_layout.rs`:
- Around line 1-8: The top-level use imports include a redundant alias "self as
skia_safe" which is a no-op and flagged by clippy; update the import so it
directly uses skia_safe without the "self as skia_safe" rename (locate the use
statement that mentions skia_safe and textlayout::{...} and remove the "self as
skia_safe," part), keeping the rest of the imported items (FontCollection,
Paragraph, ParagraphBuilder, ParagraphStyle, RectHeightStyle, RectWidthStyle,
TextStyle, TypefaceFontProvider, Color, FontMgr, Point) unchanged.
- Around line 247-262: The nested check `if last_end <= text.len()` is
redundant; simplify the branch that handles trailing newline by removing that
inner guard and directly using the last line metrics safely — replace the inner
block with an if let Some(last) = result.last() { let line_height = last.ascent
+ last.descent; result.push(LineMetrics { start_index: text.len(), end_index:
text.len(), baseline: last.baseline + line_height, ascent: last.ascent, descent:
last.descent, }); } so you no longer rely on an always-true `last_end <=
text.len()` and avoid unwrap() on `result.last()`; keep the outer condition
using `last_end` and `text.ends_with('\n')` as-is.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/grida-text-edit/src/layout.rscrates/grida-text-edit/src/lib.rscrates/grida-text-edit/src/simple_layout.rscrates/grida-text-edit/src/skia_layout.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/grida-text-edit/src/layout.rs
Comment on lines
+272
to
+281
| for lm in &metrics { | ||
| let top = lm.baseline as f32 - lm.ascent as f32; | ||
| let bot = lm.baseline as f32 + lm.descent as f32; | ||
| if y >= top - 0.5 && y <= bot + 0.5 { | ||
| if lm.end_index.saturating_sub(lm.start_index) <= 1 { | ||
| return utf16_to_utf8_offset(text, lm.start_index).min(text.len()); | ||
| } | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Early return fires for single-character lines, always snapping to line start regardless of x.
The condition end_index.saturating_sub(start_index) <= 1 catches lines with exactly one UTF-16 unit (e.g. a line containing a single ASCII character). For such a line the cursor is always returned as lm.start_index, making it impossible to position after the lone character.
The intent appears to be "empty / newline-only line" → guard should be == 0:
🐛 Proposed fix
- if lm.end_index.saturating_sub(lm.start_index) <= 1 {
+ if lm.end_index.saturating_sub(lm.start_index) == 0 {
return utf16_to_utf8_offset(text, lm.start_index).min(text.len());
}📝 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.
Suggested change
| for lm in &metrics { | |
| let top = lm.baseline as f32 - lm.ascent as f32; | |
| let bot = lm.baseline as f32 + lm.descent as f32; | |
| if y >= top - 0.5 && y <= bot + 0.5 { | |
| if lm.end_index.saturating_sub(lm.start_index) <= 1 { | |
| return utf16_to_utf8_offset(text, lm.start_index).min(text.len()); | |
| } | |
| break; | |
| } | |
| } | |
| for lm in &metrics { | |
| let top = lm.baseline as f32 - lm.ascent as f32; | |
| let bot = lm.baseline as f32 + lm.descent as f32; | |
| if y >= top - 0.5 && y <= bot + 0.5 { | |
| if lm.end_index.saturating_sub(lm.start_index) == 0 { | |
| return utf16_to_utf8_offset(text, lm.start_index).min(text.len()); | |
| } | |
| break; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-text-edit/src/skia_layout.rs` around lines 272 - 281, The loop
in skia_layout.rs is prematurely returning for lines with one UTF-16 unit
because the check uses lm.end_index.saturating_sub(lm.start_index) <= 1; change
this guard to == 0 so only truly empty (newline-only) runs trigger the early
return using utf16_to_utf8_offset(text, lm.start_index).min(text.len()), and
allow single-character runs to fall through so cursor placement can be computed
using the x coordinate; update the condition near the loop over metrics and the
references to lm.start_index/lm.end_index accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A fully-featured non-rich text editor
Summary by CodeRabbit
New Features
Tests
Documentation
Chores