feat!: take Hub entity types from @privateaim/core-kit and move Hub fields to camelCase - #401
Open
tada5hi wants to merge 2 commits into
Open
feat!: take Hub entity types from @privateaim/core-kit and move Hub fields to camelCase#401tada5hi wants to merge 2 commits into
tada5hi wants to merge 2 commits into
Conversation
…ields to camelCase The Hub renamed its entity/HTTP-API vocabulary from snake_case to camelCase in 0.13.0 (PrivateAIM/hub#1806). node-ui's copies of the Hub entities came from app/services/Api.ts, generated by swagger-typescript-api from a checked-in snapshot of the hub-adapter's OpenAPI document, so they silently went stale. Hub entities now come from the Hub's own published packages instead. The hub-adapter forwards Hub entities through unchanged — its FastAPI response_models ARE flame-hub-client's models — so the Hub, not a snapshot of the adapter's OpenAPI document, is the source of truth for their shape, and @privateaim/core-kit is versioned in lockstep with the API that produces them. - New app/services/hub.ts re-exports the eleven Hub entities plus the status enums from @privateaim/core-kit and @privateaim/kit. - app/types/analysis.ts is deleted: its ProcessStatus was a hand-copy of the Hub's, annotated "As defined by the Hub". app/types/node.ts keeps ApprovalStatus as a documented alias, because the Hub declares the same two members twice (ProjectNodeApprovalStatus / AnalysisNodeApprovalStatus) while the UI approves both kinds of node through one control. Members move to SCREAMING_CASE. - Everything the node genuinely owns — kong, pod-orchestrator, event log, container logs, node settings, health, auth — still comes from Api.ts, and keeps its snake_case fields. Only the Hub half of every mixed file moves. useDataStoreList keeps DATA_ROW_UNIX_COLS snake_case because it formats kong rows while its buildProjectNameMap reads a Hub Project; DetailedDataStoreTable is kong all the way down and is untouched; AnalysisControlButtons keeps analysis_id/project_id because that FormData goes to the adapter's own /analysis/initialize; and ApproveRejectToggle keeps its approval_status form key — that is the adapter's FastAPI Form(...) parameter, not a Hub key — while the response it reads back is a Hub entity and so becomes approvalStatus. The rename also covers everything with no compiler coverage: rapiq query strings (sort=-updatedAt, fields=id,name,displayName), PrimeVue column field/sortField/ filterField props, filter-state keys, and the composite "group::status" values the hub-status filter splits at runtime. rapiq drops an unknown sort or filter key silently rather than erroring, so a missed rename returns a wrongly-ordered or unfiltered result set with nothing in the logs. An eslint no-restricted-imports rule now fails on importing a Hub entity name from ~/services/Api, so the generated copies cannot creep back in. To make that guard actually enforce, a lint script is added and wired into CI, which required fixing six pre-existing lint errors (one unused import, one unused loop parameter, four `as any` casts) in otherwise unrelated files. BREAKING CHANGE: requires FLAME Hub 0.13.0 and a hub-adapter built against flame-hub-client with camelCase models. Against an older adapter every Hub field reads as undefined.
📝 WalkthroughWalkthroughThe PR adopts shared Hub types and camelCase fields across API interfaces, analysis and project UI flows, utilities, fixtures, and tests. It adds ChangesHub API contract migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/analysis/AnalysesTable.vue (1)
217-281: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNormalize Hub
ProcessStatustoPodStatusbefore comparing execution statuses.
executionStatuscan come from Hub/ProcessStatusor be overwritten byPodProgressResponse.status(PodStatus), and the code compares it toPodStatus.Failed/PodStatus.Executed. In Hub 0.13.0,ProcessStatususes PascalCase values likeEXECUTING/EXECUTEDwhilePodStatususes lowercase values likeexecuting/executed, so theseinchecks would reject valid Hub statuses and leave execution status filtered out with no progress bars. Move the conversion at the boundary whereexecutionStatusis assigned, or compare against the Hub enum when that source is active.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/analysis/AnalysesTable.vue` around lines 217 - 281, Normalize the Hub-sourced executionStatus to the corresponding PodStatus value when assigning it in parseAnalysis, before the acceptableHubStatuses comparison; keep PodProgressResponse.status unchanged because it is already PodStatus. Ensure PascalCase ProcessStatus values such as EXECUTING and EXECUTED are converted to the lowercase PodStatus equivalents so valid statuses are not cleared.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/services/modifiedApiInterfaces.ts`:
- Around line 46-61: Update ModifiedAnalysisNode so pod-orchestrator PodStatus
and Hub ProcessStatus remain separate rather than overloading executionStatus.
Add or reuse a distinct field for the Hub execution status, and ensure any
fallback or downstream assignment explicitly normalizes the Hub value before
using it as a PodStatus; keep lifecycle counting based on the correct status
domain.
In `@eslint.config.js`:
- Around line 31-45: Add "MasterImageGroup" to the importNames list in the Api
restriction rule, alongside the existing Hub entity names, so imports from
"~/services/Api" are also prohibited.
---
Outside diff comments:
In `@app/components/analysis/AnalysesTable.vue`:
- Around line 217-281: Normalize the Hub-sourced executionStatus to the
corresponding PodStatus value when assigning it in parseAnalysis, before the
acceptableHubStatuses comparison; keep PodProgressResponse.status unchanged
because it is already PodStatus. Ensure PascalCase ProcessStatus values such as
EXECUTING and EXECUTED are converted to the lowercase PodStatus equivalents so
valid statuses are not cleared.
🪄 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 Plus
Run ID: 7f8a793c-0a8a-410b-be8c-0b7db41280d8
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
.github/workflows/ci.yamlREADME.mdapp/components/analysis/AnalysesTable.vueapp/components/analysis/AnalysisControlButtons.vueapp/components/analysis/ContainerCounter.vueapp/components/analysis/logs/ContainerLogs.vueapp/components/data-stores/create/DataStoreProjectInitializer.vueapp/components/header/DarkModeToggle.vueapp/components/projects/ProjectProposalTable.vueapp/components/table/ApproveRejectToggle.vueapp/composables/useAPIFetch.tsapp/composables/useDataStoreList.tsapp/services/hub.tsapp/services/modifiedApiInterfaces.tsapp/stores/hubStore.tsapp/types/analysis.tsapp/types/node.tsapp/utils/count-analyses.tsapp/utils/format-data-row.tsapp/utils/status-tag-severity.tseslint.config.jspackage.jsonpnpm-workspace.yamltest/components/analysis/AnalysisControlButtons.spec.tstest/components/analysis/AnalysisTable.spec.tstest/components/analysis/ContainerCounter.spec.tstest/components/analysis/constants.tstest/components/data-stores/DataStoreList.spec.tstest/components/data-stores/constants.tstest/components/data-stores/create/DataStoreProjectInitializer.spec.tstest/components/events/EventViewer.spec.tstest/components/projects/ProjectProposalTable.spec.tstest/components/projects/constants.tstest/components/table/ApproveRejectToggle.spec.tstest/composables/useDataStoreList.test.tstest/mockapi/handlers.tstest/utils/count-analyses.test.tstest/utils/format-data-row.test.ts
💤 Files with no reviewable changes (2)
- app/types/analysis.ts
- app/components/header/DarkModeToggle.vue
Comment on lines
46
to
+61
| export interface ModifiedAnalysisNode extends Omit< | ||
| AnalysisNode, | ||
| "execution_status" | ||
| "executionStatus" | ||
| > { | ||
| project_name: string | undefined | null; | ||
| analysis_name: string | undefined | null; | ||
| projectName: string | undefined | null; | ||
| analysisName: string | undefined | null; | ||
| expand: { | ||
| [key: string]: string; | ||
| }; | ||
| datastore: boolean; | ||
| execution_status: PodStatus | undefined | null; | ||
| // Deliberately the pod-orchestrator's status rather than the Hub's: the table | ||
| // shows what the local containers are doing and falls back to the Hub's | ||
| // `executionStatus` only when the pod-orchestrator has nothing to report. | ||
| executionStatus: PodStatus | undefined | null; | ||
| progress: number; | ||
| hub_statuses: HubStatuses; | ||
| hubStatuses: HubStatuses; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not overload executionStatus with two status domains.
This interface removes the Hub’s executionStatus and replaces it with PodStatus, while the comment says the same field falls back to the Hub status. Since ProcessStatus and PodStatus are explicitly non-interchangeable, preserve separate fields or normalize the Hub value before assigning the local status; otherwise downstream counting can use the wrong lifecycle domain.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/services/modifiedApiInterfaces.ts` around lines 46 - 61, Update
ModifiedAnalysisNode so pod-orchestrator PodStatus and Hub ProcessStatus remain
separate rather than overloading executionStatus. Add or reuse a distinct field
for the Hub execution status, and ensure any fallback or downstream assignment
explicitly normalizes the Hub value before using it as a PodStatus; keep
lifecycle counting based on the correct status domain.
Comment on lines
+31
to
+45
| group: ["**/services/Api"], | ||
| importNames: [ | ||
| "Analysis", | ||
| "AnalysisBucket", | ||
| "AnalysisBucketType", | ||
| "AnalysisNode", | ||
| "DetailedAnalysis", | ||
| "MasterImage", | ||
| "MasterImageCommandArgument", | ||
| "Node", | ||
| "Project", | ||
| "ProjectNode", | ||
| "Registry", | ||
| "RegistryProject", | ||
| ], |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add MasterImageGroup to the restricted import list.
app/services/hub.ts re-exports MasterImageGroup, but this rule does not forbid importing it from ~/services/Api. Add it to importNames so the lint guard covers every Hub entity exposed by the new source module.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@eslint.config.js` around lines 31 - 45, Add "MasterImageGroup" to the
importNames list in the Api restriction rule, alongside the existing Hub entity
names, so imports from "~/services/Api" are also prohibited.
Drop the `app/services/hub.ts` re-export facade and the `app/types/node.ts` alias, so every file names the package it depends on. `DetailedAnalysis` was only ever a facade-local alias for core-kit's `Analysis` and goes with it. `ApprovalStatus` resolves per call site rather than through one alias: the analyses table and the analysis controls take `AnalysisNodeApprovalStatus`, the project proposals table takes `ProjectNodeApprovalStatus`, and `ApproveRejectToggle` — which serves both — takes the shared value union now exported as `ApprovalStatusValue` from the severity helper. That helper accepts the template-literal form of both enums, which is what lets one function serve two nominally distinct enums that happen to share their members. Also fixes a stale import the earlier commit left behind: test/utils/status-tag-severity.test.ts still pointed at the deleted `~/types/analysis`, so the whole file failed to collect. Nothing caught it — eslint does not resolve module paths and `test/**` is outside the tsconfig program — and because a file that fails to collect reports no individual test results, it was invisible to a failure-count comparison too. Finally, four assertions in the data-store-name tests were left behind by 3f2801d ("fix(kong)!: harden data store name generation"), which appended a four-character hex suffix without updating the patterns they are matched against. They fail on develop as well; updating them here is what takes CI green.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/utils/status-tag-severity.test.ts (1)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover analysis-node approval statuses too.
getApprovalStatusSeveritynow accepts both approval-status enums, but this test only iteratesProjectNodeApprovalStatus. Add equivalent coverage forAnalysisNodeApprovalStatusso both Hub contracts are verified.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/utils/status-tag-severity.test.ts` at line 17, Extend the test around getApprovalStatusSeverity to also iterate over AnalysisNodeApprovalStatus, mirroring the existing ProjectNodeApprovalStatus coverage and assertions so both approval-status enums are verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/utils/status-tag-severity.test.ts`:
- Line 17: Extend the test around getApprovalStatusSeverity to also iterate over
AnalysisNodeApprovalStatus, mirroring the existing ProjectNodeApprovalStatus
coverage and assertions so both approval-status enums are verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4dd7a6e3-efa0-4a39-97ff-1abff69862f1
📒 Files selected for processing (28)
README.mdapp/components/analysis/AnalysesTable.vueapp/components/analysis/AnalysisControlButtons.vueapp/components/analysis/logs/ContainerLogs.vueapp/components/data-stores/create/DataStoreProjectInitializer.vueapp/components/projects/ProjectProposalTable.vueapp/components/table/ApproveRejectToggle.vueapp/composables/useAPIFetch.tsapp/composables/useDataStoreList.tsapp/services/modifiedApiInterfaces.tsapp/stores/hubStore.tsapp/types/node.tsapp/utils/format-data-row.tsapp/utils/status-tag-severity.tseslint.config.jstest/components/analysis/AnalysisControlButtons.spec.tstest/components/analysis/AnalysisTable.spec.tstest/components/analysis/ContainerCounter.spec.tstest/components/analysis/constants.tstest/components/data-stores/DataStoreList.spec.tstest/components/data-stores/constants.tstest/components/data-stores/create/DataStoreProjectInitializer.spec.tstest/components/projects/ProjectProposalTable.spec.tstest/components/projects/constants.tstest/components/table/ApproveRejectToggle.spec.tstest/composables/useDataStoreList.test.tstest/utils/data-store-name.test.tstest/utils/status-tag-severity.test.ts
💤 Files with no reviewable changes (1)
- app/types/node.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- app/stores/hubStore.ts
- test/components/projects/ProjectProposalTable.spec.ts
- test/components/data-stores/DataStoreList.spec.ts
- README.md
- eslint.config.js
- app/utils/format-data-row.ts
- test/components/table/ApproveRejectToggle.spec.ts
- test/composables/useDataStoreList.test.ts
- test/components/analysis/ContainerCounter.spec.ts
- app/components/data-stores/create/DataStoreProjectInitializer.vue
- test/components/data-stores/constants.ts
- app/services/modifiedApiInterfaces.ts
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
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.
FLAME Hub 0.13.0 renamed every entity/HTTP-API field from
snake_casetocamelCase(PrivateAIM/hub#1806). node-ui's copies of the Hub entities came fromapp/services/Api.ts— generated byswagger-typescript-apifrom a checked-in snapshot of the hub-adapter's OpenAPI document — so they went stale without a single error.This PR does two things: takes the Hub entities from the Hub's own packages, and moves every Hub field reference to camelCase.
Why the Hub's packages rather than the generated copies
The hub-adapter does not re-model Hub entities — it forwards them.
hub_adapter/routers/hub.pydeclaresresponse_model=list[Project]whereProjectisflame_hub.models.Project, so the adapter's wire shape isflame-hub-client's model. The Hub is therefore the source of truth for these types, and@privateaim/core-kitis versioned in lockstep with the API that produces them, whereas the checked-inswagger.jsonis a snapshot that drifts whenever the adapter lags a release.@privateaim/core-kit, and the sharedProcessStatusfrom@privateaim/kit, at each of the 26 sites that needs one — no re-export layer.app/types/analysis.tsandapp/types/node.tsare deleted. TheirProcessStatusandApprovalStatuswere hand-copies of the Hub's —analysis.tswas even annotated "As defined by the Hub". Enum members move toSCREAMING_CASE.ApprovalStatusresolves per call site. The Hub declares the same two members twice (AnalysisNodeApprovalStatus/ProjectNodeApprovalStatus) as nominally distinct enums. The analyses table and analysis controls take the analysis one, the project proposals table the project one, andApproveRejectToggle— which serves both — takes the shared value union, exported asApprovalStatusValuefrom the severity helper. That helper is typed on the template-literal form of both enums, which is what lets one function serve two nominal types that happen to share their members.@privateaim/core-kitships zod + validup as runtime deps, but node-ui imports only types and a few enums: verified fully tree-shaken —grep -rl "ZodError\|validup" .output/public/_nuxt/*.jsis empty afterpnpm build.Hub vs node-local, in the mixed files
Several field names exist on both sides. Each was decided by the type of the object, and by reading the adapter's source where the wire contract was ambiguous:
useDataStoreList.tsentry.display_name→displayName, butDATA_ROW_UNIX_COLSstays snakebuildProjectNameMapwalks a HubProject; the same file'sformatDataRowarray is applied to kongDetailedService/RouterowsDetailedDataStoreTable.vuefieldpropsAnalysisControlButtons.vueanalysis_id/project_idstayFormDataposts to the adapter's own/analysis/initialize, whoseInitializeAnalysismodel is snake_caseApproveRejectToggle.vueapproval_statusstays, response guard"approvalStatus" in …moveshub.py:160,218declareapproval_statusas a FastAPIForm(...)parameter — renaming it would 422 every approval. The response it reads back is a Hub entity.DataStoreProjectInitializer.vueproj.project?.displayName/proj.projectIdmove; theproject_id:body key staysProjectNode; the key belongs to a kong request bodytest/mockapi/handlers.ts/analysis-nodes/{id}moves/po,/analysis/initialize,/logs,/historyand/kong/*handlers are node-localserver/routes/flame/api/auth/[...].tsprofile.display_nameis the OIDC userinfo claimThe parts with no compiler coverage
Api.tsis@ts-nocheck, and the rest of the rename lives in strings. These moved too:sort: "-updated_at"→"-updatedAt"(×5),fields: "id,name,display_name"→"id,name,displayName".includevalues are relation names (project,node,analysis) and are single words, so unchanged.field/sortField/filterField/globalFilterFieldsacrossAnalysesTable.vueandProjectProposalTable.vue.fieldprops exactly or the dropdown renders and filters nothing."group::status"values the hub-status filter splits at runtime and uses to indexHubStatuses— those move as one unit with the interface keys.This matters because rapiq drops an unknown
sort/filter/includekey silently rather than rejecting it: a missed rename returns a wrongly-ordered or unfiltered result set, with nothing in the logs. Same for a stalefieldprop — the column just renders blank.The guard
An eslint
no-restricted-importsrule now errors on importing a Hub entity name from**/services/Api, pointing at@privateaim/core-kitinstead, so the generated copies cannot creep back in.pnpm build-apistill regenerates them; they are simply unusable.A guard that never runs is decoration, and CI ran only
build+test:ci. So this also adds alintscript and a CI step — which required fixing six pre-existing lint errors in otherwise unrelated files (one unused import inDarkModeToggle.vue, one unused loop parameter, fouras anycasts). Happy to drop that part if you'd rather keep the CI change separate.Verification
pnpm buildpnpm lintnpx vitest runtsc --noEmitAbout those four test fixes
developis red at3f2801d, independently of this PR —data-store-name.test.ts×2 andDataStoreProjectInitializer.spec.ts×2 fail there too. Verified by checking out3f2801dclean and running the suite: 4 failed / 207 passed, the same four.3f2801d("fix(kong)!: harden data store name generation") appended a four-character hex suffix togenerateRandomDataStoreNamewithout updating the four patterns it is matched against. CI never caught it because the workflow only triggers onpull_request, so it has never run againstdevelopitself. This PR updates the patterns; happy to split that into its own PR if you'd rather.On test-suite blind spots
Two migration bugs were caught by the suite rather than by a compiler, and both are worth knowing about:
ApproveRejectToggle.spec.tsusedApprovalStatus.Approved, which becameundefinedunderSCREAMING_CASE.status-tag-severity.test.tsimported the deleted~/types/analysis, so the whole file failed to collect.Neither was visible to eslint (it does not resolve module paths) or to
tsc(test/**is outside the tsconfig program). The second is the nastier shape: a file that fails to collect reports no individual test results, so it does not move the failure count and is invisible to any comparison based on it — only the total collected count moves (207 → 211). Wiring upvue-tscwould close this, though it will first surface the 10 pre-existing errors above.Merge order
This assumes
PrivateAIM/node-hub-api-adapterpicks up the updated client. It pinsflame-hub-client==0.4.1today, and its FastAPIresponse_models are those models, so bumping it is what turns its wire shape camelCase — no model changes needed on the adapter's side, only thecore_client.*(…)keyword arguments it passes (project_node_id=→projectNodeId=,approval_status=→approvalStatus=, …).Ship order:
PrivateAIM/node-hub-api-adapter— bumpflame-hub-client, fix its call-site kwargsAgainst an adapter still on 0.4.1 every Hub field here reads as
undefined, so this should not go out ahead of step 2.Not included
@privateaim/core-http-kit'sClientis deliberately not adopted. The adapter strips the Hub's{ data, meta }envelope —GET /projectsanswers with a bare array — so the client'sEntityCollectionResponse/EntityRecordResponsecontract does not describe what node-ui receives.@privateaim/client-vueis likewise out: node-ui is PrimeVue 4 + Tailwind 4 and client-vue is@vuecswith its own theme and an@authup/client-web-kitinstall prerequisite, so adopting it would mean two component libraries and two theme systems in one app.Summary by CodeRabbit
Enhancements
Developer Experience
Bug Fixes