fix: correct five CSV handling defects in bulk send

The bulk send rebuild shipped several parsing bugs that silently
produced wrong results rather than failing loudly.

Columns were read from the first parsed row instead of the header.
papaparse only assigns keys for values actually present in a row, so a
short first row hid every column after it: the phone column could not
be selected and the file was unusable. Read results.meta.fields.

Row-level parse errors were discarded. Only fatal stream failures were
reported, so a mis-delimited file parsed into one mangled column and
looked fine. Surface them as a warning.

The plan row cap was checked once at drop time and skipped entirely
while the subscription was still loading, so an over-cap file passed on
a limited plan and nothing revalidated it. The cap is now derived, so
it applies whenever the limit resolves.

Rejected files returned silently, leaving a dropped .xlsx with no
feedback. Report the rejection.

The preview index was left pointing into the old recipient list when
the phone column changed, hiding the preview and disabling its own Next
button. It is now clamped and reset.

Also count segments from the rendered message rather than falling back
to the raw template, which counted the placeholder literals.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
isra el
2026-07-19 01:37:50 +03:00
parent 05143b8752
commit 6da80ac4e7
3 changed files with 218 additions and 48 deletions

View File

@@ -1,4 +1,5 @@
import { describe, expect, it } from 'vitest'
import Papa from 'papaparse'
import {
buildRecipientPlan,
detectRecipientColumn,
@@ -154,3 +155,43 @@ describe('formatFileSize', () => {
expect(formatFileSize(900)).toBe('900 B')
})
})
// The upload step reads its column list straight from papaparse. These pin the
// specific parser behaviour that made reading keys off the first data row
// wrong, so a future refactor back to Object.keys(data[0]) fails here.
describe('csv column extraction', () => {
const csv = 'name,phone,city\nAlice\nBob,+14155550101,Denver'
it('meta.fields keeps every header even when the first row is short', () => {
const parsed = Papa.parse<Record<string, string>>(csv, {
header: true,
skipEmptyLines: true,
})
expect(parsed.meta.fields).toEqual(['name', 'phone', 'city'])
})
it('the first data row does not list every header', () => {
const parsed = Papa.parse<Record<string, string>>(csv, {
header: true,
skipEmptyLines: true,
})
// This is the bug: papaparse only assigns keys for values present in the
// row, so a short first row silently hides real columns from the mapping
// dropdown and the phone column becomes unselectable.
expect(Object.keys(parsed.data[0])).toEqual(['name'])
expect(Object.keys(parsed.data[0])).not.toContain('phone')
})
it('still detects the phone column from the full header list', () => {
const parsed = Papa.parse<Record<string, string>>(csv, {
header: true,
skipEmptyLines: true,
})
expect(detectRecipientColumn(parsed.meta.fields ?? [])).toBe('phone')
// Whereas the short first row offers nothing to detect.
expect(detectRecipientColumn(Object.keys(parsed.data[0]))).toBeUndefined()
})
})

View File

