-
-
Notifications
You must be signed in to change notification settings - Fork 345
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
reconcile boot and admin pages further #963
Conversation
Deployed as https://grist-paulfitz-smoosh.fly.dev (until 2024-06-01T20:59:50.109Z) |
Deployed as https://grist-paulfitz-smoosh.fly.dev (until 2024-06-07T17:38:26.512Z) |
Deployed as https://grist-paulfitz-smoosh.fly.dev (until 2024-06-07T19:33:43.669Z) |
Deployed as https://grist-paulfitz-smoosh.fly.dev (until 2024-06-08T01:56:06.616Z) |
Deployed as https://grist-paulfitz-smoosh.fly.dev (until 2024-06-08T21:37:29.757Z) |
Deployed as https://grist-paulfitz-smoosh.fly.dev (until 2024-06-08T21:41:47.286Z) |
Deployed as https://grist-paulfitz-smoosh.fly.dev (until 2024-06-12T21:00:36.329Z) |
Deployed as https://grist-paulfitz-smoosh.fly.dev (until 2024-06-12T21:18:53.365Z) |
Deployed as https://grist-paulfitz-smoosh.fly.dev (until 2024-06-12T21:36:57.187Z) |
Deployed as https://grist-paulfitz-smoosh.fly.dev (until 2024-06-12T21:56:23.056Z) |
Deployed as https://grist-paulfitz-smoosh.fly.dev (until 2024-06-12T22:21:18.125Z) |
Deployed as https://grist-paulfitz-smoosh.fly.dev (until 2024-06-13T18:18:32.163Z) |
Deployed as https://grist-paulfitz-smoosh.fly.dev (until 2024-06-14T20:18:21.394Z) |
Deployed as https://grist-paulfitz-smoosh.fly.dev (until 2024-06-15T17:59:54.147Z) |
Deployed as https://grist-paulfitz-smoosh.fly.dev (until 2024-06-15T18:00:41.943Z) |
Deployed as https://grist-paulfitz-smoosh.fly.dev (until 2024-06-15T18:16:42.039Z) |
app/client/ui/AdminPanel.ts
Outdated
if (result.success === undefined) { return '―'; } | ||
if (result.success) { return '✅'; } | ||
if (result.severity === 'warning') { return '❗'; } | ||
if (result.severity === 'hmm') { return '?'; } | ||
// remaining case is a fault. | ||
return '❌'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's worth flipping severity and success around so severity comes first, in case we make a mistake on the backend and return success + warning, for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I sat down to do this, but I'm not sure if it is right. It could be that the concept of severity is only really relevant if a check fails? To determine whether the failure is an error or a warning or a "hmm" (meaning ~ "debug" I guess). If the check succeeds, is the severity important?
It could be relevant if we were filtering results by severity, so we'd want to see e.g. only things-that-would-be-errors-if-they-failed.
Concretely: if a test with severity warning
succeeds, what icon should we show?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can choose what {success: true, severity: "warning"}
means - is it fine or is it a bug, and if fine what should be rendered. I think from your q you'd see it as a bug @Spoffy ?
…tion is undefined
… recognize it! Co-authored-by: George Gevoian <[email protected]>
Co-authored-by: George Gevoian <[email protected]>
Deployed as https://grist-paulfitz-smoosh.fly.dev (until 2024-06-15T19:34:55.380Z) |
--inspect-brk is useful if you want to debug something during startup
This self-test isn't perfect because we're running it from the backend instead of the frontend. Conceivably the backend might have trouble resolving its own url. Eventually we should move this test or something like it to something that executes in the frontend.
Deployed as https://grist-paulfitz-smoosh.fly.dev (until 2024-06-16T13:23:38.607Z) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in Authorization.ts
look good! I added some comments about reducing the risk of misuse.
Deployed as https://grist-paulfitz-smoosh.fly.dev (until 2024-06-22T19:00:25.482Z) |
Deployed as https://grist-paulfitz-smoosh.fly.dev (until 2024-06-22T19:06:07.986Z) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! All looks good on Authorizer side and with the boot-key.
I worked through our code review checklist. Weakest point seems mobile, the admin panel generally is a bit clunky though functional on mobile. This PR doesn't improve it or make it worse as far as I can see. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @paulfitz!
Making one more change after discussion with @Spoffy - Replacing |
Deployed as https://grist-paulfitz-smoosh.fly.dev (until 2024-06-22T20:24:56.453Z) |
This adds some remaining parts of the boot page to the admin panel, including the fallback mechanism for accessing it. It then removes the boot page.
One common failure mode for Grist is for the home url to be wrong. That is pretty catastrophic for the front end, since it ends up unable to access the API. The boot page avoided the problem by having dedicated relative api endpoints, but this won't work for the admin panel, since developers of the admin panel have been using regular api mechanisms, and not allowing that would slow work down. But still, it would be sad if the admin panel were unavailable just when you needed it. So this PR uses a distinct algorithm for picking the base url for the api that should be more robust and also fine to use outside of the context of document access.
The boot page arranged also for static assets to be available at a more robust base url. I've abandoned that since it seemed tricky to graft across to the admin page. That means there will be no diagnostic UI available if static asset urls are messed up. This is a less common problem in single-server setups I think? Could revisit if it proves an achilles heel.
If visited without admin privileges, the admin page now gives some basic tips in case the user is actually a flailing admin. The tips include a fallback auth mechanism to use when all else fails.