fix: connection status map wrapped in atom #1783
Conversation
This wraps the connection status map in an atom so it properly updates when an individual connection status changes.
WalkthroughThe pull request introduces a significant change in connection status management within the global state of the frontend application. The modification transitions from using a direct This change fundamentally alters how connection status data is managed, moving from a mutable data structure to a reactive atom-based approach. The new implementation creates a Jotai atom that holds a The update impacts how connection statuses are retrieved, created, and stored, with the core logic now centered around manipulating the new ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/app/store/global.ts (1)
26-26: Consider usingatomFamilyfor managing connection statusesCurrently,
ConnStatusMapAtomwraps a mutableMapwithin an atom. While you clone theMapbefore updates to maintain immutability, this can become inefficient as the number of connections grows. UtilizingatomFamilyfrom Jotai allows you to create atoms dynamically keyed by connection names, simplifying state management and enhancing performance by avoiding the need to clone the entireMap.To implement this, you can define an
atomFamilyfor connection statuses:import { atomFamily } from 'jotai/utils'; const connStatusAtomFamily = atomFamily((conn: string) => atom<ConnStatus>({ connection: conn, connected: false, error: null, status: "disconnected", hasconnected: false, activeconnnum: 0, wshenabled: false, }) );Then, adjust your
getConnStatusAtomfunction to useconnStatusAtomFamily:-function getConnStatusAtom(conn: string): PrimitiveAtom<ConnStatus> { - const connStatusMap = globalStore.get(ConnStatusMapAtom); - let rtn = connStatusMap.get(conn); - if (rtn == null) { - // ...initialize rtn... - const newConnStatusMap = new Map(connStatusMap); - newConnStatusMap.set(conn, rtn); - globalStore.set(ConnStatusMapAtom, newConnStatusMap); - } - return rtn; +function getConnStatusAtom(conn: string): PrimitiveAtom<ConnStatus> { + return connStatusAtomFamily(conn); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/store/global.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (1)
frontend/app/store/global.ts (1)
Line range hint
583-612: Avoid cloning the entireMapon each updateIn the
getConnStatusAtomfunction, cloning theconnStatusMapwhenever a new connection is added can lead to performance issues as the number of connections increases. This concern is addressed by the previous suggestion to useatomFamily, which eliminates the need to manage aMapand clone it on every update.
This wraps the connection status map in an atom so it properly updates when an individual connection status changes.