fix: read-only banner, avoid rendering it while isLoading [Backport release/0.5.z]#1139
Merged
Merged
Conversation
(cherry picked from commit 99bd439)
10 tasks
Contributor
Reviewer's GuideThis backport refactors the read-only handling to a generic "mutations disabled" concept, ensures the read-only banner is not shown while trustify info is loading, and simplifies read-only disabling by using standard isDisabled flags instead of custom helpers and tooltips across layout, buttons, and table actions. Sequence diagram for read-only banner and loading/error handling in DefaultLayoutsequenceDiagram
actor User
participant DefaultLayout
participant ReadOnlyContext
participant ReadOnlyProvider
participant useFetchTrustifyInfo
participant LoadingWrapper
participant Banner
User->>DefaultLayout: render
DefaultLayout->>ReadOnlyContext: useContext(ReadOnlyContext)
ReadOnlyContext-->>DefaultLayout: isLoading, areMutationsDisabled
DefaultLayout->>LoadingWrapper: render isFetching=isLoading
LoadingWrapper->>ReadOnlyProvider: subscribe to context
ReadOnlyProvider->>useFetchTrustifyInfo: call
useFetchTrustifyInfo-->>ReadOnlyProvider: data, isLoading, error
ReadOnlyProvider-->>ReadOnlyContext: { isLoading, areMutationsDisabled }
alt [error]
LoadingWrapper-->>Banner: render status=danger
else [no error]
opt [areMutationsDisabled && !isLoading]
DefaultLayout-->>Banner: render read-only info
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The change from
isAriaDisabled+ tooltip (READ_ONLY_TOOLTIP/readOnlyActionProps) to plainisDisabled(e.g., inReadOnlyButton,SbomToolbar, importer/advisory/SBOM tables) removes the explicit explanation and accessible affordance for why actions are disabled; consider reintroducing an accessible hint (tooltip/aria-description) so users know the instance is in read-only mode when controls are disabled. - In
useFetchTrustifyInfothe query options (staleTime, gcTime, refetch flags, retry) have been dropped; if this was not intentional, consider restoring them or documenting the new behavior, as it changes how often the trustify info is fetched and may impact perceived performance and consistency of the read-only state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The change from `isAriaDisabled` + tooltip (`READ_ONLY_TOOLTIP`/`readOnlyActionProps`) to plain `isDisabled` (e.g., in `ReadOnlyButton`, `SbomToolbar`, importer/advisory/SBOM tables) removes the explicit explanation and accessible affordance for why actions are disabled; consider reintroducing an accessible hint (tooltip/aria-description) so users know the instance is in read-only mode when controls are disabled.
- In `useFetchTrustifyInfo` the query options (staleTime, gcTime, refetch flags, retry) have been dropped; if this was not intentional, consider restoring them or documenting the new behavior, as it changes how often the trustify info is fetched and may impact perceived performance and consistency of the read-only state.
## Individual Comments
### Comment 1
<location path="client/src/app/components/ReadOnlyButton.tsx" line_range="8-10" />
<code_context>
-/** Button that is automatically aria-disabled with a tooltip when the instance is in read-only mode. */
export const ReadOnlyButton: React.FC<ButtonProps> = (props) => {
- const { isReadOnly } = React.useContext(ReadOnlyContext);
+ const { areMutationsDisabled } = React.useContext(ReadOnlyContext);
- if (isReadOnly) {
- return (
- <Tooltip content={READ_ONLY_TOOLTIP}>
- <Button {...props} isAriaDisabled />
- </Tooltip>
- );
- }
-
- return <Button {...props} />;
+ return <Button {...props} isDisabled={areMutationsDisabled} />;
};
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Switching from `isAriaDisabled` + tooltip to `isDisabled` may hurt accessibility and affordance.
With `isDisabled`, the button is no longer focusable and users lose the tooltip that explained why actions are blocked. This is an accessibility regression for keyboard and assistive tech users, and reduces UX clarity. If removing the tooltip is intentional, consider keeping `isAriaDisabled` (with an updated label/tooltip) or adding nearby explanatory text when `areMutationsDisabled` is true.
Suggested implementation:
```typescript
import { Button, Tooltip, type ButtonProps } from "@patternfly/react-core";
```
```typescript
export const ReadOnlyButton: React.FC<ButtonProps> = (props) => {
const { areMutationsDisabled } = React.useContext(ReadOnlyContext);
if (areMutationsDisabled) {
return (
<Tooltip content={READ_ONLY_TOOLTIP}>
<Button {...props} isAriaDisabled />
</Tooltip>
);
}
return <Button {...props} />;
};
```
1. Ensure that `READ_ONLY_TOOLTIP` is defined and imported in this file (for example: `import { READ_ONLY_TOOLTIP } from "./constants";`), and update its text to describe that mutations/actions are currently disabled.
2. If your project uses the new JSX runtime and doesn't globally import `React`, either add `import * as React from "react";` at the top of this file or update `React.useContext` to `useContext` with a corresponding `import { useContext } from "react";`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+8
to
+10
| const { areMutationsDisabled } = React.useContext(ReadOnlyContext); | ||
|
|
||
| if (isReadOnly) { | ||
| return ( | ||
| <Tooltip content={READ_ONLY_TOOLTIP}> | ||
| <Button {...props} isAriaDisabled /> | ||
| </Tooltip> | ||
| ); | ||
| } | ||
|
|
||
| return <Button {...props} />; | ||
| return <Button {...props} isDisabled={areMutationsDisabled} />; |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Switching from isAriaDisabled + tooltip to isDisabled may hurt accessibility and affordance.
With isDisabled, the button is no longer focusable and users lose the tooltip that explained why actions are blocked. This is an accessibility regression for keyboard and assistive tech users, and reduces UX clarity. If removing the tooltip is intentional, consider keeping isAriaDisabled (with an updated label/tooltip) or adding nearby explanatory text when areMutationsDisabled is true.
Suggested implementation:
import { Button, Tooltip, type ButtonProps } from "@patternfly/react-core";export const ReadOnlyButton: React.FC<ButtonProps> = (props) => {
const { areMutationsDisabled } = React.useContext(ReadOnlyContext);
if (areMutationsDisabled) {
return (
<Tooltip content={READ_ONLY_TOOLTIP}>
<Button {...props} isAriaDisabled />
</Tooltip>
);
}
return <Button {...props} />;
};- Ensure that
READ_ONLY_TOOLTIPis defined and imported in this file (for example:import { READ_ONLY_TOOLTIP } from "./constants";), and update its text to describe that mutations/actions are currently disabled. - If your project uses the new JSX runtime and doesn't globally import
React, either addimport * as React from "react";at the top of this file or updateReact.useContexttouseContextwith a correspondingimport { useContext } from "react";.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release/0.5.z #1139 +/- ##
=================================================
- Coverage 51.10% 51.05% -0.06%
=================================================
Files 256 256
Lines 5522 5518 -4
Branches 1672 1669 -3
=================================================
- Hits 2822 2817 -5
- Misses 2432 2434 +2
+ Partials 268 267 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Description
Backport of #1114 to
release/0.5.z.Summary by Sourcery
Adjust read-only handling to disable mutations while trustify info is loading and to avoid showing the read-only banner during loading, aligning with the updated trustify info query API.
Bug Fixes:
Enhancements:
Tests: