UN-3769 [FEAT] Sortable resource lists with co-owner ownership#2200
UN-3769 [FEAT] Sortable resource lists with co-owner ownership#2200kirtimanmishrazipstack wants to merge 13 commits into
Conversation
…arch & pagination Replace the sparse ListView/ViewTools list UI with a shared sortable ResourceTable (Name / Owned By / Created Date / Actions) across Adapters, Workflows, Prompt Studio and Connectors. The Owned By column shows the owner avatar/name/email plus co-owner count and opens the co-owner modal. Sort (name/owner/created via the header dropdowns), owner-inclusive search and pagination are server-driven through a new apply_search_and_sort helper, whose pk__in re-wrap lifts the Postgres DISTINCT ON each for_user() manager carries so any column is orderable. Prompt Studio re-applies its prompt_count annotation after the re-wrap. Delete the now-unused ListView and ViewTools. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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:
WalkthroughThe change adds shared backend search and sorting, extends the pagination hook with sort and refresh state, and migrates custom tools, adapters, connectors, and workflows to server-driven paginated ChangesResource listing modernization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ResourceTable
participant usePaginatedList
participant ResourcePage
participant BackendListEndpoint
ResourceTable->>usePaginatedList: change page, search, or sort
usePaginatedList->>ResourcePage: invoke fetchRef with list state
ResourcePage->>BackendListEndpoint: request paginated filtered resources
BackendListEndpoint-->>ResourcePage: return results and count
ResourcePage->>usePaginatedList: apply response with sequence token
usePaginatedList-->>ResourceTable: update rows and pagination
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
| Filename | Overview |
|---|---|
| frontend/src/hooks/usePaginatedList.js | Centralizes pagination, search, sorting, request sequencing, and mutation refreshes; the latest-request tracking addresses the previously reported stale-page refresh race. |
| frontend/src/components/tool-settings/tool-settings/ToolSettings.jsx | Migrates adapter lists to ResourceTable and delegates deletion-success refreshes to the shared paginated-list state without introducing a distinct unresolved failure. |
| backend/utils/list_query.py | Adds shared owner-inclusive searching and stable per-column sorting over rewrapped resource querysets. |
| frontend/src/components/widgets/resource-table/ResourceTable.jsx | Adds the shared sortable resource table used by the migrated resource-list pages. |
Reviews (13): Last reviewed commit: "Merge branch 'main' into UN-3769-Show-co..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
kirtimanmishrazipstack
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
frontend/src/components/widgets/resource-table/ResourceTable.jsx (1)
238-240: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeprecation tooltip is hardcoded to "adapter".
ResourceTableis shared across Workflow/Connector/Prompt types, butdisabledTitlesays "This adapter is deprecated". Practically only adapters setis_deprecated, so it's rarely reachable elsewhere — still, usetypefor correctness if that ever changes.Proposed tweak
- const disabledTitle = deprecated ? "This adapter is deprecated" : ""; + const disabledTitle = deprecated ? `This ${type || "item"} is deprecated` : "";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/widgets/resource-table/ResourceTable.jsx` around lines 238 - 240, Update renderActions so the deprecation disabledTitle uses the ResourceTable type dynamically instead of hardcoding “adapter”. Preserve the existing empty title for non-deprecated items and ensure the resulting message remains grammatically correct for the supported resource types.frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx (1)
167-228: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the duplicated paginated-fetch flow into a shared helper. All four resource pages reimplement the identical params-building,
results ?? data ?? []/count ?? lengthenvelope parsing, empty-page step-back recursion, and error fallback. Consolidating into one helper (e.g.runPaginatedFetch({ request, page, pageSize, search, sortBy, order, setList, setPagination, onError })) removes ~4× drift risk as these evolve.
frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx#L167-L228: replacegetListOfToolsbody with a call to the shared helper (Prompt Studio URL + params).frontend/src/components/tool-settings/tool-settings/ToolSettings.jsx#L147-L212: replacegetAdaptersbody, keeping theadapter_type/typeguard as helper input.frontend/src/pages/ConnectorsPage.jsx#L120-L168: replacegetConnectorsbody with the shared helper.frontend/src/components/workflows/workflow/Workflows.jsx#L124-L169: replacegetProjectListbody with the shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx` around lines 167 - 228, Extract the duplicated paginated-fetch behavior into a shared helper such as runPaginatedFetch, centralizing parameter construction, envelope parsing, empty-page step-back recursion, loading/error fallback, and pagination updates. Replace getListOfTools in frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx:167-228 with the helper while preserving its Prompt Studio request; replace getAdapters in frontend/src/components/tool-settings/tool-settings/ToolSettings.jsx:147-212 while passing through its adapter_type/type guard; replace getConnectors in frontend/src/pages/ConnectorsPage.jsx:120-168 and getProjectList in frontend/src/components/workflows/workflow/Workflows.jsx:124-169 similarly, preserving each request URL and list-specific state setters.backend/adapter_processor_v2/views.py (1)
182-186: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
select_related/prefetch_relatedbeforeapply_search_and_sort.apply_search_and_sortre-wraps the queryset viamodel.objects.filter(pk__in=queryset.order_by("pk").values("pk")), so relations attached to the queryset before the helper call are dropped — only theselect_related=/prefetch_related=kwargs passed into the helper actually take effect.workflow_v2/views.py'sget_queryset()already avoids this by only attaching relations via the helper call.
backend/adapter_processor_v2/views.py#L182-L186: drop the pre-helper.select_related("created_by").prefetch_related("memberships__user"), since lines 196-203 already pass these as helper kwargs.backend/connector_v2/views.py#L93-L97: drop the pre-helper.select_related("created_by").prefetch_related("memberships__user"), since lines 129-136 already pass these as helper kwargs.backend/prompt_studio/prompt_studio_core_v2/views.py#L158-L160: drop the pre-helper.prefetch_related("memberships__user"), since lines 165-172 already pass it as a helper kwarg.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/adapter_processor_v2/views.py` around lines 182 - 186, Remove the pre-helper relation loading from get_queryset in backend/adapter_processor_v2/views.py lines 182-186, backend/connector_v2/views.py lines 93-97, and backend/prompt_studio/prompt_studio_core_v2/views.py lines 158-160; retain the existing select_related and prefetch_related kwargs passed to apply_search_and_sort, which should be the only relation-loading configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/utils/list_query.py`:
- Around line 60-65: Normalize the `sort_by` value to lowercase before looking
it up in the mapping within the sort-field selection logic, while preserving the
existing default behavior when it is missing or unsupported. Keep the `order`
handling and the existing `name`, `owner`, and `created` mappings unchanged.
In `@frontend/src/components/widgets/resource-table/ResourceTable.jsx`:
- Around line 148-150: Update the isImage detection in renderName to identify
actual image URL or data schemes instead of using icon.length > 4. Ensure
compound and multi-codepoint emoji remain rendered as icons, while valid remote
or data image sources still use the image rendering path.
---
Nitpick comments:
In `@backend/adapter_processor_v2/views.py`:
- Around line 182-186: Remove the pre-helper relation loading from get_queryset
in backend/adapter_processor_v2/views.py lines 182-186,
backend/connector_v2/views.py lines 93-97, and
backend/prompt_studio/prompt_studio_core_v2/views.py lines 158-160; retain the
existing select_related and prefetch_related kwargs passed to
apply_search_and_sort, which should be the only relation-loading configuration.
In `@frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx`:
- Around line 167-228: Extract the duplicated paginated-fetch behavior into a
shared helper such as runPaginatedFetch, centralizing parameter construction,
envelope parsing, empty-page step-back recursion, loading/error fallback, and
pagination updates. Replace getListOfTools in
frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx:167-228 with
the helper while preserving its Prompt Studio request; replace getAdapters in
frontend/src/components/tool-settings/tool-settings/ToolSettings.jsx:147-212
while passing through its adapter_type/type guard; replace getConnectors in
frontend/src/pages/ConnectorsPage.jsx:120-168 and getProjectList in
frontend/src/components/workflows/workflow/Workflows.jsx:124-169 similarly,
preserving each request URL and list-specific state setters.
In `@frontend/src/components/widgets/resource-table/ResourceTable.jsx`:
- Around line 238-240: Update renderActions so the deprecation disabledTitle
uses the ResourceTable type dynamically instead of hardcoding “adapter”.
Preserve the existing empty title for non-deprecated items and ensure the
resulting message remains grammatically correct for the supported resource
types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fb0d9b1-fc7b-4327-a330-c6b7dc079fe9
📒 Files selected for processing (16)
backend/adapter_processor_v2/views.pybackend/connector_v2/views.pybackend/prompt_studio/prompt_studio_core_v2/views.pybackend/utils/list_query.pybackend/workflow_manager/workflow_v2/views.pyfrontend/src/components/custom-tools/list-of-tools/ListOfTools.jsxfrontend/src/components/custom-tools/view-tools/ViewTools.cssfrontend/src/components/custom-tools/view-tools/ViewTools.jsxfrontend/src/components/tool-settings/tool-settings/ToolSettings.jsxfrontend/src/components/widgets/list-view/ListView.cssfrontend/src/components/widgets/list-view/ListView.jsxfrontend/src/components/widgets/resource-table/ResourceTable.cssfrontend/src/components/widgets/resource-table/ResourceTable.jsxfrontend/src/components/workflows/workflow/Workflows.jsxfrontend/src/hooks/usePaginatedList.jsfrontend/src/pages/ConnectorsPage.jsx
💤 Files with no reviewable changes (4)
- frontend/src/components/widgets/list-view/ListView.css
- frontend/src/components/custom-tools/view-tools/ViewTools.css
- frontend/src/components/custom-tools/view-tools/ViewTools.jsx
- frontend/src/components/widgets/list-view/ListView.jsx
Resolve the sole modify/delete conflict on ListView.css in favour of the deletion — the dead ListView component is removed by this branch, so main's CSS tweak to it is moot. prompt_studio_core_v2/views.py auto-merged cleanly (our get_queryset rewrite + main's PG task_status block occupy different regions). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…se race Resolve SonarCloud/Greptile/CodeRabbit review on the resource-list rollout: - Extract buildPagedParams + applyPagedResponse into usePaginatedList so the four list pages stop copy-pasting the params/response blocks (clears the SonarCloud new-code duplication gate). - applyPagedResponse drops stale responses via a per-page sequence token so a slow older request can't overwrite a newer query, and returns the empty-page stepback refetch so loading isn't cleared before replacement data arrives. - ResourceTable detects image icons by URL/data scheme instead of length, so compound (ZWJ) emoji no longer render as a broken <img>. - list_query lowercases sort_by before the dict lookup (matches order handling). - ToolSettings resets loading when a delete request fails. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/hooks/usePaginatedList.js (1)
141-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse these shared helpers in the resource pages.
The supplied graph context still shows local copies of
buildPagedParamsandapplyPagedResponseinfrontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx,frontend/src/components/tool-settings/tool-settings/ToolSettings.jsx,frontend/src/components/workflows/workflow/Workflows.jsx, andfrontend/src/pages/ConnectorsPage.jsx. Import the helpers from this module and remove those copies so fixes cannot diverge across listing surfaces.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/hooks/usePaginatedList.js` at line 141, Replace the local buildPagedParams and applyPagedResponse implementations in ListOfTools, ToolSettings, Workflows, and ConnectorsPage with imports from usePaginatedList.js, removing the duplicated helper definitions while preserving each page’s existing pagination behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/hooks/usePaginatedList.js`:
- Around line 28-71: Update the paginated-list request callers, including
ConnectorsPage and equivalent resource pages, so catch and finally handlers only
apply error or loading-state updates when their request sequence still matches
seqRef.current. Preserve the existing stale-response suppression and ensure a
refetch returned by applyPagedResponse lets the newest request retain ownership
of loading until replacement data completes.
---
Nitpick comments:
In `@frontend/src/hooks/usePaginatedList.js`:
- Line 141: Replace the local buildPagedParams and applyPagedResponse
implementations in ListOfTools, ToolSettings, Workflows, and ConnectorsPage with
imports from usePaginatedList.js, removing the duplicated helper definitions
while preserving each page’s existing pagination behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 217ff658-9d02-434f-b90e-d0be05fab4e7
📒 Files selected for processing (7)
backend/utils/list_query.pyfrontend/src/components/custom-tools/list-of-tools/ListOfTools.jsxfrontend/src/components/tool-settings/tool-settings/ToolSettings.jsxfrontend/src/components/widgets/resource-table/ResourceTable.jsxfrontend/src/components/workflows/workflow/Workflows.jsxfrontend/src/hooks/usePaginatedList.jsfrontend/src/pages/ConnectorsPage.jsx
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/src/pages/ConnectorsPage.jsx
- frontend/src/components/workflows/workflow/Workflows.jsx
- backend/utils/list_query.py
- frontend/src/components/widgets/resource-table/ResourceTable.jsx
- frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx
- frontend/src/components/tool-settings/tool-settings/ToolSettings.jsx
…tion gate The first review pass left SonarCloud at 8.4% new-code duplication; the real duplicated blocks were the per-page preamble and the co-owner modal JSX, not the fetch body. Fix both: - usePaginatedList now owns fetchRef (pages assign fetchRef.current) and returns handleListRefresh, so pages drop their local fetchListRef + identical handleListRefresh useCallback. - Add CoOwnerModal, a thin wrapper mapping a useCoOwnerManagement() bag + resourceType onto the CoOwnerManagement modal; the list pages now consume the hook as one object and render <CoOwnerModal .../> instead of repeating the 11-prop invocation. Net ~150 fewer lines; duplication drops well under the 3% gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The seq guard only suppressed stale successful responses; each page's catch and finally still ran unconditionally, so a superseded request could clear loading while a newer one was pending, or surface an error for a query the user had already moved past. Gate both on seq === seqRef.current so only the newest request owns the loading state and error reporting. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx`:
- Around line 177-187: Update the ListOfTools request state around the fetch
catch handler to track an explicit list error instead of setting displayList to
an empty array on failure. Clear the error when starting or successfully
completing a current request, and render a retryable error for initial or
empty-list failures; gate the normal empty-state branches around the existing
“No prompt projects available” and “No results found” rendering on !isLoading &&
!listError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 85878000-1314-4bfa-aa3c-e35f109d93d3
📒 Files selected for processing (4)
frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsxfrontend/src/components/tool-settings/tool-settings/ToolSettings.jsxfrontend/src/components/workflows/workflow/Workflows.jsxfrontend/src/pages/ConnectorsPage.jsx
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/pages/ConnectorsPage.jsx
- frontend/src/components/tool-settings/tool-settings/ToolSettings.jsx
- frontend/src/components/workflows/workflow/Workflows.jsx
…ointAt - On fetch failure the list pages set displayList to [], so a failed initial load rendered a misleading "No X available" empty state. Track an explicit loadError instead and render a retryable error (Retry refetches the current page), so a failure is no longer shown as an empty success. - colorForSeed uses String#codePointAt over charCodeAt (SonarCloud S7758). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… membership is_owner is true for any OWNER membership, so a co-owner viewing a resource they didn't create saw "Me" over the primary owner's avatar/email. Key the "Me" label on the displayed owner email instead; the creator viewing their own resource still reads "Me" via the email match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The adapter delete catch cleared isLoading unconditionally, so a failed delete could hide the spinner for a newer in-flight fetch (search/sort/paginate/refresh) and expose obsolete results. Snapshot the request token at delete start and clear loading only if no newer fetch has taken it over. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ToolSettings was the only list driving the shared isLoading from a row delete, which produced a string of overlap races (stuck loading, clobbering a newer fetch, concurrent deletes clearing each other). Drop loading from the delete entirely, matching the other four lists: success refetches via handleListRefresh (which owns the spinner), failure just toasts. Removes the race class by construction rather than adding another guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…earlier handleListRefresh closed over pagination/search/sort, so a refresh captured in a pending mutation's .then (e.g. a delete) would refetch the stale page/search/order and overwrite the view the user had since navigated to. Make it a stable callback that reads the latest params from a ref, so post-mutation refresh always targets the current view. Fixes it for every list's create/edit/import/delete. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pipelines and API deployments still passed the removed `fetchData` option, so the hook's `fetchRef` stayed null and their pagination and search were silent no-ops. Both now assign `fetchRef` directly and drop their local `fetchListRef`. Route every fetch (navigation, last-page stepback, adapter-type reset) through `requestList`, so the recorded request params always match what lands on screen and a post-mutation refresh replays the view the user actually asked for. Realign those params with the displayed view when the newest request fails, so a failed navigation can't leave a later refresh jumping to a page that never loaded. Give the workflow edit modal its own loading flag so saving no longer drives the shared list spinner. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kirtimanmishrazipstack
left a comment
kirtimanmishrazipstack
left a comment
There was a problem hiding this comment.
Second-pass review (self-review). Pass-1 findings are both closed — the is_owner fix at ResourceTable.jsx:190 holds and preserves the creator-sees-"Me" case. CI is fully green here.
This pass focused on the 5 commits reworking the list fetch state machine, plus a fresh look at the owner axis. 3 High + 3 Medium, all verified against the source rather than inferred from the diff.
- High 1 — the OSS "Owned By" column can name someone who is no longer an owner
- High 2 —
loadErroris unreachable after the first successful load, on all 5 pages - High 3 — the Workflows list-fetch catch reports nothing (compounds High 2 into total silence)
- Medium 1 —
syncRequestedcan't undo a failed search/sort; its comment claims otherwise - Medium 2 —
requestListvoids the stepback contractapplyPagedResponsedocuments - Medium 3 — stale rationale on the
_prompt_countSubquery
Also noted, not blocking: the ?order_by param on /workflow/ is now silently ignored (main honoured it for modified_at). No frontend senders remain, so it just needs a line in the breaking-change section rather than a code change.
Separately — the .distinct() calls in all five for_user() managers are no-ops (the sharing helpers return ValuesQuerySet subqueries used as IN, which can't multiply outer rows; they're leftovers from the pre-ResourceMembership join era). That does not make the pk__in re-wrap wrong — while those .distinct() calls stand, Postgres still requires ORDER BY to lead with the DISTINCT ON expression, so the re-wrap is load-bearing exactly as written. Worth a follow-up ticket that deletes the .distinct() calls, the re-wrap, both hint params, and the _prompt_count Subquery together.
| }; | ||
|
|
||
| const renderOwner = (item) => { | ||
| const email = item?.[ownerEmailProp]; |
There was a problem hiding this comment.
High — the "Owned By" column can name someone who is not an owner.
All four OSS pages pass ownerEmailProp="created_by_email" (Workflows.jsx:442, ToolSettings.jsx:401, ListOfTools.jsx:495, ConnectorsPage.jsx:338), and OWNER_SORT_FIELD = "created_by__email" keys the owner sort off the same column.
Repro: Alice creates workflow W → adds Bob as co-owner → Bob removes Alice. That removal is permitted — RemoveOwnerSerializer only requires another live owner (membership_serializers.py:92), and the co-owner modal renders the remove button once totalOwners > 1. Memberships are now {Bob: OWNER} but created_by is still Alice. The row renders "alice / alice@x.com" with no +N, to the whole org — and Alice no longer appears in for_user() for W at all. ?sort_by=owner sorts by that same wrong column.
The cloud side already solved this, and its docstring states the rule we're breaking here — agentic_studio_v1/serializers.py:223:
created_byis audit-only (UN-2202) and the creator can be removed as owner, so it must not be used to name the owner.
Cloud has get_owner_email() (earliest live OWNER, service accounts excluded) and passes ownerEmailProp="owner_email".
Suggested fix: mirror that method onto AdapterListSerializer / CustomToolListSerializer / ConnectorInstanceSerializer / WorkflowSerializer and switch the four ownerEmailProp values. The memberships__user prefetch is already attached in all four get_querysets, so it stays query-free. Pointing OWNER_SORT_FIELD at the same roster would also close the sort/display divergence already documented on the cloud PR.
There was a problem hiding this comment.
Fixed in db133f5. Added owner_email() to HasMembersMixin (earliest live OWNER, service accounts excluded — mirrors the cloud serializer), emitted it on all four list serializers, and switched the four ownerEmailProp values to owner_email. Query-free since the memberships__user prefetch is already attached in every get_queryset. Left OWNER_SORT_FIELD on created_by__email — the sort/display divergence stays the documented follow-up, same as cloud.
| isClickable={false} | ||
| /> | ||
| {displayList === undefined && !loadError && <SpinnerLoader />} | ||
| {displayList === undefined && loadError && ( |
There was a problem hiding this comment.
High — loadError is unreachable after the first successful load.
This gate gets displayList === undefined, which is only true before the first response lands. After that, setLoadError(true) writes state that no branch can render, and the page falls through to a length-based branch that asserts something false.
Same gating in Workflows.jsx:408, ToolSettings.jsx:370, ListOfTools.jsx:464, and cloud Projects.jsx:320.
Repro on ToolSettings: open /settings/llm with zero adapters (first fetch succeeds → displayList = []), type gpt in the search → setSearchTerm("gpt") commits immediately, the request 500s. The catch sets loadError (unrenderable) and the screen renders "No results found for this search" — a claim that was never established. The toast is transient; the on-screen state persists and is wrong.
Worth calling out that the commit adding this was titled "Show a retryable error on list-fetch failure" — as gated it only ever fires on cold-start failure, the one case users already handle by refreshing.
Suggested shape — hoist the error above the length branches and negate it on the rest:
{loadError && <EmptyState text="Couldn't load. Please try again." btnText="Retry" handleClick={handleListRefresh} />}
{!loadError && displayList === undefined && <SpinnerLoader />}
{!loadError && displayList?.length === 0 && !searchTerm && (/* onboarding */)}
{!loadError && displayList?.length === 0 && searchTerm && (/* no results */)}
{!loadError && displayList?.length > 0 && (/* table */)}There was a problem hiding this comment.
Fixed in db133f5 (OSS) and a600eed3 (cloud). Hoisted the loadError branch ahead of the length branches and gated the spinner/length branches on !loadError across all four OSS pages and cloud Projects.jsx, so a failure after the first successful load shows the retry state instead of a false "No results" claim.
| console.error("Unable to get project list"); | ||
| // Avoid an indefinite spinner when the first fetch fails. | ||
| setProjectList((prev) => prev ?? []); | ||
| // Surface a retryable error instead of a misleading empty state. |
There was a problem hiding this comment.
High — this is the only list-fetch catch of the five that reports nothing to the user or to Sentry.
Line 131 is .catch(() => { with no err parameter, and line 136 is a bare console.error("Unable to get project list") — no error object, no HTTP status, no URL, no page number.
The other four all surface it: ToolSettings.jsx:170 and ListOfTools.jsx:187 call setAlertDetails(handleException(err)), ConnectorsPage.jsx:136 calls setAlertDetails(handleException(err, "Failed to load connectors")), cloud Projects.jsx:141 calls showError(err, "Failed to load projects"). handleException is already imported here (line 46).
On its own this is minor. Combined with the loadError gating issue it means a Workflows list failure is completely invisible — no toast, no error state, and the pager silently snapping back to its previous value because setPagination never ran. Clicking page 2 on a 500 looks exactly like the app ignoring the click.
.catch((err) => {
if (seq !== seqRef.current) return;
setAlertDetails(handleException(err, "Unable to load workflows"));
setLoadError(true);
syncRequested();
})There was a problem hiding this comment.
Fixed in db133f5. The catch now takes err and calls setAlertDetails(handleException(err, "Unable to load workflows")), matching the other four pages; it keeps setLoadError(true) and syncRequested().
| const fetchRef = useRef(null); | ||
|
|
||
| // Mirrors the on-screen view each render (pagination only advances when a | ||
| // response applies), so a FAILED request can realign requestedRef with what's |
There was a problem hiding this comment.
Medium — this comment is false for three of the five fields, and syncRequested depends on it being true.
"Mirrors the on-screen view each render" holds for page/pageSize — setPagination is only ever called from applyPagedResponse, so those genuinely advance at apply time. It does not hold for search/sortBy/order: handleSearch (line 145) and handleSortChange (line 153) commit setSearchTerm/setSort before firing the request, and neither rolls back on failure.
So appliedRef is a hybrid of last-applied pagination and last-requested controls, and syncRequested copies that hybrid into requestedRef.
Consequence — on the two axes users touch most, syncRequested writes back the failed request byte-for-byte and its stated purpose ("stops the next handleListRefresh from replaying the failed target") isn't achieved. Click Owned By → Z-A, request 500s, syncRequested() records {sortBy: "owner", order: "desc"} — identical to what just failed. Delete a row: success toast fires, handleListRefresh() replays the same failing request, a second error toast appears, and the deleted row is still on screen because setList never ran.
Mixed-axis case: on page 3, type zzz, request fails → requestedRef = {page: 3, search: "zzz"}, a view that was neither requested (page 1) nor displayed (page 3, unsearched). It self-heals via the stepback cascade 3→2→1, but that's three round trips and two list flashes.
Either snapshot searchTerm/sort before the request and restore them in the page's catch, or record params in the success path where setList/setPagination actually commit. At minimum the comment needs to stop claiming an invariant the next maintainer will rely on.
There was a problem hiding this comment.
Fixed in db133f5 — comment only. Rewrote it to say page/pageSize mirror the applied view while search/sortBy/order are the last-requested values (committed before the request fires, not rolled back on failure). The behavior self-heals, so a snapshot/rollback redesign is left as a follow-up rather than expanding this PR; the comment no longer claims an invariant that does not hold.
| // Single fetch entry: records the requested view before firing, so every | ||
| // path (navigation, stepback, reset) keeps requestedRef in sync with what | ||
| // ends up on screen. Pages route their stepback (refetchPrevPage) through it. | ||
| const requestList = (page, pageSize, search, sortBy, order) => { |
There was a problem hiding this comment.
Medium — no return here, which voids the contract applyPagedResponse documents.
applyPagedResponse's JSDoc (lines 33-34, 44-45) says it steps back "returning that refetch promise so the caller's finally waits for replacement data instead of clearing loading early", and declares @return {Promise|undefined}.
All five callers now pass refetchPrevPage: () => requestList(page - 1, …), and requestList discards the value of fetchRef.current?.(…). So applyPagedResponse returns undefined on every path, the .then adopts nothing, and the Promise arm of the @return tag is unreachable.
Nothing breaks today — but only by accident. The stepback runs ++seqRef.current synchronously inside the .then, so the outer .finally sees seq !== seqRef.current and skips setLoading(false). The seq guard is the entire mechanism; the documented one doesn't exist.
That's the hazard: someone reading this JSDoc reasonably concludes the promise chain holds loading and that if (seq === seqRef.current) in .finally is redundant belt-and-braces. Removing it reintroduces exactly the early-clear bug these commits fixed. The doc also invites await applyPagedResponse(...) expecting the stepback data to have landed.
Either return fetchRef.current?.(page, pageSize, search, sortBy, order); — one word, and the doc becomes true — or delete both doc sentences and the @return tag so the seq guard is documented as the mechanism it actually is.
There was a problem hiding this comment.
Fixed in db133f5. requestList now returns fetchRef.current?.(...), so a stepback promise propagates through applyPagedResponse as its JSDoc documents. The seq guard remains the actual loading-hold mechanism (belt-and-braces, not redundant).
| select_related=("created_by",), | ||
| prefetch_related=("memberships__user",), | ||
| ) | ||
| # The pk__in re-wrap drops annotations, so re-apply prompt_count on |
There was a problem hiding this comment.
Medium — both halves of this rationale are stale. The code is correct; the stated reasons aren't, which will mislead anyone deciding whether the Subquery can be simplified away.
-
"re-apply prompt_count" — nothing was annotated before the re-wrap.
qsat line 161 isCustomTool.objects.for_user(...).prefetch_related("memberships__user")only. The annotation is applied here for the first time, not re-applied. (Pre-refactor the.annotate()did come first; the comment survived the reordering.) -
"Subquery avoids conflict with the
distinct("tool_id")thatfor_user()carries" — the queryset being annotated here has noDISTINCT ON.apply_search_and_sortreturns a freshCustomTool.objects.filter(pk__in=…)chain; thedistinct("tool_id")lives only on the inner.values("pk")subquery, which is never annotated.for_user()also returnsself.all()for service accounts and org admins, with no distinct at all.
Something like:
# apply_search_and_sort returns a fresh CustomTool.objects chain, so the
# annotation goes on afterwards. Subquery keeps the count out of the outer
# GROUP BY, which would otherwise collide with the sort column.There was a problem hiding this comment.
Fixed in db133f5. Rewrote the comment: apply_search_and_sort returns a fresh CustomTool.objects chain, so the prompt_count annotation is applied afterwards (not re-applied), and the Subquery keeps the count out of the outer GROUP BY — not a DISTINCT ON conflict, which the re-wrapped queryset does not carry.
- Owned By names a live owner: owner_email() on HasMembersMixin + 4 list serializers, instead of created_by which can be a removed creator. - Retryable load error is reachable after the first load (gate loadError ahead of the length branches on all 4 pages). - Workflows list-fetch failure surfaces via handleException, not a bare console.error. - Correct usePaginatedList appliedRef comment; requestList returns the fetch promise so applyPagedResponse's documented stepback holds. - Fix stale prompt_count Subquery rationale comment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…resource-list-views
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
|
Unstract test resultsPer-group results
Critical paths
|
1 participant
What
ResourceTable(Name / Owned By / Created Date / Actions) replacesListView/ViewToolsacross Adapters, Workflows, Prompt Studio and Connectors.apply_search_and_sortbackend helper drives server-side per-column sort, owner-inclusive search and pagination for all four list endpoints.Why
How
backend/utils/list_query.py: apk__inre-wrap lifts the PostgresDISTINCT ONeachfor_user()manager carries so any column is orderable; Prompt Studio re-applies itsprompt_countannotation after the re-wrap.ResourceTableuses custom sort-dropdown headers;usePaginatedListgains sort state and pages fetch?sort_by/?order/?searchserver-side.ListViewandViewTools(0 remaining consumers).Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
for_user()-filtered. Endpoints keep the bare-array response unless?page/?page_sizeis sent, so existing API callers are unaffected.Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
Screenshots
Add before merge.
Checklist
I have read and understood the Contribution Guidelines.