fix: Multiple note creation and conditional navigation UI (Issue #6) #8
Conversation
…ons for better user experience
…e functionality fixes and UI enhancements
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis pull request implements multi-note support for the Code Context Notes VSCode extension, enabling multiple notes on a single line. Changes include backend query methods for retrieving notes by position, thread state management in the comment controller, updated CodeLens to show per-line aggregated notes, navigation commands, and comprehensive architecture documentation of the current system. Changes
Sequence DiagramsequenceDiagram
participant User
participant CodeLens
participant CommentController
participant NoteManager
participant threadStates as Thread State Map
rect rgb(200, 220, 250)
note over User,NoteManager: Adding a Note to Line with Existing Notes
User->>CodeLens: Click "➕ Add Note" on line 42
CodeLens->>CommentController: openCommentEditor(line 42)
CommentController->>NoteManager: getNotesAtPosition(filePath, 42)
NoteManager-->>CommentController: [note1, note2]
CommentController->>threadStates: Update threadStates["file.ts:42"]
threadStates-->>CommentController: threadKey
CommentController->>User: Open editor for new note
end
rect rgb(220, 250, 200)
note over User,threadStates: Navigating Between Notes in Thread
User->>CommentController: navigateNextNote(threadKey)
CommentController->>threadStates: Get current thread state
threadStates-->>CommentController: MultiNoteThreadState(currentIndex=0)
CommentController->>threadStates: Increment currentIndex to 1
CommentController->>CommentController: updateThreadDisplay(threadKey)
CommentController->>User: Show note 2 in thread
end
rect rgb(250, 220, 200)
note over User,threadStates: Loading Document with Multiple Notes Per Line
User->>CommentController: loadCommentsForDocument()
CommentController->>NoteManager: getAllNotes(filePath)
NoteManager-->>CommentController: [note1, note2, note3, ...]
CommentController->>CommentController: Group notes by line
loop For each line with notes
CommentController->>threadStates: Create threadKey and state
CommentController->>User: Create single thread per line
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes The changes span multiple architectural layers (backend queries, data models, thread state management, UI components, and commands) with heterogeneous modifications. The CommentController refactor introduces significant state management logic and async/await patterns. Multi-note thread state tracking and contextValue normalization require careful verification against edge cases. Documentation is extensive but adds review overhead. Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
5-5: Version consistency.Manifest is
0.1.7, while the PR text mentions bump tov0.1.8. Align before release (and update in-code log if kept).
🧹 Nitpick comments (4)
docs/CURRENT-ARCHITECTURE-ANALYSIS.md (1)
1-676: Excellent comprehensive architecture documentation.This is high-quality documentation that thoroughly covers the extension's architecture, including storage, data model, UI components, commands, and current limitations. The detailed analysis provides an excellent foundation for understanding the codebase and planning the multi-note feature implementation.
As an optional improvement, consider adding language specifiers to the fenced code blocks at lines 31, 96, 102, 268, 292, 492, 513, 545, and 572 for better syntax highlighting and rendering (e.g.,
```typescriptor```text).src/extension.ts (1)
350-357: DRY up noteId extraction fromcontextValue.Same
:multi-suffix stripping is repeated. Extract a tiny helper and reuse.Apply this localized change in each spot:
- const noteId = contextValue.replace(/:multi$/, ''); + const noteId = extractNoteId(contextValue);Add this helper once (outside changed ranges, e.g., near the top of the file):
function extractNoteId(contextValue?: string): string { return (contextValue ?? '').replace(/:multi$/, ''); }Also applies to: 381-388, 538-545, 586-593
src/codeLensProvider.ts (1)
144-174: Minor: simplify preview derivation.
stripMarkdownalready collapses whitespace including newlines;split('\n')[0]is redundant. Not urgent.src/commentController.ts (1)
64-119: Multi-note thread creation looks good overall.The refactor to support multiple notes per line is well-structured. Thread reuse by line position, state initialization, and display updates are all correct.
Minor edge case: Line 102 uses
findIndexto setcurrentIndex. If the note isn't found innotesAtPosition, this would be -1. While unlikely (the note should exist at that position), consider adding validation:const noteIndex = notesAtPosition.findIndex(n => n.id === note.id); if (noteIndex === -1) { throw new Error(`Note ${note.id} not found at position ${note.lineRange.start}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
ANALYSIS_COMPLETE.md(1 hunks)ANALYSIS_INDEX.md(1 hunks)ARCHITECTURE_ANALYSIS_SUMMARY.txt(1 hunks)CHANGELOG.md(1 hunks)CLAUDE.md(1 hunks)COMPONENT_INTERACTION_MAP.md(1 hunks)docs/CURRENT-ARCHITECTURE-ANALYSIS.md(1 hunks)docs/MULTI-NOTE-IMPLEMENTATION-PLAN.md(1 hunks)docs/TODO.md(1 hunks)package.json(2 hunks)src/codeLensProvider.ts(3 hunks)src/commentController.ts(12 hunks)src/extension.ts(6 hunks)src/noteManager.ts(1 hunks)src/types.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
docs/TODO.md
📄 CodeRabbit inference engine (CLAUDE.md)
Always update docs/TODO.md after making any changes
Files:
docs/TODO.md
🧠 Learnings (1)
📚 Learning: 2025-10-17T19:18:50.173Z
Learnt from: CR
PR: jnahian/code-context-notes#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-17T19:18:50.173Z
Learning: Applies to docs/TODO.md : Always update docs/TODO.md after making any changes
Applied to files:
CLAUDE.md
🧬 Code graph analysis (3)
src/noteManager.ts (1)
src/types.ts (2)
Note(37-58)LineRange(8-13)
src/codeLensProvider.ts (1)
src/types.ts (1)
Note(37-58)
src/commentController.ts (1)
src/types.ts (2)
MultiNoteThreadState(153-162)Note(37-58)
🪛 LanguageTool
docs/CURRENT-ARCHITECTURE-ANALYSIS.md
[uncategorized] ~396-~396: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...les - Converts between Note objects and markdown format - Handles soft deletes - All not...
(MARKDOWN_NNP)
[style] ~673-~673: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...tiple threads per line 5. Commands: Need to handle ambiguity when multiple notes at...
(REP_NEED_TO_VB)
ARCHITECTURE_ANALYSIS_SUMMARY.txt
[style] ~7-~7: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...RSION: 0.1.7 ANALYSIS DATE: October 19, 2025 FOCUS: Understanding current architectu...
(MISSING_COMMA_AFTER_YEAR)
[uncategorized] ~97-~97: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...Delete, View History - Supports: Full markdown formatting, multi-line text Current UI...
(MARKDOWN_NNP)
COMPONENT_INTERACTION_MAP.md
[style] ~380-~380: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...culation per window - Can be slow on very large files 3. **NoteManager cache invalidat...
(EN_WEAK_ADJECTIVE)
docs/MULTI-NOTE-IMPLEMENTATION-PLAN.md
[uncategorized] ~327-~327: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...e content (>1000 chars) - [ ] Test with markdown formatting in multiple notes - [ ] Test...
(MARKDOWN_NNP)
[uncategorized] ~456-~456: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ar UI indicators and documentation ### High Risk - None identified ## Future Enhancemen...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
ANALYSIS_INDEX.md
[style] ~11-~11: Consider a different adjective to strengthen your wording.
Context: ... Length: 500+ lines - Best For: Deep understanding, migration planning - **C...
(DEEP_PROFOUND)
[style] ~318-~318: Consider a different adjective to strengthen your wording.
Context: ... | | CURRENT-ARCHITECTURE-ANALYSIS.md | Deep understanding | 10+ pages | 45-60 min |...
(DEEP_PROFOUND)
🪛 markdownlint-cli2 (0.18.1)
docs/CURRENT-ARCHITECTURE-ANALYSIS.md
31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
96-96: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
102-102: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
268-268: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
292-292: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
492-492: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
513-513: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
545-545: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
572-572: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
COMPONENT_INTERACTION_MAP.md
5-5: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
108-108: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
209-209: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
254-254: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
290-290: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
327-327: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/MULTI-NOTE-IMPLEMENTATION-PLAN.md
82-82: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
87-87: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
111-111: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
211-211: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
228-228: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
ANALYSIS_INDEX.md
159-159: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
204-204: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
221-221: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
273-273: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
286-286: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/TODO.md
1647-1647: Bare URL used
(MD034, no-bare-urls)
1660-1660: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1665-1665: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1835-1835: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1841-1841: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1893-1893: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1905-1905: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1915-1915: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (24)
CLAUDE.md (1)
1-2: LGTM - Improved changelog management directive.The updated directive to consolidate multiple TODOs of the same feature/improvement/fix into single changelog items will result in cleaner, more readable release notes.
src/types.ts (1)
150-162: LGTM - Well-designed interface for multi-note threading.The
MultiNoteThreadStateinterface is cleanly designed with clear responsibilities:
noteIdsarray enables tracking multiple notes at a positioncurrentIndexsupports navigation between noteslineRangeandfilePathproperly scope the thread contextThe interface aligns well with the multi-note feature objectives outlined in issue #6.
COMPONENT_INTERACTION_MAP.md (1)
1-414: LGTM - Valuable component interaction documentation.The component interaction map provides clear visualization of how the extension's components work together, including data flows, responsibilities, and key interactions. The ASCII diagrams and flow sequences make the architecture easy to understand.
ANALYSIS_COMPLETE.md (1)
1-200: LGTM - Clear summary of analysis deliverables.This summary document effectively captures the scope and findings of the architecture analysis, making it easy to understand what was analyzed and what the key takeaways are.
CHANGELOG.md (1)
17-51: LGTM - Well-structured changelog entry for v0.1.8.The changelog entry clearly documents:
- Fixed multi-note creation and navigation issues
- UI improvements with conditional icon-only buttons
- Technical implementation details
The structure follows best practices with separate Fixed, Changed, and Technical sections, making it easy for users and developers to understand the changes.
docs/MULTI-NOTE-IMPLEMENTATION-PLAN.md (1)
1-483: LGTM - Comprehensive implementation plan for multi-note support.This implementation plan is thorough and well-structured, covering:
- Phased approach with clear goals for each phase
- UI strategy with rationale (sequential display with navigation)
- Migration and compatibility considerations (minimal changes needed)
- Testing strategy with specific test cases
- Timeline and success metrics
The plan demonstrates careful consideration of backward compatibility and user experience while enabling the multi-note feature.
ANALYSIS_INDEX.md (1)
1-327: LGTM - Helpful documentation navigation guide.The index provides valuable guidance for navigating the documentation set, including:
- Quick navigation to primary and supporting documents
- "Finding What You Need" section with specific topic pointers
- Recommended reading orders for different objectives
- Quick reference table with estimated read times
This makes the comprehensive documentation more accessible and user-friendly.
package.json (2)
140-156: New commands registered — looks good.The three commands (previousNote, nextNote, addNoteToLine) are clearly declared with icons and category. Implementation exists in src/extension.ts.
229-237: Menu when-clauses and button layout — OK, with one cross-file caveat.
- Regex gating for multi-note nav via
/:multi$/is appropriate; edit/history/delete remain available for both single and multi via the UUID prefix match. LGTM.- Cross-file: The “Add Note” title-button invokes
codeContextNotes.addNoteToLinewith avscode.Comment(provided by VS Code), but the CodeLens version of the same command passes a plain object{ filePath, lineStart }. Ensure the handler accepts both shapes (see proposed fix in src/extension.ts).Also applies to: 239-257
src/noteManager.ts (1)
335-371: Per-line and range APIs — solid and idiomatic.Filtering by inclusive bounds and overlap logic is correct; deleted notes are excluded via
getNotesForFile. LGTM.src/extension.ts (1)
739-744: Subscriptions updated — good.New command disposables are correctly pushed to
context.subscriptions.
Please re-run manual paths for:
- Comment title “Add Note” button.
- CodeLens “➕ Add Note” click.
- Multi-note previous/next.
src/codeLensProvider.ts (2)
47-78: CodeLens “➕ Add Note” arg shape — matches proposed handler.This passes
{ filePath, lineStart }, which is correct for a non-comment surface. Ensure the handler in src/extension.ts accepts this (see fix provided).
37-46: Per-line aggregation and titles — LGTM.Grouping by
lineRange.startand projecting a single view lens plus an add-note lens per line reads well and minimizes clutter.Also applies to: 61-69, 85-99
docs/TODO.md (1)
1329-1982: Documentation is comprehensive and well-structured.The documentation thoroughly covers the multi-note implementation, bug fixes, and UI enhancements. Technical details, testing checklists, and visual diagrams are all helpful for understanding the changes.
src/commentController.ts (10)
8-8: LGTM: Multi-note infrastructure properly initialized.The addition of
MultiNoteThreadStateimport and the newthreadStatesmap correctly establishes the foundation for multi-note support. The change from note-ID-based to thread-key-based storage is appropriate for grouping notes by line.Also applies to: 18-19, 26-26
57-59: Thread key format is simple and effective.The
getThreadKeymethod using${filePath}:${lineStart}is straightforward. While file paths can contain colons (e.g., Windows drive letters), the numeric line number makes parsing unambiguous. This is acceptable for internal use.
156-190: LGTM: Conditional button display using contextValue.The addition of the
isMultiNoteparameter and the:multisuffix incontextValueis a clean approach to enable conditional button display in VS Code's comment UI. This works well with thewhenclauses in package.json.
196-238: Navigation methods implemented correctly.The
navigateNextNoteandnavigatePreviousNotemethods properly implement wrap-around navigation with modulo arithmetic. ThegetCurrentNoteIdgetter is straightforward and correct. Early returns for single-note threads are appropriate.
243-264: LGTM: Thread update properly migrated to thread-key-based lookup.The
updateCommentThreadmethod correctly uses thread keys for lookup and properly handles both updating existing threads and creating new ones. The async changes are appropriate for callingupdateThreadDisplay.
269-310: LGTM: Multi-note thread deletion properly implemented.The
deleteCommentThreadmethod correctly handles both removing individual notes from multi-note threads and disposing threads when the last note is removed. Index adjustment logic ensurescurrentIndexstays within bounds.
315-375: LGTM: Document loading correctly groups notes by line.The refactor to group notes by line position and create one thread per line is appropriate for multi-note support. The cleanup of both
commentThreadsandthreadStatesmaps is correct.
383-408: LGTM: closeAllCommentEditors updated for thread-key architecture.The parameter change from
exceptNoteIdtoexceptThreadKeyand the cleanup of both maps are appropriate for the multi-note implementation. Conditional editing state clearing is correct.
605-642: LGTM: Focus, history, and edit methods properly migrated to thread keys.The three methods (
focusNoteThread,showHistoryInThread,enableEditMode) all follow a consistent pattern:
- Get note to determine line range
- Calculate thread key
- Find or create thread
- Update currentIndex if multi-note thread
- Perform the specific action
This approach correctly handles both single and multi-note threads.
Also applies to: 647-700, 705-752
798-839: LGTM: Save edited note correctly searches through threads.The
saveEditedNoteByIdmethod appropriately searches through all threads to find the note, then usesupdateThreadDisplayto refresh the multi-note display. Error handling for missing threads is correct.
…ailed instructions and UI updates
Description
This PR fixes the multiple note creation functionality that was broken due to incomplete migration to the multi-note system, and enhances the UI by moving navigation controls from markdown to conditional icon-only buttons.
Type of Change
Related Issues
Fixes #6
Changes Made
Fixed: Multiple Note Creation & Navigation (Issue #6)
Problem: Thread lookup methods were using note IDs instead of thread keys, breaking multi-note functionality
focusNoteThread(),showHistoryInThread(),enableEditMode(), andsaveEditedNoteById()to use thread keys (filePath:lineStart)Changed: Conditional Navigation Buttons with Icon-Only UI
UI Enhancement: Moved navigation from markdown header to native VS Code icon buttons
$(chevron-left)) and Next ($(chevron-right)) buttons that only appear when there are multiple notes on the same line[+] [Edit] [History] [Delete][<] [>] [+] [Edit] [History] [Delete]noteId, multi-notes usenoteId:multisuffix for conditional button displayTechnical Changes
src/commentController.ts:
createComment()to addisMultiNoteparameter and set contextValue conditionally (lines 156-190)updateThreadDisplay()to calculate and pass multi-note state (lines 124-151)closeAllCommentEditors()to handle thread keys properlysrc/extension.ts:
.replace(/:multi$/, '')nextNote,previousNote,addNoteToLine,editNote,saveNote,deleteNoteFromComment,viewNoteHistorypackage.json:
whenclauses for buttonswhen: comment =~ /:multi$/(only for multi-note threads)when: comment =~ /^[a-f0-9-]+/(for all notes)src/codeLensProvider.ts:
Testing
Test Coverage:
Manual Testing Steps:
Screenshots (if applicable)
Before (Markdown Header):
```
┌─────────────────────────────────────────────┐
│ Note 1 of 3 │
│ │
│ [< Previous] | [Next >] | [+ Add Note] │
│ ─────────────────────────────────────────── │
│ │
│ This is the actual note content... │
└─────────────────────────────────────────────┘
```
After (Icon Buttons - Single Note):
```
┌─────────────────────────────────────────────┐
│ │
│ [+] [Edit] [History] [Del] │
│ ─────────────────────────────────────────── │
│ This is the actual note content... │
└─────────────────────────────────────────────┘
```
After (Icon Buttons - Multiple Notes):
```
┌─────────────────────────────────────────────┐
│ Note 1 of 3 │
│ [<] [>] [+] [Edit] [History] [Del] │
│ ─────────────────────────────────────────── │
│ This is the actual note content... │
└─────────────────────────────────────────────┘
```
Checklist
Documentation
Breaking Changes
No breaking changes. This is fully backward compatible.
Performance Impact
Positive performance impact:
Additional Notes
Benefits:
Version: v0.1.8 (Patch release)
Type: Bug fixes + UI enhancement
Priority: High (broken multi-note feature)
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
UI/UX Improvements