refactor(tests): fix bugs, remove tech debt, add missing coverage

[leszek3737]

• Fix false positive tests (unfiltered_entries not set in search/selection)
• Fix buffer_to_string missing newline separator between rows
• Replace SystemTime::now() with UNIX_EPOCH in test builders
• Add Cha::regular_file(size) constructor, TestEntry validation
• Replace use crate::* with explicit imports in all 14 test files
• Remove duplicate tests, extract shared helpers (dispatch_key, TestFixture, etc.)
• Replace magic numbers with constants (TERMINAL_HEIGHT, VISIBLE_HEIGHT, TERMINAL_WIDTH)
• Fix misleading test names (compare, history, selection, user_menu)
• Add missing test coverage: viewer (scroll/page/search/hex/close), history (Home/End/dedup), overwrite (ExtractArchive/CreateArchive), pickers (early return/Home/End/ArchiveMenu), selection (toggle off/right panel/edge cases)
• Fix shell::push_history non-consecutive dedup
• Add CompareMode::label() method
• Remove unnecessary tempfile operations from memory-only tests
• Fix inconsistent import paths (app:: vs lc::app::)
• Fix menu_rename test to actually verify rename via FS

Summary by Sourcery

Refine test utilities and interaction tests across selection, viewer, history, overwrite, pickers, menus, key handling, and search while introducing small core helpers for file metadata, comparison labeling, and shell history.

Bug Fixes:

• Correct test helpers to build regular file metadata deterministically and fix buffer-to-string rendering to preserve line structure.
• Update history handling to deduplicate non-consecutive commands correctly and ensure overwrite, rename, and menu tests exercise real filesystem behavior.
• Align selection, keybinding, picker, and user menu tests with actual behavior, preventing false positives around modes, navigation, and empty states.

Enhancements:

• Introduce constants and shared helpers for dispatching events, key handling, and tree entry creation to reduce duplication and stabilize tests.
• Add a CompareMode::label helper and use it in directory comparison flows for clearer, centralized labeling.
• Simplify tests by using panel set_entries APIs and removing unnecessary tempfile and on-disk operations from purely in-memory scenarios.

Tests:

• Expand coverage for viewer scrolling, paging, search, hex toggling, and close behavior.
• Add picker and history tests for Home/End navigation, early returns when not in picker mode, archive menu navigation, and hotlist status messaging.
• Strengthen selection, keybinding, key event dispatch, search, overwrite, and menu rename tests to cover edge cases such as dotdot entries, wrapping, empty panels, and additional pending actions.

[sourcery-ai]

Reviewer's Guide

Extensive refactor of integration-style tests to reduce tech debt, fix several subtle behavioral bugs (search/selection, buffer rendering, history dedup, overwrite handling), and add missing coverage for viewer, pickers, history, overwrite, selection, compare, and user menu flows, plus a few small core changes (Cha::regular_file, CompareMode::label, shell history behavior).

Sequence diagram for updated shell::push_history behavior

sequenceDiagram
    actor User
    participant Shell as shell
    participant State as AppState

    User->>Shell: push_history(state, cmd)
    Shell->>Shell: cmd.trim().is_empty()
    alt [cmd is empty]
        Shell-->>User: return
    else [cmd is non_empty]
        Shell->>State: command_history.retain(entry != cmd)
        Shell->>State: command_history.push_back(cmd.to_string())
        Shell->>Shell: command_history.len() > MAX_HISTORY
        alt [len > MAX_HISTORY]
            Shell->>State: command_history.pop_front()
        end
        Shell-->>User: return
    end

Loading

File-Level Changes

