Conversation
There was a problem hiding this comment.
Pull request overview
Adds per-goal icon/emoji support across the UI and API typings, enabling users to select an emoji for a goal and persist it via the existing Goal update API.
Changes:
- Display a goal icon on cards (with a default fallback).
- Add emoji picker flow in GoalManager to set/update a goal’s icon.
- Extend
GoalTypeScript model with aniconfield (and update dependencies/lockfile metadata).
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ui/pages/Main/goals/GoalCard.tsx | Renders goal icon (fallback 🎯) and adds icon styling. |
| src/ui/features/goalmanager/GoalManager.tsx | Adds emoji picker + icon update flow and related UI containers/state. |
| src/ui/features/goalmanager/GoalIcon.tsx | Minor sizing tweak for rendered goal icon. |
| src/api/types.ts | Extends Goal interface with icon. |
| package-lock.json | Adds license metadata for emoji-mart entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| accountId: string | ||
| transactionIds: string[] | ||
| tagIds: string[] | ||
| icon: string | null |
There was a problem hiding this comment.
PR description says the Icon field is optional, but the Goal interface makes icon a required property. If the backend may omit this field for existing goals, this should be modeled as optional (e.g., icon?: string | null) or the API layer should normalize missing icons to null so the type is accurate.
| icon: string | null | |
| icon?: string | null |
| font-size: 1rem; | ||
| ` | ||
|
|
||
| const Icon = styled.h1` |
There was a problem hiding this comment.
The icon is rendered/styled as an h1, which is a heading element and can be confusing for screen readers and document outline. Consider using a non-heading element (e.g., span/div) and add appropriate accessibility semantics (role="img" + aria-label, or aria-hidden if decorative).
| const Icon = styled.h1` | |
| const Icon = styled.div.attrs({ 'aria-hidden': 'true' })` |
| </AddIconButtonContainer> | ||
|
|
||
| <GoalIconContainer shouldShow={hasIcon()}> | ||
| <GoalIcon icon={goal.icon} onClick={addIconOnClick} /> |
There was a problem hiding this comment.
GoalIconContainer visibility is driven by hasIcon() (local icon state), but the rendered icon is sourced from goal.icon (Redux). After selecting an emoji there can be a render where hasIcon() is true while goal.icon is still null/old, causing the icon area to show blank. Use a single source of truth (derive hasIcon from the same value you render, or render from icon state consistently).
| <GoalIcon icon={goal.icon} onClick={addIconOnClick} /> | |
| <GoalIcon icon={icon} onClick={addIconOnClick} /> |
| setIcon(emoji.native) | ||
| setEmojiPickerIsOpen(false) | ||
|
|
||
| const updatedGoal: Goal = { | ||
| ...props.goal, | ||
| icon: emoji.native ?? props.goal.icon, |
There was a problem hiding this comment.
Local state is updated with setIcon(emoji.native), but the value you persist is updatedGoal.icon (which falls back to props.goal.icon). To avoid local/UI state drifting from the persisted value, set the local icon state from the same value you assign into updatedGoal.icon (and normalize to null when absent).
| setIcon(emoji.native) | |
| setEmojiPickerIsOpen(false) | |
| const updatedGoal: Goal = { | |
| ...props.goal, | |
| icon: emoji.native ?? props.goal.icon, | |
| const nextIcon = emoji.native ?? props.goal.icon ?? null | |
| setIcon(nextIcon) | |
| setEmojiPickerIsOpen(false) | |
| const updatedGoal: Goal = { | |
| ...props.goal, | |
| icon: nextIcon, |
Changes Made
Iconfield to Goal modelTesting