Skip to content

Refresh open oracle and timestamp UI metadata#116

Merged
KillariDev merged 3 commits intomainfrom
feature/open-oracle-ui-refresh
Apr 13, 2026
Merged

Refresh open oracle and timestamp UI metadata#116
KillariDev merged 3 commits intomainfrom
feature/open-oracle-ui-refresh

Conversation

@KillariDev
Copy link
Copy Markdown
Collaborator

Summary

  • Updated open oracle, pool, reporting, and question views to show richer metadata and more readable token-specific labels.
  • Added a reusable TimestampValue component that renders absolute and relative times consistently across the UI.
  • Improved open oracle report details with refreshed pricing actions, token symbols, callback metadata, and additional status fields.
  • Tightened wallet and pool loading states so balances and pool metadata only appear when the relevant bootstrap/data loading is complete.

Testing

  • Not run (PR summary only)
  • Expected validation path: bun tsc
  • Expected validation path: bun test
  • Expected validation path: bun run format
  • Expected validation path: bun run check
  • Expected validation path: bun run knip

- add token symbols and richer report details
- let users refresh price and choose approval amounts
- improve loading states and defer wallet balance display
- add a reusable timestamp component with relative and zero-state display
- switch UI sections to the new UTC timestamp formatting
- add formatter coverage for absolute and relative timestamps
- Consolidate repeated metric rendering into `MetricField`
- Add shared view, input, and parsing helpers with tests
- Refresh oracle, market, reporting, and security pool sections
@github-actions
Copy link
Copy Markdown

🛡️ Defender Against the Dark Arts

3 issues found

Empty string handling for className and valueClassName creates inconsistent attribute rendering

📍 ui/ts/components/MetricField.tsx:12

The ternary conditions on lines 12 and 14 treat empty string ('') as falsy by converting to undefined, but explicitly allow undefined as a valid prop value. This creates an inconsistent state where:

  • className='' → renders no class attribute (undefined)
  • className=undefined → renders class="undefined" (string)
    This inconsistency can lead to unexpected DOM attributes and potential CSS selector confusion. The pattern also suggests possible misunderstanding of how Preact/React handle falsy className values (they already treat undefined/null/false as no attribute).

While not a direct security vulnerability, this pattern could be exploited in phishing scenarios where attackers manipulate class names to mimic legitimate UI elements by passing empty strings vs undefined in different contexts.

Potential supply chain risk from new utility imports

📍 ui/ts/hooks/useSecurityVaultOperations.ts:8

The diff introduces three new imports from local utility files: 'sameAddress' from '../lib/address.js', 'requireDefined' from '../lib/required.js', and 'runLoadRequest' from '../lib/loadState.js'. While these appear to be internal utilities, their sudden introduction without review could indicate a supply chain compromise if these files were recently added or modified by an untrusted source. Malicious code hidden in these utilities could exfiltrate data, bypass security checks, or introduce backdoors, especially since 'runLoadRequest' abstracts loading logic and 'requireDefined' handles error throwing.

Address comparison replacement with custom 'sameAddress' function

📍 ui/ts/hooks/useSecurityVaultOperations.ts:76

The diff replaces direct string comparison with case-insensitive toLowerCase() calls with a custom 'sameAddress' function in multiple places (lines 76, 103, 206). While this may improve address comparison safety, it introduces a dependency on an unverified utility. If 'sameAddress' is implemented incorrectly or maliciously (e.g., always returning true for certain addresses), it could allow unauthorized access to vault operations or bypass read-only checks, posing a security risk.

@github-actions
Copy link
Copy Markdown

🏛️ Architect

9 issues found

Inconsistent naming convention for timestamp modifier class

📍 ui/css/index.css:593

The stylesheet follows a pattern where state modifiers are separate classes applied alongside the base class (e.g., .currency-value.loading, .timestamp-value.loading). The newly added .timestamp-value-relative combines the base and modifier into a single hyphenated name, violating this convention. This risks misuse (e.g., applying .timestamp-value-relative without .timestamp-value, missing inherited font and color) and breaks naming consistency. Rename the modifier to .relative and use the selector .timestamp-value.relative, ensuring the element carries both classes.

Direct window object usage in useState initializer breaks server-side rendering

📍 ui/ts/components/SecurityPoolWorkflowSection.tsx:59

The useState initializer at line 59 directly accesses window.location.search, which will throw a ReferenceError in server-side rendering or non-browser environments. This tightly couples the component to the browser, violating clean architecture principles and preventing universal rendering. Guard the access with a typeof check or defer to useEffect.

Misuse of void operator with async function

📍 ui/ts/hooks/useOpenOracleOperations.ts:257

The refreshPrice arrow function uses the void operator to discard the promise returned by refreshOpenOracleInitialReportState. If the async operation fails, the rejection will be unhandled, potentially causing unhandled promise rejection errors that can crash the app or lead to silent failures. The correct approach is to either return the promise to the caller or attach a catch handler to handle errors.

