refactor(batch-exports): decouple kea logics by extracting batchExportConfigLogic#52003
Open
refactor(batch-exports): decouple kea logics by extracting batchExportConfigLogic#52003
Conversation
Contributor
Prompt To Fix All With AIThis is a comment left during a code review.
Path: frontend/src/scenes/hog-functions/backfills/hogFunctionBackfillsLogic.tsx
Line: 63-69
Comment:
**Race condition fix may itself miss the event**
The `loadHogFunctionSuccess` listener only catches the action if it fires *after* `hogFunctionBackfillsLogic` is mounted. However, `LemonTabs` renders only the active tab's content (see `LemonTabs.tsx` line 152-154: `{activeTab.content}`), meaning `HogFunctionBackfills` — and therefore this logic — is not mounted until the user clicks the Backfills tab.
In the common user journey:
1. User navigates to the hog function page → `hogFunctionConfigurationLogic` loads → `loadHogFunctionSuccess` fires.
2. User clicks the Backfills tab → `hogFunctionBackfillsLogic` mounts.
3. At this point `loadHogFunctionSuccess` has already fired and the listener never triggers.
For a hog function without `batch_export_id`, the skeleton is displayed indefinitely because `enableHogFunctionBackfills` is never called.
A robust solution handles both cases: config already loaded at mount time, and config loading after mount:
```ts
afterMount(({ actions, values }) => {
// If hogFunctionConfigurationLogic has already loaded (configuration.id is set)
// and batch_export_id is genuinely absent, enable immediately.
if (values.configuration?.id && !values.configuration.batch_export_id) {
actions.enableHogFunctionBackfills()
}
}),
listeners(({ actions, values }) => ({
loadHogFunctionSuccess: () => {
// Handle the case where config loads after this logic mounts.
if (!values.configuration.batch_export_id) {
actions.enableHogFunctionBackfills()
}
},
})),
```
Note: the E2E test (`hog-function-backfills.spec.ts`) and the Storybook mock both set `batch_export_id` on the hog function, so they exercise only the already-enabled path and would not catch this regression.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: frontend/src/scenes/data-pipelines/batch-exports/batchExportBackfillsLogic.test.ts
Line: 104-106
Comment:
**Superfluous dependency in test setup**
`batchExportBackfillsLogic` now connects to `batchExportConfigLogic` (the lightweight loader), not `batchExportConfigFormLogic`. Pre-mounting the heavyweight form logic here works only because `batchExportConfigFormLogic` in turn auto-mounts `batchExportConfigLogic` via its own `connect()`. The form logic itself is not needed by the subject under test.
This is misleading — it implies the backfills logic still depends on the form logic — and it mounts a large chunk of logic (form state, validation, `beforeUnload`, etc.) that this test doesn't exercise. Replacing with the lightweight logic better reflects the actual production dependency:
```suggestion
const configLogic = batchExportConfigLogic({ id: MOCK_BATCH_EXPORT_ID })
configLogic.mount()
await expectLogic(configLogic).toFinishAllListeners()
```
You'll need to add `import { batchExportConfigLogic } from './batchExportConfigLogic'` at the top of the file.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: frontend/src/scenes/hog-functions/backfills/hogFunctionBackfillsLogic.test.ts
Line: 26-63
Comment:
**Test file doesn't test `hogFunctionBackfillsLogic`**
The `describe` block is named `'hogFunctionBackfillsLogic'` but the test mounts and asserts on `batchExportBackfillsLogic`, never touching `hogFunctionBackfillsLogic` itself.
The critical behavioral change in this PR — the `loadHogFunctionSuccess` listener calling `enableHogFunctionBackfills` — has no test coverage. Consider adding tests that:
- Verify `enableHogFunctionBackfills` is called when `loadHogFunctionSuccess` fires and `batch_export_id` is absent.
- Verify `enableHogFunctionBackfills` is NOT called when `loadHogFunctionSuccess` fires and `batch_export_id` is already set.
- Verify the logic handles the "already loaded at mount" scenario (the P1 concern above).
Having test coverage here would also catch the missed-event edge case described in the comment on `hogFunctionBackfillsLogic.tsx`.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "test: fix mock" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
frontend/src/scenes/hog-functions/backfills/hogFunctionBackfillsLogic.tsx
Show resolved
Hide resolved
frontend/src/scenes/data-pipelines/batch-exports/batchExportBackfillsLogic.test.ts
Outdated
Show resolved
Hide resolved
frontend/src/scenes/hog-functions/backfills/hogFunctionBackfillsLogic.test.ts
Show resolved
Hide resolved
rossgray
force-pushed
the
ross/batch-exports-kea-refactor
branch
from
d9513db to
25e4537
Compare
Contributor
|
Size Change: 0 B Total Size: 110 MB ℹ️ View Unchanged
|
rossgray
added
the
update-snapshots
Split the heavyweight batchExportConfigFormLogic into two: a lightweight batchExportConfigLogic that only loads/caches the config, and the existing form logic for editing. Child logics (runs, backfills, modal) now connect to the config logic instead, allowing them to mount independently — fixing the Kea mounting error when rendering backfills from the hog function scene.
rossgray
force-pushed
the
ross/batch-exports-kea-refactor
branch
from
25e4537 to
5bcccd7
Compare
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
1 participant
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
The batch export child logics (
batchExportRunsLogic,batchExportBackfillsLogic,batchExportBackfillModalLogic) were tightly coupled tobatchExportConfigurationLogicviaconnect()— a heavyweight logic that owns form state, validation,beforeUnload, and test step execution. This meant child logics couldn't be mounted independently.When
HogFunctionBackfillstried to render the backfills tab for hog function destinations, it needed to mountbatchExportBackfillsLogic, which auto-mounted the full form logic — causing a Kea store path error because no parentBindLogicprovided it.Additionally,
hogFunctionBackfillsLogichad a race condition: itsafterMountcheckedconfiguration.batch_export_idbefore the config had loaded, causing an unnecessary call toenable_backfillson every tab open.Changes
Most of the changes here are either adding/updating tests or due to renaming the logic.
Therefore when reviewing these changes, it probably makes most sense to review
products/batch_exports/frontend/README.mdfirst, as it provides the architectural overview.Extract
batchExportConfigLogic(~35 lines) — a lightweight logic that only loads and cachesbatchExportConfigfrom the API. Child logics now connect here instead of the form logic, so they can be mounted independently (e.g. from the hog function backfills tab).Rename
batchExportConfigurationLogic→batchExportConfigFormLogic— clarifies that this logic owns the editing experience (form state, validation, dirty-checking, test steps, save/delete). It now connects tobatchExportConfigLogicfor its config data.Fix race condition in
hogFunctionBackfillsLogic— replacedafterMountwith aloadHogFunctionSuccesslistener soenableHogFunctionBackfillsonly fires after the config has loaded andbatch_export_idis confirmed missing.Add architecture docs to
products/batch_exports/frontend/README.mdwith a logic dependency tree, component table, and external consumer notes.How did you test this code?
pnpm --filter=@posthog/frontend jest batchExport hogFunctionBackfills --no-coverage)Publish to changelog?
No
Docs update
No docs update needed.
🤖 LLM context
This PR was co-authored with Claude Code. The refactor was planned using a dependency graph analysis of the batch export Kea logics, with alternatives (passing config as props, wrapping with BindLogic for the full form logic) considered and rejected. See
products/batch_exports/frontend/README.mdfor the architecture overview.