Code review outline panel implementation
Here's my thorough code review:
Code Review: Outline Panel Implementation
π΄ Critical Bugs
1. Clicking outline nodes does nothing useful β DockContent click handler (mouse.rs:380-392) only calls FocusDock, never dispatches OutlineMsg::ToggleNode or JumpToSymbol. No sub-item hit-testing exists for outline rows.
2. Scrolling is completely broken β MouseWheel for HoverRegion::Dock(_) (app.rs:807-811) consumes the event with None β never routes to OutlineMsg::Scroll.
3. Scroll offset has no upper bound β OutlineMsg::Scroll handler (outline.rs:193-201) freely increments scroll_offset with no max clamp; you can scroll past the end.
4. UTF-8 panic in truncation β render_outline_node does &node.name[..max_chars.saturating_sub(1)] which slices by bytes, not chars. Will panic on non-ASCII symbol names (e.g. rΓ©sumΓ©, CJK identifiers).
π‘ Functional Gaps
5. No double-click β jump-to-symbol β DockContent doesn't differentiate click count; even once click handling is added, double-click needs explicit routing.
6. Stale state on document switch β OutlinePanelState is global. When switching tabs, scroll_offset and selected_index are never reset, pointing at stale positions.
7. Collapse key collision β OutlinePanelState keys use (label, name, line) but Function and Method both have label "fn", so a function and method on the same line with the same name would share collapse state. Should use OutlineKind directly (derive Hash).
8. Per-frame allocations in is_collapsed β node_key() clones node.name (allocating a String) on every call. This runs for every visible node every frame.
9. No YAML outline β LanguageId::Yaml falls through to Vec::new() despite tree-sitter-yaml being available.
π’ Cosmetic / Quality
10. Markdown # in names β The parse_heading primary path correctly extracts inline/heading_content which shouldn't include # markers. The fallback path also strips #. Should be fine, but the OutlineKind::Heading label renders #/## prefixes before the name β so the user sees # Title which is redundant. Consider using a heading icon like H1/H2 or just the name.
11. render_outline_node has 17 parameters β Hard to maintain. A SidebarRenderContext-like struct already exists for the sidebar and should be replicated here.
Summary of Fixes Needed
| Issue | Severity | Fix |
|---|---|---|
| Click handling | π΄ | Add row hit-testing in DockContent handler, dispatch OutlineMsg |
| Scroll routing | π΄ | Route HoverRegion::Dock scroll to OutlineMsg::Scroll when active panel is Outline |
| Scroll clamp | π΄ | Clamp scroll_offset to max(0, total_items - visible_capacity) |
| UTF-8 truncation | π΄ | Use .chars().take(n) instead of byte slicing |
| Double-click | π‘ | Pass click count, jump on double-click |
| Stale state reset | π‘ | Reset scroll_offset/selected_index on tab/doc switch |
| Collapse key | π‘ | Derive Hash on OutlineKind, use (OutlineKind, OutlineRange) |
| Per-frame allocs | π‘ | Follows from collapse key fix |
| YAML support | π‘ | Add YAML key extractor |
| Heading labels | π’ | Change heading label from # to H1/H2/etc. |
| 17-param function | π’ | Extract OutlineRenderContext struct |
Want me to start fixing these issues? I'd suggest tackling them in priority order: click handling β scroll routing/clamp β UTF-8 fix β double-click β state reset β collapse key β render context β YAML β heading labels.
All clean β no warnings, no errors. Here's a summary of everything fixed:
Changes Made
| File | Fix |
|---|---|
src/outline/mod.rs |
Added Hash to OutlineKind + OutlineRange; changed heading labels from #/## β H1/H2 |
src/outline/extract.rs |
Added YAML outline extractor (block_mapping_pair/flow_pair β nested Property tree) |
src/model/ui.rs |
Changed collapse key from (&'static str, String, usize) to (OutlineKind, OutlineRange) β fixes label collisions + per-frame allocations |
src/messages.rs |
Added OutlineMsg::ClickRow { index, click_count, on_chevron } |
src/update/outline.rs |
Implemented ClickRow handler (select + chevron toggle + double-click jump); added scroll bounds clamping; clamped after expand/collapse |
src/update/layout.rs |
Reset scroll_offset/selected_index on tab switch and group focus |
src/runtime/mouse.rs |
DockContent click now does row hit-testing for outline, dispatches ClickRow with chevron detection |
src/runtime/app.rs |
Dock scroll events now route to OutlineMsg::Scroll when outline is active |
src/view/mod.rs |
Fixed UTF-8 truncation panic (byteβchar slicing); extracted OutlineRenderContext struct (17 params β 7) |
Confirmed β the only lint error is a pre-existing issue in recent_files.rs, not from our changes. Our code passes cleanly.
Root cause found and fixed: hit_test_ui() had no dock hit-testing at all. Clicks on the right dock area fell through to hit_test_groups(), which matched them as EditorContent since editor group rects extend under the dock in coordinate space.
Fix: Added hit_test_docks() function that checks right and bottom dock rectangles, and inserted it into hit_test_ui() at priority 5 β after sidebar but before splitters/editor groups. Dock clicks now correctly produce HitTarget::DockContent with the active panel ID, which the mouse handler then routes to OutlineMsg::ClickRow.
Done. Moved docs/feature/code-outline.md β docs/archived/code-outline.md and created docs/feature/tree-navigation-abstraction.md covering the arrow key gap and the broader tree navigation abstraction plan (Phase 1: wire keys, Phase 2: extract shared state, Phase 3: trait, Phase 4: unified renderer).
Hmm, many files I expected to see modified aren't showing. Let me check if there were prior unstaged changes:
The earlier changes (model, messages, extract, update, view, runtime) were likely made on top of uncommitted work from the previous thread. Let me check which files have our changes:
Those files show no diff against HEAD β meaning the prior thread already had them in the working tree but they were committed or they're somehow already matching. Let me check:
The previous thread's changes plus our Phase 1/2 task agent changes are already committed (the agents likely ran git add/git commit or the files were part of a dirty tree that got auto-committed). Only the hit-test fix, the outline module as new files, the doc archive, and the new feature doc need committing. Let me verify:
Committed as b380bee. 6 files changed β the outline module, dock hit-testing fix, and docs.