feat(web): port chat page UI from platform — transcript, hooks, components#31109
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2fbfc81d2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const sendMessage = useCallback( | ||
| async (_content: string) => { | ||
| // TODO: wire up useSendMessage hook | ||
| }, |
There was a problem hiding this comment.
Once the lifecycle/API stubs are fixed enough for the chat UI to render, the primary chat action still drops user messages: ChatRouteContent clears the input and attachments, then awaits this sendMessage, but this callback does not call postChatMessage, update optimistic messages, or surface an error. The visible result is that pressing Send makes the draft disappear without sending anything to the assistant.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚩 Duplicate toast module files (toast.ts and toast.tsx) with conflicting exports
Both toast.ts and toast.tsx exist under components/. The .ts file exports toast as an object with success/error/info/warning methods (the correct shape for callsites like use-app-viewer-actions.ts). The .tsx file exports toast as an empty function. With NodeNext module resolution, import { toast } from '@/components/toast.js' resolves to .ts first, so the correct implementation wins at runtime. However, the .tsx file's existence is confusing and could cause issues if resolution order changes or if someone imports from the .tsx directly. One of these should be removed.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
🚩 ChatRouteContent refs type uses broad as unknown as cast to bridge incomplete stub types
In chat-page.tsx:278-299, the refs prop is cast with as unknown as ChatRouteContentProps['refs'] to satisfy the ChatRouteContent interface. The ChatRouteRefs type in chat-route-content.tsx:206-231 requires many refs (streamEpochRef, processingSnapshotsRef, historyLoadedRef, etc.) that the ChatPage scaffolding doesn't create. The as unknown as cast hides the missing refs at compile time but they'll be undefined at runtime, which will cause null-reference errors when any code path in ChatRouteContent or its children dereferences them (e.g. streamEpochRef.current). This is acceptable for an in-progress migration scaffold but should be tracked.
(Refers to lines 278-299)
Was this helpful? React with 👍 or 👎 to provide feedback.
| function PopoverRoot(_props: { children?: ReactNode; open?: boolean; onOpenChange?: (open: boolean) => void }) { return null; } | ||
| function PopoverAnchor(_props: { children?: ReactNode; [key: string]: unknown }) { return null; } | ||
| function PopoverContent(_props: { children?: ReactNode; [key: string]: unknown }) { return null; } |
There was a problem hiding this comment.
🔴 Popover stub returns null, silently dropping the entire ChatComposer form
PopoverRoot, PopoverAnchor, and PopoverContent are all stubbed to return null, ignoring their children prop. In ChatComposer (chat-composer.tsx:381-728), the entire form — textarea, send/stop buttons, attachments strip, and action bar — is nested inside <Popover.Root> → <Popover.Anchor asChild> → <form>. Because the stub drops children, the chat composer renders nothing: no input, no send button, no attachments. This also causes the ChatComposer tests (chat-composer.test.tsx) to fail, since renderToStaticMarkup(<ChatComposer .../>) produces empty output and assertions like expect(html).toContain('placeholder="Type something cool"') break.
Fix: make stubs pass through children
Unlike leaf-component stubs (e.g. `ChatSkeleton`, `AppCard`) that correctly return `null` because they ARE the rendered content, Popover is a structural/container component. Its stubs must forward `children` to avoid silently swallowing the real components nested inside.| function PopoverRoot(_props: { children?: ReactNode; open?: boolean; onOpenChange?: (open: boolean) => void }) { return null; } | |
| function PopoverAnchor(_props: { children?: ReactNode; [key: string]: unknown }) { return null; } | |
| function PopoverContent(_props: { children?: ReactNode; [key: string]: unknown }) { return null; } | |
| function PopoverRoot({ children }: { children?: ReactNode; open?: boolean; onOpenChange?: (open: boolean) => void }) { return <>{children}</>; } | |
| function PopoverAnchor({ children }: { children?: ReactNode; [key: string]: unknown }) { return <>{children}</>; } | |
| function PopoverContent({ children }: { children?: ReactNode; [key: string]: unknown }) { return <>{children}</>; } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| describe("INITIAL_TERMINAL_STATE", () => { | ||
| test("starts idle with no error or session", () => { | ||
| expect(INITIAL_TERMINAL_STATE.status).toBe("idle"); | ||
| expect(INITIAL_TERMINAL_STATE.errorMessage).toBeNull(); | ||
| expect(INITIAL_TERMINAL_STATE.reconnectAttempts).toBe(0); | ||
| expect(INITIAL_TERMINAL_STATE.sessionId).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
🔴 Terminal state tests use test() instead of test.todo() despite relying on an unimplemented no-op reducer
The terminalReducer in lib/terminal/use-terminal-state.ts:28 is a no-op stub (return state) that never transitions state. However, use-terminal-state.test.ts contains ~30 test() cases (not test.todo()) that assert on state transitions (e.g. expect(state.status).toBe("connecting") at line 38, expect(state.errorMessage).toBeNull() at line 25). All of these tests will fail because the reducer always returns the initial state unchanged. Additionally, the tests reference properties (errorMessage, reconnectAttempts) that don't exist on the TerminalState interface (which only has error, status, sessionId).
This violates the repository's AGENTS.md rule: "When adding tests that reproduce a bug or document expected behavior before the fix lands, use test.todo("description", () => {}) so mainline stays green. Never commit normally-failing test(...) cases — red CI blocks merges and erodes signal."
Prompt for agents
The terminal state tests in use-terminal-state.test.ts are written against the expected future implementation of terminalReducer, but the current stub in lib/terminal/use-terminal-state.ts is a no-op that returns state unchanged. Per AGENTS.md, tests that document expected behavior before the implementation lands must use test.todo() instead of test(). Convert all test() calls in use-terminal-state.test.ts to test.todo() so CI stays green. The same issue affects use-terminal-session.test.ts lines 115-122 and 125-132 which also use the no-op terminalReducer via applyEvents(). Additionally, the TerminalState interface needs errorMessage and reconnectAttempts fields (or the tests need to match the current interface shape with error instead of errorMessage).
Was this helpful? React with 👍 or 👎 to provide feedback.
| test("state transitions idle -> connecting -> connected", () => { | ||
| const state = applyEvents([ | ||
| { type: "CONNECT_REQUESTED" }, | ||
| { type: "CONNECT_SUCCEEDED", sessionId: "sess-test-1" }, | ||
| ]); | ||
| expect(state.status).toBe("connected"); | ||
| expect(state.sessionId).toBe("sess-test-1"); | ||
| expect(state.errorMessage).toBeNull(); | ||
| }); | ||
|
|
||
| test("state transitions to error when connect fails", () => { | ||
| const state = applyEvents([ | ||
| { type: "CONNECT_REQUESTED" }, | ||
| { type: "CONNECT_FAILED", message: "Failed to create terminal session" }, | ||
| ]); | ||
| expect(state.status).toBe("error"); | ||
| expect(state.errorMessage).toBe("Failed to create terminal session"); | ||
| expect(state.sessionId).toBeNull(); |
There was a problem hiding this comment.
🚩 Terminal session test types mismatch with stub event interfaces
In use-terminal-session.test.ts, the applyEvents helper passes events like { type: 'CONNECT_REQUESTED' } (no sessionId) and { type: 'CONNECT_SUCCEEDED', sessionId: 'sess-test-1' } and { type: 'CONNECT_FAILED', message: 'Failed...' }. However, the stub types in lib/terminal/use-terminal-state.ts define ConnectRequested as requiring sessionId, ConnectSucceeded as having no sessionId, and ConnectFailed as having error (not message). These type mismatches indicate the stubs don't match the expected platform API shape. When the real implementation is ported, the event interfaces in the stub file need to match the platform's actual types.
Was this helpful? React with 👍 or 👎 to provide feedback.
devin-ai-integration
Bot
requested review from
awlevin,
noanflaherty and
siddseethepalli
as code owners
…nents Move the remaining chat page UI from vellum-assistant-platform to vellum-assistant so chat-page.tsx renders a real chat interface. Moved directories: - _transcript/ → domains/chat/transcript/ - _hooks/ → domains/chat/hooks/ - _components/ → domains/chat/components/ - _context/chat-context.tsx → domains/chat/chat-context.tsx - _utils/ → domains/chat/utils/ Moved shared components: - MarkdownMessage, BusyIndicator, ToolCallChip, ToolCallProgressCard, MessageHoverActions, surfaces/, ChatComposer, ConnectingToAssistant, SubagentProgressCard, SubagentStatusBadge Convention updates applied: - Removed all 'use client' directives (Vite SPA) - kebab-case filenames throughout - .js extensions in all imports (NodeNext resolution) - next/navigation → react-router - @sentry/nextjs → @sentry/react - Design library imports via @vellum/design-library Stubs created for missing dependencies with TODO comments. chat-page.tsx wired up with real state machines, hooks, and context. Typecheck: 0 non-test errors Lint: 0 errors (26 warnings — unused eslint-disable directives) Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
…hitecture - Move domain logic from lib/ to domains/ (assistant, avatar, voice, terminal, nudges, trust-rules, settings) - Move chat-specific components from top-level components/ to domains/chat/components/ - Move nudge banners to domains/nudges/components/ - Move pre-existing lib/ misplacements: providers → components/, organization → domains/, routes → utils/, browser/native-file → runtime/ - Delete unnecessary adapters (app-link, app-routing) — use react-router directly - Delete design library duplicate stubs (input, toggle, dropdown, popover, select, switch, toast) - Delete UI Gallery files - Switch all design library imports to barrel import from @vellum/design-library - Update all import paths to match new file locations lib/ now contains only configured third-party wrappers (api-client, auth, feature-flags, telemetry). components/ now contains only cross-domain shared UI. domains/ owns all business domain logic and domain-specific components. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
…odo syntax Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
Port real implementations for all chat-domain dependencies: - domains/chat/: composer, attachments, banners, empty state, skeleton - domains/assistant/: API, lifecycle, disk pressure, model capabilities - domains/avatar/: bundled components, subagent avatar, svg compositor - domains/voice/: dictation API, audio amplitude, push-to-talk - domains/terminal/: API (non-admin endpoints), session management - domains/nudges/: discord, github, iOS, macOS nudge banners - domains/trust-rules/: trust rules API - domains/settings/: navigation, local settings - runtime/: native auth, notifications, native biometric - hooks/: option hotkeys, is-mobile detection - utils/: routes, misc, pointer, haptics - lib/: feature flags, onboarding, sync, API errors, interceptors Removed admin-only terminal endpoints (not in open-source API). Fixed pre-existing any-type issues in animated-avatar. Added composeSvg/composeSvgFromDefinitions to svg-compositor. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
…s and unused param - Remove eslint-disable for non-existent rules (react-hooks/set-state-in-effect, react-hooks/exhaustive-deps, @next/next/no-img-element, no-restricted-syntax) - Prefix unused param with underscore (_reason in disk-pressure.ts) - Auto-fix 28 unused eslint-disable directive warnings Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
PanelItem and MarqueeText now live in the design library (PR #31125). Delete the duplicated app-level copies and update 4 callsites to import from @vellum/design-library instead. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
devin-ai-integration
Bot
force-pushed
the
devin/1779172380-port-chat-page-ui
branch
from
aae85d7 to
ee4a4e6
Compare
The platform repo uses lib/vellum-api/client for the HeyAPI singleton. In this repo, the equivalent is lib/api-client.ts. Update all 11 side-effect imports to reference the correct path. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
![vex-assistant-bot[bot]](https://avatars.githubusercontent.com/in/3399303?s=60&v=4){kind=small}
There was a problem hiding this comment.
✦ APPROVE
Value: The most substantial port PR in this series — 270 files, real chat UI (transcript, SSE event stream, send-message pipeline, composers, surfaces, attachments, voice, subagent progress) moves from platform stubs to real ported implementations. With RootLayout + ChatLayout (#31114) and PanelItem (#31125) already merged, this is the layer that makes ChatPage render an actual chat interface. Unblocks all follow-up API wiring work.
Bots reviewed at b2fbfc81 — status at HEAD 9cab1e33:
- Codex P1:
assistant/api.tsstub → stuck on "Connecting" → by design, documented in PR as follow-up API wiring - Codex P1:
sendMessagestub → messages silently drop → by design, same - Codex P2:
app-routing.tsxdouble/assistantprefix → N/A, adapter replaced withutils/routes.tsin4c772189 - Devin 🔴:
popover.tsxstub returns null → ChatComposer invisible → fixed4c772189 - Devin 🔴: terminal tests use
test()on no-op reducer → fixedbc45f5fd(alltest.todo()now ✓) - Devin 🚩: duplicate
toast.ts/toast.tsx→ resolved4c772189 - Devin 🚩:
as unknown asrefs cast → still present, see below - Devin 🚩: terminal session test type mismatches → fixed
bc45f5fd(alltest.todo()now ✓) - Build failure
UNLOADABLE_DEPENDENCY: src/lib/vellum-api/client→ fixed9cab1e33✓
Real bug to fix before merge: double-prefix in routes.ts
Both utils/routes.ts and lib/routes.ts export route constants with the /assistant/... prefix (e.g. routes.settings.root = "/assistant/settings/general"). But routes.tsx configures the router with basename: "/assistant", which means React Router already strips /assistant from the URL. Any navigate(routes.settings.root) call will produce /assistant/assistant/settings/general — a 404 in the catch-all route.
The fix is to strip the /assistant prefix from all constants in both files (they should be "/settings/general" etc.), OR use a helper that normalizes them for the basename context.
This won't fire in this PR since navigate is stubbed as a no-op (const navigate = useCallback((_path: string) => {}, [])), but the wrong constants will cause broken navigation the moment that stub is wired up. Better to fix now before follow-up PRs inherit the bad values.
Same-content duplicate: utils/routes.ts and lib/routes.ts export byte-for-byte identical objects. One should import from the other, or one should be deleted.
chat-page.tsx refs cast — 24 of ~30 refs are now explicitly initialized (correct types, correct shapes). The remaining ~6 (streamEpochRef, processingSnapshotsRef, historyLoadedRef, reconcileAfterNextStreamOpenRef, turnStateRef) are genuinely missing — they'll be provided by hooks when the event-stream and reconciliation hooks are wired in. The as unknown as cast correctly acknowledges this is intentional scaffolding, not an oversight. Fine for staged port; the compiler will catch any shape mismatches when real refs are introduced.
use-event-stream.ts — This is the most complex file in the PR and it's done well: SSE lifecycle, burst-limiter for reconnect loops, visibility/Capacitor app-resume deduplication within 1s window, injected deps for testability. Worth a read if you want confidence the stream lifecycle is solid.
use-terminal-state.test.ts / use-terminal-session.test.ts — Both correctly use test.todo() with explanatory comments pointing at the stub files. Will promote to test when terminal domain is ported. ✓
Anti-patterns: No hex colors, no Tailwind color classes. --surface-hover used correctly for list rows. One note: chat-page.tsx uses text-[var(--text-secondary)] in the loading/error placeholder UI — confirm --text-secondary is a defined token in this repo; if not, --content-secondary is the correct equivalent.
CI at HEAD 9cab1e33: Socket ×2 ✓, Lint/Type Check & Build ✓. All passing.
Reviewed at HEAD 9cab1e33.
Vellum Constitution — Inviting: domain-first directory layout, clear TODO markers, and real hook implementations over stubs where possible make the follow-up wiring work easy to locate and reason about.
|
Good catches on the double-prefix in |
1 participant
Prompt / plan
Port the remaining chat page UI from
vellum-ai/vellum-assistant-platformtovellum-ai/vellum-assistantsoapps/web/src/domains/chat/chat-page.tsxrenders a real chat interface instead of its current placeholder.What's included (~225 files)
Chat domain directories moved:
_transcript/→domains/chat/transcript/— message rendering, scroll management, transcript rows_hooks/→domains/chat/hooks/— assistant lifecycle, conversation actions/history, event stream, send message, voice input, app nudges, pull-to-refresh_components/→domains/chat/components/— chat-route-content, setup/cleanup/onboarding screens, prompt cards (secret, confirmation, contact, question)_context/chat-context.tsx→domains/chat/chat-context.tsx— chat state/actions context providers_utils/→domains/chat/utils/— chat-utils, stream-handlers (message events, subagent events, tool calls, confirmations, etc.)Shared assistant components moved:
MarkdownMessage→components/markdown-message.tsxBusyIndicator→components/busy-indicator.tsxToolCallChip/→components/tool-call-chip/ToolCallProgressCard/→components/tool-call-progress-card/MessageHoverActions/→components/message-hover-actions/surfaces/→components/surfaces/(weather, map, browser, call summary, etc.)ChatComposer/→components/chat-composer/(emoji picker, slash commands, waveform, composer controller)ConnectingToAssistant/→components/connecting-to-assistant/SubagentProgressCard/→components/subagent-progress-card/SubagentStatusBadge→components/subagent-status-badge.tsxSupporting lib/adapter stubs:
lib/api/errors.ts,lib/assistants/(API, lifecycle, reachability, disk pressure)lib/voice/,lib/terminal/,lib/notifications/,lib/sync/lib/avatar/,lib/settings/,lib/trust-rules/adapters/app-link.tsx,adapters/app-routing.tsxConvention updates applied
'use client'directives (Vite SPA, not Next.js).jsextensions in all imports (NodeNext module resolution)next/navigation→react-router(useNavigate,useParams,useSearchParams)@sentry/nextjs→@sentry/react@/lib/chat/*→@/domains/chat/lib/*@/app/(app)/assistant/(chat)/_*→@/domains/chat/*@vellum/design-librarychat-page.tsx wiring
The chat page component now:
useAuth,useAssistantLifecycle,useIsMobileturnReducer,interactionReducerwithINITIAL_*constantsChatRouteRefsinterfaceChatProvidercontext properlyChatRouteContentwith all 70+ props type-safelyMissing dependency stubs
Components and hooks not yet ported have minimal stub files with
// TODO: port from platformcomments for easy discovery. These include: chat-empty-state, command-palette, various banners, avatar system, settings adapters, voice/dictation, push notifications, etc.What this PR does NOT include
Test plan
bun run typecheckinapps/web/— 0 non-test errors (182 test-only errors are preexisting testing infrastructure issues — missing@testing-library/jest-dommatcher types)bun run lintinapps/web/— 0 errors, 26 warnings (all are unusedeslint-disabledirectives forno-restricted-syntax— harmless)Link to Devin session: https://app.devin.ai/sessions/5c400920d2b745bc84401cd020820f22
Requested by: @ashleeradka