Wire shared chat lifecycle into security notifications#27
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughShare participant lifecycle events (invite accepted, participant left/removed) are now tracked as security events with push notifications. Push token registration extended to support shared chats via ownerUserId/shareId query parameters. Event recording and delivery centralized in relay; removed participants have active client sockets closed. ChangesShare Security Events and Push Token Registration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
app/lib/push-registration-url.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. app/lib/push-registration.test.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@app/lib/push-registration-url.ts`:
- Around line 3-7: The URL builder adds ownerUserId and shareId independently,
which allows invalid single-parameter combinations; update the function in
app/lib/push-registration-url.ts to validate the share scope as an atomic pair:
require that options.ownerUserId and options.shareId are either both present or
both absent before calling url.searchParams.set, and if only one is provided
throw or return an explicit error (or omit both) to avoid constructing a URL the
relay will reject; locate the logic around the new URL(...) to implement this
check.
In `@relay/src/relay-object.ts`:
- Around line 439-446: When storing a push token with the new composite key
(tokenKey = userId ? `${userId}:${deviceId}` : deviceId) ensure any stale legacy
entry keyed by the plain deviceId is removed to avoid duplicate notifications:
after computing tokenKey in the update block for state.pushTokens (where userId,
deviceId, token, platform, createdAt, updatedAt are set), if userId is present
and state.pushTokens[deviceId] exists, delete state.pushTokens[deviceId];
preserve createdAt semantics by using state.pushTokens[tokenKey]?.createdAt when
creating the new entry. This cleanup should occur atomically in the same update
flow that writes the new tokenKey.
- Around line 755-761: The current branch that handles options.deliverToUserId
calls deliverSecurityAlert with pushTokens: [], which prevents push
notifications for participant-targeted alerts; update the branch so it fetches
the target user's push tokens (same way you do for options.ownerUserId) and pass
those tokens into deliverSecurityAlert instead of an empty array—locate the
conditional using options.deliverToUserId and options.ownerUserId and replace
the hardcoded pushTokens: [] with the actual user's push token list retrieved
from the user/token lookup used elsewhere in this module.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 17e9cc2e-7db6-4da5-a478-afbd1218b4ef
📒 Files selected for processing (10)
app/app/(main)/_layout.tsxapp/lib/push-registration-url.tsapp/lib/push-registration.test.tsapp/lib/push-registration.tsapp/stores/security.tsrelay/src/index.tsrelay/src/relay-object.tsrelay/src/security-delivery.tsrelay/src/security.test.tsrelay/src/security.ts
Summary
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Tests