Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds "context chips" — file/folder context for prompts — with config options, file-system readers, UI picker/previews, async processing of selected context, propagation into LLM request payloads, and rendering toggles across panes and clients. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PromptPane
participant FilePicker
participant MainView
participant FileSystem
participant LLMClient
User->>PromptPane: Type "@" → openContextPicker()
PromptPane->>FilePicker: openContextPicker(isContextMode=true, maxDepth)
User->>FilePicker: Select files/folders → return contextChips[]
User->>PromptPane: Send prompt (with contextChips)
PromptPane->>MainView: SendPromptReadyWithContextMsg(prompt, attachments, contextChips)
MainView->>MainView: makeProcessContextChipsCmd(prompt, attachments, chips, maxDepth)
MainView->>FileSystem: ReadFolderContents / ReadFileContent (respect maxDepth)
FileSystem-->>MainView: formatted contents, file list
MainView->>MainView: build ContextContent (FormatFolderContents)
MainView->>LLMClient: BuildRequest(prompt + ContextContent)
LLMClient-->>User: LLM response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
gugugiyu
force-pushed
the
feat/context-import-impl
branch
from
446bc86 to
2bd3a38
Compare
|
Rebased to remove the green theme. That was from another branch, oops. |
gugugiyu
force-pushed
the
feat/context-import-impl
branch
from
2bd3a38 to
f828162
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In `@components/context-picker.go`:
- Line 46: The FilterValue method currently returns zone.Mark(i.Path, i.Name),
which embeds invisible zone markers and can break list.Model filtering; change
ContextPickerItem.FilterValue to return a plain string (e.g., i.Name or i.Path +
"/" + i.Name) without using zone.Mark so that the list filter matches visible
text correctly, and keep zone.Mark usage only where rendering needs invisible
markers (e.g., in the display/render function).
- Around line 126-137: The key handler in the tea.KeyMsg case for the context
picker currently treats "q" and "esc" as quit unconditionally, causing those
keys to be consumed while the user is typing a filter; modify the handler in
components/context-picker.go (the switch on msg.String() inside the tea.KeyMsg
case) to first check the list's filtering state (e.g., via the list model's
filtering flag or IsFiltering/FilterState) and only interpret "q" and "esc" as
quit when not actively filtering; preserve the existing behavior for "enter" and
keep setting l.SelectedPath, l.quitting, and sending
util.SendViewModeChangedMsg(l.PrevView) when appropriate.
- Around line 151-161: The Update loop currently calls updatePreview on every
message causing repeated os.Stat/os.ReadFile; add a previewPath string field to
the ContextPicker struct and use it as a cache: in the Update method (around
where l.list.Update and GetSelectedItem are used) compare the selected item's
Path to l.previewPath and only call l.updatePreview(item.Path) if they differ;
when you successfully set l.preview (inside updatePreview or after reading the
file) also set l.previewPath = item.Path, and when selection is nil or a folder
clear both l.preview and l.previewPath to avoid stale caching.
- Around line 59-64: The delegate's Render currently always sets an icon
("📁"/"📄") instead of respecting ContextPicker.showIcons; fix it by adding a
showIcons bool to the delegate struct (or otherwise passing
ContextPicker.showIcons into the delegate when constructing it) and then
conditionalize the icon assignment inside the delegate.Render (the block that
sets icon := "" / if i.IsFolder { icon = "📁 " } else { icon = "📄 " }) so that
if showIcons is false the icon stays empty; update the delegate constructor call
from ContextPicker to pass showIcons and use that field in Render.
- Around line 163-193: In updatePreview: avoid reading the whole file by opening
the file (os.Open), using file.Stat only to enforce the 5MB size limit, then use
a bufio.Scanner to read up to 10 lines and join them into l.preview; before
scanning, peek at the first ~512 bytes (e.g., via bufio.Reader or ReadAt) and
reject binary files by checking for a null byte (0x00) and set l.preview = ""
with a debug log if found; ensure you close the file, handle and log any errors
via util.Slog.Error/Debug, and keep existing behavior for size checks and error
paths (referencing updatePreview, util.Slog, and maxPreviewSize).
In `@config/config-handler.go`:
- Around line 234-238: The code treats c.ContextMaxDepth == 0 as “unset” which
prevents users from legitimately setting depth 0; instead change ContextMaxDepth
to a pointer (e.g., *int) following the MaxAttachmentSizeMb pattern, update the
defaulting logic to check for nil and set to 2 only when nil, and audit all uses
to dereference safely (use a helper like getContextMaxDepth() or check for nil
before using). Also adjust YAML/JSON unmarshal handling so an explicit 0 stays
non-nil and update any docs or validation that reference ContextMaxDepth to
reflect the pointer semantics.
- Around line 56-57: Existing configs unmarshalling yields
ShowContextIcons=false (Go zero value) which breaks the intended default true;
change the Config struct field ShowContextIcons from bool to *bool (matching
IncludeReasoningTokensInContext) and update setDefaults to set it to
pointer(true) when nil, then update all code that reads ShowContextIcons to
dereference the pointer (e.g., use *cfg.ShowContextIcons or helper accessors) so
missing JSON values default to true while explicit false remains respected.
In `@panes/prompt-pane.go`:
- Around line 442-486: In processContextPickerUpdates, don’t silently ignore the
os.Stat error for p.contextPicker.SelectedPath; when os.Stat(selectedPath)
returns an error, log it (including selectedPath and the error) before skipping
chip creation so failures are debuggable; use the PromptPane logger (e.g.
p.logger.Errorf or similar) if available, otherwise use the standard log
package, and keep the existing behavior of not appending a chip when stat fails.
In `@util/file-reader.go`:
- Around line 118-134: The EOF error check in IsTextFile (and similarly in
DetectEncoding) uses a fragile string comparison err.Error() != "EOF"; replace
this with a proper EOF check using errors.Is(err, io.EOF) (or err == io.EOF) and
import the errors/io package as needed, so the read error handling treats
wrapped EOFs correctly and only returns false for non-EOF errors while still
validating buf[:n] when EOF occurs.
- Around line 16-28: ReadFileContent currently reads the entire file into memory
and can OOM on very large files; before reading, check the file size (os.Stat)
and reject or limit reads above a configured cap (e.g., 5MB to match
updatePreview) or use an io.LimitedReader to read only up to that cap, then
return a clear error when the file exceeds the limit; update callers like
processContextChips (and any consumer expecting full content) to handle the
"file too large" error path appropriately and keep utf8.Valid on the
buffered/truncated content (referencing ReadFileContent and
processContextChips/updatePreview in changes).
- Around line 178-201: Remove the unused DetectEncoding and IsTextFile functions
from util/file-reader.go: delete both function bodies (the UTF-8/NULL-byte
checks) and any references to them, and then remove now-unused imports (e.g.,
bytes and utf8) from the file's import block so there are no leftover dead
symbols; ensure ReadFileContent remains the single source of text-vs-binary
detection.
In `@util/shared-types.go`:
- Around line 126-132: Remove the dead type definition FileContext from
util/shared-types.go: delete the entire type block named FileContext (which
defines Path, Name, IsFolder, Size) since the codebase only uses
FileContextChip; ensure no other code references FileContext before removing and
run tests/compile to confirm no usages remain.
In `@views/main-view.go`:
- Around line 360-374: The call to m.processContextChips in the Update loop
blocks the main Bubble Tea goroutine because it reads file/folder contents
synchronously; refactor by moving the heavy work into an asynchronous tea.Cmd
that runs the file I/O off the main thread and returns a message with the result
(e.g., define a ContextChipsProcessedMsg and a cmd makeProcessContextChipsCmd
that calls m.processContextChips or its refactored helper), dispatch that cmd
where processContextChips is currently invoked, and in the Update case that
handles ContextChipsProcessedMsg append the constructed content to
m.sessionOrchestrator.ArrayOfMessages (using the same "user" Role/Content
composition) so UI remains responsive.
🧹 Nitpick comments (6)
util/shared-types.go (1)
116-124:Typefield is redundant withIsFolder.
FileContextChip.Type(values"file"or"folder") duplicates the information already captured byIsFolder bool. Every usage in the PR setsTypebased onIsFolder. Consider removingTypeto avoid keeping two fields in sync.util/file-reader.go (1)
30-99:ReadFolderContentsandCountFilesInFolderduplicate the entire traversal logic.Both functions implement the same
WalkDirwith identical depth calculation, hidden-file skipping, and media-file filtering. Extract a shared walker/filter helper to avoid maintaining two copies.Sketch of a shared helper
// walkFilteredFiles calls fn for each eligible (non-hidden, non-media) file up to maxDepth. func walkFilteredFiles(path string, maxDepth int, fn func(filePath string, d fs.DirEntry) error) error { return filepath.WalkDir(path, func(filePath string, d fs.DirEntry, err error) error { if err != nil { return err } if filePath == path { return nil } relPath, err := filepath.Rel(path, filePath) if err != nil { return err } depth := strings.Count(relPath, string(filepath.Separator)) if depth > maxDepth { if d.IsDir() { return fs.SkipDir } return nil } if strings.HasPrefix(filepath.Base(filePath), ".") { if d.IsDir() { return fs.SkipDir } return nil } if d.IsDir() { return nil } ext := strings.ToLower(filepath.Ext(filePath)) if slices.Contains(MediaExtensions, ext) { return nil } return fn(filePath, d) }) }Also applies to: 204-260
views/main-view.go (1)
628-661:processContextChipsdoes not deduplicate chips.If the same file/folder is selected multiple times (user adds it via
@twice), its content will be included in the prompt multiple times. Consider deduplicating bychip.Path.Proposed dedup
func (m MainView) processContextChips(chips []util.FileContextChip) string { var contextContent strings.Builder maxDepth := m.config.ContextMaxDepth + seen := make(map[string]bool) for _, chip := range chips { + if seen[chip.Path] { + continue + } + seen[chip.Path] = true + if chip.IsFolder {panes/prompt-pane.go (1)
640-653: Config is re-read from context on every picker open.
config.FromContext(p.mainCtx)is called each time the context picker opens, but config is immutable at runtime. Consider caching the relevant config values in thePromptPanestruct duringNewPromptPaneto avoid repeated context lookups.components/context-picker.go (2)
195-230: TemporaryContextPickerjust to callscanDirectoryis unnecessary.
scanDirectorydoesn't use anyContextPickerstate — it only accessesutil.Slogandutil.MediaExtensions. Make it a standalone function or a package-level helper.
215-218: Package-level style variables are mutated inNewContextPicker.
contextPickerListItemSpanandcontextPickerListItemSpanSelected(lines 32-36) are package-level vars reassigned on everyNewContextPickercall. While unlikely to be a concurrency issue in a single-threaded TUI, this is a code smell. Consider making these fields ofContextPickeror the delegate.
| func IsTextFile(path string) bool { | ||
| file, err := os.Open(path) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| defer file.Close() | ||
|
|
||
| // Read first 512 bytes to check | ||
| buf := make([]byte, 512) | ||
| n, err := file.Read(buf) | ||
| if err != nil && err.Error() != "EOF" { | ||
| return false | ||
| } | ||
|
|
||
| // Check if the content is valid UTF-8 | ||
| return utf8.Valid(buf[:n]) | ||
| } |
There was a problem hiding this comment.
EOF comparison via string is fragile — use errors.Is or direct comparison.
Lines 128 and 190 compare err.Error() != "EOF" instead of using errors.Is(err, io.EOF). String comparison is brittle and breaks if the error is wrapped.
Proposed fix for IsTextFile (same pattern for DetectEncoding)
+ "io"
"os" n, err := file.Read(buf)
- if err != nil && err.Error() != "EOF" {
+ if err != nil && err != io.EOF {
return false
}Also applies to: 178-201
🤖 Prompt for AI Agents
In `@util/file-reader.go` around lines 118 - 134, The EOF error check in
IsTextFile (and similarly in DetectEncoding) uses a fragile string comparison
err.Error() != "EOF"; replace this with a proper EOF check using errors.Is(err,
io.EOF) (or err == io.EOF) and import the errors/io package as needed, so the
read error handling treats wrapped EOFs correctly and only returns false for
non-EOF errors while still validating buf[:n] when EOF occurs.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@components/context-picker.go`:
- Around line 90-116: The preview block added in ContextPicker.View (checking
l.preview and using previewStyle with MaxHeight(10)) can cause overflow because
the list height isn't reduced; update ContextPicker.View to compute the
previewHeight (use the same 10 lines or derive from previewStyle) and subtract
it from the list's display height before rendering the list (call the list's
SetHeight/SetSize or equivalent on l.list so its height = totalAvailableHeight -
previewHeight when l.preview != ""), ensuring the preview and list fit within
the allocated space and preserve existing behavior when no preview is shown.
- Around line 135-149: The Enter key handling currently always selects an item
and closes the picker; guard it the same way as the esc/q branch by checking
filtering state and input focus. In the `case "enter"` block around
`l.GetSelectedItem()`, wrap the selection logic with the same condition:
`l.list.FilterState() != list.Filtering && !l.list.FilterInput.Focused()`; if
the condition is false, do not select/quit (return without changing
`l.SelectedPath`/`l.quitting`), otherwise perform the existing `l.SelectedPath =
...`, `l.quitting = true` and `util.SendViewModeChangedMsg(l.PrevView)`.
In `@config/config-handler.go`:
- Around line 234-245: The code sets default for c.ContextMaxDepth but does not
validate negative values; update the config validation (either in setDefaults or
validateConfig) to clamp or reject negative values for ContextMaxDepth: if
c.ContextMaxDepth != nil and *c.ContextMaxDepth < 0 then set it to 0 (or return
a validation error), ensuring any usage of c.ContextMaxDepth (e.g., directory
scanning that compares depth > *c.ContextMaxDepth) will not behave incorrectly;
reference the c.ContextMaxDepth field and the setDefaults/validateConfig
functions when applying the change.
In `@views/main-view.go`:
- Around line 385-412: The handler for util.ContextChipsProcessed currently
returns early on any msg.Errors and discards the prompt; instead, when
msg.Errors is non-empty, still append the user's message (using
m.sessionOrchestrator.ArrayOfMessages with Role "user" and Content msg.Prompt +
contextContent and Attachments msg.Attachments) and continue the usual flow
(m.viewMode = util.NormalMode, m.controlsLocked = true, m.setProcessingContext
and the tea.Sequence that follows), but also surface errors non-blockingly by
emitting or appending an error/notification (e.g., create a util.MakeErrorMsg or
util.NotificationMsg containing strings.Join(msg.Errors, "\n") and send it via
the same return sequence rather than returning immediately). Locate this logic
in the switch case handling util.ContextChipsProcessed and replace the early
return on msg.Errors with the non-blocking notification approach so partial
results are preserved.
🧹 Nitpick comments (4)
util/file-reader.go (2)
44-111: Duplicated traversal logic betweenReadFolderContentsandCountFilesInFolder.Both functions implement nearly identical directory-walking logic (depth calculation, hidden-file/dir skipping, media-file skipping). Consider extracting a shared
walkEligibleFileshelper that accepts a callback, reducing the ~50 lines of duplication.♻️ Sketch of a shared walker
+// walkEligibleFiles walks non-hidden, non-media files up to maxDepth and calls fn for each. +func walkEligibleFiles(path string, maxDepth int, fn func(filePath string, d fs.DirEntry) error) error { + return filepath.WalkDir(path, func(filePath string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if filePath == path { + return nil + } + relPath, err := filepath.Rel(path, filePath) + if err != nil { + return err + } + depth := strings.Count(relPath, string(filepath.Separator)) + if depth > maxDepth { + if d.IsDir() { + return fs.SkipDir + } + return nil + } + if strings.HasPrefix(filepath.Base(filePath), ".") { + if d.IsDir() { + return fs.SkipDir + } + return nil + } + if d.IsDir() { + return nil + } + ext := strings.ToLower(filepath.Ext(filePath)) + if slices.Contains(MediaExtensions, ext) { + return nil + } + return fn(filePath, d) + }) +}Also applies to: 171-227
128-152:FormatFileContentuses the full absolute path in headers — consider relative paths.Line 135 embeds
path(which is an absolute filesystem path fromReadFolderContents) directly into the formatted output that gets sent as prompt context. This leaks the user's full directory structure (e.g.,/home/user/projects/...) into the LLM prompt. Using a relative path (relative to the selected folder) would be more meaningful for the model and less noisy.components/context-picker.go (2)
182-201: Redundantos.Statcall insideupdatePreview.The caller at Line 168 already calls
os.Statand passes themodTime. InsideupdatePreview, Line 184 callsos.Statagain on the same path to getinfo.Size(). You could pass the size (or theos.FileInfo) from the caller to avoid the extra syscall on every selection change.♻️ Proposed refactor
-func (l *ContextPicker) updatePreview(filePath string, modTime time.Time) { - // Get file info to check size - info, err := os.Stat(filePath) - if err != nil { - util.Slog.Error("failed to get file info for preview", "path", filePath, "error", err.Error()) - l.preview = "" - l.previewPath = "" - l.previewModTime = time.Time{} - return - } - - // Don't preview files larger than 5MB to save performance - const maxPreviewSize = 5 * 1024 * 1024 // 5MB in bytes - if info.Size() > maxPreviewSize { +func (l *ContextPicker) updatePreview(filePath string, info os.FileInfo) { + const maxPreviewSize = 5 * 1024 * 1024 + if info.Size() > maxPreviewSize {And at the call site:
- if err == nil && (item.Path != l.previewPath || info.ModTime() != l.previewModTime) { - l.updatePreview(item.Path, info.ModTime()) + if err == nil && (item.Path != l.previewPath || info.ModTime() != l.previewModTime) { + l.updatePreview(item.Path, info)
278-313:scanDirectorydoesn't useContextPickerstate — needless temporary instance.Line 286-287 creates a throwaway
ContextPickerjust to callscanDirectory. SincescanDirectorydoesn't access any receiver fields, it should be a standalone function (or at minimum, called directly on the picker being constructed).♻️ Suggested change
Either make it a package-level function:
-func (l *ContextPicker) scanDirectory(dir string, currentDepth, maxDepth int) []list.Item { +func scanDirectory(dir string, currentDepth, maxDepth int) []list.Item {Or call it on the result picker and build the list after construction.
| case tea.KeyMsg: | ||
| switch msg.String() { | ||
| case "esc", "q": | ||
| // Only quit if not actively filtering (check both FilterState and filter input focus) | ||
| if l.list.FilterState() != list.Filtering && !l.list.FilterInput.Focused() { | ||
| l.quitting = true | ||
| return *l, util.SendViewModeChangedMsg(l.PrevView) | ||
| } | ||
| case "enter": | ||
| if item, ok := l.GetSelectedItem(); ok { | ||
| l.SelectedPath = item.Path | ||
| l.quitting = true | ||
| return *l, util.SendViewModeChangedMsg(l.PrevView) | ||
| } | ||
| } |
There was a problem hiding this comment.
enter key is not guarded against active filtering — will select instead of confirming filter.
The esc/q keys are correctly guarded (Line 139), but enter at Line 143 unconditionally selects the item. When the user is typing a filter and presses enter to confirm it, this intercepts the keypress, selects whatever item is highlighted, and closes the picker.
🐛 Proposed fix
case "enter":
+ if l.list.FilterState() == list.Filtering || l.list.FilterInput.Focused() {
+ break
+ }
if item, ok := l.GetSelectedItem(); ok {
l.SelectedPath = item.Path
l.quitting = true
return *l, util.SendViewModeChangedMsg(l.PrevView)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case tea.KeyMsg: | |
| switch msg.String() { | |
| case "esc", "q": | |
| // Only quit if not actively filtering (check both FilterState and filter input focus) | |
| if l.list.FilterState() != list.Filtering && !l.list.FilterInput.Focused() { | |
| l.quitting = true | |
| return *l, util.SendViewModeChangedMsg(l.PrevView) | |
| } | |
| case "enter": | |
| if item, ok := l.GetSelectedItem(); ok { | |
| l.SelectedPath = item.Path | |
| l.quitting = true | |
| return *l, util.SendViewModeChangedMsg(l.PrevView) | |
| } | |
| } | |
| case tea.KeyMsg: | |
| switch msg.String() { | |
| case "esc", "q": | |
| // Only quit if not actively filtering (check both FilterState and filter input focus) | |
| if l.list.FilterState() != list.Filtering && !l.list.FilterInput.Focused() { | |
| l.quitting = true | |
| return *l, util.SendViewModeChangedMsg(l.PrevView) | |
| } | |
| case "enter": | |
| if l.list.FilterState() == list.Filtering || l.list.FilterInput.Focused() { | |
| break | |
| } | |
| if item, ok := l.GetSelectedItem(); ok { | |
| l.SelectedPath = item.Path | |
| l.quitting = true | |
| return *l, util.SendViewModeChangedMsg(l.PrevView) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@components/context-picker.go` around lines 135 - 149, The Enter key handling
currently always selects an item and closes the picker; guard it the same way as
the esc/q branch by checking filtering state and input focus. In the `case
"enter"` block around `l.GetSelectedItem()`, wrap the selection logic with the
same condition: `l.list.FilterState() != list.Filtering &&
!l.list.FilterInput.Focused()`; if the condition is false, do not select/quit
(return without changing `l.SelectedPath`/`l.quitting`), otherwise perform the
existing `l.SelectedPath = ...`, `l.quitting = true` and
`util.SendViewModeChangedMsg(l.PrevView)`.
|
|
||
| // Set default context max depth to 2 for recursive file scanning | ||
| if c.ContextMaxDepth == nil { | ||
| defaultDepth := 2 | ||
| c.ContextMaxDepth = &defaultDepth | ||
| } | ||
|
|
||
| // Set default show context icons to true | ||
| if c.ShowContextIcons == nil { | ||
| defaultShowIcons := true | ||
| c.ShowContextIcons = &defaultShowIcons | ||
| } |
There was a problem hiding this comment.
No validation for negative ContextMaxDepth.
A user could set "contextMaxDepth": -1 in their config, which would silently cause all directory entries to be skipped (since depth > -1 is always true). Consider clamping to >= 0 in setDefaults or validateConfig.
🛡️ Proposed fix
if c.ContextMaxDepth == nil {
defaultDepth := 2
c.ContextMaxDepth = &defaultDepth
+ } else if *c.ContextMaxDepth < 0 {
+ defaultDepth := 0
+ c.ContextMaxDepth = &defaultDepth
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Set default context max depth to 2 for recursive file scanning | |
| if c.ContextMaxDepth == nil { | |
| defaultDepth := 2 | |
| c.ContextMaxDepth = &defaultDepth | |
| } | |
| // Set default show context icons to true | |
| if c.ShowContextIcons == nil { | |
| defaultShowIcons := true | |
| c.ShowContextIcons = &defaultShowIcons | |
| } | |
| // Set default context max depth to 2 for recursive file scanning | |
| if c.ContextMaxDepth == nil { | |
| defaultDepth := 2 | |
| c.ContextMaxDepth = &defaultDepth | |
| } else if *c.ContextMaxDepth < 0 { | |
| defaultDepth := 0 | |
| c.ContextMaxDepth = &defaultDepth | |
| } | |
| // Set default show context icons to true | |
| if c.ShowContextIcons == nil { | |
| defaultShowIcons := true | |
| c.ShowContextIcons = &defaultShowIcons | |
| } |
🤖 Prompt for AI Agents
In `@config/config-handler.go` around lines 234 - 245, The code sets default for
c.ContextMaxDepth but does not validate negative values; update the config
validation (either in setDefaults or validateConfig) to clamp or reject negative
values for ContextMaxDepth: if c.ContextMaxDepth != nil and *c.ContextMaxDepth <
0 then set it to 0 (or return a validation error), ensuring any usage of
c.ContextMaxDepth (e.g., directory scanning that compares depth >
*c.ContextMaxDepth) will not behave incorrectly; reference the c.ContextMaxDepth
field and the setDefaults/validateConfig functions when applying the change.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@components/context-picker.go`:
- Around line 219-236: In error paths where you currently clear l.preview and
l.previewPath (specifically after failing to open the file, after the peek Read
error, and in the binary-check and UTF-8 error branches), also reset
l.previewModTime to its zero value (time.Time{}) so the cache check that
compares mod times (l.previewModTime) cannot be falsely satisfied; update the
same branches that clear l.preview/l.previewPath to set l.previewModTime =
time.Time{} (ensure you import time if not already present).
In `@views/main-view.go`:
- Around line 707-740: Remove the unused duplicate method processContextChips
from views/main-view.go: it duplicates the chip-processing logic already
implemented in makeProcessContextChipsCmd and is not referenced anywhere; delete
the entire function (func (m MainView) processContextChips(...){...}) and ensure
no remaining references to processContextChips exist, relying on
makeProcessContextChipsCmd for the canonical implementation, then run
build/tests to verify nothing breaks.
- Around line 405-417: The current code appends util.MakeErrorMsg(errorMsg) into
the tea.Sequence which triggers the ErrorEvent handler and aborts the in-flight
completion; instead, do not append MakeErrorMsg to the sequence after
m.chatPane.DisplayCompletion — remove that append and emit the error
notification via a non-blocking/log-only path (e.g., call a non-blocking
logger/notification helper or send the message via a fire-and-forget mechanism
that does not route through the ErrorEvent path). Update the block that builds
sequence (referencing sequence, m.chatPane.DisplayCompletion, and
util.MakeErrorMsg) to log/post the warning asynchronously (or via a dedicated
util.LogWarning/NotifyNonBlocking helper) so the completion continues
uninterrupted.
🧹 Nitpick comments (4)
components/context-picker.go (4)
198-207: Redundantos.Statcall — file info is already available from the caller.
Update()at line 184 already callsos.Statand passesmodTime. Here at line 200,os.Statis called again on the same path just to check the size. Pass theos.FileInfofrom the caller instead of re-statting.♻️ Proposed refactor
Change the signature to accept
os.FileInfo:-func (l *ContextPicker) updatePreview(filePath string, modTime time.Time) { - // Get file info to check size - info, err := os.Stat(filePath) - if err != nil { - util.Slog.Error("failed to get file info for preview", "path", filePath, "error", err.Error()) - l.preview = "" - l.previewPath = "" - l.previewModTime = time.Time{} - return - } +func (l *ContextPicker) updatePreview(filePath string, info os.FileInfo) {And at the call site (line 186):
- l.updatePreview(item.Path, info.ModTime()) + l.updatePreview(item.Path, info)Then use
info.ModTime()when assigningl.previewModTimeat line 291.
90-112: Height adjustment/restore approach inView()is fragile.Mutating
l.list's height before rendering and restoring it afterward works only if nothing else reads the height in between. If the list'sView()triggers any side effect that persists the reduced height, the restore won't fully undo it. A safer approach is to compute the available height without mutating the list:♻️ Suggested approach
Instead of mutating and restoring, compute the preview-adjusted height once in
SetSizeand store bothtotalHeightandlistHeight(accounting for preview). ThenView()only reads, never writes.
314-317: Package-level style variables are mutated inNewContextPicker.
contextPickerListItemSpanandcontextPickerListItemSpanSelectedare package-level vars whoseForegroundis reassigned every timeNewContextPickeris called. If the picker is recreated with different color schemes, this silently affects any other reference. Consider making these fields on theContextPickerstruct or on the delegate.
331-397:scanDirectorydoesn't need a receiver — it's called on a throwaway instance.At line 302–303, a temporary
ContextPicker{}is created solely to callscanDirectory. Since the method doesn't use anyContextPickerstate, make it a standalone function.♻️ Proposed change
-func (l *ContextPicker) scanDirectory(dir string, currentDepth, maxDepth int) []list.Item { +func scanDirectory(dir string, currentDepth, maxDepth int) []list.Item {And at line 303:
- tempPicker := ContextPicker{} - items := tempPicker.scanDirectory(baseDir, 0, maxDepth) + items := scanDirectory(baseDir, 0, maxDepth)
| // Open file for reading | ||
| file, err := os.Open(filePath) | ||
| if err != nil { | ||
| util.Slog.Error("failed to open file for preview", "path", filePath, "error", err.Error()) | ||
| l.preview = "" | ||
| l.previewPath = "" | ||
| return | ||
| } | ||
| defer file.Close() | ||
|
|
||
| // Peek at first 512 bytes to check for binary content | ||
| peekBuf := make([]byte, 512) | ||
| n, err := file.Read(peekBuf) | ||
| if err != nil && err != io.EOF { | ||
| util.Slog.Error("failed to peek at file for preview", "path", filePath, "error", err.Error()) | ||
| l.preview = "" | ||
| l.previewPath = "" | ||
| return |
There was a problem hiding this comment.
previewModTime not cleared on some error paths.
Lines 223–225 and 233–236 clear preview and previewPath but do not reset previewModTime. This means a subsequent file with the same mod time could incorrectly hit the cache check at line 185.
🐛 Proposed fix
file, err := os.Open(filePath)
if err != nil {
util.Slog.Error("failed to open file for preview", "path", filePath, "error", err.Error())
l.preview = ""
l.previewPath = ""
+ l.previewModTime = time.Time{}
return
}
defer file.Close()
// Peek at first 512 bytes to check for binary content
peekBuf := make([]byte, 512)
n, err := file.Read(peekBuf)
if err != nil && err != io.EOF {
util.Slog.Error("failed to peek at file for preview", "path", filePath, "error", err.Error())
l.preview = ""
l.previewPath = ""
+ l.previewModTime = time.Time{}
return
}The same applies to the binary-check and UTF-8 error paths at lines 250–263 and 266–271.
🤖 Prompt for AI Agents
In `@components/context-picker.go` around lines 219 - 236, In error paths where
you currently clear l.preview and l.previewPath (specifically after failing to
open the file, after the peek Read error, and in the binary-check and UTF-8
error branches), also reset l.previewModTime to its zero value (time.Time{}) so
the cache check that compares mod times (l.previewModTime) cannot be falsely
satisfied; update the same branches that clear l.preview/l.previewPath to set
l.previewModTime = time.Time{} (ensure you import time if not already present).
|
Thanks for taking this feature into work! Been putting this ticket off for too long. I’ve had a look at look the implementation and have the following comments:
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@clients/gemini.go`:
- Around line 445-452: The code concatenates ContextContent onto messageContent
with no separator and logs noisy/debug text; update the concatenation in the
Gemini, OpenAI, and OpenRouter clients so when singleMessage.ContextContent is
non-empty you append a separator (e.g., "\n\n" or at minimum "\n") before adding
singleMessage.ContextContent to messageContent, remove the stale "(FIXED)"
suffix from the debug message, and delete the noisy else-branch debug log so you
only log when ContextContent is actually included (adjust the util.Slog.Debug
call in the blocks that reference singleMessage.ContextContent/messageContent in
clients/gemini.go, clients/openai.go, and clients/openrouter.go).
In `@components/file-picker.go`:
- Around line 224-274: filterFilePickerView currently calls os.ReadDir on every
render (via View), causing excessive I/O; modify the FilePicker to cache
directory entries and only refresh that cache when m.filepicker.CurrentDirectory
changes (or when an explicit refresh is requested). Add a field on FilePicker to
store cached entries and the cachedDir path, update the cache in the code path
that changes the current directory (or lazily refresh inside
filterFilePickerView by comparing cachedDir to m.filepicker.CurrentDirectory),
and then use the cached entries for filtering and building the view instead of
calling os.ReadDir on every invocation.
- Around line 108-152: The key handler currently forces filter focus via a
printable-char heuristic and allows "q" to close even while typing; remove the
heuristic block that sets m.filterInputFocused based on single-character
printable keys and the associated enter/tab blur heuristic, so filter focus is
only toggled by the explicit Ctrl+/ handler (which calls m.filterInput.Focus()
and sets m.filterInputFocused = true) and by explicit blur actions (e.g., Esc
handling that calls m.filterInput.Blur()); also change the "q" case so it only
triggers quitting when m.filterInputFocused is false (i.e., ignore/consume "q"
while filter is focused) while leaving the existing Esc two-stage logic intact.
In `@panes/prompt-pane.go`:
- Around line 510-518: The @ trigger logic currently only checks p.input.Value()
in the default input branch and thus never fires when the user is in
TextEditMode; update the TextEditMode handling (the case that handles
TextEditMode) to perform the same check as the default branch: inspect the
editor buffer (the TextEditMode input content) to see if the @ was typed at the
start or after a space, set p.filePicker.IsContextMode = true and dispatch
util.SendViewModeChangedWithContextMsg(util.FilePickerMode, true). Ensure you
reference the same functions/fields (TextEditMode handling, p.input.Value()
equivalent for the editor, filePicker.IsContextMode,
SendViewModeChangedWithContextMsg, FilePickerMode) so behavior is identical from
the editor and single-line input.
In `@util/shared-types.go`:
- Around line 118-126: The struct FileContextChip currently stores redundant
folder info in both IsFolder (bool) and Type (string), risking inconsistency;
remove the Type field and rely on IsFolder (or keep Type and remove IsFolder if
you prefer string), update any code that reads/writes Type to use IsFolder (or
derive Type from IsFolder when needed), and add JSON tags for exported fields on
FileContextChip so serialization matches LocalStoreMessage.ContextChips (e.g.,
json:"path", json:"name", json:"is_folder", json:"size", json:"file_count")
ensuring all callers that marshal/unmarshal ContextChips still work.
🧹 Nitpick comments (10)
clients/openrouter.go (1)
270-277: Clean up leftover debug artifacts in comments and log messages.The
// FIX:comment prefix and"(FIXED)"in the log message are development-time leftovers. Also, theelsebranch logs "No ContextContent" for every single message (including all assistant messages), which will be noisy in debug logs.🧹 Suggested cleanup
- // FIX: Include ContextContent in the message sent to LLM + // Include ContextContent in the message sent to LLM if singleMessage.ContextContent != "" { - util.Slog.Debug("OpenRouter: Including ContextContent in message (FIXED)", + util.Slog.Debug("OpenRouter: Including ContextContent in message", "contextContentLength", len(singleMessage.ContextContent)) messageContent += singleMessage.ContextContent - } else { - util.Slog.Debug("OpenRouter: No ContextContent in message") }clients/openai.go (1)
273-280: Same leftover debug artifacts as inopenrouter.go.Same
// FIX:comment and"(FIXED)"log message. See the same suggestion inopenrouter.go— clean these up consistently across both clients.🧹 Suggested cleanup
- // FIX: Include ContextContent in the message sent to LLM + // Include ContextContent in the message sent to LLM if singleMessage.ContextContent != "" { - util.Slog.Debug("OpenAI: Including ContextContent in message (FIXED)", + util.Slog.Debug("OpenAI: Including ContextContent in message", "contextContentLength", len(singleMessage.ContextContent)) messageContent += singleMessage.ContextContent - } else { - util.Slog.Debug("OpenAI: No ContextContent in message") }components/file-picker.go (2)
332-359:GetPreviewViewcallsos.Staton every render.Similar to
filterFilePickerView,GetPreviewViewperformsos.Stateach time it's called fromView(). Since the preview file only changes on selection, cache the stat result alongsidepreviewContentwhen the file is selected.
58-68: Context mode setsAllowedTypesto empty while media mode usesMediaExtensions.In context mode (
isContextMode = true),AllowedTypesis set to[]string{}which allows all files, and media files are filtered out at selection time (line 176). In media mode (isContextMode = false),AllowedTypesis set toMediaExtensions.This means the non-context mode only allows selecting media files (images, videos, etc.) — which makes sense for the attachment flow. The context mode allows all files but rejects media at selection time. This works but gives a poor UX since users see media files they can't select. Consider whether the filepicker's
AllowedTypescould exclude media files upfront in context mode, though this depends on whetherAllowedTypessupports exclusion patterns.views/main-view.go (2)
618-641: Magic number10in preview height calculation.Line 622:
previewHeight := m.terminalHeight - 10uses an unexplained constant. Consider extracting it as a named constant (e.g.,filePickerChromeHeight) or computing it from actual border/margin sizes to avoid breakage at small terminal sizes.
449-452: Consider removing verbose debug logs before merge.Lines 449, 452, 463, and 656 add debug logging for mouse clicks and render cycles. These are useful during development but will produce noise in debug mode during normal usage. Consider removing or gating behind a more verbose trace level.
panes/prompt-pane.go (4)
446-457:FileCountis always 0 for folders — consider populating or removing the field.When a folder is selected,
chip.FileCountis hardcoded to0(line 456), which is indistinguishable from "no files" vs. "unknown." If the count isn't needed yet, consider omitting the assignment (the zero value already applies) and adding a comment, or populate it with a directory listing count if it's intended to be displayed downstream.
430-483: Duplicate-check logic is repeated — extract a generic helper.The duplicate-detection pattern (linear scan + boolean flag) is copy-pasted for
contextChips(lines 437-442) andattachments(lines 467-472). A small helper likecontainsPath([]T, string) bool(or usingslices.ContainsFunc) would reduce duplication.Example using slices.ContainsFunc
- isDuplicate := false - for _, existingChip := range p.contextChips { - if existingChip.Path == selectedPath { - isDuplicate = true - break - } - } - - if !isDuplicate { + if !slices.ContainsFunc(p.contextChips, func(c util.FileContextChip) bool { + return c.Path == selectedPath + }) {Apply the same pattern for the attachments duplicate check.
417-422: Verbose debug logging on every key press in file-picker mode.This fires for every
tea.KeyMsgwhile inFilePickerMode. Consider gating it behind a more specific condition or removing it before merge, as it can generate significant noise in debug logs.
802-830: Public getters expose internal presentation details — consider narrowing the surface.
GetInfoLabelStyle()andGetInfoBlockStyle()return package-level style variables rather than instance state, andGetInputContainerStyle()leaks the container style. These couple the main view tightly to prompt-pane internals. If these are only used by the main view for the file-picker layout, a single method that renders the full file-picker section (accepting height) would be more encapsulated.
| // FIX: Include ContextContent in the message sent to LLM | ||
| if singleMessage.ContextContent != "" { | ||
| util.Slog.Debug("Gemini: Including ContextContent in message (FIXED)", | ||
| "contextContentLength", len(singleMessage.ContextContent)) | ||
| messageContent += singleMessage.ContextContent | ||
| } else { | ||
| util.Slog.Debug("Gemini: No ContextContent in message") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "ContextContent" --type=go -C3Repository: BalanceBalls/nekot
Length of output: 11374
🏁 Script executed:
ls -la clients/*.go | wc -l && ls clients/*.go | xargs -I {} basename {}Repository: BalanceBalls/nekot
Length of output: 109
🏁 Script executed:
# Check OpenAI client for context
sed -n '260,290p' clients/openai.go
# Also check what messageContent is initialized with and what happens before ContextContent
sed -n '240,290p' clients/gemini.goRepository: BalanceBalls/nekot
Length of output: 2218
🏁 Script executed:
sed -n '430,460p' clients/gemini.goRepository: BalanceBalls/nekot
Length of output: 875
ContextContent is concatenated without separators — affects all three client implementations.
When Content and ContextContent are both non-empty, they merge directly with no delimiter (line 449: messageContent += singleMessage.ContextContent). This will concatenate the last word of the prompt directly with the first word of the context, producing invalid text. Add at least a newline separator.
This same issue exists in clients/openai.go (line 277) and clients/openrouter.go (line 274)—all three clients should be fixed together.
Additionally, the debug message contains a stale "(FIXED)" suffix, and the else branch (lines 450-452) logs for every message without context, which is noisy and redundant.
Proposed fix
- // FIX: Include ContextContent in the message sent to LLM
if singleMessage.ContextContent != "" {
- util.Slog.Debug("Gemini: Including ContextContent in message (FIXED)",
+ util.Slog.Debug("Gemini: Including ContextContent in message",
"contextContentLength", len(singleMessage.ContextContent))
- messageContent += singleMessage.ContextContent
- } else {
- util.Slog.Debug("Gemini: No ContextContent in message")
+ messageContent += "\n" + singleMessage.ContextContent
}Apply the same fix to clients/openai.go and clients/openrouter.go.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // FIX: Include ContextContent in the message sent to LLM | |
| if singleMessage.ContextContent != "" { | |
| util.Slog.Debug("Gemini: Including ContextContent in message (FIXED)", | |
| "contextContentLength", len(singleMessage.ContextContent)) | |
| messageContent += singleMessage.ContextContent | |
| } else { | |
| util.Slog.Debug("Gemini: No ContextContent in message") | |
| } | |
| if singleMessage.ContextContent != "" { | |
| util.Slog.Debug("Gemini: Including ContextContent in message", | |
| "contextContentLength", len(singleMessage.ContextContent)) | |
| messageContent += "\n" + singleMessage.ContextContent | |
| } |
🤖 Prompt for AI Agents
In `@clients/gemini.go` around lines 445 - 452, The code concatenates
ContextContent onto messageContent with no separator and logs noisy/debug text;
update the concatenation in the Gemini, OpenAI, and OpenRouter clients so when
singleMessage.ContextContent is non-empty you append a separator (e.g., "\n\n"
or at minimum "\n") before adding singleMessage.ContextContent to
messageContent, remove the stale "(FIXED)" suffix from the debug message, and
delete the noisy else-branch debug log so you only log when ContextContent is
actually included (adjust the util.Slog.Debug call in the blocks that reference
singleMessage.ContextContent/messageContent in clients/gemini.go,
clients/openai.go, and clients/openrouter.go).
|
@BalanceBalls Thanks for replying, I have reuse the file-picker.go component instead. From my testing, both the image pane and chat pane should all be working now. And the keybinding for hide/show the context content will be Below some images on the updated UI (Ran with llama.cpp, model Gemma3, 4B): Context selection (text-based, for the image selector, we'll also have a filter to only allow images): Context injected: Image injected: I've opt for the more vertical |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@components/file-picker.go`:
- Around line 376-413: filterFilePickerView currently calls m.recursiveSearch
and assigns to m.searchResults but because View() uses a value receiver that
copy‑assigns, the assignment is discarded and recursiveSearch runs on every
render; move the search invocation and caching into the pointer‑receiver Update
method on FilePicker so recursiveSearch is only executed when
m.filterInput.Value() or m.filepicker.CurrentDirectory (or m.searchDepth)
changes, storing results in the FilePicker.searchResults field, and have
filterFilePickerView simply read the cached searchResults (and handle empty
state) without calling recursiveSearch.
- Around line 664-736: The current updatePreviewFromView method is fragile
because it parses the rendered ANSI view to find the selected file; instead,
stop reverse-engineering the output and derive the selected item from the
filepicker state (e.g., use m.filepicker.CurrentDirectory together with the
filepicker's selected index/cursor position or the file list backing the view)
rather than scanning view strings; replace the parsing logic in
updatePreviewFromView to fetch the selected filename from the file list (or
searchResults/m.filepicker selection API), then compute fullPath (similar to
current logic) and update m.previewFile and m.previewContent via
m.getFilePreviewContent only when different.
- Around line 499-529: The function getFilePreviewContent currently uses
os.ReadFile(path) which loads the entire file into memory; change it to open the
file (os.Open) and read with a size limit (use io.LimitReader or read into a
buffer sized by maxReadFileSize) so only up to the configured max (e.g.,
maxReadFileSize or 10000+margin) bytes are read, handle read errors and close
the file, then convert the read bytes to string and apply the same truncation
logic; keep the isTextFile check and directory handling as-is and reference
getFilePreviewContent and isTextFile when making the change.
- Around line 122-151: The isTextFile function currently duplicates extensions
from util.CodeExtensions and uses os.ReadFile which loads the whole file into
memory; change it to (1) deduplicate the extra textExtensions against
util.CodeExtensions (remove entries like .txt .md .json .yaml .yml .toml .ini
.cfg .conf that already exist in util.CodeExtensions) and (2) replace
os.ReadFile(path) with opening the file (os.Open) and reading only a small fixed
buffer (e.g., use io.ReadAtLeast or file.Read into a 1024-byte slice) to check
UTF-8 validity, returning false on Open/read errors; keep the rest of isTextFile
logic and reference the function name isTextFile and util.CodeExtensions when
making changes.
In `@views/main-view.go`:
- Around line 413-416: Remove the dead assignment where m.pendingContextChips is
set to msg.ContextChips then immediately overwritten; delete the line
"m.pendingContextChips = msg.ContextChips" so only the intentional clearing
assignment "m.pendingContextChips = []util.FileContextChip{}" remains,
referencing m.pendingContextChips and msg.ContextChips to locate the code.
🧹 Nitpick comments (5)
util/file-reader.go (1)
44-111:ReadFolderContentsandCountFilesInFolderduplicate the entire WalkDir filtering logic.The skip-hidden, skip-media, depth-check, and root-skip logic is copy-pasted between these two functions. Consider extracting a shared
walkFilteredFileshelper that accepts a visitor callback to reduce duplication and ensure future filter changes apply consistently.♻️ Sketch of shared helper
+// walkFilteredFiles walks a directory applying common filters and calls fn for each eligible file. +func walkFilteredFiles(path string, maxDepth int, fn func(filePath string, d fs.DirEntry) error) error { + return filepath.WalkDir(path, func(filePath string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if filePath == path { + return nil + } + relPath, err := filepath.Rel(path, filePath) + if err != nil { + return err + } + depth := strings.Count(relPath, string(filepath.Separator)) + if depth > maxDepth { + if d.IsDir() { + return fs.SkipDir + } + return nil + } + baseName := filepath.Base(filePath) + if strings.HasPrefix(baseName, ".") { + if d.IsDir() { + return fs.SkipDir + } + return nil + } + if d.IsDir() { + return nil + } + ext := strings.ToLower(filepath.Ext(filePath)) + if slices.Contains(MediaExtensions, ext) { + return nil + } + return fn(filePath, d) + }) +}Also applies to: 171-227
util/formatter.go (1)
81-137: Context chips are not rendered in visual mode.Lines 88-93 return early for
isVisualModewithout processing context chips. If this is intentional (visual mode is meant to be a compact view), this is fine. Otherwise, users won't see any context chip information when in visual/selection mode.components/file-picker.go (1)
657-662: Regex compiled on every call in a hot rendering path.
stripANSIcompiles a new regex each invocation. Since it's called fromView()→ preview rendering, move the compiled regex to a package-level variable.♻️ Proposed fix
+var ansiRegex = regexp.MustCompile(`\x1b\[[0-9;]*m`) + func stripANSI(s string) string { - ansiRegex := regexp.MustCompile(`\x1b\[[0-9;]*m`) return ansiRegex.ReplaceAllString(s, "") }Note:
util.StripAnsiCodesat line 367-370 ofutil/formatter.goalready does the same thing — consider reusing it.panes/prompt-pane.go (1)
534-542: Inconsistent UTF-8 handling between input modes for@trigger.The TextEditMode path (line 526) correctly uses
utf8.DecodeLastRuneInString, but the default path (line 538) uses raw byte indexingcurrentValue[len(currentValue)-1] == ' '. While this works for ASCII space, it's inconsistent. Consider usingDecodeLastRuneInStringin both paths for consistency.♻️ Consistent UTF-8 check
currentValue := p.input.Value() - if len(currentValue) == 0 || currentValue[len(currentValue)-1] == ' ' { + if len(currentValue) == 0 { + p.filePicker.IsContextMode = true + return util.SendViewModeChangedWithContextMsg(util.FilePickerMode, true) + } + lastRune, _ := utf8.DecodeLastRuneInString(currentValue) + if lastRune == ' ' { p.filePicker.IsContextMode = true return util.SendViewModeChangedWithContextMsg(util.FilePickerMode, true) }views/main-view.go (1)
626-668: Magic number for preview height calculation.Line 630 uses
m.terminalHeight - 10with no explanation. Consider extracting this as a named constant or adding a comment explaining what the 10 accounts for (borders, reminders, status bar, etc.).
| func isTextFile(path string) bool { | ||
| // Check extension against known text/code extensions | ||
| ext := strings.ToLower(filepath.Ext(path)) | ||
| for _, textExt := range util.CodeExtensions { | ||
| if ext == textExt { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| // Additional common text extensions | ||
| textExtensions := []string{".txt", ".md", ".markdown", ".rst", ".log", ".csv", ".json", ".xml", ".yaml", ".yml", ".toml", ".ini", ".cfg", ".conf"} | ||
| for _, textExt := range textExtensions { | ||
| if ext == textExt { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| // Try to read a small portion to check if it's valid UTF-8 | ||
| content, err := os.ReadFile(path) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| // Check first 1024 bytes for UTF-8 validity | ||
| checkSize := 1024 | ||
| if len(content) < checkSize { | ||
| checkSize = len(content) | ||
| } | ||
| return utf8.Valid(content[:checkSize]) | ||
| } |
There was a problem hiding this comment.
isTextFile reads the entire file into memory to check 1024 bytes.
os.ReadFile(path) on line 140 loads the complete file. For a multi-GB file, this will OOM the process — only to inspect 1024 bytes. Use os.Open + io.ReadAtLeast (or a fixed buffer read) instead.
Also, line 132 duplicates extensions already present in util.CodeExtensions (.txt, .md, .json, .yaml, .yml, .toml, .ini, .cfg, .conf).
🐛 Proposed fix
func isTextFile(path string) bool {
ext := strings.ToLower(filepath.Ext(path))
- for _, textExt := range util.CodeExtensions {
- if ext == textExt {
- return true
- }
- }
-
- // Additional common text extensions
- textExtensions := []string{".txt", ".md", ".markdown", ".rst", ".log", ".csv", ".json", ".xml", ".yaml", ".yml", ".toml", ".ini", ".cfg", ".conf"}
- for _, textExt := range textExtensions {
- if ext == textExt {
- return true
- }
+ if slices.Contains(util.CodeExtensions, ext) {
+ return true
+ }
+ // Additional extensions not in CodeExtensions
+ extra := []string{".rst", ".log", ".csv"}
+ if slices.Contains(extra, ext) {
+ return true
}
- // Try to read a small portion to check if it's valid UTF-8
- content, err := os.ReadFile(path)
+ f, err := os.Open(path)
if err != nil {
return false
}
-
- // Check first 1024 bytes for UTF-8 validity
- checkSize := 1024
- if len(content) < checkSize {
- checkSize = len(content)
- }
- return utf8.Valid(content[:checkSize])
+ defer f.Close()
+ buf := make([]byte, 1024)
+ n, _ := f.Read(buf)
+ return n > 0 && utf8.Valid(buf[:n])
}🤖 Prompt for AI Agents
In `@components/file-picker.go` around lines 122 - 151, The isTextFile function
currently duplicates extensions from util.CodeExtensions and uses os.ReadFile
which loads the whole file into memory; change it to (1) deduplicate the extra
textExtensions against util.CodeExtensions (remove entries like .txt .md .json
.yaml .yml .toml .ini .cfg .conf that already exist in util.CodeExtensions) and
(2) replace os.ReadFile(path) with opening the file (os.Open) and reading only a
small fixed buffer (e.g., use io.ReadAtLeast or file.Read into a 1024-byte
slice) to check UTF-8 validity, returning false on Open/read errors; keep the
rest of isTextFile logic and reference the function name isTextFile and
util.CodeExtensions when making changes.
| func (m FilePicker) filterFilePickerView(filterText string) string { | ||
| // Get the current directory from the file picker | ||
| currentDir := m.filepicker.CurrentDirectory | ||
|
|
||
| // In context mode, use recursive search | ||
| if m.IsContextMode { | ||
| // Perform recursive search | ||
| m.searchResults = m.recursiveSearch(filterText, m.searchDepth) | ||
|
|
||
| // If no matches, show a message | ||
| if len(m.searchResults) == 0 { | ||
| return currentDir + "\n\nNo files match filter: " + m.filterInput.Value() | ||
| } | ||
|
|
||
| // Build the filtered view with relative paths | ||
| var lines []string | ||
| lines = append(lines, currentDir) | ||
| lines = append(lines, "") | ||
|
|
||
| for _, result := range m.searchResults { | ||
| // Format the entry line with relative path | ||
| name := result.RelPath | ||
| if result.IsDir { | ||
| name += "/" | ||
| } | ||
|
|
||
| // Add size info | ||
| sizeStr := formatSize(result.Size) | ||
|
|
||
| // Add indentation based on depth for visual hierarchy | ||
| depth := strings.Count(result.RelPath, string(filepath.Separator)) | ||
| indent := strings.Repeat(" ", depth) | ||
|
|
||
| lines = append(lines, indent+name+" "+sizeStr) | ||
| } | ||
|
|
||
| return strings.Join(lines, "\n") | ||
| } |
There was a problem hiding this comment.
recursiveSearch is executed on every render frame when a filter is active.
filterFilePickerView is called from View() (a value receiver), so m.searchResults = m.recursiveSearch(...) on line 383 is assigned to a copy and discarded. This means the filesystem walk runs on every Bubble Tea render cycle with a non-empty filter — potentially hundreds of times per second.
Cache search results in Update() (which has a pointer receiver) and only re-run when the filter text or directory changes.
🤖 Prompt for AI Agents
In `@components/file-picker.go` around lines 376 - 413, filterFilePickerView
currently calls m.recursiveSearch and assigns to m.searchResults but because
View() uses a value receiver that copy‑assigns, the assignment is discarded and
recursiveSearch runs on every render; move the search invocation and caching
into the pointer‑receiver Update method on FilePicker so recursiveSearch is only
executed when m.filterInput.Value() or m.filepicker.CurrentDirectory (or
m.searchDepth) changes, storing results in the FilePicker.searchResults field,
and have filterFilePickerView simply read the cached searchResults (and handle
empty state) without calling recursiveSearch.
| // getFilePreviewContent reads and returns the content of a file for preview | ||
| // Only reads content for text files, returns appropriate message for others | ||
| func (m FilePicker) getFilePreviewContent(path string) string { | ||
| // Check if it's a directory | ||
| info, err := os.Stat(path) | ||
| if err != nil { | ||
| return "Error reading file: " + err.Error() | ||
| } | ||
| if info.IsDir() { | ||
| return "[Directory]" | ||
| } | ||
|
|
||
| // Check if it's a text file | ||
| if !isTextFile(path) { | ||
| return "[Binary file - preview not available]" | ||
| } | ||
|
|
||
| // Read file content | ||
| content, err := os.ReadFile(path) | ||
| if err != nil { | ||
| return "Error reading file: " + err.Error() | ||
| } | ||
|
|
||
| // Convert to string and limit to reasonable size | ||
| contentStr := string(content) | ||
| if len(contentStr) > 10000 { | ||
| contentStr = contentStr[:10000] + "\n... (truncated)" | ||
| } | ||
|
|
||
| return contentStr | ||
| } |
There was a problem hiding this comment.
getFilePreviewContent reads the entire file with no size limit.
os.ReadFile(path) on line 517 loads the full file before truncating to 10,000 characters. A large file (e.g., a multi-GB log) will consume all available memory. Apply a read limit similar to maxReadFileSize or use io.LimitReader.
🐛 Proposed fix
- content, err := os.ReadFile(path)
- if err != nil {
+ f, err := os.Open(path)
+ if err != nil {
+ return "Error reading file: " + err.Error()
+ }
+ defer f.Close()
+ // Read at most ~10KB for preview
+ buf := make([]byte, 10240)
+ n, err := f.Read(buf)
+ if err != nil && n == 0 {
return "Error reading file: " + err.Error()
}
-
- contentStr := string(content)
- if len(contentStr) > 10000 {
- contentStr = contentStr[:10000] + "\n... (truncated)"
+ contentStr := string(buf[:n])
+ if n == len(buf) {
+ contentStr += "\n... (truncated)"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // getFilePreviewContent reads and returns the content of a file for preview | |
| // Only reads content for text files, returns appropriate message for others | |
| func (m FilePicker) getFilePreviewContent(path string) string { | |
| // Check if it's a directory | |
| info, err := os.Stat(path) | |
| if err != nil { | |
| return "Error reading file: " + err.Error() | |
| } | |
| if info.IsDir() { | |
| return "[Directory]" | |
| } | |
| // Check if it's a text file | |
| if !isTextFile(path) { | |
| return "[Binary file - preview not available]" | |
| } | |
| // Read file content | |
| content, err := os.ReadFile(path) | |
| if err != nil { | |
| return "Error reading file: " + err.Error() | |
| } | |
| // Convert to string and limit to reasonable size | |
| contentStr := string(content) | |
| if len(contentStr) > 10000 { | |
| contentStr = contentStr[:10000] + "\n... (truncated)" | |
| } | |
| return contentStr | |
| } | |
| // getFilePreviewContent reads and returns the content of a file for preview | |
| // Only reads content for text files, returns appropriate message for others | |
| func (m FilePicker) getFilePreviewContent(path string) string { | |
| // Check if it's a directory | |
| info, err := os.Stat(path) | |
| if err != nil { | |
| return "Error reading file: " + err.Error() | |
| } | |
| if info.IsDir() { | |
| return "[Directory]" | |
| } | |
| // Check if it's a text file | |
| if !isTextFile(path) { | |
| return "[Binary file - preview not available]" | |
| } | |
| // Read file content | |
| f, err := os.Open(path) | |
| if err != nil { | |
| return "Error reading file: " + err.Error() | |
| } | |
| defer f.Close() | |
| // Read at most ~10KB for preview | |
| buf := make([]byte, 10240) | |
| n, err := f.Read(buf) | |
| if err != nil && n == 0 { | |
| return "Error reading file: " + err.Error() | |
| } | |
| contentStr := string(buf[:n]) | |
| if n == len(buf) { | |
| contentStr += "\n... (truncated)" | |
| } | |
| return contentStr | |
| } |
🤖 Prompt for AI Agents
In `@components/file-picker.go` around lines 499 - 529, The function
getFilePreviewContent currently uses os.ReadFile(path) which loads the entire
file into memory; change it to open the file (os.Open) and read with a size
limit (use io.LimitReader or read into a buffer sized by maxReadFileSize) so
only up to the configured max (e.g., maxReadFileSize or 10000+margin) bytes are
read, handle read errors and close the file, then convert the read bytes to
string and apply the same truncation logic; keep the isTextFile check and
directory handling as-is and reference getFilePreviewContent and isTextFile when
making the change.
| // updatePreviewFromView extracts the currently selected file from filepicker's rendered view | ||
| // This allows preview to update when navigating with arrow keys, not just on Enter | ||
| func (m *FilePicker) updatePreviewFromView(view string) { | ||
| if !m.IsContextMode { | ||
| return | ||
| } | ||
|
|
||
| util.Slog.Debug("FilePicker: updatePreviewFromView called", "view_length", len(view)) | ||
| util.Slog.Debug("FilePicker: CurrentDirectory", "path", m.filepicker.CurrentDirectory) | ||
| util.Slog.Debug("FilePicker: searchResults count", "count", len(m.searchResults)) | ||
|
|
||
| // Parse the view to find the currently selected file (marked with cursor) | ||
| lines := strings.Split(view, "\n") | ||
| for i, line := range lines { | ||
| // Look for cursor indicator (filepicker uses > for cursor) | ||
| if strings.Contains(line, ">") { | ||
| util.Slog.Debug("FilePicker: Found cursor line", "line_index", i, "line_content", line) | ||
|
|
||
| // Strip ANSI escape codes | ||
| cleanLine := stripANSI(line) | ||
| util.Slog.Debug("FilePicker: Cleaned line (no ANSI)", "line_content", cleanLine) | ||
|
|
||
| // Extract file path from the line | ||
| // Format: "> 4.1kB clients" or "> 18kB chat-pane.go" | ||
| // The format is: cursor + spaces + size + spaces + filename | ||
| parts := strings.Split(cleanLine, ">") | ||
| if len(parts) > 1 { | ||
| rest := strings.TrimSpace(parts[1]) | ||
| util.Slog.Debug("FilePicker: Extracted rest after >", "rest", rest) | ||
|
|
||
| // Split by spaces to get size and filename | ||
| // Format: "4.1kB clients" -> ["4.1kB", "clients"] | ||
| fields := strings.Fields(rest) | ||
| if len(fields) >= 2 { | ||
| // The filename is the last field | ||
| fileName := fields[len(fields)-1] | ||
| util.Slog.Debug("FilePicker: Extracted filename", "filename", fileName) | ||
|
|
||
| // Get full path by combining with current directory | ||
| if !strings.HasPrefix(fileName, "/") && !strings.HasPrefix(fileName, "~") { | ||
| // Relative path | ||
| fullPath := filepath.Join(m.filepicker.CurrentDirectory, fileName) | ||
| util.Slog.Debug("FilePicker: Constructed full path", "full_path", fullPath) | ||
|
|
||
| // Check if file exists | ||
| if _, err := os.Stat(fullPath); err != nil { | ||
| util.Slog.Error("FilePicker: File does not exist", "path", fullPath, "error", err) | ||
| } else { | ||
| util.Slog.Debug("FilePicker: File exists", "path", fullPath) | ||
| } | ||
|
|
||
| // Only update if different from current preview | ||
| if fullPath != m.previewFile { | ||
| util.Slog.Debug("FilePicker: Updating preview from navigation", "path", fullPath) | ||
| m.previewFile = fullPath | ||
| m.previewContent = m.getFilePreviewContent(fullPath) | ||
| } else { | ||
| util.Slog.Debug("FilePicker: Preview file unchanged", "path", fullPath) | ||
| } | ||
| } else { | ||
| util.Slog.Debug("FilePicker: Path is absolute or home, using as-is", "path", fileName) | ||
| if fileName != m.previewFile { | ||
| m.previewFile = fileName | ||
| m.previewContent = m.getFilePreviewContent(fileName) | ||
| } | ||
| } | ||
| } else { | ||
| util.Slog.Warn("FilePicker: Could not parse filename from line", "fields", fields) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
updatePreviewFromView parses the rendered view string — extremely fragile.
This method reverse-engineers the filepicker's rendered output by looking for > characters and splitting on whitespace to extract filenames. This will break if:
- The filepicker library changes its rendering format
- A filename contains spaces
- The cursor character appears elsewhere in the rendered output
Consider using the filepicker's public API (e.g., tracking CurrentDirectory + cursor index) rather than parsing rendered ANSI output.
🤖 Prompt for AI Agents
In `@components/file-picker.go` around lines 664 - 736, The current
updatePreviewFromView method is fragile because it parses the rendered ANSI view
to find the selected file; instead, stop reverse-engineering the output and
derive the selected item from the filepicker state (e.g., use
m.filepicker.CurrentDirectory together with the filepicker's selected
index/cursor position or the file list backing the view) rather than scanning
view strings; replace the parsing logic in updatePreviewFromView to fetch the
selected filename from the file list (or searchResults/m.filepicker selection
API), then compute fullPath (similar to current logic) and update m.previewFile
and m.previewContent via m.getFilePreviewContent only when different.
| // Store FileContents for display when expanded | ||
| m.pendingContextChips = msg.ContextChips | ||
| // Clear pending context chips | ||
| m.pendingContextChips = []util.FileContextChip{} |
There was a problem hiding this comment.
Dead code: pendingContextChips is assigned then immediately cleared.
Line 414 assigns msg.ContextChips to m.pendingContextChips, and line 416 immediately overwrites it with an empty slice. The first assignment has no effect.
🐛 Proposed fix — remove the dead assignment
- // Store FileContents for display when expanded
- m.pendingContextChips = msg.ContextChips
// Clear pending context chips
m.pendingContextChips = []util.FileContextChip{}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Store FileContents for display when expanded | |
| m.pendingContextChips = msg.ContextChips | |
| // Clear pending context chips | |
| m.pendingContextChips = []util.FileContextChip{} | |
| // Clear pending context chips | |
| m.pendingContextChips = []util.FileContextChip{} |
🤖 Prompt for AI Agents
In `@views/main-view.go` around lines 413 - 416, Remove the dead assignment where
m.pendingContextChips is set to msg.ContextChips then immediately overwritten;
delete the line "m.pendingContextChips = msg.ContextChips" so only the
intentional clearing assignment "m.pendingContextChips =
[]util.FileContextChip{}" remains, referencing m.pendingContextChips and
msg.ContextChips to locate the code.
|
From my testing, file preview and fuzzy matching should be working now. |
|
Actually I'll do a bit of refactoring tmr, the file-picker.go is growing too large |
|
@BalanceBalls, hey would love to feedback on this, are the changes okay? Were there any bugs? |
|
@gugugiyu great work! I did a little testing, works quite well for me. Will test it more next week. Currently, only 2 things I'd like to be improved:
I will try my best to review the code during the next week, there is a lot of it :) |
|
@BalanceBalls changed as needed, I've opted for the dynamic scaling style initially but yeah fullscreen seems better. Filter field now also show only when there's filter value btw. |
|
Tested again with the latest changes:
|
2 participants
An implementation of (#20):
Selection mode:
Summary by CodeRabbit
New Features
Configuration
Documentation