Harden background lifecycle and popup synchronization#1467
Conversation
|
/review |
1 similar comment
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
CI Agent Review
Three issues identified: (1) A runtime bug where popup_websiteIconChanged doesn't update popupRefreshGeneration.value, allowing stale popup_UpdateHomePage messages to overwrite fresh state — directly undermining the generation counter's purpose. (2) Side-effect default parameters (popupRefreshGeneration = bumpPopupRefreshGeneration()) hide global state mutations in what appears to be an optional parameter, making it easy for future callers to inadvertently advance the generation counter. (3) The type-validator split for MessageToPopupPayload, PopupMessage, and MessageToPopup breaks the codebase's single-source-of-truth invariant (funtypes.Static<typeof X>), introducing a maintenance hazard where the manual type union and runtime validator can silently diverge.
| @@ -315,6 +323,8 @@ export function App() { | |||
| return undefined | |||
| } | |||
| case 'popup_websiteIconChanged': { | |||
There was a problem hiding this comment.
Bug: popupRefreshGeneration.value is NOT updated after accepting a popup_websiteIconChanged message, unlike the updateHomePage and popup_settingsUpdated handlers which both set popupRefreshGeneration.value = updateGeneration / parsed.popupRefreshGeneration. This allows stale popup_UpdateHomePage messages to pass the staleness check and overwrite all popup state with outdated data.
Reproduction: (1) Background bumps gen to 15 and sends icon changes. (2) popup_websiteIconChanged gen=15 arrives, accepted, icon updated — but popupRefreshGeneration.value stays at 0. (3) A stale popup_UpdateHomePage gen=10 arrives; staleness check 10 >= 0 passes, so stale state overwrites ALL popup data including the freshly-updated icon. Fix: add popupRefreshGeneration.value = parsed.popupRefreshGeneration after the staleness check, consistent with the other two handlers.
| function connectToPort(websiteTabConnections: WebsiteTabConnections, socket: WebsiteSocket, websiteOrigin: string, settings: Settings, connectWithActiveAddress: bigint | undefined): true { | ||
| function connectToPort( | ||
| websiteTabConnections: WebsiteTabConnections, | ||
| socket: WebsiteSocket, |
There was a problem hiding this comment.
The default parameter popupRefreshGeneration = bumpPopupRefreshGeneration() hides a global state mutation inside what reads as an optional parameter. Every call site that omits this argument silently increments the generation counter — e.g., onContentScriptConnected in background-startup.ts calling updateExtensionIcon(websiteTabConnections, socket.tabId, websiteOrigin) will bump the counter, potentially causing the popup to discard logically-fresh messages. This pattern makes correctness dependent on every call site knowing that omitting the parameter has a non-obvious side effect. Consider requiring the generation explicitly at all call sites or using a context object to make the data flow visible.
| @@ -883,8 +884,32 @@ export const ImportSimulationStack = funtypes.ReadonlyObject({ | |||
| data: InterceptorSimulationExport, | |||
| }) | |||
|
|
|||
There was a problem hiding this comment.
The type declarations for MessageToPopupPayload, PopupMessage, and MessageToPopup now use manually-written type unions instead of the codebase's established pattern type X = funtypes.Static<typeof X>. This breaks the single-source-of-truth guarantee — the TypeScript type and runtime validator are now separate declarations that must be manually kept in sync. When a new message variant is added, a developer must update both the manual type union and the runtime validator union; forgetting one causes silent type-validator divergence that neither the type system nor runtime checks will catch. If this split is necessary (e.g., due to TypeScript circular reference limits), it should be documented with a comment explaining why these three types can't use funtypes.Static, so future developers don't follow either pattern inconsistently.
|
/review |
There was a problem hiding this comment.
CI Agent Review
The changeset introduces a popup refresh generation mechanism to prevent stale UI updates and fixes several async handling issues. The main architectural concern is that the type definitions for MessageToPopupPayload, PopupMessage, and MessageToPopup break the established single-source-of-truth pattern, creating a risk of type-validator divergence that the compiler cannot detect.
|
|
||
| export type MessageToPopupPayload = funtypes.Static<typeof MessageToPopupPayload> | ||
| export const MessageToPopupPayload = funtypes.Union( | ||
| type MessageToPopupPayloadType = |
There was a problem hiding this comment.
Single source of truth violation: MessageToPopupPayload, PopupMessage, and MessageToPopup now use hand-written type unions (MessageToPopupPayloadType, MessageToPopupType) alongside separate runtime validators (MessageToPopupPayloadRuntype, PopupMessageRuntype, MessageToPopupRuntype). The established pattern throughout this file derives TypeScript types via funtypes.Static<typeof X>, guaranteeing the type and validator can never diverge. With separate definitions, adding a new message variant requires updating both the type union and the validator — if only one is updated, the compiler won't catch the inconsistency. The same goal (replacing PartialUpdateHomePage with fully-validated UpdateHomePage) could be achieved by simply swapping PartialUpdateHomePage for UpdateHomePage in the existing funtypes.Union(...) call and continuing to derive the type via funtypes.Static.
|
/review |
1 similar comment
|
/review |
There was a problem hiding this comment.
CI Agent Review
Type-runtime decoupling in interceptor-messages.ts creates a type-safety regression where TypeScript consumers believe message data is fully validated, but the runtime validator accepts unknown data. The importSettings function has also expanded in scope, embedding orchestration concerns in a previously simple data function, and the access management module now directly reads storage state, breaking its previous parameter-only dependency contract.
| */ | ||
| type MessageToPopupPayloadType = | ||
| | MessageToPopupSimple | ||
| | WebsiteIconChanged |
There was a problem hiding this comment.
The manually written MessageToPopupPayloadType references UpdateConfirmTransactionDialog (fully-typed data) and UpdateConfirmTransactionDialogPendingTransactions (fully-typed data), but the runtime validator MessageToPopupPayloadRuntype still uses UpdateConfirmTransactionDialogPartial and UpdateConfirmTransactionDialogPendingTransactionsPartial (both with data: Unknown). This is a type-safety regression: in the old code, funtypes.Static<typeof MessageToPopupPayload> correctly reflected that data was unknown, so consumers handled it carefully. Now TypeScript believes the data is fully validated, but the runtime accepts any shape — a future developer accessing parsed.data.visualizedSimulatorState etc. will get no type warning even though the runtime never validated those fields. Consider either using the same Partial variants in the manual type, or upgrading the runtime validators to use the fully-typed variants.
| tokenPriceService: TokenPriceService, | ||
| resetSimulationServices: ResetSimulationServices, | ||
| settingsData: ImportSettings, | ||
| ) { |
There was a problem hiding this comment.
importSettings has grown from (settingsData: ImportSettings) to (websiteTabConnections, ethereum, tokenPriceService, resetSimulationServices, settingsData), embedding access-management orchestration (updateWebsiteApprovalAccesses) and popup notification logic inside what was previously a pure data-import function. A cleaner approach would be to keep importSettings as a data function and let the caller in background.ts (which already holds the orchestration context) drive the refresh pipeline after import.
| import type { TokenPriceService } from '../simulation/services/priceEstimator.js' | ||
| import type { ResetSimulationServices } from '../simulation/serviceLifecycle.js' | ||
| import { mergeStoredWebsiteMetadata } from '../utils/websiteIcons.js' | ||
| import { getAllTabStates } from './storageVariables.js' |
There was a problem hiding this comment.
accessManagement.ts now imports getAllTabStates from storageVariables.js, directly reading storage state inside updateWebsiteApprovalAccesses to discover and refresh icon states for tabs without active port connections. Previously, this module only operated on the websiteTabConnections map passed as a parameter, keeping access management logic contained to the connections it was given. This mixes a storage-read concern into the access-control layer — if the tab state storage format changes, accessManagement.ts must now be updated too.
|
/review |
There was a problem hiding this comment.
CI Agent Review
All agents converged on a single code quality issue: misleading indentation of closing braces at the end of updateTabConnections in accessManagement.ts. The for-loop closing brace is indented at 2 tabs (should be 1 tab) and the function closing brace is indented at 1 tab (should be 0 tabs). While not a runtime bug since JavaScript scopes by braces rather than indentation, the inconsistency can confuse future maintainers and should be corrected.
| @@ -212,9 +244,9 @@ async function updateTabConnections(ethereum: EthereumClientService, tokenPriceS | |||
| } | |||
| } | |||
| for (const { tabId, websiteOrigin } of iconRefreshTargets.values()) { | |||
There was a problem hiding this comment.
Misleading indentation of closing braces. The } closing the for loop is at 2-tab indentation (should be 1 tab), and the } closing updateTabConnections on the next line is at 1-tab indentation (should be 0 tabs). While this doesn't affect runtime behavior, it can confuse maintainers about the block structure. Consider reindenting as \t} and } respectively.
|
/review |
There was a problem hiding this comment.
CI Agent Review
A bug in popupRefreshGeneration.ts can cause the popup to permanently ignore all state updates after a Chrome MV3 service worker restart. The counter is an in-memory variable initialized to Date.now() but can advance beyond the current wall clock when bumpPopupRefreshGeneration() is called multiple times within the same millisecond. If the service worker is terminated and restarted by Chrome, the counter resets to a new Date.now() that may be lower than the generation value already stored in an open popup, causing shouldIgnoreOutdatedPopupRefreshMessage to reject every subsequent message until the user manually reopens the popup.
| @@ -0,0 +1,7 @@ | |||
| let popupRefreshGeneration = Date.now() | |||
There was a problem hiding this comment.
Service worker restart regression: popupRefreshGeneration is initialized to Date.now(), but rapid successive calls to bumpPopupRefreshGeneration() within the same millisecond push the counter beyond the current wall clock. When Chrome MV3 terminates and restarts the service worker, the counter resets to a new Date.now() that can be lower than what an open popup previously received. The popup's shouldIgnoreOutdatedPopupRefreshMessage will then permanently reject all incoming messages, freezing the UI until the popup is manually closed and reopened. Consider persisting the counter to browser.storage.local or having the popup detect service worker restarts and reset its local generation.
|
/review |
There was a problem hiding this comment.
CI Agent Review
The allTabStates parameter added to updateWebsiteApprovalAccesses is an encapsulation violation — every call site passes await getAllTabStates() identically, meaning the function's internal dependency has leaked outward into its entire call graph. This data should be fetched internally by the function rather than requiring all 9+ callers to await and provide it.
| @@ -307,16 +339,16 @@ export const areWeBlocking = async (websiteTabConnections: WebsiteTabConnections | |||
| return false | |||
| } | |||
There was a problem hiding this comment.
The allTabStates parameter is an encapsulation violation. All 9+ call sites (in popupMessageHandlers.ts, background.ts, and interceptorAccess.ts) pass await getAllTabStates() identically. When every caller provides the same value for a parameter, the parameter should be the function's own responsibility to fetch. Other functions in this codebase (e.g., updateExtensionIcon, getSettings) fetch their own dependencies internally rather than requiring callers to do so. If the way tab states are fetched ever changes, every call site must be updated instead of just this one function. Every new call site must remember to call await getAllTabStates(), creating a silent failure mode if forgotten.
|
/review |
There was a problem hiding this comment.
CI Agent Review
Two architectural concerns: (1) dead legacy dispatch logic in updateWebsiteApprovalAccesses remains after overload removal, and (2) popupRefreshGeneration is manually threaded through ~10 function signatures as tramp data rather than being encapsulated.
| @@ -307,16 +340,15 @@ export const areWeBlocking = async (websiteTabConnections: WebsiteTabConnections | |||
| return false | |||
There was a problem hiding this comment.
Dead legacy dispatch logic: the two overload declarations were removed, and all call sites now use the new 7-argument signature, yet the function body still contains usingLegacySignature = tokenPriceServiceOrWebsiteTabConnections instanceof Map, union-typed parameter names (tokenPriceServiceOrWebsiteTabConnections, resetSimulationServicesOrSettings, websiteTabConnectionsOrPrompt, settingsOrPrompt), and the conditional dispatch branches. No caller passes a Map for the second argument, so usingLegacySignature is always false and the legacy branch is unreachable. This dead code misleads future maintainers into thinking two calling conventions are supported. The union parameters and runtime dispatch should be removed entirely, with parameters renamed to their actual types (tokenPriceService, resetSimulationServices, etc.).
| bumpPopupRefreshGeneration(), | ||
| ) ? 'hasAccess' : 'noAccess' | ||
| } | ||
| if (access === 'noAccess' || access === 'interceptorDisabled') return access |
There was a problem hiding this comment.
The popupRefreshGeneration: number parameter is tramp data — it's threaded through verifyAccess → connectToPort → updateExtensionIcon → setInterceptorIcon, and similarly through disconnectFromPort, updateTabConnections, updateWebsiteApprovalAccesses. None of the intermediate functions use the value for their own logic; they merely forward it. This increases coupling across the entire call chain. If another coordination value needs to be added (e.g., a simulationGeneration), every function in this chain will need yet another parameter. Consider encapsulating the generation into the message-sending layer or a context object so only the boundary functions need to know about it.
KillariDev
force-pushed
the
t3code/8539035f
branch
from
b5b7ca0 to
6b60848
Compare
|
/review |
There was a problem hiding this comment.
CI Agent Review
The popupRefreshGeneration parameter is threaded through ~10 function signatures across four modules (accessManagement.ts, iconHandler.ts, popupMessageHandlers.ts, background-startup.ts), mixing a popup message-ordering concern into unrelated function responsibilities like icon management and port connection lifecycle. The new popupRefreshGeneration.ts module already exports getPopupRefreshGeneration(), which could be called directly in downstream functions, eliminating the parameter threading while preserving the same ordering guarantee since bumpPopupRefreshGeneration() executes synchronously before async work begins.
| @@ -78,15 +85,15 @@ async function waitForLoadedTab(tabId: number) { | |||
| } | |||
There was a problem hiding this comment.
updateExtensionIcon and setInterceptorIcon now carry a popupRefreshGeneration parameter that exists solely to be forwarded to sendPopupMessageToOpenWindows. These functions are about icon management, not popup refresh coordination. Since popupRefreshGeneration.ts exports getPopupRefreshGeneration(), downstream functions could call it directly instead of receiving the generation via parameter. The parameter is only needed at the ~10 call sites where bumpPopupRefreshGeneration() is called (to capture the value before async work), not in every intermediate function. This would reduce coupling and prevent future callers from silently passing a stale/zero value that breaks staleness detection.
| @@ -150,9 +162,16 @@ async function getActiveAddressForDomain(websiteOrigin: string, settings: Settin | |||
| return undefined | |||
There was a problem hiding this comment.
connectToPort, disconnectFromPort, updateTabConnections, and updateWebsiteApprovalAccesses all received a popupRefreshGeneration: number parameter that mixes popup message-ordering into port connection lifecycle. These functions' core responsibilities don't involve popup refresh generation — they forward it only because updateExtensionIcon now requires it (which itself only forwards it to sendPopupMessageToOpenWindows). Using getPopupRefreshGeneration() directly in the icon handler's sendPopupMessageToOpenWindows call would eliminate this cross-cutting parameter threading across all four modules.
|
/review |
There was a problem hiding this comment.
CI Agent Review
Two issues identified: (1) incomplete migration of updateWebsiteApprovalAccesses leaves dead legacy dispatch logic behind with union-typed parameters that could trap future callers, and (2) partial-update message handlers (popup_websiteIconChanged, popup_settingsUpdated) can advance popupRefreshGeneration past the generation of in-flight popup_UpdateHomePage messages, causing valid full-page updates to be rejected as stale.
| @@ -325,10 +348,37 @@ export function updateWebsiteApprovalAccesses( | |||
| const promptForAccesses = typeof (usingLegacySignature ? websiteTabConnectionsOrPrompt : promptForAccessesIfNeeded) === 'boolean' | |||
There was a problem hiding this comment.
Incomplete migration: the overload signatures were removed, but the runtime usingLegacySignature dispatch (instanceof Map check, undefined as never casts, re-assignment of websiteTabConnections/settings from different parameters) remains in the function body. Since all callers now use the new 6-argument signature, this legacy branch is unreachable dead code. However, the union-typed parameters (TokenPriceService | WebsiteTabConnections, ResetSimulationServices | Settings, etc.) still allow the old argument ordering to pass type-checking. A future developer could accidentally pass arguments in the legacy order, TypeScript would accept it, and the dead usingLegacySignature path would activate — executing untested logic with as never casts that bypass type safety. Consider completing the migration by giving each parameter a single concrete type and removing the runtime dispatch entirely.
| @@ -315,14 +323,17 @@ export function App() { | |||
| return undefined | |||
There was a problem hiding this comment.
Race condition risk: popup_websiteIconChanged and popup_settingsUpdated handlers unconditionally set popupRefreshGeneration.value to their incoming generation. If a popup_UpdateHomePage with a lower generation is in-flight (e.g., generation 15 requested via requestCachedHomeData), and a popup_websiteIconChanged arrives with generation 20 (e.g., from an icon update triggered by updateWebsiteApprovalAccesses), the popupRefreshGeneration counter advances to 20 and the pending generation-15 full-page update is rejected as stale. This leaves the popup with stale full-page data (address book, simulation state, etc.) alongside a freshly updated icon/message, until another refresh cycle produces a higher generation number. Consider whether partial-update handlers should advance popupRefreshGeneration only if they carry a generation that the popup hasn't already seen from a full-page update, or use a separate generation counter for partial updates.
- Removed legacy `updateWebsiteApprovalAccesses` overload signature path - Made `ethereum`, `tokenPriceService`, and `resetSimulationServices` optional in tab update flow - Guarded access-prompt path with explicit service availability checks - Updated `extensionIconDeduping` tests to call new explicit signature with `undefined` services
- Add separate pending and icon-specific generation tracking in `App` to ignore outdated home payloads after settings refresh - Gate icon state updates with icon generation so stale data cannot regress popup header icon - Add regression test ensuring settings updates block stale lower-generation home updates
|
/review |
- Normalize indentation in `app/ts/components/App.tsx` switch-case block for message handling - Preserve existing `popup_settingsUpdated` and `popup_UnexpectedErrorOccured` logic without behavior changes
|
/review |
There was a problem hiding this comment.
CI Agent Review
No significant issues found. The changes introduce a popup refresh generation mechanism for ordering async messages, make previously-synchronous operations properly async, and improve type safety by replacing PartialUpdateHomePage (which used data: funtypes.Unknown) with fully-validated UpdateHomePage in the message union.
2 participants
Summary
Testing
bun testbun run setup-chromebun run typecheckbun run lintbun run test:chrome-communication(after Chromium/Chrome available)bun run benchmark:popup-lifecycle(if validating popup refresh behavior)