fix: unify gateway recovery overlay and polish OpenClaw copy#45
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a GatewayRecoveryOverlay component to manage and display the UI during gateway recovery or failure. It includes state logic, updated translations—standardizing terminology to 'OpenClaw'—and comprehensive unit tests. A critical bug was identified in src/lib/host-events.ts where an event listener is registered twice, potentially causing duplicate event execution.
src/lib/host-events.ts
Outdated
| ipc.on(ipcChannel, listener); | ||
| // preload's `on()` wraps the callback in an internal subscription function | ||
| // and returns a cleanup function that removes that exact wrapper. We MUST | ||
| // use the returned cleanup rather than calling `off(channel, listener)`, | ||
| // because `listener` !== the internal wrapper and removeListener would be | ||
| // a no-op, leaking the subscription. | ||
| const unsubscribe = ipc.on(ipcChannel, listener); | ||
| if (typeof unsubscribe === 'function') { | ||
| return unsubscribe; | ||
| } | ||
| // Fallback for environments where on() doesn't return cleanup | ||
| return () => { | ||
| ipc.off(ipcChannel, listener); | ||
| }; |
There was a problem hiding this comment.
The ipc.on(ipcChannel, listener) call is made twice, once on line 51 and again on line 57. This will cause the event handler to be registered twice, and consequently, it will be executed twice for every event, which can lead to unexpected behavior and bugs.
The first call on line 51 appears to be a leftover from the previous implementation and should be removed. The correct implementation should only register the listener once.
// preload's `on()` wraps the callback in an internal subscription function
// and returns a cleanup function that removes that exact wrapper. We MUST
// use the returned cleanup rather than calling `off(channel, listener)`,
// because `listener` !== the internal wrapper and removeListener would be
// a no-op, leaking the subscription.
const unsubscribe = ipc.on(ipcChannel, listener);
if (typeof unsubscribe === 'function') {
return unsubscribe;
}
// Fallback for environments where on() doesn't return cleanup
return () => {
ipc.off(ipcChannel, listener);
};|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new GatewayRecoveryOverlay component and associated state management logic to handle UI feedback when the OpenClaw gateway is restarting or fails to recover. It includes comprehensive unit tests for the recovery flow, updates to Chinese translations for consistency (renaming "Gateway" to "OpenClaw Service"), and a refactor of the host event subscription logic to better handle cleanup. The review feedback identifies unrelated layout changes in the update dialog that should be reverted to maintain scope.
|
|
||
| {status === 'downloading' && progress && ( | ||
| <div className="mt-4 rounded-2xl border border-black/6 bg-background/70 px-4 py-3 text-sm text-muted-foreground dark:border-white/10"> | ||
| <div className="mt-4 rounded-2xl border border-black/6 bg-background/70 px-4 py-3 mr-5 text-sm text-muted-foreground dark:border-white/10"> |
There was a problem hiding this comment.
The mr-5 class was added here, but it seems unrelated to the main goals of this pull request, which are focused on the gateway recovery overlay and i18n copy. This might be an unintentional change that could affect the layout. If this margin is not required, it would be best to remove it to avoid unintended side effects.
| <div className="mt-4 rounded-2xl border border-black/6 bg-background/70 px-4 py-3 mr-5 text-sm text-muted-foreground dark:border-white/10"> | |
| <div className="mt-4 rounded-2xl border border-black/6 bg-background/70 px-4 py-3 text-sm text-muted-foreground dark:border-white/10"> |
|
|
||
| {status === 'downloaded' && autoInstallCountdown != null && autoInstallCountdown >= 0 && ( | ||
| <div className="mt-4 rounded-2xl border border-black/6 bg-background/70 px-4 py-3 text-sm text-muted-foreground dark:border-white/10"> | ||
| <div className="mt-4 rounded-2xl border border-black/6 bg-background/70 px-4 py-3 mr-5 text-sm text-muted-foreground dark:border-white/10"> |
There was a problem hiding this comment.
Similar to the change above, the mr-5 class was added here. This seems unrelated to the main goals of this pull request. If this margin is not required, it would be best to remove it to avoid unintended layout side effects.
| <div className="mt-4 rounded-2xl border border-black/6 bg-background/70 px-4 py-3 mr-5 text-sm text-muted-foreground dark:border-white/10"> | |
| <div className="mt-4 rounded-2xl border border-black/6 bg-background/70 px-4 py-3 text-sm text-muted-foreground dark:border-white/10"> |
Summary:
Validation: