Implement DotrainRegistry SDK in webapp
#2342
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughMultiple crates and packages are updated to integrate registry-based data loading and introduce multi-document YAML support for scenarios. Data flows shift from direct static method calls and manual fetching to centralized registry provider patterns, with error handling refined across parsing and initialization paths. Changes
Sequence DiagramsequenceDiagram
participant App as App/+layout.svelte
participant Registry as DotrainRegistry
participant Manager as RegistryManager
participant DeployLayout as deploy/+layout.ts
participant OrderLayout as deploy/[orderName]/+layout.ts
participant OrderPage as OrderPage.svelte
participant UI as DeploymentsSection
Note over App,UI: Previous Flow (Removed)
App->>App: fetch registry & dotrains via URL
App->>DotrainOrderGui: newFromState/newWithDeployment
Note over App,UI: New Flow (Current)
App->>Registry: Registry.new(url)
App->>Manager: RegistryManager(url)
App->>App: Store registry & manager in context
DeployLayout->>Registry: getAllOrderDetails()
Registry-->>DeployLayout: validOrders, invalidOrders
OrderLayout->>Registry: getDeploymentDetails(orderName)
Registry-->>OrderLayout: deployments map
OrderLayout->>OrderPage: Pass orderDetail, deployments
OrderPage->>UI: Pass deployments prop
UI->>UI: Render deployments from prop (no fetch)
OrderPage->>Registry: getGui(orderName, deploymentKey)
Registry-->>OrderPage: GUI instance
OrderPage->>OrderPage: Render GUI with markdown
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–75 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Comment |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/ui-components/src/__tests__/loadRegistryUrl.test.ts (1)
48-61: Add coverage for thevalidationResult.errorbranchYou’re covering the success path and both Error/non-Error rejection paths, but the new implementation also handles the case where
DotrainRegistry.validateresolves with anerrorobject (and uses.readableMsg). That branch isn’t exercised here.Please add a test along the lines of:
(DotrainRegistry.validate as Mock).mockResolvedValueOnce({ error: { readableMsg: 'Validation failed' } }); await expect(loadRegistryUrl(testUrl, mockRegistryManager)).rejects.toThrow('Validation failed'); expect(mockRegistryManager.setRegistry).not.toHaveBeenCalled(); expect(window.location.reload).not.toHaveBeenCalled();to lock in the intended behavior.
Also applies to: 63-77
packages/ui-components/src/lib/services/loadRegistryUrl.ts (1)
1-27: Ensure TS tooling and screenshot requirements are metSince this is TS in
packages/ui-componentsand affects frontend behavior, please ensure for this PR that:
nix develop -c npm run format -w @rainlanguage/ui-componentsnix develop -c npm run lint -w @rainlanguage/ui-componentsnix develop -c npm run check -w @rainlanguage/ui-componentsnix develop -c npm run test -w @rainlanguage/ui-componentshave been run and are green, and attach a screenshot of the built webapp showing the updated registry URL flow, per repo guidelines.
packages/webapp/src/routes/deploy/[orderName]/layout.test.ts (1)
62-72: Make the redirect test fail ifloaddoes not throwThe current
try/catchpattern will pass even ifloadresolves successfully (no redirect), because there is no assertion in the success path. Restructure to assert thatloadrejects and thatredirectwas called:- it('should redirect if order name is not found in validOrders', async () => { - try { - await load({ - params: { orderName: 'non-existent-order' }, - parent: mockParent - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any); - } catch { - expect(redirect).toHaveBeenCalledWith(307, '/deploy'); - } - }); + it('should redirect if order name is not found in validOrders', async () => { + await expect( + load({ + params: { orderName: 'non-existent-order' }, + parent: mockParent + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any) + ).rejects.toEqual(expect.anything()); + + expect(redirect).toHaveBeenCalledWith(307, '/deploy'); + });This ensures the test fails if
loadever stops throwing.crates/settings/src/unit_test.rs (1)
84-92: Functionally correct ScenarioCfg.documents init; consider reusing helperInitializing
documentswithArc::new(Vec::new())is type‑correct and works. For consistency with the rest of the settings crate, you might prefer to call the shareddefault_documents()helper instead, but this is optional.packages/ui-components/src/lib/components/deployment/OrderPage.svelte (1)
20-29: Non-OK response silently fails without feedback.When
response.okis false (e.g., 404, 500), the function returns without setting an error ormarkdownContent. The user sees only the plain description without any indication that markdown fetch failed.Consider setting an error message for non-OK responses:
const fetchMarkdownContent = async (url: string) => { try { const response = await fetch(url); if (response.ok) { markdownContent = await response.text(); + } else { + error = `Failed to fetch markdown (${response.status})`; } } catch { error = `Failed to fetch markdown`; } };
| if (!errorMessage) { | ||
| try { | ||
| const registryResult = await DotrainRegistry.new(registryUrl); | ||
| console.log(registryResult); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove debug logging before merge.
This console.log statement appears to be debug code that should be removed from production.
- console.log(registryResult);🤖 Prompt for AI Agents
In packages/webapp/src/routes/+layout.ts around line 51, there's a leftover
debug console.log(registryResult); statement; remove this console.log line (or
replace it with appropriate structured logging at debug level if persistent
logging is required) so no debug output is emitted in production.
Comment on lines
173
to
214
| it('should return errorMessage if registry fails to load', async () => { | ||
| mockRegistryNew.mockRejectedValueOnce(new Error('Network error')); | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const result = await load({ fetch: mockFetch } as any); | ||
| const result = await load({ url: new URL('http://localhost:3000') } as any); | ||
|
|
||
| expect(result).toHaveProperty('stores', null); | ||
| expect(result).toHaveProperty('errorMessage'); | ||
| expect(result.errorMessage).toContain('Failed to get site config settings.'); | ||
| expect(result.errorMessage).toContain('Error status: 404'); | ||
| expect(result.errorMessage).toContain('Failed to load registry'); | ||
| }); | ||
|
|
||
| it('should return errorMessage if response.text() fails', async () => { | ||
| mockFetch.mockResolvedValueOnce({ | ||
| ok: true, | ||
| text: () => Promise.reject(new Error('Invalid YAML')) | ||
| it('should return errorMessage if RaindexClient fails to initialize', async () => { | ||
| const mockRegistry = { settings: vi.fn().mockReturnValue('settings') }; | ||
| mockRegistryNew.mockResolvedValueOnce({ | ||
| value: mockRegistry | ||
| }); | ||
| mockRaindexClientNew.mockReturnValue({ | ||
| error: { readableMsg: 'Malformed settings' } | ||
| }); | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const result = await load({ fetch: mockFetch } as any); | ||
|
|
||
| expect(result).toHaveProperty('stores', null); | ||
| expect(result).toHaveProperty('errorMessage'); | ||
| expect(result.errorMessage).toContain('Failed to get site config settings.'); | ||
| expect(result.errorMessage).toContain('Invalid YAML'); | ||
| }); | ||
|
|
||
| it('should handle fetch failure', async () => { | ||
| mockFetch.mockRejectedValueOnce(new Error('Network error')); | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const result = await load({ fetch: mockFetch } as any); | ||
| const result = await load({ url: new URL('http://localhost:3000') } as any); | ||
|
|
||
| expect(result).toHaveProperty('stores', null); | ||
| expect(result).toHaveProperty('errorMessage'); | ||
| expect(result.errorMessage).toContain('Failed to get site config settings.'); | ||
| expect(result.errorMessage).toContain('Network error'); | ||
| expect(result.errorMessage).toContain('Malformed settings'); | ||
| }); | ||
|
|
||
| it('should handle empty or malformed settings YAML', async () => { | ||
| (RaindexClient.new as Mock).mockReturnValue({ | ||
| value: undefined, | ||
| error: { | ||
| msg: 'Malformed settings', | ||
| readableMsg: 'Malformed settings' | ||
| } | ||
| it('should initialize when registry and RaindexClient succeed', async () => { | ||
| const mockRegistry = { settings: 'settings' }; | ||
| mockRegistryNew.mockResolvedValueOnce({ | ||
| value: mockRegistry | ||
| }); | ||
| mockFetch.mockResolvedValueOnce({ | ||
| ok: true, | ||
| text: () => Promise.resolve('malformed') | ||
| mockRaindexClientNew.mockReturnValue({ | ||
| value: { client: true } | ||
| }); | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const result = await load({ fetch: mockFetch } as any); | ||
| const result = await load({ url: new URL('http://localhost:3000') } as any); | ||
|
|
||
| expect(result).toHaveProperty('stores', null); | ||
| expect(result).toHaveProperty('errorMessage'); | ||
| expect(result.errorMessage).toContain('Malformed settings'); | ||
| expect(result.errorMessage).toContain('Malformed settings'); | ||
| expect(result.errorMessage).toBeUndefined(); | ||
| expect(result.stores).not.toBeNull(); | ||
| expect(result.registry).toEqual(mockRegistry); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding test coverage for local database initialization failure.
The tests cover registry failure and RaindexClient failure, but there's no test for the SQLiteWasmDatabase.new failure path. Adding this would ensure complete error handling coverage.
Example test case to add:
it('should return errorMessage if local database fails to initialize', async () => {
const mockRegistry = { settings: 'settings' };
mockRegistryNew.mockResolvedValueOnce({
value: mockRegistry
});
mockRaindexClientNew.mockReturnValue({
value: { client: true }
});
mockLocalDbNew.mockReturnValue({
error: { readableMsg: 'Database init failed' }
});
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const result = await load({ url: new URL('http://localhost:3000') } as any);
expect(result).toHaveProperty('stores', null);
expect(result.errorMessage).toContain('Error initializing local database');
});🤖 Prompt for AI Agents
In packages/webapp/src/routes/+layout.ts around lines 173 to 214, add a test
that covers the SQLiteWasmDatabase.new failure path: mock the registry and
RaindexClient to succeed (as done in existing tests), set mockLocalDbNew to
return an error object with readableMsg 'Database init failed', call load({ url:
new URL('http://localhost:3000') } as any), and assert that result.stores is
null (or hasProperty 'stores', null) and result.errorMessage contains 'Error
initializing local database' (or contains the readableMsg), following the same
test structure and eslint-disable comment used in the other tests.
Comment on lines
29
to
33
| if (!deployment || !registry || !orderName) { | ||
| setTimeout(() => { | ||
| goto('/deploy'); | ||
| }, 5000); | ||
| } |
Contributor
There was a problem hiding this comment.
setTimeout runs outside lifecycle hook, may execute during SSR.
The redirect logic with setTimeout executes at module evaluation time (not inside onMount), which could cause issues during server-side rendering where goto may not behave as expected.
Consider moving this logic inside onMount to ensure it only runs client-side:
+ onMount(async () => {
+ if (!deployment || !registry || !orderName) {
+ setTimeout(() => {
+ goto('/deploy');
+ }, 5000);
+ return;
+ }
+
- if (!deployment || !registry || !orderName) {
- setTimeout(() => {
- goto('/deploy');
- }, 5000);
- }
-
- onMount(async () => {
- if (!deployment || !registry || !orderName) return;
-
const serializedState = stateFromUrl || undefined;
// ... rest of onMount
});📝 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.
Suggested change
| if (!deployment || !registry || !orderName) { | |
| setTimeout(() => { | |
| goto('/deploy'); | |
| }, 5000); | |
| } | |
| onMount(async () => { | |
| if (!deployment || !registry || !orderName) { | |
| setTimeout(() => { | |
| goto('/deploy'); | |
| }, 5000); | |
| return; | |
| } | |
| const serializedState = stateFromUrl || undefined; | |
| // ... rest of onMount | |
| }); |
🤖 Prompt for AI Agents
In packages/webapp/src/routes/deploy/[orderName]/[deploymentKey]/+page.svelte
around lines 29-33 the setTimeout redirect runs at module evaluation time which
can execute during SSR; move the redirect logic into a client-only lifecycle
hook: wrap the existence check and setTimeout/goto call inside onMount so it
only runs in the browser, and import onMount from 'svelte' if not already
present.
Comment on lines
69
to
82
| it('should redirect if order details are not found in validOrders', async () => { | ||
| mockParent.mockResolvedValue({ | ||
| registryDotrains: [{ name: 'incomplete-order', dotrain: 'https://incomplete.example.com' }], | ||
| validOrders: [ | ||
| { name: 'other-order', details: { name: 'Other', description: '', config: {} } } | ||
| ] | ||
| ], | ||
| invalidOrders: [], | ||
| registry: { | ||
| getDeploymentDetails: vi.fn().mockReturnValue({ value: mockDeployments }) | ||
| } |
Contributor
There was a problem hiding this comment.
Fix mismatch between test name and fixture for “missing details” case
This test claims to cover “order details are not found in validOrders”, but the fixture has no validOrders entry for 'incomplete-order', so it effectively duplicates the “order name not found” scenario.
If the intention is to simulate an order entry with missing details, consider aligning the fixture with the param, e.g.:
- mockParent.mockResolvedValue({
- validOrders: [
- { name: 'other-order', details: { name: 'Other', description: '', config: {} } }
- ],
- invalidOrders: [],
- registry: {
- getDeploymentDetails: vi.fn().mockReturnValue({ value: mockDeployments })
- }
- });
+ mockParent.mockResolvedValue({
+ validOrders: [
+ {
+ name: 'incomplete-order',
+ // @ts-expect-error simulate missing details shape
+ details: null
+ }
+ ],
+ invalidOrders: [],
+ registry: {
+ getDeploymentDetails: vi.fn().mockReturnValue({ value: mockDeployments })
+ }
+ });This way the test actually exercises the “no details for an otherwise valid order name” branch.
🤖 Prompt for AI Agents
In packages/webapp/src/routes/deploy/[orderName]/layout.test.ts around lines 74
to 82, the test name says it should cover the case where an order exists in
validOrders but its details are missing, yet the fixture instead omits the order
entirely (duplicating the “order name not found” case); update the mockParent
fixture to include a validOrders entry for 'incomplete-order' (the test param)
with its details property missing or explicitly null/undefined so the test
actually exercises the "order present but details missing" branch, and ensure
other parts of the fixture (invalidOrders/registry) remain consistent with that
scenario.
Comment on lines
16
to
27
| export const load: LayoutLoad<LoadResult> = async ({ parent }) => { | ||
| const parentData = await parent(); | ||
| const registry = (parentData as { registry?: DotrainRegistry | null }).registry ?? null; | ||
|
|
||
| if (!registry) { | ||
| return { | ||
| validOrders: [], | ||
| invalidOrders: [], | ||
| registry, | ||
| error: 'Registry not loaded' | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Parent data type assertion could be more explicit.
The type assertion (parentData as { registry?: DotrainRegistry | null }) works but is somewhat fragile. Consider defining a proper parent type or using a type guard for better type safety.
+type RootLayoutData = {
+ registry?: DotrainRegistry | null;
+};
export const load: LayoutLoad<LoadResult> = async ({ parent }) => {
const parentData = await parent();
- const registry = (parentData as { registry?: DotrainRegistry | null }).registry ?? null;
+ const registry = (parentData as RootLayoutData).registry ?? 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.
Suggested change
| export const load: LayoutLoad<LoadResult> = async ({ parent }) => { | |
| const parentData = await parent(); | |
| const registry = (parentData as { registry?: DotrainRegistry | null }).registry ?? null; | |
| if (!registry) { | |
| return { | |
| validOrders: [], | |
| invalidOrders: [], | |
| registry, | |
| error: 'Registry not loaded' | |
| }; | |
| } | |
| type RootLayoutData = { | |
| registry?: DotrainRegistry | null; | |
| }; | |
| export const load: LayoutLoad<LoadResult> = async ({ parent }) => { | |
| const parentData = await parent(); | |
| const registry = (parentData as RootLayoutData).registry ?? null; | |
| if (!registry) { | |
| return { | |
| validOrders: [], | |
| invalidOrders: [], | |
| registry, | |
| error: 'Registry not loaded' | |
| }; | |
| } |
🤖 Prompt for AI Agents
In packages/webapp/src/routes/deploy/+layout.ts around lines 16–27, the current
cast "(parentData as { registry?: DotrainRegistry | null })" is fragile; define
an explicit parent-data interface (e.g. interface ParentData { registry?:
DotrainRegistry | null }) and use it when calling parent() (or pass the generic
to LayoutLoad) so parentData is strongly typed, or implement a small type guard
function isParentData(x): x is ParentData and use that to narrow parentData
before accessing registry; replace the inline assertion with the typed variable
or guard to ensure type safety.
0xgleb
approved these changes
Dec 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Chained PR
Dependent PR
Motivation
See issues:
Now that we have everything in place for the registry implementation in webapp, this PR does just that.
Solution
Web app bootstrap now loads the dotrain registry via WASM (DotrainRegistry) and builds RaindexClient from the registry’s embedded settings instead of fetching a standalone YAML
Deploy flows read order/deployment details directly from the registry and pass them through the route hierarchy, eliminating per-component DotrainOrderGui fetches and the old handleGuiInitialization helper
Registry/order validation tests were rewritten around the new registry-driven data flow, simplifying component tests to consume provided props and removing the old async mocks
Settings backend now tracks full document sets on ScenarioCfg, provides default_documents(), caches remote tokens on the orderbook YAML, and surfaces clearer missing-field errors for networks/orderbooks/orders
Tauri commands were updated to pass the new scenario docs
fix #2078
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
Release Notes
New Features
?registry=query parameter with persistent storage.Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.