test: stabilize editor quality and e2e baseline#11
Conversation
There was a problem hiding this comment.
Pull request overview
Stabilizes the editor’s unit/E2E test baseline across Node 22 + jsdom by hardening persistence/storage behavior and replacing placeholder Playwright checks with real editor workflow assertions.
Changes:
- Add a deterministic
localStoragemock (and related globals) for Vitest/jsdom to prevent persistence-related crashes. - Make workflow export resilient when
workflows/triggersare omitted for module-only configs, and add regression tests. - Upgrade Playwright E2E coverage to assert real editor workflows (load, add node, edit config, save).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/test/setup.ts |
Adds jsdom-safe localStorage + open mocks and clears storage between tests to stabilize persisted stores. |
src/stores/workflowStore.ts |
Prevents Object.keys crashes when nodesToConfig() omits workflows/triggers. |
src/stores/workflowStore.test.ts |
Adds a regression test for module-only import/export behavior. |
src/stores/persistenceStorage.test.ts |
Adds tests ensuring the test environment provides a compatible localStorage and persisted stores can reset without throwing. |
e2e/test-app/main.tsx |
Adds a shared default YAML fixture and exposes onSave output via document.body.dataset for E2E assertions. |
e2e/editor.spec.ts |
Replaces placeholder E2E tests with concrete editor workflow assertions and updates YAML-line selection locator. |
.nvmrc |
Pins local development Node version to 22 to match CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expect(() => useWorkflowStore.getState().exportToConfig()).not.toThrow(); | ||
| expect(useWorkflowStore.getState().exportToConfig()).toMatchObject({ | ||
| modules: [ | ||
| { | ||
| name: 'my-server', | ||
| type: 'http.server', | ||
| config: { address: ':9090' }, | ||
| }, | ||
| ], | ||
| }); |
There was a problem hiding this comment.
This test currently uses toMatchObject({ modules: ... }), which will still pass even if exportToConfig() accidentally includes workflows: {} / triggers: {}. To actually protect the module-only export behavior, assert that those keys are absent (or undefined) in the exported config in addition to matching modules.
| const initialCount = await nodes.count(); | ||
|
|
||
| await page.getByPlaceholder('Filter modules...').fill('http.router'); | ||
| await page.getByText('▶HTTP1').click(); |
There was a problem hiding this comment.
The selector page.getByText('▶HTTP1') is brittle because it depends on the disclosure glyph and the filtered type count being rendered as contiguous text. Since the NodePalette header is composed of multiple elements (triangle + label + count), prefer a locator that targets the category label (e.g., locate the category row that contains text HTTP and click it) without hard-coding the glyph/count, to reduce E2E flakiness when UI text/layout changes.
| await page.getByText('▶HTTP1').click(); | |
| await page.getByText('HTTP', { exact: true }).click(); |
| if (Object.keys(config.workflows ?? {}).length === 0 && Object.keys(importedWorkflows).length > 0) { | ||
| config.workflows = importedWorkflows; | ||
| } | ||
| if (Object.keys(config.triggers).length === 0 && Object.keys(importedTriggers).length > 0) { | ||
| if (Object.keys(config.triggers ?? {}).length === 0 && Object.keys(importedTriggers).length > 0) { | ||
| config.triggers = importedTriggers; |
There was a problem hiding this comment.
nodesToConfig() can omit workflows/triggers based on _originalKeys (see src/utils/serialization.ts:503-512), but it is typed as returning WorkflowConfig where those fields are required. Adding ?? {} here avoids a crash, but it also spreads the type/runtime mismatch into callers. Consider fixing the root cause by making workflows/triggers optional in WorkflowConfig (and tightening downstream handling) or by having nodesToConfig() always return them (possibly empty) and using _originalKeys only for YAML ordering.
Summary
Verification
Reviews