refactor(schema): address PR #62/#63 code-review follow-ups#64
Merged
Conversation
Code-review follow-ups for the zoom-bridge (#62) and schema-graph (#63) PRs. Correctness / behavior: - ch-client: the transitive-lineage nodeCap now counts only *linked* nodes, not every standalone table — so a single large DB of mostly-unrelated tables no longer truncates on round 1 and skips its cross-DB lineage walk (#63 review). - schema-graph: guard the db-prefix label strip with `&& n.name` so a node with an empty name can't be rewritten to a blank title (#63 review). Altitude / cleanup: - dot-layout: split the schema-specific "lineage first, singles gridded below" policy out of the generic dagre wrapper into a dedicated `schemaLayout()`; `dagreLayout` reverts to its plain generic form (no opts). Drops the dead `usedCols` (== cols) term and the always-built `connected` set on the pipeline path (#62/#63 review). - dom: extract the fixed-popover anchor recipe (divide client coords by the zoom) into a pure, 100%-covered `fixedAnchor()` helper; the File menu and the Save/user popover now share it instead of hand-transcribing the math — which also moves the previously-untested anchoredPopover arithmetic under test (#62). - app: simplify `dragCtx.scale` to report the page zoom without the redundant per-axis ternary (dragValue already ignores scale for the % axes) (#62). - dom: note in zoomScale that the measured element is immaterial (page-global) (#62). Tests added for schemaLayout (grid extent + no-lineage + no-singles + empty), fixedAnchor (both corners + floors), the ·inner label-skip, and the linked-only nodeCap. All 1020 tests pass; dot-layout/dom back to 100% coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019kE9qbgBNBrfNgwg9fRsMJ
BorisTyshkevich
added a commit
that referenced
this pull request
Jun 28, 2026
Bugfix release rolling up the schema-graph + zoom work since 0.1.3: - #64: code-review follow-ups (schemaLayout extraction, fixedAnchor helper, linked-only transitive nodeCap, label-strip guard). - #65: drop the detail-pane "Insert SHOW CREATE" button, ring the selected node (double border), and clear that ring on every pane-close path (incl. Esc). No new runtime deps. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019kE9qbgBNBrfNgwg9fRsMJ
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.
Follow-up to the high-effort
/code-reviewof #62 (zoom coordinate bridge) and #63 (schema-graph: draw all tables / singles below / drop db prefix). Both reviews found no correctness bug; this PR fixes the substantive behavior items and the cleanup/altitude notes.Correctness / behavior
ch-client.js) — fix(schema): draw every table in a whole-DB graph, linked or not #63's main finding. The cap that bounds cross-DB lineage expansion was counting every standalone table now that the whole-DB view keeps them, so a single large DB of mostly-unrelated tables trippedtruncatedon round 1 and never followed its few cross-DB links. Now only edge-participating nodes count toward the cap; standalone tables (which never drive expansion) don't truncate the walk.&& n.name(schema-graph.js) — fix(schema): draw every table in a whole-DB graph, linked or not #63. Prevents a focus-db node with an empty name from being rewritten to a blank card title.Altitude / cleanup
schemaLayout()(dot-layout.js) — fix(zoom): bridge html{zoom} in full-view panel + drag/popover coordinates #62/fix(schema): draw every table in a whole-DB graph, linked or not #63 altitude. The schema-specific "lineage first, edge-less tables gridded below" policy moves out of the generic dagre wrapper into its own pure function;dagreLayoutreverts to its plain form (noopts/isolatedLast). Also drops the deadusedCols(provably== cols) and theconnectedset that was built even on the pipeline path.fixedAnchor()(dom.js) — fix(zoom): bridge html{zoom} in full-view panel + drag/popover coordinates #62 reuse + coverage. The "anchor a fixed popover under a button, dividing client coords by the zoom" recipe was hand-transcribed in three spots; the File menu and the Save/user popover now share one pure, 100%-covered helper — which also brings the previously-untestedanchoredPopoverarithmetic (app.js, lower gate) under test.dragCtx.scale(app.js) — drop the redundant per-axis ternary (dragValuealready ignoresscalefor the % axes).zoomScaledoc — the measured element is immaterial (zoom is page-global).The two design-level altitude notes (per-rule
calc(/var(--zoom))CSS pattern; grid-in-layout) are mitigated by the centralized--zoomvar + theschemaLayout/fixedAnchorextractions; a deeper "don't usezoom" rework was judged out of scope.Tests
New tests for
schemaLayout(grid extent with exact px, no-lineage, no-singles fallback, empty/missing-keys),fixedAnchor(both corners + min/gap floors), the·innerlabel-skip, and the linked-only nodeCap (standalone tables don't truncate the cross-DB walk). 1020 tests pass;dot-layout.jsanddom.jsback to 100% across the board.Verification
Built, deployed to antalya, drove
bentoclick(15 tables) post-refactor: inline + full view still render all 15 with the lineage chain first (y≤460) and the 12 singles gridded below (y≥711), card titles stripped of thebentoclick.prefix — identical to #63, confirming theschemaLayoutextraction is behavior-preserving.🤖 Generated with Claude Code
https://claude.ai/code/session_019kE9qbgBNBrfNgwg9fRsMJ