-
Notifications
You must be signed in to change notification settings - Fork 7
Remove config source #1689
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove config source #1689
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/ui-components/src/lib/types/appStores.ts (1)
15-19
: Property types and naming updated for consistency.The interface correctly:
- Renames
subgraphUrl
tosubgraph
- Updates from string type to
SubgraphCfg
type- Updates
activeOrderbook
to useOrderbookCfg
- Updates
activeAccounts
to useAccountCfg
These changes align with the PR objectives of removing the config source and splitting the Config struct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
packages/ui-components/src/__tests__/DeploymentSteps.test.ts
(4 hunks)packages/ui-components/src/__tests__/DropdownActiveSubgraphs.test.ts
(2 hunks)packages/ui-components/src/lib/__mocks__/settings.ts
(1 hunks)packages/ui-components/src/lib/components/ListViewOrderbookFilters.svelte
(2 hunks)packages/ui-components/src/lib/components/deployment/DeploymentSteps.svelte
(3 hunks)packages/ui-components/src/lib/components/dropdown/DropdownActiveSubgraphs.svelte
(1 hunks)packages/ui-components/src/lib/components/tables/OrdersListTable.svelte
(1 hunks)packages/ui-components/src/lib/components/tables/VaultsListTable.svelte
(4 hunks)packages/ui-components/src/lib/index.ts
(1 hunks)packages/ui-components/src/lib/types/appStores.ts
(1 hunks)tauri-app/src/routes/orders/[network]-[orderHash]/+page.svelte
(1 hunks)tauri-app/src/routes/orders/[network]-[orderHash]/page.test.ts
(4 hunks)tauri-app/src/routes/orders/add/+page.svelte
(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/ui-components/src/__tests__/DeploymentSteps.test.ts (1)
packages/ui-components/src/lib/__mocks__/settings.ts (1)
mockConfig
(4-68)
packages/ui-components/src/__tests__/DropdownActiveSubgraphs.test.ts (1)
packages/ui-components/src/lib/__mocks__/settings.ts (1)
mockConfig
(4-68)
tauri-app/src/routes/orders/[network]-[orderHash]/page.test.ts (1)
packages/ui-components/src/lib/__mocks__/stores.ts (1)
mockSettingsStore
(42-46)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: test
- GitHub Check: Deploy-Preview
- GitHub Check: git-clean
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (40)
packages/ui-components/src/lib/components/tables/OrdersListTable.svelte (1)
46-52
: Updated configuration access pattern.The code correctly implements the new configuration structure where settings are now nested under the
orderbook
namespace, and subgraph entries use object format with expliciturl
property. This change aligns with the broader refactoring of removing theConfigSource
type.packages/ui-components/src/lib/components/deployment/DeploymentSteps.svelte (3)
6-7
: Updated import type from ConfigSource to Config.The import correctly reflects the new configuration structure introduced in this PR.
45-45
: Updated settings type from Writable to Writable.The prop type is properly updated to use the new Config type.
74-74
: Updated subgraph URL access pattern.The code now correctly accesses the subgraph URL through the nested
orderbook
structure and the expliciturl
property.tauri-app/src/routes/orders/[network]-[orderHash]/+page.svelte (3)
27-28
: Updated orderbooks access pattern.The code now correctly accesses orderbook configuration through the nested
orderbook
structure, aligning with the new Config type implementation.
31-33
: Updated subgraphs access pattern.The code now correctly accesses subgraph configuration through the nested
orderbook
structure and explicitly uses theurl
property.
35-37
: Updated networks access pattern.The code now correctly accesses network configuration through the nested
orderbook
structure.packages/ui-components/src/lib/index.ts (1)
134-134
: Renamed export to match new config structure.The mock export name has been updated from
mockConfigSource
tomockConfig
to reflect the transition from the removedConfigSource
type to the newConfig
type.packages/ui-components/src/__tests__/DeploymentSteps.test.ts (4)
13-13
: Import updated to use new Config structure.The import has been updated from
mockConfigSource
tomockConfig
to align with the PR objective of removing the config source and using the new nested Config structure.
93-93
: Updated settings prop to use the new Config structure.The
settings
prop indefaultProps
now correctly useswritable(mockConfig)
instead of the previouswritable(mockConfigSource)
.
166-166
: Updated config access pattern to use nested structure.The access to the subgraph URL has been correctly updated to use
mockConfig.orderbook.subgraphs.mainnet.url
instead of accessing it directly from the root level of the config object.
469-469
: Updated config access pattern for Flare subgraph.The access to the Flare subgraph URL has been correctly updated to use
mockConfig.orderbook.subgraphs.flare.url
following the new nested Config structure.packages/ui-components/src/__tests__/DropdownActiveSubgraphs.test.ts (4)
5-6
: Updated imports for new Config structure.The imports have been properly updated to use
mockConfig
instead ofmockConfigSource
and import the newConfig
andSubgraphCfg
types from@rainlanguage/orderbook
.
10-27
: Refactored mock settings to use nested Config structure.The mock settings have been correctly restructured to nest subgraphs under the
orderbook
namespace with each subgraph now havingkey
andurl
properties instead of being direct string values.
28-28
: Updated type definition for activeSubgraphsStore.The type of
activeSubgraphsStore
has been properly updated fromWritable<Record<string, string>>
toWritable<Record<string, SubgraphCfg>>
to match the new subgraph configuration type.
71-99
: Updated test assertions to match new Config structure.The test assertions have been correctly updated to expect objects with
key
andurl
properties instead of direct string values, matching the new Config structure.tauri-app/src/routes/orders/[network]-[orderHash]/page.test.ts (4)
4-4
: Updated import to use Config type.The import has been properly updated to use the
Config
type from@rainlanguage/orderbook
instead of the previousConfigSource
type.
60-90
: Refactored mock settings to use nested Config structure.The mock settings object has been correctly restructured to use a nested
orderbook
object containingorderbooks
,subgraphs
, andnetworks
keys. Each entity now includes the appropriate properties likekey
,chainId
(instead ofchain-id
), etc., and the object is properly cast to theConfig
type.
100-100
: Updated empty settings structure.The empty settings object has been correctly updated to use
{ orderbook: {} }
as the starting point, aligning with the new Config structure.
118-130
: Updated partial settings to use nested structure.The partial settings object has been correctly refactored to nest
orderbooks
andnetworks
under theorderbook
namespace, consistent with the new Config structure.packages/ui-components/src/lib/components/ListViewOrderbookFilters.svelte (3)
11-11
: Updated type imports for new Config structure.The import statement has been properly updated to import
AccountCfg
,Config
, andSubgraphCfg
types from@rainlanguage/orderbook
instead of the previousConfigSource
type.
15-16
: Updated prop type definitions for new Config structure.The type annotations for the exported props have been correctly updated:
settings
now usesWritable<Config | undefined>
accounts
now usesReadable<Record<string, AccountCfg>>
activeSubgraphs
now usesWritable<Record<string, SubgraphCfg>>
These changes align with the new typed Config structure where simple string mappings are replaced with richer objects.
Also applies to: 20-20
33-33
: Updated conditional check for nested Config structure.The conditional check for displaying the "No networks added" alert has been correctly updated to check
isEmpty($settings?.orderbook?.networks)
instead ofisEmpty($settings?.networks)
, reflecting the new nested structure.packages/ui-components/src/lib/components/dropdown/DropdownActiveSubgraphs.svelte (3)
4-7
: Type updates correctly align with the new Config structure.The component has been properly updated to use the new
Config
andSubgraphCfg
types from@rainlanguage/orderbook
, aligning with the PR objective of removing the config source.
9-15
: Accessing updated nested structure correctly.The component now properly accesses subgraphs through the new nested path
settings.orderbook.subgraphs
instead of the previous flat structure. This change is consistent with splitting the Config struct intoOrderbookConfig
andDotrainOrderConfig
.
17-23
: Event handler updated to work with the new SubgraphCfg type.The event handler now correctly processes
Record<string, SubgraphCfg>
objects rather than simple strings, matching the more structured configuration approach introduced in this PR.packages/ui-components/src/lib/__mocks__/settings.ts (2)
1-68
: Mock configuration structure correctly updated to match new Config type.The mock has been extensively restructured to match the new nested configuration approach with separate
orderbook
namespace. This properly reflects the splitting of theConfig
struct intoOrderbookConfig
andDotrainOrderConfig
as mentioned in the PR objectives.
70-76
: Store types updated to reflect new Config structure.The writable store and helper methods have been properly updated to use the new
Config
type, ensuring type safety throughout the application.tauri-app/src/routes/orders/add/+page.svelte (4)
28-28
: Import updated to match new configuration API.The import has been simplified to only include the new
mergeDotrainConfigWithSettings
function, removing the now obsolete conversion functions.
48-48
: Access pattern updated for new nested configuration structure.Deployments are now correctly accessed through the nested
dotrainOrder
property, reflecting the new configuration structure where data is split intoDotrainOrderConfig
andOrderbookConfig
.
78-81
: Updated reactive statement to use new config structure.The reactive statement for generating Rainlang strings has been properly updated to access scenarios through the nested
dotrainOrder
property.
125-128
: Config merging simplified with direct approach.The function now directly uses
mergeDotrainConfigWithSettings
which returns aConfig
instance, removing the intermediate conversion steps that were previously needed withConfigSource
.packages/ui-components/src/lib/types/appStores.ts (2)
1-3
: Import types from the new Config structure.The import statement has been correctly updated to use the new types from the reorganized configuration model.
5-8
: Core store interface properties updated to use new Config types.The main configuration stores have been properly updated to use
Config
,SubgraphCfg
, andAccountCfg
types, ensuring type safety throughout the application.packages/ui-components/src/lib/components/tables/VaultsListTable.svelte (6)
13-18
: Import statements updated for new configuration typesThe import statements have been correctly updated to reference the new configuration types (
AccountCfg
,Config
,OrderbookCfg
,SubgraphCfg
) which replace the previousConfigSource
andOrderbookConfigSource
types. This change aligns with the PR objective of removing the config source and splitting the Config struct.
30-31
: Props updated to use new typed configuration objectsThe props have been properly updated to use the new configuration types. Notably:
activeOrderbook
now usesOrderbookCfg
instead of the previous typesubgraphUrl
has been renamed tosubgraph
with typeSubgraphCfg
These changes are consistent with the move from string-based configuration to structured, typed configuration objects.
35-37
: Collection types updated for configuration changesThe collection type props have been updated to use the new configuration structure:
activeSubgraphs
now uses a record ofSubgraphCfg
objectssettings
now uses the newConfig
typeThis improves type safety and aligns with the new configuration model where configs are strongly typed objects rather than string mappings.
41-43
: ActiveAccounts type updated to use AccountCfgThe
activeAccounts
property has been correctly updated to use the newAccountCfg
type, maintaining consistency with the rest of the configuration changes.
54-61
: Updated multiSubgraphArgs to use new nested configuration structureThe reactive computation for
multiSubgraphArgs
has been properly updated to reflect the new nested configuration structure where subgraphs are now accessed via$settings?.orderbook?.subgraphs
instead of directly from$settings?.subgraphs
. The code also correctly extracts the URL property from each subgraph configuration object.
94-95
: Updated query enablement conditionThe query enablement condition has been updated from checking
$subgraphUrl
to checking$subgraph
, which is appropriate given the prop rename and type change. This ensures the query only runs when a valid subgraph configuration is available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
packages/ui-components/src/__tests__/ListViewOrderbookFilters.test.ts (2)
56-74
:⚠️ Potential issueFix missing
orderbook
wrapper inbeforeEach
reset
ThebeforeEach
block resetsmockSettings
with an object missing the top-levelorderbook
property, which the component relies on at runtime (settings.orderbook.networks
). This will cause undefined lookups.Apply this diff to wrap networks and subgraphs under
orderbook
:beforeEach(() => { - mockSettings.set({ - networks: { - ethereum: { - key: 'ethereum', - rpc: 'https://rpc.ankr.com/eth', - chainId: 1, - networkId: 1, - currency: 'ETH' - } - }, - subgraphs: { - mainnet: { - key: 'mainnet', - url: 'mainnet-url' - } - } - } as unknown as Config); + mockSettings.set({ + orderbook: { + networks: { + ethereum: { + key: 'ethereum', + rpc: 'https://rpc.ankr.com/eth', + chainId: 1, + networkId: 1, + currency: 'ETH' + } + }, + subgraphs: { + mainnet: { + key: 'mainnet', + url: 'mainnet-url' + } + } + } + } as unknown as Config); });
76-82
:⚠️ Potential issueWrap empty networks/subgraphs under
orderbook
in test
In the “shows no networks alert” test, themockSettings.set
also omits theorderbook
wrapper. Since the component readssettings.orderbook.networks
, this will break the alert logic.Use this diff:
test('shows no networks alert when networks are empty', () => { - mockSettings.set({ networks: {}, subgraphs: {} } as unknown as Config); + mockSettings.set({ + orderbook: { + networks: {}, + subgraphs: {} + } + } as unknown as Config); render(ListViewOrderbookFilters, defaultProps);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/ui-components/src/__tests__/ListViewOrderbookFilters.test.ts
(3 hunks)
🔇 Additional comments (1)
packages/ui-components/src/__tests__/ListViewOrderbookFilters.test.ts (1)
5-5
: Import updated to newConfig
type
The test now importsConfig
instead of the legacyConfigSource
, which aligns with the PR objective to remove the config source. Ensure that@rainlanguage/orderbook
exportsConfig
from the correct module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
♻️ Duplicate comments (4)
tauri-app/src/lib/services/pickConfig.ts (1)
18-21
: 🧹 Nitpick (assertive)Add explicit return type annotation to pickScenarios function.
Similar to pickDeployments, this function would benefit from an explicit return type annotation.
export function pickScenarios( mergedConfig: Config | undefined, activeNetworkRef: string | undefined, -) { +): Record<string, any> {The exact return type should match what's in the
scenarios
structure from the Config type.packages/ui-components/src/__tests__/ListViewOrderbookFilters.test.ts (1)
23-41
: Mock still usesas unknown as Config
– consider safer typingThe double-cast keeps popping up and could easily mask structural drift between the real
Config
type and the fixture.
APartial<Config>
(or a purpose-built factory) narrows that risk while preserving flexibility.I flagged the very same pattern in an earlier round and understand you decided to leave it as-is, so marking this as a duplicate for traceability only.
Also applies to: 56-76
packages/webapp/src/routes/+layout.ts (2)
13-15
: Avoid hard-coding the remote settings URLEmbedding a SHA-pinned GitHub raw URL forces a redeploy every time
settings.yaml
changes and makes local/offline dev painful.
Read the endpoint from an env variable (e.g.PUBLIC_SETTINGS_URL
) or server-side config instead.```diff
-const response = await fetch(
- 'https://raw.githubusercontent.com/findolor/sample-dotrains/6865d8c76dc4182c5f6f41c20299cb017a6298fe/settings.yaml'
-);
+const response = await fetch(import.meta.env.PUBLIC_SETTINGS_URL);
```
30-37
: 🧹 Nitpick (assertive)Null-safe access in
activeNetworkOrderbooks
orderbook.network
could beundefined
in malformed configs; accessing.key
directly will throw. A tiny guard keeps the UI alive:- (orderbook) => orderbook.network.key === $activeNetworkRef + (orderbook) => orderbook.network?.key === $activeNetworkRef(No functional change for valid data.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
packages/ui-components/src/__tests__/ListViewOrderbookFilters.test.ts
(3 hunks)packages/webapp/src/routes/+layout.ts
(16 hunks)tauri-app/src/lib/services/pickConfig.ts
(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tauri-app/src/lib/services/pickConfig.ts (1)
tauri-app/src/lib/stores/settings.ts (1)
activeNetworkRef
(65-65)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: git-clean
- GitHub Check: Deploy-Preview
- GitHub Check: test
- GitHub Check: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: test
🔇 Additional comments (1)
packages/webapp/src/routes/+layout.ts (1)
192-196
:❌ Incorrect review comment
parseYaml
is not mocked – tests will explode at runtime
vi.mocked(parseYaml).mockReturnValue(...)
works only ifparseYaml
is already a Jest/Vitest mock.
Unlesspackages/webapp/test-setup.ts
callsvi.mock('@rainlanguage/orderbook')
,
.mockReturnValue
isundefined
⇒ TypeError: mockReturnValue is not a function.Quick fix:
vi.mock('@rainlanguage/orderbook', () => ({ parseYaml: vi.fn(), })); import { parseYaml } from '@rainlanguage/orderbook';(or
vi.spyOn
if you want to preserve the real implementation elsewhere).Also applies to: 231-234, 248-252, 268-272, 298-302, 342-344, 380-383
🏁 Script executed:
#!/bin/bash # Locate test-setup.ts setup_file=$(fd test-setup.ts | head -n1) echo "Test setup file: $setup_file" echo "-----------" sed -n '1,200p' "$setup_file" echo echo "Searching for mocks of '@rainlanguage/orderbook' across the repo" rg -n "rainlanguage/orderbook" .Length of output: 22905
🏁 Script executed:
#!/bin/bash # Display the first 200 lines of the webapp test-setup to check for parseYaml mock setup_file_webapp=$(fd --ignore-case test-setup.ts | grep packages/webapp/test-setup.ts | head -n1) echo "Webapp Test setup file: $setup_file_webapp" echo "-----------" sed -n '1,200p' "$setup_file_webapp"Length of output: 801
parseYaml is already mocked in webapp’s test-setup – no change needed
The
packages/webapp/test-setup.ts
file contains:vi.mock('@rainlanguage/orderbook', () => { const DotrainOrderGui = vi.fn(); // …other method mocks… return { DotrainOrderGui, parseYaml: vi.fn() }; });Since
parseYaml
is defined as avi.fn()
, yourvi.mocked(parseYaml).mockReturnValue(...)
calls are valid and will not throw.No updates required here.
Likely an incorrect or invalid review comment.
|
||
export interface LayoutData { | ||
stores: AppStoresInterface; | ||
} | ||
|
||
export const load = async ({ fetch }) => { | ||
const response = await fetch( | ||
'https://raw.githubusercontent.com/rainlanguage/rain.strategies/refs/heads/main/settings.json' | ||
'https://raw.githubusercontent.com/findolor/sample-dotrains/6865d8c76dc4182c5f6f41c20299cb017a6298fe/settings.yaml' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to update rain.strategies instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
packages/ui-components/src/lib/utils/configHelpers.ts (1)
14-45
: 🧹 Nitpick (assertive)Inline tests ship to production bundle
The
import.meta.vitest
guard is tree-shaken only when using Vite. If this
code is consumed by other bundlers (Rollup, Webpack, ts-node via Jest,
etc.) the test payload is included in the distributable artefact,
inflating size and leaking dev-only deps.Consider moving the tests to
src/lib/utils/__tests__/configHelpers.test.ts
(or similar) instead.packages/webapp/src/routes/+layout.ts (1)
61-63
:⚠️ Potential issue
activeSubgraphs
should storeSubgraphCfg
, not raw stringsDown-stream helpers and components assume the value shape
{ key, url }
.
Storing plain URLs breaks those assumptions (see
VaultsListTable.svelte → getMultiSubgraphArgs
).-import type { Config, OrderbookCfg, OrderbookConfig } from '@rainlanguage/orderbook'; +import type { + Config, + OrderbookCfg, + OrderbookConfig, + SubgraphCfg, +} from '@rainlanguage/orderbook'; … - activeSubgraphs: writable<Record<string, string>>({}), + activeSubgraphs: writable<Record<string, SubgraphCfg>>({}),Remember to update any producers of this store (fetch logic, tests, etc.).
♻️ Duplicate comments (1)
packages/webapp/src/routes/+layout.ts (1)
30-37
: 🧹 Nitpick (assertive)Left-over
unknown as
cast can be eliminatedNow that
pickBy
is imported tree-shake-friendly, we can drop the
unknown as
escape hatch and rely on proper typing, e.g.:if (!$settings?.orderbooks) return {}; return Object.fromEntries( Object.entries($settings.orderbooks).filter( ([, ob]) => ob.network.key === $activeNetworkRef, ), ) as Record<string, OrderbookCfg>;Removes an
any
back-door and improves maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
packages/ui-components/src/lib/components/dropdown/DropdownOrderListAccounts.svelte
(1 hunks)packages/ui-components/src/lib/components/tables/VaultsListTable.svelte
(4 hunks)packages/ui-components/src/lib/utils/configHelpers.ts
(1 hunks)packages/webapp/src/routes/+layout.ts
(16 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/webapp/src/routes/+layout.ts (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1749
File: tauri-app/src/lib/test/settings.test.ts:8-111
Timestamp: 2025-05-07T06:58:32.478Z
Learning: In the rain.orderbook project, stores like `activeAccountsItems` and `activeSubgraphs` are derived stores that should not be directly updated, as their values are derived from the `settings` store.
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: test
- GitHub Check: git-clean
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: Deploy-Preview
- GitHub Check: test
- GitHub Check: test
packages/ui-components/src/lib/components/dropdown/DropdownOrderListAccounts.svelte
Outdated
Show resolved
Hide resolved
packages/ui-components/src/lib/components/tables/VaultsListTable.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 29
🔭 Outside diff range comments (1)
crates/js_api/src/gui/order_operations.rs (1)
260-277
: 🧹 Nitpick (assertive)Skip zero-amount deposits to avoid an RPC hit
check_allowance
is invoked even whendeposit_amount == 0
, which costs an
extra RPC round-trip per token and clutters logs. Bail out early:-for (token_address, deposit_amount) in deposits_map { - let deposit_args = DepositArgs { … }; - let token_allowance = self.check_allowance(&deposit_args, &owner).await?; - … +for (token_address, deposit_amount) in deposits_map { + if deposit_amount.is_zero() { + continue; + } + …
♻️ Duplicate comments (4)
packages/ui-components/src/lib/components/dropdown/DropdownOrderListAccounts.svelte (1)
2-10
: Fixed potential undefined store dereference and updated typeThe updates properly address a potential runtime error by providing a default empty readable store for the
accounts
prop, ensuring that$accounts
will never throw an error when dereferenced. The type is also updated to use the more structuredAccountCfg
type instead of simple strings.tauri-app/src/lib/services/pickConfig.ts (1)
22-27
: 💡 Verification agent🧩 Analysis chain
Similar issue with nested property access in pickScenarios.
The code assumes that
d.deployer.network.key
will always be defined, but if any deployment scenario lacks a deployer or network configuration, this could cause runtime errors.
🏁 Script executed:
#!/bin/bash # Check for null checking patterns around deployer.network.key access rg -A 5 'deployer\.network\.key' # Check if there are tests that verify behavior with incomplete scenario data rg -A 10 'pickScenarios.*undefined' --include='*.test.ts'Length of output: 3407
🏁 Script executed:
#!/bin/bash rg -n -C3 'deployer\.network\.key' tauri-app/src/lib/services/pickConfig.tsLength of output: 515
🏁 Script executed:
#!/bin/bash sed -n '1,200p' tauri-app/src/lib/services/pickConfig.tsLength of output: 814
Ensure null-safe nested property access in pickScenarios
In
pickScenarios
you’re accessingd.deployer.network.key
directly, which will throw if a scenario is missingdeployer
ornetwork
. Add an optional-chain or guard to handle undefined cases:Suggested diff in
tauri-app/src/lib/services/pickConfig.ts
(lines 22–27):return mergedConfig?.dotrainOrder.scenarios ? pickBy( - mergedConfig.dotrainOrder.scenarios, - (d) => d.deployer.network.key === activeNetworkRef, + mergedConfig.dotrainOrder.scenarios, + (d) => + // guard against missing deployer or network + d.deployer?.network?.key === activeNetworkRef, ) : {};• File: tauri-app/src/lib/services/pickConfig.ts
• Function:pickScenarios
(lines 22–27)Also add tests covering scenarios with missing
deployer
ornetwork
to prevent regressions.packages/ui-components/src/__tests__/ListViewOrderbookFilters.test.ts (1)
29-47
: Usingas unknown as Config
for mocks – already discussed previously
This double cast remains; since the author explicitly declined the earlier suggestion to tighten the mock type, no further action is taken here.tauri-app/src/lib/stores/settings.ts (1)
88-94
: Heavy cast hides type driftCasting
orderbooks
throughunknown
defeats type-safety and was raised in
previous reviews. If you still preferpickBy
, you can keep strong typing:return pickBy( $settings.orderbook.orderbooks, (o): o is OrderbookCfg => o.network.key === $activeNetworkRef, );No extra runtime cost, but the compiler now guards future refactors.
🛑 Comments failed to post (29)
packages/ui-components/src/lib/components/detail/VaultDetail.svelte (1)
43-43: 💡 Verification agent
🧩 Analysis chain
Updated configuration access pattern.
The code now accesses the subgraph URL using a new path structure, which aligns with the PR's objective of restructuring the configuration. This change ensures the component works with the new
Config
structure that has replaced the previousConfigSource
.
🏁 Script executed:
#!/bin/bash # Check if the new configuration structure is consistently applied across the codebase rg -g "*.svelte" -g "*.ts" "subgraphs\?\.\[.*\]\.url" --context=2Length of output: 67
🏁 Script executed:
#!/bin/bash # Search for any usage of `subgraphs` across .svelte and .ts files to verify the new access pattern rg -n -g "*.svelte" -g "*.ts" "subgraphs" -C2Length of output: 27092
Inconsistent subgraph config access across components
VaultDetail.svelte now correctly uses the new top-level
subgraphs
path ($settings?.subgraphs?.[network]?.url
), but several other components are still accessing subgraphs undersettings.orderbook.subgraphs
. To avoid runtime errors and maintain a single source of truth, please update all instances of:
$settings.orderbook.subgraphs[…]
to
$settings.subgraphs[…]
Key locations needing attention:
- packages/ui-components/src/lib/components/deployment/DeploymentSteps.svelte:74
- packages/ui-components/src/lib/components/dropdown/DropdownActiveSubgraphs.svelte:9
- packages/ui-components/src/lib/components/tables/OrdersListTable.svelte:47
- (And any other
.svelte
/.ts
files referencingsettings.orderbook.subgraphs
)Example replacement in DeploymentSteps.svelte:
- subgraphUrl = $settings.orderbook.subgraphs[value].url; + subgraphUrl = $settings.subgraphs[value]?.url || '';Please apply the same pattern wherever the old path remains.
Committable suggestion skipped: line range outside the PR's diff.
packages/ui-components/src/lib/components/checkbox/CheckboxActiveOrders.svelte (1)
7-7: 🧹 Nitpick (assertive)
Consider using more explicit reactive logic
The current reactive statement might be a bit confusing at first glance. It might be clearer to express the relationship between the checkbox state and the store value more explicitly.
-$: activeOrderStatus.set(includeInactive ? undefined : true); +$: { + // When includeInactive is true, set to undefined (show all orders) + // When includeInactive is false, set to true (show only active orders) + activeOrderStatus.set(includeInactive ? undefined : true); +}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.$: { // When includeInactive is true, set to undefined (show all orders) // When includeInactive is false, set to true (show only active orders) activeOrderStatus.set(includeInactive ? undefined : true); }
tauri-app/src/lib/services/pickConfig.ts (1)
8-15: 💡 Verification agent
🧩 Analysis chain
Consider adding optional chaining for nested object access.
While
dotrainOrder
may not be optional, accessing nested properties likescenarios[d.scenario.key]
could result in runtime errors if any intermediate values are undefined (e.g., if a deployment has an invalid scenario key).
🏁 Script executed:
#!/bin/bash # Check if there are cases where deployment scenario keys might not exist in scenarios ast-grep --pattern $'mergedConfig.dotrainOrder.scenarios[d.scenario.key]' rg -A 5 'scenario\.key' | grep -v 'optional'Length of output: 6610
🏁 Script executed:
#!/bin/bash # Display the relevant file to inspect types and context sed -n '1,200p' tauri-app/src/lib/services/pickConfig.tsLength of output: 814
Add optional chaining to guard against missing scenario entries
To prevent a runtime exception when a deployment’s
scenario.key
isn’t present inmergedConfig.dotrainOrder.scenarios
, update the predicate inpickDeployments
to safely access nested properties:• File:
tauri-app/src/lib/services/pickConfig.ts
• Function:pickDeployments
• Replace lines 11–15 with:- pickBy( - mergedConfig.dotrainOrder.deployments, - (d) => - mergedConfig.dotrainOrder.scenarios[d.scenario.key].deployer.network.key === - activeNetworkRef, - ) + pickBy( + mergedConfig.dotrainOrder.deployments, + (d) => + mergedConfig.dotrainOrder.scenarios?.[d.scenario.key]?.deployer?.network?.key === + activeNetworkRef, + )This ensures that if
scenarios[d.scenario.key]
(or its nested fields) isundefined
, the predicate simply evaluates tofalse
instead of throwing.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.return mergedConfig?.dotrainOrder.deployments ? pickBy( mergedConfig.dotrainOrder.deployments, (d) => mergedConfig.dotrainOrder.scenarios?.[d.scenario.key]?.deployer?.network?.key === activeNetworkRef, ) : {};
packages/ui-components/src/__tests__/ListViewOrderbookFilters.test.ts (3)
91-95: 🧹 Nitpick (assertive)
as URL
cast hides an invalid stub – consider a minimalURL
instance instead
Casting an object literal ({ pathname: '/vaults' }
) toURL
satisfies the compiler but breaks type safety; consuming code might call otherURL
properties and fail at runtime. Either create a realURL
(new URL('https://example.com/vaults')
) or use a partial type ({ pathname: '/vaults' } as Pick<URL,'pathname'>
).
8-9:
⚠️ Potential issue
vi.hoisted
should remain synchronous – drop theawait
and the dynamicimport()
vi.hoisted()
executes in a hoisted, synchronous context. Supplying it anasync import()
forces the wrapper to return aPromise
, so addingawait
“works” only because top-levelawait
is enabled, yet it defeats the purpose of hoisting (mock factories must be synchronous and side-effect free). This can cause the mock to be initialised after the test suite starts, leading to brittle or flaky tests.-const { mockPageStore } = await vi.hoisted(() => import('$lib/__mocks__/stores.ts')); +const { mockPageStore } = vi.hoisted(() => { + // use `require` to keep it synchronous (Vitest transpiles this under ESM) + // eslint-disable-next-line @typescript-eslint/no-var-requires + return require('$lib/__mocks__/stores.ts'); +});If you really need the ESM import path, perform a normal top-level
import
outsidevi.hoisted
.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// Before: // const { mockPageStore } = await vi.hoisted(() => import('$lib/__mocks__/stores.ts')); // After: const { mockPageStore } = vi.hoisted(() => { // use `require` to keep it synchronous (Vitest transpiles this under ESM) // eslint-disable-next-line @typescript-eslint/no-var-requires return require('$lib/__mocks__/stores.ts'); });
60-80: 🛠️ Refactor suggestion
Page-store state leaks across tests – reset it in
beforeEach
beforeEach
mutatesmockSettings
but leavesmockPageStore
untouched, while several tests (/vaults
,/orders
) permanently change its value. The last test explicitly callsmockPageStore.reset()
, meaning test outcome depends on execution order. Add a reset insidebeforeEach
(or anafterEach
) to guarantee isolation.beforeEach(() => { + mockPageStore.reset(); // ensure clean URL/path state mockSettings.set({
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.beforeEach(() => { mockPageStore.reset(); // ensure clean URL/path state mockSettings.set({ orderbook: { networks: { ethereum: { key: 'ethereum', rpc: 'https://rpc.ankr.com/eth', chainId: 1, networkId: 1, currency: 'ETH' } }, subgraphs: { mainnet: { key: 'mainnet', url: 'mainnet-url' } } } } as unknown as Config); });
crates/common/src/frontmatter.rs (2)
6-9:
⚠️ Potential issueFunction signature changed but tests were not updated – compilation will fail
parse_frontmatter
is now synchronous and requires the extravalidate: bool
parameter, yet every call in this file’s tests still uses the old async form (parse_frontmatter(...).await
). This will produce:
error[E0277]:
{Result<…, …>}is not a future
andexpected 2 parameters, found 1
.Proposed minimal fix (applies to every test):
- let config = parse_frontmatter(TEST_DOTRAIN.to_string()).await.unwrap(); + let config = parse_frontmatter(TEST_DOTRAIN.to_string(), true).unwrap();You can keep the
#[tokio::test]
async wrapper, but the call itself must be synchronous and supply the validation flag (true/false as appropriate). Consider extracting a helper to avoid repeating the same line in ~20 places.Committable suggestion skipped: line range outside the PR's diff.
7-9: 🧹 Nitpick (assertive)
Micro-nit: avoid intermediate
to_string()
when not needed
RainDocument::get_front_matter
returns&str
; cloning into aString
is unavoidable for the Vec, but you can inline the default and avoid the first borrow:-let frontmatter = RainDocument::get_front_matter(dotrain.as_str()).unwrap_or(""); -Config::try_from_yaml(vec![frontmatter.to_string()], validate) +let frontmatter = RainDocument::get_front_matter(&dotrain).unwrap_or_default(); +Config::try_from_yaml(vec![frontmatter.to_owned()], validate)Minor, but it saves one borrow conversion and clarifies intent.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let frontmatter = RainDocument::get_front_matter(&dotrain).unwrap_or_default(); Config::try_from_yaml(vec![frontmatter.to_owned()], validate) }
crates/js_api/src/gui/mod.rs (1)
65-69: 🧹 Nitpick (assertive)
Default
impl duplicates the zero-cost#[derive(Default)]
you already qualify forBecause every field of
DotrainOrderGui
implementsDefault
, you can simply#[derive(Default)] pub struct DotrainOrderGui { … }and delete the manual
impl Default
.
This avoids future divergence betweennew()
anddefault()
, trims 5 lines, and lets the compiler keep the impl in sync for you.crates/js_api/src/gui/order_operations.rs (4)
296-298: 🧹 Nitpick (assertive)
Double mutable borrow through iterator closure is fragile
Inside
update_bindings
the closure passed tomap
calls
self.get_field_value
, causing nested mutable borrows ofself
. While it
compiles today thanks to non-lexical lifetimes, any future borrow ofself
inside the loop body will trip borrow-checker errors.Gather the data first, then do the update:
let values: HashMap<_, _> = self .field_values .iter() .map(|(k, v)| (k.clone(), v.value.clone())) .collect(); yaml.update_bindings(values)?;
471-490:
⚠️ Potential issue
output_token_infos
ignores input–only deposit tokens
approvals
are derived from deposits, butoutput_token_infos
is built only
from order outputs. If the strategy requires a deposit on an input token
the lookup will fail withTokenNotFound
.Populate the map from the set of deposit tokens instead:
for (token_addr, _) in deposits_map { … }or reuse the earlier
deposits_map
for O(1) access.
199-200: 🛠️ Refactor suggestion
prepare_calldata_generation
performs a second YAML parse each callReturning
self.get_current_deployment()
forces a reparsing of the YAML after
all mutations, but the already-borroweddeployment
could simply be returned
(aftermutate_and_refresh
steps). For hot paths (e.g. repeated
generate_*_calldatas
) this doubles parsing cost.- self.get_current_deployment() + Ok(deployment)If you keep the refresh, add a short comment explaining why the extra parse is
required.Committable suggestion skipped: line range outside the PR's diff.
58-60:
⚠️ Potential issueTypeScript shape of
IOVaultIds
may not match runtime data
#[tsify(type = "Map<string, (string | undefined)[]>")]
advertises strings
while the vector actually holdsOption<U256>
(i.e. big-ints serialised as
hex). In JS this becomes an array of{"hex": …}
objects, not plain strings,
which consumers will fail to parse.Two safer options:
-#[tsify(type = "Map<string, (string | undefined)[]>")] +#[tsify(type = "Map<string, (bigint | undefined)[]>")]or stringify in Rust before exposing.
Unit tests only cover Rust side, so this slip won’t be caught.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.pub struct IOVaultIds( #[tsify(type = "Map<string, (bigint | undefined)[]>")] pub HashMap<String, Vec<Option<U256>>>, );
packages/ui-components/src/__tests__/awaitTransactionIndexing.test.ts (1)
30-37: 🧹 Nitpick (assertive)
Fake timers are restored but not the system clock
vi.useFakeTimers()
replaces both timers and the system clock.
If code under test relies onDate.now()
orperformance.now()
you should
callvi.setSystemTime
or restore the clock inafterEach
.
vi.useRealTimers()
resets timers but leaves the mocked clock unchanged.Add:
afterEach(() => { vi.useRealTimers(); vi.setSystemTime(Date.now()); });packages/ui-components/src/__tests__/transactionStore.test.ts (2)
39-45: 🛠️ Refactor suggestion
TIMEOUT_ERROR
shadowed with a different literalThe mocked module redefines
TIMEOUT_ERROR
with a different string than the
real implementation. Code comparing error identity rather than merely
presence will behave differently in test vs production, hiding bugs.Prefer:
import { TIMEOUT_ERROR as REAL_TIMEOUT } from '$lib/services/awaitTransactionIndexing'; vi.mock('../lib/services/awaitTransactionIndexing', () => ({ ...actual, awaitSubgraphIndexing: vi.fn(), }));and keep the constant identical.
594-645: 🧹 Nitpick (assertive)
Promise race condition in deployment test
await new Promise(resolve => setTimeout(resolve, 0));
relies on the micro-task
queue to let the store update. When Jest switches to real timers or the
execution environment differs, this may become flaky.Use
await vi.nextTick()
(Vitest helper) or expose anonStatusChange
subscription in the store and wait for the specific status instead of a bare
delay.crates/settings/src/remote/chains/chainid.rs (2)
60-66: 🧹 Nitpick (assertive)
Minor: unify error naming with the new config error scheme
You renamed many
*ConfigSourceError
enums to*CfgError
across the repo; consider following that convention here (ChainIdError
→ChainIdCfgError
) to keep naming predictable.
93-151:
⚠️ Potential issueRemoved type is still referenced – tests will no longer compile
NetworkConfigSource
was deleted in this PR, yet the whole test-suite (tests
module) still instantiates and asserts against that type (NetworkConfigSource::try_from(...)
etc.).
The crate will fail to compile because the symbol is now missing.Suggested fix – drop the obsolete assertions or migrate them to the surviving
NetworkCfg
API:- let network_cfg_source = NetworkConfigSource::try_from(chain_id.clone()); - assert_eq!(network_cfg_source, Err(ChainIdError::NoRpc)); + // removed NetworkConfigSource – only validate the remaining conversion + let network_cfg = chain_id.clone() + .try_into_network_cfg(Arc::new(RwLock::new(StrictYaml::String(String::new())))); + assert_eq!(network_cfg, Err(ChainIdError::NoRpc));Apply the same clean-up in the other two tests that still mention
NetworkConfigSource
.Without this change
cargo test
will fail at type-checking.Committable suggestion skipped: line range outside the PR's diff.
packages/ui-components/src/lib/components/ListViewOrderbookFilters.svelte (2)
35-35: 🧹 Nitpick (assertive)
Guard against undefined
$settings
beforeisEmpty
lodash.isEmpty(undefined)
returnstrue
, but calling it each render when$settings
is stillundefined
may hide real “loading” states.
Consider an early guard:{#if !$settings} <!-- settings still loading --> {:else if isEmpty($settings.orderbook?.networks)}
25-27: 🧹 Nitpick (assertive)
Introduce explicit declarations for reactive vars
$: isVaultsPage = ...
implicitly creates the variables, but adding an explicitlet
improves readability and avoids ESLint/TS lint errors in some setups.-$: isVaultsPage = $page.url.pathname === '/vaults'; -$: isOrdersPage = $page.url.pathname === '/orders'; +let isVaultsPage: boolean; +let isOrdersPage: boolean; + +$: isVaultsPage = $page.url.pathname === '/vaults'; +$: isOrdersPage = $page.url.pathname === '/orders';crates/js_api/src/gui/select_tokens.rs (1)
139-166: 🧹 Nitpick (assertive)
Helper methods mutate YAML through shared reference
add_record_to_yaml
/remove_record_from_yaml
take&self
but perform interior mutation on the shared YAML value.
That is safe thanks to the internalRwLock
, but please document this explicitly with a short comment so future refactors don’t downgrade the lock guarantees./// Note: safe interior mutation – YAML documents are protected by `RwLock`.
crates/js_api/src/gui/state_management.rs (2)
298-323: 🧹 Nitpick (assertive)
Hard-coding a base-64 blob is brittle
The snapshot string will change on any (even benign) modification to the
serialization format, gzip level, or the dotrain template, causing noisy test
failures. Consider:
- Verifying round-trip behaviour (
deserialize_state(serialize_state())
) instead of matching an exact constant.- Storing the snapshot under
tests/fixtures/*.bin
and regenerating it with a helper if the algorithm legitimately changes.This keeps the test purposeful while lowering maintenance burden.
2-2: 🧹 Nitpick (assertive)
Unused
eval
andReflect
imports increase binary size
eval
andReflect
are only referenced inside the test-module, yet they are imported unconditionally.
On non-test builds this produces an “unused import” warning and slightly bloats the Wasm payload.-use js_sys::{eval, Reflect}; +#[cfg(test)] +use js_sys::{eval, Reflect};(The same
cfg(test)
gate can be applied toJsValue
if it is also test-only.)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.#[cfg(test)] use js_sys::{eval, Reflect};
packages/ui-components/src/lib/storesGeneric/cachedWritableStore.ts (3)
66-83: 🧹 Nitpick (assertive)
cachedWritableInt
is defined but not re-exportedDown-stream code cannot import
cachedWritableInt
unless
packages/ui-components/src/lib/index.ts
re-exports it (only the optional
variants are exported at the moment). Either export it alongside the others
or drop the helper if unused to avoid dead code.
113-122: 🧹 Nitpick (assertive)
cachedWritableIntOptional
silently falls back to0
When the cached value is unparsable and
defaultValue
isundefined
, the
helper returns0
. Returningundefined
would be more consistent with the
type signature and avoids accidentally treating “no value” as a literal zero.- return isNaN(parsed) ? (defaultValue ?? 0) : parsed; + return isNaN(parsed) ? defaultValue : parsed;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const cachedWritableIntOptional = (key: string, defaultValue = undefined) => cachedWritableOptionalStore<number>( key, defaultValue, (v) => v.toString(), (v) => { const parsed = Number.parseInt(v); return isNaN(parsed) ? defaultValue : parsed; } );
36-55: 🧹 Nitpick (assertive)
Guard access to
localStorage
in non-browser environmentsReferencing the global
localStorage
object when it is undefined (SSR, unit
tests, Web Workers) raises aReferenceError
before enteringtry { … }
.
A simpletypeof
guard prevents the throw:-const getCache = () => { +const getCache = () => { try { - const cached = localStorage.getItem(key); + if (typeof localStorage === 'undefined') return defaultValue; + const cached = localStorage.getItem(key); …Apply the same check in
setCache
.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const getCache = () => { try { if (typeof localStorage === 'undefined') return defaultValue; const cached = localStorage.getItem(key); return cached !== null ? deserialize(cached) : defaultValue; } catch { return defaultValue; } }; const setCache = (value?: T) => { try { if (value !== undefined) { localStorage.setItem(key, serialize(value)); } else { localStorage.removeItem(key); } } catch { // Silently ignore localStorage errors to allow the application to function // without persistence in environments where localStorage is unavailable } };
packages/webapp/src/routes/+layout.ts (1)
12-17: 🛠️ Refactor suggestion
Hard-coded “blob” URL is not fetchable & lacks error-handling
fetch('https://github.com/.../blob/…/settings.yaml')
downloads HTML, not the raw YAML – the call only works in tests because it is mocked.
Additionally, a non-200 response will still be parsed, polluting thesettings
store with invalid data.- const response = await fetch( - 'https://github.com/rainlanguage/rain.strategies/blob/246947a82cbc12a9cb6bbfa07b4743d0ec5e5ff2/settings.yaml' - ); - const settingsYaml = await response.text(); + const response = await fetch( + import.meta.env.PUBLIC_SETTINGS_URL ?? + 'https://raw.githubusercontent.com/rainlanguage/rain.strategies/246947a82cbc12a9cb6bbfa07b4743d0ec5e5ff2/settings.yaml', + ); + if (!response.ok) { + throw new Error(`Failed to download settings.yaml – ${response.status} ${response.statusText}`); + } + const settingsYaml = await response.text();• Uses the raw-content endpoint (or a configurable env var).
• Guards against 4xx/5xx so downstream parsing never receives HTML.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const response = await fetch( import.meta.env.PUBLIC_SETTINGS_URL ?? 'https://raw.githubusercontent.com/rainlanguage/rain.strategies/246947a82cbc12a9cb6bbfa07b4743d0ec5e5ff2/settings.yaml', ); if (!response.ok) { throw new Error(`Failed to download settings.yaml – ${response.status} ${response.statusText}`); } const settingsYaml = await response.text(); const settings = writable<OrderbookConfig>(parseYaml([settingsYaml]).orderbook);
packages/ui-components/src/lib/services/awaitTransactionIndexing.ts (1)
118-123: 🛠️ Refactor suggestion
setInterval
causes overlapping polls – switch to recursivesetTimeout
Because
setInterval
fires regardless of theawait
duration, a slow
subgraph can trigger concurrentfetchData
calls, wasting bandwidth and
risking rate-limits. A simple recursive timer keeps at most one in flight.-const checkInterval = setInterval(async () => { +const poll = async () => { attempts++; try { const data = await fetchData(subgraphUrl, txHash); @@ } - if (attempts >= maxAttempts) { - clearInterval(checkInterval); + if (attempts >= maxAttempts) { resolve({ error: TIMEOUT_ERROR }); return; } - }, interval); + setTimeout(poll, interval); + }; + poll();This guarantees sequential polling and lets the promise settle cleanly.
tauri-app/src/lib/stores/settings.ts (1)
15-31: 🧹 Nitpick (assertive)
EMPTY_SETTINGS
omits optional fields – leads to repetitiveundefined
guards
enableSentry
and several downstream checks repeatedly test
$settings.orderbook.sentry
,networks !== undefined
, etc.
PopulatingEMPTY_SETTINGS
with fully initialised (but empty) sub-objects
lets you drop many of these guards and simplifies derived stores.orderbook: { - networks: {}, - subgraphs: {}, - metaboards: {}, - orderbooks: {}, - accounts: {}, - tokens: {}, - deployers: {}, + networks: {}, + subgraphs: {}, + metaboards: {}, + orderbooks: {}, + accounts: {}, + tokens: {}, + deployers: {}, + sentry: true, // default values prevent optional-chaining noise },(Apply the same idea to
dotrainOrder
.)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const EMPTY_SETTINGS: Config = { orderbook: { networks: {}, subgraphs: {}, metaboards: {}, orderbooks: {}, accounts: {}, tokens: {}, deployers: {}, sentry: true, // default values prevent optional-chaining noise }, dotrainOrder: { orders: {}, scenarios: {}, charts: {}, deployments: {}, }, };
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
crates/common/src/frontmatter.rs (1)
385-389
:⚠️ Potential issueEntire test module duplicated – will cause symbol re-definition errors
A second
mod tests { … }
starting here re-introduces all the same test names.
The compiler will emit “function already defined” errors and the crate will not build.Delete the duplicated block or split the additional cases into a different module name.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
crates/common/src/frontmatter.rs
(1 hunks)tauri-app/src/lib/stores/settings.ts
(12 hunks)
🧰 Additional context used
🧠 Learnings (1)
tauri-app/src/lib/stores/settings.ts (2)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1749
File: tauri-app/src/lib/test/settings.test.ts:8-111
Timestamp: 2025-05-07T06:58:32.478Z
Learning: In the rain.orderbook project, stores like `activeAccountsItems` and `activeSubgraphs` are derived stores that should not be directly updated, as their values are derived from the `settings` store.
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1700
File: tauri-app/src/lib/mocks/mockConfigSource.ts:17-20
Timestamp: 2025-04-28T10:58:15.675Z
Learning: In mock configuration files for testing purposes, such as mockConfigSource.ts, inconsistencies between referenced subgraphs and defined subgraphs may be intentional and acceptable for specific test scenarios.
🧬 Code Graph Analysis (1)
tauri-app/src/lib/stores/settings.ts (4)
packages/ui-components/src/lib/index.ts (3)
cachedWritableStore
(120-120)cachedWritableStringOptional
(122-122)mockConfig
(140-140)packages/ui-components/src/lib/storesGeneric/cachedWritableStore.ts (2)
cachedWritableStore
(30-64)cachedWritableStringOptional
(131-137)packages/ui-components/src/lib/__mocks__/settings.ts (1)
mockConfig
(4-68)tauri-app/src/lib/__tests__/matchers.ts (1)
expect
(5-5)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: Deploy-Preview
- GitHub Check: git-clean
- GitHub Check: test
🔇 Additional comments (1)
tauri-app/src/lib/stores/settings.ts (1)
51-55
:❓ Verification inconclusive
Confirm
parseYaml
signature – validation flag appears to be omitted
parseYaml([str])
is called with a single argument, whereas the Rust counterpart now takes a secondvalidate: bool
parameter. If the JS/TS implementation mirrors this change it should be:return parseYaml([str], /* validate */ true);Please verify the exported signature of
@rainlanguage/orderbook/parseYaml
:If a second parameter exists, update all call-sites to match.
🏁 Script executed:
#!/bin/bash # Locate the TypeScript declaration of parseYaml fd parseYaml node_modules/@rainlanguage/orderbook | xargs -r grep -n "function parseYaml"Length of output: 215
I’ll locate the import in your code and then search the installed package for its definition:
🏁 Script executed:
#!/bin/bash # 1) Find where you import and call parseYaml in settings.ts rg "parseYaml" -n tauri-app/src/lib/stores/settings.ts # 2) Search for any declaration or implementation of parseYaml in node_modules rg "parseYaml" -n node_modulesLength of output: 187
Manual verification required: confirm
parseYaml
signature change
Our automated search didn’t locate the TS declaration forparseYaml
(likely becausenode_modules
isn’t present). Please check the exported signature in@rainlanguage/orderbook
—does it now accept a secondvalidate: boolean
argument?• In tauri-app/src/lib/stores/settings.ts (line 53):
return parseYaml([str]/*, /* validate */ true*/);If the signature is now
parseYaml(input: string[], validate: boolean)
, update this call to:return parseYaml([str], true);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
tauri-app/src/lib/stores/settings.ts (1)
160-170
: 🧹 Nitpick (assertive)
async
keyword is unusedThe subscription callback performs no
await
; keepingasync
adds needless micro-tasks and confuses readers.-settings.subscribe(async () => { +settings.subscribe(() => {Repeat for
activeNetworkOrderbooks.subscribe
(line 228).
♻️ Duplicate comments (3)
packages/webapp/src/routes/+layout.ts (2)
12-15
: 🧹 Nitpick (assertive)🛠️ Refactor suggestion
Avoid hard-coding the remote settings URL
The URL is frozen to a specific commit SHA.
Operations cannot patch the YAML (or redirect to another location) without redeploying the webapp. Expose the URL via an environment variable or a config file instead.-const response = await fetch( - 'https://github.com/rainlanguage/rain.strategies/blob/246947a82cbc12a9cb6bbfa07b4743d0ec5e5ff2/settings.yaml' -); +const response = await fetch(import.meta.env.PUBLIC_SETTINGS_URL);
198-201
:⚠️ Potential issue
parseYaml
is not a mock – tests will throw
vi.mocked(parseYaml).mockReturnValue(...)
only works ifparseYaml
is already a mock function.
As written, it’s the real implementation, so.mockReturnValue
isundefined → TypeError
.-vi.mocked(parseYaml).mockReturnValue(mockConfig as unknown as Config); +vi.mock('@rainlanguage/orderbook', () => ({ + parseYaml: vi.fn().mockReturnValue(mockConfig as Config), +}));Apply the same fix wherever the pattern repeats (e.g. lines 236-239, 253-256, 273-276…).
tauri-app/src/lib/stores/settings.ts (1)
88-94
: Same unchecked cast as in the webappReplace the
as Record<string, OrderbookCfg>
with a typed predicate to retain compile-time safety (see previous comment in +layout.ts).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
packages/webapp/src/routes/+layout.ts
(14 hunks)tauri-app/src/lib/stores/settings.ts
(12 hunks)
🧰 Additional context used
🧠 Learnings (1)
tauri-app/src/lib/stores/settings.ts (2)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1749
File: tauri-app/src/lib/test/settings.test.ts:8-111
Timestamp: 2025-05-07T06:58:32.478Z
Learning: In the rain.orderbook project, stores like `activeAccountsItems` and `activeSubgraphs` are derived stores that should not be directly updated, as their values are derived from the `settings` store.
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1700
File: tauri-app/src/lib/mocks/mockConfigSource.ts:17-20
Timestamp: 2025-04-28T10:58:15.675Z
Learning: In mock configuration files for testing purposes, such as mockConfigSource.ts, inconsistencies between referenced subgraphs and defined subgraphs may be intentional and acceptable for specific test scenarios.
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Deploy-Preview
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: git-clean
🔇 Additional comments (2)
tauri-app/src/lib/stores/settings.ts (2)
191-199
: 🧹 Nitpick (assertive)Extra
.map
is a no-op afterfilter
Object.entries(...).filter(...).map(([k,v])=>[k,v])
just reconstructs the same tuples.- const updatedActiveAccounts = Object.fromEntries( - Object.entries($settings.orderbook.accounts ?? {}) - .filter(([key, value]) => { - if (key in currentActiveAccounts) { - return currentActiveAccounts[key] === value.address; - } - return false; - }) - .map(([key, value]) => [key, value.address]), - ); + const updatedActiveAccounts = Object.fromEntries( + Object.entries($settings.orderbook.accounts ?? {}).filter( + ([k, v]) => currentActiveAccounts[k] === v.address, + ).map(([k, v]) => [k, v.address]), + );Likely an incorrect or invalid review comment.
1-1
:❌ Incorrect review comment
Inconsistent store library – use
svelte/store
for parityThe rest of the codebase (e.g. webapp) imports stores from
svelte/store
.
Pulling@square/svelte-store
here fragments the dependency graph and increases bundle size unless you have a strong reason to fork the API.-import { derived, get } from '@square/svelte-store'; +import { derived, get } from 'svelte/store';
🏁 Script executed:
#!/bin/bash # Find all occurrences of '@square/svelte-store' imports across the repo rg -n "from ['\"]@square/svelte-store['\"]" # Find all occurrences of 'svelte/store' imports across the repo rg -n "from ['\"]svelte/store['\"]"Length of output: 9498
🌐 Web query:
What is the npm package @square/svelte-store and how does it differ from svelte/store?
💡 Result:
What is
@square/svelte-store
?The
@square/svelte-store
npm package is an extension of Svelte's officialsvelte/store
package, designed and maintained by Square. It builds upon the default store functionality in Svelte by introducing enhanced capabilities for managing asynchronous data in a reactive way. The package is intended as a drop-in replacement forsvelte/store
, including all of its features, while also adding new types of stores and extended functionality for better handling of async workflows[1][6].At its core,
@square/svelte-store
introduces the concept of "Loadable" stores. These stores behave like standard Svelte stores—you can subscribe, derive, and use the$
accessor—but they also include aload
method. This method is asynchronous and resolves to the store's value after any required async behavior is completed. This enables more granular control over loading states and async data management in Svelte apps, allowing you to easily display loading states and automatically sequence async data dependencies[1][6].Key Features of
@square/svelte-store
- All Features of
svelte/store
: Fully compatible with standard Svelte stores—readable, writable, derived, etc.- Loadable Stores: New store type with a
.load()
async method for handling data-fetching and async tasks.- Async Workflow Support: Loadable stores simplify complex data flows that involve async operations, making them seamless and reactive within Svelte components.
- Data Dependency Management: Derived loadable stores automatically load their dependencies before resolving themselves[1][6].
How Does It Differ from
svelte/store
?Here is a comparison between the two:
Feature svelte/store
@square/svelte-store
Basic store types (writable, readable) Yes Yes Reactive subscriptions & $
syntaxYes Yes Derived stores Yes Yes Async data management (loadable stores) No Yes (with .load()
method on stores)Built-in async state control & sequencing No Yes Drop-in replacement compatibility — Yes (can be imported instead of svelte/store
)Handles async dependencies in derived data No Yes Key Differences:
svelte/store
provides a minimal, synchronous, and highly reactive interface for state management in Svelte, but leaves all async logic (such as loading data from a server) up to the developer to manage outside the store[5][8][9].@square/svelte-store
directly integrates async support by allowing stores themselves to manage loading, awaiting, and resolving of async data, simplifying the code required to manage async state and making async logic reactive and composable by default[1][6].Example
With a loadable store, you could do:
{#await myLoadableStore.load()} <p>Currently loading...</p> {:then} <p>Your loaded data is: {$myLoadableStore}</p> {/await}Here, the store’s
load
method resolves once async fetching is done, and Svelte reactivity keeps your UI in sync[1][6].Summary
- Use
@square/svelte-store
if your app relies heavily on async data and you want that logic to integrate smoothly with Svelte's reactivity, taking advantage of loadable stores and automatic async dependency handling.- Use
svelte/store
for simple, synchronous state or if you want to manage async flows separately.
@square/svelte-store
is an advanced, backwards-compatible enhancement for state handling in Svelte apps where async data is a first-class concern[1][6].Citations:
- 1: https://www.npmjs.com/package/@square%2Fsvelte-store
- 2: https://github.com/square/svelte-store/issues
- 3: https://developer.squareup.com/blog/accept-payments-with-square-and-svelte/
- 4: https://github.com/square/svelte-store/blob/main/package.json
- 5: https://svelte.dev/docs/svelte-store
- 6: https://www.npmjs.com/package/@square/svelte-store/v/0.2.1
- 7: sveltejs/kit#6098
- 8: https://svelte.dev/docs/svelte/stores
- 9: https://coderpad.io/blog/development/a-guide-to-svelte-stores/
- 10: https://developer.squareup.com/blog/accept-payments-with-square-and-vue/
🏁 Script executed:
#!/bin/bash # Search for usage of the .load() method in the tauri-app code to see if @square/svelte-store's loadable features are in use rg -n "load\(" -g "tauri-app/src/lib/**"Length of output: 377
Keep
@square/svelte-store
for loadable store functionalityThe Tauri app relies on the async
.load()
API provided by@square/svelte-store
, which does not exist in the vanillasvelte/store
. Switching imports would break these features:
- tauri-app/src/lib/stores/order.ts – calls
subgraph.load()
in multiple places- tauri-app/src/lib/stores/forkBlockNumber.ts – calls
rpcUrl.load()
Because these “loadable” stores are core to your async data handling, continue using
@square/svelte-store
. Ignore the suggestion to replace it withsvelte/store
.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
crates/settings/src/remote/chains/chainid.rs (1)
69-90
: 🧹 Nitpick (assertive)Implementation looks good but consider documenting the purpose of the document parameter.
The
try_into_network_cfg
method has a clear implementation for selecting a valid RPC URL and building a NetworkCfg. However, the purpose of thedocument: Arc<RwLock<StrictYaml>>
parameter isn't immediately obvious from the code.impl ChainId { + /// Attempts to convert a ChainId into a NetworkCfg by finding a valid RPC URL. + /// + /// The document parameter contains the parent YAML configuration that owns this network. pub fn try_into_network_cfg( self, document: Arc<RwLock<StrictYaml>>, ) -> Result<NetworkCfg, ChainIdError> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
crates/common/src/frontmatter.rs
(8 hunks)crates/settings/src/remote/chains/chainid.rs
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: Deploy-Preview
- GitHub Check: test
- GitHub Check: test
- GitHub Check: git-clean
🔇 Additional comments (6)
crates/settings/src/remote/chains/chainid.rs (2)
3-3
: Import updated to use NetworkCfg instead of NetworkConfigSource.The import change reflects the removal of ConfigSource-based types, aligning with the PR objective to remove the config source.
97-151
: Test cases properly verify the network configuration conversion.The tests cover the expected error cases (no RPC, unsupported RPC URLs) and the successful path, ensuring the method behaves correctly under various conditions.
crates/common/src/frontmatter.rs (4)
3-3
: Updated import to use Config and ParseConfigError.This change properly reflects the transition from ConfigSource to the new Config structure.
23-25
: Tests updated to use synchronous API and new Config accessor methods.All tests have been properly converted from async to synchronous, and updated to use the new getter methods (
get_networks()
,get_network()
, etc.) instead of direct field access. This is consistent with the new Config structure that uses private fields with public accessor methods.Also applies to: 48-50, 65-67, 82-84, 99-101, 122-124, 139-141, 155-157, 173-175, 186-188, 289-291, 332-334, 341-343, 350-352, 368-373
147-147
: Tests updated to access nested keys correctly.Tests now properly access nested keys through the
.key
or.address
fields, reflecting the new nested config structure. This is particularly evident in scenarios, deployments, tokens, orders, and account references.Also applies to: 151-151, 179-183, 303-303
374-387
: New test added to verify overall configuration counts.A new test has been added to verify the counts of all configuration sections, ensuring the integrity of the entire configuration structure after parsing. This is a good practice to maintain overall test coverage.
Things to test out manually
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
tauri-app/src/routes/orders/add/+page.svelte (1)
126-132
: 🧹 Nitpick (assertive)Consider debouncing
updateMergedConfig
to avoid a network floodReactive statement at line 59 invokes
updateMergedConfig()
on every keystroke in the text area.
Because the function performs I/O and youawait
it, rapid typing spawns overlapping promises and unnecessary parsing work. Debounce or throttle the call (you already haveuseDebouncedFn
in this component) to improve responsiveness and reduce load.
♻️ Duplicate comments (2)
tauri-app/src/lib/stores/settings.ts (2)
210-214
: Deep-compare of subgraph objects viaJSON.stringify
already discussedThe performance/clarity implications of serialising objects on every settings update were flagged in an earlier review and acknowledged.
No further action required here.
88-95
: Unchecked cast onpickBy
previously notedThe cast to
Record<string, OrderbookCfg>
loses type-safety; using a generic predicate preserves it without assertion.
As this was raised and intentionally deferred earlier, marking as duplicate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
tauri-app/src-tauri/Cargo.lock
is excluded by!**/*.lock
📒 Files selected for processing (22)
crates/common/src/fuzz/impls.rs
(1 hunks)crates/js_api/src/lib.rs
(1 hunks)packages/ui-components/src/__tests__/ListViewOrderbookFilters.test.ts
(3 hunks)packages/ui-components/src/__tests__/VaultDetail.test.ts
(1 hunks)packages/ui-components/src/lib/__mocks__/stores.ts
(4 hunks)packages/ui-components/src/lib/components/ListViewOrderbookFilters.svelte
(1 hunks)packages/ui-components/src/lib/components/deployment/DeploymentSteps.svelte
(3 hunks)packages/ui-components/src/lib/components/detail/VaultDetail.svelte
(1 hunks)packages/ui-components/src/lib/components/dropdown/DropdownActiveSubgraphs.svelte
(1 hunks)packages/ui-components/src/lib/components/dropdown/DropdownOrderListAccounts.svelte
(1 hunks)packages/ui-components/src/lib/components/tables/OrdersListTable.svelte
(1 hunks)packages/ui-components/src/lib/components/tables/VaultsListTable.svelte
(4 hunks)packages/ui-components/src/lib/index.ts
(1 hunks)packages/ui-components/src/lib/types/appStores.ts
(1 hunks)packages/webapp/src/routes/vaults/+page.svelte
(2 hunks)tauri-app/src-tauri/src/commands/charts.rs
(1 hunks)tauri-app/src-tauri/src/main.rs
(1 hunks)tauri-app/src/__tests__/pickConfig.test.ts
(2 hunks)tauri-app/src/__tests__/settings.test.ts
(0 hunks)tauri-app/src/lib/stores/settings.ts
(8 hunks)tauri-app/src/routes/orders/add/+page.svelte
(4 hunks)tauri-app/src/routes/vaults/+page.svelte
(2 hunks)
💤 Files with no reviewable changes (1)
- tauri-app/src/tests/settings.test.ts
🧰 Additional context used
🧠 Learnings (3)
crates/js_api/src/lib.rs (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1715
File: crates/js_api/src/common/mod.rs:111-118
Timestamp: 2025-04-30T09:28:36.960Z
Learning: In the rain.orderbook repository, the WASM tests are already properly configured with conditional compilation using `#[cfg(target_family = "wasm")]` and `#[cfg(not(target_family = "wasm"))]`, and don't require additional `wasm_bindgen_test_configure!(run_in_browser)` directives.
packages/ui-components/src/lib/types/appStores.ts (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1749
File: tauri-app/src/lib/test/settings.test.ts:8-111
Timestamp: 2025-05-07T06:58:32.478Z
Learning: In the rain.orderbook project, stores like `activeAccountsItems` and `activeSubgraphs` are derived stores that should not be directly updated, as their values are derived from the `settings` store.
tauri-app/src/lib/stores/settings.ts (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1749
File: tauri-app/src/lib/test/settings.test.ts:8-111
Timestamp: 2025-05-07T06:58:32.478Z
Learning: In the rain.orderbook project, stores like `activeAccountsItems` and `activeSubgraphs` are derived stores that should not be directly updated, as their values are derived from the `settings` store.
🧬 Code Graph Analysis (2)
tauri-app/src-tauri/src/main.rs (1)
tauri-app/src-tauri/src/commands/config.rs (2)
merge_configstrings
(11-23)parse_configstring
(6-8)
packages/ui-components/src/lib/__mocks__/stores.ts (2)
packages/webapp/src/lib/__mocks__/mockWeb3Config.ts (1)
mockWeb3Config
(5-16)packages/ui-components/src/lib/__mocks__/settings.ts (1)
mockSettingsStore
(72-76)
🪛 ESLint
tauri-app/src/lib/stores/settings.ts
[error] 14-14: 'waitFor' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (35)
tauri-app/src-tauri/src/commands/charts.rs (1)
3-3
: Skip: Non-functional import reorder.This change only reorders the
std::collections::HashMap
import and does not affect any logic. No action needed.crates/common/src/fuzz/impls.rs (1)
674-702
: New SETTINGS constant for test fixturesThis new YAML string provides a standardized test fixture with placeholders for subgraphs, metaboards, orderbooks, tokens, orders, and deployments. This is a good addition that aligns with the PR objective of removing the config source and standardizing how configuration is handled across the codebase.
The structured YAML format here appears to match the expected format needed by the new configuration system.
tauri-app/src/routes/vaults/+page.svelte (2)
8-8
: Updated import to use the new subgraph structureChanged from
subgraphUrl
tosubgraph
to reflect the updated configuration structure where subgraph entries are now objects containing a URL property rather than plain strings.
43-43
: Updated prop to match the new subgraph structureChanged the prop name from
subgraphUrl
tosubgraph
to align with the updated settings store structure.tauri-app/src-tauri/src/main.rs (2)
15-15
: Removed the convert_configstring_to_config importThis change aligns with the PR objective of removing the config source. The code now uses
parse_configstring
andmerge_configstrings
which work directly with YAML strings and return aConfig
object without needing an intermediate conversion step.
63-64
:✅ Verification successful
Verify that these functions are still referenced in the invoke_handler
The
parse_configstring
andmerge_configstrings
functions are still included in the invocation handlers list, but the removedconvert_configstring_to_config
function is not present in this list. This suggests that other code changes account for this function's removal.
🏁 Script executed:
#!/bin/bash # Check if there are any remaining references to convert_configstring_to_config in the codebase rg -i "convert_configstring_to_config"Length of output: 38
🏁 Script executed:
#!/bin/bash # Inspect the invocation handler configuration in main.rs rg -n "invoke_handler" -A5 -B5 tauri-app/src-tauri/src/main.rs rg -n "generate_handler" -A5 -B5 tauri-app/src-tauri/src/main.rsLength of output: 831
🏁 Script executed:
#!/bin/bash # Verify that parse_configstring and merge_configstrings are listed in the invoke handler rg -n "parse_configstring" -A2 -B2 tauri-app/src-tauri/src/main.rs rg -n "merge_configstrings" -A2 -B2 tauri-app/src-tauri/src/main.rsLength of output: 1129
Confirmed parse_configstring and merge_configstrings in invoke handler
Both
parse_configstring
andmerge_configstrings
are listed in thetauri::generate_handler!
invocation (main.rs lines 63–64), and there are no remaining references to the removedconvert_configstring_to_config
. The removal is safe.packages/ui-components/src/__tests__/VaultDetail.test.ts (2)
34-37
: Updated test mock structure to match new configuration formatThe
mockSettings
store now correctly reflects the new structure of subgraph entries as objects withkey
andurl
fields instead of simple strings. This is consistent with the changes in the configuration structure where entities are now represented as typed objects rather than plain values.
99-99
:✅ Verification successful
Verify test with new configuration structure
The test expectation still uses the URL directly (
https://example.com
), which suggests that thegetVault
function receives the URL directly rather than the subgraph object. Ensure that the code correctly extracts the URL from the subgraph object before passing it togetVault
.
🏁 Script executed:
#!/bin/bash # Check how getVault is called with the subgraph parameter in the codebase rg "getVault\(.+subgraph" -A 3 -B 3Length of output: 37
🏁 Script executed:
#!/bin/bash # Locate getVault definition rg -n "export function getVault" -C 3 . # Locate all calls to getVault in the UI components rg -n "getVault\(" -C 3 packages/ui-components/srcLength of output: 1058
🏁 Script executed:
#!/bin/bash # Display import and usage in VaultDetail.svelte echo "=== VaultDetail.svelte (lines 1-70) ===" sed -n '1,70p' packages/ui-components/src/lib/components/detail/VaultDetail.svelte echo "" echo "=== Import statements for getVault ===" rg -n "import .*getVault" -C 3 packages/ui-components/src/lib/components/detail/VaultDetail.svelte echo "" echo "=== VaultDetail.test.ts (first 200 lines) ===" sed -n '1,200p' packages/ui-components/src/__tests__/VaultDetail.test.ts echo "" echo "=== All getVault references in test ===" rg -n "getVault" -n packages/ui-components/src/__tests__/VaultDetail.test.ts -C 3Length of output: 10539
No action needed: Test correctly uses extracted URL and vault ID
The
VaultDetail.svelte
component pulls the subgraph URL viaconst subgraphUrl = $settings?.subgraphs?.[network]?.url || '';and calls
getVault(subgraphUrl, id);The test’s expectation
expect(getVault).toHaveBeenCalledWith('https://example.com', '100');matches this behavior. No changes are required.
packages/ui-components/src/lib/components/ListViewOrderbookFilters.svelte (1)
34-34
: Configuration access path updated correctlyThe nested path
$settings?.orderbook?.networks
now properly reflects the restructured configuration model where networks configuration is accessed under theorderbook
property rather than directly on thesettings
object. This change aligns with the broader refactoring of the configuration data model across the codebase.crates/js_api/src/lib.rs (1)
3-4
: New config module addition for WASM bindingsThe new
config
module is properly conditionally compiled for the WASM target family, consistent with the module structure of the project. This addition is part of the objective to remove the legacy config source and introduce a new synchronous YAML parsing capability for configurations.Let's verify that the new module is being used correctly throughout the codebase:
#!/bin/bash # Look for imports or uses of the new config module rg "use.*js_api::config" --type rustpackages/ui-components/src/lib/components/dropdown/DropdownOrderListAccounts.svelte (1)
3-3
: Improved accounts data processingThe component now properly uses the
getAccountsAsOptions
helper function to transform the strongly-typed account configurations into the format required by the dropdown. This change supports the transition from simple string maps to typedAccountCfg
objects as part of the configuration restructuring.Also applies to: 8-8
packages/ui-components/src/lib/components/tables/OrdersListTable.svelte (1)
45-51
: Updated subgraph configuration access path and structureThe changes properly adapt to the new configuration structure where:
- Subgraphs are now accessed via
$settings?.orderbook?.subgraphs
instead of directly from$settings?.subgraphs
- Subgraph entries are now objects with a
url
property rather than simple string valuesThe mapping logic has been correctly updated to extract the URL from the subgraph configuration object.
packages/ui-components/src/lib/components/deployment/DeploymentSteps.svelte (3)
6-6
: Updated import to use the new Config typeThis change aligns with the PR objective of removing ConfigSource in favor of the new Config structure.
46-46
: Updated settings prop type to use ConfigThe prop type has been correctly updated from
Writable<ConfigSource>
toWritable<Config>
to match the new configuration structure.
75-75
: Updated configuration access patternThe subgraph URL access path has been updated to match the new nested config structure, correctly accessing the URL via
$settings.orderbook.subgraphs[value].url
.packages/ui-components/src/lib/components/detail/VaultDetail.svelte (1)
43-43
: Updated subgraph URL accessThe subgraph access has been updated to match the new config structure, correctly accessing the URL property of the subgraph object.
Note that this component still uses optional chaining while the previous component doesn't. This is likely intentional based on how these values are initialized in different contexts.
packages/ui-components/src/lib/index.ts (1)
145-145
: Renamed mock export to match new config structureThe mock export has been properly renamed from
mockConfigSource
tomockConfig
to reflect the new configuration structure being used throughout the application.packages/webapp/src/routes/vaults/+page.svelte (3)
5-5
: Updated imports to remove redundant itemsRemoved
orderHash
andactiveSubgraphs
imports from$lib/stores/settings
since they're now being destructured from$page.data.stores
.
8-20
: Updated store destructuring to include new propertiesThe destructuring has been updated to include
subgraph
,orderHash
,activeNetworkOrderbooks
, andactiveSubgraphs
from$page.data.stores
, which correctly aligns with the new store structure.
52-66
: Updated component props to match new config structureThe
VaultsListTable
component props have been updated to usesubgraph
instead ofsubgraphUrl
and to includeactiveSubgraphs
, correctly matching the component's updated interface.packages/ui-components/src/__tests__/ListViewOrderbookFilters.test.ts (3)
5-5
: Type import updated to reflect new configuration structure.The import change from
ConfigSource
toConfig
aligns with the PR objective of splitting the configuration structure and removing the settings keyword.
29-47
: Mock settings structure updated to use nested configuration.The mock configuration has been restructured to nest networks and subgraphs under the
orderbook
key, matching the newConfig
structure. Network entries now include explicitkey
properties and use camelCase property names.
83-83
: Empty networks test updated for nested structure.The test for empty networks has been correctly updated to reflect the new nested structure with
orderbook
as the parent object for networks and subgraphs.packages/ui-components/src/lib/components/tables/VaultsListTable.svelte (4)
2-2
: Added utility for standardized subgraph argument handling.Using the centralized
getMultiSubgraphArgs
utility function improves code consistency and maintainability across components.
21-21
: Updated prop name and type for configuration refactoring.Renamed
subgraphUrl
tosubgraph
to reflect that it now stores aSubgraphCfg
object rather than just a URL string, matching the updatedAppStoresInterface
definition.
42-42
: Simplified subgraph arguments with utility function.The reactive statement for
multiSubgraphArgs
now uses the utility functiongetMultiSubgraphArgs
, replacing custom transformation logic and standardizing this operation.
74-74
: Updated query enabled condition for new subgraph type.The query enabled condition now checks for the presence of
$subgraph
instead of$subgraphUrl
, matching the renamed prop and updated type.packages/ui-components/src/lib/components/dropdown/DropdownActiveSubgraphs.svelte (3)
3-4
: Updated type imports for new configuration structure.Import statements now use the new
Config
andSubgraphCfg
types from the orderbook package, aligning with the configuration refactoring.
6-6
: Updated settings prop type and access pattern.The
settings
prop type is nowConfig
instead ofConfigSource
, and subgraphs are accessed via the nested pathsettings.orderbook.subgraphs
instead of directly fromsettings.subgraphs
.Also applies to: 9-9
17-23
: Updated event handler for new subgraph data structure.The
handleStatusChange
event handler now works withRecord<string, SubgraphCfg>
instead of simple string records, and accesses subgraphs via the nested path, ensuring type safety.packages/ui-components/src/lib/__mocks__/stores.ts (3)
1-1
: Imports updated to handle two different Config types.Changed from importing
ConfigSource
toConfig
from orderbook package, and renamed the wagmiConfig
import toWagmiConfig
to avoid naming conflicts.Also applies to: 5-5
26-26
: Updated mock store types to match new configuration structure.The mock stores now use the updated types:
Config | undefined
for settings andWagmiConfig
for wagmi configuration, with appropriate type casting.Also applies to: 39-39
45-45
: Updated mock subscription methods with correct parameter types.The
mockSetSubscribeValue
methods now accept the correct parameter types (Config | undefined
andWagmiConfig
), ensuring type safety when mocking store values.Also applies to: 126-126
tauri-app/src/__tests__/pickConfig.test.ts (1)
189-256
: 👍 Tests accurately capture the expected filtering behaviourAssertions for both populated and empty cases are clear and exhaustive.
No issues spotted.packages/ui-components/src/lib/types/appStores.ts (1)
15-18
: Type ofactiveAccounts
remains correct – nice catchSwitching to
AccountCfg
aligns with the new config model.
No further action needed.
let mergedConfig: Config | undefined = undefined; | ||
let openAddOrderModal = false; | ||
|
||
let composedRainlangForScenarios: Map<ScenarioCfg, string> = new Map(); | ||
|
||
$: deployments = mergedConfig?.deployments; | ||
$: deployments = mergedConfig?.dotrainOrder.deployments; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional-chaining missing after dotrainOrder
can crash the page
If mergedConfig
is defined but dotrainOrder
is undefined
(e.g. parsing failed, partial YAML), the expression tries to evaluate .deployments
on undefined
, throwing at runtime.
-$: deployments = mergedConfig?.dotrainOrder.deployments;
+$: deployments = mergedConfig?.dotrainOrder?.deployments;
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
$: deployments = mergedConfig?.dotrainOrder.deployments; | |
$: deployments = mergedConfig?.dotrainOrder?.deployments; |
$: debouncedGenerateRainlangStrings( | ||
$globalDotrainFile.text, | ||
mergedConfig?.dotrainOrder.scenarios, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same null-safety issue when forwarding scenarios
dotrainOrder
can still be undefined
, so protect the chain:
- $: debouncedGenerateRainlangStrings(
- $globalDotrainFile.text,
- mergedConfig?.dotrainOrder.scenarios,
- );
+ $: debouncedGenerateRainlangStrings(
+ $globalDotrainFile.text,
+ mergedConfig?.dotrainOrder?.scenarios,
+ );
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
$: debouncedGenerateRainlangStrings( | |
$globalDotrainFile.text, | |
mergedConfig?.dotrainOrder.scenarios, | |
); | |
$: debouncedGenerateRainlangStrings( | |
$globalDotrainFile.text, | |
mergedConfig?.dotrainOrder?.scenarios, | |
); |
deployer: { | ||
key: 'network1', | ||
address: '0xabcdef', | ||
network: { | ||
key: 'network1', | ||
rpc: 'rpc-url', | ||
chainId: 14, | ||
}, | ||
}, | ||
}, | ||
network1: { | ||
key: 'network1', | ||
address: '0xabcdef', | ||
network: { | ||
bindings: {}, | ||
deployer: { | ||
key: 'network1', | ||
rpc: 'rpc-url', | ||
chainId: 14, | ||
address: '0xabcdef', | ||
network: { | ||
key: 'network1', | ||
rpc: 'rpc-url', | ||
chainId: 14, | ||
}, | ||
}, | ||
}, | ||
'network1.buy': { | ||
key: 'network1.buy', | ||
bindings: {}, | ||
deployer: { | ||
key: 'network1', | ||
address: '0xabcdef', | ||
network: { | ||
key: 'network1', | ||
rpc: 'rpc-url', | ||
chainId: 14, | ||
}, | ||
}, | ||
}, | ||
}, | ||
charts: {}, | ||
deployments: { | ||
sell: { | ||
key: 'sell', | ||
scenario: { | ||
key: 'network1.sell', | ||
bindings: {}, | ||
deployer: { | ||
key: 'network1', | ||
address: '0xabcdef', | ||
network: { | ||
key: 'network1', | ||
rpc: 'rpc-url', | ||
chainId: 14, | ||
}, | ||
}, | ||
}, | ||
order: { | ||
key: 'sell', | ||
inputs: [], | ||
outputs: [], | ||
network: { | ||
key: 'network1', | ||
rpc: 'rpc-url', | ||
chainId: 14, | ||
}, | ||
}, | ||
}, | ||
buy: { | ||
key: 'buy', | ||
scenario: { | ||
key: 'network1.buy', | ||
bindings: {}, | ||
deployer: { | ||
key: 'network1', | ||
address: '0xabcdef', | ||
network: { | ||
key: 'network1', | ||
rpc: 'rpc-url', | ||
chainId: 14, | ||
}, | ||
}, | ||
}, | ||
order: { | ||
key: 'buy', | ||
inputs: [], | ||
outputs: [], | ||
network: { | ||
key: 'network1', | ||
rpc: 'rpc-url', | ||
chainId: 14, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
charts: {}, | ||
deployments: { | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Fixture looks good but can be made immutable for safety
Declaring the big config
object as const
prevents accidental mutation inside tests and gives stricter type-checking for literals:
-const config: Config = {
+const config = {
/* … */
-} as Config;
+} as const satisfies Config;
Not blocking, just a quick robustness win.
Committable suggestion skipped: line range outside the PR's diff.
settings: Writable<Config>; | ||
activeSubgraphs: Writable<Record<string, SubgraphCfg>>; | ||
accounts: Readable<Record<string, AccountCfg>>; | ||
activeAccountsItems: Writable<Record<string, string>>; | ||
showInactiveOrders: Writable<boolean | undefined>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
activeSubgraphs
& activeAccountsItems
should be Readable
, not Writable
Per project convention (see learning note) these stores are derived from settings
, so exposing them as Writable
invites accidental writes that break reactivity.
- activeSubgraphs: Writable<Record<string, SubgraphCfg>>;
- activeAccountsItems: Writable<Record<string, string>>;
+ activeSubgraphs: Readable<Record<string, SubgraphCfg>>;
+ activeAccountsItems: Readable<Record<string, string>>;
Changing only the type annotation is not enough—ensure the actual store implementations use derived
, not writable
.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
settings: Writable<Config>; | |
activeSubgraphs: Writable<Record<string, SubgraphCfg>>; | |
accounts: Readable<Record<string, AccountCfg>>; | |
activeAccountsItems: Writable<Record<string, string>>; | |
showInactiveOrders: Writable<boolean | undefined>; | |
// packages/ui-components/src/lib/types/appStores.ts | |
export interface AppStoresInterface { | |
settings: Writable<Config>; | |
activeSubgraphs: Readable<Record<string, SubgraphCfg>>; | |
accounts: Readable<Record<string, AccountCfg>>; | |
activeAccountsItems: Readable<Record<string, string>>; | |
showInactiveOrders: Writable<boolean | undefined>; | |
} |
import { waitFor } from '@testing-library/svelte'; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused test-only import to keep production bundle lean
waitFor
is never referenced in this module, yet pulling it in from @testing-library/svelte
ships a dev-only dependency to production and fails the ESLint check you already surfaced.
Simply drop the import (or move it inside the import.meta.vitest
block if you plan to use it in the future).
-import { waitFor } from '@testing-library/svelte';
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
import { waitFor } from '@testing-library/svelte'; |
🧰 Tools
🪛 ESLint
[error] 14-14: 'waitFor' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
This is closed as there are too many conflicts and it is easier to create two separate PRs to close tied issues |
Motivation
See issue: #1364
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
[ ] included screenshots (if this involves a front-end change)fix #1364
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores
Documentation