feat: add real shared worker #1684
Conversation
WalkthroughThe pull request introduces significant modifications to the handling of worker contexts within the application. The Changes
Possibly related PRs
Suggested labels
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: 3
🧹 Outside diff range and nitpick comments (3)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnRelayChain.js (1)
47-49: Send error information back via 'port' in case of exceptionsWhen an error occurs, it is logged to the console, but the caller is not informed about the failure. Consider sending an error message back through the
portso that the caller can handle errors appropriately.Modify the
catchblock to send the error message:} catch (error) { console.error(`getAssetOnRelayChain: Error fetching balances for ${chainName}:`, error); + port.postMessage(JSON.stringify({ + functionName: 'getAssetOnRelayChain', + error: error.message, + })); }packages/extension-polkagate/src/components/contexts.tsx (1)
30-30: LGTM! Good architectural improvement.The transition from
WorkertoMessagePortis a solid improvement that better supports SharedWorker communication patterns. MessagePort provides a dedicated communication channel, which is more suitable for shared worker scenarios.packages/extension-polkagate/src/hooks/useAssetsBalances.ts (1)
Line range hint
270-360: Consider adding error handling for worker communication.While the message handling logic is solid, it would be beneficial to add error handling for worker communication failures.
const handleWorkerMessages = useCallback(() => { if (!worker) { return; } + worker.onerror = (e: MessageEvent<string>) => { + console.error('Worker error:', e); + // Handle worker errors appropriately + }; + worker.onmessage = (e: MessageEvent<string>) => { try { const message = e.data;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
packages/extension-polkagate/src/components/contexts.tsx(1 hunks)packages/extension-polkagate/src/hooks/useAssetsBalances.ts(1 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js(2 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js(2 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnRelayChain.js(1 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js(2 hunks)packages/extension-polkagate/src/util/workers/sharedWorker.js(2 hunks)packages/extension-ui/src/Popup/contexts/WorkerProvider.tsx(1 hunks)packages/extension/package.json(1 hunks)packages/extension/src/packageInfo.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/extension/src/packageInfo.ts
- packages/extension/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/extension-polkagate/src/util/workers/sharedWorker.js
[error] 33-33: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 14-15: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js
[error] 65-65: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (8)
packages/extension-ui/src/Popup/contexts/WorkerProvider.tsx (4)
12-13: LGTM: Updated state to use MessagePort
The change from using Worker to MessagePort in the state reflects the shift to using a SharedWorker's communication port, which is appropriate and aligns with the new architecture.
16-24: Proper initialization of SharedWorker and MessagePort
The SharedWorker is correctly instantiated using the appropriate module URL, and the workerPort is properly accessed and started for communication.
28-29: Proper cleanup of MessagePort in useEffect
Closing the workerPort in the cleanup function ensures that the communication port is properly disconnected when the component unmounts, preventing potential memory leaks.
35-35: Context provider updated to supply MessagePort
Updating the WorkerContext.Provider to provide the port instead of the worker correctly reflects the updated communication model using MessagePort.
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js (1)
15-15: Function signature updated to include 'port' parameter
The function getAssetOnAssetHub now correctly accepts the port parameter for messaging, aligning with the new communication model using MessagePort for communication between the worker and the main thread.
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js (2)
8-16: LGTM! Well-documented parameters.
The JSDoc comments clearly document the expected parameter types, improving code maintainability.
64-64: 🛠️ Refactor suggestion
Refactor the assignment in expression.
The current implementation uses assignment within an expression, which can be confusing and is flagged by static analysis. Consider splitting this into separate statements for better readability.
- results[address]?.push(asset) ?? (results[address] = [asset]);
+ if (results[address]) {
+ results[address].push(asset);
+ } else {
+ results[address] = [asset];
+ }Likely invalid or redundant comment.
packages/extension-polkagate/src/hooks/useAssetsBalances.ts (1)
136-136: LGTM! Good transition to MessagePort.
The parameter type change from Worker to MessagePort aligns well with the SharedWorker architecture.
packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
packages/extension-polkagate/src/hooks/useNFT.tsx (3)
14-17: Add JSDoc documentation for the NftItemsWorker interface.Consider adding JSDoc documentation to improve code maintainability and help other developers understand the purpose of this interface and its properties.
+/** + * Interface representing the worker message format for NFT operations + * @property {string} functionName - The name of the worker function to execute + * @property {NftItemsType} results - The NFT items data returned from the worker + */ export interface NftItemsWorker { functionName: string; results: NftItemsType; }
20-20: Consider moving NFT_FUNCTION_NAME to a shared constants file.Since this constant needs to match between the worker and the hook, it should be moved to a shared location to prevent potential mismatches and improve maintainability.
Line range hint
44-46: Review commented-out state updates.There are several commented-out
setFetching(false)calls. This could lead to the fetching state never being reset in error scenarios.if (!NFTs) { notify(t('Unable to fetch NFT/Unique items!'), 'info'); - // setFetching(false); + setFetching(false); return; } // ... and similar changes for other error casesAlso applies to: 62-64, 69-70
packages/extension-polkagate/src/util/workers/shared-helpers/getNFTs.js (2)
4-4: Consider removing the@ts-nocheckdirectiveThe
// @ts-nocheckcomment disables TypeScript checks for the entire file, which might hide potential type errors and issues. It's advisable to remove this directive or limit its scope to only the necessary sections to ensure type safety and catch potential bugs early.
256-257: Adjust the backoff strategy to implement exponential backoffThe comment mentions an exponential backoff strategy, but the current implementation uses linear backoff with
tryCount * 1000milliseconds. To implement true exponential backoff, consider usingMath.pow(2, tryCount) * 1000to increase the delay exponentially with each retry.Apply this change to adjust the backoff strategy:
- // Wait for a delay before retrying (e.g., exponential backoff) - await new Promise((resolve) => setTimeout(resolve, tryCount * 1000)); + // Wait for a delay before retrying (exponential backoff) + await new Promise((resolve) => setTimeout(resolve, Math.pow(2, tryCount) * 1000));packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (1)
34-36: Avoid using@ts-ignoreby properly typing variablesUsing
// @ts-ignoresuppresses TypeScript errors, which may hide potential issues and reduce code quality. Instead of ignoring these errors, consider properly typing theapiobject and its properties to resolve the TypeScript issues.Example:
- Define a type or interface for the
apiobject that includes the_optionsandproviderproperties.- Ensure that
api._options.provider.endpointis correctly recognized by TypeScript.This will enhance code reliability and maintainability by leveraging TypeScript's type-checking capabilities.
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js (1)
66-66: Avoid assignments within expressions for better readabilityUsing assignments inside expressions can make the code harder to read and understand. It may also lead to unintentional side effects. Consider refactoring the code to separate the assignment from the expression for clarity.
Apply this change to improve code readability:
- results[address]?.push(asset) ?? (results[address] = [asset]); + if (results[address]) { + results[address].push(asset); + } else { + results[address] = [asset]; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 66-66: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
packages/extension-polkagate/src/hooks/useNFT.tsx(3 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js(2 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js(2 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnRelayChain.js(1 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js(2 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getNFTs.js(1 hunks)packages/extension-polkagate/src/util/workers/sharedWorker.js(1 hunks)packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js
- packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js
🧰 Additional context used
🪛 Biome (1.9.4)
packages/extension-polkagate/src/util/workers/sharedWorker.js
[error] 15-16: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js
[error] 66-66: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (4)
packages/extension-polkagate/src/hooks/useNFT.tsx (1)
25-25: Verify worker initialization and availability.
Ensure that the worker is properly initialized before use.
✅ Verification successful
Let me gather more information about the worker initialization by checking the implementation details.
Worker initialization is properly handled
The worker implementation is well-structured with proper initialization and error handling:
- Worker is initialized in
WorkerProviderusingSharedWorker - Port communication is properly started with
workerPort.start() - The hook
useWorkercorrectly checks for context availability - Cleanup is handled on unmount
- Error handling is implemented in the shared worker
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check worker initialization and related files
# Check for worker registration
rg -A 5 "registerWorker|SharedWorker"
# Check for worker implementation file
fd -e js -e ts "worker" --full-path
Length of output: 2560
Script:
#!/bin/bash
# Check worker implementation and usage
# Check the shared worker implementation
cat packages/extension-polkagate/src/util/workers/sharedWorker.js
# Check the worker hook implementation
cat packages/extension-polkagate/src/hooks/useWorker.ts
# Check WorkerProvider implementation
cat packages/extension-ui/src/Popup/contexts/WorkerProvider.tsx
Length of output: 4285
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnRelayChain.js (1)
18-54: Good use of structured error handling and messaging
The implementation effectively uses try...catch...finally blocks to handle asynchronous operations and ensures that resources are properly managed. The results are correctly sent back through the port, enhancing communication reliability.
packages/extension-polkagate/src/util/workers/sharedWorker.js (2)
35-36: Wrap variable declaration in switch-case to limit scope
Declaring assetsToBeFetched directly inside a case without a block can cause it to be accessible in other cases, potentially leading to errors. Wrap the case statement in braces to limit the scope of the variable to that case.
Apply this change to wrap the case block:
case 'getAssetOnMultiAssetChain':
+ {
// eslint-disable-next-line no-case-declarations
const assetsToBeFetched = assetsChains[parameters.chainName];
// Existing code...
getAssetOnMultiAssetChain(assetsToBeFetched, ...params, port).catch(console.error);
break;
+ }16-16:
Avoid reassigning the global onconnect variable
Reassigning the global onconnect variable may lead to unintended side effects by overriding essential functionality. It's recommended to use self.addEventListener('connect', ...) to register the event handler without modifying the global variable.
Apply this change to prevent reassigning the global variable:
- onconnect = (event) => {
+ self.addEventListener('connect', (event) => {🧰 Tools
🪛 Biome (1.9.4)
[error] 15-16: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/extension-polkagate/src/hooks/useAssetsBalances.ts (2)
285-311: LGTM: Improved immutability handlingThe implementation correctly creates new object references at each level of the nested structure, which is crucial for React's state updates. This ensures proper re-rendering when the state changes.
Consider using a deep clone utility for better maintainability:
- const combinedAsset = { - ...(fetchedAssets || DEFAULT_SAVED_ASSETS), - balances: { - ...(fetchedAssets?.balances || DEFAULT_SAVED_ASSETS.balances) - } - }; + const combinedAsset = structuredClone(fetchedAssets || DEFAULT_SAVED_ASSETS);
402-402: Consider adding error handling for postMessageWhile the implementation is consistent, consider adding error handling for the
postMessagecalls to handle potential messaging failures.- worker.postMessage({ functionName, parameters: { address: _addresses, chainName, userAddedEndpoints } }); + try { + worker.postMessage({ functionName, parameters: { address: _addresses, chainName, userAddedEndpoints } }); + } catch (error) { + console.error(`Failed to post message to worker: ${error.message}`); + // Consider notifying the user through setAlerts + }Also applies to: 422-422
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.gitignore(1 hunks)packages/extension-polkagate/src/hooks/useAssetsBalances.ts(7 hunks)packages/extension-polkagate/src/hooks/useNFT.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension-polkagate/src/hooks/useNFT.tsx
🔇 Additional comments (3)
packages/extension-polkagate/src/hooks/useAssetsBalances.ts (3)
136-136: LGTM: Worker type update aligns with SharedWorker implementation
The parameter type change from Worker to MessagePort is consistent with the transition to using SharedWorker for improved worker communication.
Line range hint 320-391: LGTM: Improved event handling with proper cleanup
The switch to using addEventListener with proper cleanup through removeEventListener is a significant improvement. This prevents potential memory leaks and follows event handling best practices.
484-493: LGTM: Proper worker message handling setup
The addition of handleWorkerMessages and its inclusion in the dependency array ensures proper setup and cleanup of worker message handling.
# [0.34.0](v0.33.3...v0.34.0) (2024-12-06) ### Features * add real shared worker ([#1684](#1684)) ([b190fc1](b190fc1))
Close: #1683
Summary by CodeRabbit
Release Notes
New Features
Workerto aSharedWorker, enhancing communication between the main thread and the worker.MessagePortfor improved messaging.Bug Fixes
Documentation
Chores
0.33.1in relevant files.