[luv-343] feat: per-CLI multi-select control panel in Policies → Configure#344
Open
NiveditJain wants to merge 5 commits intomainfrom
Open
[luv-343] feat: per-CLI multi-select control panel in Policies → Configure#344NiveditJain wants to merge 5 commits intomainfrom
NiveditJain wants to merge 5 commits intomainfrom
Conversation
Extends getHooksConfigAction() with a `clis` array (one entry per INTEGRATION_TYPES) carrying the installed/detected/settingsPath state for all 7 agent CLIs. Backend prep for the upcoming multi-CLI selector in the Policies → Configure tab; the legacy installedScopes/settingsPath fields remain for back-compat. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…igure Replaces the Claude-only install banner with a multi-CLI control panel that covers all 7 supported agent CLIs. Each CLI gets a checkbox row with brand- colored accent rail, install status pill (`Active` / `Detected` / `Inactive`), and user-scope settings path. A 7-segment coverage strip across the top and a glowing-LED status header give a glanceable view of which CLIs are protected. Users multi-select CLIs and hit `Apply changes` to install/uninstall the diff in one round-trip; pending changes are flagged in pink (`+ install` / `− remove`) before commit. Detected CLIs are pre-checked on first load so a fresh user is one click from full coverage. Backend: getHooksConfigAction() now returns a `clis` array populated from listIntegrations() in src/hooks/integrations.ts; the existing per-CLI install actions (which already accepted a cli list) are wired up by the UI for the first time. Legacy installedScopes / settingsPath payload fields kept for back-compat. Adds an action-layer unit test that asserts the clis payload shape, ordering, and per-CLI flag mapping. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds per-CLI install metadata to ChangesMulti-CLI Hooks Control Panel
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… drop coverage strip The post-Apply reload was running a useEffect that re-checked any CLI whose binary is on PATH (`c.installed || c.detected`), so unchecking a still-detected CLI like Codex and clicking Apply made the box flip back instantly — the remove call had succeeded server-side, but the UI made it look like a no-op. Fix: only pre-check `detected` CLIs on the FIRST config load (initial-mount nudge for new users); on every subsequent reload, sync `checkedClis` strictly to `installed`. After Apply the boxes now reflect the new install reality. Also drops the 7-segment horizontal coverage strip per design feedback. The per-row vertical accent rails stay. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
__tests__/actions/get-hooks-config.test.ts (1)
62-98: 🏗️ Heavy liftAdd coverage for the new client-side install state machine.
These tests validate the server payload, but the PR’s main behavior change is the
checkedClis/ pending-diff / apply / reinstall flow inapp/policies/hooks-client.tsx. A small component-level test matrix for detected preselect, uninstall, apply diff, and reinstall semantics would lock down the new behavior and catch regressions there.As per coding guidelines, "Always add unit tests for new behaviour. Place tests in tests/. Unit tests live in tests/hooks/, e2e tests in tests/e2e/hooks/."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/actions/get-hooks-config.test.ts` around lines 62 - 98, Add unit tests for the new client-side install state machine in app/policies/hooks-client.tsx: create component-level tests under __tests__/hooks/ that exercise the HooksClient (or exported component) through the checkedClis / pending-diff / applyDiff / reinstall flow, asserting initial detected-preselect behavior, toggling uninstall, applying the pending diff updates, and the reinstall semantics; use the existing test pattern in __tests__/actions/get-hooks-config.test.ts to locate fixtures and leverage render/fireEvent (testing-library) or equivalent to simulate clicks and verify checkedClis state transitions and UI labels after each action.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/policies/hooks-client.tsx`:
- Around line 1050-1056: handleReinstall is using Array.from(checkedClis) which
includes detected-but-not-installed CLIs; change handleReinstall to filter
checkedClis down to only CLIs that are currently installed (e.g., intersect
checkedClis with your CLI data source and keep items where cli.installed or
cli.isInstalled is true) before calling installHooksWebAction("user", targets);
if the filtered targets array is empty, return early; apply the same filtering
logic to the code path that shows the warning toast so “Reinstall” only ever
targets already-installed CLIs (refer to handleReinstall, checkedClis, and
installHooksWebAction).
- Around line 1035-1046: The handleApply function performs two separate
mutations (installHooksWebAction and removeHooksWebAction) but only calls
reload() when both succeed; move the reload into a finally path so the UI is
refreshed even after a partial failure, and preserve/set the action error on
failure; specifically, inside handleApply (which uses startTransition and sets
setActionError), wrap the await installHooksWebAction("user", toInstall) and
await removeHooksWebAction("user", toRemove) calls in the existing try/catch but
add a finally block that always calls await reload() (or ensures reload() is
invoked after the try/catch), so that the page state is resynced after any
partial success while still setting setActionError(e instanceof Error ?
e.message : "Failed to apply changes.") on error.
- Around line 968-975: The effect currently reseeds checkedClis from config.clis
(using setCheckedClis) whenever config changes, which re-checks
detected-but-uninstalled CLIs after reload(); change it so seeding happens only
on initial mount or only for truly installed CLIs: either run the useEffect once
(useEffect(..., [])) or add a guard/ref (e.g., initializedRef) so setCheckedClis
only sets from c.installed || c.detected on first load, or alternatively merge
with existing checkedClis (preserve user unchecked choices) by only adding
installed IDs and not re-adding detected IDs on subsequent config updates;
update the useEffect around setCheckedClis, referencing checkedClis,
setCheckedClis, config.clis, and reload() as needed.
---
Nitpick comments:
In `@__tests__/actions/get-hooks-config.test.ts`:
- Around line 62-98: Add unit tests for the new client-side install state
machine in app/policies/hooks-client.tsx: create component-level tests under
__tests__/hooks/ that exercise the HooksClient (or exported component) through
the checkedClis / pending-diff / applyDiff / reinstall flow, asserting initial
detected-preselect behavior, toggling uninstall, applying the pending diff
updates, and the reinstall semantics; use the existing test pattern in
__tests__/actions/get-hooks-config.test.ts to locate fixtures and leverage
render/fireEvent (testing-library) or equivalent to simulate clicks and verify
checkedClis state transitions and UI labels after each action.
🪄 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: CHILL
Plan: Pro
Run ID: b553c916-f0f1-40b1-97e3-15d8521db395
📒 Files selected for processing (5)
CHANGELOG.md__tests__/actions/get-hooks-config.test.tsapp/actions/get-hooks-config.tsapp/policies/hooks-client.tsxdocs/dashboard.mdx
Two issues flagged on PR #344: 1. handleApply skipped reload() on partial-failure paths — if install succeeded but remove failed, the next click would compute the diff against stale UI state. Move reload() into a finally block so the page resyncs after every batch attempt regardless of partial outcomes. 2. handleReinstall iterated over Array.from(checkedClis), which includes detected-but-not-installed CLIs that the UI pre-checks as a one-click adoption hint. Clicking Reinstall could therefore silently install brand- new CLIs from a button labeled "Reinstall". Filter to the intersection of checked × installedCliSet so the action matches its label; first-time installs go through Apply. 3. Re-wire the "Policies are not installed" toast button from handleReinstall to handleApply so the first-install flow (where installedCliSet is empty) still works from the toast. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/policies/hooks-client.tsx (1)
1153-1161: 💤 Low valueReinstall button can appear enabled but do nothing.
The disabled condition
checkedCount === 0doesn't account for the case where all checked CLIs are detected-but-not-installed. In that scenario, the button appears enabled, buthandleReinstallreturns early becausetargets(the intersection of checked × installed) is empty.🔧 Suggested fix: derive reinstall targets and use in disabled condition
+ const reinstallTargets = useMemo( + () => Array.from(installedCliSet).filter((id) => checkedClis.has(id)), + [installedCliSet, checkedClis], + ); + const handleReinstall = () => { - const targets = Array.from(installedCliSet).filter((id) => checkedClis.has(id)); - if (targets.length === 0) return; + if (reinstallTargets.length === 0) return; startTransition(async () => { // ... - await installHooksWebAction("user", targets); + await installHooksWebAction("user", reinstallTargets); // ... }); };Then update the button:
<Button variant="outline" size="sm" onClick={handleReinstall} - disabled={isPending || checkedCount === 0} + disabled={isPending || reinstallTargets.length === 0} className="text-xs h-7 px-3 font-mono" > Reinstall </Button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/policies/hooks-client.tsx` around lines 1153 - 1161, The Reinstall button can be enabled when checkedCount > 0 but there are no actual reinstall targets (checked items intersecting installed CLIs), so change the logic to compute the actual reinstall targets inside the component (e.g., derive a "reinstallTargets" array by filtering checked items against installed/available items the same way handleReinstall does), then use reinstallTargets.length === 0 to disable the Button; update the Button's disabled prop (currently disabled={isPending || checkedCount === 0}) to disabled={isPending || reinstallTargets.length === 0} and pass reinstallTargets into handleReinstall (or have handleReinstall use that precomputed list) so the button is only enabled when there are real targets to act on.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/policies/hooks-client.tsx`:
- Around line 1153-1161: The Reinstall button can be enabled when checkedCount >
0 but there are no actual reinstall targets (checked items intersecting
installed CLIs), so change the logic to compute the actual reinstall targets
inside the component (e.g., derive a "reinstallTargets" array by filtering
checked items against installed/available items the same way handleReinstall
does), then use reinstallTargets.length === 0 to disable the Button; update the
Button's disabled prop (currently disabled={isPending || checkedCount === 0}) to
disabled={isPending || reinstallTargets.length === 0} and pass reinstallTargets
into handleReinstall (or have handleReinstall use that precomputed list) so the
button is only enabled when there are real targets to act on.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66cfe712-6adb-4a4a-a03f-568d3c7abc6d
📒 Files selected for processing (1)
app/policies/hooks-client.tsx
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Apply changesto install/uninstall the diff in one round-trip; pending changes are flagged in pink (+ install/− remove) before commit, and detected-but-not-installed CLIs are pre-checked so a fresh user is one click from full coverage.getHooksConfigAction()now returns aclis: { id, label, installed, settingsPath, detected }[]array populated fromlistIntegrations()(src/hooks/integrations.ts). The existinginstallHooksWebAction(scope, cli?)/removeHooksWebAction(scope, cli?)server actions — which already accepted a per-CLI list — are wired up by the UI for the first time. LegacyinstalledScopes/settingsPathfields kept for back-compat.enabledPoliciesis global in~/.failproofai/policies-config.json).Test plan
bun x vitest run— 71 files / 1611 tests pass__tests__/actions/get-hooks-config.test.ts— 4 tests cover payload shape, registry order, per-CLI installed/detected flags, settingsPath, and label resolutionbun run lint— 0 errors (1 pre-existing warning intool-input-output.tsx)bun run build— Next.js build + tsc pass/policies?tab=policies, verify all 7 CLIs render with checkboxes, toggle Codex on → Apply →~/.codex/hooks.jsongains failproofai entries, uncheck Claude → Apply → settings file restored on reloadfailproofai policies --install --cli claudecontinues to work unchanged🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests