Create Snapshots for OSS #856
Conversation
📝 WalkthroughWalkthroughAdds a run-scoped “Create Snapshot” flow: updates RunActionsMenu to accept runId and link to a new create-snapshot route/page. Implements form, schema, selects, submission hook, and utility for naming. Adds infinite queries for pipelines and runs. Minor UI class tweaks to icons/text truncation. No changes to existing public data-fetching signatures except explicit return type and new exports. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Header as RunsDetailHeader
participant Menu as RunActionsMenu
participant Router
participant Page as CreateSnapshotFromRunPage
participant Form as CreateSnapshotForm
participant Hook as useSubmitHandler
participant Data as fetchPipelineRun/useUpdateSnapshot
participant Nav as Navigation
User->>Header: Open run details
Header->>Menu: Render with runId
User->>Menu: Click "New Snapshot"
Menu->>Router: Link to runs.createSnapshot(runId)
Router->>Page: Load page (protected)
Page->>Data: usePipelineRun(runId)
Data-->>Page: Run data or error
Page->>Form: Render with run (if loaded)
Note over Form,Hook: On submit
Form->>Hook: handleCreateSnapshot(name, pipelineId, runId)
alt originalSnapshotId provided
Hook->>Data: useUpdateSnapshot.mutate({ snapshotId, name })
else Fetch snapshotId from run
Hook->>Data: fetchPipelineRun(runId)
Data-->>Hook: { snapshotId? }
alt snapshotId found
Hook->>Data: useUpdateSnapshot.mutate({ snapshotId, name })
else
Hook-->>Form: Toast "Snapshot not found"
end
end
Data-->>Hook: onSuccess
Hook-->>Nav: Navigate to snapshot overview
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested reviewers
Poem
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/components/pipeline-snapshots/create/_form-components/create-snapshot-form-footer.tsx (1)
13-13: Consider explicit navigation instead of history-based.Using
navigate(-1)can be unpredictable if users arrive via direct link or external navigation. Consider navigating to a specific route (e.g., the run detail page or runs list) for more predictable UX.Example:
-<Button type="button" onClick={() => navigate(-1)} intent="secondary" size="md"> +<Button type="button" onClick={() => navigate(routes.projects.runs.detail(runId))} intent="secondary" size="md">Note: You would need to pass
runIdas a prop to implement this suggestion.src/components/pipeline-snapshots/create/name-helper.ts (1)
1-3: Consider improving the name generation strategy.The current approach has a few limitations:
- Timestamp length:
Date.now()produces 13-digit millisecond timestamps, creating long names like"my-run-snapshot-1728205177123".- Not user-friendly: Millisecond timestamps are hard to read and compare.
- Potential collisions: Rapid submissions could theoretically generate the same timestamp.
- No input validation: Empty or very long
runNamevalues could produce problematic names.Consider alternatives:
export function generateSnapshotName(runName: string) { - return `${runName}-snapshot-${Date.now()}`; + // Option 1: Use shorter, more readable format + const timestamp = new Date().toISOString().replace(/[:.]/g, '-').slice(0, -5); + return `${runName}-snapshot-${timestamp}`; + + // Option 2: Use a counter or random suffix + const suffix = Math.random().toString(36).substring(2, 8); + return `${runName}-snapshot-${suffix}`; }Or add validation:
export function generateSnapshotName(runName: string) { const sanitizedName = runName.trim() || 'snapshot'; const timestamp = Date.now(); return `${sanitizedName}-${timestamp}`; }src/components/pipeline-snapshots/create/run-select.tsx (1)
28-32: Skip querying until a pipeline is selectedWithout a pipeline, the request fires with an empty
pipeline_id, producing avoidable errors and an “Error fetching runs” stub in the UI. Gate the query withenabled: !!pipelineso it waits until the user picks a pipeline.const runQuery = useInfiniteQuery({ ...allPipelineRunsInfinite({ params: { pipeline_id: pipeline, sort_by: "desc:created" } - }) + }), + enabled: Boolean(pipeline) });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/app/pipelines/[pipelineId]/snapshots/columns/index.tsx(1 hunks)src/app/runs/[id]/Header.tsx(1 hunks)src/app/runs/[id]/RunActionMenu.tsx(3 hunks)src/app/runs/[id]/create-snapshot/page.tsx(1 hunks)src/app/runs/columns.tsx(1 hunks)src/components/pipeline-snapshots/create/_form-components/create-snapshot-form-footer.tsx(1 hunks)src/components/pipeline-snapshots/create/_form-components/create-snapshot-form-header.tsx(1 hunks)src/components/pipeline-snapshots/create/form-schema.ts(1 hunks)src/components/pipeline-snapshots/create/form.tsx(1 hunks)src/components/pipeline-snapshots/create/name-helper.ts(1 hunks)src/components/pipeline-snapshots/create/pipeline-select.tsx(1 hunks)src/components/pipeline-snapshots/create/run-select.tsx(1 hunks)src/components/pipeline-snapshots/create/use-submit-handler.ts(1 hunks)src/data/pipeline-runs/all-pipeline-runs-query.ts(2 hunks)src/data/pipeline-runs/pipeline-run-detail-query.ts(1 hunks)src/data/pipelines/index.ts(2 hunks)src/router/Router.tsx(2 hunks)src/router/routes.tsx(1 hunks)
🔇 Additional comments (6)
src/app/runs/columns.tsx (1)
65-65: LGTM! Icon layout improvement.Adding
shrink-0prevents the icon from being compressed in flex layouts when space is constrained, maintaining consistent visual sizing alongside dynamic text content.src/app/pipelines/[pipelineId]/snapshots/columns/index.tsx (1)
60-60: LGTM!The addition of
shrink-0to the icon prevents it from shrinking in the flex container, andtruncateon the text prevents overflow. Both are appropriate UI improvements.Also applies to: 67-67
src/router/routes.tsx (1)
34-34: LGTM!The new
createSnapshotroute generator follows the existing pattern and integrates cleanly with the runs routes.src/data/pipelines/index.ts (1)
13-21: LGTM!The infinite query implementation follows TanStack Query v5 best practices:
- Properly uses
pageParamin the query function- Correctly implements
getNextPageParamwith null termination- Distinguishes the infinite query key with an "infinite" suffix
src/app/runs/[id]/Header.tsx (1)
41-41: LGTM!Passing
runIdtoRunActionsMenuenables the new snapshot creation flow from the run context.src/components/pipeline-snapshots/create/_form-components/create-snapshot-form-footer.tsx (1)
18-18: Verify the spinner color scheme.The spinner uses
border-theme-text-negative(typically red/error color) combined withborder-b-theme-text-brand. This creates a two-tone effect that may not align with the brand's loading indicator style. Ensure this is intentional.Typical spinner patterns use a single color with one transparent/lighter edge:
-<div className="h-4 w-4 animate-spin rounded-rounded border-2 border-theme-text-negative border-b-theme-text-brand" /> +<div className="h-4 w-4 animate-spin rounded-rounded border-2 border-theme-border-moderate border-b-theme-text-brand" />
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
@znegrin Within this PR, I created the option to create a snapshot from the run detail.
In this case, pipeline and run are prefilled already, and disabled.
Do you think there will be another practical way, where the user selects the pipeline first, and the run afterwards, so basically starting with an entirely empty form for creating a snapshot?
In case no, we can prob drop teh entire Run and Pipeline Selects we're having right now.
What do you think?
|
@Cahllagerfeld this was designed with the two dropdowns for the "Run a pipeline" option that we have in both the pipelines and the runs list, so users can click there and then select it from scratch. Not sure if this is used or even relevant, but this was the logic behind it. I agree that for the cases where users don't need to select anything, we can skip these dropdowns, as users already have the information in the header. You can see in my new designs that I am not showing them anymore for certain cases. From what I've seen, we have four different ways to create, edit, or run a snapshot, if I'm correct:
|
|
Okay I got it 👍 |
2 participants
Summary by CodeRabbit
New Features
Style
Testing
Create snapshot