mirror of
https://github.com/bitsocialnet/5chan.git
synced 2026-08-03 07:41:04 +02:00
Tommaso CasaburiandGitHub
5dc5408a15
Fix codebase audit regressions while preserving UI/UX behavior and adding review-driven hardening.
58 lines
18 KiB
Markdown
58 lines
18 KiB
Markdown
# React Anti-Patterns Audit
|
|
|
|
## Summary
|
|
|
|
Overall React health is moderate-to-good: the codebase mostly follows the repo's architectural rules (Zustand stores cover real shared state, Bitsocial hooks cover data fetching, Router is used for navigation). The anti-pattern concentration is in three places — a pervasive "derived-state via effect" style (scroll, title, keydown, resize), the two large `Post` components (post-desktop/post-mobile duplicate effect-heavy logic), and a handful of hook-level issues in `use-directories`, `use-publish-reply`, and the `ModQueue` subtree. There is almost no shared-state-in-`useState` anti-pattern left; the remaining `useState` calls are legitimately local UI state.
|
|
|
|
## Findings
|
|
|
|
### Critical
|
|
|
|
- **src/hooks/use-directories.ts:331-462** — `useDirectories` and `useDirectoriesState` are effectively the same hook duplicated wholesale (same `useState` + `useEffect` + module-cache + localStorage + GitHub fetch pattern, ~60 lines each). Both fall under the "no `useEffect` for data fetching; no copy-paste logic" rules, and both power virtually every view via `useTheme`, `BoardLayout`, `Board`, `Catalog`, `Post`, `ModQueue`, `BoardsBar`, etc. *Fix:* Collapse into a single Zustand directories store whose `ensureHydrated()` action is called once at app start (or from a thin shared wrapper hook). Remove the duplicated `useState`/`useEffect` bodies; keep `useDirectories`/`useDirectoriesMetadata` as pure selectors. This would also remove the awkward `return cacheCommunities || state.communities || ...` ternary on line 397.
|
|
- **src/views/mod-queue/mod-queue.tsx:672-772** — `ModQueueCountItem` is a "render-null-just-to-fire-an-effect-into-the-parent" pattern. Each feed item mounts, runs `useEffect(() => onStatusChange(cid, { awaiting, urgent }), [...])` (line 682), and the parent `ModQueueButtonContent` aggregates a `statusMap` `useState<Map>` via `handleStatusChange`. Also has a stale-cleanup effect on line 710. This is textbook derived state pushed up via effects, and causes an O(feed) cascade of renders whenever anything in the feed changes. *Fix:* Compute `normalCount`/`urgentCount` during render in `ModQueueButtonContent` with a plain `.reduce` over `feed` (the awaiting/urgent predicates are pure functions of `comment`, `currentTime`, `alertThresholdSeconds`). Delete `ModQueueCountItem`, `statusMap`, `handleStatusChange`, and the cleanup effect.
|
|
- **src/components/reply-modal/reply-modal.tsx:104-288** — 8 `useEffect`s in one component, several of which are orchestrating state that could be derived or moved into the existing `useReplyModalStore`/`usePublishReply` hooks. Notable: line 156 syncs `parentCidRef.current.style.width` from DOM measurement (fine), but line 163 mixes imperative textarea focus with an escape-key listener, line 186 is a dead one-shot `useEffect(..., [])` setting selection range on an empty textarea, line 196 imperatively mutates `textRef.current.value` (bypassing React) and calls `setTimeout`, line 226 guards an infinite loop via `lastProcessedQuoteInsertRequestIdRef` (see the comment on line 234 — the author acknowledged the effect fights itself). *Fix:* Split modal state into a `useReplyModalController` hook; model quote-insertion as an imperative handler triggered by store subscription (`useEffect` on a `requestId` is the smell), not an effect driven by changing store values.
|
|
|
|
### High
|
|
|
|
- **src/components/post-desktop/post-desktop.tsx:930-1102 vs src/components/post-mobile/post-mobile.tsx:651-779** — post-desktop and post-mobile share three near-identical effect blocks: (1) `setResetFunction` when in post-page/pending view (desktop 930, mobile 651), (2) `repliesResetRequestId` reset guard using a ref (desktop 937, mobile 658), (3) Virtuoso scroll-state save listener (desktop 1089, mobile 767). Both files are ~1000+ LOC and most of the reply-rendering logic above these effects is also duplicated (`filterRepliesForDisplay`, `directRepliesByParentCid`, `useQuotedByMap`, `useReplyHeightEstimates`, `useProgressiveRender`, etc.). *Fix:* Extract a `usePostReplies({ isMobile })` hook that owns the shared effects + memos, plus a `useVirtuosoStateSave(key, enabled)` hook for (3). The two components can keep only layout differences.
|
|
- **src/hooks/use-publish-reply.ts:135-208 vs src/hooks/use-publish-post.ts:86-98** — Both hooks use `useState` for publish-error/state-string that the child component (`PostForm`, `ReplyModal`) only reads back, plus an effect that resets those states when the publish inputs change. This is derived state synced by effect; the reset values are all `null`/`false`, and "when any publish input changes, error goes away" is a render-time computation using the previous error plus the latest input key. The two hooks also share the `abandonPublishRef`/`publishOptionsWithAbandon`/`createBaseOptions`/`setPublishXxxOptions` pattern almost verbatim (~80 LOC of duplication). *Fix:* Extract a `usePublishLifecycle(store, { kind: 'post' | 'reply' })` helper; move the `publishError` reset into the publish action itself (on success/failure) instead of a dependency-array-driven effect. Consolidate the duplicate option-merging boilerplate.
|
|
- **src/components/boards-bar/boards-bar.tsx:31-62, 367-385** — `SearchBar` (desktop) has three separate effects for (a) focus on mount, (b) click-outside close, (c) escape-key close; `BoardsBarMobile` then repeats a `setShowSearchBar` pattern plus a scroll-listener `useState(visible)` + debounced-scroll-effect for nav hide/show. The click-outside + escape-to-close + focus-on-mount triplet is repeated across `SettingsModal`, `CatalogFilters`, `ChallengeModal`, `ReplyModal`, `CreateBoardModal`. *Fix:* Extract `useEscapeToClose(onClose)`, `useClickOutsideToClose(ref, onClose)`, `useAutoFocus(ref)` into `src/hooks/`. `useScrollDirection()` would replace the hand-rolled scroll listener + `visible` state.
|
|
- **src/views/board/board.tsx:313-413** — Seven `useEffect`s in close proximity, most of which are route-sync effects guarded by `isVisible`. Several of them only derive pagination / infinite-scroll redirects from `location.pathname` + `effectiveInfiniteScroll` + `totalPages` (lines 378, 386, 396) — candidates for computing the navigation *during the click handler or render* rather than after mount. The "has been visible" scroll-to-top effect (line 549) uses a ref flag to simulate a once-per-mount guard; a `key` on the subtree or `useNavigationType()` at render would be cleaner. Document-title effect on line 579 duplicates the same pattern as in catalog.tsx:779 and post.tsx:333.
|
|
- **src/views/catalog/catalog.tsx:320-795** — Six `useEffect`s, including a canonical-redirect effect (line 320) that duplicates the board.tsx:378 effect, a `setCurrentCommunityAddress` store-sync effect (line 339), a `reset()` triggered by `filteredComments.length` via a ref latch (line 513) — also duplicated at board.tsx:402 — a `setResetFunction` effect (line 522), and a doc-title effect (line 779). *Fix:* Extract `useCanonicalMultiboardRedirect(location)`, `useFeedResetFunction(reset, isVisible)`, and `useReplayFeedReset(filteredComments, reset)` hooks shared between board and catalog.
|
|
- **Document title + scroll-to-top duplication across views** (`src/views/blotter/blotter.tsx:15`, `src/views/faq/faq.tsx:7`, `src/views/pass/pass.tsx:32-38`, `src/views/rules/rules.tsx:181`, `src/views/pending-post/pending-post.tsx:19`, `src/views/home/home.tsx:193`, `src/views/archive/archive.tsx:208`, `src/views/board/board.tsx:579`, `src/views/catalog/catalog.tsx:779`, `src/views/post/post.tsx:333`) — 10 views each implement their own `useEffect(() => { document.title = ...; window.scrollTo(0, 0); }, [deps])`. *Fix:* Add `useDocumentTitle(title)` and `useScrollToTopOnMount()` (or combined `useViewInit({ title })`) in `src/hooks/` and replace.
|
|
- **src/components/catalog-search/catalog-search.tsx:19-26** — URL query param `?q=` is the source of truth, but the effect on line 19 pushes `queryParam` into the `useCatalogFiltersStore` on every change. The component also holds a separate `searchState` `useState` and computes `inputValue` and `openSearch` by mixing URL, local state, and store. This is the "syncing derived state with an effect" + "boolean flag soup" smells combined. *Fix:* Make the store a selector of the URL (compute `searchFilter` from `location.search` during render, or subscribe via a single `useSearchFilterFromUrl()` selector); remove the effect. Model `searchState` as a discriminated `{ mode: 'closed' } | { mode: 'open'; value: string }`.
|
|
|
|
### Medium
|
|
|
|
- **src/hooks/use-fetch-gif-first-frame.ts:85-135** — `useState` + `useEffect` + module-level `Set<string>` for failed URLs + localForage cache. Classic "data fetching in an effect" that the repo rule says should live in a Bitsocial hook or a Zustand store. Also uses status strings (`'idle'|'loading'|'ready'|'failed'`) which is fine, but the `setGifFirstFrame((prev) => ...)` identity checks (line 90, 95, 99, 126) are a code smell saying the effect is firing too often. *Fix:* Either (a) promote to a dedicated store indexed by URL with a thunk, or (b) turn into a `useSyncExternalStore`-backed cache so React subscribes rather than polls.
|
|
- **src/hooks/use-comment-media-info.ts:19-66** — Fetches thumbnail dimensions by constructing `new Image()` inside `useEffect`, stores the result in component-local `useState`, then re-derives `CommentMediaInfo` with the stored dimensions in a `useMemo`. This is "data fetch in effect" and the resulting `thumbnailDimensions` is local per-consumer — two components rendering the same `link` will both fetch and double-measure. *Fix:* Move the dimension cache to a module-level `Map<string, { w, h }>` or Zustand store keyed by `link`, driven by an async action invoked during render (suspense-style) or by a Bitsocial hook.
|
|
- **src/hooks/use-window-width.ts:3-15 + src/hooks/use-is-mobile.ts** — `useState` + resize-listener `useEffect`. Fine in isolation, but every consumer subscribes independently (mobile vs desktop posts both call `useWindowWidth`, as do reply-height estimates, catalog, boards-bar). On resize, every subscriber rerenders. *Fix:* Back with `useSyncExternalStore` so React can bail out when the value hasn't changed, or keep the subscription in a tiny Zustand store so the read surface is selector-based.
|
|
- **src/views/account-data-editor/account-data-editor.tsx:52-78** — `useState` holds a single `EditorState` object with `phase`, `AceEditor`, `aceOnBeforeLoad`, and `text`. The effect on line 59 runs only when `phase === 'loading'` to import `react-ace`, then `setEditorState` replaces everything. This is a fine state machine, but handler functions mutate only `text` (line 141, 147) while phase is mutated via `handleContinue`/`handleGoBack`. The `account` dependency on line 78 will re-trigger the effect when account changes mid-load, which will double-import Ace. *Fix:* Split into `phase` + `editor` + `text` `useState`s or `useReducer` with explicit actions; guard the dynamic import with a ref so it only runs once per `phase === 'loading'` transition.
|
|
- **src/components/settings-modal/settings-modal.tsx:45-65** — `expandedSections` (a `Set<string>`) is local `useState` that shadows the URL hash, and an effect on line 60 re-syncs the set from `hash` with an explicit `// eslint-disable-next-line react-hooks/exhaustive-deps`. The disable comment is a red flag: the author silenced the lint rule because the effect is "wrong" by the rules but the fix is deeper. *Fix:* Derive `expandedSections` from `hash` plus a separate "user-added" set; or keep a single Zustand store for expanded sections so the hash is just the deep-linkable entry point.
|
|
- **src/views/post/post.tsx:293-297, 315-331, 381-429** — Seven effects on the `PostPage` component: `/not-found` redirect (line 293), "scroll to top unless thread-top intent" split across two effects (lines 315, 321) with `consumedThreadTopScrollRef` coordinating them, a cleanup-only `resetThreadLiveUpdates` effect (line 381), a thread-switch reset effect (line 387), and a refresh-by-cid orchestration effect (line 397-429). The split between the two scroll effects is explicitly documented in-file as subtle (comment at line 310) — a sign this is better modeled as one `useLayoutEffect` on `[location.key]` that reads the intent synchronously. *Fix:* Combine the two scroll effects into one keyed on `location.key`; move the refresh-by-cid orchestration into `useThreadLiveUpdatesStore` as an action invoked from a subscription in the store rather than an effect in the view.
|
|
- **src/components/reply-quote-preview/reply-quote-preview.tsx:133-148, 304-311** — Desktop and mobile variants each run their own `window.addEventListener('resize', handleResize)` effect; same pattern (and same comment style) in `src/components/markdown/external-number-quote-link.tsx:83`, and `src/components/markdown/markdown.tsx:70`. *Fix:* One `useWindowResize(callback)` hook.
|
|
- **src/components/edit-menu/edit-menu.tsx:45-110, 165-222** — Five `useState` slots plus `defaultPublishEditOptions` `useMemo` plus `publishCommentEditOptions` `useState` initialized from the memo. When `defaultPublishEditOptions` changes (lots of deps on line 92), the local `useState` is *not* reset — so opening the menu after the comment state changes can show stale options. Combined with `banDuration` (line 165) also initialized from `defaultPublishEditOptions.commentModeration?.author?.banExpiresAt` only once, this is a "state initialized from props and never re-synced" bug-magnet. *Fix:* Use a `key={cid}` wrapper or derive during render; for the editable slice of state that must be local, reset it in the "open" handler rather than by effect.
|
|
|
|
### Low
|
|
|
|
- **src/components/markdown/external-number-quote-link.tsx:53-56** — Four correlated `useState`s (`isResolving`, `isPreviewOpen`, `previewState`, `previewPosition`). A discriminated `previewState` union (`{ kind: 'idle' } | { kind: 'resolving' } | { kind: 'open'; position } | { kind: 'error' }`) would eliminate impossible combinations and the `setIsResolving(false)` bookkeeping.
|
|
- **src/components/catalog-filters/catalog-filters.tsx:113-116, 240-252** — Two more escape-key-close effects; covered by the suggested `useEscapeToClose` extraction.
|
|
- **src/components/challenge-modal/challenge-modal.tsx:129-157, 232-305** — Four effects; line 129 sends a theme message post-load (fine), line 135 is a window-message handler (fine), line 232 and 297 are focus-on-mount and escape-to-close — again covered by the shared hooks extraction.
|
|
- **src/index.tsx:52-58** — `window.history.back()` inside the Capacitor back-button listener. Since the HashRouter is available at that point, this could be `navigate(-1)` via a small bridge, but the Capacitor listener is fired outside React tree, so calling `window.history.back()` directly is defensible. *Fix:* none required; keep the comment explicit and ignore from the "no manual history" rule since it's a native bridge, not in-app nav.
|
|
- **src/views/pass/pass.tsx:32-38** — Two adjacent `useEffect(() => { ... }, [])` blocks; trivially combinable into one. Covered by `useDocumentTitle`/`useScrollToTopOnMount`.
|
|
- **src/views/pending-post/pending-post.tsx:19, 28, 34** — Three effects, two of which are navigation side-effects. The `post`-driven redirect on line 34 fires on every `post` identity change; should depend on `post?.cid` + `postCommunityAddress` not on `post` as a whole.
|
|
- **src/components/boards-bar/boards-bar.tsx:140-142** — `useBoardsBarVisibilityStore.getState().initialize()` called from an empty-deps `useEffect`. If the store's initialize is idempotent this is fine, but it's an implicit "mount once, anywhere" that would be cleaner as a top-level store side-effect on creation.
|
|
- **src/components/post-form/post-form.tsx:391-476** — Five effects for display-name hydration, publish success navigation, reply success close, and upload callbacks — each fires once per success and sets a `hasInitializedDisplayName` ref to guard re-runs. The ref-gating pattern (also in reply-modal.tsx:282 and use-publish-reply.ts:147) is a known smell that says "this should be an event, not an effect".
|
|
|
|
## Top 5 Actions
|
|
|
|
1. **Consolidate `useDirectories` / `useDirectoriesState` into a Zustand store.** Removes ~100 LOC of duplicated fetch-in-effect, simplifies the "stable reference for memoization" contract that currently leaks into every consumer, and fixes the rule-violating data-fetch-in-effect at the root of the dependency tree (`src/hooks/use-directories.ts:331-462`).
|
|
|
|
2. **Delete the `ModQueueCountItem` / `statusMap` effect cascade.** Compute `normalCount` and `urgentCount` purely during render in `ModQueueButtonContent` (`src/views/mod-queue/mod-queue.tsx:672-772`). This removes a per-feed-item render-null-to-fire-effect pattern that's both a textbook anti-pattern and a real performance hazard on boards with large queues.
|
|
|
|
3. **Create shared view-init hooks: `useDocumentTitle`, `useScrollToTopOnMount`, `useEscapeToClose`, `useClickOutsideToClose`, `useAutoFocus`, `useWindowResize`, `useScrollDirection`.** Replace the ~20 scattered `useEffect`s across views and modals (`blotter`, `faq`, `pass`, `rules`, `home`, `archive`, `board`, `catalog`, `post`, `pending-post`, plus `boards-bar`, `settings-modal`, `catalog-filters`, `challenge-modal`, `reply-modal`, `reply-quote-preview`, `markdown`, `external-number-quote-link`). High leverage, low risk — most replacements are mechanical.
|
|
|
|
4. **Refactor post-desktop / post-mobile shared logic into `usePostReplies` and `useVirtuosoStateSave` hooks.** Both files are ~1000+ LOC, they duplicate reply filtering, height estimation, progressive rendering, fresh-replies registration, reset-function wiring, and Virtuoso scroll-state save listeners. Extracting the shared tail end (from `freshRepliesForRender` onward) into a hook cuts both files roughly in half and removes the parallel-edit risk.
|
|
|
|
5. **Split route-sync effects in `board.tsx` and `catalog.tsx` into shared hooks.** Specifically: `useCanonicalMultiboardRedirect`, `useFeedResetFunction(reset, isVisible)`, `useReplayFeedResetOnAccountComments(filteredComments, reset)`, `useScrollToTopOnFirstVisible(isVisible, navigationType)`. Both views currently each carry 6-9 near-duplicate effects; consolidating them reduces effect-count at the view layer and makes the remaining view code closer to pure render.
|