mirror of
https://github.com/duckietm/Nitro-V3.git
synced 2026-06-19 15:06:20 +00:00
Revert feature-folder migration; keep classic src/components + src/hooks layout
Decision: the src/features/<feature>/ layout introduced as proposal #3 (pilot on doorbell in 8ec9d27) is not the convention the team wants. The existing src/components/<area>/ + src/hooks/<area>/ 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/<area>/<feature>/ * hooks under src/hooks/<area>/<feature?>/ (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
This commit is contained in:
+44
-62
@@ -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:
|
> **Update:** an earlier version of this document proposed a
|
||||||
```
|
> `src/features/<feature>/` layout (vertical slices). The pilot on the
|
||||||
src/components/wired-tools/ (views)
|
> doorbell widget showed that the existing `src/components/<area>/` +
|
||||||
src/hooks/wired-tools/ (hooks)
|
> `src/hooks/<area>/` split is the convention the team wants to keep.
|
||||||
src/api/wired/ (utility functions, mixed with the wired runtime)
|
> The pilot has been rolled back; this section is left as a record of
|
||||||
```
|
> the decision.
|
||||||
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.
|
|
||||||
|
|
||||||
**Solution.** Feature folders. Each feature owns its complete vertical
|
**Current convention** (the one to follow):
|
||||||
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
|
|
||||||
```
|
|
||||||
|
|
||||||
**Rule.** A feature folder may import:
|
- **Views** live under `src/components/<area>/<feature>/*.tsx`
|
||||||
- React, third-party libs, the renderer SDK
|
(e.g. `src/components/room/widgets/doorbell/DoorbellWidgetView.tsx`).
|
||||||
- `src/common/` (UI primitives)
|
- **Hooks** live under `src/hooks/<area>/<feature?>/*.ts`
|
||||||
- `src/api/` (cross-cutting helpers — `LocalizeText`, `SendMessageComposer`)
|
(e.g. `src/hooks/rooms/widgets/useDoorbellState.ts`). Multiple hooks
|
||||||
- Other features **only via their public `index.ts`**
|
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.
|
Discoverability is acceptable as long as the **naming** is consistent —
|
||||||
|
`useDoorbellState` / `useDoorbellActions` / `DoorbellWidgetView` are
|
||||||
**Status.** Pilot done on `src/features/doorbell/` (the doorbell widget,
|
greppable in seconds even though they live in three separate directory
|
||||||
small enough to migrate cleanly in one PR). The legacy
|
trees.
|
||||||
`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.
|
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
@@ -230,9 +208,11 @@ a Zustand slice (#5). `useCatalogActions` is a stateless export — just
|
|||||||
functions that compose composers.
|
functions that compose composers.
|
||||||
|
|
||||||
**Status.** Pilot done on `useDoorbellWidget`:
|
**Status.** Pilot done on `useDoorbellWidget`:
|
||||||
- `src/features/doorbell/hooks/useDoorbellState.ts` — the users list,
|
- `src/hooks/rooms/widgets/useDoorbellState.ts` — the users list,
|
||||||
derived from three events using `useNitroEventReducer`-like pattern.
|
derived from three events using a `useNitroEventReducer`-like pattern.
|
||||||
- `src/features/doorbell/hooks/useDoorbellActions.ts` — `answer(name, flag)`.
|
- `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
|
It's a small hook so the split looks almost theatrical, but the shape is
|
||||||
the same one we want to apply to `useCatalog`.
|
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
|
**Solution.** Adopt **Zustand** for cross-feature UI state. Each feature
|
||||||
owns one slice:
|
owns one slice:
|
||||||
```ts
|
```ts
|
||||||
// src/features/wired-tools/state/wiredToolsSlice.ts
|
// src/state/wired-tools.ts (or src/components/wired-tools/wiredToolsStore.ts)
|
||||||
export const useWiredToolsStore = create<WiredToolsState>()((set) => ({
|
export const useWiredToolsStore = create<WiredToolsState>()((set) => ({
|
||||||
activeTab: 'monitor',
|
activeTab: 'monitor',
|
||||||
setActiveTab: (tab) => set({ activeTab: tab }),
|
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
|
- **`WiredCreatorToolsView` split** — types/constants/helpers extracted to
|
||||||
sibling files; main view 4493 → 3901 lines.
|
sibling files; main view 4493 → 3901 lines.
|
||||||
- **Pattern #1 (`useNitroEventState`)** — implemented + 1 pilot.
|
- **Pattern #1 (`useNitroEventState`)** — implemented + 1 pilot.
|
||||||
- **Pattern #3 (feature folder)** — pilot on `src/features/doorbell/`.
|
- **Pattern #3 (feature folder)** — **rejected**; the existing
|
||||||
- **Pattern #4 (split god-hook)** — pilot on the doorbell hook.
|
`src/components/<area>/` + `src/hooks/<area>/` 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
|
- **Pattern #2 (`useNitroQuery`)** — adapter prototype written, not yet
|
||||||
enabled (needs `yarn add @tanstack/react-query`).
|
enabled (needs `yarn add @tanstack/react-query`).
|
||||||
- **Pattern #5 (Zustand store)** — skeleton written, not yet enabled
|
- **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
|
1. **Enable React Query** (`yarn add @tanstack/react-query`) and migrate
|
||||||
one read-only `useCatalog` fetch as a second pilot. Highest impact, low
|
one read-only `useCatalog` fetch as a second pilot. Highest impact, low
|
||||||
risk.
|
risk.
|
||||||
2. **Migrate one mid-sized feature to feature folders** (e.g. `mod-tools`
|
2. **Enable Zustand** and migrate the `let isCreatingRoom` /
|
||||||
or `campaign`). Mostly mechanical, validates the pattern at a real
|
|
||||||
scale.
|
|
||||||
3. **Enable Zustand** and migrate the `let isCreatingRoom` /
|
|
||||||
`createRoomTimeout` singleton in `NavigatorRoomCreatorView`. Trivial,
|
`createRoomTimeout` singleton in `NavigatorRoomCreatorView`. Trivial,
|
||||||
makes the Compiler stop complaining about cross-component variable
|
makes the Compiler stop complaining about cross-component variable
|
||||||
writes.
|
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
|
Vitest + jsdom + a tiny mock layer for the renderer would unblock every
|
||||||
refactor below.
|
refactor below.
|
||||||
5. **Split `useCatalog`** — the biggest god-hook. Only do this *after*
|
4. **Split `useCatalog`** — the biggest god-hook, following the doorbell
|
||||||
#1 and #5 in this list (React Query removes 60% of the file's
|
pattern (`useCatalogData` / `useCatalogUiState` / `useCatalogActions`
|
||||||
responsibility, Zustand handles its UI state).
|
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
|
Anything else (the per-tab `WiredCreatorTools` split, the
|
||||||
`react-compiler/react-compiler` warnings, the `set-state-in-effect`
|
`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
|
- **Doorbell close button didn't close** while users were pending
|
||||||
(`useEffect(() => setIsVisible(!!users.length))` overrode the close).
|
(`useEffect(() => setIsVisible(!!users.length))` overrode the close).
|
||||||
Fixed by `src/features/doorbell/views/DoorbellWidgetView.tsx` (separate
|
Fixed by `src/components/room/widgets/doorbell/DoorbellWidgetView.tsx`
|
||||||
`dismissed` state, visibility computed in render).
|
(separate `dismissed` state, visibility computed in render).
|
||||||
- **Doorbell optimistic remove without rollback** — the original
|
- **Doorbell optimistic remove without rollback** — the original
|
||||||
`answer()` removed the user from the local list before the server
|
`answer()` removed the user from the local list before the server
|
||||||
confirmed via `RSDE_ACCEPTED`/`RSDE_REJECTED`, leaving client and
|
confirmed via `RSDE_ACCEPTED`/`RSDE_REJECTED`, leaving client and
|
||||||
|
|||||||
@@ -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 (
|
||||||
|
<NitroCardView className="nitro-widget-doorbell" theme="primary-slim">
|
||||||
|
<NitroCardHeaderView headerText={ LocalizeText('navigator.doorbell.title') } onCloseClick={ () => setDismissed(true) } />
|
||||||
|
<NitroCardContentView gap={ 0 } overflow="hidden">
|
||||||
|
<Column gap={ 2 }>
|
||||||
|
<Grid className="text-black font-bold border-bottom px-1 pb-1" gap={ 1 }>
|
||||||
|
<div className="col-span-6">{ LocalizeText('generic.username') }</div>
|
||||||
|
<div className="col-span-6" />
|
||||||
|
</Grid>
|
||||||
|
</Column>
|
||||||
|
<Column className="striped-children" gap={ 0 } overflow="auto">
|
||||||
|
{ users.map(userName => (
|
||||||
|
<Grid key={ userName } alignItems="center" className="text-black border-bottom p-1" gap={ 1 }>
|
||||||
|
<div className="col-span-6">{ userName }</div>
|
||||||
|
<div className="col-span-6">
|
||||||
|
<div className="flex items-center gap-1 justify-end">
|
||||||
|
<Button variant="success" onClick={ () => answer(userName, true) }>
|
||||||
|
{ LocalizeText('generic.accept') }
|
||||||
|
</Button>
|
||||||
|
<Button variant="danger" onClick={ () => answer(userName, false) }>
|
||||||
|
{ LocalizeText('generic.deny') }
|
||||||
|
</Button>
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
|
</Grid>
|
||||||
|
)) }
|
||||||
|
</Column>
|
||||||
|
</NitroCardContentView>
|
||||||
|
</NitroCardView>
|
||||||
|
);
|
||||||
|
};
|
||||||
|
|||||||
@@ -1,3 +0,0 @@
|
|||||||
export { useDoorbellActions } from './hooks/useDoorbellActions';
|
|
||||||
export { useDoorbellState } from './hooks/useDoorbellState';
|
|
||||||
export { DoorbellWidgetView } from './views/DoorbellWidgetView';
|
|
||||||
@@ -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 (
|
|
||||||
<NitroCardView className="nitro-widget-doorbell" theme="primary-slim">
|
|
||||||
<NitroCardHeaderView headerText={ LocalizeText('navigator.doorbell.title') } onCloseClick={ () => setDismissed(true) } />
|
|
||||||
<NitroCardContentView gap={ 0 } overflow="hidden">
|
|
||||||
<Column gap={ 2 }>
|
|
||||||
<Grid className="text-black font-bold border-bottom px-1 pb-1" gap={ 1 }>
|
|
||||||
<div className="col-span-6">{ LocalizeText('generic.username') }</div>
|
|
||||||
<div className="col-span-6" />
|
|
||||||
</Grid>
|
|
||||||
</Column>
|
|
||||||
<Column className="striped-children" gap={ 0 } overflow="auto">
|
|
||||||
{ users.map(userName => (
|
|
||||||
<Grid key={ userName } alignItems="center" className="text-black border-bottom p-1" gap={ 1 }>
|
|
||||||
<div className="col-span-6">{ userName }</div>
|
|
||||||
<div className="col-span-6">
|
|
||||||
<div className="flex items-center gap-1 justify-end">
|
|
||||||
<Button variant="success" onClick={ () => answer(userName, true) }>
|
|
||||||
{ LocalizeText('generic.accept') }
|
|
||||||
</Button>
|
|
||||||
<Button variant="danger" onClick={ () => answer(userName, false) }>
|
|
||||||
{ LocalizeText('generic.deny') }
|
|
||||||
</Button>
|
|
||||||
</div>
|
|
||||||
</div>
|
|
||||||
</Grid>
|
|
||||||
)) }
|
|
||||||
</Column>
|
|
||||||
</NitroCardContentView>
|
|
||||||
</NitroCardView>
|
|
||||||
);
|
|
||||||
};
|
|
||||||
@@ -3,6 +3,8 @@ export * from './useAvatarInfoWidget';
|
|||||||
export * from './useChatCommandSelector';
|
export * from './useChatCommandSelector';
|
||||||
export * from './useChatInputWidget';
|
export * from './useChatInputWidget';
|
||||||
export * from './useChatWidget';
|
export * from './useChatWidget';
|
||||||
|
export * from './useDoorbellActions';
|
||||||
|
export * from './useDoorbellState';
|
||||||
export * from './useDoorbellWidget';
|
export * from './useDoorbellWidget';
|
||||||
export * from './useFilterWordsWidget';
|
export * from './useFilterWordsWidget';
|
||||||
export * from './useFriendRequestWidget';
|
export * from './useFriendRequestWidget';
|
||||||
|
|||||||
+2
-2
@@ -2,8 +2,8 @@ import { GetRoomSession } from '../../../api';
|
|||||||
|
|
||||||
/**
|
/**
|
||||||
* Imperative actions for the doorbell. Stateless on purpose — split from
|
* Imperative actions for the doorbell. Stateless on purpose — split from
|
||||||
* useDoorbellState (proposal #4) so components that only need to dispatch
|
* useDoorbellState so components that only need to dispatch an answer
|
||||||
* an answer don't subscribe to the events.
|
* don't subscribe to the events.
|
||||||
*/
|
*/
|
||||||
export const useDoorbellActions = () => ({
|
export const useDoorbellActions = () => ({
|
||||||
answer: (userName: string, flag: boolean): void =>
|
answer: (userName: string, flag: boolean): void =>
|
||||||
+2
-4
@@ -1,12 +1,10 @@
|
|||||||
import { RoomSessionDoorbellEvent } from '@nitrots/nitro-renderer';
|
import { RoomSessionDoorbellEvent } from '@nitrots/nitro-renderer';
|
||||||
import { useCallback, useLayoutEffect, useRef, useState } from 'react';
|
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)
|
* Reduces the three doorbell events (DOORBELL, RSDE_ACCEPTED, RSDE_REJECTED)
|
||||||
* into a single users array.
|
* into a single users array. Data-only hook split — actions live in
|
||||||
*
|
|
||||||
* This is the proposal #4 split: data-only hook. Actions are in
|
|
||||||
* useDoorbellActions.
|
* useDoorbellActions.
|
||||||
*/
|
*/
|
||||||
export const useDoorbellState = (): readonly string[] =>
|
export const useDoorbellState = (): readonly string[] =>
|
||||||
@@ -1,9 +1,10 @@
|
|||||||
import { useDoorbellActions, useDoorbellState } from '../../../features/doorbell';
|
import { useDoorbellActions } from './useDoorbellActions';
|
||||||
|
import { useDoorbellState } from './useDoorbellState';
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* @deprecated Use `useDoorbellState` and `useDoorbellActions` from
|
* @deprecated Use `useDoorbellState` and `useDoorbellActions` directly.
|
||||||
* `src/features/doorbell` directly. This shim is kept so existing
|
* This shim preserves the old `{ users, answer }` shape so existing
|
||||||
* imports via the `hooks` barrel keep working.
|
* imports keep working.
|
||||||
*/
|
*/
|
||||||
export const useDoorbellWidget = () =>
|
export const useDoorbellWidget = () =>
|
||||||
{
|
{
|
||||||
|
|||||||
Reference in New Issue
Block a user