feat: link to managed seed shoot and combine readiness#2834
Open
petersutter wants to merge 1 commit intomasterfrom
Open
feat: link to managed seed shoot and combine readiness#2834petersutter wants to merge 1 commit intomasterfrom
petersutter wants to merge 1 commit intomasterfrom
Conversation
petersutter
requested review from
grolu,
holgerkoser and
klocke-io
as code owners
gardener-prow
bot
added
do-not-merge/needs-kind
📝 WalkthroughWalkthroughAdds end-to-end support for managed seeds and managed seed shoots: backend cache accessors, services, routes, Socket.IO synchronize handlers and watches, authorization checks, and frontend stores, composables, components, router guards and tests to fetch, present and live-sync garden-scoped managed-seed and managed-seed-shoot data. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client
participant Router as Router Guard
participant API as Backend API
participant Auth as Authorization Service
participant Cache as Cache Store
participant Socket as Socket.IO
Browser->>Router: navigate (seeds page, admin)
Router->>API: GET /api/namespaces/garden/managedseeds
API->>Auth: canListManagedSeedsInGardenNamespace(user)
Auth-->>API: allowed
API->>Cache: getManagedSeedsInGardenNamespace()
Cache-->>API: [managedSeed objects]
API-->>Browser: simplified managed seeds
Note over Socket,Cache: real-time sync
Socket->>Cache: watch managedseeds (garden)
Cache-->>Socket: ADDED/MODIFIED/DELETED events
Socket-->>Browser: emit "managedseeds" events
Browser->>Browser: update managedSeedStore / UI
Browser->>API: GET /api/namespaces/garden/managedseed-shoots
API->>Auth: canListShootsInGardenNamespace(user)
Auth-->>API: allowed
API->>Cache: getManagedSeedForShootInGardenNamespace(name) / getShootByName
Cache-->>API: shoot + status
API-->>Browser: simplified managed-seed-shoots
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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)
📝 Coding Plan
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 |
|
[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 |
petersutter
added
kind/enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (7)
frontend/__tests__/composables/useApi.spec.js (1)
36-52: Test looks good, but coverage forgetManagedSeedsForGardenNamespaceis missing.The test correctly verifies the
getManagedSeedShootsForGardenNamespaceendpoint. Consider adding a similar test forgetManagedSeedsForGardenNamespaceto maintain consistent coverage for both new API functions.💡 Suggested test for getManagedSeedsForGardenNamespace
describe('#getManagedSeedsForGardenNamespace', () => { it('should fetch managed seeds from the garden namespace endpoint', async () => { fetch.mockResponseOnce(JSON.stringify([]), { headers: { 'content-type': 'application/json; charset=UTF-8', }, }) const res = await api.getManagedSeedsForGardenNamespace() expect(res.status).toBe(200) expect(res.data).toEqual([]) expect(fetch).toBeCalledTimes(1) const [req] = fetch.mock.calls[0] expect(req.url).toBe('http://localhost:3000/api/namespaces/garden/managedseeds') }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/composables/useApi.spec.js` around lines 36 - 52, Add a new test block mirroring the existing describe('#getManagedSeedShootsForGardenNamespace') case to cover getManagedSeedsForGardenNamespace: mock fetch response with fetch.mockResponseOnce(JSON.stringify([]), { headers: { 'content-type': 'application/json; charset=UTF-8' } }), call api.getManagedSeedsForGardenNamespace(), assert res.status === 200 and res.data equals [], assert fetch called once, and assert the requested URL equals 'http://localhost:3000/api/namespaces/garden/managedseeds' so the new function getManagedSeedsForGardenNamespace is covered.frontend/src/store/managedSeedShoot.js (1)
50-52: Consider null-safety forshootByNamewhen list is uninitialized.The
findfunction from lodash will returnundefinedwhen called withnullas the collection, which is safe. However, if this behavior is intentional, it might be worth adding a brief comment or early return for clarity, especially sinceisInitialdistinguishes the null state.♻️ Optional: Add explicit null check for clarity
function shootByName (namespace, name) { + if (list.value === null) { + return undefined + } return find(list.value, { metadata: { namespace, name } }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/store/managedSeedShoot.js` around lines 50 - 52, The helper shootByName currently calls find(list.value, { metadata: { namespace, name } }) which can receive null when list is uninitialized; add an explicit null-safety guard in the shootByName function (check list.value or isInitial) and return null/undefined early if the list is not ready so callers see the intent clearly; reference the shootByName function and the list.value access and use an early return to avoid relying on implicit lodash behavior.backend/lib/watches/managedseeds.js (1)
22-24: Consider consistency in delete handler invocation.The
deletehandler passes the deleted object asnewObject:informer.on('delete', object => handleEvent('DELETED', object))While this works due to the
newObject ?? oldObjectfallback on line 13, semantically the deleted object is the "old" object. For consistency with theupdatehandler pattern and clearer intent:♻️ Optional: Pass deleted object as oldObject
informer.on('add', object => handleEvent('ADDED', object)) informer.on('update', (object, oldObject) => handleEvent('MODIFIED', object, oldObject)) -informer.on('delete', object => handleEvent('DELETED', object)) +informer.on('delete', object => handleEvent('DELETED', undefined, object))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/watches/managedseeds.js` around lines 22 - 24, The delete handler currently calls handleEvent('DELETED', object) which treats the deleted resource as the "new" object; change it to pass the deleted resource as the oldObject (i.e., call handleEvent('DELETED', undefined, object)) so it matches the update pattern and the function signature of handleEvent(newObject, oldObject).backend/__tests__/services.managedseedshoots.spec.cjs (1)
39-50: Add the inverse authorization case.This only proves the
canListManagedSeedsInGardenNamespace = falsepath. A regression wherecanListShootsInGardenNamespacestops being enforced would still pass this suite.💡 Suggested test
it('should reject unauthorized users', async () => { authorization.canListManagedSeedsInGardenNamespace.mockResolvedValue(false) await expect(managedseedshoots.list({ user, namespace: 'garden' })).rejects.toEqual(expect.objectContaining({ statusCode: 403, message: 'You are not allowed to list managed seed shoots in the garden namespace', })) expect(authorization.canListManagedSeedsInGardenNamespace).toHaveBeenCalledWith(user) expect(authorization.canListShootsInGardenNamespace).toHaveBeenCalledWith(user) expect(cache.getManagedSeedsInGardenNamespace).not.toHaveBeenCalled() }) + + it('should reject users without shoot access', async () => { + authorization.canListShootsInGardenNamespace.mockResolvedValue(false) + + await expect(managedseedshoots.list({ user, namespace: 'garden' })).rejects.toEqual(expect.objectContaining({ + statusCode: 403, + message: 'You are not allowed to list managed seed shoots in the garden namespace', + })) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/__tests__/services.managedseedshoots.spec.cjs` around lines 39 - 50, Add a test that covers the inverse authorization path by simulating canListManagedSeedsInGardenNamespace returning true but canListShootsInGardenNamespace returning false, then call managedseedshoots.list({ user, namespace: 'garden' }) and assert it rejects with a 403 and the same message; also assert that authorization.canListManagedSeedsInGardenNamespace was calledWith(user), authorization.canListShootsInGardenNamespace was calledWith(user), and that cache.getManagedSeedsInGardenNamespace was not called to ensure both checks are enforced.frontend/src/views/GSeedList.vue (1)
142-150: Sanitize persisted sort keys when the seed headers change.This PR changes the available columns, but the view still restores
seedSortByfrom local storage verbatim on mount. A stale key from an older session can leave the table in an invalid sort state until the user manually resets it.💡 Suggested follow-up
onMounted(() => { updateTableSettings() if (seedSortBy.value?.length) { - sortBy.value = [...seedSortBy.value] + const validHeaderKeys = new Set(allHeaders.value.map(({ key }) => key)) + const persistedSortBy = seedSortBy.value.filter(({ key }) => validHeaderKeys.has(key)) + sortBy.value = persistedSortBy.length ? persistedSortBy : [{ key: 'name', order: 'asc' }] + seedSortBy.value = sortBy.value } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/views/GSeedList.vue` around lines 142 - 150, Persisted seedSortBy may reference column keys that no longer exist; when restoring seedSortBy in GSeedList.vue, validate the saved key against the current headers (the array with objects containing title/key/defaultSelected, e.g., the 'shoot' header using getManagedSeedShootName). If the saved key is not found among current header.key values, discard it and fall back to a safe default (the header with defaultSelected true or an explicit fallback like 'name'), then persist that sanitized value. Implement this check where seedSortBy/localStorage is read on mount/setup so stale keys are never applied to the table.backend/lib/io/helper.js (1)
70-81: Separate the garden-scoped permissions inreq.user.profilesto preserve access control granularity.The composite flag
canGetManagedSeedAndShootInGardenNsuses AND logic, meaning a user with onlycanListManagedSeedsInGardenNamespacepermission (withoutcanListShootsInGardenNamespace) will be denied access to managed seeds in the IO layer. Bothbackend/lib/io/managedseeds.jsandbackend/lib/io/index.jscheck only the composite flag, which incorrectly locks out users who should have access to managed seeds alone.Suggested change
const profiles = Object.freeze({ canListProjects, canListSeeds, + canListManagedSeedsInGardenNamespace, + canListShootsInGardenNamespace, canGetManagedSeedAndShootInGardenNs: canListManagedSeedsInGardenNamespace && canListShootsInGardenNamespace, })Then update the access resolvers:
backend/lib/io/managedseeds.js:18→ checkcanListManagedSeedsInGardenNamespacealonebackend/lib/io/managedseedshoots.js:18→ check both permissions (keep composite or check both explicitly)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/io/helper.js` around lines 70 - 81, The composite profile flag canGetManagedSeedAndShootInGardenNs (created from canListManagedSeedsInGardenNamespace && canListShootsInGardenNamespace in the profiles object) incorrectly denies users who have only canListManagedSeedsInGardenNamespace; change profiles to expose the two boolean flags separately (keep canListManagedSeedsInGardenNamespace and canListShootsInGardenNamespace) and remove or stop relying on the composite flag for managed-seed-only checks; then update the access checks: in managedseeds.js use canListManagedSeedsInGardenNamespace alone to authorize managed-seed access, and in managedseedshoots.js continue to require both permissions (either by checking the two flags explicitly or by using the composite where appropriate).backend/__tests__/acceptance/io.spec.cjs (1)
566-571: Don’t preload access-review mocks in the JWT rejection tests.These cases should prove authentication fails before any authorization check runs. Seeding four successful
mockRequestresponses here makes the tests pass even if expired or refresh-required tokens start hitting the authz backend.♻️ Tighten the setup
beforeEach(() => { - mockRequest - .mockImplementationOnce(fixtures.auth.mocks.reviewSelfSubjectAccess()) - .mockImplementationOnce(fixtures.auth.mocks.reviewSelfSubjectAccess()) - .mockImplementationOnce(fixtures.auth.mocks.reviewSelfSubjectAccess()) - .mockImplementationOnce(fixtures.auth.mocks.reviewSelfSubjectAccess()) timestamp = Math.floor(Date.now() / 1000) })Add
expect(mockRequest).not.toHaveBeenCalled()in the two tests below so they keep asserting the auth short-circuit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/__tests__/acceptance/io.spec.cjs` around lines 566 - 571, Remove the preloaded authorization responses by deleting the four mockImplementationOnce(...fixtures.auth.mocks.reviewSelfSubjectAccess()) calls so the JWT rejection tests don't seed authz results; then, in the two JWT-rejection test cases that assert short-circuit authentication failure, add expect(mockRequest).not.toHaveBeenCalled() to ensure no authorization requests were made. Target the mockRequest setup and the two tests that assert token expiry/refresh-required behavior so they verify auth is short-circuited before any call to fixtures.auth.mocks.reviewSelfSubjectAccess().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/__tests__/acceptance/io.spec.cjs`:
- Around line 566-571: Remove the preloaded authorization responses by deleting
the four
mockImplementationOnce(...fixtures.auth.mocks.reviewSelfSubjectAccess()) calls
so the JWT rejection tests don't seed authz results; then, in the two
JWT-rejection test cases that assert short-circuit authentication failure, add
expect(mockRequest).not.toHaveBeenCalled() to ensure no authorization requests
were made. Target the mockRequest setup and the two tests that assert token
expiry/refresh-required behavior so they verify auth is short-circuited before
any call to fixtures.auth.mocks.reviewSelfSubjectAccess().
In `@backend/__tests__/services.managedseedshoots.spec.cjs`:
- Around line 39-50: Add a test that covers the inverse authorization path by
simulating canListManagedSeedsInGardenNamespace returning true but
canListShootsInGardenNamespace returning false, then call
managedseedshoots.list({ user, namespace: 'garden' }) and assert it rejects with
a 403 and the same message; also assert that
authorization.canListManagedSeedsInGardenNamespace was calledWith(user),
authorization.canListShootsInGardenNamespace was calledWith(user), and that
cache.getManagedSeedsInGardenNamespace was not called to ensure both checks are
enforced.
In `@backend/lib/io/helper.js`:
- Around line 70-81: The composite profile flag
canGetManagedSeedAndShootInGardenNs (created from
canListManagedSeedsInGardenNamespace && canListShootsInGardenNamespace in the
profiles object) incorrectly denies users who have only
canListManagedSeedsInGardenNamespace; change profiles to expose the two boolean
flags separately (keep canListManagedSeedsInGardenNamespace and
canListShootsInGardenNamespace) and remove or stop relying on the composite flag
for managed-seed-only checks; then update the access checks: in managedseeds.js
use canListManagedSeedsInGardenNamespace alone to authorize managed-seed access,
and in managedseedshoots.js continue to require both permissions (either by
checking the two flags explicitly or by using the composite where appropriate).
In `@backend/lib/watches/managedseeds.js`:
- Around line 22-24: The delete handler currently calls handleEvent('DELETED',
object) which treats the deleted resource as the "new" object; change it to pass
the deleted resource as the oldObject (i.e., call handleEvent('DELETED',
undefined, object)) so it matches the update pattern and the function signature
of handleEvent(newObject, oldObject).
In `@frontend/__tests__/composables/useApi.spec.js`:
- Around line 36-52: Add a new test block mirroring the existing
describe('#getManagedSeedShootsForGardenNamespace') case to cover
getManagedSeedsForGardenNamespace: mock fetch response with
fetch.mockResponseOnce(JSON.stringify([]), { headers: { 'content-type':
'application/json; charset=UTF-8' } }), call
api.getManagedSeedsForGardenNamespace(), assert res.status === 200 and res.data
equals [], assert fetch called once, and assert the requested URL equals
'http://localhost:3000/api/namespaces/garden/managedseeds' so the new function
getManagedSeedsForGardenNamespace is covered.
In `@frontend/src/store/managedSeedShoot.js`:
- Around line 50-52: The helper shootByName currently calls find(list.value, {
metadata: { namespace, name } }) which can receive null when list is
uninitialized; add an explicit null-safety guard in the shootByName function
(check list.value or isInitial) and return null/undefined early if the list is
not ready so callers see the intent clearly; reference the shootByName function
and the list.value access and use an early return to avoid relying on implicit
lodash behavior.
In `@frontend/src/views/GSeedList.vue`:
- Around line 142-150: Persisted seedSortBy may reference column keys that no
longer exist; when restoring seedSortBy in GSeedList.vue, validate the saved key
against the current headers (the array with objects containing
title/key/defaultSelected, e.g., the 'shoot' header using
getManagedSeedShootName). If the saved key is not found among current header.key
values, discard it and fall back to a safe default (the header with
defaultSelected true or an explicit fallback like 'name'), then persist that
sanitized value. Implement this check where seedSortBy/localStorage is read on
mount/setup so stale keys are never applied to the table.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a9327e0-d28a-4e19-9c0d-b49d341a70a6
⛔ Files ignored due to path filters (2)
backend/__tests__/acceptance/__snapshots__/api.managedseeds.spec.cjs.snapis excluded by!**/*.snapcharts/__tests__/gardener-dashboard/application/dashboard/__snapshots__/clusterrole.spec.js.snapis excluded by!**/*.snap
📒 Files selected for processing (49)
backend/__tests__/acceptance/api.managedseeds.spec.cjsbackend/__tests__/acceptance/io.cors.spec.cjsbackend/__tests__/acceptance/io.spec.cjsbackend/__tests__/hooks.spec.cjsbackend/__tests__/services.managedseeds.spec.cjsbackend/__tests__/services.managedseedshoots.spec.cjsbackend/__tests__/watches.spec.cjsbackend/lib/cache/index.jsbackend/lib/hooks.jsbackend/lib/io/dispatcher.jsbackend/lib/io/helper.jsbackend/lib/io/index.jsbackend/lib/io/managedseeds.jsbackend/lib/io/managedseedshoots.jsbackend/lib/routes/index.jsbackend/lib/routes/managedseeds.jsbackend/lib/routes/managedseedshoots.jsbackend/lib/services/authorization.jsbackend/lib/services/index.jsbackend/lib/services/managedseeds.jsbackend/lib/services/managedseedshoots.jsbackend/lib/utils/index.jsbackend/lib/watches/index.jsbackend/lib/watches/managedseeds.jsbackend/lib/watches/shoots.jscharts/gardener-dashboard/charts/application/templates/dashboard/clusterrole.yamlfrontend/__tests__/composables/useApi.spec.jsfrontend/src/components/GManagedSeedShootLink.vuefrontend/src/components/GSeedListRow.vuefrontend/src/components/GSeedStatusTags.vuefrontend/src/components/GShootListRow.vuefrontend/src/components/SeedDetails/GSeedDetailsCard.vuefrontend/src/components/SeedDetails/GSeedMonitoringCard.vuefrontend/src/composables/useApi/api.jsfrontend/src/composables/useManagedSeedShootForSeed.jsfrontend/src/composables/useSeedEffectiveConditions.jsfrontend/src/composables/useSeedItem/helper.jsfrontend/src/composables/useSeedItem/index.jsfrontend/src/composables/useSeedTableSorting.jsfrontend/src/composables/useShootAdvertisedAddresses.jsfrontend/src/router/guards.jsfrontend/src/store/managedSeed.jsfrontend/src/store/managedSeedShoot.jsfrontend/src/store/socket/helper.jsfrontend/src/store/socket/index.jsfrontend/src/views/GSeedItemPlaceholder.vuefrontend/src/views/GSeedList.vuefrontend/src/views/GShootItemPlaceholder.vuefrontend/src/views/GShootList.vue
💤 Files with no reviewable changes (1)
- frontend/src/composables/useSeedItem/helper.js
petersutter
force-pushed
the
enh/link-to-managed-seed-shoot
branch
from
3533048 to
cea4f92
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
frontend/src/components/SeedDetails/GSeedMonitoringCard.vue (1)
59-72: Consider adding icons for visual consistency.The existing Plutono tile (line 44) specifies
icon="mdi-developer-board", but the new Shoot Plutono and Shoot Prometheus tiles omit the icon prop. This creates visual inconsistency within the card.If the omission is intentional (to visually subordinate these links), this is fine. Otherwise, consider adding appropriate icons.
♻️ Suggested change
<g-link-list-tile v-if="managedSeedShootPlutonoUrl" + icon="mdi-developer-board" app-title="Shoot Plutono" :url="managedSeedShootPlutonoUrl" :url-text="managedSeedShootPlutonoUrl" content-class="pt-0" /> <g-link-list-tile v-if="managedSeedShootPrometheusUrl" + icon="mdi-chart-line" app-title="Shoot Prometheus" :url="managedSeedShootPrometheusUrl" :url-text="managedSeedShootPrometheusUrl" content-class="pt-0" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/SeedDetails/GSeedMonitoringCard.vue` around lines 59 - 72, The two g-link-list-tile components rendering Shoot Plutono and Shoot Prometheus (using managedSeedShootPlutonoUrl and managedSeedShootPrometheusUrl) omit the icon prop causing visual inconsistency with the existing Plutono tile; update those components to pass an appropriate icon prop (e.g., icon="mdi-developer-board" or another suitable mdi-* icon) to match the other tiles, or if omission was intentional, add a brief comment above each g-link-list-tile explaining the deliberate visual subordination.
🤖 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/watches/shoots.js`:
- Around line 55-58: The current delete/update handler returns early when
cache.getManagedSeedForShootInGardenNamespace(name) yields no managedSeed,
causing missed DELETED events; change the logic so that when managedSeed is
undefined you still emit cleanup DELETED events to the
"managedseed-shoots;garden" clients based on a persisted tracking structure
(e.g., a previouslyPublishedManagedSeedShoots map keyed by shoot name or UID)
and then remove the entry from that tracking map; update the code around the
managedSeed variable usage to consult this map on missing lookups, publish the
DELETED event for previously-associated clients, and clear the tracked record so
stale entries cannot remain.
In `@frontend/src/composables/useSeedEffectiveConditions.js`:
- Around line 23-27: The code currently treats only undefined as missing and
returns managedSeedShootConditions.value even when it's null; update the check
in useSeedEffectiveConditions (or replace the conditional) so that null is
treated as missing too — e.g., check for non-nullish value
(managedSeedShootConditions.value != null) or use the nullish coalescing
behavior to return seedConditions.value when managedSeedShootConditions.value is
null or undefined, ensuring downstream rendering gets seedConditions.value
instead of null.
In `@frontend/src/views/GSeedList.vue`:
- Around line 142-150: The SHOOT column's sort key currently falls back to an
empty string which mismatches the displayed unmanaged label; update the value
fallback in the column definition (the object with title 'SHOOT' and value: item
=> getManagedSeedShootName(item) || '') to use 'Unmanaged' instead of '', and
make the same change wherever getManagedSeedShootName(item) is used for
sorting/display (the other occurrence referenced around lines 249-253) so
sorting/search uses the same 'Unmanaged' text shown to users.
---
Nitpick comments:
In `@frontend/src/components/SeedDetails/GSeedMonitoringCard.vue`:
- Around line 59-72: The two g-link-list-tile components rendering Shoot Plutono
and Shoot Prometheus (using managedSeedShootPlutonoUrl and
managedSeedShootPrometheusUrl) omit the icon prop causing visual inconsistency
with the existing Plutono tile; update those components to pass an appropriate
icon prop (e.g., icon="mdi-developer-board" or another suitable mdi-* icon) to
match the other tiles, or if omission was intentional, add a brief comment above
each g-link-list-tile explaining the deliberate visual subordination.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 273a679c-19fd-4769-a581-9865d2d88ece
⛔ Files ignored due to path filters (2)
backend/__tests__/acceptance/__snapshots__/api.managedseeds.spec.cjs.snapis excluded by!**/*.snapcharts/__tests__/gardener-dashboard/application/dashboard/__snapshots__/clusterrole.spec.js.snapis excluded by!**/*.snap
📒 Files selected for processing (50)
backend/__tests__/acceptance/api.managedseeds.spec.cjsbackend/__tests__/acceptance/io.cors.spec.cjsbackend/__tests__/acceptance/io.spec.cjsbackend/__tests__/hooks.spec.cjsbackend/__tests__/services.managedseeds.spec.cjsbackend/__tests__/services.managedseedshoots.spec.cjsbackend/__tests__/watches.spec.cjsbackend/lib/cache/index.jsbackend/lib/hooks.jsbackend/lib/io/dispatcher.jsbackend/lib/io/helper.jsbackend/lib/io/index.jsbackend/lib/io/managedseeds.jsbackend/lib/io/managedseedshoots.jsbackend/lib/routes/index.jsbackend/lib/routes/managedseeds.jsbackend/lib/routes/managedseedshoots.jsbackend/lib/services/authorization.jsbackend/lib/services/index.jsbackend/lib/services/managedseeds.jsbackend/lib/services/managedseedshoots.jsbackend/lib/utils/index.jsbackend/lib/watches/index.jsbackend/lib/watches/managedseeds.jsbackend/lib/watches/shoots.jscharts/gardener-dashboard/charts/application/templates/dashboard/clusterrole.yamlfrontend/__tests__/composables/useApi.spec.jsfrontend/src/components/GManagedSeedShootLink.vuefrontend/src/components/GSeedListRow.vuefrontend/src/components/GSeedStatusTags.vuefrontend/src/components/GShootListRow.vuefrontend/src/components/SeedDetails/GSeedDetailsCard.vuefrontend/src/components/SeedDetails/GSeedInfrastructureCard.vuefrontend/src/components/SeedDetails/GSeedMonitoringCard.vuefrontend/src/composables/useApi/api.jsfrontend/src/composables/useManagedSeedShootForSeed.jsfrontend/src/composables/useSeedEffectiveConditions.jsfrontend/src/composables/useSeedItem/helper.jsfrontend/src/composables/useSeedItem/index.jsfrontend/src/composables/useSeedTableSorting.jsfrontend/src/composables/useShootAdvertisedAddresses.jsfrontend/src/router/guards.jsfrontend/src/store/managedSeed.jsfrontend/src/store/managedSeedShoot.jsfrontend/src/store/socket/helper.jsfrontend/src/store/socket/index.jsfrontend/src/views/GSeedItemPlaceholder.vuefrontend/src/views/GSeedList.vuefrontend/src/views/GShootItemPlaceholder.vuefrontend/src/views/GShootList.vue
💤 Files with no reviewable changes (2)
- frontend/src/composables/useSeedItem/helper.js
- frontend/src/components/SeedDetails/GSeedInfrastructureCard.vue
🚧 Files skipped from review as they are similar to previous changes (29)
- frontend/src/views/GShootList.vue
- backend/lib/services/authorization.js
- backend/lib/io/index.js
- backend/lib/routes/managedseeds.js
- backend/lib/io/managedseedshoots.js
- backend/lib/routes/managedseedshoots.js
- frontend/src/components/GShootListRow.vue
- backend/lib/utils/index.js
- backend/tests/watches.spec.cjs
- frontend/src/components/GManagedSeedShootLink.vue
- backend/lib/io/managedseeds.js
- backend/tests/services.managedseedshoots.spec.cjs
- frontend/src/store/socket/index.js
- backend/lib/io/helper.js
- frontend/src/store/socket/helper.js
- backend/tests/acceptance/api.managedseeds.spec.cjs
- backend/lib/watches/index.js
- backend/lib/services/index.js
- frontend/src/store/managedSeedShoot.js
- frontend/src/store/managedSeed.js
- frontend/src/composables/useManagedSeedShootForSeed.js
- backend/lib/routes/index.js
- frontend/tests/composables/useApi.spec.js
- frontend/src/views/GSeedItemPlaceholder.vue
- frontend/src/composables/useSeedItem/index.js
- frontend/src/router/guards.js
- frontend/src/composables/useShootAdvertisedAddresses.js
- backend/lib/io/dispatcher.js
- backend/lib/services/managedseedshoots.js
Comment on lines
+55
to
+58
| const managedSeed = cache.getManagedSeedForShootInGardenNamespace(name) | ||
| if (!managedSeed) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Emit cleanup events even when managed-seed lookup disappears.
On Line 55, delete/update handling depends on current cache state. If the ManagedSeed was removed first, Line 57 returns early and clients in managedseed-shoots;garden can miss a DELETED event, leaving stale entries.
Proposed fix (track previously published managed-seed shoots)
export default (io, informer, options) => {
const nsp = io.of('/')
const { shootsWithIssues = new Set() } = options ?? {}
+ const managedSeedShootUids = new Set()
const publishManagedSeedShoots = event => {
const { type, object } = event
const { namespace, name, uid } = object.metadata
if (namespace !== 'garden') {
return
}
const managedSeed = cache.getManagedSeedForShootInGardenNamespace(name)
- if (!managedSeed) {
- return
+ if (managedSeed) {
+ managedSeedShootUids.add(uid)
+ nsp.to('managedseed-shoots;garden').emit('managedseed-shoots', { type, uid })
+ return
}
- nsp.to('managedseed-shoots;garden').emit('managedseed-shoots', { type, uid })
+ if (managedSeedShootUids.has(uid)) {
+ managedSeedShootUids.delete(uid)
+ nsp.to('managedseed-shoots;garden').emit('managedseed-shoots', { type: 'DELETED', uid })
+ }
}🤖 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 55 - 58, The current
delete/update handler returns early when
cache.getManagedSeedForShootInGardenNamespace(name) yields no managedSeed,
causing missed DELETED events; change the logic so that when managedSeed is
undefined you still emit cleanup DELETED events to the
"managedseed-shoots;garden" clients based on a persisted tracking structure
(e.g., a previouslyPublishedManagedSeedShoots map keyed by shoot name or UID)
and then remove the entry from that tracking map; update the code around the
managedSeed variable usage to consult this map on missing lookups, publish the
DELETED event for previously-associated clients, and clear the tracked record so
stale entries cannot remain.
Comment on lines
+23
to
+27
| if (managedSeedShootConditions.value !== undefined) { | ||
| return managedSeedShootConditions.value | ||
| } | ||
|
|
||
| return seedConditions.value |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether managedSeedShootConditions is ever initialized/assigned to null.
rg -n -C3 '\bmanagedSeedShootConditions\b' frontend
rg -n -C3 'ref\s*\(\s*null\s*\)|shallowRef\s*\(\s*null\s*\)|\.value\s*=\s*null|\breturn\s+null\b' frontendRepository: gardener/dashboard
Length of output: 50374
🏁 Script executed:
# Check useManagedSeedShoot composable to understand if conditions can be null
cd frontend/src/composables && grep -r "useManagedSeedShoot" --include="*.js" -l | head -5Repository: gardener/dashboard
Length of output: 91
🏁 Script executed:
# Examine useManagedSeedShoot function definition
cat frontend/src/composables/useManagedSeedShoot.jsRepository: gardener/dashboard
Length of output: 141
🏁 Script executed:
# Search for where managedSeedShoot is assigned/updated to see if conditions can be null
rg -n "status.*conditions|conditions.*null" frontend/src --type js -A 2 -B 2 | head -80Repository: gardener/dashboard
Length of output: 3556
🏁 Script executed:
# Check if managedSeedShoot is coming from a store and examine how it's populated
rg -n "managedSeedShootStore\|useManagedSeedShoot" frontend/src/composables --type js -A 3 | head -50Repository: gardener/dashboard
Length of output: 44
🏁 Script executed:
# Look at the actual test for useSeedEffectiveConditions if it exists
fd -e "spec.js" -e "test.js" | xargs grep -l "useSeedEffectiveConditions" 2>/dev/nullRepository: gardener/dashboard
Length of output: 46
🏁 Script executed:
# Check the GSeedStatusTags.vue component that uses this composable to understand usage
grep -A 10 -B 5 "useSeedEffectiveConditions\|effectiveConditions" frontend/src/components/GSeedStatusTags.vue | head -40Repository: gardener/dashboard
Length of output: 880
🏁 Script executed:
# Find where managedSeedShoot comes from in useManagedSeedShootForSeed
cat frontend/src/composables/useManagedSeedShootForSeed.jsRepository: gardener/dashboard
Length of output: 1971
🏁 Script executed:
# Check managedSeedShoot store to see if it can provide null values
fd "managedSeedShoot" frontend/src/store --type fRepository: gardener/dashboard
Length of output: 100
🏁 Script executed:
# Look at how managedSeedShoot is used and what shootByName returns
cat frontend/src/store/managedSeedShoot.js | head -100Repository: gardener/dashboard
Length of output: 1709
🏁 Script executed:
# Check if there are any tests for useSeedEffectiveConditions
find frontend/__tests__ -name "*useSeedEffectiveConditions*" -o -name "*SeedEffectiveConditions*"Repository: gardener/dashboard
Length of output: 44
🏁 Script executed:
# Check broader test coverage for this composable
grep -r "useSeedEffectiveConditions\|effectiveConditions" frontend/__tests__ --include="*.js" | head -20Repository: gardener/dashboard
Length of output: 44
Treat null as missing and fall back to seed conditions.
On line 23, only undefined is treated as absent. If managedSeedShootConditions.value is null, the function returns null instead of falling back to seedConditions.value, which can break downstream condition rendering.
💡 Proposed fix
- if (managedSeedShootConditions.value !== undefined) {
+ if (managedSeedShootConditions.value != null) {
return managedSeedShootConditions.value
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (managedSeedShootConditions.value !== undefined) { | |
| return managedSeedShootConditions.value | |
| } | |
| return seedConditions.value | |
| if (managedSeedShootConditions.value != null) { | |
| return managedSeedShootConditions.value | |
| } | |
| return seedConditions.value |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/composables/useSeedEffectiveConditions.js` around lines 23 - 27,
The code currently treats only undefined as missing and returns
managedSeedShootConditions.value even when it's null; update the check in
useSeedEffectiveConditions (or replace the conditional) so that null is treated
as missing too — e.g., check for non-nullish value
(managedSeedShootConditions.value != null) or use the nullish coalescing
behavior to return seedConditions.value when managedSeedShootConditions.value is
null or undefined, ensuring downstream rendering gets seedConditions.value
instead of null.
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:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note:
Summary by CodeRabbit
New Features
Tests
Refactor