improve coding agent#19
Conversation
…w validation File Attachments for Workflow Assistant: - Add file upload UI with drag-and-drop, paste, and file picker support - Support images (inline preview), PDFs, Word docs, and text files - Create attachments library for building multi-modal AI content - Process images inline for AI; extract text from other file types - Register files with agent component for proper tracking Workflow Validation Refactoring: - Move validate_workflow_steps.ts from engine/ to validation/ directory - Rename variable_references/ to variables/ with cleaner file names - Add step-specific validation modules (action, condition, llm, loop, trigger) - Create shared constants.ts for StepType definition - Export isValidStepType helper from validate_step_config JEXL Expression Validation Improvements: - Add parseJexlExpression for raw JEXL syntax (condition expressions) - Validate |length filter is applied to arrays, not objects - Detect .length property usage and suggest |length transform - Extract full field paths from complex expressions for validation API Consistency Updates: - Rename paginated result field from 'page' to 'items' across: - customers.ts, products.ts, documents.ts, conversations.ts - Query helpers and action output schemas - Agent tool helpers and UI components
- Process image and document attachments separately - Parse documents (PDFs, etc.) to extract text content for AI context - Send images as multi-modal content directly to AI - Include parsed document text in AI prompt while showing markdown links in UI - Truncate very long documents (>50KB) to avoid context limits - Save clean user messages for display without workflow context pollution
📝 WalkthroughWalkthroughThis PR introduces multi-modal file attachment support across chat and automation assistant components, centralizes and reorganizes the workflow validation infrastructure into dedicated modules with step-type validators and constants, renames pagination result fields from Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Comment |
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
services/platform/app/(app)/dashboard/[id]/chat/components/chat-input.tsx (2)
267-269: Use semantic colors instead of hardcoded blue colors.As per coding guidelines, hardcoded colors should be replaced with design system semantic colors.
- <div className="size-full bg-gradient-to-br from-blue-100 to-blue-200 flex items-center justify-center"> - <span className="text-xs text-blue-600">IMG</span> + <div className="size-full bg-muted flex items-center justify-center"> + <span className="text-xs text-muted-foreground">IMG</span> </div>
361-365: Use CSS variable or semantic color instead of hardcoded SVG fill.The hardcoded fill color
#9CA3AFshould use a semantic color. Consider usingcurrentColorwith a text utility class on the parent, or replace with an icon component that inherits color.- <path - d="M11.3331 0.666687V6.66669C11.3331 7.37393 11.052..." - fill="#9CA3AF" - /> + <path + d="M11.3331 0.666687V6.66669C11.3331 7.37393 11.052..." + fill="currentColor" + />services/platform/app/(app)/dashboard/[id]/chat/components/message-bubble.tsx (4)
35-49: Type duplication withlayout.tsx.The
FileAttachmentinterface is defined both here and inservices/platform/app/(app)/dashboard/[id]/chat/layout.tsx(Lines 12-18). Consider importing from the layout to maintain a single source of truth.+import type { FileAttachment } from '../layout'; + -interface FileAttachment { - fileId: Id<'_storage'>; - fileName: string; - fileType: string; - fileSize: number; - previewUrl?: string; // Local preview URL for optimistic display -}
246-275: Use semantic colors instead of hardcoded color classes.Per coding guidelines, avoid hardcoded colors like
bg-gray-100,bg-blue-100,bg-red-100. Use design system semantic colors instead.const getFileTypeInfo = (type: string, name: string) => { if (type.startsWith('image/')) - return { icon: '🖼️', label: 'IMG', bgColor: 'bg-blue-100' }; + return { icon: '🖼️', label: 'IMG', bgColor: 'bg-muted' }; if (type === 'application/pdf') - return { icon: '📄', label: 'PDF', bgColor: 'bg-red-100' }; + return { icon: '📄', label: 'PDF', bgColor: 'bg-muted' }; if ( type.includes('word') || name.endsWith('.doc') || name.endsWith('.docx') ) - return { icon: '📝', label: 'DOC', bgColor: 'bg-blue-100' }; + return { icon: '📝', label: 'DOC', bgColor: 'bg-muted' }; if (type === 'text/plain') - return { icon: '📄', label: 'TXT', bgColor: 'bg-gray-100' }; - return { icon: '📎', label: 'FILE', bgColor: 'bg-gray-100' }; + return { icon: '📄', label: 'TXT', bgColor: 'bg-muted' }; + return { icon: '📎', label: 'FILE', bgColor: 'bg-muted' }; };Similarly, update other hardcoded colors throughout the component (e.g.,
bg-gray-200→bg-muted,text-gray-800→text-foreground,text-gray-500→text-muted-foreground).
292-298: Hardcoded colors in image thumbnail container.- <div className="size-11 rounded-lg bg-gray-200 bg-center bg-cover bg-no-repeat overflow-hidden"> + <div className="size-11 rounded-lg bg-muted bg-center bg-cover bg-no-repeat overflow-hidden">
303-329: Hardcoded colors in file attachment card.- className="bg-gray-100 rounded-lg px-2 py-1.5 flex items-center gap-2 hover:bg-gray-200 transition-colors max-w-[216px]" + className="bg-muted rounded-lg px-2 py-1.5 flex items-center gap-2 hover:bg-muted/80 transition-colors max-w-[13.5rem]" > ... - <div className="text-sm font-medium text-gray-800 truncate"> + <div className="text-sm font-medium text-foreground truncate"> {attachment.fileName} </div> - <div className="text-xs text-gray-500"> + <div className="text-xs text-muted-foreground">services/platform/convex/workflow/actions/customer/customer_action.ts (1)
100-103: Consider aligning the TypeScript type with the validator for better IDE support.The
updatesfield is typed asRecord<string, unknown>, but the runtime validator at line 140 usescustomerUpdateValidatorwith specific fields. While the learnings indicate thatRecord<string, unknown>is acceptable when validation is at the mutation level, having a more specific type would provide better IDE support and compile-time checks.Consider updating the type to match the validator structure:
| { operation: 'update'; customerId: string; - updates: Record<string, unknown>; + updates: { + name?: string; + email?: string; + status?: 'active' | 'churned' | 'potential'; + source?: string; + locale?: string; + address?: CustomerAddress; + metadata?: Record<string, unknown>; + }; };Based on learnings, maintaining separate TypeScript types alongside validators provides IDE support and compile-time checking, while the validator handles runtime validation.
1abcf68 to
990b521
Compare
- web/use-document-meta + legal-page: thread frontmatter.noindex through to the meta hook so legal pages actually emit `<meta name=robots content=noindex,nofollow>`. Pre-fix, every legal doc declared noindex: true in YAML but the wrapper hook ignored it and every legal URL shipped indexable. Round-2 review CRITICAL #26 / F.1. - chat/legal-hold-indicator: replace banned toLocaleDateString() call with useFormatDate(). AGENTS.md prohibits toLocale*; pre-fix, the placedAt date ignored the configured locale + timezone. Round-2 review CRITICAL #23 / F.2. - legal-hold/place-hold-dialog: defense-in-depth `if (!orgConfirmed) return;` at the top of onSubmit so a determined user (devtools removeAttribute, programmatic submit) cannot bypass the org-wide hold typed-confirmation gate. Round-2 review CRITICAL #19 / F.3. - chat/chat-history-sidebar: archived list now uses isThreadHeld() like the active list — pre-fix, the archived section's heldThreadIds.has() ignored orgWideHeld so org-wide holds rendered no lock indicator on archived threads. - chat/chat-actions: unarchive button tooltip now swaps to legalHold.badges.blockedByHold when held, mirroring archive/delete. Pre-fix, a held thread's disabled unarchive button showed only the generic "Unarchive" tooltip with no explanation. - governance/hooks/mutations: useRestoreSoftDeletedRow's invalidation predicate now reads queryKey[1] (the convexQuery function name) instead of queryKey[0] (always the literal 'convexQuery'). Pre-fix, the predicate never matched and post-restore cache was stale on multi-page trash views. Lint and typecheck clean.
- web/use-document-meta + legal-page: thread frontmatter.noindex through to the meta hook so legal pages actually emit `<meta name=robots content=noindex,nofollow>`. Pre-fix, every legal doc declared noindex: true in YAML but the wrapper hook ignored it and every legal URL shipped indexable. Round-2 review CRITICAL #26 / F.1. - chat/legal-hold-indicator: replace banned toLocaleDateString() call with useFormatDate(). AGENTS.md prohibits toLocale*; pre-fix, the placedAt date ignored the configured locale + timezone. Round-2 review CRITICAL #23 / F.2. - legal-hold/place-hold-dialog: defense-in-depth `if (!orgConfirmed) return;` at the top of onSubmit so a determined user (devtools removeAttribute, programmatic submit) cannot bypass the org-wide hold typed-confirmation gate. Round-2 review CRITICAL #19 / F.3. - chat/chat-history-sidebar: archived list now uses isThreadHeld() like the active list — pre-fix, the archived section's heldThreadIds.has() ignored orgWideHeld so org-wide holds rendered no lock indicator on archived threads. - chat/chat-actions: unarchive button tooltip now swaps to legalHold.badges.blockedByHold when held, mirroring archive/delete. Pre-fix, a held thread's disabled unarchive button showed only the generic "Unarchive" tooltip with no explanation. - governance/hooks/mutations: useRestoreSoftDeletedRow's invalidation predicate now reads queryKey[1] (the convexQuery function name) instead of queryKey[0] (always the literal 'convexQuery'). Pre-fix, the predicate never matched and post-restore cache was stale on multi-page trash views. Lint and typecheck clean.
Closes #3, #19, #20, #21, #22, #23, #24, #25, #26, #29, #39 — frontend audio UX + resolver tests. - `message-bubble.tsx` renders a single stable `<VoiceOutputIndicator>` per assistant message instead of three separate mounts (inline- streaming + two toolbar copies). The previous shape unmounted the inline indicator at streaming-end → triggered `stop()` → mounted a fresh toolbar indicator with a `mountTimeRef` captured AFTER all chunks were created → auto-play short-circuited and the user heard silence at the stream-end boundary. The single mount keeps `mountTimeRef` stable across both phases. (#3) - `use-voice-output.ts` tracks every retry `setTimeout` id in a `Set` ref and clears them on unmount + on message change. The prior code let the 1.5s backoff timer fire after unmount and re-invoke `synthesize` against a dead component. (#19) - `use-voice-output.ts` caps the synthesis queue at `MAX_TTS_QUEUE_DEPTH = 50`. When full, drops the new task and surfaces `QUEUE_OVERFLOW` via the error sink so the user sees why playback paused. `MAX_IN_FLIGHT` previously throttled concurrent dispatch but did not bound queue depth. (#20) - `use-voice-output.ts` catch branch now falls back to `'UNKNOWN_NETWORK'` when `extractConvexErrorCode` returns undefined (network drop, action timeout). Previously the only signal was `console.error`; the indicator stayed stuck with no actionable message. (#21) - `use-voice-output-player.ts` re-calls `primeAudio(el)` at the start of every `play()` invocation and drops the `el.load()` in `stop()`. Together these stop iOS Safari from expiring the user-activation token between messages of a session. (#22) - `voice-output-context.tsx` + `prime-audio.ts`: per-provider audio element ownership. Each `<VoiceOutputProvider>` constructs its own `<audio>` via `useMemo` and exposes it via `useVoiceAudioElement()`. The prior module-level singleton meant arena split-view's two providers stomped each other's `src` mid-playback. `primeAudio(el?)` now takes the element to pre-warm; callers without a provider scope (settings page) call it with `undefined` and only the AudioContext is banked. (#23) - `voice-output-indicator.tsx` classifies error codes into `retryable | config | terminal`. Config codes (NO_PROVIDER, HOST_POLICY, forbidden) render a `<Link>` to Settings → AI providers; terminal codes (BUDGET_EXCEEDED, QUEUE_OVERFLOW, char- cap) render a non-interactive `<Badge>`. Only retryable codes keep the click-to-retry button. Stops the tap→fail→tap→fail loop on unrecoverable errors. (#24) - `voice-output-announcer.tsx` now reads `{ state, errorCode }` from the announcer store and speaks the per-code reason on transitions into `'error'` (e.g. "Voice provider not configured"). Screen- reader users on touch devices — where the indicator's per-code tooltip is unreachable — now hear the actionable reason instead of the generic "Voice output failed". (#25) - `personalization-settings.tsx` composes the `providerUnavailable` hint into the Switch's `description` prop (a ReactNode) when `providerAvailable === false`. The hint now lands in the same `aria-describedby` block as the base description, so SR focus on the Switch reads it. The duplicate sibling `<Text>` is removed. (#26) - `voice-output-announcer.tsx` drains announcements through a small queue with a 1500ms hold per entry. Rapid transitions (playing → blocked → error in <1500ms) no longer clobber the previous text mid-utterance; each entry plays in order. (#39) - `resolve_tts_model.test.ts` adds the missing call-contract assertions (tag=text-to-speech, orgSlug propagation, providerName propagation on a pinned-provider call) and three failure-path tests that pin the resolver's re-throw behaviour for UNKNOWN_MODEL, UNKNOWN_PROVIDER, and plain rejections. Without these, a regression that hard-coded `tag: 'chat'` or dropped `orgSlug` would have passed every prior test silently. (#29) - i18n: `voiceOutputErrorConfig`, `voiceOutputErrorOpenSettings`, `voiceOutputErrorQueueOverflow`, `voiceOutputErrorNetwork` added to en/de/fr. The pre-existing orphan `voiceOutputErrorProvider` is removed (superseded by `voiceOutputErrorConfig`).
Closes #3, #19, #20, #21, #22, #23, #24, #25, #26, #29, #39 — frontend audio UX + resolver tests. - `message-bubble.tsx` renders a single stable `<VoiceOutputIndicator>` per assistant message instead of three separate mounts (inline- streaming + two toolbar copies). The previous shape unmounted the inline indicator at streaming-end → triggered `stop()` → mounted a fresh toolbar indicator with a `mountTimeRef` captured AFTER all chunks were created → auto-play short-circuited and the user heard silence at the stream-end boundary. The single mount keeps `mountTimeRef` stable across both phases. (#3) - `use-voice-output.ts` tracks every retry `setTimeout` id in a `Set` ref and clears them on unmount + on message change. The prior code let the 1.5s backoff timer fire after unmount and re-invoke `synthesize` against a dead component. (#19) - `use-voice-output.ts` caps the synthesis queue at `MAX_TTS_QUEUE_DEPTH = 50`. When full, drops the new task and surfaces `QUEUE_OVERFLOW` via the error sink so the user sees why playback paused. `MAX_IN_FLIGHT` previously throttled concurrent dispatch but did not bound queue depth. (#20) - `use-voice-output.ts` catch branch now falls back to `'UNKNOWN_NETWORK'` when `extractConvexErrorCode` returns undefined (network drop, action timeout). Previously the only signal was `console.error`; the indicator stayed stuck with no actionable message. (#21) - `use-voice-output-player.ts` re-calls `primeAudio(el)` at the start of every `play()` invocation and drops the `el.load()` in `stop()`. Together these stop iOS Safari from expiring the user-activation token between messages of a session. (#22) - `voice-output-context.tsx` + `prime-audio.ts`: per-provider audio element ownership. Each `<VoiceOutputProvider>` constructs its own `<audio>` via `useMemo` and exposes it via `useVoiceAudioElement()`. The prior module-level singleton meant arena split-view's two providers stomped each other's `src` mid-playback. `primeAudio(el?)` now takes the element to pre-warm; callers without a provider scope (settings page) call it with `undefined` and only the AudioContext is banked. (#23) - `voice-output-indicator.tsx` classifies error codes into `retryable | config | terminal`. Config codes (NO_PROVIDER, HOST_POLICY, forbidden) render a `<Link>` to Settings → AI providers; terminal codes (BUDGET_EXCEEDED, QUEUE_OVERFLOW, char- cap) render a non-interactive `<Badge>`. Only retryable codes keep the click-to-retry button. Stops the tap→fail→tap→fail loop on unrecoverable errors. (#24) - `voice-output-announcer.tsx` now reads `{ state, errorCode }` from the announcer store and speaks the per-code reason on transitions into `'error'` (e.g. "Voice provider not configured"). Screen- reader users on touch devices — where the indicator's per-code tooltip is unreachable — now hear the actionable reason instead of the generic "Voice output failed". (#25) - `personalization-settings.tsx` composes the `providerUnavailable` hint into the Switch's `description` prop (a ReactNode) when `providerAvailable === false`. The hint now lands in the same `aria-describedby` block as the base description, so SR focus on the Switch reads it. The duplicate sibling `<Text>` is removed. (#26) - `voice-output-announcer.tsx` drains announcements through a small queue with a 1500ms hold per entry. Rapid transitions (playing → blocked → error in <1500ms) no longer clobber the previous text mid-utterance; each entry plays in order. (#39) - `resolve_tts_model.test.ts` adds the missing call-contract assertions (tag=text-to-speech, orgSlug propagation, providerName propagation on a pinned-provider call) and three failure-path tests that pin the resolver's re-throw behaviour for UNKNOWN_MODEL, UNKNOWN_PROVIDER, and plain rejections. Without these, a regression that hard-coded `tag: 'chat'` or dropped `orgSlug` would have passed every prior test silently. (#29) - i18n: `voiceOutputErrorConfig`, `voiceOutputErrorOpenSettings`, `voiceOutputErrorQueueOverflow`, `voiceOutputErrorNetwork` added to en/de/fr. The pre-existing orphan `voiceOutputErrorProvider` is removed (superseded by `voiceOutputErrorConfig`).
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.