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

Dashboard: microapp v2 #999

Merged
merged 32 commits into from
Sep 5, 2024
Merged

Dashboard: microapp v2 #999

merged 32 commits into from
Sep 5, 2024

Conversation

koonpeng
Copy link
Collaborator

@koonpeng koonpeng commented Aug 16, 2024

These are changes to the "microapp" architecture to make it more modular. The goal is to eventually combine react-components and the current dashboard and make it into a framework from which new dashboards can be easily created.

Configurable Microapps

Previously microapp is a "one and done" thing where there is no way to customize them. Now microapps can have customization options via a factory method.

foo.tsx

export interface FooProps {
  greeting: string
}

export default function Foo({ greeting }: FooProps) {
  return <div>{greeting}</div>
}

foo-app.ts

export default function createFooApp(config: FooProps) {
  return createMicroApp('foo', 'Foo', () => import('./foo'), (settings) => config);
}

The fourth argument to createMicroApp is a callback that returns the props that will be passed to the component when rendering. The map app is one example where the pattern is used.

MicroApp with Settings

On top of configurable micro apps, they can now also store runtime settings, in the props callback in createMicroApp, it receives a settings object, which has a microAppSettings: { [k: string]: unknown } field. Micro apps can store their settings there under their appId.

Lazy loaded Microapps

All microapps are now lazy loaded, notice above that when creating the foo app, the Foo component is loaded via a dynamic import. This allows vite to code split the micro apps and load only the microapps that are currently being used in a workspace.

Currently this results in reduction of the largest bundle from ~3mb to ~1.5mb.

Deferred Contexts

Normally contexts must always have a default value, in some cases, we may have a context that must always be available but it is not possible to set a default value. Previously the solution is to use null as the default value but through the blessing and curse of typescript, it means that we must have a non null assertion everywhere we use it. RmfIngress is one such example where it creates a lot of boilerplate.

Deferred contexts is a convenient wrapper that does the assertions. It will throw an error if no value is provided to the context yet.

export const [useFoo, FooProvider] = createDeferredContext<Foo>();

function MyComponent() {
  const foo = useFoo();
  foo.doStuff(); // no need for null assertion
  ...
}

function App() {
  const foo = new Foo();
  return <FooProvider value={foo}><MyComponent /></FooProvider>;
}

Easy dashboard customization

With the new microapps architecture, a new dashboard can be easily created with minimal code. Below is an example to create a minimal dashboard with a map, doors, lifts, robots and mutex groups app.

const mapApp = createMapApp({
  attributionPrefix: 'Open-RMF',
  defaultMapLevel: 'L1',
  defaultRobotZoom: 20,
  defaultZoom: 6,
});

const homeWorkspace: InitialWindow[] = [
  {
    layout: { x: 0, y: 0, w: 7, h: 4 },
    microApp: robotsApp,
  },
  { layout: { x: 8, y: 0, w: 5, h: 8 }, microApp: mapApp },
  { layout: { x: 0, y: 0, w: 7, h: 4 }, microApp: doorsApp },
  { layout: { x: 0, y: 0, w: 7, h: 4 }, microApp: liftsApp },
  { layout: { x: 8, y: 0, w: 5, h: 4 }, microApp: robotMutexGroupsApp },
];

export default function App() {
  return (
    <RmfDashboard
      apiServerUrl="http://localhost:8000"
      trajectoryServerUrl="http://localhost:8006"
      authenticator={new StubAuthenticator()}
      helpLink="https://osrf.github.io/ros2multirobotbook/rmf-core.html"
      reportIssueLink="https://github.com/open-rmf/rmf-web/issues"
      resources={{ fleets: {}, logos: { header: '/resources/defaultLogo.png' }}}
      tasks={{
        allowedTasks: [],
        pickupZones: [],
        cartIds: [],
      }}
      tabs={[
        {
          name: 'Home',
          route: '',
          element: <Workspace initialWindows={homeWorkspace} />,
        },
      ]}
    />
  );
}

Theming

Previously, themes are fixed, the only way to change the theme is to change the dashboard itself. With the move to a dashboard framework architecture, it is now possible to create dashboards with different themes. The examples/custom-theme shows an example of a dashboard with custom theme.

Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
@koonpeng koonpeng marked this pull request as draft August 16, 2024 08:45
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
@koonpeng koonpeng marked this pull request as ready for review August 22, 2024 08:48
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
@koonpeng koonpeng requested a review from aaronchongth August 29, 2024 02:33
Copy link
Member

