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.

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

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

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

Conversation

yael-spinner
Copy link

@yael-spinner yael-spinner commented Sep 22, 2024

This update introduces a function that changes the default state based on the type of application being deployed, ensuring optimal configuration for different scenarios while maintaining backward compatibility.
Related issue: #1837
Related enhancement PR: #209

Signed-off-by: yael-spinner <yaels83191@gmail.com>
Signed-off-by: yael-spinner <yaels83191@gmail.com>
Signed-off-by: yael-spinner <yaels83191@gmail.com>
@yael-spinner yael-spinner marked this pull request as ready for review October 9, 2024 11:56
Signed-off-by: yael-spinner <yaels83191@gmail.com>
Signed-off-by: yael-spinner <yaels83191@gmail.com>
Copy link

codecov bot commented Oct 29, 2024

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Project coverage is 42.08%. Comparing base (b654645) to head (f549ed7).
Report is 241 commits behind head on main.

Files with missing lines Patch % Lines
...s/applications/analysis-wizard/analysis-wizard.tsx 84.61% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2106      +/-   ##
==========================================
+ Coverage   39.20%   42.08%   +2.88%     
==========================================
  Files         146      175      +29     
  Lines        4857     5643     +786     
  Branches     1164     1403     +239     
==========================================
+ Hits         1904     2375     +471     
- Misses       2939     3147     +208     
- Partials       14      121     +107     
Flag Coverage Δ
client 42.08% <84.61%> (+2.88%) ⬆️
server ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@rszwajko rszwajko left a comment

Choose a reason for hiding this comment

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

Please check my questions below.

Copy link
Collaborator

@rszwajko rszwajko left a comment

Choose a reason for hiding this comment

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

Thanks for providing the context for the changes. I have some more comments (see below). I need also verify how useEffect behaves in this place - will comment here when I'm done.

Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

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

Looks on the right track, but it can be paired down a bit and still work properly.

@@ -163,10 +194,9 @@ export const AnalysisWizard: React.FC<IAnalysisWizard> = ({
const methods = useForm<AnalysisWizardFormValues>({
defaultValues: {
artifact: null,
mode: "source-code-deps",
mode: determineMode(applications),
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

Comment on lines 65 to 72
// 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.

shirael and others added 2 commits October 31, 2024 12:36
Signed-off-by: shirael <shira22423@gmail.com>
Arrange the notes on the change default mode
Copy link
Author

@yael-spinner yael-spinner left a comment

Choose a reason for hiding this comment

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

I updated the code according to the previous requests and comments, If there are any questions, I'm available for clarification."

Copy link
Collaborator

@rszwajko rszwajko left a comment

Choose a reason for hiding this comment

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

Looks good! Consider the comment below as nice-to-have but not mandatory. It would be also nice to change the title of this PR to better reflect the change and remove hyphens between words.

Signed-off-by: yael-spinner <yaels83191@gmail.com>
@yael-spinner yael-spinner changed the title ✨ Change-the-default-mode ✨ Fix Divergent Branch Issue and Update Default Mode Logic Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants