From 0755285708723fc6cfadd2bcfd724b1c2b0e9213 Mon Sep 17 00:00:00 2001 From: simoleo89 Date: Mon, 11 May 2026 16:31:53 +0000 Subject: [PATCH] Revert feature-folder migration; keep classic src/components + src/hooks layout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Decision: the src/features// layout introduced as proposal #3 (pilot on doorbell in 8ec9d27) is not the convention the team wants. The existing src/components// + src/hooks// split is the one that stays. What's reverted - src/features/doorbell/ is removed entirely. The doorbell view and the two hooks move back under the classic paths: src/features/doorbell/views/DoorbellWidgetView.tsx -> src/components/room/widgets/doorbell/DoorbellWidgetView.tsx src/features/doorbell/hooks/useDoorbellState.ts -> src/hooks/rooms/widgets/useDoorbellState.ts src/features/doorbell/hooks/useDoorbellActions.ts -> src/hooks/rooms/widgets/useDoorbellActions.ts - The compat shims that lived in those classic paths are dropped now that the real files are back. - src/hooks/rooms/widgets/index.ts adds the two new hooks alongside the existing useDoorbellWidget shim (kept as a deprecated wrapper so any external consumer importing the old shape via the barrel keeps working). What's preserved - The split between data and actions (proposal #4) — useDoorbellState and useDoorbellActions remain two separate hooks. This was the actual improvement, and it's independent of where the files sit. - The bug fixes from 8ec9d27 (close button race, optimistic-remove rollback) — both still present, just in the new path. - src/state/createNitroStore.ts and src/api/nitro-query/createNitroQuery.ts are left where they are. They aren't feature folders; they're cross-cutting framework code (Zustand skeleton, React Query adapter prototype) that any feature can consume. Doc - docs/ARCHITECTURE.md section #3 is rewritten to record the decision rather than recommend the layout. It now describes the convention to follow: * views under src/components/// * hooks under src/hooks/// (siblings, not subfolders per widget) * sibling .types/.constants/.helpers files for view-specific code (e.g. WiredCreatorTools.*.ts) - "What's already in place" and "Recently fixed" sections updated to point at the new paths. - "How to pick the next refactor PR" no longer mentions feature-folder migration as an option. Note: the five extra feature folders started this session (reconnect, nitropedia, ads, hc-center, campaign) were never committed; they only existed in the working tree and have been restored from HEAD. Verification - find src/features -type f -> 0 (directory removed). - npx tsc --noEmit on all touched files: clean (only the project-wide pre-existing TS2307 about @nitrots/nitro-renderer not installed locally remains, same as before). - npx eslint on all touched files: 0 errors, 0 warnings. https://claude.ai/code/session_01GrR87LAqnAEyKG2ZbmQt5Q --- docs/ARCHITECTURE.md | 106 ++++++++---------- .../widgets/doorbell/DoorbellWidgetView.tsx | 47 +++++++- src/features/doorbell/index.ts | 3 - .../doorbell/views/DoorbellWidgetView.tsx | 47 -------- src/hooks/rooms/widgets/index.ts | 2 + .../rooms/widgets}/useDoorbellActions.ts | 4 +- .../rooms/widgets}/useDoorbellState.ts | 6 +- src/hooks/rooms/widgets/useDoorbellWidget.ts | 9 +- 8 files changed, 101 insertions(+), 123 deletions(-) delete mode 100644 src/features/doorbell/index.ts delete mode 100644 src/features/doorbell/views/DoorbellWidgetView.tsx rename src/{features/doorbell/hooks => hooks/rooms/widgets}/useDoorbellActions.ts (72%) rename src/{features/doorbell/hooks => hooks/rooms/widgets}/useDoorbellState.ts (89%) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index b98bc33..e822c12 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -152,56 +152,34 @@ using `useMessageEventState` — they're not requests. --- -### 3. Feature folders +### 3. Feature folders ~~(adopted)~~ — **rejected, keep the current layout** -**Problem.** The current layout splits ownership across three trees: -``` -src/components/wired-tools/ (views) -src/hooks/wired-tools/ (hooks) -src/api/wired/ (utility functions, mixed with the wired runtime) -``` -A change to "the wired-tools panel" touches all three. Discoverability is -poor: a new contributor reading `WiredCreatorToolsView.tsx` cannot guess -`useWiredTools` lives 4 directory levels away. +> **Update:** an earlier version of this document proposed a +> `src/features//` layout (vertical slices). The pilot on the +> doorbell widget showed that the existing `src/components//` + +> `src/hooks//` split is the convention the team wants to keep. +> The pilot has been rolled back; this section is left as a record of +> the decision. -**Solution.** Feature folders. Each feature owns its complete vertical -slice: -``` -src/features/wired-tools/ -├── index.ts (public API: only what other features can import) -├── views/ (React components) -├── hooks/ (feature-local hooks) -├── state/ (zustand slices, when they exist) -├── types.ts -├── constants.ts -└── helpers.ts -``` +**Current convention** (the one to follow): -**Rule.** A feature folder may import: -- React, third-party libs, the renderer SDK -- `src/common/` (UI primitives) -- `src/api/` (cross-cutting helpers — `LocalizeText`, `SendMessageComposer`) -- Other features **only via their public `index.ts`** +- **Views** live under `src/components///*.tsx` + (e.g. `src/components/room/widgets/doorbell/DoorbellWidgetView.tsx`). +- **Hooks** live under `src/hooks///*.ts` + (e.g. `src/hooks/rooms/widgets/useDoorbellState.ts`). Multiple hooks + for the same widget go in the same folder as siblings, not in a + per-widget subfolder. +- **Pure helpers / constants / types** that are specific to one view + go in sibling files next to the view (see + `src/components/wired-tools/WiredCreatorTools.{types,constants,helpers}.ts` + for the established pattern). +- **Cross-cutting** utilities continue to live under `src/api/` and + `src/common/`. -A feature folder must **not** reach into another feature's internals. - -**Status.** Pilot done on `src/features/doorbell/` (the doorbell widget, -small enough to migrate cleanly in one PR). The legacy -`src/components/room/widgets/doorbell/DoorbellWidgetView.tsx` and -`src/hooks/rooms/widgets/useDoorbellWidget.ts` are kept as compat-shim -re-exports (one line each) so existing import paths still work — they can -be deleted in a follow-up PR. - -**Migration order suggested.** -Smallest features first to validate the pattern, then bigger: -1. doorbell (done) -2. campaign, ads, mod-tools (each <500 lines) -3. notification-center, help, hc-center -4. catalog, inventory, navigator, wired-tools (multi-thousand lines each) - -A `jscodeshift` codemod could rewrite import paths in bulk, but each -feature's relative-path imports (`../../api`, etc.) need to be re-targeted -to the new depth — codemod-able but verify by running tsc per feature. +Discoverability is acceptable as long as the **naming** is consistent — +`useDoorbellState` / `useDoorbellActions` / `DoorbellWidgetView` are +greppable in seconds even though they live in three separate directory +trees. --- @@ -230,9 +208,11 @@ a Zustand slice (#5). `useCatalogActions` is a stateless export — just functions that compose composers. **Status.** Pilot done on `useDoorbellWidget`: -- `src/features/doorbell/hooks/useDoorbellState.ts` — the users list, - derived from three events using `useNitroEventReducer`-like pattern. -- `src/features/doorbell/hooks/useDoorbellActions.ts` — `answer(name, flag)`. +- `src/hooks/rooms/widgets/useDoorbellState.ts` — the users list, + derived from three events using a `useNitroEventReducer`-like pattern. +- `src/hooks/rooms/widgets/useDoorbellActions.ts` — `answer(name, flag)`. +- `src/hooks/rooms/widgets/useDoorbellWidget.ts` kept as a deprecated + shim that composes the two so existing consumers don't break. It's a small hook so the split looks almost theatrical, but the shape is the same one we want to apply to `useCatalog`. @@ -261,7 +241,7 @@ There is no single source of truth, no devtools, no time-travel. **Solution.** Adopt **Zustand** for cross-feature UI state. Each feature owns one slice: ```ts -// src/features/wired-tools/state/wiredToolsSlice.ts +// src/state/wired-tools.ts (or src/components/wired-tools/wiredToolsStore.ts) export const useWiredToolsStore = create()((set) => ({ activeTab: 'monitor', setActiveTab: (tab) => set({ activeTab: tab }), @@ -331,8 +311,11 @@ The current branch (`claude/update-react-typescript-He2rs`) has applied: - **`WiredCreatorToolsView` split** — types/constants/helpers extracted to sibling files; main view 4493 → 3901 lines. - **Pattern #1 (`useNitroEventState`)** — implemented + 1 pilot. -- **Pattern #3 (feature folder)** — pilot on `src/features/doorbell/`. -- **Pattern #4 (split god-hook)** — pilot on the doorbell hook. +- **Pattern #3 (feature folder)** — **rejected**; the existing + `src/components//` + `src/hooks//` layout is kept. +- **Pattern #4 (split god-hook)** — applied to the doorbell hook: + `src/hooks/rooms/widgets/useDoorbellState.ts` (data) + + `src/hooks/rooms/widgets/useDoorbellActions.ts` (actions). - **Pattern #2 (`useNitroQuery`)** — adapter prototype written, not yet enabled (needs `yarn add @tanstack/react-query`). - **Pattern #5 (Zustand store)** — skeleton written, not yet enabled @@ -349,19 +332,18 @@ Order of value/risk for the next contributor: 1. **Enable React Query** (`yarn add @tanstack/react-query`) and migrate one read-only `useCatalog` fetch as a second pilot. Highest impact, low risk. -2. **Migrate one mid-sized feature to feature folders** (e.g. `mod-tools` - or `campaign`). Mostly mechanical, validates the pattern at a real - scale. -3. **Enable Zustand** and migrate the `let isCreatingRoom` / +2. **Enable Zustand** and migrate the `let isCreatingRoom` / `createRoomTimeout` singleton in `NavigatorRoomCreatorView`. Trivial, makes the Compiler stop complaining about cross-component variable writes. -4. **Add tests** (still the #1 thing missing — see "What I'd fix" notes). +3. **Add tests** (still the #1 thing missing — see "What I'd fix" notes). Vitest + jsdom + a tiny mock layer for the renderer would unblock every refactor below. -5. **Split `useCatalog`** — the biggest god-hook. Only do this *after* - #1 and #5 in this list (React Query removes 60% of the file's - responsibility, Zustand handles its UI state). +4. **Split `useCatalog`** — the biggest god-hook, following the doorbell + pattern (`useCatalogData` / `useCatalogUiState` / `useCatalogActions` + as siblings under `src/hooks/catalog/`). Only do this *after* #1 and + #2 in this list (React Query removes 60% of the file's responsibility, + Zustand handles its UI state). Anything else (the per-tab `WiredCreatorTools` split, the `react-compiler/react-compiler` warnings, the `set-state-in-effect` @@ -437,8 +419,8 @@ data-corrupting. - **Doorbell close button didn't close** while users were pending (`useEffect(() => setIsVisible(!!users.length))` overrode the close). - Fixed by `src/features/doorbell/views/DoorbellWidgetView.tsx` (separate - `dismissed` state, visibility computed in render). + Fixed by `src/components/room/widgets/doorbell/DoorbellWidgetView.tsx` + (separate `dismissed` state, visibility computed in render). - **Doorbell optimistic remove without rollback** — the original `answer()` removed the user from the local list before the server confirmed via `RSDE_ACCEPTED`/`RSDE_REJECTED`, leaving client and diff --git a/src/components/room/widgets/doorbell/DoorbellWidgetView.tsx b/src/components/room/widgets/doorbell/DoorbellWidgetView.tsx index e7ce45c..62c8fff 100644 --- a/src/components/room/widgets/doorbell/DoorbellWidgetView.tsx +++ b/src/components/room/widgets/doorbell/DoorbellWidgetView.tsx @@ -1 +1,46 @@ -export { DoorbellWidgetView } from '../../../../features/doorbell'; +import { FC, useState } from 'react'; +import { LocalizeText } from '../../../../api'; +import { Button, Column, Grid, NitroCardContentView, NitroCardHeaderView, NitroCardView } from '../../../../common'; +import { useDoorbellActions, useDoorbellState } from '../../../../hooks'; + +export const DoorbellWidgetView: FC = () => +{ + const users = useDoorbellState(); + const { answer } = useDoorbellActions(); + const [ dismissed, setDismissed ] = useState(false); + + const isVisible = !dismissed && users.length > 0; + + if(!isVisible) return null; + + return ( + + setDismissed(true) } /> + + + +
{ LocalizeText('generic.username') }
+
+ + + + { users.map(userName => ( + +
{ userName }
+
+
+ + +
+
+ + )) } + + + + ); +}; diff --git a/src/features/doorbell/index.ts b/src/features/doorbell/index.ts deleted file mode 100644 index 66c5c20..0000000 --- a/src/features/doorbell/index.ts +++ /dev/null @@ -1,3 +0,0 @@ -export { useDoorbellActions } from './hooks/useDoorbellActions'; -export { useDoorbellState } from './hooks/useDoorbellState'; -export { DoorbellWidgetView } from './views/DoorbellWidgetView'; diff --git a/src/features/doorbell/views/DoorbellWidgetView.tsx b/src/features/doorbell/views/DoorbellWidgetView.tsx deleted file mode 100644 index cc7b4bd..0000000 --- a/src/features/doorbell/views/DoorbellWidgetView.tsx +++ /dev/null @@ -1,47 +0,0 @@ -import { FC, useState } from 'react'; -import { LocalizeText } from '../../../api'; -import { Button, Column, Grid, NitroCardContentView, NitroCardHeaderView, NitroCardView } from '../../../common'; -import { useDoorbellActions } from '../hooks/useDoorbellActions'; -import { useDoorbellState } from '../hooks/useDoorbellState'; - -export const DoorbellWidgetView: FC = () => -{ - const users = useDoorbellState(); - const { answer } = useDoorbellActions(); - const [ dismissed, setDismissed ] = useState(false); - - const isVisible = !dismissed && users.length > 0; - - if(!isVisible) return null; - - return ( - - setDismissed(true) } /> - - - -
{ LocalizeText('generic.username') }
-
- - - - { users.map(userName => ( - -
{ userName }
-
-
- - -
-
- - )) } - - - - ); -}; diff --git a/src/hooks/rooms/widgets/index.ts b/src/hooks/rooms/widgets/index.ts index ea35008..b1632a6 100644 --- a/src/hooks/rooms/widgets/index.ts +++ b/src/hooks/rooms/widgets/index.ts @@ -3,6 +3,8 @@ export * from './useAvatarInfoWidget'; export * from './useChatCommandSelector'; export * from './useChatInputWidget'; export * from './useChatWidget'; +export * from './useDoorbellActions'; +export * from './useDoorbellState'; export * from './useDoorbellWidget'; export * from './useFilterWordsWidget'; export * from './useFriendRequestWidget'; diff --git a/src/features/doorbell/hooks/useDoorbellActions.ts b/src/hooks/rooms/widgets/useDoorbellActions.ts similarity index 72% rename from src/features/doorbell/hooks/useDoorbellActions.ts rename to src/hooks/rooms/widgets/useDoorbellActions.ts index 7ba7775..b7ff282 100644 --- a/src/features/doorbell/hooks/useDoorbellActions.ts +++ b/src/hooks/rooms/widgets/useDoorbellActions.ts @@ -2,8 +2,8 @@ import { GetRoomSession } from '../../../api'; /** * Imperative actions for the doorbell. Stateless on purpose — split from - * useDoorbellState (proposal #4) so components that only need to dispatch - * an answer don't subscribe to the events. + * useDoorbellState so components that only need to dispatch an answer + * don't subscribe to the events. */ export const useDoorbellActions = () => ({ answer: (userName: string, flag: boolean): void => diff --git a/src/features/doorbell/hooks/useDoorbellState.ts b/src/hooks/rooms/widgets/useDoorbellState.ts similarity index 89% rename from src/features/doorbell/hooks/useDoorbellState.ts rename to src/hooks/rooms/widgets/useDoorbellState.ts index f39326c..5a692cd 100644 --- a/src/features/doorbell/hooks/useDoorbellState.ts +++ b/src/hooks/rooms/widgets/useDoorbellState.ts @@ -1,12 +1,10 @@ import { RoomSessionDoorbellEvent } from '@nitrots/nitro-renderer'; import { useCallback, useLayoutEffect, useRef, useState } from 'react'; -import { useNitroEvent } from '../../../hooks'; +import { useNitroEvent } from '../../events'; /** * Reduces the three doorbell events (DOORBELL, RSDE_ACCEPTED, RSDE_REJECTED) - * into a single users array. - * - * This is the proposal #4 split: data-only hook. Actions are in + * into a single users array. Data-only hook split — actions live in * useDoorbellActions. */ export const useDoorbellState = (): readonly string[] => diff --git a/src/hooks/rooms/widgets/useDoorbellWidget.ts b/src/hooks/rooms/widgets/useDoorbellWidget.ts index 22bffe1..e7e99cf 100644 --- a/src/hooks/rooms/widgets/useDoorbellWidget.ts +++ b/src/hooks/rooms/widgets/useDoorbellWidget.ts @@ -1,9 +1,10 @@ -import { useDoorbellActions, useDoorbellState } from '../../../features/doorbell'; +import { useDoorbellActions } from './useDoorbellActions'; +import { useDoorbellState } from './useDoorbellState'; /** - * @deprecated Use `useDoorbellState` and `useDoorbellActions` from - * `src/features/doorbell` directly. This shim is kept so existing - * imports via the `hooks` barrel keep working. + * @deprecated Use `useDoorbellState` and `useDoorbellActions` directly. + * This shim preserves the old `{ users, answer }` shape so existing + * imports keep working. */ export const useDoorbellWidget = () => {