Inconsistent loading abstraction leads to architectural drift

📍 ui/ts/hooks/useSecurityPoolCreation.ts:39

The loadMarketById function was refactored to use the runLoadRequest abstraction, which centralizes request guarding, loading state transitions, and error handling. However, loadDuplicateOriginPoolState retains a manual try/catch implementation that duplicates this logic. This violates DRY, increases maintenance burden, and creates inconsistency in how asynchronous loading operations are handled across the same module. The manual implementation also risks leaving loading state stuck (as seen in the finally block that only clears if isCurrent()), a bug that the abstraction would prevent.

Inconsistent error handling strategy introduced by runLoadRequest abstraction

📍 ui/ts/hooks/useZoltarUniverse.ts:70

The refactoring introduces runLoadRequest with onError: () => undefined across all loading functions, which suppresses errors and prevents proper error propagation. This changes the error handling contract: previously, loadZoltarUniverse would throw on errors (caught in createChildUniverse), but now it resolves to undefined, causing ensureZoltarUniverse to throw generic errors and masking root causes. This violates the principle of explicit error handling and hinders debugging and maintainability.

Logical inconsistency in case-insensitive comparison handling of undefined values

📍 ui/ts/lib/caseInsensitive.ts:8

The function sameCaseInsensitiveText checks normalizedLeft !== undefined but does not symmetrically check normalizedRight. This leads to inconsistent behavior when both inputs are undefined: it returns false, whereas typical equality semantics would consider two undefined values as equal. This violates the principle of symmetric comparison and could cause subtle bugs in conditional logic across the codebase, especially if this utility is used for key comparisons or validations.

Breaking change: removal of parseDecimalInput function

📍 ui/ts/lib/inputs.ts

The parseDecimalInput function, which was exported and used for parsing decimal string inputs with unit conversion, has been removed without replacement. The new parseOptionalBigIntInput function does not handle decimal parsing and has a different signature, causing backward compatibility issues. Callers of parseDecimalInput will encounter runtime errors. This violates API stability and may lead to code duplication as callers implement their own decimal parsing or import viem directly, leading to inconsistency.

Breaking change: Removal of exported constant REPORTING_OUTCOME_OPTIONS

📍 ui/ts/lib/reporting.ts:4

The constant REPORTING_OUTCOME_OPTIONS was previously exported and likely imported by other modules for general use. Changing it to a non-exported internal constant without providing an alternative or deprecation path will cause TypeScript compilation errors for existing importers, breaking backward compatibility and potentially disrupting dependent code across the codebase.

Architectural boundary violation by introducing utility dependence in domain logic

📍 ui/ts/lib/reportingDomain.ts:2

The reporting domain function getEscalationTimeRemaining now directly depends on utility modules (./time.js and ./required.js). This creates inappropriate coupling between the domain layer and utility/helper layers, violating separation of concerns. Domain logic should remain independent of infrastructural or utility concerns; validation and time calculation utilities belong in a different layer. This coupling makes the domain logic harder to test in isolation and introduces hidden dependencies.

@github-actions
Copy link
Copy Markdown

🔒 Security Reviewer

1 issue found

Premature bootstrap completion signal allows race condition

📍 ui/ts/hooks/useOnchainState.ts:117

The walletBootstrapComplete flag is set to true immediately after address retrieval (line 117) and in the catch block (line 140), before balance/chainId loading completes. This creates a race condition where components may assume wallet initialization is fully complete while sensitive operations (balance checks, chain validation) are still pending. An attacker could exploit this timing window to trigger wallet-dependent functionality before proper authorization or state validation occurs. The flag should only be set after all wallet state loading attempts have settled (success or failure), as originally implemented in the finally block.

@github-actions
Copy link
Copy Markdown

🔧 Expensive Linter

15 issues found

Trailing comma in JSON array

📍 .claude/settings.local.json:41

The last element in the 'allow' array contains a trailing comma, which violates the JSON standard (RFC 8259). This can cause parsing errors in strict JSON parsers, potentially breaking configuration loading.

Inconsistent CSS selector grouping for timestamp-value states

📍 ui/css/index.css:587

The new rules for .timestamp-value.loading, .timestamp-value.unavailable, and .timestamp-value.zero are grouped into a single rule, while the existing codebase consistently defines each state separately (e.g., .currency-value.loading, .currency-value.unavailable). This violates the established pattern of separate rules for each modifier class, reducing readability and maintainability.

Missing commas in type definition

📍 ui/ts/components/MetricField.tsx:4

TypeScript object type properties must be separated by commas or semicolons. The MetricFieldProps type definition lacks commas between properties, resulting in a syntax error that prevents compilation.

Unsafe calls to normalizeAddress with possibly undefined values

📍 ui/ts/components/SecurityPoolWorkflowSection.tsx:71

