Conversation
WalkthroughThis update implements a major refactor of the configuration system across the codebase. The previous flat Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Store
participant Backend
participant ConfigParser
User->>UI: Initiate config load (YAML)
UI->>Backend: Fetch YAML config file(s)
Backend->>ConfigParser: parseYaml([yamlStrings], validate?)
ConfigParser-->>Backend: Parsed Config (nested structure)
Backend-->>UI: Return Config
UI->>Store: Update settings with Config
Store-->>UI: Provide derived stores (orderbook, dotrainOrder, subgraphs, etc.)
UI->>User: Render UI with new config data
sequenceDiagram
participant User
participant UI
participant Store
participant Backend
participant ConfigParser
User->>UI: Edit settings (YAML)
UI->>Backend: parseYaml([yamlStrings], validate=true)
Backend->>ConfigParser: parseYaml([yamlStrings], validate)
ConfigParser-->>Backend: Validated Config or Error
Backend-->>UI: Return Config or Error
UI->>Store: Update settings or show error
Store-->>UI: Update derived stores
UI->>User: Show updated UI or error message
sequenceDiagram
participant UI
participant Backend
participant ConfigParser
UI->>Backend: parseDotrainAndYaml(dotrain, settings, validate)
Backend->>ConfigParser: parse_frontmatter(dotrain), parseYaml([settings], validate)
ConfigParser-->>Backend: Merged Config object
Backend-->>UI: Return merged Config
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 49
🔭 Outside diff range comments (8)
packages/ui-components/src/__tests__/DeploymentSteps.test.ts (1)
457-462: 🧹 Nitpick (assertive)Same network-key coupling as above
See previous comment – avoid hard-coding
flare.packages/webapp/src/lib/services/resetActiveNetworkRef.ts (1)
18-24: 🧹 Nitpick (assertive)Cache
Object.keys(networks)to avoid redundant work
Object.keys(networks)is computed twice in quick succession.
Store the result in a local variable to keep the code a bit leaner and easier to read.-const networks = $settings.orderbook.networks; - -if (Object.keys(networks).length > 0) { - activeNetworkRef.set(Object.keys(networks)[0]); +const networks = $settings.orderbook.networks; +const networkKeys = Object.keys(networks); + +if (networkKeys.length > 0) { + activeNetworkRef.set(networkKeys[0]); } else { activeNetworkRef.set(undefined); }packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/fullDeployment.test.ts (1)
1-6: 🧹 Nitpick (assertive)Consolidate imports from the same mocked module
The file statically imports
mockConfig&useAccount(line 5) and then immediately performs a hoisted dynamic import of the same module (line 11).
This loads the module twice under Vitest’s module graph and can lead to subtle mock-state duplication.-import { mockConfig, useAccount, type DeploymentArgs } from '@rainlanguage/ui-components'; +import { type DeploymentArgs } from '@rainlanguage/ui-components'; +import { mockConfig, useAccount } from '@rainlanguage/ui-components'; // ← keep only one styleAfter adjusting, remove the redundant entries from the
vi.hoistedimport if they’re no longer necessary.packages/orderbook/test/js_api/common.test.ts (1)
121-140: 🧹 Nitpick (assertive)Redundant test & mis-leading fixture
This new test repeats the error scenario already covered in lines 111-118 (“undefined deployment”) but with a different YAML shape.
Consider merging both into a single table-driven test or delete one of them to keep the suite fast and focused.Minor: the fixture YAML omits the
deploymentsblock entirely, so the error is not “missing deployment field” but “missingdeploymentssection”. Rename accordingly for clarity.tauri-app/src/lib/mocks/mockConfig.ts (1)
1-65: 🛠️ Refactor suggestionMock bypasses type-safety – fill missing fields or use
satisfiesinstead of a double cast
as unknown as Confighides the fact that several mandatory top-level keys (dotrainOrder, possiblytokens, …) are missing.
Prefer keeping the compiler honest:-export const mockConfig: Config = { +export const mockConfig = { orderbook: { /* …existing data… */ }, + dotrainOrder: { orders: {}, scenarios: {}, charts: {}, deployments: {} } -} as unknown as Config; +} satisfies Config;
satisfies(TS 4.9) verifies the shape at compile-time without forcing a cast.packages/ui-components/src/lib/components/dropdown/DropdownActiveSubgraphs.svelte (1)
9-15: 🧹 Nitpick (assertive)Reactive recomputation creates a brand-new object on every render
dropdownOptionsis rebuilt on each reactive run; for large subgraph sets this can churn.
Consider memoising:$: dropdownOptions = Object.keys(settings.orderbook.subgraphs ?? {}).reduce( (acc, k) => (acc[k] = k, acc), {} as Record<string, string> );No functional change, just avoids extra objects in the reducer.
crates/settings/src/gui.rs (1)
100-117: 🧹 Nitpick (assertive)Extra
Arcclone is redundantInside
try_into_guiyou already obtain anArc<TokenCfg>withArc::clone.
Wrapping it once more withtoken.clone()does anotherArcincrement unnecessarily:- Ok(GuiDepositCfg { - token: Some(token.clone()), + Ok(GuiDepositCfg { + token: Some(token), // `token` is already an ArcThis is purely alloc-free but cleans up intent.
tauri-app/src/routes/orders/add/+page.svelte (1)
123-129: 🧹 Nitpick (assertive)Heavy parsing on every keystroke – debounce recommended
updateMergedConfigcallsparseDotrainAndYamlimmediately on each text change (see reactive$globalDotrainFile.text).
YAML + front-matter parsing is fairly expensive and blocks the UI thread in Tauri.Wrap
updateMergedConfigin the sameuseDebouncedFnpattern you already use elsewhere (e.g. authoring meta) to keep typing smooth.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (80)
crates/cli/src/commands/order/add.rs(1 hunks)crates/common/src/dotrain_order/mod.rs(3 hunks)crates/common/src/frontmatter.rs(10 hunks)crates/common/src/unit_tests/mod.rs(3 hunks)crates/js_api/src/common/mod.rs(5 hunks)crates/js_api/src/config/mod.rs(1 hunks)crates/js_api/src/gui/mod.rs(3 hunks)crates/js_api/src/lib.rs(1 hunks)crates/settings/src/chart.rs(1 hunks)crates/settings/src/config.rs(1 hunks)crates/settings/src/config_source.rs(0 hunks)crates/settings/src/deployer.rs(2 hunks)crates/settings/src/deployment.rs(4 hunks)crates/settings/src/gui.rs(12 hunks)crates/settings/src/lib.rs(0 hunks)crates/settings/src/merge.rs(0 hunks)crates/settings/src/network.rs(3 hunks)crates/settings/src/order.rs(13 hunks)crates/settings/src/orderbook.rs(3 hunks)crates/settings/src/remote/chains/chainid.rs(1 hunks)crates/settings/src/remote/chains/mod.rs(0 hunks)crates/settings/src/scenario.rs(10 hunks)crates/settings/src/test.rs(1 hunks)crates/settings/src/token.rs(4 hunks)crates/settings/src/unit_test.rs(3 hunks)crates/settings/src/yaml/dotrain.rs(3 hunks)crates/settings/src/yaml/mod.rs(4 hunks)crates/settings/src/yaml/orderbook.rs(8 hunks)packages/orderbook/test/js_api/common.test.ts(2 hunks)packages/ui-components/src/__tests__/DeploymentSteps.test.ts(4 hunks)packages/ui-components/src/__tests__/DropdownActiveSubgraphs.test.ts(2 hunks)packages/ui-components/src/__tests__/DropdownOrderListAccounts.test.ts(1 hunks)packages/ui-components/src/__tests__/ListViewOrderbookFilters.test.ts(4 hunks)packages/ui-components/src/__tests__/VaultDetail.test.ts(1 hunks)packages/ui-components/src/lib/__fixtures__/settings-12-11-24.json(1 hunks)packages/ui-components/src/lib/__mocks__/settings.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(2 hunks)packages/ui-components/src/lib/components/dropdown/DropdownActiveSubgraphs.svelte(2 hunks)packages/ui-components/src/lib/components/dropdown/DropdownOrderListAccounts.svelte(1 hunks)packages/ui-components/src/lib/components/tables/OrdersListTable.svelte(3 hunks)packages/ui-components/src/lib/components/tables/VaultsListTable.svelte(3 hunks)packages/ui-components/src/lib/index.ts(1 hunks)packages/ui-components/src/lib/types/appStores.ts(1 hunks)packages/ui-components/src/lib/utils/configHelpers.ts(1 hunks)packages/webapp/src/__tests__/resetActiveNetworkRef.test.ts(2 hunks)packages/webapp/src/__tests__/resetActiveOrderbookRef.test.ts(2 hunks)packages/webapp/src/lib/services/resetActiveNetworkRef.ts(1 hunks)packages/webapp/src/lib/stores/settings.ts(2 hunks)packages/webapp/src/routes/+layout.ts(18 hunks)packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/fullDeployment.test.ts(4 hunks)packages/webapp/src/routes/orders/[network]-[orderHash]/+page.svelte(1 hunks)packages/webapp/src/routes/vaults/+page.svelte(0 hunks)packages/webapp/src/routes/vaults/[network]-[id]/+page.svelte(1 hunks)tauri-app/src-tauri/src/commands/config.rs(1 hunks)tauri-app/src-tauri/src/error.rs(2 hunks)tauri-app/src-tauri/src/main.rs(2 hunks)tauri-app/src/__tests__/pickConfig.test.ts(2 hunks)tauri-app/src/__tests__/settings.test.ts(10 hunks)tauri-app/src/lib/components/CodeMirrorConfig.svelte(2 hunks)tauri-app/src/lib/components/ModalExecute.svelte(1 hunks)tauri-app/src/lib/components/ModalExecute.test.ts(2 hunks)tauri-app/src/lib/mocks/mockConfig.ts(1 hunks)tauri-app/src/lib/mocks/mockConfigSource.ts(0 hunks)tauri-app/src/lib/services/applySettings.test.ts(2 hunks)tauri-app/src/lib/services/applySettings.ts(2 hunks)tauri-app/src/lib/services/config.ts(1 hunks)tauri-app/src/lib/services/configCodemirrorProblems.ts(2 hunks)tauri-app/src/lib/services/order.ts(4 hunks)tauri-app/src/lib/services/pickConfig.ts(2 hunks)tauri-app/src/lib/stores/order.ts(2 hunks)tauri-app/src/lib/stores/settings.ts(7 hunks)tauri-app/src/lib/stores/walletconnect.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(5 hunks)tauri-app/src/routes/settings/+page.svelte(3 hunks)tauri-app/src/routes/vaults/+page.svelte(0 hunks)
💤 Files with no reviewable changes (7)
- packages/webapp/src/routes/vaults/+page.svelte
- tauri-app/src/routes/vaults/+page.svelte
- crates/settings/src/lib.rs
- tauri-app/src/lib/mocks/mockConfigSource.ts
- crates/settings/src/remote/chains/mod.rs
- crates/settings/src/merge.rs
- crates/settings/src/config_source.rs
🧰 Additional context used
🧠 Learnings (8)
tauri-app/src/lib/services/order.ts (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1891
File: packages/webapp/src/lib/services/handleAddOrder.ts:94-94
Timestamp: 2025-06-09T11:14:12.358Z
Learning: In the handleAddOrder function in packages/webapp/src/lib/services/handleAddOrder.ts, using an empty string fallback for undefined subgraphUrl is acceptable. When subgraphUrl is missing/empty, transaction indexing cannot be shown to the user, but the deployment functionality still works and users can manually navigate to the order page to view their deployed orders. This graceful degradation approach is preferred over making subgraphUrl required.
packages/webapp/src/lib/services/resetActiveNetworkRef.ts (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1916
File: packages/webapp/src/lib/services/resetActiveNetworkRef.ts:18-20
Timestamp: 2025-06-10T12:08:23.128Z
Learning: In the new NewConfig system that replaced ConfigSource in the rain.orderbook project, the settings store guarantees that $settings.orderbook.networks (and similar nested fields) will always be defined, so null/undefined safety checks are not needed when accessing these fields.
crates/js_api/src/lib.rs (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1744
File: crates/subgraph/src/orderbook_client.rs:44-46
Timestamp: 2025-05-14T05:13:59.713Z
Learning: In the rain.orderbook project, WASM dependencies are intentionally made available in non-WASM targets to facilitate testing of WASM-related functionality, so conditional compilation guards like `#[cfg(target_family = "wasm")]` should not be added to imports or implementations that may be needed for tests.
packages/ui-components/src/lib/index.ts (1)
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1831
File: packages/ui-components/src/__tests__/transactionStore.test.ts:3-5
Timestamp: 2025-05-19T17:16:54.732Z
Learning: In the rain.orderbook project, there is an ongoing migration of transaction handling from the legacy transactionStore to a new TransactionManager. TransactionStatusMessage has been moved to lib/types/transaction.ts, while TransactionErrorMessage is still in transactionStore but will be moved once other transactions are migrated.
tauri-app/src/lib/stores/order.ts (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1891
File: packages/webapp/src/lib/services/handleAddOrder.ts:94-94
Timestamp: 2025-06-09T11:14:12.358Z
Learning: In the handleAddOrder function in packages/webapp/src/lib/services/handleAddOrder.ts, using an empty string fallback for undefined subgraphUrl is acceptable. When subgraphUrl is missing/empty, transaction indexing cannot be shown to the user, but the deployment functionality still works and users can manually navigate to the order page to view their deployed orders. This graceful degradation approach is preferred over making subgraphUrl required.
crates/js_api/src/common/mod.rs (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1744
File: crates/js_api/src/subgraph/order.rs:109-114
Timestamp: 2025-05-09T05:28:22.089Z
Learning: In the rain.orderbook project, SubgraphError contains or converts from OrderbookSubgraphClientError, so using OrderbookSubgraphClientError in error creation followed by a conversion to SubgraphError (via ? operator) is acceptable.
packages/ui-components/src/lib/__fixtures__/settings-12-11-24.json (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1916
File: packages/ui-components/src/lib/__fixtures__/settings-12-11-24.json:182-195
Timestamp: 2025-06-10T12:04:54.107Z
Learning: In test fixture files like `packages/ui-components/src/lib/__fixtures__/settings-12-11-24.json`, network configuration inconsistencies (such as matchain using Polygon's RPC, chainId, and currency while having its own network key) are acceptable since they are used for testing purposes only.
tauri-app/src/lib/stores/settings.ts (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1916
File: packages/webapp/src/lib/services/resetActiveNetworkRef.ts:18-20
Timestamp: 2025-06-10T12:08:23.128Z
Learning: In the new NewConfig system that replaced ConfigSource in the rain.orderbook project, the settings store guarantees that $settings.orderbook.networks (and similar nested fields) will always be defined, so null/undefined safety checks are not needed when accessing these fields.
🧬 Code Graph Analysis (18)
tauri-app/src/lib/services/order.ts (1)
tauri-app/src/lib/stores/settings.ts (1)
subgraph(108-112)
tauri-app/src-tauri/src/main.rs (3)
tauri-app/src/__tests__/pickConfig.test.ts (1)
config(6-188)tauri-app/src-tauri/src/commands/config.rs (2)
parse_dotrain_and_yaml(11-23)parse_yaml(6-8)crates/js_api/src/config/mod.rs (1)
parse_yaml(5-10)
tauri-app/src/lib/mocks/mockConfig.ts (1)
packages/ui-components/src/lib/__mocks__/settings.ts (1)
mockConfig(4-68)
packages/ui-components/src/__tests__/DropdownOrderListAccounts.test.ts (1)
tauri-app/src/lib/stores/settings.ts (2)
accounts(125-125)activeAccountsItems(126-137)
tauri-app/src/lib/components/ModalExecute.test.ts (2)
packages/webapp/src/lib/stores/settings.ts (1)
settings(30-41)tauri-app/src/lib/stores/settings.ts (2)
settings(46-62)EMPTY_SETTINGS(15-32)
tauri-app/src/lib/stores/order.ts (2)
tauri-app/src/lib/stores/settings.ts (1)
subgraph(108-112)crates/subgraph/src/orderbook_client/mod.rs (1)
url(77-79)
packages/ui-components/src/__tests__/DropdownActiveSubgraphs.test.ts (1)
packages/ui-components/src/lib/__mocks__/settings.ts (1)
mockConfig(4-68)
packages/ui-components/src/lib/utils/configHelpers.ts (2)
tauri-app/src/lib/stores/settings.ts (2)
subgraphs(149-151)accounts(125-125)tauri-app/src/lib/__tests__/matchers.ts (1)
expect(5-5)
packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/fullDeployment.test.ts (1)
packages/ui-components/src/lib/__mocks__/settings.ts (1)
mockConfig(4-68)
packages/webapp/src/__tests__/resetActiveOrderbookRef.test.ts (1)
packages/webapp/src/lib/services/resetActiveOrderbookRef.ts (1)
resetActiveOrderbookRef(12-30)
crates/common/src/unit_tests/mod.rs (1)
crates/common/src/frontmatter.rs (1)
parse_frontmatter(5-11)
crates/cli/src/commands/order/add.rs (2)
tauri-app/src/__tests__/pickConfig.test.ts (1)
config(6-188)crates/common/src/frontmatter.rs (1)
parse_frontmatter(5-11)
crates/settings/src/chart.rs (2)
crates/settings/src/yaml/mod.rs (1)
get_document(449-452)crates/settings/src/orderbook.rs (1)
get_document(214-217)
crates/settings/src/deployer.rs (5)
crates/settings/src/orderbook.rs (2)
validate_address(37-39)to_readable_msg(197-206)crates/settings/src/token.rs (3)
validate_address(39-41)value(43-44)to_readable_msg(364-375)crates/settings/src/config.rs (1)
to_readable_msg(160-236)crates/settings/src/yaml/mod.rs (1)
to_readable_msg(238-317)crates/settings/src/deployment.rs (1)
to_readable_msg(176-185)
tauri-app/src/lib/services/pickConfig.ts (1)
tauri-app/src/lib/stores/settings.ts (1)
activeNetworkRef(68-68)
crates/settings/src/network.rs (1)
crates/settings/src/yaml/mod.rs (1)
to_readable_msg(238-317)
packages/ui-components/src/lib/__mocks__/stores.ts (1)
packages/ui-components/src/lib/__mocks__/settings.ts (1)
mockSettingsStore(72-76)
crates/settings/src/yaml/dotrain.rs (3)
tauri-app/src-tauri/src/commands/order.rs (1)
orders(23-26)crates/settings/src/scenario.rs (1)
key(221-221)crates/settings/src/gui.rs (1)
deployments(89-163)
🪛 Biome (1.9.4)
packages/webapp/src/routes/+layout.ts
[error] 26-26: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
tauri-app/src/__tests__/pickConfig.test.ts
[error] 5-188: Do not export from a test file.
(lint/suspicious/noExportsInTest)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: test
- 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, ob-rs-test, true)
- GitHub Check: git-clean
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- 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-test)
- GitHub Check: Deploy-Preview
- GitHub Check: test
🔇 Additional comments (56)
tauri-app/src/lib/components/CodeMirrorConfig.svelte (1)
5-6: Change aligns with new parsing API – looks goodImport & callback were updated to the new
parseConfigProblemshelper. No issues spotted and the async diagnostic contract is preserved.Also applies to: 16-17
packages/ui-components/src/lib/components/ListViewOrderbookFilters.svelte (1)
34-35:⚠️ Potential issuePotential runtime crash when
orderbookis undefined
$settings.orderbookis dereferenced without a safety-check.
If the store ever emitsundefinedor an object missing theorderbookkey (e.g. during initial load), Svelte will throwCannot read properties of undefined (reading 'networks').Proposed fix:
- {#if isEmpty($settings.orderbook.networks)} + {#if isEmpty($settings?.orderbook?.networks)}Apply the same optional-chaining wherever the block relies on
orderbook.⛔ Skipped due to learnings
Learnt from: findolor PR: rainlanguage/rain.orderbook#1916 File: packages/webapp/src/lib/services/resetActiveNetworkRef.ts:18-20 Timestamp: 2025-06-10T12:08:23.128Z Learning: In the new NewConfig system that replaced ConfigSource in the rain.orderbook project, the settings store guarantees that $settings.orderbook.networks (and similar nested fields) will always be defined, so null/undefined safety checks are not needed when accessing these fields.tauri-app/src/lib/components/ModalExecute.svelte (1)
35-37:⚠️ Potential issueMissing optional-chaining can break wallet modal
$settings?.orderbook.networksstill omits a null-guard onorderbook.
During the very first render$settingsmay be an empty object, producing an exception.- const existingNetwork = Object.entries($settings?.orderbook.networks || {}).find( + const existingNetwork = Object.entries($settings?.orderbook?.networks || {}).find(Same guard should be used anywhere else you access nested config keys in this component.
⛔ Skipped due to learnings
Learnt from: findolor PR: rainlanguage/rain.orderbook#1916 File: packages/webapp/src/lib/services/resetActiveNetworkRef.ts:18-20 Timestamp: 2025-06-10T12:08:23.128Z Learning: In the new NewConfig system that replaced ConfigSource in the rain.orderbook project, the settings store guarantees that $settings.orderbook.networks (and similar nested fields) will always be defined, so null/undefined safety checks are not needed when accessing these fields.packages/ui-components/src/__tests__/VaultDetail.test.ts (1)
44-52: Mocks updated to new nested shape – nice and tidyThe test fixture now mirrors the
orderbook.subgraphsnesting introduced in the refactor. Good attention to consistency.packages/ui-components/src/lib/index.ts (1)
155-158: Export rename looks good
mockConfigSourcewas removed; exportingmockConfigkeeps downstream tests
working and aligns with the newConfigmodel. 👍tauri-app/src/routes/settings/+page.svelte (1)
39-40: LGTM – new parser wiring looks correct
parseYamlis injected intoapplySettingsin the same way the old parser was, so the behavioural contract is maintained. No issues spotted.packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/fullDeployment.test.ts (1)
213-215: Assertion updates accurately reflect the new nested config structureChanging the
subgraphUrlexpectation tomockConfig.orderbook.subgraphs.flare.urlkeeps the tests aligned with the refactored config. ✔️Also applies to: 358-360, 492-494
crates/js_api/src/gui/mod.rs (1)
318-319: Rename propagated correctly
ParseGuiConfigSourceError→ParseGuiConfigErrorwas updated in the error enum as well as theto_readable_msgarm, maintaining consistency. No further action needed.packages/webapp/src/routes/orders/[network]-[orderHash]/+page.svelte (1)
18-23:⚠️ Potential issueGuard against missing
orderbooknamespace
$settings.orderbookis accessed directly. If the store has not yet been populated (e.g. first render while settings load asynchronously) this will throw and break hydration.
Use optional-chaining all the way down (or provide sane defaults) to avoid a hard crash:-const orderbookAddress = $settings.orderbook.orderbooks[network]?.address || ''; -const subgraphUrl = $settings.orderbook.subgraphs[network]?.url || ''; -const rpcUrl = $settings.orderbook.networks[network]?.rpc || ''; -const chainId = $settings.orderbook.networks[network]?.chainId || 0; +const orderbookAddress = $settings.orderbook?.orderbooks?.[network]?.address ?? ''; +const subgraphUrl = $settings.orderbook?.subgraphs?.[network]?.url ?? ''; +const rpcUrl = $settings.orderbook?.networks?.[network]?.rpc ?? ''; +const chainId = $settings.orderbook?.networks?.[network]?.chainId ?? 0;⛔ Skipped due to learnings
Learnt from: findolor PR: rainlanguage/rain.orderbook#1916 File: packages/webapp/src/lib/services/resetActiveNetworkRef.ts:18-20 Timestamp: 2025-06-10T12:08:23.128Z Learning: In the new NewConfig system that replaced ConfigSource in the rain.orderbook project, the settings store guarantees that $settings.orderbook.networks (and similar nested fields) will always be defined, so null/undefined safety checks are not needed when accessing these fields.packages/ui-components/src/lib/components/tables/VaultsListTable.svelte (1)
15-18: 🧹 Nitpick (assertive)Import grouping nit
getVaults/SgVaultWithSubgraphNamecome from the same package as earlier imports; grouping them together improves readability and avoids duplicate lines.-import { getVaults, type SgVaultWithSubgraphName } from '@rainlanguage/orderbook'; +import { getVaults, type SgVaultWithSubgraphName } from '@rainlanguage/orderbook';Likely an incorrect or invalid review comment.
packages/ui-components/src/__tests__/DropdownOrderListAccounts.test.ts (1)
13-24:Details
❌ Incorrect review comment
AccountCfgfields appear to be mis-used
AccountCfgis expected to hold a blockchain address (e.g.0x…) and usually a label (or similar) for display.
In the test fixture theaddressfield is populated with"Label X"– a human-readable label – while the actual address is missing.
This will compile (becausestringsatisfies the type) but it defeats the purpose of the field and can hide bugs in production code that relies on the real address value.- address1: { - key: 'address1', - address: 'Label 1' + address1: { + key: 'address1', + label: 'Label 1', // display label + address: '0x0000000000000000000000000000000000000001' // actual address },Repeat for the other test accounts.
Update the assertions (
activeAccountsItems) if they depend on the previous structure.
Failing to align the fixture with the real shape ofAccountCfgmay lead to false-positive test results.
🏁 Script executed:
#!/bin/bash # Show the imports and fixture context in the test file head -n 20 packages/ui-components/src/__tests__/DropdownOrderListAccounts.test.ts # Locate the definition of AccountCfg rg -n 'export (interface|type) AccountCfg' -A3 -B3 # Find all occurrences of AccountCfg in the codebase for additional context rg -n 'AccountCfg' -A2 -B2Length of output: 12382
Drop the fixture change – the review is incorrect
TheAccountCfgTypeScript definition (imported from@rainlanguage/orderbook) only includeskeyandaddress(astring), with nolabelfield. Using simple placeholder strings like"Label 1"foraddressin tests is a common pattern to verify mapping/display logic without needing real blockchain addresses. Introducing alabelproperty or real 0x… addresses would break the type and isn’t required here.Likely an incorrect or invalid review comment.
tauri-app/src-tauri/src/error.rs (1)
101-104: 👍 UnifiedParseConfigErrorintegration looks correctVariant added and import switched cleanly; serialisation still delegates to
Display.
No further action required.tauri-app/src-tauri/src/main.rs (1)
60-62: Handler list updated – make sure front-end calls are migratedThe old commands (
parse_configstring,merge_configstrings, …) are gone.
Verify that every client (webapp, CLI, CI scripts) now callsparse_yaml/parse_dotrain_and_yaml; otherwise Tauri will return “unknown command”.crates/js_api/src/common/mod.rs (1)
76-88: Avoid double allocation ofdotrainand an unnecessary full clone of the deployment
dotrain.to_string()is performed twice anddeployment_ref.deref().clone()copies the wholeDeploymentCfg.
A small refactor removes both extra allocations:- let config = parse_frontmatter(dotrain.to_string(), None).await?; + let dotrain_owned = dotrain.to_owned(); + let config = parse_frontmatter(dotrain_owned.clone(), None).await?; - let add_order_args = - AddOrderArgs::new_from_deployment(dotrain.to_string(), deployment_ref.deref().clone()) + let add_order_args = + AddOrderArgs::new_from_deployment(dotrain_owned, deployment_ref.clone())
AddOrderArgs::new_from_deploymentalready takes an ownedDeploymentCfg, so passing theArc/Rc(if that’s the inner type) clone directly is cheaper and removes the need forstd::ops::Derefaltogether.
[ suggest_optional_refactor ]tauri-app/src/lib/components/ModalExecute.test.ts (1)
34-35: Nit: reset is already performed inbeforeEach
settings.set(EMPTY_SETTINGS);insidebeforeEachis fine; ensure no test adds another reset later which could hide side-effects.crates/settings/src/remote/chains/chainid.rs (1)
76-88: Re-evaluate exclusion of allws/wssRPC endpointsThe filter
!rpc.scheme().starts_with("ws")discards bothwsandwssURLs; many providers only expose WebSocket endpoints (e.g. Alchemy’s free tier).
If the intention is just to avoid insecurews, consider:!matches!(rpc.scheme(), "ws")or allow
wssexplicitly. Otherwise document the limitation in the error variant.packages/ui-components/src/lib/components/deployment/DeploymentSteps.svelte (1)
46-47: Prop type update looks good, double-check upstream usagesThe prop has been migrated to
Writable<Config>, which aligns with the new config model.
Please grep the UI layer for any lingeringWritable<ConfigSource>references to avoid type-mismatch compilation errors.crates/settings/src/token.rs (3)
39-46: Propagation path fromParseTokenConfigErrortoYamlError
update_address()and several helper functions use the?operator withParseTokenConfigError, expecting an automatic conversion intoYamlError.
Ensureimpl From<ParseTokenConfigError> for YamlErrorexists after the enum rename; otherwise the crate will fail to compile.
307-309: Duplicate-key check returns a wrapped error – confirmYamlError::ParseTokenConfigErrorvariant
YamlError::ParseTokenConfigError(ParseTokenConfigError::RemoteTokenKeyShadowing(..))is generated here.
Double-check thatYamlErrorwas updated to expose aParseTokenConfigErrorvariant (the old one wasParseTokenConfigSourceError). A mismatch will break compilation.
352-376: Enum rename is consistent – good job
ParseTokenConfigErrorcleanly replaces the*ConfigSourcepredecessor, andto_readable_msg()provides user-friendly output.
No issues spotted in this segment.packages/ui-components/src/__tests__/DropdownActiveSubgraphs.test.ts (1)
72-80: Test expectations correctly cover the new object shapeThe assertions now compare against
{ key, url }objects, matching the updated store contract – looks good.crates/common/src/dotrain_order/mod.rs (1)
16-19: LGTM! Consistent error type renaming.The renaming from
ParseConfigSourceErrortoParseConfigErroris consistent with the broader refactoring effort to remove theConfigSourceabstraction and transition to the unifiedConfigmodel.Also applies to: 43-43, 102-104
crates/settings/src/test.rs (1)
92-252: Well-structured test data for the new configuration model.The mock YAML constants provide comprehensive test coverage for both orderbook and dotrain configurations, supporting the new unified parsing approach. The data structure correctly represents the nested configuration model with all necessary domains.
crates/settings/src/chart.rs (1)
652-652: Consistent error type renaming.The renaming from
ParseChartConfigSourceErrortoParseChartConfigErroraligns with the pattern across other modules in this refactoring effort.Also applies to: 660-660
packages/ui-components/src/lib/utils/configHelpers.ts (2)
1-8: Clean utility functions for configuration transformation.The helper functions provide clear transformations from the new nested configuration structure to formats expected by UI components. The use of
satisfiesoperator provides good type safety.Note: The
satisfiesoperator requires TypeScript 4.9 or higher. Please ensure the project's TypeScript version meets this requirement.
14-44: Excellent inline test coverage.The Vitest inline tests provide good coverage for both utility functions with clear test cases.
packages/webapp/src/__tests__/resetActiveNetworkRef.test.ts (1)
4-4: Test suite successfully migrated to new configuration structure.The tests have been properly updated to work with the new nested
Configstructure, maintaining the same test coverage while correctly accessing networks under theorderbooknamespace. The mock factory includes all required properties forNetworkCfg.Also applies to: 10-17, 25-59
crates/common/src/unit_tests/mod.rs (1)
321-330: Verify that the lookup key really is a deployer key
get_deployers()returns a map keyed by deployer key, but the code queries it withtest_config.scenario_name.
If the scenario name ever diverges from the deployer key (e.g. scenario uses a different deployer viadeployer:field), this will silently fall through toScenarioNotFound.Please double-check the data model (scenario → deployer) and confirm the right key is used, or derive the deployer key from the scenario before the lookup.
Example fix:- .get(&self.settings.test_config.scenario_name) + .get( + &self + .settings + .test_config + .scenario + .deployer // or whatever field stores the deployer key + )tauri-app/src/routes/orders/[network]-[orderHash]/page.test.ts (1)
60-93: 🧹 Nitpick (assertive)Avoid the blanket
as unknown as ConfigcastThe test sidesteps the type-checker, which means a future schema change won’t surface here.
Prefer typing the literal asPartial<Config>(or a purpose-built helper) and only cast at the final call site:const mockCfg: Partial<Config> = { orderbook: { /* … */ }, }; mockSettingsStore.mockSetSubscribeValue(mockCfg as Config);This keeps IntelliSense & compile-time safety for the structure you do provide without forcing every field to be present.
⛔ Skipped due to learnings
Learnt from: hardingjam PR: rainlanguage/rain.orderbook#1597 File: packages/ui-components/src/__tests__/OrderDetail.test.ts:120-120 Timestamp: 2025-04-08T09:18:46.653Z Learning: In test files for the Rain Orderbook project, it's acceptable to bypass TypeScript's strict typing using constructs like `as unknown as [Type]` to create simplified mock objects that don't need to implement the entire interface.crates/settings/src/gui.rs (1)
174-183: Error type rename – check downstream crates
ParseGuiConfigErrorreplaced the oldParseGuiConfigSourceError.
Ensure every consumer crate (js_api, wasm, TS bindings) updated theirmatcharms; otherwise they’ll fail to compile or will hit the wildcard arm silently.packages/webapp/src/lib/stores/settings.ts (1)
96-107:activeSubgraphsround-trips complex objects through JSON
SubgraphCfgmay later includeMap,BigInt, orDateinstances whichJSON.stringifycannot safely serialise / revive. If the backend evolves, this will silently drop information.You could:
- Persist only the subgraph key, not the full object, or
- Use a custom (de)serialiser that is schema-aware.
tauri-app/src/lib/services/applySettings.test.ts (1)
26-40: YAML scalar type mismatch:versionis parsed as number, but test expects string
settingsContentdefinesversion: 1(unquoted).
YAML loaders treat this as a number, yet the test asserts
parsedConfig.orderbook.version === '1'(string).Either quote the scalar in the fixture or update the expectation:
- const settingsContent = ` -version: 1 + const settingsContent = ` +version: '1'or change the expected object’s type accordingly.
Without this fix the test will falsely pass only because the mock is injected,
masking a real integration bug.crates/settings/src/orderbook.rs (1)
187-205: SomeParseOrderbookConfigErrorvariants appear unused – confirm necessityAfter the refactor, only
AddressParseErroris currently referenced inside
OrderbookCfg.NetworkNotFoundErrorandSubgraphNotFoundErrorare now
emitted throughYamlError::Field::InvalidValueinstead.If these variants are no longer produced anywhere, consider removing them (or
re-routing the emitting code) to keep the error surface minimal.Tagging for verification rather than deletion because other modules may still
use them.crates/js_api/src/config/mod.rs (2)
4-10: LGTM! Clean WASM binding implementation.The function properly exposes the YAML parsing functionality with appropriate error handling and sensible defaults for the validation flag.
174-188: Good test coverage for the WASM binding.The test effectively verifies that the exported function produces identical results to the underlying API, with comprehensive test data covering both orderbook and dotrain configurations.
packages/ui-components/src/lib/__fixtures__/settings-12-11-24.json (1)
1-332: Test fixture correctly updated to match new Config structure.The fixture has been properly restructured to nest all configuration under the
"orderbook"key, aligning with the newConfigtype. The enriched object structures provide comprehensive test coverage.Note: As established in previous reviews, network configuration inconsistencies (like matchain using Polygon's RPC and chain ID) are acceptable in test fixtures.
crates/common/src/frontmatter.rs (2)
3-11: Function signature and implementation correctly updated.The migration from
ConfigSourcetoConfigis properly implemented, with the newConfig::try_from_yamlmethod handling the parsing logic.
29-401: Test suite properly migrated to new Config API.All tests have been correctly updated to use the new getter methods and data structures. The test coverage remains comprehensive, validating all configuration aspects.
crates/settings/src/network.rs (2)
51-63: Method signatures correctly updated with new error type.The validation methods now properly return the renamed
ParseNetworkConfigError, maintaining consistency with the codebase-wide refactoring.
253-286: Error enum properly renamed with excellent user-friendly messages.The
ParseNetworkConfigErrorenum and itsto_readable_msgimplementation provide clear, actionable error messages that will help users understand and fix configuration issues.tauri-app/src/__tests__/settings.test.ts (1)
11-484: Tests properly updated for the new Config structure.The test suite has been comprehensively updated to reflect the migration from
ConfigSourceto the nestedConfigstructure. All assertions and mock data correctly use the neworderbooknamespace.crates/settings/src/unit_test.rs (1)
15-102: Well-structured refactoring of test configuration types.The renaming to
UnitTestConfigand introduction of theScenarioTeststruct with comprehensive configuration options improves the test framework's capabilities. The wasm serialization attributes are properly configured.crates/settings/src/yaml/orderbook.rs (2)
101-191: Excellent refactoring using delegation pattern.The refactoring to have key-retrieval methods delegate to map-returning methods reduces code duplication and follows the DRY principle effectively. This makes the code more maintainable.
197-215: Improved boolean parsing with explicit validation.The updated
get_sentrymethod now properly validates boolean string representations and provides clear error messages for invalid values. The return typeOption<bool>better represents the optional nature of the setting.crates/settings/src/order.rs (1)
53-847: Consistent error type migration and improved parsing clarity.The migration from
ParseOrderConfigSourceErrortoParseOrderConfigErroris applied consistently throughout the file. The refactored input/output parsing logic (lines 573-730) maintains the same functionality while improving code readability with better structure and indentation.packages/ui-components/src/lib/types/appStores.ts (2)
1-22: LGTM! Type safety improvements enhance code reliability.The migration to strongly typed configuration objects (
Config,SubgraphCfg,AccountCfg,OrderbookCfg) from loosely typed strings improves type safety and will help catch configuration-related errors at compile time.
5-8:Details
✅ Verification successful
Verify removal of optional types won't cause initialization issues.
The
settingsandaccountsstores are no longer optional/undefined. Please ensure that:
- These stores are properly initialized with default values before being accessed
- Any code that previously handled undefined states for these stores is updated
🏁 Script executed:
#!/bin/bash # Search for code that might handle undefined settings or accounts rg -A 3 'settings.*\?' | grep -E '(undefined|null)' rg -A 3 'accounts.*\?' | grep -E '(undefined|null)' # Look for store initialization fd -e ts -e js | xargs rg 'writable.*Config|readable.*AccountCfg' -A 5Length of output: 9343
Verified:
settingsandaccountsstores are always initialized
All UI-library mocks and tests inpackages/ui-components/src/lib/__mocks__call
writable<Config>(…)andwritable<Record<string,AccountCfg>>({}), and no UI code uses optional chaining on these stores. Removing the optional qualifiers does not introduce any uninitialized paths.crates/settings/src/scenario.rs (1)
41-191: LGTM! Consistent error type renaming.The renaming from
ParseScenarioConfigSourceErrortoParseScenarioConfigErroris applied consistently throughout the file, aligning with the removal of theConfigSourceabstraction.crates/settings/src/yaml/dotrain.rs (1)
85-91: LGTM! Good API design with backwards compatibility.The addition of methods returning full configuration maps (
get_orders(),get_scenarios(),get_deployments(),get_charts()) improves the API flexibility while maintaining backwards compatibility through the existing key-only methods.crates/settings/src/yaml/mod.rs (1)
7-9: LGTM! Comprehensive and consistent error type renaming.All error types have been systematically renamed from
Parse*ConfigSourceErrortoParse*ConfigError, with updates applied consistently across imports, enum variants, PartialEq implementation, and error message formatting.Also applies to: 177-189, 222-230, 286-312
packages/ui-components/src/lib/__mocks__/stores.ts (2)
5-5: Good use of type alias to avoid naming conflicts.Using
WagmiConfigas an alias for@wagmi/core'sConfigtype prevents confusion with the@rainlanguage/orderbook'sConfigtype. This makes the code more maintainable.
26-26:Details
❓ Verification inconclusive
Verify type safety of the settings fixture cast.
The cast
as unknown as Configbypasses TypeScript's type checking. Ensure thatsettingsFixturefrom the JSON file matches the expectedConfigstructure to avoid runtime errors.Run the following script to verify the structure of the settings fixture:
🏁 Script executed:
#!/bin/bash # Description: Verify that the settings fixture matches the expected Config structure # Check if the settings fixture file exists and examine its structure cat packages/ui-components/src/lib/__fixtures__/settings-12-11-24.json | jq 'keys' | head -20 # Search for the Config type definition to understand expected structure ast-grep --pattern 'export interface Config { $$$ }'Length of output: 168
I can’t locate the
Configdefinition yet. Let’s inspect the mocks file’s imports and search the repo forConfigtypes:
🏁 Script executed:
#!/bin/bash # 1. Show the top of the mocks file to see where Config is imported sed -n '1,30p' packages/ui-components/src/lib/__mocks__/stores.ts # 2. Search for any TypeScript definitions of Config in the repo rg -n "export interface Config" -t ts . rg -n "interface Config" -t ts . rg -n "type Config" -t ts .Length of output: 2338
Confirm
settingsFixtureJSON shape matchesConfiginterface from@rainlanguage/orderbook.The cast
settingsFixture as unknown as Configbypasses TypeScript’s checks. Our quick inspection ofsettings-12-11-24.jsonshows only an"orderbook"key, but theConfiginterface may require additional fields. To avoid runtime mismatches, please:• packages/ui-components/src/lib/mocks/stores.ts (line 26)
– Review thesettings-12-11-24.jsonfixture and compare its full structure against theConfiginterface in@rainlanguage/orderbook.
– Add any missing properties to the JSON or consider a safer cast/parsing approach.packages/webapp/src/routes/+layout.ts (1)
30-37: Well-implemented error handling for YAML parsing.The error handling pattern with early return and readable error messages provides a good user experience when configuration parsing fails.
crates/settings/src/config.rs (3)
159-237: Excellent error handling with user-friendly messages.The
to_readable_msgmethod provides clear, actionable error messages that help users identify and fix configuration issues. The consistent pattern of mentioning "YAML configuration" in error messages is helpful.
365-485: Well-designed getter methods with consistent patterns.The dual getter pattern (get all vs get by key) provides flexibility for different use cases. The consistent error handling and clear error messages make the API user-friendly. Returning references to Arc-wrapped values is efficient for read-heavy workloads.
488-883: Comprehensive test coverage.The test suite thoroughly covers all aspects of the new configuration structure, including:
- All config sections (orderbook and dotrain_order)
- All entity types and their properties
- Nested relationships between entities
- Optional fields and default values
| import { getAccountsAsOptions } from '$lib/utils/configHelpers'; | ||
| export let accounts: AppStoresInterface['accounts']; | ||
| export let activeAccountsItems: AppStoresInterface['activeAccountsItems']; | ||
|
|
||
| $: options = $accounts; | ||
| $: options = getAccountsAsOptions($accounts); | ||
| </script> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Declare options to keep TypeScript happy
Reactive assignments create the variable at runtime, but the TS compiler will complain about an implicit any. Pre-declare with an explicit type:
+ let options: Record<string, string> = {};
$: options = getAccountsAsOptions($accounts);This also helps IDEs with intellisense.
🤖 Prompt for AI Agents
In
packages/ui-components/src/lib/components/dropdown/DropdownOrderListAccounts.svelte
around lines 4 to 9, the variable 'options' is assigned reactively but not
declared beforehand, causing TypeScript to infer an implicit 'any' type. To fix
this, explicitly declare the 'options' variable with the appropriate type before
the reactive assignment to satisfy TypeScript and improve IDE intellisense
support.
There was a problem hiding this comment.
i don't see this issue so no need to change the code
There was a problem hiding this comment.
@findolor Got it—if your TypeScript setup isn’t flagging an implicit-any warning, we can leave the code as-is. Let me know if anything else comes up!
| #[cfg(target_family = "wasm")] | ||
| pub mod config; | ||
| #[cfg(target_family = "wasm")] |
There was a problem hiding this comment.
config module hidden from non-WASM test builds – will break unit‐tests
All other modules that are needed by tests use
#[cfg(any(target_family = "wasm", test))], but config is guarded by
#[cfg(target_family = "wasm")] only.
As learned in PR #1744, WASM-only gates prevent non-wasm test targets from
compiling, because tests still import these symbols.
-#[cfg(target_family = "wasm")]
-pub mod config;
+#[cfg(any(target_family = "wasm", test))]
+pub mod config;This keeps production binaries unchanged while restoring test visibility.
🤖 Prompt for AI Agents
In crates/js_api/src/lib.rs around lines 5 to 7, the config module is currently
only included for the wasm target family, which causes non-wasm test builds to
fail since tests also need access to this module. Modify the cfg attribute to
#[cfg(any(target_family = "wasm", test))] so that the config module is included
both for wasm targets and during testing, ensuring tests can compile without
affecting production builds.
There was a problem hiding this comment.
test attribute is needed if we need to test out using non wasm environments. config does not have those so this is okay
There was a problem hiding this comment.
@findolor Understood. If no non-WASM tests exercise the config module, keeping the existing
#[cfg(target_family = "wasm")] guard is perfectly fine.
Thanks for the clarification—I’ll mark this resolved.
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1744
File: crates/subgraph/src/orderbook_client.rs:44-46
Timestamp: 2025-05-14T05:13:59.713Z
Learning: In the rain.orderbook project, WASM dependencies are intentionally made available in non-WASM targets to facilitate testing of WASM-related functionality, so conditional compilation guards like `#[cfg(target_family = "wasm")]` should not be added to imports or implementations that may be needed for tests.
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.
| url: value.url, | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
value.url may be undefined; no runtime guard
subgraph objects are typed but not runtime-validated. If the config is
malformed and url is missing, the invoke call will pass undefined, leading
to opaque backend errors.
Add a sanity check:
-if (!value) {
- throw new Error('Subgraph not found');
-}
+if (!value?.url) {
+ throw new Error('Subgraph url not found in settings');
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tauri-app/src/lib/services/order.ts around lines 34 to 35, the value.url may
be undefined and is passed without validation, which can cause backend errors.
Add a runtime check to verify that value.url is defined before using it; if it
is undefined, handle the error gracefully or provide a default/fallback value to
prevent passing undefined to the invoke call.
There was a problem hiding this comment.
that url field is not an optional field in the subgraph. this logic is fine
There was a problem hiding this comment.
@findolor Got it—since SubgraphCfg.url is guaranteed to be non-optional, the existing logic is fine as-is. Thanks for clarifying!
✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1917
File: tauri-app/src/lib/services/order.ts:34-35
Timestamp: 2025-06-11T11:15:14.987Z
Learning: In the settings schema, `SubgraphCfg` always includes a mandatory `url` field, so additional runtime checks for its presence are unnecessary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1831
File: packages/ui-components/src/__tests__/Transaction.test.ts:32-194
Timestamp: 2025-05-19T15:07:45.148Z
Learning: The TransactionStore always requires a subgraphUrl property and will always attempt to perform subgraph indexing. It may throw an error if the URL is invalid or fails to fetch, but it doesn't skip indexing when subgraphUrl is present.
| const value = get(subgraph); | ||
| if (!value) { | ||
| throw new Error('Subgraph not found'); | ||
| } | ||
| await invoke('order_remove', { |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Throwing hard error deviates from existing “graceful degradation” rule
Previously missing subgraph URLs degraded to an empty string (see
handleAddOrder learning). Replacing that with a thrown error will crash the
flow instead of showing the order page without indexing.
Consider reverting to the soft-fallback pattern or at least surfacing the
problem through the toast/error-handling layer to avoid uncaught exceptions.
Maintain graceful degradation when subgraph is missing
The current implementation in tauri-app/src/lib/services/order.ts throws a hard error if get(subgraph) is falsy, which will crash the Tauri flow instead of allowing the user to view their order page without indexing. To align with our existing “graceful degradation” approach (see handleAddOrder in packages/webapp/src/lib/services/handleAddOrder.ts), please:
- In
tauri-app/src/lib/services/order.ts:21-25, replace the thrown exception with a soft fallback or explicit user notification. - Example soft‐fallback pattern:
const value = get(subgraph) || ''; await invoke('order_remove', { subgraphUrl: value, /* …other params… */ });
- If you must surface the missing-URL case, catch the missing‐value scenario and show a toast/error message rather than letting an uncaught exception bubble up.
This change will prevent unexpected crashes while still informing users of any subgraph-indexing limitations.
🤖 Prompt for AI Agents
In tauri-app/src/lib/services/order.ts around lines 21 to 25, replace the thrown
error when get(subgraph) returns a falsy value with a soft fallback by assigning
an empty string instead. Modify the code to use const value = get(subgraph) ||
'' and proceed with the invoke call using this value. If you want to notify the
user about the missing subgraph, catch this case and display a toast or error
message instead of throwing an uncaught exception, ensuring the flow does not
crash and maintains graceful degradation.
There was a problem hiding this comment.
no need to do this as the error is handled in the ui side. another thing is at this point should be in the settings
There was a problem hiding this comment.
@findolor Got it—thanks for the clarification. I’ll leave the guard unchanged since the UI already handles the error and the subgraph should be present via settings at this stage. 👍
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1891
File: packages/webapp/src/lib/services/handleAddOrder.ts:94-94
Timestamp: 2025-06-09T11:14:12.358Z
Learning: In the handleAddOrder function in packages/webapp/src/lib/services/handleAddOrder.ts, using an empty string fallback for undefined subgraphUrl is acceptable. When subgraphUrl is missing/empty, transaction indexing cannot be shown to the user, but the deployment functionality still works and users can manually navigate to the order page to view their deployed orders. This graceful degradation approach is preferred over making subgraphUrl required.
| if ($settings?.orderbook.networks) { | ||
| for (const network of Object.values($settings.orderbook.networks)) { | ||
| rpcMap[network.chainId] = network.rpc; | ||
| chains.push(network.chainId); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Tiny type/robustness improvement for rpcMap population
network.chainId is a number but Record<string,string> expects string keys.
While JS coerces numbers to strings, being explicit avoids lint noise:
- rpcMap[network.chainId] = network.rpc;
+ rpcMap[String(network.chainId)] = network.rpc;Also guard with optional chaining to avoid runtime when orderbook exists but
networks is undefined:
if ($settings?.orderbook?.networks) { … }🤖 Prompt for AI Agents
In tauri-app/src/lib/stores/walletconnect.ts around lines 74 to 78, update the
if condition to use optional chaining for networks as
$settings?.orderbook?.networks to prevent runtime errors if networks is
undefined. Also, explicitly convert network.chainId to a string when assigning
keys to rpcMap to match the Record<string,string> type and avoid lint warnings.
There was a problem hiding this comment.
an orderbook always has a network field. no need to change that
There was a problem hiding this comment.
@findolor Got it—if orderbook.networks is always defined we can skip the extra optional-chaining guard.
One small point still worth considering: rpcMap is declared as Record<string, string>, so assigning a numeric chainId causes a type-mismatch that the compiler will flag. Converting it explicitly keeps the code noise-free without affecting behaviour:
rpcMap[String(network.chainId)] = network.rpc;
chains.push(network.chainId); // `chains` remains number[]Happy to leave everything else unchanged if that fits your expectations.
| return !isNil(mergedConfig) | ||
| ? pickBy( | ||
| mergedConfigSource.deployments, | ||
| (d) => mergedConfig?.scenarios?.[d.scenario]?.deployer?.network?.key === activeNetworkRef, | ||
| mergedConfig.dotrainOrder.deployments, | ||
| (d) => | ||
| mergedConfig.dotrainOrder.scenarios[d.scenario.key]?.deployer?.network?.key === | ||
| activeNetworkRef, | ||
| ) |
There was a problem hiding this comment.
Runtime crash risk when d.scenario is undefined
In some configs a deployment might reference a non-existent scenario (or validation might be disabled).
d.scenario.key will throw a TypeError. Use optional chaining like you did below, or filter earlier:
mergedConfig?.dotrainOrder.deployments,
(d) =>
d?.scenario?.key &&
mergedConfig.dotrainOrder.scenarios[d.scenario.key]?.deployer?.network?.key ===
activeNetworkRef,🤖 Prompt for AI Agents
In tauri-app/src/lib/services/pickConfig.ts around lines 8 to 14, the code risks
a runtime crash because it accesses d.scenario.key without checking if
d.scenario is defined. To fix this, update the predicate function to use
optional chaining for d.scenario.key, ensuring it only proceeds if d.scenario
and d.scenario.key exist. This prevents TypeError by safely handling cases where
d.scenario might be undefined.
| ? pickBy( | ||
| mergedConfig.dotrainOrder.scenarios, | ||
| (d) => d?.deployer?.network?.key === activeNetworkRef, | ||
| ) | ||
| : {}; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor: duplicate predicate logic – extract helper
Both pickDeployments and pickScenarios check the same deployer.network.key === activeNetworkRef condition. A small matchesNetwork(deployer, key) helper would remove duplication and keep the two functions symmetric.
🤖 Prompt for AI Agents
In tauri-app/src/lib/services/pickConfig.ts around lines 23 to 27, both
pickDeployments and pickScenarios functions duplicate the predicate logic
checking if deployer.network.key equals activeNetworkRef. Extract this condition
into a helper function named matchesNetwork that takes deployer and key as
arguments and returns the boolean result. Replace the duplicated predicate logic
in both functions with calls to this new helper to remove duplication and
improve symmetry.
There was a problem hiding this comment.
no need to do this
There was a problem hiding this comment.
Rate Limit Exceeded
@findolor have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 40 seconds before sending another message.
| export async function applySettings( | ||
| settingsContent: string, | ||
| settingsStore: Writable<ConfigSource | undefined>, | ||
| settingsStore: Writable<Config>, | ||
| settingsTextStore: Writable<string>, | ||
| parseConfigSourceFn: ParseConfigSourceFn, | ||
| ParseConfigFn: ParseConfigFn, | ||
| ): Promise<ApplySettingsResult> { | ||
| try { | ||
| settingsTextStore.set(settingsContent); | ||
| const parsedConfig = await parseConfigSourceFn(settingsContent); | ||
| const parsedConfig = await ParseConfigFn([settingsContent]); | ||
| settingsStore.set(parsedConfig); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameter name shadows the type alias – rename for clarity & avoid collisions
ParseConfigFn is used both as the type and the value of the parameter.
While legal in TS (types live in a separate namespace), it is confusing and
harms readability—especially when the value is passed around below.
export async function applySettings(
settingsContent: string,
settingsStore: Writable<Config>,
settingsTextStore: Writable<string>,
- ParseConfigFn: ParseConfigFn,
+ parseConfigFn: ParseConfigFn,
): Promise<ApplySettingsResult> {
try {
settingsTextStore.set(settingsContent);
- const parsedConfig = await ParseConfigFn([settingsContent]);
+ const parsedConfig = await parseConfigFn([settingsContent]);This avoids the namespace clash and follows the lower-camel-case convention
for variables/parameters.
📝 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 async function applySettings( | |
| settingsContent: string, | |
| settingsStore: Writable<ConfigSource | undefined>, | |
| settingsStore: Writable<Config>, | |
| settingsTextStore: Writable<string>, | |
| parseConfigSourceFn: ParseConfigSourceFn, | |
| ParseConfigFn: ParseConfigFn, | |
| ): Promise<ApplySettingsResult> { | |
| try { | |
| settingsTextStore.set(settingsContent); | |
| const parsedConfig = await parseConfigSourceFn(settingsContent); | |
| const parsedConfig = await ParseConfigFn([settingsContent]); | |
| settingsStore.set(parsedConfig); | |
| export async function applySettings( | |
| settingsContent: string, | |
| settingsStore: Writable<Config>, | |
| settingsTextStore: Writable<string>, | |
| parseConfigFn: ParseConfigFn, | |
| ): Promise<ApplySettingsResult> { | |
| try { | |
| settingsTextStore.set(settingsContent); | |
| const parsedConfig = await parseConfigFn([settingsContent]); | |
| settingsStore.set(parsedConfig); |
🤖 Prompt for AI Agents
In tauri-app/src/lib/services/applySettings.ts around lines 12 to 21, the
parameter named ParseConfigFn shadows the type alias of the same name, causing
confusion. Rename the parameter to a lowerCamelCase name like parseConfigFn to
avoid the namespace clash and improve readability. Update all references to this
parameter within the function accordingly.
There was a problem hiding this comment.
no need to make any changes
There was a problem hiding this comment.
Rate Limit Exceeded
@findolor have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 6 seconds before sending another message.
| const parsedConfig: Config = { | ||
| orderbook: { | ||
| version: '1', | ||
| networks: { mainnet: { key: 'mainnet', chainId: 1, rpc: 'rpc' } }, | ||
| }, | ||
| } as unknown as Config; | ||
| mockParseConfigFn = vi.fn().mockResolvedValue(parsedConfig); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Unnecessary as unknown as Config cast – use satisfies for safer typing
The literal already matches the Config shape being asserted. Prefer the
… satisfies Config pattern to keep type-checking while eliminating the
double cast:
- const parsedConfig: Config = {
+ const parsedConfig = {
orderbook: {
version: '1',
networks: { mainnet: { key: 'mainnet', chainId: 1, rpc: 'rpc' } },
},
- } as unknown as Config;
+ } satisfies Config;Cleaner, safer, and no run-time effect.
📝 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 parsedConfig: Config = { | |
| orderbook: { | |
| version: '1', | |
| networks: { mainnet: { key: 'mainnet', chainId: 1, rpc: 'rpc' } }, | |
| }, | |
| } as unknown as Config; | |
| mockParseConfigFn = vi.fn().mockResolvedValue(parsedConfig); | |
| const parsedConfig = { | |
| orderbook: { | |
| version: '1', | |
| networks: { mainnet: { key: 'mainnet', chainId: 1, rpc: 'rpc' } }, | |
| }, | |
| } satisfies Config; | |
| mockParseConfigFn = vi.fn().mockResolvedValue(parsedConfig); |
🤖 Prompt for AI Agents
In tauri-app/src/lib/services/applySettings.test.ts around lines 34 to 40,
replace the double type assertion 'as unknown as Config' on the parsedConfig
object with the 'satisfies Config' syntax. This change maintains type safety
without unnecessary casting, improving code clarity and type checking without
affecting runtime behavior.
There was a problem hiding this comment.
Rate Limit Exceeded
@findolor have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 53 seconds before sending another message.
| export const mockConfig: Config = { | ||
| orderbook: { | ||
| networks: { | ||
| mainnet: { | ||
| key: 'mainnet', | ||
| rpc: 'https://mainnet.infura.io/v3/YOUR-PROJECT-ID', | ||
| chainId: 1, | ||
| label: 'Ethereum Mainnet', | ||
| currency: 'ETH' | ||
| } | ||
| }, | ||
| subgraphs: { | ||
| mainnet: { | ||
| key: 'mainnet', | ||
| url: 'https://api.thegraph.com/subgraphs/name/mainnet' | ||
| }, | ||
| flare: { | ||
| key: 'flare', | ||
| url: 'https://api.thegraph.com/subgraphs/name/flare' | ||
| }, | ||
| testnet: { | ||
| key: 'testnet', | ||
| url: 'https://api.thegraph.com/subgraphs/name/testnet' | ||
| } | ||
| }, | ||
| orderbooks: { | ||
| orderbook1: { | ||
| key: 'orderbook1', | ||
| address: '0xOrderbookAddress1', | ||
| network: { | ||
| key: 'mainnet', | ||
| rpc: 'https://mainnet.infura.io/v3/YOUR-PROJECT-ID', | ||
| chainId: 1, | ||
| label: 'Ethereum Mainnet', | ||
| currency: 'ETH' | ||
| }, | ||
| subgraph: { | ||
| key: 'uniswap', | ||
| url: 'https://api.thegraph.com/subgraphs/name/uniswap' | ||
| }, | ||
| label: 'Orderbook 1' | ||
| } | ||
| }, | ||
| deployers: { | ||
| deployer1: { | ||
| key: 'deployer1', | ||
| address: '0xDeployerAddress1', | ||
| network: { | ||
| key: 'mainnet', | ||
| rpc: 'https://mainnet.infura.io/v3/YOUR-PROJECT-ID', | ||
| chainId: 1, | ||
| label: 'Ethereum Mainnet', | ||
| currency: 'ETH' | ||
| } | ||
| } | ||
| }, | ||
| metaboards: { | ||
| metaboard1: 'https://example.com/metaboard1' | ||
| }, | ||
| accounts: { | ||
| name_one: 'address_one', | ||
| name_two: 'address_two' | ||
| } | ||
| }, | ||
| subgraphs: { | ||
| mainnet: 'https://api.thegraph.com/subgraphs/name/mainnet' | ||
| }, | ||
| orderbooks: { | ||
| orderbook1: { | ||
| address: '0xOrderbookAddress1', | ||
| network: 'mainnet', | ||
| subgraph: 'uniswap', | ||
| label: 'Orderbook 1' | ||
| } | ||
| }, | ||
| deployers: { | ||
| deployer1: { | ||
| address: '0xDeployerAddress1', | ||
| network: 'mainnet', | ||
| label: 'Deployer 1' | ||
| } | ||
| }, | ||
| metaboards: { | ||
| metaboard1: 'https://example.com/metaboard1' | ||
| }, | ||
| accounts: { | ||
| name_one: 'address_one', | ||
| name_two: 'address_two' | ||
| } | ||
| }; | ||
| } as unknown as Config; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Replace the double cast with a type-safe satisfies assertion
mockConfig is first declared with an explicit Config type and then cast
with as unknown as Config, which is redundant and defeats structural checks.
-export const mockConfig: Config = {
+export const mockConfig = {
orderbook: {
…
- }
-} as unknown as Config;
+ }
+} satisfies Config;This keeps compile-time validation and avoids the extra cast noise.
📝 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 mockConfig: Config = { | |
| orderbook: { | |
| networks: { | |
| mainnet: { | |
| key: 'mainnet', | |
| rpc: 'https://mainnet.infura.io/v3/YOUR-PROJECT-ID', | |
| chainId: 1, | |
| label: 'Ethereum Mainnet', | |
| currency: 'ETH' | |
| } | |
| }, | |
| subgraphs: { | |
| mainnet: { | |
| key: 'mainnet', | |
| url: 'https://api.thegraph.com/subgraphs/name/mainnet' | |
| }, | |
| flare: { | |
| key: 'flare', | |
| url: 'https://api.thegraph.com/subgraphs/name/flare' | |
| }, | |
| testnet: { | |
| key: 'testnet', | |
| url: 'https://api.thegraph.com/subgraphs/name/testnet' | |
| } | |
| }, | |
| orderbooks: { | |
| orderbook1: { | |
| key: 'orderbook1', | |
| address: '0xOrderbookAddress1', | |
| network: { | |
| key: 'mainnet', | |
| rpc: 'https://mainnet.infura.io/v3/YOUR-PROJECT-ID', | |
| chainId: 1, | |
| label: 'Ethereum Mainnet', | |
| currency: 'ETH' | |
| }, | |
| subgraph: { | |
| key: 'uniswap', | |
| url: 'https://api.thegraph.com/subgraphs/name/uniswap' | |
| }, | |
| label: 'Orderbook 1' | |
| } | |
| }, | |
| deployers: { | |
| deployer1: { | |
| key: 'deployer1', | |
| address: '0xDeployerAddress1', | |
| network: { | |
| key: 'mainnet', | |
| rpc: 'https://mainnet.infura.io/v3/YOUR-PROJECT-ID', | |
| chainId: 1, | |
| label: 'Ethereum Mainnet', | |
| currency: 'ETH' | |
| } | |
| } | |
| }, | |
| metaboards: { | |
| metaboard1: 'https://example.com/metaboard1' | |
| }, | |
| accounts: { | |
| name_one: 'address_one', | |
| name_two: 'address_two' | |
| } | |
| }, | |
| subgraphs: { | |
| mainnet: 'https://api.thegraph.com/subgraphs/name/mainnet' | |
| }, | |
| orderbooks: { | |
| orderbook1: { | |
| address: '0xOrderbookAddress1', | |
| network: 'mainnet', | |
| subgraph: 'uniswap', | |
| label: 'Orderbook 1' | |
| } | |
| }, | |
| deployers: { | |
| deployer1: { | |
| address: '0xDeployerAddress1', | |
| network: 'mainnet', | |
| label: 'Deployer 1' | |
| } | |
| }, | |
| metaboards: { | |
| metaboard1: 'https://example.com/metaboard1' | |
| }, | |
| accounts: { | |
| name_one: 'address_one', | |
| name_two: 'address_two' | |
| } | |
| }; | |
| } as unknown as Config; | |
| export const mockConfig = { | |
| orderbook: { | |
| networks: { | |
| mainnet: { | |
| key: 'mainnet', | |
| rpc: 'https://mainnet.infura.io/v3/YOUR-PROJECT-ID', | |
| chainId: 1, | |
| label: 'Ethereum Mainnet', | |
| currency: 'ETH' | |
| } | |
| }, | |
| subgraphs: { | |
| mainnet: { | |
| key: 'mainnet', | |
| url: 'https://api.thegraph.com/subgraphs/name/mainnet' | |
| }, | |
| flare: { | |
| key: 'flare', | |
| url: 'https://api.thegraph.com/subgraphs/name/flare' | |
| }, | |
| testnet: { | |
| key: 'testnet', | |
| url: 'https://api.thegraph.com/subgraphs/name/testnet' | |
| } | |
| }, | |
| orderbooks: { | |
| orderbook1: { | |
| key: 'orderbook1', | |
| address: '0xOrderbookAddress1', | |
| network: { | |
| key: 'mainnet', | |
| rpc: 'https://mainnet.infura.io/v3/YOUR-PROJECT-ID', | |
| chainId: 1, | |
| label: 'Ethereum Mainnet', | |
| currency: 'ETH' | |
| }, | |
| subgraph: { | |
| key: 'uniswap', | |
| url: 'https://api.thegraph.com/subgraphs/name/uniswap' | |
| }, | |
| label: 'Orderbook 1' | |
| } | |
| }, | |
| deployers: { | |
| deployer1: { | |
| key: 'deployer1', | |
| address: '0xDeployerAddress1', | |
| network: { | |
| key: 'mainnet', | |
| rpc: 'https://mainnet.infura.io/v3/YOUR-PROJECT-ID', | |
| chainId: 1, | |
| label: 'Ethereum Mainnet', | |
| currency: 'ETH' | |
| } | |
| } | |
| }, | |
| metaboards: { | |
| metaboard1: 'https://example.com/metaboard1' | |
| }, | |
| accounts: { | |
| name_one: 'address_one', | |
| name_two: 'address_two' | |
| } | |
| } | |
| } satisfies Config; |
🤖 Prompt for AI Agents
In packages/ui-components/src/lib/__mocks__/settings.ts between lines 4 and 69,
the mockConfig object is redundantly cast using 'as unknown as Config' after
being explicitly typed as Config. Remove the explicit type annotation on
mockConfig and instead use a type-safe 'satisfies Config' assertion at the end
of the object declaration to maintain compile-time validation without the
unnecessary double cast.
There was a problem hiding this comment.
Rate Limit Exceeded
@findolor have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 48 seconds before sending another message.
| return Err(YamlError::ParseDeploymentConfigError( | ||
| ParseDeploymentConfigError::NoMatch, | ||
| )); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor: NoMatch error could include the conflicting keys for easier debugging
Currently the error gives a generic message. Including the scenario &
order keys (or their deployers) would make user diagnostics quicker.
No code change shown as this is an optional enhancement.
🤖 Prompt for AI Agents
In crates/settings/src/deployment.rs around lines 103 to 106, the NoMatch error
returned is generic and does not provide details about the conflicting keys.
Modify the error construction to include the scenario and order keys or their
deployers in the error message or error variant, so that users can more easily
diagnose which keys caused the conflict.
There was a problem hiding this comment.
this is not a part of this PR
There was a problem hiding this comment.
Rate Limit Exceeded
@findolor have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 27 seconds before sending another message.
| Object.entries($settings.orderbook.subgraphs).filter(([key, value]) => { | ||
| if (key in currentActiveSubgraphs) { | ||
| return currentActiveSubgraphs[key] === value; | ||
| return JSON.stringify(currentActiveSubgraphs[key]) === JSON.stringify(value); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using a deep equality function instead of JSON.stringify.
Using JSON.stringify for deep comparison works but has performance overhead and can fail with circular references. Consider using a dedicated deep equality function like lodash's isEqual.
- return JSON.stringify(currentActiveSubgraphs[key]) === JSON.stringify(value);
+ // Import isEqual from lodash at the top
+ return isEqual(currentActiveSubgraphs[key], value);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tauri-app/src/lib/stores/settings.ts at line 217, replace the
JSON.stringify-based comparison with a deep equality check using a dedicated
function like lodash's isEqual. Import isEqual from lodash and use it to compare
currentActiveSubgraphs[key] and value to improve performance and handle edge
cases like circular references.
There was a problem hiding this comment.
no need to change this
There was a problem hiding this comment.
@findolor Understood—no changes will be suggested here.
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1917
File: tauri-app/src/routes/orders/[network]-[orderHash]/+page.svelte:26-37
Timestamp: 2025-06-11T11:22:24.893Z
Learning: In the tauri-app codebase, the `settings` Svelte store is always initialized with a complete `Config` object whose `orderbook` field (and nested maps such as `orderbooks`, `subgraphs`, and `networks`) is guaranteed to exist, so optional chaining on those paths is unnecessary.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1916
File: packages/webapp/src/lib/services/resetActiveNetworkRef.ts:18-20
Timestamp: 2025-06-10T12:08:23.128Z
Learning: In the new NewConfig system that replaced ConfigSource in the rain.orderbook project, the settings store guarantees that $settings.orderbook.networks (and similar nested fields) will always be defined, so null/undefined safety checks are not needed when accessing these fields.
| () => { | ||
| try { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider explicit return in deserializer function.
The deserializer arrow function body appears incomplete. While the try-catch handles errors correctly, consider making the success path more explicit for clarity.
- () => {
+ (str) => {📝 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.
| () => { | |
| try { | |
| (str) => { | |
| try { |
🤖 Prompt for AI Agents
In tauri-app/src/lib/stores/settings.ts around lines 50 to 51, the deserializer
arrow function uses a try-catch block but lacks an explicit return statement on
the success path. To fix this, ensure the function explicitly returns the
deserialized value inside the try block before the catch, making the function's
output clear and unambiguous.
There was a problem hiding this comment.
we do not need this
There was a problem hiding this comment.
Rate Limit Exceeded
@findolor have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 40 seconds before sending another message.
| export const config: Config = { | ||
| networks: { | ||
| network1: { | ||
| key: 'network1', | ||
| rpc: 'rpc-url', | ||
| chainId: 14, | ||
| }, | ||
| network2: { | ||
| key: 'network2', | ||
| rpc: 'rpc-url', | ||
| chainId: 137, | ||
| }, | ||
| }, | ||
| subgraphs: { | ||
| network1: { | ||
| key: 'some-key', | ||
| url: 'some-url', | ||
| }, | ||
| }, | ||
| metaboards: { | ||
| network1: 'some-url', | ||
| }, | ||
| orderbooks: { | ||
| network1: { | ||
| key: 'network1', | ||
| address: '0x123456', | ||
| network: { | ||
| orderbook: { | ||
| version: '1', | ||
| networks: { | ||
| network1: { | ||
| key: 'network1', | ||
| rpc: 'rpc-url', | ||
| chainId: 14, | ||
| }, | ||
| subgraph: { | ||
| network2: { | ||
| key: 'network2', | ||
| rpc: 'rpc-url', | ||
| chainId: 137, | ||
| }, | ||
| }, | ||
| subgraphs: { | ||
| network1: { | ||
| key: 'some-key', | ||
| url: 'some-url', | ||
| }, | ||
| }, | ||
| }, | ||
| deployers: { | ||
| network1: { | ||
| key: 'network1', | ||
| address: '0xabcdef', | ||
| network: { | ||
| metaboards: { | ||
| network1: 'some-url', | ||
| }, | ||
| orderbooks: { | ||
| network1: { | ||
| key: 'network1', | ||
| rpc: 'rpc-url', | ||
| chainId: 14, | ||
| address: '0x123456', | ||
| network: { | ||
| key: 'network1', | ||
| rpc: 'rpc-url', | ||
| chainId: 14, | ||
| }, | ||
| subgraph: { | ||
| key: 'some-key', | ||
| url: 'some-url', | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| tokens: {}, | ||
| orders: { | ||
| buy: { | ||
| key: 'buy', | ||
| inputs: [], | ||
| outputs: [], | ||
| network: { | ||
| deployers: { | ||
| network1: { | ||
| key: 'network1', | ||
| rpc: 'rpc-url', | ||
| chainId: 14, | ||
| address: '0xabcdef', | ||
| network: { | ||
| key: 'network1', | ||
| rpc: 'rpc-url', | ||
| chainId: 14, | ||
| }, | ||
| }, | ||
| }, | ||
| sell: { | ||
| key: 'sell', | ||
| inputs: [], | ||
| outputs: [], | ||
| network: { | ||
| key: 'network1', | ||
| rpc: 'rpc-url', | ||
| chainId: 14, | ||
| tokens: {}, | ||
| accounts: { | ||
| name_one: { | ||
| key: 'name_one', | ||
| address: 'address_one', | ||
| }, | ||
| name_two: { | ||
| key: 'name_two', | ||
| address: 'address_two', | ||
| }, | ||
| }, | ||
| }, | ||
| scenarios: { | ||
| 'network1.sell': { | ||
| key: 'network1.sell', | ||
| bindings: {}, | ||
| deployer: { | ||
| key: 'network1', | ||
| address: '0xabcdef', | ||
| dotrainOrder: { | ||
| orders: { | ||
| buy: { | ||
| key: 'buy', | ||
| inputs: [], | ||
| outputs: [], | ||
| network: { | ||
| key: 'network1', | ||
| rpc: 'rpc-url', | ||
| chainId: 14, | ||
| }, | ||
| }, | ||
| }, | ||
| network1: { | ||
| key: 'network1', | ||
| bindings: {}, | ||
| deployer: { | ||
| key: 'network1', | ||
| address: '0xabcdef', | ||
| sell: { | ||
| key: 'sell', | ||
| inputs: [], | ||
| outputs: [], | ||
| network: { | ||
| key: 'network1', | ||
| rpc: 'rpc-url', | ||
| chainId: 14, | ||
| }, | ||
| }, | ||
| }, | ||
| 'network1.buy': { | ||
| key: 'network1.buy', | ||
| bindings: {}, | ||
| deployer: { | ||
| scenarios: { | ||
| 'network1.sell': { | ||
| key: 'network1.sell', | ||
| bindings: {}, | ||
| 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.
🛠️ Refactor suggestion
Move shared test data to a dedicated test utilities file.
Exporting from test files is generally discouraged as it can lead to coupling between tests and confusion about the purpose of the code. Consider moving the config constant to a dedicated test utilities or fixtures file.
Create a new file tauri-app/src/__tests__/fixtures/configFixtures.ts:
import type { Config } from '@rainlanguage/orderbook';
export const mockConfig: Config = {
orderbook: {
// ... rest of the config object
},
dotrainOrder: {
// ... rest of the config object
}
};Then update this test file:
-export const config: Config = {
- orderbook: {
- // ... configuration
- },
- dotrainOrder: {
- // ... configuration
- }
-};
+import { mockConfig as config } from './fixtures/configFixtures';🧰 Tools
🪛 Biome (1.9.4)
[error] 5-188: Do not export from a test file.
(lint/suspicious/noExportsInTest)
🤖 Prompt for AI Agents
In tauri-app/src/__tests__/pickConfig.test.ts from lines 6 to 188, the config
constant is currently defined and exported within the test file, which can cause
unwanted coupling and confusion. To fix this, move the entire config object to a
new dedicated test utilities file named
tauri-app/src/__tests__/fixtures/configFixtures.ts, export it there as
mockConfig, and then import mockConfig into pickConfig.test.ts to use in the
tests instead of defining it inline.
There was a problem hiding this comment.
not needed for now
There was a problem hiding this comment.
Rate Limit Exceeded
@findolor have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 30 seconds before sending another message.
| 'https://raw.githubusercontent.com/rainlanguage/rain.strategies/c300d06cc8b460b06104849679284a2b0600240d/settings.yaml' | ||
| ); | ||
| if (!response.ok) { | ||
| throw new Error('Error status: ' + response.status.toString()); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use template literal instead of string concatenation.
Replace string concatenation with a template literal for better readability and consistency.
- throw new Error('Error status: ' + response.status.toString());
+ throw new Error(`Error status: ${response.status}`);📝 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.
| throw new Error('Error status: ' + response.status.toString()); | |
| - throw new Error('Error status: ' + response.status.toString()); | |
| + throw new Error(`Error status: ${response.status}`); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 26-26: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
🤖 Prompt for AI Agents
In packages/webapp/src/routes/+layout.ts at line 26, replace the string
concatenation used in the Error constructor with a template literal. Change the
code to use backticks and embed the response.status value directly within the
string for improved readability and consistency.
There was a problem hiding this comment.
no need to change this one
There was a problem hiding this comment.
Rate Limit Exceeded
@findolor have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 10 seconds before sending another message.
| const subgraph = derived([settings, activeOrderbook], ([$settings, $activeOrderbook]) => | ||
| $settings.orderbook.subgraphs !== undefined && | ||
| Object.entries($settings.orderbook.subgraphs).length > 0 && | ||
| $activeOrderbook?.subgraph !== undefined | ||
| ? $settings.orderbook.subgraphs[$activeOrderbook.subgraph.key] | ||
| : undefined | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Breaking change: subgraphUrl renamed to subgraph.
The store has been renamed from subgraphUrl to subgraph and now returns a SubgraphCfg object instead of a string URL. Ensure all components consuming this store are updated to access the URL via subgraph.url.
Run the following script to find components that might need updating:
🏁 Script executed:
#!/bin/bash
# Description: Find usages of the old subgraphUrl store that need to be updated
# Search for references to subgraphUrl in the codebase
rg -A 3 "subgraphUrl" --type svelte --type ts --type js
# Search for components that might be using the subgraph-related stores
ast-grep --pattern '$stores.subgraphUrl'Length of output: 129
🏁 Script executed:
#!/bin/bash
# Description: Find any remaining references to the old subgraphUrl store
rg -n "subgraphUrl" -g '*.svelte' -g '*.ts' -g '*.js'Length of output: 16998
Update all references from subgraphUrl to the new subgraph store and access its url property
There are numerous usages of the old subgraphUrl variable, prop and test fixture across both webapp and ui-components. Since the derived store now returns a SubgraphCfg object, you must:
- Replace any declaration or prop
with
let subgraphUrl: string | undefined = …
let subgraph: SubgraphCfg | undefined = …
- Update every occurrence of
subgraphUrlin templates and scripts tosubgraph?.url - In tests, change expectations and mock calls from
.subgraphUrlto.subgraph.url - Audit service and provider functions that take
subgraphUrl: stringand update signatures to acceptSubgraphCfg(or pull.urlbefore passing)
Key locations to adjust (non-exhaustive):
• packages/webapp/src/routes//+page.svelte – declarations and uses in Orders and Vaults routes
• packages/webapp/src/lib/services/handleVaultDeposit.ts, handleVaultWithdraw.ts, handleRemoveOrder.ts
• packages/ui-components/src/lib/stores/transactionStore.ts
• packages/ui-components/src/lib/components//**/*.svelte – DeploymentSteps, VaultDetail, OrderDetail, charts & tables
• All tests under packages/webapp/src/__tests__ and packages/ui-components/src/__tests__ referencing subgraphUrl
Before merging, run:
rg -g '*.svelte' -g '*.ts' -g '*.js' "subgraphUrl"to ensure no dangling references remain.
🤖 Prompt for AI Agents
In packages/webapp/src/routes/+layout.ts around lines 73 to 79, the store
previously named subgraphUrl has been renamed to subgraph and now returns a
SubgraphCfg object instead of a string URL. Update all references from
subgraphUrl to subgraph and access the URL via subgraph.url. This includes
changing variable declarations from string or undefined to SubgraphCfg or
undefined, updating template and script usages to use subgraph?.url, modifying
test expectations and mocks to use subgraph.url, and adjusting service and
provider function signatures to accept SubgraphCfg or extract the URL before
passing. After these changes, run a global search for subgraphUrl to ensure no
references remain.
There was a problem hiding this comment.
sometimes in the repository we use subgraph and other times subgraphurl. the variables you see are the places we actually need the subraph url as a prop so we do not need to make any changes here
There was a problem hiding this comment.
@findolor Got it—thanks for clarifying. I’ll keep in mind that subgraph (full object) and subgraphUrl (string) coexist by design and that the occurrences you pointed out are the correct ones for passing the raw URL, so no further changes are required here.
✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1917
File: packages/webapp/src/routes/+layout.ts:73-79
Timestamp: 2025-06-11T11:49:05.841Z
Learning: In this codebase it is intentional to have both `subgraph` (holding a `SubgraphCfg` object) and `subgraphUrl` (holding just the URL string). Wherever only the URL is needed as a prop, the `subgraphUrl` variable is retained, and no renaming to `subgraph` is expected.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1917
File: tauri-app/src/lib/stores/order.ts:10-15
Timestamp: 2025-06-11T11:27:14.379Z
Learning: In this codebase, Svelte writable/derived stores (e.g., `subgraph` in `tauri-app/src/lib/stores/settings.ts`) expose a custom asynchronous `.load()` helper that retrieves the current value, so calls like `await subgraph.load()` are valid.
| impl Config { | ||
| pub fn try_from_yaml(yaml: Vec<String>, validate: bool) -> Result<Self, ParseConfigError> { | ||
| let dotrain_yaml = DotrainYaml::new(yaml.clone(), validate)?; | ||
| let orderbook_yaml = OrderbookYaml::new(yaml, validate)?; | ||
|
|
||
| let networks = orderbook_yaml | ||
| .get_networks() | ||
| .unwrap_or_default() | ||
| .into_iter() | ||
| .map(|(name, subgraph)| { | ||
| ( | ||
| name.clone(), | ||
| Arc::new(SubgraphCfg { | ||
| document: Arc::new(RwLock::new(StrictYaml::String("".to_string()))), | ||
| key: name.clone(), | ||
| url: subgraph.clone(), | ||
| }), | ||
| ) | ||
| }) | ||
| .collect::<HashMap<String, Arc<SubgraphCfg>>>(); | ||
|
|
||
| let metaboards = item | ||
| .metaboards | ||
| .map(|(k, v)| (k, Arc::new(v))) | ||
| .collect::<HashMap<_, _>>(); | ||
| let subgraphs = orderbook_yaml | ||
| .get_subgraphs() | ||
| .unwrap_or_default() | ||
| .into_iter() | ||
| .map(|(name, metaboard)| (name, Arc::new(metaboard))) | ||
| .collect::<HashMap<String, Arc<Url>>>(); | ||
|
|
||
| let orderbooks = item | ||
| .orderbooks | ||
| .map(|(k, v)| (k, Arc::new(v))) | ||
| .collect::<HashMap<_, _>>(); | ||
| let metaboards = orderbook_yaml | ||
| .get_metaboards() | ||
| .unwrap_or_default() | ||
| .into_iter() | ||
| .map(|(name, orderbook)| { | ||
| Ok(( | ||
| name.clone(), | ||
| Arc::new(orderbook.try_into_orderbook(name, &networks, &subgraphs)?), | ||
| )) | ||
| }) | ||
| .collect::<Result<HashMap<String, Arc<OrderbookCfg>>, ParseConfigSourceError>>()?; | ||
|
|
||
| let tokens = item | ||
| .tokens | ||
| .map(|(k, v)| (k, Arc::new(v.url))) | ||
| .collect::<HashMap<_, _>>(); | ||
| let orderbooks = orderbook_yaml | ||
| .get_orderbooks() | ||
| .unwrap_or_default() | ||
| .into_iter() | ||
| .map(|(name, token)| { | ||
| Ok(( | ||
| name.clone(), | ||
| Arc::new(token.try_into_token(&name, &networks)?), | ||
| )) | ||
| }) | ||
| .collect::<Result<HashMap<String, Arc<TokenCfg>>, ParseConfigSourceError>>()?; | ||
|
|
||
| let deployers = item | ||
| .deployers | ||
| .map(|(k, v)| (k, Arc::new(v))) | ||
| .collect::<HashMap<_, _>>(); | ||
| let tokens = orderbook_yaml | ||
| .get_tokens() | ||
| .unwrap_or_default() | ||
| .into_iter() | ||
| .map(|(name, deployer)| { | ||
| Ok(( | ||
| name.clone(), | ||
| Arc::new(deployer.try_into_deployer(name, &networks)?), | ||
| )) | ||
| }) | ||
| .collect::<Result<HashMap<String, Arc<DeployerCfg>>, ParseConfigSourceError>>()?; | ||
|
|
||
| let orders = item | ||
| .orders | ||
| .map(|(k, v)| (k, Arc::new(v))) | ||
| .collect::<HashMap<_, _>>(); | ||
| let deployers = orderbook_yaml | ||
| .get_deployers() | ||
| .unwrap_or_default() | ||
| .into_iter() | ||
| .map(|(name, order)| { | ||
| Ok(( | ||
| name, | ||
| Arc::new(order.try_into_order(&deployers, &orderbooks, &tokens)?), | ||
| )) | ||
| }) | ||
| .collect::<Result<HashMap<String, Arc<OrderCfg>>, ParseConfigSourceError>>()?; | ||
|
|
||
| // Initialize an empty HashMap for all scenarios | ||
| let mut scenarios = HashMap::new(); | ||
|
|
||
| // Directly iterate over scenarios if it's a HashMap | ||
| for (name, scenario_string) in item.scenarios { | ||
| let scenario_map = scenario_string.try_into_scenarios( | ||
| name.clone(), | ||
| &ScenarioParent::default(), | ||
| &deployers, | ||
| )?; | ||
|
|
||
| // Merge the scenarios | ||
| scenarios.extend(scenario_map); | ||
| } | ||
|
|
||
| let deployments = item | ||
| .deployments | ||
| .map(|(k, v)| (k, Arc::new(v))) | ||
| .collect::<HashMap<_, _>>(); | ||
| let orders = dotrain_yaml | ||
| .get_orders() | ||
| .unwrap_or_default() | ||
| .into_iter() | ||
| .map(|(name, deployment)| { | ||
| Ok(( | ||
| name, | ||
| Arc::new(deployment.try_into_deployment(&scenarios, &orders)?), | ||
| )) | ||
| }) | ||
| .collect::<Result<HashMap<String, Arc<DeploymentCfg>>, ParseConfigSourceError>>()?; | ||
|
|
||
| let charts = item | ||
| .charts | ||
| .map(|(k, v)| (k, Arc::new(v))) | ||
| .collect::<HashMap<_, _>>(); | ||
| let scenarios = dotrain_yaml | ||
| .get_scenarios() | ||
| .unwrap_or_default() | ||
| .into_iter() | ||
| .map(|(name, chart)| { | ||
| Ok(( | ||
| name.clone(), | ||
| Arc::new(chart.try_into_chart(name, &scenarios)?), | ||
| )) | ||
| }) | ||
| .collect::<Result<HashMap<String, Arc<ChartCfg>>, ParseConfigSourceError>>()?; | ||
|
|
||
| let accounts = item.accounts.map(|wl| { | ||
| wl.into_iter() | ||
| .map(|(name, address)| (name, Arc::new(address))) | ||
| .collect::<HashMap<String, Arc<String>>>() | ||
| }); | ||
|
|
||
| let gui = match item.gui { | ||
| Some(g) => Some(g.try_into_gui(&deployments, &tokens)?), | ||
| None => None, | ||
| }; | ||
| .map(|(k, v)| (k, Arc::new(v))) | ||
| .collect::<HashMap<_, _>>(); | ||
| let deployments = dotrain_yaml | ||
| .get_deployments() | ||
| .unwrap_or_default() | ||
| .into_iter() | ||
| .map(|(k, v)| (k, Arc::new(v))) | ||
| .collect::<HashMap<_, _>>(); | ||
| let charts = dotrain_yaml | ||
| .get_charts() | ||
| .unwrap_or_default() | ||
| .into_iter() | ||
| .map(|(k, v)| (k, Arc::new(v))) | ||
| .collect::<HashMap<_, _>>(); | ||
| let sentry = orderbook_yaml.get_sentry()?; | ||
| let version = orderbook_yaml.get_spec_version()?; | ||
| let gui = dotrain_yaml.get_gui(None)?; | ||
| let accounts = orderbook_yaml | ||
| .get_accounts() | ||
| .unwrap_or_default() | ||
| .into_iter() | ||
| .map(|(k, v)| (k, Arc::new(v))) | ||
| .collect::<HashMap<_, _>>(); | ||
|
|
||
| let config = Config { | ||
| version: item.version, | ||
| let orderbook_config = OrderbookConfig { | ||
| networks, | ||
| subgraphs, | ||
| metaboards, | ||
| orderbooks, | ||
| tokens, | ||
| deployers, | ||
| sentry, | ||
| version, | ||
| accounts, | ||
| }; | ||
| let dotrain_order_config = DotrainOrderConfig { | ||
| orders, | ||
| scenarios, | ||
| charts, | ||
| deployments, | ||
| sentry: item.sentry, | ||
| accounts, | ||
| gui, | ||
| charts, | ||
| }; | ||
|
|
||
| let config = Config { | ||
| dotrain_order: dotrain_order_config, | ||
| orderbook: orderbook_config, | ||
| }; | ||
| Ok(config) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider documenting the rationale for wrapping all config values in Arc.
All configuration values are wrapped in Arc<T>. While this enables thread-safe shared ownership, it adds overhead for cloning references. Consider adding a comment explaining why this pattern is necessary, especially if the config is mostly read-only after initialization.
Add documentation above the Config struct:
+/// Configuration structure that wraps all values in Arc to enable efficient
+/// shared ownership across multiple threads and components without deep cloning.
#[derive(Debug, Serialize, Deserialize, Default, Clone, PartialEq)]
#[serde(rename_all = "camelCase")]
#[cfg_attr(target_family = "wasm", derive(Tsify))]
pub struct Config {📝 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.
| impl Config { | |
| pub fn try_from_yaml(yaml: Vec<String>, validate: bool) -> Result<Self, ParseConfigError> { | |
| let dotrain_yaml = DotrainYaml::new(yaml.clone(), validate)?; | |
| let orderbook_yaml = OrderbookYaml::new(yaml, validate)?; | |
| let networks = orderbook_yaml | |
| .get_networks() | |
| .unwrap_or_default() | |
| .into_iter() | |
| .map(|(name, subgraph)| { | |
| ( | |
| name.clone(), | |
| Arc::new(SubgraphCfg { | |
| document: Arc::new(RwLock::new(StrictYaml::String("".to_string()))), | |
| key: name.clone(), | |
| url: subgraph.clone(), | |
| }), | |
| ) | |
| }) | |
| .collect::<HashMap<String, Arc<SubgraphCfg>>>(); | |
| let metaboards = item | |
| .metaboards | |
| .map(|(k, v)| (k, Arc::new(v))) | |
| .collect::<HashMap<_, _>>(); | |
| let subgraphs = orderbook_yaml | |
| .get_subgraphs() | |
| .unwrap_or_default() | |
| .into_iter() | |
| .map(|(name, metaboard)| (name, Arc::new(metaboard))) | |
| .collect::<HashMap<String, Arc<Url>>>(); | |
| let orderbooks = item | |
| .orderbooks | |
| .map(|(k, v)| (k, Arc::new(v))) | |
| .collect::<HashMap<_, _>>(); | |
| let metaboards = orderbook_yaml | |
| .get_metaboards() | |
| .unwrap_or_default() | |
| .into_iter() | |
| .map(|(name, orderbook)| { | |
| Ok(( | |
| name.clone(), | |
| Arc::new(orderbook.try_into_orderbook(name, &networks, &subgraphs)?), | |
| )) | |
| }) | |
| .collect::<Result<HashMap<String, Arc<OrderbookCfg>>, ParseConfigSourceError>>()?; | |
| let tokens = item | |
| .tokens | |
| .map(|(k, v)| (k, Arc::new(v.url))) | |
| .collect::<HashMap<_, _>>(); | |
| let orderbooks = orderbook_yaml | |
| .get_orderbooks() | |
| .unwrap_or_default() | |
| .into_iter() | |
| .map(|(name, token)| { | |
| Ok(( | |
| name.clone(), | |
| Arc::new(token.try_into_token(&name, &networks)?), | |
| )) | |
| }) | |
| .collect::<Result<HashMap<String, Arc<TokenCfg>>, ParseConfigSourceError>>()?; | |
| let deployers = item | |
| .deployers | |
| .map(|(k, v)| (k, Arc::new(v))) | |
| .collect::<HashMap<_, _>>(); | |
| let tokens = orderbook_yaml | |
| .get_tokens() | |
| .unwrap_or_default() | |
| .into_iter() | |
| .map(|(name, deployer)| { | |
| Ok(( | |
| name.clone(), | |
| Arc::new(deployer.try_into_deployer(name, &networks)?), | |
| )) | |
| }) | |
| .collect::<Result<HashMap<String, Arc<DeployerCfg>>, ParseConfigSourceError>>()?; | |
| let orders = item | |
| .orders | |
| .map(|(k, v)| (k, Arc::new(v))) | |
| .collect::<HashMap<_, _>>(); | |
| let deployers = orderbook_yaml | |
| .get_deployers() | |
| .unwrap_or_default() | |
| .into_iter() | |
| .map(|(name, order)| { | |
| Ok(( | |
| name, | |
| Arc::new(order.try_into_order(&deployers, &orderbooks, &tokens)?), | |
| )) | |
| }) | |
| .collect::<Result<HashMap<String, Arc<OrderCfg>>, ParseConfigSourceError>>()?; | |
| // Initialize an empty HashMap for all scenarios | |
| let mut scenarios = HashMap::new(); | |
| // Directly iterate over scenarios if it's a HashMap | |
| for (name, scenario_string) in item.scenarios { | |
| let scenario_map = scenario_string.try_into_scenarios( | |
| name.clone(), | |
| &ScenarioParent::default(), | |
| &deployers, | |
| )?; | |
| // Merge the scenarios | |
| scenarios.extend(scenario_map); | |
| } | |
| let deployments = item | |
| .deployments | |
| .map(|(k, v)| (k, Arc::new(v))) | |
| .collect::<HashMap<_, _>>(); | |
| let orders = dotrain_yaml | |
| .get_orders() | |
| .unwrap_or_default() | |
| .into_iter() | |
| .map(|(name, deployment)| { | |
| Ok(( | |
| name, | |
| Arc::new(deployment.try_into_deployment(&scenarios, &orders)?), | |
| )) | |
| }) | |
| .collect::<Result<HashMap<String, Arc<DeploymentCfg>>, ParseConfigSourceError>>()?; | |
| let charts = item | |
| .charts | |
| .map(|(k, v)| (k, Arc::new(v))) | |
| .collect::<HashMap<_, _>>(); | |
| let scenarios = dotrain_yaml | |
| .get_scenarios() | |
| .unwrap_or_default() | |
| .into_iter() | |
| .map(|(name, chart)| { | |
| Ok(( | |
| name.clone(), | |
| Arc::new(chart.try_into_chart(name, &scenarios)?), | |
| )) | |
| }) | |
| .collect::<Result<HashMap<String, Arc<ChartCfg>>, ParseConfigSourceError>>()?; | |
| let accounts = item.accounts.map(|wl| { | |
| wl.into_iter() | |
| .map(|(name, address)| (name, Arc::new(address))) | |
| .collect::<HashMap<String, Arc<String>>>() | |
| }); | |
| let gui = match item.gui { | |
| Some(g) => Some(g.try_into_gui(&deployments, &tokens)?), | |
| None => None, | |
| }; | |
| .map(|(k, v)| (k, Arc::new(v))) | |
| .collect::<HashMap<_, _>>(); | |
| let deployments = dotrain_yaml | |
| .get_deployments() | |
| .unwrap_or_default() | |
| .into_iter() | |
| .map(|(k, v)| (k, Arc::new(v))) | |
| .collect::<HashMap<_, _>>(); | |
| let charts = dotrain_yaml | |
| .get_charts() | |
| .unwrap_or_default() | |
| .into_iter() | |
| .map(|(k, v)| (k, Arc::new(v))) | |
| .collect::<HashMap<_, _>>(); | |
| let sentry = orderbook_yaml.get_sentry()?; | |
| let version = orderbook_yaml.get_spec_version()?; | |
| let gui = dotrain_yaml.get_gui(None)?; | |
| let accounts = orderbook_yaml | |
| .get_accounts() | |
| .unwrap_or_default() | |
| .into_iter() | |
| .map(|(k, v)| (k, Arc::new(v))) | |
| .collect::<HashMap<_, _>>(); | |
| let config = Config { | |
| version: item.version, | |
| let orderbook_config = OrderbookConfig { | |
| networks, | |
| subgraphs, | |
| metaboards, | |
| orderbooks, | |
| tokens, | |
| deployers, | |
| sentry, | |
| version, | |
| accounts, | |
| }; | |
| let dotrain_order_config = DotrainOrderConfig { | |
| orders, | |
| scenarios, | |
| charts, | |
| deployments, | |
| sentry: item.sentry, | |
| accounts, | |
| gui, | |
| charts, | |
| }; | |
| let config = Config { | |
| dotrain_order: dotrain_order_config, | |
| orderbook: orderbook_config, | |
| }; | |
| Ok(config) | |
| } | |
| /// Configuration structure that wraps all values in Arc to enable efficient | |
| /// shared ownership across multiple threads and components without deep cloning. | |
| #[derive(Debug, Serialize, Deserialize, Default, Clone, PartialEq)] | |
| #[serde(rename_all = "camelCase")] | |
| #[cfg_attr(target_family = "wasm", derive(Tsify))] | |
| pub struct Config { | |
| dotrain_order: DotrainOrderConfig, | |
| orderbook: OrderbookConfig, | |
| } |
🤖 Prompt for AI Agents
In crates/settings/src/config.rs around lines 256 to 355, the code wraps all
configuration values in Arc<T> to enable thread-safe shared ownership, but this
adds cloning overhead. Add a comment above the Config struct explaining that Arc
is used to allow safe concurrent access and sharing of config data across
threads without copying the underlying data, which is important because the
config is mostly read-only after initialization.
There was a problem hiding this comment.
no need to add additional documentatiton
There was a problem hiding this comment.
Rate Limit Exceeded
@findolor have exceeded the limit for the number of chat messages per hour. Please wait 2 minutes and 0 seconds before sending another message.
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (6)
tauri-app/src/lib/stores/order.ts (1)
15-37: 🧹 Nitpick (assertive)Minor optimisation: avoid three sequential
await subgraph.load()invocations
getSubgraphUrl()is called separately in each query/command, causing three independent loads during a single UI action.
Cache the URL per invocation context to spare the extra promise churn:- async (id: string) => { - const url = await getSubgraphUrl(); + async (id: string) => { + const url = cachedUrl ?? (cachedUrl = await getSubgraphUrl());(You can keep
cachedUrlin module scope or pass it down.)Purely optional – the current code works correctly.
packages/ui-components/src/lib/components/detail/VaultDetail.svelte (1)
44-57:⚠️ Potential issue
subgraphUrlis not reactive – value will never update after mountBecause the assignment is executed once at component initialisation, later changes to
$settingsornetworkwon’t propagate:const subgraphUrl = $settings.orderbook.subgraphs[network]?.url ?? '';Make it reactive:
-const subgraphUrl = $settings.orderbook.subgraphs[network]?.url ?? ''; +$: subgraphUrl = $settings.orderbook.subgraphs[network]?.url ?? '';or declare
let subgraphUrl; $: subgraphUrl = ….Failing to do so risks stale data if the user switches networks after the component mounts.
packages/ui-components/src/__tests__/DeploymentSteps.test.ts (1)
48-55: 🧹 Nitpick (assertive)Redundant
networkIdfield in mock may drift from real typeThe updated config model already uses
chainId; keeping a parallelnetworkIdrisks diverging from production types and adds noise to the fixture.
Consider droppingnetworkIdunless production code explicitly relies on it.packages/ui-components/src/lib/components/dropdown/DropdownActiveSubgraphs.svelte (1)
9-15: 🛠️ Refactor suggestionAdd null-safety around
settings.orderbook.subgraphsThe store may briefly contain an empty object (or even
undefined) during boot.
UsingObject.keysonundefinedwill blow up.-$: dropdownOptions = Object.keys(settings.orderbook.subgraphs ?? {}).reduce( +$: dropdownOptions = Object.keys(settings.orderbook?.subgraphs ?? {}).reduce(packages/webapp/src/routes/vaults/[network]-[id]/+page.svelte (1)
50-55: 🛠️ Refactor suggestionEliminate unsafe
as Hexcasts after adding the null-check- toAddress: orderbookAddress as Hex, + toAddress: orderbookAddress,Same applies to
account: $account as Hex– check for a connected wallet first
and pass the genuineHexvalue without casting.packages/webapp/src/lib/stores/settings.ts (1)
96-107: 🧹 Nitpick (assertive)Validate
SubgraphCfground-tripping
activeSubgraphsis stored as JSON, which is fine as long as every field insideSubgraphCfgis JSON-serialisable.
If a future schema revision introducesBigInt,Map,Set, or class instances, they’ll silently stringify to empty objects or throw.Consider adding a lightweight assert/validator before
JSON.stringifyso mis-shaped objects surface early.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
packages/ui-components/src/__tests__/DeploymentSteps.test.ts(6 hunks)packages/ui-components/src/lib/components/deployment/DeploymentSteps.svelte(3 hunks)packages/ui-components/src/lib/components/detail/VaultDetail.svelte(2 hunks)packages/ui-components/src/lib/components/dropdown/DropdownActiveSubgraphs.svelte(2 hunks)packages/ui-components/src/lib/components/tables/OrdersListTable.svelte(4 hunks)packages/ui-components/src/lib/components/tables/VaultsListTable.svelte(3 hunks)packages/webapp/src/__tests__/resetActiveOrderbookRef.test.ts(2 hunks)packages/webapp/src/lib/services/resetActiveOrderbookRef.ts(1 hunks)packages/webapp/src/lib/stores/settings.ts(2 hunks)packages/webapp/src/routes/vaults/[network]-[id]/+page.svelte(1 hunks)tauri-app/src-tauri/src/commands/config.rs(1 hunks)tauri-app/src/lib/stores/order.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
tauri-app/src/lib/stores/order.ts (2)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1891
File: packages/webapp/src/lib/services/handleAddOrder.ts:94-94
Timestamp: 2025-06-09T11:14:12.358Z
Learning: In the handleAddOrder function in packages/webapp/src/lib/services/handleAddOrder.ts, using an empty string fallback for undefined subgraphUrl is acceptable. When subgraphUrl is missing/empty, transaction indexing cannot be shown to the user, but the deployment functionality still works and users can manually navigate to the order page to view their deployed orders. This graceful degradation approach is preferred over making subgraphUrl required.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1917
File: tauri-app/src/lib/stores/order.ts:10-15
Timestamp: 2025-06-11T11:27:14.379Z
Learning: In this codebase, Svelte writable/derived stores (e.g., `subgraph` in `tauri-app/src/lib/stores/settings.ts`) expose a custom asynchronous `.load()` helper that retrieves the current value, so calls like `await subgraph.load()` are valid.
packages/ui-components/src/lib/components/detail/VaultDetail.svelte (2)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1917
File: tauri-app/src/routes/orders/[network]-[orderHash]/+page.svelte:26-37
Timestamp: 2025-06-11T11:22:24.893Z
Learning: In the tauri-app codebase, the `settings` Svelte store is always initialized with a complete `Config` object whose `orderbook` field (and nested maps such as `orderbooks`, `subgraphs`, and `networks`) is guaranteed to exist, so optional chaining on those paths is unnecessary.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1891
File: packages/webapp/src/lib/services/handleAddOrder.ts:94-94
Timestamp: 2025-06-09T11:14:12.358Z
Learning: In the handleAddOrder function in packages/webapp/src/lib/services/handleAddOrder.ts, using an empty string fallback for undefined subgraphUrl is acceptable. When subgraphUrl is missing/empty, transaction indexing cannot be shown to the user, but the deployment functionality still works and users can manually navigate to the order page to view their deployed orders. This graceful degradation approach is preferred over making subgraphUrl required.
packages/ui-components/src/lib/components/deployment/DeploymentSteps.svelte (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1891
File: packages/webapp/src/lib/services/handleAddOrder.ts:94-94
Timestamp: 2025-06-09T11:14:12.358Z
Learning: In the handleAddOrder function in packages/webapp/src/lib/services/handleAddOrder.ts, using an empty string fallback for undefined subgraphUrl is acceptable. When subgraphUrl is missing/empty, transaction indexing cannot be shown to the user, but the deployment functionality still works and users can manually navigate to the order page to view their deployed orders. This graceful degradation approach is preferred over making subgraphUrl required.
packages/webapp/src/lib/stores/settings.ts (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1917
File: tauri-app/src/routes/orders/[network]-[orderHash]/+page.svelte:26-37
Timestamp: 2025-06-11T11:22:24.893Z
Learning: In the tauri-app codebase, the `settings` Svelte store is always initialized with a complete `Config` object whose `orderbook` field (and nested maps such as `orderbooks`, `subgraphs`, and `networks`) is guaranteed to exist, so optional chaining on those paths is unnecessary.
🧬 Code Graph Analysis (3)
tauri-app/src/lib/stores/order.ts (2)
tauri-app/src/lib/stores/settings.ts (1)
subgraph(108-112)crates/subgraph/src/orderbook_client/mod.rs (1)
url(77-79)
packages/ui-components/src/__tests__/DeploymentSteps.test.ts (1)
packages/ui-components/src/lib/__mocks__/settings.ts (1)
mockConfig(4-68)
packages/webapp/src/lib/stores/settings.ts (2)
tauri-app/src/lib/stores/settings.ts (2)
settings(46-62)activeSubgraphs(152-163)packages/ui-components/src/lib/index.ts (1)
cachedWritableStore(126-126)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: Deploy-Preview
- GitHub Check: test
- GitHub Check: git-clean
- GitHub Check: test
🔇 Additional comments (11)
tauri-app/src-tauri/src/commands/config.rs (2)
5-8: Clean synchronous implementation.The function correctly implements synchronous YAML parsing by delegating to
Config::try_from_yaml. The past review comment about removing the async keyword has been addressed.
10-23: Well-structured dotrain parsing with proper error handling.The function correctly extracts frontmatter from the dotrain document with a safe fallback to an empty string, then delegates to the unified
Config::try_from_yamlmethod. The synchronous implementation addresses the past review comment about removing the async keyword.packages/webapp/src/lib/services/resetActiveOrderbookRef.ts (1)
17-23: Guard against an undefined store value before callingObject.keys()
get(activeNetworkOrderbooksStore)can legally returnundefinedon first load or when the store was reset.
CallingObject.keys(undefined)will throw aTypeError, which is then re-thrown as a generic “Error resetting …” and loses the original context.- if (Object.keys($activeNetworkOrderbooks).length > 0) { + if ($activeNetworkOrderbooks && Object.keys($activeNetworkOrderbooks).length > 0) {This keeps the logic identical while preventing an avoidable exception.
tauri-app/src/lib/stores/order.ts (1)
7-13: Nice extraction ofgetSubgraphUrlhelperMoving the repeated subgraph retrieval into a single helper improves readability and centralises error handling. 👍
packages/ui-components/src/__tests__/DeploymentSteps.test.ts (2)
167-172: Good update to new config pathThe expectation now matches the nested
orderbook.subgraphs.mainnet.urlstructure – test remains meaningful. 👍
458-462: Correct argument propagation for non-mainnet deploymentVerifying the
flareURL guards against hard-coding a single network in tests – nice coverage improvement.packages/ui-components/src/lib/components/tables/VaultsListTable.svelte (2)
41-44: Efficient derivation ofmultiSubgraphArgsUsing a helper keeps template logic slim and the
enabledflag now prevents wasteful queries when no subgraphs are available. Good refactor.
78-79: Query fires only when data is available – nice touchThe new condition avoids the earlier GraphQL 400s. 👍
packages/ui-components/src/lib/components/deployment/DeploymentSteps.svelte (1)
75-76:⚠️ Potential issuePossible null-dereference on
orderbook– add defensive optional chaining
$settings.orderbookis accessed without verifying that the async settings store
has been populated. On a cold start the component mounts with an empty config,
leading toCannot read properties of undefined (reading 'subgraphs').- subgraphUrl = $settings.orderbook.subgraphs[value]?.url ?? ''; + subgraphUrl = $settings.orderbook?.subgraphs?.[value]?.url ?? '';This keeps the graceful-degradation behaviour (empty string) while removing the crash path.
⛔ Skipped due to learnings
Learnt from: findolor PR: rainlanguage/rain.orderbook#1917 File: tauri-app/src/routes/orders/[network]-[orderHash]/+page.svelte:26-37 Timestamp: 2025-06-11T11:22:24.893Z Learning: In the tauri-app codebase, the `settings` Svelte store is always initialized with a complete `Config` object whose `orderbook` field (and nested maps such as `orderbooks`, `subgraphs`, and `networks`) is guaranteed to exist, so optional chaining on those paths is unnecessary.packages/webapp/src/lib/stores/settings.ts (2)
24-29: Docstring now matches reality – nice catch
The@defaultannotation was updated toEMPTY_CONFIG, eliminating the previous mismatch.
30-40: YAML vs JSON: confirm we’re not drifting from the new config standardThe surrounding refactor switched the canonical on-disk format to YAML (see
tauri-app/src/lib/stores/settings.ts, which usesparseYaml).
Here we still serialize/deserialize withJSON.stringify/JSON.parse.Please double-check that this is intentional. If the webapp is expected to share the same settings blob as the tauri app (or future clients), persisting JSON here will break cross-client portability.
If we do want YAML everywhere, the change is minimal:
- (value) => JSON.stringify(value), - (str) => { - try { - return JSON.parse(str) as Config; - } catch { - return EMPTY_CONFIG; - } - }, + (value) => yamlStringify(value), + (str) => { + const res = parseYaml([str]); + return res.error ? EMPTY_CONFIG : res.value; + },(using whatever helpers you already wired up).
| $: multiSubgraphArgs = getMultiSubgraphArgs( | ||
| Object.keys($activeSubgraphs).length > 0 ? $activeSubgraphs : $settings.orderbook.subgraphs | ||
| ); | ||
|
|
There was a problem hiding this comment.
Guard multiSubgraphArgs against undefined and always normalise to an array
getMultiSubgraphArgs is assumed to always return an array, but its contract isn’t enforced here.
If the helper is ever refactored to return undefined/null (e.g. when no subgraphs are configured) the subsequent .length access and the enabled flag will explode at runtime.
-$: multiSubgraphArgs = getMultiSubgraphArgs(
- Object.keys($activeSubgraphs).length > 0 ? $activeSubgraphs : $settings.orderbook.subgraphs
- );
+$: multiSubgraphArgs =
+ getMultiSubgraphArgs(
+ Object.keys($activeSubgraphs).length > 0
+ ? $activeSubgraphs
+ : $settings.orderbook.subgraphs,
+ ) ?? [];📝 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.
| $: multiSubgraphArgs = getMultiSubgraphArgs( | |
| Object.keys($activeSubgraphs).length > 0 ? $activeSubgraphs : $settings.orderbook.subgraphs | |
| ); | |
| $: multiSubgraphArgs = | |
| getMultiSubgraphArgs( | |
| Object.keys($activeSubgraphs).length > 0 | |
| ? $activeSubgraphs | |
| : $settings.orderbook.subgraphs, | |
| ) ?? []; |
🤖 Prompt for AI Agents
In packages/ui-components/src/lib/components/tables/OrdersListTable.svelte
around lines 42 to 45, ensure that the result of getMultiSubgraphArgs is always
an array to prevent runtime errors. After calling getMultiSubgraphArgs, check if
the result is undefined or null, and if so, assign an empty array instead. This
guarantees that multiSubgraphArgs is always an array and safe to use with
.length and other array operations.
There was a problem hiding this comment.
that function always returns an array
There was a problem hiding this comment.
@findolor Got it—thanks for confirming that getMultiSubgraphArgs is guaranteed to return an array. I’ll mark my earlier concern as resolved and won’t push the extra guard.
| enabled: multiSubgraphArgs.length > 0 | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
enabled flag can crash when multiSubgraphArgs is falsy
After the previous guard, also bullet-proof the flag:
- enabled: multiSubgraphArgs.length > 0
+ enabled: (multiSubgraphArgs?.length ?? 0) > 0📝 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.
| enabled: multiSubgraphArgs.length > 0 | |
| }); | |
| enabled: (multiSubgraphArgs?.length ?? 0) > 0 | |
| }); |
🤖 Prompt for AI Agents
In packages/ui-components/src/lib/components/tables/OrdersListTable.svelte
around lines 80 to 81, the enabled flag is set using multiSubgraphArgs.length
without checking if multiSubgraphArgs is truthy, which can cause a crash if it
is null or undefined. Fix this by adding a check to ensure multiSubgraphArgs is
truthy before accessing its length, for example by using a condition like
multiSubgraphArgs && multiSubgraphArgs.length > 0.
There was a problem hiding this comment.
no need to add this
There was a problem hiding this comment.
@findolor Understood—no changes will be made to the enabled flag. Thanks for clarifying!
| items.reduce( | ||
| (acc, key) => ({ ...acc, [key]: (settings.orderbook.subgraphs ?? {})[key] }), | ||
| {} as Record<string, SubgraphCfg> | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid repeating expensive optional-chained look-ups
Minor readability / perf nit: cache the subgraphs map instead of dereferencing
settings.orderbook.subgraphs three times in the same expression.
- items.reduce(
- (acc, key) => ({ ...acc, [key]: (settings.orderbook.subgraphs ?? {})[key] }),
+ const subgraphs = settings.orderbook?.subgraphs ?? {};
+ items.reduce((acc, key) => ({ ...acc, [key]: subgraphs[key] }),📝 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.
| items.reduce( | |
| (acc, key) => ({ ...acc, [key]: (settings.orderbook.subgraphs ?? {})[key] }), | |
| {} as Record<string, SubgraphCfg> | |
| ) | |
| const subgraphs = settings.orderbook?.subgraphs ?? {}; | |
| items.reduce((acc, key) => ({ ...acc, [key]: subgraphs[key] }), | |
| {} as Record<string, SubgraphCfg> | |
| ) |
🤖 Prompt for AI Agents
In
packages/ui-components/src/lib/components/dropdown/DropdownActiveSubgraphs.svelte
around lines 20 to 23, the expression repeatedly accesses
settings.orderbook.subgraphs multiple times within the reduce function, which is
inefficient. To fix this, assign settings.orderbook.subgraphs (or an empty
object if undefined) to a local variable before the reduce call, then reference
this variable inside the reduce callback to avoid redundant optional chaining
and improve readability and performance.
There was a problem hiding this comment.
no need to do this
| const subgraphUrl = $settings.orderbook.subgraphs[network]?.url || ''; | ||
| const chainId = $settings.orderbook.networks[network]?.chainId || 0; | ||
| const orderbookAddress = $settings.orderbook.orderbooks[network]?.address ?? ''; | ||
| const rpcUrl = $settings.orderbook.networks[network]?.rpc || ''; | ||
| const { account } = useAccount(); |
There was a problem hiding this comment.
orderbookAddress/rpcUrl may be empty yet still cast to Hex – validate first
Both values default to an empty string when missing.
Down-stream helpers (handleVaultDeposit/Withdraw) assume a valid 0x-prefixed
address / RPC URL. Passing '' as Hex silently bypasses type-safety and produces
hard-to-trace failures.
-const orderbookAddress = $settings.orderbook.orderbooks[network]?.address ?? '';
+const orderbookAddress =
+ $settings.orderbook.orderbooks[network]?.address ??
+ DeploymentStepsError.throw(`Orderbook address missing for network '${network}'`);(Use whatever local error helper you prefer; the key point is fail fast.)
Remove the as Hex casts once the value is guaranteed to be correct; the compiler
will be satisfied.
📝 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 subgraphUrl = $settings.orderbook.subgraphs[network]?.url || ''; | |
| const chainId = $settings.orderbook.networks[network]?.chainId || 0; | |
| const orderbookAddress = $settings.orderbook.orderbooks[network]?.address ?? ''; | |
| const rpcUrl = $settings.orderbook.networks[network]?.rpc || ''; | |
| const { account } = useAccount(); | |
| const subgraphUrl = $settings.orderbook.subgraphs[network]?.url || ''; | |
| const chainId = $settings.orderbook.networks[network]?.chainId || 0; | |
| - const orderbookAddress = $settings.orderbook.orderbooks[network]?.address ?? ''; | |
| + const orderbookAddress = | |
| + $settings.orderbook.orderbooks[network]?.address ?? | |
| + DeploymentStepsError.throw( | |
| + `Orderbook address missing for network '${network}'` | |
| + ); | |
| const rpcUrl = $settings.orderbook.networks[network]?.rpc || ''; | |
| const { account } = useAccount(); |
🤖 Prompt for AI Agents
In packages/webapp/src/routes/vaults/[network]-[id]/+page.svelte around lines 18
to 22, the variables orderbookAddress and rpcUrl default to empty strings when
missing, but are later cast to Hex type without validation, which can cause
silent failures. To fix this, add validation checks to ensure these values are
non-empty and valid before casting to Hex. If validation fails, use a local
error helper to fail fast with a clear error message. Remove the 'as Hex' casts
until the values are confirmed valid so the compiler enforces type safety.
There was a problem hiding this comment.
that error will be surfaced in the wrapped functions so it is safe to leave these variables as it is
There was a problem hiding this comment.
@findolor Got it—thanks for clarifying. I’ll keep in mind that validation for these values happens inside the helper functions, so there’s no need for additional checks at the call-site.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/webapp/src/__tests__/resetActiveOrderbookRef.test.ts (1)
10-10:keystill duplicatesaddress– acknowledged prior feedback
Previous rounds already covered that duplicatingkey=addresscan be confusing ifkeyis meant to be a human-readable identifier. The author explicitly opted to keep it, so no action required here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/webapp/src/__tests__/resetActiveOrderbookRef.test.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (1)
packages/webapp/src/__tests__/resetActiveOrderbookRef.test.ts (1)
12-19: 🧹 Nitpick (assertive)Hard-coded RPC/Subgraph endpoints risk leaking to production
Even though this is test code, literals such as an Infura URL placeholder and a fake subgraph URL occasionally get copied into non-test fixtures. Consider replacing them with obvious dummy strings (e.g."http://localhost"or"DUMMY_URL") to avoid accidental usage outside tests.- rpc: 'https://mainnet.infura.io/v3/your-infura-project-id' + rpc: 'http://localhost/DUMMY_RPC' ... - url: 'https://api.subgraph.com/subgraphs/name/your-subgraph-name/your-subgraph-id' + url: 'http://localhost/DUMMY_SUBGRAPH'⛔ Skipped due to learnings
Learnt from: hardingjam PR: rainlanguage/rain.orderbook#1700 File: tauri-app/src/lib/mocks/mockConfigSource.ts:6-6 Timestamp: 2025-04-28T10:58:11.124Z Learning: In mock data for testing in this codebase, it's acceptable to use URL placeholders like 'https://mainnet.infura.io/v3/YOUR-PROJECT-ID' as they clearly indicate the expected format for actual implementation.
| mockActiveNetworkOrderbooksStore = writable<Record<string, OrderbookCfg> | null>( | ||
| null | ||
| ) as AppStoresInterface['activeNetworkOrderbooks']; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Type-safe writable initialisation
Casting to AppStoresInterface['activeNetworkOrderbooks'] hides a real type mismatch.
Initialise the store with the correct union type instead.
-mockActiveNetworkOrderbooksStore = writable<Record<string, OrderbookCfg> | null>(
- null
-) as AppStoresInterface['activeNetworkOrderbooks'];
+mockActiveNetworkOrderbooksStore =
+ writable<Record<string, OrderbookCfg> | null>(null);📝 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.
| mockActiveNetworkOrderbooksStore = writable<Record<string, OrderbookCfg> | null>( | |
| null | |
| ) as AppStoresInterface['activeNetworkOrderbooks']; | |
| mockActiveNetworkOrderbooksStore = | |
| writable<Record<string, OrderbookCfg> | null>(null); |
🤖 Prompt for AI Agents
In packages/webapp/src/__tests__/resetActiveOrderbookRef.test.ts around lines 50
to 52, the writable store is being cast to
AppStoresInterface['activeNetworkOrderbooks'], which hides a type mismatch. To
fix this, initialize the writable store directly with the correct union type
that matches AppStoresInterface['activeNetworkOrderbooks'] instead of using a
type cast. This ensures type safety and prevents potential runtime errors.
There was a problem hiding this comment.
this is valid for test cases
There was a problem hiding this comment.
@findolor Got it—leaving the explicit cast in place for the test context makes sense. Thanks for clarifying!
| const orderbooks: Record<string, OrderbookCfg> = { | ||
| orderbook1: createMockOrderbookConfig('0x123', 'Orderbook One'), | ||
| orderbook2: createMockOrderbookConfig('0x456', 'Orderbook Two') | ||
| }; | ||
| mockActiveNetworkOrderbooksStore = writable<Record<string, OrderbookConfigSource>>(orderbooks); | ||
| mockActiveNetworkOrderbooksStore = writable<Record<string, OrderbookCfg>>(orderbooks); | ||
| resetActiveOrderbookRef(mockActiveOrderbookRef, mockActiveNetworkOrderbooksStore); | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const calledValue = (mockActiveOrderbookRef.set as any).mock.calls[0][0]; | ||
| expect(Object.keys(orderbooks)).toContain(calledValue); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid as any by using the spy reference directly
vi.spyOn already returns a typed mock; casting to any forfeits type-safety.
-const calledValue = (mockActiveOrderbookRef.set as any).mock.calls[0][0];
+const setSpy = vi.spyOn(mockActiveOrderbookRef, 'set');
+const calledValue = setSpy.mock.calls[0][0];Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/webapp/src/__tests__/resetActiveOrderbookRef.test.ts around lines 32
to 40, avoid using 'as any' to cast the mockActiveOrderbookRef.set call.
Instead, directly use the typed mock returned by vi.spyOn to access the
mock.calls array, preserving type safety. Replace the cast with proper usage of
the spy's typed mock properties.
There was a problem hiding this comment.
this is valid for test cases
There was a problem hiding this comment.
@findolor Got it—happy to leave the as any cast in the tests. I’ll avoid flagging this pattern in future test-only code.
✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1917
File: packages/webapp/src/__tests__/resetActiveOrderbookRef.test.ts:32-40
Timestamp: 2025-06-11T12:39:28.598Z
Learning: In this repository, the maintainer (findolor) is fine with using `as any` casts inside test files; such casts should not be flagged as issues.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
We will be creating new issues to remove config source and config structs from the repo. This PR is closed because of this |
1 participant
Motivation
See issue: #1364
Config source is not used anymore for the yaml parsing. It should be removed from the repository to migrate to the new parsing logic.
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
fix #1364
Summary by CodeRabbit
New Features
orderbookanddotrainOrder.Refactor
Configtype and nested access patterns.Bug Fixes