feat: Seed List#2776
Merged
Merged
Conversation
petersutter
requested review from
grolu,
holgerkoser and
klocke-io
as code owners
gardener-prow
Bot
added
do-not-merge/needs-kind
petersutter
force-pushed
the
enh/seed-list
branch
from
9d7ac22 to
f286e7a
Compare
|
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 Seeds management UI and SeedList route plus multiple new components and composables for sorting, filtering, status normalization, and URL-synced search; updates navigation, routes, store persistence, and adds comprehensive unit tests for the new helpers and composables. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant View as GSeedList
participant UrlSync as useUrlSearchSync
participant Filter as useTableFilter
participant Sorting as useSeedTableSorting
participant Store as LocalStore
User->>View: open Seeds view
View->>UrlSync: init search sync (param 'q')
UrlSync-->>View: reactive search ref
View->>Sorting: init sorting (load seedSortBy)
Sorting->>Store: read seedSortBy
Sorting-->>View: sortBy & comparators
View->>Filter: init filtering (items, filterFn, searchQuery)
Filter-->>View: filteredItems
User->>View: change search / change sort / toggle columns
View->>UrlSync: update search -> sync URL
View->>Filter: compute filtered items
View->>Sorting: sort filtered items -> sorted items
View->>Store: persist selectedColumns, seedSortBy
View-->>User: render updated table
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
gardener-prow
Bot
removed
the
do-not-merge/needs-kind
![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
🤖 Fix all issues with AI agents
In `@frontend/src/components/GSeedStatus.vue`:
- Around line 140-142: The computed property popoverKey accesses
seedMetadata.value.uid directly which will throw if seedMetadata.value is
undefined; update the popoverKey computed (the computed named popoverKey) to
safely handle missing seedMetadata.value by checking for existence (or using a
fallback/empty string or 'unknown' id) before concatenating the UID so the
template doesn't error when seedMetadata.value is undefined.
- Around line 106-118: The component defines a required prop popperKey but never
uses it—popoverKey is computed from seedMetadata.value.uid instead—so either
remove the unused prop or use it; to fix, pick one: (a) delete the popperKey
entry from defineProps and update any parent usages, or (b) change the computed
popoverKey to use the prop (popperKey) as the primary value (falling back to
seedMetadata.value.uid if needed) and remove the required flag if fallback is
allowed; locate defineProps, the computed popoverKey, and any references to
seedMetadata.value.uid to implement the chosen option.
In `@frontend/src/components/GTableSearch.vue`:
- Around line 10-26: The Esc key handler currently emits null which breaks the
String-typed v-model contract; update the `@keyup.esc` handler on the v-text-field
(the element binding tooltipProps and :model-value="modelValue") to emit an
empty string instead of null (i.e., emit('update:modelValue', '') ) so
modelValue remains a String default ('') and downstream code keeps correct
types.
In `@frontend/src/composables/useOperationStatus.js`:
- Around line 23-29: The computed getters operationType and operationState (and
similarly defaultLastMessage) assume lastOperation.value is non-null and will
throw if it's null/undefined; update them to defensively access
lastOperation.value by checking existence first (e.g., use lastOperation.value
&& lastOperation.value.type / .state or optional chaining
lastOperation.value?.type) and fall back to the existing defaults ('Create' and
'Pending' and defaultLastMessage's fallback) so the computed functions return
safe values when lastOperation.value is null/undefined; ensure you update the
operationType, operationState, and defaultLastMessage definitions accordingly.
In `@frontend/src/composables/useTableSorting/helper.js`:
- Around line 19-42: In getReadinessSortVal's iteratee,
configStore.conditionForType(type) can be undefined causing .sortOrder to throw;
change the code to safely resolve a default sortOrder when
conditionForType(type) is missing (e.g. use optional chaining or a nullish
coalescing fallback and a numeric/default string like 99999999), then padStart
that resolved value and proceed—update the iteratee in getReadinessSortVal to
call configStore.conditionForType(type)?.sortOrder ?? <default> and use that
instead of accessing .sortOrder directly.
- Around line 44-83: In getLastOperationSortVal update the inProgress checks so
0 is treated as in-progress by testing for null/undefined rather than
truthiness; replace occurrences of "!!operation.progress" with a explicit
null/undefined check (e.g., operation.progress !== null && operation.progress
!== undefined) and ensure the existing check operation.progress !== 100 remains,
so inProgress becomes operation.progress !== null && operation.progress !==
undefined && operation.progress !== 100 && operation.state !== 'Failed'.
In `@frontend/src/router/routes.js`:
- Around line 281-301: The SeedList route (seedListRoute) currently only hides
the menu via meta but allows direct access; replace its beforeEnter:
addNamespaceToUrl with an admin-checking guard (or a composed guard) that first
verifies authnStore.isAdmin and aborts/redirects non-admins (e.g., next({ name:
'NotFound' }) or redirect to home), and only if the user is admin invoke
addNamespaceToUrl to continue; update the route's beforeEnter to call this admin
guard (or compose requireAdminBeforeEnter with addNamespaceToUrl) so non-admins
cannot navigate directly to the SeedList view.
🧹 Nitpick comments (2)
frontend/__tests__/composables/useTableFilter/index.spec.js (1)
117-133: Consider adding tests for ref validation.The composable throws
TypeErrorwhenitemsorsearchQueryare not refs, but these error paths are not tested. Adding tests for these cases would improve coverage.📝 Suggested additional test cases
it('should throw error when items is not a ref', () => { const searchQuery = ref('') const filterFn = (item, query) => item.name.includes(query) expect(() => { useTableFilter({ items: [], searchQuery, filterFn }) }).toThrow('items must be a ref object') }) it('should throw error when searchQuery is not a ref', () => { const items = ref([]) const filterFn = (item, query) => item.name.includes(query) expect(() => { useTableFilter({ items, searchQuery: '', filterFn }) }).toThrow('searchQuery must be a ref object') })frontend/src/composables/useTableSorting/index.js (1)
13-24: Avoid mutating caller-provideddefaultSortBy
ref(defaultSortBy)keeps the same array reference; downstream mutations can leak back to the caller. A shallow copy prevents accidental coupling.♻️ Suggested change
- const sortBy = ref(defaultSortBy) + const sortBy = ref(defaultSortBy.map(item => ({ ...item })))
Comment on lines
+140
to
+142
| const popoverKey = computed(() => { | ||
| return `g-seed-status:${seedMetadata.value.uid}` | ||
| }) |
There was a problem hiding this comment.
Add null safety for seedMetadata.value access.
If seedMetadata.value is undefined, accessing .uid will throw a runtime error.
🛡️ Proposed fix
const popoverKey = computed(() => {
- return `g-seed-status:${seedMetadata.value.uid}`
+ return `g-seed-status:${seedMetadata.value?.uid ?? ''}`
})📝 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
| const popoverKey = computed(() => { | |
| return `g-seed-status:${seedMetadata.value.uid}` | |
| }) | |
| const popoverKey = computed(() => { | |
| return `g-seed-status:${seedMetadata.value?.uid ?? ''}` | |
| }) |
🤖 Prompt for AI Agents
In `@frontend/src/components/GSeedStatus.vue` around lines 140 - 142, The computed
property popoverKey accesses seedMetadata.value.uid directly which will throw if
seedMetadata.value is undefined; update the popoverKey computed (the computed
named popoverKey) to safely handle missing seedMetadata.value by checking for
existence (or using a fallback/empty string or 'unknown' id) before
concatenating the UID so the template doesn't error when seedMetadata.value is
undefined.
Comment on lines
+23
to
+29
| const operationType = computed(() => { | ||
| return lastOperation.value.type || 'Create' | ||
| }) | ||
|
|
||
| const operationState = computed(() => { | ||
| return lastOperation.value.state || 'Pending' | ||
| }) |
There was a problem hiding this comment.
Add null safety for lastOperation.value access.
If lastOperation.value is null or undefined, accessing .type or .state will throw a runtime error. Consider adding defensive checks.
🛡️ Proposed fix
const operationType = computed(() => {
- return lastOperation.value.type || 'Create'
+ return lastOperation.value?.type || 'Create'
})
const operationState = computed(() => {
- return lastOperation.value.state || 'Pending'
+ return lastOperation.value?.state || 'Pending'
})Also apply similar changes to defaultLastMessage:
const defaultLastMessage = computed(() => {
- return lastOperation.value.description || 'No description'
+ return lastOperation.value?.description || 'No description'
})📝 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
| const operationType = computed(() => { | |
| return lastOperation.value.type || 'Create' | |
| }) | |
| const operationState = computed(() => { | |
| return lastOperation.value.state || 'Pending' | |
| }) | |
| const operationType = computed(() => { | |
| return lastOperation.value?.type || 'Create' | |
| }) | |
| const operationState = computed(() => { | |
| return lastOperation.value?.state || 'Pending' | |
| }) |
🤖 Prompt for AI Agents
In `@frontend/src/composables/useOperationStatus.js` around lines 23 - 29, The
computed getters operationType and operationState (and similarly
defaultLastMessage) assume lastOperation.value is non-null and will throw if
it's null/undefined; update them to defensively access lastOperation.value by
checking existence first (e.g., use lastOperation.value &&
lastOperation.value.type / .state or optional chaining
lastOperation.value?.type) and fall back to the existing defaults ('Create' and
'Pending' and defaultLastMessage's fallback) so the computed functions return
safe values when lastOperation.value is null/undefined; ensure you update the
operationType, operationState, and defaultLastMessage definitions accordingly.
Comment on lines
+19
to
+42
| export function getReadinessSortVal ({ | ||
| conditions, | ||
| lastOperationTime, | ||
| creationTime, | ||
| isErrorFn, | ||
| configStore, | ||
| }) { | ||
| if (!conditions.length) { | ||
| // items without conditions have medium priority | ||
| const priority = '00000100' | ||
| const time = lastOperationTime ?? creationTime | ||
| return `${priority}-${time}` | ||
| } | ||
| const iteratee = ({ type, status = 'True', lastTransitionTime = '1970-01-01T00:00:00Z' }) => { | ||
| const isError = isErrorFn(status) | ||
| // items without any error have lowest priority | ||
| const priority = !isError | ||
| ? '99999999' | ||
| : padStart(configStore.conditionForType(type).sortOrder, 8, '0') | ||
| return `${priority}-${lastTransitionTime}` | ||
| } | ||
| // the condition with the lowest priority and transitionTime is used | ||
| return head(conditions.map(iteratee).sort()) | ||
| } |
There was a problem hiding this comment.
Guard against unknown condition types to avoid sort crashes
configStore.conditionForType(type) can be undefined for new/unknown condition types, which will throw on .sortOrder and break list rendering. Please add a safe fallback.
🛠️ Suggested hardening
- const priority = !isError
- ? '99999999'
- : padStart(configStore.conditionForType(type).sortOrder, 8, '0')
+ const sortOrder = configStore?.conditionForType?.(type)?.sortOrder
+ const priority = !isError
+ ? '99999999'
+ : padStart(sortOrder ?? 0, 8, '0')📝 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
| export function getReadinessSortVal ({ | |
| conditions, | |
| lastOperationTime, | |
| creationTime, | |
| isErrorFn, | |
| configStore, | |
| }) { | |
| if (!conditions.length) { | |
| // items without conditions have medium priority | |
| const priority = '00000100' | |
| const time = lastOperationTime ?? creationTime | |
| return `${priority}-${time}` | |
| } | |
| const iteratee = ({ type, status = 'True', lastTransitionTime = '1970-01-01T00:00:00Z' }) => { | |
| const isError = isErrorFn(status) | |
| // items without any error have lowest priority | |
| const priority = !isError | |
| ? '99999999' | |
| : padStart(configStore.conditionForType(type).sortOrder, 8, '0') | |
| return `${priority}-${lastTransitionTime}` | |
| } | |
| // the condition with the lowest priority and transitionTime is used | |
| return head(conditions.map(iteratee).sort()) | |
| } | |
| export function getReadinessSortVal ({ | |
| conditions, | |
| lastOperationTime, | |
| creationTime, | |
| isErrorFn, | |
| configStore, | |
| }) { | |
| if (!conditions.length) { | |
| // items without conditions have medium priority | |
| const priority = '00000100' | |
| const time = lastOperationTime ?? creationTime | |
| return `${priority}-${time}` | |
| } | |
| const iteratee = ({ type, status = 'True', lastTransitionTime = '1970-01-01T00:00:00Z' }) => { | |
| const isError = isErrorFn(status) | |
| // items without any error have lowest priority | |
| const sortOrder = configStore?.conditionForType?.(type)?.sortOrder | |
| const priority = !isError | |
| ? '99999999' | |
| : padStart(sortOrder ?? 0, 8, '0') | |
| return `${priority}-${lastTransitionTime}` | |
| } | |
| // the condition with the lowest priority and transitionTime is used | |
| return head(conditions.map(iteratee).sort()) | |
| } |
🤖 Prompt for AI Agents
In `@frontend/src/composables/useTableSorting/helper.js` around lines 19 - 42, In
getReadinessSortVal's iteratee, configStore.conditionForType(type) can be
undefined causing .sortOrder to throw; change the code to safely resolve a
default sortOrder when conditionForType(type) is missing (e.g. use optional
chaining or a nullish coalescing fallback and a numeric/default string like
99999999), then padStart that resolved value and proceed—update the iteratee in
getReadinessSortVal to call configStore.conditionForType(type)?.sortOrder ??
<default> and use that instead of accessing .sortOrder directly.
Comment on lines
+44
to
+83
| export function getLastOperationSortVal ({ | ||
| operation = {}, | ||
| lastErrors = [], | ||
| metadata = {}, | ||
| status = {}, | ||
| isUserErrorFn = ({ lastErrors }) => isUserError(errorCodesFromArray(lastErrors)), | ||
| }) { | ||
| const isError = operation.state === 'Failed' || lastErrors.length | ||
| const ignoredFromReconciliation = isReconciliationDeactivated(metadata) | ||
|
|
||
| if (ignoredFromReconciliation) { | ||
| return isError | ||
| ? 400 | ||
| : 450 | ||
| } | ||
|
|
||
| const userError = isUserErrorFn({ | ||
| operation, | ||
| lastErrors, | ||
| metadata, | ||
| status, | ||
| }) | ||
| const inProgress = operation.progress !== 100 && operation.state !== 'Failed' && !!operation.progress | ||
|
|
||
| if (userError) { | ||
| return inProgress | ||
| ? Number('3' + padStart(operation.progress, 2, '0')) | ||
| : 200 | ||
| } | ||
| if (isError) { | ||
| return inProgress | ||
| ? Number('1' + padStart(operation.progress, 2, '0')) | ||
| : 0 | ||
| } | ||
| return inProgress | ||
| ? Number('6' + padStart(operation.progress, 2, '0')) | ||
| : isStatusHibernated(status) | ||
| ? 500 | ||
| : 700 | ||
| } |
There was a problem hiding this comment.
Treat 0% progress as in-progress
!!operation.progress ignores 0, so a 0%-progress operation gets sorted like a completed one. Consider checking for null/undefined instead.
🛠️ Suggested fix
- const inProgress = operation.progress !== 100 && operation.state !== 'Failed' && !!operation.progress
+ const hasProgress = operation.progress !== undefined && operation.progress !== null
+ const inProgress = hasProgress && operation.progress !== 100 && operation.state !== 'Failed'📝 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
| export function getLastOperationSortVal ({ | |
| operation = {}, | |
| lastErrors = [], | |
| metadata = {}, | |
| status = {}, | |
| isUserErrorFn = ({ lastErrors }) => isUserError(errorCodesFromArray(lastErrors)), | |
| }) { | |
| const isError = operation.state === 'Failed' || lastErrors.length | |
| const ignoredFromReconciliation = isReconciliationDeactivated(metadata) | |
| if (ignoredFromReconciliation) { | |
| return isError | |
| ? 400 | |
| : 450 | |
| } | |
| const userError = isUserErrorFn({ | |
| operation, | |
| lastErrors, | |
| metadata, | |
| status, | |
| }) | |
| const inProgress = operation.progress !== 100 && operation.state !== 'Failed' && !!operation.progress | |
| if (userError) { | |
| return inProgress | |
| ? Number('3' + padStart(operation.progress, 2, '0')) | |
| : 200 | |
| } | |
| if (isError) { | |
| return inProgress | |
| ? Number('1' + padStart(operation.progress, 2, '0')) | |
| : 0 | |
| } | |
| return inProgress | |
| ? Number('6' + padStart(operation.progress, 2, '0')) | |
| : isStatusHibernated(status) | |
| ? 500 | |
| : 700 | |
| } | |
| export function getLastOperationSortVal ({ | |
| operation = {}, | |
| lastErrors = [], | |
| metadata = {}, | |
| status = {}, | |
| isUserErrorFn = ({ lastErrors }) => isUserError(errorCodesFromArray(lastErrors)), | |
| }) { | |
| const isError = operation.state === 'Failed' || lastErrors.length | |
| const ignoredFromReconciliation = isReconciliationDeactivated(metadata) | |
| if (ignoredFromReconciliation) { | |
| return isError | |
| ? 400 | |
| : 450 | |
| } | |
| const userError = isUserErrorFn({ | |
| operation, | |
| lastErrors, | |
| metadata, | |
| status, | |
| }) | |
| const hasProgress = operation.progress !== undefined && operation.progress !== null | |
| const inProgress = hasProgress && operation.progress !== 100 && operation.state !== 'Failed' | |
| if (userError) { | |
| return inProgress | |
| ? Number('3' + padStart(operation.progress, 2, '0')) | |
| : 200 | |
| } | |
| if (isError) { | |
| return inProgress | |
| ? Number('1' + padStart(operation.progress, 2, '0')) | |
| : 0 | |
| } | |
| return inProgress | |
| ? Number('6' + padStart(operation.progress, 2, '0')) | |
| : isStatusHibernated(status) | |
| ? 500 | |
| : 700 | |
| } |
🤖 Prompt for AI Agents
In `@frontend/src/composables/useTableSorting/helper.js` around lines 44 - 83, In
getLastOperationSortVal update the inProgress checks so 0 is treated as
in-progress by testing for null/undefined rather than truthiness; replace
occurrences of "!!operation.progress" with a explicit null/undefined check
(e.g., operation.progress !== null && operation.progress !== undefined) and
ensure the existing check operation.progress !== 100 remains, so inProgress
becomes operation.progress !== null && operation.progress !== undefined &&
operation.progress !== 100 && operation.state !== 'Failed'.
251e79b to
150d8ae
Compare
|
@holgerkoser, @klocke-io, @grolu You have pull request review open invite, please check |
petersutter
force-pushed
the
enh/seed-list
branch
from
32b1e25 to
5759f28
Compare
grolu
reviewed
Mar 3, 2026
Member
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: grolu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
gardener-prow
Bot
added
the
approved
Member
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: aad5c81d4928e363336e63c5a78489745f58f9bf |
This was referenced Mar 27, 2026
This was referenced Apr 16, 2026
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
2 participants
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
Improvements
Tests