Conversation
|
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 |
b36b4c6 to
47b2328
Compare
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe pull request adds reusable facet search behavior and a feature-flagged workflows list. It adds workflow and email-template summary APIs with filtering, pagination, sender resolution, metrics, and generated contracts. The new frontend combines workflow and template rows, supports facet and text search, URL synchronization, optional columns, row actions, and summary rendering. Tests and Storybook stories cover API responses, accessibility, keyboard behavior, filtering, pagination, loading, errors, and feature-flag states. Priority: ➖ Normal Merge Risk: 🔵 Low · up to With the new list enabled, quickly editing search text back to an earlier value can hide workflows that only the server search found. The list stays unchanged when the flag is off. This can merge with a small follow-up. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The reviewed list and summary endpoints did not show a new route to hidden workflows or credential-bearing action data. Some server-side checks for actions launched from the new list remain unverified, so the assessment is not a clean bill of health. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/workflows/frontend/Workflows/WorkflowsListV2/workflowsListV2Logic.ts-395-399 (1)
395-399: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClearing or shortening the text leaves the old server search result in place.
The listener only calls
searchWorkflowswhen the text has at least 3 characters. It never clearsserverSearch.matchesTextcompares the server result to the current text, so a stale result cannot match the wrong text. A stale result can still block a new request, though. For example, the user typesrenewsand thenrenew, which starts a new search. The user then typesrenewsagain. Therenewresult is still in flight, sovalues.serverSearch?.textstill equalsrenews. The listener skips the request. When therenewresponse arrives, it replacesserverSearch, and rows found only by the server forrenewsdisappear.Fix: in the guard, also compare against the text that was last requested, or always dispatch
searchWorkflowswhen the text differs from the pending request.Proposed fix
- listeners(({ actions, values }) => ({ + listeners(({ actions, values, cache }) => ({ setValue: ({ value }) => { const text = value.text.trim() - if (text.length >= MIN_SERVER_SEARCH_LENGTH && values.serverSearch?.text !== text) { + if (text.length >= MIN_SERVER_SEARCH_LENGTH && cache.lastSearchText !== text) { + cache.lastSearchText = text actions.searchWorkflows(text) } },
🧹 Nitpick comments (2)
products/workflows/backend/api/hog_flow_list.py (1)
585-589: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGenerate the
typeparameter fromWORKFLOW_TYPESlike its siblings.
typeuses a hand-written description, whileexclude_typeuses_comma_list_parameter(..., WORKFLOW_TYPES, ...). IfHogFlowTypegains a value, thetypedocs go out of date andexclude_typedoes not. Buildtypewith_comma_list_parameterand keep the extra semantics text in the description.products/workflows/backend/api/test/test_hog_flow_summaries.py (1)
375-375: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winExercise query counts as an ordinary member.
self.useris the organization owner, so this test does not exercise object-level access checks for an ordinary member. The equal query counts can therefore miss a per-row access-control query. Log in a non-admin member and grant that member access to the created workflows before measuring queries.products/workflows/backend/api/test/test_hog_flow_access_control.pyLine 253 identifies the owner bypass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: efe5cbd9-0104-413e-9b25-84ea9a15c7e6
⛔ Files ignored due to path filters (7)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlproducts/messaging/frontend/generated/api.schemas.tsis excluded by!**/generated/**products/messaging/frontend/generated/api.tsis excluded by!**/generated/**products/workflows/frontend/generated/api.schemas.tsis excluded by!**/generated/**products/workflows/frontend/generated/api.tsis excluded by!**/generated/**services/mcp/src/generated/workflows/api.tsis excluded by!**/generated/**services/mcp/src/tools/generated/workflows.tsis excluded by!**/generated/**
📒 Files selected for processing (49)
frontend/src/lib/components/FacetSearchBar/FacetSearchBar.stories.tsxfrontend/src/lib/components/FacetSearchBar/FacetSearchBar.test.tsxfrontend/src/lib/components/FacetSearchBar/FacetSearchBar.tsxfrontend/src/lib/components/FacetSearchBar/facetQuery.test.tsfrontend/src/lib/components/FacetSearchBar/facetQuery.tsfrontend/src/lib/components/FacetSearchBar/facetSearchBarLogic.test.tsfrontend/src/lib/components/FacetSearchBar/facetSearchBarLogic.tsfrontend/src/lib/components/owners.yamlfrontend/src/lib/constants.tsxfrontend/src/lib/lemon-ui/LemonButton/LemonButton.tsxfrontend/src/lib/lemon-ui/LemonInput/LemonInput.tsxfrontend/src/lib/lemon-ui/LemonSnack/LemonSnack.tsxposthog/api/app_metrics2.pyposthog/cdp/test/test_validation.pyposthog/cdp/validation.pyposthog/settings/web.pyproducts/messaging/backend/api/message_templates.pyproducts/messaging/backend/api/test/test_message_templates.pyproducts/messaging/backend/email_senders.pyproducts/messaging/mcp/tools.yamlproducts/workflows/CONTRIBUTING.mdproducts/workflows/backend/api/hog_flow.pyproducts/workflows/backend/api/hog_flow_list.pyproducts/workflows/backend/api/test/test_hog_flow.pyproducts/workflows/backend/api/test/test_hog_flow_access_control.pyproducts/workflows/backend/api/test/test_hog_flow_summaries.pyproducts/workflows/frontend/Workflows/WorkflowDispatchIcons.tsxproducts/workflows/frontend/Workflows/WorkflowStatusTag.tsxproducts/workflows/frontend/Workflows/WorkflowsListV2/WorkflowSendsCell.test.tsxproducts/workflows/frontend/Workflows/WorkflowsListV2/WorkflowSendsCell.tsxproducts/workflows/frontend/Workflows/WorkflowsListV2/WorkflowsListV2.stories.tsxproducts/workflows/frontend/Workflows/WorkflowsListV2/WorkflowsListV2.tsxproducts/workflows/frontend/Workflows/WorkflowsListV2/WorkflowsListV2ColumnsMenu.tsxproducts/workflows/frontend/Workflows/WorkflowsListV2/workflowListFacets.tsproducts/workflows/frontend/Workflows/WorkflowsListV2/workflowListRows.test.tsproducts/workflows/frontend/Workflows/WorkflowsListV2/workflowListRows.tsproducts/workflows/frontend/Workflows/WorkflowsListV2/workflowsListV2Fixtures.tsproducts/workflows/frontend/Workflows/WorkflowsListV2/workflowsListV2Logic.test.tsproducts/workflows/frontend/Workflows/WorkflowsListV2/workflowsListV2Logic.tsproducts/workflows/frontend/Workflows/WorkflowsTable.tsxproducts/workflows/frontend/Workflows/hogflows/steps/HogFlowSteps.tsxproducts/workflows/frontend/WorkflowsScene.test.tsxproducts/workflows/frontend/WorkflowsScene.tsxproducts/workflows/mcp/tools.yamlproducts/workflows/package.jsonservices/mcp/schema/generated-tool-definitions.jsonservices/mcp/schema/tool-definitions-all.jsonservices/mcp/src/api/generated.tsservices/mcp/tests/unit/__snapshots__/tool-schemas/workflows-list.json
Included review availability: Your plan provides up to 12 included reviews per hour; 7 remain after this review.
47b2328 to
4ce5faa
Compare
4ce5faa to
7cbee12
Compare
7cbee12 to
ff0173f
Compare
`GET hog_flows/summaries/` returns the MCP summary fields plus the workflow type, without the step graph, so the workflows list can load every row. It takes the list's filters and search, sorts on created_at so a save during a paged load does not move rows, applies the access-level filter itself, and is gzipped. Both list actions now select the creator in the same query. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ff0173f to
fb99165
Compare
The list v2 applies its facets in the browser, so the name-first search tier hid step content matches that the facets would have kept. The summaries action now matches name, description and step content in one pass. The existing list keeps its tiered search. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Behind the `workflows-list-v2` flag, the Workflows tab shows a compact list with a pill search bar over it. - Loads every row from `hog_flows/summaries/` (500 per page) through the generated client. - Filters on status, type, trigger, owner, health and created by, and matches free text on name and description. Text of 3+ characters also asks the server, so step names and email content still match. - Health and the "Last 7 days" column come from one lazy call to `hog_flows/metrics/global/` after the list renders. A failed call shows "Unavailable" and leaves the list working. - Optional columns are picked from the "..." menu and persist. - Filters live in `q` and `text` URL params. Old list params redirect once. - Shares the row menu, status tag and archive, restore and delete dialogs with the flag-off list, which now sends those calls to the team id through the generated client. - `FacetSearchBar` in `lib/components` holds the generic bar. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Each row shows its description under the name as one muted line, with the full text in a tooltip. Owner is a default column. - FacetSearchBar moves from lib/components into the list v2 folder of the workflows product, with its tests and stories. - Row actions run one at a time per workflow. A repeated press is ignored and the menu item shows a loading state until it finishes. - Enable, disable, archive and restore take status and updated_at from the server answer, so Updated and the sort stay current. - While the server search for the current text is pending, the table shows its loading state instead of "No workflows match". A failed server search keeps the client matches and shows a notice. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1fbb79f to
97972c5
Compare
Note
Stacked on #106561 (the slim
hog_flows/summaries/endpoint). Only the commits afteraafae709320(the #106561 head) are this PR's own. Review it here: Silthus/posthog@aafae70...workflows-list-v2/search-barThis PR targets
masterbecause GitHub stacks cannot span a fork. It is rebased ontomasteronce #106561 merges.Problem
summariesendpoint this needs. This PR adds the frontend, behind theworkflows-list-v2flag.Changes
workflows-list-v2on, the Workflows tab shows a pill search bar over one compact list of workflows, most recently updated first.sta, Tab, then picking a value adds aStatus: Activepill.-status:excludes. Free text stays text.status:activealso makes a pill, once the token is complete.Owner: @handlein the description, else the creator's first name or email name.@handle) and Updated. The "…" menu next to "New workflow" adds Type, Trigger, Created by, Last 7 days and Health, or hides Owner. The choice persists per browser.hog_flows/metrics/global/endpoint, made after the list loads.search(debounced, previous request cancelled), so step names and email content still match.hog_flows/summaries/(500 per page,type=messaging,automation,loop) through the generated client.qholds the pills andtextthe free text. Oldstatus,type,trigger_type,created_byandsearchparams move into them once.statusandupdated_atfrom the server answer, so Updated and the sort stay current.workflowRowActionsmodule on the generated client, and oneWorkflowRowMenuOverlayandWorkflowStatusTag.FacetSearchBarlives next to its only consumer, inproducts/workflows/frontend/Workflows/WorkflowsListV2/FacetSearchBar/. It is controlled and has no workflow-specific code, so it can move to a shared layer when a second product needs it.LemonInputaccepts the combobox ARIA props,LemonButtonacceptsaria-selected, andLemonSnacktakes an optionalcloseLabel. The bar needs them for its combobox role, its option state and labeled pill remove buttons.Storybook screenshots, invented data only (
Scenes-App/Workflows/List v2):The earlier screenshots in Silthus#163 (comment) show the first, larger version (email templates, the Sends column, channel and from facets). They no longer match this PR.
How did you test this code?
workflowsListV2Logic.test.ts:updated_atinto the row. Catches a stale Updated column and sort.pendinguntil it answers, thendone. A failed one reportsfailed, keeps the browser matches and shows no toast. Catches the "No workflows match" flash and a toast per keystroke.nextlink that never ends shows the load error. Catches an endless load.health:idlenarrows once they arrive. Catches the list waiting on metrics, or health never filling in.qandtextonce, and007survives the URL round trip.workflowListRows.test.ts: owner parsing (Co-owner:andPrevious owner:don't count), health buckets from metrics rows, and the trigger read from an unknowntriggervalue. Catches wrong owners, wrong health, and a crash on a workflow without a trigger.WorkflowsScene.test.tsx: keyboard filtering writesq=status:active; page 2 works and a new filter goes back to page one; Owner shows with no saved columns; a stale saved column is ignored; the flag off makes no v2 requests.workflowRowActions.test.tsandworkflowsLogic.test.ts: the shared dialogs reach the team's endpoint, run the refresh, and skip it on failure. A failed archive or delete also clears the pending state.updated_attests fail with the guard and the merge removed, checked locally.facetQuery.test.ts,facetSearchBarLogic.test.ts,FacetSearchBar.test.tsx: parsing, counts, suggestion order and the keyboard paths of the generic bar.tsgo --noEmitoverfrontend(0 errors),oxlintandoxfmton the changed files, andhogli ci:preflight --strictin the pre-push hook.--baseline-commit 2bd04659765) reports 0 findings on this layer.test-new-events-schema: not needed. The diff is frontend only and touches no event ingestion, event reads or SQL over events.👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Release status
Automatic notifications
Docs update
None. No doc under
docs/covers the workflows list UI.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code,
claude-opus-5-5[1m]products/messagingchange./writing-ui-components,/placing-product-frontend-code,/adopting-generated-api-types,/writing-tests,/writing-user-facing-copy,/writing-code-comments,/writing-pr-descriptions.FacetSearchBarinto the workflows product, added the description line and the Owner default, and fixed three findings from an adversarial review: double-submit on row actions, the "No workflows match" flash during server search, and a staleupdated_atafter status changes.LemonInput,LemonButtonandLemonSnackstay. Without them the bar needs DOM attribute writes or a hand-rolled input. The PR touchesfrontend/src/lib/constants.tsxfor the flag in any case.proof/wl-v2-search-slim-2branch, becausehogli pr:upload-imageneeds PostHog write access.products/workflows/package.jsongains@testing-library/user-eventas a dev dependency for the scene test.🤖 Generated with Claude Code