Implement redesigned UI workflows#213
Conversation
🛡️ Defender Against the Dark Arts✅ No issues found. Great job! |
🔒 Security Reviewer1 issue found Cross-site scripting (XSS) via unsanitized priceSourceUrl📍 ui/ts/components/OpenOracleSection.tsx:86 The function renderInitialPriceSourceLabel uses the priceSourceUrl value directly in an anchor tag's href attribute without any validation or sanitization. If an attacker can control this URL (e.g., by creating a malicious Open Oracle game with a crafted price source), they could inject a javascript: or data: URL, leading to stored XSS when a user clicks the link. This could allow theft of session information, wallet compromise, or other client-side attacks. |
🏛️ Architect21 issues found Hardcoded ABI parameter types in deployment bytecode generation📍 solidity/ts/testsuite/simulator/utils/contracts/deployPeripherals.ts:65 Two functions ( Monolithic CSS architecture with no separation of concernsNew styles are appended to a 3000+ line monolithic CSS file with no architectural separation. This continues a pattern where all component styles share a global namespace, creating high coupling between unrelated UI elements. The file violates the single responsibility principle and will not scale. Any change to base styles risks breaking multiple components due to implicit dependencies. Inconsistent design token abstraction across new componentsThe newly added styles mix CSS custom properties with hardcoded RGBA values. For example, Semantic class proliferation anti-pattern in component variantsComponent state variants are implemented as separate semantic classes ( Tight coupling of App component to multiple feature statesThe showTransactionSuccessNotice variable directly depends on result states from many different feature hooks (openOracleResult, securityPoolResult, securityPoolOverviewResult, securityVaultResult, tradingResult, reportingResult, forkAuctionResult, poolPriceOracleResult). This creates high coupling between the top-level App component and the internal state of various features, violating Separation of Concerns and the Single Responsibility Principle. This design will hinder scalability and maintainability, as any change to those hooks' return types or semantics could break this logic, and the component becomes a God object with many dependencies. Duplication of eligibility logic and messages for child universe deployment📍 ui/ts/components/MarketOverviewSection.tsx:45 The conditions and user-facing reason messages that determine whether a child universe can be deployed are duplicated across multiple locations: (1) the action configuration passed to ChildUniversesSection (lines 115-119), (2) the availability prop of the TransactionActionButton inside the OperationModal (lines 143-155), and (3) the detail strings in the childUniverseRequirements array (lines 45-49). This violates the DRY principle, making the code harder to maintain; any change to business rules or wording must be updated in all places, increasing the risk of inconsistencies. Modal closes incorrectly when a different child universe deployment completes📍 ui/ts/components/MarketOverviewSection.tsx:52 The useEffect that resets selectedChildOutcomeIndex when zoltarChildUniversePendingOutcomeIndex transitions to a non-pending state does not verify that the completed deployment corresponds to the currently selected child universe. If the user switches the selection while a deployment is in progress, the modal will still close when that original deployment finishes, even though the user may be working with a different child. This leads to unexpected UI behavior and a poor user experience. The effect should only clear the selection if the completed outcome matches the selected outcome. MarketSection violates Single Responsibility Principle📍 ui/ts/components/MarketSection.tsx The component accumulates multiple responsibilities: managing local UI state (fork modal), computing derived presentation data (universeStage, forkAction), orchestrating side effects (navigation redirection, auto-loading, modal cleanup), and rendering several conditional sections including a modal workflow. This high cohesion makes the component difficult to test, understand, and maintain. Consider extracting modal state and derived data into custom hooks and splitting the conditional view logic into smaller, focused components. God Component and Single Responsibility Violation📍 ui/ts/components/OpenOracleSection.tsx:135 OpenOracleSection and its helper functions have grown too large and violate the Single Responsibility Principle. renderReportDetailsCard (340 lines) and renderSelectedReportActionSection (219 lines) each handle multiple concerns: UI composition, business logic for readiness requirements, modal orchestration, data formatting, and state management. This creates high coupling, makes testing difficult, and increases maintenance burden. The component should be decomposed into smaller, focused components with dedicated hooks, and business logic (e.g., requirement arrays, availability calculations) should be extracted to domain services or utilities. Inconsistent lock state between derived stage and form controls📍 ui/ts/components/ReportingSection.tsx:82 The component computes a reporting stage via WorkflowSummaryStrip currentStep mismatched with hardcoded steps📍 ui/ts/components/ReportingSection.tsx:135 The Missing abstraction boundary between UI and domain types📍 ui/ts/components/RequirementsChecklist.tsx:1 Direct import of domain type ('ReadinessBlocker') into UI component creates tight coupling. UI layer should depend on abstractions (DTOs/view models) rather than domain entities to maintain separation of concerns. Modal does not capture snapshot of deployment state, causing state divergence during open lifetime📍 ui/ts/components/ScalarDeploymentSection.tsx:31 The OperationModal for child universe creation directly uses live state (selectedScalarOutcomeIndex, selectedScalarChild, scalarDeployRequirements) from the parent component. This allows the modal's content and deployment target to change if the user interacts with other UI elements (e.g., adjusting the scalar outcome picker) while the modal is open, violating the principle of a modal as a bounded, consistent decision context. Additionally, the modal conditionally renders its body only when selectedScalarChild exists, resulting in an empty modal for the primary use case of deploying a new universe. To resolve: capture the deployment state (outcome index, child universe summary, requirements) when opening the modal and base the modal's UI and action on that snapshot, isolating it from subsequent state changes. Duplication of validation logic and messages between button state and requirements checklist📍 ui/ts/components/SecurityPoolSection.tsx:49 The component maintains two separate representations of the same validation state: SecurityPoolWorkflowSection violates Single Responsibility Principle (God Component)📍 ui/ts/components/SecurityPoolWorkflowSection.tsx:402 The component has grown to 1083 lines and now handles pool lookup, vault operations with 4 complex modals, trading, reporting, fork workflows, and validation logic. This God Component anti-pattern creates high coupling, poor testability, and makes changes risky. It should be decomposed into smaller, focused components (e.g., PoolSection, VaultOperationsSection, WorkflowSection) with clear separation of concerns. Duplication of vault operation validation logic across component and modals📍 ui/ts/components/SecurityPoolWorkflowSection.tsx:356 Business validation rules for vault operations (ownership, network, oracle state, balance, amounts) are defined both as guard messages in the main component (lines 356-395) and again as checklist items inside each OperationModal (lines 932-938, 986-992, 1024-1030, 1053-1058). This violates DRY, increases maintenance burden, and creates risk of inconsistent behavior when rules change. A single source of truth for validation constraints should be established. Complex derived state and direct mutation coupling in vault workflow📍 ui/ts/components/SecurityPoolWorkflowSection.tsx:322 The component computes many derived guard messages and validation values inline (lines 322-395) and directly mutates the shared Inconsistent bytecode encoding pattern introduces architectural fragility📍 ui/ts/contracts/deploymentHelpers.ts:75 The Inconsistent readiness state logic for 'Settle Report' action in 'dispute' mode📍 ui/ts/lib/openOracleReadiness.ts:47 The 'Settle Report' action within the 'dispute' mode branch (lines 42-50) unconditionally sets readiness to 'warning' when unblocked, ignoring the connection state. This differs from the identical 'Settle Report' action in the 'settle' mode branch (lines 52-61) and the 'Initial Report' action (lines 22-31), which set readiness to 'ready' when connected and 'warning' when disconnected. This inconsistency suggests a potential copy-paste error and leads to different UI behavior for the same action depending on the mode, violating the principle of least surprise. Inconsistent action identifiers and misuse of blockedActions field in LifecycleStagePresentation returns📍 ui/ts/lib/securityPoolStage.ts:9 The function returns LifecycleStagePresentation objects where the availableActions and blockedActions arrays contain inconsistent strings. Specifically, action identifiers are not standardized (e.g., 'Vault actions' vs 'Vaults' for the same concept), and blockedActions sometimes holds action identifiers (e.g., 'Vault actions') and sometimes descriptive messages (e.g., 'Routine operational workflows may be limited'). This violates the principle of consistent data contracts, leading to ambiguity and potential errors in UI components that rely on these values for enabling/disabling actions or displaying messages. Duplication between renderDisputeActionSection and renderSettleActionSection violates DRY📍 ui/ts/tests/openOracleSection.test.tsx:276 The two functions are nearly identical, differing only in the first argument (mode) and the default overrides for openOracleReportDetails. This duplication violates the DRY principle, creating a maintenance hazard: any change to the common rendering logic must be applied to both places, increasing the risk of inconsistencies and bugs. Refactoring to extract a shared, parameterized helper would centralize the logic and improve maintainability. |
🔧 Expensive Linter19 issues found Inconsistent deployment bytecode generation method📍 solidity/ts/testsuite/simulator/utils/contracts/deployPeripherals.ts:65 The file predominantly uses Long conditional line reduces readabilityThe conditional expression for Excessive line length in function signature📍 ui/ts/components/ActionLauncherCard.tsx:13 Line 13 contains a long function signature that exceeds typical line length limits (e.g., 80-100 characters), reducing readability. The destructured parameters should be split across multiple lines. Excessive line length in JSX element📍 ui/ts/components/ActionLauncherCard.tsx:23 Line 23 contains a JSX element with numerous props, resulting in a very long line that violates common line length conventions and hinders maintainability. The props should be broken into multiple lines with proper indentation. Import ordering breaks established grouping convention📍 ui/ts/components/DeploymentRouteContent.tsx:1 The new import of ActionReadinessPanel from './ActionReadinessPanel.js' was added at the very top, before the existing type import from 'preact'. This splits the block of regular imports from './' into two separate groups, violating the original pattern where all regular imports from './' appear contiguously immediately after the type import from 'preact'. Maintaining consistent import grouping improves readability and adheres to the repository's established style. Component returns undefined instead of null📍 ui/ts/components/LifecycleStageBanner.tsx:8 In React, components should return null to indicate no render. Returning undefined is unconventional and may cause type mismatches or runtime issues, as React's expected return type is ReactElement | null. Import order: Type imports should be grouped at the end📍 ui/ts/components/MarketSection.tsx:14 The original codebase groups all type imports ( Excessive line length in line 15📍 ui/ts/components/OperationModal.tsx:15 Line 15 exceeds 200 characters, far beyond typical line length limits (80-120 characters). This severely reduces readability and maintainability. The line should be split, for example by extracting the selector string into an array and joining it. Incorrect file extension in import path📍 ui/ts/components/ResultBanner.tsx:1 The import statement uses a Import statements not in alphabetical order📍 ui/ts/components/SecurityPoolSection.tsx:12 The imports for local components are sorted alphabetically throughout the file. However, 'ResultBanner' is placed after 'RouteWorkflowPanel'. Alphabetically, 'ResultBanner' (starting with 'Res') should come before 'RouteWorkflowPanel' (starting with 'Rou'), so 'ResultBanner' is misplaced. Import statement placed before local component imports📍 ui/ts/components/SecurityPoolsOverviewSection.tsx:1 The import for 'useState' from 'preact/hooks' appears before the block of relative component imports. The original file groups all relative imports (from './') together, followed by external packages, internal libraries, and types. This new import breaks that grouping pattern. Overly long line in filter return statement📍 ui/ts/components/SecurityPoolsOverviewSection.tsx:67 The return statement in the filteredSecurityPools callback is a single line of over 250 characters, combining four includes checks. This makes the code difficult to read and conflicts with the repository's style of breaking long expressions. Inconsistent deployment bytecode encoding method📍 ui/ts/contracts/deploymentHelpers.ts:75 The function getPriceOracleManagerAndOperatorQueuerFactoryByteCode uses concatHex and encodeAbiParameters directly with hard-coded types, deviating from the established pattern in the same file where all other similar functions (e.g., getShareTokenFactoryByteCode, getEscalationGameFactoryByteCode) use encodeDeployData with the artifact ABI. This inconsistency reduces maintainability and risks divergence if the contract ABI changes but hard-coded types are not updated. Inconsistent conditional brace style📍 ui/ts/lib/securityPoolStage.ts:5 The first conditional (line 5) uses implicit return without braces, while all subsequent conditionals (lines 6, 16, 26, 36) use explicit braces. This violates consistency within the same function. Extra comma in function call arguments📍 ui/ts/tests/appStatusNotices.test.tsx:42 A trailing comma appears after the closing parenthesis of the h function call within the renderIntoDocument call. This is invalid syntax for a function call with a single argument and would cause a parse error, preventing the code from running. Imports not sorted alphabetically📍 ui/ts/tests/appStatusNotices.test.tsx:3 Third-party imports are not in alphabetical order, while local imports are sorted alphabetically. This inconsistency contradicts the apparent import sorting convention used in the file. The third-party imports should be reordered to '@testing-library/dom', 'bun:test', 'preact'. createBinaryForkQuestion missing overrides parameter and return type annotation📍 ui/ts/tests/marketSection.test.tsx:43 All test data factory functions in this file (createAccountState, createZoltarUniverse, createMarketSectionProps) declare an optional 'overrides' parameter of the appropriate Partial type and explicitly state their return type. The newly added createBinaryForkQuestion omits both, which breaks the established pattern, reduces type safety, and limits flexibility for future tests. External imports are not sorted alphabetically📍 ui/ts/tests/openOracleRouteSection.test.tsx:3 The external import statements for 'bun:test' and '@testing-library/dom' are not in alphabetical order. In this codebase, relative imports are already alphabetically sorted, indicating that alphabetical ordering is enforced across import groups. External imports should appear as: '@testing-library/dom' before 'bun:test'. Test file name case does not match component name📍 ui/ts/tests/reportingSection.test.tsx The test file is named 'reportingSection.test.tsx' (lowercase 'r'), while the component under test is named 'ReportingSection' (PascalCase). Consistent naming conventions in the codebase dictate that test files mirror the component's exact name, including case. |
🐛 Bug Hunter29 issues found Missing border style in .notice-stack-item modifier classesThe .notice-stack-item base class does not define any border. The modifier classes .warning, .pending, and .success only set border-color, which has no visible effect without a border width and style (e.g., border: 1px solid). This results in these notice items not displaying a border, contrary to likely design intent. Add a base border declaration (e.g., border: 1px solid var(--border-default)) to .notice-stack-item; the existing border-color overrides in the modifiers will then apply correctly. Inverted logic for showTransactionSuccessNotice on open-oracle and security-pools routesThe condition controlling the display of the transaction success notice is inverted for the 'open-oracle' and 'security-pools' routes. Currently, the notice is shown when the corresponding result variables are undefined (i.e., when no transaction has succeeded), and hidden when they are defined (i.e., when a transaction did succeed). This yields incorrect UI behavior: success notices appear inappropriately and actual successes are not indicated. The logic should be inverted to show the notice only when at least one relevant result is defined. TransactionActionButton enabled when action.onAction is null📍 ui/ts/components/ActionLauncherCard.tsx:23 The disabled prop for TransactionActionButton checks only for undefined on action.onAction (action.onAction === undefined), but the onClick handler uses optional chaining (action.onAction?.()), which safely handles both null and undefined. If action.onAction is null, the button will be enabled (since null !== undefined) but clicking it will do nothing because null?.() returns undefined. This results in a button that appears interactive but has no effect, which is incorrect behavior. Incorrect completion check for deployment sections📍 ui/ts/components/DeploymentRouteContent.tsx:69 The Create Child Universe transaction button lacks required field validation📍 ui/ts/components/ForkAuctionSection.tsx:616 The TransactionActionButton inside the OperationModal for creating a child universe is enabled even when prerequisite conditions are not met. It only checks wallet connection and network, but ignores two critical requirements: a loaded pool context (hasLoadedPoolContext) and a selected outcome (forkAuctionForm.selectedOutcome). This allows users to submit a transaction with missing parameters, which will revert or cause incorrect behavior. The button should be disabled when any requirement is unsatisfied, and the availability reason should indicate the specific unmet condition. The bug is in the renderStageActionButton call (line 616). It should be replaced with a TransactionActionButton that includes additional disabled checks for hasLoadedPoolContext and forkAuctionForm.selectedOutcome, along with appropriate reason messages. Duplicate keys in action lists when action strings are not unique📍 ui/ts/components/LifecycleStageBanner.tsx:20 The component uses the action string as the key for list items in both availableActions and blockedActions. If the same action string appears multiple times in either array, React will encounter duplicate keys, causing warnings and potential rendering inconsistencies. Incorrect pending label for child universe grid action button📍 ui/ts/components/MarketOverviewSection.tsx:122 The grid action button for child universes uses a pendingLabel of 'Opening...' while the pending condition is based on zoltarChildUniversePendingOutcomeIndex, which indicates a deployment transaction is in progress. The label should reflect the actual pending operation, e.g., 'Deploying universe...' to match the modal's own pendingLabel and the original behavior. This misleads users about the state of the operation. Missing universe guard in auto-load questions effect📍 ui/ts/components/MarketSection.tsx:115 The useEffect responsible for auto-loading Zoltar questions runs even when the current universe ID is undefined. It will call onLoadZoltarQuestions without a valid universe, which may cause unnecessary network requests or runtime errors. The effect should return early if currentUniverseId is undefined. Component returns undefined instead of null for empty items📍 ui/ts/components/NoticeStack.tsx:9 When the items array is empty, NoticeStack returns undefined, which is not a valid return value for a React component. This can cause runtime errors or unexpected rendering behavior, as React expects components to return React elements, null, or arrays. Returning undefined violates React's contract and should be corrected to return null. Dispute action lacks numeric validation for swap amounts📍 ui/ts/components/OpenOracleSection.tsx:168 The dispute readiness checklist ( Create Open Oracle game form lacks comprehensive input validation📍 ui/ts/components/OpenOracleSection.tsx:865 The create form readiness checklist ( Selected report modal state persists when changing reports📍 ui/ts/components/OpenOracleSection.tsx:800 The component uses local state Typo in event listener removal prevents cleanup📍 ui/ts/components/OperationModal.tsx:32 In the cleanup function of the useEffect, the event listener is removed using the string 'keykeyDown' instead of the correct event type 'keydown'. This mismatch means the keydown event listener added on line 30 is never removed, leading to memory leaks and potential duplicate event handlers when the modal is opened and closed multiple times, causing multiple invocations of the onClose callback on Escape key presses. WorkflowSummaryStrip receives an invalid currentStep value that does not match any step in the steps array📍 ui/ts/components/ReportingSection.tsx:135 The WorkflowSummaryStrip component is given currentStep={reportingStage?.label ?? 'Reporting'}, but the steps array is ['Reporting Open', 'Escalation', 'Withdrawal']. The fallback 'Reporting' is not a defined step. Additionally, reportingStage.label can be 'Pre-Reporting' (when the market end time has not passed) or an escalation phase string (e.g., 'Dispute') that also do not match any step. This results in the currentStep not aligning with the steps, preventing correct highlighting of the current workflow stage and misrepresenting the progress. Deployment modal lacks actionable content when no existing child universe is selected📍 ui/ts/components/ScalarDeploymentSection.tsx:131 The OperationModal's children are conditionally rendered only when selectedScalarChild is defined. For deploying a new child universe, selectedScalarChild is undefined (since it doesn't exist yet). This causes the modal to render no content and omit the 'Deploy Universe' button, making it impossible for users to deploy a new child universe via the UI. The modal should always show the requirements checklist and the deploy button, displaying an appropriate message when no child universe exists. Deposit REP action does not validate deposit amount positivity📍 ui/ts/components/SecurityPoolWorkflowSection.tsx:356 The depositGuardMessage condition does not check if depositAmount is defined and greater than zero. This allows the deposit action to proceed with invalid amounts (empty or zero), potentially causing onDepositRep to be called with an invalid amount, leading to transaction errors or unexpected behavior. Liquidate Vault action does not disable button when oracle price is invalid📍 ui/ts/components/SecurityPoolWorkflowSection.tsx:439 The 'liquidate-vault' action sets readiness to 'blocked' when oracle price is invalid, but does not set the blocker property for that condition. Since TransactionActionButton availability depends only on blocker and onAction, the button remains enabled when price is invalid, allowing users to attempt liquidation with an invalid oracle price, which should be prevented. workflow.securityPoolAddress not cleared when switching to browse or create views📍 ui/ts/components/SecurityPoolsSection.tsx:42 The openView function does not clear workflow.securityPoolAddress when switching to 'browse' or 'create' views, violating the invariant that workflow.securityPoolAddress should only be non-empty when view is 'operate'. This leads to stale selection state, such as having a pool selected in browse view after navigating from operate or create, which can cause incorrect behavior in components that rely on this invariant. Incorrect handling of null eyebrow prop📍 ui/ts/components/StickyObjectContext.tsx:13 The conditional rendering for the eyebrow prop only checks for undefined using Array passed instead of string for targetOutcomeIndexes in setAllTargetOutcomeIndexes📍 ui/ts/components/TradingSection.tsx:118 The setAllTargetOutcomeIndexes function calls onTradingFormChange with an array for the targetOutcomeIndexes property. However, the rest of the TradingSection component expects targetOutcomeIndexes to be a comma-separated string (as evidenced by the parsing logic using .split(',')). Passing an array will cause a TypeError when attempting to call .split on the array in subsequent renders, breaking functionality. Missing optional chaining and fallback for capacity values in Mint Complete Sets section📍 ui/ts/components/TradingSection.tsx:187 The capacity paragraph inside the Mint Complete Sets section directly accesses selectedPool.totalSecurityBondAllowance and selectedPool.totalRepDeposit without checking if they are defined. Since these properties are accessed elsewhere using optional chaining (e.g., selectedPool?.totalRepDeposit), they may be undefined. If they are undefined, calling .toString() on undefined will throw a TypeError, causing the component to crash. Incorrect readiness state for settle action in dispute mode📍 ui/ts/lib/openOracleReadiness.ts:47 In the 'dispute' actionMode, the 'settle-report' action unconditionally sets readiness to 'warning' when no blocker is present, regardless of connection status. This is inconsistent with other actions (e.g., 'initial-report' and 'settle' modes) where readiness becomes 'ready' when connected and no blocker exists. This bug will cause the settle action to appear in a warning state even when it should be ready, leading to incorrect UI behavior and potentially preventing users from settling when appropriate. Incorrect available action in dispute stage📍 ui/ts/lib/openOracleStage.ts:17 In the 'dispute' case, the availableActions array incorrectly includes 'Settle when the dispute window ends', implying that settling is an available action during the dispute window. However, settling should only become available after the dispute window ends. Additionally, blockedActions is empty, but it should block the settle action (e.g., 'Settle') to prevent users from attempting to settle during this stage. This leads to incorrect UI behavior and potential user confusion or errors. Test does not properly simulate local outcome for suppression scenario📍 ui/ts/tests/appStatusNotices.test.tsx:27 The test named 'suppresses global transaction success when the route already shows a local outcome summary' sets Test does not properly wait for modal to appear📍 ui/ts/tests/forkAuctionSection.test.tsx:162 The test 'launches create child universe in a focused modal' clicks the 'Open Child Universe Flow' button to open a modal. It then uses Type import paths incorrectly use .js extension📍 ui/ts/tests/openOracleRouteSection.test.tsx:9 The test file imports TypeScript types from '../types/app.js' and '../types/components.js'. TypeScript type imports require the source files to have a .ts, .tsx, or .d.ts extension; a .js file cannot provide type definitions. This will cause a compilation error, preventing the test from running at all. Race condition: 'Settle Report' button clicked without ensuring presence📍 ui/ts/tests/openOracleSection.integration.test.tsx:415 In the third integration test (starting around line 317), after verifying the contract state indicates action mode is 'settle', the test directly clicks the 'Settle Report' button without first waiting for it to be rendered in the DOM. This introduces a race condition where the UI may not have updated to display the button yet, causing intermittent test failures. The test for 'Initial Report' correctly waits for the button to appear before clicking, but this wait is missing for 'Settle Report'. Test expects dispute button in settle-only mode📍 ui/ts/tests/openOracleSection.test.tsx:467 The test 'renders settle-only controls after the dispute window closes' was changed to use renderSettleActionSection, which should render only settle controls. However, the test assertions still check for a dispute button to be present and a 'Dispute Report' section, while expecting the settle button to be undefined. This is inverted: in settle-only mode, the dispute button should not exist, the settle button should exist and be enabled, and the section title should be 'Settle Report' (not 'Dispute Report'). The test will fail because it looks for UI elements that are not rendered in this mode. Withdraw deposit indexes initialized as string instead of array📍 ui/ts/tests/reportingSection.test.tsx:71 In the createReportingForm helper function, the withdrawDepositIndexes property is initialized as an empty string (''). However, based on the property name (plural 'indexes') and typical usage in form state for multi-select fields, it should be an empty array ([]) to match the expected type ReportingFormState. This type mismatch can cause runtime errors in the ReportingSection component if it assumes an array and uses array methods (e.g., .map(), .filter(), .length), leading to test failures, exceptions, or incorrect behavior during rendering or interactions. |
🧠 Wise Man35 issues found Inconsistent bytecode encoding patterns and unused imports📍 solidity/ts/testsuite/simulator/utils/contracts/deployPeripherals.ts:2 The file uses two different methods to construct contract deployment bytecode: Recommendation: Revert Missing base border on .notice-stack-itemThe .notice-stack-item class defines layout (display: grid; gap) but does not include a border property. Its modifier classes (.blocking, .warning, .pending, .success) set border-color, expecting a border to be present. Without a base border (width and style), these color declarations have no effect. This is inconsistent with similar components like .action-launcher-card, which define a base border. Add a border definition to the .notice-stack-item base class, e.g., border: 1px solid var(--border-default); to ensure the border-color modifiers render correctly. Extract complex ternary logic into a clearly named functionThe ternary expression determining
Extract this into a pure function with a descriptive name like
The function should accept the relevant state values as parameters and return a boolean clearly mapping route-specific conditions. Missing input validation for required props📍 ui/ts/components/ActionReadinessPanel.tsx:10 The component assumes that the 'actions' prop is always a non-null array of objects each with a 'key' property. Without runtime validation, passing undefined, null, or an array with missing 'key' properties will cause runtime errors when mapping. Even with TypeScript compile-time checks, runtime validation is essential for data from external sources or props passed from non-TypeScript code. Potential performance issue from repeated O(n) operations📍 ui/ts/components/ActionReadinessPanel.tsx:18 The component calls 'countReadyActions(actions)' and also maps over 'actions' to render cards, resulting in two separate iterations over the same array. For large action lists, this doubles the iteration cost unnecessarily. Combining these into a single pass would reduce computational overhead. Destructuring order does not match type definition order📍 ui/ts/components/AppStatusNotices.tsx:22 The destructuring of props in the function signature does not follow the order defined in the AppStatusNoticesProps type. This inconsistency harms readability and makes it harder to compare the implementation with the type, potentially leading to maintenance errors. The properties should be reordered to match the type definition order exactly. Unnecessary div wrapper in deployment sections mapping📍 ui/ts/components/DeploymentRouteContent.tsx The mapping over deploymentSections introduces an extra wrapper for sections that are not fully deployed. This wrapper exists solely to attach a key prop and does not provide any semantic or styling value. Adding an unnecessary DOM node increases DOM complexity, may interfere with existing CSS (particularly the workflow-stack container), and deviates from the original structure where each section is a direct child. The key can be moved directly onto the DeploymentSection component, removing the wrapper and simplifying the markup.
Typographical error in JSX: extra closing brace📍 ui/ts/components/LifecycleStageBanner.tsx:24 Line 24 contains an extra closing brace '}' which introduces a syntax error. This would cause the code to fail parsing or runtime rendering. It should be corrected to match the structure of line 20 for consistency and correctness. Code duplication in rendering action lists📍 ui/ts/components/LifecycleStageBanner.tsx:18 The JSX for rendering available and blocked actions (lines 18-21 and 22-25) is duplicated, violating the DRY principle. This increases maintenance burden and risk of inconsistencies. Extract the repeated logic into a reusable helper function or component to improve clarity and reduce redundancy. Duplicated deployment readiness logic and messages📍 ui/ts/components/MarketOverviewSection.tsx:115 The deployment readiness condition and associated user-facing messages are duplicated in multiple places: the availability object within the ChildUniversesSection's action prop, the availability prop of the TransactionActionButton inside the OperationModal, and the childUniverseRequirements array. This violates the DRY principle and increases the risk of inconsistent behavior if the logic or messages change in one place but not others. Centralizing these checks and messages into a single helper or constant would improve maintainability and reduce future errors. Magic string literals for view identifiers are duplicated throughout the component📍 ui/ts/components/MarketSection.tsx The view identifiers 'questions', 'create', 'fork', and 'migrate' appear as string literals in multiple locations: conditional checks, tab options, and effect logic. This duplication violates the DRY principle and makes refactoring error-prone, as a change to one occurrence must be manually propagated to all others, risking inconsistencies. Extract these identifiers into a centralized constant or enum to ensure consistency and improve maintainability. Unstable callback dependency in useEffect📍 ui/ts/components/OperationModal.tsx:5 The useEffect hook depends on the Accessibility: initial focus and duplicated selector📍 ui/ts/components/OperationModal.tsx:5 The modal sets initial focus to the close button, violating WAI-ARIA best practices for modal dialogs. Focus should be set to the first focusable element or the dialog container to ensure a logical keyboard navigation order. Additionally, the CSS selector for focusable elements is hard-coded in the keydown handler; it should be extracted to a constant to avoid duplication and ease maintenance. Replace nested ternary operators in guard messages with guard clauses📍 ui/ts/components/ReportingSection.tsx:90 The reportGuardMessage and withdrawGuardMessage variables are defined using deeply nested ternary operators combined with nullish coalescing. This pattern is difficult to read, increases cognitive load, and makes future modifications error-prone. Refactoring these into linear guard clauses (early assignments) improves readability, aligns with best practices for clear control flow, and enhances maintainability. DRY violation: duplicate condition strings and logic for deployment requirements📍 ui/ts/components/ScalarDeploymentSection.tsx:52 The deployment requirements checklist and the button's availability reason are built from the same set of conditions but are duplicated in code. This leads to maintenance issues where a change to one condition must be reflected in two places. Refactor to a single source of truth for the conditions and derive both the checklist and the reason from it. Unnecessary dependency in useEffect causing redundant executions📍 ui/ts/components/ScalarDeploymentSection.tsx:70 The useEffect on lines 70-74 includes Duplication of error message for forked universe condition📍 ui/ts/components/SecurityPoolSection.tsx:240 The user-facing string 'Security pools cannot be created after Zoltar has forked.' appears identically in two locations: (1) as the Import statements are not organized by source and alphabetically sorted📍 ui/ts/components/SecurityPoolWorkflowSection.tsx:1 The file mixes third-party, relative, and type imports without a consistent ordering. Standard practice is to group imports by: standard library, external packages, internal absolute imports, then relative imports, with alphabetical sorting within groups. This improves readability and maintainability. Vault action modal identifiers are magic strings📍 ui/ts/components/SecurityPoolWorkflowSection.tsx:169 String literals 'deposit-rep', 'withdraw-rep', 'set-bond-allowance', 'claim-fees', and 'liquidate-vault' are repeated across type definitions, state initialization, event handlers, and conditional rendering. This duplication is error-prone and hinders refactoring and type safety. Define these as constant values (e.g., const enum or object) and reference them consistently. Component violates Single Responsibility Principle by mixing multiple workflows and modals📍 ui/ts/components/SecurityPoolWorkflowSection.tsx:214 The SecurityPoolWorkflowSection component handles four distinct views (vaults, trading, reporting, fork) and contains inline modal implementations for four vault operations. At over 1000 lines, this violates SRP, making the component hard to understand, test, and maintain. It should be decomposed into smaller, focused components: one per view and a reusable VaultOperationModal component. Guard message logic duplicates common precondition checks📍 ui/ts/components/SecurityPoolWorkflowSection.tsx:356 Each vault action defines a guard message with a long ternary chain checking similar preconditions (vault ownership, wallet connection, network, state). This duplication scatters business rules and risks inconsistencies. Extract a validation helper function that returns a structured readiness object for any action based on a declarative schema of required conditions. Modals use inline conditional rendering that obscures content structure📍 ui/ts/components/SecurityPoolWorkflowSection.tsx:886 Each OperationModal renders content like Duplicate event handler logic violates DRY principle📍 ui/ts/components/SecurityPoolsSection.tsx:74 The inline arrow functions for Inconsistent number formatting in Mint capacity description📍 ui/ts/components/TradingSection.tsx:187 The Mint Complete Sets section displays bigint values using Inconsistent deployment bytecode encoding approach📍 ui/ts/contracts/deploymentHelpers.ts:75 The function
Recommendation: Revert to using Inconsistent action representation in lifecycle stage presentations📍 ui/ts/lib/securityPoolStage.ts:9 The function returns LifecycleStagePresentation objects where availableActions and blockedActions fields contain a mix of action identifiers and descriptive messages across different conditions. For example, in the 'universe-mismatch' case, blockedActions lists specific action keys like 'Vault actions' and 'Trading', while in 'fork-active', blockedActions contains a general message 'Routine operational workflows may be limited'. Additionally, action strings vary (e.g., 'Vaults' vs 'Vault actions'), which can lead to mismatches if referenced elsewhere. This inconsistency compromises maintainability and may cause errors in consuming code that expects a uniform structure. Standardizing these through defined constants and ensuring blockedActions only includes action keys (with reasons in the detail field) would improve clarity and reduce risk. Test props duplication violates DRY principle📍 ui/ts/tests/appStatusNotices.test.tsx The props objects passed to AppStatusNotices in both test cases are nearly identical, with only showTransactionSuccessNotice differing. This duplication increases maintenance burden and risks inconsistencies when props are updated. Extracting a common base props object and using object spread with overrides in each test would improve maintainability and readability. Avoid mutable default parameters in helper function📍 ui/ts/tests/deploymentRouteContent.test.tsx:13 The Ensure all cleanup actions run even if one fails📍 ui/ts/tests/forkAuctionSection.test.tsx:126 The afterEach hook calls cleanupRenderedComponent and then restoreDomEnvironment sequentially. If cleanupRenderedComponent throws, restoreDomEnvironment will never be called, risking resource leakage between tests. Wrap each cleanup in a try/finally or separate try/catch to guarantee execution of all teardown steps regardless of errors. Use fireEvent.click instead of manual MouseEvent dispatch📍 ui/ts/tests/marketSection.test.tsx:282 The test 'opens the fork workflow in a modal' manually creates and dispatches a MouseEvent. This bypasses testing-library's event simulation which provides realistic user interaction and ensures proper cleanup. Manual event creation can lead to inconsistent test behavior. Replace with fireEvent.click, which is already imported. Replace magic await Promise.resolve() with waitFor for async UI updates📍 ui/ts/tests/marketSection.test.tsx:4 The test 'opens root-universe child-universe deployment in a modal' uses Refactor duplicated render action section functions📍 ui/ts/tests/openOracleSection.test.tsx:276 The functions renderDisputeActionSection and renderSettleActionSection contain nearly identical code, differing only in the action mode and the default openOracleReportDetails overrides. This duplication violates the DRY principle, making the code harder to maintain and increasing the risk of bugs when changes are made to one but not the other. A better design is to extract the common logic into a single parameterized helper function that takes mode and report details as arguments. This would improve readability, reduce complexity, and make test setup more consistent. Test assertion lacks precision for expected element count📍 ui/ts/tests/securityPoolWorkflowSection.test.tsx:356 The test uses Removed assertion may leave uncovered behavior📍 ui/ts/tests/securityPoolWorkflowSection.test.tsx:355 The replaced assertion checked for the presence of 'Claimable Fees' text. The new assertions check for a 'Vault Action Launchers' heading and 'Claim Fees' buttons, but do not verify that any reference to claimable fees remains in the DOM. If the UI still needs to display fee amounts or a label related to claimable fees, that expectation is now untested. Consider whether the semantic meaning of 'claimable fees' should still be validated somewhere in this test. Tests should use Testing Library queries instead of direct DOM selectors📍 ui/ts/tests/securityPoolsSection.test.ts:555 The test 'hides the truth auction metric when a listed pool has no truth auction address' uses document.body.querySelectorAll('.metric-label') to find elements. This couples the test to implementation details (CSS class names) and makes it brittle. Instead, use within(document.body).queryByText('Truth Auction') which is resilient to structural changes and aligns with user-centric testing. This is a known anti-pattern in Testing Library. |
- Move reporting, vault, pool, and migration guard logic into lib helpers - Add availability tests for deployment and transaction buttons - Reuse guard messaging across sections to keep UI disabled states consistent
- Remove the standalone action readiness panel and inline blocker copy - Embed action launchers in the Open Oracle and Security Pool sections - Rework selected pool summary layout and update affected tests
1 participant
Summary
Testing