fix(widget): enhance error type processing#7114
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
WalkthroughConsolidates and exposes typed Ethereum provider events and ProviderMessage, strengthens error handling (using robust stringification and unknown-typed callbacks), removes an unused dev dependency, and restructures EIP-6963 type definitions to avoid external dev-only imports. Several UI wrappers receive a CSS class for targeting. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| callback(null, result) | ||
| } catch (error) { | ||
| callback(error, null) | ||
| callback(stringifyError(error), null) |
There was a problem hiding this comment.
The only actual change in this file, other changes are just eslint fix result
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
libs/types/src/eip6963-types.ts (2)
27-30: Consider exportingRequestArgumentsfor public API consumers.
RequestArgumentsis used in the publicEIP1193Providertype (line 6), but the interface itself is not exported. Consumers who need to construct or type-check request arguments won't have access to this type.Proposed fix
-interface RequestArguments { +export interface RequestArguments { readonly method: string readonly params?: readonly unknown[] | object }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/types/src/eip6963-types.ts` around lines 27 - 30, RequestArguments is referenced by the public EIP1193Provider type but is not exported; update the declaration of the RequestArguments interface to be exported so downstream consumers can import and use it (i.e., change the interface declaration to export the RequestArguments symbol so EIP1193Provider and callers have access to the type).
20-25: Note:EIP6963ProviderInfois duplicated inlibs/iframe-transport/src/types.ts.This interface has an identical definition in
libs/iframe-transport/src/types.ts(lines 70-75). Consider consolidating these definitions to avoid drift, either by having iframe-transport import from@cowprotocol/typesor by extracting to a shared location.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/types/src/eip6963-types.ts` around lines 20 - 25, The EIP6963ProviderInfo interface is duplicated; remove the duplicate by choosing a single source of truth: keep the definition in the types package (EIP6963ProviderInfo in libs/types) and update the other location to import/re-export it (replace the inline interface in iframe-transport with an import of EIP6963ProviderInfo), or move the interface to a shared module and have both packages import that module; ensure you update exports so consumers still get EIP6963ProviderInfo and run type checks to confirm no breaking changes.libs/iframe-transport/src/iframeRpcProvider/WidgetEthereumProvider.ts (1)
489-501: SimplifystringifyErrorto avoid redundant type assertion.Line 493 uses
(error as Error).messagewhich violates the coding guideline against using type casts when stronger typing is available. The logic can be streamlined using optional chaining.Proposed simplification
function stringifyError(error: unknown): string | null { if (!error) return null if (typeof error === 'string') return error if (error instanceof Error) return error.message - if ((error as Error).message) return (error as Error).message + + // Handle duck-typed error objects with a message property + if (typeof error === 'object' && 'message' in error && typeof (error as { message: unknown }).message === 'string') { + return (error as { message: string }).message + } try { return JSON.stringify(error) } catch { const errorMessage = 'Unknown WidgetEthereumProvider error' console.error(errorMessage, error) return errorMessage } }Alternatively, a simpler approach using optional chaining with a type guard:
Alternative using type narrowing
function stringifyError(error: unknown): string | null { if (!error) return null if (typeof error === 'string') return error if (error instanceof Error) return error.message - if ((error as Error).message) return (error as Error).message + + // Handle duck-typed error objects + const maybeMessage = (error as Record<string, unknown>)?.message + if (typeof maybeMessage === 'string') return maybeMessage try { return JSON.stringify(error) } catch { const errorMessage = 'Unknown WidgetEthereumProvider error' console.error(errorMessage, error) return errorMessage } }As per coding guidelines: "Use NEVER with
anytype in TypeScript; preferunknownor stronger types."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/iframe-transport/src/iframeRpcProvider/WidgetEthereumProvider.ts` around lines 489 - 501, The stringifyError function currently uses a redundant type assertion "(error as Error).message"; replace that with an optional-chained access to message (e.g., error?.message) or a proper typeof/type guard so you avoid casting unknown to Error, keep the existing checks for falsy and string, and retain the JSON.stringify fallback and catch block; update the function implementation around stringifyError to use optional chaining or a type-narrowing check instead of the explicit cast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/iframe-transport/src/iframeRpcProvider/WidgetEthereumProvider.ts`:
- Around line 489-501: The stringifyError function currently uses a redundant
type assertion "(error as Error).message"; replace that with an optional-chained
access to message (e.g., error?.message) or a proper typeof/type guard so you
avoid casting unknown to Error, keep the existing checks for falsy and string,
and retain the JSON.stringify fallback and catch block; update the function
implementation around stringifyError to use optional chaining or a
type-narrowing check instead of the explicit cast.
In `@libs/types/src/eip6963-types.ts`:
- Around line 27-30: RequestArguments is referenced by the public
EIP1193Provider type but is not exported; update the declaration of the
RequestArguments interface to be exported so downstream consumers can import and
use it (i.e., change the interface declaration to export the RequestArguments
symbol so EIP1193Provider and callers have access to the type).
- Around line 20-25: The EIP6963ProviderInfo interface is duplicated; remove the
duplicate by choosing a single source of truth: keep the definition in the types
package (EIP6963ProviderInfo in libs/types) and update the other location to
import/re-export it (replace the inline interface in iframe-transport with an
import of EIP6963ProviderInfo), or move the interface to a shared module and
have both packages import that module; ensure you update exports so consumers
still get EIP6963ProviderInfo and run type checks to confirm no breaking
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bc1d8ec9-734a-4fab-8c30-498931f8c191
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
libs/iframe-transport/src/iframeRpcProvider/WidgetEthereumProvider.tslibs/types/package.jsonlibs/types/src/eip6963-types.tslibs/widget-react/src/lib/CowSwapWidget.tsx
💤 Files with no reviewable changes (1)
- libs/types/package.json
elena-zh
left a comment
elena-zh
left a comment
There was a problem hiding this comment.
Hey @shoom3301 , idk why, but TWAP widget is not displayed when I hide orders table. Could you please check why?
it is working fine on develop
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/cowswap-frontend/src/modules/trade/pure/TradePageLayout/index.tsx (1)
35-37: Hoist the shared class name into a constant.
trade-orders-tableis now a contract between this selector and multiple pages. Exporting a single constant for it would remove the typo risk and keep future renames atomic.♻️ Proposed refactor
+export const TRADE_ORDERS_TABLE_CLASS = 'trade-orders-table' as const + export const PageWrapper = styled.div<{ isUnlocked: boolean secondaryOnLeft?: boolean maxWidth?: string hideOrdersTable?: boolean }>` @@ - > .trade-orders-table { + > .${TRADE_ORDERS_TABLE_CLASS} { display: ${({ isUnlocked }) => (!isUnlocked ? 'none' : '')}; } `Then reuse
TRADE_ORDERS_TABLE_CLASSfrom the pages instead of repeating the literal. As per coding guidelines, "Hoist repeating strings/tooltips into constants colocated with the feature".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cowswap-frontend/src/modules/trade/pure/TradePageLayout/index.tsx` around lines 35 - 37, Create and export a single constant like TRADE_ORDERS_TABLE_CLASS to hold the shared classname (replace the literal ".trade-orders-table" in the styled selector within TradePageLayout/index.tsx), update the styled selector to reference that constant (ensuring it still renders as a class selector where needed), and then import and use TRADE_ORDERS_TABLE_CLASS from the other pages/components that currently repeat the literal so renames are atomic and typo-prone strings are eliminated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/cowswap-frontend/src/modules/trade/pure/TradePageLayout/index.tsx`:
- Around line 35-37: Create and export a single constant like
TRADE_ORDERS_TABLE_CLASS to hold the shared classname (replace the literal
".trade-orders-table" in the styled selector within TradePageLayout/index.tsx),
update the styled selector to reference that constant (ensuring it still renders
as a class selector where needed), and then import and use
TRADE_ORDERS_TABLE_CLASS from the other pages/components that currently repeat
the literal so renames are atomic and typo-prone strings are eliminated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 24d5df43-97d7-4b84-8f3e-5b4f0d801483
📒 Files selected for processing (3)
apps/cowswap-frontend/src/modules/trade/pure/TradePageLayout/index.tsxapps/cowswap-frontend/src/pages/AdvancedOrders/AdvancedOrders.page.tsxapps/cowswap-frontend/src/pages/LimitOrders/RegularLimitOrders.page.tsx
|
@elena-zh thanks! Fixed. |
3 participants
Summary
Fixes #7113
@web3-react/typesdependency intypeslib (commit)WidgetEthereumProviderandCowSwapWidget.errorargument ofcatchis unknown and must be validated in runtime. (commit)To Test
Summary by CodeRabbit
Bug Fixes
Refactor
UI
Chores