fix(ContainerList): group containers missing engineName #13554
Conversation
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
📝 WalkthroughWalkthroughThis PR adds engineName support and propagation across container data and group UI models. It updates ContainerInfo, ContainerGroupPartInfoUI, and ContainerGroupInfoUI to include an optional engineName. Container grouping logic in container-utils now forwards engineName for COMPOSE, POD, and STANDALONE cases, and sets engineName on aggregated group objects. Tests were updated to supply and assert engineName propagation for compose and pod scenarios. Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
packages/renderer/src/lib/container/container-utils.ts (2)
127-131: Bug: optional chaining misuse may cause runtime error in getOpeningUrlsIf Ports is undefined,
containerInfo.Ports?.filter(...)returns undefined, and the subsequent.map(...)calls will throw. Reuse getPorts to avoid this.- getOpeningUrls(containerInfo: ContainerInfo): string[] { - return containerInfo.Ports?.filter(port => port.PublicPort) - .map(port => port.PublicPort) - .map(port => `http://localhost:${port}`); - } + getOpeningUrls(containerInfo: ContainerInfo): string[] { + return this.getPorts(containerInfo) + .map(port => port.PublicPort) + .map(port => `http://localhost:${port}`); + }
215-222: Include engineId in the Standalone group for uniformityThe
ContainerGroupPartInfoUIobjects for COMPOSE and POD both includeengineId, but the STANDALONE branch does not. Downstream components (e.g.ContainerDetailsSummary.svelte) expectgroupInfo.engineId, so we should add it here.• File: packages/renderer/src/lib/container/container-utils.ts
Location: return block forContainerGroupInfoTypeUI.STANDALONE(≈ lines 215–222)return { name: this.getName(containerInfo), type: ContainerGroupInfoTypeUI.STANDALONE, status: (containerInfo.Status ?? '').toUpperCase(), engineType: containerInfo.engineType, id: containerInfo.Id, // since the container is standalone, let's use its ID as groupInfo#id + engineId: containerInfo.engineId, engineName: containerInfo.engineName, };This ensures all group types expose
engineIdconsistently and avoids undefined references in consumers.
🧹 Nitpick comments (2)
packages/renderer/src/lib/container/container-utils.spec.ts (1)
447-466: Add a standalone-group test for engineName propagationFor completeness, add a test ensuring standalone groups also carry engineName from the container. Example:
+test('standalone container group should propagate engineName', async () => { + const containerInfo = { + Id: 'standalone1', + Image: 'docker.io/library/busybox:latest', + Names: ['/standalone1'], + State: 'RUNNING', + ImageID: 'sha256:dummy-sha256', + engineId: 'podman', + engineName: 'podman', + Labels: {}, + } as unknown as ContainerInfo; + + const [containerUI] = [containerUtils.getContainerInfoUI(containerInfo)]; + const groups = containerUtils.getContainerGroups([containerUI]); + expect(groups).toHaveLength(1); + const group = groups[0]; + expect(group.type).toBe(ContainerGroupInfoTypeUI.STANDALONE); + expect(group.engineName).toBe('podman'); +});I can add this test in the right spot if you confirm the preferred location.
packages/renderer/src/lib/container/container-utils.ts (1)
100-102: Minor: simplify getPorts by removing identity map- return containerInfo.Ports?.filter(port => port.PublicPort).map(port => port) || []; + return containerInfo.Ports?.filter(port => port.PublicPort) ?? [];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/renderer/src/lib/container/container-utils.spec.ts(6 hunks)packages/renderer/src/lib/container/container-utils.ts(4 hunks)
🔇 Additional comments (7)
packages/renderer/src/lib/container/container-utils.spec.ts (4)
162-162: Compose tests: engineName added to inputs — LGTMIncluding engineName: 'podman' in both compose containers is correct and ensures the group engineName can be asserted deterministically.
Also applies to: 171-171
182-182: Compose group engineName assertion — LGTMAsserting group.engineName === 'podman' covers the regression and validates propagation.
199-199: Pod tests: engineName added to inputs — LGTMSetting engineName: 'podman' on both pod containers is appropriate for verifying group propagation.
Also applies to: 208-208
218-218: Pod group engineName assertion — LGTMAssertion confirms propagation for pods; aligns with the bug fix.
packages/renderer/src/lib/container/container-utils.ts (3)
196-197: Compose group: propagate engineName — LGTMIncluding engineName here fixes the missing environment for compose groups and matches how id is already disambiguated by engineId.
209-211: Pod group: propagate engineName — LGTMPropagation looks correct and aligns with the UI’s switch to engineName.
250-251: getContainerGroups: propagate engineName — LGTMCarrying engineName into ContainerGroupInfoUI ensures UI visibility for Pods/Compose. Good.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
packages/renderer/src/lib/container/container-utils.ts (3)
191-197: Propagate engineName with a safe fallback for COMPOSE groupsIf any source doesn’t set engineName, environment could still be empty. Use a fallback to engineType to keep UI populated.
Apply:
type: ContainerGroupInfoTypeUI.COMPOSE, engineId: containerInfo.engineId, engineType: containerInfo.engineType, - engineName: containerInfo.engineName, + engineName: containerInfo.engineName ?? containerInfo.engineType,
215-222: Standalone groups: add engineName fallbackKeeps environment info populated if engineName is missing.
Apply:
type: ContainerGroupInfoTypeUI.STANDALONE, status: (containerInfo.Status ?? '').toUpperCase(), engineType: containerInfo.engineType, id: containerInfo.Id, // since the container is standalone, let's use its ID as groupInfo#id - engineName: containerInfo.engineName, + engineName: containerInfo.engineName ?? containerInfo.engineType,
241-253: Group aggregation: handle missing engineName gracefullyOn creating a non-standalone group, fall back from
group.engineNameto the first container’sengineName, then togroup.engineType. This ensures every group has anengineNamevalue.Apply this change in
packages/renderer/src/lib/container/container-utils.ts (inside ContainerUtils.getContainerGroups):engineId: group.engineId, engineType: group.engineType, - engineName: group.engineName, + engineName: group.engineName + ?? container.engineName + ?? group.engineType, allContainersCount: 0, containers: [],–
group.engineNameremains the primary source
–container.engineName(the current ContainerInfo) covers cases where the group lacks an engineName
–group.engineType(e.g.'podman' | 'docker') is the final fallback
🧹 Nitpick comments (3)
packages/renderer/src/lib/container/container-utils.spec.ts (2)
162-163: Good: asserting engineName propagation for COMPOSE groupsThe additions validate the fix by ensuring engineName flows into group results. Consider adding a STANDALONE case to assert engineName is preserved there as well, to prevent regressions.
Also applies to: 171-172, 182-183
199-200: Good: asserting engineName propagation for POD groupsSolid coverage. Optional: add a test where two pods share the same name across different engines to ensure groups remain distinct and engineName is correct (see code comment in container-utils.ts about POD id uniqueness).
Also applies to: 208-209, 218-219
packages/renderer/src/lib/container/container-utils.ts (1)
203-211: Two tweaks for POD groups: (1) engineName fallback, (2) id uniqueness across engines
- Add a fallback for engineName (same rationale as COMPOSE).
- To avoid cross-engine collisions, consider prefixing pod id with engineId (compose already does this).
Apply:
- id: podInfo.id, + id: `${containerInfo.engineId}:${podInfo.id}`, status: (podInfo.status ?? '').toUpperCase(), engineId: containerInfo.engineId, engineType: containerInfo.engineType, - engineName: containerInfo.engineName, + engineName: containerInfo.engineName ?? containerInfo.engineType,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/renderer/src/lib/container/container-utils.spec.ts(6 hunks)packages/renderer/src/lib/container/container-utils.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit tests / macos-15
- GitHub Check: unit tests / windows-2025
- GitHub Check: smoke e2e tests (development)
- GitHub Check: smoke e2e tests (production)
|
thanks @axel7083 |
What does this PR do?
Update
ContainerUtilsto properly provide theengineNamewhen creatingContainerGroupInfoUIobjectsScreenshot / video of UI
What issues does this PR fix or reference?
Fixes #13553
How to test this PR?