Deployment Playground #861
Conversation
Cahllagerfeld
force-pushed
the
feat/859--playground
branch
from
b8eb1c3 to
e2ec4e6
Compare
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
| "elkjs": "^0.10.0", | ||
| "featureos-widget": "^0.0.32", | ||
| "immer": "^10.1.1", | ||
| "json-schema-faker": "^0.5.9", |
There was a problem hiding this comment.
Im not so sure about this lib
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a new Deployment Playground feature with inputs, outputs, and preview visualization, including Monaco JSON editing and invocation flow. Updates routing to expose the playground, adjusts deployment page layout, introduces invocation types and mutation, configures Monaco workers, and tweaks build chunking. Minor error-handling improvements and a small UI prop addition included. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Router
participant DeploymentPage as Deployment Playground Page
participant Queries as TanStack Query
participant API
User->>Router: Navigate to /deployments/:id/playground
Router->>DeploymentPage: Render
DeploymentPage->>Queries: fetchDeploymentDetail(id)
Queries-->>DeploymentPage: deployment (incl. snapshot_id)
DeploymentPage->>Queries: fetchSnapshotDetail(snapshot_id) [enabled if id]
Queries-->>DeploymentPage: snapshot
DeploymentPage-->>User: Render Inputs + Outputs
sequenceDiagram
autonumber
actor User
participant Inputs as PlaygroundInputs
participant Form as useInvokeForm
participant Invoke as useInvokeDeployment
participant API
participant Outputs as PlaygroundOutputs
User->>Inputs: Click "Run"
Inputs->>Form: handleSubmit()
Form->>Invoke: mutate({ url, authKey, payload:{ parameters } })
Invoke->>API: POST /.../invoke
API-->>Invoke: JSON { success, outputs|error, metadata, execution_time }
Invoke-->>Outputs: onSuccess(data)
Outputs-->>User: Preview or JSON view, run card, actions
User->>Outputs: Delete run (optional)
Outputs->>API: DELETE /runs/:id (via useDeleteRun)
API-->>Outputs: 204/OK
Outputs-->>User: Cleared state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/data/pipeline-snapshots/fetch-snapshot-detail.ts (1)
23-34: Extract duplicated error handling to a shared utility.This error extraction pattern appears in both
fetch-snapshot-detail.tsandfetch-detail.ts. Consider extracting it to a shared utility function to reduce duplication and ensure consistent error handling across all fetch functions.Example shared utility:
// src/lib/extract-error-message.ts export function extractErrorMessage(data: unknown, fallbackMessage: string): string { if (Array.isArray(data)) { return data[1] || data[0] || fallbackMessage; } if (typeof data === 'object' && data !== null && 'detail' in data) { const detail = (data as { detail: unknown }).detail; if (Array.isArray(detail)) { return detail[1] || detail[0] || fallbackMessage; } return String(detail); } return fallbackMessage; }Then use it in both files:
const errorData: string = await res .json() - .then((data) => { - if (Array.isArray(data)) { - return data[1]; - } - if (Array.isArray(data.detail)) { - return data.detail[1]; - } - return data.detail; - }) + .then((data) => extractErrorMessage(data, `Error while fetching pipeline snapshot ${snapshotId}`)) .catch(() => `Error while fetching pipeline snapshot ${snapshotId}`);
♻️ Duplicate comments (1)
package.json (1)
28-28: Ensure heavy libs are lazy-loaded and split
monaco-editorandjson-schema-fakercan bloat the main bundle. Load them on demand and keep them in separate chunks.Run to confirm dynamic imports aren’t statically bundled:
#!/bin/bash # Check static vs dynamic imports for monaco and json-schema-faker rg -nP -C2 --type=ts --type=tsx '\bfrom\s+[\"'\"']monaco-editor[\"'\"']|\bimport\(\s*[\"'\"']monaco-editor[\"'\"']\s*\)' rg -nP -C2 --type=ts --type=tsx '\bfrom\s+[\"'\"']@monaco-editor/react[\"'\"']' rg -nP -C2 --type=ts --type=tsx '\bfrom\s+[\"'\"']json-schema-faker[\"'\"']|\bimport\(\s*[\"'\"']json-schema-faker[\"'\"']\s*\)'If you see static
from "monaco-editor"orfrom "json-schema-faker"in app code, switch to dynamicimport()or ensure they live behind route-level code-splitting.Also applies to: 41-41, 44-44
🧹 Nitpick comments (21)
vite.config.ts (1)
31-42: Consider adding documentation for chunk splitting strategy.The chunk splitting logic is clear, but adding comments explaining the rationale (e.g., separating heavy dependencies, grouping related libraries) would help future maintainers understand the bundling strategy.
Example:
function splitChunks(id: string) { + // Schema validation libraries if (id.includes("zod")) return "@zod"; + + // Third-party widgets if (id.includes("featureos-widget")) return "@featureos-widget"; + + // Heavy utility libraries if (id.includes("json-schema-faker")) return "@json-schema-faker"; if (id.includes("monaco-editor")) return "@monaco-editor"; + + // Graph/visualization libraries if (id.includes("reactflow")) return "@reactflow"; if (id.includes("radix") || id.includes("elkjs")) return "@radix";src/components/collapsible-chevron.tsx (1)
12-17: Simplify className construction.The template literal syntax is unnecessarily complex. Consider simplifying the className construction for better readability.
Apply this diff:
<ChevronDown className={cn( - ` ${ - open ? "" : "-rotate-90" - } size-5 shrink-0 rounded-md fill-neutral-500 transition-transform duration-200 hover:bg-neutral-200`, + "size-5 shrink-0 rounded-md fill-neutral-500 transition-transform duration-200 hover:bg-neutral-200", + !open && "-rotate-90", className )} />src/components/deployments/playground/preview-visualization/components/boolean-value.tsx (1)
1-17: LGTM! Good accessibility implementation.The component properly handles boolean visualization with accessible screen reader text. The sr-only span is a good practice.
Optional: The
value === truecheck on line 8 could be simplified to justvaluesince it's typed as boolean:-{value === true ? ( +{value ? (src/app/deployments/[deploymentId]/playground/_components/outputs/run-card-avatar.tsx (1)
13-16: Returnnullinstead of empty divs.Returning empty
<div></div>elements adds unnecessary DOM nodes. When there's nothing to render, returnnullinstead.Apply this diff:
-if (runQuery.isPending) return <Skeleton className="h-5 w-[100px]" />; -if (runQuery.isError) return <div></div>; +if (runQuery.isPending) { + return <Skeleton className="h-5 w-[100px]" />; +} +if (runQuery.isError) { + return null; +} const user = runQuery.data?.resources?.user; -if (!user) return <div></div>; +if (!user) { + return null; +}src/app/deployments/[deploymentId]/playground/_components/outputs/outputs-switcher.tsx (1)
17-17: Avoid unsafe cast on setActiveViewGuard the value instead of casting.
- setActiveView={(val) => setActiveView(val as PlaygroundOutputsView)} + setActiveView={(val) => { + if (val === "preview" || val === "json") setActiveView(val); + }}As per coding guidelines
src/app/deployments/[deploymentId]/playground/_components/outputs/delete-alert.tsx (2)
45-47: Prevent double actions while mutatingDisable the trigger while
isPending.- <Button type="button" intent="danger" emphasis="subtle" size="md"> + <Button type="button" intent="danger" emphasis="subtle" size="md" disabled={isPending}> Delete </Button>As per coding guidelines
24-24: Scope query invalidationIf your cache uses specific keys (e.g.,
["runs", { runId }]), invalidate more narrowly to reduce cache churn.As per coding guidelines
src/components/deployments/playground/preview-visualization/components/collapsible-string-value.tsx (2)
5-5: Use PascalCase for component namesRename to
CollapsibleValuefor consistency and to avoid import mistakes.-export function Collapsiblevalue({ value }: { value: string }) { +export function CollapsibleValue({ value }: { value: string }) {As per coding guidelines
1-3: Improve accessibility and clean classes
- Add
useId,aria-expanded, andaria-controls.- Remove stray
from-bg-theme-surface-primary.- Use
type="button"to avoid unintended form submits.-import { useState } from "react"; +import { useId, useState } from "react"; @@ - const [isExpanded, setIsExpanded] = useState(false); + const [isExpanded, setIsExpanded] = useState(false); + const contentId = useId(); @@ - <div - className={`whitespace-pre-wrap ${shouldCollapse ? "max-h-[200px] overflow-hidden" : ""}`} - > + <div + id={contentId} + className={`whitespace-pre-wrap ${shouldCollapse ? "max-h-[200px] overflow-hidden" : ""}`} + > {value} </div> @@ - {shouldCollapse && ( - <div className="from-bg-theme-surface-primary pointer-events-none absolute bottom-0 left-0 right-0 h-[100px] bg-gradient-to-t from-theme-surface-primary to-transparent" /> - )} + {shouldCollapse && ( + <div className="pointer-events-none absolute bottom-0 left-0 right-0 h-[100px] bg-gradient-to-t from-theme-surface-primary to-transparent" /> + )} @@ - <button - onClick={() => setIsExpanded(!isExpanded)} - className="flex items-center gap-1 text-primary-400" - > - <ExpandCollapseLineIcon expanded={isExpanded} /> + <button + type="button" + aria-expanded={isExpanded} + aria-controls={contentId} + onClick={() => setIsExpanded(!isExpanded)} + className="flex items-center gap-1 text-primary-400" + > + <ExpandCollapseLineIcon aria-hidden="true" expanded={isExpanded} /> {isExpanded ? "Show less" : "Show more"} </button>As per coding guidelines
Also applies to: 6-6, 15-18, 21-23, 27-33
package.json (1)
58-58: Watch for Zod 3.24.x type regressions3.24.x had reported TS type issues in some setups; pin to a known-good patch if you hit typing perf/instantiation problems. Based on learnings
src/app/deployments/[deploymentId]/playground/page.content.tsx (2)
27-29: Remove console.log in success handlerAvoid shipping logs; use telemetry or UI updates instead.
- onSuccess: (data) => { - console.log(data); - } + onSuccess: () => { + // handle success UI update here + }
32-47: Stabilize submit callback to reduce re-rendersWrap
submitDeploymentinuseCallbacksoPlaygroundInputsdoesn’t re-render unnecessarily when parent renders.-export function PlaygroundPageContent({ snapshot, deployment }: Props) { +export function PlaygroundPageContent({ snapshot, deployment }: Props) { @@ - function submitDeployment(data: unknown) { + const submitDeployment = useCallback((data: unknown) => { @@ - } + }, [deployment?.metadata?.auth_key, deployment?.body?.url, invokeDeployment]);As per coding guidelines
Also applies to: 51-55
src/app/deployments/[deploymentId]/playground/page.tsx (1)
19-22: Avoid non-null assertion and gate snapshot query more tightly
- Passing snapshotId! to detail risks eager URL/key construction with undefined.
- Also skip fetching the snapshot when the deployment isn’t running or lacks URL.
Consider:
- Don’t assert non-null; only enable when all prerequisites are true:
- snapshotId present
- deployment is running
- deployment URL exists
Example:
-const snapshotQuery = useQuery({ - ...pipelineSnapshotQueries.detail(snapshotId!), - enabled: !!snapshotId -}); +const isDeploymentRunnable = + deploymentQuery.data?.body?.status === "running" && + Boolean(deploymentQuery.data?.body?.url); + +const snapshotQuery = useQuery({ + ...pipelineSnapshotQueries.detail(snapshotId as string), + enabled: Boolean(snapshotId && isDeploymentRunnable) +});Ensure pipelineSnapshotQueries.detail does not eagerly consume a non-string (undefined) id during options construction. If it does, wrap options creation in useMemo only when snapshotId is defined.
Also applies to: 35-38
src/components/deployments/playground/preview-visualization/index.tsx (1)
12-32: Use stable keys instead of array indexIndexes as keys can cause incorrect collapsible state when order changes. Use the entry title (object key), which is stable and unique in this context.
-{nonObjects.map(([title, value], idx) => ( - <CollapsibleCard ... key={idx}> +{nonObjects.map(([title, value]) => ( + <CollapsibleCard ... key={title}> ... ))} -{Object.entries(objects).map(([title, value], idx) => ( - <CollapsibleCard ... key={idx}> +{Object.entries(objects).map(([title, value]) => ( + <CollapsibleCard ... key={title}> ... ))} -{Object.entries(arrays).map(([title, value], idx) => ( - <CollapsibleCard ... key={idx}> +{Object.entries(arrays).map(([title, value]) => ( + <CollapsibleCard ... key={title}> ... ))}src/components/deployments/playground/preview-visualization/components/simple-value.tsx (1)
3-3: Verify component name casingImport uses Collapsiblevalue (lowercase v). Confirm the exported component name matches to avoid subtle import mistakes and readability issues. Prefer PascalCase: CollapsibleValue.
src/app/deployments/[deploymentId]/playground/_components/inputs/editor.tsx (1)
11-21: Scope JSON schema to this editor instanceUsing fileMatch ["*"] applies the schema globally. Bind the schema to a specific model URI and pass path to the Editor.
- monaco.languages.json.jsonDefaults.setDiagnosticsOptions({ + const MODEL_URI = "inmemory://models/deployment-input.json"; + const SCHEMA_URI = "inmemory://schemas/deployment-input-schema.json"; + monaco.languages.json.jsonDefaults.setDiagnosticsOptions({ enableSchemaRequest: true, validate: true, schemas: [ { - uri: "http://zenml.io/deployment-input-schema.json", - fileMatch: ["*"], + uri: SCHEMA_URI, + fileMatch: [MODEL_URI], schema: jsonSchema } ] });And pass the model path:
<Editor + path="inmemory://models/deployment-input.json" value={value} onChange={onChange} language="json" options={{ minimap: { enabled: false } }} className="h-full border border-theme-border-moderate" onMount={handleEditorMount} />Also applies to: 25-33
src/data/deployment-invocations/invoke.ts (1)
9-13: Make authKey optionalSemantically optional; simplifies config.
type InvokeConfig = { url: string; payload: DeploymentInvocationRequest; - authKey: string | undefined; + authKey?: string; };src/app/deployments/[deploymentId]/playground/_components/outputs/content.tsx (2)
25-30: Remove redundant|| undefinedoperators.The expressions
metadata.run_id || undefinedandmetadata.run_name || undefinedon Lines 27-28 are redundant. If these properties are already typed asstring | undefined, the|| undefinedhas no effect (an empty string would become undefined, which may not be intended). If the goal is to convert falsy values to undefined, be explicit about the desired behavior.If the properties are already
string | undefined, remove the redundant operators:<PlaygroundRunCard success={outputs.success} - runId={metadata.run_id || undefined} - runName={metadata.run_name || undefined} + runId={metadata.run_id} + runName={metadata.run_name} duration={outputs.execution_time} />If you specifically need to convert empty strings to undefined:
- runId={metadata.run_id || undefined} - runName={metadata.run_name || undefined} + runId={metadata.run_id || undefined} + runName={metadata.run_name || undefined}
32-54: Consider extracting nested ternaries for clarity.The nested ternary operators on Lines 32-48 handle three distinct rendering paths (preview, JSON, error). While functional, this could be more readable with helper variables or early returns.
Consider this refactor:
{outputs.success === true ? ( <> {activeView === "preview" ? ( <PlaygroundOutputsPreviewVisualization json={outputs.outputs} /> ) : ( <Editor value={JSON.stringify(outputs.outputs, null, "\t")} language="json" options={{ minimap: { enabled: false }, readOnly: true, wrappingStrategy: "advanced", wordWrap: "on", wordWrapColumn: 80 }} className="h-full border border-theme-border-moderate" /> )} </> ) : ( <PlaygroundEmptyState title="Failed to invoke deployment" subtitle={outputs.error || "Failed to invoke deployment"} /> )}src/types/deploymen-invocations.ts (2)
18-29: Simplify the redundant nullability pattern.Line 28 uses
error?: string | null, which combines optional (?) with explicit null union. This is redundant—optional already allowsundefined. Consider using justerror?: stringorerror: string | nulldepending on whether the API explicitly returnsnullor omits the field.Apply this diff to simplify the type if the API omits the field when there's no error:
- error?: string | null; + error?: string;Or if the API explicitly returns
null:- error?: string | null; + error: string | null;
31-56: Simplify redundant nullability patterns.Lines 45, 49, and 51 use the pattern
?: T | null, which is redundant. Optional (?) already permitsundefined, so adding| nullis unnecessary unless the API explicitly distinguishes between missing fields and explicitnullvalues.Apply this diff if fields are omitted when not present:
- snapshot_name?: string | null; + snapshot_name?: string; /** The name of the pipeline. */ pipeline_name: string; /** The ID of the pipeline run. */ - run_id?: string | null; + run_id?: string; /** The name of the pipeline run. */ - run_name?: string | null; + run_name?: string;Or if the API explicitly returns
nullfor these fields:- snapshot_name?: string | null; + snapshot_name: string | null; /** The name of the pipeline. */ pipeline_name: string; /** The ID of the pipeline run. */ - run_id?: string | null; + run_id: string | null; /** The name of the pipeline run. */ - run_name?: string | null; + run_name: string | null;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yamlsrc/assets/icons/collapse_lines.svgis excluded by!**/*.svgsrc/assets/icons/expand_lines.svgis excluded by!**/*.svg
📒 Files selected for processing (41)
package.json(2 hunks)src/app/deployments/[deploymentId]/_layout/header/tabs.tsx(2 hunks)src/app/deployments/[deploymentId]/layout.tsx(1 hunks)src/app/deployments/[deploymentId]/page.tsx(1 hunks)src/app/deployments/[deploymentId]/playground/_components/error.tsx(1 hunks)src/app/deployments/[deploymentId]/playground/_components/inputs/editor.tsx(1 hunks)src/app/deployments/[deploymentId]/playground/_components/inputs/index.tsx(1 hunks)src/app/deployments/[deploymentId]/playground/_components/inputs/invoke-form.ts(1 hunks)src/app/deployments/[deploymentId]/playground/_components/loader.tsx(1 hunks)src/app/deployments/[deploymentId]/playground/_components/outputs/content.tsx(1 hunks)src/app/deployments/[deploymentId]/playground/_components/outputs/delete-alert.tsx(1 hunks)src/app/deployments/[deploymentId]/playground/_components/outputs/empty-state.tsx(1 hunks)src/app/deployments/[deploymentId]/playground/_components/outputs/footer.tsx(1 hunks)src/app/deployments/[deploymentId]/playground/_components/outputs/header.tsx(1 hunks)src/app/deployments/[deploymentId]/playground/_components/outputs/index.tsx(1 hunks)src/app/deployments/[deploymentId]/playground/_components/outputs/outputs-switcher.tsx(1 hunks)src/app/deployments/[deploymentId]/playground/_components/outputs/run-card-avatar.tsx(1 hunks)src/app/deployments/[deploymentId]/playground/_components/outputs/run-card.tsx(1 hunks)src/app/deployments/[deploymentId]/playground/_components/outputs/types.ts(1 hunks)src/app/deployments/[deploymentId]/playground/page.content.tsx(1 hunks)src/app/deployments/[deploymentId]/playground/page.tsx(1 hunks)src/components/collapsible-chevron.tsx(1 hunks)src/components/deployments/playground/playground-view-switcher.tsx(1 hunks)src/components/deployments/playground/preview-visualization/components/array-value.tsx(1 hunks)src/components/deployments/playground/preview-visualization/components/boolean-value.tsx(1 hunks)src/components/deployments/playground/preview-visualization/components/collapsible-string-value.tsx(1 hunks)src/components/deployments/playground/preview-visualization/components/expand-collapse-icon.tsx(1 hunks)src/components/deployments/playground/preview-visualization/components/object-value.tsx(1 hunks)src/components/deployments/playground/preview-visualization/components/playground-value-collapsible.tsx(1 hunks)src/components/deployments/playground/preview-visualization/components/simple-value.tsx(1 hunks)src/components/deployments/playground/preview-visualization/index.tsx(1 hunks)src/components/deployments/playground/preview-visualization/services/sort-json-keys.ts(1 hunks)src/components/deployments/playground/preview-visualization/services/string-threshhold.ts(1 hunks)src/data/deployment-invocations/invoke.ts(1 hunks)src/data/deployments/fetch-detail.ts(1 hunks)src/data/pipeline-snapshots/fetch-snapshot-detail.ts(1 hunks)src/monaco-setup.ts(1 hunks)src/router/Router.tsx(2 hunks)src/router/routes.tsx(1 hunks)src/types/deploymen-invocations.ts(1 hunks)vite.config.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.tsx
⚙️ CodeRabbit configuration file
src/**/*.tsx: Review React components for adherence to the ZenML design system, accessibility standards, and efficient state management.
Files:
src/components/deployments/playground/preview-visualization/components/simple-value.tsxsrc/app/deployments/[deploymentId]/playground/page.tsxsrc/app/deployments/[deploymentId]/playground/page.content.tsxsrc/components/collapsible-chevron.tsxsrc/app/deployments/[deploymentId]/playground/_components/error.tsxsrc/router/routes.tsxsrc/components/deployments/playground/preview-visualization/components/array-value.tsxsrc/components/deployments/playground/preview-visualization/components/collapsible-string-value.tsxsrc/router/Router.tsxsrc/app/deployments/[deploymentId]/playground/_components/outputs/footer.tsxsrc/app/deployments/[deploymentId]/playground/_components/outputs/outputs-switcher.tsxsrc/app/deployments/[deploymentId]/playground/_components/outputs/empty-state.tsxsrc/components/deployments/playground/preview-visualization/index.tsxsrc/app/deployments/[deploymentId]/playground/_components/outputs/run-card-avatar.tsxsrc/app/deployments/[deploymentId]/playground/_components/outputs/delete-alert.tsxsrc/app/deployments/[deploymentId]/playground/_components/inputs/index.tsxsrc/app/deployments/[deploymentId]/playground/_components/outputs/content.tsxsrc/app/deployments/[deploymentId]/page.tsxsrc/app/deployments/[deploymentId]/_layout/header/tabs.tsxsrc/components/deployments/playground/preview-visualization/components/object-value.tsxsrc/app/deployments/[deploymentId]/playground/_components/loader.tsxsrc/components/deployments/playground/preview-visualization/components/expand-collapse-icon.tsxsrc/app/deployments/[deploymentId]/playground/_components/outputs/run-card.tsxsrc/app/deployments/[deploymentId]/playground/_components/outputs/index.tsxsrc/app/deployments/[deploymentId]/layout.tsxsrc/components/deployments/playground/preview-visualization/components/playground-value-collapsible.tsxsrc/app/deployments/[deploymentId]/playground/_components/outputs/header.tsxsrc/components/deployments/playground/playground-view-switcher.tsxsrc/app/deployments/[deploymentId]/playground/_components/inputs/editor.tsxsrc/components/deployments/playground/preview-visualization/components/boolean-value.tsx
src/**/*.ts
⚙️ CodeRabbit configuration file
src/**/*.ts: Evaluate TypeScript modules and hooks for strict typing, predictable side effects, and consistent error handling.
Files:
src/types/deploymen-invocations.tssrc/components/deployments/playground/preview-visualization/services/string-threshhold.tssrc/app/deployments/[deploymentId]/playground/_components/outputs/types.tssrc/components/deployments/playground/preview-visualization/services/sort-json-keys.tssrc/data/pipeline-snapshots/fetch-snapshot-detail.tssrc/data/deployment-invocations/invoke.tssrc/monaco-setup.tssrc/app/deployments/[deploymentId]/playground/_components/inputs/invoke-form.tssrc/data/deployments/fetch-detail.ts
🔇 Additional comments (20)
src/app/deployments/[deploymentId]/playground/_components/outputs/empty-state.tsx (1)
1-9: LGTM! Clean empty state component.The component correctly renders an empty state for playground outputs with appropriate styling and messaging.
src/components/deployments/playground/preview-visualization/components/expand-collapse-icon.tsx (1)
1-17: LGTM! Clean icon toggle implementation.The component is well-structured with clear props and straightforward conditional rendering. The use of
cnfor className merging follows best practices.src/app/deployments/[deploymentId]/playground/_components/outputs/run-card-avatar.tsx (1)
18-24: LGTM! Proper null safety and type handling.The component correctly handles the optional chaining and boolean conversion for the service account flag.
src/app/deployments/[deploymentId]/playground/_components/loader.tsx (1)
1-14: LGTM!The component is simple, well-structured, and correctly typed. The use of the Spinner from the component library and the optional subtitle prop provides a clean loading experience.
src/app/deployments/[deploymentId]/playground/_components/outputs/types.ts (1)
1-1: LGTM!The union type is well-defined and appropriate for view switching in the outputs UI.
src/router/routes.tsx (1)
54-55: LGTM!The new playground route follows the existing pattern and is correctly structured. The route function signature and path interpolation are consistent with other routes in the codebase.
src/app/deployments/[deploymentId]/_layout/header/tabs.tsx (1)
2-2: LGTM!The playground tab is correctly added following the existing pattern. The import, routing, and icon usage are all consistent with the codebase conventions.
Also applies to: 28-33
src/app/deployments/[deploymentId]/playground/_components/outputs/header.tsx (1)
1-16: LGTM!The header component is clean and well-structured. The props are correctly typed, and the layout appropriately separates the title from the view switcher.
src/app/deployments/[deploymentId]/playground/_components/outputs/footer.tsx (1)
1-14: LGTM!The footer component is well-structured with appropriate conditional rendering. The props are correctly typed, and the layout properly positions the delete action.
package.json (1)
28-28: Compatibility confirmed:@monaco-editor/react@^4.7.0supportsmonaco-editor@^0.54.0
monaco-editor@^0.54.0falls within the peer dependency range (>=0.25.0 <1).src/app/deployments/[deploymentId]/playground/_components/inputs/index.tsx (1)
18-56: LGTM!The form structure is well-organized with proper button types, disabled states during invocation, and clean integration with react-hook-form's Controller pattern.
src/app/deployments/[deploymentId]/playground/_components/outputs/run-card.tsx (1)
8-30: LGTM!The component structure is clean with proper prop typing, conditional rendering for the Run Details button, and appropriate use of the Link component with the asChild pattern.
src/app/deployments/[deploymentId]/playground/_components/outputs/index.tsx (1)
1-22: LGTM!Clean implementation with proper conditional rendering logic. The three-state pattern (loading, empty, content) is correctly implemented and easy to follow.
src/components/deployments/playground/playground-view-switcher.tsx (2)
6-27: LGTM!Proper forwardRef implementation with correct TypeScript types, spread props handling, and displayName set for better debugging.
28-51: LGTM!Clean controlled component implementation with proper type composition extending Radix Tabs props. The value/onValueChange wiring is correct.
src/components/deployments/playground/preview-visualization/components/array-value.tsx (1)
8-15: LGTM!Clean collapsible wrapper implementation with proper state management for the open/close toggle.
src/components/deployments/playground/preview-visualization/components/object-value.tsx (2)
9-16: LGTM!Consistent collapsible wrapper pattern matching the ArrayValue component structure.
18-51: LGTM!Clean implementation with proper use of
sortJsonKeysfor categorization and recursive rendering of nested structures. The variable naming in the destructured entries is correct.src/types/deploymen-invocations.ts (2)
1-16: LGTM! Type definition is well-structured.The
DeploymentInvocationRequesttype is well-defined with appropriate use ofunknownfor the parameters field (safer thanany), clear documentation, and sensible optional fields.
58-70: LGTM! Validation error types are well-structured.The
InvocationValidationErrorandInvocationValidationErrorDetailtypes appropriately model validation errors with location tracking, messages, and error types. The structure supports detailed error reporting.
src/app/deployments/[deploymentId]/playground/_components/inputs/index.tsx
Show resolved
Hide resolved
src/app/deployments/[deploymentId]/playground/_components/inputs/invoke-form.ts
Show resolved
Hide resolved
src/app/deployments/[deploymentId]/playground/_components/outputs/delete-alert.tsx
Show resolved
Hide resolved
src/app/deployments/[deploymentId]/playground/_components/outputs/outputs-switcher.tsx
Show resolved
Hide resolved
src/app/deployments/[deploymentId]/playground/_components/outputs/run-card.tsx
Show resolved
Hide resolved
|
I applied the changes from the other review, in this PR already |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Style