@@ -1,7 +1,7 @@
'use client'
import { useCallback, useMemo, useRef, useState } from 'react'
import { useDropzone } from 'react-dropzone'
import { useDropzone, type FileRejection } from 'react-dropzone'
import Papa from 'papaparse'
import {
AlertCircle,
@@ -66,6 +66,7 @@ export default function BulkSend() {
const [simSubscriptionId, setSimSubscriptionId] = useState<number>()
const [previewIndex, setPreviewIndex] = useState(0)
const [fileError, setFileError] = useState<string | null>(null)
const [fileWarning, setFileWarning] = useState<string | null>(null)
const templateRef = useRef<HTMLTextAreaElement>(null)
const { data: devices } = useDevices()
@@ -89,53 +90,94 @@ export default function BulkSend() {
setFileName(null)
setRecipientColumn('')
setPreviewIndex(0)
setFileWarning(null)
}
const onDrop = useCallback(
(accepted: File[]) => {
const file = accepted[0]
if (!file) return
// Derived rather than checked once at drop time. The subscription can still
// be loading when a file lands, in which case maxRows is undefined and the
// old imperative check was skipped entirely and never revisited, so an
// over-cap file sailed through on a limited plan.
const rowCapExceeded = maxRows !== undefined && rows.length > maxRows
const rowCapUnknown = rows.length > 0 && maxRows === undefined
if (file.size > MAX_FILE_SIZE) {
const handleRecipientColumnChange = (value: string) => {
setRecipientColumn(value)
// plan.valid is rebuilt from the new column, so an index into the old
// list can point past the end of the new one.
setPreviewIndex(0)
}
const onDrop = useCallback((accepted: File[], rejections: FileRejection[]) => {
const file = accepted[0]
// react-dropzone still calls onDrop when every file was filtered out by
// `accept`, so returning silently here left a rejected drop with no
// feedback at all: nothing happened and nothing said why.
if (!file) {
const rejected = rejections[0]
if (rejected) {
setFileError(
`That file is ${formatFileSize(file.size)}. The limit is ${formatFileSize(MAX_FILE_SIZE)}.`
rejections.length > 1
? 'Please drop a single CSV file.'
: `"${rejected.file.name}" is not a CSV. Export your spreadsheet as CSV and try again.`
)
return
}
return
}
Papa.parse<CsvRow>(file, {
header: true,
skipEmptyLines: true,
complete: (results) => {
const parsed = (results.data ?? []).filter(Boolean)
if (file.size > MAX_FILE_SIZE) {
setFileError(
`That file is ${formatFileSize(file.size)}. The limit is ${formatFileSize(MAX_FILE_SIZE)}.`
)
return
}
if (parsed.length === 0) {
setFileError('That file has no rows. Check it has a header row.')
resetFile()
return
}
Papa.parse<CsvRow>(file, {
header: true,
skipEmptyLines: true,
complete: (results) => {
const parsed = (results.data ?? []).filter(Boolean)
if (maxRows !== undefined && parsed.length > maxRows) {
setFileError(
`That file has ${parsed.length.toLocaleString()} rows. Your plan allows ${maxRows.toLocaleString()} per bulk send.`
)
resetFile()
return
}
if (parsed.length === 0) {
setFileError('That file has no rows. Check it has a header row.')
resetFile()
return
}
const headers = Object.keys(parsed[0]).filter(Boolean)
setRows(parsed)
setColumns(headers)
setFileName(file.name)
setRecipientColumn(detectRecipientColumn(headers) ?? '')
setPreviewIndex(0)
setFileError(null)
},
error: () => setFileError('That file could not be read as CSV.'),
})
},
[maxRows]
)
// Take the header papaparse actually parsed. Reading keys off the
// first data row instead loses every column that row happened to
// leave blank, because papaparse only assigns keys for values that
// are present.
const headers = (results.meta?.fields ?? []).filter(Boolean)
if (headers.length === 0) {
setFileError('That file has no header row, so columns cannot be mapped.')
resetFile()
return
}
setRows(parsed)
setColumns(headers)
setFileName(file.name)
setRecipientColumn(detectRecipientColumn(headers) ?? '')
setPreviewIndex(0)
setFileError(null)
// Row-level problems are reported here, not through `error`, which
// only fires on a fatal stream failure. A mis-delimited file parses
// "successfully" into one mangled column, so this is worth saying.
const problems = results.errors ?? []
setFileWarning(
problems.length > 0
? `${problems.length} row${problems.length === 1 ? '' : 's'} did not parse cleanly (first problem on row ${
(problems[0].row ?? 0) + 1
}: ${problems[0].message}). Check the delimiter and quoting if the columns below look wrong.`
: null
)
},
error: () => setFileError('That file could not be read as CSV.'),
})
}, [])
const { getRootProps, getInputProps, isDragActive } = useDropzone({
onDrop,
@@ -161,14 +203,30 @@ export default function BulkSend() {
? (selectedDevice as any).simInfo.sims
: []
const previewRow = plan.valid[previewIndex]
// Clamped, so the preview can never be stranded past the end of a shorter
// list. Anything that shrinks plan.valid (changing the phone column, most
// obviously) would otherwise hide the preview and disable its own Next
// button, leaving Previous as the only way back.
const safePreviewIndex = Math.min(
previewIndex,
Math.max(0, plan.valid.length - 1)
)
const previewRow = plan.valid[safePreviewIndex]
const previewMessage = previewRow
? renderTemplate(template, previewRow.data)
: ''
const segments = getSegmentInfo(previewMessage || template)
// Counted from the rendered message, never the raw template: falling back to
// the template counts the literal "{{ name }}" placeholders and misstates
// the segment cost.
const segments = getSegmentInfo(previewMessage)
const hasFile = rows.length > 0
const mapped = hasFile && Boolean(recipientColumn) && plan.valid.length > 0
const mapped =
hasFile &&
!rowCapExceeded &&
!rowCapUnknown &&
Boolean(recipientColumn) &&
plan.valid.length > 0
const composed = mapped && template.trim().length > 0 && Boolean(deviceId)
const {
@@ -351,6 +409,26 @@ export default function BulkSend() {
<AlertDescription>{fileError}</AlertDescription>
</Alert>
)}
{rowCapExceeded && (
<Alert variant='destructive'>
<AlertCircle className='h-4 w-4' />
<AlertTitle>That file is over your plan limit</AlertTitle>
<AlertDescription>
It has {rows.length.toLocaleString()} rows and your plan allows{' '}
{maxRows!.toLocaleString()} per bulk send. Remove the extra rows
or upgrade your plan.
</AlertDescription>
</Alert>
)}
{fileWarning && !fileError && (
<Alert>
<AlertCircle className='h-4 w-4' />
<AlertTitle>Some rows did not parse cleanly</AlertTitle>
<AlertDescription>{fileWarning}</AlertDescription>
</Alert>
)}
</StepShell>
{/* 2. Map */}
@@ -364,7 +442,10 @@ export default function BulkSend() {
<div className='grid gap-4 sm:grid-cols-2'>
<div className='space-y-1.5'>
<Label htmlFor='recipient-column'>Phone number column</Label>
<Select value={recipientColumn} onValueChange={setRecipientColumn}>
<Select
value={recipientColumn}
onValueChange={handleRecipientColumnChange}
>
<SelectTrigger id='recipient-column'>
<SelectValue placeholder='Select a column' />
</SelectTrigger>
@@ -538,23 +619,25 @@ export default function BulkSend() {
size='icon'
className='h-6 w-6'
aria-label='Previous recipient'
disabled={previewIndex === 0}
onClick={() => setPreviewIndex((i) => Math.max(0, i - 1))}
disabled={safePreviewIndex === 0}
onClick={() =>
setPreviewIndex(Math.max(0, safePreviewIndex - 1))
}
>
<ChevronLeft className='h-3.5 w-3.5' />
</Button>
<span className='text-xs tabular-nums text-muted-foreground'>
{previewIndex + 1} / {plan.valid.length}
{safePreviewIndex + 1} / {plan.valid.length}
</span>
<Button
variant='ghost'
size='icon'
className='h-6 w-6'
aria-label='Next recipient'
disabled={previewIndex >= plan.valid.length - 1}
disabled={safePreviewIndex >= plan.valid.length - 1}
onClick={() =>
setPreviewIndex((i) =>
Math.min(plan.valid.length - 1, i + 1)
setPreviewIndex(
Math.min(plan.valid.length - 1, safePreviewIndex + 1)
)
}
>

View File

@@ -154,4 +154,50 @@ test.describe('bulk send (mocked API, no real backend)', () => {
await expect(page.getByText(/Unknown column reference/)).toBeVisible()
await expect(page.getByText(/no column named "nickname"/)).toBeVisible()
})
// The columns used to be read off the first parsed row rather than the
// header. papaparse only assigns keys for values actually present, so a
// short first row made every column after it vanish: the phone column could
// not be selected and the whole file was unusable.
test('keeps every column when the first data row is short', async ({
page,
context,
}) => {
await authenticate(context)
await mockApi(page)
await page.goto('/dashboard/messaging/bulk')
await uploadCsv(
page,
[
'name,phone,order_id',
'Missing Everything Else',
'Bob Martinez,+16475550187,ORD-1044',
].join('\n')
)
await expect(page.getByText('2 rows, 3 columns')).toBeVisible()
// The real proof: the phone column was still detected and mapped, which is
// impossible if the column list came from that first row.
await expect(page.getByLabel('Phone number column')).toContainText('phone')
await expect(page.getByText('1 will receive a message')).toBeVisible()
})
test('explains why a non-CSV file was rejected', async ({ page, context }) => {
await authenticate(context)
await mockApi(page)
await page.goto('/dashboard/messaging/bulk')
// Rejected drops used to return silently, so nothing happened and nothing
// said why.
await page.locator('input[type="file"]').setInputFiles({
name: 'contacts.xlsx',
mimeType:
'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet',
buffer: Buffer.from('not a csv'),
})
await expect(page.getByText(/is not a CSV/)).toBeVisible()
})
})