Skip to content

fix(app): address code audit findings — app module#68

Merged
leszek3737 merged 2 commits into
mainfrom
fi2
Jun 4, 2026
Merged

fix(app): address code audit findings — app module#68
leszek3737 merged 2 commits into
mainfrom
fi2

Conversation

@leszek3737

Copy link
Copy Markdown
Owner
  • Fix wrong MIME types: Julia .jl → text/x-julia, bat/cmd → text/x-msdos-batch, .mm → text/x-objective-c++
  • Fix case-sensitive FS mismatch in CONFIG_EXACT_NAMES (exact match on Linux, case-insensitive on macOS/Windows)
  • Fix thread leak in JobRunner Drop (single reaper thread, 5s deadline)
  • Fix shell error merging (log raw_err, return PermissionDenied over NotFound)
  • Fix FileEntryBuilder accepting empty names
  • Fix FileSize rounding edge case (while-loop for cascading promotion)
  • Cache canonical path per watcher poll, dedup events per path
  • Cache grapheme_count in TextInput (O(n) → O(1) cursor ops)
  • Box large DialogKind variants (240B → 24B stack size)
  • Bound channel in job_runner (sync_channel(128))
  • Add panel history limit (256 entries)
  • Add hotlist validation (filter empty/whitespace paths)
  • Add BufWriter in debug_log, single stderr lock
  • Add OnceLock cache for get_shell
  • Add #[must_use] on paths.rs public functions
  • Add SortMode methods and Hash derive
  • Extract helpers in user_menu (non_utf8_err, collect_body_lines)
  • Split poll_watcher_events into drain/accumulate/apply
  • Improve tests: panel_with_n helper, WatcherHarness, let/else patterns, remove duplicates

- Fix wrong MIME types: Julia .jl → text/x-julia, bat/cmd →
  text/x-msdos-batch, .mm → text/x-objective-c++
- Fix case-sensitive FS mismatch in CONFIG_EXACT_NAMES (exact match
  on Linux, case-insensitive on macOS/Windows)
- Fix thread leak in JobRunner Drop (single reaper thread, 5s deadline)
- Fix shell error merging (log raw_err, return PermissionDenied
  over NotFound)
- Fix FileEntryBuilder accepting empty names
- Fix FileSize rounding edge case (while-loop for cascading promotion)
- Cache canonical path per watcher poll, dedup events per path
- Cache grapheme_count in TextInput (O(n) → O(1) cursor ops)
- Box large DialogKind variants (240B → 24B stack size)
- Bound channel in job_runner (sync_channel(128))
- Add panel history limit (256 entries)
- Add hotlist validation (filter empty/whitespace paths)
- Add BufWriter in debug_log, single stderr lock
- Add OnceLock cache for get_shell
- Add #[must_use] on paths.rs public functions
- Add SortMode methods and Hash derive
- Extract helpers in user_menu (non_utf8_err, collect_body_lines)
- Split poll_watcher_events into drain/accumulate/apply
- Improve tests: panel_with_n helper, WatcherHarness,
  let/else patterns, remove duplicates

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sorry @leszek3737, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces various performance optimizations, refactorings, and bug fixes across the codebase, notably caching grapheme counts in TextInput, boxing large DialogKind variants to reduce stack size, caching shell paths, and batching file watcher events to reduce UI thread overhead. Feedback focuses on addressing a potential panic in ends_with_ignore_ascii_case when slicing non-char boundaries, ensuring BufWriter flushes on every write to prevent lost debug logs, removing a premature atomic optimization in directory creation, and eliminating redundant blocking disk I/O and system calls in directory tree traversal and watcher event processing.

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.

Comment thread src/app/mime.rs
Comment thread src/app/debug_log.rs Outdated
Comment thread src/app/dir_tree.rs Outdated
Comment thread src/app/watcher_sync.rs
Comment thread src/app/debug_log.rs Outdated
@greptile-apps

greptile-apps Bot commented Jun 4, 2026

Copy link
Copy Markdown

Greptile Summary

This PR addresses a broad code audit across the app module, introducing ~35 targeted fixes and improvements spanning correctness bugs, resource management, and performance micro-optimisations.

  • Watcher refactor (watcher_sync.rs): splits poll_watcher_events into drain_events / accumulate_changes / apply_panel_changes, caches canonical paths per poll to avoid repeated canonicalize() syscalls, and deduplicates file events by path via a HashMap. A logic bug is present in the Renamed handler where to matching either panel incorrectly schedules a full refresh on both panels.
  • TextInput O(1) cursor (text_input.rs): caches grapheme_count as a struct field updated incrementally on every mutation; all existing call sites that do direct .text = assignment follow the documented cursor_end() / recompute_grapheme_count() protocol, but the pub text field leaves the invariant unenforced by the type system.
  • Resource fixes: job-runner teardown drops from 3 threads to 1 polling reaper, job channel bounded at 128, shell detection cached in OnceLock, debug log wrapped in BufWriter with periodic flushing.

Confidence Score: 4/5

Safe to merge with the watcher rename-event fix applied; all other changes are well-scoped and follow existing patterns.

The watcher refactor is the highest-risk area. The Renamed event handler incorrectly fans out a DirEvent::Touch to both panels whenever either panel matches the rename target, causing the uninvolved panel to perform an unnecessary full directory read and potentially disrupting its cursor/scroll state. Every other changed file is low-risk: the TextInput cache is correct at all current call sites, the job-runner polling reaper is a clean simplification, and the boxing/MIME/config fixes are mechanical.

src/app/watcher_sync.rs — the Renamed event accumulation logic around lines 177–186

