Skip to content

chore(user-interviews): address facade isolation review nits#59241

Merged
pauldambra merged 3 commits into
masterfrom
posthog-code/user-interviews-facade-review-followups
May 20, 2026
Merged

chore(user-interviews): address facade isolation review nits#59241
pauldambra merged 3 commits into
masterfrom
posthog-code/user-interviews-facade-review-followups

Conversation

@pauldambra
Copy link
Copy Markdown
Member

Problem

Follow-up nits from the review of #59132 (the user_interviews facade + contracts isolation PR):

  • facade/contracts.py used stdlib dataclasses.dataclass — the recent architecture guidance prefers pydantic.dataclasses.dataclass so contracts get runtime validation at construction time.
  • facade/__init__.py and presentation/__init__.py were empty, which CLAUDE.md discourages.
  • The facade implemented business logic inline (regex parsing + ORM query) — facades should stay thin and call logic.py.

Changes

  • Swap dataclasses.dataclass for pydantic.dataclasses.dataclass in products/user_interviews/backend/facade/contracts.py.
  • Add products/user_interviews/backend/logic.py containing parse_interviewee_identifier and has_replied (the regex + ORM call). The facade now just delegates.
  • Populate facade/__init__.py with public re-exports (mirrors the experiments facade convention) and add a docstring to presentation/__init__.py.

Public facade surface (parse_interviewee_identifier, has_replied, IntervieweeIdentity) is unchanged, so callers in posthog/api/sharing.py and presentation/views.py keep working as-is.

How did you test this code?

I'm an agent — no manual testing performed. Existing automated coverage in products/user_interviews/backend/test_facade.py exercises both functions through the facade and should continue to pass unchanged.

Publish to changelog?

no

🤖 Agent context

Authored by PostHog Code (an agent) in response to the human author asking it to apply three specific review comments from #59132. Decisions:

  • Kept the facade as plain functions (not a class) since the existing surface was function-based and the upstream callers import the functions directly.
  • Used pydantic.dataclasses.dataclass rather than pydantic.BaseModel — products/architecture.md describes this as the preferred upgrade over stdlib dataclasses, and IntervieweeIdentity has no need for BaseModel-only features.
  • Followed the experiments product's facade/__init__.py shape (re-exports + __all__) and the error_tracking product's presentation/__init__.py shape (single docstring) since both already match this codebase's existing conventions.

Created with PostHog Code

- Swap stdlib dataclass for pydantic.dataclasses.dataclass in
  facade/contracts.py — matches the architecture guidance for runtime
  validation of the public contract.
- Move parse_interviewee_identifier + has_replied into backend/logic.py
  so the facade only delegates and stays thin and stable.
- Populate facade/__init__.py with public re-exports (parse_interviewee_identifier,
  has_replied, IntervieweeIdentity) and add a docstring to
  presentation/__init__.py so neither file is an empty __init__.

Generated-By: PostHog Code
Task-Id: 639c774a-3c16-4b58-94a4-35682c62ade2
@pauldambra pauldambra marked this pull request as ready for review May 20, 2026 16:56
@assign-reviewers-posthog assign-reviewers-posthog Bot requested review from a team, MattBro, fercgomes and rafaeelaudibert and removed request for a team May 20, 2026 16:57
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 20, 2026

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
products/user_interviews/backend/logic.py:12
**`logic.py` imports from the `facade` package — dependency direction is inverted**

`logic.py` is the inner/lower layer; `facade/` is the outer layer that wraps it. Having `logic.py` import from `facade/contracts.py` means the lower layer depends on the upper layer, which is backwards. In practice this doesn't cause a circular import today (since `contracts.py` itself imports nothing from `logic`), but it makes the layering confusing and fragile — any future import added to `contracts.py` could create a cycle.

The fix is to move `IntervieweeIdentity` (and `contracts.py`) one level up to `products/user_interviews/backend/contracts.py` so that both `logic.py` and `facade/api.py` import it from a neutral sibling module. `facade/__init__.py` can continue to re-export it for the public surface.

Reviews (1): Last reviewed commit: "chore(user-interviews): address facade i..." | Re-trigger Greptile

Comment thread products/user_interviews/backend/logic.py
@pauldambra pauldambra added the stamphog Request AI review from stamphog label May 20, 2026
Reviewer pointed out the inversion: logic.py was importing from
facade/contracts.py, which makes the inner layer depend on the outer
one. No cycle today (contracts has no imports from logic) but fragile.

Move contracts.py one level up to backend/contracts.py so logic.py and
facade/api.py both import it from a neutral sibling. facade/__init__.py
keeps re-exporting IntervieweeIdentity so external callers continue to
import it via the facade package.

Generated-By: PostHog Code
Task-Id: 639c774a-3c16-4b58-94a4-35682c62ade2
Copy link
Copy Markdown

@stamphog stamphog Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bot reviewer raised a valid unresolved concern: the new logic.py imports IntervieweeIdentity from facade/contracts.py, making the inner layer depend on the outer layer — the opposite of the intended direction. Per review policy, bot comments with valid concerns that go unaddressed require escalation.

@stamphog stamphog Bot removed the stamphog Request AI review from stamphog label May 20, 2026
@pauldambra pauldambra added the stamphog Request AI review from stamphog label May 20, 2026
github-actions[bot]
github-actions Bot previously approved these changes May 20, 2026
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bot's concern about inverted dependency direction was raised on an older commit and has been fully addressed: contracts.py was moved out of facade/ to backend/, so logic.py now imports from the sibling backend/contracts.py rather than from the outer facade/ layer. Dependency direction is now correct, and no showstoppers remain.

…tion

CI's "Lint product structure" check (tools/hogli-commands/.../product/checks.py)
requires every product listed in tach.toml's canonical facade alternation to
have backend/facade/contracts.py — moving contracts.py to backend/contracts.py
broke that invariant.

This matches products/architecture.md, which puts contracts.py under facade/,
and the established pattern in mcp_analytics, experiments, and access_control
where logic.py also imports from facade/contracts.py.

The greptile bot's layering concern is noted but doesn't survive the codebase
convention: facade/contracts.py is a public-surface marker (no Django imports,
tracked as Turbo contract inputs), not the outer wrapping layer it might look
like from the path alone.

Generated-By: PostHog Code
Task-Id: 639c774a-3c16-4b58-94a4-35682c62ade2
@github-actions github-actions Bot dismissed their stale review May 20, 2026 17:33

New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean facade isolation refactor — moves logic out of the facade into a dedicated layer, makes the facade thin, and wires up proper exports. The noted inverted dependency (logic.py importing IntervieweeIdentity from facade/contracts.py) is an architectural nit on a no-dependency frozen dataclass, not a functional or safety issue, and the inline concern is marked resolved.

@pauldambra pauldambra enabled auto-merge (squash) May 20, 2026 17:43
@pauldambra pauldambra merged commit 54f13e2 into master May 20, 2026
261 of 263 checks passed
@pauldambra pauldambra deleted the posthog-code/user-interviews-facade-review-followups branch May 20, 2026 18:37
@deployment-status-posthog
Copy link
Copy Markdown

deployment-status-posthog Bot commented May 20, 2026

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-05-20 20:18 UTC Run
prod-us ✅ Deployed 2026-05-20 20:33 UTC Run
prod-eu ✅ Deployed 2026-05-20 20:36 UTC Run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stamphog Request AI review from stamphog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant