feat(ui): add host-agnostic config editor and shared UI runtime plumbing#361
Merged
Merged
Conversation
Introduce a dedicated config editor resource with structured get_config payloads and shared host-context/compact-row infrastructure so MCP UIs behave consistently across hosts while tightening config key validation and preview type coverage.
Contributor
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Contributor
📝 WalkthroughWalkthroughAdds a new config-editor UI and runtime, centralizes config field definitions and validation, enhances get_config/set_config_value (including shell discovery), refactors file-preview to use a compact-row/host-context system, and adds a tool-bridge and shared UI helpers and styles. Changes
Sequence DiagramsequenceDiagram
participant User
participant ConfigEditor as Config Editor (UI)
participant ToolBridge as Tool Bridge
participant Host as Host/Server
participant ConfigTools as Config Tools
participant Disk as File System
User->>ConfigEditor: Open editor / request data
ConfigEditor->>ToolBridge: callTool("get_config")
ToolBridge->>Host: proxy call (host helper or postMessage fallback)
Host->>ConfigTools: getConfig()
ConfigTools->>ConfigTools: detectAvailableShells()
ConfigTools->>Disk: read system shell files / env
Disk-->>ConfigTools: shell list + system info
ConfigTools-->>Host: return structured config + uiHints
Host-->>ToolBridge: payload
ToolBridge-->>ConfigEditor: payload
ConfigEditor->>User: render editable entries
User->>ConfigEditor: edit & apply
ConfigEditor->>ToolBridge: callTool("set_config_value", {key,value})
ToolBridge->>Host: proxy call
Host->>ConfigTools: setConfigValue(key,value)
ConfigTools->>ConfigTools: validate/parse (arrays, types)
ConfigTools->>Disk: write config
Disk-->>ConfigTools: success
ConfigTools-->>Host: updated config
Host-->>ToolBridge: result
ToolBridge-->>ConfigEditor: success -> refresh UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
codeant-ai
Bot
added
the
size:XXL
Contributor
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+87
to
+145
| async function callViaFallback(name: string, args: ToolArgs): Promise<unknown> { | ||
| if (!host.parent || !host.addEventListener || !host.removeEventListener) { | ||
| throw new Error('JSON-RPC fallback is unavailable in this host environment.'); | ||
| } | ||
|
|
||
| const parent = host.parent; | ||
| const addListener = host.addEventListener; | ||
| const removeListener = host.removeEventListener; | ||
|
|
||
| requestCounter += 1; | ||
| const requestId = `${idPrefix}:${requestCounter}`; | ||
|
|
||
| return new Promise((resolve, reject) => { | ||
| const timeoutHandle = setTimeout(() => { | ||
| removeListener('message', onMessage); | ||
| reject(new Error(`Tool call fallback timed out after ${timeoutMs}ms (request: ${requestId})`)); | ||
| }, timeoutMs); | ||
|
|
||
| const onMessage: MessageListener = (event) => { | ||
| const payload = event.data; | ||
| if (event.source !== parent) { | ||
| return; | ||
| } | ||
| if (targetOrigin !== '*' && event.origin !== targetOrigin) { | ||
| return; | ||
| } | ||
| if (!isObject(payload) || payload.id !== requestId) { | ||
| return; | ||
| } | ||
|
|
||
| clearTimeout(timeoutHandle); | ||
| removeListener('message', onMessage); | ||
|
|
||
| if (isObject(payload.error)) { | ||
| const rawMessage = payload.error.message; | ||
| const message = typeof rawMessage === 'string' | ||
| ? rawMessage | ||
| : 'Unknown tools/call fallback error'; | ||
| reject(new Error(`Tool call fallback failed: ${message}`)); | ||
| return; | ||
| } | ||
|
|
||
| resolve(payload.result); | ||
| }; | ||
|
|
||
| addListener('message', onMessage); | ||
| parent.postMessage( | ||
| { | ||
| jsonrpc: '2.0', | ||
| id: requestId, | ||
| method: 'tools/call', | ||
| params: { | ||
| name, | ||
| arguments: args, | ||
| }, | ||
| }, | ||
| targetOrigin | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
Suggestion: In the JSON-RPC fallback path you detach addEventListener/removeEventListener from the host into local variables and then call them as bare functions, which in browsers will likely throw an "Illegal invocation" TypeError because the native methods expect to be invoked with the host object as this, breaking the entire fallback tool call mechanism; instead, call these methods directly on host so they retain the correct this binding. [logic error]
Severity Level: Critical 🚨
- ❌ Config editor cannot fetch config via tool bridge.
- ❌ Config updates via set_config_value always fail silently.
- ❌ JSON-RPC fallback between iframe and parent is broken.
- ⚠️ Embedded Desktop Commander UI unusable in postMessage hosts.
Suggested change
| async function callViaFallback(name: string, args: ToolArgs): Promise<unknown> { | |
| if (!host.parent || !host.addEventListener || !host.removeEventListener) { | |
| throw new Error('JSON-RPC fallback is unavailable in this host environment.'); | |
| } | |
| const parent = host.parent; | |
| const addListener = host.addEventListener; | |
| const removeListener = host.removeEventListener; | |
| requestCounter += 1; | |
| const requestId = `${idPrefix}:${requestCounter}`; | |
| return new Promise((resolve, reject) => { | |
| const timeoutHandle = setTimeout(() => { | |
| removeListener('message', onMessage); | |
| reject(new Error(`Tool call fallback timed out after ${timeoutMs}ms (request: ${requestId})`)); | |
| }, timeoutMs); | |
| const onMessage: MessageListener = (event) => { | |
| const payload = event.data; | |
| if (event.source !== parent) { | |
| return; | |
| } | |
| if (targetOrigin !== '*' && event.origin !== targetOrigin) { | |
| return; | |
| } | |
| if (!isObject(payload) || payload.id !== requestId) { | |
| return; | |
| } | |
| clearTimeout(timeoutHandle); | |
| removeListener('message', onMessage); | |
| if (isObject(payload.error)) { | |
| const rawMessage = payload.error.message; | |
| const message = typeof rawMessage === 'string' | |
| ? rawMessage | |
| : 'Unknown tools/call fallback error'; | |
| reject(new Error(`Tool call fallback failed: ${message}`)); | |
| return; | |
| } | |
| resolve(payload.result); | |
| }; | |
| addListener('message', onMessage); | |
| parent.postMessage( | |
| { | |
| jsonrpc: '2.0', | |
| id: requestId, | |
| method: 'tools/call', | |
| params: { | |
| name, | |
| arguments: args, | |
| }, | |
| }, | |
| targetOrigin | |
| ); | |
| }); | |
| async function callViaFallback(name: string, args: ToolArgs): Promise<unknown> { | |
| if (!host.parent || !host.addEventListener || !host.removeEventListener) { | |
| throw new Error('JSON-RPC fallback is unavailable in this host environment.'); | |
| } | |
| const parent = host.parent; | |
| requestCounter += 1; | |
| const requestId = `${idPrefix}:${requestCounter}`; | |
| return new Promise((resolve, reject) => { | |
| const timeoutHandle = setTimeout(() => { | |
| host.removeEventListener('message', onMessage); | |
| reject(new Error(`Tool call fallback timed out after ${timeoutMs}ms (request: ${requestId})`)); | |
| }, timeoutMs); | |
| const onMessage: MessageListener = (event) => { | |
| const payload = event.data; | |
| if (event.source !== parent) { | |
| return; | |
| } | |
| if (targetOrigin !== '*' && event.origin !== targetOrigin) { | |
| return; | |
| } | |
| if (!isObject(payload) || payload.id !== requestId) { | |
| return; | |
| } | |
| clearTimeout(timeoutHandle); | |
| host.removeEventListener('message', onMessage); | |
| if (isObject(payload.error)) { | |
| const rawMessage = payload.error.message; | |
| const message = typeof rawMessage === 'string' | |
| ? rawMessage | |
| : 'Unknown tools/call fallback error'; | |
| reject(new Error(`Tool call fallback failed: ${message}`)); | |
| return; | |
| } | |
| resolve(payload.result); | |
| }; | |
| host.addEventListener('message', onMessage); | |
| parent.postMessage( | |
| { | |
| jsonrpc: '2.0', | |
| id: requestId, | |
| method: 'tools/call', | |
| params: { | |
| name, | |
| arguments: args, | |
| }, | |
| }, | |
| targetOrigin | |
| ); | |
| }); | |
| } |
Steps of Reproduction ✅
1. In a browser environment, call `bootstrapConfigEditorApp()` from
`src/ui/config-editor/src/app.ts:720` so the Config Editor UI initializes normally.
2. Inside `bootstrapConfigEditorApp`, a tool bridge is created with `const bridge =
createToolBridge();` at `src/ui/config-editor/src/app.ts:726`, which uses the default host
`globalThis` (the `window` object) as configured in `createToolBridge` at
`src/ui/shared/tool-bridge.ts:80-85`.
3. After the MCP `App` connects via `connectWithSharedHostContext`
(`src/ui/shared/host-context.ts:60-77`), the `onConnected` callback in
`bootstrapConfigEditorApp` (`src/ui/config-editor/src/app.ts:864-875`) invokes
`refreshConfigFromServer`, which calls `bridge.callTool('get_config', {});` at
`src/ui/config-editor/src/app.ts:840`.
4. In `createToolBridge.callTool` (`src/ui/shared/tool-bridge.ts:148-165`), because
`host.openai` and `host.mcp` are undefined on `window`, the function falls through to
`callViaFallback(name, normalizedArgs)` at line 164, entering `callViaFallback` at
`src/ui/shared/tool-bridge.ts:87-145`.
5. Inside `callViaFallback`, the code detaches `host.addEventListener` and
`host.removeEventListener` into `addListener` and `removeListener` at lines 92-94, then
invokes `addListener('message', onMessage);` at line 132. In a module (strict mode)
browser context, `addListener` is an unbound reference to `window.addEventListener`, so it
executes with `this === undefined` and throws a TypeError `"Illegal invocation"`,
preventing the message listener from being registered and causing the
`bridge.callTool('get_config', {})` promise to reject.
6. Because the fallback JSON-RPC `postMessage` bridge never successfully attaches its
`message` handler, all Config Editor tool calls (`get_config`, `set_config_value`) made
via the bridge from `src/ui/config-editor/src/app.ts` fail, leaving the config payload
unset and the editor unable to load or apply configuration.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/ui/shared/tool-bridge.ts
**Line:** 87:145
**Comment:**
*Logic Error: In the JSON-RPC fallback path you detach `addEventListener`/`removeEventListener` from the host into local variables and then call them as bare functions, which in browsers will likely throw an "Illegal invocation" TypeError because the native methods expect to be invoked with the host object as `this`, breaking the entire fallback tool call mechanism; instead, call these methods directly on `host` so they retain the correct `this` binding.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Contributor
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
test/test-file-handlers.js (1)
286-289: Minor naming inconsistency.The function name
testReadFilePreviewMetadatastill references "Metadata" while the test description and console logs now say "preview structured content". Consider renaming the function totestReadFilePreviewStructuredContentfor consistency.Proposed fix
-async function testReadFilePreviewMetadata() { +async function testReadFilePreviewStructuredContent() { console.log('\n--- Test 9: read_file preview structured content ---');And update the call site:
- await testReadFilePreviewMetadata(); + await testReadFilePreviewStructuredContent();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-file-handlers.js` around lines 286 - 289, Rename the test function testReadFilePreviewMetadata to testReadFilePreviewStructuredContent and update any call sites or references that invoke testReadFilePreviewMetadata accordingly (e.g., where the test is registered or called), so the function name matches the console log and test description about "preview structured content".src/ui/shared/host-context.ts (2)
34-36: Type assertion may mask runtime type mismatches.The
variablesobject is validated asRecord<string, unknown>viaisObjectRecord, but is cast toRecord<string, string>when passed toapplyHostStyleVariables. If the host provides non-string values, this could cause unexpected behavior.♻️ Suggested defensive handling
if (variables) { - applyHostStyleVariables(variables as Record<string, string>); + const stringVars: Record<string, string> = {}; + for (const [key, val] of Object.entries(variables)) { + if (typeof val === 'string') { + stringVars[key] = val; + } + } + applyHostStyleVariables(stringVars); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/shared/host-context.ts` around lines 34 - 36, The code casts variables to Record<string,string> which can hide non-string values validated only as Record<string,unknown> by isObjectRecord; update the call site around variables and isObjectRecord to defensively coerce or filter entries before calling applyHostStyleVariables: iterate over the validated variables object (the variables identifier), build a new Record<string,string> that converts non-string values to strings (e.g., String(value)) or skips them and optionally log a warning, then pass that sanitized record to applyHostStyleVariables so the function always receives string values.
16-18: Consider excluding arrays fromisObjectRecord.The current implementation returns
truefor arrays sincetypeof [] === 'object'. This may cause unexpected behavior if array values are passed to functions expecting plain objects.♻️ Suggested refinement
export function isObjectRecord(value: unknown): value is Record<string, unknown> { - return typeof value === 'object' && value !== null; + return typeof value === 'object' && value !== null && !Array.isArray(value); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/shared/host-context.ts` around lines 16 - 18, isObjectRecord currently returns true for arrays because typeof [] === 'object'; update the function isObjectRecord to exclude arrays by adding an explicit Array.isArray check (e.g., return typeof value === 'object' && value !== null && !Array.isArray(value)) so only plain object records satisfy the type guard; keep the signature isObjectRecord(value: unknown): value is Record<string, unknown>.src/tools/config.ts (2)
48-56: Bare shell names added without existence validation on Windows.Lines 53-55 add bare shell names (e.g.,
'powershell.exe','cmd.exe') to the available shells list without verifying they exist. While these are typically available on Windows, this could result in non-functional entries if a user has an unusual system configuration.This is likely acceptable for a hints list, but consider adding a note in the
uiHintsthat these are suggestions rather than validated paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/config.ts` around lines 48 - 56, The loop over candidates adds bare shell names to the list without verifying they exist (in the for...of loop that calls pathExists and add); either perform existence validation for bare names on Windows by resolving them (use pathExists on resolved or PATH-located candidates before calling add) or mark these entries as unvalidated suggestions by updating the uiHints text to state they are hints rather than validated paths; update the logic around candidates/pathExists/add to validate or the uiHints string accordingly and be sure to reference the same functions (candidates, pathExists, add, uiHints) when making the change.
200-223: Array conversion logic is complex with redundant paths.The nested conditionals for array handling have overlapping cases. The check at line 209 (
!originalString.includes('[')) is redundant since JSON.parse would have already succeeded for valid JSON arrays.Consider simplifying:
♻️ Simplified array handling
// Special handling for known array configuration keys if (fieldDefinition.valueType === 'array' && !Array.isArray(valueToStore)) { - if (typeof valueToStore === 'string') { - const originalString = valueToStore; - try { - const parsedValue = JSON.parse(originalString); - valueToStore = parsedValue; - } catch (parseError) { - console.error(`Failed to parse string as array for ${parsed.data.key}: ${parseError}`); - // If parsing failed and it's a single value, convert to an array with one item - if (!originalString.includes('[')) { - valueToStore = [originalString]; - } - } - } else if (valueToStore !== null) { - // If not a string or array (and not null), convert to an array with one item - valueToStore = [String(valueToStore)]; - } - - // Ensure the value is an array after all our conversions - if (!Array.isArray(valueToStore)) { - console.error(`Value for ${parsed.data.key} is still not an array, converting to array`); - valueToStore = [String(valueToStore)]; + if (typeof valueToStore === 'string') { + try { + const parsed = JSON.parse(valueToStore); + valueToStore = Array.isArray(parsed) ? parsed : [valueToStore]; + } catch { + // Not valid JSON - treat as single item + valueToStore = [valueToStore]; + } + } else if (valueToStore !== null) { + valueToStore = [String(valueToStore)]; + } else { + valueToStore = []; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/config.ts` around lines 200 - 223, The array conversion block for fieldDefinition.valueType === 'array' is overly complex and has redundant checks (notably the originalString.includes('[') after JSON.parse). Replace it with a single flow in the handler for valueToStore: if it's a string attempt JSON.parse and if the parsed result is an array use it, otherwise wrap the original string in an array; if it's already an array use it as-is; if it's non-null and not an array convert to [String(valueToStore)]; finally ensure Array.isArray(valueToStore) and only then assign—use parsed.data.key in log messages and include parseError details when JSON.parse fails. This targets the valueToStore conversion logic where fieldDefinition.valueType, valueToStore, parsed.data.key and parseError are referenced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/config-editor/src/app.ts`:
- Around line 769-787: The current app.updateModelContext call includes full
configuration values (payload.entries and change.value) which may expose
secrets; instead, redact sensitive values before sending by replacing
entry.value and change.value with a sanitized representation (e.g., "<redacted>"
or a hashed/summary string) and only include non-sensitive metadata (entry.key,
entry.label, valueType, changedKey) or a boolean/summary flag for whether a
value changed; update the construction of content and structuredContent passed
to app.updateModelContext (references: payload.entries, change,
app.updateModelContext, structuredContent.changedValue) so no raw config values
are serialized into the model context.
- Around line 532-533: refreshSettingSummary currently interpolates raw key into
container.querySelector which can throw if key contains selector
meta-characters; change it to avoid selector interpolation by either using
CSS.escape(key) when available (e.g.
container.querySelector(`[data-setting-summary-key="${CSS.escape(key)}"]`) ) or,
more robustly, select all elements with the attribute and find the matching one
by value (e.g. container.querySelectorAll('[data-setting-summary-key]').forEach
/ Array.from(...).find(el => el.getAttribute('data-setting-summary-key') ===
key)). Update the code in the refreshSettingSummary function (and any other
places using container.querySelector with data-setting-summary-key) to use one
of these safe approaches.
- Around line 802-807: The onConfigChanged callback should call
controller.apply() to refresh state from the server and only after that resolves
write the refreshed payload and sync context, so change the body of
onConfigChanged to await controller.apply(), then call
widgetState.write(controller.state.payload) and syncModelContext('widget-edit',
change), and finally trigger a UI rerender (e.g., call the component's
setState/forceUpdate or equivalent render trigger) so the updated payload is
reflected in the UI; reference: onConfigChanged, controller.apply,
widgetState.write, syncModelContext.
- Around line 163-168: The numeric parsing branch (the valueType === 'number'
block using rawValue and numeric) currently coerces an empty string to 0; update
it to reject empty/whitespace input before calling Number: if rawValue.trim()
=== '' return { ok: false, message: 'Enter a valid number.' }; otherwise
continue with Number(rawValue) and the existing Number.isFinite check so blank
fields do not become 0.
In `@src/ui/config-editor/src/array-modal.ts`:
- Around line 10-15: The modal is being closed in a finally block even when
persistence fails; update the save flow so close() is called only after a
successful onSave resolution: remove or replace the finally { close(); } with
logic that awaits onSave(entryKey, items), and on successful completion calls
close(), while catching and handling errors (e.g., show an error/toast and do
not call close()). Update any identical finally usages around the same save
handler (see the save-related code that references onSave and close, including
the similar block at lines ~182-193) so failures do not dismiss the editor.
In `@src/ui/shared/tool-bridge.ts`:
- Around line 93-94: The extracted method references addListener =
host.addEventListener and removeListener = host.removeEventListener lose their
EventTarget this binding and cause "Illegal invocation"; replace them with bound
wrapper functions that call host.addEventListener(...) and
host.removeEventListener(...), e.g. const addListener = (type, listener,
options?) => host.addEventListener(type, listener, options) and const
removeListener = (type, listener, options?) => host.removeEventListener(type,
listener, options), so all subsequent calls that use addListener/removeListener
use the correct host context (refer to host, addListener, removeListener, and
the call sites around where listeners are registered/unregistered).
In `@src/ui/styles/apps/config-editor.css`:
- Around line 278-286: The CSS uses the keyword with wrong casing: in the rule
targeting ".array-modal-card header button svg" change the property value
"stroke: currentColor" to lowercase "currentcolor" to satisfy Stylelint casing
rules; update only the stroke declaration in that selector (and any other
occurrences of "currentColor") and re-run the linter to ensure no further casing
violations.
- Around line 328-336: The CSS uses the nonstandard casing "currentColor" in the
.array-modal-item-remove svg rule; update the stroke value to the lowercase form
"currentcolor" to match the project's casing convention and other rules (locate
the .array-modal-item-remove svg selector and change the stroke: currentColor;
declaration to stroke: currentcolor;).
In `@src/ui/styles/components/compact-row.css`:
- Around line 26-33: The CSS uses the keyword value "currentColor" with
incorrect casing per Stylelint; update the .compact-chevron rule to use the
canonical lowercase keyword "currentcolor" for the fill property (i.e., change
fill: currentColor; to fill: currentcolor;) so it satisfies the linter while
leaving other properties and transitions unchanged.
---
Nitpick comments:
In `@src/tools/config.ts`:
- Around line 48-56: The loop over candidates adds bare shell names to the list
without verifying they exist (in the for...of loop that calls pathExists and
add); either perform existence validation for bare names on Windows by resolving
them (use pathExists on resolved or PATH-located candidates before calling add)
or mark these entries as unvalidated suggestions by updating the uiHints text to
state they are hints rather than validated paths; update the logic around
candidates/pathExists/add to validate or the uiHints string accordingly and be
sure to reference the same functions (candidates, pathExists, add, uiHints) when
making the change.
- Around line 200-223: The array conversion block for fieldDefinition.valueType
=== 'array' is overly complex and has redundant checks (notably the
originalString.includes('[') after JSON.parse). Replace it with a single flow in
the handler for valueToStore: if it's a string attempt JSON.parse and if the
parsed result is an array use it, otherwise wrap the original string in an
array; if it's already an array use it as-is; if it's non-null and not an array
convert to [String(valueToStore)]; finally ensure Array.isArray(valueToStore)
and only then assign—use parsed.data.key in log messages and include parseError
details when JSON.parse fails. This targets the valueToStore conversion logic
where fieldDefinition.valueType, valueToStore, parsed.data.key and parseError
are referenced.
In `@src/ui/shared/host-context.ts`:
- Around line 34-36: The code casts variables to Record<string,string> which can
hide non-string values validated only as Record<string,unknown> by
isObjectRecord; update the call site around variables and isObjectRecord to
defensively coerce or filter entries before calling applyHostStyleVariables:
iterate over the validated variables object (the variables identifier), build a
new Record<string,string> that converts non-string values to strings (e.g.,
String(value)) or skips them and optionally log a warning, then pass that
sanitized record to applyHostStyleVariables so the function always receives
string values.
- Around line 16-18: isObjectRecord currently returns true for arrays because
typeof [] === 'object'; update the function isObjectRecord to exclude arrays by
adding an explicit Array.isArray check (e.g., return typeof value === 'object'
&& value !== null && !Array.isArray(value)) so only plain object records satisfy
the type guard; keep the signature isObjectRecord(value: unknown): value is
Record<string, unknown>.
In `@test/test-file-handlers.js`:
- Around line 286-289: Rename the test function testReadFilePreviewMetadata to
testReadFilePreviewStructuredContent and update any call sites or references
that invoke testReadFilePreviewMetadata accordingly (e.g., where the test is
registered or called), so the function name matches the console log and test
description about "preview structured content".
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
package.jsonscripts/build-ui-runtime.cjssrc/config-field-definitions.tssrc/server.tssrc/tools/config.tssrc/ui/config-editor/index.htmlsrc/ui/config-editor/src/app.tssrc/ui/config-editor/src/array-modal.tssrc/ui/config-editor/src/main.tssrc/ui/file-preview/shared/preview-file-types.tssrc/ui/file-preview/src/app.tssrc/ui/resources.tssrc/ui/shared/compact-row.tssrc/ui/shared/host-context.tssrc/ui/shared/tool-bridge.tssrc/ui/shared/tool-shell.tssrc/ui/styles/apps/config-editor.csssrc/ui/styles/apps/file-preview.csssrc/ui/styles/components/compact-row.csstest/test-file-handlers.js
💤 Files with no reviewable changes (1)
- src/ui/styles/apps/file-preview.css
Comment on lines
+532
to
+533
| const refreshSettingSummary = (key: string): void => { | ||
| const summaryElement = container.querySelector(`[data-setting-summary-key="${key}"]`) as HTMLElement | null; |
Contributor
There was a problem hiding this comment.
Avoid raw key interpolation in selector construction.
Interpolating key into querySelector(...) can throw when keys contain selector meta characters.
💡 Proposed fix
- const summaryElement = container.querySelector(`[data-setting-summary-key="${key}"]`) as HTMLElement | null;
+ const summaryElement = Array.from(
+ container.querySelectorAll<HTMLElement>('[data-setting-summary-key]')
+ ).find((node) => node.dataset.settingSummaryKey === key) ?? null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const refreshSettingSummary = (key: string): void => { | |
| const summaryElement = container.querySelector(`[data-setting-summary-key="${key}"]`) as HTMLElement | null; | |
| const refreshSettingSummary = (key: string): void => { | |
| const summaryElement = Array.from( | |
| container.querySelectorAll<HTMLElement>('[data-setting-summary-key]') | |
| ).find((node) => node.dataset.settingSummaryKey === key) ?? null; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/config-editor/src/app.ts` around lines 532 - 533,
refreshSettingSummary currently interpolates raw key into
container.querySelector which can throw if key contains selector
meta-characters; change it to avoid selector interpolation by either using
CSS.escape(key) when available (e.g.
container.querySelector(`[data-setting-summary-key="${CSS.escape(key)}"]`) ) or,
more robustly, select all elements with the attribute and find the matching one
by value (e.g. container.querySelectorAll('[data-setting-summary-key]').forEach
/ Array.from(...).find(el => el.getAttribute('data-setting-summary-key') ===
key)). Update the code in the refreshSettingSummary function (and any other
places using container.querySelector with data-setting-summary-key) to use one
of these safe approaches.
Comment on lines
+769
to
+787
| const values = payload.entries | ||
| .map((entry) => `${entry.key}=${JSON.stringify(entry.value)}`) | ||
| .join(', '); | ||
| const changeText = change | ||
| ? `Updated ${change.key} to ${JSON.stringify(change.value)}.` | ||
| : 'Configuration updated.'; | ||
| app.updateModelContext({ | ||
| content: [{ type: 'text', text: `${changeText} Snapshot (${reason}): ${values}` }], | ||
| structuredContent: { | ||
| reason, | ||
| changedKey: change?.key, | ||
| changedValue: change?.value, | ||
| entries: payload.entries.map((entry) => ({ | ||
| key: entry.key, | ||
| label: entry.label, | ||
| value: entry.value, | ||
| valueType: entry.valueType, | ||
| })), | ||
| }, |
Contributor
There was a problem hiding this comment.
Minimize/redact config values before sending model context snapshots.
Full config value snapshots in model context can expose sensitive data. Prefer sending changed key metadata or redacted summaries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/config-editor/src/app.ts` around lines 769 - 787, The current
app.updateModelContext call includes full configuration values (payload.entries
and change.value) which may expose secrets; instead, redact sensitive values
before sending by replacing entry.value and change.value with a sanitized
representation (e.g., "<redacted>" or a hashed/summary string) and only include
non-sensitive metadata (entry.key, entry.label, valueType, changedKey) or a
boolean/summary flag for whether a value changed; update the construction of
content and structuredContent passed to app.updateModelContext (references:
payload.entries, change, app.updateModelContext, structuredContent.changedValue)
so no raw config values are serialized into the model context.
Comment on lines
+802
to
+807
| onConfigChanged: (change) => { | ||
| if (controller.state.payload) { | ||
| widgetState.write(controller.state.payload); | ||
| } | ||
| syncModelContext('widget-edit', change); | ||
| }, |
Contributor
There was a problem hiding this comment.
Schedule a rerender after successful config change propagation.
controller.apply() refreshes state from server, but this callback does not trigger a rerender, so UI can stay stale relative to refreshed payload.
💡 Proposed fix
onConfigChanged: (change) => {
if (controller.state.payload) {
widgetState.write(controller.state.payload);
}
syncModelContext('widget-edit', change);
+ scheduleRender();
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/config-editor/src/app.ts` around lines 802 - 807, The onConfigChanged
callback should call controller.apply() to refresh state from the server and
only after that resolves write the refreshed payload and sync context, so change
the body of onConfigChanged to await controller.apply(), then call
widgetState.write(controller.state.payload) and syncModelContext('widget-edit',
change), and finally trigger a UI rerender (e.g., call the component's
setState/forceUpdate or equivalent render trigger) so the updated payload is
reflected in the UI; reference: onConfigChanged, controller.apply,
widgetState.write, syncModelContext.
Comment on lines
+10
to
+15
| interface CreateArrayModalControllerOptions { | ||
| container: HTMLElement; | ||
| parseEntryItems: (entry: ArrayModalEntry) => string[]; | ||
| formatEntryTitle: (entry: ArrayModalEntry) => string; | ||
| onSave: (entryKey: string, items: string[]) => void | Promise<void>; | ||
| } |
Contributor
There was a problem hiding this comment.
Close the modal only on successful save.
The current finally { close(); } path closes the editor even on failed persistence, which can discard in-modal work and force users to re-enter values.
💡 Proposed fix
interface CreateArrayModalControllerOptions {
container: HTMLElement;
parseEntryItems: (entry: ArrayModalEntry) => string[];
formatEntryTitle: (entry: ArrayModalEntry) => string;
- onSave: (entryKey: string, items: string[]) => void | Promise<void>;
+ onSave: (entryKey: string, items: string[]) => boolean | Promise<boolean>;
}
...
modalSave?.addEventListener('click', async () => {
if (!modalEntryKey) {
return;
}
const changedKey = modalEntryKey;
modalItems = collectModalItemsFromDom();
- try {
- await onSave(changedKey, modalItems);
- } finally {
+ const saved = await onSave(changedKey, modalItems);
+ if (saved) {
close();
}
});Also applies to: 182-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/config-editor/src/array-modal.ts` around lines 10 - 15, The modal is
being closed in a finally block even when persistence fails; update the save
flow so close() is called only after a successful onSave resolution: remove or
replace the finally { close(); } with logic that awaits onSave(entryKey, items),
and on successful completion calls close(), while catching and handling errors
(e.g., show an error/toast and do not call close()). Update any identical
finally usages around the same save handler (see the save-related code that
references onSave and close, including the similar block at lines ~182-193) so
failures do not dismiss the editor.
Comment on lines
+26
to
+33
| .compact-chevron { | ||
| width: 14px; | ||
| height: 14px; | ||
| fill: currentColor; | ||
| color: var(--muted); | ||
| flex-shrink: 0; | ||
| transition: transform 200ms ease, color 150ms ease; | ||
| } |
Contributor
There was a problem hiding this comment.
Fix currentColor casing per Stylelint rules.
Proposed fix
.compact-chevron {
width: 14px;
height: 14px;
- fill: currentColor;
+ fill: currentcolor;
color: var(--muted);
flex-shrink: 0;
transition: transform 200ms ease, color 150ms ease;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .compact-chevron { | |
| width: 14px; | |
| height: 14px; | |
| fill: currentColor; | |
| color: var(--muted); | |
| flex-shrink: 0; | |
| transition: transform 200ms ease, color 150ms ease; | |
| } | |
| .compact-chevron { | |
| width: 14px; | |
| height: 14px; | |
| fill: currentcolor; | |
| color: var(--muted); | |
| flex-shrink: 0; | |
| transition: transform 200ms ease, color 150ms ease; | |
| } |
🧰 Tools
🪛 Stylelint (17.3.0)
[error] 29-29: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/styles/components/compact-row.css` around lines 26 - 33, The CSS uses
the keyword value "currentColor" with incorrect casing per Stylelint; update the
.compact-chevron rule to use the canonical lowercase keyword "currentcolor" for
the fill property (i.e., change fill: currentColor; to fill: currentcolor;) so
it satisfies the linter while leaving other properties and transitions
unchanged.
Owner
switch box colors is weird in comparison to what claude uses itself |
Owner
|
I would make this one expanded from first call |
Validate blank numeric input, keep array modal open on save failures, bind tool-bridge listeners safely, and tune host-driven toggle styling/expansion behavior for embedded hosts.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/ui/config-editor/src/app.ts (4)
535-536:⚠️ Potential issue | 🟠 MajorAvoid raw key interpolation in selector construction.
Line 536 can throw when a config key contains selector meta characters. Use attribute matching without selector interpolation.
💡 Proposed fix
- const refreshSettingSummary = (key: string): void => { - const summaryElement = container.querySelector(`[data-setting-summary-key="${key}"]`) as HTMLElement | null; + const refreshSettingSummary = (key: string): void => { + const summaryElement = Array.from( + container.querySelectorAll<HTMLElement>('[data-setting-summary-key]') + ).find((node) => node.dataset.settingSummaryKey === key) ?? null; if (!summaryElement) { return; }#!/bin/bash # Verify selector interpolation site and inspect potential key sources. rg -nP '\[data-setting-summary-key="\$\{key\}"\]' src/ui/config-editor/src/app.ts -C2 fd 'config-field-definitions\.(js|ts)$' --exec rg -nP '^\s*[A-Za-z0-9_]+\s*:' {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/config-editor/src/app.ts` around lines 535 - 536, The selector construction in refreshSettingSummary by interpolating key into querySelector can throw for keys containing selector meta-characters; instead, select elements by the attribute name (e.g., container.querySelectorAll('[data-setting-summary-key]')) and then find the one whose getAttribute('data-setting-summary-key') === key (or use CSS.escape(key) if you prefer to keep a single selector), updating the logic in refreshSettingSummary to avoid direct string interpolation into querySelector.
772-790:⚠️ Potential issue | 🟠 MajorRedact config values before model-context sync.
Line 772-Line 790 sends raw config values (
entry.value,change.value) into model context snapshots. This is a privacy/compliance risk.💡 Proposed fix
- const values = payload.entries - .map((entry) => `${entry.key}=${JSON.stringify(entry.value)}`) - .join(', '); + const values = payload.entries + .map((entry) => `${entry.key}=<redacted>`) + .join(', '); const changeText = change - ? `Updated ${change.key} to ${JSON.stringify(change.value)}.` + ? `Updated ${change.key}.` : 'Configuration updated.'; app.updateModelContext({ content: [{ type: 'text', text: `${changeText} Snapshot (${reason}): ${values}` }], structuredContent: { reason, changedKey: change?.key, - changedValue: change?.value, + changedValue: change ? '<redacted>' : undefined, entries: payload.entries.map((entry) => ({ key: entry.key, label: entry.label, - value: entry.value, + value: '<redacted>', valueType: entry.valueType, })), }, }).catch(() => {#!/bin/bash # Verify raw value serialization in model context payload. rg -nP 'updateModelContext|JSON\.stringify\(entry\.value\)|changedValue:\s*change\?\.value|value:\s*entry\.value' src/ui/config-editor/src/app.ts -C3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/config-editor/src/app.ts` around lines 772 - 790, The model-context snapshot is including raw config values (payload.entries -> entry.value and change?.value) via JSON.stringify and direct assignment in app.updateModelContext; redact sensitive values before syncing by replacing entry.value and change.value with a masked placeholder (e.g., "<redacted>" or a sanitized summary) or removing them entirely while preserving other metadata (key, label, valueType, reason, changedKey). Update the map over payload.entries and the structuredContent.changedValue construction used in app.updateModelContext so they use the redacted/masked value instead of the raw entry.value/change.value, and ensure the content text uses the redacted representation as well.
565-573:⚠️ Potential issue | 🟠 MajorKeep array modal open when apply returns
ok: false.Line 568-Line 573 resolves without throwing on failed apply, so the modal still closes despite unsuccessful persistence.
💡 Proposed fix
onSave: async (changedKey, items) => { controller.setSelection(changedKey); controller.setDraftValue(items.join('\n')); const result = await controller.apply(); emitTooltip(result); - if (result.ok) { - emitConfigChanged(changedKey, items); - } + if (!result.ok) { + throw new Error(result.tooltip?.message ?? 'Failed to save list changes.'); + } + emitConfigChanged(changedKey, items); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/config-editor/src/app.ts` around lines 565 - 573, The onSave handler currently awaits controller.apply() and always resolves, which lets the caller close the array modal even when persistence fails; change onSave (the async callback using controller.setSelection, controller.setDraftValue, controller.apply, emitTooltip, emitConfigChanged) so that if the apply result has result.ok === false it emits the tooltip and then rejects (throw) or returns a rejected promise with the apply error/details instead of resolving—only when result.ok is true should it call emitConfigChanged and allow normal completion so the modal can close.
805-810:⚠️ Potential issue | 🟠 MajorSchedule rerender after successful config-change propagation.
Line 805-Line 810 updates state/context but does not trigger rerender, so UI can remain stale after refreshed payload updates.
💡 Proposed fix
onConfigChanged: (change) => { if (controller.state.payload) { widgetState.write(controller.state.payload); } syncModelContext('widget-edit', change); + scheduleRender(); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/config-editor/src/app.ts` around lines 805 - 810, The onConfigChanged handler writes the new payload and syncs context but never triggers a UI update, leaving the UI stale; after widgetState.write(controller.state.payload) and syncModelContext('widget-edit', change) ensure you schedule a rerender by invoking the component's update mechanism (e.g., call the controller's state updater or refresh method such as controller.setState/update/refresh or a dedicated widgetState.notify/refresh) so the view reflects the propagated config change; place this call only after successful write/sync to avoid redundant renders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/ui/config-editor/src/app.ts`:
- Around line 535-536: The selector construction in refreshSettingSummary by
interpolating key into querySelector can throw for keys containing selector
meta-characters; instead, select elements by the attribute name (e.g.,
container.querySelectorAll('[data-setting-summary-key]')) and then find the one
whose getAttribute('data-setting-summary-key') === key (or use CSS.escape(key)
if you prefer to keep a single selector), updating the logic in
refreshSettingSummary to avoid direct string interpolation into querySelector.
- Around line 772-790: The model-context snapshot is including raw config values
(payload.entries -> entry.value and change?.value) via JSON.stringify and direct
assignment in app.updateModelContext; redact sensitive values before syncing by
replacing entry.value and change.value with a masked placeholder (e.g.,
"<redacted>" or a sanitized summary) or removing them entirely while preserving
other metadata (key, label, valueType, reason, changedKey). Update the map over
payload.entries and the structuredContent.changedValue construction used in
app.updateModelContext so they use the redacted/masked value instead of the raw
entry.value/change.value, and ensure the content text uses the redacted
representation as well.
- Around line 565-573: The onSave handler currently awaits controller.apply()
and always resolves, which lets the caller close the array modal even when
persistence fails; change onSave (the async callback using
controller.setSelection, controller.setDraftValue, controller.apply,
emitTooltip, emitConfigChanged) so that if the apply result has result.ok ===
false it emits the tooltip and then rejects (throw) or returns a rejected
promise with the apply error/details instead of resolving—only when result.ok is
true should it call emitConfigChanged and allow normal completion so the modal
can close.
- Around line 805-810: The onConfigChanged handler writes the new payload and
syncs context but never triggers a UI update, leaving the UI stale; after
widgetState.write(controller.state.payload) and syncModelContext('widget-edit',
change) ensure you schedule a rerender by invoking the component's update
mechanism (e.g., call the controller's state updater or refresh method such as
controller.setState/update/refresh or a dedicated widgetState.notify/refresh) so
the view reflects the propagated config change; place this call only after
successful write/sync to avoid redundant renders.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/ui/config-editor/src/app.tssrc/ui/config-editor/src/array-modal.tssrc/ui/shared/tool-bridge.tssrc/ui/styles/apps/config-editor.css
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui/styles/apps/config-editor.css
This was referenced Apr 3, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
CodeAnt-AI Description
Add a host-agnostic interactive Config Editor UI and shared UI runtime primitives
What Changed
Impact
✅ Clearer editable configuration UI from tool output✅ Shorter cycle to apply and verify config changes✅ Consistent embed behavior across hosts (compact header, theme, and context)💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Improvements
Style
Tests