Important Files Changed

Filename Overview
src/app/watcher_sync.rs Significant refactor splitting poll_watcher_events into drain/accumulate/apply phases with per-poll canonical-path caching. Logic bug in Renamed handling: the `
src/app/types/text_input.rs Adds a cached grapheme_count field to make cursor ops O(1). All current mutation sites correctly follow the update protocol; the invariant is only documented, not enforced by the type system since text remains pub.
src/app/job_runner.rs Replaces the double-thread (reaper + join-waiter) teardown with a single polling reaper using is_finished(); bounds the job channel at 128; upgrades Ordering::Relaxed to SeqCst for cancel flag stores.
src/app/shell.rs Caches get_shell result in OnceLock (separate caches for menu/interactive on non-Windows); removes alias wrappers; fixes the TerminalRestoreGuard double-call bug by setting already_restored unconditionally before the error chain.
src/app/types/dialogs.rs Extracts five large DialogKind variants into separate structs and boxes them; all construction and match sites in the PR are updated correctly.
src/app/debug_log.rs Wraps the log file in BufWriter, flushes every 256 writes rather than every write, and locks stderr once for fallback writes; also prevents redundant create_dir_all calls with DIR_CREATED flag.
src/app/mime.rs Fixes three MIME type assignments (.jl → text/x-julia, bat/cmd → text/x-msdos-batch, .mm → text/x-objective-c++) and replaces the lowercase-then-match pattern with eq_ignore_ascii_case throughout.
src/app/types/panel.rs Caps panel history at 256 entries; refactors sync_unfiltered_selection and source_entries; adds mark_dirty helper. Uses Vec::remove(0) for history eviction (O(n)), acceptable for 256-entry cap.
src/app/types/file_entry.rs Guards FileEntryBuilder::build against empty names; fixes FileSize rounding via while-loop; caches display_modified in time_str to avoid repeated formatting.
src/app/config.rs Filters empty/whitespace hotlist entries; adds debug logging for canonicalize failures; removes an unnecessary unwrap_or_default() call.
src/app/watcher_sync/tests.rs Adds WatcherHarness and EventHarness test fixtures; replaces fragile index-based assertions with name-based lookups; adds panel_with_n helpers and let/else patterns.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[poll_watcher_events] --> B[drain_events\nup to 256 from channel]
    B --> C{events empty?}
    C -- yes --> D[return false]
    C -- no --> E[PanelCache::from_panel\nleft + right]
    E --> F[accumulate_changes\nloop over events]
    F --> G{event type}
    G -- Created/Modified --> H[event_is_panel_dir_cached\nleft? right?]
    G -- Deleted --> I[event_is_panel_dir_cached\nleft? right?]
    G -- Renamed from/to --> J[Check 'from' left/right\nCheck 'to' left/right ⚠️ OR-bug]
    G -- Overflow --> K[overflow = true]
    H --> L[AccumulatedChanges]
    I --> L
    J --> L
    K --> L
    L --> M[apply_panel_changes]
    M --> N[apply_dir_event left]
    M --> O[apply_dir_event right]
    M --> P{needs_full_refresh?}
    P -- yes --> Q[full_refresh_panels\nread_directory IO]
    P -- no --> R[apply_file_events\nper-path upsert/remove]
    Q --> S[rebuild_visible_entries]
    R --> S
    S --> T[return dirty]
Loading

Reviews (1): Last reviewed commit: "fix(app): address code audit findings — ..." | Re-trigger Greptile

Comment thread src/app/watcher_sync.rs Outdated
Comment thread src/app/types/text_input.rs
- Replace byte-slicing with char-iter in ends_with_ignore_ascii_case
- Flush debug_log after every write to prevent log loss on crash
- Remove DIR_CREATED premature AtomicBool optimization
- Remove canonicalize() calls from event_is_panel_dir_cached
- Use entry.file_type() cached from read_dir instead of symlink_metadata
- Fix rename event touching wrong panel (|| → independent ifs)
@leszek3737
leszek3737 merged commit c72e5e8 into main Jun 4, 2026
5 checks passed
@leszek3737
leszek3737 deleted the fi2 branch June 4, 2026 15:13
leszek3737 added a commit that referenced this pull request Jun 13, 2026
WS-A notify watcher:
- pair renames by cookie/path instead of FIFO pop_front
- platform-correct unwatch error handling (ENOENT/EINVAL vs
ERROR_NOT_FOUND)
- rebuild guarded state on mutex poison instead of continuing on partial
state
- bound pending_from queue with overflow handling
- reduce per-event PathBuf clones and lock cycles in the hot path

WS-B directory tree:
- push entries with read_error on metadata failure and on detected
cycles (regression #68: entries no longer silently dropped)
- return Option from file_key; skip cycle detection when identifiers are
unavailable (avoids Windows (0,0) key collision)
- size children Vec from a single dir read

WS-C watcher sync:
- dedup cached apply/remove and watcher upsert/remove via generics
- compute clean_path once per event in path_matches_any
- lazy allocation in drain_events

WS-D path/reader/cha/mod:
- strip leading separators cross-platform (Windows backslash
absolute-path bug)
- Cow-based env expansion, std::env::var fast path
- os_str_to_arc without unwrap; dedup filename conversion
- UID_CACHE poison dump+rebuild+clear_poison
- unify Cha is_* via ChaMode; document time-API access; PermBits table
- fs re-exports (FileEntry, Cha, WatchEvent); fix sequential-read doc

Tests: rename-pairing interleave, pending_from overflow, dir-tree
read_error + cycle + descent-skip, deterministic cooldown.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant