Reagan/eng 4716 UI for desktop app#438
Merged
Merged
Conversation
Pure data layer for an AI SDK Elements-shaped message model: typed parts (text, reasoning, tool, file, notify) and a side-effect-free fromHlEvents adapter that converts the legacy HlEvent stream into UIMessageV2 records. No renderer changes yet; this exists so we can drop in individual elements components (Attachments, InlineCitation, etc.) surgically without rewriting the transcript. The 'thinking' HlEvent is mapped to a text part because in this codebase it carries the assistant's streaming reply text (claude-code content_block_delta.text_delta, codex agent_message), not chain-of- thought. Real reasoning blocks would need a new HlEvent type. Includes 19-case unit suite covering streaming, multi-turn flush, tool timing pairing, error/done terminals, and done.summary fallback for tool-only iterations.
Drops in an AI SDK Elements-shaped Attachments composable (Attachments / Attachment / AttachmentPreview / AttachmentInfo) and an AttachmentList convenience helper, wired into ChatTurn so consecutive non-hoisted file_output entries collapse into a single grid instead of one FileCard per row. Single file_outputs (image or otherwise) also flow through the same component so layout is consistent. The trailing hoisted-image magazine layout is untouched — that's intentional UX. Clicks on attachment chips still reveal the file in the system file manager via the existing electronAPI bridge.
The renderer has ~134 ad-hoc console.* calls scattered across modules
with no on-disk trail — when something breaks in production, the only
record lives in DevTools. This adds a small structured logger:
makeLogger('EnginePicker') → { debug, info, warn, error }
Console output keeps the existing [Module] action.subaction convention
so visual grep still works. Warn and error are forwarded over a new
renderer:log IPC channel to the existing rendererLogger ChannelLogger,
which writes JSONL to <userData>/logs/renderer.log alongside main and
browser logs. Info forwards only when localStorage.rendererLog is set
to 'verbose'; debug never forwards.
Includes payload sanitization on the main side (level whitelist,
namespace/message length caps, extras key cap, oversized string
truncation) so a bad renderer payload can't pollute the on-disk log.
26 unit tests cover both the IPC validator and the renderer-side
logger (forwarding rules, Error normalization, mode toggling).
The existing console.* call sites continue to work unchanged — this
is opt-in per module. See logger.README.md for the migration recipe.
Two highest-value call sites converted from console.* to makeLogger:
- hub/index.tsx: window.error and unhandledrejection handlers. These
capture renderer crashes — exactly the case where DevTools-only
logging is least useful. They now hit the on-disk renderer.log.
- hub/EnginePicker.tsx: 19 call sites across status refresh, install,
login, and provider settings flows. Engine bring-up is the most
common place users hit failures; persisting warn/error here gives
a real trail to read after the fact.
About 110 other console.* call sites are left as-is. They keep working;
migrate opportunistically when touching the surrounding code.
User attachments (pasted/dropped images sent with a prompt) were
persisted into session_attachments by turn_index, but no IPC handler
exposed them to the renderer — so the chat transcript showed only the
prompt text and the image silently vanished from view even though the
agent received and processed it.
End-to-end wiring:
- SessionManager.getAttachmentsByTurnIndex passthrough (manager owns
session-scoped reads; the IPC handler doesn't touch the DB
directly).
- sessions:get-attachments-by-turn IPC handler — validates
sessionId/turnIndex, encodes bytes as data: URLs with the recorded
MIME so the renderer can <img src=...> without raw blob plumbing.
- electronAPI.sessions.getAttachmentsByTurn preload bridge.
- attachmentTurnIndex propagated through OutputEntry so the renderer
knows which turn's blobs to fetch.
- sessionId threaded through ChatTranscript → ChatTurn → UserBubble.
- UserBubble lazily fetches on mount and renders via the existing
AttachmentList composable above the bubble text.
Data URLs are sent lazily on demand, so the session payload stays
small. Image bytes never travel as raw buffers across IPC.
User-attached images now render as compact square cards (160-180px) with the image filling the entire tile, instead of small thumb + name chips. Filename and size fade in as a bottom overlay on hover/focus so the visual is image-first like AI SDK Elements' attachments grid. Gallery is a new variant on the existing Attachments composable; the chip-style 'grid' variant is unchanged for agent file_outputs where the name + meta are the primary affordance. UserBubble uses gallery.
180px was reading as a hero image above a short user prompt. 120px keeps the gallery feeling like attachment chips, leaves the bubble as the focal element, and still surfaces enough preview to be recognizable. Hover overlay unchanged.
object-fit: cover at width/height: 100% was upscaling tiny images (blur) and center-cropping wide screenshots (showed only the middle strip). Switching to width/height: auto with max-width/height: 100% and object-fit: contain means images render at their natural size up to the 120px frame, then shrink-to-fit with aspect preserved. Trade-off: a wide screenshot now gets dark letterbox bars top/bottom inside the tile instead of a cropped strip. The hover overlay still shows filename + size for context. Worth the safety — no pixelated upscales, no surprise cropping.
Letterboxed thumbnails read as 'safe' but break the polished image- forward grid the gallery variant is meant to look like. Going back to cover — small images upscale and wide images get center-cropped, which is the accepted trade-off for tiles that feel like attachment cards rather than thumbnails.
fs.watch fires multiple change events while a file is being written. The previous (filename, size) dedup let every intermediate size through, so a single screenshot save produced 2-3 file_output events and the chat rendered the same image multiple times. Debounce per filename (200ms) and only emit after the size stops growing.
Previously the preview card rendered as soon as the session attached a browser, showing a default-icon placeholder before any navigation. Wait for a real http(s) URL before rendering the wrap at all.
Two coupled pieces for surfacing agent-emitted HTML artifacts (plans, explanations, comparisons) as interactive cards instead of as inline fenced-code markdown. htmlBlocks.ts — streaming extractor Pure stateful parser: HtmlBlockExtractor.feed(chunk) → events. Hides three engines' chunking variance (claude-code's per-token deltas, codex's paragraph chunks, browsercode/opencode's one-liner parts) behind a single (text, html_block, text, ...) output. Uses strict fence regex during streaming (require \n terminator) and lax during the final end() flush (accept end-of-input) so fences arriving at the very last token still resolve. Closer uses a lookahead so the trailing \n stays in the buffer for the following text — fixes paragraph-break preservation. HtmlBlock.tsx — sandboxed renderer iframe srcdoc + sandbox="allow-same-origin" (no allow-scripts). Static HTML+CSS only — JS in artifacts is intentionally blocked. Parent measures contentDocument.scrollHeight on load + via a ResizeObserver on documentElement/body so layout shifts from font/ image loads reflect into the iframe height. Capped at 720px tall; overflowing content auto-collapses to 360px with an Expand toggle. Streaming-incomplete blocks render with a 'streaming…' badge so the partial render reads as intentional. <base target="_blank"> means any anchor opens in the user's default browser, not inside the sandboxed frame. Hard benchmark: 36 unit tests covering single block, mixed prose, back-to-back blocks, fence split at every chunk boundary (1-char chunking invariance), never-closed streams, adversarial lookalike content (```js, bare ```, mid-line fences), per-engine fixtures (claude-code many-tiny-deltas, codex paragraph chunks, browsercode one-liner), and round-trip byte preservation. Not wired into ChatTurn yet — that's a separate change once we decide the prompt-engineering side (system-prompt nudge for engines to emit ```html blocks for plans/explanations).
End-to-end: agent emits ```html fenced blocks → engine adapter system
prompt teaches every supported agent (claude-code, codex, opencode/
browsercode) when to use them → ChatTurn's StreamingProse runs the
extractor and renders each block as a sandboxed <HtmlBlock> instead
of inline markdown.
Provider-neutral guidance lives in skillIndexPrompt.ts as a new
constant HTML_BLOCK_GUIDANCE_LINES spread into every adapter's
wrapPrompt alongside the existing skill discovery lines. Adapter-side
changes are import-and-spread only — no per-engine branching.
Renderer:
- StreamingProse short-circuits when no html block is present so the
existing typewriter + stable-markdown flow is untouched for plain
prose. When blocks ARE present, render text and html_block events
in document order; typewriter is suppressed (it doesn't compose
with iframe layout). Streaming-incomplete blocks surface their
complete=false state down to HtmlBlock for the streaming badge.
Test spec for the floated-image case was adjusted to also accept the
existing .chat-step__image render path (first image becomes the
float anchor; only subsequent files flow through the Attachments
grid).
House style for agent-emitted ```html blocks: 3px solid #000 borders, hard offset shadows (no blur), square corners, flat fills, two-accent palette against a cream/off-white card lifted off the renderer's dark surface. Documents the visual language, mandatory rules, system-font stack (no external resources, sandbox blocks them), and component recipes (card, button, status pill, step list, comparison, table) plus a copy-paste reference artifact. Lives in stock/interaction-skills so it ships with every harness and shows up in 'agent-skill search'. The HTML_BLOCK_GUIDANCE_LINES nudge in skillIndexPrompt.ts already tells agents when to use HTML blocks; this skill answers the 'okay but what should they look like' question.
Theme propagation:
- htmlBlockGuidanceLines(theme) replaces the static constant. Each
engine adapter calls resolveThemeMode() at wrapPrompt time and
passes the resolved 'light' | 'dark' into the function so the
agent sees a 'UI THEME: light' / 'UI THEME: dark' line.
- HtmlBlock reads useThemeMode() and threads the resolved value into
wrap(), which swaps a small token set (fg, link, code bg, rules)
per theme. The agent's own styles still win — these tokens are
only the fallback for naked HTML.
Skill rewrite: 'neobrutalist-html.md' is now pure styling. Removed:
- 'When to use' / 'When not to use' guidance (that's the agent's
call per task)
- Component recipes (card, button, status pill, step list,
comparison, table)
- The reference plan artifact
What stays: mandatory visual rules (3px solid #000 borders, hard
offset shadows, square corners, flat fills, system-font stack),
banned items, two palettes (light vs dark) with role-mapped hexes,
two-accent rule, typography sizes. The agent picks structure and
elements freely; we only constrain the look.
htmlBlockGuidanceLines(theme) now emits two lines instead of one: UI THEME: dark. When you emit a ```html block, use the active palette below. The full per-theme reference lives in the 'neobrutalist-html' interaction skill. Active palette — card bg #1c1c20, border #f4ecd8, shadow #f4ecd8, foreground #f4ecd8. Accents: red #ff5252, blue #4ea3ff, green #3ddc84, gold #ffd400. Pick one bold accent + one secondary per artifact. The agent can paste those hexes verbatim without re-reading the skill file. The skill stays the source-of-truth reference for both palettes together; this just removes the lookup step at use time. ACTIVE_PALETTE in skillIndexPrompt.ts is hand-kept in sync with the skill — change them together.
…r affordance, theme tokens
Restructure the hub sidebar to surface primary CTAs at the top: - New top section with two rows: 'New chat' (returns to dashboard) and 'Search' (opens the native pill). - Existing agent list is now an explicit 'Agents' group below, with the + button moved into the group header. Renderer-only; no IPC, no behavior changes for sessions.
Replace the ad-hoc filename pill row in the dashboard composer with the existing AI SDK Elements-shaped <AttachmentList> from chat-v2. Image attachments now render real thumbnails (via object URLs); non-image files keep the file-icon-with-extension preview. Same component now drives both dashboard composer and chat transcript, so any future attachment polish lives in one place. - AttachmentList/Attachment learn an optional onRemove + close-button overlay (revealed on hover). Chat usage is unaffected. - Object URLs are revoked on remove/unmount.
- Lift the inline hub-toolbar header into a small <Navbar /> component; rename .hub-toolbar* CSS to .hub-navbar* to match. - Drop the 'Browser Use' brand button and the top-right 'New agent' CTA (the sidebar's 'New chat' + per-row actions cover the same ground). - Reduce navbar height 70px -> 46px and remove the 36px top padding; the chat/dashboard now starts much closer to the top edge. - macOS only: pin the MemoryIndicator beside the traffic-light cluster at the same height/baseline (uses [data-platform='mac'] set in index.tsx via UA sniff). On Windows/Linux the indicator flows inline in the navbar with no chip chrome. - MemoryIndicator and Settings buttons gain a subtle bordered hover treatment (border:1px transparent -> --color-border-subtle on hover) so they read as the same chip family without layout shift.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
17 issues found across 60 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/renderer/hub/chat-v2/Attachments.tsx">
<violation number="1" location="app/src/renderer/hub/chat-v2/Attachments.tsx:54">
P1: This component can render a nested `<button>` (remove button inside clickable attachment button) when both `onClick` and `onRemove` are provided.</violation>
</file>
<file name="app/src/renderer/hub/chat-v2/AskForm.tsx">
<violation number="1" location="app/src/renderer/hub/chat-v2/AskForm.tsx:133">
P2: `canSubmit` is true for an empty question list, which enables submitting an empty ask form.</violation>
<violation number="2" location="app/src/renderer/hub/chat-v2/AskForm.tsx:175">
P2: Cached submissions do not persist "Other" free-text values, causing submitted answers to re-render with missing content after remount.</violation>
</file>
<file name="app/src/renderer/hub/chat-v2/optionListStore.ts">
<violation number="1" location="app/src/renderer/hub/chat-v2/optionListStore.ts:16">
P2: Cache key construction is delimiter-ambiguous and can collide across different option/question sets, causing incorrect submission state restoration.</violation>
</file>
<file name="app/src/renderer/hub/chat-v2/fromHlEvents.ts">
<violation number="1" location="app/src/renderer/hub/chat-v2/fromHlEvents.ts:20">
P2: Module-level `_idCounter` makes `fromHlEvents` impure: repeated calls with the same events produce different IDs. If this is called on re-render, every message gets a new key, causing React to unmount/remount all children. Consider deriving deterministic IDs from the event index or accepting a seed/offset parameter.</violation>
<violation number="2" location="app/src/renderer/hub/chat-v2/fromHlEvents.ts:232">
P2: `file_output` doesn't close preceding streaming text/reasoning parts. After a `file_output` is appended, `closeStreamingText` (called later by `done`) iterates from the end, encounters the non-text file part, and returns without closing the earlier streaming text—leaving it in `state: 'streaming'` on a `status: 'done'` message.</violation>
<violation number="3" location="app/src/renderer/hub/chat-v2/fromHlEvents.ts:246">
P2: `notify` has the same issue as `file_output`—it doesn't close preceding streaming text/reasoning, leaving them stuck in `'streaming'` state if a `done` event follows.</violation>
</file>
<file name="app/src/renderer/hub/chat-v2/htmlBlocks.ts">
<violation number="1" location="app/src/renderer/hub/chat-v2/htmlBlocks.ts:481">
P2: Partial-parse path doesn't clamp `min` to 0 like `validateSection` does. A negative `"min"` value in the streaming JSON would pass through unclamped, producing an `OptionListSection` that violates the invariant the full-parse path enforces.</violation>
</file>
<file name="app/src/main/hl/engines/runEngine.ts">
<violation number="1" location="app/src/main/hl/engines/runEngine.ts:453">
P2: Pending debounced output events are dropped on process exit because timers are canceled instead of flushed.</violation>
</file>
<file name="app/src/renderer/hub/chat-v2/HtmlBlock.tsx">
<violation number="1" location="app/src/renderer/hub/chat-v2/HtmlBlock.tsx:75">
P2: Stale closure in ResizeObserver causes expand/collapse to malfunction. The ResizeObserver captures the initial `measureAndSet` where `naturalHeight` is always `FALLBACK_HEIGHT_PX` in the closure. On subsequent ticks, the auto-collapse condition is always met for tall content, re-collapsing the view even after the user clicks "Expand". Use a ref to track whether auto-collapse has already fired instead of relying on the stale state value.</violation>
</file>
<file name="app/src/main/hl/stock/interaction-skills/options-block.md">
<violation number="1" location="app/src/main/hl/stock/interaction-skills/options-block.md:62">
P2: "AND" should be "OR" — the intent is that options missing any of the three required fields (id, image, title) are dropped, not only options missing all three.</violation>
<violation number="2" location="app/src/main/hl/stock/interaction-skills/options-block.md:253">
P2: Broken cross-reference — "the JS snippet above" doesn't exist. The "Getting the data from the page" section only has prose principles, no code block.</violation>
</file>
<file name="app/src/main/index.ts">
<violation number="1" location="app/src/main/index.ts:1735">
P2: This handler eagerly base64-encodes and IPC-transfers bytes for every attachment, including non-images that the UI does not preview. For large PDFs/text files this can create very large synchronous payloads and cause noticeable main/renderer slowdown.</violation>
</file>
<file name="app/src/renderer/hub/chat-v2/OptionList.tsx">
<violation number="1" location="app/src/renderer/hub/chat-v2/OptionList.tsx:197">
P2: When submit is blocked only by missing "Other" text, the button label can show `section 0` due to an unhandled `findIndex` result of `-1`.</violation>
<violation number="2" location="app/src/renderer/hub/chat-v2/OptionList.tsx:278">
P1: Single-select Enter submits with stale state because submit is called from a deferred callback created before the selection state update is applied.</violation>
</file>
<file name="app/src/renderer/hub/chat/ChatTurn.tsx">
<violation number="1" location="app/src/renderer/hub/chat/ChatTurn.tsx:76">
P1: Guard the optional IPC call result before chaining promises; otherwise this can throw when the Electron bridge method is unavailable.</violation>
<violation number="2" location="app/src/renderer/hub/chat/ChatTurn.tsx:805">
P2: Use optional chaining on `.catch` (or guard the returned promise) to avoid click-time crashes when `revealOutput` is unavailable.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
The BrowserPreview component now intentionally renders nothing until a session has a previewable http(s) lastUrl. The stale test still expected the old pre-navigation placeholder, so update the coverage to guard the current three-state contract: no URL renders nothing, URL without frame renders the placeholder, and URL with frame renders the image. Constraint: BrowserPreview intentionally gates rendering on previewable lastUrl. Rejected: Reverting the component to show the pre-navigation placeholder | that would reintroduce visual noise before navigation. Confidence: high Scope-risk: narrow Tested: yarn test tests/unit/hub/BrowserPreview.spec.tsx Tested: yarn eslint tests/unit/hub/BrowserPreview.spec.tsx Not-tested: full yarn test blocked by unrelated better-sqlite3 Node ABI mismatch. Co-authored-by: OmX <omx@oh-my-codex.dev>
The reported issues all traced to small state-boundary mismatches: cached answers lacked the complete payload, streaming transcript transforms left stale live parts behind later non-text events, and several renderer IPC/UI paths assumed capabilities or DOM structure that were not guaranteed. This keeps the fixes local to the affected chat, option, attachment, output-watch, and documentation surfaces while adding regression coverage for each behavioral failure. Constraint: The full unit suite is blocked locally by a better_sqlite3 native module ABI mismatch under Node v22.22.2. Rejected: Change shared renderer architecture | each issue had a narrow local cause and regression surface. Confidence: high Scope-risk: moderate Directive: Preserve deterministic event IDs and complete option submission records when extending chat-v2 transforms or stores. Tested: yarn test tests/unit/chat-v2/Attachments.spec.tsx tests/unit/chat-v2/AskForm.spec.tsx tests/unit/chat-v2/OptionList.spec.tsx tests/unit/chat-v2/HtmlBlock.spec.tsx tests/unit/chat-v2/fromHlEvents.test.ts tests/unit/chat-v2/optionBlocks.test.ts tests/unit/chat-v2/optionListStore.test.ts tests/unit/hub/ChatTurn.spec.tsx tests/unit/hl/runEngineHarnessWatcher.test.ts Tested: yarn typecheck Tested: yarn lint Tested: git diff --check Not-tested: Full yarn test completion because tests/unit/sessions/SessionDb.schemaIdentity.test.ts fails to load better_sqlite3.node compiled for NODE_MODULE_VERSION 145 while current Node expects 127. Co-authored-by: OmX <omx@oh-my-codex.dev>
There was a problem hiding this comment.
1 issue found across 22 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Ask form selections were already restored by encoded question identity, but the accompanying Other text still used the question array index. A reordered form with the same cache key could therefore display the right Other selection with another question's text. This stores AskForm free-text answers in the submission record by the same per-question cache key used for selection restoration, while leaving OptionList's index-based section cache unchanged. Constraint: Ask questions do not expose stable ids, so the cache identity is derived from header plus question text. Rejected: Reuse the old otherText array fallback | it preserves the exact reorder bug for records without keyed values. Confidence: high Scope-risk: narrow Directive: Do not restore AskForm free-text answers by array position unless ask questions gain explicit stable ids. Tested: yarn test tests/unit/chat-v2/AskForm.spec.tsx Tested: yarn typecheck Tested: yarn lint Tested: git diff --check Co-authored-by: OmX <omx@oh-my-codex.dev>
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
1 participant
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.
Summary by cubic
Delivers the new desktop chat UI primitives (sandboxed HTML blocks, options/ask pickers, unified attachments) and structured renderer logging to improve UX, safety, and diagnostics. Also stabilizes chat edge cases and reduces duplicate outputs and console noise as part of ENG-4716.
New Features
neobrutalist-htmlskill and injects guidance intoclaude-code,codex, andbrowsercode.optionspicker andaskquestionnaire with resume-on-submit.Attachmentscomponent; file_outputs collapse into grids; user-attached images shown inline viasessions:get-attachments-by-turn.makeLoggerin the renderer +renderer:logIPC to write to on-diskrenderer.log; migrated hub entry andEnginePicker.Navbar, sidebar quick actions (New chat, Search), hide browser preview until a real URL.Bug Fixes
file_outputper completed write and flushes pending events on fast engine exit.Written for commit 3f12d58. Summary will update on new commits. Review in cubic