Skip to content

Commit a87e78a

Browse files
andresthorandresthor
committed
feat(integrations): add visual mode multi-file selection for snacks.explorer
Enable selecting multiple files in snacks.explorer using vim's visual mode. This brings the snacks.explorer integration up to par with others, such as oil, nvim-tree and NvimTree - Pass visual range parameters to _get_snacks_explorer_selection() - Use picker's list API to convert row numbers to items (row2idx/get) - Handle edge cases like nil items and empty file paths - Add comprehensive test coverage for visual selection scenarios
1 parent 840c147 commit a87e78a

File tree

7 files changed

+571
-16
lines changed

7 files changed

+571
-16
lines changed

‎00-final-peer-pr-review-1.md

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
# PR Review: Add snacks.explorer Support
2+
3+
## Summary
4+
5+
This PR adds support for snacks.explorer file explorer integration to claudecode.nvim, bringing it to feature parity with existing file explorer integrations (nvim-tree, neo-tree, oil.nvim).
6+
7+
## Overall Assessment
8+
9+
The implementation is **high quality** and follows the project's established patterns well. The code is clean, well-documented, and includes comprehensive test coverage. However, there is one **critical issue** that must be addressed before merging.
10+
11+
## Positive Aspects
12+
13+
### 1. **Follows Established Patterns**
14+
15+
- The implementation correctly follows the existing integration pattern used for other file explorers
16+
- Uses the same function signatures and return patterns as existing integrations
17+
- Properly handles error cases with meaningful error messages
18+
19+
### 2. **Comprehensive Implementation**
20+
21+
- Supports single file selection
22+
- Supports visual mode multi-file selection
23+
- Implements proper fallback behavior (selected → current)
24+
- Handles edge cases like empty file paths and nil items
25+
26+
### 3. **Excellent Test Coverage**
27+
28+
- 10+ test cases covering all major scenarios
29+
- Tests for error conditions (module not found, no active explorer)
30+
- Tests for visual mode selection with edge cases
31+
- Mock-based testing approach consistent with the project
32+
33+
### 4. **Clean Code Structure**
34+
35+
- Well-documented functions with proper JSDoc comments
36+
- Clear variable names and logical flow
37+
- Proper error handling with pcall for module loading
38+
39+
### 5. **Resilient File Path Detection**
40+
41+
- Tries multiple fields (file, path, item.file, item.path) to find file paths
42+
- Handles different data structures that snacks.explorer might use
43+
- Filters out empty or nil file paths appropriately
44+
45+
## Critical Issue
46+
47+
### **Accidental snacks.nvim Repository Inclusion**
48+
49+
The branch contains an entire copy of the snacks.nvim repository in the project root:
50+
51+
```
52+
Untracked files:
53+
snacks.nvim/
54+
```
55+
56+
This appears to be accidentally added and **must be removed** before merging. This adds unnecessary bloat to the repository and could cause confusion.
57+
58+
**Fix**:
59+
60+
```bash
61+
rm -rf snacks.nvim/
62+
```
63+
64+
## Minor Suggestions
65+
66+
### 1. **Consider Adding Integration Documentation**
67+
68+
While the README.md was updated to mention snacks.explorer support, it might be helpful to add a note in ARCHITECTURE.md or DEVELOPMENT.md about the new integration, similar to how other integrations are documented.
69+
70+
### 2. **Potential Performance Consideration**
71+
72+
In the visual mode selection handling, the code iterates through each row and makes individual API calls:
73+
74+
```lua
75+
for row = visual_start, visual_end do
76+
local idx = explorer.list:row2idx(row)
77+
-- ...
78+
end
79+
```
80+
81+
For large selections, this could potentially be optimized if snacks.explorer provides a batch API. However, this is not a blocking issue and the current implementation is acceptable.
82+
83+
### 3. **Type Annotations**
84+
85+
Consider adding more specific type annotations for the snacks.explorer API objects in the comments, if available. This would help future maintainers understand the expected data structures.
86+
87+
## Code Quality Checklist
88+
89+
- ✅ Passes luacheck with 0 warnings/errors
90+
- ✅ Follows project coding standards
91+
- ✅ Comprehensive test coverage
92+
- ✅ Proper error handling
93+
- ✅ No breaking changes to existing functionality
94+
- ✅ Documentation updated
95+
- ❌ Clean working tree (contains untracked snacks.nvim directory)
96+
97+
## Recommendation
98+
99+
**Approve with changes required**: The implementation is excellent and ready to merge once the snacks.nvim directory is removed from the repository. This is a valuable addition that brings support for what has become LazyVim's default file explorer.
100+
101+
## Testing Recommendations
102+
103+
Before merging, please verify:
104+
105+
1. Remove the snacks.nvim directory
106+
2. Run `make` to ensure all tests pass
107+
3. Manually test with a real snacks.explorer instance
108+
4. Test the edge cases (empty explorer, no files, visual selection)
109+
110+
## Commendation
111+
112+
The contributor has done an excellent job understanding the codebase and implementing this feature in a way that perfectly matches the existing patterns. The test coverage is particularly impressive, showing attention to detail and commitment to code quality.

