Refactor/cleanup - #11
Conversation
- Add NewEnergyChart with smooth Catmull-Rom curves and time-aligned data positioning - Add NewTimeContextToggle for 1day/3day/1week/1month/3month/1year views - Implement time bucket aggregation with placeholder points for natural curves - Add precise time label positioning with 23-notch hourly grid system - Update data service with context-aware filtering and alignment logic - Integrate new chart components into Home screen navigation
- Extract Chat functionality into dedicated Chat screen component - Clean up Home.tsx by removing chat-related code (570→160 lines) - Add useChatInputFocus hook for input focus management - Configure Chat as formSheet modal in MainNavigator - Maintain Home as background with energy chart and time context - Remove unused imports and navigation listeners from Home
…ameters - Add ChartDisplayContext with 29 exported parameters for chart display management - Integrate time range shortcuts (1day, 3day, 1week, 1month, 3month, 1year) - Implement smart date range calculations with timezone support - Add formatted labels for ChartHeader (headerTopLabel, headerBottomLabel) - Include special handling for 'today' view with different display options - Fix month abbreviations to show 'Sept' instead of 'Sep' - Update TimeDisplayToggle to use context with backward compatibility - Modify ChartHeader to use context-provided formatted labels - Wrap Home screen with ChartDisplayContextProvider Provides centralized chart display logic with automatic dateStart/dateEnd calculation from shortcuts, scalable for custom date ranges.
…edundant code - Fix race conditions between TimeContext and ChartDisplayContext intervals - Eliminate 60+ lines of duplicate astronomical calculation logic - Create shared dateFormatters utility for consistent month formatting - Implement reactive dependency pattern using timeInfo timestamp - Consolidate formatMonthDay logic into reusable utility - Simplify TimeDisplayToggle backward compatibility logic - Add comprehensive refactoring documentation and validation report Architecture improvement: Single source of truth pattern with TimeContext managing intervals and ChartDisplayContext reacting to changes.
…docs - Complete modular architecture implementation - Remove temporary ChartHeader-analysis.md file - Consolidate chart display and time context improvements - Maintain 86% code reduction in Home.tsx - Preserve all functionality with cleaner separation of concerns
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @masonomara, 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 introduces a substantial architectural overhaul, primarily focusing on enhancing the application's data visualization and user context management. It centralizes time and location data handling, implements a new charting system, and significantly refactors the main user interface by separating chat functionalities into a dedicated screen. These changes aim to improve the application's modularity, maintainability, and prepare it for future feature development, while also updating core dependencies and server configurations.
Highlights
- Chart and Time Context Overhaul: The application's time and chart display systems have been significantly refactored. A new
ChartDisplayContextcentralizes time range management and display formatting, reacting to updates from a revampedTimeContextthat now integratesreact-native-localizeand geolocation for precise time and location data. The oldEnergyChartandContextTogglecomponents have been replaced withNewEnergyChart,ChartHeader, andTimeDisplayTogglefor improved visualization and time range selection (1D, 3D, 1W, 1M, 3M, 1Y). - UI/Logic Separation: The main
Homescreen has been streamlined to focus solely on chart display. All chat-related UI and logic, previously intertwined with the home screen, have been extracted and moved to a new, dedicatedChat.tsxscreen, enhancing modularity and maintainability. - Localization Integration: The
react-native-localizelibrary has been integrated across the Android and iOS build configurations (build.gradle,MainApplication.kt,settings.gradle,Podfile,Podfile.lock,Info.plist,package.json,package-lock.json), enabling better internationalization capabilities, particularly for date and time formatting. - Refactoring Validation and Documentation: Extensive self-documentation of the refactoring process has been added, including
refactor/state.jsonandrefactor/validation-report.md. These files detail the project's objectives (e.g., eliminating redundant code, fixing race conditions), completed tasks, architectural improvements (with before/after diagrams), and quality metrics, demonstrating a thorough and deliberate approach to the changes. - Server URL Updates: Hardcoded server endpoint configurations within
agentService.ts,authService.ts, andmcpService.tshave been updated to point totides-006.mpazbot.workers.dev, consolidating the application's backend communication.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This is a substantial refactoring pull request that modernizes the application's state management for time and charts, introducing a more robust TimeContext and a new ChartDisplayContext. The new chart implementation using Skia is a great improvement. The overall changes significantly enhance the architecture and maintainability. I've identified a few areas for improvement, primarily concerning duplicated and inconsistent date logic that should be centralized. I've also provided feedback on some placeholder code and an architectural question raised in one of the planning documents.
| const xDomain = useMemo(() => { | ||
| const now = new Date(); // Use current date | ||
| let startDate = new Date(now); | ||
|
|
||
| // Calculate full time range based on context | ||
| switch (timeDisplayContext) { | ||
| case "1day": | ||
| startDate.setDate(now.getDate() - 1); | ||
| return [startDate.getTime(), now.getTime()]; | ||
| case "3day": | ||
| startDate.setDate(now.getDate() - 3); | ||
| return [startDate.getTime(), now.getTime()]; | ||
| case "1week": | ||
| startDate.setDate(now.getDate() - 7); | ||
| return [startDate.getTime(), now.getTime()]; | ||
| case "1month": | ||
| startDate.setDate(now.getDate() - 31); | ||
| return [startDate.getTime(), now.getTime()]; | ||
| case "3month": | ||
| startDate.setDate(now.getDate() - 90); | ||
| return [startDate.getTime(), now.getTime()]; | ||
| case "1year": | ||
| startDate.setDate(now.getDate() - 365); | ||
| return [startDate.getTime(), now.getTime()]; | ||
| default: | ||
| return data.length > 0 | ||
| ? [ | ||
| Math.min(...data.map((d) => d.x)), | ||
| Math.max(...data.map((d) => d.x)), | ||
| ] | ||
| : [0, 1]; | ||
| } | ||
| }, [timeDisplayContext, data]); |
There was a problem hiding this comment.
This xDomain calculation is duplicated from ChartDisplayContext and is also inconsistent with it (e.g., 1month is 31 days here vs. 30 days in the context). This can lead to bugs and maintenance issues. The xDomain should be derived from a single source of truth.
Since this component is already wrapped in ChartDisplayContextProvider via Home.tsx, you can consume the context directly to get the date range. This would involve:
- Removing the
timeDisplayContextprop. - Using
useChartDisplayContext()to getdateStartanddateEnd. - Defining
xDomainbased on these context values.
| const requestLocationPermission = useCallback(async (): Promise<boolean> => { | ||
| setPermissions((prev) => ({ ...prev, location: "requesting" })); | ||
|
|
||
| try { | ||
| await fetchLocationAndAstronomicalData(); | ||
| return permissions.location === "granted"; | ||
| } catch (err) { | ||
| setPermissions((prev) => ({ ...prev, location: "denied" })); | ||
| return false; | ||
| } | ||
| }, [fetchLocationAndAstronomicalData, permissions.location]); |
There was a problem hiding this comment.
This function has a couple of issues:
- The
catchblock is unreachable becausefetchLocationAndAstronomicalDatahandles its own errors and doesn't re-throw. - The return statement
return permissions.location === "granted";uses a stale value ofpermissionsfrom theuseCallbackclosure. The state update insidefetchLocationAndAstronomicalDatais asynchronous and won't be reflected here.
To fix this, fetchLocationAndAstronomicalData should be modified to return a boolean indicating success, and this function should use that return value directly.
const requestLocationPermission = useCallback(async (): Promise<boolean> => {
setPermissions((prev) => ({ ...prev, location: "requesting" }));
// This assumes fetchLocationAndAstronomicalData is refactored to return a boolean for success.
const success = await fetchLocationAndAstronomicalData();
return success;
}, [fetchLocationAndAstronomicalData]);
| {Array.from({ length: 20 }, (_, i) => ( | ||
| <View | ||
| key={i} | ||
| style={{ | ||
| height: 100, | ||
| backgroundColor: i % 2 === 0 ? "pink" : "cyan", | ||
| marginVertical: 5, | ||
| marginHorizontal: 16, | ||
| }} | ||
| > | ||
| <Text | ||
| style={{ | ||
| fontSize: 24, | ||
| color: "black", | ||
| textAlign: "center", | ||
| paddingTop: 35, | ||
| }} | ||
| > | ||
| ITEM {i + 1} | ||
| </Text> | ||
| </View> | ||
| ))} | ||
| </ScrollView> |
There was a problem hiding this comment.
This appears to be placeholder content for testing the scroll view. It should be removed and replaced with the actual chat message rendering logic before this feature is finalized.
| **Question**: Single component with switch cases vs. 6 separate components? | ||
|
|
||
| **Recommendation needed** for optimal organization approach. |
There was a problem hiding this comment.
Regarding the architectural question, a single component with switch cases (or a similar conditional rendering strategy) is generally more maintainable for this use case. It avoids duplicating common logic like Skia canvas setup, scales, and layout. You can use helper functions or sub-components to handle the specific logic for each time context to keep the main component clean and organized.
1 participant
No description provided.