Destroy consumers of rejected duplicates, keep state updaters pure

src/__tests__/app.test.tsx

@@ -1,7 +1,7 @@
 import { fireEvent, render, screen, waitFor, within } from '@testing-library/react'
 import userEvent from '@testing-library/user-event'
-import { type ChangeEvent } from 'react'
-import { SourceMapGenerator } from 'source-map'
+import { type ChangeEvent, StrictMode } from 'react'
+import { SourceMapConsumer, SourceMapGenerator } from 'source-map'
 import { describe, expect, test, vi } from 'vitest'
 
 import App from '../app.tsx'
@@ -219,6 +219,10 @@ describe('source maps', () => {
     // Mock the click method to test that it gets called by the keydown handler
     const clickSpy = vi.spyOn(fileUploadButton, 'click')
 
+    // Other keys must not open the file selector
+    fireEvent.keyDown(fileUploadButton, { code: 'KeyA' })
+    expect(clickSpy).not.toHaveBeenCalled()
+
     // Fire the keydown event with Enter
     fireEvent.keyDown(fileUploadButton, { code: 'Enter' })
 
@@ -329,6 +333,84 @@ describe('source maps', () => {
   })
 })
 
+describe('source map lifecycle', () => {
+  // destroy() lives on the prototype of the concrete consumer class, which the
+  // library does not export, so grab it from a throwaway instance.
+  async function spyOnConsumerDestroy() {
+    const consumer = await new SourceMapConsumer(regular.sourcemaps[0].content)
+    const prototype = Object.getPrototypeOf(consumer) as { destroy: () => void }
+    consumer.destroy()
+    return vi.spyOn(prototype, 'destroy')
+  }
+
+  test('destroys the consumer of a rejected duplicate source map', async () => {
+    const destroySpy = await spyOnConsumerDestroy()
+
+    render(<App />)
+    const user = userEvent.setup()
+
+    const sourceMapFileInput = screen.getByLabelText(/choose files/i)
+    const makeFile = () => new File([regular.sourcemaps[0].content], regular.sourcemaps[0].fileName)
+
+    await user.upload(sourceMapFileInput, makeFile())
+    await screen.findByRole('list', { name: /sourcemaps list/i })
+    expect(destroySpy).not.toHaveBeenCalled()
+
+    await user.upload(sourceMapFileInput, makeFile())
+    await waitFor(() => expect(destroySpy).toHaveBeenCalledOnce())
+
+    const sourcemapList = screen.getByRole('list', { name: /sourcemaps list/i })
+    expect(within(sourcemapList).getAllByRole('listitem')).toHaveLength(1)
+
+    destroySpy.mockRestore()
+  })
+
+  test('adds a source map only once when identical files are selected together', async () => {
+    const destroySpy = await spyOnConsumerDestroy()
+
+    render(<App />)
+    const user = userEvent.setup()
+
+    const sourceMapFileInput = screen.getByLabelText(/choose files/i)
+    const files = [
+      new File([regular.sourcemaps[0].content], 'copy-one.js.map'),
+      new File([regular.sourcemaps[0].content], 'copy-two.js.map'),
+    ]
+
+    await user.upload(sourceMapFileInput, files)
+
+    const sourcemapList = await screen.findByRole('list', { name: /sourcemaps list/i })
+    expect(within(sourcemapList).getAllByRole('listitem')).toHaveLength(1)
+    expect(destroySpy).toHaveBeenCalledOnce()
+
+    destroySpy.mockRestore()
+  })
+
+  test('destroys the consumer exactly once when deleting under StrictMode', async () => {
+    const destroySpy = await spyOnConsumerDestroy()
+
+    render(
+      <StrictMode>
+        <App />
+      </StrictMode>,
+    )
+    const user = userEvent.setup()
+
+    const sourceMapFileInput = screen.getByLabelText(/choose files/i)
+    const file = new File([regular.sourcemaps[0].content], regular.sourcemaps[0].fileName)
+    await user.upload(sourceMapFileInput, file)
+
+    const deleteButton = await screen.findByRole('button', { name: 'delete' })
+    await user.click(deleteButton)
+
+    const sourcemapList = screen.queryByRole('list', { name: /sourcemaps list/i })
+    expect(sourcemapList).not.toBeInTheDocument()
+    expect(destroySpy).toHaveBeenCalledOnce()
+
+    destroySpy.mockRestore()
+  })
+})
+
 describe('column numbers', () => {
   // Generated line 1 has mappings at 0-based columns 0, 10, and 11 which lead
   // to different original lines, so an off-by-one in the column conversion

src/lib.ts

@@ -25,7 +25,7 @@ export function transform(sourceMaps: SourceMap[], stackTrace: null | StackTrace
 function tryGetOriginalPosition(
   stackFrame: StackFrame,
   bindings: Record<string, SourceMap>,
-): null | NullableMappedPosition {
+): null | OriginalPosition {
   const { column, file, line } = toUnifiedPosition(stackFrame)
 
   if (!file || !bindings[file] || line == null || line < 1 || column == null) {
@@ -38,15 +38,29 @@ function tryGetOriginalPosition(
     line,
   })
 
-  return result.source == null ? null : result
+  return isResolvedPosition(result) ? result : null
+}
+
+interface OriginalPosition {
+  column: number
+  line: number
+  name: null | string
+  source: string
+}
+
+// A found mapping always carries line and column along with the source.
+function isResolvedPosition(
+  position: NullableMappedPosition,
+): position is NullableMappedPosition & OriginalPosition {
+  return position.source != null
 }
 
 function generateStackTraceLine(position: UnifiedPosition) {
   const { column, file, line, method } = position
   return `  at${method ? ` ${method}` : ''} (${file}:${line}:${column})`
 }
 
-function toUnifiedPosition(position: NullableMappedPosition | StackFrame): UnifiedPosition {
+function toUnifiedPosition(position: OriginalPosition | StackFrame): UnifiedPosition {
   if (isStackFrame(position)) {
     return {
       column: position.column,
@@ -58,14 +72,14 @@ function toUnifiedPosition(position: NullableMappedPosition | StackFrame): Unifi
 
   return {
     // The source-map library returns 0-based columns, stack traces use 1-based.
-    column: position.column == null ? null : position.column + 1,
+    column: position.column + 1,
     file: position.source,
     line: position.line,
     method: position.name,
   }
 }
 
-function isStackFrame(position: NullableMappedPosition | StackFrame): position is StackFrame {
+function isStackFrame(position: OriginalPosition | StackFrame): position is StackFrame {
   return 'lineNumber' in position
 }
 

src/use-sourcemaps-store.ts

@@ -5,23 +5,36 @@ import { SourceMap } from './source-map.ts'
 export function useSourcemapsStore() {
   const [sourceMaps, setSourceMaps] = useState<SourceMap[]>([])
 
+  // Deduplication and consumer destruction happen outside the state updaters:
+  // updaters must be pure, StrictMode invokes them twice.
   function addSourceMaps(value: (null | SourceMap)[] | SourceMap) {
     const candidates = (Array.isArray(value) ? value : [value]).filter(
       (sm): sm is SourceMap => sm !== null,
     )
-    setSourceMaps(prev => {
-      const toAdd = candidates.filter(sm => !prev.some(s => s.isEqual(sm)))
-      return toAdd.length ? [...prev, ...toAdd] : prev
-    })
+
+    const toAdd: SourceMap[] = []
+
+    for (const candidate of candidates) {
+      if ([...sourceMaps, ...toAdd].some(sm => sm.isEqual(candidate))) {
+        candidate.consumer.destroy()
+      } else {
+        toAdd.push(candidate)
+      }
+    }
+
+    if (toAdd.length) {
+      setSourceMaps(prev => [...prev, ...toAdd])
+    }
   }
 
   function deleteSourceMap(id: number) {
-    setSourceMaps(prev => {
-      const target = prev.find(sm => sm.id === id)
-      if (!target) return prev
-      target.consumer.destroy()
-      return prev.filter(sm => sm.id !== id)
-    })
+    for (const sourceMap of sourceMaps) {
+      if (sourceMap.id === id) {
+        sourceMap.consumer.destroy()
+      }
+    }
+
+    setSourceMaps(prev => prev.filter(sm => sm.id !== id))
   }
 
   return { addSourceMaps, deleteSourceMap, sourceMaps }

vite.config.ts

@@ -21,6 +21,7 @@ export default defineConfig({
       provider: 'v8',
       reporter: ['text', 'json'],
       include: ['src/**/*.{ts,tsx}'],
+      thresholds: { 100: true },
     },
   },
 })