Skip to content

Apply comment visibility and expiry rules across Clips - #8088

Merged
3mdistal merged 35 commits into
mainfrom
t3/clips-comment-visibility
Oct 27, 2026
Merged

3mdistal merged 35 commits into
mainfrom
t3/clips-comment-visibility

Conversation

@3mdistal

@3mdistal 3mdistal commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Turning comments off hides saved comment text from viewers in notifications, search, playback and agent context. Owners retain their history and can delete their own comments. Creating, replying, editing, comment reactions and resolution remain disabled. Recording reactions keep their independent setting.

Recording expiry uses the recording-owner exception across discovery/counts, shared reads, linked meeting content, AI/export/analytics actions, notifications and queued email/AI work. SQL filtering happens before result limits and counts. Meeting ownership and read capabilities do not substitute for recording ownership. Malformed or unsupported legacy expiry stays unavailable to nonowners; owner searches report recording IDs needing repair. New expiry writes are validated and normalized to UTC.

Meeting access alone returns meeting notes, participants and action items. Linked Clip fields require a recording grant, and transcript access requires that grant or the meeting's explicit "Include full transcript" setting. Both paths preserve the Clip's password, Trash, expiry and takedown checks, with the existing recording-owner exceptions. Meeting readers, agent context, transcript search and public API/anonymous SSR use this boundary. Returned recording/transcript fields are explicit projections without passwords, media URLs or provider internals. Reusing a recording requires recording editor access; note generation revalidates transcript permission and meeting-editor access after cleanup.

Long-running transcription, meeting cleanup, title/summary generation and media uploads recheck expiry before committing results. Native saves, imports, restores and cloud transcript writes check the locked recording's current owner and expiry before changing content or version history. AI title/summary/chapter and filler-removal queues recheck at the shared atomic request/status write, including same-title refinements. Refused media replacements discard new uploads and preserve existing files.

Transcript and meeting-finalize jobs bind the initiating email to their signed scope and restore that caller, including early and delayed retries. Transcript storage rows retain the recording's actual owner. Manual jobs therefore use the initiator's provider context rather than implicitly borrowing the owner's personal context. Automatic owner processing remains available. Caller-bound jobs fail closed on workers predating the binding; legacy owner-processing jobs remain accepted.

A deleted meeting recording supplies no recording context or orphan transcript text; finalization can still use an independent override. An authorized metadata edit that makes a recording immediately expire succeeds with a bounded acknowledgement rather than returning unavailable content.

Follows #8059, merged as b339198. This PR now targets main through a normal merge of 90395b3, retaining current transcript-readiness and meeting-stop behavior alongside the shared visibility rules. There is no migration or backfill. These checks do not promise an atomic expiry lease across all external work.

Verification:

  • Failing-first action, rendering and executed Drizzle/PGlite regressions cover healthy/owner controls, anonymous/agent reads, filtering before pagination, and persisted meeting/transcript outcomes. The latest meeting-grant regressions produce 18 failures with eight controls on unchanged production; The current meeting-permission repair additionally reproduces two failures with 26 controls before its production edit; 61 focused tests pass across three files after rechecking editor access before persistence. These use the actual sharing resolver and PostgreSQL-compatible SQL, including separate meeting/recording grants, explicit transcript publication, safe projections and revocation during cleanup. Earlier locked-write and signed-token tests preserve transcript versions and caller binding.
  • The current full Clips run passes 438 of 439 suites: 4,598 tests pass. Two Windows ffmpeg launch failures and their associated unhandled rejection also reproduce on unchanged Respect recording Trash and comment settings #8059; no skips or baselines were added.
  • Clips typecheck, stricter compiler checks, formatting, LF/diff checks and strict lint across 129 changed source files pass. CI-equivalent format checks pass across 132 changed files, and standard lint passes across the changed source files after removing an unused test import. The refreshed full guard sweep passes 98 checks; the unchanged native compiler launcher recovery passes the remaining check, covering all 99. One obsolete route source-text assertion was replaced by executable loader coverage; its reason is visible in the test diff.
  • Earlier root-operated E2B checks verified owner Delete-only controls, optimistic deletion/reload persistence, enabled-comment controls and hidden viewer history, including keyboard and 390px layout. Real PostgreSQL 15 action checks cover six owner/viewer search cases; PostgreSQL 15.19 checks cover 19 malformed/date/offset/precision cases. All browsers and sandboxes were terminated. The later meeting-access and delayed-write repairs affect server logic; that comment-cleanup interaction coverage remains applicable.
  • The refreshed frozen install rebuilt the current workspace packages; the normal restack retains both main's transcript-readiness checks and these expiry checks. Two Sol high/default read-only rounds inspected the reader surface. Later bounded write/caller repairs were parent-reviewed and regression-tested without another independent round.

