refactor(portal): add filters in URL, clickable Rows, improved search, other UI changes#587
Merged
TilmanHaupt merged 33 commits intomainfrom Mar 25, 2026
Merged
refactor(portal): add filters in URL, clickable Rows, improved search, other UI changes#587TilmanHaupt merged 33 commits intomainfrom
TilmanHaupt merged 33 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds zod validation for deep-link image search params, refactors Images to a promise/Suspense-driven component with URL-synced filters/sorting, introduces URL and API helper modules and tests, expands image list UI (per-image/bulk actions, create/upload flow), and changes BFF to fetch all Glance pages and apply server-side filtering. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Images wrapper)
participant TRPC as TRPC Client
participant BFF as BFF / imageRouter
participant Glance as Glance API
Client->>TRPC: createImagesPromise(sort, direction, search, filters)
TRPC->>BFF: HTTP request (list all images)
BFF->>Glance: GET /images?... (follow next links)
Glance-->>BFF: page results (multiple)
BFF->>BFF: accumulate pages, apply filters (name, visibility, status, owner)
BFF-->>TRPC: aggregated images[]
TRPC-->>Client: Promise resolves with GlanceImage[]
Client->>Client: render ImagesContent via Suspense
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
apps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/urlHelpers.ts (1)
3-12: Move the images search contract into one shared module.
ImagesSearchParamsis re-declared here, again inapps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/List.tsxon Lines 31-40, and separately asimagesSearchSchemainapps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/$.tsxon Lines 56-71. Those copies will drift the next time a filter is added or renamed. Export a single schema/type pair and reuse it everywhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/urlHelpers.ts around lines 3 - 12, ImagesSearchParams and the imagesSearchSchema are duplicated across modules; extract a single shared module that exports the ImagesSearchParams type and imagesSearchSchema (or equivalent validation schema) and replace the local declarations with imports. Create a new file exporting both unique symbols (e.g., ImagesSearchParams and imagesSearchSchema), update the usages in the modules that currently declare them (referencing the local ImagesSearchParams in urlHelpers.ts, the copy in List.tsx, and imagesSearchSchema in compute/$.tsx), and remove the duplicate declarations so all three files import the canonical exports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/apiHelpers.ts:
- Around line 37-44: The mapping currently turns an undefined canUpdateMember
into true; change this to fail-closed by treating missing permission data as
false: update the mapping inside the .then(([canCreate, canDelete, canUpdate,
canCreateMember, canDeleteMember, canUpdateMember]) => ({ ... })) to set
canUpdateMember to a boolean that defaults to false (e.g., use canUpdateMember
?? false or Boolean(canUpdateMember)), ensuring canUserBulk's undefined result
does not grant the member-update capability.
In
`@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/List.tsx:
- Around line 304-339: The code is creating image-fetching promises in two
places which causes duplicate requests: imagesPromise is initialized from the
current component state and also recreated in the useEffect that syncs URL
params; similar duplication occurs for
suggestedImagesPromise/acceptedImagesPromise (and the other ranges noted). Fix
by consolidating fetching to a single source of truth — drive all fetches from
the URL-syncing useEffect: initialize
imagesPromise/suggestedImagesPromise/acceptedImagesPromise (and
permissionsPromise if applicable) to a neutral value (e.g., null) or a lazy
no-op, remove calls to setImagesPromise from your navigation/handler code (so
handlers only call navigate()), and only call
createImagesPromise/createSuggestedImagesPromise/createAcceptedImagesPromise
inside the useEffect that uses parseFiltersFromUrl and searchParams; reference
state setters setImagesPromise, setSuggestedImagesPromise,
setAcceptedImagesPromise, parseFiltersFromUrl, createImagesPromise,
createSuggestedImagesPromise, createAcceptedImagesPromise, and navigate to
locate and update the code.
- Around line 123-135: selectedImages can contain IDs not in the current
displayedImages, so the filters for deletableImages, protectedImages,
activeImages, and deactivatedImages incorrectly treat missing finds as falsy and
include hidden IDs; update the logic to first compute the intersection of
selectedImages and displayedImages (e.g., derive visibleSelected =
selectedImages.filter(id => displayedImages.some(img => img.id === id))) and
then base deletableImages, protectedImages, activeImages, and deactivatedImages
on visibleSelected (using displayedImages.find(...) as you do now), or
alternatively clear selectedImages whenever displayedImages changes to ensure
only visible items are considered; reference the symbols selectedImages,
displayedImages, deletableImages, protectedImages, activeImages,
deactivatedImages, and IMAGE_STATUSES when applying the fix.
In
`@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/$.tsx:
- Around line 56-71: imagesSearchSchema currently accepts arbitrary strings for
sort/filter params and must instead whitelist allowed values; update
imagesSearchSchema (the const imagesSearchSchema) to replace z.string() fields
with z.enum([...]).optional() (or z.union of literals) for status, visibility,
disk_format, container_format and sortBy using the exact allowed literals used
by the backend and by client.compute.listImagesWithSearch.query (e.g., allowed
sort keys like "name","created_at","size", allowed visibility values like
"public","private", allowed disk/container formats and status values), and
change protected to a boolean-style enum (e.g.,
z.enum(["true","false"]).optional() or z.boolean().optional() with proper
parsing) so only supported values pass validation; after that, update
parsing/usage in -components/Images/List.tsx and
-components/Images/apiHelpers.ts to rely on the validated, normalized values (or
convert enums to the shapes expected by listImagesWithSearch.query).
- Line 84: The shared parent route's validateSearch using
imagesSearchSchema.parse(search) (in $.tsx) enforces a strict z.object and will
strip unknown keys, causing future sibling tabs' search params to be silently
dropped; fix by either moving validateSearch off the shared $.tsx into the
images-only route so only images use imagesSearchSchema, or change
imagesSearchSchema to allow extra keys (call .passthrough() on the schema) so
validateSearch at the parent will not remove unrelated search params. Ensure you
update the reference to validateSearch and imagesSearchSchema accordingly.
---
Nitpick comments:
In
`@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/urlHelpers.ts:
- Around line 3-12: ImagesSearchParams and the imagesSearchSchema are duplicated
across modules; extract a single shared module that exports the
ImagesSearchParams type and imagesSearchSchema (or equivalent validation schema)
and replace the local declarations with imports. Create a new file exporting
both unique symbols (e.g., ImagesSearchParams and imagesSearchSchema), update
the usages in the modules that currently declare them (referencing the local
ImagesSearchParams in urlHelpers.ts, the copy in List.tsx, and
imagesSearchSchema in compute/$.tsx), and remove the duplicate declarations so
all three files import the canonical exports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3abb6c9e-5dd7-445e-b298-e6039f043263
📒 Files selected for processing (5)
apps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/$.tsxapps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/List.tsxapps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/apiHelpers.tsapps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/urlHelpers.test.tsapps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/urlHelpers.ts
...outes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/apiHelpers.ts
Show resolved
Hide resolved
...ent/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/List.tsx
Outdated
Show resolved
Hide resolved
...ent/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/List.tsx
Show resolved
Hide resolved
.../aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/$.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/List.tsx (2)
97-108:⚠️ Potential issue | 🟠 MajorSelection state can include IDs not visible in current filter results.
When filters, search, or member status change,
selectedImagesmay still contain IDs from the previous dataset. Sinceimages.find()returnsundefinedfor those hidden IDs, Line 98 treats them as deletable (!undefined?.protected→true). This can leak stale selections into bulk actions.Either derive these arrays from the intersection of
selectedImagesandimages, or clearselectedImageswhen the dataset changes.Suggested fix
+ const selectedImageIds = new Set(selectedImages) + const visibleSelectedImages = images.filter((image) => selectedImageIds.has(image.id)) + - const deletableImages = selectedImages.filter( - (imageId) => !images.find((image: GlanceImage) => image.id === imageId)?.protected - ) - const protectedImages = selectedImages.filter( - (imageId) => images.find((image: GlanceImage) => image.id === imageId)?.protected - ) - const activeImages = selectedImages.filter( - (imageId) => images.find((image: GlanceImage) => image.id === imageId)?.status === IMAGE_STATUSES.ACTIVE - ) - const deactivatedImages = selectedImages.filter( - (imageId) => images.find((image: GlanceImage) => image.id === imageId)?.status === IMAGE_STATUSES.DEACTIVATED - ) + const deletableImages = visibleSelectedImages.filter((image) => !image.protected).map((image) => image.id) + const protectedImages = visibleSelectedImages.filter((image) => image.protected).map((image) => image.id) + const activeImages = visibleSelectedImages + .filter((image) => image.status === IMAGE_STATUSES.ACTIVE) + .map((image) => image.id) + const deactivatedImages = visibleSelectedImages + .filter((image) => image.status === IMAGE_STATUSES.DEACTIVATED) + .map((image) => image.id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/List.tsx around lines 97 - 108, selectedImages may contain IDs not present in the current images array, causing images.find(...) to return undefined and incorrectly classifying hidden IDs as deletable/active/etc.; update the logic that computes deletableImages/protectedImages/activeImages/deactivatedImages to first intersect selectedImages with the current images IDs (or filter selectedImages by images.some(img => img.id === id)) so all lookups only run on visible images, or alternatively clear selectedImages when the images dataset changes (e.g., in an effect watching images/search/filter) to avoid stale IDs; refer to selectedImages, images, GlanceImage, and the arrays deletableImages/protectedImages/activeImages/deactivatedImages when making this change.
276-320:⚠️ Potential issue | 🟠 MajorDouble data fetch on mount and missing dependency in effect.
The initial
useStatecallback (Line 276-281) creates a promise immediately, then theuseEffect(Lines 285-309) runs on mount and creates another identical promise. This doubles the initial API call.Additionally,
filterSettings.filtersis used inbuildFilterParams(Line 304) but is not in the dependency array, which could cause stale closures.Consider driving all fetching from the effect alone by initializing
imagesPromiseto a deferred/lazy value, or skip the effect's fetch when params haven't changed from initial values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/List.tsx around lines 276 - 320, The component currently creates imagesPromise in useState and again in the useEffect causing duplicate fetches; change initialization so imagesPromise is not created eagerly (initialize to null/deferred) and let the useEffect exclusively call createImagesPromise via setImagesPromise (use the existing setImagesPromise and createImagesPromise symbols), and also add filterSettings.filters to the effect dependency array (since buildFilterParams uses it) so the effect reruns with fresh filter data; keep createPermissionsPromise usage unchanged.apps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/apiHelpers.ts (1)
43-43:⚠️ Potential issue | 🟠 MajorFail closed when a permission lookup is missing.
If
canUserBulkreturnsundefinedforimages:update_member, this line grants the capability by defaulting totrue. Missing permission data should disable the capability, not enable it.Suggested fix
- canUpdateMember: canUpdateMember ?? true, + canUpdateMember: canUpdateMember ?? false,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/apiHelpers.ts at line 43, The current default grants update-member permission when the permission lookup is missing; change the default to deny by setting canUpdateMember to false when canUserBulk returns undefined (e.g., replace "canUpdateMember: canUpdateMember ?? true" with a falsy-safe fallback like "canUpdateMember: canUpdateMember ?? false" or explicitly Boolean-checking the result of canUserBulk for the "images:update_member" check); update any related call sites that assume a missing value means allowed to instead treat undefined as denied to ensure "images:update_member" is fail-closed.
🧹 Nitpick comments (1)
apps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/List.tsx (1)
411-441: Consider adding an ErrorBoundary for rejected promises.
ImagesContentusesuse()to consumeimagesPromiseandpermissionsPromise. If either promise rejects, the error will propagate up with no local handling. Wrapping theSuspensein anErrorBoundarywould provide a graceful fallback for API failures.Example structure
<ErrorBoundary fallback={<ErrorMessage />}> <Suspense fallback={<LoadingSpinner />}> <ImagesContent ... /> </Suspense> </ErrorBoundary>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/List.tsx around lines 411 - 441, Wrap the Suspense/ImagesContent block in an ErrorBoundary so rejected promises from use() (imagesPromise or permissionsPromise) are caught and a user-friendly fallback is shown; locate the ImagesContent usage and replace the direct Suspense with an ErrorBoundary (using the app's existing ErrorBoundary or react-error-boundary) that renders a simple error fallback (e.g., ErrorMessage or ErrorFallback component) and inside it keep the same Suspense/fallback and pass all current props (imagesPromise, permissionsPromise, searchTerm, setSearchTerm, sortSettings, handleSortChange, filterSettings, handleFilterChange, selectedImages, setSelectedImages, createModalOpen, setCreateModalOpen, deleteAllModalOpen, setDeleteAllModalOpen, deactivateAllModalOpen, setDeactivateAllModalOpen, activateAllModalOpen, setActivateAllModalOpen, memberStatusView, setMemberStatusView) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/List.tsx:
- Around line 187-195: The ImageListView call currently passes suggestedImages
and acceptedImages as empty arrays, so isPending/isAccepted badges never render;
update the props to populate suggestedImages with images when memberStatusView
=== "pending" and acceptedImages with images when memberStatusView ===
"accepted" (or derive both by filtering the images list by member status if
images contains mixed statuses) — locate the ImageListView usage in List.tsx and
replace the hardcoded [] values for suggestedImages and acceptedImages with the
appropriate filtered arrays based on the memberStatusView state so ImageTableRow
can show the correct badges.
---
Duplicate comments:
In
`@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/apiHelpers.ts:
- Line 43: The current default grants update-member permission when the
permission lookup is missing; change the default to deny by setting
canUpdateMember to false when canUserBulk returns undefined (e.g., replace
"canUpdateMember: canUpdateMember ?? true" with a falsy-safe fallback like
"canUpdateMember: canUpdateMember ?? false" or explicitly Boolean-checking the
result of canUserBulk for the "images:update_member" check); update any related
call sites that assume a missing value means allowed to instead treat undefined
as denied to ensure "images:update_member" is fail-closed.
In
`@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/List.tsx:
- Around line 97-108: selectedImages may contain IDs not present in the current
images array, causing images.find(...) to return undefined and incorrectly
classifying hidden IDs as deletable/active/etc.; update the logic that computes
deletableImages/protectedImages/activeImages/deactivatedImages to first
intersect selectedImages with the current images IDs (or filter selectedImages
by images.some(img => img.id === id)) so all lookups only run on visible images,
or alternatively clear selectedImages when the images dataset changes (e.g., in
an effect watching images/search/filter) to avoid stale IDs; refer to
selectedImages, images, GlanceImage, and the arrays
deletableImages/protectedImages/activeImages/deactivatedImages when making this
change.
- Around line 276-320: The component currently creates imagesPromise in useState
and again in the useEffect causing duplicate fetches; change initialization so
imagesPromise is not created eagerly (initialize to null/deferred) and let the
useEffect exclusively call createImagesPromise via setImagesPromise (use the
existing setImagesPromise and createImagesPromise symbols), and also add
filterSettings.filters to the effect dependency array (since buildFilterParams
uses it) so the effect reruns with fresh filter data; keep
createPermissionsPromise usage unchanged.
---
Nitpick comments:
In
`@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/List.tsx:
- Around line 411-441: Wrap the Suspense/ImagesContent block in an ErrorBoundary
so rejected promises from use() (imagesPromise or permissionsPromise) are caught
and a user-friendly fallback is shown; locate the ImagesContent usage and
replace the direct Suspense with an ErrorBoundary (using the app's existing
ErrorBoundary or react-error-boundary) that renders a simple error fallback
(e.g., ErrorMessage or ErrorFallback component) and inside it keep the same
Suspense/fallback and pass all current props (imagesPromise, permissionsPromise,
searchTerm, setSearchTerm, sortSettings, handleSortChange, filterSettings,
handleFilterChange, selectedImages, setSelectedImages, createModalOpen,
setCreateModalOpen, deleteAllModalOpen, setDeleteAllModalOpen,
deactivateAllModalOpen, setDeactivateAllModalOpen, activateAllModalOpen,
setActivateAllModalOpen, memberStatusView, setMemberStatusView) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6de4851a-c6b4-472d-a84c-0551de75e58a
📒 Files selected for processing (2)
apps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/List.tsxapps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/apiHelpers.ts
...ent/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/List.tsx
Outdated
Show resolved
Hide resolved
Previously, the BFF only fetched a single page of images (e.g., 25 items) from OpenStack and then applied search/filters on that subset. This caused missing results - searching for an image that existed on page 2+ would return no results. Changes: - Fetch ALL pages from OpenStack when loading images (no pagination) - Apply all filtering in BFF after fetching complete dataset - Handle visibility="all" correctly (don't send to OpenStack API) - Add loading overlay to indicate when fetching all images - Fix both listImagesWithSearch and listImagesWithPagination endpoints This ensures search and filters work across the entire image catalog, not just the first page of results.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
...unts/$accountId/projects/$projectId/compute/-components/Images/-components/ImageListView.tsx
Fixed
Show fixed
Hide fixed
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/$.tsx (1)
65-99:⚠️ Potential issue | 🟠 MajorMove this validation off the shared compute route or make the schema passthrough.
validateSearchnow runs on the parent/_auth/.../compute/$route, and a plainz.object()strips unknown keys. Any sibling compute tab that later adds its own search state will have those params silently removed by the images schema. If this has to stay on the parent route,.passthrough()is the minimal safe fix.Also applies to: 112-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/$.tsx around lines 65 - 99, The imagesSearchSchema used by validateSearch on the shared parent compute route is a strict z.object which will strip unknown sibling tab search params; either move imagesSearchSchema off the shared route into the images-specific child route or make it permissive by converting imagesSearchSchema into a passthrough schema (call .passthrough() on the z.object) so unknown keys are preserved when validateSearch runs at the parent level; locate imagesSearchSchema and validateSearch in the compute route (/_auth/.../compute/$) to implement the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/List.tsx:
- Around line 290-296: imagesPromise is created from raw createImagesPromise
calls so TRPC cache invalidations (listImagesWithPagination, getImageById) no
longer refresh the list; update the component so images come from an
invalidation-aware source: either replace the local imagesPromise with a
trpcReact.useQuery call (or a useMemo that reads trpcReact query cache) using
the same params (tie to sortSettings, searchTerm, filterSettings,
memberStatusView) or add and pass a refetch callback into ImageListView that
calls setImagesPromise(...) with a fresh createImagesPromise whenever mutations
succeed; reference imagesPromise, setImagesPromise, createImagesPromise,
createPermissionsPromise, and ImageListView when applying the change.
In `@apps/aurora-portal/src/server/Compute/routers/imageRouter.ts`:
- Around line 73-79: The minimalQuery construction in imageRouter (the object
named minimalQuery passed to applyImageQueryParams) only copies sort fields,
which drops supported filters like disk_format, container_format, protected, and
member_status; update the code so minimalQuery includes all relevant filter
fields from queryInput (or change applyImageQueryParams to merge the entire
queryInput into the ListImagesInput) so that disk_format, container_format,
protected, member_status (and any other supported filters) are preserved for all
list paths (see minimalQuery, queryInput, applyImageQueryParams,
ListImagesInput).
---
Duplicate comments:
In
`@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/$.tsx:
- Around line 65-99: The imagesSearchSchema used by validateSearch on the shared
parent compute route is a strict z.object which will strip unknown sibling tab
search params; either move imagesSearchSchema off the shared route into the
images-specific child route or make it permissive by converting
imagesSearchSchema into a passthrough schema (call .passthrough() on the
z.object) so unknown keys are preserved when validateSearch runs at the parent
level; locate imagesSearchSchema and validateSearch in the compute route
(/_auth/.../compute/$) to implement the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ccff2fe0-b5c3-453f-9d0a-374cbfff5e06
📒 Files selected for processing (6)
apps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/$.tsxapps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/-components/ImageListView.tsxapps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/List.tsxapps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/apiHelpers.tsapps/aurora-portal/src/server/Compute/helpers/imageHelpers.tsapps/aurora-portal/src/server/Compute/routers/imageRouter.ts
...ent/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/List.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/-components/ImageListView.tsx (1)
641-660: Precompute lookup sets before mapping the rows.Lines 643-645 and Line 654 do linear scans for every rendered image. Once the list gets large, the grid render becomes quadratic. Build ID
Sets once and use.has()in the row props and toggle handler.Refactor sketch
-import { useState, useEffect, useRef, ReactNode } from "react" +import { useState, useEffect, useMemo, useRef, ReactNode } from "react" ... + const selectedImageIds = useMemo(() => new Set(selectedImages), [selectedImages]) + const suggestedImageIds = useMemo(() => new Set(suggestedImages.map(({ id }) => id)), [suggestedImages]) + const acceptedImageIds = useMemo(() => new Set(acceptedImages.map(({ id }) => id)), [acceptedImages]) ... - isSelected={!!selectedImages.find((imageId) => imageId === image.id)} - isPending={!!suggestedImages.find(({ id: imageId }) => imageId === image.id)} - isAccepted={!!acceptedImages.find(({ id: imageId }) => imageId === image.id)} + isSelected={selectedImageIds.has(image.id)} + isPending={suggestedImageIds.has(image.id)} + isAccepted={acceptedImageIds.has(image.id)} ... - const isImageSelected = !!selectedImages.find((imageId) => imageId === image.id) + const isImageSelected = selectedImageIds.has(image.id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/-components/ImageListView.tsx around lines 641 - 660, ImageListView currently does repeated linear scans inside the ImageTableRow mapping (using selectedImages.find, suggestedImages.find, acceptedImages.find and again in the onSelect toggle), causing O(n^2) rendering for large lists; fix by precomputing three ID Sets (e.g., selectedIds = new Set(selectedImages), suggestedIds = new Set(suggestedImages.map(i=>i.id)), acceptedIds = new Set(acceptedImages.map(i=>i.id)) ) before mapping and replace .find calls with .has(image.id), and in the onSelect handler use the selectedIds lookup and update setSelectedImages by producing a new array without scanning (toggle: if selectedIds.has(image.id) remove id, else append id) so row props and toggle run O(1) per row; update references to ImageListView, selectedImages, suggestedImages, acceptedImages, setSelectedImages, and ImageTableRow accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/-components/ImageListView.tsx:
- Around line 604-612: The header checkbox currently uses selectedImages.length
=== images.length which can be wrong when selectedImages includes IDs not in the
current visible images; change the checked logic to test membership against
visible IDs (e.g., images.every(img => selectedImages.includes(img.id))) and
update the onChange handler to toggle only the visible image IDs: when all
visible IDs are selected remove those IDs from selectedImages (filter out
images' ids), otherwise add the visible IDs to selectedImages (union them with
existing selections) using the existing selectedImages, images and
setSelectedImages symbols.
- Around line 711-731: The global early-return that renders the standalone
loading row is unmounting the modal subtree while mutations are pending, causing
EditImageDetailsModal/EditImageMetadataModal/DeleteImageModal to disappear
mid-submit; fix by ensuring the modals are rendered even when the list shows the
global loading row—either move the early-return loading UI into a conditional
that only replaces the list content (not the entire component) or render the
three modals (EditImageDetailsModal, EditImageMetadataModal, DeleteImageModal)
outside/above that early-return path so they remain mounted; also verify each
modal's isLoading prop uses the appropriate mutation pending flag
(updateImageMutation.isPending or deleteImageMutation.isPending) and that
isDisabled for DeleteImageModal safely reads selectedImage.protected.
---
Nitpick comments:
In
`@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/-components/ImageListView.tsx:
- Around line 641-660: ImageListView currently does repeated linear scans inside
the ImageTableRow mapping (using selectedImages.find, suggestedImages.find,
acceptedImages.find and again in the onSelect toggle), causing O(n^2) rendering
for large lists; fix by precomputing three ID Sets (e.g., selectedIds = new
Set(selectedImages), suggestedIds = new Set(suggestedImages.map(i=>i.id)),
acceptedIds = new Set(acceptedImages.map(i=>i.id)) ) before mapping and replace
.find calls with .has(image.id), and in the onSelect handler use the selectedIds
lookup and update setSelectedImages by producing a new array without scanning
(toggle: if selectedIds.has(image.id) remove id, else append id) so row props
and toggle run O(1) per row; update references to ImageListView, selectedImages,
suggestedImages, acceptedImages, setSelectedImages, and ImageTableRow
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3593a20-311a-4826-a778-56c49761a19c
📒 Files selected for processing (1)
apps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/-components/ImageListView.tsx
...unts/$accountId/projects/$projectId/compute/-components/Images/-components/ImageListView.tsx
Show resolved
Hide resolved
...unts/$accountId/projects/$projectId/compute/-components/Images/-components/ImageListView.tsx
Show resolved
Hide resolved
Add .passthrough() to imagesSearchSchema to prevent sibling tabs' search params from being stripped. The parent route validates all search params, so without passthrough, future search params for instances, keypairs, servergroups, or flavors tabs would be silently dropped.
Add .passthrough() to imagesSearchSchema to prevent sibling tabs' search params from being stripped. The parent route validates all search params, so without passthrough, future search params for instances, keypairs, servergroups, or flavors tabs would be silently dropped.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/-components/ImageListView.tsx (1)
55-55:⚠️ Potential issue | 🟡 MinorTrailing space in import path will likely cause module resolution failure.
The import path
"./ManageImageAccessModal "has a trailing space that will cause the bundler to fail to resolve the module.🐛 Proposed fix
-import { ManageImageAccessModal } from "./ManageImageAccessModal " +import { ManageImageAccessModal } from "./ManageImageAccessModal"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/-components/ImageListView.tsx at line 55, The import for ManageImageAccessModal has a trailing space in the module specifier which will break module resolution; update the import statement that references ManageImageAccessModal (the import line importing ManageImageAccessModal) to remove the trailing space so the path is "./ManageImageAccessModal" (ensure the symbol ManageImageAccessModal continues to be imported correctly and rebuild to verify resolution).
🧹 Nitpick comments (3)
apps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/-components/ImageListView.tsx (3)
643-654: Consider using.includes()or a Set for membership checks.Using
.find()with a comparison callback is functionally correct but.includes()is more idiomatic for simple ID lookups:-isSelected={!!selectedImages.find((imageId) => imageId === image.id)} +isSelected={selectedImages.includes(image.id)}-const isImageSelected = !!selectedImages.find((imageId) => imageId === image.id) +const isImageSelected = selectedImages.includes(image.id)For larger lists, consider converting
selectedImagesto aSetfor O(1) lookups.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/-components/ImageListView.tsx around lines 643 - 654, Replace the repeated .find(...) membership checks with .includes(...) for readability and performance (or convert arrays to Sets for O(1) lookups) in ImageListView: change uses of selectedImages.find((imageId) => imageId === image.id), suggestedImages.find(({ id: imageId }) => imageId === image.id), and acceptedImages.find(({ id: imageId }) => imageId === image.id) to selectedImages.includes(image.id), suggestedImages.includes(image.id) and acceptedImages.includes(image.id) respectively (or build Sets once and use selectedImageSet.has(image.id) etc.), and update the onSelect handler’s isImageSelected computation to use the new includes/Set approach.
222-222: Remove debug console.log statement.This debug logging statement should be removed before merging to production.
🧹 Proposed fix
onData: (data) => { - console.log(`Upload: ${data?.percent}%`) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/-components/ImageListView.tsx at line 222, Remove the debug console.log call in the ImageListView component (the console.log(`Upload: ${data?.percent}%`) statement) so no debug output is left in production; locate it inside the ImageListView upload/progress handler and delete that line (or replace it with an appropriate structured logger call if your codebase requires centralized logging).
746-773: Array truthiness check is always true.
selectedImagesis anArray<string>, which is always truthy (even when empty[]). This conditional doesn't guard anything. Either checkselectedImages.length > 0or remove the conditional since the modals are already controlled by theirisOpenprops.♻️ Proposed fix
- {selectedImages && ( + {selectedImages.length > 0 && ( <> <DeleteImagesModalOr simply remove the conditional wrapper since modals check
isOpeninternally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/-components/ImageListView.tsx around lines 746 - 773, The conditional wrapper using selectedImages is incorrect because an Array<string> is always truthy; in ImageListView.tsx remove the outer guard or change it to selectedImages.length > 0 so the modal JSX (DeleteImagesModal, DeactivateImagesModal, ActivateImagesModal) is not rendered based on a meaningless truthiness check—prefer removing the conditional entirely since each modal is already controlled by its isOpen prop (keep all props and handlers like deletableImages, protectedImages, handleBulkDelete, handleBulkDeactivate, handleBulkActivate unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/$.tsx:
- Line 115: The route is currently hard-failing because validateSearch uses
imagesSearchSchema.parse(search) which throws on invalid params; change
validateSearch to use a safe parse pattern that degrades invalid optional fields
(e.g., use imagesSearchSchema.parse(search).catch?. or better: call
imagesSearchSchema.safeParse/searchSchema.parse inside a try/catch and on
failure set the optional image filter fields to undefined) so malformed
deep-link values don't crash the page—update the validateSearch implementation
to catch parse errors and return a sanitized search object with invalid optional
image filter fields set to undefined, referencing imagesSearchSchema and the
validateSearch handler in the route.
---
Outside diff comments:
In
`@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/-components/ImageListView.tsx:
- Line 55: The import for ManageImageAccessModal has a trailing space in the
module specifier which will break module resolution; update the import statement
that references ManageImageAccessModal (the import line importing
ManageImageAccessModal) to remove the trailing space so the path is
"./ManageImageAccessModal" (ensure the symbol ManageImageAccessModal continues
to be imported correctly and rebuild to verify resolution).
---
Nitpick comments:
In
`@apps/aurora-portal/src/client/routes/_auth/accounts/`$accountId/projects/$projectId/compute/-components/Images/-components/ImageListView.tsx:
- Around line 643-654: Replace the repeated .find(...) membership checks with
.includes(...) for readability and performance (or convert arrays to Sets for
O(1) lookups) in ImageListView: change uses of selectedImages.find((imageId) =>
imageId === image.id), suggestedImages.find(({ id: imageId }) => imageId ===
image.id), and acceptedImages.find(({ id: imageId }) => imageId === image.id) to
selectedImages.includes(image.id), suggestedImages.includes(image.id) and
acceptedImages.includes(image.id) respectively (or build Sets once and use
selectedImageSet.has(image.id) etc.), and update the onSelect handler’s
isImageSelected computation to use the new includes/Set approach.
- Line 222: Remove the debug console.log call in the ImageListView component
(the console.log(`Upload: ${data?.percent}%`) statement) so no debug output is
left in production; locate it inside the ImageListView upload/progress handler
and delete that line (or replace it with an appropriate structured logger call
if your codebase requires centralized logging).
- Around line 746-773: The conditional wrapper using selectedImages is incorrect
because an Array<string> is always truthy; in ImageListView.tsx remove the outer
guard or change it to selectedImages.length > 0 so the modal JSX
(DeleteImagesModal, DeactivateImagesModal, ActivateImagesModal) is not rendered
based on a meaningless truthiness check—prefer removing the conditional entirely
since each modal is already controlled by its isOpen prop (keep all props and
handlers like deletableImages, protectedImages, handleBulkDelete,
handleBulkDeactivate, handleBulkActivate unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26dc6a4b-6ad9-4113-8af7-338b0b7fc30d
📒 Files selected for processing (2)
apps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/$.tsxapps/aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/-components/ImageListView.tsx
.../aurora-portal/src/client/routes/_auth/accounts/$accountId/projects/$projectId/compute/$.tsx
Outdated
Show resolved
Hide resolved
Convert image and flavor table rows to use onClick navigation instead of link elements. This provides a better UX by making the entire row clickable. Added stopPropagation to checkbox and popup menu cells to prevent triggering row navigation when interacting with these elements.
Route member_status filter to the correct endpoint (listSharedImagesByMemberStatus) instead of passing it to listImagesWithSearch which doesn't handle it. This fixes the 'Show Suggested Images' and 'Show Accepted Images' filters that stopped working.
Move member status filtering (All/Suggested/Accepted Images) from popup menu to TabNavigation component positioned below filters. Extends ListToolbar to support optional tabs prop for reusable tab navigation pattern.
TilmanHaupt
changed the title
TilmanHaupt
commented
Mar 22, 2026
TilmanHaupt
commented
Mar 22, 2026
TilmanHaupt
commented
Mar 22, 2026
...ccountId/projects/$projectId/compute/-components/Flavors/-components/FlavorListContainer.tsx
Show resolved
Hide resolved
TilmanHaupt
commented
Mar 22, 2026
...outes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/apiHelpers.ts
Show resolved
Hide resolved
TilmanHaupt
commented
Mar 23, 2026
- Add multi-page pagination test for listImagesWithPagination - Add multi-page search tests for listImagesWithSearch - Test MIN_RESULTS_WHEN_SEARCHING early-stop behavior - Add MAX_PAGES safety limit test (1000 pages) - Explicitly set next: undefined in single-page tests - Update MAX_PAGES from 100 to 1000 to support larger deployments
andypf
reviewed
Mar 25, 2026
...ent/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/List.tsx
Outdated
Show resolved
Hide resolved
andypf
reviewed
Mar 25, 2026
Collaborator
andypf
left a comment
andypf
left a comment
There was a problem hiding this comment.
I left a few comments. Please address them
...client/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Overview.tsx
Show resolved
Hide resolved
mark-karnaukh-extern-sap
previously requested changes
Mar 25, 2026
Contributor
mark-karnaukh-extern-sap
left a comment
mark-karnaukh-extern-sap
left a comment
There was a problem hiding this comment.
@TilmanHaupt please check comments
...unts/$accountId/projects/$projectId/compute/-components/Images/-components/ImageTableRow.tsx
Show resolved
Hide resolved
...outes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/apiHelpers.ts
Show resolved
Hide resolved
...ent/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/List.tsx
Outdated
Show resolved
Hide resolved
...ent/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/List.tsx
Outdated
Show resolved
Hide resolved
...ent/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/List.tsx
Outdated
Show resolved
Hide resolved
...ent/routes/_auth/accounts/$accountId/projects/$projectId/compute/-components/Images/List.tsx
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
@TilmanHaupt overall good job, check out the comments left! 🙂👍
…yped images filter
mark-karnaukh-extern-sap
approved these changes
Mar 25, 2026
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
3 participants
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.
Summary
Checklist
Summary by CodeRabbit (deleted wrong statements by Tilman)
New Features
Refactor
Bug Fixes
Tests