Persist in-progress question answers per question#3126
Conversation
When the agent asks a question, the answer box kept its state only in local component state, so switching to another session (unmounting the view) erased anything half-typed. Add a per-question draft store keyed by the question's tool-call id: the active step, selected options, and free-text input are saved on every edit and restored on remount. The draft is cleared when the question is submitted or cancelled, and the selector is now keyed per question so a new question no longer inherits the previous one's state. Generated-By: PostHog Code Task-Id: 624a18e6-9eac-4d18-8bd4-62491e117547
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
React Doctor found 1 issue in 1 file · 1 warning. 1 warning
Reviewed by React Doctor for commit |
|
Reviews (1): Last reviewed commit: "Persist in-progress question answers per..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const draftReportMountedRef = useRef(false); | ||
| useEffect(() => { | ||
| if (!draftReportMountedRef.current) { | ||
| draftReportMountedRef.current = true; | ||
| return; | ||
| } | ||
| onDraftChangeRef.current?.( | ||
| internalStep, | ||
| Array.from(checkedOptions), | ||
| customInput, | ||
| ); | ||
| }, [checkedOptions, customInput, internalStep]); |
There was a problem hiding this comment.
When internalStep changes (step navigation), this effect fires with the updated internalStep but still-current checkedOptions and customInput from the previous step, because React effects share the state snapshot of the render in which they run. The restoreStepAnswer effect that resets those values runs in the same commit phase, so the parent receives onDraftChange(newStep, oldOptions, oldInput) before the correction fires. handleDraftChange in the parent temporarily overwrites the saved answer for newStep with the previous step's selections, and the setDraft effect persists that to storage. A second render corrects it, but there is a window where the persisted draft has incorrect step data.
| useEffect(() => { | ||
| setDraft(questionId, { | ||
| activeStep, | ||
| stepAnswers: Object.fromEntries(stepAnswers), | ||
| }); | ||
| }, [questionId, activeStep, stepAnswers, setDraft]); |
There was a problem hiding this comment.
This useEffect runs on mount and writes { activeStep: 0, stepAnswers: {} } even when the user has not interacted at all. clearDraft is only called from resolveSelect and handleCancel, so if the agent removes the question (component unmounts without either firing), the entry lingers in Electron storage permanently. Over many conversations each with multiple questions, this grows without bound. A TTL or a cleanup pass keyed to session/conversation lifecycle would prevent unbounded accumulation.
Rule Used: When using global state, consider the trade-offs i... (source)
Learned From
PostHog/posthog#32692
|
|
||
| function questionToolCall(toolCallId: string): PermissionToolCall { | ||
| return { | ||
| toolCallId, | ||
| title: "Question", | ||
| _meta: { | ||
| codeToolKind: "question", | ||
| questions: [ | ||
| { | ||
| question: "What should the button say?", | ||
| header: "Button label", | ||
| options: [{ label: "Submit" }, { label: "Continue" }], | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
as unknown as PermissionToolCall sidesteps TypeScript's type-checking. If PermissionToolCall gains new required fields in the future, the test will silently compile while potentially failing at runtime in unexpected ways. Including all required properties in the literal (or in a shared makePermissionToolCall factory that the existing test suite already has, if one exists) makes the mock self-documenting and catches interface drift at compile time.
Rule Used: When creating mock objects for tests, include all ... (source)
Learned From
PostHog/posthog#32521
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
- Report the current-step draft imperatively from the edit handlers (typing/toggling) instead of a state effect. The effect keyed on internalStep could fire with the previous step's selections before restoreStepAnswer corrected them, briefly persisting wrong step data. - Only persist a draft while it holds real content, and clear it otherwise, so untouched questions (or ones the agent removes without a submit/cancel) don't leave empty entries accumulating in storage. - Tighten the test tool-call mock to `satisfies PermissionToolCall` and add a test that an untouched question leaves no draft. Generated-By: PostHog Code Task-Id: 624a18e6-9eac-4d18-8bd4-62491e117547
1 participant
Problem
If you start typing an answer to a question the agent asked and then switch to another session (e.g. to copy something), the typed text is erased. The answer box kept its state only in local React state, so unmounting the view lost it.
Changes
How did you test this?
questionDraftStore.test.ts(store isolation, clearing, persistence) andQuestionPermission.test.tsx(type → unmount → remount restores the text; no leak across question ids; draft cleared on answer).pnpm --filter @posthog/ui testfor the permissions + action-selector suites,pnpm --filter @posthog/ui typecheck, and Biome lint/format — all pass.Automatic notifications
Created with PostHog from a Slack thread