feat(feedback): add feedback modal#698
feat(feedback): add feedback modal#698abhinandan13jan wants to merge 4 commits intokonflux-ci:mainfrom
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Modal Component & Styling src/components/FeedbackSection/FeedbackModal.tsx, src/components/FeedbackSection/FeedbackModal.scss |
Main feedback modal implementing multi-step workflow with Formik state management, lazy-loaded sections via React.Suspense, and section-specific validation. Modal layout includes a content panel with ErrorBoundary and side image. Footer renders dynamic actions based on current section (Preview on GitHub, Submit feedback, Back, Cancel). |
Form Sections src/components/FeedbackSection/components/BeginningSection.tsx, src/components/FeedbackSection/components/BugRFESection.tsx, src/components/FeedbackSection/components/FeedbackSection.tsx |
Three distinct form sections: BeginningSection for navigation, BugRFESection for conditional bug/feature inputs with optional system info checkbox, and FeedbackSection for user rating and feedback description. |
Validation & Constants src/components/FeedbackSection/FeedbackValidationSchemas.ts, src/components/FeedbackSection/consts.ts |
Yup validation schemas for bug (title/description), feature (title/description), and feedback (scale/description) sections. Constants file exports baseURL for GitHub issues and FeedbackSections enum. |
Utilities src/components/FeedbackSection/feedback-utils.ts |
URL generation helpers getBugURL and getFeatureURL that construct GitHub issue URLs with encoded parameters. getBugURL conditionally appends system information (user-agent, environment, image-proxy) when requested. |
Integration & Tests src/components/Header/HelpDropdown.tsx, src/components/Header/__tests__/HelpDropdown.spec.tsx |
Integrates feedback modal into Help dropdown menu with new "Share feedback" item and associated test coverage. |
Component Tests src/components/FeedbackSection/__tests__/BeginningSection.spec.tsx, src/components/FeedbackSection/__tests__/BugRFESection.spec.tsx, src/components/FeedbackSection/__tests__/FeedbackModal.spec.tsx |
Unit tests validating component rendering, section-specific text content, and modal initialization with mocked Konflux hooks. |
Sequence Diagram
sequenceDiagram
participant User
participant HelpDropdown
participant FeedbackModal
participant FormSections as Form Sections
participant GitHub
User->>HelpDropdown: Click "Share feedback"
HelpDropdown->>FeedbackModal: createFeedbackModal()
FeedbackModal->>FeedbackModal: Initialize (BeginningSection)
FeedbackModal->>User: Render modal with 3 options
User->>FormSections: Select "Report a bug"
FormSections->>FeedbackModal: setCurrentSection(BugSection)
FeedbackModal->>FeedbackModal: Validate & render BugRFESection
FeedbackModal->>User: Display bug form with optional system info
User->>FormSections: Fill form and click "Preview on GitHub"
FormSections->>GitHub: getBugURL(bugData) opens GitHub URL
FeedbackModal->>FeedbackModal: Close modal
alt User selects Feedback
User->>FormSections: Select "Share feedback"
FormSections->>FeedbackModal: setCurrentSection(FeedbackSection)
FeedbackModal->>User: Display rating and feedback form
User->>FormSections: Submit feedback
FormSections->>FeedbackModal: Trigger final submission
FeedbackModal->>FeedbackModal: Close modal
end
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
- feat(FeatureFlag): enhance feature flags implementation #370: Adds test utilities and global Jest mocks for useKonfluxPublicInfo hook, referenced in FeedbackModal test mocking strategy.
Suggested labels
ok-to-test
Suggested reviewers
- Katka92
- janaki29
- rrosatti
🚥 Pre-merge checks | ✅ 3 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Merge Conflict Detection | ❌ Merge conflicts detected (40 files): ⚔️ e2e-tests/tests/basic-happy-path.spec.ts (content)⚔️ src/components/EnterpriseContract/SecurityEnterpriseContractTab.tsx (content)⚔️ src/components/EnterpriseContract/__tests__/SecurityEnterpriseContractTab.spec.tsx (content)⚔️ src/components/EnterpriseContract/__tests__/useEnterpriseContractResultFromLogs.spec.tsx (content)⚔️ src/components/EnterpriseContract/useEnterpriseContractResultFromLogs.tsx (content)⚔️ src/components/EnterpriseContract/utils.tsx (content)⚔️ src/components/Header/HelpDropdown.tsx (content)⚔️ src/components/Header/__tests__/HelpDropdown.spec.tsx (content)⚔️ src/components/PipelineRun/PipelineRunDetailsView/PipelineRunDetailsView.tsx (content)⚔️ src/components/PipelineRun/PipelineRunDetailsView/__tests__/PipelineRunDetailsView.spec.tsx (content)⚔️ src/components/PipelineRun/PipelineRunDetailsView/tabs/PipelineRunSecurityEnterpriseContractTab.tsx (content)⚔️ src/components/ReleaseService/ReleasePlan/TriggerRelease/SnapshotDropdown.tsx (content)⚔️ src/components/ReleaseService/ReleasePlan/TriggerRelease/TriggerReleaseFormPage.tsx (content)⚔️ src/components/ReleaseService/ReleasePlan/TriggerRelease/__tests__/SnapshotDropdown.spec.tsx (content)⚔️ src/components/SnapshotDetails/SnapshotDetailsView.tsx (content)⚔️ src/components/SnapshotDetails/__tests__/SnapshotDetailsView.spec.tsx (content)⚔️ src/components/Snapshots/SnapshotsListView/SnapshotsList.tsx (content)⚔️ src/components/Snapshots/SnapshotsListView/SnapshotsListRow.tsx (content)⚔️ src/components/Snapshots/SnapshotsListView/SnapshotsListView.tsx (content)⚔️ src/components/Snapshots/SnapshotsListView/SnapshotsTab.tsx (content)⚔️ src/components/Snapshots/SnapshotsListView/__tests__/SnapshotsListView.spec.tsx (content)⚔️ src/components/Snapshots/SnapshotsListView/snapshot-actions.ts (content)⚔️ src/components/Snapshots/SnapshotsListView/types.ts (content)⚔️ src/components/TaskRunDetailsView/TaskRunDetailsView.tsx (content)⚔️ src/components/TaskRunDetailsView/__tests__/TaskRunDetailsView.spec.tsx (content)⚔️ src/components/TaskRunDetailsView/tabs/TaskRunSecurityEnterpriseContractTab.tsx (content)⚔️ src/components/TaskRunDetailsView/tabs/__tests__/TaskRunSecurityEnterpriseContractTab.spec.tsx (content)⚔️ src/consts/documentation.ts (content)⚔️ src/hooks/__tests__/useK8sAndKarchResources.spec.ts (content)⚔️ src/hooks/__tests__/useTaskRunsV2.spec.ts (content)⚔️ src/hooks/useK8sAndKarchResources.ts (content)⚔️ src/hooks/useSnapshots.ts (content)⚔️ src/hooks/useTaskRunsV2.ts (content)⚔️ src/shared/components/empty-state/FilteredEmptyState.tsx (content)⚔️ src/shared/hooks/__tests__/useTriggerReleaseAction.spec.ts (content)⚔️ src/shared/hooks/useTriggerReleaseAction.ts (content)⚔️ src/utils/__tests__/enterprise-contract-utils.spec.tsx (content)⚔️ src/utils/__tests__/pipeline-utils.spec.ts (content)⚔️ src/utils/enterprise-contract-utils.tsx (content)⚔️ src/utils/pipeline-utils.ts (content)These conflicts must be resolved before merging into main. |
Resolve conflicts locally and push changes to this branch. |
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Title check | ✅ Passed | The pull request title 'feat(feedback): add feedback modal' clearly and concisely describes the main change: adding a new feedback modal feature to the application. |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
⚔️ Resolve merge conflicts (beta)
- Auto-commit resolved conflicts to branch
add-feedback-modal - Post resolved changes as copyable diffs in a comment
Tip
Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In `@src/components/FeedbackSection/components/BeginningSection.tsx`:
- Line 83: In BeginningSection (component BeginningSection or the JSX text node
containing the sentence), fix the typo and grammar by replacing the string
"Share yout ideas for new feature to make Konflux even better for all users."
with "Share your ideas for new features to make Konflux even better for all
users." so it uses "your" and plural "features".
- Around line 32-48: Replace the clickable-only Card usage so keyboard users can
activate the cards: for each Card that currently uses onClick (the one that
calls setCurrentSection(FeedbackSections.FeedbackSection) and the other two
similar cards), refactor to use PatternFly's CardHeader with selectableActions
providing an onClickAction that calls the same setCurrentSection handler and add
an accessible label (aria-label or aria-labelledby) to the Card/CardHeader;
alternatively, if you choose not to use selectableActions, add an onKeyDown
handler that listens for Enter/Space and invokes setCurrentSection alongside
keeping onClick, and ensure isSelectable/isClickable and tabindex are set so the
cards receive focus.
In `@src/components/FeedbackSection/components/BugRFESection.tsx`:
- Around line 52-58: The label text in BugRFESection.tsx is hardcoded to "bug"
even when currentSection indicates a feature request; update the label
generation in the BugRFESection component to use currentSection (or a derived
readable string) so it renders "Please provide detailed description of the bug"
for bug sections and "Please provide detailed description of the feature" (or
similar) for feature sections—locate the JSX where label is set (the label prop
containing the Tooltip and OutlinedQuestionCircleIcon) and replace the static
word "bug" with a conditional or lookup based on currentSection (e.g., map
currentSection to "bug" | "feature") so the text reflects the active section.
In `@src/components/FeedbackSection/components/FeedbackSection.tsx`:
- Around line 22-60: The RadioGroupField in FeedbackSection uses the wrong form
key; change the name prop on RadioGroupField from "feedback.feedbackScale" to
"feedback.scale" so it matches the Yup/validation schema
(FeedbackSectionValidationSchema) and the expected values shape consumed
downstream; update any tests or consumers that reference feedbackScale to use
feedback.scale if present.
- Around line 27-33: Fix the user-facing typos in the FeedbackSection component:
change "haapy" to "happy" in the heading around the HelpPopover usage (in
FeedbackSection) and change "sattisfied" to "satisfied" inside the Text
component using TextContent / TextVariants.p; update the literal strings only so
the displayed text reads "How happy are you with recent experience using
Konflux" and "Please rate using the following scale, 5 - very satisfied to 1 -
very dissatisfied."
- Around line 64-79: The two InputField components (the one with
name="feedback.description" and the one with name="feedback.email") share the
same data-test value ("bug-description"); change them to unique, descriptive
test attributes (e.g., data-test="feedback-description" for the description
InputField and data-test="feedback-email" for the email InputField) so tests can
target each field unambiguously; update only the data-test props on the
InputField elements to these new values.
In `@src/components/FeedbackSection/consts.ts`:
- Around line 1-2: Replace the hardcoded baseURL constant that points to a
personal fork with a fail-fast placeholder or config-driven value: stop shipping
export const baseURL =
'https://github.com/abhinandan13jan/konflux-ui-test/issues/new?' and instead
read from a required environment/build-time variable (e.g.,
process.env.FEEDBACK_BASE_URL or a build constant) and throw or assert at app
startup if it's unset or still the placeholder; update the symbol baseURL in
src/components/FeedbackSection/consts.ts and ensure callers of baseURL handle
the absence accordingly so a wrong repo URL cannot be shipped.
In `@src/components/FeedbackSection/feedback-utils.ts`:
- Around line 22-31: The URL builder can end up encoding the literal "undefined"
when konfluxInfo fields are missing because encodeURIComponent is called on
undefined; update the logic around additionalInfoURL in feedback-utils.ts to use
safe fallbacks or conditional inclusion: for example, compute safe values (e.g.,
const userAgent = bug.getAdditionalInfo ? info['user-agent'] ?? '' : '') and
only append params when those safe values are non-empty, or use nullish
coalescing (?? '') before calling encodeURIComponent for info.environment and
info['image-proxy']; update the construction of additionalInfoURL and the final
url so encodeURIComponent never receives undefined and parameters are omitted
when empty.
- Around line 15-16: Change the GetFeatureURL type alias to accept a FeatureInfo
rather than a BugInfo: update the declaration of GetFeatureURL from "(bug:
BugInfo) => string" to "(feature: FeatureInfo) => string" so it matches the
implementation that uses the parameter named "feature" and the correct type;
ensure FeatureInfo is imported/available in the file and update any references
that relied on the old parameter name if necessary.
In `@src/components/FeedbackSection/FeedbackModal.scss`:
- Line 8: The SCSS has an invalid quoted border declaration in
FeedbackModal.scss ("border-right: '1px solid red'") which should be removed or
replaced with a proper unquoted border property; locate the offending rule in
FeedbackModal.scss and either delete that line or change it to a valid
declaration using an unquoted width/style/color and a PatternFly design token
for the color (for example use the PatternFly danger/neutral token via
var(...)). Ensure the final property reads as a normal CSS border-right (no
quotes) and references the PatternFly token instead of a hard-coded "red".
In `@src/components/FeedbackSection/FeedbackModal.tsx`:
- Around line 67-92: The Formik instance in FeedbackModal does not reinitialize
when currentSection changes (so initialValues for bug/feature are not applied);
update the Formik usage to force a remount or reinitialize on section change by
adding either enableReinitialize={true} to the <Formik> props or adding
key={currentSection} to the <Formik> element so changing FeedbackSections
(currentSection/FeedbackSections) resets form state and prevents handleSubmit
from receiving undefined shapes.
- Around line 46-63: The handleSubmit function uses values.bug and
values.feature without guarding for undefined; update handleSubmit to
defensively check that the extracted bug/feature exists before calling
getBugURL/getFeatureURL and window.open (e.g., if currentSection ===
FeedbackSections.BugSection then if (!values?.bug) { onClose(null, {
submitClicked: true }); return; } else const url = getBugURL(values.bug,
konfluxInfo) ...), and do the analogous guard for
FeedbackSections.FeatureSection; keep the existing onClose(null, {
submitClicked: true }) behavior when the field is missing or after successful
submission and avoid calling getBugURL/getFeatureURL with undefined.
- Around line 39-44: Add a new feature flag entry named feedback-modal in
src/feature-flags/flags.ts with defaultEnabled: false and status: 'wip', then
gate the FeedbackModal and its UI trigger by the feature flag: update the
HelpDropdown menu item that renders the "Share feedback" entry to only render
when the flag is on (use IfFeature or useIsOnFeatureFlag) and wrap or
conditionally render the FeedbackModal component itself (the FeedbackModal React
component and its caller where onClose is passed) behind the same flag so both
the modal and its trigger are disabled by default.
In `@src/components/Header/HelpDropdown.tsx`:
- Around line 67-73: The DropdownItem for "Share feedback" wrongly reuses
key="about", causing duplicate React keys; update that DropdownItem (component
DropdownItem with onClick=handleFeedbackClick and
data-test="help-dropdown-feedback") to use a unique key (e.g., "feedback" or
"help-feedback") so it no longer collides with the "About Konflux" DropdownItem
that currently uses key="about".
🧹 Nitpick comments (6)
src/components/FeedbackSection/FeedbackValidationSchemas.ts (1)
23-31: Consider adding email validation to the feedback schema.The
FeedbackSectionform includes an optional email field (feedback.email), but this schema doesn't validate its format. If an email is provided, it should at minimum be validated as a proper email address.Proposed addition
export const FeedbackSectionValidationSchema = yup.object({ feedback: yup.object({ scale: yup.number().required('Required'), description: yup .string() .required('Required') .max(2000, 'Should be less than 2000 characters.'), + email: yup.string().email('Must be a valid email address'), }), });src/components/FeedbackSection/FeedbackModal.tsx (1)
18-18: Use absolute import for the asset.Line 18 uses a relative path
../../assets/overview/overview-banner.svg. Per coding guidelines, prefer the absolute~/assets/overview/overview-banner.svgalias. As per coding guidelines: "Use absolute imports with configured path aliases."Proposed fix
-import OverviewBannerSvg from '../../assets/overview/overview-banner.svg'; +import OverviewBannerSvg from '~/assets/overview/overview-banner.svg';src/components/FeedbackSection/FeedbackModal.scss (2)
2-2: Hardcodedborder-radius: 5px— prefer a PatternFly design token.Both Line 2 and Line 18 use a hardcoded
5px. Consider using a PatternFly border-radius token (e.g.,var(--pf-v5-global--BorderRadius--sm)) for consistency with the design system.Based on learnings: "Use PatternFly design tokens (var(--pf-global--spacer--md), var(--pf-global--primary-color--100), etc.) instead of hardcoded values in SCSS."
Also applies to: 18-18
12-12: Hardcodedmin-height: 500px.A magic number for the minimum height. Consider extracting this to a CSS custom property or using a PatternFly token if an appropriate one exists, to improve maintainability.
src/components/Header/HelpDropdown.tsx (1)
67-73: Consider gating the new "Share feedback" item behind a feature flag.The coding guidelines recommend wrapping new features with client-side feature flags during development, especially given the PR notes remaining work items (final URLs, SVG updates, Sentry integration). Wrapping this with
IfFeaturewould allow merging safely while those items are completed.As per coding guidelines: "Wrap new features with client-side feature flags during development."
src/components/Header/__tests__/HelpDropdown.spec.tsx (1)
1-1: Consider migrating touserEvent.setup()instead offireEvent.The coding guidelines prefer
userEvent.setup()with async interactions over synchronousfireEvent. This applies to the entire file (including pre-existing tests), so it could be addressed in a follow-up.As per coding guidelines: "Use
userEvent.setup()and async user interactions instead of synchronousfireEventfor testing user actions."
| <Card | ||
| className="feedback-modal__card-container-card" | ||
| onClick={() => setCurrentSection(FeedbackSections.FeedbackSection)} | ||
| > | ||
| <CardTitle> | ||
| <TextContent> | ||
| <Text component={TextVariants.p}>Share feedback</Text> | ||
| </TextContent> | ||
| </CardTitle> | ||
| <CardBody> | ||
| <TextContent> | ||
| <Text component={TextVariants.small}> | ||
| Please share your experience using Konflux to the product team | ||
| </Text> | ||
| </TextContent> | ||
| </CardBody> | ||
| </Card> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n "src/components/FeedbackSection/components/BeginningSection.tsx"Repository: konflux-ci/konflux-ui
Length of output: 3516
🌐 Web query:
PatternFly 5 React Card component isSelectable isClickable keyboard accessibility
💡 Result:
isSelectable: PatternFly adds focus/hover selectable styling and automatically addstabindex="0"so the card can be reached with standard keyboard tabbing. You still need to implement the actual “select on keyboard” behavior (typically handling Enter/Space) unless you use the built-in selectable input/actions patterns. [1]- Clickable/actionable cards (
isClickable): For keyboard accessibility, PatternFly’s recommended approach is to make the action a real button or anchor viaCardHeader’sselectableActions(useonClickActionorto). For clickable-only cards, you must also provide an accessible name for that control viaselectableActionAriaLabel/selectableActionAriaLabelledby. [2][3] - Don’t nest extra interactive controls inside a card that is meant to be selectable-only or clickable-only; use the “actionable and selectable” pattern when you need both. [3]
- Accessible naming: give the card an accessible name with
aria-labeloraria-labelledby(often pointing toCardTitle’sid). Also note PatternFly’s warning thataria-label/aria-labelledbyon a plaindivwrapper isn’t well supported without an appropriate role. [1]
Sources: [1] Card accessibility (React customization) [2] CardHeader selectableActions API (aria-label requirements, onClickAction/to) [3] Card docs (selectable/clickable behavior + guidance about interactive content)
Cards are not keyboard-accessible and require proper event handling or action-based patterns.
These Card components rely solely on onClick. Keyboard-only users cannot activate them. PatternFly's isSelectable and isClickable props add focusable styling and tabindex="0", but do not automatically make onClick handlers respond to keyboard input (Enter/Space).
For keyboard accessibility, you must either:
- Add keyboard event handling (
onKeyDownfor Enter/Space keys) alongside the props, or - Use PatternFly's recommended pattern: wrap with
CardHeaderusingselectableActionswithonClickAction, and add accessible naming viaaria-labeloraria-labelledby
The second approach (CardHeader/selectableActions) is preferred as it handles keyboard semantics automatically. Apply to all three cards (lines 32, 51, 71).
🤖 Prompt for AI Agents
In `@src/components/FeedbackSection/components/BeginningSection.tsx` around lines
32 - 48, Replace the clickable-only Card usage so keyboard users can activate
the cards: for each Card that currently uses onClick (the one that calls
setCurrentSection(FeedbackSections.FeedbackSection) and the other two similar
cards), refactor to use PatternFly's CardHeader with selectableActions providing
an onClickAction that calls the same setCurrentSection handler and add an
accessible label (aria-label or aria-labelledby) to the Card/CardHeader;
alternatively, if you choose not to use selectableActions, add an onKeyDown
handler that listens for Enter/Space and invokes setCurrentSection alongside
keeping onClick, and ensure isSelectable/isClickable and tabindex are set so the
cards receive focus.
| <CardBody> | ||
| <TextContent> | ||
| <Text component={TextVariants.small}> | ||
| Share yout ideas for new feature to make Konflux even better for all users. |
There was a problem hiding this comment.
Typo: "yout" → "your".
Line 83: "Share yout ideas for new feature to make Konflux even better for all users." should be "Share your ideas for new features to make Konflux even better for all users."
Proposed fix
- Share yout ideas for new feature to make Konflux even better for all users.
+ Share your ideas for new features to make Konflux even better for all users.📝 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.
| Share yout ideas for new feature to make Konflux even better for all users. | |
| Share your ideas for new features to make Konflux even better for all users. |
🤖 Prompt for AI Agents
In `@src/components/FeedbackSection/components/BeginningSection.tsx` at line 83,
In BeginningSection (component BeginningSection or the JSX text node containing
the sentence), fix the typo and grammar by replacing the string "Share yout
ideas for new feature to make Konflux even better for all users." with "Share
your ideas for new features to make Konflux even better for all users." so it
uses "your" and plural "features".
| label={ | ||
| <> | ||
| Please provide detailed description of the bug{' '} | ||
| <Tooltip content="more info"> | ||
| <OutlinedQuestionCircleIcon /> | ||
| </Tooltip> | ||
| </> |
There was a problem hiding this comment.
Description label is hardcoded to "bug" even in the Feature section.
Line 54 reads "Please provide detailed description of the bug" regardless of currentSection. When the user is requesting a feature, this label is wrong.
Proposed fix
label={
<>
- Please provide detailed description of the bug{' '}
+ {currentSection === FeedbackSections.BugSection
+ ? 'Please provide detailed description of the bug'
+ : 'Please provide detailed description of the feature'}{' '}
<Tooltip content="more info">
<OutlinedQuestionCircleIcon />
</Tooltip>
</>
}📝 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.
| label={ | |
| <> | |
| Please provide detailed description of the bug{' '} | |
| <Tooltip content="more info"> | |
| <OutlinedQuestionCircleIcon /> | |
| </Tooltip> | |
| </> | |
| label={ | |
| <> | |
| {currentSection === FeedbackSections.BugSection | |
| ? 'Please provide detailed description of the bug' | |
| : 'Please provide detailed description of the feature'}{' '} | |
| <Tooltip content="more info"> | |
| <OutlinedQuestionCircleIcon /> | |
| </Tooltip> | |
| </> | |
| } |
🤖 Prompt for AI Agents
In `@src/components/FeedbackSection/components/BugRFESection.tsx` around lines 52
- 58, The label text in BugRFESection.tsx is hardcoded to "bug" even when
currentSection indicates a feature request; update the label generation in the
BugRFESection component to use currentSection (or a derived readable string) so
it renders "Please provide detailed description of the bug" for bug sections and
"Please provide detailed description of the feature" (or similar) for feature
sections—locate the JSX where label is set (the label prop containing the
Tooltip and OutlinedQuestionCircleIcon) and replace the static word "bug" with a
conditional or lookup based on currentSection (e.g., map currentSection to "bug"
| "feature") so the text reflects the active section.
| <RadioGroupField | ||
| name="feedback.feedbackScale" | ||
| label={ | ||
| <> | ||
| <b> | ||
| How haapy are you with recent experience using Konflux{' '} | ||
| <HelpPopover headerContent="More info" bodyContent="more info" /> | ||
| </b> | ||
| <TextContent> | ||
| <Text component={TextVariants.p}> | ||
| Please rate using the following scale, 5 - very sattisfied to 1 - very | ||
| dissatisfied. | ||
| </Text> | ||
| </TextContent> | ||
| </> | ||
| } | ||
| options={[ | ||
| { | ||
| value: 5, | ||
| label: '5', | ||
| }, | ||
| { | ||
| value: 4, | ||
| label: '4', | ||
| }, | ||
| { | ||
| value: 3, | ||
| label: '3', | ||
| }, | ||
| { | ||
| value: 2, | ||
| label: '2', | ||
| }, | ||
| { | ||
| value: 1, | ||
| label: '1', | ||
| }, | ||
| ]} | ||
| /> |
There was a problem hiding this comment.
Critical: Form field name feedback.feedbackScale doesn't match validation schema feedback.scale.
The RadioGroupField uses name="feedback.feedbackScale" (line 23), but FeedbackSectionValidationSchema validates feedback.scale. Yup will never see the radio value under the expected key, so:
- The
requiredvalidation onscalewill always fail (or be bypassed depending on Formik's config), and - The submitted
values.feedbackobject will havefeedbackScaleinstead ofscale, which downstream consumers won't expect.
Rename the field to feedback.scale to align with the schema.
Proposed fix
<RadioGroupField
- name="feedback.feedbackScale"
+ name="feedback.scale"📝 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.
| <RadioGroupField | |
| name="feedback.feedbackScale" | |
| label={ | |
| <> | |
| <b> | |
| How haapy are you with recent experience using Konflux{' '} | |
| <HelpPopover headerContent="More info" bodyContent="more info" /> | |
| </b> | |
| <TextContent> | |
| <Text component={TextVariants.p}> | |
| Please rate using the following scale, 5 - very sattisfied to 1 - very | |
| dissatisfied. | |
| </Text> | |
| </TextContent> | |
| </> | |
| } | |
| options={[ | |
| { | |
| value: 5, | |
| label: '5', | |
| }, | |
| { | |
| value: 4, | |
| label: '4', | |
| }, | |
| { | |
| value: 3, | |
| label: '3', | |
| }, | |
| { | |
| value: 2, | |
| label: '2', | |
| }, | |
| { | |
| value: 1, | |
| label: '1', | |
| }, | |
| ]} | |
| /> | |
| <RadioGroupField | |
| name="feedback.scale" | |
| label={ | |
| <> | |
| <b> | |
| How haapy are you with recent experience using Konflux{' '} | |
| <HelpPopover headerContent="More info" bodyContent="more info" /> | |
| </b> | |
| <TextContent> | |
| <Text component={TextVariants.p}> | |
| Please rate using the following scale, 5 - very sattisfied to 1 - very | |
| dissatisfied. | |
| </Text> | |
| </TextContent> | |
| </> | |
| } | |
| options={[ | |
| { | |
| value: 5, | |
| label: '5', | |
| }, | |
| { | |
| value: 4, | |
| label: '4', | |
| }, | |
| { | |
| value: 3, | |
| label: '3', | |
| }, | |
| { | |
| value: 2, | |
| label: '2', | |
| }, | |
| { | |
| value: 1, | |
| label: '1', | |
| }, | |
| ]} | |
| /> |
🤖 Prompt for AI Agents
In `@src/components/FeedbackSection/components/FeedbackSection.tsx` around lines
22 - 60, The RadioGroupField in FeedbackSection uses the wrong form key; change
the name prop on RadioGroupField from "feedback.feedbackScale" to
"feedback.scale" so it matches the Yup/validation schema
(FeedbackSectionValidationSchema) and the expected values shape consumed
downstream; update any tests or consumers that reference feedbackScale to use
feedback.scale if present.
| How haapy are you with recent experience using Konflux{' '} | ||
| <HelpPopover headerContent="More info" bodyContent="more info" /> | ||
| </b> | ||
| <TextContent> | ||
| <Text component={TextVariants.p}> | ||
| Please rate using the following scale, 5 - very sattisfied to 1 - very | ||
| dissatisfied. |
There was a problem hiding this comment.
Typos in user-facing text: "haapy" and "sattisfied".
Line 27: "haapy" → "happy". Line 32: "sattisfied" → "satisfied".
Proposed fix
- How haapy are you with recent experience using Konflux{' '}
+ How happy are you with your recent experience using Konflux?{' '}- Please rate using the following scale, 5 - very sattisfied to 1 - very
+ Please rate using the following scale, 5 - very satisfied to 1 - very📝 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.
| How haapy are you with recent experience using Konflux{' '} | |
| <HelpPopover headerContent="More info" bodyContent="more info" /> | |
| </b> | |
| <TextContent> | |
| <Text component={TextVariants.p}> | |
| Please rate using the following scale, 5 - very sattisfied to 1 - very | |
| dissatisfied. | |
| How happy are you with your recent experience using Konflux?{' '} | |
| <HelpPopover headerContent="More info" bodyContent="more info" /> | |
| </b> | |
| <TextContent> | |
| <Text component={TextVariants.p}> | |
| Please rate using the following scale, 5 - very satisfied to 1 - very | |
| dissatisfied. |
🤖 Prompt for AI Agents
In `@src/components/FeedbackSection/components/FeedbackSection.tsx` around lines
27 - 33, Fix the user-facing typos in the FeedbackSection component: change
"haapy" to "happy" in the heading around the HelpPopover usage (in
FeedbackSection) and change "sattisfied" to "satisfied" inside the Text
component using TextContent / TextVariants.p; update the literal strings only so
the displayed text reads "How happy are you with recent experience using
Konflux" and "Please rate using the following scale, 5 - very satisfied to 1 -
very dissatisfied."
| margin: 0; | ||
| } | ||
| &__content { | ||
| border-right: '1px solid red'; |
There was a problem hiding this comment.
Bug: Quoted CSS value renders as a string literal, not a border.
border-right: '1px solid red' — the single quotes make this a CSS string value, so the browser will ignore it entirely. Additionally, 1px solid red looks like debug styling that shouldn't ship.
Remove this line or replace it with a valid, unquoted declaration using a PatternFly design token for the color.
Proposed fix
&__content {
- border-right: '1px solid red';
height: 100%;🤖 Prompt for AI Agents
In `@src/components/FeedbackSection/FeedbackModal.scss` at line 8, The SCSS has an
invalid quoted border declaration in FeedbackModal.scss ("border-right: '1px
solid red'") which should be removed or replaced with a proper unquoted border
property; locate the offending rule in FeedbackModal.scss and either delete that
line or change it to a valid declaration using an unquoted width/style/color and
a PatternFly design token for the color (for example use the PatternFly
danger/neutral token via var(...)). Ensure the final property reads as a normal
CSS border-right (no quotes) and references the PatternFly token instead of a
hard-coded "red".
| const FeedbackModal: React.FC<React.PropsWithChildren<ComponentProps>> = ({ onClose }) => { | ||
| const [currentSection, setCurrentSection] = React.useState<FeedbackSections>( | ||
| FeedbackSections.BeginningSection, | ||
| ); | ||
|
|
||
| const [konfluxInfo] = useKonfluxPublicInfo(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for feedback-related flags in flags.ts
rg -n 'feedback' --type=ts -i src/feature-flags/flags.tsRepository: konflux-ci/konflux-ui
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check the current content of flags.ts to see defined flags
head -100 src/feature-flags/flags.tsRepository: konflux-ci/konflux-ui
Length of output: 3114
🏁 Script executed:
#!/bin/bash
# Search for IfFeature or useIsOnFeatureFlag usage in FeedbackModal.tsx
rg -n 'IfFeature|useIsOnFeatureFlag' src/components/FeedbackSection/FeedbackModal.tsxRepository: konflux-ci/konflux-ui
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check HelpDropdown.tsx for FeedbackModal usage and any feature flag wrapping
rg -n 'FeedbackModal|Feedback' src/components/HelpDropdown/ -A 3Repository: konflux-ci/konflux-ui
Length of output: 133
🏁 Script executed:
#!/bin/bash
# Get the full FeedbackModal.tsx file to check for flag implementation
wc -l src/components/FeedbackSection/FeedbackModal.tsxRepository: konflux-ci/konflux-ui
Length of output: 117
🏁 Script executed:
#!/bin/bash
# Get full flags.ts to see all defined flags
cat src/feature-flags/flags.ts | tail -50Repository: konflux-ci/konflux-ui
Length of output: 1625
🏁 Script executed:
#!/bin/bash
# Read the FeedbackModal.tsx file to understand its content
cat -n src/components/FeedbackSection/FeedbackModal.tsxRepository: konflux-ci/konflux-ui
Length of output: 8283
🏁 Script executed:
#!/bin/bash
# Search for where FeedbackModal is imported or used
rg -n 'FeedbackModal' --type=tsx --type=tsRepository: konflux-ci/konflux-ui
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Search for FeedbackModal imports and usage
rg -n 'FeedbackModal' --type=tsRepository: konflux-ci/konflux-ui
Length of output: 808
🏁 Script executed:
#!/bin/bash
# Also search in tsx files using different approach
fd -e tsx -e ts -x rg -l 'FeedbackModal' {}Repository: konflux-ci/konflux-ui
Length of output: 212
🏁 Script executed:
#!/bin/bash
# Search for where createFeedbackModal is used
rg -n 'createFeedbackModal'Repository: konflux-ci/konflux-ui
Length of output: 383
🏁 Script executed:
#!/bin/bash
# Check HelpDropdown.tsx to see the full context
cat -n src/components/Header/HelpDropdown.tsxRepository: konflux-ci/konflux-ui
Length of output: 3068
Wrap the FeedbackModal feature and its trigger behind a feature flag.
Per project conventions, new features must be gated using the IfFeature component or useIsOnFeatureFlag hook. Add a new flag to src/feature-flags/flags.ts (e.g., feedback-modal with defaultEnabled: false and status: 'wip'), then wrap both the FeedbackModal component and the "Share feedback" menu item in HelpDropdown.
🤖 Prompt for AI Agents
In `@src/components/FeedbackSection/FeedbackModal.tsx` around lines 39 - 44, Add a
new feature flag entry named feedback-modal in src/feature-flags/flags.ts with
defaultEnabled: false and status: 'wip', then gate the FeedbackModal and its UI
trigger by the feature flag: update the HelpDropdown menu item that renders the
"Share feedback" entry to only render when the flag is on (use IfFeature or
useIsOnFeatureFlag) and wrap or conditionally render the FeedbackModal component
itself (the FeedbackModal React component and its caller where onClose is
passed) behind the same flag so both the modal and its trigger are disabled by
default.
| const handleSubmit = (values: { bug?: BugInfo; feature?: FeatureInfo }) => { | ||
| if (currentSection === FeedbackSections.BugSection) { | ||
| const { bug } = values; | ||
| const url = getBugURL(bug, konfluxInfo); | ||
| window.open(url, '_blank'); | ||
| onClose(null, { submitClicked: true }); | ||
| } | ||
|
|
||
| if (currentSection === FeedbackSections.FeatureSection) { | ||
| const { feature } = values; | ||
| const url = getFeatureURL(feature); | ||
| window.open(url, '_blank'); | ||
| onClose(null, { submitClicked: true }); | ||
| } | ||
| if (currentSection === FeedbackSections.FeedbackSection) { | ||
| // Add Sentry/Segment connection here | ||
| onClose(null, { submitClicked: true }); | ||
| } |
There was a problem hiding this comment.
handleSubmit destructures potentially undefined values without guards.
values.bug (line 48) and values.feature (line 55) could be undefined if Formik wasn't properly reinitialized (see prior comment). Even after fixing reinitialization, a defensive check is good practice since handleSubmit's parameter type allows all fields to be optional.
Proposed defensive check
const handleSubmit = (values: { bug?: BugInfo; feature?: FeatureInfo }) => {
if (currentSection === FeedbackSections.BugSection) {
const { bug } = values;
+ if (!bug) return;
const url = getBugURL(bug, konfluxInfo);
window.open(url, '_blank');
onClose(null, { submitClicked: true });
}
if (currentSection === FeedbackSections.FeatureSection) {
const { feature } = values;
+ if (!feature) return;
const url = getFeatureURL(feature);📝 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 handleSubmit = (values: { bug?: BugInfo; feature?: FeatureInfo }) => { | |
| if (currentSection === FeedbackSections.BugSection) { | |
| const { bug } = values; | |
| const url = getBugURL(bug, konfluxInfo); | |
| window.open(url, '_blank'); | |
| onClose(null, { submitClicked: true }); | |
| } | |
| if (currentSection === FeedbackSections.FeatureSection) { | |
| const { feature } = values; | |
| const url = getFeatureURL(feature); | |
| window.open(url, '_blank'); | |
| onClose(null, { submitClicked: true }); | |
| } | |
| if (currentSection === FeedbackSections.FeedbackSection) { | |
| // Add Sentry/Segment connection here | |
| onClose(null, { submitClicked: true }); | |
| } | |
| const handleSubmit = (values: { bug?: BugInfo; feature?: FeatureInfo }) => { | |
| if (currentSection === FeedbackSections.BugSection) { | |
| const { bug } = values; | |
| if (!bug) return; | |
| const url = getBugURL(bug, konfluxInfo); | |
| window.open(url, '_blank'); | |
| onClose(null, { submitClicked: true }); | |
| } | |
| if (currentSection === FeedbackSections.FeatureSection) { | |
| const { feature } = values; | |
| if (!feature) return; | |
| const url = getFeatureURL(feature); | |
| window.open(url, '_blank'); | |
| onClose(null, { submitClicked: true }); | |
| } | |
| if (currentSection === FeedbackSections.FeedbackSection) { | |
| // Add Sentry/Segment connection here | |
| onClose(null, { submitClicked: true }); | |
| } |
🤖 Prompt for AI Agents
In `@src/components/FeedbackSection/FeedbackModal.tsx` around lines 46 - 63, The
handleSubmit function uses values.bug and values.feature without guarding for
undefined; update handleSubmit to defensively check that the extracted
bug/feature exists before calling getBugURL/getFeatureURL and window.open (e.g.,
if currentSection === FeedbackSections.BugSection then if (!values?.bug) {
onClose(null, { submitClicked: true }); return; } else const url =
getBugURL(values.bug, konfluxInfo) ...), and do the analogous guard for
FeedbackSections.FeatureSection; keep the existing onClose(null, {
submitClicked: true }) behavior when the field is missing or after successful
submission and avoid calling getBugURL/getFeatureURL with undefined.
| <Formik | ||
| initialValues={ | ||
| currentSection === FeedbackSections.BugSection | ||
| ? { | ||
| bug: { title: '', description: '', getAdditionalInfo: false }, | ||
| } | ||
| : currentSection === FeedbackSections.FeatureSection | ||
| ? { | ||
| feature: { title: '', description: '' }, | ||
| } | ||
| : { | ||
| feedback: { | ||
| description: '', | ||
| scale: 5, | ||
| }, | ||
| } | ||
| } | ||
| onSubmit={handleSubmit} | ||
| validationSchema={ | ||
| currentSection === FeedbackSections.BugSection | ||
| ? BugSectionValidationSchema | ||
| : currentSection === FeedbackSections.FeatureSection | ||
| ? FeatureSectionValidationSchema | ||
| : FeedbackSectionValidationSchema | ||
| } | ||
| > |
There was a problem hiding this comment.
Critical: Formik won't reinitialize when switching sections.
currentSection starts as BeginningSection, so initialValues is set to the feedback shape. When the user navigates to Bug or Feature, currentSection changes but Formik does not re-derive initialValues by default. The form fields for bug.* or feature.* will have no initial values, and handleSubmit will destructure undefined.
Fix: Either add enableReinitialize to <Formik>, or add a key={currentSection} to force a full remount when the section changes (which also clears touched/errors — likely the desired behavior here).
Recommended fix — add key to force remount
<Formik
+ key={currentSection}
initialValues={
currentSection === FeedbackSections.BugSection📝 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.
| <Formik | |
| initialValues={ | |
| currentSection === FeedbackSections.BugSection | |
| ? { | |
| bug: { title: '', description: '', getAdditionalInfo: false }, | |
| } | |
| : currentSection === FeedbackSections.FeatureSection | |
| ? { | |
| feature: { title: '', description: '' }, | |
| } | |
| : { | |
| feedback: { | |
| description: '', | |
| scale: 5, | |
| }, | |
| } | |
| } | |
| onSubmit={handleSubmit} | |
| validationSchema={ | |
| currentSection === FeedbackSections.BugSection | |
| ? BugSectionValidationSchema | |
| : currentSection === FeedbackSections.FeatureSection | |
| ? FeatureSectionValidationSchema | |
| : FeedbackSectionValidationSchema | |
| } | |
| > | |
| <Formik | |
| key={currentSection} | |
| initialValues={ | |
| currentSection === FeedbackSections.BugSection | |
| ? { | |
| bug: { title: '', description: '', getAdditionalInfo: false }, | |
| } | |
| : currentSection === FeedbackSections.FeatureSection | |
| ? { | |
| feature: { title: '', description: '' }, | |
| } | |
| : { | |
| feedback: { | |
| description: '', | |
| scale: 5, | |
| }, | |
| } | |
| } | |
| onSubmit={handleSubmit} | |
| validationSchema={ | |
| currentSection === FeedbackSections.BugSection | |
| ? BugSectionValidationSchema | |
| : currentSection === FeedbackSections.FeatureSection | |
| ? FeatureSectionValidationSchema | |
| : FeedbackSectionValidationSchema | |
| } | |
| > |
🤖 Prompt for AI Agents
In `@src/components/FeedbackSection/FeedbackModal.tsx` around lines 67 - 92, The
Formik instance in FeedbackModal does not reinitialize when currentSection
changes (so initialValues for bug/feature are not applied); update the Formik
usage to force a remount or reinitialize on section change by adding either
enableReinitialize={true} to the <Formik> props or adding key={currentSection}
to the <Formik> element so changing FeedbackSections
(currentSection/FeedbackSections) resets form state and prevents handleSubmit
from receiving undefined shapes.
| <DropdownItem | ||
| key="about" | ||
| onClick={handleFeedbackClick} | ||
| data-test="help-dropdown-feedback" | ||
| > | ||
| Share feedback | ||
| </DropdownItem> |
There was a problem hiding this comment.
Bug: Duplicate React key="about" on the feedback DropdownItem.
Line 68 reuses key="about" which is already assigned to the "About Konflux" item on Line 61. Duplicate keys break React's reconciliation and can cause rendering issues (e.g., stale state, incorrect DOM updates).
Proposed fix
<DropdownItem
- key="about"
+ key="feedback"
onClick={handleFeedbackClick}
data-test="help-dropdown-feedback"
>📝 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.
| <DropdownItem | |
| key="about" | |
| onClick={handleFeedbackClick} | |
| data-test="help-dropdown-feedback" | |
| > | |
| Share feedback | |
| </DropdownItem> | |
| <DropdownItem | |
| key="feedback" | |
| onClick={handleFeedbackClick} | |
| data-test="help-dropdown-feedback" | |
| > | |
| Share feedback | |
| </DropdownItem> |
🤖 Prompt for AI Agents
In `@src/components/Header/HelpDropdown.tsx` around lines 67 - 73, The
DropdownItem for "Share feedback" wrongly reuses key="about", causing duplicate
React keys; update that DropdownItem (component DropdownItem with
onClick=handleFeedbackClick and data-test="help-dropdown-feedback") to use a
unique key (e.g., "feedback" or "help-feedback") so it no longer collides with
the "About Konflux" DropdownItem that currently uses key="about".
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (25.27%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #698 +/- ##
==========================================
- Coverage 86.35% 85.19% -1.17%
==========================================
Files 761 770 +9
Lines 57880 58603 +723
Branches 6803 5886 -917
==========================================
- Hits 49985 49925 -60
- Misses 7812 8577 +765
- Partials 83 101 +18
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 220 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/types/component.ts (1)
15-24:⚠️ Potential issue | 🟡 MinorRemove the unused
urlfield or clarify its intended purpose with a strong doc comment.The top-level
url?: stringinComponentSourceis never accessed anywhere in the codebase. All implementations consistently usesource.git.url. Either remove this unused field, or if it's reserved for future use (e.g., for non-git container image sources), document the intent explicitly.package.json (1)
25-58:⚠️ Potential issue | 🟡 MinorAdd
js-yamlas an explicit dependency.The
downloadYamlutility insrc/utils/common-utils.tsximportsdumpfromjs-yaml(line 2), butjs-yamlis not listed in dependencies—only present as a transitive dependency inyarn.lock. This creates fragility if upstream dependencies change their dependency trees. Addjs-yamlexplicitly todependenciesinpackage.json.src/components/ImportForm/ComponentSection/SourceSection.tsx (1)
37-48:⚠️ Potential issue | 🟡 MinorForgejo annotation is unnecessarily cleared before being set.
The guard on line 37 clears
gitProviderAnnotationfor any type that isn't GitHub or GitLab. Since Forgejo is now a recognized provider, it gets cleared on line 38, then immediately re-set on line 47. Update the guard to also exclude Forgejo:Proposed fix
- if (gitType !== GitProvider.GITHUB && gitType !== GitProvider.GITLAB) { + if (gitType !== GitProvider.GITHUB && gitType !== GitProvider.GITLAB && gitType !== GitProvider.FORGEJO) { await setFieldValue('gitProviderAnnotation', ''); }Alternatively, refactor the chain into a switch or mapping for cleaner maintenance as more providers are added.
🤖 Fix all issues with AI agents
In @.github/workflows/periodic-job-local.yaml:
- Around line 77-80: The YARN_CACHE_FOLDER export only affects the current run
shell and won't persist to later steps; update the workflow step that currently
creates /mnt/yarn_cache and sets YARN_CACHE_FOLDER so that, instead of relying
on export, it appends the YARN_CACHE_FOLDER key/value to the $GITHUB_ENV file
(so subsequent steps see it). Locate the lines that create and chown
/mnt/yarn_cache and replace the export YARN_CACHE_FOLDER action with writing the
YARN_CACHE_FOLDER=/mnt/yarn_cache entry into GITHUB_ENV (referencing the
YARN_CACHE_FOLDER variable and $GITHUB_ENV in the change).
In @.github/workflows/pr-check.yaml:
- Around line 135-143: The workflow step sets YARN_CACHE_FOLDER with export
which only affects the current shell; change that to persist the variable across
steps by writing "YARN_CACHE_FOLDER=/mnt/yarn_cache" to GITHUB_ENV (use the
runner provided $GITHUB_ENV) instead of using export; update the block that
currently creates /mnt/yarn_cache and exports YARN_CACHE_FOLDER so it echoes the
variable into GITHUB_ENV (ensure the variable name matches where later steps
expect it, e.g., YARN_CACHE_FOLDER) and keep the mkdir/chown lines for
/mnt/yarn_cache.
In @.github/workflows/pr-report.yaml:
- Around line 19-25: Move the "Enable Corepack 📦" step so it runs after the
"Setup Node.js" step (the one using actions/setup-node@v6 with node-version:
'20'); specifically, reorder the workflow steps so "Setup Node.js" executes
before the "Enable Corepack 📦" step to ensure Corepack shims are created
against the installed Node binary (maintain the node-version: '20' value).
In `@e2e-tests/entrypoint.sh`:
- Around line 8-15: The script currently performs "cd /e2e" and "cd /tmp/e2e"
without checking for failures, so if a cd fails the script continues in the
wrong directory; update the entrypoint to fail-fast by ensuring each cd is
guarded (e.g., after the "cd /e2e" and "cd /tmp/e2e" statements add a check that
exits on failure or enable strict mode with a safe error-handling fallback), and
emit a clear error message before exiting so artifacts are not written to
unexpected locations.
In `@README.md`:
- Line 25: Change the phrase "A step by step series of examples that tell you
how to get a development environment running:" to use the compound adjective
form "step-by-step"; locate the sentence containing "A step by step series of
examples..." in the README and replace it with "A step-by-step series of
examples that tell you how to get a development environment running:" to correct
the grammar.
In
`@src/components/ComponentsPage/ComponentDetails/__tests__/ComponentDetailsView.spec.tsx`:
- Around line 16-18: The jest.mock call currently targets a relative path so the
mock doesn't match the actual import; update the mock declaration that
references useComponent (the jest.mock(...) wrapping useComponent: jest.fn()) to
use the same module alias used by the component (the path your source imports)
so the mock applies correctly to the imported useComponent hook.
- Around line 4-8: Replace deep relative imports in this test with the project's
configured path aliases: import FLAGS from '~/feature-flags/flags',
useIsOnFeatureFlag and useFeatureFlags from '~/feature-flags/hooks',
useComponent from '~/hooks/useComponents', and renderWithQueryClientAndRouter
and mockUseNamespaceHook from '~/unit-test-utils' (or the exact alias used for
unit-test utilities). Update the import statements referencing FLAGS,
useIsOnFeatureFlag, useFeatureFlags, useComponent,
renderWithQueryClientAndRouter, and mockUseNamespaceHook to use those absolute
aliases so the test follows the coding guidelines.
In `@src/components/FeedbackSection/__tests__/BugRFESection.spec.tsx`:
- Around line 1-3: Reorder the import statements so the absolute import comes
before relative imports: move the import of formikRenderer (from
'~/unit-test-utils') above the imports of BugRFESection and FeedbackSections;
ensure the import list reads with the absolute module first, then the relative
module imports (references: formikRenderer, BugRFESection, FeedbackSections).
- Around line 16-24: In the BugRFESection.spec.tsx test for "should show RFEForm
when current Section is FeatureSection", remove the incorrect assertion that
checks for 'Please provide detailed description of the bug' and either delete
that line or replace it with the correct feature text assertion (matching the
component output when currentSection === FeedbackSections.FeatureSection, e.g.,
the feature description string rendered by BugRFESection); target the test block
that renders <BugRFESection currentSection={FeedbackSections.FeatureSection} />
to make the correction.
In `@src/components/FeedbackSection/__tests__/FeedbackModal.spec.tsx`:
- Around line 9-13: Rename the test "should initialize with BeginingSection
Cards" to "should initialize with BeginningSection Cards" and make the
assertions consistent by replacing raw getByText calls with explicit expect
assertions; in the "FeedbackModal" test use render(<FeedbackModal />) as before,
but change screen.getByText('...') to expect(screen.getByText('Share
feedback')).toBeInTheDocument(), expect(screen.getByText('Report a
bug')).toBeInTheDocument(), and expect(screen.getByText('Request a new
feature')).toBeInTheDocument() so all assertions use the explicit
expect(...).toBeInTheDocument() style.
- Line 1: Replace the bare render call with the shared Formik-aware renderer so
FeedbackModal tests run inside a Formik context: import and use formikRenderer
from '~/unit-test-utils' (like the BugRFESection tests do) when rendering
<FeedbackModal /> in FeedbackModal.spec.tsx so components nested in Formik
(FeedbackModal, BugRFESection, BeginingSection, etc.) receive the Formik context
during the test.
In `@src/shared/utils/git-utils.tsx`:
- Around line 39-41: The detectGitType function currently checks for forgejo
with hasDomain(url, 'forgejo.org') which only matches the exact domain; update
detectGitType (and its use of hasDomain) to detect Forgejo subdomains the same
way validation does—either reuse GIT_URL.DOMAIN_REGEX or change the check to
match domains that end with ".forgejo.org" (or a regex like
/(^|\.)forgejo\.org$/) so URLs such as "v14.next.forgejo.org" return
GitProvider.FORGEJO; adjust the condition in detectGitType accordingly (replace
the hasDomain(...) check) to ensure detection aligns with validation and
path-generation logic.
🧹 Nitpick comments (18)
src/components/FeedbackSection/__tests__/FeedbackModal.spec.tsx (1)
8-18: Test coverage is minimal — only happy-path rendering is verified.There are no tests for user interactions (e.g., clicking a card to navigate to the bug/feature/feedback section) or for the cancel button's behavior (does it call an
onClosecallback?). Based on learnings: "Test both success and error scenarios for all components and features."Want me to draft additional interaction tests for card navigation and cancel behavior?
src/components/FeedbackSection/__tests__/BeginningSection.spec.tsx (1)
4-12:setCurrentSectionMockis created but never asserted — add interaction tests.The mock is passed as a prop but no test verifies it gets called when a user clicks a card. This is the primary behavior of
BeginningSection. Also, the test name has the same "BeginingSection" typo.Sketch of an interaction test
+import userEvent from '@testing-library/user-event'; import { render } from '@testing-library/react'; import BeginningSection from '../components/BeginningSection'; const setCurrentSectionMock = jest.fn(); describe('BeginningSection', () => { - it('should initialize with BeginingSection', () => { + it('should render all feedback option cards', () => { const screen = render(<BeginningSection setCurrentSection={setCurrentSectionMock} />); - screen.getByText('Share feedback'); - screen.getByText('Report a bug'); - screen.getByText('Request a new feature'); + expect(screen.getByText('Share feedback')).toBeInTheDocument(); + expect(screen.getByText('Report a bug')).toBeInTheDocument(); + expect(screen.getByText('Request a new feature')).toBeInTheDocument(); }); + + it('should call setCurrentSection when a card is clicked', async () => { + const user = userEvent.setup(); + const { getByText } = render(<BeginningSection setCurrentSection={setCurrentSectionMock} />); + await user.click(getByText('Report a bug')); + expect(setCurrentSectionMock).toHaveBeenCalled(); + }); });src/utils/common-utils.tsx (1)
26-34:downloadYamlimplementation is clean and functional.Two minor observations:
- The
as stringcast ondump(resource)is unnecessary —dumpalready returnsstring.- If
dumpthrows (e.g., on unexpected input), the error propagates unhandled. Consider wrapping in a try/catch if this is called from a UI click handler, to avoid uncaught exceptions.Optional: remove redundant cast and add error guard
export const downloadYaml = <T extends K8sResourceCommon>(resource: T) => { if (!resource) return; - const resourceYaml = dump(resource) as string; - const blob = new Blob([resourceYaml], { - type: 'text/yaml;charset=utf-8', - }); - const filename = `${resource.metadata?.name || 'resource'}.yaml`; - saveAs(blob, filename); + try { + const resourceYaml = dump(resource); + const blob = new Blob([resourceYaml], { + type: 'text/yaml;charset=utf-8', + }); + const filename = `${resource.metadata?.name || 'resource'}.yaml`; + saveAs(blob, filename); + } catch (e) { + // eslint-disable-next-line no-console + console.error('Failed to download YAML:', e); + } };e2e-tests/Containerfile (1)
15-20: Hardcoded Yarn version in wrapper script (yarn-4.12.0.cjs) is fragile.If
.yarnrc.ymlyarnPathis bumped, this wrapper must be updated in lockstep. Consider deriving the path dynamically:Suggested improvement
-RUN printf '#!/bin/sh\nexec node /tmp/e2e/.yarn/releases/yarn-4.12.0.cjs "$@"\n' > /usr/local/bin/yarn && \ +RUN YARN_CJS=$(ls /tmp/e2e/.yarn/releases/yarn-*.cjs | head -1) && \ + printf '#!/bin/sh\nexec node %s "$@"\n' "$YARN_CJS" > /usr/local/bin/yarn && \ chmod +x /usr/local/bin/yarnsrc/hooks/__tests__/useLatestBuildPipelines.spec.ts (1)
71-87: Inconsistent mock data: missing push event-type label in the third test.The "should return build pipelines" test (Line 50) constructs
mockPushPipelineRunwith the'pipelinesascode.tekton.dev/event-type': 'push'label, but this test still uses the rawtestPipelineRuns[DataState.RUNNING]without it. While the test still passes because the K8s watch mock bypasses label-selector filtering, the inconsistency reduces test clarity and could mask regressions if the hook's internal logic ever checks this label.Consider applying the same
mockPushPipelineRunpattern here for consistency.src/utils/__tests__/common-utils.spec.ts (1)
134-142: Consider usingasync/awaitfor the FileReader assertion.Returning a
.then()promise works butasync/awaitis more consistent with the rest of the codebase's testing style.♻️ Suggested refactor
- // Verify blob content by reading it - const reader = new FileReader(); - const readPromise = new Promise<string>((resolve) => { - reader.onload = (e) => resolve(e.target?.result as string); - }); - reader.readAsText(blob as Blob); - return readPromise.then((content) => { - expect(content).toBe(mockYamlContent); - }); + // Verify blob content by reading it + const reader = new FileReader(); + const readPromise = new Promise<string>((resolve) => { + reader.onload = (e) => resolve(e.target?.result as string); + }); + reader.readAsText(blob as Blob); + const content = await readPromise; + expect(content).toBe(mockYamlContent);Note: The test function signature on Line 109 would need to become
async:it('should create Blob with correct YAML content', async () => {src/components/TaskRunDetailsView/__tests__/TaskRunDetailsView.spec.tsx (1)
38-38: Remove commented-out code.Line 38 has a commented-out
sanitizeHtmlMockvariable. Dead code should be removed to keep the test file clean. Based on learnings: in the konflux-ci/konflux-ui repository, if code contains comments that suppress lint warnings or errors, remind contributors to fix the underlying issue rather than silencing the rule. Similarly, commented-out code should be cleaned up.src/components/ComponentsPage/ComponentDetails/__tests__/ComponentDetailsView.spec.tsx (1)
68-73: Prefer semantic querygetByRole('progressbar')overgetByTestId('spinner').PatternFly's
<Spinner>renders withrole="progressbar". UsinggetByRoleis more aligned with testing-library best practices and the pattern used in sibling test files (e.g.,TaskRunDetailsView.spec.tsxusesscreen.getByRole('progressbar')).♻️ Suggested fix
- expect(screen.getByTestId('spinner')).toBeInTheDocument(); + expect(screen.getByRole('progressbar')).toBeInTheDocument();As per coding guidelines: "Prioritize semantic queries (
getByRole,getByLabelText) over implementation-detail queries (getByTestId,getByClassName)".package.json (1)
139-139:packageManagerversion diverges from coding guidelines.The coding guidelines specify
"packageManager": "yarn@4.6.0"but this setsyarn@4.12.0. If this is an intentional upgrade, the coding guidelines should be updated accordingly to avoid confusion for future contributors.src/components/ImportForm/validation.utils.ts (1)
13-13: Consider listing more supported providers in the error message.The validation message
'Must include a domain (e.g. github)'could be more helpful now that the regex accepts multiple providers. Something like'Must be a supported Git provider (e.g. GitHub, GitLab, Forgejo)'would give users clearer guidance on what's expected.src/utils/__tests__/validation-utils.spec.ts (1)
5-29: Good test coverage for the new Forgejo regex support.One gap:
bitbucketis also in the regex but has no test case. Consider adding a test for Bitbucket URLs to ensure full coverage of all supported providers.src/components/ImportForm/ComponentSection/SourceSection.tsx (1)
75-75: Placeholder text doesn't mention Forgejo.The input placeholder still says
"Enter a GitHub or GitLab repository URL"but Forgejo is now a supported provider. Consider updating it to include Forgejo for consistency with the rest of this feature.src/components/ComponentsPage/ComponentDetails/ComponentDetailsView.tsx (1)
1-15: Mixed import style: absolute vs. relative paths.Line 4 uses the
~/shared/...alias while lines 5–14 use deep relative paths (../../../...). The project guidelines prefer absolute imports with configured path aliases (~/components,~/utils,~/models,@routes, etc.).Consider aligning all imports to use the path aliases for consistency.
♻️ Suggested alignment
-import emptyStateImgUrl from '../../../assets/Components.svg'; -import { FeatureFlagIndicator } from '../../../feature-flags/FeatureFlagIndicator'; -import { FLAGS } from '../../../feature-flags/flags'; -import { IfFeature } from '../../../feature-flags/hooks'; -import { useComponent } from '../../../hooks/useComponents'; -import { COMPONENTS_PATH, COMPONENT_DETAILS_V2_PATH } from '../../../routes/paths'; -import { RouterParams } from '../../../routes/utils'; -import AppEmptyState from '../../../shared/components/empty-state/AppEmptyState'; -import { useNamespace } from '../../../shared/providers/Namespace/useNamespaceInfo'; -import { DetailsPage } from '../../DetailsPage'; -import GitRepoLink from '../../GitLink/GitRepoLink'; +import emptyStateImgUrl from '~/assets/Components.svg'; +import { FeatureFlagIndicator } from '~/feature-flags/FeatureFlagIndicator'; +import { FLAGS } from '~/feature-flags/flags'; +import { IfFeature } from '~/feature-flags/hooks'; +import { useComponent } from '~/hooks/useComponents'; +import { COMPONENTS_PATH, COMPONENT_DETAILS_V2_PATH } from '@routes/paths'; +import { RouterParams } from '@routes/utils'; +import AppEmptyState from '~/shared/components/empty-state/AppEmptyState'; +import { useNamespace } from '~/shared/providers/Namespace/useNamespaceInfo'; +import { DetailsPage } from '~/components/DetailsPage'; +import GitRepoLink from '~/components/GitLink/GitRepoLink';As per coding guidelines: "Use absolute imports with configured path aliases:
~/components,~/types,~/k8s,~/utils,~/models,@routes"src/components/ComponentsPage/ComponentDetails/index.ts (1)
23-24: Cross-directory re-export from a sibling feature folder.Line 24 re-exports
ComponentDetailsTabfrom../../Components/ComponentDetails/tabs/ComponentDetailsTab, which reaches into the olderComponentsfeature directory from the newerComponentsPagedirectory. This creates a coupling between the two feature directories.This is fine for now if it's a deliberate reuse strategy, but worth noting for future cleanup when the v2 components page is fully established.
.github/workflows/pr-check.yaml (1)
43-49: Hardcoded allowlist requires manual updates for team changes.Replacing the org membership API call with a static
ALLOWED_USERSarray means every team roster change requires a workflow file update and merge. This is a pragmatic trade-off (no token/API dependency), but worth documenting the maintenance expectation — or consider extracting the list to a separate config file or GitHub team-based check.src/routes/page-routes/components-page.tsx (1)
2-7: Inconsistent import style: relative paths vs. path aliases.Line 1 uses the
@routesalias, but the new imports on lines 2–7 use relative paths. Per coding guidelines, prefer absolute imports with configured path aliases (~/components,@routes).Proposed fix
-import { - ComponentDetailsTab, - ComponentDetailsViewLayout, - componentDetailsViewLoader, -} from '../../components/ComponentsPage/ComponentDetails'; -import { COMPONENT_DETAILS_V2_PATH, COMPONENTS_PATH } from '../paths'; +import { + ComponentDetailsTab, + ComponentDetailsViewLayout, + componentDetailsViewLoader, +} from '~/components/ComponentsPage/ComponentDetails'; +import { COMPONENT_DETAILS_V2_PATH, COMPONENTS_PATH } from '@routes/paths';As per coding guidelines: "Use absolute imports with configured path aliases:
~/components,~/types,~/k8s,~/utils,~/models,@routes"Dockerfile.instrumented (1)
24-27: Hardcoded Yarn version in wrapper script is fragile.The filename
yarn-4.12.0.cjsis hardcoded. If the bundled Yarn version changes, this line silently breaks. Consider deriving the filename dynamically.Proposed fix
-RUN printf '#!/bin/sh\nexec node /opt/app-root/src/.yarn/releases/yarn-4.12.0.cjs "$@"\n' > /usr/local/bin/yarn && \ - chmod +x /usr/local/bin/yarn +RUN YARN_CJS=$(ls /opt/app-root/src/.yarn/releases/yarn-*.cjs | head -1) && \ + printf '#!/bin/sh\nexec node %s "$@"\n' "$YARN_CJS" > /usr/local/bin/yarn && \ + chmod +x /usr/local/bin/yarnDockerfile (1)
13-16: Same hardcoded Yarn version asDockerfile.instrumented.The
yarn-4.12.0.cjsfilename is duplicated here. The same dynamic resolution suggested forDockerfile.instrumentedapplies.Proposed fix
-RUN printf '#!/bin/sh\nexec node /opt/app-root/src/.yarn/releases/yarn-4.12.0.cjs "$@"\n' > /usr/local/bin/yarn && \ - chmod +x /usr/local/bin/yarn +RUN YARN_CJS=$(ls /opt/app-root/src/.yarn/releases/yarn-*.cjs | head -1) && \ + printf '#!/bin/sh\nexec node %s "$@"\n' "$YARN_CJS" > /usr/local/bin/yarn && \ + chmod +x /usr/local/bin/yarn
| # Configure Yarn Berry cache location | ||
| sudo mkdir -p /mnt/yarn_cache | ||
| sudo chown -R runner:docker /mnt/yarn_cache | ||
| export YARN_CACHE_FOLDER=/mnt/yarn_cache |
There was a problem hiding this comment.
YARN_CACHE_FOLDER won't persist to subsequent steps.
export only sets the variable within the current run block's shell. To make it available in later steps, append it to $GITHUB_ENV.
Proposed fix
- export YARN_CACHE_FOLDER=/mnt/yarn_cache
+ echo "YARN_CACHE_FOLDER=/mnt/yarn_cache" >> "$GITHUB_ENV"📝 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.
| # Configure Yarn Berry cache location | |
| sudo mkdir -p /mnt/yarn_cache | |
| sudo chown -R runner:docker /mnt/yarn_cache | |
| export YARN_CACHE_FOLDER=/mnt/yarn_cache | |
| # Configure Yarn Berry cache location | |
| sudo mkdir -p /mnt/yarn_cache | |
| sudo chown -R runner:docker /mnt/yarn_cache | |
| echo "YARN_CACHE_FOLDER=/mnt/yarn_cache" >> "$GITHUB_ENV" |
🤖 Prompt for AI Agents
In @.github/workflows/periodic-job-local.yaml around lines 77 - 80, The
YARN_CACHE_FOLDER export only affects the current run shell and won't persist to
later steps; update the workflow step that currently creates /mnt/yarn_cache and
sets YARN_CACHE_FOLDER so that, instead of relying on export, it appends the
YARN_CACHE_FOLDER key/value to the $GITHUB_ENV file (so subsequent steps see
it). Locate the lines that create and chown /mnt/yarn_cache and replace the
export YARN_CACHE_FOLDER action with writing the
YARN_CACHE_FOLDER=/mnt/yarn_cache entry into GITHUB_ENV (referencing the
YARN_CACHE_FOLDER variable and $GITHUB_ENV in the change).
| - name: Move npm/yarn cache to /mnt | ||
| run: | | ||
| sudo mkdir -p /mnt/npm_cache | ||
| sudo chown -R runner:docker /mnt/npm_cache | ||
| sudo npm config set cache /mnt/npm_cache --global | ||
| # Configure Yarn Berry cache location | ||
| sudo mkdir -p /mnt/yarn_cache | ||
| sudo chown -R runner:docker /mnt/yarn_cache | ||
| export YARN_CACHE_FOLDER=/mnt/yarn_cache |
There was a problem hiding this comment.
YARN_CACHE_FOLDER won't persist to subsequent steps.
export in a run block only affects the current shell. Later steps (e.g., yarn install) won't see it. Use $GITHUB_ENV instead.
Proposed fix
- export YARN_CACHE_FOLDER=/mnt/yarn_cache
+ echo "YARN_CACHE_FOLDER=/mnt/yarn_cache" >> $GITHUB_ENV📝 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.
| - name: Move npm/yarn cache to /mnt | |
| run: | | |
| sudo mkdir -p /mnt/npm_cache | |
| sudo chown -R runner:docker /mnt/npm_cache | |
| sudo npm config set cache /mnt/npm_cache --global | |
| # Configure Yarn Berry cache location | |
| sudo mkdir -p /mnt/yarn_cache | |
| sudo chown -R runner:docker /mnt/yarn_cache | |
| export YARN_CACHE_FOLDER=/mnt/yarn_cache | |
| - name: Move npm/yarn cache to /mnt | |
| run: | | |
| sudo mkdir -p /mnt/npm_cache | |
| sudo chown -R runner:docker /mnt/npm_cache | |
| sudo npm config set cache /mnt/npm_cache --global | |
| # Configure Yarn Berry cache location | |
| sudo mkdir -p /mnt/yarn_cache | |
| sudo chown -R runner:docker /mnt/yarn_cache | |
| echo "YARN_CACHE_FOLDER=/mnt/yarn_cache" >> $GITHUB_ENV |
🤖 Prompt for AI Agents
In @.github/workflows/pr-check.yaml around lines 135 - 143, The workflow step
sets YARN_CACHE_FOLDER with export which only affects the current shell; change
that to persist the variable across steps by writing
"YARN_CACHE_FOLDER=/mnt/yarn_cache" to GITHUB_ENV (use the runner provided
$GITHUB_ENV) instead of using export; update the block that currently creates
/mnt/yarn_cache and exports YARN_CACHE_FOLDER so it echoes the variable into
GITHUB_ENV (ensure the variable name matches where later steps expect it, e.g.,
YARN_CACHE_FOLDER) and keep the mkdir/chown lines for /mnt/yarn_cache.
| - name: Enable Corepack 📦 | ||
| run: corepack enable | ||
|
|
||
| - name: Setup Node.js | ||
| uses: actions/setup-node@v6 | ||
| with: | ||
| node-version: '20' |
There was a problem hiding this comment.
Move corepack enable after Setup Node.js.
corepack enable is currently run before actions/setup-node@v6. Since setup-node may install a different Node binary (v20), the corepack shims created beforehand could point to the wrong Node installation. The standard ordering is: checkout → setup-node → corepack enable.
Proposed fix: reorder steps
- name: Checkout
uses: actions/checkout@v5
- - name: Enable Corepack 📦
- run: corepack enable
-
- name: Setup Node.js
uses: actions/setup-node@v6
with:
node-version: '20'
+ - name: Enable Corepack 📦
+ run: corepack enable
+
- name: Install dependencies📝 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.
| - name: Enable Corepack 📦 | |
| run: corepack enable | |
| - name: Setup Node.js | |
| uses: actions/setup-node@v6 | |
| with: | |
| node-version: '20' | |
| - name: Setup Node.js | |
| uses: actions/setup-node@v6 | |
| with: | |
| node-version: '20' | |
| - name: Enable Corepack 📦 | |
| run: corepack enable |
🤖 Prompt for AI Agents
In @.github/workflows/pr-report.yaml around lines 19 - 25, Move the "Enable
Corepack 📦" step so it runs after the "Setup Node.js" step (the one using
actions/setup-node@v6 with node-version: '20'); specifically, reorder the
workflow steps so "Setup Node.js" executes before the "Enable Corepack 📦" step
to ensure Corepack shims are created against the installed Node binary (maintain
the node-version: '20' value).
| if [ -d "/e2e" ]; then | ||
| cd /e2e | ||
| npm i | ||
| npx cypress install | ||
| yarn install | ||
| yarn cypress install | ||
| chmod -R a+rwx ../e2e | ||
| else | ||
| cd /tmp/e2e | ||
| fi |
There was a problem hiding this comment.
Guard cd failures to avoid running in the wrong directory.
If cd /e2e or cd /tmp/e2e fails, the script continues and can write artifacts to unexpected locations. Add a fail-fast guard.
✅ Suggested fix
- cd /e2e
+ cd /e2e || exit 1
@@
- cd /tmp/e2e
+ cd /tmp/e2e || exit 1📝 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.
| if [ -d "/e2e" ]; then | |
| cd /e2e | |
| npm i | |
| npx cypress install | |
| yarn install | |
| yarn cypress install | |
| chmod -R a+rwx ../e2e | |
| else | |
| cd /tmp/e2e | |
| fi | |
| if [ -d "/e2e" ]; then | |
| cd /e2e || exit 1 | |
| yarn install | |
| yarn cypress install | |
| chmod -R a+rwx ../e2e | |
| else | |
| cd /tmp/e2e || exit 1 | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 9-9: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 14-14: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
In `@e2e-tests/entrypoint.sh` around lines 8 - 15, The script currently performs
"cd /e2e" and "cd /tmp/e2e" without checking for failures, so if a cd fails the
script continues in the wrong directory; update the entrypoint to fail-fast by
ensuring each cd is guarded (e.g., after the "cd /e2e" and "cd /tmp/e2e"
statements add a check that exits on failure or enable strict mode with a safe
error-handling fallback), and emit a clear error message before exiting so
artifacts are not written to unexpected locations.
|
|
||
| > **Note:** This project uses [Yarn Berry (v4.x)](https://yarnpkg.com/) as the package manager. The correct Yarn version is automatically managed via the `packageManager` field in `package.json` and Corepack. | ||
|
|
||
| A step by step series of examples that tell you how to get a development environment running: |
There was a problem hiding this comment.
Minor grammar: "step by step" → "step-by-step".
When used as a compound adjective before a noun, it should be hyphenated.
Proposed fix
-A step by step series of examples that tell you how to get a development environment running:
+A step-by-step series of examples that tell you how to get a development environment running:📝 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.
| A step by step series of examples that tell you how to get a development environment running: | |
| A step-by-step series of examples that tell you how to get a development environment running: |
🧰 Tools
🪛 LanguageTool
[grammar] ~25-~25: Use a hyphen to join words.
Context: ... in package.json and Corepack. A step by step series of examples that tell you...
(QB_NEW_EN_HYPHEN)
[grammar] ~25-~25: Use a hyphen to join words.
Context: ... package.json and Corepack. A step by step series of examples that tell you ho...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
In `@README.md` at line 25, Change the phrase "A step by step series of examples
that tell you how to get a development environment running:" to use the compound
adjective form "step-by-step"; locate the sentence containing "A step by step
series of examples..." in the README and replace it with "A step-by-step series
of examples that tell you how to get a development environment running:" to
correct the grammar.
| import BugRFESection from '../components/BugRFESection'; | ||
| import { FeedbackSections } from '../consts'; | ||
| import { formikRenderer } from '~/unit-test-utils'; |
There was a problem hiding this comment.
Fix import order per ESLint rule.
The ~/unit-test-utils absolute import should precede the relative imports.
Proposed fix
-import BugRFESection from '../components/BugRFESection';
-import { FeedbackSections } from '../consts';
-import { formikRenderer } from '~/unit-test-utils';
+import { formikRenderer } from '~/unit-test-utils';
+import BugRFESection from '../components/BugRFESection';
+import { FeedbackSections } from '../consts';📝 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.
| import BugRFESection from '../components/BugRFESection'; | |
| import { FeedbackSections } from '../consts'; | |
| import { formikRenderer } from '~/unit-test-utils'; | |
| import { formikRenderer } from '~/unit-test-utils'; | |
| import BugRFESection from '../components/BugRFESection'; | |
| import { FeedbackSections } from '../consts'; |
🧰 Tools
🪛 ESLint
[error] 3-3: ~/unit-test-utils import should occur before import of ../components/BugRFESection
(import/order)
🤖 Prompt for AI Agents
In `@src/components/FeedbackSection/__tests__/BugRFESection.spec.tsx` around lines
1 - 3, Reorder the import statements so the absolute import comes before
relative imports: move the import of formikRenderer (from '~/unit-test-utils')
above the imports of BugRFESection and FeedbackSections; ensure the import list
reads with the absolute module first, then the relative module imports
(references: formikRenderer, BugRFESection, FeedbackSections).
| it('should show RFEForm when current Section is FeatureSection', () => { | ||
| const screen = formikRenderer( | ||
| <BugRFESection currentSection={FeedbackSections.FeatureSection} />, | ||
| ); | ||
| screen.getByText('Request a new feature'); | ||
| screen.getByText(/Please provide detailed description of the feature/); | ||
| screen.getByText('Title'); | ||
| screen.getByText(/Please provide detailed description of the bug/); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find and display the BugRFESection component
fd 'BugRFESection.tsx' --exclude '__tests__' -x cat {}Repository: konflux-ci/konflux-ui
Length of output: 2882
🏁 Script executed:
#!/bin/bash
# Also examine the full test file to understand the context
fd 'BugRFESection.spec.tsx' -x wc -l {} && fd 'BugRFESection.spec.tsx' -x cat {}Repository: konflux-ci/konflux-ui
Length of output: 1191
Remove the incorrect assertion on line 23.
The test asserts 'Please provide detailed description of the bug' in the FeatureSection test, but the component renders 'Please provide detailed description of the feature' when currentSection === FeedbackSections.FeatureSection. This assertion will fail or is a leftover copy-paste error from the BugSection test. Delete line 23 or replace it with the correct feature-related assertion.
🤖 Prompt for AI Agents
In `@src/components/FeedbackSection/__tests__/BugRFESection.spec.tsx` around lines
16 - 24, In the BugRFESection.spec.tsx test for "should show RFEForm when
current Section is FeatureSection", remove the incorrect assertion that checks
for 'Please provide detailed description of the bug' and either delete that line
or replace it with the correct feature text assertion (matching the component
output when currentSection === FeedbackSections.FeatureSection, e.g., the
feature description string rendered by BugRFESection); target the test block
that renders <BugRFESection currentSection={FeedbackSections.FeatureSection} />
to make the correction.
| @@ -0,0 +1,19 @@ | |||
| import { render } from '@testing-library/react'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate and examine FeedbackModal component
fd 'FeedbackModal\.tsx' --exclude '__tests__' --type fRepository: konflux-ci/konflux-ui
Length of output: 113
🏁 Script executed:
#!/bin/bash
# Check the test file content
cat -n src/components/FeedbackSection/__tests__/FeedbackModal.spec.tsxRepository: konflux-ci/konflux-ui
Length of output: 871
🏁 Script executed:
#!/bin/bash
# Examine FeedbackModal component content
cat -n src/components/FeedbackSection/FeedbackModal.tsxRepository: konflux-ci/konflux-ui
Length of output: 8315
🏁 Script executed:
#!/bin/bash
# Check what rendering utilities exist in unit-test-utils
fd 'rendering-utils\|test.*utils' --type f | grep -E 'unit-test-utils|test-utils'Repository: konflux-ci/konflux-ui
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Look for formikRenderer usage in existing tests
rg 'formikRenderer|renderWithQueryClientAndRouter|renderWithQueryClient' --type tsx --type ts -A 2Repository: konflux-ci/konflux-ui
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Find unit-test-utils directory and rendering utilities
find . -path '*unit-test-utils*' -name '*.ts' -o -path '*unit-test-utils*' -name '*.tsx' | head -20Repository: konflux-ci/konflux-ui
Length of output: 749
🏁 Script executed:
#!/bin/bash
# Search for formikRenderer and other rendering utilities
rg 'formikRenderer|renderWithQueryClientAndRouter|renderWithQueryClient' -l --max-count 20Repository: konflux-ci/konflux-ui
Length of output: 6617
🏁 Script executed:
#!/bin/bash
# Examine the rendering-utils.tsx file
cat -n src/unit-test-utils/rendering-utils.tsxRepository: konflux-ci/konflux-ui
Length of output: 3271
🏁 Script executed:
#!/bin/bash
# Check other FeedbackSection test files to see rendering patterns
find src/components/FeedbackSection -name '*.spec.tsx' -o -name '*.spec.ts'Repository: konflux-ci/konflux-ui
Length of output: 259
🏁 Script executed:
#!/bin/bash
# Look at BeginningSection to see if it needs React Query or Router
cat -n src/components/FeedbackSection/components/BeginningSection.tsxRepository: konflux-ci/konflux-ui
Length of output: 3516
🏁 Script executed:
#!/bin/bash
# Check what rendering utility is used in other FeedbackSection tests
head -20 src/components/FeedbackSection/__tests__/BeginningSection.spec.tsxRepository: konflux-ci/konflux-ui
Length of output: 537
🏁 Script executed:
#!/bin/bash
# Check BugRFESection test
head -30 src/components/FeedbackSection/__tests__/BugRFESection.spec.tsxRepository: konflux-ci/konflux-ui
Length of output: 1122
Use formikRenderer from ~/unit-test-utils to wrap FeedbackModal.
FeedbackModal wraps its content in a <Formik> context (lines 67–92). Child components like BugRFESection, which depend on Formik context, require this wrapper. The BugRFESection test correctly uses formikRenderer from ~/unit-test-utils/rendering-utils, but FeedbackModal's test does not. For consistency and proper context setup, use formikRenderer instead of bare render:
import { formikRenderer } from '~/unit-test-utils';
it('should initialize with BeginingSection Cards', () => {
const screen = formikRenderer(<FeedbackModal />);
// ... rest of test
});🤖 Prompt for AI Agents
In `@src/components/FeedbackSection/__tests__/FeedbackModal.spec.tsx` at line 1,
Replace the bare render call with the shared Formik-aware renderer so
FeedbackModal tests run inside a Formik context: import and use formikRenderer
from '~/unit-test-utils' (like the BugRFESection tests do) when rendering
<FeedbackModal /> in FeedbackModal.spec.tsx so components nested in Formik
(FeedbackModal, BugRFESection, BeginingSection, etc.) receive the Formik context
during the test.
| it('should initialize with BeginingSection Cards', () => { | ||
| const screen = render(<FeedbackModal />); | ||
| screen.getByText('Share feedback'); | ||
| screen.getByText('Report a bug'); | ||
| screen.getByText('Request a new feature'); |
There was a problem hiding this comment.
Typo in test name and inconsistent assertion style.
- "BeginingSection" → "BeginningSection" (missing 'n').
- Lines 11-13 rely on implicit throw-on-miss from
getByText, while line 17 wraps the query inexpect(...).toBeInTheDocument(). Pick one style consistently — the explicitexpectform is preferred for readability.
Proposed fix
- it('should initialize with BeginingSection Cards', () => {
- const screen = render(<FeedbackModal />);
- screen.getByText('Share feedback');
- screen.getByText('Report a bug');
- screen.getByText('Request a new feature');
+ it('should initialize with BeginningSection Cards', () => {
+ const { getByText } = render(<FeedbackModal />);
+ expect(getByText('Share feedback')).toBeInTheDocument();
+ expect(getByText('Report a bug')).toBeInTheDocument();
+ expect(getByText('Request a new feature')).toBeInTheDocument();📝 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.
| it('should initialize with BeginingSection Cards', () => { | |
| const screen = render(<FeedbackModal />); | |
| screen.getByText('Share feedback'); | |
| screen.getByText('Report a bug'); | |
| screen.getByText('Request a new feature'); | |
| it('should initialize with BeginningSection Cards', () => { | |
| const { getByText } = render(<FeedbackModal />); | |
| expect(getByText('Share feedback')).toBeInTheDocument(); | |
| expect(getByText('Report a bug')).toBeInTheDocument(); | |
| expect(getByText('Request a new feature')).toBeInTheDocument(); |
🤖 Prompt for AI Agents
In `@src/components/FeedbackSection/__tests__/FeedbackModal.spec.tsx` around lines
9 - 13, Rename the test "should initialize with BeginingSection Cards" to
"should initialize with BeginningSection Cards" and make the assertions
consistent by replacing raw getByText calls with explicit expect assertions; in
the "FeedbackModal" test use render(<FeedbackModal />) as before, but change
screen.getByText('...') to expect(screen.getByText('Share
feedback')).toBeInTheDocument(), expect(screen.getByText('Report a
bug')).toBeInTheDocument(), and expect(screen.getByText('Request a new
feature')).toBeInTheDocument() so all assertions use the explicit
expect(...).toBeInTheDocument() style.
| if (hasDomain(url, 'forgejo.org')) { | ||
| return GitProvider.FORGEJO; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'forgejo' --type ts --type tsx -C 3 -iRepository: konflux-ci/konflux-ui
Length of output: 92
🏁 Script executed:
cd src/shared/utils && cat -n git-utils.tsx | head -60Repository: konflux-ci/konflux-ui
Length of output: 2298
🏁 Script executed:
rg -n 'hasDomain|GitProvider' src/shared/utils/git-utils.tsx -A 2 -B 2Repository: konflux-ci/konflux-ui
Length of output: 1431
🏁 Script executed:
fd validation-utils.ts src/ && cat src/shared/utils/validation-utils.ts | grep -n -C 3 'forgejo'Repository: konflux-ci/konflux-ui
Length of output: 373
🏁 Script executed:
rg -i 'forgejo' -g '!node_modules' -C 3Repository: konflux-ci/konflux-ui
Length of output: 10057
🏁 Script executed:
cat src/utils/validation-utils.ts | grep -n -A 5 -B 5 'forgejo' -iRepository: konflux-ci/konflux-ui
Length of output: 824
🏁 Script executed:
grep -r 'codeberg' src/ 2>/dev/null || echo "No codeberg references found"Repository: konflux-ci/konflux-ui
Length of output: 93
🏁 Script executed:
cat src/utils/validation-utils.ts | head -100 | tail -50Repository: konflux-ci/konflux-ui
Length of output: 1617
🏁 Script executed:
find src -name "git-utils*" -type fRepository: konflux-ci/konflux-ui
Length of output: 158
🏁 Script executed:
# Check the old git-utils.tsx for comparison
cat src/utils/git-utils.tsx | grep -n -A 10 "endsWith.*forgejo"Repository: konflux-ci/konflux-ui
Length of output: 357
🏁 Script executed:
# Check if src/shared/utils/git-utils.tsx is a new file or replacement
git log --oneline -n 20 src/shared/utils/git-utils.tsx src/utils/git-utils.tsx 2>/dev/null | head -20Repository: konflux-ci/konflux-ui
Length of output: 109
🏁 Script executed:
# Check imports to see which file is being used
rg 'from.*git-utils' src/ -g '!node_modules' --type ts --type tsx | head -20Repository: konflux-ci/konflux-ui
Length of output: 92
🏁 Script executed:
# Show the full detectGitType implementation in old location for comparison
cat src/utils/git-utils.tsx | head -50Repository: konflux-ci/konflux-ui
Length of output: 1513
🏁 Script executed:
# Check what domains Forgejo instances might use
rg 'forgejo|codeberg' src/utils/__tests__/git-utils.spec.tsx -A 3 -B 1Repository: konflux-ci/konflux-ui
Length of output: 268
🏁 Script executed:
# Check if there's any documentation or constants about Forgejo instance domains
rg -i 'codeberg\|forgejo.*domain\|forgejo.*instance' src/ -g '!node_modules'Repository: konflux-ci/konflux-ui
Length of output: 47
Match domain pattern matching across validation and detection logic.
The detectGitType function only detects forgejo.org exactly, but the validation regex (GIT_URL.DOMAIN_REGEX) accepts any domain matching forgejo\.[\w.-]+ (e.g., v14.next.forgejo.org, custom Forgejo instances). This creates an inconsistency: URLs like https://v14.next.forgejo.org/user/repo pass validation but return GitProvider.UNSURE instead of GitProvider.FORGEJO.
The path generation logic (src/utils/git-utils.tsx) uses endsWith('forgejo.org') which correctly handles subdomains. Update the detection to match: use endsWith('.forgejo.') or a similar pattern-based approach instead of the hardcoded hasDomain(url, 'forgejo.org') check.
🤖 Prompt for AI Agents
In `@src/shared/utils/git-utils.tsx` around lines 39 - 41, The detectGitType
function currently checks for forgejo with hasDomain(url, 'forgejo.org') which
only matches the exact domain; update detectGitType (and its use of hasDomain)
to detect Forgejo subdomains the same way validation does—either reuse
GIT_URL.DOMAIN_REGEX or change the check to match domains that end with
".forgejo.org" (or a regex like /(^|\.)forgejo\.org$/) so URLs such as
"v14.next.forgejo.org" return GitProvider.FORGEJO; adjust the condition in
detectGitType accordingly (replace the hasDomain(...) check) to ensure detection
aligns with validation and path-generation logic.
abhinandan13jan
force-pushed
the
add-feedback-modal
branch
from
0940160 to
316898d
Compare
abhinandan13jan
changed the title
abhinandan13jan
requested review from
Katka92 and
janaki29
and removed request for
a team
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/FeedbackSection/FeedbackValidationSchemas.ts`:
- Around line 23-31: The schema FeedbackSectionValidationSchema is missing
validation for the email input rendered as feedback.email in
FeedbackSection.tsx; add an optional email rule under the feedback object (e.g.,
add feedback.email: yup.string().email('Invalid email').notRequired() or
.nullable() so empty values pass but any provided value is checked for proper
email format), updating the FeedbackSectionValidationSchema object to include
this new field.
🧹 Nitpick comments (6)
src/components/FeedbackSection/consts.ts (1)
1-2: Stale TODO comment should be removed or clarified.Line 1 still reads
// update baseURL to point to correct codebase, but the URL now points tokonflux-ci/support. If this is the intended final URL, remove the comment. If it still needs updating, make the comment more specific about what's remaining.src/components/FeedbackSection/FeedbackModal.scss (2)
2-2: Use PatternFly design tokens instead of hardcoded5pxfor border-radius.Lines 2 and 55 use
border-radius: 5px. Per project guidelines, prefer PatternFly design tokens (e.g.,var(--pf-v5-global--BorderRadius--sm)) over hardcoded values. As per coding guidelines: "Use PatternFly design tokens (var(--pf-global--spacer--md), var(--pf-global--primary-color--100), etc.) instead of hardcoded values in SCSS."
7-8: Excessive!importantoverrides throughout the file.Lines 8, 11, 32, 34, 38, 41, and 51 all use
!important. This is often a sign of fighting PatternFly's default styles. Consider using more specific selectors or PatternFly's built-in customization props/CSS variables to avoid the override chain. Not blocking, but worth addressing to reduce long-term maintenance burden.src/components/FeedbackSection/components/BugRFESection.tsx (1)
14-14: Replace raw<div>elements with PatternFly layout components.Lines 14, 30, 31, and 41 use raw
<div>tags for layout. Per coding guidelines, use PatternFly layout components (Stack,StackItem,Flex,FlexItem, etc.) instead. As per coding guidelines: "NEVER use raw HTML layout tags (,, ) or custom CSS layouts. Always use PatternFly layout components." src/components/FeedbackSection/components/FeedbackSection.tsx (1)
9-9: Replace raw<div>elements with PatternFly layout components.Lines 9 and 20 use raw
<div>tags. Per coding guidelines, always use PatternFly layout components instead. As per coding guidelines: "NEVER use raw HTML layout tags (,, ) or custom CSS layouts." src/components/FeedbackSection/__tests__/BeginningSection.spec.tsx (1)
7-12: Typo in test name andsetCurrentSectionMockis never exercised.
- "BeginingSection" → "BeginningSection".
- The mock is created but no test verifies that clicking "Report a bug" / "Request a new feature" / "Share feedback" calls
setCurrentSectionwith the correct argument. This leaves the component's primary behavior untested.Consider adding interaction tests using
userEvent.setup():Proposed additions
+import userEvent from '@testing-library/user-event'; + describe('BeginningSection', () => { - it('should initialize with BeginingSection', () => { + it('should render all feedback option cards', () => { const screen = render(<BeginningSection setCurrentSection={setCurrentSectionMock} />); screen.getByText('Share feedback'); screen.getByText('Report a bug'); screen.getByText('Request a new feature'); }); + + it('should call setCurrentSection when a card is clicked', async () => { + const user = userEvent.setup(); + const screen = render(<BeginningSection setCurrentSection={setCurrentSectionMock} />); + await user.click(screen.getByText('Report a bug')); + expect(setCurrentSectionMock).toHaveBeenCalled(); + }); });As per coding guidelines: "Use
userEvent.setup()and async user interactions instead of synchronousfireEventfor testing user actions" and "Test both success and error scenarios for all components and features."
| export const FeedbackSectionValidationSchema = yup.object({ | ||
| feedback: yup.object({ | ||
| scale: yup.number().required('Required'), | ||
| description: yup | ||
| .string() | ||
| .required('Required') | ||
| .max(2000, 'Should be less than 2000 characters.'), | ||
| }), | ||
| }); |
There was a problem hiding this comment.
Missing email validation in FeedbackSectionValidationSchema.
The FeedbackSection.tsx component renders an email field (feedback.email), but this schema doesn't validate it. If the user provides an email, there's no format validation. Consider adding an optional email field with format check:
Proposed addition
export const FeedbackSectionValidationSchema = yup.object({
feedback: yup.object({
scale: yup.number().required('Required'),
description: yup
.string()
.required('Required')
.max(2000, 'Should be less than 2000 characters.'),
+ email: yup.string().email('Must be a valid email address'),
}),
});📝 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.
| export const FeedbackSectionValidationSchema = yup.object({ | |
| feedback: yup.object({ | |
| scale: yup.number().required('Required'), | |
| description: yup | |
| .string() | |
| .required('Required') | |
| .max(2000, 'Should be less than 2000 characters.'), | |
| }), | |
| }); | |
| export const FeedbackSectionValidationSchema = yup.object({ | |
| feedback: yup.object({ | |
| scale: yup.number().required('Required'), | |
| description: yup | |
| .string() | |
| .required('Required') | |
| .max(2000, 'Should be less than 2000 characters.'), | |
| email: yup.string().email('Must be a valid email address'), | |
| }), | |
| }); |
🤖 Prompt for AI Agents
In `@src/components/FeedbackSection/FeedbackValidationSchemas.ts` around lines 23
- 31, The schema FeedbackSectionValidationSchema is missing validation for the
email input rendered as feedback.email in FeedbackSection.tsx; add an optional
email rule under the feedback object (e.g., add feedback.email:
yup.string().email('Invalid email').notRequired() or .nullable() so empty values
pass but any provided value is checked for proper email format), updating the
FeedbackSectionValidationSchema object to include this new field.
There was a problem hiding this comment.
This svg doesn't looks right for dark theme. Collaborate with ux to improve the svg. You can also refer to the patternfly feedback form svg for light and dark theme
There was a problem hiding this comment.
Primary color for konflux is of orange shade. This red color in svg might conflict with the overall theme of konflux
| <CardBody> | ||
| <TextContent> | ||
| <Text component={TextVariants.small}> | ||
| Describe the bug you encountered. For urgent issues, use #konflux-user-forum |
There was a problem hiding this comment.
There should be link here for #konflux-user-forum
| }; | ||
|
|
||
| return ( | ||
| <Formik |
There was a problem hiding this comment.
I dont think you need formik for such simple forms. You can just use HTML forms here without formik
| <Button | ||
| variant="primary" | ||
| type={ButtonType.submit} | ||
| isDisabled={isEmpty(touched) || !isEmpty(errors) || isSubmitting} | ||
| > | ||
| Preview on Github | ||
| </Button> | ||
| )} | ||
| {currentSection === FeedbackSections.FeedbackSection && ( | ||
| <Button | ||
| variant="primary" | ||
| type={ButtonType.submit} | ||
| isDisabled={isEmpty(touched) || !isEmpty(errors) || isSubmitting} | ||
| > | ||
| Submit feedback | ||
| </Button> | ||
| )} | ||
| {currentSection !== FeedbackSections.BeginningSection && ( | ||
| <Button | ||
| variant="secondary" | ||
| onClick={() => setCurrentSection(FeedbackSections.BeginningSection)} | ||
| > | ||
| Back | ||
| </Button> | ||
| )} | ||
| <Button variant="link" onClick={() => onClose(null, { submitClicked: false })}> | ||
| Cancel | ||
| </Button> |
There was a problem hiding this comment.
These buttons should be part of their own forms
2 participants
Fixes
KFLUXUI-1035
KFLUXUI-1036
KFLUXUI-1037
Description
Adds MastHead button, Feedback Modal with Beginning sections, Bug creation Form, Feature Request Form & Feedback Form.
Remaining work
Screen shots / Gifs for design review
Type of change
How to test or reproduce?
Browser conformance:
Summary by CodeRabbit