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

feat(webapp): Add settings/apps page #1424

Merged
merged 9 commits into from
Aug 24, 2022
Merged

Conversation

StasDachinsky
Copy link
Contributor

@StasDachinsky StasDachinsky commented Aug 18, 2022

Brief

Changes

  • added settings/apps page
  • small types refactor in settings reducer
  • added spinners on apps loading and deletion
  • added confirmation input into app deletion modal
  • added warning message

Concerns

@StasDachinsky StasDachinsky requested review from eh-am, kolesnikovae and shaleynikov and removed request for kolesnikovae August 18, 2022 16:47
@StasDachinsky StasDachinsky force-pushed the feat/apps-page-frontend branch from 3613486 to 85eb9b4 Compare August 18, 2022 16:47
@StasDachinsky StasDachinsky force-pushed the feat/apps-page-frontend branch from 85eb9b4 to f13d6b5 Compare August 18, 2022 16:53
@eh-am
Copy link
Collaborator

eh-am commented Aug 18, 2022

/create-server

@eh-am
Copy link
Collaborator

eh-am commented Aug 18, 2022

Will review more in depth later, but right now:
1 - #1416 (comment)
2 - Delete should only be available for pyroscope instances behind auth

name="Search app input"
/>
</div>
<table
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem to be using the table component #1403

Copy link
Contributor Author

@StasDachinsky StasDachinsky Aug 18, 2022

Choose a reason for hiding this comment

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

my bad, fixed in the next commit

async (app: App, thunkAPI) => {
const res = await deleteAppAPI({ name: app.name });

// eslint-disable-next-line @typescript-eslint/no-floating-promises
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's true we do have this rule, but I wonder why it doesn't trigger in other places like here https://github.com/pyroscope-io/pyroscope/blob/main/webapp/javascript/redux/reducers/continuous.ts#L230

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it does, but warnings only

) : (
<tr>
<td className={appsStyles.appsTableEmptyMessage} colSpan={7}>
The list is empty
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are distinguishing between an empty list (ie no apps) vs there's no apps bc it's a loading state?

Copy link
Contributor Author

@StasDachinsky StasDachinsky Aug 18, 2022

Choose a reason for hiding this comment

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

Rn it's just an empty list message while data is loading, do we have some kinda spinner component for it? If so, we need to update user list too

Comment on lines +27 to +30
const response = await request(`/api/apps`, {
method: 'DELETE',
body: JSON.stringify({ name }),
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can take a long time (there are instances with 15+ minutes). How much is the timeout currently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are using fetch for request, default timeouts is 300s in Chrome and 90s in Firefox and looks like there is no way to increase this values. Possible fix may be switching to axios or similar lib

@github-actions
Copy link
Contributor

github-actions bot commented Aug 18, 2022

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
webapp/public/assets/app.js 418.12 KB (+0.35% 🔺) 8.4 s (+0.35% 🔺) 3.7 s (-12.4% 🔽) 12.1 s
webapp/public/assets/app.css 15.4 KB (+1.12% 🔺) 308 ms (+1.12% 🔺) 0 ms (+100% 🔺) 308 ms
webapp/public/assets/styles.css 9.46 KB (0%) 190 ms (0%) 0 ms (+100% 🔺) 190 ms
packages/pyroscope-flamegraph/dist/index.js 91.63 KB (+0.94% 🔺) 1.9 s (+0.94% 🔺) 1.4 s (-35.99% 🔽) 3.2 s
packages/pyroscope-flamegraph/dist/index.node.js 91.46 KB (+0.91% 🔺) 1.9 s (+0.91% 🔺) 494 ms (-23.21% 🔽) 2.4 s
packages/pyroscope-flamegraph/dist/index.css 7.05 KB (+0.45% 🔺) 141 ms (+0.45% 🔺) 0 ms (+100% 🔺) 141 ms

@codecov
Copy link

codecov bot commented Aug 18, 2022

Codecov Report

Merging #1424 (36ad635) into main (a11a068) will increase coverage by 0.14%.
The diff coverage is 83.34%.

@@            Coverage Diff             @@
##             main    #1424      +/-   ##
==========================================
+ Coverage   68.37%   68.51%   +0.14%     
==========================================
  Files         129      129              
  Lines        4252     4248       -4     
  Branches     1155     1138      -17     
==========================================
+ Hits         2907     2910       +3     
+ Misses       1340     1333       -7     
  Partials        5        5              
Impacted Files Coverage Δ
webapp/javascript/ui/Modals/Modal.module.css 61.54% <ø> (ø)
webapp/javascript/ui/Table.module.scss 61.54% <ø> (ø)
webapp/javascript/ui/Table.tsx 86.57% <50.00%> (+0.42%) ⬆️
webapp/javascript/ui/LoadingSpinner.tsx 100.00% <100.00%> (ø)
webapp/javascript/ui/Modals/index.ts 39.14% <100.00%> (ø)
webapp/javascript/components/Sidebar.tsx 86.91% <0.00%> (ø)
packages/pyroscope-models/src/profile.ts 94.12% <0.00%> (+1.81%) ⬆️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@StasDachinsky StasDachinsky force-pushed the feat/apps-page-frontend branch from cdfb495 to cee6097 Compare August 19, 2022 08:40

useEffect(() => {
dispatch(reloadApps());
});
Copy link
Member

Choose a reason for hiding this comment

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

I tried running this and currently it gets into an infinite loop. I think maybe there needs to be an empty array here as the second argument or something like that (not a big expert in react so I'm not sure)

Screen Shot 2022-08-19 at 2 58 19 AM

Copy link
Member

Choose a reason for hiding this comment

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

Addressed in 66cc28d

import { getAppTableRows } from './getAppTableRows';

const headRow = [
{ name: '', label: '', sortable: 0 },
Copy link
Member

Choose a reason for hiding this comment

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

I think this one is not needed. E.g look at this screenshot, "Name" is too far to the right.
Screen Shot 2022-08-19 at 3 00 57 AM

It looks like there are 2 td elements per tr, so it would make sense to have 2 for th as well.

Copy link
Member

Choose a reason for hiding this comment

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

Addressed in 66cc28d

@eh-am
Copy link
Collaborator

eh-am commented Aug 19, 2022

/create-server

@eh-am
Copy link
Collaborator

eh-am commented Aug 19, 2022

Looks good! Have a couple thoughts:

  1. There's hover effect, but no actual action when clicking, which is kinda misleading IMO.
Kapture.2022-08-19.at.12.33.40.mp4
  1. It it possible to follow what we do in the CLI (Add apps page to the frontend #1416 (comment)), and a) add more info on what's going to happen when deleting (ie it will take lots of time) and b) add a confirmation that requires retyping the app name (not super sold on this if it's too difficult)

For a) That message needs to be overwriten, since behaviour will change in the cloud

  1. Which is similar to 2, but it's important to in the popup to show the app name you are deleting, since this is a destructive action.
    image

@Rperry2174
Copy link
Contributor

image

Lets add the actual app name that they're deleting to this message

.popup label {
color: var(--ps-neutral-2);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this style because it hide inputLabel in modal window and looks like we are not using inputLabel anywhere else

@StasDachinsky
Copy link
Contributor Author

Screen.Recording.2022-08-22.at.14.04.23.mov

.then(() => {
setAppsInProcessing(appsInProcessing.filter((x) => x !== app.name));
setDeletedApps([...deletedApps, app.name]);
// eslint-disable-next-line @typescript-eslint/no-floating-promises
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's better to move same rules to top of the file (line 48, 54 etc.)?

/* eslint-disable @typescript-eslint/no-floating-promises */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -0,0 +1,10 @@
import { z } from 'zod';

export const appModel = z.object({
Copy link
Contributor

Choose a reason for hiding this comment

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

@eh-am should we use uppercase in zod entities?

Copy link
Contributor

Choose a reason for hiding this comment

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

like appModel => AppModel

Copy link
Contributor

Choose a reason for hiding this comment

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

or AppSchema

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made names here like in this users.ts file

type: 'pristine' | 'loading' | 'loaded' | 'failed';
data?: Users;
};
enum FetchStatus {
Copy link
Contributor

Choose a reason for hiding this comment

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

enum keys and values are the same, maybe it's better to use Numeric enums, instead of String ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, but we are using the same strings statuses in other reducers and I think we should refactor them all in the other task

@@ -91,9 +93,14 @@ function Table({
table,
tableBodyRef,
className,
isLoading,
Copy link
Contributor

Choose a reason for hiding this comment

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

You've added loading state for all tables in the app, @eh-am does it work for web app?
image


import styles from './AppTableItem.module.css';

interface IDeleteButtorProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

we dont use I prefix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

function confirmDelete({
objectName,
objectType,
object,
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use objectName prop instead of object ? since they have same string type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

const isLoading = useAppSelector(selectIsLoadingApps);
const [search, setSearchField] = useState('');
const [appsInProcessing, setAppsInProcessing] = useState([] as string[]);
const [deletedApps, setDeletedApps] = useState([] as string[]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const [deletedApps, setDeletedApps] = useState([] as string[]);
const [deletedApps, setDeletedApps] = useState<string[]>([]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -0,0 +1,10 @@
import { z } from 'zod';

export const appModel = z.object({
Copy link
Contributor

Choose a reason for hiding this comment

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

or AppSchema

@eh-am
Copy link
Collaborator

eh-am commented Aug 22, 2022

/create-server

@petethepig petethepig self-requested a review August 24, 2022 02:39
Copy link
Member

@petethepig petethepig left a comment

Choose a reason for hiding this comment

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

LGTM

@StasDachinsky
Copy link
Contributor Author

Last version with changes requested by @Rperry2174

Screen.Recording.2022-08-24.at.17.29.32.mov

@Rperry2174 Rperry2174 merged commit f87ce69 into main Aug 24, 2022
@Rperry2174 Rperry2174 deleted the feat/apps-page-frontend branch August 24, 2022 15:47
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.

6 participants