✨ Manage Profiles page UX improvements#1223
✨ Manage Profiles page UX improvements#1223ibolton336 wants to merge 5 commits intokonveyor: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. |
ibolton336
changed the title
ibolton336
force-pushed
the
profiles-form-empty-state
branch
from
b98d364 to
88be395
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors the Profile Manager UI across multiple files. ProfileEditorForm: consolidates status UI into a HelperText region, removes Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@webview-ui/src/components/ProfileManager/ProfileList.tsx`:
- Around line 128-138: The Delete DropdownItem currently only disables based on
profile.readOnly; update its isDisabled prop to also respect the component’s
isDisabled analysis flag (same as Make Active and Duplicate) so deletion is
blocked during analysis — locate the DropdownItem that calls
setProfileToDelete(profile) and setIsOpen(false) and change its isDisabled check
to combine profile.readOnly with the existing isDisabled flag (e.g., isDisabled
|| profile.readOnly) so it mirrors the other actions.
In `@webview-ui/src/components/ProfileManager/ProfileManagerPage.tsx`:
- Around line 246-248: The JSX block rendering ProfileEditorForm has extra
two-space indentation causing lint errors; locate the conditional rendering
around selectedProfile in ProfileManagerPage (the ternary/conditional that
returns <ProfileEditorForm ... />) and remove the extra leading spaces so the
opening tag for ProfileEditorForm and its props (e.g., allProfiles={profiles})
align with the surrounding JSX indentation style used in ProfileManagerPage.tsx;
ensure the closing tag and any nested props follow the same indentation to
satisfy the linter.
🧹 Nitpick comments (3)
webview-ui/src/index.css (1)
238-244: Consider using a CSS variable for theme consistency.The hardcoded
rgba(0, 0, 0, 0.25)may not work well across all VS Code themes (particularly light themes where a darker border might be more appropriate). Given that the rest of this file uses CSS variable mappings for theming, consider defining a variable or using an existing border color variable.That said, if this value was tested across both light and dark VS Code themes and works acceptably, this is fine to keep as-is for now.
webview-ui/src/components/ProfileManager/ProfileManagerPage.tsx (1)
222-222: Magic number for masthead height.The
calc(100vh - 56px)assumes the masthead is exactly 56px tall. If PatternFly's masthead height changes in future versions, this could cause layout issues.Consider either:
- Adding a comment explaining the 56px assumption
- Using a CSS variable if PatternFly provides one for masthead height
This is low-risk for now but worth noting.
webview-ui/src/components/ProfileManager/ProfileEditorForm.tsx (1)
38-55: UnusedisPendingstate inuseDebouncedCallback.The hook still tracks
isPending(lines 40, 44, 50, 53), but since line 81 only destructurescallback, this state is computed but never read. This is dead code that could be cleaned up.♻️ Simplified hook without isPending
function useDebouncedCallback(callback: (...args: any[]) => void, delay: number) { const timeoutRef = useRef<NodeJS.Timeout | null>(null); - const [isPending, setIsPending] = useState(false); - return { - callback: (...args: any[]) => { - setIsPending(true); - if (timeoutRef.current) { - clearTimeout(timeoutRef.current); - } - timeoutRef.current = setTimeout(() => { - callback(...args); - setIsPending(false); - }, delay); - }, - isPending, - }; + return (...args: any[]) => { + if (timeoutRef.current) { + clearTimeout(timeoutRef.current); + } + timeoutRef.current = setTimeout(() => { + callback(...args); + }, delay); + }; }Then update usage:
- const { callback: debouncedChange } = useDebouncedCallback(onChange, 300); + const debouncedChange = useDebouncedCallback(onChange, 300);
ibolton336
force-pushed
the
profiles-form-empty-state
branch
2 times, most recently
from
7b8a2d3 to
13383c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@webview-ui/src/components/ProfileManager/ProfileList.tsx`:
- Around line 147-159: The ConfirmDialog usage in ProfileList passes
confirmButtonText="Delete" but ConfirmDialog component does not destructure or
forward confirmButtonText to WarningModal; update the ConfirmDialog component to
destructure confirmButtonText (alongside confirmButtonVariant) and pass it into
the underlying WarningModal (the same place confirmButtonVariant is forwarded),
so the "Delete" label from ProfileList is actually used; check ConfirmDialog's
parameter list and its call to WarningModal to add this prop.
🧹 Nitpick comments (3)
webview-ui/src/components/ProfileManager/ProfileManagerPage.tsx (2)
201-213: Auto-saved indicator is static.The "Auto-saved" status always displays regardless of actual save state. The
useDebouncedCallbackhook inProfileEditorForm.tsxexposesisPendingwhich could indicate when a save is in progress. Consider lifting this state up or using a context to show "Saving..." during debounce and "Auto-saved" after completion for better user feedback.
222-222: Consider using CSS variable or calc for masthead height.The hardcoded
56pxfor masthead height is fragile. If the masthead styling changes, this will need manual adjustment. Consider using a CSS variable or measuring the actual masthead height dynamically.webview-ui/src/components/ProfileManager/ProfileEditorForm.tsx (1)
38-55:isPendingis defined but never used.The
useDebouncedCallbackhook (lines 40, 53) exposesisPendingstate, but line 81 only destructurescallback. This creates unused state updates. Either useisPendingto show a saving indicator (which would address the static "Auto-saved" feedback in the masthead), or remove it from the hook to avoid unnecessary re-renders.Option A: Remove unused state from hook
function useDebouncedCallback(callback: (...args: any[]) => void, delay: number) { const timeoutRef = useRef<NodeJS.Timeout | null>(null); - const [isPending, setIsPending] = useState(false); - return { - callback: (...args: any[]) => { - setIsPending(true); + return (...args: any[]) => { if (timeoutRef.current) { clearTimeout(timeoutRef.current); } timeoutRef.current = setTimeout(() => { callback(...args); - setIsPending(false); }, delay); - }, - isPending, }; }Then update line 81:
- const { callback: debouncedChange } = useDebouncedCallback(onChange, 300); + const debouncedChange = useDebouncedCallback(onChange, 300);Also applies to: 81-81
ibolton336
force-pushed
the
profiles-form-empty-state
branch
from
dd948cf to
cc4e731
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
webview-ui/src/components/ProfileManager/ProfileList.tsx (2)
147-159:⚠️ Potential issue | 🟡 Minor
confirmButtonText="Delete"is passed but not consumed byConfirmDialog.Per the relevant code snippet of
ConfirmDialog.tsx, theconfirmButtonTextprop is declared in the interface but never destructured or forwarded toWarningModal. The "Delete" label has no effect here. Consider fixingConfirmDialogto actually use this prop, or remove the prop from the call site to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/components/ProfileManager/ProfileList.tsx` around lines 147 - 159, ConfirmDialog is receiving confirmButtonText but not forwarding it to the modal; update the ConfirmDialog component to destructure the confirmButtonText prop (alongside existing props) and pass it through to the inner WarningModal (or whatever child renders the confirm button) so the label is used, or alternatively remove the confirmButtonText prop from callers like the ProfileList usage; ensure you reference ConfirmDialog's prop interface and the child component (WarningModal) when making the change.
128-138:⚠️ Potential issue | 🟡 MinorDelete dropdown item should also respect the
isDisabledprop.Make Active (line 113) and Duplicate (line 124) both check
isDisabled(which reflects analysis-in-progress state), but the Delete action only checksprofile.readOnly. This allows users to initiate deletion during active analysis.🛡️ Proposed fix
<DropdownItem key="delete" onClick={() => { setProfileToDelete(profile); setIsOpen(false); }} - isDisabled={profile.readOnly} + isDisabled={profile.readOnly || isDisabled} isDanger > Delete </DropdownItem>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/components/ProfileManager/ProfileList.tsx` around lines 128 - 138, The Delete DropdownItem only checks profile.readOnly and can be clicked during analysis; update the Delete item's isDisabled to mirror the other actions by combining the analysis-in-progress disable flag with the readOnly check (i.e., use the same isDisabled prop/variable used by Active and Duplicate so the Delete button is disabled when analysis is in progress or profile.readOnly is true), leaving the onClick handler (setProfileToDelete, setIsOpen) unchanged.
🧹 Nitpick comments (1)
webview-ui/src/components/ProfileManager/ProfileEditorForm.tsx (1)
38-55:isPendingstate inuseDebouncedCallbackcauses unnecessary re-renders.Line 81 destructures only
callback, but the hook still callssetIsPending(true/false)on every debounce cycle (lines 44, 50). These state updates trigger re-renders of the form component with no consumer for the value.Either remove the
isPendingstate entirely or gate it behind a flag so the hook doesn't trigger needless renders.♻️ Proposed fix — remove unused isPending
function useDebouncedCallback(callback: (...args: any[]) => void, delay: number) { const timeoutRef = useRef<NodeJS.Timeout | null>(null); - const [isPending, setIsPending] = useState(false); return { callback: (...args: any[]) => { - setIsPending(true); if (timeoutRef.current) { clearTimeout(timeoutRef.current); } timeoutRef.current = setTimeout(() => { callback(...args); - setIsPending(false); }, delay); }, - isPending, }; }And at the call site:
- const { callback: debouncedChange } = useDebouncedCallback(onChange, 300); + const debouncedChange = useDebouncedCallback(onChange, 300);(Adjust the return type to return the callback directly if
isPendingis no longer needed.)Also applies to: 81-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/components/ProfileManager/ProfileEditorForm.tsx` around lines 38 - 55, The hook useDebouncedCallback currently keeps an isPending state and calls setIsPending(true/false) on each debounce which causes unnecessary re-renders; remove the isPending state and all setIsPending calls, change the hook to return the debounced callback directly (instead of an object with callback and isPending) and update call sites that destructure { callback } to accept the returned function (or rename as needed); also adjust the hook's return type accordingly so no unused state is maintained.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webview-ui/src/components/ProfileManager/ProfileManagerPage.tsx`:
- Around line 201-213: The "Auto-saved" success indicator in ProfileManagerPage
is static and misleading when ProfileEditorForm has validation errors; update
the UI to reflect real saveability by either (A) hiding or disabling the green
CheckCircleIcon and "Auto-saved" span when the form reports validation errors
(lift or pass a hasValidationErrors boolean from ProfileEditorForm into
ProfileManagerPage and conditionally render the Flex block), or (B) change the
text to "Auto-save enabled" and keep it always visible to indicate mode rather
than state; modify the ProfileManagerPage JSX surrounding the
CheckCircleIcon/"Auto-saved" span and adjust ProfileEditorForm props/state to
expose hasValidationErrors accordingly.
- Around line 224-232: The left sidebar in ProfileManagerPage currently uses
overflow: "hidden" on the sidebar div which clips long profile lists; update the
sidebar container (the div in ProfileManagerPage that sets width: "220px",
flexShrink: 0, borderRight, padding) to allow scrolling by replacing overflow:
"hidden" with a scrollable behavior (e.g., overflowY: "auto" or overflow:
"auto") and ensure its height is constrained (e.g., maxHeight: "100vh" or
height: "100%") so the scrollbar appears for long lists without breaking layout.
---
Duplicate comments:
In `@webview-ui/src/components/ProfileManager/ProfileList.tsx`:
- Around line 147-159: ConfirmDialog is receiving confirmButtonText but not
forwarding it to the modal; update the ConfirmDialog component to destructure
the confirmButtonText prop (alongside existing props) and pass it through to the
inner WarningModal (or whatever child renders the confirm button) so the label
is used, or alternatively remove the confirmButtonText prop from callers like
the ProfileList usage; ensure you reference ConfirmDialog's prop interface and
the child component (WarningModal) when making the change.
- Around line 128-138: The Delete DropdownItem only checks profile.readOnly and
can be clicked during analysis; update the Delete item's isDisabled to mirror
the other actions by combining the analysis-in-progress disable flag with the
readOnly check (i.e., use the same isDisabled prop/variable used by Active and
Duplicate so the Delete button is disabled when analysis is in progress or
profile.readOnly is true), leaving the onClick handler (setProfileToDelete,
setIsOpen) unchanged.
---
Nitpick comments:
In `@webview-ui/src/components/ProfileManager/ProfileEditorForm.tsx`:
- Around line 38-55: The hook useDebouncedCallback currently keeps an isPending
state and calls setIsPending(true/false) on each debounce which causes
unnecessary re-renders; remove the isPending state and all setIsPending calls,
change the hook to return the debounced callback directly (instead of an object
with callback and isPending) and update call sites that destructure { callback }
to accept the returned function (or rename as needed); also adjust the hook's
return type accordingly so no unused state is maintained.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/e2e/pages/vscode.page.tstests/e2e/tests/base/profile-management.test.tswebview-ui/src/components/ProfileManager/ProfileEditorForm.tsxwebview-ui/src/components/ProfileManager/ProfileList.tsxwebview-ui/src/components/ProfileManager/ProfileManagerPage.tsxwebview-ui/src/index.css
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/pages/vscode.page.ts
- webview-ui/src/index.css
| <Flex alignItems={{ default: "alignItemsCenter" }} spaceItems={{ default: "spaceItemsXs" }}> | ||
| <FlexItem> | ||
| <CheckCircleIcon | ||
| color="var(--pf-t--global--icon--color--status--success--default)" | ||
| style={{ fontSize: "0.75rem" }} | ||
| /> | ||
| </FlexItem> | ||
| <FlexItem> | ||
| <span style={{ fontSize: "0.75rem", color: "var(--pf-t--global--text--color--subtle)" }}> | ||
| Auto-saved | ||
| </span> | ||
| </FlexItem> | ||
| </Flex> |
There was a problem hiding this comment.
"Auto-saved" indicator is always shown, even when the form has unsaved validation errors.
This static indicator doesn't reflect actual save state. When the ProfileEditorForm has validation errors, changes are intentionally not saved (the debounced callback is gated by validation). Showing a green "Auto-saved" checkmark in that scenario is misleading.
Consider either hiding this indicator when validation errors exist (you'd need to lift hasValidationErrors or pass save status up), or changing the label to something like "Auto-save enabled" to convey the mode rather than the current state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webview-ui/src/components/ProfileManager/ProfileManagerPage.tsx` around lines
201 - 213, The "Auto-saved" success indicator in ProfileManagerPage is static
and misleading when ProfileEditorForm has validation errors; update the UI to
reflect real saveability by either (A) hiding or disabling the green
CheckCircleIcon and "Auto-saved" span when the form reports validation errors
(lift or pass a hasValidationErrors boolean from ProfileEditorForm into
ProfileManagerPage and conditionally render the Flex block), or (B) change the
text to "Auto-save enabled" and keep it always visible to indicate mode rather
than state; modify the ProfileManagerPage JSX surrounding the
CheckCircleIcon/"Auto-saved" span and adjust ProfileEditorForm props/state to
expose hasValidationErrors accordingly.
| <div | ||
| style={{ | ||
| width: "220px", | ||
| flexShrink: 0, | ||
| borderRight: "1px solid var(--pf-t--global--border--color--default)", | ||
| padding: "1rem", | ||
| overflow: "hidden", | ||
| }} | ||
| > |
There was a problem hiding this comment.
Left sidebar clips long profile lists — needs scroll.
overflow: "hidden" (line 230) will silently clip profiles that extend beyond the viewport. With no scrollbar, users cannot reach profiles at the bottom of a long list.
🐛 Proposed fix
<div
style={{
width: "220px",
flexShrink: 0,
borderRight: "1px solid var(--pf-t--global--border--color--default)",
padding: "1rem",
- overflow: "hidden",
+ overflowY: "auto",
}}
>📝 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.
| <div | |
| style={{ | |
| width: "220px", | |
| flexShrink: 0, | |
| borderRight: "1px solid var(--pf-t--global--border--color--default)", | |
| padding: "1rem", | |
| overflow: "hidden", | |
| }} | |
| > | |
| <div | |
| style={{ | |
| width: "220px", | |
| flexShrink: 0, | |
| borderRight: "1px solid var(--pf-t--global--border--color--default)", | |
| padding: "1rem", | |
| overflowY: "auto", | |
| }} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webview-ui/src/components/ProfileManager/ProfileManagerPage.tsx` around lines
224 - 232, The left sidebar in ProfileManagerPage currently uses overflow:
"hidden" on the sidebar div which clips long profile lists; update the sidebar
container (the div in ProfileManagerPage that sets width: "220px", flexShrink:
0, borderRight, padding) to allow scrolling by replacing overflow: "hidden" with
a scrollable behavior (e.g., overflowY: "auto" or overflow: "auto") and ensure
its height is constrained (e.g., maxHeight: "100vh" or height: "100%") so the
scrollbar appears for long lists without breaking layout.
fdf4242 to
4dbe52c
Compare
|
@coderabbitai create a task for me to implement an e2e test for importing the profiles from the manage profiles page |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 57
Here's a summary of what was captured:
|
ibolton336
force-pushed
the
profiles-form-empty-state
branch
2 times, most recently
from
fdcf584 to
c99340d
Compare
- Add masthead with split button for New Profile/Import actions - Fixed split-panel layout with independent scrolling - Replace empty state with proper PatternFly EmptyState component - Switch profile list from DataList to SimpleList - Replace active label with compact star icon - Fix profile name truncation and kebab menu wrapping - Move profile info and auto-save status to masthead - Fix split button border color in theme - Change custom rules labels to outline variant Signed-off-by: Ian Bolton <ibolton@redhat.com> Signed-off-by: ibolton336 <ibolton@redhat.com>
Signed-off-by: Ian Bolton <ibolton@redhat.com> Signed-off-by: ibolton336 <ibolton@redhat.com>
Signed-off-by: Ian Bolton <ibolton@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: ibolton336 <ibolton@redhat.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: ibolton336 <ibolton@redhat.com>
ibolton336
force-pushed
the
profiles-form-empty-state
branch
from
c99340d to
8bb715b
Compare
2 participants
Screen.Recording.2026-01-28.at.5.23.04.PM.mov
Improve Profile Manager UI/UX
Summary
Redesigns the Profile Manager screen with improved layout, visual hierarchy, and user experience. The changes address multiple UX issues including the sparse empty state, cramped form layout, and various styling inconsistencies.
Changes
Layout & Structure
EmptyStatecomponent featuring icon, descriptive text, and action buttonsProfile List (Left Panel)
DataListtoSimpleListfor cleaner, less bordered appearanceappendTo: "inline"to dropdown for proper positioningProfile Editor Form (Right Panel)
HelperTextitems for validation errors and disabled warningsStyling Fixes
.pf-v6-c-menu-toggle.pf-m-split-button.pf-m-primaryborder colorFiles Changed
webview-ui/src/components/ProfileManager/ProfileManagerPage.tsxwebview-ui/src/components/ProfileManager/ProfileList.tsxwebview-ui/src/components/ProfileManager/ProfileEditorForm.tsxwebview-ui/src/index.cssSummary by CodeRabbit
UI Improvements
Tests