✨ Add a slot for "asset repository credentials" to the application manage credentials modal #2579
Conversation
WalkthroughAdds "asset" identity kind and related locale entries; refactors credentials UI to a modal and bulk patch flow using numeric identity IDs; replaces per-app bulk update with a patch-and-update mutation; updates REST typings; makes utility and form validation changes. No backend API surface changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Table as ApplicationsTable
participant Modal as ApplicationIdentityModal
participant Form as ApplicationIdentityForm
participant Q as useBulkPatchApplicationsMutation
participant API as REST.updateApplication
participant Server
participant Toast as Toastr
User->>Table: Open "Manage credentials"
Table->>Modal: render(applications: DecoratedApplication[])
Modal->>Form: mount (shows source/maven/asset id selects)
User->>Form: submit selections
Form->>Q: mutate({ applications, patchFn })
loop per application
Q->>API: PUT /applications/{id} (void, updated identities)
API->>Server: request
alt success
Server-->>API: 2xx
API-->>Q: AxiosResponse<void>
else failure
Server-->>API: 4xx/5xx
API-->>Q: Error
end
end
Q-->>Form: { success[], failure[] }
alt all success
Form->>Toast: toastr.success (singular/plural)
Form->>Modal: onClose()
else partial/any failures
Form->>Toast: toastr.success + toastr.fail (pluralized)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/app/pages/applications/applications-table/applications-table.tsx (1)
239-246: YAML download is JSON-stringifiedIf getTasksByIds returns YAML text, JSON.stringify wraps it incorrectly. Emit text for YAML.
- const blob = new Blob([JSON.stringify(tasks, null, 2)], { + const payload = + typeof tasks === "string" ? tasks : JSON.stringify(tasks, null, 2); + const blob = new Blob([payload], { type: selectedFormat === "json" ? "application/json" : "application/x-yaml", });
🧹 Nitpick comments (9)
client/src/app/queries/identities.ts (1)
65-69: Type identitiesByKind to preserve key safety.Without an explicit type, TS may infer
{}, degrading downstream safety. Recommend narrowing to IdentityKind keys.Apply this diff:
- const identitiesByKind = useMemo(() => { - return data === undefined ? {} : group(data, (item) => item.kind); - }, [data]); + const identitiesByKind = useMemo<Partial<Record<IdentityKind, Identity[]>>>(() => { + return data ? (group(data, (item) => item.kind) as Record<IdentityKind, Identity[]>) : {}; + }, [data]);And add the missing type import at the top (outside this hunk):
import { Identity, New, IdentityKind } from "@app/api/models";client/public/locales/en/translation.json (1)
142-148: Plural forms: keep markup consistent across variants.“many/other” use , but “one/two/few” don’t. Align for a consistent UX.
Apply one of:
- Add to one/two/few, or
- Remove from many/other.
client/src/app/pages/identities/utils.ts (1)
31-32: Default lookup extended — consider single-pass build.Current mapValues + find() is O(k·n). Optional: compute defaults in one pass for better scalability.
Here’s a drop-in alternative:
export const lookupDefaults = (identities: Identity[]) => { const out: Record<IdentityKind, Identity | undefined> = { source: undefined, maven: undefined, proxy: undefined, "basic-auth": undefined, bearer: undefined, asset: undefined, }; for (const i of identities) { if (i.default && out[i.kind] === undefined) out[i.kind] = i; } return out; };client/src/app/pages/applications/application-identity-form/application-identity-modal.tsx (2)
18-22: Localize modal title for consistencyUse i18n like the rest of the UI.
-import React from "react"; +import React from "react"; +import { useTranslation } from "react-i18next"; @@ -}> = ({ applications, onClose }) => { +}> = ({ applications, onClose }) => { + const { t } = useTranslation(); @@ - <Modal + <Modal isOpen={true} variant="medium" - title="Manage credentials" + title={t("actions.manageCredentials")} onClose={onClose} >
10-12: Guard against empty arraysThe form reads applications[0]; add a length guard to avoid runtime errors if an empty array ever slips through.
- if (!applications) { + if (!applications || applications.length === 0) { return null; }client/src/app/pages/applications/applications-table/applications-table.tsx (2)
531-534: Add “asset” to credential-type filtersThe PR introduces an asset identity kind; expose it in the filters.
selectOptions: [ { value: "source", label: "Source" }, { value: "maven", label: "Maven" }, { value: "proxy", label: "Proxy" }, + { value: "asset", label: "Asset" }, ],
208-217: Simplify credentials modal state (unused "create")"create" is never set; simplify types and derived value.
- ] = useState<"create" | DecoratedApplication[] | null>(null); + ] = useState<DecoratedApplication[] | null>(null); @@ - const applicationsCredentialsToUpdate = - saveApplicationsCredentialsModalState !== "create" - ? saveApplicationsCredentialsModalState - : null; + const applicationsCredentialsToUpdate = saveApplicationsCredentialsModalState;client/src/app/pages/applications/application-identity-form/application-identity-form.tsx (2)
170-179: Ensure applications array is non-empty before reading applications[0]Add a guard or assert earlier to avoid undefined access.
export const ApplicationIdentityForm: React.FC< ApplicationIdentityFormProps > = ({ applications, onClose }) => { + if (!applications.length) return null; @@ } = useForm<FormValues>({ defaultValues: { source: firstIdentityOfKind(applications[0], "source")?.id ?? null, maven: firstIdentityOfKind(applications[0], "maven")?.id ?? null, asset: firstIdentityOfKind(applications[0], "asset")?.id ?? null, },
199-206: Localize field labels and warning textAligns with the rest of the form using i18n keys.
- label={"Source repository credentials"} + label={t("terms.sourceRepositoryCredentials")} @@ - label={"Maven settings"} + label={t("terms.mavenSettings")} @@ - label={"Asset repository credentials"} + label={t("terms.assetRepositoryCredentials")} @@ - {existingIdentitiesError && ( - <Text> + {existingIdentitiesError && ( + <Text> <WarningTriangleIcon className={spacing.mrSm} color="orange" /> - One or more of the selected applications have already been assigned - credentials. Any changes made will override the existing values. + {t("message.existingIdentitiesOverrideNotice")} </Text> )}Also applies to: 224-229, 249-271, 274-280
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
client/public/locales/en/translation.json(4 hunks)client/src/app/api/models.ts(1 hunks)client/src/app/api/rest.ts(1 hunks)client/src/app/pages/applications/application-identity-form/application-identity-form.tsx(4 hunks)client/src/app/pages/applications/application-identity-form/application-identity-modal.tsx(1 hunks)client/src/app/pages/applications/application-identity-form/field-names.ts(0 hunks)client/src/app/pages/applications/application-identity-form/validation-schema.ts(0 hunks)client/src/app/pages/applications/applications-table/applications-table.tsx(6 hunks)client/src/app/pages/identities/utils.ts(2 hunks)client/src/app/queries/applications.ts(2 hunks)client/src/app/queries/identities.ts(2 hunks)client/src/app/utils/model-utils.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- client/src/app/pages/applications/application-identity-form/validation-schema.ts
- client/src/app/pages/applications/application-identity-form/field-names.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-21T06:01:43.078Z
Learnt from: sjd78
PR: konveyor/tackle2-ui#2467
File: client/src/app/queries/generators.ts:20-20
Timestamp: 2025-07-21T06:01:43.078Z
Learning: The tackle2-ui codebase uses React Query v4 (tanstack/react-query v4.22.0), where the onError callback in query configurations is still valid and supported. The deprecation of onError callbacks only applies to React Query v5, so suggestions about removing them are premature until the codebase is upgraded.
Applied to files:
client/src/app/queries/applications.ts
🧬 Code graph analysis (6)
client/src/app/pages/applications/application-identity-form/application-identity-modal.tsx (2)
client/src/app/pages/applications/useDecoratedApplications.ts (1)
DecoratedApplication(39-69)client/src/app/pages/applications/application-identity-form/application-identity-form.tsx (1)
ApplicationIdentityForm(74-304)
client/src/app/utils/model-utils.tsx (1)
client/src/app/components/SimpleSelect.tsx (1)
OptionWithValue(11-14)
client/src/app/pages/applications/applications-table/applications-table.tsx (1)
client/src/app/pages/applications/application-identity-form/application-identity-modal.tsx (1)
ApplicationIdentityModal(6-24)
client/src/app/queries/applications.ts (2)
client/src/app/api/models.ts (1)
Application(138-163)client/src/app/api/rest.ts (1)
updateApplication(312-313)
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx (9)
client/src/app/pages/applications/useDecoratedApplications.ts (1)
DecoratedApplication(39-69)client/src/app/api/models.ts (4)
Identity(219-233)IdentityKind(217-217)Ref(85-88)Application(138-163)client/src/app/components/SimpleSelect.tsx (2)
OptionWithValue(11-14)SimpleSelect(31-69)client/src/app/components/NotificationsContext.tsx (1)
NotificationsContext(31-33)client/src/app/queries/identities.ts (1)
useFetchIdentities(58-78)client/src/app/queries/applications.ts (2)
UpdateAllApplicationsResult(121-124)useBulkPatchApplicationsMutation(166-182)client/src/app/utils/model-utils.tsx (2)
toRef(236-242)toOptionLike(202-207)client/src/app/components/HookFormPFFields/HookFormPFGroupController.tsx (1)
HookFormPFGroupController(42-91)client/src/app/Constants.ts (1)
DEFAULT_SELECT_MAX_HEIGHT(21-21)
client/src/app/api/rest.ts (1)
client/src/app/api/models.ts (2)
Application(138-163)JsonDocument(21-21)
⏰ 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 (12)
client/src/app/api/models.ts (2)
214-215: Add “asset” kind — looks consistent with the UI updates.IdentityKinds now includes "asset"; IdentityKind auto-updates via the tuple. No issues spotted.
214-215: Verify backend supports the new kind.Confirm hub OpenAPI and server validations already accept kind:"asset", otherwise create/update flows will fail.
client/src/app/queries/identities.ts (2)
1-2: New imports are fine.useMemo and radash/group usage is appropriate here.
70-77: Return payload extension LGTM.Adding identitiesByKind alongside existing fields maintains backward compatibility.
client/public/locales/en/translation.json (3)
595-597: Success toasts: LGTM.Keys and placeholders look correct.
610-612: Failure toasts: LGTM.Keys and placeholders look correct.
639-640: New validation messages: LGTM.Names align with common schema validators.
client/src/app/pages/identities/utils.ts (1)
11-12: Adds display label for new kind — good.“Asset Repository” fits the pattern used by other kinds.
client/src/app/queries/applications.ts (1)
165-182: Bulk patch mutation wiring looks goodCache invalidation precedes the external onSuccess; aligns with other mutations.
client/src/app/pages/applications/applications-table/applications-table.tsx (1)
1355-1358: LGTM: new ApplicationIdentityModal integrationClean handoff; state reset on close is correct.
client/src/app/api/rest.ts (1)
312-314: Axios generics updated to void — consistent with 204 responsesThe change aligns return types with hub behavior. No issues spotted.
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx (1)
146-168: Validation schema looks goodOption sets are derived from fetched identities and accept null for clearing.
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx
Show resolved
Hide resolved
Signed-off-by: Scott J Dickerson <[email protected]>
Signed-off-by: Scott J Dickerson <[email protected]>
Signed-off-by: Scott J Dickerson <[email protected]>
…ntities Signed-off-by: Scott J Dickerson <[email protected]>
Signed-off-by: Scott J Dickerson <[email protected]>
sjd78
force-pushed
the
credentials_kind_asset
branch
from
926dc0c to
35b2eec
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
client/src/app/queries/applications.ts (1)
126-151: Per-app patch errors can abort the whole mutation; patch inside the task and catch errorsIf
patch(application)throws for any item, the throw happens beforePromise.allSettled, aborting the batch and losing granular reporting. Move patching inside the per-item async and catch both patch and update errors.- const patchedApplications = applications.map((application) => - patch(application) - ); - - // TODO: Ideally we'd have a single request to update all applications instead of one per application. - const results = await Promise.allSettled( - patchedApplications.map(async (application) => { - try { - const response = await updateApplication(application); - return { - success: { application, response }, - }; - } catch (error) { - return { - failure: { application, cause: error as Error }, - }; - } - }) - ); + // TODO: Ideally we'd have a single request to update all applications instead of one per application. + const results = await Promise.allSettled( + applications.map(async (application) => { + try { + const patched = patch(application); + const response = await updateApplication(patched); + return { success: { application: patched, response } }; + } catch (error) { + return { failure: { application, cause: error as Error } }; + } + }) + );client/src/app/pages/applications/application-identity-form/application-identity-form.tsx (1)
127-139: Do not wipe unrelated identity kinds; preserve non-managed identitiesCurrent patch overwrites
identitieswith only source/maven/asset, dropping other kinds (e.g., proxy). Preserve all non-managed kinds and merge the selected ones.- const onSubmit = ({ source, maven, asset }: FormValues) => { - const updatedIdentities: Ref[] = [ + const onSubmit = ({ source, maven, asset }: FormValues) => { + const updatedIdentities: Ref[] = [ toRef(identitiesByKind.source?.find((i) => i.id === source)), toRef(identitiesByKind.maven?.find((i) => i.id === maven)), toRef(identitiesByKind.asset?.find((i) => i.id === asset)), ].filter(Boolean); - const patch = (application: Application) => { - return Object.assign({}, application, { - identities: updatedIdentities, - }); - }; + const managedIds = new Set<number>([ + ...(identitiesByKind.source ?? []).map((i) => i.id), + ...(identitiesByKind.maven ?? []).map((i) => i.id), + ...(identitiesByKind.asset ?? []).map((i) => i.id), + ]); + + const patch = (application: Application) => { + const preserved = (application.identities ?? []).filter( + (ref) => !managedIds.has(ref.id) + ); + return { + ...application, + identities: [...preserved, ...updatedIdentities], + }; + };
🧹 Nitpick comments (9)
client/src/app/pages/applications/application-identity-form/application-identity-modal.tsx (1)
15-21: Localize the modal title and add aria-labelUse i18n for consistency with the rest of the UI and improve a11y.
Apply within this hunk:
- <Modal + <Modal isOpen={true} variant="medium" - title="Manage credentials" - onClose={onClose} + title={t("dialog.title.manageCredentials")} + aria-label={t("dialog.title.manageCredentials")} + onClose={onClose} >Additionally add outside this hunk:
import { useTranslation } from "react-i18next"; … export const ApplicationIdentityModal: React.FC<…> = ({ applications, onClose }) => { const { t } = useTranslation(); … }client/src/app/pages/applications/applications-table/applications-table.tsx (2)
126-126: Follow-up: include “asset” in the identities filter optionsThis PR adds a new identity kind. Update the identities filter select options in this file to include
{ value: "asset", label: "Asset" }so users can filter by the new kind.
1612-1648: Localize download modal action labelsUse existing translation keys for consistency.
actions={[ - <Button key="confirm" variant="primary" onClick={handleDownload}> - Download - </Button>, + <Button key="confirm" variant="primary" onClick={handleDownload}> + {t("actions.download")} + </Button>, <Button key="cancel" variant="link" onClick={() => setIsDownloadModalOpen(false)} > - Cancel + {t("actions.cancel")} </Button>, ]}client/src/app/pages/applications/application-identity-form/application-identity-form.tsx (6)
146-168: Validation message key mismatch: use “oneOf” (not “notOneOf”)Schemas use
yup.oneOf(…)but error keys reference “notOneOf”. Align the message keys.- source: yup + source: yup .number() .nullable() .oneOf( - [...sourceIdentityOptions.map((o) => o.value), null], - t("validation.notOneOf") + [...sourceIdentityOptions.map((o) => o.value), null], + t("validation.oneOf") ), - maven: yup + maven: yup .number() .nullable() .oneOf( - [...mavenIdentityOptions.map((o) => o.value), null], - t("validation.notOneOf") + [...mavenIdentityOptions.map((o) => o.value), null], + t("validation.oneOf") ), - asset: yup + asset: yup .number() .nullable() .oneOf( - [...assetIdentityOptions.map((o) => o.value), null], - t("validation.notOneOf") + [...assetIdentityOptions.map((o) => o.value), null], + t("validation.oneOf") ),
175-179: Optional: default to “no selection” when multiple apps have mixed credentialsUsing the first app’s identity as the form default can accidentally overwrite others. For bulk edits, prefer
nullunless all selected apps share the same identity per kind.Example outside this hunk:
const commonOrNull = (apps: DecoratedApplication[], kind: IdentityKind) => { const ids = new Set( apps .map((a) => firstIdentityOfKind(a, kind)?.id) .filter((v): v is number => v != null) ); return ids.size === 1 ? [...ids][0] : null; }; useForm<FormValues>({ defaultValues: { source: commonOrNull(applications, "source"), maven: commonOrNull(applications, "maven"), asset: commonOrNull(applications, "asset"), }, … });
199-221: Localize field labels and ARIA labelsHard-coded strings break translations. Use
t(...)keys used elsewhere.- name="source" - label={"Source repository credentials"} + name="source" + label={t("terms.sourceRepositoryCredentials")} … - toggleAriaLabel="Source credentials" + toggleAriaLabel={t("terms.sourceRepositoryCredentials")}
224-247: Localize “Maven settings” label and ARIA labelAlign with i18n usage.
- name="maven" - label={"Maven settings"} + name="maven" + label={t("terms.mavenSettings")} … - toggleAriaLabel="Maven settings" + toggleAriaLabel={t("terms.mavenSettings")}
249-271: Localize “Asset repository credentials” label and ARIA labelNew field should be translated like others.
- name="asset" - label={"Asset repository credentials"} + name="asset" + label={t("terms.assetRepositoryCredentials")} … - toggleAriaLabel="Asset" + toggleAriaLabel={t("terms.assetRepositoryCredentials")}
274-281: Localize and clarify the bulk override warningUse i18n and consider bolding the action effect.
- <Text> - <WarningTriangleIcon className={spacing.mrSm} color="orange" /> - One or more of the selected applications have already been assigned - credentials. Any changes made will override the existing values. - </Text> + <Text> + <WarningTriangleIcon className={spacing.mrSm} color="orange" /> + {t("message.bulkIdentityOverrideWarning")} + </Text>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
client/public/locales/en/translation.json(4 hunks)client/src/app/api/models.ts(1 hunks)client/src/app/api/rest.ts(1 hunks)client/src/app/pages/applications/application-identity-form/application-identity-form.tsx(4 hunks)client/src/app/pages/applications/application-identity-form/application-identity-modal.tsx(1 hunks)client/src/app/pages/applications/application-identity-form/field-names.ts(0 hunks)client/src/app/pages/applications/application-identity-form/validation-schema.ts(0 hunks)client/src/app/pages/applications/applications-table/applications-table.tsx(6 hunks)client/src/app/pages/identities/utils.ts(2 hunks)client/src/app/queries/applications.ts(2 hunks)client/src/app/utils/model-utils.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- client/src/app/pages/applications/application-identity-form/field-names.ts
- client/src/app/pages/applications/application-identity-form/validation-schema.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- client/src/app/api/models.ts
- client/src/app/utils/model-utils.tsx
- client/public/locales/en/translation.json
- client/src/app/pages/identities/utils.ts
- client/src/app/api/rest.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-21T06:01:43.078Z
Learnt from: sjd78
PR: konveyor/tackle2-ui#2467
File: client/src/app/queries/generators.ts:20-20
Timestamp: 2025-07-21T06:01:43.078Z
Learning: The tackle2-ui codebase uses React Query v4 (tanstack/react-query v4.22.0), where the onError callback in query configurations is still valid and supported. The deprecation of onError callbacks only applies to React Query v5, so suggestions about removing them are premature until the codebase is upgraded.
Applied to files:
client/src/app/queries/applications.ts
🧬 Code graph analysis (3)
client/src/app/pages/applications/application-identity-form/application-identity-modal.tsx (1)
client/src/app/pages/applications/useDecoratedApplications.ts (1)
DecoratedApplication(39-69)
client/src/app/pages/applications/applications-table/applications-table.tsx (1)
client/src/app/pages/applications/application-identity-form/application-identity-modal.tsx (1)
ApplicationIdentityModal(6-24)
client/src/app/queries/applications.ts (2)
client/src/app/api/models.ts (1)
Application(138-163)client/src/app/api/rest.ts (1)
updateApplication(312-313)
⏰ 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 (4)
client/src/app/pages/applications/application-identity-form/application-identity-modal.tsx (1)
6-12: LGTM: Thin wrapper keeps modal orchestration close to the formNull-guarding
applicationsand colocating the modal with the form simplifies consumers.client/src/app/queries/applications.ts (1)
172-179: LGTM: Invalidate queries before forwarding results to consumer
awaiting the invalidation avoids stale lists in listeners consuming the results.client/src/app/pages/applications/applications-table/applications-table.tsx (2)
1230-1231: LGTM: Action groups separated into credentials and dependenciesClearer grouping improves discoverability of per-row actions.
1355-1358: LGTM: Switch to ApplicationIdentityModalEncapsulating the form + modal reduces duplication and keeps state management cleaner.
- Retain non-managed identities when updating applications - Mutation to patch then update inside the promise so if the patch fails for a single application, it will still update the remaining applications. - `toOptionLike` will allow 0 and empty string values Signed-off-by: Scott J Dickerson <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
client/src/app/queries/applications.ts (1)
134-148: Granular per-app patch/update via allSettled looks goodMoving patch inside the per-app async and catching errors fixes the earlier “abort on first patch error” behavior and ensures full result aggregation.
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx (1)
127-157: Preserve non-managed identities: resolved as requestedThe patch now keeps identities outside source/maven/asset and overwrites only managed kinds.
🧹 Nitpick comments (8)
client/src/app/queries/applications.ts (2)
129-132: Tighten patch input type to prevent accidental mutationMake the patch arg readonly to discourage in-place mutation.
- patch: (application: Application) => Application; + patch: (application: Readonly<Application>) => Application;
121-124: Avoid leaking Axios types in public result shapeDecouple the exported type from Axios by returning minimal data you actually use.
- import axios, { AxiosError, AxiosResponse } from "axios"; + import axios, { AxiosError } from "axios";-export interface UpdateAllApplicationsResult { - success: { application: Application; response: AxiosResponse<void> }[]; +export interface UpdateAllApplicationsResult { + success: { application: Application }[]; failure: { application: Application; cause: Error }[]; }- const response = await updateApplication(patchedApplication); - return { - success: { application: patchedApplication, response }, - }; + await updateApplication(patchedApplication); + return { success: { application: patchedApplication } };Also applies to: 137-141, 2-2
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx (6)
44-55: Render explicit option labels instead of relying on toStringPatternFly Select renders children; provide children to avoid edge cases.
return identities.map((identity) => ({ value: identity.id, toString: () => identity.name, + props: { children: identity.name }, }));
160-182: Memoize schema to option lists to prevent transient invalid statesKeeps resolver stable and revalidates only when options/i18n change.
- const validationSchema = yup.object({ + const validationSchema = React.useMemo( + () => + yup.object({ source: yup .number() .nullable() .oneOf( [...sourceIdentityOptions.map((o) => o.value), null], t("validation.notOneOf") ), maven: yup .number() .nullable() .oneOf( [...mavenIdentityOptions.map((o) => o.value), null], t("validation.notOneOf") ), asset: yup .number() .nullable() .oneOf( [...assetIdentityOptions.map((o) => o.value), null], t("validation.notOneOf") ), - }); + }), + [sourceIdentityOptions, mavenIdentityOptions, assetIdentityOptions, t] + );
206-212: Long title when many appsListing all names in the title can overflow. Consider “{firstName} and {n} more” for large selections.
216-218: Internationalize field labelsThese user-facing strings should use t(...) for consistency with the rest of the form.
- label={"Source repository credentials"} + label={t("applications.identities.sourceLabel")}- label={"Maven settings"} + label={t("applications.identities.mavenLabel")}- label={"Asset repository credentials"} + label={t("applications.identities.assetLabel")}Also applies to: 241-243, 266-267
223-225: Align aria label with field labelImprove accessibility consistency.
- toggleAriaLabel="Source credentials" + toggleAriaLabel="Source repository credentials"
272-275: Clarify asset select identifiers and aria labelMake IDs consistent and aria label descriptive.
- toggleId="asset-toggle" - id="asset" - toggleAriaLabel="Asset" + toggleId="asset-credentials-toggle" + id="asset-credentials" + toggleAriaLabel="Asset repository credentials"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx(4 hunks)client/src/app/queries/applications.ts(2 hunks)client/src/app/utils/model-utils.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-21T06:01:43.078Z
Learnt from: sjd78
PR: konveyor/tackle2-ui#2467
File: client/src/app/queries/generators.ts:20-20
Timestamp: 2025-07-21T06:01:43.078Z
Learning: The tackle2-ui codebase uses React Query v4 (tanstack/react-query v4.22.0), where the onError callback in query configurations is still valid and supported. The deprecation of onError callbacks only applies to React Query v5, so suggestions about removing them are premature until the codebase is upgraded.
Applied to files:
client/src/app/queries/applications.ts
🧬 Code graph analysis (3)
client/src/app/utils/model-utils.tsx (1)
client/src/app/components/SimpleSelect.tsx (1)
OptionWithValue(11-14)
client/src/app/queries/applications.ts (2)
client/src/app/api/models.ts (1)
Application(138-163)client/src/app/api/rest.ts (1)
updateApplication(312-313)
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx (8)
client/src/app/pages/applications/useDecoratedApplications.ts (1)
DecoratedApplication(39-69)client/src/app/api/models.ts (4)
Identity(219-233)IdentityKind(217-217)Ref(85-88)Application(138-163)client/src/app/components/SimpleSelect.tsx (2)
OptionWithValue(11-14)SimpleSelect(31-69)client/src/app/queries/identities.ts (1)
useFetchIdentities(58-78)client/src/app/queries/applications.ts (2)
UpdateAllApplicationsResult(121-124)useBulkPatchApplicationsMutation(163-179)client/src/app/utils/model-utils.tsx (3)
toRef(238-244)toRefs(256-260)toOptionLike(202-209)client/src/app/components/HookFormPFFields/HookFormPFGroupController.tsx (1)
HookFormPFGroupController(42-91)client/src/app/Constants.ts (1)
DEFAULT_SELECT_MAX_HEIGHT(21-21)
⏰ 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 (4)
client/src/app/utils/model-utils.tsx (1)
202-209: Simplify nullish check & support object comparisons- return value === null || value === undefined + return value == nullStrict
===works for primitives but misses equal‐by‐id objects—add a comparator‐based helper (e.g.toOptionLikeBy/toOptionLikeById) for options carrying objects.client/src/app/queries/applications.ts (1)
162-179: Invalidate then fan-out success callback: LGTMHook wiring and cache invalidation are correct; async onSuccess is fine in RQ v4.
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx (2)
165-167: Confirm i18n key for a oneOf validatorYou’re calling oneOf but using message key “validation.notOneOf”. Verify this is intentional and mapped to correct copy.
Also applies to: 172-174, 179-181
198-203: Warning hidden for single-app edits — intentional?The override warning is suppressed when editing one app. Confirm UX; overriding an existing credential on a single app might still merit the warning.
…nage credentials modal (konveyor#2579) Resolves: konveyor#2581 To support asset generation, applications need to have an associated "Asset repository credentials" identity. Add a slot to the application manage credentials modal for this new association. Related changes: - Cleanup the application rest function return types - Add the `ApplicationIdentityModal` to keep the modal management closer to the form - Add "asset" as a valid identity kind - Refactor the mutation as `useBulkPatchApplicationsMutation` to bring the handling inline with how the asset gen/platform awarness wizards handle making multiple calls -- Promise.allSettled() with reporting based on individual update success/fails. ## Summary by CodeRabbit - **New Features** - Bulk manage credentials for multiple applications, now including Asset repository credentials. - Added modals: Manage credentials, Import Applications, Manage dependencies, and Download analysis (JSON/YAML). - **Improvements** - Actions in the Applications table split into groups for credentials and dependencies. - Clearer per-application success/failure notifications for bulk updates. - **Localization** - Enhanced pluralized dialogs/toasts and a new validation message for invalid selections. --------- Signed-off-by: Scott J Dickerson <[email protected]>
2 participants
Resolves: #2581
To support asset generation, applications need to have an associated "Asset repository credentials" identity.
Add a slot to the application manage credentials modal for this new association.
Related changes:
ApplicationIdentityModalto keep the modal management closer to the formuseBulkPatchApplicationsMutationto bring the handling inline with how the asset gen/platform awarness wizards handle making multiple calls -- Promise.allSettled() with reporting based on individual update success/fails.Screenshot:
Note: Currently includes #2578
Summary by CodeRabbit
New Features
Improvements
Localization