‎00-final-peer-pr-review-2.md

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
# PR Review: Add snacks.explorer support to claudecode.nvim
2+
3+
## Summary
4+
5+
This PR adds support for the snacks.explorer file explorer to claudecode.nvim, bringing it to feature parity with existing file explorer integrations (nvim-tree, neo-tree, oil.nvim). The implementation is well-structured and follows the established patterns in the codebase.
6+
7+
## Strengths
8+
9+
### 1. Consistent Integration Pattern
10+
11+
The implementation follows the existing architecture perfectly:
12+
13+
- Uses the same integration module pattern as other file explorers
14+
- Properly detects `snacks_picker_list` filetype across all relevant modules
15+
- Implements both single file and multi-file selection modes
16+
17+
### 2. Improved Architecture for Visual Selection
18+
19+
Unlike other integrations that handle visual selection in `visual_commands.lua`, this implementation cleanly delegates visual range handling to the integration module itself. This is actually a better architectural pattern that:
20+
21+
- Keeps the logic more modular and testable
22+
- Reduces coupling between modules
23+
- Makes the visual selection logic easier to understand and maintain
24+
25+
### 3. Comprehensive Test Coverage
26+
27+
Excellent test coverage in `tests/unit/snacks_explorer_spec.lua`:
28+
29+
- Tests for module availability
30+
- Error handling scenarios
31+
- Single and multi-file selection
32+
- Visual mode selection with edge cases
33+
- Alternative field name handling (file, path, item.file, item.path)
34+
35+
### 4. Proper API Usage
36+
37+
The implementation correctly uses the snacks.nvim picker API:
38+
39+
- Uses `picker.get({ source = "explorer" })` to find active explorers
40+
- Leverages both `selected()` and `current()` methods with appropriate fallbacks
41+
- Handles visual mode using the list API (`row2idx`, `get`)
42+
43+
## Areas for Improvement
44+
45+
### 1. **Error Handling Enhancement**
46+
47+
The current implementation silently skips invalid items. Consider adding debug logging:
48+
49+
```lua
50+
-- In _get_snacks_explorer_selection()
51+
if not file_path or file_path == "" then
52+
require("claudecode.logger").debug("Skipping item with empty/nil path")
53+
end
54+
```
55+
56+
### 2. **Performance Consideration**
57+
58+
For large visual selections, the current implementation processes each row individually. While this is fine for typical use, consider batching if performance becomes an issue with very large selections.
59+
60+
### 3. **Documentation Comments**
61+
62+
The function documentation is good, but could be enhanced:
63+
64+
```lua
65+
--- Get selected files from snacks.explorer
66+
--- Uses the picker API to get the current selection
67+
--- @param visual_start number|nil Start line of visual selection (1-indexed)
68+
--- @param visual_end number|nil End line of visual selection (1-indexed)
69+
--- @return table files List of absolute file paths
70+
--- @return string|nil error Error message if operation failed
71+
```
72+
73+
### 4. **Untracked File Issue**
74+
75+
There's an untracked `snacks.nvim` directory in the repository that should be removed and added to `.gitignore` if it's a local development dependency.
76+
77+
## Minor Issues
78+
79+
### 1. **Inconsistent Naming**
80+
81+
The code uses both "snacks.explorer" and "snacks-explorer" in different places:
82+
83+
- Comments refer to "snacks.explorer"
84+
- Tree type returns "snacks-explorer"
85+
86+
While this doesn't affect functionality, standardizing on one format would improve consistency.
87+
88+
### 2. **Missing Edge Case**
89+
90+
The visual mode selection doesn't handle the case where `explorer.list` is nil but visual range is provided. Consider adding:
91+
92+
```lua
93+
if visual_start and visual_end then
94+
if not explorer.list then
95+
require("claudecode.logger").debug("Visual selection requested but explorer has no list API")
96+
-- Fall through to other selection methods
97+
else
98+
-- existing code
99+
end
100+
end
101+
```
102+
103+
## Recommendations
104+
105+
1. **Remove the untracked `snacks.nvim` directory** before merging
106+
2. **Run `make` to ensure all tests pass** - The PR author should confirm this passes in their environment
107+
3. **Consider adding the debug logging** for better troubleshooting
108+
4. **Update the plugin documentation** to mention snacks.explorer support in more places (not just README examples)
109+
110+
## Verdict
111+
112+
This is a **high-quality PR** that adds valuable functionality to the plugin. The code is clean, well-tested, and follows the established patterns. With the minor improvements suggested above (especially removing the untracked directory), this PR is ready for merge.
113+
114+
The contributor has done an excellent job understanding the codebase architecture and implementing the feature in a way that's consistent with existing code. The comprehensive test coverage is particularly commendable.

