fix: login device email undefined values #36880
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: ead7012 The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36880 +/- ##
===========================================
+ Coverage 66.57% 66.59% +0.01%
===========================================
Files 3346 3348 +2
Lines 114645 114626 -19
Branches 21085 21307 +222
===========================================
+ Hits 76329 76333 +4
+ Misses 35625 35608 -17
+ Partials 2691 2685 -6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
yash-rajpal
dismissed
their stale review
Not sure about it, checking with Martin
WalkthroughUpdates session handling to read the User-Agent via a Headers interface and always pass a string to the UA parser. Adds a changeset entry for a patch release documenting a fix for Login Detection emails where User-Agent had been undefined. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Client
participant DDP as DDP Connection
participant Session as Session Handler
participant H as Headers
participant UA as UA Parser
participant Mail as Email Service
User->>Client: Login
Client->>DDP: Establish connection (HTTP headers)
DDP->>Session: onLoginDetected(connection)
Session->>H: Get "user-agent"
H-->>Session: userAgentString | empty string
Session->>UA: parse(userAgentString)
UA-->>Session: device/browser info
Session->>Mail: Compose & send Login Detection email
Mail-->>User: Email delivered
note over Session,UA: Changed: guarded header access and guaranteed string to parser
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.changeset/thin-parents-beam.md (1)
5-5: Polish the release note for clarity and searchability.Consider explicitly naming the HTTP header and removing ambiguity: e.g., ‘Fix login detection emails showing "undefined" when the User-Agent header is missing or unparsable (CORE-1188).’
apps/meteor/ee/server/lib/deviceManagement/session.ts (2)
106-107: Reuse the normalized UA string to populatemailData.userAgent.For consistency and to avoid case/key-path drift, set
mailData.userAgent = userAgentString;here instead of re-reading headers.Replace:
- mailData.userAgent = connection.httpHeaders['user-agent'] || '';
With:
- mailData.userAgent = userAgentString;
75-85: Optional: prevent "undefined" text in email fields.Interpolations like
${browser.name} ${browser.version}and${os.name}can yield"undefined"strings. Consider safe joins and fallbacks (e.g.,t('Unknown')) to guarantee clean output.Example:
const joinDefined = (...parts: Array<string | undefined>): string => parts.filter(Boolean).join(' ').trim(); const mailData = { // ... browserInfo: joinDefined(browser.name, browser.version) || t('Unknown'), osInfo: os.name || t('Unknown'), deviceInfo: joinDefined(device.type || t('Device_Management_Device_Unknown'), device.vendor, device.model, cpu.architecture), // ... };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/thin-parents-beam.md(1 hunks)apps/meteor/ee/server/lib/deviceManagement/session.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/ee/server/lib/deviceManagement/session.ts (1)
6-6: Remove the type-only import of Headers from node-fetch.tsconfig (apps/meteor/tsconfig.json) already includes the DOM lib, so the global Headers type is available — importing from node-fetch is unnecessary and couples this file to node-fetch typings. Remove the import.
Apply this diff:
-import type { Headers } from 'node-fetch';
MartinSchoeler
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Fixes the issue of undefined values in the login detections emails
Before:
After:
Issue(s)
CORE-1188
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Chores