Repository navigation
feat(reborn): replace tool permission selects with custom menu - #5769
Conversation
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted review state before this projection. |
|
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 new ChangesSelectMenu rollout
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsUI
participant SelectMenuTrigger
participant SelectMenuListbox
participant PermissionAPI
User->>SettingsUI: open Tools settings
SettingsUI->>SelectMenuTrigger: render current permission label
User->>SelectMenuTrigger: click / ArrowDown
SelectMenuTrigger->>SelectMenuListbox: open listbox, set aria-expanded
User->>SelectMenuListbox: select option (click/Enter)
SelectMenuListbox->>SettingsUI: onChange(newPermission)
SettingsUI->>PermissionAPI: save permission
alt save succeeds
PermissionAPI-->>SettingsUI: success
SettingsUI->>SelectMenuTrigger: update label
else save fails
PermissionAPI-->>SettingsUI: error "Permission denied"
SettingsUI->>SelectMenuTrigger: revert label, show alert
end
Possibly related PRs
🚥 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 |
|
@claude review |
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Verdict: ✅ Approved
Findings: 0 blocking / 0 notes
Next: No reviewer action needed.
Head: c7fb8dad751a9f8456cb2f56d7ab8b3bc481068d
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
No blocking correctness, security, or coverage issues found in the reviewed PR. The new select-menu behavior is covered by focused JS tests and the updated Reborn E2E manifest includes the persisted permission-menu scenario.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas. - Use
@ironloopai statusto check queued/running/completed/stale/stalled state while reviewers run.
This comment was marked as resolved.
This comment was marked as resolved.
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.16% — 282153 / 331314 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)
|
|
@claude review |
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/design-system/select-menu.js`:
- Around line 167-173: The outside-click path in the select menu still invokes
closeMenu() with the default focus-restoring behavior, so update the
outsideClickEntryRef.current.close callback in select-menu.js to call closeMenu
with restoreFocus disabled. Keep the Escape/keyboard close path unchanged so it
can still restore focus, and make sure the change is applied where the
outside-click handler is wired up.
🪄 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: b6894423-3c15-4037-8ac6-efda6e313dec
📒 Files selected for processing (3)
crates/ironclaw_webui_v2/static/js/design-system/select-menu.jscrates/ironclaw_webui_v2/static/js/design-system/select-menu.test.mjstests/e2e/scenarios/test_reborn_webui_v2_legacy_tool_permissions.py
This comment was marked as resolved.
This comment was marked as resolved.
|
🚅 Deployed to the ironclaw-pr-5769 environment in ironclaw-ci-preview
|
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/design-system/select-menu.js`:
- Around line 198-202: The select menu button props currently always include
aria-owns even when the listbox is closed, leaving an IDREF to an element that
is not rendered. Update the buttonListboxProps logic in the select menu
component so aria-owns is only applied when open, or remove it entirely since
aria-controls already handles the open relationship and the listbox is a
sibling. Keep the open-state and activeOptionId handling in the same
buttonListboxProps object.
🪄 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: 4e097971-6872-442a-8b8d-a906b9f8feb7
📒 Files selected for processing (2)
crates/ironclaw_webui_v2/static/js/design-system/select-menu.jscrates/ironclaw_webui_v2/static/js/design-system/select-menu.test.mjs
# Conflicts: # crates/ironclaw_webui_v2/frontend/src/pages/settings/components/tools-tab.tsx
Summary
SelectMenucomponent with keyboard navigation, outside-click dismissal, selected-state styling, and accessible listbox semantics.Linked Issue
Closes #5770
Validation
node --test crates/ironclaw_webui_v2/static/js/design-system/select-menu.test.mjs crates/ironclaw_webui_v2/static/js/pages/settings/components/tools-tab.test.mjsnpm run buildfromcrates/ironclaw_webui_v2/frontendgit diff --checktests/e2e/.venv/bin/python -m py_compile tests/e2e/helpers.py tests/e2e/scenarios/test_reborn_webui_v2_legacy_tool_permissions.pytests/e2e/.venv/bin/pytest --collect-only -q tests/e2e/scenarios/test_reborn_webui_v2_legacy_tool_permissions.py::test_reborn_legacy_tool_permission_menu_persists_after_reloadSecurity Impact
No. This only changes Reborn WebUI v2 presentation and client-side interaction for tool permission controls.
Database Impact
No schema or migration changes.
Blast Radius
Limited to the Reborn WebUI v2 Tools settings permission control, the shared static design-system select component, and related unit/E2E coverage.
Rollback Plan
Revert this PR to restore the previous native browser select controls for tool permissions.