Add emoji support to goals and optimize transaction fetching#126
Add emoji support to goals and optimize transaction fetching#126Saniya1976 wants to merge 2 commits intofencer-so:mainfrom
Conversation
…tching and rendering
…nts, and refactor GoalManager emoji picker handler
There was a problem hiding this comment.
Pull request overview
Adds goal emoji icon support in the Goal Manager / Goal Card UI and improves transactions rendering behavior, while extending the frontend API helper with additional PUT update methods.
Changes:
- Render optional
Goal.iconin goal cards and add emoji selection/removal UI in the Goal Manager. - Improve transactions list rendering (avoid mutating props, add missing React keys) and parallelize per-transaction tag fetches.
- Add API
update*helpers for Transactions/Users/Tags/Accounts and prevent duplicate goal IDs in the goals list.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ui/pages/Main/transactions/TransactionsContent.tsx | Avoids in-place sorting of props and adds key to transaction list items. |
| src/ui/pages/Main/transactions/TransactionItem.tsx | Parallel tag fetching via Promise.all and effect dependency update. |
| src/ui/pages/Main/goals/GoalCard.tsx | Displays goal emoji icon when present. |
| src/ui/features/goalmanager/GoalManager.tsx | Adds icon state, emoji picker UI, and persistence via goal update calls. |
| src/store/goalsSlice.ts | Prevents duplicate goal IDs in state.list on create. |
| src/api/types.ts | Adds optional icon?: string to Goal type. |
| src/api/lib.ts | Adds PUT update helpers for Transaction/User/Tag/Account. |
Comments suppressed due to low confidence (1)
src/api/lib.ts:93
- There are multiple trailing blank lines at the end of the module. Please remove them to keep the file clean and consistent.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <IconContainer onClick={() => setIsPickerOpen(!isPickerOpen)}> | ||
| {icon ? ( | ||
| <> | ||
| <IconText>{icon}</IconText> | ||
| <RemoveIcon onClick={(e) => { | ||
| e.stopPropagation(); | ||
| updateIcon(""); | ||
| }}>×</RemoveIcon> |
There was a problem hiding this comment.
This new click-target uses non-interactive elements (div) for actions (open picker / remove icon). That breaks keyboard navigation and screen reader semantics. Prefer a button (or the existing TransparentButton) with appropriate aria-labels and focus styles, and keep the remove control as a separate button inside the container.
| <RemoveIcon onClick={(e) => { | ||
| e.stopPropagation(); | ||
| updateIcon(""); | ||
| }}>×</RemoveIcon> |
There was a problem hiding this comment.
This block introduces semicolons and double quotes (e.g. e.stopPropagation(); and updateIcon("")), but the repo's Prettier config enforces semi: false and singleQuote: true. Please format this section (or run Prettier) so it matches project formatting to avoid CI/style check failures.
| <RemoveIcon onClick={(e) => { | |
| e.stopPropagation(); | |
| updateIcon(""); | |
| }}>×</RemoveIcon> | |
| <RemoveIcon | |
| onClick={e => { | |
| e.stopPropagation() | |
| updateIcon('') | |
| }} | |
| > | |
| × | |
| </RemoveIcon> |
| <TopContainer> | ||
| <IconContainer onClick={() => setIsPickerOpen(!isPickerOpen)}> | ||
| {icon ? ( | ||
| <> | ||
| <IconText>{icon}</IconText> | ||
| <RemoveIcon onClick={(e) => { | ||
| e.stopPropagation(); | ||
| updateIcon(""); | ||
| }}>×</RemoveIcon> | ||
| </> | ||
| ) : ( | ||
| <AddIconContainer> | ||
| <FontAwesomeIcon icon={faPlus} size="2x" /> | ||
| <AddIconText>Add Icon</AddIconText> | ||
| </AddIconContainer> | ||
| )} | ||
| </IconContainer> |
There was a problem hiding this comment.
There is an existing GoalIcon component under src/ui/features/goalmanager/GoalIcon.tsx, but this change re-implements icon rendering and click handling inline. Since GoalIcon appears unused now, consider either reusing it here (to keep icon UI consistent) or removing the unused component to avoid dead code.
| useEffect(() => { | ||
| async function fetch(tagId: string): Promise<Tag> { | ||
| const response = await axios.get(`${API_ROOT}/api/Tag/${tagId}`) | ||
| return response.data | ||
| } |
There was a problem hiding this comment.
The effect kicks off async requests but doesn't include any cleanup/abort handling. If the component unmounts before the requests resolve, setTags(...) can run after unmount and trigger React warnings. Consider adding an AbortController for the axios request(s) or an isMounted/cancelled flag in the effect cleanup before calling setTags.
| const fetchedTags = await Promise.all(props.transaction.tagIds.map(fetch)) | ||
| setTags(fetchedTags) |
There was a problem hiding this comment.
Promise.all(...) will reject on the first failed tag request, and since the returned promise isn't caught, this can surface as an unhandled rejection and leave the component in a perpetual loading state. Wrap the fetch in try/catch (or use Promise.allSettled) and decide on a fallback (e.g., skip failed tags, show none, or surface an error).
| const fetchedTags = await Promise.all(props.transaction.tagIds.map(fetch)) | |
| setTags(fetchedTags) | |
| try { | |
| const fetchedTags = await Promise.all(props.transaction.tagIds.map(fetch)) | |
| setTags(fetchedTags) | |
| } catch (error) { | |
| console.error('Failed to fetch tags for transaction', props.transaction.id, error) | |
| setTags([]) | |
| } |
Combined Goal Persistence & Test Coverage Update
This PR delivers all the required updates for both the Frontend and Backend repositories:
1. Frontend Enhancements (This Repo):
PUTmethods for Transactions, Users, Tags, and Accounts in lib.ts.2. Backend Enhancements (See CommBank-Server Repo):
UserId.Backend changes are committed to the same branch
feature/goal-icon-and-testsin the Saniya1976/CommBank-Server repository.