The refactoring replaced safe optional-chaining .toLowerCase() calls with normalizeAddress without guarding against undefined. This can throw runtime errors when selectedPoolManagerAddress, selectedPool?.securityPoolAddress, or accountState.address are undefined (e.g., when no pool is selected or wallet not connected). These calls should be guarded.

  • Line 71: selectedPoolManagerAddress may be undefined when no pool selected.
  • Line 87: selectedPool?.securityPoolAddress may be undefined when pool not found.
  • Line 89: accountState.address may be undefined when wallet not connected.

Trailing comma in useState initializer function call

📍 ui/ts/components/SecurityPoolsSection.tsx:18

The useState initializer contains an unnecessary trailing comma after the arrow function argument. This is a clear deviation from the existing codebase style, which does not use trailing commas in function calls.

Inconsistent use of MetricField component in compact layout for latest action

📍 ui/ts/components/SecurityVaultSection.tsx:180

The 'latestAction' constant uses paragraph elements to display Action and Transaction values, while the MetricField component is used for all other metric displays throughout the component in both compact and full layouts. This breaks consistency in UI component usage and styling.

Import order not alphabetical

📍 ui/ts/components/ZoltarMigrationSection.tsx:3

The imports from './' are not sorted alphabetically. The combined import of getMigrationOutcomeSplitLimit, MigrationOutcomeUniversesSection (line 10) appears after TransactionHashLink and UniverseLink, but should appear between MetricField and TransactionHashLink to maintain alphabetical order consistent with other import groups.

Non-alphabetical import order for '../lib/' modules

📍 ui/ts/hooks/useSecurityVaultOperations.ts:7

Imports from '../lib/' are not sorted alphabetically, deviating from the repository's established style. The current order (clients, address, errors, inputs, marketForm, required, securityVault, loadState, writeAction) should be alphabetical: address, clients, errors, inputs, loadState, marketForm, required, securityVault, writeAction. Additionally, the marketForm import is split into two lines instead of being combined into a single statement.

Inconsistent error handling pattern

📍 ui/ts/hooks/useZoltarFork.ts:31

The getZoltarAddress function now uses requireDefined for undefined checking, while other functions in the same file (e.g., loadZoltarForkAccess) use explicit if checks for undefined values. This creates a stylistic inconsistency in how undefined values are handled.

Import order not alphabetical within group

📍 ui/ts/hooks/useZoltarUniverse.ts:7

The new import runLoadRequest from '../lib/loadState.js' is placed after '../lib/marketCreation.js' and before '../lib/requestGuard.js', breaking alphabetical ordering within the ../lib/ import group. The file's established pattern alphabetizes imports from ../lib/ (clients, errors, marketCreation, requestGuard). To maintain consistency, the new import should be inserted between errors.js and marketCreation.js.

Missing optional chaining operator before toLowerCase call

📍 ui/ts/lib/caseInsensitive.ts:2

The function normalizeCaseInsensitiveText is intended to safely handle undefined inputs, but the current implementation will throw a TypeError when value is undefined because it attempts to call toLowerCase on undefined. The optional chaining operator should also be applied to the toLowerCase call: value?.trim()?.toLowerCase().

Inconsistent spacing after colon in type annotations

📍 ui/ts/lib/loadState.ts:2

In the type RunLoadRequestParameters, optional properties (isCurrent, onStart, onError) have a space after the colon in their type annotations, while required properties (setLoading, load, onSuccess) do not, violating consistent formatting conventions.

Inconsistent export status for similarly named constants

📍 ui/ts/lib/reporting.ts:4

The constant REPORTING_OUTCOME_OPTIONS was previously exported but is now internal, while REPORTING_OUTCOME_DROPDOWN_OPTIONS is exported. Both use uppercase naming, which typically indicates exported constants in TypeScript codebases. This creates a naming inconsistency where an uppercase constant is not exported, potentially violating conventions that reserve uppercase for exported values.

Incorrect indentation of top-level describe block

📍 ui/ts/tests/address.test.ts:8

The describe block starting at line 8 is indented with a leading tab, but top-level statements should have no indentation. This causes the entire test suite to be over-indented and violates common code formatting standards. The closing brace on line 25 also has an extra tab.

Inconsistent arrow function body style in test callbacks

📍 ui/ts/tests/loadState.test.ts:64

The onSuccess callback in the third test (line 64) uses an expression body (onSuccess: () => undefined), while all other callbacks (setLoading, onStart, onSuccess in other tests, onError) use block bodies. This inconsistency violates code style uniformity. For maintainability and readability, all callbacks should consistently use block bodies, especially since some callbacks require multiple statements.

@github-actions
Copy link
Copy Markdown

🐛 Bug Hunter

25 issues found

TypeError in getTruthAuctionWindow due to bigint and number addition

📍 ui/ts/components/ForkAuctionSection.tsx:20

The function getTruthAuctionWindow attempts to add details.truthAuctionStartedAt (a bigint, as evidenced by comparison to 0n) to AUCTION_TIME_SECONDS (likely a number, based on its usage in formatDuration calls). In JavaScript, adding a bigint and a number throws a TypeError, causing the component to crash when auctionWindow is calculated for non-zero truthAuctionStartedAt.

