Skip to content

Large batch of tiny follow-up fixes #2303

Closed

Description

@kantord

Small, non-blocking follow-ups collected from reviews of the Studio alignment work (reviews of #2225 to #2256). None changes a design and none needs discussion, so any of them can ride along in a nearby PR. This issue also absorbs the open follow-up bundles #2300 and #2142 (see the end).

Draft PR #2309 applies the ticked items in sections A, D and E. They stay ticked here once it merges; the unticked ones are not in it.

Draft stack #2325 (PRs #2311 to #2324) applies the items below that name a draft PR. Items are ticked once a PR is drafted; they stay open until it merges. Items marked closed or already done need no PR.

A. From the review of the merged alignment PRs (#2225, #2226, #2232)

  • One token formatter (feat(studio): add the @/ alias and port the prototype's foundation primitives and utilities #2225). formatTokens exists in three places with different behavior: lib/formatters.ts (number, toFixed(1), gives "1.0M"), features/chat/turn-stats.ts (string, goes through Number(BigInt(value))) and a local copy in features/chat/chat-session-controls.tsx (truncates, gives "1m" with a lowercase m). The same count renders differently in the session controls and in turn stats. Use one formatter. Also fix the comment in turn-stats.ts that says the BigInt step avoids precision loss; Number() of the BigInt loses exactly the same precision.
  • Remove prototype-history comments (feat(studio): add the @/ alias and port the prototype's foundation primitives and utilities #2225). lib/error-message.ts references "D7-43" and "Phase 7", which mean nothing in this repo.
  • Redundant px-4 (feat(studio): converge card, table, tooltip, and input with the prototype #2226). features/memory/facts-about-you.tsx passes px-4 on the sortable heads, but TableHead already defaults to px-4. Visual no-op; delete the classes.
  • Show a failed background refetch on the Learning page (feat(studio): align Settings > Learning with the prototype #2232). In features/learning/learning-settings-page.tsx the list error renders only when items.length === 0, so a failed refetch with earlier pages loaded shows nothing and the row can keep showing as Pending. Render the error whenever isError is true, without double-rendering next to the Load more error. Narrow case: it needs two consecutive failures right after a successful decision, and the 409 path self-heals.
  • Optional: "<1m ago" (feat(studio): align Settings > Learning with the prototype #2232). formatRelativeTime returns <1m and proposal-row.tsx renders {updated} ago. Consider "just now". Note: this changes the shared formatRelativeTime, which the schedule detail page will also use once the relative-time item in section C lands. Closed, no change: the prototype shows "<1m ago" everywhere and never "just now", so Studio matches the design.

B. Studio chat approvals (from the review of #1975; was #2142, parent #1848)

Worth fixing:

  • Plan-verdict logic is duplicated and has diverged. chat-workspace.tsx (planUnavailableReason, respondToPlan, separate in-flight/uncertain sets, viewedSessionId guard) vs side-thread-panel.tsx (plan folded into the ordinary path, no uncertain state for plans). After an ambiguous failure, retry/Escape can be allowed on one surface and blocked on the other. Extract one usePlanVerdictControl hook. Draft refactor(studio): move the workspace's verdict state onto VerdictLedger #2323 (migrate the workspace to the shared ledger) and refactor(studio): share one plan verdict flow via usePlanVerdictControl #2324 (one usePlanVerdictControl hook).
  • chat-workspace.tsx is ~3400 lines. Move plan-continuation follow/reattach state into plan-continuation.ts. Draft refactor(studio): move Message out of chat-workspace #2322 moves Message out, and refactor(studio): share one plan verdict flow via usePlanVerdictControl #2324 moves the plan follow and reattach state out. consumeRun and the authorization flow are not moved: too entangled for a small PR.
  • Pending-ask card ternary is copied at 3 sites (chat-transcript.tsx, chat-workspace.tsx, side-thread-panel.tsx). Extract a PendingAskCard. Draft refactor(studio): extract PendingAskCard from four copied ternaries #2321. It was copied at four sites, not three.
  • resolvePlanAsk maps generic failed_precondition to 409 "stale" (apps/server/src/mecatl/chat.ts). Keep only stale_run_control, ask_not_pending, not_awaiting_plan, or log the underlying code. Closed with no change to the mapping: the only generic failed_precondition on this path is "a live run owns its plan continuation", so the four-code set is right. Narrowing it would turn that case into an "uncertain" verdict. Logging the code needs a logger the service does not have.
  • Docs: in user-docs/building/deployment/studio.md, the new Review tool calls and plans heading sits above the unrelated "Review authorization" paragraph. Already fixed on main: the headings are in the right order.
  • Test gaps: 500ms draft-clear boundary (499/500ms); held Escape (repeat: true) cannot fire ask deny / plan iterate / panel / stop; verdict keys with a dialog or menu open (or state in docs that only Escape applies); a planted secret in ask args/diff. Draft fix(studio): make the Escape double-press window strictly under 500 ms #2318 (499/500 ms boundary, held Escape at each layer, same-tick double-click on ordinary verdicts, side thread). The rest already existed. The planted-secret test is dropped: scrubbing happens in the engine and the UI renders args as given.

Nitpicks:

  • Add unmatchedApprovals() to chat-state.ts (the message.tools?.some(approvalMatchesToolCall) check is repeated 7 times). Already done on main (chat-state.ts).
  • Use one key helper: settledAskKeys uses runId\0askId, approvalKey uses a JSON tuple with sessionId. Draft refactor(studio): move the workspace's verdict state onto VerdictLedger #2323 (settledAskKey). settledAskKeys itself stays: stream retract events reset the ledger, so a replayed permission.ask would otherwise bring back a ghost card.
  • plan-continuation.ts: replace the hand-rolled AbortController/deadline/Promise.race with AbortSignal.any([signal, AbortSignal.timeout(ms)]). Already done on main (plan-continuation.ts uses AbortSignal.any).
  • chat-escape.ts: drop the redundant focusin branch in changed(); hoist disarm() and reuse it in the second useEffect. Draft fix(studio): make the Escape double-press window strictly under 500 ms #2318 drops the changed alias and fixes the 500 ms boundary. The focusin listener stays on purpose: a focus move should cancel an armed press.
  • stalePlanAsk reuses code stale_run_control, so clients cannot tell a stale plan ask from a stale permission ask. Draft fix(studio): give a stale plan ask its own error code #2311 (stale_plan_ask).
  • runtime.ts copyCapabilities hardcodes storageMigration: false and derives bash from shell; add a comment or fix the schema. Check detail()'s new client.server.compatibility() call does not add a round trip per request. Draft fix(studio): stop a compatibility round trip per request #2312: comments on both fields, and detail() no longer makes a compatibility RPC per request. The fields stay in the schema.
  • ci.yml: STUDIO_REQUIRE_MECATED=1 on the Playwright step is read only by mecated-mock.test.ts, not by chat-approval.spec.ts. Drop it or make the spec honour it. Already gone from ci.yml.
  • Docs: the Escape paragraph duplicates the Keyboard shortcuts page; state once and link. Draft docs(studio): shrink the Escape section to what the app does not show #2314.
  • Tests: malformed/missing args at card level, unmatched callId permission ask, double-click sends one POST, confirm useRealTimers in afterEach in chat-escape.test.tsx. Draft fix(studio): make the Escape double-press window strictly under 500 ms #2318 covers the boundary, held Escape and the double-click tests. Most of the rest already existed on main. The planted-secret test is dropped.

C. Studio schedules: detail refresh, focus and shortcuts (follow-up to #2194, from its second review; was #2300, parent #2210)

Correctness and accessibility details on the schedule detail page and the inventory that #2194 left for later. None blocks use today.

Behavior:

Focus and keyboard:

Code:

Layout, from triaging #1852's leftovers:

Not included: the detail page does not poll while a schedule is merely scheduled, so "in 30m" and a newly fired run can go stale until the window regains focus. Judged low value, since the page is mostly static configuration and the inventory polls every 5s.

D. From the post-merge review of #2233, #2251, #2252 and #2253

Related: a queued message whose send fails is not restored (#2307, filed under #2302).

E. From the review of the last three alignment PRs (#2254, #2255, #2256)

Line links point at each PR's head commit, because these PRs weren't merged when reviewed.

F. From the review of the skill Files section (#2155)

  • Declare the transport-level statuses on the knowledge routes' OpenAPI responses. None of the 16 knowledge routes declares 400 or 429 (routes/knowledge.ts), although app.onError can return both (a 429 from a rate-limited daemon was reproduced on the skill files route). Do it for all of them at once, so the routes stay consistent. Draft fix(studio): declare 400 and 429 on the knowledge routes #2313. It is 15 routes, not 16.

Merged issues

#2300 (section C) and #2142 (section B) were folded into this issue and closed as duplicates. Their text was copied here unchanged apart from checkboxes and grouping.

Part of #2210.

Fully or partially written by an AI agent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions