feat(seedstats): add seed shoot capacity and health stats with live updates#2901
Open
petersutter wants to merge 9 commits into
Open
feat(seedstats): add seed shoot capacity and health stats with live updates#2901petersutter wants to merge 9 commits into
petersutter wants to merge 9 commits into
Conversation
petersutter
requested review from
grolu,
holgerkoser and
klocke-io
as code owners
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
gardener-prow
Bot
added
size/XXL
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a SeedStat feature: backend service, routes, cache indices, Socket.IO seedstats rooms and emissions; frontend API, store, composables, components, views, and many tests; also ticket cache indexing, startup hooks, and minor infra/.gitignore updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant API
participant SeedService
participant Cache
participant TicketsCache
Browser->>API: GET /api/seedstats?unhealthyFilterMask=...
API->>SeedService: list({ user, unhealthyFilterMask })
SeedService->>Cache: getSeeds(), getShootsBySeedName(seedName)
Cache-->>SeedService: seed & shoot data (indexed)
SeedService->>TicketsCache: getIssuesForShoot(projectName,name) [as needed]
TicketsCache-->>SeedService: issue labels
SeedService-->>API: SeedStat objects
API-->>Browser: 200 + SeedStat payloads
sequenceDiagram
participant Watcher
participant Cache
participant SocketNsp as Socket.IO_Nsp
participant Rooms
participant Subscribers
Watcher->>Cache: resolve seedName -> seedUid
Cache-->>Watcher: seedUid
Watcher->>SocketNsp: getJoinedRooms(seedName)
SocketNsp-->>Watcher: matching seedstats rooms
Watcher->>Rooms: emit 'seedstats' { type:'MODIFIED', uid }
Rooms-->>Subscribers: receive seedstats update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
backend/lib/io/shoots.js (1)
39-42: Log message may be misleading when joining multiple rooms.The
joinRoomfunction can be called with an array of rooms (line 63), but the log message uses singular "room". Consider adjusting the message or logging each room individually for clarity.💡 Suggested fix
const joinRoom = room => { - logger.debug('User %s joined room [%s]', user.id, room) + logger.debug('User %s joined room(s) [%s]', user.id, room) return socket.join(room) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/io/shoots.js` around lines 39 - 42, joinRoom's log uses singular "room" but joinRoom(room) can receive an array; update joinRoom to detect Array.isArray(room) and either log each room (e.g., iterate and call logger.debug per room with user.id) or log a pluralized message with the rooms serialized, then call socket.join(room) as before; change references in the joinRoom function (logger.debug, socket.join, and user.id) so the log accurately reflects single or multiple rooms.backend/lib/cache/index.js (1)
97-99: Consider returning a consistent type fromgetShootsBySeedName.The method returns a
MapIteratorwhen shoots exist but an empty array[]when none exist. While both are iterable and the current code handles this (viafor...ofand explicitArray.from()conversions), the inconsistent return type could cause confusion for future maintainers or callers.💡 Suggested fix for consistent return type
getShootsBySeedName (seedName) { - return this.#seedNameToShoots.get(seedName)?.values() ?? [] + const shoots = this.#seedNameToShoots.get(seedName) + return shoots ? [...shoots.values()] : [] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/cache/index.js` around lines 97 - 99, getShootsBySeedName currently returns a MapIterator when entries exist and an array when none exist; change it to always return the same type (an array) by converting the MapIterator from this.#seedNameToShoots.get(seedName)?.values() to an array (e.g. via Array.from or spread) and return an empty array when there is no entry so callers of getShootsBySeedName always receive an Array of shoots.backend/lib/watches/seeds.js (1)
25-25: Drop the unusedoldObjectargument in the update subscription.
handleEventnow accepts only(type, newObject), so the extra parameter in the update listener can be removed for clarity.✂️ Small cleanup
-informer.on('update', (object, oldObject) => handleEvent('MODIFIED', object, oldObject)) +informer.on('update', object => handleEvent('MODIFIED', object))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/watches/seeds.js` at line 25, The update listener is passing an unused oldObject to handleEvent; update the informer.on('update', ...) subscription so it calls handleEvent('MODIFIED', object) with only the new object, removing the extra oldObject parameter; locate the informer.on('update', (object, oldObject) => handleEvent('MODIFIED', object, oldObject)) line and change the callback to call handleEvent with only the type and newObject to match handleEvent(type, newObject).frontend/src/store/seedStat.js (1)
82-98: Consider error handling insubscribewhen closing the previous subscription.If
closeSubscription()throws (e.g., socket timeout), the function will propagate the error and the new subscription won't be established. Depending on desired behavior, you may want to catch and log this error, then proceed with the new subscription.🛡️ Optional: graceful handling of close errors
async function subscribe (options = {}) { const sameSubscription = isEqual(subscription.value, options) if (sameSubscription && list.value !== null) { if (!subscribed.value && socketStore.connected) { await openSubscription(options) } return } if (subscribed.value || subscription.value) { - await closeSubscription() + try { + await closeSubscription() + } catch (err) { + logger.debug('Failed to close previous subscription: %s', err.message) + } } subscription.value = options await fetchSeedStats(options) await openSubscription(options) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/store/seedStat.js` around lines 82 - 98, The subscribe function currently propagates errors from closeSubscription and can block establishing a new subscription; wrap the await closeSubscription() call in a try/catch inside subscribe (referencing subscribe, closeSubscription, openSubscription, fetchSeedStats, subscription.value and subscribed.value), log or report the caught error via your logger/console, and then continue to set subscription.value, call fetchSeedStats(options) and openSubscription(options) so a failing close does not prevent the new subscription from being created.frontend/src/components/GShootHealthDonut.vue (1)
167-168: Consider clamping computed values to prevent negative segment values.If upstream data is inconsistent (e.g.,
totalUnhealthyShoots > shootCount),healthyShootscould become negative, potentially causing unexpected SVG rendering behavior.🛡️ Defensive clamping
-const hiddenUnhealthy = computed(() => totalUnhealthy.value - matchingUnhealthy.value) -const healthyShoots = computed(() => shootCount.value - totalUnhealthy.value) +const hiddenUnhealthy = computed(() => Math.max(0, totalUnhealthy.value - matchingUnhealthy.value)) +const healthyShoots = computed(() => Math.max(0, shootCount.value - totalUnhealthy.value))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/GShootHealthDonut.vue` around lines 167 - 168, Clamp computed segment values to a non-negative range to avoid negative SVG segments: in GShootHealthDonut.vue update the computed properties hiddenUnhealthy and healthyShoots to ensure they never go below zero (use shootCount, totalUnhealthy, matchingUnhealthy as inputs), e.g., compute hiddenUnhealthy as max(0, totalUnhealthy - matchingUnhealthy) and healthyShoots as max(0, shootCount - totalUnhealthy) so rendering always receives non-negative segment sizes.backend/__tests__/acceptance/api.seedstats.spec.js (1)
27-38: Cache index cleanup is optional and may not be necessary.The
seedShootsBySeedNameIndexfunction populates the cache but has no explicit cleanup. However, this same pattern exists in other acceptance tests (e.g.,io.spec.js) without reported issues, and no reset function has been created for this index elsewhere in the codebase. If test isolation becomes a concern, consider adding cleanup similar tocache.cache.resetTicketCache()called in other test files, but it is not required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/__tests__/acceptance/api.seedstats.spec.js` around lines 27 - 38, The test helper seedShootsBySeedNameIndex populates the cache via cache.indexShootsBySeedName but does not clean up the index afterward; to preserve test isolation add an explicit reset at the end of the helper (or provide a paired teardown) that calls the cache reset method used elsewhere (e.g., cache.cache.resetTicketCache or an equivalent reset function) so the index added by seedShootsBySeedNameIndex is cleared between tests; update the seedShootsBySeedNameIndex function to register or invoke that reset/cleanup after adding shoots.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/__tests__/hooks.spec.js`:
- Around line 114-117: The test for beforeListen stubs/asserts
cache.indexProjectsByNamespace but misses the new derived indexer
cache.indexShootsBySeedName; update the spec to mock cache.indexShootsBySeedName
(e.g., vi.fn()) alongside cache.indexProjectsByNamespace before calling
beforeListen, and add an assertion that cache.indexShootsBySeedName was called
(or calledWith expected args) after beforeListen returns; modify both
occurrences (around the existing stubs at the top and the second block around
lines 136-138) to keep the test isolated and prevent regressions.
In `@backend/lib/io/index.js`:
- Line 55: Wrap the async io.on('connection', async socket => { ... }) handler's
awaited room-join operations (notably helper.joinPrivateRoom(socket) and
subsequent socket.join(...) calls) in a try-catch block; on catch, log/emitthe
error and fail the socket deterministically (e.g., socket.emit('error',
err.message) and socket.disconnect(true) or call the same error handler pattern
used in 'subscribe'/'unsubscribe'/'synchronize') so unhandled promise rejections
are prevented and join failures are handled cleanly.
In `@backend/lib/io/seedstats.js`:
- Around line 79-90: getJoinedRooms currently filters out per-seed rooms when no
seedName is passed because appliesToSeed is written as ({ seedName: roomSeedName
}) => !roomSeedName || roomSeedName === seedName; change the predicate so that
when the caller does not provide seedName all rooms (global and all per-seed)
are returned, and when seedName is provided only global rooms and rooms matching
that seed are returned; update appliesToSeed in getJoinedRooms to something
like: return true for !roomSeedName OR seedName is undefined/null OR
roomSeedName === seedName (i.e. use the condition !roomSeedName || seedName ==
null || roomSeedName === seedName), keeping references to isSeedStatsRoom and
parseRoomName unchanged.
In `@backend/lib/services/seedstats.js`:
- Around line 234-255: The current getTicketsForShoot repeatedly scans
cache.getTicketCache().getIssues(), causing O(shoots×issues) work; instead build
a one-time index of issues by projectName+shootName for the current seedstats
refresh and use that index in getTicketsForShoot (or add a memoized lookup keyed
by shoot). Concretely: when handling the seedstats refresh (the function that
calls getTicketsForShoot/shouldHideShoot), iterate once over
cache.getTicketCache().getIssues() and populate a Map keyed by
`${issue.metadata.projectName}/${issue.metadata.name}` (or a nested Map by
projectName then name), then change getTicketsForShoot to read from that Map
(using getProjectNameForShoot(shoot) and shoot.metadata.name) and return [] if
missing; this avoids repeated full-array filters.
In `@frontend/src/components/SeedDetails/GSeedInfrastructureCard.vue`:
- Around line 155-175: The subscribe/unsubscribe calls can race; serialize them
by introducing a shared operation chain (e.g., a local Promise/lock) used by the
watcher and onUnmounted so each operation awaits the previous one before calling
seedStatStore.subscribe or seedStatStore.unsubscribe; specifically, add a
module-scoped variable like lastOp: Promise<void> = Promise.resolve(), wrap any
subscribe/unsubscribe calls in an async function that does `await lastOp` then
performs the async store call and assigns its returned promise to lastOp (and
still catch and forward errors to appStore.setError), and use that same wrapper
from the watch callback and onUnmounted so no subscribe can complete
out-of-order after a later unsubscribe.
In `@frontend/src/composables/useSeedItem/index.js`:
- Around line 257-259: The computed getters seedShootCount,
seedTotalUnhealthyShoots and seedUnhealthyShoots currently coerce missing data
into 0 (using ?? 0), which hides loading/unsubscribed/error states; change each
computed to return undefined when seedStat.value or the nested counts are
missing (e.g. remove the "?? 0" and rely on seedStat.value?.counts?.… returning
undefined) so consumers can detect undefined and render placeholders/skeletons
instead.
In `@frontend/src/composables/useShootListFilters.js`:
- Around line 16-20: The filter label for the exclusion rule is misleading:
update the FILTER_LABELS entry with key 'noOperatorAction' to a label that
matches the actual exclusion rule (e.g., "No operator action required" or
similar) so active filter chips and the donut tooltip correctly describe
excluded shoots; modify the label string in the FILTER_LABELS array where the
object has key 'noOperatorAction' to the chosen, accurate wording.
---
Nitpick comments:
In `@backend/__tests__/acceptance/api.seedstats.spec.js`:
- Around line 27-38: The test helper seedShootsBySeedNameIndex populates the
cache via cache.indexShootsBySeedName but does not clean up the index afterward;
to preserve test isolation add an explicit reset at the end of the helper (or
provide a paired teardown) that calls the cache reset method used elsewhere
(e.g., cache.cache.resetTicketCache or an equivalent reset function) so the
index added by seedShootsBySeedNameIndex is cleared between tests; update the
seedShootsBySeedNameIndex function to register or invoke that reset/cleanup
after adding shoots.
In `@backend/lib/cache/index.js`:
- Around line 97-99: getShootsBySeedName currently returns a MapIterator when
entries exist and an array when none exist; change it to always return the same
type (an array) by converting the MapIterator from
this.#seedNameToShoots.get(seedName)?.values() to an array (e.g. via Array.from
or spread) and return an empty array when there is no entry so callers of
getShootsBySeedName always receive an Array of shoots.
In `@backend/lib/io/shoots.js`:
- Around line 39-42: joinRoom's log uses singular "room" but joinRoom(room) can
receive an array; update joinRoom to detect Array.isArray(room) and either log
each room (e.g., iterate and call logger.debug per room with user.id) or log a
pluralized message with the rooms serialized, then call socket.join(room) as
before; change references in the joinRoom function (logger.debug, socket.join,
and user.id) so the log accurately reflects single or multiple rooms.
In `@backend/lib/watches/seeds.js`:
- Line 25: The update listener is passing an unused oldObject to handleEvent;
update the informer.on('update', ...) subscription so it calls
handleEvent('MODIFIED', object) with only the new object, removing the extra
oldObject parameter; locate the informer.on('update', (object, oldObject) =>
handleEvent('MODIFIED', object, oldObject)) line and change the callback to call
handleEvent with only the type and newObject to match handleEvent(type,
newObject).
In `@frontend/src/components/GShootHealthDonut.vue`:
- Around line 167-168: Clamp computed segment values to a non-negative range to
avoid negative SVG segments: in GShootHealthDonut.vue update the computed
properties hiddenUnhealthy and healthyShoots to ensure they never go below zero
(use shootCount, totalUnhealthy, matchingUnhealthy as inputs), e.g., compute
hiddenUnhealthy as max(0, totalUnhealthy - matchingUnhealthy) and healthyShoots
as max(0, shootCount - totalUnhealthy) so rendering always receives non-negative
segment sizes.
In `@frontend/src/store/seedStat.js`:
- Around line 82-98: The subscribe function currently propagates errors from
closeSubscription and can block establishing a new subscription; wrap the await
closeSubscription() call in a try/catch inside subscribe (referencing subscribe,
closeSubscription, openSubscription, fetchSeedStats, subscription.value and
subscribed.value), log or report the caught error via your logger/console, and
then continue to set subscription.value, call fetchSeedStats(options) and
openSubscription(options) so a failing close does not prevent the new
subscription from being created.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a2b717d-8bb3-4de8-95dd-2550ae90b3d7
📒 Files selected for processing (61)
.gitignorebackend/__tests__/acceptance/api.members.spec.jsbackend/__tests__/acceptance/api.seedstats.spec.jsbackend/__tests__/acceptance/api.terminals.spec.jsbackend/__tests__/acceptance/api.tickets.spec.jsbackend/__tests__/acceptance/io.cors.spec.jsbackend/__tests__/acceptance/io.spec.jsbackend/__tests__/cache.spec.jsbackend/__tests__/helpers/cache.jsbackend/__tests__/hooks.spec.jsbackend/__tests__/io.seedstats.spec.jsbackend/__tests__/services.seedstats.spec.jsbackend/__tests__/services.terminals.spec.jsbackend/__tests__/utils.spec.jsbackend/__tests__/watches.spec.jsbackend/lib/cache/index.jsbackend/lib/hooks.jsbackend/lib/io/dispatcher.jsbackend/lib/io/helper.jsbackend/lib/io/index.jsbackend/lib/io/seedstats.jsbackend/lib/io/shoots.jsbackend/lib/routes/index.jsbackend/lib/routes/seedstats.jsbackend/lib/services/index.jsbackend/lib/services/seedstats.jsbackend/lib/utils/index.jsbackend/lib/watches/leases.jsbackend/lib/watches/seeds.jsbackend/lib/watches/shoots.jsfrontend/__tests__/components/GSeedInfrastructureCard.spec.jsfrontend/__tests__/components/GShootHealthDonut.spec.jsfrontend/__tests__/composables/useDonutChart.spec.jsfrontend/__tests__/composables/useSeedTableSorting.spec.jsfrontend/__tests__/composables/useShootItem.spec.jsfrontend/__tests__/composables/useShootListFilters.spec.jsfrontend/__tests__/stores/seedStat.spec.jsfrontend/__tests__/stores/shoot.spec.jsfrontend/src/components/GSeedListRow.vuefrontend/src/components/GShootHealthDonut.vuefrontend/src/components/SeedDetails/GSeedInfrastructureCard.vuefrontend/src/components/Seeds/GSeedCapacityIndicator.vuefrontend/src/composables/useApi/api.jsfrontend/src/composables/useDonutChart.jsfrontend/src/composables/useSeedItem/index.jsfrontend/src/composables/useSeedTableSorting.jsfrontend/src/composables/useShootListFilters.jsfrontend/src/composables/useSocketEventHandler.jsfrontend/src/router/guards.jsfrontend/src/store/helper.jsfrontend/src/store/localStorage.jsfrontend/src/store/seedStat.jsfrontend/src/store/shoot/helper.jsfrontend/src/store/shoot/shoot.jsfrontend/src/store/socket/helper.jsfrontend/src/store/socket/index.jsfrontend/src/utils/errorCodes.jsfrontend/src/utils/index.jsfrontend/src/views/GAdministration.vuefrontend/src/views/GSeedList.vuefrontend/src/views/GShootList.vue
💤 Files with no reviewable changes (3)
- frontend/tests/stores/shoot.spec.js
- frontend/src/router/guards.js
- frontend/src/store/localStorage.js
petersutter
force-pushed
the
enh/seed-stats-v2
branch
from
2ca3a8a to
85d3e37
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
frontend/src/components/GShootHealthDonut.vue (2)
218-227: Nit: dead branch informatCompact.For
valuein[1000, 10000),v = Math.floor(value / 100) / 10is always< 10(sincevalue/100 < 100), sov < 10 ? 1 : 0is effectively constant1. You can drop the ternary and always usetoFixed(1).♻️ Suggested simplification
if (value < 10000) { - const v = Math.floor(value / 100) / 10 - return `${v.toFixed(v < 10 ? 1 : 0)}k` + const v = Math.floor(value / 100) / 10 + return `${v.toFixed(1)}k` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/GShootHealthDonut.vue` around lines 218 - 227, The formatCompact function contains a dead branch: in the 1000–9999 range v = Math.floor(value / 100) / 10 is always < 10, so the ternary v < 10 ? 1 : 0 is redundant; update the formatCompact implementation (the function named formatCompact) to always use toFixed(1) for that branch (and you can optionally simplify the v calculation or the return to a single toFixed(1) call) so the ternary is removed and the code is clearer.
167-168: Optional: guard against transient negative derived counts.If backend updates briefly report
totalUnhealthyShoots > shootCountormatchingUnhealthyShoots > totalUnhealthyShoots(e.g., during a room re-subscription race),healthyShoots/hiddenUnhealthycould become negative and feed into the donut geometry and tooltip. AMath.max(0, …)clamp would make the component resilient to such transient inconsistencies without affecting normal behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/GShootHealthDonut.vue` around lines 167 - 168, Clamp derived counts to zero to avoid transient negatives: update the computed properties hiddenUnhealthy and healthyShoots so their returned values use Math.max(0, ...). Specifically, change hiddenUnhealthy (which currently computes totalUnhealthy.value - matchingUnhealthy.value) to return Math.max(0, totalUnhealthy.value - matchingUnhealthy.value) and change healthyShoots (which currently computes shootCount.value - totalUnhealthy.value) to return Math.max(0, shootCount.value - totalUnhealthy.value); keep the same reactive/computed wrappers and names.backend/lib/services/seedstats.js (1)
164-184: Minor: empty string silently parses to mask0.
Number('')returns0, so an explicit emptyunhealthyFilterMaskquery/option parses as "no filters" rather than being rejected. If you want to treat a missing value and an explicit empty value the same (both reject), consider requiring a non-empty string before theNumber()conversion. If the permissive behavior is intentional, ignore.♻️ Suggested tweak
if (typeof unhealthyFilterMask === 'string') { + if (unhealthyFilterMask.trim() === '') { + throw invalidUnhealthyFilterMask() + } unhealthyFilterMask = Number(unhealthyFilterMask) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/services/seedstats.js` around lines 164 - 184, The parseUnhealthyFilterMask currently treats an explicit empty string as 0 because Number('') === 0; update parseUnhealthyFilterMask to reject empty strings before converting (e.g., if typeof unhealthyFilterMask === 'string' && unhealthyFilterMask.trim() === '' then throw invalidUnhealthyFilterMask()), then proceed to Number(...) and validate with isValidUnhealthyFilterMask; reference functions: parseUnhealthyFilterMask, isValidUnhealthyFilterMask and the error helper invalidUnhealthyFilterMask and constant ALL_UNHEALTHY_FILTER_FLAGS.backend/lib/watches/shoots.js (1)
70-90: Perf:cache.getSeed()deep-clones on every shoot event.
cache.getSeed(name)(exported helper) does_.chain(...).find(...).cloneDeep().value()— an O(N) scan plus a full deep clone of the seed object — executed on every shoot add/update/delete and additionally once per affectedseedName(up to 2 per update). Here onlymetadata.uidis needed, so the clone is pure overhead. In bulk informer resync this can be a noticeable hotspot.Consider using the un-cloned lookup from the underlying store:
♻️ Suggested refactor
- for (const seedName of affectedSeedNames) { - const seedUid = cache.getSeed(seedName)?.metadata?.uid + for (const seedName of affectedSeedNames) { + const seedUid = cache.cache.get('seeds')?.find(['metadata.name', seedName])?.metadata?.uid if (!seedUid) { continue }Alternatively, add a dedicated
getSeedUidByName(name)helper that avoids the clone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/watches/shoots.js` around lines 70 - 90, The publishSeedStats function is calling cache.getSeed(seedName) which deep-clones the whole seed object (expensive) even though only metadata.uid is needed; change the lookup to avoid the clone by either (A) adding a new helper cache.getSeedUidByName(name) that returns seed.metadata.uid without clone and use that here, or (B) access the underlying store directly (e.g., cache.store or index) to retrieve the seed object reference and read .metadata.uid, then keep the rest of the loop (getJoinedRooms, shouldEmitSeedStatsEvent, nsp.to(room).emit) unchanged so only the uid is read without cloning the entire seed.backend/lib/cache/index.js (1)
97-99: Inconsistent return type: iterator vs array.
getShootsBySeedNamereturns aMapIteratorwhen the seed is known and a plainArray([]) when not.for..ofconsumers work either way, but any caller using array methods (e.g.,.length,.map,.filter) would silently break for the known-seed path. Consider normalizing to an array for a consistent API.♻️ Suggested tweak
getShootsBySeedName (seedName) { - return this.#seedNameToShoots.get(seedName)?.values() ?? [] + const bucket = this.#seedNameToShoots.get(seedName) + return bucket ? Array.from(bucket.values()) : [] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/cache/index.js` around lines 97 - 99, getShootsBySeedName currently returns a Map iterator when a seed exists and an array when it doesn't, causing inconsistent types; change it to always return an array by materializing the iterator (for example, use Array.from on this.#seedNameToShoots.get(seedName)?.values()) and keep the fallback as an empty array so callers can reliably use array methods like .length/.map/.filter.backend/__tests__/acceptance/io.spec.js (1)
260-320: Minor:'seeds'is now listed both indefaultRoomsand in individual assertions.Since
defaultRoomsnow includes'seeds'(line 220), everynew Set([...defaultRooms, 'seeds', ...])in this block (and similar ones at lines ~293, ~300, ~317, ~336 below, plus ~501, ~519, ~537, ~555 in the admin block) contains a redundant'seeds'. TheSetdedupes so tests still pass, but the expectations read as if the room is asserted twice. Consider cleaning these up for clarity.♻️ Example cleanup
expect(getRooms(socket, nsp)).toEqual(new Set([ ...defaultRooms, - 'seeds', 'shoots;garden-foo/fooShoot', ]))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/__tests__/acceptance/io.spec.js` around lines 260 - 320, The test assertions redundantly include 'seeds' alongside defaultRooms (which already contains 'seeds'); update the expectations to remove the duplicate literal so they read new Set([...defaultRooms, 'shoots;garden-foo', ...]) etc. Locate uses around the getRooms(socket, nsp) checks in the describe block (references: defaultRooms, getRooms, nsp, subscribe, unsubscribe, publishEvent) and delete the extra 'seeds' entries in each new Set([...defaultRooms, 'seeds', ...]) occurrence (including similar cases in the admin block) so the Set contents are clearer while preserving test semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/lib/cache/index.js`:
- Around line 140-146: findProjectByNamespace now throws NotFound for unknown
namespaces and four callers don't handle that; update each caller (the calls in
terminals utils, terminals index, MemberManager, and tickets route) to wrap the
call to findProjectByNamespace(namespace) in a try/catch that specifically
catches NotFound and handles it appropriately for the context (e.g., return
null/undefined or skip the operation, or send a 404 response from the route), or
alternatively add a safe existence check in the cache (e.g., a
hasProjectForNamespace(namespace) method) and use that before calling
findProjectByNamespace to avoid the throw.
In `@frontend/src/components/GShootHealthDonut.vue`:
- Around line 243-257: The ariaLabel computed currently uses static plural nouns
and must pluralize based on counts: update the ariaLabel computed (function
ariaLabel and its usage of shootCount.value, matchingUnhealthy.value,
healthyShoots.value, hiddenUnhealthy.value, and filterDescription.value) to
choose "shoot" vs "shoots" (and "excluded" phrasing) when the numeric value ===
1; e.g., build each phrase using a small inline conditional that picks singular
when count === 1 and plural otherwise, and keep the rest of the structure
(including the hiddenUnhealthy filter desc) unchanged.
In `@frontend/src/composables/useApi/api.js`:
- Around line 261-266: The getSeedStat function currently calls
encodeURIComponent on a possibly undefined name causing requests to
/api/seedstats/undefined; update getSeedStat to validate that name is provided
and is a non-empty string (e.g., if (!name || typeof name !== 'string') throw
new Error(...)) before calling encodeURIComponent, so callers must pass a name
(use getSeedStats for the "all seeds" case); reference getSeedStat to add this
guard and keep existing encodeURIComponent and withQuery usage unchanged.
In `@frontend/src/store/helper.js`:
- Around line 21-22: The lock TTL is set to 30_000ms but socket
acknowledgementTimeout is 60_000ms, allowing a second caller to acquire() while
the first synchronize/subscribe is still in flight; update the expiresAt
calculation in the lock (where this.expiresAt is set) to use the same
acknowledgementTimeout constant from frontend/src/store/socket/index.js (or
derive TTL = acknowledgementTimeout + small buffer) so the lock outlives the
longest socket ack window and prevents concurrent acquires with identical
options.
---
Nitpick comments:
In `@backend/__tests__/acceptance/io.spec.js`:
- Around line 260-320: The test assertions redundantly include 'seeds' alongside
defaultRooms (which already contains 'seeds'); update the expectations to remove
the duplicate literal so they read new Set([...defaultRooms,
'shoots;garden-foo', ...]) etc. Locate uses around the getRooms(socket, nsp)
checks in the describe block (references: defaultRooms, getRooms, nsp,
subscribe, unsubscribe, publishEvent) and delete the extra 'seeds' entries in
each new Set([...defaultRooms, 'seeds', ...]) occurrence (including similar
cases in the admin block) so the Set contents are clearer while preserving test
semantics.
In `@backend/lib/cache/index.js`:
- Around line 97-99: getShootsBySeedName currently returns a Map iterator when a
seed exists and an array when it doesn't, causing inconsistent types; change it
to always return an array by materializing the iterator (for example, use
Array.from on this.#seedNameToShoots.get(seedName)?.values()) and keep the
fallback as an empty array so callers can reliably use array methods like
.length/.map/.filter.
In `@backend/lib/services/seedstats.js`:
- Around line 164-184: The parseUnhealthyFilterMask currently treats an explicit
empty string as 0 because Number('') === 0; update parseUnhealthyFilterMask to
reject empty strings before converting (e.g., if typeof unhealthyFilterMask ===
'string' && unhealthyFilterMask.trim() === '' then throw
invalidUnhealthyFilterMask()), then proceed to Number(...) and validate with
isValidUnhealthyFilterMask; reference functions: parseUnhealthyFilterMask,
isValidUnhealthyFilterMask and the error helper invalidUnhealthyFilterMask and
constant ALL_UNHEALTHY_FILTER_FLAGS.
In `@backend/lib/watches/shoots.js`:
- Around line 70-90: The publishSeedStats function is calling
cache.getSeed(seedName) which deep-clones the whole seed object (expensive) even
though only metadata.uid is needed; change the lookup to avoid the clone by
either (A) adding a new helper cache.getSeedUidByName(name) that returns
seed.metadata.uid without clone and use that here, or (B) access the underlying
store directly (e.g., cache.store or index) to retrieve the seed object
reference and read .metadata.uid, then keep the rest of the loop
(getJoinedRooms, shouldEmitSeedStatsEvent, nsp.to(room).emit) unchanged so only
the uid is read without cloning the entire seed.
In `@frontend/src/components/GShootHealthDonut.vue`:
- Around line 218-227: The formatCompact function contains a dead branch: in the
1000–9999 range v = Math.floor(value / 100) / 10 is always < 10, so the ternary
v < 10 ? 1 : 0 is redundant; update the formatCompact implementation (the
function named formatCompact) to always use toFixed(1) for that branch (and you
can optionally simplify the v calculation or the return to a single toFixed(1)
call) so the ternary is removed and the code is clearer.
- Around line 167-168: Clamp derived counts to zero to avoid transient
negatives: update the computed properties hiddenUnhealthy and healthyShoots so
their returned values use Math.max(0, ...). Specifically, change hiddenUnhealthy
(which currently computes totalUnhealthy.value - matchingUnhealthy.value) to
return Math.max(0, totalUnhealthy.value - matchingUnhealthy.value) and change
healthyShoots (which currently computes shootCount.value - totalUnhealthy.value)
to return Math.max(0, shootCount.value - totalUnhealthy.value); keep the same
reactive/computed wrappers and names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b522b86a-a7c2-4711-98df-1ef97093209c
📒 Files selected for processing (61)
.gitignorebackend/__tests__/acceptance/api.members.spec.jsbackend/__tests__/acceptance/api.seedstats.spec.jsbackend/__tests__/acceptance/api.terminals.spec.jsbackend/__tests__/acceptance/api.tickets.spec.jsbackend/__tests__/acceptance/io.cors.spec.jsbackend/__tests__/acceptance/io.spec.jsbackend/__tests__/cache.spec.jsbackend/__tests__/helpers/cache.jsbackend/__tests__/hooks.spec.jsbackend/__tests__/io.seedstats.spec.jsbackend/__tests__/services.seedstats.spec.jsbackend/__tests__/services.terminals.spec.jsbackend/__tests__/utils.spec.jsbackend/__tests__/watches.spec.jsbackend/lib/cache/index.jsbackend/lib/hooks.jsbackend/lib/io/dispatcher.jsbackend/lib/io/helper.jsbackend/lib/io/index.jsbackend/lib/io/seedstats.jsbackend/lib/io/shoots.jsbackend/lib/routes/index.jsbackend/lib/routes/seedstats.jsbackend/lib/services/index.jsbackend/lib/services/seedstats.jsbackend/lib/utils/index.jsbackend/lib/watches/leases.jsbackend/lib/watches/seeds.jsbackend/lib/watches/shoots.jsfrontend/__tests__/components/GSeedInfrastructureCard.spec.jsfrontend/__tests__/components/GShootHealthDonut.spec.jsfrontend/__tests__/composables/useDonutChart.spec.jsfrontend/__tests__/composables/useSeedTableSorting.spec.jsfrontend/__tests__/composables/useShootItem.spec.jsfrontend/__tests__/composables/useShootListFilters.spec.jsfrontend/__tests__/stores/seedStat.spec.jsfrontend/__tests__/stores/shoot.spec.jsfrontend/src/components/GSeedListRow.vuefrontend/src/components/GShootHealthDonut.vuefrontend/src/components/SeedDetails/GSeedInfrastructureCard.vuefrontend/src/components/Seeds/GSeedCapacityIndicator.vuefrontend/src/composables/useApi/api.jsfrontend/src/composables/useDonutChart.jsfrontend/src/composables/useSeedItem/index.jsfrontend/src/composables/useSeedTableSorting.jsfrontend/src/composables/useShootListFilters.jsfrontend/src/composables/useSocketEventHandler.jsfrontend/src/router/guards.jsfrontend/src/store/helper.jsfrontend/src/store/localStorage.jsfrontend/src/store/seedStat.jsfrontend/src/store/shoot/helper.jsfrontend/src/store/shoot/shoot.jsfrontend/src/store/socket/helper.jsfrontend/src/store/socket/index.jsfrontend/src/utils/errorCodes.jsfrontend/src/utils/index.jsfrontend/src/views/GAdministration.vuefrontend/src/views/GSeedList.vuefrontend/src/views/GShootList.vue
💤 Files with no reviewable changes (3)
- frontend/tests/stores/shoot.spec.js
- frontend/src/router/guards.js
- frontend/src/store/localStorage.js
✅ Files skipped from review due to trivial changes (17)
- .gitignore
- backend/tests/acceptance/api.tickets.spec.js
- backend/tests/acceptance/api.terminals.spec.js
- backend/lib/io/shoots.js
- backend/tests/services.terminals.spec.js
- frontend/tests/composables/useSeedTableSorting.spec.js
- frontend/src/views/GAdministration.vue
- backend/tests/acceptance/api.members.spec.js
- backend/lib/hooks.js
- frontend/src/utils/index.js
- frontend/src/utils/errorCodes.js
- backend/lib/services/index.js
- backend/lib/routes/seedstats.js
- backend/tests/services.seedstats.spec.js
- backend/tests/acceptance/api.seedstats.spec.js
- backend/lib/io/seedstats.js
- backend/tests/helpers/cache.js
🚧 Files skipped from review as they are similar to previous changes (21)
- backend/lib/routes/index.js
- frontend/src/composables/useSeedTableSorting.js
- backend/lib/io/index.js
- backend/tests/hooks.spec.js
- backend/tests/utils.spec.js
- backend/lib/watches/seeds.js
- backend/tests/cache.spec.js
- frontend/src/composables/useSocketEventHandler.js
- frontend/tests/composables/useShootListFilters.spec.js
- frontend/tests/components/GSeedInfrastructureCard.spec.js
- backend/tests/io.seedstats.spec.js
- backend/lib/utils/index.js
- backend/lib/io/dispatcher.js
- frontend/src/components/SeedDetails/GSeedInfrastructureCard.vue
- frontend/src/components/Seeds/GSeedCapacityIndicator.vue
- frontend/src/store/seedStat.js
- frontend/src/views/GSeedList.vue
- frontend/tests/stores/seedStat.spec.js
- backend/lib/watches/leases.js
- frontend/src/composables/useShootListFilters.js
- frontend/src/composables/useSeedItem/index.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/lib/services/seedstats.js (1)
164-178:parseUnhealthyFilterMaskrejects missing values but silently accepts empty strings as0.
Number('')evaluates to0, which passesisValidUnhealthyFilterMask, so?unhealthyFilterMask=(empty) yields the unfiltered view while?unhealthyFilterMaskomitted yields 422. That asymmetry can confuse API clients. Consider rejecting empty strings (or all whitespace) explicitly, or — if the parameter is conceptually required — also reject''consistently withundefined.♻️ Suggested tightening
if (typeof unhealthyFilterMask === 'string') { + if (unhealthyFilterMask.trim() === '') { + throw invalidUnhealthyFilterMask() + } unhealthyFilterMask = Number(unhealthyFilterMask) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/services/seedstats.js` around lines 164 - 178, parseUnhealthyFilterMask currently converts an empty string to 0 (via Number('')) which passes isValidUnhealthyFilterMask; update parseUnhealthyFilterMask to explicitly reject empty or all-whitespace strings by checking the raw string (e.g., trim() === '') and throwing invalidUnhealthyFilterMask() before converting to Number, so that both omitted (undefined) and empty-string parameters are treated consistently; keep references to parseUnhealthyFilterMask, isValidUnhealthyFilterMask, and invalidUnhealthyFilterMask when making the change.backend/lib/cache/tickets.js (1)
11-13: Optional: avoidJSON.stringifyfor hot-path key building.
shootKeyis called on every cache mutation and lookup. A simple delimited string (with a separator unlikely to appear in resource names, e.g./) would be cheaper and equally collision-free for Kubernetes-style names.♻️ Optional refactor
-function shootKey (projectName, name) { - return JSON.stringify([projectName, name]) -} +function shootKey (projectName, name) { + return `${projectName}/${name}` +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/cache/tickets.js` around lines 11 - 13, The hot-path function shootKey currently uses JSON.stringify([projectName, name]) which is slower; replace it with a cheap delimited string construction in shootKey (e.g. projectName + '/' + name) using a separator unlikely to appear in resource names (or escape the separator if necessary) so cache lookups/mutations that call shootKey are faster and collision-free for Kubernetes-style names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/lib/services/seedstats.js`:
- Around line 257-269: getProjectNameForShoot is spamming logger.warn for the
same failing namespace on every refresh; change the logging to avoid repeated
warnings by adding a module-level Set (e.g., failedNamespaceWarned) and in
getProjectNameForShoot call cache.findProjectByNamespace(namespace) inside the
try/catch and only call logger.warn('Failed to resolve project for namespace %s:
%s', namespace, err.message) if failedNamespaceWarned does not contain
namespace, then add namespace to the set; also add a hook to clear
failedNamespaceWarned at the start or end of each seedstats aggregation/tick
(where the aggregation loop runs) so failures are re-attempted next tick;
alternatively, if you prefer simpler behavior, replace logger.warn with
logger.debug in getProjectNameForShoot to demote the noise.
---
Nitpick comments:
In `@backend/lib/cache/tickets.js`:
- Around line 11-13: The hot-path function shootKey currently uses
JSON.stringify([projectName, name]) which is slower; replace it with a cheap
delimited string construction in shootKey (e.g. projectName + '/' + name) using
a separator unlikely to appear in resource names (or escape the separator if
necessary) so cache lookups/mutations that call shootKey are faster and
collision-free for Kubernetes-style names.
In `@backend/lib/services/seedstats.js`:
- Around line 164-178: parseUnhealthyFilterMask currently converts an empty
string to 0 (via Number('')) which passes isValidUnhealthyFilterMask; update
parseUnhealthyFilterMask to explicitly reject empty or all-whitespace strings by
checking the raw string (e.g., trim() === '') and throwing
invalidUnhealthyFilterMask() before converting to Number, so that both omitted
(undefined) and empty-string parameters are treated consistently; keep
references to parseUnhealthyFilterMask, isValidUnhealthyFilterMask, and
invalidUnhealthyFilterMask when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7501cecf-e339-4ea1-93e3-1fbcce30d761
📒 Files selected for processing (9)
backend/__tests__/cache.tickets.spec.jsbackend/__tests__/hooks.spec.jsbackend/__tests__/io.seedstats.spec.jsbackend/__tests__/services.seedstats.spec.jsbackend/lib/cache/tickets.jsbackend/lib/io/seedstats.jsbackend/lib/services/seedstats.jsfrontend/src/composables/useApi/api.jsfrontend/src/composables/useSeedItem/index.js
✅ Files skipped from review due to trivial changes (1)
- backend/tests/io.seedstats.spec.js
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/tests/hooks.spec.js
- backend/tests/services.seedstats.spec.js
- frontend/src/composables/useSeedItem/index.js
- backend/lib/io/seedstats.js
Comment on lines
+257
to
+269
| function getProjectNameForShoot (shoot) { | ||
| const namespace = shoot?.metadata?.namespace | ||
| if (!namespace) { | ||
| return | ||
| } | ||
| try { | ||
| const project = cache.findProjectByNamespace(namespace) | ||
| return project?.metadata?.name | ||
| } catch (err) { | ||
| logger.warn('Failed to resolve project for namespace %s: %s', namespace, err.message) | ||
| return undefined | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential log spam from getProjectNameForShoot.
When FILTER_HIDE_TICKETS is enabled, this is invoked for every unhealthy shoot per seedstats refresh (and per Socket.IO room emission). A single namespace consistently failing project resolution will produce a warn per shoot per refresh tick. Consider rate-limiting, demoting to debug, or memoizing failures within a single aggregation pass.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/lib/services/seedstats.js` around lines 257 - 269,
getProjectNameForShoot is spamming logger.warn for the same failing namespace on
every refresh; change the logging to avoid repeated warnings by adding a
module-level Set (e.g., failedNamespaceWarned) and in getProjectNameForShoot
call cache.findProjectByNamespace(namespace) inside the try/catch and only call
logger.warn('Failed to resolve project for namespace %s: %s', namespace,
err.message) if failedNamespaceWarned does not contain namespace, then add
namespace to the set; also add a hook to clear failedNamespaceWarned at the
start or end of each seedstats aggregation/tick (where the aggregation loop
runs) so failures are re-attempted next tick; alternatively, if you prefer
simpler behavior, replace logger.warn with logger.debug in
getProjectNameForShoot to demote the noise.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/__tests__/stores/seedStat.spec.js`:
- Line 126: The Promise executor uses a one-line block that causes
lint/verification failures; update the test declaration for fetchPromise (and
the similar occurrence around line 151) so the executor body is on separate
lines: declare fetchPromise = new Promise(resolve => { then on the next line
assign fetchResolve = resolve and close the block on its own line. Ensure you
change both instances (the variable names fetchPromise and fetchResolve in this
test file) so each assignment is on its own line inside the executor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d7ad202-c58e-419d-9080-dc8945f94417
📒 Files selected for processing (4)
frontend/__tests__/stores/seedStat.spec.jsfrontend/src/components/SeedDetails/GSeedInfrastructureCard.vuefrontend/src/store/seedStat.jsfrontend/src/views/GSeedList.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/store/seedStat.js
gardener-prow
Bot
added
the
needs-rebase
…watcher The store's subscribe(), unsubscribe(), and synchronize() were async functions that could interleave across await boundaries — an older subscribe could resume and call openSubscription after a newer unsubscribe had already cleared the store. Replace the imperative async lifecycle with a single Vue watch on subscription + refreshNonce. onCleanup cancels stale runs at each await boundary. Public API becomes synchronous; callers simplified.
petersutter
force-pushed
the
enh/seed-stats-v2
branch
from
c37729b to
5867239
Compare
gardener-prow
Bot
removed
the
needs-rebase
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
1 participant
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.
What this PR does / why we need it:
Adds per-seed shoot health statistics and capacity indicators to the seed list and seed detail views, with real-time updates via Socket.IO.
Backend
shootCountandunhealthyShoots: { total, matching }.totalcounts all unhealthy shoots;matchingcounts only those not excluded by the active filter mask.unhealthyFilterMaskparameter controls which unhealthy shoots are excluded frommatching:GET /api/seedstats[/:name]?unhealthyFilterMask=<mask>.subscribe/unsubscribewith filter-specific rooms (seedstats;uf=<mask>,seedstats;seed=<name>;uf=<mask>). Server emits pre-computed counts per room. Only rooms with members trigger recomputation.Frontend
allocatableShootsfrom the seed spec.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note:
Summary by CodeRabbit
New Features
Improvements
Tests
Chores