-
Notifications
You must be signed in to change notification settings - Fork 166
Onboarding Wizard (config creation via web UI) #3769
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
base: main
Are you sure you want to change the base?
Onboarding Wizard (config creation via web UI) #3769
Conversation
meowjesty
left a comment
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.
Buncha comments for the Rust side.
I wonder if the wizard should be a separate crate/sub-crate of cli instead of just a mod, it has a whole js web app, so it for sure feels it deserves a bit more than just a mod wizard_yolo.
|
meowjesty
left a comment
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.
Started looking into the frontend, so far a bunch of minor issues and things I don't really know if we need? We might need them, but I don't know, do you know?
| "noImplicitAny": false, | ||
| "noUnusedParameters": false, | ||
| "skipLibCheck": true, | ||
| "allowJs": true, | ||
| "noUnusedLocals": false, | ||
| "strictNullChecks": false | ||
| } |
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.
| "noImplicitAny": false, | |
| "noUnusedParameters": false, | |
| "skipLibCheck": true, | |
| "allowJs": true, | |
| "noUnusedLocals": false, | |
| "strictNullChecks": false | |
| } | |
| "noImplicitAny": true, | |
| "noUnusedParameters": true, | |
| "skipLibCheck": true, | |
| "allowJs": false, | |
| "noUnusedLocals": true, | |
| "strictNullChecks": true | |
| } |
Do we need both this and tsconfig.app.json?
| "strict": true, | ||
| "noUnusedLocals": false, | ||
| "noUnusedParameters": false, | ||
| "noFallthroughCasesInSwitch": 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.
| "strict": true, | |
| "noUnusedLocals": false, | |
| "noUnusedParameters": false, | |
| "noFallthroughCasesInSwitch": true | |
| }, | |
| "strict": true, | |
| "noUnusedLocals": true, | |
| "noUnusedParameters": true, | |
| "noFallthroughCasesInSwitch": true | |
| }, |
Omg, how many tsconfig files do we need? Isn't there some bundler or whatever that takes it all from one place?
| }); | ||
|
|
||
| if (error) { | ||
| console.log(error) |
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.
No notification anywhere that we have an error? I think we should have something, maybe there's something wrong with the user setup, and this might leave them confused until they randomly check the browser console.
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.
Also, this won't just display [Object object], right?
And lastly, console.error(error) (I think that's the correct one?).
| const { error, isLoading, data } = useQuery({ | ||
| queryKey: ["userIsReturning"], | ||
| queryFn: () => | ||
| fetch(window.location.href + "api/v1/is-returning").then(async (res) => |
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.
AI said to use https://developer.mozilla.org/en-US/docs/Web/API/Location/origin instead of href, cause href may or may not contain a trailling /.
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.
Can we also get a global/const json object with all the backend routes? Something like:
const ALL_THE_ROUTES = {
isReturning: "/api/v1/is-returning,
};Something along these lines, so we can have a central point to define all of this strings (muh grpc).
meowjesty
left a comment
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.
Some more small stuff (I want to refresh the page and am scared of losing some of the comments, so here they are).
| <p className="text-xl text-gray-600 mb-4">Oops! Page not found</p> | ||
| <a href="/" className="text-blue-500 hover:text-blue-700 underline"> | ||
| Return to Home | ||
| </a> | ||
| </div> |
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.
No metalbear bear funny image? Like the bear confused, searching for something that they can't find.
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.
no time for whimsy, this wizard is SERIOUS
| import HomepageReturning from "@/components/HomepageReturning"; | ||
| import HomepageNewUser from "../components/HomepageNewUser"; | ||
| import { useContext } from "react"; |
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.
Why @ for returning and ../ for new?
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.
dear lord this is cursed
meowjesty
left a comment
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.
Some more frontend comments.
We use some strings here and there as id/type-ish-like, and I wonder if we could use something else, maybe an enum or the types variant thingy that ts supports.
| @@ -0,0 +1,191 @@ | |||
| import * as React from "react" | |||
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.
Am I (color)blind, or does this file not contain a single ; at the end of its lines?
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.
Adding some formatter config fixes this (I think), and it would be nice in general, so we don't end up with formatting changes in future PRs.
|
|
||
| type ActionType = typeof actionTypes | ||
|
|
||
| type Action = |
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.
react 19 introduced something to deal with actions https://react.dev/blog/2024/12/05/react-19#actions wonder if it would be applicable here (making this comment before I checked where this gets used).
| const BoilerplateStep: () => WizardStep = () => { | ||
| const boilerplateConfigs: BoilerplateCardProps[] = [ | ||
| { | ||
| id: "steal", |
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.
Nit: These id thingies kinda look like variants, so maybe they should be represented as such? Some sort of
enum Mode {
Mirrord = "mirror",
Steal = "steal",
...
}
id: FilterMode.Steal| scale_down: boolean, | ||
| config: LayerFileConfig | ||
| ) => { | ||
| if (typeof config !== "object") { |
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.
How is this possible? I mean, how would this not be an object, since the function is taking a LayerFileConfig?
| }, | ||
| ]; | ||
|
|
||
| const { config, setConfig } = useContext(ConfigDataContext); |
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'm seeing config and setConfig in vscode as any, is that right, or it's just my setup?
| const [selectedBoilerplate, setSelectedBoilerplate] = useState<string>(""); | ||
| const handleBoilerplateSelect = (boilerplateId: string) => { | ||
| setSelectedBoilerplate(boilerplateId); | ||
| if (boilerplateId === "replace") { |
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.
Would be nice if these things were not just strings we have to type and match correctly, such as enum or maybe just one of those ts variant types mirror | steal | replace that gives a compilation error if you have a typo, since they're a match on a sort of type.
| id: "boilerplate", | ||
| title: "mirrord configuration", | ||
| content: ( | ||
| <div className="space-y-6"> |
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.
kevin
| const ConfigTabs = () => { | ||
| const { config } = useContext(ConfigDataContext); | ||
| const [currentTab, setCurrentTab] = useState<string>("target"); | ||
| const [savedIncoming, setSavedIncoming] = useState<any>(readIncoming(config)); |
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.
any? 😿
| }; | ||
|
|
||
| const nextTab = () => { | ||
| if (currentTab === "target") setCurrentTab("network"); |
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.
Who doesn't love having a bunch of strings control navigation!
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 it feasible to have these tab strings as variants? Maybe setCurrentTab gets some sort of Tab enum thingy?
meowjesty
left a comment
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.
Couple more small stuff.
| import { updateConfigCopyTarget, updateConfigMode } from "../JsonUtils"; | ||
|
|
||
| export interface BoilerplateCardProps { | ||
| id: 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.
Oh, I guess the enum or the variant typescritiroo thingy would go here, like:
type IdThingy = "steal" | "mirror" | "replace";
id: IdThingySet http_filter ports iff filters are set
…or to resource listing
c35c511 to
6ce0461
Compare
d3d7caf to
72e8a2a
Compare
72e8a2a to
32f517b
Compare
f2ca4cd to
894d39a
Compare
eca3897 to
e519bef
Compare
Main issue: COR-986 (see sub-issues for task lists)
Relevant Slack channel:
#project-mirrord-wizardNotion page: MBE-1451 mirrord onboarding wizard
Unfinished:
Backend endpoints - cluster and target detailsDev-facing docs