chore: debt cleanup — app layer, keymap, CI tooling

[leszek3737]

Summary

Maintainability debt cleanup (audit PR-18). No behavior changes. All CI gates pass: cargo fmt, cargo clippy --locked --all-targets -- -D warnings, cargo test --locked (302 passed), cargo build --release --locked.

Fixed (14 findings)

CI workflows

• opencode.yml 0.0.4 #3 — Added timeout-minutes: 10 to the review job.
• opencode.yml 0.0.5 #4 — Removed dead issue_comment / pull_request_review_comment triggers (the job-level if: github.event_name == 'pull_request' guaranteed they could never fire) and the now-redundant guard.
• release.yml refactor: ownership, perf, error handling, domain extraction, clippy … #5 — Added concurrency: release-${{ github.ref }} group to prevent racing release runs on retags.
• release.yml Harden file operations and improve terminal UI reliability #6 — Added timeout-minutes: 30 (build) and timeout-minutes: 10 (release).
• rust.yml 0.0.4 #3 — Added explicit shell: bash to all run: steps.
• rust.yml 0.0.5 #4 — Added concurrency: rust-${{ github.ref }} with cancel-in-progress: true.

Cargo.toml

• feat: add centralized theme module with MC-style blue background #2 — Added panic = "abort" to [profile.release] (no catch_unwind in production code; test-only usage runs under dev profile).
• 0.0.4 #3 — Broadened exclude to include docs/.
• 0.0.5 #4 — Documented the FSEvents-only macOS notify rationale (kqueue FD-per-dir is slow on large trees).
• refactor: ownership, perf, error handling, domain extraction, clippy … #5 — Documented MSRV rationale (edition 2024 + let-chains).

App types / keymap

• app_state.rs 0.0.8 - Fix dialog/popup rendering, Implement shift-selection for contiguous file ranges #7 — Derived Default on MenuSource (Global), replaced UiState's 22-line hand-written Default with a derive.
• keymap.rs 0.0.9 - Add async file operations, file watching, and UI workflow improvements #8 — Added keybindings_are_grouped_by_mode contiguity test: fails if a binding is added out of mode-group order, preventing duplicated/interspersed headers in help output.
• modes.rs feat: add centralized theme module with MC-style blue background #2 — Improved the CompareMode::ALL drift-guard comment (variant_count is still unstable as of 1.95; magic number retained with explanation).

Docs / comments

• types/mod.rs 0.0.5 #4 — Removed stale "~30-symbol facade" comment.
• types/mod.rs refactor: ownership, perf, error handling, domain extraction, clippy … #5 — Fixed misleading sanitize_for_display comment (used in production, not test-only).
• user_menu.rs refactor: ownership, perf, error handling, domain extraction, clippy … #5 — Fixed hardcoded ~/.config/lc/menu error message to render the actual paths::user_menu_path().
• lib.rs 0.0.2 #1/feat: add centralized theme module with MC-style blue background #2 — Replaced 9-line archaeology comment with a 2-line summary.

Deferred (51 findings)

Deferred by category:

Category	Count	Reason
fs/cha.rs style + refactoring	6	Large-scale cha.rs restructuring (cfg_trivial macro, ChaKind enum, visibility, Hash) — out of scope for a focused debt PR
fs/reader.rs + watcher.rs refactoring	8	Structural refactors (re-export chains, thin wrappers, duplication, path_cache growth) — each needs careful caller analysis
app/types perf (panel.rs, text_input.rs)	4	Perf changes (clone to ref, single-pass scroll_offset, canonicalize off input thread) — require benchmarking and API changes
app/types test_helpers.rs	4	Test-builder ergonomics (EntryKind default, len validation, precedence, path required) — low ROI churn on test-only code
app/types correctness guards	5	Defensive guards (dir_tree path-prefix, panel set_filtered skip, index bounds, format_size naming) — risk masking bugs vs catching them, needs design discussion
dialogs.rs (Cow/newtype, Vec String)	2	60+ call-site String to Cow migration — deferred to dedicated API PR
Cargo.toml / LICENSE	2	Legal name change (LICENSE), unused serde derive analysis (sorting.rs) — needs owner decision
sorting.rs / panel.rs API	3	Dead API cleanup (ToggleResult, ParseSortError), as_str visibility, is_parent helper — low risk but out of scope
Test-only / notes	7	Comment corrections on test intent (tests.rs), watcher test flakiness, sync_watcher_job_state naming, main.rs file_name_str location
Comment-only / pure style	10	Minor doc/comment churn deferred per PR scope (skip pure comment churn)