‎00-final-peer-pr-review-3.md

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
# PR Review: Add Visual Mode Multi-file Selection for snacks.explorer
2+
3+
## Summary
4+
5+
This PR adds visual mode multi-file selection support for snacks.explorer, bringing it to feature parity with other supported file explorers (oil.nvim, nvim-tree, neo-tree). The implementation is clean, follows existing patterns, and includes comprehensive test coverage.
6+
7+
## Strengths
8+
9+
### 1. **Consistent Implementation Pattern**
10+
11+
The implementation follows the established pattern used by other file explorers in the codebase. The author properly:
12+
13+
- Added optional parameters to `_get_snacks_explorer_selection(visual_start, visual_end)`
14+
- Used the same visual range handling approach as oil.nvim
15+
- Maintained backwards compatibility by keeping parameters optional
16+
17+
### 2. **Comprehensive Test Coverage**
18+
19+
The test suite is thorough and covers:
20+
21+
- Basic visual mode selection
22+
- Edge cases (nil items, empty paths, missing indices)
23+
- Different item structure variations (file vs path fields)
24+
- Proper filtering of invalid entries
25+
26+
### 3. **Proper Error Handling**
27+
28+
The implementation correctly handles edge cases:
29+
30+
- Checks for nil returns from `row2idx()` and `get()`
31+
- Filters out empty file paths
32+
- Falls back gracefully when visual mode isn't active
33+
34+
### 4. **Documentation Updates**
35+
36+
The author remembered to update both README.md and dev-config.lua to mention snacks.explorer support.
37+
38+
## Areas for Improvement
39+
40+
### 1. **File Type Consistency**
41+
42+
In the keybinding configuration, the filetype is listed as `"snacks_picker_list"`:
43+
44+
```lua
45+
ft = { "NvimTree", "neo-tree", "oil", "snacks_picker_list" },
46+
```
47+
48+
While this is correct, it might be worth adding a comment explaining why this filetype differs from the others (which use their plugin names directly). This would help future contributors understand the naming convention.
49+
50+
### 2. **Depth/Root Protection Missing**
51+
52+
Other file explorer implementations have protection against selecting root-level files:
53+
54+
- nvim-tree: `if not string.match(mark.absolute_path, "^/[^/]*$") then`
55+
- neo-tree: `if depth > 1 then`
56+
57+
The snacks.explorer implementation doesn't have similar protection. While this might be intentional (snacks.explorer might handle this internally), it's worth considering for consistency.
58+
59+
### 3. **Directory Handling**
60+
61+
The oil.nvim implementation has special handling for directories (ensures they end with `/`):
62+
63+
```lua
64+
elseif entry.type == "directory" then
65+
table.insert(files, full_path:match("/$") and full_path or full_path .. "/")
66+
```
67+
68+
The snacks.explorer implementation doesn't distinguish between files and directories. This might be fine if snacks.explorer handles this internally, but it's worth verifying.
69+
70+
### 4. **Code Duplication**
71+
72+
There's some duplication in how file paths are extracted:
73+
74+
```lua
75+
local file_path = item.file or item.path or (item.item and item.item.file) or (item.item and item.item.path)
76+
```
77+
78+
This pattern appears three times in the function. Consider extracting it to a local helper function:
79+
80+
```lua
81+
local function extract_file_path(item)
82+
return item.file or item.path or (item.item and item.item.file) or (item.item and item.item.path)
83+
end
84+
```
85+
86+
## Minor Issues
87+
88+
### 1. **Untracked Test File**
89+
90+
The git status shows `tests/unit/snacks_explorer_spec.lua` as untracked (`??`). This file should be added to the commit since it contains the tests for this feature.
91+
92+
### 2. **Snacks.nvim Directory**
93+
94+
There's an entire `snacks.nvim` directory in the project root that appears to be untracked. This looks like a cloned repository and should probably be added to `.gitignore` if it's being used for local testing.
95+
96+
## Code Quality
97+
98+
The code quality is excellent:
99+
100+
- Follows Lua idioms and project conventions
101+
- Proper use of pcall for error handling
102+
- Clear variable names and good code organization
103+
- Comprehensive documentation in comments
104+
105+
## Testing Recommendations
106+
107+
Before merging, please verify:
108+
109+
1. The visual selection works correctly with actual snacks.explorer usage (not just unit tests)
110+
2. Directory selection behaves as expected
111+
3. The feature works with both single and multiple file selections
112+
4. Performance with large file lists in visual mode
113+
114+
## Conclusion
115+
116+
This is a well-implemented feature that properly extends the existing functionality to support snacks.explorer. The code follows established patterns, includes good test coverage, and maintains backwards compatibility. With the minor improvements suggested above (particularly ensuring the test file is committed), this PR is ready for merge.
117+
118+
The implementation shows good understanding of both the codebase architecture and the snacks.explorer API. The author has done a commendable job of maintaining consistency with existing file explorer integrations while adapting to snacks.explorer's unique picker-based architecture.
119+
120+
## Recommended Actions
121+
122+
1. **Required**: Add `tests/unit/snacks_explorer_spec.lua` to the commit
123+
2. **Recommended**: Consider adding root/depth protection if applicable
124+
3. **Optional**: Extract the file path resolution logic to reduce duplication
125+
4. **Optional**: Add `.gitignore` entry for the snacks.nvim directory if it's for testing

‎dev-config.lua

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ return {
2323
"<leader>as",
2424
"<cmd>ClaudeCodeTreeAdd<cr>",
2525
desc = "Add file from tree",
26-
ft = { "NvimTree", "neo-tree", "oil" },
26+
ft = { "NvimTree", "neo-tree", "oil", "snacks_picker_list" }, -- snacks.explorer uses "snacks_picker_list" filetype
2727
},
2828

2929
-- Development helpers

0 commit comments

Comments
 (0)