ACM-38199: Decorate resource statuses in YAML editor - #6591
Open
Ginxo wants to merge 11 commits into
Open
Conversation
…199) Tint success/failure conditions, emphasize reason/message, sort status and condition keys, and apply the same decorations in Search YAML. Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…M-38199) Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.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 shared Kubernetes status classification and YAML key ordering, Monaco status decorations for SyncEditor and Search YAML editors, theme-aware styling, and defensive handling for malformed URLs and development overlay errors. ChangesStatus-aware YAML editors
Defensive runtime handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant YAMLEditor
participant statusYamlDecorations
participant YAMLParser
participant Monaco
YAMLEditor->>statusYamlDecorations: registerSearchYamlStatusDecorations
statusYamlDecorations->>Monaco: read model content
statusYamlDecorations->>YAMLParser: parse YAML status
YAMLParser-->>statusYamlDecorations: return status nodes
statusYamlDecorations->>Monaco: apply status range decorations
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
Contributor
Author
|
/hold |
![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
🧹 Nitpick comments (1)
frontend/src/components/SyncEditor/decorate.ts (1)
48-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
as neverto satisfy the type checker.Casting to
neversilences the mismatch betweenchange.mappings: { [name: string]: any[] }andgetStatusDecorationsFromMappings's expected{ [name: string]: MappingLeaf[] } | undefinedparam, but it also suppresses any future type-checking on this call entirely (sinceneveris assignable to anything). Prefer a narrower assertion or aligning the mapping type across both files.🤖 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/decorate.ts` around lines 48 - 50, Update the call to getStatusDecorationsFromMappings in the decoration flow to remove the as never cast. Align change.mappings with the function’s expected MappingLeaf mapping type, or use a narrower assertion that preserves type checking while retaining undefined handling.
🤖 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/statusDecorations.ts`:
- Around line 260-279: The failure detection in decorateLastStateErrors is too
narrow because it only highlights terminated states with reason exactly equal to
'Error'. Update it to mark termination failures for non-success reasons such as
OOMKilled, ContainerCannotRun, or DeadlineExceeded, and for non-zero exitCode
values, while preserving the existing decoration flow. Apply the same
consolidated failure-detection behavior in decorateStatusMap in
statusYamlDecorations.ts.
- Around line 108-117: Update reorderObjectKeys to skip the dangerous keys
"__proto__", "constructor", and "prototype" before assigning entries to ordered;
preserve sorting and copying for all other keys.
- Around line 58-68: Remove the ineffective reasonOrMessage regex check from the
condition-status logic after the AsExpected case, and keep the unconditional
'failure' fallback unless a distinct non-matching outcome is explicitly
required.
In `@frontend/src/routes/Search/components/YamlEditor/statusYamlDecorations.ts`:
- Around line 5-12: Update the imports in statusYamlDecorations.ts to use the
project’s ~/ path alias instead of the deep relative path, preserving all
imported symbols and their existing source module.
- Around line 112-127: Update the terminated-container handling in the container
status decoration loop to apply STATUS_FAILURE_CLASS only when the terminated
state’s reason is Error, preserving the existing lastState decoration path and
matching the narrowed condition used by decorateLastStateErrors in
statusDecorations.ts.
In `@frontend/src/routes/Search/components/YamlEditor/utils.tsx`:
- Line 10: Replace the deep relative import of prepareResourceForYaml with the
project’s ~/ path alias, keeping the imported symbol and module unchanged.
In `@frontend/src/routes/Search/components/YamlEditor/YAMLEditor.tsx`:
- Around line 31-32: Update the imports in YAMLEditor.tsx to replace the deep
relative paths with the project’s ~/ alias, preserving the existing
prepareResourceForYaml import and statusDecorations.css side-effect import.
- Around line 82-84: Update the YAMLEditor setup to capture the disposer
returned by registerSearchYamlStatusDecorations(editor) and register it with
Monaco’s editor.onDidDispose handler, ensuring the onDidChangeModelContent
listener is removed when the editor is destroyed or remounted.
---
Nitpick comments:
In `@frontend/src/components/SyncEditor/decorate.ts`:
- Around line 48-50: Update the call to getStatusDecorationsFromMappings in the
decoration flow to remove the as never cast. Align change.mappings with the
function’s expected MappingLeaf mapping type, or use a narrower assertion that
preserves type checking while retaining undefined handling.
🪄 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: Pro Plus
Run ID: 17951c97-f738-40e1-a34f-9a7869543f5b
📒 Files selected for processing (11)
frontend/src/components/SyncEditor/SyncEditor.tsxfrontend/src/components/SyncEditor/decorate.test.tsfrontend/src/components/SyncEditor/decorate.tsfrontend/src/components/SyncEditor/process.test.tsfrontend/src/components/SyncEditor/process.tsfrontend/src/components/SyncEditor/statusDecorations.cssfrontend/src/components/SyncEditor/statusDecorations.test.tsfrontend/src/components/SyncEditor/statusDecorations.tsfrontend/src/routes/Search/components/YamlEditor/YAMLEditor.tsxfrontend/src/routes/Search/components/YamlEditor/statusYamlDecorations.tsfrontend/src/routes/Search/components/YamlEditor/utils.tsx
Filter the OpenShift console CSP Invalid URL runtime error from the plugin webpack overlay, and make Search resource/Grafana URL parsing tolerant of malformed values during local plugin development. Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Remove dead classifyCondition branch, harden key reordering, broaden terminated-container failure detection, use ~/ imports, dispose Search YAML decoration listeners, and drop the as never mappings cast. Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.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/plugins/webpack.plugin.base.ts`:
- Around line 142-151: Update the runtimeErrors predicate to suppress only the
expected TypeError whose message contains “Failed to construct 'URL'”. Remove
the broader “Invalid URL” match, and preserve returning true for all other
runtime errors.
In `@frontend/src/routes/Search/Details/DetailsOverviewPage.tsx`:
- Around line 341-345: Guard dashboard URL construction with a valid grafanaLink
after the URL parsing try/catch; in
frontend/src/routes/Search/Details/DetailsOverviewPage.tsx lines 341-345, return
the metrics URL only when grafanaLink is defined, and apply the same
valid-origin guard in frontend/src/routes/Search/searchDefinitions.tsx lines
872-876 for VMLaunchLinks.
🪄 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: Pro Plus
Run ID: ab892fae-1730-4fe9-9d4d-8b3a2c84b30d
📒 Files selected for processing (11)
frontend/PLUGIN.mdfrontend/plugins/webpack.plugin.base.tsfrontend/src/components/SyncEditor/decorate.tsfrontend/src/components/SyncEditor/statusDecorations.test.tsfrontend/src/components/SyncEditor/statusDecorations.tsfrontend/src/routes/Search/Details/DetailsOverviewPage.tsxfrontend/src/routes/Search/Details/DetailsPage.tsxfrontend/src/routes/Search/components/YamlEditor/YAMLEditor.tsxfrontend/src/routes/Search/components/YamlEditor/statusYamlDecorations.tsfrontend/src/routes/Search/components/YamlEditor/utils.tsxfrontend/src/routes/Search/searchDefinitions.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/src/routes/Search/components/YamlEditor/utils.tsx
- frontend/src/routes/Search/components/YamlEditor/YAMLEditor.tsx
- frontend/src/routes/Search/components/YamlEditor/statusYamlDecorations.ts
- frontend/src/components/SyncEditor/decorate.ts
- frontend/src/components/SyncEditor/statusDecorations.ts
- frontend/src/components/SyncEditor/statusDecorations.test.ts
Only suppress the specific Failed to construct 'URL' webpack overlay, and skip VM metrics URLs when Grafana origin parsing fails. Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Add unit tests for statusYamlDecorations and expand SyncEditor statusDecorations coverage to meet the Sonar new-code gate. Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/src/routes/Search/components/YamlEditor/statusYamlDecorations.test.ts (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the configured
~/import alias.Replace the relative import with the equivalent
~/routes/Search/components/YamlEditor/statusYamlDecorationspath.🤖 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/Search/components/YamlEditor/statusYamlDecorations.test.ts` at line 9, Update the import used by the statusYamlDecorations tests to reference statusYamlDecorations through the configured ~/routes/Search/components/YamlEditor/statusYamlDecorations alias instead of the relative path, leaving the imported symbols unchanged.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/Search/components/YamlEditor/statusYamlDecorations.test.ts`:
- Around line 154-156: Update the test’s mocked YAML model and listener setup so
the YAML content value is mutable; change that value before invoking
contentListeners[0](), then assert the subsequent deltaDecorations call contains
decorations computed with the updated status classes rather than only asserting
that it was called.
- Around line 60-63: Update the test covering the Ready condition with status
Unknown to explicitly assert that it produces no decoration, using the exact
decoration count or ranges rather than only aggregate class assertions. Locate
the relevant test in statusYamlDecorations.test.ts and preserve existing
assertions for decorated conditions.
---
Nitpick comments:
In
`@frontend/src/routes/Search/components/YamlEditor/statusYamlDecorations.test.ts`:
- Line 9: Update the import used by the statusYamlDecorations tests to reference
statusYamlDecorations through the configured
~/routes/Search/components/YamlEditor/statusYamlDecorations alias instead of the
relative path, leaving the imported symbols unchanged.
🪄 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: Pro Plus
Run ID: 0a132d44-df86-4c61-93e9-e0135bdbb4c5
📒 Files selected for processing (6)
frontend/plugins/webpack.plugin.base.tsfrontend/src/components/SyncEditor/statusDecorations.test.tsfrontend/src/components/SyncEditor/statusDecorations.tsfrontend/src/routes/Search/Details/DetailsOverviewPage.tsxfrontend/src/routes/Search/components/YamlEditor/statusYamlDecorations.test.tsfrontend/src/routes/Search/searchDefinitions.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/plugins/webpack.plugin.base.ts
- frontend/src/routes/Search/searchDefinitions.tsx
- frontend/src/routes/Search/Details/DetailsOverviewPage.tsx
- frontend/src/components/SyncEditor/statusDecorations.test.ts
- frontend/src/components/SyncEditor/statusDecorations.ts
Assert neutral Unknown conditions stay undecorated, verify refresh recomputation from mutable YAML, and use the ~/ import alias. Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
@Ginxo This looks great! I just found one issue with Placements where it is decorating a success condition as an error: 
|
Contributor
Author
Amazing catch! I'm wondering whether:
please clarify @fxiang1 ❤️ |
Contributor
@Ginxo I think to make it easier we can just consider the two scenarios:
|
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Contributor
Author
|
@fxiang1 already covered thanks for pointing to it! ❤️ |
Contributor
|
/lgtm |
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Contributor
Author
|
/unhold |
|
Contributor
|
/lgtm |
Contributor
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fxiang1, Ginxo 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 |
Contributor
Author
|
/retest |
19 tasks
Contributor
|
@Ginxo You might have to merge with the latest main branch to get the konflux EC checks to pass. |
Contributor
|
/hold - We are past feature freeze for ACM 5.0. This PR is for a story, so we should probably defer to 5.1. |
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
status.conditionswith green/red tints (success vs failure), darker emphasis onreason/message, and red highlights forunavailableReplicas > 0and failedlastState.terminated.status/ conditions for readability (type→status→reason→message; other status keys alpha thenconditions/containerStatuses).Jira: https://redhat.atlassian.net/browse/ACM-38199
Test plan
status.conditions(success + failure) and confirm green/red tints and darker reason/messagetype,status,reason,message(then others)statuskey order: other keys alphabetically, thenconditions, thencontainerStatusesunavailableReplicas: 1is tinted redcontainerStatuses[].lastStatewithterminated.reason: Erroris tinted redstatusDecorations.test.ts, SyncEditor process/decorate testsstatusConditionSuccessdecorationsReviewer note
@jswanke — Search YAML editor was included intentionally. Please confirm whether that should remain in scope for ACM-38199 or be split out.
Made with Cursor
Summary by CodeRabbit
Summary
New Features
Bug Fixes
Documentation
Tests