Rebuilds update workflow state machine #1372
Conversation
WalkthroughAdds automatic publish-on-rebuild support: propagates an Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API as Product API
participant Workflow as State Machine
participant Build as Build Service
participant Publish as Publish Service
participant Notify as Notification Queue
User->>API: Trigger Rebuild (isAutomatic=true)
API->>Workflow: create instance (isAutomatic=true, autoPublishOnRebuild)
Workflow->>Build: run build
Build-->>Workflow: Build Successful
rect rgb(230, 245, 230)
note right of Workflow: Evaluate guard\nautoPublishOnRebuild && isAutomatic && type=Rebuild
end
alt Guard passes
Workflow->>Publish: transition to Publish
Publish-->>Workflow: Publish Completed
Workflow->>Notify: notifyAutoPublishOwner(productId)
Notify-->>Owner: enqueue email (autoPublishOnRebuildCompleted)
else Guard fails
Workflow->>Workflow: await manual publish
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
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)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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 |
James-Voight
force-pushed
the
Rebuilds-Update-WorkflowStateMachine
branch
from
ae8eadb to
49a9ee3
Compare
FyreByrd
left a comment
FyreByrd
left a comment
There was a problem hiding this comment.
This is a good start. Hopefully we can get testing figured out soon.
src/lib/products/server.ts
Outdated
| options: new Set(product.ProductDefinition[flowType].WorkflowOptions), | ||
| workflowType: product.ProductDefinition[flowType].Type | ||
| workflowType: product.ProductDefinition[flowType].Type, | ||
| isAutomatic: false |
There was a problem hiding this comment.
Since doProductAction is likely going to be used for the software update feature, you will probably want to have isAutomatic as either an optional parameter, or a parameter with a default value. Either way you do it, you should be able to modify the function signature of doProductAction without needing to modify the callsites.
| } | ||
| }); | ||
| if (!product?.Project.OwnerId) return; | ||
| void getQueues().Emails.add(`Notify Owner of Auto Publish for Product #${productId}`, { |
There was a problem hiding this comment.
What's the purpose of the void keyword here? Why not await?
There was a problem hiding this comment.
I have the same question in a few other locations.
James-Voight
force-pushed
the
Rebuilds-Update-WorkflowStateMachine
branch
from
49a9ee3 to
65510c3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/lib/server/workflow/dbProcedures.ts (1)
55-82: Consider adding error handling for robustness.The function silently returns on missing product or owner without logging. Consider adding logging or error handling for:
- Database read failures
- Missing product scenarios
- Email queueing failures
This would aid debugging if auto-publish notifications fail to send.
Example:
export async function notifyAutoPublishOwner(productId: string) { const product = await DatabaseReads.products.findUnique({ where: { Id: productId }, select: { ProductDefinition: { select: { Name: true } }, Project: { select: { Name: true, OwnerId: true } } } }); - if (!product?.Project.OwnerId) return; + if (!product) { + console.warn(`notifyAutoPublishOwner: Product ${productId} not found`); + return; + } + if (!product.Project.OwnerId) { + console.warn(`notifyAutoPublishOwner: No owner for product ${productId}`); + return; + } await getQueues().Emails.add(`Notify Owner of Auto Publish for Product #${productId}`, { type: BullMQ.JobType.Email_SendNotificationToUser, userId: product.Project.OwnerId, messageKey: 'autoPublishOnRebuildCompleted', messageProperties: { projectName: product.Project.Name ?? '', productName: product.ProductDefinition.Name ?? '' } }); }src/lib/server/workflow/state-machine.ts (1)
815-846: Align notification condition more tightly with the auto-publish guardThe owner notification on
Publish_Completedis currently gated by:if (context.autoPublishOnRebuild && context.isAutomatic) { void notifyAutoPublishOwner(context.productId); }whereas the transition into auto‑publish is guarded by
autoPublishOnRebuild(which also checksworkflowType === WorkflowType.Rebuild).Two suggestions:
Match the guard semantics
If the email is intended only for “auto publish on rebuild” runs (not any future workflow that might setisAutomatic), consider also checking the workflow type, e.g.:if ( context.autoPublishOnRebuild && context.isAutomatic && context.workflowType === WorkflowType.Rebuild ) { void notifyAutoPublishOwner(context.productId); }This keeps the notification tied to the same high‑level condition as the auto‑publish path.
Optional: handle notification failures explicitly
notifyAutoPublishOwneris async and is fire‑and‑forget here. That matches the existing pattern for queueing jobs, but if you ever see unhandled rejection noise from this path, consider adding a.catchinsidenotifyAutoPublishOwneror here to log and swallow failures.src/lib/server/workflow/index.ts (2)
150-205: Snapshot restore defaultsisAutomaticcorrectly and exposes new inputsParsing
instance.Contextinto aWorkflowInstanceContext, then applying:context.isAutomatic ??= false;before building the returned
contextandinput:
- Gives older snapshots (which lack
isAutomatic) a safe default offalse.- Ensures
input.isAutomaticandcontext.isAutomaticstay in sync.- Adds
autoPublishOnRebuildtoinputbased on the currentProjectflag, so snapshot restores always reflect the latest project setting rather than whatever might have been in the stored context.One small nit: after the nullish‑assignment,
context.isAutomatic ?? falsein theinputconstruction is redundant; you can just usecontext.isAutomaticif you want to simplify slightly.
366-405: Consider using the filtered context for both create and updateIn
createSnapshot, you build afilteredcontext that strips non‑instance fields:const filtered = { ...context, productId: undefined, hasAuthors: undefined, hasReviewers: undefined, autoPublishOnRebuild: undefined, productType: undefined, options: undefined } as WorkflowInstanceContext;but only use it on
update, whilecreatestill persists the fullcontext:create: { State: ..., Context: JSON.stringify(context), ... }, update: { State: ..., Context: JSON.stringify(filtered) }That means the very first row for a workflow instance may still serialize
autoPublishOnRebuild,productId, etc., even though subsequent updates do not.Behavior-wise this is harmless (the restore path rebuilds
inputfrom DB fields and overlays it), but for consistency and to keepworkflowInstances.Contextstrictly toWorkflowInstanceContext, you could also usefilteredin thecreatebranch:- create: { - State: Workflow.stateName(this.currentState ?? { id: 'Start' }), - Context: JSON.stringify(context), + create: { + State: Workflow.stateName(this.currentState ?? { id: 'Start' }), + Context: JSON.stringify(filtered),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/lib/products/server.ts(2 hunks)src/lib/server/email-service/locales/en-us.json(2 hunks)src/lib/server/email-service/locales/es-419.json(2 hunks)src/lib/server/email-service/locales/fr-FR.json(2 hunks)src/lib/server/job-executors/product.ts(1 hunks)src/lib/server/job-executors/system.ts(1 hunks)src/lib/server/workflow/dbProcedures.ts(2 hunks)src/lib/server/workflow/index.ts(5 hunks)src/lib/server/workflow/state-machine.ts(5 hunks)src/lib/workflowTypes.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-12T14:07:02.200Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1283
File: src/routes/(authenticated)/tasks/[product_id]/+page.server.ts:285-303
Timestamp: 2025-09-12T14:07:02.200Z
Learning: In src/routes/(authenticated)/tasks/[product_id]/+page.server.ts, FyreByrd prefers to optimize filterAvailableActions by creating Sets at the caller level rather than inside the function, so the function would take Set<number> arguments instead of arrays for better performance and separation of concerns.
Applied to files:
src/lib/products/server.ts
📚 Learning: 2025-09-04T14:26:59.326Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1227
File: src/lib/server/job-executors/product.ts:247-250
Timestamp: 2025-09-04T14:26:59.326Z
Learning: In src/lib/server/job-executors/product.ts, the createLocal function's catch block returns false instead of rethrowing errors. This was implemented intentionally to fix another issue, so any changes to this error handling should be carefully evaluated for downstream impacts.
Applied to files:
src/lib/products/server.tssrc/lib/server/job-executors/product.ts
📚 Learning: 2025-09-04T16:23:55.891Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1227
File: src/lib/server/job-executors/product.ts:247-250
Timestamp: 2025-09-04T16:23:55.891Z
Learning: In src/lib/server/job-executors/product.ts, createLocal’s catch should log the error via job.log with relevant context (projectId/productDefinitionId/storeId) and still return false to preserve the intentional “no-retry” behavior.
Applied to files:
src/lib/server/job-executors/product.ts
📚 Learning: 2025-09-12T14:02:04.558Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1283
File: src/lib/server/workflow/index.ts:52-74
Timestamp: 2025-09-12T14:02:04.558Z
Learning: In the appbuilder-portal codebase, when a Product exists, it always has an associated Project relationship. The Project._count fields (Authors, Reviewers) are safe to access directly when the Product query returns a result.
Applied to files:
src/lib/server/workflow/index.ts
🧬 Code graph analysis (3)
src/lib/server/workflow/dbProcedures.ts (3)
src/lib/server/job-executors/build.ts (1)
product(11-131)src/lib/server/job-executors/publish.ts (1)
product(10-158)src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)
src/lib/server/workflow/state-machine.ts (2)
src/lib/workflowTypes.ts (1)
autoPublishOnRebuild(265-271)src/lib/server/workflow/dbProcedures.ts (1)
notifyAutoPublishOwner(55-82)
src/lib/server/workflow/index.ts (1)
src/lib/workflowTypes.ts (1)
WorkflowInstanceContext(104-137)
🔇 Additional comments (12)
src/lib/server/job-executors/product.ts (1)
247-248: LGTM: Correct initialization for manual workflow creation.The
isAutomatic: falsefield is appropriately set for manually created products viacreateLocal.src/lib/server/job-executors/system.ts (1)
976-976: LGTM: Appropriate default for migrated workflows.The
isAutomatic: falseinitialization is correct for workflow instances created during migration, as these represent pre-existing workflows rather than automatically triggered ones.src/lib/products/server.ts (2)
7-11: LGTM: Default parameter addresses previous feedback.The addition of the
isAutomatic = falsedefault parameter properly addresses the past review comment about making this parameter optional for backward compatibility.Based on learnings
56-57: LGTM: Correct propagation of isAutomatic flag.The
isAutomaticparameter is properly passed toWorkflow.createfor rebuild/republish workflows.src/lib/server/email-service/locales/en-us.json (1)
76-76: LGTM: English translations added correctly.The
autoPublishOnRebuildCompletedtranslation keys are properly added with appropriate English messages and correct placeholder variables (projectName,productName).Also applies to: 118-118
src/lib/workflowTypes.ts (4)
2-3: LGTM: Correct import optimization.The split between value import (
WorkflowType) and type-only import (RoleId) is appropriate, asWorkflowTypeis used in the runtime comparison at line 269.
136-136: LGTM: Consistent field additions.The
isAutomatic: booleanfield is properly added to bothWorkflowInstanceContextandWorkflowConfig, ensuring type consistency across the workflow system.Also applies to: 186-186
193-193: LGTM: Appropriate field addition.The
autoPublishOnRebuild: booleanfield onWorkflowInputcorrectly extends the workflow configuration for the auto-publish feature.
265-272: LGTM: Guard logic is correct.The
autoPublishOnRebuildguard properly checks all three required conditions:
- Feature is enabled (
autoPublishOnRebuild)- Workflow is automatic (
isAutomatic)- Workflow type is Rebuild
The function is correctly added to the
Guardstype union.src/lib/server/workflow/state-machine.ts (2)
17-25: Context wiring forisAutomatic/autoPublishOnRebuildlooks consistentImporting
autoPublishOnRebuildandnotifyAutoPublishOwner, and carryingisAutomatic,hasReviewers, andautoPublishOnRebuildthrough the machine context frominputmatches how the guards and notifications are used later. This keepsWorkflowContextandWorkflowInputaligned so both guard-based filtering and runtime guards have the data they need. I don’t see issues with this wiring.Also applies to: 60-68
528-563: Auto-publish transition onBuild_Successfulis correctly guardedThe new
Build_Successfultransition that jumps directly toProduct_PublishunderautoPublishOnRebuild:
- Is placed before the normal “verify and publish” path, so it naturally short-circuits only when the guard passes.
- Uses
includeWhen.guards: [autoPublishOnRebuild]plusguard: autoPublishOnRebuild, keeping the visualization/happy‑path filtering and runtime behavior in sync.- Leaves the existing Google Play startup path and the default manual path untouched.
This matches the intent of an opt‑in automatic rebuild publish without breaking existing flows.
src/lib/server/workflow/index.ts (1)
51-76: Project-levelAutoPublishOnRebuildis plumbed cleanly into workflow inputExtending the initial product query to select
Project.AutoPublishOnRebuildand threading it intoWorkflowInput.autoPublishOnRebuild(alongsidehasAuthors/hasReviewers) is straightforward and keeps all the auto‑publish inputs together. The!!check?.Project.AutoPublishOnRebuildcoercion means missing or null values safely default tofalse. This looks good.
2 participants
Rebuilds: Update WorkflowStateMachine
#1324
Summary by CodeRabbit