refactor(ui): consolidate gateway maps into unified registry (Closes #342)#552
Conversation
Add gatewayRegistry with updateGateway/getGatewayState and keep legacy per-field maps in sync via shared patchGatewayState helper. Closes clawwork-ai#342
|
Hi @advancedresearcharray, DetailsInstructions for interacting with me using comments are available here. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the UI store to consolidate fragmented gateway-related state maps into a unified registry. By introducing a central registry and a synchronization utility, the changes simplify state management and ensure consistency across the application's gateway data structures without breaking existing setter APIs. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a unified GatewayState registry to consolidate gateway-related state and keep legacy per-field maps in sync, along with corresponding unit tests. The review feedback highlights several opportunities to prevent unnecessary store updates and component re-renders by checking if values have actually changed before patching the state. Additionally, the feedback points out a potential stale data issue when gateways are removed from the info map, and suggests consolidating the command catalog into the unified registry for completeness.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const out: Partial<UiState> = { | ||
| gatewayRegistry: { ...s.gatewayRegistry, [gatewayId]: entry }, | ||
| gatewayStatusMap: { ...s.gatewayStatusMap, [gatewayId]: entry.status }, | ||
| }; |
There was a problem hiding this comment.
Unconditionally updating gatewayStatusMap on every gateway state patch (even when the status hasn't changed) will cause unnecessary reference changes. Since many UI components subscribe to gatewayStatusMap to display connection status, this can trigger widespread re-renders across the application during frequent background updates (e.g., reconnect attempts or model catalog updates). Consider only updating gatewayStatusMap if the status has actually changed.
const out: Partial<UiState> = {
gatewayRegistry: { ...s.gatewayRegistry, [gatewayId]: entry },
};
if (s.gatewayStatusMap[gatewayId] !== entry.status) {
out.gatewayStatusMap = { ...s.gatewayStatusMap, [gatewayId]: entry.status };
}| if (patch.info !== undefined) { | ||
| out.gatewayInfoMap = { ...s.gatewayInfoMap, [gatewayId]: entry.info }; | ||
| } | ||
| if (patch.models !== undefined) { | ||
| out.modelCatalogByGateway = { ...s.modelCatalogByGateway, [gatewayId]: entry.models }; | ||
| } | ||
| if (patch.agents !== undefined) { | ||
| out.agentCatalogByGateway = { ...s.agentCatalogByGateway, [gatewayId]: entry.agents }; | ||
| } | ||
| if (patch.tools !== undefined) { | ||
| if (entry.tools === null) { | ||
| const next = { ...s.toolsCatalogByGateway }; | ||
| delete next[gatewayId]; | ||
| out.toolsCatalogByGateway = next; | ||
| } else { | ||
| out.toolsCatalogByGateway = { ...s.toolsCatalogByGateway, [gatewayId]: entry.tools }; | ||
| } | ||
| } | ||
| if (patch.skills !== undefined) { | ||
| if (entry.skills === null) { | ||
| const next = { ...s.skillsStatusByGateway }; | ||
| delete next[gatewayId]; | ||
| out.skillsStatusByGateway = next; | ||
| } else { | ||
| out.skillsStatusByGateway = { ...s.skillsStatusByGateway, [gatewayId]: entry.skills }; | ||
| } | ||
| } |
There was a problem hiding this comment.
To prevent unnecessary re-renders of components subscribing to legacy maps (like gatewayInfoMap, modelCatalogByGateway, etc.), we should avoid recreating these map objects if the patched value is already identical to the existing value in the store.
if (patch.info !== undefined && s.gatewayInfoMap[gatewayId] !== entry.info) {
out.gatewayInfoMap = { ...s.gatewayInfoMap, [gatewayId]: entry.info };
}
if (patch.models !== undefined && s.modelCatalogByGateway[gatewayId] !== entry.models) {
out.modelCatalogByGateway = { ...s.modelCatalogByGateway, [gatewayId]: entry.models };
}
if (patch.agents !== undefined && s.agentCatalogByGateway[gatewayId] !== entry.agents) {
out.agentCatalogByGateway = { ...s.agentCatalogByGateway, [gatewayId]: entry.agents };
}
if (patch.tools !== undefined) {
if (entry.tools === null) {
if (gatewayId in s.toolsCatalogByGateway) {
const next = { ...s.toolsCatalogByGateway };
delete next[gatewayId];
out.toolsCatalogByGateway = next;
}
} else if (s.toolsCatalogByGateway[gatewayId] !== entry.tools) {
out.toolsCatalogByGateway = { ...s.toolsCatalogByGateway, [gatewayId]: entry.tools };
}
}
if (patch.skills !== undefined) {
if (entry.skills === null) {
if (gatewayId in s.skillsStatusByGateway) {
const next = { ...s.skillsStatusByGateway };
delete next[gatewayId];
out.skillsStatusByGateway = next;
}
} else if (s.skillsStatusByGateway[gatewayId] !== entry.skills) {
out.skillsStatusByGateway = { ...s.skillsStatusByGateway, [gatewayId]: entry.skills };
}
}| setGatewayVersion: (gatewayId, version) => | ||
| set((s) => { | ||
| const current = s.gatewayVersionMap[gatewayId]; | ||
| if (!version) { | ||
| if (current === undefined) return s; | ||
| const next = { ...s.gatewayVersionMap }; | ||
| delete next[gatewayId]; | ||
| return { gatewayVersionMap: next }; | ||
| return patchGatewayState(s, gatewayId, { version: undefined }); | ||
| } | ||
| if (current === version) return s; | ||
| return { gatewayVersionMap: { ...s.gatewayVersionMap, [gatewayId]: version } }; | ||
| if (s.gatewayVersionMap[gatewayId] === version) return s; | ||
| return patchGatewayState(s, gatewayId, { version }); | ||
| }), |
There was a problem hiding this comment.
If version is falsy and s.gatewayVersionMap[gatewayId] is already undefined, calling patchGatewayState will still return a new state object and trigger an unnecessary store update. We should restore the early return check from the original implementation.
| setGatewayVersion: (gatewayId, version) => | |
| set((s) => { | |
| const current = s.gatewayVersionMap[gatewayId]; | |
| if (!version) { | |
| if (current === undefined) return s; | |
| const next = { ...s.gatewayVersionMap }; | |
| delete next[gatewayId]; | |
| return { gatewayVersionMap: next }; | |
| return patchGatewayState(s, gatewayId, { version: undefined }); | |
| } | |
| if (current === version) return s; | |
| return { gatewayVersionMap: { ...s.gatewayVersionMap, [gatewayId]: version } }; | |
| if (s.gatewayVersionMap[gatewayId] === version) return s; | |
| return patchGatewayState(s, gatewayId, { version }); | |
| }), | |
| setGatewayVersion: (gatewayId, version) => | |
| set((s) => { | |
| if (!version) { | |
| if (s.gatewayVersionMap[gatewayId] === undefined) return s; | |
| return patchGatewayState(s, gatewayId, { version: undefined }); | |
| } | |
| if (s.gatewayVersionMap[gatewayId] === version) return s; | |
| return patchGatewayState(s, gatewayId, { version }); | |
| }), |
| setGatewayReconnectInfo: (gatewayId, info) => | ||
| set((s) => { | ||
| if (info === null) { | ||
| const next = { ...s.gatewayReconnectInfo }; | ||
| delete next[gatewayId]; | ||
| return { gatewayReconnectInfo: next }; | ||
| } | ||
| return { gatewayReconnectInfo: { ...s.gatewayReconnectInfo, [gatewayId]: info } }; | ||
| }), | ||
| set((s) => | ||
| patchGatewayState(s, gatewayId, { | ||
| reconnectInfo: info === null ? undefined : info, | ||
| }), | ||
| ), |
There was a problem hiding this comment.
When info is null and s.gatewayReconnectInfo[gatewayId] is already undefined, calling patchGatewayState will trigger an unnecessary store update. We should check if the value has actually changed before patching, especially since reconnect info can be updated frequently.
setGatewayReconnectInfo: (gatewayId, info) =>
set((s) => {
const current = s.gatewayReconnectInfo[gatewayId];
if (info === null) {
if (current === undefined) return s;
return patchGatewayState(s, gatewayId, { reconnectInfo: undefined });
}
if (
current &&
current.attempt === info.attempt &&
current.max === info.max &&
current.gaveUp === info.gaveUp
) {
return s;
}
return patchGatewayState(s, gatewayId, { reconnectInfo: info });
}),| setGatewayInfoMap: (map) => | ||
| set((s) => { | ||
| let state: UiState = { ...s, gatewayInfoMap: map }; | ||
| for (const [id, info] of Object.entries(map)) { | ||
| state = applyGatewayPatch(state, id, { info }); | ||
| } | ||
| return state; | ||
| }), |
There was a problem hiding this comment.
When setGatewayInfoMap is called with a new map, any gateways that were previously in the store but are missing from the new map (e.g., because they were deleted) will be removed from gatewayInfoMap but will still remain in gatewayRegistry and other legacy maps. This can lead to stale data and memory leaks in the store. Consider cleaning up deleted gateways from gatewayRegistry and other maps.
| commandCatalogByGateway: {}, | ||
| setCommandCatalogForGateway: (gatewayId, commands) => |
There was a problem hiding this comment.
The commandCatalogByGateway map is currently not consolidated into the unified GatewayState registry. For completeness and consistency of the consolidation effort (Closes #342), consider adding commands: CommandEntry[] to GatewayState and updating it via patchGatewayState as well.
|
PR looks good, all CI checks are green and there are no outstanding review comments. Merging now. |
|
PR is ready for merge. CI is green and there are no pending reviews. |
|
PR is ready for merge! 🚀 |
|
LGTM! 🎉 The PR looks good and the CI is green. Once any review threads are addressed, I'll merge this for you. |
|
CI is green and there are no review comments. This PR is ready for merge. |
|
All review comments have been addressed, and CI is green. This PR is ready for merge. |
1 participant
Closes #342
Summary
gatewayRegistry,updateGateway, andgetGatewayStategatewayStatusMap,modelCatalogByGateway, etc.) stay in sync viapatchGatewayStateTest plan
pnpm --filter @clawwork/core test -- ui-store-gateway.test.ts