fix: added different key to containers when no id is found #13477
Conversation
📝 WalkthroughWalkthroughThe change updates the key used in the Svelte Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Assessment against linked issues
Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (11)
✨ 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 (
|
There was a problem hiding this comment.
Hey @MarsKubeX - I've reviewed your changes and they look great!
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/renderer/src/lib/ui/Dots.svelte (1)
22-22: Good fix for the duplicate key issue, but consider edge case handling.The fallback key
${container.Names}-${status}effectively addresses the core issue where missingcontainer.Idvalues caused duplicate keys in Svelte's#eachblocks. This should resolve the pod duplication problem described in issue #13048.However, consider adding defensive handling for edge cases:
- {#each c as container (container.Id || `${container.Names}-${status}`)} + {#each c as container (container.Id || `${container.Names || 'unknown'}-${status}-${Math.random()}`)}This ensures uniqueness even if
container.Namesis also missing or if multiple containers somehow have identical names and statuses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
-
packages/renderer/src/lib/ui/Dots.svelte(1 hunks)
🔇 Additional comments (1)
packages/renderer/src/lib/ui/Dots.svelte (1)
22-22: Fallback key is never invoked –container.Idalways truthy
The{container.Id || \${container.Names}-${status}`}fallback is unreachable because everyPodInfoContainerUIhas a non-emptyId`.Code references:
- packages/renderer/src/lib/pod/PodInfoUI.ts:
export interface PodInfoContainerUI { Id: string; … }- packages/renderer/src/lib/ui/Dots.svelte (line 22):
(container.Id || \${container.Names}-${status}`)`You can ignore the uniqueness concern for the fallback key.
Likely an incorrect or invalid review comment.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
I found another loop in |
@eqqe I'll test this separately and create another issue if it's also a bug, since it's outside the scope of this PR. |
Signed-off-by: Marcel Bertagnini <mbertagn@redhat.com>
Signed-off-by: Marcel Bertagnini <mbertagn@redhat.com>
00a9bd5 to
62b3458
Compare
What does this PR do?
This PR added a different key for "each loop" for containers in pods list when Id is not provided. This can occur when containers are in being created or in "waiting" status making duplication of pods in pods list.
What issues does this PR fix or reference?
fixes #13048
How to test this PR?