Incorrect conditional logic for className and valueClassName handling

📍 ui/ts/components/MetricField.tsx:12

The ternary conditions className === '' ? undefined : className and valueClassName === '' ? undefined : valueClassName incorrectly treat empty strings as equivalent to undefined. In HTML/React, an empty string for className results in a class="" attribute, which is valid but often unnecessary. However, converting empty strings to undefined changes the rendered output by omitting the attribute entirely. This can alter behavior in CSS or JavaScript that checks for the presence of the className attribute, not just its value. For example, CSS attribute selectors like [class] would match an element with class="" but not with no class attribute. This is a functional difference that can break styles or scripts relying on attribute existence.

Incorrect currency suffix for Fee amount in report details

📍 ui/ts/components/OpenOracleSection.tsx:332

The 'Fee' field in the Economics section of the report details card hardcodes the currency suffix as 'ETH'. However, the fee amount's denomination depends on the feeToken flag: when feeToken is true, the fee is in token1 and should display token1Symbol; when false, it is in ETH. The hardcoded 'ETH' suffix leads to incorrect display when feeToken is true, potentially misleading users about the fee's currency.

Missing Created At timestamp in report displays

📍 ui/ts/components/OpenOracleSection.tsx

The 'Created At' timestamp, indicating when the Open Oracle report was created, is missing from both the report summary card and the report details card. In the summary card, it was previously shown after 'Current Amount2' and before 'Report Timestamp'. In the details card, it was under the Status section before 'Report Timestamp'. The underlying data (report.createdAt and openOracleReportDetails.createdAt) is available but not rendered, resulting in incomplete information for users.

Zero timestamp rendered without time semantics and relative information

📍 ui/ts/components/TimestampValue.tsx:43

When timestamp equals 0n and zeroText is not provided, the component returns a containing only the absolute timestamp. This deviates from the normal rendering for other timestamps, which uses a element with both absolute and relative timestamps and includes a dateTime attribute. The zero case also omits the relative time and the title tooltip, resulting in incomplete information and reduced accessibility.

Extra wrapper div breaks consistent grid structure for workflow metrics

📍 ui/ts/components/ZoltarMigrationSection.tsx:224

The Approved REP metric and its associated detail paragraph are wrapped in an unnecessary

within the workflow-metric-grid, while other metrics are direct children. This inconsistent DOM hierarchy breaks the expected CSS grid layout, causing misalignment and styling issues. The detail paragraph should be a direct child of workflow-metric-grid (as in the original structure) rather than nested inside a wrapper div.

Race condition in loadOracleReport due to missing request guard

📍 ui/ts/hooks/useOpenOracleOperations.ts:88

The loadOracleReport function does not use an isCurrent guard when calling runLoadRequest, allowing concurrent requests to race. An earlier slower request can overwrite state after a later request has already updated it, causing stale or mismatched report details to be displayed and form fields (like reportId) becoming inconsistent with the loaded data. This leads to incorrect UI behavior and potential user confusion.

parseApproveAmount silently falls back to default approval amount on invalid input

📍 ui/ts/hooks/useOpenOracleOperations.ts:166

When parseBigIntInput throws due to invalid input, parseApproveAmount catches the error and returns the default OPEN_ORACLE_APPROVAL_AMOUNT. This causes user-provided invalid amounts to be ignored without any error feedback, resulting in unintended approval amounts being used. The function should propagate the error to fail the action and allow the user to correct the input.

Whitespace-only input in withdrawDepositIndexes not treated as empty

📍 ui/ts/hooks/useReportingOperations.ts:74

The old implementation used trim() to check if withdrawDepositIndexes was empty, correctly treating whitespace-only input as empty and defaulting to all deposit indexes. The new code passes the raw input directly to resolveOptionalBigIntListInput without trimming, which likely does not consider whitespace-only strings empty. This causes a behavioral regression: users entering spaces (e.g., accidental whitespace) will trigger a parse error or incorrect fallback instead of defaulting to all deposits.

Incorrect type comparison in market details reuse check

📍 ui/ts/hooks/useSecurityPoolCreation.ts:149

The condition in the createPool function compares a bigint (marketDetails.value?.questionId) to a string (parameters.questionId.toString()), which always evaluates to false due to type mismatch. This prevents the intended reuse of already loaded market details, causing unnecessary API calls and potential data inconsistency if market details change между loads.

Balance/allowance loading errors clear vault details

📍 ui/ts/hooks/useSecurityVaultOperations.ts:76

In loadSecurityVault, when loading a vault for the connected user (vaultAddress equals accountAddress), the function awaits reloadSecurityVaultRepBalance and reloadSecurityVaultRepAllowance. If either of these reloads fails (e.g., network error), the exception propagates out of the load function. runLoadRequest then invokes onError, which clears securityVaultDetails and sets an error state, even though the vault details were successfully retrieved. This results in the vault details being lost and an error displayed, which is incorrect: balance/allowance loading failures should be isolated and must not invalidate the successfully loaded vault details.

