Repository navigation
fix(sql-editor): prevent Y-axis settings from being shared between series - #55753
Conversation
…ries When multiple Y-axes are configured in SQL insights, editing one series' visualization settings (label, color, etc.) could intermittently copy those settings to another series. This happened because settings objects were passed by reference through the query round-trip cycle (reducer -> axesChanged -> setQuery -> _setQuery -> reducer), allowing multiple Y-axis entries to share the same settings object reference. Deep clone settings at all boundary points where they enter or leave the reducer state to ensure each Y-axis always has an independent copy. Closes PostHog#30932
Prompt To Fix All With AIThis is a comment left during a code review.
Path: frontend/src/queries/nodes/DataVisualization/dataVisualizationLogic.ts
Line: 551-563
Comment:
**Repeated ternary pattern (OnceAndOnlyOnce)**
The expression `settings ? cloneSettings(settings) : DefaultAxisSettings()` is duplicated three times across the initial state and the two `_setQuery` handlers. Folding both helpers into one would remove the repetition:
```typescript
const cloneOrDefaultSettings = (settings?: AxisSeriesSettings): AxisSeriesSettings =>
settings ? cloneSettings(settings) : DefaultAxisSettings()
```
Then each call site becomes the single, obvious `cloneOrDefaultSettings(axis.settings)` / `cloneOrDefaultSettings(column.settings)`.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(sql-editor): prevent Y-axis settings..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
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 |
|
Still relevant - this fixes a real bug where Y-axis settings are shared by reference between series in SQL insights. Happy to rebase if needed. |
… duplication Removes repeated `settings ? cloneSettings(settings) : DefaultAxisSettings()` ternary across Y-axis and tabular column handlers by introducing a single helper.
|
Reviews (2): Last reviewed commit: "refactor(sql-editor): extract cloneOrDef..." | Re-trigger Greptile |
|
Rebased on current master. This fix is still needed — the bug (Y-axis settings being shared by reference between series) is reproducible in the current codebase. |
|
Still valid and relevant — happy to rebase on latest master if needed. |
|
Still relevant — bumping to prevent auto-close. |
|
Gentle ping on this one — it's been open ~47 days. Happy to address any feedback or rebase if needed! |
|
|
||
| /** Deep clone settings to prevent shared references between Y-axis entries */ | ||
| const cloneSettings = (settings: AxisSeriesSettings): AxisSeriesSettings => | ||
| mergeObject({}, settings) as AxisSeriesSettings |
There was a problem hiding this comment.
Seems we are already using lodash.mergeObject here, but structuredClone would probably be the more idiomatic way in this codebase.
Problem
Closes #30932
When multiple Y-axes are configured in SQL insights, editing one series' visualization settings (display name, color, etc.) intermittently copies those settings to the previous series.
Changes
The root cause is shared object references between Y-axis settings entries. Settings objects flow through a round-trip cycle:
selectedYAxisreducer ->axesChangedlistener ->setQuery-> parent component ->_setQuery->selectedYAxisreducer. Throughout this cycle, settings were passed by reference rather than being cloned, so multiple Y-axis entries could end up sharing the same settings object.Added a
cloneSettingshelper (usinglodash.mergedeep copy) and applied it at all boundary points:selectedYAxis_setQueryhandlers for bothselectedYAxisandtabularColumnSettingsaxesChangedlistener when writing settings to the queryThis ensures each Y-axis entry always has an independent settings copy.
How did you test this code?
Agent-authored PR. The code formatting has been verified via
pnpm --filter=@posthog/frontend format. CI will run the existing test suites.Publish to changelog?
no
Docs update
not needed
LLM context
Agent-authored PR. The fix builds on top of the mutation fix in #53055, which addressed the
lodash.mergetarget mutation but did not prevent shared references between series entries across the query round-trip.