Merge pull request #6462 from Shopify/fix-observe-user-stable

[stable] Set the user ID for Observe

Author: gonzaloriestra

packages/cli-kit/src/private/node/session.test.ts

@@ -460,6 +460,82 @@ describe('getLastSeenUserIdAfterAuth', () => {
   })
 })
 
+describe('setLastSeenUserIdAfterAuth', () => {
+  beforeEach(() => {
+    // Reset the userId before each test
+    setLastSeenUserIdAfterAuth(undefined as any)
+  })
+
+  test('sets the userId correctly', async () => {
+    // Given
+    const testUserId = 'test-user-id-123'
+
+    // When
+    setLastSeenUserIdAfterAuth(testUserId)
+
+    // Then
+    const retrievedUserId = await getLastSeenUserIdAfterAuth()
+    expect(retrievedUserId).toBe(testUserId)
+  })
+
+  test('overwrites existing userId when called multiple times', async () => {
+    // Given
+    const firstUserId = 'first-user-id'
+    const secondUserId = 'second-user-id'
+
+    // When
+    setLastSeenUserIdAfterAuth(firstUserId)
+    let retrievedUserId = await getLastSeenUserIdAfterAuth()
+    expect(retrievedUserId).toBe(firstUserId)
+
+    setLastSeenUserIdAfterAuth(secondUserId)
+    retrievedUserId = await getLastSeenUserIdAfterAuth()
+
+    // Then
+    expect(retrievedUserId).toBe(secondUserId)
+  })
+
+  test('handles empty string userId', async () => {
+    // Given
+    const emptyUserId = ''
+
+    // When
+    setLastSeenUserIdAfterAuth(emptyUserId)
+
+    // Then
+    const retrievedUserId = await getLastSeenUserIdAfterAuth()
+    // Empty string is falsy, so it falls back to 'unknown'
+    expect(retrievedUserId).toBe('unknown')
+  })
+
+  test('handles undefined userId', async () => {
+    // Given & When
+    setLastSeenUserIdAfterAuth(undefined as any)
+
+    // Then
+    const retrievedUserId = await getLastSeenUserIdAfterAuth()
+    // Undefined is falsy, so it falls back to 'unknown'
+    expect(retrievedUserId).toBe('unknown')
+  })
+
+  test('persists userId across multiple getLastSeenUserIdAfterAuth calls', async () => {
+    // Given
+    const testUserId = 'persistent-user-id'
+    setLastSeenUserIdAfterAuth(testUserId)
+
+    // When - Call getLastSeenUserIdAfterAuth multiple times
+    const firstCall = await getLastSeenUserIdAfterAuth()
+    const secondCall = await getLastSeenUserIdAfterAuth()
+    const thirdCall = await getLastSeenUserIdAfterAuth()
+
+    // Then - All calls should return the same userId and fetchSessions should not be called
+    expect(firstCall).toBe(testUserId)
+    expect(secondCall).toBe(testUserId)
+    expect(thirdCall).toBe(testUserId)
+    expect(fetchSessions).not.toHaveBeenCalled()
+  })
+})
+
 describe('getLastSeenAuthMethod', () => {
   beforeEach(() => {
     vi.mocked(getCurrentSessionId).mockReturnValue(undefined)

packages/cli-kit/src/public/node/error-handler.test.ts

@@ -5,23 +5,30 @@ import {mockAndCaptureOutput} from './testing/output.js'
 import * as error from './error.js'
 import {hashString} from '../../public/node/crypto.js'
 import {isLocalEnvironment} from '../../private/node/context/service.js'
+import {getLastSeenUserIdAfterAuth} from '../../private/node/session.js'
 import {settings} from '@oclif/core'
 import {beforeEach, describe, expect, test, vi} from 'vitest'
 
 const onNotify = vi.fn()
+const capturedEventHandler = vi.fn()
 let lastBugsnagEvent: {addMetadata: ReturnType<typeof vi.fn>} | undefined
 
 vi.mock('process')
 vi.mock('@bugsnag/js', () => {
   return {
     default: {
-      notify: (reportedError: any, args: any, callback: any) => {
+      notify: (reportedError: any, eventHandler: any, callback: any) => {
         onNotify(reportedError)
-        if (typeof args === 'function') {
-          const event = {addMetadata: vi.fn()}
-          lastBugsnagEvent = event as any
-          args(event)
+        // Create a mock event to pass to the event handler
+        const mockEvent = {
+          severity: '',
+          unhandled: false,
+          setUser: vi.fn(),
+          addMetadata: vi.fn(),
         }
+        eventHandler(mockEvent)
+        capturedEventHandler(mockEvent)
+        lastBugsnagEvent = mockEvent as any
         callback(null)
       },
       isStarted: () => true,
@@ -33,6 +40,7 @@ vi.mock('./cli.js')
 vi.mock('./context/local.js')
 vi.mock('../../public/node/crypto.js')
 vi.mock('../../private/node/context/service.js')
+vi.mock('../../private/node/session.js')
 vi.mock('@oclif/core', () => ({
   settings: {
     debug: false,
@@ -47,9 +55,11 @@ beforeEach(() => {
   vi.mocked(hashString).mockReturnValue('hashed-macaddress')
   vi.mocked(isUnitTest).mockReturnValue(true)
   onNotify.mockClear()
+  capturedEventHandler.mockClear()
   lastBugsnagEvent = undefined
   vi.mocked(settings).debug = false
   vi.mocked(isLocalEnvironment).mockReturnValue(false)
+  vi.mocked(getLastSeenUserIdAfterAuth).mockResolvedValue('test-user-id-123')
 })
 
 describe('errorHandler', async () => {
@@ -168,6 +178,8 @@ describe('skips sending errors to Bugsnag', () => {
 describe('sends errors to Bugsnag', () => {
   test('processes Error instances as unhandled', async () => {
     const toThrow = new Error('In test')
+    capturedEventHandler.mockClear()
+
     const res = await sendErrorToBugsnag(toThrow, 'unexpected_error')
     expect(res.reported).toEqual(true)
     expect(res.unhandled).toEqual(true)
@@ -217,6 +229,43 @@ describe('sends errors to Bugsnag', () => {
     expect(mockOutput.debug()).toMatch('Error reporting to Bugsnag: Error: Bugsnag is down')
   })
 
+  test('sets user ID from getLastSeenUserIdAfterAuth when reporting to Bugsnag', async () => {
+    // Given
+    capturedEventHandler.mockClear()
+    const testUserId = 'specific-test-user-id'
+    vi.mocked(getLastSeenUserIdAfterAuth).mockResolvedValue(testUserId)
+    const toThrow = new Error('In test')
+
+    // When
+    const res = await sendErrorToBugsnag(toThrow, 'unexpected_error')
+
+    // Then
+    expect(res.reported).toEqual(true)
+    expect(capturedEventHandler).toHaveBeenCalled()
+
+    const mockEvent = capturedEventHandler.mock.calls[0]![0]
+    expect(mockEvent.setUser).toHaveBeenCalledWith(testUserId)
+    expect(mockEvent.severity).toEqual('error')
+    expect(mockEvent.unhandled).toEqual(true)
+  })
+
+  test('handles missing user ID gracefully', async () => {
+    // Given
+    capturedEventHandler.mockClear()
+    vi.mocked(getLastSeenUserIdAfterAuth).mockResolvedValue('unknown')
+    const toThrow = new Error('In test')
+
+    // When
+    const res = await sendErrorToBugsnag(toThrow, 'unexpected_error')
+
+    // Then
+    expect(res.reported).toEqual(true)
+    expect(capturedEventHandler).toHaveBeenCalled()
+
+    const mockEvent = capturedEventHandler.mock.calls[0]![0]
+    expect(mockEvent.setUser).toHaveBeenCalledWith('unknown')
+  })
+
   test('attaches custom metadata with allowed slice_name when startCommand is present', async () => {
     await metadata.addSensitiveMetadata(() => ({
       commandStartOptions: {startTime: Date.now(), startCommand: 'app dev', startArgs: []},

packages/cli-kit/src/public/node/error-handler.ts

@@ -16,6 +16,7 @@ import {outputDebug, outputInfo} from '../../public/node/output.js'
 import {bugsnagApiKey, reportingRateLimit} from '../../private/node/constants.js'
 import {CLI_KIT_VERSION} from '../common/version.js'
 import {runWithRateLimit} from '../../private/node/conf-store.js'
+import {getLastSeenUserIdAfterAuth} from '../../private/node/session.js'
 import {settings, Interfaces} from '@oclif/core'
 import StackTracey from 'stacktracey'
 import Bugsnag, {Event} from '@bugsnag/js'
@@ -125,11 +126,13 @@ export async function sendErrorToBugsnag(
 
     if (report) {
       initializeBugsnag()
+      const userId = await getLastSeenUserIdAfterAuth()
       await new Promise((resolve, reject) => {
         outputDebug(`Reporting ${unhandled ? 'unhandled' : 'handled'} error to Bugsnag: ${reportableError.message}`)
         const eventHandler = (event: Event) => {
           event.severity = 'error'
           event.unhandled = unhandled
+          event.setUser(userId)
           // Attach command metadata so we know which CLI command triggered the error
           const {commandStartOptions} = metadata.getAllSensitiveMetadata()
           const {startCommand} = commandStartOptions ?? {}