feat(gateway): unified settings page with subtabs#1191
feat(gateway): unified settings page with subtabs#1191ilblackdragon wants to merge 12 commits intostagingfrom
Conversation
github-actions
bot
added
scope: channel/web
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a comprehensive overhaul of the web gateway's settings and configuration pages. The primary goal is to enhance usability and clarity by consolidating various settings into a new, well-organized 'Settings' tab with distinct sub-panels. This refactoring improves the visual presentation of configuration options, streamlines the management of extensions and channels, and removes unnecessary debug information from the user interface. It also lays the groundwork for more robust settings management with import/export capabilities and a standardized confirmation flow for user actions. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 a significant polish to the settings page UI, refactoring the previous 'Extensions' and 'Skills' tabs into a unified 'Settings' area with sub-tabs. The debug 'Registered Tools' section has been removed, and the associated backend and frontend code cleaned up. The changes are well-structured, using a data-driven approach for the new settings UI and improving user experience with loading skeletons and better confirmation dialogs. I've identified a couple of areas for minor refactoring in the JavaScript to improve code reuse and remove dead code.
src/channels/web/static/app.js
Outdated
| if (currentTab === 'settings' && currentSettingsSubtab === 'extensions') loadExtensions(); | ||
| if (currentTab === 'settings' && currentSettingsSubtab === 'mcp') loadMcpServers(); | ||
| if (currentTab === 'settings' && currentSettingsSubtab === 'channels') loadChannelsStatus(); |
There was a problem hiding this comment.
You've introduced a great helper function refreshCurrentSettingsTab() later in the file. To improve maintainability and reduce duplication, you can use it here as well.
| if (currentTab === 'settings' && currentSettingsSubtab === 'extensions') loadExtensions(); | |
| if (currentTab === 'settings' && currentSettingsSubtab === 'mcp') loadMcpServers(); | |
| if (currentTab === 'settings' && currentSettingsSubtab === 'channels') loadChannelsStatus(); | |
| if (currentTab === 'settings') refreshCurrentSettingsTab(); |
There was a problem hiding this comment.
Done — replaced with if (currentTab === 'settings') refreshCurrentSettingsTab(); in f8ffd9e.
src/channels/web/static/app.js
Outdated
| if (currentTab === 'settings' && currentSettingsSubtab === 'extensions') loadExtensions(); | ||
| if (currentTab === 'settings' && currentSettingsSubtab === 'mcp') loadMcpServers(); | ||
| if (currentTab === 'settings' && currentSettingsSubtab === 'channels') loadChannelsStatus(); |
There was a problem hiding this comment.
Similar to the previous comment, you can use the refreshCurrentSettingsTab() helper function here to avoid code duplication.
| if (currentTab === 'settings' && currentSettingsSubtab === 'extensions') loadExtensions(); | |
| if (currentTab === 'settings' && currentSettingsSubtab === 'mcp') loadMcpServers(); | |
| if (currentTab === 'settings' && currentSettingsSubtab === 'channels') loadChannelsStatus(); | |
| if (currentTab === 'settings') refreshCurrentSettingsTab(); |
src/channels/web/static/app.js
Outdated
| function formatGroupName(name) { | ||
| return name.charAt(0).toUpperCase() + name.slice(1).replace(/_/g, ' '); | ||
| } | ||
|
|
||
| function formatSettingLabel(name) { | ||
| var s = name.replace(/_/g, ' '); | ||
| return s.charAt(0).toUpperCase() + s.slice(1); | ||
| } |
There was a problem hiding this comment.
These helper functions formatGroupName and formatSettingLabel appear to be unused. To keep the codebase clean, it's best to remove them.
- Backend: add ActiveConfigSnapshot to expose resolved LLM backend, model, and enabled channels via /api/gateway/status - Add missing Agent settings (daily cost cap, actions/hour, local tools) - Add Sandbox, Routines, Safety, Skills, and Search setting groups - Settings import/export (JSON download + file upload) - Active env defaults shown as placeholders in Inference settings - Styled confirmation modals replace window.confirm() for remove actions - Global restart banner persists across settings subtab switches - Client-side validation with min/max constraints on number inputs - Accessibility: aria-label on inputs, role=status on save indicators - Settings search filters rows across current subtab - Smooth CSS transitions for conditional field visibility (showWhen) - Tunnel settings in Channels subtab - Mobile responsive settings layout at 768px breakpoint - i18n keys for toolbar, search, and import/export in en + zh-CN Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… section Remove the "Registered Tools" table from the extensions tab (debug info not useful to end users), clean up associated CSS/i18n/JS. Additional settings page UI polish: extension card state styling, layout refinements. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ilblackdragon
force-pushed
the
settings-page-polish
branch
from
de532c8 to
452da4b
Compare
ilblackdragon
changed the title
There was a problem hiding this comment.
Pull request overview
Updates the web gateway’s configuration UI by consolidating Extensions/Skills into a new Settings tab layout, enhancing extension card styling, and removing the end-user-facing “Registered Tools” debug section. It also wires the frontend to the settings import/export APIs and exposes resolved runtime config via /api/gateway/status.
Changes:
- Replace separate Extensions/Skills top-level tabs with a Settings tab + subtabs (Inference/Agent/Channels/Networking/Extensions/MCP/Skills).
- Remove “Registered Tools” UI, associated CSS, i18n keys, and frontend fetch/render logic.
- Add
ActiveConfigSnapshotto gateway state and extend/api/gateway/statuswith LLM + enabled-channels info.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ws_gateway_integration.rs | Update GatewayState construction with new active_config field. |
| tests/support/gateway_workflow_harness.rs | Update GatewayState construction with new active_config field. |
| tests/openai_compat_integration.rs | Update GatewayState construction with new active_config field in test servers. |
| src/main.rs | Ensure WASM channels dir exists; populate gateway active_config snapshot. |
| src/channels/web/ws.rs | Update test GatewayState with active_config. |
| src/channels/web/test_helpers.rs | Update TestGatewayBuilder state with active_config. |
| src/channels/web/static/style.css | Add Settings layout/styles, extension card state styling, and remove tools table CSS. |
| src/channels/web/static/index.html | Introduce Settings tab layout + confirmation modal; remove Extensions/Skills top-level tabs and tools table section. |
| src/channels/web/static/i18n/zh-CN.js | Add Settings-related keys; remove Registered Tools keys; adjust extensions labels. |
| src/channels/web/static/i18n/en.js | Add Settings-related keys; remove Registered Tools keys; adjust extensions labels. |
| src/channels/web/static/app.js | Add Settings subtab routing, settings import/export/search, channels status UI; remove tools fetch/render. |
| src/channels/web/server.rs | Add ActiveConfigSnapshot to GatewayState; extend /api/gateway/status response. |
| src/channels/web/mod.rs | Add builder method with_active_config and plumb snapshot through state rebuild. |
Comments suppressed due to low confidence (3)
src/channels/web/static/app.js:4341
renderCardsSkeleton()wraps skeleton cards in a new<div class="extensions-list">…</div>. Several callers (e.g.#extensions-list,#skills-list,#mcp-servers-list) are already.extensions-listcontainers, so this creates nested.extensions-listelements and can break layout/styling. Consider returning only the card markup (no outer wrapper) or making the wrapper optional based on the caller.
e.preventDefault();
if (currentTab === 'memory') {
document.getElementById('memory-search').focus();
} else {
document.getElementById('chat-input').focus();
}
return;
src/channels/web/static/app.js:5032
- Export/import UX strings are hard-coded in English (and export doesn’t surface any success toast), even though i18n keys like
settings.exportSuccess,settings.importSuccess, andsettings.importFailedwere added. To meet the PR’s i18n test plan, useI18n.t(...)for these messages and show a localized success toast after export.
},
];
function loadNetworkingSettings() {
var container = document.getElementById('settings-networking-content');
container.innerHTML = renderSettingsSkeleton(4);
apiFetch('/api/settings/export').then(function(data) {
var settings = data.settings || {};
container.innerHTML = '';
renderStructuredSettingsInto(container, NETWORKING_SETTINGS, settings, {});
}).catch(function(err) {
container.innerHTML = '<div class="empty-state">Failed to load settings: '
+ escapeHtml(err.message) + '</div>';
});
}
function formatGroupName(name) {
return name.charAt(0).toUpperCase() + name.slice(1).replace(/_/g, ' ');
}
function formatSettingLabel(name) {
var s = name.replace(/_/g, ' ');
return s.charAt(0).toUpperCase() + s.slice(1);
}
// --- Toasts ---
function showToast(message, type) {
const container = document.getElementById('toasts');
const toast = document.createElement('div');
toast.className = 'toast toast-' + (type || 'info');
toast.textContent = message;
container.appendChild(toast);
// Trigger slide-in
requestAnimationFrame(() => toast.classList.add('visible'));
setTimeout(() => {
toast.classList.remove('visible');
src/channels/web/static/app.js:4229
- The structured settings definitions introduce many user-visible labels/descriptions as hard-coded English strings (group names, labels, descriptions). This prevents the Settings tab from being fully localized for zh-CN as described in the test plan. Consider moving these strings into i18n keys and referencing them via
I18n.t(...)when rendering rows/groups.
installBtn.addEventListener('click', (function(s, btn) {
return function() {
if (!confirm('Install skill "' + s + '" from ClawHub?')) return;
btn.disabled = true;
btn.textContent = I18n.t('extensions.installing');
installSkill(s, null, btn);
};
})(slug, installBtn));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/channels/web/static/app.js
Outdated
| label.textContent = active ? 'Active' : 'Inactive'; | ||
| actions.appendChild(label); | ||
| card.appendChild(actions); | ||
|
|
||
| return card; | ||
| } | ||
|
|
||
| // --- Networking Settings --- | ||
|
|
There was a problem hiding this comment.
Good catch — removed data-i18n="btn.confirm" from the button in a7aeff4. The label is now always set dynamically by showConfirmModal().
| return e.kind !== 'mcp_server' && e.kind !== 'wasm_channel' && e.kind !== 'channel' && !e.installed; | ||
| }); | ||
|
|
||
| // Available WASM extensions | ||
| var wasmSection = document.getElementById('available-wasm-section'); | ||
| if (wasmEntries.length === 0) { |
There was a problem hiding this comment.
Fixed — Configure/Reconfigure now uses I18n.t('ext.configure') / I18n.t('ext.reconfigure') (keys already existed). Done in a7aeff4.
| <button class="settings-subtab" data-settings-subtab="skills" data-i18n="tab.skills">Skills</button> | ||
| </div> | ||
| <div class="extensions-section" id="available-wasm-section"> | ||
| <h3 data-i18n="extensions.available">Available WASM Extensions</h3> | ||
| <div class="extensions-list" id="available-wasm-list"> | ||
| <div class="empty-state" data-i18n="common.loading">Loading...</div> | ||
| <div class="settings-content"> |
There was a problem hiding this comment.
Added data-i18n-placeholder="settings.searchPlaceholder" to the input in a7aeff4.
- Use refreshCurrentSettingsTab() in SSE event handlers to reduce duplication - Remove unused formatGroupName/formatSettingLabel helpers - Use i18n keys for MCP Configure/Reconfigure buttons - Add data-i18n-placeholder to settings search input - Remove data-i18n from confirm modal button (set dynamically by showConfirmModal) - Fix cargo fmt in main.rs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…on-check] - Update TABS list: replace extensions/skills with settings - Add settings_subtab/settings_subpanel selectors to helpers - Update test_connection, test_skills, test_extensions, test_wasm_lifecycle to navigate via Settings > subtab instead of top-level tabs - Move MCP card tests to use go_to_mcp() helper (MCP is now a separate subtab) - Remove tools table tests and mock_ext_apis tools= parameter - Fix CSP violation: replace inline onclick on confirm modal cancel button Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR consolidates the web gateway’s Extensions/Skills UI into a single Settings tab with sidebar subtabs, adds settings import/export + search, and extends the gateway status payload to support richer “active config” display.
Changes:
- Replaces top-level Extensions/Skills tabs with a unified Settings tab + subtabs (Inference/Agent/Channels/Networking/Extensions/MCP/Skills).
- Adds frontend structured settings editor (load/save/delete), import/export, search UI, and a reusable confirmation modal for destructive actions.
- Extends
/api/gateway/statusto include anactive_configsnapshot (LLM backend/model + enabled channels) and updates tests accordingly.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ws_gateway_integration.rs | Adds active_config to test gateway state construction. |
| tests/support/gateway_workflow_harness.rs | Adds active_config to harness gateway state. |
| tests/openai_compat_integration.rs | Adds active_config to OpenAI-compat test gateway state. |
| tests/e2e/scenarios/test_wasm_lifecycle.py | Updates navigation to Settings > Extensions subtab. |
| tests/e2e/scenarios/test_skills.py | Updates navigation to Settings > Skills subtab via helper. |
| tests/e2e/scenarios/test_extensions.py | Updates Extensions/MCP E2E tests for Settings subtabs and removes tools-table checks. |
| tests/e2e/helpers.py | Adds Settings subtab selectors; updates top-level tabs list. |
| src/main.rs | Ensures WASM channels dir exists; injects ActiveConfigSnapshot into gateway. |
| src/channels/web/ws.rs | Updates web channel test state with active_config. |
| src/channels/web/test_helpers.rs | Updates TestGatewayBuilder to set default active_config. |
| src/channels/web/static/style.css | Adds Settings layout/toolbar/modal/skeleton styling and refines extension card styling. |
| src/channels/web/static/index.html | Introduces Settings tab layout, subtabs, toolbar, and confirm modal markup. |
| src/channels/web/static/i18n/en.js | Adds settings-related i18n keys; updates extensions strings; removes tools keys. |
| src/channels/web/static/i18n/zh-CN.js | Adds settings-related i18n keys; updates extensions strings; removes tools keys. |
| src/channels/web/static/app.js | Implements settings subtabs, structured settings editor, channels/MCP views, confirm modal, and 204 handling. |
| src/channels/web/server.rs | Adds ActiveConfigSnapshot + includes it in /api/gateway/status. |
| src/channels/web/mod.rs | Adds with_active_config() builder method and stores snapshot in gateway state. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/channels/web/static/app.js
Outdated
| mcpList.innerHTML = '<div class="empty-state">No MCP servers available</div>'; | ||
| } | ||
| }).catch(function(err) { | ||
| mcpList.innerHTML = '<div class="empty-state">Failed to load MCP servers: ' | ||
| + escapeHtml(err.message) + '</div>'; |
There was a problem hiding this comment.
Fixed — empty state now uses I18n.t('mcp.noServers') and error uses I18n.t('common.loadFailed'). Done in 93cd14b.
src/channels/web/static/app.js
Outdated
| }).catch(function(err) { | ||
| showToast('Export failed: ' + err.message, 'error'); |
There was a problem hiding this comment.
Fixed — export now shows I18n.t('settings.exportSuccess') on success and uses i18n for the error toast. Done in 93cd14b.
| }).then(function() { | ||
| showToast('Settings imported successfully', 'success'); | ||
| loadSettingsSubtab(currentSettingsSubtab); | ||
| }).catch(function(err) { | ||
| showToast('Import failed: ' + err.message, 'error'); | ||
| }); | ||
| } catch (e) { | ||
| showToast('Invalid JSON file', 'error'); | ||
| } |
There was a problem hiding this comment.
Fixed — all three import toasts now use I18n.t('settings.importSuccess') and I18n.t('settings.importFailed', { message }). Done in 93cd14b.
src/channels/web/static/index.html
Outdated
| <div class="settings-content"> | ||
| <div class="settings-toolbar"> | ||
| <div class="settings-search"> | ||
| <input type="text" id="settings-search-input" data-i18n-placeholder="settings.searchPlaceholder" placeholder="Search settings..." aria-label="Search settings"> |
There was a problem hiding this comment.
Fixed — added data-i18n-attr="aria-label" + data-i18n="settings.searchPlaceholder" so both the placeholder and aria-label get translated. Done in 93cd14b.
| Promise.all([ | ||
| apiFetch('/api/gateway/status').catch(function() { return {}; }), | ||
| apiFetch('/api/extensions').catch(function() { return { extensions: [] }; }), | ||
| apiFetch('/api/extensions/registry').catch(function() { return { entries: [] }; }), |
There was a problem hiding this comment.
Good catch — there is no /api/channels/status endpoint; the channels subtab fetches from /api/gateway/status + extensions APIs. Updated the PR description to reflect the actual implementation.
| var rows = activePanel.querySelectorAll('.settings-row'); | ||
| var visibleCount = 0; | ||
| rows.forEach(function(row) { | ||
| var text = row.textContent.toLowerCase(); | ||
| if (query === '' || text.indexOf(query) !== -1) { | ||
| row.classList.remove('search-hidden'); | ||
| visibleCount++; | ||
| } else { |
There was a problem hiding this comment.
Fixed — visibleCount now only increments for rows that are not .hidden (i.e. not hidden by showWhen conditions). Done in 93cd14b.
src/channels/web/static/app.js
Outdated
| builtinList.appendChild(renderBuiltinChannelCard( | ||
| 'CLI', | ||
| 'Terminal UI with Ratatui', | ||
| enabledChannels.indexOf('repl') !== -1, |
There was a problem hiding this comment.
Fixed — CLI card now checks for 'cli' channel key instead of 'repl'. Done in 93cd14b.
src/channels/web/static/app.js
Outdated
| group: 'LLM Provider', | ||
| settings: [ | ||
| { key: 'llm_backend', label: 'Backend', description: 'LLM inference provider', | ||
| type: 'select', options: ['nearai', 'anthropic', 'openai', 'ollama', 'openai_compatible', 'tinfoil', 'bedrock'] }, | ||
| { key: 'selected_model', label: 'Model', description: 'Model name or ID for the selected backend', type: 'text' }, | ||
| { key: 'ollama_base_url', label: 'Ollama URL', description: 'Base URL for Ollama API', type: 'text', | ||
| showWhen: { key: 'llm_backend', value: 'ollama' } }, | ||
| { key: 'openai_compatible_base_url', label: 'OpenAI-compatible URL', description: 'Base URL for OpenAI-compatible API', type: 'text', | ||
| showWhen: { key: 'llm_backend', value: 'openai_compatible' } }, | ||
| { key: 'bedrock_region', label: 'Bedrock Region', description: 'AWS region for Bedrock', type: 'text', | ||
| showWhen: { key: 'llm_backend', value: 'bedrock' } }, | ||
| { key: 'bedrock_cross_region', label: 'Cross-Region', description: 'Enable cross-region inference', type: 'text', | ||
| showWhen: { key: 'llm_backend', value: 'bedrock' } }, | ||
| { key: 'bedrock_profile', label: 'AWS Profile', description: 'AWS profile for Bedrock auth', type: 'text', | ||
| showWhen: { key: 'llm_backend', value: 'bedrock' } }, | ||
| ] | ||
| }, | ||
| { | ||
| group: 'Embeddings', | ||
| settings: [ | ||
| { key: 'embeddings.enabled', label: 'Enabled', description: 'Enable vector embeddings for memory search', type: 'boolean' }, | ||
| { key: 'embeddings.provider', label: 'Provider', description: 'Embeddings API provider', | ||
| type: 'select', options: ['openai', 'nearai'] }, | ||
| { key: 'embeddings.model', label: 'Model', description: 'Embedding model name', type: 'text' }, |
There was a problem hiding this comment.
Agreed this should be localized. Adding i18n keys for all settings labels/descriptions is a bigger change (100+ strings across two locale files) — will address in a follow-up PR to keep this one focused.
There was a problem hiding this comment.
Done — all settings labels, descriptions, group titles, and channel card strings now use I18n.t() keys. Added 120+ cfg.* and channels.* keys to both en.js and zh-CN.js. Done in 9885f7e.
…sion-check]
- Use I18n.t() for MCP empty state, export/import toasts, confirm modal
- Fix CLI channel card using wrong channel key ('repl' -> 'cli')
- Fix settings search counting hidden rows as visible
- Add aria-label i18n for settings search input
- Add common.loadFailed i18n key (en + zh-CN)
- Update E2E tests: WASM channel tests use Channels subtab,
remove tests use custom confirm modal instead of window.confirm
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Review: Unified settings page with subtabs
Ambitious feature PR -- settings page with subtabs, import/export, search, mobile responsive, i18n. The backend changes (ActiveConfigSnapshot, /api/gateway/status) are reasonable. The frontend scope is very large (836 lines added to app.js alone).
Blocking
1. E2E failures (4 tests)
Extension card rendering tests fail because the UI restructuring changed button elements:
test_wasm_channel_setup_states-- "Setup" button selector no longer matchestest_wasm_channel_pairing_state-- "Reconfigure" button missingtest_wasm_channel_active_state-- "Reconfigure" button missingtest_wasm_channel_failed_renders-- "Reconfigure" button missing
The ext_configure_btn selector can't find elements with the expected text. The PR's HTML changes likely restructured the extension card buttons. Please update the E2E selectors or restore the expected button structure.
2. Features E2E also failing -- needs investigation.
Non-blocking observations
- 1704 additions is very large for a single PR. Consider whether the settings import/export, search, mobile responsive layout, and accessibility improvements could be separate follow-up PRs.
- The
main.rschanges add config resolution logic -- this should follow the module-owned initialization rule (config resolution belongs insrc/config/, notmain.rs). - Good i18n coverage (en + zh-CN).
Title check: accurate -- this is a unified settings page with subtabs.
…kip-regression-check] - WASM channel tests: filter by display name to avoid matching built-in channel cards in the Channels subtab - Skills remove test: click confirm modal button instead of using window.confirm (skill removal now uses custom confirm modal) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Unifies the web gateway’s configuration UI by replacing separate Extensions/Skills top-level tabs with a single Settings tab that contains multiple subtabs (Inference, Agent, Channels, Networking, Extensions, MCP, Skills). This is supported by new frontend settings tooling (search, import/export, confirm modal), updated SSE refresh behavior, and a status endpoint enhancement to expose resolved runtime configuration.
Changes:
- Introduces a new Settings tab layout (sidebar subtabs + toolbar), moving Extensions/MCP/Skills content under Settings.
- Adds structured settings editors backed by
/api/settings/*endpoints, plus import/export and search. - Extends
/api/gateway/statusto include active LLM/channel config snapshot and updates tests/builders accordingly.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ws_gateway_integration.rs | Initializes new active_config field in test server state. |
| tests/support/gateway_workflow_harness.rs | Initializes new active_config field in harness-built gateway state. |
| tests/openai_compat_integration.rs | Initializes new active_config field in OpenAI compat integration tests. |
| tests/e2e/scenarios/test_wasm_lifecycle.py | Updates navigation to Settings > Extensions subtab. |
| tests/e2e/scenarios/test_skills.py | Updates navigation to Settings > Skills subtab and confirms modal-based removal flow. |
| tests/e2e/scenarios/test_extensions.py | Refactors E2E tests for Settings subtabs; removes tools-table coverage; adds modal confirmation expectations. |
| tests/e2e/helpers.py | Adds Settings subtab selectors and confirm modal selectors; updates tab list. |
| src/main.rs | Ensures WASM channels directory exists; injects active config snapshot into gateway state. |
| src/channels/web/ws.rs | Updates WS test helper state initialization to include active_config. |
| src/channels/web/test_helpers.rs | Updates test gateway builder state initialization to include active_config. |
| src/channels/web/static/style.css | Adds Settings layout styling, card state styling, toolbar/modal styles; removes tools table styles. |
| src/channels/web/static/index.html | Replaces Extensions/Skills tabs with Settings tab + subtabs and adds confirmation modal markup. |
| src/channels/web/static/i18n/zh-CN.js | Adds Settings-related i18n keys and removes unused tools keys. |
| src/channels/web/static/i18n/en.js | Adds Settings-related i18n keys and removes unused tools keys. |
| src/channels/web/static/app.js | Implements Settings subtabs, settings editor/search/import/export, modal confirmation, 204 handling, and updated SSE refresh logic. |
| src/channels/web/server.rs | Adds ActiveConfigSnapshot to GatewayState and exposes it in /api/gateway/status. |
| src/channels/web/mod.rs | Adds with_active_config() to inject active config snapshot into gateway state. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/channels/web/static/app.js
Outdated
| // Extension setup flows can surface approvals while user is on Settings > Extensions. | ||
| if (currentTab === 'settings' && currentSettingsSubtab === 'extensions') loadExtensions(); |
There was a problem hiding this comment.
Fixed — now calls refreshCurrentSettingsTab() for any settings subtab, not just extensions. Done in d63883a.
There was a problem hiding this comment.
Fixed in d63883a — now calls refreshCurrentSettingsTab() for any settings subtab, not just Extensions.
src/channels/web/static/app.js
Outdated
| var html = '<div class="extensions-list">'; | ||
| for (var i = 0; i < (count || 3); i++) { | ||
| html += '<div class="skeleton-card"><div class="skeleton-bar" style="width:60%;height:14px"></div><div class="skeleton-bar" style="width:90%;height:10px"></div><div class="skeleton-bar" style="width:40%;height:10px"></div></div>'; | ||
| } | ||
| html += '</div>'; |
There was a problem hiding this comment.
Fixed — removed the <div class="extensions-list"> wrapper from renderCardsSkeleton(). It now returns only the skeleton card markup, avoiding nested grid issues. Done in d63883a.
There was a problem hiding this comment.
Fixed in d63883a — removed the <div class="extensions-list"> wrapper. renderCardsSkeleton() now returns only skeleton card markup.
| var query = this.value.toLowerCase(); | ||
| var activePanel = document.querySelector('.settings-subpanel.active'); | ||
| if (!activePanel) return; | ||
| var rows = activePanel.querySelectorAll('.settings-row'); |
…ion-check] - approval_needed SSE: refresh any active settings subtab, not just Extensions — approvals can surface from Channels/MCP setup flows too - renderCardsSkeleton: remove nested .extensions-list wrapper that caused skeleton cards to render constrained inside grid cells Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressing the blocking review from @zmanian: E2E failures — all resolved:
CI is fully green as of e110159. Latest commit d63883a addresses remaining review items (approval_needed subtab refresh + renderCardsSkeleton wrapper). |
…ion-check] Use expect_response to deterministically wait for the /api/extensions reload triggered by handleAuthCompleted → refreshCurrentSettingsTab, instead of a fixed 600ms sleep that was too short under CI load. Also remove stale /api/extensions/tools route handler. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR consolidates the web gateway’s Extensions and Skills top-level tabs into a single Settings tab with sidebar subtabs, and expands the gateway status/config surface to support the new UI (settings import/export, channels status, and config snapshotting).
Changes:
- Replaces Extensions/Skills tabs with a unified Settings page (subtabs + toolbar + confirm modal) and updates styling/i18n accordingly.
- Updates SSE/UI refresh behaviors and adds
apiFetchsupport for HTTP 204 responses (used by settings PUT/DELETE/import). - Extends
/api/gateway/statusto include an active config snapshot (LLM backend/model + enabled channels) and updates Rust + E2E/integration tests for the new state fields and navigation.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ws_gateway_integration.rs | Adds active_config to gateway state initialization for tests. |
| tests/support/gateway_workflow_harness.rs | Adds active_config to harness-created gateway state. |
| tests/openai_compat_integration.rs | Adds active_config to test server state initializations. |
| tests/e2e/scenarios/test_wasm_lifecycle.py | Updates navigation to Settings > Extensions subtab. |
| tests/e2e/scenarios/test_skills.py | Updates navigation to Settings > Skills and confirm-modal removal flow. |
| tests/e2e/scenarios/test_extensions.py | Refactors E2E flows for Settings subtabs (Extensions/Channels/MCP) and removes tools-table checks. |
| tests/e2e/helpers.py | Adds selectors for Settings subtabs/panels, confirm modal, channels cards; updates tab list. |
| src/main.rs | Ensures WASM channels directory exists; injects active config snapshot into gateway channel. |
| src/channels/web/ws.rs | Updates test state to include active_config. |
| src/channels/web/test_helpers.rs | Updates test gateway builder state to include active_config. |
| src/channels/web/static/style.css | Adds Settings layout/toolbar/modal styling and refines extension card styling states. |
| src/channels/web/static/index.html | Introduces Settings tab/subpanels and confirm modal; removes Extensions/Skills top-level panels. |
| src/channels/web/static/i18n/zh-CN.js | Adds Settings-related keys; removes unused tools keys; updates Extensions wording. |
| src/channels/web/static/i18n/en.js | Adds Settings-related keys; removes unused tools keys; updates Extensions wording. |
| src/channels/web/static/app.js | Implements Settings subtab loading (settings editor, channels status, MCP separation), confirm modal, 204 handling, and refresh logic. |
| src/channels/web/server.rs | Adds ActiveConfigSnapshot to GatewayState and returns it in /api/gateway/status. |
| src/channels/web/mod.rs | Plumbs active_config through GatewayChannel with with_active_config(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cb.checked = !!value; | ||
| cb.setAttribute('aria-label', ariaLabel); | ||
| cb.addEventListener('change', function() { saveSetting(def.key, cb.checked); }); | ||
| inputWrap.appendChild(cb); |
| document.querySelectorAll('.settings-subpanel').forEach(function(p) { | ||
| p.classList.toggle('active', p.id === 'settings-' + subtab); | ||
| }); | ||
| loadSettingsSubtab(subtab); |
| // Built-in Channels section | ||
| var builtinSection = document.createElement('div'); | ||
| builtinSection.className = 'extensions-section'; | ||
| var builtinTitle = document.createElement('h3'); | ||
| builtinTitle.textContent = 'Built-in Channels'; | ||
| builtinSection.appendChild(builtinTitle); |
| var kindEl = document.createElement('span'); | ||
| kindEl.className = 'ext-kind kind-builtin'; | ||
| kindEl.textContent = 'Built-in'; | ||
| header.appendChild(kindEl); | ||
|
|
||
| var statusDot = document.createElement('span'); | ||
| statusDot.className = 'ext-auth-dot ' + (active ? 'authed' : 'unauthed'); | ||
| statusDot.title = active ? 'Active' : 'Inactive'; | ||
| header.appendChild(statusDot); | ||
|
|
||
| card.appendChild(header); | ||
|
|
||
| var desc = document.createElement('div'); | ||
| desc.className = 'ext-desc'; | ||
| desc.textContent = description; | ||
| card.appendChild(desc); | ||
|
|
||
| if (detail) { | ||
| var detailEl = document.createElement('div'); | ||
| detailEl.className = 'ext-url'; | ||
| detailEl.textContent = detail; | ||
| card.appendChild(detailEl); | ||
| } | ||
|
|
||
| var actions = document.createElement('div'); | ||
| actions.className = 'ext-actions'; | ||
| var label = document.createElement('span'); | ||
| label.className = 'ext-active-label'; | ||
| label.textContent = active ? 'Active' : 'Inactive'; | ||
| actions.appendChild(label); |
…p-regression-check] Inject a counter wrapper around refreshCurrentSettingsTab to verify it's actually called, and wait for the async fetch to complete before asserting the reload count. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…l cards [skip-regression-check] Move 120+ hardcoded strings in settings definitions (INFERENCE_SETTINGS, AGENT_SETTINGS, NETWORKING_SETTINGS) and channel card labels to i18n keys. Render functions now resolve labels via I18n.t() so the settings page translates when switching locales. Covers: group titles, setting labels/descriptions, built-in channel names/descriptions, and the "No settings found" empty state. Both en.js and zh-CN.js updated with all new cfg.* and channels.* keys. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR consolidates the web gateway’s Extensions and Skills top-level tabs into a single Settings tab with sidebar subtabs (Inference/Agent/Channels/Networking/Extensions/MCP/Skills), updating the frontend UI, i18n strings, and E2E/integration tests accordingly. It also extends /api/gateway/status to expose an “active config” snapshot used by the new Channels/Inference UIs.
Changes:
- Replaces Extensions/Skills top-level tabs with a unified Settings tab + subpanels, plus a reusable confirmation modal.
- Adds
ActiveConfigSnapshotto the web gateway state and includesllm_backend,llm_model,enabled_channelsin/api/gateway/status. - Updates Rust integration tests and Python E2E tests/selectors to match the new navigation and modal flows.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ws_gateway_integration.rs | Initializes GatewayState.active_config in test server state. |
| tests/support/gateway_workflow_harness.rs | Initializes GatewayState.active_config in workflow harness state. |
| tests/openai_compat_integration.rs | Initializes GatewayState.active_config in OpenAI compat test servers. |
| tests/e2e/scenarios/test_wasm_lifecycle.py | Updates navigation from Extensions tab to Settings > Extensions subtab. |
| tests/e2e/scenarios/test_skills.py | Updates navigation to Settings > Skills and confirms removal via modal. |
| tests/e2e/scenarios/test_extensions.py | Refactors navigation helpers for Settings subtabs; removes tools-table tests; adapts flows to custom confirm modal and new Channels/MCP subtabs. |
| tests/e2e/helpers.py | Adds selectors for settings subtabs/subpanels, confirm modal, and channels cards; updates tab list. |
| src/main.rs | Ensures WASM channels dir exists; injects ActiveConfigSnapshot into gateway for status endpoint. |
| src/channels/web/ws.rs | Updates test gateway state with active_config. |
| src/channels/web/test_helpers.rs | Updates TestGatewayBuilder to include active_config. |
| src/channels/web/static/style.css | Adds styling for Settings layout, toolbars, skeletons, and confirmation modal; tweaks extension card states. |
| src/channels/web/static/index.html | Replaces Extensions/Skills panels with Settings layout + subpanels; adds confirmation modal markup. |
| src/channels/web/static/i18n/en.js | Adds Settings/Channels/settings-editor strings; removes tools.* strings. |
| src/channels/web/static/i18n/zh-CN.js | Same as en.js for zh-CN locale. |
| src/channels/web/static/app.js | Implements Settings subtab routing, settings editor, channels/mcp loaders, confirm modal, import/export; updates SSE refresh behavior; handles HTTP 204 in apiFetch(). |
| src/channels/web/server.rs | Introduces ActiveConfigSnapshot and returns new fields from /api/gateway/status. |
| src/channels/web/mod.rs | Adds GatewayChannel::with_active_config to populate the status snapshot. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/channels/web/static/app.js
Outdated
| container.innerHTML = ''; | ||
| renderStructuredSettingsInto(container, NETWORKING_SETTINGS, settings, {}); | ||
| }).catch(function(err) { | ||
| container.innerHTML = '<div class="empty-state">Failed to load settings: ' |
There was a problem hiding this comment.
Fixed — all 3 occurrences of hardcoded "Failed to load settings" now use I18n.t('common.loadFailed'). Done in a657ddd.
src/channels/web/static/app.js
Outdated
| URL.revokeObjectURL(url); | ||
| showToast(I18n.t('settings.exportSuccess'), 'success'); | ||
| }).catch(function(err) { | ||
| showToast(I18n.t('settings.importFailed', { message: err.message }), 'error'); |
There was a problem hiding this comment.
Good catch — was using settings.importFailed for export errors. Added settings.exportFailed key and switched the export catch to use it. Done in a657ddd.
| builtinList.appendChild(renderBuiltinChannelCard( | ||
| I18n.t('channels.cli'), | ||
| I18n.t('channels.cliDesc'), | ||
| enabledChannels.indexOf('cli') !== -1, | ||
| I18n.t('channels.runWith', { cmd: 'ironclaw run --cli' }) | ||
| )); | ||
|
|
||
| builtinList.appendChild(renderBuiltinChannelCard( | ||
| I18n.t('channels.repl'), | ||
| I18n.t('channels.replDesc'), | ||
| enabledChannels.indexOf('repl') !== -1, | ||
| I18n.t('channels.runWith', { cmd: 'ironclaw run --repl' }) |
There was a problem hiding this comment.
The CLI TUI (CliChannel, Ratatui-based) is a separate channel from REPL (ReplChannel, simple stdin). They have distinct channel names — 'cli' is correct here. No change needed.
| var kindEl = document.createElement('span'); | ||
| kindEl.className = 'ext-kind kind-builtin'; | ||
| kindEl.textContent = 'Built-in'; | ||
| header.appendChild(kindEl); | ||
|
|
||
| var statusDot = document.createElement('span'); | ||
| statusDot.className = 'ext-auth-dot ' + (active ? 'authed' : 'unauthed'); | ||
| statusDot.title = active ? 'Active' : 'Inactive'; | ||
| header.appendChild(statusDot); | ||
|
|
||
| card.appendChild(header); | ||
|
|
||
| var desc = document.createElement('div'); | ||
| desc.className = 'ext-desc'; | ||
| desc.textContent = description; | ||
| card.appendChild(desc); | ||
|
|
||
| if (detail) { | ||
| var detailEl = document.createElement('div'); | ||
| detailEl.className = 'ext-url'; | ||
| detailEl.textContent = detail; | ||
| card.appendChild(detailEl); | ||
| } | ||
|
|
||
| var actions = document.createElement('div'); | ||
| actions.className = 'ext-actions'; | ||
| var label = document.createElement('span'); | ||
| label.className = 'ext-active-label'; | ||
| label.textContent = active ? 'Active' : 'Inactive'; | ||
| actions.appendChild(label); |
There was a problem hiding this comment.
Fixed — "Built-in" now uses I18n.t('ext.builtin'), and Active/Inactive labels use I18n.t('ext.active') / I18n.t('ext.inactive'). Done in a657ddd.
| var ariaLabel = I18n.t(def.label) + (def.description ? '. ' + I18n.t(def.description) : ''); | ||
| var placeholderText = activeValue ? 'env: ' + activeValue : (def.placeholder || 'env default'); | ||
|
|
||
| if (def.type === 'boolean') { | ||
| var cb = document.createElement('input'); | ||
| cb.type = 'checkbox'; | ||
| cb.checked = !!value; | ||
| cb.setAttribute('aria-label', ariaLabel); | ||
| cb.addEventListener('change', function() { saveSetting(def.key, cb.checked); }); | ||
| inputWrap.appendChild(cb); | ||
| } else if (def.type === 'select' && def.options) { | ||
| var sel = document.createElement('select'); | ||
| sel.className = 'settings-select'; | ||
| sel.setAttribute('data-setting-key', def.key); | ||
| sel.setAttribute('aria-label', ariaLabel); | ||
| var emptyOpt = document.createElement('option'); | ||
| emptyOpt.value = ''; | ||
| emptyOpt.textContent = activeValue ? '\u2014 env: ' + activeValue + ' \u2014' : '\u2014 use env default \u2014'; | ||
| if (!value && value !== false && value !== 0) emptyOpt.selected = true; | ||
| sel.appendChild(emptyOpt); |
There was a problem hiding this comment.
Fixed — placeholder strings now use I18n.t('settings.envValue', { value }), I18n.t('settings.envDefault'), and I18n.t('settings.useEnvDefault'). Done in a657ddd.
src/channels/web/static/app.js
Outdated
|
|
||
| var saved = document.createElement('span'); | ||
| saved.className = 'settings-saved-indicator'; | ||
| saved.textContent = '\u2713 Saved'; |
There was a problem hiding this comment.
Fixed — now uses I18n.t('settings.saved'). Done in a657ddd.
…n-check]
- Fix export error toast using wrong i18n key (importFailed → exportFailed)
- Replace "Failed to load settings:" with I18n.t('common.loadFailed')
- Localize renderBuiltinChannelCard: "Built-in", "Active", "Inactive"
- Localize settings placeholders: "env: ", "env default", "use env default"
- Localize "✓ Saved" indicator
- Add new i18n keys to both en.js and zh-CN.js
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR consolidates the web gateway UI’s Extensions and Skills into a single Settings tab with sidebar subtabs, adds settings import/export/search, and updates the gateway status API to surface “active config” details needed by the new UI.
Changes:
- Replaces top-level Extensions/Skills tabs with a unified Settings tab and subtabs (Inference/Agent/Channels/Networking/Extensions/MCP/Skills) plus a reusable confirmation modal.
- Updates the web gateway frontend to fetch/edit settings via
/api/settings/exportand refresh correct subtabs on SSE events; adds 204-handling toapiFetch. - Extends
/api/gateway/statusto includellm_backend,llm_model, andenabled_channels, and updates Rust + E2E/integration tests accordingly.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ws_gateway_integration.rs | Updates gateway state initialization for new active_config field. |
| tests/support/gateway_workflow_harness.rs | Adds default active_config to test gateway state. |
| tests/openai_compat_integration.rs | Adds default active_config in integration test servers. |
| tests/e2e/scenarios/test_wasm_lifecycle.py | Updates navigation for Settings > Extensions subtab. |
| tests/e2e/scenarios/test_skills.py | Updates navigation to Settings > Skills and confirm-modal removal flow. |
| tests/e2e/scenarios/test_extensions.py | Refactors tests for new Settings subtabs (Extensions/Channels/MCP) and confirm modal. |
| tests/e2e/helpers.py | Adds selectors for settings subtabs/subpanels and confirm modal; updates tab list. |
| src/main.rs | Ensures WASM channels dir exists; injects ActiveConfigSnapshot into gateway channel. |
| src/channels/web/ws.rs | Updates web WS test state with default active_config. |
| src/channels/web/test_helpers.rs | Updates test gateway builder to include default active_config. |
| src/channels/web/static/style.css | Adds styling for settings layout, toolbar, skeletons, modal, and updated card states. |
| src/channels/web/static/index.html | Replaces Extensions/Skills panels with unified Settings layout + confirm modal markup. |
| src/channels/web/static/i18n/zh-CN.js | Adds new Settings/Channels i18n keys and removes tools-table keys. |
| src/channels/web/static/i18n/en.js | Adds new Settings/Channels i18n keys and removes tools-table keys. |
| src/channels/web/static/app.js | Implements settings subtabs, structured settings editor, search/export/import, confirm modal, and updated SSE refresh logic. |
| src/channels/web/server.rs | Introduces ActiveConfigSnapshot and includes it in /api/gateway/status response. |
| src/channels/web/mod.rs | Adds gateway builder support for with_active_config() and stores snapshot in state. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function showConfirmModal(title, message, onConfirm, confirmLabel, confirmClass) { | ||
| var modal = document.getElementById('confirm-modal'); | ||
| document.getElementById('confirm-modal-title').textContent = title; | ||
| document.getElementById('confirm-modal-message').textContent = message || ''; | ||
| document.getElementById('confirm-modal-message').style.display = message ? '' : 'none'; | ||
| var btn = document.getElementById('confirm-modal-btn'); | ||
| btn.textContent = confirmLabel || I18n.t('btn.confirm'); | ||
| btn.className = confirmClass || 'btn-danger'; | ||
| _confirmModalCallback = onConfirm; | ||
| modal.style.display = 'flex'; | ||
| } | ||
|
|
||
| function closeConfirmModal() { | ||
| document.getElementById('confirm-modal').style.display = 'none'; | ||
| _confirmModalCallback = null; | ||
| } | ||
|
|
||
| document.getElementById('confirm-modal-btn').addEventListener('click', function() { | ||
| if (_confirmModalCallback) _confirmModalCallback(); | ||
| closeConfirmModal(); | ||
| }); | ||
| document.getElementById('confirm-modal-cancel-btn').addEventListener('click', closeConfirmModal); | ||
|
|
There was a problem hiding this comment.
Fixed in c59ae34 — added Escape key handler, click-outside-to-dismiss on overlay, and focus on the confirm button when modal opens.
| builtinList.appendChild(renderBuiltinChannelCard( | ||
| I18n.t('channels.cli'), | ||
| I18n.t('channels.cliDesc'), | ||
| enabledChannels.indexOf('cli') !== -1, | ||
| I18n.t('channels.runWith', { cmd: 'ironclaw run --cli' }) | ||
| )); | ||
|
|
||
| builtinList.appendChild(renderBuiltinChannelCard( | ||
| I18n.t('channels.repl'), | ||
| I18n.t('channels.replDesc'), | ||
| enabledChannels.indexOf('repl') !== -1, | ||
| I18n.t('channels.runWith', { cmd: 'ironclaw run --repl' }) | ||
| )); |
There was a problem hiding this comment.
Same as prior comment — CLI TUI (CliChannel) and REPL (ReplChannel) are distinct backend channels with separate names. 'cli' is the correct identifier here.
| document.getElementById('settings-search-input').addEventListener('input', function() { | ||
| var query = this.value.toLowerCase(); | ||
| var activePanel = document.querySelector('.settings-subpanel.active'); | ||
| if (!activePanel) return; | ||
| var rows = activePanel.querySelectorAll('.settings-row'); | ||
| var visibleCount = 0; | ||
| rows.forEach(function(row) { | ||
| var text = row.textContent.toLowerCase(); | ||
| if (query === '' || text.indexOf(query) !== -1) { | ||
| row.classList.remove('search-hidden'); | ||
| if (!row.classList.contains('hidden')) visibleCount++; | ||
| } else { | ||
| row.classList.add('search-hidden'); | ||
| } | ||
| }); | ||
| // Show/hide group titles based on visible children | ||
| var groups = activePanel.querySelectorAll('.settings-group'); | ||
| groups.forEach(function(group) { | ||
| var visibleRows = group.querySelectorAll('.settings-row:not(.search-hidden):not(.hidden)'); | ||
| if (visibleRows.length === 0 && query !== '') { | ||
| group.style.display = 'none'; | ||
| } else { | ||
| group.style.display = ''; | ||
| } | ||
| }); | ||
| // Show/hide empty state | ||
| var existingEmpty = activePanel.querySelector('.settings-search-empty'); | ||
| if (existingEmpty) existingEmpty.remove(); | ||
| if (query !== '' && visibleCount === 0) { | ||
| var empty = document.createElement('div'); | ||
| empty.className = 'settings-search-empty'; | ||
| empty.textContent = I18n.t('settings.noMatchingSettings', { query: this.value }); | ||
| activePanel.appendChild(empty); | ||
| } |
There was a problem hiding this comment.
Fixed in c59ae34 — search handler now returns early if the active panel has no .settings-row elements, avoiding the spurious empty-state on Extensions/MCP/Skills/Channels subtabs.
| <!-- Confirmation Modal --> | ||
| <div id="confirm-modal" class="modal-overlay" style="display:none"> | ||
| <div class="modal"> | ||
| <h3 id="confirm-modal-title"></h3> | ||
| <p id="confirm-modal-message"></p> | ||
| <div class="modal-actions"> | ||
| <button id="confirm-modal-cancel-btn" class="btn-secondary" data-i18n="btn.cancel">Cancel</button> | ||
| <button id="confirm-modal-btn" class="btn-danger">Confirm</button> | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Fixed in c59ae34 — added role="dialog", aria-modal="true", aria-labelledby="confirm-modal-title" to the modal overlay, and focus is set on the confirm button when the modal opens.
…ip-regression-check] - Add role="dialog", aria-modal="true", aria-labelledby to confirm modal - Focus confirm button when modal opens - Close modal on Escape key or overlay click - Skip settings search on non-settings panels (Extensions/MCP/Skills/Channels) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Large frontend PR (3400+ lines) that replaces separate Extensions/Skills tabs with a unified Settings page. CI is green with all checks passing.
Given this is primarily frontend HTML/CSS/JS changes in the web gateway, a few observations:
1. PR scope is very large
This touches settings API integration, channels UI, extension cards, MCP servers, confirmation modals, SSE handlers, i18n, and removes the tools debug section -- all in one PR. Consider whether any of these could be split out (e.g., the confirmation modal, the 204 handling fix, the i18n changes) to make review more tractable.
2. Title accuracy
Title says "unified settings page with subtabs" which accurately describes the change. Good.
3. Test coverage
The test plan is manual-only (checkboxes). For a UI refactor of this size, E2E tests covering the basic navigation flow (open settings, click through subtabs, verify content loads) would provide meaningful regression protection.
4. Removed "Registered Tools" debug section
The PR description mentions removing the tools table. Confirm this doesn't break any E2E tests that depend on /api/extensions/tools endpoint.
This needs a thorough manual review of the frontend behavior. I'll defer final judgment to a reviewer who can test the UI interactively. The Rust-side changes appear minimal (likely just the 204 handling in apiFetch).
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Re-review: unified settings page with subtabs
Both blocking items from my previous review have been resolved:
-
E2E failures (4 tests) -- Fixed in commits
f8ffd9eande110159. Selectors updated fromtab_button(extensions)totab_button(settings)+settings_subtab(extensions). Skills remove confirm also fixed. -
Features E2E -- All E2E tests now passing (core, extensions, features all green).
Additional improvements since last review:
- Full i18n for settings labels, descriptions, and channel cards (
9885f7e,a657ddd) - Confirm modal a11y:
role="dialog",aria-modal="true", Esc/click-outside dismiss (c59ae34) - Search guard for empty row sets
- Cancel button added to confirm modal with CSP-compliant event listener
CI fully green including E2E tests and all Clippy variants.
Non-blocking notes (unchanged from last review):
- Config resolution logic in
main.rsshould eventually move tosrc/config/per module-owned initialization rule [skip-regression-check]used on most commits -- acceptable for frontend-only changes
LGTM.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Withdrawing approval -- code looks good but the settings page redesign needs manual user testing before merge (subtab navigation, import/export, search, mobile layout). Leaving as comment review until testing is complete.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Pending manual user testing (see previous comment).
Summary
Replaces the separate Extensions and Skills top-level tabs with a single Settings tab containing a sidebar with subtabs:
/api/settings, grouped by prefix)/api/gateway/status+ extensions APIs, with configure/reconnect actionsKey changes
/api/settings/export, renders grouped key-value editors with save/reset per setting, tiered into Inference/Agent/Channels/Networking by prefix/api/gateway/status) and messaging channels (WASM channel extensions) with full stepper/pairing UIloadMcpServers()functionauth_completedandextension_statusnow refresh the correct subtab viarefreshCurrentSettingsTab()apiFetchnow handles HTTP 204 (No Content) without trying to parse JSONtools.*keys; hardcoded strings migrated toI18n.t()Test plan
/api/extensions/toolsfetch🤖 Generated with Claude Code