feat(trust-pages): refactor trust pages and add metadata card #3
Conversation
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
Reviewer's GuideThis PR restructures the Trust Roots pages by extracting row and drawer subcomponents, centralizing the data model in a new file, and enhancing the metadata view with a new "role" column. Entity relationship diagram for TrustRootKind and related data typeserDiagram
TRUSTROOTKIND ||--o{ CERTIFICATE : has
TRUSTROOTKIND ||--o{ TUF_METADATA : has
TRUSTROOTKIND ||--o{ TRUSTROOTEVENTS : has
CERTIFICATE {
string subject
string issuer
string type
string status
string fingerprint
}
TUF_METADATA {
string role
int version
string expires
string status
}
TRUSTROOTEVENTS {
string timestamp
string type
string message
}
TRUSTROOTKIND {
string id
string name
string source
string type
string lastUpdated
}
Class diagram for the new Trust Roots data model and componentsclassDiagram
class TrustRootKind {
+string id
+string name
+string source
+string type
+string lastUpdated
+Certificate[] certificates
+TUFMetadata[] tufMetadata
+TrustRootEvents[] events
}
class Certificate {
+string subject
+string issuer
+string type
+string status
+string fingerprint
}
class TUFMetadata {
+string role
+number version
+string expires
+STATUS status
}
class TrustRootRow {
+string id
+LastStatus lastStatus
+boolean isRunning
+TrustRootKind trustRoot
}
class TrustRootsDrawerContent {
+string trustRootId
}
class TrustRootMetadataTab {
+TrustRootKind trustRoot
}
class TrustRootCertificateTab {
+TrustRootKind trustRoot
}
class TrustRootInfoTab {
+TrustRootKind trustRoot
}
TrustRootKind "1" -- "*" Certificate : contains
TrustRootKind "1" -- "*" TUFMetadata : contains
TrustRootKind "1" -- "*" TrustRootEvents : contains
TrustRootRow "1" -- "1" TrustRootKind : references
TrustRootsDrawerContent "1" -- "1" TrustRootKind : finds
TrustRootMetadataTab "1" -- "1" TrustRootKind : displays
TrustRootCertificateTab "1" -- "1" TrustRootKind : displays
TrustRootInfoTab "1" -- "1" TrustRootKind : displays
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}
There was a problem hiding this comment.
Hey @abhinandan13jan - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/app/Trust/TrustRoots/components/DraweContents/TrustRootCertificateTab.tsx:28` </location>
<code_context>
+ </Tr>
+ </Thead>
+ <Tbody>
+ {trustRoot.certificates.map((cert, idx) => (
+ <Tr key={idx}>
+ <Td>{cert.subject}</Td>
</code_context>
<issue_to_address>
No unique key for certificates.
Use a unique identifier from each certificate, such as a fingerprint, as the key instead of the array index to ensure stable React reconciliation.
Suggested implementation:
```typescript
{trustRoot.certificates.map((cert) => (
<Tr key={cert.fingerprint}>
```
This change assumes that each certificate object in `trustRoot.certificates` has a unique `fingerprint` property. If the property is named differently or not guaranteed to be unique, you should use another unique identifier available on the certificate object.
</issue_to_address>
### Comment 2
<location> `src/app/Trust/TrustRoots/components/DraweContents/TrustRootMetadataTab.tsx:28` </location>
<code_context>
+ </Tr>
+ </Thead>
+ <Tbody>
+ {trustRoot.tufMetadata.map((metadata, idx) => (
+ <Tr key={idx}>
+ <Td>{metadata.version}</Td>
</code_context>
<issue_to_address>
Array index used as key for metadata rows.
Prefer using a unique property from the metadata object, like version or a combination of role and version, as the key instead of the array index.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
{trustRoot.tufMetadata.map((metadata, idx) => (
<Tr key={idx}>
=======
{trustRoot.tufMetadata.map((metadata) => (
<Tr key={`${metadata.role}-${metadata.version}`}>
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `src/app/Trust/TrustRoots/components/StatusIcon.tsx:8` </location>
<code_context>
+import { MinusIcon } from '@patternfly/react-icons';
+
+type StatusIconProps = {
+ status: string;
+};
+
</code_context>
<issue_to_address>
StatusIcon prop type could be more specific.
Consider changing the status prop type to 'success' | 'error' | null for improved type safety and clarity.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
type StatusIconProps = {
status: string;
};
=======
type StatusIconProps = {
status: 'success' | 'error' | null;
};
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| </Tr> | ||
| </Thead> | ||
| <Tbody> | ||
| {trustRoot.certificates.map((cert, idx) => ( |
There was a problem hiding this comment.
suggestion: No unique key for certificates.
Use a unique identifier from each certificate, such as a fingerprint, as the key instead of the array index to ensure stable React reconciliation.
Suggested implementation:
{trustRoot.certificates.map((cert) => (
<Tr key={cert.fingerprint}>This change assumes that each certificate object in trustRoot.certificates has a unique fingerprint property. If the property is named differently or not guaranteed to be unique, you should use another unique identifier available on the certificate object.
Comment on lines
28
to
29
| {trustRoot.tufMetadata.map((metadata, idx) => ( | ||
| <Tr key={idx}> |
There was a problem hiding this comment.
suggestion (bug_risk): Array index used as key for metadata rows.
Prefer using a unique property from the metadata object, like version or a combination of role and version, as the key instead of the array index.
Suggested change
| {trustRoot.tufMetadata.map((metadata, idx) => ( | |
| <Tr key={idx}> | |
| {trustRoot.tufMetadata.map((metadata) => ( | |
| <Tr key={`${metadata.role}-${metadata.version}`}> |
abhinandan13jan
force-pushed
the
add-trust-colum
branch
3 times, most recently
from
2ae0068 to
1bd38ba
Compare
abhinandan13jan
force-pushed
the
add-trust-colum
branch
from
1bd38ba to
040dc83
Compare
abhinandan13jan
force-pushed
the
add-trust-colum
branch
from
040dc83 to
3f27988
Compare
abhinandan13jan
changed the title
abhinandan13jan
changed the title
abhinandan13jan
changed the title
kahboom
approved these changes
Jul 4, 2025
Collaborator
kahboom
left a comment
kahboom
left a comment
There was a problem hiding this comment.
Hey @abhinandan13jan , overall the PR looks great. Thanks for the screenshot, referencing the Jira from the PR, and linking to this from the Jira. I left a feedback point about accessing the tufMetadata array with square bracket notation, and about removing the commented code, but I wouldn't call these blocking. Thanks for this contribution!! Hope you have a nice weekend.
| } | ||
|
|
||
| const TrustRootMetadataTab: React.FC<TrustRootMetadataTabProps> = ({ trustRoot }) => { | ||
| if (!trustRoot) { |
Collaborator
There was a problem hiding this comment.
Here we should also check if tufMetadata exists and its length is greater than 0, see below.
Contributor
Author
There was a problem hiding this comment.
Yes makes sense. I updated the early exit check
| if (!trustRoot) { | ||
| return <TrustRootNotFound />; | ||
| } | ||
| const latestTUF = trustRoot?.tufMetadata[0]; |
Collaborator
There was a problem hiding this comment.
It's worth double checking that the backend will not return a trustRoot object at all if the tufMetadata array is empty. Because if it still does, and that array is empty, this will result in a type error when we try to access its properties. One way around this is to check the length of tufMetadata is greater than 0 (see above).
Contributor
Author
There was a problem hiding this comment.
updated in earlier line
|
|
||
| export default TrustRootMetadataTab; | ||
|
|
||
| // const table = <> |
Collaborator
There was a problem hiding this comment.
I think we can remove this now and add in a table in the future if customers request it
abhinandan13jan
force-pushed
the
add-trust-colum
branch
from
1f32373 to
873f73b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Issue
https://issues.redhat.com/browse/SECURESIGN-2587
Screenshot
Summary by Sourcery
Refactor trust roots pages by extracting list rows and drawer tabs into reusable components, centralizing sample data in a new model, and enhance the metadata table with a “Role” column.
New Features:
Enhancements:
Chores: