fix(preview): stop column flicker by carrying enriched columns into preview refetches#422
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
Summary by CodeRabbit
WalkthroughThis PR refactors preview carry-over into source-explicit helpers and applies them during preview resource loading to prevent column-set flapping. New helpers carryOverMetricsColumnsFrom(oldItems,newItems) and carryOverServiceEndpointColumnsFrom(oldItems,newItems) preserve enriched metric and endpoint rollup columns by copying them from a drilled-in cache entry or, if empty, from current right-side items. The integration runs before priming the item cache for Pod, Node, and Service previews. A new test suite verifies carry-over from cache, fallback behavior, endpoint preservation, cluster-isolation, and kind-mismatch skipping. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/app/cursor.go (1)
141-188:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude
ClusterNamein carry-over keys to prevent cross-cluster column bleed.Line 153 and Line 218 key by
namespace+nameonly. In union mode, identical resources across clusters collide, so one cluster’s carried columns can overwrite another’s. UseClusterName+namespace+namein both helpers.Proposed fix
-func carryOverMetricsColumnsFrom(oldItems, newItems []model.Item) { +func carryOverMetricsColumnsFrom(oldItems, newItems []model.Item) { @@ - type itemKey struct{ ns, name string } + type itemKey struct{ cluster, ns, name string } @@ - oldMetrics[itemKey{item.Namespace, item.Name}] = cols + oldMetrics[itemKey{item.ClusterName, item.Namespace, item.Name}] = cols @@ - key := itemKey{newItems[i].Namespace, newItems[i].Name} + key := itemKey{newItems[i].ClusterName, newItems[i].Namespace, newItems[i].Name} @@ -func carryOverServiceEndpointColumnsFrom(oldItems, newItems []model.Item) { +func carryOverServiceEndpointColumnsFrom(oldItems, newItems []model.Item) { @@ - type itemKey struct{ ns, name string } + type itemKey struct{ cluster, ns, name string } @@ - old[itemKey{item.Namespace, item.Name}] = cols + old[itemKey{item.ClusterName, item.Namespace, item.Name}] = cols @@ - key := itemKey{newItems[i].Namespace, newItems[i].Name} + key := itemKey{newItems[i].ClusterName, newItems[i].Namespace, newItems[i].Name}Also applies to: 213-254
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/app/cursor.go` around lines 141 - 188, The carry-over mapping keys only use namespace+name which causes cross-cluster collisions; update the itemKey struct used in carryOverMetricsColumnsFrom (and the other carry-over helper later in the file) to include ClusterName and populate it from the model.Item (e.g. item.ClusterName) when building oldMetrics and when computing key for newItems (use the same ClusterName+Namespace+Name tuple), and make the analogous change in the other carryover function that currently keys by namespace+name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/app/cursor.go`:
- Around line 141-188: The carry-over mapping keys only use namespace+name which
causes cross-cluster collisions; update the itemKey struct used in
carryOverMetricsColumnsFrom (and the other carry-over helper later in the file)
to include ClusterName and populate it from the model.Item (e.g.
item.ClusterName) when building oldMetrics and when computing key for newItems
(use the same ClusterName+Namespace+Name tuple), and make the analogous change
in the other carryover function that currently keys by namespace+name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ff26af8d-4cb5-4db9-a209-8b8b9da16738
📒 Files selected for processing (3)
internal/app/cursor.gointernal/app/preview_carryover_test.gointernal/app/update_resources_loaded_preview.go
db07496 to
3d83496
Compare
The metrics and service-endpoint carry-over helpers keyed old-item columns by namespace+name only. In union mode the same namespace+name can exist in several clusters, so one cluster's carried columns could overwrite another's. Key both lookups by cluster+namespace+name. Addresses CodeRabbit review on PR #422.
…review refetches Follow-up to #408 (#410): the sort is stable now, but the preview's column set flapped — 'several surprise columns display and then disappear'. Preview fetches return unenriched items: no CPU/MEM metrics columns (the drilled list merges those asynchronously via the pod/node list metrics ticks) and no Service endpoint rollup. The rt-level watch tick drops the preview cache fingerprint every interval, so each hover refetch replaced enriched items with unenriched ones, and the auto-detected extras flipped (NODE/QoS/IP appear when CPU/MEM vanish, and back). The preview cache prime also overwrote the drilled list's enriched itemCache entry with unenriched data, so the next drill-in flashed the same way. Mirror the main list's anti-flash mechanism in the preview handler: extract carryOverMetricsColumnsFrom / carryOverServiceEndpointColumnsFrom (source-explicit cores of the existing middleItems-based helpers) and carry enriched columns from the drill-in cache (fallback: the currently shown preview items of the same kind) into fresh Pod/Node/Service preview loads, before the cache prime so the prime keeps the enrichment.
The metrics and service-endpoint carry-over helpers keyed old-item columns by namespace+name only. In union mode the same namespace+name can exist in several clusters, so one cluster's carried columns could overwrite another's. Key both lookups by cluster+namespace+name. Addresses CodeRabbit review on PR #422.
f916464 to
71d2331
Compare
Fixes the latest comment on #408: "The sort order seems stable now but the columns are not - several surprise columns display in the preview and then after a moment disappear."
Root cause
Preview fetches return unenriched items — no CPU/MEM metrics columns (the drilled list merges those asynchronously via the pod/node list-metrics ticks) and no Service endpoint rollup. Two consequences:
itemCacheentry with unenriched data, so the next drill-in flashed the same way.Fix
Mirror the main list's existing anti-flash mechanism in the preview handler:
carryOverMetricsColumnsFrom/carryOverServiceEndpointColumnsFrom— source-explicit cores of the existingmiddleItems-based helpers.updateResourcesLoadedPreview, carry enriched columns from the drill-in cache (fallback: the currently shown preview items of the same kind) into fresh Pod/Node/Service preview loads, before the cache prime so the prime keeps the enrichment instead of clobbering it.Test plan
go test -race,go vet,golangci-lint(0 issues),gofumptclean