ACM-32879-Appset-Topology-Topology-Alert-window-5#6493
Open
jeswanke wants to merge 14 commits into
Open
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jeswanke The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
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:
📝 WalkthroughWalkthroughThe PR adds topology alert analysis and action modals, refactors diagram construction for asynchronous alerts, introduces custom topology shapes, extends SyncEditor controls, and enables measured modal wizard layouts with callback-driven ApplicationSet editing. ChangesApplication topology
Sync editor controls
Wizard modal editing
Estimated code review effort: 5 (Critical) | ~120 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/routes/Applications/CreateArgoApplication/CreatePullApplicationSet.test.tsx (1)
542-562: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd accessibility assertions to both changed component tests.
frontend/src/routes/Applications/CreateArgoApplication/CreatePullApplicationSet.test.tsx#L542-L562: runaxeagainst the rendered edit page.frontend/src/routes/Applications/CreateArgoApplication/CreatePushApplicationSet.test.tsx#L466-L489: run the same assertion for the push-model edit page.As per coding guidelines, “Every component test must include an accessibility assertion with
jest-axe.”🤖 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 `@frontend/src/routes/Applications/CreateArgoApplication/CreatePullApplicationSet.test.tsx` around lines 542 - 562, Update the edit-page tests in frontend/src/routes/Applications/CreateArgoApplication/CreatePullApplicationSet.test.tsx:542-562 and frontend/src/routes/Applications/CreateArgoApplication/CreatePushApplicationSet.test.tsx:466-489 to run jest-axe against each rendered edit page and assert that no accessibility violations are found. Add the assertions after the existing render/readiness steps, reusing the repository’s established axe testing pattern.Source: Coding guidelines
frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/components/DetailsView.tsx (1)
121-123: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass the unwrapped node into the toolbar actions.
renderToolbarstill receivescurrentNode, so the Logs/Edit YAML callbacks can be invoked with the layout node instead of the underlying resource node thatdetailsNodeunwraps to.🤖 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 `@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/components/DetailsView.tsx` around lines 121 - 123, Update the DetailsView flow around currentUpdatedNode and renderToolbar so toolbar actions receive the unwrapped underlying resource node from detailsNode, rather than the layout node. Ensure the Logs and Edit YAML callbacks use this unwrapped node while preserving the existing node selection and toolbar behavior.
🟡 Minor comments (6)
frontend/src/components/SyncEditor/SyncEditorDiff.tsx-124-124 (1)
124-124: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake
renderSideBySidedynamically reactive.Currently,
applyDiffLayoutFromContaineris invoked on mount and resize, but it is not automatically executed whenrenderSideBySidePropchanges. If a parent component dynamically updates this prop after mounting, the diff layout will not reflect the change until the browser window is resized.Consider adding a
useEffectto trigger layout updates when the callback updates.💡 Proposed fix to ensure reactivity
if (diffEditorRef.current) { diffEditorRef.current.layout({ width, height }) } }, [renderSideBySideProp]) + + useEffect(() => { + applyDiffLayoutFromContainer() + }, [applyDiffLayoutFromContainer])🤖 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 `@frontend/src/components/SyncEditor/SyncEditorDiff.tsx` at line 124, Update the SyncEditor layout effect around applyDiffLayoutFromContainer so it also runs whenever renderSideBySideProp changes, not only on mount or resize. Add the dependency-aware effect using the existing layout callback and preserve the current resize behavior.frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopologyApplications.ts-264-268 (1)
264-268: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the YAML path matching the ApplicationSet source shape.
This action always highlights
spec.template.spec.sources; single-source ApplicationSets usespec.template.spec.source, so the editor cannot focus the relevant field. Select the path fromappSet.specs.raw.🤖 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 `@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopologyApplications.ts` around lines 264 - 268, Update the edit-YAML action in the ApplicationSet topology analysis to derive highlightEditorPath from appSet.specs.raw, using spec.template.spec.source for single-source configurations and spec.template.spec.sources for multi-source configurations. Preserve the existing editYaml action and appSet node while selecting the path that matches the actual ApplicationSet source shape.frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/model/topology.test.ts-408-415 (1)
408-415: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert that
alertsPromiseexists before awaiting it.
await undefinedsucceeds, so this test would not catch removal of asynchronous analysis.Proposed fix
expect(addDiagramDetails).toHaveBeenCalled() expect(computeNodeStatus).toHaveBeenCalled() expect(diagramElements).toBeDefined() + expect(alertsPromise).toBeDefined() await alertsPromiseAs per path instructions,
getDiagramElementsconfirms that non-null statuses must create this promise.🤖 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 `@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/model/topology.test.ts` around lines 408 - 415, Update the test around getDiagramElements to explicitly assert that alertsPromise is defined before awaiting it, while preserving the existing addDiagramDetails, computeNodeStatus, and diagramElements assertions.Source: Path instructions
frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/createSuggestsPlacement.ts-144-146 (1)
144-146: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDump the full Placement specification.
The fallback says “Current specification” but serializes only
placement.spec.clusterSets, hiding predicates and other fields needed to diagnose unknown failures.Proposed fix
- const currentYaml = jsYaml.dump(placement.spec.clusterSets ?? {}, { indent: 2 }).split('\n') + const currentYaml = jsYaml.dump(placement.spec ?? {}, { indent: 2 }).split('\n')🤖 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 `@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/createSuggestsPlacement.ts` around lines 144 - 146, Update the default branch of createSuggestsPlacement to serialize the complete placement.spec object, rather than only placement.spec.clusterSets, so the “Current specification” suggestion includes predicates and all other fields.frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/modals/EditAppSetModal.tsx-14-15 (1)
14-15: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winForward
showWizardInputor remove it from the modal API.EditAppSetModalContentdrops the prop, so any caller-provided wizard target is ignored.🤖 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 `@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/modals/EditAppSetModal.tsx` around lines 14 - 15, Update the EditAppSetModal API so the showWizardInput value is forwarded through to EditAppSetModalContent and used by the modal flow, or remove showWizardInput from the modal props and all callers if it is no longer needed. Ensure caller-provided wizard targets are not silently ignored.frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/CustomEllipse.test.tsx-42-223 (1)
42-223: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMissing accessibility assertions in component tests.
Component tests must include an accessibility assertion using
jest-axe(e.g.,expect(await axe(container)).toHaveNoViolations()). As per coding guidelines, every component test must adhere to this rule.
frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/CustomEllipse.test.tsx#L42-L223: Add ajest-axeassertion to the test suite.frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/StyledNode.test.tsx#L369-L440: Add ajest-axeassertion to the custom shape tests.🤖 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 `@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/CustomEllipse.test.tsx` around lines 42 - 223, Add jest-axe accessibility assertions to the component tests: in frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/CustomEllipse.test.tsx (lines 42-223), update the relevant CustomEllipse test flow to await axe on the rendered container and assert no violations; likewise, in frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/StyledNode.test.tsx (lines 369-440), add the same assertion to the custom shape tests. Ensure both suites import and use the established axe matcher setup.Source: Coding guidelines
🧹 Nitpick comments (3)
frontend/src/wizards/WizardPage.css (1)
23-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftImplement the modal layout through supported PatternFly composition.
The modal sizing currently spans direct
.pf-v6-*overrides and custom HTML wrappers.
frontend/src/wizards/WizardPage.css#L23-L44: remove selectors overriding PatternFly internals.frontend/src/wizards/WizardPage.tsx#L69-L77: replace the custom outer shell with PatternFly layout primitives.frontend/src/routes/Applications/CreateArgoApplication/EditArgoApplicationSet.tsx#L232-L246: remove the duplicate inline-styled modal wrapper.As per coding guidelines, use PatternFly variants and modifiers instead of custom PatternFly CSS and custom HTML.
🤖 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 `@frontend/src/wizards/WizardPage.css` around lines 23 - 44, Replace the custom modal layout with supported PatternFly composition: remove the direct .pf-v6-* overrides in frontend/src/wizards/WizardPage.css at lines 23-44, replace the custom outer shell in frontend/src/wizards/WizardPage.tsx at lines 69-77 with appropriate PatternFly layout primitives and variants, and remove the duplicate inline-styled modal wrapper in frontend/src/routes/Applications/CreateArgoApplication/EditArgoApplicationSet.tsx at lines 232-246.Source: Coding guidelines
frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/types.ts (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the copyright header before all imports.
The required header is currently at Line 18, so this source file does not start with it.
As per coding guidelines, “All source files must start with the copyright header.”
🤖 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 `@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/types.ts` at line 2, Move the existing copyright header in the topology types source file to the very beginning, before the TopologyAlert import and any other content. Preserve the header text unchanged.Source: Coding guidelines
frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/model/topology.ts (1)
28-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
Servicea type-only import.
Serviceis only used inservices: Service[]; importing it as a value violates the repository’s type-only import rule.Proposed fix
-import { Service, type Placement } from '../../../../../resources' +import type { Placement, Service } from '../../../../../resources'As per coding guidelines, “Use
import typefor type-only imports to avoid generating runtime imports.”🤖 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 `@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/model/topology.ts` at line 28, Update the import from the resources module used by the topology model so Service is imported as a type-only symbol, while preserving the existing Placement import usage and services: Service[] typing.Source: Coding guidelines
🤖 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
`@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopologyApplications.ts`:
- Around line 99-103: Update isBadDeployResourceHealth to return true when
either the normalized health status is not Healthy or the normalized sync status
is not Synced, replacing the current && condition. Reuse the existing
status-normalization logic rather than comparing raw values directly.
In
`@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopologyAppSet.ts`:
- Around line 54-75: The placement-reference validation currently runs only when
no topology placement node exists, so missing references are skipped when one
node is present. Update the analysis flow around placement and
collectReferencedPlacements to run reference validation unconditionally,
treating a matching topology placement node or entry in placements as existing
before calling missingPlacementAlert; add a regression test covering mixed
existing and missing references from matrix or merge generators.
In
`@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/createSuggestsApplication.ts`:
- Around line 147-169: Update createSuggestsApplication to derive the displayed
source value and YAML editor path from applicationSet.spec.template.spec,
selecting source for single-source ApplicationSets and sources otherwise. Apply
this consistently across all affected suggestions and edit actions, preserving
the existing behavior for multi-source templates. Add a regression test covering
a single-source ApplicationSet, including its rendered YAML and highlighted
editor path.
In
`@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/utils.ts`:
- Around line 98-106: Update isErrorCondition to evaluate errorKeywords against
both the lowercased condition.type and condition.reason when present, rather
than selecting only one via reasonLower ?? typeLower. Preserve the existing
positive-status check and return true whenever either field contains an error
keyword.
In
`@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/ApplicationTopology.tsx`:
- Around line 192-207: Handle rejection of alertsPromise in the
topology-analysis flow by adding a rejection handler that clears or otherwise
resets the displayed alerts to prevent stale results. Keep the existing
cancellation guard and finally cleanup, including analyzingTimer and isAnalyzing
state updates, unchanged.
- Around line 163-165: Update the topology-loading flow around the topology
availability check so the derived nodes and alerts state is cleared before
returning when topology is unavailable. Preserve the existing early return while
ensuring stale data from the previous application is not retained during
loading.
- Around line 220-221: Update the effect dependency array associated with the
eslint suppression to include topology, statuses, canUpdateStatuses, t, and
placements alongside startup and refreshTime. Remove the exhaustive-deps
suppression once the full dependency list is restored, preserving the existing
alert analysis logic.
In
`@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/modals/EditYamlModal.tsx`:
- Around line 397-420: Update the Save button state in the EditYamlModal
component to also disable when resources is empty or while the resource is
reloading, preventing the onSave handler from dumping resources[0] when
unavailable. Preserve the existing readOnly and editorValidationStatus checks.
In
`@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/modals/LogsModal.tsx`:
- Around line 318-342: Update the logsError handling in LogsModal so it renders
the AcmAlert within the normal log viewer layout instead of returning early.
Preserve the resource URL link and error details, while keeping the pod and
container selectors available so users can switch resources after a failure.
- Around line 220-278: Update the effect’s asynchronous handlers in LogsModal to
ignore results and errors from aborted requests. Guard both the fleetLogsRequest
and fetchRetry success and failure callbacks using their corresponding
abortController.signal.aborted (or an equivalent request-generation check),
including setLogsError and setIsLoadingLogs, while preserving updates for the
active request.
- Around line 189-208: Update the fullscreen listener setup in the useEffect
cleanup to unregister the exact callback references registered with
screenfull.on for both change and error events. Define the handlers once within
the effect, reuse them in screenfull.on and screenfull.off, and preserve the
existing state updates.
In
`@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/StyledNode.tsx`:
- Around line 90-95: In StyledNode, replace the inline CustomShape definition
inside getCustomShape with a useCallback-stabilized custom shape factory
declared before the return, using isMulti and shouldPulse as dependencies. Pass
this stable factory to DefaultNode so the custom shape component identity
remains unchanged between renders.
- Around line 90-95: In StyledNode, replace the inline getCustomShape component
factory with a useCallback-memoized customShapeFactory defined before the
return, using isMulti and shouldPulse as dependencies. Pass this stable factory
to DefaultNode so CustomShape does not remount on every render.
In
`@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/TopologyAlerts.tsx`:
- Around line 173-233: The alert timers created in the sortedInputAlerts effect
and closeAction must be tracked and canceled during cleanup. Add refs for
stagger, fade, and dismissal timeout handles; register each created timer, clear
and remove them when inputs change or the component unmounts, and ensure
canceled timers cannot re-add alerts or update state.
- Around line 119-149: Use currentAlertsKey in TopologyAlerts to scope
dismissedIdsRef: accept it in the component destructuring, clear dismissedIdsRef
and related visible/dismissal state when the key changes, and ensure alerts for
a new application or analysis are eligible to display. Pass the
application/analysis key from ApplicationTopology and add a regression test
verifying a previously dismissed alert reappears after the key changes.
In
`@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/css/Drawer.css`:
- Around line 25-50: Replace the `.pf-v6-c-toolbar__content`,
`.pf-v6-c-toolbar__group`, `.pf-v6-c-divider`, and `.pf-v6-c-button` overrides
with PatternFly toolbar variants/props and a local wrapper for the required
spacing and alignment. Remove selectors targeting PatternFly internals while
preserving the toolbar layout, and add the required empty line before the
`border` declaration in `.topologyDetails .details-view-toolbar`.
In
`@frontend/src/routes/Applications/CreateArgoApplication/EditArgoApplicationSet.tsx`:
- Around line 171-174: Move the missing-ApplicationSet validation before the
JSON.parse(JSON.stringify(applicationSet)) clone in the edit flow. Return via
onApplicationSetNotFound() immediately when applicationSet is undefined, and
preserve the existing cloning and update behavior for valid ApplicationSets.
In `@frontend/src/wizards/WizardPage.tsx`:
- Line 5: Use type-only imports consistently at all affected sites: in
frontend/src/wizards/WizardPage.tsx:5-5, move ReactElement and ReactNode to
import type; in
frontend/src/routes/Applications/CreateArgoApplication/EditApplicationSetPage.tsx:4-4,
import PathParam with import type; and in
frontend/src/routes/Applications/CreateArgoApplication/EditArgoApplicationSet.tsx:15-28,
separate type-only resource and editor symbols from runtime imports.
---
Outside diff comments:
In
`@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/components/DetailsView.tsx`:
- Around line 121-123: Update the DetailsView flow around currentUpdatedNode and
renderToolbar so toolbar actions receive the unwrapped underlying resource node
from detailsNode, rather than the layout node. Ensure the Logs and Edit YAML
callbacks use this unwrapped node while preserving the existing node selection
and toolbar behavior.
In
`@frontend/src/routes/Applications/CreateArgoApplication/CreatePullApplicationSet.test.tsx`:
- Around line 542-562: Update the edit-page tests in
frontend/src/routes/Applications/CreateArgoApplication/CreatePullApplicationSet.test.tsx:542-562
and
frontend/src/routes/Applications/CreateArgoApplication/CreatePushApplicationSet.test.tsx:466-489
to run jest-axe against each rendered edit page and assert that no accessibility
violations are found. Add the assertions after the existing render/readiness
steps, reusing the repository’s established axe testing pattern.
---
Minor comments:
In `@frontend/src/components/SyncEditor/SyncEditorDiff.tsx`:
- Line 124: Update the SyncEditor layout effect around
applyDiffLayoutFromContainer so it also runs whenever renderSideBySideProp
changes, not only on mount or resize. Add the dependency-aware effect using the
existing layout callback and preserve the current resize behavior.
In
`@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopologyApplications.ts`:
- Around line 264-268: Update the edit-YAML action in the ApplicationSet
topology analysis to derive highlightEditorPath from appSet.specs.raw, using
spec.template.spec.source for single-source configurations and
spec.template.spec.sources for multi-source configurations. Preserve the
existing editYaml action and appSet node while selecting the path that matches
the actual ApplicationSet source shape.
In
`@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/createSuggestsPlacement.ts`:
- Around line 144-146: Update the default branch of createSuggestsPlacement to
serialize the complete placement.spec object, rather than only
placement.spec.clusterSets, so the “Current specification” suggestion includes
predicates and all other fields.
In
`@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/modals/EditAppSetModal.tsx`:
- Around line 14-15: Update the EditAppSetModal API so the showWizardInput value
is forwarded through to EditAppSetModalContent and used by the modal flow, or
remove showWizardInput from the modal props and all callers if it is no longer
needed. Ensure caller-provided wizard targets are not silently ignored.
In
`@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/model/topology.test.ts`:
- Around line 408-415: Update the test around getDiagramElements to explicitly
assert that alertsPromise is defined before awaiting it, while preserving the
existing addDiagramDetails, computeNodeStatus, and diagramElements assertions.
In
`@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/CustomEllipse.test.tsx`:
- Around line 42-223: Add jest-axe accessibility assertions to the component
tests: in
frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/CustomEllipse.test.tsx
(lines 42-223), update the relevant CustomEllipse test flow to await axe on the
rendered container and assert no violations; likewise, in
frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/StyledNode.test.tsx
(lines 369-440), add the same assertion to the custom shape tests. Ensure both
suites import and use the established axe matcher setup.
---
Nitpick comments:
In
`@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/model/topology.ts`:
- Line 28: Update the import from the resources module used by the topology
model so Service is imported as a type-only symbol, while preserving the
existing Placement import usage and services: Service[] typing.
In
`@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/types.ts`:
- Line 2: Move the existing copyright header in the topology types source file
to the very beginning, before the TopologyAlert import and any other content.
Preserve the header text unchanged.
In `@frontend/src/wizards/WizardPage.css`:
- Around line 23-44: Replace the custom modal layout with supported PatternFly
composition: remove the direct .pf-v6-* overrides in
frontend/src/wizards/WizardPage.css at lines 23-44, replace the custom outer
shell in frontend/src/wizards/WizardPage.tsx at lines 69-77 with appropriate
PatternFly layout primitives and variants, and remove the duplicate
inline-styled modal wrapper in
frontend/src/routes/Applications/CreateArgoApplication/EditArgoApplicationSet.tsx
at lines 232-246.
🪄 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: 3fed08f7-b89e-4d12-a4a0-08f2232176ac
⛔ Files ignored due to path filters (1)
frontend/public/locales/en/translation.jsonis excluded by!frontend/public/locales/**
📒 Files selected for processing (62)
frontend/packages/react-form-wizard/src/Wizard.tsxfrontend/src/components/SyncEditor/SyncEditor.tsxfrontend/src/components/SyncEditor/SyncEditorDiff.tsxfrontend/src/components/SyncEditor/process.tsfrontend/src/components/SyncEditor/synchronize.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationDetails/ApplicationDetails.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/ApplicationTopology.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/__fixtures__/topologyAnalysisFixtures.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopology.test.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopology.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopologyAppSet.test.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopologyAppSet.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopologyApplications.test.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopologyApplications.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopologyClusters.test.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopologyClusters.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/createSuggestsApplication.test.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/createSuggestsApplication.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/createSuggestsAppset.test.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/createSuggestsAppset.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/createSuggestsPlacement.test.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/createSuggestsPlacement.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/utils.test.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/utils.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/components/DetailsView.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/components/LogsContainer.cssfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/components/LogsContainer.test.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/components/LogsContainer.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/components/YAMLContainer.test.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/components/YAMLContainer.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/modals/EditAppSetModal.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/modals/EditYamlModal.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/modals/LogsModal.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/model/NodeDetailsProviderStatuses.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/model/topology.test.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/model/topology.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/model/topologyAppSet.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/Topology.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/CustomEllipse.test.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/CustomEllipse.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/CustomEllipseAnchor.test.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/CustomEllipseAnchor.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/MultiEllipse.test.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/MultiEllipse.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/StyledEdge.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/StyledNode.test.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/StyledNode.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/TopologyAlerts.test.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/TopologyAlerts.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/nodeIcons.tsxfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/nodeStyle.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/css/Drawer.cssfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/types.tsfrontend/src/routes/Applications/Applications.tsxfrontend/src/routes/Applications/CreateArgoApplication/CreatePullApplicationSet.test.tsxfrontend/src/routes/Applications/CreateArgoApplication/CreatePushApplicationSet.test.tsxfrontend/src/routes/Applications/CreateArgoApplication/EditApplicationSetPage.tsxfrontend/src/routes/Applications/CreateArgoApplication/EditArgoApplicationSet.tsxfrontend/src/typings.d.tsfrontend/src/wizards/Argo/ArgoWizard.tsxfrontend/src/wizards/WizardPage.cssfrontend/src/wizards/WizardPage.tsx
💤 Files with no reviewable changes (8)
- frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/components/LogsContainer.css
- frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/MultiEllipse.tsx
- frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/components/LogsContainer.test.tsx
- frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/topology/components/MultiEllipse.test.tsx
- frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/components/LogsContainer.tsx
- frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/components/YAMLContainer.tsx
- frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/components/YAMLContainer.test.tsx
- frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/model/NodeDetailsProviderStatuses.tsx
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Signed-off-by: John Swanke <jswanke@redhat.com>
Signed-off-by: John Swanke <jswanke@redhat.com>
Signed-off-by: John Swanke <jswanke@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopologyClusters.ts`:
- Around line 146-150: Wrap the hardcoded `'OpenShift GitOps Operator issues'`
title passed to `createTopologyAlert` in the file’s existing `t()` translation
function, matching the localized alert titles used elsewhere in
`analyzeTopologyClusters` while leaving the description handling unchanged.
In
`@frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopologyDeployments.ts`:
- Around line 45-54: Update the message construction near title in the topology
deployment analysis to use a single i18next translation key with interpolation
for kind and name, rather than concatenating t('Resource did not deploy') with a
template literal. Preserve the existing message meaning and values while
allowing translations to control word order.
🪄 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: 5b221894-f504-4661-b62c-23c5301065d8
📒 Files selected for processing (6)
frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopologyAppSet.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopologyApplications.test.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopologyApplications.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopologyClusters.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopologyDeployments.test.tsfrontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopologyDeployments.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopologyApplications.test.ts
- frontend/src/routes/Applications/ApplicationDetails/ApplicationTopology/analysis/analyzeTopologyApplications.ts
…-32879-Appset-Topology-Topology-Alert-window-5 Signed-off-by: John Swanke <jswanke@redhat.com>
Signed-off-by: John Swanke <jswanke@redhat.com>
…-32879-Appset-Topology-Topology-Alert-window-5 Signed-off-by: John Swanke <jswanke@redhat.com>
Signed-off-by: John Swanke <jswanke@redhat.com>
|
|
@jeswanke: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
📝 Summary
implementation notes:
https://docs.google.com/document/d/1vourRy1EXeW3Lzkil8CubyrplJ9A2yYRIbVTF49K2tM/edit?tab=t.0#heading=h.y71s9opsqi5z
demo:
https://redhat-internal.slack.com/archives/C02684L8JMV/p1784316944188839
Ticket Summary (Title):
Appset Topology: Topology Alert window
Ticket Link:
https://redhat.atlassian.net/browse/ACM-32879
Type of Change:
✅ Checklist
General
ACM-12340 Fix bug with...)If Feature
If Bugfix
🗒️ Notes for Reviewers
Summary by CodeRabbit