✨ Add facts modal/code viewer & detail drawer tab #1057

ibolton336 · 2023-06-21T18:45:28Z

Adds initial implementation of the facts modal, analysis detail drawer tab for facts, and code viewer for facts json.

Notes: This does not cover the filter/sort logic for facts based on sources. There is no mock data available from the api for this yet, but we will be required to collate the fact sources ourselves on the UI side based on the fact keys returned from the hub. konveyor/tackle2-hub#396 for more info on how the api will look.

TODO: #1061
TODO: How should we type facts with an unknown data type?

codecov · 2023-06-21T18:53:52Z

Codecov Report

Patch coverage: 40.00% and project coverage change: -0.01 ⚠️

Comparison is base (500ca24) 47.07% compared to head (7cbf77e) 47.06%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1057      +/-   ##
==========================================
- Coverage   47.07%   47.06%   -0.01%     
==========================================
  Files         177      177              
  Lines        4412     4417       +5     
  Branches      980      983       +3     
==========================================
+ Hits         2077     2079       +2     
- Misses       2321     2324       +3     
  Partials       14       14

Flag	Coverage Δ
unitests	`47.06% <40.00%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
client/src/app/api/rest.ts	`55.01% <40.00%> (-0.24%)`	⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

mturley

Just a couple things.

mturley · 2023-06-21T19:47:28Z

client/src/app/api/models.ts

+
+export type Fact = {
+  name: string;
+  data: any;


I was going to suggest we replace any with unknown here to prevent any issues with code assuming it's a string (JSON.stringify won't care what it is), but since we're going to revisit this to support structured facts it's probably fine to leave it as-is (and it stands out as something to be fixed anyway which is maybe good)

mturley · 2023-06-21T19:51:03Z

client/src/app/pages/applications/components/application-detail-drawer/application-facts.tsx

+      {filteredFacts.map((fact) => {
+        return (
+          <div>
+            <Button


This flat list of link-buttons with no real layout aside from line breaks really bothers me for some reason, but that's a design issue. Maybe we can revisit this with Justin. I think even just a compact table with one unlabeled column, or maybe a DataList (https://www.patternfly.org/v4/components/data-list), something to make it look less plain. Outside the scope of this PR though, just thinking out loud.

Opened an issue for this & linked in the description.

mturley · 2023-06-21T19:54:46Z

...pplications/components/application-detail-drawer/fact-detail-modal/fact-code-snip-viewer.tsx

+      language={Language.json}
+      height="450px"
+      code={JSON.stringify(fact.data)}


The design calls for displaying this in YAML rather than JSON right? Since we already have the js-yaml package installed we could import yaml from 'js-yaml'; and then use yaml.dump(fact.data) instead of JSON.stringify(fact.data), and use Language.yaml instead of Language.json. I think that should be sufficient, although we may want to wrap the yaml.dump in a try-catch by lifting this code out into a variable. I suppose the API always gives us JSON and all JSON can be stringified to YAML so maybe that's fine.

Ah, looking at the docs we can avoid this ever throwing an exception by using the skipInvalid option:

code={yaml.dump(fact.data, { skipInvalid: true })}

mturley · 2023-06-21T19:56:37Z

client/src/app/queries/facts.ts

+  const { data, isLoading, error, refetch } = useQuery(
+    [FactsQueryKey, applicationID],
+    {
+      queryFn: async () => await getFacts(applicationID),


I noticed this recently in a lot of our queries (and I am guilty of copying it), but we don't actually need async/await for these queryFns if all we're doing is calling a function that already returns a promise. We may want to simplify all these at some point to:

queryFn: () => getFacts(applicationID)

however this is harmless as-is, just a nitpick.

sjd78

Really just some style questions

sjd78 · 2023-06-21T19:26:49Z

...pplications/components/application-detail-drawer/fact-detail-modal/fact-code-snip-viewer.tsx

+    <CodeEditor
+      isReadOnly
+      isDarkTheme
+      isLineNumbersVisible
+      language={Language.json}
+      height="450px"
+      code={JSON.stringify(fact.data)}
+    />


This is basically cheating! PF has some nice stuff integrated.

It took a little bit of configuring in https://github.com/konveyor/tackle2-ui/pull/966/files, but not much! now we just have it for wherever we want it

Now it would be really good to get code splitting working so the main app bundle isn't too huge.

PS - I know how to get it working, just haven't worked it up into a skinny PR yet.

We would love the help with code splitting! It's something I've never really taken the time to figure out.

sjd78 · 2023-06-21T20:13:08Z

client/src/app/api/rest.ts

+
+// Facts
+
+export const getFacts = (id: number | string | undefined) =>


The other query functions have an explicit return type Promise<?>. Does it make sense to add one here?

This one may fall under the same category as this comment. I am unsure how to type this unstructured fact that is just a json/yaml object that hasn't been parsed yet.

client/src/app/queries/facts.ts

sjd78 · 2023-06-21T20:20:42Z

client/src/app/pages/applications/components/application-detail-drawer/application-facts.tsx

+          </div>
+        );
+      })}
+      {selectedFactForDetailModal ? (


You use the form {renderCondition && (<element />)} form in other places, why ternary here?

I was trying to match the style we used in issue-affected-files-table for consistency's sake.

We're not consistent about it in this repo, but I've always tried to favor the ternary because && can fall back to falsy values that still render something (like 0). https://kentcdodds.com/blog/use-ternaries-rather-than-and-and-in-jsx

Signed-off-by: ibolton336 <ibolton@redhat.com>

mturley

LGTM assuming we revisit the Fact/UnstructuredFact types in a followup.

ibolton336 force-pushed the add-facts-modal branch 3 times, most recently from c48ad3d to d415d01 Compare June 21, 2023 18:47

ibolton336 changed the title ~~Add facts modal~~ ✨ Add facts modal & detail drawer tab for facts Jun 21, 2023

ibolton336 changed the title ~~✨ Add facts modal & detail drawer tab for facts~~ ✨ Add facts modal/code viewer & detail drawer tab Jun 21, 2023

mturley reviewed Jun 21, 2023

View reviewed changes

sjd78 reviewed Jun 21, 2023

View reviewed changes

sjd78 mentioned this pull request Jun 21, 2023

View report / task data in yaml #835

Closed

ibolton336 added 6 commits June 21, 2023 17:21

Add initial code for showing facts tab data

20425e9

Signed-off-by: ibolton336 <ibolton@redhat.com>

Fix facts api call lifecycle

1bcf1ee

Signed-off-by: ibolton336 <ibolton@redhat.com>

New facts component, add filter placeholder, fix fact TS

c1795c6

Signed-off-by: ibolton336 <ibolton@redhat.com>

Add code editor viewer for fact json

114d4f4

Signed-off-by: ibolton336 <ibolton@redhat.com>

Cleanup

c221947

Signed-off-by: ibolton336 <ibolton@redhat.com>

PR comments addressed

657f20f

Signed-off-by: ibolton336 <ibolton@redhat.com>

ibolton336 force-pushed the add-facts-modal branch from 26e084d to 657f20f Compare June 21, 2023 21:36

mturley approved these changes Jun 22, 2023

View reviewed changes

Merge branch 'main' into add-facts-modal

7cbf77e

ibolton336 merged commit 3cbebe2 into konveyor:main Jun 22, 2023

ibolton336 deleted the add-facts-modal branch June 22, 2023 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add facts modal/code viewer & detail drawer tab #1057

✨ Add facts modal/code viewer & detail drawer tab #1057

ibolton336 commented Jun 21, 2023 •

edited

Loading

codecov bot commented Jun 21, 2023 •

edited

Loading

mturley left a comment

mturley Jun 21, 2023

mturley Jun 21, 2023

ibolton336 Jun 21, 2023

mturley Jun 21, 2023 •

edited

Loading

mturley Jun 21, 2023

mturley Jun 21, 2023

sjd78 left a comment

sjd78 Jun 21, 2023

mturley Jun 21, 2023

sjd78 Jun 21, 2023

mturley Jun 22, 2023

sjd78 Jun 21, 2023

ibolton336 Jun 21, 2023

sjd78 Jun 21, 2023

ibolton336 Jun 21, 2023 •

edited

Loading

mturley Jun 21, 2023

mturley left a comment


		// Facts

		export const getFacts = (id: number \| string \| undefined) =>

✨ Add facts modal/code viewer & detail drawer tab #1057

✨ Add facts modal/code viewer & detail drawer tab #1057

Conversation

ibolton336 commented Jun 21, 2023 • edited Loading

codecov bot commented Jun 21, 2023 • edited Loading

Codecov Report

mturley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mturley Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjd78 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibolton336 Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mturley left a comment

Choose a reason for hiding this comment

ibolton336 commented Jun 21, 2023 •

edited

Loading

codecov bot commented Jun 21, 2023 •

edited

Loading

mturley Jun 21, 2023 •

edited

Loading

ibolton336 Jun 21, 2023 •

edited

Loading