Skip to content

Update registry to take in serialized state when constructing gui#2178

Merged
findolor merged 4 commits intomainfrom
codex/update-registry.getgui-to-accept-serialized-state
Oct 16, 2025
Merged

Update registry to take in serialized state when constructing gui#2178
findolor merged 4 commits intomainfrom
codex/update-registry.getgui-to-accept-serialized-state

Conversation

@findolor
Copy link
Copy Markdown
Collaborator

@findolor findolor commented Oct 7, 2025

See issue: #2120

Summary

  • place the optional serialized state argument ahead of the callback in DotrainRegistry.getGui so it aligns with DotrainOrderGui.newFromState
  • refresh the registry architecture notes and TypeScript tests to document and cover the new parameter order

Testing

  • cargo fmt

https://chatgpt.com/codex/tasks/task_e_68e4b573c36c8333b73d01d8a5a72073

Summary by CodeRabbit

  • New Features

    • Restore a GUI from a previously saved (serialized) state during creation.
    • Serialize the current GUI state for later restoration.
    • Programmatically set and get GUI field values.
  • Bug Fixes

    • Graceful fallback to default GUI when restoring from invalid or incompatible serialized state.
  • Documentation

    • Updated architecture docs to reflect the enhanced GUI state restoration flow.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Oct 7, 2025

Walkthrough

Adds an optional serializedState parameter to DotrainRegistry.getGui. When provided, getGui attempts DotrainOrderGui::new_from_state(serializedState, ...) and falls back to new_with_deployment(...) on failure. Exports, docs, and tests updated; GUI gains serialize/restore and field-access APIs.

Changes

Cohort / File(s) Summary
Docs: API description
crates/js_api/ARCHITECTURE.md
Documented updated getGui(orderKey, deploymentKey, serializedState?, stateCallback?) signature and state restoration behavior.
Rust: Registry API and control flow
crates/js_api/src/registry.rs
Extended get_gui with serialized_state: Option<String> (wasm serializedState); branch to use DotrainOrderGui::new_from_state when provided, fallback to new_with_deployment; map GUI errors to DotrainRegistryError::GuiError.
Tests: JS API usage and state flow
packages/orderbook/test/js_api/dotrainRegistry.test.ts
Updated test calls to new getGui signature (insert serializedState arg), added serialize/restore test using gui.serializeState(), gui.setFieldValue(), and gui.getFieldValue(), and adjusted existing tests for new callback position.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor JS as JS Caller
  participant DR as DotrainRegistry
  participant GUI as DotrainOrderGui

  JS->>DR: getGui(orderKey, deploymentKey, serializedState?, stateCallback?)
  alt serializedState provided
    DR->>GUI: new_from_state(serializedState, stateCallback)
    alt success
      GUI-->>DR: Gui instance
    else failure
      DR->>GUI: new_with_deployment(orderKey, deploymentKey, stateCallback)
      GUI-->>DR: Gui instance or error
    end
  else no serializedState
    DR->>GUI: new_with_deployment(orderKey, deploymentKey, stateCallback)
    GUI-->>DR: Gui instance or error
  end
  DR-->>JS: Gui instance or DotrainRegistryError::GuiError

  note over JS,GUI: GUI exposes serializeState(), setFieldValue(), getFieldValue()
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

rust, test

Suggested reviewers

  • 0xgleb
  • hardyjosh

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title accurately and concisely describes the primary change—updating the registry’s GUI constructor to accept a serialized state—without extraneous details. It clearly communicates the main intent of the pull request and would be meaningful to a reviewer scanning the commit history. The phrasing is specific to the affected component (registry) and the new capability (serialized state) while remaining succinct.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codex/update-registry.getgui-to-accept-serialized-state

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0756335 and 4ab7bdc.