Password-aware comment actions remain in the separately reviewed #8092/#8093 stack: password-aware reads plus recording/password-version-bound proof for writes. The landed #8059 base retains that existing action-helper gap until the follow-ups land.

Limits: UI fixtures used fake sessions with auth enabled; Google sign-in and live Slack/email delivery are not covered. The new worker identity handoff has in-process coverage, not a live mixed-build deployment or provider-billing check. Current-head CI and Builder review remain required before paired landing.

@github-actions

github-actions Bot commented Oct 23, 2026 •

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.

Copy link
Copy Markdown
Contributor

Thanks for the update and follow-up details. This adds an owner-only read-only discussion panel and changes how disabled comments appear across Clips history, so please get Shomix's Clips-owner review before approval.

@3mdistal

Copy link
Copy Markdown
Contributor Author

Alice confirmed the comments-off policy on Oct 22: viewers lose access to saved comments everywhere, while owners retain read access. This PR implements that policy, including the owner read-only panel. It will remain open pending the requested Clips-owner review and your approval.

Copy link
Copy Markdown
Contributor

Thanks for the update and policy confirmation. I checked the owner share-link redirect and the documented cleanup exception; this still needs Shomix’s Clips-owner review before approval.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

@3mdistal 3mdistal changed the title Apply comment visibility settings to recording history Apply comment visibility and expiry rules across Clips Oct 27, 2026
builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

@3mdistal

Copy link
Copy Markdown
Contributor Author

The two review summaries on 2932829 describe the same get-meeting finding differently. It is addressed in the current head: get-meeting calls loadMeetingRecordingContent, which requires recording access for Clip metadata and recording access or the explicit meeting transcript opt-in for transcript data, after source protection checks. Responses use explicit projections. The original inline thread is resolved with the implementation and failing-first evidence: #8088 (comment). The current regression file passes all 26 tests; no further code change is needed for the repeated summary.

  • Codex AI

Base automatically changed from t3/clips-trash-comment-install-fixes to main October 27, 2026 11:30
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 — looks good ✅

Review Details

Incremental code review — PR #8088

Reviewed the updated PR at head 42202d5df44a2d35cdb14bdc037f30c1222fe78b. This is a high-risk authorization/privacy change overall. Since the previous reviewed head, the update adds a post-provider assertAccess("meeting", ..., "editor") check before finalized meeting notes are persisted, plus regression coverage for revoked and downgraded meeting access. The check is inside the existing failure-handling path, so denied callers do not write the generated summary or action items. The previously open finding on this exact gap is verified fixed and has been resolved (PRRT_kwDORlS_j86wrEmJ).

The three independent review passes found no additional confirmed issues in this incremental change; the remaining diff is limited to the meeting-finalization authorization fix and tests. No new review comments are warranted. 🧪 Browser testing: Skipped — this incremental diff only changes server-side authorization and tests; no UI files or visual behavior changed.

@3mdistal
3mdistal merged commit 2d5ae56 into main Oct 27, 2026
45 of 46 checks passed
@3mdistal
3mdistal deleted the t3/clips-comment-visibility branch October 27, 2026 12:34
3mdistal added a commit that referenced this pull request Oct 27, 2026
# Conflicts:
#	docs/environment-variables.md
#	packages/core/src/server/index.ts
#	templates/clips/server/plugins/db.ts
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.

3 participants