feat(graph+ux): auto-commit edges, mobile fixes, responsive navbar#83
Merged
feat(graph+ux): auto-commit edges, mobile fixes, responsive navbar#83
Conversation
Graph exploration: clicking a stub to add a new node now auto-discovers and commits all edges between that node and existing committed nodes. Batch commit/undo so the whole set is a single undo action. Mobile graph: fix race condition where zoomToFit ran at default 800x600 before ResizeObserver corrected to actual mobile dimensions. Responsive force parameters (tighter layout on small screens). Navbar: responsive mobile menu with three-dot toggle — nav links were scrolling off-screen on mobile. Single UserMenu instance for Playwright compatibility. UserMenu: SSR loading state now renders a clickable "Sign In" link instead of an invisible empty div. ImageUpload: remove confusing Upload/URL mode toggle — drop zone is always visible, with a small "or paste image URL" link below. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ghost
previously approved these changes
Mar 3, 2026
ghost
left a comment
There was a problem hiding this comment.
Code Review - PR #83
Summary: Adds auto-discovery of edges when expanding the graph, fixes mobile graph rendering, and improves mobile navbar/upload UX.
Changes reviewed:
- Graph exploration: auto-commit all edges between a newly added node and existing committed nodes (batch undo/redo)
- Mobile graph fix: re-zoom after ResizeObserver corrects dimensions, responsive force parameters
- Responsive navbar: three-dot mobile menu replaces scrolling nav links
- UserMenu SSR: loading state renders clickable "Sign In" link instead of invisible div
- ImageUpload: simplified UX — always-visible drop zone with "or paste URL" fallback
Quality Assessment:
| Aspect | Status | Notes |
|---|---|---|
| Code Quality | pass | Clean, well-structured changes with good comments |
| Tests | pass | 24/24 tests pass (subgraph + auth), TypeScript clean |
| Security | pass | No new auth surfaces or user input handling |
| Performance | pass | Edge auto-discovery is O(links) which is acceptable for this graph size |
Issues found:
GraphView.tsx:226: The dependency[dimensions.width < 600]is a computed boolean expression in a dependency array. React will compare the boolean result which works correctly, but the eslint-disable comment papers over a legitimate warning. Use a derivedisMobilestate variable oruseMemoinstead of putting a comparison expression directly in the deps array.
Suggestions:
GraphView.tsx:207: The magic-number checkprev.width === 800 && prev.height === 600to detect "initial default dimensions" is brittle — if the default ever changes, this silently breaks. Consider using a ref boolean likehasReceivedRealDimensionsinstead.Navbar.tsx: The mobile dropdown doesn't close when the user taps outside of it (unlike theUserMenuwhich has an outside-click listener). On mobile this could be annoying — consider adding a click-outside handler or a semi-transparent backdrop that closes the menu.
Verdict: APPROVE
Core logic is solid — the batch commit/undo pattern in useSubgraphExploration is well-designed and the edge auto-discovery in handlePointerDown correctly handles the full graph scan. The mobile fixes directly address real usability issues. The suggestions above are minor polish items.
- Replace computed boolean in deps array with derived `isMobile` variable - Replace magic-number default check with `hasRealDimensionsRef` boolean - Add click-outside handler to close mobile navbar dropdown Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ghost
approved these changes
Mar 3, 2026
ghost
left a comment
There was a problem hiding this comment.
Re-review - PR #83 (round 2)
All three items from round 1 have been addressed:
- Fixed:
dimensions.width < 600in dependency array → derivedisMobilevariable at component top level, used inuseEffectdeps - Fixed: Magic-number
800x600check →hasRealDimensionsRefboolean that flips once ResizeObserver fires - Fixed: Mobile navbar click-outside →
useEffectwithmousedownlistener onnavRef, only active whenmobileOpenis true
No new issues or suggestions. Clean code.
Verdict: APPROVE
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
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
zoomToFitran at default 800×600 before ResizeObserver corrected to actual mobile dimensions; responsive force parameters (tighter layout on small screens)Test plan
🤖 Generated with Claude Code