Skip to content

fix(share): return to video after signing in from denied share page (#2192) - #2287

Open
massmarketconsumer-arch wants to merge 1 commit into
CapSoftware:mainfrom
massmarketconsumer-arch:fix/share-page-signin-redirect
Open

fix(share): return to video after signing in from denied share page (#2192)#2287
massmarketconsumer-arch wants to merge 1 commit into
CapSoftware:mainfrom
massmarketconsumer-arch:fix/share-page-signin-redirect

Conversation

@massmarketconsumer-arch

@massmarketconsumer-arch massmarketconsumer-arch commented Sep 12, 2026

Copy link
Copy Markdown

Resolves #2192

Summary

When a viewer lands on a restricted share page (/s/{videoId} with email_restriction_login_required or a private video), PolicyDeniedView previously linked to /login without a next redirect query parameter. After authenticating, the viewer was redirected to /dashboard rather than back to the requested video.

This PR:

  1. Passes videoId into PolicyDeniedView and constructs loginHref = videoId ? /login?next=${encodeURIComponent(/s/${videoId})} : "/login".
  2. Adds a prominent "Sign in" button using @cap/ui Button (with asChild) for restricted access and private video views.
  3. Keeps email_restriction_denied copy-only (omits sign-in button since the viewer is already logged in with an unauthorized domain).
  4. Extracts PolicyDeniedView into apps/web/app/s/[videoId]/_components/PolicyDeniedView.tsx following the modular structure of _components/.

Testing

  • Added unit tests in apps/web/__tests__/unit/share-policy-denied.test.ts covering:
    • Private video view generating /login?next=%2Fs%2F{videoId} links and sign-in button.
    • email_restriction_login_required generating return links and sign-in button.
    • email_restriction_denied rendering copy-only without sign-in button.
    • Graceful fallback to /login when videoId is omitted.
  • Verified all unit tests pass cleanly (4/4 tests passing).
  • Validated with Biome (0 lint / formatting errors).

RetriggerConfidence Score: 4/5

The PR appears safe to merge, with non-blocking improvements recommended for the authenticated private-video experience and test robustness.

Findings

  1. P2 Authenticated sign-in loops
  2. P2 Tests depend on child order
Fix with agent prompt
### Issue 1
apps/web/app/s/[videoId]/_components/PolicyDeniedView.tsx:10-13
A signed-in non-owner can reach this private-video view, but both sign-in links send them to `/login?next=/s/{videoId}`. Because the login page detects the existing session and immediately returns them to the same denied page, this action creates a no-op redirect loop. This is non-blocking, but the sign-in option should only appear for anonymous viewers, or authenticated viewers should get an account-switch path.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

### Issue 2
apps/web/__tests__/unit/share-policy-denied.test.ts:11-18
These tests inspect unrendered React children through fixed array positions. An unrelated markup reorder will break them, while they do not verify the actual anchor produced by `Button`, Radix `Slot`, and Next.js `Link`. This makes the tests brittle and gives less confidence in the rendered behavior; render the component to static markup and assert its visible text and anchor attributes instead. The same pattern also appears in the other tests in this file.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

  • Passes the video ID into the denial view and encodes the share route in the login next parameter.
  • Adds a prominent sign-in button for private and login-required views.
  • Keeps domain-denied views copy-only.
  • Adds unit coverage for denial reasons and generated destinations.

Reviews (1) · Last reviewed commit: "fix(share): redirect back to video after..."

Comment on lines +10 to +13
export function PolicyDeniedView({ videoId, reason }: PolicyDeniedViewProps) {
const loginHref = videoId
? `/login?next=${encodeURIComponent(`/s/${videoId}`)}`
: "/login";

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.

P2 Authenticated sign-in loops

A signed-in non-owner can reach this private-video view, but both sign-in links send them to /login?next=/s/{videoId}. Because the login page detects the existing session and immediately returns them to the same denied page, this action creates a no-op redirect loop. This is non-blocking, but the sign-in option should only appear for anonymous viewers, or authenticated viewers should get an account-switch path.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/s/[videoId]/_components/PolicyDeniedView.tsx
Line: 10-13

Comment:
**Authenticated sign-in loops**

A signed-in non-owner can reach this private-video view, but both sign-in links send them to `/login?next=/s/{videoId}`. Because the login page detects the existing session and immediately returns them to the same denied page, this action creates a no-op redirect loop. This is non-blocking, but the sign-in option should only appear for anonymous viewers, or authenticated viewers should get an account-switch path.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +11 to +18
const children = (view as ReactElement<{ children: ReactElement[] }>).props
.children;
const titleElement = children[1];
const buttonElement = children[3];

expect(titleElement.props.children).toBe("This video is private");
expect(buttonElement).toBeDefined();

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.

P2 Tests depend on child order

These tests inspect unrendered React children through fixed array positions. An unrelated markup reorder will break them, while they do not verify the actual anchor produced by Button, Radix Slot, and Next.js Link. This makes the tests brittle and gives less confidence in the rendered behavior; render the component to static markup and assert its visible text and anchor attributes instead. The same pattern also appears in the other tests in this file.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/__tests__/unit/share-policy-denied.test.ts
Line: 11-18

Comment:
**Tests depend on child order**

These tests inspect unrendered React children through fixed array positions. An unrelated markup reorder will break them, while they do not verify the actual anchor produced by `Button`, Radix `Slot`, and Next.js `Link`. This makes the tests brittle and gives less confidence in the rendered behavior; render the component to static markup and assert its visible text and anchor attributes instead. The same pattern also appears in the other tests in this file.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

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.

Restricted share page has no way back to the video after sign-in

1 participant