Repository navigation
feat(agent): add /btw command for quick side questions - #76461
Conversation
|
😎 Stack merged successfully - details. |
Prompt To Fix All With AI### Issue 1
products/desktop/packages/ui/src/features/sessions/sideQuestionStore.ts:68-72
**Concurrent request replaces active state**
If a user submits a second `/btw` while the first is pending, `ask` replaces the first entry before the adapter rejects the concurrent request. The card then shows the second request's “already in progress” error, while the first request's successful answer is discarded by the stale-answer guard.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix: scope side-question cards to their ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@haacked this is your PostHog/code#3733 remade in the monorepo (main there is frozen after the desktop import). Your commits are intact, only paths moved plus small conflict fixes noted in the description. Can you review and approve if it looks good? |
|
Closing for now: this has to work with cloud first. |
|
I'll make it work with cloude. |
Prompt To Fix All With AI### Issue 1
products/desktop/packages/ui/src/features/sessions/sideQuestionStore.ts:82-86
**Dismissal loses in-flight state**
When a user dismisses a pending side-question card and submits another `/btw`, `dismiss` deletes the only UI-side record of the active request without cancelling it. The new request then reaches Claude's one-at-a-time guard and fails with “A side question is already in progress,” leaving the user unable to ask again until the invisible first request finishes.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (2): Last reviewed commit: "fix: scope side-question cards to their ..." | Re-trigger Greptile |
ba47d06 to
25f588f
Compare
🦔 Hogbox preview · ✅ ready▶ Open the preview
commit |
🤖 CI report✅ Trunk lane — non-backend laneThis PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes. ✅ Bundle size — no changeUncompressed size of every built Total: 65.81 MiB · no change No file changed by more than 1000 B. Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report ✅ Eager graph — within budgetHow much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy
🟢 Largest files eagerly shipped from
|
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 285.5 KiB | ../node_modules/.pnpm/posthog-js@1.410.1/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 238.4 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 231.5 KiB | ../node_modules/.pnpm/posthog-js@1.410.1/node_modules/posthog-js/dist/module.js |
| 154.3 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 104.5 KiB | src/lib/api.ts |
| 95.2 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.20 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.20 MiB · 17 files | no change | ████░░░░░░ 38.4% of 5.72 MiB |
| Deferred (lazy) | 2.08 MiB · 33 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 724.0 KiB | dist/toolbar/toolbar-app-AGPXPBM6.css |
| 552.2 KiB | dist/toolbar/chunk-chunk-NG2USC2E.js |
| 484.6 KiB | dist/toolbar/chunk-chunk-HNXQZODW.js |
| 133.6 KiB | dist/toolbar/chunk-chunk-WCMPKPJC.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.0 KiB | dist/toolbar/toolbar-app-2IWOFSQG.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-6UNPKQ3S.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-KX2YW6HG.js |
| 12.2 KiB | dist/toolbar/chunk-chunk-PIK3PADE.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +393 B (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1396.54 MiB · 🔺 +393 B (+0.0%)
✅ Backend coverage — no measured backend lines changed
🧪 Backend test coverage
No measured backend lines changed in this PR (patch coverage n/a).
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
|
React Doctor found 1 issue in 1 file · 1 warning. 1 warning
Reviewed by React Doctor for commit |
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
b7aa04c to
e726f86
Compare
2c65ee6 to
729a7f6
Compare
The /btw fork owns a separate AbortController that the base close path never sees, so closing a session mid-answer left the fork streaming (and consuming tokens) until its 120s timeout fired. closeSession now aborts sideQuestionAbort before delegating to the base close path, matching the supersede path that already aborts the fork. Adds a regression test that starts a gated side question, closes the session, and asserts the fork's controller was aborted. Generated-By: PostHog Desktop Task-Id: 13761e40-4243-459f-86d3-cbadf3174f28
A structured task run stores a json_schema outputFormat on the live session's query options. The /btw fork spread `...rest` over those options and reset every other inherited key (tools, mcpServers, plugins, etc.) but not outputFormat, so on a structured session the side question answer was forced into the task's unrelated schema and rendered as schema-shaped JSON or dropped as no answer. Strip outputFormat in the destructure alongside sessionId and hooks so the fork always answers as plain text. Adds a regression test that gives the live session a json_schema outputFormat and asserts the fork's options carry none. Generated-By: PostHog Desktop Task-Id: 13761e40-4243-459f-86d3-cbadf3174f28
The command menu merged /btw into every session's suggestions, so on a session that cannot answer side questions (for example a codex session, or the new-task composer with no session) picking /btw dead-ended in an error toast. getCommandSuggestions now filters /btw out unless sessionSupportsSideQuestion is true, reusing the same capability check the runtime callback uses. The command's own runtime guard stays for users who type /btw by hand. Adds parameterized cases asserting /btw shows on a claude session and is hidden on a codex session and when there is no session. Generated-By: PostHog Desktop Task-Id: 13761e40-4243-459f-86d3-cbadf3174f28
256ccd4 to
ac26fe3
Compare
charlesvien
added
the
stamphog
PostHog
|
Resolved the merge conflict flagged on this PR. It is Why it surfaced here. The base branch How I updated the stack (bottom-up, so master's changes never enter this PR's diff):
The two real collisions, both additive and both resolved by keeping each side:
Verification. This PR's file set is byte-identical to before the update — the same 32 files, and Locally before publishing: All 7 review threads remain resolved. The rebase dismissed the earlier stamphog approval, so I re-applied the label to get the required approval back. 🦉 via talyn.dev |
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additive desktop-only feature (new one-shot side-question fork), no dependency/CI/migration/auth/billing changes; the concurrency and dismissal edge cases a bot reviewer previously flagged are demonstrably fixed and test-covered in the current diff, and the author is the owning team with strong, current familiarity plus a clean automated security review.
- Author wrote 100% of the modified lines and has 22 merged PRs in these paths (familiarity STRONG).
- 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 734L, 21F substantive, 1806L/32F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (1806L, 32F, single-area, feat) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ ac26fe3 · reviewed head ac26fe3 |
|
Swept this layer again. Nothing left to change on it. Review threads. All 7 are resolved; none unresolved or awaiting a reply. CI. Green on the current head Mergeability. What is still holding it. GitHub reports That needs an approval on the base layer, which is a decision for a reviewer on that PR rather than a change to this one. Once #80023 is approved and merged, this layer should go straight to mergeable with no further work. 🦉 via talyn.dev |
|
Both Greptile findings were reviewed against the current head ( Issue 1 — "concurrent request replaces active state." Issue 2 — "dismissal loses in-flight state." There is no one-at-a-time guard in the adapter to trip. A newer side question aborts the fork it supersedes ( 🦉 via talyn.dev |
|
Swept this layer again on Review threads. All 7 resolved, none awaiting a reply. CI. Green: 83 passing, 0 failing, on the current head. The one non-green run is the Wiz SAST Scanner, which reports Mergeability. What is holding it. Stack ordering, not this layer. #80023 is no longer a draft, but it has not been approved yet, so it cannot land and this layer stays That finding is about the backend command's rate limiting, which lives entirely in #80023's diff. Picking a limit (per user, per run, per organization, and at what rate) is a product call rather than a mechanical fix, so I have not made it from this layer. Once it is settled there and #80023 merges, this PR should go straight to mergeable with no further change. 🦉 via talyn.dev |
|
The stack blocker is cleared. This layer is unchanged — nothing needed fixing here — but the layer below it now has what it was missing. What changed. #80023 was sitting at State now, on On the 🦉 via talyn.dev |
|
Swept this layer again on Review threads. All 7 resolved, none awaiting a reply. The two Greptile top-level findings were answered against this head in an earlier comment; nothing new has come in since. CI. Green on the current head: 41 passing, 0 failing, nothing still running. The single non-green run is the Wiz SAST Scanner at Mergeability. What changed since the last sweep. #80023 picked up an approving review, so its This layer still reads 🦉 via talyn.dev |
|
Swept this layer again on This layer. All 7 review threads resolved. CI green on the current head: 87 passing, 0 failing, nothing still running (the single non-green run is the Wiz SAST Scanner at Cleared on the layer below. What is still holding the stack. #80023 has That is the only thing left, and it is not something I can supply. Once #80023 picks up a human approval and merges, this layer should go straight to mergeable. 🦉 via talyn.dev |
|
Re-verified this layer on Review threads. 7 total, 0 unresolved, none awaiting a reply. CI. Merge state. Why it still reads New detail on the parent. The automated approval path there is already exhausted, so re-triggering it will not help — the 🦉 via talyn.dev |
The PR was retargeted to a different base branch, so the approved diff is no longer what was reviewed. Stamphog re-reviews automatically.
|
/trunk merge |
There was a problem hiding this comment.
The new backend side_question command sits in public-API/quota territory, but it's gated by the same access and usage-limit checks as other commands, has dedicated backend tests, a human APPROVED review on the current head (@dmarticus), a clean security-bot pass, and the two Greptile concurrency concerns were verifiably fixed (tests confirm the fix). The author's STRONG familiarity with nearly all touched lines further covers the cross-team backend files.
- Author wrote 97% of the modified lines and has 38 merged PRs in these paths (familiarity STRONG).
- dmarticus reviewed the current head.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 757L, 23F substantive, 1926L/39F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (1926L, 39F, two-areas, feat) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ ac26fe3 · reviewed head ac26fe3 |
Generated-By: PostHog Desktop Task-Id: 1c4a2e91-405e-4984-a746-2035fbf9d616
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Adds a well-isolated feature (tool-less transcript fork) with thorough tests covering the isolation guarantees (empty tools/mcpServers/settingSources, deny-all canUseTool, abort semantics); the author is on the owning team with STRONG familiarity, and the two substantive Greptile findings were verified against the current diff as actually addressed (the store's ask guard and the adapter's supersede/abort logic).
- Author wrote 100% of the modified lines and has 22 merged PRs in these paths (familiarity STRONG).
- 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 734L, 21F substantive, 1806L/32F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (1806L, 32F, single-area, feat) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ ff939e5 · reviewed head ff939e5 |
HostHog preview —
|
|
/trunk merge |
|
The stack is gone — #80023 merged, GitHub retargeted this PR onto Why an update was needed. #80023 landed as a squash, so its commits are not ancestors of The merge was conflict-free, but it moved a fair amount of desktop code, so I verified locally rather than trusting that:
Current state. CI green on the head: 86 passing, 0 failing, nothing running. Every required context is satisfied, including I also fixed the description's opening line, which still said the backend half merges first. On 🦉 via talyn.dev |
|
/trunk merge |
|
This PR is already queued as a stacked merge. Cancel it first to re-submit. |
The backend half (the
side_questiontask-run command) landed in #80023, so this now targetsmasterdirectly. This layer is desktop-only, per the desktop backend coupling check.Ports PostHog/code#3733 into the monorepo (PostHog/code
mainis frozen after the desktop import; open PRs there are remade here). Commits preserve the original authorship.Problem
Users mid-conversation with Code often want a quick answer about something in the current session (what changed, what a function does) without derailing the active turn or polluting the conversation history. There was no way to do this without either interrupting the running agent or starting a whole new session.
Changes
Adds a
/btwcommand that answers a one-shot side question by forking the live session's transcript, similar to how Claude Code's own/btwworks. It works on both local and cloud sessions.The fork is a single turn with no tools:
tools: [],allowedTools: [],mcpServers: {}, and an always-denycanUseToolindependently block tool use, and it gets its own session id and abort controller so nothing on the live session is touched. The answer renders as a dismissible card above the composer and never enters the session transcript, so it works both mid-turn and while idle.For local sessions, the Claude adapter advertises the capability during
initialize(), the same way it advertisessteering. Workspace-server, the host-router, andSessionServicethread the capability through, and the UI gates the command onsessionSupportsSideQuestion.On cloud sessions, the sandbox runs the same
@posthog/agentbuild, so the fork already works there. Theside_questioncommand reaches it over the command channel that #80023 adds, and the answer returns as the command's result rather than on the event stream, which keeps it out of the transcript. Cloud runs never negotiate the ACP capability with the desktop client, so the gate falls back toadapter === "claude". A sandbox predating the command rejects it, and the error surfaces on the card.A second
/btwon the same run is refused while one is pending, and the card says so. Dismissing the card or starting a new run lets a new question through, and the adapter aborts the fork it supersedes. Answers and cards are keyed bytaskRunId, so an answer never displays under a session that reconnected or started a new run. A pending side question keeps the idle timer from reaping the session, and the answer region is aria-live so screen readers announce it.Here's a screenshot of it in action:
How did you test this code?
Ran the full test suites for the touched packages (
@posthog/agent,@posthog/core,@posthog/workspace-server,@posthog/ui) andpnpm typecheckon each; everything passes. The cloud path was exercised by running a cloud task end to end, which surfaced the unregisteredside_questioncommand schema that the last commit fixes. I haven't manually exercised/btwon a local session in the running Electron app.After the port: from
products/desktop/,pnpm install --frozen-lockfile,pnpm typecheck(24/24 clean), full@posthog/agent(1792) and@posthog/core(2961) suites,@posthog/workspace-serversrc/services/agenttests (103) and the@posthog/uisessions suite (608) all pass. The workspace-serversrc/dbtests were skipped locally (unbuilt better-sqlite3 bindings in the dev checkout, unrelated to this change); desktop CI runs the full suite.