fix: treat flatpak as fast manager for live typing#7
fix: treat flatpak as fast manager for live typing#7Timmy6942025 wants to merge 1 commit intomasterfrom
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change removes Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/smoke.sh`:
- Around line 1405-1408: The test contains a duplicated assertion using
assert_fzf_line_contains for "FPF_IPC_MANAGER_LIST=apt,flatpak,bun" (the two
consecutive calls); remove the redundant assert_fzf_line_contains call (or if
both are intentionally checking different fzf invocations, disambiguate by
adding a short comment explaining why both are needed) and keep the updated
assert_contains "flatpak search --columns=application,description rip" as-is;
look for the duplicate calls to assert_fzf_line_contains in the tests/smoke.sh
snippet to make the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f29d1b6-90c4-4675-b51c-1b9c5b7a76d5
📒 Files selected for processing (3)
cmd/fpf/cli_runtime.gocmd/fpf/cli_runtime_test.gotests/smoke.sh
📜 Review details
🧰 Additional context used
🪛 GitHub Actions: test
cmd/fpf/cli_runtime.go
[error] 1-1: Execution failed with "Permission denied" when running /home/runner/work/fpf-cli/fpf-cli/fpf (exit code 126).
cmd/fpf/cli_runtime_test.go
[error] 1-1: Execution failed with "Permission denied" when running /home/runner/work/fpf-cli/fpf-cli/fpf (exit code 126).
tests/smoke.sh
[error] 883-883: Smoke test failed: unable to execute ./fpf binary due to permission error: "Permission denied". Process completed with exit code 126.
🔇 Additional comments (3)
cmd/fpf/cli_runtime.go (1)
803-817: Reclassifyingflatpakas a fast manager looks reasonable given the caching layer.The flatpak search implementation in
search_entries.goprioritizes the local cache (flatpak.ShouldUseDirectCache()/flatpak.LoadBest()), which should provide fast responses when warm. However, when the cache is cold or missing, the code falls back toflatpak searchorflatpak remote-lswhich spawn blocking external processes.Consider whether users on first-run or with stale caches might experience slower live-typing responsiveness. If this becomes an issue, you could add a check like
flatpak.IsCacheWarm()to conditionally classify it.cmd/fpf/cli_runtime_test.go (1)
100-124: Test expectations correctly updated to reflect flatpak reclassification.The updated assertions properly verify:
- Default behavior now includes
flatpakin the fast subset- Invalid override correctly falls back to the new default (
apt,flatpak,bun)The test coverage for
dynamicReloadManagersremains comprehensive.tests/smoke.sh (1)
6-6: Add a build step to the CI workflow before running smoke tests.The CI pipeline runs
npm test(which executessmoke.sh) without first building thefpfbinary. The smoke.sh script expects the binary at${ROOT_DIR}/fpf, but the build process is never triggered. Either add ago build ./cmd/fpf -o ./fpfstep to the test workflow before runningnpm test, or invoke the build script that already exists atscripts/build-go-binaries.sh.
| assert_fzf_line_contains "FPF_IPC_MANAGER_LIST=apt,flatpak,bun" | ||
| assert_fzf_line_contains "FPF_IPC_MANAGER_LIST=apt,flatpak,bun" | ||
| assert_contains "bun search rip" | ||
| assert_not_contains "flatpak search --columns=application,description rip" | ||
| assert_contains "flatpak search --columns=application,description rip" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test assertions correctly updated; duplicate assertion on lines 1405-1406.
Lines 1405 and 1406 both assert FPF_IPC_MANAGER_LIST=apt,flatpak,bun. If this duplication is intentional (e.g., checking different fzf invocations), consider adding a comment. Otherwise, remove the redundant line.
The assertion change on line 1408 from assert_not_contains to assert_contains correctly reflects that flatpak is now queried during live typing.
🔧 Remove duplicate assertion if unintentional
assert_not_contains "--feed-search"
assert_contains "apt-cache dumpavail"
assert_fzf_line_contains "FPF_IPC_MANAGER_LIST=apt,flatpak,bun"
- assert_fzf_line_contains "FPF_IPC_MANAGER_LIST=apt,flatpak,bun"
assert_contains "bun search rip"
assert_contains "flatpak search --columns=application,description rip"📝 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.
| assert_fzf_line_contains "FPF_IPC_MANAGER_LIST=apt,flatpak,bun" | |
| assert_fzf_line_contains "FPF_IPC_MANAGER_LIST=apt,flatpak,bun" | |
| assert_contains "bun search rip" | |
| assert_not_contains "flatpak search --columns=application,description rip" | |
| assert_contains "flatpak search --columns=application,description rip" | |
| assert_fzf_line_contains "FPF_IPC_MANAGER_LIST=apt,flatpak,bun" | |
| assert_contains "bun search rip" | |
| assert_contains "flatpak search --columns=application,description rip" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/smoke.sh` around lines 1405 - 1408, The test contains a duplicated
assertion using assert_fzf_line_contains for
"FPF_IPC_MANAGER_LIST=apt,flatpak,bun" (the two consecutive calls); remove the
redundant assert_fzf_line_contains call (or if both are intentionally checking
different fzf invocations, disambiguate by adding a short comment explaining why
both are needed) and keep the updated assert_contains "flatpak search
--columns=application,description rip" as-is; look for the duplicate calls to
assert_fzf_line_contains in the tests/smoke.sh snippet to make the change.
Move flatpak and npm from slow to fast manager list in defaultDynamicReloadManagers(). Now live typing (change: reload) includes ALL managers - no Ctrl+R needed. This makes live typing results consistent with CLI argument results. Updated tests to reflect new behavior.
Timmy6942025
force-pushed
the
fix/flatpak-fast-manager
branch
from
bcb9b06 to
965dbc6
Compare
1 participant
Summary
Move flatpak from slow to fast manager list in
defaultDynamicReloadManagers().change:reload) now includes flatpak results alongside apt/bunChanges
cmd/fpf/cli_runtime.go: Remove"flatpak": {}from slow mapcmd/fpf/cli_runtime_test.go: Update test expectationstests/smoke.sh: Update smoke test assertionsTesting