Conversation
📝 WalkthroughWalkthroughThe PR refactors node type handling into a datastore requirement system. It replaces the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 (2)
components/header/AvatarButton.vue (1)
108-121: Toggle click handling looks correct but consider disabling the toggle until state is loaded.The
.stop/.preventmodifiers properly prevent double-firing. However, whiledatastoreStateis still loading (i.e.,datastoreRequiredisnull), the toggle shows as off and is fully interactive. A click at that point would callsetDatastoreRequired(!null)→setDatastoreRequired(true), which may be surprising if the server already had it set totrue. Consider either disabling the toggle or hiding the menu item until the initial fetch resolves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/AvatarButton.vue` around lines 108 - 121, The toggle is interactive while the initial datastore state is still loading (datastoreRequired === null) which can cause surprising state flips; update the AvatarButton.vue code around the toggle/menu item (the anchor with v-if="item.isToggle", the ToggleSwitch component, and the setDatastoreRequired handler) to either hide the menu item until datastoreRequired is non-null or disable interaction while loading—e.g., render the item only when datastoreRequired !== null or add a disabled guard (both for the <a> click and the ToggleSwitch) so clicks are ignored when datastoreRequired === null; ensure you still respect the existing click modifiers (.prevent/.stop) and only call setDatastoreRequired when datastoreRequired is not null.test/components/header/AvatarButton.spec.ts (1)
25-33: No test coverage for the new toggle behavior.The mock for
useDatastoreRequirementis set up correctly, but no test case exercises the "Require Data Store" toggle — e.g., verifying that clicking it callssetDatastoreRequiredwith the expected value, or that theToggleSwitchreflects thedatastoreRequiredstate. Consider adding a dedicated test.Do you want me to generate a test case for the toggle behavior?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/components/header/AvatarButton.spec.ts` around lines 25 - 33, Add a test that exercises the "Require Data Store" toggle: render the AvatarButton component (or the component wrapper that contains ToggleSwitch), mock useDatastoreRequirement as you already do (with datastoreRequired: ref(true) and setDatastoreRequired = vi.fn()), assert that the ToggleSwitch initially reflects datastoreRequired === true, simulate a user click/trigger on the ToggleSwitch, and assert that setDatastoreRequired was called with the toggled value (false) and that the ToggleSwitch updates accordingly; reference the mocked hook useDatastoreRequirement, the mock function setDatastoreRequired, and the ToggleSwitch within AvatarButton to locate where to add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/analysis/AnalysesTable.vue`:
- Around line 36-50: The current logic assumes useDatastoreRequirement() always
returns a valid datastoreState and treats null as false; update the code that
derives datastoreState/datastoreRequired/nodeType to explicitly handle a null
result by introducing an "unknown" fallback (e.g., datastoreState === null ->
datastoreRequired = null or 'unknown') and update computed properties
datastoreBadgeSeverity and datastoreBadgeTooltip to branch on the unknown case
(e.g., show neutral/severity "warning" and tooltip "Data store status unknown")
instead of defaulting to not-required; also remove any non-null assertions where
the prop is passed so the component receives the explicit null/unknown value and
can render a safe disabled state. Ensure these changes are applied to the same
patterns around datastoreState/datastoreRequired/nodeType usage (including the
other occurrences noted).
In `@components/analysis/AnalysisControlButtons.vue`:
- Around line 43-46: Tests mounting AnalysisControlButtons are missing the
required prop requireDatastore which causes warnings/failures; update both mount
calls in test/components/analysis/AnalysisControlButtons.spec.ts that
instantiate the AnalysisControlButtons component to pass the prop (e.g.,
requireDatastore: true or use the same test fixture value as production like
:requireDatastore="datastoreRequired" or datastoreRequired!) so the mounted
wrapper includes the required Boolean prop; locate the mounts that call
mount(AnalysisControlButtons, { props: {...} }) and add requireDatastore to the
props object.
In `@composables/useAPIFetch.ts`:
- Around line 36-42: getNodeConfiguration is defined but unused; update
useDatastoreRequirement to call getNodeConfiguration(...) instead of directly
invoking useNuxtApp().$hubApi("/node/settings", ...), passing through the same
opts and handling the returned Promise/useAPIFetch result (NodeSettings) so
behavior and typing remain identical; alternatively, if you prefer not to change
useDatastoreRequirement, remove the unused getNodeConfiguration export to avoid
dead code.
In `@composables/useDatastoreRequirement.ts`:
- Line 1: The file is missing the NodeSettings type import used in the type
assertions at lines where "as NodeSettings" appears; update the import line that
currently imports NodeTypeResponse to also import NodeSettings from the same
module (e.g., add NodeSettings to the import from "~/services/Api") so the type
assertions in useDatastoreRequirement (references to NodeSettings) compile
without TypeScript errors.
- Around line 41-55: The setter setDatastoreRequired currently writes
nodeConfigResp.data_required directly and ignores the aggregator-type rule;
update setDatastoreRequired to inspect nodeConfigResp.type and only set
datastoreState.value.datastoreRequired to nodeConfigResp.data_required when
nodeConfigResp.type !== "aggregator" (otherwise set it to false), referencing
the existing symbols setDatastoreRequired, nodeConfigResp and datastoreState to
locate the code path to change.
- Around line 18-25: The falsy checks in the composable (involving
datastoreState.value.datastoreRequired and datastoreState.value.nodeType) cause
unnecessary re-fetches because they treat explicit false (and potentially empty
string) as uninitialized; change the guards in useDatastoreRequirement to check
for null/undefined explicitly (e.g., datastoreState.value.datastoreRequired ==
null or datastoreState.value.datastoreRequired === undefined) before calling
useNuxtApp().$hubApi("/node/settings"), and for nodeType check only for
null/undefined (or if empty string is meaningful, handle that case explicitly)
so that an explicit false or valid empty string does not trigger the fetch; keep
the existing nodeConfigResp handling (nodeConfigResp.data_required) but only
execute it when the state was truly uninitialized.
In `@services/hub_adapter_swagger.json`:
- Around line 767-773: Update the OpenAPI requestBody description for the
/node/settings endpoint so it correctly describes the NodeSettings payload
instead of "start analysis": locate the requestBody -> content ->
application/json -> schema referencing "NodeSettings" and change the description
to a clear, accurate phrase such as "Node settings payload" or "Configuration
for node settings" so API consumers are not misled.
---
Nitpick comments:
In `@components/header/AvatarButton.vue`:
- Around line 108-121: The toggle is interactive while the initial datastore
state is still loading (datastoreRequired === null) which can cause surprising
state flips; update the AvatarButton.vue code around the toggle/menu item (the
anchor with v-if="item.isToggle", the ToggleSwitch component, and the
setDatastoreRequired handler) to either hide the menu item until
datastoreRequired is non-null or disable interaction while loading—e.g., render
the item only when datastoreRequired !== null or add a disabled guard (both for
the <a> click and the ToggleSwitch) so clicks are ignored when datastoreRequired
=== null; ensure you still respect the existing click modifiers (.prevent/.stop)
and only call setDatastoreRequired when datastoreRequired is not null.
In `@test/components/header/AvatarButton.spec.ts`:
- Around line 25-33: Add a test that exercises the "Require Data Store" toggle:
render the AvatarButton component (or the component wrapper that contains
ToggleSwitch), mock useDatastoreRequirement as you already do (with
datastoreRequired: ref(true) and setDatastoreRequired = vi.fn()), assert that
the ToggleSwitch initially reflects datastoreRequired === true, simulate a user
click/trigger on the ToggleSwitch, and assert that setDatastoreRequired was
called with the toggled value (false) and that the ToggleSwitch updates
accordingly; reference the mocked hook useDatastoreRequirement, the mock
function setDatastoreRequired, and the ToggleSwitch within AvatarButton to
locate where to add the test.
| // Data Store Requirement Check | ||
| const { datastoreState } = await useDatastoreRequirement(); | ||
| const datastoreRequired = computed( | ||
| () => datastoreState.value.datastoreRequired, | ||
| ); | ||
| const nodeType = computed(() => datastoreState.value.nodeType); | ||
|
|
||
| const datastoreBadgeSeverity = computed(() => | ||
| datastoreRequired.value ? "danger" : "secondary", | ||
| ); | ||
| const datastoreBadgeTooltip = computed(() => | ||
| datastoreRequired.value | ||
| ? "Data store missing!" | ||
| : "Data store missing, but not required", | ||
| ); |
There was a problem hiding this comment.
Handle unknown datastoreRequired explicitly.
If useDatastoreRequirement yields null (e.g., failed fetch), the current logic treats it as false, enabling controls and showing the “not required” tooltip. Consider defaulting to a safe value or surfacing an “unknown” state, and remove the non-null assertion when passing the prop.
🛡️ Safer fallback example
-const datastoreRequired = computed(
- () => datastoreState.value.datastoreRequired,
-);
+const datastoreRequired = computed(
+ () => datastoreState.value.datastoreRequired ?? true,
+);
-const datastoreBadgeTooltip = computed(() =>
- datastoreRequired.value
- ? "Data store missing!"
- : "Data store missing, but not required",
-);
+const datastoreBadgeTooltip = computed(() =>
+ datastoreState.value.datastoreRequired === null
+ ? "Datastore requirement unknown"
+ : datastoreRequired.value
+ ? "Data store missing!"
+ : "Data store missing, but not required",
+);-:requireDatastore="datastoreRequired!"
+:requireDatastore="datastoreRequired"Also applies to: 659-664, 745-746
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/analysis/AnalysesTable.vue` around lines 36 - 50, The current
logic assumes useDatastoreRequirement() always returns a valid datastoreState
and treats null as false; update the code that derives
datastoreState/datastoreRequired/nodeType to explicitly handle a null result by
introducing an "unknown" fallback (e.g., datastoreState === null ->
datastoreRequired = null or 'unknown') and update computed properties
datastoreBadgeSeverity and datastoreBadgeTooltip to branch on the unknown case
(e.g., show neutral/severity "warning" and tooltip "Data store status unknown")
instead of defaulting to not-required; also remove any non-null assertions where
the prop is passed so the component receives the explicit null/unknown value and
can render a safe disabled state. Ensure these changes are applied to the same
patterns around datastoreState/datastoreRequired/nodeType usage (including the
other occurrences noted).
| requireDatastore: { | ||
| type: Boolean, | ||
| required: true, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=vue "<AnalysisControlButtons" -C3Repository: PrivateAIM/node-ui
Length of output: 89
🏁 Script executed:
fd -e vue -x grep -l "AnalysisControlButtons" {} + | head -20Repository: PrivateAIM/node-ui
Length of output: 1413
🏁 Script executed:
rg -n "AnalysisControlButtons" --type-list | head -5Repository: PrivateAIM/node-ui
Length of output: 451
🏁 Script executed:
rg -n "AnalysisControlButtons" -C3 --no-heading | head -100Repository: PrivateAIM/node-ui
Length of output: 5034
🏁 Script executed:
sed -n '735,755p' components/analysis/AnalysesTable.vueRepository: PrivateAIM/node-ui
Length of output: 964
🏁 Script executed:
grep -n "requireDatastore" components/analysis/AnalysesTable.vueRepository: PrivateAIM/node-ui
Length of output: 122
🏁 Script executed:
grep -n "requireDatastore" test/components/analysis/AnalysisControlButtons.spec.ts | head -20Repository: PrivateAIM/node-ui
Length of output: 44
🏁 Script executed:
sed -n '53,64p' test/components/analysis/AnalysisControlButtons.spec.tsRepository: PrivateAIM/node-ui
Length of output: 514
🏁 Script executed:
sed -n '277,288p' test/components/analysis/AnalysisControlButtons.spec.tsRepository: PrivateAIM/node-ui
Length of output: 506
Update test mount calls to include the required requireDatastore prop.
The production usage in AnalysesTable.vue correctly passes :requireDatastore="datastoreRequired!". However, the test file at test/components/analysis/AnalysisControlButtons.spec.ts has two mount calls (lines 53-64 and 277-288) that do not include this required prop, which will cause runtime warnings or test failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/analysis/AnalysisControlButtons.vue` around lines 43 - 46, Tests
mounting AnalysisControlButtons are missing the required prop requireDatastore
which causes warnings/failures; update both mount calls in
test/components/analysis/AnalysisControlButtons.spec.ts that instantiate the
AnalysisControlButtons component to pass the prop (e.g., requireDatastore: true
or use the same test fixture value as production like
:requireDatastore="datastoreRequired" or datastoreRequired!) so the mounted
wrapper includes the required Boolean prop; locate the mounts that call
mount(AnalysisControlButtons, { props: {...} }) and add requireDatastore to the
props object.
| // Node endpoints | ||
| export function getNodeConfiguration(opts?) { | ||
| return useAPIFetch<NodeSettings>("/node/settings", { | ||
| ...opts, | ||
| method: "GET", | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'getNodeConfiguration' --type=ts --type=vueRepository: PrivateAIM/node-ui
Length of output: 89
🏁 Script executed:
rg -n 'getNodeConfiguration' --type tsRepository: PrivateAIM/node-ui
Length of output: 137
🏁 Script executed:
rg -n 'useDatastoreRequirement' --type ts | head -20Repository: PrivateAIM/node-ui
Length of output: 1005
🏁 Script executed:
cat composables/useDatastoreRequirement.tsRepository: PrivateAIM/node-ui
Length of output: 1724
Remove getNodeConfiguration or refactor useDatastoreRequirement to use it.
getNodeConfiguration is unused throughout the codebase. useDatastoreRequirement directly calls useNuxtApp().$hubApi("/node/settings", ...) instead of using this helper function. Either remove the unused function or refactor the composable to use it for consistency with other endpoints.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composables/useAPIFetch.ts` around lines 36 - 42, getNodeConfiguration is
defined but unused; update useDatastoreRequirement to call
getNodeConfiguration(...) instead of directly invoking
useNuxtApp().$hubApi("/node/settings", ...), passing through the same opts and
handling the returned Promise/useAPIFetch result (NodeSettings) so behavior and
typing remain identical; alternatively, if you prefer not to change
useDatastoreRequirement, remove the unused getNodeConfiguration export to avoid
dead code.
| @@ -0,0 +1,58 @@ | |||
| import { type NodeTypeResponse } from "~/services/Api"; | |||
There was a problem hiding this comment.
Missing NodeSettings import — will cause a TypeScript error.
NodeSettings is used as a type assertion on lines 21 and 47 (as NodeSettings), but only NodeTypeResponse is imported.
🐛 Proposed fix
-import { type NodeTypeResponse } from "~/services/Api";
+import { type NodeSettings, type NodeTypeResponse } from "~/services/Api";📝 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.
| import { type NodeTypeResponse } from "~/services/Api"; | |
| import { type NodeSettings, type NodeTypeResponse } from "~/services/Api"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composables/useDatastoreRequirement.ts` at line 1, The file is missing the
NodeSettings type import used in the type assertions at lines where "as
NodeSettings" appears; update the import line that currently imports
NodeTypeResponse to also import NodeSettings from the same module (e.g., add
NodeSettings to the import from "~/services/Api") so the type assertions in
useDatastoreRequirement (references to NodeSettings) compile without TypeScript
errors.
| if (!datastoreState.value.datastoreRequired) { | ||
| const nodeConfigResp = (await useNuxtApp() | ||
| .$hubApi("/node/settings", { method: "GET" }) | ||
| .catch(() => null)) as NodeSettings; | ||
| if (nodeConfigResp) { | ||
| dataRequired = Boolean(nodeConfigResp.data_required); | ||
| } | ||
| } |
There was a problem hiding this comment.
Falsy check causes re-fetching when datastoreRequired is false.
!datastoreState.value.datastoreRequired is true when datastoreRequired is either null (uninitialized) or false (explicitly not required). This means every call to the composable when the datastore is not required will re-fetch /node/settings, defeating the caching purpose of useState.
The same issue exists on line 27 with !datastoreState.value.nodeType, although nodeType is a string so it's only problematic if the string can be empty.
🐛 Proposed fix — use strict null checks
- if (!datastoreState.value.datastoreRequired) {
+ if (datastoreState.value.datastoreRequired === null) {- if (!datastoreState.value.nodeType) {
+ if (datastoreState.value.nodeType === null) {📝 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.
| if (!datastoreState.value.datastoreRequired) { | |
| const nodeConfigResp = (await useNuxtApp() | |
| .$hubApi("/node/settings", { method: "GET" }) | |
| .catch(() => null)) as NodeSettings; | |
| if (nodeConfigResp) { | |
| dataRequired = Boolean(nodeConfigResp.data_required); | |
| } | |
| } | |
| if (datastoreState.value.datastoreRequired === null) { | |
| const nodeConfigResp = (await useNuxtApp() | |
| .$hubApi("/node/settings", { method: "GET" }) | |
| .catch(() => null)) as NodeSettings; | |
| if (nodeConfigResp) { | |
| dataRequired = Boolean(nodeConfigResp.data_required); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composables/useDatastoreRequirement.ts` around lines 18 - 25, The falsy
checks in the composable (involving datastoreState.value.datastoreRequired and
datastoreState.value.nodeType) cause unnecessary re-fetches because they treat
explicit false (and potentially empty string) as uninitialized; change the
guards in useDatastoreRequirement to check for null/undefined explicitly (e.g.,
datastoreState.value.datastoreRequired == null or
datastoreState.value.datastoreRequired === undefined) before calling
useNuxtApp().$hubApi("/node/settings"), and for nodeType check only for
null/undefined (or if empty string is meaningful, handle that case explicitly)
so that an explicit false or valid empty string does not trigger the fetch; keep
the existing nodeConfigResp handling (nodeConfigResp.data_required) but only
execute it when the state was truly uninitialized.
| async function setDatastoreRequired(updatedRequirement: boolean) { | ||
| const nodeConfigResp = (await useNuxtApp() | ||
| .$hubApi("/node/settings", { | ||
| method: "POST", | ||
| body: { data_required: updatedRequirement }, | ||
| }) | ||
| .catch(() => null)) as NodeSettings; | ||
|
|
||
| if (nodeConfigResp) { | ||
| datastoreState.value = { | ||
| ...datastoreState.value, | ||
| datastoreRequired: nodeConfigResp.data_required, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
setDatastoreRequired ignores the aggregator-type guard.
The initial fetch on line 36 applies nodeResp.type !== "aggregator" && dataRequired, but setDatastoreRequired writes nodeConfigResp.data_required directly without the aggregator check. If an aggregator node toggles this on, the UI will show datastoreRequired = true even though aggregator nodes should never require a datastore.
🐛 Proposed fix
if (nodeConfigResp) {
datastoreState.value = {
...datastoreState.value,
- datastoreRequired: nodeConfigResp.data_required,
+ datastoreRequired:
+ datastoreState.value.nodeType !== "aggregator" &&
+ Boolean(nodeConfigResp.data_required),
};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composables/useDatastoreRequirement.ts` around lines 41 - 55, The setter
setDatastoreRequired currently writes nodeConfigResp.data_required directly and
ignores the aggregator-type rule; update setDatastoreRequired to inspect
nodeConfigResp.type and only set datastoreState.value.datastoreRequired to
nodeConfigResp.data_required when nodeConfigResp.type !== "aggregator"
(otherwise set it to false), referencing the existing symbols
setDatastoreRequired, nodeConfigResp and datastoreState to locate the code path
to change.
| "requestBody": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/NodeSettings", | ||
| "description": "Required information to start analysis" | ||
| } |
There was a problem hiding this comment.
Fix requestBody description for /node/settings.
The description currently references “start analysis,” which is unrelated to node settings and can mislead API consumers.
✏️ Suggested fix
- "description": "Required information to start analysis"
+ "description": "Node settings payload to update"📝 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.
| "requestBody": { | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "$ref": "#/components/schemas/NodeSettings", | |
| "description": "Required information to start analysis" | |
| } | |
| "requestBody": { | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "$ref": "#/components/schemas/NodeSettings", | |
| "description": "Node settings payload to update" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/hub_adapter_swagger.json` around lines 767 - 773, Update the OpenAPI
requestBody description for the /node/settings endpoint so it correctly
describes the NodeSettings payload instead of "start analysis": locate the
requestBody -> content -> application/json -> schema referencing "NodeSettings"
and change the description to a clear, accurate phrase such as "Node settings
payload" or "Configuration for node settings" so API consumers are not misled.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Changes
service_nametoservice_tag.