Repository navigation
Apply comment visibility and expiry rules across Clips - #8088
Conversation
|
Here's a visual recap of what changed: Open the full interactive recap |
|
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. |
|
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. |
|
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. |
|
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.
|
There was a problem hiding this comment.
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.
# Conflicts: # docs/environment-variables.md # packages/core/src/server/index.ts # templates/clips/server/plugins/db.ts

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:
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.