OU-1465: Integrate new proposal detail view#88
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jgbernalp: This pull request references OU-1465 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (39)
📝 WalkthroughWalkthroughReworks the proposal detail page from a tabbed layout to a single-page, phase-driven view backed by a new ChangesProposal Detail Rework
Also removed: Sequence Diagram(s)sequenceDiagram
participant ProposalDetailsPage
participant StatusGuard
participant useProposal
participant K8sAPI
participant RemediationOptionCard
ProposalDetailsPage->>useProposal: useProposal(name, namespace)
useProposal->>K8sAPI: watch Proposal/Analysis/Execution/Verification/Approval
K8sAPI-->>useProposal: resource updates
useProposal-->>ProposalDetailsPage: view, loaded, error, approveExecution
ProposalDetailsPage->>StatusGuard: render(data=view, loaded, loadError)
StatusGuard-->>ProposalDetailsPage: children or error/loading state
ProposalDetailsPage->>RemediationOptionCard: render option, onExecute
RemediationOptionCard->>ProposalDetailsPage: onExecute(index)
ProposalDetailsPage->>useProposal: approveExecution(index)
useProposal->>K8sAPI: k8sPatch approval resource
K8sAPI-->>useProposal: patch result
useProposal-->>ProposalDetailsPage: mutationInProgress/mutationError
Compact metadata
Suggested labels: enhancement, refactor, needs-review Suggested reviewers: proposal-detail-page maintainers, hooks/data-layer owners 🐰 Tabs are gone, phases now reign, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/hooks/useProposal.test.ts (1)
194-256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOutcome branches of
mapExecutionare untested.Both tests use condition
type: 'Executed', socompletedCond(which looks fortype === 'Completed') never matches, and every test here falls into the'Unknown'fallback. Thereason === 'Succeeded'andreason === 'Failed'branches inmapExecution's outcome logic have no coverage.✅ Suggested fix to actually exercise the Succeeded branch
conditions: [ { - type: 'Executed', + type: 'Completed', status: 'True', - reason: 'Success', + reason: 'Succeeded', message: 'Actions completed', lastTransitionTime: '2025-01-01T00:00:00Z', }, ], actionsTaken: [{ type: 'patch', description: 'Patched deployment', outcome: 'Succeeded' }], }, }; const result = mapExecution(options, execution); expect(result).toBeDefined(); expect(result!.originalRootCause).toBe('Memory limit too low'); - expect(result!.outcome).toBe('Unknown'); + expect(result!.outcome).toBe('Succeeded');Also consider adding a case with
reason: 'Failed'to cover theFailedbranch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useProposal.test.ts` around lines 194 - 256, The `mapExecution` tests are only exercising the `'Unknown'` fallback because the sample status condition uses `type: 'Executed'` instead of the `Completed` condition that `mapExecution` checks. Update the `mapExecution` test cases in `useProposal.test.ts` so one fixture hits the `reason === 'Succeeded'` branch and add another for `reason === 'Failed'`, using the existing `mapExecution` helper and `ExecutionResultK8s` fixtures to verify the outcome logic directly.src/hooks/useExecutionLogActions.ts (1)
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused
abortRef.
abortRef.currentis assigned but never read; cleanup already uses the localacdirectly. Consider removing the ref.Also applies to: 70-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useExecutionLogActions.ts` at line 20, The useExecutionLogActions hook has an unused abortRef that is assigned but never read, since the cleanup already uses the local AbortController instance directly. Remove the abortRef declaration and any related assignments/usages in useExecutionLogActions so the hook relies only on the local ac flow and keeps the abort logic consistent.src/components/proposals/detail/RemediationOptionCard.tsx (1)
124-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDynamic translation key bypasses static i18n extraction.
When
reversibility !== 'Partial', the rawoption.reversibilityvalue (e.g.'Full','None') is used directly as the translation key. Automated i18n extraction tools typically can't statically discover dynamic keys, risking missing entries in the locale files for those values.♻️ Proposed fix: use static literal keys
- {t( - option.reversibility === 'Partial' - ? 'Partially reversible' - : option.reversibility, - )} + {option.reversibility === 'Partial' + ? t('Partially reversible') + : option.reversibility === 'Full' + ? t('Fully reversible') + : t('Irreversible')}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/proposals/detail/RemediationOptionCard.tsx` around lines 124 - 129, The reversibility label in RemediationOptionCard uses a dynamic translation key via option.reversibility, which can bypass static i18n extraction. Update the conditional around t(...) so it always uses explicit literal keys for each supported value (for example in the RemediationOptionCard render path), rather than passing the raw option.reversibility string directly. Keep the existing “Partial” branch, and map each remaining reversibility case to a fixed translation key that extraction tools can detect.src/components/proposals/detail/AnalysisSummary.tsx (1)
89-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract confidence-color logic to match established utility pattern.
Confidence color is computed inline here with a nested ternary, while
RemediationOptionCarduses sharedgetRiskColor/getReversibilityColorutilities frommodels/proposal/utils/proposal-utilsfor the same kind of concern. Consider adding agetConfidenceColorhelper alongside those for consistency and to avoid re-implementing color-mapping logic per component.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/proposals/detail/AnalysisSummary.tsx` around lines 89 - 104, The confidence color mapping in AnalysisSummary is implemented inline with nested ternaries; extract it into a shared getConfidenceColor helper to match the existing utility pattern used by RemediationOptionCard with getRiskColor and getReversibilityColor. Add the helper alongside the other proposal color utilities in models/proposal or utils/proposal-utils, then update AnalysisSummary to call it when rendering rootCause.confidence so the color logic is centralized and reusable.src/components/proposals/detail/VerificationSummary.tsx (1)
43-53: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCheckmark icon used for "in progress" state is misleading.
The neutral/"in progress" branch reuses
CheckCircleIcon(just recolored), which visually reads as a pass even though verification hasn't completed. Consider a distinct icon (e.g., pending/clock/in-progress icon) to avoid confusing a not-yet-verified state with a passed one.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/proposals/detail/VerificationSummary.tsx` around lines 43 - 53, The neutral “in progress” branch in VerificationSummary is reusing CheckCircleIcon, which makes an unfinished verification look like a pass. Update the icon used in the allPassed ? ... : ... conditional inside VerificationSummary to a distinct in-progress/pending icon, and keep the aria-label and color aligned with the pending state so the visual meaning is clear.src/components/proposals/detail/SandboxLogViewer.tsx (1)
36-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPositional args to
useSandboxLogStreamare fragile.Five positional params, three of which are booleans/strings of the same type — easy to transpose (
isExpanded/streaming/auditOnly) during future edits without a type error.♻️ Suggested options-object signature
- const { lines, loading, error } = useSandboxLogStream( - sandbox, - isExpanded, - streaming, - sinceTime, - auditOnly, - ); + const { lines, loading, error } = useSandboxLogStream(sandbox, { + isExpanded, + streaming, + sinceTime, + auditOnly, + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/proposals/detail/SandboxLogViewer.tsx` around lines 36 - 42, The current useSandboxLogStream call in SandboxLogViewer passes five positional arguments, making the boolean/string parameters easy to mix up during future edits. Update the hook call site and the useSandboxLogStream signature to use a single options object instead of positional params, and pass the existing values through named fields so the intent is explicit and order-independent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Around line 29-33: The dependency range for `@patternfly/react-log-viewer`
points to a version that is not published, which will break installs. Update the
package.json entry for `@patternfly/react-log-viewer` to a valid released version
consistent with the other PatternFly packages, and keep the change localized to
the dependency list where `@patternfly/react-component-groups` and the other
`@patternfly/`* packages are declared.
In `@src/components/proposals/ProposalDetailPage.tsx`:
- Around line 66-72: The `useEffect` in `ProposalDetailPage` only suppresses
`react-hooks/set-state-in-effect` for `setSelectedOption`, so
`setExpandedOption` still triggers the lint rule. Update the effect so the
suppression also covers `setExpandedOption` in the same `useEffect` block, using
the `setSelectedOption` and `setExpandedOption` calls as the reference points.
---
Nitpick comments:
In `@src/components/proposals/detail/AnalysisSummary.tsx`:
- Around line 89-104: The confidence color mapping in AnalysisSummary is
implemented inline with nested ternaries; extract it into a shared
getConfidenceColor helper to match the existing utility pattern used by
RemediationOptionCard with getRiskColor and getReversibilityColor. Add the
helper alongside the other proposal color utilities in models/proposal or
utils/proposal-utils, then update AnalysisSummary to call it when rendering
rootCause.confidence so the color logic is centralized and reusable.
In `@src/components/proposals/detail/RemediationOptionCard.tsx`:
- Around line 124-129: The reversibility label in RemediationOptionCard uses a
dynamic translation key via option.reversibility, which can bypass static i18n
extraction. Update the conditional around t(...) so it always uses explicit
literal keys for each supported value (for example in the RemediationOptionCard
render path), rather than passing the raw option.reversibility string directly.
Keep the existing “Partial” branch, and map each remaining reversibility case to
a fixed translation key that extraction tools can detect.
In `@src/components/proposals/detail/SandboxLogViewer.tsx`:
- Around line 36-42: The current useSandboxLogStream call in SandboxLogViewer
passes five positional arguments, making the boolean/string parameters easy to
mix up during future edits. Update the hook call site and the
useSandboxLogStream signature to use a single options object instead of
positional params, and pass the existing values through named fields so the
intent is explicit and order-independent.
In `@src/components/proposals/detail/VerificationSummary.tsx`:
- Around line 43-53: The neutral “in progress” branch in VerificationSummary is
reusing CheckCircleIcon, which makes an unfinished verification look like a
pass. Update the icon used in the allPassed ? ... : ... conditional inside
VerificationSummary to a distinct in-progress/pending icon, and keep the
aria-label and color aligned with the pending state so the visual meaning is
clear.
In `@src/hooks/useExecutionLogActions.ts`:
- Line 20: The useExecutionLogActions hook has an unused abortRef that is
assigned but never read, since the cleanup already uses the local
AbortController instance directly. Remove the abortRef declaration and any
related assignments/usages in useExecutionLogActions so the hook relies only on
the local ac flow and keeps the abort logic consistent.
In `@src/hooks/useProposal.test.ts`:
- Around line 194-256: The `mapExecution` tests are only exercising the
`'Unknown'` fallback because the sample status condition uses `type: 'Executed'`
instead of the `Completed` condition that `mapExecution` checks. Update the
`mapExecution` test cases in `useProposal.test.ts` so one fixture hits the
`reason === 'Succeeded'` branch and add another for `reason === 'Failed'`, using
the existing `mapExecution` helper and `ExecutionResultK8s` fixtures to verify
the outcome logic directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8ce39a0f-5955-47ce-b8cb-c3e2ee2cabcf
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (39)
.ai/spec/how/k8s-data-layer.md.ai/spec/how/project-structure.md.ai/spec/what/run-lifecycle.mdlocales/en/plugin__lightspeed-agentic-console-plugin.jsonpackage.jsonsrc/__mocks__/dynamic-plugin-sdk.tssrc/components/ConfirmationModal.tsxsrc/components/StatusGuard.tsxsrc/components/configuration/AgentForm.tsxsrc/components/configuration/ApprovalPolicyTab.tsxsrc/components/proposals/DynamicComponent.tsxsrc/components/proposals/EscalateModal.tsxsrc/components/proposals/MarkdownText.tsxsrc/components/proposals/PhaseIcon.tsxsrc/components/proposals/ProposalDetailPage.tsxsrc/components/proposals/ProposalListPage.tsxsrc/components/proposals/SandboxLogViewer.tsxsrc/components/proposals/detail/AnalysisSummary.tsxsrc/components/proposals/detail/ExecutionSummary.tsxsrc/components/proposals/detail/ProposalPhaseLabel.tsxsrc/components/proposals/detail/ProposalTimeline.tsxsrc/components/proposals/detail/RemediationOptionCard.tsxsrc/components/proposals/detail/SandboxLogViewer.tsxsrc/components/proposals/detail/StageInProgress.tsxsrc/components/proposals/detail/VerificationSummary.tsxsrc/components/proposals/detail/detail.csssrc/components/proposals/proposal-detail.csssrc/components/proposals/sandbox-log-viewer.csssrc/constants.tssrc/hooks/useExecutionLogActions.tssrc/hooks/useProposal.test.tssrc/hooks/useProposal.tssrc/hooks/useSandboxLogStream.tssrc/models/proposal-views.tssrc/models/proposal.tssrc/test-helpers.tssrc/utils/approval.tssrc/utils/proposal-utils.tsstart-console.sh
💤 Files with no reviewable changes (7)
- src/components/proposals/MarkdownText.tsx
- src/components/proposals/sandbox-log-viewer.css
- src/components/proposals/SandboxLogViewer.tsx
- src/components/proposals/EscalateModal.tsx
- src/components/proposals/proposal-detail.css
- src/components/proposals/DynamicComponent.tsx
- src/components/proposals/PhaseIcon.tsx
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
jgbernalp
force-pushed
the
integrate-new-proposal-detail-view
branch
from
2f02e5f to
d401125
Compare
2 participants
This PR:
proposal-execute.mov
proposal-failed.mov