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

Introduce Nativelink Web UI #1449

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SchahinRohani
Copy link
Contributor

@SchahinRohani SchahinRohani commented Nov 1, 2024

Description

It's still a work in progress and highly experimental

Checklist

  • Updated documentation if needed
  • Tests added/amended
  • bazel test //... passes locally
  • PR is contained in a single commit, using git amend see some docs

This change is Reviewable

It's still a work in progress and highly experimental
Copy link
Member

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

Reviewed 41 of 41 files at r1, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and all files reviewed, and pending CI: Bazel Dev / macos-13, Bazel Dev / macos-14, Bazel Dev / ubuntu-24.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Coverage, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, NativeLink.com Cloud / Remote Cache / macos-14, NativeLink.com Cloud / Remote Cache / ubuntu-24.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, Web Platform Deployment / macos-14, Web Platform Deployment / ubuntu-24.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (22.04), integration-tests (22.04), macos-13, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, and 14 discussions need to be resolved


flake.nix line 424 at r1 (raw file):

              nativelink-x86_64-linux
              publish-ghcr
              nativelink-ui # not working yet

fyi: I can take a shot at making this image work.


tools/pre-commit-hooks.nix line 68 at r1 (raw file):

      # Bun binary lockfile
      "web/platform/bun.lockb"
      "web/ui/bun.lockb"

nit: Should we use bun workspaces here to have only a single bun.lockb? It might help with only having a single version of every dep, but would also make the ui less "standalone" I guess. No strong opinion on what approach is better.


web/ui/.gitignore line 1 at r1 (raw file):

# Logs

Most of this seems unnecessary. Let's reduce this to a minimum.


web/ui/.env.example line 1 at r1 (raw file):

BRIDGE_URL=

This file needs documentation or we should just put it in the readme that users should create this file.


web/ui/image.nix line 6 at r1 (raw file):

  pullImage,
  ...
}: let

fyi: I'll go over this later.


web/ui/index.html line 5 at r1 (raw file):

  <head>
    <meta charset="UTF-8">
    <!-- <link rel="icon" href="/favicon.ico"> -->

nit: Do we need this comment?


web/ui/package.json line 3 at r1 (raw file):

{
  "name": "nativelink-dashboard",
  "version": "0.0.0",

nit: Would it make sense to give this the same version as nativelink?


web/ui/README.md line 5 at r1 (raw file):

This template should help get you started developing with Vue 3 in Vite.

## Recommended IDE Setup

nit: Add fancy emojis to the ## titles.


web/ui/tsconfig.node.json line 2 at r1 (raw file):

{
  "extends": "@tsconfig/node20/tsconfig.json",

nit: node 20?


web/ui/vite.config.ts line 10 at r1 (raw file):

// https://vite.dev/config/

nit: unnecessary comment


web/ui/src/assets/logo-dark.svg line 1 at r1 (raw file):

<?xml version="1.0" encoding="UTF-8"?>

I'm pretty sure we have this logo in the codebase somewhere already and probably don't need to add it again.


web/ui/src/assets/logo-light.svg line 1 at r1 (raw file):

<?xml version="1.0" encoding="UTF-8"?>

Same here.


web/ui/src/assets/main.css line 3 at r1 (raw file):

@import './base.css';

/* #app {

nit: Remove comment


web/ui/src/components/Footer.vue line 4 at r1 (raw file):

<footer class="fixed bottom-0 left-0 flex h-10 w-[calc(100%-15px)] justify-center bg-background text-center font-normal">
    <div class="flex w-2/3 items-center justify-between text-sm font-normal text-muted-foreground">
      <a href="#">© 2024 Trace Machina, Inc.</a>

nit: I might be wrong here, but I think "Copyright (c)" would be more accessible than the copyright icon. But we should probably double-check.


web/ui/src/components/icons/IconDocumentation.vue line 1 at r1 (raw file):

<template>

nit: Can't we just import this and the other icons as components from some library?


web/ui/src/views/AsideView.vue line 39 at r1 (raw file):

            <nav class="ml-2 space-y-1 w-full">
                <RouterLink to="/"
                class="inline-flex items-center whitespace-nowrap rounded-md font-medium ring-offset-background transition-colors focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 hover:bg-accent hover:text-accent-foreground px-4 py-2 w-full justify-start text-base h-14 cursor-pointer bg-accent text-accent-foreground"

nit: looks like a lot of duplication for the classes in this file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants