Repository navigation
feat(playbooks): show agent-led progress and run history - #249
Conversation
github-actions
Bot
added
vouch:trusted
72a0dcd to
4ed3668
Compare
github-actions
Bot
added
size:XXL
4ed3668 to
ceb6526
Compare
github-actions
Bot
added
size:XL
bryantderosier
left a comment
bryantderosier
left a comment
There was a problem hiding this comment.
Review of the progress board and runs page. Inline comments cover the specific changes I want.
The main issue is polling cost. Every open thread polls every 2.5s whether or not it has ever run a playbook, and each poll re-parses YAML on the host. That needs to change before this ships. It matters most over remote and tunnel connections.
Also needed, but not attached to a specific line:
- The before/after UI screenshots AGENTS.md asks for on UI changes. Please add them before this merges.
- FORK.md ledger rows for contract 38 on
ChatView.tsx,CommandPalette.tsx,ChatComposer.tsx,SidebarChrome.tsx,client-runtime/package.json, anddocs/user/composer.md, plus a new row forcomposer-logic.ts. The case 38 prose covers these files; the ledger rows don't. - The
SidebarChromenested ternary gets re-indented (+7/-7) where inserting before: nullwould be a one-line append.
| }; | ||
| const sync = () => { | ||
| clearInterval(timer); | ||
| if (document.visibilityState === "visible") timer = setInterval(read, 2_500); |
There was a problem hiding this comment.
2.5s is far hotter than anything else in J5 (inbox is 7.5s, squadrons 30s). Every poll goes through listForThread/listAll → progressRows → readDefinition, which means two realpath calls, a stat, a read, and a YAML parse and decode for up to 20 definitions on the host. One thread left open over a tunnel is about 24 requests a minute, forever.
Please poll only while the fetched runs include an active run (refresh on focus or on thread turn events otherwise), or refresh when a playbook change bumps the J5 revision counter.
Separately, this hook is nearly a line-for-line copy of createVisibleRefreshHook in apps/web/src/j5/useVisibleRefresh.ts. Please extend that helper with a per-instance/enabled variant and delete this copy.
| }), | ||
| ); | ||
| const [selectedRunId, setSelectedRunId] = useState<string | null>(null); | ||
| usePlaybookRunsRefresh( |
There was a problem hiding this comment.
This mounts for every server thread and stays enabled even when the thread has zero runs (the "keeps polling an empty supported thread" test asserts it). Please gate it on the thread having an active run, per the comment on the hook.
| import { usePlaybookRunsRefresh } from "./usePlaybookRunsRefresh"; | ||
| import { PlaybookStepStrip } from "./PlaybookStepStrip"; | ||
|
|
||
| function PlaybookRunCard({ |
There was a problem hiding this comment.
This page renders up to 100 PlaybookRunCards, each with a PlaybookStepStrip of up to 100 tabbable tooltip triggers. That's around 10k focusable nodes, all re-rendered on every poll because nothing is memoized. Please memo the card, or collapse the strip to a single progress bar with one tooltip on the overview.
| const unsupported = query.data?.supported === false; | ||
| usePlaybookRunsRefresh(query.refresh, connected && !unsupported && !query.isPending); | ||
| const data = query.data?.supported ? query.data : null; | ||
| // ponytail: issue priority is page-local; global triage needs ordering before server pagination. |
There was a problem hiding this comment.
Nit: same as the ponytail: comment in #248. It records a limitation instead of describing usage. Please remove or rewrite it.
| export function sortPlaybookRuns(runs: ReadonlyArray<PlaybookProgress>) { | ||
| return runs.toSorted( | ||
| (a, b) => | ||
| Number(!!b.issue) - Number(!!a.issue) || |
There was a problem hiding this comment.
This ranks any run with an issue first, including completed and cancelled ones. Combined with the #248 behavior where terminal runs get an issue once their YAML is gone, a deleted playbook pushes all its finished runs above healthy active runs. Please rank only active runs with issues first, and update the test that currently asserts the opposite.
| @@ -5508,6 +5522,7 @@ export const ChatComposer = memo(function ChatComposer(props: ChatComposerProps) | |||
| skills={selectedProviderSkills} | |||
| containerClassName={cn(isComposerResting && "min-w-0 flex-1")} | |||
| className={cn( | |||
| activePendingProgress && "min-h-10 max-h-28", | |||
There was a problem hiding this comment.
activePendingProgress is the pending user-question state, not playbooks. This changes composer sizing for every pending-question prompt, and FORK case 38 doesn't mention it. Please drop it from this PR, or split it into its own fix with a FORK note.
| export function parseStandaloneComposerSlashCommand( | ||
| text: string, | ||
| ): Exclude<ComposerSlashCommand, "model"> | null { | ||
| export function parseStandaloneComposerSlashCommand(text: string): "plan" | "default" | null { |
There was a problem hiding this comment.
This rewrites the upstream signature (Exclude<ComposerSlashCommand, "model"> → literal "plan" | "default") rather than appending. Please use Exclude<ComposerSlashCommand, "model" | "playbook">, which is a one-token append and matches the case 38 description.
| @@ -6758,7 +6760,7 @@ export default function ChatView(props: ChatViewProps) { | |||
| }, | |||
| ] | |||
| : sendContextPreviewAnnotations; | |||
| const promptForSend = promptRef.current; | |||
| const promptForSend = expandPlaybookPrompt(promptRef.current); | |||
There was a problem hiding this comment.
promptForSend is now the expanded text, and the failed-send retry path (~7358-7373) restores it into the composer. After a failed /playbook foo send, the composer shows "Start playbook foo" instead of what I typed. Please restore the original promptRef.current value.
| to follow agent-led runs across your connected environments. The overview shows | ||
| each run's owner thread, agent activity, and current step. Select a run to open | ||
| its thread, or choose **All** to include completed and cancelled runs. This | ||
| overview contains runs from the agent-led model; earlier playbook history is not |
There was a problem hiding this comment.
There was never an earlier playbook engine or history on j5/main, so telling users "earlier playbook history is not imported" describes a migration that never applied to them. Please delete the sentence.
| @@ -260,6 +268,11 @@ export const SidebarUtilityMenu = memo(function SidebarUtilityMenu() { | |||
| label="Usage" | |||
| onClick={handleUsageClick} | |||
| /> | |||
| <SidebarUtilityItem | |||
| icon={<BookOpenIcon />} | |||
| label="Playbooks" | |||
There was a problem hiding this comment.
My standing fork rule is to mount new UI inside existing J5-owned surfaces rather than adding nav entries, routes, or search entries to upstream files. This PR adds a top-level /playbooks route, this sidebar item, and a CommandPalette action. Please move the runs overview into a section on the J5-owned Fleet page, which gets the upstream nav and route edits down to zero or one. If a standalone page is really the right call, argue for it in the PR body so we can sign off explicitly, like we did for the persona settings section.
7219397 to
bf941d4
Compare
|
Warning Review limit reached
This review includes 29 billable files and costs up to $7.25. Or wait 59 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: Jacksondr5/j5code/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (29)
Comment |
c63c37c to
a7194b6
Compare
a7194b6 to
16ca431
Compare
Carried from j5/main 2a2fb48 onto the upstream V2 candidate. Conflict: ws.ts keeps the candidate's single skills import beside the playbook imports; ChatView.tsx keeps upstream's direct-annotation reference in the prompt, expands /playbook on top of it, and restores the typed draft (not the expansion) on send failure via draftTextForRestore, dropping the element-contexts ref upstream removed; composer-logic.ts keeps upstream's trigger kinds, shortcut import and submission intents and appends playbook to the slash commands; docs/user/composer.md stays upstream (docs/user/playbooks.md already documents /playbook); FORK.md updates case 45's text and its ledger rows (45). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Joins fork head 2cf4ad7 (j5/main, including #247, #241, #244, #242, #214, #215, #216, #248, #249, #250, #251) with the reviewed candidate (j5/upstream-sync-20260924-candidate), which descends from frozen upstream 67a2be0. Upstream force-rewrote history, so per FORK.md's rewrite runbook the candidate was built from the upstream tree with pin 62aef85 as the content base, then carried each j5/main PR since 8f56083 onto it and adapted it to upstream V2. This merge's tree equals the candidate tree exactly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 participants
What changed
Playbook progress is visible in the thread board and in a Playbook runs section on Fleet, with active/all filtering, pagination, owner-thread links, and environment status.
/playbookexpands through the ordinary composer send path.Part 2 of 4 for #194. Base:
codex/agent-led-pb-01-runtime.Review fixes
Validation
79 focused tests passed across the shared refresh hook, Playbook presentation, composer logic, and composer submission. Web and client-runtime typechecks, formatting, diff checks, and the web build passed. Scoped lint completed with warnings only.
Web and desktop share the changed view. The shared ordering helper is available to mobile; this PR adds no native mobile UI. Authenticated environment transport and provider contracts are unchanged. Active-to-terminal transitions, hidden views, pending reads, and multiple refresh consumers have focused coverage.
UI evidence
Before/after screenshots and integrated browser verification remain pending explicit permission required by AGENTS.md. They are still required before merge.
Updated with GPT-6 in Codex.