Skip to content

Respect recording Trash and comment settings - #8059

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

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

Conversation

@3mdistal

@3mdistal 3mdistal commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Land with #8088 so the comments-off privacy policy reaches every reader together.

What changed

Activity email recipients use the same owner-aware Trash policy as notification reads. Owner previews remain available, and restoring a recording restores normal notification recipients. Existing password, expiry, resource-access, and notification preferences still apply.

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. Completed bug-report context also follows the same recording lifecycle gate.

Share links now show the existing “Clip unavailable” state after the first Not found response, including missing and trashed recordings. The obsolete missing-row retry no longer holds visitors on a loading placeholder. Anonymous server data and social metadata also treat trashed recordings as missing. Initial loading, session discovery, live private access requests, password prompts, and processing polling remain intact.

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 retains its existing comment-history behavior pending that follow-up.

Verification

  • The final array-bounds narrowing passes 15 screen-context tests and the unmodified compiler-strictness guard, launched through the same TypeScript binary via Node on Windows. Lifecycle behavior and the earlier technical review remain applicable.

  • Three activity-recipient regressions failed before the repair; thirteen controls passed. The final root rerun passes all 82 focused tests across four files. The five-file repair received a fresh Sol high/default review with no findings; typecheck, strict lint, oracle/source-test guards, formatting, LF and diff checks pass. Database/access/mail boundaries are mocked; no live email delivery is claimed.

  • After merging main (a4031062), all 86 focused comment/lifecycle tests and Clips typecheck pass. Main’s expiry controls and catalog metadata are retained alongside the shared Trash/comment-setting gate. Current strict lint, the oracle guard, source-reading-test guard, formatting, and diff checks pass.

  • Original action and route regressions reproduced before production edits; all 229 tests in thirteen relevant suites passed for those fixes.

  • Both unavailable-page cases reproduced with failing-first rendering tests. The loader/meta regression also failed before the metadata repair for public and private Trash. The earlier route suite passed 134 tests in thirteen files, and the earlier root rerun passed 173 tests across eight affected suites.

  • Root-owned E2B Chromium verification on 71fb7a9 shows missing and trashed links display “Clip unavailable” after one 404 on desktop and mobile. Private access-request and password-prompt controls pass. Authentication was enabled, data was fake, and the sandbox and browser contexts were terminated afterward.

  • The completed bug-report regressions failed first for Trash and expiry; 31 focused tests now pass, including owner and available-editor controls.

  • Clips typecheck and formatting pass. Independent Sol reviews found no actionable issues.

  • Earlier sweeps passed all 93 repository guards and dependency-setup recovery reruns, with GNU grep on PATH. The metadata repair additionally passes the SSR cache and test guards, and the prior CI strict-lint policy reported zero findings across all 31 changed TypeScript files. The oracle-change comments explicitly document why the obsolete retry-helper and source-string assertions were replaced by failing-first rendering tests; the current oracle guard passes. No guard baseline was changed.

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[bot]

This comment was marked as outdated.

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.

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 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 review summary

Reviewed PR #8059 at the latest head (a089bd1). The PR applies Trash and expiry access rules across recording data, media, comment actions, recording context, share SSR, and activity notifications, while preserving owner Trash previews and restoring access after untrash. This is a high-risk privacy/access-control change. The latest commit only changes how view-screen.ts extracts the first recording row from the query result; the logic remains equivalent, and the full current diff received four independent balanced reviews.

No new actionable issues were confirmed in the PR diff. One reviewer raised whether expired owners should receive activity notifications, but the existing policy and added notification-list tests explicitly filter expired recordings for owners too, so this is not a new regression. The approach otherwise consistently shares lifecycle checks and adds useful regression coverage. Landing remains coordinated with #8088 for the comments-off notification/search policy.

🧪 Browser testing: Skipped — browser-test-planner is unavailable in this environment; no independent browser run was performed.

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.

3 participants