UN-3770 [FIX] Make list pagination consistent across shared resource endpoints#2208
Open
chandrasekharan-zipstack wants to merge 2 commits into
Open
UN-3770 [FIX] Make list pagination consistent across shared resource endpoints#2208chandrasekharan-zipstack wants to merge 2 commits into
chandrasekharan-zipstack wants to merge 2 commits into
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
Contributor
Summary by CodeRabbit
WalkthroughBackend list APIs now use deterministic ordering and full-row deduplication. Frontend list consumers share pagination helpers and paginated list state for multi-page fetching, search, refreshes, and pagination controls. ChangesList pagination consistency
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ListComponent
participant usePaginatedResource
participant API
participant Database
User->>ListComponent: search or change page
ListComponent->>usePaginatedResource: update pagination or search
usePaginatedResource->>API: request filtered page
API->>Database: apply filters, distinct, and deterministic ordering
Database-->>API: return results and count
API-->>usePaginatedResource: return paginated response
usePaginatedResource-->>ListComponent: update rows and pagination
ListComponent-->>User: render list and controls
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
chandrasekharan-zipstack
marked this pull request as ready for review
July 24, 2026 11:56
Contributor
|
| Filename | Overview |
|---|---|
| frontend/src/hooks/usePaginatedResource.js | Centralizes paginated list state, empty-page fallback, search, refresh behavior, and stale-response suppression. |
| backend/utils/filters/ordering_filter.py | Extends DRF ordering so request-selected non-unique fields retain a primary-key tie-breaker. |
| frontend/src/components/tool-settings/tool-settings/ToolSettings.jsx | Migrates adapter settings to the shared server-side pagination hook and incorporates the prior stale-response fix. |
| backend/adapter_processor_v2/views.py | Enables deterministic modified-time ordering and supported client ordering for paginated adapter lists. |
| backend/utils/tests/test_list_pagination.py | Adds shared endpoint coverage for page partitioning, deterministic custom ordering, deduplication, and search counts. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
UI[Shared resource list UI] --> Hook[usePaginatedResource]
Hook -->|page, page_size, search| API[Django list endpoint]
API --> Filter[Search and deterministic ordering]
Filter --> Page[OptionalPagination]
Page -->|count and results| Hook
Hook --> View[ViewTools and Pagination]
Selector[Selector or dropdown] --> AllPages[fetchAllPages]
AllPages --> API
AllPages -->|flattened resources| Selector
Reviews (2): Last reviewed commit: "UN-3770 [FIX] Address review: pk tie-bre..." | 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (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
Contributor
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
Contributor
Author
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.
|
Contributor
Unstract test resultsPer-group results
Critical paths
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
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
Comment on lines
+163
to
+198
| def test_client_ordering_keeps_pk_tiebreaker(self) -> None: | ||
| """``?ordering=`` replaces the view default, so pk must still be appended. | ||
|
|
||
| Every row here shares one ``modified_at``. Primary keys are random | ||
| UUIDs, so ordering by pk is unrelated to insertion order — the returned | ||
| sequence only matches if pk survived as the tie-breaker. | ||
| """ | ||
| for endpoint in LIST_ENDPOINTS: | ||
| with self.subTest(kind=endpoint.kind): | ||
| created = [] | ||
| for i in range(6): | ||
| obj = self._create(endpoint, f"{endpoint.kind}-tied-{i}") | ||
| self._stamp(obj, BASE_TIME) | ||
| created.append(obj) | ||
| expected = [ | ||
| getattr(obj, endpoint.name_field) | ||
| for obj in sorted(created, key=lambda o: str(o.pk)) | ||
| ] | ||
|
|
||
| pages = [ | ||
| self._list( | ||
| endpoint, | ||
| self.owner, | ||
| page=n, | ||
| page_size=3, | ||
| ordering="modified_at", | ||
| ) | ||
| for n in (1, 2) | ||
| ] | ||
| names = [ | ||
| n | ||
| for page in pages | ||
| for n in self._names(endpoint, page.data["results"]) | ||
| ] | ||
|
|
||
| assert names == expected |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make client ordering distinguishable from default fallback.
All rows have the same modified_at, so accepted ordering=modified_at and fallback default -modified_at, pk produce the same PK sequence. Add at least two timestamp groups, with ties within each, and assert ascending modified_at, pk across pages.
🤖 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 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.
Comment on lines
+90
to
+111
| const { | ||
| items: adapterList, | ||
| setItems: setAdapterList, | ||
| isLoading, | ||
| setIsLoading, | ||
| searchTerm, | ||
| setSearchTerm, | ||
| pagination, | ||
| fetchPage, | ||
| refresh: handleListRefresh, | ||
| handlePaginationChange, | ||
| handleSearch, | ||
| } = usePaginatedResource({ | ||
| request: (params) => | ||
| axiosPrivate({ | ||
| method: "GET", | ||
| url: `/api/v1/unstract/${sessionDetails?.orgId}/adapter/`, | ||
| params: { ...params, adapter_type: type?.toUpperCase() }, | ||
| }), | ||
| onError: (err) => setAlertDetails(handleException(err)), | ||
| defaultPageSize: DEFAULT_PAGE_SIZE, | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Manually toggling the shared isLoading around an un-awaited handleListRefresh() races the refresh's own loading state. handleListRefresh() (→ fetchPage) sets isLoading true synchronously and then suspends on the network await; a caller's own .finally(() => setIsLoading(false)) on a sibling promise chain resolves within a few microtask ticks — well before that network call returns — so isLoading flips false while the refresh is still in flight, and the UI briefly shows the pre-action data with no loading indicator until the refreshed data snaps in.
frontend/src/components/tool-settings/tool-settings/ToolSettings.jsx#L90-L111:handleDelete(unchanged, lines 144-158) wraps the delete call withsetIsLoading(true)/.finally(() => setIsLoading(false))while its.then()fireshandleDeleteSuccess()→handleListRefresh()without awaiting it; drop the manual toggle (or await the refresh) sofetchPagealone owns the flag:
axiosPrivate(requestOptions)
.then(() => handleDeleteSuccess())
.catch((err) => setAlertDetails(handleException(err)));frontend/src/components/workflows/workflow/Workflows.jsx#L65-L78:editProject(unchanged, lines 104-131) has the same shape — its edit branch callshandleListRefresh()un-awaited inside a.then()that's wrapped bysetLoading(true)/.finally(() => setLoading(false)); tiesetLoading(false)to whichever branch actually runs instead of the outer.finally.
📍 Affects 2 files
frontend/src/components/tool-settings/tool-settings/ToolSettings.jsx#L90-L111(this comment)frontend/src/components/workflows/workflow/Workflows.jsx#L65-L78
🤖 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 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.
Comment on lines
+65
to
78
| items: projectList, | ||
| isLoading: loading, | ||
| setIsLoading: setLoading, | ||
| pagination, | ||
| setPagination, | ||
| searchTerm, | ||
| fetchPage, | ||
| refresh: handleListRefresh, | ||
| handlePaginationChange, | ||
| handleSearch, | ||
| } = usePaginatedList({ | ||
| fetchData: (...args) => fetchListRef.current?.(...args), | ||
| } = usePaginatedResource({ | ||
| request: (params) => projectApiService.getProjectList(params), | ||
| onError: () => console.error("Unable to get project list"), | ||
| defaultPageSize: DEFAULT_PAGE_SIZE, | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Load failures are silently swallowed — no alert shown to the user.
Every other converted list page (ListOfTools.jsx, ToolSettings.jsx, ConnectorsPage.jsx) routes onError through setAlertDetails(handleException(...)). Here it only does console.error, so if the workflow list fails to load, the user sees an empty/loading page with no explanation.
🩹 Proposed fix
} = usePaginatedResource({
request: (params) => projectApiService.getProjectList(params),
- onError: () => console.error("Unable to get project list"),
+ onError: (err) =>
+ setAlertDetails(handleException(err, "Unable to get project list")),
defaultPageSize: DEFAULT_PAGE_SIZE,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| items: projectList, | |
| isLoading: loading, | |
| setIsLoading: setLoading, | |
| pagination, | |
| setPagination, | |
| searchTerm, | |
| fetchPage, | |
| refresh: handleListRefresh, | |
| handlePaginationChange, | |
| handleSearch, | |
| } = usePaginatedList({ | |
| fetchData: (...args) => fetchListRef.current?.(...args), | |
| } = usePaginatedResource({ | |
| request: (params) => projectApiService.getProjectList(params), | |
| onError: () => console.error("Unable to get project list"), | |
| defaultPageSize: DEFAULT_PAGE_SIZE, | |
| }); | |
| items: projectList, | |
| isLoading: loading, | |
| setIsLoading: setLoading, | |
| pagination, | |
| searchTerm, | |
| fetchPage, | |
| refresh: handleListRefresh, | |
| handlePaginationChange, | |
| handleSearch, | |
| } = usePaginatedResource({ | |
| request: (params) => projectApiService.getProjectList(params), | |
| onError: (err) => | |
| setAlertDetails(handleException(err, "Unable to get project list")), | |
| defaultPageSize: DEFAULT_PAGE_SIZE, | |
| }); |
🤖 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 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
#2187 added opt-in pagination to workflows, prompt studio, adapters and connectors, but only the Workflows page got wired up. This finishes the job for the other three and removes the thing that blocked them.
Why the rollout stalled
The adapter, connector and prompt studio
for_user()managers usedDISTINCT ON. Postgres requires aDISTINCT ONquery to order by the distinct expression first, so those three viewsets were pinned toorder_by("id")and physically could not order bymodified_at. Workflows shipped only because its manager already used plain.distinct().Git archaeology says this is copy-paste lineage, not design —
8a081fa2(#465) introduced it for adapters and it was copied into #471 and #1476, while #1462 independently used plain.distinct()for workflows.Backend
.distinct("id")/.distinct("tool_id")→ plain.distinct(). Every arm of the sharing predicate is a PK subquery (resources_visible_via_groups/resources_visible_via_memberships), not a join, so there are no duplicate rows to collapse. The swap is behaviour-preserving; Workflows is the live proof.order_by()with declarativeordering = ["-modified_at", "pk"].OrderingFilteris already inDEFAULT_FILTER_BACKENDS, so nofilter_backendsoverride is needed — important, because overriding it would silently dropOrganizationFilterBackend.?order_by=asc|descfrom the Workflow viewset. It was added in UN-3770 [FEAT] Opt-in pagination for list endpoints; wire Workflows page #2187 and has zero consumers in either repo;?ordering=covers it through the standard filter.Frontend
helpers/pagination.js:unwrapList(bare array or envelope) andfetchAllPages(pages to exhaustion, single request when unpaginated).fetchAllPages— otherwise they'd silently show only the first 50 rows after the flip.usePaginatedList. The client-sideuseListSearchfilter is deleted.<Pagination>moved intoViewTools, so all four pages share one implementation — page size 10, size changer on,["10","20","50"]. Workflows hadshowSizeChanger={false}; that inconsistency is gone.Workflows.jsxassigningfetchListRef.currentduring render.Safety
Endpoints remain opt-in paginated in this PR, so every change works against both response shapes. Flipping to unconditional
CustomPaginationis deliberately a separate ~3-line PR, after this is validated on staging.Ordered separately: unstract-python-client#24 (clone script page-following) and the matching cloud-plugin PR.
Tests
backend/utils/tests/test_list_pagination.py— 3 tests parametrized across all four endpoints (12 subtests):countmatches?search=narrows both rows andcountVerified these actually fail without the fix: restoring
.distinct("id")makes Postgres reject the query outright.utils/,permissions/, four resource apps): 293 passed, 28 skipped🤖 Generated with Claude Code
https://claude.ai/code/session_018HXYFVCZGi7YN9qjDGyJCQ