fix: stop a null user profile killing the server from a notification payload - #6321
Draft
hero101 wants to merge 1 commit into
Draft
fix: stop a null user profile killing the server from a notification payload#6321hero101 wants to merge 1 commit into
hero101 wants to merge 1 commit into
Conversation
…payload
Three notification payload builders dereference `user.profile.displayName`
without a guard. Every caller loads the user with `relations: { profile: true }`,
so a null profile means the ROW is incomplete — not that the relation was
forgotten — and one such row makes the builder throw.
That alone would be a failed request. It is worse than that:
`notifyPlatformGlobalRoleChange` invokes its builder WITHOUT `await` and
without a catch (both call sites), so the rejection escapes as an unhandled
rejection, and under Node's default `--unhandled-rejections=throw` the process
terminates.
Observed twice during live verification of
workspace#027-platform-role-redesign — once via platform role revocation, once
via `createDiscussion` — each time taking the whole server down mid-run.
- `resolveUserDisplayName()` falls back to the user's name, then their email,
so a notification still carries a usable identifier instead of failing.
Applied at all three sites (`buildPlatformUserRemovedNotificationPayload`,
`getUserPayloadOrFail`, `createUserPayloadFromUser`).
- `notifyPlatformGlobalRoleChange` wraps its dispatch in try/catch and logs
through Winston. Notifying is best-effort by design; with un-awaited call
sites it must never be able to take the process down, whatever the adapter
does next.
Pre-existing on develop and not introduced by 027 — that feature only makes
the role-revocation path hot enough to hit it reliably.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
📊 PR Metrics SummaryTitle: fix: stop a null user profile killing the server from a notification payload
Flags
Thresholds{
"critical_loc": 200,
"simple_loc": 100,
"file_count": 10
} |
|
Schema Diff Summary: No blocking changes
Baseline branch: develop |
Contributor
There was a problem hiding this comment.
Pull request overview
Prevents notification payload generation and best-effort notification dispatch from crashing the NestJS server when a User has a null profile (and when notification dispatch fails more generally), addressing an unhandled-rejection process termination scenario.
Changes:
- Added
resolveUserDisplayName()in the external notification adapter and used it at the three previously unguardeduser.profile.displayNamedereferences. - Wrapped
platformGlobalRoleChangeddispatch innotifyPlatformGlobalRoleChange()withtry/catchand Winston logging to prevent unhandled rejections from terminating the process.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/services/adapters/notification-external-adapter/notification.external.adapter.ts | Adds a displayName fallback helper and applies it to three payload builders that previously dereferenced user.profile.displayName unguarded. |
| src/platform/platform-role/platform.role.resolver.mutations.ts | Adds Winston logger injection and wraps notification dispatch in try/catch to prevent unhandled rejection process termination. |
Comment on lines
+1279
to
+1284
| private resolveUserDisplayName(user: IUser): string { | ||
| return ( | ||
| user.profile?.displayName || | ||
| `${user.firstName ?? ''} ${user.lastName ?? ''}`.trim() || | ||
| user.email | ||
| ); |
Comment on lines
+202
to
+218
| // Both call sites above invoke this WITHOUT `await`, so anything this | ||
| // rejects with becomes an unhandled rejection — and under Node's default | ||
| // `--unhandled-rejections=throw` that terminates the process rather than | ||
| // failing the request. Observed twice in live verification: a user row | ||
| // with a null profile made the payload builder throw, and the server | ||
| // exited mid-run. Notifying is best-effort by design; it must never be | ||
| // able to take the process down, whatever the adapter does next. | ||
| try { | ||
| await this.notificationPlatformAdapter.platformGlobalRoleChanged( | ||
| notificationInput | ||
| ); | ||
| } catch (error: any) { | ||
| this.logger.error( | ||
| `Unable to dispatch platform global role change notification (user=${user.id}, role=${role}, type=${type}): ${error?.message}`, | ||
| error?.stack, | ||
| LogContext.NOTIFICATIONS | ||
| ); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Three notification payload builders dereference
user.profile.displayNameunguarded. A user row with a null profile makes the builder throw — and becausenotifyPlatformGlobalRoleChangecalls its builder withoutawaitand without a catch, that rejection escapes as an unhandled rejection. Under Node's default--unhandled-rejections=throw, the process terminates.Observed twice during live verification of
workspace#027-platform-role-redesign— once via platform role revocation, once viacreateDiscussion— each time taking the whole server down mid-run.Why it isn't just a missing relation
Every caller loads the user with
relations: { profile: true }. A nullprofiletherefore means the row itself is incomplete, not that someone forgot to request it. Guarding is the correct response; adding the relation would change nothing.Changes
notification.external.adapter.tsresolveUserDisplayName()— falls back to the user's name, then their email, so a notification still carries a usable identifier instead of failing to send. Applied at all three sites:buildPlatformUserRemovedNotificationPayload,getUserPayloadOrFail,createUserPayloadFromUser.platform.role.resolver.mutations.tsnotifyPlatformGlobalRoleChangewraps its dispatch in try/catch, logging through Winston. Notifying is best-effort by design; with un-awaited call sites it must never be able to take the process down, whatever the adapter does next.Both halves matter independently: the guard fixes the trigger seen so far, the try/catch stops any future notifier failure from escalating from "notification not sent" to "server exited".
Provenance
Pre-existing on
develop; not introduced by 027. That feature only makes the role-revocation path hot enough to hit it reliably. Filed separately rather than inside the 27-commit role-redesign PR, which is under a security hold and will not ship soon.Verification
pnpm lintclean (tsc + biome, 3096 files). No test added — reproducing it needs a user row with a null profile, which is a data state rather than something the suite can construct through the API; happy to add one against a seeded fixture if you'd prefer.🤖 Generated with Claude Code
https://claude.ai/code/session_01HZSiMjuPXnj2Un3pd8WKC3