trunk-merge/pr-106919/508d3d99-ccdc-45aa-9dee-736762fc5bd9-bisection - #108908
Closed
trunk-io[bot] wants to merge 25 commits into
Closed
trunk-io[bot] wants to merge 25 commits into
trunk-io[bot] wants to merge 25 commits into
Conversation
Canvas comment access now follows the space of the canvas. A task id on a canvas comment is optional metadata, and the API checks it only when a comment sets a new one. Comment activity rows can exist without a task, so the canvas owner and mentioned users get notified. Agents read all threads on a canvas through two new endpoints and MCP tools behind the canvas-comments-mcp flag. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: b496dfaa-ef83-402b-8d41-4579119729f5
Rename the base queryset parameter so it does not share a name with the list of thread entries, and look up an artifact owner only when the comment has a task. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: b496dfaa-ef83-402b-8d41-4579119729f5
The canvas comment tools add the canvas-comments-mcp flag, so the list of flags that tool definitions use grows to 37. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: b496dfaa-ef83-402b-8d41-4579119729f5
- Skills ship to every MCP client, so they no longer name the flag-gated canvas comment tools. The tool descriptions carry that guidance while the flag is on. - A truncated comment body ends its chunk on a UTF-8 character boundary, so the next chunk does not lose a multibyte character. - The Slack DM path does not load a task for a canvas comment, because it never reads it. - The two new canvas comment tests have return annotations. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: b496dfaa-ef83-402b-8d41-4579119729f5
Canvas comments now use the scope "canvas". A data migration moves existing "desktop_canvas" rows to the new name, and its reverse moves them back. Desktop builds that predate the rename still send "desktop_canvas", and pods on the previous release can write it during a deploy. The API accepts that name on input, normalizes it to "canvas", reads rows with either name, and returns "canvas". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: b496dfaa-ef83-402b-8d41-4579119729f5
The list filter already accepts both scope names, so it does not convert the query value. Analytics and thread-event payloads only see comments the new code wrote, which already carry "canvas". Desktop converts the activity feed scope itself, so the facade passes the stored value through. The scope names are inlined into the one set and one function that use them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: b496dfaa-ef83-402b-8d41-4579119729f5
Pods on the previous release treat "canvas" as an ordinary comment scope: they skip the space check and list those rows in unscoped queries. So this release protects both names but keeps storing "desktop_canvas". The rename of stored rows moves to a follow-up that lands after every pod protects both names. Canvas comments no longer write activity log rows, because the activity log cannot check the canvas's space. Rows written before this are hidden by the same visibility rule the ticket comment rows use. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: b496dfaa-ef83-402b-8d41-4579119729f5
An edit skipped the canvas-to-task check when the stored taskId was unchanged, even if item_id moved the comment to another canvas, and it skipped the check for every reply. Now only a new reply, which takes its link from the root, and an edit that keeps both the stored task and the canvas skip it. Completing or reopening a canvas comment task no longer writes an activity log row, and the visibility rule hides the rows written before this. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: b496dfaa-ef83-402b-8d41-4579119729f5
The comment list looked up Slack mirrors by the requested scope only, so a canvas thread mirrored under the other scope name lost its Slack link in the response. The lookup now matches both canvas scope names, like the comment filter does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: b496dfaa-ef83-402b-8d41-4579119729f5
The task comment list and detail endpoints included canvas comments by their taskId and did not check the canvas's space, so a caller who could see the task but not that space could read the comments. Both now include a canvas comment only when the requester can see its canvas. The facade's activity feed uses the same visible-canvas query. Migration canvas.0021 masks the text of activity log rows written for canvas comments and their replies before comments stopped writing them. Reply rows have the scope "Comment", so the visibility rule could not hide them. The migration finds them per team through the canvas comment roots. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: b496dfaa-ef83-402b-8d41-4579119729f5
Regenerated after the rebase onto master. The shared task_id help text now names the canvas scope, and the shorter string fits on one line. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: b496dfaa-ef83-402b-8d41-4579119729f5
A canvas comment without a task is checked only against the canvas's space. For a sandbox token that used the user's full channel access, so a task sandbox could read or write comments on a teammate's canvas in a private space the user belongs to. The canvas API gives sandbox tokens only public canvases and canvases the user created. The comment checks now apply the same limit to sandbox requests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: b496dfaa-ef83-402b-8d41-4579119729f5
Behind the posthog-desktop-canvas-comments flag, the Comments tab, the breadcrumb button and the text-selection action work on a canvas that no agent task backs. Canvas comment focus is keyed by the canvas, not by a task, and the activity feed accepts rows without a task id. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: b496dfaa-ef83-402b-8d41-4579119729f5
The freeform view, the grid view and the breadcrumb now read "comments on" from useCanvasCommentsEnabled instead of three copies of the flag check. The text-selection comment action no longer takes an enabled prop, because the freeform view does not render it when comments are off. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: b496dfaa-ef83-402b-8d41-4579119729f5
Desktop now sends and reads the scope "canvas" for canvas comments. commentScopeFromWire reads the old name "desktop_canvas" as "canvas", so older task timeline events and deep links still open their canvas thread. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: b496dfaa-ef83-402b-8d41-4579119729f5
The activity row wrote comment focus under the canvas key before it knew where activation would go, so the Activity rail, which opens the task, lost the thread. Each activation path now writes focus where its target reads it. The rail opens a canvas comment row that no task backs on the canvas, not on an empty selection. Canvas comment lists no longer key on the task id, which the backend ignores for canvas threads. TaskCommentsList passes the task id to task-keyed stores and uses the canvas key only for comment focus. The comment scope list is defined once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: b496dfaa-ef83-402b-8d41-4579119729f5
Opening a canvas thread from a task's comment list, or from its timeline through that list, wrote the focus under the task key and then opened the canvas. The canvas reads focus under its own key, so it did not reveal the thread. The list now writes the focus under the canvas key when it opens the canvas. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: b496dfaa-ef83-402b-8d41-4579119729f5
The comment list's send-to-agent actions need a task id, and a canvas comment list can now have none. The composer and thread replies offer the action only when a task exists, as the selected-text action already does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: b496dfaa-ef83-402b-8d41-4579119729f5
- skip heading-like lines inside fenced code when placing page edits - normalize blank lines only at edit boundaries - bound llm retries by one caller deadline - match content boundaries by path segment in both filters - follow same-site sitemap redirects and keep custom-port pages - cap the opportunities refresh response - give CONTENT_AUTOPILOT_MODEL its own default and drop the unused safety model setting Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t edits Match fence openers and closers separately, so an info-string fence line inside a code block no longer closes it and a backtick info string no longer opens one. Return a named section from _find_section for the tuple-return semgrep rule, and give the fake client in the call_json test a type mypy can resolve.
…762fc5bd9-bisection
…762fc5bd9-bisection
Contributor
🤖 CI report
|
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughCanvas comments now use a canonical stored scope and can be listed, retrieved, and created without an associated task. The changes add canvas visibility checks, taskless activity handling, desktop comment controls, and MCP read tools. Content Autopilot adds prompt and schema definitions, LLM request handling, Markdown edit utilities, and updates to site discovery and opportunity listing. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Batch the historical activity-log masking before merging to bound deployment work on larger installations. Update canvas comment guidance so taskless feedback is discoverable and private-space access is described accurately, including sandbox restrictions. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The inspected paths retain project isolation, Canvas visibility checks, restricted automated access, and recipient authorization before sending comment content. No introduced security vulnerability was established. Compatibility during deployment and rollback, and some Content Autopilot execution paths, remain incompletely verified. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
Full details: Description checkExplanation The description only documents Trunk Merge batching. It does not explain the user problem, changes, tests, release status, documentation status, or required agent context for the canvas comments and Content Autopilot changes. Resolution Replace the batch boilerplate with a complete repository PR description. Add Problem, Changes, How did you test this code?, Release status, Automatic notifications, Docs update, and Agent context sections. Describe the user-visible canvas comment and Content Autopilot behavior, list the automated tests that were run, document feature-flag status, and complete the required agent gates.
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
products/canvas/skills/composing-grid-canvases/SKILL.md-94-94 (1)
94-94: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPoint the skill at the canvas comment tools.
Line 94 now says comment threads belong to the canvas. Line 95 still tells agents to use
tasks-comments-listandtasks-comments-retrieveon their task. Those tools can miss threads with no task ID, and a canvas can have no task at all. The newcanvas-comments-listandcanvas-comments-retrievetools are behind thecanvas-comments-mcpflag. The path instructions forbid flag-gated tools in skills, so this skill should name the genericcomments-listtool filtered byscope=canvas.Source: Path instructions
products/platform_features/mcp/tools.yaml-318-319 (1)
318-319: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument private-space and sandbox access separately.
Regular MCP callers can comment on canvases in visible spaces, including private spaces where they are members. Sandbox OAuth callers can use
comments-create, but private-space membership alone does not grant them access.Suggested description update
- enforced: public-channel canvases and the authenticated user's personal-channel canvases are accessible. + enforced: users can read and write comments on canvases in spaces they can see, including private spaces where + they are members. Sandbox OAuth callers can access public canvases and canvases they created, but do not + inherit private-space access from their user.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 4be4190c-afd1-4262-a9f7-6f4169149c06
⛔ Files ignored due to path filters (8)
products/canvas/frontend/generated/api.schemas.tsis excluded by!**/generated/**products/canvas/frontend/generated/api.tsis excluded by!**/generated/**products/platform_features/frontend/generated/api.schemas.tsis excluded by!**/generated/**products/tasks/frontend/generated/api.schemas.tsis excluded by!**/generated/**products/tasks/frontend/generated/api.zod.tsis excluded by!**/generated/**services/mcp/src/generated/canvas/api.tsis excluded by!**/generated/**services/mcp/src/generated/platform_features/api.tsis excluded by!**/generated/**services/mcp/src/tools/generated/canvas.tsis excluded by!**/generated/**
📒 Files selected for processing (93)
posthog/api/comments.pyposthog/api/test/test_comment_slack_sync.pyposthog/api/test/test_comments.pyposthog/comment/access.pyposthog/models/activity_logging/activity_log.pyposthog/models/comment/comment.pyposthog/models/comment/utils.pyposthog/settings/web.pyposthog/tasks/test/test_email.pyposthog/test/activity_logging/test_activity_logging.pyproducts/canvas/backend/comment_access.pyproducts/canvas/backend/migrations/0021_mask_canvas_comment_activity.pyproducts/canvas/backend/migrations/max_migration.txtproducts/canvas/backend/presentation/serializers.pyproducts/canvas/backend/presentation/views.pyproducts/canvas/backend/tests/test_canvas_api.pyproducts/canvas/backend/tests/test_migration_mask_canvas_comment_activity.pyproducts/canvas/mcp/tools.yamlproducts/canvas/skills/composing-grid-canvases/SKILL.mdproducts/canvas/skills/querying-canvas-data/SKILL.mdproducts/desktop/docs/DEEP-LINKS.mdproducts/desktop/packages/api-client/src/posthog-client.tsproducts/desktop/packages/core/src/canvas/taskActivity.tsproducts/desktop/packages/core/src/comments/anchors.test.tsproducts/desktop/packages/core/src/comments/anchors.tsproducts/desktop/packages/core/src/links/task-link.test.tsproducts/desktop/packages/shared/src/domain-types.tsproducts/desktop/packages/shared/src/feature-flag-keys.jsonproducts/desktop/packages/shared/src/flags.tsproducts/desktop/packages/ui/src/features/canvas/components/ActivityRow.test.tsxproducts/desktop/packages/ui/src/features/canvas/components/ActivityRow.tsxproducts/desktop/packages/ui/src/features/canvas/components/ActivityTimeline.tsxproducts/desktop/packages/ui/src/features/canvas/components/ChannelsSidebar.tsxproducts/desktop/packages/ui/src/features/canvas/components/ShellLayout.test.tsxproducts/desktop/packages/ui/src/features/canvas/components/ShellLayout.tsxproducts/desktop/packages/ui/src/features/canvas/components/SpaceHeaderRow.test.tsxproducts/desktop/packages/ui/src/features/canvas/components/TaskCommentsList.test.tsxproducts/desktop/packages/ui/src/features/canvas/components/TaskCommentsList.tsxproducts/desktop/packages/ui/src/features/canvas/components/activityFeed.test.tsproducts/desktop/packages/ui/src/features/canvas/components/activityFeed.tsproducts/desktop/packages/ui/src/features/canvas/components/activityPresentation.test.tsproducts/desktop/packages/ui/src/features/canvas/components/activityPresentation.tsproducts/desktop/packages/ui/src/features/canvas/components/activityRows.tsxproducts/desktop/packages/ui/src/features/canvas/components/openActivityItem.tsproducts/desktop/packages/ui/src/features/canvas/components/taskArtifactRows.tsproducts/desktop/packages/ui/src/features/canvas/freeform/CanvasSelectionCommentAction.test.tsxproducts/desktop/packages/ui/src/features/canvas/freeform/CanvasSelectionCommentAction.tsxproducts/desktop/packages/ui/src/features/canvas/freeform/CanvasSidePanel.test.tsxproducts/desktop/packages/ui/src/features/canvas/freeform/CanvasSidePanel.tsxproducts/desktop/packages/ui/src/features/canvas/freeform/FreeformCanvasView.tsxproducts/desktop/packages/ui/src/features/canvas/freeform/canvasSidePanelVisibility.test.tsproducts/desktop/packages/ui/src/features/canvas/freeform/canvasSidePanelVisibility.tsproducts/desktop/packages/ui/src/features/canvas/grid/GridCanvasView.tsxproducts/desktop/packages/ui/src/features/canvas/grid/GridChatPanel.tsxproducts/desktop/packages/ui/src/features/canvas/hooks/useActivityTaskMenu.test.tsxproducts/desktop/packages/ui/src/features/canvas/hooks/useActivityTaskMenu.tsproducts/desktop/packages/ui/src/features/canvas/hooks/useCanvasCommentsEnabled.tsproducts/desktop/packages/ui/src/features/canvas/stores/activityDetailStore.tsproducts/desktop/packages/ui/src/features/deep-links/useHandleOpenTask.tsproducts/desktop/packages/ui/src/features/sessions/commentNavigationStore.test.tsproducts/desktop/packages/ui/src/features/sessions/commentNavigationStore.tsproducts/desktop/packages/ui/src/features/sessions/components/useComments.test.tsproducts/desktop/packages/ui/src/features/sessions/components/useComments.tsproducts/platform_features/mcp/tools.yamlproducts/tasks/backend/facade/api.pyproducts/tasks/backend/facade/contracts.pyproducts/tasks/backend/logic/services/comment_activity.pyproducts/tasks/backend/logic/services/comment_slack_dm.pyproducts/tasks/backend/logic/services/task_comments.pyproducts/tasks/backend/migrations/0133_task_comment_activity_optional_task.pyproducts/tasks/backend/migrations/max_migration.txtproducts/tasks/backend/models.pyproducts/tasks/backend/presentation/serializers.pyproducts/tasks/backend/presentation/views/api.pyproducts/tasks/backend/presentation/views/channels_api.pyproducts/tasks/backend/tasks/tasks.pyproducts/tasks/backend/tests/test_comment_activity.pyproducts/tasks/backend/tests/test_comment_slack_dm.pyproducts/web_analytics/backend/content_autopilot/edits.pyproducts/web_analytics/backend/content_autopilot/llm.pyproducts/web_analytics/backend/content_autopilot/opportunities.pyproducts/web_analytics/backend/content_autopilot/prompts.pyproducts/web_analytics/backend/content_autopilot/site_discovery.pyproducts/web_analytics/backend/facade/content_autopilot.pyproducts/web_analytics/backend/presentation/views/content_autopilot.pyproducts/web_analytics/backend/test/test_content_autopilot_generation.pyproducts/web_analytics/backend/test/test_content_autopilot_opportunities.pyproducts/web_analytics/backend/test/test_content_autopilot_site_discovery.pyservices/mcp/schema/generated-tool-definitions.jsonservices/mcp/schema/tool-definitions-all.jsonservices/mcp/src/api/generated.tsservices/mcp/tests/unit/__snapshots__/tool-schemas/comments-list.jsonservices/mcp/tests/unit/tool-filtering.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 7 remain after this review.
Comment on lines
+11
to
+32
| def mask_canvas_comment_activity(apps: Apps, schema_editor: BaseDatabaseSchemaEditor) -> None: | ||
| Comment = apps.get_model("posthog", "Comment") | ||
| ActivityLog = apps.get_model("posthog", "ActivityLog") | ||
| root_ids_by_team: dict[int, list[str]] = defaultdict(list) | ||
| for team_id, comment_id in Comment.objects.filter( | ||
| scope__in=CANVAS_COMMENT_SCOPES, source_comment__isnull=True | ||
| ).values_list("team_id", "id"): | ||
| root_ids_by_team[team_id].append(str(comment_id)) | ||
|
|
||
| for team_id, root_ids in root_ids_by_team.items(): | ||
| rows = ActivityLog.objects.filter(team_id=team_id).filter( | ||
| Q(scope__in=CANVAS_COMMENT_SCOPES) | Q(scope="Comment", item_id__in=root_ids) | ||
| ) | ||
| for row in rows.only("id", "detail"): | ||
| changes = (row.detail or {}).get("changes") or [] | ||
| masked = False | ||
| for change in changes: | ||
| if isinstance(change, dict) and change.get("field") == "content" and change.get("after") != "masked": | ||
| change["after"] = "masked" | ||
| masked = True | ||
| if masked: | ||
| row.save(update_fields=["detail"]) |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Batch this data migration over ActivityLog.
The migration loads every canvas root comment ID into memory. It then builds one unbounded item_id__in list per team. It saves ActivityLog rows one at a time inside one transaction. ActivityLog is a large table. On a team with many canvas comments, this can hold locks for a long time and stall the deploy. It can also produce a very large IN clause.
Fix:
- Iterate root IDs in fixed-size chunks and use
.iterator(). - Collect the changed rows and write them with
bulk_update(..., ["detail"], batch_size=...). - Set
atomic = Falseso each batch commits separately. The masking is idempotent, so a retry is safe. Alternatively, move the backfill to a management command or async job.
As per coding guidelines: "Avoid migrations that process rows individually on large tables… they may take forever or lock the entire table" and "Break large updates into small batches."
Source: Coding guidelines
Author
|
|
trunk-io
Bot
deleted the
trunk-merge/pr-106919/508d3d99-ccdc-45aa-9dee-736762fc5bd9-bisection
branch
September 30, 2026 05:38
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
2 participants
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.
This pull request was created and is being managed by Trunk Merge.
This pull request is based on the master branch at SHA 1c66a26fd9f969905a64bc564e302f70bee1736d.
See more details about each PR in the batch here:
When CI completes, this pull request will be closed automatically.
Pull Requests Being Tested
This pull request is testing a batch with the changes from pull requests 106919 and 108809 - batching documentation.
Pull request 106919 is stacked on pull request 106900, whose changes are included here and will be merged with it.
Batch Bisection
This pull request is in a batch bisection. Pull requests successfully tested by this PR will re-enter the main queue.