fix(platform): refine toast icons, CSV header detection, and vendor bulk delete#1321
Merged
Merged
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThis PR coordinates changes across multiple components to refactor the toast notification system by removing explicit success variants, enhance the vendor management table with row selection and bulk deletion, streamline product import parsing from row-based CSV mapping to record-based mapping, simplify CSV header detection in the file parsing utility, update import form UI hints, and add corresponding internationalization strings. The changes span toast behavior, file import workflows, data table enhancements, and localization files. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/platform/app/components/ui/feedback/toaster.tsx (1)
74-77:⚠️ Potential issue | 🟡 MinorLocalize the close button
aria-labelinstead of hardcoding English.Line 76 uses a hardcoded
aria-label="Close", which breaks the repo’s i18n/accessibility rule for ARIA attributes.💡 Suggested fix
+import { useT } from '@/lib/i18n/client'; + export function Toaster() { + const { t: tCommon } = useT('common'); const { toasts } = useToast(); @@ <ToastPrimitives.Close className="text-foreground/50 hover:text-foreground absolute top-2.5 right-2.5 rounded-md p-1 opacity-0 transition-opacity group-hover:opacity-100 group-[.destructive]:text-red-300 group-[.destructive]:hover:text-red-50 focus:opacity-100 focus:ring-2 focus:outline-none group-[.destructive]:focus:ring-red-400 group-[.destructive]:focus:ring-offset-red-600" - aria-label="Close" + aria-label={tCommon('actions.close')} >As per coding guidelines: “Do NOT hardcode text” and “ALWAYS use translation keys for
aria-labelvalues. Never hardcode English in ARIA attributes”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/app/components/ui/feedback/toaster.tsx` around lines 74 - 77, Replace the hardcoded aria-label on the ToastPrimitives.Close element with a localized string: import/obtain the repo's translation helper (e.g., t or useTranslation) in the component and use the appropriate translation key (e.g., "close" or "toast.close") for the aria-label on ToastPrimitives.Close so ARIA text is not hardcoded; update the Toast component to call the translator and pass the translated value into aria-label on ToastPrimitives.Close.services/platform/app/hooks/__tests__/use-file-import.test.ts (1)
158-208:⚠️ Potential issue | 🟡 MinorAdd coverage for the header-driven CSV path, not just
record().These tests only exercise
productMappers.record()directly. They would not catch regressions in the new first-row-is-header flow, the headerless failure path, or the currentparseCSV()vsparseFile()mismatch for products. As per coding guidelines, "Tests should cover happy paths, edge cases, and error conditions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/app/hooks/__tests__/use-file-import.test.ts` around lines 158 - 208, Add unit tests that exercise the header-driven CSV import flow in addition to productMappers.record(): write tests that call parseCSV (and/or the product import wrapper that reads first-row-as-header) with a CSV where the first row contains header names (including mixed case like ImageUrl/imageUrl/imageurl) to ensure headers map to product fields, a test where the CSV has no header row to verify the headerless failure path returns the expected error/behavior, and a test that exercises the parseFile vs parseCSV code path mismatch for products to assert both functions produce the same product mapping; reference productMappers.record, parseCSV, and parseFile to locate and extend existing tests and assert on mapped fields (name/title fallback, defaults, imageUrl normalization, and error cases).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/platform/app/hooks/use-file-import.ts`:
- Around line 246-250: The csv row mapper in useFileImport currently returns
null (csv: (_row...) => { return null; }) which causes parseCSV to drop all rows
because parseCSVWithMapper is invoked without a recordMapper; fix by having the
csv mapper translate a headered CSV row into a record and forward to the
existing record mapper (or simply delegate to record: (r)=>...): replace the csv
mapper implementation so it builds a Record<string, unknown> from the header/row
pair and calls the existing record mapper or returns its result, ensuring
parseCSV and parseCSVWithMapper produce the same parsed output as file-based CSV
imports.
In `@services/platform/lib/utils/file-parsing.ts`:
- Around line 95-96: The CSV parsing currently only treats the first row as
headers when rows.length >= 2, which misses single-row header-only files; change
the header extraction condition in the parsing logic that sets headers (the
block using rows.shift()?.map((h) => h.toLowerCase()) ?? null) to run whenever
any row exists (e.g., rows.length > 0 or rows.length >= 1) so the first row is
always treated as headers when present, preserving the lowercase mapping and
existing fallback behavior.
In `@services/platform/messages/de.json`:
- Line 3158: Update the German localization string for the key "statusNote" so
the status is quoted and reads naturally in UI copy; replace the awkward value
"Importierte Kunden werden als (Aktiv) markiert" with a version using German
typographic quotes, e.g. "Importierte Kunden werden als „Aktiv“ markiert"
(ensure you update the value for the "statusNote" key in
services/platform/messages/de.json).
In `@services/platform/messages/en.json`:
- Around line 3273-3278: The importForm namespace in the translations is missing
the formatHint key referenced by t('importForm.formatHint') in
vendor-import-form.tsx; add an "formatHint" entry under "importForm" in
services/platform/messages/en.json (and add the same key/value in
services/platform/messages/de.json) so the manual vendor import hint renders
correctly, keeping the string similar to the existing localeHint/expectedColumns
entries.
---
Outside diff comments:
In `@services/platform/app/components/ui/feedback/toaster.tsx`:
- Around line 74-77: Replace the hardcoded aria-label on the
ToastPrimitives.Close element with a localized string: import/obtain the repo's
translation helper (e.g., t or useTranslation) in the component and use the
appropriate translation key (e.g., "close" or "toast.close") for the aria-label
on ToastPrimitives.Close so ARIA text is not hardcoded; update the Toast
component to call the translator and pass the translated value into aria-label
on ToastPrimitives.Close.
In `@services/platform/app/hooks/__tests__/use-file-import.test.ts`:
- Around line 158-208: Add unit tests that exercise the header-driven CSV import
flow in addition to productMappers.record(): write tests that call parseCSV
(and/or the product import wrapper that reads first-row-as-header) with a CSV
where the first row contains header names (including mixed case like
ImageUrl/imageUrl/imageurl) to ensure headers map to product fields, a test
where the CSV has no header row to verify the headerless failure path returns
the expected error/behavior, and a test that exercises the parseFile vs parseCSV
code path mismatch for products to assert both functions produce the same
product mapping; reference productMappers.record, parseCSV, and parseFile to
locate and extend existing tests and assert on mapped fields (name/title
fallback, defaults, imageUrl normalization, and error cases).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b9d26eb-23e6-41a1-8995-a8eec164b895
📒 Files selected for processing (16)
services/platform/app/components/ui/entity/entity-delete-dialog.tsxservices/platform/app/components/ui/feedback/toaster.tsxservices/platform/app/features/agents/components/agent-delete-dialog.tsxservices/platform/app/features/customers/components/customer-import-form.tsxservices/platform/app/features/products/components/products-import-dialog.tsxservices/platform/app/features/settings/api-keys/components/api-key-revoke-dialog.tsxservices/platform/app/features/settings/providers/components/providers-table.tsxservices/platform/app/features/settings/teams/components/team-delete-dialog.tsxservices/platform/app/features/vendors/components/vendor-import-form.tsxservices/platform/app/features/vendors/components/vendors-table.tsxservices/platform/app/features/vendors/hooks/use-vendors-table-config.tsxservices/platform/app/hooks/__tests__/use-file-import.test.tsservices/platform/app/hooks/use-file-import.tsservices/platform/lib/utils/file-parsing.tsservices/platform/messages/de.jsonservices/platform/messages/en.json
💤 Files with no reviewable changes (4)
- services/platform/app/features/agents/components/agent-delete-dialog.tsx
- services/platform/app/components/ui/entity/entity-delete-dialog.tsx
- services/platform/app/features/settings/api-keys/components/api-key-revoke-dialog.tsx
- services/platform/app/features/settings/teams/components/team-delete-dialog.tsx
Comment on lines
+246
to
+250
| csv: (_row: string[], _index: number) => { | ||
| return null; | ||
| }, | ||
| excel: (record: Record<string, unknown>) => { | ||
| /** Record-based mapper used by both CSV (with headers) and Excel imports */ | ||
| record: (record: Record<string, unknown>) => { |
There was a problem hiding this comment.
This breaks useFileImport().parseCSV() for valid product CSVs.
Line 247 makes the CSV mapper reject every row, but parseCSV() in this hook still calls parseCSVWithMapper(csvText, csvMapper) without a recordMapper. So a valid headered product CSV parses correctly from a File, but the same CSV text comes back empty through parseCSV().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/hooks/use-file-import.ts` around lines 246 - 250, The
csv row mapper in useFileImport currently returns null (csv: (_row...) => {
return null; }) which causes parseCSV to drop all rows because
parseCSVWithMapper is invoked without a recordMapper; fix by having the csv
mapper translate a headered CSV row into a record and forward to the existing
record mapper (or simply delegate to record: (r)=>...): replace the csv mapper
implementation so it builds a Record<string, unknown> from the header/row pair
and calls the existing record mapper or returns its result, ensuring parseCSV
and parseCSVWithMapper produce the same parsed output as file-based CSV imports.
Comment on lines
95
to
96
| if (rows.length >= 2) { | ||
| headers = rows.shift()?.map((h) => h.toLowerCase()) ?? null; |
There was a problem hiding this comment.
Treat the first row as headers whenever a row exists.
Lines 95-96 leave single-row CSVs on the positional path. That means a header-only customer/vendor file like email,name,locale is parsed as one real record, and a single-row headerless file still bypasses the new header-required flow. This can create bogus imports and makes the new behavior inconsistent.
Suggested fix
- if (rows.length >= 2) {
+ if (rows.length > 0) {
headers = rows.shift()?.map((h) => h.toLowerCase()) ?? null;
}📝 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
| if (rows.length >= 2) { | |
| headers = rows.shift()?.map((h) => h.toLowerCase()) ?? null; | |
| if (rows.length > 0) { | |
| headers = rows.shift()?.map((h) => h.toLowerCase()) ?? null; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/lib/utils/file-parsing.ts` around lines 95 - 96, The CSV
parsing currently only treats the first row as headers when rows.length >= 2,
which misses single-row header-only files; change the header extraction
condition in the parsing logic that sets headers (the block using
rows.shift()?.map((h) => h.toLowerCase()) ?? null) to run whenever any row
exists (e.g., rows.length > 0 or rows.length >= 1) so the first row is always
treated as headers when present, preserving the lowercase mapping and existing
fallback behavior.
| "formatHint": "Format: E-Mail, Name (optional), Sprache (optional). Ein Kunde pro Zeile.", | ||
| "localeHint": "Unterstützte Sprachen: en, fr, de, zh, sowie erweiterte Formate wie en-US, de-DE und de-CH.", | ||
| "statusNote": "Importierte Kunden werden als (Aktiv) markiert" | ||
| "statusNote": "Importierte Kunden werden als (Aktiv) markiert", |
There was a problem hiding this comment.
Polish the German status note wording.
Line 3158 reads awkwardly: als (Aktiv) markiert. Prefer quoted status text for natural UI copy.
Suggested copy fix
- "statusNote": "Importierte Kunden werden als (Aktiv) markiert",
+ "statusNote": "Importierte Kunden werden als „Aktiv“ markiert",📝 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
| "statusNote": "Importierte Kunden werden als (Aktiv) markiert", | |
| "statusNote": "Importierte Kunden werden als „Aktiv" markiert", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/messages/de.json` at line 3158, Update the German
localization string for the key "statusNote" so the status is quoted and reads
naturally in UI copy; replace the awkward value "Importierte Kunden werden als
(Aktiv) markiert" with a version using German typographic quotes, e.g.
"Importierte Kunden werden als „Aktiv“ markiert" (ensure you update the value
for the "statusNote" key in services/platform/messages/de.json).
Comment on lines
3273
to
3278
| "importForm": { | ||
| "manualEntry": "Manual entry", | ||
| "upload": "Upload", | ||
| "formatHint": "Format: email, name (optional), locale (optional). One vendor per line.", | ||
| "localeHint": "You can use the following supported locales: en, fr, de, zh, as well as extended formats like en-US, de-DE, and de-CH." | ||
| "localeHint": "You can use the following supported locales: en, fr, de, zh, as well as extended formats like en-US, de-DE, and de-CH.", | ||
| "expectedColumns": "Expected columns: email, name, locale" | ||
| }, |
There was a problem hiding this comment.
Missing vendors.importForm.formatHint translation key breaks manual vendor import hint.
services/platform/app/features/vendors/components/vendor-import-form.tsx (Line 96) still calls t('importForm.formatHint'), but this namespace does not define formatHint. Add it here (and mirror in services/platform/messages/de.json) to avoid missing-key rendering.
💡 Suggested patch (en.json)
"importForm": {
"manualEntry": "Manual entry",
"upload": "Upload",
+ "formatHint": "Format: email, name (optional), locale (optional). One vendor per line.",
"localeHint": "You can use the following supported locales: en, fr, de, zh, as well as extended formats like en-US, de-DE and de-CH.",
"expectedColumns": "Expected columns: email, name, locale"
},📝 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
| "importForm": { | |
| "manualEntry": "Manual entry", | |
| "upload": "Upload", | |
| "formatHint": "Format: email, name (optional), locale (optional). One vendor per line.", | |
| "localeHint": "You can use the following supported locales: en, fr, de, zh, as well as extended formats like en-US, de-DE, and de-CH." | |
| "localeHint": "You can use the following supported locales: en, fr, de, zh, as well as extended formats like en-US, de-DE, and de-CH.", | |
| "expectedColumns": "Expected columns: email, name, locale" | |
| }, | |
| "importForm": { | |
| "manualEntry": "Manual entry", | |
| "upload": "Upload", | |
| "formatHint": "Format: email, name (optional), locale (optional). One vendor per line.", | |
| "localeHint": "You can use the following supported locales: en, fr, de, zh, as well as extended formats like en-US, de-DE, and de-CH.", | |
| "expectedColumns": "Expected columns: email, name, locale" | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/messages/en.json` around lines 3273 - 3278, The importForm
namespace in the translations is missing the formatHint key referenced by
t('importForm.formatHint') in vendor-import-form.tsx; add an "formatHint" entry
under "importForm" in services/platform/messages/en.json (and add the same
key/value in services/platform/messages/de.json) so the manual vendor import
hint renders correctly, keeping the string similar to the existing
localeHint/expectedColumns entries.
yannickmonney
requested changes
Apr 10, 2026
| "upload": "Upload", | ||
| "formatHint": "Format: E-Mail, Name (optional), Sprache (optional). Ein Lieferant pro Zeile.", | ||
| "localeHint": "Du kannst die folgenden unterstützten Sprachen verwenden: en, fr, de, zh, sowie erweiterte Formate wie en-US, de-DE und de-CH." | ||
| "localeHint": "Du kannst die folgenden unterstützten Sprachen verwenden: en, fr, de, zh, sowie erweiterte Formate wie en-US, de-DE und de-CH.", |
Contributor
There was a problem hiding this comment.
Locale is not restricted anylonger
| "upload": "Upload", | ||
| "formatHint": "Format: email, name (optional), locale (optional). One vendor per line.", | ||
| "localeHint": "You can use the following supported locales: en, fr, de, zh, as well as extended formats like en-US, de-DE, and de-CH." | ||
| "localeHint": "You can use the following supported locales: en, fr, de, zh, as well as extended formats like en-US, de-DE, and de-CH.", |
Contributor
There was a problem hiding this comment.
Locale is not restricted anylonger
| providerName: deleteProvider.name, | ||
| }); | ||
| toast({ title: t('providers.deleted'), variant: 'success' }); | ||
| toast({ title: t('providers.deleted') }); |
Contributor
There was a problem hiding this comment.
I merged some code adding success variant, you may also wanna remove it for those toast messages.
…ulk delete - Simplify toaster to always show variant icons (info default) instead of conditional icon/no-icon layout - Remove explicit 'success' variant from delete/revoke toasts (default handles it) - Replace brittle heuristic-based CSV header detection with first-row-is-header approach and drop the COMMON_HEADER_NAMES dictionary - Unify product import mappers: rename excel→record, make CSV fallback return null so headerless files fail clearly instead of silently misaligning columns - Add expectedColumns hints to customer and vendor import forms - Add bulk delete support to vendors table with row selection
… hint text - Change CSV header detection from rows.length >= 2 to rows.length > 0 so a header-only CSV is correctly treated as headers with zero data rows - Update localeHint text to reflect that locale is no longer restricted to a fixed set (now accepts any valid BCP 47 tag)
Israeltheminer
force-pushed
the
fix/import-ux-and-toast-refinements
branch
from
3d72877 to
922b1b8
Compare
…ions Delete and revoke confirmations are neutral acknowledgements, not celebratory successes — show the info icon instead of the green check.
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.
Summary
successvariant from delete/revoke toasts — the default toast style now handles all cases uniformlyCOMMON_HEADER_NAMESdictionary) with a simpler first-row-is-header approachexcel→record, make CSV fallback returnnullso headerless files fail clearly instead of silently misaligning columnsexpectedColumnsguidance hints to customer and vendor import formsTest plan
Summary by CodeRabbit
Release Notes
New Features
Style