Allow ProductDefinition to be configured to start manual rebuilds at Synchronize Data#1352
Allow ProductDefinition to be configured to start manual rebuilds at Synchronize Data#1352
Conversation
This was not removed earlier when modifying the migration sql
WalkthroughAdds a nullable StartManualRebuildAt field to ProductDefinitions (DB + Prisma), migration logic to populate it from existing Properties.ShouldExecute, exposes it in product APIs and workflow types, and adds UI and localization to configure the manual rebuild start state. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin UI
participant Server as App Server
participant DB as Database
participant Migrator as Migration Job
participant Worker as Workflow Engine
rect rgb(230,245,255)
Admin->>Server: submit product definition (create/edit) with StartManualRebuildAt
Server->>DB: persist ProductDefinitions.StartManualRebuildAt
end
rect rgb(245,245,230)
Migrator->>DB: read ProductDefinitions.Properties
Migrator->>Migrator: parse JSON -> extract ShouldExecute targets
alt exactly one target found
Migrator->>DB: update StartManualRebuildAt for that product
Migrator-->>Migrator: mark updatedProductDefinitions = true
else multiple/none found
Migrator-->>Migrator: log info, skip
end
end
rect rgb(235,255,235)
Admin->>Server: trigger manual rebuild
Server->>DB: read ProductDefinitions.StartManualRebuildAt
Server->>Worker: create workflow (type: rebuild, start = StartManualRebuildAt)
Worker->>Worker: execute workflow from specified start state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Reasoning: multiple layers changed (DB migration, parsing-heavy migration job, API/server payloads, types, and UI). Migration logic and workflow persistence require careful review; other changes are repetitive and lower risk. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/server/job-executors/system.ts (1)
724-739: Fix always-true updatedProductDefinitions flagUsing !!array is always true. Base it on length or actual updates.
- updatedProductDefinitions: !!prodDefsNeedUpdate, + updatedProductDefinitions: prodDefsNeedUpdate.length > 0,If you adopt the counter in the previous diff, prefer returning the count instead (e.g., updatedProductDefinitionsCount).
🧹 Nitpick comments (5)
src/lib/products/server.ts (1)
54-60: Consider validating before casting to WorkflowState.The code casts
StartManualRebuildAtdirectly toWorkflowStatewithout validation. If the database contains an invalid value (due to manual editing or data migration), this could cause runtime issues downstream.Apply this diff to add defensive validation:
workflowType: product.ProductDefinition[flowType].Type, // ISSUE #1324 use isAutomatic here start: (action === 'rebuild' && - (product.ProductDefinition.StartManualRebuildAt as WorkflowState)) || + product.ProductDefinition.StartManualRebuildAt && + Object.values(WorkflowState).includes(product.ProductDefinition.StartManualRebuildAt as WorkflowState) + ? (product.ProductDefinition.StartManualRebuildAt as WorkflowState) + : undefined) || undefinedsrc/routes/(authenticated)/admin/settings/product-definitions/edit/+page.server.ts (2)
17-17: Make field nullish and drop String() to avoid hidden-control validation failuresWhen the select is hidden, the field may be absent; v.nullable won’t accept undefined. Also, String(...) is redundant.
Apply:
- startManualRebuildAt: v.nullable(v.literal(String(WorkflowState.Synchronize_Data))), + startManualRebuildAt: v.nullish(v.literal(WorkflowState.Synchronize_Data)),
67-75: Clear StartManualRebuildAt when rebuildWorkflow is unsetPrevents stale state if a user selects a start point then removes the rebuild flow.
data: { TypeId: form.data.applicationType, Name: form.data.name, WorkflowId: form.data.workflow, RebuildWorkflowId: form.data.rebuildWorkflow, - StartManualRebuildAt: form.data.startManualRebuildAt, + StartManualRebuildAt: form.data.rebuildWorkflow + ? form.data.startManualRebuildAt ?? null + : null, RepublishWorkflowId: form.data.republishWorkflow, Description: form.data.description, Properties: form.data.properties }src/routes/(authenticated)/admin/settings/product-definitions/edit/+page.svelte (1)
99-111: Avoid sending "null" string; ensure field presence/reset when hiddenvalue={null} serializes as "null". If the control is hidden, the field may be missing.
Option A (simplest):
- <option value={null}>{WorkflowState.Product_Build}</option> + <option value="">{WorkflowState.Product_Build}</option>…and pair with the server change to map '' -> null (via v.nullish as proposed).
Option B (also reset on hide):
+ $: if (!$form.rebuildWorkflow) $form.startManualRebuildAt = null;Option C (ensure field present when hidden):
{#if !$form.rebuildWorkflow} <input type="hidden" name="startManualRebuildAt" value="" /> {/if}src/lib/workflowTypes.ts (1)
251-255: Harden canJump against non-enum targetsIf a plain string slips in, short-circuit before comparison.
export function canJump(args: { context: WorkflowContext }, params: JumpParams): boolean { - return ( - args.context.start === params.target && includeStateOrTransition(args.context, params.filter) - ); + const target = params.target; + if (typeof target === 'string' && !isWorkflowState(target)) return false; + return args.context.start === target && includeStateOrTransition(args.context, params.filter); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/lib/locales/en-US.json(1 hunks)src/lib/locales/es-419.json(1 hunks)src/lib/locales/fr-FR.json(1 hunks)src/lib/prisma/migrations/14_start_rebuilds_at_state/migration.sql(1 hunks)src/lib/prisma/schema.prisma(1 hunks)src/lib/products/server.ts(3 hunks)src/lib/server/job-executors/system.ts(3 hunks)src/lib/workflowTypes.ts(2 hunks)src/routes/(authenticated)/admin/settings/product-definitions/edit/+page.server.ts(3 hunks)src/routes/(authenticated)/admin/settings/product-definitions/edit/+page.svelte(2 hunks)src/routes/(authenticated)/admin/settings/product-definitions/new/+page.server.ts(2 hunks)src/routes/(authenticated)/admin/settings/product-definitions/new/+page.svelte(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/routes/(authenticated)/admin/settings/product-definitions/edit/+page.server.ts (1)
src/lib/valibot.ts (1)
idSchema(3-3)
src/lib/server/job-executors/system.ts (2)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)src/lib/server/database/index.ts (1)
DatabaseWrites(68-71)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: typecheck-lint
- GitHub Check: build-and-test
🔇 Additional comments (7)
src/lib/locales/en-US.json (1)
537-537: LGTM!The translation key follows the naming convention and is appropriately placed within the Product Definitions section.
src/lib/prisma/schema.prisma (1)
221-221: LGTM!The field definition is correct and follows the schema conventions. The placement between rebuild and republish workflow fields is logical.
src/lib/prisma/migrations/14_start_rebuilds_at_state/migration.sql (1)
1-2: LGTM!The migration correctly adds a nullable TEXT column that aligns with the schema definition.
src/routes/(authenticated)/admin/settings/product-definitions/new/+page.server.ts (1)
15-15: The validation schema is correct as-is and requires no changes.The schema at line 15 uses
v.literal(String(WorkflowState.Synchronize_Data))to accept only theSynchronize_Datavalue or null. The UI in+page.svelte(lines 104-105) confirms this is intentional—it presents exactly two options:nullandSynchronize_Data. Thev.literal()approach is appropriate here since only one non-null value is expected. No refactoring tov.union()orv.picklist()is needed.Likely an incorrect or invalid review comment.
src/routes/(authenticated)/admin/settings/product-definitions/edit/+page.server.ts (1)
9-9: LGTM on importing WorkflowState for schema literalImport aligns with new field usage.
src/routes/(authenticated)/admin/settings/product-definitions/edit/+page.svelte (1)
11-11: LGTM on importing WorkflowState for option labelsConsistent with server schema.
src/lib/workflowTypes.ts (1)
133-135: LGTM: Persisting start in contextKeeps DB snapshots self-contained; matches migration/PR intent.
src/routes/(authenticated)/admin/settings/product-definitions/new/+page.svelte
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
I have contacted Kalaam. They said they would review at their next team meeting (Tuesday, 10/28/25). Thanks for implementing this. If they don't need it, then it would likely be better not to add the complexity. |
|
Waiting to hear back from Kalaam. They are meeting on Tuesday, 10/28/25 |
|
I don't think they need it at the moment. I am moving it back to draft until we make sure they need it. |
2 participants
Fixes #1333
Below screenshot shows product definition automatically updated by startup migration task
Summary by CodeRabbit
New Features
Chores