Total: 65 findings — 14 fixed, 51 deferred.

Summary by Sourcery

Clean up maintainability debt across the app layer, keymap tests, configuration, and CI workflows without changing runtime behavior.

Bug Fixes:

• Ensure user menu load errors report the actual configured global menu path instead of a hardcoded location.

Enhancements:

• Derive defaults for UI and menu source types to simplify state initialization and remove hand-written implementations.
• Add a drift-guard test to enforce mode-grouped keybindings and keep help output headers consistent.
• Clarify crate-level and type-level documentation and comments for public API usage and internal utilities.
• Document release profile panic behavior and platform-specific filesystem watching choices in Cargo configuration.

Build:

• Broaden Cargo package exclude patterns to omit docs from distribution and set release builds to abort on panic.

CI:

• Add concurrency groups, timeouts, and explicit shells to Rust and release workflows to avoid overlapping runs and improve CI reliability.
• Simplify the opencode review workflow triggers while keeping review behavior intact.

Documentation:

• Tighten and update library, type, and helper documentation to better reflect current usage and rationale.

Tests:

• Add a contiguity test that enforces keybindings being grouped by mode to prevent inconsistent help output sections.

[sourcery-ai]

Reviewer's Guide

Maintainability-focused cleanup across CI workflows, Cargo configuration, app state types, keymap tests, and documentation/comments, with no functional behavior changes but added invariants and clearer configuration rationale.

Sequence diagram for updated user menu loading error handling

sequenceDiagram
    actor Caller
    participant user_menu
    participant paths

    Caller->>user_menu: load_menu_with_warnings(panel_dir, filename)
    user_menu->>paths: user_menu_path()
    paths-->>user_menu: global
    user_menu->>user_menu: locate_menu_file_with_global(panel_dir, global.as_deref())
    alt menu_found
        user_menu-->>Caller: Ok(LoadedMenu)
    else menu_not_found
        user_menu->>user_menu: format("No user menu file found (searched: {}/.mc.menu, {})")
        user_menu-->>Caller: Err(String)
    end

Loading

File-Level Changes

Change	Details	Files
Simplified app state defaults and strengthened keymap invariants.	Derived Default for UiState instead of maintaining a manual implementation Derived Default for MenuSource and marked Global as the default variant Added a test that enforces KEYBINDINGS remain grouped by mode to keep help output headers consistent Clarified the CompareMode::ALL drift-guard test comment to document reliance on a magic number until variant_count is stabilized	src/app/types/app_state.rs src/app/user_menu.rs src/app/keymap.rs src/app/types/modes.rs
Improved user menu error reporting to reflect actual configuration.	Switched load_menu_with_warnings to use locate_menu_file_with_global with the configured global menu path Changed the missing-menu error message to render the resolved global path or a clear "no global path configured" notice	src/app/user_menu.rs
Tightened CI workflow robustness with timeouts, concurrency controls, and explicit shells.	Added timeout-minutes to the rust workflow job and all rust job steps use explicit shell: bash Introduced concurrency groups for rust and release workflows to avoid overlapping runs on the same ref Set timeouts for release build and publish jobs and ensured shell: bash on the release build step Removed unreachable issue_comment/pull_request_review_comment triggers and an unnecessary job-level if guard from opencode.yml, adding a timeout to the review job	.github/workflows/rust.yml .github/workflows/release.yml .github/workflows/opencode.yml
Refined Cargo configuration for packaging and runtime behavior, with documented rationale.	Documented the MSRV reason inline on rust-version (edition 2024 plus let-chains usage) Broadened the crate exclude list to skip docs/ from the published package Configured profile.release to use panic = "abort" given no production catch_unwind usage Added a comment explaining why macOS uses the FSEvents-only notify backend instead of kqueue	Cargo.toml
Updated comments and docs to match current API and usage, reducing stale or misleading guidance.	Trimmed a long historical comment in lib.rs to a concise explanation of how the binary and tests consume the crate Removed a speculative facade-growth comment from app types re-exports Corrected the sanitize_for_display comment to note its production display usage alongside tests	src/lib.rs src/app/types/mod.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 keybindings_are_grouped_by_mode test can be simplified and made more efficient by tracking the last seen mode and a set of completed modes rather than repeatedly calling contains on a Vec, which would more directly express the "no mode may reappear after a different mode" invariant.
• The error message construction in load_menu_with_warnings now has a small block of logic to derive global_display; if similar messaging is needed elsewhere, consider extracting this into a helper to avoid duplicating the (no global menu path configured) handling.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `keybindings_are_grouped_by_mode` test can be simplified and made more efficient by tracking the last seen mode and a set of completed modes rather than repeatedly calling `contains` on a `Vec`, which would more directly express the "no mode may reappear after a different mode" invariant.
- The error message construction in `load_menu_with_warnings` now has a small block of logic to derive `global_display`; if similar messaging is needed elsewhere, consider extracting this into a helper to avoid duplicating the `(no global menu path configured)` handling.

