refactor(ui): replace toast emojis with lucide icons#188
Conversation
📝 WalkthroughWalkthroughThe changes implement a migration from Unicode emoji icons to Lucide React icon components in the Toast notification system. A new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR replaces emoji glyphs in toast notifications with Confidence Score: 5/5Safe to merge — clean, well-scoped icon replacement with no logic changes and updated tests. All changes are purely presentational: emoji strings replaced with typed Lucide SVG components. No logic, state, or API surface is altered. Tests are extended to cover the new icons, and the existing behavioural tests remain intact. No files require special attention.
|
| Filename | Overview |
|---|---|
| src/components/Toast/Toast.tsx | Replaces emoji icon: string entries with Icon: LucideIcon in NOTIFICATION_META, renders the icon via <Icon data-testid=… /> inside an aria-hidden span — clean and correct. |
| src/components/Toast/Toast.test.tsx | Adds getByTestId assertions for each notification type's icon; existing behaviour tests (auto-dismiss, navigation, payload summary) are preserved. |
| package.json | Adds lucide-react ^1.8.0 as a production dependency, appropriate given the icon components are rendered at runtime. |
| package-lock.json | Lock file updated to resolve lucide-react@1.8.0 with a valid integrity hash; peer-dep range covers React 16–19. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[useNotifications hook] -->|notifications array| B[Toast component]
B -->|map each notification| C[ToastItem]
C --> D{NOTIFICATION_META lookup}
D -->|review_request| E[Eye icon]
D -->|ci_failure| F[CircleX icon]
D -->|pr_approved| G[CircleCheck icon]
E --> H[aria-hidden span wrapper]
F --> H
G --> H
H --> I[button: label plus summary]
I -->|click| J[onNavigate + onDismiss]
I -->|5s timeout| K[auto-dismiss]
Reviews (1): Last reviewed commit: "refactor(ui): replace toast emojis with ..." | Re-trigger Greptile
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/Toast/Toast.tsx (1)
80-80: Consider unique icon test IDs per toast instance.Line 80 uses only
notification.type, so same-type toasts can produce duplicate test IDs and brittle selectors.Proposed tweak
- data-testid={`toast-icon-${notification.type}`} + data-testid={`toast-icon-${notification.type}-${notification.id}`}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Toast/Toast.tsx` at line 80, The test-id for the toast icon currently uses only notification.type which can collide for multiple toasts of the same type; update the data-testid in Toast.tsx (the attribute set on the icon element) to include a unique per-instance identifier such as notification.id or a generated index (e.g., use notification.id or a passed-in prop like toastId) so the test selector becomes unique per toast instance (e.g., combine type and id); ensure the chosen identifier exists on the notification object or is passed down from the notification list where notifications are created.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/Toast/Toast.tsx`:
- Line 80: The test-id for the toast icon currently uses only notification.type
which can collide for multiple toasts of the same type; update the data-testid
in Toast.tsx (the attribute set on the icon element) to include a unique
per-instance identifier such as notification.id or a generated index (e.g., use
notification.id or a passed-in prop like toastId) so the test selector becomes
unique per toast instance (e.g., combine type and id); ensure the chosen
identifier exists on the notification object or is passed down from the
notification list where notifications are created.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 59b1b77b-6655-486e-97e2-d5255e448323
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.jsonsrc/components/Toast/Toast.test.tsxsrc/components/Toast/Toast.tsx
1 participant
Summary
lucide-reactSVG iconsValidation
npm test -- src/components/Toast/Toast.test.tsxnpm run lint -- src/components/Toast/Toast.tsx src/components/Toast/Toast.test.tsxnpm run buildCloses #176
Summary by cubic
Replaced toast emojis with
lucide-reactSVG icons for a more consistent UI. Labels, navigation targets, and payload summaries remain unchanged.Refactors
Notificationtypes to Lucide components (Eye,CircleX,CircleCheck) usingLucideIcon.data-testid="toast-icon-{type}"and add tests to assert icon presence.Dependencies
lucide-react^1.8.0.Written for commit 6c0f791. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests
Chores