feat: select widget lazy loading #38867
Conversation
WalkthroughThis update enhances the form evaluation process by introducing support for pagination of dynamic values. It adds new action types and corresponding action creator functions, integrates new saga functions to handle paginated dynamic value fetching, and updates both the reducer and DropDownControl component to correctly process and display these dynamic values. Additionally, new tests and type refinements are provided to support the updated functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI Component
participant AC as Action Creator
participant Saga as Dynamic Values Saga
participant API as API Service
participant Reducer as Trigger Reducer
UI->>AC: fetchFormDynamicValNextPage(payload)
AC->>Saga: Dispatch FETCH_FORM_DYNAMIC_VAL_NEXT_PAGE_INIT
Saga->>API: Call fetchDynamicValueSaga(payload)
API-->>Saga: Return dynamic values
Saga->>Reducer: Dispatch FETCH_FORM_DYNAMIC_VAL_NEXT_PAGE_SUCCESS
Reducer-->>UI: Update state with concatenated dynamic values
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
ayushpahwa
changed the base branch from
release
to
feat/multi-select-widget-grouping
github-actions
bot
added
Task
![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
🧹 Nitpick comments (2)
app/client/src/components/formControls/DropDownControl.tsx (1)
490-496: Consider using optional chaining here.For consistency, you could simplify the code by using optional chaining when destructuring
dynamicFetchedValues. However, your existing null checks and fallback assignments are functionally correct.- const { data } = dynamicFetchedValues; - if (data && data.content && data.startIndex != null) { + const { data } = dynamicFetchedValues ?? {}; + if (data?.content && data?.startIndex != null) { // ... }🧰 Tools
🪛 Biome (1.9.4)
[error] 496-496: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/actions/evaluationActions.ts (1)
134-148: Consider improving the action creator implementation.
- The function returns undefined when payload is not provided, which could lead to runtime issues.
- The payload type could be extracted to a separate interface for better reusability.
Consider this implementation:
+interface FetchFormDynamicValNextPagePayload { + value: ConditionalOutput; + dynamicFetchedValues: DynamicValues; + actionId: string; + datasourceId: string; + pluginId: string; + identifier: string; +} -export const fetchFormDynamicValNextPage = (payload?: { - value: ConditionalOutput; - dynamicFetchedValues: DynamicValues; - actionId: string; - datasourceId: string; - pluginId: string; - identifier: string; -}) => { - if (payload) { - return { - type: ReduxActionTypes.FETCH_FORM_DYNAMIC_VAL_NEXT_PAGE_INIT, - payload, - }; - } -}; +export const fetchFormDynamicValNextPage = ( + payload: FetchFormDynamicValNextPagePayload, +) => ({ + type: ReduxActionTypes.FETCH_FORM_DYNAMIC_VAL_NEXT_PAGE_INIT, + payload, +});
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/client/src/actions/evaluationActions.ts(2 hunks)app/client/src/ce/constants/ReduxActionConstants.tsx(1 hunks)app/client/src/ce/sagas/index.tsx(2 hunks)app/client/src/components/formControls/DropDownControl.test.tsx(7 hunks)app/client/src/components/formControls/DropDownControl.tsx(6 hunks)app/client/src/reducers/evaluationReducers/triggerReducer.ts(3 hunks)app/client/src/sagas/FormEvaluationSaga.ts(4 hunks)app/client/src/selectors/formSelectors.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/reducers/evaluationReducers/triggerReducer.ts
[error] 89-89: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
app/client/src/components/formControls/DropDownControl.tsx
[error] 496-496: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (7)
app/client/src/components/formControls/DropDownControl.tsx (3)
32-106: Well-structured grouping logic.The
buildGroupedOptionsfunction and its memoized counterpart are cleanly implemented with the necessary fallback logic. This structure efficiently handles grouping options while avoiding data mutation.
399-407: Great pagination trigger on scroll.Your scroll event logic properly checks for pagination needs and triggers the next page fetch. This ensures smooth incremental loading in the dropdown.
556-570: Well-structured action dispatch.The
fetchFormTriggerNextPagedispatcher follows a clear pattern and handles the payload gracefully. No issues found.app/client/src/reducers/evaluationReducers/triggerReducer.ts (2)
66-104: Robust handling of next-page dynamic values.The new reducer case effectively prevents duplicate calls and merges incoming content into existing data. Good use of
immerfor immutability.🧰 Tools
🪛 Biome (1.9.4)
[error] 89-89: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
88-91: Safe optional chaining usage.Despite the static analysis notice, your prior null checks ensure this optional chaining doesn't lead to runtime errors. The flow is logically correct here.
🧰 Tools
🪛 Biome (1.9.4)
[error] 89-89: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
app/client/src/ce/sagas/index.tsx (1)
24-26: Addition of formEvaluationSagas looks good.Including
formEvaluationSagasin your saga array extends the form evaluation functionalities seamlessly.Also applies to: 86-86
app/client/src/ce/constants/ReduxActionConstants.tsx (1)
754-757: LGTM! Well-structured action type constants.The new action types follow the established naming convention and clearly indicate their purpose for form dynamic value pagination.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/client/src/sagas/FormEvaluationSaga.ts (1)
174-215:⚠️ Potential issueError handling needs improvement.
The error handling in
fetchPaginatedDynamicValuesSaganeeds enhancement with proper error propagation and retry mechanism.The previous review comment about error handling and timeouts is still valid. Please implement the suggested improvements:
- Add retry mechanism
- Add request timeouts
- Dispatch error action on failure
- Remove console logging
🧹 Nitpick comments (1)
app/client/src/sagas/FormEvaluationSaga.ts (1)
269-282: Fix typo in TODO comment.There's a typo in the TODO comment: "TODOD" should be "TODO".
- // TODOD: Find a better way to pass the workspaceId + // TODO: Find a better way to pass the workspaceId
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/sagas/FormEvaluationSaga.ts(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (3)
app/client/src/sagas/FormEvaluationSaga.ts (3)
2-10: LGTM! Clean import organization.The imports are well-organized and the new utility
objectKeysis appropriately imported.Also applies to: 42-42
150-150: LGTM! Improved type safety with objectKeys.The replacement of
Object.keyswithobjectKeysenhances type safety.
376-383: LGTM! Well-structured saga watcher.The
formEvaluationSagasfunction is well-structured usingallandtakeLatestfor handling paginated dynamic value fetching.
| } | ||
| } | ||
|
|
||
| /* --------------------------------------------------- |
There was a problem hiding this comment.
this comment is not much useful. 😅
| } from "reducers/evaluationReducers/formEvaluationReducer"; | ||
|
|
||
| /** | ||
| * Interface for grouped dropdown options with label and children |
There was a problem hiding this comment.
All these comments are not that useful. They are bloating the code.
It's pretty obvious this is an interface for dropdown grouped options.
Try removing such comments, you will realize it is still readable.
Using Typescript and using good variable/function name are enough for good documentation part here.
| * @property {string} label - Display label for the option group | ||
| * @property {SelectOptionProps[]} children - Array of options within this group | ||
| */ | ||
| export interface DropDownGroupedOptionsInterface { |
There was a problem hiding this comment.
Using Interface as suffix in the DropDownGroupedOptionsInterface name does not add any value.
| * Groups dropdown options based on provided configuration | ||
| * The grouping is only done if the optionGroupConfig is provided | ||
| * The default group is "others" if not provided |
There was a problem hiding this comment.
The grouping is only done if the optionGroupConfig is provided The default group is "others" if not provided
Now this is a good comments. This is helping reader know about the special case/infromation.
| * @param {Record<string, DropDownGroupedOptionsInterface>} [optionGroupConfig] - Configuration for grouping options | ||
| * @returns {DropDownGroupedOptionsInterface[] | null} Grouped options array or null if no grouping needed | ||
| */ | ||
| function buildGroupedOptions( |
There was a problem hiding this comment.
isn't this file doing too much work. I would have extracted this util to a utils file. I think it's fine for now.
There was a problem hiding this comment.
Yeah, I thought of taking it out but I felt the context switching might be too high.
|
|
||
| /** | ||
| * Main dropdown control component supporting single/multi select functionality | ||
| * @class DropDownControl |
There was a problem hiding this comment.
This comment about @class and @extends does not add any value.
From the one look, i can tell it's a class that extends another class.
| * If multi select is not enabled, we just set the value to an empty string | ||
| * @param {string | undefined} value - The value to remove | ||
| */ | ||
| function onRemoveOptions(value: string | undefined) { |
There was a problem hiding this comment.
the valuename is vague. It could be optionToRemove. it's help in readability.
| * Renders the dropdown field component | ||
| * @param {Object} props - Component props | ||
| * @returns {JSX.Element} Rendered dropdown component | ||
| */ | ||
| function renderDropdown( |
There was a problem hiding this comment.
if a function is too long, a high level comment would really help the reader about what's happeng and what are the special case the function is handling. otherwise the reader will have to read each line one by one.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
app/client/src/components/formControls/DropDownControl.tsx (5)
32-35: Consider renaming the interface.The
Interfacesuffix in the name is redundant. Consider renaming to justDropDownGroupedOption.-export interface DropDownGroupedOptions { +export interface DropDownGroupedOption { label: string; children: SelectOptionProps[]; }
120-127: Extract repeated pagination payload type.The pagination payload type is repeated in multiple places. Consider extracting it to a shared type to improve maintainability.
+type PaginationPayload = { + value: ConditionalOutput; + dynamicFetchedValues: DynamicValues; + actionId: string; + datasourceId: string; + pluginId: string; + identifier: string; +}; export interface DropDownControlProps extends ControlProps { // ...other props - paginationPayload?: { - value: ConditionalOutput; - dynamicFetchedValues: DynamicValues; - actionId: string; - datasourceId: string; - pluginId: string; - identifier: string; - }; + paginationPayload?: PaginationPayload; }Also applies to: 136-143, 457-464
149-174: Simplify dependency change detection.The nested conditions make the code harder to follow. Consider using early returns and flattening the conditions.
componentDidUpdate(prevProps: Props) { - if (this.props.fetchOptionsConditionally && this.props.isMultiSelect) { - const dependencies = matchExact( - MATCH_ACTION_CONFIG_PROPERTY, - this.props.conditionals?.fetchDynamicValues?.condition, - ); - let hasDependenciesChanged = false; - - if (dependencies?.length) { - dependencies.forEach((depPath) => { - const prevValue = get(prevProps.formValues, depPath); - const currValue = get(this.props.formValues, depPath); - - if (prevValue !== currValue) hasDependenciesChanged = true; - }); - } - - if (hasDependenciesChanged) { - this.props.updateConfigPropertyValue( - this.props.formName, - this.props.configProperty, - [], - ); - } - } + if (!this.props.fetchOptionsConditionally || !this.props.isMultiSelect) return; + + const dependencies = matchExact( + MATCH_ACTION_CONFIG_PROPERTY, + this.props.conditionals?.fetchDynamicValues?.condition, + ); + + if (!dependencies?.length) return; + + const hasDependenciesChanged = dependencies.some( + (depPath) => + get(prevProps.formValues, depPath) !== get(this.props.formValues, depPath) + ); + + if (hasDependenciesChanged) { + this.props.updateConfigPropertyValue( + this.props.formName, + this.props.configProperty, + [], + ); + }
482-482: Use optional chaining for safer access.Consider using optional chaining to safely access nested properties.
-if (data && data.content && data.startIndex != null) { +if (data?.content && data?.startIndex != null) {🧰 Tools
🪛 Biome (1.9.4)
[error] 482-482: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
393-401: Consider debouncing the scroll handler.The scroll handler might fire too frequently. Consider debouncing it to improve performance.
+import { debounce } from "lodash"; -function handlePopupScroll(e: React.UIEvent<HTMLDivElement>) { +const handlePopupScroll = debounce((e: React.UIEvent<HTMLDivElement>) => { if (!props.nextPageNeeded || !props.paginationPayload) return; const target = e.currentTarget; if (target.scrollHeight - target.scrollTop === target.clientHeight) { props.fetchFormTriggerNextPage(props.paginationPayload); } -} +}, 150);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/components/formControls/DropDownControl.tsx(7 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/components/formControls/DropDownControl.tsx
[error] 482-482: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
| } catch (err) { | ||
| // eslint-disable-next-line no-console | ||
| console.error(e); | ||
| } finally { | ||
| return { isLoading, options, formValues }; | ||
| console.error(err); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling.
The catch block only logs to console. Consider proper error handling and typing.
-} catch (err) {
+} catch (err: unknown) {
// eslint-disable-next-line no-console
- console.error(err);
+ console.error('Failed to process dropdown options:', err instanceof Error ? err.message : err);
+ // Consider showing an error state in the UI
+ isLoading = false;
+ options = [];
}📝 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.
| } catch (err) { | |
| // eslint-disable-next-line no-console | |
| console.error(e); | |
| } finally { | |
| return { isLoading, options, formValues }; | |
| console.error(err); | |
| } | |
| } catch (err: unknown) { | |
| // eslint-disable-next-line no-console | |
| console.error('Failed to process dropdown options:', err instanceof Error ? err.message : err); | |
| // Consider showing an error state in the UI | |
| isLoading = false; | |
| options = []; | |
| } |
|
@jsartisan thanks for the thorough review. I was trying out AI to make file more readable. I'll be more careful around it now. I have updated the comments, please do take a look again. Thanks! |
Description
This PR adds capability of server side pagination to the dropdown form component. There is another PR in works to add server side search. To ensure both grouping and pagination work correctly, the dropdown control component is refactored by adding memoization and fixing some rendering issues.
Fixes #38079
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13306719132
Commit: 01f4649
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 13 Feb 2025 12:58:12 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes