feat(os+extension): complete OS chat and Ask Signet UX overhaul + Chrome/Firefox Extension Improvements#450
feat(os+extension): complete OS chat and Ask Signet UX overhaul + Chrome/Firefox Extension Improvements#450
Conversation
This comment was marked as low quality.
This comment was marked as low quality.
| if (!match) return message; | ||
| const executable = match[1]; | ||
| if (executable === "docker") { | ||
| return "Docker CLI is required for this MCP app but was not found in PATH. Install Docker Desktop (with CLI), then restart Signet daemon and retry."; |
There was a problem hiding this comment.
We may want to defer this until we've sorted out all of the duplicated provider logic, this is insane. (not your fault)
|
Hi @aaf2tbz - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the feature work in |
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Automated review by pr-reviewer | model: claude-sonnet-4-6 | commit:
fcb93b72
[Automated Review — pr-reviewer bot] PR #450 introduces several UX and reliability improvements but has two blocking issues: a scoping invariant violation on memory reads and an unhandled promise rejection that causes silent render-window lockup.
Confidence: High [sufficient_diff_evidence, targeted_context_included] - Both blocking issues are directly readable in the added code. The missing agent_id is unambiguous in refreshBrowserCategories (SidebarGroups.svelte). The missing .catch() on fetchWidgetHtml().then() is unambiguous in OsTab.svelte's SSE handler. The SpeechRecognition.start(track) misuse is verifiable against the Web Speech API spec. No runtime trace or cross-module context is needed to confirm these findings.
| } | ||
|
|
||
| if ( | ||
| tags.has("browser-tool") || |
There was a problem hiding this comment.
Agent scoping violation: refreshBrowserCategories fetches /api/memories?limit=500&offset=0 without an agent_id parameter. AGENTS.md mandates threading agent_id through every read/write that touches user data. In a multi-agent workspace this will return all agents' memories, not just the active agent's. The fix is to pass the current agent ID: /api/memories?limit=500&offset=0&agent_id=${encodeURIComponent(agentId)}.
| } | ||
| }); | ||
| } else if (event.type === "widget.error" && event.payload?.serverId) { | ||
| onWidgetGenerationFailed(event.payload.serverId); |
There was a problem hiding this comment.
Unhandled promise rejection: fetchWidgetHtml(event.payload.serverId).then(html => { ... }) has no .catch(). If the fetch rejects (network error, daemon restart), neither onWidgetGenerated nor onWidgetGenerationFailed is called. The render window stays permanently in a loading/blank state with no error surfaced and no recovery path. Add .catch(err => { onWidgetGenerationFailed(event.payload.serverId); setRenderWindowError(event.payload.serverId, err instanceof Error ? err.message : 'Failed to load widget.'); }).
| } | ||
|
|
||
| const ctor = getSpeechRecognitionCtor(); | ||
| if (!ctor) { |
There was a problem hiding this comment.
The Web Speech API SpeechRecognition.start() accepts no arguments — passing a MediaStreamTrack is outside the spec and silently ignored in all major browsers. The microphone device selection via getUserMedia is therefore a no-op: activeMicStream is acquired and held open but speech recognition always uses the browser-default microphone, making the device picker non-functional. Either remove the getUserMedia acquisition entirely (keep the select for informational purposes only) or document that device selection is a best-effort hint that only labels are retrieved from.
| const nextErrors = { ...renderWindowErrors }; | ||
| delete nextErrors[appId]; | ||
| renderWindowErrors = nextErrors; | ||
| return; |
There was a problem hiding this comment.
Silent failure in loadAppIntoRenderWindow: when the entry cannot be found after a tray refresh and activeRenderTabId is null (empty render window), the condition if (activeRenderTabId) setRenderWindowError(...) silently drops the error. The user sees nothing. Either always emit an error (using a local notice or a toast), or show it in the empty-state region.
| title="Clear all render tabs" | ||
| onclick={clearRenderWindow} | ||
| disabled={renderWindowTabIds.length === 0} | ||
| > |
There was a problem hiding this comment.
The tab close target is a <span> acting as an interactive control without role="button" or tabindex="0". It is unreachable by keyboard. Consider replacing with a <button> (with type="button") so it participates in the same focus model as the surrounding render tab buttons.
| type="button" | ||
| onclick={() => { browserSessionsExpanded = !browserSessionsExpanded; }} | ||
| > | ||
| <span>Browser Sessions</span> |
There was a problem hiding this comment.
CSS indentation inconsistency: .skill-command-preview block is written at column 0 while all surrounding style rules are indented to match the <style> block. This is a formatting divergence that may confuse future editors. The bun run check step in the dashboard should catch this.
Summary
This PR delivers a full OS chat and browser-extension UX/reliability overhaul, including provider/model routing fixes, Ask Signet in-page workflow upgrades, MCP render-window improvements, voice input support, and popup/navigation cleanup.
Context
The existing branch work solved multiple cross-surface issues (OS chat + extension): unstable model resolution, outdated fallback behavior, missing context-menu/overlay reliability, rough render-window ergonomics, and popup layout regressions. This PR consolidates and rebases those fixes onto latest
main.Changes
OS Chat / Dashboard
Daemon
/api/os/chat+/api/os/chat/modelsbehavior for provider/model reliability.Browser Extension
Send to Signet Os,View MCP Servers, transcribe routing through Ask Signet flow).Testing
bun run check(dashboard)bun run typecheck && bun run build(extension)bun run build.ts(daemon)PR Readiness Checklist
INDEX.md+dependencies.yaml)Checklist Status (blue checks)
origin/mainchore/chrome-extension-rework