Change	Details	Files
Standardize test helpers and panel setup APIs to reduce duplication and use realistic panel state paths/entries.	Introduce constants for terminal dimensions and visible height in tests and use them instead of magic numbers. Add helper functions for dispatching events/keys in tests (dispatch_test_event, dispatch_key) and for constructing dummy tree entries. Update tests to use PanelState::set_entries and helper TestEntry builder instead of directly mutating listing.entries or using populate_panel, removing populate_panel. Tighten TestEntry::build to validate non-empty names and use Cha::regular_file/dummy_dir instead of ad-hoc Cha initialization.	src/tests/helpers.rs src/tests/keyevents.rs src/tests/keybinds.rs src/tests/selection.rs src/tests/compare.rs src/tests/viewer.rs src/tests/search.rs src/tests/menu.rs
Fix selection, navigation, and key handling tests and add coverage for edge cases in both panels.	Replace direct calls to handle_normal_mode with dispatch_key in selection/keybind tests, removing now-unused viewer/job plumbing and terminal height arguments. Use PanelState::set_entries instead of assignment to listing.entries across selection/keybind tests so selection stats and caches behave correctly. Add tests for Insert key selection behavior, shift-selection wrapping, skipping '..', and operating on the right panel. Ensure selected_or_current_paths uses selection correctly and handles '..' and empty/degenerate panels without panicking.	src/tests/selection.rs src/tests/keybinds.rs
Improve picker behavior and coverage (history, hotlist, archive menu, user menu) including boundary navigation and early-return paths.	Refactor picker tests to construct AppState directly with directory_hotlist, command_history, and mode instead of using now-removed hotlist_set and default AppState wiring. Add tests for picker behavior on empty/single-item lists, early return when not in ListPicker mode, and Home/End key handling for history and hotlist pickers. Add coverage and behavior checks for ArchiveMenu picker: Esc back to Normal, navigation clamping (no wrap), Home/End behavior. Extend user menu picker tests to verify non-wrapping navigation, Enter dismissing into a Confirm dialog, and error/ok cases when opening menu files via menu mode and F2 key paths.	src/tests/pickers.rs src/tests/history.rs src/tests/user_menu.rs
Simplify key event dispatch tests and ensure proper handling of repeat/destructive keys and mouse/resize events.	Introduce dispatch_test_event helper that wraps dispatch_event with default viewer/job/size wiring, and update keyevents tests to use it. Switch keyevents tests from populate_panel to PanelState::set_entries + TestEntry, reducing filesystem usage where not needed. Verify that key repeat events update navigation and text input but ignore destructive operations, and that unhandled events return false while resize returns true.	src/tests/keyevents.rs src/tests/helpers.rs
Expand viewer tests to cover scrolling, paging, search, hex-mode toggling, and closing via Esc while preserving previous mode.	Add helper create_test_file for generating temporary files with specific content for viewer tests. Add tests verifying scroll_up/down and page_up/down clamp to valid ranges based on file length. Add tests for viewer search query behavior, cycling through matches with next/prev, and clearing search when query is empty. Add tests for toggling between text and hex ViewMode and ensuring viewer close via Esc returns to previous AppMode and clears viewer/image loaders.	src/tests/viewer.rs src/tests/helpers.rs
Adjust compare directory feature tests to use in-memory TestEntry and new CompareMode::label API, while broadening coverage.	Refactor compare tests to rely on TestEntry entries (without actual filesystem writes) where possible and simplify path setup. Add compare_quick test for empty directories to assert that UI still shows a summary dialog. Introduce CompareMode::label method and use it from compare_directories to generate mode-specific labels, updating tests to iterate over CompareMode::ALL and assert dialog labels accordingly. Remove redundant compare mode picker tests that hard-coded mode names now covered via label() and ALL.	src/tests/compare.rs src/app/types/modes.rs src/input/pickers.rs
Tighten search behavior and coverage including filter application and handling of dirty unfiltered caches and empty panels.	Refactor search tests to use shared TERMINAL_HEIGHT constant and to focus on cursor preservation, refreshing when unfiltered cache is dirty, and restoring unfiltered entries when clearing filters. Add apply_search_filter tests that ensure filters are applied against unfiltered_entries and that non-matching filters clear the visible entries list. Add guards for empty-panel search mode flows to ensure Esc/char input leave AppMode in a sane state without panics.	src/tests/search.rs src/tests/helpers.rs
Clarify overwrite-check behavior for copy/move/delete/archive operations and extend test coverage.	Rename several overwrite tests for clarity (move conflict / same-file / overwrite / delete cases) and update them to use byte string writes consistently. Add tests confirming that ExtractArchive and CreateArchive pending actions never report overwrite conflicts via dialogs::check_overwrite_conflict. Minor tweaks to test setup to use enum variants from the local PendingAction type directly (with use/import changes).	src/tests/overwrite.rs
Fix shell history behavior to deduplicate non-consecutive entries by value while still enforcing max history length and add regression tests.	Change push_history to retain all entries not equal to the new cmd, then push cmd to the back, instead of only deduplicating the last entry. Keep max history trimming behavior (pop_front when exceeding MAX_HISTORY). Add tests verifying that re-issuing an older command moves it to the end and removes the earlier occurrence while preserving intervening commands.	src/app/shell.rs src/tests/history.rs
Improve user menu menu_rename flow test to assert actual filesystem rename via dialog handling.	Update menu_rename test to configure a real file with size>0 using TestEntry, so rename logic considers it a valid file. After opening the rename dialog via menu handling, send Enter through handle_dialog to perform the rename, then assert that the old path is gone and the new path exists on disk. Use test_terminal and ratatui::Size consistent with other tests.	src/tests/menu.rs
Add Cha::regular_file helper and use it from tests to avoid SystemTime::now dependency and better model file metadata.	Add Cha::regular_file(size) constructor that sets regular-file mode, size, timestamps at UNIX_EPOCH, and basic uid/gid/dev/nlink defaults. Update TestEntry::build to call Cha::regular_file for size>0, and Cha::dummy_dir otherwise, eliminating ad-hoc mode/mtime/btime setup and SystemTime::now usage in tests.	src/fs/cha.rs src/tests/helpers.rs
Miscellaneous cleanups: imports, buffer rendering helper, and GitNexus metadata counts.	Replace wildcard crate imports in tests with explicit lc::app / lc::ui paths and targeted function imports like handle_alt_keys, handle_navigation_keys, launch_editor, file_name_str, etc. Fix buffer_to_string helper to insert newline separators between rows while walking buffer cells, matching how content is rendered on screen. Update AGENTS.md and CLAUDE.md GitNexus index metadata counts to reflect new symbols/relationships/flows after refactor. Remove unused hotlist persistence roundtrip test and other obsolete tests tied to old APIs.	src/tests/helpers.rs src/tests/misc.rs AGENTS.md CLAUDE.md src/tests/pickers.rs

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've left some high level feedback:

