Mobile app expansion (uniwind, dashboard/diary)#645
Mobile app expansion (uniwind, dashboard/diary)#645CodeWithCJ merged 3 commits intoCodeWithCJ:mainfrom
Conversation
Summary of ChangesHello @apedley, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the SparkyFitness mobile application by introducing new Dashboard and Diary screens, providing users with detailed daily health insights. A major architectural shift involved migrating the styling to Uniwind (TailwindCSS for React Native) and integrating Tanstack Query for robust data management. These changes enhance the app's functionality, improve its visual consistency, and lay a strong foundation for future feature development by centralizing API interactions and state management. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an impressive and substantial pull request that significantly modernizes the mobile app. The migration to Uniwind for styling and TanStack Query for data fetching are excellent architectural improvements that will make the codebase more maintainable and performant. The addition of the Dashboard and Diary screens, along with the restructuring of navigation and settings, greatly expands the app's functionality. The extensive test coverage for the new hooks and services is also highly commendable. I've identified a couple of important issues related to a potential race condition and an unhandled promise rejection that should be addressed, but overall, this is a fantastic contribution.
| Promise.all([ | ||
| Ionicons.getImageSource('home', 24, '#999999'), | ||
| Ionicons.getImageSource('sync', 24, '#999999'), | ||
| Ionicons.getImageSource('grid', 24, '#999999'), | ||
| Ionicons.getImageSource('book', 24, '#999999'), | ||
| Ionicons.getImageSource('settings', 24, '#999999'), | ||
| Ionicons.getImageSource('document-text', 24, '#999999'), | ||
| ]).then(([home, settings, document]) => { | ||
| if (home && settings && document) { | ||
| setIcons({ home, settings, document }); | ||
| ]).then(([sync, dashboard, book, settings]) => { | ||
| if (sync && dashboard && book && settings) { | ||
| setIcons({ sync, dashboard, book, settings }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This Promise.all call is missing a .catch() block. If any of the Ionicons.getImageSource promises reject (e.g., due to a network issue or an invalid icon name), it will result in an unhandled promise rejection. In production builds, this can crash the app. It's crucial to handle potential errors from asynchronous operations to ensure app stability.
]).then(([sync, dashboard, book, settings]) => {
if (sync && dashboard && book && settings) {
setIcons({ sync, dashboard, book, settings });
}
}).catch(error => {
console.error('Failed to load tab icons:', error);
});
There was a problem hiding this comment.
These are bundled files so this shouldn't be an issue but I added a catch anyway.
| onOpenWebDashboard={openWebDashboard} | ||
| isConnected={isConnected} | ||
| checkServerConnection={checkServerConnection} | ||
| checkServerConnection={() => refetchConnection().then(() => isConnected)} |
There was a problem hiding this comment.
There's a potential race condition here due to a stale closure over the isConnected state. The then callback will use the value of isConnected from the render cycle in which checkServerConnection was defined, not the value from after the refetch completes. This could lead to the ConnectionStatus component receiving an outdated status.
To fix this, you should use the result of the refetchConnection promise, which contains the updated data.
| checkServerConnection={() => refetchConnection().then(() => isConnected)} | |
| checkServerConnection={() => refetchConnection().then(queryResult => queryResult.data ?? false)} |
There was a problem hiding this comment.
This isn't a bug now but it could be in the future. Fixed
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an impressive and extensive update to the mobile app. The migration to uniwind for styling and tanstack-query for data fetching is a significant architectural improvement that will greatly benefit maintainability and performance. The new Dashboard and Diary screens look fantastic, and the UI/UX enhancements, especially in the Settings screen, are well-thought-out. The addition of comprehensive tests for all the new hooks and services is particularly commendable.
I've found a couple of minor areas for improvement related to ensuring full consistency with the new theming system by replacing a few remaining hardcoded colors with theme variables. My detailed comments are on the respective lines.
Overall, this is an excellent pull request. Great job!
| const getStatusColor = (status: string) => { | ||
| switch (status) { | ||
| case 'SUCCESS': return '#28a745'; | ||
| case 'WARNING': return '#ffc107'; | ||
| case 'INFO': return '#007bff'; | ||
| case 'DEBUG': return '#6c757d'; | ||
| default: return '#dc3545'; | ||
| } | ||
| }; |
There was a problem hiding this comment.
These status colors are hardcoded. To ensure consistency with the new theming system, you should use the theme variables from global.css.
You can achieve this by:
- Importing
useCSSVariablefromuniwind. - Calling it at the top of the
LogScreencomponent to get the theme colors. - Using these variables inside this
getStatusColorfunction.
Example:
const [successColor, warningColor, dangerColor] = useCSSVariable([
'--color-text-success',
'--color-text-warning',
'--color-text-danger',
]);
// ... inside getStatusColor
case 'SUCCESS': return successColor;| activeOpacity={0.7} | ||
| > | ||
| <Text className="text-base font-semibold text-text-primary">View Logs</Text> | ||
| <Icon name="chevron-forward" size={20} color="#999" /> |
There was a problem hiding this comment.
This icon color is hardcoded. To make it theme-aware, you should use a CSS variable.
- At the top of the
SettingsScreencomponent, get the muted text color:
const textMuted = useCSSVariable('--color-text-muted') as string;- Then, use this variable for the icon's color:
<Icon name="chevron-forward" size={20} color={textMuted} />
|
@apedley It's impossible to review a PR with 10k additions in a single commit. At least have different commit per feature or tests. I haven't looked deeply into the mobile app but can't we share the whole logic with the forntend? My next goal with the frontend is to migrate everything into Tanstack Query. |
Yes I know it's a giant PR. I had a git mishap that was going to take me hours to fix but I'm the only one working on the mobile app currently so I okay'ed it with CodewithCJ. Sharing logic isn't really possible but sharing types definitely is and I was going to ask you how you felt about it. I don't really have any experience with doing that but I do know some people use openapi to zod generators. Is the front end using zod at all? I'm probably going to use it along with react-hook-form once I get into add/edit territory. |
|
I haven't done it but it's possible to share them using a monorepo with a @shared/api folder or something. Turborepo seems like the best tool for it. I can look into the details because it's going to save us alot of work. You define a new package using pnpm where you can install stuff or use it in every other tool. |
Turborepo shared types package does seem like the way to go. I'm on discord in the dev channel if you want to talk more |
3 participants
Expanded the mobile app by adding dashboard and diary screens. These screens are purely read only for now.
Migrated to uniwind (tailwindcss for react native)
Added tanstack query for API fetching, retrying, and caching
Updated theme and tweaked images
Restructured settings
Moved main screen to sync screen
I'm sorry about the giant PR!