Dashboard search, paging, and payload fixes - #9
Open
TurtIeSocks wants to merge 13 commits into
Open
Conversation
The API never sent a Content-Encoding header, so every response went out
uncompressed no matter what the client advertised. The status endpoint is
the one that hurts: it serialises every device, worker and controller on
each poll, and the bulk of that JSON is the same field names repeated
once per record. That redundancy is exactly what deflate is built for,
and the saving grows with the number of connections a deployment
reports.
Once the response takes longer to transfer than the poll interval,
requests start overlapping and never catch up. Compression is what keeps
a poll inside its own interval.
The middleware also covers the embedded UI. Serving this repository's own
build, measured against a running binary:
JS 668,735 -> 200,496 bytes
CSS 67,972 -> 12,233 bytes
Three exclusions are deliberate:
- WebSocket upgrades. The middleware refuses any request carrying
Connection: Upgrade, so device and controller sockets keep their
hijackable ResponseWriter.
- Already-compressed bodies. Logcat replies are a zip built in memory,
and the static assets include png, ico and woff2. Deflating those
costs CPU for a fraction of a percent and can grow the response.
- Bodies under 1 KiB, where the gzip header and trailer outweigh what
the deflate stream saves. The many small action acknowledgements fall
into this bucket.
Gzip is registered after the logger so the response size the logger
reports is the size that actually went over the wire.
Uses gin-contrib/gzip rather than a hand-rolled handler: Accept-Encoding
negotiation, writer pooling, Content-Length handling and the refusal to
double-compress add up to well past the point where owning the code is
cheaper than depending on it, and the failure modes are subtle. It comes
from the same org as the gin-contrib/static dependency already in use.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four defects in the shared table machinery, fixed together because they
live in the same lines: making itemCount a required option changed every
call site, and the same edits carry the search rework.
1. The page index survived a filter change.
useTablePagination exposed resetPage() and no caller ever used it, so
searching from anything but the first page sliced past the end of the
results and drew an empty table. The footer read like this, with a
non-empty result set behind it:
before search "76-100 of 1400" "Page 4 of 56"
after search "76-50 of 50" "Page 4 of 2" 0 rows drawn
Fifty rows matched and none were shown. The hook now takes the filtered
row count and a reset key. A new key, the search term, goes back to the
first page; a changing count only clamps, because the count also moves
on its own as devices join and drop off the poll and resetting on that
would drag the reader back every few seconds.
2. Paging forward never scrolled.
The old handler scrolled to the bottom of the document, and only when
navigating backward off a partly-filled last page. Every other page
change left the reader at the foot of the page they had just left,
looking at the footer of a table whose rows had all been replaced. The
hook now scrolls the table back into view on any page change, honouring
prefers-reduced-motion.
3. One search box ran two filters over different fields.
The worker tables filtered devices in the parent and workers in the
child, each against its own field list, with only the child trimming its
term. Two consequences, both reported as "search behaves strangely":
- A term only the inner list knew about could never reach it, because
the outer pass had already discarded the device. Typing "active" or
"inactive" always came back empty.
- A worker id pasted out of a log with a trailing space matched
nothing, because the outer pass looked for the space too.
There is now one filter per table over one declared field list. Each
list lives in a sibling module next to its table, and doubles as the
text shown when a search matches nothing, so the two cannot drift apart
again. session.controller.id is dropped from all three lists: it was
searched everywhere and populated nowhere.
Controllers gain UUID as a searchable field. It is the identifier a
support conversation quotes and it was the one thing on the row that
could not be searched for.
4. Worker rows collided on their React key.
Worker ids are only unique within a device, and these tables flatten
across devices, so the same id can appear on several rows at once.
Expanding any one of them expanded every namesake. Rows are now keyed on
device id and worker id together.
Also: tables render an empty state instead of a bare header row, naming
the term and the fields it was matched against, and the card heading
reports the filtered count against the total ("Workers (50 of 500)")
rather than the pre-filter count it used to show.
Verified against the mock large profile, 50 devices and 500 workers:
searching from page 3 now lands on "1-25 of 111" / "Page 1 of 5" with
rows drawn; " active " matches 298 of 500, agreeing with the in-use
stat card; an unmatched term shows the empty state and lists the fields.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Remove Dead is the only destructive action in the app that asked for no
confirmation and reported no result. It fired straight into
PUT /api/device/_/action/delete, and the reply, which carries the number
of entries removed, was parsed and discarded. Nothing appeared on screen
either way, so a working call and a failing one were indistinguishable,
and there was no way to tell that the button was wired up at all.
The backend was never the problem: deleteDevice() treats "_" as "every
unconnected device", calls DeleteUnconnectedDevices(), and returns the
count.
Now the button opens a confirmation naming what will happen, and the
result becomes a toast: the number removed on success, a plain note when
there was nothing to remove, and the error text when the call fails.
Every other device action in this tree already reports through
toast.promise; this brings the last one into line.
Also corrects the mock handler, which returned {ok: true} where the real
API returns {status, message, devices_count}. The mock removed the
devices but could not report how many, so the UI correctly said "no dead
devices to remove" while several disappeared from the table. A mock that
disagrees with the endpoint it stands in for hides exactly this kind of
bug.
Verified against the mock large profile: 50 devices with 44 connected,
confirmation shown, toast reads "Removed 6 dead devices", table drops to
44.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Avg Request Duration rendered 0 in every time window on a healthy deployment. The values were never zero, but Math.round() flattened anything below half a millisecond, so an instance running well was indistinguishable from a metric that had stopped collecting. Durations under 10 ms now keep two decimals, and anything above zero but under a hundredth of a millisecond renders as "<0.01" rather than collapsing back to 0. The requests/s column already had this treatment; the duration column did not. The four rows were also four near-identical copies of the same markup, so the fix would otherwise have had to land in four places. They are now one array and one map. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The logo was a 475x475 PNG weighing 225,847 bytes, rendered by the header at size-8, which is 32 CSS pixels. It was the single largest asset the app served, larger after compression than any JavaScript chunk, and PNG barely deflates so serving it gzipped would not have helped. Resampled to 96x96, which still covers a 3x display at the size it is actually drawn: before 225,847 bytes after 13,020 bytes 94% smaller Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The library declared 33 runtime dependencies and imported 13. Most of the extras were never used at all: dnd-kit, cmdk, recharts, lodash, date-fns, libphonenumber-js, react-hook-form, dompurify, html-react-parser, vaul, sonner, next-themes, query-string, diacritic, inflection, react-dropzone, react-error-boundary. Three packages it did import were missing from the manifest entirely (motion, react-toastify, @number-flow/react) and only resolved because the workspace root happened to carry them. That was survivable while this library was consumed by copying its source. It stops being survivable the moment it is published, because every consumer would install react-admin and recharts to get a table. Also removes seven files left over from a react-admin scaffold: field-types, i18n-provider, notify-auth-error, sanitize-input-rest-props, resolve-label, unknown-types and are-ids-equal. None is exported from the library index, and nothing in the application referenced any of them. They were the only importers of ra-core, ra-i18n-polyglot and ra-language-english. React moves to peerDependencies, which is what a component library should have been declaring; the application already pins and dedupes it in its Vite config. msw is an optional peer: it is needed only by the mock handlers, which a consumer pulls in explicitly or not at all. One stale line goes from the stylesheet too: a Tailwind @source pointing at apps/rotom-ui/src, a directory that does not exist. Harmless while Tailwind scans the real path alongside it, but a published package whose class scanning depends on @source lines should not carry paths that never resolve. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reusing this library outside the repository meant unpacking a source tarball at a pinned commit. No version history, no integrity check, and no way to see which revision was in use without reading a stamp file. The library is now published as @unownhash/rotom-base-ui. Two ways in: a base-ui-v<version> tag publishes that version under "latest", validated against package.json the same way release.yml validates version.txt against version.go; a push to main that touches the library publishes <version>-main.<short sha> under the "main" tag, which is what replaces pinning a commit. A consumer can track main without waiting for a release and still record exactly what it installed. The workflow deliberately carries no paths: filter. A paths filter applies to tag pushes as well as branch pushes, and a tag usually lands on a commit that changed nothing under libs/base-ui, so the filter would have silently skipped the release the tag existed to make. The branch case is gated inside the job with an explicit diff instead. It builds and lints the application against the library before publishing. Shipping TypeScript source means consumers compile it, so the application build is the only thing standing between a broken library and a broken consumer. Better to find that here than after someone installs it. Three changes were needed to make the package publishable: The name. GitHub Packages resolves an npm scope to the account that owns the repository, so @rotom-ng/base-ui could not be published under UnownHash at all. It is now @unownhash/rotom-base-ui, and the workspace resolves it by that same specifier, so local development and an installed copy agree. The stylesheet. It carried an @source pointing up and across into apps/rotom-ng-ui/src. Fine while the library was read out of the workspace, useless once installed from a registry, at which point every class used only by the application would have been dropped from the build with no error. The library now scans only itself, and the application has its own stylesheet that imports the library's and scans its own tree. The emitted CSS is byte-identical at 67.97 kB, so nothing was lost in the split. Vite alias. The tsconfig paths plugin resolves modules but not CSS @import, so the package name needed a real alias. Written in array form because order matters: the bare "@" alias is a prefix of "@unownhash/" and would otherwise claim it. The react and react-dom aliases become explicit bare-plus-subpath pairs to preserve the prefix matching the object form gave them for free. Also drops libs/base-ui/src/index.css, an empty file nothing imported. Verified with npm pack: 103 files, and the three export-map targets (index, styles/globals.css, mocks) are all present in the tarball. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Workers, devices and controllers each live on their own route, and the identifiers relating them were rendered as inert text. Following one meant reading an id, switching tabs, and typing it back in. Controllers cannot be joined to workers at all. A controller's worker_id is its own name rather than a foreign key, and workers come back without a session controller, so there is no edge there to render. The edge that does exist is worker.device_id against device.id. The workers table gains a Device column that navigates to that device. It takes the slot the Controller ID column occupied, which was permanently blank for the reason above: one dead column out, one working join in, same column count. The devices table's Origin cell navigates the other way, to that device's workers. Cross-linking needs the target page's filter to be addressable, so table search moves from component state into the URL as ?q=. Three things fall out of that and the third is the point: a search survives a reload, a search can be pasted to someone else, and one page can link into another's filter. It replaces rather than pushes, so typing does not bury the previous page under one history entry per keystroke. Verified against the mock medium profile: a worker's Device cell opens /devices?q=device-0 with the box pre-filled and the table reading "Devices (1 of 15)"; that device's Origin cell returns to /workers?q=device-0 reading "Workers (7 of 100)". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The metrics endpoint is served by promhttp.HandlerFor, which negotiates Accept-Encoding itself and returns an already-gzipped body when the scraper asks for one. Wrapping that in the new middleware produced a response compressed twice: a client decompresses once and is handed another gzip stream instead of exposition text. Caught by the existing app tests, which read the body expecting the "go_" prefix and got binary. The middleware already declines to double-compress a body whose handler set Content-Encoding before writing, but that guard inspects the first Write, and promhttp's own gzip writer does not present its header there. Excluding the path is the honest fix: an endpoint that does its own content negotiation should not have a second compressor over it. Prometheus exposition still goes out compressed, by the handler that owns it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every request to the API has to carry the session header, or the server rejects it whenever an api secret is configured. Device actions and the status polls were converted when that landed; four call sites were not. controllers-table disconnect and reconnect execute-job-modal run a job jobs-page clear a job instance, reload jobs All four used bare fetch, so with a secret set they would 401 and surface as a generic failure toast rather than sending the operator back to the login form. apiFetch attaches the header, keeps same-origin cookies, and converts a 401 into the AuthError that AuthGate watches for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The pagination control works out an absolute target page from the page it was rendered with, so two clicks landing in the same render both computed the same target and the second was a no-op. Clicking Next twice in quick succession moved forward one page. The hook now records the page it rendered with, recovers the step the reader asked for, and applies it to whatever the current value is by the time the update runs, clamped to the last page. Two fast clicks advance two pages. Kept inside the hook rather than changing the pagination component's props, which are shared by every table. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every page replaced its entire contents with a full-page error the moment one refresh failed, throwing away the table, the reader's page position, and whatever was in the search box. A dashboard that polls every five seconds will miss a poll occasionally, and losing the whole view over a transient blip is worse than showing numbers a few seconds old. The guard is now on the data rather than on isSuccess. That distinction is the whole fix, and it is not obvious: a refetch that fails while cached data is present flips status to "error" and isSuccess to false but leaves data in place. Verified directly against query-core: after success status=success isSuccess=true hasData=true after failed refetch status=error isSuccess=false hasData=true So keying the full-page error off isSuccess, which is what the first attempt at this did, would have kept wiping the view. With data present and a failure, the page renders as usual above a banner saying the numbers are the last successful update. With no data at all, the full-page error stands. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
libs/base-ui is a workspace member, so its dependency list is recorded in the root lockfile. Pruning that list left the lockfile stale, and CI installs with --frozen-lockfile, so the next push would have failed before running a single test. Most of the change is the react-admin tree dropping out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Usability and performance fixes across the dashboard.
Content-Encoding, so everything went out uncompressed. Serving this repo's own build: JS 668,735 to 200,496 bytes, CSS 67,972 to 12,233./metricsis left alone, since promhttp compresses it itself76-50 of 50,Page 4 of 2activeandinactivewere unreachable, a trailing space returned nothing, andsession.controller.idwas searched in three places but never populateddevice_idplusid. Worker ids are only unique within a device and these tables flatten across devices, so expanding one row expanded every namesakedevices_countapiFetch. They used barefetch, so they would 401 whenever an api secret is configured?q=, so one page can link into another's filterlibs/base-uifrom 33 declared dependencies to the 13 it imports, and drop the react-admin scaffold that was the only thing usingra-coreControllers are still sent in full on every poll. Gzip makes that survivable, but paging them server side breaks the status contract and needs its own PR.
Verified:
go test -race ./...,go vet, andgolangci-lintclean, UI builds, gzip checked against a running binary.