Incorrect zoltarUniverseMissing flag when Zoltar step is not deployed

📍 ui/ts/hooks/useZoltarUniverse.ts:75

In the refactored loadZoltarUniverse, the onSuccess handler sets zoltarUniverseMissing to true whenever the loaded universe is undefined. This overrides the correct value (false) that was set earlier when the Zoltar contract hasn't been deployed, leading to a false 'universe missing' state. The original code avoided this by returning early before the missing flag was set to true.

normalizeCaseInsensitiveText throws TypeError for undefined input

📍 ui/ts/lib/caseInsensitive.ts:1

The function normalizeCaseInsensitiveText uses optional chaining only for the trim() call, but then unconditionally calls toLowerCase() on the result. When value is undefined, value?.trim() returns undefined, and calling toLowerCase() on undefined throws a TypeError. The function's return type (string | undefined) and its usage in sameCaseInsensitiveText clearly expect it to return undefined for undefined input, not throw.

Incorrect normalization of '.' input in normalizeDecimalInput

📍 ui/ts/lib/decimal.ts:6

The function normalizeDecimalInput does not properly handle the input '.'. It returns '0.' which may not be parsable by parseUnits, whereas for input '0.', it correctly returns '0.0'. This inconsistency leads to parsing errors for '.' input, causing parseDecimalInput to throw a generic error when '.' is provided. The normalization logic should ensure that any string with a decimal point has at least one digit after it, similar to how '0.' is handled.

getTimeRemaining may return negative values after refactoring

📍 ui/ts/lib/forkAuction.ts:46

The new implementation of getTimeRemaining delegates to getSharedTimeRemaining without ensuring the result is non-negative when targetTime is less than or equal to currentTime. The previous implementation explicitly returned 0n in such cases. If getSharedTimeRemaining does not clamp negative values to zero, this function will return negative bigint values for past target times, leading to incorrect countdown displays and potential logical errors in downstream calculations.

resolveOptionalBigIntListInput throws on empty list and undefined inputs instead of returning fallback

📍 ui/ts/lib/inputs.ts:54

The function is intended to provide a fallback when an optional bigint list input is not meaningfully provided. However, it only returns the fallback when the entire input string is empty after trimming. It fails to handle (a) inputs that are non-empty strings but yield no valid entries after splitting and filtering (e.g., ',' or ' , , '), causing parseBigIntListInput to throw an error, and (b) undefined values, which cause a TypeError when calling trim(). This breaks the optional resolver contract and can lead to unexpected crashes.

Callbacks before try block bypass cleanup on error

📍 ui/ts/lib/loadState.ts:12

The setLoading(true) and onStart?.() calls are placed before the try block. If either throws an exception, the finally block that resets the loading state never executes. This results in the loading state remaining true indefinitely, causing a resource leak and UI malfunction. Moving these calls inside the try block ensures the finally block runs even if they fail, properly resetting the loading state.

Incorrect validation placement in getEscalationTimeRemaining

📍 ui/ts/lib/reportingDomain.ts:6

The function uses requireDefined on the result of getTimeRemaining to enforce that details.escalationEndTime is defined. However, requireDefined should be applied directly to details.escalationEndTime before calling getTimeRemaining. This misplaced validation means the intended error ('Escalation end time is required') will not be thrown when escalationEndTime is undefined because getTimeRemaining will likely throw a TypeError (mixing BigInt and undefined) during arithmetic, preventing requireDefined from executing. Even if getTimeRemaining returns undefined for other reasons, the error message would be misleading. The current implementation does not reliably validate the required input and leads to incorrect error handling.

No functional issues found

📍 ui/ts/lib/required.ts

The requireDefined function correctly checks for undefined values and throws an error with the provided message when undefined. It returns the value otherwise. No logic mistakes, incorrect behavior, or edge case failures are present in the code as written.

Incorrect import extension for TypeScript file

📍 ui/ts/lib/securityVault.ts:2

The import statement uses './address.js' extension, but in a TypeScript codebase, local imports should typically use './address' or './address.ts' to reference the TypeScript source file. Using .js may cause module resolution issues in development environments or with certain TypeScript configurations, leading to runtime errors if the corresponding .js file is not present or properly compiled.

Runtime type error due to missing input validation

📍 ui/ts/lib/time.ts:3

The function getTimeRemaining can throw a TypeError when targetTime or currentTime are not of type bigint. Specifically, the comparison on line 3 using <= will throw if targetTime is not a bigint (or undefined, which is handled) or if currentTime is not a bigint. This leads to a runtime crash in scenarios where invalid types are passed, violating the expected behavior of returning a time remaining value or undefined.

Test expects incorrect capitalization for 'invalid' outcome

📍 ui/ts/tests/forkAuction.test.ts:8

