Open
Conversation
Ref https://linear.app/ghost/issue/BER-3381/add-bulk-member-importing-flow Somewhere in the original port, importing members was lost. This adds it back.
Contributor
WalkthroughThis pull request adds a CSV-based member import feature. It introduces a new ImportMembersModal React component implementing a multi-stage workflow (file pick/drag-and-drop, CSV parsing, automatic field-type detection, interactive field mapping UI, label assignment, upload, and result handling). A new field-mapping module provides CSV parsing/unparsing, type detection, mapping utilities, and error formatting. The modal is exported from bulk-action-modals and integrated into the members actions dropdown. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/posts/src/views/members/components/bulk-action-modals/import-members/field-mapping.ts (1)
206-213: Trimming values may discard significant whitespace in quoted fields.CSV fields within quotes typically preserve whitespace, but
current.trim()is applied regardless. For example," John "would becomeJohn. If preserving intentional whitespace in member notes or names is needed, consider trimming only unquoted values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/components/bulk-action-modals/import-members/field-mapping.ts` around lines 206 - 213, The CSV parser currently calls current.trim() unconditionally when finalizing a field, which removes intentional whitespace inside quoted fields; change the push logic to only trim when the field is not quoted (use the inQuotes flag) — i.e., when encountering a comma or newline (the blocks using inQuotes, current and row.push), push row.push(inQuotes ? current : current.trim()), then reset current and reset/maintain inQuotes appropriately so quoted-field whitespace is preserved.apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsx (2)
60-74: Consider consolidating state withuseReducer.The component has 15 separate
useStatehooks. For complex multi-stage workflows like this, a singleuseReducerwith typed actions can make state transitions more predictable and easier to reason about. However, the current approach is functional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsx` around lines 60 - 74, The component uses many separate useState hooks (state, file, fileData, mapping, selectedLabels, dataPreviewIndex, mappingError, showMappingErrors, importResponse, errorMessage, errorHeader, showTryAgainButton, dragOver, fileError, fileInputRef) making transitions hard to track; replace these with a single useReducer by defining an ImportState shape that includes all current pieces (status, file, fileData, mapping, selectedLabels, etc.), create a discriminated Action union (e.g., SET_STATUS, SET_FILE, SET_FILE_DATA, SET_MAPPING_ERROR, TOGGLE_MAPPING_ERRORS, SET_IMPORT_RESPONSE, SET_ERROR, RESET, etc.), implement a reducer function (handle actions to update the appropriate fields) and swap all setX calls to dispatch actions and replace fileInputRef with useRef kept outside the reducer or included as part of state if needed; ensure types for state and actions are exported/typed and update handlers (onDrop/onChange/submit) to dispatch the new actions accordingly.
500-522: Label selector allows only single label, but state is an array.The
selectedLabelsstate isstring[], but the UI only allows selecting one label at a time. If multi-label support is intended for the future, this is fine. Otherwise, simplifying toselectedLabel: string | nullwould be clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsx` around lines 500 - 522, The label UI uses a single-select but the state is string[] (selectedLabels) — simplify to a single string (e.g., selectedLabel: string | null) to match the Select usage: replace selectedLabels with selectedLabel, change setSelectedLabels to setSelectedLabel, update the Select value to selectedLabel ?? '__none__', adjust onValueChange to setSelectedLabel(val === '__none__' ? null : val), and update any other references in the ImportMembersModal component (or helper functions) that currently expect an array to use the single string (or convert to array where needed).apps/posts/src/views/members/components/members-actions.tsx (1)
220-224: Consider wiringonCompleteto refresh the member list.The
ImportMembersModalaccepts an optionalonCompletecallback (invoked when import finishes), but it's not provided here. After a successful import, users may expect to see newly imported members without manually refreshing.💡 Example integration
If there's a query invalidation or refetch mechanism available:
<ImportMembersModal labels={labels} open={showImportModal} onOpenChange={setShowImportModal} + onComplete={() => { + // Trigger member list refresh if needed + }} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/components/members-actions.tsx` around lines 220 - 224, ImportMembersModal is missing the optional onComplete handler so newly imported members aren't shown automatically; wire its onComplete to close the modal (use setShowImportModal(false)) and trigger the members list refresh by calling your existing refetch/invalidate mechanism (e.g., refetchMembers() or queryClient.invalidateQueries('members')) inside that handler so the UI updates after import completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsx`:
- Around line 109-130: Add an onerror handler to the FileReader used in the
importMembersModal so file read failures surface to the user: in the block that
creates reader (where reader.onload sets parseCSV, setFileData, detectFieldTypes
and sets mapping/mappingError) also set reader.onerror to capture the error
event and call setMappingError with a user-friendly message (include
error.message if available) and clear any partial state (e.g., setFileData([])
or setMapping(null)); ensure the handler references the same reader and state
setters used in the existing code (parseCSV, setFileData, setMapping,
setMappingError, MembersFieldMapping).
- Around line 295-309: The blob URL created with URL.createObjectURL in the
import flow is never revoked, causing a memory leak; update the logic around
unparseErrorCSV / errorCsvUrl and setImportResponse to revoke any existing blob
URL before creating a new one and also revoke the URL in the component cleanup
and in the reset function: when building errorCsvUrl check if
importResponse.errorCsvUrl exists and call
URL.revokeObjectURL(importResponse.errorCsvUrl) before creating a new blob URL,
and add a useEffect cleanup that revokes importResponse.errorCsvUrl on unmount
(and ensure reset() also revokes and clears errorCsvUrl from state). Ensure you
reference and update the state field setImportResponse / importResponse, the
errorCsvUrl variable generation, the reset function, and the component unmount
cleanup.
In
`@apps/posts/src/views/members/components/bulk-action-modals/import-members/field-mapping.ts`:
- Around line 80-115: The sampleData function currently produces 25 samples (10
head + 5 middle + 10 tail) and starts validatedSet as [{}], creating an extra
empty row; change sample generation so the total number of sampled rows equals
validationSampleSize and remove the initial empty object. Specifically, in
sampleData: initialize validatedSet = []; when nonEmptyKeyEntries.length >
validationSampleSize compute headSize = Math.floor(validationSampleSize/3),
tailSize = headSize, middleSize = validationSampleSize - headSize - tailSize,
slice head = data.slice(0, headSize), tail = data.slice(-tailSize), compute a
centered middle slice of length middleSize around Math.floor(data.length/2),
then combine [...head, ...middle, ...tail] (which will total
validationSampleSize) and assign values into validatedSet rows (creating rows as
needed) for the given key; also ensure when nonEmptyKeyEntries.length <=
validationSampleSize you still populate validatedSet without leaving holes.
---
Nitpick comments:
In
`@apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsx`:
- Around line 60-74: The component uses many separate useState hooks (state,
file, fileData, mapping, selectedLabels, dataPreviewIndex, mappingError,
showMappingErrors, importResponse, errorMessage, errorHeader,
showTryAgainButton, dragOver, fileError, fileInputRef) making transitions hard
to track; replace these with a single useReducer by defining an ImportState
shape that includes all current pieces (status, file, fileData, mapping,
selectedLabels, etc.), create a discriminated Action union (e.g., SET_STATUS,
SET_FILE, SET_FILE_DATA, SET_MAPPING_ERROR, TOGGLE_MAPPING_ERRORS,
SET_IMPORT_RESPONSE, SET_ERROR, RESET, etc.), implement a reducer function
(handle actions to update the appropriate fields) and swap all setX calls to
dispatch actions and replace fileInputRef with useRef kept outside the reducer
or included as part of state if needed; ensure types for state and actions are
exported/typed and update handlers (onDrop/onChange/submit) to dispatch the new
actions accordingly.
- Around line 500-522: The label UI uses a single-select but the state is
string[] (selectedLabels) — simplify to a single string (e.g., selectedLabel:
string | null) to match the Select usage: replace selectedLabels with
selectedLabel, change setSelectedLabels to setSelectedLabel, update the Select
value to selectedLabel ?? '__none__', adjust onValueChange to
setSelectedLabel(val === '__none__' ? null : val), and update any other
references in the ImportMembersModal component (or helper functions) that
currently expect an array to use the single string (or convert to array where
needed).
In
`@apps/posts/src/views/members/components/bulk-action-modals/import-members/field-mapping.ts`:
- Around line 206-213: The CSV parser currently calls current.trim()
unconditionally when finalizing a field, which removes intentional whitespace
inside quoted fields; change the push logic to only trim when the field is not
quoted (use the inQuotes flag) — i.e., when encountering a comma or newline (the
blocks using inQuotes, current and row.push), push row.push(inQuotes ? current :
current.trim()), then reset current and reset/maintain inQuotes appropriately so
quoted-field whitespace is preserved.
In `@apps/posts/src/views/members/components/members-actions.tsx`:
- Around line 220-224: ImportMembersModal is missing the optional onComplete
handler so newly imported members aren't shown automatically; wire its
onComplete to close the modal (use setShowImportModal(false)) and trigger the
members list refresh by calling your existing refetch/invalidate mechanism
(e.g., refetchMembers() or queryClient.invalidateQueries('members')) inside that
handler so the UI updates after import completes.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsxapps/posts/src/views/members/components/bulk-action-modals/import-members/field-mapping.tsapps/posts/src/views/members/components/bulk-action-modals/index.tsapps/posts/src/views/members/components/members-actions.tsx
Comment on lines
+109
to
+130
| const reader = new FileReader(); | ||
| reader.onload = (e) => { | ||
| const text = e.target?.result as string; | ||
| const data = parseCSV(text); | ||
| setFileData(data); | ||
|
|
||
| if (data.length > 0) { | ||
| const detectedMapping = detectFieldTypes(data); | ||
| const fieldMapping = new MembersFieldMapping(detectedMapping); | ||
| setMapping(fieldMapping); | ||
|
|
||
| // Validate mapping | ||
| if (!fieldMapping.getKeyByValue('email')) { | ||
| setMappingError('Please map "Email" to one of the fields in the CSV.'); | ||
| } else { | ||
| setMappingError(null); | ||
| } | ||
| } else { | ||
| setMappingError('File is empty, nothing to import. Please select a different file.'); | ||
| } | ||
| }; | ||
| reader.readAsText(file); |
Contributor
There was a problem hiding this comment.
Add error handling for FileReader.
The FileReader lacks an onerror handler. If the file read fails (e.g., file becomes inaccessible, permissions issue), the user won't receive feedback.
🛡️ Proposed fix to add error handling
const reader = new FileReader();
reader.onload = (e) => {
const text = e.target?.result as string;
const data = parseCSV(text);
setFileData(data);
if (data.length > 0) {
const detectedMapping = detectFieldTypes(data);
const fieldMapping = new MembersFieldMapping(detectedMapping);
setMapping(fieldMapping);
// Validate mapping
if (!fieldMapping.getKeyByValue('email')) {
setMappingError('Please map "Email" to one of the fields in the CSV.');
} else {
setMappingError(null);
}
} else {
setMappingError('File is empty, nothing to import. Please select a different file.');
}
};
+ reader.onerror = () => {
+ setFileError('Failed to read the file. Please try again.');
+ setFile(null);
+ };
reader.readAsText(file);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const reader = new FileReader(); | |
| reader.onload = (e) => { | |
| const text = e.target?.result as string; | |
| const data = parseCSV(text); | |
| setFileData(data); | |
| if (data.length > 0) { | |
| const detectedMapping = detectFieldTypes(data); | |
| const fieldMapping = new MembersFieldMapping(detectedMapping); | |
| setMapping(fieldMapping); | |
| // Validate mapping | |
| if (!fieldMapping.getKeyByValue('email')) { | |
| setMappingError('Please map "Email" to one of the fields in the CSV.'); | |
| } else { | |
| setMappingError(null); | |
| } | |
| } else { | |
| setMappingError('File is empty, nothing to import. Please select a different file.'); | |
| } | |
| }; | |
| reader.readAsText(file); | |
| const reader = new FileReader(); | |
| reader.onload = (e) => { | |
| const text = e.target?.result as string; | |
| const data = parseCSV(text); | |
| setFileData(data); | |
| if (data.length > 0) { | |
| const detectedMapping = detectFieldTypes(data); | |
| const fieldMapping = new MembersFieldMapping(detectedMapping); | |
| setMapping(fieldMapping); | |
| // Validate mapping | |
| if (!fieldMapping.getKeyByValue('email')) { | |
| setMappingError('Please map "Email" to one of the fields in the CSV.'); | |
| } else { | |
| setMappingError(null); | |
| } | |
| } else { | |
| setMappingError('File is empty, nothing to import. Please select a different file.'); | |
| } | |
| }; | |
| reader.onerror = () => { | |
| setFileError('Failed to read the file. Please try again.'); | |
| setFile(null); | |
| }; | |
| reader.readAsText(file); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsx`
around lines 109 - 130, Add an onerror handler to the FileReader used in the
importMembersModal so file read failures surface to the user: in the block that
creates reader (where reader.onload sets parseCSV, setFileData, detectFieldTypes
and sets mapping/mappingError) also set reader.onerror to capture the error
event and call setMappingError with a user-friendly message (include
error.message if available) and clear any partial state (e.g., setFileData([])
or setMapping(null)); ensure the handler references the same reader and state
setters used in the existing code (parseCSV, setFileData, setMapping,
setMappingError, MembersFieldMapping).
Comment on lines
+295
to
+309
| const errorCsv = unparseErrorCSV(errorsWithFormattedMessages); | ||
| const errorCsvBlob = new Blob([errorCsv], {type: 'text/csv'}); | ||
| const errorCsvUrl = URL.createObjectURL(errorCsvBlob); | ||
| const importLabel = importData.meta.import_label; | ||
| const errorCsvName = importLabel | ||
| ? `${importLabel.name} - Errors.csv` | ||
| : `Import ${moment().format('YYYY-MM-DD HH:mm')} - Errors.csv`; | ||
|
|
||
| setImportResponse({ | ||
| importedCount, | ||
| errorCount, | ||
| errorCsvUrl, | ||
| errorCsvName, | ||
| errorList: Object.values(errorListMap) | ||
| }); |
Contributor
There was a problem hiding this comment.
Memory leak: Revoke blob URL when component unmounts or on new import.
URL.createObjectURL creates a blob URL that persists until explicitly revoked. Without cleanup, repeated imports will accumulate orphaned blob URLs.
🧹 Proposed fix to revoke blob URL
Add cleanup in the reset function and when component unmounts:
+ // Store ref for cleanup
+ const errorCsvUrlRef = useRef<string | null>(null);
+
const reset = useCallback(() => {
+ // Revoke previous blob URL to prevent memory leak
+ if (errorCsvUrlRef.current) {
+ URL.revokeObjectURL(errorCsvUrlRef.current);
+ errorCsvUrlRef.current = null;
+ }
setState('INIT');
// ... rest of reset
}, []);
+
+ // Cleanup on unmount
+ useEffect(() => {
+ return () => {
+ if (errorCsvUrlRef.current) {
+ URL.revokeObjectURL(errorCsvUrlRef.current);
+ }
+ };
+ }, []);And when creating the URL:
const errorCsvUrl = URL.createObjectURL(errorCsvBlob);
+ // Store for cleanup
+ if (errorCsvUrlRef.current) {
+ URL.revokeObjectURL(errorCsvUrlRef.current);
+ }
+ errorCsvUrlRef.current = errorCsvUrl;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsx`
around lines 295 - 309, The blob URL created with URL.createObjectURL in the
import flow is never revoked, causing a memory leak; update the logic around
unparseErrorCSV / errorCsvUrl and setImportResponse to revoke any existing blob
URL before creating a new one and also revoke the URL in the component cleanup
and in the reset function: when building errorCsvUrl check if
importResponse.errorCsvUrl exists and call
URL.revokeObjectURL(importResponse.errorCsvUrl) before creating a new blob URL,
and add a useEffect cleanup that revokes importResponse.errorCsvUrl on unmount
(and ensure reset() also revokes and clears errorCsvUrl from state). Ensure you
reference and update the state field setImportResponse / importResponse, the
errorCsvUrl variable generation, the reset function, and the component unmount
cleanup.
Comment on lines
+80
to
+115
| function sampleData(data: Record<string, string>[], validationSampleSize = 30): Record<string, string>[] { | ||
| if (!data || data.length <= validationSampleSize) { | ||
| return data; | ||
| } | ||
|
|
||
| const validatedSet: Record<string, string>[] = [{}]; | ||
| const sampleKeys = Object.keys(data[0]); | ||
|
|
||
| sampleKeys.forEach((key) => { | ||
| const nonEmptyKeyEntries = data.filter(entry => entry[key] && entry[key].trim() !== ''); | ||
|
|
||
| if (nonEmptyKeyEntries.length <= validationSampleSize) { | ||
| nonEmptyKeyEntries.forEach((entry, index) => { | ||
| if (!validatedSet[index]) { | ||
| validatedSet[index] = {}; | ||
| } | ||
| validatedSet[index][key] = entry[key]; | ||
| }); | ||
| } else { | ||
| const partitionSize = Math.floor(validationSampleSize / 3); | ||
| const head = data.slice(0, partitionSize); | ||
| const tail = data.slice(data.length - partitionSize, data.length); | ||
| const middleIndex = Math.floor(data.length / 2); | ||
| const middle = data.slice(middleIndex - 2, middleIndex + 3); | ||
|
|
||
| [...head, ...middle, ...tail].forEach((entry, index) => { | ||
| if (!validatedSet[index]) { | ||
| validatedSet[index] = {}; | ||
| } | ||
| validatedSet[index][key] = entry[key]; | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| return validatedSet; | ||
| } |
Contributor
There was a problem hiding this comment.
Sampling logic doesn't match documentation.
The docstring says "30 total" samples, but the implementation samples:
- 10 from head (
partitionSize = floor(30/3) = 10) - 5 from middle (
middleIndex - 2tomiddleIndex + 3) - 10 from tail
This yields 25 samples, not 30. Additionally, the initial validatedSet = [{}] creates an extra empty object at index 0.
🔧 Suggested fix
-function sampleData(data: Record<string, string>[], validationSampleSize = 30): Record<string, string>[] {
+function sampleData(data: Record<string, string>[], validationSampleSize = 30): Record<string, string>[] {
if (!data || data.length <= validationSampleSize) {
return data;
}
- const validatedSet: Record<string, string>[] = [{}];
+ const validatedSet: Record<string, string>[] = [];
const sampleKeys = Object.keys(data[0]);
sampleKeys.forEach((key) => {
const nonEmptyKeyEntries = data.filter(entry => entry[key] && entry[key].trim() !== '');
if (nonEmptyKeyEntries.length <= validationSampleSize) {
nonEmptyKeyEntries.forEach((entry, index) => {
if (!validatedSet[index]) {
validatedSet[index] = {};
}
validatedSet[index][key] = entry[key];
});
} else {
const partitionSize = Math.floor(validationSampleSize / 3);
const head = data.slice(0, partitionSize);
const tail = data.slice(data.length - partitionSize, data.length);
- const middleIndex = Math.floor(data.length / 2);
- const middle = data.slice(middleIndex - 2, middleIndex + 3);
+ const middleStart = Math.floor(data.length / 2) - Math.floor(partitionSize / 2);
+ const middle = data.slice(middleStart, middleStart + partitionSize);
[...head, ...middle, ...tail].forEach((entry, index) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function sampleData(data: Record<string, string>[], validationSampleSize = 30): Record<string, string>[] { | |
| if (!data || data.length <= validationSampleSize) { | |
| return data; | |
| } | |
| const validatedSet: Record<string, string>[] = [{}]; | |
| const sampleKeys = Object.keys(data[0]); | |
| sampleKeys.forEach((key) => { | |
| const nonEmptyKeyEntries = data.filter(entry => entry[key] && entry[key].trim() !== ''); | |
| if (nonEmptyKeyEntries.length <= validationSampleSize) { | |
| nonEmptyKeyEntries.forEach((entry, index) => { | |
| if (!validatedSet[index]) { | |
| validatedSet[index] = {}; | |
| } | |
| validatedSet[index][key] = entry[key]; | |
| }); | |
| } else { | |
| const partitionSize = Math.floor(validationSampleSize / 3); | |
| const head = data.slice(0, partitionSize); | |
| const tail = data.slice(data.length - partitionSize, data.length); | |
| const middleIndex = Math.floor(data.length / 2); | |
| const middle = data.slice(middleIndex - 2, middleIndex + 3); | |
| [...head, ...middle, ...tail].forEach((entry, index) => { | |
| if (!validatedSet[index]) { | |
| validatedSet[index] = {}; | |
| } | |
| validatedSet[index][key] = entry[key]; | |
| }); | |
| } | |
| }); | |
| return validatedSet; | |
| } | |
| function sampleData(data: Record<string, string>[], validationSampleSize = 30): Record<string, string>[] { | |
| if (!data || data.length <= validationSampleSize) { | |
| return data; | |
| } | |
| const validatedSet: Record<string, string>[] = []; | |
| const sampleKeys = Object.keys(data[0]); | |
| sampleKeys.forEach((key) => { | |
| const nonEmptyKeyEntries = data.filter(entry => entry[key] && entry[key].trim() !== ''); | |
| if (nonEmptyKeyEntries.length <= validationSampleSize) { | |
| nonEmptyKeyEntries.forEach((entry, index) => { | |
| if (!validatedSet[index]) { | |
| validatedSet[index] = {}; | |
| } | |
| validatedSet[index][key] = entry[key]; | |
| }); | |
| } else { | |
| const partitionSize = Math.floor(validationSampleSize / 3); | |
| const head = data.slice(0, partitionSize); | |
| const tail = data.slice(data.length - partitionSize, data.length); | |
| const middleStart = Math.floor(data.length / 2) - Math.floor(partitionSize / 2); | |
| const middle = data.slice(middleStart, middleStart + partitionSize); | |
| [...head, ...middle, ...tail].forEach((entry, index) => { | |
| if (!validatedSet[index]) { | |
| validatedSet[index] = {}; | |
| } | |
| validatedSet[index][key] = entry[key]; | |
| }); | |
| } | |
| }); | |
| return validatedSet; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/posts/src/views/members/components/bulk-action-modals/import-members/field-mapping.ts`
around lines 80 - 115, The sampleData function currently produces 25 samples (10
head + 5 middle + 10 tail) and starts validatedSet as [{}], creating an extra
empty row; change sample generation so the total number of sampled rows equals
validationSampleSize and remove the initial empty object. Specifically, in
sampleData: initialize validatedSet = []; when nonEmptyKeyEntries.length >
validationSampleSize compute headSize = Math.floor(validationSampleSize/3),
tailSize = headSize, middleSize = validationSampleSize - headSize - tailSize,
slice head = data.slice(0, headSize), tail = data.slice(-tailSize), compute a
centered middle slice of length middleSize around Math.floor(data.length/2),
then combine [...head, ...middle, ...tail] (which will total
validationSampleSize) and assign values into validatedSet rows (creating rows as
needed) for the given key; also ensure when nonEmptyKeyEntries.length <=
validationSampleSize you still populate validatedSet without leaving holes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsx (2)
109-130:⚠️ Potential issue | 🟡 MinorHandle
FileReaderfailures explicitly.Read errors are currently silent (
onloadonly), so failed reads can leave stale mapping state with no clear user feedback.Proposed fix
const reader = new FileReader(); reader.onload = (e) => { const text = e.target?.result as string; const data = parseCSV(text); setFileData(data); if (data.length > 0) { const detectedMapping = detectFieldTypes(data); const fieldMapping = new MembersFieldMapping(detectedMapping); setMapping(fieldMapping); // Validate mapping if (!fieldMapping.getKeyByValue('email')) { setMappingError('Please map "Email" to one of the fields in the CSV.'); } else { setMappingError(null); } } else { setMappingError('File is empty, nothing to import. Please select a different file.'); } }; + reader.onerror = () => { + setFileData([]); + setMapping(null); + setMappingError('Failed to read this file. Please try another CSV file.'); + }; reader.readAsText(file);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsx` around lines 109 - 130, The FileReader currently only sets reader.onload so read failures are silent; add explicit reader.onerror (and optionally reader.onabort) handlers to clear any stale state by calling setFileData([]) and setMapping(null) and to set a user-visible error via setMappingError with the FileReader error message (e.g., "Failed to read file: " + reader.error?.message) so parseCSV/detectFieldTypes aren't run on failed reads; keep the existing onload logic unchanged and reference the same reader, setFileData, setMapping, setMappingError, parseCSV, and detectFieldTypes symbols.
76-91:⚠️ Potential issue | 🟡 MinorRevoke generated error CSV blob URLs.
URL.createObjectURLis used, but URLs are never revoked during reset/re-import/unmount, which leaks memory over repeated imports.Proposed fix
export function ImportMembersModal({ @@ }: ImportMembersModalProps) { + const errorCsvUrlRef = useRef<string | null>(null); @@ const reset = useCallback(() => { + if (errorCsvUrlRef.current) { + URL.revokeObjectURL(errorCsvUrlRef.current); + errorCsvUrlRef.current = null; + } setState('INIT'); @@ }, []); + + useEffect(() => { + return () => { + if (errorCsvUrlRef.current) { + URL.revokeObjectURL(errorCsvUrlRef.current); + } + }; + }, []); @@ - const errorCsvUrl = URL.createObjectURL(errorCsvBlob); + if (errorCsvUrlRef.current) { + URL.revokeObjectURL(errorCsvUrlRef.current); + } + const errorCsvUrl = URL.createObjectURL(errorCsvBlob); + errorCsvUrlRef.current = errorCsvUrl;Also applies to: 295-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsx` around lines 76 - 91, The reset function and component unmount need to revoke any blob URLs created via URL.createObjectURL to avoid memory leaks; update the reset callback (where setFile, setFileData, setMapping, setFileError are reset) to call URL.revokeObjectURL for any stored file/fileData/blob URL before clearing them, and add a useEffect cleanup that revokes those URLs on unmount (also apply the same revoke logic to the other reset-like block around the 295-309 area); reference the reset function, setFile, setFileData, and the component cleanup useEffect to locate where to revoke the URLs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsx`:
- Around line 76-91: The reset function resets all state but doesn't clear the
underlying file input DOM value, so selecting the same CSV won't re-trigger
onChange; add a ref (e.g., fileInputRef) to the file input element and in reset
(the reset function shown) set fileInputRef.current.value = '' (with a
null-check and proper typing as HTMLInputElement) in addition to setFile(null);
apply the same DOM-clear to the other reset/try-again path mentioned in the
comment so both flows clear the input element.
- Around line 356-377: The upload dropzone div is not keyboard accessible;
update the div wrapping the dropzone (the element using fileInputRef, dragOver,
handleDragLeave, handleDragOver, handleDrop) to include role="button" and
tabIndex={0}, and add an onKeyDown handler that listens for Enter or Space and
calls fileInputRef.current?.click() (same action as the onClick). Ensure the
handler prevents default for Space to avoid page scroll and reuses existing
fileInputRef and click logic so keyboard users can open the file picker.
---
Duplicate comments:
In
`@apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsx`:
- Around line 109-130: The FileReader currently only sets reader.onload so read
failures are silent; add explicit reader.onerror (and optionally reader.onabort)
handlers to clear any stale state by calling setFileData([]) and
setMapping(null) and to set a user-visible error via setMappingError with the
FileReader error message (e.g., "Failed to read file: " + reader.error?.message)
so parseCSV/detectFieldTypes aren't run on failed reads; keep the existing
onload logic unchanged and reference the same reader, setFileData, setMapping,
setMappingError, parseCSV, and detectFieldTypes symbols.
- Around line 76-91: The reset function and component unmount need to revoke any
blob URLs created via URL.createObjectURL to avoid memory leaks; update the
reset callback (where setFile, setFileData, setMapping, setFileError are reset)
to call URL.revokeObjectURL for any stored file/fileData/blob URL before
clearing them, and add a useEffect cleanup that revokes those URLs on unmount
(also apply the same revoke logic to the other reset-like block around the
295-309 area); reference the reset function, setFile, setFileData, and the
component cleanup useEffect to locate where to revoke the URLs.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsx
Comment on lines
+76
to
+91
| const reset = useCallback(() => { | ||
| setState('INIT'); | ||
| setFile(null); | ||
| setFileData(null); | ||
| setMapping(null); | ||
| setSelectedLabels([]); | ||
| setDataPreviewIndex(0); | ||
| setMappingError(null); | ||
| setShowMappingErrors(false); | ||
| setImportResponse(null); | ||
| setErrorMessage(null); | ||
| setErrorHeader('Import error'); | ||
| setShowTryAgainButton(true); | ||
| setDragOver(false); | ||
| setFileError(null); | ||
| }, []); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsx | head -200Repository: TryGhost/Ghost
Length of output: 7848
🏁 Script executed:
cat -n apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsx | tail -n +200Repository: TryGhost/Ghost
Length of output: 30460
Clear file input value to allow re-selecting the same CSV after reset.
In browsers, file input onChange doesn't fire when selecting the same file if the input value hasn't changed. This prevents users from re-importing the same CSV after clicking "Start over" or "Try again".
Fix
const reset = useCallback(() => {
setState('INIT');
setFile(null);
setFileData(null);
setMapping(null);
setSelectedLabels([]);
setDataPreviewIndex(0);
setMappingError(null);
setShowMappingErrors(false);
setImportResponse(null);
setErrorMessage(null);
setErrorHeader('Import error');
setShowTryAgainButton(true);
setDragOver(false);
setFileError(null);
+ if (fileInputRef.current) {
+ fileInputRef.current.value = '';
+ }
}, []);
const handleFileInputChange = useCallback((e: React.ChangeEvent<HTMLInputElement>) => {
const selectedFile = e.target.files?.[0];
if (selectedFile) {
handleFileSelected(selectedFile);
}
+ e.target.value = '';
}, [handleFileSelected]);Also applies to: 151-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsx`
around lines 76 - 91, The reset function resets all state but doesn't clear the
underlying file input DOM value, so selecting the same CSV won't re-trigger
onChange; add a ref (e.g., fileInputRef) to the file input element and in reset
(the reset function shown) set fileInputRef.current.value = '' (with a
null-check and proper typing as HTMLInputElement) in addition to setFile(null);
apply the same DOM-clear to the other reset/try-again path mentioned in the
comment so both flows clear the input element.
Comment on lines
+356
to
+377
| <div | ||
| className={cn( | ||
| 'flex cursor-pointer flex-col items-center justify-center rounded-md border-2 border-dashed p-10 transition-colors', | ||
| dragOver | ||
| ? 'border-green-500 bg-green-50 dark:bg-green-950/20' | ||
| : 'border-grey-300 hover:border-grey-400' | ||
| )} | ||
| onClick={() => fileInputRef.current?.click()} | ||
| onDragLeave={handleDragLeave} | ||
| onDragOver={handleDragOver} | ||
| onDrop={handleDrop} | ||
| > | ||
| <LucideIcon.Upload className="mb-2 size-6 text-grey-600" /> | ||
| <span className="text-sm text-grey-700">Select or drop a CSV file</span> | ||
| <input | ||
| ref={fileInputRef} | ||
| accept=".csv" | ||
| className="hidden" | ||
| type="file" | ||
| onChange={handleFileInputChange} | ||
| /> | ||
| </div> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "import-members-modal.tsx" 2>/dev/null | head -5Repository: TryGhost/Ghost
Length of output: 143
🏁 Script executed:
sed -n '340,385p' ./apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsxRepository: TryGhost/Ghost
Length of output: 2807
🏁 Script executed:
# Search for similar patterns - custom divs used as buttons with keyboard handlers
rg -A 5 'role="button"' --type tsx --type ts apps/posts/src/views/members/components/ | head -30Repository: TryGhost/Ghost
Length of output: 85
🏁 Script executed:
# Search for similar button patterns in the components directory
rg 'role="button"' apps/posts/src/views/members/components/ -A 3 | head -40Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Search more broadly for role="button" in the codebase
rg 'role="button"' --max-count 10 | head -20Repository: TryGhost/Ghost
Length of output: 3112
🏁 Script executed:
# Check the activitypub settings.tsx for how role="button" is implemented
rg -B 5 -A 10 'role="button"' apps/activitypub/src/views/preferences/components/settings.tsxRepository: TryGhost/Ghost
Length of output: 373
🏁 Script executed:
# Search for onKeyDown handlers with role="button" or button-like divs
rg -B 3 -A 8 'onKeyDown.*\(e\)' apps/posts/src/views/members/components/ | head -50Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Search more broadly for onKeyDown handlers in the codebase
rg 'onKeyDown' apps/posts/src/ -B 2 -A 5 | head -60Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Check if there are any keyboard handlers for Enter or Space keys in the codebase
rg "key === 'Enter'" --max-count 10 | head -20Repository: TryGhost/Ghost
Length of output: 2289
🏁 Script executed:
# Check for Space key handling with button roles
rg "e.key === ' '" --max-count 10Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Search for common accessibility patterns - aria-label with role="button"
rg 'aria-label' apps/posts/src/ -B 2 -A 2 | head -40Repository: TryGhost/Ghost
Length of output: 1724
🏁 Script executed:
# Search for tabIndex in the posts app
rg 'tabIndex' apps/posts/src/ -B 2 -A 2 | head -50Repository: TryGhost/Ghost
Length of output: 571
🏁 Script executed:
# Let me check if there are similar file upload or dropzone components in the codebase
rg -l 'handleDrop\|handleDragOver' apps/posts/src/ | head -5Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Check the full import-members-modal.tsx file to see how handlers are defined
wc -l ./apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsxRepository: TryGhost/Ghost
Length of output: 147
🏁 Script executed:
# Search for the handler definitions in the file
rg -n 'handleDragLeave|handleDragOver|handleDrop|handleFileInputChange' ./apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsxRepository: TryGhost/Ghost
Length of output: 616
🏁 Script executed:
# Look at the handler implementations
sed -n '151,180p' ./apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsxRepository: TryGhost/Ghost
Length of output: 1023
Make the upload dropzone keyboard accessible.
The clickable <div> is mouse-only right now; keyboard-only users cannot trigger file selection here, which blocks task completion. The component needs role="button", tabIndex={0}, and an onKeyDown handler to activate on Enter or Space.
Proposed fix
<div
className={cn(
'flex cursor-pointer flex-col items-center justify-center rounded-md border-2 border-dashed p-10 transition-colors',
dragOver
? 'border-green-500 bg-green-50 dark:bg-green-950/20'
: 'border-grey-300 hover:border-grey-400'
)}
+ aria-label="Select or drop a CSV file"
onClick={() => fileInputRef.current?.click()}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ fileInputRef.current?.click();
+ }
+ }}
onDragLeave={handleDragLeave}
onDragOver={handleDragOver}
onDrop={handleDrop}
+ role="button"
+ tabIndex={0}
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsx`
around lines 356 - 377, The upload dropzone div is not keyboard accessible;
update the div wrapping the dropzone (the element using fileInputRef, dragOver,
handleDragLeave, handleDragOver, handleDrop) to include role="button" and
tabIndex={0}, and add an onKeyDown handler that listens for Enter or Space and
calls fileInputRef.current?.click() (same action as the onClick). Ensure the
handler prevents default for Space to avoid page scroll and reuses existing
fileInputRef and click logic so keyboard users can open the file picker.
Comment on lines
+182
to
+251
| /** | ||
| * Parse a CSV file into an array of objects with header keys. | ||
| * Simple implementation that handles quoted fields and newlines within quotes. | ||
| */ | ||
| export function parseCSV(text: string): Record<string, string>[] { | ||
| const rows: string[][] = []; | ||
| let current = ''; | ||
| let inQuotes = false; | ||
| let row: string[] = []; | ||
|
|
||
| for (let i = 0; i < text.length; i++) { | ||
| const char = text[i]; | ||
| const next = text[i + 1]; | ||
|
|
||
| if (inQuotes) { | ||
| if (char === '"' && next === '"') { | ||
| current += '"'; | ||
| i += 1; // skip escaped quote | ||
| } else if (char === '"') { | ||
| inQuotes = false; | ||
| } else { | ||
| current += char; | ||
| } | ||
| } else { | ||
| if (char === '"') { | ||
| inQuotes = true; | ||
| } else if (char === ',') { | ||
| row.push(current.trim()); | ||
| current = ''; | ||
| } else if (char === '\n' || (char === '\r' && next === '\n')) { | ||
| row.push(current.trim()); | ||
| current = ''; | ||
| if (row.some(cell => cell !== '')) { | ||
| rows.push(row); | ||
| } | ||
| row = []; | ||
| if (char === '\r') { | ||
| i += 1; // skip \n after \r | ||
| } | ||
| } else { | ||
| current += char; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Handle last row | ||
| if (current || row.length > 0) { | ||
| row.push(current.trim()); | ||
| if (row.some(cell => cell !== '')) { | ||
| rows.push(row); | ||
| } | ||
| } | ||
|
|
||
| if (rows.length < 2) { | ||
| return []; | ||
| } | ||
|
|
||
| const headers = rows[0]; | ||
| const data: Record<string, string>[] = []; | ||
|
|
||
| for (let i = 1; i < rows.length; i++) { | ||
| const obj: Record<string, string> = {}; | ||
| headers.forEach((header, j) => { | ||
| obj[header] = rows[i][j] || ''; | ||
| }); | ||
| data.push(obj); | ||
| } | ||
|
|
||
| return data; | ||
| } |
Member
There was a problem hiding this comment.
Is Ember using a CSV parsing library or does it also implement CSV parsing itself? Lets use a library if possible
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
2 participants
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.
Ref https://linear.app/ghost/issue/BER-3381/add-bulk-member-importing-flow
Somewhere in the original port, importing members was lost. This adds it back.