Implement Software Update Rebuild Code#1377
Implement Software Update Rebuild Code#1377ThePitter wants to merge 42 commits intosillsdev:developfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a complete software update administration feature with localization strings, product rebuild workflow, database persistence layer, real-time SSE updates, and admin UI for initiating and monitoring software rebuilds across multiple organizations. Changes
Sequence DiagramsequenceDiagram
participant Admin as Admin UI
participant Server as Page Server
participant DB as Database
participant Queue as Job Queue
participant Build as Build Executor
participant Updates as SoftwareUpdates
Admin->>Server: Submit rebuild form with comment
Server->>DB: Get eligible products (check RebuildOnSoftwareUpdate,<br/>active workflows, versions)
DB-->>Server: Return eligible products
Server->>DB: recordRebuilds() - create SoftwareUpdates<br/>records grouped by org/BuildEngine/AppType/version
DB-->>Server: Return update IDs
Server->>Queue: Dispatch rebuild jobs via doProductAction<br/>for each eligible product
Queue-->>Server: Jobs queued
Server-->>Admin: Return initiator, counts, timestamp
note over Admin: Polling begins every 10s
Admin->>Server: Query rebuild status via SSE
Queue->>Build: Execute rebuild job
Build->>Build: Rebuild product
Build->>Updates: completeForProduct(productId)
Updates->>DB: Check if all linked products<br/>have successful builds with matching version
DB-->>Updates: Verification result
alt All products completed
Updates->>DB: Mark SoftwareUpdate as DateCompleted
Updates->>Server: Emit SSE notification
Server-->>Admin: Push completion status update
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
FyreByrd
force-pushed
the
develop
branch
2 times, most recently
from
95e992b to
264d56f
Compare
ThePitter
force-pushed
the
feature/software-update-rebuild
branch
from
7e5635e to
09bb94b
Compare
|
Everything is setup but am uncertain where the comment is supposed to be stored? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/routes/(authenticated)/admin/settings/software-update/+page.server.ts (1)
37-58: Consider filtering products to only rebuild those eligible for rebuild.The query includes
WorkflowInstanceandProductBuilds, but doesn't filter products that:
- Already have an active workflow instance (shouldn't rebuild if already rebuilding)
- Haven't completed initial publication yet
Consider adding a filter to exclude products already being rebuilt:
include: { Products: { + where: { + WorkflowInstance: null + }, include: {Or add logic after the query to skip products with active workflows before calling
doProductAction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
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/routes/(authenticated)/admin/settings/+layout.svelte(1 hunks)src/routes/(authenticated)/admin/settings/software-update/+page.server.ts(1 hunks)src/routes/(authenticated)/admin/settings/software-update/+page.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/routes/(authenticated)/admin/settings/software-update/+page.server.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/routes/(authenticated)/admin/settings/software-update/+page.server.ts
📚 Learning: 2025-09-22T14:59:53.366Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts:214-221
Timestamp: 2025-09-22T14:59:53.366Z
Learning: In the AppBuilder portal, bulk project actions in the route `/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts` are organization-scoped by design. The route parameter `[orgId=idNumber]` ensures all operations happen within a single organization context, so using the route-level orgId for all projects in bulk actions like `doProjectAction` is correct and safe.
Applied to files:
src/routes/(authenticated)/admin/settings/software-update/+page.server.ts
🧬 Code graph analysis (1)
src/routes/(authenticated)/admin/settings/software-update/+page.server.ts (2)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)src/lib/products/server.ts (1)
doProductAction(7-88)
🪛 GitHub Actions: Test and Lint
src/routes/(authenticated)/admin/settings/+layout.svelte
[error] 21-21: Prettier formatting error reported by ESLint during 'npm run lint': Insert a space character (prettier/prettier) at column 68.
🪛 GitHub Check: checks / typecheck-lint
src/routes/(authenticated)/admin/settings/+layout.svelte
[failure] 21-21:
Insert ·
src/routes/(authenticated)/admin/settings/software-update/+page.svelte
[failure] 11-11:
Replace ········ with ····
[failure] 10-10:
Delete ··
[failure] 8-8:
Delete ··
[warning] 7-7:
$lib/paraglide/messages import should occur after import of $lib/components/settings/LabeledFormInput.svelte
[failure] 7-7:
Delete ··
[warning] 6-6:
'getLocale' is defined but never used
[warning] 6-6:
$lib/paraglide/runtime import should occur after import of $lib/components/settings/LabeledFormInput.svelte
[failure] 6-6:
Delete ··
[failure] 5-5:
Delete ··
[warning] 4-4:
$lib/utils import should occur after import of $lib/components/settings/LabeledFormInput.svelte
[failure] 4-4:
Replace ···· with ··
[failure] 3-3:
Delete ··
[warning] 2-2:
./$types type import should occur after import of sveltekit-superforms
[failure] 2-2:
Delete ··
🔇 Additional comments (3)
src/lib/locales/fr-FR.json (1)
551-557: LGTM! French localization keys added correctly.The new localization keys for the software update feature are properly structured and the translations appear accurate.
src/lib/locales/es-419.json (1)
576-582: LGTM! Spanish localization keys added correctly.The Spanish translations for the software update feature are properly structured and consistent with other locale files.
src/lib/locales/en-US.json (1)
576-582: LGTM! English localization keys properly defined.The base English localization keys for the software update feature are clear and well-structured.
FyreByrd
left a comment
FyreByrd
left a comment
There was a problem hiding this comment.
As best as I can tell, all of the CodeRabbit stuff this time around is accurate; please address those.
Moving forward, please make sure you are running npm run check and npm run format and fix all reported errors before pushing.
There are some potential changes for the software updates page that @chrisvire and I had discussed briefly earlier last week; we will need to discuss those more in depth and update the issues accordingly. One of us will let you know when that happens.
|
See Comment in #1249 For things that need to be changed before making this PR complete again. |
ThePitter
force-pushed
the
feature/software-update-rebuild
branch
from
97f04df to
613c8b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts (3)
15-36: Consider extracting duplicate organization resolution logic.The organization resolution logic (lines 16-36) is nearly identical to the logic in the
startaction (lines 106-126). Extracting this into a shared helper function would reduce duplication and maintenance burden.async function resolveTargetOrgs( params: { orgId?: string }, locals: App.Locals ): Promise<number[]> { const organizationId = Number(params.orgId); if (!isNaN(organizationId)) { locals.security.requireAdminOfOrg(organizationId); return [organizationId]; } const orgs = await DatabaseReads.userRoles.findMany({ where: { UserId: locals.security.userId, RoleId: { in: [1, 2] } }, select: { OrganizationId: true } }); const searchOrgs = new Set<number>(); for (const org of orgs) { locals.security.requireAdminOfOrg(org.OrganizationId); searchOrgs.add(org.OrganizationId); } return Array.from(searchOrgs); }Also applies to: 106-126
47-50: PrefermapoverforEachwithpush.- const organizationsReadable: string[] = []; - names.forEach((name) => { - organizationsReadable.push(name.Name ? name.Name : 'Unknown Organization'); - }); + const organizationsReadable = names.map((n) => n.Name ?? 'Unknown Organization');
156-162: Consider usingPromise.allSettledfor resilience.If one
doProductActionfails,Promise.allwill reject immediately and no results are returned. UsingPromise.allSettledwould allow partial success and provide feedback about which products failed.- await Promise.all( - projects.flatMap((project) => - project.Products.map((p) => - doProductAction(p.Id, ProductActionType.Rebuild, form.data.comment) - ) - ) - ); + const results = await Promise.allSettled( + projects.flatMap((project) => + project.Products.map((p) => + doProductAction(p.Id, ProductActionType.Rebuild, form.data.comment) + ) + ) + ); + + const failed = results.filter((r) => r.status === 'rejected'); + if (failed.length > 0) { + console.error(`${failed.length} product rebuild(s) failed`); + // Optionally return partial failure info to the client + } return { ok: true, form };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
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/products/server.ts(3 hunks)src/lib/server/workflow/index.ts(2 hunks)src/routes/(authenticated)/+layout.svelte(1 hunks)src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts(1 hunks)src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/locales/es-419.json
🧰 Additional context used
🧠 Learnings (3)
📚 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.tssrc/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-22T14:59:53.366Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts:214-221
Timestamp: 2025-09-22T14:59:53.366Z
Learning: In the AppBuilder portal, bulk project actions in the route `/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts` are organization-scoped by design. The route parameter `[orgId=idNumber]` ensures all operations happen within a single organization context, so using the route-level orgId for all projects in bulk actions like `doProjectAction` is correct and safe.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.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/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
🧬 Code graph analysis (1)
src/lib/server/workflow/index.ts (1)
src/lib/workflowTypes.ts (1)
WorkflowConfig(180-184)
🪛 GitHub Check: checks / typecheck-lint
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
[warning] 53-53:
'projects' is assigned a value but never used
🔇 Additional comments (5)
src/lib/products/server.ts (1)
7-11: LGTM!The optional
commentparameter is correctly added todoProductActionand appropriately propagated to bothWorkflow.createfor Rebuild/Republish actions and persisted inProductTransitionsfor Cancel actions.Also applies to: 53-61, 87-87
src/lib/server/workflow/index.ts (1)
51-55: LGTM!The
Workflow.createmethod signature correctly accepts an optionalcommentparameter and persists it in theStartWorkflowtransition record. This maintains backward compatibility with existing callers.Also applies to: 89-97
src/routes/(authenticated)/+layout.svelte (1)
220-229: LGTM!The new navigation item follows the established pattern for admin links, correctly uses
activeOrgUrlfor organization-scoped navigation, and is appropriately gated by the admin role check.src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte (1)
18-25: Verify post-submission navigation preserves org context.After a successful form submission, the navigation goes to
/software-updatewithout preserving the currentorgIdfrom the URL. If the user initiated the rebuild for a specific organization, they'll be redirected to the "all organizations" view.Is this intentional, or should the navigation preserve the current organization context? If preservation is desired:
+ import { page } from '$app/state'; + const { form, enhance } = superForm(data.form, { onUpdated({ form }) { if (form.valid) { - goto(localizeHref(base)); + goto(localizeHref(page.url.pathname)); toast('success', m.admin_software_update_toast_success()); } } });src/lib/locales/en-US.json (1)
559-565: LGTM!The new localization keys follow the established naming conventions and provide clear, user-friendly messages for the software update feature.
FyreByrd
left a comment
FyreByrd
left a comment
There was a problem hiding this comment.
This is starting to look better.
A few things for now:
- There is some duplicated/incorrect/unnecessary logic when getting a list of organizations on the server side. Please take a look at
src/routes/users/org/[[id=idNumber]]/+page.server.tsfor inspiration. - We recently added a
SystemVersionstable with information on AppBuilder versions. Please use this to further filter projects. - Both the load function and the action should have the same filtration logic for orgs/projects/products. Please break this out into one or more functions (in the same file) for consistency/reusability.
I will be working on creating a database table to record software updates for you to write to/read from for this PR. I will ping one of you when that's ready.
|
Just opened #1400 to add a table you can write to/display from for in progress software updates. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts (1)
24-41: Super-admin branch indetermineTargetOrgsdoesn’t truly return “all organizations”.The docstring says super admins get all organizations, but the implementation reads from
userRoles:const orgs = locals.security.isSuperAdmin ? await DatabaseReads.userRoles.findMany({ select: { OrganizationId: true } }) : await DatabaseReads.userRoles.findMany({ where: { UserId: locals.security.userId, RoleId: { in: [RoleId.SuperAdmin, RoleId.OrgAdmin] } }, select: { OrganizationId: true } });This only returns orgs that have at least one
userRolesrow, and for non-super-admins, only those where the current user holds specific roles. That can diverge from the stated behavior and prior feedback that super admins should be able to target all orgs, independent of their memberships.A clearer and more accurate approach would be something like:
async function determineTargetOrgs(locals: App.Locals): Promise<number[]> { if (locals.security.isSuperAdmin) { const orgs = await DatabaseReads.organizations.findMany({ select: { Id: true } }); return orgs.map((o) => o.Id); } const roles = await DatabaseReads.userRoles.findMany({ where: { UserId: locals.security.userId, RoleId: { in: [RoleId.SuperAdmin, RoleId.OrgAdmin] } }, select: { OrganizationId: true } }); return Array.from(new Set(roles.map((r) => r.OrganizationId))); }This matches the docstring and ensures super admins truly cover all organizations, while non-super-admins are constrained to orgs where they’re actually admins. Based on learnings, this aligns better with FyreByrd’s expectation that super admins see all orgs, not just those where they have explicit org-role rows.
🧹 Nitpick comments (3)
src/lib/locales/es-419.json (1)
559-566: Software-update strings look good; consider localizingdownloads_title.The new
admin_*software_update*keys read well and align with the described behavior. One minor nit:downloads_titleis still"Downloads"in the es-419 locale—if this is user-facing, you may want to translate it (e.g.,"Descargas") for consistency with the rest of the file.src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts (2)
11-14: Whitespace-only comments still pass validation; consider trimming input.
v.minLength(1, 'Comment is required')only enforces length, so a comment like" "will be accepted even though it’s effectively empty. If you truly require a meaningful comment, consider trimming before checking length, e.g.:-const formSchema = v.object({ - comment: v.pipe(v.string(), v.minLength(1, 'Comment is required')) - // Since we are only getting a comment, I do not believe we need a properties: propertiesSchema here. -}); +const formSchema = v.object({ + comment: v.pipe( + v.string(), + v.trim(), + v.minLength(1, 'Comment is required') + ) + // Since we are only getting a comment, I do not believe we need a properties: propertiesSchema here. +});Please double-check that
v.trim()(or the equivalent in your valibot version) is available and behaves as expected, or apply an equivalent trim via a transform/refinement.
123-165: SystemVersions usage is simplified to “latest row”; may need per-appType/buildEngine logic.Right now, the action treats the most recent
systemVersionsrow as a single global AppBuilder version:const systemVersion = await DatabaseReads.systemVersions.findFirst({ orderBy: { DateCreated: 'desc' } }); const currentAppBuilderVersion = systemVersion?.Version ?? null;and then excludes products already at that version via:
NOT: [ { VersionBuilt: currentAppBuilderVersion }, { ProductBuilds: { some: { Version: currentAppBuilderVersion } } } ]From earlier guidance,
SystemVersions.*is intended to be keyed byProject.ApplicationTypeIdand the effectiveBuildEngineUrl, and “what should be” is looked up per(applicationType, buildEngine)combination. If there are multiple rows for different combinations, using just the single latest row may misclassify some products (either rebuilding ones that are already up to date for their specific combo or vice versa).Consider evolving this to:
- Look up the appropriate “target” SystemVersions row per project (keyed by
ApplicationTypeIdand effectiveBuildEngineUrl), and- Filter products per project against that target version.
This will be more complex (likely requiring either a pre-fetched map of SystemVersions or additional queries), but it will more closely match the intended “out-of-date relative to its own AppBuilder/engine combo” semantics. Based on learnings, this would align with the earlier
what isvswhat should bedesign for SystemVersions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
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/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts(1 hunks)src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte
- src/lib/locales/fr-FR.json
- src/lib/locales/en-US.json
🧰 Additional context used
🧠 Learnings (9)
📚 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/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-22T14:59:53.366Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts:214-221
Timestamp: 2025-09-22T14:59:53.366Z
Learning: In the AppBuilder portal, bulk project actions in the route `/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts` are organization-scoped by design. The route parameter `[orgId=idNumber]` ensures all operations happen within a single organization context, so using the route-level orgId for all projects in bulk actions like `doProjectAction` is correct and safe.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.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/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-12T14:31:23.906Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1283
File: src/lib/server/workflow/index.ts:52-74
Timestamp: 2025-09-12T14:31:23.906Z
Learning: In the appbuilder-portal codebase, optional chaining in database queries (like `check?.Project._count.Authors`) is primarily used to satisfy TypeScript's type checker rather than handling expected null cases. The `check` being null should realistically never happen in the normal application flow.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-22T15:00:43.129Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(authenticated)/users/[id=idNumber]/settings/+layout.server.ts:7-10
Timestamp: 2025-09-22T15:00:43.129Z
Learning: In user settings layouts, use the `adminOrgs` function from `$lib/users/server` to determine admin access rather than throwing 403 errors upfront. The `adminOrgs` function creates a filter that returns organizations where the subject user is a member AND the current user is an admin of that organization.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-25T19:44:14.845Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(authenticated)/projects/import/[id=idNumber]/+page.server.ts:24-26
Timestamp: 2025-09-25T19:44:14.845Z
Learning: The `requireHasRole(organizationId, roleId, orOrgAdmin)` method in the Security class allows organization admins to pass role checks when the third parameter `orOrgAdmin` is set to `true`, even if they don't have the specific role being checked for.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.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/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-10-01T15:58:37.578Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1317
File: src/routes/(authenticated)/users/+page.svelte:51-55
Timestamp: 2025-10-01T15:58:37.578Z
Learning: In the appbuilder-portal codebase, the $orgActive store (from $lib/stores) is designed to always contain a valid organization ID and should never be null. Code that syncs values to $orgActive should include guard clauses to prevent null assignments.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-10T13:46:43.497Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1275
File: src/auth.ts:240-245
Timestamp: 2025-09-10T13:46:43.497Z
Learning: In src/auth.ts, the products branch intentionally uses params.id as a string for database lookups, not as a number. This is the correct behavior and should not be changed to numeric parsing.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
🧬 Code graph analysis (1)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts (2)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)src/lib/products/server.ts (1)
doProductAction(7-97)
⏰ 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 (1)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts (1)
129-175: Rebuild selection + triggering logic looks consistent with the intended behavior.The
projectsquery and the subsequentPromise.alloverprojects.flatMap(project => project.Products)correctly:
- Limit to non-archived projects with
RebuildOnSoftwareUpdate: truein the target orgs.- Exclude products with an active
WorkflowInstance.- Exclude products already at the chosen “current” AppBuilder version (when defined).
- Pass the validated
commentthrough todoProductActionwithProductActionType.Rebuild.This wiring matches the described software-update semantics and cleanly reuses the existing product action pipeline.
FyreByrd
left a comment
FyreByrd
left a comment
There was a problem hiding this comment.
See comments below for my feedback on your most recent changes.
I am looking forward to seeing what y'all come up with for UI. If you need ideas I can send a DM of a rough sketch I had in mind.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts (3)
28-34: Remove redundantRoleId.SuperAdminfrom query filter.The query includes
RoleId.SuperAdminin the filter, but any user who is a SuperAdmin has already returned on lines 21-26. The query will never match SuperAdmin roles at this point.Apply this diff:
const roles = await DatabaseReads.userRoles.findMany({ where: { UserId: locals.security.userId, - RoleId: { in: [RoleId.SuperAdmin, RoleId.OrgAdmin] } + RoleId: RoleId.OrgAdmin }, select: { OrganizationId: true } });
59-59: Complete the JSDoc comment.The comment is incomplete: "Array of Pro"
Apply this diff:
- * @returns Array of Pro + * @returns Array of products that need to be rebuilt
73-77: Fix comment or implementation mismatch.Line 73 comment states "We need this to identify the latest build, assuming WorkflowBuildId is monotonically increasing", but line 77 orders by
Id: 'desc'instead ofWorkflowBuildId. Either update the comment to reflect ordering by Id, or order by WorkflowBuildId if that's the intended behavior.If ordering by WorkflowBuildId is correct:
ProductBuilds: { - orderBy: { Id: 'desc' }, // Order by ID descending to get the 'latest' build + orderBy: { WorkflowBuildId: 'desc' }, // Order by WorkflowBuildId descending to get the 'latest' build take: 1, // Only take the most recentOtherwise, update the comment to match the implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts(1 hunks)src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte
🧰 Additional context used
🧠 Learnings (10)
📚 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/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-22T14:59:53.366Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts:214-221
Timestamp: 2025-09-22T14:59:53.366Z
Learning: In the AppBuilder portal, bulk project actions in the route `/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts` are organization-scoped by design. The route parameter `[orgId=idNumber]` ensures all operations happen within a single organization context, so using the route-level orgId for all projects in bulk actions like `doProjectAction` is correct and safe.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.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/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-12T14:31:23.906Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1283
File: src/lib/server/workflow/index.ts:52-74
Timestamp: 2025-09-12T14:31:23.906Z
Learning: In the appbuilder-portal codebase, optional chaining in database queries (like `check?.Project._count.Authors`) is primarily used to satisfy TypeScript's type checker rather than handling expected null cases. The `check` being null should realistically never happen in the normal application flow.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-22T15:00:43.129Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(authenticated)/users/[id=idNumber]/settings/+layout.server.ts:7-10
Timestamp: 2025-09-22T15:00:43.129Z
Learning: In user settings layouts, use the `adminOrgs` function from `$lib/users/server` to determine admin access rather than throwing 403 errors upfront. The `adminOrgs` function creates a filter that returns organizations where the subject user is a member AND the current user is an admin of that organization.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-25T19:44:14.845Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(authenticated)/projects/import/[id=idNumber]/+page.server.ts:24-26
Timestamp: 2025-09-25T19:44:14.845Z
Learning: The `requireHasRole(organizationId, roleId, orOrgAdmin)` method in the Security class allows organization admins to pass role checks when the third parameter `orOrgAdmin` is set to `true`, even if they don't have the specific role being checked for.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.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/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-10-01T15:58:37.578Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1317
File: src/routes/(authenticated)/users/+page.svelte:51-55
Timestamp: 2025-10-01T15:58:37.578Z
Learning: In the appbuilder-portal codebase, the $orgActive store (from $lib/stores) is designed to always contain a valid organization ID and should never be null. Code that syncs values to $orgActive should include guard clauses to prevent null assignments.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-10T13:46:43.497Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1275
File: src/auth.ts:240-245
Timestamp: 2025-09-10T13:46:43.497Z
Learning: In src/auth.ts, the products branch intentionally uses params.id as a string for database lookups, not as a number. This is the correct behavior and should not be changed to numeric parsing.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-10-02T01:36:08.124Z
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1318
File: src/lib/components/OrganizationDropdown.svelte:30-39
Timestamp: 2025-10-02T01:36:08.124Z
Learning: In the `src/lib/components/OrganizationDropdown.svelte` component, the `organizations` array will always contain at least one organization, so there's no need to handle the empty array case.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
🧬 Code graph analysis (1)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts (2)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)src/lib/products/server.ts (1)
doProductAction(7-97)
⏰ 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 (2)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts (2)
179-183: Consider error handling for partial rebuild failures.If any
doProductActioncall fails duringPromise.all, the entire operation fails immediately and previously triggered rebuilds cannot be rolled back. Consider whether this is the desired behavior or if you need:
- Individual error handling with
Promise.allSettledto track which rebuilds succeeded/failed- Returning partial success information to the user
- Transaction-like behavior or compensation logic
Should partial rebuild failures be handled gracefully? If so, consider using
Promise.allSettled:- await Promise.all( - productsToRebuild.map((p) => - doProductAction(p.id, ProductActionType.Rebuild, form.data.comment) - ) - ); - - return { form, ok: true }; + const results = await Promise.allSettled( + productsToRebuild.map((p) => + doProductAction(p.id, ProductActionType.Rebuild, form.data.comment) + ) + ); + + const failures = results.filter((r) => r.status === 'rejected'); + if (failures.length > 0) { + // Handle partial failures - return info about what succeeded/failed + return { + form, + ok: false, + partial: true, + failedCount: failures.length, + totalCount: results.length + }; + } + + return { form, ok: true };
107-107: Verify BuildEngineUrl fallback behavior.When
BuildEngineUrlis null, the code uses an empty string. Should this use theDEFAULT_BUILD_ENGINE_URLenvironment variable instead, consistent with other parts of the codebase?
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
deployment/ci/docker-compose.yml (1)
5-5: Quoting changes appear unrelated to feature scope.The PR objective is to implement Software Update Rebuild Code with admin UI and workflow features, but this file only contains cosmetic YAML string quoting normalization (double quotes → single quotes). While the formatting is valid and consistent, these changes seem tangential to the feature.
Consider consolidating formatting changes into a separate PR to keep the commit history focused and easier to review for the feature implementation.
Also applies to: 20-20, 22-34
src/lib/server/email-service/locales/fr-FR.json (1)
77-77: Fix spacing inconsistency in the subject line.Line 77 uses
"Scriptoria :"(with space after colon), while all other entries in the subject section (e.g., lines 48–76) consistently use"Scriptoria:"(no space). Align this with the existing pattern.- "autoPublishOnRebuildCompleted": "Scriptoria : Publication automatique réussie de {{productName}} dans {{projectName}}", + "autoPublishOnRebuildCompleted": "Scriptoria: Publication automatique réussie de {{productName}} dans {{projectName}}",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.codeclimate.yml(2 hunks).vscode/settings.json(1 hunks)README.md(7 hunks)deployment/ci/docker-compose.yml(2 hunks)deployment/development/docker-compose.yml(5 hunks)eslint-rules/tsconfig.json(1 hunks)renovate.json(2 hunks)src/lib/otel/development_config.yml(1 hunks)src/lib/otel/production_config.yml(2 hunks)src/lib/products/server.ts(3 hunks)src/lib/server/bullmq/pause.ts(1 hunks)src/lib/server/bullmq/types.ts(4 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(6 hunks)src/lib/server/workflow/state-machine.ts(6 hunks)src/lib/workflowTypes.ts(4 hunks)src/routes/(authenticated)/+layout.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- renovate.json
- src/lib/otel/development_config.yml
- .vscode/settings.json
- eslint-rules/tsconfig.json
- deployment/development/docker-compose.yml
- README.md
- .codeclimate.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/(authenticated)/+layout.svelte
🧰 Additional context used
🧠 Learnings (7)
📚 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-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/products/server.tssrc/lib/server/workflow/index.tssrc/lib/workflowTypes.tssrc/lib/server/job-executors/product.tssrc/lib/server/workflow/state-machine.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-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/pause.tssrc/lib/server/workflow/state-machine.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-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-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/workflow/state-machine.ts
🧬 Code graph analysis (3)
src/lib/products/server.ts (1)
src/lib/server/workflow/index.ts (1)
Workflow(36-592)
src/lib/server/workflow/index.ts (1)
src/lib/workflowTypes.ts (2)
WorkflowConfig(183-188)WorkflowInstanceContext(105-138)
src/lib/server/workflow/state-machine.ts (4)
src/lib/server/bullmq/types.ts (3)
Build(101-108)Product(77-82)Product(165-171)src/lib/server/bullmq/queues.ts (1)
getQueues(150-155)src/lib/workflowTypes.ts (1)
autoPublishOnRebuild(268-274)src/lib/server/workflow/dbProcedures.ts (1)
notifyAutoPublishOwner(55-82)
🪛 Biome (2.1.2)
src/lib/products/server.ts
[error] 11-11: expected , but instead found parentJobId
Remove parentJobId
(parse)
⏰ 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 (22)
src/lib/server/email-service/locales/fr-FR.json (1)
119-120: Localization additions look good.The French translations are grammatically sound and properly structured. The new
autoPublishOnRebuildCompletedentries in both the subject and notification sections are consistent with the template variables and formatting used elsewhere in the file.src/lib/server/email-service/locales/es-419.json (2)
77-77: Missing localization entry in body section.Per the AI summary,
autoPublishOnRebuildCompletedshould be added to three sections (body, subject, and notification), but this code shows only subject (line 77) and notification (line 120) additions. The body section (lines 3–46) appears to lack the corresponding entry.Verify that the body section includes
"autoPublishOnRebuildCompleted"with appropriate Spanish text describing the automatic publication completion.Also applies to: 120-120
77-77: Spanish translations are grammatically sound and follow localization patterns.Both translations correctly use template variables and follow the established naming and text conventions:
- Line 77 (subject): Includes "Scriptoria:" prefix, consistent with other subject entries
- Line 120 (notification): No prefix, consistent with other notification entries
- Template variables {{productName}} and {{projectName}} are correctly positioned
Also applies to: 120-120
src/lib/server/job-executors/product.ts (1)
247-249: LGTM!The addition of
isAutomatic: falsecorrectly satisfies the updatedWorkflowConfigtype requirement. This is appropriate for locally-created products which are not part of automated rebuild flows.src/lib/products/server.ts (2)
55-66: LGTM!The
Workflow.createcall correctly passes the newparentJobIdandisAutomaticfields in the config object, and thecommentas a separate argument. This aligns with the updated signature insrc/lib/server/workflow/index.ts.
92-92: Good addition for audit trail.Adding the
Commentfield to the Cancel transition record improves traceability by capturing the reason for cancellation.src/lib/server/workflow/dbProcedures.ts (1)
55-82: LGTM!The
notifyAutoPublishOwnerfunction is well-implemented with proper null guards and follows existing notification patterns in the codebase. The early return on missingOwnerIdprevents unnecessary queue operations.src/lib/server/job-executors/system.ts (1)
954-962: LGTM!Adding
isAutomatic: falseto the migrated workflow context is correct. Legacy migrated instances should default to non-automatic since they were not created through the new automated rebuild flow.src/lib/server/workflow/index.ts (4)
51-55: LGTM!The updated
createmethod signature properly acceptsparentJobIdvia the config extension andcommentas a separate parameter. This design cleanly separates workflow configuration from transition metadata.
74-81: LGTM!The workflow input correctly propagates
parentJobIdandautoPublishOnRebuildfrom the project settings. The double-negation!!check?.Project.AutoPublishOnRebuildsafely converts to boolean.
193-194: Good backward compatibility handling.The nullish coalescing assignment
context.isAutomatic ??= falseensures that workflows restored from snapshots created before this change will have a valid boolean value, preventing runtime errors in downstream logic.
373-382: LGTM!Correctly excludes runtime-derived fields (
autoPublishOnRebuild,productType,options, etc.) from the persisted context. These fields are re-computed on restore from the database relationships.src/lib/server/bullmq/types.ts (4)
37-39: LGTM!The
Rebuild_Parentjob type is appropriately placed with the build jobs and clearly documented as a parent job for admin-initiated rebuilds.
90-97: LGTM!The
Build.Parentinterface is well-designed with a requiredprojectIdfor identifying the rebuild scope and an optionalinitiatedByfor audit tracking. The comment clarifies this is a grouping type not processed by build workers.
342-342: LGTM!The
JobTypeMapcorrectly mapsRebuild_ParenttoBuild.Parent, ensuring type safety throughout the codebase.
305-308: Verify the parent job processing is handled appropriately.The
Rebuild_Parentjob is included inBuildJobunion. Ensure the build worker has logic to handle or skip this job type, since per the comment it's "not processed by build workers" but is a grouping parent job.src/lib/workflowTypes.ts (1)
137-137: LGTM! Well-structured type extensions and guard implementation.The new fields (
isAutomatic,parentJobId,autoPublishOnRebuild) integrate cleanly into the existing type hierarchy, and theautoPublishOnRebuildguard function correctly validates all three required conditions (autoPublishOnRebuild flag, isAutomatic flag, and Rebuild workflow type).Also applies to: 187-187, 194-196, 268-275
src/lib/server/email-service/locales/en-us.json (1)
77-77: LGTM! Localization keys properly structured.The new auto-publish notification keys follow the existing naming conventions and include appropriate placeholders for project and product names.
Also applies to: 119-120
src/lib/server/workflow/state-machine.ts (4)
61-61: LGTM! Context properly initialized with new fields.The context initialization correctly propagates
isAutomaticandautoPublishOnRebuildfrom the input, enabling the workflow to support automatic rebuild and auto-publish functionality.Also applies to: 68-68
504-531: LGTM! Clean refactor with proper parent job linkage.The refactored payload construction improves clarity by separating data and options objects. The conditional parent linkage (lines 527-529) correctly associates child build jobs with the parent rebuild job when
parentJobIdis provided.
556-565: LGTM! Auto-transition correctly gated.The new transition to
Product_Publishis properly guarded using theautoPublishOnRebuildfunction from workflowTypes, ensuring automatic publishing only occurs when all conditions are met (autoPublishOnRebuild flag, isAutomatic flag, and Rebuild workflow type).
840-857: LGTM! Notifications properly gated.The
notifyAutoPublishOwnercalls are correctly conditional on bothautoPublishOnRebuildandisAutomaticflags, ensuring product owners are notified only when automatic publishing completes on rebuild workflows.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/(authenticated)/products/[id]/files/+page.server.ts (1)
93-106: CallingrequireProjectReadAccesswithoutprojectwill always 404Here
projectis correctly fetched and validated:const project = (await DatabaseReads.products.findUnique(...))?.Project; if (!project) error(404);But the subsequent call omits it:
locals.security.requireProjectReadAccess( await DatabaseReads.groupMemberships.findMany({ ... }) // project not passed here );Given the current
Security.requireProjectReadAccessimplementation, a missingprojectargument is treated as falsy and results inerror(404, 'Project not found'), so this action will always fail with 404.You likely meant to keep passing
projecthere. Suggested fix:- locals.security.requireProjectReadAccess( - await DatabaseReads.groupMemberships.findMany({ - where: { UserId: locals.security.userId, GroupId: project.GroupId } - }) - ); + locals.security.requireProjectReadAccess( + await DatabaseReads.groupMemberships.findMany({ + where: { UserId: locals.security.userId, GroupId: project.GroupId } + }), + project + );
🧹 Nitpick comments (6)
src/lib/server/database/Projects.ts (1)
50-58: Reassign userMapping change looks good; consider guarding against nullexisting
The updated
userMapping: [{ from: existing!.OwnerId, to: ownerId }]correctly matches the narrowed{ from, to }[]type for the Reassign operation and preserves existing behavior (single mapping from previous owner to new owner). No issues with the change itself.Independently of this PR,
existingis nullable (findUniquecan returnnull), but it’s used with a non‑null assertion both when computingorgId/groupId/ownerIdand inuserMapping. Ifidwere ever invalid or the project deleted concurrently, this would throw before thetry/catchand surface as an unhandled error.You could defensively guard once after the lookup and then drop the
!:const existing = await prisma.projects.findUnique({ where: { Id: id } });
- if (!existing) {
- return false;
- }
- const orgId = projectData.OrganizationId ?? existing!.OrganizationId;
- const groupId = projectData.GroupId ?? existing!.GroupId;
- const ownerId = projectData.OwnerId ?? existing!.OwnerId;
- const orgId = projectData.OrganizationId ?? existing.OrganizationId;
- const groupId = projectData.GroupId ?? existing.GroupId;
- const ownerId = projectData.OwnerId ?? existing.OwnerId;
…
userMapping: [{ from: existing!.OwnerId, to: ownerId }]
userMapping: [{ from: existing.OwnerId, to: ownerId }]That keeps the new job payload shape while making the function safer under unexpected IDs.Also applies to: 70-78
src/lib/server/job-executors/userTasks.ts (1)
62-87: Reassign operation lacks validation; confirm this design is intentional.The reassignment logic (lines 62-87) bypasses the role validation that exists in the Create/Update/Delete path (line 173:
.filter((t) => allUsers[t.UserId].has(t.Role))).However, this appears intentional rather than accidental. Reassign is semantically different: it transfers existing tasks from one specific user to another via direct user mapping, whereas Create/Update/Delete regenerate tasks based on role eligibility. The current sole caller (Projects.ts) reassigns tasks during owner changes, where the new owner implicitly has the required role.
The concern is whether this design will remain valid if Reassign is used elsewhere. Consider adding:
- Documentation clarifying why Reassign has no role check
- Validation at the call site to ensure reassignments are to users with compatible roles (if needed for future use cases)
src/lib/server/job-executors/email.ts (1)
97-126: Reviewer and owner email construction is consistent; minor duplication opportunity onlyUsing
product.Project.Reviewersandproduct.Project.Ownerdirectly for recipient lists, locale selection, and owner-specificreviewerNamesis consistent with the rest of this module and should behave as expected, including the case with zero reviewers (owner email still sent).If you touch this again, consider extracting a tiny helper to build the shared
properties/reviewerNamesso the reviewer and owner branches don’t need to recompute similar data, but this is purely a readability win and not required for this PR.src/lib/server/build-engine-api/types.ts (1)
67-72: Consider adding runtime validation or explicit property mapping.The
toStatusResponsefunction uses a type assertion with the spread operator, which bypasses TypeScript's type checking. This allows any properties frombodyto leak through without validation, potentially violating theStatusResponsecontract (e.g., wrong types forversions,created, orupdated).Consider either:
- Explicitly mapping known properties:
{ responseType: 'status', status, versions: body.versions as StatusResponse['versions'], ... }- Adding runtime validation to ensure body matches the expected shape
Example with explicit mapping:
export function toStatusResponse( status: number, body: Record<string, unknown> = {} ): StatusResponse { - return { responseType: 'status', status, ...body } as StatusResponse; + return { + responseType: 'status', + status, + ...(body.versions && { versions: body.versions as StatusResponse['versions'] }), + ...(body.created && { created: body.created as string }), + ...(body.updated && { updated: body.updated as string }) + }; }src/auth.ts (1)
255-267: Optionalprojectparameter does not match runtime behavior
requireProjectReadAccessnow typesprojectas optional, but the implementation still hard-fails with a 404 wheneverprojectis falsy. That means any caller that relies on the new optional signature and omitsprojectwill always get a 404, even when a valid project was already fetched separately.Either keep the parameter required in the type, or update the implementation to have a well-defined behavior when
projectis omitted (e.g., treat “no project argument” differently from “project not found”). As it stands, the optional typing is misleading and has already enabled at least one broken call site.src/app.d.ts (1)
28-35: Security type now allows an unsafe call patternThe global
Security.requireProjectReadAccesssignature now makesprojectoptional, even though the concrete implementation still errors whenprojectis falsy. This loosened type is what allowed a call withoutprojectinproducts/[id]/files/+page.server.tsto compile but fail at runtime.To keep TS honest, consider either:
- Making
projectrequired again in this declaration, or- Explicitly documenting and implementing a distinct behavior for
project === undefinedvs.project === null(e.g., overloads), if you truly need a “no-project argument” variant.Right now the type surface doesn’t reflect the actual behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
.github/workflows/setup.yml(2 hunks)Dockerfile(0 hunks)README.md(0 hunks)run(0 hunks)src/app.d.ts(1 hunks)src/auth.ts(1 hunks)src/lib/components/settings/Toggle.svelte(1 hunks)src/lib/prisma/migrations/18_system_version_hash/migration.sql(0 hunks)src/lib/prisma/migrations/19_track_updates/migration.sql(0 hunks)src/lib/prisma/schema.prisma(0 hunks)src/lib/products/index.ts(0 hunks)src/lib/products/server.ts(3 hunks)src/lib/projects/sse.ts(1 hunks)src/lib/server/build-engine-api/types.ts(2 hunks)src/lib/server/bullmq/types.ts(1 hunks)src/lib/server/database/Projects.ts(1 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/build.ts(3 hunks)src/lib/server/job-executors/email.ts(4 hunks)src/lib/server/job-executors/system.ts(1 hunks)src/lib/server/job-executors/userTasks.ts(3 hunks)src/lib/server/workflow/index.ts(4 hunks)src/lib/server/workflow/state-machine.ts(1 hunks)src/lib/workflowTypes.ts(0 hunks)src/routes/(authenticated)/+layout.svelte(2 hunks)src/routes/(authenticated)/products/[id]/files/+page.server.ts(1 hunks)src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts(1 hunks)src/routes/(authenticated)/projects/[id=idNumber]/modals/Properties.svelte(3 hunks)src/routes/(authenticated)/tasks/[product_id]/instructions/GooglePlay_Configuration.svelte(3 hunks)src/routes/(authenticated)/tasks/[product_id]/instructions/GooglePlay_Verify_And_Publish.svelte(1 hunks)src/routes/(authenticated)/tasks/[product_id]/instructions/Verify_And_Publish.svelte(1 hunks)src/routes/(unauthenticated)/docs/[filename]/+server.ts(0 hunks)
💤 Files with no reviewable changes (9)
- README.md
- src/routes/(unauthenticated)/docs/[filename]/+server.ts
- Dockerfile
- run
- src/lib/workflowTypes.ts
- src/lib/products/index.ts
- src/lib/prisma/migrations/18_system_version_hash/migration.sql
- src/lib/prisma/schema.prisma
- src/lib/prisma/migrations/19_track_updates/migration.sql
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/server/bullmq/types.ts
- src/lib/server/email-service/locales/fr-FR.json
- src/lib/server/email-service/locales/es-419.json
🧰 Additional context used
🧠 Learnings (11)
📚 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/build.tssrc/lib/server/job-executors/email.tssrc/lib/server/workflow/state-machine.tssrc/lib/server/job-executors/system.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/build.ts
📚 Learning: 2025-09-10T13:51:14.948Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1275
File: src/routes/(authenticated)/projects/[id=idNumber]/+page.svelte:191-193
Timestamp: 2025-09-10T13:51:14.948Z
Learning: In the appbuilder-portal codebase, the `productsToAdd` property in project data is guaranteed to always be an array (possibly empty), never undefined.
Applied to files:
src/lib/server/job-executors/build.ts
📚 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/job-executors/build.tssrc/lib/server/workflow/index.tssrc/lib/products/server.tssrc/lib/server/workflow/state-machine.ts
📚 Learning: 2025-09-25T19:59:36.396Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts:22-29
Timestamp: 2025-09-25T19:59:36.396Z
Learning: In the AppBuilder portal project, the Security type is declared globally in src/app.d.ts and does not need to be explicitly imported when used in TypeScript files throughout the application.
Applied to files:
src/app.d.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/routes/(authenticated)/products/[id]/files/+page.server.tssrc/lib/products/server.tssrc/lib/server/job-executors/userTasks.tssrc/routes/(authenticated)/projects/[id=idNumber]/+page.server.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/routes/(authenticated)/products/[id]/files/+page.server.tssrc/routes/(authenticated)/projects/[id=idNumber]/+page.server.tssrc/lib/server/workflow/state-machine.ts
📚 Learning: 2025-09-10T13:46:43.497Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1275
File: src/auth.ts:240-245
Timestamp: 2025-09-10T13:46:43.497Z
Learning: In src/auth.ts, the products branch intentionally uses params.id as a string for database lookups, not as a number. This is the correct behavior and should not be changed to numeric parsing.
Applied to files:
src/routes/(authenticated)/products/[id]/files/+page.server.ts
📚 Learning: 2025-10-13T17:26:55.905Z
Learnt from: chrisvire
Repo: sillsdev/appbuilder-portal PR: 1343
File: src/routes/(unauthenticated)/api/auth/token/+server.ts:15-19
Timestamp: 2025-10-13T17:26:55.905Z
Learning: In src/routes/(unauthenticated)/api/auth/token/+server.ts, the GET handler correctly uses requireNothing() at the beginning because this endpoint initiates the authentication flow for AppBuilders. Auth establishment endpoints should allow unauthenticated access initially, as users may need to be redirected to login.
Applied to files:
src/routes/(authenticated)/products/[id]/files/+page.server.ts
📚 Learning: 2025-09-22T14:59:53.366Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts:214-221
Timestamp: 2025-09-22T14:59:53.366Z
Learning: In the AppBuilder portal, bulk project actions in the route `/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts` are organization-scoped by design. The route parameter `[orgId=idNumber]` ensures all operations happen within a single organization context, so using the route-level orgId for all projects in bulk actions like `doProjectAction` is correct and safe.
Applied to files:
src/routes/(authenticated)/products/[id]/files/+page.server.tssrc/routes/(authenticated)/projects/[id=idNumber]/+page.server.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/job-executors/userTasks.tssrc/lib/projects/sse.ts
🧬 Code graph analysis (5)
src/lib/products/server.ts (1)
src/lib/server/workflow/index.ts (1)
Workflow(36-582)
src/lib/server/job-executors/userTasks.ts (2)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)src/lib/server/database/index.ts (1)
DatabaseWrites(68-71)
src/lib/server/job-executors/email.ts (4)
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/email-service/locales/locale.ts (1)
translate(35-54)src/lib/server/email-service/EmailTemplates.ts (3)
addProperties(23-36)ReviewProductTemplate(19-19)NotificationTemplate(14-14)
src/lib/server/workflow/state-machine.ts (1)
src/lib/server/bullmq/queues.ts (1)
getQueues(150-155)
src/lib/server/job-executors/system.ts (2)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)src/lib/server/database/index.ts (1)
DatabaseWrites(68-71)
🔇 Additional comments (22)
src/lib/server/email-service/locales/en-us.json (1)
44-44: Clean removal of retryBuild infrastructure from locale strings.The removal of retryBuild notification entries from both
notifications.bodyandnotifications.subjectis correctly implemented, with proper JSON formatting (no trailing commas on the final entries in each section). TherecordNotFoundentries serve as appropriate fallback messages for missing record scenarios and align well with the broader PR objective to simplify retry-based workflows.Also applies to: 85-85
src/lib/components/settings/Toggle.svelte (1)
47-48: Icon order & active-state styling may invert visual on/off semanticsSwapping the order and
color="white"betweenonIconandoffIconwill change which icon is emphasized and potentially which one appears in the “on” vs “off” state, depending on how.toggleis styled (e.g., sibling selectors or first/last-child rules). Please double-check the rendered toggle in both states to ensure this matches the intended design for all usages ofToggle.src/lib/server/job-executors/system.ts (1)
157-196: The current design handles versioning correctly; race conditions are unlikely and error handling is atomic.The pre-fetch at line 157 combined with subsequent updates is safe given BullMQ's default concurrency of 1 for this worker, so concurrent execution is not a concern. The lack of try-catch is intentional—if any database operation fails, the job fails atomically rather than leaving partial updates. If partial-failure tolerance is desired (updating some versions while skipping failed ones), consider using
Promise.allSettled()instead ofPromise.all()and filtering for errors.src/lib/server/job-executors/userTasks.ts (2)
6-6: LGTM: Type-only import is appropriate.The change to a type-only import for
RoleIdis correct since it's only used for type assertions (line 151) and not as a runtime value. This improves tree-shaking and makes the type-level usage explicit.
28-28: LGTM: Removal of unused field improves query efficiency.The
OrganizationIdfield is not referenced anywhere in the function body, so removing it from the select clause is a good optimization.src/lib/projects/sse.ts (1)
36-40: SelectApplicationTypeIdinstead ofDescriptionfor the filterThe concern about using
Descriptionas a filter is valid—descriptions are typically non-unique and fragile for filtering. However, the current code is actually using valid Prisma syntax for a to-one relation filter (not a list relation), so the suggestion to usesome: { ... }is incorrect and would cause a type error.Instead, update the select to include the
Id:ApplicationType: { select: { + Id: true, Description: true } },Then filter by the relation's Id directly (without
some, which applies only to list relations):const productDefinitions = ( await DatabaseReads.organizationProductDefinitions.findMany({ where: { OrganizationId: project.OrganizationId, ProductDefinition: { - ApplicationTypes: project.ApplicationType + ApplicationTypes: { + Id: project.ApplicationType.Id + } } },Likely an incorrect or invalid review comment.
src/lib/server/job-executors/email.ts (2)
56-95: Product fetch, artifact URL extraction, and comment-aware messageId look solid; verify i18n coverageThe switch to including
Project(withOrganization,Reviewers,Owner),ProductArtifacts, andProductDefinitionplus deriving URLs viaProductArtifacts.find(...)is correct and clearer, and usingproduct.WorkflowCommentto appendWithCommenttomessageIdand populatecommentinpropertieskeeps email content aligned with stored workflow state.Please double‑check that all
notifications.subject.*/notifications.body.*keys exist for the new*WithCommentvariants (e.g.,reviewProductWithComment,reviewPwaProductWithComment, etc.) so we don’t hit missing-translation or template-property errors at runtime.
239-248: Low‑priority super‑admin emails now ignorelinkand always useNotificationTemplate; ensure callers embed links in the bodyAlways using
NotificationTemplateand droppingLinkUrl/UrlTextparameters simplifies this path and avoids dependency on the link-specific template, assuming any URLs are now baked into the localizednotifications.body.<messageKey>strings viamessageProperties.Please confirm that no existing producers of
NotifySuperAdminsLowPrioritystill expect a clickable link supplied via a separatelinkfield; if they do, their templates or message properties will need updating to keep the link visible.src/routes/(authenticated)/projects/[id=idNumber]/modals/Properties.svelte (1)
80-83: Event wiring for compute type selection/default looks goodThe
onchangehandler callingupdateComputeType(e.currentTarget.value as ComputeType)together withbind:value={computeType}, and the "Default" button callingupdateComputeType()are consistent and keep the select and JSON in sync once the parsing issue above is addressed. No changes needed here from a logic standpoint.Also applies to: 113-114
src/routes/(authenticated)/tasks/[product_id]/instructions/Verify_And_Publish.svelte (1)
10-16: LGTM!The documentation URL update to
scriptoria.io/docsis consistent with the broader domain consolidation across the PR..github/workflows/setup.yml (2)
154-154: LGTM!The double-quote formatting for environment variable values is a minor style consistency improvement.
182-184: LGTM!Consistent quoting style for test environment variables.
src/routes/(authenticated)/tasks/[product_id]/instructions/GooglePlay_Verify_And_Publish.svelte (1)
14-18: LGTM!The documentation URL update to
scriptoria.io/docsis consistent with the broader domain consolidation, and the single-line anchor formatting improves readability.src/lib/server/job-executors/build.ts (3)
8-8: LGTM!Import simplified to only include
fetchPackageName, consistent with the removal of compute-type management functionality.
151-153: LGTM!The
Propertiesfield was removed from the query as it's no longer needed after removing the compute-type retry logic.
279-285: The structured comment format is handled correctly downstream.The
TaskCommentcomponent (src/lib/products/components/TaskComment.svelte) explicitly parses thesystem.build-failed,{consoleText}format, extracting the URL and presenting it as a clickable link to the console text. The change is compatible with existing downstream processing.src/lib/server/workflow/state-machine.ts (1)
767-772: The code change is correct. The type definition forEmail.SendNotificationToReviewersatsrc/lib/server/bullmq/types.ts:234-236only requirestypeandproductIdfields—nocommentfield exists in the interface. The payload being sent matches the type definition exactly.src/routes/(authenticated)/tasks/[product_id]/instructions/GooglePlay_Configuration.svelte (1)
4-35: Content and link updates look consistentThe revised navigation text, Play listing wording, keystore guidance, and updated documentation URL all look coherent and consistent with the rest of the instructions. No code-level issues here.
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts (1)
56-57: Rebuild toggles are now dev-only
showRebuildTogglesis restricted toenv.NODE_ENV === 'development', which will hide these controls in staging/preview/prod regardless of any APP_ENV-style flags you may have. If you still need to exercise rebuild toggles outside local dev, consider widening this condition (e.g., to include specific non-prod environments).src/lib/products/server.ts (1)
7-11: Comment propagation through product actions looks correctThe extended
doProductActionsignature withcomment?: stringcleanly threads the comment intoWorkflow.createfor Rebuild/Republish and into theCancelWorkflowProductTransitions record. Existing call sites that don’t care about comments remain compatible since the parameter is optional.Also applies to: 53-61, 81-88
src/routes/(authenticated)/+layout.svelte (1)
220-229: Admin software-update nav and help URL change are consistentThe new
/software-updatenav item is correctly gated by the existing admin check, usesisUrlActive/activeOrgUrl, and follows the same drawer-close pattern as other admin links. The updated Help link URL is a straightforward target change with no code-side impact.Also applies to: 341-344
src/lib/server/workflow/index.ts (1)
51-56: Workflow comment plumbing and state-change simplification look soundThe optional
commentparameter onWorkflow.createis correctly persisted on the initialStartWorkflowtransition and then reused viainspect/updateProductTransitionsand the UserTasks update job. ThestateChangesimplification to a straight state-name comparison, along with the adjusted visualization logic for transitions without explicit targets, is consistent with the removal of retry-specific behavior and shouldn’t affect normal flows.Also applies to: 90-96, 237-261, 287-287, 355-359
becca-perk
force-pushed
the
feature/software-update-rebuild
branch
from
2cb79b0 to
550a6a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts (2)
98-127: Optimize N+1 query pattern.Lines 104-114 execute a separate
systemVersions.findUniquequery for each product, creating an N+1 performance problem that will degrade significantly as the number of eligible products grows.Fetch all required SystemVersions in a single query upfront, then perform in-memory lookups:
const productsForRebuild: ProductToRebuild[] = []; + // Fetch all unique SystemVersions needed + const uniqueKeys = Array.from( + new Set( + eligibleProducts.map( + (p) => `${p.Project.Organization.BuildEngineUrl ?? ''}|${p.Project.TypeId}` + ) + ) + ); + + const systemVersionsRaw = await DatabaseReads.systemVersions.findMany({ + where: { + OR: uniqueKeys.map((key) => { + const [buildEngineUrl, typeIdStr] = key.split('|'); + return { + BuildEngineUrl: buildEngineUrl, + ApplicationTypeId: Number(typeIdStr) + }; + }) + }, + select: { + BuildEngineUrl: true, + ApplicationTypeId: true, + Version: true + } + }); + + // Create lookup map + const systemVersionsMap = new Map( + systemVersionsRaw.map((sv) => + [`${sv.BuildEngineUrl}|${sv.ApplicationTypeId}`, sv.Version] + ) + ); // 2. Iterate through eligible products to perform the cross-model version check. for (const product of eligibleProducts) { const latestProductBuild = product.ProductBuilds[0]; const latestVersion = latestProductBuild?.AppBuilderVersion ?? null; - // Get the required SystemVersion for this specific project's type and organization's build engine URL. - const requiredSystemVersion = await DatabaseReads.systemVersions.findUnique({ - where: { - BuildEngineUrl_ApplicationTypeId: { - BuildEngineUrl: product.Project.Organization.BuildEngineUrl ?? '', - ApplicationTypeId: product.Project.TypeId - } - }, - select: { - Version: true - } - }); - - const requiredVersion = requiredSystemVersion?.Version ?? null; + // Look up the required SystemVersion from the map + const key = `${product.Project.Organization.BuildEngineUrl ?? ''}|${product.Project.TypeId}`; + const requiredVersion = systemVersionsMap.get(key) ?? null; // 3. Apply the final filtering logic:
140-141: Fix inverted security check logic.The security check conditions are backwards:
- Line 140: When
params.orgIdexists (truthy), it callsrequireAdminOfOrgIn(searchOrgs)checking admin rights for all orgs- Line 141: When
params.orgIdis missing, it callsrequireAdminOfOrg(Number(params.orgId))which evaluates torequireAdminOfOrg(NaN)and will failThis is the opposite of the correct logic in the
startaction (lines 174-175).Apply this diff to match the correct logic in the
startaction:// Determine what organizations are being affected const searchOrgs = await getOrganizations(locals, params); - if (Number(params.orgId)) locals.security.requireAdminOfOrgIn(searchOrgs); - else locals.security.requireAdminOfOrg(Number(params.orgId)); + if (isNaN(Number(params.orgId))) locals.security.requireAdminOfOrgIn(searchOrgs); + else locals.security.requireAdminOfOrg(Number(params.orgId));
🧹 Nitpick comments (1)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts (1)
28-36: Remove redundant RoleId.SuperAdmin from the filter.Line 31 includes
RoleId.SuperAdminin the role filter, but super admins have already returned early at line 21-26. This filter will only execute for non-super-admin users, making theSuperAdmininclusion redundant.Apply this diff:
const roles = await DatabaseReads.userRoles.findMany({ where: { UserId: locals.security.userId, - RoleId: { in: [RoleId.SuperAdmin, RoleId.OrgAdmin] } + RoleId: RoleId.OrgAdmin }, select: { OrganizationId: true } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
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/products/server.ts(3 hunks)src/lib/server/workflow/index.ts(2 hunks)src/routes/(authenticated)/+layout.svelte(1 hunks)src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts(1 hunks)src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/lib/products/server.ts
- src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte
- src/lib/locales/fr-FR.json
- src/lib/server/workflow/index.ts
- src/routes/(authenticated)/+layout.svelte
- src/lib/locales/es-419.json
🧰 Additional context used
🧠 Learnings (14)
📚 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/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-22T14:59:53.366Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts:214-221
Timestamp: 2025-09-22T14:59:53.366Z
Learning: In the AppBuilder portal, bulk project actions in the route `/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts` are organization-scoped by design. The route parameter `[orgId=idNumber]` ensures all operations happen within a single organization context, so using the route-level orgId for all projects in bulk actions like `doProjectAction` is correct and safe.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.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/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-12T14:31:23.906Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1283
File: src/lib/server/workflow/index.ts:52-74
Timestamp: 2025-09-12T14:31:23.906Z
Learning: In the appbuilder-portal codebase, optional chaining in database queries (like `check?.Project._count.Authors`) is primarily used to satisfy TypeScript's type checker rather than handling expected null cases. The `check` being null should realistically never happen in the normal application flow.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-22T15:00:43.129Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(authenticated)/users/[id=idNumber]/settings/+layout.server.ts:7-10
Timestamp: 2025-09-22T15:00:43.129Z
Learning: In user settings layouts, use the `adminOrgs` function from `$lib/users/server` to determine admin access rather than throwing 403 errors upfront. The `adminOrgs` function creates a filter that returns organizations where the subject user is a member AND the current user is an admin of that organization.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-25T19:44:14.845Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(authenticated)/projects/import/[id=idNumber]/+page.server.ts:24-26
Timestamp: 2025-09-25T19:44:14.845Z
Learning: The `requireHasRole(organizationId, roleId, orOrgAdmin)` method in the Security class allows organization admins to pass role checks when the third parameter `orOrgAdmin` is set to `true`, even if they don't have the specific role being checked for.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.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/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-10-01T15:58:37.578Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1317
File: src/routes/(authenticated)/users/+page.svelte:51-55
Timestamp: 2025-10-01T15:58:37.578Z
Learning: In the appbuilder-portal codebase, the $orgActive store (from $lib/stores) is designed to always contain a valid organization ID and should never be null. Code that syncs values to $orgActive should include guard clauses to prevent null assignments.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-10T13:46:43.497Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1275
File: src/auth.ts:240-245
Timestamp: 2025-09-10T13:46:43.497Z
Learning: In src/auth.ts, the products branch intentionally uses params.id as a string for database lookups, not as a number. This is the correct behavior and should not be changed to numeric parsing.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-10-02T01:36:08.124Z
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1318
File: src/lib/components/OrganizationDropdown.svelte:30-39
Timestamp: 2025-10-02T01:36:08.124Z
Learning: In the `src/lib/components/OrganizationDropdown.svelte` component, the `organizations` array will always contain at least one organization, so there's no need to handle the empty array case.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.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/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-10T13:51:14.948Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1275
File: src/routes/(authenticated)/projects/[id=idNumber]/+page.svelte:191-193
Timestamp: 2025-09-10T13:51:14.948Z
Learning: In the appbuilder-portal codebase, the `productsToAdd` property in project data is guaranteed to always be an array (possibly empty), never undefined.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-25T03:51:13.814Z
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts:7-8
Timestamp: 2025-09-25T03:51:13.814Z
Learning: In the organization invitation flow in src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts, the checkInviteErrors function properly handles null/undefined invite tokens by accepting inviteToken?: string | null and having an explicit guard clause if (!inviteToken) return { error: 'not found' }, making the non-null assertion on searchParams.get('t')! safe since error handling is centralized.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-25T03:51:13.814Z
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts:7-8
Timestamp: 2025-09-25T03:51:13.814Z
Learning: In the organization invitation flow in src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts, the checkInviteErrors function handles validation of invite tokens including null/undefined cases, so non-null assertion on searchParams.get('t') is safe because error handling is centralized in checkInviteErrors.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
🧬 Code graph analysis (1)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts (2)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)src/lib/products/server.ts (1)
doProductAction(7-97)
⏰ 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 (3)
src/lib/locales/en-US.json (1)
559-567: LGTM!The localization keys are well-structured and align with the new admin software update feature. The key names follow existing conventions and clearly indicate their purpose.
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts (2)
39-46: LGTM!The function correctly handles both cases: when a specific organization ID is provided via route parameter, and when it's missing (in which case it determines all target organizations).
165-186: LGTM!The action handler correctly validates the form, performs proper security checks (lines 174-175), fetches eligible products, and triggers rebuilds in parallel using
Promise.all. The logic is sound and efficient.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts (2)
98-127: N+1 query pattern persists.The loop on lines 99-127 still performs a database query per product (
DatabaseReads.systemVersions.findUniqueon line 104). This was flagged in a previous review and marked as addressed, but the code still exhibits the N+1 pattern.Consider batching the SystemVersion lookups as previously suggested: fetch all unique
(BuildEngineUrl, ApplicationTypeId)pairs upfront, then use an in-memory map for lookups.
140-141: Security check logic is inverted.The security check logic here is backwards compared to the correct implementation in the
startaction (lines 209-210):
- Line 140: When
params.orgIdis a valid number (truthy), it callsrequireAdminOfOrgIn(searchOrgs)instead ofrequireAdminOfOrg- Line 141: When
params.orgIdis undefined/NaN (falsy), it callsrequireAdminOfOrg(NaN)which will always failThis was previously flagged and appears unresolved.
Proposed fix
const searchOrgs = await getOrganizations(locals, params); - if (Number(params.orgId)) locals.security.requireAdminOfOrgIn(searchOrgs); - else locals.security.requireAdminOfOrg(Number(params.orgId)); + if (isNaN(Number(params.orgId))) locals.security.requireAdminOfOrgIn(searchOrgs); + else locals.security.requireAdminOfOrg(Number(params.orgId));
🧹 Nitpick comments (3)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte (1)
51-80: Hardcoded English text should be localized.Several strings are hardcoded in English which breaks i18n consistency:
- Line 52:
"Update Summary"- Line 61:
"Projects:"- Line 65:
"Products:"- Line 70:
"Project Names:"- Line 76:
"Target Versions:"Additionally, the pluralization logic on lines 62 and 66 (
!== 1 ? 's' : '') is English-specific and won't work correctly for other languages. Consider using a proper i18n pluralization function.src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts (2)
153-182: Consider consolidating database queries.There are multiple sequential queries that could be optimized:
getProductsForRebuildalready fetches product/project data- Line 157-170 re-fetches products just to get
Project.Name- Line 174-182 re-fetches projects by ID
Consider extending
getProductsForRebuildto includeProject.Namein its select clause, eliminating the need for the subsequent queries.Proposed optimization sketch
// In getProductsForRebuild, add Name to Project select: Project: { select: { TypeId: true, + Name: true, Organization: { select: { BuildEngineUrl: true } } } } // Then return project info along with product data, eliminating // the need for separate affectedProducts and projects queries in load()
220-224: Consider error handling for partial rebuild failures.
Promise.allwill reject immediately if anydoProductActioncall fails, potentially leaving the operation in an inconsistent state where some products started rebuilding and others didn't.Consider using
Promise.allSettledto handle partial failures gracefully, or wrap with try-catch to provide meaningful feedback about which products failed.Example with Promise.allSettled
- await Promise.all( - productsToRebuild.map((p) => - doProductAction(p.id, ProductActionType.Rebuild, form.data.comment) - ) - ); + const results = await Promise.allSettled( + productsToRebuild.map((p) => + doProductAction(p.id, ProductActionType.Rebuild, form.data.comment) + ) + ); + + const failedCount = results.filter((r) => r.status === 'rejected').length; + if (failedCount > 0) { + // Log or handle partial failures + console.warn(`${failedCount} of ${productsToRebuild.length} rebuilds failed to start`); + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts(1 hunks)src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (13)
📚 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/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-22T14:59:53.366Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts:214-221
Timestamp: 2025-09-22T14:59:53.366Z
Learning: In the AppBuilder portal, bulk project actions in the route `/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts` are organization-scoped by design. The route parameter `[orgId=idNumber]` ensures all operations happen within a single organization context, so using the route-level orgId for all projects in bulk actions like `doProjectAction` is correct and safe.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.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/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-12T14:31:23.906Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1283
File: src/lib/server/workflow/index.ts:52-74
Timestamp: 2025-09-12T14:31:23.906Z
Learning: In the appbuilder-portal codebase, optional chaining in database queries (like `check?.Project._count.Authors`) is primarily used to satisfy TypeScript's type checker rather than handling expected null cases. The `check` being null should realistically never happen in the normal application flow.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-22T15:00:43.129Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(authenticated)/users/[id=idNumber]/settings/+layout.server.ts:7-10
Timestamp: 2025-09-22T15:00:43.129Z
Learning: In user settings layouts, use the `adminOrgs` function from `$lib/users/server` to determine admin access rather than throwing 403 errors upfront. The `adminOrgs` function creates a filter that returns organizations where the subject user is a member AND the current user is an admin of that organization.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-25T19:44:14.845Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(authenticated)/projects/import/[id=idNumber]/+page.server.ts:24-26
Timestamp: 2025-09-25T19:44:14.845Z
Learning: The `requireHasRole(organizationId, roleId, orOrgAdmin)` method in the Security class allows organization admins to pass role checks when the third parameter `orOrgAdmin` is set to `true`, even if they don't have the specific role being checked for.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.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/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-10-01T15:58:37.578Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1317
File: src/routes/(authenticated)/users/+page.svelte:51-55
Timestamp: 2025-10-01T15:58:37.578Z
Learning: In the appbuilder-portal codebase, the $orgActive store (from $lib/stores) is designed to always contain a valid organization ID and should never be null. Code that syncs values to $orgActive should include guard clauses to prevent null assignments.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-10T13:46:43.497Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1275
File: src/auth.ts:240-245
Timestamp: 2025-09-10T13:46:43.497Z
Learning: In src/auth.ts, the products branch intentionally uses params.id as a string for database lookups, not as a number. This is the correct behavior and should not be changed to numeric parsing.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-10-02T01:36:08.124Z
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1318
File: src/lib/components/OrganizationDropdown.svelte:30-39
Timestamp: 2025-10-02T01:36:08.124Z
Learning: In the `src/lib/components/OrganizationDropdown.svelte` component, the `organizations` array will always contain at least one organization, so there's no need to handle the empty array case.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.tssrc/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte
📚 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/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-25T03:51:13.814Z
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts:7-8
Timestamp: 2025-09-25T03:51:13.814Z
Learning: In the organization invitation flow in src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts, the checkInviteErrors function properly handles null/undefined invite tokens by accepting inviteToken?: string | null and having an explicit guard clause if (!inviteToken) return { error: 'not found' }, making the non-null assertion on searchParams.get('t')! safe since error handling is centralized.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
📚 Learning: 2025-09-25T03:51:13.814Z
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts:7-8
Timestamp: 2025-09-25T03:51:13.814Z
Learning: In the organization invitation flow in src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts, the checkInviteErrors function handles validation of invite tokens including null/undefined cases, so non-null assertion on searchParams.get('t') is safe because error handling is centralized in checkInviteErrors.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts
🧬 Code graph analysis (1)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts (2)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)src/lib/products/server.ts (1)
doProductAction(7-97)
🪛 GitHub Check: checks / typecheck-lint
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte
[failure] 19-19:
Unexpected any. Specify a different type
[warning] 5-5:
$lib/components/settings/DataDisplayBox.svelte import should occur before import of $lib/components/settings/LabeledFormInput.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). (1)
- GitHub Check: checks / build-and-test
🔇 Additional comments (1)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.server.ts (1)
20-37: LGTM!The helper functions correctly handle the super admin case by fetching all organizations, and for regular admins, properly query user roles filtered by
RoleId.SuperAdminandRoleId.OrgAdmin. The use ofSetto deduplicate organization IDs is appropriate.
|
I added some enhancements to the UI that were suggested here: #1249 (comment) However, I am still having trouble testing it because we are still not able to publish the build, and I believe that is required for the rebuilds. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte (2)
17-31: Define a proper type for the server response instead ofas any.The type assertion on line 19 bypasses TypeScript's type safety. This was previously flagged and a solution was proposed.
🔎 Proposed fix
+interface RebuildResponse { + ok?: boolean; + initiatedBy?: string; + comment?: string; + productCount?: number; + timestamp?: string; +} + const { form, enhance, reset } = superForm(data.form, { resetForm: false, onUpdated({ form }) { - // Type assertion to access server action response properties - const response = form.data as any; + const response = form.data as RebuildResponse; if (form.valid && response.ok) {
104-122: Fix wrong localization key and hardcoded strings in summary display.This section has multiple localization issues already flagged in previous reviews:
- Line 109 uses
admin_software_update_affected_organizationsas the key but displayssummary.initiatedBy- semantically incorrect- "Rebuild Started" (line 106) and "Products Rebuilding:" (line 119) are hardcoded English
🔎 Proposed fix
<DataDisplayBox - title="Rebuild Started" + title={m.admin_software_update_rebuild_started()} fields={[ { - key: 'admin_software_update_affected_organizations', + key: 'admin_software_update_initiated_by', value: summary.initiatedBy }, { key: 'admin_nav_software_update_comment', value: summary.comment } ]} > <p style="padding-left: 1rem; text-indent: -1rem"> - <b>Products Rebuilding:</b> + <b>{m.admin_software_update_products_rebuilding()}:</b> {summary.productCount ?? 0} </p>Add the corresponding keys to your locale files.
🧹 Nitpick comments (1)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte (1)
50-80: Localize hardcoded English labels in the Update Summary section.Several labels are hardcoded in English and should use localization keys for consistency:
- "Update Summary" (line 52)
- "Projects:" (line 61)
- "Products:" (line 65)
- "Project Names:" (line 70)
- "Target Versions:" (line 76)
🔎 Suggested approach
Add localization keys to your locale files and reference them:
<DataDisplayBox - title="Update Summary" + title={m.admin_software_update_summary()} fields={[ { key: 'admin_software_update_affected_organizations', value: data.organizations } ]} > <p style="padding-left: 1rem; text-indent: -1rem"> - <b>Projects:</b> + <b>{m.admin_software_update_projects_label()}:</b> {data.affectedProjectCount} project{data.affectedProjectCount !== 1 ? 's' : ''} </p> <p style="padding-left: 1rem; text-indent: -1rem"> - <b>Products:</b> + <b>{m.admin_software_update_products_label()}:</b> {data.affectedProductCount} product{data.affectedProductCount !== 1 ? 's' : ''} </p> {#if data.affectedProjects && data.affectedProjects.length > 0} <p style="padding-left: 1rem; text-indent: -1rem" class="text-sm opacity-75"> - <b>Project Names:</b> + <b>{m.admin_software_update_project_names()}:</b> {data.affectedProjects.join(', ')} </p> {/if} {#if data.affectedVersions && data.affectedVersions.length > 0} <p style="padding-left: 1rem; text-indent: -1rem" class="text-sm opacity-75"> - <b>Target Versions:</b> + <b>{m.admin_software_update_target_versions()}:</b> {data.affectedVersions.join(', ')} </p> {/if}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-02T01:36:08.124Z
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1318
File: src/lib/components/OrganizationDropdown.svelte:30-39
Timestamp: 2025-10-02T01:36:08.124Z
Learning: In the `src/lib/components/OrganizationDropdown.svelte` component, the `organizations` array will always contain at least one organization, so there's no need to handle the empty array case.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte
📚 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/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte
📚 Learning: 2025-09-22T15:01:28.480Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/lib/projects/index.ts:185-197
Timestamp: 2025-09-22T15:01:28.480Z
Learning: In the AppBuilder portal project, there's a policy inconsistency in canClaimProject: the docstring says "AppBuilder or OrgAdmin roles" but the implementation only allows OrgAdmin (via canModifyProject → isAdminForOrg). The UI code suggests AppBuilder users should have some project modification rights. This needs clarification from the maintainer.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte
🪛 GitHub Check: checks / typecheck-lint
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte
[failure] 19-19:
Unexpected any. Specify a different type
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte (1)
112-130: Unresolved localization issues remain in the "Rebuild Started" section.This section still contains issues previously flagged in past reviews:
- Line 114:
"Rebuild Started"is hardcoded English and should use a localization key.- Line 117: The field key
'admin_software_update_affected_organizations'is semantically incorrect—it displayssummary.initiatedBy(the user who started the rebuild) but uses a key meant for organization names. This should be'admin_software_update_initiated_by'or similar.- Line 127:
"Products Rebuilding:"is hardcoded English and should use a localization key.🔎 Proposed fix
{#if showSummary && summary} <DataDisplayBox - title="Rebuild Started" + title={m.admin_software_update_rebuild_started()} fields={[ { - key: 'admin_software_update_affected_organizations', + key: 'admin_software_update_initiated_by', value: summary.initiatedBy }, { key: 'admin_nav_software_update_comment', value: summary.comment } ]} > <p style="padding-left: 1rem; text-indent: -1rem"> - <b>Products Rebuilding:</b> + <b>{m.admin_software_update_products_rebuilding()}</b> {summary.productCount ?? 0} </p> </DataDisplayBox> {/if}Add the corresponding keys to your locale files.
🧹 Nitpick comments (1)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte (1)
34-34: Timestamp field is captured but never displayed.Line 34 stores
response.timestampin the summary object, but this field is never rendered in the "Rebuild Started" DataDisplayBox (lines 112-131). Consider either displaying the timestamp to provide users with a complete audit trail or removing it from the summary state if it's not needed.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-02T01:36:08.124Z
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1318
File: src/lib/components/OrganizationDropdown.svelte:30-39
Timestamp: 2025-10-02T01:36:08.124Z
Learning: In the `src/lib/components/OrganizationDropdown.svelte` component, the `organizations` array will always contain at least one organization, so there's no need to handle the empty array case.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte
📚 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/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.svelte
📚 Learning: 2025-09-22T15:01:28.480Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/lib/projects/index.ts:185-197
Timestamp: 2025-09-22T15:01:28.480Z
Learning: In the AppBuilder portal project, there's a policy inconsistency in canClaimProject: the docstring says "AppBuilder or OrgAdmin roles" but the implementation only allows OrgAdmin (via canModifyProject → isAdminForOrg). The UI code suggests AppBuilder users should have some project modification rights. This needs clarification from the maintainer.
Applied to files:
src/routes/(authenticated)/software-update/[[orgId=idNumber]]/+page.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). (1)
- GitHub Check: checks / build-and-test
…nd removed persistent values
Also fix: Authentication needs to handle multiple orgs properly.
…, improved UI, and SSE support for status updates
- Extend JobType enum to include SvelteSSE_UpdateSoftwareUpdates. - Introduce UpdateSoftwareUpdates interface in SvelteProjectSSE namespace. - Modify recordRebuilds function to handle organization-specific rebuild requests. - Implement getRebuildsForOrgIds function to fetch rebuilds based on organization IDs. - Update SSE endpoint to emit rebuild updates for relevant organizations. - Enhance RebuildCard and ApplicationTypesSelector components for better display and handling of software updates. - Refactor code for improved readability and maintainability.
…error handling in SSE updates
… logging, and edit style of rebuildcard
Instead of passing organization IDs in a query-string parameter, use the post body. This avoids parsing.
This generally reduces the entire getProductsForRebuild to two query+map/reduce operations, one to fetch the current version table and the other to fetch and filter through products.
Once again overhauling load to further simplify the query and data returned by it.
judah-sotomayor
force-pushed
the
feature/software-update-rebuild
branch
from
1864845 to
b7c225b
Compare
This temporarily removes SSE functionality for the sake of making sure MVP is there. Basically, provide a stable framework to build on top of to provide SSE. Queries are much simplified. The function to get rebuilds has been redesigned to only fetch rebuilds from either one organization or every organization the user is a member of. This streamlines the code. I have also removed a few files and consolidated their code into other files: sse.ts into the software-updates/index.ts file, and ApplicationTypesSelector into a built-in section of the main page.svelte.
Add SSE back, adjusted to take advantage of the simplified software update functions.
FyreByrd
left a comment
FyreByrd
left a comment
There was a problem hiding this comment.
This looks good so far. I think we will need to talk a bit about what metadata we need to keep track of, as well as a good strategy for cancelling updates.
| * Checks if a specific product's successful build completes any open SoftwareUpdates. | ||
| * Called after each successful build for immediate completion detection. | ||
| */ | ||
| export async function completeForProduct(productId: string): Promise<void> { |
There was a problem hiding this comment.
So, if I understand this correctly, an update is marked as complete as soon as any one product within it has been built successfully?
There was a problem hiding this comment.
Sorry to complicate things, but we may need a bit more metadata on the relation between products and software updates...
There was a problem hiding this comment.
Will be resolved as per our call and discord discussion.
| } | ||
|
|
||
| if (ok) { | ||
| const orgIds = [...new Set<number>(u.Products.map((p) => p.Project.Organization.Id))]; |
There was a problem hiding this comment.
If this function is just for marking a single product as having completed its portion of the update, you only need to update SSE for one org.
There was a problem hiding this comment.
It is meant to run on the end of every product build in order to check if this product is the last one a software update is waiting for. If it is, close the software update.
| {#if $errors.comment} | ||
| <span class="text-error text-sm">{m.admin_software_update_comment_required()}</span> | ||
| {/if} | ||
| <!--<span class="validator-hint">{m.admin_software_update_comment_required()}</span>--> |
There was a problem hiding this comment.
@becca-perk You didn't need to make this change. This should have been fine as is and would have matched the error handling we have elsewhere.
There was a problem hiding this comment.
There was an issue going on where after submitting the form for a software update the comment box would pop up a validation error saying you need a comment to submit the form. This commit was supposed to fix that, but if it's not the right approach do you have any suggestions?
There was a problem hiding this comment.
Do you want the form to reset after it's been submitted? If so, that may be where the error is coming from... let me look into it real quick.
There was a problem hiding this comment.
Ok. If you want to do it this way, it's probably fine. Otherwise I found one of the few locations that has a similar problem. You can look at /projects/[id]/forms/Reviewers.svelte if you want to see how I went about resolving it.
4 participants
Creates new Software Update page in Admin and enables administrators to chose to begin automated builds.
See #1249
Summary by CodeRabbit
New Features
Enhancements