[fix] Stop creating the chat bootstrap row from inside an atom read - #5920
[fix] Stop creating the chat bootstrap row from inside an atom read#5920moataz-hjaiji wants to merge 1 commit into
Conversation
`generationRowIdsAtom` answered "which rows exist" AND created the blank first user message when there were none, writing to the store from inside its own read. Jotai flags that (`Detected store mutation during atom read`), but it is worse than a warning: `cancelTestsMutationAtom` reads the row ids from inside a write, so cancelling on an empty chat could re-entrantly create the row mid-write. The read is now pure. `needsChatBootstrapRowAtom` answers the condition and `ensureChatBootstrapRowAtom` performs the write, re-checking every condition inside the write so repeated callers still yield exactly one row. Driven from a null-rendering `ChatRowBootstrap` child of `MainLayout` rather than `ExecutionItems`: comparison-mode chat never goes through that component — the layout renders `GenerationComparisonRenderer`, which reads the row ids directly — so hosting it there would silently cost comparison chat its blank first turn. `MainLayout` is the single shared entry for every playground surface, so one instance covers both views exactly once. Agents are excluded. They carry `is_chat` too, so the chat branch ran for them, but the agent surface renders `AgentGenerationPanel` and never reads `sharedMessageIdsAtomFamily` — the row was write-only cost there. Fixes Agenta-AI#5344
|
@moataz-hjaiji is attempting to deploy a commit to the agenta projects Team on Vercel. A member of the Team first needs to authorize it. |
dosubot
Bot
added
the
size:M
|
Hi @moataz-hjaiji, thanks for opening a pull request. 🙏 This PR was automatically closed because it does not yet meet our contribution requirements:
We ask for this so every change is documented and demonstrably tested before review. How to get it reopened See the Contributing guide and Creating your first PR. If you think this was closed in error, leave a comment and a maintainer will take a look. |
github-actions
Bot
added
the
incomplete-pr
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe playground now detects missing initial chat rows without mutating during reads. MainLayout invokes an idempotent bootstrap action, which creates a blank user message only for supported chat entities. ChangesChat row bootstrap
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MainLayout
participant needsChatBootstrapRowAtom
participant ensureChatBootstrapRowAtom
participant PlaygroundStore
MainLayout->>needsChatBootstrapRowAtom: read bootstrap condition
needsChatBootstrapRowAtom-->>MainLayout: report whether a row is needed
MainLayout->>ensureChatBootstrapRowAtom: request row creation
ensureChatBootstrapRowAtom->>PlaygroundStore: recheck and add blank user message
Possibly related PRs
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
Summary
Fixes #5344 —
Detected store mutation during atom readin the agent playground.Thanks @ardaerzin for walking the graph on the issue; this follows the shape you laid out there, including the host correction.
Root cause.
generationRowIdsAtomanswered "which rows exist" and created chat mode's blank first user message when there were none, writing to the store from inside its own read:It isn't only a console warning.
cancelTestsMutationAtomreads the row ids from inside a write (execution/generationSelectors.ts:207), so cancelling on an empty chat could re-entrantly create the blank row mid-write. The "once per onboarding page load, silent everywhere else" pattern is a mount-timing artifact — the trailinggeton line 971 clears jotai's dev flag when that atom recomputes and doesn't when theset's own flush already did. The impurity is present on every chat surface; it is just silent on most of them.Fix.
generationRowIdsAtom's read is now pure — chat mode returnssharedMessageIdsAtomFamily(loadableId), nothing else.needsChatBootstrapRowAtom(pure) answers the condition;ensureChatBootstrapRowAtomperforms the write and re-checks every condition inside the write, so repeated callers in one tick still yield exactly one row.<ChatRowBootstrap />child ofMainLayoutdrives it from an effect.Why
MainLayoutand notExecutionItems. Comparison-mode chat never goes throughExecutionItems— the layout rendersGenerationComparisonRenderer, andGenerationComparisonChatOutputreadsgenerationRowIdsdirectly (ExecutionItemComparisonView/GenerationComparisonChatOutput/index.tsx:46). Hosting the effect inExecutionItemswould have silently cost comparison chat its blank first turn, andExecutionItemsalso mounts once per column.MainLayoutis the single shared entry for every playground surface, so one instance covers single + comparison exactly once. It is a separate child so the row-count subscription re-renders a null component rather than the whole layout.Agents are excluded (
isChat && !isAgent, root entity viaisAgentModeAtomFamily). Agents carryis_chat—createEphemeralAppFromTemplateseedsis_chat: type === "chat" || type === "agent"— so the chat branch ran for them, but the agent surface rendersAgentGenerationPaneland never readssharedMessageIdsAtomFamily. The row was write-only cost there. Nothing outsideexecution/selectors.tsreads that family, so this removes work rather than changing behaviour anyone observes.No new dependency —
jotai-effectis not in the tree and this does not add it.Testing
Verified locally
pnpm vitest runinpackages/agenta-playground: 17 test files, 221 tests passed.tsc --noEmitclean for both the package andweb/oss.eslintandprettierclean on the changedsrcfiles.Not verified in a browser by me — worth confirming on a fresh onboarding load that the warning is gone and the blank composer row still appears in single and comparison chat.
Added or updated tests
packages/agenta-playground/tests/unit/chatBootstrapRow.test.ts— 6 tests, the directory had no coverage for the bootstrap:generationRowIdsAtomnever creates a row (the regression itself)ensureChatBootstrapRowAtomcreates exactly one row for an empty chat playgroundOne detail worth flagging for review: the tests set
playgroundStoreAtomto the test store inbeforeEach. My first version did not, and the purity test passed with the bug re-introduced —playgroundStoreAtomdefaults to jotai's global store, so the impure write landed there while the assertions read an isolatedcreateStore(). With the wiring in place, re-introducing the old read fails it correctly:The comment in
beforeEachrecords why that line is load-bearing.QA follow-up
isChatMode === undefined) settles into a blank row once the flags arrive.MainLayout(WorkflowRevisionDrawerWrapper,ConfigureEvaluator,CreateEvaluatorDrawer) still get the row.cancelTestsMutationAtom; worth a manual pass.Demo
N/A — no visible UI change. The blank first row behaves exactly as before; the fix is where it is created from.
Checklist