⚡ Optimize Date Parsing in calculateMostActiveDay - #513
Conversation
…g in loop Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughSummary by CodeRabbit
Walkthrough
Changesカレンダー集計
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
PR Summary by QodoOptimize calculateMostActiveDay by parsing the start date once
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
30 rules 1. Invalid first date poisons
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| const startDay = new Date(`${calendar[0].date}T00:00:00Z`).getUTCDay(); | ||
|
|
||
| for (const day of calendar) { | ||
| for (let i = 0; i < calendar.length; i++) { | ||
| const day = calendar[i]; | ||
| if (day.count === 0) { | ||
| continue; | ||
| } | ||
| const weekday = new Date(`${day.date}T00:00:00Z`).getUTCDay(); | ||
| const weekday = (startDay + i) % 7; | ||
| weekdayTotals[weekday] += day.count; |
There was a problem hiding this comment.
1. Invalid first date poisons 🐞 Bug ≡ Correctness
calculateMostActiveDay now computes all weekdays from calendar[0].date; if that first date is invalid, startDay becomes NaN and all contributions are accumulated into a non-numeric array property, causing the function to return null even when later days have valid dates and counts.
Agent Prompt
### Issue description
`calculateMostActiveDay` parses only the first calendar date and derives all subsequent weekdays via `(startDay + i) % 7`. If `calendar[0].date` is malformed/invalid, `new Date(...).getUTCDay()` yields `NaN`, and the modulo arithmetic produces `NaN` for every element. This causes writes to `weekdayTotals[NaN]` (a non-index property) and the function returns `null` despite nonzero contributions.
### Issue Context
Other code in this repo already treats invalid date parsing as a realistic possibility and guards with `Number.isNaN(date.getTime())` before using `getUTCDay()`.
### Fix Focus Areas
- src/lib/github.ts[91-113]
### Suggested fix
1. Parse the first date into a `Date` object and validate it using `Number.isNaN(date.getTime())`.
2. If invalid, fall back to the previous per-entry parsing approach (optionally with per-entry NaN checks), so later valid entries are still counted.
3. Keep the optimized modulo path when the first date is valid.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/github.ts`:
- Around line 98-105: Update calculateMostActiveDay to derive weekday from each
day.date rather than using startDay + i, so zero-count entries or missing
calendar dates cannot shift the weekday calculation. Preserve the existing count
aggregation and add validation covering non-contiguous dates across a week
boundary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d81c5223-2a9b-4c06-b33a-3da0a0428bca
📒 Files selected for processing (1)
src/lib/github.ts
| const startDay = new Date(`${calendar[0].date}T00:00:00Z`).getUTCDay(); | ||
|
|
||
| for (const day of calendar) { | ||
| for (let i = 0; i < calendar.length; i++) { | ||
| const day = calendar[i]; | ||
| if (day.count === 0) { | ||
| continue; | ||
| } | ||
| const weekday = new Date(`${day.date}T00:00:00Z`).getUTCDay(); | ||
| const weekday = (startDay + i) % 7; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline src/lib/github.ts --items all --type function
fi
rg -n -C 15 \
'calculateMostActiveDay|contributionCalendar|contributionDays|calendar' \
src/lib/github.ts \
src/lib/__tests__/github/fetchContributions.test.ts
rg -n -C 8 \
'\bcalculateMostActiveDay\b' \
--glob '*.{ts,tsx,js,jsx}' .Repository: Hiroki-org/github-user-summary
Length of output: 23085
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the test fixtures/top-level contributors collection to identify which mock is used by the sort test.
rg -n -C 12 'MOCK_CONTRIBUTIONS|contributionCollection|contributionCalendar|weeks|contributionDays' src/lib/__tests__/github src/lib/github.ts
# Read only the fixture definitions and relevant test body around the sort test.
python3 - <<'PY'
from pathlib import Path
p = Path("src/lib/__tests__/github/fetchContributions.test.ts")
txt = p.read_text()
for needle in ("const MOCK_CONTRIBUTIONS", "describe(", '"カレンダーデータが日付順にソートされている"'):
print(f"\n--- {needle!r} ---")
idx = txt.find(needle if needle.startswith(('const MOCK_CONTRIBUTIONS', '"')) else needle[1:])
# broad context around first occurrence for each
for s in ("const MOCK_CONTRIBUTIONS", '"カレンダーデータが日付順にソートされている"'):
i = txt.find(s)
if i >= 0:
lo = max(0, i - 300)
hi = min(len(txt), i + 2000)
print("\n\nTEXT:")
print(txt[lo:hi])
PY
# Behavioral probe: compare index-based weekday assignment vs date-based assignment for non-concentric ordered inputs.
python3 - <<'PY'
from datetime import datetime, UTC
calendar = [
{"date": "2024-01-01", "count": 1},
{"date": "2024-01-03", "count": 2},
{"date": "2024-01-05", "count": 3},
{"date": "2024-01-06", "count": 4},
]
weekdayTotals_index = [0] * 7
startDay = datetime.strptime(calendar[0]["date"], "%Y-%m-%d").utcdate().weekday()
for i, day in enumerate(calendar):
if day["count"] == 0:
continue
weekdayTotals_index[(startDay + i) % 7] += day["count"]
weekdayTotals_date = [0] * 7
for day in calendar:
if day["count"] == 0:
continue
weekdayTotals_date[datetime.strptime(day["date"], "%Y-%m-%d").utcdate().weekday()] += day["count"]
print(calendar)
print("index-based totals:", weekdayTotals_index)
print("date-based totals:", weekdayTotals_date)
print("index-based most-active weekdays:", [d for d, c in enumerate(weekdayTotals_index) if c == max(weekdayTotals_index)])
print("date-day names:", [["Sunday", "Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday"][d for d, c in enumerate(weekdayTotals_date) if c == max(weekdayTotals_date)][0]])
PYRepository: Hiroki-org/github-user-summary
Length of output: 32996
🌐 Web query:
GitHub GraphQL contributionCalendar contributionDays date order
💡 Result:
In the GitHub GraphQL API, the contribution calendar data is structured hierarchically, and the order of contribution days is deterministic based on the calendar timeline [1][2]. Within the contributionCalendar object, data is organized as follows: 1. Weeks: The contributionCalendar field contains a weeks list (ContributionCalendarWeek) [1][2]. This list is ordered chronologically by time, with each week representing a distinct seven-day period [1][2]. 2. Days: Each ContributionCalendarWeek object contains a contributionDays list (ContributionCalendarDay) [1][2]. Within each week, the days are ordered chronologically, starting from the first day of that week [1][2]. Each day object includes a date field, which allows you to programmatically verify the chronological sequence [1][2]. You can retrieve this information by querying the user's contributionsCollection [3][4]. Since the API returns these objects in an ordered array structure (weeks followed by the days within those weeks), you do not need to perform manual sorting to maintain the chronological order of the calendar [1][4].
Citations:
- 1: https://docs.github.com/en/graphql/reference/users
- 2: https://docs.github.com/en/enterprise-cloud@latest/graphql/reference/users
- 3: https://stackoverflow.com/questions/18262288/finding-total-contributions-of-a-user-from-github-api
- 4: https://python.code-maven.com/python-github/github/github-graphql-contribution-counts
曜日を day.date から取得してください。
calculateMostActiveDay() は startDay + i で実際に存在しない曜日にカウントを加算します。calendar が昇順でも欠落日を含む場合は曜日より大きなズレになります。週境界を超えて連続していない日付での mostActiveDay 検証を追加してください。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/github.ts` around lines 98 - 105, Update calculateMostActiveDay to
derive weekday from each day.date rather than using startDay + i, so zero-count
entries or missing calendar dates cannot shift the weekday calculation. Preserve
the existing count aggregation and add validation covering non-contiguous dates
across a week boundary.
1 participant
💡 What: The optimization refactors
calculateMostActiveDayto parse theDateobject exactly once for the first day in the calendar array, rather than parsing a newDateobject for every single day. Subsequent weekdays are calculated using simple modulo arithmetic(startDay + i) % 7.🎯 Why: The calendar array represents consecutive days (up to 365 days). Instantiating a new
Dateobject inside the loop adds unnecessary parsing and object allocation overhead.📊 Measured Improvement: A local benchmark using 10,000 iterations over a 365-day mock calendar showed a massive reduction in execution time:
PR created automatically by Jules for task 6923459134969605479 started by @is0692vs
Greptile Summary
calculateMostActiveDayの日付解析を先頭要素の1回だけに減らし、連続する日付の曜日をインデックスから算出する最適化です。nullとして返します。Confidence Score: 5/5
現在の contribution calendar の生成経路では連続した日付列が渡されるため、このPRは安全にマージできると考えます。
空配列は明示的に処理され、通常の連続日付列では先頭曜日とインデックスから従来と同じ曜日区分が得られます。
Important Files Changed
Reviews (1): Last reviewed commit: "perf(github): optimize calculateMostActi..." | Re-trigger Greptile
Context used: