thin settings email more#29163
thin settings email more#29163chrisnojima-zoom merged 11 commits intonojima/HOTPOT-next-670-clean-2from
Conversation
There was a problem hiding this comment.
Pull request overview
This PR continues the “Zustand store pruning” effort by moving transient settings success banners to route params / local component state, deleting the now-unneeded People store, and introducing a typed engine-action listener layer so mounted features can react to ephemeral engine events without store plumbing.
Changes:
- Remove transient
addedEmail/addedPhonestore fields and migrate the banners tosettingsAccountTabroute params + local state. - Delete
shared/stores/people.tsxand migrate People refresh behavior to a typeduseEngineActionListener('keybase.1.homeUI.homeUIRefresh', ...). - Add
shared/engine/action-listener.tsxand wire it intoshared/constants/init/shared.tsxto fan out engine actions to feature listeners.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| skill/zustand-store-pruning/SKILL.md | Documents new repo assumptions and guidance for using engine listeners vs store plumbing. |
| shared/stores/tests/settings-phone.test.ts | Updates tests to validate resetState clears cached phone numbers after pruning banner state. |
| shared/stores/tests/settings-email.test.ts | Removes tests tied to deleted addedEmail banner state. |
| shared/stores/tests/people.test.ts | Removes tests for the deleted People store. |
| shared/stores/settings-phone.tsx | Removes transient addedPhone and thin RPC wrapper actions from the store. |
| shared/stores/settings-email.tsx | Removes transient addedEmail state; adds onSuccess callback for verify flow. |
| shared/stores/people.tsx | Deletes the People Zustand store (refreshCount + markViewed plumbing). |
| shared/settings/routes.tsx | Introduces SettingsAccountRouteParams to carry banner params to the Account tab. |
| shared/settings/root-phone.tsx | Updates navigation to settingsAccountTab to pass params object (for typed route params). |
| shared/settings/root-desktop-tablet.tsx | Updates desktop tab navigator param typing to allow settingsAccountTab params. |
| shared/settings/notifications/hooks.tsx | Updates navigation to settingsAccountTab to pass params object. |
| shared/settings/account/use-add-email.tsx | Stops writing banner state into the email store; relies on caller success callback. |
| shared/settings/account/index.tsx | Moves banner state to local state + route params; replaces phone RPC wrapper with C.useRPC. |
| shared/settings/account/email-phone-row.tsx | Moves phone visibility RPC out of store; plumbs email verify success callback up. |
| shared/settings/account/confirm-delete.tsx | Replaces store phone delete wrapper with direct RPC via C.useRPC. |
| shared/settings/account/add-modals.tsx | After add/verify, navigates to Account tab with banner params (replace). |
| shared/router-v2/route-params.tsx | Extends param extraction to handle screens typed with route.params?. |
| shared/people/todo.tsx | Updates navigation to settingsAccountTab and routes verify-email success via onSuccess. |
| shared/people/container.tsx | Subscribes directly to homeUIRefresh via engine listener; moves markViewed out of store. |
| shared/engine/action-listener.tsx | Adds typed engine action subscription + hook, plus reset integration. |
| shared/engine/action-listener.test.ts | Adds unit tests for engine listener dispatch, unsubscribe, and reset behavior. |
| shared/constants/init/shared.tsx | Removes People store forwarding and fans out all engine actions to feature listeners. |
| plans/store-split.md | New plan doc describing the store cleanup series and migration rules. |
| plans/inbox-load-fail.md | Removes an investigation note document. |
| plans/engine-listener.md | New plan doc describing the engine listener migration strategy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 62 out of 63 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const deletePhoneNumber = C.useRPC(T.RPCGen.phoneNumbersDeletePhoneNumberRpcPromise) | ||
| const editEmail = useSettingsEmailState(s => s.dispatch.editEmail) | ||
| const onConfirm = () => { | ||
| if (itemType === 'phone') { | ||
| editPhone(itemAddress, true) | ||
| deletePhoneNumber([{phoneNumber: itemAddress}], () => {}, () => {}) | ||
| } else { |
There was a problem hiding this comment.
This delete-phone RPC uses no-op success/error handlers, which will swallow failures silently. The previous store wrapper logged rejected promises; consider adding an error handler (log and/or show an error banner) so users aren’t left thinking the delete succeeded when it didn’t.
| import {type settingsAccountTab, settingsPasswordTab} from '@/constants/settings' | ||
| import type {SettingsAccountRouteParams} from '../routes' |
There was a problem hiding this comment.
settingsAccountTab is imported as a type-only import, but it’s used in a typeof settingsAccountTab type query. Type-only imports can’t be used with typeof (they have no value symbol), so this will fail typechecking. Import settingsAccountTab as a value (remove the type modifier) and keep typeof settingsAccountTab where needed.
| const _onClearSupersededPhoneNumber = (phone: string) => { | ||
| editPhone(phone, true) | ||
| deletePhoneNumber( | ||
| [{phoneNumber: phone}], | ||
| () => {}, | ||
| () => {} | ||
| ) |
There was a problem hiding this comment.
These C.useRPC(phoneNumbersDeletePhoneNumberRpcPromise) calls pass no-op success/error handlers. Previously the store wrapper used ignorePromise, which at least logged RPC failures; with no-op handlers errors are silently swallowed. Please add an error handler (at minimum log via logger/C utilities, or surface a banner) so failures aren’t invisible.
| setVisibilityPhoneNumber( | ||
| [ | ||
| { | ||
| phoneNumber: contactKey, | ||
| visibility: setSearchable | ||
| ? T.RPCGen.IdentityVisibility.public | ||
| : T.RPCGen.IdentityVisibility.private, | ||
| }, | ||
| ], | ||
| () => {}, | ||
| () => {} | ||
| ) |
There was a problem hiding this comment.
This phoneNumbersSetVisibilityPhoneNumberRpcPromise call uses no-op success/error handlers, which will silently swallow RPC errors. Since the previous dispatch.editPhone path logged promise rejections via ignorePromise, consider adding an error handler here (log and/or revert UI) so failures are observable.
chrisnojima-zoom
merged commit d967a73
into
nojima/HOTPOT-next-670-clean-2
No description provided.