Include links to jobs in product details #1384
Conversation
WalkthroughAdds centralized BullMQ enums, a new Prisma QueueRecords model and migration, threads an optional transition id through the workflow/state-machine and job executors, creates QueueRecords when jobs are queued, exposes records to super-admin SSE, and surfaces job-record links in product UI. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant SM as StateMachine
participant WF as Workflow
participant Bull as BullMQ
participant Worker as BullWorker
participant DB as Database
participant UI as Admin UI
Note over SM,WF: resolve transition then enqueue
SM->>WF: currentProductTransition(productId, initialState)
WF->>DB: query ProductTransitions (prefer DateTransition=null, include QueueRecords)
DB-->>WF: ProductTransition (Id)
SM->>Bull: enqueue Job(payload + transition)
Bull->>Worker: deliver job
Worker->>DB: create QueueRecords (ProductTransitionId, Queue, JobType, JobId) in transaction
DB-->>Worker: persisted record
Worker->>Worker: job.updateProgress(0) & set span attribute (queue/id)
Note over UI,DB: admin reads records
UI->>DB: load transitions + QueueRecords (if super-admin)
DB-->>UI: transitions with QueueRecords
UI->>UI: render links to admin queues
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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). (2)
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 |
FyreByrd
force-pushed
the
feature/track-jobs
branch
from
7c40fe3 to
b979742
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: 3
🧹 Nitpick comments (1)
src/lib/server/bullmq/BullWorker.ts (1)
31-52: LGTM! Robust queue record logging with idempotency.The guard checks and
skipDuplicates: truecorrectly handle job retries, preventing duplicate QueueRecords. The assertion on Line 37 is safe given the guard on Line 31.Minor note: The
.replace(/#/g, '%23')on Line 49 appears redundant sinceencodeURIComponentalready encodes#as%23. Consider simplifying:- `${encodeURIComponent(r.Queue)}/${encodeURIComponent(r.JobId).replace(/#/g, '%23')}` + `${encodeURIComponent(r.Queue)}/${encodeURIComponent(r.JobId)}`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/lib/bullmq.ts(1 hunks)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/16_track_jobs/migration.sql(1 hunks)src/lib/prisma/schema.prisma(1 hunks)src/lib/products/components/ProductDetails.svelte(4 hunks)src/lib/projects/sse.ts(2 hunks)src/lib/server/bullmq/BullWorker.ts(2 hunks)src/lib/server/bullmq/types.ts(10 hunks)src/lib/server/job-executors/build.ts(10 hunks)src/lib/server/job-executors/polling.ts(2 hunks)src/lib/server/job-executors/product.ts(8 hunks)src/lib/server/job-executors/publish.ts(11 hunks)src/lib/server/job-executors/userTasks.ts(1 hunks)src/lib/server/workflow/index.ts(4 hunks)src/lib/server/workflow/state-machine.ts(10 hunks)src/routes/(authenticated)/admin/settings/build-engines/+page.svelte(2 hunks)src/routes/(authenticated)/projects/[id=idNumber]/ProductCard.svelte(1 hunks)src/routes/(authenticated)/workflow-instances/[product_id]/+page.server.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1115
File: portal/node-server/BullWorker.ts:74-83
Timestamp: 2025-05-12T13:16:39.942Z
Learning: In BullMQ, using a fixed `jobId` provides idempotency for jobs. Without a `jobId`, each application restart will add a new job with a unique ID to the queue, causing startup jobs to accumulate. However, completed jobs are not automatically reprocessed - they remain in the completed state.
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1115
File: portal/node-server/BullWorker.ts:74-83
Timestamp: 2025-05-12T13:16:39.942Z
Learning: In BullMQ, using a fixed `jobId` provides idempotency for jobs. Without a `jobId`, each application restart will add a new job with a unique ID to the queue, causing startup jobs to accumulate over time. Completed jobs are not automatically reprocessed regardless of whether a `jobId` is specified.
📚 Learning: 2025-05-12T13:19:42.118Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1115
File: portal/common/bullmq/types.ts:205-220
Timestamp: 2025-05-12T13:19:42.118Z
Learning: In the AppBuilder portal, `System_CheckEngineStatuses` and `System_RefreshLangTags` job types are designed to run both periodically (as recurring jobs) and immediately upon startup, which is why they're included in both the `RecurringJob` and `StartupJob` type definitions.
Applied to files:
src/lib/bullmq.tssrc/lib/server/bullmq/types.ts
📚 Learning: 2025-09-12T14:57:08.647Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1283
File: src/routes/(authenticated)/tasks/[product_id]/+page.server.ts:252-264
Timestamp: 2025-09-12T14:57:08.647Z
Learning: In src/routes/(authenticated)/tasks/[product_id]/+page.server.ts, FyreByrd prefers to use transition.target[0] prediction mechanism for determining post-send state rather than calling flow.state() after flow.send() due to race condition concerns. The prediction approach is sufficient for redirect decisions between staying on task page vs going to projects page.
Applied to files:
src/lib/server/job-executors/product.tssrc/lib/server/workflow/state-machine.tssrc/lib/server/job-executors/userTasks.tssrc/lib/server/workflow/index.tssrc/routes/(authenticated)/workflow-instances/[product_id]/+page.server.tssrc/lib/products/components/ProductDetails.sveltesrc/routes/(authenticated)/projects/[id=idNumber]/ProductCard.sveltesrc/lib/projects/sse.tssrc/lib/server/job-executors/build.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.tssrc/lib/server/job-executors/userTasks.tssrc/lib/server/bullmq/types.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.tssrc/lib/server/bullmq/BullWorker.tssrc/lib/server/job-executors/build.tssrc/lib/server/bullmq/types.tssrc/lib/server/job-executors/publish.ts
📚 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/server/workflow/state-machine.tssrc/lib/server/job-executors/userTasks.tssrc/routes/(authenticated)/workflow-instances/[product_id]/+page.server.tssrc/routes/(authenticated)/projects/[id=idNumber]/ProductCard.svelte
📚 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-17T20:58:49.908Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1283
File: src/routes/(authenticated)/tasks/[product_id]/+page.svelte:48-71
Timestamp: 2025-09-17T20:58:49.908Z
Learning: In the codebase, ProductId fields are UUID strings, not numbers. In UserTasks model: `ProductId String db.Uuid` and in Products model: `Id String id db.Uuid`. Route parameters like page.params.product_id are also strings, so strict equality comparisons between ProductId and route parameters work correctly without type conversion.
Applied to files:
src/lib/prisma/schema.prisma
📚 Learning: 2025-08-04T18:34:58.818Z
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1186
File: src/hooks.server.ts:34-41
Timestamp: 2025-08-04T18:34:58.818Z
Learning: In the AppBuilder Portal codebase, BullMQ workers are only initialized when not building (`if (!building)` in BullWorker constructor), and the graceful shutdown handler in hooks.server.ts also only runs when not building. Therefore, the worker.worker property is guaranteed to be defined in the shutdown context, making optional chaining defensive rather than necessary.
Applied to files:
src/lib/server/bullmq/BullWorker.ts
📚 Learning: 2025-09-10T15:11:55.861Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1275
File: src/routes/(authenticated)/projects/[id=idNumber]/+page.svelte:221-221
Timestamp: 2025-09-10T15:11:55.861Z
Learning: In Svelte 5, component props are declared using interface Props and destructured from $props(), not with the export let syntax used in Svelte 4. For example, instead of "export let canEdit: boolean = false;", Svelte 5 uses interface Props { canEdit: boolean; } and let { canEdit }: Props = $props();
Applied to files:
src/routes/(authenticated)/projects/[id=idNumber]/ProductCard.svelte
📚 Learning: 2025-09-17T21:07:18.139Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1275
File: src/lib/projects/index.ts:183-201
Timestamp: 2025-09-17T21:07:18.139Z
Learning: In the AppBuilder portal project, org admins must be in the project group to claim/take ownership of projects. This is enforced both in client-side logic (canClaimProject function) and server-side validation.
Applied to files:
src/lib/projects/sse.ts
🧬 Code graph analysis (7)
src/lib/server/workflow/state-machine.ts (1)
src/lib/server/bullmq/queues.ts (1)
getQueues(150-155)
src/lib/server/workflow/index.ts (2)
src/lib/server/database/index.ts (1)
DatabaseWrites(68-71)src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)
src/lib/server/bullmq/BullWorker.ts (1)
src/lib/server/database/index.ts (1)
DatabaseWrites(68-71)
src/lib/projects/sse.ts (1)
src/lib/utils/roles.ts (1)
isSuperAdmin(33-35)
src/lib/server/job-executors/build.ts (2)
src/lib/server/job-executors/publish.ts (1)
product(10-161)src/lib/server/build-engine-api/types.ts (1)
BuildResponse(163-171)
src/lib/server/bullmq/types.ts (1)
src/lib/server/build-engine-api/types.ts (1)
Channels(177-177)
src/lib/server/job-executors/publish.ts (2)
src/lib/server/job-executors/build.ts (1)
product(11-134)src/lib/server/build-engine-api/types.ts (1)
ReleaseResponse(186-195)
⏰ 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: checks / build-and-test
- GitHub Check: check / build-and-test
🔇 Additional comments (17)
src/lib/locales/es-419.json (1)
227-227: LGTM - Spanish translation is correct.The Spanish translation "Registros de trabajo" for "Job Records" is accurate and appropriately placed.
src/lib/locales/en-US.json (1)
227-227: LGTM - English translation is clear.The "Job Records" label is clear and follows the existing naming conventions.
src/lib/bullmq.ts (1)
1-59: Well-structured enum definitions for BullMQ job metadata.The three enums provide clear type-safe identifiers for queues, job types, and schedulers. The JobType categories are well-organized, making the codebase more maintainable.
src/lib/server/job-executors/polling.ts (2)
44-53: Transitions metadata correctly propagated through build workflow.The transitions data is appropriately passed to the Build PostProcess job, maintaining the audit trail through the workflow pipeline.
100-109: Transitions metadata correctly propagated through publish workflow.Consistent with the build workflow, transitions data is passed to the Publish PostProcess job for tracking purposes.
src/lib/server/job-executors/userTasks.ts (1)
38-51: Queue records protection added to cleanup logic.The additional
QueueRecords: { none: {} }condition ensures that ProductTransitions with associated job metadata are preserved during PreExecuteEntries cleanup. This prevents accidental deletion of transitions that have recorded job execution history.src/routes/(authenticated)/workflow-instances/[product_id]/+page.server.ts (1)
91-97: Queue records data added to transitions query.Including QueueRecords with specific fields (Queue, JobId, JobType) enables the UI to display job metadata for each transition, supporting the debugging use case outlined in issue #1375.
src/routes/(authenticated)/admin/settings/build-engines/+page.svelte (2)
3-3: Appropriate use of new QueueName enum.Importing QueueName from the new centralized enums file promotes consistency across the codebase.
45-53: Improved UX with actionable link in empty state.The empty state now provides a helpful link to the "Check System Statuses" recurring job, making it easier for admins to diagnose why build engine information is missing. The use of QueueName.System_Recurring ensures consistency with queue definitions.
src/lib/projects/sse.ts (1)
162-170: LGTM! Secure admin-only access control.The conditional inclusion of QueueRecords based on
isSuperAdmincorrectly restricts queue metadata visibility to super admins only. This aligns with the PR objective of providing admin-level job search/tracking capabilities.src/lib/prisma/migrations/16_track_jobs/migration.sql (1)
1-15: LGTM! Well-designed schema with appropriate constraints.The composite primary key on
(Queue, JobId)correctly enforces uniqueness per queue while allowing the same JobId across different queues. The CASCADE delete ensures cleanup when transitions are removed, and the index onProductTransitionIdsupports efficient queries.src/lib/server/job-executors/product.ts (2)
50-51: LGTM! Consistent transitions propagation.All notification paths in the
createfunction correctly propagatejob.data.transitionsto their respective notification calls, ensuring traceability from jobs to notifications.Also applies to: 75-76, 84-85, 100-101
262-337: LGTM! Consistent notification helper signatures.All notification helpers use optional
transitions?: number[]parameters and consistently include them in email job payloads, maintaining backward compatibility while enabling job tracking.src/lib/server/workflow/state-machine.ts (1)
302-311: LGTM! Consistent async transition tracking across workflow states.The async entry/action pattern correctly:
- Awaits
Workflow.currentProductTransitionto capture the transition ID at state entry time- Transforms null results to undefined (safe for optional field)
- Passes transitions to job payloads for downstream tracking
The xstate v5 model properly handles async actions in entry arrays, and the workflow correctly remains in each state until jobs complete and trigger the next transition.
Also applies to: 504-527, 664-693, 784-792, 800-820, 1054-1063
src/lib/server/job-executors/publish.ts (1)
94-95: LGTM! Consistent transitions propagation in publish workflow.All notification and polling paths correctly propagate
transitionsthrough the publish executor, maintaining consistency with the patterns established inbuild.tsandproduct.ts.Also applies to: 102-103, 142-143, 209-210, 239-245, 273-384
src/lib/server/job-executors/build.ts (1)
71-72: LGTM! Consistent transitions propagation in build workflow.The build executor correctly propagates
transitionsthrough all notification and polling paths, maintaining the established pattern across all executors.Also applies to: 79-80, 115-116, 278-279, 283-289, 304-413
src/routes/(authenticated)/projects/[id=idNumber]/ProductCard.svelte (1)
16-18: LGTM! Good refactoring to shared type.Importing the
Transitiontype fromProductDetails.svelteensures type consistency across components and establishes a single source of truth for the Transition shape.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/products/components/ProductDetails.svelte (1)
167-202: Consider adding QueueRecords to mobile view.The desktop table (lines 106-166) includes the QueueRecords section for admin debugging, but the mobile table completely omits this functionality. Admin users on mobile devices won't be able to access job records for debugging purposes.
Since this feature aids debugging (linking to
/admin/jobs/...), consider adding a similar collapsible section to the mobile table layout, perhaps after line 194 or within the existing transition row structure.
♻️ Duplicate comments (1)
src/lib/products/components/ProductDetails.svelte (1)
8-26: Missing Prisma import in module script.This issue was already flagged in a previous review. The
Prismatype is used but not imported in the module script, causing a type-checking failure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/products/components/ProductDetails.svelte(4 hunks)src/lib/server/bullmq/BullWorker.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/server/bullmq/BullWorker.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1186
File: src/hooks.server.ts:34-41
Timestamp: 2025-08-04T18:34:58.818Z
Learning: In the AppBuilder Portal codebase, BullMQ workers are only initialized when not building (`if (!building)` in BullWorker constructor), and the graceful shutdown handler in hooks.server.ts also only runs when not building. Therefore, the worker.worker property is guaranteed to be defined in the shutdown context, making optional chaining defensive rather than necessary.
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1115
File: portal/node-server/BullWorker.ts:74-83
Timestamp: 2025-05-12T13:16:39.942Z
Learning: In BullMQ, using a fixed `jobId` provides idempotency for jobs. Without a `jobId`, each application restart will add a new job with a unique ID to the queue, causing startup jobs to accumulate. However, completed jobs are not automatically reprocessed - they remain in the completed state.
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1115
File: portal/node-server/BullWorker.ts:74-83
Timestamp: 2025-05-12T13:16:39.942Z
Learning: In BullMQ, using a fixed `jobId` provides idempotency for jobs. Without a `jobId`, each application restart will add a new job with a unique ID to the queue, causing startup jobs to accumulate over time. Completed jobs are not automatically reprocessed regardless of whether a `jobId` is specified.
📚 Learning: 2025-09-12T14:57:08.647Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1283
File: src/routes/(authenticated)/tasks/[product_id]/+page.server.ts:252-264
Timestamp: 2025-09-12T14:57:08.647Z
Learning: In src/routes/(authenticated)/tasks/[product_id]/+page.server.ts, FyreByrd prefers to use transition.target[0] prediction mechanism for determining post-send state rather than calling flow.state() after flow.send() due to race condition concerns. The prediction approach is sufficient for redirect decisions between staying on task page vs going to projects page.
Applied to files:
src/lib/products/components/ProductDetails.svelte
⏰ 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: checks / build-and-test
- GitHub Check: check / build-and-test
🔇 Additional comments (2)
src/lib/products/components/ProductDetails.svelte (2)
44-44: Good refactoring to use the exported Transition type.This improves type reusability and allows other components to import and use the same type definition.
145-145: URL encoding correctly implemented.The code properly uses
encodeURIComponentto encode the job ID, which correctly handles all reserved characters including/,?, and#. This resolves the concern from the previous review.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/server/bullmq/BullWorker.ts (1)
48-48: Remove unnecessaryencodeURIComponentin span attribute.The Queue and JobId values are being encoded with
encodeURIComponent, but span attributes are not URLs and don't require encoding. Queue names and JobIds are already safe strings from the system.span.setAttribute( 'job.records', - records.map((r) => `${encodeURIComponent(r.Queue)}/${encodeURIComponent(r.JobId)}`) + records.map((r) => `${r.Queue}/${r.JobId}`) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/server/bullmq/BullWorker.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1186
File: src/hooks.server.ts:34-41
Timestamp: 2025-08-04T18:34:58.818Z
Learning: In the AppBuilder Portal codebase, BullMQ workers are only initialized when not building (`if (!building)` in BullWorker constructor), and the graceful shutdown handler in hooks.server.ts also only runs when not building. Therefore, the worker.worker property is guaranteed to be defined in the shutdown context, making optional chaining defensive rather than necessary.
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1115
File: portal/node-server/BullWorker.ts:74-83
Timestamp: 2025-05-12T13:16:39.942Z
Learning: In BullMQ, using a fixed `jobId` provides idempotency for jobs. Without a `jobId`, each application restart will add a new job with a unique ID to the queue, causing startup jobs to accumulate. However, completed jobs are not automatically reprocessed - they remain in the completed state.
📚 Learning: 2025-08-04T18:34:58.818Z
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1186
File: src/hooks.server.ts:34-41
Timestamp: 2025-08-04T18:34:58.818Z
Learning: In the AppBuilder Portal codebase, BullMQ workers are only initialized when not building (`if (!building)` in BullWorker constructor), and the graceful shutdown handler in hooks.server.ts also only runs when not building. Therefore, the worker.worker property is guaranteed to be defined in the shutdown context, making optional chaining defensive rather than necessary.
Applied to files:
src/lib/server/bullmq/BullWorker.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/bullmq/BullWorker.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/bullmq/BullWorker.ts
🧬 Code graph analysis (1)
src/lib/server/bullmq/BullWorker.ts (1)
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: checks / build-and-test
- GitHub Check: check / build-and-test
🔇 Additional comments (2)
src/lib/server/bullmq/BullWorker.ts (2)
32-50: Verify database write failure handling strategy.If
DatabaseWrites.queueRecords.createManyAndReturnfails (e.g., database unavailable), the entire job will fail and throw. This couples job execution success to metadata tracking success.Please confirm this is the intended behavior. If metadata tracking should be non-blocking, consider wrapping the database operation in a try-catch that logs errors but allows job execution to proceed.
53-60: Good addition of explicit error rethrowing.The error is properly recorded in the span, logged to console, and then rethrown. This ensures BullMQ can handle job failures correctly (retries, dead letter queue, etc.) while maintaining observability.
There was a problem hiding this comment.
Actionable comments posted: 1
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/product.ts (1)
324-337: Restore BuildEngine URL derivation
BuildEngine.Requests.getURLandTokenreturns an object; concatenating it directly yields"[object Object]/job-admin", so every failure email/link will break. Pull out the.urlbefore appending the path.- const endpoint = await BuildEngine.Requests.getURLandToken(organizationId); - const buildEngineUrl = endpoint + '/job-admin'; + const { url } = await BuildEngine.Requests.getURLandToken(organizationId); + const buildEngineUrl = url + '/job-admin';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/lib/server/bullmq/BullWorker.ts(2 hunks)src/lib/server/bullmq/queues.ts(2 hunks)src/lib/server/bullmq/types.ts(10 hunks)src/lib/server/job-executors/build.ts(10 hunks)src/lib/server/job-executors/polling.ts(2 hunks)src/lib/server/job-executors/product.ts(8 hunks)src/lib/server/job-executors/publish.ts(11 hunks)src/lib/server/workflow/index.ts(6 hunks)src/lib/server/workflow/state-machine.ts(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/server/job-executors/build.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1115
File: portal/node-server/BullWorker.ts:74-83
Timestamp: 2025-05-12T13:16:39.942Z
Learning: In BullMQ, using a fixed `jobId` provides idempotency for jobs. Without a `jobId`, each application restart will add a new job with a unique ID to the queue, causing startup jobs to accumulate. However, completed jobs are not automatically reprocessed - they remain in the completed state.
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1186
File: src/hooks.server.ts:34-41
Timestamp: 2025-08-04T18:34:58.818Z
Learning: In the AppBuilder Portal codebase, BullMQ workers are only initialized when not building (`if (!building)` in BullWorker constructor), and the graceful shutdown handler in hooks.server.ts also only runs when not building. Therefore, the worker.worker property is guaranteed to be defined in the shutdown context, making optional chaining defensive rather than necessary.
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1115
File: portal/node-server/BullWorker.ts:74-83
Timestamp: 2025-05-12T13:16:39.942Z
Learning: In BullMQ, using a fixed `jobId` provides idempotency for jobs. Without a `jobId`, each application restart will add a new job with a unique ID to the queue, causing startup jobs to accumulate over time. Completed jobs are not automatically reprocessed regardless of whether a `jobId` is specified.
📚 Learning: 2025-09-12T14:57:08.647Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1283
File: src/routes/(authenticated)/tasks/[product_id]/+page.server.ts:252-264
Timestamp: 2025-09-12T14:57:08.647Z
Learning: In src/routes/(authenticated)/tasks/[product_id]/+page.server.ts, FyreByrd prefers to use transition.target[0] prediction mechanism for determining post-send state rather than calling flow.state() after flow.send() due to race condition concerns. The prediction approach is sufficient for redirect decisions between staying on task page vs going to projects page.
Applied to files:
src/lib/server/workflow/state-machine.tssrc/lib/server/job-executors/product.tssrc/lib/server/job-executors/publish.tssrc/lib/server/workflow/index.ts
📚 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/server/workflow/state-machine.ts
📚 Learning: 2025-08-04T18:34:58.818Z
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1186
File: src/hooks.server.ts:34-41
Timestamp: 2025-08-04T18:34:58.818Z
Learning: In the AppBuilder Portal codebase, BullMQ workers are only initialized when not building (`if (!building)` in BullWorker constructor), and the graceful shutdown handler in hooks.server.ts also only runs when not building. Therefore, the worker.worker property is guaranteed to be defined in the shutdown context, making optional chaining defensive rather than necessary.
Applied to files:
src/lib/server/bullmq/queues.tssrc/lib/server/bullmq/BullWorker.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/bullmq/queues.tssrc/lib/server/bullmq/BullWorker.tssrc/lib/server/bullmq/types.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/bullmq/queues.tssrc/lib/server/job-executors/publish.tssrc/lib/server/bullmq/types.ts
📚 Learning: 2025-05-12T13:19:42.118Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1115
File: portal/common/bullmq/types.ts:205-220
Timestamp: 2025-05-12T13:19:42.118Z
Learning: In the AppBuilder portal, `System_CheckEngineStatuses` and `System_RefreshLangTags` job types are designed to run both periodically (as recurring jobs) and immediately upon startup, which is why they're included in both the `RecurringJob` and `StartupJob` type definitions.
Applied to files:
src/lib/server/bullmq/types.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 (6)
src/lib/server/workflow/state-machine.ts (2)
src/lib/server/bullmq/queues.ts (1)
getQueues(199-204)src/lib/workflowTypes.ts (1)
isDeprecated(78-80)
src/lib/server/bullmq/queues.ts (4)
src/lib/server/bullmq/types.ts (2)
Job(247-247)BaseJob(22-26)src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)src/lib/server/workflow/index.ts (1)
Workflow(36-637)src/lib/server/database/index.ts (1)
DatabaseWrites(68-71)
src/lib/server/bullmq/BullWorker.ts (1)
src/lib/otel/index.ts (1)
error(46-53)
src/lib/server/job-executors/publish.ts (2)
src/lib/server/job-executors/build.ts (1)
product(11-134)src/lib/server/build-engine-api/types.ts (1)
ReleaseResponse(186-195)
src/lib/server/bullmq/types.ts (1)
src/lib/server/build-engine-api/types.ts (1)
Channels(177-177)
src/lib/server/workflow/index.ts (2)
src/lib/server/database/index.ts (1)
DatabaseWrites(68-71)src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)
🔇 Additional comments (2)
src/lib/server/workflow/index.ts (1)
519-546: Nice encapsulation for transition lookup.
Pulling the “active or latest” transition intoWorkflow.currentProductTransitionkeeps the queue/record plumbing centralized and cuts down on duplication across the new BullMQ call sites.src/lib/server/job-executors/publish.ts (1)
239-383: Threading transition IDs into notifications pays off.
Plumbing the optional transition through every publish notification lets BullBoard/job details link straight back to the workflow record without extra lookups—great for the debugging experience the PR is aiming for.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib/server/bullmq/queues.ts (1)
125-138: Fallback recovery logic adds complexity and could mask issues.The fallback recovery mechanism attempts to find the current product transition when the original transition ID is stale or invalid. While this adds resilience, consider:
- Masking issues: This could hide bugs where incorrect transition IDs are being passed through the workflow
- Context accuracy:
Workflow.currentProductTransitionreturns the most recent transition, which may not match the job's actual context if transitions have progressed- Debugging trade-off: For debugging purposes, having some record is better than none, but the recovered transition might not accurately reflect what was intended
This approach seems reasonable for the PR's debugging objectives, but document this behavior and consider logging warnings when fallbacks occur to help identify underlying issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/server/bullmq/queues.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1115
File: portal/node-server/BullWorker.ts:74-83
Timestamp: 2025-05-12T13:16:39.942Z
Learning: In BullMQ, using a fixed `jobId` provides idempotency for jobs. Without a `jobId`, each application restart will add a new job with a unique ID to the queue, causing startup jobs to accumulate. However, completed jobs are not automatically reprocessed - they remain in the completed state.
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1186
File: src/hooks.server.ts:34-41
Timestamp: 2025-08-04T18:34:58.818Z
Learning: In the AppBuilder Portal codebase, BullMQ workers are only initialized when not building (`if (!building)` in BullWorker constructor), and the graceful shutdown handler in hooks.server.ts also only runs when not building. Therefore, the worker.worker property is guaranteed to be defined in the shutdown context, making optional chaining defensive rather than necessary.
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1115
File: portal/node-server/BullWorker.ts:74-83
Timestamp: 2025-05-12T13:16:39.942Z
Learning: In BullMQ, using a fixed `jobId` provides idempotency for jobs. Without a `jobId`, each application restart will add a new job with a unique ID to the queue, causing startup jobs to accumulate over time. Completed jobs are not automatically reprocessed regardless of whether a `jobId` is specified.
📚 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/bullmq/queues.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/bullmq/queues.ts
📚 Learning: 2025-08-04T18:34:58.818Z
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1186
File: src/hooks.server.ts:34-41
Timestamp: 2025-08-04T18:34:58.818Z
Learning: In the AppBuilder Portal codebase, BullMQ workers are only initialized when not building (`if (!building)` in BullWorker constructor), and the graceful shutdown handler in hooks.server.ts also only runs when not building. Therefore, the worker.worker property is guaranteed to be defined in the shutdown context, making optional chaining defensive rather than necessary.
Applied to files:
src/lib/server/bullmq/queues.ts
📚 Learning: 2025-09-12T14:57:08.647Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1283
File: src/routes/(authenticated)/tasks/[product_id]/+page.server.ts:252-264
Timestamp: 2025-09-12T14:57:08.647Z
Learning: In src/routes/(authenticated)/tasks/[product_id]/+page.server.ts, FyreByrd prefers to use transition.target[0] prediction mechanism for determining post-send state rather than calling flow.state() after flow.send() due to race condition concerns. The prediction approach is sufficient for redirect decisions between staying on task page vs going to projects page.
Applied to files:
src/lib/server/bullmq/queues.ts
🧬 Code graph analysis (1)
src/lib/server/bullmq/queues.ts (1)
src/lib/server/bullmq/types.ts (2)
Job(247-247)BaseJob(22-26)
⏰ 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). (3)
- GitHub Check: checks / build-and-test
- GitHub Check: checks / typecheck-lint
- GitHub Check: check / build-and-test
🔇 Additional comments (5)
src/lib/server/bullmq/queues.ts (5)
1-21: LGTM: Imports support the new job record tracking.The new imports (
prismaInternal,Workflow, andBaseJob) are necessary for the job record creation functionality and are used appropriately throughout the file.
120-148: Transaction client usage is now correct.The previous critical issue regarding transaction client usage has been resolved. The code now correctly uses the
txparameter throughout the transaction callback (lines 121, 140) instead of the root Prisma clients.
117-154: Verify async event handler behavior is intentional.The
createJobRecordfunction is registered as an event handler for thewaitingevent but is not awaited. This means:
- Job processing begins immediately without waiting for the record to be created
- If record creation fails or is slow, the job may complete before a record exists
- This is likely intentional to avoid blocking job processing, but could lead to missing records in edge cases
Please confirm this fire-and-forget behavior aligns with your debugging requirements.
136-136: Consider implications of mutating job data in Redis.The call to
job.updateData()modifies the job's persisted data in Redis, replacing the stale transition ID with the recovered one. This means:
- Future reads or retries will see the corrected transition ID
- The original (incorrect) transition ID is lost
- If multiple workers process the same job concurrently, there could be race conditions
While this ensures consistency going forward, consider whether you need to preserve the original transition ID for debugging purposes (e.g., logging it before the update).
162-182: Event handler registrations align with PR objectives.The
createJobRecordhandler is correctly registered on the relevant queues (Builds, Products, Publishing, Polling, Emails) and intentionally excluded from others (SystemRecurring, SystemStartup, Projects, UserTasks, SvelteSSE), matching the PR's stated tracking scope.
FyreByrd
force-pushed
the
feature/track-jobs
branch
from
87d45e2 to
ea31a38
Compare
|
Fixed merge conflicts |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/server/job-executors/product.ts (1)
324-337: Bug:endpointis an object, not a string.
BuildEngine.Requests.getURLandTokenreturns an object withurlandtokenproperties. Concatenating it directly with a string will produce"[object Object]/job-admin".Apply this diff to fix:
const endpoint = await BuildEngine.Requests.getURLandToken(organizationId); - const buildEngineUrl = endpoint + '/job-admin'; + const buildEngineUrl = endpoint.url + '/job-admin';src/lib/server/workflow/state-machine.ts (1)
302-319: XState entry actions are not awaited—useinvokefor async work.Entry actions in XState are fire-and-forget and do not wait for async operations to complete. The
asyncentry handler here will start the job queue operation but the state machine will continue immediately, creating a race condition where:
- The state transitions before the job is guaranteed to be queued
- The transition ID from
Workflow.currentProductTransitionmay not be reliable if timing variesFor async operations that must complete before or coordinate with state transitions, use XState's
invokepattern (services/actors) instead of entry actions, or ensure this fire-and-forget behavior is intentional and has no ordering requirements.
♻️ Duplicate comments (2)
src/lib/products/components/ProductDetails.svelte (2)
8-26: Add the missingPrismaimport in the module script.The
Transitiontype usesPrisma.ProductTransitionsGetPayload, butPrismais only imported in the instance script (line 30), not the module script. Types in Svelte module scripts need their own imports.Apply this diff at the top of the module script:
<script lang="ts" module> + import type { Prisma } from '@prisma/client'; + /** Shows details modal for product with productId */
84-91: Add security attributes to the external link.Links with
target="_blank"should includerel="noopener noreferrer"to prevent the new page from accessingwindow.opener.<a class="link" href="/admin/jobs/queue/{rec.Queue}/{encodeURIComponent(rec.JobId)}" target="_blank" + rel="noopener noreferrer" >
🧹 Nitpick comments (3)
src/lib/server/bullmq/queues.ts (1)
117-154: Transaction client usage is now correct.The function properly uses the
txclient provided by$transactionfor bothfindFirstandcreateoperations. This addresses the previous review feedback.One observation: after
job.updateData()on line 136, the code usesjob.data.transition!on line 142. SinceupdateDatais awaited and BullMQ updatesjob.datasynchronously after the Redis write, this should work correctly. However, usingfounddirectly would be clearer:} else { job.log(`Transition ${job.data.transition} not found. Replacing with ${found}`); job.updateData({ ...job.data, transition: found }); } } return await tx.queueRecords.create({ data: { - ProductTransitionId: job.data.transition!, + ProductTransitionId: existing?.Id ?? found!, Queue: job.queueName, JobType: job.data.type, JobId: job.id! // this is in fact defined (checked in above if) } });Alternatively, capture the resolved transition ID before the conditional:
+ const transitionId = existing?.Id ?? found; + if (!transitionId) { + job.log('Error recovering transition. No job record created.'); + console.error(`Error recovering transition ${job.data.transition}`); + return; + } + return await tx.queueRecords.create({ data: { - ProductTransitionId: job.data.transition!, + ProductTransitionId: transitionId, ...src/lib/server/workflow/state-machine.ts (1)
664-673: Consider extracting repeated transition-fetching pattern.The pattern for fetching transition ID is duplicated multiple times:
transition: await Workflow.currentProductTransition( context.productId, WorkflowState.Create_App_Store_Entry ).then((pt) => pt?.Id ?? undefined)A helper function could reduce repetition:
const getTransitionId = async (productId: string, state: WorkflowState) => Workflow.currentProductTransition(productId, state).then((pt) => pt?.Id);src/lib/server/workflow/index.ts (1)
519-547: Consider combining queries incurrentProductTransition.The method performs two separate database queries that could potentially be combined into one:
public static async currentProductTransition(ProductId: string, InitialState?: string) { const transitions = await DatabaseReads.productTransitions.findMany({ where: { ProductId, InitialState }, select: { Id: true, DateTransition: true }, orderBy: [ { DateTransition: { sort: 'asc', nulls: 'first' } } ], take: 1 }); return transitions.at(0) ?? null; }This would reduce database round-trips. However, if the current separation is intentional for clarity or specific ordering requirements, the existing approach is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/lib/bullmq.ts(1 hunks)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/16_track_jobs/migration.sql(1 hunks)src/lib/prisma/schema.prisma(1 hunks)src/lib/products/components/ProductDetails.svelte(7 hunks)src/lib/projects/sse.ts(2 hunks)src/lib/server/bullmq/BullWorker.ts(2 hunks)src/lib/server/bullmq/queues.ts(2 hunks)src/lib/server/bullmq/types.ts(10 hunks)src/lib/server/job-executors/build.ts(10 hunks)src/lib/server/job-executors/polling.ts(2 hunks)src/lib/server/job-executors/product.ts(8 hunks)src/lib/server/job-executors/publish.ts(11 hunks)src/lib/server/job-executors/userTasks.ts(1 hunks)src/lib/server/workflow/index.ts(6 hunks)src/lib/server/workflow/state-machine.ts(10 hunks)src/routes/(authenticated)/admin/settings/build-engines/+page.svelte(2 hunks)src/routes/(authenticated)/projects/[id=idNumber]/ProductCard.svelte(1 hunks)src/routes/(authenticated)/workflow-instances/[product_id]/+page.server.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- src/lib/locales/es-419.json
- src/lib/projects/sse.ts
- src/lib/locales/fr-FR.json
- src/lib/server/job-executors/build.ts
- src/routes/(authenticated)/admin/settings/build-engines/+page.svelte
- src/lib/server/job-executors/userTasks.ts
- src/lib/server/job-executors/publish.ts
- src/routes/(authenticated)/workflow-instances/[product_id]/+page.server.ts
- src/lib/server/bullmq/BullWorker.ts
- src/lib/prisma/migrations/16_track_jobs/migration.sql
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1115
File: portal/node-server/BullWorker.ts:74-83
Timestamp: 2025-05-12T13:16:39.942Z
Learning: In BullMQ, using a fixed `jobId` provides idempotency for jobs. Without a `jobId`, each application restart will add a new job with a unique ID to the queue, causing startup jobs to accumulate. However, completed jobs are not automatically reprocessed - they remain in the completed state.
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1186
File: src/hooks.server.ts:34-41
Timestamp: 2025-08-04T18:34:58.818Z
Learning: In the AppBuilder Portal codebase, BullMQ workers are only initialized when not building (`if (!building)` in BullWorker constructor), and the graceful shutdown handler in hooks.server.ts also only runs when not building. Therefore, the worker.worker property is guaranteed to be defined in the shutdown context, making optional chaining defensive rather than necessary.
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1115
File: portal/node-server/BullWorker.ts:74-83
Timestamp: 2025-05-12T13:16:39.942Z
Learning: In BullMQ, using a fixed `jobId` provides idempotency for jobs. Without a `jobId`, each application restart will add a new job with a unique ID to the queue, causing startup jobs to accumulate over time. Completed jobs are not automatically reprocessed regardless of whether a `jobId` is specified.
📚 Learning: 2025-09-12T14:57:08.647Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1283
File: src/routes/(authenticated)/tasks/[product_id]/+page.server.ts:252-264
Timestamp: 2025-09-12T14:57:08.647Z
Learning: In src/routes/(authenticated)/tasks/[product_id]/+page.server.ts, FyreByrd prefers to use transition.target[0] prediction mechanism for determining post-send state rather than calling flow.state() after flow.send() due to race condition concerns. The prediction approach is sufficient for redirect decisions between staying on task page vs going to projects page.
Applied to files:
src/lib/products/components/ProductDetails.sveltesrc/lib/server/bullmq/queues.tssrc/lib/server/workflow/state-machine.tssrc/lib/server/job-executors/product.tssrc/routes/(authenticated)/projects/[id=idNumber]/ProductCard.sveltesrc/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/bullmq/queues.tssrc/lib/server/job-executors/product.tssrc/lib/server/bullmq/types.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/bullmq/queues.tssrc/lib/server/bullmq/types.ts
📚 Learning: 2025-08-04T18:34:58.818Z
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1186
File: src/hooks.server.ts:34-41
Timestamp: 2025-08-04T18:34:58.818Z
Learning: In the AppBuilder Portal codebase, BullMQ workers are only initialized when not building (`if (!building)` in BullWorker constructor), and the graceful shutdown handler in hooks.server.ts also only runs when not building. Therefore, the worker.worker property is guaranteed to be defined in the shutdown context, making optional chaining defensive rather than necessary.
Applied to files:
src/lib/server/bullmq/queues.ts
📚 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/server/workflow/state-machine.tssrc/routes/(authenticated)/projects/[id=idNumber]/ProductCard.svelte
📚 Learning: 2025-09-17T20:58:49.908Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1283
File: src/routes/(authenticated)/tasks/[product_id]/+page.svelte:48-71
Timestamp: 2025-09-17T20:58:49.908Z
Learning: In the codebase, ProductId fields are UUID strings, not numbers. In UserTasks model: `ProductId String db.Uuid` and in Products model: `Id String id db.Uuid`. Route parameters like page.params.product_id are also strings, so strict equality comparisons between ProductId and route parameters work correctly without type conversion.
Applied to files:
src/lib/prisma/schema.prisma
📚 Learning: 2025-05-12T13:19:42.118Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1115
File: portal/common/bullmq/types.ts:205-220
Timestamp: 2025-05-12T13:19:42.118Z
Learning: In the AppBuilder portal, `System_CheckEngineStatuses` and `System_RefreshLangTags` job types are designed to run both periodically (as recurring jobs) and immediately upon startup, which is why they're included in both the `RecurringJob` and `StartupJob` type definitions.
Applied to files:
src/lib/bullmq.tssrc/lib/server/bullmq/types.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 (5)
src/lib/server/bullmq/queues.ts (1)
src/lib/server/bullmq/types.ts (10)
Job(248-248)BaseJob(22-26)BuildJob(250-250)RecurringJob(251-253)StartupJob(254-257)ProductJob(274-278)PublishJob(258-258)PollJob(259-259)UserTasksJob(260-260)EmailJob(261-270)
src/lib/server/workflow/state-machine.ts (3)
src/lib/server/bullmq/queues.ts (1)
getQueues(198-203)src/lib/server/workflow/index.ts (1)
Workflow(36-637)src/lib/workflowTypes.ts (1)
isDeprecated(78-80)
src/lib/bullmq.ts (1)
src/lib/server/bullmq/types.ts (3)
QueueName(6-6)JobType(6-6)JobSchedulerId(6-6)
src/lib/server/bullmq/types.ts (1)
src/lib/server/build-engine-api/types.ts (1)
Channels(171-171)
src/lib/server/workflow/index.ts (2)
src/lib/server/database/index.ts (1)
DatabaseWrites(68-71)src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)
⏰ 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). (4)
- GitHub Check: checks / typecheck-lint
- GitHub Check: checks / build-and-test
- GitHub Check: check / typecheck-lint
- GitHub Check: check / build-and-test
🔇 Additional comments (22)
src/lib/locales/en-US.json (1)
207-207: LGTM!The new locale key follows the existing naming convention and is placed appropriately after
products_details.src/lib/server/job-executors/polling.ts (2)
44-53: LGTM!The
transitionfield is correctly propagated from the polling job data to theBuild_PostProcesspayload, maintaining the chain of transition tracking through the workflow.
100-109: LGTM!Consistent transition propagation to the
Publish_PostProcesspayload.src/lib/server/job-executors/product.ts (7)
46-52: LGTM!The
transitionparameter is correctly passed to the notification helper.
71-77: LGTM!Consistent transition propagation to the
notifyConnectionFailedhelper.
79-86: LGTM!Consistent transition propagation to the
notifyFailedhelper.
96-102: LGTM!Consistent transition propagation to the
notifyCreatedhelper.
262-278: LGTM!The function signature and payload structure are updated consistently to include the optional
transitionparameter.
280-296: LGTM!Consistent implementation with other notification helpers.
298-314: LGTM!Consistent implementation with other notification helpers.
src/lib/server/bullmq/queues.ts (1)
161-182: LGTM!The
createJobRecordhandler is correctly attached to the relevant queues (Builds, Products, Publishing, Polling, Emails) while intentionally excluding system, project, user tasks, and SSE queues as per the PR objectives.src/lib/products/components/ProductDetails.svelte (3)
78-95: LGTM on functionality!The
queueRecordssnippet correctly renders a collapsible list of job links with proper URL encoding. The use of<details>/<summary>provides a clean collapsible UI.
154-160: LGTM!The QueueRecords are correctly integrated into the desktop transition table with proper
colspanand border handling.
201-207: LGTM!Consistent mobile view integration matching the desktop implementation.
src/routes/(authenticated)/projects/[id=idNumber]/ProductCard.svelte (1)
16-19: LGTM!Centralizing the
Transitiontype definition inProductDetails.svelteand importing it here eliminates type duplication and improves maintainability.src/lib/prisma/schema.prisma (1)
280-289: LGTM!The
QueueRecordsmodel is well-designed:
- Composite primary key
(Queue, JobId)correctly enforces uniqueness per queue- Cascade delete ensures cleanup when parent
ProductTransitionsare removed- Index on
ProductTransitionIdoptimizes transition-based lookupssrc/lib/server/workflow/index.ts (2)
338-341: LGTM!The addition of
QueueRecords: { none: {} }to the delete filter correctly preserves transitions that have associated job records, ensuring job tracking data is not lost during cleanup.
481-516: LGTM!The logic to prevent duplicate transition entries for states with existing job records is sound. Querying for transitions with
QueueRecords: { some: {} }and filtering them out avoids creating redundant entries while preserving job tracking integrity.src/lib/bullmq.ts (1)
1-59: LGTM!Well-organized enum definitions centralized for shared access between client and server code. The human-readable string values aid debugging and log readability. The categorization with comments improves maintainability.
src/lib/server/bullmq/types.ts (3)
5-6: LGTM - Good centralization of shared enums.Moving the enum definitions to a shared location (
$lib/bullmq) improves code organization and reduces duplication.
22-26: LGTM - Infrastructure for job tracking.The
BaseJobinterface provides uniform infrastructure for the job tracking feature. WhileproductIdandtransitionaren't used by all job types (System, Email, SSE, etc.), having optional fields available across all jobs simplifies the tracking implementation without causing issues.
248-311: LGTM - Comprehensive type mappings.The
JobTypeMapand union types provide excellent type safety for job handling throughout the codebase.
|
Still more race conditions? |
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
Still need to hook in transitionId... Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
FyreByrd
force-pushed
the
feature/track-jobs
branch
from
ea31a38 to
ddae65d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/lib/products/components/ProductDetails.svelte (2)
8-26: Add the missing Prisma import in the module script.
Prismais referenced in the module script (lines 8-26) but only imported in the instance script (line 30). In Svelte, module and instance scripts have separate scopes, so this will fail type-checking.Apply this diff at the top of the module script:
<script lang="ts" module> + import type { Prisma } from '@prisma/client'; + /** Shows details modal for product with productId */
84-90: Add security attributes to external link.Links with
target="_blank"should includerel="noopener noreferrer"to prevent potential security issues withwindow.openeraccess.Apply this diff:
<a class="link" href="/admin/jobs/queue/{rec.Queue}/{encodeURIComponent(rec.JobId)}" target="_blank" + rel="noopener noreferrer" >src/lib/server/bullmq/types.ts (1)
169-169: Fix comment typo.The comment references "JobBase" but the interface is named "BaseJob".
Apply this diff:
- transition?: number; // added for compatibility with JobBase + transition?: number; // added for compatibility with BaseJob
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/lib/bullmq.ts(1 hunks)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/16_track_jobs/migration.sql(1 hunks)src/lib/prisma/schema.prisma(1 hunks)src/lib/products/components/ProductDetails.svelte(7 hunks)src/lib/projects/sse.ts(2 hunks)src/lib/server/bullmq/BullWorker.ts(2 hunks)src/lib/server/bullmq/queues.ts(2 hunks)src/lib/server/bullmq/types.ts(10 hunks)src/lib/server/job-executors/build.ts(10 hunks)src/lib/server/job-executors/polling.ts(2 hunks)src/lib/server/job-executors/product.ts(8 hunks)src/lib/server/job-executors/publish.ts(11 hunks)src/lib/server/job-executors/userTasks.ts(1 hunks)src/lib/server/workflow/index.ts(6 hunks)src/lib/server/workflow/state-machine.ts(10 hunks)src/routes/(authenticated)/admin/settings/build-engines/+page.svelte(2 hunks)src/routes/(authenticated)/projects/[id=idNumber]/ProductCard.svelte(1 hunks)src/routes/(authenticated)/workflow-instances/[product_id]/+page.server.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- src/lib/locales/es-419.json
- src/lib/locales/fr-FR.json
- src/lib/server/bullmq/BullWorker.ts
- src/lib/server/job-executors/polling.ts
- src/lib/server/job-executors/product.ts
- src/lib/prisma/migrations/16_track_jobs/migration.sql
- src/routes/(authenticated)/projects/[id=idNumber]/ProductCard.svelte
- src/lib/locales/en-US.json
- src/routes/(authenticated)/workflow-instances/[product_id]/+page.server.ts
- src/routes/(authenticated)/admin/settings/build-engines/+page.svelte
- src/lib/server/job-executors/publish.ts
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1115
File: portal/node-server/BullWorker.ts:74-83
Timestamp: 2025-05-12T13:16:39.942Z
Learning: In BullMQ, using a fixed `jobId` provides idempotency for jobs. Without a `jobId`, each application restart will add a new job with a unique ID to the queue, causing startup jobs to accumulate. However, completed jobs are not automatically reprocessed - they remain in the completed state.
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1115
File: portal/node-server/BullWorker.ts:74-83
Timestamp: 2025-05-12T13:16:39.942Z
Learning: In BullMQ, using a fixed `jobId` provides idempotency for jobs. Without a `jobId`, each application restart will add a new job with a unique ID to the queue, causing startup jobs to accumulate over time. Completed jobs are not automatically reprocessed regardless of whether a `jobId` is specified.
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1186
File: src/hooks.server.ts:34-41
Timestamp: 2025-08-04T18:34:58.818Z
Learning: In the AppBuilder Portal codebase, BullMQ workers are only initialized when not building (`if (!building)` in BullWorker constructor), and the graceful shutdown handler in hooks.server.ts also only runs when not building. Therefore, the worker.worker property is guaranteed to be defined in the shutdown context, making optional chaining defensive rather than necessary.
📚 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/userTasks.tssrc/lib/server/bullmq/queues.tssrc/lib/server/bullmq/types.ts
📚 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/server/job-executors/userTasks.tssrc/lib/server/workflow/state-machine.ts
📚 Learning: 2025-09-12T14:57:08.647Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1283
File: src/routes/(authenticated)/tasks/[product_id]/+page.server.ts:252-264
Timestamp: 2025-09-12T14:57:08.647Z
Learning: In src/routes/(authenticated)/tasks/[product_id]/+page.server.ts, FyreByrd prefers to use transition.target[0] prediction mechanism for determining post-send state rather than calling flow.state() after flow.send() due to race condition concerns. The prediction approach is sufficient for redirect decisions between staying on task page vs going to projects page.
Applied to files:
src/lib/server/job-executors/userTasks.tssrc/lib/server/workflow/state-machine.tssrc/lib/server/workflow/index.tssrc/lib/products/components/ProductDetails.sveltesrc/lib/server/bullmq/queues.tssrc/lib/server/job-executors/build.ts
📚 Learning: 2025-09-17T21:07:18.139Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1275
File: src/lib/projects/index.ts:183-201
Timestamp: 2025-09-17T21:07:18.139Z
Learning: In the AppBuilder portal project, org admins must be in the project group to claim/take ownership of projects. This is enforced both in client-side logic (canClaimProject function) and server-side validation.
Applied to files:
src/lib/projects/sse.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-17T20:58:49.908Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1283
File: src/routes/(authenticated)/tasks/[product_id]/+page.svelte:48-71
Timestamp: 2025-09-17T20:58:49.908Z
Learning: In the codebase, ProductId fields are UUID strings, not numbers. In UserTasks model: `ProductId String db.Uuid` and in Products model: `Id String id db.Uuid`. Route parameters like page.params.product_id are also strings, so strict equality comparisons between ProductId and route parameters work correctly without type conversion.
Applied to files:
src/lib/prisma/schema.prisma
📚 Learning: 2025-05-12T13:19:42.118Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1115
File: portal/common/bullmq/types.ts:205-220
Timestamp: 2025-05-12T13:19:42.118Z
Learning: In the AppBuilder portal, `System_CheckEngineStatuses` and `System_RefreshLangTags` job types are designed to run both periodically (as recurring jobs) and immediately upon startup, which is why they're included in both the `RecurringJob` and `StartupJob` type definitions.
Applied to files:
src/lib/bullmq.tssrc/lib/server/bullmq/types.ts
📚 Learning: 2025-08-04T18:34:58.818Z
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1186
File: src/hooks.server.ts:34-41
Timestamp: 2025-08-04T18:34:58.818Z
Learning: In the AppBuilder Portal codebase, BullMQ workers are only initialized when not building (`if (!building)` in BullWorker constructor), and the graceful shutdown handler in hooks.server.ts also only runs when not building. Therefore, the worker.worker property is guaranteed to be defined in the shutdown context, making optional chaining defensive rather than necessary.
Applied to files:
src/lib/server/bullmq/queues.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/bullmq/queues.tssrc/lib/server/job-executors/build.tssrc/lib/server/bullmq/types.ts
🧬 Code graph analysis (7)
src/lib/server/workflow/state-machine.ts (2)
src/lib/server/bullmq/queues.ts (1)
getQueues(198-203)src/lib/server/workflow/index.ts (1)
Workflow(36-637)
src/lib/projects/sse.ts (1)
src/lib/utils/roles.ts (1)
isSuperAdmin(33-35)
src/lib/server/workflow/index.ts (2)
src/lib/server/database/index.ts (1)
DatabaseWrites(68-71)src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)
src/lib/bullmq.ts (1)
src/lib/server/bullmq/types.ts (3)
QueueName(6-6)JobType(6-6)JobSchedulerId(6-6)
src/lib/server/bullmq/queues.ts (3)
src/lib/server/bullmq/types.ts (12)
Job(248-248)BaseJob(22-26)BuildJob(250-250)QueueName(6-6)RecurringJob(251-253)StartupJob(254-257)ProductJob(274-278)ProjectJob(279-279)PublishJob(258-258)PollJob(259-259)UserTasksJob(260-260)EmailJob(261-270)src/lib/server/workflow/index.ts (1)
Workflow(36-637)src/lib/server/bullmq/BullWorker.ts (9)
Builds(54-66)SystemRecurring(68-106)SystemStartup(108-151)Products(153-169)Projects(171-183)Publishing(185-197)Polling(199-213)UserTasks(215-225)Emails(227-267)
src/lib/server/job-executors/build.ts (2)
src/lib/server/job-executors/publish.ts (1)
product(10-161)src/lib/server/build-engine-api/types.ts (1)
BuildResponse(157-165)
src/lib/server/bullmq/types.ts (1)
src/lib/server/build-engine-api/types.ts (1)
Channels(171-171)
⏰ 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). (4)
- GitHub Check: checks / build-and-test
- GitHub Check: checks / typecheck-lint
- GitHub Check: check / typecheck-lint
- GitHub Check: check / build-and-test
🔇 Additional comments (28)
src/lib/server/job-executors/build.ts (2)
71-72: Consistent transition propagation through build workflow.The transition parameter is correctly threaded through all notification calls and the polling job data, enabling job tracking as intended.
Also applies to: 79-80, 115-116
304-324: Signature updates are consistent with the pattern.The
notifyConnectionFailedfunction correctly accepts the optionaltransitionparameter and includes it in the email job payload. The same pattern is correctly applied tonotifyUnableToCreate,notifyCompleted, andnotifyFailed.src/lib/server/job-executors/userTasks.ts (1)
42-55: Correct safeguard to preserve transitions with job records.Adding
QueueRecords: { none: {} }ensures that ProductTransitions with associated job records are not prematurely deleted during the "Clear PreExecuteEntries" phase. This is essential for maintaining job tracking integrity.src/lib/projects/sse.ts (1)
164-174: Appropriate access control for QueueRecords.The conditional inclusion of
QueueRecordsbased onisSuperAdmin(userSession.roles)correctly restricts job tracking details to super administrators, preventing exposure of internal system information to regular users.src/lib/server/bullmq/queues.ts (1)
161-182: Queue event registration looks good.The
createJobRecordhandler is correctly attached to thewaitingevent for the appropriate queues (Builds, Products, Publishing, Polling, Emails). This aligns with the PR objective to track jobs for product-related actions while excluding UserTasks and System queues as intended.src/lib/products/components/ProductDetails.svelte (2)
78-95: Well-structured snippet for queue records display.The collapsible
<details>element with record count in the summary provides a clean UX for displaying job links without cluttering the transition history. TheencodeURIComponentusage correctly handles special characters in job IDs.
135-167: Desktop table integration is correctly implemented.The conditional rendering of QueueRecords rows with proper
no-borderclass handling and colspan ensures seamless visual integration with the existing transition display.src/lib/server/workflow/index.ts (6)
59-59: LGTM!Adding
Id: trueto the Project selection is necessary for line 100, where the project ID is passed tocreateMany.
338-342: LGTM!The additional filter
QueueRecords: { none: {} }correctly prevents deletion of transitions that have associated job records, preserving the job tracking history.
481-494: LGTM!The query correctly identifies pending transitions that already have queue records, preventing duplicate entries in the subsequent filter at line 516.
516-516: LGTM!The filter correctly excludes transitions that already have queue records from being recreated.
519-547: Consider race condition implications.The method fetches the current transition ID to attach to jobs. There's a potential race window between:
- Calling
currentProductTransitionto get a transition ID- Adding the job to the queue
- The transition completing (DateTransition being set)
This could result in jobs being attached to transitions that complete before the job is added. However, given the PR's goal of enabling job search and debugging, this may be acceptable.
Based on the PR comments, chrisvire asked about race conditions. Please verify this approach meets the requirements.
593-594: LGTM!Adding
DestinationState: destinationStateto the update ensures the destination is recorded when the transition completes, consistent with the create path.src/lib/server/workflow/state-machine.ts (7)
302-311: LGTM!The async entry handler correctly fetches the transition ID for the Product_Creation state and attaches it to the job payload.
504-527: LGTM!The async entry handler correctly fetches the transition ID for the Product_Build state.
664-693: LGTM!Both Continue actions in the Create_App_Store_Entry state correctly fetch and attach the transition ID to the GetVersionCode job.
784-793: LGTM!The Email_Reviewers action correctly tracks the transition for email jobs, consistent with the PR's goal of tracking emails sent by workflow actions.
801-821: LGTM!The async entry handler correctly fetches the transition ID for the Product_Publish state.
1055-1066: LGTM!The Migrate action correctly attaches the transition ID for the GetVersionCode job, mapping deprecated "Set Google Play Uploaded" state to Create_App_Store_Entry.
25-25: Circular import is safe here but note the dependency.This import creates a circular dependency:
state-machine.tsimportsWorkflowfromindex.ts, whileindex.tsimportsWorkflowStateMachinefromstate-machine.ts. However, sinceWorkflow.currentProductTransition()is a static method called only during runtime (not at module initialization), TypeScript handles this correctly. No initialization order issues exist.src/lib/prisma/schema.prisma (2)
276-276: LGTM!The one-to-many relationship between ProductTransitions and QueueRecords is correctly defined.
281-290: Change JobId field type to accommodate BullMQ's auto-generated numeric IDs, or ensure all jobs provide custom string jobIds.BullMQ auto-generates numeric (integer) job IDs by default. The schema currently defines
JobIdasString, but most jobs in the codebase do not provide customjobIdparameters, meaningjob.idwill be numeric for most jobs. This creates an implicit type coercion that is fragile and type-unsafe.Either:
- Change
JobId StringtoJobId Int(orBigIntif needed for large counters), or- Ensure all
.add()calls provide custom stringjobIdparameters⛔ Skipped due to learnings
Learnt from: FyreByrd Repo: sillsdev/appbuilder-portal PR: 1283 File: src/routes/(authenticated)/tasks/[product_id]/+page.svelte:48-71 Timestamp: 2025-09-17T20:58:49.908Z Learning: In the codebase, ProductId fields are UUID strings, not numbers. In UserTasks model: `ProductId String db.Uuid` and in Products model: `Id String id db.Uuid`. Route parameters like page.params.product_id are also strings, so strict equality comparisons between ProductId and route parameters work correctly without type conversion.Learnt from: FyreByrd Repo: sillsdev/appbuilder-portal PR: 1115 File: portal/node-server/BullWorker.ts:74-83 Timestamp: 2025-05-12T13:16:39.942Z Learning: In BullMQ, using a fixed `jobId` provides idempotency for jobs. Without a `jobId`, each application restart will add a new job with a unique ID to the queue, causing startup jobs to accumulate over time. Completed jobs are not automatically reprocessed regardless of whether a `jobId` is specified.Learnt from: FyreByrd Repo: sillsdev/appbuilder-portal PR: 1115 File: portal/node-server/BullWorker.ts:74-83 Timestamp: 2025-05-12T13:16:39.942Z Learning: In BullMQ, using a fixed `jobId` provides idempotency for jobs. Without a `jobId`, each application restart will add a new job with a unique ID to the queue, causing startup jobs to accumulate. However, completed jobs are not automatically reprocessed - they remain in the completed state.src/lib/bullmq.ts (3)
1-12: LGTM!The QueueName enum provides type-safe queue identifiers with human-readable string values.
14-52: LGTM!The JobType enum comprehensively covers all job types with human-readable string values, well-organized by category.
54-59: LGTM!The JobSchedulerId enum provides type-safe identifiers for recurring jobs.
src/lib/server/bullmq/types.ts (3)
5-6: LGTM!Importing enums from
$lib/bullmqand re-exporting them maintains backward compatibility while making them available to both client and server code.
22-26: LGTM!The BaseJob interface provides a clean common base with the transition tracking field that enables the job search functionality described in the PR objectives.
28-246: LGTM!All job interfaces consistently extend BaseJob, maintaining type discrimination while inheriting the transition tracking field.
Co-Authored-By: Aidan Jones <aidan_jones@sil.org>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/server/bullmq/queues.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1115
File: portal/node-server/BullWorker.ts:74-83
Timestamp: 2025-05-12T13:16:39.942Z
Learning: In BullMQ, using a fixed `jobId` provides idempotency for jobs. Without a `jobId`, each application restart will add a new job with a unique ID to the queue, causing startup jobs to accumulate. However, completed jobs are not automatically reprocessed - they remain in the completed state.
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1186
File: src/hooks.server.ts:34-41
Timestamp: 2025-08-04T18:34:58.818Z
Learning: In the AppBuilder Portal codebase, BullMQ workers are only initialized when not building (`if (!building)` in BullWorker constructor), and the graceful shutdown handler in hooks.server.ts also only runs when not building. Therefore, the worker.worker property is guaranteed to be defined in the shutdown context, making optional chaining defensive rather than necessary.
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1115
File: portal/node-server/BullWorker.ts:74-83
Timestamp: 2025-05-12T13:16:39.942Z
Learning: In BullMQ, using a fixed `jobId` provides idempotency for jobs. Without a `jobId`, each application restart will add a new job with a unique ID to the queue, causing startup jobs to accumulate over time. Completed jobs are not automatically reprocessed regardless of whether a `jobId` is specified.
📚 Learning: 2025-12-04T23:01:23.804Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1384
File: src/lib/server/workflow/index.ts:92-103
Timestamp: 2025-12-04T23:01:23.804Z
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/bullmq/queues.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/bullmq/queues.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/bullmq/queues.ts
📚 Learning: 2025-08-04T18:34:58.818Z
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1186
File: src/hooks.server.ts:34-41
Timestamp: 2025-08-04T18:34:58.818Z
Learning: In the AppBuilder Portal codebase, BullMQ workers are only initialized when not building (`if (!building)` in BullWorker constructor), and the graceful shutdown handler in hooks.server.ts also only runs when not building. Therefore, the worker.worker property is guaranteed to be defined in the shutdown context, making optional chaining defensive rather than necessary.
Applied to files:
src/lib/server/bullmq/queues.ts
📚 Learning: 2025-09-12T14:57:08.647Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1283
File: src/routes/(authenticated)/tasks/[product_id]/+page.server.ts:252-264
Timestamp: 2025-09-12T14:57:08.647Z
Learning: In src/routes/(authenticated)/tasks/[product_id]/+page.server.ts, FyreByrd prefers to use transition.target[0] prediction mechanism for determining post-send state rather than calling flow.state() after flow.send() due to race condition concerns. The prediction approach is sufficient for redirect decisions between staying on task page vs going to projects page.
Applied to files:
src/lib/server/bullmq/queues.ts
⏰ 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: check / build-and-test
- GitHub Check: checks / build-and-test
🔇 Additional comments (1)
src/lib/server/bullmq/queues.ts (1)
165-185: LGTM: Event registrations are selective and appropriate.The
waitingevent listeners are correctly registered only on the queues mentioned in the PR objectives (Builds, Products, Publishing, Polling, Emails), while UserTasks, Projects, SSE, and System queues are intentionally excluded as documented.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Closes #1375
Tracked:
Not Tracked:
Summary by CodeRabbit
New Features
Localization
Chores
UI
✏️ Tip: You can customize this high-level summary in your review settings.