dnd: decouple target classification from the orchestrator#54
Merged
Conversation
bareforge.dnd.drag's resolve-target! mixed DOM walks, geometry classification, and effectful side-effects (highlight mutations, strip mount, self-recursion). Pull the *decision* — which of slot- row / layer-row / canvas-element / needs-strips / invalid does this gesture target — out into a new pure namespace, bareforge.dnd.target, that consumes a snapshot map and returns a tagged result. drag.cljs continues to drive the state machine (next commit will rewire resolve-target! to use the classifier). The classifier is unit-tested in target_test.cljs against hand- curated snapshots covering each branch of the legacy resolver: - :invalid (outside canvas; stale canvas-element id) - :canvas-element :before / :after / :inside - :canvas-element :inside on a container? true / strips? false tag (e.g. x-button) — must not return :needs-strips - :needs-strips with strip-host-id nil (first hover) - :needs-strips with strip-host-id pointing at a different node (stale strips need replacement) - :canvas-element :inside when strip-host-id equals hovered id (idempotency under stationary cursor — prevents flicker) - :slot-row with :hide-stale-strips? true / false - :layer-row with position classification classify-position moves from drag.cljs to dnd/target with it. drag_test.cljs is dropped — its 11 tests are reborn in target_test alongside 13 new classifier tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
resolve-target! braided four concerns — collect cursor + DOM context, decide which target type the gesture lands on, decide whether strips need mounting, apply highlights — into one 78-line cond with a self-recursive branch. The recursion existed because mounting slot strips inserts new DOM under the cursor and the second pass needs to pick the freshly-mounted strip up via element-under-cursor; the `re-entered?` flag bounded the depth. Split into three small effectful helpers plus a thin dispatcher: - `collect-snapshot` reads the DOM / state / slot-strip host once and returns a Clojure map the classifier consumes. - `target/classify-drop-target` (pure, previous commit) decides. - `apply-result!` projects the tagged result onto drag-state + DOM, with no business logic of its own. - `resolve-target!` runs collect → classify → if :needs-strips, mount strips → re-collect → re-classify → apply. The two-pass branch is explicit at the call site instead of a recursive call buried in a `re-entered?` argument, and the classifier's `:needs-strips` precondition (`not= id strip-host-id`) bounds the loop to exactly two iterations. `node-position` is gone — its work (BCR read + tag lookup + classify-position call) is inlined into `collect-snapshot` since the snapshot already carries `:hovered-rect` and `:container?`. drag.cljs grows ~28 lines net because the five-helper shape is wordier than the original braided cond, but the wins are structural: the decision is pure and unit-tested, and the strip- mount lifecycle is visible at the call site rather than buried in a recursive function argument. Manual smoke tests per the plan's verification section pending before opening the PR. Co-Authored-By: Claude Opus 4.7 (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
Closes the final audit item (#7). `bareforge.dnd.drag/resolve-target!` was 78 lines of cond braiding four concerns — DOM walks, geometry classification, strip-mount logic, and highlight mutation — and resolving "which slot did the user drop into?" via an imperative self-recursion at the old line 437. The recursion existed because mounting slot strips inserts new DOM under the cursor, and the second pass needed to pick the freshly-mounted strip up via `element-under-cursor`; the `re-entered?` flag bounded the depth.
This refactor extracts the decision into a new pure namespace `bareforge.dnd.target` and rewires drag.cljs as a thin orchestrator. The recursion is gone.
Two commits
`dnd: extract classify-drop-target as pure target classifier` — creates `src/bareforge/dnd/target.cljs` with two pure functions: `classify-position` (moved from drag.cljs) and the new `classify-drop-target`. The classifier consumes a snapshot map describing what the cursor sees and returns a tagged action — `:slot-row` / `:layer-row` / `:canvas-element` / `:needs-strips` / `:invalid`. drag.cljs continues to drive the state machine; the test file `drag_test.cljs` is replaced by `target_test.cljs` with the original 11 `classify-position` cases plus 13 new classifier cases against hand-curated snapshots.
`dnd: rewrite resolve-target! to dispatch on the classifier result` — splits `resolve-target!` into three small effectful helpers (`collect-snapshot`, `apply-result!`, plus the dispatcher itself) over the pure classifier. The two-pass strip-mount loop is an explicit branch at the call site rather than a recursive call buried in an argument; the classifier's `:needs-strips` precondition (`not= id strip-host-id`) bounds it to exactly two iterations by construction.
Notable contract details
File deltas
Test plan
Out of scope (audit follow-ups)
🤖 Generated with Claude Code