📒 Files selected for processing (1)
  • crates/js_api/src/registry.rs (5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
crates/*/{src,tests,benches,examples}/**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Rust code must pass clippy with all warnings denied (cargo clippy --workspace --all-targets --all-features -D warnings)

Files:

  • crates/js_api/src/registry.rs
crates/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Rust code lives in the workspace under crates/* (e.g., cli, common, bindings, js_api, quote, subgraph, settings, math, integration_tests)

Files:

  • crates/js_api/src/registry.rs
**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Format Rust with nix develop -c cargo fmt --all
Lint Rust with nix develop -c rainix-rs-static (preconfigured flags)
Use snake_case for Rust crates/modules and PascalCase for types

Files:

  • crates/js_api/src/registry.rs
🧬 Code graph analysis (1)
crates/js_api/src/registry.rs (2)
crates/js_api/src/gui/state_management.rs (2)
  • new_from_state (220-302)
  • serialize_state (118-192)
crates/js_api/src/gui/mod.rs (1)
  • new_with_deployment (156-182)
🔇 Additional comments (5)
crates/js_api/src/registry.rs (5)

380-394: LGTM! Documentation examples properly updated.

The examples now correctly show undefined as the placeholder for the optional serializedState parameter and demonstrate both the callback-only and state-restoration use cases. This addresses the previous review feedback.


414-418: LGTM! Parameter correctly positioned.

The serialized_state parameter is properly placed as the third argument, aligning with DotrainOrderGui::new_from_state as specified in the PR objectives. The parameter documentation clearly describes its purpose and fallback behavior.


428-459: LGTM! Fallback logic correctly implemented.

The branching logic properly handles both scenarios:

  • When serialized_state is provided, it attempts restoration via new_from_state and silently falls back to new_with_deployment on failure (as documented in the parameter description).
  • When None, it directly uses new_with_deployment.

The error mapping to DotrainRegistryError::GuiError is appropriate, and the callback cloning is necessary for Rust's move semantics across different branches.


1320-1386: Excellent test coverage! Previous issues resolved.

The tests have been updated to correctly pass arguments in the new order (serialized_state as 3rd, callback as 4th), addressing the previous review feedback. The test suite now comprehensively covers:

  • Basic usage without serialization (lines 1320-1327)
  • Capturing default state (line 1329)
  • State restoration from valid serialized data (lines 1363-1372)
  • Silent fallback to defaults when invalid state is provided (lines 1374-1386)

The assertions properly verify that restored state matches saved state and that fallback behavior produces the expected default state.


1388-1394: LGTM! Error test correctly updated.

The error case test has been properly updated to use the new four-parameter signature, maintaining correct argument order while preserving the error handling verification logic.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

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/registry.rs (1)

49-61: Outdated example still shows 3-arg getGui

Earlier example still uses getGui("fixed-limit", "mainnet", stateCallback). Update to include the serializedState placeholder to avoid confusion.

Suggested tweak in the doc snippet:

  • From: getGui("fixed-limit", "mainnet", stateCallback)
  • To: getGui("fixed-limit", "mainnet", null, stateCallback)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b1cef0d and f0c994a.

📒 Files selected for processing (3)
  • crates/js_api/ARCHITECTURE.md (2 hunks)
  • crates/js_api/src/registry.rs (5 hunks)
  • packages/orderbook/test/js_api/dotrainRegistry.test.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
crates/*/{src,tests,benches,examples}/**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Rust code must pass clippy with all warnings denied (cargo clippy --workspace --all-targets --all-features -D warnings)

Files:

  • crates/js_api/src/registry.rs
crates/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Rust code lives in the workspace under crates/* (e.g., cli, common, bindings, js_api, quote, subgraph, settings, math, integration_tests)

Files:

  • crates/js_api/src/registry.rs
**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Format Rust with nix develop -c cargo fmt --all
Lint Rust with nix develop -c rainix-rs-static (preconfigured flags)
Use snake_case for Rust crates/modules and PascalCase for types

Files:

  • crates/js_api/src/registry.rs
packages/orderbook/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Orderbook TypeScript package must pass type/lint checks (npm run check -w @rainlanguage/orderbook)

Files:

  • packages/orderbook/test/js_api/dotrainRegistry.test.ts
packages/orderbook/**/*.{test,spec}.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Orderbook tests must pass (npm run test -w @rainlanguage/orderbook)

Files:

  • packages/orderbook/test/js_api/dotrainRegistry.test.ts
packages/**

📄 CodeRabbit inference engine (AGENTS.md)

JavaScript/Svelte projects live under packages/* (webapp, ui-components, orderbook)

Files:

  • packages/orderbook/test/js_api/dotrainRegistry.test.ts
packages/**/*.{test,spec}.ts

📄 CodeRabbit inference engine (AGENTS.md)

Name TypeScript test files as *.test.ts or *.spec.ts (Vitest)

Files:

  • packages/orderbook/test/js_api/dotrainRegistry.test.ts
🧠 Learnings (1)
📚 Learning: 2025-06-04T10:21:01.388Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1907
File: packages/orderbook/test/common/test.test.ts:75-77
Timestamp: 2025-06-04T10:21:01.388Z
Learning: The DotrainOrder.create API in packages/orderbook/test/common/test.test.ts is internal and not used directly in consumer applications, so API changes here don't require external breaking change documentation.

Applied to files:

  • packages/orderbook/test/js_api/dotrainRegistry.test.ts
🧬 Code graph analysis (1)
crates/js_api/src/registry.rs (2)
crates/js_api/src/gui/state_management.rs (2)
  • new_from_state (220-302)
  • serialize_state (118-192)
crates/js_api/src/gui/mod.rs (1)
  • new_with_deployment (156-182)
🔇 Additional comments (4)
crates/js_api/ARCHITECTURE.md (1)

133-134: Docs: getGui signature correctly updated

Signature now matches runtime API and TS usage: (orderKey, deploymentKey, serializedState?, stateCallback?). Looks good.

packages/orderbook/test/js_api/dotrainRegistry.test.ts (1)

295-343: Tests align with reordered API and cover restoration

  • Four-arg getGui usage is correct.
  • Restoration test exercises serializeState/getFieldValue properly.
crates/js_api/src/registry.rs (2)

380-395: Updated examples reflect new parameter order

The examples now include serializedState before stateCallback and show both “null + callback” and “restore from saved state” flows. Good.


414-459: Behavior on invalid serialized state: confirm silent fallback is intended

Current logic silently falls back to new_with_deployment on any new_from_state error. This hides deserialization/validation issues from callers. If intentional, fine; otherwise consider:

  • Emitting a readable warning in the error path, or
  • Falling back only on dotrain mismatch/decoding errors and surfacing other GUI errors.

No blocker; just confirm desired UX.

Comment thread crates/js_api/src/registry.rs
@findolor findolor changed the title refactor: reorder registry GUI state parameter Update registry to take in serialized state when constructing gui Oct 7, 2025
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f0c994a and 0756335.

📒 Files selected for processing (1)
  • crates/js_api/src/registry.rs (5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
crates/*/{src,tests,benches,examples}/**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Rust code must pass clippy with all warnings denied (cargo clippy --workspace --all-targets --all-features -D warnings)

Files:

  • crates/js_api/src/registry.rs
crates/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Rust code lives in the workspace under crates/* (e.g., cli, common, bindings, js_api, quote, subgraph, settings, math, integration_tests)

Files:

  • crates/js_api/src/registry.rs
**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Format Rust with nix develop -c cargo fmt --all
Lint Rust with nix develop -c rainix-rs-static (preconfigured flags)
Use snake_case for Rust crates/modules and PascalCase for types

Files:

  • crates/js_api/src/registry.rs
🧬 Code graph analysis (1)
crates/js_api/src/registry.rs (2)
crates/js_api/src/gui/state_management.rs (2)
  • new_from_state (220-302)
  • serialize_state (118-192)
crates/js_api/src/gui/mod.rs (1)
  • new_with_deployment (156-182)

Comment thread crates/js_api/src/registry.rs
@findolor findolor requested review from 0xgleb and hardyjosh October 8, 2025 15:50
@findolor findolor merged commit 9328c13 into main Oct 16, 2025
19 checks passed
@github-actions
Copy link
Copy Markdown
Contributor

@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:

  • Simple bug fixes, typos, or minor refactoring
  • Single-purpose changes affecting 1-2 files
  • Documentation updates
  • Configuration tweaks
  • Changes that require minimal context to review

Review Effort: Would have taken 5-10 minutes

Examples:

  • Fix typo in variable name
  • Update README with new instructions
  • Adjust configuration values
  • Simple one-line bug fixes
  • Import statement cleanup

Medium (M)

Characteristics:

  • Feature additions or enhancements
  • Refactoring that touches multiple files but maintains existing behavior
  • Breaking changes with backward compatibility
  • Changes requiring some domain knowledge to review

Review Effort: Would have taken 15-30 minutes

Examples:

  • Add new feature or component
  • Refactor common utility functions
  • Update dependencies with minor breaking changes
  • Add new component with tests
  • Performance optimizations
  • More complex bug fixes

Large (L)

Characteristics:

  • Major feature implementations
  • Breaking changes or API redesigns
  • Complex refactoring across multiple modules
  • New architectural patterns or significant design changes
  • Changes requiring deep context and multiple review rounds

Review Effort: Would have taken 45+ minutes

Examples:

  • Complete new feature with frontend/backend changes
  • Protocol upgrades or breaking changes
  • Major architectural refactoring
  • Framework or technology upgrades

Additional Factors to Consider

When deciding between sizes, also consider:

  • Test coverage impact: More comprehensive test changes lean toward larger classification
  • Risk level: Changes to critical systems bump up a size category
  • Team familiarity: Novel patterns or technologies increase complexity

Notes:

  • the assessment must be for the totality of the PR, that means comparing the base branch to the last commit of the PR
  • the assessment output must be exactly one of: S, M or L (single-line comment) in format of: SIZE={S/M/L}
  • do not include any additional text, only the size classification
  • your assessment comment must not include tips or additional sections
  • do NOT tag me or anyone else on your comment

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Oct 16, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the full diff statistics for the PR
git diff origin/main --stat

# Get a summary of the changes
git diff origin/main --shortstat

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Get the list of commits in this branch
git log --oneline origin/main..HEAD

# Get detailed stats for all changes
git diff origin/main..HEAD --numstat

# Count total lines changed
git diff origin/main..HEAD --numstat | awk '{added+=$1; deleted+=$2} END {print "Total additions:", added, "Total deletions:", deleted}'

Length of output: 267361


SIZE=M


🧠 Learnings used
Learnt from: rouzwelt
PR: rainlanguage/rain.orderbook#0
File: :0-0
Timestamp: 2025-05-21T21:24:42.275Z
Learning: For PR #1884, after each commit review, I should provide a new comment with a PR size report in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding irrelevant files such as lock files (package-lock.json, cargo.lock, etc.).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DotrainRegistry.getGui: accept optional serialized state to restore GUI

3 participants