Experiment: auto reload settings by naps62 · Pull Request #73 · ethui/extension

naps62 · 2026-01-28T09:57:14Z

No description provided.

Copilot

Pull request overview

This PR implements automatic settings reload functionality for the ethui browser extension. The main change replaces the static endpoint configuration with a dynamic dev mode toggle that allows switching between development and production WebSocket endpoints without requiring a browser restart.

Changes:

Replaced static endpoint string in settings with a dynamic devMode boolean flag that switches between development (port 9102) and production (port 9002) endpoints
Added real-time settings change listeners that automatically update the icon color, reconnect WebSocket connections, and update UI when dev mode is toggled
Created a new React-based settings UI within the popup that replaces the old options page and provides instant auto-save functionality

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
src/settings.ts	Refactored to use devMode boolean instead of endpoint string; added getEndpoint helper function
src/popup/index.tsx	Added navigation, devMode state management, and storage change listeners
src/popup/index.html	Added responsive styles for expanded mode
src/popup/hooks/useConnectionState.ts	Extracted connection check logic and improved reconnection on state reset
src/popup/components/SettingsView.tsx	New component implementing auto-save settings UI
src/popup/components/Header.tsx	Enhanced with navigation buttons and devMode-based icon coloring
src/popup/components/DisconnectedView.tsx	Added navigation props
src/popup/components/ConnectedView.tsx	Added navigation props
src/options/index.ts	Updated to use devMode instead of endpoint field
src/options/index.html	Updated UI to show devMode checkbox instead of endpoint text field
src/background/utils.ts	New utility for updating extension icon based on devMode
src/background/index.ts	Added settings change listener, connection tracking, and reconnection logic
src/background/connectionState.ts	Added resetConnectionState function and updated endpoint usage
manifest/base.json	Changed options_ui to point to popup with settings view

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-28T11:22:14Z

+    const listener = (changes: Record<string, { newValue?: unknown }>) => {
+      if (changes.devMode?.newValue !== undefined) {
+        setDevMode(changes.devMode.newValue as boolean);
+      }
+    };


The storage change listener in popup/index.tsx doesn't check the areaName parameter, unlike the listener in src/background/index.ts line 56 which checks for 'sync'. This inconsistency means the popup could respond to changes from other storage areas (e.g., 'local', 'managed'). For consistency and to prevent potential issues, add an areaName check: if (areaName !== 'sync') return; at the start of the listener function.

Copilot · 2026-01-28T11:22:14Z

+
+        if (intentionalClose) {
+          // Settings change - don't reconnect, don't set disconnected state
+          intentionalClose = false;


The intentionalClose flag could have a race condition. If a WebSocket close event happens naturally (due to network issues) at the same time as settings are changed, the flag might be set to true but then the natural close event could reset it to false (line 177), causing the next settings-triggered close to be treated as an error. Consider using a more robust approach, such as checking a timestamp or having separate handling for settings changes that doesn't rely on a boolean flag that can be affected by concurrent events.

Suggested change

intentionalClose = false;

Copilot · 2026-01-28T11:22:14Z

+          <Button
+            variant="ghost"
+            size="sm"
+            className="h-6 w-6 p-0"
+            onClick={onBack}
+          >
+            <ArrowLeft className="h-4 w-4" />
+          </Button>
+        )}
+        <img
+          src={`/icons/ethui-${iconColor}.svg`}
+          alt="ethui"
+          className="h-5 w-5"
+        />
        <span className="font-medium text-sm">{title}</span>
