Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

✨ Fix Divergent Branch Issue and Update Default Mode Logic #2106

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

✨ Fix Divergent Branch Issue and Update Default Mode Logic #2106

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
47fdc9a
Change-the-default-mode
yael-spinner Sep 22, 2024
9a1e1fe
finish change the default mode
yael-spinner Oct 8, 2024
fcd823f
successful change the default mode
yael-spinner Oct 9, 2024
ed501e1
Merge branch 'main' into s_y_change_deafault_mode
yael-spinner Oct 9, 2024
ab92d33
Final update to default mode
yael-spinner Oct 10, 2024
84fd2cb
Merge branch 's_y_change_deafault_mode' of https://github.com/yael-sp…
yael-spinner Oct 10, 2024
cb65e37
Final update on default mode change before upload
yael-spinner Oct 27, 2024
4b7eecf
Arrange the notes on the change default mode
shirael Oct 31, 2024
3b430cc
Merge pull request #1 from shirael/s_y_change_deafault_mode
yael-spinner Oct 31, 2024
78957e2
Merge branch 'main' into s_y_change_deafault_mode
rszwajko Oct 31, 2024
f549ed7
Completely final update by changing the default state before uploading
yael-spinner Nov 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 46 additions & 3 deletions client/src/app/pages/applications/analysis-wizard/analysis-wizard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import * as React from "react";
import React, { useEffect } from "react";
import { useIsMutating } from "@tanstack/react-query";
import { FormProvider, useForm } from "react-hook-form";
import {
Expand Down Expand Up @@ -51,6 +51,37 @@ interface IAnalysisWizard {
isOpen: boolean;
}

const determineMode = (
applications: Application[]
):
| "binary"
| "source-code-deps"
| "source-code"
| "binary-upload"
| undefined => {
// If only one application is selected
if (applications.length === 1) {
const { repository, binary } = applications[0];
rszwajko marked this conversation as resolved.
Show resolved Hide resolved
// If the application has a repository or both repository and binary definitions, return "source-code-deps"
// If the application has only binary definitions, return "binary"
// If no definitions are present, return undefined
return repository || (repository && binary)
? "source-code-deps"
: binary && binary !== ""
? "binary"
: undefined;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. Just put it in a function, iterate through the applications array and get the mode for each application. At the end either all of the modes are the same, or they are different. Same = return that mode, different = return undefined.

// Check if all selected applications have only binary definitions
const allBinary = applications.every((app) => app.binary);
// Check if all selected applications have repository or binary definitions
rszwajko marked this conversation as resolved.
Show resolved Hide resolved
const allSourceDeps = applications.every(
(app) => app.repository || app.binary
);
// If all applications are binary, return "binary"
// If all applications are repository or a mix of repository and binary, return "source-code-deps"
// If neither condition is met, return undefined
return allBinary ? "binary" : allSourceDeps ? "source-code-deps" : undefined;
};
const defaultTaskData: TaskData = {
tagger: {
enabled: true,
Expand Down Expand Up @@ -163,10 +194,9 @@ export const AnalysisWizard: React.FC<IAnalysisWizard> = ({
const methods = useForm<AnalysisWizardFormValues>({
defaultValues: {
artifact: null,
mode: "source-code-deps",
mode: determineMode(applications),
formLabels: [],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
mode: determineMode(applications),
mode: determineMode(applications) ?? "source-code-deps",

That way, if a mode can't be figured out from the selected apps, the fallback/default mode is the original default.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sjd78 unfortunately @yael-spinner hit a real bug - the AnalysisWizard gets re-rendered with every change on the applications screen which means that the defaults are being set at start time when no apps are selected.
So useEffect is necessary here as a workaround. The good news is that using (our old friend) key property we can force re-creating the wizard when selected apps change. The following fix worked for me:

iff --git a/client/src/app/pages/applications/applications-table/applications-table.tsx b/client/src/app/pages/applications/applications-table/applications-table.tsx
index d102d69a..d4affbb4 100644
--- a/client/src/app/pages/applications/applications-table/applications-table.tsx
+++ b/client/src/app/pages/applications/applications-table/applications-table.tsx
@@ -1060,6 +1060,7 @@ export const ApplicationsTable: React.FC = () => {
 
         <TaskGroupProvider>
           <AnalysisWizard
+            key={selectedRows.map(({ name }) => name).join()}
             applications={selectedRows}
             isOpen={isAnalyzeModalOpen}
             onClose={() => {
(END)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right! I forgot about the order state gets loaded up in modals like this. Using the key prop does make sense.

For some reference: https://react.dev/learn/preserving-and-resetting-state#resetting-a-form-with-a-key

So use the key attribute and drop useEffect()?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sjd78 @rszwajko I changed the code according to your comments

selectedTargets: [],
// defaults will be passed as initialFilterValues to the table hook
targetFilters: undefined,
rszwajko marked this conversation as resolved.
Show resolved Hide resolved
selectedSourceLabels: [],
withKnownLibs: "app",
Expand All @@ -188,6 +218,19 @@ export const AnalysisWizard: React.FC<IAnalysisWizard> = ({
mode: "all",
});

useEffect(() => {
const mode = determineMode(applications);
rszwajko marked this conversation as resolved.
Show resolved Hide resolved

// Check if the mode is undefined
if (mode) {
methods.setValue("mode", mode); // Update the mode value in the form
} else {
// If the mode is undefined, you can decide what to do
// For example: set a default value or do nothing
methods.setValue("mode", "source-code-deps"); // Here you can replace it with your default value
}
}, [applications]); // Trigger the effect when `applications` changes

const { handleSubmit, watch, reset } = methods;
const values = watch();

Expand Down
4 changes: 3 additions & 1 deletion client/src/app/pages/applications/analysis-wizard/set-mode.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,9 @@ export const SetMode: React.FC<ISetMode> = ({ isSingleApp, isModeValid }) => {
toggleAriaLabel="Analysis mode dropdown toggle"
aria-label={name}
value={value}
onChange={onChange}
onChange={(value) => {
rszwajko marked this conversation as resolved.
Show resolved Hide resolved
onChange(value); // Update the value in the field
}}
options={options}
/>
)}
Expand Down
Loading