clean up tb#29064
Conversation
chrisnojima
force-pushed
the
nojima/ZCLIENT-tb-clean
branch
from
950b399 to
c071cbe
Compare
There was a problem hiding this comment.
Pull request overview
Refactors the Team Building (TB) UI code to reduce duplication and improve readability, including extracting helpers/types, consolidating store selectors, and simplifying some UI mapping logic.
Changes:
- Removes/rewrites several large prop/type “Pick” usages in favor of local prop types and helper functions across TB components.
- Simplifies service accent color selection and tab-bar click handling on native/desktop.
- Consolidates several Zustand store reads using
C.useShallow(...)and restructures TB list-body data/handlers.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/team-building/types.tsx | Removes an exported Props type that was previously used for Pick<...> patterns. |
| shared/team-building/shared.tsx | Replaces a switch with map-based service accent color lookup. |
| shared/team-building/service-tab-bar.native.tsx | Cleans up unused props and simplifies onClick wiring. |
| shared/team-building/service-tab-bar.desktop.tsx | Extracts desktop service layout logic into a helper. |
| shared/team-building/search-result/people-result.tsx | Simplifies dropdown prop wiring and extracts menu item construction. |
| shared/team-building/search-result/common-result.tsx | Extracts small helpers/components to clarify rendering logic. |
| shared/team-building/recs-and-recos.tsx | Replaces Pick<Types.Props,...> with explicit local prop types; minor refactors. |
| shared/team-building/list-body.tsx | Major refactor: helper extraction, consolidated selectors, and clearer effect logic. |
| shared/team-building/index.tsx | Refactors TeamBuilding component by extracting “data” and “actions” hooks/helpers. |
| shared/team-building/filtered-service-tab-bar.tsx | Extracts visible-services computation into a helper. |
| shared/team-building/contacts.tsx | Consolidates settings-contact store reads using C.useShallow(...). |
| shared/constants/types/team-building.tsx | Changes SearchResults typing to readonly map types. |
| AGENTS.md | Adds (duplicate) guidance about preferring consolidated selectors with C.useShallow(...). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export type Query = string | ||
|
|
||
| export type SearchResults = Map<Query, Map<ServiceIdWithContact, Array<User>>> | ||
| export type SearchResults = ReadonlyMap<Query, ReadonlyMap<ServiceIdWithContact, ReadonlyArray<User>>> |
There was a problem hiding this comment.
SearchResults was changed to ReadonlyMap<...>, but the team-building store mutates searchResults (via mapGetEnsureValue(...).set(...) in shared/stores/team-building.tsx). This type change will break TypeScript compilation unless the store is updated or the type remains mutable. Consider keeping SearchResults as a Map, or introducing a separate exported ReadonlySearchResults type for consumers while the store keeps a mutable Map internally.
| export type SearchResults = ReadonlyMap<Query, ReadonlyMap<ServiceIdWithContact, ReadonlyArray<User>>> | |
| export type SearchResults = Map<Query, Map<ServiceIdWithContact, ReadonlyArray<User>>> | |
| export type ReadonlySearchResults = ReadonlyMap<Query, ReadonlyMap<ServiceIdWithContact, ReadonlyArray<User>>> |
AGENTS.md
Outdated
| - Do not add new exported functions, types, or constants unless they are required outside the file. Prefer file-local helpers for one-off implementation details and tests. | ||
| - During refactors, do not delete existing guards, conditionals, or platform/test-specific behavior unless you have proven they are dead and the user asked for that behavior change. Port checks like `androidIsTestDevice` forward into the new code path instead of silently dropping them. | ||
| - When addressing PR or review feedback, including bot or lint-style suggestions, do not apply it mechanically. Verify that the reported issue is real in this codebase and that the proposed fix is consistent with repo rules and improves correctness, behavior, or maintainability before making changes. | ||
| - When a component reads multiple adjacent values from the same store hook, prefer a consolidated selector with `C.useShallow(...)` instead of multiple separate subscriptions. |
There was a problem hiding this comment.
AGENTS.md now contains the same guidance about consolidating store subscriptions with C.useShallow(...) twice (lines 6 and 10). Please remove the duplicate entry to avoid confusion and keep the repo notes concise.
| - When a component reads multiple adjacent values from the same store hook, prefer a consolidated selector with `C.useShallow(...)` instead of multiple separate subscriptions. |
shared/team-building/list-body.tsx
Outdated
| onSearchForMore: (len: number) => void | ||
| onChangeText: (newText: string) => void | ||
| onFinishTeamBuilding: () => void | ||
| offset: unknown |
There was a problem hiding this comment.
ListBodyProps still requires an offset prop, but ListBody no longer destructures or uses it anywhere in this file. If it’s no longer needed, drop it from the prop type (and call sites); if it is needed for animations/scroll behavior, wire it into the relevant component rather than leaving it unused.
| offset: unknown |
Uh oh!
There was an error while loading. Please reload this page.