Rebuilds update workflow state machine#1372
Rebuilds update workflow state machine#1372James-Voight wants to merge 4 commits intosillsdev:developfrom
Conversation
📝 WalkthroughWalkthroughPropagates an isAutomatic flag into product actions and workflow instances, adds autoPublishOnRebuild to workflow input/guards, introduces guarded auto-transition to publish after build, notifies project owner via queued email on automatic publish, and adds locale keys for the notification. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API as Product API
participant Workflow as Workflow Engine
participant Build as Build Service
participant Publish as Publish Service
participant Queue 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->>Queue: notifyAutoPublishOwner(productId)
Queue-->>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 | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.
James-Voight
force-pushed
the
Rebuilds-Update-WorkflowStateMachine
branch
from
4016687 to
53e957c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib/server/workflow/index.ts (1)
189-207: Minor redundancy inisAutomaticfallback.Line 190 already ensures
context.isAutomaticisfalseif undefined (via??=), so the?? falseon line 203 is redundant. However, this doesn't affect correctness.Optional cleanup
input: { workflowType: instance.WorkflowDefinition.Type, productType: instance.WorkflowDefinition.ProductType, options: new Set(instance.WorkflowDefinition.WorkflowOptions), hasAuthors: !!instance.Product.Project._count.Authors, hasReviewers: !!instance.Product.Project._count.Reviewers, autoPublishOnRebuild: !!instance.Product.Project.AutoPublishOnRebuild, - isAutomatic: context.isAutomatic ?? false, + isAutomatic: context.isAutomatic, productId, existingApp: !!context.environment[ENVKeys.GOOGLE_PLAY_EXISTING] }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/lib/products/server.tssrc/lib/server/email-service/locales/en-us.jsonsrc/lib/server/email-service/locales/es-419.jsonsrc/lib/server/email-service/locales/fr-FR.jsonsrc/lib/server/job-executors/product.tssrc/lib/server/job-executors/system.tssrc/lib/server/workflow/dbProcedures.tssrc/lib/server/workflow/index.tssrc/lib/server/workflow/state-machine.tssrc/lib/workflowTypes.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/lib/products/server.ts
- src/lib/server/workflow/dbProcedures.ts
- src/lib/server/job-executors/system.ts
- src/lib/server/email-service/locales/fr-FR.json
- src/lib/server/email-service/locales/en-us.json
- src/lib/server/email-service/locales/es-419.json
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-04T23:01:23.843Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1384
File: src/lib/server/workflow/index.ts:92-103
Timestamp: 2025-12-04T23:01:23.843Z
Learning: In src/lib/server/workflow/index.ts, the Workflow.create method intentionally creates a snapshot before creating product transitions (around line 83) and another after starting the flow (around line 103). The first snapshot is required to prevent errors when the initial state is Product Build (as in Rebuild workflows). The second snapshot ensures the user tasks job has up-to-date information after transitions are created and the flow is started.
Applied to files:
src/lib/server/workflow/state-machine.tssrc/lib/server/workflow/index.tssrc/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
📚 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/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
🧬 Code graph analysis (1)
src/lib/server/workflow/state-machine.ts (1)
src/lib/workflowTypes.ts (2)
autoPublishOnRebuild(277-283)newGPApp(270-276)
⏰ 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)
- GitHub Check: checks / build-and-test
🔇 Additional comments (14)
src/lib/workflowTypes.ts (5)
2-3: LGTM!Good refactor to use type-only import for
RoleIdwhile keepingWorkflowTypeas a value import since it's used at runtime.
137-138: LGTM!The
isAutomaticfield is appropriately added toWorkflowInstanceContextfor persistence across workflow state transitions.
189-190: LGTM!Consistent addition of
isAutomatictoWorkflowConfig.
196-198: LGTM!The
autoPublishOnRebuildfield is properly added toWorkflowInput, enabling the feature flag to flow through the workflow system.
277-288: LGTM!The
autoPublishOnRebuildguard function follows the established pattern of other guards and correctly validates all three required conditions. TheGuardstype union is properly extended to include it.src/lib/server/job-executors/product.ts (1)
243-249: LGTM!Correctly sets
isAutomatic: falsefor local product creation. This ensures that manually created products don't trigger auto-publish behavior, reserving that for system-initiated rebuild workflows.src/lib/server/workflow/state-machine.ts (5)
17-26: LGTM!Imports are correctly added for the new
autoPublishOnRebuildguard andnotifyAutoPublishOwnerprocedure.
61-69: LGTM!Context is correctly initialized with
isAutomaticandautoPublishOnRebuildfrom the input.
550-564: LGTM!The auto-publish transition is correctly positioned after the App Store Preview path and before the Create App Store Entry path. The guard is properly applied both for visualization (
includeWhen) and runtime (guard). The console log provides useful debugging information for automatic workflows.
844-848: LGTM!The guard logic correctly distinguishes between new Google Play apps (which go to Make_It_Live) and other products or existing Google Play apps (which go directly to Published).
829-848: Review comment is based on an incorrect assumption aboutisAutomatic.The core premise—that "
isAutomaticis only set totruefor Rebuild workflows"—is not supported by the codebase. All observable code paths initializeisAutomatictofalse:
- Default parameter:
isAutomatic = false(products/server.ts)- Job executor (product):
isAutomatic: false- Job executor (system):
isAutomatic: false- Workflow initialization:
isAutomatic: context.isAutomatic ?? falseNo code in the repository sets
isAutomatictotrue. If this is intentional (notification never sent) or if there's a missing implementation, that's a separate issue. However, the inline conditioncontext.autoPublishOnRebuild && context.isAutomaticis not "less restrictive than the guard"—ifisAutomaticis nevertrue, adding aworkflowTypecheck is unnecessary. The proposed refactoring suggestions are therefore not actionable based on the current code state.Likely an incorrect or invalid review comment.
src/lib/server/workflow/index.ts (3)
58-70: LGTM!Database query correctly extended to fetch
AutoPublishOnRebuildfrom the Project for use in workflow initialization.
71-78: LGTM!The
autoPublishOnRebuildflag is correctly initialized from the Project setting with proper null-safety using!!.
370-380: LGTM!Correctly excludes
autoPublishOnRebuildfrom the persisted snapshot context since it's part of the workflow input derived from Project settings, not the instance state. This is consistent with the treatment of other input-derived fields.
2 participants
Rebuilds: Update WorkflowStateMachine
#1324
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.