Feature/report comments - #1269
Merged
Merged
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
ddpui/core/reports/report_service.py (1)
666-679: LGTM! Clean helper method for token management.Good encapsulation of the token generation logic. The method is idempotent and correctly uses
update_fieldsfor efficient partial update.Minor: The docstring mentions
Args:section in the AI summary context but the actual docstring omits it. Consider adding for consistency:"""Ensure the snapshot has a share token, generating one if needed. ... Args: snapshot: The ReportSnapshot instance Returns: The share token string. """,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/core/reports/report_service.py` around lines 666 - 679, Add the missing Args section to the ensure_share_token docstring: document the snapshot parameter (type ReportSnapshot and that it's the ReportSnapshot instance to ensure has a share token) and keep the existing Returns description; update the docstring above ensure_share_token to include an "Args:" block mentioning snapshot and its role.ddpui/core/comments/comment_service.py (1)
314-388: Consider removing or documenting these unused helper methods.
_group_comments_by_targetand_compute_target_statesappear to be superseded by the raw SQL approach inget_comment_states. If they're kept for fallback or testing purposes, add a comment explaining their purpose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/core/comments/comment_service.py` around lines 314 - 388, The two helper methods _group_comments_by_target and _compute_target_states are unused now that get_comment_states uses raw SQL; either remove them or add a concise explanatory comment/docstring above each (or a module-level note) stating they are retained intentionally for fallback/testing and are superseded by get_comment_states, and include references to their signatures (_group_comments_by_target(comments) and _compute_target_states(targets, read_statuses, mentioned_ids)) so future readers know why they remain in comment_service.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ddpui/core/comments/comment_service.py`:
- Around line 253-259: The code builds user_email_json via a raw f-string which
can break JSON if orguser.user.email contains quotes; replace that with proper
JSON serialization (e.g., import json and set user_email_json =
json.dumps(orguser.user.email)) and keep passing user_email_json into the
CommentService._COMMENT_STATES_SQL parameter list alongside orguser.id and
snapshot_id so the query receives a safely-escaped JSON string.
- Around line 74-77: The OrgUser lookup currently builds users_map by querying
OrgUser.objects.filter(user__email__in=all_emails) which returns matches across
all orgs; restrict the query to the current comment batch's org by adding an org
filter (e.g. .filter(org=<current_org>, user__email__in=all_emails)). Update the
OrgUser.objects.filter call in the users_map construction to include the
appropriate org reference (for example snapshot.org or org from the surrounding
scope) so resolution is tenant-isolated.
In `@ddpui/core/reports/report_service.py`:
- Line 49: The FrozenDashboardConfig schema is missing the dashboard_id field so
the dict returned by _freeze_dashboard (which includes "dashboard_id":
dashboard.id) is being stripped during validation and
snapshot.frozen_dashboard.get("dashboard_id") returns None; update the
FrozenDashboardConfig class to declare dashboard_id: Optional[int] = None
(keeping other fields intact) so Pydantic includes this key, then run tests or
validations to confirm snapshot.frozen_dashboard now contains the dashboard_id
used later in snapshot processing.
In `@ddpui/schemas/comment_schema.py`:
- Around line 85-89: The fallback mention-resolution query currently builds
users_map from OrgUser.objects.filter(user__email__in=emails) which lacks org
isolation; update that filter to include the comment's organization (e.g., add
org=comment.org or org_id=comment.org_id) so the lookup restricts to OrgUser
records for the same org, keep select_related("user") and the rest of the logic
unchanged.
---
Nitpick comments:
In `@ddpui/core/comments/comment_service.py`:
- Around line 314-388: The two helper methods _group_comments_by_target and
_compute_target_states are unused now that get_comment_states uses raw SQL;
either remove them or add a concise explanatory comment/docstring above each (or
a module-level note) stating they are retained intentionally for
fallback/testing and are superseded by get_comment_states, and include
references to their signatures (_group_comments_by_target(comments) and
_compute_target_states(targets, read_statuses, mentioned_ids)) so future readers
know why they remain in comment_service.py.
In `@ddpui/core/reports/report_service.py`:
- Around line 666-679: Add the missing Args section to the ensure_share_token
docstring: document the snapshot parameter (type ReportSnapshot and that it's
the ReportSnapshot instance to ensure has a share token) and keep the existing
Returns description; update the docstring above ensure_share_token to include an
"Args:" block mentioning snapshot and its role.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ed0969e-ed54-4cba-ad8d-220ed71a1703
📒 Files selected for processing (6)
ddpui/api/report_api.pyddpui/core/comments/comment_service.pyddpui/core/reports/pdf_export_service.pyddpui/core/reports/report_service.pyddpui/schemas/comment_schema.pyddpui/tests/core/comments/test_comment_service.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ddpui/core/reports/pdf_export_service.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
ddpui/api/comments_api.py (1)
150-166:⚠️ Potential issue | 🟠 MajorMap
CommentValidationErrorfromupdate_comment()to a 400.
CommentService.update_comment()rejects soft-deleted comments withCommentValidationError, but this handler only translates not-found and permission failures. Those requests still bubble as 500s.🐛 Proposed fix
except CommentNotFoundError as err: raise HttpError(404, str(err)) from err + except CommentValidationError as err: + raise HttpError(400, str(err)) from err except CommentPermissionError as err: raise HttpError(403, str(err)) from err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/api/comments_api.py` around lines 150 - 166, The try/except around CommentService.update_comment in the comments API only maps CommentNotFoundError and CommentPermissionError to HttpError responses, so CommentValidationError (raised for soft-deleted comments) escapes as a 500; add an except CommentValidationError as err branch after the other handlers to raise HttpError(400, str(err)) when CommentService.update_comment(...) raises CommentValidationError, keeping the existing response formation for successful updates (api_response and CommentResponse.from_model).ddpui/core/comments/mention_service.py (1)
45-60:⚠️ Potential issue | 🟡 MinorFilter the author out of
mentioned_users.
process_mentions()currently stores and notifies the author when their own email is present. That contradicts the “Skips self-mentions” contract below and still lets crafted payloads generate self-notifications.🐛 Proposed fix
mentioned_users = list( OrgUser.objects.filter( org=org, user__email__in=mentioned_emails, - ).select_related("user") + ) + .exclude(pk=author.pk) + .select_related("user") )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/core/comments/mention_service.py` around lines 45 - 60, Filter out the author from the resolved mentioned_users list before storing or notifying: after obtaining mentioned_users (OrgUser instances), remove any entry whose user.id (or user.email) equals author.id (or author.email), and if the resulting list is empty return [] so you don't call MentionService.store_mentioned_emails or MentionService.notify_mentioned_users with the author included; update the code around the mentioned_users variable (used for calling MentionService.store_mentioned_emails and MentionService.notify_mentioned_users) to use the filtered list.ddpui/core/comments/comment_service.py (1)
47-59:⚠️ Potential issue | 🟠 MajorValidate
target_type/chart_idon the read paths too.
create_comment()already rejects impossible targets, but_fetch_comments()andmark_as_read()still don't. That leaveslist_comments()returning200/[]for bad requests, andmark_as_read()able to persist read cursors for snapshots/charts the caller should not be able to address.🐛 Suggested direction
def _fetch_comments( snapshot: ReportSnapshot, target_type: str, chart_id: Optional[int], ) -> list: """Fetch comments for a target, ordered chronologically.""" + CommentService._validate_target(snapshot, target_type, chart_id) query = Q(snapshot=snapshot, target_type=target_type) def mark_as_read( snapshot_id: int, orguser: OrgUser, target_type: str, chart_id: Optional[int] = None, ) -> None: """Mark a target's comments as read by upserting CommentReadStatus.""" + snapshot = CommentService._get_snapshot(snapshot_id, orguser.org) + CommentService._validate_target(snapshot, target_type, chart_id) CommentReadStatus.objects.update_or_create( user=orguser, - snapshot_id=snapshot_id, + snapshot=snapshot, target_type=target_type, chart_id=chart_id if target_type == CommentTargetType.CHART else None, defaults={"last_read_at": timezone.now()}, )Also applies to: 270-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/core/comments/comment_service.py` around lines 47 - 59, Ensure read paths validate the target_type/chart_id combination like create_comment does: in _fetch_comments (the function shown) add explicit validation that if target_type == CommentTargetType.CHART then chart_id must be provided, and if target_type != CommentTargetType.CHART then chart_id must be None (raise CommentValidationError on invalid combos); apply the same validation logic inside mark_as_read (the method that persists read cursors) and any other read helper used by list_comments so invalid requests return a validation error instead of silently returning 200/[] or persisting illegal cursors.ddpui/models/comment.py (1)
92-97:⚠️ Potential issue | 🟠 Major
chart_id=NULLleaves summary read cursors unenforced.For summary rows,
chart_idisNULL, so thisunique_togetherstill allows duplicate(user, snapshot, target_type='summary', chart_id=NULL)records. Once that happens,mark_as_read()can fail withMultipleObjectsReturned, and the state query can double-count rows.🐛 Proposed fix
class Meta: db_table = "comment_read_status" - unique_together = [("user", "snapshot", "target_type", "chart_id")] + constraints = [ + models.UniqueConstraint( + fields=["user", "snapshot", "target_type"], + condition=models.Q( + chart_id__isnull=True, + target_type=CommentTargetType.SUMMARY, + ), + name="uniq_comment_read_status_summary", + ), + models.UniqueConstraint( + fields=["user", "snapshot", "target_type", "chart_id"], + condition=models.Q(chart_id__isnull=False), + name="uniq_comment_read_status_chart", + ), + ] indexes = [ models.Index(fields=["user", "snapshot"]), ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/models/comment.py` around lines 92 - 97, The current Meta.unique_together allows duplicate rows when chart_id IS NULL (summary rows), so replace unique_together with explicit UniqueConstraint entries that enforce uniqueness separately for NULL and non-NULL chart_id cases: add one UniqueConstraint(fields=["user","snapshot","target_type","chart_id"]) conditioned on ~Q(chart_id__isnull=True) (non-NULL chart_id) and another UniqueConstraint(fields=["user","snapshot","target_type"]) conditioned on Q(chart_id__isnull=True) (summary rows), updating the Meta class in ddpui.models.comment and import Q from django.db.models; this will prevent duplicates that cause mark_as_read() MultipleObjectsReturned and double-counting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ddpui/core/comments/comment_service.py`:
- Around line 188-202: The SQL that computes unread_count and
unread_mentioned_count incorrectly includes the viewer's own comments; update
the CASE conditions in the SUMs (the blocks producing unread_count and
unread_mentioned_count that reference crs.last_read_at and "comment".created_at)
to exclude authored comments by adding a check against "comment".author_id
(e.g., AND "comment".author_id != %s or similar param) so they match
_annotate_is_new() behavior, and add a regression test for the author-as-viewer
scenario to ensure the badge does not show unread immediately after posting.
---
Duplicate comments:
In `@ddpui/api/comments_api.py`:
- Around line 150-166: The try/except around CommentService.update_comment in
the comments API only maps CommentNotFoundError and CommentPermissionError to
HttpError responses, so CommentValidationError (raised for soft-deleted
comments) escapes as a 500; add an except CommentValidationError as err branch
after the other handlers to raise HttpError(400, str(err)) when
CommentService.update_comment(...) raises CommentValidationError, keeping the
existing response formation for successful updates (api_response and
CommentResponse.from_model).
In `@ddpui/core/comments/comment_service.py`:
- Around line 47-59: Ensure read paths validate the target_type/chart_id
combination like create_comment does: in _fetch_comments (the function shown)
add explicit validation that if target_type == CommentTargetType.CHART then
chart_id must be provided, and if target_type != CommentTargetType.CHART then
chart_id must be None (raise CommentValidationError on invalid combos); apply
the same validation logic inside mark_as_read (the method that persists read
cursors) and any other read helper used by list_comments so invalid requests
return a validation error instead of silently returning 200/[] or persisting
illegal cursors.
In `@ddpui/core/comments/mention_service.py`:
- Around line 45-60: Filter out the author from the resolved mentioned_users
list before storing or notifying: after obtaining mentioned_users (OrgUser
instances), remove any entry whose user.id (or user.email) equals author.id (or
author.email), and if the resulting list is empty return [] so you don't call
MentionService.store_mentioned_emails or MentionService.notify_mentioned_users
with the author included; update the code around the mentioned_users variable
(used for calling MentionService.store_mentioned_emails and
MentionService.notify_mentioned_users) to use the filtered list.
In `@ddpui/models/comment.py`:
- Around line 92-97: The current Meta.unique_together allows duplicate rows when
chart_id IS NULL (summary rows), so replace unique_together with explicit
UniqueConstraint entries that enforce uniqueness separately for NULL and
non-NULL chart_id cases: add one
UniqueConstraint(fields=["user","snapshot","target_type","chart_id"])
conditioned on ~Q(chart_id__isnull=True) (non-NULL chart_id) and another
UniqueConstraint(fields=["user","snapshot","target_type"]) conditioned on
Q(chart_id__isnull=True) (summary rows), updating the Meta class in
ddpui.models.comment and import Q from django.db.models; this will prevent
duplicates that cause mark_as_read() MultipleObjectsReturned and
double-counting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4d8a75a-6496-4504-bf8c-cb0166320eb0
📒 Files selected for processing (7)
ddpui/api/comments_api.pyddpui/core/comments/comment_service.pyddpui/core/comments/mention_service.pyddpui/models/comment.pyddpui/schemas/comment_schema.pyddpui/tests/core/comments/test_comment_service.pyddpui/tests/core/comments/test_mention_notifications.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ddpui/tests/core/comments/test_mention_notifications.py
Comment on lines
+188
to
+202
| SUM( | ||
| CASE | ||
| WHEN crs.last_read_at IS NULL | ||
| OR "comment".created_at > crs.last_read_at | ||
| THEN 1 ELSE 0 | ||
| END | ||
| ) AS unread_count, | ||
| SUM( | ||
| CASE | ||
| WHEN (crs.last_read_at IS NULL | ||
| OR "comment".created_at > crs.last_read_at) | ||
| AND "comment".mentioned_emails::jsonb @> %s::jsonb | ||
| THEN 1 ELSE 0 | ||
| END | ||
| ) AS unread_mentioned_count |
There was a problem hiding this comment.
Unread state disagrees with is_new for authored comments.
_annotate_is_new() marks the viewer's own comments as not new, but this SQL still counts them in unread_count and unread_mentioned_count. That leaves the badge in "unread" immediately after the current user posts, even though the list view shows nothing new.
🐛 Proposed fix
SUM(
CASE
- WHEN crs.last_read_at IS NULL
- OR "comment".created_at > crs.last_read_at
+ WHEN "comment".author_id <> %s
+ AND (
+ crs.last_read_at IS NULL
+ OR "comment".created_at > crs.last_read_at
+ )
THEN 1 ELSE 0
END
) AS unread_count,
SUM(
CASE
- WHEN (crs.last_read_at IS NULL
- OR "comment".created_at > crs.last_read_at)
+ WHEN "comment".author_id <> %s
+ AND (
+ crs.last_read_at IS NULL
+ OR "comment".created_at > crs.last_read_at
+ )
AND "comment".mentioned_emails::jsonb @> %s::jsonb
THEN 1 ELSE 0
END
) AS unread_mentioned_count
@@
- [user_email_json, orguser.id, snapshot_id],
+ [orguser.id, orguser.id, user_email_json, orguser.id, snapshot_id],Also applies to: 239-242
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ddpui/core/comments/comment_service.py` around lines 188 - 202, The SQL that
computes unread_count and unread_mentioned_count incorrectly includes the
viewer's own comments; update the CASE conditions in the SUMs (the blocks
producing unread_count and unread_mentioned_count that reference
crs.last_read_at and "comment".created_at) to exclude authored comments by
adding a check against "comment".author_id (e.g., AND "comment".author_id != %s
or similar param) so they match _annotate_is_new() behavior, and add a
regression test for the author-as-viewer scenario to ensure the badge does not
show unread immediately after posting.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ddpui/models/comment.py (1)
93-98:⚠️ Potential issue | 🟠 MajorFix nullable
chart_iduniqueness to prevent duplicate summary cursors.Line 95 uses
unique_togetherwith nullablechart_id; SQL allows multipleNULLrows, so duplicate(user, snapshot, target_type='summary', chart_id=NULL)records can slip in.Proposed fix
class Meta: db_table = "comment_read_status" - unique_together = [("user", "snapshot", "target_type", "chart_id")] + constraints = [ + models.UniqueConstraint( + fields=["user", "snapshot", "target_type"], + condition=models.Q( + target_type=CommentTargetType.SUMMARY.value, + chart_id__isnull=True, + ), + name="uniq_comment_read_status_summary", + ), + models.UniqueConstraint( + fields=["user", "snapshot", "target_type", "chart_id"], + condition=models.Q( + target_type=CommentTargetType.CHART.value, + chart_id__isnull=False, + ), + name="uniq_comment_read_status_chart", + ), + ] indexes = [ models.Index(fields=["user", "snapshot"]), ]Use this to verify the current assumption points in code before applying the migration:
#!/bin/bash set -euo pipefail echo "Model uniqueness definition:" cat -n ddpui/models/comment.py | sed -n '88,110p' echo echo "Read-status callsites that assume one row per key:" rg -n -C3 'CommentReadStatus\.objects\.filter\(|update_or_create\(' ddpui/core/comments/comment_service.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/models/comment.py` around lines 93 - 98, The current Meta.unique_together in CommentReadStatus allows multiple rows with chart_id=NULL because SQL treats NULLs as distinct; replace unique_together with a UniqueConstraint that normalizes NULL chart_id values (e.g., using Coalesce) so NULLs are treated as a concrete value for uniqueness, e.g., add a models.UniqueConstraint with expressions=(models.F('user'), models.F('snapshot'), models.F('target_type'), models.functions.Coalesce(models.F('chart_id'), models.Value(-1))), name='uniq_commentread_user_snapshot_target_chart_coalesced'; update migrations accordingly and run the provided verification grep (CommentReadStatus usages in comment_service.py) before applying the migration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ddpui/models/comment.py`:
- Around line 93-98: The current Meta.unique_together in CommentReadStatus
allows multiple rows with chart_id=NULL because SQL treats NULLs as distinct;
replace unique_together with a UniqueConstraint that normalizes NULL chart_id
values (e.g., using Coalesce) so NULLs are treated as a concrete value for
uniqueness, e.g., add a models.UniqueConstraint with
expressions=(models.F('user'), models.F('snapshot'), models.F('target_type'),
models.functions.Coalesce(models.F('chart_id'), models.Value(-1))),
name='uniq_commentread_user_snapshot_target_chart_coalesced'; update migrations
accordingly and run the provided verification grep (CommentReadStatus usages in
comment_service.py) before applying the migration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3c38a661-0b78-4a0a-b7c1-4a620aac3d50
📒 Files selected for processing (2)
ddpui/migrations/0156_add_comment_snapshot_index.pyddpui/models/comment.py
Ishankoradia
approved these changes
Mar 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
API
Migrations
Documentation
Tests
Chores