feat(tasks): link a Channel to the desktop-fs folder that renders it#72463
feat(tasks): link a Channel to the desktop-fs folder that renders it#72463adboio wants to merge 2 commits into
Conversation
A channel exists as two unrelated rows today: the Channel model (feed, membership, Task.channel FK) and a desktop-surface FileSystem folder (the tree the app renders — canvases and task filings nest under its path). The only join between them is display-name matching, with the normalization function mirrored client- and server-side. Anything that needs to go from a task to the channel's folder (e.g. agent-side canvas placement) is stranded one hop short, or has to scrape filing-row conventions. Give Channel a folder FK (nullable, SET_NULL, db_constraint=False like the model's other FKs, one live channel per folder via a partial unique constraint). Clients that own folder creation pass folder_id on the resolve-or-create POST, or PATCH it onto an existing channel (personal included) to backfill lazily. Claim semantics: first claim wins, a claimed folder can't be stolen, re-linking is allowed once the old folder row is gone, and non-desktop / non-folder / foreign-team rows are ignored. The task_channels serializer now returns folder_id, so any consumer can resolve task -> channel -> folder purely by ids. Generated-By: PostHog Code Task-Id: 09832237-2f58-4ab7-adf6-231aeb82e1fe
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ) | ||
| if folder_id is None: | ||
| return Response(ChannelSerializer(result).data) | ||
| linked = tasks_facade.link_channel_folder(pk, self.team_id, folder_id=folder_id) |
There was a problem hiding this comment.
Any team member who knows another member's personal-channel UUID can change its folder association. This call passes only team_id, and link_channel_folder does not check the channel's created_by_id, so the new personal-channel mutation bypasses the owner-specific boundary used when reading personal channels.
Rule Used: When implementing new features, ensure that owners... (source)
Learned From
PostHog/posthog#31236
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/tasks/backend/presentation/views/channels_api.py
Line: 112
Comment:
**Personal Channel Ownership Bypass**
Any team member who knows another member's personal-channel UUID can change its folder association. This call passes only `team_id`, and `link_channel_folder` does not check the channel's `created_by_id`, so the new personal-channel mutation bypasses the owner-specific boundary used when reading personal channels.
**Rule Used:** When implementing new features, ensure that owners... ([source](https://app.greptile.com/posthog-org-19734/-/custom-context?memory=9655b466-451a-401a-9ba0-5bf3e7b7f9f8))
**Learned From**
[PostHog/posthog#31236](https://github.com/PostHog/posthog/pull/31236)
How can I resolve this? If you propose a fix, please make it concise.| channel.folder_id = folder_id | ||
| try: | ||
| with transaction.atomic(): | ||
| channel.save(update_fields=["folder", "updated_at"]) | ||
| except IntegrityError: | ||
| channel.folder_id = None |
There was a problem hiding this comment.
When a channel contains a dangling folder ID and the requested replacement is already claimed, the save rolls back but this handler changes only the in-memory object to None. The response reports folder_id: null while the database still contains the old UUID, so a later read returns different state.
| channel.folder_id = folder_id | |
| try: | |
| with transaction.atomic(): | |
| channel.save(update_fields=["folder", "updated_at"]) | |
| except IntegrityError: | |
| channel.folder_id = None | |
| channel.folder_id = folder_id | |
| try: | |
| with transaction.atomic(): | |
| channel.save(update_fields=["folder", "updated_at"]) | |
| except IntegrityError: | |
| channel.refresh_from_db(fields=["folder"]) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/tasks/backend/facade/api.py
Line: 5004-5009
Comment:
**Failed Repair Returns Wrong Link**
When a channel contains a dangling folder ID and the requested replacement is already claimed, the save rolls back but this handler changes only the in-memory object to `None`. The response reports `folder_id: null` while the database still contains the old UUID, so a later read returns different state.
```suggestion
channel.folder_id = folder_id
try:
with transaction.atomic():
channel.save(update_fields=["folder", "updated_at"])
except IntegrityError:
channel.refresh_from_db(fields=["folder"])
```
How can I resolve this? If you propose a fix, please make it concise.| name = serializers.CharField( | ||
| max_length=128, help_text="Channel name, rendered as #<name>. Normalized to lowercase-dashed." | ||
| max_length=128, | ||
| required=False, |
There was a problem hiding this comment.
This serializer is used for both POST and PATCH, so making name optional also marks it optional in the generated create schema. Generated clients can submit a schema-valid create request without a name, but create() always rejects that request with 400; separate create and update serializers are needed to keep the API contract accurate.
Context Used: docs/published/handbook/engineering/type-system.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/tasks/backend/presentation/serializers.py
Line: 1349-1351
Comment:
**Create Schema Marks Name Optional**
This serializer is used for both POST and PATCH, so making `name` optional also marks it optional in the generated create schema. Generated clients can submit a schema-valid create request without a name, but `create()` always rejects that request with 400; separate create and update serializers are needed to keep the API contract accurate.
**Context Used:** docs/published/handbook/engineering/type-system.md ([source](https://app.greptile.com/posthog-org-19734/github/PostHog/posthog/-/custom-context?memory=2d5b82d8-8608-4823-8983-4faaa9415b96))
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!
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| except IntegrityError: | ||
| channel.folder_id = None |
There was a problem hiding this comment.
After an IntegrityError, the in-memory channel object has stale data. The transaction was rolled back, so the database still has the original folder_id value, but the in-memory object now has folder_id = None. When the caller link_channel_folder (line 5019) returns _channel_to_dto(channel), it will return incorrect data showing folder_id as None when the database actually has a different value.
Fix by refreshing from the database:
except IntegrityError:
channel.refresh_from_db()This same bug affects line 5103 where resolve_channel calls _link_channel_folder and then returns the DTO without refreshing.
| except IntegrityError: | |
| channel.folder_id = None | |
| except IntegrityError: | |
| channel.refresh_from_db() | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
🦔 Hogbox preview · ✅ ready▶ Open the preview
commit |
🤖 CI report✅ Bundle size — no changeUncompressed size of every built Total: 64.19 MiB · no change No file changed by more than 1000 B. Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report ✅ Eager graph — within budgetHow much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy
🟢 Largest files eagerly shipped from
|
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 281.3 KiB | ../node_modules/.pnpm/posthog-js@1.404.1/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 235.8 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 223.4 KiB | ../node_modules/.pnpm/posthog-js@1.404.1/node_modules/posthog-js/dist/module.js |
| 167.1 KiB | src/queries/validators.js |
| 154.3 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 105.9 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 93.2 KiB | ../packages/quill/packages/quill/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Dist folder size — 🔺 +1.5 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1349.19 MiB · 🔺 +1.5 KiB (+0.0%)
⚠️ Backend coverage — 94.0% of changed backend lines covered — 6 uncovered
🧪 Backend test coverage
Patch coverage — changed backend lines (products + core): ███████████████████░ 94.0% (104 / 110)
| File | Patch | Uncovered changed lines |
|---|---|---|
products/tasks/backend/presentation/views/channels_api.py |
82.6% | 71, 101, 105, 107 |
products/tasks/backend/facade/api.py |
92.0% | 4996, 5017 |
🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 29789077554 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
demo |
███████████░░░░░░░░░ 56.2% |
1,497 / 2,663 |
tasks |
█████████████░░░░░░░ 66.3% |
26,426 / 39,851 |
signals |
████████████████░░░░ 79.7% |
19,751 / 24,782 |
data_modeling |
████████████████░░░░ 80.0% |
4,834 / 6,045 |
cdp |
████████████████░░░░ 80.7% |
3,118 / 3,864 |
notebooks |
█████████████████░░░ 85.0% |
7,101 / 8,355 |
cohorts |
█████████████████░░░ 86.2% |
4,063 / 4,715 |
agent_platform |
█████████████████░░░ 86.4% |
3,807 / 4,405 |
actions |
█████████████████░░░ 86.6% |
717 / 828 |
product_tours |
█████████████████░░░ 87.5% |
1,266 / 1,447 |
exports |
██████████████████░░ 88.4% |
6,943 / 7,853 |
conversations |
██████████████████░░ 89.1% |
16,387 / 18,386 |
mcp_analytics |
██████████████████░░ 89.2% |
2,514 / 2,819 |
dashboards |
██████████████████░░ 89.3% |
5,928 / 6,640 |
engineering_analytics |
██████████████████░░ 89.5% |
5,507 / 6,154 |
alerts |
██████████████████░░ 89.9% |
4,054 / 4,508 |
error_tracking |
██████████████████░░ 90.1% |
9,757 / 10,828 |
early_access_features |
██████████████████░░ 90.1% |
1,031 / 1,144 |
streamlit_apps |
██████████████████░░ 90.4% |
2,501 / 2,767 |
slack_app |
██████████████████░░ 90.6% |
8,989 / 9,926 |
marketing_analytics |
██████████████████░░ 90.8% |
11,514 / 12,684 |
stamphog |
██████████████████░░ 91.0% |
3,993 / 4,387 |
product_analytics |
██████████████████░░ 91.3% |
5,786 / 6,337 |
data_warehouse |
██████████████████░░ 92.5% |
18,819 / 20,348 |
ai_observability |
███████████████████░ 92.7% |
14,921 / 16,093 |
workflows |
███████████████████░ 92.8% |
5,520 / 5,949 |
web_analytics |
███████████████████░ 92.9% |
13,853 / 14,913 |
surveys |
███████████████████░ 93.0% |
5,724 / 6,157 |
posthog_ai |
███████████████████░ 93.2% |
1,325 / 1,421 |
approvals |
███████████████████░ 93.3% |
3,395 / 3,640 |
reminders |
███████████████████░ 93.4% |
468 / 501 |
endpoints |
███████████████████░ 94.1% |
8,606 / 9,143 |
revenue_analytics |
███████████████████░ 94.5% |
3,598 / 3,809 |
skills |
███████████████████░ 94.5% |
2,893 / 3,061 |
review_hog |
███████████████████░ 94.6% |
6,806 / 7,191 |
logs |
███████████████████░ 95.4% |
9,935 / 10,416 |
experiments |
███████████████████░ 95.7% |
24,561 / 25,671 |
replay_vision |
███████████████████░ 95.8% |
13,850 / 14,459 |
annotations |
███████████████████░ 96.2% |
732 / 761 |
feature_flags |
███████████████████░ 96.3% |
16,265 / 16,891 |
warehouse_sources |
███████████████████░ 96.5% |
289,438 / 300,067 |
user_interviews |
███████████████████░ 96.5% |
2,639 / 2,735 |
customer_analytics |
███████████████████░ 97.3% |
7,780 / 7,999 |
data_catalog |
███████████████████░ 97.5% |
2,346 / 2,407 |
pulse |
████████████████████ 98.4% |
2,017 / 2,049 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
⚠️ Django migration SQL — 1 new migration to review
We've detected new migrations on this PR. Review the SQL output for each migration:
products/tasks/backend/migrations/0063_channel_folder_channel_task_channel_folder_unique.py
BEGIN;
--
-- Add field folder to channel
--
ALTER TABLE "posthog_task_channel" ADD COLUMN "folder_id" uuid NULL;
--
-- Create constraint task_channel_folder_unique on model channel
--
CREATE UNIQUE INDEX "task_channel_folder_unique" ON "posthog_task_channel" ("folder_id") WHERE (NOT "deleted" AND "folder_id" IS NOT NULL);
CREATE INDEX "posthog_task_channel_folder_id_fb10b098" ON "posthog_task_channel" ("folder_id");
COMMIT;Last updated: 2026-07-21 00:05 UTC (2be8198)
✅ Django migration risk — migration analysis complete
We've analyzed your migrations for potential risks.
Summary: 0 Safe | 1 Needs Review | 0 Blocked
⚠️ Needs Review
May have performance impact
tasks.0063_channel_folder_channel_task_channel_folder_unique
└─ #1 ✅ AddField
Adding nullable field requires brief lock
model: channel, field: folder
└─ #2 ⚠️ AddConstraint
Adding constraint may lock table (use NOT VALID pattern)
model: channel
📚 How to Deploy These Changes Safely
AddConstraint:
Add constraints in 2 phases without locking, using the PostHog helpers:
-
AddConstraintNotValid (instant, validates new rows only, no table scan)
-
ValidateConstraint in a separate migration (scans table with non-blocking lock)
from posthog.migration_helpers import AddConstraintNotValid, ValidateConstraint
See the migration safety guide
AddField:
This operation acquires a brief lock but doesn't rewrite the table.
Deployment uses lock timeouts with automatic retries, so lock contention will cause retries rather than connection pile-up.
Last updated: 2026-07-21 00:05 UTC (2be8198)
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| def link_channel_folder(channel_id: str | UUID, team_id: int, *, folder_id: UUID) -> contracts.ChannelDTO | str: | ||
| """Attach the desktop-fs folder that renders a channel (personal channels | ||
| included). Returns the DTO — unchanged if the claim was ignored — or ``not_found``.""" | ||
| channel = Channel.objects.select_related("created_by").filter(id=channel_id, team_id=team_id, deleted=False).first() |
There was a problem hiding this comment.
Medium: Personal channel ownership bypass
This query allows any team member who obtains another user's personal channel ID to attach an arbitrary team folder to it and receive its DTO. Pass the requesting user ID into this function and reject personal channels whose created_by_id does not match, consistent with _visible_channel and task channel validation.
PR overviewThis PR adds backend support for linking a task Channel to the desktop-fs folder that renders it. The touched API path appears to create or return the channel-folder association DTO used by the desktop filesystem integration. There is one open security issue: the current channel-folder linking path does not enforce ownership for personal channels, so a team member who knows another user’s personal channel ID could attach a team folder to it and receive the resulting DTO. No issues have been fixed or addressed yet, so this ownership check remains the main blocker before the PR is in a good security state. Open issues (1)
Fixed/addressed: 0 · PR risk: 6/10 |
1 participant
Problem
A channel exists as two unrelated rows: the
Channelmodel (feed, membership,Task.channelFK) and a desktop-surfaceFileSystemfolder — the tree the desktop app actually renders, under whose path canvases and task filings nest. The only join between them is display-name matching, with the normalization function mirrored client- and server-side (normalize_channel_name↔normalizeChannelName). Anything that needs to walk from a task to its channel's folder — e.g. agent-side canvas placement in PostHog/code#3594 — is stranded one id hop short, and ends up scraping filing-row path conventions or relaying channel names through the model.Changes
Channel.folderFK →FileSystem(nullable,SET_NULL,db_constraint=Falselike the model's other FKs, one live channel per folder via a partial unique constraint).POST /task_channels/) accepts an optionalfolder_id;PATCH /task_channels/:id/acceptsfolder_idtoo (personal channels included — rename stays public-only), so clients that own folder creation can link at creation and backfill lazily.ChannelSerializernow returnsfolder_id, so any consumer resolvestask → channel → folderpurely by ids.Follow-up in
posthog/code: passfolder_idfrom the app's existingresolveTaskChannel/useBackendChannelseam, and switch the canvas tool's placement to this join (replacing the filing-row lookup).How did you test this code?
pytest products/tasks/backend/tests/test_channels_api.py— 42 passed (8 new: link-on-resolve, first-claim-wins, exclusivity across channels, personal-channel PATCH link, foreign/non-desktop/non-folder rejection, relink after folder deletion, rename preserves link, empty PATCH 400).pytest posthog/api/test/test_api_docs.py— schema generation passes with the new field.posthog_task_channeltable).Created with PostHog Code