feat(git-service): add PR button to create and update quickstarts - #357
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
Summary by CodeRabbit
WalkthroughThe creator now supports feature-gated repository quickstart loading, metadata editing, and pull-request creation or updates. It adds source-step form support, API utilities, YAML editor states, alerts, retry handling, and automated coverage. ChangesGit-service quickstart workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
actor Author
participant CreatorYAMLView
participant useCreatePR
participant QuickstartAPI
Author->>CreatorYAMLView: Select repository quickstart
CreatorYAMLView->>QuickstartAPI: List and retrieve quickstart content
QuickstartAPI-->>CreatorYAMLView: Return YAML content
Author->>CreatorYAMLView: Create pull request
CreatorYAMLView->>useCreatePR: Submit generated files
useCreatePR->>QuickstartAPI: Check existence and create or update pull request
QuickstartAPI-->>useCreatePR: Return pull-request result
useCreatePR-->>CreatorYAMLView: Update success or error state
CreatorYAMLView-->>Author: Display result or retry action
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
e58d766 to
dbbcba3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Creator.tsx (1)
185-201: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the repository quickstart metadata name in the generated files
SourceSelectorpasses the repository name throughmetadata.name, butfilesreplaces it with a slug ofspec.displayName.FileDownloadthen derivesquickstartNamefrom that slug,useCreatePRchecks and reports it as missing when it differs frommetadata.name, and the request is sent asisUpdate: false. UsequickStart.metadata.namewhen available and only fall back to the display-name slug for scratch/blank entries.🤖 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 `@src/Creator.tsx` around lines 185 - 201, Update the files useMemo in Creator to preserve quickStart.metadata.name when it is available, using the existing displayName-derived slug only for scratch or blank entries without metadata.name. Ensure adjustedQuickstart.metadata.name and downstream FileDownload/useCreatePR flows receive the repository name unchanged.
🧹 Nitpick comments (7)
src/components/creator/SourceSelector.tsx (2)
236-238: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the inline scroll style into the stylesheet.
Lines 238 uses an inline
styleobject formaxHeightandoverflowY. The cohort already addsCreatorYAMLView.scss. Put the rule in a stylesheet class for consistency and theming.🤖 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 `@src/components/creator/SourceSelector.tsx` around lines 236 - 238, Replace the inline maxHeight and overflowY style on the results container in SourceSelector with a dedicated stylesheet class, then add the corresponding rule to the existing CreatorYAMLView.scss stylesheet and apply that class to the container.
42-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffKind detection depends on display-name text and breaks silently.
detectKindFromSpecmatchesspec.type.textagainstmeta.displayNameafter normalization. The stored YAML holds the display label, not the kind identifier. If a display name is changed inmeta.ts, every existing repository quickstart stops resolving to a kind, and the function returnsnullwithout any message. The wizard then shows the kind step unset while the other fields are populated.Consider matching on a stable identifier, and set the error state when detection fails.
🤖 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 `@src/components/creator/SourceSelector.tsx` around lines 42 - 57, Update detectKindFromSpec to resolve spec.type.text using a stable kind identifier rather than meta.displayName, while preserving normalization as needed for stored YAML values. When no kind matches, propagate the failure to the wizard’s error state so an unresolved kind is surfaced instead of leaving the kind step unset.src/components/creator/steps/source.tsx (1)
4-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
NAME_SOURCEtosteps/common.ts.Every other field-name constant lives in
src/components/creator/steps/common.ts, for exampleNAME_KIND,NAME_TAGS, and the newNAME_METADATA_NAME.NAME_SOURCEis declared here instead. Keep field names in one module so consumers import from a single place.🤖 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 `@src/components/creator/steps/source.tsx` around lines 4 - 5, Move the NAME_SOURCE constant from the source step module into steps/common.ts alongside NAME_KIND, NAME_TAGS, and NAME_METADATA_NAME, then update all consumers to import it from the common module while leaving STEP_SOURCE in place.src/components/creator/CreatorWizard.tsx (1)
301-334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicated download button.
Both branches of the
showGitServiceconditional render the same "Download all"Buttonwith the same props. Only the extra Create PR button differs. Render the download button once and render the Create PR button conditionally.♻️ Proposed refactor
<StackItem> - {showGitService ? ( - <Flex spaceItems={{ default: 'spaceItemsSm' }}> - <FlexItem> - <Button - variant="primary" - icon={<DownloadIcon />} - onClick={() => files.forEach((file) => doDownload(file))} - > - Download all ({files.length}) files - </Button> - </FlexItem> - <FlexItem> - <Button - variant="primary" - icon={prLoading ? undefined : <CodeBranchIcon />} - onClick={handleCreatePR} - isDisabled={!canCreatePR || prLoading} - isLoading={prLoading} - > - {prLoading ? 'Creating PR...' : 'Create PR'} - </Button> - </FlexItem> - </Flex> - ) : ( - <Button - variant="primary" - icon={<DownloadIcon />} - onClick={() => files.forEach((file) => doDownload(file))} - > - Download all ({files.length}) files - </Button> - )} + <Flex spaceItems={{ default: 'spaceItemsSm' }}> + <FlexItem> + <Button + variant="primary" + icon={<DownloadIcon />} + onClick={() => files.forEach((file) => doDownload(file))} + > + Download all ({files.length}) files + </Button> + </FlexItem> + {showGitService && ( + <FlexItem> + <Button + variant="primary" + icon={prLoading ? undefined : <CodeBranchIcon />} + onClick={handleCreatePR} + isDisabled={!canCreatePR || prLoading} + isLoading={prLoading} + > + {prLoading ? 'Creating PR...' : 'Create PR'} + </Button> + </FlexItem> + )} + </Flex> </StackItem>🤖 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 `@src/components/creator/CreatorWizard.tsx` around lines 301 - 334, Refactor the showGitService rendering in CreatorWizard so the shared “Download all” Button is rendered once, while the Create PR Button remains conditionally rendered only when showGitService is true. Preserve the existing button props, download handler, and Create PR loading/disabled behavior.src/components/creator/useCreatePR.ts (1)
27-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the
'untitled-quickstart'sentinel into a shared constant.The literal
'untitled-quickstart'appears at line 20, line 30, and inquickstartExistsinsrc/utils/createQuickstartPR.ts(line 28). Three copies of the same magic value can drift. Export one constant fromsrc/utils/createQuickstartPR.tsand import it here.🤖 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 `@src/components/creator/useCreatePR.ts` around lines 27 - 30, Define and export a shared constant for the 'untitled-quickstart' sentinel in createQuickstartPR.ts, replace that file’s matching literals with the constant, and import it into useCreatePR.ts for the canCreatePR check and any nearby comparison. Remove the duplicated string literals while preserving the existing behavior.src/Creator.tsx (1)
163-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated tag-flattening logic in
src/Creator.tsx. The same block that sortsbundles, maps them to{ kind: 'bundle', value }, and then appends every entry oftagsappears three times in this file. The three copies must stay in agreement, because the preview, the generatedmetadata.yaml, and the quickstart YAML must all carry identical tags.
src/Creator.tsx#L163-L183: extract the flattening into one helper, for examplebuildAllTags(bundles, tags), and call it from thequickStartmemo.src/Creator.tsx#L141-L149: replace the inline copy insidesetKindwith a call to the same helper.src/Creator.tsx#L203-L211: replace the inline copy inside thefilesmemo with a call to the same helper.🤖 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 `@src/Creator.tsx` around lines 163 - 183, Extract the shared bundle-and-tag flattening logic into a single helper such as buildAllTags(bundles, tags), preserving bundle sorting and tag appending order. Replace the duplicated inline implementations in setKind, the quickStart memo, and the files memo with calls to this helper so preview, metadata.yaml, and quickstart YAML remain identical.src/utils/createQuickstartPR.ts (1)
40-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSurface the server error detail on PR creation failure.
createQuickstartPRlets the raw Axios error propagate.useCreatePRrenderserr.message, which is generic text such as "Request failed with status code 422". The user cannot see why the Git service rejected the request. Extract the server message when it is present.♻️ Optional refactor
export const createQuickstartPR = async ( files: PRFile[], metadata: PRMetadata ): Promise<PRResponse> => { - const { data } = await axios.post<{ data: PRResponse }>( - `${API_BASE}/pull-request`, - { files, metadata } - ); - return data.data; + try { + const { data } = await axios.post<{ data: PRResponse }>( + `${API_BASE}/pull-request`, + { files, metadata } + ); + return data.data; + } catch (err) { + if (axios.isAxiosError(err)) { + const detail = + (err.response?.data as { message?: string } | undefined)?.message ?? + err.message; + throw new Error(detail); + } + throw err; + } };🤖 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 `@src/utils/createQuickstartPR.ts` around lines 40 - 49, Update createQuickstartPR to catch Axios request failures, extract the server-provided error message from the response when available, and rethrow or propagate that detail so useCreatePR displays it instead of only the generic Axios status message; preserve the existing successful response handling.
🤖 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 `@src/components/creator/CreatorWizard.tsx`:
- Around line 453-456: Update the useMemo dependency array for the schema
created by makeSchema to include both filterData and chrome alongside
showGitService, ensuring the schema refreshes when loaded filters or bundle data
change. If intentionally omitting either dependency to avoid remounting,
document that rationale in a comment near the memo, consistent with the existing
initialValues comment.
- Around line 247-255: Update the useCreatePR call in FileDownload to pass
quickstartName only when showGitService is enabled, and pass null otherwise.
Preserve the hook’s existing null handling so quickstartExists is not invoked
while the feature flag is off.
- Around line 314-322: Update the useCreatePR result destructuring in
CreatorWizard to include isUpdate, then use it in the Button label so existing
quickstarts display an update-specific action while new quickstarts retain
“Create PR”. Preserve the existing prLoading label and behavior.
- Around line 336-376: Replace the plain close Buttons in the success and danger
alerts within the CreatorWizard render with PatternFly’s AlertActionCloseButton,
wiring onClose to setPrResult(null) and setPrError(null) respectively while
preserving the existing alert behavior.
In `@src/components/creator/CreatorYAMLView.test.tsx`:
- Around line 904-912: Update the mocked repository fixture in CreatorYAMLView
tests so getting-started.yml contains only the spec fields, while metadata.yaml
contains the metadata fields; adjust the assertions to verify the editor content
includes metadata merged from metadata.yaml.
In `@src/components/creator/CreatorYAMLView.tsx`:
- Around line 442-456: The useCreatePR hook currently performs PR lookups
regardless of the showCreatePR flag. Update useCreatePR and its call site in
CreatorYAMLView to accept the feature-flag state, suppress lookup requests and
related PR state when disabled, and preserve the existing behavior when the flag
is enabled.
- Around line 723-747: Update handleLoadFromRepo in
src/components/creator/CreatorYAMLView.tsx (lines 723-747) to parse
metadata.yaml and merge its metadata with the selected quickstart YAML before
setting editor content, while preserving the existing normalization behavior.
Update the fixture and assertions in
src/components/creator/CreatorYAMLView.test.tsx (lines 904-912) to place
metadata in metadata.yaml and verify the merged editor content retains it.
- Around line 1022-1025: Update the CreatorYAMLView PR submission state around
handleCreatePR so validationWarnings indicating missing spec.displayName or
spec.description are treated as blocking failures. Include that required-field
validation state in the button’s isDisabled condition while preserving the
existing parse-error, download, and loading checks.
- Around line 446-456: Update the Create PR flow in CreatorYAMLView and
useCreatePR so isUpdate remains unresolved until the quickstartExists lookup for
the current parsedName completes, disabling submission while it is pending.
Ignore stale lookup results from previous parsedName values, and only allow
handleCreatePR after the current existence check has completed with the correct
isUpdate value.
- Around line 566-569: Update CreatorYAMLView’s parsed-name flow to forward the
canonical metadata.name through CreatorWizard to its parent, then use that value
in Creator.tsx file generation and useCreatePR directory selection. Replace the
independent spec.displayName-derived name for generated files while preserving
the existing parsedName synchronization behavior.
In `@src/components/creator/SourceSelector.tsx`:
- Around line 207-214: Update the source-selection form group around the “Select
source” label so the label is associated with the DataList via a uniquely
identified element and matching aria-labelledby (or use PatternFly FormGroup).
In the source option button rendering near the repository and other source
entries, add aria-pressed reflecting whether each option’s name equals the
selected value, preserving the existing selection logic and styling.
- Around line 89-97: Update the filtering callback in the filtered useMemo to
guard qs.displayName before calling toLowerCase, while continuing to match
qs.name and available displayName values against search. Preserve the existing
behavior of the quickstart list and its dependency array.
- Around line 120-128: Update the YAML handling in SourceSelector’s selection
flow so both a missing quickstart YAML file and a falsy YAML.parse result set
the component’s error state before returning. Preserve the existing early-return
behavior while ensuring the selected item is reported as invalid rather than
silently proceeding.
- Around line 115-196: Update handleSelectRepo to invoke the same form-reset
operation used by handleSelectScratch before applying the newly loaded
repository quickstart. Perform the reset at the start of the load, before
setting the repository name or populating parsed metadata and spec fields, so
omitted values cannot persist from a prior selection.
In `@src/components/creator/useCreatePR.ts`:
- Around line 19-25: Update the useEffect around quickstartExists to track
whether the effect is still active, ignore results from stale quickstartName
values, and prevent updates after unmount. Attach a catch handler that preserves
a safe isUpdate state on lookup failure, and clean up by marking the effect
inactive when dependencies change or the component unmounts.
- Around line 41-48: Update the co-author trailer logic in the Create PR flow
around useCreatePR to avoid publishing identity.email unless public disclosure
is explicitly approved. Prefer the existing approved no-reply identity or omit
the trailer; if email disclosure remains supported, add a clear user-facing
disclosure and opt-out through the PR button or related tooltips.
- Around line 49-61: Sanitize quickstartName before constructing the
createQuickstartPR options, assigning the restricted value to safeName. Use
safeName for branchName, directoryName, and the conditional existingPath while
preserving the existing naming and update behavior.
In `@src/utils/createQuickstartPR.ts`:
- Around line 61-66: Update listRepoQuickstarts to default the response’s
missing data.data.quickstarts value to an empty array before returning it, while
preserving the existing array when present so SourceSelector can safely call
filter.
- Around line 27-38: Update quickstartExists to propagate axios lookup failures
instead of returning false, while preserving false for invalid or absent
quickstart names and true/false based on exact query results. Adjust useCreatePR
to handle the propagated failure state by waiting or displaying an error, and
prevent PR creation until the lookup succeeds.
---
Outside diff comments:
In `@src/Creator.tsx`:
- Around line 185-201: Update the files useMemo in Creator to preserve
quickStart.metadata.name when it is available, using the existing
displayName-derived slug only for scratch or blank entries without
metadata.name. Ensure adjustedQuickstart.metadata.name and downstream
FileDownload/useCreatePR flows receive the repository name unchanged.
---
Nitpick comments:
In `@src/components/creator/CreatorWizard.tsx`:
- Around line 301-334: Refactor the showGitService rendering in CreatorWizard so
the shared “Download all” Button is rendered once, while the Create PR Button
remains conditionally rendered only when showGitService is true. Preserve the
existing button props, download handler, and Create PR loading/disabled
behavior.
In `@src/components/creator/SourceSelector.tsx`:
- Around line 236-238: Replace the inline maxHeight and overflowY style on the
results container in SourceSelector with a dedicated stylesheet class, then add
the corresponding rule to the existing CreatorYAMLView.scss stylesheet and apply
that class to the container.
- Around line 42-57: Update detectKindFromSpec to resolve spec.type.text using a
stable kind identifier rather than meta.displayName, while preserving
normalization as needed for stored YAML values. When no kind matches, propagate
the failure to the wizard’s error state so an unresolved kind is surfaced
instead of leaving the kind step unset.
In `@src/components/creator/steps/source.tsx`:
- Around line 4-5: Move the NAME_SOURCE constant from the source step module
into steps/common.ts alongside NAME_KIND, NAME_TAGS, and NAME_METADATA_NAME,
then update all consumers to import it from the common module while leaving
STEP_SOURCE in place.
In `@src/components/creator/useCreatePR.ts`:
- Around line 27-30: Define and export a shared constant for the
'untitled-quickstart' sentinel in createQuickstartPR.ts, replace that file’s
matching literals with the constant, and import it into useCreatePR.ts for the
canCreatePR check and any nearby comparison. Remove the duplicated string
literals while preserving the existing behavior.
In `@src/Creator.tsx`:
- Around line 163-183: Extract the shared bundle-and-tag flattening logic into a
single helper such as buildAllTags(bundles, tags), preserving bundle sorting and
tag appending order. Replace the duplicated inline implementations in setKind,
the quickStart memo, and the files memo with calls to this helper so preview,
metadata.yaml, and quickstart YAML remain identical.
In `@src/utils/createQuickstartPR.ts`:
- Around line 40-49: Update createQuickstartPR to catch Axios request failures,
extract the server-provided error message from the response when available, and
rethrow or propagate that detail so useCreatePR displays it instead of only the
generic Axios status message; preserve the existing successful response
handling.
🪄 Autofix
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: Pro Plus
Run ID: 96b36ac0-69e8-4aa0-bde5-e93691794449
📒 Files selected for processing (16)
cypress/component/CreatorYAMLView.cy.tsxsrc/Creator.tsxsrc/components/creator/CreatorWizard.test.tsxsrc/components/creator/CreatorWizard.tsxsrc/components/creator/CreatorYAMLView.scsssrc/components/creator/CreatorYAMLView.test.tsxsrc/components/creator/CreatorYAMLView.tsxsrc/components/creator/SourceSelector.tsxsrc/components/creator/meta.tssrc/components/creator/schema.tsxsrc/components/creator/steps/common.tssrc/components/creator/steps/kind.tsxsrc/components/creator/steps/source.tsxsrc/components/creator/useCreatePR.tssrc/utils/createQuickstartPR.test.tssrc/utils/createQuickstartPR.ts
| const { | ||
| prLoading, | ||
| prResult, | ||
| prError, | ||
| canCreatePR, | ||
| handleCreatePR, | ||
| setPrResult, | ||
| setPrError, | ||
| } = useCreatePR(quickstartName); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Skip the existence lookup when the feature flag is off.
useCreatePR(quickstartName) runs on every render of FileDownload, including when showGitService is false. The hook effect then calls quickstartExists, which issues a GET to /api/quickstarts/v1/quickstarts for every user who reaches the download step. The flag is disabled in all environments today, so this request has no purpose and its result is never shown.
Pass null when the flag is off. The hook already treats null as "no lookup".
🛠️ Proposed fix
} = useCreatePR(quickstartName);- } = useCreatePR(quickstartName);
+ } = useCreatePR(showGitService ? quickstartName : 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.
| const { | |
| prLoading, | |
| prResult, | |
| prError, | |
| canCreatePR, | |
| handleCreatePR, | |
| setPrResult, | |
| setPrError, | |
| } = useCreatePR(quickstartName); | |
| const { | |
| prLoading, | |
| prResult, | |
| prError, | |
| canCreatePR, | |
| handleCreatePR, | |
| setPrResult, | |
| setPrError, | |
| } = useCreatePR(showGitService ? quickstartName : null); |
🤖 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 `@src/components/creator/CreatorWizard.tsx` around lines 247 - 255, Update the
useCreatePR call in FileDownload to pass quickstartName only when showGitService
is enabled, and pass null otherwise. Preserve the hook’s existing null handling
so quickstartExists is not invoked while the feature flag is off.
| <Button | ||
| variant="primary" | ||
| icon={prLoading ? undefined : <CodeBranchIcon />} | ||
| onClick={handleCreatePR} | ||
| isDisabled={!canCreatePR || prLoading} | ||
| isLoading={prLoading} | ||
| > | ||
| {prLoading ? 'Creating PR...' : 'Create PR'} | ||
| </Button> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The button label does not state whether the action updates an existing quickstart.
useCreatePR returns isUpdate, and the hook builds a different branch name, commit message, and PR body for updates. The button always reads "Create PR". When the quickstart already exists, the action opens an update pull request. The label misleads the user about the effect.
The hook already exposes isUpdate. Use it in the label.
🛠️ Proposed change
- {prLoading ? 'Creating PR...' : 'Create PR'}
+ {prLoading
+ ? 'Creating PR...'
+ : isUpdate
+ ? 'Update PR'
+ : 'Create PR'}Add isUpdate to the destructured hook result at lines 247-255.
🤖 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 `@src/components/creator/CreatorWizard.tsx` around lines 314 - 322, Update the
useCreatePR result destructuring in CreatorWizard to include isUpdate, then use
it in the Button label so existing quickstarts display an update-specific action
while new quickstarts retain “Create PR”. Preserve the existing prLoading label
and behavior.
| const yamlContent = | ||
| 'metadata:\n name: getting-started\nspec:\n displayName: GS\n'; | ||
| mockedGetRepoQuickstartContent.mockResolvedValueOnce({ | ||
| name: 'getting-started', | ||
| files: [ | ||
| { name: 'metadata.yaml', content: 'kind: QuickStarts\n' }, | ||
| { name: 'getting-started.yml', content: yamlContent }, | ||
| ], | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the split-file repository fixture.
This fixture puts metadata in getting-started.yml. Repository quickstarts use a separate metadata.yaml, so this test cannot detect that the loader discards metadata. Make the quickstart YAML spec-only and assert that the editor contains merged metadata from metadata.yaml.
🤖 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 `@src/components/creator/CreatorYAMLView.test.tsx` around lines 904 - 912,
Update the mocked repository fixture in CreatorYAMLView tests so
getting-started.yml contains only the spec fields, while metadata.yaml contains
the metadata fields; adjust the assertions to verify the editor content includes
metadata merged from metadata.yaml.
| const user = await chrome.auth.getUser(); | ||
| const identity = user?.identity?.user; | ||
| if (identity?.email) { | ||
| const name = | ||
| [identity.first_name, identity.last_name].filter(Boolean).join(' ') || | ||
| identity.email; | ||
| commitMessage += `\n\nCo-authored-by: ${name} <${identity.email}>`; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Candidate files:"
fd -a 'useCreatePR\.ts$|Create.*PR|create.*pr|Quickstart|Quickstarts' . | sed 's#^\./##' | head -100
echo
echo "Target file outline:"
ast-grep outline src/components/creator/useCreatePR.ts --view compact || true
echo
echo "Target file relevant snippet:"
cat -n src/components/creator/useCreatePR.ts | sed -n '1,140p'
echo
echo "Search for useCreatePR and PR creation UI:"
rg -n "useCreatePR|Create PR|create.*PR|commitMessage|Co-authored-by|identity\.email|identity\.first_name|identity\.last_name|RedHatInsights/quickstarts|quickstarts" -S . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' | head -300Repository: RedHatInsights/learning-resources
Length of output: 30781
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "CreatorWizard PR-related section:"
cat -n src/components/creator/CreatorWizard.tsx | sed -n '220,335p'
echo
echo "CreatorYAMLView PR-related section:"
cat -n src/components/creator/CreatorYAMLView.tsx | sed -n '900,1035p'
echo
echo "Tests around Co-authored-by:"
cat -n src/components/creator/CreatorYAMLView.test.tsx | sed -n '760,805p'
echo
echo "Find all Co-authored-by and email disclosures in tracked docs:"
rg -n "Co-authored-by|co-authored|your email|console user|login.*email|email.*published|RedHatInsights/quickstarts|quickstarts repository" -S . --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' | sed -n '1,220p'Repository: RedHatInsights/learning-resources
Length of output: 12687
Confirm public co-author email disclosure is approved.
The Create PR flow publishes identity.email (with identity.first_name/identity.last_name) as a Co-authored-by trailer, but the PR button and tooltips do not say that the user identity is attached, and the user cannot opt out. If the legal/privacy review does not approve publishing console user email in public commit metadata, omit the email or use a no-reply address.
🤖 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 `@src/components/creator/useCreatePR.ts` around lines 41 - 48, Update the
co-author trailer logic in the Create PR flow around useCreatePR to avoid
publishing identity.email unless public disclosure is explicitly approved.
Prefer the existing approved no-reply identity or omit the trailer; if email
disclosure remains supported, add a clear user-facing disclosure and opt-out
through the PR button or related tooltips.
| const result = await createQuickstartPR(files, { | ||
| branchName: `qs-${prefix}-${quickstartName}-${timestamp}`, | ||
| commitMessage, | ||
| prTitle: `feat(quickstarts): ${prefix} ${quickstartName}`, | ||
| prBody: `${ | ||
| isUpdate ? 'Updating' : 'Adding new' | ||
| } quickstart via the Quickstarts Creator tool.\n\nDirectory: docs/quickstarts/${quickstartName}/`, | ||
| isUpdate, | ||
| directoryName: quickstartName, | ||
| ...(isUpdate | ||
| ? { existingPath: `docs/quickstarts/${quickstartName}/` } | ||
| : {}), | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Sanitize quickstartName before it is used in the branch name and path.
branchName and existingPath interpolate quickstartName directly. The name derives from the generated YAML filename, which derives from the display name slug in src/Creator.tsx (lines 186-189). That slug lowercases the text and replaces whitespace, but it keeps characters that Git refnames reject, for example ~, ^, :, ?, *, [, \, and ... A display name that contains those characters produces an invalid ref and the request fails at the server with an opaque error.
Restrict the value to a safe character set before building the branch name and path.
🛠️ Proposed fix
try {
const timestamp = Date.now();
const prefix = isUpdate ? 'update' : 'create';
+ const safeName = quickstartName
+ .toLowerCase()
+ .replace(/[^a-z0-9._-]/g, '-')
+ .replace(/-{2,}/g, '-')
+ .replace(/^[-.]+|[-.]+$/g, '');
+ if (!safeName) {
+ setPrError('Quickstart name is not valid for a branch name.');
+ setPrLoading(false);
+ return;
+ }Then replace quickstartName with safeName in branchName, directoryName, and existingPath.
🤖 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 `@src/components/creator/useCreatePR.ts` around lines 49 - 61, Sanitize
quickstartName before constructing the createQuickstartPR options, assigning the
restricted value to safeName. Use safeName for branchName, directoryName, and
the conditional existingPath while preserving the existing naming and update
behavior.
|
/retest |
| @@ -9,34 +9,54 @@ import React, { | |||
| import { | |||
There was a problem hiding this comment.
can you add a confirmation step when creating a pr? probably a popup modal that confirms and waits for the pr created
…de review updates
2 participants
Description
RHCLOUD-48689
RHCLOUD-48694
Adds git-service integration to the quickstarts creator, enabling users to submit new quickstarts as pull requests directly from the UI and browse/load existing quickstarts from the GitHub repo. All changes are gated behind the
platform.learning-resources.quickstarts.git-serviceUnleash flag.Changes:
useCreatePRhookSourceSelector.tsx)createQuickstartPRAPI utility (src/utils/createQuickstartPR.ts) for submitting PRs to the backend git-serviceCreatorYAMLView,createQuickstartPR, and updatedCreatorWizardtestsScreenshots
Before:
After:
Anything reviewers should know?
The Unleash flag
platform.learning-resources.quickstarts.git-serviceis currently disabled in all environments. No user-facing changes until the flag is enabled.Checklist
AI disclosure
Assisted by: Claude Code