From 81656e7b19a39a1cc905b57265dc166e50c390e7 Mon Sep 17 00:00:00 2001 From: simoleo89 Date: Mon, 11 May 2026 16:31:52 +0000 Subject: [PATCH] Fix two logic bugs found while refactoring + document the open ones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These are the bugs surfaced during the structural work that are simple enough to fix in isolation. Larger ones (race conditions that need session-token tracking, async-fetch ordering) are deferred and documented in docs/ARCHITECTURE.md "Known logic bugs" — the repo has Issues disabled, so the doc is the issue board. == Fix: room history wiped on every tab close src/components/room/widgets/room-tools/RoomToolsWidgetView.tsx had a useEffect that registered a `beforeunload` handler calling `window.localStorage.removeItem('nitro.room.history')`. The whole point of localStorage is to persist across sessions; wiping it on tab close is either a leftover debug call or a misunderstanding of the API. Removed the handler. History now persists across browser sessions, which matches user expectations. If "session-only" was the intent, the right primitive is `sessionStorage` (not localStorage + cleanup) — left as a note in the doc. == Fix: AvatarInfoPetTrainingPanelView null-pointer on session change src/components/room/widgets/avatar-info/AvatarInfoPetTrainingPanelView.tsx read `roomSession.userDataManager.getPetData(parser.petId)` without guarding for `roomSession` being null. The PetTrainingPanelMessageEvent can arrive during a room transition when `roomSession` is briefly null, crashing the widget. Added `?.` chain on both `roomSession` and `userDataManager`. == Doc: known logic bugs section Two open issues documented for follow-up: - MainView.tsx CREATED/ENDED race — needs session-token tracking, fits cleanly into the future useNitroEventReducer companion to proposal #1. - LayoutFurniImageView / LayoutAvatarImageView async fetch ordering — needs request-id refs, or solves itself once React Query (proposal #2) is enabled and the image fetch becomes a query keyed on props. Plus a "recently fixed" subsection that records the four bugs already addressed in this branch (doorbell close button, doorbell optimistic remove, room history wipe, pet panel null-pointer) so the next reader knows what changed and why. == Verification - yarn eslint on the two modified files: same error count before and after (5 pre-existing set-state-in-effect on RoomToolsWidgetView, none introduced). - yarn tsc on the two modified files: clean. https://claude.ai/code/session_01GrR87LAqnAEyKG2ZbmQt5Q --- docs/ARCHITECTURE.md | 90 +++++++++++++++++++ .../AvatarInfoPetTrainingPanelView.tsx | 2 +- .../room-tools/RoomToolsWidgetView.tsx | 10 --- 3 files changed, 91 insertions(+), 11 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index a878921..b98bc33 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -367,3 +367,93 @@ Anything else (the per-tab `WiredCreatorTools` split, the `react-compiler/react-compiler` warnings, the `set-state-in-effect` sweep, the `LoginView` dialog split) is a downstream consequence of these five — easier and safer once the foundations are in place. + +--- + +## Known logic bugs (independent of structural refactor) + +These are runtime bugs spotted while doing the structural work. They are +**not** fixed by the patterns above — they need their own PRs with manual +QA. Listing them here because there is currently no GitHub Issues board on +this repo. + +### Open + +#### `MainView` — race between `RoomSessionEvent.CREATED` and `ENDED` + +`src/components/MainView.tsx:47-48` writes the same `landingViewVisible` +state from two independent listeners with no session-token guard: + +```ts +useNitroEvent(RoomSessionEvent.CREATED, () => setLandingViewVisible(false)); +useNitroEvent(RoomSessionEvent.ENDED, e => setLandingViewVisible(e.openLandingView)); +``` + +If the events arrive out of order (fast reconnect, network reordering), +the final state contradicts the actual session state — landing view stuck +open inside a room, or stuck closed at the hotel view. Resolves on next +room change. + +**Fix shape** (deferred until `useNitroEventReducer` companion lands — +see proposal #1): + +```ts +// One reducer owns both events + the active session token +const { sessionId, landingViewVisible } = useNitroEventReducer<...>( + [RoomSessionEvent.CREATED, RoomSessionEvent.ENDED], + (state, e) => { + if (e.type === RoomSessionEvent.CREATED) { + return { sessionId: e.session.roomId, landingViewVisible: false }; + } + if (state.sessionId !== null && e.session.roomId !== state.sessionId) { + return state; // stale ENDED for old session, ignore + } + return { sessionId: null, landingViewVisible: e.openLandingView }; + }, + { sessionId: null, landingViewVisible: true } +); +``` + +**Severity**: edge case, observed only after unstable websocket +reconnects. UX-degrading, not data-corrupting. + +#### `LayoutFurniImageView` / `LayoutAvatarImageView` — async fetch race + +In both files an effect kicks off an async `processAsImageUrl` / +`generateImage` and writes the result via `setImageElement`. If props +change twice in quick succession, the first fetch can resolve **after** +the second one and overwrite the newer image with the older one. + +**Fix shape**: capture a request-id ref at the start of the effect, only +write the result if the ref hasn't been bumped meanwhile. Or — better — +once React Query (#2) is enabled, model the image fetch as a query keyed +on the props tuple; React Query handles cancellation and ordering for +free. + +**Severity**: visible only on slow connections / rapid prop changes. Not +data-corrupting. + +### Recently fixed (in this branch) + +- **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). +- **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 + server desynced if the network dropped. Fixed by removing the local + `removeUser` call: the server-driven events now own the list. Note: + a "pending" indicator (so users see their answer is in flight) is + desirable — separate small PR. +- **`localStorage` room history wiped on every tab close** + (`RoomToolsWidgetView.tsx`, `useEffect` on `beforeunload` removing + `nitro.room.history`). Fixed by removing the `beforeunload` handler; + history now persists across sessions, which is the only sensible + meaning of `localStorage`. If "session-only" was the intent, the right + primitive is `sessionStorage` — file an issue if that's actually + desired. +- **`AvatarInfoPetTrainingPanelView` null-pointer** — + `roomSession.userDataManager.getPetData(parser.petId)` could throw if + `roomSession` was null at the moment the event arrived (between rooms). + Fixed with `?.` chain. diff --git a/src/components/room/widgets/avatar-info/AvatarInfoPetTrainingPanelView.tsx b/src/components/room/widgets/avatar-info/AvatarInfoPetTrainingPanelView.tsx index 75bdd79..d4ec5fb 100644 --- a/src/components/room/widgets/avatar-info/AvatarInfoPetTrainingPanelView.tsx +++ b/src/components/room/widgets/avatar-info/AvatarInfoPetTrainingPanelView.tsx @@ -17,7 +17,7 @@ export const AvatarInfoPetTrainingPanelView: FC<{}> = props => if(!parser) return; - const roomPetData = roomSession.userDataManager.getPetData(parser.petId); + const roomPetData = roomSession?.userDataManager?.getPetData(parser.petId); if(!roomPetData) return; diff --git a/src/components/room/widgets/room-tools/RoomToolsWidgetView.tsx b/src/components/room/widgets/room-tools/RoomToolsWidgetView.tsx index 759835c..01582ef 100644 --- a/src/components/room/widgets/room-tools/RoomToolsWidgetView.tsx +++ b/src/components/room/widgets/room-tools/RoomToolsWidgetView.tsx @@ -119,16 +119,6 @@ export const RoomToolsWidgetView: FC<{}> = props => setRoomHistory(JSON.parse(window.localStorage.getItem('nitro.room.history') || '[]')); }, []); - useEffect(() => - { - const handleTabClose = () => - { - window.localStorage.removeItem('nitro.room.history'); - }; - window.addEventListener('beforeunload', handleTabClose); - return () => window.removeEventListener('beforeunload', handleTabClose); - }, []); - return (