---

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.}

[greptile-apps]

Greptile Summary

This PR performs application-layer and CI maintenance without changing normal application behavior.

• Derives equivalent defaults for UI and menu-source state and improves the missing-menu diagnostic.
• Adds a keymap-ordering regression test and updates internal documentation.
• Adds workflow concurrency, timeouts, and explicit shells.
• Adjusts release packaging and panic behavior in Cargo configuration.

Confidence Score: 4/5

The release concurrency behavior should be fixed before merging because moving a tag during an active run can publish binaries from the superseded commit.

Same-tag runs are serialized while the older run is allowed to finish, so it can publish artifacts built from the old tag event before the updated run starts.

Files Needing Attention: .github/workflows/release.yml

Important Files Changed

Filename	Overview
.github/workflows/release.yml	Adds timeouts and same-tag serialization, but retaining obsolete runs can publish stale artifacts after a tag is moved.
.github/workflows/rust.yml	Adds ref-scoped cancellation, a job timeout, and explicit Bash shells without an identified correctness issue.
Cargo.toml	Updates package exclusions and release panic behavior; no actionable regression was established.
src/app/types/app_state.rs	Replaces the manual UiState default with an equivalent derived implementation.
src/app/user_menu.rs	Preserves menu lookup semantics while reporting the actual configured global menu path.
src/app/keymap.rs	Adds a test enforcing the mode-contiguity invariant required by help generation.

_{Reviews (1): Last reviewed commit: "chore: debt — app types, keymap, CI time..." | Re-trigger Greptile}

[greptile-apps]

Obsolete tag run still publishes

When a version tag is moved while its original release run is active, cancel-in-progress: false lets that obsolete run publish binaries built from the superseded commit before the updated run starts, leaving the tag's release with stale artifacts.

[opencode-agent]

I've completed a thorough review. Here is my assessment.

Review Summary

Overall: High-quality, well-scoped debt cleanup. Approve. All CI gates verified green locally: cargo fmt --check, cargo clippy --locked --all-targets -- -D warnings, cargo test --locked (302 passed), cargo build --release --locked.

Verified Correct (no issues)

panic = "abort" (Cargo.toml) — Safe. The only catch_unwind is in src/fs/watcher/tests.rs:913 (test-only, runs under dev profile where panic=unwind). Terminal cleanup on panic is preserved because install_panic_hook() (main.rs:59-65) calls leave_tui_stdout() and panic hooks still execute under panic = "abort" before the process aborts.

UiState derive Default (app_state.rs) — Behavior-identical. I verified all 18 fields produce the same values as the deleted hand-written impl, including user_menu_source: MenuSource::Global now via the enum's #[default].

keybindings_are_grouped_by_mode test (keymap.rs:668) — Logic is correct. Tracing through [A,A,B,B,C,C,A], the 7th element correctly trips the assertion (seen_modes.last() is C, not A). It catches both non-contiguous and duplicate mode groups.

sanitize_for_display comment (types/mod.rs:53) — Verified accurate. The function is used in production at file_entry.rs:58 and file_entry.rs:199, not test-only as the old comment claimed.

user_menu.rs error message fix — A genuine bug fix. The hardcoded ~/.config/lc/menu was wrong when XDG_CONFIG_HOME is set or on Windows. The refactor into locate_menu_file_with_global cleanly avoids a second paths::user_menu_path() call and ensures the error message shows the exact path searched. No tests assert on the message text.

opencode.yml trigger removal — Correct. The removed issue_comment/pull_request_review_comment triggers were dead code; the job-level if: github.event_name == 'pull_request' guaranteed they never ran.

Minor Observation (non-blocking)