• The new shell::push_history implementation uses retain on every call, which is O(n) and shifts all elements; if history grows large this may be unnecessarily expensive—consider scanning from the back and removing only the first matching entry (or using a small capped Vec) to keep dedup semantics while reducing per-call cost.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The new `shell::push_history` implementation uses `retain` on every call, which is O(n) and shifts all elements; if history grows large this may be unnecessarily expensive—consider scanning from the back and removing only the first matching entry (or using a small capped Vec) to keep dedup semantics while reducing per-call cost.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[gemini-code-assist]

Code Review

This pull request refactors command history deduplication to move non-consecutive duplicate commands to the end of the history list, introduces a label helper method for CompareMode, and adds a regular_file constructor to Cha. Additionally, it extensively refactors the test suite to use cleaner helper patterns, removes redundant test setups, and adds comprehensive unit tests covering history navigation, viewer scrolling/searching, archive overwrite checks, and user menu picker boundaries. As there are no review comments, no additional feedback is provided.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[greptile-apps]

Greptile Summary

This PR refactors the test suite across 14 test files: replacing use crate::* with explicit imports, migrating direct listing.entries assignments to panel.set_entries() (which correctly populates unfiltered_entries), extracting shared helpers, and adding missing coverage for viewer, history, pickers, overwrite, and selection edge cases. It also ships three small production fixes — a corrected buffer_to_string that inserts row separators, a new Cha::regular_file(size) constructor with deterministic UNIX_EPOCH timestamps, and CompareMode::label() to eliminate a duplicate match block.

