refactor: clean up timezone boundaries, add goal auto-progress sync, style error UI, secure user auth, improve accessibility, and optimize performance [tested]#202
Conversation
|
@omkhandare55 is attempting to deploy a commit to the PRIYANSHU DOSHI's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Thanks for your first PR on DevTrack! 🎉
A maintainer will review it within 48 hours. While you wait:
- Make sure CI is passing (type-check + lint)
- Double-check the PR description is filled out and the issue is linked
- Feel free to ask questions in Discussions if you need help
|
merge conflicts, resolve them. |
|
yes |
505bdc3 to
712b322
Compare
|
done |
|
please add gssoc lables |
|
Closes #190 — cc @Priyanshu-byte-coder for label assignment and CI approval 🙏 |
Priyanshu-byte-coder
left a comment
There was a problem hiding this comment.
Feature concept is solid, but two bugs in the sync route prevent it from working.
Bug 1 — wrong column name (sync always returns 0 goals)
In sync/route.ts:
The query uses .eq("week_start", currentWeekStart()) but the column is period_start — the same column set by getPeriodStart() in the existing goals route. Because week_start does not exist, Supabase silently returns 0 rows and no goals ever update.
Also, period_start is a full ISO timestamp so a string equality match on a date won't hit. Use a range filter:
const { data: commitGoals } = await supabaseAdmin
.from('goals')
.select('id')
.eq('user_id', user.id)
.eq('unit', 'commits')
.gte('period_start', currentWeekStart() + 'T00:00:00.000Z')
.lte('period_start', currentWeekEnd());Bug 2 — currentWeekStart() is wrong on Sundays + timezone bug
d.setDate(now.getDate() - now.getDay() + 1) — on Sunday (now.getDay() === 0): -0 + 1 = +1, giving next Monday instead of this week's Monday. The existing getPeriodStart() already handles this with const diff = day === 0 ? -6 : 1 - day.
Also .toISOString().slice(0, 10) uses UTC — same timezone bug fixed by PR #198. Correct version:
function currentWeekStart(): string {
const now = new Date();
const day = now.getDay();
const diff = day === 0 ? -6 : 1 - day;
const monday = new Date(now);
monday.setDate(now.getDate() + diff);
return monday.getFullYear() + '-'
+ String(monday.getMonth() + 1).padStart(2, '0') + '-'
+ String(monday.getDate()).padStart(2, '0');
}Everything else looks good — refresh button, auto-synced badge, unit dropdown, migration are all clean. Fix these two and this is ready.
|
Hi @Priyanshu-byte-coder, I've addressed both bugs from your review:
Could you also please add the appropriate GSSoC labels ( |
|
/review |
|
This PR conflicts with recently merged changes. Please rebase onto main: |
40eb77e to
85def43
Compare
|
Great feature — the sync logic, confetti burst, and GoalTracker UI all look solid. Three fixes needed:
Fix these three and this is mergeable. |
|
Please merge it |
|
Thanks for the review, @Priyanshu-byte-coder! I've gone ahead and pushed those three fixes: Migration timestamp conflict: Renamed the migration to 20260519000000_add_goal_unit_and_sync.sql so it cleanly follows the RLS migration. |
|
please addd gssoc approved tags |
Priyanshu-byte-coder
left a comment
There was a problem hiding this comment.
1. Migration default mismatch — migration sets unit text not null default 'manual' but sync route filters on unit = 'commits'. All existing goals stay 'manual' and never auto-sync. Change migration default to 'commits'.
2. Server timezone bug — setHours(0,0,0,0) uses server local time. Use setUTCHours(0,0,0,0) for consistent week boundaries.
3. Silent sync failures — catch block logs only to console. Show an inline error or toast when sync fails (expired token, missing repo scope).
4. Missing EOF newline on GoalTracker.tsx.
92c4efb to
0cc3d1e
Compare
|
Hi @Priyanshu-byte-coder! I have successfully:
Since this required database migrations, timezone fixes, and clean error handling UI over 3 days of work, could you please review and apply the |
…on logic, and cache api queries
|
hi @Priyanshu-byte-coder please merge it |
|
@Priyanshu-byte-coder please add level label |
Priyanshu-byte-coder
left a comment
There was a problem hiding this comment.
Issues found in this PR:
-
Missing EOF newline — add a trailing newline to all modified files.
-
Raw Tailwind color classes — replace
text-red-*/bg-red-*withtext-[var(--destructive)]/ appropriate CSS var equivalents. All colors must use CSS variables for theme support.
|
hi @Priyanshu-byte-coder |
Summary
Closes #190
Goals with
unit = 'commits'now auto-update theircurrentvalue by reading the user's actual GitHub commit count for the current week — no manual updates needed.Changes
supabase/migrations/20260517000000_add_goal_unit_and_sync.sqlunitcolumn ('commits'|'manual', default'manual')last_synced_atcolumn (set on every sync)src/app/api/goals/sync/route.ts(new file)POST /api/goals/sync— queries GitHub search API for commits in the current week, updates allunit = 'commits'goals with the real countsrc/app/api/goals/route.tsPOST /api/goalsnow accepts an optionalunitfield ('commits'|'manual')src/components/GoalTracker.tsxPOST /api/goals/syncwhen the dashboard mountscurrentvalue is never overwrittenAcceptance Criteria
unit = 'commits'auto-updatecurrentfrom/api/metrics/contributionsPOST /api/goals/sync— fetches contributions for current period, updatescurrentfor all commit-based goals