+        {trailing}
+      </div>
+      <div className="flex items-center gap-1">
+        {onSettings && (
+          <Button
+            variant="ghost"
+            size="sm"
+            className="h-6 w-6 p-0"
+            onClick={onSettings}
+          >
+            <Settings className="h-4 w-4" />
+          </Button>
+        )}
+        {onExpand && (
+          <Button
+            variant="ghost"
+            size="sm"
+            className="h-6 w-6 p-0"
+            onClick={onExpand}
+          >
+            <Maximize2 className="h-4 w-4" />
+          </Button>


The icon buttons (Back, Settings, Expand) lack accessible labels. Screen reader users won't know what these buttons do since they only contain icon elements. Add aria-label attributes to each Button component to describe their function, e.g., aria-label="Go back", aria-label="Open settings", aria-label="Expand to new tab".

Copilot · 2026-01-28T11:22:14Z

+    if (changes.logLevel?.newValue) {
+      settings.logLevel = changes.logLevel.newValue as Settings["logLevel"];
+      log.setLevel(settings.logLevel);
+      log.debug("Log level changed to", settings.logLevel);
+    }


The check for devMode changes using !== undefined (line 64) is more lenient than the check for logLevel which uses truthy checking (line 58). This means setting devMode to false will correctly trigger the handler, but setting logLevel to an empty string (which shouldn't happen but could due to corruption) would not trigger the handler. For consistency and robustness, consider using !== undefined for logLevel as well, or add validation to ensure logLevel is one of the valid values.

Copilot · 2026-01-28T11:22:15Z

+    storage.sync
+      .get(defaultSettings as Record<string, unknown>)
+      .then((items) => {
+        setDevMode(items.devMode as boolean);


Missing error handling for storage.sync.get operation. If the storage operation fails, the devMode state will not be initialized correctly, but there will be no error feedback. Add a .catch() handler to handle potential storage errors gracefully.

Suggested change

setDevMode(items.devMode as boolean);

setDevMode(items.devMode as boolean);

})

.catch((error) => {

// Keep default devMode if loading fails, but log the error for debugging

console.error("Failed to load devMode setting from storage.sync:", error);

Copilot · 2026-01-28T11:22:15Z

+        Developer Mode
+      </label>
+      <p style="font-size: 12px; color: #666;">
+        Connects to port ethui's debug build instead of release. Meant for developers and contributors only.


Grammatical error: "Connects to port ethui's debug build" should be "Connects to ethui's debug build on port 9102" or "Connects to port 9102 (ethui's debug build)". The current phrasing is unclear because "port" and "ethui's debug build" are not grammatically connected properly.

Suggested change

Connects to port ethui's debug build instead of release. Meant for developers and contributors only.

Connects to ethui's debug build instead of the release build. Meant for developers and contributors only.

Copilot · 2026-01-28T11:22:15Z

+      devMode,
+    };
+
+    storage.sync.set(options).then(() => {


The type casting for storage.sync.set is inconsistent. In src/options/index.ts line 18, the Settings object is cast to Record<string, unknown> before calling storage.sync.set, but here the Settings object is passed directly. While Settings extends Record<string, unknown> so this works, the inconsistency could lead to confusion. For consistency across the codebase, either cast here as well or remove the cast in options/index.ts.

Copilot · 2026-01-28T11:22:16Z

+  useEffect(() => {
+    if (!initialized.current) return;
+
+    const options: Settings = {
+      logLevel: logLevel || defaultSettings.logLevel,
+      devMode,
+    };
+
+    storage.sync.set(options).then(() => {
+      setStatus("Saved");
+      setTimeout(() => setStatus(""), 750);
+    });
+  }, [logLevel, devMode]);


The auto-save effect (lines 33-45) will trigger on every change to logLevel or devMode after initialization, which could lead to multiple rapid saves if a user quickly toggles settings. Consider adding debouncing to prevent excessive storage writes. For example, using a setTimeout to delay the save by 500ms and clearing the previous timeout on each change. This would improve performance and reduce unnecessary storage operations.

Copilot · 2026-01-28T11:22:16Z

+    storage.sync.get(defaultSettings).then((items) => {
+      setLogLevel(items.logLevel as Settings["logLevel"]);
+      setDevMode(items.devMode as boolean);
+      initialized.current = true;
+    });
+  }, []);
+
+  // Auto-save on change
+  useEffect(() => {
+    if (!initialized.current) return;
+
+    const options: Settings = {
+      logLevel: logLevel || defaultSettings.logLevel,
+      devMode,
+    };
+
+    storage.sync.set(options).then(() => {
+      setStatus("Saved");
+      setTimeout(() => setStatus(""), 750);
+    });


Missing error handling for storage operations. Both storage.sync.get (line 25) and storage.sync.set (line 41) should have .catch() handlers to handle potential storage errors gracefully. Without error handling, if storage operations fail (e.g., due to quota exceeded or permissions issues), the user will have no feedback and the component state might become inconsistent. Consider adding error handling that displays an appropriate error message to the user.

Copilot · 2026-01-28T11:22:16Z

+      for (const [tabId, conn] of activeConnections) {
+        log.debug(`Closing connection for tab ${tabId}`);
+        conn.close();
+      }


After closing all active connections when devMode changes, the connections remain in the activeConnections map. While they will eventually be removed when port.onDisconnect fires (line 247), there's no guarantee this will happen immediately. If the settings are changed multiple times in quick succession, the loop will try to close connections that have already been closed, potentially calling conn.close() multiple times. While this might not cause errors (since closeWebSocket checks if ws exists), it could lead to unnecessary operations and log spam. Consider either clearing the activeConnections map after closing all connections, or adding a flag to track whether a connection has already been closed.

Suggested change

}

}

// Clear the map to avoid repeatedly closing the same connections if settings change again

activeConnections.clear();

wip

0b1e757

naps62 added the A-feature label Jan 28, 2026

naps62 added 2 commits January 28, 2026 10:20

wip

1d42a53

wip

a4e56ce

naps62 marked this pull request as ready for review January 28, 2026 11:09

Copilot AI review requested due to automatic review settings January 28, 2026 11:09

naps62 merged commit 67615ff into main Jan 28, 2026
2 checks passed

naps62 deleted the experiment branch January 28, 2026 11:10

Copilot started reviewing on behalf of naps62 January 28, 2026 11:10 View session

Copilot AI reviewed Jan 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experiment: auto reload settings#73

Experiment: auto reload settings#73
naps62 merged 3 commits intomainfrom
experiment

naps62 commented Jan 28, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-        setDevMode(items.devMode as boolean);
+        setDevMode(items.devMode as boolean);
+      })
+      .catch((error) => {
+        // Keep default devMode if loading fails, but log the error for debugging
+        console.error("Failed to load devMode setting from storage.sync:", error);

	Connects to port ethui's debug build instead of release. Meant for developers and contributors only.
	Connects to ethui's debug build instead of the release build. Meant for developers and contributors only.

-      }
+      }
+      // Clear the map to avoid repeatedly closing the same connections if settings change again
+      activeConnections.clear();

Conversation

naps62 commented Jan 28, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants