Tel 298 switch from contexts to zustand #254
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
alexwhelan12
force-pushed
the
TEL-298-Switch-from-contexts-to-Zustand
branch
from
0b1ecef to
cc00ffb
Compare
alexwhelan12
force-pushed
the
TEL-298-Switch-from-contexts-to-Zustand
branch
from
cc00ffb to
a3a4672
Compare
alexwhelan12
force-pushed
the
TEL-298-Switch-from-contexts-to-Zustand
branch
from
30bd683 to
3561844
Compare
WalkthroughThis PR migrates state management from React Context to Zustand stores. Four context providers are removed and replaced with four store hooks. Five new effect manager components handle side effects previously embedded in providers. Import paths across 40+ components are updated to consume from stores instead of contexts. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/client/src/components/global/SocketManager.tsx (1)
51-111: Clean upconnect/disconnectlisteners to avoid handler accumulationThe issue is confirmed: the
connectanddisconnectlisteners registered in the effect are never removed in the cleanup. They use inline anonymous functions, which cannot be unsubscribed viasocketIO.off()later. IfSocketManageris unmounted and remounted while the socket remains connected, duplicate handlers would accumulate—inconsistent with how the other listeners (onCarConnect,onCarDisconnect,onCarLatency,onLapCoords) are properly handled.Extract
onConnectandonDisconnectasuseCallbackfunctions and unsubscribe them in cleanup to match the established pattern:+ const onConnect = useCallback(() => { + setCurrentAppState((prev) => ({ ...prev, socketConnected: true })); + }, [setCurrentAppState]); + + const onDisconnect = useCallback(() => { + setCurrentAppState((prev) => ({ ...prev, socketConnected: false })); + }, [setCurrentAppState]); + useEffect(() => { socketIO.connect(); const id = setInterval(() => { start.current = Date.now(); socketIO.emit("ping", () => { const duration = Date.now() - (start.current as number); setCurrentAppState((prev) => ({ ...prev, userLatency: duration })); }); }, 10000); socketIO.on("carDisconnect", onCarDisconnect); socketIO.on("carConnect", onCarConnect); socketIO.on("carLatency", onCarLatency); socketIO.on("lapCoords", onLapCoords); - socketIO.on("connect", () => { - setCurrentAppState((prev) => ({ ...prev, socketConnected: true })); - }); - socketIO.on("disconnect", () => { - setCurrentAppState((prev) => ({ ...prev, socketConnected: false })); - }); + socketIO.on("connect", onConnect); + socketIO.on("disconnect", onDisconnect); return () => { socketIO.disconnect(); clearInterval(id); socketIO.off("carDisconnect", onCarDisconnect); socketIO.off("carConnect", onCarConnect); socketIO.off("carLatency", onCarLatency); socketIO.off("lapCoords", onLapCoords); + socketIO.off("connect", onConnect); + socketIO.off("disconnect", onDisconnect); }; - }, [ - onCarConnect, - onCarDisconnect, - onCarLatency, - onLapCoords, - setCurrentAppState, - ]); + }, [ + onCarConnect, + onCarDisconnect, + onCarLatency, + onLapCoords, + onConnect, + onDisconnect, + setCurrentAppState, + ]);packages/client/src/components/molecules/PlaybackMolecules/PlaybackSlider.tsx (1)
63-100: Add missing dependencies to useEffect.The useEffect hook uses
sortedData(line 78) andsetCurrentPacket(line 79) but only includessortedData.lengthin the dependency array. WhilesetCurrentPacketis likely stable andsortedData.lengthchanges whensortedDatachanges, it's safer to explicitly include all used dependencies to avoid potential stale closure issues.Apply this diff:
- }, [PLAYBACK_DURATION, isPlaying, sortedData.length]); + }, [PLAYBACK_DURATION, isPlaying, sortedData, stepSize, setCurrentPacket]);
🧹 Nitpick comments (8)
packages/client/src/components/containers/MLContainer.tsx (1)
8-8: Consider removing the unused import.The
useAppStateimport is not used anywhere in this component. Unless this is being prepared for an upcoming change, it can be safely removed to keep the imports clean.-import { useAppState } from "@/stores/useAppState";packages/client/src/stores/useAppState.ts (1)
3-32: Consider movingIPlaybackDateTimeto a shared types moduleUsing
IPlaybackDateTimehere via an import from a component (PlaybackDatePicker) works because it’s a type‑only import, but it inverts the usual dependency direction (stores depending on UI). For long‑term maintainability, consider extractingIPlaybackDateTimeinto a sharedtypesmodule that both the store andPlaybackDatePickercan import from.packages/client/src/components/containers/BottomInformationContainer.tsx (1)
6-29: Favourites removal works with the new store; consider tightening hook depsThe favourites removal logic still works correctly with
useAppState’ssetCurrentAppState, and keeping localStorage in sync is preserved.You could optionally simplify the
useCallbackdeps to avoid unnecessary re‑creations:const favourites = currentAppState.favourites; const handleRemoveFavourite = useCallback( (favourite: string) => { const newFavourites = favourites.filter((item) => item !== favourite); setCurrentAppState((prev) => ({ ...prev, favourites: newFavourites })); localStorage.setItem("favourites", JSON.stringify(newFavourites)); }, [favourites, setCurrentAppState], );This keeps the dependency list focused on the values actually used.
packages/client/src/components/transformers/PISTransformer.tsx (1)
10-117: Add-to-favourites works with the store; consider deduplicating favourites logicUsing
setCurrentAppStatehere to append to thefavouritesarray (with a max of 8) and syncing to localStorage is consistent with the rest of the app and should behave as before the migration.Since the add/remove favourites logic now exists in both
PISTransformerandBottomInformationContainer, you might later want to extract a small helper (e.g.updateFavourites(add/remove)or a custom hook) to keep this behavior centralized.packages/client/src/stores/useLapData.ts (1)
7-27: Lap data store and formatter look correct; consider keeping display formatting closer to UI.The store +
formatLapDataimplementation looks solid: the rounding and array updates are correct, and the fetch flow + notification on failure is reasonable for this PR’s scope.If you ever need to sort/filter by timestamp or support non‑US locales, it might be worth leaving
timeStampas aDate/ISO string in the store and doingtoLocaleStringat render time instead, but that’s an optional future refactor rather than a blocker.Also applies to: 38-68
packages/client/src/components/molecules/HeroMolecules/CarGraphicComponent.tsx (1)
30-30: Consider removing unused variable.The
currentAppStateis retrieved from the store but doesn't appear to be used anywhere in the component. If it's not needed for future functionality, consider removing it to keep the code clean.Apply this diff if not needed:
- const { currentAppState } = useAppState();packages/client/src/components/global/LapDataListenerManager.tsx (1)
56-58: Radio mode handling is incomplete.The radio connection type has a TODO placeholder. This is acceptable for now as the other connection types are properly implemented.
Would you like me to help implement the radio mode handling, or is this planned for a future iteration?
packages/client/src/components/global/PacketListenerManager.tsx (1)
10-13: Limit Zustand subscription scope to the setter
Subscribing to the whole packet store here meansPacketListenerManagerre-renders every time telemetry updates, even though the component only needs the setter. Given packet frequency, that’s a lot of wasted work. Please grab just the setter with a selector (orusePacketStore.getState) so this component stays render-free while still dispatching updates.- const { setCurrentPacket } = usePacketStore(); + const setCurrentPacket = usePacketStore( + (state) => state.setCurrentPacket, + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
docs/CLIENT.md(0 hunks)packages/client/src/components/atoms/BatteryIcon.tsx(1 hunks)packages/client/src/components/atoms/SpeedAtom.tsx(1 hunks)packages/client/src/components/atoms/ThrottleIcon.tsx(1 hunks)packages/client/src/components/containers/BottomInformationContainer.tsx(1 hunks)packages/client/src/components/containers/MLContainer.tsx(1 hunks)packages/client/src/components/containers/MapContainer.tsx(2 hunks)packages/client/src/components/global/AppStateEffectsManager.tsx(1 hunks)packages/client/src/components/global/EffectsProvider.tsx(1 hunks)packages/client/src/components/global/LapDataListenerManager.tsx(1 hunks)packages/client/src/components/global/Loading.tsx(1 hunks)packages/client/src/components/global/LoadingWrapper.tsx(1 hunks)packages/client/src/components/global/PacketListenerManager.tsx(1 hunks)packages/client/src/components/global/SocketManager.tsx(5 hunks)packages/client/src/components/molecules/HeroMolecules/CarGraphicComponent.tsx(2 hunks)packages/client/src/components/molecules/HeroMolecules/GearParkBrakeComponent.tsx(1 hunks)packages/client/src/components/molecules/LogoStatusMolecules/DataPickerMolecules/DatePickerColumn.tsx(1 hunks)packages/client/src/components/molecules/LogoStatusMolecules/PlaybackDatePicker.tsx(2 hunks)packages/client/src/components/molecules/LogoStatusMolecules/SettingsComponent.tsx(1 hunks)packages/client/src/components/molecules/LogoStatusMolecules/StatusComponent.tsx(2 hunks)packages/client/src/components/molecules/MapMolecules/Map.tsx(1 hunks)packages/client/src/components/molecules/MapMolecules/MapControls.tsx(1 hunks)packages/client/src/components/molecules/MapMolecules/MapText.tsx(1 hunks)packages/client/src/components/molecules/PlaybackMolecules/PlaybackSlider.tsx(2 hunks)packages/client/src/components/tabs/AnalysisTab.tsx(1 hunks)packages/client/src/components/tabs/RaceTab.tsx(3 hunks)packages/client/src/components/transformers/PISTransformer.tsx(1 hunks)packages/client/src/contexts/AppStateContext.tsx(0 hunks)packages/client/src/contexts/LapDataContext.tsx(0 hunks)packages/client/src/contexts/PacketContext.tsx(0 hunks)packages/client/src/contexts/PlayBackContext.tsx(0 hunks)packages/client/src/hooks/PIS/PIS.battery.tsx(1 hunks)packages/client/src/hooks/PIS/PIS.faults.tsx(1 hunks)packages/client/src/hooks/PIS/PIS.mbms.tsx(1 hunks)packages/client/src/hooks/PIS/PIS.motor.tsx(1 hunks)packages/client/src/hooks/PIS/PIS.mppt.tsx(1 hunks)packages/client/src/hooks/PIS/useUnitsHandler.tsx(1 hunks)packages/client/src/pages/[[...slug]].tsx(1 hunks)packages/client/src/pages/_app.tsx(2 hunks)packages/client/src/stores/useAppState.ts(1 hunks)packages/client/src/stores/useLapData.ts(1 hunks)packages/client/src/stores/usePacket.ts(1 hunks)packages/client/src/stores/usePlayback.ts(1 hunks)
💤 Files with no reviewable changes (5)
- docs/CLIENT.md
- packages/client/src/contexts/AppStateContext.tsx
- packages/client/src/contexts/PacketContext.tsx
- packages/client/src/contexts/LapDataContext.tsx
- packages/client/src/contexts/PlayBackContext.tsx
🔇 Additional comments (26)
packages/client/src/stores/usePlayback.ts (1)
5-13: Playback store implementation is clean and type‑safeThe shape of
IPlaybackStoreand thecreatecall are consistent and minimal. ReplacingplaybackDataimmutably viasetPlaybackDatashould work well with selector-based consumers.If you later add more actions, keeping this “single source of truth” pattern (all mutations through the store) will help avoid diverging playback state between components.
packages/client/src/stores/useAppState.ts (1)
39-65: App state store shape and updater pattern look solidThe
currentAppStatedefault covers all fields inIAppState, and thesetCurrentAppState(updater)API mirrors React’s functionalsetState, which is nice and predictable. As long as call sites always doprev => ({ ...prev, field: newValue }), this will remain safe and future‑proof if you add more fields toIAppState.One small enhancement you might consider later is exporting the
IAppStatetype for reuse in strongly‑typed selectors or effects.packages/client/src/components/molecules/LogoStatusMolecules/SettingsComponent.tsx (1)
4-18: Store migration for app settings looks consistentSwitching to
{ APPUNITS, CONNECTIONTYPES, useAppState }from the store keeps the existing usage pattern (currentAppState,setCurrentAppState) intact and matches the new app state API, so the settings logic should behave the same as before.packages/client/src/components/tabs/AnalysisTab.tsx (1)
6-8: Analysis tab import aligns with the new app state storeImporting
useAppStatefrom the store is consistent with the broader migration away from contexts. There’s no behavioral change in this file yet; if you don’t end up needing app state here, you can safely remove the import later.packages/client/src/components/containers/MapContainer.tsx (1)
5-22: Review comment is incorrect; store initialization prevents the reported issueThe store always initializes
currentPacketwithgenerateFakeTelemetryData(), which populatesTelemetry.GpsLatitudeandTelemetry.GpsLongitude. There is no pathway wherecurrentPacket.Telemetryis undefined when accessed at lines 48–49, eliminating the runtime concern cited in the review.Likely an incorrect or invalid review comment.
packages/client/src/components/global/Loading.tsx (1)
6-6: Store hook swap inLoadingis consistent with the new architecture.Importing
useAppStatefrom the store module while keeping the hook usage unchanged keeps this component aligned with the Zustand migration without changing behavior.packages/client/src/components/molecules/LogoStatusMolecules/DataPickerMolecules/DatePickerColumn.tsx (1)
3-3: Playback date picker now correctly targets the app-state store.Switching
useAppStateto the store module while keeping thesetCurrentAppStatefunctional updater maintains existing behavior and fits the new state management approach.Also applies to: 73-80
packages/client/src/components/molecules/MapMolecules/Map.tsx (1)
20-22: Map module is now wired to the app-state store API.Adding the
useAppStateimport here keeps this module consistent with the broader move to store-based state and prepares it for any future state integration without affecting current behavior.packages/client/src/components/molecules/MapMolecules/MapControls.tsx (1)
7-9: Map controls module is aligned with the app-state store setup.Importing
useAppStatefrom the store keeps this file consistent with the rest of the migration, without altering current behavior in the controls component.packages/client/src/hooks/PIS/PIS.motor.tsx (1)
5-8: LGTM! Clean migration to Zustand store.The switch from
usePacket()tousePacketStore()follows the consistent pattern across this migration, and thecurrentPacketusage remains unchanged.packages/client/src/hooks/PIS/PIS.mppt.tsx (1)
3-6: LGTM! Consistent store migration.The migration to
usePacketStore()is correctly implemented with no logic changes.packages/client/src/components/molecules/MapMolecules/MapText.tsx (1)
4-4: LGTM! SocketManager import updated correctly.The
socketIOimport path has been successfully migrated to the newSocketManagerlocation.packages/client/src/hooks/PIS/useUnitsHandler.tsx (1)
2-10: LGTM! Store migration executed correctly.Both
useAppStateandAPPUNITSare now sourced from the Zustand store, with no changes to the unit handling logic.packages/client/src/hooks/PIS/PIS.faults.tsx (1)
6-9: LGTM! Packet store migration is consistent.The migration from
usePacket()tousePacketStore()follows the same pattern as other PIS hooks in this PR.packages/client/src/components/atoms/SpeedAtom.tsx (1)
5-9: LGTM! Store integration looks good.The
usePacketStore()migration maintains the existing speed calculation logic without any behavioral changes.packages/client/src/components/global/EffectsProvider.tsx (1)
1-15: LGTM! Clean orchestration of effect managers.The
EffectsProvidercorrectly composes the four effect manager components. The fragment wrapper and ordering look appropriate for initializing side effects.packages/client/src/components/molecules/HeroMolecules/GearParkBrakeComponent.tsx (1)
2-5: LGTM! Final component migration completed.The
usePacketStore()integration is consistent with the migration pattern throughout this PR.packages/client/src/hooks/PIS/PIS.battery.tsx (1)
8-11: LGTM! Clean migration to store-based state.The migration from context-based
usePacket()to store-basedusePacketStore()is correctly implemented. The hook properly destructurescurrentPacketand the rest of the logic remains unchanged.packages/client/src/components/molecules/PlaybackMolecules/PlaybackSlider.tsx (1)
7-24: LGTM! Store migration correctly implemented.The migration from context-based hooks to store-based hooks (
usePacketStoreandusePlaybackStore) is properly implemented. The selector pattern on lines 22-24 is a good practice for extracting only the needed state.packages/client/src/components/global/LoadingWrapper.tsx (1)
1-80: LGTM! Clean migration to store-based state.The migration from context-based
useAppStateto the store-based version is straightforward and correctly implemented. All logic remains consistent with only the import source changing.packages/client/src/components/atoms/ThrottleIcon.tsx (1)
1-6: LGTM! Store migration correctly implemented.The migration to
usePacketStoreis clean and straightforward. The component correctly destructurescurrentPacketand maintains its existing behavior.packages/client/src/hooks/PIS/PIS.mbms.tsx (1)
4-7: LGTM! Clean migration to store-based state.The migration from context-based to store-based
currentPacketaccess is correctly implemented. The hook properly usesusePacketStore()and maintains consistent usage throughout.packages/client/src/components/atoms/BatteryIcon.tsx (1)
1-5: LGTM! Store migration correctly implemented.The component correctly migrates to
usePacketStore()for accessingcurrentPacket. The implementation is clean and maintains the existing behavior.packages/client/src/components/molecules/HeroMolecules/CarGraphicComponent.tsx (1)
13-30: LGTM! Store migration correctly implemented.The migration to store-based hooks (
usePacketStoreanduseAppState) is properly done. The component correctly destructures the needed state.packages/client/src/components/global/LapDataListenerManager.tsx (2)
14-16: LGTM! Initial data fetch correctly implemented.The initial lap data fetch on component mount is properly implemented with the correct dependency array.
19-64: LGTM! Connection type handling correctly implemented.The effect properly handles different connection types:
- Playback mode: clears lap data
- Network mode: sets up socket listeners with proper cleanup
- Demo mode: clears lap data
- Radio mode: has a TODO placeholder
The socket listener cleanup in the return function is correctly implemented, and the dependency array is complete.
| useEffect(() => { | ||
| setTimeout(() => { | ||
| setCurrentAppState((prev) => ({ | ||
| ...prev, | ||
| loading: false, | ||
| })); | ||
| }, 5000); | ||
| }, [setCurrentAppState]); |
There was a problem hiding this comment.
Unconditionally clearing loading masks real connection failures.
This effect always flips loading to false after five seconds, even if socketConnected/radioConnected are still false. As soon as a connection stalls, the spinner disappears forever and currentAppState.loading reports a success that never happened. Guard the timeout so it only clears loading after a successful connection (or cancel it when connectivity is still down), and ensure you clear the timer on unmount.
🤖 Prompt for AI Agents
In packages/client/src/components/global/AppStateEffectsManager.tsx around lines
46 to 53, the effect unconditionally clears loading after 5s which masks real
connection failures; change it to start the timeout only when a successful
connection exists (e.g. socketConnected || radioConnected), include
socketConnected and radioConnected in the dependency array, store the timer id
in a ref/local variable and clearTimeout in the effect cleanup so the timer is
canceled on unmount or when connectivity changes, and ensure you only call
setCurrentAppState to set loading: false if the connectivity check still passes
when the timer fires.
| import { socketIO } from "@/components/global/SocketManager"; | ||
| import useUnitsHandler from "@/hooks/PIS/useUnitsHandler"; | ||
| import { UnitType } from "@/objects/PIS/PIS.interface"; | ||
| import { useAppState } from "@/stores/useAppState"; |
There was a problem hiding this comment.
Remove unused import.
The useAppState import is not used anywhere in this component.
Apply this diff:
-import { useAppState } from "@/stores/useAppState";🤖 Prompt for AI Agents
In packages/client/src/components/molecules/MapMolecules/MapText.tsx around line
7, the import "useAppState" is unused in the component; remove the unused import
line to clean up the code and avoid linter warnings, ensuring any needed
functionality is preserved or imported from the correct module if required
elsewhere.
Summary by CodeRabbit
Refactor
Documentation