Repository navigation
Fetch policy categories and tags on demand in shared pickers #99125
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| import {clearPolicyCategoriesLoadingState, getPolicyCategories} from '@libs/actions/Policy/Category'; | ||
| import getNonEmptyStringOnyxID from '@libs/getNonEmptyStringOnyxID'; | ||
|
|
||
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
|
|
||
| import {useEffect, useRef} from 'react'; | ||
|
|
||
| import useNetwork from './useNetwork'; | ||
| import useOnyx from './useOnyx'; | ||
|
|
||
| /** | ||
| * Fetches a policy's categories on demand when a category picker mounts, so a lazy-loaded account does not show only | ||
| * the category already on the expense. | ||
| * | ||
| * The guard is the RAM-only loading state written by `getPolicyCategories`, not "we already tried once": a read that | ||
| * failed leaves `hasOnceLoaded` false, so the next mount or reconnect retries it. Gating on the collection existing | ||
| * would not work either, since a partial collection holding just the selected category is indistinguishable from a | ||
| * complete one. `isLoading` is applied optimistically, so concurrent pickers for the same policy do not each fire a | ||
| * redundant `GetPolicyCategories` read (`API.read` does not dedupe in flight reads). | ||
| */ | ||
|
Contributor
There was a problem hiding this comment. @MelvinBot Please shorten this comment, it is too long. We only want 2-3 lines comment
Contributor
Author
There was a problem hiding this comment. Shortened from 22 lines to 3 in |
||
| function useLoadPolicyCategories(policyID: string | undefined) { | ||
| const [loadingState] = useOnyx(`${ONYXKEYS.COLLECTION.RAM_ONLY_POLICY_CATEGORIES_LOADING_STATE}${getNonEmptyStringOnyxID(policyID)}`); | ||
|
Contributor
There was a problem hiding this comment. If
Contributor
Author
There was a problem hiding this comment. Confirmed, and fixed in With no The flag now mirrors the effect's own guard: Reachable in practice: the bulk-edit category page gets its policyID from Added |
||
| const [policyCategories] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_CATEGORIES}${getNonEmptyStringOnyxID(policyID)}`); | ||
| const isLoading = !!loadingState?.isLoading; | ||
| const hasOnceLoaded = !!loadingState?.hasOnceLoaded; | ||
| const hasCategories = policyCategories !== undefined; | ||
|
|
||
| // The policyID this instance has already dispatched a read for. A failed read clears `isLoading` without setting | ||
| // `hasOnceLoaded`, which would otherwise re-satisfy the effect's guard and retry in a tight loop. Scoping the | ||
| // attempt to this instance keeps the retry cadence at "next time a picker opens" (or the next reconnect below). | ||
| const requestedPolicyIDRef = useRef<string>(undefined); | ||
|
|
||
| const {isOffline} = useNetwork({ | ||
| onReconnect: () => { | ||
| if (!policyID || hasOnceLoaded) { | ||
| return; | ||
| } | ||
| // A read cut off by the disconnect never gets a response, so its `failureData` never applies and | ||
| // `isLoading` would stay true for the rest of the session. Clear it so the effect below can retry. | ||
| requestedPolicyIDRef.current = undefined; | ||
| clearPolicyCategoriesLoadingState(policyID); | ||
| }, | ||
| }); | ||
|
|
||
| useEffect(() => { | ||
| if (isOffline || !policyID || isLoading || hasOnceLoaded || requestedPolicyIDRef.current === policyID) { | ||
| return; | ||
| } | ||
| requestedPolicyIDRef.current = policyID; | ||
| getPolicyCategories(policyID); | ||
| }, [policyID, isOffline, isLoading, hasOnceLoaded]); | ||
|
|
||
| // True only while the read is unsettled and there is nothing to render yet, so the picker shows a skeleton instead | ||
| // of an empty list. The `policyID` check mirrors the effect's guard: with no policyID no read is ever dispatched, | ||
| // so the absent loading state would otherwise keep the skeleton up forever. | ||
| const isLoadingPolicyCategories = !!policyID && !isOffline && !hasOnceLoaded && !hasCategories && (isLoading || loadingState === undefined); | ||
|
|
||
| return {isLoadingPolicyCategories}; | ||
| } | ||
|
|
||
| export default useLoadPolicyCategories; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| import {clearPolicyTagsLoadingState, openPolicyTagsPage} from '@libs/actions/Policy/Tag'; | ||
| import getNonEmptyStringOnyxID from '@libs/getNonEmptyStringOnyxID'; | ||
|
|
||
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
|
|
||
| import {useEffect, useRef} from 'react'; | ||
|
|
||
| import useNetwork from './useNetwork'; | ||
| import useOnyx from './useOnyx'; | ||
|
|
||
| /** | ||
| * Fetches a policy's tags on demand when a tag picker mounts, so a lazy-loaded account does not show only the tag | ||
| * already on the expense. `OpenPolicyTagsPage` is the only tag read available to the client today. | ||
| * | ||
| * The guard is the RAM-only loading state written by `openPolicyTagsPage`, not "we already tried once": a read that | ||
| * failed leaves `hasOnceLoaded` false, so the next mount or reconnect retries it. Gating on the collection existing | ||
| * would not work either, since a partial collection holding just the selected tag is indistinguishable from a | ||
| * complete one. `isLoading` is applied optimistically, so concurrent pickers for the same policy do not each fire a | ||
| * redundant read (`API.read` does not dedupe in flight reads). | ||
| */ | ||
|
Contributor
There was a problem hiding this comment. @MelvinBot Please shorten this comment, it is too long. We only want 2-3 lines comment
Contributor
Author
There was a problem hiding this comment. Both tag comments shortened to 3 lines each in |
||
| function useLoadPolicyTags(policyID: string | undefined) { | ||
| const [loadingState] = useOnyx(`${ONYXKEYS.COLLECTION.RAM_ONLY_POLICY_TAGS_LOADING_STATE}${getNonEmptyStringOnyxID(policyID)}`); | ||
|
Contributor
There was a problem hiding this comment. If
Contributor
Author
There was a problem hiding this comment. Confirmed, and fixed in With no
The flag now mirrors the effect's guard: Covered by the new |
||
| const [policyTags] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_TAGS}${getNonEmptyStringOnyxID(policyID)}`); | ||
| const isLoading = !!loadingState?.isLoading; | ||
| const hasOnceLoaded = !!loadingState?.hasOnceLoaded; | ||
| const hasTags = policyTags !== undefined; | ||
|
|
||
| // The policyID this instance has already dispatched a read for. A failed read clears `isLoading` without setting | ||
| // `hasOnceLoaded`, which would otherwise re-satisfy the effect's guard and retry in a tight loop. Scoping the | ||
| // attempt to this instance keeps the retry cadence at "next time a picker opens" (or the next reconnect below). | ||
| const requestedPolicyIDRef = useRef<string>(undefined); | ||
|
|
||
| const {isOffline} = useNetwork({ | ||
| onReconnect: () => { | ||
| if (!policyID || hasOnceLoaded) { | ||
| return; | ||
| } | ||
| // A read cut off by the disconnect never gets a response, so its `failureData` never applies and | ||
| // `isLoading` would stay true for the rest of the session. Clear it so the effect below can retry. | ||
| requestedPolicyIDRef.current = undefined; | ||
| clearPolicyTagsLoadingState(policyID); | ||
| }, | ||
| }); | ||
|
|
||
| useEffect(() => { | ||
| if (isOffline || !policyID || isLoading || hasOnceLoaded || requestedPolicyIDRef.current === policyID) { | ||
| return; | ||
| } | ||
| requestedPolicyIDRef.current = policyID; | ||
| openPolicyTagsPage(policyID); | ||
| }, [policyID, isOffline, isLoading, hasOnceLoaded]); | ||
|
|
||
| // True only while the read is unsettled and there is nothing to render yet, so the picker shows a skeleton instead | ||
| // of an empty list. The `policyID` check mirrors the effect's guard: with no policyID no read is ever dispatched, | ||
| // so the absent loading state would otherwise keep the skeleton up forever. | ||
| const isLoadingPolicyTags = !!policyID && !isOffline && !hasOnceLoaded && !hasTags && (isLoading || loadingState === undefined); | ||
|
|
||
| return {isLoadingPolicyTags}; | ||
| } | ||
|
|
||
| export default useLoadPolicyTags; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the collection is absent,
categoriesCountis zero, soshouldShowTextInputis false;BaseSelectionListWithSectionsconsumesisLoadingNewOptionsonly in that unmounted text input, while its empty-list skeleton is controlled byshouldShowLoadingPlaceholder. Consequently the picker still renders the selected-only row—or a blank list when nothing is selected—throughout the fetch, allowing interaction with incomplete data instead of showing the loading state described here. The tag picker has the same wiring.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valid — fixed in 05a59bc.
Confirmed:
isLoadingNewOptionsis only read insidetextInputComponent(), which returnsnullwhenshouldShowTextInputis false. With the collection absent,categoriesCountis 0, so the text input was unmounted and the prop did nothing. The skeleton is gated onitemsCount === 0 && shouldShowLoadingPlaceholder.Both pickers now pass
shouldShowLoadingPlaceholderand empty the sections while the fetch is in flight —src/components/CategoryPicker/index.tsx:132-142,src/components/TagPicker/index.tsx:198-211. Emptying the sections is the load-bearing half:shouldShowLoadingPlaceholderalone still renders the selected-only row, which is the flash this PR is trying to remove.The skeleton also now has a terminal state. It is driven by the loading state from the fix in the sibling thread rather than by "collection is still undefined", so a failed read ends the skeleton instead of leaving it up for the rest of the session.