feat(creator): add Load from File to YAML editor#299
feat(creator): add Load from File to YAML editor#299platex-rehor-bot wants to merge 2 commits intoRedHatInsights:masterfrom
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
WalkthroughUpdates add YAML transform for Jest, a comprehensive test for CreatorYAMLView, and extend CreatorYAMLView, CreatorWizard, and Creator to support loading, parsing, validating, and downloading YAML with shared quickstart/bundles/tags state and kind propagation; default quickstart template metadata is adjusted. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as CreatorYAMLView
participant FileInput as Hidden File Input
participant FR as FileReader
participant Parser as YAML Parser
participant State as Wizard/Component State
participant Download as downloadFile util
User->>UI: Click "Load from File"
UI->>UI: If editor has content -> show confirm
alt user confirms
UI->>FileInput: open picker
User->>FileInput: select .yaml/.yml
FileInput->>FR: provide File
FR->>FR: readAsText(file)
FR-->>UI: file text (or error)
UI->>Parser: parse YAML text
Parser-->>UI: parsed object or parse error
UI->>State: update editor value, quickStart, bundles, tags, kind
UI->>Download: enable/disable download button
else user cancels
UI-->>User: no change
end
User->>UI: Click "Download Files (n)"
UI->>Download: iterate wizard.files -> downloadFile(...) per file
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 51 minutes and 17 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/components/creator/CreatorYAMLView.test.tsx (2)
182-196: Consider also asserting the specific parser message surfaces.
YAML.parseerror messages are part of the user-visible alert ("{message}. Showing previous valid state in preview."). Asserting only on the static title "YAML Parse Error" means a regression that suppresses the dynamic reason would still pass. An additional assertion on the trailing sentence would tighten the test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/creator/CreatorYAMLView.test.tsx` around lines 182 - 196, The test in CreatorYAMLView.test.tsx only asserts the static title "YAML Parse Error" and should also assert the dynamic parser message from YAML.parse is rendered; update the 'shows parse error for invalid YAML' test that renders <CreatorYAMLView /> and fires the file change to additionally expect the specific parser message (or the full alert sentence like "{message}. Showing previous valid state in preview.") is present in the document so regressions that drop the dynamic reason will fail; locate the file input test that uses mockFile and add an assertion (e.g., regex matching the parser error text) after the waitFor that checks the alert body contains the YAML.parse error text.
198-206: Minor: empty-files test does not assert confirm was not invoked.The test validates that no callback fires, but the actual code path this exercises (
filefalsy → early return) is insidehandleFileSelected, which is only reached after the picker was already opened. Consider also assertingparseErrorstays unset and the editor value is unchanged, to guard against future regressions where an empty selection might mutate state.fireEvent.change(fileInput, { target: { files: [] } }); expect(onChangeSpec).not.toHaveBeenCalled(); + expect(screen.queryByText(/YAML Parse Error/i)).not.toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/creator/CreatorYAMLView.test.tsx` around lines 198 - 206, The test "does nothing when no file is selected" should also verify that the confirmation dialog wasn't invoked and that component state didn't change: mock/spy on global confirm (or window.confirm) and assert it was not called, and assert CreatorYAMLView's parseError remains unset and the editor value is unchanged after firing the change event (i.e., capture the editor's initial value from the rendered output before the event and compare it after). Locate the behavior in handleFileSelected and add these assertions alongside the existing onChangeQuickStartSpec check.src/components/creator/CreatorYAMLView.tsx (1)
153-183: Optional: extract the shared "confirm overwrite" flow.
handleLoadSampleandhandleLoadFromFileshare the same "if content then confirm" pattern with only the message and follow-up action differing. A small helper would reduce duplication and keep the messages in one place:♻️ Sketch
const confirmOverwriteIfDirty = (message: string): boolean => { if (yamlContent.trim() === '') return true; return window.confirm(message); }; const handleLoadSample = () => { if (!confirmOverwriteIfDirty('This will overwrite your current work. Are you sure?')) return; setYamlContent(DEFAULT_QUICKSTART_YAML); parseAndUpdateQuickstart(DEFAULT_QUICKSTART_YAML); }; const handleLoadFromFile = () => { if (!confirmOverwriteIfDirty('Loading a file will overwrite your current work. Are you sure?')) return; fileInputRef.current?.click(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/creator/CreatorYAMLView.tsx` around lines 153 - 183, Extract the duplicated "confirm overwrite" logic into a small helper like confirmOverwriteIfDirty(message: string): boolean that checks yamlContent.trim() === '' and returns true if empty or calls window.confirm(message) otherwise; then simplify handleLoadSample to return early if the helper returns false before calling setYamlContent(DEFAULT_QUICKSTART_YAML) and parseAndUpdateQuickstart(DEFAULT_QUICKSTART_YAML), and simplify handleLoadFromFile to return early if the helper returns false before calling fileInputRef.current?.click(); reference the symbols confirmOverwriteIfDirty, handleLoadSample, handleLoadFromFile, yamlContent, setYamlContent, parseAndUpdateQuickstart, and fileInputRef in your changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/creator/CreatorYAMLView.tsx`:
- Around line 185-203: The file-read operation in handleFileSelected currently
omits FileReader error handling so read failures are silent; add reader.onerror
(and optionally reader.onabort) handlers to call the existing parse error
pathway (e.g., setParseError or the same alert used by parseAndUpdateQuickstart)
with a clear message and underlying error info, clear or reset yaml state if
appropriate, and log the error for diagnostics; keep the current success flow
using setYamlContent and parseAndUpdateQuickstart in reader.onload.
- Around line 170-183: The confirm dialog in handleLoadFromFile fires because
yamlContent contains the initial placeholder comments; update the guard to only
ask for confirmation when the user has actually modified the editor by either
(a) introducing a constant INITIAL_YAML_PLACEHOLDER (matching the initial
yamlContent value) and change the check to skip confirmation when currentContent
=== '' or currentContent === INITIAL_YAML_PLACEHOLDER, or (b) add a dirty
boolean (set true in handleEditorChange) and only prompt when dirty is true;
modify handleLoadFromFile to reference the chosen symbol
(INITIAL_YAML_PLACEHOLDER or dirty) and leave handleLoadSample behavior
consistent with this change.
---
Nitpick comments:
In `@src/components/creator/CreatorYAMLView.test.tsx`:
- Around line 182-196: The test in CreatorYAMLView.test.tsx only asserts the
static title "YAML Parse Error" and should also assert the dynamic parser
message from YAML.parse is rendered; update the 'shows parse error for invalid
YAML' test that renders <CreatorYAMLView /> and fires the file change to
additionally expect the specific parser message (or the full alert sentence like
"{message}. Showing previous valid state in preview.") is present in the
document so regressions that drop the dynamic reason will fail; locate the file
input test that uses mockFile and add an assertion (e.g., regex matching the
parser error text) after the waitFor that checks the alert body contains the
YAML.parse error text.
- Around line 198-206: The test "does nothing when no file is selected" should
also verify that the confirmation dialog wasn't invoked and that component state
didn't change: mock/spy on global confirm (or window.confirm) and assert it was
not called, and assert CreatorYAMLView's parseError remains unset and the editor
value is unchanged after firing the change event (i.e., capture the editor's
initial value from the rendered output before the event and compare it after).
Locate the behavior in handleFileSelected and add these assertions alongside the
existing onChangeQuickStartSpec check.
In `@src/components/creator/CreatorYAMLView.tsx`:
- Around line 153-183: Extract the duplicated "confirm overwrite" logic into a
small helper like confirmOverwriteIfDirty(message: string): boolean that checks
yamlContent.trim() === '' and returns true if empty or calls
window.confirm(message) otherwise; then simplify handleLoadSample to return
early if the helper returns false before calling
setYamlContent(DEFAULT_QUICKSTART_YAML) and
parseAndUpdateQuickstart(DEFAULT_QUICKSTART_YAML), and simplify
handleLoadFromFile to return early if the helper returns false before calling
fileInputRef.current?.click(); reference the symbols confirmOverwriteIfDirty,
handleLoadSample, handleLoadFromFile, yamlContent, setYamlContent,
parseAndUpdateQuickstart, and fileInputRef in your 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 9afd8dd2-edd2-44ea-bf0a-c24ab8dc1846
📒 Files selected for processing (3)
jest.config.jssrc/components/creator/CreatorYAMLView.test.tsxsrc/components/creator/CreatorYAMLView.tsx
|
@platex-rehor-bot this is a UI change, can you provide screenshots? |
|
@Hyperkid123 Here are the screenshots of the "Load from File" button added to the Creator YAML editor: Default state — empty editor with both buttons
After clicking "Load Sample Template" — editor populated with YAML content
The new "Load from File" button (with the upload icon) sits next to the existing "Load Sample Template" button. Clicking it opens a native file picker filtered to Note: Screenshots taken via Storybook with PatternFly 6 styling. The component renders identically in the full HCC environment. |
platex-rehor-bot
force-pushed
the
bot/RHCLOUD-40758
branch
from
605e8c5 to
21b1a26
Compare
|
@platex-rehor-bot, the editor seems to be loading the content nicely. However, we do miss the same from the wizard in which we download the required files. If we upload the yaml, make changes, we would like to download the files the same way we do in the wizard. |
|
@Hyperkid123 Good catch — added a Download Files button to the YAML editor toolbar in 7467d84. It works the same way as the wizard download:
7 new unit tests cover the download flows (button state, file generation, fallback name, tag handling). |
|
@platex-rehor-bot ok that works. There are a few things missing. The yaml/wizard should share the same underlying yaml schema. So I should be able to switch between the two and not lose any data. The yaml creator is also missing the metadata file creation that is in the wizard form. We also want to be able to create/edit these. But they have to be validated to only use the supported values and attributes. |
RHCLOUD-40758 Add Creator YAML editor with bidirectional state sync between wizard and YAML tabs, schema validation, metadata file creation, and file load/download support. - Wizard → YAML: switching to YAML tab serializes current wizard state (spec, bundles, tags) into the editor - YAML → Wizard: switching back populates wizard form fields from shared state via initialValues - Kind detection: extracts ItemKind from spec.type.text, propagates to parent for correct metadata generation - Schema validation: warns on missing required fields, unknown tag kinds, unknown spec fields, and unrecognized types - File operations: Load from File, Load Sample Template, Download Files (uses parent-generated files for consistency with wizard) - Sample template updated with metadata tags (bundle, content) - 27 unit tests covering sync, validation, and file operations Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
platex-rehor-bot
force-pushed
the
bot/RHCLOUD-40758
branch
from
7467d84 to
06b3366
Compare
|
@Hyperkid123 Thanks for the feedback! Addressed all three points in 06b3366: 1. Shared schema — wizard ↔ YAML sync:
2. Metadata file creation in YAML mode:
3. Validation for supported values/attributes:
All previous commits squashed into one atomic commit. 27 tests passing, lint clean. |
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)
src/components/creator/CreatorYAMLView.tsx (1)
296-357:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPersist the full loaded YAML metadata, not just tags/spec.
This parser reads
metadata.name, but it never sends that name upstream—only bundles, tags, andspecare propagated. After a file upload, switching tabs reserializes from shared state and silently rewrites the identifier instead of preserving the uploaded YAML. The same pattern drops any other metadata/top-level fields that are not explicitly forwarded.A safer shape here is to lift a full
ExtendedQuickstart/metadata update callback into shared state and use that as the round-trip source of truth, rather than reconstructing from partial callbacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/creator/CreatorYAMLView.tsx` around lines 296 - 357, The parser builds quickstartObj but only forwards parts (bundles, tags, spec) so uploaded metadata.name and other top-level fields are lost; fix by adding/using a single callback that lifts the full ExtendedQuickstart into shared state (e.g., call onChangeQuickstart(quickstartObj) or add that prop) instead of only calling onChangeMetadataTags/onChangeQuickStartSpec/onChangeBundles/onChangeTags, ensure you pass quickstartObj (with metadata and spec) so detectKind(spec) still runs and preserve metadata.name and any extra metadata fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/creator/CreatorYAMLView.tsx`:
- Around line 244-258: getInitialYaml currently only serializes when
quickStart.spec.displayName exists, causing drafts with other fields (kind,
tags, description, tasks) to fall back to PLACEHOLDER_YAML; update
getInitialYaml so it serializes whenever a quickStart draft actually has spec
content (e.g., quickStart is present and either spec has any keys or any of
spec.displayName, spec.description, spec.tasks, or quickStart.spec.tags are set)
by calling serializeToYaml(quickStart, currentBundles || [], currentTags || {})
in that case and only returning PLACEHOLDER_YAML when there is no
quickStart/spec content at all; reference getInitialYaml, quickStart,
serializeToYaml, currentBundles, currentTags, and PLACEHOLDER_YAML.
- Around line 38-50: detectKind currently only matches meta.displayName exactly
(case-insensitive) so "Quick start" with a space doesn't match your existing
kind; update detectKind to normalize both the incoming typeText and each
meta.displayName by removing spaces (and lowercasing) before comparing (or
special-case typeText === 'quick start' to map to the intended kind) so the
built-in sample ("Quick start") resolves to the correct ItemKind from
ALL_KIND_ENTRIES and currentKind is not left null.
---
Outside diff comments:
In `@src/components/creator/CreatorYAMLView.tsx`:
- Around line 296-357: The parser builds quickstartObj but only forwards parts
(bundles, tags, spec) so uploaded metadata.name and other top-level fields are
lost; fix by adding/using a single callback that lifts the full
ExtendedQuickstart into shared state (e.g., call
onChangeQuickstart(quickstartObj) or add that prop) instead of only calling
onChangeMetadataTags/onChangeQuickStartSpec/onChangeBundles/onChangeTags, ensure
you pass quickstartObj (with metadata and spec) so detectKind(spec) still runs
and preserve metadata.name and any extra metadata fields.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: fb9057ed-04e3-469e-8ce5-41b0ec0568ae
📒 Files selected for processing (6)
jest.config.jssrc/Creator.tsxsrc/components/creator/CreatorWizard.tsxsrc/components/creator/CreatorYAMLView.test.tsxsrc/components/creator/CreatorYAMLView.tsxsrc/data/quickstart-templates.ts
✅ Files skipped from review due to trivial changes (1)
- jest.config.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/creator/CreatorYAMLView.test.tsx
RHCLOUD-40758 - detectKind() now normalizes whitespace so "Quick start" matches "Quickstart" - getInitialYaml() checks all wizard fields, not just displayName - Remove incorrect apiVersion assertion from Cypress test
Re: metadata.name round-trip (outside-diff comment)Good catch by @coderabbitai — when a YAML file is loaded, However, this is by design in the current architecture:
Making Since this PR's scope is adding YAML file preview/import, I'll leave the current behavior as-is. The uploaded name does appear correctly in the editor and preview, it just doesn't survive a tab switch (same as pre-existing behavior with the wizard). If we want full round-trip metadata fidelity, that would be a good follow-up ticket. |
|
Tip For best results, initiate chat on the files or code changes.
Would you like me to open a follow-up GitHub issue to track full
Just say the word and I'll create it.
|
2 participants
Summary
.yaml/.ymlfiles from disk into the Monaco editor for preview and editingparseAndUpdateQuickstart()flowCreatorYAMLViewcovering both file loading and sample template flowsChanges: 3 files —
CreatorYAMLView.tsx(component),CreatorYAMLView.test.tsx(new tests),jest.config.js(addyamlto transform allowlist)RHCLOUD-40758
Test plan
.yamlfile → content loads into editor, preview updatesnpm test)🤖 Generated with Claude Code