Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughThe PR introduces three new BullMQ job types for granular project SSE updates (groups, org, products), expands SSE data endpoints with dedicated getters, minifies product data structures with abbreviated field names, refactors workflow/build parameter names (J/CB/CR), and adds database job enqueueing and UI updates for product counts and organization displays. Changes
Sequence DiagramsequenceDiagram
actor User
participant Client
participant Server as Project SSE Server
participant Queue as BullMQ
participant Worker as SSE Worker
participant Database
User->>Client: Triggers data change<br/>(e.g., update product, author, org)
Client->>Server: POST request to database endpoint
Server->>Database: Execute update operation
Database-->>Server: Confirmation
Server->>Queue: Enqueue job<br/>(SvelteSSE_UpdateProjectGroups/Org/Products)
Queue-->>Server: Job queued
Server-->>Client: Response
Worker->>Queue: Poll job queue
Queue-->>Worker: Retrieve job
Worker->>Database: Fetch latest data<br/>(getProjectGroupData/OrgData/Products)
Database-->>Worker: Return minified data
Worker->>Client: Emit SSE event<br/>(projectGroups/projectOrg/productData)
Client->>Client: Update local state
Client-->>User: Reflect changes in UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/server/database/Organizations.ts (1)
50-97:⚠️ Potential issue | 🟠 MajorMake the membership change and cleanup atomic.
This now splits one logical operation across two separate
users.updatecalls. If the first disconnect succeeds and the second cleanup fails, the user is removed from the organization while still retaining org groups/roles, leaving permissions and project visibility inconsistent.🧩 Suggested shape
- const updated = - (enabled || !userHasProjectInOrg) && - !!(await prisma.users.update({ - where: { Id: UserId }, - data: { - Organizations: { - [enabled ? 'connect' : 'disconnect']: { - Id: OrganizationId - } - } - }, - select: { - Id: true - } - })); - - if (updated) { - // remove user from groups/roles too - if (!enabled) { - await prisma.users.update({ - where: { Id: UserId }, - data: { - Groups: { - disconnect: await prisma.groups.findMany({ - where: { OwnerId: OrganizationId, Users: { some: { Id: UserId } } }, - select: { Id: true } - }) - }, - UserRoles: { - deleteMany: { - OrganizationId - } - } - } - }); - } - await getQueues().SvelteSSE.add( + if (!(enabled || !userHasProjectInOrg)) return false; + + const projectIds = await prisma.$transaction(async (tx) => { + await tx.users.update({ + where: { Id: UserId }, + data: { + Organizations: { + [enabled ? 'connect' : 'disconnect']: { + Id: OrganizationId + } + } + }, + select: { Id: true } + }); + + if (!enabled) { + const groups = await tx.groups.findMany({ + where: { OwnerId: OrganizationId, Users: { some: { Id: UserId } } }, + select: { Id: true } + }); + + await tx.users.update({ + where: { Id: UserId }, + data: { + Groups: { disconnect: groups }, + UserRoles: { deleteMany: { OrganizationId } } + } + }); + } + + return ( + await tx.projects.findMany({ + where: { OrganizationId }, + select: { Id: true } + }) + ).map((p) => p.Id); + }); + + await getQueues().SvelteSSE.add( `Update Projects for Org #${OrganizationId} (user #${UserId} ${enabled ? 'added' : 'removed'})`, { type: BullMQ.JobType.SvelteSSE_UpdateProjectGroups, - projectIds: ( - await prisma.projects.findMany({ - where: { OrganizationId }, - select: { Id: true } - }) - ).map((p) => p.Id) + projectIds } ); - } - - return updated; + return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/database/Organizations.ts` around lines 50 - 97, The membership change and cleanup must be made atomic: wrap the logic in a prisma.$transaction so the Organizations connect/disconnect and the Groups disconnect plus UserRoles deleteMany occur in one transactional update. Specifically, when handling the removal case (enabled === false) fetch the group Ids with prisma.groups.findMany, then in the same transaction call prisma.users.update with data containing Organizations: { disconnect: { Id: OrganizationId } }, Groups: { disconnect: [ ...groupIdObjects ] }, and UserRoles: { deleteMany: { OrganizationId } } (or include the connect for add case in the same update), ensuring all updates to users.update (the Organizations change, Groups.disconnect, and UserRoles.deleteMany) happen inside one prisma.$transaction so either all succeed or none do.src/routes/(authenticated)/tasks/[product_id]/+page.svelte (1)
61-79:⚠️ Potential issue | 🟠 MajorHandle the “task disappeared while waiting” path.
The waiting branch only resolves when
productTasks.lengthis still non-zero. If the action completes by removing the task, this effect leaveswaitingstuck forever; when it was also the last task, the outertasks.lengthguard prevents the effect from running at all.🧭 Suggested fix
$effect(() => { - if ($userTasksSSE?.tasks.length) { - const productTasks = $userTasksSSE.tasks.filter((t) => t.P === page.params.product_id); + const tasks = $userTasksSSE?.tasks; + if (!tasks) return; + + const productTasks = tasks.filter((t) => t.P === page.params.product_id); const fallback = new Date().valueOf(); - const oldTask = productTasks.find((t) => (t.U?.valueOf() ?? fallback) <= data.loadTime); + const oldTask = productTasks.find((t) => (t.U?.valueOf() ?? fallback) <= data.loadTime); const waitRead = untrack(() => waiting); - // waiting and task updated - if (waitRead && productTasks.length && !oldTask) { - invalidate('task:id:load'); - waiting = false; - } + // waiting and task updated or removed + if (waitRead && !oldTask) { + if (productTasks.length) { + invalidate('task:id:load'); + waiting = false; + } else { + history.back(); + } + } // not waiting but task updated/deleted (by some other user) else if (!(waitRead || oldTask)) { if (productTasks.length) { invalidate('task:id:load'); } else { history.back(); } toast('warning', m.tasks_reloaded()); } - } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(authenticated)/tasks/[product_id]/+page.svelte around lines 61 - 79, The effect currently only clears waiting when productTasks.length is non-zero, so if the task was removed while the user was waiting waiting can remain true forever; update the logic around $userTasksSSE/products by adding an explicit branch that handles the "waiting and productTasks.length === 0" case: when waitRead (untrack waiting) is true and productTasks.length is 0, set waiting = false and perform the appropriate follow-up (e.g., history.back() and/or call toast('warning', m.tasks_reloaded()) or invalidate('task:id:load') as fits the UX), leaving the existing branches for the other cases unchanged so waiting is never left stuck.
🧹 Nitpick comments (9)
src/routes/(authenticated)/admin/settings/stores/edit/+page.server.ts (1)
30-32: Naming:userCountcounts organizations, not users.The variable name
userCountis misleading. The query counts organizations that have projects with products linked to this store:await DatabaseReads.organizations.count({ where: { Projects: { some: { Products: { some: { StoreId: id } } } } } });Consider renaming to
orgCountororganizationCountfor clarity and consistency with what's being counted.♻️ Suggested rename
- const userCount = await DatabaseReads.organizations.count({ + const orgCount = await DatabaseReads.organizations.count({ where: { Projects: { some: { Products: { some: { StoreId: id } } } } } });And update the return:
options: await DatabaseReads.storeTypes.findMany(), - userCount + orgCount };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(authenticated)/admin/settings/stores/edit/+page.server.ts around lines 30 - 32, Rename the misleading variable userCount to organizationCount (or orgCount) where it's assigned from DatabaseReads.organizations.count and update any subsequent references and the returned object to use the new name; locate the assignment using DatabaseReads.organizations.count({ where: { Projects: { some: { Products: { some: { StoreId: id } } } } } }) and change both the declaration and the field in the returned data so the name accurately reflects that organizations are being counted.src/routes/(authenticated)/admin/settings/stores/+page.server.ts (1)
8-48: Consider combining the two store queries for efficiency.Two separate
findManyqueries fetch stores data:
- Lines 8-13: Stores with
StoreTypeandOwner- Lines 19-39: Stores with
OrganizationsandProductsThese could potentially be combined into a single query with all necessary includes, reducing database round trips.
♻️ Potential combined query approach
export const load = (async (event) => { event.locals.security.requireSuperAdmin(); const stores = await DatabaseReads.stores.findMany({ include: { StoreType: true, - Owner: { select: { Name: true } } + Owner: { select: { Name: true } }, + Organizations: { + select: { + Id: true, + Name: true + } + }, + Products: { + select: { + Project: { + select: { + OrganizationId: true + } + } + }, + distinct: 'ProjectId' + } } }); return { stores, - users: new Map( - ( - await DatabaseReads.stores.findMany({ - select: { - Id: true, - Organizations: { - select: { - Id: true, - Name: true - } - }, - Products: { - select: { - Project: { - select: { - OrganizationId: true - } - } - }, - distinct: 'ProjectId' - } - } - }) - ).map((s) => [ - s.Id, - s.Organizations.map((o) => ({ - Name: o.Name, - Products: s.Products.filter((p) => p.Project.OrganizationId === o.Id).length - })).filter((o) => !!o.Products) - ]) - ) + users: new Map( + stores.map((s) => [ + s.Id, + s.Organizations.map((o) => ({ + Name: o.Name, + Products: s.Products.filter((p) => p.Project.OrganizationId === o.Id).length + })).filter((o) => !!o.Products) + ]) + ) }; }) satisfies PageServerLoad;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(authenticated)/admin/settings/stores/+page.server.ts around lines 8 - 48, The two separate DatabaseReads.stores.findMany calls should be merged into a single query to avoid duplicate round-trips: fetch stores once with include: StoreType, Owner (select Name), Organizations (select Id, Name) and Products (select Project -> OrganizationId) with distinct: 'ProjectId'; assign that result to the existing stores variable and then build the users Map from that same stores array (map each store by Id and compute Organizations[].Products counts by filtering s.Products where p.Project.OrganizationId === o.Id), removing the second DatabaseReads.stores.findMany entirely.src/lib/server/database/Users.ts (1)
115-126: Duplicate database query for project IDs.Lines 120-123 execute the same query as lines 80-83 (
prisma.projects.findMany({ where: { OrganizationId }, select: { Id: true } })). Consider storing the result from the first query and reusing it for the SSE update to avoid the extra database round-trip.♻️ Proposed fix to reuse query result
if (!(enabled && existing)) { + const projectIds = ( + await prisma.projects.findMany({ + where: { OrganizationId }, + select: { Id: true } + }) + ).map((p) => p.Id); + /* * Only enqueue tasks when: * 1. The role is OrgAdmin */ if (role === RoleId.OrgAdmin) { await getQueues().UserTasks.addBulk( - ( - await prisma.projects.findMany({ - where: { OrganizationId }, - select: { Id: true } - }) - ).flatMap((p) => [ + projectIds.flatMap((id) => [ { - name: `${enabled ? 'Add' : 'Remove'} OrgAdmin tasks for User #${UserId} on Project #${p.Id}`, + name: `${enabled ? 'Add' : 'Remove'} OrgAdmin tasks for User #${UserId} on Project #${id}`, data: { type: BullMQ.JobType.UserTasks_Workflow, scope: 'Project', - projectId: p.Id, + projectId: id, // ... rest unchanged } }, { - name: `${enabled ? 'Add' : 'Remove'} OrgAdmin data deletion requests for User #${UserId} on Project #${p.Id}`, + name: `${enabled ? 'Add' : 'Remove'} OrgAdmin data deletion requests for User #${UserId} on Project #${id}`, data: { type: BullMQ.JobType.UserTasks_DeleteRequest, scope: 'Project', - projectId: p.Id, + projectId: id, // ... rest unchanged } } ]) ); } await getQueues().SvelteSSE.add( `Update Projects for Org #${OrganizationId} (role #${role} user #${UserId} ${enabled ? 'added' : 'removed'})`, { type: BullMQ.JobType.SvelteSSE_UpdateProjectGroups, - projectIds: ( - await prisma.projects.findMany({ - where: { OrganizationId }, - select: { Id: true } - }) - ).map((p) => p.Id) + projectIds } ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/database/Users.ts` around lines 115 - 126, The code calls prisma.projects.findMany twice to get project IDs; capture the result of the first call (the findMany that selects { Id }) into a local variable (e.g., projectRows or projectIdsRows) when it is first executed and reuse its .map(p => p.Id) when calling getQueues().SvelteSSE.add, replacing the second prisma.projects.findMany; update the getQueues().SvelteSSE.add call (which currently passes projectIds: (await prisma.projects.findMany(...)).map(...)) to reference the previously stored result so you avoid the duplicate DB query.src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts (1)
60-63: Parallelize the new page-data fetches.These four reads are awaited in series, so the page now pays the sum of all query latencies before it can render. That is a measurable regression on the exact path this PR is trying to speed up.
⚡ Suggested refactor
- return { - projectData: await getProjectDetails(projectId, locals.security.sessionForm), - groupData: await getProjectGroupData(projectId, locals.security.sessionForm), - orgData: await getProjectOrgData(projectId, locals.security.sessionForm), - productData: await getProjectProducts(projectId, locals.security.sessionForm), + const [projectData, groupData, orgData, productData] = await Promise.all([ + getProjectDetails(projectId, locals.security.sessionForm), + getProjectGroupData(projectId, locals.security.sessionForm), + getProjectOrgData(projectId, locals.security.sessionForm), + getProjectProducts(projectId, locals.security.sessionForm) + ]); + + return { + projectData, + groupData, + orgData, + productData,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(authenticated)/projects/[id=idNumber]/+page.server.ts around lines 60 - 63, The four sequential awaits (getProjectDetails, getProjectGroupData, getProjectOrgData, getProjectProducts) cause unnecessary latency; run them in parallel using Promise.all (or Promise.allSettled if you need per-call fault tolerance) and then assign the results to projectData, groupData, orgData, and productData respectively, ensuring you still pass locals.security.sessionForm to each call and handle errors/results after the Promise resolution.src/lib/server/database/ProductDefinitions.ts (1)
61-74: Consider optimizing the disconnect query.The current approach fetches all application types from the database to compute which ones to disconnect. For large numbers of application types, this could be inefficient.
♻️ Proposed optimization
Fetch only the currently connected application types instead of all types:
data: ApplicationTypes ? { ...data, ApplicationTypes: { connect: ApplicationTypes.map((n) => ({ Id: n })), - disconnect: (await prisma.applicationTypes.findMany({ select: { Id: true } })).filter( - (at) => !ApplicationTypes.includes(at.Id) - ) + disconnect: ( + await prisma.productDefinitions.findUnique({ + where: { Id: id }, + select: { ApplicationTypes: { select: { Id: true } } } + }) + )?.ApplicationTypes.filter((at) => !ApplicationTypes.includes(at.Id)) ?? [] } } : data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/database/ProductDefinitions.ts` around lines 61 - 74, The update currently calls prisma.applicationTypes.findMany to get all ApplicationType Ids to compute disconnects, which is inefficient; change the logic in the prisma.productDefinitions.update block to first query only the currently connected ApplicationTypes for the given Id (use prisma.productDefinitions.findUnique or findFirst selecting ApplicationTypes.Id for the record identified by id), compute the disconnect list by comparing those connected Ids to the incoming ApplicationTypes array, then pass connect with ApplicationTypes.map(...) and disconnect with the computed subset. Update references around prisma.productDefinitions.update, ApplicationTypes, id and data so the disconnect uses the fetched connected IDs instead of fetching all applicationTypes.src/routes/(authenticated)/projects/[id=idNumber]/+page.svelte (2)
259-264: Consider using a Map for Store lookups.The
orgData.Stores.find()call is O(n) per product. For pages with many products, creating a store map would be more efficient.♻️ Proposed optimization
const { productMap, availableProducts } = $derived.by(() => { const activeProducts = new Set(productData.products.map((p) => p.PD)); return { productMap: new Map(orgData.ProductDefinitions.map((pd) => [pd.Id, pd])), + storeMap: new Map(orgData.Stores.map((s) => [s.Id, s])), availableProducts: orgData.ProductDefinitions.filter( (pd) => !activeProducts.has(pd.Id) && pd._count.Organizations ) }; });Then use
storeMap.get(p.S)!instead oforgData.Stores.find((s) => s.Id === p.S)!.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(authenticated)/projects/[id=idNumber]/+page.svelte around lines 259 - 264, The product mapping is doing O(n) Store lookups via orgData.Stores.find for each product; create a Map from orgData.Stores keyed by Id (e.g., const storeMap = new Map(orgData.Stores.map(s => [s.Id, s]))) before you build products, then in the productData.products.map replace orgData.Stores.find((s) => s.Id === p.S)! with storeMap.get(p.S)! (or handle missing values) so productMap and the products.toSorted call use O(1) store lookups.
94-98: Verify all required data streams are available before rendering.The guard only checks
projectData && groupData, but the rendering also depends onorgDataandproductData(e.g., lines 249, 256, 259). Consider extending the guard:♻️ Proposed fix
- {`#if` !(projectData && groupData)} + {`#if` !(projectData && groupData && orgData && productData)} <div class="flex justify-center items-center h-64"> <span class="loading loading-spinner loading-lg"></span> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(authenticated)/projects/[id=idNumber]/+page.svelte around lines 94 - 98, The current render guard only checks projectData and groupData but the page also uses orgData and productData; update the initial conditional that shows the loading spinner (the {`#if` ...} check) to require all four data streams (projectData, groupData, orgData, productData) before rendering the main content so downstream references (projectData, groupData, orgData, productData) are always defined when used.src/lib/projects/sse.ts (2)
218-273: Parallelize the org-scoped fetches.
StoresandProductDefinitionsare independent onceprojectis loaded, but the current object literal awaits them serially. Running them together trims response time without changing the result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/projects/sse.ts` around lines 218 - 273, The two independent DB calls (DatabaseReads.stores.findMany and DatabaseReads.productDefinitions.findMany) are awaited serially; change to run them in parallel by creating both promises (calling the two findManys without awaiting) and then await Promise.all to destructure the results into Stores and ProductDefinitions (use the existing project variable and same where/select shapes), replacing the current sequential awaits so both queries execute concurrently.
119-185: Run the independent lookups concurrently.These four awaits execute left-to-right, so this SSE path waits for each database round-trip in sequence.
Promise.allwould keep the same payload while removing avoidable latency from the path this PR is optimizing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/projects/sse.ts` around lines 119 - 185, The four independent DB lookups (possibleOwners, possibleGroups, possibleAuthors, and userGroups) are awaited sequentially; refactor to run them concurrently using Promise.all: start the three DatabaseReads.findMany calls and the userGroupsForOrg(userSession.userId, project.OrganizationId) call without awaiting, then await Promise.all and assign results back to the corresponding returned fields (possibleOwners, possibleGroups, possibleAuthors, userGroups) while keeping the same select/mapper logic (keep DatabaseReads.users.findMany, DatabaseReads.groups.findMany, and userGroupsForOrg references intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/projects/sse.ts`:
- Around line 522-548: The initial emit occurs before registering the subscriber
so updates can be missed and thrown errors inside updateCb can skip cleanup; fix
by registering SSEPageUpdates.on(stream, updateCb) first, then route the initial
snapshot through the same path (e.g., call updateCb([id]) or a shared helper) so
the initial send uses the same error/cleanup logic, and wrap updateCb body in
try/catch that on any thrown error calls SSEPageUpdates.off(stream, updateCb)
and clearInterval(pingInterval) before rethrowing or returning; ensure
emit/query/ stringify and the pingInterval cleanup follow the shared flow so
off() and clearInterval() always run on failure.
- Around line 339-363: minifyProductDetails currently assumes fields
BuildEngineJobId, CurrentBuildId, CurrentReleaseId, ProductBuilds, and
ProductPublications always exist, but the query conditionally omits them when
isSuper is false; update minifyProductDetails to guard access: check for
existence of ProductBuilds and ProductPublications before calling .map(), and
provide safe defaults for BuildEngineJobId/CurrentBuildId/CurrentReleaseId
(e.g., null or undefined-handling) so the function works whether those keys are
present or omitted; reference the minifyProductDetails function and the
ProductBuilds/ProductPublications/BuildEngineJobId/CurrentBuildId/CurrentReleaseId
symbols when making these defensive checks.
- Around line 332-337: The UserTasks relation selection is missing an orderBy,
causing non-deterministic ordering; update the query that selects UserTasks (the
block with select: { DateCreated: true, UserId: true }) to include an explicit
orderBy (e.g., orderBy: { DateCreated: 'asc' }) so product.UT.slice(-1)[0]
consistently refers to the same item; ensure you pick asc or desc to match the
intended semantics (asc makes the last element the most recent).
In `@src/lib/server/database/ProductTransitions.ts`:
- Around line 111-134: The conditional branches in ProductTransitions.ts now
call tx.productBuilds.updateManyAndReturn and
tx.productPublications.updateManyAndReturn which return arrays of selected rows,
causing a type mismatch with callers of tryConnect (used in system.ts) that
expect the { count: number } shape; revert these two calls to use updateMany
(tx.productBuilds.updateMany and tx.productPublications.updateMany) so they
return the { count } result or, if you need the selected values, wrap/normalize
the updateManyAndReturn result into an object matching the original contract
(e.g., { count: result.length } or transform to the expected structure) so
tryConnect retains a consistent return type used by system.ts.
In `@src/lib/server/database/Projects.ts`:
- Around line 94-106: The SSE enqueue calls (getQueues().SvelteSSE.add with
BullMQ.JobType.SvelteSSE_UpdateProjectGroups and SvelteSSE_UpdateProject) must
not cause the function to return false after a successful DB write; make these
notifications fire-and-forget or handle their errors separately so DB success
isn't masked—either move both getQueues().SvelteSSE.add calls outside the
existing try-catch that surrounds the DB update, or wrap each add in its own
try/catch (or use .catch()) to log errors without altering the function’s return
value; reference the groupId check, existing?.GroupId, id, and the two JobType
constants when locating the lines to change.
In `@src/routes/`(authenticated)/admin/settings/stores/edit/+page.svelte:
- Line 63: The prop/field named userCount is misleading because it actually
counts organizations; rename it to organizationCount (or orgCount) across the
data contract and UI: update the server-side response that populates the data
object (where userCount is set from the query) to use organizationCount, adjust
any types/interfaces, and update all client-side usages (e.g., the conditional
attr in +page.svelte that currently uses data.userCount) to reference
data.organizationCount instead; ensure both server and client names match so the
condition "$form.owner === null && data.organizationCount > 1" works correctly.
In `@src/routes/`(authenticated)/tasks/+page.svelte:
- Around line 38-40: The table rows currently use onclick={() =>
goto(localizeHref(`/tasks/${task.P}`))} and are not keyboard-accessible; replace
the interactive behavior with a real anchor or make the cell content an <a
href="{localizeHref(`/tasks/${task.P}`)}"> link so keyboard users can focus and
activate it (or, if you must keep the row clickable, add tabindex="0" and handle
Enter/Space key events to call goto plus ARIA role="button"); update the same
pattern found around the other task rows/cells (the blocks using goto and
localizeHref at the other noted locations) to ensure consistent focusability and
keyboard activation.
- Around line 96-120: The row's onclick handler (onclick={() =>
goto(localizeHref(`/tasks/${task.P}`))}) on the <tr> is being triggered when
clicking the project <a> link; update the project link element (the <a> that
renders userTasks.projects.get(task.Pj)) to stop event propagation so the row
click doesn't fire — e.g., add a click handler that calls e.stopPropagation (or
use Svelte's on:click|stopPropagation) on that <a> so the link navigation only
triggers the anchor and not the parent tr's goto.
In `@src/routes/`(authenticated)/workflow-instances/[product_id]/+page.server.ts:
- Around line 84-129: The second DB lookup using
DatabaseReads.products.findUniqueOrThrow for minifyProductDetails risks a race
if the earlier product check (variable product validated at line ~59) changes;
instead reuse the already-fetched product or perform a single query that selects
all needed fields for minifyProductDetails and getURLandToken, or change the
second call to DatabaseReads.products.findUnique and explicitly null-check
before calling minifyProductDetails; update references to product,
minifyProductDetails, and getURLandToken(product.Project.Organization)
accordingly so no unhandled exception can occur if the record was removed
between queries.
---
Outside diff comments:
In `@src/lib/server/database/Organizations.ts`:
- Around line 50-97: The membership change and cleanup must be made atomic: wrap
the logic in a prisma.$transaction so the Organizations connect/disconnect and
the Groups disconnect plus UserRoles deleteMany occur in one transactional
update. Specifically, when handling the removal case (enabled === false) fetch
the group Ids with prisma.groups.findMany, then in the same transaction call
prisma.users.update with data containing Organizations: { disconnect: { Id:
OrganizationId } }, Groups: { disconnect: [ ...groupIdObjects ] }, and
UserRoles: { deleteMany: { OrganizationId } } (or include the connect for add
case in the same update), ensuring all updates to users.update (the
Organizations change, Groups.disconnect, and UserRoles.deleteMany) happen inside
one prisma.$transaction so either all succeed or none do.
In `@src/routes/`(authenticated)/tasks/[product_id]/+page.svelte:
- Around line 61-79: The effect currently only clears waiting when
productTasks.length is non-zero, so if the task was removed while the user was
waiting waiting can remain true forever; update the logic around
$userTasksSSE/products by adding an explicit branch that handles the "waiting
and productTasks.length === 0" case: when waitRead (untrack waiting) is true and
productTasks.length is 0, set waiting = false and perform the appropriate
follow-up (e.g., history.back() and/or call toast('warning', m.tasks_reloaded())
or invalidate('task:id:load') as fits the UX), leaving the existing branches for
the other cases unchanged so waiting is never left stuck.
---
Nitpick comments:
In `@src/lib/projects/sse.ts`:
- Around line 218-273: The two independent DB calls
(DatabaseReads.stores.findMany and DatabaseReads.productDefinitions.findMany)
are awaited serially; change to run them in parallel by creating both promises
(calling the two findManys without awaiting) and then await Promise.all to
destructure the results into Stores and ProductDefinitions (use the existing
project variable and same where/select shapes), replacing the current sequential
awaits so both queries execute concurrently.
- Around line 119-185: The four independent DB lookups (possibleOwners,
possibleGroups, possibleAuthors, and userGroups) are awaited sequentially;
refactor to run them concurrently using Promise.all: start the three
DatabaseReads.findMany calls and the userGroupsForOrg(userSession.userId,
project.OrganizationId) call without awaiting, then await Promise.all and assign
results back to the corresponding returned fields (possibleOwners,
possibleGroups, possibleAuthors, userGroups) while keeping the same
select/mapper logic (keep DatabaseReads.users.findMany,
DatabaseReads.groups.findMany, and userGroupsForOrg references intact).
In `@src/lib/server/database/ProductDefinitions.ts`:
- Around line 61-74: The update currently calls prisma.applicationTypes.findMany
to get all ApplicationType Ids to compute disconnects, which is inefficient;
change the logic in the prisma.productDefinitions.update block to first query
only the currently connected ApplicationTypes for the given Id (use
prisma.productDefinitions.findUnique or findFirst selecting ApplicationTypes.Id
for the record identified by id), compute the disconnect list by comparing those
connected Ids to the incoming ApplicationTypes array, then pass connect with
ApplicationTypes.map(...) and disconnect with the computed subset. Update
references around prisma.productDefinitions.update, ApplicationTypes, id and
data so the disconnect uses the fetched connected IDs instead of fetching all
applicationTypes.
In `@src/lib/server/database/Users.ts`:
- Around line 115-126: The code calls prisma.projects.findMany twice to get
project IDs; capture the result of the first call (the findMany that selects {
Id }) into a local variable (e.g., projectRows or projectIdsRows) when it is
first executed and reuse its .map(p => p.Id) when calling
getQueues().SvelteSSE.add, replacing the second prisma.projects.findMany; update
the getQueues().SvelteSSE.add call (which currently passes projectIds: (await
prisma.projects.findMany(...)).map(...)) to reference the previously stored
result so you avoid the duplicate DB query.
In `@src/routes/`(authenticated)/admin/settings/stores/+page.server.ts:
- Around line 8-48: The two separate DatabaseReads.stores.findMany calls should
be merged into a single query to avoid duplicate round-trips: fetch stores once
with include: StoreType, Owner (select Name), Organizations (select Id, Name)
and Products (select Project -> OrganizationId) with distinct: 'ProjectId';
assign that result to the existing stores variable and then build the users Map
from that same stores array (map each store by Id and compute
Organizations[].Products counts by filtering s.Products where
p.Project.OrganizationId === o.Id), removing the second
DatabaseReads.stores.findMany entirely.
In `@src/routes/`(authenticated)/admin/settings/stores/edit/+page.server.ts:
- Around line 30-32: Rename the misleading variable userCount to
organizationCount (or orgCount) where it's assigned from
DatabaseReads.organizations.count and update any subsequent references and the
returned object to use the new name; locate the assignment using
DatabaseReads.organizations.count({ where: { Projects: { some: { Products: {
some: { StoreId: id } } } } } }) and change both the declaration and the field
in the returned data so the name accurately reflects that organizations are
being counted.
In `@src/routes/`(authenticated)/projects/[id=idNumber]/+page.server.ts:
- Around line 60-63: The four sequential awaits (getProjectDetails,
getProjectGroupData, getProjectOrgData, getProjectProducts) cause unnecessary
latency; run them in parallel using Promise.all (or Promise.allSettled if you
need per-call fault tolerance) and then assign the results to projectData,
groupData, orgData, and productData respectively, ensuring you still pass
locals.security.sessionForm to each call and handle errors/results after the
Promise resolution.
In `@src/routes/`(authenticated)/projects/[id=idNumber]/+page.svelte:
- Around line 259-264: The product mapping is doing O(n) Store lookups via
orgData.Stores.find for each product; create a Map from orgData.Stores keyed by
Id (e.g., const storeMap = new Map(orgData.Stores.map(s => [s.Id, s]))) before
you build products, then in the productData.products.map replace
orgData.Stores.find((s) => s.Id === p.S)! with storeMap.get(p.S)! (or handle
missing values) so productMap and the products.toSorted call use O(1) store
lookups.
- Around line 94-98: The current render guard only checks projectData and
groupData but the page also uses orgData and productData; update the initial
conditional that shows the loading spinner (the {`#if` ...} check) to require all
four data streams (projectData, groupData, orgData, productData) before
rendering the main content so downstream references (projectData, groupData,
orgData, productData) are always defined when used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7bde78a8-211f-416f-bd6c-b8c1db8581dc
📒 Files selected for processing (51)
src/lib/bullmq.tssrc/lib/icons/index.tssrc/lib/organizations/components/StoreListDisplay.sveltesrc/lib/products/components/BuildArtifacts.sveltesrc/lib/products/components/ProductDetails.sveltesrc/lib/products/components/ReleaseInfo.sveltesrc/lib/products/index.tssrc/lib/projects/listener.tssrc/lib/projects/sse.tssrc/lib/server/bullmq/BullWorker.tssrc/lib/server/bullmq/types.tssrc/lib/server/database/Authors.tssrc/lib/server/database/Organizations.tssrc/lib/server/database/ProductDefinitions.tssrc/lib/server/database/ProductTransitions.tssrc/lib/server/database/Products.tssrc/lib/server/database/Projects.tssrc/lib/server/database/Reviewers.tssrc/lib/server/database/Stores.tssrc/lib/server/database/UserTasks.tssrc/lib/server/database/Users.tssrc/lib/server/job-executors/polling.tssrc/lib/server/job-executors/system.tssrc/lib/utils/sorting.tssrc/lib/workflowTypes.tssrc/routes/(authenticated)/+layout.sveltesrc/routes/(authenticated)/admin/settings/organizations/edit/+page.sveltesrc/routes/(authenticated)/admin/settings/product-definitions/edit/+page.server.tssrc/routes/(authenticated)/admin/settings/product-definitions/new/+page.server.tssrc/routes/(authenticated)/admin/settings/stores/+page.server.tssrc/routes/(authenticated)/admin/settings/stores/+page.sveltesrc/routes/(authenticated)/admin/settings/stores/edit/+page.server.tssrc/routes/(authenticated)/admin/settings/stores/edit/+page.sveltesrc/routes/(authenticated)/organizations/[id=idNumber]/settings/products/+page.server.tssrc/routes/(authenticated)/organizations/[id=idNumber]/settings/products/+page.sveltesrc/routes/(authenticated)/organizations/[id=idNumber]/settings/stores/+page.server.tssrc/routes/(authenticated)/organizations/[id=idNumber]/settings/stores/+page.sveltesrc/routes/(authenticated)/organizations/[id=idNumber]/settings/stores/edit/+page.sveltesrc/routes/(authenticated)/projects/[id=idNumber]/+page.server.tssrc/routes/(authenticated)/projects/[id=idNumber]/+page.sveltesrc/routes/(authenticated)/projects/[id=idNumber]/ProductCard.sveltesrc/routes/(authenticated)/projects/[id=idNumber]/modals/DeleteProduct.sveltesrc/routes/(authenticated)/projects/[id=idNumber]/modals/Properties.sveltesrc/routes/(authenticated)/projects/[id=idNumber]/sse/+server.tssrc/routes/(authenticated)/projects/[id=idNumber]/sse/groups/+server.tssrc/routes/(authenticated)/projects/[id=idNumber]/sse/org/+server.tssrc/routes/(authenticated)/projects/[id=idNumber]/sse/products/+server.tssrc/routes/(authenticated)/tasks/+page.sveltesrc/routes/(authenticated)/tasks/[product_id]/+page.sveltesrc/routes/(authenticated)/workflow-instances/[product_id]/+page.server.tssrc/routes/(authenticated)/workflow-instances/[product_id]/+page.svelte
src/routes/(authenticated)/admin/settings/stores/edit/+page.svelte
Outdated
Show resolved
Hide resolved
src/routes/(authenticated)/workflow-instances/[product_id]/+page.server.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/routes/(authenticated)/workflow-instances/[product_id]/+page.server.ts (1)
84-129:⚠️ Potential issue | 🟡 MinorAvoid a second product lookup here.
Line 19 already established that the product exists. If it disappears before Line 85,
findUniqueOrThrow()turns that into an unhandled 500 instead of the intended 404. Reusing the first query result, or making the second lookup nullable, avoids that edge-case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(authenticated)/workflow-instances/[product_id]/+page.server.ts around lines 84 - 129, The second call to DatabaseReads.products.findUniqueOrThrow(...) before calling minifyProductDetails can throw a 500 if the product vanished; instead reuse the earlier fetched product object (the variable used around line 19) when calling minifyProductDetails, or change this lookup to DatabaseReads.products.findUnique(...) and handle a null by returning the intended 404; update the call site that currently passes the findUniqueOrThrow result into minifyProductDetails (and the use of product.Project.Organization for getURLandToken) to use the reused product variable (or the nullable result) so you avoid a redundant throw.src/lib/projects/sse.ts (1)
522-546:⚠️ Potential issue | 🟠 MajorSubscribe before sending the initial snapshot.
The first
emit()happens beforeSSEPageUpdates.on()is registered. Any update job that lands in that gap is dropped, so a newly opened page can miss its first refresh and stay stale until another event arrives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/projects/sse.ts` around lines 522 - 546, The initial snapshot is emitted before registering the update listener, which can drop updates that occur in the gap; fix by registering the update callback with SSEPageUpdates.on(stream, updateCb) before calling emit(event, stringify(await query(id, userSession))), and ensure the same error/cleanup logic currently inside updateCb (SSEPageUpdates.off(stream, updateCb) and clearInterval(pingInterval)) is applied if the initial emit fails (check the returned error and remove the listener/clear interval as needed); keep using the existing functions updateCb, query, stringify, emit, SSEPageUpdates.on/off, stream, and pingInterval so the lifecycle and cleanup remain consistent.
🧹 Nitpick comments (4)
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts (1)
59-69: Consider parallelizing the four data fetches.The load function now makes four sequential async calls (
getProjectDetails,getProjectGroupData,getProjectOrgData,getProjectProducts). Since these are independent queries, they could be parallelized usingPromise.allto reduce total load time.♻️ Optional: Parallelize data fetching
return { - projectData: await getProjectDetails(projectId, locals.security.sessionForm), - groupData: await getProjectGroupData(projectId, locals.security.sessionForm), - orgData: await getProjectOrgData(projectId, locals.security.sessionForm), - productData: await getProjectProducts(projectId, locals.security.sessionForm), + ...(await (async () => { + const [projectData, groupData, orgData, productData] = await Promise.all([ + getProjectDetails(projectId, locals.security.sessionForm), + getProjectGroupData(projectId, locals.security.sessionForm), + getProjectOrgData(projectId, locals.security.sessionForm), + getProjectProducts(projectId, locals.security.sessionForm) + ]); + return { projectData, groupData, orgData, productData }; + })()), authorForm: await superValidate(valibot(addAuthorSchema)),Or more readably:
const [projectData, groupData, orgData, productData] = await Promise.all([ getProjectDetails(projectId, locals.security.sessionForm), getProjectGroupData(projectId, locals.security.sessionForm), getProjectOrgData(projectId, locals.security.sessionForm), getProjectProducts(projectId, locals.security.sessionForm) ]); return { projectData, groupData, orgData, productData, authorForm: await superValidate(valibot(addAuthorSchema)), // ... };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(authenticated)/projects/[id=idNumber]/+page.server.ts around lines 59 - 69, The four independent fetches (getProjectDetails, getProjectGroupData, getProjectOrgData, getProjectProducts) are executed sequentially; wrap them in a single Promise.all to run them in parallel and then destructure the results into projectData, groupData, orgData, productData before returning; keep the remaining await calls (superValidate for addAuthorSchema, addReviewerSchema, projectActionSchema) unchanged and preserve jobsAvailable and showRebuildToggles in the returned object.src/lib/server/database/Users.ts (1)
78-126: Duplicate Prisma query for projects within the same code path.When
role === RoleId.OrgAdmin, the code queriesprisma.projects.findMany({ where: { OrganizationId } })twice: once at lines 80-83 for task creation and again at lines 120-123 for the SSE notification. These queries return the same data.Consider extracting the project IDs once and reusing:
♻️ Proposed refactor to eliminate duplicate query
if (!(enabled && existing)) { + const orgProjectIds = ( + await prisma.projects.findMany({ + where: { OrganizationId }, + select: { Id: true } + }) + ).map((p) => p.Id); + /* * Only enqueue tasks when: * 1. The role is OrgAdmin */ if (role === RoleId.OrgAdmin) { await getQueues().UserTasks.addBulk( - ( - await prisma.projects.findMany({ - where: { OrganizationId }, - select: { Id: true } - }) - ).flatMap((p) => [ + orgProjectIds.flatMap((pId) => [ { - name: `${enabled ? 'Add' : 'Remove'} OrgAdmin tasks for User #${UserId} on Project #${p.Id}`, + name: `${enabled ? 'Add' : 'Remove'} OrgAdmin tasks for User #${UserId} on Project #${pId}`, data: { type: BullMQ.JobType.UserTasks_Workflow, scope: 'Project', - projectId: p.Id, + projectId: pId, // ... rest unchanged } }, { - name: `${enabled ? 'Add' : 'Remove'} OrgAdmin data deletion requests for User #${UserId} on Project #${p.Id}`, + name: `${enabled ? 'Add' : 'Remove'} OrgAdmin data deletion requests for User #${UserId} on Project #${pId}`, data: { type: BullMQ.JobType.UserTasks_DeleteRequest, scope: 'Project', - projectId: p.Id, + projectId: pId, // ... rest unchanged } } ]) ); } getQueues().SvelteSSE.add( `Update Projects for Org #${OrganizationId} (role #${role} user #${UserId} ${enabled ? 'added' : 'removed'})`, { type: BullMQ.JobType.SvelteSSE_UpdateProjectGroups, - projectIds: ( - await prisma.projects.findMany({ - where: { OrganizationId }, - select: { Id: true } - }) - ).map((p) => p.Id) + projectIds: orgProjectIds } ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/database/Users.ts` around lines 78 - 126, The block handling RoleId.OrgAdmin runs prisma.projects.findMany({ where: { OrganizationId } }) twice (once inside getQueues().UserTasks.addBulk and again for getQueues().SvelteSSE.add); change this by calling prisma.projects.findMany(...) once, store the result (or just the project Ids) in a local variable (e.g., projectRecords or projectIds), and reuse that variable in the UserTasks.addBulk flatMap and in SvelteSSE.add; update references inside the created job objects to use the cached project id values and keep existing variables like OrganizationId, UserId, enabled, role, and RoleId.OrgAdmin unchanged.src/routes/(authenticated)/projects/[id=idNumber]/+page.svelte (1)
82-90: Build a store lookup once instead of scanning for every product.
productMapavoids repeated product-definition lookups, but each render still doesorgData.Stores.find(...)per product. Since this page now recomputes on SSE updates, adding astoreMapin the same$derived.by()block keeps the join linear.♻️ Suggested refactor
- const { productMap, availableProducts } = $derived.by(() => { + const { productMap, storeMap, availableProducts } = $derived.by(() => { const activeProducts = new Set(productData.products.map((p) => p.PD)); return { productMap: new Map(orgData.ProductDefinitions.map((pd) => [pd.Id, pd])), + storeMap: new Map(orgData.Stores.map((s) => [s.Id, s])), availableProducts: orgData.ProductDefinitions.filter( (pd) => !activeProducts.has(pd.Id) && pd._count.Organizations ) }; }); @@ {`@const` products = productData.products.map((p) => ({ ...p, ProductDefinition: productMap.get(p.PD)!, - Store: orgData.Stores.find((s) => s.Id === p.S)! + Store: storeMap.get(p.S)! }))}Also applies to: 259-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(authenticated)/projects/[id=idNumber]/+page.svelte around lines 82 - 90, The code repeatedly calls orgData.Stores.find(...) per product causing O(n*m) work on each SSE recompute; add a storeMap in the same $derived.by() block (alongside productMap) that builds a Map from orgData.Stores by Id, then replace all calls to orgData.Stores.find(...) in this component (and the other affected lines around 259-263) with lookups into storeMap.get(storeId) so product rendering/join becomes linear; update references to use storeMap where the component currently expects a store from orgData.Stores.find.src/lib/products/components/ProductDetails.svelte (1)
55-72: Precompute the build/release lookup once.
getBuildOrPubis called while rendering transitions, and each call linearly scansproduct.PB/product.PR. On products with longer histories this turns into repeated nested work for the modal.Possible refactor
+ const buildByTransition = $derived(new Map((product.PB ?? []).map((pb) => [pb.T, pb.I]))); + const releaseByTransition = $derived(new Map((product.PR ?? []).map((pr) => [pr.T, pr.I]))); + function getBuildOrPub(trans: MinifiedProductDetails['PT'][number]) { const ret = { J: product.J, CB: 0, CR: 0 }; if (trans.S === WorkflowState.Product_Build) { - const currentBuild = product.PB?.find((pb) => pb.T === trans.I); - if (currentBuild) { - ret.CB = currentBuild.I; - } + ret.CB = buildByTransition.get(trans.I) ?? 0; } else if (trans.S === WorkflowState.Product_Publish) { - const currentRelease = product.PR?.find((pp) => pp.T === trans.I); - if (currentRelease) { - ret.CR = currentRelease.I; - } + ret.CR = releaseByTransition.get(trans.I) ?? 0; } return ret; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/products/components/ProductDetails.svelte` around lines 55 - 72, getBuildOrPub currently does a linear search over product.PB and product.PR on every call; precompute lookup maps once (e.g., buildMapByT for PB and PR keyed by T -> I) in the component initialization/top of the render logic, then change getBuildOrPub to read from those maps instead of using .find; reference the existing getBuildOrPub function and the product.PB / product.PR arrays when adding the maps and updating the function to use them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/products/components/ProductDetails.svelte`:
- Around line 8-10: The Props interface in the module script references types
that are only imported in the instance script (Prisma and
MinifiedProductDetails), causing a module-scope type error; fix it by importing
the missing type definitions into the module script (add imports for Prisma and
MinifiedProductDetails into the <script lang="ts" module> block) or
alternatively move the Props declaration into the instance <script lang="ts">
where those imports already exist so Props can resolve correctly; ensure the
type import names exactly match Prisma and MinifiedProductDetails used by the
Props interface.
In `@src/lib/projects/sse.ts`:
- Around line 298-315: The BuildEngineUrl is being stringified via a template
literal which turns null/undefined into the strings "null"/"undefined"; change
the isSuper branch to capture the result of getURLandToken(...) into a variable
and assign BuildEngineUrl to that variable's url only if it's not nullish (i.e.,
avoid using `${...}`), so use the raw value (or optional chaining and nullish
coalescing) from getURLandToken(...) instead of stringifying it; locate the
expression around BuildEngineUrl, getURLandToken, and
DatabaseReads.organizations.findFirstOrThrow to implement this check so
linkToBuildEngine receives undefined when no URL is present.
In `@src/lib/server/database/Products.ts`:
- Around line 160-171: The code only checks ProductDefinitionId and StoreId to
set newDefinition/newStore, which lets changing projectId bypass org-allowance
checks; when loading the existing row with prisma.products.findUnique (selecting
ProductDefinitionId and StoreId) also select ProjectId and treat a ProjectId
change as requiring revalidation — i.e., set newDefinition = true and newStore =
true if product.ProjectId !== projectId so the existing store/definition are
rechecked against the new project’s org, and apply the same change to the other
identical block (lines ~247-265) that computes newDefinition/newStore.
In `@src/lib/server/database/Stores.ts`:
- Around line 52-59: The update function (exported update) currently calls
prisma.stores.update but does not trigger SSE for projectOrg, causing stale
labels; after the prisma.stores.update completes, enqueue a
SvelteSSE_UpdateProjectOrg event for any projects whose orgs are linked to this
store or that currently use it (use the store id to query related projects/orgs
via the same relationships used by getProjectOrgData), ensuring you call the SSE
enqueue/dispatch path used elsewhere in the codebase so projectOrg viewers
receive the update.
---
Duplicate comments:
In `@src/lib/projects/sse.ts`:
- Around line 522-546: The initial snapshot is emitted before registering the
update listener, which can drop updates that occur in the gap; fix by
registering the update callback with SSEPageUpdates.on(stream, updateCb) before
calling emit(event, stringify(await query(id, userSession))), and ensure the
same error/cleanup logic currently inside updateCb (SSEPageUpdates.off(stream,
updateCb) and clearInterval(pingInterval)) is applied if the initial emit fails
(check the returned error and remove the listener/clear interval as needed);
keep using the existing functions updateCb, query, stringify, emit,
SSEPageUpdates.on/off, stream, and pingInterval so the lifecycle and cleanup
remain consistent.
In `@src/routes/`(authenticated)/workflow-instances/[product_id]/+page.server.ts:
- Around line 84-129: The second call to
DatabaseReads.products.findUniqueOrThrow(...) before calling
minifyProductDetails can throw a 500 if the product vanished; instead reuse the
earlier fetched product object (the variable used around line 19) when calling
minifyProductDetails, or change this lookup to
DatabaseReads.products.findUnique(...) and handle a null by returning the
intended 404; update the call site that currently passes the findUniqueOrThrow
result into minifyProductDetails (and the use of product.Project.Organization
for getURLandToken) to use the reused product variable (or the nullable result)
so you avoid a redundant throw.
---
Nitpick comments:
In `@src/lib/products/components/ProductDetails.svelte`:
- Around line 55-72: getBuildOrPub currently does a linear search over
product.PB and product.PR on every call; precompute lookup maps once (e.g.,
buildMapByT for PB and PR keyed by T -> I) in the component initialization/top
of the render logic, then change getBuildOrPub to read from those maps instead
of using .find; reference the existing getBuildOrPub function and the product.PB
/ product.PR arrays when adding the maps and updating the function to use them.
In `@src/lib/server/database/Users.ts`:
- Around line 78-126: The block handling RoleId.OrgAdmin runs
prisma.projects.findMany({ where: { OrganizationId } }) twice (once inside
getQueues().UserTasks.addBulk and again for getQueues().SvelteSSE.add); change
this by calling prisma.projects.findMany(...) once, store the result (or just
the project Ids) in a local variable (e.g., projectRecords or projectIds), and
reuse that variable in the UserTasks.addBulk flatMap and in SvelteSSE.add;
update references inside the created job objects to use the cached project id
values and keep existing variables like OrganizationId, UserId, enabled, role,
and RoleId.OrgAdmin unchanged.
In `@src/routes/`(authenticated)/projects/[id=idNumber]/+page.server.ts:
- Around line 59-69: The four independent fetches (getProjectDetails,
getProjectGroupData, getProjectOrgData, getProjectProducts) are executed
sequentially; wrap them in a single Promise.all to run them in parallel and then
destructure the results into projectData, groupData, orgData, productData before
returning; keep the remaining await calls (superValidate for addAuthorSchema,
addReviewerSchema, projectActionSchema) unchanged and preserve jobsAvailable and
showRebuildToggles in the returned object.
In `@src/routes/`(authenticated)/projects/[id=idNumber]/+page.svelte:
- Around line 82-90: The code repeatedly calls orgData.Stores.find(...) per
product causing O(n*m) work on each SSE recompute; add a storeMap in the same
$derived.by() block (alongside productMap) that builds a Map from orgData.Stores
by Id, then replace all calls to orgData.Stores.find(...) in this component (and
the other affected lines around 259-263) with lookups into storeMap.get(storeId)
so product rendering/join becomes linear; update references to use storeMap
where the component currently expects a store from orgData.Stores.find.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a23c4bd1-e038-449c-b397-b6be814f28de
📒 Files selected for processing (32)
src/lib/bullmq.tssrc/lib/products/components/BuildArtifacts.sveltesrc/lib/products/components/ProductDetails.sveltesrc/lib/products/components/ReleaseInfo.sveltesrc/lib/products/index.tssrc/lib/projects/listener.tssrc/lib/projects/sse.tssrc/lib/server/bullmq/BullWorker.tssrc/lib/server/bullmq/types.tssrc/lib/server/database/Authors.tssrc/lib/server/database/Organizations.tssrc/lib/server/database/ProductDefinitions.tssrc/lib/server/database/ProductTransitions.tssrc/lib/server/database/Products.tssrc/lib/server/database/Projects.tssrc/lib/server/database/Reviewers.tssrc/lib/server/database/Stores.tssrc/lib/server/database/Users.tssrc/lib/workflowTypes.tssrc/routes/(authenticated)/admin/settings/stores/edit/+page.server.tssrc/routes/(authenticated)/admin/settings/stores/edit/+page.sveltesrc/routes/(authenticated)/organizations/[id=idNumber]/settings/products/+page.server.tssrc/routes/(authenticated)/organizations/[id=idNumber]/settings/products/+page.sveltesrc/routes/(authenticated)/organizations/[id=idNumber]/settings/stores/edit/+page.sveltesrc/routes/(authenticated)/projects/[id=idNumber]/+page.server.tssrc/routes/(authenticated)/projects/[id=idNumber]/+page.sveltesrc/routes/(authenticated)/projects/[id=idNumber]/ProductCard.sveltesrc/routes/(authenticated)/projects/[id=idNumber]/modals/DeleteProduct.sveltesrc/routes/(authenticated)/projects/[id=idNumber]/modals/Properties.sveltesrc/routes/(authenticated)/projects/[id=idNumber]/sse/products/+server.tssrc/routes/(authenticated)/workflow-instances/[product_id]/+page.server.tssrc/routes/(authenticated)/workflow-instances/[product_id]/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (13)
- src/lib/server/database/Organizations.ts
- src/lib/bullmq.ts
- src/lib/products/components/ReleaseInfo.svelte
- src/routes/(authenticated)/projects/[id=idNumber]/modals/Properties.svelte
- src/lib/server/database/ProductDefinitions.ts
- src/lib/server/database/ProductTransitions.ts
- src/routes/(authenticated)/projects/[id=idNumber]/sse/products/+server.ts
- src/lib/server/database/Authors.ts
- src/routes/(authenticated)/organizations/[id=idNumber]/settings/products/+page.server.ts
- src/routes/(authenticated)/organizations/[id=idNumber]/settings/products/+page.svelte
- src/routes/(authenticated)/projects/[id=idNumber]/modals/DeleteProduct.svelte
- src/routes/(authenticated)/organizations/[id=idNumber]/settings/stores/edit/+page.svelte
- src/routes/(authenticated)/admin/settings/stores/edit/+page.svelte
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/lib/projects/sse.ts (2)
523-545:⚠️ Potential issue | 🟠 MajorSubscribe before sending the initial snapshot.
Lines 523-545 still emit the first payload before
SSEPageUpdates.on(stream, updateCb)is registered. Any update that lands in that gap is lost, so the client can start from stale data until the next change. Register first and reuse the same refresh path for the initial send.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/projects/sse.ts` around lines 523 - 545, The initial snapshot is emitted before the listener is registered, risking lost updates; move the SSEPageUpdates.on(stream, updateCb) registration ahead of the initial send and reuse the same update flow by invoking updateCb([id]) (or calling the same internal path) to emit the initial snapshot so the handler logic and its error/cleanup behavior are shared; ensure emit, query, stringify, updateCb and pingInterval semantics are preserved (i.e., register SSEPageUpdates.on(stream, updateCb) first, then call updateCb([id]) to perform the initial query+emit and keep the same error handling that removes the listener and clears pingInterval).
330-335:⚠️ Potential issue | 🟡 MinorAdd
orderBytoUserTasks.Line 330 selects a to-many Prisma relation with no ordering. Prisma does not guarantee relation order here, so any downstream logic that treats the last task as the current/latest one can flip between refreshes.
💡 Suggested fix
UserTasks: { select: { DateCreated: true, UserId: true - } + }, + orderBy: { + DateCreated: 'asc' + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/projects/sse.ts` around lines 330 - 335, The UserTasks relation selection is missing an explicit order, so relation order is nondeterministic; update the UserTasks selection (in the sse.ts query) to include an orderBy on DateCreated (e.g., orderBy: { DateCreated: 'asc' }) so the returned array has a stable chronological order (ascending makes the last element the latest task as downstream logic expects); keep the existing select (DateCreated, UserId) and add the orderBy clause inside the UserTasks block.
🧹 Nitpick comments (2)
src/lib/products/index.ts (1)
260-261: Consider optional chaining for consistency.Lines 260-261 use
&&for null checks, whileminifyTransitionuses optional chaining (?.) for similar patterns. Optional chaining would be more idiomatic here.♻️ Proposed refactor
- WS: product.WorkflowInstance && product.WorkflowInstance.State, - WT: product.WorkflowInstance && product.WorkflowInstance.WorkflowDefinition.Type, + WS: product.WorkflowInstance?.State, + WT: product.WorkflowInstance?.WorkflowDefinition.Type,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/products/index.ts` around lines 260 - 261, Replace the logical-AND null checks with optional chaining for consistency: update the WS and WT fields that currently use "product.WorkflowInstance && product.WorkflowInstance.State" and "product.WorkflowInstance && product.WorkflowInstance.WorkflowDefinition.Type" to use optional chaining on product.WorkflowInstance (and WorkflowDefinition) so they read via product.WorkflowInstance?.State and product.WorkflowInstance?.WorkflowDefinition?.Type, matching the style used in minifyTransition.src/lib/projects/sse.ts (1)
122-184: Parallelize the independent SSE reads.Lines 122-184, 219-272, and 298-399 await unrelated queries one after another. On these hot SSE paths that turns one refresh into several serial DB round trips even though the results can be fetched concurrently.
Promise.allhere would better align with the PR's performance goal.Also applies to: 219-272, 298-399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/projects/sse.ts` around lines 122 - 184, The three independent DB reads (possibleOwners using DatabaseReads.users.findMany, possibleGroups using DatabaseReads.groups.findMany, and possibleAuthors using DatabaseReads.users.findMany) and the userGroupsForOrg call should be executed in parallel instead of awaited serially; wrap those calls in a Promise.all (or multiple Promise.all batches if you prefer grouping) and then destructure the resolved values into possibleOwners, possibleGroups, possibleAuthors, and userGroups (map the userGroups result to .map(g => g.Id) after resolution); apply the same pattern to the other mentioned blocks (the awaits at 219-272 and 298-399) so unrelated queries run concurrently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/products/index.ts`:
- Around line 104-113: The JSDoc comment incorrectly documents the shortcodes:
update the block so PR is described as ProductPublications and PT as
ProductTransitions to match the actual mapping used in the code (the
objects/constants named ProductPublications/ProductTransitions and their short
keys PR/PT); ensure the comment text replaces the duplicated PR entry with PT:
ProductTransitions so the JSDoc and the runtime mappings are consistent.
- Around line 206-218: The JSDoc key casing doesn't match the code: the doc
lists "Act: ActiveTransition" while the code uses "AcT" for the same mapping;
fix by renaming the code key "AcT" to "Act" (or alternatively update the JSDoc
to "AcT" if you prefer that convention) so the key for ActiveTransition is
consistently "Act" across the module—search for the "AcT" identifier in
src/lib/products/index.ts and replace it with "Act" (or update the JSDoc) and
run tests/lint to ensure no other references break.
In `@src/lib/projects/sse.ts`:
- Around line 69-75: Catch blocks in the SSE fetchers currently convert every
exception into a generic error(500); instead, update the catch logic in the
functions using span.recordException / span.setStatus (the blocks around the
throw error(500)) to: if the caught error is an instance of HttpError re-throw
it unchanged; if it is a NotFoundError from findUniqueOrThrow (or matches a
"NotFound" sentinel) throw error(404) with a clear message; otherwise record the
exception on the span, set status as ERROR, and then throw error(500). Apply
this change to all catch sites in this file that currently do throw error(500)
so expected lookup failures and upstream HTTP errors preserve their proper
status.
---
Duplicate comments:
In `@src/lib/projects/sse.ts`:
- Around line 523-545: The initial snapshot is emitted before the listener is
registered, risking lost updates; move the SSEPageUpdates.on(stream, updateCb)
registration ahead of the initial send and reuse the same update flow by
invoking updateCb([id]) (or calling the same internal path) to emit the initial
snapshot so the handler logic and its error/cleanup behavior are shared; ensure
emit, query, stringify, updateCb and pingInterval semantics are preserved (i.e.,
register SSEPageUpdates.on(stream, updateCb) first, then call updateCb([id]) to
perform the initial query+emit and keep the same error handling that removes the
listener and clears pingInterval).
- Around line 330-335: The UserTasks relation selection is missing an explicit
order, so relation order is nondeterministic; update the UserTasks selection (in
the sse.ts query) to include an orderBy on DateCreated (e.g., orderBy: {
DateCreated: 'asc' }) so the returned array has a stable chronological order
(ascending makes the last element the latest task as downstream logic expects);
keep the existing select (DateCreated, UserId) and add the orderBy clause inside
the UserTasks block.
---
Nitpick comments:
In `@src/lib/products/index.ts`:
- Around line 260-261: Replace the logical-AND null checks with optional
chaining for consistency: update the WS and WT fields that currently use
"product.WorkflowInstance && product.WorkflowInstance.State" and
"product.WorkflowInstance && product.WorkflowInstance.WorkflowDefinition.Type"
to use optional chaining on product.WorkflowInstance (and WorkflowDefinition) so
they read via product.WorkflowInstance?.State and
product.WorkflowInstance?.WorkflowDefinition?.Type, matching the style used in
minifyTransition.
In `@src/lib/projects/sse.ts`:
- Around line 122-184: The three independent DB reads (possibleOwners using
DatabaseReads.users.findMany, possibleGroups using
DatabaseReads.groups.findMany, and possibleAuthors using
DatabaseReads.users.findMany) and the userGroupsForOrg call should be executed
in parallel instead of awaited serially; wrap those calls in a Promise.all (or
multiple Promise.all batches if you prefer grouping) and then destructure the
resolved values into possibleOwners, possibleGroups, possibleAuthors, and
userGroups (map the userGroups result to .map(g => g.Id) after resolution);
apply the same pattern to the other mentioned blocks (the awaits at 219-272 and
298-399) so unrelated queries run concurrently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a675abb8-d719-4f2f-8d2a-cc95a51359df
📒 Files selected for processing (2)
src/lib/products/index.tssrc/lib/projects/sse.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/lib/projects/sse.ts (2)
537-567:⚠️ Potential issue | 🟠 MajorSubscribe before emitting the first snapshot.
The initial
query()/emit()happens beforeSSEPageUpdates.on(stream, updateCb). Any update published in that gap is lost, so the client can start from stale data. Register the listener first, then route the initial snapshot through the same refresh helper.💡 Suggested fix
export function createProducer<T>( id: number, userSession: Session['user'], stream: keyof SSEPageEvents, event: string, query: (id: number, userSession: Session['user']) => Promise<T> ) { return produce(async function start({ emit, lock }) { - // User will be allowed to see project updates until they reload - // even if their permission is revoked during the SSE connection. - const { error } = emit(event, stringify(await query(id, userSession))); - if (error) { - return; - } - async function updateCb(updateId: number[]) { + async function sendSnapshot() { + const data = await query(id, userSession); + return emit(event, stringify(data)).error; + } + + async function updateCb(updateId: number[]) { // This is a little wasteful because it will calculate much of the same data // multiple times if multiple users are connected to the same project page. if (updateId.includes(id)) { try { - const data = await query(id, userSession); - const { error } = emit(event, stringify(data)); - if (error) { + if (await sendSnapshot()) { SSEPageUpdates.off(stream, updateCb); clearInterval(pingInterval); } } catch { SSEPageUpdates.off(stream, updateCb); @@ } } SSEPageUpdates.on(stream, updateCb); + if (await sendSnapshot()) { + SSEPageUpdates.off(stream, updateCb); + return; + } const pingInterval = setInterval(function onDisconnect() { const { error } = emit('ping', ''); if (!error) return; SSEPageUpdates.off(stream, updateCb); clearInterval(pingInterval);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/projects/sse.ts` around lines 537 - 567, The initial snapshot is emitted before subscribing, allowing missed updates—move the SSEPageUpdates.on(stream, updateCb) registration above the first query/emit and reuse the same update flow (e.g., call updateCb([id]) or extract the emit+query logic into a helper used by both updateCb and the initial send) so the initial snapshot is routed through the same refresh helper; ensure updateCb, SSEPageUpdates.off(stream, updateCb), clearInterval(pingInterval), query(id, userSession), emit(event, stringify(...)) and the pingInterval behavior remain intact and error handling is preserved.
70-80:⚠️ Potential issue | 🟠 MajorMap Prisma not-found errors before the generic 500 path.
These handlers are checking
P2001, but Prisma ORM v6 surfaces missing records fromfindUniqueOrThrow()/findFirstOrThrow()asPrismaClientKnownRequestErrorcodeP2025. As written, a missing project falls through toerror(500), and any upstreamHttpErroris also flattened into 500. Re-throwHttpErrors first, then mapP2025to 404 before the generic fallback. (docs.prisma.io)In Prisma ORM v6, what error code do findUniqueOrThrow() and findFirstOrThrow() throw when no record is found?Also applies to: 191-201, 283-293, 426-436
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/projects/sse.ts` around lines 70 - 80, The catch blocks currently flatten all errors into a 500 and check for Prisma code 'P2001'; change them to first re-throw any HttpError (so existing HttpError is preserved), then map PrismaClientKnownRequestError with code 'P2025' to throw error(404) (this is the v6 missing-record code), and otherwise throw error(500); update the checks in the catch surrounding span.recordException / span.setStatus (and any other similar catch blocks using PrismaClientKnownRequestError and error(404)/error(500)) to use 'P2025' and to re-throw instances of HttpError before mapping Prisma errors.
🧹 Nitpick comments (2)
src/lib/projects/sse.ts (2)
124-190: Parallelize the group-side lookups.
possibleOwners,possibleGroups,possibleAuthors, anduserGroupsForOrg()all depend only onproject, but they are awaited one after another. On an SSE refresh path, that adds avoidable round-trip latency.♻️ Suggested refactor
- return { - authors: project.Authors, - reviewers: project.Reviewers, - possibleOwners: await DatabaseReads.users.findMany({ - ... - }), - possibleGroups: await DatabaseReads.groups.findMany({ - ... - }), - possibleAuthors: await DatabaseReads.users.findMany({ - ... - }), - userGroups: (await userGroupsForOrg(userSession.userId, project.OrganizationId)).map( - (g) => g.Id - ) - }; + const [possibleOwners, possibleGroups, possibleAuthors, userGroups] = await Promise.all([ + DatabaseReads.users.findMany({ + ... + }), + DatabaseReads.groups.findMany({ + ... + }), + DatabaseReads.users.findMany({ + ... + }), + userGroupsForOrg(userSession.userId, project.OrganizationId) + ]); + + return { + authors: project.Authors, + reviewers: project.Reviewers, + possibleOwners, + possibleGroups, + possibleAuthors, + userGroups: userGroups.map((g) => g.Id) + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/projects/sse.ts` around lines 124 - 190, The three sequential awaits (possibleOwners, possibleGroups, possibleAuthors) and the call to userGroupsForOrg are independent once you have project and id, causing unnecessary latency; change them to run in parallel by creating the Promises (using DatabaseReads.users.findMany for possibleOwners and possibleAuthors, DatabaseReads.groups.findMany for possibleGroups, and userGroupsForOrg for userGroups) and await them together with Promise.all, then map the resolved userGroups to IDs — update the variables possibleOwners, possibleGroups, possibleAuthors, and userGroups accordingly while retaining the existing query shapes and select clauses.
227-281: Run the org lookups concurrently.
Stores.findMany(...)andProductDefinitions.findMany(...)are independent onceprojectis loaded, but this path waits for the first query before starting the second.Promise.allwould shave one database round-trip off every org SSE refresh.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/projects/sse.ts` around lines 227 - 281, The two independent DB calls DatabaseReads.stores.findMany(...) and DatabaseReads.productDefinitions.findMany(...) are executed sequentially; update the code to run them concurrently with Promise.all after project is available: start both promises for Stores and ProductDefinitions at the same time, await Promise.all([storesPromise, productDefsPromise]), then return the resolved results using the same select shapes so the return object still has Stores and ProductDefinitions properties; keep the existing where/select clauses intact and reference the same project.OrganizationId and id/TypeId values when creating the promises.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/products/index.ts`:
- Around line 206-218: The JSDoc for the exported MinifiedProductCard is missing
a definition for the key P (product.Properties) and still defines PrT
incorrectly; update the JSDoc block that documents the minified card contract to
add an entry for "P: product.Properties" (or similar descriptive name) and
change the PrT doc to reflect that it comes from
PreviousTransition.DateTransition (or the exact property name used), ensuring
the exported MinifiedProductCard contract fully documents P and the corrected
PrT; locate the JSDoc near the MinifiedProductCard export and add these two
entries so downstream consumers see the complete contract.
In `@src/lib/projects/sse.ts`:
- Around line 309-326: getProjectProducts currently skips any project lookup for
non-super users, causing invalid/deleted project ids to return an empty payload
instead of 404; fix by explicitly verifying project existence for non-super
callers before querying products. In getProjectProducts, when isSuper is false
call DatabaseReads.projects.findFirstOrThrow (or equivalent
Projects.findFirstOrThrow) with where: { Id: id } to ensure a NotFound/throw is
raised, then proceed to DatabaseReads.products.findMany({ where: { ProjectId: id
} }); keep the existing super-path that uses
DatabaseReads.organizations.findFirstOrThrow and BuildEngineUrl unchanged.
---
Duplicate comments:
In `@src/lib/projects/sse.ts`:
- Around line 537-567: The initial snapshot is emitted before subscribing,
allowing missed updates—move the SSEPageUpdates.on(stream, updateCb)
registration above the first query/emit and reuse the same update flow (e.g.,
call updateCb([id]) or extract the emit+query logic into a helper used by both
updateCb and the initial send) so the initial snapshot is routed through the
same refresh helper; ensure updateCb, SSEPageUpdates.off(stream, updateCb),
clearInterval(pingInterval), query(id, userSession), emit(event, stringify(...))
and the pingInterval behavior remain intact and error handling is preserved.
- Around line 70-80: The catch blocks currently flatten all errors into a 500
and check for Prisma code 'P2001'; change them to first re-throw any HttpError
(so existing HttpError is preserved), then map PrismaClientKnownRequestError
with code 'P2025' to throw error(404) (this is the v6 missing-record code), and
otherwise throw error(500); update the checks in the catch surrounding
span.recordException / span.setStatus (and any other similar catch blocks using
PrismaClientKnownRequestError and error(404)/error(500)) to use 'P2025' and to
re-throw instances of HttpError before mapping Prisma errors.
---
Nitpick comments:
In `@src/lib/projects/sse.ts`:
- Around line 124-190: The three sequential awaits (possibleOwners,
possibleGroups, possibleAuthors) and the call to userGroupsForOrg are
independent once you have project and id, causing unnecessary latency; change
them to run in parallel by creating the Promises (using
DatabaseReads.users.findMany for possibleOwners and possibleAuthors,
DatabaseReads.groups.findMany for possibleGroups, and userGroupsForOrg for
userGroups) and await them together with Promise.all, then map the resolved
userGroups to IDs — update the variables possibleOwners, possibleGroups,
possibleAuthors, and userGroups accordingly while retaining the existing query
shapes and select clauses.
- Around line 227-281: The two independent DB calls
DatabaseReads.stores.findMany(...) and
DatabaseReads.productDefinitions.findMany(...) are executed sequentially; update
the code to run them concurrently with Promise.all after project is available:
start both promises for Stores and ProductDefinitions at the same time, await
Promise.all([storesPromise, productDefsPromise]), then return the resolved
results using the same select shapes so the return object still has Stores and
ProductDefinitions properties; keep the existing where/select clauses intact and
reference the same project.OrganizationId and id/TypeId values when creating the
promises.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce8b9446-5b33-429d-b513-78702426850b
📒 Files selected for processing (2)
src/lib/products/index.tssrc/lib/projects/sse.ts
~30% reduction - Palettes for Project Name & Product Definition Name - Reduced key length
Net ~50% reduction in packet size
Contributor
Author
|
Rebase for merge conflicts |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/server/database/Organizations.ts (1)
83-130:⚠️ Potential issue | 🟠 MajorMake the disable flow atomic.
Disabling a user now spans multiple writes. If the second
users.update()fails after the org disconnect succeeds, the user is left outside the organization but still attached to org groups/roles. Please perform the revoke flow in a single transaction, then enqueue the SSE update after commit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/database/Organizations.ts` around lines 83 - 130, Wrap the revoke flow in a single transaction so the org disconnect plus group disconnect and UserRoles.deleteMany happen atomically: inside prisma.$transaction(async (tx) => { first fetch the groups via tx.groups.findMany({ where: { OwnerId: OrganizationId, Users: { some: { Id: UserId } } }, select: { Id: true } }); then call tx.users.update(...) for UserId with data that both disconnects the Organization (Organizations: { disconnect: { Id: OrganizationId } }) and disconnects the groups (Groups: { disconnect: groups.map(g => ({ Id: g.Id })) }) and also removes roles (UserRoles: { deleteMany: { OrganizationId } }); return the update result; }); only after the transaction resolves successfully, call getQueues().SvelteSSE.add(...) to enqueue the SSE update (use the same BullMQ.JobType.SvelteSSE_UpdateProjectGroups and projects query as before).
♻️ Duplicate comments (5)
src/lib/projects/sse.ts (3)
541-565:⚠️ Potential issue | 🟠 MajorSubscribe before sending the initial snapshot.
emit(event, stringify(await query(...)))runs beforeSSEPageUpdates.on(stream, updateCb), so any update that lands in that window is dropped. Register the listener first and route the initial snapshot through the same refresh path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/projects/sse.ts` around lines 541 - 565, Register the SSE listener before sending the first snapshot: move the SSEPageUpdates.on(stream, updateCb) call to precede the initial emit and reuse the same update flow by invoking updateCb([id]) (or otherwise routing the initial snapshot through updateCb) instead of calling emit(event, stringify(await query(...))) directly; this ensures the listener (updateCb) is active to handle any concurrent updates and will still perform the emit/query/error handling (including SSEPageUpdates.off(stream, updateCb) and clearInterval(pingInterval)) consistently.
309-335:⚠️ Potential issue | 🟠 MajorNon-super callers still get a false “empty products” payload for missing projects.
The super-admin path verifies project existence through the organization lookup, but the non-super path goes straight to
findMany({ ProjectId: id }). For a deleted or invalid project id, this returns{ products: [] }instead of the 404 that the other project SSE getters return.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/projects/sse.ts` around lines 309 - 335, The non-super branch currently skips validating that the Project exists, so DatabaseReads.products.findMany({ ProjectId: id }) returns an empty array for missing/deleted projects instead of a 404; add an explicit existence check for the project before fetching products (mirror the super-admin behavior): call DatabaseReads.projects.findUnique or findFirstOrThrow (or equivalent) for the Project with Id === id and throw/not-found when missing, then proceed to call DatabaseReads.products.findMany; keep the existing isSuperAdmin check and BuildEngineUrl logic intact and use the project existence result to gate the products lookup so non-super callers receive the same 404 behavior as super-admins.
347-352:⚠️ Potential issue | 🟡 MinorMake
UserTasksordering explicit.This relation is emitted as an array but has no
orderBy. Prisma does not guarantee a stable order here, so any consumer that treats the list as chronological can render inconsistent “latest task” data across refreshes.💡 Minimal fix
UserTasks: { select: { DateCreated: true, UserId: true - } + }, + orderBy: { + DateCreated: 'asc' + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/projects/sse.ts` around lines 347 - 352, The UserTasks relation is returned as an unordered array; update the query that builds the UserTasks selection (the include/select block referencing UserTasks in src/lib/projects/sse.ts) to add an explicit orderBy on DateCreated (e.g., orderBy: { DateCreated: 'desc' } to get newest-first or 'asc' for oldest-first) alongside the existing select so consumers get a stable chronological order.src/routes/(authenticated)/workflow-instances/[product_id]/+page.server.ts (1)
88-133:⚠️ Potential issue | 🟡 MinorAvoid the second throw-on-miss product lookup here.
Line 63 already establishes existence, but Lines 89-131 reopen the same row with
findUniqueOrThrow(). If the product is deleted between those reads, this page turns a normal not-found into a 500. Reuse the earlier result or make the second read nullable and handle the miss explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(authenticated)/workflow-instances/[product_id]/+page.server.ts around lines 88 - 133, The product is fetched twice causing a potential 500 if it’s deleted between reads; replace the second DatabaseReads.products.findUniqueOrThrow call by reusing the earlier product object (or perform findUnique and handle null) before calling minifyProductDetails so you don’t throw again: pass the previously retrieved product into minifyProductDetails (or change findUniqueOrThrow to findUnique and add explicit null handling) and keep the getURLandToken(product.Project.Organization).url ?? undefined usage.src/lib/server/database/Products.ts (1)
162-170:⚠️ Potential issue | 🟠 MajorChanging
ProjectIdstill bypasses org-allowance revalidation.This block only compares the old and new
ProductDefinitionId/StoreId. If a product is moved to another project without changing those ids,newDefinitionandnewStoreboth stay false, so the org checks later short-circuit to true. LoadProjectIdhere too and treat a project change as forcing both revalidations.💡 Minimal fix
if (productId) { const product = await prisma.products.findUnique({ where: { Id: productId }, - select: { ProductDefinitionId: true, StoreId: true } + select: { ProjectId: true, ProductDefinitionId: true, StoreId: true } }); if (product) { - newDefinition = product.ProductDefinitionId !== productDefinitionId; - newStore = product.StoreId !== storeId; + const newProject = product.ProjectId !== projectId; + newDefinition = newProject || product.ProductDefinitionId !== productDefinitionId; + newStore = newProject || product.StoreId !== storeId; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/database/Products.ts` around lines 162 - 170, The code only loads ProductDefinitionId and StoreId from prisma.products.findUnique, so moving a product between projects (ProjectId change) doesn't flip newDefinition/newStore; update the findUnique select to include ProjectId, compare product.ProjectId to the incoming projectId, and when they differ set a flag (or set newDefinition = true and newStore = true) to force both revalidations; adjust variable names used in the surrounding logic (newDefinition, newStore, productDefinitionId, storeId, projectId, and the prisma.products.findUnique call) so a project change triggers the same org-allowance checks as a definition/store change.
🧹 Nitpick comments (2)
src/lib/server/database/Users.ts (1)
78-83: Reuse the org project lookup in this branch.This does the same
projects.findMany({ where: { OrganizationId } })twice. Reusing oneprojectIdsarray would cut the DB work in half on every role toggle.♻️ Suggested simplification
if (role === RoleId.OrgAdmin) { + const projectIds = ( + await prisma.projects.findMany({ + where: { OrganizationId }, + select: { Id: true } + }) + ).map((p) => p.Id); + await getQueues().UserTasks.addBulk( - ( - await prisma.projects.findMany({ - where: { OrganizationId }, - select: { Id: true } - }) - ).flatMap((p) => [ + projectIds.flatMap((projectId) => [ { - name: `${enabled ? 'Add' : 'Remove'} OrgAdmin tasks for User #${UserId} on Project #${p.Id}`, + name: `${enabled ? 'Add' : 'Remove'} OrgAdmin tasks for User #${UserId} on Project #${projectId}`, data: { type: BullMQ.JobType.UserTasks_Workflow, scope: 'Project', - projectId: p.Id, + projectId, operation: { type: enabled ? BullMQ.UserTasks.OpType.Create : BullMQ.UserTasks.OpType.Delete, users: [UserId], @@ { - name: `${enabled ? 'Add' : 'Remove'} OrgAdmin data deletion requests for User #${UserId} on Project #${p.Id}`, + name: `${enabled ? 'Add' : 'Remove'} OrgAdmin data deletion requests for User #${UserId} on Project #${projectId}`, data: { type: BullMQ.JobType.UserTasks_DeleteRequest, scope: 'Project', - projectId: p.Id, + projectId, operation: { type: enabled ? BullMQ.UserTasks.OpType.Create : BullMQ.UserTasks.OpType.Delete, users: [UserId], @@ getQueues().SvelteSSE.add( `Update Projects for Org #${OrganizationId} (role #${role} user #${UserId} ${enabled ? 'added' : 'removed'})`, { type: BullMQ.JobType.SvelteSSE_UpdateProjectGroups, - projectIds: ( - await prisma.projects.findMany({ - where: { OrganizationId }, - select: { Id: true } - }) - ).map((p) => p.Id) + projectIds } );Also applies to: 119-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/database/Users.ts` around lines 78 - 83, The code calls prisma.projects.findMany({ where: { OrganizationId } }) twice; fetch the projects once into a projectIds (or projectIds = projects.map(p => p.Id)) variable and reuse that array when calling getQueues().UserTasks.addBulk in both branches (the current block around getQueues().UserTasks.addBulk and the other branch around lines 119-125) so you eliminate the duplicate DB query and pass the same projectIds to the queue payloads.src/routes/(authenticated)/admin/settings/stores/+page.server.ts (1)
15-48: Aggregate these counts in the database.This second query materializes every matching
Productsrow and then re-counts them per organization in JS. On larger datasets, the load cost grows with total products rather than total stores. Please push the aggregation into Prisma/SQL, or cache the derived counts, before building theMap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(authenticated)/admin/settings/stores/+page.server.ts around lines 15 - 48, The code loads all Products for each store then counts per organization in JS which is O(total products); instead push the aggregation into the DB: run a grouped/count query (e.g., use DatabaseReads.products.groupBy or DatabaseReads.products.aggregate with grouping by Project.OrganizationId and StoreId) to return rows like { StoreId, OrganizationId, count } and then build the Map from those aggregated rows instead of using s.Products and filtering in the page loader; update the part that currently calls DatabaseReads.stores.findMany and the map construction that references s.Products to consume the grouped counts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/projects/sse.ts`:
- Around line 70-80: Update the Prisma not-found error checks to use the correct
error code 'P2025' instead of 'P2001' where you handle
PrismaClientKnownRequestError in the catch blocks (e.g., the catch that calls
span.recordException / span.setStatus and then checks e instanceof
PrismaClientKnownRequestError && e.code === 'P2001'); change those checks to
e.code === 'P2025' so findUniqueOrThrow()/findFirstOrThrow() return 404 via
throw error(404) instead of falling through to throw error(500). Apply the same
change in the other similar catch sites referenced (lines around the handlers
that throw error(404)/error(500) for projects/products).
In `@src/lib/server/database/ProductDefinitions.ts`:
- Around line 76-85: The SSE fan-out currently only finds projects by
Organization.ProductDefinitions.some.Id === id; update the
prisma.projects.findMany query used in the getQueues().SvelteSSE.add call so
projectIds includes projects that either belong to orgs with ProductDefinitions
some { Id: id } OR that have Products some { ProductDefinitionId: id } (i.e. add
an OR condition combining Organization: { ProductDefinitions: { some: { Id: id }
} } and Products: { some: { ProductDefinitionId: id } } ), keep the same select:
{ Id: true } and mapping (.map(p => p.Id)) so the queued job covers projects
still referencing the definition via Products as well as via the org.
In `@src/lib/server/database/Users.ts`:
- Around line 255-265: The SvelteSSE update currently only refreshes projects in
invite.Groups, so role-only invites (invite.Roles) never trigger org-wide
project updates; in acceptInvite() update the getQueues().SvelteSSE.add payload
so projectIds is the union of projects where GroupId in invite.Groups plus all
projects with OrganizationId == invite.OrganizationId whenever invite.Roles is
present (invite.Roles?.length > 0). Concretely, change the
prisma.projects.findMany filter used in the getQueues().SvelteSSE.add call (and
the surrounding logic in acceptInvite) to use an OR: [{ GroupId: { in:
invite.Groups } }, { OrganizationId: invite.OrganizationId }] but only include
the OrganizationId clause when invite.Roles exists, and dedupe the resulting
.map(p => p.Id) before passing to the
BullMQ.JobType.SvelteSSE_UpdateProjectGroups job.
---
Outside diff comments:
In `@src/lib/server/database/Organizations.ts`:
- Around line 83-130: Wrap the revoke flow in a single transaction so the org
disconnect plus group disconnect and UserRoles.deleteMany happen atomically:
inside prisma.$transaction(async (tx) => { first fetch the groups via
tx.groups.findMany({ where: { OwnerId: OrganizationId, Users: { some: { Id:
UserId } } }, select: { Id: true } }); then call tx.users.update(...) for UserId
with data that both disconnects the Organization (Organizations: { disconnect: {
Id: OrganizationId } }) and disconnects the groups (Groups: { disconnect:
groups.map(g => ({ Id: g.Id })) }) and also removes roles (UserRoles: {
deleteMany: { OrganizationId } }); return the update result; }); only after the
transaction resolves successfully, call getQueues().SvelteSSE.add(...) to
enqueue the SSE update (use the same
BullMQ.JobType.SvelteSSE_UpdateProjectGroups and projects query as before).
---
Duplicate comments:
In `@src/lib/projects/sse.ts`:
- Around line 541-565: Register the SSE listener before sending the first
snapshot: move the SSEPageUpdates.on(stream, updateCb) call to precede the
initial emit and reuse the same update flow by invoking updateCb([id]) (or
otherwise routing the initial snapshot through updateCb) instead of calling
emit(event, stringify(await query(...))) directly; this ensures the listener
(updateCb) is active to handle any concurrent updates and will still perform the
emit/query/error handling (including SSEPageUpdates.off(stream, updateCb) and
clearInterval(pingInterval)) consistently.
- Around line 309-335: The non-super branch currently skips validating that the
Project exists, so DatabaseReads.products.findMany({ ProjectId: id }) returns an
empty array for missing/deleted projects instead of a 404; add an explicit
existence check for the project before fetching products (mirror the super-admin
behavior): call DatabaseReads.projects.findUnique or findFirstOrThrow (or
equivalent) for the Project with Id === id and throw/not-found when missing,
then proceed to call DatabaseReads.products.findMany; keep the existing
isSuperAdmin check and BuildEngineUrl logic intact and use the project existence
result to gate the products lookup so non-super callers receive the same 404
behavior as super-admins.
- Around line 347-352: The UserTasks relation is returned as an unordered array;
update the query that builds the UserTasks selection (the include/select block
referencing UserTasks in src/lib/projects/sse.ts) to add an explicit orderBy on
DateCreated (e.g., orderBy: { DateCreated: 'desc' } to get newest-first or 'asc'
for oldest-first) alongside the existing select so consumers get a stable
chronological order.
In `@src/lib/server/database/Products.ts`:
- Around line 162-170: The code only loads ProductDefinitionId and StoreId from
prisma.products.findUnique, so moving a product between projects (ProjectId
change) doesn't flip newDefinition/newStore; update the findUnique select to
include ProjectId, compare product.ProjectId to the incoming projectId, and when
they differ set a flag (or set newDefinition = true and newStore = true) to
force both revalidations; adjust variable names used in the surrounding logic
(newDefinition, newStore, productDefinitionId, storeId, projectId, and the
prisma.products.findUnique call) so a project change triggers the same
org-allowance checks as a definition/store change.
In `@src/routes/`(authenticated)/workflow-instances/[product_id]/+page.server.ts:
- Around line 88-133: The product is fetched twice causing a potential 500 if
it’s deleted between reads; replace the second
DatabaseReads.products.findUniqueOrThrow call by reusing the earlier product
object (or perform findUnique and handle null) before calling
minifyProductDetails so you don’t throw again: pass the previously retrieved
product into minifyProductDetails (or change findUniqueOrThrow to findUnique and
add explicit null handling) and keep the
getURLandToken(product.Project.Organization).url ?? undefined usage.
---
Nitpick comments:
In `@src/lib/server/database/Users.ts`:
- Around line 78-83: The code calls prisma.projects.findMany({ where: {
OrganizationId } }) twice; fetch the projects once into a projectIds (or
projectIds = projects.map(p => p.Id)) variable and reuse that array when calling
getQueues().UserTasks.addBulk in both branches (the current block around
getQueues().UserTasks.addBulk and the other branch around lines 119-125) so you
eliminate the duplicate DB query and pass the same projectIds to the queue
payloads.
In `@src/routes/`(authenticated)/admin/settings/stores/+page.server.ts:
- Around line 15-48: The code loads all Products for each store then counts per
organization in JS which is O(total products); instead push the aggregation into
the DB: run a grouped/count query (e.g., use DatabaseReads.products.groupBy or
DatabaseReads.products.aggregate with grouping by Project.OrganizationId and
StoreId) to return rows like { StoreId, OrganizationId, count } and then build
the Map from those aggregated rows instead of using s.Products and filtering in
the page loader; update the part that currently calls
DatabaseReads.stores.findMany and the map construction that references
s.Products to consume the grouped counts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3f699c3-04cf-487e-9fe8-5673a7e26fc1
📒 Files selected for processing (48)
src/lib/bullmq.tssrc/lib/icons/index.tssrc/lib/organizations/components/StoreListDisplay.sveltesrc/lib/products/components/BuildArtifacts.sveltesrc/lib/products/components/ProductDetails.sveltesrc/lib/products/components/ReleaseInfo.sveltesrc/lib/products/index.tssrc/lib/projects/listener.tssrc/lib/projects/sse.tssrc/lib/server/bullmq/BullWorker.tssrc/lib/server/bullmq/types.tssrc/lib/server/database/Authors.tssrc/lib/server/database/Organizations.tssrc/lib/server/database/ProductDefinitions.tssrc/lib/server/database/ProductTransitions.tssrc/lib/server/database/Products.tssrc/lib/server/database/Projects.tssrc/lib/server/database/Reviewers.tssrc/lib/server/database/Stores.tssrc/lib/server/database/Users.tssrc/lib/utils/sorting.tssrc/lib/workflowTypes.tssrc/routes/(authenticated)/+layout.sveltesrc/routes/(authenticated)/admin/settings/organizations/edit/+page.sveltesrc/routes/(authenticated)/admin/settings/product-definitions/edit/+page.server.tssrc/routes/(authenticated)/admin/settings/product-definitions/new/+page.server.tssrc/routes/(authenticated)/admin/settings/stores/+page.server.tssrc/routes/(authenticated)/admin/settings/stores/+page.sveltesrc/routes/(authenticated)/admin/settings/stores/edit/+page.server.tssrc/routes/(authenticated)/admin/settings/stores/edit/+page.sveltesrc/routes/(authenticated)/organizations/[id=idNumber]/settings/products/+page.server.tssrc/routes/(authenticated)/organizations/[id=idNumber]/settings/products/+page.sveltesrc/routes/(authenticated)/organizations/[id=idNumber]/settings/stores/+page.server.tssrc/routes/(authenticated)/organizations/[id=idNumber]/settings/stores/+page.sveltesrc/routes/(authenticated)/organizations/[id=idNumber]/settings/stores/edit/+page.sveltesrc/routes/(authenticated)/projects/[id=idNumber]/+page.server.tssrc/routes/(authenticated)/projects/[id=idNumber]/+page.sveltesrc/routes/(authenticated)/projects/[id=idNumber]/ProductCard.sveltesrc/routes/(authenticated)/projects/[id=idNumber]/modals/DeleteProduct.sveltesrc/routes/(authenticated)/projects/[id=idNumber]/modals/Properties.sveltesrc/routes/(authenticated)/projects/[id=idNumber]/sse/+server.tssrc/routes/(authenticated)/projects/[id=idNumber]/sse/groups/+server.tssrc/routes/(authenticated)/projects/[id=idNumber]/sse/org/+server.tssrc/routes/(authenticated)/projects/[id=idNumber]/sse/products/+server.tssrc/routes/(authenticated)/tasks/+page.sveltesrc/routes/(authenticated)/tasks/[product_id]/+page.sveltesrc/routes/(authenticated)/workflow-instances/[product_id]/+page.server.tssrc/routes/(authenticated)/workflow-instances/[product_id]/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (20)
- src/lib/server/database/Authors.ts
- src/routes/(authenticated)/organizations/[id=idNumber]/settings/stores/edit/+page.svelte
- src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts
- src/routes/(authenticated)/admin/settings/stores/edit/+page.svelte
- src/routes/(authenticated)/projects/[id=idNumber]/sse/org/+server.ts
- src/routes/(authenticated)/workflow-instances/[product_id]/+page.svelte
- src/routes/(authenticated)/tasks/+page.svelte
- src/routes/(authenticated)/organizations/[id=idNumber]/settings/stores/+page.svelte
- src/routes/(authenticated)/admin/settings/stores/+page.svelte
- src/routes/(authenticated)/admin/settings/organizations/edit/+page.svelte
- src/routes/(authenticated)/projects/[id=idNumber]/sse/groups/+server.ts
- src/lib/server/bullmq/BullWorker.ts
- src/routes/(authenticated)/admin/settings/stores/edit/+page.server.ts
- src/lib/server/database/Projects.ts
- src/routes/(authenticated)/organizations/[id=idNumber]/settings/products/+page.server.ts
- src/routes/(authenticated)/projects/[id=idNumber]/modals/DeleteProduct.svelte
- src/routes/(authenticated)/+layout.svelte
- src/lib/server/database/Reviewers.ts
- src/lib/products/components/BuildArtifacts.svelte
- src/lib/icons/index.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes:
Summary by CodeRabbit
New Features
Improvements