Conversation
Extracts data-fetching and transformation logic from DataStoreList.vue into a dedicated composable with exported pure helpers for testing.
…projects in DataStoreProjectInitializer
…eProjectInitializer directly
…redundant dropdown field
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughCentralized DataStore listing into a new composable, moved project fetching into the initializer, extracted a help-panel component, added datastore cleanup API and UI handling, extended types/utilities, updated tests/fixtures, and added Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "DataStoreList UI"
participant Composable as "useDataStoreList"
participant API as "Api / Services"
UI->>Composable: await useDataStoreList()
Composable->>API: GET /services (getDataStores lazy)
Composable->>API: GET /projects (getProjects lazy)
API-->>Composable: services payload
API-->>Composable: projects payload
Composable->>Composable: format rows, filter kong-admin-service, enrich routes (projectId)
Composable-->>UI: { dataStores, projectNameMap, loading=false, refresh }
UI->>Composable: refresh() (on user action)
Composable->>API: repeat fetch/update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 docstrings
🧪 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: 9
🧹 Nitpick comments (2)
app/components/data-stores/create/DataStoreHelpBox.vue (1)
5-5: NarrowhelpFieldprop type toHelpTextField | undefinedfor type safety.Line 5 currently accepts any string, which weakens compile-time guarantees for the
HelpTextField-based branches. Prefer the enum type directly.Suggested fix
-const props = defineProps<{ helpField: string | undefined }>(); +const props = defineProps<{ helpField: HelpTextField | undefined }>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/data-stores/create/DataStoreHelpBox.vue` at line 5, Change the helpField prop type from a generic string to the specific enum type HelpTextField by modifying the defineProps call (replace `{ helpField: string | undefined }` with `{ helpField: HelpTextField | undefined }`) and ensure the HelpTextField enum is imported or referenced in this file so the compiler recognizes it; update any usages relying on string behavior to use the enum values if necessary.app/components/data-stores/create/DataStoreProjectInitializer.vue (1)
138-142: Non-null assertion relies on implicit validation.The
!assertion onselectedProject.value!.iddepends on prior validation failing if no project is selected. Consider adding explicit validation forselectedProjectfor clarity and safety against future refactoring.♻️ Proposed fix to add explicit validation
const configSettings: BodyKongInitializeKongInitializePost = { datastore: datastoreSettings, - project_id: selectedProject.value!.id, + project_id: selectedProject.value?.id ?? "", ds_type: selectedDataStoreType.value, };Or add explicit validation before constructing
configSettings:if (!selectedProject.value) { toast.add({ severity: "error", summary: "Value missing", detail: "Project is not selected!", life: 5000, }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/data-stores/create/DataStoreProjectInitializer.vue` around lines 138 - 142, The code currently uses a non-null assertion selectedProject.value!.id when building configSettings (BodyKongInitializeKongInitializePost); add an explicit check for selectedProject.value before constructing configSettings (e.g., if (!selectedProject.value) { show an error toast and return }) so you avoid relying on implicit validation and prevent runtime crashes—place this check just before the configSettings assignment (the block that references datastoreSettings, project_id, and ds_type).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/data-stores/create/DataStoreHelpPanel.vue`:
- Around line 13-18: The icon-only close Button in DataStoreHelpPanel.vue lacks
an accessible name; add an explicit accessible label by updating the Button (the
one with class "help-box-close-btn" and `@click`="emit('closeHelp')") to include
aria-label="Close help box" (or aria-labelledby pointing to a visible label) so
screen readers get a clear name instead of relying solely on v-tooltip.top; keep
the tooltip but ensure aria-label matches its text.
In `@app/components/data-stores/create/DataStoreProjectInitializer.vue`:
- Around line 33-43: The code calling getProjectNodes() lacks handling for
non-success or thrown errors, leaving availableProjects empty and the user
uninformed; wrap the call in try/catch and handle the projStatus !== "success"
case by setting an error state (e.g., projectLoadError or similar reactive ref)
and logging the error, and populate that error/ref so the template (near the
project dropdown) can show a user-facing message; update places referencing
projects, projStatus, and availableProjects to use this error state when the
fetch fails.
In `@app/components/data-stores/DataStoreList.vue`:
- Around line 8-9: You are optimistically removing an item by filtering
dataStores.value then immediately calling refresh(), which repopulates
dataStores from the server and can reintroduce the just-deleted store; instead,
either delay calling refresh() until the delete API confirms success (call
refresh() in the delete promise's success handler) or maintain a
pending-deletions set (e.g., track dsName in a pendingDeletes collection used to
filter incoming data in useDataStoreList) so that refresh() respects optimistic
removals until the server acknowledges deletion; update the code paths around
dataStores.value, refresh(), useDataStoreList(), and the delete handler to
implement one of these approaches.
In `@app/composables/useDataStoreList.ts`:
- Around line 19-20: extractProjectIdFromPath dereferences paths[0]! without
guarding against undefined/empty arrays which can throw at runtime; update the
function to safely handle missing or empty paths by using checks or optional
chaining (e.g., verify paths && paths.length>0 or use paths?.[0]) and return
undefined if the first path or the second segment is absent, instead of using
the non-null assertion; keep the function name extractProjectIdFromPath and
ensure it returns the segment at index 1 only when present.
- Around line 43-47: The code is unsafely asserting formatDataRow(...) as
ModifiedDetailedService[] before timestamps are normalized; fix by removing the
direct cast and explicitly normalize/add created_at and updated_at on the
formatted rows (use formatDataRow and then map over its result) so each item has
both timestamp objects (converted to the modifiedTimestamp shape used by
ModifiedDetailedService) before casting or returning them; reference
formatDataRow, ModifiedDetailedService, and the optional
DetailedService.created_at/updated_at fields (format-data-row.ts currently only
rewrites keys that exist) and ensure you fill or transform missing timestamp
fields in the mapping step so no .created_at.short or .updated_at.short access
can fail at runtime.
- Around line 53-60: Remove the unsafe cast "as Route[]" when assigning the
result of formatDataRow to store.routes; formatDataRow mutates timestamp fields
and adds an expand property so you must either update the type of store.routes
to a proper transformed Route shape (create a TransformedRoute/ExpandedRoute
interface including created_at/updated_at objects and expand) or assign to a
correctly typed temporary variable (e.g. const transformed = formatDataRow(...))
and then set store.routes = transformed; also update the subsequent forEach to
use the transformed type and keep the call to extractProjectIdFromPath as-is to
populate projectId.
In `@app/utils/format-data-row.ts`:
- Around line 17-20: The exported formatDataRow signature accepts rowEntries:
ProjectNode[] | DetailedService[] but the .map() callback and
parseUnixTimestamp() types exclude DetailedService; create a single union type
(e.g., FormattableRow = ProjectNode | DetailedService) and replace the separate
unions in the formatDataRow parameter, the .map() callback's input type, and the
parseUnixTimestamp() parameter type with this FormattableRow to ensure
consistency and remove the need for downstream type assertions; update any
references to ProjectNode/DetailedService in those function signatures
(formatDataRow, parseUnixTimestamp, and the anonymous map callback) to use
FormattableRow.
In `@test/components/data-stores/create/DataStoreProjectInitializer.spec.ts`:
- Around line 1-3: The test imports defineComponent from Vue but uses ref() in
the mock, causing a runtime error; update the import statement in
DataStoreProjectInitializer.spec.ts to also import ref from "vue" (add ref
alongside defineComponent) so the mock that calls ref() resolves correctly and
the test can execute.
In `@test/composables/useDataStoreList.test.ts`:
- Line 13: Tests are using "as any" to bypass types when calling
buildProjectNameMap; remove those casts and either (A) supply properly typed
Project fixtures in the test (create objects matching the Project interface,
using explicit name: undefined or id: undefined where needed) for the calls in
the useDataStoreList.test.ts test, or (B) if the function truly accepts looser
shapes, narrow the function signature of buildProjectNameMap to accept Array<{
id?: any; name?: any }> so the existing test data is valid; update the test
cases referencing buildProjectNameMap to use the chosen approach and remove all
"as any" casts.
---
Nitpick comments:
In `@app/components/data-stores/create/DataStoreHelpBox.vue`:
- Line 5: Change the helpField prop type from a generic string to the specific
enum type HelpTextField by modifying the defineProps call (replace `{ helpField:
string | undefined }` with `{ helpField: HelpTextField | undefined }`) and
ensure the HelpTextField enum is imported or referenced in this file so the
compiler recognizes it; update any usages relying on string behavior to use the
enum values if necessary.
In `@app/components/data-stores/create/DataStoreProjectInitializer.vue`:
- Around line 138-142: The code currently uses a non-null assertion
selectedProject.value!.id when building configSettings
(BodyKongInitializeKongInitializePost); add an explicit check for
selectedProject.value before constructing configSettings (e.g., if
(!selectedProject.value) { show an error toast and return }) so you avoid
relying on implicit validation and prevent runtime crashes—place this check just
before the configSettings assignment (the block that references
datastoreSettings, project_id, and ds_type).
🪄 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: 199658db-125f-4d70-b457-6d7fd46ccce1
📒 Files selected for processing (14)
.gitignoreapp/components/data-stores/DataStoreList.vueapp/components/data-stores/create/DataStoreHelpBox.vueapp/components/data-stores/create/DataStoreHelpPanel.vueapp/components/data-stores/create/DataStoreProjectInitializer.vueapp/components/data-stores/create/ResourceManagerTabs.vueapp/components/data-stores/create/index.tsapp/composables/useDataStoreList.tsapp/pages/data-stores/create.vueapp/utils/format-data-row.tstest/components/analysis/AnalysisControlButtons.spec.tstest/components/data-stores/constants.tstest/components/data-stores/create/DataStoreProjectInitializer.spec.tstest/composables/useDataStoreList.test.ts
💤 Files with no reviewable changes (2)
- test/components/data-stores/constants.ts
- app/components/data-stores/create/ResourceManagerTabs.vue
Comment on lines
+8
to
+9
| dataStores.value = dataStores.value.filter((store) => store.name !== dsName); | ||
| refresh(); |
There was a problem hiding this comment.
Avoid overwriting the optimistic delete with an immediate refresh.
refresh() causes useDataStoreList() to repopulate dataStores.value from the server. If the deletion has not propagated yet, the just-filtered row gets written back and the table shows the deleted store again. Refresh only after the delete is confirmed, or keep a pending-deletions filter when applying fresh data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/data-stores/DataStoreList.vue` around lines 8 - 9, You are
optimistically removing an item by filtering dataStores.value then immediately
calling refresh(), which repopulates dataStores from the server and can
reintroduce the just-deleted store; instead, either delay calling refresh()
until the delete API confirms success (call refresh() in the delete promise's
success handler) or maintain a pending-deletions set (e.g., track dsName in a
pendingDeletes collection used to filter incoming data in useDataStoreList) so
that refresh() respects optimistic removals until the server acknowledges
deletion; update the code paths around dataStores.value, refresh(),
useDataStoreList(), and the delete handler to implement one of these approaches.
| { id: "abc-123", name: "My Project" }, | ||
| { id: "def-456", name: "Other Project" }, | ||
| ]; | ||
| const map = buildProjectNameMap(projects as any); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f useDataStoreListRepository: PrivateAIM/node-ui
Length of output: 139
🏁 Script executed:
cat -n test/composables/useDataStoreList.test.tsRepository: PrivateAIM/node-ui
Length of output: 1818
🏁 Script executed:
cat -n app/composables/useDataStoreList.tsRepository: PrivateAIM/node-ui
Length of output: 2935
🏁 Script executed:
cat -n app/composables/useDataStoreList.ts | head -20Repository: PrivateAIM/node-ui
Length of output: 936
🏁 Script executed:
fd -t f Api.tsRepository: PrivateAIM/node-ui
Length of output: 81
🏁 Script executed:
rg "export (type|interface) (Project|DetailedAnalysis)" app/services/Api.ts -A 10Repository: PrivateAIM/node-ui
Length of output: 650
🏁 Script executed:
rg "export interface Project\b" app/services/Api.ts -A 20Repository: PrivateAIM/node-ui
Length of output: 438
🏁 Script executed:
rg "no-explicit-any" . -t ts --type-add 'ts:*.ts' | head -20Repository: PrivateAIM/node-ui
Length of output: 142
🏁 Script executed:
fd -t f "eslint" -E node_modulesRepository: PrivateAIM/node-ui
Length of output: 78
🏁 Script executed:
cat eslint.config.jsRepository: PrivateAIM/node-ui
Length of output: 892
Remove the as any casts from test fixtures.
The test fixtures at these lines don't match the declared Project[] | DetailedAnalysis[] parameter type and use as any to bypass TypeScript checking. Replace with properly typed test data that matches the actual function contract:
- Line 13: valid
Projectshape - Line 20:
Projectwith undefined name (usename: undefinedwithout casting, or adjust type) - Line 26:
Projectwith undefined id (useid: undefinedwithout casting, or adjust type)
Alternatively, narrow the function's input type to Array<{ id?: any; name?: any }> to match what it actually reads.
Also applies to: 20-20, 26-26
🧰 Tools
🪛 ESLint
[error] 13-13: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/composables/useDataStoreList.test.ts` at line 13, Tests are using "as
any" to bypass types when calling buildProjectNameMap; remove those casts and
either (A) supply properly typed Project fixtures in the test (create objects
matching the Project interface, using explicit name: undefined or id: undefined
where needed) for the calls in the useDataStoreList.test.ts test, or (B) if the
function truly accepts looser shapes, narrow the function signature of
buildProjectNameMap to accept Array<{ id?: any; name?: any }> so the existing
test data is valid; update the test cases referencing buildProjectNameMap to use
the chosen approach and remove all "as any" casts.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/components/header/CleanupDialog.vue (2)
130-134: Edge case: count of 0 won't be displayed.If the API returns
count: 0orzombies: "0", the conditionif (modifiedResources)evaluates to falsy, so the message won't include the count. If displaying "0 modified resources" is meaningful feedback, consider using an explicit check.♻️ Optional: Show count even when zero
if (cleanupResponse) { let detail = "Cleanup request successfully submitted"; - if (modifiedResources) { + if (modifiedResources !== 0 && modifiedResources !== "") { detail += ", number of modified resources: " + modifiedResources; }Or to always show it:
- if (modifiedResources) { - detail += ", number of modified resources: " + modifiedResources; - } + detail += ", number of modified resources: " + modifiedResources;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/header/CleanupDialog.vue` around lines 130 - 134, The message-building logic around cleanupResponse uses a truthy check on modifiedResources which hides legitimate zero counts; update the condition in the CleanupDialog.vue block that appends to the detail string (the variables detail, modifiedResources and cleanupResponse) to explicitly check for null/undefined (e.g., modifiedResources != null or typeof modifiedResources !== "undefined") so that a value of 0 or "0" is still included in the message; ensure the same explicit check is used for any similar zombies/count fields so zero is displayed.
94-128: Error handling pattern could swallow the actual error details.The
.catch()handlers show a generic error toast but discard the actual error. Consider logging the error or displaying more specific error information to aid debugging.Additionally, the type assertion after
.catch()is technically incorrect since.catch()returnsvoid(making the expression potentiallyundefined), though it's safely guarded by theif (cleanupResponse)check at line 130.♻️ Optional: Capture and log error details
if (endpoint === "datastore") { cleanupResponse = (await useNuxtApp() .$hubApi(`/kong/${endpoint}`, { method: "DELETE", }) - .catch(() => { + .catch((error) => { + console.error("Datastore cleanup failed:", error); toast.add({ severity: "error", summary: "Unable to perform cleanup", detail: "There was an error while deleting or restarting the resource.", life: 5000, }); })) as DeleteService;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/header/CleanupDialog.vue` around lines 94 - 128, The current .catch() swallows the error and the inline type assertions after the chained .catch() are unsafe; wrap the await useNuxtApp().$hubApi(...) calls in a proper try/catch instead of chaining .catch(), e.g., in CleanupDialog.vue call the API inside a try block and assign the result to a typed variable (cleanupResponse typed as DeleteService | CleanupPodResponse | null), and in the catch(err) log the full error (console.error or processLogger) and include meaningful info in the toast (err.message or a parsed error detail) so you don't lose diagnostics; update both branches that call useNuxtApp().$hubApi and remove the post-.catch() type assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/components/header/CleanupDialog.vue`:
- Around line 130-134: The message-building logic around cleanupResponse uses a
truthy check on modifiedResources which hides legitimate zero counts; update the
condition in the CleanupDialog.vue block that appends to the detail string (the
variables detail, modifiedResources and cleanupResponse) to explicitly check for
null/undefined (e.g., modifiedResources != null or typeof modifiedResources !==
"undefined") so that a value of 0 or "0" is still included in the message;
ensure the same explicit check is used for any similar zombies/count fields so
zero is displayed.
- Around line 94-128: The current .catch() swallows the error and the inline
type assertions after the chained .catch() are unsafe; wrap the await
useNuxtApp().$hubApi(...) calls in a proper try/catch instead of chaining
.catch(), e.g., in CleanupDialog.vue call the API inside a try block and assign
the result to a typed variable (cleanupResponse typed as DeleteService |
CleanupPodResponse | null), and in the catch(err) log the full error
(console.error or processLogger) and include meaningful info in the toast
(err.message or a parsed error detail) so you don't lose diagnostics; update
both branches that call useNuxtApp().$hubApi and remove the post-.catch() type
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d132a740-9559-45d0-8bed-03332b8595ae
📒 Files selected for processing (3)
app/components/header/CleanupDialog.vueapp/services/Api.tsapp/services/hub_adapter_swagger.json
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.
Update the data store components to use
computed()where needed, refactor certain services as composables, and remove orphaned componentsSummary by CodeRabbit
Refactor
New Features
Tests