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 |
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
🤖 CI report
|
HostHog preview —
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCanvas comment targets now use the Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Opening a canvas thread from a task timeline does not focus the thread, and some taskless activity rows cannot be opened from the rail. Align the focus keys before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The reviewed server code checks canvas visibility for comment reads and writes, and the Desktop change retains the canvas as the comment target when no task exists. The remaining risk is rollout order: enabling the feature before the required server protection is deployed could expose a contract the older deployment does not safely support. No introduced authorization bypass was 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✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
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 (1)
products/desktop/packages/ui/src/features/canvas/freeform/CanvasSidePanel.tsx-287-287 (1)
287-287: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve legacy
desktop_canvascomments when queryingcanvas.
CanvasSidePanelandGridChatPanelnow query comments withscope=canvas. The backend applies an exactscope=scopefilter, so existing rows withscope=desktop_canvasare excluded before they reach the frontend. Users can lose existing comments in both freeform and grid views.Add a backend compatibility alias or migrate the legacy rows. Frontend response normalization cannot restore rows that the query excludes.
Suggested fix
-queryset = queryset.filter(scope=scope) +queryset = queryset.filter( + scope__in=("canvas", "desktop_canvas") if scope == "canvas" else (scope,) +)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: db01397f-1378-42f8-b95d-0efd3a63e8bc
📒 Files selected for processing (26)
products/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/ui/src/features/canvas/components/ActivityRow.test.tsxproducts/desktop/packages/ui/src/features/canvas/components/ActivityTimeline.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/TaskCommentsList.test.tsxproducts/desktop/packages/ui/src/features/canvas/components/activityFeed.test.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.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/grid/GridChatPanel.tsxproducts/desktop/packages/ui/src/features/canvas/hooks/useActivityTaskMenu.test.tsxproducts/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.ts
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
0b12805 to
4a522ca
Compare
4a522ca to
c396285
Compare
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/desktop/packages/ui/src/features/canvas/components/openActivityItem.ts-51-53 (1)
51-53: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle taskless non-canvas activity items in the rail.
The feed keeps every item with
taskId === null, including items without a canvas comment target. The rail then callsopenActivityItem, which returns for those items. The row cannot be opened or selected.Keep taskless canvas comments on the canvas path. Select other taskless items in the rail.
Suggested fix
if (!item.taskId) { - openActivityItem(item); + if (item.channelId && item.commentTarget?.scope === "canvas") { + openActivityItem(item); + } else { + selectActivityItem(item); + } return; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 92273440-3e67-4ca0-ab1f-84573231ec1b
📒 Files selected for processing (8)
products/desktop/packages/api-client/src/posthog-client.tsproducts/desktop/packages/core/src/comments/anchors.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/ChannelsSidebar.tsxproducts/desktop/packages/ui/src/features/canvas/components/TaskCommentsList.tsxproducts/desktop/packages/ui/src/features/canvas/components/openActivityItem.tsproducts/desktop/packages/ui/src/features/sessions/components/useComments.ts
💤 Files with no reviewable changes (1)
- products/desktop/packages/ui/src/features/canvas/components/ActivityRow.tsx
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 7 remain after this review.
2e3ae4b to
6076e8b
Compare
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
50f71eb to
0a3fde4
Compare
Problem
Changes
posthog-desktop-canvas-commentsflag.canvas. #106900 protects that name with the canvas space check, so this PR is stacked on it.commentScopeFromWirereads the old namedesktop_canvasascanvas, because the backend stores it until #107353, and older task timeline events and deep links carry it.COMMENT_SCOPESnow definesCommentScope, so the scope list is written once.Warning
Merge and turn on the flag only after #106900 deploys. An older backend rejects a canvas comment without a task, and it does not apply the space check to the
canvasscope.No new layout or copy. The only visible difference is the Comments tab and button state on a canvas without a task, so no screenshots.
How did you test this code?
CanvasSidePanel.test.tsx: opens comments on a canvas with no task, and disables the tab when comments are off. It fails if the tab depends on a task again.CanvasSelectionCommentAction.test.tsx: runs with and without a task, and checks the canvas focus key. It fails if the action needs a task or focuses the wrong key.ActivityRow.test.tsxandTaskCommentsList.test.tsx: now assert the canvas focus key for canvas threads.ActivityRow.test.tsxruns a mention through the feed, the rail, and the rail without a task. It fails if a path writes focus where its target does not read it, or if the rail opens nothing.TaskCommentsList.test.tsxopens a canvas thread from the task list. It fails if the canvas key gets no focus.anchors.test.ts: readscanvas,desktop_canvasand unknown scopes. It fails if an old deep link or timeline event stops opening its canvas thread.@posthog/ui,@posthog/core,@posthog/codeand@posthog/web, the mobile type baseline, Biome, and the canvas, sessions, deep-link and core canvas Vitest suites pass locally.Not done: rendering the change in Storybook or the running app, and manual testing.
Release status
The flag
posthog-desktop-canvas-commentsdoes not exist yet.Automatic notifications
Docs update
products/desktop/docs/DEEP-LINKS.mdnow namescanvasfor thescopeparameter, and says the app also acceptsdesktop_canvas.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: PostHog Desktop (Claude Code), Claude Opus 5.5
/writing-tests,/writing-pr-descriptions,/writing-simplified-technical-english.canvas-comments-v2. It becameposthog-desktop-canvas-commentsto match the other Desktop flag keys.gh pr list --search "canvas comments").useCanvasCommentsEnabled, and removed theenabledprop from the selected-text action.canvasscope finding moved the stored rename into feat(canvas): store canvas comments under the canvas scope #107353 and put this PR above feat(canvas): let canvas comments work without a task #106900. The task-timeline focus finding is fixed inTaskCommentsList, the step that opens the canvas./code-review(high): fixed the rail focus, the task-less rail row, the canvas list key, the task keys inTaskCommentsList, and the scope list. Rejected: sendingcanvaswithout a flag, because the stack keeps this PR behind feat(canvas): let canvas comments work without a task #106900; an empty task id in the row menu, because the backend writes a null task only for canvas rows; and renaming the focus store, which is cosmetic./security-review: no findings.Created with PostHog Desktop
🤖 Generated with Claude Code