Repository navigation
fix(webui): retain tool permission selection while saving - #6339
Merged
think-in-universe merged 10 commits intoJul 21, 2026
Merged
Conversation
italic-jinxin
added
size: M
Contributor
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6339
July 20, 2026 08:46
Destroyed
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughTool permission saves now retain the selected pending value, expose a busy state, gate concurrent responses by request ID, validate persisted responses, abort requests after 30 seconds, and add unit and Reborn E2E coverage. ChangesTool permission save flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
actor User
participant ToolRow
participant useTools
participant updateToolPermission
participant SettingsAPI
User->>ToolRow: Select new permission
ToolRow->>useTools: setPermission(tool, state)
useTools-->>ToolRow: Pending state and aria-busy
useTools->>updateToolPermission: Save with requestId
updateToolPermission->>SettingsAPI: POST permission update
SettingsAPI-->>updateToolPermission: Response or timeout
updateToolPermission-->>useTools: Success or error
useTools-->>ToolRow: Saved state or rollback
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@claude review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
|
🚅 Deployed to the ironclaw-pr-6339 environment in ironclaw-ci-preview
|
Contributor
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 86.36% — 319873 / 370378 lines Per-crate breakdown (65 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (3 entry/entries excluded from the accounting above)
|
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6339
July 20, 2026 09:27
Destroyed
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6339
July 20, 2026 09:44
Destroyed
italic-jinxin
marked this pull request as ready for review
July 20, 2026 10:20
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ssion-optimistic-state # Conflicts: # crates/ironclaw_webui/frontend/src/pages/settings/lib/settings-api.test.ts
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6339
July 20, 2026 10:25
Destroyed
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_webui/frontend/src/pages/settings/hooks/useTools.ts (1)
47-68: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winCache write in
onSuccessisn't guarded against out-of-order stale completions.
queryClient.setQueryDataruns unconditionally at Line 48, before the staleness check at Line 58 (pendingRequestIds.current[name] !== requestId). That check only gates the pending/saved-indicator cleanup, not the cache write itself. Consider: user savesalways_allow(id=1) thendisabled(id=2). If id=2 resolves first (cache→disabled, pending cleared, "Saved" shown), and id=1's response arrives later (also success, since it did land on the server before being superseded), the cache is silently overwritten back toalways_allow— a stale response regresses an already-confirmed newer value, with no follow-up reconciliation (unlike the error path, which callsinvalidateQueries).This is the same class of concurrency issue flagged in the earlier review (older-success/newer-failure ordering) — that case is now tested and handled via
invalidateQuerieson error, but the success/success out-of-order case is still open and untested.🔒 Proposed fix: monotonic per-tool "latest applied" guard
+ const latestAppliedRequestId = React.useRef({}); + const mutation = useMutation({ mutationFn: async ({ name, state }) => throwIfApiFailed(await updateToolPermission(name, state), "Save failed"), onSuccess: (data, { name, state, requestId }) => { + if ((latestAppliedRequestId.current[name] || 0) > requestId) return; + latestAppliedRequestId.current[name] = requestId; queryClient.setQueryData(["settings-tools"], (old) => {Please also add a regression test for the success-after-success out-of-order case (newer resolves first, older resolves later).
🤖 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 `@crates/ironclaw_webui/frontend/src/pages/settings/hooks/useTools.ts` around lines 47 - 68, Guard the cache update in the onSuccess handler against stale out-of-order responses by tracking the latest applied request per tool and ignoring any success whose requestId is older than that value; keep the existing pending/saved-indicator guard and update the tracking state only when applying a current response. Add a regression test covering the newer success resolving first followed by the older success, asserting the cache retains the newer tool state.
🤖 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
`@crates/ironclaw_webui/frontend/src/pages/settings/components/tools-tab.test.ts`:
- Around line 187-205: Add an integration test that renders ToolsTab using the
mocked useTools() data with pendingPermissions containing an entry for the tool
name, then locate the resulting ToolRow and assert its pendingPermission prop
matches the entry’s state. Update the renderToolsModule mock setup as needed
while preserving the existing direct ToolRow coverage.
In `@crates/ironclaw_webui/frontend/src/pages/settings/lib/settings-api.ts`:
- Around line 163-177: Update updateToolPermission to validate that the API
response contains a well-formed persisted entry before returning success,
treating a missing or malformed data.entry as an error rather than passing null
or undefined downstream. Ensure failures prevent optimistic success handling,
and add a test covering responses with missing and malformed entries.
In `@tests/e2e/scenarios/test_reborn_webui_v2_legacy_tool_permissions.py`:
- Around line 358-360: Extract the inline settings-tool-permission selector from
the test around _tool_row into the shared SEL_V2 selector definitions, then
update the locator to reference that SEL_V2 symbol. Preserve the existing
selector value and test behavior.
---
Outside diff comments:
In `@crates/ironclaw_webui/frontend/src/pages/settings/hooks/useTools.ts`:
- Around line 47-68: Guard the cache update in the onSuccess handler against
stale out-of-order responses by tracking the latest applied request per tool and
ignoring any success whose requestId is older than that value; keep the existing
pending/saved-indicator guard and update the tracking state only when applying a
current response. Add a regression test covering the newer success resolving
first followed by the older success, asserting the cache retains the newer tool
state.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 217a7971-1716-481f-890e-7527e11a4997
📒 Files selected for processing (8)
crates/ironclaw_webui/frontend/src/pages/settings/components/tools-tab.test.tscrates/ironclaw_webui/frontend/src/pages/settings/components/tools-tab.tsxcrates/ironclaw_webui/frontend/src/pages/settings/hooks/useTools.test.tscrates/ironclaw_webui/frontend/src/pages/settings/hooks/useTools.tscrates/ironclaw_webui/frontend/src/pages/settings/lib/settings-api.test.tscrates/ironclaw_webui/frontend/src/pages/settings/lib/settings-api.tstests/e2e/reborn_coverage_tests.txttests/e2e/scenarios/test_reborn_webui_v2_legacy_tool_permissions.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_webui/frontend/src/pages/settings/lib/settings-api.ts (1)
204-213:⚠️ Potential issue | 🔴 CriticalMissing backend evidence validation on save.
Violates
crates/ironclaw_webui/**invariant: "Capability completion may be claimed only from the structured outcome and admitted evidence; empty output is an error unless explicitly allowed by the contract."updateToolPermissionreturns success even ifdata.entryis missing, allowing an optimistic success state without durable backend evidence.🛡️ Proposed fix
const data = await apiFetch(`${SETTINGS_TOOLS_BASE}/${encodeURIComponent(name)}`, { method: "POST", body: JSON.stringify({ state: normalized }), signal: controller.signal, }); + if (!data?.entry) { + throw new Error("Save did not return a persisted tool entry"); + } return { success: true, tool: toolFromConfigEntry(data.entry), entry: data.entry };🤖 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 `@crates/ironclaw_webui/frontend/src/pages/settings/lib/settings-api.ts` around lines 204 - 213, Update updateToolPermission to validate that the POST response contains a valid data.entry before returning success; treat a missing or empty entry as an error and preserve the existing timeout cleanup. Only construct toolFromConfigEntry and claim success after backend evidence is present.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@crates/ironclaw_webui/frontend/src/pages/settings/lib/settings-api.ts`:
- Around line 204-213: Update updateToolPermission to validate that the POST
response contains a valid data.entry before returning success; treat a missing
or empty entry as an error and preserve the existing timeout cleanup. Only
construct toolFromConfigEntry and claim success after backend evidence is
present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6c08435a-2d01-439f-b0f9-725cf77a91b6
📒 Files selected for processing (1)
crates/ironclaw_webui/frontend/src/pages/settings/lib/settings-api.ts
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6339
July 20, 2026 10:34
Destroyed
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/ironclaw_webui/frontend/src/pages/settings/hooks/useTools.ts (1)
78-100: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftConcurrent saves swallow per-tool error state.
Because
useMutationtracks state globally across the hook, if a user modifies Tool B while Tool A's save is still pending, the new mutation supersedes the active observer state. If Tool A subsequently fails,mutation.erroris not updated, and the UI silently swallows the error.As per coding guidelines, do not lose evidence in UI results. Manage the error state independently of the React Query mutation so that stale failures on concurrent saves are reliably surfaced.
🐛 Proposed fix for `useTools.ts`
@@ -15,6 +15,7 @@ const [savedTools, setSavedTools] = React.useState({}); const [pendingPermissions, setPendingPermissions] = React.useState({}); + const [saveError, setSaveError] = React.useState(null); const nextRequestId = React.useRef(0); const pendingRequestIds = React.useRef({}); const savedTimeouts = React.useRef({}); @@ -71,9 +72,12 @@ }, onError: (_error, { name, requestId }) => { + const isLatest = pendingRequestIds.current[name] === requestId; clearPendingPermission(name, requestId); queryClient.invalidateQueries({ queryKey: ["settings-tools"] }); + if (isLatest) { + setSaveError(_error); + } }, }); @@ -82,6 +86,7 @@ const requestId = nextRequestId.current + 1; nextRequestId.current = requestId; pendingRequestIds.current[name] = requestId; mutation.reset(); + setSaveError(null); setPendingPermissions((prev) => ({ ...prev, [name]: { requestId, state }, @@ -97,7 +102,7 @@ setPermission, savedTools, pendingPermissions, - error: mutation.error, + error: saveError, }; }🤖 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 `@crates/ironclaw_webui/frontend/src/pages/settings/hooks/useTools.ts` around lines 78 - 100, Update the useTools hook’s setPermission flow to track save errors independently for each tool/request instead of relying on the shared mutation.error state. Preserve request IDs so concurrent failures, including stale failures from earlier saves, are recorded and surfaced for the corresponding tool without being overwritten by later mutations.Source: Coding guidelines
crates/ironclaw_webui/frontend/src/pages/settings/hooks/useTools.test.ts (1)
81-176: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate assertions to filter for function updaters.
If
useTools.tsis fixed to tracksaveErrorindependently using a direct value, multiple tests will break becausecalls.filter(...)forsetStatewill mistakenly capture thesetSaveErrorvalue (nullorError) instead of thesetPendingPermissionsupdater function, resulting inTypeError: updater is not a function.Filter for function updaters to ensure the correct state update is asserted.
🛠️ Proposed fix
@@ -90,7 +90,7 @@ tools.setPermission("builtin.echo", "disabled"); - const pendingUpdate = calls.find((call) => call.type === "setState"); + const pendingUpdate = calls.find((call) => call.type === "setState" && typeof call.updater === "function"); assert.ok(pendingUpdate, "expected the pending permission to update immediately"); @@ -104,7 +104,7 @@ tools.setPermission("builtin.echo", "disabled"); - const pendingPermission = calls.find((call) => call.type === "setState").updater({}); + const pendingPermission = calls.find((call) => call.type === "setState" && typeof call.updater === "function").updater({}); const variables = calls.find((call) => call.type === "mutate").payload; getMutationOptions().onError(new Error("permission denied"), variables); - const rollbackUpdate = calls.filter((call) => call.type === "setState").at(-1); + const rollbackUpdate = calls.filter((call) => call.type === "setState" && typeof call.updater === "function").at(-1); assert.deepEqual(JSON.parse(JSON.stringify(rollbackUpdate.updater(pendingPermission))), {}); @@ -124,7 +124,7 @@ tools.setPermission("builtin.echo", "disabled"); const mutations = calls.filter((call) => call.type === "mutate"); - const pendingUpdates = calls.filter((call) => call.type === "setState"); + const pendingUpdates = calls.filter((call) => call.type === "setState" && typeof call.updater === "function"); const pendingPermissions = pendingUpdates.reduce( @@ -138,7 +138,7 @@ getMutationOptions().onError(new Error("permission denied"), mutations[1].payload); - const rollbackUpdate = calls.filter((call) => call.type === "setState").at(-1); + const rollbackUpdate = calls.filter((call) => call.type === "setState" && typeof call.updater === "function").at(-1); assert.deepEqual(🤖 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 `@crates/ironclaw_webui/frontend/src/pages/settings/hooks/useTools.test.ts` around lines 81 - 176, Update the setState call selections in the permission-save tests, especially “failed permission saves clear the pending selection...” and “a newer failed save...” to filter for entries whose updater is a function before invoking updater. Keep assertions focused on the pending-permissions updater and avoid capturing direct saveError state values.
🤖 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.
Outside diff comments:
In `@crates/ironclaw_webui/frontend/src/pages/settings/hooks/useTools.test.ts`:
- Around line 81-176: Update the setState call selections in the permission-save
tests, especially “failed permission saves clear the pending selection...” and
“a newer failed save...” to filter for entries whose updater is a function
before invoking updater. Keep assertions focused on the pending-permissions
updater and avoid capturing direct saveError state values.
In `@crates/ironclaw_webui/frontend/src/pages/settings/hooks/useTools.ts`:
- Around line 78-100: Update the useTools hook’s setPermission flow to track
save errors independently for each tool/request instead of relying on the shared
mutation.error state. Preserve request IDs so concurrent failures, including
stale failures from earlier saves, are recorded and surfaced for the
corresponding tool without being overwritten by later mutations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5a6d8cc7-1284-4619-b492-e80237548082
📒 Files selected for processing (2)
crates/ironclaw_webui/frontend/src/pages/settings/hooks/useTools.test.tscrates/ironclaw_webui/frontend/src/pages/settings/hooks/useTools.ts
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6339
July 20, 2026 10:51
Destroyed
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6339
July 20, 2026 10:55
Destroyed
Contributor
Author
|
@claude review |
Code Review: PR #6339Reviewed by: Security & Safety, Architecture & Patterns, Bug Scan, and Performance & Production agents SummaryNo critical issues found. The PR implements optimistic UI state for tool permission changes with strong defensive design, comprehensive test coverage, and proper error handling. Key Strengths
Minor Observations (Not Issues)
Verification
Result: Approved — production ready. |
think-in-universe
enabled auto-merge
July 21, 2026 10:31
think-in-universe
approved these changes
Jul 21, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 21, 2026
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6339
July 21, 2026 11:10
Destroyed
think-in-universe
approved these changes
Jul 21, 2026
think-in-universe
deleted the
issue-6331-tool-permission-optimistic-state
branch
July 21, 2026 12:32
21 of 29 tasks
7 tasks done
This branch was successfully deployed
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
2 participants
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
Change Type
Linked Issue
Closes #6331
Validation
TZ=UTC ./node_modules/.bin/vitest run./node_modules/.bin/tsc --noEmitnode --experimental-strip-types scripts/check-source-conventions.ts./node_modules/.bin/vite buildcargo test -p ironclaw_webui --all-featurescargo clippy -p ironclaw_webui --all-features --all-targets -- -D warningsscripts/pre-commit-safety.shSecurity Impact
None. This changes only client-side presentation state while using the existing authenticated tool-permission API.
Reborn Trust-Boundary Checklist
N/A. No trust boundary, authorization, runtime, or persistence behavior changed.
Database Impact
None. No schema, migration, or persistence changes.
Blast Radius
Limited to the WebUI Settings → Tools permission dropdown and its frontend test coverage.
Rollback Plan
Revert the two commits to restore the previous server-value-only rendering behavior and remove the associated regression coverage.
Review Follow-Through
The pending, success, failure rollback, and stale-response paths were reviewed. No blocking findings or additional follow-up work were identified.
Review track: B