The test expects 'Invalid' for input 'invalid', but based on common labeling conventions and consistency with 'yes'/'Yes' and 'no'/'No', the expected output should likely be 'invalid' (lowercase) to match the input case pattern. The current expectation 'Invalid' with capital 'I' is inconsistent and may cause the test to fail if the implementation follows standard case matching.

Incorrect expected output for formatRelativeTimestamp with large future timestamp

📍 ui/ts/tests/formatters.test.ts

The test expects 'in 1d 1h 1m 1s' for a difference of 90,061 seconds, but the calculation for days, hours, minutes, and seconds is incorrect. 90,061 seconds equals 1 day, 1 hour, 1 minute, and 1 second only if the remainder after subtracting days is exactly 3,661 seconds (1 hour = 3,600 seconds, remainder 61 seconds; 1 minute = 60 seconds, remainder 1 second). However, 90,061 - 86,400 = 3,661, and 3,661 - 3,600 = 61, and 61 - 60 = 1, so the breakdown is correct. But the test expects the string 'in 1d 1h 1m 1s', which is consistent with the breakdown. Upon re-evaluation, the breakdown is accurate, so this is not an issue.

Incorrect assertion for error message in test

📍 ui/ts/tests/required.test.ts:10

The test on line 10 uses toThrow('missing') which performs substring matching on error messages. This allows errors with messages like 'missing value' to pass the test, but the expected behavior is to throw an error with exactly the message 'missing'. This can result in false positives and inadequate test coverage, potentially allowing bugs in the implementation to go unnoticed.

Type error in resolveEnumValue test cases

📍 ui/ts/tests/viewState.test.ts:9

In the test for resolveEnumValue, calls on lines 9 and 10 specify generic type 'browse' | 'create' but pass 'invalid' and undefined which are not assignable to this union type, causing TypeScript compilation errors.

@github-actions
Copy link
Copy Markdown

🧠 Wise Man

28 issues found

Inconsistent naming and formatting in exit code echo commands

📍 .claude/settings.local.json:25

The allow list contains multiple variations of commands that echo exit codes, with inconsistent casing (EXIT, exit, Exit), inconsistent spacing (e.g., 'EXIT:$?' vs 'EXIT: $?'), and semantically ambiguous naming ('Done' vs 'Exit'). This violates consistency and clarity principles, making the configuration harder to understand and maintain. Standardizing to a single consistent pattern would improve long-term readability and reduce cognitive load.

Missing white-space: nowrap in .timestamp-value base class

📍 ui/css/index.css:582

The .timestamp-value class does not include white-space: nowrap, unlike similar value display classes (.currency-value, .address-value) which enforce single-line display. This inconsistency can cause timestamps to wrap unexpectedly in UI layouts, leading to visual breaks or misalignment. Adding white-space: nowrap ensures consistent behavior across all value elements and prevents potential layout issues.

Inconsistent state initialization pattern with direct window access

📍 ui/ts/components/MarketSection.tsx:57

The useState initializer directly accesses window.location.search, which is a browser global. This can cause issues if the component is ever rendered in a non-browser environment (e.g., server-side rendering or testing with JSDOM). While the existing code already used window, the refactor maintained this pattern instead of abstracting browser-specific logic. Consider using a custom hook or abstraction to read URL parameters that safely handles non-browser environments.

Fee amount suffix hardcoded to 'ETH' ignoring feeToken indicator

📍 ui/ts/components/OpenOracleSection.tsx:331

In the Economics section, the fee amount is always displayed with suffix 'ETH' regardless of the actual fee token. The OpenOracleReportDetails includes a feeToken boolean that indicates whether the fee is denominated in token1 (true) or ETH (false). This mismatch can mislead users into thinking the fee is always in ETH, which is inconsistent with the separate Fee Token field. The suffix should be dynamic: token1Symbol when feeToken is true, otherwise 'ETH'.

Magic strings for view state identifiers should be replaced with constants

📍 ui/ts/components/OpenOracleSection.tsx:484

String literals 'browse', 'create', and 'selected-report' are repeatedly used to represent view states. This pattern is error-prone and reduces readability. Define these as named constants (e.g., VIEW_BROWSE, VIEW_CREATE, VIEW_SELECTED_REPORT) at the top of the file and use them consistently in all state updates and comparisons.

Incorrect use of MetricField component for Universe metric with actions

📍 ui/ts/components/OverviewPanels.tsx:90

The MetricField component is designed for simple label-value pairs with a single child representing the value. For the Universe metric, it is used with multiple children: the value text and a conditional actions div. This deviates from the expected usage pattern and will alter the HTML structure—placing the actions inside the value container (e.g., a element) if MetricField wraps children—whereas the original structure kept actions as a sibling to the value element. This change can break existing CSS selectors or styling that rely on the original DOM hierarchy, and introduces inconsistency in how MetricField is applied across metrics.

Repeated address normalization and long expressions hinder readability

📍 ui/ts/components/SecurityVaultSection.tsx:92

