feat(nvcf-ui): import the self-hosted UI into the monorepo - #662
feat(nvcf-ui): import the self-hosted UI into the monorepo#662balajinvda wants to merge 6 commits into
Conversation
Imports ngc/apps/nvcf-ui at 2c6b0b711b46c964cd4b3a119c5dd467540efbfc into src/uis/nvcf-ui, grouped by artifact kind alongside src/clis/nvcf-cli rather than under control-plane services: the UI is a shipped user-facing artifact, not a control-plane service. 267 of the upstream's 271 tracked files come across. Excluded: - .gitlab-ci.yml, superseded by this repository's release path - node_modules/.vite/.../results.json, a committed vitest cache artifact - .vscode/, editor-local settings Rewrites bare uppercase SIS to describe the cluster API in prose instead. All five occurrences were comments or documentation, so nothing structural changed. The OSS snapshot tooling has historically dropped whole files containing those bare tokens, which would have silently removed backend/cmd/server/main.go from the public mirror and broken the build with no error pointing at the cause. Adds /src/uis/ to CODEOWNERS under nvcf-control-plane-dev, per review request. The subtree keeps the upstream MODULE.bazel for now and is listed in .bazelignore, so the root module does not try to load packages whose labels are still relative to the upstream module root. That is deliberately temporary and runs against the recent consolidation of every nested module into the root; merging the JS rules into the root MODULE.bazel and rewriting the labels needs a real build to validate and belongs in its own change. Not included, and tracked in #634: - root MODULE.bazel merge and the bazel.yml matrix row - an npm collector for tools/collect-dependencies; until it exists the UI's third-party tree does not reach dependencies.md NOTICE is regenerated to absorb the UI's own NOTICE. All 182 imported source files already carry SPDX headers, and tools/ci/check-license passes. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request imports the self-hosted NVCF UI into the monorepo. It adds the React frontend, Go backend, control-plane monitoring, token watching, OpenAPI specifications, Helm chart, Bazel and OCI build integration, tests, mocks, documentation, and licensing metadata. ChangesNVCF UI monorepo import
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Two CI failures from the import. check-nested-modules rejected src/uis/nvcf-ui because a new nested MODULE.bazel is unclassified. Recorded as `ledger`, not `exception`: the category means the migration is outstanding rather than excused, and the entry leaves the file entirely once the subtree builds from the root module. This does re-open the consolidation backlog, which had reached zero services awaiting migration. dependencies.md was stale because the UI's Go backend pulls in github.com/rs/xid. Regenerated with the real generator rather than hand-applying the diff CI printed, since the two have disagreed before. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (38)
src/uis/nvcf-ui/.claude/skills/add-feature/SKILL.md-39-39 (1)
39-39: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language tag to the fenced tree block.
markdownlintreports Line 39 because the fence has no language. Usetextfor this file-tree example.Proposed fix
-``` +```text🤖 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 `@src/uis/nvcf-ui/.claude/skills/add-feature/SKILL.md` at line 39, Update the fenced tree block in SKILL.md to specify the text language tag, changing the untagged fence to a text-tagged fence while preserving the file-tree example content.Source: Linters/SAST tools
src/uis/nvcf-ui/AGENTS.md-9-9 (1)
9-9: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd language identifiers to both Markdown fences.
src/uis/nvcf-ui/AGENTS.md#L9-L9: addtextor another suitable language.src/uis/nvcf-ui/README.md#L55-L55: addtextor another suitable language.🤖 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 `@src/uis/nvcf-ui/AGENTS.md` at line 9, Add a suitable language identifier, such as text, to the Markdown fence in src/uis/nvcf-ui/AGENTS.md at lines 9-9 and src/uis/nvcf-ui/README.md at lines 55-55.Source: Linters/SAST tools
src/uis/nvcf-ui/AGENTS.md-56-60 (1)
56-60: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the subtree build and test commands.
This file documents lint and code-generation commands, but it does not document the commands that build the subtree or run its tests. Add the exact native commands and their scope, such as
task buildandtask test, plus backend-specific commands when they differ.As per path instructions, each
AGENTS.mdmust stay under 400 lines and include subtree build commands, test commands, code style, and subtree-specific conventions.Also applies to: 73-79
🤖 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 `@src/uis/nvcf-ui/AGENTS.md` around lines 56 - 60, Update the Testing section of AGENTS.md to document the exact native commands for building and testing the subtree, including their scope and any backend-specific variants where applicable. Retain the existing testing guidance and ensure the file continues to include subtree build commands, test commands, code style, and conventions within the 400-line limit.Source: Path instructions
src/uis/nvcf-ui/README.md-63-64 (1)
63-64: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign CI and release documentation with the imported scope.
The imported subtree does not contain the CI and release automation described by these documents. The PR defers that integration.
src/uis/nvcf-ui/README.md#L63-L64: remove.gitlab-ci.ymlfrom the tree or identify the external pipeline; mark the image-publishing statement at Lines 46-47 as external or deferred.src/uis/nvcf-ui/SECURITY.md#L190-L195: identify the external semantic-release owner, or mark the release process as planned or deferred.🤖 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 `@src/uis/nvcf-ui/README.md` around lines 63 - 64, Update src/uis/nvcf-ui/README.md lines 63-64 to remove .gitlab-ci.yml from the repository tree or identify the external pipeline, and mark the image-publishing statement at lines 46-47 as external or deferred. Update src/uis/nvcf-ui/SECURITY.md lines 190-195 to identify the external semantic-release owner or explicitly mark the release process as planned or deferred.src/uis/nvcf-ui/README.md-5-10 (1)
5-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the Go toolchain requirement for Task commands.
backend/go.modrequires Go 1.26.2. KeepGo 1.24+only if the README states that automatic toolchain switching must download Go 1.26.2; otherwise, raise the prerequisite to Go 1.26.2.🤖 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 `@src/uis/nvcf-ui/README.md` around lines 5 - 10, Update the Prerequisites section to align the Go requirement with backend/go.mod: either change Go to 1.26.2 or explicitly document that Task commands automatically download and switch to Go 1.26.2 while retaining Go 1.24+.src/uis/nvcf-ui/ui/src/features/accounts/getActiveNcaId.ts-25-27 (1)
25-27: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEnable stale account-list revalidation.
Pass
revalidateIfStale: truetoensureQueryData. This triggers a background refresh when the cached account list is stale.🤖 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 `@src/uis/nvcf-ui/ui/src/features/accounts/getActiveNcaId.ts` around lines 25 - 27, Update the ensureQueryData call in getActiveNcaId to pass revalidateIfStale: true alongside getGetCloudAccountsQueryOptions(), enabling background refreshes for stale cached account lists.Source: Coding guidelines
src/uis/nvcf-ui/ui/src/features/clusters/ClustersListPending.tsx-21-28 (1)
21-28: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a user-visible test for the pending view.
ClustersListPendingis a new route component. The supplied list test covers only resolved data and empty data. Add a Vitest and Testing Library test that delays the clusters request and asserts the pending UI.As per coding guidelines: “When adding or modifying a component, add or update its corresponding tests.”
🤖 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 `@src/uis/nvcf-ui/ui/src/features/clusters/ClustersListPending.tsx` around lines 21 - 28, Add a Vitest/Testing Library test for ClustersListPending that delays the clusters request, renders the pending route state, and asserts the visible ClustersPageHeading and DataTable.Skeleton UI. Extend the existing clusters list test coverage without changing the component implementation or resolved/empty-state assertions.Source: Coding guidelines
src/uis/nvcf-ui/ui/src/features/clusters/ClustersList.tsx-42-51 (1)
42-51: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not create a detail link without a cluster ID.
Line 45 replaces an absent
clusterIdwith"". The response model permits an absentclusterId, andsrc/uis/nvcf-ui/ui/src/features/clusters/ClustersList.test.tsxuses that case. The table then creates a detail link with no resource identifier.Render plain text when
clusterIdis absent. CreateLinkonly when the ID is non-empty. Add assertions for both ID-present and ID-absent rows.Proposed fix
- cell: ({ row, getValue }) => ( - <Anchor asChild> - <Link - params={{ clusterId: row.original.clusterId ?? "" }} - to="/clusters/$clusterId" - viewTransition - > - {getValue() ?? row.original.clusterId ?? "—"} - </Link> - </Anchor> - ), + cell: ({ row, getValue }) => { + const clusterId = row.original.clusterId; + const label = getValue() ?? clusterId ?? "—"; + + if (!clusterId) return label; + + return ( + <Anchor asChild> + <Link + params={{ clusterId }} + to="/clusters/$clusterId" + viewTransition + > + {label} + </Link> + </Anchor> + ); + },🤖 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 `@src/uis/nvcf-ui/ui/src/features/clusters/ClustersList.tsx` around lines 42 - 51, Update the cluster column renderer in ClustersList so rows without a non-empty row.original.clusterId render plain text instead of an Anchor/Link with an empty parameter; retain the detail link for rows with an ID and add test assertions in ClustersList.test.tsx covering both present and absent cluster IDs.src/uis/nvcf-ui/ui/src/features/dashboard/components/ClusterStatsPanel.tsx-30-45 (1)
30-45: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the Not Ready drill-down consistent with the aggregate.
clustersSelectcounts every status except READY and UNHEALTHY as Not Ready. The link filters only NOT_READY, CORDON, and CORDON_AND_DRAIN. A PAUSED, FAILED, ABANDONED, or DELETED cluster increases the dashboard count but is absent after the user selects Not Ready.Derive the aggregate and link values from one shared status set. Add coverage for an omitted status.
Also applies to: 112-124
🤖 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 `@src/uis/nvcf-ui/ui/src/features/dashboard/components/ClusterStatsPanel.tsx` around lines 30 - 45, Update clustersSelect and the Not Ready drill-down filter to use one shared status set containing NOT_READY, CORDON, CORDON_AND_DRAIN, PAUSED, FAILED, ABANDONED, and DELETED. Ensure every status counted in the Not Ready aggregate is included by the link, and add coverage for at least one previously omitted status such as PAUSED.src/uis/nvcf-ui/ui/src/features/dashboard/Dashboard.test.tsx-283-285 (1)
283-285: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the five-function limit.
The test proves that
fn-newestis present andfn-oldestis absent. A regression that renders one to four functions still passes. Assert all five expected functions, or assert the rendered recent-function count.🤖 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 `@src/uis/nvcf-ui/ui/src/features/dashboard/Dashboard.test.tsx` around lines 283 - 285, Strengthen the dashboard test around the recent-functions rendering assertion: verify that all five expected functions are present, including the existing fn-newest case, or assert that the rendered recent-function collection has a count of five. Keep the fn-oldest absence assertion to preserve the recency boundary check.src/uis/nvcf-ui/ui/src/features/dashboard/components/ReadyClustersPanel.tsx-45-50 (1)
45-50: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClamp the selected GPU page after GPU data changes.
If page two is selected and the same cluster later has only two GPU types,
visibleGpusbecomes empty. The pagination control is then hidden, so the user cannot return to page one.Derive a page value clamped to the current GPU-entry count before slicing. Pass that value to
Pagination. Add a refresh or rerender test for this transition.Also applies to: 84-96
🤖 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 `@src/uis/nvcf-ui/ui/src/features/dashboard/components/ReadyClustersPanel.tsx` around lines 45 - 50, Clamp the selected GPU page in the ReadyClustersPanel component to the valid range derived from gpuEntries.length before calculating visibleGpus, ensuring reduced GPU data falls back to the last available page. Use the clamped page for both slicing and the Pagination component, and add a test covering a transition from page two to a dataset with only two GPU entries.src/uis/nvcf-ui/ui/src/features/dashboard/components/RecentFunctionsList.tsx-36-36 (1)
36-36: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
~/aliases for imports fromui/src.Replace all relative UI-source imports with their
~/equivalents.
src/uis/nvcf-ui/ui/src/features/dashboard/components/RecentFunctionsList.tsx#L36-L36: use~/features/functions/components/StatusBadge.src/uis/nvcf-ui/ui/src/features/dashboard/components/StatPanelBoundary.tsx#L22-L22: use~/features/dashboard/components/StatPanelSkeleton.src/uis/nvcf-ui/ui/src/features/dashboard/routes.tsx#L54-L54: useimport("~/features/dashboard/Dashboard").src/uis/nvcf-ui/ui/src/features/functions/FunctionDetail.test.tsx#L31-L31: use~/features/functions/routes.src/uis/nvcf-ui/ui/src/features/functions/FunctionDetail.tsx#L42-L44: use~/features/functions/components/StatusBadge,~/features/functions/constants, and~/features/functions/routes.src/uis/nvcf-ui/ui/src/features/functions/FunctionsList.test.tsx#L26-L26: use~/features/functions/routes.src/uis/nvcf-ui/ui/src/features/functions/FunctionsList.tsx#L24-L28: use~/features/functions/components/FunctionCard,~/features/functions/components/FunctionsPageHeading,~/features/functions/constants,~/features/functions/hooks/useFunctionsWithDeployments, and~/features/functions/types.src/uis/nvcf-ui/ui/src/features/functions/FunctionsListPending.tsx#L19-L20: use~/features/functions/components/FunctionCardand~/features/functions/components/FunctionsPageHeading.src/uis/nvcf-ui/ui/src/features/functions/components/FunctionCard.test.tsx#L21-L22: use~/features/functions/typesand~/features/functions/components/FunctionCard.As per coding guidelines, “Use the
~/path alias for imports fromui/src/.”🤖 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 `@src/uis/nvcf-ui/ui/src/features/dashboard/components/RecentFunctionsList.tsx` at line 36, Replace the listed relative imports with the ~/ path aliases: in src/uis/nvcf-ui/ui/src/features/dashboard/components/RecentFunctionsList.tsx lines 36-36 use ~/features/functions/components/StatusBadge; in src/uis/nvcf-ui/ui/src/features/dashboard/components/StatPanelBoundary.tsx lines 22-22 use ~/features/dashboard/components/StatPanelSkeleton; in src/uis/nvcf-ui/ui/src/features/dashboard/routes.tsx lines 54-54 use ~/features/dashboard/Dashboard; in src/uis/nvcf-ui/ui/src/features/functions/FunctionDetail.test.tsx lines 31-31 use ~/features/functions/routes; in FunctionDetail.tsx lines 42-44 use the specified ~/features/functions aliases for StatusBadge, constants, and routes; in FunctionsList.test.tsx lines 26-26 use ~/features/functions/routes; in FunctionsList.tsx lines 24-28 use the specified ~/features/functions aliases for FunctionCard, FunctionsPageHeading, constants, useFunctionsWithDeployments, and types; in FunctionsListPending.tsx lines 19-20 use the component aliases; and in components/FunctionCard.test.tsx lines 21-22 use the types and FunctionCard aliases.Source: Coding guidelines
src/uis/nvcf-ui/ui/src/features/dashboard/routes.tsx-35-50 (1)
35-50: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle rejected cache warmups.
ensureQueryDatacan reject when a cache miss request fails. Attach.catch(() => undefined)to each fire-and-forget call to prevent unhandled-rejection reporting while preserving the query error for panel boundaries.🤖 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 `@src/uis/nvcf-ui/ui/src/features/dashboard/routes.tsx` around lines 35 - 50, Update each fire-and-forget ensureQueryData call in the dashboard route loader to attach a catch handler that resolves to undefined. Apply this to the calls using getGetClustersQueryOptions, getGetControlPlaneStatusQueryOptions, getGetAllFunctionsQueryOptions, and getGetAllFunctionDeploymentsQueryOptions, while preserving the original query errors for panel boundaries.src/uis/nvcf-ui/ui/src/features/dashboard/components/RecentFunctionsList.tsx-151-168 (1)
151-168: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset the error boundary when
ncaIdchanges.If Account A fails, the boundary remains in its fallback after switching to Account B. Add
resetKeys={[ncaId]}toErrorBoundaryso the new account can render.🤖 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 `@src/uis/nvcf-ui/ui/src/features/dashboard/components/RecentFunctionsList.tsx` around lines 151 - 168, Add resetKeys={[ncaId]} to the ErrorBoundary in RecentFunctionsList so changing accounts resets the fallback and allows the new account’s functions to render.src/uis/nvcf-ui/ui/src/hooks/useLocalStorage.ts-42-44 (1)
42-44: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle cross-document
localStorage.clear()events.When another document calls
clear(),StorageEvent.keyisnull. The current handler ignores this event, so the hook can retain a stale value. Notify subscribers whenevent.keyisnullor matcheskey, and add a regression test for the clear event.🤖 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 `@src/uis/nvcf-ui/ui/src/hooks/useLocalStorage.ts` around lines 42 - 44, Update the onStorage handler in useLocalStorage to call onChange when event.key is null or equals key, covering cross-document localStorage.clear() events while preserving the existing matching-key behavior. Add a regression test that dispatches or simulates a clear event with a null key and verifies subscribers are notified.Source: Coding guidelines
src/uis/nvcf-ui/ui/src/features/functions/routes.tsx-82-88 (1)
82-88: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
ensureQueryDatafor the optional deployment preload.Replace
prefetchQuerywithensureQueryDataandrevalidateIfStale: true. Catch rejected promises because deployment loading is optional.🤖 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 `@src/uis/nvcf-ui/ui/src/features/functions/routes.tsx` around lines 82 - 88, Update the optional deployment preload around getGetFunctionDeploymentQueryOptions to use queryClient.ensureQueryData with revalidateIfStale enabled instead of prefetchQuery. Handle rejected promises so a failed optional deployment load does not propagate as an unhandled rejection.Source: Coding guidelines
src/uis/nvcf-ui/ui/src/components/DataTable/Filters.tsx-23-44 (1)
23-44: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the missing DataTable control tests.
DataTable.test.tsxcovers filtering, row-count labels, and page clamping. Add coverage for pagination page changes, debounced search timing, and theSearchdismissal path. Run(cd src/uis/nvcf-ui/ui && pnpm test)and record the result before merge.🤖 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 `@src/uis/nvcf-ui/ui/src/components/DataTable/Filters.tsx` around lines 23 - 44, Extend DataTable.test.tsx with coverage for pagination page changes in Pagination.tsx (lines 32-86), debounced search timing and the Search dismissal path in Search.tsx (lines 24-62), and retain existing filter coverage for Filters.tsx (lines 23-44) and row-count coverage for ItemCount.tsx (lines 21-38); no direct component changes are required. Run the UI test suite with the specified pnpm test command and record its result before merging.Source: Coding guidelines
src/uis/nvcf-ui/ui/src/mocks/store/handlers.ts-43-92 (1)
43-92: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd mock-handler tests.
Cover
getStoreHandlerstenant isolation and unknown-record fallbacks. Add coverage forall-statuses; existing tests cover onlyempty-listandno-deployment. Runtask testinsrc/uis/nvcf-ui/uiand record the result before merge.🤖 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 `@src/uis/nvcf-ui/ui/src/mocks/store/handlers.ts` around lines 43 - 92, Add tests covering getStoreHandlers tenant isolation and unknown-record fallback responses for functions, deployments, and clusters; include the all-statuses scenario alongside the existing empty-list and no-deployment scenarios. In src/uis/nvcf-ui/ui/src/mocks/server.ts:18-21, src/uis/nvcf-ui/ui/src/mocks/scenarios/functions/all-statuses.ts:24-42, src/uis/nvcf-ui/ui/src/mocks/scenarios/functions/empty-list.ts:18-20, and src/uis/nvcf-ui/ui/src/mocks/scenarios/functions/no-deployment.ts:18-31, update only what is necessary to expose or exercise these cases; the handler implementation at src/uis/nvcf-ui/ui/src/mocks/store/handlers.ts:43-92 requires no direct change unless tests identify incorrect behavior. Run task test from src/uis/nvcf-ui/ui and record the result before merge.Source: Coding guidelines
src/uis/nvcf-ui/ui/index.html-20-24 (1)
20-24: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the Vite base URL for the favicon. Replace
/favicon.icowith%BASE_URL%favicon.ico. The root route already defines the"NVCF"document title.🤖 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 `@src/uis/nvcf-ui/ui/index.html` around lines 20 - 24, Update the favicon link in the HTML head to use Vite’s %BASE_URL% prefix instead of the root-relative /favicon.ico path, preserving the existing favicon filename and document title.src/uis/nvcf-ui/ui/scripts/vendor-kui-css.js-22-22 (1)
22-22: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd Node version constraint and fetch timeout.
import.meta.dirname(line 22) requires Node 20.11 or later. Thepackage.jsondeclares noenginesfield, so older Node versions producejoin(undefined, ...)and an unclearTypeErrorinstead of a version error. Add anengines.nodeconstraint toui/package.json.The
fetchcall (line 40) has no timeout. If the CDN is unresponsive, the script hangs indefinitely with no output. Passsignal: AbortSignal.timeout(30_000)to the fetch options.♻️ Proposed timeout
files.map(async (file) => { - const res = await fetch(`${cdn}/${file}`); + const res = await fetch(`${cdn}/${file}`, { + signal: AbortSignal.timeout(30_000), + }); if (!res.ok) throw new Error(`${file}: ${res.status}`);🤖 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 `@src/uis/nvcf-ui/ui/scripts/vendor-kui-css.js` at line 22, Add an engines.node constraint to the UI package metadata requiring Node 20.11 or newer, matching the use of import.meta.dirname in the vendor script. Update the fetch call in the same script to pass AbortSignal.timeout(30_000) through its options so unresponsive CDN requests terminate.src/uis/nvcf-ui/ui/src/rootRoute.tsx-39-42 (1)
39-42: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the
~/alias for UI source modules.These module specifiers resolve under
ui/src/but bypass the required source-root alias.
src/uis/nvcf-ui/ui/src/rootRoute.tsx#L39-L42: Replace each./...UI-source import with its~/...path.src/uis/nvcf-ui/ui/src/router.tsx#L19-L32: Replace component, feature, library, and root-route relative imports with~/...paths.src/uis/nvcf-ui/ui/src/mocks/store/index.ts#L18-L19: Replace relative re-export module specifiers with~/mocks/store/...paths.src/uis/nvcf-ui/ui/src/components/DataTable/Skeleton.tsx#L28-L28: ImportToolbarthrough~/components/DataTable/Toolbar.src/uis/nvcf-ui/ui/src/components/DataTable/Sort.tsx#L25-L26: Import DataTable modules through~/components/DataTable/....src/uis/nvcf-ui/ui/src/components/DataTable/index.ts#L18-L32: Import compound-component modules through~/components/DataTable/....src/uis/nvcf-ui/ui/src/components/DataTable/utils/columns.test.ts#L20-L20: Import the utility through~/components/DataTable/utils/columns.As per coding guidelines, use the
~/path alias for imports fromui/src/.🤖 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 `@src/uis/nvcf-ui/ui/src/rootRoute.tsx` around lines 39 - 42, Replace all relative UI-source imports with the ~/ alias: update imports in src/uis/nvcf-ui/ui/src/rootRoute.tsx lines 39-42, src/uis/nvcf-ui/ui/src/router.tsx lines 19-32, and src/uis/nvcf-ui/ui/src/mocks/store/index.ts lines 18-19; update the DataTable imports in src/uis/nvcf-ui/ui/src/components/DataTable/Skeleton.tsx line 28, Sort.tsx lines 25-26, index.ts lines 18-32, and utils/columns.test.ts line 20 using the specified ~/ paths.Source: Coding guidelines
src/uis/nvcf-ui/ui/src/components/AsyncBoundary.tsx-33-47 (1)
33-47: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd behavior coverage for
AsyncBoundary.No test covers its loading fallback or query-error reset behavior. Add a Vitest and Testing Library test, or explain why existing higher-level coverage makes a component test unnecessary.
🤖 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 `@src/uis/nvcf-ui/ui/src/components/AsyncBoundary.tsx` around lines 33 - 47, Add focused Vitest and Testing Library coverage for AsyncBoundary, verifying that fallback renders while children suspend and that the query-error reset path retries rendering after recovery. If equivalent higher-level tests already cover both behaviors, document that coverage instead of adding a duplicate component test.Source: Coding guidelines
src/uis/nvcf-ui/ui/src/rootRoute.tsx-116-118 (1)
116-118: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake cloud accounts a route-loader dependency.
Replace the discarded
prefetchQuery()call inbeforeLoadwith a rootloaderthat returnsensureQueryData({ ...getGetCloudAccountsQueryOptions(), revalidateIfStale: true }). Add tests for successful loading and route errors.🤖 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 `@src/uis/nvcf-ui/ui/src/rootRoute.tsx` around lines 116 - 118, Replace the discarded prefetchQuery call in the root route’s beforeLoad with a loader that returns ensureQueryData using getGetCloudAccountsQueryOptions() and revalidateIfStale: true, making cloud accounts a route-loader dependency. Add tests covering successful loader resolution and propagated route errors.Source: Coding guidelines
src/uis/nvcf-ui/ui/src/components/CopyButton.tsx-61-66 (1)
61-66: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winShow the success state only after the copy operation succeeds.
The handler discards the
copyToClipboardpromise. It then setsdata-copiedbefore the operation completes. A rejected copy therefore displays a false success state and can produce an unhandled rejection.Await the operation. Set
data-copiedonly after success. Handle the failure without showing the check icon.🤖 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 `@src/uis/nvcf-ui/ui/src/components/CopyButton.tsx` around lines 61 - 66, Update the onClick handler in CopyButton to await copyToClipboard(value) before setting data-copied. Handle rejected copy operations so failures do not produce unhandled rejections or set the success state, while preserving the existing success-state timeout behavior.src/uis/nvcf-ui/ui/src/components/DataTable/Content.tsx-63-77 (1)
63-77: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExpose the sort state with
aria-sort.Map
"asc"and"desc"to"ascending"and"descending"onTableHeaderCell, and omit the attribute when unsorted. Add a user-visible test for both states.TableHeaderCellforwards native<th>attributes.🤖 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 `@src/uis/nvcf-ui/ui/src/components/DataTable/Content.tsx` around lines 63 - 77, Update the TableHeaderCell in the sortable header rendering to set aria-sort to "ascending" or "descending" based on the existing sorted state, and omit the attribute when unsorted. Add a user-visible test covering both ascending and descending states.src/uis/nvcf-ui/ui/src/utils/formatters.test.ts-18-28 (1)
18-28: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCover the remaining exported formatters.
src/uis/nvcf-ui/ui/src/utils/formatters.tsLine 29 and Line 45 add user-visible formatting behavior, but this file only teststoTitleCase. Add cases for compact numbers, a valid local ISO timestamp, and thenullandundefineddate fallback.As per coding guidelines, “Code changes must include tests, or the Pull Request must explain why tests are unnecessary.”
🤖 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 `@src/uis/nvcf-ui/ui/src/utils/formatters.test.ts` around lines 18 - 28, Extend the formatter test suite in the existing describe block to cover the remaining exported functions from formatters.ts: verify compact-number formatting, formatting of a valid local ISO timestamp, and identical fallback behavior for null and undefined dates. Reuse the established Vitest table-driven style and assert the expected user-visible strings.Source: Coding guidelines
src/uis/nvcf-ui/ui/src/utils/resize-observer.ts-24-33 (1)
24-33: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReset scheduler state before callbacks run.
A callback that throws at Line 29 bypasses Line 31 and Line 32.
rafIdthen remains non-null, so Line 25 blocks every later resize update. Snapshot and clear the pending state before invoking callbacks. Add a regression test where one callback throws and a later resize still schedules.Proposed fix
rafId = requestAnimationFrame(() => { - for (const element of pending) { + const elements = [...pending]; + pending.clear(); + rafId = null; + + for (const element of elements) { callbacks.get(element)?.(); } - pending.clear(); - rafId = null; });As per coding guidelines, “Code changes must include tests, or the Pull Request must explain why tests are unnecessary.”
🤖 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 `@src/uis/nvcf-ui/ui/src/utils/resize-observer.ts` around lines 24 - 33, Update scheduleUpdate so it snapshots and clears pending and resets rafId before invoking any callbacks, ensuring a thrown callback cannot block future resize scheduling. Preserve callback execution for the captured entries, and add a regression test proving a throwing callback does not prevent a later resize update from scheduling.Source: Coding guidelines
src/uis/nvcf-ui/ui/src/components/DataTable/utils/tableSearchSchema.ts-21-22 (1)
21-22: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire integer pagination values.
pageandpageSizeflow directly into TanStack Table pagination state. Add.int()before.min(1)and add regression tests for decimal values falling back to1and10.🤖 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 `@src/uis/nvcf-ui/ui/src/components/DataTable/utils/tableSearchSchema.ts` around lines 21 - 22, Update the page and pageSize fields in the table search schema to require integer values by applying the integer constraint before the existing minimum validation, while preserving their current defaults and fallback behavior. Add regression coverage verifying decimal inputs fall back to 1 for page and 10 for pageSize.Source: Coding guidelines
src/uis/nvcf-ui/ui/src/utils/clipboard.test.ts-32-44 (1)
32-44: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore
document.execCommandafter this test.Line 38 replaces a global DOM method by direct assignment.
vi.restoreAllMocks()at Line 23 does not undo that assignment. Later tests can inherit the stub. Save and restore the original property, or use a restorable mock.🤖 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 `@src/uis/nvcf-ui/ui/src/utils/clipboard.test.ts` around lines 32 - 44, Update the test that stubs document.execCommand to preserve and restore the original DOM method after the test completes. Use a restorable mock or explicit cleanup around the assignment so later tests do not inherit the stub, while keeping the existing fallback and textarea cleanup assertions unchanged.src/uis/nvcf-ui/ui/src/components/OverflowGroup.test.tsx-21-21 (1)
21-21: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the required
~/alias for UI-source imports.
src/uis/nvcf-ui/ui/src/components/OverflowGroup.test.tsx#L21-L21: replace./OverflowGroupwith~/components/OverflowGroup.src/uis/nvcf-ui/ui/src/utils/clipboard.test.ts#L19-L19: replace./clipboardwith~/utils/clipboard.As per coding guidelines, use the
~/path alias for imports fromui/src/.🤖 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 `@src/uis/nvcf-ui/ui/src/components/OverflowGroup.test.tsx` at line 21, Update the import in src/uis/nvcf-ui/ui/src/components/OverflowGroup.test.tsx at lines 21-21 to use the ~/components/OverflowGroup alias instead of the relative path; likewise update src/uis/nvcf-ui/ui/src/utils/clipboard.test.ts at lines 19-19 to import clipboard through ~/utils/clipboard.Source: Coding guidelines
src/uis/nvcf-ui/ui/src/components/NotFound.tsx-22-39 (1)
22-39: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd behavior tests for the new route components.
The supplied import contains no corresponding tests or test exemption for these user-visible components.
src/uis/nvcf-ui/ui/src/components/NotFound.tsx#L22-L39: test the not-found status and dashboard navigation action.src/uis/nvcf-ui/ui/src/components/RouteErrorFallback.tsx#L22-L41: test error-message fallback and retry invalidation.src/uis/nvcf-ui/ui/src/components/RouteSpinner.tsx#L21-L27: test the accessible loading description.As per coding guidelines, when adding or modifying a component, add or update its corresponding tests.
🤖 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 `@src/uis/nvcf-ui/ui/src/components/NotFound.tsx` around lines 22 - 39, Add behavior tests for NotFound in src/uis/nvcf-ui/ui/src/components/NotFound.tsx:22-39 covering the not-found status and dashboard navigation; add tests for RouteErrorFallback in src/uis/nvcf-ui/ui/src/components/RouteErrorFallback.tsx:22-41 covering the error-message fallback and retry invalidation; and add tests for RouteSpinner in src/uis/nvcf-ui/ui/src/components/RouteSpinner.tsx:21-27 verifying its accessible loading description.Source: Coding guidelines
src/uis/nvcf-ui/ui/src/components/OverflowGroup.tsx-110-153 (1)
110-153: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHide
+Nafter expansion.When
expandedis true,hiddenChildrenstill contains the previously hidden entries. Lines 110-119 still create+N, and Lines 143-153 render it beside all children andSee Less. Render the overflow trigger only before expansion. Add an assertion that+Nis absent after expansion.As per coding guidelines, update the component's corresponding test.
Proposed fix
- const overflowButton = hiddenChildren.length > 0 && ( + const overflowButton = !expanded && hiddenChildren.length > 0 && (🤖 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 `@src/uis/nvcf-ui/ui/src/components/OverflowGroup.tsx` around lines 110 - 153, Update OverflowGroup’s overflowButton rendering so the +N trigger is created and displayed only when expanded is false, while preserving the existing popover behavior before expansion and rendering all children with See Less after expansion. Add or update the corresponding component test to assert that +N is absent when expanded.Source: Coding guidelines
src/uis/nvcf-ui/ui/src/utils/clipboard.ts-26-41 (1)
26-41: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport success only after a successful copy.
Line 38 discards the
document.execCommand("copy")result. IfwriteTextrejects and the fallback returnsfalse, this helper resolves successfully. The downstream handler insrc/uis/nvcf-ui/ui/src/components/CopyButton.tsxLines 61-67 marks the value as copied before this promise settles. Return a failure result, await it in each consumer, and add a failed-fallback test.Proposed coordinated fix
- export async function copyToClipboard(text: string): Promise<void> { + export async function copyToClipboard(text: string): Promise<boolean> { try { await navigator.clipboard.writeText(text); + return true; } catch { // ... try { - document.execCommand("copy"); + return document.execCommand("copy"); } finally { document.body.removeChild(textarea); } } }- onClick={() => { - void copyToClipboard(value); + onClick={async () => { + const copied = await copyToClipboard(value).catch(() => false); + if (!copied) return; // Set the copied state here. }}🤖 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 `@src/uis/nvcf-ui/ui/src/utils/clipboard.ts` around lines 26 - 41, Update copyToClipboard to return or propagate the document.execCommand("copy") result instead of discarding it, and make each CopyButton consumer await and inspect that result before marking the value as copied. Add coverage for a failed fallback copy, preserving success behavior for navigator.clipboard.writeText and successful fallback copies.src/uis/nvcf-ui/backend/internal/utils/utils.go-40-52 (1)
40-52: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe fatal message loses the variable name because
logLevelis shadowed.Line 43 declares a
zerolog.LevelnamedlogLevel, which shadows the const on line 29 for the rest of the function. Line 45 therefore formats the parsed level, not"LOG_LEVEL". On a parse error the level isNoLevel, which renders as an empty string, so the operator seesInvalidwith no variable name and no rejected value. The code still compiles, because the short variable declaration takes effect only after line 43.Rename the const and include the rejected value. Compare against
zerolog.NoLevelinstead of the empty string.🐛 Proposed fix
-const logLevel = "LOG_LEVEL" +const logLevelEnv = "LOG_LEVEL" @@ func ConfigLogger() zerolog.Logger { logger := zerolog.New(os.Stdout).With().Caller().Timestamp().Logger() // To disable logging entirely, pass [zerolog.Disabled] - logLevel, err := zerolog.ParseLevel(os.Getenv(logLevel)) + raw := os.Getenv(logLevelEnv) + level, err := zerolog.ParseLevel(raw) if err != nil { - logger.Fatal().Err(err).Msgf("Invalid %s", logLevel) + logger.Fatal().Err(err).Msgf("Invalid %s=%q", logLevelEnv, raw) } - if logLevel.String() == "" { - logLevel = zerolog.InfoLevel + if level == zerolog.NoLevel { + level = zerolog.InfoLevel } - return logger.Level(logLevel) + return logger.Level(level) }The rename also requires an update in
src/uis/nvcf-ui/backend/internal/utils/utils_test.goat line 77:t.Setenv(logLevelEnv, tt.level)🤖 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 `@src/uis/nvcf-ui/backend/internal/utils/utils.go` around lines 40 - 52, Update ConfigLogger to use distinct names for the LOG_LEVEL environment-variable constant and parsed zerolog.Level, then report both the variable name and rejected value in the parse-error log. Replace the empty-string check with a comparison against zerolog.NoLevel, and update utils_test.go to use the renamed environment-variable constant.src/uis/nvcf-ui/backend/internal/middleware/middleware.go-60-75 (1)
60-75: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRe-panic on
http.ErrAbortHandler; remove the write guard.
hlog.AccessHandleralready suppresses duplicateWriteHeadercalls and records the upstream status.ReverseProxyuseshttp.ErrAbortHandlerfor interrupted streams, butPanicRecoverycurrently swallows it. Re-panic before logging.🤖 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 `@src/uis/nvcf-ui/backend/internal/middleware/middleware.go` around lines 60 - 75, Update PanicRecovery to detect http.ErrAbortHandler immediately after recover and re-panic it before logging or writing a response. Remove the unconditional w.WriteHeader(http.StatusInternalServerError) from the recovery handler, while preserving error logging for other panics.src/uis/nvcf-ui/helm/templates/vault-agent-configmap.yaml-24-34 (1)
24-34: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake the token JSON valid when a secret lookup is skipped. If
sis-apiis absent, the trailing comma causesjson.Unmarshalto fail. The watcher exits on an invalid initial file and ignores the invalid reload. Use conditional comma placement so every rendered combination is valid JSON.🤖 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 `@src/uis/nvcf-ui/helm/templates/vault-agent-configmap.yaml` around lines 24 - 34, Update the Vault template JSON block around the secret lookups for nvcfApiToken, nvctApiToken, and sisApiToken so commas are emitted conditionally based on whether another token field will be rendered. Ensure every combination of available or skipped secrets produces valid JSON, including when sis-api is absent.Source: Linters/SAST tools
src/uis/nvcf-ui/tools/workspace_status.sh-43-48 (1)
43-48: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winValidate OCI tag inputs before emitting stamp keys.
src/uis/nvcf-ui/BUILD.bazeluses//rules/oci, whose push template consumesSTABLE_VERSIONandSTABLE_OCI_TAG.NVCF_VERSIONand exact Git tags are emitted verbatim, so/,+, leading invalid characters, or excessive length can make the push fail.STABLE_GIT_BRANCHis not used by the OCI template and must remain unchanged for CLI metadata.🤖 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 `@src/uis/nvcf-ui/tools/workspace_status.sh` around lines 43 - 48, Update the stamp generation in workspace_status.sh to validate or normalize NVCF_VERSION and exact Git-tag-derived values before emitting STABLE_VERSION and STABLE_OCI_TAG, ensuring OCI-compatible characters, starting character, and length while preserving the existing commit/dirty composition. Leave STABLE_GIT_BRANCH unchanged because it remains CLI metadata only.src/uis/nvcf-ui/helm/templates/deployment.yaml-51-60 (1)
51-60: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSecurity Misconfiguration (CWE-250)
Reachability: External · Exploitability: Difficult
Harden both UI containers by default
The default container context already enforces non-root execution and drops all capabilities. Add
allowPrivilegeEscalation: false,seccompProfile.type: RuntimeDefault, andreadOnlyRootFilesystem: trueto the defaults for both deployments. Add writable volumes only where application writes require them.🤖 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 `@src/uis/nvcf-ui/helm/templates/deployment.yaml` around lines 51 - 60, Harden the default container security contexts in both src/uis/nvcf-ui/helm/templates/deployment.yaml:51-60 and src/uis/nvcf-ui/helm/templates/control-plane-deployment.yaml:47-56 by adding allowPrivilegeEscalation: false, seccompProfile.type: RuntimeDefault, and readOnlyRootFilesystem: true alongside the existing non-root and capability settings. Add writable volume mounts and volumes only for paths each UI container must write to, preserving read-only root filesystems otherwise.Source: Linters/SAST tools
| ```bash | ||
| VITE_MOCK=true task dev | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the included UI task name.
src/uis/nvcf-ui/Taskfile.yml does not define a root dev task. Both commands fail with a missing-task error.
Replace task dev with task ui:dev in mocked mode and real-cluster mode.
Proposed fix
-VITE_MOCK=true task dev
+VITE_MOCK=true task ui:dev
@@
-task dev
+task ui:devAlso applies to: 33-35
🤖 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 `@src/uis/nvcf-ui/.claude/skills/run/SKILL.md` around lines 17 - 19, Update
both mocked-mode and real-cluster-mode commands in the run skill to invoke the
defined UI task name, replacing task dev with task ui:dev while preserving the
existing VITE_MOCK setting and command context.
| server := http.Server{ | ||
| Addr: fmt.Sprintf("0.0.0.0:%d", port), | ||
| Handler: handler, | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Set timeouts on the HTTP server.
The server has no ReadHeaderTimeout, ReadTimeout, WriteTimeout, or IdleTimeout. A slow client can hold connections open and exhaust server resources. Static analysis flags the same problem.
🛡️ Proposed fix
server := http.Server{
- Addr: fmt.Sprintf("0.0.0.0:%d", port),
- Handler: handler,
+ Addr: fmt.Sprintf("0.0.0.0:%d", port),
+ Handler: handler,
+ ReadHeaderTimeout: 10 * time.Second,
+ ReadTimeout: 30 * time.Second,
+ WriteTimeout: 60 * time.Second,
+ IdleTimeout: 120 * time.Second,
}🤖 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 `@src/uis/nvcf-ui/backend/cmd/server/main.go` around lines 121 - 124, Update
the http.Server initialization in main to configure nonzero ReadHeaderTimeout,
ReadTimeout, WriteTimeout, and IdleTimeout values, using the project's
established timeout constants or appropriate durations so slow clients cannot
hold connections indefinitely.
Source: Linters/SAST tools
| func newProxy(target *url.URL, token func() (string, bool), logger zerolog.Logger) http.Handler { | ||
| proxy := &httputil.ReverseProxy{ | ||
| Rewrite: func(pr *httputil.ProxyRequest) { | ||
| pr.SetURL(target) | ||
| pr.SetXForwarded() | ||
| pr.Out.Host = target.Host | ||
| }, | ||
| ErrorHandler: func(w http.ResponseWriter, r *http.Request, err error) { | ||
| if errors.Is(err, context.Canceled) { // client went away; nothing to send | ||
| return | ||
| } | ||
| logger.Error().Err(err).Msgf("upstream %s error", target.Host) | ||
| w.WriteHeader(http.StatusBadGateway) | ||
| }, | ||
| } | ||
|
|
||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| t, ok := token() | ||
| if !ok || t == "" { | ||
| w.WriteHeader(http.StatusInternalServerError) | ||
| return | ||
| } | ||
| r.Header.Set("Authorization", "Bearer "+t) | ||
| proxy.ServeHTTP(w, r) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for authentication/authorization in the backend middleware and the Helm route definitions.
set -euo pipefail
fd -t f 'middleware.go' | xargs -r rg -n -C4 'Auth|auth|Token|Bearer|Forbidden|Unauthorized'
fd -t f -e yaml -e yml . src/uis/nvcf-ui/helm | xargs -r rg -n -C4 'auth|oidc|jwt|filters|extensionRef|requestAuthentication'Repository: NVIDIA/nvcf
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- server routing and middleware ---'
sed -n '1,210p' src/uis/nvcf-ui/backend/cmd/server/main.go
printf '%s\n' '--- backend files and deployment manifests ---'
git ls-files src/uis/nvcf-ui | grep -E '(^|/)(.*(helm|chart|route|ingress|gateway|middleware|auth).*)\.(go|yaml|yml|tpl)$' | head -200
printf '%s\n' '--- exact route and authentication references ---'
rg -n -C3 --glob '!vendor/**' --glob '!**/node_modules/**' \
'(/v2/nvcf/accounts|/v1/nvct/accounts|/v1/si/accounts|newProxy|Authorization|authentication|RequestAuthentication|AuthorizationPolicy|oauth|oidc|jwt|Bearer)' \
src/uis/nvcf-uiRepository: NVIDIA/nvcf
Length of output: 49354
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- HTTPRoute ---'
cat -n src/uis/nvcf-ui/helm/templates/httproute.yaml
printf '%s\n' '--- Helm values relevant to exposure ---'
cat -n src/uis/nvcf-ui/helm/values.yaml | sed -n '1,180p'
printf '%s\n' '--- deployment and service exposure ---'
cat -n src/uis/nvcf-ui/helm/templates/deployment.yaml
cat -n src/uis/nvcf-ui/helm/templates/service.yaml
printf '%s\n' '--- documented trust boundary ---'
cat -n src/uis/nvcf-ui/SECURITY.md | sed -n '88,175p'Repository: NVIDIA/nvcf
Length of output: 25925
Missing Caller Authentication Before Privileged Credential Injection (CWE-306): Missing Authentication for Critical Function
Enforce caller authentication before injecting the service token.
The BFF has no caller authentication or authorization. The default HTTPRoute has no authentication filter, and the route is optional. Ensure every exposed deployment places an authenticating gateway before /v2/nvcf/accounts/, /v1/nvct/accounts/, and /v1/si/accounts/; otherwise any reachable caller can access upstream APIs with the BFF's credentials. Replace the hard-coded http upstream URLs with TLS/mTLS or enforce equivalent network encryption.
🤖 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 `@src/uis/nvcf-ui/backend/cmd/server/main.go` around lines 145 - 170, Update
the deployment and routing configuration around newProxy so every exposed
deployment authenticates and authorizes callers before requests reach the
protected account paths. Ensure the gateway covers /v2/nvcf/accounts/,
/v1/nvct/accounts/, and /v1/si/accounts/ even when the HTTPRoute is optional,
and replace plaintext upstream targets passed to newProxy with TLS/mTLS or an
equivalent encrypted network path.
| func (w *Watcher) load() error { | ||
| data, err := os.ReadFile(w.tokensPath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| var t apiTokens | ||
| if err := json.Unmarshal(data, &t); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| w.mu.Lock() | ||
|
|
||
| // Mark all tokens valid and cancel any in-flight expiry goroutines. | ||
| w.nvcf.value, w.nvcf.valid = t.NvcfApiToken, true | ||
| w.nvct.value, w.nvct.valid = t.NvctApiToken, true | ||
| w.sis.value, w.sis.valid = t.SisApiToken, true |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
load marks a token valid even when the value is empty.
json.Unmarshal leaves a missing or empty field as "". Lines 150-152 then set valid = true for that empty value. startExpiryWatcher fails to parse the empty string, logs a warning, and returns, so the token stays "valid" for the process lifetime. The proxy in cmd/server/main.go receives ("", true) and forwards the request with an empty credential instead of failing fast with a clear error.
Set valid only when the value is non-empty, and report the missing token.
🐛 Proposed fix
+ logger := zerolog.Ctx(w.ctx)
+ set := func(name, value string) bool {
+ if value == "" {
+ logger.Error().Msgf("%s token missing from %s", name, w.tokensPath)
+ return false
+ }
+ return true
+ }
+
w.mu.Lock()
- // Mark all tokens valid and cancel any in-flight expiry goroutines.
- w.nvcf.value, w.nvcf.valid = t.NvcfApiToken, true
- w.nvct.value, w.nvct.valid = t.NvctApiToken, true
- w.sis.value, w.sis.valid = t.SisApiToken, true
+ // Mark non-empty tokens valid and cancel any in-flight expiry goroutines.
+ w.nvcf.value, w.nvcf.valid = t.NvcfApiToken, set("nvcf", t.NvcfApiToken)
+ w.nvct.value, w.nvct.valid = t.NvctApiToken, set("nvct", t.NvctApiToken)
+ w.sis.value, w.sis.valid = t.SisApiToken, set("sis", t.SisApiToken)📝 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.
| func (w *Watcher) load() error { | |
| data, err := os.ReadFile(w.tokensPath) | |
| if err != nil { | |
| return err | |
| } | |
| var t apiTokens | |
| if err := json.Unmarshal(data, &t); err != nil { | |
| return err | |
| } | |
| w.mu.Lock() | |
| // Mark all tokens valid and cancel any in-flight expiry goroutines. | |
| w.nvcf.value, w.nvcf.valid = t.NvcfApiToken, true | |
| w.nvct.value, w.nvct.valid = t.NvctApiToken, true | |
| w.sis.value, w.sis.valid = t.SisApiToken, true | |
| func (w *Watcher) load() error { | |
| data, err := os.ReadFile(w.tokensPath) | |
| if err != nil { | |
| return err | |
| } | |
| var t apiTokens | |
| if err := json.Unmarshal(data, &t); err != nil { | |
| return err | |
| } | |
| logger := zerolog.Ctx(w.ctx) | |
| set := func(name, value string) bool { | |
| if value == "" { | |
| logger.Error().Msgf("%s token missing from %s", name, w.tokensPath) | |
| return false | |
| } | |
| return true | |
| } | |
| w.mu.Lock() | |
| // Mark non-empty tokens valid and cancel any in-flight expiry goroutines. | |
| w.nvcf.value, w.nvcf.valid = t.NvcfApiToken, set("nvcf", t.NvcfApiToken) | |
| w.nvct.value, w.nvct.valid = t.NvctApiToken, set("nvct", t.NvctApiToken) | |
| w.sis.value, w.sis.valid = t.SisApiToken, set("sis", t.SisApiToken) |
🤖 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 `@src/uis/nvcf-ui/backend/internal/token-watcher/watcher.go` around lines 137 -
152, Update Watcher.load so each token’s valid flag is set only when its
unmarshaled value is non-empty, and report missing or empty tokens through the
existing error-reporting mechanism. Ensure empty credentials are not exposed as
valid to the proxy while preserving valid-token loading and expiry-watcher
behavior.
| # Helm hook Job that provisions the OpenBao JWT signing roles the nvcf-ui BFF | ||
| # server needs to mint cluster, NVCF API, and NVCT tokens. Runs pre-install and | ||
| # pre-upgrade so the auth role and sign policies exist before the server pods | ||
| # come up and the Vault Agent tries to log in. | ||
| {{- $bao := .Values.nvcfUi.openbaoMigrations }} | ||
| apiVersion: batch/v1 | ||
| kind: Job |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Render the migration Job only when it is enabled.
This template always runs the pre-install and pre-upgrade Job. An installation without the external ServiceAccount or root-token Secret will fail before the application resources are installed.
Add an openbaoMigrations.enabled value. Wrap the complete Job with that condition. The PR context describes this hook as opt-in.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 20-20: syntax error: expected the node content, but found '-'
(syntax)
🤖 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 `@src/uis/nvcf-ui/helm/templates/hook-openbao-migrations.yaml` around lines 16
- 22, Add an enabled guard around the complete Job manifest in the
hook-openbao-migrations template, using the existing
.Values.nvcfUi.openbaoMigrations configuration via $bao. Introduce or consume
openbaoMigrations.enabled so the pre-install and pre-upgrade migration Job
renders only when explicitly enabled, while preserving the existing Job content
unchanged.
| return useSuspenseQueries({ | ||
| queries: [ | ||
| { | ||
| ...getGetAllFunctionsSuspenseQueryOptions(ncaId), | ||
| ...queryOptions, | ||
| }, | ||
| { | ||
| ...getGetAllFunctionDeploymentsSuspenseQueryOptions(ncaId), | ||
| ...queryOptions, | ||
| }, | ||
| ], | ||
| combine(results) { | ||
| const base = combine(results); | ||
| return (select ? select(base) : base) as T; | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/uis/nvcf-ui/ui/src/features/functions/FunctionsList.test.tsx --items all
rg -n -C 5 'useSuspenseQueries|allSettled|getGetAllFunctionDeployments' \
src/uis/nvcf-ui/ui/src/features/functionsRepository: NVIDIA/nvcf
Length of output: 7170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- hook ---'
cat -n src/uis/nvcf-ui/ui/src/features/functions/hooks/useFunctionsWithDeployments.ts
printf '%s\n' '--- route ---'
sed -n '35,90p' src/uis/nvcf-ui/ui/src/features/functions/routes.tsx
printf '%s\n' '--- function-list tests ---'
cat -n src/uis/nvcf-ui/ui/src/features/functions/FunctionsList.test.tsx
printf '%s\n' '--- related consumers and query option types ---'
rg -n -C 4 'useFunctionsWithDeployments|SharedQueryOptions|BaseCombineResult|functionDeployments' \
src/uis/nvcf-ui/ui/src/features/functions \
src/uis/nvcf-ui/ui/src/generated/apiRepository: NVIDIA/nvcf
Length of output: 12294
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- hook ---'
cat -n src/uis/nvcf-ui/ui/src/features/functions/hooks/useFunctionsWithDeployments.ts
printf '%s\n' '--- route ---'
sed -n '35,90p' src/uis/nvcf-ui/ui/src/features/functions/routes.tsx
printf '%s\n' '--- function-list tests ---'
cat -n src/uis/uis/nvcf-ui/ui/src/features/functions/FunctionsList.test.tsx
printf '%s\n' '--- related consumers and query option types ---'
rg -n -C 4 'useFunctionsWithDeployments|SharedQueryOptions|BaseCombineResult|functionDeployments' \
src/uis/nvcf-ui/ui/src/features/functions \
src/uis/nvcf-ui/ui/src/generated/apiRepository: NVIDIA/nvcf
Length of output: 5280
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- package versions ---'
rg -n '"`@tanstack/react-query`"|"`@tanstack/react-router`"' \
package.json '**/package.json' '**/pnpm-lock.yaml' '**/yarn.lock' '**/package-lock.json' 2>/dev/null || true
printf '%s\n' '--- generated query options ---'
rg -n -A 35 -B 8 \
'getGetAllFunctionDeployments(Suspense)?QueryOptions|getGetAllFunctions(Suspense)?QueryOptions' \
src/uis/nvcf-ui/ui/src/generated/api/function-deployment/function-deployment.ts \
src/uis/nvcf-ui/ui/src/generated/api/function-management/function-management.ts
printf '%s\n' '--- deployment mocks and function-list test setup ---'
rg -n -C 8 \
'getGetAllFunctionDeploymentsMockHandler|function-deployment|deployments' \
src/uis/nvcf-ui/ui/src/mocks \
src/uis/nvcf-ui/ui/src/features/functions/FunctionsList.test.tsx
printf '%s\n' '--- all direct hook tests/usages ---'
rg -n -C 5 'useFunctionsWithDeployments' src/uis/nvcf-ui/ui/srcRepository: NVIDIA/nvcf
Length of output: 49260
🌐 Web query:
TanStack Query React useSuspenseQueries error behavior throwOnError query state data undefined v5
💡 Result:
In TanStack Query v5, useSuspenseQueries is designed to strictly adhere to the Suspense pattern, which means the throwOnError option is not supported for individual queries and cannot be passed as an argument [1][2][3]. Regarding error behavior and data state: 1. Default Error Handling: By default, useSuspenseQueries (like useSuspenseQuery) only throws errors to the nearest React Error Boundary if no previously cached data is available [4][5]. If stale data exists in the cache, the hook will return that data instead of throwing, prioritizing the display of stale information over an error state [4][6][5]. 2. data Undefined State: Because useSuspenseQueries guarantees that data is defined in the successful rendering path, it will not return undefined data while in the success state [1][2][5]. If you encounter data is undefined, it typically indicates that your queryFn returned undefined or failed to return a value, which is not allowed [7]. 3. Manual Error Propagation: If you need to force an error to be thrown to an Error Boundary even when stale data is present, you must manually throw the error in your component [4][8][5]. A common pattern is: tsx const results = useSuspenseQueries({ queries: [...], }); // Manual throw for Error Boundary results.forEach((result) => { if (result.error && !result.isFetching) { throw result.error; } }); Because throwOnError is disallowed by design in useSuspenseQueries to ensure the hook's contract (data is always defined) remains consistent with Suspense, you cannot disable this behavior or configure it via the hook options [9][3]. If your use case requires granular control over whether to show stale data versus an error boundary, you may need to use useQueries with suspense: false and handle the loading/error states manually [5][3].
Citations:
- 1: https://tanstack.com/query/latest/docs/framework/react/reference/useSuspenseQueries
- 2: https://tanstack.com/query/v5/docs/framework/react/reference/useSuspenseQueries.md
- 3: throwOnError: false doesn't work with useSuspenseQueries TanStack/query#9711
- 4: https://tanstack.com/query/latest/docs/framework/react/guides/suspense
- 5: https://tanstack.com/query/v5/docs/framework/react/guides/suspense
- 6: useSuspenseQuery do not throw when a "refetch" throws? TanStack/query#7585
- 7: Unhandled Runtime Error: data is undefined in useSuspenseQuery with Next.js and Tanstack Query TanStack/query#7550
- 8: https://tanstack.com/query/latest/docs/framework/react/guides/suspense.md
- 9: useSuspenseQuery - throw error while having stale data TanStack/query#6960
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- UI dependency manifests ---'
fd -i -t f 'package.json|pnpm-lock.yaml|yarn.lock|package-lock.json' . | while read -r file; do
if rg -q '`@tanstack/react-query`' "$file"; then
printf '%s\n' "--- $file"
rg -n -C 2 '`@tanstack/react-query`' "$file"
fi
done
printf '%s\n' '--- query client defaults ---'
rg -n -C 8 'QueryClient|defaultOptions|retry|throwOnError|suspense' \
src/uis/nvcf-ui/ui/src/lib src/uis/nvcf-ui/ui/src/testing \
src/uis/nvcf-ui/ui/src 2>/dev/null | head -n 240
printf '%s\n' '--- route error boundary and test harness ---'
rg -n -C 8 'RouteErrorFallback|defaultErrorComponent|renderWithRouter' \
src/uis/nvcf-ui/ui/srcRepository: NVIDIA/nvcf
Length of output: 50367
Keep deployment enrichment non-blocking.
If the deployment request fails before data is cached, useSuspenseQueries throws and does not call combine. FunctionsList then cannot render functions without deployment enrichment, which conflicts with functionsListRoute treating deployment failures as best effort. Use a non-suspense deployment query, or make deployments mandatory in the route. Add a regression test for this failure path.
🤖 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
`@src/uis/nvcf-ui/ui/src/features/functions/hooks/useFunctionsWithDeployments.ts`
around lines 64 - 79, The useFunctionsWithDeployments hook currently suspends
the entire result when the deployment query fails, preventing FunctionsList from
rendering base functions. Update the deployment query in
useFunctionsWithDeployments to use a non-suspense, best-effort path while
preserving the suspense behavior for function data and existing select/combine
handling; add a regression test covering an uncached deployment failure and
confirming functions still render without enrichment.
Source: Coding guidelines
| const body = [204, 205, 304].includes(res.status) | ||
| ? null | ||
| : await res.json().catch(() => null); | ||
|
|
||
| if (!res.ok) throw new HttpError(res, body); | ||
|
|
||
| return body as T; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject malformed JSON from successful responses.
Lines 36-42 convert a JSON parsing failure into null and cast it to T. The generated clusters client uses customFetch. ClustersList.tsx then converts this null into an empty data set at Line 116.
This hides a malformed successful response as “No data.” Other callers can dereference null while TypeScript reports T. For successful responses that are not explicit no-content responses, rethrow the JSON parsing failure. Keep best-effort parsing only for non-2xx HttpError bodies. Update fetch.test.ts Lines 52-59 to expect rejection.
Proposed fix
- const body = [204, 205, 304].includes(res.status)
- ? null
- : await res.json().catch(() => null);
+ let body: unknown = null;
+
+ if (![204, 205, 304].includes(res.status)) {
+ try {
+ body = await res.json();
+ } catch (error) {
+ if (res.ok) throw error;
+ }
+ }
if (!res.ok) throw new HttpError(res, body);📝 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.
| const body = [204, 205, 304].includes(res.status) | |
| ? null | |
| : await res.json().catch(() => null); | |
| if (!res.ok) throw new HttpError(res, body); | |
| return body as T; | |
| let body: unknown = null; | |
| if (![204, 205, 304].includes(res.status)) { | |
| try { | |
| body = await res.json(); | |
| } catch (error) { | |
| if (res.ok) throw error; | |
| } | |
| } | |
| if (!res.ok) throw new HttpError(res, body); | |
| return body as T; |
🤖 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 `@src/uis/nvcf-ui/ui/src/lib/fetch.ts` around lines 36 - 42, Update customFetch
so JSON parsing failures for successful, non-no-content responses are rethrown
instead of converted to null or cast to T. Preserve null for explicit 204, 205,
and 304 responses, and retain best-effort parsing for non-2xx responses before
throwing HttpError. Update the corresponding fetch.test.ts case to assert
rejection.
|
|
||
| import { ThemeProvider } from "@nvidia/foundations-react-core"; | ||
| import { QueryClientProvider } from "@tanstack/react-query"; | ||
| import { ReactQueryDevtools } from "@tanstack/react-query-devtools"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
ReactQueryDevtools comes from a devDependency but renders unconditionally.
@tanstack/react-query-devtools is listed under devDependencies in ui/package.json, and src/main.tsx imports and renders it on every code path. A production install that omits dev dependencies then fails to build. The devtools panel and its bundle also ship to end users.
Guard the render on import.meta.env.DEV, or move the package to dependencies if the panel is intentional in production.
Also applies to: 55-55
🤖 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 `@src/uis/nvcf-ui/ui/src/main.tsx` at line 20, Update the ReactQueryDevtools
usage in main.tsx so it is rendered only when import.meta.env.DEV is true,
keeping the devtools package out of production builds and runtime paths while
preserving its development behavior.
| enableMocking().then(() => { | ||
| const rootElement = document.getElementById("app"); | ||
| if (rootElement && !rootElement.innerHTML) { | ||
| const root = ReactDOM.createRoot(rootElement); | ||
| root.render( | ||
| <ThemeProvider | ||
| density="standard" | ||
| global | ||
| target="html" | ||
| theme={getStoredTheme()} | ||
| > | ||
| <QueryClientProvider client={queryClient}> | ||
| <RouterProvider context={{ queryClient }} router={router} /> | ||
| <ReactQueryDevtools /> | ||
| </QueryClientProvider> | ||
| </ThemeProvider>, | ||
| ); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Render the app even when mock setup fails.
enableMocking() can reject. The dynamic import of ./mocks/browser can fail, and worker.start() rejects when service-worker registration is refused, for example on an insecure origin. The chain has no rejection handler, so .then() never runs, the root stays empty, and the user sees a blank page with only an unhandled rejection in the console.
Log the failure and continue to render.
🐛 Proposed fix
-enableMocking().then(() => {
+function renderApp() {
const rootElement = document.getElementById("app");
if (rootElement && !rootElement.innerHTML) {
const root = ReactDOM.createRoot(rootElement);
@@
);
}
-});
+}
+
+enableMocking()
+ .catch((error) => {
+ console.error("Failed to start request mocking", error);
+ })
+ .finally(renderApp);📝 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.
| enableMocking().then(() => { | |
| const rootElement = document.getElementById("app"); | |
| if (rootElement && !rootElement.innerHTML) { | |
| const root = ReactDOM.createRoot(rootElement); | |
| root.render( | |
| <ThemeProvider | |
| density="standard" | |
| global | |
| target="html" | |
| theme={getStoredTheme()} | |
| > | |
| <QueryClientProvider client={queryClient}> | |
| <RouterProvider context={{ queryClient }} router={router} /> | |
| <ReactQueryDevtools /> | |
| </QueryClientProvider> | |
| </ThemeProvider>, | |
| ); | |
| } | |
| }); | |
| function renderApp() { | |
| const rootElement = document.getElementById("app"); | |
| if (rootElement && !rootElement.innerHTML) { | |
| const root = ReactDOM.createRoot(rootElement); | |
| root.render( | |
| <ThemeProvider | |
| density="standard" | |
| global | |
| target="html" | |
| theme={getStoredTheme()} | |
| > | |
| <QueryClientProvider client={queryClient}> | |
| <RouterProvider context={{ queryClient }} router={router} /> | |
| <ReactQueryDevtools /> | |
| </QueryClientProvider> | |
| </ThemeProvider>, | |
| ); | |
| } | |
| } | |
| enableMocking() | |
| .catch((error) => { | |
| console.error("Failed to start request mocking", error); | |
| }) | |
| .finally(renderApp); |
🤖 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 `@src/uis/nvcf-ui/ui/src/main.tsx` around lines 42 - 60, Update the
enableMocking() startup chain so rejected mock initialization is logged and
still proceeds to the existing app-rendering logic. Add a rejection handler or
equivalent fallback around enableMocking(), preserving the current rootElement
check and ReactDOM.createRoot rendering path.
| @import "@nvidia/foundations-tailwind-plugin"; | ||
| @source "../node_modules/@nvidia/foundations-react-core"; | ||
|
|
||
| #style-root { | ||
| @tailwind utilities; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm the installed Tailwind version and locate the Stylelint rule configuration.
rg -n -C 2 '"tailwindcss"|`@tailwindcss`' src/uis/nvcf-ui/ui/package.json
rg -n --hidden --glob '!node_modules/**' --glob '!dist/**' -C 2 \
'at-rule-no-unknown|ignoreAtRules|`@tailwind`|`@source`' src/uis/nvcf-uiRepository: NVIDIA/nvcf
Length of output: 14892
Use the Tailwind v4 entrypoint and allow @source in Stylelint. Tailwind resolves to v4.2.4. Replace @tailwind utilities with @import "tailwindcss" and configure Stylelint to accept @source.
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 23-23: Unexpected unknown at-rule "@source" (scss/at-rule-no-unknown)
(scss/at-rule-no-unknown)
[error] 26-26: Unexpected unknown at-rule "@tailwind" (scss/at-rule-no-unknown)
(scss/at-rule-no-unknown)
🤖 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 `@src/uis/nvcf-ui/ui/src/styles.css` around lines 22 - 26, Update the
`#style-root` block in styles.css to use Tailwind v4’s `@import` "tailwindcss"
entrypoint instead of `@tailwind` utilities, and adjust the Stylelint
configuration to recognize the existing `@source` directive.
Sources: Learnings, Linters/SAST tools
The import landed source-only: the subtree kept its upstream MODULE.bazel and sat in .bazelignore, so nothing in this repository built, tested or linted its 185 TypeScript and 29 Go files. Excluding the upstream .gitlab-ci.yml had also removed the six quality gates it used to run, replacing them with none. This finishes the migration so the code is actually verified here. check-nested-modules now reports `awaiting migration: 0` again, so the import no longer works against the consolidation. The merge, in the order the failures surfaced: - Lifted aspect_rules_js and rules_nodejs plus the node and npm extensions into the root MODULE.bazel, repointed at //src/uis/nvcf-ui/ui:pnpm-lock.yaml. - Registered the backend in go.work.bazel and rewrote subtree-relative labels. - Dropped the subtree's MODULE.bazel, .bazelrc, rules/ and platforms/; the root already provides distroless_static, rules/oci and every Go dependency the backend imports. - Lifted the subtree's .bazelignore entries into the root. aspect_rules_js refuses to evaluate npm_translate_lock unless every nested node_modules pnpm will create is ignored, so this is required, not tidiness. - Ported go_web_oci_image into the root rules/oci. The root copy was not equivalent; it had no such rule. - Dropped that rule's repo_tag parameter. Its own doc said it existed because package_name() is empty at a repo root; at src/uis/nvcf-ui it is not, so the root's derived load tag applies and the workaround is obsolete. - Removed //platforms:sources and //rules/oci:sources from the container's source bundle. Those are root build infrastructure, not UI source. - Aligned the backend's controller-runtime from v0.24.1 to v0.22.5 to match the rest of the repository. A version-specific replace pins client-go to v0.34.2 while MVS was selecting controller-runtime v0.24.1 from this module, and 0.24 needs client-go APIs that 0.34 does not have. Merges main for the pnpm dependency collector added in #657. Without it the UI's npm tree never reached dependencies.md; the regenerated file now records 531 Node.js packages. Adds the nvcf-ui matrix row so the subtree builds and tests on its own changes. Verified locally: `bazel build //src/uis/nvcf-ui/...` covers 31 targets and `bazel test //src/uis/nvcf-ui/...` passes 5 of 5, including the frontend vitest suite. The controller-runtime downgrade is proven at compile and unit-test level only. The token-watcher is the component using it, and its tests do not exercise cache behaviour, so a reviewer from the UI team should confirm 0.22.5 is acceptable at runtime. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rules/oci/private/go_web.bzl (1)
75-77: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReject an empty
binarieslist before indexing it.
mandatory = Truedoes not reject an explicitly empty list, andallow_emptydefaults toTrue. Setallow_empty = Falseor validatebinariesbefore accessingbinaries[0].🤖 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 `@rules/oci/private/go_web.bzl` around lines 75 - 77, Validate that binaries is non-empty before the fallback in the entrypoint assignment accesses binaries[0], or configure the corresponding attribute with allow_empty = False so an explicitly empty list is rejected while preserving the existing entrypoint behavior.
🤖 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 @.github/workflows/bazel.yml:
- Around line 150-151: Update the workflow’s affected-target scheduling logic so
root dependency or shared Bazel rule changes also schedule the go-root nvcf-ui
row, rather than limiting the java_shared_changed path to java-* rows. Preserve
the existing nvcf-ui build-container configuration and ensure its build and
tests run for shared-root changes.
---
Outside diff comments:
In `@rules/oci/private/go_web.bzl`:
- Around line 75-77: Validate that binaries is non-empty before the fallback in
the entrypoint assignment accesses binaries[0], or configure the corresponding
attribute with allow_empty = False so an explicitly empty list is rejected while
preserving the existing entrypoint behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0bd0103c-f1c3-47c1-921a-976ee2cee2a8
⛔ Files ignored due to path filters (3)
MODULE.bazel.lockis excluded by!**/*.lock,!**/MODULE.bazel.locksrc/uis/nvcf-ui/backend/go.sumis excluded by!**/*.sumtools/collect-dependencies/go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
.bazelignore.github/workflows/bazel.ymlMODULE.bazelNOTICEdependencies.mdgo.work.bazelrules/oci/defs.bzlrules/oci/private/go_web.bzlsrc/uis/nvcf-ui/BUILD.bazelsrc/uis/nvcf-ui/backend/BUILD.bazelsrc/uis/nvcf-ui/backend/cmd/control-plane/BUILD.bazelsrc/uis/nvcf-ui/backend/cmd/server/BUILD.bazelsrc/uis/nvcf-ui/backend/go.modsrc/uis/nvcf-ui/backend/internal/control-plane/BUILD.bazelsrc/uis/nvcf-ui/backend/internal/middleware/BUILD.bazelsrc/uis/nvcf-ui/backend/internal/token-watcher/BUILD.bazelsrc/uis/nvcf-ui/backend/internal/utils/BUILD.bazelsrc/uis/nvcf-ui/ui/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (11)
- src/uis/nvcf-ui/backend/go.mod
- .bazelignore
- NOTICE
- src/uis/nvcf-ui/backend/cmd/server/BUILD.bazel
- src/uis/nvcf-ui/backend/BUILD.bazel
- src/uis/nvcf-ui/ui/BUILD.bazel
- src/uis/nvcf-ui/backend/internal/utils/BUILD.bazel
- src/uis/nvcf-ui/backend/internal/control-plane/BUILD.bazel
- src/uis/nvcf-ui/BUILD.bazel
- src/uis/nvcf-ui/backend/cmd/control-plane/BUILD.bazel
- src/uis/nvcf-ui/backend/internal/token-watcher/BUILD.bazel
| image-credential-helper|src/compute-plane-services/image-credential-helper|false|go-root|build-container | ||
| nvcf-ui|src/uis/nvcf-ui|false|go-root|build-container' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Schedule nvcf-ui for root-owned dependency changes.
nvcf-ui is a go-root row, so the root row excludes it at Line 327-332. However, MODULE.bazel, go.work.bazel, and shared Bazel rules trigger the root row. The java_shared_changed branch schedules only java-* rows. A change to a root dependency or rule can therefore skip the UI build and tests.
Add a shared-root trigger for affected go-root rows, or include nvcf-ui in the corresponding validation.
🤖 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 @.github/workflows/bazel.yml around lines 150 - 151, Update the workflow’s
affected-target scheduling logic so root dependency or shared Bazel rule changes
also schedule the go-root nvcf-ui row, rather than limiting the
java_shared_changed path to java-* rows. Preserve the existing nvcf-ui
build-container configuration and ensure its build and tests run for shared-root
changes.
`bazel (root)` failed while `bazel (nvcf-ui)` passed. Adding the backend to go.work.bazel changes dependency resolution for every Go module in the repository, not just this subtree, and MVS took k8s.io/apiextensions-apiserver from v0.34.1 to v0.36.0 because this module still required k8s.io at v0.36.0. A version-specific replace pins client-go to v0.34.2, so the v0.36 apiextensions code referenced symbols that version does not have. Pins k8s.io/api, apimachinery and client-go to v0.34.2 and apiextensions-apiserver to v0.34.1, matching the rest of the repository. Applied with `go mod edit` and deliberately no `go mod tidy`: tidy re-resolves these upward from the transitive graph, which is what silently undid the equivalent fix for controller-runtime earlier. Verified with a full `bazel build //...` rather than the subtree alone. The subtree-only build is what let this through: it cannot observe a change in workspace-wide version resolution. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
TestLoadConfigAuthCredentials failed in CI on this branch while passing on
main, and the difference was not the code. On main the test is a remote-cache
hit on every run and has never actually executed in CI. Changing
go.work.bazel here invalidated the cache key, so this is its first real run.
The test is not hermetic. state.DefaultStateManager is constructed at package
init, capturing $HOME before any test can change it, so the test's
t.Setenv("HOME", t.TempDir()) does not move the state path. LoadConfig falls
back to that state file for credentials, so a stray ~/.nvcf-cli.state supplies
an API key: "nothing set" then finds credentials, and "complete oauth2 setup"
resolves to bearer instead of oauth2. CI runs tests with HOME=/tmp, which is
shared and writable.
Adds state.ResetDefaultStateManager so a test can rebuild the manager after
pointing HOME somewhere private, and calls it at both sites that override HOME.
Production behaviour is unchanged: a CLI process's home directory does not
change after start, and nothing outside tests calls it.
Reproduced before fixing by seeding $HOME/.nvcf-cli.state with an apiKey and
running against main, which fails exactly as CI did; the same command passes
with this change. Full suite: 21 of 21 pass with --nocache_test_results.
Worth noting beyond this test: cache hits can mask tests that would fail if
they ran. A periodic uncached run on main would surface others like it.
Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
|
@kristinapathak Will we need to update the root SECURITY.md and .security-triage.yaml as well? |
3 participants
Why
Closes #634. The self-hosted NVCF UI is developed outside this repository today.
Now that every first-party service builds from the root Bazel module here, the
UI should live alongside them so it shares one build, one CI matrix, one
dependency audit, and one release path.
Source:
ngc/apps/nvcf-uiat2c6b0b711b46c964cd4b3a119c5dd467540efbfc.What changed
Imports 267 of the upstream's 271 tracked files into
src/uis/nvcf-ui, groupedby artifact kind alongside
src/clis/nvcf-clirather than under control-planeservices, since the UI is a shipped user-facing artifact and not a
control-plane service. Layout is
ui/(React/TypeScript, pnpm),backend/(Go),
helm/, andspec/.Excluded from the import:
.gitlab-ci.ymlnode_modules/.vite/.../results.json.vscode/Rewrites bare uppercase
SISto describe the cluster API in prose. All fiveoccurrences were comments or documentation, so nothing structural changed. The
OSS snapshot tooling has historically dropped entire files containing those bare
tokens, which would have silently removed
backend/cmd/server/main.gofrom thepublic mirror. Worth noting for the reviewer:
.github/CODEOWNERSonmainalready contains
SIStoday, so that gate may no longer be active on this path.The rewrite is cheap and reads fine either way, but I did not want to overstate
it as a confirmed near-miss.
Adds
/src/uis/ @NVIDIA/nvcf-control-plane-devto CODEOWNERS as requested inreview. Note the team slug:
nvcf-control-plane-services-devreturns 404 fromthe org API, and
nvcf-control-plane-devis what every other entry uses, so Iwent with the one that exists. A slug that does not resolve silently disables
the code-owner gate rather than erroring, so it is worth confirming this is the
intended team.
Customer Release Notes
Not customer visible. This moves source only; nothing is built or published
differently yet.
Plan Summary
Not applicable.
Usage
Not applicable.
Testing
tools/ci/check-licensepasses: 291 files checked, MPL audit in sync, NOTICE upto date after regenerating it to absorb the UI's own NOTICE. All 182 imported
.ts/.tsx/.gosource files already carry SPDX headers, confirmed by countrather than by sampling.
No Bazel build of the subtree runs in this PR, by design. See below.
Notes
The subtree keeps the upstream
MODULE.bazelfor now and is listed in the root.bazelignore, so the root module does not try to load packages whose labelsare still relative to the upstream module root.
This is deliberately temporary and runs against the recent consolidation of all
17 nested modules into the root. Merging the JS rules into the root
MODULE.bazelmeans rewriting every label in the UI's BUILD files(
//ui:...becomes//src/uis/nvcf-ui/ui:...) and validating with a realbuild that needs Node and pnpm. That is its own reviewable change, and doing it
here would bury a 267-file import under a build migration.
Good news relative to the issue text: #634 assumed no JavaScript ruleset existed
anywhere. The UI already builds under Bazel with
aspect_rules_js3.2.2 andrules_nodejs6.7.3, and already shipsLICENSE,NOTICE,THIRD-PARTY.txt,and
license-header.txt. ItsMODULE.bazeldeps are close to a superset of theroot module's, so the remaining work is a merge rather than a design exercise.
Follow-ups, tracked in #634:
MODULE.bazel, rewrite BUILD labels, dropthe
.bazelignoreentry, and add abazel.ymlmatrix rowtools/collect-dependencies. It has collectors forGo, Helm, Java, Python, and Rust but none for npm, so until it exists the
UI's third-party tree does not reach
dependencies.mdhelm/stays in the subtree or moves todeploy/helm/nvcf-uiReferences
Closes #634
Related Merge Requests/Pull Requests
None
Dependencies
No new dependencies are added to the root module by this PR. The UI's own
third-party tree arrives with the import and is enumerated in its
THIRD-PARTY.txtandNOTICE; wiring it into this repository'sdependencies.mdis blocked on the npm collector above and should gate thedependency review.
Github commit:
feat(nvcf-ui): import the self-hosted UI into the monorepo
Closes #634
Co-authored-by: Balaji Ganesan bganesan@nvidia.com
Summary by CodeRabbit