-
Notifications
You must be signed in to change notification settings - Fork 0
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
/state and /history pages #1
Conversation
@@ -1,6 +1,5 @@ | |||
import { useRouteLoaderData } from "@remix-run/react"; | |||
import clsx from "clsx"; | |||
import Ajv from "ajv"; |
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.
Ripped out AJV. Not going to do much / any client side validation as long as the Evolver-NG continues to respond with sane validation error messaging.
@@ -1,6 +1,6 @@ | |||
import { expect, describe, it } from "vitest"; | |||
import { loader, action } from "~/root"; | |||
import { prefs } from "~/prefs-cookie"; | |||
import { userPrefs } from "~/cookies.server"; |
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.
More descriptive naming of the purpose of this cookie (reight now it persists their selected theme between page refresh / visits), .server
tells remix framework this code will only run on the server.
app/components/ErrorNotifs.tsx
Outdated
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.
not going to hand role notifications, using react-toastify
package instead.
app/components/EvolverTree.tsx
Outdated
@@ -0,0 +1,72 @@ | |||
import { |
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.
Stub for an experiment in having a file-tree representation of the Evolver config. Not being used anywhere. happy to delete if YAGNI proponents amongst us.
@@ -4,11 +4,11 @@ import clsx from "clsx"; | |||
|
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.
just style tweaks in this file.
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.
Looks good, just a few comments, let me know what you think
const { evolverState } = useLoaderData<typeof loader>(); | ||
return ( | ||
<div> | ||
<VialGrid stateData={evolverState.state} ip={ip_addr} /> |
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 vialCount could technically come from the evolver config, do you have access to such via global state or something?
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.
For complete context, goal is to have a view of the evolver with active vial slots, displayed in their index positions relative to inactive slots.
Right now I cant use the config to infer, vialCount, or the general layout, irl of the vials. For example - for an evolver with hardware running on vials 0,2, all i can figure out from the config is that there are 2 vials, and possibly a "slot" at index 1, that's empty.
What i'd like to do is represent all slots on a grid that matches the hardware layout. To do this the config would have to include some properties it doesn't currently have, at a minimum a "max_index", or even better x,y dimensions from the grid.
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.
At present, the config does contain something that is meant to give the overall physical setup, but it is presently just a flat list of vial numbers. So in your above case, if that list was [0,1,2]
then you could know index 1
was empty, but if it was [0,2]
you could conclude it just doesn't exist. This is vials
property at root level of config. But yeah, it would lack information about the physical location on the box. For that we would need some other structures or a way to represent null slots, something like
[[0, None, None, None],
[None, 1, None, None],
...
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.
When someone is looking in the ui at data for vial identified as "vial 5", by the config response. We must have some way for them to know that it's the vial on the second row, in the second position, that's what the 5
refers to.
[
[0,1,2,3],
[4,5 <- this one ,6,7],
[8,9,10,11],
[12,13,14,15]
]
right now the vials
top level property for the evolver in your office is:
[0,5,10,15]
Which is not enough info to make the required association. Must have max_index and dimensions.
All that to say - yeah - 2d array makes sense to me. Just in case it's not a typo, in your example, replace the 1 with 5:
[[0, None, None, None],
[None, 5, None, None],
...
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 can see this needs more in the back to support the front better and some decisions to make there, so I think what you have works for now, I've created ssec-jhu/evolver-ng#153 to track this.
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.
yeah makes sense. I can definitely lift the 16
default into a global variable to encapsulate it. If I come across similar things I'll also hoist them - altogether to make sure it's clear the front end is making assumptions about the state of the world outside of what the ng service is describing.
app/routes/devices.$ip_addr.tsx
Outdated
export async function loader({ params }: LoaderFunctionArgs) { | ||
const { ip_addr } = params; | ||
const evolverClient = createClient({ | ||
baseUrl: `http://${ip_addr}:${ENV.DEFAULT_DEVICE_PORT}`, |
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 read this as the device identifier is ip_addr
, but wondering if this might be rather {ip_addr}:{port}
, since the port served by another device could be different from the default.
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.
yeah that's a good point, or a hash of ip, port and the name... Ultimately it's the name of the evolver not really its address that matters. Later evolvers could be at urls too. I'll have a think about refactoring this.
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.
cool - can do in a separate issue, of course
path: "/", | ||
sameSite: "lax", | ||
secrets: ["s3cret1"], | ||
secure: true, |
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.
is this right? I suspect most of the time we will not have a secure connection since it will be local-only network access.
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.
interesting, this wasn't a problem during dev, looks like the clients (browsers) treat localhost as a special case:
https://stackoverflow.com/questions/65006250/secure-cookie-on-http-localhost
I think this'll be ok for now. If this is ever hosted as web app it's unlikely it will be without SSL
app/utils/getSensorProperty.ts
Outdated
if (forVials.length === 0) { | ||
vialValues.set(key, values[key][targetProperty]); | ||
} | ||
// only show the vials in forVials | ||
else if (forVials.includes(key)) { | ||
vialValues.set(key, values[key][targetProperty]); | ||
} |
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.
maybe combine the conditions? if (forVials.length === 0 || forVials.includes(key)) {...
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.
yeah good idea, I'll also revise some of this stuff to use the new history api features you've included.
afc2b9d
to
5be7e7a
Compare
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.
Looks good, I just have a few comments inline, thanks!
app/components/VialGrid.tsx
Outdated
let renderSubKey = true; | ||
switch (subKey) { | ||
case "vial": | ||
case "name": | ||
renderSubKey = false; | ||
break; | ||
|
||
default: | ||
break; | ||
} |
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.
could this be simplified to:
let renderSubKey = !["vial", "name"].includes(subKey)
and possibly even store a global constant for the exclusions list.
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.
yep adding it as a global.
}; | ||
|
||
const UpdateDeviceIntentEnum = z.enum(["update_evolver"], { | ||
required_error: "an intent is required", |
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.
Could you maybe add a comment on what intent is in this case? maybe also a little description of the workflow of this in a comment up here?
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.
Added a comment. for completeness, this article describes the pattern here:
https://sergiodxa.com/articles/multiple-forms-per-route-in-remix
} | ||
return redirect(`/devices/${id}/config?mode=view`); | ||
} catch (error) { | ||
return submission.reply({ formErrors: ["unable to update device"] }); |
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 think when we get here its not because of json errors since that'll return above, so at this point something unexpected happened and it would probably be useful to have the http error code and any text that came with it in the error message.
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.
Ok will do.
|
||
return ( | ||
<div> | ||
<HardwareTable |
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.
Is this right to be HardwareTable
?
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.
yeah copy-pasta this is just a stub, no controller response available just yet. Will remove.
breadcrumb: ({ | ||
params, | ||
}: { | ||
params: { id: string; hardware_name: string }; |
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.
maybe hardware_name
is not right here?
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.
Yeah will remove that param isn't in the path and isn't used in the breadcrumb.
app/utils/handleFileUpload.ts
Outdated
@@ -0,0 +1,20 @@ | |||
import { EvolverConfigWithoutDefaults } from "client"; |
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.
handleFileUpload
sounds maybe too generic. Might change to handleEvolverConfigFileUpload
or the like
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 made this more generic, ht "evolver config" is now a generic type on the fileUpload helper.
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.
import React from "react";
export function handleFileUpload<T>({
e,
setData,
}: {
e: React.ChangeEvent<HTMLInputElement>;
setData: React.Dispatch<React.SetStateAction<T>>;
}) {
const fileReader = new FileReader();
if (e.target.files?.[0]) {
fileReader.readAsText(e.target.files[0], "UTF-8");
fileReader.onload = (event) => {
const uploadedConfig = JSON.parse(event.target?.result as string);
setData(uploadedConfig);
};
}
}
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.
Nice, thanks!!
Adds vial state overview view:
Adds charts for hardware history, filterable by vial and hardware property.