The autoLoadKey and hasLoadedCurrentVault computations repeat the normalizeAddress(... ) ?? '' pattern and are expressed in long, single-line statements. This violates DRY principles and reduces readability. Extract the normalized vault and pool addresses into well-named constants and break the hasLoadedCurrentVault condition across multiple lines to clarify intent and improve maintainability.

Unnecessary interval timer when currentTimestamp is provided

📍 ui/ts/components/TimestampValue.tsx:21

The useEffect hook unconditionally sets up an interval to update the 'now' state every second. However, when the currentTimestamp prop is provided, 'now' should remain static and does not require periodic updates. This results in wasted resources due to unnecessary timer callbacks and state update attempts, which can degrade performance especially with multiple component instances.

Inconsistent loading pattern for fork auction details

📍 ui/ts/hooks/useForkAuctionOperations.ts:65

The loadForkAuction function was refactored to use runLoadRequest, centralizing loading state and error handling. However, runForkAuctionAction still loads details inline when not cached, duplicating logic and leading to inconsistent UX (missing loading indicator, potentially misleading error messages). All loading of fork auction details should go through the same abstraction.

Misuse of void operator with async function call

📍 ui/ts/hooks/useOpenOracleOperations.ts:266

The refreshPrice method uses the void operator to call refreshOpenOracleInitialReportState, an async function. This discards the returned promise, potentially leading to unhandled promise rejections and silent failures. The void operator should be removed to allow proper promise handling by callers.

Redundant await in load callback

📍 ui/ts/hooks/usePriceOracleManager.ts:32

The load callback passed to runLoadRequest uses an async arrow function with an unnecessary await expression. Since loadOracleManagerDetails returns a promise, the await can be omitted to simplify the code and reduce cognitive load without changing behavior.

Unnecessary async/await in refreshState callback

📍 ui/ts/hooks/usePriceOracleManager.ts:51

The refreshState callback in runWriteAction is defined as an async function that only awaits loadPoolOracleManager. Since loadPoolOracleManager returns a promise, the async keyword and await can be removed, simplifying the code to a direct return of the promise.

Missing error handling in reporting details reload after successful actions

📍 ui/ts/hooks/useReportingOperations.ts:55

The success callback in runReportingAction reloads reporting details without handling potential errors from loadReportingDetails. If this reload fails, the error propagates as an unhandled promise rejection, potentially causing unresponsive UI or inconsistent state. Add a try-catch block to capture errors and update reportingError accordingly.

Inconsistent use of runLoadRequest abstraction

📍 ui/ts/hooks/useSecurityPoolCreation.ts:39

The function loadDuplicateOriginPoolState manually manages loading state, error handling, and request cancellation with a try-catch-finally block, while loadMarketById uses the runLoadRequest helper for the same pattern. This inconsistency violates DRY principles, increases cognitive load, and makes the codebase harder to maintain. All similar loading operations should adopt the runLoadRequest abstraction to ensure uniform behavior and reduce duplicated boilerplate.

Effect dependency array throws when zoltarUniverse is undefined and misses critical dependencies

📍 ui/ts/hooks/useZoltarFork.ts:194

The effect dependency array at lines 194-196 includes an expression that calls .map() on zoltarUniverse?.childUniverses without checking for undefined, which throws a TypeError when zoltarUniverse is undefined. Additionally, the dependency array omits zoltarUniverse?.universeId and zoltarUniverse?.reputationToken as separate dependencies, while the effect's callback (loadZoltarForkAccess) uses these properties directly. This causes the effect to not re-run when these values change, leading to stale data. The current approach also creates a new array/string on every render from childUniverses.map(), which is inefficient and destabilizes the dependency.

Callback functions are re-created on every render due to ineffective memoization

📍 ui/ts/hooks/useZoltarFork.ts:52

The hook returns object properties that include function references (loadZoltarForkAccess, approveZoltarForkRep, forkZoltar). These functions are re-created on every render because:

  1. loadZoltarForkAccess is not wrapped in useCallback at all.
  2. approveZoltarForkRep is wrapped in useCallback but depends on runZoltarForkAction, which itself is not memoized and changes on every render, making the useCallback ineffective.
  3. forkZoltar is not wrapped in useCallback.

This causes the hook's return object to be a new object on every render, breaking referential equality and causing unnecessary re-renders in any parent component that uses React.memo/Preact.memo or useMemo on the returned values. The functions should be memoized with useCallback and proper dependency arrays to ensure stable references across renders when their inputs haven't changed.

Incorrect zoltarUniverseMissing state when Zoltar step not deployed

📍 ui/ts/hooks/useZoltarUniverse.ts:70

The refactoring incorrectly sets zoltarUniverseMissing to true when the Zoltar deployment step hasn't been completed. Originally, the early return inside the loadZoltarUniverse try block prevented the later logic that sets zoltarUniverseMissing from running, leaving it as false. After moving the check inside runLoadRequest's load callback, the function returns undefined, which is treated as a successful result and triggers onSuccess. The onSuccess handler then sets zoltarUniverseMissing based on requestedUniverseId !== 0n, incorrectly marking the universe as missing. This breaks the intended semantics: when Zoltar isn't deployed, the universe isn't considered missing; it's simply unavailable.

