mirror of
https://github.com/duckietm/Nitro-V3.git
synced 2026-06-19 15:06:20 +00:00
Split usePollWidget into subscriptions + actions (proposal #4) + doc update
usePollWidget bundled two unrelated responsibilities:
- three useNitroEvent listeners that bridge RoomSessionPollEvent
(OFFER / ERROR / CONTENT) onto the UI event bus via DispatchUiEvent
— pure side-effects, zero local state, should mount once;
- three imperative actions (startPoll, rejectPoll, answerPoll) that
every consumer wants, but which shouldn't re-register the listeners.
In practice the only consumer of usePollWidget was useWordQuizWidget,
which needed only `answerPoll` — but pulled in the three subscriptions
as a side effect every time the word-quiz widget rendered. That's the
classic god-hook anti-pattern this proposal targets.
Split (mirrors the doorbell pattern already in place):
- src/hooks/rooms/widgets/usePollSubscriptions.ts (new): the three
bridge listeners, returns void. Should be mounted ONCE at the
highest stable level above poll-aware UI (room widgets root). For
now still mounted by the shim — follow-up PR can move it.
- src/hooks/rooms/widgets/usePollActions.ts (new): the three
imperative actions. Defensive `?.` on roomSession so a poll action
during a room transition no longer crashes.
- src/hooks/rooms/widgets/usePollWidget.ts: kept as a deprecated shim
that composes both — preserves the old `{ startPoll, rejectPoll,
answerPoll }` shape so existing consumers don't break.
- src/hooks/rooms/widgets/useWordQuizWidget.ts: migrated to import
usePollActions directly. The word-quiz widget no longer registers
poll subscriptions transitively — its render no longer has the side
effect of subscribing to three renderer events.
Doc
- docs/ARCHITECTURE.md "What's already in place": records both god-hook
splits (doorbell + poll), the now-enabled React Query and Zustand,
and the test infrastructure. Removes the "not yet enabled" markers
for #2 and #5.
- "How to pick the next refactor PR": rewritten to reflect that the
foundations are done. New priority order:
1. migrate useCatalog's read-only fetches to useNitroQuery,
2. hoist usePollSubscriptions to room-session level,
3. split useCatalog along the doorbell/poll lines,
4. broaden Vitest coverage,
5. per-tab WiredCreatorToolsView split.
Verification
- yarn eslint on the touched files: 0 errors / 0 warnings.
- yarn test: 22/22 passing, 2 files, ~1.0s.
- Existing useWordQuizWidget consumers (RoomWidgetsView ->
WordQuizWidgetView) unaffected — they import from the barrel which
still re-exports the same shape.
https://claude.ai/code/session_01GrR87LAqnAEyKG2ZbmQt5Q
This commit is contained in:
+43
-27
@@ -313,13 +313,21 @@ The current branch (`claude/update-react-typescript-He2rs`) has applied:
|
|||||||
- **Pattern #1 (`useNitroEventState`)** — implemented + 1 pilot.
|
- **Pattern #1 (`useNitroEventState`)** — implemented + 1 pilot.
|
||||||
- **Pattern #3 (feature folder)** — **rejected**; the existing
|
- **Pattern #3 (feature folder)** — **rejected**; the existing
|
||||||
`src/components/<area>/` + `src/hooks/<area>/` layout is kept.
|
`src/components/<area>/` + `src/hooks/<area>/` layout is kept.
|
||||||
- **Pattern #4 (split god-hook)** — applied to the doorbell hook:
|
- **Pattern #4 (split god-hook)** — applied to:
|
||||||
`src/hooks/rooms/widgets/useDoorbellState.ts` (data) +
|
- doorbell: `useDoorbellState` (data) + `useDoorbellActions`;
|
||||||
`src/hooks/rooms/widgets/useDoorbellActions.ts` (actions).
|
- poll: `usePollSubscriptions` (3 listeners) + `usePollActions`
|
||||||
- **Pattern #2 (`useNitroQuery`)** — adapter prototype written, not yet
|
(3 imperative actions). `useWordQuizWidget` migrated to import
|
||||||
enabled (needs `yarn add @tanstack/react-query`).
|
`usePollActions` directly (it doesn't need the subscriptions).
|
||||||
- **Pattern #5 (Zustand store)** — skeleton written, not yet enabled
|
- **Pattern #2 (`useNitroQuery`)** — **enabled**: `@tanstack/react-query`
|
||||||
(needs `yarn add zustand`).
|
installed, `QueryClientProvider` mounted, real adapter in
|
||||||
|
`src/api/nitro-query/`, first migration on `OfferView`.
|
||||||
|
- **Pattern #5 (Zustand store)** — **enabled**: `zustand` installed,
|
||||||
|
`createNitroStore` is now a real re-export, first migration converts
|
||||||
|
the `let isCreatingRoom` / `createRoomTimeout` singleton in
|
||||||
|
`NavigatorRoomCreatorView` to `useRoomCreatorStore`.
|
||||||
|
- **Test infrastructure** — Vitest 3 + jsdom + @testing-library set up.
|
||||||
|
22 smoke tests passing on the pure helpers
|
||||||
|
(`WiredCreatorTools.helpers.ts`) and the new Zustand store.
|
||||||
- **Bonus (error boundaries)** — `WidgetErrorBoundary` applied at
|
- **Bonus (error boundaries)** — `WidgetErrorBoundary` applied at
|
||||||
`RoomWidgetsView`.
|
`RoomWidgetsView`.
|
||||||
|
|
||||||
@@ -327,28 +335,36 @@ The current branch (`claude/update-react-typescript-He2rs`) has applied:
|
|||||||
|
|
||||||
## How to pick the next refactor PR
|
## How to pick the next refactor PR
|
||||||
|
|
||||||
Order of value/risk for the next contributor:
|
Foundations #1–#3 (React Query, Zustand, Vitest) are **done**. Order of
|
||||||
|
value/risk for the next contributor:
|
||||||
|
|
||||||
1. **Enable React Query** (`yarn add @tanstack/react-query`) and migrate
|
1. **Migrate `useCatalog`'s read-only fetches to `useNitroQuery`.**
|
||||||
one read-only `useCatalog` fetch as a second pilot. Highest impact, low
|
Biggest expected payoff (cache + dedup + loading state for free).
|
||||||
risk.
|
Move the page-tree fetch first; the imperative purchase/gift flows
|
||||||
2. **Enable Zustand** and migrate the `let isCreatingRoom` /
|
stay where they are. Adds tests against the new hooks as you go.
|
||||||
`createRoomTimeout` singleton in `NavigatorRoomCreatorView`. Trivial,
|
2. **Mount `usePollSubscriptions` once at room-session level** instead
|
||||||
makes the Compiler stop complaining about cross-component variable
|
of inside `useWordQuizWidget`. The shim in `usePollWidget` works for
|
||||||
writes.
|
now but is wrong design: subscriptions don't belong inside an actions
|
||||||
3. **Add tests** (still the #1 thing missing — see "What I'd fix" notes).
|
hook. Right place is probably `RoomWidgetsView` or wherever poll
|
||||||
Vitest + jsdom + a tiny mock layer for the renderer would unblock every
|
state should be observable.
|
||||||
refactor below.
|
3. **Split `useCatalog` along the doorbell/poll lines**
|
||||||
4. **Split `useCatalog`** — the biggest god-hook, following the doorbell
|
(`useCatalogData` / `useCatalogUiState` / `useCatalogActions`,
|
||||||
pattern (`useCatalogData` / `useCatalogUiState` / `useCatalogActions`
|
siblings under `src/hooks/catalog/`). Only after #1 — React Query
|
||||||
as siblings under `src/hooks/catalog/`). Only do this *after* #1 and
|
removes ~60% of the file's responsibility, Zustand absorbs the UI
|
||||||
#2 in this list (React Query removes 60% of the file's responsibility,
|
state slice.
|
||||||
Zustand handles its UI state).
|
4. **Wider Vitest coverage**: add cases for `useDoorbellState` (event
|
||||||
|
reducer), `useNitroQuery` (timeout + cleanup), the smaller pure
|
||||||
|
formatters in `src/api/`. ~20 cases gets us to a meaningful smoke
|
||||||
|
baseline.
|
||||||
|
5. **Per-tab split of `WiredCreatorToolsView`** (Monitor / Inspection /
|
||||||
|
Variables / Settings panels). Needs a tiny Zustand slice for the
|
||||||
|
shared state, then each tab moves to its own file. Unblocks the
|
||||||
|
React Compiler memoization on the parent module.
|
||||||
|
|
||||||
Anything else (the per-tab `WiredCreatorTools` split, the
|
Anything else (the `LoginView` dialog split, the
|
||||||
`react-compiler/react-compiler` warnings, the `set-state-in-effect`
|
`react-compiler/react-compiler` warnings on the remaining big files,
|
||||||
sweep, the `LoginView` dialog split) is a downstream consequence of these
|
the `set-state-in-effect` sweep) is a downstream consequence of the
|
||||||
five — easier and safer once the foundations are in place.
|
above — easier and safer once the foundations are in place.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|||||||
@@ -10,6 +10,8 @@ export * from './useFilterWordsWidget';
|
|||||||
export * from './useFriendRequestWidget';
|
export * from './useFriendRequestWidget';
|
||||||
export * from './useFurniChooserWidget';
|
export * from './useFurniChooserWidget';
|
||||||
export * from './usePetPackageWidget';
|
export * from './usePetPackageWidget';
|
||||||
|
export * from './usePollActions';
|
||||||
|
export * from './usePollSubscriptions';
|
||||||
export * from './usePollWidget';
|
export * from './usePollWidget';
|
||||||
export * from './useUserChooserWidget';
|
export * from './useUserChooserWidget';
|
||||||
export * from './useWordQuizWidget';
|
export * from './useWordQuizWidget';
|
||||||
|
|||||||
@@ -0,0 +1,19 @@
|
|||||||
|
import { useRoom } from '../useRoom';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Imperative poll actions. Stateless on purpose — split from
|
||||||
|
* usePollSubscriptions so components that only need to dispatch a
|
||||||
|
* vote / accept / reject don't also register the global subscription
|
||||||
|
* listeners.
|
||||||
|
*/
|
||||||
|
export const usePollActions = () =>
|
||||||
|
{
|
||||||
|
const { roomSession = null } = useRoom();
|
||||||
|
|
||||||
|
return {
|
||||||
|
startPoll: (pollId: number) => roomSession?.sendPollStartMessage(pollId),
|
||||||
|
rejectPoll: (pollId: number) => roomSession?.sendPollRejectMessage(pollId),
|
||||||
|
answerPoll: (pollId: number, questionId: number, answers: string[]) =>
|
||||||
|
roomSession?.sendPollAnswerMessage(pollId, questionId, answers)
|
||||||
|
};
|
||||||
|
};
|
||||||
@@ -0,0 +1,47 @@
|
|||||||
|
import { RoomSessionPollEvent } from '@nitrots/nitro-renderer';
|
||||||
|
import { DispatchUiEvent, RoomWidgetPollUpdateEvent } from '../../../api';
|
||||||
|
import { useNitroEvent } from '../../events';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Bridges the three poll-related renderer events (OFFER / ERROR / CONTENT)
|
||||||
|
* onto the UI event bus. Pure subscription — no React state, no return
|
||||||
|
* value. Mount this once where polls should be observable.
|
||||||
|
*
|
||||||
|
* This is the "subscriptions" half of the proposal #4 split for
|
||||||
|
* usePollWidget. The "actions" half is in usePollActions.
|
||||||
|
*/
|
||||||
|
export const usePollSubscriptions = (): void =>
|
||||||
|
{
|
||||||
|
useNitroEvent<RoomSessionPollEvent>(RoomSessionPollEvent.OFFER, event =>
|
||||||
|
{
|
||||||
|
const pollEvent = new RoomWidgetPollUpdateEvent(RoomWidgetPollUpdateEvent.OFFER, event.id);
|
||||||
|
|
||||||
|
pollEvent.summary = event.summary;
|
||||||
|
pollEvent.headline = event.headline;
|
||||||
|
|
||||||
|
DispatchUiEvent(pollEvent);
|
||||||
|
});
|
||||||
|
|
||||||
|
useNitroEvent<RoomSessionPollEvent>(RoomSessionPollEvent.ERROR, event =>
|
||||||
|
{
|
||||||
|
const pollEvent = new RoomWidgetPollUpdateEvent(RoomWidgetPollUpdateEvent.ERROR, event.id);
|
||||||
|
|
||||||
|
pollEvent.summary = event.summary;
|
||||||
|
pollEvent.headline = event.headline;
|
||||||
|
|
||||||
|
DispatchUiEvent(pollEvent);
|
||||||
|
});
|
||||||
|
|
||||||
|
useNitroEvent<RoomSessionPollEvent>(RoomSessionPollEvent.CONTENT, event =>
|
||||||
|
{
|
||||||
|
const pollEvent = new RoomWidgetPollUpdateEvent(RoomWidgetPollUpdateEvent.CONTENT, event.id);
|
||||||
|
|
||||||
|
pollEvent.startMessage = event.startMessage;
|
||||||
|
pollEvent.endMessage = event.endMessage;
|
||||||
|
pollEvent.numQuestions = event.numQuestions;
|
||||||
|
pollEvent.questionArray = event.questionArray;
|
||||||
|
pollEvent.npsPoll = event.npsPoll;
|
||||||
|
|
||||||
|
DispatchUiEvent(pollEvent);
|
||||||
|
});
|
||||||
|
};
|
||||||
@@ -1,52 +1,17 @@
|
|||||||
import { RoomSessionPollEvent } from '@nitrots/nitro-renderer';
|
import { usePollActions } from './usePollActions';
|
||||||
import { DispatchUiEvent, RoomWidgetPollUpdateEvent } from '../../../api';
|
import { usePollSubscriptions } from './usePollSubscriptions';
|
||||||
import { useNitroEvent } from '../../events';
|
|
||||||
import { useRoom } from '../useRoom';
|
|
||||||
|
|
||||||
const usePollWidgetState = () =>
|
/**
|
||||||
|
* @deprecated Prefer `usePollSubscriptions` (mount once, top-level) and
|
||||||
|
* `usePollActions` (anywhere a component dispatches a vote/accept/reject).
|
||||||
|
* This shim preserves the old `{ startPoll, rejectPoll, answerPoll }`
|
||||||
|
* shape for existing consumers, but each call also re-mounts the three
|
||||||
|
* subscription listeners — which is wrong if the hook is called from
|
||||||
|
* multiple places.
|
||||||
|
*/
|
||||||
|
export const usePollWidget = () =>
|
||||||
{
|
{
|
||||||
const { roomSession = null } = useRoom();
|
usePollSubscriptions();
|
||||||
|
|
||||||
const startPoll = (pollId: number) => roomSession.sendPollStartMessage(pollId);
|
return usePollActions();
|
||||||
|
|
||||||
const rejectPoll = (pollId: number) => roomSession.sendPollRejectMessage(pollId);
|
|
||||||
|
|
||||||
const answerPoll = (pollId: number, questionId: number, answers: string[]) => roomSession.sendPollAnswerMessage(pollId, questionId, answers);
|
|
||||||
|
|
||||||
useNitroEvent<RoomSessionPollEvent>(RoomSessionPollEvent.OFFER, event =>
|
|
||||||
{
|
|
||||||
const pollEvent = new RoomWidgetPollUpdateEvent(RoomWidgetPollUpdateEvent.OFFER, event.id);
|
|
||||||
|
|
||||||
pollEvent.summary = event.summary;
|
|
||||||
pollEvent.headline = event.headline;
|
|
||||||
|
|
||||||
DispatchUiEvent(pollEvent);
|
|
||||||
});
|
|
||||||
|
|
||||||
useNitroEvent<RoomSessionPollEvent>(RoomSessionPollEvent.ERROR, event =>
|
|
||||||
{
|
|
||||||
const pollEvent = new RoomWidgetPollUpdateEvent(RoomWidgetPollUpdateEvent.ERROR, event.id);
|
|
||||||
|
|
||||||
pollEvent.summary = event.summary;
|
|
||||||
pollEvent.headline = event.headline;
|
|
||||||
|
|
||||||
DispatchUiEvent(pollEvent);
|
|
||||||
});
|
|
||||||
|
|
||||||
useNitroEvent<RoomSessionPollEvent>(RoomSessionPollEvent.CONTENT, event =>
|
|
||||||
{
|
|
||||||
const pollEvent = new RoomWidgetPollUpdateEvent(RoomWidgetPollUpdateEvent.CONTENT, event.id);
|
|
||||||
|
|
||||||
pollEvent.startMessage = event.startMessage;
|
|
||||||
pollEvent.endMessage = event.endMessage;
|
|
||||||
pollEvent.numQuestions = event.numQuestions;
|
|
||||||
pollEvent.questionArray = event.questionArray;
|
|
||||||
pollEvent.npsPoll = event.npsPoll;
|
|
||||||
|
|
||||||
DispatchUiEvent(pollEvent);
|
|
||||||
});
|
|
||||||
|
|
||||||
return { startPoll, rejectPoll, answerPoll };
|
|
||||||
};
|
};
|
||||||
|
|
||||||
export const usePollWidget = usePollWidgetState;
|
|
||||||
|
|||||||
@@ -3,7 +3,7 @@ import { useEffect, useState } from 'react';
|
|||||||
import { VoteValue } from '../../../api';
|
import { VoteValue } from '../../../api';
|
||||||
import { useNitroEvent } from '../../events';
|
import { useNitroEvent } from '../../events';
|
||||||
import { useRoom } from '../useRoom';
|
import { useRoom } from '../useRoom';
|
||||||
import { usePollWidget } from './usePollWidget';
|
import { usePollActions } from './usePollActions';
|
||||||
|
|
||||||
const DEFAULT_DISPLAY_DELAY = 4000;
|
const DEFAULT_DISPLAY_DELAY = 4000;
|
||||||
const SIGN_FADE_DELAY = 3;
|
const SIGN_FADE_DELAY = 3;
|
||||||
@@ -16,7 +16,7 @@ const useWordQuizWidgetState = () =>
|
|||||||
const [ questionClearTimeout, setQuestionClearTimeout ] = useState<ReturnType<typeof setTimeout>>(null);
|
const [ questionClearTimeout, setQuestionClearTimeout ] = useState<ReturnType<typeof setTimeout>>(null);
|
||||||
const [ answerCounts, setAnswerCounts ] = useState<Map<string, number>>(new Map());
|
const [ answerCounts, setAnswerCounts ] = useState<Map<string, number>>(new Map());
|
||||||
const [ userAnswers, setUserAnswers ] = useState<Map<number, VoteValue>>(new Map());
|
const [ userAnswers, setUserAnswers ] = useState<Map<number, VoteValue>>(new Map());
|
||||||
const { answerPoll = null } = usePollWidget();
|
const { answerPoll } = usePollActions();
|
||||||
const { roomSession = null } = useRoom();
|
const { roomSession = null } = useRoom();
|
||||||
|
|
||||||
const clearQuestion = () =>
|
const clearQuestion = () =>
|
||||||
|
|||||||
Reference in New Issue
Block a user