perf(canvas): skip ViewportCull bitset build when all leaves visible#628
Merged
softmarshmallow merged 1 commit intocanaryfrom Apr 5, 2026
Merged
perf(canvas): skip ViewportCull bitset build when all leaves visible#628softmarshmallow merged 1 commit intocanaryfrom
softmarshmallow merged 1 commit intocanaryfrom
Conversation
ViewportCull::from_plan allocated and populated a VisibilitySet bitset of every visible leaf id on every full-draw frame. At fit-zoom on large scenes (135K-node fixture, dashboards, pasteboards) every leaf is visible, so the bitset is O(N) work whose answer is always "true". Change the cull to carry an Option<VisibilitySet>: None represents the "all visible" case and is_leaf_visible short-circuits to true. Detect the case by comparing live region count + promoted count against the LayerList length (regions and promoted are disjoint — see the region/promoted partitioning in draw_layers_with_scene_cache_skip). Criterion (CPU raster, bench_viewport_culling): 50k/all_visible 24.678ms -> 24.077ms -2.44% (p=0.00) 5k/all_visible 3.881ms -> 3.818ms -1.76% (p=0.00) partial/corner/zoomed_in/empty: no regression (p > 0.16) The fast path only fires when every leaf is visible; culled frames pay one extra Option match branch per leaf which the benchmarks show is below noise. Savings scale linearly with node count, targeting the view-only fit-zoom path on large scenes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
6 Skipped Deployments
|
WalkthroughThis PR replaces the Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Editor as Editor UI
participant Renderer as Render Pipeline
participant HTMLCSSModule as htmlcss Module
participant SkiaPaint as Skia Picture
rect rgba(0, 200, 100, 0.5)
Note over User,SkiaPaint: New MarkdownEmbed Rendering Flow
User->>Editor: Create MarkdownEmbedNode with markdown text
Editor->>Renderer: render_frame_with_plan_state()
Renderer->>Renderer: Compute FrameRenderStrategy
Renderer->>HTMLCSSModule: markdown_to_styled_html(markdown)
HTMLCSSModule->>HTMLCSSModule: Wrap in <html> with GitHub CSS
HTMLCSSModule-->>Renderer: HTML string
Renderer->>HTMLCSSModule: render(html_string, fonts)
HTMLCSSModule-->>Renderer: SkiaRenderingContext (measured height)
Renderer->>SkiaPaint: Draw rendered content via Picture
SkiaPaint-->>Renderer: Picture with cull rect
Renderer->>Editor: Display rendered markdown node
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9b7feec4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+2149
to
+2151
| 0.0, | ||
| cull.width(), | ||
| cull.height(), |
There was a problem hiding this comment.
The markdown embed paint path now clips to picture.cull_rect() instead of the node’s own size, so when markdown content is taller than the node frame (e.g. newly created/imported embeds with fixed 300px height), the text is drawn outside the node and can overlap neighboring layers. In PainterPictureLayer::MarkdownEmbed, this regresses previous behavior that clipped to md_layer.width/md_layer.height; the clip should be bounded by the node frame (or intersected with it).
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/grida-canvas-schema/grida.ts (2)
2924-2952:⚠️ Potential issue | 🟠 MajorDo not route
markdown_embedthrough the generic fallback.This branch constructs a node without the required
markdownfield, socreateNodeDataFromPrototypeWithoutChildren({ type: "markdown_embed" }, id)returns an invalid object and pushesundefinedcontent handling downstream.🛠️ Suggested fix
+ case "markdown_embed": { + return { + name: "markdown_embed", + type: "markdown_embed", + active: true, + locked: false, + opacity: 1, + z_index: 0, + rotation: 0, + layout_target_width: 100, + layout_target_height: 100, + layout_positioning: "absolute", + markdown: "", + ...prototype, + id: id, + } as MarkdownEmbedNode; + } case "bitmap": case "ellipse": case "iframe": case "image": case "line": - case "markdown_embed": case "richtext": case "tspan": case "vector":🤖 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 2924 - 2952, The branch grouping includes "markdown_embed" but constructs a generic node missing the required markdown field; update the switch in createNodeDataFromPrototypeWithoutChildren so "markdown_embed" is removed from the generic fallback and instead implement a dedicated case for "markdown_embed" that returns a node with the same base properties but includes the required markdown field (e.g., markdown: "" or prototype.markdown) and any other markdown-specific properties from prototype; ensure you reference prototype.type, id and return the result as the correct node type rather than letting it fall through to UnknownNode.
1230-1252:⚠️ Potential issue | 🟠 MajorBump the document schema version with this new node kind.
Adding
markdown_embedchanges the serialized document surface, butgrida.program.document.SCHEMA_VERSIONis still0.91.0-beta+20260311. Older readers will report these files as compatible and then fail on a node type they do not know how to decode.Based on learnings: Bump
CanvasDocument.schema_versionwhen writer behavior changes meaningfully, especially for Breaking changes, and keep it in sync with TSgrida.program.document.SCHEMA_VERSION.Also applies to: 2377-2407
🤖 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 1230 - 1252, The document schema must be bumped to reflect the new markdown_embed node kind: update CanvasDocument.schema_version to a new higher version string (and any other serialized schema constant) and ensure it matches the TypeScript constant grida.program.document.SCHEMA_VERSION; specifically, after adding the new node union entries (ComputedNode and the node kind code paths where MarkdownEmbedNode/ComputedMarkdownEmbedNode were added) increment the schema_version value and update the corresponding TS SCHEMA_VERSION so readers will reject older formats that lack the new node kind.crates/grida-canvas/src/node/factory.rs (1)
383-399:⚠️ Potential issue | 🟠 MajorAdd MarkdownEmbed round-trip test cases to both Rust and TypeScript suites.
The factory method creates
MarkdownEmbedNodeRec, and the schema definesMarkdownEmbedNodein the union (grida.fbs line 1604). However, neithercrates/grida-canvas/tests/fbs_roundtrip.rs(1993 lines) norpackages/grida-canvas-io/__tests__/format-roundtrip.test.ts(3348 lines) contain any MarkdownEmbed test case. Add appended union member test coverage to both suites to prevent format drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/node/factory.rs` around lines 383 - 399, The factory creates MarkdownEmbedNodeRec via create_markdown_embed_node but there are no round-trip tests covering the MarkdownEmbed union member, so add test cases to both Rust and TypeScript round-trip suites: in the Rust tests file (crates/grida-canvas/tests/fbs_roundtrip.rs) append a new test that constructs a MarkdownEmbedNodeRec (use create_markdown_embed_node), serializes and deserializes through the same fbs schema union path used by other node tests and asserts equality; likewise in the TypeScript suite (packages/grida-canvas-io/__tests__/format-roundtrip.test.ts) add a matching test that builds the MarkdownEmbed node object, runs the existing format round-trip helpers, and asserts the round-tripped payload matches the original, ensuring the MarkdownEmbed variant is covered in the union JSON/Flatbuffers round-trip matrix.
🧹 Nitpick comments (3)
docs/wg/feat-2d/optimization.md (1)
879-884: Add blank line before the table (markdownlint MD058).Static analysis flagged a missing blank line before the table at line 880. This can cause rendering issues in some Markdown parsers.
📝 Proposed fix
Measured on 01-135k.perf.grida (135K nodes): + | Scenario | Before p95 | After p95 | Before MAX | After MAX | |---|---|---|---|---|🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/wg/feat-2d/optimization.md` around lines 879 - 884, Add a blank line immediately above the Markdown table that begins with "| Scenario | Before p95 | After p95 | Before MAX | After MAX |" so the table is separated from the preceding paragraph (fixes markdownlint MD058); simply insert one empty line before that table block in optimization.md.crates/grida-canvas/src/node/schema.rs (1)
1256-1277: Keeptype_label()human-readable.
type_label()is documented as a user-facing label, butMarkdownEmbedleaks the internal enum rename. If this string is surfaced anywhere,Markdownreads better.✏️ Minimal tweak
- Node::MarkdownEmbed(_) => "MarkdownEmbed", + Node::MarkdownEmbed(_) => "Markdown",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/node/schema.rs` around lines 1256 - 1277, The type_label() match arm for the Node::MarkdownEmbed variant returns the internal enum name "MarkdownEmbed" which is not user-friendly; update the match arm in the type_label() method so Node::MarkdownEmbed(_) => "Markdown" (keep the return type &'static str), leaving other arms unchanged so the user-facing label reads "Markdown" instead of the internal variant name.editor/grida-canvas/editor.ts (1)
847-875: Centralize themarkdown_embeddefaults.
EditorDocumentStore.createMarkdownEmbedNode()now hard-codes a second default bundle for this node type, and it already drifted fromeditor/grida-canvas/reducers/tools/initial-node.ts(markdownstarts empty here but the tool-path default is prefilled). Different insertion paths will create visibly different nodes unless these defaults live in one shared helper/constants module.🤖 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 847 - 875, The createMarkdownEmbedNode method hard-codes defaults for the "markdown_embed" node that conflict with the defaults in editor/grida-canvas/reducers/tools/initial-node.ts; extract the canonical default object for markdown_embed into a shared constant (e.g., MARKDOWN_EMBED_DEFAULTS) in a common helper/constants module, update EditorDocumentStore.createMarkdownEmbedNode to import and use that constant (instead of inlining the bundle), and update the tools/initial-node.ts to import the same constant so all insertion paths share identical defaults (ensure the id-specific fields like _$id and idgen-generated id are still set at creation time after cloning the shared defaults).
🤖 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/examples/tool_io_grida.rs`:
- Line 469: The inspector uses inconsistent labels: update the
Node::MarkdownEmbed match arm (currently returning "markdown") to return the new
type label "markdown_embed" so FBS/ZIP path matches the JSON path; locate the
string returned for Node::MarkdownEmbed in the match that maps Node variants to
type labels and change it to "markdown_embed".
In `@crates/grida-canvas/src/htmlcss/layout.rs`:
- Around line 329-333: The tag-based fallback apply_faux_table_style_by_tag
currently runs for any non-table computed display and can override an explicit
author display (e.g., display:flex) on table wrappers; change the logic so
apply_faux_table_style_by_tag(&el.tag, &mut style) is only called when the
element's computed display is the default table-wrapper value (i.e., only when
el.display equals the engine's default row-group/table-wrapper display),
otherwise skip the tag fallback so explicit el.display values are respected;
update the conditional around super::faux_table::apply_faux_table_style and the
check that gates calling apply_faux_table_style_by_tag to test el.display
against the default table-wrapper display.
In `@crates/grida-canvas/src/io/io_grida_fbs.rs`:
- Around line 4505-4506: decode_markdown_embed_node is extracting position with
r.transform.x()/y() but decode_shape_layout builds the transform using
from_box_center_raw, so those values are shifted by the rotation center; fix by
applying the inverse transform used in encode_basic_shape_node (call
reverse_from_box_center on the transform returned by decode_shape_layout or
otherwise reverse the from_box_center_raw offset) before reading x/y so you
retrieve the original layout position (mirror what encode_basic_shape_node does
using reverse_from_box_center).
In `@crates/grida-dev/src/editor/document.rs`:
- Around line 194-220: The measured height from measure_content_height in the
affects_w branch can be 0.0 and bypasses the 1px minimum used by the main resize
path; clamp the measured value before applying the resize by enforcing a minimum
(e.g., 1.0) and use that clamped value when constructing the
MutationCommand::Resize for id, so in the block handling Node::MarkdownEmbed /
Node::HTMLEmbed (where measured is set) replace the raw measured h with a
clamped_h = measured.max(1.0) (or equivalent) before calling
self.apply(&MutationCommand::Resize { id: *id, width: None, height:
Some(clamped_h) }).
In `@editor/grida-canvas-hosted/ai/tools/canvas-use.ts`:
- Around line 250-253: The description string in the make_from_markdown tool
currently promises “GitHub markdown styling”; update the description in the
export const make_from_markdown = tool({ ... }) block to avoid overstating
fidelity—change wording to something like “Grida markdown styling” or
“GitHub-like markdown styling” (or similar) so it references the actual
grida-markdown.css renderer instead of claiming exact GitHub styling; keep the
rest of the title/description fields and behavior unchanged.
In `@fixtures/css/grida-markdown.css`:
- Line 71: The font-family declarations using SF Mono need the family name
quoted; locate the declarations like `font-family: SF Mono, Menlo, Consolas,
monospace;` (and the second occurrence) and update both font stacks to quote the
family name as "SF Mono" so Stylelint no longer flags the rule.
In `@format/grida.fbs`:
- Around line 1490-1498: The FlatBuffers table MarkdownEmbedNodeProperties lacks
explicit field IDs; update the table declaration so each field (fill_paints,
markdown, corner_radius) has an explicit (id: N) annotation (e.g., assign
consecutive unique ids) to ensure schema evolvability and stable binary
compatibility; modify the table MarkdownEmbedNodeProperties to include these
(id: N) annotations for fill_paints, markdown, and corner_radius accordingly.
- Around line 280-283: Add round-trip tests that include the new union member
MarkdownEmbed: in the Rust test file fbs_roundtrip.rs add a test that constructs
a node/document containing the MarkdownEmbed variant (matching the struct in
crates/grida-canvas/src/node/schema.rs), serializes it with the existing FBS
encode helper, deserializes it back and asserts equality with the original; in
the TypeScript test file format-roundtrip.test.ts add a corresponding test that
builds the TS document model with MarkdownEmbed (from
packages/grida-canvas-schema/grida.ts), uses the same encode/decode helpers
already used by other cases, and asserts deep equality after round-trip. Use the
existing tests in those files as templates so you reuse the same helper
functions and assertion style.
In `@packages/grida-canvas-io/format.ts`:
- Around line 1918-1971: Add round-trip tests for MarkdownEmbedNode in the
TypeScript and Rust test suites: construct a MarkdownEmbedNode with non-default
fields (markdown string, corner_radius and per-corner
rectangular_corner_radius_*, corner_smoothing, and some fill paints), encode it
using the existing codec path that touches format.paint.encode.fillPaints and
format.shape.encode.rectangularCornerRadiusTrait and the fbs.MarkdownEmbedNode/
fbs.MarkdownEmbedNodeProperties flow, then decode and assert the decoded node
equals the original (deep equality) to validate full encode→decode fidelity;
mirror the same case in the Rust fbs roundtrip test using the crate's existing
roundtrip helper so the test covers Node::MarkdownEmbedNode variant and its
Properties.
In `@packages/grida-canvas-io/index.ts`:
- Around line 295-310: The filetype() helper recognizes markdown but
decodeFromClipboardItems() never preserves or checks for "text/markdown", so
clipboard.read() paths downgrade or lose markdown; update
decodeFromClipboardItems() to detect clipboard items with type "text/markdown"
(and common markdown extensions when converting File items) and return/preserve
the ValidFileType "text/markdown" instead of treating it as plain text or
dropping it; specifically, in decodeFromClipboardItems() add checks for
item.type === "text/markdown" (and the same ext-based logic used in filetype())
and map those branches to return the same result shape and MIME label that
filetype() produces so markdown is passed through end-to-end.
In `@packages/grida-canvas-schema/grida.ts`:
- Around line 2387-2399: The MarkdownEmbedNode interface currently extends
IRectangularShapeTrait which exposes corner_smoothing and
rectangular_stroke_width_* that the runtime ignores; to fix this, stop
over-promising by replacing the IRectangularShapeTrait extension on
MarkdownEmbedNode with a narrow trait that only includes the preserved
rounded-corner fill box properties (create or use an IRectangularFillTrait or
explicitly list allowed shape properties on MarkdownEmbedNode), or alternatively
add and wire the missing rectangular_stroke_width_* and corner_smoothing fields
through the Rust/render pipeline end-to-end; update the declaration of
MarkdownEmbedNode (and any related type aliases) to reference the new/narrowed
trait or explicit properties and remove the undesired fields so the public
schema matches runtime behavior.
---
Outside diff comments:
In `@crates/grida-canvas/src/node/factory.rs`:
- Around line 383-399: The factory creates MarkdownEmbedNodeRec via
create_markdown_embed_node but there are no round-trip tests covering the
MarkdownEmbed union member, so add test cases to both Rust and TypeScript
round-trip suites: in the Rust tests file
(crates/grida-canvas/tests/fbs_roundtrip.rs) append a new test that constructs a
MarkdownEmbedNodeRec (use create_markdown_embed_node), serializes and
deserializes through the same fbs schema union path used by other node tests and
asserts equality; likewise in the TypeScript suite
(packages/grida-canvas-io/__tests__/format-roundtrip.test.ts) add a matching
test that builds the MarkdownEmbed node object, runs the existing format
round-trip helpers, and asserts the round-tripped payload matches the original,
ensuring the MarkdownEmbed variant is covered in the union JSON/Flatbuffers
round-trip matrix.
In `@packages/grida-canvas-schema/grida.ts`:
- Around line 2924-2952: The branch grouping includes "markdown_embed" but
constructs a generic node missing the required markdown field; update the switch
in createNodeDataFromPrototypeWithoutChildren so "markdown_embed" is removed
from the generic fallback and instead implement a dedicated case for
"markdown_embed" that returns a node with the same base properties but includes
the required markdown field (e.g., markdown: "" or prototype.markdown) and any
other markdown-specific properties from prototype; ensure you reference
prototype.type, id and return the result as the correct node type rather than
letting it fall through to UnknownNode.
- Around line 1230-1252: The document schema must be bumped to reflect the new
markdown_embed node kind: update CanvasDocument.schema_version to a new higher
version string (and any other serialized schema constant) and ensure it matches
the TypeScript constant grida.program.document.SCHEMA_VERSION; specifically,
after adding the new node union entries (ComputedNode and the node kind code
paths where MarkdownEmbedNode/ComputedMarkdownEmbedNode were added) increment
the schema_version value and update the corresponding TS SCHEMA_VERSION so
readers will reject older formats that lack the new node kind.
---
Nitpick comments:
In `@crates/grida-canvas/src/node/schema.rs`:
- Around line 1256-1277: The type_label() match arm for the Node::MarkdownEmbed
variant returns the internal enum name "MarkdownEmbed" which is not
user-friendly; update the match arm in the type_label() method so
Node::MarkdownEmbed(_) => "Markdown" (keep the return type &'static str),
leaving other arms unchanged so the user-facing label reads "Markdown" instead
of the internal variant name.
In `@docs/wg/feat-2d/optimization.md`:
- Around line 879-884: Add a blank line immediately above the Markdown table
that begins with "| Scenario | Before p95 | After p95 | Before MAX | After MAX
|" so the table is separated from the preceding paragraph (fixes markdownlint
MD058); simply insert one empty line before that table block in optimization.md.
In `@editor/grida-canvas/editor.ts`:
- Around line 847-875: The createMarkdownEmbedNode method hard-codes defaults
for the "markdown_embed" node that conflict with the defaults in
editor/grida-canvas/reducers/tools/initial-node.ts; extract the canonical
default object for markdown_embed into a shared constant (e.g.,
MARKDOWN_EMBED_DEFAULTS) in a common helper/constants module, update
EditorDocumentStore.createMarkdownEmbedNode to import and use that constant
(instead of inlining the bundle), and update the tools/initial-node.ts to import
the same constant so all insertion paths share identical defaults (ensure the
id-specific fields like _$id and idgen-generated id are still set at creation
time after cloning the shared defaults).
🪄 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: fc9740be-5208-4635-a563-ffb73b0d7914
⛔ Files ignored due to path filters (1)
crates/grida-canvas/src/io/generated/grida.rsis excluded by!**/generated/**
📒 Files selected for processing (47)
crates/grida-canvas/assets/css/grida-markdown.csscrates/grida-canvas/examples/tool_io_grida.rscrates/grida-canvas/examples/tool_io_svg.rscrates/grida-canvas/src/cache/compositor/promotion.rscrates/grida-canvas/src/htmlcss/collect.rscrates/grida-canvas/src/htmlcss/faux_table.rscrates/grida-canvas/src/htmlcss/github_markdown.rscrates/grida-canvas/src/htmlcss/layout.rscrates/grida-canvas/src/htmlcss/mod.rscrates/grida-canvas/src/io/id_converter.rscrates/grida-canvas/src/io/io_grida.rscrates/grida-canvas/src/io/io_grida_fbs.rscrates/grida-canvas/src/layout/engine.rscrates/grida-canvas/src/layout/into_taffy.rscrates/grida-canvas/src/node/factory.rscrates/grida-canvas/src/node/scene_graph.rscrates/grida-canvas/src/node/schema.rscrates/grida-canvas/src/painter/geometry.rscrates/grida-canvas/src/painter/layer.rscrates/grida-canvas/src/painter/markdown.rscrates/grida-canvas/src/painter/mod.rscrates/grida-canvas/src/painter/painter.rscrates/grida-canvas/src/painter/painter_debug_node.rscrates/grida-canvas/src/resources/mod.rscrates/grida-canvas/src/runtime/cost_prediction.rscrates/grida-canvas/src/runtime/frame_strategy.rscrates/grida-canvas/src/runtime/mod.rscrates/grida-canvas/src/runtime/scene.rscrates/grida-dev/src/bench/load_bench.rscrates/grida-dev/src/editor/document.rscrates/grida-dev/src/editor/mutation.rscrates/grida-dev/src/main.rsdocs/wg/feat-2d/optimization.mdeditor/grida-canvas-hosted/ai/tools/canvas-use.tseditor/grida-canvas-react-renderer-dom/nodes/index.tseditor/grida-canvas-react-renderer-dom/nodes/markdown-embed.tsxeditor/grida-canvas-react-renderer-dom/nodes/node.tsxeditor/grida-canvas-react/use-data-transfer.tseditor/grida-canvas/editor.i.tseditor/grida-canvas/editor.tseditor/grida-canvas/reducers/tools/initial-node.tsfixtures/css/README.mdfixtures/css/grida-markdown.cssformat/grida.fbspackages/grida-canvas-io/format.tspackages/grida-canvas-io/index.tspackages/grida-canvas-schema/grida.ts
💤 Files with no reviewable changes (2)
- crates/grida-canvas/src/painter/mod.rs
- crates/grida-canvas/src/painter/markdown.rs
| Node::AttributedText(_) => "attributed_text", | ||
| Node::Error(_) => "error", | ||
| Node::Markdown(_) => "markdown", | ||
| Node::MarkdownEmbed(_) => "markdown", |
There was a problem hiding this comment.
Use the new type label consistently in the inspector.
The JSON path reports markdown_embed, but the FBS/ZIP path still reports markdown, so the same node type shows up under different names depending on the source format.
Suggested fix
- Node::MarkdownEmbed(_) => "markdown",
+ Node::MarkdownEmbed(_) => "markdown_embed",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Node::MarkdownEmbed(_) => "markdown", | |
| Node::MarkdownEmbed(_) => "markdown_embed", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/examples/tool_io_grida.rs` at line 469, The inspector
uses inconsistent labels: update the Node::MarkdownEmbed match arm (currently
returning "markdown") to return the new type label "markdown_embed" so FBS/ZIP
path matches the JSON path; locate the string returned for Node::MarkdownEmbed
in the match that maps Node variants to type labels and change it to
"markdown_embed".
Comment on lines
+329
to
+333
| // Faux-table: override display/flex for CSS table elements. | ||
| // Try display-based override first, then tag-based fallback for | ||
| // row-group wrappers (<thead>, <tbody>, <tfoot>). | ||
| if !super::faux_table::apply_faux_table_style(el.display, &mut style) { | ||
| super::faux_table::apply_faux_table_style_by_tag(&el.tag, &mut style); |
There was a problem hiding this comment.
Don't let the tag fallback override explicit display on table wrappers.
This branch runs for any non-table computed display, so <thead>/<tbody>/<tfoot> with author CSS like display:flex or display:block will still be forced back into faux-table mode here. Gate the tag fallback to the default table-wrapper case only; otherwise explicit display overrides can't opt out of the emulation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/htmlcss/layout.rs` around lines 329 - 333, The
tag-based fallback apply_faux_table_style_by_tag currently runs for any
non-table computed display and can override an explicit author display (e.g.,
display:flex) on table wrappers; change the logic so
apply_faux_table_style_by_tag(&el.tag, &mut style) is only called when the
element's computed display is the default table-wrapper value (i.e., only when
el.display equals the engine's default row-group/table-wrapper display),
otherwise skip the tag fallback so explicit el.display values are respected;
update the conditional around super::faux_table::apply_faux_table_style and the
check that gates calling apply_faux_table_style_by_tag to test el.display
against the default table-wrapper display.
Comment on lines
+4505
to
+4506
| let x = r.transform.x(); | ||
| let y = r.transform.y(); |
There was a problem hiding this comment.
Round-trip data corruption: position extraction doesn't reverse from_box_center transform.
decode_markdown_embed_node uses decode_shape_layout, which creates the transform via from_box_center_raw. This shifts the translation component by the rotation center offset. Using r.transform.x() and r.transform.y() directly returns the shifted values, not the original layout position.
Compare with encode_basic_shape_node at line 3896-3897 which correctly uses reverse_from_box_center.
🐛 Proposed fix
fn encode_markdown_embed_node<'a, A: flatbuffers::Allocator + 'a>(
fbb: &mut flatbuffers::FlatBufferBuilder<'a, A>,
r: &MarkdownEmbedNodeRec,
node_id: &str,
parent_id: &str,
position: &str,
) -> flatbuffers::WIPOffset<fbs::NodeSlot<'a>> {
- let x = r.transform.x();
- let y = r.transform.y();
+ let (x, y) = reverse_from_box_center(&r.transform, r.size.width, r.size.height);
let plt = affine_to_rotation_transform(&r.transform);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let x = r.transform.x(); | |
| let y = r.transform.y(); | |
| fn encode_markdown_embed_node<'a, A: flatbuffers::Allocator + 'a>( | |
| fbb: &mut flatbuffers::FlatBufferBuilder<'a, A>, | |
| r: &MarkdownEmbedNodeRec, | |
| node_id: &str, | |
| parent_id: &str, | |
| position: &str, | |
| ) -> flatbuffers::WIPOffset<fbs::NodeSlot<'a>> { | |
| let (x, y) = reverse_from_box_center(&r.transform, r.size.width, r.size.height); | |
| let plt = affine_to_rotation_transform(&r.transform); |
🤖 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 4505 - 4506,
decode_markdown_embed_node is extracting position with r.transform.x()/y() but
decode_shape_layout builds the transform using from_box_center_raw, so those
values are shifted by the rotation center; fix by applying the inverse transform
used in encode_basic_shape_node (call reverse_from_box_center on the transform
returned by decode_shape_layout or otherwise reverse the from_box_center_raw
offset) before reading x/y so you retrieve the original layout position (mirror
what encode_basic_shape_node does using reverse_from_box_center).
Comment on lines
+194
to
+220
| if affects_w { | ||
| if let Ok(node) = self.scene.graph.get_node(id) { | ||
| let actual_w = new_width.unwrap_or(*w); | ||
| let measured = match node { | ||
| Node::MarkdownEmbed(n) => { | ||
| let html = cg::htmlcss::markdown_to_styled_html(&n.markdown); | ||
| cg::htmlcss::measure_content_height( | ||
| &html, | ||
| actual_w, | ||
| &app.renderer().fonts, | ||
| ) | ||
| .ok() | ||
| } | ||
| Node::HTMLEmbed(n) => cg::htmlcss::measure_content_height( | ||
| &n.html, | ||
| actual_w, | ||
| &app.renderer().fonts, | ||
| ) | ||
| .ok(), | ||
| _ => None, | ||
| }; | ||
| if let Some(h) = measured { | ||
| mutated |= self.apply(&MutationCommand::Resize { | ||
| id: *id, | ||
| width: None, | ||
| height: Some(h), | ||
| }); |
There was a problem hiding this comment.
Clamp the auto-measured height before applying it.
The primary resize path enforces a 1px minimum, but this follow-up bypasses that. Fresh MarkdownEmbed/HTMLEmbed nodes are initialized with empty content, so measure_content_height() can legitimately return 0.0 and collapse the node on the first width resize.
Suggested fix
- if let Some(h) = measured {
+ if let Some(h) = measured {
mutated |= self.apply(&MutationCommand::Resize {
id: *id,
width: None,
- height: Some(h),
+ height: Some(h.max(1.0)),
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if affects_w { | |
| if let Ok(node) = self.scene.graph.get_node(id) { | |
| let actual_w = new_width.unwrap_or(*w); | |
| let measured = match node { | |
| Node::MarkdownEmbed(n) => { | |
| let html = cg::htmlcss::markdown_to_styled_html(&n.markdown); | |
| cg::htmlcss::measure_content_height( | |
| &html, | |
| actual_w, | |
| &app.renderer().fonts, | |
| ) | |
| .ok() | |
| } | |
| Node::HTMLEmbed(n) => cg::htmlcss::measure_content_height( | |
| &n.html, | |
| actual_w, | |
| &app.renderer().fonts, | |
| ) | |
| .ok(), | |
| _ => None, | |
| }; | |
| if let Some(h) = measured { | |
| mutated |= self.apply(&MutationCommand::Resize { | |
| id: *id, | |
| width: None, | |
| height: Some(h), | |
| }); | |
| if affects_w { | |
| if let Ok(node) = self.scene.graph.get_node(id) { | |
| let actual_w = new_width.unwrap_or(*w); | |
| let measured = match node { | |
| Node::MarkdownEmbed(n) => { | |
| let html = cg::htmlcss::markdown_to_styled_html(&n.markdown); | |
| cg::htmlcss::measure_content_height( | |
| &html, | |
| actual_w, | |
| &app.renderer().fonts, | |
| ) | |
| .ok() | |
| } | |
| Node::HTMLEmbed(n) => cg::htmlcss::measure_content_height( | |
| &n.html, | |
| actual_w, | |
| &app.renderer().fonts, | |
| ) | |
| .ok(), | |
| _ => None, | |
| }; | |
| if let Some(h) = measured { | |
| mutated |= self.apply(&MutationCommand::Resize { | |
| id: *id, | |
| width: None, | |
| height: Some(h.max(1.0)), | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-dev/src/editor/document.rs` around lines 194 - 220, The measured
height from measure_content_height in the affects_w branch can be 0.0 and
bypasses the 1px minimum used by the main resize path; clamp the measured value
before applying the resize by enforcing a minimum (e.g., 1.0) and use that
clamped value when constructing the MutationCommand::Resize for id, so in the
block handling Node::MarkdownEmbed / Node::HTMLEmbed (where measured is set)
replace the raw measured h with a clamped_h = measured.max(1.0) (or equivalent)
before calling self.apply(&MutationCommand::Resize { id: *id, width: None,
height: Some(clamped_h) }).
Comment on lines
250
to
+253
| export const make_from_markdown = tool({ | ||
| title: "Create Node from Markdown", | ||
| description: | ||
| "Create a node from a markdown string with new text node. Return the ID of the created node.", | ||
| "Create a styled markdown embed node from a GFM markdown string. Renders headings, lists, code blocks, tables, etc. with GitHub markdown styling. Return the ID of the created node.", |
There was a problem hiding this comment.
Avoid promising exact GitHub styling here.
The renderer/docs in this patch call the stylesheet grida-markdown.css, so saying “GitHub markdown styling” overstates fidelity and can skew the tool planner's expectations. “Grida markdown styling” or “GitHub-like markdown styling” would be safer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@editor/grida-canvas-hosted/ai/tools/canvas-use.ts` around lines 250 - 253,
The description string in the make_from_markdown tool currently promises “GitHub
markdown styling”; update the description in the export const make_from_markdown
= tool({ ... }) block to avoid overstating fidelity—change wording to something
like “Grida markdown styling” or “GitHub-like markdown styling” (or similar) so
it references the actual grida-markdown.css renderer instead of claiming exact
GitHub styling; keep the rest of the title/description fields and behavior
unchanged.
Comment on lines
+280
to
283
|
|
||
| // embeds | ||
| MarkdownEmbed, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that MarkdownEmbed is handled in Rust schema and IO layers
echo "=== Checking Rust schema for MarkdownEmbed ==="
rg -n "MarkdownEmbed" crates/grida-canvas/src/node/schema.rs || echo "Not found in schema.rs"
echo ""
echo "=== Checking Rust IO for MarkdownEmbed ==="
rg -n "MarkdownEmbed" crates/grida-canvas/src/io/ || echo "Not found in io/"
echo ""
echo "=== Checking TS schema for markdown_embed ==="
rg -ni "markdown.?embed" packages/grida-canvas-schema/grida.ts || echo "Not found in grida.ts"
echo ""
echo "=== Checking round-trip tests ==="
rg -l "MarkdownEmbed\|markdown_embed" crates/grida-canvas/tests/ packages/grida-canvas-io/__tests__/ 2>/dev/null || echo "No test files found with MarkdownEmbed"Repository: gridaco/grida
Length of output: 12282
🏁 Script executed:
#!/bin/bash
echo "=== Locating round-trip test files ==="
find . -name "fbs_roundtrip.rs" -o -name "format-roundtrip.test.ts" 2>/dev/null
echo ""
echo "=== Inspecting fbs_roundtrip.rs ==="
if [ -f "crates/grida-canvas/tests/fbs_roundtrip.rs" ]; then
wc -l crates/grida-canvas/tests/fbs_roundtrip.rs
echo ""
head -100 crates/grida-canvas/tests/fbs_roundtrip.rs
fi
echo ""
echo "=== Inspecting format-roundtrip.test.ts ==="
if [ -f "packages/grida-canvas-io/__tests__/format-roundtrip.test.ts" ]; then
wc -l packages/grida-canvas-io/__tests__/format-roundtrip.test.ts
echo ""
head -100 packages/grida-canvas-io/__tests__/format-roundtrip.test.ts
fiRepository: gridaco/grida
Length of output: 6397
🏁 Script executed:
#!/bin/bash
echo "=== Rust test: Looking for gen_* tests and node type patterns ==="
rg "gen_.*\(\)|Node::" crates/grida-canvas/tests/fbs_roundtrip.rs | head -60
echo ""
echo "=== TS test: Looking for test cases and node types ==="
rg 'it\(|type:' packages/grida-canvas-io/__tests__/format-roundtrip.test.ts | head -80
echo ""
echo "=== Checking if MarkdownEmbed/markdown_embed appears anywhere in tests ==="
rg -i "markdown" crates/grida-canvas/tests/fbs_roundtrip.rs packages/grida-canvas-io/__tests__/format-roundtrip.test.tsRepository: gridaco/grida
Length of output: 6846
Round-trip tests missing for MarkdownEmbed.
The Rust runtime model in crates/grida-canvas/src/node/schema.rs and TS document model in packages/grida-canvas-schema/grida.ts have been properly updated with MarkdownEmbed support. However, round-trip tests for this new union member are missing from both crates/grida-canvas/tests/fbs_roundtrip.rs and packages/grida-canvas-io/__tests__/format-roundtrip.test.ts. Add test cases for MarkdownEmbed to ensure encode/decode correctness across both Rust and TypeScript implementations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@format/grida.fbs` around lines 280 - 283, Add round-trip tests that include
the new union member MarkdownEmbed: in the Rust test file fbs_roundtrip.rs add a
test that constructs a node/document containing the MarkdownEmbed variant
(matching the struct in crates/grida-canvas/src/node/schema.rs), serializes it
with the existing FBS encode helper, deserializes it back and asserts equality
with the original; in the TypeScript test file format-roundtrip.test.ts add a
corresponding test that builds the TS document model with MarkdownEmbed (from
packages/grida-canvas-schema/grida.ts), uses the same encode/decode helpers
already used by other cases, and asserts deep equality after round-trip. Use the
existing tests in those files as templates so you reuse the same helper
functions and assertion style.
Comment on lines
+1490
to
+1498
| /// Properties for a Markdown embed node. | ||
| table MarkdownEmbedNodeProperties { | ||
| /// Background fill paints for the markdown container. | ||
| fill_paints:[PaintStackItem]; | ||
| /// The GitHub Flavored Markdown source text. | ||
| markdown:string; | ||
| /// Per-corner rectangular corner radius. | ||
| corner_radius:RectangularCornerRadiusTrait; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit field IDs to MarkdownEmbedNodeProperties for schema evolvability.
Per coding guidelines, FlatBuffers fields should have explicit (id: N) annotations. This ensures stable binary compatibility when fields are added or reordered in the future.
📝 Proposed fix
table MarkdownEmbedNodeProperties {
/// Background fill paints for the markdown container.
- fill_paints:[PaintStackItem];
+ fill_paints:[PaintStackItem] (id: 0);
/// The GitHub Flavored Markdown source text.
- markdown:string;
+ markdown:string (id: 1);
/// Per-corner rectangular corner radius.
- corner_radius:RectangularCornerRadiusTrait;
+ corner_radius:RectangularCornerRadiusTrait (id: 2);
}As per coding guidelines: "Use explicit field IDs (id: N) in FlatBuffers schema definitions and treat them as immutable".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Properties for a Markdown embed node. | |
| table MarkdownEmbedNodeProperties { | |
| /// Background fill paints for the markdown container. | |
| fill_paints:[PaintStackItem]; | |
| /// The GitHub Flavored Markdown source text. | |
| markdown:string; | |
| /// Per-corner rectangular corner radius. | |
| corner_radius:RectangularCornerRadiusTrait; | |
| } | |
| /// Properties for a Markdown embed node. | |
| table MarkdownEmbedNodeProperties { | |
| /// Background fill paints for the markdown container. | |
| fill_paints:[PaintStackItem] (id: 0); | |
| /// The GitHub Flavored Markdown source text. | |
| markdown:string (id: 1); | |
| /// Per-corner rectangular corner radius. | |
| corner_radius:RectangularCornerRadiusTrait (id: 2); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@format/grida.fbs` around lines 1490 - 1498, The FlatBuffers table
MarkdownEmbedNodeProperties lacks explicit field IDs; update the table
declaration so each field (fill_paints, markdown, corner_radius) has an explicit
(id: N) annotation (e.g., assign consecutive unique ids) to ensure schema
evolvability and stable binary compatibility; modify the table
MarkdownEmbedNodeProperties to include these (id: N) annotations for
fill_paints, markdown, and corner_radius accordingly.
Comment on lines
+1918
to
+1971
| case "markdown_embed": { | ||
| const mdNode = node as grida.program.nodes.MarkdownEmbedNode; | ||
|
|
||
| const fillPaintsFiltered = paints(mdNode, "fill"); | ||
| const fillPaintsOffset = format.paint.encode.fillPaints( | ||
| builder, | ||
| fillPaintsFiltered, | ||
| fbs.MarkdownEmbedNodeProperties.createFillPaintsVector | ||
| ); | ||
| const markdownOffset = builder.createString(mdNode.markdown ?? ""); | ||
| const mdCornerUniform = mdNode.corner_radius; | ||
| const cornerRadiusOffset = | ||
| format.shape.encode.rectangularCornerRadiusTrait(builder, { | ||
| rectangular_corner_radius_top_left: | ||
| mdNode.rectangular_corner_radius_top_left ?? mdCornerUniform, | ||
| rectangular_corner_radius_top_right: | ||
| mdNode.rectangular_corner_radius_top_right ?? mdCornerUniform, | ||
| rectangular_corner_radius_bottom_left: | ||
| mdNode.rectangular_corner_radius_bottom_left ?? | ||
| mdCornerUniform, | ||
| rectangular_corner_radius_bottom_right: | ||
| mdNode.rectangular_corner_radius_bottom_right ?? | ||
| mdCornerUniform, | ||
| corner_smoothing: mdNode.corner_smoothing, | ||
| }); | ||
|
|
||
| fbs.MarkdownEmbedNodeProperties.startMarkdownEmbedNodeProperties( | ||
| builder | ||
| ); | ||
| fbs.MarkdownEmbedNodeProperties.addFillPaints( | ||
| builder, | ||
| fillPaintsOffset | ||
| ); | ||
| fbs.MarkdownEmbedNodeProperties.addMarkdown( | ||
| builder, | ||
| markdownOffset | ||
| ); | ||
| fbs.MarkdownEmbedNodeProperties.addCornerRadius( | ||
| builder, | ||
| cornerRadiusOffset | ||
| ); | ||
| const propertiesOffset = | ||
| fbs.MarkdownEmbedNodeProperties.endMarkdownEmbedNodeProperties( | ||
| builder | ||
| ); | ||
|
|
||
| fbs.MarkdownEmbedNode.startMarkdownEmbedNode(builder); | ||
| fbs.MarkdownEmbedNode.addNode(builder, systemNodeTraitOffset); | ||
| fbs.MarkdownEmbedNode.addLayer(builder, layerOffset); | ||
| fbs.MarkdownEmbedNode.addProperties(builder, propertiesOffset); | ||
| nodeOffset = fbs.MarkdownEmbedNode.endMarkdownEmbedNode(builder); | ||
| nodeType = fbs.Node.MarkdownEmbedNode; | ||
| break; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate expected schema/model/test files
fd -i 'grida.fbs$'
fd -i 'grida.ts$'
fd -i 'schema.rs$'
fd -i 'format-roundtrip.test.ts$'
fd -i 'fbs_roundtrip.rs$'
# 2) Verify MarkdownEmbed wiring across schema, TS codec/model, Rust runtime, and round-trip tests
rg -n --hidden -S 'MarkdownEmbedNode|markdown_embed|NodeType\.MarkdownEmbed' \
-g '*grida.fbs' \
-g '*format.ts' \
-g '*grida.ts' \
-g '*schema.rs' \
-g '*format-roundtrip.test.ts' \
-g '*fbs_roundtrip.rs'Repository: gridaco/grida
Length of output: 4200
🏁 Script executed:
# Check if MarkdownEmbedNode appears in the TypeScript round-trip test file
grep -n 'MarkdownEmbedNode\|markdown_embed' packages/grida-canvas-io/__tests__/format-roundtrip.test.ts 2>/dev/null || echo "NOT FOUND in TS test"Repository: gridaco/grida
Length of output: 77
🏁 Script executed:
# Check if MarkdownEmbedNode appears in the Rust round-trip test file
grep -n 'MarkdownEmbedNode\|markdown_embed' crates/grida-canvas/tests/fbs_roundtrip.rs 2>/dev/null || echo "NOT FOUND in Rust test"Repository: gridaco/grida
Length of output: 79
🏁 Script executed:
# Verify grida.fbs compiles with flatc
cd format && flatc --version && flatc -c grida.fbs 2>&1 | head -20 || echo "flatc check failed"Repository: gridaco/grida
Length of output: 136
Add MarkdownEmbedNode round-trip tests in both Rust and TypeScript test files.
The schema, TypeScript model, Rust runtime, and codec are properly wired for MarkdownEmbedNode, but round-trip test coverage is missing. Add test cases in:
packages/grida-canvas-io/__tests__/format-roundtrip.test.tscrates/grida-canvas/tests/fbs_roundtrip.rs
These tests are required to verify encode/decode round-trip fidelity when schema variants are added.
🤖 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 1918 - 1971, Add round-trip
tests for MarkdownEmbedNode in the TypeScript and Rust test suites: construct a
MarkdownEmbedNode with non-default fields (markdown string, corner_radius and
per-corner rectangular_corner_radius_*, corner_smoothing, and some fill paints),
encode it using the existing codec path that touches
format.paint.encode.fillPaints and
format.shape.encode.rectangularCornerRadiusTrait and the fbs.MarkdownEmbedNode/
fbs.MarkdownEmbedNodeProperties flow, then decode and assert the decoded node
equals the original (deep equality) to validate full encode→decode fidelity;
mirror the same case in the Rust fbs roundtrip test using the crate's existing
roundtrip helper so the test covers Node::MarkdownEmbedNode variant and its
Properties.
Comment on lines
295
to
+310
| export function filetype( | ||
| file: File | ||
| ): [true, ValidFileType] | [false, string] { | ||
| const type = file.type || file.name.split(".").pop() || file.name; | ||
| // Always check extension — browsers often report inconsistent MIME | ||
| // types for markdown (text/plain, application/octet-stream, empty). | ||
| const ext = (file.name.split(".").pop() || "").toLowerCase(); | ||
| const mime = (file.type || "").toLowerCase(); | ||
| if ( | ||
| mime === "text/markdown" || | ||
| mime === "text/x-markdown" || | ||
| ext === "md" || | ||
| ext === "markdown" || | ||
| ext === "mdown" || | ||
| ext === "mkd" | ||
| ) { | ||
| return [true, "text/markdown" as const]; |
There was a problem hiding this comment.
Wire markdown through decodeFromClipboardItems() too.
These additions only become reachable from decode()'s DataTransferItem.kind === "file" path. decodeFromClipboardItems() still never checks for text/markdown, so navigator.clipboard.read() pastes will downgrade markdown to plain text or drop it entirely depending on the source.
🔧 Suggested follow-up in decodeFromClipboardItems()
for (const clipboardItem of clipboardItems) {
const types = clipboardItem.types;
// Check for HTML first (Grida/Figma clipboard)
if (types.includes("text/html")) {
try {
const blob = await clipboardItem.getType("text/html");
const html = await blob.text();
@@
} catch {}
}
+
+ const markdownType = (
+ ["text/markdown", "text/x-markdown"] as const
+ ).find((type) => types.includes(type));
+ if (markdownType) {
+ try {
+ const blob = await clipboardItem.getType(markdownType);
+ const file = new File([blob], "clipboard.md", {
+ type: "text/markdown",
+ });
+ decodedItems.push({ type: "text/markdown", file });
+ continue;
+ } catch {}
+ }
// Check for image/svg+xml file
if (types.includes("image/svg+xml")) {Also applies to: 332-348
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/grida-canvas-io/index.ts` around lines 295 - 310, The filetype()
helper recognizes markdown but decodeFromClipboardItems() never preserves or
checks for "text/markdown", so clipboard.read() paths downgrade or lose
markdown; update decodeFromClipboardItems() to detect clipboard items with type
"text/markdown" (and common markdown extensions when converting File items) and
return/preserve the ValidFileType "text/markdown" instead of treating it as
plain text or dropping it; specifically, in decodeFromClipboardItems() add
checks for item.type === "text/markdown" (and the same ext-based logic used in
filetype()) and map those branches to return the same result shape and MIME
label that filetype() produces so markdown is passed through end-to-end.
Comment on lines
+2387
to
+2399
| export interface MarkdownEmbedNode | ||
| extends | ||
| i.IBaseNode, | ||
| i.ISceneNode, | ||
| i.ILayerTrait, | ||
| i.ILayoutChildTrait, | ||
| i.ICornerRadius, | ||
| i.IRectangularShapeTrait, | ||
| i.IFill<cg.Paint> { | ||
| readonly type: "markdown_embed"; | ||
| /** GFM markdown source text. */ | ||
| markdown: string; | ||
| } |
There was a problem hiding this comment.
IRectangularShapeTrait over-promises styling that the runtime does not carry.
This interface now exposes corner_smoothing and rectangular_stroke_width_*, but the visible Rust import/render path for markdown_embed only preserves the rounded-corner fill box. That leaves the public schema accepting fields that are silently ignored or lost on load. Either narrow the trait here or plumb those fields through end-to-end.
🤖 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 2387 - 2399, The
MarkdownEmbedNode interface currently extends IRectangularShapeTrait which
exposes corner_smoothing and rectangular_stroke_width_* that the runtime
ignores; to fix this, stop over-promising by replacing the
IRectangularShapeTrait extension on MarkdownEmbedNode with a narrow trait that
only includes the preserved rounded-corner fill box properties (create or use an
IRectangularFillTrait or explicitly list allowed shape properties on
MarkdownEmbedNode), or alternatively add and wire the missing
rectangular_stroke_width_* and corner_smoothing fields through the Rust/render
pipeline end-to-end; update the declaration of MarkdownEmbedNode (and any
related type aliases) to reference the new/narrowed trait or explicit properties
and remove the undesired fields so the public schema matches runtime behavior.
This was referenced Apr 5, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ViewportCull::from_planallocated an O(N)VisibilitySetbitset every full-draw frame even when every leaf in the scene was visible (the common fit-zoom case on large scenes).None= all visible →is_leaf_visibleshort-circuits totrue. Detect the case by comparing live region count + promoted count against theLayerListlength.Measured impact (Criterion, CPU raster,
bench_viewport_culling)50k/all_visible5k/all_visible50k/zoomed_in_1pct50k/empty_offscreen5k/partial_25pct,corner_5pct,zoomed_in_1pctWhy this helps view-only camera performance
At fit-zoom on large scenes (135K-node fixtures, dashboards, pasteboards), every node is in the visible set. The previous code iterated all ~136K leaf ids, resized a
Vec<u64>, and OR-ed in bits on every full-draw frame. The new path allocates nothing and the per-leaf check is a singlematchonOption. Culled frames pay one extraOptionmatch branch per leaf, which the benchmarks show is below noise.Test plan
cargo test -p cg— 673 unit + 33 doc tests passcargo check -p cg -p grida-canvas-wasm -p grida-dev— all three crates compilecargo clippy -p cg --no-deps— no new warnings in changed filecargo fmt --allbench_viewport_culling(all_visible + culled scenarios)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
.mdand.markdownfiles to create embedded nodes.Chores