feat(web): Introduce Tree View and Circles View in Web Viewer#1799
Conversation
|
@hugogu is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
hugogu
force-pushed
the
feature/viewer-update
branch
from
137a3ed to
72aa3d8
Compare
hugogu
changed the title
hugogu
changed the title
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 9934 tests passed 4 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
hugogu
force-pushed
the
feature/viewer-update
branch
from
72aa3d8 to
37f3899
Compare
✨ PR AutofixFound fixable formatting / unused-import issues across 22 changed lines. Comment |
hugogu
force-pushed
the
feature/viewer-update
branch
3 times, most recently
from
574d6c6 to
94a00db
Compare
|
Claude finished @magyargergo's task in 7m 47s —— View job Production-Readiness Review: PR #1799 — Tree View and Circles View in Web ViewerTasks
Review bar for this PRApplied: DoD.md v2.0.0, AGENTS.md v1.7.0, GUARDRAILS.md, CONTRIBUTING.md, TESTING.md, ARCHITECTURE.md (read; web-only, no pipeline boundaries in scope). All five DoD review gates applied (correctness, readability, architecture, security, performance). The PR-specific bar — large-graph readability improvement without browser freezes, React/Sigma lifecycle safety, accessible controls, complete i18n, valid CI — was used as the primary acceptance criteria. DoD.md — missing docs: All required docs present. Problem being solvedIssue #1797 (open): the force-directed graph becomes an unreadable dense cluster for moderately large repositories. Acceptance criteria include meaningfully easier readability for large graphs, distinguishable communities, structural navigation, preserved force graph behavior, and reasonable browser performance. This PR adds Tree View and Circles View as switchable alternatives, not replacements. Current PR stateHEAD SHA: Commit list (3 commits from base):
The head SHA visible in the PR (94a00db) is NOT the current head; CI status: All checks passed per the bot comment (typecheck ✅, unit tests ✅ across 3 platforms, E2E ✅ for web changes). Vercel deployment is blocked pending team authorization. PR Autofix found 22 fixable lines; whether the current head resolved them is unknown (no second autofix comment). CodeQL flagged an unused variable in Merge status and mergeabilityStatus: checks pending / review blocked All automated CI checks passed for the current head. However the Vercel authorization is unresolved, the CodeQL security warning is unresolved, and the PR Autofix comment is unresolved. Branch protection requiring these is unknown, but all three must be addressed. Based on visible state: checks passing, but bot comments unresolved — not currently mergeable under standard production-readiness criteria. Repository history considered
Branch hygiene assessmentClassification: merge-from-main present but harmless and merge-safe Both merge-from-main commits ( Understanding of the changeThe PR introduces
FindingsFinding A: Synchronous tree layout freezes the main thread for large graphsSeverity: blocker Risk: Evidence found:
Algorithmic bounds (confirmed, not estimated): Recommended fix: Gate tree view (and large circles view) behind a node-count check. For N > a documented threshold (e.g. 10,000 nodes), either: (a) show a "graph too large for tree view" message; (b) chunk the layout with Blocks merge: YES Finding B: E2E tests skip when services absent; coverage is shallowSeverity: high Risk: Evidence found:
Recommended fix: Add non-skipping vitest integration tests covering mode-switch behavior with a fixture graph (these can run without the backend). Add at least one test verifying that switching to tree mode while a node is selected resets selection. Add a bounded algorithmic test asserting that Blocks merge: YES (together with Finding A, the production feature is effectively untested for its core value proposition of large-graph readability) Finding C: Mode switch clears all user selection state without warningSeverity: medium Risk: Switching view modes destroys the selected node, hover state, and resets zoom. A user who selects a node, switches to tree view, and then back to force view has lost their selection. This is counter to typical tab/view switching UX where semantic state is preserved. Evidence found:
Recommended fix: Preserve Blocks merge: no (UX regression, not a correctness or safety issue) Finding D: Force graph behavior — no regression detectedStatus: confirmed (no regression)
Finding E: Math.random() in edge curvature for tree and circles adaptersSeverity: medium Risk: Evidence found:
Recommended fix: Replace Blocks merge: no (but causes visible flicker on mode re-entry) Finding F: Constants duplicated between layout files and useSigma.tsSeverity: medium Risk: Evidence found:
Recommended fix: Import and reuse Blocks merge: no Finding G: CodeQL unused variable
|
| Test | What it covers | Gap |
|---|---|---|
tree-layout.test.ts |
8 tests for layer ordering, grid distribution, parent-child X alignment, edge attraction | No performance bound for large N |
graph-adapter.test.ts |
3 tests for tree adapter: node presence, hierarchy vs cross-cutting edge styling, IMPORTS as cross-cut | No circles adapter tests; no determinism test; no large-N test |
graph.test.tsx |
Default mode = 'force', setter works | Only 2 tests; no mode-switch-with-selection or filter interaction |
tree-view.spec.ts |
Tab switching active state, file tree click → "Clear" button appears | Skips without services; no graph render verification |
filter-panel.test.ts |
Icon mappings, color legend coverage | Correctly covers the new Property/Const/Target icons |
Notable absence: no test for knowledgeGraphToCirclesGraphology, no test that changing graphViewMode correctly routes to the right adapter, no test that the filter effect re-applies on mode switch, and no synthetic performance guard.
Vitest config change (vitest.config.ts): the change updated the mermaid alias path — unrelated to this PR's feature but may be a housekeeping fix.
Branch hygiene and repository history
Two merge-from-main commits present; both bring only non-web backend fixes. All 18 changed files are causally connected to the tree/circles feature. No workflow churn, no lockfile churn, no generated artifacts. The branch is narrowly focused — satisfies the PR #362 precedent of staying focused.
Hidden Unicode and hygiene
- Conflict markers: none found (visual inspection of all changed files).
- Console logs/debug flags: none introduced.
- TODOs without issue references: none.
- Hardcoded localhost:
tree-view.spec.ts:14-15useslocalhost:4747/localhost:5173as defaults withprocess.envoverrides — acceptable for E2E setup. - Skipped tests:
tree-view.spec.tsskip is conditional on service availability — documented behavior. - Hidden Unicode: could not run scanner (sandbox restriction); manual inspection found nothing suspicious.
- Unused variable at
useSigma.ts:1314(const r) — CodeQL confirmed, unresolved.
Back-and-forth avoided by verifying
- The merge-from-main commits were verified directly in git log — confirmed harmless, no web changes.
- The current HEAD was verified (
1519e0e, not94a00dbas visible in some PR UI views). deterministicHashwas confirmed present in both layout files — avoiding a question about whether random jitter was justified.- The FA2/noverlap streaming path was confirmed unmodified — PR [codex] fix large repository graph loading #732 is not regressed.
- Constants duplication was directly verified by reading both files — the comment "must match" confirms this is an acknowledged risk, not just a style concern.
Open questions (unavoidable)
-
What is the supported graph size for Tree View? The PR is motivated by large-graph readability but documents no limit. Is there an intended cap, or is the expectation that
calculateTreeLayoutis fast enough at the sizes users have (e.g., is the real-world p99 graph size < 5K nodes)? -
Was the autofix comment resolved by the latest push? The autofix bot found 22 fixable lines on the prior head. The current head is newer but no second autofix comment appeared.
-
Is selection-clearing on mode switch intentional product decision? The PR description does not address it.
Final verdict
not production-ready
The implementation of tree and circles layouts is architecturally sound, well-tested at the unit level, and properly localized. However, Finding A (synchronous calculateTreeLayout main-thread blocking) is a confirmed blocker for the PR's stated goal of improving large-graph readability: the feature cannot be safely used for the repository sizes documented in open issues #705 and #761, and will produce multi-second to multi-minute browser freezes at those scales. The PR makes no claim about supported graph sizes and provides no user-facing guard, which means the feature would be harmful rather than helpful for large repositories.
Additionally, the CodeQL warning (unused variable r, Finding G) is unresolved and must be addressed before merge per the project's own review gates. The E2E coverage (Finding B) relies on conditional skipping and does not validate the core graph-switching behavior independently of live services. Together, these gaps mean the feature is not validatable in CI without live services and is actively unsafe for the target use case.
The recommended path to merge: (1) add a node-count guard with a user-visible fallback for tree view at the large-graph threshold; (2) remove the unused r variable to clear the CodeQL warning; (3) replace Math.random() edge curvature with deterministicHash; (4) import shared constants from circles-layout.ts instead of duplicating; (5) add aria-pressed to view mode buttons; (6) add at least one non-skipping vitest test covering mode-switch behavior with a fixture graph.
|
@hugogu could you please review these findings from claude? 🙏 |
|
@magyargergo sure, will try do it today. |
I recommend looking up other PRs since they might have addressed this problem for you, they just weren't merged in because the main branch diverged so much that it would have been too much effort to rebase. Plus, we didn't have time to pick this up. So, we really appriciate your work here! |
|
Also some layouts that you may find interesting https://cambridge-intelligence.com/blog/automatic-graph-layouts/ |
Thanks for sharing it, it's a great reference, I realize the new Circles view is essentially the Radical Layout in it, and the Tree View is essentially the virtical Sequential Layout . Let me rename them in UI. I'm interested to implement some of the other layouts over time, gradually. (I suppose I shall not raise one PR with 10,000+ lines of code changes, shall I?) And it is my habit to rebase onto latest main if my PR fall behind, not a trouble to me. |
|
Ideally, we should do it incrementally |
hugogu
force-pushed
the
feature/viewer-update
branch
from
8cd4667 to
419e831
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code reviewCLAUDE.md compliance — GitNexus workflow rules Per CLAUDE.md (GitNexus rules section):
This PR modifies several indexed symbols ( Additionally, 3 bugs were found inline (see review comments):
|
hugogu
force-pushed
the
feature/viewer-update
branch
from
2c72a71 to
5db5465
Compare
|
CLAUDE.md violation: Per the CLAUDE.md GitNexus rules:
There is no evidence in the PR description or commit messages that |
There was a problem hiding this comment.
Pull request overview
Adds two alternate graph layouts (tree/sequential and concentric circles/radial) to the web viewer, including a new view-mode state, UI toggle, layout/adaptation logic, and accompanying unit + E2E coverage to improve readability on larger repositories.
Changes:
- Introduces deterministic initial layout generators for Tree and Circles views, plus Graphology adapters to seed Sigma with the new coordinates.
- Adds graphViewMode to app state and a Force / Sequential / Radial toggle in the canvas UI, wiring the mode into
useSigmalayout execution. - Expands test coverage (Vitest + Playwright) and updates i18n strings and Vitest configuration to pick up new colocated tests.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| gitnexus-web/vitest.config.ts | Extends Vitest include globs to pick up src/**/*.test.* tests. |
| gitnexus-web/test/unit/filter-panel.test.ts | Updates icon mapping expectations for new node labels (Property/Const). |
| gitnexus-web/src/locales/en/graph.json | Adds view-mode labels for the new toggle UI. |
| gitnexus-web/src/locales/zh-CN/graph.json | Adds Chinese view-mode labels for the new toggle UI. |
| gitnexus-web/src/lib/tree-layout.ts | New tree/sequential layout calculation (layered + proportional seeding + constrained relaxation). |
| gitnexus-web/src/lib/tree-layout.test.ts | Unit tests validating tree layout layering, spread, ordering, and stability properties. |
| gitnexus-web/src/lib/circles-layout.ts | New circles/radial layout calculation (ring assignment + parent-centered angular seeding). |
| gitnexus-web/src/lib/lucide-icons.tsx | Re-exports the Network icon for the new view toggle. |
| gitnexus-web/src/lib/constants.ts | Updates default visible/filterable labels to include Property/Const. |
| gitnexus-web/src/lib/graph-adapter.ts | Adds Graphology adapters for tree/circles layouts and switches to MultiGraph to preserve multiple edges per pair. |
| gitnexus-web/src/lib/graph-adapter.test.ts | Tests new adapters (layout presence, edge styling, and a performance budget check). |
| gitnexus-web/src/hooks/useSigma.ts | Adds tree/circles physics loops + layout switching support, and edge styling tweaks based on hierarchy-ness. |
| gitnexus-web/src/hooks/useAppState.tsx | Surfaces graphViewMode and its setter through app state. |
| gitnexus-web/src/hooks/app-state/graph.tsx | Stores graphViewMode in the graph state context. |
| gitnexus-web/src/hooks/app-state/graph.test.tsx | Unit tests for graphViewMode default and setter behavior. |
| gitnexus-web/src/components/GraphCanvas.tsx | Adds the Force/Sequential/Radial toggle UI and rebuilds the Sigma graph based on view mode. |
| gitnexus-web/src/components/FileTreePanel.tsx | Adds icon support for Property/Const nodes. |
| gitnexus-web/e2e/tree-view.spec.ts | E2E coverage for switching layouts and basic interaction in sequential layout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add alternate graph layouts to the web viewer with new graph view state, canvas controls, adapters, and Sigma layout logic for tree and concentric-circle rendering. Include layout and adapter tests plus tree-view E2E coverage aligned with the English UI labels, and tune node visibility, edge layering, large-graph behavior, and tree-layer spacing so the new views stay readable. Follow up the tree-view work by keeping noisy variables hidden by default and mapping Property/Const icons so filter coverage stays in sync with the expanded node taxonomy. Co-authored-by: OpenAI Codex <noreply@openai.com> AI-model: GPT-5 Codex
Finding A (blocker): calculateTreeLayout runs 14 synchronous spring iterations over all edges and nodes — O(N×E×14) + O(N log N) per layer per iteration — with no size guard. At 10K+ nodes this freezes the main thread for several seconds. Fix: make SPRING_ITERATIONS adaptive: - N > 10 000 → 0 iterations (proportional initial layout only) - N > 3 000 → 4 iterations - otherwise → 14 iterations (unchanged behaviour for small graphs) Also removes the unused `const r` at useSigma.ts:1314, which was a leftover after the radial-resistance decomposition was removed. This clears the CodeQL "unused variable" warning (Finding G). Co-authored-by: Claude <noreply@anthropic.com> AI-model: claude-sonnet-4-6
…ound Finding B (high): knowledgeGraphToCirclesGraphology had zero test coverage. Adds three new tests: - ring placement: verifies Folder→ring 0, File→ring 1, Function→ring 3 and confirms circles-specific attributes (circlesRing, circlesAnchorX/Y) are set while tree attributes (treeAnchorX/Y) are absent. - edge styling: CONTAINS is marked isHierarchyEdge=true with the hierarchy colour; CALLS is cross-cutting with its own colour. - CALLS cross-cutting: a lone CALLS edge between two Functions is correctly identified as a non-hierarchy edge. Also adds a performance-bound test for the tree adapter at 2 000 nodes / 4 000 edges (the adaptive 14-iteration path) asserting completion within 2 s — catches regressions to the O(N×E×iterations) main-thread blocking that Finding A identified. Co-authored-by: Claude <noreply@anthropic.com> AI-model: claude-sonnet-4-6
… Layout
Aligns the UI labels with standard graph layout terminology from the
Cambridge Intelligence taxonomy (cambridge-intelligence.com/blog/automatic-graph-layouts):
Tree View → Sequential Layout (顺序布局)
Circles → Radial Layout (径向布局)
Force Graph → Force Graph (unchanged)
Internal graphViewMode keys ('tree', 'circles', 'force') are unchanged —
only the displayed strings in en/zh-CN locales and the E2E button selectors
are updated.
Co-authored-by: Claude <noreply@anthropic.com>
AI-model: claude-sonnet-4-6
For graphs with N > 5 000 nodes, each rAF frame of runTreeLayout was
doing O(N log N) sort + O(N × k) repulsion pair comparisons (k ≈ 2 400
for a 20 K-node graph spread across 1 080 px at range 130). At that
scale each frame took hundreds of ms, making the canvas appear completely
frozen even though the physics loop was still running.
Fix mirrors the circles layout adaptive strategy:
N > 5 000 (large):
- Skip repulsion pass (O(N × k) → 0)
- Skip spread-force sort (O(N log N) → 0)
- Velocity cap raised to ±12 / ±6 px so nodes cover ground faster
- Damping 0.58, 1 sim step/frame, 30 s max duration
- Looser early-stop thresholds (max v 0.05, avg v 0.03, active 2 %)
N > 1 500 (medium):
- Velocity cap raised to ±6 / ±3 px
- 24 s max duration
- Repulsion and spread still active
N ≤ 1 500 (small):
- Unchanged behaviour (velocity ±3/±2, 18 s, all forces active)
Layer gravity (O(N)) and edge springs (O(E)) run for all graph sizes —
they provide the structural pull that replaces repulsion at large N.
Co-authored-by: Claude <noreply@anthropic.com>
AI-model: claude-sonnet-4-6
…ection
The sigma 'clickNode', 'clickStage', 'enterNode', and 'leaveNode' handlers
are registered in a one-time useEffect (empty dep array). They captured
options.onNodeClick via closure, so they always called the initial version
of handleNodeClick — the one created before the graph loaded where
`if (!graph) return` exits immediately.
Consequence: clicking a node in the canvas never updated the app-level
selectedNode state. This broke:
- The Focus Depth filter (warning "Select a node to apply depth filter"
persisted even after a canvas click)
- The depth hop filter not applying (selectedNode was always null)
- The code panel not opening on canvas node click
Fix: store the three callback props in refs (onNodeClickRef, onNodeHoverRef,
onStageClickRef) and update them synchronously on every render. The sigma
event handlers now read from the refs, so they always invoke the latest
version of the callbacks without needing to re-register.
Co-authored-by: Claude <noreply@anthropic.com>
AI-model: claude-sonnet-4-6
Bug 1 (useSigma.ts): forces in the tree physics loop were computed once before the sub-steps loop and reused for every step, causing 2× displacement on slow frames (>64ms, simulationSteps>1). Fix: move forceX/forceY Maps and all force accumulation (layer gravity, edge springs, repulsion, spread) inside the loop so each sub-step integrates from current node positions. Bug 2 (graph-adapter.ts): all three adapters used `graph.hasEdge(src,tgt)` as a dedup guard, which silently drops any second edge between the same node pair. A CALLS relationship between nodes that also have a CONTAINS edge was always lost. Fix: switch from `new Graph()` to `new MultiGraph()` (allows multiple edges per pair) and dedup by `rel.id` instead of by node pair. Bug 3 (graph-adapter.test.ts): the cross-cutting edge styling test never executed its CALLS branch because Bug 2 dropped the CALLS edge before the assertion ran. Fix: assert `sigmaGraph.size === 2` and verify both edges individually after collecting attrs by relationType. Co-authored-by: Claude <noreply@anthropic.com> AI-model: claude-sonnet-4-5
- Move radial layout force accumulation inside the sub-step loop so forces are recomputed from updated node positions each iteration instead of using stale forces computed before the loop began - Revert knowledgeGraphToGraphology from MultiGraph back to Graph with node-pair deduplication to prevent ForceAtlas2 from double-applying spring forces for node pairs that share multiple relation types - Add Target to the lucide-icons import in FileTreePanel.tsx so the Const node type icon resolves without a ReferenceError Co-authored-by: Claude <noreply@anthropic.com> AI-model: claude-sonnet-4-6
Edge visibility (useSigma.ts): HAS_METHOD / HAS_PROPERTY edges were hidden when any edge-type filter was active because those types are not in the EdgeType union. Normalize HAS_METHOD → DEFINES and HAS_PROPERTY → CONTAINS before the visibleTypes.includes() guard so Kotlin/Java hierarchy edges follow the same filter logic as their semantic equivalents. Force-mode edge styles (graph-adapter.ts): HAS_METHOD / HAS_PROPERTY fell back to the default gray color in the force-graph adapter because EDGE_STYLES had no entries for them. Added explicit entries using the same hues as DEFINES/CONTAINS so force mode renders Kotlin/Java hierarchy edges consistently with tree/circles. Accessibility (GraphCanvas.tsx, locales): the layout-mode switcher (Force / Tree / Circles) had no ARIA semantics. Added role="tablist" on the container and role="tab" + aria-selected on each button. Added the viewModes.label i18n key (used as aria-label on the tablist) to en and zh-CN locale files. Flaky test (graph-adapter.test.ts): replaced the hard 2 s wall-clock assertion with a structural check (node count + edge count) that is deterministic across CI hardware. Timing tests are inherently flaky and provide no correctness signal. Co-authored-by: Claude <noreply@anthropic.com> AI-model: claude-sonnet-4-5
hugogu
force-pushed
the
feature/viewer-update
branch
from
397e564 to
4e5df3d
Compare
Summary
Add alternate graph layouts to the web viewer with new graph view state, canvas controls, adapters, and Sigma layout logic for tree and concentric-circle rendering. Include layout and adapter tests plus tree-view E2E coverage, and tune node visibility, edge layering, and large-graph behavior so the new views stay readable.
The views are switchable via a set of toggle buttons:
The original view :
The circles view for the same data:
Another original view:
Equivalent circles view:
Equivalent Tree View:
Motivation / context
Close #1797
Areas touched
gitnexus/(CLI / core / MCP server)gitnexus-web/(Vite / React UI).github/(workflows, actions)eval/or other toolingAGENTS.md,CLAUDE.md,.cursor/,llms.txt, etc.)Scope & constraints
In scope
Explicitly out of scope / not done here
N/A
Implementation notes
There are two extra views are introduced.
Testing & verification
cd gitnexus && npm testcd gitnexus && npm run test:integration(if core/indexing/MCP paths changed)cd gitnexus && npx tsc --noEmitcd gitnexus-web && npm test(if web changed)cd gitnexus-web && npx tsc -b --noEmit(if web changed)gitnexus-web/e2e/)Risk & rollout
Checklist
AGENTS.md/ overlays changed: headers, scope block, and changelog updated per project conventions