Normalize legacy weather cache labels#348
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 pull request introduces a normalization process for legacy weather cache entries to ensure consistent metadata across the application. It includes updates to the weather service for data retrieval and the UI component for proper labeling, along with corresponding tests. Feedback identifies logic inconsistencies regarding the treatment of the current date—specifically, that it is incorrectly labeled as historical/recorded rather than a forecast—and warns of potential timezone-related errors in date comparisons.
| : 'open-meteo'; | ||
| const isHistorical = typeof rawDay.isHistorical === 'boolean' | ||
| ? rawDay.isHistorical | ||
| : dataSource === 'historical-average' || day.date <= todayISO; |
There was a problem hiding this comment.
The comparison day.date <= todayISO incorrectly marks the current day as historical. In the main fetch logic (fetchOpenMeteoDaily, line 545), today is treated as a forecast. This inconsistency will cause legacy cache entries containing today's date to be labeled as "Recorded" instead of "Forecast" when read and normalized.
| : dataSource === 'historical-average' || day.date <= todayISO; | |
| : dataSource === 'historical-average' || day.date < todayISO; |
| if (hasForecast) return 'Forecast'; | ||
| if (hasRecorded) return 'Recorded'; | ||
| if (!hasExplicitSourceMetadata) { | ||
| return sourceDays.every(d => d.date > todayISO) ? 'Hist. avg.' : 'Recorded'; |
There was a problem hiding this comment.
The use of todayISO (defined as UTC at line 14) for comparison with d.date (which follows local-date semantics from the service) can lead to off-by-one errors in labeling depending on the user's timezone and time of day. Additionally, this fallback logic labels today as "Recorded" (since today > todayISO is false), which is inconsistent with the "Forecast" label usually applied to the current day. Consider using a consistent date representation and matching the service's heuristic for today's status.
References
- Maintain consistent local-calendar-day semantics for both reading and writing to avoid bugs caused by mixed UTC and local time behavior.
Confidence Score: 3/5Safe to merge for the targeted blank-label fix, but the normalization logic will mislabel future days in any legacy cache entry that spans both past and future dates. The legacyAllFuture flag treats the entire daily-weather batch as a single unit. For a legacy cache entry whose trip start date has since passed, the remaining future days get isForecast:true because the flag is false (some past dates exist) and the fallback path sets open-meteo + !isHistorical = isForecast. Those future days were originally fetched as historical-average data, so 'Forecast' is a misleading label. The tests cover the two clean scenarios (all-future, all-past) but not the mixed-range case that triggers the incorrect classification. src/app/services/weatherService.ts — specifically the legacyAllFuture decision and the per-day dataSource assignment in normalizeCachedWeatherSummary. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[readCache] --> B[filter expired dailyWeather]
B --> C[normalizeCachedWeatherSummary]
C --> D{hasExplicitSourceMetadata?}
D -- yes --> E[keep existing dataSource / infer missing flags per day]
D -- no --> F{legacyAllFuture? all days > today?}
F -- yes --> G[dataSource = historical-average, isHistorical = true, isForecast = false]
F -- no --> H[dataSource = open-meteo, isHistorical = day <= today, isForecast = open-meteo AND NOT isHistorical]
H --> I[future days in mixed range get isForecast=true, should be historical-average]
E --> J[WeatherSummary component]
G --> J
J --> K{hasHistAvg / hasForecast / hasRecorded?}
K -- flags set --> L[Return matching label]
K -- no flags AND not hasExplicitSourceMetadata --> M[Fallback: all-future = Hist. avg. else Recorded]
Reviews (1): Last reviewed commit: "Normalize legacy weather cache labels" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const legacyAllFuture = !hasExplicitSourceMetadata && summary.dailyWeather.every(day => day.date > todayISO); | ||
|
|
||
| return { | ||
| ...summary, | ||
| dailyWeather: summary.dailyWeather.map(day => { | ||
| const rawDay = day as Partial<WeatherData>; | ||
| const dataSource = isKnownWeatherDataSource(rawDay.dataSource) | ||
| ? rawDay.dataSource | ||
| : legacyAllFuture | ||
| ? 'historical-average' | ||
| : 'open-meteo'; | ||
| const isHistorical = typeof rawDay.isHistorical === 'boolean' | ||
| ? rawDay.isHistorical | ||
| : dataSource === 'historical-average' || day.date <= todayISO; |
There was a problem hiding this comment.
legacyAllFuture is an all-or-nothing flag: if any day in the summary has a past date, every day without explicit source metadata gets dataSource: 'open-meteo'. For the future dates in that batch, the subsequent isForecast inference then resolves to true (open-meteo + not historical → forecast). Legacy entries for a trip that was originally cached entirely in the future but whose start date has since passed will therefore show "Forecast" in the UI for the remaining future days, even though that data came from the historical-average API. Per-day inference — day.date > todayISO ? 'historical-average' : 'open-meteo' — would correctly handle these mixed-range entries.
| /** | ||
| * @jest-environment jsdom | ||
| */ | ||
|
|
||
| import { render, screen } from '@testing-library/react'; | ||
| import '@testing-library/jest-dom'; | ||
|
|
||
| import WeatherSummary from '@/app/components/Weather/WeatherSummary'; | ||
| import type { WeatherSummary as WeatherSummaryType } from '@/app/types/weather'; | ||
|
|
||
| const buildLegacySummary = (dates: string[]): WeatherSummaryType => ({ | ||
| locationId: 'location-1', | ||
| startDate: dates[0], | ||
| endDate: dates[dates.length - 1], | ||
| dailyWeather: dates.map(date => ({ | ||
| id: `weather-${date}`, | ||
| date, | ||
| coordinates: [47.5, 19] as [number, number], | ||
| temperature: { min: 10, max: 20, average: 15 }, | ||
| precipitation: { total: 0, probability: null }, | ||
| wind: { speed: null, direction: null }, | ||
| conditions: { description: 'Clear', icon: 'sun', code: 0, cloudCover: null, humidity: null }, | ||
| fetchedAt: '2026-06-01T00:00:00.000Z', | ||
| })) as WeatherSummaryType['dailyWeather'], | ||
| summary: { | ||
| averageTemp: 15, | ||
| totalPrecipitation: 0, | ||
| predominantCondition: 'Clear', | ||
| }, | ||
| }); | ||
|
|
||
| describe('WeatherSummary', () => { | ||
| afterEach(() => { | ||
| jest.useRealTimers(); | ||
| }); | ||
|
|
||
| it('labels all-future legacy cached weather as historical averages', () => { | ||
| jest.useFakeTimers(); | ||
| jest.setSystemTime(new Date('2026-06-01T12:00:00.000Z')); | ||
|
|
||
| render(<WeatherSummary summary={buildLegacySummary(['2027-06-01', '2027-06-02'])} />); | ||
|
|
||
| expect(screen.getByText('Weather · Hist. avg.')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('labels past legacy cached weather as recorded instead of rendering a blank source label', () => { | ||
| jest.useFakeTimers(); | ||
| jest.setSystemTime(new Date('2026-06-01T12:00:00.000Z')); | ||
|
|
||
| render(<WeatherSummary summary={buildLegacySummary(['2026-05-31', '2026-06-01'])} />); | ||
|
|
||
| expect(screen.getByText('Weather · Recorded')).toBeInTheDocument(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
AGENTS.md requires all components to be tested with jest-axe for WCAG AA compliance. This new test file exercises WeatherSummary rendering but does not call axe(container) / expect(results).toHaveNoViolations(). Adding at least one jest-axe pass-through check would keep the file consistent with the project standard.
1 participant
Summary
Validation