🐛 Block save when no targets are selected #1231
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ 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. |
ibolton336
changed the title
Contributor
📝 WalkthroughWalkthroughAnalysisPage removes a local profile-invalid flag and derives profileValidationErrors from rawConfigErrors (types: Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webview-ui/src/components/AnalysisPage/ConfigButton/ConfigButton.tsx (1)
24-41:⚠️ Potential issue | 🟡 MinorAvoid empty
aria-labeland missing tooltip whenwarningMessageis"".An empty string passes
??unchanged and fails the tooltip condition, which can yield an empty label and no warning affordance. Prefer a truthy fallback check.🔧 Suggested fix
- aria-label={ - hasWarning ? (warningMessage ?? "Configuration needs attention") : "Configuration" - } + aria-label={ + hasWarning + ? (warningMessage || "Configuration needs attention") + : "Configuration" + } ... - if (hasWarning && warningMessage) { - return <Tooltip content={warningMessage}>{button}</Tooltip>; - } + if (hasWarning) { + return ( + <Tooltip content={warningMessage || "Configuration needs attention"}> + {button} + </Tooltip> + ); + }
🤖 Fix all issues with AI agents
In `@shared/src/labelSelector.ts`:
- Around line 6-12: The current hasTargetInLabelSelector function uses
/konveyor\.io\/target=\S+/ which treats negated selectors like
"!konveyor.io/target=eap7" as matches; add tests for negated-only selectors
(e.g., "!konveyor.io/target=eap7" and "!konveyor.io/target=eap7 &&
!konveyor.io/target=eap6") and decide intended behavior: if negated-only should
not count as a positive target, change hasTargetInLabelSelector to ignore
patterns preceded by "!" (e.g., use a regex that ensures the match is not
preceded by "!" or perform a simple parse that skips tokens starting with "!"
before testing for "konveyor.io/target="), update unit tests accordingly to
assert the chosen behavior, and keep function name hasTargetInLabelSelector as
the entry point to locate the change.
In `@webview-ui/src/components/ProfileManager/ProfileEditorForm.tsx`:
- Around line 190-229: canSaveProfile and validateAndSave currently ignore the
profile name validity, allowing autosave of empty/duplicate names; update the
logic to include a name validation check (use the existing name validator
function, e.g., validateProfileName or validateName) so that
canSaveProfile(profileToCheck, targets) returns false when the profile name is
invalid and validateAndSave(updatedProfile, targets) also gates saving on name
validity; update callers (updateLabelSelector, handleDefaultRulesChange,
handleRemoveCustomRule) to rely on the amended canSaveProfile/validateAndSave
behavior so autosave is prevented unless name, targets, and rules all validate.
🧹 Nitpick comments (3)
shared/src/__tests__/labelSelector.test.ts (1)
191-235: Good test coverage, consider adding edge case tests.The tests comprehensively cover the main scenarios. Consider adding tests for these edge cases to document expected behavior:
- Negated targets:
!konveyor.io/target=eap7- clarifies whether exclusions count as "having targets"- Empty target value:
konveyor.io/target=- verifies the\S+requirement- Partial match in different context:
some-other-konveyor.io/target=value- ensures the pattern matches correctly💡 Suggested additional test cases
+ it("should return true for negated targets (exclusion still indicates target configuration)", () => { + expect(hasTargetInLabelSelector("!konveyor.io/target=eap7")).toBe(true); + }); + + it("should return false for empty target value", () => { + expect(hasTargetInLabelSelector("konveyor.io/target=")).toBe(false); + });vscode/core/src/utilities/configuration.ts (2)
146-154: Potential duplicate errors for empty label selectors.When
profile.labelSelectoris empty or whitespace-only, bothinvalidLabelSelector(line 148) andprofileNoTargets(line 153) errors will be added sincehasTargetInLabelSelectorreturnsfalsefor empty strings.If showing both errors is intentional (to communicate both that the selector is invalid AND that targets are needed), this is fine. Otherwise, consider making the target check conditional on having a valid (non-empty) label selector:
💡 Potential refinement to avoid duplicate errors
// Check label selector if (!profile.labelSelector?.trim()) { draft.configErrors.push(createConfigError.invalidLabelSelector()); - } - - // Check for no target technologies in label selector - if (!hasTargetInLabelSelector(profile.labelSelector)) { + } else if (!hasTargetInLabelSelector(profile.labelSelector)) { + // Check for no target technologies only if label selector is non-empty draft.configErrors.push(createConfigError.profileNoTargets()); }
219-227: Same consideration applies here forupdateActiveProfileValidity.Similar to
updateConfigErrors, this function may add bothinvalidLabelSelectorandprofileNoTargetserrors when the label selector is empty. Consider applying the same conditional logic if only one error should be shown.
ibolton336
force-pushed
the
no-targets-warning
branch
from
d016a7b to
7448d4e
Compare
ibolton336
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@webview-ui/src/components/ProfileManager/ProfileEditorForm.tsx`:
- Around line 286-299: The status indicator currently only checks
targetsValidation and rulesValidation, causing it to ignore nameValidation;
update the conditional that determines the icon and status message in the
ProfileEditorForm component (the logic using targetsValidation, rulesValidation,
isSaving) to also consider nameValidation so that when nameValidation ===
"error" the UI shows the error state (ExclamationTriangleIcon and "Fix errors to
save") and not the success/saving messages; ensure both the icon branch and the
message branch include nameValidation in the combined error check.
🧹 Nitpick comments (2)
webview-ui/src/components/ProfileManager/ProfileEditorForm.tsx (2)
215-221: Consider usingvalidateAndSavefor consistency.This handler uses
canSaveProfilewhileupdateLabelSelectorusesvalidateAndSave. UsingvalidateAndSavehere would make the approach consistent and ensure validation state is set synchronously rather than relying on theuseEffect.♻️ Suggested refactor for consistency
const handleDefaultRulesChange = (_e: React.FormEvent, checked: boolean) => { const updated = { ...localProfile, useDefaultRules: checked }; setLocalProfile(updated); - if (canSaveProfile(updated, selectedTargets)) { - debouncedChange(updated); - } + validateAndSave(updated, selectedTargets); };
223-230: Same consistency suggestion applies here.Like
handleDefaultRulesChange, this could usevalidateAndSavefor a uniform approach across all handlers.♻️ Suggested refactor for consistency
const handleRemoveCustomRule = (index: number) => { const updatedRules = localProfile.customRules.filter((_, i) => i !== index); const newProfile = { ...localProfile, customRules: updatedRules }; setLocalProfile(newProfile); - if (canSaveProfile(newProfile, selectedTargets)) { - debouncedChange(newProfile); - } + validateAndSave(newProfile, selectedTargets); };
ibolton336
force-pushed
the
no-targets-warning
branch
from
7b15616 to
badc799
Compare
ibolton336
force-pushed
the
no-targets-warning
branch
from
71603af to
8cd1e70
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@webview-ui/src/components/ProfileManager/ProfileEditorForm.tsx`:
- Around line 240-246: The remove/close handlers currently allow modifying a
read-only profile; update handleRemoveCustomRule (and the close handler
referenced around the same block) to no-op when the profile is read-only by
checking localProfile.readOnly (or profile.readOnly) along with existing
isDisabled checks—i.e., early-return if localProfile.readOnly || isDisabled
before computing updatedRules or calling debouncedChange—so removal and close
actions are gated for read-only profiles.
- Around line 141-178: handleBlur trims the name for debouncedChange but does
not update localProfile, leaving the input showing trailing spaces; after
successful validateName(localProfile) update the local state with the trimmed
name (e.g., call setLocalProfile({ ...localProfile, name:
localProfile.name.trim() })) and then call debouncedChange with that trimmed
profile so both the persisted value and the rendered input are synchronized;
keep validateName and isNameValid logic unchanged.
Comment on lines
+141
to
178
| // Validates profile name and updates validation state | ||
| const validateName = (profileToCheck: AnalysisProfile): boolean => { | ||
| const trimmedName = profileToCheck.name.trim(); | ||
| const isDuplicate = | ||
| trimmedName !== profile.name && allProfiles.some((p) => p.name === trimmedName); | ||
| const isEmpty = trimmedName === ""; | ||
|
|
||
| if (isEmpty) { | ||
| setNameValidation("error"); | ||
| setNameErrorMsg("Profile name is required."); | ||
| return; | ||
| return false; | ||
| } | ||
|
|
||
| if (isDuplicate) { | ||
| setNameValidation("error"); | ||
| setNameErrorMsg("A profile with this name already exists."); | ||
| return; | ||
| return false; | ||
| } | ||
|
|
||
| setNameValidation("default"); | ||
| setNameErrorMsg(null); | ||
| debouncedChange({ ...localProfile, name: trimmedName }); | ||
| return true; | ||
| }; | ||
|
|
||
| // Checks name validity without updating state (for canSaveProfile) | ||
| const isNameValid = (profileToCheck: AnalysisProfile): boolean => { | ||
| const trimmedName = profileToCheck.name.trim(); | ||
| const isDuplicate = | ||
| trimmedName !== profile.name && allProfiles.some((p) => p.name === trimmedName); | ||
| const isEmpty = trimmedName === ""; | ||
| return !isEmpty && !isDuplicate; | ||
| }; | ||
|
|
||
| const handleBlur = () => { | ||
| if (validateName(localProfile)) { | ||
| debouncedChange({ ...localProfile, name: localProfile.name.trim() }); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Sync trimmed names back into local state on blur.
handleBlur saves a trimmed name but leaves localProfile.name unchanged, so the input can display trailing spaces while the persisted value is trimmed. Consider updating local state to the trimmed value when saving.
🛠️ Proposed fix
const handleBlur = () => {
- if (validateName(localProfile)) {
- debouncedChange({ ...localProfile, name: localProfile.name.trim() });
- }
+ const trimmedName = localProfile.name.trim();
+ const updated = { ...localProfile, name: trimmedName };
+ if (validateName(updated)) {
+ setLocalProfile(updated);
+ debouncedChange(updated);
+ }
};📝 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.
Suggested change
| // Validates profile name and updates validation state | |
| const validateName = (profileToCheck: AnalysisProfile): boolean => { | |
| const trimmedName = profileToCheck.name.trim(); | |
| const isDuplicate = | |
| trimmedName !== profile.name && allProfiles.some((p) => p.name === trimmedName); | |
| const isEmpty = trimmedName === ""; | |
| if (isEmpty) { | |
| setNameValidation("error"); | |
| setNameErrorMsg("Profile name is required."); | |
| return; | |
| return false; | |
| } | |
| if (isDuplicate) { | |
| setNameValidation("error"); | |
| setNameErrorMsg("A profile with this name already exists."); | |
| return; | |
| return false; | |
| } | |
| setNameValidation("default"); | |
| setNameErrorMsg(null); | |
| debouncedChange({ ...localProfile, name: trimmedName }); | |
| return true; | |
| }; | |
| // Checks name validity without updating state (for canSaveProfile) | |
| const isNameValid = (profileToCheck: AnalysisProfile): boolean => { | |
| const trimmedName = profileToCheck.name.trim(); | |
| const isDuplicate = | |
| trimmedName !== profile.name && allProfiles.some((p) => p.name === trimmedName); | |
| const isEmpty = trimmedName === ""; | |
| return !isEmpty && !isDuplicate; | |
| }; | |
| const handleBlur = () => { | |
| if (validateName(localProfile)) { | |
| debouncedChange({ ...localProfile, name: localProfile.name.trim() }); | |
| } | |
| }; | |
| // Validates profile name and updates validation state | |
| const validateName = (profileToCheck: AnalysisProfile): boolean => { | |
| const trimmedName = profileToCheck.name.trim(); | |
| const isDuplicate = | |
| trimmedName !== profile.name && allProfiles.some((p) => p.name === trimmedName); | |
| const isEmpty = trimmedName === ""; | |
| if (isEmpty) { | |
| setNameValidation("error"); | |
| setNameErrorMsg("Profile name is required."); | |
| return false; | |
| } | |
| if (isDuplicate) { | |
| setNameValidation("error"); | |
| setNameErrorMsg("A profile with this name already exists."); | |
| return false; | |
| } | |
| setNameValidation("default"); | |
| setNameErrorMsg(null); | |
| return true; | |
| }; | |
| // Checks name validity without updating state (for canSaveProfile) | |
| const isNameValid = (profileToCheck: AnalysisProfile): boolean => { | |
| const trimmedName = profileToCheck.name.trim(); | |
| const isDuplicate = | |
| trimmedName !== profile.name && allProfiles.some((p) => p.name === trimmedName); | |
| const isEmpty = trimmedName === ""; | |
| return !isEmpty && !isDuplicate; | |
| }; | |
| const handleBlur = () => { | |
| const trimmedName = localProfile.name.trim(); | |
| const updated = { ...localProfile, name: trimmedName }; | |
| if (validateName(updated)) { | |
| setLocalProfile(updated); | |
| debouncedChange(updated); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@webview-ui/src/components/ProfileManager/ProfileEditorForm.tsx` around lines
141 - 178, handleBlur trims the name for debouncedChange but does not update
localProfile, leaving the input showing trailing spaces; after successful
validateName(localProfile) update the local state with the trimmed name (e.g.,
call setLocalProfile({ ...localProfile, name: localProfile.name.trim() })) and
then call debouncedChange with that trimmed profile so both the persisted value
and the rendered input are synchronized; keep validateName and isNameValid logic
unchanged.
…lid profiles - Add 'profile-no-targets' ConfigError type to warn when active profile has no targets - Add hasTargetInLabelSelector() utility to check for targets in label selector - Update backend validation to check for no targets in active profile - Update ProfileEditorForm to not auto-save when validation fails - Move validation logic out of render method into helper functions - Centralize profile warnings through ConfigAlerts instead of one-off alerts - Add tooltip to ConfigButton when there are config warnings - Simplify AnalysisPage to use rawConfigErrors for all warning states - Add tests for hasTargetInLabelSelector function Signed-off-by: Ian Bolton <[email protected]>
The UI already prevents saving profiles without targets, making the backend validation redundant. The existing invalidLabelSelector error handles truly empty selectors. - Remove profile-no-targets ConfigError type - Remove hasTargetInLabelSelector() utility and tests - Remove profile-no-targets checks from backend validation - Keep UI-side validation in ProfileEditorForm (blocks saves, shows error) Signed-off-by: Ian Bolton <[email protected]>
- Add validateName() function that validates and updates state - Add isNameValid() pure function for checking without side effects - Update canSaveProfile() to check name, targets, and rules - Update validateAndSave() to gate saving on name validity - Update auto-save status indicator to show name validation errors - Prevents autosave of empty or duplicate profile names Signed-off-by: Ian Bolton <[email protected]>
The ServerStatusToggle was using rawConfigErrors.length > 0 which included all config errors like genai-disabled. This caused the Start button to be disabled even when the profile was valid. Now filters to only profile validation errors: - no-active-profile - invalid-label-selector - no-custom-rules This fixes the e2e test failure where the Start button was disabled. Signed-off-by: Ian Bolton <[email protected]>
- Add early return in handleRemoveCustomRule for read-only/disabled profiles - Update onClose condition to check profile.readOnly in addition to isDisabled - Ensures read-only profiles cannot have their custom rules modified Signed-off-by: Ian Bolton <[email protected]>
ibolton336
force-pushed
the
no-targets-warning
branch
from
8cd1e70 to
254be25
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@webview-ui/src/components/ProfileManager/ProfileEditorForm.tsx`:
- Around line 205-230: validateAndSave currently calls isNameValid (which only
checks validity) so a bad name blocks saving without updating validation state;
change validateAndSave to call validateName(updatedProfile) and use its result
to set validation state before deciding to save. Specifically, inside
validateAndSave replace the isProfileNameValid = isNameValid(...) call with a
call to validateName(updatedProfile) (or invoke whatever validateName
returns/side-effects to update UI state), then proceed to check isTargetsValid
and isRulesValid and call debouncedChange(updatedProfile) only if all are true;
updateLabelSelector remains the same but will now trigger validateAndSave which
will surface name errors via validateName.
Comment on lines
+205
to
+230
| // Helper to check if a profile can be saved (all validation passes) | ||
| const canSaveProfile = (profileToCheck: AnalysisProfile, targets: string[]): boolean => { | ||
| const hasValidName = isNameValid(profileToCheck); | ||
| const hasTargets = targets.length > 0; | ||
| const hasRules = | ||
| profileToCheck.useDefaultRules || (profileToCheck.customRules?.length ?? 0) > 0; | ||
| return hasValidName && hasTargets && hasRules; | ||
| }; | ||
|
|
||
| // Validates and optionally saves if valid | ||
| const validateAndSave = (updatedProfile: AnalysisProfile, targets: string[]) => { | ||
| const isProfileNameValid = isNameValid(updatedProfile); | ||
| const isTargetsValid = validateTargets(targets); | ||
| const isRulesValid = validateRules(updatedProfile); | ||
|
|
||
| if (isProfileNameValid && isTargetsValid && isRulesValid) { | ||
| debouncedChange(updatedProfile); | ||
| } | ||
| }; | ||
|
|
||
| const updateLabelSelector = (sources: string[], targets: string[]) => { | ||
| const selector = buildLabelSelector(sources, targets); | ||
| const updatedProfile = { ...localProfile, labelSelector: selector }; | ||
| setLocalProfile(updatedProfile); | ||
| validateAndSave(updatedProfile, targets); | ||
| }; |
Contributor
There was a problem hiding this comment.
Surface name validation when validateAndSave blocks a save.
validateAndSave uses isNameValid (no state updates), so a name-invalid edit can block saves while the status still appears “saved.” Consider using validateName here so the UI reflects the error.
🔧 Suggested fix
- const isProfileNameValid = isNameValid(updatedProfile);
+ const isProfileNameValid = validateName(updatedProfile);📝 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.
Suggested change
| // Helper to check if a profile can be saved (all validation passes) | |
| const canSaveProfile = (profileToCheck: AnalysisProfile, targets: string[]): boolean => { | |
| const hasValidName = isNameValid(profileToCheck); | |
| const hasTargets = targets.length > 0; | |
| const hasRules = | |
| profileToCheck.useDefaultRules || (profileToCheck.customRules?.length ?? 0) > 0; | |
| return hasValidName && hasTargets && hasRules; | |
| }; | |
| // Validates and optionally saves if valid | |
| const validateAndSave = (updatedProfile: AnalysisProfile, targets: string[]) => { | |
| const isProfileNameValid = isNameValid(updatedProfile); | |
| const isTargetsValid = validateTargets(targets); | |
| const isRulesValid = validateRules(updatedProfile); | |
| if (isProfileNameValid && isTargetsValid && isRulesValid) { | |
| debouncedChange(updatedProfile); | |
| } | |
| }; | |
| const updateLabelSelector = (sources: string[], targets: string[]) => { | |
| const selector = buildLabelSelector(sources, targets); | |
| const updatedProfile = { ...localProfile, labelSelector: selector }; | |
| setLocalProfile(updatedProfile); | |
| validateAndSave(updatedProfile, targets); | |
| }; | |
| // Validates and optionally saves if valid | |
| const validateAndSave = (updatedProfile: AnalysisProfile, targets: string[]) => { | |
| const isProfileNameValid = validateName(updatedProfile); | |
| const isTargetsValid = validateTargets(targets); | |
| const isRulesValid = validateRules(updatedProfile); | |
| if (isProfileNameValid && isTargetsValid && isRulesValid) { | |
| debouncedChange(updatedProfile); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@webview-ui/src/components/ProfileManager/ProfileEditorForm.tsx` around lines
205 - 230, validateAndSave currently calls isNameValid (which only checks
validity) so a bad name blocks saving without updating validation state; change
validateAndSave to call validateName(updatedProfile) and use its result to set
validation state before deciding to save. Specifically, inside validateAndSave
replace the isProfileNameValid = isNameValid(...) call with a call to
validateName(updatedProfile) (or invoke whatever validateName
returns/side-effects to update UI state), then proceed to check isTargetsValid
and isRulesValid and call debouncedChange(updatedProfile) only if all are true;
updateLabelSelector remains the same but will now trigger validateAndSave which
will surface name errors via validateName.
djzager
approved these changes
Feb 6, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Screen.Recording.2026-02-02.at.3.38.17.PM.mov
Resolves #1228
Summary by CodeRabbit
New Features
Improvements