Skip to content

Respect recording Trash and comment settings - #8059

Open
3mdistal wants to merge 3 commits into
mainfrom
t3/clips-trash-comment-install-fixes
Open

3mdistal wants to merge 3 commits into
mainfrom
t3/clips-trash-comment-install-fixes

Conversation

@3mdistal

@3mdistal 3mdistal commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Landing is on hold pending a reviewable merge path that includes #8088’s comment-history privacy fix and Shomix’s Clips-owner sign-off.

What changed

Apply one shared Trash check to recording data, media, recording context, and notifications. Owners retain their Trash preview, and restoring a recording restores normal access.

Comment actions share recording lifecycle and discussion-setting checks. Replies reuse the existing comment action, and comment reactions follow the same expiry policy. Recording context omits disabled comments. Comment deletion respects Trash access while allowing authorized cleanup with comments disabled.

The clarified comments-off policy is implemented separately in #8088, stacked on this PR. That follow-up hides saved comments from viewers in notifications and search while preserving the owner's read access. This PR keeps its current code and history behavior until the follow-up merges.

Verification

  • Reproduced both behaviors with failing-first action and route tests on the unchanged baseline.
  • All 229 tests in the thirteen relevant suites pass, including owner preview, restored access, enabled and disabled comments, comment listing, recording context, notifications, cleanup, and reply compatibility.
  • Clips typecheck and formatting pass.
  • Independent Sol reviews of both fixes found no blocking issues.
  • Repository guard sweep completed, with relevant guards rerun for the follow-up. Guards pass with GNU grep on PATH, including dead-settings-keys. Localization catalogs, migration-manifest, E2E-quarantine, and changed-copy checks pass.

No schema or stored-data changes.

@github-actions

Copy link
Copy Markdown
Contributor

Here's a visual recap of what changed:

Visual recap

Open the full interactive recap

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builder reviewed your changes and found 1 potential issue 🟡

Review Details

Incremental review summary

The latest update adds Trash protection to delete-comment, with tests showing non-owners are blocked while owners retain cleanup access. I verified that this addresses the prior delete-path finding and resolved that thread. The separate notification-preview comment remains open; this update's tests explicitly preserve historical previews when discussion is disabled, so I did not re-report it. The lifecycle checks are otherwise consistent across the changed recording/media/context paths. One related read path still appears inconsistent: recording search can return comment snippets without consulting the discussion setting. This is a high-risk PR because it changes authorization and comment data access.

New finding

  • 🟡 MEDIUM — search-recordings can return saved comment text even when disabled comments are withheld from the comment list and player payload.

🧪 Browser testing: Skipped — the browser-test-planner agent is unavailable in this environment.

Comment thread templates/clips/server/lib/comment-access.ts

Copy link
Copy Markdown
Contributor

Thanks for the update and owner confirmation. The current #8059 head still leaves disabled-comment snippets visible through search and notifications, while #8088 carries that fix on top. I’m holding this until the privacy fix is in the reviewable merge path and Shomix signs off.

Copy link
Copy Markdown
Contributor Author

Understood. Landing remains on hold until the reviewable merge path includes #8088’s notification/search policy and Shomix’s Clips-owner sign-off is recorded. #8088 remains linked and stacked for that review; I will not land the current #8059 head on its own.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants