ACM-33855 wizard review page filter to just show differences#6259
Merged
jeswanke merged 18 commits intoJun 10, 2026
Merged
Conversation
Signed-off-by: John Swanke <jswanke@redhat.com>
|
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:
📝 WalkthroughWalkthroughAdds default-data snapshot support and default-array input registration to the wizard, implements a persisted "show changes only" review filter with path/value comparison, introduces getItemValue YAML-path resolver, and refactors SyncEditor to support diff view, diff navigation, model refresh, decoration changes, and improved Monaco test mocks. ChangesForm Wizard: Default snapshot & review
SyncEditor, decorations, and Monaco mocks
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ 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: 8
🤖 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/packages/react-form-wizard/src/review/ReviewStepFilterList.tsx`:
- Around line 70-88: The current reviewValuesEqualAtPath uses JSON.stringify
fallback which is key-order sensitive; replace that fallback with a
deterministic deep equality for arrays and plain objects: implement object
comparison by checking both are objects (or arrays), compare array lengths and
recursively compare elements, and for objects compare sorted keys (or iterate
keys from Object.keys(a).sort()), ensure same key set and recursively compare
values via reviewValuesEqualAtPath; keep existing short-circuits for
booleans/null/undefined and the Object.is check. Use the function name
reviewValuesEqualAtPath to locate and replace the JSON.stringify-based
comparison with this order-insensitive deep-equal logic.
In `@frontend/packages/react-form-wizard/src/review/utils.ts`:
- Around line 179-194: getItemValue currently strips the ";id=..." discriminator
from yamlPath before selecting an array element, causing wrong matches; fix by
extracting and preserving the id discriminator (e.g., with /;id=([^;]+)$/) into
a variable like idDiscriminator before you call clean/normalized/kindHead, then
when Array.isArray(item) inside getItemValue use that idDiscriminator in the
find callback: if idDiscriminator exists, prefer matching the resource by its
unique id field (check common places like resource.id or resource.metadata?.id)
and fall back to the kindHead match only when no discriminator is present. This
preserves per-resource selection while keeping the rest of the path logic
intact.
In `@frontend/src/components/SyncEditor/SyncEditor.tsx`:
- Around line 235-236: The detection for full-document paste in SyncEditor.tsx
is too broad (only checks endLineNumber) and can misfire on partial
replacements; update the if condition that currently checks
selection?.startColumn and selection?.endLineNumber to require the selection to
start at line 1 column 1 and end at the document's last line and its max column.
Concretely, change the condition around the autoCreateNs branch (where
resetDefaultSnapshotOnNextReportRef is set) to verify selection.startLineNumber
=== 1 && selection.startColumn === 1 && selection.endLineNumber ===
model.getLineCount() && selection.endColumn ===
model.getLineMaxColumn(selection.endLineNumber) so only true full-range
selections trigger the reset.
- Around line 701-705: The decorate call is using editorHasFocus which is false
in diff mode even when the modified diff pane is focused; change the argument to
use a combined focus state (e.g., const combinedFocus = editorHasFocus ||
(isDiffMode && modifiedEditorHasFocus)) and pass combinedFocus into
decorate(...) instead of editorHasFocus so decorations/scroll behavior treat the
focused diff pane as focused; update any related callers that compute focus for
diff/modified panes accordingly (refer to decorate, editorHasFocus, isDiffMode,
and modifiedEditorHasFocus).
- Around line 377-390: The blur listener created via
activeEditor.onDidBlurEditorWidget is not disposed, causing listener
accumulation; capture the returned disposable (from
activeEditor.onDidBlurEditorWidget(...)) and store it in a variable, then call
disposable.dispose() in the effect cleanup (and/or before re-assigning when
activeEditor changes) so the subscription is removed; update the useEffect that
registers the listener around activeEditor and ensure setClickedOnFilteredLine
and setEditorHasFocus logic remains inside the disposed callback.
In `@frontend/src/components/SyncEditor/SyncEditorToolbar.tsx`:
- Line 90: The title fallback in SyncEditorToolbar uses a hardcoded string
'YAML' instead of the translation function; update the JSX so the fallback uses
the i18n translator (e.g., t('YAML') or an existing key) when editorTitle is
falsy, ensuring you import/use the same t function used elsewhere in the
component and preserve the current prop name editorTitle and className
"sy-c-code-editor__title".
In `@frontend/src/lib/wizardStrings.ts`:
- Line 44: The new label reviewShowChangesOnlyLabel currently uses a hardcoded
English string t('Show changes'); replace it with a hierarchical translation key
(e.g., 'wizard.review.showChanges') in frontend/src/lib/wizardStrings.ts by
calling t('wizard.review.showChanges') for reviewShowChangesOnlyLabel, then add
the corresponding entry and translations to the locale files (e.g., en.json)
under the same 'wizard.review.showChanges' key so no user-facing English text
remains in code.
In `@frontend/src/wizards/Argo/ArgoWizard.tsx`:
- Around line 286-289: The title rendering incorrectly checks resources
truthiness which causes an empty array to display the Edit title; instead use
the computed editMode variable consistently. Update the title logic that
currently inspects resources (around where the UI renders the heading) to check
if editMode === EditMode.Edit (and otherwise EditMode.Create) and remove/replace
any direct resources-based checks; ensure any other UI branches (e.g.,
defaultData assignment using defaultData = resources and related conditional
rendering) also rely on editMode so create vs edit state is unified.
🪄 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: fb6a59f5-fcb2-449f-9737-4689bdc4c7f6
⛔ Files ignored due to path filters (1)
frontend/public/locales/en/translation.jsonis excluded by!frontend/public/locales/**
📒 Files selected for processing (35)
frontend/packages/react-form-wizard/src/Wizard.tsxfrontend/packages/react-form-wizard/src/contexts/DataContext.tsxfrontend/packages/react-form-wizard/src/contexts/DefaultDataContext.tsxfrontend/packages/react-form-wizard/src/contexts/StringContext.tsxfrontend/packages/react-form-wizard/src/index.tsfrontend/packages/react-form-wizard/src/inputs/Input.tsfrontend/packages/react-form-wizard/src/review/ReviewStep.cssfrontend/packages/react-form-wizard/src/review/ReviewStep.tsxfrontend/packages/react-form-wizard/src/review/ReviewStepFilterList.tsxfrontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsxfrontend/packages/react-form-wizard/src/review/ReviewStepToolbar.tsxfrontend/packages/react-form-wizard/src/review/utils.tsfrontend/src/components/SyncEditor/SyncEditor.cssfrontend/src/components/SyncEditor/SyncEditor.test.tsxfrontend/src/components/SyncEditor/SyncEditor.tsxfrontend/src/components/SyncEditor/SyncEditorDiff.tsxfrontend/src/components/SyncEditor/SyncEditorToolbar.tsxfrontend/src/components/SyncEditor/decorate.test.tsfrontend/src/components/SyncEditor/decorate.tsfrontend/src/components/SyncEditor/process.tsfrontend/src/lib/wizardStrings.tsfrontend/src/routes/Applications/CreateArgoApplication/CreatePullApplicationSet.tsxfrontend/src/routes/Applications/CreateArgoApplication/CreatePushApplicationSet.tsxfrontend/src/routes/Applications/CreateArgoApplication/EditArgoApplicationSet.tsxfrontend/src/routes/Governance/policies/CreatePolicy.tsxfrontend/src/routes/Governance/policies/CreatePolicyAutomation.tsxfrontend/src/routes/Governance/policies/EditPolicy.tsxfrontend/src/routes/Governance/policies/EditPolicyAutomation.tsxfrontend/src/routes/Governance/policy-sets/CreatePolicySet.tsxfrontend/src/routes/Governance/policy-sets/EditPolicySet.tsxfrontend/src/routes/Infrastructure/Clusters/ManagedClusters/ImportCluster/ImportCluster.tsxfrontend/src/routes/Infrastructure/Clusters/Placements/CreatePlacement/CreatePlacement.tsxfrontend/src/routes/Infrastructure/Clusters/Placements/CreatePlacement/EditPlacement.tsxfrontend/src/wizards/Argo/ArgoWizard.tsxfrontend/src/wizards/Argo/CreateArgoResources.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/components/SyncEditor/SyncEditor.tsx`:
- Around line 870-873: The container's compare-mode class is currently applied
using only showChanges; change the className logic on the element that uses
pageRef (the div with className `sync-editor__container`) to add
`sync-editor__container--show-changes` only when the full diff-eligibility
condition is true (i.e., showChanges && defaultResources !== undefined && !mock)
so the compare-only styling matches the actual diff rendering conditions
(references: pageRef, showChanges, defaultResources, mock, and
`sync-editor__container`).
🪄 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: 9a49706a-61e1-449e-89e8-d495d83802c2
📒 Files selected for processing (3)
frontend/src/components/SyncEditor/SyncEditor.cssfrontend/src/components/SyncEditor/SyncEditor.tsxfrontend/src/components/SyncEditor/decorate.ts
Signed-off-by: John Swanke <jswanke@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/__mocks__/`@monaco-editor/react.tsx:
- Line 303: The mock for useMonaco returns a tuple but the real
`@monaco-editor/react` exports useMonaco() that returns monaco_editor | null;
update the mock for useMonaco (the exported symbol useMonaco) to return a single
value (null or a mocked monaco instance) with the correct type
(monaco.editor.IStandaloneCodeEditor | typeof monaco | null as appropriate)
instead of [null, () => undefined], so change the exported implementation to
return null (or a typed mock) matching the library's return shape and update any
related type annotations to ensure compatibility.
🪄 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: 8a24ee9f-a041-40cf-8ebc-ee5c3fcda88d
⛔ Files ignored due to path filters (1)
frontend/public/locales/en/translation.jsonis excluded by!frontend/public/locales/**
📒 Files selected for processing (8)
frontend/__mocks__/@monaco-editor/react.tsxfrontend/__mocks__/react-monaco-editor.tsxfrontend/jest.config.tsfrontend/packages/react-form-wizard/src/review/ReviewStepFilterList.tsxfrontend/src/components/SyncEditor/SyncEditor.test.tsxfrontend/src/components/SyncEditor/SyncEditor.tsxfrontend/src/components/SyncEditor/decorate.test.tsfrontend/src/lib/wizardStrings.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/lib/wizardStrings.ts
- frontend/packages/react-form-wizard/src/review/ReviewStepFilterList.tsx
- frontend/src/components/SyncEditor/SyncEditor.tsx
| export const Editor = MonacoEditor | ||
| export const DiffEditor = MonacoEditor | ||
| export const loader = { config: () => undefined } | ||
| export const useMonaco = () => [null, () => undefined] as const |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Confirm library typing for v4.6.0
curl -s https://unpkg.com/@monaco-editor/react@4.6.0/dist/index.d.ts | rg -n 'useMonaco'
# 2) Check local usage expectations
rg -nP '\buseMonaco\s*\(' frontend -C2Repository: stolostron/console
Length of output: 355
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search repo-wide for any usage patterns of `useMonaco(`
rg -nP '\buseMonaco\s*\(' . -C2Repository: stolostron/console
Length of output: 44
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find any mention/imports of useMonaco
rg -nP '\buseMonaco\b' . -S -g'*.{ts,tsx,js,jsx}'
# 2) Look for destructuring patterns that would indicate tuple usage
rg -nP '\[\s*[^]]*\]\s*=\s*useMonaco\s*\(' . -S -g'*.{ts,tsx,js,jsx}'
rg -nP '\(\s*[^)]*\)\s*=\s*useMonaco\s*\(' . -S -g'*.{ts,tsx,js,jsx}'
rg -nP '\{[^}]*\}\s*=\s*useMonaco\s*\(' . -S -g'*.{ts,tsx,js,jsx}'Repository: stolostron/console
Length of output: 176
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Repo-wide (all files) search for any mention of `useMonaco`
rg -nP '\buseMonaco\b' . -SRepository: stolostron/console
Length of output: 176
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP '\buseMonaco\b' frontend/__mocks__/@monaco-editor/react.tsxRepository: stolostron/console
Length of output: 130
Fix useMonaco mock to match @monaco-editor/react’s return type
frontend/__mocks__/@monaco-editor/react.tsx currently returns a tuple ([null, () => undefined]), but @monaco-editor/react defines useMonaco() as returning monaco_editor | null directly. Aligning the mock avoids type/behavior mismatches.
Proposed fix
-export const useMonaco = () => [null, () => undefined] as const
+export const useMonaco = () => null🤖 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/__mocks__/`@monaco-editor/react.tsx at line 303, The mock for
useMonaco returns a tuple but the real `@monaco-editor/react` exports useMonaco()
that returns monaco_editor | null; update the mock for useMonaco (the exported
symbol useMonaco) to return a single value (null or a mocked monaco instance)
with the correct type (monaco.editor.IStandaloneCodeEditor | typeof monaco |
null as appropriate) instead of [null, () => undefined], so change the exported
implementation to return null (or a typed mock) matching the library's return
shape and update any related type annotations to ensure compatibility.
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/__mocks__/`@monaco-editor/react.tsx:
- Around line 203-206: There is a duplicate property definition for
onDidChangeModelContent in the mock module: the simple stub
onDidChangeModelContent: () => mockDisposable() is shadowed by a later
callback-storing implementation; remove the earlier stub so only the
callback-storing version remains (locate the two occurrences of
onDidChangeModelContent in the `@monaco-editor/react` mock and delete the first
stub that returns mockDisposable, keeping the richer implementation that
registers and stores callbacks).
In `@frontend/src/components/SyncEditor/process.test.ts`:
- Around line 118-131: The CachedValuesType.path and related test overrides are
too narrow: change CachedValuesType.path from string to string | (string |
number)[] and widen cached value types (e.g., cacheFiltered and any filter
cached `value`) to accept non-string shapes (string | number | (string |
number)[] or unknown) to match MappingType.value (string | (string|number)[]);
update the test helper processUserDefaults override types accordingly so its
cachedSecrets/cacheFiltered entries accept path: string | (string|number)[] and
non-string values, and ensure any usages of MappingType.$p are compatible with
the revised CachedValuesType.
🪄 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: 94497880-1f7e-4714-9347-3b90897e24ec
📒 Files selected for processing (4)
frontend/__mocks__/@monaco-editor/react.tsxfrontend/src/components/SyncEditor/SyncEditor.test.tsxfrontend/src/components/SyncEditor/process.test.jsfrontend/src/components/SyncEditor/process.test.ts
💤 Files with no reviewable changes (1)
- frontend/src/components/SyncEditor/process.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/SyncEditor/SyncEditor.test.tsx
Signed-off-by: John Swanke <jswanke@redhat.com>
Contributor
|
Looks great John! I will put on hold in case Kevin has anything. /lgtm |
KevinFCormier
changed the title
Signed-off-by: John Swanke <jswanke@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/packages/react-form-wizard/src/review/ReviewStepContexts.tsx`:
- Around line 229-240: Add a regression test that verifies
DefaultArrayInputRegisterProvider rebuilds when defaultDataSnapshot is reset:
mount the provider with an initial snapshot and steps (using
stepRegister.getSteps()/buildDefaultArrayInputRegisterFromStepTrees), let it
capture (observe lastCapturedSnapshotRef behavior indirectly), then call
update(..., true) to reset defaultDataSnapshot and update the step tree via
sr.setStepTree (or bump stepRegister.version) to reflect the new snapshot;
finally assert the provider’s register (or ReviewStepFilterList output /
rowMatchesChangesOnlyFilter behavior) reflects the new snapshot’s ARRAY_INPUT
subtrees rather than the stale ones. Ensure the test references
DefaultArrayInputRegisterProvider, buildDefaultArrayInputRegisterFromStepTrees,
stepRegister (sr.setStepTree / version), and ReviewStepFilterList /
rowMatchesChangesOnlyFilter so it fails if rebuild doesn’t occur.
In `@frontend/src/components/SyncEditor/SyncEditorDiff.test.tsx`:
- Around line 114-115: Tests in this suite use fireEvent.change for simulating
typing, which violates the guideline; import userEvent from
'`@testing-library/user-event`' and replace fireEvent.change(input, { target: {
value: '...' } }) with the equivalent user-facing calls (e.g., await
userEvent.clear(input) then await userEvent.type(input, 'apiVersion: v1\nkind:
Pod\n') or await userEvent.paste(input, '...') as appropriate), and update the
test functions to be async where needed; apply the same replacements for the
other occurrences around the suite (the lines referenced near 124-127 and
145-146) and remove the fireEvent usage.
- Around line 59-193: Add a jest-axe accessibility assertion to this test suite
by rendering the component via renderSyncEditorDiff and calling await
axe(container) then expect(...).toHaveNoViolations(); e.g., add a test (or
extend an existing one such as the "mounts the diff editor..." case) that
obtains the rendered container from renderSyncEditorDiff, awaits axe(container)
and asserts toHaveNoViolations(); ensure you reference the axe import and use
await so the accessibility check runs after the diff editor is mounted.
🪄 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: caac5b08-ab52-474d-b903-1a1baf7862e4
⛔ Files ignored due to path filters (1)
frontend/public/locales/en/translation.jsonis excluded by!frontend/public/locales/**
📒 Files selected for processing (13)
frontend/__mocks__/@monaco-editor/react.tsxfrontend/packages/react-form-wizard/src/Wizard.tsxfrontend/packages/react-form-wizard/src/contexts/StringContext.tsxfrontend/packages/react-form-wizard/src/review/ReviewStep.cssfrontend/packages/react-form-wizard/src/review/ReviewStep.tsxfrontend/packages/react-form-wizard/src/review/ReviewStepContexts.tsxfrontend/packages/react-form-wizard/src/review/ReviewStepFilterList.tsxfrontend/packages/react-form-wizard/src/review/utils.tsfrontend/src/components/SyncEditor/SyncEditor.cssfrontend/src/components/SyncEditor/SyncEditor.test.tsxfrontend/src/components/SyncEditor/SyncEditorDiff.test.tsxfrontend/src/components/SyncEditor/process.test.tsfrontend/src/lib/wizardStrings.ts
💤 Files with no reviewable changes (1)
- frontend/packages/react-form-wizard/src/review/utils.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/packages/react-form-wizard/src/contexts/StringContext.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- frontend/src/lib/wizardStrings.ts
- frontend/packages/react-form-wizard/src/review/ReviewStep.tsx
- frontend/src/components/SyncEditor/SyncEditor.css
- frontend/packages/react-form-wizard/src/Wizard.tsx
- frontend/src/components/SyncEditor/process.test.ts
- frontend/mocks/@monaco-editor/react.tsx
- frontend/packages/react-form-wizard/src/review/ReviewStepFilterList.tsx
Comment on lines
+59
to
+193
| describe('SyncEditorDiff', () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks() | ||
| mockUseResizeObserver.mockImplementation((_target, callback) => { | ||
| mockResizeCallback.mockImplementation(callback) | ||
| return {} as ResizeObserver | ||
| }) | ||
| }) | ||
|
|
||
| it('renders nothing when showChanges is false', () => { | ||
| renderSyncEditorDiff({ showChanges: false }) | ||
| expect(screen.queryByRole('textbox', { name: /monaco-diff/i })).not.toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('renders nothing when defaultResources is undefined', () => { | ||
| renderSyncEditorDiff({ defaultResources: undefined }) | ||
| expect(screen.queryByRole('textbox', { name: /monaco-diff/i })).not.toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('renders nothing when mock mode is enabled', () => { | ||
| renderSyncEditorDiff({ mock: true }) | ||
| expect(screen.queryByRole('textbox', { name: /monaco-diff/i })).not.toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('mounts the diff editor and exposes imperative handle accessors', async () => { | ||
| const ref = createRef<SyncEditorDiffHandle>() | ||
| const { onDiffEditorInstanceChange, onActiveInstancesChange } = renderSyncEditorDiff({}, ref) | ||
|
|
||
| await waitFor(() => expect(screen.getByRole('textbox', { name: /monaco-diff/i })).toBeInTheDocument()) | ||
|
|
||
| expect(onActiveInstancesChange).toHaveBeenCalled() | ||
| expect(onDiffEditorInstanceChange).toHaveBeenCalled() | ||
| expect(ref.current?.getDiffEditor()).not.toBeNull() | ||
| expect(ref.current?.getOriginalEditor()).not.toBeNull() | ||
| expect(ref.current?.getModifiedEditor()).not.toBeNull() | ||
| expect(ref.current?.getDiffEditorMonaco()).not.toBeNull() | ||
| }) | ||
|
|
||
| it('invokes diff navigator previous and next from the imperative handle', async () => { | ||
| const ref = createRef<SyncEditorDiffHandle>() | ||
| renderSyncEditorDiff({}, ref) | ||
| await waitFor(() => expect(screen.getByRole('textbox', { name: /monaco-diff/i })).toBeInTheDocument()) | ||
|
|
||
| expect(() => ref.current?.previous()).not.toThrow() | ||
| expect(() => ref.current?.next()).not.toThrow() | ||
|
|
||
| await act(async () => { | ||
| await new Promise((resolve) => requestAnimationFrame(resolve)) | ||
| }) | ||
| }) | ||
|
|
||
| it('notifies onChange when the modified pane content changes', async () => { | ||
| const { onChange } = renderSyncEditorDiff() | ||
| const input = await waitFor(() => screen.getByRole('textbox', { name: /monaco-diff/i })) | ||
|
|
||
| fireEvent.change(input, { target: { value: 'apiVersion: v1\nkind: Pod\n' } }) | ||
|
|
||
| await waitFor(() => expect(onChange).toHaveBeenCalled()) | ||
| expect(onChange.mock.calls[0][0]).toContain('kind: Pod') | ||
| }) | ||
|
|
||
| it('notifies focus changes when the diff editor gains and loses focus', async () => { | ||
| const { onDiffEditorFocusChange } = renderSyncEditorDiff() | ||
| const input = await waitFor(() => screen.getByRole('textbox', { name: /monaco-diff/i })) | ||
|
|
||
| fireEvent.focus(input) | ||
| expect(onDiffEditorFocusChange).toHaveBeenCalledWith(true) | ||
|
|
||
| fireEvent.blur(input) | ||
| await act(async () => { | ||
| await new Promise((resolve) => requestAnimationFrame(resolve)) | ||
| }) | ||
| expect(onDiffEditorFocusChange).toHaveBeenCalledWith(false) | ||
| }) | ||
|
|
||
| it('does not report blur when focus moves to a diff toolbar control', async () => { | ||
| const { onDiffEditorFocusChange } = renderSyncEditorDiff() | ||
| const input = await waitFor(() => screen.getByRole('textbox', { name: /monaco-diff/i })) | ||
|
|
||
| fireEvent.focus(input) | ||
| onDiffEditorFocusChange.mockClear() | ||
|
|
||
| const toolbarButton = document.createElement('button') | ||
| toolbarButton.id = 'diff-prev-button' | ||
| document.body.appendChild(toolbarButton) | ||
| toolbarButton.focus() | ||
| fireEvent.blur(input) | ||
|
|
||
| await act(async () => { | ||
| await new Promise((resolve) => requestAnimationFrame(resolve)) | ||
| }) | ||
|
|
||
| expect(onDiffEditorFocusChange).not.toHaveBeenCalledWith(false) | ||
| toolbarButton.remove() | ||
| }) | ||
|
|
||
| it('cleans up and notifies instance change when diff view is hidden', async () => { | ||
| const { onDiffEditorInstanceChange, onDiffEditorFocusChange, rerender, props } = renderSyncEditorDiff() | ||
|
|
||
| await waitFor(() => expect(screen.getByRole('textbox', { name: /monaco-diff/i })).toBeInTheDocument()) | ||
| onDiffEditorInstanceChange.mockClear() | ||
| onDiffEditorFocusChange.mockClear() | ||
|
|
||
| rerender(<SyncEditorDiff {...props} showChanges={false} />) | ||
|
|
||
| expect(screen.queryByRole('textbox', { name: /monaco-diff/i })).not.toBeInTheDocument() | ||
| expect(onDiffEditorInstanceChange).toHaveBeenCalled() | ||
| expect(onDiffEditorFocusChange).toHaveBeenCalledWith(false) | ||
| }) | ||
|
|
||
| it('applies layout when the resize observer fires', async () => { | ||
| renderSyncEditorDiff() | ||
| await waitFor(() => expect(screen.getByRole('textbox', { name: /monaco-diff/i })).toBeInTheDocument()) | ||
|
|
||
| const host = document.querySelector('.sync-editor__diff-host') as HTMLDivElement | ||
| jest.spyOn(host, 'getBoundingClientRect').mockReturnValue({ | ||
| width: 900, | ||
| height: 400, | ||
| x: 0, | ||
| y: 0, | ||
| top: 0, | ||
| left: 0, | ||
| right: 900, | ||
| bottom: 400, | ||
| toJSON: () => ({}), | ||
| }) | ||
|
|
||
| await act(async () => { | ||
| mockResizeCallback() | ||
| await new Promise((resolve) => requestAnimationFrame(resolve)) | ||
| }) | ||
|
|
||
| expect(mockUseResizeObserver).toHaveBeenCalled() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Add a jest-axe accessibility assertion for SyncEditorDiff tests.
This component test suite is missing the required accessibility check.
As per coding guidelines: "Every component test must include an accessibility test with jest-axe: expect(await axe(container)).toHaveNoViolations()".
🤖 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.test.tsx` around lines 59 -
193, Add a jest-axe accessibility assertion to this test suite by rendering the
component via renderSyncEditorDiff and calling await axe(container) then
expect(...).toHaveNoViolations(); e.g., add a test (or extend an existing one
such as the "mounts the diff editor..." case) that obtains the rendered
container from renderSyncEditorDiff, awaits axe(container) and asserts
toHaveNoViolations(); ensure you reference the axe import and use await so the
accessibility check runs after the diff editor is mounted.
Comment on lines
+114
to
+115
| fireEvent.change(input, { target: { value: 'apiVersion: v1\nkind: Pod\n' } }) | ||
|
|
There was a problem hiding this comment.
Replace fireEvent interactions with userEvent in this suite.
These interaction paths are implemented with fireEvent, which conflicts with the frontend test guideline and can under-represent real user behavior.
As per coding guidelines: "Use @testing-library/user-event for user interactions in tests — not fireEvent".
Also applies to: 124-127, 145-146
🤖 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.test.tsx` around lines 114
- 115, Tests in this suite use fireEvent.change for simulating typing, which
violates the guideline; import userEvent from '`@testing-library/user-event`' and
replace fireEvent.change(input, { target: { value: '...' } }) with the
equivalent user-facing calls (e.g., await userEvent.clear(input) then await
userEvent.type(input, 'apiVersion: v1\nkind: Pod\n') or await
userEvent.paste(input, '...') as appropriate), and update the test functions to
be async where needed; apply the same replacements for the other occurrences
around the suite (the lines referenced near 124-127 and 145-146) and remove the
fireEvent usage.
…-33855-Wizard-Review-Page-Filter-to-just-show-differences-2 Signed-off-by: John Swanke <jswanke@redhat.com>
Contributor
Author
|
/hold |
Contributor
|
/lgtm |
KevinFCormier
left a comment
KevinFCormier
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the updates @jeswanke. Much better. Couple things:
- Can you please take a look at the Sonar issues... there are a bunch of minor things that could be cleaned up.
- If you can fix the green background behind changes - sometimes when the form is narrow, the text extends past the green box.
Signed-off-by: John Swanke <jswanke@redhat.com>
|
KevinFCormier
approved these changes
Jun 9, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fxiang1, jeswanke, KevinFCormier 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 |
12 tasks
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
3 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.
📝 Summary
demo is here:
https://redhat-internal.slack.com/archives/C02684L8JMV/p1780001491259069
coding details are here:
https://docs.google.com/document/d/1uhesSDfaXbi4vwQO8hz56MTRd--UGpt6R84JH8gEZgM/edit?tab=t.0
Ticket Summary (Title):
wizard review page filter to just show differences
Ticket Link:
https://redhat.atlassian.net/browse/ACM-33855
Type of Change:
✅ Checklist
General
ACM-12340 Fix bug with...)If Feature
If Bugfix
🗒️ Notes for Reviewers
Summary by CodeRabbit
New Features
Style
Tests