Redesigning_ingest_ui#1699
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new Unified Ingestion UI and wiring: a grouped source→connection table with warehouse header/details, search/empty states, inline schema-change badges, per-connection sync controls/logs, centralized styles/utilities, a comprehensive useIngestData hook, CreateConnectionForm preselection, and ingest page integration; includes related TransformWorkflow (FlowEditor/Canvas/NodeDetailModal) UI changes. Changes
Sequence Diagram(s)mermaid User->>UI: click "Sync" / "Add Connection" / search Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1699 +/- ##
==========================================
- Coverage 75.33% 68.92% -6.42%
==========================================
Files 107 122 +15
Lines 7962 8704 +742
Branches 1866 2070 +204
==========================================
+ Hits 5998 5999 +1
- Misses 1927 2543 +616
- Partials 37 162 +125 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Connections/CreateConnectionForm.tsx (1)
275-286:⚠️ Potential issue | 🟠 MajorDuplicate websocket message send —
sendJsonMessageis called twice.
setLoading(true)andsendJsonMessage(...)are each called twice in this effect. This fires the schema catalog request twice per source selection, which wastes resources and could cause race conditions withlastJsonMessageprocessing.🐛 Proposed fix — remove the duplicate call
useEffect(() => { if (watchSourceSelection?.id && !connectionId) { setLoading(true); sendJsonMessage({ sourceId: watchSourceSelection.id, }); - setLoading(true); - sendJsonMessage({ - sourceId: watchSourceSelection.id, - }); } }, [watchSourceSelection]);
🤖 Fix all issues with AI agents
In `@INGEST_REDESIGN.md`:
- Around line 200-215: The doc uses two spellings ("canceled" from the API and
"cancelled" in app UI/lock states) which is confusing; update INGEST_REDESIGN.md
to explicitly state that lastRun.status uses the Airbyte API value "canceled"
while the app's display and lock.status use "cancelled", and normalize the table
labels to show both forms where relevant (e.g., use `lastRun.status ===
'canceled' (Airbyte)` and `lock.status === 'cancelled' (UI/lock)`) and add one
short clarifying note explaining the distinction; reference the table rows for
`lastRun.status`, `lock.status`, and the `syncingConnectionIds.includes(id)`
entry when making this change.
- Around line 385-388: The table row for StatusIcon is missing the third "Used
for" cell causing a column mismatch; update the markdown table so each row has
three cells by adding the missing description for StatusIcon (e.g., "Connection
status indicator" or the accurate usage) in the third column for the
`StatusIcon` row; ensure the `StatusIcon` entry references the symbol
`StatusIcon` and the file `src/components/Connections/Connections.tsx` and that
all rows (`LogCard`, `QueueTooltip`, `StatusIcon`) have three pipe-separated
cells to match the header.
In `@src/components/Ingest/ConnectionRow.tsx`:
- Around line 199-221: The "+ Add Connection" control rendered when
isLastConnection && permissions.includes('can_create_connection') uses a
non-interactive Typography with onClick (onAddConnection) and is not
keyboard-accessible; replace it with a proper interactive element (preferably
MUI Button) or, if keeping Typography, add role="button", tabIndex={0}, and an
onKeyDown handler that calls onAddConnection for Enter/Space and ensure sx
retains visual styles; update the element where onAddConnection is used and
adjust accessibility attributes accordingly.
In `@src/components/Ingest/ConnectionSyncButton.tsx`:
- Around line 40-50: handleSync currently awaits syncConnection without
try/catch so if syncConnection throws tempSyncState remains true and the button
stays disabled; wrap the await call in a try/catch (or try/finally) around the
syncConnection(deploymentId, connectionId) call in handleSync, ensure you call
setTempSyncState(false) in a finally block (or in catch and after success) and
remove connectionId from syncing list via setSyncingConnectionIds (reverse the
previous add) on error/finally, and keep the existing res?.error === 'ERROR'
handling for non-throwing responses; reference handleSync, syncConnection,
setTempSyncState, setSyncingConnectionIds, and trackAmplitudeEvent when making
the change.
In `@src/components/Ingest/SchemaChangeBadge.tsx`:
- Around line 38-49: The "Review" interactive text uses Typography with onClick
(component: Typography, prop: onReview, and prop: disabled) but is not keyboard-
or screen-reader-accessible; replace the Typography with MUI Button
variant="text" (import Button) and wire disabled and onClick to the Button so it
is focusable and announced as a button, or if you prefer to keep Typography, add
role="button", tabIndex={0}, and an onKeyDown handler that calls onReview for
Enter/Space (respecting disabled) to ensure keyboard activation and proper ARIA
semantics.
In `@src/components/Ingest/SourceRow.tsx`:
- Around line 165-188: The clickable "+ Add Connection" in SourceRow is
implemented as a Typography with only onClick and thus isn't
keyboard-accessible; update the element that renders the label in the SourceRow
component so it includes role="button", tabIndex={0}, and an onKeyDown handler
that triggers onAddConnection(source.sourceId) when Enter or Space is pressed
(mirror the fix applied in ConnectionRow), while preserving existing sx props
and the isSourceNew/pulseGlowSx behavior to maintain visuals and focusability.
- Around line 56-90: Add a cleanup effect that clears any pending timeouts and
nulls refs on unmount: in the component, implement a useEffect with an empty
dependency array that if groupLeaveRef.current exists calls
clearTimeout(groupLeaveRef.current) and sets groupLeaveRef.current = null, and
similarly clears and nulls connLeaveRef.current; also optionally call
setIsGroupHovered(false) and setHoveredConnIdx(null) inside that cleanup to
ensure state is reset. This ensures pending handlers from handleGroupMouseLeave
and handleConnMouseLeave are cancelled on unmount.
In `@src/components/Ingest/StatusIcon.tsx`:
- Around line 15-23: StatusIcon currently forwards the sx prop to every rendered
icon except when status === 'queued' where it renders QueueTooltip without sx;
update the queued branch to pass sx through (e.g., <QueueTooltip sx={sx}
queueInfo={queueInfo} />) and ensure QueueTooltip's props/type (QueueTooltip)
accept and apply the sx prop so sizing/style stays consistent across statuses.
In `@src/components/Ingest/SyncStatusCell.tsx`:
- Around line 24-49: The cancellation branch that sets jobStatus = 'cancelled'
when lock?.status === 'cancelled' should also record that the cancel came from
the lock so the UI can show lock-based time info instead of lastRunTime when
lastRun is null; update the logic in SyncStatusCell.tsx (where jobStatus,
jobStatusColor, lock, lastRun, lastRunTime, and syncingConnectionIds are used)
to set a flag (e.g., cancelledFromLock) or set a jobTime variable to
lock.updatedAt/lock.createdAt when lock?.status === 'cancelled', and then change
the terminal-state time rendering (the block that currently falls back to
lastRunTime(lastRun?.startTime)) to prefer the lock-based time when
cancelledFromLock is true (or jobTime is present) and only fall back to
lastRunTime otherwise.
In `@src/components/Ingest/UnifiedIngestionView.tsx`:
- Around line 68-73: The component UnifiedIngestionView currently only checks
data.isLoading and falls back to EmptyState on failure; update the useIngestData
hook to destructure and return SWR error values for sourcesData and
connectionsData (e.g., include sourcesError and connectionsError alongside
sourcesData/connectionsData/isLoading), then in UnifiedIngestionView add a
conditional branch that checks these error fields (from the useIngestData
return) and renders an error UI or error message (instead of EmptyState) when
either sourcesError or connectionsError is truthy; ensure you reference and
update the existing symbols useIngestData, sourcesData, connectionsData,
sourcesError, connectionsError, isLoading, and UnifiedIngestionView so callers
consume the new error fields.
In `@src/components/Ingest/useIngestData.ts`:
- Around line 472-503: The recursive polling in refreshConnectionSchema and
handleRefreshConnectionDirect uses an unbounded checkRefresh which can recurse
forever; extract the polling into a shared helper (e.g., pollTaskStatus or
waitForTaskCompletion) and replace each local checkRefresh with a call to that
helper, implementing a retry ceiling (maxRetries or timeout) and converting
recursion to an iterative loop or controlled recursion that stops after the max
attempts; ensure the helper handles unexpected statuses by logging/errorToast,
resolves on 'completed' to call mutateConnections where needed, rejects or
returns failure on 'failed' or when maxRetries is exceeded, and preserves
setIsRefreshing state handling in the callers (refreshConnectionSchema and
handleRefreshConnectionDirect).
- Around line 348-374: The issue is that pollForFlowRun is invoked
fire-and-forget in syncConnection, causing the finally block to remove
connectionId from syncingConnectionIds before polling completes; to fix it,
await pollForFlowRun(response.flow_run_id) (or capture its Promise and await it)
so the function only reaches the finally block after polling finishes, and
propagate/handle any errors from pollForFlowRun inside the try/catch so
syncingConnectionIds is only cleared after the poll completes; locate
syncConnection and replace the un-awaited pollForFlowRun call with an awaited
call (or await the Promise) and adjust control flow accordingly.
- Around line 298-302: fetchSchemaChanges currently closes over session and
isn't stable, causing stale-session behavior when called later (and ESLint warns
when used in the effect and returned from the hook). Wrap fetchSchemaChanges in
useCallback and include session (and any other values it uses) in its dependency
array so it is recreated whenever session changes; then update the useEffect to
depend on fetchSchemaChanges (or keep session only if you prefer, but ESLint
will accept the memoized function). Ensure the same useCallback-wrapped
fetchSchemaChanges is the function returned from the hook so consumers always
get the up-to-date closure.
In `@src/components/Ingest/WarehouseDetailsPanel.tsx`:
- Around line 86-96: The Link rendering the external URL (the Link element that
uses href={row.link} and target="_blank"} inside WarehouseDetailsPanel) should
include rel="noopener noreferrer" to prevent the opened page from accessing
window.opener; update that Link JSX to add the rel attribute with both values
(rel="noopener noreferrer") while keeping existing props (href, target, sx, and
children).
In `@src/components/Ingest/WarehouseHeader.tsx`:
- Around line 129-136: The Next.js Image usage in WarehouseHeader.tsx (the
<Image> rendering warehouse.icon) will fail for Airbyte CDN URLs unless Next's
image config allows that host; update next.config.js to add the Airbyte CDN to
images.domains or add an images.remotePatterns entry that matches the Airbyte
CDN path (e.g., connectors.airbyte.com or
connecters.airbyte.com/files/metadata/*) so remote warehouse.icon URLs are
permitted, then rebuild. Ensure the change targets the Next config export
(nextConfig / module.exports) so the Image component can load remote icons at
runtime.
🧹 Nitpick comments (15)
src/components/Ingest/ingestStyles.ts (1)
3-18: Hardcoded color tokens bypass MUI theming.All colors are defined as static hex values rather than referencing MUI theme palette tokens. This means dark mode or theme overrides won't automatically propagate to these components. If theme support is planned, consider aligning these with
theme.palettein the future.src/components/Ingest/useIngestData.ts (2)
155-176: Inconsistent data-fetching: source definitions use manualuseEffect+httpGetwhile other data uses SWR.Warehouse, sources, and connections all use
useSWR, but source definitions are fetched manually. This loses SWR's caching, deduplication, and revalidation benefits. Also,globalContextis used inside the effect but not listed in the dependency array (line 176), which would trigger an ESLintreact-hooks/exhaustive-depswarning.♻️ Consider using useSWR for source definitions
- const [sourceDefs, setSourceDefs] = useState<SourceDefOption[]>([]); - const [sourceDefsLoading, setSourceDefsLoading] = useState(false); - - useEffect(() => { - if (!session) return; - const fetchSourceDefs = async () => { - ... - }; - fetchSourceDefs(); - }, [session]); + const { data: sourceDefsRaw, isLoading: sourceDefsLoading } = useSWR( + session ? 'airbyte/source_definitions' : null, + { revalidateOnFocus: false } + ); + + const sourceDefs: SourceDefOption[] = useMemo(() => { + if (!sourceDefsRaw) return []; + return sourceDefsRaw.map((el: any) => ({ + id: el.sourceDefinitionId, + label: el.name, + dockerRepository: el.dockerRepository, + dockerImageTag: el.dockerImageTag, + tag: el.dockerImageTag, + })); + }, [sourceDefsRaw]);
77-826: The hook manages ~30+ state variables and ~90 return values — consider decomposition.
useIngestDatais a "God hook" that centralizes warehouse state, source CRUD, connection CRUD, sync logic, schema changes, stream clearing, search, new-item tracking, and multiple dialog states. This makes it difficult to test individual concerns, increases re-render surface (any state change re-renders all consumers), and is hard to navigate.Consider splitting into focused hooks (e.g.,
useWarehouse,useSourceActions,useConnectionActions,useSyncState,useSchemaChanges) and composing them inuseIngestDataor at the view level.src/components/Ingest/StatusIcon.tsx (1)
9-13: Props useanytypes where stronger types are available.
sxcould be typed asSxProps<Theme>from MUI, andqueueInfocould use theQueuedRuntimeInfotype from the Connections module (ornull). This would improve type safety and catch misuse at compile time.src/components/Ingest/SearchToolbar.tsx (1)
43-59: Fixedwidth: 380may clip on narrow viewports.If the ingest page is ever rendered in a narrower container or on mobile, the fixed pixel width could cause overflow. Consider
maxWidth: 380with a flexible width, or a responsive value.src/components/Ingest/SourceCell.tsx (1)
94-94: Magic numberml: 6.5couples to icon size and gap.The left margin of
6.5(52px) is presumably36px icon + 12px gap + 4pxto align actions under the text. If the icon container size (line 37-38) or gap (line 33) changes, this will misalign. Consider deriving this from a shared constant or using CSS alignment instead.src/components/Ingest/WarehouseDetailsPanel.tsx (2)
25-35: SimplifywarehouseForHelperconstruction with a spread.
WarehouseInfoandWarehouseshare identical fields. The manual property-by-property mapping can be replaced with a spread, reducing boilerplate and maintenance if fields change.♻️ Suggested simplification
- const warehouseForHelper: Warehouse = { - airbyteWorkspaceId: warehouse.airbyteWorkspaceId, - destinationId: warehouse.destinationId, - destinationDefinitionId: warehouse.destinationDefinitionId, - name: warehouse.name, - wtype: warehouse.wtype, - icon: warehouse.icon, - connectionConfiguration: warehouse.connectionConfiguration, - airbyteDockerRepository: warehouse.airbyteDockerRepository, - tag: warehouse.tag, - }; + const warehouseForHelper: Warehouse = { ...warehouse };
43-50: FixedmaxHeightof 500px may clip content for warehouses with many properties.If a warehouse type returns a large number of key-value pairs (e.g., BigQuery with GCS Staging adds extra rows, plus common fields), the
500pxcap could clip content without any scroll indicator sinceoverflow: 'hidden'is set. Consider using a larger value orautoheight with a different animation approach.src/components/Ingest/SyncStatusCell.tsx (1)
104-109: Status label is displayed as a raw lowercase string.
{jobStatus}renders values like"success","failed","running"as-is. Consider capitalizing the first letter for a more polished UI presentation (e.g.,"Success","Failed","Running").✨ Suggested change
<Typography variant="body2" sx={{ fontWeight: 700, color: jobStatusColor, fontSize: '0.8rem' }} > - {jobStatus} + {jobStatus.charAt(0).toUpperCase() + jobStatus.slice(1)} </Typography>src/components/Ingest/SourceTable.tsx (2)
19-40: Consider reducing prop drilling with a context or composite object.
SourceTableaccepts 21 props, most of which are passed straight through toSourceRow. This makes both components harder to maintain and extend. Consider grouping related callbacks into an object (e.g.,sourceActions,connectionActions,syncState) or providing them via a React context.Also applies to: 62-83
170-191: Inconsistent pattern:isSourceNewis pre-evaluated,isConnectionNewis passed as a function.Line 172 calls
isSourceNew(group.source.sourceId)and passes a boolean, while line 173 passesisConnectionNewas a function reference. This is likely intentional (one source per group, multiple connections), but it creates an asymmetric API surface forSourceRow. Documenting or making both consistent (e.g., always pass functions) would improve readability.src/components/Ingest/UnifiedIngestionView.tsx (2)
18-19: Consider destructuringuseIngestData()to improve readability.Accessing everything via
data.xxx(~50+ references) makes it difficult to see at a glance which pieces of state this component actually depends on. Destructuring the hook's return value (or at least grouping into sub-objects likedialogs,handlers,permissions) would make the dependency surface explicit and improve readability.
56-65: Duplicated "add source" handler.The same two-call sequence (
setSourceIdToEdit('') + setShowSourceDialog(true)) appears in both theSearchToolbar(Lines 60–63) and theEmptyState(Lines 77–80). Extract a singlehandleAddSourcecallback to avoid drift.♻️ Suggested refactor
export default function UnifiedIngestionView() { const data = useIngestData(); + + const handleAddSource = useCallback(() => { + data.setSourceIdToEdit(''); + data.setShowSourceDialog(true); + }, [data]); return ( <Box> ... <SearchToolbar searchText={data.searchText} onSearchChange={data.setSearchText} - onAddSource={() => { - data.setSourceIdToEdit(''); - data.setShowSourceDialog(true); - }} + onAddSource={handleAddSource} canCreateSource={data.permissions.includes('can_create_source')} /> ... <EmptyState searchText={data.searchText} - onAddSource={() => { - data.setSourceIdToEdit(''); - data.setShowSourceDialog(true); - }} + onAddSource={handleAddSource} canCreateSource={data.permissions.includes('can_create_source')} />(Also add
import { useCallback } from 'react'at the top.)Also applies to: 74-82
src/components/Ingest/ConnectionRow.tsx (1)
13-38: Large prop surface (26 props) — consider grouping.This component accepts 26 individual props, many of which are passed through to children (
permissions,syncingConnectionIds,setSyncingConnectionIds,syncConnection,trackAmplitudeEvent). Consider grouping related props into objects (e.g., asyncPropsbag or ahandlersobject) to reduce the signature size and make call sites less noisy.INGEST_REDESIGN.md (1)
24-28: Fenced code blocks lack language specifiers.Multiple ASCII-art diagram blocks have no language identifier (flagged by markdownlint MD040). Use
```textor```plaintextfor these blocks to satisfy linters and improve rendering in some Markdown viewers.Also applies to: 38-42, 50-66, 86-110, 151-156, 165-169, 177-183, 187-192, 241-247, 422-427
| | Condition | Status text | Color | Icon | Sync button | | ||
| |-----------|------------|-------|------|-------------| | ||
| | `lastRun.status === 'succeeded'` | `success` | `#399D47` | `TaskAltIcon` | [Sync] enabled | | ||
| | `lastRun.status === 'canceled'` | `cancelled` | `#DAA520` | `CancelIcon` | [Sync] enabled | | ||
| | `lastRun.status` anything else | `failed` | `#981F1F` | `WarningAmberIcon` | [Sync] enabled | | ||
| | No lastRun | `Never synced` | grey | — | [Sync] enabled (pulse if new) | | ||
|
|
||
| ### Active states (lock present) | ||
|
|
||
| | Condition | Status text | Extra info | Sync button | | ||
| |-----------|------------|-----------|-------------| | ||
| | `lock.status === 'running'` | `running` | "Triggered by: {email}" + "{time}" | Disabled + spinner icon | | ||
| | `lock.status === 'queued'` | `queued` | Queue tooltip (position, wait time) | **[Cancel]** button | | ||
| | `lock.status === 'locked'` or `'complete'` | `locked` | "Triggered by: {email}" + "{time}" | Disabled + spinner icon | | ||
| | `lock.status === 'cancelled'` | `cancelled` | — | Disabled + spinner icon | | ||
| | `syncingConnectionIds.includes(id)` (no lock yet) | `queued` | — | Disabled + spinner icon | |
There was a problem hiding this comment.
Inconsistent spelling of 'cancelled' vs 'canceled'.
The doc mixes canceled (Line 203, from API values) and cancelled (Lines 214, display/lock status). While the API value spelling is dictated by Airbyte, the documentation should clarify this distinction to avoid confusion — e.g., add a brief note that canceled is the Airbyte API value while cancelled is used in the app's display and lock status.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~203-~203: Do not mix variants of the same word (‘cancelled’ and ‘canceled’) within a single text.
Context: ...d | | lastRun.status === 'canceled' | cancelled | #DAA520 | CancelIcon | [Sync] en...
(EN_EXACT_COHERENCY_RULE)
[uncategorized] ~214-~214: Do not mix variants of the same word (‘cancelled’ and ‘canceled’) within a single text.
Context: ... "{time}" | Disabled + spinner icon | | lock.status === 'cancelled' | cancelled | — | Disabled + spinne...
(EN_EXACT_COHERENCY_RULE)
[uncategorized] ~214-~214: Do not mix variants of the same word (‘cancelled’ and ‘canceled’) within a single text.
Context: ...con | | lock.status === 'cancelled' | cancelled | — | Disabled + spinner icon | | `syn...
(EN_EXACT_COHERENCY_RULE)
🤖 Prompt for AI Agents
In `@INGEST_REDESIGN.md` around lines 200 - 215, The doc uses two spellings
("canceled" from the API and "cancelled" in app UI/lock states) which is
confusing; update INGEST_REDESIGN.md to explicitly state that lastRun.status
uses the Airbyte API value "canceled" while the app's display and lock.status
use "cancelled", and normalize the table labels to show both forms where
relevant (e.g., use `lastRun.status === 'canceled' (Airbyte)` and `lock.status
=== 'cancelled' (UI/lock)`) and add one short clarifying note explaining the
distinction; reference the table rows for `lastRun.status`, `lock.status`, and
the `syncingConnectionIds.includes(id)` entry when making this change.
| | `LogCard` | `src/components/Logs/LogCard.tsx` | Real-time sync logs at bottom | | ||
| | `QueueTooltip` | `src/components/Connections/Connections.tsx` (exported) | Queue position tooltip | | ||
| | `StatusIcon` | `src/components/Connections/Connections.tsx` (not exported — need to extract or inline) | | ||
|
|
There was a problem hiding this comment.
Table row missing a cell — column count mismatch.
Line 387 has only 2 cells but the table header defines 3 columns (Component | File | Used for). The StatusIcon row is missing the "Used for" column, which will render incorrectly in most Markdown parsers.
📝 Suggested fix
-| `StatusIcon` | `src/components/Connections/Connections.tsx` (not exported — need to extract or inline) |
+| `StatusIcon` | `src/components/Connections/Connections.tsx` (not exported — need to extract or inline) | Status icon for sync states |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | `LogCard` | `src/components/Logs/LogCard.tsx` | Real-time sync logs at bottom | | |
| | `QueueTooltip` | `src/components/Connections/Connections.tsx` (exported) | Queue position tooltip | | |
| | `StatusIcon` | `src/components/Connections/Connections.tsx` (not exported — need to extract or inline) | | |
| | `LogCard` | `src/components/Logs/LogCard.tsx` | Real-time sync logs at bottom | | |
| | `QueueTooltip` | `src/components/Connections/Connections.tsx` (exported) | Queue position tooltip | | |
| | `StatusIcon` | `src/components/Connections/Connections.tsx` (not exported — need to extract or inline) | Status icon for sync states | |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 387-387: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
🤖 Prompt for AI Agents
In `@INGEST_REDESIGN.md` around lines 385 - 388, The table row for StatusIcon is
missing the third "Used for" cell causing a column mismatch; update the markdown
table so each row has three cells by adding the missing description for
StatusIcon (e.g., "Connection status indicator" or the accurate usage) in the
third column for the `StatusIcon` row; ensure the `StatusIcon` entry references
the symbol `StatusIcon` and the file
`src/components/Connections/Connections.tsx` and that all rows (`LogCard`,
`QueueTooltip`, `StatusIcon`) have three pipe-separated cells to match the
header.
| {isLastConnection && permissions.includes('can_create_connection') && ( | ||
| <Box | ||
| sx={{ | ||
| mt: 0.75, | ||
| opacity: isHovered ? 1 : 0, | ||
| transition: 'opacity 0.25s ease', | ||
| pointerEvents: isHovered ? 'auto' : 'none', | ||
| }} | ||
| > | ||
| <Typography | ||
| variant="body2" | ||
| onClick={onAddConnection} | ||
| sx={{ | ||
| color: colors.primary, | ||
| cursor: 'pointer', | ||
| fontWeight: 600, | ||
| '&:hover': { textDecoration: 'underline' }, | ||
| }} | ||
| > | ||
| + Add Connection | ||
| </Typography> | ||
| </Box> | ||
| )} |
There was a problem hiding this comment.
+ Add Connection is not keyboard-accessible.
The Typography element with onClick (Line 210) is not reachable via keyboard navigation — it has no tabIndex, role="button", or onKeyDown handler. Users relying on keyboard or assistive technology won't be able to trigger this action.
Use a Button or at minimum add role="button", tabIndex={0}, and an onKeyDown handler for Enter/Space.
♿ Suggested fix
<Typography
variant="body2"
onClick={onAddConnection}
+ role="button"
+ tabIndex={0}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ onAddConnection();
+ }
+ }}
sx={{
color: colors.primary,
cursor: 'pointer',
fontWeight: 600,
'&:hover': { textDecoration: 'underline' },
}}
>🤖 Prompt for AI Agents
In `@src/components/Ingest/ConnectionRow.tsx` around lines 199 - 221, The "+ Add
Connection" control rendered when isLastConnection &&
permissions.includes('can_create_connection') uses a non-interactive Typography
with onClick (onAddConnection) and is not keyboard-accessible; replace it with a
proper interactive element (preferably MUI Button) or, if keeping Typography,
add role="button", tabIndex={0}, and an onKeyDown handler that calls
onAddConnection for Enter/Space and ensure sx retains visual styles; update the
element where onAddConnection is used and adjust accessibility attributes
accordingly.
| const handleSync = async () => { | ||
| setTempSyncState(true); | ||
| trackAmplitudeEvent('[Sync-connection] Button Clicked'); | ||
| if (!syncingConnectionIds.includes(connectionId)) { | ||
| setSyncingConnectionIds((prev) => [...prev, connectionId]); | ||
| } | ||
| const res = await syncConnection(deploymentId, connectionId); | ||
| if (res?.error === 'ERROR') { | ||
| setTempSyncState(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Missing error handling — if syncConnection throws, the button stays permanently disabled.
handleSync awaits syncConnection without a try/catch. If the call throws (e.g., network failure), tempSyncState remains true and the Sync button stays disabled until the component remounts or the lock polling cycle resets it. The res?.error === 'ERROR' check on line 47 only handles a specific non-throwing error shape.
🐛 Proposed fix — wrap in try/catch
const handleSync = async () => {
setTempSyncState(true);
trackAmplitudeEvent('[Sync-connection] Button Clicked');
if (!syncingConnectionIds.includes(connectionId)) {
setSyncingConnectionIds((prev) => [...prev, connectionId]);
}
- const res = await syncConnection(deploymentId, connectionId);
- if (res?.error === 'ERROR') {
+ try {
+ const res = await syncConnection(deploymentId, connectionId);
+ if (res?.error === 'ERROR') {
+ setTempSyncState(false);
+ }
+ } catch {
setTempSyncState(false);
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleSync = async () => { | |
| setTempSyncState(true); | |
| trackAmplitudeEvent('[Sync-connection] Button Clicked'); | |
| if (!syncingConnectionIds.includes(connectionId)) { | |
| setSyncingConnectionIds((prev) => [...prev, connectionId]); | |
| } | |
| const res = await syncConnection(deploymentId, connectionId); | |
| if (res?.error === 'ERROR') { | |
| setTempSyncState(false); | |
| } | |
| }; | |
| const handleSync = async () => { | |
| setTempSyncState(true); | |
| trackAmplitudeEvent('[Sync-connection] Button Clicked'); | |
| if (!syncingConnectionIds.includes(connectionId)) { | |
| setSyncingConnectionIds((prev) => [...prev, connectionId]); | |
| } | |
| try { | |
| const res = await syncConnection(deploymentId, connectionId); | |
| if (res?.error === 'ERROR') { | |
| setTempSyncState(false); | |
| } | |
| } catch { | |
| setTempSyncState(false); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@src/components/Ingest/ConnectionSyncButton.tsx` around lines 40 - 50,
handleSync currently awaits syncConnection without try/catch so if
syncConnection throws tempSyncState remains true and the button stays disabled;
wrap the await call in a try/catch (or try/finally) around the
syncConnection(deploymentId, connectionId) call in handleSync, ensure you call
setTempSyncState(false) in a finally block (or in catch and after success) and
remove connectionId from syncing list via setSyncingConnectionIds (reverse the
previous add) on error/finally, and keep the existing res?.error === 'ERROR'
handling for non-throwing responses; reference handleSync, syncConnection,
setTempSyncState, setSyncingConnectionIds, and trackAmplitudeEvent when making
the change.
| <Typography | ||
| variant="body2" | ||
| onClick={disabled ? undefined : onReview} | ||
| sx={{ | ||
| color: disabled ? colors.textTertiary : colors.primary, | ||
| cursor: disabled ? 'default' : 'pointer', | ||
| fontWeight: 600, | ||
| '&:hover': disabled ? {} : { textDecoration: 'underline' }, | ||
| }} | ||
| > | ||
| Review | ||
| </Typography> |
There was a problem hiding this comment.
"Review" action is not keyboard-accessible or screen-reader-friendly.
The Typography element with onClick is not focusable via keyboard (no tabIndex), has no ARIA role, and won't be announced as interactive by assistive technology. Consider using a Button variant="text" or adding role="button", tabIndex={0}, and an onKeyDown handler for Enter/Space.
♿ Proposed fix: use a Button instead
- <Typography
- variant="body2"
- onClick={disabled ? undefined : onReview}
- sx={{
- color: disabled ? colors.textTertiary : colors.primary,
- cursor: disabled ? 'default' : 'pointer',
- fontWeight: 600,
- '&:hover': disabled ? {} : { textDecoration: 'underline' },
- }}
- >
- Review
- </Typography>
+ <Button
+ variant="text"
+ size="small"
+ disabled={disabled}
+ onClick={onReview}
+ sx={{
+ color: disabled ? colors.textTertiary : colors.primary,
+ fontWeight: 600,
+ textTransform: 'none',
+ minWidth: 'unset',
+ p: 0,
+ '&:hover': { textDecoration: 'underline', backgroundColor: 'transparent' },
+ }}
+ >
+ Review
+ </Button>Remember to add Button to the MUI import on line 1.
🤖 Prompt for AI Agents
In `@src/components/Ingest/SchemaChangeBadge.tsx` around lines 38 - 49, The
"Review" interactive text uses Typography with onClick (component: Typography,
prop: onReview, and prop: disabled) but is not keyboard- or
screen-reader-accessible; replace the Typography with MUI Button variant="text"
(import Button) and wire disabled and onClick to the Button so it is focusable
and announced as a button, or if you prefer to keep Typography, add
role="button", tabIndex={0}, and an onKeyDown handler that calls onReview for
Enter/Space (respecting disabled) to ensure keyboard activation and proper ARIA
semantics.
| useEffect(() => { | ||
| if (session) { | ||
| fetchSchemaChanges(); | ||
| } | ||
| }, [session]); |
There was a problem hiding this comment.
fetchSchemaChanges is not stable and uses a stale closure for session.
fetchSchemaChanges (line 283) captures session from the outer closure but is not wrapped in useCallback. It is both called in the useEffect at line 299 and returned in the hook's public API (line 790). If a consumer calls fetchSchemaChanges after session changes, it may use the stale reference from when the function was created. Also, ESLint would flag fetchSchemaChanges as a missing dependency of this effect.
🛡️ Consider wrapping in useCallback
- const fetchSchemaChanges = async () => {
+ const fetchSchemaChanges = useCallback(async () => {
try {
const data = await httpGet(session, 'airbyte/v1/connection/schema_change');
...
} catch (err: any) {
console.error(err);
}
- };
+ }, [session]);🤖 Prompt for AI Agents
In `@src/components/Ingest/useIngestData.ts` around lines 298 - 302,
fetchSchemaChanges currently closes over session and isn't stable, causing
stale-session behavior when called later (and ESLint warns when used in the
effect and returned from the hook). Wrap fetchSchemaChanges in useCallback and
include session (and any other values it uses) in its dependency array so it is
recreated whenever session changes; then update the useEffect to depend on
fetchSchemaChanges (or keep session only if you prefer, but ESLint will accept
the memoized function). Ensure the same useCallback-wrapped fetchSchemaChanges
is the function returned from the hook so consumers always get the up-to-date
closure.
| const syncConnection = async (deploymentId: string, connectionId: string) => { | ||
| setExpandSyncLogs(true); | ||
| if (!deploymentId) { | ||
| errorToast('Deployment not created', [], globalContext); | ||
| return; | ||
| } | ||
| try { | ||
| const response = await httpPost(session, `prefect/v1/flows/${deploymentId}/flow_run/`, {}); | ||
| if (response?.detail) { | ||
| errorToast(response.detail, [], globalContext); | ||
| return { error: 'ERROR' }; | ||
| } | ||
| if (!response?.flow_run_id) { | ||
| errorToast('Something went wrong', [], globalContext); | ||
| return { error: 'ERROR' }; | ||
| } | ||
| successToast('Sync initiated successfully', [], globalContext); | ||
| pollForFlowRun(response.flow_run_id); | ||
| mutateConnections(); | ||
| } catch (err: any) { | ||
| console.error(err); | ||
| errorToast(err.message, [], globalContext); | ||
| return { error: 'ERROR' }; | ||
| } finally { | ||
| setSyncingConnectionIds((prev) => prev.filter((id) => id !== connectionId)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
pollForFlowRun is fire-and-forget, causing syncingConnectionIds to clear prematurely.
On line 365, pollForFlowRun is called without await, so the finally block on line 372 runs immediately — removing the connectionId from syncingConnectionIds before the poll completes. The UI will stop showing the syncing indicator while the sync is still in progress.
🐛 Proposed fix: await the poll
successToast('Sync initiated successfully', [], globalContext);
- pollForFlowRun(response.flow_run_id);
+ await pollForFlowRun(response.flow_run_id);
mutateConnections();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const syncConnection = async (deploymentId: string, connectionId: string) => { | |
| setExpandSyncLogs(true); | |
| if (!deploymentId) { | |
| errorToast('Deployment not created', [], globalContext); | |
| return; | |
| } | |
| try { | |
| const response = await httpPost(session, `prefect/v1/flows/${deploymentId}/flow_run/`, {}); | |
| if (response?.detail) { | |
| errorToast(response.detail, [], globalContext); | |
| return { error: 'ERROR' }; | |
| } | |
| if (!response?.flow_run_id) { | |
| errorToast('Something went wrong', [], globalContext); | |
| return { error: 'ERROR' }; | |
| } | |
| successToast('Sync initiated successfully', [], globalContext); | |
| pollForFlowRun(response.flow_run_id); | |
| mutateConnections(); | |
| } catch (err: any) { | |
| console.error(err); | |
| errorToast(err.message, [], globalContext); | |
| return { error: 'ERROR' }; | |
| } finally { | |
| setSyncingConnectionIds((prev) => prev.filter((id) => id !== connectionId)); | |
| } | |
| }; | |
| const syncConnection = async (deploymentId: string, connectionId: string) => { | |
| setExpandSyncLogs(true); | |
| if (!deploymentId) { | |
| errorToast('Deployment not created', [], globalContext); | |
| return; | |
| } | |
| try { | |
| const response = await httpPost(session, `prefect/v1/flows/${deploymentId}/flow_run/`, {}); | |
| if (response?.detail) { | |
| errorToast(response.detail, [], globalContext); | |
| return { error: 'ERROR' }; | |
| } | |
| if (!response?.flow_run_id) { | |
| errorToast('Something went wrong', [], globalContext); | |
| return { error: 'ERROR' }; | |
| } | |
| successToast('Sync initiated successfully', [], globalContext); | |
| await pollForFlowRun(response.flow_run_id); | |
| mutateConnections(); | |
| } catch (err: any) { | |
| console.error(err); | |
| errorToast(err.message, [], globalContext); | |
| return { error: 'ERROR' }; | |
| } finally { | |
| setSyncingConnectionIds((prev) => prev.filter((id) => id !== connectionId)); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@src/components/Ingest/useIngestData.ts` around lines 348 - 374, The issue is
that pollForFlowRun is invoked fire-and-forget in syncConnection, causing the
finally block to remove connectionId from syncingConnectionIds before polling
completes; to fix it, await pollForFlowRun(response.flow_run_id) (or capture its
Promise and await it) so the function only reaches the finally block after
polling finishes, and propagate/handle any errors from pollForFlowRun inside the
try/catch so syncingConnectionIds is only cleared after the poll completes;
locate syncConnection and replace the un-awaited pollForFlowRun call with an
awaited call (or await the Promise) and adjust control flow accordingly.
| const refreshConnectionSchema = async () => { | ||
| handleConnMenuClose(); | ||
| setIsRefreshing(true); | ||
| try { | ||
| const response = await httpGet( | ||
| session, | ||
| `airbyte/v1/connections/${connMenuConnectionId}/catalog` | ||
| ); | ||
| const checkRefresh = async () => { | ||
| const refreshResponse = await httpGet(session, 'tasks/stp/' + response.task_id); | ||
| if (refreshResponse.progress?.length > 0) { | ||
| const lastStatus = refreshResponse.progress[refreshResponse.progress.length - 1].status; | ||
| if (lastStatus === 'failed') { | ||
| errorToast('Failed to refresh connection', [], globalContext); | ||
| return; | ||
| } else if (lastStatus === 'completed') { | ||
| successToast('Connection refreshed successfully', [], globalContext); | ||
| mutateConnections(); | ||
| return; | ||
| } | ||
| } | ||
| await delay(2000); | ||
| await checkRefresh(); | ||
| }; | ||
| await checkRefresh(); | ||
| } catch (err: any) { | ||
| console.error(err); | ||
| errorToast('Failed to refresh connection', [], globalContext); | ||
| } finally { | ||
| setIsRefreshing(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Unbounded recursion in checkRefresh with no retry limit or max-depth guard.
Both refreshConnectionSchema (line 480) and handleRefreshConnectionDirect (line 582) define a recursive checkRefresh that calls itself indefinitely every 2 seconds if the task status is neither 'failed' nor 'completed'. If the backend returns an unexpected status or the task hangs, this will run forever — eventually exhausting the call stack or keeping the UI in a loading state indefinitely.
Additionally, these two functions are near-identical duplicates.
🛡️ Proposed fix: add a max-retries guard and extract a shared helper
Extract a shared helper and add a retry ceiling:
+const MAX_REFRESH_POLLS = 60; // ~2 min at 2s intervals
+
+const pollRefreshTask = async (
+ session: any,
+ taskId: string,
+ onSuccess: () => void,
+ onFailure: () => void,
+) => {
+ let attempts = 0;
+ const check = async () => {
+ if (attempts++ >= MAX_REFRESH_POLLS) {
+ onFailure();
+ return;
+ }
+ const resp = await httpGet(session, 'tasks/stp/' + taskId);
+ if (resp.progress?.length > 0) {
+ const lastStatus = resp.progress[resp.progress.length - 1].status;
+ if (lastStatus === 'failed') { onFailure(); return; }
+ if (lastStatus === 'completed') { onSuccess(); return; }
+ }
+ await delay(2000);
+ await check();
+ };
+ await check();
+};Then use this in both refreshConnectionSchema and handleRefreshConnectionDirect.
Also applies to: 578-605
🤖 Prompt for AI Agents
In `@src/components/Ingest/useIngestData.ts` around lines 472 - 503, The recursive
polling in refreshConnectionSchema and handleRefreshConnectionDirect uses an
unbounded checkRefresh which can recurse forever; extract the polling into a
shared helper (e.g., pollTaskStatus or waitForTaskCompletion) and replace each
local checkRefresh with a call to that helper, implementing a retry ceiling
(maxRetries or timeout) and converting recursion to an iterative loop or
controlled recursion that stops after the max attempts; ensure the helper
handles unexpected statuses by logging/errorToast, resolves on 'completed' to
call mutateConnections where needed, rejects or returns failure on 'failed' or
when maxRetries is exceeded, and preserves setIsRefreshing state handling in the
callers (refreshConnectionSchema and handleRefreshConnectionDirect).
| <Link | ||
| href={row.link} | ||
| target="_blank" | ||
| sx={{ | ||
| color: colors.primary, | ||
| textDecoration: 'none', | ||
| '&:hover': { textDecoration: 'underline' }, | ||
| }} | ||
| > | ||
| {row.value} | ||
| </Link> |
There was a problem hiding this comment.
Add rel="noopener noreferrer" to the external link.
The <Link> has target="_blank" but no rel attribute. While MUI's Link may handle this internally in some versions, explicitly adding rel="noopener noreferrer" is a best practice to prevent the opened page from accessing window.opener.
🔒 Proposed fix
<Link
href={row.link}
target="_blank"
+ rel="noopener noreferrer"
sx={{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Link | |
| href={row.link} | |
| target="_blank" | |
| sx={{ | |
| color: colors.primary, | |
| textDecoration: 'none', | |
| '&:hover': { textDecoration: 'underline' }, | |
| }} | |
| > | |
| {row.value} | |
| </Link> | |
| <Link | |
| href={row.link} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| sx={{ | |
| color: colors.primary, | |
| textDecoration: 'none', | |
| '&:hover': { textDecoration: 'underline' }, | |
| }} | |
| > | |
| {row.value} | |
| </Link> |
🤖 Prompt for AI Agents
In `@src/components/Ingest/WarehouseDetailsPanel.tsx` around lines 86 - 96, The
Link rendering the external URL (the Link element that uses href={row.link} and
target="_blank"} inside WarehouseDetailsPanel) should include rel="noopener
noreferrer" to prevent the opened page from accessing window.opener; update that
Link JSX to add the rel attribute with both values (rel="noopener noreferrer")
while keeping existing props (href, target, sx, and children).
| {warehouse.icon ? ( | ||
| <Image | ||
| src={warehouse.icon} | ||
| width={24} | ||
| height={24} | ||
| alt="warehouse icon" | ||
| style={{ objectFit: 'contain' }} | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find next.config files
fd "next\.config" --type fRepository: DalgoT4D/webapp
Length of output: 73
🏁 Script executed:
#!/bin/bash
# Check the content of next.config files if found
if [ -f next.config.js ]; then
cat next.config.js
elif [ -f next.config.ts ]; then
cat next.config.ts
elif [ -f next.config.mjs ]; then
cat next.config.mjs
fiRepository: DalgoT4D/webapp
Length of output: 2739
🏁 Script executed:
#!/bin/bash
# Examine the WarehouseHeader.tsx file around the specified lines
if [ -f "src/components/Ingest/WarehouseHeader.tsx" ]; then
wc -l "src/components/Ingest/WarehouseHeader.tsx"
echo "---"
sed -n '120,150p' "src/components/Ingest/WarehouseHeader.tsx"
fiRepository: DalgoT4D/webapp
Length of output: 964
🏁 Script executed:
#!/bin/bash
# Search for warehouse.icon definition and where it comes from
rg "warehouse\.icon|icon.*warehouse" --type ts --type tsx -A 3 -B 3Repository: DalgoT4D/webapp
Length of output: 86
🏁 Script executed:
#!/bin/bash
# Search for warehouse interface/type definition
rg "interface.*[Ww]arehouse|type.*[Ww]arehouse" -A 10 --type ts --type=pythonRepository: DalgoT4D/webapp
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Search for warehouse.icon in the codebase
rg "warehouse\.icon" -B 5 -A 5Repository: DalgoT4D/webapp
Length of output: 2729
🏁 Script executed:
#!/bin/bash
# Look for warehouse data sources and API calls
rg "warehouse.*=" --max-count=20 | head -40Repository: DalgoT4D/webapp
Length of output: 3987
🏁 Script executed:
#!/bin/bash
# Check if there are any remote icon URLs being used
rg "icon.*http|http.*icon|CDN|cdn.*icon" | head -20Repository: DalgoT4D/webapp
Length of output: 26232
Add image domain configuration to next.config.js for remote warehouse icons.
The warehouse.icon passed to the Next.js Image component contains remote URLs from Airbyte's CDN (e.g., https://connectors.airbyte.com/files/metadata/airbyte/...). Without images.remotePatterns or images.domains configured in next.config.js, Next.js will reject these remote images at runtime.
🤖 Prompt for AI Agents
In `@src/components/Ingest/WarehouseHeader.tsx` around lines 129 - 136, The
Next.js Image usage in WarehouseHeader.tsx (the <Image> rendering
warehouse.icon) will fail for Airbyte CDN URLs unless Next's image config allows
that host; update next.config.js to add the Airbyte CDN to images.domains or add
an images.remotePatterns entry that matches the Airbyte CDN path (e.g.,
connectors.airbyte.com or connecters.airbyte.com/files/metadata/*) so remote
warehouse.icon URLs are permitted, then rebuild. Ensure the change targets the
Next config export (nextConfig / module.exports) so the Image component can load
remote icons at runtime.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/TransformWorkflow/FlowEditor/Components/Nodes/__tests__/DbtSourceModelNode.test.tsx (1)
196-251:⚠️ Potential issue | 🟡 MinorTest names claim border verification but only assert text presence.
should render unpublished model nodes with dashed border,should render published model nodes with solid border, andshould render source nodes with solid border...all just check thattest_input_nameis in the document. They don't assert any border-related styling. Either rename the tests to reflect what they actually verify, or add actual style assertions.src/components/TransformWorkflow/FlowEditor/FlowEditor.tsx (1)
336-347:⚠️ Potential issue | 🟡 MinorMissing dependencies in
useEffectforcanvasAction.
handleRunWorkflow,syncSources, andfetchSourcesModelsare called inside this effect but not listed as dependencies. While the ESLint suppression on the other effect (line 333) is noted, this effect at line 336 has no suppression comment and could reference stale closures oversession,globalContext, etc.
🤖 Fix all issues with AI agents
In
`@src/components/TransformWorkflow/FlowEditor/Components/__tests__/NodeDetailModal.test.tsx`:
- Around line 68-79: The test for NodeDetailModal currently guards the click
with "if (closeButton)" which lets the test pass when the button is missing;
replace that guard with an explicit assertion that the close button exists
(e.g., assert the variable found by searching closeButtons for the element with
data-testid="CloseIcon" is defined/present) and then fireEvent.click on it and
expect defaultProps.onClose toHaveBeenCalledTimes(1); update the assertions
around the closeButton lookup in the 'calls onClose when close button is
clicked' test to fail when the element isn't found so the test actually verifies
the button is rendered and clickable.
In `@src/components/TransformWorkflow/FlowEditor/Components/Canvas.tsx`:
- Around line 383-386: Replace the bare Graph constructor and simple
setNode/setEdge calls with the same explicit constructor and node/edge payloads
used in layoutComponent: instantiate the graph with the options object (e.g. new
Dagre.graphlib.Graph({ directed: true, multigraph: false, compound: false })),
call g.setNode(n.id, n) inside nodes.forEach and g.setEdge(e.source, e.target,
e) inside edges.forEach so the library receives full node/edge payloads, and
keep components as the result of Dagre.graphlib.alg.components(g) typed as
string[][]. Ensure you mirror the exact constructor/options and payload pattern
used in layoutComponent.
🧹 Nitpick comments (14)
src/components/TransformWorkflow/FlowEditor/Components/Nodes/OperationNode.tsx (1)
100-111: Consider adding anaria-labelto the delete button.The
IconButtonhas adata-testidbut noaria-label, which makes it inaccessible to screen readers. Since the button only contains an icon with no visible text, an accessible name is needed.♿ Proposed fix
<IconButton sx={{ position: 'absolute', right: -12, top: -12, padding: '2px', zIndex: 1 }} + aria-label="Delete operation" onClick={(event) => { event.stopPropagation(); handleDeleteAction(); }} data-testid="closebutton" >src/utils/common.tsx (1)
156-165:index % 3couples lightness and saturation, limiting variety to 3 combos instead of 9.Since both arrays are indexed by the same expression (
index % 3), the (saturation, lightness) pairs are always(50,44),(42,52), or(55,58). If you want more visual spread, use independent moduli:♻️ Suggested tweak
- const lightness = [44, 52, 58][index % 3]; - const saturation = [50, 42, 55][index % 3]; + const lightness = [44, 52, 58][index % 3]; + const saturation = [50, 42, 55][(index * 7) % 3];src/components/TransformWorkflow/FlowEditor/Components/LowerSectionTabs/PreviewPane.tsx (1)
60-74: Modal-mode guard doesn't reset state when props are removed.If
propSchema/propTabletransition from defined → undefined (e.g., parent conditionally passes them), the first effect (lines 61-65) won't clearmodelToPreview, leaving stale modal data while the second effect resumes responding to context. This is fine if the component always unmounts when modal closes, but fragile if it's ever reused.Also, pagination state (
currentPageIndex,sortedColumn,sortOrder) is not reset whenmodelToPreviewchanges via props, which could show data for the new table at the old page/sort. Consider resetting pagination in the prop-driven effect.Suggested improvement
useEffect(() => { if (propSchema && propTable) { setModelToPreview({ schema: propSchema, table: propTable }); + setCurrentPageIndex(1); + setSortedColumn(undefined); + setSortOrder(1); + } else { + // Allow context-driven flow to take over + setModelToPreview(null); } }, [propSchema, propTable]);src/components/TransformWorkflow/FlowEditor/Components/LowerSectionTabs/StatisticsPane.tsx (1)
393-407: Same modal-mode pattern as PreviewPane — consider clearing state on prop removal.Same concern as in
PreviewPane: ifpropSchema/propTablebecome undefined without unmounting,modelToPreviewretains stale data. If the component always unmounts with the modal, this is fine in practice.src/components/TransformWorkflow/FlowEditor/Components/Nodes/DbtSourceModelNode.tsx (1)
87-100: Opacity transition on dimmed nodes — consider pointer-events.When
isDimmedis true (opacity 0.3), the node is still fully interactive. If the intent is to de-emphasize non-matching nodes during search/filter, consider also disabling pointer events to prevent accidental clicks.src/components/TransformWorkflow/FlowEditor/Components/ProjectTree.tsx (1)
201-201:hideHeaderis now unused.After relaxing the close-button condition to only check
onClose(line 293), thehideHeaderdestructured fromuseParentCommunication()is no longer referenced anywhere in this component. Consider removing it to keep the code clean.Suggested fix
- const { hideHeader } = useParentCommunication(); + const { } = useParentCommunication();Or remove the line entirely if no other destructured values are needed.
src/components/TransformWorkflow/FlowEditor/Components/__tests__/NodeDetailModal.test.tsx (1)
52-59: Consider tightening theschema.tableassertion.
toBeGreaterThanOrEqual(1)is lenient — if you know the expected count, assert the exact number for a more precise test.src/components/TransformWorkflow/FlowEditor/Components/NodeDetailModal.tsx (1)
33-33:initialTabchanges after mount won't take effect.
useState(initialTab)captures the value only on first render. If the modal stays mounted and is reopened with a differentinitialTab, the tab won't reset. Consider syncing viauseEffector keying the component from the parent.Option: sync with useEffect
const [selectedTab, setSelectedTab] = useState<NodeDetailTab>(initialTab); + + React.useEffect(() => { + if (open) setSelectedTab(initialTab); + }, [open, initialTab]);src/components/TransformWorkflow/FlowEditor/FlowEditor.tsx (2)
45-49: Debugconsole.logleft in production code.Line 47 logs internal state on every node addition from the project tree. Consider removing it.
Proposed fix
const handleNodeClick = (nodes: NodeApi<any>[]) => { if (nodes.length > 0 && nodes[0].isLeaf) { - console.log('adding a node to canvas from project tree component', nodes[0].data); setCanvasAction({ type: 'add-srcmodel-node', data: nodes[0].data }); } };
349-369: Height100vhmay cause issues in mobile browsers or embedded contexts.
100vhdoesn't account for mobile browser chrome or potential parent containers. If this is always used inside MUI's fullscreen Dialog (from UITransformTab), the Dialog already handles sizing. Consider using100%height with the parent providing the constraint, or100dvhfor better mobile support.src/components/TransformWorkflow/FlowEditor/Components/Canvas.tsx (4)
354-366: Extract node dimension constants to avoid magic numbers.
160and60are used both in the Dagre graph config (Lines 338-339) and in the bounding-box calculation here. Extract them as named constants (e.g.,NODE_WIDTH,NODE_HEIGHT) so they stay in sync.
1286-1291:onNodeClickis unconditionally bound — verify this is intentional.All other interaction handlers (
onPaneClick,onNodeDragStop,onNodesChange, etc.) are guarded bycanInteractWithCanvas(), butonNodeClick={handleNodeClick}is always active. If highlighting should work in locked/preview/read-only mode this is correct, but it's inconsistent with the pattern used for the other handlers.
1427-1447: No upper bound on spacing increase.The decrease button floors at
nodesep: 20, ranksep: 60, but increase has no ceiling. Rapid clicks could produce an unusably spread-out graph. Consider capping (e.g.,nodesep ≤ 300, ranksep ≤ 600).💡 Proposed cap
onClick={() => { const newSpacing = { - nodesep: layoutSpacing.nodesep + 20, - ranksep: layoutSpacing.ranksep + 40, + nodesep: Math.min(300, layoutSpacing.nodesep + 20), + ranksep: Math.min(600, layoutSpacing.ranksep + 40), }; setLayoutSpacing(newSpacing); reLayoutCanvas(layoutDirection, newSpacing); }}
460-466: State shape fornodeDetailModal— consider a discriminated union or separateopenstate.Carrying
schema,table, andnodeNameas empty strings when closed is a minor smell. Anull | { schema, table, nodeName, initialTab }pattern would make the "closed" state unambiguous and avoid passing empty strings to the modal.
| it('calls onClose when close button is clicked', () => { | ||
| render(<NodeDetailModal {...defaultProps} />); | ||
|
|
||
| const closeButtons = screen.getAllByRole('button'); | ||
| const closeButton = closeButtons.find( | ||
| (btn) => btn.querySelector('[data-testid="CloseIcon"]') !== null | ||
| ); | ||
| if (closeButton) { | ||
| fireEvent.click(closeButton); | ||
| expect(defaultProps.onClose).toHaveBeenCalledTimes(1); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Close-button test silently passes if the button isn't found.
The if (closeButton) guard means this test will pass even if the close button is missing from the DOM, defeating its purpose. Assert that the button exists before clicking.
Proposed fix
const closeButtons = screen.getAllByRole('button');
const closeButton = closeButtons.find(
(btn) => btn.querySelector('[data-testid="CloseIcon"]') !== null
);
- if (closeButton) {
- fireEvent.click(closeButton);
- expect(defaultProps.onClose).toHaveBeenCalledTimes(1);
- }
+ expect(closeButton).toBeDefined();
+ fireEvent.click(closeButton!);
+ expect(defaultProps.onClose).toHaveBeenCalledTimes(1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('calls onClose when close button is clicked', () => { | |
| render(<NodeDetailModal {...defaultProps} />); | |
| const closeButtons = screen.getAllByRole('button'); | |
| const closeButton = closeButtons.find( | |
| (btn) => btn.querySelector('[data-testid="CloseIcon"]') !== null | |
| ); | |
| if (closeButton) { | |
| fireEvent.click(closeButton); | |
| expect(defaultProps.onClose).toHaveBeenCalledTimes(1); | |
| } | |
| }); | |
| it('calls onClose when close button is clicked', () => { | |
| render(<NodeDetailModal {...defaultProps} />); | |
| const closeButtons = screen.getAllByRole('button'); | |
| const closeButton = closeButtons.find( | |
| (btn) => btn.querySelector('[data-testid="CloseIcon"]') !== null | |
| ); | |
| expect(closeButton).toBeDefined(); | |
| fireEvent.click(closeButton!); | |
| expect(defaultProps.onClose).toHaveBeenCalledTimes(1); | |
| }); |
🤖 Prompt for AI Agents
In
`@src/components/TransformWorkflow/FlowEditor/Components/__tests__/NodeDetailModal.test.tsx`
around lines 68 - 79, The test for NodeDetailModal currently guards the click
with "if (closeButton)" which lets the test pass when the button is missing;
replace that guard with an explicit assertion that the close button exists
(e.g., assert the variable found by searching closeButtons for the element with
data-testid="CloseIcon" is defined/present) and then fireEvent.click on it and
expect defaultProps.onClose toHaveBeenCalledTimes(1); update the assertions
around the closeButton lookup in the 'calls onClose when close button is
clicked' test to fail when the element isn't found so the test actually verifies
the button is rendered and clickable.
| const g = new Dagre.graphlib.Graph(); | ||
| nodes.forEach((n) => g.setNode(n.id)); | ||
| edges.forEach((e) => g.setEdge(e.source, e.target)); | ||
| const components: string[][] = Dagre.graphlib.alg.components(g); |
There was a problem hiding this comment.
Same build failure: Graph() constructor and forEach return values.
Apply the same fix here as in layoutComponent above.
🐛 Proposed fix
- const g = new Dagre.graphlib.Graph();
- nodes.forEach((n) => g.setNode(n.id));
- edges.forEach((e) => g.setEdge(e.source, e.target));
+ const g = new Dagre.graphlib.Graph({ directed: true, multigraph: false });
+ for (const n of nodes) {
+ g.setNode(n.id);
+ }
+ for (const e of edges) {
+ g.setEdge(e.source, e.target);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const g = new Dagre.graphlib.Graph(); | |
| nodes.forEach((n) => g.setNode(n.id)); | |
| edges.forEach((e) => g.setEdge(e.source, e.target)); | |
| const components: string[][] = Dagre.graphlib.alg.components(g); | |
| const g = new Dagre.graphlib.Graph({ directed: true, multigraph: false }); | |
| for (const n of nodes) { | |
| g.setNode(n.id); | |
| } | |
| for (const e of edges) { | |
| g.setEdge(e.source, e.target); | |
| } | |
| const components: string[][] = Dagre.graphlib.alg.components(g); |
🧰 Tools
🪛 Biome (2.3.13)
[error] 384-384: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
[error] 385-385: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🪛 GitHub Actions: Dalgo CI
[error] 384-384: Type error: Dagre graphlib Graph() constructor expected 2 arguments, but received 1. This occurs during Next.js build while compiling Canvas.tsx.
🤖 Prompt for AI Agents
In `@src/components/TransformWorkflow/FlowEditor/Components/Canvas.tsx` around
lines 383 - 386, Replace the bare Graph constructor and simple setNode/setEdge
calls with the same explicit constructor and node/edge payloads used in
layoutComponent: instantiate the graph with the options object (e.g. new
Dagre.graphlib.Graph({ directed: true, multigraph: false, compound: false })),
call g.setNode(n.id, n) inside nodes.forEach and g.setEdge(e.source, e.target,
e) inside edges.forEach so the library receives full node/edge payloads, and
keep components as the result of Dagre.graphlib.alg.components(g) typed as
string[][]. Ensure you mirror the exact constructor/options and payload pattern
used in layoutComponent.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/TransformWorkflow/FlowEditor/Components/Nodes/DbtSourceModelNode.tsx (1)
55-70:⚠️ Potential issue | 🟡 Minor
setCanvasNodeis called aftersetCanvasAction— the action handler may read stale node state.In
handleSelectNode,setCanvasActionwith'open-opconfig-panel'is dispatched on line 57 beforesetCanvasNodeis called on line 62. If the action handler synchronously reads the canvas node, it will see the previous node. InOperationNode, the order is reversed (node set first, then action). Consider reordering for consistency:Suggested fix
const handleSelectNode = () => { + setCanvasNode(nodeProps); + setPreviewAction({ + type: 'preview', + data: { + schema: nodeProps.data.dbtmodel?.schema || '', + table: nodeProps.data.dbtmodel?.name || '', + }, + }); if (permissions.includes('can_create_dbt_model')) { setCanvasAction({ type: 'open-opconfig-panel', data: 'create', }); } - setCanvasNode(nodeProps); - setPreviewAction({ - type: 'preview', - data: { - schema: nodeProps.data.dbtmodel?.schema || '', - table: nodeProps.data.dbtmodel?.name || '', - }, - }); };
🤖 Fix all issues with AI agents
In `@src/components/TransformWorkflow/FlowEditor/Components/Canvas.tsx`:
- Around line 1193-1214: The highlighted edge colors in the styledEdges useMemo
(using symbols styledEdges, highlightedEdgeIds, MarkerType.ArrowClosed)
currently set to teal (`#00897B`); update the highlighted stroke and markerEnd
color values to the design-specified blue (`#1976D2`) so both the stroke and arrow
marker use `#1976D2` when isHighlighted is true (leave non-highlighted colors and
transitions unchanged); if the teal was intentional, instead update the
UI4T_REDESIGN.md spec to reflect `#00897B`.
In `@UI4T_REDESIGN.md`:
- Around line 354-361: The design doc specifies blue (`#1976D2`) for node/edge
highlighting but Canvas.tsx currently uses teal (`#00897B`); update the
implementation so the highlight color matches the design or update the doc to
match the chosen color. In Canvas.tsx locate the highlight color usage (search
for the literal "#00897B" and the styled logic around isHighlighted/isDimmed,
the useMemo that generates styled nodes/edges, and any edge stroke/node border
definitions) and replace the color constant with "#1976D2" (or, if preferring
teal, change the design doc entry and the color constant reference) so the code
and UI4T_REDESIGN.md remain consistent.
🧹 Nitpick comments (12)
src/components/TransformWorkflow/FlowEditor/Components/Nodes/OperationNode.tsx (4)
76-89: Hardcoded color values could be centralized.The teal color
#00897Band its rgba variants appear here and throughoutDbtSourceModelNode.tsx. Consider extracting these into shared theme constants or theingestStyles.tsmentioned in the summary, so future palette changes are single-point edits.
36-37: Use strict equality (===) instead of loose equality (==).
edgesGoingIntoNode.length + edgesEmanatingOutOfNode.length == 0should use=== 0. Both operands are numbers so it won't cause a bug, but strict equality is the standard convention in TypeScript.Suggested fix
- edgesGoingIntoNode.length + edgesEmanatingOutOfNode.length == 0 ? false : true, + edgesGoingIntoNode.length + edgesEmanatingOutOfNode.length > 0,
2-2: Bareimport 'react'is unnecessary.With the automatic JSX runtime (used by Next.js), a side-effect-only import of React is not needed. This can be safely removed.
145-152: 9px font size may cause accessibility/readability issues on lower-DPI screens.
fontSize: '9px'is quite small. Consider whether this meets your minimum readability requirements, especially for users on standard displays.src/components/TransformWorkflow/FlowEditor/Components/Nodes/DbtSourceModelNode.tsx (2)
48-49: Same loose-equality issue asOperationNode.Suggested fix
- edgesGoingIntoNode.length + edgesEmanatingOutOfNode.length == 0 ? false : true, + edgesGoingIntoNode.length + edgesEmanatingOutOfNode.length > 0,
104-120: 8px font for the schema label is very small.Same concern as the 9px in
OperationNode— 8px text may be hard to read on standard displays. If this is intentional for the compact node design, consider at least ensuring sufficient contrast (currently#757575on presumably white/light background, which may not meet WCAG AA for text this small).src/components/TransformWorkflow/FlowEditor/Components/Canvas.tsx (5)
338-339:forEachcallbacks implicitly return values (Biome lint error).
g.setEdge()andg.setNode()return the graph instance, so theseforEacharrow functions implicitly return a value, which Biome flags as suspicious. Usefor...ofloops or add braces to discard the return value, consistent with the pattern you'd adopt for lines 377–378.♻️ Proposed fix
- componentEdges.forEach((edge: Edge) => g.setEdge(edge.source, edge.target)); - componentNodes.forEach((node: CanvasNodeRender) => g.setNode(node.id, {})); + for (const edge of componentEdges) { + g.setEdge(edge.source, edge.target); + } + for (const node of componentNodes) { + g.setNode(node.id, {}); + }
387-392: Non-null assertion onnodeMap.get(id)could cause runtime error.Line 389 uses
nodeMap.get(id)!which will beundefinedat runtime if a node ID returned byDagre.graphlib.alg.components()somehow doesn't exist innodeMap. While this should be impossible if the graph was built correctly, a defensive filter is cheap insurance.🛡️ Defensive approach
- const compNodes = nodeIds.map((id) => nodeMap.get(id)!); + const compNodes = nodeIds + .map((id) => nodeMap.get(id)) + .filter((n): n is CanvasNodeRender => n !== undefined);
1120-1154: Consider extracting BFS traversal functions out of the component.
getDownstreamPathandgetUpstreamPathare pure functions (no dependency on component state beyond theedgesparameter). Defining them inside the component body means they're re-created on every render. Moving them to module scope or a utility file improves readability and avoids unnecessary allocations.
1156-1168: Clicking a mid-graph node always highlights downstream — consider documenting this UX choice.The heuristic at line 1159 (
outgoingEdges.length > 0 → downstream, else → upstream) means a node in the middle of a chain will always show downstream, never upstream. This is a reasonable default, but users might expect shift-click or right-click to toggle direction. Consider a brief inline comment explaining the intent, or offering both via modifier keys in a future iteration.
1278-1278:onNodeClickis always active regardless ofcanInteractWithCanvas().Other interaction handlers (
onNodeDragStop,onPaneClick,onNodesChange, etc.) are guarded behindcanInteractWithCanvas(), butonNodeClickon line 1278 is unconditionally bound tohandleNodeClick. If this is intentional (allowing read-only path highlighting even in locked/preview mode), it's fine — but it differs from the guarding pattern used for every other handler. Consider adding a brief comment to document the intent.UI4T_REDESIGN.md (1)
21-91: Add language identifiers to fenced code blocks.The ASCII diagrams in fenced code blocks (lines 21, 40, 59, 69, 84) lack language identifiers, which markdownlint flags (MD040). Use
```textfor ASCII art blocks to silence the warning and improve rendering in some Markdown viewers.
| const styledEdges = useMemo(() => { | ||
| if (!hasHighlight) return edges; | ||
| return edges.map((edge) => { | ||
| const isHighlighted = highlightedEdgeIds.has(edge.id); | ||
| return { | ||
| ...edge, | ||
| type: edge.type, | ||
| style: isHighlighted | ||
| ? { stroke: '#00897B', strokeWidth: 2, transition: 'stroke 0.3s, opacity 0.3s' } | ||
| : { | ||
| stroke: '#D0D0D0', | ||
| strokeWidth: 1, | ||
| opacity: 0.15, | ||
| transition: 'stroke 0.3s, opacity 0.3s', | ||
| }, | ||
| animated: isHighlighted, | ||
| markerEnd: isHighlighted | ||
| ? { type: MarkerType.ArrowClosed, width: 16, height: 16, color: '#00897B' } | ||
| : { type: MarkerType.Arrow, width: 12, height: 12, color: '#D0D0D0' }, | ||
| }; | ||
| }); | ||
| }, [edges, highlightedEdgeIds, hasHighlight]); |
There was a problem hiding this comment.
Highlight color inconsistency with design document.
The edge highlight stroke uses #00897B (teal) on line 1201, while the design document (UI4T_REDESIGN.md, line 355) specifies #1976D2 (blue) for path highlighting. If the teal color was a deliberate departure from the spec, consider updating the design doc to match; otherwise, align the code with the spec.
🤖 Prompt for AI Agents
In `@src/components/TransformWorkflow/FlowEditor/Components/Canvas.tsx` around
lines 1193 - 1214, The highlighted edge colors in the styledEdges useMemo (using
symbols styledEdges, highlightedEdgeIds, MarkerType.ArrowClosed) currently set
to teal (`#00897B`); update the highlighted stroke and markerEnd color values to
the design-specified blue (`#1976D2`) so both the stroke and arrow marker use
`#1976D2` when isHighlighted is true (leave non-highlighted colors and transitions
unchanged); if the teal was intentional, instead update the UI4T_REDESIGN.md
spec to reflect `#00897B`.
| **1. Node path highlighting** ✅ | ||
| - Click any node with outgoing edges → highlights all downstream nodes + edges in blue (#1976D2) | ||
| - Click an end node (no outgoing edges) → highlights upstream path back to source | ||
| - Click canvas background → clears all highlights | ||
| - Highlighted nodes: blue border + blue glow shadow | ||
| - Highlighted edges: blue stroke, thicker line (2.5px), animated dashed movement | ||
| - Non-highlighted nodes/edges dim to 0.3 opacity with 0.2s transition | ||
| - Implementation: BFS graph traversal (downstream/upstream) in Canvas.tsx, `isHighlighted`/`isDimmed` flags on node data, `useMemo` for styled nodes/edges |
There was a problem hiding this comment.
Design doc specifies blue (#1976D2) for path highlighting but implementation uses teal (#00897B).
Line 355 states the highlight color is #1976D2 (blue), and lines 358–359 describe blue borders/strokes. However, Canvas.tsx line 1201 uses #00897B (teal). Please reconcile the document with the final implementation choice so future developers aren't misled.
🤖 Prompt for AI Agents
In `@UI4T_REDESIGN.md` around lines 354 - 361, The design doc specifies blue
(`#1976D2`) for node/edge highlighting but Canvas.tsx currently uses teal
(`#00897B`); update the implementation so the highlight color matches the design
or update the doc to match the chosen color. In Canvas.tsx locate the highlight
color usage (search for the literal "#00897B" and the styled logic around
isHighlighted/isDimmed, the useMemo that generates styled nodes/edges, and any
edge stroke/node border definitions) and replace the color constant with
"#1976D2" (or, if preferring teal, change the design doc entry and the color
constant reference) so the code and UI4T_REDESIGN.md remain consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/TransformWorkflow/FlowEditor/Components/Canvas.tsx`:
- Around line 326-339: The forEach callbacks using componentEdges and
componentNodes currently return values from g.setEdge(...) and g.setNode(...),
triggering the Biome lint rule; change the callbacks to use block bodies and
call g.setEdge(edge.source, edge.target); and g.setNode(node.id, {}) without
returning anything (e.g., componentEdges.forEach((edge: Edge) => {
g.setEdge(...); });). Also confirm the Dagre constructor usage by ensuring the
instantiation of new Dagre.graphlib.Graph() matches the installed `@dagrejs/dagre`
API (or pass the expected options object) for both this Canvas code and the
other getLayoutedElements call to avoid arity issues.
🧹 Nitpick comments (2)
src/components/TransformWorkflow/FlowEditor/Components/Canvas.tsx (2)
348-357: Magic numbers160/60duplicated from graph config.These node-dimension constants appear in both the graph config (lines 331–332) and the bounding-box calculation. Extract them into named constants (e.g.,
NODE_WIDTH/NODE_HEIGHT) to keep them in sync and improve readability.
1120-1154: Consider extracting BFS helpers outside the component.
getDownstreamPathandgetUpstreamPathare pure functions (no dependency on component state). Moving them to module scope (or a utility file) avoids re-creation on every render and makes them independently testable.
| const g = new Dagre.graphlib.Graph().setDefaultEdgeLabel(() => ({})); | ||
| g.setGraph({ | ||
| rankdir: direction, | ||
| nodesep: spacing.nodesep, | ||
| edgesep: 50, | ||
| width: 160, | ||
| height: 60, | ||
| marginx: 20, | ||
| marginy: 20, | ||
| ranksep: spacing.ranksep, | ||
| }); | ||
|
|
||
| componentEdges.forEach((edge: Edge) => g.setEdge(edge.source, edge.target)); | ||
| componentNodes.forEach((node: CanvasNodeRender) => g.setNode(node.id, {})); |
There was a problem hiding this comment.
Biome lint: forEach callbacks should not return a value; also verify Graph() constructor arity.
Lines 338–339 use expression-body arrows whose return values (g.setEdge(…) / g.setNode(…)) are silently discarded by forEach, triggering the Biome useIterableCallbackReturn error. Additionally, verify that new Dagre.graphlib.Graph() with zero arguments is valid for the installed @dagrejs/dagre version — the same issue was flagged on the other call site in getLayoutedElements.
Proposed fix
- const g = new Dagre.graphlib.Graph().setDefaultEdgeLabel(() => ({}));
+ const g = new Dagre.graphlib.Graph({ directed: true, multigraph: false, compound: false })
+ .setDefaultEdgeLabel(() => ({}));
g.setGraph({
rankdir: direction,
nodesep: spacing.nodesep,
edgesep: 50,
width: 160,
height: 60,
marginx: 20,
marginy: 20,
ranksep: spacing.ranksep,
});
- componentEdges.forEach((edge: Edge) => g.setEdge(edge.source, edge.target));
- componentNodes.forEach((node: CanvasNodeRender) => g.setNode(node.id, {}));
+ for (const edge of componentEdges) {
+ g.setEdge(edge.source, edge.target);
+ }
+ for (const node of componentNodes) {
+ g.setNode(node.id, {});
+ }🧰 Tools
🪛 Biome (2.3.13)
[error] 338-338: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
[error] 339-339: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In `@src/components/TransformWorkflow/FlowEditor/Components/Canvas.tsx` around
lines 326 - 339, The forEach callbacks using componentEdges and componentNodes
currently return values from g.setEdge(...) and g.setNode(...), triggering the
Biome lint rule; change the callbacks to use block bodies and call
g.setEdge(edge.source, edge.target); and g.setNode(node.id, {}) without
returning anything (e.g., componentEdges.forEach((edge: Edge) => {
g.setEdge(...); });). Also confirm the Dagre constructor usage by ensuring the
instantiation of new Dagre.graphlib.Graph() matches the installed `@dagrejs/dagre`
API (or pass the expected options object) for both this Canvas code and the
other getLayoutedElements call to avoid arity issues.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/TransformWorkflow/FlowEditor/Components/Canvas.tsx (1)
444-447:⚠️ Potential issue | 🟡 MinorRedundant initial state:
isLockedistrueregardless ofisPreviewMode.Line 444 reads
isLocked: isPreviewMode ? true : true— both branches evaluate totrue, making the ternary meaningless. This looks like an oversight from earlier iteration.🐛 Simplify
- isLocked: isPreviewMode ? true : true, // Start locked until we confirm we can acquire lock + isLocked: true, // Start locked until we confirm we can acquire lock
🧹 Nitpick comments (6)
src/components/TransformWorkflow/FlowEditor/Components/Nodes/DbtSourceModelNode.tsx (3)
33-39: Consider usingCanvasNodeTypeEnuminstead of a string literal.Line 35 compares
nodeProps.typeagainst the string'model'. For consistency and refactor-safety, prefer using theCanvasNodeTypeEnum.Modelenum value already imported inCanvas.tsx.♻️ Suggested change
+import { CanvasNodeTypeEnum } from '@/types/transform-v2.types'; ... - const isUnpublished = nodeProps.type === 'model' && nodeProps.data.isPublished === false; + const isUnpublished = nodeProps.type === CanvasNodeTypeEnum.Model && nodeProps.data.isPublished === false;
87-101: Handletop: '60%'may need tuning if the node height varies.Both left and right
Handleelements use a fixedtop: '60%'. With the schema badge addingpaddingTop: '10px'to the outer box, the effective center of the visible node body is shifted. This should be visually verified, especially when the schema label is absent (empty string), as the handles may appear misaligned.
128-139: Hardcodedwidth: '160px'is in sync withCanvas.tsxbounding-box calculation (line 355).Good that these values match. Consider extracting the node width as a shared constant to prevent them from drifting apart.
src/components/TransformWorkflow/FlowEditor/Components/Canvas.tsx (3)
386-392: Non-null assertion onnodeMap.get(id)!(line 389).Since
nodeMapis built from the samenodesarray andcomponentsis derived from node IDs set on the graph, this assertion is safe in practice. However, a defensive guard (or.filter(Boolean)) would prevent a silent crash if the data ever becomes inconsistent.♻️ Defensive alternative
- const compNodes = nodeIds.map((id) => nodeMap.get(id)!); + const compNodes = nodeIds.map((id) => nodeMap.get(id)).filter((n): n is CanvasNodeRender => !!n);
1120-1154: BFS traversal helpers are correct but re-created on every render.
getDownstreamPathandgetUpstreamPathare pure functions that only depend on their arguments — they don't reference any component state or props. Extracting them outside the component (or wrapping inuseCallback) avoids unnecessary re-creation and keeps the component body leaner.♻️ Extract to module-level functions
+// Graph traversal: BFS downstream (source→target direction) +const getDownstreamPath = (startNodeId: string, allEdges: Edge[]) => { + const visitedNodes = new Set<string>([startNodeId]); + const visitedEdges = new Set<string>(); + const queue = [startNodeId]; + while (queue.length > 0) { + const current = queue.shift()!; + for (const edge of allEdges) { + if (edge.source === current && !visitedNodes.has(edge.target)) { + visitedNodes.add(edge.target); + visitedEdges.add(edge.id); + queue.push(edge.target); + } + } + } + return { nodeIds: visitedNodes, edgeIds: visitedEdges }; +}; + +// Graph traversal: BFS upstream (target→source direction) +const getUpstreamPath = (startNodeId: string, allEdges: Edge[]) => { + const visitedNodes = new Set<string>([startNodeId]); + const visitedEdges = new Set<string>(); + const queue = [startNodeId]; + while (queue.length > 0) { + const current = queue.shift()!; + for (const edge of allEdges) { + if (edge.target === current && !visitedNodes.has(edge.source)) { + visitedNodes.add(edge.source); + visitedEdges.add(edge.id); + queue.push(edge.source); + } + } + } + return { nodeIds: visitedNodes, edgeIds: visitedEdges }; +}; + const Canvas = ({ ... }: CanvasProps) => { ... - // Graph traversal: BFS downstream ... - const getDownstreamPath = ... - // Graph traversal: BFS upstream ... - const getUpstreamPath = ...
884-893: Verboseconsole.logstatements in event handlers.Lines 885–886, 891, 896, 916, and several others contain debug logging that will appear in production. Consider removing or gating these behind a debug flag.
1 participant
Summary by CodeRabbit
New Features
Style
Tests
Utilities