feat(studio): DataDesigner details profiler results#428
Open
steramae-nvidia wants to merge 1 commit into
Open
Conversation
Signed-off-by: Sean Teramae <steramae@nvidia.com>
📝 WalkthroughWalkthroughAdds a dataset profiler results section, a builder config side panel, and a categorical histogram chart to the Data Designer job details page. Introduces three shared hooks for job routing, artifacts fileset resolution, and analysis loading. Refactors the route and fileset section to consume the new hooks. Also updates ChangesData Designer Job Details: Profiler, Config Panel, and Shared Hooks
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
web/packages/studio/src/routes/DataDesignerJobDetailsRoute/builderConfig.ts (1)
17-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark summary interface fields as readonly.
These interfaces represent parsed summary snapshots and should be immutable at the type level.
Proposed diff
export interface BuilderConfigColumnSummary { - name: string; - type: string; - modelAlias?: string; + readonly name: string; + readonly type: string; + readonly modelAlias?: string; } export interface BuilderConfigModelSummary { - alias: string; - model: string; - provider?: string; + readonly alias: string; + readonly model: string; + readonly provider?: string; } export interface BuilderConfigSeedSummary { - type: string; - samplingStrategy?: string; + readonly type: string; + readonly samplingStrategy?: string; } export interface BuilderConfigSummary { - columnCount: number; - columns: BuilderConfigColumnSummary[]; - columnTypeBreakdown: Array<{ type: string; count: number }>; - models: BuilderConfigModelSummary[]; - seed?: BuilderConfigSeedSummary; - constraintCount: number; - profilerCount: number; - processorNames: string[]; - libraryVersion?: string; + readonly columnCount: number; + readonly columns: BuilderConfigColumnSummary[]; + readonly columnTypeBreakdown: Array<{ type: string; count: number }>; + readonly models: BuilderConfigModelSummary[]; + readonly seed?: BuilderConfigSeedSummary; + readonly constraintCount: number; + readonly profilerCount: number; + readonly processorNames: string[]; + readonly libraryVersion?: string; }As per coding guidelines:
web/**/*.{ts,tsx}: Use \readonly` for immutable properties in TypeScript interfaces and types`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/builderConfig.ts` around lines 17 - 44, Add the readonly keyword to all property declarations in the four summary interfaces (BuilderConfigColumnSummary, BuilderConfigModelSummary, BuilderConfigSeedSummary, and BuilderConfigSummary) to mark them as immutable at the type level. For each property in these interfaces, prefix the property name with the readonly keyword to ensure the data structures represent immutable snapshots that cannot be modified after creation.Source: Coding guidelines
web/packages/studio/src/routes/DataDesignerJobDetailsRoute/DataDesignerConfigPanel.tsx (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
FCas type-only.
FCis only used in type positions here.Proposed diff
-import { FC, useMemo } from 'react'; +import { useMemo, type FC } from 'react';As per coding guidelines:
web/**/*.{ts,tsx}: Use \import type` for type-only imports in TypeScript`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/DataDesignerConfigPanel.tsx` at line 14, The import statement at the top of DataDesignerConfigPanel.tsx imports both FC and useMemo from 'react' using a single import statement. Since FC is only used in type positions (as a functional component type), it should be imported using import type syntax instead of regular import. Split the import statement so that FC is imported with import type while useMemo remains in a regular import statement.Source: Coding guidelines
web/packages/studio/src/routes/DataDesignerJobDetailsRoute/JobOutputFilesetSection.tsx (1)
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConvert type-only React imports to
import type.
ComponentPropsandFCare type-only and should useimport typein this TSX file.Proposed fix
-import { ComponentProps, FC, useCallback, useEffect, useMemo, useState } from 'react'; +import { useCallback, useEffect, useMemo, useState } from 'react'; +import type { ComponentProps, FC } from 'react';As per coding guidelines,
web/**/*.{ts,tsx}: Useimport typefor type-only imports in TypeScript.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/JobOutputFilesetSection.tsx` at line 20, In the import statement at the top of JobOutputFilesetSection.tsx, separate the type-only imports from the runtime imports. Move ComponentProps and FC to a new line using import type syntax, keeping useCallback, useEffect, useMemo, and useState in the regular import statement from React. This ensures type-only imports are clearly distinguished from runtime dependencies according to the project's TypeScript guidelines.Source: Coding guidelines
web/packages/studio/src/routes/DataDesignerJobDetailsRoute/index.tsx (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
import typeforFCin this TSX module.
FCis type-only and should be imported withimport typeto match the TypeScript guideline.Proposed fix
-import { FC, useState } from 'react'; +import { useState } from 'react'; +import type { FC } from 'react';As per coding guidelines,
web/**/*.{ts,tsx}: Useimport typefor type-only imports in TypeScript.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/index.tsx` at line 25, The import statement at the top of the DataDesignerJobDetailsRoute file imports FC as a type-only import along with useState in a single import statement. Separate this into two imports: create a type-only import for FC using import type, and keep a regular import for useState from react. This ensures that type-only imports are explicitly marked as such, following TypeScript guidelines that help with tree-shaking and code clarity.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/DataDesignerConfigPanel.tsx`:
- Around line 59-63: The key prop for the mapped KVPair components uses only
model.alias and column.name respectively, which can collide when duplicate
aliases or names exist (such as multiple "(unnamed)" entries), causing React to
mis-reconcile elements. Replace the key prop in both the model mapping (around
line 59-63 in the summary.models.map) and the column mapping (around line 75-79)
with a stable unique identifier that includes both the alias/name and the array
index, or another property that ensures uniqueness across duplicates, to prevent
reconciliation issues.
In
`@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/DatasetProfilerSection.tsx`:
- Around line 57-67: The loading state condition in the first if statement is
checking hasAnalysis instead of the actual analysis data, which causes the
empty-state message to appear when hasAnalysis is true but the data is still
loading. Change the condition from checking !hasAnalysis to checking !analysis
(or checking if analysis is falsy/undefined) to properly gate the skeleton
loading state. This ensures the skeleton loader displays whenever data is
actually still loading, regardless of the hasAnalysis flag status. The fix
applies to the loading branch condition at the beginning of this component
logic.
In
`@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/datasetProfilerTypes.ts`:
- Around line 148-153: The getPercentComplete function can return values above
100 or below 0 due to backend count drift, which breaks progress rendering in
consumers expecting valid percentage values. Clamp the calculated percentage
result to the range [0, 100] by wrapping the return statement with Math.max(0,
Math.min(100, ...)) to ensure the function always returns a value between 0 and
100 inclusive.
- Around line 136-137: The type predicate isLLMColumnStatistics narrows to only
LLMTextColumnStatistics, but the runtime check using LLM_COLUMN_TYPES actually
validates against four different LLM column types (llm-text, llm-code,
llm-structured, and llm-judge). Create a union type that includes all four LLM
column statistic variants, then update the return type of the
isLLMColumnStatistics predicate to use this union type instead of
LLMTextColumnStatistics to align the type guard with the actual runtime
behavior.
In
`@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/useDataDesignerArtifactsFileset.ts`:
- Around line 26-29: The useDataDesignerListCreateJobResults hook call is not
capturing error information from its response. Currently when the hook fails,
it's being treated identically to the case where there are simply no results,
causing incorrect error messages to display downstream. Extract the error state
that the hook returns (in addition to the data and isLoading states already
being destructured) and ensure this error state is used by downstream code to
properly render an error state instead of conflating it with empty results.
Apply this same fix to all usages of useDataDesignerListCreateJobResults
including the location at lines 74-89.
In
`@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/useDataDesignerJobAnalysis.ts`:
- Around line 33-36: The useDataDesignerJobAnalysis hook is not capturing errors
from the useDataDesignerListCreateJobResults query call. When the results
listing fails, the hook currently reports hasAnalysis as false and isError as
false, misclassifying backend failures as missing analysis. Extract the error
state from the useDataDesignerListCreateJobResults destructuring (similar to how
data and isLoading are extracted) and incorporate it into the hook's return
value so that actual backend errors are properly surfaced to consumers rather
than being silently dropped.
---
Nitpick comments:
In `@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/builderConfig.ts`:
- Around line 17-44: Add the readonly keyword to all property declarations in
the four summary interfaces (BuilderConfigColumnSummary,
BuilderConfigModelSummary, BuilderConfigSeedSummary, and BuilderConfigSummary)
to mark them as immutable at the type level. For each property in these
interfaces, prefix the property name with the readonly keyword to ensure the
data structures represent immutable snapshots that cannot be modified after
creation.
In
`@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/DataDesignerConfigPanel.tsx`:
- Line 14: The import statement at the top of DataDesignerConfigPanel.tsx
imports both FC and useMemo from 'react' using a single import statement. Since
FC is only used in type positions (as a functional component type), it should be
imported using import type syntax instead of regular import. Split the import
statement so that FC is imported with import type while useMemo remains in a
regular import statement.
In `@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/index.tsx`:
- Line 25: The import statement at the top of the DataDesignerJobDetailsRoute
file imports FC as a type-only import along with useState in a single import
statement. Separate this into two imports: create a type-only import for FC
using import type, and keep a regular import for useState from react. This
ensures that type-only imports are explicitly marked as such, following
TypeScript guidelines that help with tree-shaking and code clarity.
In
`@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/JobOutputFilesetSection.tsx`:
- Line 20: In the import statement at the top of JobOutputFilesetSection.tsx,
separate the type-only imports from the runtime imports. Move ComponentProps and
FC to a new line using import type syntax, keeping useCallback, useEffect,
useMemo, and useState in the regular import statement from React. This ensures
type-only imports are clearly distinguished from runtime dependencies according
to the project's TypeScript guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 63a66446-24f0-4fc3-a33c-ef9c32f8b383
📒 Files selected for processing (14)
web/packages/studio/src/constants/constants.tsweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/CategoricalHistogramChart.tsxweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/ColumnProfileCard.tsxweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/DataDesignerConfigPanel.tsxweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/DatasetProfilerSection.tsxweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/JobOutputFilesetSection.tsxweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/builderConfig.test.tsweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/builderConfig.tsweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/datasetProfilerTypes.test.tsweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/datasetProfilerTypes.tsweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/index.tsxweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/useDataDesignerArtifactsFileset.tsweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/useDataDesignerJobAnalysis.tsweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/useDataDesignerJobFromRoute.ts
Comment on lines
+59
to
+63
| {summary.models.map((model) => ( | ||
| <KVPair | ||
| key={model.alias} | ||
| label={model.alias} | ||
| value={model.provider ? `${model.model} (${model.provider})` : model.model} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use stable unique keys for mapped KV pairs.
key={model.alias} / key={column.name} collides on duplicate aliases/names (e.g., (unnamed) fallbacks), which can mis-reconcile rows.
Proposed diff
- {summary.models.map((model) => (
+ {summary.models.map((model, index) => (
<KVPair
- key={model.alias}
+ key={`${model.alias}-${index}`}
label={model.alias}
value={model.provider ? `${model.model} (${model.provider})` : model.model}
/>
))}
@@
- {summary.columns.map((column) => (
+ {summary.columns.map((column, index) => (
<KVPair
- key={column.name}
+ key={`${column.name}-${index}`}
label={column.name}
value={column.modelAlias ? `${column.type} · ${column.modelAlias}` : column.type}
/>
))}Also applies to: 75-79
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/DataDesignerConfigPanel.tsx`
around lines 59 - 63, The key prop for the mapped KVPair components uses only
model.alias and column.name respectively, which can collide when duplicate
aliases or names exist (such as multiple "(unnamed)" entries), causing React to
mis-reconcile elements. Replace the key prop in both the model mapping (around
line 59-63 in the summary.models.map) and the column mapping (around line 75-79)
with a stable unique identifier that includes both the alias/name and the array
index, or another property that ensures uniqueness across duplicates, to prevent
reconciliation issues.
Comment on lines
+57
to
+67
| if (isLoading && !hasAnalysis) { | ||
| return ( | ||
| <ColumnGrid> | ||
| {Array.from({ length: 6 }, (_, i) => ( | ||
| <Skeleton key={i} className="h-40 w-full rounded-lg" /> | ||
| ))} | ||
| </ColumnGrid> | ||
| ); | ||
| } | ||
|
|
||
| if (!hasAnalysis) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix loading gate to avoid false “no column statistics” state.
With hasAnalysis === true and download still in flight, this path skips skeleton and renders the empty-state message. Gate on analysis instead of hasAnalysis for the loading branch.
Proposed fix
- if (isLoading && !hasAnalysis) {
+ if (isLoading && !analysis) {
return (
<ColumnGrid>
{Array.from({ length: 6 }, (_, i) => (
<Skeleton key={i} className="h-40 w-full rounded-lg" />
))}
</ColumnGrid>
);
}Also applies to: 75-108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/DatasetProfilerSection.tsx`
around lines 57 - 67, The loading state condition in the first if statement is
checking hasAnalysis instead of the actual analysis data, which causes the
empty-state message to appear when hasAnalysis is true but the data is still
loading. Change the condition from checking !hasAnalysis to checking !analysis
(or checking if analysis is falsy/undefined) to properly gate the skeleton
loading state. This ensures the skeleton loader displays whenever data is
actually still loading, regardless of the hasAnalysis flag status. The fix
applies to the loading branch condition at the beginning of this component
logic.
Comment on lines
+136
to
+137
| export const isLLMColumnStatistics = (stats: ColumnStatistics): stats is LLMTextColumnStatistics => | ||
| LLM_COLUMN_TYPES.has(stats.column_type); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and explore its structure
fd -t f "datasetProfilerTypes.ts" web/packages/studio/src/Repository: NVIDIA-NeMo/nemo-platform
Length of output: 246
🏁 Script executed:
# Read the entire file to understand context
cat -n web/packages/studio/src/routes/DataDesignerJobDetailsRoute/datasetProfilerTypes.tsRepository: NVIDIA-NeMo/nemo-platform
Length of output: 11846
🏁 Script executed:
# Search for LLM_COLUMN_TYPES definition
rg "LLM_COLUMN_TYPES" web/packages/studio/src/routes/DataDesignerJobDetailsRoute/Repository: NVIDIA-NeMo/nemo-platform
Length of output: 463
Type predicate must narrow to union of all LLM variants.
Line 136 declares narrowing to LLMTextColumnStatistics, but line 137's runtime check (LLM_COLUMN_TYPES) accepts 'llm-text', 'llm-code', 'llm-structured', and 'llm-judge'. This breaks the type guard contract—TypeScript narrows to the wrong type.
Create a union type for all four LLM variants and update the predicate return type:
Fix
+export type LLMColumnStatistics =
+ | LLMTextColumnStatistics
+ | LLMCodeColumnStatistics
+ | LLMStructuredColumnStatistics
+ | LLMJudgedColumnStatistics;
+
-export const isLLMColumnStatistics = (stats: ColumnStatistics): stats is LLMTextColumnStatistics =>
+export const isLLMColumnStatistics = (stats: ColumnStatistics): stats is LLMColumnStatistics =>
LLM_COLUMN_TYPES.has(stats.column_type);📝 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.
Suggested change
| export const isLLMColumnStatistics = (stats: ColumnStatistics): stats is LLMTextColumnStatistics => | |
| LLM_COLUMN_TYPES.has(stats.column_type); | |
| export type LLMColumnStatistics = | |
| | LLMTextColumnStatistics | |
| | LLMCodeColumnStatistics | |
| | LLMStructuredColumnStatistics | |
| | LLMJudgedColumnStatistics; | |
| export const isLLMColumnStatistics = (stats: ColumnStatistics): stats is LLMColumnStatistics => | |
| LLM_COLUMN_TYPES.has(stats.column_type); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/datasetProfilerTypes.ts`
around lines 136 - 137, The type predicate isLLMColumnStatistics narrows to only
LLMTextColumnStatistics, but the runtime check using LLM_COLUMN_TYPES actually
validates against four different LLM column types (llm-text, llm-code,
llm-structured, and llm-judge). Create a union type that includes all four LLM
column statistic variants, then update the return type of the
isLLMColumnStatistics predicate to use this union type instead of
LLMTextColumnStatistics to align the type guard with the actual runtime
behavior.
Comment on lines
+148
to
+153
| export const getPercentComplete = (results: DatasetProfilerResults): number => { | ||
| if (results.target_num_records <= 0) { | ||
| return 0; | ||
| } | ||
| return (100 * results.num_records) / results.target_num_records; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clamp completion percentage to [0, 100].
Line 152 can return values above 100 (or below 0) when backend counts drift, which can break determinate progress rendering in consumers.
Proposed fix
export const getPercentComplete = (results: DatasetProfilerResults): number => {
if (results.target_num_records <= 0) {
return 0;
}
- return (100 * results.num_records) / results.target_num_records;
+ const percent = (100 * results.num_records) / results.target_num_records;
+ return Math.max(0, Math.min(100, percent));
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/datasetProfilerTypes.ts`
around lines 148 - 153, The getPercentComplete function can return values above
100 or below 0 due to backend count drift, which breaks progress rendering in
consumers expecting valid percentage values. Clamp the calculated percentage
result to the range [0, 100] by wrapping the return statement with Math.max(0,
Math.min(100, ...)) to ensure the function always returns a value between 0 and
100 inclusive.
Comment on lines
+26
to
+29
| const { data: resultsResponse, isLoading: isResultsLoading } = | ||
| useDataDesignerListCreateJobResults(workspace, jobName, { | ||
| query: { refetchInterval: isTerminal ? false : 3000 }, | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Surface results-list errors from the hook.
A failed useDataDesignerListCreateJobResults call is currently collapsed into the same state as “no artifacts result”, so downstream renders the wrong message instead of an error state.
Proposed fix
- const { data: resultsResponse, isLoading: isResultsLoading } =
+ const {
+ data: resultsResponse,
+ isLoading: isResultsLoading,
+ isError: isResultsError,
+ error: resultsError,
+ } =
useDataDesignerListCreateJobResults(workspace, jobName, {
query: { refetchInterval: isTerminal ? false : 3000 },
});
...
return {
workspace,
jobName,
job,
isTerminal,
artifactsResult,
filesetLoc,
filesetWorkspace,
filesetName,
listFilesParams,
files,
isResultsLoading,
+ isResultsError,
+ resultsError,
isFilesLoading,
isFilesError,
filesError,
};Also applies to: 74-89
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/useDataDesignerArtifactsFileset.ts`
around lines 26 - 29, The useDataDesignerListCreateJobResults hook call is not
capturing error information from its response. Currently when the hook fails,
it's being treated identically to the case where there are simply no results,
causing incorrect error messages to display downstream. Extract the error state
that the hook returns (in addition to the data and isLoading states already
being destructured) and ensure this error state is used by downstream code to
properly render an error state instead of conflating it with empty results.
Apply this same fix to all usages of useDataDesignerListCreateJobResults
including the location at lines 74-89.
Comment on lines
+33
to
+36
| const { data: resultsResponse, isLoading: isResultsLoading } = | ||
| useDataDesignerListCreateJobResults(workspace, jobName, { | ||
| query: { enabled: enabled && Boolean(workspace && jobName) }, | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not drop results-list errors in this hook.
When listing results fails, this hook currently reports hasAnalysis: false and isError: false, which misclassifies backend failures as “no analysis generated”.
Proposed fix
- const { data: resultsResponse, isLoading: isResultsLoading } =
+ const {
+ data: resultsResponse,
+ isLoading: isResultsLoading,
+ isError: isResultsError,
+ error: resultsError,
+ } =
useDataDesignerListCreateJobResults(workspace, jobName, {
query: { enabled: enabled && Boolean(workspace && jobName) },
});
...
return {
analysis: analysisQuery.data,
hasAnalysis,
isLoading: isResultsLoading || (hasAnalysis && analysisQuery.isLoading),
- isError: analysisQuery.isError,
- error: analysisQuery.error,
+ isError: isResultsError || analysisQuery.isError,
+ error: resultsError ?? analysisQuery.error,
};Also applies to: 61-67
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/useDataDesignerJobAnalysis.ts`
around lines 33 - 36, The useDataDesignerJobAnalysis hook is not capturing
errors from the useDataDesignerListCreateJobResults query call. When the results
listing fails, the hook currently reports hasAnalysis as false and isError as
false, misclassifying backend failures as missing analysis. Extract the error
state from the useDataDesignerListCreateJobResults destructuring (similar to how
data and isLoading are extracted) and incorporate it into the hook's return
value so that actual backend errors are properly surfaced to consumers rather
than being silently dropped.
Contributor
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes