🐛 Remove kind=asset from the Credentials pages #2611
Conversation
WalkthroughRemoves the "asset" identity kind and related UI/form logic. Introduces role-annotated references in models and updates Application.identities typing. Refactors identities page to compute usage metadata, manage default identity assignment/unset, integrate modal-based create/edit, and add notifications. Tests/imports are adjusted; minor import cleanups elsewhere. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Identities Page
participant API as Backend API
note over UI: Compute identity meta (usage, defaults)
User->>UI: Click "Set as default" on identity (kind K)
UI->>API: GET identities (current defaults for kind K)
alt Existing default for K
UI->>API: PATCH existing default (unset default)
end
UI->>API: PATCH selected identity (set default)
API-->>UI: 200 OK
UI-->>User: Show success notification and refresh list
sequenceDiagram
autonumber
actor User
participant UI as Identities Page
participant Modal as IdentityFormModal
participant API as Backend API
User->>UI: Click Create/Edit identity
UI->>Modal: Open with form state
Modal->>Modal: Validate (yupResolver, kind≠asset)
alt Submit valid
Modal->>API: POST/PUT identity payload (source/bearer/...)
API-->>Modal: 200/201 OK
Modal-->>UI: Close with success
UI-->>User: Show success notification and refresh
else Validation/API error
Modal-->>User: Show inline errors / error toast
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sjd78
force-pushed
the
drop_asset_kind
branch
3 times, most recently
from
a7891a1 to
08ae536
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
client/src/app/pages/identities/identities.tsx (2)
481-496: Fix crash when applications data is still loading.applications can be undefined; calling applications.filter will throw. Default it to [].
- const { data: applications } = useFetchApplications(); + const { data: applications = [] } = useFetchApplications();
270-273: Use a stable unique key for rows.Using name risks key collisions and incorrect row updates; use id.
- <Tr key={identity.name} {...getTrProps({ item: identity })}> + <Tr key={identity.id} {...getTrProps({ item: identity })}>
🧹 Nitpick comments (17)
client/src/app/api/models.ts (3)
142-144: IdentityRole union collapses to plain string
ManagedIdentityRole | stringwidens to juststring, soIdentityRoleprovides no extra type-safety overstring. Consider either:
- Dropping
IdentityRoleand usingstringdirectly, or- Branding/customizing non-managed roles (e.g.,
ManagedIdentityRole | \custom:${string}``) to keep literals narrow, or- Keep
IdentityRolefor semantics but acknowledge it's only documentation.-export type IdentityRole = ManagedIdentityRole | string; +// Option A (semantics only) +export type IdentityRole = string; +// Option B (brand custom roles to keep managed literals narrow) +// export type IdentityRole = ManagedIdentityRole | `custom:${string}`;
215-221: IdentityKinds drops "asset": add assertion tests to prevent regressionsNow that
"asset"is removed, consider a unit/integration test that the Identity Type select does not show Asset. See suggested test diff in identity-form.test.tsx comment.
142-144: Clarify Managed vs. UI-removed "asset"Keeping
"asset"inManagedIdentityRolewhile removing kind=asset from Credentials UI is fine (role ≠ kind), but add a brief comment to avoid confusion.-export type ManagedIdentityRole = "source" | "maven" | "asset"; +// Reserved roles used on Application.identities (UI may not expose corresponding kinds) +export type ManagedIdentityRole = "source" | "maven" | "asset";client/src/app/pages/identities/components/identity-form/kind-simple-username-password-form.tsx (1)
19-30: Prefer useWatch to avoid stale reads of passwordUsing
getValues()once can makeisPasswordEncryptedstale during edits. Track the field withuseWatchfor consistent re-rendering and toggle visibility behavior.-import { useFormContext } from "react-hook-form"; +import { useFormContext, useWatch } from "react-hook-form"; @@ - const { control, getValues, resetField } = useFormContext(); - const values = getValues(); + const { control, resetField } = useFormContext(); + const password = useWatch({ control, name: "password" }); @@ - const isPasswordEncrypted = identity?.password === values.password; + const isPasswordEncrypted = identity?.password === password;Also applies to: 46-57
client/src/app/pages/identities/components/DefaultLabel.tsx (1)
10-16: Remove unused DefaultLabelProps or use it for the component
DefaultLabelPropsis declared but not used. Either use it in the FC type annotation or drop it to avoid drift.-export interface DefaultLabelProps { - identity: Identity; -} - -export const DefaultLabel: React.FC<{ identity: Identity }> = ({ +export interface DefaultLabelProps { + identity: Identity; +} + +export const DefaultLabel: React.FC<DefaultLabelProps> = ({ identity, }) => {client/src/app/pages/identities/components/identity-form/kind-maven-settings-file-form.tsx (2)
15-22: Avoid stale values: watch only the fields you needSwap
getValues()foruseWatchon["kind","settings","settingsFilename","default"]to drive re-renders only when relevant fields change.-import { useFormContext } from "react-hook-form"; +import { useFormContext, useWatch } from "react-hook-form"; @@ - const { control, getValues, setValue } = useFormContext(); - const values = getValues(); + const { control, setValue } = useFormContext(); + const values = useWatch({ control, name: ["kind", "settings", "settingsFilename", "default"] }) as any;
79-83: Accept both text/xml and application/xml for settings uploadsSome environments produce
application/xml. Expandacceptto reduce false rejections.- accept: { "text/xml": [".xml"] }, + accept: { "text/xml": [".xml"], "application/xml": [".xml"] },client/src/app/pages/identities/components/identity-form/kind-bearer-token-form.tsx (1)
11-13: Watch the key field to keep encryption sentinel accurateMirror the source/simple forms by watching
keyinstead ofgetValues()to avoid staleisKeyEncryptedand ensure the eye toggle shows/hides correctly.-import { useFormContext } from "react-hook-form"; +import { useFormContext, useWatch } from "react-hook-form"; @@ - const { control, getValues, resetField } = useFormContext(); - const values = getValues(); + const { control, resetField } = useFormContext(); + const keyValue = useWatch({ control, name: "key" }); @@ - const isKeyEncrypted = identity?.key === values.key; + const isKeyEncrypted = identity?.key === keyValue;Also applies to: 21-41
client/src/app/pages/identities/components/identity-form/kind-source-form.tsx (1)
2-3: Limit useWatch subscriptions to needed fields
useWatch({ control })subscribes to the entire form and can cause unnecessary re-renders. Watch only fields used in this component.- const { control, setValue, resetField } = useFormContext(); - const values = useWatch({ control }); + const { control, setValue, resetField } = useFormContext(); + const values = useWatch({ + control, + name: ["userCredentials", "password", "key", "kind", "default", "keyFilename"], + }) as any;client/src/app/pages/identities/components/identity-form/__tests__/identity-form.test.tsx (2)
145-162: Trim excessive waitFor timeouts for snappier testsThe 3000ms timeouts aren’t necessary for synchronous input changes and slow down the suite. Consider removing custom timeouts and relying on default.
- await waitFor( - () => { + await waitFor(() => { fireEvent.change(nameInput, { target: { value: "identity-name" }, }); @@ - }, - { - timeout: 3000, - } - ); + });Also applies to: 217-226
34-38: Add a regression test to ensure Asset is not offeredGuard against reintroducing kind=asset to the Type select.
@@ it("Display form on initial load", async () => { @@ }); + + it("Type list must not include Asset", async () => { + render(<IdentityForm onClose={mockChangeValue} />); + const typeSelector = await screen.findByLabelText("Type select dropdown toggle"); + fireEvent.click(typeSelector); + expect(screen.queryByText(/Asset/i)).not.toBeInTheDocument(); + });client/src/app/pages/identities/identities.tsx (4)
223-227: Correct the table aria-label.This is a credentials table, not “Business service”.
- aria-label="Business service table" + aria-label="Credentials table"
213-219: Align pagination idPrefix with the page.Minor naming nit to avoid confusion in tests and DOM ids.
- idPrefix="business-service-table" + idPrefix="identities-table"Also applies to: 378-382
488-514: Memoize identityMeta to avoid recomputing on every render.Cheap now, but this walks multiple collections each render. Memoize on dependencies.
- const identityMeta = objectify( - identities, - ({ id }) => id, - ({ id, default: isDefault, kind }) => { + const identityMeta = useMemo( + () => + objectify( + identities, + ({ id }) => id, + ({ id, default: isDefault, kind }) => { const someTrackers = trackers.filter((t) => t.identity?.id === id).length; - const someApplications = applications.filter((a) => + const someApplications = applications.filter((a) => a.identities?.some((i) => i.id === id) ).length; const someTargets = targets.filter( (t) => t.ruleset?.identity?.id === id ).length; const okToSetAsDefault = !isDefault && ["source", "maven"].includes(kind); const okToRemoveDefault = isDefault && ["source", "maven"].includes(kind); return { id, someTrackers, someApplications, someTargets, inUse: someTrackers > 0 || someApplications > 0 || someTargets > 0, okToSetAsDefault, okToRemoveDefault, }; - } - ); + } + ), + [identities, trackers, targets, applications] + );
335-343: Follow-up on TODO for default assignment confirmation.Setting a new default may trigger discovery runs; if that’s impactful, consider a confirmation even when no existing default for the kind is set. I can wire a lightweight confirm using your Notifications/ConfirmDialog patterns.
client/src/app/pages/identities/components/identity-form/identity-form.tsx (2)
225-229: Silence Biome’s false-positive onthenin Yup.when()clauses.Biome flags objects with a
thenkey; here it’s required by Yup. Add an inline ignore before the offending properties.userCredentials: yup.mixed<UserCredentials>().when("kind", { is: (kind: IdentityKind) => kind === "source", + /* biome-ignore lint/suspicious/noThenProperty: Yup's when() options intentionally use a 'then' property */ then: (schema) => schema.required().oneOf(["userpass", "source"]), }), ... .when("kind", { is: (kind: IdentityKind) => kind === "basic-auth", + /* biome-ignore lint/suspicious/noThenProperty: Yup's when() options intentionally use a 'then' property */ then: (schema) => schema .required(t("validation.required")) .min(3, t("validation.minLength", { length: 3 })) .max(120, t("validation.maxLength", { length: 120 })) .email("Username must be a valid email"), }) .when(["kind", "userCredentials"], { is: (kind: IdentityKind, userCredentials: UserCredentials) => kind === "source" && userCredentials === "userpass", + /* biome-ignore lint/suspicious/noThenProperty: Yup's when() options intentionally use a 'then' property */ then: (schema) => schema .required(t("validation.required")) .min(3, t("validation.minLength", { length: 3 })) .max(120, t("validation.maxLength", { length: 120 })), }), ... .when("kind", { is: (kind: IdentityKind) => kind === "basic-auth", + /* biome-ignore lint/suspicious/noThenProperty: Yup's when() options intentionally use a 'then' property */ then: (schema) => schema .required(t("validation.required")) .min(3, t("validation.minLength", { length: 3 })) .max(281, t("validation.maxLength", { length: 281 })), }) .when(["kind", "userCredentials"], { is: (kind: IdentityKind, userCredentials: UserCredentials) => kind === "source" && userCredentials === "userpass", + /* biome-ignore lint/suspicious/noThenProperty: Yup's when() options intentionally use a 'then' property */ then: (schema) => schema .required(t("validation.required")) .min(3, t("validation.minLength", { length: 3 })) .max(120, t("validation.maxLength", { length: 120 })), }), ... .when(["kind", "userCredentials"], { is: (kind: IdentityKind, userCredentials: UserCredentials) => kind === "source" && userCredentials === "source", // If we want to verify key contents before saving, add the yup test in the then function + /* biome-ignore lint/suspicious/noThenProperty: Yup's when() options intentionally use a 'then' property */ then: (schema) => schema.required(t("validation.required")), }) .when("kind", { is: (kind: IdentityKind) => kind === "bearer", + /* biome-ignore lint/suspicious/noThenProperty: Yup's when() options intentionally use a 'then' property */ then: (schema) => schema.required(t("validation.required")), }),Also applies to: 234-241, 249-265, 266-273, 294-302, 307-316
419-425: “Jira username or email” vs email-only validation.Label allows username or email, but validation enforces email. If non-email usernames are valid, drop
.email(...); otherwise adjust label.Also applies to: 244-264
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
client/src/app/api/models.ts(3 hunks)client/src/app/pages/applications/application-identity-form/application-identity-form.tsx(7 hunks)client/src/app/pages/applications/application-identity-form/application-identity-modal.tsx(1 hunks)client/src/app/pages/identities/components/DefaultLabel.tsx(1 hunks)client/src/app/pages/identities/components/identity-form/__tests__/identity-form.test.tsx(1 hunks)client/src/app/pages/identities/components/identity-form/identity-form.tsx(6 hunks)client/src/app/pages/identities/components/identity-form/kind-asset-form.tsx(0 hunks)client/src/app/pages/identities/components/identity-form/kind-bearer-token-form.tsx(1 hunks)client/src/app/pages/identities/components/identity-form/kind-maven-settings-file-form.tsx(1 hunks)client/src/app/pages/identities/components/identity-form/kind-simple-username-password-form.tsx(1 hunks)client/src/app/pages/identities/components/identity-form/kind-source-form.tsx(1 hunks)client/src/app/pages/identities/identities.tsx(2 hunks)client/src/app/pages/identities/utils.ts(1 hunks)
💤 Files with no reviewable changes (1)
- client/src/app/pages/identities/components/identity-form/kind-asset-form.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-29T16:01:04.207Z
Learnt from: sjd78
PR: konveyor/tackle2-ui#2583
File: client/src/app/pages/identities/components/identity-form/kind-source-form.tsx:17-26
Timestamp: 2025-08-29T16:01:04.207Z
Learning: Skip i18n/internationalization suggestions for the file client/src/app/pages/identities/components/identity-form/kind-source-form.tsx - hardcoded strings like "Username/Password" and "Private Key/Passphrase" should remain as-is.
Applied to files:
client/src/app/pages/identities/components/identity-form/kind-maven-settings-file-form.tsxclient/src/app/pages/identities/components/DefaultLabel.tsxclient/src/app/pages/identities/components/identity-form/kind-bearer-token-form.tsxclient/src/app/pages/identities/components/identity-form/kind-source-form.tsxclient/src/app/pages/identities/components/identity-form/kind-simple-username-password-form.tsxclient/src/app/pages/identities/utils.tsclient/src/app/pages/applications/application-identity-form/application-identity-form.tsxclient/src/app/pages/identities/components/identity-form/identity-form.tsx
📚 Learning: 2025-09-12T18:54:17.513Z
Learnt from: sjd78
PR: konveyor/tackle2-ui#2610
File: client/src/app/pages/applications/application-identity-form/application-identity-form.tsx:146-151
Timestamp: 2025-09-12T18:54:17.513Z
Learning: In ApplicationIdentityForm multi-edit mode, the form state is intentionally applied as-is across all selected applications. This means that even untouched form fields will overwrite existing values across all applications, which is the expected bulk-edit behavior for standardizing identity credentials across multiple applications.
Applied to files:
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx
🧬 Code graph analysis (2)
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx (5)
client/src/app/api/models.ts (5)
IdentityRole(143-143)Identity(225-239)ManagedIdentityRole(142-142)RefWithRole(93-95)Application(145-170)client/src/app/utils/model-utils.tsx (1)
toRef(238-244)client/src/app/pages/applications/useDecoratedApplications.ts (1)
DecoratedApplication(39-69)client/src/app/components/NotificationsContext.tsx (1)
NotificationsContext(31-33)client/src/app/queries/identities.ts (1)
useFetchIdentities(58-78)
client/src/app/pages/identities/components/identity-form/identity-form.tsx (1)
client/src/app/api/models.ts (1)
IdentityKind(223-223)
🪛 Biome (2.1.2)
client/src/app/pages/identities/components/identity-form/identity-form.tsx
[error] 268-268: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit-test
- GitHub Check: build-and-upload-for-global-ci
🔇 Additional comments (9)
client/src/app/api/models.ts (1)
93-95: RefWithRole addition and Application.identities type change: verify downstream usage and API contractChanging
Application.identitiesfromRef[]toRefWithRole<IdentityRole>[]is source-compatible for reads, but writers now may attachrole. Please verify:
- Call sites that spread/serialize
Applicationinclude/expectrolewhere needed.- Hub OpenAPI (main/docs/openapi3.json) reflects
roleon identity refs forApplication.Run to find potential assumptions on identities shape and asset references:
#!/bin/bash # Review reads/writes of Application.identities rg -nP -C2 '\bApplication\b.*identit' --type ts --type tsx rg -nP -C2 '\.identities\b' --type ts --type tsx # Any lingering checks for kind=asset rg -nP -C2 'kind\s*[:=]\s*["\']asset["\']|\b"asset"\b' --type ts --type tsx # Places assuming Ref (no role) in identities rg -nP -C2 '\b{id:\s*\w+,\s*name:\s*\w+}' --type ts --type tsxAlso applies to: 156-156
client/src/app/pages/identities/components/DefaultLabel.tsx (1)
24-27: Verify KIND_VALUES aligns with IdentityKinds changesEnsure
KIND_VALUESno longer includes"asset"so the tooltip reflects only supported kinds.#!/bin/bash rg -nP -C1 'KIND_VALUES' client/src/app/pages/identities -g '!**/__tests__/**' rg -nP '"asset"' client/src/app/pages/identitiesclient/src/app/pages/applications/application-identity-form/application-identity-modal.tsx (1)
6-7: LGTM: import order tweak onlyNo functional changes; modal composition remains intact.
client/src/app/pages/identities/components/identity-form/kind-source-form.tsx (1)
63-82: Field reset on credential type change: confirm this UXResetting
default,user,key, andpasswordwhen switching credential type is reasonable, but it’s destructive. Confirm this is intended, especially for accidental toggles.client/src/app/pages/identities/components/identity-form/__tests__/identity-form.test.tsx (1)
2-5: MSW setup: ensure global lifecycle handles server start/resetYou’re using
server.useper test, which is fine if globalsetupTestscallsserver.listen()and resets/close in hooks. Confirm that’s present to avoid cross-test leakage.#!/bin/bash # Look for MSW server lifecycle in test config fd -a 'setupTests' --extension js --extension ts --extension tsx rg -n 'server\.listen|server\.resetHandlers|server\.close' --glob '**/*setup*.*'Also applies to: 16-22
client/src/app/pages/identities/utils.ts (1)
3-5: LGTM: Asset kind cleanly removed from identity utils.Constants, options, and defaults align with the reduced IdentityKind set. No issues.
Also applies to: 6-12, 22-35
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx (2)
100-105: Asset role pulling from source-kind identities — looks correct given kind removal.Deriving asset role options from identitiesByKind.source matches the shift to role-based usage while removing the asset kind.
147-166: Confirm intended bulk-edit semantics for managed roles.Form produces only managed roles and drops prior managed roles when fields are cleared (null). This matches the “apply-as-is across all selected apps” behavior from #2610; just confirming that’s desired here too.
Also applies to: 168-175
client/src/app/pages/identities/components/identity-form/identity-form.tsx (1)
70-82: Scope userCredentials to source only — good.Limiting credentials mode to kind === "source" aligns with removing asset-specific branches.
Part 2 of 2: https://issues.redhat.com/browse/MTA-6122 Part 2 of 2: konveyor#2599 Follows: konveyor#2610 Signed-off-by: Scott J Dickerson <[email protected]>
sjd78
force-pushed
the
drop_asset_kind
branch
from
08ae536 to
1128cf5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/src/app/pages/identities/identities.tsx (2)
2-2: Consider centralizing error handling and notification patterns.The error handling pattern with
AxiosErrorandgetAxiosErrorMessageis repeated across multiple mutation callbacks. This could be simplified with a shared error handler utility.Consider extracting a reusable mutation handler factory:
+ const useMutationWithNotification = <T extends any[], R>( + mutation: (...args: T) => R, + successMessage: (result: any) => string, + errorHandler?: (error: AxiosError) => void + ) => { + const { pushNotification } = React.useContext(NotificationsContext); + const defaultErrorHandler = (error: AxiosError) => { + pushNotification({ + title: getAxiosErrorMessage(error), + variant: "danger", + }); + }; + + return mutation( + (result) => { + pushNotification({ + title: successMessage(result), + variant: "success", + }); + }, + errorHandler || defaultErrorHandler + ); + };Also applies to: 478-478, 493-494, 503-503, 556-559, 576-583, 586-599
588-600: Potential race condition indoAssignDefaultIdentity.The async operation could fail between unsetting the existing default and setting the new one, leaving no default identity. Consider wrapping both operations in a transaction or recovering from partial failures.
Consider adding error recovery:
const doAssignDefaultIdentity = async (newDefault: Identity) => { try { // Unset the existing default if necessary const existingDefault = defaultIdentities[newDefault.kind]; if (existingDefault && existingDefault.id !== newDefault.id) { - await updateIdentityAsync({ ...existingDefault, default: false }); + try { + await updateIdentityAsync({ ...existingDefault, default: false }); + } catch (e) { + // Log but continue - we still want to set the new default + console.error("Failed to unset existing default:", e); + } } updateIdentity({ ...newDefault, default: true }); } catch (e) { onMutationError(e as AxiosError); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
client/src/app/api/models.ts(0 hunks)client/src/app/pages/identities/components/DefaultLabel.tsx(1 hunks)client/src/app/pages/identities/components/identity-form/__tests__/identity-form.test.tsx(1 hunks)client/src/app/pages/identities/components/identity-form/identity-form.tsx(6 hunks)client/src/app/pages/identities/components/identity-form/kind-asset-form.tsx(0 hunks)client/src/app/pages/identities/components/identity-form/kind-bearer-token-form.tsx(1 hunks)client/src/app/pages/identities/components/identity-form/kind-maven-settings-file-form.tsx(1 hunks)client/src/app/pages/identities/components/identity-form/kind-simple-username-password-form.tsx(1 hunks)client/src/app/pages/identities/components/identity-form/kind-source-form.tsx(1 hunks)client/src/app/pages/identities/identities.tsx(2 hunks)client/src/app/pages/identities/utils.ts(1 hunks)
💤 Files with no reviewable changes (2)
- client/src/app/pages/identities/components/identity-form/kind-asset-form.tsx
- client/src/app/api/models.ts
✅ Files skipped from review due to trivial changes (1)
- client/src/app/pages/identities/components/identity-form/kind-bearer-token-form.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- client/src/app/pages/identities/components/identity-form/kind-simple-username-password-form.tsx
- client/src/app/pages/identities/components/DefaultLabel.tsx
- client/src/app/pages/identities/utils.ts
- client/src/app/pages/identities/components/identity-form/kind-maven-settings-file-form.tsx
- client/src/app/pages/identities/components/identity-form/kind-source-form.tsx
- client/src/app/pages/identities/components/identity-form/tests/identity-form.test.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T16:01:04.207Z
Learnt from: sjd78
PR: konveyor/tackle2-ui#2583
File: client/src/app/pages/identities/components/identity-form/kind-source-form.tsx:17-26
Timestamp: 2025-08-29T16:01:04.207Z
Learning: Skip i18n/internationalization suggestions for the file client/src/app/pages/identities/components/identity-form/kind-source-form.tsx - hardcoded strings like "Username/Password" and "Private Key/Passphrase" should remain as-is.
Applied to files:
client/src/app/pages/identities/components/identity-form/identity-form.tsx
🧬 Code graph analysis (1)
client/src/app/pages/identities/components/identity-form/identity-form.tsx (1)
client/src/app/api/models.ts (1)
IdentityKind(223-223)
🪛 Biome (2.1.2)
client/src/app/pages/identities/components/identity-form/identity-form.tsx
[error] 268-268: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit-test
- GitHub Check: build-and-upload-for-global-ci
🔇 Additional comments (6)
client/src/app/pages/identities/identities.tsx (2)
488-513: Good computation of identity metadata.The use of
objectifyfrom radash provides an efficient way to create the identity lookup map. The metadata computation correctly aggregates usage across trackers, applications, and targets to determine whether an identity is safe to delete or modify.
336-342: Inconsistent confirmation behavior for setting default identity.When there's an existing default identity, the user is prompted for confirmation (line 336), but when there isn't one, the operation proceeds without confirmation (line 341). The comment on lines 338-340 suggests this might need confirmation as it triggers tech-discovery and language-discovery.
Should setting a default identity always require confirmation when it triggers discovery operations? This inconsistency might confuse users.
client/src/app/pages/identities/components/identity-form/identity-form.tsx (4)
175-194: Race condition handling in form submission.Similar to the identities page, there's a potential race condition between unsetting the existing default and creating/updating the new identity. However, the error handling here is better structured with try/catch.
70-82: Clean refactoring of credential type detection.The removal of asset-related logic and simplification to only handle "source" kind credentials makes the code cleaner and more maintainable.
264-273: Validation improvements for source credentials.The addition of validation for source credentials with userpass ensures proper data integrity. The validation rules correctly enforce minimum and maximum length constraints.
265-273: Fixthenproperty issue in yup validation schema.The static analysis correctly identifies that using
thenas an object property could cause issues. This is a yup schema method, not a property assignment.The validation schema syntax is correct - this is how yup's conditional validation works with the
whenmethod. The static analysis warning appears to be a false positive. However, if you're seeing any runtime issues, ensure you're using a compatible version of yup.
Part 2 of 2: https://issues.redhat.com/browse/MTA-6122 Part 2 of 2: konveyor#2599 Remove display and CRUD support for `kind=asset` identities from the Credentials pages. konveyor#2610 adds the use of a `role` on the `application.identities` Ref[] to indicate how each reference identity should be used. Signed-off-by: Scott J Dickerson <[email protected]>
2 participants
Part 2 of 2: https://issues.redhat.com/browse/MTA-6122
Part 2 of 2: #2599
Remove display and CRUD support for
kind=assetidentities from the Credentials pages. #2610 adds the use of aroleon theapplication.identitiesRef[] to indicate how each reference identity should be used.Summary by CodeRabbit
New Features
Refactor
Tests
Style