-
Notifications
You must be signed in to change notification settings - Fork 1
refactor: migrate Embedding Landscape to IconRailPanel #288
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Fix case mismatch in database stats (nodes.Concept → nodes.concepts) that caused Concepts and Sources to show 0 on the home page - Remove truncated descriptions from explorer sidebar items (keep on main view items where they serve as help stubs) - Add NavigationGraph component: static deterministic graph on home page that uses the platform's own visual language to explain itself. Nodes are clickable and navigate to corresponding views. - Widen home page content area (max-w-4xl → max-w-6xl)
Replace custom w-72 sidebar with the shared IconRailPanel component, matching the pattern used by ExplorerView, PolarityExplorer, BlockEditor, and ReportWorkspace. Two tabs: Ontologies (visibility list + stats footer) and Settings (color scheme, perplexity, metric, grounding, regenerate). Regenerate also available as a rail action for quick access.
Code Review: IconRailPanel Migration + Home Workspace AdditionsPR Size: 4 files, +475/-250 lines -- standard review tier. This PR contains two distinct changes: (1) migrating the Embedding Landscape sidebar to 1. IconRailPanel Migration -- Well ExecutedThe migration from the custom sidebar to What looks solid:
Pattern consistency confirmed: The signature 2. Stats Footer Layout -- Potential Scroll ConflictLocation: Observation: The Ontologies tab content uses Looking at <div className="flex-1 overflow-y-auto">
{tabs.find((t) => t.id === activeTab)?.content}
</div>The parent already has
However, there is a subtle concern: the parent also has Suggestion: Consider adding 3. Regenerate Action: Loading State FeedbackLocation: The Regenerate rail action triggers Suggestion: This is minor since the main canvas shows a loading state, but if other rail actions in the codebase show loading state, this should match for consistency. 4. HomeWorkspace Changes -- Out of Scope but ReviewingThe PR description says "migrates Embedding Landscape to IconRailPanel" but also:
These are separate concerns from the IconRailPanel migration. Not blocking, but worth noting in the PR description for traceability. The stats field rename in particular could be a regression risk if the API still returns 5. NavigationGraph Component -- Self-Contained and Clean
Minor note: The SVG uses CSS custom properties ( One concern: 6. No Test CoverageNo test files were added or modified. The Summary
No blocking issues. The IconRailPanel migration is the right move for consistency across the 5+ workspace views. |
Add @verified docstrings to EmbeddingLandscapeWorkspace, NavigationGraph, and HomeWorkspace per project docstring policy.
Summary
EmbeddingLandscapeWorkspacewith the sharedIconRailPanelcomponentTest plan