Repository navigation
fix(webui-v2): surface tool permission save errors - #5699
Conversation
italic-jinxin
added
size: M
|
| Reviewer | State | Verdict | Findings | Last update |
|---|---|---|---|---|
ironloop/common-reviewer (reviewer) |
Superseded | N/A | N/A | 2026-07-07T08:55:37.199Z |
Reviewer summaries
| Reviewer | Detail |
|---|---|
ironloop/common-reviewer (reviewer) |
Superseded by a newer PR head. New head: ef4ab49. Previous verdict: Approved. |
Recent activity
| Time | Reviewer | State | Detail |
|---|---|---|---|
| 2026-07-06T15:03:44.356Z | ironloop/common-reviewer (reviewer) |
Queued | Waiting for this reviewer lane to become available. |
| 2026-07-06T15:03:44.400Z | ironloop/common-reviewer (reviewer) |
Queued | Added to the local review work handoff. |
| 2026-07-06T15:03:48.094Z | ironloop/common-reviewer (reviewer) |
Started | Reviewer worker started attempt 1. |
| 2026-07-06T15:03:52.627Z | ironloop/common-reviewer (reviewer) |
Workspace ready | Prepared isolated checkout (merge_ref) at 9dab1cd. |
| 2026-07-06T15:05:35.480Z | ironloop/common-reviewer (reviewer) |
Superseded | Old-head reviewer is still running after newer head ef4ab49 replaced it. Codex is reviewing; process live; elapsed 1m 44s; timeout in 18m 16s; last heartbeat 2026-07-06T15:05:35.480Z. Activity (stderr): ...dsWith(";"); 31 continue; 32 } 33 const stripped = stripExport(line); 34 if (stripped != null) lines.push(stripped);…. |
| 2026-07-06T15:05:46.837Z | ironloop/common-reviewer (reviewer) |
Result captured | Approved; 0 blocking findings. |
| 2026-07-06T15:05:46.837Z | ironloop/common-reviewer (reviewer) |
Completed | Review completed and terminal status was persisted. |
| 2026-07-07T08:55:37.199Z | ironloop/common-reviewer (reviewer) |
Superseded | A newer PR head replaced this review (ef4ab49). |
Available commands
@ironloopai agents@ironloopai review@ironloopai review --agent <agent-id-or-alias>@ironloopai status
Run metadata
Admission: webhook accepted the request and IronLoop persisted review state before this projection.
github-actions
Bot
added
size: S
|
@claude review |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Verdict: ✅ Approved
Findings: 0 blocking / 0 notes
Next: No reviewer action needed.
Head: 832c807fca8bef1bbddfa2c6f93b259c9232b5c3
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
No blocking issues found. The change wires the tool-permission mutation error into the Tools settings tab and adds focused unit plus Playwright coverage for failed saves without broadening the behavior surface.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloop review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloop reviewwhen the fix may affect multiple areas. - Use
@ironloop statusto check queued/running/completed/stale/stalled state while reviewers run.
|
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 (2)
💤 Files with no reviewable changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a permission-save error alert to the WebUI v2 Tools tab, sourced from ChangesTool permission save failure surfacing
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ToolsTab
participant useTools
participant API
User->>ToolsTab: change tool permission
ToolsTab->>useTools: setPermission(name, state)
useTools->>useTools: mutation.reset()
useTools->>API: POST /api/webchat/v2/settings/tools/{tool}
API-->>useTools: 403 permission_denied
useTools-->>ToolsTab: permissionError
ToolsTab-->>User: render error.saveFailed alert
Related issues: Suggested labels: webui-v2, frontend, tests Suggested reviewers: none identified from provided context No sandbox, trust-boundary, secrets, egress, or migration invariants are implicated here — this is client-side error UI plus test coverage, gated by existing lint/format/CI checks. Poem:
🚥 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds error handling for permission save failures in the tools tab of the settings page, displaying a translated alert banner when an error occurs, and includes corresponding unit and end-to-end tests. The feedback suggests replacing hardcoded Tailwind red colors with theme-aware semantic variables and color-mix to ensure the alert banner remains readable in both light and dark modes.
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.
|
No issues found. |
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.45% — 276149 / 323178 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 (4 entry/entries excluded from the accounting above)
|
![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 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_v2/static/js/pages/settings/components/tools-tab.js`:
- Line 187: The shared save error from useTools() is persisting across retries,
so the tools-tab.js banner can show a stale failure for the wrong save attempt.
Update the save flow in useTools.js and/or the ToolsTab component to call
mutation.reset() before starting a new permission update, or otherwise track the
error per row in setPermission so only the failing tool is marked. Refer to
useTools, setPermission, and the error value returned from useMutation when
wiring the retry/reset behavior.
🪄 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: 393ab814-06f3-4b65-9f09-f141f542d461
📒 Files selected for processing (3)
crates/ironclaw_webui_v2/static/js/pages/settings/components/tools-tab.jscrates/ironclaw_webui_v2/static/js/pages/settings/components/tools-tab.test.mjstests/e2e/scenarios/test_reborn_webui_v2_legacy_tool_permissions.py
🗂️ Archived IronLoop Review: reviewerThis result is from an older PR head and is no longer the active review.
Archived summaryNo concrete blocking issues found in the PR diff. The change surfaces failed tool-permission saves, resets the previous mutation error before retry, and adds focused unit/e2e coverage for the regression path. |
github-actions
Bot
added
size: M
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Verdict: ✅ Approved
Findings: 0 blocking / 0 notes
Next: No reviewer action needed.
Head: e860afaf57a76289214c1d9a659125ceccfd5928
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
No blocking issues found. The change surfaces tool permission mutation failures in the settings UI, clears stale mutation errors before retrying, and adds focused unit plus e2e coverage for the failure path.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloop review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloop reviewwhen the fix may affect multiple areas. - Use
@ironloop statusto check queued/running/completed/stale/stalled state while reviewers run.
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Verdict: ✅ Approved
Findings: 0 blocking / 0 notes
Next: No reviewer action needed.
Head: e860afaf57a76289214c1d9a659125ceccfd5928
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
No blocking issues found. The change surfaces tool permission mutation failures in the settings UI, clears stale mutation errors before retrying, and adds focused unit plus e2e coverage for the failure path.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloop review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloop reviewwhen the fix may affect multiple areas. - Use
@ironloop statusto check queued/running/completed/stale/stalled state while reviewers run.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_v2/static/js/pages/settings/hooks/useTools.test.mjs`:
- Around line 6-14: The local sourceForTest helper in useTools.test.mjs is
duplicated and still uses the fragile single-line import skip logic, so it can
break on wrapped imports. Extract the hardened helper shared with
tools-tab.test.mjs, or otherwise update this copy to use the same skippingImport
approach, and keep the export rewriting in sync with the shared test utility.
🪄 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: 1f29323a-4167-4745-9f3d-4bfe155e4448
📒 Files selected for processing (4)
crates/ironclaw_webui_v2/static/js/pages/settings/components/tools-tab.jscrates/ironclaw_webui_v2/static/js/pages/settings/components/tools-tab.test.mjscrates/ironclaw_webui_v2/static/js/pages/settings/hooks/useTools.jscrates/ironclaw_webui_v2/static/js/pages/settings/hooks/useTools.test.mjs
|
🚅 Deployed to the ironclaw-pr-5699 environment in ironclaw-ci-preview
|
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Verdict: ✅ Approved
Findings: 0 blocking / 0 notes
Next: No reviewer action needed.
Head: 92b4d7b70dede0a407952b1062e0a4bd611a6bdc
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
No concrete blocking issues found in the PR diff. The change surfaces failed tool-permission saves, resets the previous mutation error before retry, and adds focused unit/e2e coverage for the regression path.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloop review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloop reviewwhen the fix may affect multiple areas. - Use
@ironloop statusto check queued/running/completed/stale/stalled state while reviewers run.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_v2/static/js/test-utils/source-for-test.mjs`:
- Around line 1-19: The export-stripping logic in sourceForTest only rewrites
export function declarations, so other export forms can slip through and break
vm.runInNewContext as invalid script syntax. Update sourceForTest to strip all
export variants it may encounter in the loaded source, not just function
declarations, and keep the transformation centered in the line-processing loop
that builds lines before __testExports is appended.
🪄 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: 0f13fff1-410d-4951-ba05-96aac25cfff2
📒 Files selected for processing (3)
crates/ironclaw_webui_v2/static/js/pages/settings/components/tools-tab.test.mjscrates/ironclaw_webui_v2/static/js/pages/settings/hooks/useTools.test.mjscrates/ironclaw_webui_v2/static/js/test-utils/source-for-test.mjs
github-actions
Bot
added
size: L
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Verdict: ✅ Approved
Findings: 0 blocking / 0 notes
Next: No reviewer action needed.
Head: 186da7c86fd6d005bac2699f77615fa2fb78cfe0
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
No concrete, actionable regressions found in the PR. The change surfaces tool permission save failures in the settings UI and adds focused unit/E2E coverage for that behavior.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloop review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloop reviewwhen the fix may affect multiple areas. - Use
@ironloop statusto check queued/running/completed/stale/stalled state while reviewers run.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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_v2/static/js/test-utils/source-for-test.mjs`:
- Around line 3-14: The export stripping logic in stripExport only removes the
first line of a multi-line brace export, leaving continuation lines behind as
invalid statements. Update stripExport to track multi-line export blocks,
similar to the import-skipping behavior, so all lines in an export { ... } list
are omitted until the closing brace and semicolon are reached.
- Around line 3-18: The default-export rewriting in stripExport is inconsistent
for named default functions/classes because it preserves the original identifier
while exportBinding("default") still expects __defaultExport. Update stripExport
so named default function/class declarations are aliased to __defaultExport in
the same way as anonymous defaults, and add a regression test covering export
default function foo() {} and export default class Foo {} when requesting the
"default" binding.
In `@crates/ironclaw_webui_v2/static/js/test-utils/source-for-test.test.mjs`:
- Around line 11-47: The current test in sourceForTest only covers anonymous
default exports, so it misses the __defaultExport bug for named default
declarations. Update the sourceForTest fixture test to include a named default
export case in fixture.js, such as a default function or class, and request
"default" in the exportNames passed to sourceForTest; then assert the default
export is available through __testExports so the ReferenceError regression is
covered.
🪄 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: ba4af55f-ca0e-4786-a7ff-1f29b9fad075
📒 Files selected for processing (2)
crates/ironclaw_webui_v2/static/js/test-utils/source-for-test.mjscrates/ironclaw_webui_v2/static/js/test-utils/source-for-test.test.mjs
github-actions
Bot
added
size: M
This branch was successfully deployed
2 participants
Summary
error.saveFailedcopy so failed per-tool saves show a visible alert.Linked Issue
Closes #5698
Validation
node --test crates/ironclaw_webui_v2/static/js/pages/settings/components/tools-tab.test.mjsnode --test crates/ironclaw_webui_v2/static/js/pages/settings/**/*.test.mjsIRONCLAW_WEBUI_V2_DIST_DIR=/tmp/ironclaw-webui-v2-issue-5698 npm run buildpython -m py_compile tests/e2e/scenarios/test_reborn_webui_v2_legacy_tool_permissions.pySecurity Impact
No security-sensitive behavior changes. This only surfaces an already-failed authenticated settings mutation to the user.
Database Impact
No schema or migration changes.
Blast Radius
Limited to WebUI v2 Settings → Tools error rendering and its frontend/e2e regression coverage.
Rollback Plan
Revert this PR to return failed per-tool permission saves to the prior silent UI behavior.
Review track: A