diff --git a/.github/workflows/ci-frontend.yml b/.github/workflows/ci-frontend.yml index 44aade675885..0c1bdab7d425 100644 --- a/.github/workflows/ci-frontend.yml +++ b/.github/workflows/ci-frontend.yml @@ -700,10 +700,10 @@ jobs: jest: runs-on: ubuntu-latest # Draft PRs run the selective mode as a single unsharded job over every - # test reachable from the changed files. A PR touching a scene component - # can pull in ~700 suites, which does not fit in 15 minutes. 20 covers it; - # the ready-for-review fanout shards well below either limit. - timeout-minutes: 20 + # test reachable from the changed files. A PR touching shared components + # can pull in enough suites to exceed 20 minutes. The ready-for-review + # fanout shards the same tests well below this limit. + timeout-minutes: 30 needs: [changes, select-jest-tests] # A status function is required because select-jest-tests is skipped on ready # PRs and master pushes, and a skipped need would otherwise cascade a skip here. diff --git a/.semgrep/rules/security/idor-team-scoped-models.yaml b/.semgrep/rules/security/idor-team-scoped-models.yaml index 6b275336bf88..a166796e3a96 100644 --- a/.semgrep/rules/security/idor-team-scoped-models.yaml +++ b/.semgrep/rules/security/idor-team-scoped-models.yaml @@ -308,6 +308,7 @@ rules: |Task |TaskActivity |TaskArtifact + |TaskCommentActivity |TaskAutomation |TaskPresence |TaskRun @@ -648,6 +649,7 @@ rules: |Task |TaskActivity |TaskArtifact + |TaskCommentActivity |TaskAutomation |TaskPresence |TaskRun diff --git a/posthog/api/comments.py b/posthog/api/comments.py index 23f7ed7b1d43..4f27f158438c 100644 --- a/posthog/api/comments.py +++ b/posthog/api/comments.py @@ -1,4 +1,7 @@ +import logging +from datetime import datetime from typing import TYPE_CHECKING, Any, cast +from uuid import UUID from django.core import exceptions as django_exceptions from django.db import transaction @@ -7,6 +10,7 @@ from drf_spectacular.utils import extend_schema from rest_framework import exceptions, pagination, serializers, viewsets +from rest_framework.generics import get_object_or_404 from rest_framework.request import Request from rest_framework.response import Response @@ -25,6 +29,8 @@ if TYPE_CHECKING: from posthog.rbac.user_access_control import UserAccessControl +logger = logging.getLogger(__name__) + def _normalize_scope(scope: Any) -> Any: """Match how the serializer will store a submitted scope. @@ -58,6 +64,104 @@ def _require_ticket_editor_access( raise exceptions.PermissionDenied("You do not have access to this ticket") +def _record_task_comment_activity( + comment: Comment, + mentions: list[int], + *, + activity_at: datetime | None = None, + include_relationship_recipients: bool = True, +) -> None: + if comment.scope not in {"task", "task_artifact", "desktop_canvas"}: + return + + try: + from products.tasks.backend.facade.api import ( # noqa: PLC0415 — keeps the generic comments API decoupled from the tasks product + record_comment_activity, + ) + + owner_id = None + if comment.scope == "desktop_canvas" and comment.item_id: + from products.canvas.backend.comment_access import canvas_owner_id # noqa: PLC0415 + + owner_id = canvas_owner_id(team_id=comment.team_id, canvas_id=comment.item_id) + + record_comment_activity( + team_id=comment.team_id, + comment_id=comment.id, + mentioned_user_ids=mentions, + include_relationship_recipients=include_relationship_recipients, + target_owner_id=owner_id, + activity_at=activity_at, + ) + except Exception: + logger.exception("Failed to project task comment activity", extra={"comment_id": str(comment.id)}) + from products.tasks.backend.facade.api import ( # noqa: PLC0415 — keeps the generic comments API decoupled from the tasks product + enqueue_comment_activity_retry, + ) + + activity_at_value = activity_at.isoformat() if activity_at else None + transaction.on_commit( + lambda: enqueue_comment_activity_retry( + team_id=comment.team_id, + comment_id=str(comment.id), + mentioned_user_ids=mentions, + include_relationship_recipients=include_relationship_recipients, + target_owner_id=owner_id, + activity_at=activity_at_value, + ) + ) + + +def _mentions_allowed_for_comment_target( + *, team_id: int, scope: str, item_id: str | None, item_context: dict | None +) -> bool: + if scope not in {"task", "task_artifact", "desktop_canvas"}: + return True + task_id = item_id if scope == "task" else (item_context or {}).get("taskId") + if not task_id: + return False + from products.tasks.backend.facade.api import task_comment_mentions_allowed # noqa: PLC0415 + + return task_comment_mentions_allowed(team_id=team_id, task_id=task_id) + + +def _task_comment_target_is_accessible( + *, team_id: int, user_id: int | None, task_id: str, scope: str, item_id: str | None +) -> bool: + from products.tasks.backend.facade.api import task_comment_target_is_accessible # noqa: PLC0415 + + if scope != "desktop_canvas": + return task_comment_target_is_accessible( + team_id=team_id, + user_id=user_id, + task_id=task_id, + scope=scope, + item_id=item_id, + ) + if not task_comment_target_is_accessible( + team_id=team_id, + user_id=user_id, + task_id=task_id, + scope="task", + item_id=task_id, + ): + return False + + from products.canvas.backend.comment_access import canvas_belongs_to_task # noqa: PLC0415 + + try: + parsed_task_id = UUID(task_id) + except ValueError: + return False + if not item_id: + return False + return canvas_belongs_to_task( + team_id=team_id, + canvas_id=item_id, + task_id=parsed_task_id, + ) + + class CommentSerializer(serializers.ModelSerializer): def _extract_mentions_from_rich_content(self, rich_content: dict | None) -> list[int]: if not rich_content: @@ -81,7 +185,12 @@ def find_mentions(node: Any) -> None: find_mentions(rich_content) return mentions - created_by = UserBasicSerializer(read_only=True) + created_by = UserBasicSerializer(read_only=True, allow_null=True) + item_context = serializers.JSONField( + required=False, + allow_null=True, + help_text="Metadata for the comment target, anchor, thread state, and owning task.", + ) deleted = ClassicBehaviorBooleanFieldSerializer() mentions = serializers.ListField(child=serializers.IntegerField(), write_only=True, required=False) slug = serializers.CharField(write_only=True, required=False) @@ -164,6 +273,27 @@ def validate(self, data): # parent — so losing ticket editor access after creation, re-scoping a comment into or out # of a ticket, and replying into a thread on another ticket are all caught, not just fresh # ticket-message creation. + if not instance and source_comment is not None: + root = source_comment.source_comment or source_comment + data["source_comment"] = root + data["scope"] = root.scope + data["item_id"] = root.item_id + reply_context = data.get("item_context") or {} + # Replies inherit the root's context (anchor, taskId) so filters keep + # working, but a reply's own signal keys must survive the merge. + data["item_context"] = { + **(root.item_context or {}), + **({"is_emoji": reply_context["is_emoji"]} if "is_emoji" in reply_context else {}), + **( + {"threadState": reply_context["threadState"]} + if reply_context.get("threadState") in ("resolved", "open") + else {} + ), + } + source_comment = root + scope = root.scope + item_id = root.item_id + scopes_and_items = {(scope, item_id)} if instance: scopes_and_items.add((instance.scope, instance.item_id)) @@ -177,6 +307,20 @@ def validate(self, data): user_access_control=self.context["get_user_access_control"](), ) + target_scope = data.get("scope", instance.scope if instance else None) + target_item_id = data.get("item_id", instance.item_id if instance else None) + target_context = data.get("item_context", instance.item_context if instance else None) or {} + if target_scope in {"task", "task_artifact", "desktop_canvas"}: + task_id = target_item_id if target_scope == "task" else target_context.get("taskId") + if not _task_comment_target_is_accessible( + team_id=self.context["get_team"]().id, + user_id=request.user.id, + task_id=task_id or "", + scope=target_scope, + item_id=target_item_id, + ): + raise exceptions.PermissionDenied("You do not have access to this task comment target") + # Skip content validation when soft-deleting a comment is_deleting = data.get("deleted") is True if not is_deleting: @@ -218,6 +362,13 @@ def create(self, validated_data: Any) -> Any: validated_data["team_id"] = self.context["team_id"] mentions = self._filter_mentions_to_organization(mentions, self.context["get_organization"]().id) + if not _mentions_allowed_for_comment_target( + team_id=self.context["team_id"], + scope=validated_data["scope"], + item_id=validated_data.get("item_id"), + item_context=validated_data.get("item_context"), + ): + mentions = [] comment = super().create(validated_data) @@ -225,6 +376,7 @@ def create(self, validated_data: Any) -> Any: send_discussions_mentioned.delay(comment.id, mentions, slug) produce_discussion_mention_events(comment, mentions, slug) send_mention_notifications(comment, mentions, slug) + _record_task_comment_activity(comment, mentions) return comment @@ -238,6 +390,13 @@ def update(self, instance: Comment, validated_data: dict, **kwargs: Any) -> Comm request = self.context["request"] mentions = self._filter_mentions_to_organization(mentions, self.context["get_organization"]().id) + if not _mentions_allowed_for_comment_target( + team_id=instance.team_id, + scope=validated_data.get("scope", instance.scope), + item_id=validated_data.get("item_id", instance.item_id), + item_context=validated_data.get("item_context", instance.item_context), + ): + mentions = [] with transaction.atomic(): locked_instance = Comment.objects.select_for_update().get(pk=instance.pk) @@ -256,6 +415,12 @@ def update(self, instance: Comment, validated_data: dict, **kwargs: Any) -> Comm send_discussions_mentioned.delay(updated_instance.id, mentions, slug) produce_discussion_mention_events(updated_instance, mentions, slug) send_mention_notifications(updated_instance, mentions, slug) + _record_task_comment_activity( + updated_instance, + mentions, + activity_at=timezone.now(), + include_relationship_recipients=False, + ) return updated_instance @@ -274,6 +439,9 @@ class CommentListQueryParamsSerializer(serializers.Serializer): ), ) item_id = serializers.CharField(required=False, help_text="Filter by the ID of the resource being commented on.") + task_id = serializers.UUIDField( + required=False, help_text="Owning task for task, task_artifact, and desktop_canvas comment scopes." + ) search = serializers.CharField(required=False, help_text="Full-text search within comment content.") source_comment = serializers.CharField(required=False, help_text="Filter replies to a specific parent comment.") kind = serializers.ChoiceField( @@ -388,6 +556,22 @@ def _require_ticket_viewer_access_for_pk(self) -> None: # Match the list path, where a denied ticket's comments are simply absent. raise exceptions.NotFound() + def safely_get_object(self, queryset: QuerySet) -> Comment: + lookup_url_kwarg = self.lookup_url_kwarg or self.lookup_field + lookup_value = self.kwargs[lookup_url_kwarg] + comment = get_object_or_404(queryset, **{self.lookup_field: lookup_value}) + if comment.scope in {"task", "task_artifact", "desktop_canvas"}: + task_id = comment.item_id if comment.scope == "task" else (comment.item_context or {}).get("taskId") + if not _task_comment_target_is_accessible( + team_id=self.team_id, + user_id=self.request.user.id, + task_id=task_id or "", + scope=comment.scope, + item_id=comment.item_id, + ): + raise exceptions.NotFound() + return comment + def _filter_ticket_scoped_queryset(self, queryset: QuerySet, item_id: str | None) -> QuerySet: """Ticket-carrying comments are ticket content — restrict them to tickets the caller has viewer access to, mirroring TicketViewSet's own object-level filtering.""" @@ -431,14 +615,24 @@ def safely_get_queryset(self, queryset: QuerySet) -> QuerySet: queryset = queryset.filter(scope=scope) if scope in TICKET_COMMENT_SCOPES: queryset = self._filter_ticket_scoped_queryset(queryset, params.get("item_id")) + elif scope in {"task", "task_artifact", "desktop_canvas"}: + task_id = params.get("task_id") + item_id = params.get("item_id") + if not _task_comment_target_is_accessible( + team_id=self.team_id, + user_id=self.request.user.id, + task_id=task_id or "", + scope=scope, + item_id=item_id, + ): + return queryset.none() + if scope != "task": + queryset = queryset.filter(item_context__taskId=str(task_id)) elif self.action in ("list", "count"): - # Ticket-carrying comments (customer messages and internal ticket discussions) never - # appear in unscoped enumeration — only when explicitly requested by scope. - queryset = queryset.exclude(scope__in=TICKET_COMMENT_SCOPES) + # Product-owned scopes require their own object-level access checks and must + # never leak through an unscoped generic comments query. + queryset = queryset.exclude(scope__in=[*TICKET_COMMENT_SCOPES, "task", "task_artifact", "desktop_canvas"]) else: - # Detail actions (retrieve, thread, send_to_slack, ...) carry no scope param, so the - # branch above never gates them — and API scope access doesn't cover session callers - # denied the ticket. Check the pk target's own ticket instead. self._require_ticket_viewer_access_for_pk() if params.get("item_id"): diff --git a/posthog/api/test/test_comments.py b/posthog/api/test/test_comments.py index 5ed55eb4a6da..379a51977428 100644 --- a/posthog/api/test/test_comments.py +++ b/posthog/api/test/test_comments.py @@ -1,9 +1,12 @@ +from datetime import timedelta from typing import Any from posthog.test.base import APIBaseTest, QueryMatchingTest from unittest import mock +from django.apps import apps from django.conf import settings +from django.utils import timezone from parameterized import parameterized from rest_framework import status @@ -12,6 +15,8 @@ from posthog.models.activity_logging.activity_log import ActivityLog from posthog.models.comment import Comment from posthog.models.comment.utils import build_comment_item_url, extract_plain_text_from_rich_content +from posthog.models.oauth import OAuthAccessToken, OAuthApplication +from posthog.temporal.oauth import ARRAY_APP_CLIENT_ID_DEV, POSTHOG_AI_APP_CLIENT_ID_DEV from products.conversations.backend.models import Ticket from products.conversations.backend.models.constants import Channel, Status @@ -20,6 +25,589 @@ class TestComments(APIBaseTest, QueryMatchingTest): + def _sandbox_task_comment_client( + self, task_id=None, *, client_id=ARRAY_APP_CLIENT_ID_DEV, scopes="task:read comment:read" + ): + app = OAuthApplication.objects.create( + name="Task comments sandbox", + client_id=client_id, + client_type=OAuthApplication.CLIENT_CONFIDENTIAL, + authorization_grant_type=OAuthApplication.GRANT_AUTHORIZATION_CODE, + redirect_uris="https://example.com/callback", + algorithm="RS256", + organization=self.organization, + user=self.user, + ) + token = OAuthAccessToken.objects.create( + user=self.user, + application=app, + token="pha_task_comments", + scope=scopes, + expires=timezone.now() + timedelta(hours=1), + scoped_teams=[self.team.id], + sandbox_task_id=task_id, + ) + self.client.logout() + self.client.credentials(HTTP_AUTHORIZATION=f"Bearer {token.token}") + return self.client + + def _task_artifact_target(self, *, public: bool = True, creator=None): + task_channel_model = apps.get_model("tasks", "Channel") + task_model = apps.get_model("tasks", "Task") + task_run_model = apps.get_model("tasks", "TaskRun") + channel = None + if public: + channel, _ = task_channel_model.objects.unscoped().get_or_create( + team=self.team, + name="comment-test", + defaults={"created_by": self.user}, + ) + task = task_model.objects.create( + team=self.team, + title="Comment target", + created_by=creator or self.user, + channel=channel, + ) + task_run_model.objects.create( + team=self.team, + task=task, + artifacts=[{"id": "artifact-1", "name": "report.md", "type": "output"}], + ) + return task + + def test_task_artifact_comments_require_a_visible_owning_task(self) -> None: + task = self._task_artifact_target() + payload: dict[str, Any] = { + "content": "Review this", + "scope": "task_artifact", + "item_id": "artifact-1", + "item_context": {"anchor": {"kind": "document"}, "taskId": str(task.id)}, + } + + created = self.client.post(f"/api/projects/{self.team.id}/comments", payload) + assert created.status_code == status.HTTP_201_CREATED + without_task = self.client.get(f"/api/projects/{self.team.id}/comments?scope=task_artifact&item_id=artifact-1") + assert without_task.json()["results"] == [] + unscoped = self.client.get(f"/api/projects/{self.team.id}/comments?item_id=artifact-1") + assert unscoped.json()["results"] == [] + with_task = self.client.get( + f"/api/projects/{self.team.id}/comments?scope=task_artifact&item_id=artifact-1&task_id={task.id}" + ) + assert [row["id"] for row in with_task.json()["results"]] == [created.json()["id"]] + + def test_task_comments_list_artifacts_comments_and_one_comment(self) -> None: + task = self._task_artifact_target() + task_run_model = apps.get_model("tasks", "TaskRun") + task_run_model.objects.create( + team=self.team, + task=task, + artifacts=[{"id": "artifact-1", "name": "latest-report.md", "type": "output"}], + ) + canvas_id = "019fcbe9-839f-7571-ad42-31aa5f615112" + apps.get_model("tasks", "TaskThreadMessage").objects.for_team(self.team.id).create( + team=self.team, + task=task, + event="canvas_created", + content="Canvas created", + payload={ + "canvas_name": "Research canvas", + "canvas_url": f"https://app.posthog.com/code/canvas/channel/{canvas_id}", + }, + ) + root = Comment.objects.create( + team=self.team, + created_by=self.user, + scope="task_artifact", + item_id="artifact-1", + item_context={ + "taskId": str(task.id), + "anchor": {"kind": "text", "quote": "important output", "start": 0, "end": 16}, + "canvasVersionId": "version-2", + }, + content="Please tighten this section", + ) + Comment.objects.create( + team=self.team, + created_by=self.user, + scope="task_artifact", + item_id="artifact-1", + item_context={"taskId": str(task.id), "anchor": {"kind": "document"}}, + source_comment=root, + content="Done", + ) + Comment.objects.create( + team=self.team, + created_by=self.user, + scope="task_artifact", + item_id="artifact-1", + item_context={"taskId": str(task.id), "threadState": "unexpected"}, + source_comment=root, + content="Malformed state is still a reply", + ) + client = self._sandbox_task_comment_client(task.id) + + artifacts = client.get(f"/api/projects/{self.team.id}/tasks/{task.id}/artifacts/") + assert artifacts.status_code == status.HTTP_200_OK + assert artifacts.json() == { + "artifacts": [ + { + "id": "artifact-1", + "type": "artifact", + "name": "latest-report.md", + }, + {"id": canvas_id, "type": "canvas", "name": "Research canvas"}, + ] + } + + comments = client.get( + f"/api/projects/{self.team.id}/tasks/{task.id}/comments/?artifact_id=artifact-1", + ) + assert comments.status_code == status.HTTP_200_OK + assert comments.json()["comments"] == [ + { + "id": str(root.id), + "target": {"id": "artifact-1", "type": "artifact", "name": "latest-report.md"}, + "content": "Please tighten this section", + "content_truncated": False, + "selected_text": "important output", + "created_at": root.created_at.isoformat().replace("+00:00", "Z"), + "reply_count": 2, + "resolved": False, + } + ] + + detail = client.get(f"/api/projects/{self.team.id}/tasks/{task.id}/comments/{root.id}/") + assert detail.status_code == status.HTTP_200_OK + assert [comment["content"] for comment in detail.json()["comments"]] == [ + "Please tighten this section", + "Done", + "Malformed state is still a reply", + ] + assert all(not comment["content_truncated"] for comment in detail.json()["comments"]) + assert all(comment["content_next_offset"] is None for comment in detail.json()["comments"]) + assert detail.json()["comments"][0]["anchor"] == { + "kind": "text", + "quote": "important output", + } + assert detail.json()["comments"][0]["canvas_version_id"] == "version-2" + assert detail.json()["next"] is None + + def test_task_comment_bodies_are_byte_bounded_and_continuable(self) -> None: + task = self._task_artifact_target() + root = Comment.objects.create( + team=self.team, + created_by=self.user, + scope="task_artifact", + item_id="artifact-1", + item_context={"taskId": str(task.id), "anchor": {"kind": "document"}}, + content="é" * 40_000, + ) + client = self._sandbox_task_comment_client(task.id) + + listed = client.get(f"/api/projects/{self.team.id}/tasks/{task.id}/comments/").json()["comments"][0] + assert len(listed["content"].encode("utf-8")) <= 1024 + assert listed["content_truncated"] is True + + detail = client.get(f"/api/projects/{self.team.id}/tasks/{task.id}/comments/{root.id}/").json() + first_chunk = detail["comments"][0] + assert len(first_chunk["content"].encode("utf-8")) <= 64 * 1024 + assert first_chunk["content_truncated"] is True + assert first_chunk["content_next_offset"] is not None + + continuation = client.get( + f"/api/projects/{self.team.id}/tasks/{task.id}/comments/{root.id}/", + {"comment_id": str(root.id), "content_offset": first_chunk["content_next_offset"]}, + ).json()["comments"][0] + assert continuation["content"] + assert continuation["content_truncated"] is False + assert continuation["content_next_offset"] is None + + def test_task_comments_use_an_opaque_cursor(self) -> None: + task = self._task_artifact_target() + for content in ("First", "Second"): + Comment.objects.create( + team=self.team, + created_by=self.user, + scope="task", + item_id=str(task.id), + content=content, + ) + client = self._sandbox_task_comment_client(task.id) + + first = client.get( + f"/api/projects/{self.team.id}/tasks/{task.id}/comments/?limit=1", + ).json() + second = client.get( + f"/api/projects/{self.team.id}/tasks/{task.id}/comments/?limit=1&cursor={first['next']}", + ).json() + + assert [row["content"] for row in first["comments"]] == ["Second"] + assert [row["content"] for row in second["comments"]] == ["First"] + assert second["next"] is None + + def test_task_comments_scan_past_resolved_roots(self) -> None: + task = self._task_artifact_target() + roots = [] + for content in ("Open older", "Resolved newer"): + roots.append( + Comment.objects.create( + team=self.team, + created_by=self.user, + scope="task", + item_id=str(task.id), + content=content, + ) + ) + Comment.objects.create( + team=self.team, + created_by=self.user, + scope="task", + item_id=str(task.id), + source_comment=roots[1], + item_context={"threadState": "resolved"}, + content="resolved", + ) + client = self._sandbox_task_comment_client(task.id) + + response = client.get( + f"/api/projects/{self.team.id}/tasks/{task.id}/comments/?limit=1", + ).json() + + assert [row["content"] for row in response["comments"]] == ["Open older"] + + def test_task_comment_replies_are_paginated(self) -> None: + task = self._task_artifact_target() + root = Comment.objects.create( + team=self.team, + created_by=self.user, + scope="task", + item_id=str(task.id), + content="Root", + ) + Comment.objects.create( + team=self.team, + created_by=self.user, + scope="task", + item_id=str(task.id), + source_comment=root, + content="Reply", + ) + client = self._sandbox_task_comment_client(task.id) + + first = client.get(f"/api/projects/{self.team.id}/tasks/{task.id}/comments/{root.id}/?limit=1").json() + second = client.get( + f"/api/projects/{self.team.id}/tasks/{task.id}/comments/{root.id}/?limit=1&cursor={first['next']}", + ).json() + + assert [row["content"] for row in first["comments"]] == ["Root"] + assert [row["content"] for row in second["comments"]] == ["Reply"] + assert second["next"] is None + + def test_task_comments_cannot_read_another_task_comment(self) -> None: + current_task = self._task_artifact_target() + other_task = self._task_artifact_target() + other_comment = Comment.objects.create( + team=self.team, + created_by=self.user, + scope="task", + item_id=str(other_task.id), + item_context={"anchor": {"kind": "document"}}, + content="Other task comment", + ) + client = self._sandbox_task_comment_client(current_task.id) + + response = client.get( + f"/api/projects/{self.team.id}/tasks/{current_task.id}/comments/{other_comment.id}/", + ) + + assert response.status_code == status.HTTP_404_NOT_FOUND + + def test_task_comments_require_the_sandbox_task_binding(self) -> None: + task = self._task_artifact_target() + + response = self.client.get( + f"/api/projects/{self.team.id}/tasks/{task.id}/artifacts/", + ) + + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_task_comments_ask_legacy_sandboxes_to_restart(self) -> None: + task = self._task_artifact_target() + client = self._sandbox_task_comment_client() + + response = client.get( + f"/api/projects/{self.team.id}/tasks/{task.id}/artifacts/", + ) + + assert response.status_code == status.HTTP_403_FORBIDDEN + assert "Restart the task" in str(response.json()) + + def test_task_comments_reject_an_alternate_task_url_for_the_same_user(self) -> None: + bound_task = self._task_artifact_target() + other_task = self._task_artifact_target() + client = self._sandbox_task_comment_client(bound_task.id) + + response = client.get( + f"/api/projects/{self.team.id}/tasks/{other_task.id}/artifacts/", + ) + + assert response.status_code == status.HTTP_404_NOT_FOUND + + def test_task_comments_reject_a_posthog_ai_sandbox_token(self) -> None: + task = self._task_artifact_target() + client = self._sandbox_task_comment_client(task.id, client_id=POSTHOG_AI_APP_CLIENT_ID_DEV) + + response = client.get( + f"/api/projects/{self.team.id}/tasks/{task.id}/artifacts/", + ) + + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_task_comments_require_comment_read_scope(self) -> None: + task = self._task_artifact_target() + client = self._sandbox_task_comment_client(task.id, scopes="task:read") + + response = client.get(f"/api/projects/{self.team.id}/tasks/{task.id}/comments/") + + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_task_artifact_comments_reject_mismatched_and_private_targets(self) -> None: + other = User.objects.create_and_join(self.organization, "private-task-owner@posthog.com", "password") + task = self._task_artifact_target(public=False, creator=other) + payload: dict[str, Any] = { + "content": "Should not land", + "scope": "task_artifact", + "item_id": "artifact-1", + "item_context": {"anchor": {"kind": "document"}, "taskId": str(task.id)}, + } + assert self.client.post(f"/api/projects/{self.team.id}/comments", payload).status_code == 403 + + visible_task = self._task_artifact_target() + payload["item_context"]["taskId"] = str(visible_task.id) + payload["item_id"] = "not-on-visible-task" + assert self.client.post(f"/api/projects/{self.team.id}/comments", payload).status_code == 403 + + def test_canvas_comments_use_the_relational_canvas_owner(self) -> None: + task = self._task_artifact_target() + channel = task.channel + canvas_model = apps.get_model("canvas", "Canvas") + canvas = canvas_model.objects.unscoped().create( + team=self.team, + channel=channel, + name="Launch canvas", + created_by=self.user, + ) + canvas_version_model = apps.get_model("canvas", "CanvasSourceVersion") + canvas_version_model.objects.unscoped().create( + team=self.team, + canvas=canvas, + source_hash="a" * 64, + source_object_key="canvases/test/source.json", + source_size=2, + task_id=task.id, + created_by=self.user, + ) + mentioned = User.objects.create_and_join(self.organization, "canvas-mentioned@posthog.com", "password") + payload: dict[str, Any] = { + "content": "Review this canvas", + "scope": "desktop_canvas", + "item_id": str(canvas.id), + "item_context": {"anchor": {"kind": "document"}, "taskId": str(task.id)}, + "mentions": [mentioned.id], + } + + created = self.client.post(f"/api/projects/{self.team.id}/comments", payload) + + assert created.status_code == status.HTTP_201_CREATED + with_task = self.client.get( + f"/api/projects/{self.team.id}/comments?scope=desktop_canvas&item_id={canvas.id}&task_id={task.id}" + ) + assert [row["id"] for row in with_task.json()["results"]] == [created.json()["id"]] + task_activity_model = apps.get_model("tasks", "TaskCommentActivity") + assert ( + task_activity_model.objects.unscoped() + .filter( + team=self.team, + user=mentioned, + task=task, + comment_id=created.json()["id"], + ) + .exists() + ) + + other_task = self._task_artifact_target() + payload["item_context"]["taskId"] = str(other_task.id) + assert self.client.post(f"/api/projects/{self.team.id}/comments", payload).status_code == 403 + + def test_comment_without_a_mention_notifies_the_task_owner(self) -> None: + task = self._task_artifact_target() + owner = User.objects.create_and_join(self.organization, "owner@posthog.com", "password") + task.created_by = owner + task.save(update_fields=["created_by"]) + + created = self.client.post( + f"/api/projects/{self.team.id}/comments", + { + "content": "Review when ready", + "scope": "task_artifact", + "item_id": "artifact-1", + "item_context": {"anchor": {"kind": "document"}, "taskId": str(task.id)}, + }, + ) + + assert created.status_code == status.HTTP_201_CREATED + activity_model = apps.get_model("tasks", "TaskCommentActivity") + activity = activity_model.objects.unscoped().get(team=self.team, user=owner, comment_id=created.json()["id"]) + assert activity.kind == "owned_item_comment" + + @mock.patch("products.tasks.backend.tasks.tasks.project_task_comment_activity.delay") + @mock.patch("products.tasks.backend.facade.api.record_comment_activity", side_effect=RuntimeError("activity down")) + def test_activity_projection_failure_schedules_recovery( + self, + _record_activity: mock.Mock, + retry_projection: mock.Mock, + ) -> None: + task = self._task_artifact_target() + + with self.captureOnCommitCallbacks(execute=True): + response = self.client.post( + f"/api/projects/{self.team.id}/comments", + { + "content": "Still persist this", + "scope": "task_artifact", + "item_id": "artifact-1", + "item_context": {"anchor": {"kind": "document"}, "taskId": str(task.id)}, + }, + ) + + assert response.status_code == status.HTTP_201_CREATED + assert Comment.objects.filter(id=response.json()["id"], team=self.team).exists() + retry_projection.assert_called_once_with( + team_id=self.team.id, + comment_id=response.json()["id"], + mentioned_user_ids=[], + include_relationship_recipients=True, + target_owner_id=None, + activity_at=None, + ) + + def test_reply_inherits_its_root_comment_target(self) -> None: + task = self._task_artifact_target() + root = self.client.post( + f"/api/projects/{self.team.id}/comments", + { + "content": "Root", + "scope": "task_artifact", + "item_id": "artifact-1", + "item_context": {"anchor": {"kind": "document"}, "taskId": str(task.id)}, + }, + ).json() + + response = self.client.post( + f"/api/projects/{self.team.id}/comments", + { + "content": "Reply", + "scope": "Insight", + "item_id": "another-resource", + "item_context": {"taskId": "00000000-0000-4000-8000-000000000000", "is_emoji": True}, + "source_comment": root["id"], + }, + ) + + assert response.status_code == status.HTTP_201_CREATED + assert response.json()["source_comment"] == root["id"] + assert response.json()["scope"] == "task_artifact" + assert response.json()["item_id"] == "artifact-1" + assert response.json()["item_context"] == { + "anchor": {"kind": "document"}, + "taskId": str(task.id), + "is_emoji": True, + } + + @parameterized.expand( + [ + ("resolved", True), + ("open", True), + ("unexpected", False), + ] + ) + def test_reply_thread_state_survives_root_context_merge(self, thread_state: str, kept: bool) -> None: + task = self._task_artifact_target() + root = self.client.post( + f"/api/projects/{self.team.id}/comments", + { + "content": "Root", + "scope": "task_artifact", + "item_id": "artifact-1", + "item_context": {"anchor": {"kind": "document"}, "taskId": str(task.id)}, + }, + ).json() + + response = self.client.post( + f"/api/projects/{self.team.id}/comments", + { + "content": "Resolved this thread", + "item_context": {"threadState": thread_state}, + "source_comment": root["id"], + }, + ) + + assert response.status_code == status.HTTP_201_CREATED + item_context = response.json()["item_context"] + if kept: + assert item_context["threadState"] == thread_state + else: + assert "threadState" not in item_context + + @mock.patch("posthog.api.comments.send_mention_notifications") + def test_personal_channel_comments_ignore_mentions(self, send_notifications: mock.Mock) -> None: + task = self._task_artifact_target() + task.channel.channel_type = "personal" + task.channel.save(update_fields=["channel_type"]) + mentioned = User.objects.create_and_join(self.organization, "private-mentioned@posthog.com", "password") + + response = self.client.post( + f"/api/projects/{self.team.id}/comments", + { + "content": "This stays private @[Mentioned](private-mentioned@posthog.com)", + "scope": "task_artifact", + "item_id": "artifact-1", + "item_context": {"anchor": {"kind": "document"}, "taskId": str(task.id)}, + "mentions": [mentioned.id], + }, + ) + + assert response.status_code == status.HTTP_201_CREATED + send_notifications.assert_not_called() + task_activity_model = apps.get_model("tasks", "TaskCommentActivity") + assert not task_activity_model.objects.unscoped().filter(team=self.team, user=mentioned, task=task).exists() + + @mock.patch("posthog.api.comments._record_task_comment_activity") + def test_edit_mentions_do_not_repeat_relationship_notifications(self, record_activity: mock.Mock) -> None: + task = self._task_artifact_target() + mentioned = User.objects.create_and_join(self.organization, "mentioned@posthog.com", "password") + created = self.client.post( + f"/api/projects/{self.team.id}/comments", + { + "content": "Old comment", + "scope": "task_artifact", + "item_id": "artifact-1", + "item_context": {"anchor": {"kind": "document"}, "taskId": str(task.id)}, + }, + ) + assert created.status_code == status.HTTP_201_CREATED + record_activity.reset_mock() + + response = self.client.patch( + f"/api/projects/{self.team.id}/comments/{created.json()['id']}" + f"?scope=task_artifact&item_id=artifact-1&task_id={task.id}", + {"content": "Edited mention", "mentions": [mentioned.id]}, + ) + + assert response.status_code == status.HTTP_200_OK + assert record_activity.call_args.kwargs["include_relationship_recipients"] is False + assert record_activity.call_args.kwargs["activity_at"] is not None + def _create_comment(self, data: dict | None = None) -> Any: if data is None: data = {} diff --git a/posthog/migrations/1291_oauthaccesstoken_sandbox_task_id.py b/posthog/migrations/1291_oauthaccesstoken_sandbox_task_id.py new file mode 100644 index 000000000000..0cbcc7f6bf20 --- /dev/null +++ b/posthog/migrations/1291_oauthaccesstoken_sandbox_task_id.py @@ -0,0 +1,13 @@ +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [("posthog", "1290_backfill_identity_provider_config_identifiers")] + + operations = [ + migrations.AddField( + model_name="oauthaccesstoken", + name="sandbox_task_id", + field=models.UUIDField(blank=True, null=True), + ), + ] diff --git a/posthog/migrations/max_migration.txt b/posthog/migrations/max_migration.txt index f2d211f6cee0..5f9c157d5aed 100644 --- a/posthog/migrations/max_migration.txt +++ b/posthog/migrations/max_migration.txt @@ -1 +1 @@ -1290_backfill_identity_provider_config_identifiers +1291_oauthaccesstoken_sandbox_task_id diff --git a/posthog/models/oauth.py b/posthog/models/oauth.py index 696384830fe9..809b547a0510 100644 --- a/posthog/models/oauth.py +++ b/posthog/models/oauth.py @@ -523,6 +523,8 @@ class Meta(AbstractAccessToken.Meta): scoped_teams: ArrayField = ArrayField(models.IntegerField(), null=True, blank=True) scoped_organizations: ArrayField = ArrayField(models.CharField(max_length=100), null=True, blank=True) + # Server-minted sandbox binding: task-scoped APIs must not trust a caller-supplied task header alone. + sandbox_task_id: models.UUIDField = models.UUIDField(null=True, blank=True) # When set, this token was minted by a staff user impersonating `user`. Used to revoke # tokens at impersonation end. SET_NULL so the customer's tokens survive admin deactivation. diff --git a/posthog/temporal/oauth.py b/posthog/temporal/oauth.py index a3dc8fb7daa6..abbd863c54e8 100644 --- a/posthog/temporal/oauth.py +++ b/posthog/temporal/oauth.py @@ -1,5 +1,6 @@ from datetime import timedelta from typing import Literal +from uuid import UUID from django.conf import settings from django.utils import timezone @@ -41,11 +42,11 @@ # OAuth applications used to mint sandbox agent tokens. The Array applications also # issue interactive Desktop grants, so membership in this set does not prove sandbox origin. +POSTHOG_CODE_OAUTH_APP_CLIENT_IDS = frozenset({ARRAY_APP_CLIENT_ID_US, ARRAY_APP_CLIENT_ID_EU, ARRAY_APP_CLIENT_ID_DEV}) + SANDBOX_OAUTH_APP_CLIENT_IDS = frozenset( { - ARRAY_APP_CLIENT_ID_US, - ARRAY_APP_CLIENT_ID_EU, - ARRAY_APP_CLIENT_ID_DEV, + *POSTHOG_CODE_OAUTH_APP_CLIENT_IDS, POSTHOG_AI_APP_CLIENT_ID_US, POSTHOG_AI_APP_CLIENT_ID_EU, POSTHOG_AI_APP_CLIENT_ID_DEV, @@ -226,7 +227,9 @@ def get_sandbox_oauth_app(application: SandboxOAuthApplication = "array") -> OAu return get_array_app() -def _mint_oauth_access_token(user, team_id: int, *, app: OAuthApplication, scopes: list[str]) -> str: +def _mint_oauth_access_token( + user, team_id: int, *, app: OAuthApplication, scopes: list[str], sandbox_task_id: UUID | None = None +) -> str: token_value = generate_random_oauth_access_token(None) OAuthAccessToken.objects.create( @@ -236,6 +239,7 @@ def _mint_oauth_access_token(user, team_id: int, *, app: OAuthApplication, scope expires=timezone.now() + timedelta(seconds=TOKEN_EXPIRATION_SECONDS), scope=" ".join(dict.fromkeys(scopes)), scoped_teams=[team_id], + sandbox_task_id=sandbox_task_id, ) return token_value @@ -249,6 +253,7 @@ def create_oauth_access_token_for_user( include_internal_scopes: bool = True, include_mcp_builtin_agent_scope: bool = False, application: SandboxOAuthApplication = "array", + sandbox_task_id: UUID | None = None, ) -> str: resolved = resolve_scopes(scopes, include_internal_scopes=include_internal_scopes) if include_mcp_builtin_agent_scope: @@ -257,7 +262,7 @@ def create_oauth_access_token_for_user( # does not narrow the token's other scopes. resolved.append(MCP_BUILT_IN_AGENT_SCOPE) app = get_sandbox_oauth_app(application) - return _mint_oauth_access_token(user, team_id, app=app, scopes=list(resolved)) + return _mint_oauth_access_token(user, team_id, app=app, scopes=list(resolved), sandbox_task_id=sandbox_task_id) def get_wizard_app() -> OAuthApplication: diff --git a/posthog/temporal/tests/test_oauth.py b/posthog/temporal/tests/test_oauth.py index af7b552da7d9..a78da73e554f 100644 --- a/posthog/temporal/tests/test_oauth.py +++ b/posthog/temporal/tests/test_oauth.py @@ -1,3 +1,5 @@ +from uuid import uuid4 + from django.test import SimpleTestCase, TestCase, override_settings from parameterized import parameterized @@ -164,6 +166,18 @@ def test_posthog_ai_application_uses_dev_app(self) -> None: assert access_token.application_id == app.id assert access_token.scoped_teams == [team.id] + @override_settings(CLOUD_DEPLOYMENT="DEV") + def test_task_binding_is_persisted_only_when_supplied(self) -> None: + self._create_oauth_app(ARRAY_APP_CLIENT_ID_DEV, "Array Dev App") + user, team = self._create_user_and_team() + task_id = uuid4() + + bound = create_oauth_access_token_for_user(user, team.id, sandbox_task_id=task_id) + unbound = create_oauth_access_token_for_user(user, team.id) + + assert OAuthAccessToken.objects.get(token=bound).sandbox_task_id == task_id + assert OAuthAccessToken.objects.get(token=unbound).sandbox_task_id is None + @override_settings(CLOUD_DEPLOYMENT="DEV") def test_posthog_ai_application_requires_existing_app(self) -> None: user, team = self._create_user_and_team() diff --git a/products/canvas/backend/comment_access.py b/products/canvas/backend/comment_access.py new file mode 100644 index 000000000000..1a123cad45da --- /dev/null +++ b/products/canvas/backend/comment_access.py @@ -0,0 +1,30 @@ +from uuid import UUID + +from django.core.exceptions import ValidationError +from django.db.models import Q + +from products.canvas.backend.models import Canvas + + +def canvas_belongs_to_task(*, team_id: int, canvas_id: str, task_id: UUID) -> bool: + try: + return ( + Canvas.objects.for_team(team_id) + .filter(id=canvas_id, deleted=False) + .filter(Q(generation_task_id=task_id) | Q(source_versions__task_id=task_id)) + .exists() + ) + except (ValueError, ValidationError): + return False + + +def canvas_owner_id(*, team_id: int, canvas_id: str) -> int | None: + try: + return ( + Canvas.objects.for_team(team_id) + .filter(id=canvas_id, deleted=False) + .values_list("created_by_id", flat=True) + .first() + ) + except (ValueError, ValidationError): + return None diff --git a/products/canvas/backend/presentation/views.py b/products/canvas/backend/presentation/views.py index 7f6a9e564ed5..840bb6903f23 100644 --- a/products/canvas/backend/presentation/views.py +++ b/products/canvas/backend/presentation/views.py @@ -454,6 +454,14 @@ def revert(self, request: Request, *args: Any, **kwargs: Any) -> Response: operation_id="canvases_builds_retrieve", responses={200: CanvasBuildsResponseSerializer}, request=None, + parameters=[ + OpenApiParameter( + name="version_id", + type=OpenApiTypes.UUID, + required=False, + description="Include the retained ready build for this historical source version.", + ) + ], ) @action(methods=["GET"], detail=True) def builds(self, request: Request, *args: Any, **kwargs: Any) -> Response: @@ -471,6 +479,21 @@ def builds(self, request: Request, *args: Any, **kwargs: Any) -> Response: published = canvas.builds.filter(id=canvas.published_build_id).first() if published is not None: builds.append(published) + requested_version_id = request.query_params.get("version_id") + if requested_version_id: + try: + requested_version = canvas.source_versions.filter(id=requested_version_id).first() + except DjangoValidationError: + requested_version = None + if requested_version is None: + return Response({"detail": "Version not found for this canvas."}, status=status.HTTP_404_NOT_FOUND) + historical_build = ( + canvas.builds.filter(source_version_id=requested_version.id, status=CanvasBuild.STATUS_READY) + .order_by("-created_at") + .first() + ) + if historical_build is not None and all(build.id != historical_build.id for build in builds): + builds.append(historical_build) response = { "published_build_id": str(canvas.published_build_id) if canvas.published_build_id else None, "current_version_id": (str(canvas.current_source_version_id) if canvas.current_source_version_id else None), diff --git a/products/canvas/backend/tests/test_canvas_api.py b/products/canvas/backend/tests/test_canvas_api.py index 1d595661b700..c0a4de271f01 100644 --- a/products/canvas/backend/tests/test_canvas_api.py +++ b/products/canvas/backend/tests/test_canvas_api.py @@ -403,6 +403,26 @@ def test_builds_lifecycle_includes_published_build_beyond_window(self): assert body["published_build_id"] == str(published.id) assert str(published.id) in {build["id"] for build in body["builds"]} + def test_builds_lifecycle_includes_requested_historical_build_beyond_window(self): + canvas_id, v1, v2 = self._published_canvas() + historical = CanvasBuild.objects.unscoped().get(canvas_id=canvas_id, source_version_id=v1) + historical.status = CanvasBuild.STATUS_READY + historical.save(update_fields=["status"]) + version = CanvasSourceVersion.objects.unscoped().get(id=v2) + with team_scope(self.team.id): + for _ in range(25): + CanvasBuild.objects.create( + team_id=self.team.id, + canvas_id=canvas_id, + source_version=version, + status=CanvasBuild.STATUS_FAILED, + ) + + response = self.client.get(f"/api/projects/{self.team.id}/canvases/{canvas_id}/builds/?version_id={v1}") + + assert response.status_code == status.HTTP_200_OK + assert str(historical.id) in {build["id"] for build in response.json()["builds"]} + def test_build_with_pruned_artifacts_advertises_no_url(self): canvas_id, v1, _ = self._published_canvas() build = CanvasBuild.objects.unscoped().filter(canvas_id=canvas_id).first() diff --git a/products/canvas/backend/tests/test_cloud_builder.py b/products/canvas/backend/tests/test_cloud_builder.py index 2337bc96dc5f..6673fa547572 100644 --- a/products/canvas/backend/tests/test_cloud_builder.py +++ b/products/canvas/backend/tests/test_cloud_builder.py @@ -95,6 +95,18 @@ def test_runtime_uses_the_document_bound_message_port(self) -> None: self.assertIn('event.data?.type!=="connect"', runtime) self.assertIn("event.ports[0]", runtime) self.assertIn("port?.postMessage", runtime) + self.assertIn('event.data?.type==="set-comment-highlights"', runtime) + self.assertIn('CSS.highlights.set("posthog-canvas-comment"', runtime) + self.assertNotIn("ph-canvas-comment-outline", runtime) + self.assertIn('type:"comment-activate"', runtime) + self.assertIn("event.preventDefault();event.stopPropagation()", runtime) + self.assertIn("if(!items.length||timer)return", runtime) + self.assertNotIn("clearTimeout(timer);timer=setTimeout(()=>render(items),100)", runtime) + self.assertIn('document.addEventListener("selectionchange"', runtime) + self.assertNotIn('document.addEventListener("mouseup"', runtime) + self.assertIn('event.data?.type==="clear-text-selection"', runtime) + self.assertIn("getSelection()?.removeAllRanges()", runtime) + self.assertIn("if(selection&&!selection.isCollapsed)return", runtime) self.assertNotIn("parent.postMessage({channel,...message}", runtime) def test_runtime_bounds_host_side_effects(self) -> None: diff --git a/products/canvas/frontend/generated/api.schemas.ts b/products/canvas/frontend/generated/api.schemas.ts index bf56caca63b6..fc18800506ad 100644 --- a/products/canvas/frontend/generated/api.schemas.ts +++ b/products/canvas/frontend/generated/api.schemas.ts @@ -632,6 +632,13 @@ export type CanvasesListParams = { offset?: number } +export type CanvasesBuildsRetrieveParams = { + /** + * Include the retained ready build for this historical source version. + */ + version_id?: string +} + export type CanvasesSourceRetrieveParams = { /** * Read this historical source version instead of the head (for version browsing). diff --git a/products/canvas/frontend/generated/api.ts b/products/canvas/frontend/generated/api.ts index 07da42d4073b..5ce456ab7c30 100644 --- a/products/canvas/frontend/generated/api.ts +++ b/products/canvas/frontend/generated/api.ts @@ -21,6 +21,7 @@ import type { CanvasSourceResponseApi, CanvasValidateRequestApi, CanvasValidateResponseApi, + CanvasesBuildsRetrieveParams, CanvasesListParams, CanvasesSourceRetrieveParams, CanvasesVersionsRetrieveParams, @@ -137,8 +138,20 @@ export const canvasesDestroy = async (projectId: string, id: string, options?: R }) } -export const getCanvasesBuildsRetrieveUrl = (projectId: string, id: string) => { - return `/api/projects/${projectId}/canvases/${id}/builds/` +export const getCanvasesBuildsRetrieveUrl = (projectId: string, id: string, params?: CanvasesBuildsRetrieveParams) => { + const normalizedParams = new URLSearchParams() + + Object.entries(params || {}).forEach(([key, value]) => { + if (value !== undefined) { + normalizedParams.append(key, value === null ? 'null' : String(value)) + } + }) + + const stringifiedParams = normalizedParams.toString() + + return stringifiedParams.length > 0 + ? `/api/projects/${projectId}/canvases/${id}/builds/?${stringifiedParams}` + : `/api/projects/${projectId}/canvases/${id}/builds/` } /** @@ -151,9 +164,10 @@ export const getCanvasesBuildsRetrieveUrl = (projectId: string, id: string) => { export const canvasesBuildsRetrieve = async ( projectId: string, id: string, + params?: CanvasesBuildsRetrieveParams, options?: RequestInit ): Promise => { - return apiMutator(getCanvasesBuildsRetrieveUrl(projectId, id), { + return apiMutator(getCanvasesBuildsRetrieveUrl(projectId, id, params), { ...options, method: 'GET', }) diff --git a/products/canvas/packages/canvas_builder/build.mjs b/products/canvas/packages/canvas_builder/build.mjs index 04f06d4e630e..0ea602c9f5da 100644 --- a/products/canvas/packages/canvas_builder/build.mjs +++ b/products/canvas/packages/canvas_builder/build.mjs @@ -24,6 +24,8 @@ const forbiddenHtml = /(?:src|href)\s*=\s*["']\s*(javascript|data:text\/html|vbs const extensions = ['', '.ts', '.tsx', '.js', '.jsx', '.css', '.json', '.svg', '.txt'] const runtimePath = 'assets/canvas-runtime.js' const runtime = `(()=>{const channel="posthog-canvas",pending=new Map;let sequence=0,port;const post=(message)=>port?.postMessage({channel,...message});const call=(method,payload)=>new Promise((resolve,reject)=>{const id=String(++sequence);const timer=setTimeout(()=>{pending.delete(id);reject(new Error("Canvas request timed out"));},30000);pending.set(id,{resolve,reject,timer});post({type:"data-request",id,method,payload});});const receive=(event)=>{if(event.data?.channel!==channel||event.data?.type!=="data-response")return;const request=pending.get(event.data.id);if(!request)return;pending.delete(event.data.id);clearTimeout(request.timer);event.data.ok?request.resolve(event.data.result):request.reject(new Error(event.data.error??"Canvas request failed"));};const capture=(event,properties,distinctId)=>{const normalized=properties??{};let serialized;try{serialized=JSON.stringify(normalized)}catch{throw new Error("Canvas capture properties must be serializable")};if(typeof serialized!=="string"||serialized.length>16384)throw new Error("Canvas capture properties are too large");return call("capture",{event,properties:normalized,distinctId})};const openExternal=(value)=>{const url=new URL(value);if(url.protocol!=="https:"||!(url.hostname==="posthog.com"||url.hostname.endsWith(".posthog.com")))throw new Error("Canvas external URL is not allowed");post({type:"open-external",url:url.href})};window.ph={loadInsight:(shortId,options)=>call("loadInsight",{shortId,dateRange:options?.dateRange}),query:(query,params)=>call("query",typeof query==="string"?{hogql:query,params:params??{}}:{query,params:params??{}}),capture,openExternal};addEventListener("message",(event)=>{if(port||event.source!==parent||event.data?.channel!==channel||event.data?.type!=="connect"||!event.ports[0])return;port=event.ports[0];port.addEventListener("message",receive);port.start();if(document.readyState!=="loading")post({type:"ready"});if(document.readyState==="complete")post({type:"rendered"});});addEventListener("error",(event)=>post({type:"error",message:event.message||"Canvas runtime error",stack:event.error?.stack}));addEventListener("unhandledrejection",(event)=>post({type:"error",message:event.reason instanceof Error?event.reason.message:String(event.reason),stack:event.reason instanceof Error?event.reason.stack:undefined}));addEventListener("DOMContentLoaded",()=>post({type:"ready"}));addEventListener("load",()=>post({type:"rendered"}));})();` +const selectionRuntime = `(()=>{const channel="posthog-canvas";let port,timer=0;const post=message=>port?.postMessage({channel,...message}),clear=()=>post({type:"text-selection-cleared"}),clearNative=()=>{getSelection()?.removeAllRanges();clear()},report=()=>{clearTimeout(timer);timer=setTimeout(()=>{const selection=getSelection();if(!selection||selection.isCollapsed||selection.rangeCount===0){clear();return}const range=selection.getRangeAt(0);if(!document.body.contains(range.startContainer)||!document.body.contains(range.endContainer)){clear();return}const before=document.createRange();before.selectNodeContents(document.body);before.setEnd(range.startContainer,range.startOffset);const through=document.createRange();through.selectNodeContents(document.body);through.setEnd(range.endContainer,range.endOffset);const whole=document.createRange();whole.selectNodeContents(document.body);const text=whole.toString(),start=before.toString().length,end=through.toString().length,quote=text.slice(start,end);if(!quote.trim()||quote.length>10000){clear();return}const rect=range.getBoundingClientRect();post({type:"text-selection",selection:{quote,prefix:text.slice(Math.max(0,start-32),start),suffix:text.slice(end,end+32),start,end,rect:{top:rect.top,right:rect.right,bottom:rect.bottom,left:rect.left}}})},80)};addEventListener("message",event=>{if(port||event.source!==parent||event.data?.channel!==channel||event.data?.type!=="connect"||!event.ports[0])return;port=event.ports[0];port.addEventListener("message",event=>{if(event.data?.channel===channel&&event.data?.type==="clear-text-selection")clearNative()});port.start()});document.addEventListener("selectionchange",report)})();` +const highlightRuntime = `(()=>{const channel="posthog-canvas",style=document.createElement("style");style.textContent="::highlight(posthog-canvas-comment){background:rgba(250,204,21,.32);color:inherit}::highlight(posthog-canvas-comment-active){background:rgba(250,204,21,.48);color:inherit}";document.head.appendChild(style);let items=[],ranges=[],port,timer=0;const indexText=()=>{const walker=document.createTreeWalker(document.body,NodeFilter.SHOW_TEXT),entries=[];let text="";for(let node=walker.nextNode();node;node=walker.nextNode()){const start=text.length;text+=node.data;entries.push({node,start,end:text.length})}return{text,entries}},rangeAt=(index,start,end)=>{const find=offset=>{let low=0,high=index.entries.length-1,match=null;while(low<=high){const middle=low+high>>1,entry=index.entries[middle];if(offsetentry.end)low=middle+1;else{match=entry;high=middle-1}}return match},startEntry=find(start),endEntry=find(end);if(!startEntry||!endEntry)return null;const range=document.createRange();range.setStart(startEntry.node,start-startEntry.start);range.setEnd(endEntry.node,end-endEntry.start);return range},resolve=(text,anchor)=>{if(text.slice(anchor.start,anchor.end)===anchor.quote)return{start:anchor.start,end:anchor.end};const matches=[];for(let start=text.indexOf(anchor.quote);start>=0;start=text.indexOf(anchor.quote,start+Math.max(anchor.quote.length,1))){const end=start+anchor.quote.length,prefix=text.slice(Math.max(0,start-anchor.prefix.length),start),suffix=text.slice(end,end+anchor.suffix.length);matches.push({start,end,score:(anchor.prefix&&prefix===anchor.prefix?2:0)+(anchor.suffix&&suffix===anchor.suffix?2:0)})}if(matches.length===1)return matches[0];matches.sort((a,b)=>b.score-a.score);return matches[0]?.score&&matches[0].score!==matches[1]?.score?matches[0]:null},render=next=>{items=next||[];ranges=[];if(!window.Highlight||!window.CSS||!CSS.highlights)return;const normal=new Highlight,active=new Highlight,index=indexText();for(const item of items){const hit=resolve(index.text,item.anchor),range=hit&&rangeAt(index,hit.start,hit.end);if(range){ranges.push({id:item.id,range});(item.active?active:normal).add(range)}}CSS.highlights.set("posthog-canvas-comment",normal);CSS.highlights.set("posthog-canvas-comment-active",active)};addEventListener("message",event=>{if(port||event.source!==parent||event.data?.channel!==channel||event.data?.type!=="connect"||!event.ports[0])return;port=event.ports[0];port.addEventListener("message",event=>{if(event.data?.channel===channel&&event.data?.type==="set-comment-highlights")render(event.data.highlights)});port.start()});document.addEventListener("click",event=>{const selection=getSelection();if(selection&&!selection.isCollapsed)return;for(const item of ranges)for(const rect of item.range.getClientRects())if(event.clientX>=rect.left&&event.clientX<=rect.right&&event.clientY>=rect.top&&event.clientY<=rect.bottom){event.preventDefault();event.stopPropagation();port?.postMessage({channel,type:"comment-activate",id:item.id});return}},true);new MutationObserver(()=>{if(!items.length||timer)return;timer=setTimeout(()=>{timer=0;render(items)},500)}).observe(document.body,{childList:true,characterData:true,subtree:true})})();` const platformStylesheet = ` @import "tailwindcss"; @import "@posthog/quill/tokens.css"; @@ -338,7 +340,7 @@ async function buildCanvas(project) { } const cssPath = `assets/canvas-platform-${sha256(platformCss).slice(0, 10)}.css` files.push(artifact(cssPath, platformCss)) - files.push(artifact(runtimePath, runtime)) + files.push(artifact(runtimePath, `${runtime}\n${selectionRuntime}\n${highlightRuntime}`)) const head = `` html = html.includes('') ? html.replace('', `${head}`) : `${head}${html}` files.unshift(artifact(project.entryHtml, html)) diff --git a/products/desktop/packages/api-client/src/generated.ts b/products/desktop/packages/api-client/src/generated.ts index fc57711b82c0..05490a7e78fb 100644 --- a/products/desktop/packages/api-client/src/generated.ts +++ b/products/desktop/packages/api-client/src/generated.ts @@ -4366,7 +4366,7 @@ export namespace Schemas { export type ColorMode = "light" | "dark"; export type Comment = { id: string; - created_by: UserBasic & unknown; + created_by: (UserBasic & unknown) | null; deleted?: (boolean | null) | undefined; mentions?: Array | undefined; slug?: string | undefined; @@ -4375,9 +4375,10 @@ export namespace Schemas { version: number; created_at: string; item_id?: (string | null) | undefined; - item_context?: null | undefined; + item_context?: unknown; scope: string; source_comment?: (string | null) | undefined; + completed_at?: (string | null) | undefined; }; export type CompareItem = { label: string; value: string }; export type ConclusionEnum = "won" | "lost" | "inconclusive" | "stopped_early" | "invalid"; @@ -18454,7 +18455,7 @@ export namespace Endpoints { path: "/api/projects/{project_id}/comments/"; requestFormat: "json"; parameters: { - query: Partial<{ cursor: string; item_id: string; scope: string; search: string; source_comment: string }>; + query: Partial<{ cursor: string; item_id: string; task_id: string; scope: string; search: string; source_comment: string }>; path: { project_id: string }; }; responses: { 200: Schemas.PaginatedCommentList }; diff --git a/products/desktop/packages/api-client/src/posthog-client.ts b/products/desktop/packages/api-client/src/posthog-client.ts index 2c5df489f998..933680155c5c 100644 --- a/products/desktop/packages/api-client/src/posthog-client.ts +++ b/products/desktop/packages/api-client/src/posthog-client.ts @@ -199,6 +199,29 @@ export interface TaskSessionStorageAccess { content_sha256: string | null; } +/** + * The commentable resources this client knows how to address. `scope` is a + * free-form column on the backend `Comment` model, so adding a resource is a + * new member here plus a caller — no migration and no endpoint. + */ +export type CommentScope = "task_artifact" | "desktop_canvas" | "task"; + +/** Named `Resource*` so it never collides with the DOM's global `Comment`. + * Optimistic rows do not have a server version yet, while item_context is a + * real JSON value despite the generated serializer's historically narrow type. */ +export type ResourceComment = Omit & { + version?: number; +}; + +export interface CreateResourceCommentRequest { + scope: CommentScope; + itemId: string; + content: string; + context: unknown; + sourceCommentId?: string; + mentions?: number[]; +} + /** Thrown when the backend rejects a cloud run with a 429 usage-limit error. */ export class CloudUsageLimitError extends Error { limitType: UsageLimitType; @@ -2717,8 +2740,7 @@ export class PostHogAPIClient { return (await response.json()) as TaskMention[]; } - // Tasks the current user is involved in (created, mentioned, or messaged), - // one row per task, newest activity first. + // Task lifecycle and individual comment activity, newest first. async getTaskActivity(options?: { before?: string; beforeId?: string; @@ -2741,8 +2763,7 @@ export class PostHogAPIClient { return (await response.json()) as TaskActivityPage; } - // Read state is per task, so callers name the tasks the user has seen rather than - // clearing the whole feed. + // Task lifecycle activity clears by task timestamp; comment activity clears by row id. async markTaskActivityRead( activities: TaskActivityReadMarker[], ): Promise { @@ -3236,6 +3257,54 @@ export class PostHogAPIClient { return data.url; } + async getResourceComments( + scope: CommentScope, + itemId: string, + taskId: string, + ): Promise { + const MAX_COMMENT_PAGES = 50; + const teamId = await this.getTeamId(); + const comments: ResourceComment[] = []; + let cursor: string | undefined; + for (let pageIndex = 0; pageIndex < MAX_COMMENT_PAGES; pageIndex++) { + const page = await this.api.get("/api/projects/{project_id}/comments/", { + path: { project_id: String(teamId) }, + query: { scope, item_id: itemId, task_id: taskId, cursor }, + }); + comments.push(...page.results); + cursor = page.next + ? (new URL(page.next).searchParams.get("cursor") ?? undefined) + : undefined; + if (!cursor) return comments; + } + log.warn( + `getResourceComments hit MAX_PAGES (${MAX_COMMENT_PAGES}); returning partial results`, + { scope, itemId, returned: comments.length }, + ); + return comments; + } + + async createResourceComment( + request: CreateResourceCommentRequest, + ): Promise { + const teamId = await this.getTeamId(); + const payload = { + content: request.content, + scope: request.scope, + item_id: request.itemId, + item_context: request.context, + source_comment: request.sourceCommentId ?? null, + mentions: request.mentions ?? [], + // Resolution is represented by a thread-state reply so this stays on the + // same PAT-compatible write path as ordinary comments. + is_task: false, + }; + return await this.api.post("/api/projects/{project_id}/comments/", { + path: { project_id: String(teamId) }, + body: payload as unknown as Schemas.Comment, + }); + } + async getTaskSessionStorageAccess( taskId: string, runId: string, diff --git a/products/desktop/packages/core/src/canvas/canvasBuildSchemas.test.ts b/products/desktop/packages/core/src/canvas/canvasBuildSchemas.test.ts index 12e20ef04cd7..5876655596e2 100644 --- a/products/desktop/packages/core/src/canvas/canvasBuildSchemas.test.ts +++ b/products/desktop/packages/core/src/canvas/canvasBuildSchemas.test.ts @@ -4,6 +4,7 @@ import { type CanvasBuildRecord, currentHeadBuildFailure, hasActiveCanvasBuild, + historicalCanvasBuild, latestFinishedCanvasBuild, publishedCanvasBuild, } from "./canvasBuildSchemas"; @@ -74,6 +75,18 @@ describe("canvas build lifecycle", () => { expect(publishedCanvasBuild(value)).toBe(ready); }); + it("selects a retained ready artifact for a historical version", () => { + const ready = build("historical", "ready"); + ready.sourceVersionId = "version-1"; + + expect( + historicalCanvasBuild( + lifecycle([build("latest", "ready"), ready]), + "version-1", + ), + ).toBe(ready); + }); + it("surfaces a failed build of the current head even when an older published build is also ready", () => { // b_old (sv-old) is the pinned/published live build; a newer publish (b_new, // sv-new = current head) failed. latestFinishedCanvasBuild returns the first @@ -136,7 +149,7 @@ describe("canvas build lifecycle", () => { }, } as unknown as ProjectApiClient); - const result = await service.getBuilds("canvas-1"); + const result = await service.getBuilds({ id: "canvas-1" }); expect(result.publishedBuildId).toBe("b0"); expect(result.builds[0]).toMatchObject({ diff --git a/products/desktop/packages/core/src/canvas/canvasBuildSchemas.ts b/products/desktop/packages/core/src/canvas/canvasBuildSchemas.ts index fe779d1a9221..9ec9e9cdfa5a 100644 --- a/products/desktop/packages/core/src/canvas/canvasBuildSchemas.ts +++ b/products/desktop/packages/core/src/canvas/canvasBuildSchemas.ts @@ -42,6 +42,20 @@ export function publishedCanvasBuild( ); } +export function historicalCanvasBuild( + lifecycle: CanvasBuildLifecycle, + versionId: string, +): CanvasBuildRecord | null { + return ( + lifecycle.builds.find( + (build) => + build.sourceVersionId === versionId && + build.buildStatus === "ready" && + !!build.artifactUrl, + ) ?? null + ); +} + export const canvasBuildLifecycleSchema = z.object({ /** The live (last successful, still-eligible) build, null until one completes. */ publishedBuildId: z.string().nullable(), diff --git a/products/desktop/packages/core/src/canvas/dashboardSchemas.ts b/products/desktop/packages/core/src/canvas/dashboardSchemas.ts index 2d5fb978f1cb..094291d39d59 100644 --- a/products/desktop/packages/core/src/canvas/dashboardSchemas.ts +++ b/products/desktop/packages/core/src/canvas/dashboardSchemas.ts @@ -73,6 +73,11 @@ export const createDashboardInput = z.object({ export const dashboardIdInput = z.object({ id: z.string().min(1) }); +export const canvasBuildsInput = z.object({ + id: z.string().min(1), + versionId: z.string().min(1).optional(), +}); + export const canvasSourceInput = z.object({ id: z.string().min(1), // Read a historical version's files instead of the head. diff --git a/products/desktop/packages/core/src/canvas/dashboardsService.test.ts b/products/desktop/packages/core/src/canvas/dashboardsService.test.ts index 9d13b489af6b..fac1b52b0d56 100644 --- a/products/desktop/packages/core/src/canvas/dashboardsService.test.ts +++ b/products/desktop/packages/core/src/canvas/dashboardsService.test.ts @@ -77,7 +77,7 @@ describe("DashboardsService.list", () => { describe("DashboardsService.getBuilds", () => { it("normalizes the lifecycle payload", async () => { const { api } = fakeApi({ - "canvases/c1/builds/": { + "canvases/c1/builds/?version_id=v1": { published_build_id: "b1", current_version_id: "v1", builds: [ @@ -97,7 +97,7 @@ describe("DashboardsService.getBuilds", () => { }); const service = new DashboardsService(api); - const lifecycle = await service.getBuilds("c1"); + const lifecycle = await service.getBuilds({ id: "c1", versionId: "v1" }); expect(lifecycle.publishedBuildId).toBe("b1"); expect(lifecycle.currentVersionId).toBe("v1"); diff --git a/products/desktop/packages/core/src/canvas/dashboardsService.ts b/products/desktop/packages/core/src/canvas/dashboardsService.ts index 2a6e186e75db..d21830ba9502 100644 --- a/products/desktop/packages/core/src/canvas/dashboardsService.ts +++ b/products/desktop/packages/core/src/canvas/dashboardsService.ts @@ -258,12 +258,21 @@ export class DashboardsService { // Read a canvas's build lifecycle (pointers + recent builds). Publishing // queues a build server-side; callers poll this until it settles. - async getBuilds(id: string): Promise { + async getBuilds(input: { + id: string; + versionId?: string; + }): Promise { + const suffix = input.versionId + ? `?version_id=${encodeURIComponent(input.versionId)}` + : ""; const body = await this.api.json<{ published_build_id: string | null; current_version_id: string | null; builds: Record[]; - }>(`canvases/${encodeURIComponent(id)}/builds/`, "load canvas builds"); + }>( + `canvases/${encodeURIComponent(input.id)}/builds/${suffix}`, + "load canvas builds", + ); // Each build row is already validated by tryToBuildRecord; this endpoint is // polled every couple of seconds during builds, so don't re-run the whole // lifecycle schema (which would zod-parse every record a second time). diff --git a/products/desktop/packages/core/src/canvas/freeformSchemas.test.ts b/products/desktop/packages/core/src/canvas/freeformSchemas.test.ts index 7f1a8381e79c..154b32c32a5f 100644 --- a/products/desktop/packages/core/src/canvas/freeformSchemas.test.ts +++ b/products/desktop/packages/core/src/canvas/freeformSchemas.test.ts @@ -1,7 +1,10 @@ import { describe, expect, it } from "vitest"; -import { canvasToHostMessageSchema } from "./freeformSchemas"; +import { + canvasToHostMessageSchema, + hostToCanvasMessageSchema, +} from "./freeformSchemas"; -describe("canvasToHostMessageSchema open-external", () => { +describe("canvasToHostMessageSchema", () => { const message = (url: string) => ({ channel: "posthog-canvas", type: "open-external", @@ -32,4 +35,80 @@ describe("canvasToHostMessageSchema open-external", () => { false, ); }); + + it("accepts a bounded text selection and rejects oversized selected text", () => { + const selection = { + channel: "posthog-canvas", + type: "text-selection", + selection: { + quote: "selected text", + prefix: "before ", + suffix: " after", + start: 7, + end: 20, + rect: { top: 10, right: 80, bottom: 30, left: 20 }, + }, + }; + + expect(canvasToHostMessageSchema.safeParse(selection).success).toBe(true); + expect( + canvasToHostMessageSchema.safeParse({ + ...selection, + selection: { ...selection.selection, quote: "x".repeat(10_001) }, + }).success, + ).toBe(false); + }); + + it("accepts an explicit selection-cleared event", () => { + expect( + canvasToHostMessageSchema.safeParse({ + channel: "posthog-canvas", + type: "text-selection-cleared", + }).success, + ).toBe(true); + }); + + it("accepts a bounded comment activation", () => { + expect( + canvasToHostMessageSchema.safeParse({ + channel: "posthog-canvas", + type: "comment-activate", + id: "comment-1", + }).success, + ).toBe(true); + }); + + it("accepts bounded comment highlights", () => { + expect( + hostToCanvasMessageSchema.safeParse({ + channel: "posthog-canvas", + type: "set-comment-highlights", + highlights: [ + { + id: "comment-1", + active: false, + anchor: { + quote: "selected text", + prefix: "before ", + suffix: " after", + start: 7, + end: 20, + }, + }, + ], + }).success, + ).toBe(true); + }); + + it("accepts a host request to clear native text selection", () => { + expect( + hostToCanvasMessageSchema.parse({ + channel: "posthog-canvas", + type: "clear-text-selection", + }), + ).toEqual({ + channel: "posthog-canvas", + type: "clear-text-selection", + }); + }); }); diff --git a/products/desktop/packages/core/src/canvas/freeformSchemas.ts b/products/desktop/packages/core/src/canvas/freeformSchemas.ts index b5ee108a51a8..8d73e8ce3cd3 100644 --- a/products/desktop/packages/core/src/canvas/freeformSchemas.ts +++ b/products/desktop/packages/core/src/canvas/freeformSchemas.ts @@ -1,5 +1,6 @@ import { isSafePostHogUrl } from "@posthog/shared"; import { z } from "zod"; +import { textCommentAnchorDataSchema } from "../comments/anchors"; // The template id for freeform-React canvases. Stored on a canvas's meta so the // generation path can resolve the right system prompt. @@ -126,6 +127,28 @@ export type CanvasAnalyticsConfig = z.infer; export const canvasThemeSchema = z.enum(["light", "dark"]); export type CanvasTheme = z.infer; +const canvasTextSelectionDataSchema = textCommentAnchorDataSchema.extend({ + rect: z.object({ + top: z.number().finite(), + right: z.number().finite(), + bottom: z.number().finite(), + left: z.number().finite(), + }), +}); +export const canvasTextSelectionSchema = canvasTextSelectionDataSchema.refine( + ({ start, end }) => end > start, +); +export type CanvasTextSelection = z.infer; + +export const canvasCommentHighlightSchema = z.object({ + id: z.string().min(1).max(128), + active: z.boolean(), + anchor: textCommentAnchorDataSchema.refine(({ start, end }) => end > start), +}); +export type CanvasCommentHighlight = z.infer< + typeof canvasCommentHighlightSchema +>; + // host -> iframe export const hostToCanvasMessageSchema = z.discriminatedUnion("type", [ // First frame: hand the iframe its source + the run mode. The iframe does not @@ -143,6 +166,7 @@ export const hostToCanvasMessageSchema = z.discriminatedUnion("type", [ // already correct; live theme changes use the `set-theme` frame below // (which re-themes without remounting). Absent = light. theme: canvasThemeSchema.optional(), + highlights: z.array(canvasCommentHighlightSchema).max(500).optional(), }), // Live theme change: re-apply light/dark WITHOUT remounting the app. Sent on // its own (not folded into `init`) so toggling the host theme — or an OS @@ -152,6 +176,15 @@ export const hostToCanvasMessageSchema = z.discriminatedUnion("type", [ type: z.literal("set-theme"), theme: canvasThemeSchema, }), + z.object({ + channel: z.literal(CANVAS_CHANNEL), + type: z.literal("set-comment-highlights"), + highlights: z.array(canvasCommentHighlightSchema).max(500), + }), + z.object({ + channel: z.literal(CANVAS_CHANNEL), + type: z.literal("clear-text-selection"), + }), // Reply to a data-request, correlated by `id`. z.object({ channel: z.literal(CANVAS_CHANNEL), @@ -223,5 +256,19 @@ export const canvasToHostMessageSchema = z.discriminatedUnion("type", [ type: z.literal("open-external"), url: z.string().refine(isSafePostHogUrl), }), + z.object({ + channel: z.literal(CANVAS_CHANNEL), + type: z.literal("text-selection"), + selection: canvasTextSelectionSchema, + }), + z.object({ + channel: z.literal(CANVAS_CHANNEL), + type: z.literal("text-selection-cleared"), + }), + z.object({ + channel: z.literal(CANVAS_CHANNEL), + type: z.literal("comment-activate"), + id: z.string().min(1).max(128), + }), ]); export type CanvasToHostMessage = z.infer; diff --git a/products/desktop/packages/core/src/canvas/services.ts b/products/desktop/packages/core/src/canvas/services.ts index 640344b6253e..061c2bd6cdcb 100644 --- a/products/desktop/packages/core/src/canvas/services.ts +++ b/products/desktop/packages/core/src/canvas/services.ts @@ -57,8 +57,11 @@ export interface IDashboardsService { versionId: string; expectedCurrentVersionId: string | null; }): Promise; - // Read a canvas's build lifecycle (pointers + recent builds). - getBuilds(id: string): Promise; + // Read a canvas's build lifecycle, optionally including a historical build. + getBuilds(input: { + id: string; + versionId?: string; + }): Promise; actOnBuild(input: CanvasBuildActionInput): Promise; rename(input: { id: string; name: string }): Promise; delete(id: string): Promise; diff --git a/products/desktop/packages/core/src/canvas/taskActivity.test.ts b/products/desktop/packages/core/src/canvas/taskActivity.test.ts index 09a00c8d3ca0..ebd8f1aa8d4d 100644 --- a/products/desktop/packages/core/src/canvas/taskActivity.test.ts +++ b/products/desktop/packages/core/src/canvas/taskActivity.test.ts @@ -40,11 +40,30 @@ describe("toTaskActivityItems", () => { snippet: "ping @[Me](me@posthog.com)", author: ann, messageId: "m1", + commentId: null, + commentTarget: null, isUnread: true, }, ]); }); + it("maps a comment activity target for exact navigation", () => { + const [item] = toTaskActivityItems([ + activity({ + latest_message_id: null, + latest_comment_id: "comment-1", + latest_comment_scope: "task_artifact", + latest_comment_item_id: "artifact-1", + }), + ]); + + expect(item.commentId).toBe("comment-1"); + expect(item.commentTarget).toEqual({ + scope: "task_artifact", + itemId: "artifact-1", + }); + }); + it("labels untitled tasks and tolerates missing optional values", () => { const [item] = toTaskActivityItems([ activity({ diff --git a/products/desktop/packages/core/src/canvas/taskActivity.ts b/products/desktop/packages/core/src/canvas/taskActivity.ts index 0d4b9ab422e1..d251ef43708d 100644 --- a/products/desktop/packages/core/src/canvas/taskActivity.ts +++ b/products/desktop/packages/core/src/canvas/taskActivity.ts @@ -3,12 +3,13 @@ import type { TaskActivityKind, UserBasic, } from "@posthog/shared/domain-types"; +import type { CommentTarget } from "../comments/anchors"; /** * The Activity feed — tasks the current user is involved in (created, mentioned * in, or messaged in) — as served by the backend task-activity index - * (`getTaskActivity`). One row per task, newest activity first; the client only - * maps DTOs to items. + * (`getTaskActivity`). Task state collapses per task, while comment notifications + * are individual entries; the client only maps DTOs to items. */ export interface TaskActivityItem { @@ -25,6 +26,8 @@ export interface TaskActivityItem { snippet: string; author: UserBasic | null; messageId: string | null; + commentId?: string | null; + commentTarget?: CommentTarget | null; isUnread: boolean; } @@ -43,6 +46,14 @@ export function toTaskActivityItems( snippet: row.snippet, author: row.latest_author ?? null, messageId: row.latest_message_id ?? null, + commentId: row.latest_comment_id ?? null, + commentTarget: + row.latest_comment_scope && row.latest_comment_item_id + ? { + scope: row.latest_comment_scope as CommentTarget["scope"], + itemId: row.latest_comment_item_id, + } + : null, isUnread: row.is_unread, })); } diff --git a/products/desktop/packages/core/src/comments/anchors.test.ts b/products/desktop/packages/core/src/comments/anchors.test.ts new file mode 100644 index 000000000000..2da4660705ec --- /dev/null +++ b/products/desktop/packages/core/src/comments/anchors.test.ts @@ -0,0 +1,123 @@ +import { describe, expect, it } from "vitest"; +import { + createTextCommentAnchor, + isThreadResolved, + parseCommentContext, + resolveTextCommentAnchor, +} from "./anchors"; + +describe("artifact text anchors", () => { + it("creates and resolves a verified positional anchor", () => { + const text = "Before selected words after"; + const anchor = createTextCommentAnchor(text, 7, 21); + + if (!anchor) throw new Error("Expected an anchor"); + expect(resolveTextCommentAnchor(text, anchor)).toEqual({ + start: 7, + end: 21, + status: "exact", + }); + }); + + it("reanchors a quote after surrounding content changes", () => { + const original = "Before selected words after"; + const anchor = createTextCommentAnchor(original, 7, 21); + if (!anchor) throw new Error("Expected an anchor"); + const changed = `New introduction. ${original}`; + + expect(resolveTextCommentAnchor(changed, anchor)).toEqual({ + start: 25, + end: 39, + status: "reanchored", + }); + }); + + it("uses context to disambiguate repeated quotes", () => { + const original = "first repeated phrase then second repeated phrase end"; + const start = original.lastIndexOf("repeated phrase"); + const anchor = createTextCommentAnchor( + original, + start, + start + "repeated phrase".length, + ); + if (!anchor) throw new Error("Expected an anchor"); + const changed = `prefix ${original}`; + + expect(resolveTextCommentAnchor(changed, anchor)?.start).toBe( + changed.lastIndexOf("repeated phrase"), + ); + }); + + it("orphans deleted and ambiguous text instead of guessing", () => { + const deleted = createTextCommentAnchor("unique text", 0, 6); + if (!deleted) throw new Error("Expected an anchor"); + expect(resolveTextCommentAnchor("replacement", deleted)).toBeNull(); + + const ambiguous = { + kind: "text" as const, + quote: "same", + prefix: "", + suffix: "", + start: 100, + end: 104, + }; + expect(resolveTextCommentAnchor("same x same", ambiguous)).toBeNull(); + }); + + it("rejects whitespace-only selections", () => { + expect(createTextCommentAnchor("a b", 1, 4)).toBeNull(); + }); + + it("rejects selections larger than the persisted anchor contract", () => { + const text = "x".repeat(10_001); + expect(createTextCommentAnchor(text, 0, text.length)).toBeNull(); + }); + + it("validates versioned comment context and anchor bounds", () => { + expect( + parseCommentContext({ + anchor: { kind: "document" }, + canvasVersionId: "version-2", + }), + ).toEqual({ + anchor: { kind: "document" }, + canvasVersionId: "version-2", + }); + expect( + parseCommentContext({ + anchor: { + kind: "text", + quote: "x".repeat(10_001), + prefix: "", + suffix: "", + start: 0, + end: 10_001, + }, + }), + ).toBeNull(); + }); + + it("uses the latest thread-state event for resolution", () => { + const root = { completed_at: null }; + const event = (state: "resolved" | "open", created_at: string) => ({ + created_at, + item_context: { + anchor: { kind: "document" as const }, + threadState: state, + }, + }); + + expect( + isThreadResolved(root, [ + event("resolved", "2026-01-01T00:00:00Z"), + event("open", "2026-01-01T00:01:00Z"), + ]), + ).toBe(false); + expect( + isThreadResolved(root, [ + event("open", "2026-01-01T00:00:00Z"), + event("resolved", "2026-01-01T00:01:00Z"), + ]), + ).toBe(true); + }); +}); diff --git a/products/desktop/packages/core/src/comments/anchors.ts b/products/desktop/packages/core/src/comments/anchors.ts new file mode 100644 index 000000000000..ff3458986a00 --- /dev/null +++ b/products/desktop/packages/core/src/comments/anchors.ts @@ -0,0 +1,189 @@ +import type { CommentScope } from "@posthog/api-client/posthog-client"; +import { z } from "zod"; + +const CONTEXT_LENGTH = 32; +const MAX_QUOTE_LENGTH = 10_000; + +/** + * Addresses one commentable thing. `itemId` must be the resource's STABLE id + * (an artifact id, a canvas row id) — never a name or a version, so comments + * survive renames and reverts. + */ +export type CommentTarget = { + scope: CommentScope; + itemId: string; +}; + +/** The target as one string, for map keys and cache-key membership tests. */ +export function commentTargetKey(target: CommentTarget): string { + return `${target.scope}:${target.itemId}`; +} + +export function isSameCommentTarget( + a: CommentTarget | null, + b: CommentTarget | null, +): boolean { + return a?.scope === b?.scope && a?.itemId === b?.itemId; +} + +export const textCommentAnchorDataSchema = z.object({ + quote: z.string().min(1).max(MAX_QUOTE_LENGTH), + prefix: z.string().max(CONTEXT_LENGTH), + suffix: z.string().max(CONTEXT_LENGTH), + start: z.number().int().nonnegative(), + end: z.number().int().positive(), +}); + +export const textCommentAnchorSchema = textCommentAnchorDataSchema + .extend({ + kind: z.literal("text"), + }) + .refine(({ start, end }) => end > start, { + message: "Text anchor end must follow its start", + }); + +export const regionCommentAnchorSchema = z.object({ + kind: z.literal("region"), + x: z.number().min(0).max(1), + y: z.number().min(0).max(1), + width: z.number().min(0).max(1), + height: z.number().min(0).max(1), +}); + +const documentCommentAnchorSchema = z.object({ + kind: z.literal("document"), +}); + +export const commentAnchorSchema = z.discriminatedUnion("kind", [ + textCommentAnchorSchema, + regionCommentAnchorSchema, + documentCommentAnchorSchema, +]); + +export type TextCommentAnchor = z.infer; +export type RegionCommentAnchor = z.infer; +export type CommentAnchor = z.infer; + +export const commentContextSchema = z.object({ + anchor: commentAnchorSchema, + threadState: z.enum(["resolved", "open"]).optional(), + /** Immutable canvas source version rendered when the comment was made. */ + canvasVersionId: z.string().min(1).optional(), + // The task the commented resource belongs to. Artifact and canvas ids live in a run's + // JSON rather than a table, so the server can't get back to the task without being told. + taskId: z.string().optional(), +}); + +export type CommentContext = z.infer; + +export function parseCommentContext(value: unknown): CommentContext | null { + const parsed = commentContextSchema.safeParse(value); + return parsed.success ? parsed.data : null; +} + +export type ThreadStateComment = { + created_at: string; + item_context?: unknown; +}; + +export function isThreadResolved( + root: { completed_at?: string | null }, + replies: ThreadStateComment[], +): boolean { + const latestState = replies + .map((comment) => ({ + createdAt: comment.created_at, + state: parseCommentContext(comment.item_context)?.threadState, + })) + .filter( + ( + entry, + ): entry is { + createdAt: string; + state: "resolved" | "open"; + } => !!entry.state, + ) + .sort((a, b) => a.createdAt.localeCompare(b.createdAt)) + .at(-1)?.state; + return latestState ? latestState === "resolved" : !!root.completed_at; +} + +export type ResolvedTextAnchor = { + start: number; + end: number; + status: "exact" | "reanchored"; +}; + +export function createTextCommentAnchor( + text: string, + start: number, + end: number, +): TextCommentAnchor | null { + const safeStart = Math.max(0, Math.min(start, text.length)); + const safeEnd = Math.max(safeStart, Math.min(end, text.length)); + const quote = text.slice(safeStart, safeEnd); + if (!quote.trim() || quote.length > MAX_QUOTE_LENGTH) return null; + + return { + kind: "text", + quote, + prefix: text.slice(Math.max(0, safeStart - CONTEXT_LENGTH), safeStart), + suffix: text.slice(safeEnd, safeEnd + CONTEXT_LENGTH), + start: safeStart, + end: safeEnd, + }; +} + +/** + * Resolve a persisted text quote without ever guessing. The stored position is + * verified first. If content moved, prefix/suffix disambiguate quote matches; + * ties are deliberately treated as orphaned. + */ +export function resolveTextCommentAnchor( + text: string, + anchor: TextCommentAnchor, +): ResolvedTextAnchor | null { + if (text.slice(anchor.start, anchor.end) === anchor.quote) { + return { start: anchor.start, end: anchor.end, status: "exact" }; + } + + const candidates: number[] = []; + let from = 0; + while (from <= text.length - anchor.quote.length) { + const match = text.indexOf(anchor.quote, from); + if (match < 0) break; + candidates.push(match); + from = match + Math.max(anchor.quote.length, 1); + } + if (candidates.length === 0) return null; + if (candidates.length === 1) { + const start = candidates[0]; + return { + start, + end: start + anchor.quote.length, + status: "reanchored", + }; + } + + const ranked = candidates + .map((start) => { + const end = start + anchor.quote.length; + const prefix = text.slice( + Math.max(0, start - anchor.prefix.length), + start, + ); + const suffix = text.slice(end, end + anchor.suffix.length); + let score = 0; + if (anchor.prefix && prefix === anchor.prefix) score += 2; + if (anchor.suffix && suffix === anchor.suffix) score += 2; + return { start, score }; + }) + .sort((a, b) => b.score - a.score); + if (ranked[0].score === 0 || ranked[0].score === ranked[1].score) return null; + + return { + start: ranked[0].start, + end: ranked[0].start + anchor.quote.length, + status: "reanchored", + }; +} diff --git a/products/desktop/packages/core/src/panels/panelStoreHelpers.ts b/products/desktop/packages/core/src/panels/panelStoreHelpers.ts index 2b41453240bf..aaff4ad4b2ad 100644 --- a/products/desktop/packages/core/src/panels/panelStoreHelpers.ts +++ b/products/desktop/packages/core/src/panels/panelStoreHelpers.ts @@ -62,6 +62,35 @@ export function getLeafPanel( return panel?.type === "leaf" ? panel : null; } +/** + * The artifact the user is looking at, if any: the focused panel's active tab + * when that is an artifact, else any other panel's. Lets a pane elsewhere (the + * task's comment list) narrow itself to whatever is on screen. + */ +export function activeArtifactId(layout: TaskLayout): string | null { + const activeArtifact = (node: PanelNode): string | null => { + if (node.type !== "leaf") { + for (const child of node.children) { + const found = activeArtifact(child); + if (found) return found; + } + return null; + } + const active = node.content.tabs.find( + (tab) => tab.id === node.content.activeTabId, + ); + return active?.data.type === "artifact" ? active.data.artifactId : null; + }; + + const focused = layout.focusedPanelId + ? getLeafPanel(layout.panelTree, layout.focusedPanelId) + : null; + return ( + (focused ? activeArtifact(focused) : null) ?? + activeArtifact(layout.panelTree) + ); +} + export function getGroupPanel( tree: PanelNode, panelId: string, diff --git a/products/desktop/packages/core/src/sessions/sessionService.ts b/products/desktop/packages/core/src/sessions/sessionService.ts index cf59a5d3196e..2e39fe537410 100644 --- a/products/desktop/packages/core/src/sessions/sessionService.ts +++ b/products/desktop/packages/core/src/sessions/sessionService.ts @@ -9,6 +9,10 @@ import type { SessionConfigSelectOption, SessionUpdate, } from "@agentclientprotocol/sdk"; +import type { + CreateResourceCommentRequest, + ResourceComment, +} from "@posthog/api-client/posthog-client"; import { type AcpMessage, type Adapter, @@ -49,6 +53,7 @@ import { isTerminalStatus, type Task, } from "@posthog/shared/domain-types"; +import type { CommentTarget } from "../comments/anchors"; import type { SpeechKind, SpeechSource } from "../speech/identifiers"; import { CONTEXT_WINDOW_OPTION_CATEGORY, @@ -7495,6 +7500,65 @@ export class SessionService { } } + async getResourceComments( + target: CommentTarget, + taskId: string, + ): Promise { + const authStatus = await this.getAuthCredentialsStatus(); + if (authStatus.kind !== "ready") return []; + return authStatus.auth.client.getResourceComments( + target.scope, + target.itemId, + taskId, + ); + } + + /** + * Comments for several resources at once, for surfaces that centralize threads + * across a task's artifacts and canvases. Returns one flat list — every row + * already carries `scope` and `item_id`, so callers group without bookkeeping. + * Fanning out here (rather than in a hook) keeps the multi-source read in a + * service and lets the caller hold a single query. + */ + async getResourceCommentsForTargets( + targets: CommentTarget[], + taskId: string, + ): Promise { + const authStatus = await this.getAuthCredentialsStatus(); + if (authStatus.kind !== "ready" || targets.length === 0) return []; + const client = authStatus.auth.client; + const pages: ResourceComment[][] = Array.from( + { length: targets.length }, + () => [], + ); + let nextIndex = 0; + const worker = async () => { + while (nextIndex < targets.length) { + const index = nextIndex++; + const target = targets[index]; + pages[index] = await client.getResourceComments( + target.scope, + target.itemId, + taskId, + ); + } + }; + await Promise.all( + Array.from({ length: Math.min(4, targets.length) }, worker), + ); + return pages.flat(); + } + + async createResourceComment( + request: CreateResourceCommentRequest, + ): Promise { + const authStatus = await this.getAuthCredentialsStatus(); + if (authStatus.kind !== "ready") { + throw new Error("Sign in to comment"); + } + return authStatus.auth.client.createResourceComment(request); + } + async getCloudRunArtifacts( taskId: string, runId: string, diff --git a/products/desktop/packages/host-router/src/routers/dashboards.router.ts b/products/desktop/packages/host-router/src/routers/dashboards.router.ts index 7603b889ea0d..723d084a9375 100644 --- a/products/desktop/packages/host-router/src/routers/dashboards.router.ts +++ b/products/desktop/packages/host-router/src/routers/dashboards.router.ts @@ -4,6 +4,7 @@ import { canvasBuildRecordSchema, } from "@posthog/core/canvas/canvasBuildSchemas"; import { + canvasBuildsInput, canvasSourceInput, canvasSourceSchema, canvasVersionSchema, @@ -62,12 +63,12 @@ export const dashboardsRouter = router({ .revertToVersion(input), ), builds: publicProcedure - .input(dashboardIdInput) + .input(canvasBuildsInput) .output(canvasBuildLifecycleSchema) .query(({ ctx, input }) => ctx.container .get(DASHBOARDS_SERVICE) - .getBuilds(input.id), + .getBuilds(input), ), actOnBuild: publicProcedure .input(canvasBuildActionInputSchema) diff --git a/products/desktop/packages/shared/src/domain-types.ts b/products/desktop/packages/shared/src/domain-types.ts index 648357416236..8e5d8c19269e 100644 --- a/products/desktop/packages/shared/src/domain-types.ts +++ b/products/desktop/packages/shared/src/domain-types.ts @@ -172,6 +172,8 @@ export type TaskActivityKind = | "completed" | "message" | "mention" + | "thread_reply" + | "owned_item_comment" | "created"; /** @@ -190,6 +192,9 @@ export interface TaskActivity { snippet: string; latest_author?: UserBasic | null; latest_message_id?: string | null; + latest_comment_id?: string | null; + latest_comment_scope?: string | null; + latest_comment_item_id?: string | null; is_unread: boolean; } @@ -204,6 +209,7 @@ export interface TaskActivityPage { export interface TaskActivityReadMarker { task_id: string; seen_before: string; + activity_id?: string; } export interface TaskActivityMarkReadResult { diff --git a/products/desktop/packages/shared/src/git-domain.ts b/products/desktop/packages/shared/src/git-domain.ts index 24cb09add12a..934652b4aed9 100644 --- a/products/desktop/packages/shared/src/git-domain.ts +++ b/products/desktop/packages/shared/src/git-domain.ts @@ -5,6 +5,7 @@ import { z } from "zod"; export const prReviewCommentUserSchema = z.object({ login: z.string(), avatar_url: z.string(), + isBot: z.boolean().optional(), }); export const prReviewCommentSchema = z.object({ @@ -156,6 +157,7 @@ export type GetPrChecksOutput = z.infer; export const prConversationCommentSchema = z.object({ id: z.number(), author: z.string(), + isBot: z.boolean().optional(), avatarUrl: z.string().nullable(), body: z.string(), createdAt: z.string(), diff --git a/products/desktop/packages/ui/src/features/canvas/components/ActivityHoverCard.tsx b/products/desktop/packages/ui/src/features/canvas/components/ActivityHoverCard.tsx index 82097a9e9723..87f102e2e1da 100644 --- a/products/desktop/packages/ui/src/features/canvas/components/ActivityHoverCard.tsx +++ b/products/desktop/packages/ui/src/features/canvas/components/ActivityHoverCard.tsx @@ -114,7 +114,7 @@ export function ActivityHoverCard({
{items.map((item) => ( diff --git a/products/desktop/packages/ui/src/features/canvas/components/ActivityPanel.test.tsx b/products/desktop/packages/ui/src/features/canvas/components/ActivityPanel.test.tsx new file mode 100644 index 000000000000..202ba3bca543 --- /dev/null +++ b/products/desktop/packages/ui/src/features/canvas/components/ActivityPanel.test.tsx @@ -0,0 +1,170 @@ +import type { Task } from "@posthog/shared/domain-types"; +import { act, fireEvent, render, screen } from "@testing-library/react"; +import { + afterEach, + beforeEach, + describe, + expect, + it, + type MockInstance, + vi, +} from "vitest"; + +vi.mock("@posthog/ui/features/canvas/hooks/useThreadConversation", () => ({ + useThreadConversation: () => ({ + timeline: [], + agentStatus: null, + events: [], + isPromptPending: false, + isReady: true, + members: [], + currentUser: null, + isTaskAuthor: true, + canForward: true, + draft: "", + setDraft: vi.fn(), + isSubmitDisabled: false, + submit: vi.fn(), + sendMessageToAgent: vi.fn(), + deleteMessage: vi.fn(), + onMentionInsert: vi.fn(), + }), +})); +vi.mock("@posthog/ui/features/canvas/components/ActivityTimeline", () => ({ + ActivityTimeline: () =>
timeline body
, +})); +vi.mock("@posthog/ui/features/canvas/components/TaskArtifactsList", () => ({ + TaskArtifactsList: () =>
artifacts body
, +})); +vi.mock("@posthog/ui/features/canvas/components/TaskCommentsList", () => ({ + TaskCommentsList: () =>
comments body
, +})); +vi.mock("@posthog/ui/features/canvas/components/ChannelFeedView", () => ({ + TaskCard: () =>
task card
, +})); +vi.mock("@posthog/ui/features/canvas/components/ThreadPanel", () => ({ + AgentStatusLine: () =>
agent status
, + ThreadLoadingState: () =>
loading
, + ThreadReplyComposer: () =>
composer
, +})); +vi.mock("@posthog/ui/features/tasks/queries", () => ({ + taskDetailQuery: () => ({ queryKey: ["task"], queryFn: vi.fn() }), +})); +vi.mock("@tanstack/react-query", () => ({ + useQuery: () => ({ data: undefined }), +})); +vi.mock("@posthog/ui/shell/analytics", () => ({ track: vi.fn() })); + +import { useCommentNavigationStore } from "@posthog/ui/features/sessions/commentNavigationStore"; +import { ActivityPanel } from "./ActivityPanel"; + +const task = { id: "task-1", title: "Ship it" } as unknown as Task; + +function renderPanel(taskId = "task-1") { + return render( + , + ); +} + +describe("ActivityPanel", () => { + let scrollTo: MockInstance; + + beforeEach(() => { + scrollTo = vi.spyOn(Element.prototype, "scrollTo"); + useCommentNavigationStore.setState({ + focusByTask: {}, + resolutionsByTarget: {}, + }); + }); + + afterEach(() => { + scrollTo.mockRestore(); + }); + + it("offers comments as a third tab beside the timeline and artifacts", () => { + renderPanel(); + + expect(screen.getByRole("tab", { name: "Timeline" })).toBeTruthy(); + expect(screen.getByRole("tab", { name: "Artifacts" })).toBeTruthy(); + fireEvent.click(screen.getByRole("tab", { name: "Comments" })); + + expect(screen.getByText("comments body")).toBeTruthy(); + // The composer belongs to the conversation, not to a list of threads. + expect(screen.queryByText("composer")).toBeNull(); + }); + + // A thread picked on the artifact itself lands in this tab, so the pick has + // to bring the tab with it. + it("switches to comments when a thread is picked elsewhere", () => { + renderPanel(); + expect(screen.getByText("timeline body")).toBeTruthy(); + + act(() => + useCommentNavigationStore + .getState() + .requestCommentFocus( + "task-1", + { scope: "task_artifact", itemId: "artifact-1" }, + "comment-1", + ), + ); + + expect(screen.getByText("comments body")).toBeTruthy(); + }); + + it("leaves a focus request for another task alone", () => { + renderPanel(); + + act(() => + useCommentNavigationStore + .getState() + .requestCommentFocus( + "task-2", + { scope: "task_artifact", itemId: "artifact-1" }, + "comment-1", + ), + ); + + expect(screen.getByText("timeline body")).toBeTruthy(); + }); + + // A focus left over from an earlier visit must not hijack the panel, and the + // panel is reused across tasks without remounting. + it("does not open comments for a focus that predates the task", () => { + useCommentNavigationStore + .getState() + .requestCommentFocus( + "task-2", + { scope: "task_artifact", itemId: "artifact-1" }, + "comment-1", + ); + const { rerender } = renderPanel("task-1"); + + rerender( + , + ); + + expect(screen.getByText("timeline body")).toBeTruthy(); + }); + + // Only the timeline reads bottom-up; the thread lists put what matters on top. + it("keeps the comments list where it was scrolled to", () => { + renderPanel(); + expect(scrollTo).toHaveBeenCalled(); + const timelineScrolls = scrollTo.mock.calls.length; + + fireEvent.click(screen.getByRole("tab", { name: "Comments" })); + + expect(scrollTo.mock.calls.length).toBe(timelineScrolls); + }); +}); diff --git a/products/desktop/packages/ui/src/features/canvas/components/ActivityPanel.tsx b/products/desktop/packages/ui/src/features/canvas/components/ActivityPanel.tsx index c59a26979b89..3d212e4cd56c 100644 --- a/products/desktop/packages/ui/src/features/canvas/components/ActivityPanel.tsx +++ b/products/desktop/packages/ui/src/features/canvas/components/ActivityPanel.tsx @@ -9,23 +9,26 @@ import type { Task } from "@posthog/shared/domain-types"; import { ActivityTimeline } from "@posthog/ui/features/canvas/components/ActivityTimeline"; import { TaskCard } from "@posthog/ui/features/canvas/components/ChannelFeedView"; import { TaskArtifactsList } from "@posthog/ui/features/canvas/components/TaskArtifactsList"; +import { TaskCommentsList } from "@posthog/ui/features/canvas/components/TaskCommentsList"; import { AgentStatusLine, ThreadLoadingState, ThreadReplyComposer, } from "@posthog/ui/features/canvas/components/ThreadPanel"; import { useThreadConversation } from "@posthog/ui/features/canvas/hooks/useThreadConversation"; +import { useCommentNavigationStore } from "@posthog/ui/features/sessions/commentNavigationStore"; import { buildConversationItems } from "@posthog/ui/features/sessions/components/buildConversationItems"; import { taskDetailQuery } from "@posthog/ui/features/tasks/queries"; import { track } from "@posthog/ui/shell/analytics"; import { useQuery } from "@tanstack/react-query"; import { useCallback, useEffect, useMemo, useRef, useState } from "react"; -type ActivityTab = "timeline" | "artifacts"; +type ActivityTab = "timeline" | "artifacts" | "comments"; const ACTIVITY_TABS: readonly { key: ActivityTab; label: string }[] = [ { key: "timeline", label: "Timeline" }, { key: "artifacts", label: "Artifacts" }, + { key: "comments", label: "Comments" }, ] as const; /** The 32px row this panel leads with: the tabs are the header, so the strip @@ -161,15 +164,55 @@ function ActivityConversation({ [tab, events, isPromptPending], ); + // A thread picked on the artifact itself lives in the Comments tab, so the + // pick has to bring the tab with it. Only a fresh request switches tabs: a + // focus left over from an earlier visit must not hijack the panel on mount. + const commentFocus = useCommentNavigationStore( + (state) => state.focusByTask[taskId], + ); + const acknowledgeCommentsTabOpen = useCommentNavigationStore( + (state) => state.acknowledgeCommentsTabOpen, + ); + // Tracks the task too: this panel is reused across tasks without remounting, + // so a nonce seen for the previous task says nothing about this one. + const seenFocus = useRef<{ taskId: string; nonce: number | null }>({ + taskId, + nonce: null, + }); + useEffect(() => { + if (seenFocus.current.taskId !== taskId) { + seenFocus.current = { taskId, nonce: null }; + return; + } + if ( + commentFocus?.openCommentsTab && + commentFocus.nonce !== seenFocus.current.nonce + ) { + seenFocus.current = { taskId, nonce: commentFocus.nonce }; + // Not handleTabChange: a programmatic switch isn't a user tab change. + setTab("comments"); + } + }, [commentFocus, taskId]); + useEffect(() => { + if (tab === "comments" && commentFocus?.openCommentsTab) { + acknowledgeCommentsTabOpen(taskId, commentFocus.nonce); + } + }, [acknowledgeCommentsTabOpen, commentFocus, tab, taskId]); + const scrollRef = useRef(null); // biome-ignore lint/correctness/useExhaustiveDependencies: scroll when rendered thread content changes useEffect(() => { + // Only the timeline reads bottom-up; the other tabs put what matters on top. + if (tab !== "timeline") return; scrollRef.current?.scrollTo({ top: scrollRef.current.scrollHeight }); }, [timeline, events.length, agentStatus?.phase, tab]); const showComposer = tab === "timeline"; const body = () => { + if (tab === "comments") { + return ; + } if (tab === "artifacts") { return ( ({ + toChannelDashboard: vi.fn(), + toChannelTask: vi.fn(), + toTaskDetail: vi.fn(), +})); + +vi.mock("@posthog/ui/router/navigationBridge", () => ({ + navigateToChannelDashboard: navigation.toChannelDashboard, + navigateToChannelTask: navigation.toChannelTask, + navigateToTaskDetail: navigation.toTaskDetail, +})); +vi.mock("@posthog/ui/shell/analytics", () => ({ track: vi.fn() })); + +import { useCommentNavigationStore } from "@posthog/ui/features/sessions/commentNavigationStore"; +import { ActivityRow, activityHeadline } from "./ActivityView"; function item(overrides: Partial): TaskActivityItem { return { @@ -21,6 +36,15 @@ function item(overrides: Partial): TaskActivityItem { } describe("activityHeadline", () => { + beforeEach(() => { + navigation.toChannelTask.mockReset(); + navigation.toChannelDashboard.mockReset(); + navigation.toTaskDetail.mockReset(); + useCommentNavigationStore.setState({ + focusByTask: {}, + resolutionsByTarget: {}, + }); + }); it.each([ [ "completed run", @@ -28,6 +52,33 @@ describe("activityHeadline", () => { "The agent completed this task", ], ["agent reply", item({ activityKind: "message" }), "The agent replied"], + [ + "thread reply", + item({ + activityKind: "thread_reply", + author: { + id: 2, + uuid: "author", + email: "author@posthog.com", + first_name: "Ann", + }, + }), + "replied to a thread you participated in", + ], + [ + "canvas owner comment", + item({ + activityKind: "owned_item_comment", + commentTarget: { scope: "desktop_canvas", itemId: "canvas-1" }, + author: { + id: 2, + uuid: "author", + email: "author@posthog.com", + first_name: "Ann", + }, + }), + "commented on your canvas", + ], [ "own reply", item({ @@ -59,4 +110,43 @@ describe("activityHeadline", () => { ); expect(getByText("#me")).toBeInTheDocument(); }); + + it("opens an activity mention at its exact comment thread", () => { + const activity = item({ + activityKind: "mention", + channelId: "channel-1", + commentId: "comment-1", + commentTarget: { scope: "desktop_canvas", itemId: "canvas-1" }, + author: { + id: 2, + uuid: "author", + email: "author@posthog.com", + first_name: "Ann", + }, + }); + + render( + , + ); + const activityButton = screen.getByText("mentioned you").closest("button"); + if (!activityButton) throw new Error("Expected activity row button"); + fireEvent.click(activityButton); + + expect(navigation.toChannelDashboard).toHaveBeenCalledWith( + "channel-1", + "canvas-1", + ); + expect(navigation.toChannelTask).not.toHaveBeenCalled(); + expect(useCommentNavigationStore.getState().focusByTask["task-1"]).toEqual({ + target: { scope: "desktop_canvas", itemId: "canvas-1" }, + threadId: "comment-1", + nonce: expect.any(Number), + openCommentsTab: true, + }); + }); }); diff --git a/products/desktop/packages/ui/src/features/canvas/components/ActivityView.tsx b/products/desktop/packages/ui/src/features/canvas/components/ActivityView.tsx index cd22d7c9364d..4871cf946fc0 100644 --- a/products/desktop/packages/ui/src/features/canvas/components/ActivityView.tsx +++ b/products/desktop/packages/ui/src/features/canvas/components/ActivityView.tsx @@ -28,8 +28,11 @@ import { MentionText } from "@posthog/ui/features/canvas/components/MentionText" import { useChannelsLayout } from "@posthog/ui/features/canvas/hooks/useChannelsLayout"; import { useMarkTaskActivityRead } from "@posthog/ui/features/canvas/hooks/useMarkTaskActivityRead"; import { useTaskActivity } from "@posthog/ui/features/canvas/hooks/useTaskActivity"; +import { useCanvasChatPanelStore } from "@posthog/ui/features/canvas/stores/canvasChatPanelStore"; +import { useThreadPanelStore } from "@posthog/ui/features/canvas/stores/threadPanelStore"; import { copyChannelLink } from "@posthog/ui/features/canvas/utils/copyChannelLink"; import { userDisplayName } from "@posthog/ui/features/canvas/utils/userDisplay"; +import { useCommentNavigationStore } from "@posthog/ui/features/sessions/commentNavigationStore"; import { PageHeader, PageHeaderActions, @@ -40,6 +43,7 @@ import { PageHeaderTitleRow, } from "@posthog/ui/primitives/PageHeader"; import { + navigateToChannelDashboard, navigateToChannelTask, navigateToTaskDetail, } from "@posthog/ui/router/navigationBridge"; @@ -65,6 +69,17 @@ function ChannelSuffix({ channelName }: { channelName: string | null }) { ); } +function ownedItemName(item: TaskActivityItem): string { + switch (item.commentTarget?.scope) { + case "desktop_canvas": + return "canvas"; + case "task_artifact": + return "artifact"; + default: + return "task"; + } +} + /** The lead line describing what happened, chosen by the row's activity kind. */ export function activityHeadline( item: TaskActivityItem, @@ -112,6 +127,26 @@ export function activityHeadline( ); + case "thread_reply": + return ( + <> + + {userDisplayName(item.author)} + {" "} + replied to a thread you participated in + + + ); + case "owned_item_comment": + return ( + <> + + {userDisplayName(item.author)} + {" "} + commented on your {ownedItemName(item)} + + + ); default: return "You created this task"; } @@ -149,10 +184,23 @@ export function ActivityRow({ task_id: item.taskId, }); onOpen(item); + if (item.commentId && item.commentTarget) { + useCommentNavigationStore + .getState() + .requestCommentFocus(item.taskId, item.commentTarget, item.commentId); + } onNavigate?.(); + if (channelId && item.commentTarget?.scope === "desktop_canvas") { + useCanvasChatPanelStore.getState().openComments(); + navigateToChannelDashboard(channelId, item.commentTarget.itemId); + return; + } // The channel thread route is the deep-link target; unfiled tasks fall // back to the plain task view. if (channelId) { + if (item.commentId) { + useThreadPanelStore.getState().setCollapsed(false); + } navigateToChannelTask(channelId, item.taskId); } else { navigateToTaskDetail(item.taskId); @@ -270,7 +318,13 @@ export function ActivityView() { // reached any other way, so the feed converges either way. const markRead = useCallback( (item: TaskActivityItem) => - markTasksRead([{ task_id: item.taskId, seen_before: item.activityAt }]), + markTasksRead([ + { + task_id: item.taskId, + seen_before: item.activityAt, + ...(item.commentId ? { activity_id: item.id } : {}), + }, + ]), [markTasksRead], ); const markAllRead = useCallback(() => { @@ -311,8 +365,8 @@ export function ActivityView() { No activity yet - Tasks you create, get tagged in, or reply to across{" "} - {spacesLayout ? "spaces" : "channels"} land here. + Task updates and comment notifications across{" "} + {spacesLayout ? "spaces" : "channels"} appear here. @@ -320,7 +374,7 @@ export function ActivityView() {
{items.map((item) => ( - Tasks you're involved in across spaces. + Task updates and comment notifications across spaces. @@ -379,7 +433,7 @@ export function ActivityView() { Activity - Tasks you're involved in across{" "} + Task updates and comment notifications across{" "} {spacesLayout ? "spaces" : "channels"}.
diff --git a/products/desktop/packages/ui/src/features/canvas/components/MentionComposer.tsx b/products/desktop/packages/ui/src/features/canvas/components/MentionComposer.tsx index df0826b9945c..71b1b95dfdf6 100644 --- a/products/desktop/packages/ui/src/features/canvas/components/MentionComposer.tsx +++ b/products/desktop/packages/ui/src/features/canvas/components/MentionComposer.tsx @@ -29,6 +29,8 @@ interface MentionComposerProps { placeholder?: string; rows?: number; inputClassName?: string; + /** Put the caret in the editor on mount, for a composer the user just opened. */ + autoFocus?: boolean; /** Rendered inside the input group after the editor (send button etc.). */ children?: ReactNode; } @@ -57,6 +59,7 @@ export function MentionComposer({ onValueChange, onSubmit, members, + autoFocus = false, allowAgentMention = false, onMentionInsert, placeholder, @@ -99,6 +102,7 @@ export function MentionComposer({ const editor = useEditor( { + autofocus: autoFocus ? "end" : false, extensions: [ StarterKit.configure({ heading: false, diff --git a/products/desktop/packages/ui/src/features/canvas/components/TaskArtifactsList.test.tsx b/products/desktop/packages/ui/src/features/canvas/components/TaskArtifactsList.test.tsx index cb0a58dc5671..c91f8062e533 100644 --- a/products/desktop/packages/ui/src/features/canvas/components/TaskArtifactsList.test.tsx +++ b/products/desktop/packages/ui/src/features/canvas/components/TaskArtifactsList.test.tsx @@ -1,5 +1,5 @@ import type { Task, TaskRun, TaskRunArtifact } from "@posthog/shared"; -import { fireEvent, render, screen } from "@testing-library/react"; +import { fireEvent, render, screen, within } from "@testing-library/react"; import { beforeEach, describe, expect, it, vi } from "vitest"; const mocks = vi.hoisted(() => ({ @@ -33,6 +33,36 @@ vi.mock("@posthog/ui/features/pr-review/usePrComments", () => ({ vi.mock("@posthog/ui/features/pr-review/usePrReviewThreads", () => ({ usePrReviewThreads: () => ({ data: undefined }), })); +vi.mock("@posthog/ui/features/sessions/components/useComments", () => ({ + useCommentsForTargetsQuery: () => ({ + data: [ + { + id: "comment-1", + source_comment: null, + item_id: "a", + content: "Tighten this summary", + created_at: "2024-01-01T00:00:00Z", + item_context: { anchor: { kind: "document" } }, + }, + { + id: "reply-1", + source_comment: "comment-1", + item_id: "a", + content: "Agreed", + created_at: "2024-01-01T00:01:00Z", + item_context: { anchor: { kind: "document" } }, + }, + { + id: "comment-2", + source_comment: null, + item_id: "a", + content: "Second thread", + created_at: "2024-01-01T00:02:00Z", + item_context: { anchor: { kind: "document" } }, + }, + ], + }), +})); import { useReviewNavigationStore } from "@posthog/ui/features/code-review/reviewNavigationStore"; import { TaskArtifactsList } from "./TaskArtifactsList"; @@ -123,15 +153,29 @@ describe("TaskArtifactsList", () => { expect(screen.getByText("Pull request #2")).toBeTruthy(); }); - it("lists the files the agent uploaded, with their size", () => { + it("lists uploaded files with their comment count", () => { + mocks.runs = [ + run("run-1", { artifacts: [outputFile({ id: "a", size: 16861 })] }), + ]; + + render(); + + const row = screen.getByText("report.md").closest("button"); + expect(row).not.toBeNull(); + expect(within(row as HTMLElement).getByText("2")).toBeTruthy(); + expect(within(row as HTMLElement).queryByText(/File|KB/)).toBeNull(); + }); + + // The threads themselves live in the Comments tab now, so the pane must not + // grow a second list of them. + it("leaves the thread list to the Comments tab", () => { mocks.runs = [ run("run-1", { artifacts: [outputFile({ id: "a", size: 16861 })] }), ]; render(); - expect(screen.getByText("report.md")).toBeTruthy(); - expect(screen.getByText("File · 17 KB")).toBeTruthy(); + expect(screen.queryByText("Tighten this summary")).toBeNull(); }); // The row should read like the chat's file list: markdown looks like @@ -196,7 +240,7 @@ describe("TaskArtifactsList", () => { render(); expect(screen.getAllByText("report.md")).toHaveLength(1); - expect(screen.getByText("File · 2 KB")).toBeTruthy(); + expect(screen.queryByText(/File ·|KB/)).toBeNull(); }); it.each([ diff --git a/products/desktop/packages/ui/src/features/canvas/components/TaskArtifactsList.tsx b/products/desktop/packages/ui/src/features/canvas/components/TaskArtifactsList.tsx index c01cafd5a6f7..46a4aae73b43 100644 --- a/products/desktop/packages/ui/src/features/canvas/components/TaskArtifactsList.tsx +++ b/products/desktop/packages/ui/src/features/canvas/components/TaskArtifactsList.tsx @@ -1,28 +1,25 @@ import { ArrowSquareOutIcon, + ChatCircleIcon, PackageIcon, SlackLogoIcon, } from "@phosphor-icons/react"; -import { - OUTPUT_ARTIFACT_TYPES, - parseRunArtifacts, - type RunArtifact, -} from "@posthog/core/canvas/runArtifactSchemas"; +import type { ResourceComment } from "@posthog/api-client/posthog-client"; import type { ThreadTimelineRow } from "@posthog/core/canvas/threadTimeline"; import { + Badge, Empty, EmptyDescription, EmptyHeader, EmptyMedia, EmptyTitle, } from "@posthog/quill"; -import { readPrUrls } from "@posthog/shared"; -import type { - Task, - TaskRun, - TaskThreadMessage, -} from "@posthog/shared/domain-types"; +import type { Task, TaskThreadMessage } from "@posthog/shared/domain-types"; import { iconForTemplate } from "@posthog/ui/features/canvas/components/canvasTemplateIcon"; +import { + buildRows, + commentTargets, +} from "@posthog/ui/features/canvas/components/taskArtifactRows"; import { useTaskRuns } from "@posthog/ui/features/canvas/hooks/useTaskRuns"; import { canvasArtifactOpenHandler } from "@posthog/ui/features/canvas/utils/canvasArtifactNavigation"; import { openPrInReview } from "@posthog/ui/features/code-review/openPrInReview"; @@ -30,97 +27,13 @@ import { usePrArtifact } from "@posthog/ui/features/git-interaction/usePrArtifac import { usePanelLayoutStore } from "@posthog/ui/features/panels/panelLayoutStore"; import { usePrComments } from "@posthog/ui/features/pr-review/usePrComments"; import { usePrReviewThreads } from "@posthog/ui/features/pr-review/usePrReviewThreads"; +import { buildCommentThreads } from "@posthog/ui/features/sessions/components/commentViewTypes"; +import { useCommentsForTargetsQuery } from "@posthog/ui/features/sessions/components/useComments"; import { FileIcon } from "@posthog/ui/primitives/FileIcon"; import { openExternalUrl } from "@posthog/ui/shell/openExternal"; -import { formatFileSize } from "@posthog/ui/utils/formatFileSize"; import { type ReactNode, useMemo, useState } from "react"; -type ArtifactRow = - | { kind: "pr"; key: string; url: string } - | { kind: "canvas"; key: string; name: string; url: string | null } - | { - kind: "file"; - key: string; - artifactId: string | null; - name: string; - runId: string | null; - size: number | undefined; - } - | { kind: "slack"; key: string; url: string }; - -function readRunOutputs(run: TaskRun): RunArtifact[] { - return parseRunArtifacts( - (run as { artifacts?: unknown }).artifacts, - OUTPUT_ARTIFACT_TYPES, - ); -} - -function buildRows( - task: Task, - timeline: ThreadTimelineRow[], - runs: TaskRun[], -): ArtifactRow[] { - const rows: ArtifactRow[] = []; - const seenPrUrls = new Set(); - - const addPr = (url: string, key: string) => { - if (seenPrUrls.has(url)) return; - seenPrUrls.add(url); - rows.push({ kind: "pr", key, url }); - }; - - for (const row of timeline) { - if (row.kind !== "artifact") continue; - if (row.artifact.kind === "pr") { - addPr(row.artifact.url, row.message.id); - } else { - rows.push({ - kind: "canvas", - key: row.message.id, - name: row.artifact.name, - url: row.artifact.url, - }); - } - } - - const allRuns = - runs.length > 0 ? runs : task.latest_run ? [task.latest_run] : []; - - // Re-uploading a file replaces it rather than adding a second one: agents - // revise a deliverable and upload it again under the same name, so keeping - // every copy would bury the current one under its own drafts. - const newestByName = new Map(); - for (const run of allRuns) { - for (const outputPr of readPrUrls(run.output)) { - addPr(outputPr, `output-pr:${outputPr}`); - } - for (const file of readRunOutputs(run)) { - if (!file.name) continue; - const previous = newestByName.get(file.name); - const isNewer = - !previous || - (file.uploaded_at ?? "") >= (previous.file.uploaded_at ?? ""); - if (isNewer) newestByName.set(file.name, { file, runId: run.id }); - } - } - for (const [name, { file, runId }] of newestByName) { - rows.push({ - kind: "file", - key: `file:${file.id ?? file.storage_path ?? name}`, - artifactId: file.id ?? null, - name, - runId, - size: file.size, - }); - } - - const slackUrl = task.latest_run?.state?.slack_thread_url; - if (typeof slackUrl === "string" && slackUrl) { - rows.push({ kind: "slack", key: "slack-thread", url: slackUrl }); - } - - return rows; -} +const EMPTY_COMMENTS: ResourceComment[] = []; function ArtifactListRow({ icon, @@ -133,7 +46,7 @@ function ArtifactListRow({ }: { icon: ReactNode; title: string; - detail?: string | null; + detail?: ReactNode; external?: boolean; onOpen?: () => void; /** Renders a trailing button that leaves the app instead of opening the @@ -227,13 +140,30 @@ function PrRow({ ); } -function CanvasRow({ name, url }: { name: string; url: string | null }) { +function CanvasRow({ + name, + url, + commentCount, +}: { + name: string; + url: string | null; + commentCount: number; +}) { const open = canvasArtifactOpenHandler(url); return ( 0 ? ( + + + {commentCount} + + ) : ( + "Canvas" + ) + } onOpen={open} /> ); @@ -244,13 +174,14 @@ function FileRow({ runId, artifactId, name, - size, + commentCount, }: { taskId: string; runId: string | null; artifactId: string | null; name: string; - size: number | undefined; + /** Supplied by the pane's single comments query so each row doesn't fetch. */ + commentCount: number; }) { const openArtifactTab = usePanelLayoutStore((state) => state.openArtifactTab); const canOpen = !!runId && !!artifactId; @@ -267,7 +198,16 @@ function FileRow({ } title={name} - detail={["File", formatFileSize(size)].filter(Boolean).join(" · ")} + detail={ + commentCount > 0 ? ( + + + {commentCount} + + ) : ( + "File" + ) + } onOpen={onOpen} /> ); @@ -289,6 +229,22 @@ export function TaskArtifactsList({ () => buildRows(task, timeline, runs), [task, timeline, runs], ); + // One query for every row's badge, so N resources cost one request rather + // than one per row. The threads themselves live in the Comments tab. + const targets = useMemo(() => commentTargets(rows), [rows]); + const commentsQuery = useCommentsForTargetsQuery(targets, task.id); + const comments = commentsQuery.data ?? EMPTY_COMMENTS; + // Open threads only, so a row's badge agrees with what the Comments tab + // shows on the same resource. + const openCountByItem = useMemo(() => { + const counts = new Map(); + for (const thread of buildCommentThreads(comments)) { + const itemId = thread.root.item_id; + if (thread.resolved || !itemId) continue; + counts.set(itemId, (counts.get(itemId) ?? 0) + 1); + } + return counts; + }, [comments]); if (rows.length === 0) { return ( @@ -307,6 +263,20 @@ export function TaskArtifactsList({ ); } + if (commentsQuery.isError) { + return ( + + + + + + Couldn't load comment counts + Refresh the page to try again. + + + ); + } + return (
{rows.map((row) => @@ -317,7 +287,14 @@ export function TaskArtifactsList({ openInPlaceTaskId={canOpenInPlace ? task.id : undefined} /> ) : row.kind === "canvas" ? ( - + ) : row.kind === "file" ? ( ) : ( ({ + runs: [] as TaskRun[], + comments: [] as unknown[], + activeArtifactId: null as string | null, + prConversation: [] as unknown[], + prReviewThreads: [] as unknown[], + openArtifactTab: vi.fn(), + openPrInReview: vi.fn(), + openExternalUrl: vi.fn(), + requestScrollToFile: vi.fn(), + prReply: vi.fn(async () => true), + prResolve: vi.fn(async () => true), + createComment: vi.fn(), + setResolved: vi.fn(), + createdFor: [] as unknown[], + resolvedFor: [] as unknown[], + queriedTargets: [] as unknown[], +})); + +function openThread(body: string): void { + const card = screen.getByText(body).closest("[data-comment-thread-id]"); + expect(card).not.toBeNull(); + fireEvent.click( + within(card as HTMLElement).getByRole("button", { + name: "Open comment thread", + }), + ); +} + +vi.mock("@posthog/ui/features/canvas/hooks/useTaskRuns", () => ({ + useTaskRuns: () => ({ runs: mocks.runs, isLoading: false }), +})); +vi.mock("@posthog/ui/features/canvas/hooks/useOrgMembers", () => ({ + useOrgMembers: () => ({ members: [] }), +})); +vi.mock("@posthog/ui/features/panels/panelLayoutStore", () => ({ + usePanelLayoutStore: () => mocks.openArtifactTab, + useActiveArtifactId: () => mocks.activeArtifactId, +})); +vi.mock("@posthog/ui/features/pr-review/usePrCommentsForUrls", () => ({ + usePrCommentsForUrls: (urls: string[]) => ({ + byUrl: new Map(urls.map((url) => [url, mocks.prConversation])), + isLoading: false, + }), +})); +vi.mock("@posthog/ui/features/pr-review/usePrReviewThreadsForUrls", () => ({ + usePrReviewThreadsForUrls: (urls: string[]) => ({ + byUrl: new Map(urls.map((url) => [url, mocks.prReviewThreads])), + isLoading: false, + }), +})); +vi.mock("@posthog/ui/features/git-interaction/usePrDetails", () => ({ + usePrTitles: () => ({}), +})); +vi.mock("@posthog/ui/shell/openExternal", () => ({ + openExternalUrl: (url: string) => mocks.openExternalUrl(url), +})); +// GitHub bodies render through MarkdownRenderer; the wiring under test is the +// list, not the markdown pipeline, so keep it to plain text here. +vi.mock("@posthog/ui/features/editor/components/MarkdownRenderer", () => ({ + MarkdownRenderer: ({ content }: { content: string }) => ( + {content} + ), +})); +vi.mock("@posthog/ui/features/code-review/openPrInReview", () => ({ + openPrInReview: (taskId: string, url: string) => + mocks.openPrInReview(taskId, url), +})); +vi.mock("@posthog/ui/features/code-review/reviewNavigationStore", () => ({ + useReviewNavigationStore: { + getState: () => ({ requestScrollToFile: mocks.requestScrollToFile }), + }, +})); +// Tiptap's editor renders no placeholder attribute and drags a lot of DOM into +// jsdom; the wiring under test is which target a composed comment posts to. +vi.mock("@posthog/ui/features/sessions/components/CommentComposer", () => ({ + CommentComposer: ({ + placeholder, + onSubmit, + }: { + placeholder: string; + onSubmit: (content: string, mentions: number[]) => void; + }) => ( + + ), +})); +vi.mock("@posthog/ui/features/code-review/hooks/usePrCommentActions", () => ({ + usePrCommentActions: () => ({ + reply: mocks.prReply, + resolve: mocks.prResolve, + }), +})); +vi.mock("@posthog/ui/features/sessions/components/useComments", () => ({ + isOptimisticComment: (comment: ResourceComment) => + comment.id.startsWith("optimistic-"), + useCommentsQuery: (target: unknown) => { + if (target) mocks.queriedTargets.push([target]); + return { + data: mocks.comments, + isLoading: false, + }; + }, + useCommentsForTargetsQuery: (targets: unknown) => { + if (Array.isArray(targets) && targets.length > 0) { + mocks.queriedTargets.push(targets); + } + return { + data: mocks.comments, + isLoading: false, + }; + }, + useCreateComment: (target: unknown) => { + mocks.createdFor.push(target); + return { mutateAsync: mocks.createComment, isPending: false }; + }, + useSetCommentResolved: (target: unknown) => { + mocks.resolvedFor.push(target); + return { mutate: mocks.setResolved, isPending: false }; + }, +})); + +import { useCommentNavigationStore } from "@posthog/ui/features/sessions/commentNavigationStore"; +import { TaskCommentsList } from "./TaskCommentsList"; + +const task = { id: "task-1", latest_run: null } as unknown as Task; + +function run(artifacts: Partial[], id = "run-1"): TaskRun { + return { id, output: null, artifacts } as unknown as TaskRun; +} + +function outputFile( + overrides: Partial, +): Partial { + return { + type: "output", + name: "report.md", + storage_path: "runs/1/report.md", + ...overrides, + }; +} + +function prRun(url: string): TaskRun { + return { + id: "run-pr", + output: { pr_url: url }, + artifacts: [], + } as unknown as TaskRun; +} + +function reviewThread(overrides: Record = {}) { + return { + nodeId: "node-1", + isResolved: false, + rootId: 501, + filePath: "packages/ui/src/App.tsx", + comments: [ + { + id: 501, + body: "This needs a guard", + path: "packages/ui/src/App.tsx", + line: 12, + user: { login: "octocat", avatar_url: "" }, + created_at: "2024-01-02T00:00:00Z", + }, + ], + ...overrides, + }; +} + +function comment(overrides: Partial): ResourceComment { + return { + id: "comment-1", + created_by: null, + content: "Tighten this summary", + created_at: "2024-01-01T00:00:00Z", + item_id: "a", + item_context: { anchor: { kind: "document" } }, + scope: "task_artifact", + source_comment: null, + ...overrides, + } as ResourceComment; +} + +describe("TaskCommentsList", () => { + beforeEach(() => { + mocks.runs = [ + run([ + outputFile({ id: "a", name: "report.md" }), + outputFile({ + id: "b", + name: "summary.md", + storage_path: "runs/1/summary.md", + }), + ]), + ]; + mocks.comments = [ + comment({}), + comment({ + id: "reply-1", + source_comment: "comment-1", + content: "Agreed", + created_at: "2024-01-01T00:01:00Z", + }), + comment({ + id: "comment-2", + item_id: "b", + content: "Second thread", + created_at: "2024-01-01T00:02:00Z", + }), + ]; + mocks.activeArtifactId = null; + mocks.prConversation = []; + mocks.prReviewThreads = []; + mocks.openArtifactTab.mockReset(); + mocks.openPrInReview.mockReset(); + mocks.openExternalUrl.mockReset(); + mocks.requestScrollToFile.mockReset(); + mocks.prReply.mockClear(); + mocks.prResolve.mockClear(); + mocks.createComment.mockReset(); + mocks.createComment.mockResolvedValue({ id: "created-comment" }); + mocks.setResolved.mockReset(); + mocks.createdFor = []; + mocks.resolvedFor = []; + mocks.queriedTargets = []; + useCommentNavigationStore.setState({ + focusByTask: {}, + resolutionsByTarget: {}, + }); + }); + + it("queries and displays only the current canvas when restricted", async () => { + const onCanvasCommentOpen = vi.fn(); + mocks.comments = [ + comment({ + item_id: "canvas-1", + scope: "desktop_canvas", + content: "Canvas feedback", + item_context: { + anchor: { + kind: "text", + quote: "important copy", + prefix: "", + suffix: "", + start: 0, + end: 14, + }, + canvasVersionId: "version-2", + }, + }), + ]; + + render( + "V2"} + onCanvasCommentOpen={onCanvasCommentOpen} + />, + ); + + expect(mocks.queriedTargets.at(-1)).toEqual([ + { scope: "desktop_canvas", itemId: "canvas-1" }, + ]); + expect(screen.getByText("Canvas feedback")).toBeInTheDocument(); + expect(screen.getByText("“important copy”")).toBeInTheDocument(); + expect(screen.getByText("V2 ·")).toBeInTheDocument(); + expect(screen.queryByText("Selected text")).not.toBeInTheDocument(); + expect(screen.queryByText("Whole canvas")).not.toBeInTheDocument(); + expect(screen.queryByLabelText("Filter by source")).not.toBeInTheDocument(); + expect(screen.queryByText("Launch canvas")).not.toBeInTheDocument(); + expect(mocks.createdFor.at(-1)).toEqual({ + scope: "desktop_canvas", + itemId: "canvas-1", + }); + + openThread("Canvas feedback"); + expect(onCanvasCommentOpen).toHaveBeenCalledWith("version-2"); + + await act(async () => { + fireEvent.click(screen.getByText(/Comment on this canvas/)); + }); + expect(mocks.createComment).toHaveBeenCalledWith({ + content: "Composed comment", + context: { + anchor: { kind: "document" }, + canvasVersionId: "version-2", + }, + mentions: [], + }); + }); + + it("shows selected artifact text alongside its source", () => { + mocks.comments = [ + comment({ + item_context: { + anchor: { + kind: "text", + quote: "Purpose", + prefix: "", + suffix: "", + start: 0, + end: 7, + }, + }, + }), + ]; + + render(); + + expect(screen.getByText("report.md")).toBeTruthy(); + expect(screen.getByText("“Purpose”")).toBeTruthy(); + }); + + it("loads canvas comments from a local-development artifact link", () => { + mocks.runs = []; + mocks.comments = [ + comment({ + item_id: "canvas-1", + scope: "desktop_canvas", + content: "Linked canvas feedback", + }), + ]; + + const timeline = [ + { + kind: "artifact", + timestamp: 1, + message: { id: "message-1" }, + artifact: { + kind: "canvas", + name: "Dev Joke Machine", + url: "http://localhost:8000/code/canvas/channel-1/canvas-1", + }, + }, + ] as unknown as ThreadTimelineRow[]; + + render(); + + expect(mocks.queriedTargets.at(-1)).toContainEqual({ + scope: "desktop_canvas", + itemId: "canvas-1", + }); + expect(screen.getByText("Linked canvas feedback")).toBeInTheDocument(); + }); + + // The tab is the one place to see every thread the task produced, so each row + // has to say which artifact it came from. + it("lists open threads from every artifact, newest first", () => { + render(); + + const newest = screen.getByText("Second thread"); + const oldest = screen.getByText("Tighten this summary"); + expect( + newest.compareDocumentPosition(oldest) & Node.DOCUMENT_POSITION_FOLLOWING, + ).toBeTruthy(); + expect(screen.getByText("summary.md")).toBeTruthy(); + expect(screen.getByText("report.md")).toBeTruthy(); + expect(screen.getByText(/1 reply/)).toBeTruthy(); + // The resolve/reopen reply is thread state, not a comment of its own. + expect(screen.queryByText("Agreed")).toBeTruthy(); + }); + + it("opens the artifact a thread belongs to and focuses that thread", () => { + render(); + + openThread("Tighten this summary"); + + expect(mocks.openArtifactTab).toHaveBeenCalledWith("task-1", { + runId: "run-1", + artifactId: "a", + name: "report.md", + }); + expect(useCommentNavigationStore.getState().focusByTask["task-1"]).toEqual({ + target: { scope: "task_artifact", itemId: "a" }, + threadId: "comment-1", + nonce: expect.any(Number), + openCommentsTab: true, + }); + }); + + it("opens an artifact when activity requests its comment thread", () => { + render(); + + act(() => { + useCommentNavigationStore + .getState() + .requestCommentFocus( + "task-1", + { scope: "task_artifact", itemId: "a" }, + "comment-1", + ); + }); + + expect(mocks.openArtifactTab).toHaveBeenCalledWith("task-1", { + runId: "run-1", + artifactId: "a", + name: "report.md", + }); + }); + + it("opens the saved canvas version when activity requests its thread", () => { + const onCanvasCommentOpen = vi.fn(); + mocks.comments = [ + comment({ + item_id: "canvas-1", + scope: "desktop_canvas", + content: "Historical canvas feedback", + item_context: { + anchor: { kind: "document" }, + canvasVersionId: "version-2", + }, + }), + ]; + + render( + , + ); + + act(() => { + useCommentNavigationStore + .getState() + .requestCommentFocus( + "task-1", + { scope: "desktop_canvas", itemId: "canvas-1" }, + "comment-1", + ); + }); + + expect(onCanvasCommentOpen).toHaveBeenCalledWith("version-2"); + }); + + // Clicking the same thread twice has to scroll twice, so every request is a + // new nonce rather than a no-op set. + it("re-requests focus for a thread already focused", () => { + render(); + + openThread("Tighten this summary"); + const first = useCommentNavigationStore.getState().focusByTask["task-1"]; + openThread("Tighten this summary"); + const second = useCommentNavigationStore.getState().focusByTask["task-1"]; + + expect(second?.nonce).toBeGreaterThan(first?.nonce ?? 0); + }); + + it("filters between open and resolved threads", () => { + mocks.comments = [ + comment({}), + comment({ + id: "state-1", + source_comment: "comment-1", + content: "Resolved this thread", + created_at: "2024-01-01T00:03:00Z", + item_context: { + anchor: { kind: "document" }, + threadState: "resolved", + }, + }), + comment({ + id: "comment-2", + item_id: "b", + content: "Second thread", + created_at: "2024-01-01T00:02:00Z", + }), + ]; + + render(); + + expect(screen.getByText("Second thread")).toBeTruthy(); + expect(screen.queryByText("Tighten this summary")).toBeNull(); + + fireEvent.click(screen.getByLabelText("Filter comments")); + fireEvent.click(screen.getByText("Resolved (1)")); + + expect(screen.getByText("Tighten this summary")).toBeTruthy(); + expect(screen.queryByText("Second thread")).toBeNull(); + }); + + it("warns when the anchored text the thread points at has changed", () => { + useCommentNavigationStore.setState({ + resolutionsByTarget: { + "task_artifact:a": new Map([["comment-1", "orphaned" as const]]), + }, + }); + + render(); + + expect(screen.getByText("The highlighted text changed")).toBeTruthy(); + }); + + it("replies and resolves against the thread's own resource", () => { + render(); + + // Each row builds its mutations from its own target, since the list spans + // several resources. + expect(mocks.createdFor).toContainEqual({ + scope: "task_artifact", + itemId: "a", + }); + expect(mocks.resolvedFor).toContainEqual({ + scope: "task_artifact", + itemId: "b", + }); + + const thread = screen + .getByText("Tighten this summary") + .closest("[data-comment-thread-id]") as HTMLElement; + fireEvent.click(within(thread).getByText("Resolve")); + + expect(mocks.setResolved).toHaveBeenCalledWith({ + root: expect.objectContaining({ id: "comment-1" }), + resolved: true, + }); + }); + + // The pane follows what's on screen, but a reader who picks a source owns the + // filter from then on. + it("narrows to the artifact open in the main pane", () => { + mocks.activeArtifactId = "b"; + + render(); + + expect(screen.getByText("Second thread")).toBeTruthy(); + expect(screen.queryByText("Tighten this summary")).toBeNull(); + }); + + it("labels an active artifact that has no comments", () => { + mocks.runs = [ + run([ + outputFile({ id: "a", name: "report.md" }), + outputFile({ + id: "empty", + name: "empty.md", + storage_path: "runs/1/empty.md", + }), + ]), + ]; + mocks.comments = [comment({})]; + mocks.activeArtifactId = "empty"; + + render(); + + expect(screen.getByLabelText("Filter by source")).toHaveTextContent( + "empty.md", + ); + expect(screen.queryByText("Tighten this summary")).toBeNull(); + fireEvent.click(screen.getByLabelText("Filter by source")); + const emptySourceOption = screen + .getAllByText("empty.md") + .at(-1) + ?.closest('[role="menuitemradio"]'); + expect(emptySourceOption).toHaveTextContent("0"); + }); + + it("stops following the main pane once a source is picked by hand", () => { + mocks.activeArtifactId = "b"; + const { rerender } = render(); + + fireEvent.click(screen.getByLabelText("Filter by source")); + fireEvent.click(screen.getByText(/^All sources/)); + mocks.activeArtifactId = "a"; + rerender(); + + expect(screen.getByText("Second thread")).toBeTruthy(); + expect(screen.getByText("Tighten this summary")).toBeTruthy(); + }); + + it("lists a PR's review threads and conversation comments", () => { + mocks.runs = [prRun("https://github.com/acme/repo/pull/7")]; + mocks.comments = []; + mocks.prReviewThreads = [reviewThread()]; + mocks.prConversation = [ + { + id: 900, + author: "octocat", + avatarUrl: null, + body: "Shipping this", + createdAt: "2024-01-03T00:00:00Z", + url: "https://github.com/acme/repo/pull/7#issuecomment-900", + }, + ]; + + render(); + + expect(screen.getByText("This needs a guard")).toBeTruthy(); + expect(screen.getByText("Shipping this")).toBeTruthy(); + expect(screen.getAllByText("PR #7").length).toBe(2); + // Only the file-anchored thread can be resolved on GitHub. + expect(screen.getAllByText("Resolve")).toHaveLength(1); + // The conversation comment can't be handled here, so it links out instead. + expect(screen.getByText("View on GitHub")).toBeTruthy(); + }); + + it("links a conversation comment out to GitHub", () => { + mocks.runs = [prRun("https://github.com/acme/repo/pull/7")]; + mocks.comments = []; + mocks.prConversation = [ + { + id: 900, + author: "octocat", + avatarUrl: null, + body: "Shipping this", + createdAt: "2024-01-03T00:00:00Z", + url: "https://github.com/acme/repo/pull/7#issuecomment-900", + }, + ]; + + render(); + fireEvent.click(screen.getByText("View on GitHub")); + + expect(mocks.openExternalUrl).toHaveBeenCalledWith( + "https://github.com/acme/repo/pull/7#issuecomment-900", + ); + }); + + it("opens a PR thread in the review pane at its file", () => { + mocks.runs = [prRun("https://github.com/acme/repo/pull/7")]; + mocks.comments = []; + mocks.prReviewThreads = [reviewThread()]; + + render(); + openThread("This needs a guard"); + + expect(mocks.openPrInReview).toHaveBeenCalledWith( + "task-1", + "https://github.com/acme/repo/pull/7", + ); + expect(mocks.requestScrollToFile).toHaveBeenCalledWith( + "task-1", + "packages/ui/src/App.tsx", + ); + }); + + it("replies and resolves a PR thread on GitHub", () => { + mocks.runs = [prRun("https://github.com/acme/repo/pull/7")]; + mocks.comments = []; + mocks.prReviewThreads = [reviewThread()]; + + render(); + const thread = screen + .getByText("This needs a guard") + .closest("[data-comment-thread-id]") as HTMLElement; + fireEvent.click(within(thread).getByText("Resolve")); + + expect(mocks.prResolve).toHaveBeenCalledWith("node-1", true); + expect(mocks.setResolved).not.toHaveBeenCalled(); + }); + + // Not every comment belongs to a deliverable; some are about the work. + it("posts a comment on the task itself", async () => { + render(); + + await act(async () => { + fireEvent.click(screen.getByText(/Comment on this task/)); + }); + + expect(mocks.createdFor).toContainEqual({ + scope: "task", + itemId: "task-1", + }); + expect(mocks.createComment).toHaveBeenCalledWith( + expect.objectContaining({ + content: "Composed comment", + context: { anchor: { kind: "document" } }, + }), + ); + }); + + it("shows an empty state pointing at the artifact surfaces", () => { + mocks.comments = []; + + render(); + + expect(screen.getByText("No open comments")).toBeTruthy(); + }); +}); diff --git a/products/desktop/packages/ui/src/features/canvas/components/TaskCommentsList.tsx b/products/desktop/packages/ui/src/features/canvas/components/TaskCommentsList.tsx new file mode 100644 index 000000000000..4f08d147baaf --- /dev/null +++ b/products/desktop/packages/ui/src/features/canvas/components/TaskCommentsList.tsx @@ -0,0 +1,723 @@ +import { + CaretDownIcon, + ChatCircleIcon, + FunnelSimpleIcon, + GitPullRequestIcon, +} from "@phosphor-icons/react"; +import type { ResourceComment } from "@posthog/api-client/posthog-client"; +import type { ThreadTimelineRow } from "@posthog/core/canvas/threadTimeline"; +import { commentTargetKey } from "@posthog/core/comments/anchors"; +import { + Button, + DropdownMenu, + DropdownMenuContent, + DropdownMenuRadioGroup, + DropdownMenuRadioItem, + DropdownMenuTrigger, + Empty, + EmptyDescription, + EmptyHeader, + EmptyMedia, + EmptyTitle, + Spinner, +} from "@posthog/quill"; +import type { + Task, + TaskThreadMessage, + UserBasic, +} from "@posthog/shared/domain-types"; +import { iconForTemplate } from "@posthog/ui/features/canvas/components/canvasTemplateIcon"; +import { + buildRows, + type CommentSource, + commentSources, + taskCommentTarget, +} from "@posthog/ui/features/canvas/components/taskArtifactRows"; +import { + byNewestActivity, + prCommentThreads, + resourceCommentThreads, + type SourceKind, + type TaskCommentThread, + threadSourceOptions, +} from "@posthog/ui/features/canvas/components/taskCommentThreads"; +import { useOrgMembers } from "@posthog/ui/features/canvas/hooks/useOrgMembers"; +import { useTaskRuns } from "@posthog/ui/features/canvas/hooks/useTaskRuns"; +import { canvasArtifactOpenHandler } from "@posthog/ui/features/canvas/utils/canvasArtifactNavigation"; +import { usePrCommentActions } from "@posthog/ui/features/code-review/hooks/usePrCommentActions"; +import { openPrInReview } from "@posthog/ui/features/code-review/openPrInReview"; +import { useReviewNavigationStore } from "@posthog/ui/features/code-review/reviewNavigationStore"; +import { usePrTitles } from "@posthog/ui/features/git-interaction/usePrDetails"; +import { + useActiveArtifactId, + usePanelLayoutStore, +} from "@posthog/ui/features/panels/panelLayoutStore"; +import { usePrCommentsForUrls } from "@posthog/ui/features/pr-review/usePrCommentsForUrls"; +import { usePrReviewThreadsForUrls } from "@posthog/ui/features/pr-review/usePrReviewThreadsForUrls"; +import { useCommentNavigationStore } from "@posthog/ui/features/sessions/commentNavigationStore"; +import { CommentComposer } from "@posthog/ui/features/sessions/components/CommentComposer"; +import { CommentThreadCard } from "@posthog/ui/features/sessions/components/CommentThreadCard"; +import type { HighlightResolution } from "@posthog/ui/features/sessions/components/commentViewTypes"; +import { readCommentContext } from "@posthog/ui/features/sessions/components/commentViewTypes"; +import { + isOptimisticComment, + useCommentsForTargetsQuery, + useCommentsQuery, + useCreateComment, + useSetCommentResolved, +} from "@posthog/ui/features/sessions/components/useComments"; +import { FileIcon } from "@posthog/ui/primitives/FileIcon"; +import { useCallback, useEffect, useMemo, useRef, useState } from "react"; + +const EMPTY_COMMENTS: ResourceComment[] = []; +/** The whole task's threads in one request; slower than a single artifact's own + * poll because this one fans out across every resource. */ +const POLL_INTERVAL_MS = 30_000; +const PULSE_MS = 1_200; +const ALL_SOURCES = "all"; + +type StateFilter = "open" | "resolved"; + +/** The icon a source shows wherever it's named — the card label and the + * filter menu — so the two always agree. */ +function sourceIcon(kind: SourceKind, label: string, size = 12) { + switch (kind) { + case "pr": + return ( + + ); + case "canvas": + return iconForTemplate("", { size, className: "text-violet-9" }); + case "task": + return ; + default: + return ; + } +} + +function SourceLabel({ thread }: { thread: TaskCommentThread }) { + const replies = thread.entries.length - 1; + return ( + + {sourceIcon(thread.sourceKind, thread.sourceLabel)} + + {thread.sourceLabel} + + {thread.origin.kind === "pr-review" && ( + + · {thread.origin.filePath.split("/").at(-1)} + + )} + {replies > 0 && ( + + · {replies} {replies === 1 ? "reply" : "replies"} + + )} + + ); +} + +function CommentReference({ + root, + versionLabel, +}: { + root: ResourceComment; + versionLabel?: (versionId: string) => string | null; +}) { + const context = readCommentContext(root); + const version = context?.canvasVersionId + ? versionLabel?.(context.canvasVersionId) + : null; + const anchor = context?.anchor; + const quote = anchor?.kind === "text" ? anchor.quote : null; + if (!version && !quote) return null; + return ( + + {version && {version} ·} + {quote && ( + + “{quote}” + + )} + + ); +} + +/** + * A PostHog comment thread. Its own component so it can hold the mutations for + * its thread's resource — the list spans several, each with its own target. + */ +function ResourceThreadRow({ + thread, + source, + root, + taskId, + members, + selected, + pulsing, + resolution, + onOpen, + showSource = true, + commentVersionLabel, +}: { + thread: TaskCommentThread; + source: CommentSource; + root: ResourceComment; + taskId: string; + members: UserBasic[]; + selected: boolean; + pulsing: boolean; + resolution?: HighlightResolution; + onOpen: () => void; + showSource?: boolean; + commentVersionLabel?: (versionId: string) => string | null; +}) { + const createComment = useCreateComment(source.target, taskId); + const setResolved = useSetCommentResolved(source.target); + const rootPending = isOptimisticComment(root); + + return ( + + {showSource && } + + + } + onSelect={onOpen} + canReply={!rootPending} + canResolve={!rootPending} + onReply={async (content, mentions) => { + await createComment.mutateAsync({ + content, + sourceCommentId: root.id, + context: readCommentContext(root) ?? { anchor: { kind: "document" } }, + mentions, + }); + }} + onResolve={(resolved) => setResolved.mutate({ root, resolved })} + /> + ); +} + +/** A GitHub thread. Reply and resolve go to GitHub, not to PostHog. */ +function PrThreadRow({ + thread, + selected, + pulsing, + onOpen, +}: { + thread: TaskCommentThread; + selected: boolean; + pulsing: boolean; + onOpen: () => void; +}) { + const origin = thread.origin; + const prUrl = origin.kind === "resource" ? null : origin.prUrl; + const { reply, resolve } = usePrCommentActions(prUrl); + const [busy, setBusy] = useState(false); + + const run = async (action: () => Promise) => { + setBusy(true); + try { + if (!(await action())) throw new Error("GitHub comment action failed"); + } finally { + setBusy(false); + } + }; + + return ( + } + // Only inline review threads accept replies and resolution; conversation + // comments are read here and linked out to GitHub to act on. + canReply={origin.kind === "pr-review"} + canResolve={origin.kind === "pr-review"} + viewHref={origin.kind === "pr-conversation" ? origin.url : undefined} + onSelect={onOpen} + onReply={(content) => + run(() => + origin.kind === "pr-review" + ? reply(origin.rootCommentId, content) + : Promise.resolve(false), + ) + } + onResolve={(resolved) => + run(() => + origin.kind === "pr-review" + ? resolve(origin.threadNodeId, resolved) + : Promise.resolve(false), + ) + } + /> + ); +} + +/** + * Every comment thread on the task: its artifacts, its canvases, its pull + * requests, and the task itself. Selecting one opens where it lives and locates + * it there, which is why no surface carries a thread list of its own. + */ +export function TaskCommentsList({ + task, + timeline, + onlySource, + canvasVersionId, + commentVersionLabel, + onCanvasCommentOpen, +}: { + task: Task; + timeline: ThreadTimelineRow[]; + /** Restricts the pane to one resource known by its host, without relying on + * the task timeline to rediscover it. */ + onlySource?: CommentSource; + canvasVersionId?: string | null; + commentVersionLabel?: (versionId: string) => string | null; + onCanvasCommentOpen?: (versionId: string | null) => void; +}) { + const { runs } = useTaskRuns(onlySource ? undefined : task.id); + const { members } = useOrgMembers(); + const openArtifactTab = usePanelLayoutStore((state) => state.openArtifactTab); + const activeArtifactId = useActiveArtifactId(task.id); + const requestCommentFocus = useCommentNavigationStore( + (state) => state.requestCommentFocus, + ); + const focus = useCommentNavigationStore( + (state) => state.focusByTask[task.id], + ); + const resolutionsByTarget = useCommentNavigationStore( + (state) => state.resolutionsByTarget, + ); + const [stateFilter, setStateFilter] = useState("open"); + const [sourceFilter, setSourceFilter] = useState(ALL_SOURCES); + const [pulseThreadId, setPulseThreadId] = useState(null); + const [draft, setDraft] = useState(""); + const sourceFilterTouched = useRef(false); + const previousTaskId = useRef(task.id); + + useEffect(() => { + if (previousTaskId.current === task.id) return; + previousTaskId.current = task.id; + sourceFilterTouched.current = false; + setSourceFilter(ALL_SOURCES); + setDraft(""); + }, [task.id]); + + const rows = useMemo( + () => buildRows(task, timeline, runs), + [task, timeline, runs], + ); + const sources = useMemo( + () => (onlySource ? [onlySource] : commentSources(task.id, rows)), + [task.id, rows, onlySource], + ); + const targets = useMemo( + () => sources.map((source) => source.target), + [sources], + ); + const singleSourceComments = useCommentsQuery( + onlySource?.target ?? null, + task.id, + ); + const taskComments = useCommentsForTargetsQuery( + onlySource ? [] : targets, + task.id, + { + live: true, + intervalMs: POLL_INTERVAL_MS, + }, + ); + const commentsQuery = onlySource ? singleSourceComments : taskComments; + const prUrls = useMemo( + () => + onlySource + ? [] + : rows.flatMap((row) => (row.kind === "pr" ? [row.url] : [])), + [rows, onlySource], + ); + const prConversation = usePrCommentsForUrls(prUrls); + const prReviews = usePrReviewThreadsForUrls(prUrls); + const prTitles = usePrTitles(prUrls); + + const taskTarget = useMemo(() => taskCommentTarget(task.id), [task.id]); + const composerTarget = onlySource?.target ?? taskTarget; + const composerSourceKey = commentTargetKey(composerTarget); + const createComment = useCreateComment(composerTarget, task.id); + + const threads = useMemo(() => { + const reviewByUrl = new Map(prReviews.byUrl); + const conversationByUrl = new Map(prConversation.byUrl); + const resourceThreads = resourceCommentThreads( + commentsQuery.data ?? EMPTY_COMMENTS, + sources, + ); + const prThreads = prUrls.flatMap((prUrl) => + prCommentThreads( + prUrl, + prTitles[prUrl] ?? `PR #${prUrl.split("/").at(-1)}`, + reviewByUrl.get(prUrl) ?? [], + conversationByUrl.get(prUrl) ?? [], + ), + ); + return [...resourceThreads, ...prThreads].sort(byNewestActivity); + }, [ + commentsQuery.data, + sources, + prUrls, + prTitles, + prReviews.byUrl, + prConversation.byUrl, + ]); + + // Every source that could ever hold a thread, whether or not it has one yet. + // Validating against this rather than the loaded threads lets the filter + // follow an artifact whose comments haven't arrived, and lets the task and + // PR sources stay selectable while empty. + const knownSourceKeys = useMemo(() => { + const keys = new Set( + sources.map((source) => commentTargetKey(source.target)), + ); + for (const prUrl of prUrls) keys.add(prUrl); + return keys; + }, [sources, prUrls]); + const stateFilteredThreads = useMemo( + () => + threads.filter( + (thread) => thread.resolved === (stateFilter === "resolved"), + ), + [stateFilter, threads], + ); + const sourceOptions = useMemo( + () => + threadSourceOptions(stateFilteredThreads, [ + ...sources.map((source) => ({ + key: commentTargetKey(source.target), + label: source.name, + kind: source.kind, + })), + ...prUrls.map((prUrl) => ({ + key: prUrl, + label: prTitles[prUrl] ?? `PR #${prUrl.split("/").at(-1)}`, + kind: "pr" as const, + })), + ]), + [prTitles, prUrls, sources, stateFilteredThreads], + ); + const effectiveSourceFilter = + sourceFilter === ALL_SOURCES || knownSourceKeys.has(sourceFilter) + ? sourceFilter + : ALL_SOURCES; + const sourceLabel = + effectiveSourceFilter === ALL_SOURCES + ? "All sources" + : (sourceOptions.find((option) => option.key === effectiveSourceFilter) + ?.label ?? "All sources"); + + useEffect(() => { + if (sourceFilter !== effectiveSourceFilter) { + setSourceFilter(effectiveSourceFilter); + } + }, [effectiveSourceFilter, sourceFilter]); + + // Follow the artifact on screen until the reader picks a source themselves; + // after that the filter is theirs, not the pane's. + useEffect(() => { + if (onlySource || sourceFilterTouched.current) return; + setSourceFilter( + activeArtifactId + ? commentTargetKey({ scope: "task_artifact", itemId: activeArtifactId }) + : ALL_SOURCES, + ); + }, [activeArtifactId, onlySource]); + + const inSource = (thread: TaskCommentThread) => + effectiveSourceFilter === ALL_SOURCES || + thread.sourceKey === effectiveSourceFilter; + const scoped = threads.filter(inSource); + const openCount = scoped.filter((thread) => !thread.resolved).length; + const resolvedCount = scoped.length - openCount; + const visibleThreads = scoped.filter((thread) => + stateFilteredThreads.includes(thread), + ); + + const openThread = useCallback( + (thread: TaskCommentThread, requestThreadFocus = true) => { + const origin = thread.origin; + if (origin.kind === "pr-review" || origin.kind === "pr-conversation") { + openPrInReview(task.id, origin.prUrl); + if (origin.kind === "pr-review") { + // The review pane scrolls by file; a specific comment is as close as it + // gets until it grows a per-thread target. + useReviewNavigationStore + .getState() + .requestScrollToFile(task.id, origin.filePath); + } + return; + } + const { source, root } = origin; + if (source.kind === "canvas") { + if (requestThreadFocus) { + requestCommentFocus(task.id, source.target, root.id); + } + if (onCanvasCommentOpen) { + onCanvasCommentOpen( + readCommentContext(root)?.canvasVersionId ?? null, + ); + return; + } + canvasArtifactOpenHandler(source.url)?.(); + return; + } + // A thread on the task itself has nowhere else to open because it lives here. + if (source.kind === "task" || !source.runId) return; + openArtifactTab(task.id, { + runId: source.runId, + artifactId: source.target.itemId, + name: source.name, + }); + if (requestThreadFocus) { + requestCommentFocus(task.id, source.target, root.id); + } + }, + [onCanvasCommentOpen, openArtifactTab, requestCommentFocus, task.id], + ); + + // A thread picked on the artifact itself has to surface here, even when a + // filter is hiding it. Each request is honoured once, by nonce: resolving the + // focused thread later must not drag the filters along with it. + const focusedThreadId = focus?.threadId ?? null; + const handledFocusRef = useRef(null); + useEffect(() => { + const focusKey = focus ? `${task.id}:${focus.nonce}` : null; + if (!focus || handledFocusRef.current === focusKey) return; + const focused = threads.find((thread) => thread.id === focus.threadId); + // The thread may still be loading, so wait rather than guess its filters. + if (!focused) return; + handledFocusRef.current = focusKey; + setStateFilter(focused.resolved ? "resolved" : "open"); + setSourceFilter((current) => + current === ALL_SOURCES || current === focused.sourceKey + ? current + : ALL_SOURCES, + ); + setPulseThreadId(focus.threadId); + openThread(focused, false); + requestAnimationFrame(() => { + document + .querySelector( + `[data-comment-thread-id="${CSS.escape(focus.threadId)}"]`, + ) + ?.scrollIntoView({ behavior: "smooth", block: "nearest" }); + }); + }, [focus, openThread, threads, task.id]); + // The pulse fades on its own; owning the timer in its own effect keeps it + // cleaned up on the next pulse or on unmount, without a stray ref. + useEffect(() => { + if (!pulseThreadId) return; + const timer = setTimeout(() => setPulseThreadId(null), PULSE_MS); + return () => clearTimeout(timer); + }, [pulseThreadId]); + + const loading = + commentsQuery.isLoading || prConversation.isLoading || prReviews.isLoading; + const loadFailed = + commentsQuery.isError || prConversation.isError || prReviews.isError; + + return ( + // The parent scrolls the middle; the filters and the composer are pinned so + // they stay reachable however long the thread list grows. +
+
+ {!onlySource && ( + + + {sourceLabel} + + + } + /> + {/* Wide, single-line rows: the label truncates at the end (with the + full name on hover) and the count is pinned right with the shared + ml-auto idiom, so a long PR title stays legible and aligned. */} + + { + sourceFilterTouched.current = true; + setSourceFilter(value); + }} + > + + + All sources + + {stateFilteredThreads.length} + + + {sourceOptions.map((option) => ( + + {sourceIcon(option.kind, option.label)} + {option.label} + + {option.count} + + + ))} + + + + )} + + + {stateFilter === "open" ? "Open" : "Resolved"} + + + } + /> + + setStateFilter(value as StateFilter)} + > + + Open ({openCount}) + + + Resolved ({resolvedCount}) + + + + +
+
+ {loadFailed ? ( + + + + + + Couldn't load comments + + Refresh the page to try again. + + + + ) : loading && threads.length === 0 ? ( +
+ +
+ ) : visibleThreads.length === 0 ? ( + + + + + + + No {stateFilter === "open" ? "open" : "resolved"} comments + + + {stateFilter === "open" + ? onlySource + ? "Comment on this canvas to start a thread." + : "Comment on the task below, or open an artifact and select text to start a thread there." + : "Resolved threads will appear here."} + + + + ) : ( + visibleThreads.map((thread) => + thread.origin.kind === "resource" ? ( + openThread(thread)} + showSource={!onlySource} + commentVersionLabel={commentVersionLabel} + /> + ) : ( + openThread(thread)} + /> + ), + ) + )} +
+
+ { + await createComment.mutateAsync({ + content, + context: { + anchor: { kind: "document" }, + ...(canvasVersionId ? { canvasVersionId } : {}), + }, + mentions, + }); + setDraft(""); + // Show the thread that was just opened: open state, and a source + // filter that isn't hiding the task's own comments. + setStateFilter("open"); + if ( + sourceFilter !== ALL_SOURCES && + sourceFilter !== composerSourceKey + ) { + setSourceFilter(ALL_SOURCES); + } + }} + members={members} + placeholder={`Comment on this ${onlySource ? "canvas" : "task"}… Type @ to mention someone`} + rows={2} + disabled={createComment.isPending} + /> +
+
+ ); +} diff --git a/products/desktop/packages/ui/src/features/canvas/components/WebsiteLayout.test.tsx b/products/desktop/packages/ui/src/features/canvas/components/WebsiteLayout.test.tsx index a0017fb932c8..f81e49815b74 100644 --- a/products/desktop/packages/ui/src/features/canvas/components/WebsiteLayout.test.tsx +++ b/products/desktop/packages/ui/src/features/canvas/components/WebsiteLayout.test.tsx @@ -1,19 +1,25 @@ import { Theme } from "@radix-ui/themes"; -import { render, screen } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { fireEvent, render, screen } from "@testing-library/react"; import { describe, expect, it, vi } from "vitest"; vi.mock("@posthog/ui/features/canvas/hooks/useChannelsLayout", () => ({ useChannelsLayout: () => false, })); +vi.mock("@posthog/host-router/react", () => ({ + useHostTRPC: () => ({ + dashboards: { saveContext: { mutationKey: () => ["save-context"] } }, + }), +})); -const { useChannelTasks, useParams, usePathname, useTasks } = vi.hoisted( - () => ({ +const { useChannelTasks, useDashboard, useParams, usePathname, useTasks } = + vi.hoisted(() => ({ useChannelTasks: vi.fn(), + useDashboard: vi.fn(), useParams: vi.fn(), usePathname: vi.fn(), useTasks: vi.fn(), - }), -); + })); vi.mock("@tanstack/react-router", () => ({ Outlet: () => null, @@ -45,8 +51,35 @@ vi.mock("@posthog/ui/features/canvas/hooks/useChannels", () => ({ }), })); vi.mock("@posthog/ui/features/canvas/hooks/useDashboards", () => ({ - useDashboard: () => ({ dashboard: undefined }), + useDashboard, useDashboardMutations: () => ({}), + useCanvasVersions: () => ({ versions: [{ taskId: "version-task" }] }), +})); +vi.mock("@posthog/ui/features/sessions/components/useComments", () => ({ + useCommentsQuery: () => ({ + data: [ + { + id: "comment-1", + created_at: "2026-01-01T00:00:00Z", + content: "First", + item_id: "canvas-1", + item_context: { anchor: { kind: "document" } }, + scope: "desktop_canvas", + source_comment: null, + completed_at: null, + }, + { + id: "comment-2", + created_at: "2026-01-01T00:01:00Z", + content: "Second", + item_id: "canvas-1", + item_context: { anchor: { kind: "document" } }, + scope: "desktop_canvas", + source_comment: null, + completed_at: null, + }, + ], + }), })); vi.mock("@posthog/ui/features/canvas/stores/dashboardEditStore", () => ({ useDashboardEditStore: (sel: (s: unknown) => unknown) => @@ -60,6 +93,7 @@ vi.mock("@posthog/ui/features/canvas/freeform/CanvasFrameHost", () => ({ CanvasFrameHost: () => null, })); +import { useCanvasChatPanelStore } from "@posthog/ui/features/canvas/stores/canvasChatPanelStore"; import { useHeaderStore } from "@posthog/ui/shell/headerStore"; import { WebsiteLayout } from "./WebsiteLayout"; @@ -68,11 +102,17 @@ function renderLayout({ params, tasks = [{ id: "task-1", title: "Fix the bug" }], channelTaskIds = tasks.map((task) => task.id), + dashboard, }: { pathname: string; params: Record; tasks?: { id: string; title: string }[]; channelTaskIds?: string[]; + dashboard?: { + name: string; + templateId: string; + generationTaskId: string | null; + }; }) { usePathname.mockReturnValue(pathname); useParams.mockReturnValue(params); @@ -81,15 +121,46 @@ function renderLayout({ tasks: channelTaskIds.map((taskId) => ({ taskId })), isLoading: false, }); + useDashboard.mockReturnValue({ dashboard }); useHeaderStore.setState({ content: crumb }); render( - - - , + + + + + , ); } describe("WebsiteLayout task header actions", () => { + it.each([ + ["while generating", "task-1"], + ["after generation", null], + ])( + "shows the comment count and opens comments %s", + (_label, generationTaskId) => { + renderLayout({ + pathname: "/website/chan-1/dashboards/canvas-1", + params: { channelId: "chan-1", dashboardId: "canvas-1" }, + dashboard: { + name: "Launch", + templateId: "freeform", + generationTaskId, + }, + }); + + expect( + screen.getByRole("button", { name: /Comments/ }), + ).toHaveTextContent("Comments2"); + useCanvasChatPanelStore.setState({ collapsed: true, tab: "chat" }); + fireEvent.click(screen.getByRole("button", { name: /Comments/ })); + expect(useCanvasChatPanelStore.getState()).toMatchObject({ + collapsed: false, + tab: "comments", + }); + }, + ); + it("renders the task action row on a channel task detail", () => { renderLayout({ pathname: "/website/chan-1/tasks/task-1", diff --git a/products/desktop/packages/ui/src/features/canvas/components/WebsiteLayout.tsx b/products/desktop/packages/ui/src/features/canvas/components/WebsiteLayout.tsx index 172df777217a..e517f7473fda 100644 --- a/products/desktop/packages/ui/src/features/canvas/components/WebsiteLayout.tsx +++ b/products/desktop/packages/ui/src/features/canvas/components/WebsiteLayout.tsx @@ -1,5 +1,6 @@ import { ArrowClockwiseIcon, + ChatCircleIcon, DotsThreeIcon, LinkIcon, PencilSimpleIcon, @@ -34,14 +35,22 @@ import { useChannels } from "@posthog/ui/features/canvas/hooks/useChannels"; import { useChannelsLayout } from "@posthog/ui/features/canvas/hooks/useChannelsLayout"; import { useChannelTasks } from "@posthog/ui/features/canvas/hooks/useChannelTasks"; import { + useCanvasVersions, useDashboard, useDashboardMutations, } from "@posthog/ui/features/canvas/hooks/useDashboards"; +import { useCanvasChatPanelStore } from "@posthog/ui/features/canvas/stores/canvasChatPanelStore"; import { useDashboardEditStore, useIsDashboardEditing, } from "@posthog/ui/features/canvas/stores/dashboardEditStore"; import { copyCanvasLink } from "@posthog/ui/features/canvas/utils/copyCanvasLink"; +import { buildCommentThreads } from "@posthog/ui/features/sessions/components/commentViewTypes"; +import { useCommentsQuery } from "@posthog/ui/features/sessions/components/useComments"; +import { + MentionAvailabilityProvider, + PRIVATE_SPACE_MENTIONS_DISABLED, +} from "@posthog/ui/features/sessions/mentionAvailability"; import { TaskHeaderActions } from "@posthog/ui/features/task-detail/components/TaskHeaderActions"; import { useTasks } from "@posthog/ui/features/tasks/useTasks"; import { toast } from "@posthog/ui/primitives/toast"; @@ -278,8 +287,26 @@ function CanvasBreadcrumb({ trailing?: ReactNode; }) { const { dashboard } = useDashboard(dashboardId); + const { versions } = useCanvasVersions(dashboardId); const { renameDashboard } = useDashboardMutations(); + const openComments = useCanvasChatPanelStore((state) => state.openComments); const name = dashboard?.name ?? "Canvas"; + const commentTarget = { + scope: "desktop_canvas" as const, + itemId: dashboardId, + }; + const commentTaskId = + dashboard?.generationTaskId ?? + versions.find((version) => version.taskId)?.taskId ?? + null; + const comments = useCommentsQuery( + commentTaskId ? commentTarget : null, + commentTaskId ?? "", + { live: true }, + ); + const openCommentCount = buildCommentThreads(comments.data ?? []).filter( + (thread) => !thread.resolved, + ).length; return ( void renameDashboard(dashboardId, next)} - trailing={trailing} + trailing={ + <> + {commentTaskId && ( + + )} + {trailing} + + } /> ); } @@ -325,6 +365,11 @@ export function WebsiteLayout() { : undefined; const { channels } = useChannels(); + const mentionsDisabledReason = + channels.find((channel) => channel.id === channelId)?.channelType === + "personal" + ? PRIVATE_SPACE_MENTIONS_DISABLED + : null; const channelName = channelId ? (channels.find((c) => c.id === channelId)?.name ?? (spacesLayout ? "Space" : "Channel")) @@ -397,7 +442,9 @@ export function WebsiteLayout() { )} - + + + {/* Warm-iframe pool for canvases. Mounted once here so it persists across every in-space navigation; overlays itself onto the active canvas's diff --git a/products/desktop/packages/ui/src/features/canvas/components/activityFeed.ts b/products/desktop/packages/ui/src/features/canvas/components/activityFeed.ts index 8cb1d13cd415..a0867a55d56b 100644 --- a/products/desktop/packages/ui/src/features/canvas/components/activityFeed.ts +++ b/products/desktop/packages/ui/src/features/canvas/components/activityFeed.ts @@ -10,6 +10,7 @@ export function activityReadPayload(items: TaskActivityItem[]) { return items.map((item) => ({ task_id: item.taskId, seen_before: item.activityAt, + ...(item.commentId ? { activity_id: item.id } : {}), })); } diff --git a/products/desktop/packages/ui/src/features/canvas/components/taskArtifactRows.ts b/products/desktop/packages/ui/src/features/canvas/components/taskArtifactRows.ts new file mode 100644 index 000000000000..a44423b591ff --- /dev/null +++ b/products/desktop/packages/ui/src/features/canvas/components/taskArtifactRows.ts @@ -0,0 +1,199 @@ +import { + OUTPUT_ARTIFACT_TYPES, + parseRunArtifacts, + type RunArtifact, +} from "@posthog/core/canvas/runArtifactSchemas"; +import type { ThreadTimelineRow } from "@posthog/core/canvas/threadTimeline"; +import { + type CommentTarget, + commentTargetKey, +} from "@posthog/core/comments/anchors"; +import { readPrUrls } from "@posthog/shared"; +import type { + Task, + TaskRun, + TaskThreadMessage, +} from "@posthog/shared/domain-types"; +import { parseHttpsUrl, parseShareLink } from "@posthog/ui/utils/posthogLinks"; + +export type ArtifactRow = + | { kind: "pr"; key: string; url: string } + | { + kind: "canvas"; + key: string; + name: string; + url: string | null; + /** The canvas row id, the stable comment target (never the name). */ + dashboardId: string | null; + } + | { + kind: "file"; + key: string; + artifactId: string | null; + name: string; + runId: string | null; + } + | { kind: "slack"; key: string; url: string }; + +/** + * Somewhere a task's comment threads live. Artifacts and canvases come from the + * task's rows; the task itself is always one, holding the threads that belong + * to the work rather than to any single deliverable. + */ +export type CommentSource = + | { kind: "file"; target: CommentTarget; name: string; runId: string | null } + | { kind: "canvas"; target: CommentTarget; name: string; url: string | null } + | { kind: "task"; target: CommentTarget; name: string }; + +export function taskCommentTarget(taskId: string): CommentTarget { + return { scope: "task", itemId: taskId }; +} + +export function commentSources( + taskId: string, + rows: ArtifactRow[], +): CommentSource[] { + const sources: CommentSource[] = [ + { kind: "task", target: taskCommentTarget(taskId), name: "This task" }, + ]; + const seen = new Set(); + for (const row of rows) { + const target = targetForRow(row); + if (!target || seen.has(commentTargetKey(target))) continue; + seen.add(commentTargetKey(target)); + if (row.kind === "file") { + sources.push({ kind: "file", target, name: row.name, runId: row.runId }); + } else if (row.kind === "canvas") { + sources.push({ kind: "canvas", target, name: row.name, url: row.url }); + } + } + return sources; +} + +/** The canvas's stable row id, recovered from its share link. */ +function canvasDashboardId(url: string | null): string | null { + if (!url) return null; + const parsed = parseHttpsUrl(url); + const target = parsed ? parseShareLink(parsed.href) : null; + if (target?.kind === "canvas") return target.dashboardId; + + // Local development emits http:// canvas links, which are deliberately not + // valid external share links. Recover only the exact route's final id here; + // this value is used for an access-checked API query, never for navigation. + try { + const localUrl = new URL(url); + if (localUrl.protocol !== "http:") return null; + const segments = localUrl.pathname.split("/").filter(Boolean); + if ( + segments.length === 4 && + segments[0] === "code" && + segments[1] === "canvas" + ) { + return decodeURIComponent(segments[3]); + } + } catch { + return null; + } + return null; +} + +/** Where a row's comments live, or null when the row can't carry any. */ +function targetForRow(row: ArtifactRow): CommentTarget | null { + if (row.kind === "file" && row.artifactId) { + return { scope: "task_artifact", itemId: row.artifactId }; + } + if (row.kind === "canvas" && row.dashboardId) { + return { scope: "desktop_canvas", itemId: row.dashboardId }; + } + return null; +} + +/** + * Every commentable resource this task produced, once each. Artifacts and + * canvases share the generic comments API, differing only by scope, so a pane + * can hold one query over all of them — and two timeline messages naming the + * same canvas must not fetch it twice. + */ +export function commentTargets(rows: ArtifactRow[]): CommentTarget[] { + const byKey = new Map(); + for (const row of rows) { + const target = targetForRow(row); + if (target) byKey.set(commentTargetKey(target), target); + } + return [...byKey.values()]; +} + +function readRunOutputs(run: TaskRun): RunArtifact[] { + return parseRunArtifacts( + (run as { artifacts?: unknown }).artifacts, + OUTPUT_ARTIFACT_TYPES, + ); +} + +export function buildRows( + task: Task, + timeline: ThreadTimelineRow[], + runs: TaskRun[], +): ArtifactRow[] { + const rows: ArtifactRow[] = []; + const seenPrUrls = new Set(); + + const addPr = (url: string, key: string) => { + if (seenPrUrls.has(url)) return; + seenPrUrls.add(url); + rows.push({ kind: "pr", key, url }); + }; + + for (const row of timeline) { + if (row.kind !== "artifact") continue; + if (row.artifact.kind === "pr") { + addPr(row.artifact.url, row.message.id); + } else { + const url = row.artifact.url; + rows.push({ + kind: "canvas", + key: row.message.id, + name: row.artifact.name, + url, + dashboardId: canvasDashboardId(url), + }); + } + } + + const allRuns = + runs.length > 0 ? runs : task.latest_run ? [task.latest_run] : []; + + // Re-uploading a file replaces it rather than adding a second one: agents + // revise a deliverable and upload it again under the same name, so keeping + // every copy would bury the current one under its own drafts. + const newestByName = new Map(); + for (const run of allRuns) { + for (const outputPr of readPrUrls(run.output)) { + addPr(outputPr, `output-pr:${outputPr}`); + } + for (const file of readRunOutputs(run)) { + if (!file.name) continue; + const previous = newestByName.get(file.name); + const isNewer = + !previous || + (file.uploaded_at ?? "") >= (previous.file.uploaded_at ?? ""); + if (isNewer) newestByName.set(file.name, { file, runId: run.id }); + } + } + for (const [name, { file, runId }] of newestByName) { + rows.push({ + kind: "file", + key: `file:${file.id ?? file.storage_path ?? name}`, + artifactId: file.id ?? null, + name, + runId, + }); + } + + const slackUrl = task.latest_run?.state?.slack_thread_url; + if (typeof slackUrl === "string" && slackUrl) { + rows.push({ kind: "slack", key: "slack-thread", url: slackUrl }); + } + + return rows; +} diff --git a/products/desktop/packages/ui/src/features/canvas/components/taskCommentThreads.test.ts b/products/desktop/packages/ui/src/features/canvas/components/taskCommentThreads.test.ts new file mode 100644 index 000000000000..624d06996929 --- /dev/null +++ b/products/desktop/packages/ui/src/features/canvas/components/taskCommentThreads.test.ts @@ -0,0 +1,260 @@ +import type { ResourceComment } from "@posthog/api-client/posthog-client"; +import type { PrConversationComment, PrReviewThread } from "@posthog/shared"; +import { describe, expect, it } from "vitest"; +import type { CommentSource } from "./taskArtifactRows"; +import { + byNewestActivity, + prCommentThreads, + resourceCommentThreads, + threadSourceOptions, +} from "./taskCommentThreads"; + +const fileSource: CommentSource = { + kind: "file", + target: { scope: "task_artifact", itemId: "a" }, + name: "report.md", + runId: "run-1", +}; +const taskSource: CommentSource = { + kind: "task", + target: { scope: "task", itemId: "task-1" }, + name: "This task", +}; + +function comment(overrides: Partial): ResourceComment { + return { + id: "c1", + created_by: null, + content: "hi", + created_at: "2024-01-01T00:00:00Z", + item_id: "a", + item_context: { anchor: { kind: "document" } }, + scope: "task_artifact", + source_comment: null, + ...overrides, + } as ResourceComment; +} + +describe("resourceCommentThreads", () => { + it("keeps only threads whose resource is present, tagged with it", () => { + const threads = resourceCommentThreads( + [ + comment({ id: "c1", item_id: "a", content: "root" }), + comment({ + id: "r1", + item_id: "a", + source_comment: "c1", + content: "reply", + created_at: "2024-01-01T00:01:00Z", + }), + comment({ id: "orphan", item_id: "gone", content: "no source" }), + ], + [fileSource, taskSource], + ); + + expect(threads).toHaveLength(1); + expect(threads[0].sourceKind).toBe("file"); + expect(threads[0].sourceLabel).toBe("report.md"); + expect(threads[0].entries.map((entry) => entry.body)).toEqual([ + "root", + "reply", + ]); + }); + + // A resolve/reopen reply is thread state, not something anyone said. + it("drops thread-state replies from the visible entries", () => { + const threads = resourceCommentThreads( + [ + comment({ id: "c1", content: "root" }), + comment({ + id: "state", + source_comment: "c1", + content: "Resolved this thread", + created_at: "2024-01-01T00:02:00Z", + item_context: { + anchor: { kind: "document" }, + threadState: "resolved", + }, + }), + ], + [fileSource], + ); + + expect(threads[0].resolved).toBe(true); + expect(threads[0].entries).toHaveLength(1); + }); +}); + +describe("prCommentThreads", () => { + const reviewThread: PrReviewThread = { + nodeId: "node-1", + isResolved: true, + rootId: 501, + filePath: "src/App.tsx", + comments: [ + { + id: 501, + body: "root", + path: "src/App.tsx", + line: 3, + original_line: null, + side: "RIGHT", + start_line: null, + start_side: null, + diff_hunk: "", + user: { login: "octo", avatar_url: "http://x/a.png" }, + created_at: "2024-01-01T00:00:00Z", + updated_at: "2024-01-01T00:00:00Z", + subject_type: "line", + }, + { + id: 502, + body: "reply", + path: "src/App.tsx", + line: 3, + original_line: null, + side: "RIGHT", + start_line: null, + start_side: null, + diff_hunk: "", + user: { login: "octo", avatar_url: "" }, + created_at: "2024-01-01T00:05:00Z", + updated_at: "2024-01-01T00:05:00Z", + subject_type: "line", + }, + ], + }; + const conversation: PrConversationComment = { + id: 900, + author: "octo", + avatarUrl: null, + body: "lgtm", + createdAt: "2024-01-02T00:00:00Z", + url: "https://github.com/a/b/pull/7#c", + }; + + it("carries review-thread replies, resolution and the reply/resolve ids", () => { + const [thread] = prCommentThreads("url", "PR #7", [reviewThread], []); + + expect(thread.entries.map((entry) => entry.body)).toEqual([ + "root", + "reply", + ]); + expect(thread.resolved).toBe(true); + expect(thread.origin).toMatchObject({ + kind: "pr-review", + rootCommentId: 501, + threadNodeId: "node-1", + filePath: "src/App.tsx", + }); + expect(thread.lastActivityAt).toBe("2024-01-01T00:05:00Z"); + }); + + it("makes each conversation comment its own unresolvable thread", () => { + const [thread] = prCommentThreads("url", "PR #7", [], [conversation]); + + expect(thread.entries).toHaveLength(1); + expect(thread.resolved).toBe(false); + expect(thread.origin.kind).toBe("pr-conversation"); + }); + + it("omits GitHub bot comments without hiding human threads", () => { + const botRoot = { + ...reviewThread, + nodeId: "bot-root", + comments: reviewThread.comments.map((comment) => ({ + ...comment, + user: { ...comment.user, isBot: true }, + })), + }; + const humanRootWithBotReply = { + ...reviewThread, + nodeId: "human-root", + comments: [ + reviewThread.comments[0], + { + ...reviewThread.comments[1], + user: { ...reviewThread.comments[1].user, isBot: true }, + }, + ], + }; + + const threads = prCommentThreads( + "url", + "PR #7", + [botRoot, humanRootWithBotReply], + [conversation, { ...conversation, id: 901, isBot: true }], + ); + + expect(threads).toHaveLength(2); + expect( + threads.map((thread) => thread.entries.map((entry) => entry.body)), + ).toEqual([["root"], ["lgtm"]]); + }); +}); + +describe("threadSourceOptions / byNewestActivity", () => { + it("lists each source once and sorts newest first", () => { + const threads = [ + ...resourceCommentThreads( + [comment({ id: "c1", content: "old" })], + [fileSource], + ), + ...prCommentThreads( + "url", + "PR #7", + [], + [ + { + id: 1, + author: "octo", + avatarUrl: null, + body: "new", + createdAt: "2025-01-01T00:00:00Z", + url: null, + }, + ], + ), + ].sort(byNewestActivity); + + expect(threads[0].entries[0].body).toBe("new"); + // Options follow list order, which is newest-first, and carry the kind so + // the filter can show a matching icon. + expect( + threadSourceOptions(threads).map((option) => [option.label, option.kind]), + ).toEqual([ + ["PR #7", "pr"], + ["report.md", "file"], + ]); + }); + + // The task is the one source every task has, so it sits at the top of the + // filter regardless of when it was last touched. + it("pins the task source first, keeping the rest newest-first", () => { + const threads = [ + ...resourceCommentThreads( + [ + comment({ + id: "f1", + item_id: "a", + content: "file", + created_at: "2025-01-01T00:00:00Z", + }), + comment({ + id: "t1", + item_id: "task-1", + scope: "task", + content: "task", + created_at: "2024-01-01T00:00:00Z", + }), + ], + [fileSource, taskSource], + ), + ].sort(byNewestActivity); + + expect(threadSourceOptions(threads).map((option) => option.kind)).toEqual([ + "task", + "file", + ]); + }); +}); diff --git a/products/desktop/packages/ui/src/features/canvas/components/taskCommentThreads.ts b/products/desktop/packages/ui/src/features/canvas/components/taskCommentThreads.ts new file mode 100644 index 000000000000..0e59ed6272b1 --- /dev/null +++ b/products/desktop/packages/ui/src/features/canvas/components/taskCommentThreads.ts @@ -0,0 +1,231 @@ +import type { ResourceComment } from "@posthog/api-client/posthog-client"; +import { commentTargetKey } from "@posthog/core/comments/anchors"; +import type { PrConversationComment, PrReviewThread } from "@posthog/shared"; +import type { UserBasic } from "@posthog/shared/domain-types"; +import type { CommentSource } from "@posthog/ui/features/canvas/components/taskArtifactRows"; +import { + buildCommentThreads, + readCommentContext, +} from "@posthog/ui/features/sessions/components/commentViewTypes"; + +export type CommentEntry = { + id: string; + authorName: string; + /** A PostHog author, whose avatar and hue are the ones they have app-wide. */ + user: UserBasic | null; + /** A GitHub author, who only comes with an avatar url. */ + avatarUrl: string | null; + createdAt: string; + body: string; + /** PostHog comments carry @mention markup; GitHub bodies are markdown. */ + format: "mentions" | "markdown"; +}; + +/** How a thread is opened, replied to and resolved — one case per backend. */ +export type ThreadOrigin = + | { + kind: "resource"; + source: CommentSource; + /** The root comment, needed to reply to and resolve the thread. */ + root: ResourceComment; + } + | { + kind: "pr-review"; + prUrl: string; + filePath: string; + /** GitHub replies target a comment id, resolution a thread node id. */ + rootCommentId: number; + threadNodeId: string; + } + | { kind: "pr-conversation"; prUrl: string; url: string | null }; + +/** + * One thread in the task's comment list, whichever system it came from. The + * list renders and sorts these; only replying, resolving and opening still care + * where a thread lives, which is what `origin` carries. + */ +export type TaskCommentThread = { + /** Stable across refetches: the scroll target and React key. */ + id: string; + /** Groups threads for the source filter. */ + sourceKey: string; + sourceLabel: string; + sourceKind: "file" | "canvas" | "task" | "pr"; + entries: CommentEntry[]; + resolved: boolean; + /** Newest comment in the thread, for ordering the list. */ + lastActivityAt: string; + origin: ThreadOrigin; +}; + +function resourceAuthorName(comment: ResourceComment): string { + const user = comment.created_by; + if (!user) return "Unknown user"; + return ( + [user.first_name, user.last_name].filter(Boolean).join(" ") || user.email + ); +} + +function resourceEntry(comment: ResourceComment): CommentEntry { + return { + id: comment.id, + authorName: resourceAuthorName(comment), + user: comment.created_by, + avatarUrl: null, + createdAt: comment.created_at, + body: comment.content ?? "", + format: "mentions", + }; +} + +/** The task's own comment threads, tagged with the resource they belong to. */ +export function resourceCommentThreads( + comments: ResourceComment[], + sources: CommentSource[], +): TaskCommentThread[] { + const byItemId = new Map(); + for (const source of sources) byItemId.set(source.target.itemId, source); + + return buildCommentThreads(comments).flatMap((thread) => { + const source = thread.root.item_id + ? byItemId.get(thread.root.item_id) + : undefined; + if (!source) return []; + // A resolve/reopen reply is thread state, not something anyone said. + const visibleReplies = thread.replies.filter( + (reply) => !readCommentContext(reply)?.threadState, + ); + return [ + { + id: thread.root.id, + sourceKey: commentTargetKey(source.target), + sourceLabel: source.name, + sourceKind: source.kind, + entries: [thread.root, ...visibleReplies].map(resourceEntry), + resolved: thread.resolved, + lastActivityAt: + thread.replies.at(-1)?.created_at ?? thread.root.created_at, + origin: { kind: "resource", source, root: thread.root }, + }, + ]; + }); +} + +/** + * A PR's comments as threads. Inline review threads keep their replies and can + * be resolved; conversation comments (issue chatter, review summaries) are each + * a thread of one, since GitHub gives them neither replies nor resolution. + */ +export function prCommentThreads( + prUrl: string, + prLabel: string, + reviewThreads: PrReviewThread[], + conversation: PrConversationComment[], +): TaskCommentThread[] { + const threads: TaskCommentThread[] = reviewThreads.flatMap((thread) => { + const root = thread.comments[0]; + if (!root) return []; + const humanComments = thread.comments.filter( + (comment) => !comment.user.isBot, + ); + if (humanComments.length === 0) return []; + return [ + { + id: `pr-review-${thread.rootId}`, + sourceKey: prUrl, + sourceLabel: prLabel, + sourceKind: "pr" as const, + entries: humanComments.map((comment) => ({ + id: `pr-comment-${comment.id}`, + authorName: comment.user.login, + user: null, + avatarUrl: comment.user.avatar_url || null, + createdAt: comment.created_at, + body: comment.body, + format: "markdown" as const, + })), + resolved: thread.isResolved, + lastActivityAt: humanComments.at(-1)?.created_at ?? root.created_at, + origin: { + kind: "pr-review" as const, + prUrl, + filePath: thread.filePath, + rootCommentId: thread.rootId, + threadNodeId: thread.nodeId, + }, + }, + ]; + }); + + for (const comment of conversation) { + if (comment.isBot) continue; + threads.push({ + // Conversation items mix issue comments and review summaries, whose ids + // come from different GitHub id spaces — key on the timestamp too. + id: `pr-conversation-${comment.id}-${comment.createdAt}`, + sourceKey: prUrl, + sourceLabel: prLabel, + sourceKind: "pr", + entries: [ + { + id: `pr-conversation-${comment.id}`, + authorName: comment.author, + user: null, + avatarUrl: comment.avatarUrl, + createdAt: comment.createdAt, + body: comment.body, + format: "markdown", + }, + ], + resolved: false, + lastActivityAt: comment.createdAt, + origin: { kind: "pr-conversation", prUrl, url: comment.url }, + }); + } + + return threads; +} + +export function byNewestActivity( + a: TaskCommentThread, + b: TaskCommentThread, +): number { + return b.lastActivityAt.localeCompare(a.lastActivityAt); +} + +export type SourceKind = TaskCommentThread["sourceKind"]; +export type ThreadSourceOption = { + key: string; + label: string; + kind: SourceKind; + count: number; +}; + +/** + * The available sources for the source filter, including those without a + * thread yet. The task itself sits at the top because every task has one. + */ +export function threadSourceOptions( + threads: TaskCommentThread[], + availableSources: Omit[] = [], +): ThreadSourceOption[] { + const byKey = new Map( + availableSources.map((source) => [source.key, { ...source, count: 0 }]), + ); + for (const thread of threads) { + const source = byKey.get(thread.sourceKey); + if (!source) { + byKey.set(thread.sourceKey, { + key: thread.sourceKey, + label: thread.sourceLabel, + kind: thread.sourceKind, + count: 1, + }); + } else { + source.count += 1; + } + } + return [...byKey.values()].sort( + (a, b) => Number(b.kind === "task") - Number(a.kind === "task"), + ); +} diff --git a/products/desktop/packages/ui/src/features/canvas/freeform/BuiltCanvas.test.tsx b/products/desktop/packages/ui/src/features/canvas/freeform/BuiltCanvas.test.tsx index efbadfa48e49..6ecdd5dd7b6f 100644 --- a/products/desktop/packages/ui/src/features/canvas/freeform/BuiltCanvas.test.tsx +++ b/products/desktop/packages/ui/src/features/canvas/freeform/BuiltCanvas.test.tsx @@ -122,4 +122,83 @@ describe("BuiltCanvas", () => { }); expect(onDataRequest).not.toHaveBeenCalled(); }); + + it("opens a comment selected inside the built artifact", async () => { + const onCommentActivate = vi.fn(); + render( + , + ); + const iframe = screen.getByTitle("Canvas") as HTMLIFrameElement; + if (!iframe.contentWindow) throw new Error("Canvas iframe has no window"); + const postMessage = vi + .spyOn(iframe.contentWindow, "postMessage") + .mockImplementation(() => undefined); + + fireEvent.load(iframe); + const calls = postMessage.mock.calls as unknown as [ + unknown, + string, + Transferable[], + ][]; + const canvasPort = calls.at(-1)?.[2]?.[0] as MessagePort; + canvasPort.postMessage({ + channel: "posthog-canvas", + type: "comment-activate", + id: "comment-1", + }); + + await waitFor(() => + expect(onCommentActivate).toHaveBeenCalledWith("comment-1"), + ); + }); + + it("clears native artifact selection when the host dismisses it", async () => { + const { rerender } = render( + , + ); + const iframe = screen.getByTitle("Canvas") as HTMLIFrameElement; + if (!iframe.contentWindow) throw new Error("Canvas iframe has no window"); + const postMessage = vi + .spyOn(iframe.contentWindow, "postMessage") + .mockImplementation(() => undefined); + + fireEvent.load(iframe); + const calls = postMessage.mock.calls as unknown as [ + unknown, + string, + Transferable[], + ][]; + const canvasPort = calls.at(-1)?.[2]?.[0] as MessagePort; + const messages: unknown[] = []; + canvasPort.addEventListener("message", (event) => + messages.push((event as MessageEvent).data), + ); + canvasPort.start(); + + rerender( + , + ); + + await waitFor(() => + expect(messages).toContainEqual({ + channel: "posthog-canvas", + type: "clear-text-selection", + }), + ); + }); }); diff --git a/products/desktop/packages/ui/src/features/canvas/freeform/BuiltCanvas.tsx b/products/desktop/packages/ui/src/features/canvas/freeform/BuiltCanvas.tsx index 865393d7c5b9..0ac6ee087abe 100644 --- a/products/desktop/packages/ui/src/features/canvas/freeform/BuiltCanvas.tsx +++ b/products/desktop/packages/ui/src/features/canvas/freeform/BuiltCanvas.tsx @@ -1,12 +1,14 @@ import { assertCanvasCapability } from "@posthog/core/canvas/canvasCapabilities"; import { + type CanvasCommentHighlight, type CanvasNavIntent, + type CanvasTextSelection, canvasToHostMessageSchema, } from "@posthog/core/canvas/freeformSchemas"; import type { CanvasCapabilities } from "@posthog/shared"; import { logger } from "@posthog/ui/shell/logger"; import { openExternalUrl } from "@posthog/ui/shell/openExternal"; -import { useLayoutEffect, useRef } from "react"; +import { useEffect, useLayoutEffect, useRef } from "react"; import { createCanvasHostMessageRouter } from "./canvasHostMessageRouter"; const log = logger.scope("built-canvas"); @@ -81,6 +83,10 @@ export interface BuiltCanvasProps { onReady?: () => void; onRendered?: () => void; onNavigate?: (intent: CanvasNavIntent) => void; + onTextSelection?: (selection: CanvasTextSelection | null) => void; + onCommentActivate?: (id: string) => void; + commentHighlights?: CanvasCommentHighlight[]; + clearTextSelectionKey?: number; } export function BuiltCanvas({ @@ -91,8 +97,13 @@ export function BuiltCanvas({ onReady, onRendered, onNavigate, + onTextSelection, + onCommentActivate, + commentHighlights = [], + clearTextSelectionKey = 0, }: BuiltCanvasProps) { const iframeRef = useRef(null); + const artifactPortRef = useRef(null); const hostDocument = buildArtifactHostDocument(artifactUrl); const latest = useRef({ capabilities, @@ -101,6 +112,9 @@ export function BuiltCanvas({ onReady, onRendered, onNavigate, + onTextSelection, + onCommentActivate, + commentHighlights, }); latest.current = { capabilities, @@ -109,6 +123,9 @@ export function BuiltCanvas({ onReady, onRendered, onNavigate, + onTextSelection, + onCommentActivate, + commentHighlights, }; // biome-ignore lint/correctness/useExhaustiveDependencies: a new host document needs a fresh bridge even though the effect reads it only through the iframe. @@ -129,9 +146,33 @@ export function BuiltCanvas({ log.warn("Built canvas error", { message }); latest.current.onError?.(message, stack); }, - onReady: () => latest.current.onReady?.(), + onReady: () => { + artifactPort?.postMessage({ + channel: "posthog-canvas", + type: "set-comment-highlights", + highlights: latest.current.commentHighlights, + }); + latest.current.onReady?.(); + }, onRendered: () => latest.current.onRendered?.(), onNavigate: (intent) => latest.current.onNavigate?.(intent), + onTextSelection: (selection) => { + if (!selection) { + latest.current.onTextSelection?.(null); + return; + } + const frame = iframeRef.current?.getBoundingClientRect(); + latest.current.onTextSelection?.({ + ...selection, + rect: { + top: selection.rect.top + (frame?.top ?? 0), + right: selection.rect.right + (frame?.left ?? 0), + bottom: selection.rect.bottom + (frame?.top ?? 0), + left: selection.rect.left + (frame?.left ?? 0), + }, + }); + }, + onCommentActivate: (id) => latest.current.onCommentActivate?.(id), }), hasUserActivation: () => navigator.userActivation?.isActive === true, // Built artifacts run arbitrary published code, so an external open asks @@ -152,6 +193,7 @@ export function BuiltCanvas({ if (artifactPort) return; const bridge = new MessageChannel(); artifactPort = bridge.port1; + artifactPortRef.current = artifactPort; artifactPort.addEventListener("message", onMessage); artifactPort.start(); iframe?.contentWindow?.postMessage( @@ -171,6 +213,7 @@ export function BuiltCanvas({ } artifactPort?.close(); artifactPort = null; + artifactPortRef.current = null; }; iframe?.addEventListener("load", onLoad); @@ -179,9 +222,26 @@ export function BuiltCanvas({ iframe?.removeEventListener("load", onLoad); window.removeEventListener("message", onHostMessage); artifactPort?.close(); + artifactPortRef.current = null; }; }, [hostDocument]); + useEffect(() => { + artifactPortRef.current?.postMessage({ + channel: "posthog-canvas", + type: "set-comment-highlights", + highlights: commentHighlights, + }); + }, [commentHighlights]); + + useEffect(() => { + if (clearTextSelectionKey === 0) return; + artifactPortRef.current?.postMessage({ + channel: "posthog-canvas", + type: "clear-text-selection", + }); + }, [clearTextSelectionKey]); + return (