• push_history semantics changed: previously only consecutive duplicates were skipped; now all prior occurrences of a command are removed before the command is appended, giving true MRU-ordered dedup. The new history_dedup_non_consecutive_moves_to_end test documents and verifies this behavior.
• buffer_to_string bug fixed: the old implementation concatenated every cell symbol into one flat string without row separators, making line-level assertions impossible; the new version inserts \ between rows.
• False-positive selection/search tests fixed: tests that set listing.entries directly without populating unfiltered_entries were silently bypassing the filtered-listing code path; migrating to set_entries() corrects this.

Confidence Score: 4/5

Safe to merge. All production-code changes are small and well-tested; the test refactoring removes real false positives and adds meaningful coverage.

The only finding is a design quirk in the new dispatch_test_event helper: it creates a local viewer and job that are dropped on return, so any state set through those pointers during event dispatch is silently lost. Existing tests avoid this by calling viewer handlers directly, but the contract is invisible to future test authors. Everything else — the push_history dedup change, Cha::regular_file, CompareMode::label, and the bulk test migration — is clean and correct.

src/tests/helpers.rs — the dispatch_test_event function discards viewer and job state; worth a doc comment before the function grows more callers.

Important Files Changed

Filename	Overview
src/app/shell.rs	Semantics of push_history changed from consecutive-dedup to global-dedup-with-move-to-end; behaviorally correct and fully covered by the new history_dedup_non_consecutive_moves_to_end test.
src/app/types/modes.rs	Adds CompareMode::label() helper to centralise mode name strings; replaces an inline match in pickers.rs. Clean addition.
src/fs/cha.rs	Adds Cha::regular_file(size) constructor with deterministic UNIX_EPOCH timestamps; mode 0o100644 is correct for a regular file.
src/tests/helpers.rs	Good: fixes buffer_to_string to insert row separators, adds dispatch_key/dummy_tree_entries helpers, replaces magic numbers with constants. dispatch_test_event silently drops viewer/job state (see comment).
src/tests/viewer.rs	New coverage for scroll/page/search/hex-toggle/close is correct; trailing-newline handling by compute_line_offsets ensures the line_count=5 assumption in viewer_scroll_up_down is valid.
src/tests/selection.rs	Correctly migrated to set_entries(); new edge-case tests (dotdot skip, wrap, right-panel, toggle-off) look correct.
src/tests/compare.rs	Uses direct listing.entries assignment (not set_entries) intentionally; correct because compare_directories and apply_marks both fall back to entries when unfiltered_entries is empty.
src/tests/menu.rs	menu_rename_confirms_and_renames_file now actually calls handle_dialog and verifies the rename on the filesystem, fixing a previously incomplete test.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[push_history cmd] --> B{empty cmd?}
    B -- Yes --> C[return early]
    B -- No --> D[retain: remove all prior occurrences]
    D --> E[push_back cmd]
    E --> F{len over MAX_HISTORY?}
    F -- Yes --> G[pop_front oldest]
    F -- No --> H[done - MRU at end]
    G --> H

    subgraph PanelSetup["Panel Setup in Tests"]
        P1[set_entries fills both entries and unfiltered_entries]
        P2[Direct listing.entries assignment leaves unfiltered_entries empty]
        P1 --> P3[Correct for navigation and selection tests]
        P2 --> P4[Correct only where fallback logic handles empty unfiltered_entries]
    end

Loading

_{Reviews (1): Last reviewed commit: "refactor(tests): fix bugs, remove tech d..." | Re-trigger Greptile}

[greptile-apps]

dispatch_test_event silently discards viewer and job state

viewer and job are created as locals inside the function and dropped on return. Any state changes that dispatch_event writes through those pointers (e.g. F3 opening a ViewerState, or background jobs being spawned) are lost without error. A caller trying to verify that a key event results in a viewer being opened will silently see None and the test will behave as a false negative. The existing tests avoid this pitfall by calling viewer-mode handlers directly (e.g. handle_viewer_mode in viewer_close_via_escape), but this is a non-obvious contract that is easy to violate in future tests.