Repository navigation
feat(web): add an agent office visualization to the right panel - #200
tyler-barton-horizon wants to merge 3 commits into
Conversation
Jacksondr5
left a comment
There was a problem hiding this comment.
Review by Codex gpt-6-astra (high reasoning), delegated and relayed via Claude Code. Anchored to 046fc0049. Findings were verified by source inspection and by executing the engine in memory; nothing here is speculative.
Verdict
Request changes. The feature is well isolated and correctly adds no new telemetry harvesting. But it invents activity data, repaints continuously even when the room is quiet, and has three reproducible lifecycle bugs. Inline comments carry the specifics; the items below have no single anchor.
Isolation
Good. Four production files change, about 50 lines total (ChatView.tsx, RightPanelTabs.tsx, rightPanelStore.ts, _chat.pull-requests.tsx). None of them acquire office concepts. Contracts, server, adapters, mobile, and packages/client-runtime are untouched. Removal would be the new files plus those small integrations. One suggestion: lazy-import AgentVisualizationPanel so the renderer stays out of the initial chat bundle.
Telemetry reusability
The reusable layer is the existing subagent runtime model in client-runtime, and the office consumes the same derived AgentPanelModel as the Agents surface. A future dashboard should read that model, not office lanes. The problem is the inference layered on top inside the renderer; see the inline comment on roleForAgent.
Simplicity / YAGNI
The line count overstates the architecture. Pathfinding, depth ordering, sprite composition, and seating all back visible behavior, and the medics were asked for. The cuts worth making are the dead code called out inline and the decorative scope: two clocks, role-specific animated screens, coffee steam, daylight and window calculations, lamp gradients, and lighting flicker. One static clock and status-driven screens keep the point of the feature and are also what lets the room settle so the repaint blocker can be fixed.
Description and process
- The description claims tabs tests cover the new surface kind.
RightPanelTabs.test.tsxonly gains required props; no visualization test is added. The store singleton test is real. - The description says fixed 60 Hz simulation. Movement uses fixed accumulator steps, but timers receive wall-clock
now, so it is not a fixed-step clock. - Screenshots and video are omitted. AGENTS.md requires before/after images for UI changes and a short video for motion.
Docs
docs/internals/office-art-direction.mdis mostly discarded design history, reference research, and rendering and verification instructions that the code already answers. Keep only durable asset constraints (frame anchors, attribution) and put them near the assets.docs/user/agent-visualization.mdspends most of its length on furniture, layout, and animation, mentions "pets" and "Ops" alternatives that do not exist, and never says how to open the panel. Rewrite around opening, reading, selecting, and closing.
| ]; | ||
| if (agents.length === 0) return []; | ||
|
|
||
| const staff: Lane[] = [ |
There was a problem hiding this comment.
Blocker: fabricated telemetry. This synthetic main lane is always working with bubble text routing, regardless of what the parent thread is actually doing. That conflicts with the fork principle that the platform shows facts, never guesses. Either derive the Lead's status from real parent-thread activity or drop the lane.
There was a problem hiding this comment.
Dropped in 11258c267d. The office shows only real agent lanes now; no synthetic Lead, no invented status. The corner office remains as scenery. If a Lead ever comes back it will be driven by the parent thread's actual run status.
| }); | ||
| visibilityObserver.observe(host); | ||
| resize(); | ||
| const loop = (now: number) => { |
There was a problem hiding this comment.
Blocker: continuous repainting. The loop clears and redraws the full canvas every frame at the 30 fps cap with no settled-state exit, and clocks, coffee steam, lamp flicker, and idle wandering guarantee the room never settles. AGENTS.md forbids continuously repainting animations; a frame cap does not satisfy that.
The description also says the room including furniture is cached once. Only floors and walls are. Furniture becomes paint callbacks that are merged with actors, depth-sorted, and repainted every frame (agentOfficeEngine.ts around line 1297, const layers = [...furniture]).
Suggested shape: stop the RAF loop when nothing is in transition, and invalidate on lane changes, pointer interaction, resize, or a bounded transition (walk in, walk out, rescue). Cutting the purely decorative motion is what makes that possible.
There was a problem hiding this comment.
Addressed in c5d7927; see the reply on the re-review thread for the full account. Settled rooms schedule no frames.
There was a problem hiding this comment.
Also see 002c6b2f39: ambient events returned as rare, bounded transitions on the settle model (details on the re-review thread).
| }; | ||
| } | ||
|
|
||
| function rescueTick(sim: Simulation, lanes: ReadonlyArray<Lane>, now: number) { |
There was a problem hiding this comment.
Should-fix: failed agents respawn forever. Rescue completion marks the actor gone and updateActors deletes it, but only departing lanes get recorded in sim.departed. On the next tick this loop calls getActor for the still-failed lane, recreates the actor, and the medics come back. Running the engine in memory for 100 simulated seconds after one failure recreated the actor 77 times.
Record completed failure departures too, and clear that entry only when the agent genuinely reactivates.
There was a problem hiding this comment.
Fixed in 6cac7e6. Departures are now recorded for failed lanes too (the medics' exit adds the id), the rescue scan skips departed lanes so getActor cannot recreate the actor, and the entry clears only when the lane is live again. Regression test runs 6000 ticks after a failure and asserts zero respawns.
| }, | ||
| ]; | ||
|
|
||
| const agentLanes = agents.slice(0, 22).map((agent, index) => ({ |
There was a problem hiding this comment.
Should-fix: the cap hides live work. agents.slice(0, 22) includes retained terminal records, and the panel model orders direct agents by first observation. With 22 completed agents and one running one, the running agent is missing from both canvas and picker, and the only working lane on screen is the synthetic Lead. There is no overflow indication.
Prioritize live agents before applying the cap, and surface the omitted count.
There was a problem hiding this comment.
Fixed in 6cac7e6. Agents are ordered working, waiting, pending, idle, failed, departing before the 22 cap (stable within each group), and the picker bar shows an "N more not shown" count.
| return [...group.phases.flatMap((phase) => phase.members), ...group.unphasedMembers]; | ||
| } | ||
|
|
||
| function roleForAgent(agent: RuntimeSubagent): string { |
There was a problem hiding this comment.
Should-fix: guessed activity replaces lifecycle facts. roleForAgent and laneStatus infer meaning from substrings and the resulting Lane drops timestamps and the real status. Concretely: a pending agent renders as working; any progress text containing think becomes a thinking label; hint.includes("pm") also matches npm, so a package install turns an agent into a Jira worker.
The reusable data layer already exists in packages/client-runtime/src/state/subagentRuntime.ts and this PR correctly adds no new harvesting. Keep it that way: preserve the actual lifecycle status here (including pending), and keep the cosmetic role picking clearly decorative and local to the office. If semantic activity inference becomes a real need, it belongs as a small tested selector beside subagentRuntime.ts, not inside the renderer.
There was a problem hiding this comment.
Fixed in 6cac7e6. Research table claims are held per agent id in the simulation and released only when that agent stops researching; new claims start at the slot's table and take the next free one. Added the colliding-slot regression (slots 0 and 2 with two tables): slot 2 stays at table 1 when slot 0 leaves.
| * matching its slot and walks forward past taken ones, so seats stay put | ||
| * when other researchers come and go. Overflow researchers keep their desks. | ||
| */ | ||
| function researchTableFor(lane: Lane, lanes: ReadonlyArray<Lane>) { |
There was a problem hiding this comment.
Should-fix: research tables reshuffle. The doc comment says seats stay put, but this is first-fit allocation recomputed on every call rather than a retained claim. With two tables and researchers in slots 0 and 2, slot 2 sits at table 1; when slot 0 stops researching, slot 2 moves to table 0. Reproduced directly. The existing test uses non-colliding slots and so misses it.
Retain assignments by agent id until released, and add a regression test with colliding slot residues.
There was a problem hiding this comment.
Fixed in 6cac7e6. Lane status is a direct mapping of the runtime status, with pending kept as its own state and the progress-text "thinking" inference removed (the whiteboard trip went with it). Role picking stays decorative and local to the office, now on whole-word matches so npm no longer reads as PM.
| import type { AgentPanelModel } from "@t3tools/client-runtime/state/subagentRuntime"; | ||
| import { X } from "lucide-react"; | ||
| import { useEffect, useMemo, useRef, useState } from "react"; | ||
| import officeCreditsUrl from "../assets/office/lpc/CREDITS.md?url"; |
There was a problem hiding this comment.
Should-fix: attribution is incomplete. Only CREDITS.md ships through the asset graph. The six credits-*.txt files that CREDITS.md points to are neither imported nor linked, and the view links the OGA-BY FAQ rather than the license. OGA-BY 3.0 sections 3(b) and 4 require author, title, source, and a license copy or URI. Bundle one complete attribution document and link https://static.opengameart.org/OGA-BY-3.0.txt directly.
There was a problem hiding this comment.
Fixed in 6cac7e6. CREDITS.md is now the single attribution document, reproducing the six upstream credit files verbatim (artists, titles, sources, per-item licenses), and the view links https://static.opengameart.org/OGA-BY-3.0.txt directly next to the credits link. The separate txt files are removed.
| canvas.addEventListener("pointermove", pointerMove); | ||
| canvas.addEventListener("pointerleave", pointerLeave); | ||
| canvas.addEventListener("click", select); | ||
| void loadOfficeSheets() |
There was a problem hiding this comment.
Should-fix: load failure looks like endless loading. The catch only logs a warning, so drawWorld keeps showing "Setting up the office…" indefinitely. Render an explicit failure state with a retry.
There was a problem hiding this comment.
Fixed in 6cac7e6. Load failure now shows an explicit message with a Retry button; the cached loader promise already resets on rejection so retry re-fetches.
| gone: boolean; | ||
| exiting: boolean; | ||
| cafeSpot: Tile | null; | ||
| seatDir: Dir | null; |
There was a problem hiding this comment.
Nit: dead state and unreachable branches. seatDir is write-only (assigned at 915, 959, 964, never read). The renderer branches for map tokens w, f, and e (373–384) are unreachable because officeLayout never emits them. Simulation.departed is optional with ??= even though the only constructor initializes it. Actor movement and rescue movement also duplicate the same waypoint interpolation; a small shared helper would remove that without adding a framework.
There was a problem hiding this comment.
Fixed in 6cac7e6. Removed seatDir, the unreachable w/f/e tokens, and the optional departed set; agents and medics share one stepAlongPath helper.
bryantderosier
left a comment
There was a problem hiding this comment.
Second review round, anchored to 11258c26. I ran a three-seat review crew (code quality, correctness/performance, security) and de-duplicated against everything already posted here, so this only carries findings that are new. The repaint blocker from the first round is still open and I am not re-raising it. Each finding was verified by reading the source at this commit and, where cheap, by running the engine or officeLayout in isolation; nothing here is speculative.
Confirmed fixed from round one
Failed agents no longer respawn (hasLeft + sim.departed, regression test present). The cap now sorts live agents first and shows the omitted count. Role picking uses word boundaries and npm no longer reads as PM. Research claims are held per agent id. Load failure shows a Retry that clears the rejected promise. seatDir, the dead map tokens, and the duplicated waypoint code are gone. The synthetic Lead is gone. Attribution is complete: CREDITS.md is whitespace-identical to the six upstream credit files, every entry carries author, title, source and license, and the panel links the OGA-BY 3.0 text directly.
New findings without a single anchor
Should-fix: FORK.md is not updated and the new modules sit beside upstream components. git diff --stat 42a17bfd 11258c26 -- FORK.md is empty. FORK.md requires every touch of an upstream file to be a recorded appended case in the inventory, and this PR appends to four upstream files (ChatView.tsx, RightPanelTabs.tsx + test, rightPanelStore.ts + test, routes/_chat.pull-requests.tsx). The edits themselves are minimal and mirror the upstream Agents surface, which is good. But AgentVisualizationPanel.tsx, agentOfficeEngine.ts, officeLayout.ts, and officeSheetAssets.ts live in apps/web/src/components/ where every other fork-owned web module lives under apps/web/src/j5/. On the next upstream merge nothing marks these as ours. Move the four modules (and the assets) under apps/web/src/j5/office/ and add the inventory entry. Making the two new props optional would also drop the pull-requests route and the tabs test from the footprint.
Nit: docs index. Neither docs/user/agent-visualization.md nor the internals page is linked from docs/README.md, so the user page is unreachable from the docs entry point. personas.md and artifacts.md both have entries there.
Check: CREDITS.md line 7 says "The PNG files are unmodified upstream images." The six furniture PNGs are byte-identical to upstream (sha256 checked). The eighteen character sheets are not: body-walk.png is 19247 bytes where the upstream Body 01/02 walk sheets are 16047/15809 bytes at the same 512x256. They may be a different variant or a re-export. Obligations are still met because adaptation is disclosed in the next sentence, but the wording should match what actually shipped.
Left alone on purpose. Per-frame createLinearGradient/createRadialGradient in drawWindows and drawDeskLamps is real (up to 22 gradient objects per frame in the evening) but it disappears with the decorative cut already requested in round one, so I am not filing it separately.
Review by Claude Fable 5.1 in Claude Code, orchestrating a three-seat J5 review crew.
| const initialSize = roomSize(); | ||
| const runtime: { office: Office; sim: Simulation } = (() => { | ||
| const office = createOffice( | ||
| officeLayout(initialSize.width, initialSize.height, lanesRef.current.length - 1), |
There was a problem hiding this comment.
Blocker: off-by-one desk count seats two agents on one desk. This still passes lanes.length - 1 (here and at line 90), which is the leftover from when lane 0 was the synthetic Lead that this very commit removed. officeLayout allocates max(2, min(22, agentCount)) desks and zoneTarget seats slot s at DESK_SPOTS[s % DESK_SPOTS.length], so with N lanes the last agent wraps onto desk 0.
Reproduced with three working lanes, slots 0-2: officeLayout(1056, 624, 2) yields 2 desks and after 2400 ticks actors a0 and a2 settle on the identical tile. Same for 4, 5, and 22 agents (21 desks, a0/a21 share). The focus highlight at agentOfficeEngine.ts:1279 uses DESK_SPOTS[focused.lane.slot] without the modulo, so clicking the last agent draws no station box at all. Research table count is derived from the wrong desk count too. This also falsifies the PR body ("Every agent gets a desk") and the internals doc.
Pass lanes.length, and add a test that feeds N working lanes through officeRoster -> officeLayout -> createOffice and asserts N distinct seated tiles. The existing layout test calls officeLayout with the count directly and so never exercises this contract.
There was a problem hiding this comment.
Fixed in 6bd6393. The panel passes the full lane count (both call sites). Desks are also owned per agent id now, and the highlight uses the owned desk rather than an unmodded slot lookup. Test: 3, 5, and 22 agents through officeRoster → officeLayout → createOffice all end seated at distinct tiles.
| id: agent.id, | ||
| label: agent.title.replace(/^Subagent:\s*/i, ""), | ||
| role: roleForAgent(agent), | ||
| slot: index, |
There was a problem hiding this comment.
Should-fix: desk slots follow the status-sorted index, so every status change reshuffles the room. slot: index is assigned after the STATUS_PRIORITY sort. Reproduced: A, B, C all working -> slots {A:0, B:1, C:2}. A flips to waiting -> {B:0, C:1, A:2}. Running the sim across that flip, all three actors stand up and walk to a different desk. Every completion likewise shifts every remaining agent one desk left.
This is the desk version of the research-table reshuffle the last round fixed with researchClaims. Use the priority sort only to decide who survives the 22 cap, then assign slot from stable model order, or hold a per-id desk claim in the simulation the way research tables do. Add a regression test for slot stability across a status flip.
There was a problem hiding this comment.
Fixed in 6bd6393. The priority sort now decides admission only; slots are assigned from model order. Desks are held per agent id in sim.deskClaims like research tables. Tests: roster slots are identical across a status flip, and a seated trio stays put (no tile change, nobody moving) while one agent flips to waiting.
| const researchCount = Math.max(1, Math.round(deskCount / 3)); | ||
| const stationCount = deskCount + researchCount; | ||
| const aspect = Math.max(0.3, width / Math.max(height, 1)); | ||
| let columns = Math.max(20, Math.ceil(20 * aspect), Math.min(56, Math.round(width / 32))); |
There was a problem hiding this comment.
Should-fix: aspect ratio has no upper bound, so a short host produces a multi-gigabyte room canvas. aspect is clamped only from below (0.3). Math.ceil(20 * aspect) grows without limit and sits inside the max, so the Math.min(56, ...) cap never applies. The panel clamps room height to a minimum of 1, so a wide, short host is reachable.
Measured by running this module at this commit: 1200x1 -> 24000 columns x 20 rows, and cacheRoom allocates a 768000x640 offscreen canvas (about 1.9 GiB of pixels, past every browser's max canvas dimension, so drawImage(room) throws or paints nothing). 2560x1 with 22 agents -> 51200 columns, about 4 GiB. Even a plausible 1400x420 gives 67 columns, above the intended 56. drawSheetRoom also runs DESK_SPOTS.some() per tile, so each layout-key change during a drag re-walks the whole grid.
Desktop is protected by the 620px window minHeight. In a browser the window can be shrunk to about 100px tall, and any transient zero-height layout with nonzero width hits it. One line fixes it: clamp aspect above (something like Math.min(3, ...)) or clamp columns after the max().
There was a problem hiding this comment.
Fixed in 6bd6393. Aspect is clamped to [0.3, 3] and columns to [20, 56] after the max; when width is capped the growth loop deepens instead of widening. 2560×1 with 22 agents now yields 56 columns. Test added for the degenerate host. The per-tile DESK_SPOTS.some() in the room painter is replaced by a precomputed carpet set.
| ctx.beginPath(); | ||
| ctx.rect(0, 0, runtime.office.width, runtime.office.height); | ||
| ctx.clip(); | ||
| if (focusRef.current && !runtime.sim.actors.has(focusRef.current)) { |
There was a problem hiding this comment.
Should-fix: a failed agent that the medics carried out can never be selected. After the rescue completes the actor is deleted and the id lives only in sim.departed, but pickableLanes (line 205) excludes only departing, so the failed lane stays in the picker. Choosing it sets selectedId, then this guard sees the actor is absent and calls setSelectedId(null) on the next frame. The picker snaps back to "Office" and the details card with the "crashed" text never renders.
Either exclude carried-out lanes from the picker, or only auto-clear focus when the lane itself is gone from lanes rather than when the actor is absent from the sim.
Related nit: sim.departed only shrinks for ids still present in lanes, so it grows by one string per agent that completes or fails and then leaves the roster (50 completed agents -> size 50 with zero actors). Bounded by the thread's subagent count, so small, but it is unbounded for the life of the mount.
There was a problem hiding this comment.
Fixed in 6bd6393. Focus auto-clears only when the lane leaves the roster, so a carried-out agent stays selectable with its crashed details. sim.departed is pruned to the roster each tick.
| }); | ||
| } | ||
|
|
||
| let assetPromise: Promise<OfficeSheetAssets> | undefined; |
There was a problem hiding this comment.
Should-fix: the composed character atlas cache is module-scoped and never released. assetPromise holds characters: Map with up to CHARACTER_CACHE_LIMIT (128) canvases. Each atlas is a full sheet: walk 512x256 (512 KB RGBA), idle and sitting 192x256 (192 KB each). A 22-agent thread composes roughly 68 atlases, about 20 MB, and the FIFO cap allows about 40 MB. All of it stays resident for the life of the web app after the Visualization tab closes, and grows as new id/colour combinations appear across threads.
Clear assets.characters in the panel's effect cleanup or key the cache per mount. The FIFO eviction itself is fine for the 22 cap. (Also, line 115 uses a ! on keys().next().value that a guarded for..of with break would avoid.)
There was a problem hiding this comment.
Fixed in 6bd6393. releaseCharacterAtlases() clears the composed characters in the panel's effect cleanup; the FIFO eviction uses a guarded for..of.
| const floor = collectTiles(",", ".", "o", "d", "m"); | ||
| sim.rescues = []; | ||
| // Table indices belong to the old layout. | ||
| sim.researchClaims.clear(); |
There was a problem hiding this comment.
Nit: reflow reshuffles research tables. reflow clears researchClaims, so a panel resize can swap two seated researchers' tables because claims are re-taken in slot order. That contradicts the "without reshuffling" guarantee in the test name. When the table count is unchanged, remap the claim indices instead of clearing.
There was a problem hiding this comment.
Fixed in 6bd6393. reflow keeps both claim maps when the desk or table count is unchanged and clears only the one whose count changed.
| } | ||
| } | ||
| } | ||
| // Prime the furniture list once; its ground anchors determine draw order with agents. |
There was a problem hiding this comment.
Nit: hidden side effect and a widened type. drawSheetRoom is named as a draw routine but its main effect is to repopulate the closure-level furniture array that drawWorld later depth-sorts with actors. If the world is drawn before this runs, no furniture exists, and nothing in the name or return value says so. Return the list or split priming from painting.
At lines 452-465 the chairs array widens dir to string and tile to number[], which forces the tile as readonly [number, number] cast. Typing the array as Array<{ tile: readonly [number, number]; dir: "up" | "down" }> removes the cast.
There was a problem hiding this comment.
Fixed in c5d7927 and 6bd6393. drawSheetRoom is now paintRoom, which paints everything static and returns the furniture list; the panel keeps { canvas, furniture } as the room cache and passes it to drawWorld, so there is no closure-level side effect. The chairs array is typed and the cast is gone.
| </div> | ||
| )} | ||
| <div className="absolute bottom-2 right-3 flex gap-2 text-[10px] text-zinc-400"> | ||
| <a |
There was a problem hiding this comment.
Nit: the credits link is unreachable in the packaged desktop app. Both footer links use target="_blank". In the desktop build the renderer runs on the custom J5 scheme, and setWindowOpenHandler in apps/desktop/src/window/DesktopWindow.ts only forwards http/https through parseSafeExternalUrl and denies everything else. The OGA-BY link is https and opens externally, but officeCreditsUrl is a same-origin ?url asset on the custom scheme, so the click is denied and attribution is unreachable from the desktop UI even though the file ships in the bundle. Verified by reading the handler, not in a running desktop build. Browsers may also download a text/markdown asset rather than display it. An in-app credits dialog fixes both.
There was a problem hiding this comment.
Fixed in 6bd6393. Art credits opens an in-app dialog rendering CREDITS.md (imported as raw text); only the https license link opens externally.
| runtime.office.reflow(runtime.sim, lanes, previous.width, previous.height); | ||
| cacheRoom(); | ||
| } | ||
| const dpr = window.devicePixelRatio || 1; |
There was a problem hiding this comment.
Nit: DPR is read only inside resize(). That runs on ResizeObserver and lane-count changes. Dragging the window to a monitor with a different devicePixelRatio at the same CSS size leaves the backing store at the old ratio (blurry or oversized) until the next resize. A matchMedia('(resolution: ...)') listener or a DPR check in the loop fixes it.
There was a problem hiding this comment.
Fixed in 6bd6393. A window resize listener re-runs resize() when devicePixelRatio changed at the same CSS size.
| @@ -8129,6 +8134,8 @@ export default function ChatView(props: ChatViewProps) { | |||
| environmentId={activeThreadRef?.environmentId ?? null} | |||
| threadId={activeThreadRef?.threadId ?? null} | |||
| /> | |||
| ) : renderedRightPanelSurface?.kind === "visualization" ? ( | |||
| <AgentVisualizationPanel model={agentPanelModel} /> | |||
There was a problem hiding this comment.
Nit: no per-thread key. AgentsPanel and ArtifactsPage get thread/project keys; this mount does not. Switching threads while Visualization is active keeps the same simulation: old actors are deleted instantly with no walk-out, and sim.departed plus any selection carry across threads. Switching to another right-panel tab unmounts the panel, so everyone re-enters through the door on return. Harmless, but the two cases are inconsistent, and the user doc's only retention claim that holds is the resize one. Key it by thread id.
There was a problem hiding this comment.
Fixed in 6bd6393. The panel is keyed by environment and thread id, so switching threads remounts and everyone re-enters through the door, matching the tab-switch behaviour.
Jacksondr5
left a comment
There was a problem hiding this comment.
Re-review by Codex gpt-6-astra (high reasoning), delegated and relayed via Claude Code. Anchored to 11258c267. Each claimed fix was verified against the source, and the two bugs the first review reproduced in memory were re-run against the new code.
Verdict
Still blocked. Seven of nine prior findings are fixed and one is partially fixed. The repaint blocker is untouched, the fixes introduced two desk-assignment regressions, and the PR description was not updated.
Prior findings
| # | Finding | Status |
|---|---|---|
| 1 | Synthetic Lead | Fixed |
| 2 | Continuous repainting | Not fixed, see inline |
| 3 | Failed agents respawn after rescue | Fixed. 6,000 ticks after a failure, zero respawns, and a later running lane re-enters and clears its departure record |
| 4 | Cap hides live agents | Partially fixed. Live agents are now admitted first and the omitted count is shown, but slot assignment after the sort regresses desk stability, see inline |
| 5 | Substring status and role inference | Fixed. pending preserved, thinking inference removed, word-boundary role matching (npm no longer reads as PM) |
| 6 | Research tables reshuffle | Fixed. Claims held per agent id; colliding-slot repro now keeps the second researcher at table 1 |
| 7 | Incomplete attribution | Fixed. All six credit files are in CREDITS.md with titles, authors, sources, and per-item licenses; the panel links the OGA-BY 3.0 text directly |
| 8 | Endless loading on asset failure | Fixed. Error state with Retry; retry reruns cacheRoom without re-subscribing observers or listeners |
| 9 | Dead code and duplicated interpolation | Fixed |
Stale description
The description still says agents visit the whiteboard while thinking (removed), that floors, walls and furniture are drawn once offscreen (furniture is painted every frame), that the simulation is fixed 60 Hz (accumulator steps all receive the same RAF now, timers and clocks use wall-clock time), that store and tabs tests cover the new surface kind (only the store test exists; RightPanelTabs.test.tsx just adds required props), and that every agent gets a desk (false after the Lead removal, see inline). Screenshots and a short video are still missing and AGENTS.md requires both for UI and motion changes.
Docs
docs/user/agent-visualization.md: Lead, thinking, pets, and Ops references are gone. It still never explains how to open the panel, lines 8–14 are still furniture narration, and lines 18–19 describe clearing the selection rather than closing the panel.docs/internals/office-art-direction.md: only the synthetic-character wording changed. The discarded design history, alternative-asset research, and rendering and verification instructions remain (lines 3–21, 23–60), and the "own workstation" claim at 34–35 is now wrong.
Engine tests pass 12/12 at this head.
| }); | ||
| visibilityObserver.observe(host); | ||
| resize(); | ||
| const loop = (now: number) => { |
There was a problem hiding this comment.
Blocker, unchanged from the first review: continuous repainting. Nothing substantive changed here. Every eligible frame still clears the backing canvas, runs the accumulator, and calls drawWorld, which clears again, blits the background, then rebuilds and depth-sorts the furniture and actor layers and runs every paint callback (agentOfficeEngine.ts around line 1287, const layers = [...furniture]). cacheRoom only rasterizes floors and walls; furniture is still per-frame callbacks.
Measured by running the extracted loop with mocked canvas ops and one seated, motionless agent: over 300 eligible frames, 300 drawWorld calls, 600 clears, 9,300 furniture paints. A motionless waiting agent gives the same counts.
The decorations that stop the room from ever settling: wall clock and blinking digital-clock punctuation (~696–805), coffee steam every eight seconds (~866–881), idle wandering deadlines (~937–956), sleep Z animation (~532), monitor animation and paper shuffling (~578–638, ~815–864), time-of-day windows and lighting (~725–783). One correction to the first review: the occupancy flicker is bounded to 1,750 ms, so it is not itself perpetual.
Fix: drop the perpetual decorations, stop scheduling frames once bounded transitions (walk in, walk out, rescue) settle, and invalidate on lane changes, pointer interaction, resize, and asset readiness. This is the one remaining blocker.
There was a problem hiding this comment.
Fixed in c5d7927. Frames are scheduled only while an agent is walking, a rescue is under way, or a paper shuffle is running; isSettled() gates the next requestAnimationFrame, and the loop is woken by lane, pointer, size, visibility, and asset events. The perpetual decoration is removed (both clocks, steam, time-of-day windows and lighting, lamps, idle wandering, sleep, animated screens, the blinking bubble); windows are painted once as daytime and desk screens are a still picture per role. Furniture is baked into the room cache and only pieces in front of an actor are repainted, so per-frame cost follows agents on screen. Paper shuffles are bounded to 2.8 s and start on a seated researcher's progress change. Test: three agents settle, isSettled holds across 600 further ticks with no tile change, a lane change unsettles until the walk completes, and a shuffle ends on its own. PR body and docs rewritten to match.
There was a problem hiding this comment.
Follow-up in 002c6b2f39, on top of c5d7927. We put a little ambient life back, deliberately structured so the room still settles: each behaviour is a scheduled, bounded event, and between events no frames are scheduled. nextWakeAt() reports the earliest scheduled event and the panel sets a single timer for it; isSettled() still gates requestAnimationFrame. The events: one idle agent walks to another break spot every 30-60 s; the coffee machine puffs for 2 s roughly every 9 s; a seated idler dozes after 40 s (a state change drawn still); the wall clock, window sky, dusk tint and lamps follow the time of day and redraw at most once a minute. None of it runs in an empty office. Tests assert the empty-office case schedules nothing, the wander window and bounded walk, dozing, and a quiet-fraction bound (>75% of ticks settled with agents seated). If the cadence reads as too lively for the rule, the two knobs are WANDER_MIN_MS / STEAM_EVERY_MS and I'm happy to tune them.
| const resize = () => { | ||
| const { bounds, width, height } = roomSize(); | ||
| const lanes = lanesRef.current; | ||
| const layout = officeLayout(width, height, lanes.length - 1); |
There was a problem hiding this comment.
Should-fix, regression: too few desks after the Lead removal. This still passes lanes.length - 1, which used to account for the synthetic Lead. With three real agents the room gets two desks, and the modulo desk assignment in the engine seats agents 0 and 2 on the same tile. Reproduced with both actors at [8,6]; the third agent also gets no workstation highlight. Pass the full lane count, and add a test for the three-agent allocation. docs/internals/office-art-direction.md ("each working agent has its own workstation") is now contradicted by this.
There was a problem hiding this comment.
Fixed in 6bd6393. Same fix as Bryant's thread above: full lane count, per-id desk ownership, and a test through the real roster path for 3, 5, and 22 agents.
| .map((agent, index) => ({ agent, index, status: laneStatus(agent) })) | ||
| .sort((a, b) => STATUS_PRIORITY[a.status] - STATUS_PRIORITY[b.status] || a.index - b.index); | ||
| const shown = ordered.slice(0, MAX_AGENT_LANES); | ||
| const agentLanes = shown.map(({ agent, status }, index) => ({ |
There was a problem hiding this comment.
Should-fix, regression: seated agents swap desks on status changes. Sorting by status priority is the right way to decide who is admitted under the cap, but slot: index then re-derives the desk from the sorted position. With two seated running agents A and B, A moving to waiting flips their slots to B:0 / A:1 and both walk across the room to each other's desks; A returning to running reverses it. Keep the priority sort for admission only, and make desk ownership persistent per agent id (the same pattern the research-table claims now use).
There was a problem hiding this comment.
Fixed in 6bd6393. Admission and seating are separated: priority sort for the cap, model-order slots, and per-id desk claims. Regression test covers the A→waiting flip with A, B, C seated.
| * go. New claims start at the table matching the slot and take the next free | ||
| * one; with every table claimed the researcher keeps its desk. | ||
| */ | ||
| function researchTableFor(lane: Lane, lanes: ReadonlyArray<Lane>, sim: Simulation) { |
There was a problem hiding this comment.
Nit: research claims outlive the last researcher. Pruning happens only inside this function, but departing lanes return from targetFor before reaching it (line 933). After the sole researcher exits, researchClaims still holds its entry with zero actors. Harmless in practice because the next eligible researcher prunes before allocating, but prune once per actor update so the state is not stale on an empty roster.
There was a problem hiding this comment.
Fixed in 6bd6393. Claims are pruned once per tick in updateActors (pruneClaims), independent of routing, so an empty roster carries no stale claim.
| cafeSeats, | ||
| meeting, | ||
| researchTables, | ||
| boards: [[3, 2]] as const, |
There was a problem hiding this comment.
Nit: dead whiteboard destination. The thinking state and its whiteboard trip were removed, so nothing in the engine consumes layout.boards any more. It survives only as a reachability target in agentOfficeEngine.test.ts:41. Remove the destination and the assertion; keep the whiteboard artwork as scenery if you like.
There was a problem hiding this comment.
Fixed in 6bd6393. boards removed from the layout and the test; whiteboard tiles remain as wall art.
|
Second-round findings without an anchor, addressed on the latest push (head 3a3d395):
The PR description is rewritten to describe what ships (no whiteboard, furniture cached, accumulator stepping, actual test coverage, one desk per agent). |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: Jacksondr5/j5code/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: Comment |
A pixel-art office showing a thread's agents: each agent walks in, sits at its own desk while working, uses a reading table when its task involves research, takes a break seat when idle, and walks out when finished. A failed agent is carried out by medics. Hover or click an agent for its name, status, and task. Rendering paints only while something is in transition: an agent walking, a rescue, a bounded paper shuffle, or a rare ambient event (an idler's short walk, a steam puff, dozing, the wall clock's minute). Between events the room is settled and no frames are scheduled. Floors, walls, and furniture are cached offscreen; per-frame work scales with agents on screen. Lanes come from the existing AgentPanelModel. Status is the runtime lifecycle status; role picking is decorative and local. Desks and research tables are owned per agent id. Live agents are admitted first under the desk cap and the omitted count is shown. Artwork is LPC Revised under OGA-BY 3.0 with full attribution shipped and shown in-app. FORK.md case 40 records the integration seam; the user guide is linked from the docs index. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Appends the visualization kind to the right-panel store (with a singleton test), optional onAddVisualization / visualizationAvailable props and one entry per surface list in RightPanelTabs, and the render branch plus callback in ChatView, keyed per thread. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
35e6f35 to
ac341b7
Compare
|
Branch history rewritten (force-push with lease), head now |
…ation # Conflicts: # FORK.md
Following subagents in a list tells you what is running but not how the thread feels. This adds a Visualization surface to the right panel that renders the thread's agents as a small pixel-art office. Agents walk in when they start, sit at their own desk while working, take a reading table and shuffle papers when their task involves research and they report progress, sit in the break area when idle, and walk out when they finish. A failed agent is carried out by two medics.
Hover or click an agent for its name, status, and current task. The picker above the room offers keyboard selection. Clicking an agent highlights the workstation it owns.
How it works
visualizationright-panel kind, a singleton per thread, opened from the plus menu, the empty-panel launcher, or theVshortcut. The appended integration points are recorded as FORK.md case 44; everything else lives in J5-ownedapps/web/src/j5/office/.officeRostermaps the existingAgentPanelModelonto lanes. Status is the runtime lifecycle status (pending kept distinct); nothing is inferred from text. Role picking (which screen an agent gets, whether it uses a reading table) is decorative, local to the office, and whole-word. Live agents are admitted first under the 22-desk cap and the omitted count is shown. No server or contract changes.officeLayoutsizes the room from the pane and the lane count: one desk per agent (floor two, cap 22) plus one research table per three desks. Aspect ratio and columns are bounded.agentOfficeEngineowns pathfinding, seating, and drawing. Desks and research tables are owned per agent id, so nobody moves when neighbours change status or leave. Failed agents are carried out once and stay out until they are live again.CREDITS.md, opens in an in-app dialog, and the license text is linked directly. Characters are composed from body, head, clothing, and hair layers with per-agent shirt, hair, and trouser tints hashed from the agent id.Rendering model
The room only paints while something is in transition. Frames are scheduled while an agent walks, a rescue is under way, or a paper shuffle (2.8 s, triggered by a progress change) is running; once
isSettledreports true the loop stops. It is woken by lane, pointer, size, visibility, or asset events, or by a single timer set for the next scheduled ambient event fromnextWakeAt. A settled office schedules no frames between events.Surfaces
Web and desktop share the renderer. Mobile is not supported; native navigation is unchanged.
Tests
agentOfficeEngine.test.tscovers: station reachability and uniqueness at several pane sizes and agent counts; layout bounds for a degenerate host; roster status mapping, live-first admission under the cap with the omitted count, and slot stability across a status flip; distinct desks for 3, 5, and 22 agents through the real roster path and no desk changes on a status flip; research table claims including colliding slots and handoff; a failed agent carried out once with no respawn; hit testing of seated agents; resize without reviving departed agents; and settling (settled after arrival, unsettled on a lane change, bounded paper shuffles).rightPanelStore.test.tscovers the singleton surface kind.Screenshots and video are intentionally omitted for this draft; verified visually in a local dev instance.
Built with Claude Fable 5.1 in Claude Code.
🤖 Generated with Claude Code