Add unit tests for web/data-visualizer's Tree and OriginalDrawer - #80
Merged
Shrey5132 merged 3 commits intoJul 29, 2026
Merged
Conversation
common and runner already had solid coverage, but web/data-visualizer - the package with the actually intricate logic - had none: cycle handling in Tree.fromSerializedNode, and OriginalDrawer's pixel math (getNodeWidth/getNodeHeight, backward-arrow routing). That gap is how the margin-clipping bug shipped silently earlier. Covers: plain non-shared arrays, a self-referential array (proves getNodeWidth/getNodeHeight terminate instead of recursing through the cycle, and that the self-loop correctly routes as a backward arrow), a shared-but-acyclic structure (proves sharing alone does NOT trip the backward-arrow path), a malformed unresolved-ref fallback, and basic width/height growth invariants. Every pixel value is hand-traced against the real formulas, not just re-derived from them.
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughAdds Vitest coverage for data visualizer tree parsing and ChangesTree and drawer test coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/web/data-visualizer/src/__tests__/tree.test.ts`:
- Around line 123-125: Update the height assertion in the tree drawing test to
use Config.ArrowMarginTop, matching the vertical margin used by minY, while
leaving the width assertion based on Config.ArrowMarginHorizontal.
🪄 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 Plus
Run ID: 3b6e4bf7-0102-4d85-9e73-e5eb12548927
📒 Files selected for processing (1)
src/web/data-visualizer/src/__tests__/tree.test.ts
Comment on lines
+123
to
+125
| const minShift = Config.ArrowMarginHorizontal + Config.StrokeWidth / 2; | ||
| expect(drawer.width).toBe(nodeWidth + minShift); | ||
| expect(drawer.height).toBe(nodeHeight + minShift + Config.StrokeWidth); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the vertical arrow margin for the height expectation.
minY is calculated with Config.ArrowMarginTop, but Line 125 reuses the horizontal margin. This encodes the wrong height contract when the two margins differ.
Proposed fix
- const minShift = Config.ArrowMarginHorizontal + Config.StrokeWidth / 2;
- expect(drawer.width).toBe(nodeWidth + minShift);
- expect(drawer.height).toBe(nodeHeight + minShift + Config.StrokeWidth);
+ const minShiftX = Config.ArrowMarginHorizontal + Config.StrokeWidth / 2;
+ const minShiftY = Config.ArrowMarginTop + Config.StrokeWidth / 2;
+ expect(drawer.width).toBe(nodeWidth + minShiftX);
+ expect(drawer.height).toBe(nodeHeight + minShiftY + Config.StrokeWidth);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const minShift = Config.ArrowMarginHorizontal + Config.StrokeWidth / 2; | |
| expect(drawer.width).toBe(nodeWidth + minShift); | |
| expect(drawer.height).toBe(nodeHeight + minShift + Config.StrokeWidth); | |
| const minShiftX = Config.ArrowMarginHorizontal + Config.StrokeWidth / 2; | |
| const minShiftY = Config.ArrowMarginTop + Config.StrokeWidth / 2; | |
| expect(drawer.width).toBe(nodeWidth + minShiftX); | |
| expect(drawer.height).toBe(nodeHeight + minShiftY + Config.StrokeWidth); |
🤖 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 `@src/web/data-visualizer/src/__tests__/tree.test.ts` around lines 123 - 125,
Update the height assertion in the tree drawing test to use
Config.ArrowMarginTop, matching the vertical margin used by minY, while leaving
the width assertion based on Config.ArrowMarginHorizontal.
Follow-up to the earlier tree/OriginalDrawer tests, closing gaps that those tests inadvertently exposed rather than covered (importing Tree.ts pulls in the whole package - including 5 previously-untracked drawable components - for the first time, so istanbul started scoring their unexecuted render() bodies against the aggregate). Added: a "function" wire-node case (construction, sharing, and drawing - FunctionTreeNode.createDrawable was completely untested), the real Tree.draw() entry point DataVisualizerView.tsx actually calls (only OriginalDrawer directly was tested before), OriginalDrawer's other top-level branch (a bare leaf with no pair/array), formatLeaf's truncation/quoting logic, and ArrayDrawable's render() - the one drawable with real branching (empty-vs-leaf slots, separator line count, the BoxMinWidth clamp for a real zero-element list case). Left uncovered on purpose: ArrowDrawable, BackwardArrowDrawable, FunctionDrawable, NullDrawable - static prop-to-JSX mapping with no real branching (their arrow-routing *decisions* are already covered via OriginalDrawer; only the leaf visual components' own render bodies aren't, and treating those isn't worth the friction of reaching into memo()-wrapped internals).
Contributor
Author
Why the Coveralls status shows a decrease
Pushed a follow-up commit closing most of that gap with tests that are genuine regression coverage, not padding for the metric:
Result: -15.9% → -4.6% (82.536% → 93.763%). What's still deliberately uncovered: This is a non-blocking status check (separate from the required |
CodeRabbit caught this: OriginalDrawer's own minY update uses Config.ArrowMarginTop, distinct from minX's ArrowMarginHorizontal. The test only passed because both happen to be 15 right now - it was asserting the wrong formula, not a coincidentally-right one.
This was referenced Jul 29, 2026
Closed
martin-henz
pushed a commit
that referenced
this pull request
Jul 29, 2026
…81) * Restore Binary Tree / General Tree view modes Prof Martin flagged that dropping these during the Conductor port (deleted outright, "now fully unused") was a mistake — they're an important feature, not dead weight. Ports the old pre-Conductor frontend's classification (isBinaryTree/isGeneralTree) and layout (initializeTreeMetaData) algorithms from dataVisualizer.tsx/Tree.tsx, and the two drawers, onto the new wire format: - classify.ts: re-derives cycle/shared-structure detection and binary/general-tree classification against SerializedDataVisualizerNode (a tagged N-ary "array" node) instead of the old code's raw nested-pair walking (`structures[0]`/`structures[1]` on live objects), generalized from a fixed 2-element-pair representation since a native list isn't fixed at length 2. Keyed by RefId instead of WeakMap<object, _> — no live object identity crosses the wire. - BinaryTreeDrawer.tsx/GeneralTreeDrawer.tsx: the drawing algorithms themselves are an unchanged, verified line-for-line port (every leftCOUNTER/rightCOUNTER/EY1/EY2/scalerV/originIndex calculation) — only the state-access pattern changed, from the old code's global DataVisualizer statics (DataVisualizer.isBinTree, .TreeDepth, ...) to an injected per-call ClassificationResult. - BaseTreeNode.ts: restores nodePos/nodeColor, populated in Tree.fromSerializedNode from the classification's computed layout. - Tree.ts: draw() now takes a ViewMode and returns the matching drawer instance (mirrors the old Tree.tsx's draw()) instead of drawing directly — callers now do `.draw(viewMode).draw(x, y, key)`. - DataVisualizerView.tsx: a global Original/Binary Tree/General Tree toggle, applying across every call and argument (matching the old tool's single static toggle). Doesn't hide anything — a structure that isn't actually the selected shape still renders, via the drawers' own "not a binary/general tree" warning box. OriginalDrawer.tsx's Tree import switches to `import type` — needed to break a real circular import (Tree.ts -> BinaryTreeDrawer.tsx -> OriginalDrawer.tsx -> Tree.ts) introduced by Tree.ts now importing the drawer subclasses as values. * Add tests for classify, BinaryTreeDrawer, GeneralTreeDrawer classify.test.ts: cyclic and shared-but-acyclic structures classify as neither tree kind; a proper binary tree and a proper general tree each classify correctly with a populated layout; a flat list and a bare pair are correctly rejected as binary trees. treeViews.test.ts: both drawers' non-matching-shape warning box (exact size and message), a valid tree drawing without throwing to a positive finite size, and width/height growing with more nodes — mirrors the rigor from the original OriginalDrawer/Tree tests (#80) rather than just smoke-testing the new files. tree.test.ts: updates the one existing test that called Tree.draw(x, y, key) directly for the API change (Tree.draw(viewMode) now returns a drawer instance). * fix: correct isGeneralTreeNode to match the old tool's actual (permissive) semantics My port required an exact `[data, [childrenList, <end>]]` shape (label, then one nested branches-list) — but hand-tracing the old isGeneralTree against several worked examples shows it actually walks the *whole* proper list uniformly: any element that's itself compound is recursively re-checked, with no slot singled out as "the children list". That accepts both SICP-textbook `list(label, branches)` *and* a flat `llist(label, child1, child2, ...)`, since nothing distinguishes the second slot from the third. Concretely, draw_data(llist(1, llist(2, None, None), llist(3, None, None))) was rejected by General Tree View - a flat llist chain, not the nested-list encoding my port required. One consequence worth calling out: since a binary tree is structurally just a proper list whose compound elements happen to recurse the same way, a valid binary tree is now also always a valid general tree. Confirmed (by tracing the old algorithm by hand, not just this port) this isn't a new bug - General Tree View was always meant to be a strict superset of Binary Tree View, not a disjoint shape. Updated the one test that encoded the old (incorrect) assumption, added coverage for the flat-llist and binary-tree-is-general-tree cases. Live-verified in-browser against the reported failing case. * Improve test coverage for tree-view restoration classify.ts, BinaryTreeDrawer.tsx, GeneralTreeDrawer.tsx, OriginalDrawer.tsx and Tree.ts (this PR's own files) went from 87.76%/86.26% stmt/branch coverage to 97.13%/94.42%. Closes: - classify.ts: the data-slot-is-empty short-circuit in isBinaryTreeNode, mismatched sibling data-type labels, and an improper compound head rejecting a general tree. - BinaryTreeDrawer/GeneralTreeDrawer: drawNode's cycle-arrow (forward and backward) and function-node branches, via direct Tree/classification construction — unreachable through the real Tree.fromSerializedNode path since any AlreadyParsedTreeNode forces isSharedStructure, which always forces isBinaryTree/isGeneralTree false, so draw()'s warning-box check short-circuits before drawNode ever runs. Still real, hand-ported logic worth protecting from regression. Also added GeneralTreeDrawer's missing counterpart to BinaryTreeDrawer's existing bare-empty-terminator test. - Tree.ts: getNodeById, previously uncalled by any test. - OriginalDrawer.tsx: the unrecognized-TreeNode-subtype and null-children ArrayTreeNode fallback branches in getNodeWidth/getNodeHeight. The one remaining line in classify.ts (133) and one in BinaryTreeDrawer.tsx (78) are left uncovered: the former is provably unreachable per the preceding code's own logic (see its comment), the latter is a narrow leftCOUNTER/rightCOUNTER-both-zero arithmetic edge in the ported layout math not worth a contrived test for one line. DataVisualizerView.tsx remains untested — it needs real component-rendering infra (no react-test-renderer/jsdom precedent exists anywhere in this monorepo today), a separate decision from adding more unit tests. * Add DataVisualizerView component tests The view-mode toggle component had zero coverage - not even partial, since nothing in the package imported it before now. Covers: the no-rows placeholder, view-mode button state, multi-argument "Structure N" labeling, Previous/Next pagination clamped at both ends, ArrowLeft/ ArrowRight keyboard nav (and its suppression while focus is in an editable element), and the currentStep reset on a fresh set of rows. Uses react-test-renderer + a few hand-written window/document/ HTMLElement stubs rather than jsdom, matching this package's existing practice of precise hand-mocking (konva/react-konva) over pulling in heavier environment dependencies - no jsdom precedent exists anywhere in this monorepo. The one real snag: React 19 requires the initial TestRenderer.create() itself to run inside act(), not just later interactions, or effects (including the window.addEventListener call these tests depend on) aren't guaranteed to have run yet. DataVisualizerView.tsx: 0% (untested) -> 100% stmt/line, 95.45% branch. Package total: 97.13%/94.42% -> 97.36%/94.5% stmt/branch. * Address CodeRabbit review comments on PR #81 Fixed: - BinaryTreeDrawer.drawNode: rename unused originIndex/originX params to _originIndex/_originX (unlike GeneralTreeDrawer, Binary's own algorithm never reads them - confirmed via the pre-existing lint warning). - classify.test.ts: derive the shared-node test's sharedRef.refId from the refId the pair() helper actually assigned, instead of a hardcoded 1 that only happened to be correct because this was the file's first pair() call. - tree.test.ts: the Tree.draw('original') smoke test called drawer.draw() twice on the same stateful drawer instance; now calls once and reuses the result. - DataVisualizerView.test.tsx: the view-mode-click test only asserted the clicked Button's own `active` prop, which can't distinguish a correctly wired toggle from a bug that updates state but passes the wrong value into Tree.draw. Now spies on Tree.prototype.draw and asserts it's called with "original" before the click and "binaryTree" after. Skipped, with reasons: - GeneralTreeDrawer.tsx: the suggestion to hoist runningX2/downCOUNTER out of the per-node ArrayTreeNode visit into draw() would change behavior, not just refactor it. Checked against the old frontend's GeneralTreeDrawer.tsx (frontend/src/features/dataVisualizer/tree/ GeneralTreeDrawer.tsx:144-146) - it has the exact same per-node reset, just reading from DataVisualizer.nodeCount[0]/DataVisualizer.TreeDepth statics instead of this.classification.layout. This is a faithful line-for-line port of the old tool's actual behavior, not a regression introduced here - "fixing" it would be an unreviewed behavior change beyond this PR's restore-the-old-behavior scope. - DataVisualizerView.test.tsx migration to React Testing Library + jsdom: the suggestion asserts an "existing JSDOM test setup" that doesn't exist - grepped the whole monorepo, no jsdom/@testing-library reference anywhere. The user explicitly chose react-test-renderer + hand-rolled stubs over jsdom for this exact file in this same session, specifically because there's no jsdom precedent to build on; reversing that call on a mistaken premise isn't a "still-valid issue" to silently act on. Verification: npx tsc --noEmit, yarn eslint, yarn prettier --list-different, yarn vitest run all pass (63/63 in this package, no regressions elsewhere). --------- Co-authored-by: Shrey Jain <“shreyjain5132@email.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
Fixes #72.
common-data-visualizerandrunner-data-visualizeralready had solid coverage (11 tests, per #71's description), butweb/data-visualizer— the package containing the actually intricate logic (Tree.fromSerializedNode's cycle handling, andOriginalDrawer's pixel-math ingetNodeWidth/getNodeHeightand backward-arrow routing) — had none. As #72 points out, that's exactly the kind of code that already produced a silent bug once (the margin-clipping fix in #71).What's covered
Tree.fromSerializedNode: a plain non-shared array, a self-referential array (the wire form ofx = []; x.append(x)), a shared-but-acyclic structure (two refs to the same non-cyclic array), and a malformed unresolved-ref falling back to empty instead of throwing.OriginalDrawerpixel math, via the same trees drawn end-to-end:getNodeWidth/getNodeHeightterminate rather than recursing forever through the cycle (draw()doesn't throw/hang), and that a self-loop is correctly classified as a backward arrow (checked via the resultingminX/minYoffset baked into the final size).Every expected width/height in these tests is hand-traced against
Configconstants and the realgetNodeWidth/getNodeHeight/drawNodelogic, not just echoed back from the same formula.Note on konva mocking
Importing the real
konva/react-konvain a plain Node test environment makes Konva switch to its server-side build, which unconditionallyrequires the optionalcanvasnative package (not a dependency anywhere in this workspace). Since these tests only exercise layout arithmetic — never actual canvas rendering — I mocked both modules rather than pulling injsdom+canvasfor a logic-only test file.Test plan
yarn vitest run src/web/data-visualizer— 8/8 new tests pass.yarn build,yarn format:ci,yarn lint(0 errors, only pre-existing unrelated warnings),yarn vitest run --coverage(126/126) all pass locally.