feat(creator): add Load from File to YAML editor#299
Open
platex-rehor-bot wants to merge 2 commits intoRedHatInsights:masterfrom
Open
feat(creator): add Load from File to YAML editor#299platex-rehor-bot wants to merge 2 commits intoRedHatInsights:masterfrom
platex-rehor-bot wants to merge 2 commits intoRedHatInsights:masterfrom
Conversation
RHCLOUD-40758 Add ability to load existing YAML files into the quickstart creator YAML editor via a file picker. Includes overwrite confirmation dialog, FileReader-based loading, and YAML parse/preview on load. Also adds 12 new unit tests for CreatorYAMLView covering both Load from File and Load Sample Template flows.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 49 seconds. ⌛ 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)
WalkthroughThis PR adds file-loading capability to the CreatorYAMLView component. The Jest configuration is updated to transform the Changes
Sequence DiagramsequenceDiagram
participant User
participant Component as CreatorYAMLView
participant FileAPI as File Input API
participant YAMLParser as YAML Parser
participant State as Component State
User->>Component: Click upload button
alt Editor has content
Component->>User: Show confirmation dialog
User->>Component: Confirm or cancel
end
Component->>FileAPI: Open file picker
User->>FileAPI: Select .yaml/.yml file
FileAPI->>Component: Return file object
Component->>FileAPI: Read file as text
FileAPI->>Component: Return file content
Component->>YAMLParser: Parse YAML content
YAMLParser->>Component: Return parsed data
Component->>State: Update editor value
Component->>State: Update quickstart spec
Component->>State: Update bundles/tags
Component->>FileAPI: Reset file input
Component->>User: Render updated editor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. 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
RHCLOUD-40758 - Extract PLACEHOLDER_YAML constant and isUserContent() helper to skip confirm dialog on fresh editor (placeholder content is not user work) - Add confirmOverwriteIfDirty() shared helper to deduplicate the confirm pattern between handleLoadSample and handleLoadFromFile - Add FileReader.onerror handler to surface read failures via parseError - Add FileReader error test, strengthen parse error assertions, verify confirm is not invoked on empty file selection
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
1 participant
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
.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