panic = "abort" creates a hard dependency on install_panic_hook(). With abort, TerminalGuard's Drop (main.rs:53-57) will not run on panic — terminal restoration relies entirely on the panic hook. This is currently correct, but a future refactor that removes or reorders install_panic_hook() would silently lose terminal cleanup on panic (leaving the user's terminal in raw mode). The PR description's justification ("no catch_unwind in production") doesn't mention this coupling. Consider a one-line comment near install_panic_hook() noting that it's required for terminal safety under panic = "abort".

Rust Best Practices & Conventions

• Deriving Default over hand-written impls — idiomatic ✓
• #[default] enum attribute (stabilized 1.62) — appropriate for MSRV 1.95 ✓
• Drift-guard test for KEYBINDINGS ordering — aligns with the project's existing compare_mode_all_is_exhaustive pattern ✓
• let ... else / if let && chains used in the new code — matches conventions ✓
• Concurrency groups and timeout-minutes follow GitHub Actions best practices ✓

No bugs, no quality concerns, no convention violations found.

opencode session  |  github run

[opencode-agent]

Review

I independently verified the diff (built, ran the new test, cargo fmt --check, and confirmed the catch_unwind test under --release). Overall this is a clean, well-scoped cleanup. Approve with two minor follow-ups.

One real issue — orphaned function (created by this PR)

The user_menu.rs refactor bypassed locate_menu_file, leaving it with zero callers:

• Before (1f95aac): load_menu_with_warnings called locate_menu_file at line 461.
• After: that call site switched to locate_menu_file_with_global, and the only remaining references to locate_menu_file are its own definition (src/app/user_menu.rs:434) and nothing else (the test at line 900 uses locate_menu_file_with_global directly).

Since it's pub, rustc/clippy won't flag it (it's technically part of the lib's public API), so CI stays green — but within this project it's now dead. Per the repo's own "Surgical Changes" rule ("Remove functions that YOUR changes made unused"), either delete it or keep it as a deliberate thin wrapper with a one-line comment saying so. This is the one item I'd ask to be addressed.

One latent footgun — worth a comment, not a code change

panic = "abort" is safe — I verified the mutex-poison test (lock_or_recover_rebuilds_state_on_poison, the only catch_unwind site at src/fs/watcher/tests.rs:913) still passes under cargo test --release because Cargo auto-forces unwind for test builds. So the PR's rationale holds (stronger than stated).

However, with abort semantics TerminalGuard::drop (src/main.rs:53) will not run on panic — terminal restoration then depends entirely on install_panic_hook() (src/main.rs:59) calling leave_tui_stdout(). That's correct today, but a future refactor that drops/reorders the hook would silently leave the user's terminal in raw mode. A one-line comment near install_panic_hook() noting "required for terminal cleanup under panic = abort" would future-proof it.

On the other reviewers' comments

• Greptile P1 (stale tag publishes) — already resolved. Their review targeted the first commit, which had cancel-in-progress: false. The follow-up commit d371a06 flipped it to true (release.yml:13), so the obsolete run is now cancelled. Non-issue.
• Sourcery — simplify the contiguity test — reasonable style nit, non-blocking. The current logic is correct (I traced [A,A,B,B,C,C,A] → fails on the 7th element as intended). A cleaner form tracks last_mode explicitly and asserts against a seen set, which states the invariant ("no mode reappears") more directly than the contains + last() combo.
• Sourcery — extract global_display helper — I'd push back: single call site, premature abstraction.
• opencode.yml trigger removal — correct that they were dead under the old if: github.event_name == 'pull_request' guard. Only caveat: if comment-triggered re-reviews (à la @bot review) were ever desired, those triggers were the hook for it. Acceptable for a no-behavior-change PR; just confirm intent.

Verified correct (no concerns)

• UiState derive Default is behavior-identical field-by-field; MenuSource::Global preserved via #[default].
• user_menu.rs error message is a genuine fix — the hardcoded ~/.config/lc/menu was wrong under XDG_CONFIG_HOME/Windows; now renders the resolved path. filename param is still used downstream (filter_entries).
• keybindings_are_grouped_by_mode premise matches build_help_message (keymap.rs:614-623), which only emits a header on mode change.
• cargo fmt --check clean; new test passes.

Recommendation: merge after addressing the orphaned locate_menu_file (delete or document). The panic-hook comment and test simplification are optional polish.

opencode session  |  github run