feat: Framework Extraction Phase 1 — generic ProjNode[T] + SyncEditor[T]#57
Merged
feat: Framework Extraction Phase 1 — generic ProjNode[T] + SyncEditor[T]#57
Conversation
Add two capability traits that will parameterize ProjNode[T]: - TreeNode: structural access (children, same_kind) for reconciliation - Renderable: display/serialization (kind_tag, label, placeholder, unparse) Both traits are implemented for @ast.Term. Pure additions — no existing code changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… traits Tasks 2-5 of framework extraction: parameterize the entire projection/ package over a generic AST type T constrained by TreeNode + Renderable. - ProjNode[T]: generic struct replacing hardcoded @ast.Term - reconcile[T]: generic LCS-based reconciliation (reconcile_ast kept as alias) - InteractiveTreeNode[T], TreeEditorState[T]: generic editor model - InteractiveNodeStamp stores kind: T directly (replaces InteractiveNodeShape) - EditContext[T]: generic edit context (handlers stay Term-specific) - SourceMap methods: generic over ProjNode[T] (struct stays non-generic) - same_kind_tag removed, replaced by TreeNode::same_kind trait method Behavioral changes: - Stamp comparison uses T : Eq instead of shape enum (detects leaf value changes) - Bop operator changes now get fresh node IDs (more correct) All 186 projection tests pass. Editor package has expected compilation errors (19x @proj.ProjNode expects 1 argument) to be resolved in Task 6. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
SyncEditor is now generic over the AST type T. The constructor accepts a parser factory and memo builder, decoupling it from the lambda calculus grammar. Lambda-specific methods (get_ast, get_resolution, apply_tree_edit) stay as SyncEditor[@ast.Term] specializations. - SyncEditor::new[T] takes make_parser + build_memos factories - SyncEditor::new_lambda convenience constructor for @ast.Term - All generic methods use fn[T] syntax (66 method signatures updated) - get_proj_node and get_node require T : Eq for Memo access - projection_memo pipeline stays lambda-specific internally - All 507 tests pass, no new warnings Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
lambda-editor | 71d3357 | Commit Preview URL Branch Preview URL |
Mar 24 2026, 03:05 PM |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe PR generically parameterizes the editor and projection stack around a type parameter T (specializing many APIs to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Benchmark Comparison ReportComparing PR branch against Main Module BenchmarksBase branch: PR branch: event-graph-walker BenchmarksBase branch: PR branch: Benchmarks run with |
- reconcile[T]: remove unnecessary Eq bound (only uses TreeNode::same_kind) - reconcile_children[T]: same - from_term_node_impl[T]: TreeNode+Renderable → Renderable (only uses label) - can_skip_subtree[T]: remove unnecessary TreeNode bound - interactive_node_stamp[T]: already fixed in prior commit 2 remaining MoonBit warnings are false positives on transitively-used bounds. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
projection/tree_editor_model.mbt (1)
194-203:is_valid_dropstill uses concrete@ast.Termwhile surrounding model is generic.This function takes
@ast.Termparameters directly, which is inconsistent with the otherwise fully parameterizedTreeEditorState[T]model. This may be intentional for Phase 1 (keeping lambda-specific behavior), but consider noting this as a Phase 2 candidate for generalization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projection/tree_editor_model.mbt` around lines 194 - 203, is_valid_drop currently takes concrete `@ast.Term` parameters which conflicts with the generic TreeEditorState[T] model; change the function signature to accept the generic element type (e.g. T or TreeEditorState[T]’s element type) instead of `@ast.Term`, update any internal references and callers to use that generic type, and if you want to keep lambda-specific behavior for Phase 1 add a TODO note to generalize later; refer to the is_valid_drop function, the `@ast.Term` type, DropPosition, and the TreeEditorState[T] model when making these edits.projection/proj_node.mbt (1)
7-15: Update theProjNodedocs to match the new generic type.The surrounding header still describes this as a Term-only node, but the struct now exposes
kind : T. That mismatch will show up in generated docs and makes the new framework API look narrower than it is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projection/proj_node.mbt` around lines 7 - 15, The doc comment above the ProjNode struct still says it's a Term-only node; update that header to reflect the new generic type parameter T and that ProjNode now holds kind : T (a generic kind) rather than a Term, e.g. describe it as a generic projection node parameterized by T and mention the kind and children fields (ProjNode[T], kind : T, children : Array[ProjNode[T]]) so generated docs correctly represent the API.projection/reconcile_ast.mbt (1)
5-9: Drop the unusedEqbound from the generic reconciliation API.Line 5 and Line 40 only rely on
T::same_kind(...); nothing in this file comparesTvalues. Keeping+ Eqon the new API makes custom AST adapters implement equality for no functional reason, which unnecessarily narrows the framework surface.Also applies to: 40-44
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projection/reconcile_ast.mbt` around lines 5 - 9, The generic bound on reconcile is overly restrictive: remove the unused Eq constraint from the signature pub fn[T : TreeNode + Eq] reconcile(...) -> ProjNode[T] (and the analogous occurrence around lines 40–44) so it becomes pub fn[T : TreeNode] reconcile(...); ensure any other declarations or helper functions in this file that mirror the same generic signature are updated similarly and run tests to confirm nothing else relied on Eq.projection/text_edit_wrap.mbt (1)
128-165: MakeInsertChild's upper-bound behavior explicit.Line 134 rejects negative indices, but the
Modulepath later treats any oversized index as “insert before body.” That silent clamping is easy for callers to miss. Either validate the upper bound too, or document that large indices intentionally append at the module tail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projection/text_edit_wrap.mbt` around lines 128 - 165, The function compute_insert_child currently only rejects negative indices but silently treats index >= parent_node.children.length() in the `@ast.Term`::Module branch as "insert before body", which is ambiguous; update compute_insert_child to make upper-bound behavior explicit by adding an index-range check after obtaining parent_node (e.g., compare index against parent_node.children.length()) and either (a) return an Err("Index out of bounds") for invalid large indices, or (b) explicitly handle append-by-index (documented) by using a clear branch for index == parent_node.children.length() and only error when index > parent_node.children.length(); adjust the Module branch (and any other kind-specific branches) accordingly so parent_node.children and source_map.get_range usage is guarded by the validated index.editor/sync_editor.mbt (1)
26-41:SyncEditor::newis now coupled to the memo graph internals.This constructor now bakes
Runtime,Signal,Memo, and the exact 4-value memo tuple into the public API. That makes every non-lambda backend depend on the current projection wiring, so any future reshaping of the memo pipeline will force another external API change. Consider taking a single backend/factory record instead, and keep the low-level memo graph behind that boundary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/sync_editor.mbt` around lines 26 - 41, SyncEditor::new currently exposes memo-graph types (Runtime, Signal, Memo) and a fixed 4-tuple return shape via the build_memos parameter, coupling callers to projection internals; instead create and accept a single backend/factory record (e.g., a BuildBackend or SyncBackend factory type) with a single method (e.g., build_memos or create_backend) that encapsulates the Runtime/Signal/Memo usage and returns a backend object or record that exposes only the needed high-level operations (like get_proj_node, get_projection_map, source_map) so callers of SyncEditor::new only depend on make_parser, agent_id, and the new backend factory type (leave capture_timeout_ms unchanged). Update the SyncEditor::new signature to replace the multi-typed build_memos parameter with this single factory/record type and change internal usage to call the factory method to obtain the opaque backend; keep existing internal memo wiring private to the implementation (refer to SyncEditor::new, make_parser, build_memos, capture_timeout_ms, and any helpers that currently destructure the 4-tuple).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@editor/projection_memo.mbt`:
- Around line 18-20: The empty-buffer reset path currently clears
prev_flat_proj_ref and prev_syntax_root_ref but leaves prev_module_id_ref
intact, causing stale NodeId to be reused by to_proj_node_with_prev_module_id;
update the same reset/clear logic (the block that resets prev_flat_proj_ref,
prev_syntax_root_ref and next_id_ref) to also clear prev_module_id_ref (set it
to None/empty) so the previous module id cannot be attached to newly typed
content; ensure you apply the same change in both places mentioned (the initial
block around prev_flat_proj_ref/prev_syntax_root_ref/next_id_ref and the
analogous block referenced at lines 114-137).
In `@pkg.generated.mbti`:
- Line 10: The exported function apply_sync_json now returns String (not void)
which is intentional but undocumented; update documentation and release notes to
call out this API change and add a migration note showing callers (including
examples/ideal/main re-export wrappers) how to handle the returned String, and
update JS_INTEGRATION.md to reflect the new return type and any necessary
TypeScript/consumer-side changes.
In `@projection/tree_editor_refresh.mbt`:
- Around line 101-105: The current fast-path compares proj_node.kind and
prev_node.kind with full T equality which can trigger deep subtree comparisons;
change the checks in refresh_node_minimal (where proj_node.kind !=
prev_node.kind and where you compare labels via Renderable::label) to use a
local shape/render key instead: call TreeNode::same_kind (or compare
Renderable::kind_tag plus the local label via Renderable::label) to decide
kind-equality, and leave descendant equality to the recursive child walk; apply
the same replacement for the other occurrence that mirrors this logic.
---
Nitpick comments:
In `@editor/sync_editor.mbt`:
- Around line 26-41: SyncEditor::new currently exposes memo-graph types
(Runtime, Signal, Memo) and a fixed 4-tuple return shape via the build_memos
parameter, coupling callers to projection internals; instead create and accept a
single backend/factory record (e.g., a BuildBackend or SyncBackend factory type)
with a single method (e.g., build_memos or create_backend) that encapsulates the
Runtime/Signal/Memo usage and returns a backend object or record that exposes
only the needed high-level operations (like get_proj_node, get_projection_map,
source_map) so callers of SyncEditor::new only depend on make_parser, agent_id,
and the new backend factory type (leave capture_timeout_ms unchanged). Update
the SyncEditor::new signature to replace the multi-typed build_memos parameter
with this single factory/record type and change internal usage to call the
factory method to obtain the opaque backend; keep existing internal memo wiring
private to the implementation (refer to SyncEditor::new, make_parser,
build_memos, capture_timeout_ms, and any helpers that currently destructure the
4-tuple).
In `@projection/proj_node.mbt`:
- Around line 7-15: The doc comment above the ProjNode struct still says it's a
Term-only node; update that header to reflect the new generic type parameter T
and that ProjNode now holds kind : T (a generic kind) rather than a Term, e.g.
describe it as a generic projection node parameterized by T and mention the kind
and children fields (ProjNode[T], kind : T, children : Array[ProjNode[T]]) so
generated docs correctly represent the API.
In `@projection/reconcile_ast.mbt`:
- Around line 5-9: The generic bound on reconcile is overly restrictive: remove
the unused Eq constraint from the signature pub fn[T : TreeNode + Eq]
reconcile(...) -> ProjNode[T] (and the analogous occurrence around lines 40–44)
so it becomes pub fn[T : TreeNode] reconcile(...); ensure any other declarations
or helper functions in this file that mirror the same generic signature are
updated similarly and run tests to confirm nothing else relied on Eq.
In `@projection/text_edit_wrap.mbt`:
- Around line 128-165: The function compute_insert_child currently only rejects
negative indices but silently treats index >= parent_node.children.length() in
the `@ast.Term`::Module branch as "insert before body", which is ambiguous; update
compute_insert_child to make upper-bound behavior explicit by adding an
index-range check after obtaining parent_node (e.g., compare index against
parent_node.children.length()) and either (a) return an Err("Index out of
bounds") for invalid large indices, or (b) explicitly handle append-by-index
(documented) by using a clear branch for index == parent_node.children.length()
and only error when index > parent_node.children.length(); adjust the Module
branch (and any other kind-specific branches) accordingly so
parent_node.children and source_map.get_range usage is guarded by the validated
index.
In `@projection/tree_editor_model.mbt`:
- Around line 194-203: is_valid_drop currently takes concrete `@ast.Term`
parameters which conflicts with the generic TreeEditorState[T] model; change the
function signature to accept the generic element type (e.g. T or
TreeEditorState[T]’s element type) instead of `@ast.Term`, update any internal
references and callers to use that generic type, and if you want to keep
lambda-specific behavior for Phase 1 add a TODO note to generalize later; refer
to the is_valid_drop function, the `@ast.Term` type, DropPosition, and the
TreeEditorState[T] model when making these edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 459a739c-95aa-469f-8417-5221f08d22d9
📒 Files selected for processing (56)
crdt.mbtcrdt_test.mbtcrdt_undo.mbteditor/error_path_wbtest.mbteditor/performance_benchmark.mbteditor/pkg.generated.mbtieditor/projection_memo.mbteditor/sync_editor.mbteditor/sync_editor_parser.mbteditor/sync_editor_parser_test.mbteditor/sync_editor_sync.mbteditor/sync_editor_test.mbteditor/sync_editor_text.mbteditor/sync_editor_text_wbtest.mbteditor/sync_editor_undo.mbteditor/sync_editor_undo_test.mbteditor/sync_editor_ws.mbteditor/sync_editor_ws_wbtest.mbteditor/test_error_recovery.mbteditor/tree_edit_bridge.mbteditor/tree_edit_bridge_test.mbtexamples/ideal/main/action_model.mbtexamples/ideal/main/model.mbtexamples/rabbita/main/benchmark_support.mbtexamples/rabbita/main/main.mbtintegration_ws_test.mbtmoon.pkgpkg.generated.mbtiprojection/flat_proj.mbtprojection/pkg.generated.mbtiprojection/proj_node.mbtprojection/proj_node_json.mbtprojection/reconcile_ast.mbtprojection/scope.mbtprojection/scope_wbtest.mbtprojection/source_map.mbtprojection/text_edit.mbtprojection/text_edit_binding.mbtprojection/text_edit_commit.mbtprojection/text_edit_delete.mbtprojection/text_edit_drop.mbtprojection/text_edit_middleware.mbtprojection/text_edit_refactor.mbtprojection/text_edit_rename.mbtprojection/text_edit_structural.mbtprojection/text_edit_utils.mbtprojection/text_edit_wbtest.mbtprojection/text_edit_wrap.mbtprojection/traits.mbtprojection/traits_term.mbtprojection/tree_editor.mbtprojection/tree_editor_model.mbtprojection/tree_editor_refresh.mbtprojection/tree_editor_wbtest.mbtprojection/tree_lens.mbtprojection/tree_refresh_benchmark.mbt
Comment on lines
+18
to
+20
| let prev_flat_proj_ref : Ref[@proj.FlatProj?] = Ref::new(None) | ||
| let prev_syntax_root_ref : Ref[@seam.SyntaxNode?] = Ref::new(None) | ||
| let next_id_ref : Ref[Int] = Ref::new(0) |
There was a problem hiding this comment.
Clear prev_module_id_ref when the document is explicitly emptied.
The empty-buffer reset path drops the previous projection state but leaves prev_module_id_ref alive. After a full clear/retype cycle, to_proj_node_with_prev_module_id(...) will attach the old root NodeId to unrelated content, so NodeId-keyed UI state can bleed into the new tree.
🔧 Suggested fix
+ let prev_module_id_ref : Ref[Int?] = Ref::new(None)
let prev_flat_proj_ref : Ref[`@proj.FlatProj`?] = Ref::new(None)
let prev_syntax_root_ref : Ref[`@seam.SyntaxNode`?] = Ref::new(None)
let next_id_ref : Ref[Int] = Ref::new(0)
...
if source_text.get() == "" {
prev_flat_proj_ref.val = None
prev_syntax_root_ref.val = None
+ prev_module_id_ref.val = None
changed_def_indices_ref.val = None
return None
}
...
- let prev_module_id_ref : Ref[Int?] = Ref::new(None)Also applies to: 114-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@editor/projection_memo.mbt` around lines 18 - 20, The empty-buffer reset path
currently clears prev_flat_proj_ref and prev_syntax_root_ref but leaves
prev_module_id_ref intact, causing stale NodeId to be reused by
to_proj_node_with_prev_module_id; update the same reset/clear logic (the block
that resets prev_flat_proj_ref, prev_syntax_root_ref and next_id_ref) to also
clear prev_module_id_ref (set it to None/empty) so the previous module id cannot
be attached to newly typed content; ensure you apply the same change in both
places mentioned (the initial block around
prev_flat_proj_ref/prev_syntax_root_ref/next_id_ref and the analogous block
referenced at lines 114-137).
|
|
||
| // Values | ||
| pub fn apply_sync_json(Int, String) -> Unit | ||
| pub fn apply_sync_json(Int, String) -> String |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the pkg.generated.mbti file
find . -name "pkg.generated.mbti" -type fRepository: dowdiness/canopy
Length of output: 368
🏁 Script executed:
# Check the current state of the file
if [ -f "pkg.generated.mbti" ]; then
cat -n pkg.generated.mbti
fiRepository: dowdiness/canopy
Length of output: 3108
🏁 Script executed:
# Look for git history of changes to this file
git log --oneline -n 20 -- "pkg.generated.mbti" 2>/dev/null || echo "No git history available"Repository: dowdiness/canopy
Length of output: 131
🏁 Script executed:
# Check for any recent diffs or changes related to apply_sync_json
git diff HEAD~10..HEAD -- "pkg.generated.mbti" 2>/dev/null || echo "Unable to retrieve git diff"Repository: dowdiness/canopy
Length of output: 87
🏁 Script executed:
# Search for apply_sync_json in the codebase to understand its usage
rg "apply_sync_json" -B 2 -A 2Repository: dowdiness/canopy
Length of output: 16518
🏁 Script executed:
# Search for any compatibility wrappers or version documentation
rg "apply_sync_json|version|compat" --type mbti -B 1 -A 1Repository: dowdiness/canopy
Length of output: 88
API return-type change is intentional but lacks explicit documentation.
apply_sync_json returning String is intentional and aligns with the actual implementation and TypeScript usage expectations. However, consumers in dependent packages (e.g., examples/ideal/main) use re-export wrappers to manage this change. Ensure this change is explicitly documented in release notes or a migration guide, especially since older documentation (JS_INTEGRATION.md) still references void.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg.generated.mbti` at line 10, The exported function apply_sync_json now
returns String (not void) which is intentional but undocumented; update
documentation and release notes to call out this API change and add a migration
note showing callers (including examples/ideal/main re-export wrappers) how to
handle the returned String, and update JS_INTEGRATION.md to reflect the new
return type and any necessary TypeScript/consumer-side changes.
Allows external packages to implement these traits for their own AST types. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- proj_node.mbt: update doc comment to reflect generic ProjNode[T] - tree_editor_refresh.mbt: replace deep T equality in can_skip_subtree with TreeNode::same_kind + Renderable::label (avoids O(subtree) comparison on complex AST types) Verified not a bug: prev_module_id_ref scoping is intentional (kept across transient projection misses for stable Module IDs). reconcile Eq bound already fixed in prior commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove unused TreeNode bounds from from_term_node, from_term_node_with_state (they only call Renderable::label via from_term_node_impl) - Add explicit type annotation TreeEditorState[@ast.Term] in test to resolve unresolved type variable warning - CI uses -w @A (warnings as errors), so these were blocking Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add [@ast.Term] type parameter to TreeEditorState, InteractiveTreeNode in examples/rabbita/ and examples/ideal/ - Add dowdiness/lambda/ast import to Rabbita's moon.pkg Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projection/reconcile_ast.mbt (1)
55-84:⚠️ Potential issue | 🟠 Major
same_kindis too weak for the child LCS key.At Line 57 and Line 76, the DP only distinguishes children by
T::same_kind. With the current@ast.Termimpl inprojection/traits_term.mbt:23-37, everyVar(_)matches every otherVar(_)and everyBop(_, _, _)matches every otherBop(_, _, _). A middle insertion like[x, y] -> [x, z, y]therefore backtracks asx -> z,y -> y, so the newzinheritsx'snode_idand the unchangedxgets a fresh one. That breaks identity preservation for same-tag sibling lists. Please tighten the child-match predicate here, e.g.same_kind + Renderable::label, or a dedicated stable-match key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projection/reconcile_ast.mbt` around lines 55 - 84, The LCS child-matching currently uses only T::same_kind in the DP fill and backtrack (the comparisons in the loops referencing old_children[i - 1].kind and new_children[j - 1].kind), which is too weak and causes wrong identity preservation; update both comparisons (the if in the dp fill and the if in the backtracking that sets old_matched/new_matched) to use a stronger stable-match key instead of just T::same_kind — for example combine T::same_kind with Renderable::label (or call a new stable_match_key(old_children[i-1], new_children[j-1]) helper) so siblings with the same tag but different labels don’t erroneously match and node_id assignments stay stable.
🧹 Nitpick comments (1)
projection/tree_editor_refresh.mbt (1)
235-238: Consider simplifying boolean initialization.This pattern can be condensed using the
ispattern:♻️ Optional simplification
- let mut children_reused = match previous_loaded_children { - Some(_) => true - None => false - } + let mut children_reused = previous_loaded_children is Some(_)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projection/tree_editor_refresh.mbt` around lines 235 - 238, The boolean initialization using a match on previous_loaded_children can be simplified: replace the match that assigns children_reused with a direct check using previous_loaded_children.is_some() (or the `is` pattern equivalent) so children_reused is set directly from the Option presence; update the assignment where children_reused is defined to use this shorter idiom referencing previous_loaded_children.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@projection/reconcile_ast.mbt`:
- Around line 55-84: The LCS child-matching currently uses only T::same_kind in
the DP fill and backtrack (the comparisons in the loops referencing
old_children[i - 1].kind and new_children[j - 1].kind), which is too weak and
causes wrong identity preservation; update both comparisons (the if in the dp
fill and the if in the backtracking that sets old_matched/new_matched) to use a
stronger stable-match key instead of just T::same_kind — for example combine
T::same_kind with Renderable::label (or call a new
stable_match_key(old_children[i-1], new_children[j-1]) helper) so siblings with
the same tag but different labels don’t erroneously match and node_id
assignments stay stable.
---
Nitpick comments:
In `@projection/tree_editor_refresh.mbt`:
- Around line 235-238: The boolean initialization using a match on
previous_loaded_children can be simplified: replace the match that assigns
children_reused with a direct check using previous_loaded_children.is_some() (or
the `is` pattern equivalent) so children_reused is set directly from the Option
presence; update the assignment where children_reused is defined to use this
shorter idiom referencing previous_loaded_children.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8562df42-2ae9-4402-8eda-a944ea6a4d42
📒 Files selected for processing (9)
projection/lens_test.mbtprojection/pkg.generated.mbtiprojection/proj_node.mbtprojection/reconcile_ast.mbtprojection/traits.mbtprojection/traits_term.mbtprojection/tree_editor_model.mbtprojection/tree_editor_refresh.mbtprojection/tree_editor_wbtest.mbt
✅ Files skipped from review due to trivial changes (1)
- projection/traits.mbt
🚧 Files skipped from review as they are similar to previous changes (4)
- projection/traits_term.mbt
- projection/pkg.generated.mbti
- projection/proj_node.mbt
- projection/tree_editor_wbtest.mbt
InteractiveNodeStamp was storing kind: T, causing deep O(subtree) equality comparison via derive(Eq) on @ast.Term. Replaced with kind_tag: String (from Renderable::kind_tag) for O(1) comparison. Deep child changes are caught by the recursive child walk in can_skip_subtree and refresh_node_minimal, not by stamp equality. Also removed now-unnecessary Eq bounds from TreeEditorState methods (from_projection, refresh, expand_node, hydrate_subtree). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
MoonBit deprecated .is_some() — CI treats deprecation as error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The package-level import of dowdiness/lambda/ast in moon.pkg requires the module-level dependency in moon.mod.json. Without it, Cloudflare Workers build fails with "module is not imported" error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cloudflare Workers build uses a MoonBit version that doesn't support the Type[Param]::method() call syntax. Use type annotation on the let binding instead: `let x : Type[Param] = Type::method(...)`. Affects: examples/rabbita/, examples/ideal/ Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
ProjNode[T],SyncEditor[T],TreeEditorState[T],EditContext[T]over a generic AST typeTconstrained byTreeNode + Renderabletraits@ast.Term) implements both traits — existing behavior unchangedSyncEditor::new_lambdapreserves backward compatibility for all consumersArchitecture
Two capability traits in
projection/traits.mbt:TreeNode—children(Self) -> Array[Self],same_kind(Self, Self) -> BoolRenderable—kind_tag,label,placeholder,unparseAny MoonBit type implementing these traits can now be used with the projection pipeline. The framework supports any grammar that provides a parser + AST structure.
Changes
56 files changed (+917, -819)
projection/traits.mbt— newTreeNodeandRenderabletrait definitionsprojection/traits_term.mbt—impl TreeNode + Renderable for @ast.Termprojection/proj_node.mbt—ProjNode→ProjNode[T]projection/reconcile_ast.mbt— genericreconcile[T], backward-compatreconcile_astaliasprojection/tree_editor_model.mbt—InteractiveTreeNode[T], removedInteractiveNodeShapeprojection/text_edit.mbt—EditContext[T], handler files parameterizededitor/sync_editor.mbt—SyncEditor[T]withmake_parserfactorycrdt.mbt— concreteSyncEditor[@ast.Term], usesnew_lambdaWhat this enables
Test plan
moon checkpasses (3 cosmetic warnings from MoonBit trait bound detection)moon info && moon fmtcleanFollow-up (Phase 2)
framework/, lambda code tolang/lambda/🤖 Generated with Claude Code
Summary by CodeRabbit