feat(pds-combobox): add async to pds-combobox#651
Conversation
✅ Deploy Preview for pine-design-system ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
bot
added
bug
|
@coderabbitai review this pr |
|
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe pds-combobox component is enhanced with asynchronous data loading capabilities. New properties ( 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@libs/core/src/components/pds-combobox/pds-combobox.tsx`:
- Around line 404-465: fetchOptions can hang on slow responses, throw on
malformed asyncUrl, and assume API items are objects; wrap URL construction in a
try/catch to guard against TypeError when creating new URL(this.asyncUrl, ...),
implement a fetch timeout using AbortController (or AbortSignal.timeout) and
wire the timeout signal into the same controller used for aborting previous
requests, and make the default mapping in fetchOptions (the branch that uses
...item) defensive by checking typeof item === 'object' && item !== null before
spreading — if the item is a primitive, convert it to a safe { id: item, text:
String(item) } shape or call formatResult if present; ensure you clear the
timeout on success/failure and keep
currentPage/internalOptions/updateOptionsFromAsync usage unchanged.
In `@libs/core/src/components/pds-loader/readme.md`:
- Around line 36-38: The unordered list in
libs/core/src/components/pds-loader/readme.md has inconsistent indentation for
the items, so update the three list entries referencing pds-button,
pds-combobox, and pds-multiselect to the same indentation level as the
surrounding list (remove or add leading spaces so each line begins with the
hyphen at the same column) and ensure there is a single space after each hyphen
to satisfy markdownlint.
🧹 Nitpick comments (4)
libs/core/src/components/pds-combobox/combobox-interface.ts (1)
20-22: Prefer reusingComboboxOptioninAsyncResponse.Proposed refactor
export interface AsyncResponse { - results: Array<{ id: string | number; text: string; [key: string]: unknown }>; + results: ComboboxOption[]; totalCount?: number; }libs/core/src/components/pds-combobox/pds-combobox.tsx (3)
1246-1260: Pagination scroll handler looks good, with a minor edge case.The infinite scroll implementation is well-designed with appropriate guards. However, rapid scrolling could potentially trigger multiple debounced fetches before the
loadingflag is set (sinceloadingis only set insidefetchOptions, not immediately whendebouncedFetchAsyncOptionsis called).Consider setting a flag immediately when pagination is triggered:
💡 Optional: Prevent rapid scroll triggers
private handleScroll = (e: Event) => { - if (!this.asyncUrl || !this.hasMore || this.loading) return; + if (!this.asyncUrl || !this.hasMore || this.loading || this.fetchDebounceTimer !== undefined) return; const target = e.target as HTMLElement; const scrollBottom = target.scrollHeight - target.scrollTop - target.clientHeight;
1283-1301: Consider different loading behavior for initial load vs. pagination.Currently, when
loadingis true, the loading indicator is shown at the top of the listbox regardless of whether it's an initial load or a pagination request. During pagination, this could be confusing since existing options are still visible below.For pagination, the load-more spinner at the bottom (lines 1371-1375) is more appropriate. Consider showing the top loading indicator only when there are no existing options:
💡 Suggested improvement
- {this.loading && ( + {this.loading && this.filteredItems.length === 0 && ( <li class="pds-combobox__loading" role="presentation"> {hasSlottedLoading ? ( <slot name="loading" /> ) : ( <pds-loader size="small" /> )} </li> )}
251-251: Remove unusedobserverfield.The
observerMutationObserver field is declared but never instantiated anywhere in the component. Whiledisconnect()is called on it indisconnectedCallback, the optional chaining masks thatobserverwill always beundefinedat runtime, making this dead code.
| private async fetchOptions(query: string, page: number = 1) { | ||
| if (!this.asyncUrl) return; | ||
|
|
||
| this.abortController?.abort(); | ||
| this.abortController = new AbortController(); | ||
|
|
||
| this.loading = true; | ||
|
|
||
| try { | ||
| const url = new URL(this.asyncUrl, window.location.origin); | ||
| if (this.asyncMethod === 'GET') { | ||
| url.searchParams.set('search', query); | ||
| url.searchParams.set('page', String(page)); | ||
| } | ||
|
|
||
| const response = await fetch(url.toString(), { | ||
| method: this.asyncMethod, | ||
| signal: this.abortController.signal, | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'Accept': 'application/json', | ||
| }, | ||
| ...(this.asyncMethod === 'POST' && { | ||
| body: JSON.stringify({ search: query, page }), | ||
| }), | ||
| }); | ||
|
|
||
| if (!response.ok) throw new Error('Failed to fetch options'); | ||
|
|
||
| const data: AsyncResponse = await response.json(); | ||
|
|
||
| const formattedResults = data.results.map(item => { | ||
| if (this.formatResult) { | ||
| return this.formatResult(item); | ||
| } | ||
| return { | ||
| id: item.id, | ||
| text: item.text, | ||
| ...item, | ||
| }; | ||
| }); | ||
|
|
||
| if (page === 1) { | ||
| this.internalOptions = formattedResults; | ||
| } else { | ||
| this.internalOptions = [...this.internalOptions, ...formattedResults]; | ||
| } | ||
|
|
||
| this.hasMore = data.totalCount ? this.internalOptions.length < data.totalCount : false; | ||
| this.currentPage = page; | ||
|
|
||
| // Update DOM options from async data | ||
| this.updateOptionsFromAsync(); | ||
|
|
||
| } catch (error) { | ||
| if ((error as Error).name !== 'AbortError') { | ||
| console.error('PdsCombobox: Failed to fetch options', error); | ||
| } | ||
| } finally { | ||
| this.loading = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider adding a fetch timeout and guarding against malformed URLs.
A few observations on the fetch implementation:
-
No timeout: The fetch could hang indefinitely if the server doesn't respond. Consider using
AbortSignal.timeout()or a manual timeout. -
URL construction:
new URL(this.asyncUrl, window.location.origin)will throw aTypeErrorfor malformed URLs, which isn't caught explicitly. -
Default formatResult assumption (lines 439-444): The spread
...itemassumesitemis an object. If the API returns primitives, this will fail silently or produce unexpected results.
🛡️ Suggested improvements
private async fetchOptions(query: string, page: number = 1) {
if (!this.asyncUrl) return;
this.abortController?.abort();
this.abortController = new AbortController();
this.loading = true;
try {
- const url = new URL(this.asyncUrl, window.location.origin);
+ let url: URL;
+ try {
+ url = new URL(this.asyncUrl, window.location.origin);
+ } catch {
+ console.error('PdsCombobox: Invalid asyncUrl', this.asyncUrl);
+ return;
+ }
+
if (this.asyncMethod === 'GET') {
url.searchParams.set('search', query);
url.searchParams.set('page', String(page));
}
+ // Add timeout to prevent hanging requests
+ const timeoutId = setTimeout(() => this.abortController?.abort(), 30000);
+
const response = await fetch(url.toString(), {
method: this.asyncMethod,
signal: this.abortController.signal,
// ...
});
+ clearTimeout(timeoutId);🤖 Prompt for AI Agents
In `@libs/core/src/components/pds-combobox/pds-combobox.tsx` around lines 404 -
465, fetchOptions can hang on slow responses, throw on malformed asyncUrl, and
assume API items are objects; wrap URL construction in a try/catch to guard
against TypeError when creating new URL(this.asyncUrl, ...), implement a fetch
timeout using AbortController (or AbortSignal.timeout) and wire the timeout
signal into the same controller used for aborting previous requests, and make
the default mapping in fetchOptions (the branch that uses ...item) defensive by
checking typeof item === 'object' && item !== null before spreading — if the
item is a primitive, convert it to a safe { id: item, text: String(item) } shape
or call formatResult if present; ensure you clear the timeout on success/failure
and keep currentPage/internalOptions/updateOptionsFromAsync usage unchanged.
pixelflips
left a comment
pixelflips
left a comment
There was a problem hiding this comment.
No docs or stories? Stories might be tricky, but we should document the additions in some way.
|
also just a note for the changelog, format the pr title |
QuintonJason
changed the title
QuintonJason
force-pushed
the
fix/combobox-add-async
branch
from
f2f9abd to
13a0d54
Compare
2 participants
Description
Added comprehensive async data fetching capabilities to the
pds-comboboxcomponent to match the functionality ofpds-multiselect.This change enables the combobox to fetch options asynchronously from a URL endpoint, with support for debounced search, pagination, loading states, and both component-managed and consumer-managed async patterns.
Fixes DSS-103
Type of change
How Has This Been Tested?
Please describe the tests you've added and run to verify your changes.
Provide instructions so that we can reproduce.
Please also list any relevant details for your test configuration.
Test Configuration:
Checklist:
If not applicable, leave options unchecked.