fix(syncdeck): support LMS iframe waiting handoffs - #333
Conversation
|
Warning Review limit reached
Next review available in: 28 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe Learn SyncDeck waiting-room handoff now sets production cookies with ChangesLearn SyncDeck cookie handoff
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@activities/syncdeck/server/learnIntegration.test.ts`:
- Around line 132-139: Add a separate non-production test alongside the existing
production coverage, setting NODE_ENV to a non-production value and asserting
the cookie options include sameSite: 'lax', secure: false, and partitioned:
false. Preserve the current production assertions and reuse the existing Learn
integration test setup and cleanup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d2b1d481-0fac-4394-970e-23ab9e638420
📒 Files selected for processing (4)
.agent/knowledge/data-contracts.mdDEPLOYMENT.mdactivities/syncdeck/server/learnIntegration.test.tsactivities/syncdeck/server/learnIntegration.ts
There was a problem hiding this comment.
Pull request overview
This PR updates the Learn→SyncDeck student waiting-room browser handoff to work when launched inside an LMS-hosted iframe by switching the handoff cookie to a production-only Secure; SameSite=None; Partitioned httpOnly cookie, and adds documentation + tests to lock in the behavior.
Changes:
- Update the Learn waiting-room launch route to issue a production-only cross-site iframe-compatible handoff cookie (
SameSite=None,Secure,Partitioned). - Add server integration tests asserting cookie attributes in both development and production
NODE_ENVmodes. - Document HTTPS requirements and the iframe waiting-room handoff contract.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
DEPLOYMENT.md |
Documents the HTTPS requirement and cookie attributes for LMS iframe waiting-room launches. |
activities/syncdeck/server/learnIntegration.ts |
Adjusts the learn_syncdeck_wait cookie attributes for production iframe compatibility. |
activities/syncdeck/server/learnIntegration.test.ts |
Adds coverage for cookie attributes in non-production and asserts production attributes in the main flow test. |
.agent/knowledge/data-contracts.md |
Records the iframe waiting-room handoff contract and test/validation expectations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
activities/syncdeck/server/learnIntegration.ts:972
- The new LMS-iframe handoff cookie is a third-party cookie in production (
SameSite=None). It’s currently a session cookie (nomaxAge/expires), so it can persist longer than the server-side waiting-room entry TTL and longer than needed for the handoff. Consider bounding its lifetime toWAITING_TTL_MSto reduce unnecessary third-party cookie retention and keep client/server TTLs aligned.
res.cookie?.('learn_syncdeck_wait', cookieValue(key.secret, token.mappingId), {
path: '/',
httpOnly: true,
sameSite: isProduction ? 'none' : 'lax',
secure: isProduction,
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
activities/syncdeck/server/learnIntegration.ts:972
- The waiting-room handoff cookie is scoped to
path: '/', but it is only read by the/api/integrations/learn/v1/activities/syncdeck/wait/statusflow. Scoping the cookie path to the narrowest route prefix reduces exposure to unrelated endpoints (and avoids the cookie being sent on other requests unnecessarily).
maxAge: WAITING_TTL_MS,
path: '/',
httpOnly: true,
sameSite: isProduction ? 'none' : 'lax',
secure: isProduction,
partitioned: isProduction,
activities/syncdeck/server/learnIntegration.test.ts:320
- If the handoff cookie path is narrowed to the wait/status API prefix, this assertion should be updated accordingly (otherwise the test will keep passing even if the cookie remains overly broad).
assert.equal(waitCookie?.options.maxAge, 10 * 60 * 1000)
assert.equal(waitCookie?.options.path, '/')
assert.equal(waitCookie?.options.sameSite, 'none')
assert.equal(waitCookie?.options.secure, true)
Summary by CodeRabbit
Bug Fixes
Documentation