Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the F0 AI chat and analytics dashboard integrations to be compatible with CopilotKit v1.51+ (AG-UI), including new message preprocessing (placeholder filtering + multi-tool-call expansion), new canvas auto-open semantics tied to toolCall IDs, and a new “dataDownload” canvas entity.
Changes:
- Add message preprocessing to handle CopilotKit v1.51+ shapes (coagent placeholders + multi-tool-call messages) and adjust thinking/turn grouping accordingly.
- Introduce toolCallId propagation via
AssistantMessagecontext to restore “active card” detection + auto-open behavior after CopilotKit stopped passing toolCallId in render props. - Add a new Data Download canvas entity (card + canvas panel content + client-side CSV/XLSX download), plus significant dashboard canvas/grid UX changes (transform chart type, discard/save action bar, new DnD grid).
Reviewed changes
Copilot reviewed 56 out of 57 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/sds/ai/F0AiChat/utils/turnUtils.ts | Update thinking detection and guard for stale assistant-first message arrays. |
| packages/react/src/sds/ai/F0AiChat/utils/messageExpansion.ts | Filter CopilotKit placeholders and expand multi-tool-call messages. |
| packages/react/src/sds/ai/F0AiChat/utils/constants.ts | Add orchestrator tool name constant + small-screen breakpoint. |
| packages/react/src/sds/ai/F0AiChat/types.ts | Extend canvas content union with dataDownload. |
| packages/react/src/sds/ai/F0AiChat/providers/ToolRendererProvider.tsx | Add a context bridge for tool-call UI rendering. |
| packages/react/src/sds/ai/F0AiChat/internal-types.ts | Add detection/filtering for coagent-state-render placeholders. |
| packages/react/src/sds/ai/F0AiChat/hooks/useAutoOpenCanvas.ts | Switch auto-open from “per mount” to “once per toolCallId (global)”. |
| packages/react/src/sds/ai/F0AiChat/F0AiFullscreenChat.tsx | Render CanvasPanel in fullscreen mode when in canvas visualization mode. |
| packages/react/src/sds/ai/F0AiChat/F0AiChat.tsx | Wrap chat in the new CopilotToolRendererProvider. |
| packages/react/src/sds/ai/F0AiChat/components/messages/WelcomeScreen.tsx | Make welcome rendering more robust for non-string content and className composition. |
| packages/react/src/sds/ai/F0AiChat/components/messages/Thinking.tsx | Render thinking titles from tool call args (AG-UI shape). |
| packages/react/src/sds/ai/F0AiChat/components/messages/MessagesContainer.tsx | Preprocess messages before turn pipeline; add “waiting for first assistant response” indicator. |
| packages/react/src/sds/ai/F0AiChat/components/messages/AssistantMessage.tsx | Inject toolCallId via context; adjust tool-call UI rendering path. |
| packages/react/src/sds/ai/F0AiChat/components/messages/tests/MessagesContainer.test.ts | Update/extend tests for new thinking shape + assistant-first guard cases. |
| packages/react/src/sds/ai/F0AiChat/components/messages/stories/Thinking.stories.tsx | Update stories to use orchestratorThinking toolCalls instead of generativeUI. |
| packages/react/src/sds/ai/F0AiChat/components/messages/stories/AssistantMessage.stories.tsx | Simplify story decorator wrapper. |
| packages/react/src/sds/ai/F0AiChat/components/layout/ChatHeader.tsx | Ignore coagent placeholders when determining “has messages”. |
| packages/react/src/sds/ai/F0AiChat/components/layout/CanvasPanel.tsx | Adjust canvas container styling for mobile vs md+. |
| packages/react/src/sds/ai/F0AiChat/components/input/ChatInput.tsx | Ignore coagent placeholders when determining welcome screen state. |
| packages/react/src/sds/ai/F0AiChat/canvas/registry.ts | Register new dataDownload canvas entity. |
| packages/react/src/sds/ai/F0AiChat/canvas/entities/dataDownload/index.tsx | Define entity wiring (header/content/wrapper) and exports. |
| packages/react/src/sds/ai/F0AiChat/canvas/entities/dataDownload/DataDownloadHeader.tsx | Add canvas header with download dropdown + close button. |
| packages/react/src/sds/ai/F0AiChat/canvas/entities/dataDownload/DataDownloadContext.tsx | Track view state and derive “filtered dataset” for export. |
| packages/react/src/sds/ai/F0AiChat/canvas/entities/dataDownload/DataDownloadContent.tsx | Render dataset via OneDataCollection with paging/search/sort. |
| packages/react/src/sds/ai/F0AiChat/canvas/entities/dataDownload/DataDownloadCard.tsx | Add inline chat card that opens canvas + supports auto-open by toolCallId. |
| packages/react/src/sds/ai/F0AiChat/canvas/entities/dashboard/DashboardHeader.tsx | Replace edit-mode header actions with export button-only header. |
| packages/react/src/sds/ai/F0AiChat/canvas/entities/dashboard/DashboardContext.tsx | Track dirty state, pending layout/transforms, discardKey, and save/discard logic. |
| packages/react/src/sds/ai/F0AiChat/canvas/entities/dashboard/DashboardContent.tsx | Apply transforms without refetch; add action bar; wire chart transforms + discard resetKey. |
| packages/react/src/sds/ai/F0AiChat/canvas/entities/dashboard/DashboardCard.tsx | Move toolCallId + auto-open handling into the card via context/hook. |
| packages/react/src/sds/ai/F0AiChat/canvas/entities/dashboard/tests/DashboardHeader.test.tsx | Update tests to match removed edit controls and new context contract. |
| packages/react/src/sds/ai/F0AiChat/canvas/entities/dashboard/tests/DashboardCard.test.tsx | Update tests for new openCanvas wiring + toolCallId context usage. |
| packages/react/src/sds/ai/F0AiChat/canvas/components/CanvasCard.tsx | Allow children content and adjust layout structure. |
| packages/react/src/sds/ai/F0AiChat/canvas/CANVAS_ENTITIES.md | Document toolCallId context + auto-open hook patterns. |
| packages/react/src/sds/ai/F0AiChat/canvas/AutoOpenCanvas.tsx | Remove legacy auto-open wrapper component. |
| packages/react/src/sds/ai/F0AiChat/ARCHITECTURE.md | Update architecture doc to remove AutoOpenCanvas entry. |
| packages/react/src/sds/ai/F0AiChat/actions/core/orchestratorThinking/useOrchestratorThinkingAction.tsx | Map CopilotKit v1.51+ status values to match spinner behavior. |
| packages/react/src/sds/ai/F0AiChat/actions/core/displayDashboard/useDisplayDashboardAction.tsx | Render dashboard card only; remove AutoOpenCanvas usage and toolCallId prop passing. |
| packages/react/src/sds/ai/F0AiChat/actions/core/dataDownload/useDataDownloadAction.tsx | Switch to new DataDownloadCard and add streaming guard. |
| packages/react/src/sds/ai/F0AiChat/actions/core/dataDownload/types.ts | Add optional title for data download action props. |
| packages/react/src/sds/ai/F0AiChat/actions/core/dataDownload/download.ts | Extract CSV/XLSX download logic into standalone functions. |
| packages/react/src/sds/ai/F0AiChat/actions/core/dataDownload/DataDownload.tsx | Remove old inline DataDownload component. |
| packages/react/src/sds/ai/F0AiChat/actions/core/dataDownload/stories/DataDownload.stories.tsx | Remove old DataDownload stories. |
| packages/react/src/sds/ai/F0AiChat/actions/COPILOT_ACTIONS.md | Update docs to reflect new “card handles toolCallId + auto-open” approach. |
| packages/react/src/lib/providers/i18n/i18n-provider-defaults.ts | Add data download-related translation keys. |
| packages/react/src/experimental/RichText/RichTextDisplay/index.tsx | Allow task-list checkbox inputs through DOMPurify for GFM task lists. |
| packages/react/src/experimental/RichText/index.css | Add styling for GFM task list checkbox structure. |
| packages/react/src/experimental/F0ActionBar/index.tsx | Minor import reorder and spacing tweaks. |
| packages/react/src/examples/ApplicationFrame/index.tsx | Adjust z-index and import ordering for canvas overlay behavior. |
| packages/react/src/components/F0AnalyticsDashboard/types.ts | Add resetKey and onTransformChart to dashboard props. |
| packages/react/src/components/F0AnalyticsDashboard/F0AnalyticsDashboard.tsx | Wire resetKey/transform callback into grid and update spacing/padding. |
| packages/react/src/components/F0AnalyticsDashboard/components/MetricItem/MetricItem.tsx | Thread edit/delete props through to DashboardItem wrapper. |
| packages/react/src/components/F0AnalyticsDashboard/components/DashboardItem/DashboardItem.tsx | Replace legacy dropdown with new menu supporting chart type + downloads + delete. |
| packages/react/src/components/F0AnalyticsDashboard/components/DashboardGrid/DashboardGrid.tsx | Replace GridStack grid with custom flex-row DnD + row resize + resetKey support. |
| packages/react/src/components/F0AnalyticsDashboard/components/CollectionItem/CollectionItem.tsx | Thread edit/delete props through to DashboardItem wrapper. |
| packages/react/src/components/F0AnalyticsDashboard/components/ChartItem/ChartItem.tsx | Add chart type transform menu options and thread edit/delete props. |
| packages/react/src/components/F0AnalyticsDashboard/stories/index.stories.tsx | Enable editMode in story for updated dashboard behavior. |
| const expanded: Message[] = toolCalls.map((tc, i) => ({ | ||
| id: `${msg.id}_tc${i}`, | ||
| role: msg.role as "assistant", | ||
| content: "", | ||
| toolCalls: [tc], | ||
| })) | ||
|
|
||
| if (msg.content) { | ||
| expanded.push({ | ||
| id: `${msg.id}_text`, | ||
| role: msg.role as "assistant", | ||
| content: msg.content, | ||
| }) |
There was a problem hiding this comment.
expandToolCalls builds new Message objects without preserving the original message fields (e.g. agentName, name, parentMessageId, metadata). This can break downstream logic that relies on non-core Message properties. Prefer cloning the original message (spread) and only overriding id, content, and toolCalls for each expanded item (and ensure the text-only message preserves the original metadata too).
| * `generativeUI()` method available on message objects. | ||
| */ | ||
| const defaultRenderer: ResolveToolCallUI = (message) => | ||
| (message as any)?.generativeUI?.() ?? null | ||
|
|
There was a problem hiding this comment.
This introduces (message as any) to access generativeUI, which violates the repo's TS strict rule (no any / as any; see packages/react/AGENTS.md and packages/react/.skills/f0-code-review/SKILL.md). Define a typed interface/type-guard for messages that optionally expose generativeUI and avoid any entirely.
| export const AssistantMessage = ({ | ||
| isGenerating, | ||
| isLoading, | ||
| markdownTagRenderers, | ||
| message, | ||
| }: AssistantMessageProps) => { | ||
| messages, | ||
| }: AssistantMessageProps & { messages?: any[] }) => { | ||
| const content = message?.content || "" | ||
|
|
||
| // Resolve tool-call UI via the ToolRendererProvider. | ||
| // After message expansion in MessagesContainer, each message has at | ||
| // most one tool call, so toolCalls[0] is correct. | ||
| const resolveToolCallUI = useToolRenderer() | ||
| const isThinkingTool = | ||
| message?.role === "assistant" && | ||
| message.toolCalls?.find( | ||
| (tool) => tool.function.name === "orchestratorThinking" | ||
| ) | ||
| const subComponent = message?.generativeUI?.( | ||
| isThinkingTool | ||
| ? { | ||
| status: isLoading ? "executing" : "completed", | ||
| } | ||
| : undefined | ||
| ) | ||
| // For thinking tools, pass status so the render callback shows the | ||
| // correct spinner/completed state (v1.10.6 generativeUI receives | ||
| // these as render props). | ||
| const subComponent = message | ||
| ? ((message as any)?.generativeUI?.( | ||
| isThinkingTool | ||
| ? { status: isLoading ? "executing" : "completed" } | ||
| : undefined | ||
| ) ?? resolveToolCallUI(message, messages ?? [])) |
There was a problem hiding this comment.
AssistantMessage adds messages?: any[] and uses (message as any)?.generativeUI, which conflicts with the repo's strict TypeScript guidelines (no any / as any). Please type messages as Message[] (or the proper CopilotKit type) and replace the any cast with a narrow type guard/interface for generativeUI.
| const isThinkingTool = | ||
| message?.role === "assistant" && | ||
| message.toolCalls?.find( | ||
| (tool) => tool.function.name === "orchestratorThinking" | ||
| ) |
There was a problem hiding this comment.
isThinkingTool still compares against the hardcoded string "orchestratorThinking" even though ORCHESTRATOR_THINKING_ACTION was introduced. Use the shared constant here to avoid future drift and keep thinking detection consistent across the chat pipeline.
| const dataAdapter = useMemo( | ||
| () => ({ | ||
| paginationType: "pages" as const, | ||
| perPage: PER_PAGE, | ||
| fetchData: (options: any) => { | ||
| let filtered = [...rows] as RecordType[] | ||
|
|
There was a problem hiding this comment.
fetchData: (options: any) introduces any in production code. Please type options using the existing datasource fetch options types (e.g. the paginated fetch options used by OneDataCollection/data adapters) so search/sort/pagination fields are checked at compile time.
| DOMPurify.sanitize( | ||
| format === "markdown" | ||
| ? String(PROCESSOR.processSync(content)) | ||
| : content, | ||
| { | ||
| ADD_ATTR: ["target"], | ||
| ALLOWED_ATTR: ["href", "target", "rel", "class"], | ||
| ADD_TAGS: ["input"], | ||
| ALLOWED_ATTR: [ | ||
| "href", | ||
| "target", | ||
| "rel", | ||
| "class", | ||
| "type", | ||
| "checked", | ||
| "disabled", | ||
| ], | ||
| } |
There was a problem hiding this comment.
DOMPurify is now configured to allow <input> tags with type/checked/disabled, but this doesn’t restrict inputs to non-interactive checkbox task items. As-is, markdown/HTML could render other input types (text, submit, etc.) inside sanitized content. Consider restricting allowed inputs to type="checkbox" only and forcing them to disabled via a DOMPurify hook (or an equivalent allowlist approach) to avoid UI injection/phishing risks.
| <F0ActionBar | ||
| label="Changes detected" | ||
| isOpen={isDirty} | ||
| primaryActions={[ | ||
| { | ||
| label: translations.ai.saveChanges, |
There was a problem hiding this comment.
F0ActionBar label is hardcoded ("Changes detected"). Per repo i18n conventions, user-facing strings should come from useI18n()/translations so they can be localized.
| <div className="flex w-full flex-row items-center gap-2 pr-2"> | ||
| <F0Icon icon={ChartLine} /> | ||
| <span className="flex-1 text-base font-medium"> | ||
| Chart type |
There was a problem hiding this comment.
The dropdown submenu label "Chart type" is hardcoded. Please source it from translations (useI18n()) to avoid introducing new non-localizable UI strings.
| Chart type | |
| {translations.ai.chartType} |
| {canDrag && ( | ||
| <div | ||
| className="absolute -left-2 -top-2 z-20" | ||
| role="presentation" | ||
| onPointerDown={(e) => e.stopPropagation()} | ||
| onMouseDown={(e) => e.stopPropagation()} | ||
| className="shadow-sm absolute -left-3 top-2.5 z-20 flex cursor-grab items-center justify-center rounded bg-f1-background p-2 opacity-0 transition-opacity hover:bg-f1-background-hover active:cursor-grabbing group-hover/rowitem:opacity-100" | ||
| onMouseDown={() => { | ||
| fromHandleRef.current = true | ||
| }} | ||
| onMouseUp={() => { | ||
| fromHandleRef.current = false | ||
| }} | ||
| aria-label="Drag to reorder" | ||
| > | ||
| <F0Button | ||
| variant="outline" | ||
| hideLabel | ||
| icon={Minus} | ||
| label={t("actions.delete")} | ||
| onClick={() => handleDelete(item.id)} | ||
| size="sm" | ||
| /> | ||
| <F0Icon icon={Handle} size="xs" /> | ||
| </div> |
There was a problem hiding this comment.
The drag handle uses a non-focusable <div> with mouse handlers and an aria-label. This is not keyboard accessible and doesn’t expose a semantic control to assistive tech. Prefer using a <button> (or add role="button", tabIndex={0}, and key handlers for Enter/Space) so reordering can be initiated/accessed without a mouse.
| function parseThinkingTitle(argsJson: string): string { | ||
| try { | ||
| const parsed = JSON.parse(argsJson) | ||
| return (parsed.message as string) || "thinking" | ||
| } catch { | ||
| return "thinking" | ||
| } |
There was a problem hiding this comment.
parseThinkingTitle falls back to the hardcoded string "thinking". Since this text can be user-visible (rendered as the action item title), it should use i18n (e.g. translations.ai.thinking or a dedicated translation key) rather than an English literal.
CopilotKit v1.51+ switched to an AG-UI message model which required several adaptations to keep F0AiChat working correctly. Key changes: - **Message expansion**: AG-UI packs multiple tool calls into a single message. MessagesContainer now expands these into individual messages so the turn/thinking pipeline works correctly. - **Tool call rendering**: CopilotKit v1.51+ stores render callbacks for `available: "frontend"` actions in an internal `renderToolCalls` registry (not in `context.actions`). AssistantMessage now uses `useLazyToolRenderer` to look up and render tool calls correctly. - **ToolCallId context**: CopilotKit v1.51+ no longer passes `toolCallId` in render callback props. AssistantMessage provides it via a React context (`ToolCallIdContext`) that canvas cards read with `useToolCallId()`. - **Coagent placeholders**: Filter out empty `coagent-state-render` messages injected by CopilotKit that break welcome screen and message count logic. - **Thinking messages**: Content moved from `message.content` to `toolCalls[].function.arguments`. Thinking component now parses titles from tool call args directly instead of calling `generativeUI()`. - **Action status mapping**: Map `"inProgress"` to `"executing"` for frontend-only actions (v1.51+ changed the status name). - **Canvas auto-open**: Replaced `AutoOpenCanvas` wrapper component with `useAutoOpenCanvas` hook called inside each card. Uses a module-level Set to survive remounts. Each card manages its own auto-open. - **Guard for non-user first message**: `convertMessagesToTurns` no longer crashes when the first message is not from the user.
…ut and functionality
…ring and message processing - Added CopilotToolRendererProvider to manage tool call rendering context. - Refactored AssistantMessage to utilize the new useToolRenderer hook. - Implemented message preprocessing to expand multi-tool-call messages in MessagesContainer. - Updated constants for better maintainability and clarity in tool call handling. - Enhanced useAutoOpenCanvas to use a defined breakpoint constant.
…on and reset functionality - Added `onTransformChart` callback to handle chart type changes for dashboard items. - Introduced `resetKey` prop to reset the grid layout to its initial state. - Updated DashboardGrid and ChartItem components to support new props. - Enhanced story examples to demonstrate edit mode functionality.
…ctor tool rendering - Updated peer dependencies for CopilotKit packages to version ^1.10.6. - Refactored AssistantMessage to handle tool call rendering with updated generativeUI logic. - Simplified ToolRendererProvider to use a default renderer for better compatibility. - Enhanced message expansion utility to include tool call type information.
siguenzaraul
force-pushed
the
upgrade-copilot-backguard-compatible
branch
from
db450f1 to
09276a0
Compare
✅ No New Circular DependenciesNo new circular dependencies detected. Current count: 0 |
Keep only F0AnalyticsDashboard, dataDownload, and canvas features. Revert CopilotKit downgrade, ToolRendererProvider, message expansion refactor, and other unrelated changes.
📦 Alpha Package Version PublishedUse Use |
🔍 Visual review for your branch is published 🔍Here are the links to: |
| const dataAdapter = useMemo( | ||
| () => ({ | ||
| paginationType: "pages" as const, | ||
| perPage: PER_PAGE, | ||
| fetchData: (options: any) => { | ||
| let filtered = [...rows] as RecordType[] |
There was a problem hiding this comment.
fetchData is typed with any (options: any) and then relies on unsafe casts. In packages/react strict TypeScript, any/as any is a blocking convention (see packages/react/AGENTS.md TypeScript section). Please type options using the DataCollection fetch option types (e.g. DataCollectionPaginatedFetchOptions<...>) or destructure the fields you need with proper types so this remains type-safe.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const handleStateChange = useCallback( | ||
| (state: any) => { | ||
| const tableSettings = state.settings?.visualization?.table | ||
| const hiddenColumns = | ||
| (tableSettings?.hidden as string[] | undefined) ?? [] | ||
| const columnOrder = (tableSettings?.order as string[] | undefined) ?? [] | ||
| const sortings = state.sortings | ||
| ? [state.sortings].flat().filter(Boolean) | ||
| : [] | ||
| setViewState({ | ||
| search: state.search as string | undefined, | ||
| sortings, | ||
| hiddenColumns, | ||
| columnOrder, | ||
| }) | ||
| }, |
There was a problem hiding this comment.
handleStateChange is currently using state: any and multiple casts to pull out table settings/sortings. This should be typed as the onStateChange payload (DataCollectionStatusComplete<...>) so hidden/order/sortings/search are accessed safely and future refactors don’t silently break export filtering.
| <OneDataCollection | ||
| fullHeight | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| source={source as any} |
There was a problem hiding this comment.
OneDataCollection is receiving source={source as any}. This defeats strict typing and can hide mismatches between the source definition and the visualization config. Prefer providing the correct generic parameters to useDataCollectionSource/OneDataCollection (or shaping the source definition) so source can be passed without as any.
| <OneDataCollection | |
| fullHeight | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| source={source as any} | |
| <OneDataCollection<RecordType> | |
| fullHeight | |
| source={source} |
| import * as XLSX from "xlsx" | ||
|
|
||
| import type { DataDownloadDataset } from "./types" | ||
|
|
||
| /** | ||
| * Generate an .xlsx file from a dataset and trigger a browser download. | ||
| */ | ||
| export function downloadAsExcel( | ||
| dataset: DataDownloadDataset, | ||
| filename: string | ||
| ): void { | ||
| const { columns, rows, columnLabels } = dataset | ||
| const headers = columns.map((col) => columnLabels?.[col] ?? col) | ||
| const wsData = [ | ||
| headers, | ||
| ...rows.map((row) => columns.map((col) => row[col] ?? "")), | ||
| ] |
There was a problem hiding this comment.
downloadAsExcel / downloadAsCsv here duplicates existing export utilities in components/F0AnalyticsDashboard/utils/downloadHelpers.ts (including XLSX + CSV escaping) but with less robust value serialization (objects/dates become [object Object]). Consider reusing the shared helpers (mapping columnLabels to header names) to avoid divergence and keep export behavior consistent across the repo.
| const updatedChart = { | ||
| ...item.chart, | ||
| type: newType, | ||
| ...(newType === "bar" | ||
| ? { orientation: orientation ?? "vertical" } | ||
| : {}), | ||
| } as typeof item.chart |
There was a problem hiding this comment.
When transforming a chart, updatedChart is built by spreading the previous config and only overriding type (and sometimes orientation). This leaves type-specific fields from the old chart (e.g. orientation/stacked from bar) on the new type, and doesn’t map to funnel’s orient field. Please build a type-specific config per newType (dropping incompatible keys and mapping orientation → orient where applicable) to keep the config valid.
| const updatedChart = { | |
| ...item.chart, | |
| type: newType, | |
| ...(newType === "bar" | |
| ? { orientation: orientation ?? "vertical" } | |
| : {}), | |
| } as typeof item.chart | |
| const updatedChart: typeof item.chart = { | |
| ...item.chart, | |
| type: newType, | |
| } | |
| if (newType === "bar") { | |
| // Bar charts use `orientation`; remove funnel-specific `orient` | |
| updatedChart.orientation = orientation ?? "vertical" | |
| delete (updatedChart as { orient?: unknown }).orient | |
| } else if (newType === "funnel") { | |
| // Funnel charts use `orient`; drop bar-specific fields | |
| const targetOrient = orientation ?? "vertical" | |
| ;(updatedChart as { orient?: string }).orient = targetOrient | |
| delete updatedChart.orientation | |
| delete (updatedChart as { stacked?: unknown }).stacked | |
| } else { | |
| // Other chart types should not keep bar/funnel-specific fields | |
| delete updatedChart.orientation | |
| delete (updatedChart as { orient?: unknown }).orient | |
| delete (updatedChart as { stacked?: unknown }).stacked | |
| } |
| let y = 0 | ||
| for (const row of newRows) { | ||
| const colSpan = Math.floor(12 / Math.max(1, row.ids.length)) | ||
| const rowSpan = Math.round(row.height / 48) | ||
| let x = 0 | ||
| for (const id of row.ids) { | ||
| layout.push({ id, colSpan, rowSpan, x, y }) | ||
| x += colSpan |
There was a problem hiding this comment.
emitLayout computes rowSpan via Math.round(row.height / 48). Because row resize produces arbitrary pixel heights, rounding can reduce the stored span and cause the row to shrink noticeably when rebuilding from saved rowSpan (e.g. 263px → 5 → 240px). Consider snapping resize to 48px increments or using Math.ceil so persisted layout never under-allocates height.
|
|
||
| const row = rows[rowIdx] | ||
| const isFromThisRow = dragId ? row.ids.includes(dragId) : false | ||
| if (row.ids.length >= MAX_PER_ROW && !isFromThisRow) return |
There was a problem hiding this comment.
In handleRowDragOver, when the target row is already at MAX_PER_ROW and the dragged item comes from a different row, the function returns early without clearing dropTarget. This can leave a stale dropTarget from a previous hover and cause the item to drop into the wrong row/position on drag end. Please set setDropTarget(null) before returning in this branch.
| if (row.ids.length >= MAX_PER_ROW && !isFromThisRow) return | |
| if (row.ids.length >= MAX_PER_ROW && !isFromThisRow) { | |
| setDropTarget(null) | |
| return | |
| } |
| document.addEventListener("mousemove", onMove) | ||
| document.addEventListener("mouseup", onUp) | ||
| }, | ||
| [rows, emitLayout] |
There was a problem hiding this comment.
startResize uses both rows and itemMap, but itemMap isn’t listed in the hook deps. If items change (thus itemMap changes) without rows changing, getMinRowHeight can use a stale map. Please include itemMap (and ideally avoid capturing rows[rowIdx] directly) to keep resize constraints correct and satisfy exhaustive-deps.
| [rows, emitLayout] | |
| [rows, itemMap, emitLayout] |
| vi.mock("../DashboardContext", () => ({ | ||
| useDashboardCanvas: () => ({ | ||
| editMode: mockEditMode, | ||
| setEditMode: mockSetEditMode, | ||
| handleSave: mockHandleSave, | ||
| handleDiscard: mockHandleDiscard, | ||
| isDirty: false, | ||
| exportAsExcel: undefined, | ||
| registerExport: vi.fn(), | ||
| }), | ||
| })) |
There was a problem hiding this comment.
DashboardHeader behavior changed (edit actions removed, export button conditional on exportAsExcel), but the unit tests now only cover title + close. Please add coverage for the new export UI (button renders only when exportAsExcel is provided and triggers the export function / shows exporting state) to avoid regressions.
| const getFilteredDataset = useCallback((): DataDownloadDataset => { | ||
| const { dataset } = content | ||
| const { columns, rows, columnLabels } = dataset | ||
| const { search, sortings, hiddenColumns, columnOrder } = | ||
| viewStateRef.current | ||
|
|
||
| // Filter by search | ||
| let filtered = [...rows] | ||
| if (search?.trim()) { | ||
| const term = search.toLowerCase() | ||
| filtered = filtered.filter((row) => | ||
| columns.some((col) => { | ||
| const val = row[col] | ||
| return val != null && String(val).toLowerCase().includes(term) | ||
| }) | ||
| ) | ||
| } | ||
|
|
||
| // Sort | ||
| if (sortings?.length) { | ||
| for (const { field, order } of [...sortings].reverse()) { | ||
| filtered.sort((a, b) => { | ||
| const aVal = a[field] | ||
| const bVal = b[field] | ||
| if (aVal == null && bVal == null) return 0 | ||
| if (aVal == null) return 1 | ||
| if (bVal == null) return -1 | ||
| if (typeof aVal === "number" && typeof bVal === "number") { | ||
| return order === "asc" ? aVal - bVal : bVal - aVal | ||
| } | ||
| const cmp = String(aVal).localeCompare(String(bVal)) | ||
| return order === "asc" ? cmp : -cmp | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Filtering/sorting logic is duplicated here (getFilteredDataset) and again in DataDownloadContent’s dataAdapter.fetchData. This makes it easy for table behavior and exported dataset behavior to drift over time. Consider extracting a shared helper (e.g. filterSortDataset(dataset, viewState)) and reusing it in both places.
Add date-navigator support to F0AnalyticsDashboard with navigation filter state merged into grid filters. Update mock data to generate deterministic week-based variations across all chart types.
| return ( | ||
| <div className="h-full flex-1 pt-5"> | ||
| <OneDataCollection | ||
| fullHeight | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| source={source as any} | ||
| visualizations={visualizations} |
There was a problem hiding this comment.
source={source as any} uses as any, which is explicitly disallowed in packages/react. Please make the source type match OneDataCollection by providing the right generic parameters (or adjusting useDataCollectionSource call) so the cast can be removed.
| export function DataDownloadContent({ | ||
| content, | ||
| }: { | ||
| content: DataDownloadCanvasContent | ||
| refreshKey?: number | ||
| }) { |
There was a problem hiding this comment.
The props type includes refreshKey?: number but the component doesn't use it. With TS noUnusedParameters/linting this will fail; either destructure refreshKey and use it (e.g. to reset internal state) or remove it from the props shape.
| const { columns, rows, columnLabels } = dataset | ||
| const headers = columns.map((col) => columnLabels?.[col] ?? col) | ||
| const wsData = [ | ||
| headers, | ||
| ...rows.map((row) => columns.map((col) => row[col] ?? "")), | ||
| ] |
There was a problem hiding this comment.
When building worksheet/CSV rows, values are written as row[col] ?? "" / String(value), which will export objects as [object Object] and Dates in locale-dependent formats. Consider serializing values (null→"", Date→ISO, objects→JSON) before export to avoid corrupted spreadsheets.
| exportFilename={content.title} | ||
| /> | ||
| <F0ActionBar | ||
| label="Changes detected" |
There was a problem hiding this comment.
label="Changes detected" is a user-facing string and should be localized via useI18n()/t() per repo i18n guidelines. Add a translation key (e.g. under ai.*) and use it here.
| label="Changes detected" | |
| label={translations.ai.changesDetected} |
| row.ids.map((id) => ({ | ||
| ids: [id], | ||
| height: row.height, | ||
| })) |
There was a problem hiding this comment.
In narrow mode, displayRows flattens multi-item rows but preserves the original row's height for every item. This will make smaller widgets (e.g. metrics) inherit a chart/collection row height and render with large empty space. Compute a per-item height when flattening (based on item type / saved rowSpan) instead of reusing the shared row height.
| row.ids.map((id) => ({ | |
| ids: [id], | |
| height: row.height, | |
| })) | |
| row.ids.map((id) => { | |
| const singleRow = { ...row, ids: [id] } | |
| return { | |
| ids: [id], | |
| height: getMinRowHeight(singleRow, itemMap), | |
| } | |
| }) |
Description
Screenshots (if applicable)
[Link to Figma Design](Figma URL here)
Implementation details