feat(grida-canvas): attributed text node, in-scene text editing, and font system refactor#603
feat(grida-canvas): attributed text node, in-scene text editing, and font system refactor#603softmarshmallow merged 16 commits intomainfrom
Conversation
- Introduced a new example for rendering attributed text with mixed styles, weights, and colors in `golden_type_attributed.rs`. - Added a benchmark scene for heavy attributed text rendering in `tool_gen_bench_fixture.rs`. - Created a fixture for attributed text in `fixtures/l0_type_attributed.rs` to demonstrate various text styles. - Updated existing files to classify and handle attributed text nodes in the scene graph and SVG processing. - Enhanced geometry and paragraph caching to support measurements for attributed text. - Added new images for goldens to validate attributed text rendering. This commit expands the text rendering capabilities of the grida-canvas, allowing for richer text presentations.
… module The standalone grida-text-edit crate forced lossy conversion layers to carry style data (gradients, strokes, multi-fill stacks) across the crate boundary. By inlining it as a cg module, the editor uses Paint, CGColor, and StyledTextRun directly — no duplicate types, no round-trip data loss. - Move all grida-text-edit/src/* → crates/grida-canvas/src/text_edit/ - Delete old 542-line attributed_text_conv.rs, replace with 256-line conv.rs that maps cg types directly (no RGBA/TextFill intermediaries) - Remove grida-text-edit crate (Cargo.toml, README, workspace member) - Move wd_text_editor example to grida-dev (windowed demos live there) - Add PartialEq to gradient/alignment types for run coalescing - Add optional serde feature to math2 (AffineTransform, BoxFit are embedded in already-serializable gradient/image-paint types) - Update stale references in docs and comments
…ce CJK support - Refactored attributed text rendering by consolidating functions and improving parameter handling in `build_attributed_paragraph`. - Updated `textstyle` function to accept additional parameters for better flexibility. - Enhanced support for CJK (Korean, Japanese, Chinese) text in attributed text nodes, ensuring accurate byte offset handling. - Added tests to validate roundtrip functionality for attributed text with mixed styles and multi-byte characters. - Introduced a new SVG fixture for CJK text rendering to ensure visual consistency across platforms. This commit improves the overall robustness and usability of attributed text features in the grida-canvas.
Move user_fallback_fonts out of TextStyleRecBuildContext and into FontRepository, which is now passed directly to textstyle() and the attributed paragraph builder. This eliminates per-call Vec<String> cloning and makes FontRepository the single source of truth for fallback family resolution via make_font_families(). Add RendererOptions.use_system_fonts (default false) to optionally enable Skia's system FontMgr as the default fallback — useful for native dev previewing but disabled on WASM where no system fonts exist. Wire --system-fonts flag through grida-dev CLI for interactive mode.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
|
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:
WalkthroughInlines the text-edit engine into the canvas crate as Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Surf as SurfaceEventHandler
participant Renderer as Renderer
participant Cache as SceneCache/ParagraphCache
participant Fonts as FontRepository
participant Skia as Skia (Paragraph/Layout)
rect rgba(200,200,255,0.5)
Note over App,Surf: Text edit entry and input flow
end
App->>Surf: SurfaceEvent (Pointer/Key/Text/IME)
Surf->>App: handle_surface_event(event)
App->>Renderer: text_edit_command / update_layer_attributed_text
Renderer->>Cache: measure_attributed(attributed_string, fonts, id)
Cache->>Fonts: request font generation / families
Fonts-->>Cache: fonts + generation
Cache->>Skia: build attributed paragraphs (fill + optional stroke)
Skia-->>Cache: Paragraph(s) + metrics
Cache-->>Renderer: measurements + paragraph set
Renderer->>Skia: paint attributed paragraphs (with effects/clip)
Skia-->>Renderer: drawn
sequenceDiagram
participant SVG as SVG/Figma Import
participant Pack as SVG Pack
participant Scene as Scene Builder
participant Layout as LayoutEngine
participant Cache as SceneCache/ParagraphCache
participant Painter as Painter
rect rgba(200,255,200,0.5)
Note over SVG,Pack: Attributed text import path
end
SVG->>Pack: emit IRSVGTextChunk(s) (Uniform/Attributed)
Pack->>Scene: append_attributed_text_node(runs, default_style)
Scene->>Layout: build_taffy_subtree (AttributedText leaf)
Layout->>Cache: measure_attributed for missing schema
Cache-->>Layout: measurements
Layout->>Painter: layout + measured sizes
Painter->>Skia: render attributed paragraphs
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Introduce a complete text editing lifecycle owned by the Rust application layer, enabling double-click-to-edit, keyboard routing, click-outside-to-exit, and IME composition for both TextSpan and AttributedText nodes. Key changes: - Extend SurfaceEvent with KeyDown, TextInput, and Ime variants - Add double-click detection via ClickTracker in SurfaceState - Add handle_surface_event() unified dispatcher with keyboard/pointer routing - Support AttributedText node editing (text_edit_enter_by_id, new_attributed) - Centralized commit via TextEditCommit and apply_text_edit_commit() - Live text rendering during editing with proper cache invalidation - Pan image cache invalidation on text changes to prevent flickering - Layer shape bounds update on text reflow for correct overlay outlines - System clipboard support (Cmd+C/X/V) via arboard in NativeApplication - IME composition (Korean, Japanese, Chinese) with set_ime_allowed toggling - SurfaceResponse::redraw() helper, consolidated decoration refresh logic This is a native-only development feature — the WASM path continues using the existing host-driven text_edit_* API unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…eometry Three structural defects caused selection box width to diverge from baseline width after font fallback changes: 1. FontRepository::set_fallback_fonts() and enable_system_fallback() mutated the FontCollection without incrementing generation, so ParagraphCache returned stale measurements (tofu glyph widths). 2. FONT_LOADED change flag invalidated paragraph/picture/compositor caches but never triggered geometry rebuild, leaving GeometryCache with stale text bounds until the next full scene reload. 3. get_baseline_info_if_cached_by_id() read cached paragraphs without checking font generation, allowing stale baseline data to be used. Fix: centralize generation bumping via bump_generation(), add FONT_LOADED to the layout rebuild trigger, and add a font-generation guard to the baseline info reader. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fix three type errors introduced by AttributedTextNode addition: - Add missing `attrib` entry to fillings lookup table - Correct return/parameter types in io-figma attributed text test - Cast text field to satisfy UnknownNode intersection type narrowing Add `pnpm typecheck` step to test workflow to catch these going forward. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… workflow Introduce a new job in the GitHub Actions workflow to build the @grida/canvas-wasm package using tsup. This step ensures that the pre-compiled .wasm binary and Emscripten glue are correctly processed and available for type checking.
…s for fill color preservation - Refactored character style overrides logic to use a Map for better ID matching, ensuring accurate retrieval of style overrides. - Added a new test case to verify that blue fill colors are preserved on overridden CJK text runs, enhancing the robustness of attributed text rendering.
…scenarios Updated the frame rendering logic to ensure that stable frames do not utilize the blit content cache when rendering. This change addresses issues where the canvas could display reduced-quality content from previous unstable renders, ensuring that full-quality frames are produced consistently after camera commands.
…cies Updated the GitHub Actions workflow to disable the typecheck step in CI. This change addresses issues with TypeScript errors arising from unresolved image module declarations when a full Next.js build is not performed, ensuring smoother CI runs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
editor/grida-canvas/editor.ts (1)
1727-1735:⚠️ Potential issue | 🟠 MajorRemove unsafe type assertion—method signature and dispatch payload are misaligned.
The cast
text as string | nullat line 1734 contradicts the method signaturetext: tokens.StringValueExpression | null. The node schema defines text asPropsTextValue(=StringValueExpression), which includesPropertyAccessExpression,Identifier, andTemplateExpression—valid runtime values per the schema docs.The cast silently narrows this union to plain
string, which contradicts what the reducer expects (fullStringValueExpression). Similar methods (changeNodePropertySrc,changeNodePropertyProps) pass the full union without casting. All current call sites pass strings, but the API signature permits non-string variants, making the cast a latent bug.Solution: Remove the cast and dispatch
textas-is, consistent with similar methods.changeNodePropertyText( node_id: string, text: tokens.StringValueExpression | null ) { this.dispatch({ type: "node/change/*", node_id: node_id, - text: text as string | null, + text, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas/editor.ts` around lines 1727 - 1735, The method changeNodePropertyText currently narrows text with an unsafe cast to string in the dispatch payload; remove the cast and pass the tokens.StringValueExpression | null value through untouched so the dispatched payload matches the node schema and reducer expectations (similar to changeNodePropertySrc and changeNodePropertyProps). Update the dispatch call for type "node/change/*" to use the original text parameter (tokens.StringValueExpression | null) instead of casting to string | null, ensuring PropsTextValue variants like PropertyAccessExpression, Identifier, and TemplateExpression are preserved.crates/grida-canvas/src/text_edit/session_tests.rs (1)
318-318:⚠️ Potential issue | 🟡 MinorFix typo to unblock the
typospipeline check.Line 318 contains
hel, which is currently failing CI in the provided pipeline output.📝 Proposed fix
- s.state.cursor = 3; // between "hel" and "lo" + s.state.cursor = 3; // between the first three letters of "hello" and "lo"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/text_edit/session_tests.rs` at line 318, The test comment contains a typo token "hel" that trips the typos linter; update the comment surrounding the cursor setup (the line with s.state.cursor = 3 in session_tests.rs) to use a non-typo phrase such as replacing "hel" with "hell" or rewording to "between \"hel\" and \"lo\"" -> e.g., change the comment to "between \"hell\" and \"o\"" or otherwise rephrase so the word "hel" is removed; keep the cursor assignment (s.state.cursor = 3) unchanged.crates/grida-canvas/src/surface/state.rs (1)
113-119:⚠️ Potential issue | 🟠 MajorBase multi-click tracking on screen-space node hits.
The tracker is updated before hit resolution and with
canvas_point, so zoom/pan changes the effective double-click radius and an empty-space or overlay click can arm the next node click as a double-click. That can spuriously setdouble_clicked_nodewhen the user was only clearing selection or starting a marquee.🔧 Suggested fix
if button == PointerButton::Primary { if let Some(action) = ui_hit_regions.hit_test([screen_point[0], screen_point[1]]) { + self.click_tracker = ClickTracker::new(); return self.handle_overlay_action(action, hierarchy); } } - self.handle_pointer_down(canvas_point, button, hit_tester, hierarchy) + self.handle_pointer_down( + canvas_point, + screen_point, + button, + hit_tester, + hierarchy, + ) } @@ fn handle_pointer_down( &mut self, canvas_point: math2::vector2::Vector2, + screen_point: math2::vector2::Vector2, button: PointerButton, hit_tester: &HitTester, hierarchy: &impl Hierarchy, ) -> SurfaceResponse { let mut response = SurfaceResponse::none(); @@ - // Track multi-click sequence for double-click detection. - let click_count = self.click_tracker.register(canvas_point[0], canvas_point[1]); - let hit = hit_tester.hit_first(canvas_point); match hit { Some(id) => { + let click_count = self + .click_tracker + .register(screen_point[0], screen_point[1]); // Clicked on a node if self.modifiers.shift { self.selection.toggle(id); @@ - if click_count >= 2 { + if click_count == 2 { response.double_clicked_node = Some(id); } } None => { + self.click_tracker = ClickTracker::new(); if self.modifiers.shift { // Shift+click on empty space: keep selection, start marquee // (marquee will union with existing selection — future enhancement)Also applies to: 239-245, 252-271
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/surface/state.rs` around lines 113 - 119, The multi-click tracker is being updated before hit resolution and using canvas_point, causing zoom/pan to change double-click radius and allowing overlay/empty-space clicks to arm a node double-click; change the logic so the tracker is updated only after resolving a node hit (i.e., after calling ui_hit_regions.hit_test and the main hit tester) and use screen-space coordinates (screen_point) when recording successive clicks for functions like handle_pointer_down and handle_overlay_action so double_clicked_node is only set when the actual node was hit in screen space. Ensure all similar blocks (the ones around handle_pointer_down and the areas mentioned as also applying to lines 239-245 and 252-271) follow this order: perform hit tests (ui_hit_regions.hit_test and hit_tester), determine the hit target, then update the multi-click tracker with screen_point and finally call the handlers.crates/grida-canvas/src/runtime/scene.rs (1)
1740-1746:⚠️ Potential issue | 🟠 MajorInvalidate paragraphs before the new
FONT_LOADEDrelayout.The new
needs_layoutbranch can callrebuild_scene_caches()before the paragraph cache is cleared. On a font load that means the relayout can reuse fallback-font paragraphs and keep old geometry/layer bounds; the cache is only invalidated afterwards.💡 Minimal fix
+ if flags.contains(ChangeFlags::FONT_LOADED) { + self.scene_cache.paragraph.borrow_mut().invalidate(); + } + if has_data_changes && !flags.contains(ChangeFlags::SCENE_LOAD) { let needs_layout = flags .intersects(ChangeFlags::LAYOUT_DIRTY | ChangeFlags::FONT_LOADED) || (flags.contains(ChangeFlags::VIEWPORT_SIZE) && self.scene_has_viewport_dependent_layout()); if needs_layout { self.rebuild_scene_caches(); } } @@ - if flags.intersects(ChangeFlags::SCENE_LOAD | ChangeFlags::FONT_LOADED) { + if flags.contains(ChangeFlags::SCENE_LOAD) { self.scene_cache.paragraph.borrow_mut().invalidate(); }Also applies to: 1761-1763
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/runtime/scene.rs` around lines 1740 - 1746, The FONT_LOADED path may call rebuild_scene_caches() before paragraph geometry is invalidated, causing relayout to reuse old fallback-font paragraphs; ensure you invalidate the paragraph cache before rebuilding by calling the paragraph invalidation routine (e.g. whatever clears the paragraph cache / invalidate_paragraphs()) prior to calling self.rebuild_scene_caches() when ChangeFlags::FONT_LOADED is present in needs_layout, and make the same change at the other occurrence around the block referenced (the second rebuild_scene_caches() call).packages/grida-canvas-schema/grida.ts (1)
1221-1226:⚠️ Potential issue | 🟠 MajorKeep computed unions aligned with the new
"attrib"layer node.Line 1225 adds
"attrib"toLayerNode, butComputedNode/UnknownComputedNodein the same file still omit an attributed-text computed variant. This creates non-exhaustive type handling for the new node kind.💡 Suggested alignment patch
export type ComputedNode = | ComputedTextSpanNode + | ComputedAttributedTextNode | ComputedBitmapNode | ComputedImageNode | ComputedVideoNode | ComputedContainerNode @@ export type UnknownComputedNode = Omit< Partial<ComputedTextSpanNode> & + Partial<ComputedAttributedTextNode> & Partial<ComputedImageNode> & @@ } & i.IBaseNode & i.ISceneNode; + +export interface ComputedAttributedTextNode extends AttributedTextNode { + readonly type: "attrib"; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/grida-canvas-schema/grida.ts` around lines 1221 - 1226, LayerNode now includes the "attrib" (AttributedTextNode) variant but the computed-type unions ComputedNode and UnknownComputedNode have not been updated to include the corresponding computed/attributed-text computed variant; update the union types (ComputedNode and UnknownComputedNode) to add the attributed-text computed node type (the computed variant that corresponds to AttributedTextNode, e.g., AttributedTextComputedNode or whatever naming convention is used in the file) so exhaustiveness checks cover the "attrib" kind alongside GroupNode, TextSpanNode, ImageNode, etc.
🟠 Major comments (27)
.github/workflows/test.yml-28-34 (1)
28-34:⚠️ Potential issue | 🟠 MajorEnable typecheck in CI by adding the missing build step documented in AGENTS.md.
Typecheck is commented out because
next-env.d.ts(a Next.js-generated file) is missing at CI time. The repository's AGENTS.md (lines 223–241) documents the correct procedure: build packages, build@grida/canvas-wasm, then run typecheck. The current workflow builds packages but skips the canvas-wasm build, so typecheck fails.Update the workflow to run
pnpm turbo build --filter@grida/canvas-wasm`` before re-enabling typecheck, following the documented procedure:Suggested workflow adjustment
- name: Build Packages run: pnpm turbo build --filter='./packages/*' + - name: Build Canvas WASM + run: pnpm turbo build --filter `@grida/canvas-wasm` - # Typecheck is disabled in CI. The editor's next-env.d.ts imports - # .next/dev/types/routes.d.ts, which is only generated by `next dev` - # or `next build`. Without a full Next.js build, tsc cannot resolve - # image module declarations (*.png, *.jpg) from next/image-types/global, - # causing spurious TS2307 errors across all static image imports. - # - name: Run Typecheck - # run: pnpm typecheck + - name: Run Typecheck + run: pnpm typecheck🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yml around lines 28 - 34, The CI workflow currently has the "Run Typecheck" step commented out because next-env.d.ts is missing; follow AGENTS.md by adding the missing build for the canvas wasm package before re-enabling typecheck: insert a step that runs "pnpm turbo build --filter `@grida/canvas-wasm`" (after the existing package build steps) and then uncomment/reactivate the "Run Typecheck" step (the commented "Run Typecheck" job in the workflow and the AGENTS.md instructions referenced) so that typecheck runs with the Next.js generated next-env.d.ts present.crates/grida-canvas/src/resources/mod.rs-206-207 (1)
206-207:⚠️ Potential issue | 🟠 MajorExtract image URLs from attributed-text node-level and per-run paints.
At Lines 206–207, the empty arm skips image URL extraction entirely for
Node::AttributedText. This node type has both node-levelfillsandstrokes(likeTextSpan) and additionally per-run fill and stroke paints in eachStyledTextRunwithin theAttributedString. All of these can bePaint::Imagevariants. Without extraction, image assets referenced by the node-level paints or any styled run will never be preloaded.Extract URLs from
n.fills,n.strokes, and then iterate theattributed_string.runsto collect image URLs from each run'sfillsandstrokes(whenSome).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/resources/mod.rs` around lines 206 - 207, The Node::AttributedText arm currently skips image extraction; update it to inspect the node-level paints and per-run paints: for the AttributedText node (Node::AttributedText(n)), collect image URLs from n.fills and n.strokes (handling Paint::Image) and then iterate n.attributed_string.runs (each StyledTextRun) and collect image URLs from each run.fills and run.strokes when Some, adding them to the same URL collection logic used elsewhere; reference Paint::Image, Node::AttributedText, n.fills, n.strokes, n.attributed_string.runs, and StyledTextRun to locate and mirror the extraction used for other node types.packages/grida-canvas-io/format.ts-1127-1173 (1)
1127-1173:⚠️ Potential issue | 🟠 MajorReject unmappable styled-run ranges instead of serializing them.
run.start/run.endare encoded without any bounds check, and the decoder falls back to raw byte offsets when a reverse lookup misses. That can write corrupt run ranges on save and then deserialize ranges pasttext.lengthon read.🛡️ Suggested fix
const c2b = node.text != null ? charToByteOffsets(node.text) : undefined; const runOffsets: flatbuffers.Offset[] = []; for (const run of node.styled_runs) { + if ( + node.text == null || + run.start < 0 || + run.end < run.start || + run.end > node.text.length + ) { + throw new Error( + `Invalid styled run range [${run.start}, ${run.end})` + ); + } + const runStyleOffset = encodeTextStyleRec(builder, run.style); @@ const byteStart = runItem.start(); const byteEnd = runItem.end(); + const start = b2c?.get(byteStart); + const end = b2c?.get(byteEnd); + if (start === undefined || end === undefined) { + throw new Error( + `Styled run byte range [${byteStart}, ${byteEnd}) is not on a UTF-8 boundary` + ); + } styledRuns.push({ - start: b2c ? (b2c.get(byteStart) ?? byteStart) : byteStart, - end: b2c ? (b2c.get(byteEnd) ?? byteEnd) : byteEnd, + start, + end, style: runStyle,Also applies to: 5794-5823
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/grida-canvas-io/format.ts` around lines 1127 - 1173, The encoder currently writes run.start/run.end into fbs.StyledTextRunItem (via StyledTextRunItem.addStart/addEnd) without validating charToByteOffsets mappings, allowing unmappable or out-of-bounds ranges to be serialized; instead, when encoding node.styled_runs (and similarly in the other block referenced), validate that c2b exists and that c2b[run.start] and c2b[run.end] are defined and within bounds (and that run.start < run.end); if any mapping is missing or out-of-range, skip/reject that styled run (do not call startStyledTextRunItem / addStart / addEnd for it) and optionally emit a warning/log; ensure the same checks are applied where styled runs are encoded at the other mentioned location.packages/grida-canvas-io/format.ts-22-43 (1)
22-43:⚠️ Potential issue | 🟠 MajorHandle lone high surrogates the same way the string encoder does.
These helpers currently treat any high surrogate as a 4-byte pair without validating the next code unit is a low surrogate. For malformed strings like
"\uD800A",TextEncoderencodesU+FFFD(3 bytes) +A(1 byte) = 4 bytes total, butcharToByteOffsetsmaps position 1 to byte offset 4, shifting every subsequent styled-run offset.The fix adds a validation check: only treat as a surrogate pair if the high surrogate (0xD800–0xDBFF) is immediately followed by a low surrogate (0xDC00–0xDFFF). Lone surrogates are then encoded as U+FFFD (3 bytes) instead.
🔧 Suggested fix
function charToByteOffsets(text: string): Uint32Array { // Walk the JS string (UTF-16 code units) computing UTF-8 byte positions. const map = new Uint32Array(text.length + 1); let byteIdx = 0; for (let i = 0; i < text.length; i++) { map[i] = byteIdx; const code = text.charCodeAt(i); + const next = text.charCodeAt(i + 1); + const hasSurrogatePair = + code >= 0xd800 && + code <= 0xdbff && + next >= 0xdc00 && + next <= 0xdfff; if (code < 0x80) { byteIdx += 1; } else if (code < 0x800) { byteIdx += 2; - } else if (code >= 0xd800 && code <= 0xdbff) { + } else if (hasSurrogatePair) { // High surrogate — this char + next form a surrogate pair (4 UTF-8 bytes). byteIdx += 4; i++; // skip low surrogate map[i] = byteIdx; // low surrogate maps to byte *after* the 4-byte seq } else { byteIdx += 3; }Also applies to:
byteToCharOffsetsat lines 30–48 (same fix needed).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/grida-canvas-io/format.ts` around lines 22 - 43, charToByteOffsets currently treats any high surrogate (0xD800–0xDBFF) as a valid 4-byte UTF‑8 pair without checking the next code unit; update charToByteOffsets to only consume a surrogate pair (advance i, add 4 bytes, and set map for the low surrogate) when the following code unit is a low surrogate (0xDC00–0xDFFF); if it's not a valid low surrogate, treat the lone high surrogate as U+FFFD (add 3 bytes, do not skip the next code unit). Apply the analogous fix in byteToCharOffsets: when encountering a 4‑byte UTF‑8 sequence, only map it to two UTF‑16 code units (a high+low surrogate pair) if the decoded code point requires a surrogate pair and you can emit both code units; otherwise map that sequence to a single replacement code unit (U+FFFD) so byte-to-char offsets remain correct.packages/grida-canvas-io-figma/lib.ts-1788-1806 (1)
1788-1806:⚠️ Potential issue | 🟠 MajorPer-run paint overrides are still lossy here.
Only
fillsare consumed when emittingstyled_runs, so thestrokePaints/strokeWeightvalues collected inkiwi_text_style_trait()are dropped. Also, becausefill_paintsis only emitted when the converted array is non-empty, an explicitfills: []override falls back to the node-level paints instead of clearing them.Also applies to: 2729-2734
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/grida-canvas-io-figma/lib.ts` around lines 1788 - 1806, Per-run paint overrides drop stroke data and treat an explicit empty fills override as missing: in the styled_runs creation (where overrideFills/runFillPaints are used) also include converted stroke paints and stroke weight collected by kiwi_text_style_trait (e.g., strokePaints and strokeWeight) by converting any override stroke paints via convertPaint and emitting fields like stroke_paints and stroke_weight alongside fill_paints; also ensure that when an override provides fills: [] you emit an explicit empty fill_paints array (not omit the field) so node-level paints are not used as a fallback.packages/grida-canvas-io-figma/lib.ts-1781-1787 (1)
1781-1787:⚠️ Potential issue | 🟠 MajorClear the inherited PostScript name when a run changes font identity.
This merge can leave a run with a mismatched pair such as
fontFamily: "Pretendard"plusfontPostScriptName: "Inter-Regular". If the override changes family/weight/italic and does not provide its own PostScript name, the inherited one should be dropped before the run style is materialized.💡 Possible fix
const mergedRestStyle = { ...node.style, ...overrideStyle, }; + if ( + !("fontPostScriptName" in overrideStyle) && + ("fontFamily" in overrideStyle || + "fontWeight" in overrideStyle || + "italic" in overrideStyle) + ) { + delete mergedRestStyle.fontPostScriptName; + } const gridaStyle = restStyleToGrida(mergedRestStyle);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/grida-canvas-io-figma/lib.ts` around lines 1781 - 1787, When merging node.style and overrideStyle before calling restStyleToGrida (the mergedRestStyle -> gridaStyle flow), ensure we drop an inherited fontPostScriptName if the override changes font identity: compare node.style.fontFamily/fontWeight/fontStyle (or italic) against overrideStyle's values, and if any of those identity fields are present in overrideStyle and differ from node.style while overrideStyle does not include fontPostScriptName, remove mergedRestStyle.fontPostScriptName (or set it undefined) so a stale PostScript name isn't carried into restStyleToGrida; update the merge logic that creates mergedRestStyle accordingly.packages/grida-canvas-io-figma/lib.ts-1712-1738 (1)
1712-1738:⚠️ Potential issue | 🟠 MajorHandle pixel line-height when building attributed styles.
restStyleToGrida()only derivesline_heightfromlineHeightPercentFontSize, but the Kiwi bridge writesPIXELSleading intolineHeightPx. Any.figtext/run with pixel line height will therefore lose its intended leading once it goes through theattribpath.💡 Possible fix
const restStyleToGrida = ( style: Record<string, unknown> - ): grida.program.nodes.i.ITextStyle => ({ - font_family: (style.fontFamily as string) ?? "Inter", - font_size: (style.fontSize as number) ?? DEFAULT_FONT_SIZE, + ): grida.program.nodes.i.ITextStyle => { + const fontSize = + (style.fontSize as number) ?? DEFAULT_FONT_SIZE; + const lineHeightPercentFontSize = ( + style as { lineHeightPercentFontSize?: number } + ).lineHeightPercentFontSize; + const lineHeightPx = (style as { lineHeightPx?: number }) + .lineHeightPx; + + return { + font_family: (style.fontFamily as string) ?? "Inter", + font_size: fontSize, font_weight: ((style.fontWeight as cg.NFontWeight) ?? 400) as cg.NFontWeight, font_kerning: true, text_decoration_line: style.textDecoration ? (map.textDecorationMap[ style.textDecoration as keyof typeof map.textDecorationMap ] ?? "none") : "none", - line_height: (style as { lineHeightPercentFontSize?: number }) - .lineHeightPercentFontSize - ? (style as { lineHeightPercentFontSize: number }) - .lineHeightPercentFontSize / 100 - : undefined, + line_height: + typeof lineHeightPercentFontSize === "number" + ? lineHeightPercentFontSize / 100 + : typeof lineHeightPx === "number" + ? lineHeightPx / fontSize + : undefined, letter_spacing: (style.letterSpacing as number) ? (style.letterSpacing as number) / - ((style.fontSize as number) || DEFAULT_FONT_SIZE) + fontSize : undefined, font_postscript_name: (style.fontPostScriptName as string) || undefined, font_style_italic: (style.italic as boolean) ?? false, - }); + }; + };Also applies to: 2606-2651, 2721-2727
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/grida-canvas-io-figma/lib.ts` around lines 1712 - 1738, restStyleToGrida currently only maps line_height from lineHeightPercentFontSize and ignores pixel-based line heights written by the Kiwi bridge (lineHeightPx), causing runs with PIXELS leading to lose their leading; update restStyleToGrida to first check for a numeric lineHeightPx on the incoming style and, if present, convert it to a unitless line_height by dividing lineHeightPx by the available fontSize (falling back to DEFAULT_FONT_SIZE) and return that value, otherwise fall back to the existing lineHeightPercentFontSize handling; apply the same change to the corresponding mapping blocks referenced around the other instances (the similar functions/blocks at the locations you noted) so pixel line-heights are consistently converted to ratio-based line_height for i.ITextStyle.crates/grida-canvas/src/layout/into_taffy.rs-488-496 (1)
488-496:⚠️ Potential issue | 🟠 MajorHonor explicit attributed-text heights in Taffy.
AttributedTextNodeReccarriesheight, and the painter path already uses it as the text-box height, but this mapper always forcessize.height = auto. Fixed-height attributed text boxes will therefore lay out at content height, so vertical alignment, clipping, and selection bounds can drift from the declared box.🔧 Suggested fix
- // Height is auto for text (will be determined by content) - style.size.height = Dimension::auto(); + style.size.height = if let Some(height) = node.height { + Dimension::length(height) + } else { + Dimension::auto() + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/layout/into_taffy.rs` around lines 488 - 496, The mapper currently always forces style.size.height = Dimension::auto(), ignoring explicit heights on attributed text; update the mapping for height to mirror the width logic: if node.height (from AttributedTextNodeRec / the node variable used in into_taffy) is Some(height) set style.size.height = Dimension::length(height) else set it to Dimension::auto(), so fixed-height attributed text boxes are respected for layout, clipping and selection.crates/grida-canvas/src/svg/from_usvg_tree.rs-237-239 (1)
237-239:⚠️ Potential issue | 🟠 MajorOnly emit
Uniformwhen one visible span covers the full chunk.
visible_spans.len() == 1is not enough here. A chunk with one visible span plus hidden or otherwise skipped siblings will still serialize the entirechunk_textwith that single style.Also applies to: 276-293
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/svg/from_usvg_tree.rs` around lines 237 - 239, The code currently treats a chunk as uniform when visible_spans.len() == 1; change the logic to only emit `Uniform` when there is exactly one visible span AND that visible span covers the entire chunk text (i.e., its text/byte length or range equals chunk_text length) — use chunk.spans(), the visible_spans vector, span.is_visible() and compare the single span's covered range/length to chunk_text length; update the same check in the other block (lines ~276-293) so both places verify full coverage before serializing as `Uniform`.crates/grida-canvas/src/svg/from_usvg_tree.rs-251-265 (1)
251-265:⚠️ Potential issue | 🟠 MajorUniform chunks still drop most typography.
The attributed branch preserves
font_weight,font_style,font_family,letter_spacing, andword_spacing, but the uniform branch discards all of them. Single-span bold/italic/custom-family chunks will therefore import with default typography.Also applies to: 286-293
crates/grida-canvas/src/svg/from_usvg_tree.rs-226-229 (1)
226-229:⚠️ Potential issue | 🟠 MajorDon't trim imported chunk text.
trim()removes leading/trailing visible spaces, so the imported content and advance widths stop matching the SVG. Only skip chunks that are truly empty after visibility filtering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/svg/from_usvg_tree.rs` around lines 226 - 229, The code currently calls chunk_text.trim() and drops leading/trailing spaces (trimmed), which breaks alignment; remove the trim and the trimmed variable and instead check the actual chunk content after visibility filtering (e.g., replace let trimmed = chunk_text.trim(); if trimmed.is_empty() { continue; } with a simple if chunk_text.is_empty() { continue; } so leading/trailing visible spaces in chunk_text are preserved while truly empty chunks are still skipped).crates/grida-canvas/src/painter/painter.rs-1669-1674 (1)
1669-1674:⚠️ Potential issue | 🟠 MajorDon't always paint attributed strokes behind the fill.
The uniform text path only does stroke-first rendering for
StrokeAlign::Outside. This branch paintspara_set.strokebefore the fill for every run, soCenter/Insidestrokes lose the portion that is supposed to sit on top of the glyph fill.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/painter/painter.rs` around lines 1669 - 1674, The current loop always paints para_set.stroke before the fill, which incorrectly hides Center/Inside strokes; change the order in the painting logic so you only paint stroke before the fill when the stroke's alignment is StrokeAlign::Outside (check the stroke's alignment enum variant), and for StrokeAlign::Center or StrokeAlign::Inside paint the fill first then call stroke.paint after fill_rc.borrow().paint; update the block that calls para_set.stroke.paint and fill_rc.borrow().paint accordingly to branch on stroke.align.crates/grida-canvas/src/io/io_grida_fbs.rs-4144-4156 (1)
4144-4156:⚠️ Potential issue | 🟠 MajorAvoid turning partial stroke overrides into concrete defaults.
stroke_geometryis emitted whenever eitherstroke_widthorstroke_alignis present, but the missing half is synthesized as0.0/Center. After decode, that becomes a real override instead of inheritance, so runs that only override one stroke field stop inheriting the other from the node defaults.🛠️ One safe fallback
- let sg_offset = if run.stroke_width.is_some() || run.stroke_align.is_some() { - Some(encode_stroke_geometry( - fbb, - &StrokeStyle { - stroke_align: run.stroke_align.unwrap_or(StrokeAlign::Center), - stroke_cap: StrokeCap::Butt, - stroke_join: StrokeJoin::Miter, - stroke_miter_limit: StrokeMiterLimit::default(), - stroke_dash_array: None, - }, - run.stroke_width.unwrap_or(0.0), - None, - )) - } else { - None - }; + let sg_offset = match (run.stroke_width, run.stroke_align) { + (Some(stroke_width), Some(stroke_align)) => Some(encode_stroke_geometry( + fbb, + &StrokeStyle { + stroke_align, + stroke_cap: StrokeCap::Butt, + stroke_join: StrokeJoin::Miter, + stroke_miter_limit: StrokeMiterLimit::default(), + stroke_dash_array: None, + }, + stroke_width, + None, + )), + _ => None, + };If partial per-run inheritance is supposed to be supported, the schema needs separate optional fields instead of a single all-or-nothing
stroke_geometry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/io/io_grida_fbs.rs` around lines 4144 - 4156, The code currently emits a concrete stroke_geometry when either run.stroke_width or run.stroke_align is present, synthesizing the missing value (0.0/Center) and breaking inheritance; change the emission logic so stroke_geometry is only created when both per-run stroke fields are intentionally provided (e.g., require run.stroke_width.is_some() && run.stroke_align.is_some()), or alternatively extend encode_stroke_geometry/StrokeStyle to accept Option-valued fields and encode them as optional subfields instead of filling defaults—update the sg_offset assignment around encode_stroke_geometry and the StrokeStyle usage accordingly to preserve per-run partial inheritance.crates/grida-canvas/src/painter/painter.rs-1629-1638 (1)
1629-1638:⚠️ Potential issue | 🟠 MajorUse
text_layer.base.idconsistently for the paragraph-cache key.Line 1637 uses
text_layer.id, but the rest of the code usestext_layer.base.idfor the same purpose (lines 1716, 2091). Both fields exist onPainterPictureTextLayerand are currently set to the same value at construction, but using different fields as cache keys will cause lookups to diverge and become fragile. Align with the established pattern and usetext_layer.base.id:Diff
if let Some(sc) = self.scene_cache { sc.paragraph.borrow_mut().measure_attributed( attr, &text_layer.text_align, &text_layer.max_lines, &text_layer.ellipsis.clone(), text_layer.width, self.fonts, - Some(&text_layer.id), + Some(&text_layer.base.id), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/painter/painter.rs` around lines 1629 - 1638, The paragraph cache key usage is inconsistent: change the call to paragraph.measure_attributed that currently passes text_layer.id to instead pass text_layer.base.id so it matches other usages (e.g., in the paragraph cache lookups elsewhere) — locate the block where self.scene_cache is used and paragraph.borrow_mut().measure_attributed(...) is called and replace the id argument with text_layer.base.id to ensure consistent cache lookups for PainterPictureTextLayer.crates/grida-canvas/src/svg/pack.rs-249-255 (1)
249-255:⚠️ Potential issue | 🟠 MajorPer-run gradient paints need text bounds here.
These conversions always call
into_paint_with_opacity(None), soobjectBoundingBoxgradients skip the normalization logic fromcg/svg.rs. Paths already pass real bounds; attributed text runs will import the same gradients with the wrong scale/offset. Please thread chunk/text bounds into this helper, or explicitly reject gradient text paints until that data is available.Also applies to: 296-305
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/svg/pack.rs` around lines 249 - 255, The helper append_attributed_text_node is converting paints using into_paint_with_opacity(None), which causes objectBoundingBox gradients to be normalized with wrong dimensions for attributed text runs; update append_attributed_text_node (and the similar block at the area corresponding to lines ~296-305) to accept and thread the chunk/text bounds (from IRSVGAttributedTextChunk) into the paint conversion so you call into_paint_with_opacity(Some(bounds)) (or otherwise pass the text box to the cg/svg.rs normalization logic), or, if bounds are not available, explicitly detect gradient paints on text and reject/skip them with a clear error path. Ensure you reference IRSVGAttributedTextChunk when plumbing the bounds and update any call sites of append_attributed_text_node to provide the new bounds parameter.crates/grida-canvas/src/runtime/scene.rs-609-624 (1)
609-624:⚠️ Potential issue | 🟠 MajorClear stale attributed state on plain-text updates.
update_text_layer()only overwritestl.attributed_stringwhenSome(...)is passed. If a layer was updated throughupdate_layer_attributed_text()and later goes throughupdate_layer_text(), the old run map stays attached to the newtl.text, which reintroduces the same text/run divergence this API is trying to avoid.💡 Minimal fix
fn update_text_layer( tl: &mut crate::painter::layer::PainterPictureTextLayer, text: &str, attributed: &Option<crate::cg::types::AttributedString>, ) { tl.text = text.to_owned(); - if let Some(ref attr) = attributed { - tl.attributed_string = Some(attr.clone()); - } + tl.attributed_string = attributed.clone(); }I'd also make
update_layer_attributed_text()assert that the plain-text argument matches the attributed payload.Also applies to: 635-643
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/runtime/scene.rs` around lines 609 - 624, The plain-text updater leaves stale runs attached because update_layer_text_inner(currently called by update_layer_text(node_id, text, None)) only writes tl.attributed_string when Some(attributed) is passed; change update_layer_text_inner to clear tl.attributed_string (set it to None or an empty/cleared state) when the attributed argument is None so old runs are removed, and add an assert in update_layer_attributed_text(node_id, text, attributed) that text == attributed.string (or the attributed payload's text field) to ensure callers pass matching strings; apply the same clear/assert fix for the other occurrence around update_layer_text_inner usages mentioned (lines ~635-643).crates/grida-canvas/src/svg/pack.rs-262-270 (1)
262-270:⚠️ Potential issue | 🟠 MajorDon't make attributed-text positioning depend on run order.
The baseline correction uses the first run's font size. If the chunk starts with a small span and later contains a much larger one, the whole imported line is shifted too low; simply reordering the same spans changes geometry. Until this uses real ascent metrics, basing the temporary correction on the max run size is a safer approximation than
runs[0].💡 Safer interim fix
- let first_font_size = chunk - .runs - .first() - .map(|r| r.font_size) - .unwrap_or(16.0); + let first_font_size = chunk.runs.first().map(|r| r.font_size).unwrap_or(16.0); + let baseline_font_size = chunk + .runs + .iter() + .map(|r| r.font_size) + .fold(first_font_size, f32::max); let mut adjusted_transform = transform; // FIXME(svg text): baseline -> top-left conversion needs proper font metrics. - adjusted_transform.translate(0.0, -first_font_size); + adjusted_transform.translate(0.0, -baseline_font_size);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/svg/pack.rs` around lines 262 - 270, The baseline correction currently uses the first run's font size (computed as first_font_size from chunk.runs.first()) which makes layout depend on run order; change the logic to compute a safe approximation using the maximum font size across chunk.runs (e.g., iterate chunk.runs and take max of r.font_size) and use that max value for the translate call on adjusted_transform (replace first_font_size with max_font_size) so the temporary baseline shift is not order-dependent until real ascent metrics are used.crates/grida-canvas/src/text/attributed_paragraph.rs-150-158 (1)
150-158:⚠️ Potential issue | 🟠 MajorUse the measured paragraph height when resolving paints.
Both passes resolve gradients/image paints against
(width, width). For any non-square paragraph, that gives shaders and image fills the wrong local bounds, so vertical gradients and image-based text paints are transformed incorrectly. The caller already measures attributed text first, so thread the measured height into this builder and use(width, height)instead.Also applies to: 194-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/text/attributed_paragraph.rs` around lines 150 - 158, The paragraph paint resolution is using a square bound (width, width) which distorts gradients/images for non-square paragraphs; update the builder to accept the measured paragraph height and pass (width, height) into resolve_fill_paint instead of (width, width) wherever fills are resolved (e.g., the block around run.fills handling that calls resolve_fill_paint and the similar site at the later block around lines 194-197). Locate the functions/variables run, effective_fills, active_fills, borrowed, and resolve_fill_paint in attributed_paragraph.rs and thread the measured height through the builder API so both calls use (width, height) to compute correct local bounds for shaders and image paints.crates/grida-canvas/src/text_edit/attributed_text/conv.rs-243-248 (1)
243-248:⚠️ Potential issue | 🟠 MajorThis conversion bypasses the editor’s newline-normalization contract.
AttributedTextis documented as LF-normalized, but this path forwards rawattr.textintofrom_parts(). If scene/format I/O hands you CR or CRLF text, the editor keeps byte ranges for the unnormalized string and later offset math goes off. Normalize before run offsets are computed, or reject non-normalized input at this boundary instead of silently accepting it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/text_edit/attributed_text/conv.rs` around lines 243 - 248, The conversion currently forwards raw attr.text into AttributedText::from_parts and computes runs from attr.runs, which can leave CR/CRLF bytes unnormalized and break run offsets; fix by normalizing newlines to LF before any run-offset computation or call to AttributedText::from_parts: compute let normalized_text = normalize_newlines(&attr.text) (or return an Err if you prefer strict validation), then map attr.runs -> StyledRun based on the normalized_text (so the Into::into conversion uses the normalized string or adjust offsets after normalization), compute default_style from the resulting runs, and finally call AttributedText::from_parts(normalized_text, default_style, ...).crates/grida-canvas/src/text_edit_session.rs-83-87 (1)
83-87:⚠️ Potential issue | 🟠 MajorStyle-only attributed edits currently look unmodified.
commit_full()reusesis_modified(), butis_modified()only compares plain text againstoriginal_text. If a user changes bold/fill/stroke/etc. without changing characters,modifiedcomes backfalse, so caller-side commit logic can silently drop the attributed update.Suggested fix
pub struct ActiveTextEdit { /// The generic text editing session with ParagraphCacheLayout. pub session: CanvasTextEditSession, @@ /// Original text at session start (for commit comparison). original_text: String, + original_attributed: Option<AttributedText>, /// Whether this is an attributed text node (vs. a plain TextSpan). is_attributed: bool, } @@ Self { session, node_id, original_text: text.to_owned(), + original_attributed: None, is_attributed: false, } } @@ - let original_text = content.text().to_owned(); - let mut session = TextEditSession::with_content(layout, content); + let original_text = content.text().to_owned(); + let original_attributed = content.clone(); + let mut session = TextEditSession::with_content(layout, content); session.select_all(); Self { session, node_id, original_text, + original_attributed: Some(original_attributed), is_attributed: true, } @@ pub fn is_modified(&self) -> bool { - self.session.state.text != self.original_text + self.original_attributed + .as_ref() + .map(|original| original != &self.session.content) + .unwrap_or_else(|| self.session.state.text != self.original_text) }Also applies to: 127-145, 175-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/text_edit_session.rs` around lines 83 - 87, is_modified() currently only compares plain text against original_text so attribute-only changes (bold/color/fill/stroke) are treated as unmodified and commit_full() can drop attributed updates; fix by adding an attributes snapshot at session start (e.g., original_attributes or original_spans) and update is_modified() to return true when either text != original_text OR the current attributed metadata/spans differ from the original snapshot (when is_attributed is true); update the TextEditSession constructor to capture original attributes and ensure commit_full() continues to call is_modified() so attributed-only edits are preserved.crates/grida-canvas/src/text/attributed_paragraph.rs-84-97 (1)
84-97:⚠️ Potential issue | 🟠 MajorNode-level stroke inheritance is missing.
This builder only accepts
default_fills, and the stroke pass is created solely fromrun.strokes. That meansAttributedTextNodeRec.strokes/stroke_width/stroke_aligncan never render unless every run duplicates that stroke metadata, so node-level outlined attributed text currently paints as fill-only.Also applies to: 121-125, 175-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/text/attributed_paragraph.rs` around lines 84 - 97, The builder build_attributed_paragraph_with_images (and its inner helper build_attributed_paragraph_inner) only accepts default_fills and ignores node-level stroke metadata, so AttributedTextNodeRec.strokes, stroke_width and stroke_align are never applied unless every run duplicates them; update the paragraph construction to merge node-level strokes into the per-run stroke pass when runs lack strokes: when building the stroke pass for each run, if run.strokes is empty or None, fall back to the node-level AttributedTextNodeRec.strokes/stroke_width/stroke_align (or the equivalent node-level props passed into build_attributed_paragraph_inner), combining with run-level values where present so node-level outlines render correctly; locate code that constructs run fills/strokes in build_attributed_paragraph_inner (and similar spots around the other ranges mentioned) and apply this fallback/merge logic.crates/grida-dev/src/platform/native_application.rs-504-529 (1)
504-529:⚠️ Potential issue | 🟠 MajorEnable IME when
KeyDownenters text edit.
set_ime_allowed(true)only exists in the mouse path. Ifhandle_surface_event(KeyDown)activates editing, this branch starts acceptingTextInputwith IME still disabled, so composition never starts for that entry path.Suggested fix
if response.needs_redraw { self.window.request_redraw(); } } - // Detect text edit exit (e.g. Escape) and disable IME. - if is_editing && !self.app.text_edit_is_active() { - self.window.set_ime_allowed(false); - } + let is_editing_now = self.app.text_edit_is_active(); + if is_editing_now && !is_editing { + self.window.set_ime_allowed(true); + } else if !is_editing_now && is_editing { + self.window.set_ime_allowed(false); + } // If text editing is active, also route text input for🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/platform/native_application.rs` around lines 504 - 529, When handling KeyDown in native_application.rs, enable IME when a KeyDown causes text editing to start: after the call to self.app.handle_surface_event(surface_event) (the branch using winit_key_to_surface_key_down and response.needs_redraw), check if editing was not active before (is_editing == false) and is now active via self.app.text_edit_is_active(), and if so call self.window.set_ime_allowed(true). Mirror the existing text-edit-exit logic (which calls self.window.set_ime_allowed(false)) so IME is enabled for KeyDown-initiated edits; use the same symbols: is_editing, winit_key_to_surface_key_down, handle_surface_event, text_edit_is_active, and set_ime_allowed.crates/grida-canvas/src/window/application.rs-2367-2387 (1)
2367-2387:⚠️ Potential issue | 🟠 MajorIME preedit is sending mismatched text and attributed runs.
Lines 2370-2381 build
display_textwith the transient preedit string, but Lines 2405-2408 still convert the committedte.session.contentinto the attributed payload. During composition,update_layer_attributed_texttherefore receives text and runs that do not describe the same buffer, which can mis-style or invalidate offsets for the preedit span.Also applies to: 2405-2414
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/window/application.rs` around lines 2367 - 2387, The IME preedit path builds a display_text that merges committed text and preedit but still constructs attributed runs from the original committed te.session.content, causing mismatched text and runs; update the logic around display_text and attributed so that when te.session.preedit() is Some(non-empty) you construct an attributed payload that corresponds to the same combined buffer (i.e., apply the preedit into the content before converting to crate::cg::types::AttributedString) so update_layer_attributed_text receives matching text and runs (refer to display_text, attributed, te.session.content, te.session.preedit, and update_layer_attributed_text).crates/grida-canvas/src/window/application.rs-2266-2270 (1)
2266-2270:⚠️ Potential issue | 🟠 MajorText commits currently desync the live scene from
document_json.Lines 2266-2270 and 2279-2300 apply the edit directly to
scene.graph, butUnknownTargetApplication::process_document_transactionslater patchesself.document_jsonand reloads from that JSON snapshot. After editing a JSON-backed document, the next transaction batch can replay on stale data and drop the committed text.Also applies to: 2279-2300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/window/application.rs` around lines 2266 - 2270, The text-commit path applies edits to scene.graph (via te.commit_full() and apply_text_edit_commit) but leaves self.document_json stale, so later UnknownTargetApplication::process_document_transactions reloads from old JSON and loses the commit; update the JSON-backed state when committing text edits by applying the same edit to self.document_json (or set a flag on the session/document that prevents an immediate JSON reload) whenever apply_text_edit_commit runs, ensuring the serialized document snapshot and scene.graph stay in sync; locate te.commit_full(), apply_text_edit_commit, and UnknownTargetApplication::process_document_transactions to implement the JSON patch or skip-reload behavior.crates/grida-canvas/src/window/application.rs-1667-1695 (1)
1667-1695:⚠️ Potential issue | 🟠 MajorDon't seed every
TextSpansession with black fills.Line 1674 discards the node's actual fill stack and replaces it with
CGColor::BLACK. Any edit flow that promotes the session into attributed content while editing now starts from the wrong inherited paint state, so color-preserving operations can drift from the node being edited.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/window/application.rs` around lines 1667 - 1695, The code is seeding every TextSpan edit session with a hardcoded black fill (vec![Paint::from(CGColor::BLACK)]) which discards the node's actual fill stack; change the fills passed into ActiveTextEdit::new to use the TextSpan's real fills/inherited paint state instead: extract the fill stack from the tspan (e.g., tspan.fills or tspan.fill_stack or the node's paint information) and pass that vector of Paints (falling back to the node's computed/inherited paints if the tspan has none) into ActiveTextEdit::new rather than always using black.crates/grida-canvas/src/cg/types.rs-1928-1969 (1)
1928-1969:⚠️ Potential issue | 🟠 MajorValidate run invariants outside debug builds too.
Lines 1931-1967 only use
debug_assert!, soAttributedString::from_runs()stops enforcing its invariants in release builds. That lets malformed or empty run tables through, and later code likeAttributedString::run_text()can panic on invalid byte ranges. It also meansAttributedStringBuilder::build()can returnruns: []in release despite its docs.Also applies to: 2067-2072
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/cg/types.rs` around lines 1928 - 1969, AttributedString::from_runs currently only validates invariants with debug_assert!, which disables checks in release builds; change those debug_assert!/debug_assert_eq! calls to always-on checks (e.g., assert!/assert_eq! or explicit if !cond { panic!(...) }) so malformed runs are rejected in release too, and apply the same replacement for the corresponding validation in AttributedStringBuilder::build so empty or invalid run tables cannot slip through; reference the functions AttributedString::from_runs and AttributedStringBuilder::build and replace each debug_assert*/debug_assert_eq* there with an always-active check that produces the same error message.crates/grida-canvas/src/window/application.rs-1645-1651 (1)
1645-1651: 🛠️ Refactor suggestion | 🟠 MajorHide internal
NodeIdbehind the existing user-ID boundary.Line 1651 makes
UnknownTargetApplication::text_edit_enter_by_idpart of the public surface, but it accepts an internalNodeIdinstead of the stable string ID the rest of this application API uses. If this is only host/internal plumbing,pub(crate)would be a better fit; otherwise resolve fromUserNodeIdinside the method.As per coding guidelines "Use
UserNodeId(String) for public APIs to provide stable, serializable external identifiers" and "Maintain bidirectional mapping between NodeId and UserNodeId at the application layer for API boundary".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/window/application.rs` around lines 1645 - 1651, The public method UnknownTargetApplication::text_edit_enter_by_id currently exposes an internal NodeId; either make it non-public (change to pub(crate)) if it’s only internal plumbing, or change its signature to accept the stable external identifier (UserNodeId / String) and resolve to the internal NodeId inside the method using the application’s bidirectional mapping (the same map used elsewhere to convert UserNodeId -> NodeId). Update usages accordingly: if you keep it pub(crate) no external callers need changing; if you accept UserNodeId, call the resolver at the top of text_edit_enter_by_id, handle the missing mapping by returning false, and then proceed with the existing logic that uses NodeId.
| // Set up ImageRepository with the checkerboard | ||
| let store = Arc::new(Mutex::new(ByteStore::new())); | ||
| let checker_hash: u64 = 0xC4EC; | ||
| store | ||
| .lock() | ||
| .unwrap() | ||
| .insert(checker_hash, checker_png.as_bytes().to_vec()); | ||
| let mut images = ImageRepository::new(store); | ||
| images.insert("checker".to_string(), checker_hash); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== attributed paragraph image plumbing ==="
rg -n -C3 'fn build_attributed_paragraph|fn build_attributed_paragraph_with_images|ImageRepository|Paint::Image|ResourceRef::RID' \
crates/grida-canvas/src \
crates/grida-canvas/examples/golden_type_attributed.rsRepository: gridaco/grida
Length of output: 49608
🏁 Script executed:
sed -n '950,975p' crates/grida-canvas/examples/golden_type_attributed.rsRepository: gridaco/grida
Length of output: 723
Use build_attributed_paragraph_with_images() to wire the ImageRepository to the paragraph builder.
The example creates and populates images with the checker texture, but the call on line 967 uses build_attributed_paragraph(), which does not accept an ImageRepository parameter. This means Paint::Image fills on the attributed text will not be resolved. Switch to build_attributed_paragraph_with_images() and pass images along with the other required parameters (fonts, default_fills).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/examples/golden_type_attributed.rs` around lines 927 -
935, The example populates an ImageRepository (images) but still calls
build_attributed_paragraph(), which doesn't accept images; replace that call
with build_attributed_paragraph_with_images() and pass the populated
ImageRepository (images) along with the existing fonts and default_fills
parameters so Paint::Image fills are resolved; locate the paragraph builder call
(build_attributed_paragraph) and swap it to
build_attributed_paragraph_with_images(images, fonts, default_fills, ...)
keeping other arguments the same.
| boolean: "none", | ||
| group: "none", | ||
| tspan: "color", | ||
| attrib: "color", |
There was a problem hiding this comment.
"attrib" fill mapping is unreachable because the renderer switch still rejects "attrib".
Line 214 adds fill behavior for "attrib", but the switch (node.type) path above still throws Unknown node type for "attrib" before this map is used.
🐛 Suggested fix
case "iframe":
case "richtext": {
+ case "attrib": {
return ReactNodeRenderers[node.type];
}📝 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.
| attrib: "color", | |
| case "iframe": | |
| case "richtext": | |
| case "attrib": { | |
| return ReactNodeRenderers[node.type]; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@editor/grida-canvas-react-renderer-dom/nodes/node.tsx` at line 214, The
switch over node.type currently throws for the "attrib" node before the later
fill mapping (attrib: "color") can be used; update the switch (node.type) branch
handling in the renderer function (the switch block that throws "Unknown node
type") to include a case for "attrib" (or otherwise allow it through) so the
fill mapping entry attrib: "color" is reachable and used; specifically, add a
case "attrib" that delegates to the same handling as other simple fill nodes or
remove the throw for "attrib" so the mapping can be applied.
softmarshmallow
changed the title
…djustments - Updated height handling in Style conversion to use explicit height if set, otherwise default to auto. - Improved image URL extraction for AttributedText nodes by collecting fills and strokes from attributed string runs. - Refactored text rendering logic to ensure proper handling of empty text chunks and consistent fill/stroke attributes. These changes enhance the layout and rendering capabilities for attributed text, ensuring better visual fidelity and resource management.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/grida-canvas/src/svg/from_usvg_tree.rs (1)
164-167:⚠️ Potential issue | 🟠 MajorNon-visible text is still copied into the emitted chunk strings.
You filter spans by
is_visible(), butcombined_text, the attributed chunk text, and the uniform chunk text all still reuse the originalchunk_textunchanged. A chunk with one visible span plus hidden content will render the hidden suffix under the visible style, andvisible_spans.is_empty()currently still falls through and emits a text node. Rebuild the emitted text from the visible ranges (or drop the chunk/node when nothing is visible) and keeptext_contentin sync.Also applies to: 234-245, 267-272, 284-286, 295-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/svg/from_usvg_tree.rs` around lines 164 - 167, The emitted chunk strings currently reuse the original chunk_text (e.g., combined_text built from for chunk in text.chunks()) even when some spans are hidden, causing hidden text to be rendered; update the logic that builds combined_text, the attributed chunk text, and the uniform chunk text to reconstruct their text from only the visible span ranges (or skip emitting the chunk/node entirely when visible_spans.is_empty()), and ensure text_content is updated to match the rebuilt visible-only text; apply the same fix to the other similar blocks noted (around lines handling visible_spans and text_content at the other ranges).
🧹 Nitpick comments (1)
packages/grida-canvas-io/format.ts (1)
5665-5765: Helper is good, but style decode is still duplicated innodeTypes.text.
decodeTextStyleRecis a solid extraction. Consider wiringnodeTypes.textto use it too, so TextSpan and AttributedText stay behaviorally aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/grida-canvas-io/format.ts` around lines 5665 - 5765, The pull request has duplicated text-style decoding in nodeTypes.text; replace that duplicate logic by calling the extracted helper decodeTextStyleRec so TextSpan/AttributedText use the same behavior: locate the decoding block in the nodeTypes.text handler and remove the inline/duplicated field parsing, invoke decodeTextStyleRec(textStyleRec) (the same function shown) and merge its returned ITextStyle into the node's output; ensure the format/structs symbols referenced by decodeTextStyleRec are available in that module (import or reference the same format namespace) and remove the old duplicated variables and branches to avoid divergence.
🤖 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-canvas/src/runtime/scene.rs`:
- Around line 635-644: The helper update_text_layer currently only sets
tl.attributed_string when attributed is Some, leaving stale attributed runs if
attributed is None; update it so tl.attributed_string is explicitly cleared when
attributed is None (e.g., set tl.attributed_string = attributed.clone() or set
to None in the else branch) while still assigning tl.text, referencing
update_text_layer and the PainterPictureTextLayer::attributed_string field to
locate the change.
In `@crates/grida-canvas/src/svg/from_usvg_tree.rs`:
- Line 235: The code currently only creates an IRSVGTextChunk::Attributed when
visible_spans.len() > 1 (has_style_variation), which loses per-span attributes
for single-span runs; change the promotion logic so that any visible span that
carries attributes the uniform IR cannot represent (font
family/weight/style/spacing) causes the chunk to be IRSVGTextChunk::Attributed
even if there is only one visible span. Concretely, replace the boolean check
that sets has_style_variation (and the downstream branch that chooses the
uniform path) with a test over visible_spans that returns true if any span has
non-default or non-uniformable attributes (e.g., examine the same attributes
inspected in the Attributed construction used around the 248-262 block) so those
attributes are preserved for single-span chunks as well.
In `@packages/grida-canvas-io-figma/lib.ts`:
- Around line 1788-1807: The run push currently only emits fill_paints when the
converted fills array is non-empty and ignores per-run strokes, so per-run
overrides can't clear the base fills and any stroke overrides are lost; update
the logic around overrideFills and runs.push to (1) treat overrideFills presence
as intent — include fill_paints on the run even when the converted array is
empty (to allow clearing), and (2) likewise read an overrideStrokes (e.g. from
(overrideStyle as { strokes?: figrest.Paint[] }).strokes), convert each via
convertPaint(context, imageRefsUsed) and include a stroke_paints property on the
run when strokes were present (even if empty), ensuring you reference
convertPaint, overrideStyle, overrideFills, and the runs.push site so the
per-run stroke and empty-fill semantics are preserved.
- Around line 1781-1786: When merging node.style and overrideStyle into
mergedRestStyle before calling restStyleToGrida, ensure any inherited
fontPostScriptName is cleared whenever the override changes the font face;
detect if overrideStyle contains facial font properties (at minimum fontFamily
or fontWeight; also consider fontStyle/fontStretch) and if so delete
mergedRestStyle.fontPostScriptName (or set it to undefined) so restStyleToGrida
and the emitted run cannot mix the old PostScript name with the new face; update
the block around mergedRestStyle/gridaStyle to perform this check and clear.
In `@packages/grida-canvas-io/format.ts`:
- Around line 5794-5823: The decode currently falls back to raw byte offsets
when b2c mapping is missing (in the styledRuns push using b2c.get(byteStart) ??
byteStart and b2c.get(byteEnd) ?? byteEnd); change this to map to the nearest
valid character boundary and clamp to valid range: use
byteToCharOffsets(decodedText) to locate the closest mapped char index for
byteStart/byteEnd (e.g., nearest lower or nearest higher mapped entry), then
clamp results between 0 and decodedText.length and enforce start =
Math.min(mappedStart, mappedEnd), end = Math.max(mappedStart, mappedEnd) so
start <= end; update the push site that constructs start/end to use this
clamped/mapped logic instead of the raw-byte fallback.
- Around line 1127-1173: The code indexes c2b with run.start/run.end without
validation, which can produce out-of-range or reversed offsets; before calling
fbs.StyledTextRunItem.addStart/addEnd, clamp and validate run.start and run.end
against the source text length (use node.text.length or 0 when text is missing),
ensure start = Math.max(0, Math.min(start, textLen)) and end = Math.max(0,
Math.min(end, textLen)), and if start > end swap or set end = start; then use
these sanitized indices when looking up c2b (or fallback to the original indices
when c2b is undefined) so that encodeTextStyleRec, charToByteOffsets, and
fbs.StyledTextRunItem receive safe UTF-8 byte offsets.
- Around line 22-43: charToByteOffsets misclassifies unpaired high surrogates as
4-byte sequences; update the logic in charToByteOffsets to check that a high
surrogate (0xD800–0xDBFF) is actually followed by a valid low surrogate
(0xDC00–0xDFFF) before treating it as a 4-byte UTF‑8 pair—if the next code unit
is a valid low surrogate, increment byteIdx by 4, advance i, and set the map for
the low surrogate appropriately; otherwise treat the unpaired high surrogate as
U+FFFD and increment byteIdx by 3 without skipping the next code unit. Also
apply the same paired-vs-unpaired check and byte-length handling in the
corresponding reverse/related mapping logic referenced in the diff (the same
surrogate-handling block around lines 50–68) so unpaired surrogates consistently
map to 3 bytes.
---
Outside diff comments:
In `@crates/grida-canvas/src/svg/from_usvg_tree.rs`:
- Around line 164-167: The emitted chunk strings currently reuse the original
chunk_text (e.g., combined_text built from for chunk in text.chunks()) even when
some spans are hidden, causing hidden text to be rendered; update the logic that
builds combined_text, the attributed chunk text, and the uniform chunk text to
reconstruct their text from only the visible span ranges (or skip emitting the
chunk/node entirely when visible_spans.is_empty()), and ensure text_content is
updated to match the rebuilt visible-only text; apply the same fix to the other
similar blocks noted (around lines handling visible_spans and text_content at
the other ranges).
---
Nitpick comments:
In `@packages/grida-canvas-io/format.ts`:
- Around line 5665-5765: The pull request has duplicated text-style decoding in
nodeTypes.text; replace that duplicate logic by calling the extracted helper
decodeTextStyleRec so TextSpan/AttributedText use the same behavior: locate the
decoding block in the nodeTypes.text handler and remove the inline/duplicated
field parsing, invoke decodeTextStyleRec(textStyleRec) (the same function shown)
and merge its returned ITextStyle into the node's output; ensure the
format/structs symbols referenced by decodeTextStyleRec are available in that
module (import or reference the same format namespace) and remove the old
duplicated variables and branches to avoid divergence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b03ec3c-a207-407d-a8f3-9cd20d68526f
📒 Files selected for processing (10)
crates/grida-canvas/src/layout/into_taffy.rscrates/grida-canvas/src/resources/mod.rscrates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/src/svg/from_usvg_tree.rscrates/grida-canvas/src/text_edit/session_tests.rspackages/grida-canvas-io-figma/__tests__/iofigma.rest-api.attributed-text.test.tspackages/grida-canvas-io-figma/lib.tspackages/grida-canvas-io/__tests__/format-roundtrip.test.tspackages/grida-canvas-io/format.tspackages/grida-canvas-schema/grida.ts
✅ Files skipped from review due to trivial changes (3)
- crates/grida-canvas/src/text_edit/session_tests.rs
- crates/grida-canvas/src/layout/into_taffy.rs
- packages/grida-canvas-io/tests/format-roundtrip.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/grida-canvas/src/resources/mod.rs
- packages/grida-canvas-schema/grida.ts
| fn update_text_layer( | ||
| tl: &mut crate::painter::layer::PainterPictureTextLayer, | ||
| text: &str, | ||
| attributed: &Option<crate::cg::types::AttributedString>, | ||
| ) { | ||
| tl.text = text.to_owned(); | ||
| if let Some(ref attr) = attributed { | ||
| tl.attributed_string = Some(attr.clone()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider clearing attributed_string when updating plain text.
The helper only sets attributed_string when attributed is Some, but doesn't clear it when None. If update_layer_text is called on a node that previously had an attributed string, the stale runs could cause byte offset mismatches with the new text.
This may be intentional (assuming callers use the correct method), but explicitly clearing could be safer:
🛡️ Defensive fix to clear attributed string
fn update_text_layer(
tl: &mut crate::painter::layer::PainterPictureTextLayer,
text: &str,
attributed: &Option<crate::cg::types::AttributedString>,
) {
tl.text = text.to_owned();
- if let Some(ref attr) = attributed {
- tl.attributed_string = Some(attr.clone());
- }
+ tl.attributed_string = attributed.clone();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/runtime/scene.rs` around lines 635 - 644, The helper
update_text_layer currently only sets tl.attributed_string when attributed is
Some, leaving stale attributed runs if attributed is None; update it so
tl.attributed_string is explicitly cleared when attributed is None (e.g., set
tl.attributed_string = attributed.clone() or set to None in the else branch)
while still assigning tl.text, referencing update_text_layer and the
PainterPictureTextLayer::attributed_string field to locate the change.
| let chunk_transform = CGTransform2D::from(chunk_affine); | ||
|
|
||
| let visible_spans: Vec<_> = chunk.spans().iter().filter(|s| s.is_visible()).collect(); | ||
| let has_style_variation = visible_spans.len() > 1; |
There was a problem hiding this comment.
Single-span styled chunks still lose font family/weight/style/spacing.
Line 235 only promotes a chunk to IRSVGTextChunk::Attributed when there is more than one visible span. But the uniform path on Lines 284-291 cannot carry the extra attributes you preserve on Lines 248-262, so a chunk with one visible bold/italic/custom-font/span-spacing run will still round-trip as default-styled text. Please emit an attributed chunk whenever the visible span uses attributes the uniform IR cannot encode, not only when there are multiple visible spans.
Also applies to: 248-262, 284-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/svg/from_usvg_tree.rs` at line 235, The code
currently only creates an IRSVGTextChunk::Attributed when visible_spans.len() >
1 (has_style_variation), which loses per-span attributes for single-span runs;
change the promotion logic so that any visible span that carries attributes the
uniform IR cannot represent (font family/weight/style/spacing) causes the chunk
to be IRSVGTextChunk::Attributed even if there is only one visible span.
Concretely, replace the boolean check that sets has_style_variation (and the
downstream branch that chooses the uniform path) with a test over visible_spans
that returns true if any span has non-default or non-uniformable attributes
(e.g., examine the same attributes inspected in the Attributed construction used
around the 248-262 block) so those attributes are preserved for single-span
chunks as well.
| // Merge base style with override | ||
| const mergedRestStyle = { | ||
| ...node.style, | ||
| ...overrideStyle, | ||
| }; | ||
| const gridaStyle = restStyleToGrida(mergedRestStyle); |
There was a problem hiding this comment.
Clear inherited fontPostScriptName when a run overrides the face.
This merge keeps the base postscript name for overrides like { fontWeight: 700 }, so the emitted run can end up with an impossible tuple such as font_weight=700 plus font_postscript_name="...Regular". That can make downstream font resolution keep the old face instead of the override.
Suggested fix
- const mergedRestStyle = {
- ...node.style,
- ...overrideStyle,
- };
+ const overrideTouchesFace =
+ "fontFamily" in overrideStyle ||
+ "fontWeight" in overrideStyle ||
+ "italic" in overrideStyle;
+
+ const mergedRestStyle = {
+ ...node.style,
+ ...overrideStyle,
+ ...(overrideTouchesFace &&
+ !("fontPostScriptName" in overrideStyle)
+ ? { fontPostScriptName: undefined }
+ : {}),
+ };📝 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.
| // Merge base style with override | |
| const mergedRestStyle = { | |
| ...node.style, | |
| ...overrideStyle, | |
| }; | |
| const gridaStyle = restStyleToGrida(mergedRestStyle); | |
| // Merge base style with override | |
| const overrideTouchesFace = | |
| "fontFamily" in overrideStyle || | |
| "fontWeight" in overrideStyle || | |
| "italic" in overrideStyle; | |
| const mergedRestStyle = { | |
| ...node.style, | |
| ...overrideStyle, | |
| ...(overrideTouchesFace && | |
| !("fontPostScriptName" in overrideStyle) | |
| ? { fontPostScriptName: undefined } | |
| : {}), | |
| }; | |
| const gridaStyle = restStyleToGrida(mergedRestStyle); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/grida-canvas-io-figma/lib.ts` around lines 1781 - 1786, When merging
node.style and overrideStyle into mergedRestStyle before calling
restStyleToGrida, ensure any inherited fontPostScriptName is cleared whenever
the override changes the font face; detect if overrideStyle contains facial font
properties (at minimum fontFamily or fontWeight; also consider
fontStyle/fontStretch) and if so delete mergedRestStyle.fontPostScriptName (or
set it to undefined) so restStyleToGrida and the emitted run cannot mix the old
PostScript name with the new face; update the block around
mergedRestStyle/gridaStyle to perform this check and clear.
| // Per-run fills from override | ||
| const overrideFills = ( | ||
| overrideStyle as { fills?: figrest.Paint[] } | ||
| ).fills; | ||
| const runFillPaints = overrideFills | ||
| ? overrideFills | ||
| .map((p) => | ||
| convertPaint(p, context, imageRefsUsed) | ||
| ) | ||
| .filter((p): p is cg.Paint => p !== undefined) | ||
| : undefined; | ||
|
|
||
| runs.push({ | ||
| start: runStart, | ||
| end: i, | ||
| style: gridaStyle, | ||
| ...(runFillPaints && runFillPaints.length > 0 | ||
| ? { fill_paints: runFillPaints } | ||
| : {}), | ||
| }); |
There was a problem hiding this comment.
Don't drop explicit run paint overrides.
This only emits fill_paints when the converted fill array is non-empty, and it ignores strokes entirely. A run therefore can't clear the base fill, and any per-run stroke styling assembled in styleOverrideTable is lost during import.
Suggested fix
- const overrideFills = (
- overrideStyle as { fills?: figrest.Paint[] }
- ).fills;
- const runFillPaints = overrideFills
- ? overrideFills
- .map((p) =>
- convertPaint(p, context, imageRefsUsed)
- )
- .filter((p): p is cg.Paint => p !== undefined)
- : undefined;
+ const hasFillOverride = Object.prototype.hasOwnProperty.call(
+ overrideStyle,
+ "fills"
+ );
+ const hasStrokeOverride = Object.prototype.hasOwnProperty.call(
+ overrideStyle,
+ "strokes"
+ );
+
+ const runFillPaints = hasFillOverride
+ ? (((overrideStyle as { fills?: figrest.Paint[] }).fills ??
+ [])
+ .map((p) => convertPaint(p, context, imageRefsUsed))
+ .filter((p): p is cg.Paint => p !== undefined))
+ : undefined;
+
+ const runStrokePaints = hasStrokeOverride
+ ? (((overrideStyle as { strokes?: figrest.Paint[] }).strokes ??
+ [])
+ .map((p) => convertPaint(p, context, imageRefsUsed))
+ .filter((p): p is cg.Paint => p !== undefined))
+ : undefined;
runs.push({
start: runStart,
end: i,
style: gridaStyle,
- ...(runFillPaints && runFillPaints.length > 0
- ? { fill_paints: runFillPaints }
- : {}),
+ ...(hasFillOverride
+ ? { fill_paints: runFillPaints ?? [] }
+ : {}),
+ ...(hasStrokeOverride
+ ? { stroke_paints: runStrokePaints ?? [] }
+ : {}),
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/grida-canvas-io-figma/lib.ts` around lines 1788 - 1807, The run push
currently only emits fill_paints when the converted fills array is non-empty and
ignores per-run strokes, so per-run overrides can't clear the base fills and any
stroke overrides are lost; update the logic around overrideFills and runs.push
to (1) treat overrideFills presence as intent — include fill_paints on the run
even when the converted array is empty (to allow clearing), and (2) likewise
read an overrideStrokes (e.g. from (overrideStyle as { strokes?: figrest.Paint[]
}).strokes), convert each via convertPaint(context, imageRefsUsed) and include a
stroke_paints property on the run when strokes were present (even if empty),
ensuring you reference convertPaint, overrideStyle, overrideFills, and the
runs.push site so the per-run stroke and empty-fill semantics are preserved.
| function charToByteOffsets(text: string): Uint32Array { | ||
| // Walk the JS string (UTF-16 code units) computing UTF-8 byte positions. | ||
| const map = new Uint32Array(text.length + 1); | ||
| let byteIdx = 0; | ||
| for (let i = 0; i < text.length; i++) { | ||
| map[i] = byteIdx; | ||
| const code = text.charCodeAt(i); | ||
| if (code < 0x80) { | ||
| byteIdx += 1; | ||
| } else if (code < 0x800) { | ||
| byteIdx += 2; | ||
| } else if (code >= 0xd800 && code <= 0xdbff) { | ||
| // High surrogate — this char + next form a surrogate pair (4 UTF-8 bytes). | ||
| byteIdx += 4; | ||
| i++; // skip low surrogate | ||
| map[i] = byteIdx; // low surrogate maps to byte *after* the 4-byte seq | ||
| } else { | ||
| byteIdx += 3; | ||
| } | ||
| } | ||
| map[text.length] = byteIdx; | ||
| return map; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript, how does TextEncoder encode an unpaired UTF-16 high surrogate (e.g. "\uD800"), and what UTF-8 byte length does it produce?
💡 Result:
TextEncoder encodes an unpaired UTF-16 high surrogate such as "\uD800" by replacing it with the Unicode replacement character U+FFFD (�) and then UTF‑8 encoding that. (encoding.spec.whatwg.org)
U+FFFD in UTF‑8 is the three bytes EF BF BD, so the output length is 3 bytes. (encoding.spec.whatwg.org)
Citations:
- 1: https://encoding.spec.whatwg.org/?utm_source=openai
- 2: https://encoding.spec.whatwg.org/?utm_source=openai
Unpaired high surrogates must be encoded as 3 bytes, not 4.
The code assumes every high surrogate (0xD800–0xDBFF) encodes as a 4-byte UTF-8 sequence, but unpaired high surrogates (those not followed by a valid low surrogate 0xDC00–0xDFFF) are replaced with the U+FFFD replacement character and encode as 3 bytes. This causes byte offset misalignment.
Suggested fix
function charToByteOffsets(text: string): Uint32Array {
const map = new Uint32Array(text.length + 1);
let byteIdx = 0;
for (let i = 0; i < text.length; i++) {
map[i] = byteIdx;
const code = text.charCodeAt(i);
if (code < 0x80) {
byteIdx += 1;
} else if (code < 0x800) {
byteIdx += 2;
} else if (code >= 0xd800 && code <= 0xdbff) {
const next = i + 1 < text.length ? text.charCodeAt(i + 1) : 0;
if (next >= 0xdc00 && next <= 0xdfff) {
byteIdx += 4;
i++;
map[i] = byteIdx;
} else {
byteIdx += 3;
}
} else {
byteIdx += 3;
}
}
map[text.length] = byteIdx;
return map;
}
function byteToCharOffsets(text: string): Map<number, number> {
const map = new Map<number, number>();
let byteIdx = 0;
for (let i = 0; i < text.length; i++) {
map.set(byteIdx, i);
const code = text.charCodeAt(i);
if (code < 0x80) {
byteIdx += 1;
} else if (code < 0x800) {
byteIdx += 2;
} else if (code >= 0xd800 && code <= 0xdbff) {
const next = i + 1 < text.length ? text.charCodeAt(i + 1) : 0;
if (next >= 0xdc00 && next <= 0xdfff) {
byteIdx += 4;
i++;
} else {
byteIdx += 3;
}
} else {
byteIdx += 3;
}
}
map.set(byteIdx, text.length);
return map;
}Applies to lines 22–43 and 50–68.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/grida-canvas-io/format.ts` around lines 22 - 43, charToByteOffsets
misclassifies unpaired high surrogates as 4-byte sequences; update the logic in
charToByteOffsets to check that a high surrogate (0xD800–0xDBFF) is actually
followed by a valid low surrogate (0xDC00–0xDFFF) before treating it as a 4-byte
UTF‑8 pair—if the next code unit is a valid low surrogate, increment byteIdx by
4, advance i, and set the map for the low surrogate appropriately; otherwise
treat the unpaired high surrogate as U+FFFD and increment byteIdx by 3 without
skipping the next code unit. Also apply the same paired-vs-unpaired check and
byte-length handling in the corresponding reverse/related mapping logic
referenced in the diff (the same surrogate-handling block around lines 50–68) so
unpaired surrogates consistently map to 3 bytes.
| // Encode styled_runs — convert char indices → UTF-8 byte offsets | ||
| const c2b = | ||
| node.text != null ? charToByteOffsets(node.text) : undefined; | ||
| const runOffsets: flatbuffers.Offset[] = []; | ||
| for (const run of node.styled_runs) { | ||
| const runStyleOffset = encodeTextStyleRec(builder, run.style); | ||
|
|
||
| // Per-run fill paints | ||
| const runFillPaintsFiltered = run.fill_paints?.filter(isPaint); | ||
| const runFillPaintsOffset = | ||
| runFillPaintsFiltered && runFillPaintsFiltered.length > 0 | ||
| ? format.paint.encode.fillPaints( | ||
| builder, | ||
| runFillPaintsFiltered, | ||
| fbs.StyledTextRunItem.createFillPaintsVector | ||
| ) | ||
| : null; | ||
|
|
||
| // Per-run stroke paints | ||
| const runStrokePaintsFiltered = run.stroke_paints?.filter(isPaint); | ||
| const runStrokePaintsOffset = | ||
| runStrokePaintsFiltered && runStrokePaintsFiltered.length > 0 | ||
| ? format.paint.encode.strokePaints( | ||
| builder, | ||
| runStrokePaintsFiltered, | ||
| fbs.StyledTextRunItem.createStrokePaintsVector | ||
| ) | ||
| : null; | ||
|
|
||
| // Per-run stroke geometry | ||
| let runStrokeGeometryOffset: flatbuffers.Offset | null = null; | ||
| if (run.stroke_width !== undefined) { | ||
| runStrokeGeometryOffset = | ||
| format.shape.encode.strokeGeometryTrait(builder, { | ||
| stroke_width: run.stroke_width, | ||
| }); | ||
| } | ||
|
|
||
| fbs.StyledTextRunItem.startStyledTextRunItem(builder); | ||
| fbs.StyledTextRunItem.addStart( | ||
| builder, | ||
| c2b ? c2b[run.start] : run.start | ||
| ); | ||
| fbs.StyledTextRunItem.addEnd( | ||
| builder, | ||
| c2b ? c2b[run.end] : run.end | ||
| ); |
There was a problem hiding this comment.
Clamp and validate styled run boundaries before UTF-8 conversion.
Line 1168 and Line 1172 index c2b directly with run.start/end without bounds checks. Out-of-range or reversed ranges can emit invalid offsets into FlatBuffers.
💡 Suggested fix
- const c2b =
- node.text != null ? charToByteOffsets(node.text) : undefined;
+ const text = node.text ?? "";
+ const c2b = charToByteOffsets(text);
+ const textLen = text.length;
const runOffsets: flatbuffers.Offset[] = [];
for (const run of node.styled_runs) {
+ const start = Math.max(0, Math.min(run.start, textLen));
+ const end = Math.max(start, Math.min(run.end, textLen));
const runStyleOffset = encodeTextStyleRec(builder, run.style);
@@
fbs.StyledTextRunItem.startStyledTextRunItem(builder);
fbs.StyledTextRunItem.addStart(
builder,
- c2b ? c2b[run.start] : run.start
+ c2b[start]!
);
fbs.StyledTextRunItem.addEnd(
builder,
- c2b ? c2b[run.end] : run.end
+ c2b[end]!
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/grida-canvas-io/format.ts` around lines 1127 - 1173, The code
indexes c2b with run.start/run.end without validation, which can produce
out-of-range or reversed offsets; before calling
fbs.StyledTextRunItem.addStart/addEnd, clamp and validate run.start and run.end
against the source text length (use node.text.length or 0 when text is missing),
ensure start = Math.max(0, Math.min(start, textLen)) and end = Math.max(0,
Math.min(end, textLen)), and if start > end swap or set end = start; then use
these sanitized indices when looking up c2b (or fallback to the original indices
when c2b is undefined) so that encodeTextStyleRec, charToByteOffsets, and
fbs.StyledTextRunItem receive safe UTF-8 byte offsets.
| // Decode styled_runs — convert UTF-8 byte offsets → char indices | ||
| const decodedText = props?.text() ?? null; | ||
| const b2c = | ||
| decodedText != null ? byteToCharOffsets(decodedText) : undefined; | ||
| const styledRuns: grida.program.nodes.StyledTextRun[] = []; | ||
| if (props) { | ||
| const runsLength = props.styledRunsLength(); | ||
| for (let i = 0; i < runsLength; i++) { | ||
| const runItem = props.styledRuns(i); | ||
| if (!runItem) continue; | ||
|
|
||
| const runStyle = decodeTextStyleRec(runItem.textStyle()); | ||
|
|
||
| // Per-run fill paints | ||
| const runFillPaints = format.paint.decode.fillPaints(runItem); | ||
| // Per-run stroke paints | ||
| const runStrokePaints = format.paint.decode.strokePaints(runItem); | ||
| // Per-run stroke geometry | ||
| const runStrokeGeometry = runItem.strokeGeometry(); | ||
| const runStrokeWidth = runStrokeGeometry | ||
| ? format.shape.decode.strokeGeometryTrait(runStrokeGeometry) | ||
| .stroke_width | ||
| : undefined; | ||
|
|
||
| const byteStart = runItem.start(); | ||
| const byteEnd = runItem.end(); | ||
| styledRuns.push({ | ||
| start: b2c ? (b2c.get(byteStart) ?? byteStart) : byteStart, | ||
| end: b2c ? (b2c.get(byteEnd) ?? byteEnd) : byteEnd, | ||
| style: runStyle, |
There was a problem hiding this comment.
Avoid raw-byte fallback for unmapped run offsets in decode.
Line 5821 and Line 5822 fall back to byte offsets as char indices when a byte boundary is missing. That can produce invalid UTF-16 indices and non-roundtrippable ranges. Prefer clamped nearest valid character boundary (and enforce start <= end).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/grida-canvas-io/format.ts` around lines 5794 - 5823, The decode
currently falls back to raw byte offsets when b2c mapping is missing (in the
styledRuns push using b2c.get(byteStart) ?? byteStart and b2c.get(byteEnd) ??
byteEnd); change this to map to the nearest valid character boundary and clamp
to valid range: use byteToCharOffsets(decodedText) to locate the closest mapped
char index for byteStart/byteEnd (e.g., nearest lower or nearest higher mapped
entry), then clamp results between 0 and decodedText.length and enforce start =
Math.min(mappedStart, mappedEnd), end = Math.max(mappedStart, mappedEnd) so
start <= end; update the push site that constructs start/end to use this
clamped/mapped logic instead of the raw-byte fallback.
Updated the GitHub Actions workflow to include the typecheck step in CI, addressing previous issues with TypeScript errors. Added typegen tasks to the package.json and turbo.json files for the backgrounds, viewer, and editor applications to ensure proper type generation during builds.
Enhanced the GitHub Actions workflow by introducing a new job to build the @grida/canvas-wasm TypeScript wrapper using tsup. This step ensures that the necessary pre-compiled .wasm binary and Emscripten glue are processed correctly, facilitating type checking and improving the overall build process.
1 participant
Summary
This PR introduces
AttributedTextNode— a new node type supporting rich, per-run styled text — and builds the full stack around it: rendering, layout, cache, serialization, SVG/Figma import, and an interactive in-scene text editor for the native application.New:
AttributedTextNodeAttributedText/StyledTextRuntypes incg::typeswith per-run font weight, size, color, decoration, and stroke paintsbuild_attributed_paragraph, per-run fill/stroke paint, paragraph cache + geometry cache supporttaffy) support for attributed text nodesgolden_type_attributed, fixturel0_type_attributedRefactor: absorb
grida-text-editcrate intocg::text_editgrida-text-edit/src/*→crates/grida-canvas/src/text_edit/attributed_text_conv.rs, replaced with 256-lineconv.rsmapping cg types directly — no lossyRGBA/TextFillintermediariesgrida-text-editcrate (Cargo.toml,README, workspace member)wd_text_editorexample togrida-devRefactor: centralize font fallback through
FontRepositoryuser_fallback_fontsout of per-call context intoFontRepositoryRendererOptions.use_system_fonts(defaultfalse) — enables Skia systemFontMgrfor native dev, disabled on WASM--system-fontsCLI flag throughgrida-devNew: in-scene text editing (native only)
SurfaceEventextended withKeyDown,TextInput,ImevariantsClickTrackerdouble-click detection inSurfaceStatehandle_surface_event()unified dispatcherAttributedTextnode editing viatext_edit_enter_by_id/new_attributedTextEditCommit+apply_text_edit_commit()Cmd+C/X/V) viaarboardinNativeApplicationset_ime_allowedtogglingtext_edit_*APIFix: font change cache invalidation causing stale geometry
FontRepository::set_fallback_fonts()/enable_system_fallback()now bump generation viabump_generation()FONT_LOADEDchange flag now triggers geometry rebuild (was missing)SVG import improvements
<tspan>elements → emitAttributedTextNodefixtures/test-svg/L0/text-cjk.svgFigma import (
io-figma)AttributedTextconversion from Figma REST API character style overridesMap)iofigma.rest-api.attributed-text.test.ts(290 lines)FlatBuffers schema & serialization
format/grida.fbs: newAttributedTextNodeandStyledTextRuntablesio_grida_fbs.rs: encode/decode for attributed text (~600 lines generated, ~440 lines codec)fbs_roundtrip.rsCI / misc
@grida/canvas-wasmTypeScript build step to CIPartialEqon gradient/alignment types for run coalescingserdefeature formath2(AffineTransform,BoxFit)Summary by CodeRabbit
New Features
Chores