feat(containers): add native sidebar and detail tabs support#304
Open
frruit wants to merge 1 commit into
Open
Conversation
frruit
commented
May 21, 2026
frruit
commented
- add read-only container repository sidebar grouped by local/remote
- add native container detail tabs: Details, Versions, Distributions
- show version-level pull references with tag and digest
- support href-based loading for container-push repositories
- keep original container files untouched by using native variant files
- add read-only container repository sidebar grouped by local/remote - add native container detail tabs: Details, Versions, Distributions - show version-level pull references with tag and digest - support href-based loading for container-push repositories - keep original container files untouched by using native variant files
Author
|
@sourcery-ai review |
Reviewer's GuideIntroduces a native container repository experience with a new list + sidebar layout, a tabbed detail view (Details/Versions/Distributions), and native API clients for container repositories/remotes and repository versions, including version-level pull reference display. Sequence diagram for loading container detail and version pull referencessequenceDiagram
actor User
participant ExecutionEnvironmentDetail
participant ContainerDistributionAPI
participant ContainerRepositoryNativeAPI
participant ContainerRemoteNativeAPI
participant RepositoryVersionsTab
participant GenericPulpAPI
User->>ExecutionEnvironmentDetail: navigate to repository.detail
ExecutionEnvironmentDetail->>ContainerDistributionAPI: list({ base_path })
ContainerDistributionAPI-->>ExecutionEnvironmentDetail: distribution
alt distribution.repository exists
ExecutionEnvironmentDetail->>ContainerRepositoryNativeAPI: getByHref(distribution.repository)
ContainerRepositoryNativeAPI-->>ExecutionEnvironmentDetail: ContainerRepositoryNativeType
end
alt repository.remote or distribution.remote exists
ExecutionEnvironmentDetail->>ContainerRemoteNativeAPI: get(id from remote href)
ContainerRemoteNativeAPI-->>ExecutionEnvironmentDetail: ContainerRemoteNativeType
end
ExecutionEnvironmentDetail-->>User: render tabs (Details / Versions / Distributions)
User->>RepositoryVersionsTab: open Versions tab
RepositoryVersionsTab->>ContainerRepositoryNativeAPI: listVersionsByHref(repository.pulp_href, { number })
ContainerRepositoryNativeAPI-->>RepositoryVersionsTab: ContainerRepositoryVersionType
RepositoryVersionsTab->>GenericPulpAPI: list('content/container/tags/', { repository_version })
GenericPulpAPI-->>RepositoryVersionsTab: Tag list
RepositoryVersionsTab->>GenericPulpAPI: list('content/container/manifests/', { repository_version })
GenericPulpAPI-->>RepositoryVersionsTab: Manifest list
RepositoryVersionsTab-->>User: show version details + pull references (tag & digest)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
SourceryAI
reviewed
May 22, 2026
SourceryAI
left a comment
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In the new native execution environment list and registry list components, state and helper methods for edit/delete/sync/index (e.g.
showDeleteModal,selectedItem,syncRegistry,indexRegistry) exist butListItemActionsis rendered with emptykebabItems/buttons, so these actions are effectively unreachable; either wire up the actions or remove the unused state/methods to avoid dead code. - The
ContainerRepositorySidebarfetches a single page withpage_size: 200and does not paginate, which will silently drop repositories beyond that limit; if large installations are expected, consider adding pagination or a full list fetch pattern that scales beyond the first 200 items.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new native execution environment list and registry list components, state and helper methods for edit/delete/sync/index (e.g. `showDeleteModal`, `selectedItem`, `syncRegistry`, `indexRegistry`) exist but `ListItemActions` is rendered with empty `kebabItems`/`buttons`, so these actions are effectively unreachable; either wire up the actions or remove the unused state/methods to avoid dead code.
- The `ContainerRepositorySidebar` fetches a single page with `page_size: 200` and does not paginate, which will silently drop repositories beyond that limit; if large installations are expected, consider adding pagination or a full list fetch pattern that scales beyond the first 200 items.
## Individual Comments
### Comment 1
<location path="src/containers/execution-environment/registry-list-native.tsx" line_range="376" />
<code_context>
+ <Td>
+ <CopyURL url={item.url} />
+ </Td>
+ <ListItemActions kebabItems={[]} buttons={[]} />
+ </Tr>
+ );
</code_context>
<issue_to_address>
**issue (bug_risk):** Table row includes a non-<Td>/<Th> child, which can break table layout/accessibility.
Because PatternFly tables require each direct `<Tr>` child to be a `<Td>` or `<Th>`, rendering `<ListItemActions>` directly under `<Tr>` can break layout and accessibility. Please wrap it in a cell, for example:
```tsx
<Td isActionCell>
<ListItemActions kebabItems={[]} buttons={[]} />
</Td>
```
</issue_to_address>
### Comment 2
<location path="src/containers/execution-environment-list/execution-environment-list-native.tsx" line_range="353" />
<code_context>
+ <Td>
+ <Label>{item.remote ? t`Remote` : t`Local`}</Label>
+ </Td>
+ <ListItemActions kebabItems={[]} />
+ </Tr>
+ );
</code_context>
<issue_to_address>
**issue (bug_risk):** ListItemActions should be wrapped in a table cell instead of being a direct child of <Tr>.
Rendering `<ListItemActions>` directly under `<Tr>` creates invalid table markup. It should be wrapped in a `<Td>` (usually with `isActionCell`), for example:
```tsx
<Td isActionCell>
<ListItemActions kebabItems={[]} />
</Td>
```
</issue_to_address>
### Comment 3
<location path="src/containers/execution-environment-list/execution-environment-list-native.tsx" line_range="359-368" />
<code_context>
+ }
+
+ private queryEnvironments() {
+ this.setState({ loading: true }, () =>
+ ContainerDistributionAPI.list(this.state.params)
+ .then((result) => {
+ this.setState({
+ items: result.data.results,
+ itemCount: result.data.count,
+ loading: false,
+ });
+ })
+ .catch((e) =>
+ this.addAlert(t`Error loading environments.`, 'danger', e?.message),
+ ),
</code_context>
<issue_to_address>
**issue (bug_risk):** Loading state is never cleared on error when querying environments.
In `queryEnvironments`, `loading` is set to `true` but never reset in the `catch` block, so a failed `ContainerDistributionAPI.list` call leaves the UI stuck in a loading state. You should also set `loading: false` in the error path, e.g.:
```tsx
.catch((e) => {
this.setState({ loading: false });
this.addAlert(t`Error loading environments.`, 'danger', e?.message);
})
```
</issue_to_address>
### Comment 4
<location path="src/containers/execution-environment/registry-list-native.tsx" line_range="406-407" />
<code_context>
+ 'success',
+ ),
+ )
+ .catch((err) => {
+ const { status, statusText } = err.response;
+ this.addAlert(
+ t`Remote registry "${name}" could not be deleted.`,
</code_context>
<issue_to_address>
**issue (bug_risk):** Error handling assumes `err.response` exists, which can throw on network/other errors.
In `deleteRegistry` (and similarly `syncRegistry`/`indexRegistry`), `err.response` is destructured without a null check, so network or other non-HTTP errors (where `err.response` is undefined) will throw a new error and hide the original. Please guard access to `status`/`statusText`, e.g. via optional chaining, before calling `addAlert`.
</issue_to_address>Hi @frruit! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.| <Td> | ||
| <CopyURL url={item.url} /> | ||
| </Td> | ||
| <ListItemActions kebabItems={[]} buttons={[]} /> |
There was a problem hiding this comment.
issue (bug_risk): Table row includes a non-/ child, which can break table layout/accessibility.
Because PatternFly tables require each direct <Tr> child to be a <Td> or <Th>, rendering <ListItemActions> directly under <Tr> can break layout and accessibility. Please wrap it in a cell, for example:
<Td isActionCell>
<ListItemActions kebabItems={[]} buttons={[]} />
</Td>| <Td> | ||
| <Label>{item.remote ? t`Remote` : t`Local`}</Label> | ||
| </Td> | ||
| <ListItemActions kebabItems={[]} /> |
There was a problem hiding this comment.
issue (bug_risk): ListItemActions should be wrapped in a table cell instead of being a direct child of .
Rendering <ListItemActions> directly under <Tr> creates invalid table markup. It should be wrapped in a <Td> (usually with isActionCell), for example:
<Td isActionCell>
<ListItemActions kebabItems={[]} />
</Td>
Comment on lines
+359
to
+368
| this.setState({ loading: true }, () => | ||
| ContainerDistributionAPI.list(this.state.params) | ||
| .then((result) => { | ||
| this.setState({ | ||
| items: result.data.results, | ||
| itemCount: result.data.count, | ||
| loading: false, | ||
| }); | ||
| }) | ||
| .catch((e) => |
There was a problem hiding this comment.
issue (bug_risk): Loading state is never cleared on error when querying environments.
In queryEnvironments, loading is set to true but never reset in the catch block, so a failed ContainerDistributionAPI.list call leaves the UI stuck in a loading state. You should also set loading: false in the error path, e.g.:
.catch((e) => {
this.setState({ loading: false });
this.addAlert(t`Error loading environments.`, 'danger', e?.message);
})
Comment on lines
+406
to
+407
| .catch((err) => { | ||
| const { status, statusText } = err.response; |
There was a problem hiding this comment.
issue (bug_risk): Error handling assumes err.response exists, which can throw on network/other errors.
In deleteRegistry (and similarly syncRegistry/indexRegistry), err.response is destructured without a null check, so network or other non-HTTP errors (where err.response is undefined) will throw a new error and hide the original. Please guard access to status/statusText, e.g. via optional chaining, before calling addAlert.
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
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.