🌱 Refactor analysis wizard #2747
Conversation
Refactor analysis wizard to use the wizard pattern used
by the generate assets wizard.
- The wizard's state is managed centrally by the `useWizardReducer` hook
- Each step has its own hook-form and is initialized from the central state
- The `useTaskGroupManager` hook is used to manage the taskgroup lifecycle
- The `useFormChangeHandler` hook is used by each hook-form/step to handle
changes to the form and pushing it up the the wizard's state
Pre-requisite for konveyor#2713
Signed-off-by: Scott J Dickerson <[email protected]>
WalkthroughRefactors the Analysis Wizard to use reducer-based state management instead of form context. Introduces new hooks for form watching, taskgroup lifecycle management, and centralized wizard state. Replaces context providers with explicit props, modularizes step components, consolidates schema definitions, and removes legacy context patterns. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AnalysisWizard
participant useWizardReducer as useWizardReducer<br/>(State Management)
participant StepComponents as Step Components<br/>(AnalysisMode, SetTargets, etc.)
participant useTaskGroupManager as useTaskGroupManager<br/>(Taskgroup Lifecycle)
participant API as API
User->>AnalysisWizard: Opens wizard
AnalysisWizard->>useWizardReducer: Initialize state with initialRecipe
useWizardReducer-->>AnalysisWizard: WizardState (mode, targets, scope, customRules, options, isReady)
User->>StepComponents: Interacts with step (e.g., selects mode)
StepComponents->>StepComponents: useForm validates with schema
StepComponents->>AnalysisWizard: onStateChanged(stepState)
AnalysisWizard->>useWizardReducer: setMode/setTargets/etc. dispatch
useWizardReducer->>useWizardReducer: Draft update with Immer, compute isReady
useWizardReducer-->>AnalysisWizard: Updated WizardState
AnalysisWizard-->>StepComponents: Pass updated state to next steps
User->>AnalysisWizard: Clicks Review & Submit
AnalysisWizard->>useTaskGroupManager: createTaskGroup(taskgroupData from wizard state)
useTaskGroupManager->>API: POST /taskgroups
API-->>useTaskGroupManager: Taskgroup created
useTaskGroupManager-->>AnalysisWizard: taskGroup object
AnalysisWizard->>useTaskGroupManager: submitTaskGroup(taskGroup)
useTaskGroupManager->>API: POST /taskgroups/{id}/submit
API-->>useTaskGroupManager: Submission result
useTaskGroupManager-->>AnalysisWizard: Success/Error notification
AnalysisWizard-->>User: Navigate away / Show result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
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/applications/analysis-wizard/steps/advanced-options.tsx (1)
154-223: Target labels selector bound to wrong form field.The
HookFormPFGroupControlleron line 154-156 usesname="selectedSourceLabels"but renders a target labels selector. The component:
- Never calls
onChangeto update the form field- Uses internal
selectedTargetLabelsstate instead- Only calls
onSelectedTargetsChangedto update parent stateThis means:
- The form field
selectedSourceLabelsisn't being updated by this selector- The validation state from this field applies to the wrong UI element
Consider either:
- Removing
HookFormPFGroupControllerwrapper since this isn't actually a form-controlled field- Adding a separate form field for target labels if they need to be part of the form state
- <HookFormPFGroupController - control={control} - name="selectedSourceLabels" - label={t("wizard.terms.target", { count: 2 })} - fieldId="target-labels" - renderInput={({ + <FormGroup + label={t("wizard.terms.target", { count: 2 })} + fieldId="target-labels" + > + <Selectclient/src/app/pages/applications/analysis-wizard/schema.ts (1)
155-164: Cross-step validation references non-existent fields in this schema.The
.when()clause referencesselectedTargetLabelsandselectedTargets, but these fields don't exist inCustomRulesStepValues— they belong toSetTargetsValues. In the new per-step schema architecture, this validation will not function correctly because those fields are always undefined in this schema's context.The TODO comment suggests passing these values via hook parameters. Consider refactoring to:
-export const useCustomRulesSchema = (): yup.SchemaOf<CustomRulesStepValues> => { +export const useCustomRulesSchema = ({ + hasTargets, +}: { + hasTargets: boolean; +}): yup.SchemaOf<CustomRulesStepValues> => { return yup.object({ // ... customRulesFiles: yup .array() .of(UploadFileSchema) .when("rulesKind", { is: "manual", - then: yup.array().of(UploadFileSchema), + then: (schema) => + hasTargets ? schema : schema.min(1, "At least 1 Rule File is required"), otherwise: (schema) => schema, }) - .when(["selectedTargetLabels", "rulesKind", "selectedTargets"], { - // TODO: Rewrite this when clause, input the fields via hook params - is: ( - labels: TargetLabel[], - rulesKind: string, - selectedTargets: number - ) => - labels.length === 0 && rulesKind === "manual" && selectedTargets <= 0, - then: (schema) => schema.min(1, "At least 1 Rule File is required"), - }), + ,Would you like me to generate a complete implementation or open an issue to track this?
🧹 Nitpick comments (9)
client/src/app/hooks/useFormChangeHandler.ts (1)
48-50: Document stability requirement foronStateChangedcallback.The
onStateChangedcallback is in theuseEffectdependency array. If the caller doesn't wrap it inuseCallback, this effect will fire on every render, potentially causing excessive updates or infinite loops. Consider either:
- Adding a JSDoc comment to warn callers that
onStateChangedshould be stable (memoized)- Using a ref to store the callback to avoid re-triggering the effect
+/** + * Generic form change handler that watches specific form fields and calls a state change callback. + * @template TFormValues - The type of the form values + * @template TState - The type of the state object + * @template TWatchFields - The tuple type of field paths being watched + * + * @note The `onStateChanged` callback should be wrapped in `useCallback` to prevent + * excessive effect executions on each render. + */ export const useFormChangeHandler = <client/src/app/pages/applications/analysis-wizard/components/upload-new-rules-files.tsx (1)
53-56: Acknowledge the TODO for orphaned file cleanup.The TODO is a valid concern—files uploaded to hub before the user cancels will remain orphaned. This is acceptable as technical debt, but consider tracking it in an issue if it's not addressed in a follow-up.
Do you want me to open an issue to track implementing the cleanup of orphaned uploaded files when the modal is cancelled?
client/src/app/pages/applications/analysis-wizard/utils.ts (1)
10-26: LGTM! Consider combining duplicate case logic.The refactored
isModeSupportedfunction consolidates the previous discrete validators into a single switch-based function, which improves maintainability.The
source-code-depsandsource-codecases have identical logic and could be combined using case fallthrough:case "source-code-deps": - return isNotEmptyString(application?.repository?.url); - case "source-code": return isNotEmptyString(application?.repository?.url);client/src/app/pages/applications/analysis-wizard/steps/analysis-scope.tsx (1)
30-32: Consider movingpackageNameSchemaoutside the component.The schema is recreated on every render but never changes. Moving it to module scope would avoid unnecessary object allocation:
+const packageNameSchema = yup.string().matches(/^[a-z]+(.[a-z0-9]+)*$/, { + message: "Must be a valid Java package name", +}); + +export const AnalysisScope: React.FC<AnalysisScopeProps> = ({ ... - const packageNameSchema = yup.string().matches(/^[a-z]+(.[a-z0-9]+)*$/, { - message: "Must be a valid Java package name", - });client/src/app/pages/applications/analysis-wizard/steps/analysis-mode.tsx (1)
76-94: Consider using explicit type annotation for better type safety.The
filter(Boolean)pattern works but loses type information. An explicit type guard would be cleaner:- const analysisOptions: SelectOptionProps[] = [ + const analysisOptions = [ { value: "source-code-deps", children: "Source code + dependencies", }, ... isSingleApp && { value: "binary-upload", children: "Upload a local binary", }, - ].filter(Boolean); + ].filter((opt): opt is SelectOptionProps => Boolean(opt));client/src/app/pages/applications/analysis-wizard/steps/custom-rules.tsx (2)
159-161: Potential stale initial tab state.The
activeTabKeyis initialized usingrulesKindfromuseWatch, but sinceuseWatchsubscribes to form values, the initial value during the first render may not reflect the form'sdefaultValuesuntil after hydration. Consider usinginitialState.rulesKinddirectly for the initial state calculation:- const [activeTabKey, setActiveTabKey] = React.useState(() => - rulesKind === "manual" ? 0 : rulesKind === "repository" ? 1 : 0 + const [activeTabKey, setActiveTabKey] = React.useState(() => + initialState.rulesKind === "manual" ? 0 : initialState.rulesKind === "repository" ? 1 : 0 );
163-177: Consider triggering validation after programmatic value updates.When using
setValueprogrammatically (lines 169, 184), the form'sisValidstate may not immediately reflect the new values. SinceuseFormChangeHandlerrelies onisValidto propagate state to the parent wizard, consider adding{ shouldValidate: true }to ensure validation runs:- setValue("customRulesFiles", newCustomRulesFiles); + setValue("customRulesFiles", newCustomRulesFiles, { shouldValidate: true });This ensures the wizard's step-enabled logic stays in sync with form validity.
client/src/app/pages/applications/analysis-wizard/analysis-wizard.tsx (1)
119-127: Address TODO: VerifyassociatedCredentialsmatching logic.The TODO comment on line 126 indicates uncertainty about the credentials matching logic. The current implementation matches by name, which should work if
identity.nameis unique and consistent with what's stored inwizardState.customRules.associatedCredentials.Would you like me to help verify this logic or open an issue to track the verification?
client/src/app/pages/applications/analysis-wizard/schema.ts (1)
69-75: Consider adding explicit element types to array schemas.The schema validates that fields are arrays but doesn't enforce element types via
.of(). Whileyup.SchemaOf<SetTargetsValues>provides compile-time type safety, runtime validation is permissive. This may be intentional for a wizard step with controlled inputs.If stricter runtime validation is desired:
export const useSetTargetsSchema = (): yup.SchemaOf<SetTargetsValues> => { return yup.object({ - selectedTargetLabels: yup.array(), - selectedTargets: yup.array(), + selectedTargetLabels: yup.array().of( + yup.object().shape({ + name: yup.string().defined(), + label: yup.string().defined(), + }) + ).defined(), + selectedTargets: yup.array().defined(), targetFilters: yup.object(), }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
client/src/app/hooks/useFormChangeHandler.ts(1 hunks)client/src/app/pages/applications/analysis-wizard/__tests__/utils.test.tsx(2 hunks)client/src/app/pages/applications/analysis-wizard/analysis-wizard.tsx(5 hunks)client/src/app/pages/applications/analysis-wizard/components/TaskGroupContext.tsx(0 hunks)client/src/app/pages/applications/analysis-wizard/components/upload-binary.tsx(4 hunks)client/src/app/pages/applications/analysis-wizard/components/upload-new-rules-files.tsx(1 hunks)client/src/app/pages/applications/analysis-wizard/schema.ts(6 hunks)client/src/app/pages/applications/analysis-wizard/set-mode.tsx(0 hunks)client/src/app/pages/applications/analysis-wizard/steps/advanced-options.tsx(9 hunks)client/src/app/pages/applications/analysis-wizard/steps/analysis-mode.tsx(1 hunks)client/src/app/pages/applications/analysis-wizard/steps/analysis-scope.tsx(5 hunks)client/src/app/pages/applications/analysis-wizard/steps/custom-rules.tsx(12 hunks)client/src/app/pages/applications/analysis-wizard/steps/review.tsx(12 hunks)client/src/app/pages/applications/analysis-wizard/steps/set-targets.tsx(8 hunks)client/src/app/pages/applications/analysis-wizard/useTaskGroupManager.ts(1 hunks)client/src/app/pages/applications/analysis-wizard/useWizardReducer.ts(1 hunks)client/src/app/pages/applications/analysis-wizard/utils.ts(1 hunks)client/src/app/pages/applications/applications-table/applications-table.tsx(6 hunks)client/src/app/utils/utils.ts(1 hunks)
💤 Files with no reviewable changes (2)
- client/src/app/pages/applications/analysis-wizard/set-mode.tsx
- client/src/app/pages/applications/analysis-wizard/components/TaskGroupContext.tsx
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: sjd78
Repo: konveyor/tackle2-ui PR: 2558
File: client/src/app/pages/applications/generate-assets-wizard/generate-assets-wizard.tsx:0-0
Timestamp: 2025-08-15T22:35:55.912Z
Learning: The Generate Assets Wizard in client/src/app/pages/applications/generate-assets-wizard/generate-assets-wizard.tsx is designed to accommodate additional wizard steps in the future, so the current conditional rendering of the Results step is intentional and appropriate for the planned architecture.
📚 Learning: 2025-08-15T22:35:55.912Z
Learnt from: sjd78
Repo: konveyor/tackle2-ui PR: 2558
File: client/src/app/pages/applications/generate-assets-wizard/generate-assets-wizard.tsx:0-0
Timestamp: 2025-08-15T22:35:55.912Z
Learning: The Generate Assets Wizard in client/src/app/pages/applications/generate-assets-wizard/generate-assets-wizard.tsx is designed to accommodate additional wizard steps in the future, so the current conditional rendering of the Results step is intentional and appropriate for the planned architecture.
Applied to files:
client/src/app/pages/applications/analysis-wizard/steps/analysis-mode.tsxclient/src/app/pages/applications/analysis-wizard/useWizardReducer.tsclient/src/app/pages/applications/applications-table/applications-table.tsxclient/src/app/pages/applications/analysis-wizard/schema.tsclient/src/app/pages/applications/analysis-wizard/steps/set-targets.tsxclient/src/app/pages/applications/analysis-wizard/steps/custom-rules.tsxclient/src/app/pages/applications/analysis-wizard/analysis-wizard.tsx
📚 Learning: 2025-10-03T14:26:45.291Z
Learnt from: sjd78
Repo: konveyor/tackle2-ui PR: 2631
File: client/src/app/api/rest/applications.ts:14-15
Timestamp: 2025-10-03T14:26:45.291Z
Learning: In the tackle2-ui codebase, delete and update REST functions in files under `client/src/app/api/rest/` return `AxiosPromise<void>` (which resolves to `AxiosResponse<void>`) rather than unwrapped `Promise<void>`. This return type is expected and correctly handled by the React Query mutation hooks in `client/src/app/queries/`, which accept promise-compatible types including AxiosPromise.
Applied to files:
client/src/app/pages/applications/applications-table/applications-table.tsx
📚 Learning: 2025-08-13T08:58:45.985Z
Learnt from: sjd78
Repo: konveyor/tackle2-ui PR: 2545
File: client/src/app/pages/source-platforms/useFetchPlatformsWithTasks.ts:42-59
Timestamp: 2025-08-13T08:58:45.985Z
Learning: In the groupPlatformTasks function in client/src/app/pages/source-platforms/useFetchPlatformsWithTasks.ts, the non-null assertion on task.platform!.id after filtering for tasks with platform is acceptable and safe according to the code maintainer sjd78.
Applied to files:
client/src/app/pages/applications/analysis-wizard/useTaskGroupManager.tsclient/src/app/pages/applications/analysis-wizard/analysis-wizard.tsx
📚 Learning: 2025-07-29T14:20:50.580Z
Learnt from: sjd78
Repo: konveyor/tackle2-ui PR: 2468
File: client/src/app/pages/source-platforms/usePlatformProviderList.ts:1-3
Timestamp: 2025-07-29T14:20:50.580Z
Learning: TargetCard component is focused on transformation targets, not source platform providers. It imports Target/TargetLabel models and operates independently from usePlatformProviderList. The codebase maintains clear separation between source platforms (managed by usePlatformProviderList) and transformation targets (managed by TargetCard).
Applied to files:
client/src/app/pages/applications/analysis-wizard/steps/advanced-options.tsxclient/src/app/pages/applications/analysis-wizard/steps/set-targets.tsxclient/src/app/pages/applications/analysis-wizard/__tests__/utils.test.tsxclient/src/app/pages/applications/analysis-wizard/steps/review.tsx
📚 Learning: 2025-07-17T23:29:20.652Z
Learnt from: sjd78
Repo: konveyor/tackle2-ui PR: 2432
File: client/src/app/components/schema-defined-fields/utils.tsx:42-47
Timestamp: 2025-07-17T23:29:20.652Z
Learning: In the tackle2-ui codebase, when using yup.lazy for optional object validation, the correct TypeScript typing is `ReturnType<typeof yup.lazy<yup.AnySchema>>` instead of passing the generic type parameter directly to yup.lazy, due to the specific version of yup being used.
Applied to files:
client/src/app/pages/applications/analysis-wizard/schema.ts
📚 Learning: 2025-10-09T06:42:54.063Z
Learnt from: sjd78
Repo: konveyor/tackle2-ui PR: 2660
File: client/src/app/components/repository-fields/schema.ts:72-99
Timestamp: 2025-10-09T06:42:54.063Z
Learning: In the tackle2-ui repository, prefer the `when, is, then` style for Yup schemas (e.g., `.when(["field"], { is: ..., then: ... })`). Ignore Biome's `noThenProperty` warning in these circumstances.
Applied to files:
client/src/app/pages/applications/analysis-wizard/schema.ts
📚 Learning: 2025-07-29T14:20:50.580Z
Learnt from: sjd78
Repo: konveyor/tackle2-ui PR: 2468
File: client/src/app/pages/source-platforms/usePlatformProviderList.ts:1-3
Timestamp: 2025-07-29T14:20:50.580Z
Learning: The TargetCard component operates independently from the usePlatformProviderList hook and source platform functionality. References to cloud providers like "Azure" in TargetCard are unrelated to the platform provider list changes and serve a different purpose in the application.
Applied to files:
client/src/app/pages/applications/analysis-wizard/steps/set-targets.tsx
📚 Learning: 2025-08-29T16:01:04.207Z
Learnt from: sjd78
Repo: konveyor/tackle2-ui PR: 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/applications/analysis-wizard/steps/custom-rules.tsx
📚 Learning: 2025-08-20T21:09:47.113Z
Learnt from: sjd78
Repo: konveyor/tackle2-ui PR: 2534
File: client/src/app/pages/archetypes/components/tab-details-content.tsx:116-136
Timestamp: 2025-08-20T21:09:47.113Z
Learning: When dealing with singular/plural translation keys, prefer i18next pluralization (e.g., t("terms.stakeholder", { count: number })) over hardcoded plural keys like "terms.stakeholders" or keys with punctuation like "terms.stakeholder(s)". This provides better internationalization support and cleaner translation management.
Applied to files:
client/src/app/pages/applications/analysis-wizard/steps/review.tsx
📚 Learning: 2025-07-21T06:01:43.078Z
Learnt from: sjd78
Repo: konveyor/tackle2-ui PR: 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/pages/applications/analysis-wizard/analysis-wizard.tsx
🧬 Code graph analysis (8)
client/src/app/pages/applications/analysis-wizard/steps/analysis-mode.tsx (7)
client/src/app/api/models.ts (2)
Application(145-170)Taskgroup(499-506)client/src/app/pages/applications/analysis-wizard/schema.ts (4)
AnalysisModeState(28-30)AnalysisMode(21-21)useAnalysisModeSchema(32-56)AnalysisModeValues(23-26)client/src/app/hooks/useFormChangeHandler.ts (1)
useFormChangeHandler(16-51)client/src/app/pages/applications/analysis-wizard/utils.ts (1)
isModeSupported(10-26)client/src/app/components/HookFormPFFields/HookFormPFGroupController.tsx (1)
HookFormPFGroupController(42-91)client/src/app/components/SimpleSelectBasic.tsx (1)
SimpleSelectBasic(21-72)client/src/app/pages/applications/analysis-wizard/components/upload-binary.tsx (1)
UploadBinary(43-216)
client/src/app/pages/applications/analysis-wizard/useWizardReducer.ts (1)
client/src/app/pages/applications/analysis-wizard/schema.ts (5)
AnalysisModeState(28-30)SetTargetsState(65-67)AnalysisScopeState(87-89)CustomRulesStepState(124-126)AdvancedOptionsState(198-200)
client/src/app/pages/applications/analysis-wizard/components/upload-new-rules-files.tsx (2)
client/src/app/api/models.ts (2)
Taskgroup(499-506)UploadFile(600-609)client/src/app/components/CustomRuleFilesUpload.tsx (1)
CustomRuleFilesUpload(52-248)
client/src/app/pages/applications/analysis-wizard/useTaskGroupManager.ts (4)
client/src/app/components/NotificationsContext.tsx (1)
NotificationsContext(31-33)client/src/app/api/models.ts (2)
Taskgroup(499-506)New(9-9)client/src/app/queries/taskgroups.ts (3)
useCreateTaskgroupMutation(15-18)useSubmitTaskgroupMutation(20-37)useDeleteTaskgroupMutation(75-89)client/src/app/pages/applications/analysis-wizard/analysis-wizard.tsx (1)
defaultTaskgroup(64-71)
client/src/app/pages/applications/analysis-wizard/schema.ts (3)
client/src/app/pages/applications/analysis-wizard/steps/analysis-mode.tsx (1)
AnalysisMode(34-155)client/src/app/pages/applications/analysis-wizard/utils.ts (1)
useAnalyzableApplicationsByMode(42-55)client/src/app/pages/applications/analysis-wizard/steps/analysis-scope.tsx (1)
AnalysisScope(23-155)
client/src/app/pages/applications/analysis-wizard/utils.ts (2)
client/src/app/api/models.ts (1)
Application(145-170)client/src/app/utils/utils.ts (1)
isNotEmptyString(267-269)
client/src/app/pages/applications/analysis-wizard/steps/set-targets.tsx (6)
client/src/app/queries/targets.ts (1)
useFetchTargets(19-67)client/src/app/components/FilterToolbar/FilterToolbar.tsx (1)
FilterSelectOptionProps(27-33)client/src/app/utils/rules-utils.ts (1)
toLabelValue(84-84)client/src/app/utils/utils.ts (1)
universalComparator(218-224)client/src/app/pages/applications/analysis-wizard/schema.ts (3)
SetTargetsState(65-67)useSetTargetsSchema(69-75)SetTargetsValues(59-63)client/src/app/hooks/useFormChangeHandler.ts (1)
useFormChangeHandler(16-51)
client/src/app/pages/applications/analysis-wizard/__tests__/utils.test.tsx (2)
client/src/app/api/models.ts (1)
Application(145-170)client/src/app/pages/applications/analysis-wizard/utils.ts (1)
isModeSupported(10-26)
🪛 Biome (2.1.2)
client/src/app/pages/applications/analysis-wizard/schema.ts
[error] 53-53: 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 (21)
client/src/app/utils/utils.ts (1)
267-269: LGTM!Clean utility function that properly handles
undefined,null, and whitespace-only strings.client/src/app/pages/applications/analysis-wizard/__tests__/utils.test.tsx (1)
59-191: LGTM!Comprehensive test coverage for
isModeSupportedwith good edge case handling. Tests properly validate all mode branches including the defaultfalsereturn for unknown modes.client/src/app/pages/applications/applications-table/applications-table.tsx (2)
1324-1330: LGTM!Clean removal of the
TaskGroupProviderwrapper. TheAnalysisWizardnow manages its own taskgroup lifecycle internally viauseTaskGroupManager, which aligns with the PR's goal of centralizing state management within the wizard.
847-864: Good defensive guards added for navigation.The added checks for
application?.idandarchetypeRefsToOverride?.lengthprevent potential navigation errors when these values are unexpectedly null/undefined.client/src/app/pages/applications/analysis-wizard/useWizardReducer.ts (2)
69-103: LGTM! Well-structured reducer with Immer integration.The reducer pattern is clean, with proper action handling and centralized
isReadyvalidation after each mutation.The
RESETcase correctly returns the pre-computedfirstInitialState(which hasisReadyalready calculated inuseImmerInitialState), so the validation logic is properly bypassed since it was already applied.
121-178: LGTM! Stable callback pattern is well-implemented.The hook provides properly memoized callbacks via
useCallbackwith correct dependencies. The initial state computation viauseImmerInitialStateensures the recipe is only applied once, aligning with React 18's useReducer initialization pattern as noted in the comment.client/src/app/pages/applications/analysis-wizard/steps/analysis-scope.tsx (1)
46-72: LGTM! Clean form-to-wizard state synchronization.The
useFormChangeHandlercorrectly maps all watched fields to state, and the separateuseWatchfor UI rendering keeps the component reactive. The optional chaining on watched values handles the initial undefined state correctly.client/src/app/pages/applications/analysis-wizard/steps/analysis-mode.tsx (2)
34-66: LGTM! Clean integration with form and wizard state management.The component correctly:
- Initializes form state from
initialState- Uses the schema hook for validation with proper i18n support
- Syncs form changes to wizard state via
useFormChangeHandler
145-152: LGTM! Clean UploadBinary integration.The component correctly passes task group management props and wires the artifact change callback to update form state, maintaining proper data flow between the upload component and the wizard.
client/src/app/pages/applications/analysis-wizard/components/upload-binary.tsx (3)
53-59: Potential stale state ifartifactprop changes externally.The
fileUploadProgressandfileUploadStatusstates are initialized from theartifactprop but won't update if the prop changes after mount. If the artifact can be reset externally (e.g., via wizard reset), the UI state could become inconsistent.Consider syncing state with prop changes:
+ React.useEffect(() => { + if (artifact) { + setFileUploadProgress(artifact.size > 0 ? 100 : undefined); + setFileUploadStatus("success"); + } else { + setFileUploadProgress(undefined); + setFileUploadStatus(undefined); + } + }, [artifact]);
133-155: LGTM! Clean async task group creation pattern.The lazy task group creation (
taskGroup ? taskGroup.id : (await createTaskGroup()).id) correctly ensures a task group exists before uploading, and the error handling properly catches failures from both file reading and task group creation.
157-164: LGTM! Defensive check for task group existence.The guard
if (taskGroup)correctly handles the edge case where a user might try to clear the file before the upload completes (when no task group exists yet on the server).client/src/app/pages/applications/analysis-wizard/steps/review.tsx (2)
48-55: LGTM!The component signature cleanly accepts all required data via explicit props, removing the form context dependency. This aligns well with the new reducer-based wizard architecture.
128-130: I'm unable to access the repository at this moment due to persistent clone failures. However, I can assess the review comment based on the code logic and the concern raised.The review comment raises a valid verification point about the
withKnownLibsformat assumption, but without access to the codebase, I cannot definitively confirm:
- The actual type definition of
AnalysisScopeValues.withKnownLibs- How the field is populated in the
AnalysisScopestep- Whether it's consistently formatted as a comma-separated string
withKnownLibsstring format should be verified for consistency.The code at lines 128-130 assumes
scope.withKnownLibsis a comma-separated string (e.g.,"app,oss,select"). Confirm this format aligns with:
- The type definition of
AnalysisScopeValues.withKnownLibs- How the field is populated in the
AnalysisScopestep component- Whether single values (without commas) are possible, and if so, whether
split(",")handles them correctlyThis assumption should be validated against the actual data structure and initialization logic.
client/src/app/pages/applications/analysis-wizard/steps/set-targets.tsx (1)
41-102: Good extraction of data fetching logic.The
useTargetsDatahook cleanly encapsulates target fetching, tag categories, provider computation, and label options. This improves testability and separation of concerns.client/src/app/pages/applications/analysis-wizard/analysis-wizard.tsx (2)
155-162: Address TODO: VerifyincludedPackageslogic.The TODO on line 156 questions whether the
withKnownLibs.includes("select")check is correct for determining when to include packages. Based on theAnalysisScopestep, this should be correct since "select" is the option for manually selecting packages. Consider removing the TODO after verification or clarifying the expected behavior.
81-88: LGTM!The
StepIdenum provides clear, type-safe step identification. The numeric values enable theisStepEnabledlogic to work correctly with sequential step navigation.client/src/app/pages/applications/analysis-wizard/steps/advanced-options.tsx (1)
224-295: LGTM!The sources selector is correctly implemented - it uses the form's
valuefrom thefieldprop, properly callsonChangeto update the form state, and handles selection/deselection logic correctly.client/src/app/pages/applications/analysis-wizard/schema.ts (3)
14-56: LGTM!The analysis mode schema correctly validates mode compatibility based on analyzable applications and conditionally requires the artifact for
binary-uploadmode. Thewhen/is/thenpattern on line 53 is the preferred style in this codebase, so the BiomenoThenPropertywarning can be safely ignored. Based on learnings.
77-111: LGTM!The scope schema correctly uses conditional validation. The
withKnownLibs.includes("select")check works as intended since it's checking if the string value (e.g.,"app,oss,select") contains the substring"select".
190-215: LGTM!The advanced options schema correctly validates the nested
TargetLabelshape forselectedSourceLabelsand properly marks boolean fields as.defined().
| // For custom rules and options steps, we need additional state management | ||
| const [ | ||
| selectedTargetLabelsForCustomRules, | ||
| setSelectedTargetLabelsForCustomRules, | ||
| ] = React.useState(state.targets.selectedTargetLabels); | ||
|
|
||
| const handleTargetsChanged = (newTargetsState: typeof state.targets) => { | ||
| setTargets(newTargetsState); | ||
| setSelectedTargetLabelsForCustomRules(newTargetsState.selectedTargetLabels); | ||
| }; |
There was a problem hiding this comment.
Potential state synchronization issue with selectedTargetLabelsForCustomRules.
The selectedTargetLabelsForCustomRules state is initialized from state.targets.selectedTargetLabels but only updated via handleTargetsChanged. However, the CustomRules component can also modify labels via onSelectedTargetLabelsChanged (line 372), which updates selectedTargetLabelsForCustomRules but doesn't sync back to state.targets.selectedTargetLabels.
This creates a divergence where:
SetTargetsupdates → syncs to bothCustomRulesupdates → only updates local state, notstate.targets
Consider whether changes from CustomRules should also update the wizard's target state for consistency across steps.
🤖 Prompt for AI Agents
client/src/app/pages/applications/analysis-wizard/analysis-wizard.tsx around
lines 246-255: selectedTargetLabelsForCustomRules is seeded from
state.targets.selectedTargetLabels but can be mutated by CustomRules without
updating state.targets, causing state divergence; update the
onSelectedTargetLabelsChanged handler to both
setSelectedTargetLabelsForCustomRules(newLabels) and propagate the change into
the wizard target state by calling setTargets(prev => ({ ...prev,
selectedTargetLabels: newLabels })) (or consolidate into a single source of
truth by removing the duplicate local state and always reading/writing
selectedTargetLabels from state.targets).
| onClose(); | ||
| }; | ||
|
|
||
| const isAddDisabled = !fields.every(({ status }) => status === "uploaded"); |
There was a problem hiding this comment.
Add button enabled when no files are uploaded.
When fields is empty, [].every(...) returns true, making isAddDisabled = false. This allows clicking "Add" with zero files, which is likely unintended.
- const isAddDisabled = !fields.every(({ status }) => status === "uploaded");
+ const isAddDisabled =
+ fields.length === 0 ||
+ !fields.every(({ status }) => status === "uploaded");🤖 Prompt for AI Agents
client/src/app/pages/applications/analysis-wizard/components/upload-new-rules-files.tsx
around line 63: the current expression uses [].every(...) which returns true for
an empty array and thus enables the Add button when no files exist; change the
logic to disable Add when fields is empty or when any field is not uploaded —
e.g., set isAddDisabled to true if fields.length === 0 OR not every status ===
"uploaded". Update the line to first check fields.length and then the every(...)
result so the Add button is only enabled when there is at least one field and
all fields have status "uploaded".
| onChangeRuleFile={(ruleFile) => { | ||
| const index = fields.findIndex( | ||
| (f) => f.fileName === ruleFile.fileName | ||
| ); | ||
| update(index, ruleFile); | ||
| }} |
There was a problem hiding this comment.
Guard against file not found in onChangeRuleFile.
If findIndex returns -1 (file not in fields), calling update(-1, ruleFile) may cause unexpected behavior with react-hook-form's useFieldArray.
onChangeRuleFile={(ruleFile) => {
const index = fields.findIndex(
(f) => f.fileName === ruleFile.fileName
);
- update(index, ruleFile);
+ if (index >= 0) {
+ update(index, ruleFile);
+ }
}}📝 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.
| onChangeRuleFile={(ruleFile) => { | |
| const index = fields.findIndex( | |
| (f) => f.fileName === ruleFile.fileName | |
| ); | |
| update(index, ruleFile); | |
| }} | |
| onChangeRuleFile={(ruleFile) => { | |
| const index = fields.findIndex( | |
| (f) => f.fileName === ruleFile.fileName | |
| ); | |
| if (index >= 0) { | |
| update(index, ruleFile); | |
| } | |
| }} |
🤖 Prompt for AI Agents
In
client/src/app/pages/applications/analysis-wizard/components/upload-new-rules-files.tsx
around lines 98 to 103, onChangeRuleFile calls update(index, ruleFile) without
checking the result of fields.findIndex; if findIndex returns -1 this will call
update(-1, ...) and can break useFieldArray. Fix: guard the index by testing if
index !== -1 before calling update; if index === -1, decide and implement the
desired behavior (e.g., push the new ruleFile, ignore, or log/warn) so update is
only called with a valid field array index.
| // Track selected target labels internally for this step | ||
| const [selectedTargetLabels, setSelectedTargetLabels] = React.useState< | ||
| TargetLabel[] | ||
| >([]); |
There was a problem hiding this comment.
Internal selectedTargetLabels state not initialized from props.
The selectedTargetLabels state is initialized to an empty array, but when navigating back to this step, the previously selected target labels won't be restored. Consider initializing from the parent's state or deriving from selectedTargets:
- const [selectedTargetLabels, setSelectedTargetLabels] = React.useState<
- TargetLabel[]
- >([]);
+ const [selectedTargetLabels, setSelectedTargetLabels] = React.useState<
+ TargetLabel[]
+ >(() => {
+ // Derive initial labels from selectedTargets prop
+ return selectedTargets.flatMap((target) => target.labels ?? []);
+ });Alternatively, this could be passed as a prop from the parent if it needs to be synchronized across steps.
🤖 Prompt for AI Agents
In client/src/app/pages/applications/analysis-wizard/steps/advanced-options.tsx
around lines 135 to 138, the internal state selectedTargetLabels is always
initialized to an empty array so previously selected labels are lost when
navigating back; initialize it from the incoming prop (e.g., selectedTargets or
a prop provided by the parent) and keep it in sync by either: (1) using
useState(() => initialSelectedTargets) where initialSelectedTargets comes from
props, or (2) adding a useEffect that updates setSelectedTargetLabels when the
prop changes, or (3) accept selectedTargetLabels and setSelectedTargetLabels as
props from the parent to fully synchronize across steps. Ensure you preserve the
same shape (TargetLabel[]) when initializing and syncing.
| useFormChangeHandler({ | ||
| form, | ||
| onStateChanged, | ||
| watchFields: ["selectedTargetLabels", "selectedTargets"] as const, | ||
| mapValuesToState: ([selectedTargetLabels, selectedTargets], isValid) => ({ | ||
| selectedTargetLabels, | ||
| selectedTargets, | ||
| isValid, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
targetFilters not propagated to wizard state.
The mapValuesToState function only maps selectedTargetLabels and selectedTargets, but the form's defaultValues includes targetFilters (line 135). This means filter state won't be preserved when navigating between wizard steps. Consider including targetFilters in the state propagation:
useFormChangeHandler({
form,
onStateChanged,
- watchFields: ["selectedTargetLabels", "selectedTargets"] as const,
- mapValuesToState: ([selectedTargetLabels, selectedTargets], isValid) => ({
+ watchFields: ["selectedTargetLabels", "selectedTargets", "targetFilters"] as const,
+ mapValuesToState: ([selectedTargetLabels, selectedTargets, targetFilters], isValid) => ({
selectedTargetLabels,
selectedTargets,
+ targetFilters,
isValid,
}),
});📝 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.
| useFormChangeHandler({ | |
| form, | |
| onStateChanged, | |
| watchFields: ["selectedTargetLabels", "selectedTargets"] as const, | |
| mapValuesToState: ([selectedTargetLabels, selectedTargets], isValid) => ({ | |
| selectedTargetLabels, | |
| selectedTargets, | |
| isValid, | |
| }), | |
| }); | |
| useFormChangeHandler({ | |
| form, | |
| onStateChanged, | |
| watchFields: ["selectedTargetLabels", "selectedTargets", "targetFilters"] as const, | |
| mapValuesToState: ([selectedTargetLabels, selectedTargets, targetFilters], isValid) => ({ | |
| selectedTargetLabels, | |
| selectedTargets, | |
| targetFilters, | |
| isValid, | |
| }), | |
| }); |
🤖 Prompt for AI Agents
In client/src/app/pages/applications/analysis-wizard/steps/set-targets.tsx
around lines 141 to 150, the form change handler only propagates
selectedTargetLabels and selectedTargets so targetFilters from the form's
defaultValues isn't preserved; update useFormChangeHandler to also watch
"targetFilters" (add it to watchFields) and include targetFilters in
mapValuesToState (return { selectedTargetLabels, selectedTargets, targetFilters,
isValid }), ensuring any types/const assertions are adjusted accordingly so the
wizard state retains filter values when navigating steps.
| const createTaskGroup = async ( | ||
| taskgroupData?: New<Taskgroup> | ||
| ): Promise<Taskgroup> => { | ||
| if (!taskGroup) { | ||
| const newTaskGroup = await createTaskgroupAsync( | ||
| taskgroupData || defaultTaskgroup | ||
| ); | ||
| return newTaskGroup; | ||
| } | ||
| return taskGroup; | ||
| }; |
There was a problem hiding this comment.
Potential race condition in createTaskGroup.
If createTaskGroup is called multiple times in quick succession (e.g., rapid user clicks), the !taskGroup check on line 68 could pass for each call before the state updates from the first successful creation. This could result in multiple taskgroups being created.
Consider using a ref to track in-flight creation or returning early if a creation is already pending:
+import { useContext, useRef, useState } from "react";
export const useTaskGroupManager = () => {
const { pushNotification } = useContext(NotificationsContext);
const [taskGroup, setTaskGroup] = useState<Taskgroup | null>(null);
+ const isCreatingRef = useRef(false);
// ... mutations ...
const createTaskGroup = async (
taskgroupData?: New<Taskgroup>
): Promise<Taskgroup> => {
- if (!taskGroup) {
+ if (!taskGroup && !isCreatingRef.current) {
+ isCreatingRef.current = true;
- const newTaskGroup = await createTaskgroupAsync(
- taskgroupData || defaultTaskgroup
- );
- return newTaskGroup;
+ try {
+ const newTaskGroup = await createTaskgroupAsync(
+ taskgroupData || defaultTaskgroup
+ );
+ return newTaskGroup;
+ } finally {
+ isCreatingRef.current = false;
+ }
}
return taskGroup;
};📝 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 createTaskGroup = async ( | |
| taskgroupData?: New<Taskgroup> | |
| ): Promise<Taskgroup> => { | |
| if (!taskGroup) { | |
| const newTaskGroup = await createTaskgroupAsync( | |
| taskgroupData || defaultTaskgroup | |
| ); | |
| return newTaskGroup; | |
| } | |
| return taskGroup; | |
| }; | |
| const createTaskGroup = async ( | |
| taskgroupData?: New<Taskgroup> | |
| ): Promise<Taskgroup> => { | |
| if (!taskGroup && !isCreatingRef.current) { | |
| isCreatingRef.current = true; | |
| try { | |
| const newTaskGroup = await createTaskgroupAsync( | |
| taskgroupData || defaultTaskgroup | |
| ); | |
| return newTaskGroup; | |
| } finally { | |
| isCreatingRef.current = false; | |
| } | |
| } | |
| return taskGroup; | |
| }; |
🤖 Prompt for AI Agents
In client/src/app/pages/applications/analysis-wizard/useTaskGroupManager.ts
around lines 65 to 75, the createTaskGroup function has a race condition because
multiple concurrent calls can pass the !taskGroup check before state updates;
fix it by introducing an in-flight guard (e.g., a ref boolean isCreatingRef or
store a pending creation Promise) that you set before calling
createTaskgroupAsync and clear in a finally block, ensure subsequent calls
return the pending Promise or early-return while isCreatingRef is true, and
update taskGroup state once the single creation completes so only one taskgroup
is created.
1 participant
Refactor analysis wizard to use the wizard pattern used by the generate assets wizard.
useWizardReducerhookuseTaskGroupManagerhook is used to manage the taskgroup lifecycleuseFormChangeHandlerhook is used by each hook-form/step to handle changes to the form and pushing it up the the wizard's statePre-requisite for #2713
Summary by CodeRabbit
Release Notes
New Features
Refactor
.analyzersuffix).✏️ Tip: You can customize this high-level summary in your review settings.