Skip to content

Preserve comment access after unlocking protected Clips - #8093

Open
3mdistal wants to merge 15 commits into
t3/clips-comment-access-hardeningfrom
t3/clips-password-write-hardening
Open

3mdistal wants to merge 15 commits into
t3/clips-comment-access-hardeningfrom
t3/clips-password-write-hardening

Conversation

@3mdistal

@3mdistal 3mdistal commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Viewers who unlock a password-protected Clip can keep commenting as they watch. A separate ten-minute comment grant records the successful unlock; comment actions verify its recording, password version, purpose and expiry alongside existing permissions. Owners retain password-free access. Media and agent-read grants keep their read-only scope.

Share pages renew comment access during viewing and when a tab resumes. Fresh grants are reused, and writes join an existing renewal before cancelling queries or sending. Optimistic updates appear immediately. Failed sends show inline feedback and retain drafts for retry without replacing newer typing or reply selections, including in fullscreen. Keyboard and button submissions share a synchronous guard while renewal or transport is pending, preventing repeated sends while preserving retries and later drafts. Pending sends and recovery state survive fullscreen transitions and composer dismissal. The mutation stays mounted for the page lifetime while the visible fields move into the player or close. A successful send refreshes the posted comment and clears only the unchanged submitted draft, preserving newer text for its next send. When newer work occupies the composer after a failed send, the unsent comment remains visible with Retry and local Delete. Retries preserve its text, mentions, timestamp and reply target; multiple failures survive recording refetches without replacing newer drafts.

Validation:

  • 375 relevant Clips tests across 22 files pass, including failing-first regressions for renewal, draft recovery, fullscreen feedback, repeated submits, fullscreen transitions and Cancel/Escape close/reopen. The failed-submission repair fails four cases on unchanged production, then passes all 72 focused tests in both implementation and root reruns. Earlier root verification passed 262 tests across six comment, player and access-policy suites.
  • Current core build, Clips typecheck, formatting and all 93 repository guards pass, with GNU grep on PATH.
  • Independent Sol reviews found no remaining actionable issues, including review of the newer-draft success and failed-submission recovery repairs. The protected-collaborator exception did not reproduce under the established entry policy: protected non-owners already use the password-aware share flow. Authorization code remains unchanged; existing suites and 54 runtime gate characterizations pass.
  • A full local Clips attempt had three environment failures: two missing @sentry/browser imports and a Windows FFmpeg spawn error. That full run is not claimed as passing.

Live E2B verification on a67d3992dc5b (parent 9c6016c7629a) used Chromium, the real Clips runtime and PostgreSQL, fake signed-in users, and authentication enabled. All 30,534 tracked source hashes were verified.

Case Result
Viewer unlocks through the share page and posts 200; persisted and visible after reload
Same viewer in a clean session without unlocking 403; no comment persisted
Owner posts without a password 200; persisted
Valid media and agent-read grants submit comments Both 403; no comments persisted; their read requests succeeded

Those four checks retain coverage of the unchanged authorization and ordinary comment paths; they are not relabeled as new execution.

Additional root-owned live UI verification passed on 179c188371c6 (core ancestor 9c6016c7629a) using real Chromium fullscreen, the real Clips runtime and PostgreSQL, an authenticated fake viewer, and a 1440×900 viewport. Browser clock aging triggered renewal, and selected requests were held or aborted to exercise recovery. Successful responses came from the real server.

Current-head UI case Result
Repeated submit during renewal/POST, fullscreen exit/reentry, Cancel/Escape close/reopen One POST and one stored comment; completion refreshed and cleared the draft; a later draft also sent once
Renewal failure and retry Visible inline error and retained draft across fullscreen/dismissal changes; zero failed-attempt posts; retry returned 200 and persisted once
Newer text typed while a send is pending Newer text survived the earlier success and subsequently sent normally
Failed submission beside newer text Failed row remained visible; Retry preserved its content and timestamp and persisted once; local Delete sent no request

Post-run verification confirmed six distinct intended comments and unchanged hashes for all 30,534 tracked regular files, 80 tracked symlinks, 23 PR files, and 7,601 built-core artifacts. Both served component source maps matched the candidate. Request/persistence evidence and representative screenshots were downloaded and all 22 evidence hashes verified. The initial harness assertion was corrected by settling initial requests before aging the grant; no application source changed during verification. This closes the previously incomplete share-page fullscreen/recovery check. It does not claim a live in-app collaborator bypass or an in-app fullscreen run.

Browser contexts and the preview tab were closed. Sandbox icwyo1vrwb3oahb6iwp6s was terminated and confirmed absent before its deadline. No video or review document was produced.

Stacked on #8092, with the password-write mechanism reviewed separately. The parent must not merge independently of this change.

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

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. I see you’re reproducing both current-head cases. Please push the fixes and failing-first coverage; I’ll re-check draft preservation and the in-app collaborator path on the new head before approval.

Copy link
Copy Markdown
Contributor Author

Pushed 31baf18 with the newer-draft repair and failing-first coverage: four cases failed on unchanged production, then passed with the fix. Posted comments still refresh; newer unsent text survives and can be sent afterward. All 373 relevant tests, the root’s 262-test recheck, typecheck, 93 guards and fresh Sol review pass.

For the collaborator path, protected non-owners already fail the direct-player entry gate before comments or media are returned, including explicit collaborators. They use the password-aware share flow; this PR’s purpose-scoped comment proof preserves that flow. Existing player/policy tests and 54 runtime gate characterizations pass. I could not reproduce the claimed fresh direct-player exception and left authorization unchanged. The inline thread has the disposition for your re-check.

The additional live fullscreen transition/retry check remains incomplete after a tooling timeout; its sandbox was terminated. The four earlier live authorization cases remain recorded with their tested SHA. No merge requested.

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

Code Review Summary

This high-risk PR adds purpose-scoped, short-lived comment grants for password-unlocked Clips and wires them through public-share comment reads and writes, with client renewal and resilient optimistic composer behavior. I rechecked the authorization flow and the new recovery path. The previously reported failed-draft issue is fixed: failed submissions with newer text remain visible in the thread as retryable entries. The collaborator concern is not applicable under the current route policy: non-owner viewers of password-protected recordings are denied the direct recording route and use the password-aware share flow. I verified both changes and resolved the two stale threads before this review.

Review Outcome

No new actionable issues were confirmed. Grant verification remains bound to the recording and current password version, with expiry enforced; existing lifecycle and comment permissions remain in place. The added retry/delete controls preserve failed text without overwriting newer input, and test coverage now includes this scenario. 🧪 Browser testing: Skipped — browser-test-planner is unavailable in this environment.

Copy link
Copy Markdown
Contributor Author

Current-head live E2B UI verification passed on 179c188 with fake signed-in users, authentication enabled, real Chromium and PostgreSQL. Repeated keyboard/button sends, fullscreen transitions, and Cancel/Escape reopening produced one stored comment; completion and later sends worked. Renewal errors visibly retained the draft and retried successfully. Newer text survived an earlier success. Failed-row Retry retained the original metadata beside newer text, and local Delete made no request. Six distinct intended comments persisted with no duplicates.

The post-run source/build seal and served component maps matched the candidate; the evidence was downloaded and verified. Browser contexts were closed and the sandbox was terminated. The PR body now contains these results, distinct from the earlier four authorization checks. No application code changed during this verification. The existing human-review and landing holds remain.

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