Repository navigation
[codex] Port Reborn WebUI projects and settings coverage - #5375
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds project and settings UI identification attributes, wires project thread listing to the v2 API, changes project chat navigation to thread-path routing, wires a settings toolbar into SettingsPage, and adds Playwright coverage for projects, settings search, skills, and tool permissions. ChangesProjects Page: API wiring, navigation, and selectors
Settings Page: toolbar wiring, component selectors, and E2E coverage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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 |
github-actions
Bot
added
risk: low
![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 introduces several WebChat v2 updates, including project-filtered thread listing, a settings toolbar with search and import/export capabilities, fallback channel display names, and updated navigation logic when creating chat threads. It also adds extensive E2E test coverage for legacy projects, settings search, skills, and tool permissions, along with supporting data-testid attributes. Feedback was provided on projects-page.js to ensure navigation only occurs when a thread ID is successfully created, preventing the app from navigating away and hiding error banners on failure.
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.
| navigate(nextThreadId ? `/chat/${nextThreadId}` : "/chat", { | ||
| state: { | ||
| composerDraft: t("projects.creationDraft"), | ||
| threadId: nextThreadId, | ||
| }, | ||
| }); | ||
| }, [navigate, threadsState]); | ||
| }, [navigate, threadsState, t]); |
There was a problem hiding this comment.
If threadsState.createThread() fails, an error is caught and chatFlowError is set. However, because navigate is called unconditionally, the application will immediately navigate to /chat, unmounting the ProjectsPage and discarding the local error state before the user can see the feedback banner.
To fix this, we should only navigate if nextThreadId was successfully created, allowing the error banner to remain visible on failure.
if (nextThreadId) {
navigate("/chat/" + nextThreadId, {
state: {
composerDraft: t("projects.creationDraft"),
},
});
}
}, [navigate, threadsState, t]);
c7cbcfc to
7250adb
Compare
github-actions
Bot
added
size: M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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_v2_static/static/js/pages/projects/projects-page.js (1)
57-74: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
handleCreateProjectnavigates away even when thread creation fails, discarding the error.On a
createThread()throw,setChatFlowError(...)fires but the unconditionalnavigate(...)at Line 69 still runs, unmountingProjectsPagebefore itsFeedbackBanner(Line 169) can render.handleStartConversationcorrectly keepsnavigateinside thetry. Mirror that here so failures stay on the projects page with the banner visible.Proposed fix
- try { - nextThreadId = await threadsState.createThread(); - } catch (error) { - setChatFlowError({ - type: "error", - message: error.message || t("projects.chatAutoFail"), - }); - } - - navigate(nextThreadId ? `/chat/${nextThreadId}` : "/chat", { - state: { - composerDraft: t("projects.creationDraft"), - }, - }); + try { + nextThreadId = await threadsState.createThread(); + } catch (error) { + setChatFlowError({ + type: "error", + message: error.message || t("projects.chatAutoFail"), + }); + return; + } + + navigate(nextThreadId ? `/chat/${nextThreadId}` : "/chat", { + state: { + composerDraft: t("projects.creationDraft"), + }, + });🤖 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_v2_static/static/js/pages/projects/projects-page.js` around lines 57 - 74, `handleCreateProject` currently navigates unconditionally after `threadsState.createThread()`, which causes the page to unmount even when the thread creation fails. Move the `navigate(...)` call in `handleCreateProject` so it only runs after a successful `createThread()` result, matching the `handleStartConversation` pattern, and keep failures on `ProjectsPage` so `setChatFlowError(...)` can be shown by `FeedbackBanner`.
🤖 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/static/js/pages/settings/settings-page.js`:
- Around line 111-119: The settings page is showing import failures through the
save-error path, which mislabels the message in the UI. Update the error
handling in settings-page.js so the banner tied to
SettingsToolbar/importSettings does not reuse saveError from useSettings;
instead, split save and import error state in useSettings or render a neutral
settings-operation error when importMutation.error is present. Use the existing
useSettings hook and the settings page banner logic to route import failures to
the correct message.
In `@tests/e2e/scenarios/test_reborn_webui_v2_legacy_projects.py`:
- Around line 66-225: The setup helper leaks a Playwright browser context when
navigation or the initial grid assertion fails before returning. Update
_open_mocked_projects_page() so it always closes the created context on any
exception from page.goto() or the expect(page.locator(...)) wait, while still
returning the context normally on success. Use the existing context and page
setup in _open_mocked_projects_page and keep the cleanup local to this helper so
callers do not have to change.
In `@tests/e2e/scenarios/test_reborn_webui_v2_legacy_settings_search.py`:
- Around line 321-324: The `_provider_card` helper and related v2 settings
locators are hardcoding `data-testid` selectors, which violates the e2e selector
invariant. Move the raw selector strings into `helpers.SEL_V2` and update the
affected helpers and tests to reference those shared constants instead of inline
selectors. Apply the same cleanup pattern to the other v2 UI locators mentioned
in the review, such as the search placeholder and `llm-provider-disclosure`, so
all selectors are sourced consistently from `SEL_V2`.
In `@tests/e2e/scenarios/test_reborn_webui_v2_legacy_skills.py`:
- Around line 203-217: The delete-confirmation dialog handling in
test_reborn_webui_v2_legacy_skills.py currently schedules dialog.accept() on an
unawaited task inside handle_dialog, which can make the test flaky. Update the
dialog callback used with page.once("dialog", ...) so the acceptance runs on the
awaited path for dialog handling rather than via loop.create_task, while keeping
the dialog_future assertion flow in place. Use the existing handle_dialog
callback and dialog.accept call site to locate the fix.
---
Outside diff comments:
In `@crates/ironclaw_webui_v2_static/static/js/pages/projects/projects-page.js`:
- Around line 57-74: `handleCreateProject` currently navigates unconditionally
after `threadsState.createThread()`, which causes the page to unmount even when
the thread creation fails. Move the `navigate(...)` call in
`handleCreateProject` so it only runs after a successful `createThread()`
result, matching the `handleStartConversation` pattern, and keep failures on
`ProjectsPage` so `setChatFlowError(...)` can be shown by `FeedbackBanner`.
🪄 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: dbf347ff-a8c2-4cc3-a86c-e87b87fe9abb
📒 Files selected for processing (12)
crates/ironclaw_webui_v2_static/static/js/pages/projects/components/project-filesystem-panel.jscrates/ironclaw_webui_v2_static/static/js/pages/projects/components/project-workspace-shell.jscrates/ironclaw_webui_v2_static/static/js/pages/projects/components/projects-grid.jscrates/ironclaw_webui_v2_static/static/js/pages/projects/lib/projects-api.jscrates/ironclaw_webui_v2_static/static/js/pages/projects/projects-page.jscrates/ironclaw_webui_v2_static/static/js/pages/settings/components/channels-tab.jscrates/ironclaw_webui_v2_static/static/js/pages/settings/components/tools-tab.jscrates/ironclaw_webui_v2_static/static/js/pages/settings/settings-page.jstests/e2e/scenarios/test_reborn_webui_v2_legacy_projects.pytests/e2e/scenarios/test_reborn_webui_v2_legacy_settings_search.pytests/e2e/scenarios/test_reborn_webui_v2_legacy_skills.pytests/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 (2)
tests/e2e/scenarios/test_reborn_webui_v2_legacy_settings_search.py (1)
235-258: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReturn the real LLM snapshot shape from write mocks.
The upstream v2 handlers return
LlmConfigSnapshotfor provider upsert, active-provider changes, and delete, but these mocks return{provider},{active}, and{success}. That lets provider flows pass against response bodies production never sends.Proposed fix
+ def snapshot() -> dict: + return { + "providers": llm_state["providers"], + "active": llm_state.get("active"), + } + if path == "/api/webchat/v2/llm/providers" and method == "GET": await fulfill_json( route, - { - "providers": llm_state["providers"], - "active": llm_state.get("active"), - }, + snapshot(), ) return @@ - await fulfill_json(route, {"provider": provider}) + await fulfill_json(route, snapshot()) return @@ - await fulfill_json(route, {"active": llm_state["active"]}) + await fulfill_json(route, snapshot()) return @@ llm_state["providers"] = [ provider for provider in llm_state["providers"] if provider["id"] != provider_id ] - await fulfill_json(route, {"success": True}) + if (llm_state.get("active") or {}).get("provider_id") == provider_id: + llm_state["active"] = None + await fulfill_json(route, snapshot()) returnAs per coding guidelines, "When mocking a browser/runtime API in a test, the mock signature must match the production call signature, and assertions must cover every argument passed by production code."
Also applies to: 282-295
🤖 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/e2e/scenarios/test_reborn_webui_v2_legacy_settings_search.py` around lines 235 - 258, The mocked write handlers for LLM config changes return simplified bodies that do not match the real v2 API response shape. Update the mocks in the test scenario around the provider upsert and active-provider handlers to return the same LlmConfigSnapshot-style payloads produced by the upstream endpoints, and apply the same fix to the delete flow referenced in the review. Keep the existing llm_state updates, but make the responses mirror production so assertions in the provider flow are exercised against realistic data.Source: Coding guidelines
tests/e2e/scenarios/test_reborn_webui_v2_legacy_tool_permissions.py (1)
219-251: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse
tests/e2e/helpers.py:sse_stream()for this gate stream
tests/e2e/CLAUDE.mdroutes raw SSE through the shared helper; this hand-rolledhttpx.AsyncClientstream bypasses thataiohttppath and duplicates frame parsing.🤖 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/e2e/scenarios/test_reborn_webui_v2_legacy_tool_permissions.py` around lines 219 - 251, The gate-prompt wait helper is bypassing the shared SSE path by opening a raw httpx stream and manually parsing frames. Update _wait_for_gate_prompt_after_send to use tests/e2e/helpers.py:sse_stream() instead, and keep the gate-event extraction logic there so SSE handling stays centralized and consistent with the rest of the e2e tests.Sources: Coding guidelines, Path instructions
🤖 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 `@tests/e2e/scenarios/test_reborn_webui_v2_legacy_settings_search.py`:
- Around line 235-258: The mocked write handlers for LLM config changes return
simplified bodies that do not match the real v2 API response shape. Update the
mocks in the test scenario around the provider upsert and active-provider
handlers to return the same LlmConfigSnapshot-style payloads produced by the
upstream endpoints, and apply the same fix to the delete flow referenced in the
review. Keep the existing llm_state updates, but make the responses mirror
production so assertions in the provider flow are exercised against realistic
data.
In `@tests/e2e/scenarios/test_reborn_webui_v2_legacy_tool_permissions.py`:
- Around line 219-251: The gate-prompt wait helper is bypassing the shared SSE
path by opening a raw httpx stream and manually parsing frames. Update
_wait_for_gate_prompt_after_send to use tests/e2e/helpers.py:sse_stream()
instead, and keep the gate-event extraction logic there so SSE handling stays
centralized and consistent with the rest of the e2e tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9e6ee845-12cd-415f-91a3-4dc54040585d
📒 Files selected for processing (8)
crates/ironclaw_webui_v2_static/static/js/pages/projects/projects-page.jscrates/ironclaw_webui_v2_static/static/js/pages/settings/hooks/useSettings.jscrates/ironclaw_webui_v2_static/static/js/pages/settings/settings-page.jstests/e2e/helpers.pytests/e2e/scenarios/test_reborn_webui_v2_legacy_projects.pytests/e2e/scenarios/test_reborn_webui_v2_legacy_settings_search.pytests/e2e/scenarios/test_reborn_webui_v2_legacy_skills.pytests/e2e/scenarios/test_reborn_webui_v2_legacy_tool_permissions.py
Summary
Change Type
Linked Issue
Security Impact
Trust-Boundary Checklist
Database Impact
Blast Radius
Rollback Plan
Follow-up
Validation
python3 -m py_compile tests/e2e/helpers.py tests/e2e/scenarios/test_reborn_webui_v2_legacy_projects.py tests/e2e/scenarios/test_reborn_webui_v2_legacy_settings_search.py tests/e2e/scenarios/test_reborn_webui_v2_legacy_skills.py tests/e2e/scenarios/test_reborn_webui_v2_legacy_tool_permissions.pynode --check crates/ironclaw_webui_v2_static/static/js/pages/projects/projects-page.js crates/ironclaw_webui_v2_static/static/js/pages/settings/hooks/useSettings.js crates/ironclaw_webui_v2_static/static/js/pages/settings/settings-page.jsnode --test crates/ironclaw_webui_v2_static/static/js/pages/settings/components/tools-tab.test.mjs crates/ironclaw_webui_v2_static/static/js/pages/settings/components/provider-components.test.mjs crates/ironclaw_webui_v2_static/static/js/pages/settings/components/skill-install-panel.test.mjs crates/ironclaw_webui_v2_static/static/js/pages/settings/lib/api-result.test.mjs crates/ironclaw_webui_v2_static/static/js/pages/settings/lib/settings-api.test.mjs crates/ironclaw_webui_v2_static/static/js/pages/settings/lib/llm-providers.test.mjs(51 passed)cargo fmt --all -- --check