UN-3770 [MISC] Make list pagination consistent across shared resource endpoints - #2208
Conversation
…endpoints #2187 added opt-in pagination to workflows, prompt studio, adapters and connectors but only wired the Workflows page. The other three could not follow: their `for_user()` managers used `DISTINCT ON`, which forces Postgres to order by the distinct expression, so those viewsets were pinned to `order_by("id")` and could not order by `modified_at`. Backend - Swap `.distinct("id")` / `.distinct("tool_id")` for plain `.distinct()` in the adapter, connector and prompt studio managers. Every arm of the sharing predicate is a PK subquery, not a join, so no duplicate rows exist to collapse and the swap is behaviour-preserving. Workflows has shipped this way since #1462. - Replace the per-view `order_by()` calls with a declarative `ordering = ["-modified_at", "pk"]`. `OrderingFilter` is already in `DEFAULT_FILTER_BACKENDS`, so this needs no `filter_backends` override (which would drop `OrganizationFilterBackend`). - Drop Workflow's `?order_by=asc|desc`; it has no consumer, and `?ordering=` now covers it through the standard filter. Frontend - Add `unwrapList` / `fetchAllPages` helpers. Selectors page to exhaustion rather than silently showing only the first 50 rows. - Route all adapter, connector and workflow selectors through them. - Convert the Prompt Studio, adapters and connectors pages to server-side pagination and search via `usePaginatedList`, replacing the client-side `useListSearch` filter (now deleted). - Move `<Pagination>` into `ViewTools` so all four pages share one implementation: page size 10, size changer on, `["10","20","50"]`. Workflows had the changer disabled; that inconsistency goes away. - Stop assigning `fetchListRef.current` during render in Workflows. Endpoints stay opt-in paginated here, so every change is safe against both response shapes. Flipping them to unconditional `CustomPagination` is a separate change, after this has been validated on staging. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018HXYFVCZGi7YN9qjDGyJCQ
|
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 PR adds deterministic backend ordering, queryset deduplication, owner metadata, shared frontend pagination utilities, paginated list state, and a reusable sortable resource table. Resource listings and related adapter, connector, workflow, deployment, and pipeline fetches use these updated flows. ChangesBackend list contracts
Frontend pagination and resource listings
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ResourceTable
participant usePaginatedList
participant ListAPI
User->>ResourceTable: select search, sort, or page
ResourceTable->>usePaginatedList: submit list parameters
usePaginatedList->>ListAPI: GET paginated resource list
ListAPI-->>usePaginatedList: results and count
usePaginatedList-->>ResourceTable: update rows and pagination
ResourceTable-->>User: render sorted resource table
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
|---|---|
| backend/utils/filters/ordering_filter.py | Adds a deterministic primary-key tie-breaker while preserving views that have no effective ordering. |
| backend/backend/settings/base.py | Enables deterministic ordering globally so paginated views relying on default filter backends receive the tie-breaker. |
| backend/workflow_manager/execution/views/execution.py | Replaces the execution viewset’s explicit plain ordering backend with the deterministic implementation. |
| frontend/src/hooks/usePaginatedList.js | Centralizes paginated request parameters, stale-response suppression, sorting, refreshing, and empty-page stepback behavior. |
| frontend/src/components/tool-settings/tool-settings/ToolSettings.jsx | Migrates adapter lists to server pagination and guards success, error, and loading updates against superseded requests. |
| frontend/src/components/pipelines-or-deployments/pipelines/Pipelines.jsx | Adds request sequencing and shared paginated-response handling to the pipeline list. |
| frontend/src/components/deployments/api-deployment/ApiDeployment.jsx | Adds request sequencing and shared paginated-response handling to API deployment lists. |
Reviews (19): Last reviewed commit: "UN-3770 [FIX] Route remaining paginated ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/src/components/tool-settings/tool-settings/ToolSettings.jsx (1)
145-177: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSame unreturned-promise "step back a page" issue as
Workflows.jsx/ListOfTools.jsx.
getAdaptersdoesn't return itsaxiosPrivate({...}).then()...chain, so the recursive call at line 165 also isn't returned; the outer.finallyclearsisLoadingbefore the recursive fetch resolves. See consolidated comment.🤖 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/tool-settings/tool-settings/ToolSettings.jsx` around lines 145 - 177, Update getAdapters to return the axiosPrivate promise chain, including the recursive getAdapters call when stepping back from an empty page, so the outer finally runs only after the replacement fetch resolves.frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx (1)
157-195: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSame unreturned-promise "step back a page" issue as
Workflows.jsx.
getListOfToolsdoesn't return theaxiosPrivate({...}).then()...chain, so the recursive call at line 181 also isn't returned;setIsListLoading(false)in the outer.finallyfires before the recursive fetch resolves. See consolidated comment.🤖 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 157 - 195, Update getListOfTools to return the axiosPrivate promise chain, and return the recursive getListOfTools call when stepping back from an empty page. This ensures the outer finally waits for the replacement-page request before clearing isListLoading.frontend/src/components/workflows/workflow/Workflows.jsx (1)
115-151: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winLoading flag clears before the "step back a page" recursion finishes.
getProjectListnever returns itsprojectApiService.getProjectList(params).then()...chain, and the recursive call at line 132 isn't returned either. The outer.finally(() => setLoading(false))therefore resolves immediately after triggering the recursive fetch, not after it completes —loadingbriefly goesfalsewhile stale/empty data is still on screen. See consolidated comment for the shared fix across files.🤖 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/workflows/workflow/Workflows.jsx` around lines 115 - 151, Return the projectApiService.getProjectList(params) promise chain from getProjectList, and return the recursive getProjectList(page - 1, pageSize, search) call in the empty-page branch so the outer finally runs only after the replacement fetch completes. Preserve the existing pagination and loading behavior otherwise.
🧹 Nitpick comments (1)
frontend/src/components/workflows/workflow/workflow-service.js (1)
14-19: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPass an explicit
page_sizeon the full list fetch.
WorkflowViewSet.listmaps to/workflow/withOptionalPagination, whose default is 50, andfetchAllPageskeeps requestingpageuntil the result set is complete. Since this selector is mounted fromEtlTaskDeployon app deployment, a largerpage_sizesuch as100is appropriate for this dropdown.💡 Suggested tweak
getWorkflowList: () => fetchAllPages(axiosPrivate, { url: `${path}/workflow/`, - params: { is_active: "True" }, + params: { is_active: "True", page_size: 100 }, }),🤖 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/workflows/workflow/workflow-service.js` around lines 14 - 19, Update getWorkflowList to include an explicit page_size of 100 in the params passed to fetchAllPages, while preserving the existing is_active filter and 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 `@backend/adapter_processor_v2/views.py`:
- Around line 151-153: Ensure client-supplied ordering retains a deterministic
pk tie-breaker before pagination: update the ordering configuration/filter
behavior at backend/adapter_processor_v2/views.py lines 151-153,
backend/connector_v2/views.py lines 50-52,
backend/prompt_studio/prompt_studio_core_v2/views.py lines 136-138, and
backend/workflow_manager/workflow_v2/views.py lines 80-82 so selected ordering
always appends pk while preserving direction and avoiding duplicate pk entries.
In `@backend/utils/tests/test_list_pagination.py`:
- Around line 126-149: Update test_pages_partition_the_result_set to assign
controlled modified_at values and assert each page’s result sequence follows the
declared -modified_at, pk ordering, not only set membership. Add coverage for
ordering=modified_at using tied timestamps, verifying the returned sequence and
deterministic pk tie-breaker across pages.
In `@frontend/src/pages/ConnectorsPage.jsx`:
- Around line 113-138: Update the recursive fallback in fetchConnectors so it
awaits the fetchConnectors(page - 1, pageSize, search) call before returning,
ensuring the surrounding finally block keeps loading active until the recursive
request completes.
---
Outside diff comments:
In `@frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx`:
- Around line 157-195: Update getListOfTools to return the axiosPrivate promise
chain, and return the recursive getListOfTools call when stepping back from an
empty page. This ensures the outer finally waits for the replacement-page
request before clearing isListLoading.
In `@frontend/src/components/tool-settings/tool-settings/ToolSettings.jsx`:
- Around line 145-177: Update getAdapters to return the axiosPrivate promise
chain, including the recursive getAdapters call when stepping back from an empty
page, so the outer finally runs only after the replacement fetch resolves.
In `@frontend/src/components/workflows/workflow/Workflows.jsx`:
- Around line 115-151: Return the projectApiService.getProjectList(params)
promise chain from getProjectList, and return the recursive getProjectList(page
- 1, pageSize, search) call in the empty-page branch so the outer finally runs
only after the replacement fetch completes. Preserve the existing pagination and
loading behavior otherwise.
---
Nitpick comments:
In `@frontend/src/components/workflows/workflow/workflow-service.js`:
- Around line 14-19: Update getWorkflowList to include an explicit page_size of
100 in the params passed to fetchAllPages, while preserving the existing
is_active filter and 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: 49d33f54-a099-4e50-8afb-254c46c4760d
📒 Files selected for processing (26)
backend/adapter_processor_v2/models.pybackend/adapter_processor_v2/views.pybackend/connector_v2/models.pybackend/connector_v2/views.pybackend/prompt_studio/prompt_studio_core_v2/models.pybackend/prompt_studio/prompt_studio_core_v2/views.pybackend/utils/tests/test_list_pagination.pybackend/workflow_manager/workflow_v2/views.pyfrontend/src/components/agency/configure-connector-modal/ConfigureConnectorModal.jsxfrontend/src/components/custom-tools/adapter-selection-modal/AdapterSelectionModal.jsxfrontend/src/components/custom-tools/add-llm-profile/AddLlmProfile.jsxfrontend/src/components/custom-tools/combined-output/CombinedOutput.jsxfrontend/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/helpers/custom-tools/CustomToolsHelper.jsfrontend/src/components/pipelines-or-deployments/etl-task-deploy/EtlTaskDeploy.jsxfrontend/src/components/settings/default-triad/DefaultTriad.jsxfrontend/src/components/tool-settings/tool-settings/ToolSettings.jsxfrontend/src/components/workflows/workflow/Workflows.cssfrontend/src/components/workflows/workflow/Workflows.jsxfrontend/src/components/workflows/workflow/workflow-service.jsfrontend/src/helpers/pagination.jsfrontend/src/helpers/pagination.test.jsfrontend/src/hooks/useListSearch.jsfrontend/src/pages/ConnectorsPage.jsx
💤 Files with no reviewable changes (2)
- frontend/src/hooks/useListSearch.js
- frontend/src/components/workflows/workflow/Workflows.css
…ed list hook Backend: - DeterministicOrderingFilter appends `pk` to whatever ordering is in effect. `?ordering=` replaces the view's `ordering` outright, so the tie-breaker has to be applied by the filter rather than declared on the view. Swapped into DEFAULT_FILTER_BACKENDS; a no-op for views that declare no ordering and receive no `?ordering=`. - Tests assert the returned sequence rather than set membership, and pin the tie-breaker with rows sharing one `modified_at`. Both fail without the filter. Frontend: - usePaginatedResource owns the request for all four list pages: params, unwrapping, the empty-page step back, and the loading flag. Replaces four copy-pasted fetch functions (the Sonar duplication) and the fetchListRef indirection. - Only the newest request may write state, so a slow response can no longer restore the previous page, search term or adapter type. - The loading flag is held by the superseding request, so it no longer clears while the step-back page is still in flight. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018HXYFVCZGi7YN9qjDGyJCQ
Review round addressed in
|
| Finding | Fix |
|---|---|
?ordering= drops the pk tie-breaker (Greptile P2 / CodeRabbit major, 4 files) |
DeterministicOrderingFilter appends pk in get_ordering, installed via DEFAULT_FILTER_BACKENDS |
| Test asserts page membership, not ordering | Sequence assertions + a tied-timestamp ?ordering= test |
| Stale responses overwrite active lists (Greptile P1) | Request-id guard in the new shared hook |
| Loading flag clears before the step-back page lands (CodeRabbit, 4 files) | Same hook — the superseding request owns the flag |
Rejected
Nitpick: pass page_size: 100 on getWorkflowList. This would make things worse. OptionalPagination engages only when page or page_size is present, so the current param-less call returns every workflow as a bare array in one request. Adding page_size switches pagination on and turns that single request into a 100-row paginated loop. Left as-is; fetchAllPages handles either shape and will start paging on its own once the CustomPagination flip lands.
Sonar — 7.4% duplication on new code
Valid, and the same root cause as the repeated frontend findings above: 22 duplicated lines each in ConnectorsPage.jsx and ListOfTools.jsx, from a fetch function that was copy-pasted across all four list pages.
Extracted into usePaginatedResource, which owns the request, the page/page_size/search params, response unwrapping, the empty-page step back, in-flight ordering and the loading flag. The four pages lose their fetchListRef indirection and drop 145 lines net. usePaginatedList stays for Pipelines.jsx / ApiDeployment.jsx, which are untouched by this PR.
Verification — backend suite 421 passed / 29 skipped / 36 subtests; frontend 28 tests, build clean. Both new backend tests confirmed to fail with the filter reverted, and the stale-response guard confirmed by mutation.
One note on scope: pipeline_v2/views.py:54 sets filter_backends = [OrderingFilter], which drops OrganizationFilterBackend and its org scoping. Pre-existing and unrelated to pagination, so not touched here — flagging it as worth a separate look.
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)
frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx (1)
171-184: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCreating a new item refreshes the current page instead of jumping to page 1, hiding it under newest-first ordering. Per the PR's deterministic
-modified_atordering, a newly created row always sorts first; refreshing whatever page the user happens to be on (instead of page 1) means the new item is invisible unless the user is already on page 1.
frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx#L171-L184: inhandleAddNewTool's success handler, only callhandleListRefresh()whenisEdit; otherwise callfetchPage(1, pagination.pageSize, searchTerm).frontend/src/components/tool-settings/tool-settings/ToolSettings.jsx#L140-L140:addNewItemshould branch on whethereditItemIdwas set at call time, refreshing on edit and jumping to page 1 on create.frontend/src/pages/ConnectorsPage.jsx#L214-L224:handleConnectorSavedshould branch oneditingConnector(captured before it's cleared) the same way.🤖 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 171 - 184, Update ListOfTools.jsx at lines 171-184 in handleAddNewTool’s success handler to call handleListRefresh() only for edits and call fetchPage(1, pagination.pageSize, searchTerm) for creates. Update ToolSettings.jsx at line 140 in addNewItem to branch using editItemId captured before it is cleared, refreshing edits and navigating to page 1 for creates. Update ConnectorsPage.jsx at lines 214-224 in handleConnectorSaved to branch on editingConnector captured before clearing it, using the same edit-refresh and create-page-1 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 `@backend/utils/tests/test_list_pagination.py`:
- Around line 163-198: Update test_client_ordering_keeps_pk_tiebreaker to create
at least two distinct modified_at groups with multiple rows tied within each
group. Build the expected names sorted by ascending modified_at and then
stringified pk, and keep the paginated ordering=modified_at request asserting
that the combined page results match this sequence.
In `@frontend/src/components/tool-settings/tool-settings/ToolSettings.jsx`:
- Around line 90-111: The shared loading state is cleared before asynchronous
list refreshes finish. In
frontend/src/components/tool-settings/tool-settings/ToolSettings.jsx lines
90-111, update handleDelete to remove the manual setIsLoading toggle and outer
finally, allowing handleListRefresh/fetchPage to own loading; in
frontend/src/components/workflows/workflow/Workflows.jsx lines 65-78, update
editProject so setLoading(false) is tied to the branch's completed work,
including the awaited handleListRefresh path, rather than the outer finally.
In `@frontend/src/components/workflows/workflow/Workflows.jsx`:
- Around line 65-78: Update the usePaginatedResource configuration in
Workflows.jsx so its onError handler reports failures through
setAlertDetails(handleException(...)), matching the error handling used by the
other converted list pages instead of only calling console.error. Preserve the
existing project-list request and loading behavior.
---
Outside diff comments:
In `@frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx`:
- Around line 171-184: Update ListOfTools.jsx at lines 171-184 in
handleAddNewTool’s success handler to call handleListRefresh() only for edits
and call fetchPage(1, pagination.pageSize, searchTerm) for creates. Update
ToolSettings.jsx at line 140 in addNewItem to branch using editItemId captured
before it is cleared, refreshing edits and navigating to page 1 for creates.
Update ConnectorsPage.jsx at lines 214-224 in handleConnectorSaved to branch on
editingConnector captured before clearing it, using the same edit-refresh and
create-page-1 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: eb979305-f23c-4372-9bd4-2c3913bdb440
📒 Files selected for processing (9)
backend/backend/settings/base.pybackend/utils/filters/ordering_filter.pybackend/utils/tests/test_list_pagination.pyfrontend/src/components/custom-tools/list-of-tools/ListOfTools.jsxfrontend/src/components/tool-settings/tool-settings/ToolSettings.jsxfrontend/src/components/workflows/workflow/Workflows.jsxfrontend/src/hooks/usePaginatedResource.jsfrontend/src/hooks/usePaginatedResource.test.jsfrontend/src/pages/ConnectorsPage.jsx
chandrasekharan-zipstack
changed the title
…sions The Prompt Studio, adapters and connectors listing pages are being replaced wholesale by the ResourceTable in #2200. Converting them here to usePaginatedResource only to have that work overwritten created the entire conflict surface between the two PRs, plus a second pagination hook. Reverts the four listing-page conversions, the ViewTools pagination move and the useListSearch deletion, and drops usePaginatedResource. What remains is the part #2200 depends on and cannot do itself: the DISTINCT ON removal, declarative ordering with a pk tie-breaker, and the selector page-following that keeps dropdowns whole once pagination goes unconditional. usePaginatedResource.test.js goes with the hook; its coverage should be ported onto the surviving usePaginatedList in #2200. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0159NvRHFywvkNGji8ECQqeV
…esource-list sort to ?ordering= Resolve the 4 backend views.py conflicts by taking #2208's declarative ordering (?ordering= + DeterministicOrderingFilter) and folding owner sort/search back in (created_by__email in ordering_fields + name/owner search). Delete utils/list_query.py; frontend sort emits ?ordering=. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* UN-3769 [FEAT] Sortable resource list table with server-side sort, search & 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> * UN-3769 [FIX] Dedupe list fetch into shared helpers; fix stale-response 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> * UN-3769 [FIX] Collapse duplicated list-page preamble to clear duplication 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> * UN-3769 [FIX] Gate list-fetch catch/finally on the request sequence 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> * UN-3769 [FIX] Show a retryable error on list-fetch failure; use codePointAt - 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> * UN-3769 [FIX] Track "Me" in the Owned By cell by displayed owner, not 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> * UN-3769 [FIX] Gate delete-failure loading clear on the request sequence 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> * UN-3769 [FIX] Keep adapter delete out of the shared list-loading state 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> * UN-3769 [FIX] Refresh the current list view, not the params captured 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> * UN-3769 [FIX] Fix list fetch state handling and dead pagination 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> * UN-3769 [FIX] Address self-review on resource list views - 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> * UN-3769 [FIX] Match resource-table owner avatars to Figma pastel palette Swap the saturated avatar swatches for light pastel fills paired with a darker same-hue initial, matching the design. Applies to all resource list views via the shared ResourceTable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3769 [FIX] Lighten resource-table owner-avatar initials per design Lighten avatar initial color (Ant -7 -> -6) and reduce initial size (12px -> 11px) per design review feedback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3769 [FEAT] Resource list: add Modified column + PS prompt count, default modified-desc sort Add a sortable Modified column and rename Created Date -> Created so both dates are visible. Surface the already-serialized prompt_count as "Prompts: N" on the Prompt Studio list. Default all resource lists to modified-desc so the visible Modified column matches the sort (restores #2187 Workflows ordering). Frontend-only; backend already served both dates and prompt_count. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3769 [FEAT] Resource list: relative Modified time, canonical date format, owner search-only Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3769 [FIX] Resource list review fixes: name-only search, clear-sort restores default Address Chandru's re-review on #2200: - Owned By is display-only: drop created_by__email from ordering_fields and the search Q-filter across the 4 viewsets so search matches the shown owner (name-only) instead of the creator, and no dead owner-sort surface remains. - usePaginatedList: "Clear Sort" restores the default ordering (not an empty one) and list mounts request the seeded sort, so the header and rows agree. - Remove dead code: orphaned useListSearch.js, the dead avatar-initials branch, and the unreferenced .listWrapper rule; trim over-narrated comments. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3769 [FIX] Tests: pin name-only search, dropped owner-ordering fallback, owner_email Cover the review changes in the shared list-pagination contract test: - ?search= matches the resource name only, not the owner's email. - ?ordering=created_by__email is a dropped field -> ignored, list stays newest-first (had it survived, same-creator rows would be pk-ordered). - owner_email() names the earliest live OWNER, skips service accounts, None with no owner (shared mixin, pinned once). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/utils/tests/test_list_pagination.py (1)
286-304: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover equal-timestamp owner ties.
This regression uses distinct membership timestamps, so it cannot catch nondeterministic selection when two live OWNER memberships have the same
created_at. Add a tied-timestamp case with an asserted secondary-key result.🤖 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/utils/tests/test_list_pagination.py` around lines 286 - 304, The test_owner_email_is_earliest_live_owner regression should also cover multiple live OWNER memberships sharing the same created_at. Add a tied-timestamp setup and assert owner_email() selects the owner determined by the implementation’s secondary ordering key, while preserving the existing service-account filtering and no-owner assertions.frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx (1)
466-513: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated list-state render ladder across both migrated pages. Each page reimplements the identical error → spinner → empty → search-empty →
ResourceTablebranching; the shared root cause is that no wrapper encapsulatesloadError/displayList/searchTermstate rendering alongsideResourceTable.
frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx#L466-L513: replace the four conditional branches with a shared state wrapper, passing the prompt-project empty-state text/button.frontend/src/components/tool-settings/tool-settings/ToolSettings.jsx#L374-L414: use the same wrapper, passing the adapter-specific empty-state text/button.🤖 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 466 - 513, The duplicated conditional rendering around ResourceTable should be centralized in a shared state wrapper. In frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx lines 466-513, replace the loadError, loading, empty-list, search-empty, and ResourceTable branches with the wrapper, passing the prompt-project empty-state text and New Project action; apply the same wrapper in frontend/src/components/tool-settings/tool-settings/ToolSettings.jsx lines 374-414, passing that page’s adapter-specific empty-state text and action.
🤖 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/permissions/models.py`:
- Around line 46-57: The owner_email method in backend/permissions/models.py
should make selection deterministic by choosing the minimum live non-service
OWNER using both created_at and pk as the sort key. Add a tie-stress test in
backend/utils/tests/test_list_pagination.py covering multiple eligible owners
with identical created_at values and asserting the owner selected by the
secondary key.
In `@frontend/src/components/widgets/resource-table/ResourceTable.jsx`:
- Around line 254-281: Update the row action controls around EditOutlined,
ShareAltOutlined, and DeleteOutlined to use native buttons or Ant Design Button
type="text" wrappers, preserving their existing handlers, disabled styling, and
Popconfirm behavior. Ensure each action is keyboard focusable and activatable,
with Popconfirm receiving the focusable delete trigger.
In `@frontend/src/hooks/usePaginatedList.js`:
- Around line 62-63: Normalize the payload used by the paginated list before
deriving state: in the results/total handling around setList, extract
data?.results when present, then use Array.isArray to retain only array values
and fall back to an empty array for strings, objects, or other invalid payloads.
Keep total based on the normalized results length when no count is provided.
---
Nitpick comments:
In `@backend/utils/tests/test_list_pagination.py`:
- Around line 286-304: The test_owner_email_is_earliest_live_owner regression
should also cover multiple live OWNER memberships sharing the same created_at.
Add a tied-timestamp setup and assert owner_email() selects the owner determined
by the implementation’s secondary ordering key, while preserving the existing
service-account filtering and no-owner assertions.
In `@frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx`:
- Around line 466-513: The duplicated conditional rendering around ResourceTable
should be centralized in a shared state wrapper. In
frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx lines
466-513, replace the loadError, loading, empty-list, search-empty, and
ResourceTable branches with the wrapper, passing the prompt-project empty-state
text and New Project action; apply the same wrapper in
frontend/src/components/tool-settings/tool-settings/ToolSettings.jsx lines
374-414, passing that page’s adapter-specific empty-state text and action.
🪄 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: 16d60120-8cc6-4e63-9f91-cf4a183ed09a
📒 Files selected for processing (25)
backend/adapter_processor_v2/serializers.pybackend/adapter_processor_v2/views.pybackend/connector_v2/serializers.pybackend/connector_v2/views.pybackend/permissions/models.pybackend/prompt_studio/prompt_studio_core_v2/serializers.pybackend/prompt_studio/prompt_studio_core_v2/views.pybackend/utils/tests/test_list_pagination.pybackend/workflow_manager/workflow_v2/serializers.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/deployments/api-deployment/ApiDeployment.jsxfrontend/src/components/pipelines-or-deployments/pipelines/Pipelines.jsxfrontend/src/components/tool-settings/tool-settings/ToolSettings.jsxfrontend/src/components/widgets/co-owner-management/CoOwnerModal.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.cssfrontend/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.jsx
- frontend/src/components/custom-tools/view-tools/ViewTools.css
- frontend/src/components/widgets/list-view/ListView.css
- frontend/src/components/custom-tools/view-tools/ViewTools.jsx
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/connector_v2/views.py
- backend/adapter_processor_v2/views.py
- backend/prompt_studio/prompt_studio_core_v2/views.py
- backend/workflow_manager/workflow_v2/views.py
kirtimanmishrazipstack
left a comment
kirtimanmishrazipstack
left a comment
There was a problem hiding this comment.
Reviewed the whole thing end-to-end (backend + frontend + tests). The pagination/ordering core is sound and the DISTINCT ON diagnosis is right — the archaeology in the description matches what's in the managers. Findings below are the high/medium ones only; I've left the nits out.
HIGH — the Owned By column drops the created_by_email fallback, and frictionless adapters stop reading "Unstract"
ResourceTable.jsx reads only ownerEmailProp, and every call site passes "owner_email":
const email = item?.[ownerEmailProp];
const name = isMe ? "Me" : email?.split("@")[0] || "Unknown";Two consequences.
1. Confirmed behaviour change on cloud frictionless adapters. adapter_processor_v2/serializers.py deliberately masks them:
if instance.is_friction_less:
rep["created_by_email"] = "Unstract"
else:
rep["created_by_email"] = instance.created_by.emailCloud creates those in onboarding_service.py with created_by=user, shared_to_org=True, and an OWNER membership for that same user. So today the old ListView showed "Unstract" to every other org member (and "Me" to the onboarding user). After this change the column reads owner_email(), which resolves to the onboarding user's real email — and because these adapters are org-shared, that email is now visible to the whole org. The "Unstract" masking is bypassed entirely.
2. Latent "Unknown". owner_email() returns None when there's no live non-service-account OWNER — a resource created through a platform API-key session (only OWNER is a service account, filtered out by the is_service_account check), or any row that predates the UN-2202 membership backfill. ResourceTable then renders the literal string "Unknown" with a "UN" avatar. The old ListView had an explicit fallback for exactly this and it's gone.
One line covers both:
const email = item?.[ownerEmailProp] ?? item?.created_by_email;If showing the real owner instead of "Unstract" is intentional, that's fine — but it should be a stated decision, because it changes what every cloud org sees on their preset adapters.
MEDIUM — the global filter-backend swap reaches two out-of-scope, high-volume endpoints
settings/base.py swaps DeterministicOrderingFilter into DEFAULT_FILTER_BACKENDS project-wide. I checked every viewset that declares ordering/ordering_fields. Five of them override filter_backends with DRF's own OrderingFilter and are unaffected (dashboard_metrics, execution, pipeline_v2, usage_v2, tags). Two don't, and inherit the new behaviour:
workflow_manager/file_execution/views.py—ordering = ["created_at"]→created_at, pkworkflow_manager/workflow_v2/execution_log_view.py—ordering = ["event_time"]→event_time, pk
Both use CustomPagination (always paginated), and they sit on the two highest-volume tables in the product. ExecutionLog has (wf_execution, event_time), (file_execution, event_time) and (execution_id, event_time) — none of which include the pk, so ORDER BY event_time, id can't be served by an ordered index scan and needs a Sort node on top.
To be clear, a tie-breaker on paginated logs is a correctness win — I'm not saying revert it. The ask is that it's a deliberate, measured change rather than a side effect: either EXPLAIN those two list endpoints at a realistic row count, or set filter_backends explicitly on the five resource viewsets and leave the global default alone. The description rules the latter out because it "would silently drop OrganizationFilterBackend" — it wouldn't if all three are listed. Either way the description should name these two endpoints, since nobody reading the PR title expects execution logs to be in scope.
MEDIUM — .distinct() is now provably a no-op, by the PR's own argument
Every arm of every for_user() is a PK subquery or a scalar field filter:
Q(pk__in=member_ids) | Q(shared_to_org=True) | Q(pk__in=group_shared_ids)and resources_visible_via_groups / resources_visible_via_memberships in tenant_account_v2/sharing_helpers.py both return a ValuesQuerySet subquery, not a materialised join. No join means no duplicate rows means nothing for .distinct() to collapse. The description makes exactly this argument and then keeps the clause — Postgres still executes SELECT DISTINCT over every selected column on every list query, including adapter_metadata_b / connector_metadata bytea.
Worth noting test_multi_predicate_share_yields_one_row passes with or without .distinct(), so it isn't pinning what its docstring claims it pins.
Either drop it, or keep it with a comment saying it's insurance against a future join arm — the one place a join could realistically reappear is the viewset-level filter_args, so that's what the comment should point at.
MEDIUM — nothing indexes the new default ordering
BaseModel.modified_at is DateTimeField(auto_now=True) with no db_index, and none of the four models declare Meta.indexes. Every list page now runs ORDER BY -modified_at, pk LIMIT/OFFSET; before it was ORDER BY id, straight off the pk index.
Org-scoped so it's cheap at today's row counts, but if this is becoming the default ordering for four resource types the matching change is a (organization, modified_at) index.
MEDIUM — fetchAllPages will page at 50 once the flip lands
helpers/pagination.js fires the first request with no page_size, so post-flip it inherits Pagination.PAGE_SIZE = 50. An org with 300 adapters is 6 sequential round-trips every time a selector opens — and AdapterSelectionModal fires four of these in parallel, so up to 24 requests to populate one modal.
Sending a large page_size on the first request collapses the common case to a single request and keeps the loop as the tail guard for anything past MAX_PAGE_SIZE. Inert today, so this is cheap to fix now and annoying to discover after the flip.
…ns, payload guard - owner_email(): break created_at ties by pk so the "Owned By" label is stable across requests; cover equal timestamps in the shared test. - ResourceTable row actions rendered as non-focusable icon spans; wrap in real buttons so edit/share/delete are keyboard reachable and Popconfirm gets a focusable trigger. - applyPagedResponse: guard non-array payloads (204 body, stray object) before they reach antd Table dataSource. - Client-ordering test: two modified_at groups so honored ascending order is distinguishable from the -modified_at default, not just the pk tie. - Drop redundant "# Name search." comments on the name-only search blocks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tip survives A native disabled button suppresses hover, hiding the "deprecated" tooltip. aria-disabled keeps the control focusable and hoverable; the onClick guard already no-ops when deprecated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Don't highlight the default sort column on load; the header lights up only once the user explicitly picks a sort. usePaginatedList tracks a `userSorted` flag threaded to ResourceTable/SortHeader. - Name column absorbs the slack while Owned By/Created/Modified/Actions share one compact fixed width, so they read as an evenly-spaced group and Name stays dominant. - Owner name/email ellipsize within their cell (drop the 190px cap, let the Space item shrink), removing the trailing-gap skew. - Table scrolls inside its own container below its min-width instead of crushing columns on narrow screens. - Created timestamp gets ellipsis+tooltip as a safety for long values. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018HXYFVCZGi7YN9qjDGyJCQ
Owner search matches the displayed owner — the OWNER membership that backs the Owned By column — via a search-time subquery (name OR owner email), across all four shared list endpoints. Reuses the sharing_helpers varchar/UUID object_id cast. `created_by` stays audit-only (UN-2202); service accounts and non-owner (VIEWER) members are excluded. This intentionally reverses the name-only narrowing from UN-3769: search now agrees with what the Owned By column shows, which was that change's goal. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018HXYFVCZGi7YN9qjDGyJCQ
The four owner-searchable list pages now advertise owner search via the placeholder; ToolNavBar's other consumers keep the "Search by name" default. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018HXYFVCZGi7YN9qjDGyJCQ
Adds owner_emails() (all live OWNER emails, earliest-first) on HasMembersMixin, exposed by the four list serializers. The Owned By cell still shows the primary owner + `+N` inline, but its tooltip now names every co-owner — so a search that matched a co-owner hidden behind `+N` is explainable on hover. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018HXYFVCZGi7YN9qjDGyJCQ
owner_email was owner_emails[0] — one derivable scalar of redundancy. Drop it from the four list serializers and read owner_emails[0] on the frontend instead (ResourceTable, and the cloud Projects card in the companion cloud PR). The model's owner_email() accessor stays for callers/tests that want just the head. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018HXYFVCZGi7YN9qjDGyJCQ
Revert the layout half of the earlier "sort affordance + layout" change: restore the proportional column widths (34/22/15/15/14%), the 190px owner name/email cap, and drop the min-width scroll container + Created ellipsis. The sort-affordance (userSorted) fix stays. Layout to be revisited later. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018HXYFVCZGi7YN9qjDGyJCQ
…#2221) UN-3770 [FEAT] ResourceTable: extraColumns + onRowClick override Let callers inject resource-specific columns (inserted before Actions) and override the default relative row-click nav — needed so the Prompt Studio lookups table can reuse this widget while keeping its Files/Latest Version columns and its absolute, stateful navigation. Claude-Session: https://claude.ai/code/session_018HXYFVCZGi7YN9qjDGyJCQ 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, at 8d05a7b13. The CodeRabbit round is genuinely fixed — (created_at, pk) tie-break on the owner label, the Array.isArray guard in applyPagedResponse, real <button> row actions with aria-disabled, and the two-group ordering test are all solid. The userSorted sort affordance is a nice catch too.
None of the five findings from my first pass have been touched, though, and the three new features added since then bring two new mediums. High + medium only again.
HIGH (still open) — the Owned By column has no created_by_email fallback
ResourceTable.jsx now reads:
const ownerEmails = item?.[ownerEmailsProp];
const email = Array.isArray(ownerEmails) ? ownerEmails[0] : undefined;
const name = isMe ? "Me" : email?.split("@")[0] || "Unknown";The owner_email → owner_emails refactor kept the behaviour and removed the last safety net: the prop default used to be ownerEmailProp = "created_by_email", and it is now ownerEmailsProp = "owner_emails". So both consequences from the first round stand unchanged.
- Cloud frictionless adapters.
AdapterListSerializer.to_representationstill deliberately masks them —if instance.is_friction_less: rep["created_by_email"] = "Unstract". Cloud'sonboarding_service.pycreates those withshared_to_org=Trueand an OWNER membership for the onboarding user, soowner_emails()[0]resolves to that person's real email and the whole org now sees it on their preset adapters instead of "Unstract". The mask is bypassed. - Literal "Unknown".
owner_emails()returns[]when there is no live non-service-account OWNER — a resource created through a platform API-key session, or any row predating the UN-2202 backfill. The cell renders the string "Unknown" with a "UN" avatar.
created_by_email is present in all four list payloads (workflow_v2/serializers.py:82, connector_v2/serializers.py:37, prompt_studio_core_v2/serializers.py:47, and the adapter one above), so it is still one line:
const email =
(Array.isArray(ownerEmails) ? ownerEmails[0] : undefined) ??
item?.created_by_email;If dropping the "Unstract" mask is the intent, that's a fine decision — but it needs to be a stated one, because it changes what every cloud org sees.
MEDIUM (new) — the description now contradicts the code
c1cc997f9 restored owner search, but the description still says the opposite in two places:
Owner search/sort dropped —
created_by__emailremoved fromordering_fieldsand from the name search. Search is name-only so the search box and the Owned By column agree.
and, under Tests, "name-only search". The test it refers to is now test_search_matches_name_and_owner_email.
The owner_email → owner_emails change isn't mentioned anywhere either, and that is a response-field rename on four list endpoints plus the agentic one in unstract-cloud#1673. Whatever else happens, that belongs in the description — it's the record a future reader uses to work out when the field changed shape.
MEDIUM (new) — owner search matches the email domain, so short queries return everything
resources_matching_owner_search filters on user__email__icontains=term. In a single-domain org that means the domain is part of every match:
?search=comatches@acme.comfor every user → the full list comes back- so do
com,acme,a,.c
And because the column only ever renders email.split("@")[0], the user sees an unfiltered list with nothing on screen explaining why. The tooltip mitigation only fires on hover and only when there is more than one owner.
Matching what's actually displayed — the local part — fixes it. user__email__istartswith=term is the smallest version; splitting on @ before the comparison is the exact one. Either keeps the search box honest about the column it claims to agree with.
MEDIUM (still open) — the global filter-backend swap reaches two out-of-scope endpoints
settings/base.py:630 is unchanged, and so are the two viewsets that inherit it without an override:
workflow_manager/file_execution/views.py:17-19—ordering = ["created_at"]→created_at, pkworkflow_manager/workflow_v2/execution_log_view.py:33-35—ordering = ["event_time"]→event_time, pk
Both use CustomPagination and sit on the two highest-volume tables in the product. ExecutionLog's indexes — (wf_execution, event_time), (file_execution, event_time), (execution_id, event_time) — don't include the pk, so ORDER BY event_time, id needs a Sort node on top.
Again: the tie-breaker is a correctness win on paginated logs, I'm not asking for a revert. The ask is that it be deliberate — either EXPLAIN those two at a realistic row count, or set filter_backends explicitly on the five resource viewsets. The description rules the latter out because it "would silently drop OrganizationFilterBackend", which isn't true if all three backends are listed. Either way the description should name the two endpoints; nobody reading this title expects execution logs in scope.
MEDIUM (still open) — .distinct() is a no-op, by the PR's own argument
Still present at adapter_processor_v2/models.py:62, connector_v2/models.py:54, prompt_studio_core_v2/models.py:50. Every arm of every for_user() is a PK subquery or a scalar filter, and the description says so itself — no join, no duplicate rows, nothing for .distinct() to collapse. Postgres still executes SELECT DISTINCT over every selected column on every list query, adapter_metadata_b / connector_metadata bytea included.
test_multi_predicate_share_yields_one_row passes with or without it, so it isn't pinning what its docstring says it pins.
Drop it, or keep it with a comment pointing at the one place a join could realistically reappear — the viewset-level filter_args.
MEDIUM (still open) — nothing indexes the new default ordering
No migration in the PR. BaseModel.modified_at is auto_now=True with no db_index, and none of the four models declare Meta.indexes. Every list page runs ORDER BY -modified_at, pk LIMIT/OFFSET; before it was ORDER BY id straight off the pk index. Org-scoped so it's cheap now, but if this is the default ordering for four resource types the matching change is a (organization, modified_at) index.
MEDIUM (still open) — fetchAllPages will page at 50 once the flip lands
helpers/pagination.js is unchanged: the first request goes out with no page_size, so post-flip it inherits Pagination.PAGE_SIZE = 50. An org with 300 adapters is 6 sequential round-trips every time a selector opens, and AdapterSelectionModal fires four of these in parallel — up to 24 requests for one modal. Sending a large page_size on the first request collapses the common case to one call and leaves the loop as the tail guard past MAX_PAGE_SIZE. Inert today, cheap now, annoying to discover after the flip.
Lows are held back again — happy to drop them in if you want the full list.
… index, prefix search - ResourceTable: fall back to created_by_email so rows with no live OWNER membership render the creator instead of "Unknown". - Adapter serializer: mask owner_emails to ["Unstract"] for frictionless adapters so the Owned By column keeps the org-wide mask. - Drop redundant .distinct() from adapter/connector/prompt_studio for_user (every arm is a PK subquery, no join, nothing to collapse). - Add (organization, -modified_at) index to the 4 resource models backing the default list ordering. - fetchAllPages: request MAX_PAGE_SIZE up front so the common case is one round-trip; the loop stays as the tail guard. - Pin plain OrderingFilter on the two high-volume execution-log endpoints so they don't inherit the pk tiebreaker (unindexed Sort) from the global default. - Owner search matches the email prefix (local part) so a bare domain fragment doesn't return every row in a single-domain org. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018HXYFVCZGi7YN9qjDGyJCQ
Review round 2 addressed in
|
| Finding | Fix |
|---|---|
HIGH — Owned By has no created_by_email fallback → "Unknown" |
ResourceTable now owner_emails[0] ?? created_by_email. Frictionless adapters keep the mask: the serializer masks owner_emails to ["Unstract"] too (not just created_by_email), so the org-wide mask is preserved rather than leaking the onboarding user's email. |
| MED (new) — description contradicts the code | Description updated: name-or-owner search, the owner_email → owner_emails rename, and the two execution-log endpoints named as in-scope. |
| MED (new) — owner search matches the domain, short queries return everything | resources_matching_owner_search now matches user__email__istartswith (the local part is the shown name), so co/com/acme no longer surface every row in a single-domain org. |
| MED — global filter-backend reaches 2 out-of-scope high-volume endpoints | file_execution and execution_log_view pin plain OrderingFilter explicitly (all three backends listed, so org-scoping stays), opting out of the pk tie-breaker so their (…, event_time) indexes still serve the sort. Kept the global default for the resource viewsets — incl. the cloud lookups/agentic ones that inherit it — to avoid dragging the scoping change into cloud. |
MED — .distinct() is a no-op |
Dropped from all three managers. test_multi_predicate_share_yields_one_row pins the dedup contract without it. |
| MED — nothing indexes the new default ordering | (organization, -modified_at) index added to all four resource models (4 migrations via makemigrations). |
MED — fetchAllPages pages at 50 post-flip |
First request sends page_size = MAX_PAGE_SIZE (1000); the loop stays as the tail guard past it. |
Verification — test_list_pagination.py 7 passed / 24 subtests (incl. the flipped name-and-owner search and the multi-predicate dedup with .distinct() gone), permissions 36 passed, ruff + ruff-format + biome clean on all changed files.
Lows welcome if you want to drop them in.
…st responses Two P1 findings from Greptile: - Execution-log endpoints (file_execution, execution_log_view) dropped their plain-OrderingFilter override and now inherit the global deterministic filter, so tied created_at/event_time rows can't repeat or omit across pages. Each request is already scoped to a single execution_id, so the pk tie-breaker sorts a narrow set, not the whole table. - Pipelines and ApiDeployment list fetches adopt the monotonic seq guard via applyPagedResponse, so a slow superseded response can no longer overwrite a newer search/page/type selection. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018HXYFVCZGi7YN9qjDGyJCQ
Addressed Greptile 3/5 findings (8d0f4bc)P1 — Timestamp ties destabilize pagination ( P1 — Stale responses overwrite active lists ( P2 — Custom ordering drops deterministic tie-breaker (
Lint green (ruff/ruff-format/pycln/biome); pagination contract 7/7 (24 subtests). |
…c ordering Greptile P1: ExecutionViewSet still bypassed the global DeterministicOrderingFilter with a plain OrderingFilter, so tied created_at rows could repeat or omit across pages. Four other pre-existing paginated viewsets (tags, usage_v2, dashboard_metrics, pipeline_v2) had the same override. Swapped each to DeterministicOrderingFilter, appending the pk tie-breaker while keeping their existing backends unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018HXYFVCZGi7YN9qjDGyJCQ
P1 — Explicit filter bypasses tie-breaker (d6562f4)
Swapped
Each now appends |
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
|
Unstract test resultsPer-group results
Critical paths
|
2 participants
What
Makes list pagination consistent across the four shared resource endpoints (workflows, Prompt Studio, adapters, connectors) and ships the sortable, co-owner-aware resource lists on top. Two stacked workstreams landing as one change so
mainnever sees an intermediate UI state:ResourceTablelisting UI with server-driven sort/search and an "Owned By" column.Why the rollout stalled (UN-3770)
#2187 added opt-in pagination but couldn't be turned on. The adapter, connector and Prompt Studio
for_user()managers usedDISTINCT ON, and Postgres requires aDISTINCT ONquery to order by the distinct expression first — so those viewsets were pinned toorder_by("id")(arbitrary UUID order) and physically could not order bymodified_ator name. Workflows shipped only because its manager already used plain.distinct()..distinct("id")was never providing ordering determinism; it forcedorder_by("id"), and ordering by a unique column is what made paging deterministic. That guarantee is now explicit via an appendedpktie-breaker.Backend
.distinct("id")/.distinct("tool_id")→ no.distinct()at all. Every arm of the sharing predicate is a PK subquery, not a join, so there are no duplicate rows to collapse; the clause was a no-op. Behaviour-preserving; Workflows is the live proof.ordering = ["-modified_at", "pk"]+ordering_fieldson all five viewsets, backed by a(organization, -modified_at)index on each of the four resource models.DeterministicOrderingFilterreplaces DRF'sOrderingFilterinDEFAULT_FILTER_BACKENDS, appendingpkto any ordering that lacks one (a client?ordering=replaces the view default, so the tie-breaker must be added at the filter). The execution-log endpoints (file_execution,execution_log_view) inherit this global default too — each request is scoped to a singleexecution_id, so the pk tie-breaker sorts a narrow per-execution set, and tiedcreated_at/event_timerows can't repeat or omit across pages.created_by__emailremoved fromordering_fields. Search matches the resource name or the displayed owner's email, against the OWNER membership that backs the Owned By column (not the audit-onlycreated_by); it matches the email prefix (local part) so a bare domain fragment doesn't return every row in a single-domain org.owner_emails()(a list, renamed fromowner_email()) resolves live OWNER memberships earliest-first, skipping service accounts, breakingcreated_atties by pk. Frictionless adapters maskowner_emailsto["Unstract"]org-wide, andResourceTablefalls back tocreated_by_emailwhen there is no live OWNER so no row renders "Unknown".Frontend
ResourceTable(Name / Owned By / Created / Modified / Actions) with server-driven sort, search and pagination — the parent owns the fetch.usePaginatedListhook owns paginated list state + handlers (search/sort/paginate/refresh), unwraps the opt-in envelope, drops superseded responses, and steps back a page when a delete empties the last one.User-visible change
Prompt Studio, adapters and connectors previously listed in arbitrary UUID order; they now default to most-recently-modified-first, matching Workflows. Frontend sort defaults align (
modified_at/desc) so the lists don't re-order twice.Tests
backend/utils/tests/test_list_pagination.py— 7 tests parametrized across all four endpoints (24 subtests): page partitioning newest-first, client?ordering=keeping the pk tie-breaker across two timestamp groups, multi-predicate dedup, name-and-owner-email search, dropped owner-ordering field ignored, andowner_email()earliest-live-owner incl. tied timestamps. Verified as real gates — reverting the filter / manager change fails them.Local: pagination contract file 7 passed, 24 subtests; ruff + biome clean on all changed files. CI runs the full unit/integration/e2e tiers.
Merging
Ordered separately: unstract-python-client#24 (clone-script page-following) and unstract-cloud#1673 (cloud plugin selectors, same page-following — inert until the flip). The unconditional-pagination flip (
OptionalPagination→ unconditional) is a deliberate follow-up after this is validated on staging.