refactor: decompose compute_text_edit into handler chain with middleware#54
Merged
refactor: decompose compute_text_edit into handler chain with middleware#54
Conversation
Bundle the 5 parameters of compute_text_edit (source_text, source_map, registry, flat_proj) into an EditContext struct. Add EditResult type alias. Update all 54 test call sites, the AddBinding recursive call, and the editor/tree_edit_bridge.mbt call site. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove the now-unnecessary `let { source_text, source_map, registry,
flat_proj } = ctx` from compute_text_edit since all arms delegate to
extracted handler functions.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… comments - Extract find_def_index and binding_delete_range helpers to text_edit_utils.mbt - Replace 6 duplicated def-index-lookup patterns across binding/refactor/rename files - Replace 3 duplicated delete-range 3-way branches with binding_delete_range - Inline ValidateNodeExists middleware call, removing wrap closure allocation - Remove unused wrap function and redundant dispatch comments 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. |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR refactors projection text-edit wiring: it introduces an Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Editor (apply_tree_edit)
participant API as compute_text_edit(op, ctx)
participant Middleware as ValidateNodeExists (middleware)
participant Dispatcher as core_dispatch / compute_* helpers
participant Registry as ctx.registry / source_map
Editor->>API: call(op, EditContext)
activate API
API->>Middleware: ValidateNodeExists.apply(op, ctx)
activate Middleware
Middleware->>Middleware: extract_primary_node_id(op)
alt primary id present
Middleware->>Registry: ctx.registry.contains(id)?
alt exists
Middleware->>Dispatcher: forward(op, ctx)
activate Dispatcher
Dispatcher->>Registry: lookup nodes / ranges
Dispatcher->>Dispatcher: compute SpanEdit(s) + FocusHint
Dispatcher-->>API: Ok(Some((edits, hint)))
deactivate Dispatcher
else missing
Middleware-->>API: Err("Node not found: <id>")
end
else no primary id
Middleware->>Dispatcher: forward(op, ctx)
activate Dispatcher
Dispatcher->>Registry: lookup as needed
Dispatcher-->>API: Ok(Some((edits, hint)))
deactivate Dispatcher
end
deactivate Middleware
API-->>Editor: EditResult
deactivate API
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 |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
rabbita | 56bc095 | Commit Preview URL Branch Preview URL |
Mar 23 2026, 08:08 AM |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
lambda-editor | 57e198a | Commit Preview URL Branch Preview URL |
Mar 23 2026, 08:44 AM |
Benchmark Comparison ReportComparing PR branch against Main Module BenchmarksBase branch: PR branch: event-graph-walker BenchmarksBase branch: PR branch: Benchmarks run 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: 5
🧹 Nitpick comments (1)
projection/text_edit_delete.mbt (1)
8-15: Consider indexing parent relationships for efficiency.The current parent lookup iterates through all registry entries and their children (O(n×m) complexity). For large ASTs, this could become a bottleneck. Consider maintaining a
child_to_parentmap inEditContextif profiling shows this as a hot path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projection/text_edit_delete.mbt` around lines 8 - 15, The parent lookup loop (parent_info / registry / pnode.children) is O(n×m) and should be optimized by adding and using a child->parent index; update EditContext to maintain a child_to_parent map keyed by NodeId (or the same NodeId wrapper used here) that stores (parent_id, ProjNode, child_index), populate/maintain it on node insert/delete/modify operations, and replace the nested iteration in the block that sets parent_info with a direct map lookup (using NodeId(child.node_id) as the key) so you avoid scanning registry and children.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@projection/text_edit_binding.mbt`:
- Around line 90-99: The mirrored dependency check currently prevents valid
swaps: in the "move up" block only a dependency where the current definition
(cur_def.0) uses the previous definition (prev_def.0) is unsafe, so remove the
check that returns Err when prev_fv.contains(cur_def.0); conversely, in the
"move down" block only prev depending on cur is unsafe, so remove the mirrored
check there. Update the guards around cur_fv.contains(prev_def.0) /
prev_fv.contains(cur_def.0) (and the duplicate logic at the other block around
lines referenced by prev_fv/cur_fv) so each move-direction only tests the
dependency that actually blocks that direction (use cur_fv.contains(prev_def.0)
for move-up and prev_fv.contains(cur_def.0) for move-down).
- Around line 205-211: compute_add_binding currently delegates to
compute_insert_child using module_node_id without verifying the target node is a
Module, which can allow a stale module_node_id to mutate the wrong AST; add a
guard in compute_add_binding to resolve and validate that module_node_id
references a Module node (or return/fail early) before calling
compute_insert_child, and ensure the non-Module branch in
projection/text_edit_wrap.mbt (the rewrite path around lines 172-191) does not
silently rewrite arbitrary parents—explicitly handle or error on non-Module
parents to prevent accidental AST shape mutations.
In `@projection/text_edit_refactor.mbt`:
- Around line 186-202: The MoveCursor position is using pre-edit coordinates
(range.start) when the binding deletion (del_len at del_start) occurs before
that position; update the returned cursor to post-edit coordinates by
subtracting the deleted length (use range.start - del_len) when building the
MoveCursor in the branch that handles range.start >= binding_end (the tuple with
two edits and MoveCursor), so the cursor reflects the document after applying
the binding deletion; ensure you reference the same variables (range.start,
del_len, MoveCursor) when making the adjustment.
In `@projection/text_edit_structural.mbt`:
- Around line 8-29: In compute_unwrap: validate that keep_child_index is within
bounds (0 <= keep_child_index < node.children.length()) and return Err when out
of range; then compute the post-edit cursor by using the kept child's offset
relative to its original range: get kept_range_start (from
source_map.get_range(kept.id()) or default to range.start), compute
kept_relative = kept_start - kept_range_start, and set MoveCursor(position =
range.start + kept_relative) instead of reusing the pre-edit absolute
kept_start; refer to symbols keep_child_index, node.children, kept_start,
kept_range_start, range.start and MoveCursor.
In `@projection/text_edit_wrap.mbt`:
- Around line 122-156: In compute_insert_child, reject negative index values
before any child indexing or forwarding: add a guard that returns an Err when
index < 0 (with a clear message like "Negative index") so you never access
parent_node.children[index] or pass a negative index into insert_child_at;
ensure this check runs early (before matching on parent_node.kind) and covers
both the Module branch (which indexes parent_node.children) and the non-module
branch that calls insert_child_at.
---
Nitpick comments:
In `@projection/text_edit_delete.mbt`:
- Around line 8-15: The parent lookup loop (parent_info / registry /
pnode.children) is O(n×m) and should be optimized by adding and using a
child->parent index; update EditContext to maintain a child_to_parent map keyed
by NodeId (or the same NodeId wrapper used here) that stores (parent_id,
ProjNode, child_index), populate/maintain it on node insert/delete/modify
operations, and replace the nested iteration in the block that sets parent_info
with a direct map lookup (using NodeId(child.node_id) as the key) so you avoid
scanning registry and children.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da01069b-228d-45a3-b568-2d49923e216c
📒 Files selected for processing (14)
editor/tree_edit_bridge.mbtprojection/pkg.generated.mbtiprojection/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.mbt
…dations - Move binding scoping: remove over-restrictive guards that blocked valid swaps (move-up rejected when prev used cur, move-down rejected when cur used next — both safe after swap) - InlineDefinition cursor: use post-edit coordinates (range.start - del_len) when binding deletion precedes the var - Unwrap cursor: use range.start (replacement start) instead of kept child's pre-edit position which is invalidated by the edit - AddBinding: validate target is a Module node before delegating - InsertChild: reject negative index values - Unwrap: add lower-bound check on keep_child_index Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
projection/text_edit_refactor.mbt (1)
274-279: Consider handling missing source ranges explicitly.If
source_map.get_range(uid)returnsNonefor a usage, it's silently skipped. This could lead to an inconsistent state where the binding is deleted (line 293) but some usages remain unreplaced, resulting in dangling references.While this edge case is unlikely for normal source code, defensive handling would improve robustness.
♻️ Optional: Fail if any usage lacks a source range
for uid in usage_ids { match source_map.get_range(uid) { Some(r) => usage_ranges.push((r.start, r.end)) - None => () + None => return Err("Usage node missing source range") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projection/text_edit_refactor.mbt` around lines 274 - 279, The loop over usage_ids silently skips uids where source_map.get_range(uid) returns None, which can leave usages unreplaced after binding deletion (usage_ranges is built from ranges and the binding is removed later); update the logic that iterates usage_ids to explicitly handle missing ranges by collecting missing uids and returning an error or aborting (or logging and skipping the entire refactor) if any are missing instead of silently ignoring them—modify the block that builds usage_ranges (the for uid in usage_ids { match source_map.get_range(uid) { ... } } loop) to detect None cases and fail-fast or surface a clear error referencing the offending uid(s) before proceeding to the code that deletes the binding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@projection/text_edit_wbtest.mbt`:
- Around line 1185-1195: The test's module_id is actually taken from
fp.final_expr.unwrap().id() (the final expression Var("x")), so the failure may
be due to an un-insertable parent rather than the negative index; update the
test so InsertChild targets a real insertable module node: obtain the actual
module ID from the parsed file (instead of fp.final_expr.unwrap().id()) and pass
that ID to InsertChild, then assert Err(_) to verify negative index handling;
alternatively, if you intend to keep the existing parent, rename module_id to
something like final_expr_id to reflect its true value so the test name matches
behavior. Ensure references to compute_text_edit, InsertChild, make_edit_ctx and
fp are updated accordingly.
---
Nitpick comments:
In `@projection/text_edit_refactor.mbt`:
- Around line 274-279: The loop over usage_ids silently skips uids where
source_map.get_range(uid) returns None, which can leave usages unreplaced after
binding deletion (usage_ranges is built from ranges and the binding is removed
later); update the logic that iterates usage_ids to explicitly handle missing
ranges by collecting missing uids and returning an error or aborting (or logging
and skipping the entire refactor) if any are missing instead of silently
ignoring them—modify the block that builds usage_ranges (the for uid in
usage_ids { match source_map.get_range(uid) { ... } } loop) to detect None cases
and fail-fast or surface a clear error referencing the offending uid(s) before
proceeding to the code that deletes the binding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e03668a6-8267-4ce9-9dd2-bca2076aaf26
📒 Files selected for processing (5)
projection/text_edit_binding.mbtprojection/text_edit_refactor.mbtprojection/text_edit_structural.mbtprojection/text_edit_wbtest.mbtprojection/text_edit_wrap.mbt
🚧 Files skipped from review as they are similar to previous changes (3)
- projection/text_edit_structural.mbt
- projection/text_edit_wrap.mbt
- projection/text_edit_binding.mbt
Comment on lines
+1185
to
+1195
| ///| | ||
| test "compute_text_edit: InsertChild rejects negative index" { | ||
| let text = "let x = 0\nx" | ||
| let (_, source_map, registry, fp) = parse_with_token_spans(text) | ||
| let module_id = fp.final_expr.unwrap().id() | ||
| let result = compute_text_edit( | ||
| InsertChild(parent=module_id, index=-1, kind=@ast.Term::Int(0)), | ||
| make_edit_ctx(text, source_map, registry, fp), | ||
| ) | ||
| inspect(result is Err(_), content="true") | ||
| } |
There was a problem hiding this comment.
Misleading variable name and potential test coverage gap.
The variable module_id is assigned from fp.final_expr.unwrap().id(), which retrieves the final expression's ID (Var("x")), not the module's ID. This means the test may be rejected due to the parent not being insertable, rather than the negative index validation the test name claims to verify.
Consider either:
- Use the actual module ID to isolate the negative index validation
- Rename the variable to reflect what it actually holds
Proposed fix to test negative index on a valid parent
test "compute_text_edit: InsertChild rejects negative index" {
let text = "let x = 0\nx"
- let (_, source_map, registry, fp) = parse_with_token_spans(text)
- let module_id = fp.final_expr.unwrap().id()
+ let (proj, source_map, registry, fp) = parse_with_token_spans(text)
+ let module_id = proj.id()
let result = compute_text_edit(
InsertChild(parent=module_id, index=-1, kind=@ast.Term::Int(0)),
make_edit_ctx(text, source_map, registry, fp),
)
inspect(result is Err(_), content="true")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projection/text_edit_wbtest.mbt` around lines 1185 - 1195, The test's
module_id is actually taken from fp.final_expr.unwrap().id() (the final
expression Var("x")), so the failure may be due to an un-insertable parent
rather than the negative index; update the test so InsertChild targets a real
insertable module node: obtain the actual module ID from the parsed file
(instead of fp.final_expr.unwrap().id()) and pass that ID to InsertChild, then
assert Err(_) to verify negative index handling; alternatively, if you intend to
keep the existing parent, rename module_id to something like final_expr_id to
reflect its true value so the test name matches behavior. Ensure references to
compute_text_edit, InsertChild, make_edit_ctx and fp are updated accordingly.
- Add doc comments to EditContext, core_dispatch, EditMiddleware trait, ValidateNodeExists, extract_primary_node_id, find_def_index, binding_delete_range - Update design doc: remove stale wrap() closure, add Phase 5 (bug fixes), update file layout and testing sections Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dowdiness
added a commit
that referenced
this pull request
Mar 23, 2026
- Mark text_edit decomposition as done (PR #54) - Add follow-up items: Term enum extensibility, AST transform pipeline, coordinate arithmetic audit, parent lookup index Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dowdiness
added a commit
that referenced
this pull request
Mar 23, 2026
Audited all 9 MoveCursor sites across handler files. No new bugs found — the two cursor position bugs (unwrap, inline_definition) were already fixed in the handler chain PR (#54). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 task
dowdiness
added a commit
that referenced
this pull request
Mar 23, 2026
* docs: add handler chain follow-up TODOs - Mark text_edit decomposition as done (PR #54) - Add follow-up items: Term enum extensibility, AST transform pipeline, coordinate arithmetic audit, parent lookup index Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: mark coordinate arithmetic audit as done Audited all 9 MoveCursor sites across handler files. No new bugs found — the two cursor position bugs (unwrap, inline_definition) were already fixed in the handler chain PR (#54). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dowdiness
added a commit
that referenced
this pull request
Mar 24, 2026
- Test count: 472 → 507 - Add handler chain files (PR #54) to file map and Task 5 Step 3 - Add recovery/recovery_epoch fields to SyncEditor struct - EditContext[T] parameterization for middleware pipeline Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 tasks
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
projection/text_edit.mbt(1,064 lines → 96 lines) into per-file handler functions with an exhaustive thin routerEditContextstruct to bundle the 5 parameters ofcompute_text_editEditMiddlewaretrait andValidateNodeExistsmiddleware (SuperOOP-inspired composable chain)find_def_index(was duplicated 6x),binding_delete_range(was duplicated 3x)New files
text_edit_commit.mbt,text_edit_delete.mbt,text_edit_drop.mbttext_edit_wrap.mbt,text_edit_structural.mbt,text_edit_binding.mbttext_edit_refactor.mbt,text_edit_middleware.mbtCost of adding a future operation
tree_lens.mbttext_edit.mbttree_edit_json.mbttree_editor.mbtTest plan
moon check && moon test)🤖 Generated with Claude Code
Design spec:
docs/plans/2026-03-23-handler-chain-design.mdSummary by CodeRabbit
Release Notes