feat(grida_editor): introduce the rust-native editor crate#946
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 6 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds the ChangesDocumentation Doctrine Updates
Text-to-Vector Outline Support
grida_editor Reference Editor Crate
Estimated code review effort: 5 (Critical) | ~180 minutes Sequence Diagram(s)sequenceDiagram
participant Hud
participant Interpreter
participant Editor
participant Bridge
participant Renderer
Hud->>Interpreter: apply(Intent)
Interpreter->>Editor: dispatch / commit gesture
Editor-->>Interpreter: ChangeSummary damage
Editor-->>Bridge: take_damage()
Bridge->>Renderer: reflect(damage)
Renderer-->>Bridge: frame updated
sequenceDiagram
participant LocalEditor as Local Editor
participant SyncAuthority as Sync Authority
participant RemoteEditor as Remote Editor
LocalEditor->>SyncAuthority: Submit batch
SyncAuthority->>SyncAuthority: validate and order
SyncAuthority-->>LocalEditor: Commit ack
SyncAuthority-->>RemoteEditor: Commit broadcast
RemoteEditor->>RemoteEditor: rebase and reapply
Possibly related PRs
Estimated code review effort: 5 (Critical) | ~180 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
…P-*) Promote the context-menu module to a general menu surface and add a persistent native application menu bar alongside the pointer's context menu. Both builders (canvas_menu / application_menu) return a Menu value over the command registry with enablement resolved headlessly; the bar is presented natively via muda, the context menu via ui::menu. - Rename the CTX-* contract family to MENU-* and context-menu.md to menu.md; the module now documents two builders sharing one taxonomy. - Add Group / Ungroup / GroupWithContainer commands and their Arrange keybindings (Mod+G / Mod+Shift+G / Mod+Alt+G), dispatching to the grouping module per GRP-1/GRP-4. - Add History::can_undo / can_redo as the non-mutating menu enablement gates (MENU-2), matching mode.rs's can_flatten / can_create_outlines. - snap.md: sibling groups contribute their contents, not their envelope (Groups and derived parents) — the rendered-atoms rule. - muda (Tauri) as a shell-only dep for the native bar.
…ty sections Extend the properties sheet from fills into the remaining paint/style domains, each a headless read accessor + a PropPatch mutation domain with apply/inverse round-trip contracts (DOC-2), bound through bind.rs and mirrored over the wire where the engine types aren't Serialize. - Strokes: whole paint stack plus the six geometry scalars (weight, align, cap, join, miter, dash). - Text typography (Slice A): vertical align, font size / weight / italic, line height, letter spacing — via node_text_style / _mut matchers. - Effects: one section per LayerEffects slot cardinality — layer blur (single) and shadows (multi) — not the web's flat cascade, per the dev-harness plan. - section_header widget: a bold, near-black, 12px heading line so a section reads as distinct from the grey property rows beneath it (visual contrast pass) — one fixed "heading" look, no styling system. - Contracts: doc_contracts (stroke/text/effect round-trips), paint/text/effect_contracts, ui_contracts (section-header render).
Quality-only cleanups from a /simplify review of the two feature commits (no behavior change; 443 tests green, clippy clean bar the pre-existing large_enum_variant): - widgets: extract the shared horizontal-row scaffold into `row::hrow`; Row and SectionHeader now build their (differently-styled) label into one container instead of forking the whole build body. Structure is shared; each look stays hardcoded inline (dumbness doctrine). - properties: a `section_header` helper centralizes the fixed heading height / construction across the six section sites. - menu: lift the selection-scoped enablement both builders duplicated verbatim into `SelectionCaps::resolve` — one owner, no drift (MENU-2). - document: add `WorkingCopy::has_ancestor_in`, the shared ancestor walk behind root-of-selection detection and ungroup's nested-candidate filter (was copy-pasted in app.rs). - shell: gate `menu_dirty` on a command actually being consumed (a ROUTE-2 decline no longer forces a full menubar rebuild); use `sort_by_cached_key` in ungroup so the per-node `children` lookup runs once, not on every comparison.
…m_variant) `Mutation::Patch`'s `set: PropPatch` was 432 bytes — far larger than any other variant, so every `Mutation` paid that footprint. Box it (like `Insert`'s fragment already is) to size-balance the enum and clear the `clippy::large_enum_variant` warning CI flags. Purely mechanical: the field type changes to `Box<PropPatch>`, the wire encode/decode conversions bridge the box, and every construction site wraps its `set` value in `Box::new(...)` across src and tests. Read/match sites are untouched — they ride `Deref` coercion. No behavior change; 443 tests green, clippy clean.
The editor's dev-harness chrome was a bespoke widget framework
(`UiLayer` + `widgets/*`) that painted panels as engine scene subtrees
and re-ran the engine's whole-scene layout reflow on every value change
— the source of the "canvas is slow while a panel is open" regression.
Replace it with egui (immediate-mode), hosted a-la-carte (`egui` +
`egui-winit` + `egui_glow`) on the SAME GL context Skia renders on
(shared `get_proc_address` loader; a `DirectContext::reset` dance after
egui's raw GL each frame keeps Ganesh's cached state valid).
All four chrome surfaces are egui now, read live each frame:
- Properties (`Panel::right`) — layer/transform/text/fills/strokes/
stroke-geometry/effects sections.
- Hierarchy tree (`Panel::left`) — with drag-reorder.
- Toolbar (bottom-center `Area`).
- Context menu (native `Popup` + `SubMenuButton`).
ARCH-3 is preserved verbatim: panels READ via `Editor::node_*` queries
and WRITE only through `bind::apply` — no new editing logic. egui is the
top input tier (`window_event` feeds it first; input it claims never
reaches the HUD/tools/canvas).
Retire the `UiLayer` framework entirely (~13.5k lines): delete the
`ui::{field,focus,menu,popover,properties,scroll,toolbar,widget}`
modules and all of `ui::widgets/*`, plus the five test files that only
exercised them. Survivors: `ui::bind` (the binding vocabulary egui
writes through) and `ui::hierarchy` (document->rows flatten + the
drag->drop geometry, now pure free functions). The HIER-1/2/3
conformance tests are retargeted onto those functions.
Chrome typography uses the embedded Geist family (already in the binary)
rather than egui's default Ubuntu-Light, matching the canvas.
Two pre-existing failures on PR #946, unrelated to the egui migration: - `cargo clippy --workspace -- -D warnings` runs without the `shell` feature, so `Document::has_ancestor_in` — whose only callers live in the shell (`app.rs`: PNG-export root detection + ungroup filter) — tripped `dead_code`. Allow it in the no-shell build rather than gating a general document query on the `shell` feature. - `oxfmt --check` flagged three unformatted markdown files in the crate (`TODO.md`, `docs/menu.md`, `docs/widgets-inventory.md`); format them.
![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: a307f771e6
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (7)
crates/grida/tests/text_outline.rs (1)
1-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests cover the shaping pipeline but not
Renderer::outline_text_nodeitself.Both tests call
ParagraphCache::paragraph+paragraph_to_vector_networkdirectly, which validates the conversion primitive well, but the new public methodRenderer::outline_text_node(scene.rs) has twoNone-returning branches — non-TextSpannode and unloaded scene — that remain untested. Consider adding a test that builds aScene/Renderer, loads aTextSpannode, and callsrenderer.outline_text_node(&id)directly, plus a case assertingNonefor a non-text node id and for an unloaded renderer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/grida/tests/text_outline.rs` around lines 1 - 81, The current tests validate paragraph shaping and vector conversion, but they do not exercise Renderer::outline_text_node directly or its None branches. Add Renderer/Scene-based tests that load a TextSpan node, call renderer.outline_text_node(&id), and assert it returns a nonempty outline; also add coverage for a non-TextSpan node id and for an unloaded renderer to confirm both return None.crates/grida_editor/docs/history.md (1)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a language to the fenced code block.
markdownlint (MD040) flags this fence as missing a language identifier.
📝 Proposed fix
-``` +```text entry { redo: mutation batch // applies the change undo: mutation batch // the inverse (DOC-2) context: authoring context // selection, active scene, edit mode (before, after) origin: local | remote | agent label: step name // e.g. "translate" — display only }</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@crates/grida_editor/docs/history.mdat line 22, The fenced block in the
history docs is missing a language identifier and triggers MD040; update the
markdown fence to specify an appropriate language for the snippet (for example,
the block showing the entry structure) so the lint rule is satisfied. Use the
existing fenced block in the history content as the target and keep the code
sample unchanged apart from the fence label.</details> <!-- cr-comment:v1:54f0179bbe711b2066cfa625 --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>crates/grida_editor/docs/shell.md (1)</summary><blockquote> `20-25`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **Add a language to the fenced diagram block.** markdownlint flags the ASCII-diagram fence for missing a language identifier (MD040). <details> <summary>📝 Proposed fix</summary> ```diff -``` +```text ┌───────────┬──────────────────────────┬─────────────┐🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/grida_editor/docs/shell.md` around lines 20 - 25, Add a language identifier to the fenced diagram block in the shell.md diagram section so markdownlint no longer flags MD040. Update the ASCII-art fence to use a text-style code fence for the diagram, keeping the existing block content unchanged, and make sure the fix is applied in the diagram snippet near the shell layout description.Source: Linters/SAST tools
crates/grida_editor/src/shell/mod.rs (1)
281-306: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftSelf-referential raw pointer into
shell.appis fragile against future refactors.
surface_mut_ptr()captures a pointer intoshell.appand is only correct becauseshellnever moves again — a class of bug that already caused a "launch segfault" per the comment. Any refactor that changes the order (e.g., builder functions returningshell, extra intermediate moves) can silently reintroduce UB, and nothing in the type system enforces the invariant the comment describes.Consider
Box-ing (orPin-ing)ShellAppup front so its address is guaranteed stable independent of local-variable placement, removing the reliance on compiler behavior that isn't a formal Rust guarantee.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/grida_editor/src/shell/mod.rs` around lines 281 - 306, The `surface_mut_ptr()`/`set_renderer_backend(Backend::GL(...))` setup in `Shell::run` relies on `shell.app` never moving again, which is fragile and can reintroduce the self-referential UB described in the comment. Make `ShellApp` address-stable up front by boxing or pinning it before taking the pointer, and update the `shell.app` initialization and all call sites that access the renderer backend so the pointer remains valid regardless of future refactors or temporary moves.crates/grida_editor/src/shell/window.rs (1)
17-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeprecated
HasRawWindowHandletrait usage (already flagged by#[allow(deprecated)]).
raw_window_handle::HasRawWindowHandleis deprecated: Use HasWindowHandle instead. The#[allow(deprecated)]suggests this is already a known, intentional tradeoff mirroring thegrida_devprior art this module is cribbed from, so treat as optional cleanup rather than a blocker — worth confirming whether the pinnedglutin0.32 API surface used here (ContextAttributesBuilder::build,SurfaceAttributesBuilder::build) has aHasWindowHandle-based alternative before migrating.Also applies to: 68-71
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/grida_editor/src/shell/window.rs` around lines 17 - 19, The deprecated raw_window_handle::HasRawWindowHandle import in the window setup should be reviewed and either migrated to HasWindowHandle or kept intentionally if the glutin 0.32 APIs used by the window/context code do not support the newer handle trait yet. Update the relevant window/surface creation path around the existing HasRawWindowHandle usage and the ContextAttributesBuilder/SurfaceAttributesBuilder calls, using the newer trait only if it can be wired through without changing behavior.crates/grida_editor/src/arrange.rs (1)
25-82: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for gapped multi-selections. A case like
[A, C]from[A, B, C, D]will pin down the intended one-stepForward/Backwardbehavior for interleaved selections.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/grida_editor/src/arrange.rs` around lines 25 - 82, Add a regression test around `reorder` for gapped multi-selections like `[A, C]` within `[A, B, C, D]`, since the current `ZOrder::Forward`/`ZOrder::Backward` behavior for interleaved selections is not covered. Extend the existing arrange/z-order test coverage to assert the one-step movement result returned by `reorder` (including moved ids and insertion index) for both directions so the intended behavior stays fixed.crates/grida_editor/src/tool.rs (1)
485-520: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication between click-insert and drag-insert Text handling.
Both
click_insert'sTool::Textarm andbegin_dragindependently mint an id, build the fragment, open the gesture, dispatch the insert, and setPhase::TextSession. Consider factoring the shared "open gesture + insert fragment + enter text session" sequence into one helper to avoid drift between the two paths as the tool grows.Also applies to: 386-433
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/grida_editor/src/tool.rs` around lines 485 - 520, The Tool::Text handling duplicates the same “mint id, build fragment, begin gesture, dispatch insert, and enter Phase::TextSession” flow in both click_insert and begin_drag, so factor that shared sequence into a helper and have both paths call it. Use the existing Tool::Text arm, begin_drag, insert_fragment, dispatch, and Phase::TextSession symbols to consolidate the insert/session setup and keep the two behaviors in sync as the tool evolves.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/grida_editor/docs/menu.md`:
- Around line 95-166: The markdown inventory section uses headings that skip a
level under the `## The application menu` parent, which will trip linting.
Update the subsection headings in the menu inventory block from `####` to `###`
for all related sections such as File, Edit, Object, Arrange, View, Text, and
Settings so the heading hierarchy stays consistent.
In `@crates/grida_editor/src/bin/editor.rs`:
- Line 15: The `--open` argument handling in `editor.rs` currently accepts a
missing path silently and falls back to the demo scene; update the CLI parsing
around the `--open` match arm in `main` to require a following path just like
`--listen` and `--join`. If `args.next()` returns `None`, emit an explicit error
and stop instead of leaving `options.open` unset, so `options.open` only becomes
`Some(path)` when a real path is provided.
In `@crates/grida_editor/src/editor.rs`:
- Around line 254-281: Handle stale gesture inverses in abort_gesture,
gesture_rollback, and rescind_to without panicking. The stored inverse batches
can become invalid after Origin::Remote changes, so replace the current
expect-based apply flow with the same kind of stale-entry handling used by
undo/redo: attempt to apply each inverse, and if it no longer applies, drop or
skip that gesture entry instead of crashing. Keep the fix localized around the
gesture rollback/abort paths in editor.rs and ensure any remaining valid
inverses still update damage, selection, and notifications correctly.
In `@crates/grida_editor/src/interpret.rs`:
- Around line 817-857: Freeze the pixel-grid setting at resize gesture start in
the Active::Resize setup inside interpret.rs, since the current resize path
still reads self.snap.pixel_grid live and can change behavior mid-drag. Capture
the flag alongside the existing ResizeSnapState/session freeze when building the
resize state, and have the moving-corner quantization use that frozen value
instead of re-reading self.snap.pixel_grid during the gesture.
In `@crates/grida_editor/src/menu.rs`:
- Around line 274-275: The menu label for the row using Command::ZoomSelection
is mismatched with the command it triggers. Update the label in the menu
construction around the action(Command::ZoomSelection, ...) entry so it reads
“Zoom to selection” instead of “Zoom to fit”, keeping the command wiring
unchanged.
In `@crates/grida_editor/src/mode.rs`:
- Around line 452-479: The flattened primitive path is dropping the node’s
layout slot, so `flattened_node` should preserve `layout_child` the same way
`text_outline_node` does. Update the `flattened_node` construction to carry over
the source node’s `layout_child` into the new flattened `Node`/schema object,
keeping the existing layout placement intact for flex/auto-layout nodes.
In `@crates/grida_editor/src/shell/egui_panels.rs`:
- Around line 1003-1017: The StrokeDash editor in egui_panels is collapsing
multi-value dash arrays because it reads only the first entry and writes back a
single number through emit_scalar. Update the StrokeDash handling in the panel
code to preserve the full vector from editor.node_stroke_dash(head), either by
editing the existing dash list in place or by passing the complete pattern back
instead of a single scalar, so patterns like [4, 2] remain intact.
In `@crates/grida_editor/src/snap.rs`:
- Around line 661-667: The sort comparator in the points ordering logic can
panic because `partial_cmp(...).unwrap()` is used on `main`/`cnt` values that
may be NaN. Update the comparator in the snapping code path that builds `order`
to handle non-comparable floats safely, falling back to `Ordering::Equal`
instead of unwrapping. Keep the change localized to the sort in the `snap.rs`
point ordering section so the render path remains stable on degenerate geometry.
In `@crates/grida_editor/src/ui/bind.rs`:
- Around line 1022-1053: The convert_paint function drops opacity when
converting to a solid paint: kind 0 uses SolidPaint::new_color(first_color)
without applying the extracted opacity, unlike the gradient branches that
preserve it. Update the Solid case in convert_paint to carry over the source
opacity the same way the other paint variants do, using the existing opacity
value and the SolidPaint-related symbols so a gradient-to-solid conversion keeps
transparency.
---
Nitpick comments:
In `@crates/grida_editor/docs/history.md`:
- Line 22: The fenced block in the history docs is missing a language identifier
and triggers MD040; update the markdown fence to specify an appropriate language
for the snippet (for example, the block showing the entry structure) so the lint
rule is satisfied. Use the existing fenced block in the history content as the
target and keep the code sample unchanged apart from the fence label.
In `@crates/grida_editor/docs/shell.md`:
- Around line 20-25: Add a language identifier to the fenced diagram block in
the shell.md diagram section so markdownlint no longer flags MD040. Update the
ASCII-art fence to use a text-style code fence for the diagram, keeping the
existing block content unchanged, and make sure the fix is applied in the
diagram snippet near the shell layout description.
In `@crates/grida_editor/src/arrange.rs`:
- Around line 25-82: Add a regression test around `reorder` for gapped
multi-selections like `[A, C]` within `[A, B, C, D]`, since the current
`ZOrder::Forward`/`ZOrder::Backward` behavior for interleaved selections is not
covered. Extend the existing arrange/z-order test coverage to assert the
one-step movement result returned by `reorder` (including moved ids and
insertion index) for both directions so the intended behavior stays fixed.
In `@crates/grida_editor/src/shell/mod.rs`:
- Around line 281-306: The
`surface_mut_ptr()`/`set_renderer_backend(Backend::GL(...))` setup in
`Shell::run` relies on `shell.app` never moving again, which is fragile and can
reintroduce the self-referential UB described in the comment. Make `ShellApp`
address-stable up front by boxing or pinning it before taking the pointer, and
update the `shell.app` initialization and all call sites that access the
renderer backend so the pointer remains valid regardless of future refactors or
temporary moves.
In `@crates/grida_editor/src/shell/window.rs`:
- Around line 17-19: The deprecated raw_window_handle::HasRawWindowHandle import
in the window setup should be reviewed and either migrated to HasWindowHandle or
kept intentionally if the glutin 0.32 APIs used by the window/context code do
not support the newer handle trait yet. Update the relevant window/surface
creation path around the existing HasRawWindowHandle usage and the
ContextAttributesBuilder/SurfaceAttributesBuilder calls, using the newer trait
only if it can be wired through without changing behavior.
In `@crates/grida_editor/src/tool.rs`:
- Around line 485-520: The Tool::Text handling duplicates the same “mint id,
build fragment, begin gesture, dispatch insert, and enter Phase::TextSession”
flow in both click_insert and begin_drag, so factor that shared sequence into a
helper and have both paths call it. Use the existing Tool::Text arm, begin_drag,
insert_fragment, dispatch, and Phase::TextSession symbols to consolidate the
insert/session setup and keep the two behaviors in sync as the tool evolves.
In `@crates/grida/tests/text_outline.rs`:
- Around line 1-81: The current tests validate paragraph shaping and vector
conversion, but they do not exercise Renderer::outline_text_node directly or its
None branches. Add Renderer/Scene-based tests that load a TextSpan node, call
renderer.outline_text_node(&id), and assert it returns a nonempty outline; also
add coverage for a non-TextSpan node id and for an unloaded renderer to confirm
both return None.
🪄 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: 43d3b642-3ac3-4e57-a790-ccdb8c45fb5e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (146)
.agents/skills/docs-wg/SKILL.md.agents/skills/docs/SKILL.mdCargo.tomlcrates/grida/src/runtime/scene.rscrates/grida/tests/text_outline.rscrates/grida_editor/Cargo.tomlcrates/grida_editor/README.mdcrates/grida_editor/TODO.mdcrates/grida_editor/docs/README.mdcrates/grida_editor/docs/architecture.mdcrates/grida_editor/docs/context-menu.mdcrates/grida_editor/docs/devtools.mdcrates/grida_editor/docs/document.mdcrates/grida_editor/docs/editor.mdcrates/grida_editor/docs/frame.mdcrates/grida_editor/docs/harness.mdcrates/grida_editor/docs/history.mdcrates/grida_editor/docs/hud.mdcrates/grida_editor/docs/keybindings.mdcrates/grida_editor/docs/menu.mdcrates/grida_editor/docs/properties-sheet.mdcrates/grida_editor/docs/properties.mdcrates/grida_editor/docs/routing.mdcrates/grida_editor/docs/shell.mdcrates/grida_editor/docs/ui.mdcrates/grida_editor/docs/widgets-inventory.mdcrates/grida_editor/docs/widgets.mdcrates/grida_editor/src/align.rscrates/grida_editor/src/arrange.rscrates/grida_editor/src/bin/editor.rscrates/grida_editor/src/bridge.rscrates/grida_editor/src/command.rscrates/grida_editor/src/document.rscrates/grida_editor/src/editor.rscrates/grida_editor/src/grouping.rscrates/grida_editor/src/history.rscrates/grida_editor/src/hud/chrome.rscrates/grida_editor/src/hud/click.rscrates/grida_editor/src/hud/gesture.rscrates/grida_editor/src/hud/hit.rscrates/grida_editor/src/hud/mod.rscrates/grida_editor/src/hud/vocab.rscrates/grida_editor/src/interpret.rscrates/grida_editor/src/io.rscrates/grida_editor/src/keys.rscrates/grida_editor/src/lib.rscrates/grida_editor/src/measurement.rscrates/grida_editor/src/menu.rscrates/grida_editor/src/mode.rscrates/grida_editor/src/pixel_grid.rscrates/grida_editor/src/ruler.rscrates/grida_editor/src/shell/app.rscrates/grida_editor/src/shell/egui_panels.rscrates/grida_editor/src/shell/menubar.rscrates/grida_editor/src/shell/mod.rscrates/grida_editor/src/shell/session.rscrates/grida_editor/src/shell/window.rscrates/grida_editor/src/snap.rscrates/grida_editor/src/sync.rscrates/grida_editor/src/sync_net.rscrates/grida_editor/src/tool.rscrates/grida_editor/src/transparency_grid.rscrates/grida_editor/src/traverse.rscrates/grida_editor/src/ui/README.mdcrates/grida_editor/src/ui/bind.rscrates/grida_editor/src/ui/hierarchy.rscrates/grida_editor/src/ui/mod.rscrates/grida_editor/src/vector/chrome.rscrates/grida_editor/src/vector/hit.rscrates/grida_editor/src/vector/mod.rscrates/grida_editor/src/vector/mode.rscrates/grida_editor/src/vector/ops.rscrates/grida_editor/src/wire.rscrates/grida_editor/tests/align_contracts.rscrates/grida_editor/tests/doc_contracts.rscrates/grida_editor/tests/ed_contracts.rscrates/grida_editor/tests/effect_contracts.rscrates/grida_editor/tests/flatten_contracts.rscrates/grida_editor/tests/frame_contracts.rscrates/grida_editor/tests/grp_contracts.rscrates/grida_editor/tests/hier_contracts.rscrates/grida_editor/tests/hist_contracts.rscrates/grida_editor/tests/hud_contracts.rscrates/grida_editor/tests/interpret_contracts.rscrates/grida_editor/tests/io_contracts.rscrates/grida_editor/tests/key_contracts.rscrates/grida_editor/tests/meas_contracts.rscrates/grida_editor/tests/mode_contracts.rscrates/grida_editor/tests/nudge_contracts.rscrates/grida_editor/tests/outline_contracts.rscrates/grida_editor/tests/paint_contracts.rscrates/grida_editor/tests/pxg_contracts.rscrates/grida_editor/tests/ruler_contracts.rscrates/grida_editor/tests/slice_contracts.rscrates/grida_editor/tests/snap_contracts.rscrates/grida_editor/tests/sync_contracts.rscrates/grida_editor/tests/text_contracts.rscrates/grida_editor/tests/tg_contracts.rscrates/grida_editor/tests/tool_contracts.rscrates/grida_editor/tests/translate_contracts.rscrates/grida_editor/tests/trav_contracts.rscrates/grida_editor/tests/vector_contracts.rscrates/math2/src/bezier.rscrates/math2/src/lib.rscrates/math2/tests/bezier.rsdocs/wg/canvas/_category_.jsondocs/wg/canvas/align.mddocs/wg/canvas/auto-layout.mddocs/wg/canvas/edit-mode.mddocs/wg/canvas/grouping.mddocs/wg/canvas/hierarchy.mddocs/wg/canvas/index.mddocs/wg/canvas/input.mddocs/wg/canvas/io-external.mddocs/wg/canvas/io.mddocs/wg/canvas/measurement.mddocs/wg/canvas/nudge.mddocs/wg/canvas/pixel-grid.mddocs/wg/canvas/ruler.mddocs/wg/canvas/snap.mddocs/wg/canvas/state.mddocs/wg/canvas/surface.mddocs/wg/canvas/tool.mddocs/wg/canvas/translate.mddocs/wg/canvas/transparency-grid.mddocs/wg/canvas/traversal.mddocs/wg/canvas/ux-surface/_category_.jsondocs/wg/canvas/ux-surface/index.mddocs/wg/canvas/ux-surface/selection-intent.mddocs/wg/canvas/ux-surface/selection-partition.mddocs/wg/canvas/ux-surface/selection.mddocs/wg/canvas/ux-surface/targeting.mddocs/wg/feat-crdt/_category_.jsondocs/wg/feat-crdt/index.mddocs/wg/feat-crdt/sync.mddocs/wg/feat-editor/_category_.jsondocs/wg/feat-editor/index.mddocs/wg/feat-history/_category_.jsondocs/wg/feat-history/index.mddocs/wg/feat-svg-editor/vertex-transform-box.mddocs/wg/feat-vector-network/boolean.mddocs/wg/feat-vector-network/create-outlines.mddocs/wg/feat-vector-network/flatten.mddocs/wg/feat-vector-network/index.mddocs/wg/feat-vector-network/snap-vector.mddocs/wg/feat-vector-network/vector-edit.md
Comment on lines
+254
to
+281
| pub fn abort_gesture(&mut self) { | ||
| let Some(mut gesture) = self.gesture.take() else { | ||
| return; | ||
| }; | ||
| if gesture.depth > 1 { | ||
| gesture.depth -= 1; | ||
| self.gesture = Some(gesture); | ||
| self.history.abort(); | ||
| return; | ||
| } | ||
|
|
||
| let mut summary = ChangeSummary::default(); | ||
| for inverse in gesture.inverses.iter().rev() { | ||
| let applied = self | ||
| .doc | ||
| .apply(inverse) | ||
| .expect("invariant: gesture inverses must apply (DOC-2)"); | ||
| self.tap(inverse, &applied.inverse, Origin::Local); | ||
| summary.merge(&applied.summary); | ||
| } | ||
| self.damage.merge(&summary); | ||
| self.selection = gesture.selection_before; | ||
| self.prune_selection(); | ||
| self.history.abort(); | ||
| if !summary.nodes.is_empty() { | ||
| self.notify(&summary); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
# Check whether sync.rs gates remote dispatch while a local gesture is open
rg -n -B3 -A15 'fn (install_tap|dispatch|apply_remote)' crates/grida_editor/src/sync.rs crates/grida_editor/src/sync_net.rsRepository: gridaco/grida
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files and inspect the gesture/rollback paths.
ast-grep outline crates/grida_editor/src/editor.rs --view expanded
echo '---'
ast-grep outline crates/grida_editor/src/sync.rs --view expanded
echo '---'
ast-grep outline crates/grida_editor/src/sync_net.rs --view expanded
echo '---'
# Read the specific editor functions and nearby logic.
sed -n '120,430p' crates/grida_editor/src/editor.rs
echo '---'
# Look for any guard on gesture state around sync/application paths.
rg -n -C 4 'gesture|Origin::Remote|Origin::Agent|dispatch_full|apply_remote|rescind|abort_gesture|gesture_rollback|discard_undone|discard_redone' crates/grida_editor/src/sync.rs crates/grida_editor/src/sync_net.rs crates/grida_editor/src/editor.rsRepository: gridaco/grida
Length of output: 46266
Handle stale gesture inverses without panicking.
abort_gesture, gesture_rollback, and rescind_to all .expect() that their stored inverse batches still apply. src/sync.rs applies Origin::Remote batches unconditionally, so a concurrent remote edit can invalidate an open gesture’s captured inverses and crash these paths. Match undo/redo by dropping stale entries or otherwise handling failed applies here too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/grida_editor/src/editor.rs` around lines 254 - 281, Handle stale
gesture inverses in abort_gesture, gesture_rollback, and rescind_to without
panicking. The stored inverse batches can become invalid after Origin::Remote
changes, so replace the current expect-based apply flow with the same kind of
stale-entry handling used by undo/redo: attempt to apply each inverse, and if it
no longer applies, drop or skip that gesture entry instead of crashing. Keep the
fix localized around the gesture rollback/abort paths in editor.rs and ensure
any remaining valid inverses still update damage, selection, and notifications
correctly.
Member
Author
There was a problem hiding this comment.
Deferring this one (leaving the thread open as a tracked follow-up, not fixing in this PR). The panic path is real, but the trigger requires a concurrent Origin::Remote batch invalidating an open gesture's captured inverses — and remote dispatch isn't wired yet: sync.rs is still a stub in this crate, so nothing currently applies Origin::Remote mid-gesture. A correct fix means designing the gesture ↔ remote-freeze contract (what happens to an open gesture when a remote edit lands), which is a design decision beyond this PR's scope. Guarding the .expect()s in isolation now would just paper over that unresolved semantics. Tracking for the sync milestone.
🤖 Addressed by Claude Code
There was a problem hiding this comment.
@softmarshmallow Understood — that's a reasonable call. Tying the .expect() hardening to the actual gesture↔remote-freeze contract makes more sense than a stopgap fix now, especially since sync.rs isn't wired to apply remote batches mid-gesture yet. Leaving this thread open as a tracked follow-up for the sync milestone.
✏️ Learnings added
Learnt from: softmarshmallow
Repo: gridaco/grida PR: 946
File: crates/grida_editor/src/editor.rs:254-283
Timestamp: 2026-07-06T05:19:23.046Z
Learning: In `crates/grida_editor`, as of this PR, `sync.rs`'s remote/authority dispatch is a stub and does not yet apply `Origin::Remote` batches mid-gesture. The `.expect()` calls in `crates/grida_editor/src/editor.rs` (`abort_gesture`, `gesture_rollback`, `rescind_to`) that assume stored gesture/history inverses always apply are known to be unsafe once remote dispatch is wired up (a concurrent remote edit could invalidate an open gesture's captured inverses). This is intentionally deferred and tracked for the sync milestone rather than fixed in the initial `grida_editor` crate PR; a gesture↔remote-freeze contract needs to be designed first.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+817
to
+857
| if self.active.is_none() { | ||
| editor.begin_gesture(); | ||
| // Refusals: members without a size domain or with a | ||
| // rotation the linear union map cannot express are | ||
| // skipped — the HUD needs no knowledge of this. | ||
| let baselines: Vec<(Id, ResizeBaseline)> = ids | ||
| .iter() | ||
| .filter_map(|id| { | ||
| if editor.node_rotation(id).unwrap_or(0.0) != 0.0 { | ||
| return None; | ||
| } | ||
| let pos = editor.node_position(id)?; | ||
| let size = editor.node_size(id)?; | ||
| let world = *facts.world_bounds.get(id)?; | ||
| Some((id.clone(), ResizeBaseline { pos, size, world })) | ||
| }) | ||
| .collect(); | ||
| let worlds: Vec<Rectangle> = baselines.iter().map(|(_, b)| b.world).collect(); | ||
| let initial_union = if worlds.is_empty() { | ||
| Rectangle::from_xywh(0.0, 0.0, 1.0, 1.0) | ||
| } else { | ||
| math2::rect::union(&worlds) | ||
| }; | ||
| // The session freeze (`SNAP-7`), same as translate's. | ||
| let snap = (!worlds.is_empty()).then(|| { | ||
| Box::new(ResizeSnapState { | ||
| session: snap::Session::freeze( | ||
| &worlds, | ||
| facts.snap_anchors.clone(), | ||
| facts.snap_guides.clone(), | ||
| self.snap.pixel_grid, | ||
| ), | ||
| snapped: None, | ||
| }) | ||
| }); | ||
| self.active = Some(Active::Resize { | ||
| baselines, | ||
| initial_union, | ||
| snap, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== interpret.rs outline ==\n'
ast-grep outline crates/grida_editor/src/interpret.rs --view expanded || true
printf '\n== interpret.rs translate/resize regions ==\n'
sed -n '520,940p' crates/grida_editor/src/interpret.rs | cat -n
printf '\n== snap.rs outline ==\n'
ast-grep outline crates/grida_editor/src/snap.rs --view expanded || true
printf '\n== snap.rs relevant docs ==\n'
sed -n '1,220p' crates/grida_editor/src/snap.rs | cat -nRepository: gridaco/grida
Length of output: 39042
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== resize() in snap.rs ==\n'
sed -n '500,610p' crates/grida_editor/src/snap.rs | cat -n
printf '\n== any quantized/session.quantized uses ==\n'
rg -n "quantized|pixel_grid" crates/grida_editor/src/snap.rs crates/grida_editor/src/interpret.rsRepository: gridaco/grida
Length of output: 6258
Freeze pixel-grid state for resize
The session freeze already captures self.snap.pixel_grid for snap-session setup, but the moving-corner quantization still re-reads self.snap.pixel_grid live. A mid-drag toggle can therefore change resize behavior; capture the flag at gesture start and use that frozen value here too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/grida_editor/src/interpret.rs` around lines 817 - 857, Freeze the
pixel-grid setting at resize gesture start in the Active::Resize setup inside
interpret.rs, since the current resize path still reads self.snap.pixel_grid
live and can change behavior mid-drag. Capture the flag alongside the existing
ResizeSnapState/session freeze when building the resize state, and have the
moving-corner quantization use that frozen value instead of re-reading
self.snap.pixel_grid during the gesture.
Member
Author
There was a problem hiding this comment.
Deferring this one (thread left open as a tracked follow-up). The observation is correct — the moving-corner quantization re-reads self.snap.pixel_grid live while the snap-session setup froze it — but this belongs with the broader gesture-freeze pass rather than a one-off capture here. Toggling the pixel grid mid-resize is not currently reachable from the shipped keybindings, so it's latent, not a live regression. I'd rather fix the freeze semantics uniformly (all gesture-time reads of snap state captured at gesture start) than patch this single site and leave the same shape elsewhere. Tracking alongside the gesture/remote-freeze work.
🤖 Addressed by Claude Code
There was a problem hiding this comment.
@softmarshmallow, that makes sense — thanks for the context. Agreed that patching this one call site would just leave the same live-read pattern elsewhere in the gesture state, and a uniform freeze pass (all snap/gesture-time state captured once at gesture start) is the more durable fix. Since it's not reachable via the shipped keybindings today, deferring alongside the broader gesture/remote-freeze work is reasonable. I'll leave this thread open as tracked follow-up.
✏️ Learnings added
Learnt from: softmarshmallow
Repo: gridaco/grida PR: 946
File: crates/grida_editor/src/interpret.rs:817-857
Timestamp: 2026-07-06T05:19:24.450Z
Learning: In `crates/grida_editor` (Rust), gesture-time state (e.g., `Interpreter.snap.pixel_grid` in `crates/grida_editor/src/interpret.rs`) should ideally be frozen once at gesture start rather than re-read live during the gesture (mirroring the existing `Session::freeze` pattern used for snap sessions). As of PR `#946`, this is not yet uniformly applied — e.g., resize's moving-corner quantization still re-reads `self.snap.pixel_grid` live. softmarshmallow is deferring a uniform fix (freezing all gesture-time reads of snap state at gesture start) to a broader gesture/remote-freeze work item rather than patching individual call sites one-off.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Verified findings from the CodeRabbit/Codex pass; fixed the valid ones: - document.rs: same-parent block-move undo restored siblings in ascending original index, scrambling order (e.g. `[r0,r1,r2,r3]` → `[r1,r0,r2,r3]` on undo). Restore per parent highest-index-first; + regression test. - editor.rs: `abort_gesture` only notified observers on node changes, so guide/background-only rollbacks were invisible — use `!summary.is_empty()` to match `gesture_rollback`. - ui/bind.rs: converting a paint to Solid dropped its opacity; fold it into the solid's alpha. - mode.rs: `flattened_node` dropped `layout_child`, so a flattened primitive lost its flex/auto-layout slot — preserve it like `text_outline_node`. - snap.rs: NaN-safe chrome sort comparator (`unwrap_or(Equal)`). - menu.rs: label the `ZoomSelection` row "Zoom to selection" (was "…fit"). - bin/editor.rs: a bare `--open` now errors instead of silently opening the demo scene. - docs: markdownlint fixes — menu.md heading levels (####→###), fenced-block languages in history.md and shell.md. Deferred (replied on-thread): stale-gesture-inverse panic hardening (needs a dedicated sync-semantics pass), resize pixel-grid mid-drag freeze (minor), multi-segment stroke-dash editing (tracked deferral in TODO.md).
The StrokeDash binding resolver rebuilt the dash array as vec![n], collapsing patterns like [4, 2] to a single length and dropping the gap segment. Edit the first entry in place instead, keeping the tail; 0 still clears to solid. Addresses PR #946 review (egui_panels.rs:1017).
This was referenced Jul 6, 2026
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.
Introduces
crates/grida_editor— the Rust-native Grida editor: document working copy, invertible mutations, history, interaction — all editor plumbing — built from scratch as the reference implementation of the universal canvas spec, on the read-onlygridaengine.Why this crate exists
The legacy model was "replace the DOM with a wasm canvas, keep the JS core."
@grida/canvas-wasmwas a stateless renderer; JS owned all scene state and flushed it across. It worked — until it didn't. It died of split ownership (state in JS, capability in Rust — the text editor), sync-as-architecture (mutate → sync → query → update was the architecture), a write path bolted on late, and death by accretion. The failure was architectural, not incidental.This crate answers that with six laws that are the merge bar (README §2): one owner per concern · data flows one way · the write path is first-class · one module/one concept/one golden contract · "it works" is not an argument · prototypes retire, specs carry.
What's in the box
An island: depends on
grida+math2only, nothing depends on it; the core is feature-free and rendererless (ARCH-1), the windowed shell sits behind theshellfeature. Each shipped concept is specced (RFC id) → implemented → conformance-tested.Suite: 369 tests green — the same under core (headless, no renderer) and
--features shell(incl. the raster probe binary), 0 failures — across 29 contract suites plus unit tests. Every contract test cites its RFC id (harness.md); the per-concept status ledger and remaining gaps live incrates/grida_editor/TODO.md.Status
Spec container and pioneer (README §5). The RFC cluster under
crates/grida_editor/docsis normative; this crate is its executable form. It is not a product yet — the UI is deliberately crude, the engine is read-only from here — and wasm shipping stays frozen until this rebuild can swap in as v2. Milestones M1–M4 (core → canvas → panels → the two-instance concurrent-authoring deliverable) are tracked in the TODO.Recent work included here
This branch layers the properties inspector + editing surface onto the core. Each section landed as a reusable domain following the same spine — document patch (invertible/serializable/validated) →
Binding::entryaddressing + bind-owned list resolvers (ARCH-3) → rows assembled from atoms → a contract suite:fillspaint-list domain (multiple fills; solid / gradient / image), replacing the old single-fill_solidspecial case.LayerEffectsslot cardinality (layer blur single + shadows multi), mirroring the engine'sfetypes onto the wire.MENU-*) + group / ungroup commands (GRP-*).UiLayerproperties / toolbar / context-menu chrome was replaced with egui (egui_glowshares Skia's single GL context); theARCH-3binding layer was reused verbatim.Known issues (surfaced in review)
A max-effort review of this branch surfaced the correctness bugs below (README §3 — the crate treats its own code as the first suspect). None block the draft; they are tracked for the pre-v2 hardening pass. Grouped by area, most severe first:
Sync / wire — the M4 concurrent-authoring path
wire.rs—Envelope::to_json.expect()s infallible serialization, butserde_jsonerrors on non-finitef32; a singleNaN/Infcoordinate reaching encode panics the process on copy/broadcast.sync.rs— the authority advances per-senderlast_seqfor a rejected submit but emits no commit, so the next accepted echo mismatches the still-present speculative front and hard-errors asDesync(ends the session though both sides agree on canonical).sync.rs—on_commitbuffers out-of-order commits unbounded with no gap-recovery; one permanently-droppedglobalstalls the client forever with unbounded memory growth.wire.rs— vector-region decode drops a region only when it loses all loops; a partial-loop drop emits a geometrically different fill area (non-round-trip on adversarial/older input).wire.rs—usizeon the wire (counts / indices) is platform-width, breaking the cross-arch fidelity the codec claims.Document / editor
document.rs—from_scenemints stable ids checking only already-taken ids, so a minted id that later collides with an explicitid_mapid is overwritten, orphaning the first node on import.document.rs—merge_patch_overfoldsfill_solidandfillswithout clearing the other (unlike the vector fields), so a gesture touching both coalesces into a patch thatapplyrejects as mutually-exclusive → the undo/redo entry is silently un-re-appliable.document.rs— size getter/setter domains disagree: a per-axis size patch on a half-auto container is rejected thoughset_sizesupports it (already a known-gap comment).Panels / interaction
egui_panels.rs— paint-row controls read head's paint but broadcast the write to every selected node's indexi, silently corrupting structurally-different paints on the other nodes.ui/bind.rs— editing a layer-blur radius on a Progressive blur clobbers the whole variant to Gaussian, discarding the progressive params (read-A / write-B data loss).egui_panels.rs— a hierarchy drag whose grabbed row leaves the flattened tree mid-drag loses the drop and leaveshier_draglatched (wedged tree).egui_panels.rs— the blend-mode combo maps any mode outside its 8-entry list to index 0 ("Pass through"), mis-displaying and clobbering off-list modes.hud/mod.rs— tapping a modifier after pressing a resize/rotate handle (no drag) flipsdragged = true, emitting a spurious zero-delta commit.align.rs—distributederives the span from the largest leading edge (not trailing) and lacks a non-negative-gap guard → wrong / overlapping distribution.Plus lower-severity / plausible items (chrome
partial_cmp().unwrap()NaN panic, zero-extent resize no-op,historymax_depth == 0silently dropping every entry, repeat-paste offset skipped for position-less kinds). Full write-up in the review thread.Running & testing
Draft: large introductory diff (~48k lines, the crate authored fast and not yet fully reviewed — README §3 treats its own code as the first suspect). Opened to continue across machines; see the Known-issues list above for the review pass.
Summary by CodeRabbit