WIP: tronweb2 Vite + React#1099
Conversation
|
|
||
| tronweb2: | ||
| @echo "Building tronweb2" | ||
| cd tronweb-src && npm i && npm run build |
There was a problem hiding this comment.
Maybe for peace of mind we should also version control the lockfile and have this be
| cd tronweb-src && npm i && npm run build | |
| cd tronweb-src && npm ci && npm run build |
There was a problem hiding this comment.
there's the internal / public registry problem
There was a problem hiding this comment.
^^ this!!, but the hash should match on internal and external versions (if not we have a bigger problem) :)
There was a problem hiding this comment.
hashes match, but lockfiles also have full URLs, and those won't
There was a problem hiding this comment.
iirc npm ci does not throw errors as long as hashes match
There was a problem hiding this comment.
I did not include the lockfile since I was more worried about exposing internal endpoints to the outside world on a public repo
…00KB to reduce some build noise
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I wonder how bad of an idea is to have these pollings:
| Query | Interval |
|---|---|
| Status | 30s |
| Job list | 10s |
| Job detail | 5s |
| Job run | 5s |
| Action run (running/starting) | 2s |
| Action run (finished) | none |
| Configs | none |
There was a problem hiding this comment.
Pull request overview
Adds a new React + Vite + TypeScript frontend (tronweb-src) intended to be built into tronweb2/ and served by the existing Twisted API server under /tronweb, alongside the legacy /web frontend.
Changes:
- Introduces a new Vite/React SPA with routing, UI components (tables, graphs, log viewer), and API/query helpers under
tronweb-src/. - Updates the Twisted
RootResourceto servetronweb2static assets at/tronweb. - Updates build/packaging artifacts to build and ship
tronweb2(Makefile, MANIFEST, Debian install, gitignore).
Reviewed changes
Copilot reviewed 37 out of 40 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tronweb-src/vite.config.ts | Vite build output, base path, dev proxy, Vitest config |
| tronweb-src/tsconfig.node.json | TS config for Vite tooling |
| tronweb-src/tsconfig.json | TS config for app source |
| tronweb-src/tailwind.config.ts | Tailwind theme + plugin config |
| tronweb-src/src/vite-env.d.ts | Vite typings + cytoscape-dagre module typing |
| tronweb-src/src/styles/globals.css | Global Tailwind layers + CSS variables for theming |
| tronweb-src/src/pages/JobRun.tsx | Job run detail page (timeline + action table) |
| tronweb-src/src/pages/JobList.tsx | Jobs list with filtering and state selector |
| tronweb-src/src/pages/JobDetail.tsx | Job detail page with graph + run history |
| tronweb-src/src/pages/Dashboard.tsx | Dashboard summary cards + recent activity |
| tronweb-src/src/pages/ConfigList.tsx | Config namespaces listing page |
| tronweb-src/src/pages/ConfigDetail.tsx | Config detail page showing YAML |
| tronweb-src/src/pages/ActionRun.tsx | Action run detail page with command + logs |
| tronweb-src/src/main.tsx | React entrypoint + QueryClient + router |
| tronweb-src/src/lib/utils.ts | UI utilities (class merge, time/duration formatting) |
| tronweb-src/src/lib/types.ts | Frontend API type definitions |
| tronweb-src/src/lib/queries.ts | React Query hooks for Tron API |
| tronweb-src/src/lib/api.ts | Fetch wrapper + Tron API client |
| tronweb-src/src/components/Timeline.tsx | D3-based timing visualization (dynamic imports) |
| tronweb-src/src/components/ThemeToggle.tsx | Dark mode toggle (root class + localStorage) |
| tronweb-src/src/components/StatusCard.tsx | Dashboard status counter card |
| tronweb-src/src/components/StateBadge.tsx | State pill badge component |
| tronweb-src/src/components/LogViewer.tsx | Virtualized log viewer with search/autoscroll |
| tronweb-src/src/components/DataTable.tsx | Generic table with sorting/pagination |
| tronweb-src/src/components/CommandPalette.tsx | CmdK-based quick navigation palette |
| tronweb-src/src/components/AppShell.tsx | App chrome + nav + command palette trigger |
| tronweb-src/src/components/ActionGraph.tsx | Cytoscape-based action dependency graph |
| tronweb-src/src/App.tsx | App routes and lazy-loaded pages |
| tronweb-src/public/favicon.ico | App favicon asset |
| tronweb-src/postcss.config.js | PostCSS plugins for Tailwind |
| tronweb-src/package.json | Frontend dependencies/scripts/engines |
| tronweb-src/index.html | SPA HTML entrypoint |
| tronweb-src/components.json | shadcn/ui generator config |
| tronweb-src/.gitignore | Ignores node_modules/dist/tsbuildinfo for frontend |
| tron/api/resource.py | Serves /tronweb static content from tronweb2 |
| tests/api/resource_test.py | Updates RootResource child assertion to include tronweb |
| debian/install | Installs tronweb2/ into the venv path |
| Makefile | Adds tronweb2 build target and includes it in deb builds/clean/test |
| MANIFEST.in | Includes tronweb2 in source distributions |
| .gitignore | Ignores built tronweb2/ artifacts at repo root |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| tronweb2: | ||
| @echo "Building tronweb2" | ||
| cd tronweb-src && npm i && npm run build |
There was a problem hiding this comment.
This target runs npm i without a lockfile in tronweb-src/, which makes builds non-reproducible and can unexpectedly change dependency versions over time. Consider committing a lockfile (npm/pnpm/yarn) and using the corresponding deterministic install command (e.g. npm ci) in CI/package builds.
| cd tronweb-src && npm i && npm run build | |
| cd tronweb-src && \ | |
| [ -f package-lock.json ] || (echo "Missing tronweb-src/package-lock.json; commit the lockfile and use deterministic installs for builds." >&2; exit 1) && \ | |
| npm ci && \ | |
| npm run build |
| useEffect(() => { | ||
| if (autoScroll && isStreaming && virtuosoRef.current) { | ||
| virtuosoRef.current.scrollToIndex({ | ||
| index: filteredLines.length - 1, | ||
| behavior: "smooth", | ||
| }); | ||
| } |
There was a problem hiding this comment.
When filteredLines.length is 0, this effect calls scrollToIndex({ index: -1 }), which can throw or behave unexpectedly in react-virtuoso. Add a guard to only scroll when filteredLines.length > 0.
| if (stateFilter) { | ||
| result = result.filter((j) => getLatestRunState(j) === stateFilter); | ||
| } |
There was a problem hiding this comment.
Filtering by stateFilter uses strict equality with the raw latest run state. Since ActionState includes values like starting and waiting (and Dashboard groups them under running/queued), selecting “Running”/“Queued” here won’t match jobs in those transitional states. Consider mapping starting -> running and waiting -> queued (or adding options for those states).
| const sortedData = useMemo(() => { | ||
| if (!sortKey) return data; | ||
| return [...data].sort((a, b) => { | ||
| const aVal = String(a[sortKey] ?? ""); | ||
| const bVal = String(b[sortKey] ?? ""); | ||
| const cmp = aVal.localeCompare(bVal); | ||
| return sortDir === "asc" ? cmp : -cmp; | ||
| }); |
There was a problem hiding this comment.
Sorting coerces values to strings and uses localeCompare, which will sort numeric columns incorrectly (e.g. 10 before 2) and can produce odd ordering for timestamps/durations. Consider detecting numbers/dates and sorting with an appropriate comparator (or allow a per-column compare function).
| className="px-4 py-2 text-left text-xs font-medium uppercase text-muted-foreground cursor-pointer select-none hover:text-foreground" | ||
| onClick={() => handleSort(col.key)} | ||
| > | ||
| <span className="inline-flex items-center gap-1"> | ||
| {col.header} | ||
| <ArrowUpDown className="h-3 w-3" /> |
There was a problem hiding this comment.
Column.sortable is defined but not honored: headers are always clickable and always show the sort icon. Consider only attaching the click handler / cursor styles (and rendering ArrowUpDown) when col.sortable is true, otherwise leave the header static.
| className="px-4 py-2 text-left text-xs font-medium uppercase text-muted-foreground cursor-pointer select-none hover:text-foreground" | |
| onClick={() => handleSort(col.key)} | |
| > | |
| <span className="inline-flex items-center gap-1"> | |
| {col.header} | |
| <ArrowUpDown className="h-3 w-3" /> | |
| className={cn( | |
| "px-4 py-2 text-left text-xs font-medium uppercase text-muted-foreground", | |
| col.sortable && "cursor-pointer select-none hover:text-foreground" | |
| )} | |
| onClick={col.sortable ? () => handleSort(col.key) : undefined} | |
| > | |
| <span className="inline-flex items-center gap-1"> | |
| {col.header} | |
| {col.sortable && <ArrowUpDown className="h-3 w-3" />} |
| {paginatedData.map((row, i) => ( | ||
| <tr | ||
| key={i} | ||
| className={cn( | ||
| "border-b last:border-0 transition-colors", |
There was a problem hiding this comment.
<tr key={i}> uses the row index as the React key. With sorting/pagination this can cause incorrect row reuse and subtle UI bugs. Consider accepting a getRowKey prop or using a stable identifier from the row data when available.
| function computeDuration(start: string, end: string): string { | ||
| const ms = new Date(end).getTime() - new Date(start).getTime(); |
There was a problem hiding this comment.
computeDuration uses new Date(start/end) directly, but the backend JSON encoder formats datetimes as YYYY-MM-DD HH:MM:SS (space separator). That format is not reliably parsed by browsers, so duration can become NaN/incorrect. Normalize the timestamps the same way as elsewhere (e.g. replace the space with T) or parse explicitly.
| function computeDuration(start: string, end: string): string { | |
| const ms = new Date(end).getTime() - new Date(start).getTime(); | |
| function normalizeTimestamp(timestamp: string): string { | |
| return timestamp.includes(" ") ? timestamp.replace(" ", "T") : timestamp; | |
| } | |
| function computeDuration(start: string, end: string): string { | |
| const startMs = new Date(normalizeTimestamp(start)).getTime(); | |
| const endMs = new Date(normalizeTimestamp(end)).getTime(); | |
| if (Number.isNaN(startMs) || Number.isNaN(endMs)) return ""; | |
| const ms = endMs - startMs; |
Addition of
tronweb2tronweb-srcusing React, TypeScript, Vite, Tailwind CSS, and modern UI libraries. This includes core app scaffolding, routing, components (e.g.,AppShell,CommandPalette,DataTable), configuration files, and build scripts.Integration and Serving
tron/api/resource.pyto serve the newtronweb2static files at the/tronwebpath, alongside the existing/webfrontend.Building and Packaging
Makefileto add build commands fortronweb2, ensure it is built during package creation, and clean up its build artifacts.MANIFEST.inanddebian/installto includetronweb2in source distributions and installation.setup.pyto includetronweb2as a package.