diff --git a/plans/store-split.md b/plans/store-split.md index 8fbf4134dab8..9c7d450d3ec5 100644 --- a/plans/store-split.md +++ b/plans/store-split.md @@ -6,17 +6,21 @@ Optimize for store reduction, not store proliferation. Avoid splitting `modal-he For this repo, assume most RPCs hit a local service and are cheap. Default toward reloading in the owning component instead of keeping a convenience cache unless the data truly needs to survive navigation or serve unrelated entry points. +Bias toward less frontend bookkeeping. If the Go service already owns a piece of data and the UI can cheaply query it on mount or refresh, prefer doing that over mirroring it in Zustand. A store that exists only to cache service-layer data and avoid a few additional local RPCs is not justified here. + Cross-cutting rule: when an engine action is only a screen-local refresh or prompt nudge, prefer the typed engine listener layer over a one-field store or an `init/shared.tsx` forwarding shim. +Cross-cutting rule: do not replace a removed Zustand store with module-local mutable state. No feature-local cache singletons, in-flight dedupe registries, or other hidden module-scope coordination objects that recreate store behavior in disguise. If multiple mounted descendants in one route need the same loaded value, prefer a route-owned screen component or feature-local provider. + Recommended implementation order: - [x] `settings-email` - [x] `settings-phone` - [x] `people` - [x] `recover-password` -- [ ] `settings-password` +- [x] `settings-password` - [ ] `tracker` -- [ ] `team-building` +- [x] `team-building` - [ ] `modal-header` only for param/local-state extraction, not store splitting ## Key Changes @@ -135,7 +139,7 @@ Public/API impact: - Reduce `useModalHeaderState.setState(...)` writes opportunistically. - Do not introduce new tiny stores as replacements. -### 6. Re-evaluate `settings-password` as a merge candidate only if there is a real home +### 6. Delete `settings-password` instead of keeping a convenience cache Current shape: @@ -144,13 +148,25 @@ Current shape: Planned change: -- Do not localize this to one screen. -- Merge it only if there is an obviously correct account/session store with the same lifecycle. -- Otherwise leave it as an intentionally retained tiny notification-backed cache. +- Delete `shared/stores/settings-password.tsx`; do not keep a dedicated store just to cache `randomPW`. +- Move the initial load into the owning settings UI via a shared settings-local hook or equivalent feature code. +- Use the typed engine listener layer for mounted updates only, and rely on load-on-mount or explicit refresh instead of keeping a warmed global cache. +- Do not retain frontend bookkeeping only to avoid repeated `userLoadPassphraseState` calls to the local service. +- Do not use a module-local cache or singleton dedupe layer for `randomPW`. +- For the password modal specifically, prefer a single route owner for the loaded value: + - let the password screen own the `randomPW` load and set its own header title/options + - if several mounted descendants in the same route need the value, share it through a feature-local provider rather than module scope + +Result: + +- Deleted `shared/stores/settings-password.tsx`. +- Replaced it with settings-local mounted logic that queries the service and listens for `NotifyUsers.passwordChanged` via the typed engine listener layer. Public/API impact: -- None unless a clear merge target emerges. +- Remove `usePWState`. +- Remove `init/shared.tsx` wiring that only forwards `NotifyUsers.passwordChanged` into that store. +- Update settings consumers to own their own load/unknown state. ### 7. Split `tracker` by behavior only where it reduces real store coupling @@ -190,6 +206,11 @@ Planned change: - `sendNotification` - shared session search results/recommendations if multiple subtrees still need them +Result: + +- Removed duplicated provider state for `selectedService`, `searchQuery`, and `searchLimit`. +- Kept the provider for shared builder session state and shared search results. + Public/API impact: - Preserve `TBProvider`/`useTBContext`. @@ -231,6 +252,7 @@ Critical scenarios: ## Assumptions And Defaults - Prefer route params or local feature state over creating new one-field stores. +- Never use module-local mutable caches/singletons as a substitute for a removed store. - Prefer the typed engine listener layer over store plumbing when an engine event only nudges mounted feature UI and can be safely missed while unmounted. - Keep notification-backed shared caches unless they are clearly just convenience state for one screen. - `modal-header` is not a restructuring target beyond extracting route-owned or screen-owned values. diff --git a/shared/constants/init/shared.tsx b/shared/constants/init/shared.tsx index 406c3247d0d5..e71952ed4c1f 100644 --- a/shared/constants/init/shared.tsx +++ b/shared/constants/init/shared.tsx @@ -22,7 +22,6 @@ import type * as UseChatStateType from '@/stores/chat' import type * as UseFSStateType from '@/stores/fs' import type * as UseNotificationsStateType from '@/stores/notifications' import type * as UsePinentryStateType from '@/stores/pinentry' -import type * as UseSettingsPasswordStateType from '@/stores/settings-password' import type * as UseTeamsStateType from '@/stores/teams' import type * as UseTracker2StateType from '@/stores/tracker' import type * as UnlockFoldersType from '@/stores/unlock-folders' @@ -497,13 +496,6 @@ export const _onEngineIncoming = (action: EngineGen.Actions) => { usePinentryState.getState().dispatch.onEngineIncomingImpl(action) } break - case 'keybase.1.NotifyUsers.passwordChanged': - { - const randomPW = action.payload.params.state === T.RPCGen.PassphraseState.random - const {usePWState} = require('@/stores/settings-password') as typeof UseSettingsPasswordStateType - usePWState.getState().dispatch.notifyUsersPasswordChanged(randomPW) - } - break case 'keybase.1.NotifyPhoneNumber.phoneNumbersChanged': { const {list} = action.payload.params useSettingsPhoneState.getState().dispatch.notifyPhoneNumberPhoneNumbersChanged(list ?? undefined) diff --git a/shared/settings/account/index.tsx b/shared/settings/account/index.tsx index a915a82f40d3..1979132d9172 100644 --- a/shared/settings/account/index.tsx +++ b/shared/settings/account/index.tsx @@ -9,11 +9,11 @@ import {loadSettings} from '../load-settings' import {useNavigation, type NavigationProp} from '@react-navigation/native' import {useIsFocused} from '@react-navigation/core' import {useConfigState} from '@/stores/config' -import {usePWState} from '@/stores/settings-password' import {makePhoneError, useSettingsPhoneState} from '@/stores/settings-phone' import {useSettingsEmailState} from '@/stores/settings-email' import {type settingsAccountTab, settingsPasswordTab} from '@/constants/settings' import type {SettingsAccountRouteParams} from '../routes' +import {useRandomPWState} from '../use-random-pw' export const SettingsSection = ({children}: {children: React.ReactNode}) => ( @@ -83,12 +83,12 @@ const EmailPhone = ({onEmailVerificationSuccess}: {onEmailVerificationSuccess: ( ) } -const Password = () => { +const Password = ({randomPW}: {randomPW?: boolean}) => { const navigateAppend = C.Router2.navigateAppend const onSetPassword = () => { navigateAppend({name: settingsPasswordTab, params: {}}) } - const hasPassword = usePWState(s => !s.randomPW) + const hasPassword = !randomPW let passwordLabel: string if (hasPassword) { passwordLabel = Kb.Styles.isMobile ? 'Change' : 'Change password' @@ -189,11 +189,7 @@ const AccountSettings = ({route}: Props) => { const deletePhoneNumber = C.useRPC(T.RPCGen.phoneNumbersDeletePhoneNumberRpcPromise) const [addedEmail, setAddedEmail] = React.useState(addedEmailFromRoute ?? '') const [addedPhone, setAddedPhone] = React.useState(addedPhoneFromRoute) - const {loadHasRandomPw} = usePWState( - C.useShallow(s => ({ - loadHasRandomPw: s.dispatch.loadHasRandomPw, - })) - ) + const {randomPW, reload: reloadRandomPW} = useRandomPWState() const {navigateAppend, switchTab} = C.Router2 const _onClearSupersededPhoneNumber = (phone: string) => { deletePhoneNumber( @@ -239,7 +235,7 @@ const AccountSettings = ({route}: Props) => { const onClearAddedPhone = () => setAddedPhone(false) const onReload = () => { loadSettings() - loadHasRandomPw() + reloadRandomPW() } const onStartPhoneConversation = () => { switchTab(C.Tabs.chatTab) @@ -294,7 +290,7 @@ const AccountSettings = ({route}: Props) => { - + diff --git a/shared/settings/advanced.tsx b/shared/settings/advanced.tsx index c97a56d69264..fd7989ea6a53 100644 --- a/shared/settings/advanced.tsx +++ b/shared/settings/advanced.tsx @@ -4,7 +4,6 @@ import * as T from '@/constants/types' import * as React from 'react' import {ProxySettings} from './proxy' import {processorProfileInProgressKey, traceInProgressKey} from '@/constants/settings' -import {usePWState} from '@/stores/settings-password' import {useFSState} from '@/stores/fs' import {useConfigState} from '@/stores/config' import {useShellState} from '@/stores/shell' @@ -12,6 +11,7 @@ import {ignorePromise, timeoutPromise} from '@/constants/utils' import {pprofDir} from '@/constants/platform' import {clearLocalLogs} from '@/util/misc' import {useWaitingState} from '@/stores/waiting' +import {useRandomPWState} from './use-random-pw' let initialUseNativeFrame: boolean | undefined @@ -103,16 +103,17 @@ const UseNativeFrame = () => { const LockdownCheckbox = (p: { hasRandomPW: boolean + loaded: boolean lockdownModeEnabled?: boolean setLockdownMode: (enabled: boolean) => void settingLockdownMode: boolean }) => { - const {hasRandomPW, lockdownModeEnabled, setLockdownMode, settingLockdownMode} = p + const {hasRandomPW, loaded, lockdownModeEnabled, setLockdownMode, settingLockdownMode} = p const onChangeLockdownMode = setLockdownMode const readMoreUrlProps = Kb.useClickURL('https://keybase.io/docs/lockdown/index') const label = 'Enable account lockdown mode' + (hasRandomPW ? ' (you need to set a password first)' : '') const checked = hasRandomPW || !!lockdownModeEnabled - const disabled = hasRandomPW || settingLockdownMode + const disabled = !loaded || hasRandomPW || settingLockdownMode return ( { const settingLockdownMode = C.Waiting.useAnyWaiting(C.waitingKeySettingsSetLockdownMode) - const {hasRandomPW, loadHasRandomPw} = usePWState( - C.useShallow(s => ({ - hasRandomPW: !!s.randomPW, - loadHasRandomPw: s.dispatch.loadHasRandomPw, - })) - ) + const {loaded: randomPWLoaded, randomPW} = useRandomPWState() + const hasRandomPW = !!randomPW const {onSetOpenAtLogin, openAtLogin} = useShellState( C.useShallow(s => ({ onSetOpenAtLogin: s.dispatch.setOpenAtLogin, @@ -233,9 +230,8 @@ const Advanced = () => { } React.useEffect(() => { - loadHasRandomPw() loadLockdownMode() - }, [loadHasRandomPw, loadLockdownMode]) + }, [loadLockdownMode]) return ( @@ -245,6 +241,7 @@ const Advanced = () => { {settingLockdownMode && } { {setLockdownModeError} )} - {!hasRandomPW && ( + {randomPWLoaded && !hasRandomPW && ( ( ) const DeleteConfirm = () => { - const hasPassword = usePWState(s => !s.randomPW) + const {randomPW, reload} = useRandomPWState() + const needsMobilePassphraseCheck = Kb.Styles.isMobile && randomPW !== true const deleteAccountForever = useDeleteAccount() const username = useCurrentUserState(s => s.username) const [checkData, setCheckData] = React.useState(false) @@ -50,7 +51,7 @@ const DeleteConfirm = () => { // dont do this in a preflight test return } - if (Kb.Styles.isMobile && hasPassword) { + if (needsMobilePassphraseCheck) { navigateAppend({name: 'checkPassphraseBeforeDeleteAccount', params: {}}) } else { deleteAccountForever() @@ -61,14 +62,26 @@ const DeleteConfirm = () => { + + {randomPW === undefined ? ( + + + Still checking whether this account has a password. You can continue, or retry the check. + + + Retry password check + + + ) : null} + + } description="This cannot be undone. By deleting this account, you agree that:" header={ @@ -91,6 +104,10 @@ const styles = Kb.Styles.styleSheetCreate(() => ({ padding: Kb.Styles.globalMargins.mediumLarge, }, }), + randomPWStatus: { + padding: Kb.Styles.globalMargins.small, + paddingBottom: 0, + }, })) export default DeleteConfirm diff --git a/shared/settings/logout.tsx b/shared/settings/logout.tsx index f6d18dfab0bc..7fd570405661 100644 --- a/shared/settings/logout.tsx +++ b/shared/settings/logout.tsx @@ -6,22 +6,15 @@ import * as Kb from '@/common-adapters' import {UpdatePassword, useSubmitNewPassword} from './password' import {useRequestLogout} from './use-request-logout' import {usePasswordCheck} from './use-password-check' -import {usePWState} from '@/stores/settings-password' +import {useRandomPWState} from './use-random-pw' const LogoutContainer = () => { const {checkPassword, checkPasswordIsCorrect, reset} = usePasswordCheck() - const {hasRandomPW, loadHasRandomPw} = usePWState( - C.useShallow(s => ({ - hasRandomPW: s.randomPW, - loadHasRandomPw: s.dispatch.loadHasRandomPw, - })) - ) + const {randomPW: hasRandomPW} = useRandomPWState() const {error, onSave, waitingForResponse} = useSubmitNewPassword(true) const [hasPGPKeyOnServer, setHasPGPKeyOnServer] = React.useState(undefined) const loadPgpSettings = C.useRPC(T.RPCGen.accountHasServerKeysRpcPromise) const requestLogout = useRequestLogout() - - const onBootstrap = loadHasRandomPw const onCheckPassword = checkPassword const _onLogout = () => { @@ -35,10 +28,6 @@ const LogoutContainer = () => { const [password, setPassword] = React.useState('') const [showTyping, setShowTyping] = React.useState(false) - React.useEffect(() => { - onBootstrap() - }, [onBootstrap]) - React.useEffect( () => () => { reset() diff --git a/shared/settings/password.tsx b/shared/settings/password.tsx index 0c5d6066d1b2..8e208b8a78df 100644 --- a/shared/settings/password.tsx +++ b/shared/settings/password.tsx @@ -2,8 +2,9 @@ import * as React from 'react' import * as Kb from '@/common-adapters' import * as C from '@/constants' import * as T from '@/constants/types' +import {useNavigation} from '@react-navigation/native' import {useRequestLogout} from './use-request-logout' -import {usePWState} from '@/stores/settings-password' +import {useRandomPWState} from './use-random-pw' type Props = { error: string @@ -209,9 +210,15 @@ export const useSubmitNewPassword = (thenLogout: boolean) => { } const Container = () => { - const randomPW = usePWState(s => s.randomPW) + const {randomPW} = useRandomPWState() + const navigation = useNavigation() const saveLabel = randomPW ? 'Create password' : 'Save' const {error, onSave, waitingForResponse} = useSubmitNewPassword(false) + const title = randomPW === undefined ? 'Password' : randomPW ? 'Set a password' : 'Change password' + + React.useEffect(() => { + navigation.setOptions({title}) + }, [navigation, title]) const [hasPGPKeyOnServer, setHasPGPKeyOnServer] = React.useState(undefined) const loadPgpSettings = C.useRPC(T.RPCGen.accountHasServerKeysRpcPromise) diff --git a/shared/settings/routes.tsx b/shared/settings/routes.tsx index 6a3182b8f533..ad736c146e96 100644 --- a/shared/settings/routes.tsx +++ b/shared/settings/routes.tsx @@ -7,7 +7,6 @@ import {newRoutes as walletsRoutes} from '../wallets/routes' import * as Settings from '@/constants/settings' import {defineRouteMap} from '@/constants/types/router' import {usePushState} from '@/stores/push' -import {usePWState} from '@/stores/settings-password' import {e164ToDisplay} from '@/util/phone-numbers' import type {Props as FeedbackRouteParams} from './feedback/container' @@ -33,11 +32,6 @@ const PushPromptSkipButton = () => { ) } -const PasswordHeaderTitle = () => { - const hasRandomPW = usePWState(s => !!s.randomPW) - return {hasRandomPW ? 'Set a password' : 'Change password'} -} - const CheckPassphraseCancelButton = () => { const navigateUp = C.Router2.navigateUp return ( @@ -161,7 +155,7 @@ const sharedNewModalRoutes = { getOptions: C.isMobile ? undefined : {title: 'Do you know your password?'}, }), [Settings.settingsPasswordTab]: C.makeScreen(React.lazy(async () => import('./password')), { - getOptions: {headerTitle: () => }, + getOptions: {title: 'Password'}, }), archiveModal: C.makeScreen(React.lazy(async () => import('./archive/modal')), { getOptions: {title: 'Backup'}, diff --git a/shared/settings/use-random-pw.test.tsx b/shared/settings/use-random-pw.test.tsx new file mode 100644 index 000000000000..ae3e3b01ca78 --- /dev/null +++ b/shared/settings/use-random-pw.test.tsx @@ -0,0 +1,59 @@ +/** @jest-environment jsdom */ +/// +import {act, cleanup, renderHook, waitFor} from '@testing-library/react' +import * as T from '@/constants/types' +import {notifyEngineActionListeners} from '@/engine/action-listener' +import {resetAllStores} from '@/util/zustand' +import {useRandomPWState} from './use-random-pw' + +afterEach(() => { + cleanup() + jest.restoreAllMocks() + resetAllStores() +}) + +test('loads passphrase state on mount and can reload on demand', async () => { + const loadPassphraseState = jest + .spyOn(T.RPCGen, 'userLoadPassphraseStateRpcPromise') + .mockResolvedValue(T.RPCGen.PassphraseState.random) + + const {result} = renderHook(() => useRandomPWState()) + + await waitFor(() => expect(result.current.randomPW).toBe(true)) + expect(loadPassphraseState).toHaveBeenCalledTimes(1) + + act(() => { + result.current.reload() + }) + + await waitFor(() => expect(loadPassphraseState).toHaveBeenCalledTimes(2)) +}) + +test('notification updates beat stale load results', async () => { + let resolveLoad: ((state: T.RPCGen.PassphraseState) => void) | undefined + const loadPassphraseState = jest.spyOn(T.RPCGen, 'userLoadPassphraseStateRpcPromise').mockImplementation( + async () => + new Promise(resolve => { + resolveLoad = resolve + }) + ) + + const {result} = renderHook(() => useRandomPWState()) + + await waitFor(() => expect(loadPassphraseState).toHaveBeenCalledTimes(1)) + + act(() => { + notifyEngineActionListeners({ + payload: {params: {state: T.RPCGen.PassphraseState.known}}, + type: 'keybase.1.NotifyUsers.passwordChanged', + } as never) + }) + + await waitFor(() => expect(result.current.randomPW).toBe(false)) + + act(() => { + resolveLoad?.(T.RPCGen.PassphraseState.random) + }) + + await waitFor(() => expect(result.current.randomPW).toBe(false)) +}) diff --git a/shared/settings/use-random-pw.tsx b/shared/settings/use-random-pw.tsx new file mode 100644 index 000000000000..ec2f542dc42b --- /dev/null +++ b/shared/settings/use-random-pw.tsx @@ -0,0 +1,53 @@ +import * as React from 'react' +import * as T from '@/constants/types' +import {ignorePromise} from '@/constants/utils' +import {useEngineActionListener} from '@/engine/action-listener' +import logger from '@/logger' +import {RPCError} from '@/util/errors' + +const isRandomPassphraseState = (state: T.RPCGen.PassphraseState) => state === T.RPCGen.PassphraseState.random + +export const useRandomPWState = () => { + const [randomPW, setRandomPW] = React.useState(undefined) + const requestVersionRef = React.useRef(0) + + const reload = React.useCallback(() => { + const version = requestVersionRef.current + 1 + requestVersionRef.current = version + + const load = async () => { + try { + const passphraseState = await T.RPCGen.userLoadPassphraseStateRpcPromise() + if (requestVersionRef.current !== version) { + return + } + setRandomPW(isRandomPassphraseState(passphraseState)) + } catch (error) { + if (!(error instanceof RPCError)) { + return + } + if (requestVersionRef.current !== version) { + return + } + logger.warn('Error loading hasRandomPW:', error.message) + } + } + + ignorePromise(load()) + }, []) + + React.useEffect(() => { + reload() + }, [reload]) + + useEngineActionListener('keybase.1.NotifyUsers.passwordChanged', action => { + requestVersionRef.current += 1 + setRandomPW(isRandomPassphraseState(action.payload.params.state)) + }) + + return { + loaded: randomPW !== undefined, + randomPW, + reload, + } +} diff --git a/shared/stores/settings-password.tsx b/shared/stores/settings-password.tsx deleted file mode 100644 index 54578d1b9669..000000000000 --- a/shared/stores/settings-password.tsx +++ /dev/null @@ -1,60 +0,0 @@ -import * as Z from '@/util/zustand' -import {ignorePromise} from '@/constants/utils' -import logger from '@/logger' -import {RPCError} from '@/util/errors' -import * as T from '@/constants/types' - -type Store = T.Immutable<{ - randomPW?: boolean -}> - -const initialStore: Store = { - randomPW: undefined, -} - -export type State = Store & { - dispatch: { - loadHasRandomPw: () => void - notifyUsersPasswordChanged: (randomPW: boolean) => void - resetState: () => void - } -} - -export const usePWState = Z.createZustand('settings-password', (set, get) => { - const dispatch: State['dispatch'] = { - loadHasRandomPw: () => { - // Once loaded, do not issue this RPC again. This field can only go true -> - // false (never the opposite way), and there are notifications set up when - // this happens. - const f = async () => { - if (get().randomPW !== undefined) { - return - } - try { - const passphraseState = await T.RPCGen.userLoadPassphraseStateRpcPromise() - const randomPW = passphraseState === T.RPCGen.PassphraseState.random - set(s => { - s.randomPW = randomPW - }) - } catch (error) { - if (!(error instanceof RPCError)) { - return - } - logger.warn('Error loading hasRandomPW:', error.message) - return - } - } - ignorePromise(f()) - }, - notifyUsersPasswordChanged: randomPW => { - set(s => { - s.randomPW = randomPW - }) - }, - resetState: Z.defaultReset, - } - return { - ...initialStore, - dispatch, - } -}) diff --git a/shared/stores/team-building.tsx b/shared/stores/team-building.tsx index b4f98fafaed8..9351c021d4d2 100644 --- a/shared/stores/team-building.tsx +++ b/shared/stores/team-building.tsx @@ -21,9 +21,6 @@ type Store = T.Immutable<{ error: string teamSoFar: Set searchResults: T.TB.SearchResults - searchQuery: T.TB.Query - selectedService: T.TB.ServiceIdWithContact - searchLimit: number userRecs?: Array selectedRole: T.Teams.TeamRoleType sendNotification: boolean @@ -31,11 +28,8 @@ type Store = T.Immutable<{ export const initialStore: Store = { error: '', namespace: 'invalid', - searchLimit: 11, - searchQuery: '', searchResults: new Map(), selectedRole: 'writer', - selectedService: 'keybase', sendNotification: true, teamSoFar: new Set(), } @@ -351,13 +345,10 @@ const createSlice: Z.ImmerStateCreator = (set, get) => { })) }, search: (query, service, includeContacts, limit) => { - set(s => { - s.searchLimit = limit ?? 11 - s.searchQuery = query.trim() - s.selectedService = service - }) + const searchQuery = query.trim() + const selectedService = service + const searchLimit = limit ?? 11 const f = async () => { - const {searchQuery, selectedService, searchLimit} = get() // We can only ask the api for at most 100 results if (searchLimit > 100) { logger.info('ignoring search request with a limit over 100') diff --git a/shared/stores/tests/settings-password.test.ts b/shared/stores/tests/settings-password.test.ts deleted file mode 100644 index 8b2f084d6eb9..000000000000 --- a/shared/stores/tests/settings-password.test.ts +++ /dev/null @@ -1,34 +0,0 @@ -/// -import * as T from '@/constants/types' -import {resetAllStores} from '@/util/zustand' - -import {usePWState} from '../settings-password' - -afterEach(() => { - jest.restoreAllMocks() - resetAllStores() -}) - -const flush = async () => new Promise(resolve => setImmediate(resolve)) - -test('loadHasRandomPw caches the loaded state', async () => { - const loadPassphraseState = jest - .spyOn(T.RPCGen, 'userLoadPassphraseStateRpcPromise') - .mockResolvedValue(T.RPCGen.PassphraseState.random) - - usePWState.getState().dispatch.loadHasRandomPw() - await flush() - usePWState.getState().dispatch.loadHasRandomPw() - await flush() - - expect(usePWState.getState().randomPW).toBe(true) - expect(loadPassphraseState).toHaveBeenCalledTimes(1) -}) - -test('notifyUsersPasswordChanged overwrites the cached state', () => { - usePWState.getState().dispatch.notifyUsersPasswordChanged(true) - expect(usePWState.getState().randomPW).toBe(true) - - usePWState.getState().dispatch.notifyUsersPasswordChanged(false) - expect(usePWState.getState().randomPW).toBe(false) -}) diff --git a/shared/stores/tests/team-building.test.ts b/shared/stores/tests/team-building.test.ts index 8f09a3f0b1ea..ba126aa6ae90 100644 --- a/shared/stores/tests/team-building.test.ts +++ b/shared/stores/tests/team-building.test.ts @@ -61,6 +61,8 @@ test('finishedTeamBuilding clears transient state but preserves namespace and se expect(store.getState().sendNotification).toBe(false) expect(store.getState().teamSoFar.size).toBe(1) expect(store.getState().error).toBe('') - expect(store.getState().searchQuery).toBe('') expect(store.getState().searchResults.size).toBe(0) + expect(store.getState()).not.toHaveProperty('searchLimit') + expect(store.getState()).not.toHaveProperty('searchQuery') + expect(store.getState()).not.toHaveProperty('selectedService') }) diff --git a/skill/zustand-store-pruning/SKILL.md b/skill/zustand-store-pruning/SKILL.md index ea281fbe9ee6..5fff39a1840f 100644 --- a/skill/zustand-store-pruning/SKILL.md +++ b/skill/zustand-store-pruning/SKILL.md @@ -1,12 +1,14 @@ --- name: zustand-store-pruning -description: Use when refactoring Keybase Zustand stores in `shared/stores` to remove screen-local or route-owned state, keep only truly global or cross-screen data in the store, move one-off RPC calls into components with `C.useRPC`, and split the work into safe stacked commits. +description: Use when refactoring Keybase Zustand stores in `shared/stores` to remove screen-local or route-owned state, keep only truly global or cross-screen data in the store, prefer querying the Go service layer over frontend convenience caches, move one-off RPC calls into components with `C.useRPC`, and split the work into safe stacked commits. --- # Pruning Zustand Stores Use this skill for store-by-store cleanup in the Keybase client. The goal is to shrink `shared/stores/*` down to state that is genuinely global, notification-driven, or shared across unrelated screens. +The Go service is the source of truth for a lot of this repo's state. Prefer querying the service from the owning feature over mirroring that data in Zustand. If a store exists mainly to warm a frontend cache and avoid a few extra local RPCs, that is usually a sign the store should go away. + Do not silently drop behavior. If a field or action is ambiguous, state the tradeoff and keep the behavior intact. ## Best Targets First @@ -48,6 +50,8 @@ Before keeping a cache just because several screens read it, ask whether reloadi Default assumption for this repo: RPCs usually hit a local service, so treat most reads as cheap unless you have evidence otherwise. Do not keep a Zustand cache just to avoid a small number of local RPCs. +Prefer reducing frontend bookkeeping. If a piece of state only mirrors what the service already knows, and the UI can query it on mount or refresh, delete the mirror instead of maintaining another store, notification path, and invalidation story in React. + Move it to component state if it is: - Form input text, local validation errors, banners, or submit progress @@ -58,6 +62,8 @@ Move it to component state if it is: Notification-fed UI does not automatically make state global. If a notification only updates a transient banner or screen-local status, keep the trigger where it already lands but move the rendered UI state into the owning screen unless multiple unrelated entry points truly need to read it. +Notification-fed data does not automatically justify a store-backed cache either. If mounted UI can subscribe with `useEngineActionListener(...)` and cheaply reload on mount, prefer that over keeping a small Zustand cache warm in the background. + Prefer the typed engine listener layer over store plumbing when: - An engine action is only a screen-local refresh, prompt, or UI nudge @@ -98,7 +104,14 @@ Keep RPC logic in the store if: - It fans results out to multiple screens - It maintains a shared cache that survives navigation -Do not introduce module-level mutable state to preserve store behavior. If a refactor needs ephemeral bookkeeping that is private to one store, keep it inside the store closure or model it explicitly in store state. Module scope is acceptable for stable constants, pure helpers, and imports, not mutable coordination state. +Do not introduce module-level mutable state to preserve store behavior. This includes feature-local caches, in-flight dedupe singletons, listener registries, or other hidden module-scope coordination that recreates a store in disguise. If a refactor needs ephemeral bookkeeping that is private to one store, keep it inside the store closure or model it explicitly in store state. Module scope is acceptable for stable constants, pure helpers, and imports, not mutable coordination state. + +When pruning a store, do not replace it with a module-local cache. Prefer one of these instead: + +- Keep the data local to the owning screen or modal +- Move explicit entry context through route params +- Use a feature-local provider when multiple mounted descendants in one route need to share a loaded value +- Keep the real store if the data genuinely needs cross-route or background lifetime Prefer reloading in components instead of keeping a store cache when: @@ -107,6 +120,13 @@ Prefer reloading in components instead of keeping a store cache when: - The notification path only exists to keep that convenience cache warm - You do not have a concrete reason that the cache must survive navigation or serve multiple unrelated entry points +Prefer a shared feature hook instead of a store when: + +- Several screens in one feature need the same cheap service-backed metadata +- The data does not need to survive while every consumer is unmounted +- Mounted consumers can listen for updates with `useEngineActionListener(...)` +- The alternative store would mainly exist as a convenience cache + Prefer direct store imports instead of `shared/constants/init/shared.tsx` callback plumbing when: - A store's `dispatch.defer.*` callback exists only to forward to another Zustand store diff --git a/skill/zustand-store-pruning/references/keybase-examples.md b/skill/zustand-store-pruning/references/keybase-examples.md index 8fd3a8402b66..f797e35094d8 100644 --- a/skill/zustand-store-pruning/references/keybase-examples.md +++ b/skill/zustand-store-pruning/references/keybase-examples.md @@ -56,14 +56,12 @@ Likely local: - `hasPGPKeyOnServer` - `rememberPassword` if only the password screen uses it -Maybe keep if used elsewhere or notification-backed: - -- `randomPW` - Good target shape: -- Run load and submit RPCs from the screen with `C.useRPC` -- Keep only truly shared password metadata if another part of the app consumes it +- Delete the dedicated store if `randomPW` is only a convenience cache for cheap local-service data +- Run the initial passphrase-state load from the owning settings UI or a shared settings-local hook +- Use `useEngineActionListener('keybase.1.NotifyUsers.passwordChanged', ...)` for mounted updates instead of `init/shared.tsx` -> store plumbing +- Do not keep a frontend cache only to avoid repeated `userLoadPassphraseState` calls ### `shared/stores/people.tsx`