Redundant string trimming in parseDecimalInput

📍 ui/ts/lib/decimal.ts:10

The function parseDecimalInput trims the input value (line 10) before passing it to normalizeDecimalInput, which also trims the input internally (line 4). This results in unnecessary double trimming, wasting computational resources and violating the DRY principle. Trimming logic should be centralized to improve efficiency and maintainability.

Duplicate duration breakdown logic in formatRelativeDuration and formatDuration

📍 ui/ts/lib/formatters.ts

Both formatRelativeDuration (lines 84-103) and formatDuration (lines 112-122) independently compute days, hours, and minutes from a bigint seconds value using identical arithmetic. This violates the DRY principle and increases maintenance burden; any change to the breakdown logic must be duplicated. Extract a shared helper function (e.g., getDurationComponents) that returns the components and refactor both formatters to use it, improving consistency and reducing future errors.

Duplicate bigint parsing logic in parseOptionalBigIntInput

📍 ui/ts/lib/inputs.ts:30

The parseOptionalBigIntInput function reimplements bigint parsing by directly calling BigInt, duplicating the logic already present in the existing parseBigIntInput utility. This duplication increases maintenance overhead and can lead to inconsistencies. It should be refactored to reuse parseBigIntInput wrapped in a try/catch, returning undefined on error.

Guard initial side effects with isCurrent check

📍 ui/ts/lib/loadState.ts:11

The function calls setLoading(true) and onStart() without first verifying that the request is still current. This can cause unnecessary UI updates for outdated requests, such as a persistent loading indicator or spurious onStart callbacks, because these side effects are performed even when the request is no longer relevant. To ensure only the current request affects shared state, guard these initial side effects with the same isCurrent check used after the async load.

Complex nested ternary for priceSource assignment

📍 ui/ts/lib/openOracle.ts:157

The expression determining priceSource in deriveOpenOracleInitialReportSubmissionDetails (line 157) is a deeply nested ternary that is difficult to read, understand, and maintain. This pattern violates the principle of explicitness and increases cognitive load. The same logic expressed with guard clauses or a dedicated helper function would be significantly clearer and less error-prone for future modifications.

Redundant repeated string trimming

📍 ui/ts/lib/openOracle.ts:147

The priceInput.trim() method is called multiple times (lines 147, 157) for the same input value. This is inefficient and creates unnecessary repetition. The trimmed value should be computed once and reused, which also improves consistency and reduces the chance of inconsistencies if trimming logic changes.

Hardcoded duplicate outcome labels

📍 ui/ts/lib/reporting.ts:16

The getReportingOutcomeLabel function hardcodes the labels for 'invalid', 'yes', and 'no', which are already defined in the REPORTING_OUTCOME_OPTIONS array. This violates the DRY principle and risks inconsistencies if labels are updated in one location but not the other.

Inconsistent capitalization in error message

📍 ui/ts/lib/scalarOutcome.ts:68

The error message on line 68 reads 'Scalar max must be greater than scalar min', which inconsistently uses a lowercase 's' in 'scalar min'. All other error messages and label strings consistently capitalize 'Scalar' (e.g., 'Scalar min', 'Scalar max', 'Scalar increment'). This should be corrected to 'Scalar max must be greater than Scalar min' for consistency and professionalism.

Missing explicit return type annotation

📍 ui/ts/lib/time.ts:1

The exported function getTimeRemaining lacks an explicit return type annotation. In TypeScript, explicitly annotating return types for public functions is a best practice that enhances type safety, improves code readability, and provides clear documentation for consumers. This helps prevent unintended type changes and makes the function's contract immediately apparent.

Test file mixes tests for multiple unrelated modules

📍 ui/ts/tests/address.test.ts

The test file address.test.ts imports and tests functions from two distinct modules: address.js (normalizeAddress, sameAddress) and caseInsensitive.js (sameCaseInsensitiveText). This violates the common convention of one test file per module, reducing discoverability and increasing maintenance overhead. Consider splitting the tests: keep address-related tests in address.test.ts and move the generic case-insensitive text tests to a separate caseInsensitive.test.ts file.

Misleading boolean field name feeToken

📍 ui/ts/types/contracts.ts:217

The field feeToken is declared as boolean in OpenOracleReportDetails, but its name suggests it should hold a token address (e.g., Address). This mismatched naming can mislead developers and lead to incorrect usage, such as attempting to treat the value as an address. To align with TypeScript and clean code conventions, rename the field to a boolean-friendly name like isFeeToken or feeInToken to clearly communicate its true nature.

@KillariDev KillariDev merged commit 82e7ea9 into main Apr 13, 2026
6 checks passed
@KillariDev KillariDev deleted the feature/open-oracle-ui-refresh branch April 13, 2026 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant