- Notifications
You must be signed in to change notification settings - Fork928
feat: Add support for update checks and notifications#4810
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Changes from1 commit
cc28c1bc163af8e9ed0b764ad0f50238d5a9e1ea9e979ff80b80de3c81d11dd392d06ead38c3b1c9dbc149a7bfc44f6475File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
- Loading branch information
Uh oh!
There was an error while loading.Please reload this page.
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import { makeStyles } from "@material-ui/core/styles" | ||
| import { useActor } from "@xstate/react" | ||
| import { Loader } from "components/Loader/Loader" | ||
| import { FC, Suspense, useContext, useEffect } from "react" | ||
| import { XServiceContext } from "../../xServices/StateContext" | ||
| import { Footer } from "../Footer/Footer" | ||
| import { Navbar } from "../Navbar/Navbar" | ||
| @@ -19,20 +19,35 @@ interface AuthAndFrameProps { | ||
| export const AuthAndFrame: FC<AuthAndFrameProps> = ({ children }) => { | ||
| const styles = useStyles({}) | ||
mafredri marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| const xServices = useContext(XServiceContext) | ||
| const [authState] = useActor(xServices.authXService) | ||
| const [buildInfoState] = useActor(xServices.buildInfoXService) | ||
| const [updateCheckState, updateCheckSend] = useActor( | ||
| xServices.updateCheckXService, | ||
| ) | ||
| useEffect(() => { | ||
| if (authState.matches("signedIn")) { | ||
| updateCheckSend("CHECK") | ||
| } else { | ||
| updateCheckSend("CLEAR") | ||
| } | ||
| }, [authState, updateCheckSend]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. review(site): I'm not super happy with how this turned out. I kind of wanted to represent this in the updateCheckXService, but I couldn't figure out how to do inter-machine dependencies. This snippet here enables e.g. the following scenario:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Yeah, inter-machine dependencies are tricky; I'm not sure if there's a better way.@presleyp do you have any workarounds? I think you looked at something like this recently with pagination. | ||
| return ( | ||
| <RequireAuth> | ||
| <div className={styles.site}> | ||
| <Navbar /> | ||
| {updateCheckState.context.show && ( | ||
| <div className={styles.updateCheckBanner}> | ||
| <Margins> | ||
| <UpdateCheckBanner | ||
| updateCheck={updateCheckState.context.updateCheck} | ||
| error={updateCheckState.context.error} | ||
| onDismiss={() => updateCheckSend("DISMISS")} | ||
MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. review(site): Dismissal as any user (practically only an owner) will prevent notification from showing up until browser window is reloaded, even if logged in/out in-between. This seems like acceptable behavior (managed via XState service). | ||
| /> | ||
| </Margins> | ||
| </div> | ||
| )} | ||
| <div className={styles.siteContent}> | ||
| <Suspense fallback={<Loader />}>{children}</Suspense> | ||
| </div> | ||
| @@ -48,12 +63,15 @@ const useStyles = makeStyles((theme) => ({ | ||
| minHeight: "100vh", | ||
| flexDirection: "column", | ||
| }, | ||
| updateCheckBanner: { | ||
| // Add spacing at the top and remove some from the bottom. Removal | ||
| // is necessary to avoid a visual jerk when the banner is dismissed. | ||
| // It also give a more pleasant distance to the site content when | ||
| // the banner is visible. | ||
| marginTop: theme.spacing(2), | ||
| marginBottom: -theme.spacing(2), | ||
| }, | ||
| siteContent: { | ||
| flex: 1, | ||
| }, | ||
| })) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -5,17 +5,19 @@ import * as TypesGen from "api/typesGenerated" | ||
mafredri marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| export interface UpdateCheckBannerProps { | ||
| updateCheck?: TypesGen.UpdateCheckResponse | ||
| error?: Error | unknown | ||
| onDismiss?: () => void | ||
| } | ||
| export const UpdateCheckBanner: React.FC< | ||
mafredri marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| React.PropsWithChildren<UpdateCheckBannerProps> | ||
| > = ({ updateCheck, error, onDismiss }) => { | ||
| const { t } = useTranslation("common") | ||
| return ( | ||
| <> | ||
| {!error &&updateCheck && !updateCheck.current && ( | ||
| <AlertBanner severity="info"onDismiss={onDismiss}dismissible> | ||
| <div> | ||
| <Trans | ||
| t={t} | ||
| @@ -32,6 +34,15 @@ export const UpdateCheckBanner: React.FC< | ||
| </div> | ||
| </AlertBanner> | ||
| )} | ||
| {error && ( | ||
| <AlertBanner | ||
| severity="error" | ||
| error={error} | ||
| text={t("updateCheck.error")} | ||
| onDismiss={onDismiss} | ||
| dismissible | ||
| /> | ||
| )} | ||
| </> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Just curious why we throw an error if the check fails. Perhaps it would be better if this failed silently (we could throw something in the terminal if we wanted)? Maybe not, just a thought after seeing the error on local: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I thought about it and I worried we wouldn't notice if it stops working for whatever reason. I can make it silent or add a prefix ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Yeah, maybe we could just add a console.error since we don't have Sentry. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I refactored this now, it looks like this:https://www.chromatic.com/test?appId=624de63c6aacee003aa84340&id=6388bfa22e0a55f2ff0c1f1e I'm still game to remove it if that's preferred. 👍🏻 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. @mafredri nope looks good! | ||
| ) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,85 +1,172 @@ | ||
| import { assign, createMachine } from "xstate" | ||
| import{ checkAuthorization, getUpdateCheck } from "api/api" | ||
| import{ AuthorizationResponse, UpdateCheckResponse } from "api/typesGenerated" | ||
| export const checks = { | ||
| viewUpdateCheck: "viewUpdateCheck", | ||
| } as const | ||
| export const permissionsToCheck = { | ||
| [checks.viewUpdateCheck]: { | ||
| object: { | ||
| resource_type: "update_check", | ||
| }, | ||
| action: "read", | ||
| }, | ||
| } as const | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Same here, I thought it was weird but I kept them just in case. I'll remove them and see if something breaks 👍🏻 | ||
| export type Permissions = Record<keyof typeof permissionsToCheck, boolean> | ||
| export interface UpdateCheckContext { | ||
| show: boolean | ||
| updateCheck?: UpdateCheckResponse | ||
| permissions?: Permissions | ||
| error?: Error | unknown | ||
| } | ||
| export type UpdateCheckEvent = | ||
| | { type: "CHECK" } | ||
| | { type: "CLEAR" } | ||
| | { type: "DISMISS" } | ||
| export const updateCheckMachine = createMachine( | ||
| { | ||
| id: "updateCheckState", | ||
| predictableActionArguments: true, | ||
| tsTypes: {} as import("./updateCheckXService.typegen").Typegen0, | ||
| schema: { | ||
| context: {} as UpdateCheckContext, | ||
| events: {} as UpdateCheckEvent, | ||
| services: {} as { | ||
| checkPermissions: { | ||
| data: AuthorizationResponse | ||
| } | ||
| getUpdateCheck: { | ||
| data: UpdateCheckResponse | ||
| } | ||
| }, | ||
| }, | ||
| context: { | ||
| show: false, | ||
| }, | ||
| initial: "idle", | ||
| states: { | ||
| idle: { | ||
| on: { | ||
| CHECK: { | ||
| target: "fetchingPermissions", | ||
| }, | ||
| }, | ||
| }, | ||
| fetchingPermissions: { | ||
| invoke: { | ||
| src: "checkPermissions", | ||
| id: "checkPermissions", | ||
| onDone: [ | ||
| { | ||
| actions: ["assignPermissions"], | ||
| target: "checkingPermissions", | ||
| }, | ||
| ], | ||
| onError: [ | ||
| { | ||
| actions: ["assignError"], | ||
| target: "show", | ||
| }, | ||
| ], | ||
| }, | ||
| }, | ||
| checkingPermissions: { | ||
| always: [ | ||
| { | ||
| target: "fetchingUpdateCheck", | ||
| cond: "canViewUpdateCheck", | ||
| }, | ||
| { | ||
| target: "dismissOrClear", | ||
| cond: "canNotViewUpdateCheck", | ||
| }, | ||
| ], | ||
| }, | ||
| fetchingUpdateCheck: { | ||
| invoke: { | ||
| src: "getUpdateCheck", | ||
| id: "getUpdateCheck", | ||
| onDone: [ | ||
| { | ||
| actions: ["assignUpdateCheck", "clearError"], | ||
| target: "show", | ||
| }, | ||
| ], | ||
| onError: [ | ||
| { | ||
| actions: ["assignError", "clearUpdateCheck"], | ||
| target: "show", | ||
| }, | ||
| ], | ||
| }, | ||
| }, | ||
| show: { | ||
| entry: "assignShow", | ||
| always: [ | ||
| { | ||
| target: "dismissOrClear", | ||
| }, | ||
| ], | ||
| }, | ||
| dismissOrClear: { | ||
| on: { | ||
| DISMISS: { | ||
| actions: ["assignHide"], | ||
| target: "dismissed", | ||
| }, | ||
| CLEAR: { | ||
| actions: ["clearUpdateCheck", "clearError", "assignHide"], | ||
| target: "idle", | ||
| }, | ||
| }, | ||
| }, | ||
| dismissed: { | ||
| type: "final", | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| services: { | ||
| checkPermissions: async () => | ||
| checkAuthorization({ checks: permissionsToCheck }), | ||
| getUpdateCheck: getUpdateCheck, | ||
| }, | ||
| actions: { | ||
| assignPermissions: assign({ | ||
| permissions: (_, event) => event.data as Permissions, | ||
| }), | ||
| assignShow: assign({ | ||
| show: true, | ||
| }), | ||
| assignHide: assign({ | ||
| show: false, | ||
| }), | ||
| assignUpdateCheck: assign({ | ||
| updateCheck: (_, event) => event.data, | ||
| }), | ||
| clearUpdateCheck: assign((context) => ({ | ||
| ...context, | ||
| updateCheck: undefined, | ||
| })), | ||
| assignError: assign({ | ||
| error: (_, event) => event.data, | ||
| }), | ||
| clearError: assign((context) => ({ | ||
| ...context, | ||
| error: undefined, | ||
| })), | ||
| }, | ||
| guards: { | ||
| canViewUpdateCheck: (context) => | ||
| context.permissions?.[checks.viewUpdateCheck] || false, | ||
| canNotViewUpdateCheck: (context) => | ||
| !context.permissions?.[checks.viewUpdateCheck], | ||
| }, | ||
| }, | ||
| ) | ||