Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions .github/workflows/superfile-build-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,12 @@ jobs:
steps:
- name: Checkout repository
uses: actions/checkout@v5
- name: Setup exiftool (only on linux)
- name: Setup dependencies (only on linux)
if: runner.os == 'Linux'
run: sudo apt-get update && sudo apt-get install -y exiftool
run: |
sudo apt-get update
sudo apt-get install -y exiftool
curl -sS https://raw.githubusercontent.com/ajeetdsouza/zoxide/main/install.sh | bash

- name: Set up Go
uses: actions/setup-go@v5
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ require (
github.com/charmbracelet/x/ansi v0.9.3
github.com/fatih/color v1.18.0
github.com/hymkor/trash-go v0.2.0
github.com/lazysegtree/go-zoxide v0.0.1
github.com/lazysegtree/go-zoxide v0.1.0
github.com/lithammer/shortuuid v3.0.0+incompatible
github.com/muesli/termenv v0.16.0
github.com/reinhrst/fzf-lib v0.9.0
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,8 @@ github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE=
github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
github.com/lazysegtree/go-zoxide v0.0.1 h1:PG8asMMqetoxXXfLGTvgzbWHVGybioeJpGm91mhbNoc=
github.com/lazysegtree/go-zoxide v0.0.1/go.mod h1:C1K2SDM4iHkQeFVrWBiRX3tU9jpAz6BrnoHQYSiHZl0=
github.com/lazysegtree/go-zoxide v0.1.0 h1:gL11AWS9fJDuB7FYxsh+ohf8ozMXeK31E/PC6f9jNBc=
github.com/lazysegtree/go-zoxide v0.1.0/go.mod h1:C1K2SDM4iHkQeFVrWBiRX3tU9jpAz6BrnoHQYSiHZl0=
github.com/lithammer/shortuuid v3.0.0+incompatible h1:NcD0xWW/MZYXEHa6ITy6kaXN5nwm/V115vj2YXfhS0w=
github.com/lithammer/shortuuid v3.0.0+incompatible/go.mod h1:FR74pbAuElzOUuenUHTK2Tciko1/vKuIKS9dSkDrA4w=
github.com/lucasb-eyer/go-colorful v1.2.0 h1:1nnpGOrhyZZuNyfu1QjKiUICQ74+3FNCN69Aj6K7nkY=
Expand Down
4 changes: 2 additions & 2 deletions gomod2nix.toml
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,8 @@ schema = 3
version = "v1.16.3"
hash = "sha256-dU0OgO5afQ1z5s83Y3w8Bg0ftvg+ikWbktUACEgY3OQ="
[mod."github.com/lazysegtree/go-zoxide"]
version = "v0.0.1"
hash = "sha256-pBZ6tOqhYe2ogHyFmP/7R4hc4yfJ7I9hk2D+gT14v4o="
version = "v0.1.0"
hash = "sha256-PKEV+zCKf/7MsFAMMctL/pFSnEsri1yS8Bzxcj1dLWE="
[mod."github.com/lithammer/shortuuid"]
version = "v3.0.0+incompatible"
hash = "sha256-dD6ArCHGnpo84RBy6SI2kUjMqHS4IZU+K+DgAgmRakY="
Expand Down
38 changes: 21 additions & 17 deletions src/internal/config_function.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import (
"reflect"
"runtime"

"github.com/lazysegtree/go-zoxide"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai

Do a strict review for this PR

Check if any refactoring is breaking existing behaviour.

Check if any manual testing or unit testing is must have to ensure no regressions.

Check for any bugs, over-engineering or changes that will reduce code readability/maintainability in the long term

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the full diff for PR #1042 to understand all changes
echo "=== Getting full PR diff ==="
gh pr diff 1042

echo -e "\n=== Checking for any zoxide-related imports in the codebase ==="
rg -n "zoxide" --type go

echo -e "\n=== Checking for any references to zoxidelib in the codebase ==="
rg -n "zoxidelib" --type go

echo -e "\n=== Looking for any new imports that might replace the removed one ==="
rg -n "go-zoxide" --type go

Length of output: 16294


🏁 Script executed:

#!/bin/bash
# Check if there are any build issues by examining go.mod and dependencies
echo "=== Checking go.mod for zoxide dependency ==="
cat go.mod | grep -i zoxide

echo -e "\n=== Checking if there are any other inconsistent import usages ==="
echo "Files using 'zoxide' without alias:"
rg -l 'import.*"github.com/lazysegtree/go-zoxide"$' --type go

echo -e "\nFiles using 'zoxidelib' alias:"
rg -l 'zoxidelib.*"github.com/lazysegtree/go-zoxide"' --type go

echo -e "\n=== Checking for any potential issues with the test setup ==="
echo "Checking test file structure and imports:"
rg -n -A5 -B5 "func TestZoxide" src/internal/model_test.go

echo -e "\n=== Checking if the new GetResults method is properly defined ==="
rg -n -A3 -B3 "GetResults" src/internal/ui/zoxide/

echo -e "\n=== Checking the zoxide modal close fix ==="
rg -n -A5 -B5 "m.Close()" src/internal/ui/zoxide/model.go

Length of output: 2946


@lazysegtree

After conducting a thorough strict review of PR #1042, here's my comprehensive analysis:

Overall Assessment: This is a well-executed refactoring with significant improvements

Changes Analysis:

  1. Import Consistency Fix

    • Makes zoxidelib alias consistent across all files
    • Previously config_function.go was the only file using bare zoxide import
    • No breaking changes - purely cosmetic improvement
  2. Comprehensive Test Coverage Addition

    • Adds 70 lines of robust zoxide functionality tests
    • Tests both enabled/disabled zoxide scenarios
    • Proper OS-specific handling (skips on non-Linux platforms)
    • Uses isolated temporary directories for test data
    • Tests modal interaction, navigation, and search functionality
  3. Bug Fix

    • Fixes missing m.Close() call after zoxide modal confirmation
    • Previously modal remained open after selection - now properly closes
  4. Testing Infrastructure

    • Adds defaultTestModelWithZClient() helper function
    • Adds GetResults() method for test verification
    • Well-structured test isolation and cleanup

No Regressions Identified:

  • Import alias change doesn't affect runtime behavior
  • All function signatures remain identical
  • Dependency is properly declared in go.mod
  • No breaking API changes

Recommended Testing:

While the code changes are safe, manual testing would be valuable to verify:

  • Zoxide modal opening/closing behavior
  • Directory navigation via zoxide search
  • Keyboard interaction flow (especially the confirm action fix)
  • Different zoxide configuration scenarios

Code Quality:

  • Maintains excellent code readability
  • No over-engineering detected
  • Follows existing patterns and conventions
  • Test coverage significantly improved

Verdict: This PR improves code consistency, adds missing test coverage, and fixes an actual bug. No breaking changes or regressions identified.


🧠 Learnings used
Learnt from: lazysegtree
PR: yorukot/superfile#979
File: src/internal/common/predefined_variable.go:47-47
Timestamp: 2025-08-06T10:27:20.420Z
Learning: lazysegtree specifically asked for a "strict review to find bad stuff" in PR reviews, indicating he wants comprehensive analysis that identifies all potential issues regardless of scope, not just issues within the current PR's scope.

Learnt from: lazysegtree
PR: yorukot/superfile#950
File: src/internal/ui/metadata/README.md:7-7
Timestamp: 2025-07-24T03:46:29.516Z
Learning: lazysegtree prefers to defer comprehensive unit testing to separate PRs when the current PR has grown too large, maintaining focus on the primary refactoring objectives while tracking testing requirements in dedicated GitHub issues.

Learnt from: lazysegtree
PR: yorukot/superfile#979
File: src/internal/common/predefined_variable.go:47-47
Timestamp: 2025-08-06T10:54:31.444Z
Learning: When lazysegtree says a review is "inaccurate and pre-existing issues. Not much useful," it means I should focus specifically on bugs or problems introduced by the current PR changes, not architectural concerns or code quality issues that were already present. The refactoring work in superfile PRs is generally well-implemented and should be evaluated on whether the specific changes work correctly, not on broader codebase architecture.

Learnt from: lazysegtree
PR: yorukot/superfile#967
File: src/internal/key_function.go:45-47
Timestamp: 2025-08-02T11:47:07.713Z
Learning: lazysegtree prefers to track technical debt and architectural improvements in dedicated GitHub issues when they are identified during PR reviews but are beyond the scope of the current PR, particularly for complex refactoring needs like input handling architecture that would require significant changes.

Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/handle_file_operations.go:567-570
Timestamp: 2025-07-27T08:49:09.687Z
Learning: lazysegtree prefers to defer technical debt issues like model mutation concerns to later PRs when the current PR has already grown too large, maintaining focus on the primary objectives while acknowledging the need to track such issues for future work.

Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/model_render.go:329-341
Timestamp: 2025-08-29T14:11:21.380Z
Learning: lazysegtree prefers to defer help menu rendering optimizations and other technical debt improvements to separate GitHub issues when the current PR scope has grown too large, maintaining focus on the primary refactoring objectives while tracking performance improvements for future work.

Learnt from: lazysegtree
PR: yorukot/superfile#781
File: src/internal/model_render.go:254-256
Timestamp: 2025-04-28T04:27:33.074Z
Learning: When race conditions or other issues that are unrelated to the current PR's focus are identified during review, lazysegtree prefers creating separate GitHub issues to track them rather than addressing them in the current PR. This helps maintain PR focus and scope.

Learnt from: lazysegtree
PR: yorukot/superfile#781
File: src/internal/ui/sidebar/render.go:46-48
Timestamp: 2025-04-28T03:48:46.327Z
Learning: The user (lazysegtree) prefers to keep PRs focused and manageable in size, sometimes intentionally leaving TODO comments to track minor issues for later PRs rather than addressing everything at once.

Learnt from: lazysegtree
PR: yorukot/superfile#970
File: src/internal/ui/processbar/model.go:11-26
Timestamp: 2025-08-02T17:00:37.262Z
Learning: lazysegtree prefers to track memory leak issues and feature enhancements via dedicated GitHub issues when identified during PR reviews but are beyond the current PR scope, particularly for processbar-related improvements like cleanup mechanisms and keyboard shortcuts.

Learnt from: lazysegtree
PR: yorukot/superfile#979
File: src/internal/common/predefined_variable.go:47-47
Timestamp: 2025-08-06T10:54:31.444Z
Learning: When reviewing PRs, especially large refactoring ones, focus specifically on issues introduced by the changes rather than flagging pre-existing architectural problems or code smells that were already present before the PR. Distinguish between new bugs versus existing technical debt.

Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.

Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.

Learnt from: lazysegtree
PR: yorukot/superfile#969
File: src/internal/key_function.go:40-40
Timestamp: 2025-08-03T09:34:55.721Z
Learning: lazysegtree emphasizes proper dependency direction in software architecture, preferring that low-level components (like modal handlers) should not depend on high-level components (like the main model object). He also prioritizes performance considerations, noting that creating objects on every keypress in hot code paths like key handling is inefficient and should be avoided.

Learnt from: lazysegtree
PR: yorukot/superfile#985
File: src/internal/model.go:0-0
Timestamp: 2025-08-11T01:49:30.040Z
Learning: lazysegtree prefers maintaining code correctness through proper design and invariants rather than adding defensive bounds checks at every slice access point, viewing such defensive programming as "duct taping" that can mask actual bugs instead of fixing them at their source.

Learnt from: lazysegtree
PR: yorukot/superfile#973
File: src/internal/ui/processbar/model_update.go:7-27
Timestamp: 2025-08-03T14:49:31.221Z
Learning: lazysegtree prefers to keep test-only code simple without adding production-level concerns like goroutine synchronization, cancellation contexts, or complex lifecycle management, even when such patterns might prevent potential issues, since the complexity isn't justified for test utilities.

Learnt from: lazysegtree
PR: yorukot/superfile#924
File: src/internal/model_test.go:147-168
Timestamp: 2025-08-09T02:52:22.516Z
Learning: In superfile tests, lazysegtree prefers tests that accurately mimic the actual production behavior, including file operations like creating and deleting the LastDir file, rather than isolating such operations to temporary locations when the real behavior involves modifying/deleting those specific files.

zoxidelib "github.com/lazysegtree/go-zoxide"

"github.com/yorukot/superfile/src/internal/ui/processbar"
"github.com/yorukot/superfile/src/internal/ui/rendering"
Expand All @@ -25,7 +25,7 @@ import (

// This is the only usecase of named returns, distinguish between multiple return values
func initialConfig(firstFilePanelDirs []string) (toggleDotFile bool, //nolint: nonamedreturns // See above
toggleFooter bool, zClient *zoxide.Client) {
toggleFooter bool, zClient *zoxidelib.Client) {
// Open log stream
file, err := os.OpenFile(variable.LogFile, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0666)

Expand Down Expand Up @@ -73,7 +73,7 @@ func initialConfig(firstFilePanelDirs []string) (toggleDotFile bool, //nolint: n
}

if common.Config.ZoxideSupport {
zClient, err = zoxide.New()
zClient, err = zoxidelib.New()
if err != nil {
slog.Error("Error initializing zoxide client", "error", err)
}
Expand All @@ -90,27 +90,31 @@ func initialConfig(firstFilePanelDirs []string) (toggleDotFile bool, //nolint: n
return toggleDotFile, toggleFooter, zClient
}

func updateFirstFilePanelDirs(firstFilePanelDirs []string, cwd string, zClient *zoxide.Client) {
func updateFirstFilePanelDirs(firstFilePanelDirs []string, cwd string, zClient *zoxidelib.Client) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ New issue: Deep, Nested Complexity
updateFirstFilePanelDirs has a nested complexity depth of 4, threshold = 4

Suppress

for i := range firstFilePanelDirs {
if firstFilePanelDirs[i] == "" {
firstFilePanelDirs[i] = common.Config.DefaultDirectory
}
originalPath := firstFilePanelDirs[i]
firstFilePanelDirs[i] = utils.ResolveAbsPath(cwd, firstFilePanelDirs[i])

if common.Config.ZoxideSupport && zClient != nil {
path, err := zClient.Query(firstFilePanelDirs[i])
if err == nil && path != "" {
firstFilePanelDirs[i] = path
} else {
slog.Error("Zoxide execution error", "path", path, "error", err)
firstFilePanelDirs[i] = utils.ResolveAbsPath(cwd, firstFilePanelDirs[i])
}
} else {
firstFilePanelDirs[i] = utils.ResolveAbsPath(cwd, firstFilePanelDirs[i])
}
// In case of unexpected path error, fallback to home dir
if _, err := os.Stat(firstFilePanelDirs[i]); err != nil {
slog.Error("cannot get stats for firstFilePanelDir", "error", err)
firstFilePanelDirs[i] = variable.HomeDir
// In case the path provided did not exist, use zoxide query
// else, fallback to home dir
if common.Config.ZoxideSupport && zClient != nil {
path, err := zClient.Query(originalPath)
slog.Error("Zoxide execution error", "path", path, "originalPath", originalPath, "error", err)

if err == nil && path != "" {
firstFilePanelDirs[i] = path
} else {
slog.Error("Zoxide execution error", "path", path, "originalPath", originalPath, "error", err)
firstFilePanelDirs[i] = variable.HomeDir
}
} else {
firstFilePanelDirs[i] = variable.HomeDir
}
}
}
}
Expand Down
68 changes: 68 additions & 0 deletions src/internal/model_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@ import (
"fmt"
"os"
"path/filepath"
"runtime"
"testing"

zoxidelib "github.com/lazysegtree/go-zoxide"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

Expand Down Expand Up @@ -245,3 +247,69 @@ func TestChooserFile(t *testing.T) {
})
}
}

func TestZoxide(t *testing.T) {
zoxideDataDir := t.TempDir()
zClient, err := zoxidelib.New(zoxidelib.WithDataDir(zoxideDataDir))
if err != nil {
if runtime.GOOS != utils.OsLinux {
t.Skipf("Skipping zoxide tests in non-Linux because zoxide client cannot be initialized")
} else {
t.Fatalf("zoxide initialization failed")
}
}

originalZoxideSupport := common.Config.ZoxideSupport
defer func() {
common.Config.ZoxideSupport = originalZoxideSupport
}()

curTestDir := filepath.Join(testDir, "TestZoxide")
dir1 := filepath.Join(curTestDir, "dir1")
dir2 := filepath.Join(curTestDir, "dir2")
dir3 := filepath.Join(curTestDir, "dir3")
utils.SetupDirectories(t, curTestDir, dir1, dir2, dir3)

t.Run("Zoxide tracking and navigation", func(t *testing.T) {
common.Config.ZoxideSupport = true
m := defaultTestModelWithZClient(zClient, dir1)

err := m.updateCurrentFilePanelDir(dir2)
require.NoError(t, err, "Failed to navigate to dir2")
assert.Equal(t, dir2, m.getFocusedFilePanel().location, "Should be in dir2 after navigation")

err = m.updateCurrentFilePanelDir(dir3)
require.NoError(t, err, "Failed to navigate to dir3")
assert.Equal(t, dir3, m.getFocusedFilePanel().location, "Should be in dir3 after navigation")

TeaUpdateWithErrCheck(m, utils.TeaRuneKeyMsg(common.Hotkeys.OpenZoxide[0]))
assert.True(t, m.zoxideModal.IsOpen(), "Zoxide modal should open when pressing 'z' key")

// Type "dir2" to search for it
for _, char := range "dir2" {
TeaUpdateWithErrCheck(m, utils.TeaRuneKeyMsg(string(char)))
}

results := m.zoxideModal.GetResults()
assert.GreaterOrEqual(t, len(results), 1, "Should have at least 1 directory found by zoxide UI search")

resultPaths := make([]string, len(results))
for i, result := range results {
resultPaths[i] = result.Path
}
assert.Contains(t, resultPaths, dir2, "dir2 should be found by zoxide UI search")

// Press enter to navigate to dir2
TeaUpdateWithErrCheck(m, utils.TeaRuneKeyMsg(common.Hotkeys.ConfirmTyping[0]))
assert.False(t, m.zoxideModal.IsOpen(), "Zoxide modal should close after navigation")
assert.Equal(t, dir2, m.getFocusedFilePanel().location, "Should navigate back to dir2 after zoxide selection")
})

t.Run("Zoxide disabled shows no results", func(t *testing.T) {
common.Config.ZoxideSupport = false
m := defaultTestModelWithZClient(zClient, dir1)

TeaUpdateWithErrCheck(m, utils.TeaRuneKeyMsg(common.Hotkeys.OpenZoxide[0]))
assert.True(t, m.zoxideModal.IsOpen(), "Zoxide modal should open even when ZoxideSupport is disabled")
})
}
8 changes: 8 additions & 0 deletions src/internal/test_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"time"

tea "github.com/charmbracelet/bubbletea"
zoxidelib "github.com/lazysegtree/go-zoxide"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

Expand All @@ -26,6 +27,13 @@ func defaultTestModel(dirs ...string) *model {
return m
}

func defaultTestModelWithZClient(zClient *zoxidelib.Client, dirs ...string) *model {
m := defaultModelConfig(false, false, false, dirs, zClient)
m.disableMetatdata = true
_, _ = TeaUpdate(m, tea.WindowSizeMsg{Width: 2 * common.MinimumWidth, Height: 2 * common.MinimumHeight})
return m
}

// Helper function to setup panel mode and selection
func setupPanelModeAndSelection(t *testing.T, m *model, useSelectMode bool, itemName string, selectedItems []string) {
t.Helper()
Expand Down
1 change: 1 addition & 0 deletions src/internal/ui/zoxide/model.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ func (m *Model) HandleUpdate(msg tea.Msg) (common.ModelAction, tea.Cmd) {
switch {
case slices.Contains(common.Hotkeys.ConfirmTyping, msg.String()):
action = m.handleConfirm()
m.Close()
case slices.Contains(common.Hotkeys.CancelTyping, msg.String()):
m.Close()
case slices.Contains(common.Hotkeys.ListUp, msg.String()):
Expand Down
4 changes: 4 additions & 0 deletions src/internal/ui/zoxide/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,7 @@ func (m *Model) SetMaxHeight(maxHeight int) {
}
m.maxHeight = maxHeight
}

func (m *Model) GetResults() []zoxidelib.Result {
return m.results
}
Loading