Repository navigation
fix(lemon-dialog): isolate nested form dialogs with unique kea logic keys - #62113
Merged
carlos-marchal-ph merged 5 commits intoJun 19, 2026
Merged
carlos-marchal-ph merged 5 commits into
carlos-marchal-ph merged 5 commits into
Conversation
…keys When two LemonFormDialogs are open simultaneously (e.g. the 'Save as view' dialog and its nested 'New folder' dialog), they previously shared the same lemonDialogLogic singleton. The nested dialog's setFormValues() overwrote the parent dialog's form state, making the parent's validation context point at the folder form fields instead of the view name fields. This caused the submit button to become enabled with an invalid view name after folder creation. Fix: give lemonDialogLogic a key() function keyed by dialogKey. LemonDialog.openForm generates a random dialogKey per call so every dialog instance gets its own isolated kea logic state.
👀 Auto-assigned reviewersThese soft owners were skipped because they only have minor changes here. Nothing blocks merge, so self-assign if you'd like a look:
Soft owners come from |
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
frontend/src/lib/lemon-ui/LemonDialog/LemonDialog.tsx:18-22
**Redundant `dialogKey` field definition** — `LemonFormDialogProps` already inherits `dialogKey` from `LemonDialogFormPropsType` (which it extends), so redefining it here violates the OnceAndOnlyOnce principle and risks the two JSDoc descriptions drifting out of sync. The field can simply be removed from the inline type.
```suggestion
/** Override props on the auto-generated submit button (e.g. status, children) */
primaryButtonProps?: Partial<Pick<LemonButtonProps, 'children' | 'status' | 'type' | 'icon'>>
}
```
### Issue 2 of 2
frontend/src/lib/lemon-ui/LemonDialog/lemonDialogLogic.ts:14
**`'default'` fallback is still a shared singleton** — any `<LemonFormDialog>` rendered directly as JSX without a `dialogKey` prop (e.g. the inline Storybook story) will land on the same `'default'` key. Two such dialogs mounted simultaneously would still corrupt each other's form state. The `openForm` code path is fully isolated, but the component itself remains unsafe for concurrent inline use. Generating a key inside `LemonFormDialog`'s own initialisation (e.g. via `useMemo`) rather than relying on callers to pass one would close this gap entirely.
Reviews (1): Last reviewed commit: "fix(lemon-dialog): isolate nested form d..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
18
to
22
| /** Override props on the auto-generated submit button (e.g. status, children) */ | ||
| primaryButtonProps?: Partial<Pick<LemonButtonProps, 'children' | 'status' | 'type' | 'icon'>> | ||
| /** Unique key isolating this dialog's form state. Auto-generated by LemonDialog.openForm. */ | ||
| dialogKey?: string | ||
| } |
Contributor
There was a problem hiding this comment.
dialogKey field definition — LemonFormDialogProps already inherits dialogKey from LemonDialogFormPropsType (which it extends), so redefining it here violates the OnceAndOnlyOnce principle and risks the two JSDoc descriptions drifting out of sync. The field can simply be removed from the inline type.
Suggested change
| /** Override props on the auto-generated submit button (e.g. status, children) */ | |
| primaryButtonProps?: Partial<Pick<LemonButtonProps, 'children' | 'status' | 'type' | 'icon'>> | |
| /** Unique key isolating this dialog's form state. Auto-generated by LemonDialog.openForm. */ | |
| dialogKey?: string | |
| } | |
| /** Override props on the auto-generated submit button (e.g. status, children) */ | |
| primaryButtonProps?: Partial<Pick<LemonButtonProps, 'children' | 'status' | 'type' | 'icon'>> | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/lib/lemon-ui/LemonDialog/LemonDialog.tsx
Line: 18-22
Comment:
**Redundant `dialogKey` field definition** — `LemonFormDialogProps` already inherits `dialogKey` from `LemonDialogFormPropsType` (which it extends), so redefining it here violates the OnceAndOnlyOnce principle and risks the two JSDoc descriptions drifting out of sync. The field can simply be removed from the inline type.
```suggestion
/** Override props on the auto-generated submit button (e.g. status, children) */
primaryButtonProps?: Partial<Pick<LemonButtonProps, 'children' | 'status' | 'type' | 'icon'>>
}
```
How can I resolve this? If you propose a fix, please make it concise.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!
| export const lemonDialogLogic = kea<lemonDialogLogicType>([ | ||
| path(['components', 'lemon-dialog', 'lemonDialogLogic']), | ||
| props({} as LemonDialogFormPropsType), | ||
| key(({ dialogKey }) => dialogKey ?? 'default'), |
Contributor
There was a problem hiding this comment.
'default' fallback is still a shared singleton — any <LemonFormDialog> rendered directly as JSX without a dialogKey prop (e.g. the inline Storybook story) will land on the same 'default' key. Two such dialogs mounted simultaneously would still corrupt each other's form state. The openForm code path is fully isolated, but the component itself remains unsafe for concurrent inline use. Generating a key inside LemonFormDialog's own initialisation (e.g. via useMemo) rather than relying on callers to pass one would close this gap entirely.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/lib/lemon-ui/LemonDialog/lemonDialogLogic.ts
Line: 14
Comment:
**`'default'` fallback is still a shared singleton** — any `<LemonFormDialog>` rendered directly as JSX without a `dialogKey` prop (e.g. the inline Storybook story) will land on the same `'default'` key. Two such dialogs mounted simultaneously would still corrupt each other's form state. The `openForm` code path is fully isolated, but the component itself remains unsafe for concurrent inline use. Generating a key inside `LemonFormDialog`'s own initialisation (e.g. via `useMemo`) rather than relying on callers to pass one would close this gap entirely.
How can I resolve this? If you propose a fix, please make it concise.
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the |
# Conflicts: # frontend/src/scenes/settings/Settings.tsx
Generated-By: PostHog Code Task-Id: 94ea8e27-0ee5-45e6-82ce-cfca628f0fa0
carlos-marchal-ph
approved these changes
Jun 19, 2026
fuziontech
added a commit
that referenced
this pull request
Jun 19, 2026
…fill-admin * origin/master: (203 commits) fix(experiments): add missing event properties to the backend events on the recalculation workflow. (#64923) refactor(capture): decouple v1 RequestContext from analytics Context (#64184) feat(data-warehouse): add direct_query_enabled toggle to data sources (#64685) feat(data-warehouse): persist duckling bucket on DuckgresServer (#64919) fix(dev): strip read-denied $HOME dirs from sandbox PATH (#64768) fix(mcp-analytics): keep muted text readable inside quill subtrees (#64898) feat(skills): add Scouts tab backed by a generic skill category (#64796) fix(ducklake): make get_instance_region a lazy import in common helpers (#64913) fix(replay-vision): drop vestigial scanner_type from VisionAction selection (#64737) feat(warehouse): add xmin read path for postgres source (#64721) feat(warehouse): add per-table xmin sync radio (flag-gated) (#64718) feat(warehouse): validate xmin sync type and surface availability (#64723) fix(data-warehouse): CDC improvements — fix dead orphan-slot sweeper (#64725) feat(data-warehouse): scaffold Razorpay source (#64484) fix(dags): read duckling backfill bucket from stored catalog (#64909) feat(inbox): add archived tab with restore (#64876) feat(signals): surface a banner when inbox filters are active (#64811) fix(lemon-dialog): isolate nested form dialogs with unique kea logic keys (#62113) feat(session-replay): show event triggers on mobile replay settings tab (#64859) fix(preflight): restore kafka connectivity probe (#58743) ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
lemonDialogLogicused a static kea path, making it a singleton shared across all mountedLemonFormDialoginstances. When a nested dialog is opened on top of a parent dialog (e.g. the 'Save as view' SQL editor dialog opening a nested 'New folder' dialog), the nested dialog'ssetFormValues()call overwrites the parent dialog's form state. After the nested dialog closes, the parent'sisFormValidcheck reflects the nested form's validation rules and values instead of the parent's — allowing an invalid view name (e.g. one containing spaces) to pass and the submit button to become enabled.Closes #62009
Changes
dialogKeyprop toLemonDialogFormPropsTypeandLemonFormDialogPropslemonDialogLogicfrom a singleton to a keyed logic using kea'skey()+ dynamicpath(), keyed bydialogKeyLemonDialog.openFormnow generates a randomdialogKeyper call so each dialog instance gets its own isolated kea logic state<Form>component is now passedprops={logicProps}so it binds to the correct keyed logic instanceHow did you test this code?
I am an agent. I verified the fix by code inspection: the root cause was the singleton
lemonDialogLogicbeing shared between concurrentLemonFormDialoginstances. The fix ensures each dialog instance has its own kea logic key, so form state (values, validation errors) can't bleed between dialogs. The formatter (pnpm --filter=@posthog/frontend format) ran clean on the changed files.No changelog.
🤖 Agent context
Authored by Claude Code (claude-sonnet-4-6). The fix was identified by tracing through
lemonDialogLogic's kea singleton pattern and thesetFormValues(initialValues)call that runs on everyLemonFormDialogmount — which overwrites any previously mounted dialog's form state. The solution follows the existing keyed pattern used inlemonBannerLogic.