Load and save schema files#213
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds plain-text schema file load and save to ChangesSchema File Load/Save Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant SharedSchemaDefinitionView
participant SharedSchemaDefinitionController
participant SchemaFileTransferService
participant SchemaEditorController
rect rgba(173, 216, 230, 0.5)
Note over User,SchemaEditorController: Load schema from file
User->>SharedSchemaDefinitionView: click load button
SharedSchemaDefinitionView->>SharedSchemaDefinitionView: open hidden file input
User->>SharedSchemaDefinitionView: change event (file selected)
SharedSchemaDefinitionView->>SharedSchemaDefinitionController: loadSchemaFromFile(file)
SharedSchemaDefinitionController->>SchemaFileTransferService: readSchemaTextFile(file)
SchemaFileTransferService-->>SharedSchemaDefinitionController: normalized text
SharedSchemaDefinitionController->>SchemaEditorController: loadSchemaText(text, {showErrors, preferRowMode})
SchemaEditorController-->>SharedSchemaDefinitionController: {rows, errors, applied}
SharedSchemaDefinitionController-->>SharedSchemaDefinitionView: onSchemaFileLoaded callback
SharedSchemaDefinitionView->>SharedSchemaDefinitionView: clear file input value
end
rect rgba(144, 238, 144, 0.5)
Note over User,SchemaFileTransferService: Save schema to file
User->>SharedSchemaDefinitionView: click save button
SharedSchemaDefinitionView->>SharedSchemaDefinitionController: saveSchemaToFile()
SharedSchemaDefinitionController->>SchemaFileTransferService: downloadSchemaText(text, filename)
SharedSchemaDefinitionController-->>SharedSchemaDefinitionView: onSchemaFileSaved callback
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
packages/core-ui/js/gui_components/shared/schema-file-transfer-service.jsOops! Something went wrong! :( ESLint: 10.4.1 A config object is using the "root" key, which is not supported in flat config system. Flat configs always act as if they are the root config file, so this key can be safely removed. packages/core-ui/js/gui_components/shared/test-data/schema/shared-schema-editor-controller.jsOops! Something went wrong! :( ESLint: 10.4.1 A config object is using the "root" key, which is not supported in flat config system. Flat configs always act as if they are the root config file, so this key can be safely removed. packages/core-ui/src/tests/shared/schema-file-transfer-service.test.jsOops! Something went wrong! :( ESLint: 10.4.1 A config object is using the "root" key, which is not supported in flat config system. Flat configs always act as if they are the root config file, so this key can be safely removed.
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds schema file load/save capabilities directly to the shared schema editor component so the app test-data panel, generator page, and writer-schema page all share the same import/export workflow and implementation.
Changes:
- Introduce shared text-file read and schema file transfer services, and wire them into
SharedSchemaDefinition. - Add shared-schema controller/view support + UI controls for loading a schema text file and saving the current schema to disk.
- Add/extend Jest, Storybook, and Playwright coverage for schema file load/save flows.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core-ui/src/tests/shared/shared-schema-definition-view.test.js | Adds UI/service wiring tests for load/save controls and file input flows. |
| packages/core-ui/src/tests/shared/shared-schema-definition-controller.test.js | Adds controller-level tests for forwarding file save/load through the shared service. |
| packages/core-ui/js/gui_components/shared/text-file-read-service.js | New shared FileReader-based text read service for reuse across hosts. |
| packages/core-ui/js/gui_components/shared/test-data/schema/shared-schema-editor-controller.js | Enhances loadSchemaText behavior to support file-load flows and row-mode preference. |
| packages/core-ui/js/gui_components/shared/schema-file-transfer-service.js | New shared service to normalize schema text on read and download schema text files. |
| packages/core-ui/js/gui_components/shared/schema-definition/shared-schema-definition-view.js | Adds load/save buttons and file input, and binds them to controller actions. |
| packages/core-ui/js/gui_components/shared/schema-definition/shared-schema-definition-controller.js | Adds file load/save actions and related callbacks/error handling hooks. |
| packages/core-ui/js/gui_components/shared/schema-definition/index.js | Wires default schemaFileTransferService into the shared component via services injection. |
| packages/core-ui/js/gui_components/app/import-export-adapters/file-read-service.js | Switches app adapter to re-export the new shared text file read service. |
| docs/frontend-component-migration-plan.md | Documents that schema file controls are now owned by SharedSchemaDefinition. |
| apps/web/styles.css | Adds styling for the new shared schema file action button group. |
| apps/web/src/tests/browser/shared/abstractions/components/schema-editor.component.js | Extends Playwright component abstraction with file load/save helpers. |
| apps/web/src/tests/browser/generator/functional/schema-file-load-save.spec.js | New Playwright spec validating generator schema load/save via files. |
| apps/web/src/tests/browser/generator/abstractions/components/generator-schema.component.js | Exposes file load/save methods through generator schema abstraction. |
| apps/web/src/tests/browser/app/functional/test-data/schema-file-load-save.spec.js | New Playwright spec validating app test-data panel schema load/save via files. |
| apps/web/src/tests/browser/app/abstractions/components/test-data-panel.component.js | Exposes file load/save methods through test-data panel abstraction. |
| apps/web/src/stories/shared-schema-definition.stories.js | Adds Storybook stories documenting valid/invalid schema file upload behavior. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 307442236a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Greptile SummaryThis PR adds shared plain-text schema file load and save controls to
Confidence Score: 5/5Safe to merge. The changes are well-scoped, the new service layer is injected rather than global, and all three page hosts adopt the same shared implementation without breaking their existing API contracts. The No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant View as SharedSchemaDefinitionView
participant Controller as SharedSchemaDefinitionController
participant TransferSvc as SchemaFileTransferService
participant ReadSvc as TextFileReadService
participant EditorCtrl as SharedSchemaEditorController
Note over User,EditorCtrl: Load schema file flow
User->>View: selects file via hidden input
View->>Controller: loadSchemaFromFile(file)
Controller->>TransferSvc: readSchemaTextFile(file)
TransferSvc->>ReadSvc: readText(file)
ReadSvc-->>TransferSvc: raw text
TransferSvc-->>Controller: normalised text (BOM+CRLF stripped)
Controller->>EditorCtrl: "loadSchemaText(text, {showErrors:true, preferRowMode:true})"
EditorCtrl-->>Controller: "{rows, errors, applied}"
alt "applied === true"
Controller->>Controller: callbacks.onSchemaFileLoaded(...)
end
Controller-->>View: result
Note over User,EditorCtrl: Save schema file flow
User->>View: clicks Save Schema File
View->>Controller: saveSchemaToFile()
Controller->>EditorCtrl: getSchemaText()
EditorCtrl-->>Controller: schemaText
Controller->>TransferSvc: "downloadSchemaText(schemaText, {filename})"
TransferSvc->>TransferSvc: new Download(...).downloadFile(...)
Controller->>Controller: callbacks.onSchemaFileSaved(...)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User
participant View as SharedSchemaDefinitionView
participant Controller as SharedSchemaDefinitionController
participant TransferSvc as SchemaFileTransferService
participant ReadSvc as TextFileReadService
participant EditorCtrl as SharedSchemaEditorController
Note over User,EditorCtrl: Load schema file flow
User->>View: selects file via hidden input
View->>Controller: loadSchemaFromFile(file)
Controller->>TransferSvc: readSchemaTextFile(file)
TransferSvc->>ReadSvc: readText(file)
ReadSvc-->>TransferSvc: raw text
TransferSvc-->>Controller: normalised text (BOM+CRLF stripped)
Controller->>EditorCtrl: "loadSchemaText(text, {showErrors:true, preferRowMode:true})"
EditorCtrl-->>Controller: "{rows, errors, applied}"
alt "applied === true"
Controller->>Controller: callbacks.onSchemaFileLoaded(...)
end
Controller-->>View: result
Note over User,EditorCtrl: Save schema file flow
User->>View: clicks Save Schema File
View->>Controller: saveSchemaToFile()
Controller->>EditorCtrl: getSchemaText()
EditorCtrl-->>Controller: schemaText
Controller->>TransferSvc: "downloadSchemaText(schemaText, {filename})"
TransferSvc->>TransferSvc: new Download(...).downloadFile(...)
Controller->>Controller: callbacks.onSchemaFileSaved(...)
Reviews (8): Last reviewed commit: "Normalize schema file read errors" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (1)
packages/core-ui/src/tests/shared/shared-schema-definition-view.test.js (1)
479-480: ⚡ Quick winReplace manual microtask flushing with assertion-based waiting to avoid flaky async tests.
Line 479-480 and Line 505-506 depend on current promise-chaining depth (
await Promise.resolve()twice). If the async path changes, these tests can intermittently fail despite correct behavior.♻️ Suggested test-stability patch
import { jest } from '`@jest/globals`'; import { JSDOM } from 'jsdom'; -import { fireEvent, within } from '`@testing-library/dom`'; +import { fireEvent, within, waitFor } from '`@testing-library/dom`'; @@ - fireEvent.change(fileInput); - await Promise.resolve(); - await Promise.resolve(); + fireEvent.change(fileInput); + await waitFor(() => { + expect(document.querySelectorAll('.shared-schema-row')).toHaveLength(2); + }); @@ - fireEvent.change(fileInput); - await Promise.resolve(); - await Promise.resolve(); + fireEvent.change(fileInput); + await waitFor(() => { + expect(document.querySelector('[data-role="schema-error"]').textContent.length).toBeGreaterThan(0); + });Also applies to: 505-506
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core-ui/src/tests/shared/shared-schema-definition-view.test.js` around lines 479 - 480, Replace the manual microtask flushing approach using await Promise.resolve() calls with assertion-based waiting that checks for actual DOM state or element conditions. At lines 479-480 (and the sibling location at lines 505-506), instead of relying on fixed Promise.resolve() chains, wait for specific DOM elements to appear, become visible, or reach an expected state using appropriate wait utilities or assertions. This makes the test more robust to changes in internal async behavior while ensuring the test actually validates meaningful state changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/web/src/tests/browser/app/functional/test-data/schema-file-load-save.spec.js`:
- Around line 16-17: The test assertions on getSchemaText() are executing
immediately without waiting for the async file-read and parse operations to
complete, causing intermittent CI failures due to race conditions. Replace the
immediate expect(await ...) checks at
apps/web/src/tests/browser/app/functional/test-data/schema-file-load-save.spec.js
lines 16-17 with polling assertions using expect.poll(...) or waitFor(...) to
ensure the schema text contains 'Loaded Name' and 'Loaded Status' after the
async operation completes. Apply the same polling pattern fix at
apps/web/src/tests/browser/generator/functional/schema-file-load-save.spec.js
lines 15-16 for the corresponding assertions on the generator flow.
In
`@packages/core-ui/js/gui_components/shared/schema-definition/shared-schema-definition-controller.js`:
- Around line 201-227: The loadSchemaFromFile method uses an inconsistent state
key when building error result objects. Both the early return on line 210 (when
fileTransferService is unavailable) and the catch block on line 226 (when file
reading fails) reference this.getState().rows, but the class uses schemaRows as
the actual state key elsewhere. Replace both instances of this.getState().rows
with this.getState().schemaRows to ensure the correct row payload is returned on
load failures.
In `@packages/core-ui/js/gui_components/shared/text-file-read-service.js`:
- Around line 12-43: The optional chaining in the readText method is incomplete
and will throw a TypeError when callbacks is null or undefined. The current
pattern callbacks.onX?.(args) checks if the method exists but not if callbacks
itself is defined. Fix this by adding an additional optional chaining operator
on the callbacks object itself, changing all instances of
callbacks.onProgress?.(event), callbacks.onLoad?.(text, event),
callbacks.onError?.(event), and callbacks.onAbort?.(event) to use
callbacks?.onProgress?.(event), callbacks?.onLoad?.(text, event),
callbacks?.onError?.(event), and callbacks?.onAbort?.(event) respectively. This
ensures the property access is guarded before attempting to invoke the callback
functions.
---
Nitpick comments:
In `@packages/core-ui/src/tests/shared/shared-schema-definition-view.test.js`:
- Around line 479-480: Replace the manual microtask flushing approach using
await Promise.resolve() calls with assertion-based waiting that checks for
actual DOM state or element conditions. At lines 479-480 (and the sibling
location at lines 505-506), instead of relying on fixed Promise.resolve()
chains, wait for specific DOM elements to appear, become visible, or reach an
expected state using appropriate wait utilities or assertions. This makes the
test more robust to changes in internal async behavior while ensuring the test
actually validates meaningful state changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 55b3a82e-6ea4-4211-b77d-dbfe667e2d26
📒 Files selected for processing (17)
apps/web/src/stories/shared-schema-definition.stories.jsapps/web/src/tests/browser/app/abstractions/components/test-data-panel.component.jsapps/web/src/tests/browser/app/functional/test-data/schema-file-load-save.spec.jsapps/web/src/tests/browser/generator/abstractions/components/generator-schema.component.jsapps/web/src/tests/browser/generator/functional/schema-file-load-save.spec.jsapps/web/src/tests/browser/shared/abstractions/components/schema-editor.component.jsapps/web/styles.cssdocs/frontend-component-migration-plan.mdpackages/core-ui/js/gui_components/app/import-export-adapters/file-read-service.jspackages/core-ui/js/gui_components/shared/schema-definition/index.jspackages/core-ui/js/gui_components/shared/schema-definition/shared-schema-definition-controller.jspackages/core-ui/js/gui_components/shared/schema-definition/shared-schema-definition-view.jspackages/core-ui/js/gui_components/shared/schema-file-transfer-service.jspackages/core-ui/js/gui_components/shared/test-data/schema/shared-schema-editor-controller.jspackages/core-ui/js/gui_components/shared/text-file-read-service.jspackages/core-ui/src/tests/shared/shared-schema-definition-controller.test.jspackages/core-ui/src/tests/shared/shared-schema-definition-view.test.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core-ui/src/tests/shared/schema-file-transfer-service.test.js (1)
30-36: ⚡ Quick winAvoid double-invoking
readSchemaTextFilein error-path tests.Each test currently calls the method twice (once for message, once for
cause), which can make failures harder to diagnose if behavior ever becomes stateful. Assert both in a single rejection expectation.Proposed test refactor
- await expect(service.readSchemaTextFile({ name: 'schema.txt' })).rejects.toThrow('Reading the schema file failed.'); - - try { - await service.readSchemaTextFile({ name: 'schema.txt' }); - } catch (error) { - expect(error.cause).toBe(fileReadError); - } + await expect(service.readSchemaTextFile({ name: 'schema.txt' })).rejects.toMatchObject({ + message: 'Reading the schema file failed.', + cause: fileReadError, + });- await expect(service.readSchemaTextFile({ name: 'schema.txt' })).rejects.toThrow( - 'Reading the schema file aborted.' - ); - - try { - await service.readSchemaTextFile({ name: 'schema.txt' }); - } catch (error) { - expect(error.cause).toBe(fileReadAbort); - } + await expect(service.readSchemaTextFile({ name: 'schema.txt' })).rejects.toMatchObject({ + message: 'Reading the schema file aborted.', + cause: fileReadAbort, + });Also applies to: 52-56
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core-ui/src/tests/shared/schema-file-transfer-service.test.js` around lines 30 - 36, The test for readSchemaTextFile method is invoking the service twice (once in the expect().rejects.toThrow() call and again in the try/catch block), which is inefficient and harder to diagnose if the behavior becomes stateful. Refactor to combine both assertions into a single rejection expectation: use expect().rejects.toThrow() but extend it to verify both that the error message matches 'Reading the schema file failed.' and that the error.cause property equals the fileReadError. Apply this same pattern to all instances of this test pattern in the file (including around lines 52-56) to ensure consistent and efficient error-path testing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core-ui/js/gui_components/shared/schema-file-transfer-service.js`:
- Around line 12-15: The `toSchemaFileReadError` function currently returns raw
`Error` instances directly when `error instanceof Error` is true, which bypasses
the service's normalized user-facing error contract that distinguishes between
`failed` and `aborted` states and guarantees `cause` wrapping. Instead of
returning the error directly on line 13, wrap the error according to the
service's standard error contract structure to ensure consistency with how other
error cases in this function are handled, maintaining the proper `failed` vs
`aborted` distinction and guaranteeing the `cause` field is set for standard
`Error` failures.
---
Nitpick comments:
In `@packages/core-ui/src/tests/shared/schema-file-transfer-service.test.js`:
- Around line 30-36: The test for readSchemaTextFile method is invoking the
service twice (once in the expect().rejects.toThrow() call and again in the
try/catch block), which is inefficient and harder to diagnose if the behavior
becomes stateful. Refactor to combine both assertions into a single rejection
expectation: use expect().rejects.toThrow() but extend it to verify both that
the error message matches 'Reading the schema file failed.' and that the
error.cause property equals the fileReadError. Apply this same pattern to all
instances of this test pattern in the file (including around lines 52-56) to
ensure consistent and efficient error-path testing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0cd77a14-8e05-4391-83b5-d80ab022eb2f
📒 Files selected for processing (6)
packages/core-ui/js/gui_components/shared/schema-definition/shared-schema-definition-controller.jspackages/core-ui/js/gui_components/shared/schema-file-transfer-service.jspackages/core-ui/js/gui_components/shared/test-data/schema/shared-schema-editor-controller.jspackages/core-ui/src/tests/shared/schema-file-transfer-service.test.jspackages/core-ui/src/tests/shared/shared-schema-definition-controller.test.jspackages/core-ui/src/tests/shared/shared-schema-definition-view.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core-ui/js/gui_components/shared/test-data/schema/shared-schema-editor-controller.js
eviltester
changed the title
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
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.
Summary
Why
Issue #209 asked for plain-text schema file import/export to be available consistently anywhere the shared schema editor is mounted, without duplicating page-host logic.
Impact
Users can now load an existing schema text file into the shared schema editor and save the current schema text back out from the app test-data panel, generator page, and writer-schema page.
Validation
pnpm run dev:webapp.html,generator.html, andwriter-schema.htmlpnpm run verify:uipnpm run test:browserpnpm run test:storybookpnpm run build-storybookpnpm testpnpm run verify:localSummary by CodeRabbit