Conversation
Summary
Testing
Tests: 448 passed (85 files). Lint: no errors. |
|
Implementation complete for #1393. All local checks pass:
Requesting review. |
noa-lucent
left a comment
There was a problem hiding this comment.
Clean, well-structured PR that correctly adds Secret Provider and Secret entity kinds across server and UI. The changes follow existing patterns consistently, route paths avoid collision with the existing Vault-based secrets feature, and no existing secrets code was touched.
Two minor comments on using the templateViewFor()/templateViewComponent() fallback pattern for consistency with all other entity kinds in the config view switches. Not blocking — no template overrides exist today for these kinds — but worth aligning.
Nicely done.
packages/platform-ui/src/components/entities/EntityUpsertForm.tsx
Outdated
Show resolved
Hide resolved
Summary
Lint
Typecheck
Tests
|
Rework Complete — Ready for ReviewThis PR has been reworked to use gateway-backed CRUD instead of graph/node integration. What changed:
CI: All 7 checks passing ✅Lint | Test Server | Test UI | Storybook Smoke | LiteLLM Integration | Build Server | Build UI Requesting review. |
noa-lucent
left a comment
There was a problem hiding this comment.
Thanks for the rework — the graph/node approach is fully removed and the gateway-backed CRUD pattern is the right direction. The API modules have proper boundary parsing, hooks follow the Variables pattern, types match the issue spec, and the routing/nav additions are correct.
However, there are several structural issues that need addressing before merge:
Major:
- Duplicated code across new files —
extractErrorCode(3 copies), boundary parsing helpers (isRecord,readString, etc. — 2 copies),computePaginationWindow+formatTimestamp+ pagination constants (2 copies each). These should be extracted into shared utilities. - Monolithic page components (524 + 640 lines) — the page components each handle pagination, form state, validation, submit, delete confirmation, and table rendering. Extract reusable pieces: form hooks,
<PaginationBar>,<ActionIconButton>(tooltip-wrapped icon button used 5× across both pages). - No hook tests — the reference pattern (
variables/hooks.ts) has a full test suite, but no tests were added for the new hooks.
Minor:
4. Magic perPage: 100 for provider dropdown in EntitySecretsListPage.
Previous review threads from the graph/node version have been resolved as those files are no longer in scope.
packages/platform-ui/src/features/entitySecrets/hooks/useSecretProviders.ts
Outdated
Show resolved
Hide resolved
packages/platform-ui/src/features/entitySecrets/hooks/useEntitySecrets.ts
Outdated
Show resolved
Hide resolved
Summary
Testing
|
noa-lucent
left a comment
There was a problem hiding this comment.
All 6 prior review comments verified as resolved in code:
extractErrorCodeextraction ✓ — Shared in@/lib/extractErrorCode.ts, imported by all 3 hook files (including refactoredvariables/hooks.ts).- Parsing helpers extraction ✓ — Shared in
@/api/parsing.ts, imported by both API modules. No local copies remain. - Pagination/timestamp extraction ✓ —
@/lib/pagination.ts(withgetPaginationMeta,computePaginationWindow, and named constants) and@/lib/formatTimestamp.ts. Both pages import from shared. - Page decomposition ✓ — Both pages decomposed into focused subcomponents (
Header,Table,FormDialog,DeleteDialog,ResolveDialog) and custom form hooks (useProviderForm,useSecretForm). Shared<ActionIconButton>and<PaginationBar>components extracted. Orchestrator functions are clean and readable. - Hook tests ✓ — 11 new test cases covering query success/error, mutation success notifications + cache invalidation, error notifications via
extractErrorCode, and resolve error path. - Magic number ✓ —
PROVIDER_DROPDOWN_PAGE_SIZEnamed constant replaces the inline100.
Clean, well-structured refactor. Good to merge.
Summary
Testing
|
noa-lucent
left a comment
There was a problem hiding this comment.
Clean migration from offset-based to token-based pagination. All changes verified:
- API modules:
page/perPage/totalreplaced withpageSize/pageToken/nextPageToken.readNumberimport correctly removed. Boundary parsing updated. - Hooks:
useQuery→useInfiniteQuerywith fully typed generic parameters, properinitialPageParam, andgetNextPageParamusingnormalizePageToken. - Pages: Offset state (
page,safePage,getPaginationMeta) replaced withflatMapoverdata.pagesandfetchNextPage/hasNextPage/isFetchingNextPage. TheuseEffectfor page clamping correctly removed. - PaginationBar: Simplified to "Load more" pattern matching token-based pagination semantics.
pagination.ts: Offset utilities removed,normalizePageTokenadded.DEFAULT_PAGE_SIZEretained.- Tests: 2 new multi-page token pagination tests (providers + secrets) with
fetchNextPageandtoHaveBeenNthCalledWithassertions verifying correct token forwarding.
One nit on dead readNumber export. Good to merge.
Summary
Testing
Lint
|
noa-lucent
left a comment
There was a problem hiding this comment.
Clean migration from dialog-based forms to dedicated upsert pages. All changes verified:
- New
SecretProviderUpsertPageandEntitySecretUpsertPagefollow the existingEntityUpsertPagepattern: props-drivenmode,useParamsfor entity ID,useNavigatefor cancel/success, loading overlay, not-found alert, and inline submit error. - Routes follow the established
/:entity/newand/:entity/:entityId/editconvention. - List pages properly simplified: dialog components, form hooks, and create/update mutation hooks removed. Navigation handlers delegate to the new routes. Delete uses
window.confirm, consistent withEntityListPage. - Shared utils extracted to
features/entitySecrets/utils.ts(buildProviderLabel,PROVIDER_DROPDOWN_PAGE_SIZE), shared by both the upsert page and list page. ResolveSecretDialogcorrectly converted to inlineResolveSecretPanel(no dialog needed since it's read-only).- Provider filter preserved via
?providerId=query param when navigating to create secret — picked up byuseSearchParamsin the upsert page. - Provider options fallback in edit mode correctly handles the case where the selected provider isn't in the loaded dropdown page.
One minor: dead createDisabled prop chain in SecretProvidersListPage. Not blocking.
51be8b0 to
c691806
Compare
|
Summary:
Tests:
|
noa-lucent
left a comment
There was a problem hiding this comment.
Clean SelectInput → Dropdown (Radix Select) migration. All changes verified:
- 3 page components (
SecretProviderUpsertPage,EntitySecretUpsertPage,EntitySecretsListPage) andEntityUpsertFormcorrectly migrated fromonChangeevent handlers toonValueChangedirect-value API. - Radix deselection workaround: Sentinel constants (
ALL_PROVIDERS_VALUE,EMPTY_RELATION_VALUE) correctly handle Radix Select's no-deselect constraint — applied where deselection is needed (filter dropdown, relation dropdowns), correctly omitted where selection is required (provider field in secret upsert). - Test updates: Thorough — pointer capture polyfills for jsdom,
pointerEventsCheck: 0onuserEvent.setup(),findByLabelTextfor single-relation selects,data-state="checked"assertions, removedwithin(listbox)scoping (Radix portals). All CI green. - No leftover
SelectInputreferences in the codebase.
Previous minor (createDisabled = false in SecretProvidersListPage) resolved as non-blocking.
|
Summary:
Tests:
|
noa-lucent
left a comment
There was a problem hiding this comment.
Straightforward styling pass — size="sm" added to all Input and Dropdown fields on both upsert pages for consistent compact heights. Textarea correctly left at default since its height is row-driven. Both components accept size: 'sm' | 'default' in their type definitions. No logic changes, CI green. LGTM.
* feat(entities): add secret provider kinds * refactor(secrets): move to gateway CRUD * refactor(ui): extract secrets helpers * fix(secrets): use token pagination * feat(ui): add secret upsert pages * fix(ui): stabilize dropdown selections * fix(ui): align secret form sizes
* refactor(platform): drop docker-runner * feat: add secret provider entities (#1394) * feat(entities): add secret provider kinds * refactor(secrets): move to gateway CRUD * refactor(ui): extract secrets helpers * fix(secrets): use token pagination * feat(ui): add secret upsert pages * fix(ui): stabilize dropdown selections * fix(ui): align secret form sizes * test: centralize docker client stubs
* refactor(platform): drop docker-runner * feat: add secret provider entities (#1394) * feat(entities): add secret provider kinds * refactor(secrets): move to gateway CRUD * refactor(ui): extract secrets helpers * fix(secrets): use token pagination * feat(ui): add secret upsert pages * fix(ui): stabilize dropdown selections * fix(ui): align secret form sizes * test: centralize docker client stubs
Summary
Testing
Closes #1393