feat(web,server): servers table row visual redesign with tags#101
feat(web,server): servers table row visual redesign with tags#101ZingerLittleBee merged 44 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds server tagging (validation, persistence, REST hooks), refactors servers table into composable metric cells (status-dot, tag chips, traffic quota), introduces traffic-quota utilities and tests, extends WebSocket payloads/types to include tags and cpu_cores, and wires backend tag service + APIs and integration tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Server Edit Dialog
participant API as Backend API
participant DB as Database
User->>UI: Open dialog
UI->>API: GET /api/servers/{id}/tags (useServerTags)
API->>DB: SELECT tags BY server_id
DB-->>API: tags[]
API-->>UI: tags[]
User->>UI: Edit tags
UI->>UI: parseTagsInput (validate)
User->>UI: Click Save
UI->>API: PATCH /api/servers/{id} (details)
API->>DB: update server
DB-->>API: OK
API-->>UI: OK
UI->>API: PUT /api/servers/{id}/tags (if dirty)
API->>API: validate & normalize
API->>DB: delete/insert tags (transaction)
DB-->>API: OK
API-->>UI: normalized tags
UI->>UI: update cache
sequenceDiagram
participant Client
participant WS as WebSocket Server
participant DB
Client->>WS: connect & request full_sync
WS->>DB: SELECT servers
WS->>DB: SELECT server_tags ORDER BY server_id, tag
DB-->>WS: servers[], tags[]
WS->>WS: group tags by server_id
WS->>Client: full_sync payload (includes tags, cpu_cores)
Client->>Client: merge payload (preserve static fields on empty updates)
Client->>UI: render table with tags & status dots
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/hooks/use-servers-ws.ts (1)
103-120:⚠️ Potential issue | 🟡 MinorSubtle: treating
[]as a static default can hide legitimate empties.The guard on line 110-111 suppresses updates whose value is an empty array for any static field. This is the intent for
tags/featureson noisy per-frame updates, but it also means that a legitimate "all tags removed" signal arriving via theupdatepath would be ignored, and clients would keep stale tags until the nextfull_sync. Today, tag edits appear to flow via REST + query invalidation (use-server-tags.ts), so this is latent, not actively broken — worth either a comment documenting the assumption or a stricter rule that only suppresses when the key is absent from the incoming payload rather than when it is explicitly empty.🛡️ One stricter alternative
- for (const [key, value] of Object.entries(server)) { - const isStaticDefault = - STATIC_FIELDS.has(key) && (value === null || value === 0 || (Array.isArray(value) && value.length === 0)) - if (!isStaticDefault) { - ;(merged as Record<string, unknown>)[key] = value - } - } + // For static fields, only skip when the key is absent from the incoming payload. + // Empty arrays / zeros / nulls that are explicitly present should still overwrite. + const incomingKeys = new Set(Object.keys(server)) + for (const [key, value] of Object.entries(server)) { + if (STATIC_FIELDS.has(key) && !incomingKeys.has(key)) continue + ;(merged as Record<string, unknown>)[key] = value + }(Adjust to match whatever "no-update" contract the agent/server actually produces for these frames.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/use-servers-ws.ts` around lines 103 - 120, In mergeServerUpdate, the current suppression logic treats empty arrays as a static-default and ignores explicit "clear" updates for fields like tags/features; update the guard so we only suppress when the incoming payload lacks the key (e.g., use a check for incoming key absence such as server[key] === undefined or Object.prototype.hasOwnProperty.call(server, key) rather than Array.isArray(value) && value.length === 0), keeping STATIC_FIELDS logic but allowing explicit empty arrays to overwrite existing values; refer to mergeServerUpdate and STATIC_FIELDS to locate the change.
🧹 Nitpick comments (12)
docs/superpowers/plans/2026-04-17-servers-table-row-visual-redesign.md (1)
1-2644: Plan document; implementation is reviewed in the actual code files.One small callout: Task 19's "stub"
crates/server/tests/server_tags.rscontaining only a comment creates an empty integration-test binary thatcargo testwill still link. Prefer omitting the file entirely (as the plan itself suggests) to avoid build noise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-17-servers-table-row-visual-redesign.md` around lines 1 - 2644, The empty-test stub crates/server/tests/server_tags.rs (containing only a comment) produces a linked test binary and should be removed; delete that file (or convert it to a non-test helper module) so only the real tags tests live in crates/server/tests/integration.rs, then git rm the stub and commit (e.g. "test(server): remove empty server_tags.rs stub — tests live in integration.rs"). Ensure integration.rs contains the full tags tests and re-run cargo test to verify no extra empty test binary remains.docs/superpowers/plans/2026-04-16-servers-table-density.md (1)
1-784: Documentation-only plan; no review comments.This plan file predates the current PR's redesign plan (2026-04-17) and may now be partially superseded. If it's kept for historical reference, consider adding a note pointing readers to the newer plan to avoid confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-16-servers-table-density.md` around lines 1 - 784, This plan file is historical and should be marked as superseded by the newer 2026-04-17 servers-table-density plan: add a short one-paragraph banner immediately under the "# Servers Table Density & Disk I/O Display Implementation Plan" heading that states this document is kept for historical reference and has been superseded by the 2026-04-17 plan (include that date/string so readers can find the newer doc), optionally add a "Superseded" tag or front-matter line and update the top-level summary to point readers to the newer plan, then commit with a descriptive message like "docs: mark 2026-04-16 servers table density plan as superseded by 2026-04-17".apps/web/src/components/server/tag-chip.tsx (2)
12-19: Minor:Math.truncon line 16 is redundant.
Math.imulalready returns a signed 32-bit integer, and addingcharCodeAt(i)(≤ 0xFFFF) keeps the result integer-valued —Math.truncon an integer-valued number is a no-op. You can drop it without changing semantics.♻️ Proposed simplification
function hashTag(tag: string): number { let h = 0 for (let i = 0; i < tag.length; i++) { h = Math.imul(h, 31) + tag.charCodeAt(i) - h = Math.trunc(h) } return Math.abs(h) % PALETTE.length }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/server/tag-chip.tsx` around lines 12 - 19, Remove the redundant Math.trunc call in the hashTag function: Math.imul already produces a signed 32-bit integer and adding tag.charCodeAt(i) preserves integer semantics, so delete the Math.trunc(h) step inside hashTag (the function that computes an index into PALETTE) and return Math.abs(h) % PALETTE.length as before.
32-44:key={tag}assumes unique tags per server.If duplicate tags ever slip past backend validation, React will emit a duplicate-key warning and render only one chip. The current
server_tagservice should enforce uniqueness (PK on(server_id, tag)), so this is defensive only — consider either falling back to an indexed key or adding a frontend dedup as a safety net.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/server/tag-chip.tsx` around lines 32 - 44, The tag chips use the tag string as React key (in the tags.map callback using key={tag}), which will break if duplicate tags appear; update the rendering to guarantee unique keys by using a stable index or composite key (e.g., `${tag}-${i}`) or deduplicate tags before mapping (e.g., create a unique list from tags) so that the span elements rendered in the tags.map loop have unique keys; reference the tags.map callback and key={tag} in tag-chip.tsx (and consider server_tag uniqueness in backend but implement the frontend defensive fix here).apps/web/src/hooks/use-servers-ws.test.ts (1)
213-249: Consider consolidatingbaseServerwith the existingmakeServerhelper.
baseServeris a near-duplicate ofmakeServerat lines 6-37 but with a more complete field set (addstags,features,cpu_cores,disk_read_bytes_per_sec,disk_write_bytes_per_sec). Maintaining two fixture builders risks drift asServerMetricsevolves. If feasible, extendmakeServerto cover the new fields and retirebaseServer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/use-servers-ws.test.ts` around lines 213 - 249, Replace the duplicate fixture builder by extending the existing makeServer helper to include the extra fields present in baseServer (tags, features, cpu_cores, disk_read_bytes_per_sec, disk_write_bytes_per_sec and any other fields from ServerMetrics), change makeServer’s signature to accept Partial<ServerMetrics> overrides like baseServer, update all tests that call baseServer to call makeServer instead, and then remove the baseServer function to avoid drift between the two builders.crates/server/src/router/ws/browser.rs (1)
266-275: Log tag-query failures instead of silently defaulting to empty.
unwrap_or_default()here is inconsistent with how the siblingServerService::list_serverserror is handled a few lines above (explicittracing::error!on failure). If theserver_tagquery ever errors transiently, every browser sees empty tags with no diagnostic breadcrumb. Suggest mirroring the existing pattern.🪵 Proposed fix
- let tags_rows = server_tag::Entity::find() - .order_by_asc(server_tag::Column::ServerId) - .order_by_asc(server_tag::Column::Tag) - .all(&state.db) - .await - .unwrap_or_default(); + let tags_rows = match server_tag::Entity::find() + .order_by_asc(server_tag::Column::ServerId) + .order_by_asc(server_tag::Column::Tag) + .all(&state.db) + .await + { + Ok(rows) => rows, + Err(e) => { + tracing::error!("Failed to load server tags for FullSync: {e}"); + Vec::new() + } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/server/src/router/ws/browser.rs` around lines 266 - 275, The tags query currently swallows errors via unwrap_or_default() causing silent empty tag lists; replace that with explicit error handling similar to ServerService::list_servers: await the server_tag::Entity::find() call into a Result, on Err call tracing::error! with context (e.g., "failed to query server tags") and the error, then set tags_rows to an empty Vec; keep the subsequent loop that builds tags_by_server (the HashMap<String, Vec<String>>) unchanged so browser behavior is preserved but failures are logged.apps/web/src/components/server/server-edit-dialog.tsx (1)
129-140: Minor: also resettagsDirtywhen reverting input on save failure.
saveTagsrevertstagsInputtoinitialTags.join(', ')on failure, but leavestagsDirty = true. The next submit will therefore re-run the tags PUT with the (now canonical) initial list — a redundant, no-op round-trip. SettingtagsDirty = falseafter the revert keeps subsequent saves scoped to the main mutation only.♻️ Proposed tweak
} catch (err) { if (initialTags) { setTagsInput(initialTags.join(', ')) + setTagsDirty(false) } toast.error(err instanceof Error ? err.message : t('tags_save_failed')) return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/server/server-edit-dialog.tsx` around lines 129 - 140, The saveTags handler reverts the input on error but doesn't reset the tagsDirty flag, causing a redundant PUT on the next submit; in the saveTags function (which calls tagsMutation.mutateAsync and setTagsInput with initialTags.join(', ')), after reverting set tagsDirty to false (use the state setter for tagsDirty) so the UI reflects the canonical state and prevents a no-op tags update on subsequent saves.apps/web/src/lib/traffic.ts (1)
17-23: LGTM — small, pure, well-tested.One semantic note worth acknowledging in a comment but not blocking: when
entryis undefined,usedis the lifetimenet_in_transfer + net_out_transfer, while theentrybranch uses cycle-scoped bytes. Callers that render this as a "monthly quota" bar will visually saturate to 100% for long-running servers without a configured billing cycle — which matches theDEFAULT_TRAFFIC_LIMIT_BYTESfallback intent, and the tests pin this behavior down. Consider a brief comment on the fallback branch to make the semantic mismatch explicit for future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/traffic.ts` around lines 17 - 23, Add a brief inline comment inside computeTrafficQuota explaining that when entry is undefined the computed used value (netInTransfer + netOutTransfer) is a lifetime total while when entry is present used is cycle-scoped (entry.cycle_in + entry.cycle_out), and note that this can cause long-running servers to saturate the quota bar against DEFAULT_TRAFFIC_LIMIT_BYTES by design; update the comment near the used computation and reference the entry, netInTransfer, netOutTransfer, and DEFAULT_TRAFFIC_LIMIT_BYTES symbols so future readers understand the semantic mismatch.apps/web/src/routes/_authed/servers/index.tsx (1)
95-95: Optional: memoize aMapfor traffic overview lookup instead of.findper row.
trafficOverview.find((e) => e.server_id === row.original.id)runs on every cell render (so O(N·M) per table render). AMapcomputed once alongside the data keeps the cell lookup O(1) and also makes thecolumnsdependency more stable than passing the raw array.♻️ Proposed refactor
- const { data: trafficOverview = [] } = useTrafficOverview() + const { data: trafficOverview = [] } = useTrafficOverview() + const trafficByServer = useMemo( + () => new Map(trafficOverview.map((e) => [e.server_id, e])), + [trafficOverview] + ) @@ - cell: ({ row }) => { - const entry = trafficOverview.find((e) => e.server_id === row.original.id) - return <NetworkCell entry={entry} server={row.original} /> - }, + cell: ({ row }) => <NetworkCell entry={trafficByServer.get(row.original.id)} server={row.original} />, @@ - [t, groupMap, groupOptions, statusOptions, trafficOverview] + [t, groupMap, groupOptions, statusOptions, trafficByServer]Also applies to: 210-214, 266-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_authed/servers/index.tsx` at line 95, The table currently calls trafficOverview.find(...) per-cell which is O(N·M); instead compute a Map keyed by server_id from the useTrafficOverview() result and memoize it (e.g., const trafficMap = useMemo(() => new Map(trafficOverview.map(e => [e.server_id, e])), [trafficOverview])) and then replace all per-row trafficOverview.find(...) calls in your column cell renderers with trafficMap.get(row.original.id) to make lookups O(1) and stabilize the columns' dependencies; update every column/cell that references trafficOverview.find (including the same uses in the columns definition) to use trafficMap.get instead.apps/web/src/hooks/use-server-tags.test.tsx (1)
34-51: Optional: assert request shape (URL/method/body) in the PUT test.The current mock ignores
_inputand only round-trips the body through the response. A small assertion that the request targetsPUT /api/servers/srv-1/tagswith{ tags: [...] }would catch regressions where a refactor accidentally changes the endpoint or payload wrapping, since the cache-patch assertions alone wouldn't notice that.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/use-server-tags.test.tsx` around lines 34 - 51, In the 'PUTs tags and patches both caches on success' test for useUpdateServerTags, enhance the fetch mock (vi.spyOn(globalThis, 'fetch').mockImplementationOnce) to assert the request URL, method and body shape before returning the Response: verify the request targets /api/servers/srv-1/tags, that init.method === 'PUT', and that JSON.parse((init as RequestInit).body) yields an object { tags: [...] } (matching the payload passed to mutateAsync); after those assertions return the same 200 JSON response as before.apps/web/src/routes/_authed/servers/index.cells.test.tsx (1)
108-113: Two renders in one test share the DOM.Both
rendercalls mount intodocument.bodywithout unmounting the first, soscreen.getByTextsees both subtrees. It works here only because '100%' and '0%' each appear exactly once. Consider splitting into twoitblocks or callingunmount()/cleanup()between renders for robustness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_authed/servers/index.cells.test.tsx` around lines 108 - 113, The test "clamps percentage to [0, 100]" calls render twice into the same DOM so the second assertion can see both mounted trees; fix by either splitting into two tests (e.g., two it blocks each rendering MetricBarRow with pct=150 and pct=-5) or call react-testing-library cleanup()/unmount() between the two render calls so each assertion only queries the intended render; update references to MetricBarRow, render, screen.getByText accordingly.crates/server/src/service/server_tag.rs (1)
60-67: Preferinsert_manyover a per-tag insert loop.Each iteration issues a separate round-trip inside the transaction. With up to
MAX_TAGSentries this is minor, butEntity::insert_manycollapses it to one statement and reads more idiomatically for SeaORM.♻️ Proposed refactor
- for tag in &normalized { - server_tag::ActiveModel { - server_id: Set(server_id.to_string()), - tag: Set(tag.clone()), - } - .insert(&txn) - .await?; + if !normalized.is_empty() { + let models = normalized.iter().map(|tag| server_tag::ActiveModel { + server_id: Set(server_id.to_string()), + tag: Set(tag.clone()), + }); + server_tag::Entity::insert_many(models).exec(&txn).await?; }Does sea-orm 1.0 Entity::insert_many accept an empty iterator without error?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/server/src/service/server_tag.rs` around lines 60 - 67, Replace the per-tag insert loop that builds server_tag::ActiveModel for each tag and calls .insert(&txn).await with a single server_tag::Entity::insert_many(...) call: map normalized tags into a Vec<server_tag::ActiveModel> (setting server_id via Set(server_id.to_string()) and tag via Set(tag.clone())), then call insert_many(&txn).await once inside the transaction; also guard against an empty collection by checking models.is_empty() before calling insert_many (or verify sea-orm 1.0 accepts empty iterators and skip the guard if confirmed) to avoid a no-op or runtime error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/routes/_authed/servers/index.cells.test.tsx`:
- Around line 11-17: The test's mocked Link component uses the type
React.ReactNode but React is not imported, causing typecheck failures under
strict mode; add a type-only import for React (e.g., import type React from
'react') at the top of the file so the React.ReactNode reference in the vi.mock
callback is resolved; update the file containing the vi.mock block and ensure
the import appears before the mocked Link definition.
In `@crates/server/src/service/server_tag.rs`:
- Around line 12-15: In validate_tags, the length check currently uses raw.len()
and should be performed after normalizing (trim whitespace), filtering out empty
strings, and deduplicating to match frontend behavior; so first map raw through
.trim().to_string(), filter out "".to_string(), deduplicate (e.g., using a
HashSet or unique retain to preserve order), then check that the resulting
normalized vector’s length <= MAX_TAGS and return that normalized Vec<String> or
AppError::Validation if too many. Ensure you update the code paths around
validate_tags, MAX_TAGS and the AppError::Validation message to reflect the
normalized count check.
---
Outside diff comments:
In `@apps/web/src/hooks/use-servers-ws.ts`:
- Around line 103-120: In mergeServerUpdate, the current suppression logic
treats empty arrays as a static-default and ignores explicit "clear" updates for
fields like tags/features; update the guard so we only suppress when the
incoming payload lacks the key (e.g., use a check for incoming key absence such
as server[key] === undefined or Object.prototype.hasOwnProperty.call(server,
key) rather than Array.isArray(value) && value.length === 0), keeping
STATIC_FIELDS logic but allowing explicit empty arrays to overwrite existing
values; refer to mergeServerUpdate and STATIC_FIELDS to locate the change.
---
Nitpick comments:
In `@apps/web/src/components/server/server-edit-dialog.tsx`:
- Around line 129-140: The saveTags handler reverts the input on error but
doesn't reset the tagsDirty flag, causing a redundant PUT on the next submit; in
the saveTags function (which calls tagsMutation.mutateAsync and setTagsInput
with initialTags.join(', ')), after reverting set tagsDirty to false (use the
state setter for tagsDirty) so the UI reflects the canonical state and prevents
a no-op tags update on subsequent saves.
In `@apps/web/src/components/server/tag-chip.tsx`:
- Around line 12-19: Remove the redundant Math.trunc call in the hashTag
function: Math.imul already produces a signed 32-bit integer and adding
tag.charCodeAt(i) preserves integer semantics, so delete the Math.trunc(h) step
inside hashTag (the function that computes an index into PALETTE) and return
Math.abs(h) % PALETTE.length as before.
- Around line 32-44: The tag chips use the tag string as React key (in the
tags.map callback using key={tag}), which will break if duplicate tags appear;
update the rendering to guarantee unique keys by using a stable index or
composite key (e.g., `${tag}-${i}`) or deduplicate tags before mapping (e.g.,
create a unique list from tags) so that the span elements rendered in the
tags.map loop have unique keys; reference the tags.map callback and key={tag} in
tag-chip.tsx (and consider server_tag uniqueness in backend but implement the
frontend defensive fix here).
In `@apps/web/src/hooks/use-server-tags.test.tsx`:
- Around line 34-51: In the 'PUTs tags and patches both caches on success' test
for useUpdateServerTags, enhance the fetch mock (vi.spyOn(globalThis,
'fetch').mockImplementationOnce) to assert the request URL, method and body
shape before returning the Response: verify the request targets
/api/servers/srv-1/tags, that init.method === 'PUT', and that JSON.parse((init
as RequestInit).body) yields an object { tags: [...] } (matching the payload
passed to mutateAsync); after those assertions return the same 200 JSON response
as before.
In `@apps/web/src/hooks/use-servers-ws.test.ts`:
- Around line 213-249: Replace the duplicate fixture builder by extending the
existing makeServer helper to include the extra fields present in baseServer
(tags, features, cpu_cores, disk_read_bytes_per_sec, disk_write_bytes_per_sec
and any other fields from ServerMetrics), change makeServer’s signature to
accept Partial<ServerMetrics> overrides like baseServer, update all tests that
call baseServer to call makeServer instead, and then remove the baseServer
function to avoid drift between the two builders.
In `@apps/web/src/lib/traffic.ts`:
- Around line 17-23: Add a brief inline comment inside computeTrafficQuota
explaining that when entry is undefined the computed used value (netInTransfer +
netOutTransfer) is a lifetime total while when entry is present used is
cycle-scoped (entry.cycle_in + entry.cycle_out), and note that this can cause
long-running servers to saturate the quota bar against
DEFAULT_TRAFFIC_LIMIT_BYTES by design; update the comment near the used
computation and reference the entry, netInTransfer, netOutTransfer, and
DEFAULT_TRAFFIC_LIMIT_BYTES symbols so future readers understand the semantic
mismatch.
In `@apps/web/src/routes/_authed/servers/index.cells.test.tsx`:
- Around line 108-113: The test "clamps percentage to [0, 100]" calls render
twice into the same DOM so the second assertion can see both mounted trees; fix
by either splitting into two tests (e.g., two it blocks each rendering
MetricBarRow with pct=150 and pct=-5) or call react-testing-library
cleanup()/unmount() between the two render calls so each assertion only queries
the intended render; update references to MetricBarRow, render, screen.getByText
accordingly.
In `@apps/web/src/routes/_authed/servers/index.tsx`:
- Line 95: The table currently calls trafficOverview.find(...) per-cell which is
O(N·M); instead compute a Map keyed by server_id from the useTrafficOverview()
result and memoize it (e.g., const trafficMap = useMemo(() => new
Map(trafficOverview.map(e => [e.server_id, e])), [trafficOverview])) and then
replace all per-row trafficOverview.find(...) calls in your column cell
renderers with trafficMap.get(row.original.id) to make lookups O(1) and
stabilize the columns' dependencies; update every column/cell that references
trafficOverview.find (including the same uses in the columns definition) to use
trafficMap.get instead.
In `@crates/server/src/router/ws/browser.rs`:
- Around line 266-275: The tags query currently swallows errors via
unwrap_or_default() causing silent empty tag lists; replace that with explicit
error handling similar to ServerService::list_servers: await the
server_tag::Entity::find() call into a Result, on Err call tracing::error! with
context (e.g., "failed to query server tags") and the error, then set tags_rows
to an empty Vec; keep the subsequent loop that builds tags_by_server (the
HashMap<String, Vec<String>>) unchanged so browser behavior is preserved but
failures are logged.
In `@crates/server/src/service/server_tag.rs`:
- Around line 60-67: Replace the per-tag insert loop that builds
server_tag::ActiveModel for each tag and calls .insert(&txn).await with a single
server_tag::Entity::insert_many(...) call: map normalized tags into a
Vec<server_tag::ActiveModel> (setting server_id via Set(server_id.to_string())
and tag via Set(tag.clone())), then call insert_many(&txn).await once inside the
transaction; also guard against an empty collection by checking
models.is_empty() before calling insert_many (or verify sea-orm 1.0 accepts
empty iterators and skip the guard if confirmed) to avoid a no-op or runtime
error.
In `@docs/superpowers/plans/2026-04-16-servers-table-density.md`:
- Around line 1-784: This plan file is historical and should be marked as
superseded by the newer 2026-04-17 servers-table-density plan: add a short
one-paragraph banner immediately under the "# Servers Table Density & Disk I/O
Display Implementation Plan" heading that states this document is kept for
historical reference and has been superseded by the 2026-04-17 plan (include
that date/string so readers can find the newer doc), optionally add a
"Superseded" tag or front-matter line and update the top-level summary to point
readers to the newer plan, then commit with a descriptive message like "docs:
mark 2026-04-16 servers table density plan as superseded by 2026-04-17".
In `@docs/superpowers/plans/2026-04-17-servers-table-row-visual-redesign.md`:
- Around line 1-2644: The empty-test stub crates/server/tests/server_tags.rs
(containing only a comment) produces a linked test binary and should be removed;
delete that file (or convert it to a non-test helper module) so only the real
tags tests live in crates/server/tests/integration.rs, then git rm the stub and
commit (e.g. "test(server): remove empty server_tags.rs stub — tests live in
integration.rs"). Ensure integration.rs contains the full tags tests and re-run
cargo test to verify no extra empty test binary remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb36901e-ee03-4e52-abb6-9363e7ccfae2
📒 Files selected for processing (31)
apps/web/src/components/dashboard/dashboard-switcher.tsxapps/web/src/components/server/server-card.tsxapps/web/src/components/server/server-edit-dialog.tsxapps/web/src/components/server/status-dot.test.tsxapps/web/src/components/server/status-dot.tsxapps/web/src/components/server/tag-chip.test.tsxapps/web/src/components/server/tag-chip.tsxapps/web/src/hooks/use-server-tags.test.tsxapps/web/src/hooks/use-server-tags.tsapps/web/src/hooks/use-servers-ws.test.tsapps/web/src/hooks/use-servers-ws.tsapps/web/src/lib/traffic.test.tsapps/web/src/lib/traffic.tsapps/web/src/locales/en/servers.jsonapps/web/src/locales/zh/servers.jsonapps/web/src/routes/_authed/servers/index.cells.test.tsxapps/web/src/routes/_authed/servers/index.cells.tsxapps/web/src/routes/_authed/servers/index.tsxcrates/common/src/types.rscrates/server/src/router/api/mod.rscrates/server/src/router/api/server_tag.rscrates/server/src/router/ws/browser.rscrates/server/src/service/agent_manager.rscrates/server/src/service/mod.rscrates/server/src/service/server_tag.rscrates/server/tests/integration.rsdocs/superpowers/plans/2026-04-16-servers-table-density.mddocs/superpowers/plans/2026-04-17-servers-table-row-visual-redesign.mddocs/superpowers/specs/2026-04-15-servers-table-density-design.mddocs/superpowers/specs/2026-04-17-servers-table-row-visual-redesign-design.mdtests/servers/table-row-visual-redesign.md
| vi.mock('@tanstack/react-router', () => ({ | ||
| Link: ({ children, ...props }: { children?: React.ReactNode; [k: string]: unknown }) => ( | ||
| <a data-testid="server-link" href={`/servers/${props.params && (props.params as { id: string }).id}`}> | ||
| {children} | ||
| </a> | ||
| ) | ||
| })) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm React is not imported anywhere in the test file and verify tsconfig
rg -n "from 'react'" apps/web/src/routes/_authed/servers/index.cells.test.tsx || echo "no react import"
fd -t f tsconfig.json apps/webRepository: ZingerLittleBee/ServerBee
Length of output: 107
🏁 Script executed:
cat -n apps/web/src/routes/_authed/servers/index.cells.test.tsx | head -30Repository: ZingerLittleBee/ServerBee
Length of output: 1452
🏁 Script executed:
cat apps/web/tsconfig.jsonRepository: ZingerLittleBee/ServerBee
Length of output: 281
🏁 Script executed:
rg -A 5 "React\.ReactNode" apps/web/src/routes/_authed/servers/index.cells.test.tsxRepository: ZingerLittleBee/ServerBee
Length of output: 301
🏁 Script executed:
cat apps/web/tsconfig.app.jsonRepository: ZingerLittleBee/ServerBee
Length of output: 898
Add missing React type import for React.ReactNode reference.
Line 12 uses React.ReactNode in the type annotation, but React is never imported. Although jsx: "react-jsx" in tsconfig auto-imports React for JSX syntax, it does not auto-import for type references. With strict: true enabled, this causes a typecheck failure.
🔧 Proposed fix
import { render, screen } from '@testing-library/react'
+import type { ReactNode } from 'react'
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
@@
vi.mock('@tanstack/react-router', () => ({
- Link: ({ children, ...props }: { children?: React.ReactNode; [k: string]: unknown }) => (
+ Link: ({ children, ...props }: { children?: ReactNode; [k: string]: unknown }) => (📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vi.mock('@tanstack/react-router', () => ({ | |
| Link: ({ children, ...props }: { children?: React.ReactNode; [k: string]: unknown }) => ( | |
| <a data-testid="server-link" href={`/servers/${props.params && (props.params as { id: string }).id}`}> | |
| {children} | |
| </a> | |
| ) | |
| })) | |
| import { render, screen } from '@testing-library/react' | |
| import type { ReactNode } from 'react' | |
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' | |
| vi.mock('@tanstack/react-router', () => ({ | |
| Link: ({ children, ...props }: { children?: ReactNode; [k: string]: unknown }) => ( | |
| <a data-testid="server-link" href={`/servers/${props.params && (props.params as { id: string }).id}`}> | |
| {children} | |
| </a> | |
| ) | |
| })) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/routes/_authed/servers/index.cells.test.tsx` around lines 11 -
17, The test's mocked Link component uses the type React.ReactNode but React is
not imported, causing typecheck failures under strict mode; add a type-only
import for React (e.g., import type React from 'react') at the top of the file
so the React.ReactNode reference in the vi.mock callback is resolved; update the
file containing the vi.mock block and ensure the import appears before the
mocked Link definition.
| pub fn validate_tags(raw: &[String]) -> Result<Vec<String>, AppError> { | ||
| if raw.len() > MAX_TAGS { | ||
| return Err(AppError::Validation(format!("at most {MAX_TAGS} tags"))); | ||
| } |
There was a problem hiding this comment.
Consider trimming/filtering before the count check.
raw.len() > MAX_TAGS rejects inputs that would normalize to ≤ 8 after trimming empties and dedup (e.g., 9 entries where 2 are duplicates or whitespace-only). The frontend's parseTagsInput applies the count check after dedup, so the client and server disagree on what constitutes a "too many" error for edge inputs. Minor UX inconsistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/server/src/service/server_tag.rs` around lines 12 - 15, In
validate_tags, the length check currently uses raw.len() and should be performed
after normalizing (trim whitespace), filtering out empty strings, and
deduplicating to match frontend behavior; so first map raw through
.trim().to_string(), filter out "".to_string(), deduplicate (e.g., using a
HashSet or unique retain to preserve order), then check that the resulting
normalized vector’s length <= MAX_TAGS and return that normalized Vec<String> or
AppError::Validation if too many. Ensure you update the code paths around
validate_tags, MAX_TAGS and the AppError::Validation message to reflect the
normalized count check.
# Conflicts: # apps/web/src/hooks/use-servers-ws.test.ts # crates/server/src/router/api/mod.rs # crates/server/src/router/ws/browser.rs # crates/server/tests/integration.rs
There was a problem hiding this comment.
🧹 Nitpick comments (5)
apps/web/src/hooks/use-servers-ws.test.ts (1)
216-284: LGTM — static-fields guard coverage is solid.The new
baseServerhelper andmergeServerUpdate static-fields guardsuite correctly exercise each branch of theSTATIC_FIELDSguard inuse-servers-ws.ts(empty-array fortags/features,nullforcpu_cores, and the non-empty overwrite path). Using a second helper with zero/null defaults makes the static-default semantics explicit and avoids accidental interference frommakeServer's populated values.One nit you can take or leave: since
baseServerin the incoming frames also carriestags: []andfeatures: []by default, the guard happens to also shield those from clobbering the prev values in thecpu_corestest — which is the desired behavior, but it means that test only isolatescpu_coresbecause of the guard itself. If you ever loosen the guard, this test would still pass oncpu_coresalone; consider assertingresult[0].tagsandresult[0].featuresalongsidecpu_coresto lock in the full contract.♻️ Optional tightening for the cpu_cores test
it('preserves prior cpu_cores when incoming frame carries cpu_cores: null', () => { - const prev = [baseServer({ cpu_cores: 8 })] - const incoming = [baseServer({ cpu_cores: null, cpu: 5 })] + const prev = [baseServer({ cpu_cores: 8, tags: ['prod'], features: ['docker'] })] + const incoming = [baseServer({ cpu_cores: null, cpu: 5 })] const result = mergeServerUpdate(prev, incoming) expect(result[0].cpu_cores).toBe(8) + expect(result[0].tags).toEqual(['prod']) + expect(result[0].features).toEqual(['docker']) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/use-servers-ws.test.ts` around lines 216 - 284, The cpu_cores test can pass unintentionally because baseServer for incoming frames defaults tags: [] and features: [], so the STATIC_FIELDS guard also protects those fields; update the 'preserves prior cpu_cores when incoming frame carries cpu_cores: null' test in the mergeServerUpdate suite to also assert that tags and features were preserved (e.g., expect(result[0].tags) and expect(result[0].features)) alongside the cpu_cores expectation so the test locks in the full static-fields contract; locate the test by the describe name 'mergeServerUpdate static-fields guard' and the it block name, and modify only that test assertions.crates/server/tests/integration.rs (3)
3882-3896: Edge case: validation error status assumption.The comment correctly notes
AppError::Validation→ 422. Good. One minor gap: none of the new tests assert that the 8-tag boundary is accepted (only that 9 is rejected). Consider adding a positive test with exactlyMAX_TAGSentries to lock in the inclusive/exclusive boundary, which is a common regression point.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/server/tests/integration.rs` around lines 3882 - 3896, Add a positive boundary test to ensure MAX_TAGS (8) is accepted: create a new test (or extend admin_put_rejects_too_many_tags) that builds a Vec<String> of length MAX_TAGS (use the constant or hardcode 8 if no constant exported), sends the same PUT to /api/servers/{server_id}/tags and asserts the response is success (e.g., 200 or 204) instead of 422; reference the existing test function admin_put_rejects_too_many_tags and the same register_agent/login_admin/http_client flow to reuse setup so the inclusive/exclusive boundary is locked in.
3987-4014: Cookie parsing will break ifSet-Cookieever emits attributes in a different order.
set_cookie.split(';').next()assumes thesession_token=...pair is the first segment, which is true today but is an implicit coupling to response header formatting. Since you already havehttp_client()withcookie_store(true), a cleaner approach is to log in via that client and then read the stored cookie viareqwest::cookie::Jaror attach the cookie jar to the WS request. Minor — the current form is acceptable for a test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/server/tests/integration.rs` around lines 3987 - 4014, Replace brittle manual parsing of Set-Cookie by using a reqwest cookie jar and the cookie-aware client: create a reqwest::cookie::Jar (Arc::new(reqwest::cookie::Jar::default())), build the client with ClientBuilder::cookie_provider(...) (or cookie_store(true) with an accessible jar), perform the login request using that client (replace raw_client/login_resp), then read the stored cookie from the jar via jar.cookies(&base_url.parse().unwrap()) (or jar.cookies(&login_url)) and use that value when calling into_client_request()/request.headers_mut().insert(...); update references to raw_client, login_resp, set_cookie and cookie_value accordingly.
3788-3818:register_memberhelper duplicates the admin+member creation pattern already open-coded in several tests.Tests like
test_member_read_only,test_file_write_requires_admin,test_dashboard_rbac_member_cannot_write,test_recovery_*all repeat this pattern. Since you've introduced a helper, a follow-up refactor to replace those open-coded sequences withregister_memberwould reduce ~300 lines of duplication. Not blocking for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/server/tests/integration.rs` around lines 3788 - 3818, The tests duplicate the admin+member creation/login sequence that the async helper register_member already encapsulates; replace the repeated open-coded sequences in tests such as test_member_read_only, test_file_write_requires_admin, test_dashboard_rbac_member_cannot_write, and the test_recovery_* cases with calls to register_member(base_url) to obtain an authenticated reqwest::Client for the member, removing the duplicated POST /api/users and POST /api/auth/login blocks and adjusting assertions to use the helper's result.crates/server/src/router/ws/browser.rs (1)
279-288: Tag query is unbounded — fetches every row inserver_tagon each FullSync / resync.
server_tag::Entity::find()without a filter pulls the entire table. FullSync runs on every browser WS connect and on broadcast lag (line 152), so with many servers × tags this scales with total rows regardless of how many servers are visible. Given the per-server limits (≤8 tags, ≤16 chars), the absolute ceiling is small, but it's still worth scoping the query to just the server IDs being rendered.♻️ Suggested scoping
- let tags_rows = server_tag::Entity::find() - .order_by_asc(server_tag::Column::ServerId) - .order_by_asc(server_tag::Column::Tag) - .all(&state.db) - .await - .unwrap_or_default(); + let server_ids: Vec<String> = servers.iter().map(|s| s.id.clone()).collect(); + let tags_rows = server_tag::Entity::find() + .filter(server_tag::Column::ServerId.is_in(server_ids)) + .order_by_asc(server_tag::Column::ServerId) + .order_by_asc(server_tag::Column::Tag) + .all(&state.db) + .await + .unwrap_or_default();Also consider swallowing the error silently (
unwrap_or_default) vs. logging it like thelist_serversbranch above — a persistent DB error here would result in "no tags ever" with no visibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/server/src/router/ws/browser.rs` around lines 279 - 288, The tag query currently uses server_tag::Entity::find() which fetches the entire table on every FullSync/resync; change the query to filter by only the server IDs being rendered (e.g. apply a .filter(server_tag::Column::ServerId.is_in(server_ids)) where server_ids are collected from the servers list used in the FullSync) so tags_rows only contains relevant rows, populate tags_by_server from that result, and replace unwrap_or_default() with proper error handling/logging consistent with the list_servers branch so DB errors are logged instead of silently swallowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/hooks/use-servers-ws.test.ts`:
- Around line 216-284: The cpu_cores test can pass unintentionally because
baseServer for incoming frames defaults tags: [] and features: [], so the
STATIC_FIELDS guard also protects those fields; update the 'preserves prior
cpu_cores when incoming frame carries cpu_cores: null' test in the
mergeServerUpdate suite to also assert that tags and features were preserved
(e.g., expect(result[0].tags) and expect(result[0].features)) alongside the
cpu_cores expectation so the test locks in the full static-fields contract;
locate the test by the describe name 'mergeServerUpdate static-fields guard' and
the it block name, and modify only that test assertions.
In `@crates/server/src/router/ws/browser.rs`:
- Around line 279-288: The tag query currently uses server_tag::Entity::find()
which fetches the entire table on every FullSync/resync; change the query to
filter by only the server IDs being rendered (e.g. apply a
.filter(server_tag::Column::ServerId.is_in(server_ids)) where server_ids are
collected from the servers list used in the FullSync) so tags_rows only contains
relevant rows, populate tags_by_server from that result, and replace
unwrap_or_default() with proper error handling/logging consistent with the
list_servers branch so DB errors are logged instead of silently swallowed.
In `@crates/server/tests/integration.rs`:
- Around line 3882-3896: Add a positive boundary test to ensure MAX_TAGS (8) is
accepted: create a new test (or extend admin_put_rejects_too_many_tags) that
builds a Vec<String> of length MAX_TAGS (use the constant or hardcode 8 if no
constant exported), sends the same PUT to /api/servers/{server_id}/tags and
asserts the response is success (e.g., 200 or 204) instead of 422; reference the
existing test function admin_put_rejects_too_many_tags and the same
register_agent/login_admin/http_client flow to reuse setup so the
inclusive/exclusive boundary is locked in.
- Around line 3987-4014: Replace brittle manual parsing of Set-Cookie by using a
reqwest cookie jar and the cookie-aware client: create a reqwest::cookie::Jar
(Arc::new(reqwest::cookie::Jar::default())), build the client with
ClientBuilder::cookie_provider(...) (or cookie_store(true) with an accessible
jar), perform the login request using that client (replace
raw_client/login_resp), then read the stored cookie from the jar via
jar.cookies(&base_url.parse().unwrap()) (or jar.cookies(&login_url)) and use
that value when calling into_client_request()/request.headers_mut().insert(...);
update references to raw_client, login_resp, set_cookie and cookie_value
accordingly.
- Around line 3788-3818: The tests duplicate the admin+member creation/login
sequence that the async helper register_member already encapsulates; replace the
repeated open-coded sequences in tests such as test_member_read_only,
test_file_write_requires_admin, test_dashboard_rbac_member_cannot_write, and the
test_recovery_* cases with calls to register_member(base_url) to obtain an
authenticated reqwest::Client for the member, removing the duplicated POST
/api/users and POST /api/auth/login blocks and adjusting assertions to use the
helper's result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc52a25f-3f90-4cb2-8c28-d43d7391903d
📒 Files selected for processing (12)
apps/web/src/components/dashboard/widgets/stat-number.test.tsxapps/web/src/components/server/server-card.test.tsxapps/web/src/hooks/use-realtime-metrics.test.tsxapps/web/src/hooks/use-servers-ws.test.tsapps/web/src/hooks/use-servers-ws.tsapps/web/src/locales/en/servers.jsonapps/web/src/locales/zh/servers.jsoncrates/server/src/router/api/mod.rscrates/server/src/router/ws/browser.rscrates/server/src/service/agent_manager.rscrates/server/src/service/mod.rscrates/server/tests/integration.rs
✅ Files skipped from review due to trivial changes (6)
- crates/server/src/service/mod.rs
- apps/web/src/components/dashboard/widgets/stat-number.test.tsx
- apps/web/src/components/server/server-card.test.tsx
- apps/web/src/locales/en/servers.json
- apps/web/src/hooks/use-realtime-metrics.test.tsx
- apps/web/src/locales/zh/servers.json
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/server/src/router/api/mod.rs
- apps/web/src/hooks/use-servers-ws.ts
1 participant
Summary
Rebuilds every row in
/servers?view=tableso each metric cell is a two-line block (icon + bar + % on top, compact sub-data below), replaces the status text badge with a pulsing dot column, showsserver_tagsunder the server name, and surfaces monthly traffic quota usage as a bar in the Network cell. Also extends the backendServerStatusWS payload withtags+cpu_cores, adds aserver_tagsREST surface (GET /api/servers/:id/tags,PUT /api/servers/:id/tags), and wires a tag editor intoServerEditDialog.MetricBarRow,StatusDot,TagChipRow,CpuCell/MemoryCell/DiskCell/NetworkCell/UptimeCell/NameCell, sharedlib/traffic.ts)ServerStatus.full_sync,service/server_tag.rs(validation + CRUD), REST router with admin-only write, integration tests for RBAC/validation/full_sync payloadServerEditDialogwith client-side validation, sequential PATCH→PUT save, and revert-on-PUT-failure UXSpec & plan
docs/superpowers/specs/2026-04-17-servers-table-row-visual-redesign-design.mddocs/superpowers/plans/2026-04-17-servers-table-row-visual-redesign.mdTest plan
bun run --cwd apps/web test→ 382/382 green (+35 new)bun run typecheck→ exit 0bun x ultracite check apps/web/src→ cleancargo clippy --workspace -- -D warnings→ cleancargo test --workspace→ 57 pass + 9 new integration tests for/api/servers/:id/tagsandbrowser_ws full_sync includes tags + cpu_corestests/servers/table-row-visual-redesign.mdNotes
test_server_detail_returns_runtime_capability_fields:CAP_DEFAULT56 vs 60) confirmed present on commit 3700922 before any of this work. Tracked separately.bun run buildTS inference errors in existing test fixtures (also present on 3700922) are unchanged by this PR.Summary by CodeRabbit
New Features
UI/UX Improvements
Localization