Update gui sdk to use additional yaml settings with dotrain#2338
Update gui sdk to use additional yaml settings with dotrain#2338
Conversation
WalkthroughThis PR refactors the DotrainOrderGui API to accept optional YAML settings documents alongside the main dotrain string, enabling flexible configuration merging. It introduces token_key tracking throughout config structures, implements dotrain_hash for state serialization, and updates YAML parsing to support select-token-aware contexts. All callers and tests are updated to accommodate the new parameter positions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
crates/settings/src/scenario.rs (1)
80-86: Scenario binding interpolation now uses select‑token–aware contextSwitching both interpolation sites to
context.interpolate_with_select_tokens(...)makes scenario bindings consistent with the newer select‑token semantics and should remain backward compatible as long as that helper defers to the old behavior when no select tokens are configured. Please ensure there is (or add) a test where scenario bindings depend on select‑token‑backed IO so regressions in this path are caught early.Also applies to: 103-109
crates/settings/src/order.rs (1)
436-494: I'll help you verify this review comment about theparse_vault_idsfunction incrates/settings/src/order.rs. Let me start by examining the function and its context more closely.
<function_calls>
#!/bin/bashGet the full context of the function and surrounding code
cat -n crates/settings/src/order.rs | sed -n '430,500p'
</function_calls>
#!/bin/bashSearch for how YamlError is defined and what variants exist
rg -n "enum YamlError" crates/settings/src -A 20
</function_calls>
#!/bin/bashSearch for NotFound error usage in the codebase
rg -n "YamlError::NotFound" crates/settings/src -B 2 -A 2
</function_calls>
<function_calls>
#!/bin/bashLook for tests of parse_vault_ids function
rg -n "parse_vault_ids" crates/settings/src -B 2 -A 5
</function_calls>
#!/bin/bashCheck the helper functions used in parse_vault_ids
rg -n "fn require_hash|fn require_vec|fn optional_string|fn require_string" crates/settings/src -A 10
</function_calls>Now let me check the test file to understand how this function is tested:
<function_calls>
#!/bin/bashFind test files related to order.rs
find crates/settings -name "test" -o -name "*.rs" | xargs grep -l "parse_vault_ids"
</function_calls>
<function_calls>
#!/bin/bashCheck if there are integration tests or tests in the same file
rg -n "#[test]|#[tokio::test]" crates/settings/src/order.rs -A 20 | grep -A 20 "vault"
</function_calls>Let me also look at where this function is called to understand its usage:
<function_calls>
#!/bin/bashFind all callers of parse_vault_ids
rg -n "parse_vault_ids" crates/ --type rust -B 3 -A 3
</function_calls>Let me check what the actual error should be when an order is not found:
<function_calls>
#!/bin/bashLook at the complete order.rs file around the function to understand the logic better
wc -l crates/settings/src/order.rs
</function_calls>
<function_calls>
#!/bin/bashGet more context around the function to see imports and error types
cat -n crates/settings/src/order.rs | head -50
</function_calls>Now let me examine the error handling pattern more carefully:
<function_calls>
#!/bin/bashLook at the actual error enum definition in yaml module
cat -n crates/settings/src/yaml/mod.rs | head -100
</function_calls>Let me trace through the logic step by step:
<function_calls>
#!/bin/bashGet the actual structure of YamlError
rg -n "pub enum YamlError|#[error" crates/settings/src/yaml/mod.rs -A 3 | head -60
</function_calls>Now I'll run the verification script provided in the review comment:
<function_calls>
#!/bin/bashRun cargo fmt to check formatting
cd /tmp && git clone https://github.com/rainlanguage/rain.orderbook.git 2>&1 | tail -5
cd rain.orderbook && nix develop -c cargo fmt --all 2>&1 | head -20
</function_calls>Let me check if the repository has a flake.nix or if I need to check the build setup differently:
<function_calls>
#!/bin/bashCheck the repo structure and if nix is available
ls -la crates/settings/src/order.rs
Also check if we can inspect the code directly without building
Let me get a clearer picture of the function logic and error handling:
<function_calls>
#!/bin/bashRead the exact function implementation from the file
sed -n '436,494p' crates/settings/src/order.rs
</function_calls><function_calls>
#!/bin/bashFind similar patterns where order_key or other missing keys are checked
rg -n "NotFound|KeyNotFound" crates/settings/src -B 2 -A 2 | head -60
</function_calls>
<function_calls>
#!/bin/bashLook at the require_hash function to understand what error it returns
sed -n '1,200p' crates/settings/src/order.rs | grep -A 20 "fn require_hash"
</function_calls>Let me search more broadly for the helper functions:
<function_calls>
#!/bin/bashFind where require_hash is defined
rg -n "pub fn require_hash|fn require_hash" crates/settings/src -A 8
</function_calls>Now let me understand the complete control flow by checking what require_hash does:
<function_calls>
#!/bin/bashSearch for require_hash definition in the entire crates directory
rg -n "require_hash" crates/settings/src/yaml/ -A 10
</function_calls>Let me look at the yaml module structure:
<function_calls>
#!/bin/bashList files in yaml directory
ls -la crates/settings/src/yaml/
</function_calls>
<function_calls>
#!/bin/bashFind require_hash in util file
grep -n "require_hash" crates/settings/src/yaml/util.rs -A 15
</function_calls>Perfect! Now I can see the complete picture. Let me verify the actual behavior by examining one more thing:
<function_calls>
#!/bin/bashCheck what require_hash returns when key doesn't exist
cat -n crates/settings/src/yaml/util.rs | sed -n '1,100p'
</function_calls>Now let me search for test cases related to this function:
<function_calls>
#!/bin/bashSearch for tests in order.rs or related test files
rg -n "#[test]" crates/settings/src/order.rs -A 30 | head -100
</function_calls>Let me check the yaml module's test files:
<function_calls>
#!/bin/bashFind test files
find crates/settings -name "*.rs" -type f -exec grep -l "parse_vault_ids" {} ;
</function_calls>Let me now examine the actual logic flow more carefully to understand when vault_ids would be empty:
<function_calls>
#!/bin/bashShow the complete parse_vault_ids function one more time with analysis
cat > /tmp/analyze.py << 'EOF'
Analyze the parse_vault_ids function logic
logic = """
The function parse_vault_ids does:
- Iterate through documents
- Try to get orders hash - if fail, continue (Ok wrapping means errors are silently ignored!)
- If orders is not a hash/map, the require_hash would return Err
- If orders IS a hash, look for order_key in it
- If order_key found, extract vault data
- If order_key NOT found, vault_ids stays unchanged
The key issue:
- If NO document has an "orders" key: vault_ids is empty (loop does nothing)
- If "orders" is not a map: vault_ids is empty (Err is silently consumed by Ok())
- If "orders" IS a map but order_key not in it: vault_ids is empty
When vault_ids is empty, we return InvalidType for "orders"
This is MISLEADING in case 3 (order_key not found in valid orders map)
"""print(logic)
EOF
python /tmp/analyze.py
</function_calls>Now let me verify the actual behavior by looking at how the Ok wrapper works:
<function_calls>
#!/bin/bashGet the exact lines around the require_hash call
sed -n '436,460p' crates/settings/src/order.rs
</function_calls>Based on my thorough analysis of the code, I can now provide a definitive verification of the review comment.
Let me check one more thing about error variants to be completely sure:
<function_calls>
#!/bin/bashGet the complete YamlError enum definition
sed -n '1,100p' crates/settings/src/yaml/mod.rs | grep -A 50 "pub enum YamlError"
</function_calls>Perfect. Now I have all the information I need to rewrite the review comment.
parse_vault_idsreturns a misleading error when an order key is missingThe function silently skips documents where
require_hash()fails (line 445:if let Ok(...)), so when theorderskey is absent or not a map,vault_idsremains empty and triggers theInvalidTypeerror fororders. However, whenordersis a valid map but theorder_keyis simply not present (line 450:orders_hash.get()returnsNone), the same misleading error is returned.To distinguish these cases, track whether any document had a valid
ordersmap:
- If no valid
ordersmap was found, preserve the currentInvalidTypeerror- If
orderswas found butvault_idsis empty, returnYamlError::NotFound(order_key.to_string())insteadcrates/js_api/src/registry.rs (1)
284-296: Registry now cleanly threads shared settings into all GUI/detail calls
DotrainRegistryno longer concatenates settings and order content itself; instead,settings_sources()produces anOption<Vec<String>>that is passed alongside each order’s dotrain content intoDotrainOrderGui::get_order_details,get_deployment_details,new_from_state, andnew_with_deployment. This centralizes the settings handling, keeps behavior consistent across methods, and delegates the actual merge logic toDotrainOrderGui. The only behavior distinction is that an empty settings file now results inNone(no extra documents) rather than a single empty YAML document, which is a reasonable simplification.Also applies to: 346-362, 405-471, 474-481
crates/settings/src/gui.rs (2)
274-301: Includeshort_descriptioninGuiDeploymentCfgequality check
GuiDeploymentCfg::PartialEqcurrently ignores the newshort_descriptionfield, so two deployments that differ only byshort_descriptionwill compare equal. That’s surprising for a metadata field that is surfaced to the UI.Consider extending
eqto includeshort_description:impl PartialEq for GuiDeploymentCfg { fn eq(&self, other: &Self) -> bool { self.key == other.key && self.deployment == other.deployment && self.name == other.name && self.description == other.description + && self.short_description == other.short_description && self.deposits == other.deposits && self.fields == other.fields && self.select_tokens == other.select_tokens } }
639-640: Revisit swallowingTokenCfg::parse_all_from_yamlerrors for GUI deposits
TokenCfg::parse_all_from_yamlis now called without?, and any error is silently ignored when resolving deposit tokens:
- If token parsing fails (e.g. malformed or missing
tokenssection), GUI parsing will still succeed, butGuiDepositCfg.tokenwill beNoneand later consumers may fail in less obvious ways.- Previously (assuming there was a
?), such YAML issues would surface as clearYamlErrors at parse time.If the intention is to make
GuiDepositCfg.tokentruly optional while still surfacing structural YAML errors, consider:
- Continuing to propagate structural
YamlErrors fromparse_all_from_yaml, and- Only treating a missing concrete token key as optional (i.e.
HashMap::getreturningNone).For example:
- let tokens = TokenCfg::parse_all_from_yaml(documents.clone(), context); + let tokens = TokenCfg::parse_all_from_yaml(documents.clone(), context)?; ... - if let Ok(tokens) = &tokens { - let token = tokens.get(&token_key); - deposit_token = token.map(|token| Arc::new(token.clone())); - } + if let Some(token) = tokens.get(&token_key) { + deposit_token = Some(Arc::new(token.clone())); + }Please confirm whether silently ignoring token‑parsing errors is intended here.
Also applies to: 775-787, 810-815, 1088-1093, 253-260
crates/js_api/src/gui/state_management.rs (1)
439-477: Remove leftover debug logging from WASM test
wasm_bindgen_test::console_log!("{}", SERIALIZED_STATE);intest_serialize_stateproduces noisy output every test run and doesn't contribute to assertions.Consider removing it to keep tests quiet and focused.
crates/js_api/src/gui/mod.rs (2)
411-417: Docstring contradicts parser:gui.short-descriptionis required, not optionalThe comment for
getOrderDetailssays:
gui.short-description- Brief summary (optional but recommended)But
GuiCfg::parse_order_detailsnow callsget_hash_value("short-description", ...)andrequire_string, makingshort-descriptionrequired and emitting aMissing("short-description")error (as asserted in tests).To avoid confusing consumers of the JS API, update the docstring to state that
gui.short-descriptionis required.
2058-2093: Updatenew_with_deploymentcall inselect_token_teststo match new signatureIn
select_token_tests::test_get_token_info_remote,DotrainOrderGui::new_with_deploymentis still called with the old 3‑argument signature:let mut gui = DotrainOrderGui::new_with_deployment( yaml.to_string(), "some-deployment".to_string(), None, ) .await .unwrap();But
new_with_deploymentis now defined as:pub async fn new_with_deployment( dotrain: String, settings: Option<Vec<String>>, selected_deployment: String, state_update_callback: Option<js_sys::Function>, )This will not compile — the
"some-deployment".to_string()argument is being passed whereOption<Vec<String>>is expected.Adjust the call to include the new
settingsparameter explicitly, e.g.:- let mut gui = DotrainOrderGui::new_with_deployment( - yaml.to_string(), - "some-deployment".to_string(), - None, - ) + let mut gui = DotrainOrderGui::new_with_deployment( + yaml.to_string(), + None, + "some-deployment".to_string(), + None, + )
| pub fn get_yaml_documents( | ||
| dotrain: &str, | ||
| settings: Option<Vec<String>>, | ||
| ) -> Result<Vec<Arc<RwLock<StrictYaml>>>, GuiError> { | ||
| let frontmatter = RainDocument::get_front_matter(&dotrain) | ||
| .unwrap_or("") | ||
| .to_string(); | ||
| let dotrain_yaml = | ||
| DotrainYaml::new(vec![frontmatter.clone()], DotrainYamlValidation::default())?; | ||
| let mut sources = vec![frontmatter]; | ||
| if let Some(settings) = settings { | ||
| sources.extend(settings); | ||
| } | ||
|
|
||
| let dotrain_yaml = DotrainYaml::new(sources, DotrainYamlValidation::default())?; | ||
| Ok(dotrain_yaml.documents) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reuse get_yaml_documents elsewhere to avoid duplicated frontmatter/settings parsing
You’ve centralized frontmatter + settings merging in get_yaml_documents, which is good. There may be further opportunities to reuse this helper (or a closely related one) when parsing deployment keys or GUI metadata in other modules instead of re‑implementing frontmatter extraction there.
Not urgent, but worth considering to keep YAML sourcing logic single‑sourced.
🤖 Prompt for AI Agents
In crates/js_api/src/gui/mod.rs around lines 644–657, other modules reimplement
frontmatter and settings merging instead of reusing get_yaml_documents; extract
or expose the merged-sources logic so callers can reuse it: either make a small
public/helper function that returns the Vec<String> sources (frontmatter +
settings) or change get_yaml_documents into two functions (one that returns
sources and one that returns documents) and update places that parse deployment
keys or GUI metadata to call the new helper; ensure the helper has appropriate
visibility (pub or pub(crate)) and update imports/usages so all code builds.
There was a problem hiding this comment.
not needed now. we already use this in many places
There was a problem hiding this comment.
@findolor, understood! Thanks for confirming that the get_yaml_documents helper is already being reused appropriately across the codebase.
🧠 Learnings used
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1983
File: crates/js_api/src/gui/mod.rs:964-1199
Timestamp: 2025-07-11T06:40:49.511Z
Learning: In crates/js_api/src/gui/mod.rs tests, findolor prefers to keep large hardcoded YAML strings (like the validation test YAML) inline in the test file rather than extracting them to separate external files for maintainability purposes.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1956
File: crates/common/src/raindex_client/add_orders.rs:132-132
Timestamp: 2025-07-04T10:24:29.750Z
Learning: In the rain.orderbook codebase, findolor prefers to use `unwrap_or("")` for optional frontmatter in dotrain parsing, allowing empty strings to be handled by downstream validation in `NewConfig::try_from_yaml()` rather than adding explicit early validation checks at the extraction point.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1474
File: crates/js_api/src/yaml/mod.rs:37-44
Timestamp: 2025-03-31T14:36:11.049Z
Learning: The OrderbookYaml implementation in crates/js_api/src/yaml/mod.rs intentionally parses YAML on demand without caching results. This is a deliberate design choice by the author to process YAML only when needed rather than optimizing for repeated calls.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2012
File: crates/js_api/src/registry.rs:485-512
Timestamp: 2025-07-30T07:41:39.271Z
Learning: In crates/js_api/src/registry.rs, findolor considers the current concurrent fetching of order files using futures::future::join_all without concurrency limits to be acceptable, preferring the simple approach over adding concurrency limiting mechanisms for the DotrainRegistry implementation.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1474
File: crates/js_api/src/yaml/mod.rs:31-34
Timestamp: 2025-03-31T13:57:59.660Z
Learning: The OrderbookYaml constructor in crates/js_api/src/yaml/mod.rs does not need early YAML validation. The author prefers to validate YAML only when it's actually used rather than during initialization.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1477
File: crates/settings/src/yaml/dotrain.rs:20-20
Timestamp: 2025-03-31T18:53:12.208Z
Learning: In the DotrainYaml struct, the cache field is kept public (`pub cache: Cache`) without additional getter methods, which is consistent with the existing design pattern in the codebase where other fields like `documents` are also public.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2334
File: crates/settings/src/yaml/orderbook.rs:18-18
Timestamp: 2025-11-25T14:56:29.762Z
Learning: In crates/settings/src/yaml/orderbook.rs, the `accounts` section is optional in orderbook YAML. The `AccountCfg::parse_all_from_yaml` method returns an empty HashMap when the `accounts` section is missing rather than throwing a Field(Missing(...)) error, so it doesn't need to be wrapped with `to_yaml_string_missing_check` in the `to_yaml_string` method.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1687
File: crates/js_api/src/gui/mod.rs:258-259
Timestamp: 2025-04-22T12:50:39.581Z
Learning: In `DotrainOrderGui::generate_dotrain_text()`, the call to `.to_string()` on `self.dotrain_order.dotrain()` is necessary because there are two different implementations of the `dotrain()` method - one for WASM targets returning a String and one for non-WASM targets returning a &str. The `.to_string()` ensures type compatibility across different compilation targets.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1938
File: crates/settings/src/yaml/mod.rs:176-178
Timestamp: 2025-06-18T19:23:33.747Z
Learning: In crates/settings/src/yaml/mod.rs, the YamlError enum has two distinct error variants: `KeyNotFound(String)` for when a specific YAML key is not found in a hash/map, and `NotFound(String)` for when other types of entities (like networks, orderbooks, etc.) are not found in the configuration. These serve different purposes and should not be consolidated.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2163
File: crates/js_api/src/gui/state_management.rs:196-203
Timestamp: 2025-09-24T10:59:25.666Z
Learning: In crates/js_api/src/gui/state_management.rs, the RainMetaDocumentV1Item::hash(false) method returns a slice, so wrapping the result with FixedBytes() is necessary to convert it to the expected FixedBytes type for DotrainGuiStateV1.dotrain_hash field.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1483
File: crates/settings/src/yaml/dotrain.rs:20-20
Timestamp: 2025-04-07T09:52:30.960Z
Learning: In the DotrainYaml struct, the newly added `cache` field is kept public without documentation, which the maintainer confirmed is acceptable for now.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR #1884 in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR #1884 in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR #1884 in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR #1884 in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T22:14:22.646Z
Learning: For PR #1884 in rainlanguage/rain.orderbook, I must consistently report the PR size after EVERY new commit in the exact format "TOTAL=number\nADDITIONS=number\nDELETIONS=number" without any additional text or formatting blocks. The report must exclude lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock) and can be included either as part of the review or as a separate comment.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T22:14:22.646Z
Learning: For PR #1884 in rainlanguage/rain.orderbook, I must consistently report the PR size after EVERY new commit in the exact format "TOTAL=number\nADDITIONS=number\nDELETIONS=number" without any formatting blocks. The report must exclude lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock) and can be included either as part of the review or as a separate comment.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:25:47.873Z
Learning: After each new commit review for rainlanguage/rain.orderbook repository, generate a PR size report in a separate comment with the format: "TOTAL=number ADDITIONS=number DELETIONS=number". This report should exclude irrelevant files such as lock files (e.g., package-lock.json, cargo.lock).
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:26:31.566Z
Learning: For PR #1884 and future PRs in rainlanguage/rain.orderbook repository, provide PR size reports in a separate comment with ONLY the exact format "TOTAL=number\nADDITIONS=number\nDELETIONS=number", without any additional text or explanation. Reports should exclude irrelevant files such as lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock).
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:25:47.873Z
Learning: After each new commit review for rainlanguage/rain.orderbook repository, generate a PR size report in a separate comment with the exact format: "TOTAL=number\nADDITIONS=number\nDELETIONS=number". This report should exclude irrelevant files such as lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock).
| let short_description = get_hash_value_as_option(gui, "short-description") | ||
| .map(|v| { | ||
| v.as_str().ok_or(YamlError::Field { | ||
| kind: FieldErrorKind::InvalidType { | ||
| field: "description".to_string(), | ||
| expected: "a string".to_string(), | ||
| }, | ||
| location: "gui".to_string(), | ||
| }) | ||
| }) | ||
| .transpose()?; |
There was a problem hiding this comment.
Fix field name in short-description type error
In GuiCfg::parse_from_yaml_optional, the invalid‑type error for short-description uses field: "description" instead of "short-description". This will mislead users when they provide a non‑string short-description.
Update the error construction to report "short-description" as the field name.
🤖 Prompt for AI Agents
In crates/settings/src/gui.rs around lines 665 to 675, the YAML type-error for
the short-description field incorrectly reports field: "description"; change the
constructed YamlError::Field so the FieldErrorKind::InvalidType uses field:
"short-description" (keep the rest of the error structure the same) so
non-string short-description values produce an accurate error message.
| if let Some(context) = context { | ||
| if context.select_tokens.is_none() && tokens.is_err() { | ||
| return Err(tokens.err().unwrap()); | ||
| if context.select_tokens.is_some() { | ||
| match tokens { | ||
| Ok(_) => {} | ||
| Err(YamlError::Field { | ||
| kind: FieldErrorKind::Missing(_), | ||
| .. | ||
| }) => {} | ||
| Err(err) => return Err(err), | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Token parsing relaxation under select_tokens context may hide some field-level errors
When context.select_tokens is set, any YamlError::Field with FieldErrorKind::Missing(_) from TokenCfg::parse_all_from_yaml is now ignored, not just the “tokens section missing” case. That means missing required fields inside tokens: (like an absent address) will surface later as a generic “missing yaml data for token '...’” from order parsing rather than a precise token‑level error.
If the intent is only to tolerate a completely missing tokens section for select‑token UIs, consider narrowing the match to the specific "tokens"/"root" missing case so genuine token definition errors still report accurately.
🤖 Prompt for AI Agents
In crates/settings/src/order.rs around lines 543 to 552, the current match
ignores any YamlError::Field with FieldErrorKind::Missing(_ ) when
context.select_tokens is set, which suppresses real missing-field errors inside
tokens; change the match to only ignore the specific missing-"tokens" (or root
tokens section) case: match the Missing(field) and check the field name/path
equals "tokens" (or root path) and only then skip the error, otherwise return
Err(err) so genuine token-field errors (like missing address) propagate with
their precise token-level error.
| #[wasm_export( | ||
| param_description = "Optional additional YAML configuration strings to merge with the frontmatter" | ||
| )] | ||
| settings: Option<Vec<String>>, |
There was a problem hiding this comment.
didn't we already agree not to call this "settings" anymore?
There was a problem hiding this comment.
The repo currently uses settings everywhere (DotrainRegistry, GUI SDK, webapp). We moved away from config to settings before. What did you have in mind?
Resolve conflicts in serialized state test constants after merging changes from main that affected YAML serialization ordering.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/js_api/src/gui/mod.rs (1)
167-183: Update the JS doc examples to reflect the new settings parameter.
The examples still show the old positional signature, which now shifts the deployment argument.✏️ Suggested doc update
-/// const result = await DotrainOrderGui.newWithDeployment(dotrainYaml, "mainnet-order"); +/// const result = await DotrainOrderGui.newWithDeployment(dotrainYaml, undefined, "mainnet-order"); ... -/// const result = await DotrainOrderGui.newWithDeployment( -/// dotrainYaml, -/// "mainnet-order", +/// const result = await DotrainOrderGui.newWithDeployment( +/// dotrainYaml, +/// undefined, +/// "mainnet-order",
🤖 Fix all issues with AI agents
In `@crates/js_api/src/gui/state_management.rs`:
- Line 471: The test contains a debug console_log invocation using
wasm_bindgen_test::console_log! that prints SERIALIZED_STATE; remove this debug
logging call (the wasm_bindgen_test::console_log! line) from the test in
state_management.rs so the test no longer emits console output, or alternatively
gate it behind a debug-only cfg (e.g., cfg(debug_assertions)) if you want to
keep it for local debugging; ensure the test still compiles and uses
SERIALIZED_STATE where needed for assertions.
In `@crates/settings/src/order.rs`:
- Around line 486-494: The code returns a misleading FieldErrorKind::InvalidType
when vault_ids.is_empty(); update the error to accurately reflect "no vault IDs
/ no orders found" by replacing FieldErrorKind::InvalidType with a more
appropriate variant (e.g., FieldErrorKind::MissingField or
FieldErrorKind::InvalidValue) and change the message to something like "no vault
IDs (no orders/IO tokens) found" while keeping the outer YamlError::Field, the
field name "orders" and location "root" so callers can locate the problem;
modify the error construction near the vault_ids.is_empty() check accordingly.
| let state = gui.serialize_state().unwrap(); | ||
| assert!(!state.is_empty()); | ||
| assert_eq!(state, SERIALIZED_STATE); | ||
| wasm_bindgen_test::console_log!("{}", SERIALIZED_STATE); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider removing debug logging in test code.
The console_log! statement appears to be debug output that may not be necessary in the final test.
🧹 Suggested cleanup
let state = gui.serialize_state().unwrap();
assert!(!state.is_empty());
assert_eq!(state, SERIALIZED_STATE);
- wasm_bindgen_test::console_log!("{}", SERIALIZED_STATE);
}📝 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.
| wasm_bindgen_test::console_log!("{}", SERIALIZED_STATE); | |
| let state = gui.serialize_state().unwrap(); | |
| assert!(!state.is_empty()); | |
| assert_eq!(state, SERIALIZED_STATE); | |
| } |
🤖 Prompt for AI Agents
In `@crates/js_api/src/gui/state_management.rs` at line 471, The test contains a
debug console_log invocation using wasm_bindgen_test::console_log! that prints
SERIALIZED_STATE; remove this debug logging call (the
wasm_bindgen_test::console_log! line) from the test in state_management.rs so
the test no longer emits console output, or alternatively gate it behind a
debug-only cfg (e.g., cfg(debug_assertions)) if you want to keep it for local
debugging; ensure the test still compiles and uses SERIALIZED_STATE where needed
for assertions.
| if vault_ids.is_empty() { | ||
| return Err(YamlError::Field { | ||
| kind: FieldErrorKind::InvalidType { | ||
| field: "orders".to_string(), | ||
| expected: "a map".to_string(), | ||
| }, | ||
| location: "root".to_string(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Misleading error when no vault IDs are found.
Returning InvalidType for orders suggests a schema/type issue rather than “order not found” or “no IO tokens”. Consider emitting a more accurate error.
🛠️ Suggested fix
- let mut vault_ids = HashMap::new();
+ let mut vault_ids = HashMap::new();
+ let mut found_order = false;
...
- if let Some(order_yaml) =
- orders_hash.get(&StrictYaml::String(order_key.to_string()))
- {
+ if let Some(order_yaml) =
+ orders_hash.get(&StrictYaml::String(order_key.to_string()))
+ {
+ found_order = true;
let location = format!("order '{}'", order_key);
...
- if vault_ids.is_empty() {
- return Err(YamlError::Field {
- kind: FieldErrorKind::InvalidType {
- field: "orders".to_string(),
- expected: "a map".to_string(),
- },
- location: "root".to_string(),
- });
- }
+ if !found_order {
+ return Err(YamlError::KeyNotFound(order_key.to_string()));
+ }🤖 Prompt for AI Agents
In `@crates/settings/src/order.rs` around lines 486 - 494, The code returns a
misleading FieldErrorKind::InvalidType when vault_ids.is_empty(); update the
error to accurately reflect "no vault IDs / no orders found" by replacing
FieldErrorKind::InvalidType with a more appropriate variant (e.g.,
FieldErrorKind::MissingField or FieldErrorKind::InvalidValue) and change the
message to something like "no vault IDs (no orders/IO tokens) found" while
keeping the outer YamlError::Field, the field name "orders" and location "root"
so callers can locate the problem; modify the error construction near the
vault_ids.is_empty() check accordingly.
Tauri was removed from main but this branch still had the files from a prior merge.
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
|
The author of this PR is on the CodeRabbit Free Plan. In order to use the Chat feature, please upgrade the PR author to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. |
3 participants
Chained PR
to_yaml_stringfunction to OrderbookYaml and DotrainYaml #2334Motivation
See issues:
In our current implementation, the gui sdk can only take a single dotrain string and we can only use the configuration defined in its frontmatter. We have the ability to merge additional settings while working with dot rain, but we are not using that.
This PR aims to change that by introducing additional settings to be used during different function calls in GUI SDK.
Solution
DotrainYamlandOrderbookYamlto have an ordered map for proper hashingChecks
By submitting this for review, I'm confirming I've done the following:
fix #2119
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.