@aaronchongth aaronchongth left a comment

Choose a reason for hiding this comment

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

Thanks for the herculean effort! I've not looked at the code in detail yet, but started running some of my usual tests of the dashboard, and wanted to highlight some issues and questions first,

  • dashboard crashes when a task is completed (more specifically an alert is supposed to pop up)
    image

@@ -34,6 +34,9 @@
"pnpm": {
"overrides": {
"typescript-json-schema>@types/node": "*"
},
"patchedDependencies": {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding this in, should we add some comments about this patch as well?

I'm not very familiar with patches in this manner, the .patch file was written by you manually, is that right? Or was it recommended somewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was a known problem with mui and three-fiber, the only solution I found is to do a hacky patch.

@aaronchongth
Copy link
Member

To addon, I've found that pages (tabs) are no longer contained in the viewport, and users are required to scroll, to view the lift table under System Overview as well

@koonpeng
Copy link
Collaborator Author

To addon, I've found that pages (tabs) are no longer contained in the viewport, and users are required to scroll, to view the lift table under System Overview as well

Yeah, we no longer force the height of the workspace. But it is still possible to put all the micro apps without scrolling if you change the layout.

@koonpeng
Copy link
Collaborator Author

Thanks for the herculean effort! I've not looked at the code in detail yet, but started running some of my usual tests of the dashboard, and wanted to highlight some issues and questions first,

  • dashboard crashes when a task is completed (more specifically an alert is supposed to pop up)
    image

Thanks for helping to test, should be fixed now

Copy link
Member

@aaronchongth aaronchongth left a comment

Choose a reason for hiding this comment

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

Just these 2 questions/comments, but otherwise LGTM

</ListItemIcon>
</MenuItem>
<Divider />
{/* <MenuItem
Copy link
Member

Choose a reason for hiding this comment

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

Was the admin actions menu item left out for a reason?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's just testing left overs, uncommented it now.

Comment on lines +87 to +98
{
name: 'Custom',
route: 'custom',
element: (
<LocallyPersistentWorkspace
defaultWindows={[]}
allowDesignMode
appRegistry={appRegistry}
storageKey="custom-workspace"
/>
),
},
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 we'll need to find a way to toggle the custom tab based on APP_CONFIG_ENABLE_CUSTOM_TABS, as well as the admin tab with APP_CONFIG_ENABLE_ADMIN_TAB

Or was the whole idea to remove the config variables, and let users set up the dashboard however they want with this new way? If so, then the admin tab should also be added in

or is the admin tab left out because we haven't worked out authz and actions mapping properly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah the plan is to have no build time config variables, after this transitions to a framework library, we can't have build time variables unless we integrate the build system into the framework as well.

Whether to have custom tabs can be easily configured by downstream with allowDesignMode, they can also configure the number of custom tabs and if "builtin" tabs can be customized.

The admin tab is more tricky, until the backend supports proper authz, we should never show the admin tab. But when it does support authz, we should always show the admin tab if the user is an admin. The problem is that in terms of DX, it may be disruptive to force include a tab that the user did not configure. Most likely it will not be a "tab" and instead be accessed from the user icon similar to the admin panel.

Signed-off-by: Teo Koon Peng <[email protected]>
@aaronchongth
Copy link
Member

LGTM so far! With these changes I think we'll need to start documenting how the dashboard app can be configured overall, could you also help add that in?

@koonpeng
Copy link
Collaborator Author

koonpeng commented Sep 3, 2024

There are some docs added in #1007. https://github.com/open-rmf/rmf-web/tree/koonpeng/merge-react-components/packages/rmf-dashboard-framework/docs, haven't really fully tested them in practice, would appreciate if you can help beta test it!

Copy link
Member

@aaronchongth aaronchongth left a comment

Choose a reason for hiding this comment

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

Thanks for pointing to the WIP docs, I've done a first pass on it so far and it looks good. We'll defer the update of documentation to #1007 then.

LGTM! Thanks!

@koonpeng koonpeng merged commit 63ee3a6 into main Sep 5, 2024
5 checks passed
@koonpeng koonpeng deleted the koonpeng/microapp-v2 branch September 5, 2024 01:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants