feat/gamification #14
Conversation
📝 WalkthroughWalkthroughAdds Phase 2 gamification: server routes for stats, leaderboards, and GitHub webhooks; Prisma models for achievements and weekly stats; Redis keys/TTLs and achievement constants/types; VS Code extension views/providers for Stats and Leaderboard with periodic refresh; and an optional GITHUB_WEBHOOK_SECRET env var (optional string, trimmed, min length 32). Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Ext as Extension
participant Server
participant Redis
participant DB as PostgreSQL
User->>Ext: Login
activate Ext
Ext->>Ext: startStatsRefresh() (every 60s)
loop Every 60s
Ext->>Server: GET /api/v1/stats/me
activate Server
Server->>Redis: Read streak/session/leaderboard scores
Server->>DB: Load weekly stats & recent achievements
Server-->>Ext: UserStatsDTO
deactivate Server
Ext->>Ext: updateStats() → UI refresh
Ext->>Server: GET /api/v1/leaderboards/friends
activate Server
Server->>Redis: ZSCORE/ZREVRANGE friend pipeline
Server->>DB: Fetch friend user details
Server-->>Ext: LeaderboardEntry[]
deactivate Server
Ext->>Ext: updateLeaderboard() → UI refresh
end
User->>Ext: Logout
Ext->>Ext: stopStatsRefresh()
deactivate Ext
sequenceDiagram
participant GitHub as GitHub
participant Webhook as Webhook Handler
participant Redis
participant DB as PostgreSQL
participant Broadcast as Broadcasting
GitHub->>Webhook: POST /webhooks/github (X-Hub-Signature-256)
activate Webhook
Webhook->>Webhook: verifyGitHubSignature(rawBody, signature, secret)
alt Signature Valid
alt Event: push
Webhook->>Redis: ZINCRBY weeklyCommits (userId, commitCount)
else Event: issues closed
Webhook->>DB: Create Achievement (ISSUE_CLOSED) if not exists
Webhook->>Broadcast: Broadcast to followers & user
else Event: pull_request merged
Webhook->>DB: Create Achievement (PR_MERGED) if not exists
Webhook->>Broadcast: Broadcast to followers & user
else Event: ping
Webhook->>Webhook: Log ping
end
Webhook-->>GitHub: 200 OK
else Signature Invalid
Webhook-->>GitHub: 401 Unauthorized
end
deactivate Webhook
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🤖 Fix all issues with AI agents
In @.env.example:
- Around line 18-20: Update the inline comment above GITHUB_WEBHOOK_SECRET to
use the consistent terminology "Gamification" instead of "Boss Battles" (e.g.,
change "Boss Battles - achievements from GitHub events" to "Gamification -
achievements from GitHub events") so the .env.example comment matches the rest
of the PR references.
In @apps/extension/src/extension.ts:
- Around line 282-303: The dispose() implementation doesn't clear the
NodeJS.Timeout stored in statsRefreshInterval, causing the interval to keep
running after deactivation; update dispose() to call stopStatsRefresh() (or
explicitly clearInterval(this.statsRefreshInterval) and set it to null) before
iterating disposables so that startStatsRefresh/stopStatsRefresh and the
statsRefreshInterval are properly cleaned up on dispose.
In @apps/extension/src/views/leaderboardProvider.ts:
- Around line 43-52: The formatScore method currently returns "0m" for seconds
=== 0 which may be unclear; update the private formatScore(seconds: number):
string to explicitly handle the zero case by returning a clearer label (e.g.,
"No activity" or "0m" based on product decision). Locate formatScore in
leaderboardProvider.ts and add a guard at the top that checks if seconds === 0
and returns the chosen string, otherwise proceed with the existing hours/minutes
formatting logic.
- Around line 150-157: The log in updateLeaderboard reports entries.length even
though the method truncates and stores only the top 10 in this.leaderboard;
change the logger.debug call to report the actual stored count (e.g.,
this.leaderboard.length) and keep myRank/context in the payload so the log
reflects the truncated leaderboard size.
- Around line 73-82: LeaderboardHeaderItem currently hardcodes this.id =
'leaderboard-header', causing duplicate IDs if multiple headers are created;
modify the LeaderboardHeaderItem constructor to accept a uniqueId (or compute
one from the title/count) and set this.id = uniqueId (e.g., `${title}-${count}`
or pass an explicit id param) so each instance has a distinct id; update any
call sites that instantiate LeaderboardHeaderItem to pass the new id argument
(or rely on the generated unique string).
In @apps/extension/src/views/statsProvider.ts:
- Around line 116-127: The code redundantly checks latestAchievement after
confirming this.stats.recentAchievements.length > 0; remove the inner if
(latestAchievement) and directly push the new StatsTreeItem using the first
element (e.g., const latestAchievement = this.stats.recentAchievements[0];
items.push(new StatsTreeItem(latestAchievement.title, 'Latest achievement',
'trophy'))), keeping the same variables and StatsTreeItem construction.
In @apps/server/prisma/schema.prisma:
- Around line 56-74: Leaderboards are never reset so weekly scores accumulate
indefinitely; fix by making Redis leaderboard keys week-scoped or adding a
weekly cron that resets them at Monday 00:00 UTC: either (A) change all places
that reference REDIS_KEYS.weeklyLeaderboard (leaderboards.ts, webhooks.ts) to
include the weekStart computed by getWeekStart() so keys become per-week (e.g.,
REDIS_KEYS.weeklyLeaderboard(weekStart) for both 'time' and 'commits'), or (B)
add a scheduled job that runs Monday 00:00 UTC to atomically purge/replace
REDIS_KEYS.weeklyLeaderboard('time') and REDIS_KEYS.weeklyLeaderboard('commits')
(use a single atomic Redis command or Lua script to avoid race conditions), and
update webhook increment logic in webhooks.ts to write to the week-scoped keys
or to the newly rotated keys.
In @apps/server/src/config.ts:
- Around line 36-37: Update the comment to use consistent "Gamification"
terminology instead of "Boss Battles" and add a minimum length validation to the
GITHUB_WEBHOOK_SECRET schema entry; specifically, change the comment above
GITHUB_WEBHOOK_SECRET to reference "Gamification" and modify the schema for
GITHUB_WEBHOOK_SECRET (z.string().optional()) to z.string().min(8).optional() so
that when a secret is provided it must meet the minimum length while keeping the
field optional.
In @apps/server/src/routes/stats.ts:
- Around line 251-256: pipeline.exec() is called without handling its returned
Promise or results, so Redis command failures are ignored; update the code
around pipeline.exec() to await its result, check the returned reply for errors
(or catch exceptions), and log or handle any failures via the existing logger
before proceeding (use the same error pathing as /me); reference the
pipeline.exec() call and ensure any async fire-and-forget work like void
checkStreakAchievements(userId, newStreak) still runs only after pipeline.exec()
success or after errors are logged.
- Around line 246-249: The code unconditionally uses the incoming language value
in pipeline.hincrby(REDIS_KEYS.networkIntensity(minute), `lang:${language}`, 1),
risking unbounded Redis hash growth; before that call validate and normalize the
language (e.g., lowercase and trim), check it against an allowlist (e.g.,
LANG_ALLOWLIST set of known ISO codes) or enforce a max length and map anything
not allowed to a safe bucket like 'lang:other', and only then call
pipeline.hincrby; update references around the language variable and the
pipeline.hincrby call to use the validated/normalized value.
- Around line 78-88: The pipeline result handling ignores per-command errors;
after calling pipeline.exec() inspect each tuple in results (the [error, result]
pairs returned by pipeline.exec()), check results?.[0]?.[0] and
results?.[1]?.[0] (or loop over results) for non-null errors before reading
results?.[i]?.[1] for REDIS_KEYS.streakData and REDIS_KEYS.dailySession; on
error either log the error with context (including userId and the key name) and
return/throw an appropriate error or fallback to safe defaults so you don’t
silently use stale/null data when extracting streakData and todaySessionStr.
- Around line 320-376: The achievement check in checkStreakAchievements
currently uses strict equality (streak === milestone.streak) so missed
milestones if streak jumps; change the condition to use >= and iterate
milestones in descending order (largest to smallest) so the highest applicable
milestone is awarded (e.g., sort or define milestones as [100,30,7]); keep the
existing deduplication via db.achievement.findFirst and the break after creating
an achievement to ensure only one award per update; update references to
milestones, milestone.streak, db.achievement.findFirst/create, and the break
logic accordingly.
- Around line 253-256: The fire-and-forget call to
checkStreakAchievements(userId, newStreak) can swallow thrown errors; change the
invocation to append a .catch(...) that logs the error for observability (e.g.,
checkStreakAchievements(userId, newStreak).catch(err => /* use existing logger
like processLogger.error(...) or console.error */)); ensure you reference the
same userId and newStreak values and use the project's logger instance to record
context about the failure.
- Around line 194-236: The current read-then-pipeline update using
existingStreakData and pipeline.hset(REDIS_KEYS.streakData(userId)) is
vulnerable to races; replace the separate hgetall + pipeline sequence with an
atomic Redis operation: either wrap the logic in WATCH/ MULTI/EXEC (watch
REDIS_KEYS.streakData(userId), re-read inside the transaction, compute newStreak
and longest, then MULTI.hset/expire and EXEC) or implement the read-modify-write
entirely in a Lua script invoked via EVAL that reads the hash, computes
daysDiff/newStreak/longest based on lastDate, sets count/lastDate/longest and
TTL atomically; ensure you reference REDIS_KEYS.streakData(userId), sessionKey
logic remains unchanged, and only perform the streak update when lastDate !==
today inside the atomic transaction/script.
In @apps/server/src/routes/webhooks.ts:
- Around line 216-230: The create call for achievements (db.achievement.create
in the webhook handler) assumes any error is a duplicate; instead prevent
misclassifying other DB errors by either using db.achievement.upsert with a
unique key (e.g., userId + type + metadata.issueNumber) so duplicates
update/no-op, or explicitly check via db.achievement.findFirst({ where: {
userId, type, metadata: { path: ['issueNumber'], equals: issue.number } } }) and
only call create when not found; apply the same change pattern to the other
block referenced (the similar create at lines ~284-298) so both avoid treating
non-duplicate DB errors as duplicates and preserve correct error handling.
- Around line 159-169: The switch currently casts payload unsafely before
calling handleIssuesEvent, handlePullRequestEvent, and handlePushEvent; replace
those direct assertions with Zod (or equivalent) schemas for each event (e.g.,
GitHubIssuesPayloadSchema, GitHubPullRequestPayloadSchema,
GitHubPushPayloadSchema), safeParse the incoming payload in the 'issues',
'pull_request', and 'push' cases, call the respective handler only on
parsed.success, and log/warn with parsed.error when validation fails (follow the
SessionPayloadSchema pattern used in stats.ts for schema structure and error
handling).
- Around line 122-128: The handler re-serializes request.body with
JSON.stringify (bodyString/rawBody) which breaks GitHub signature verification;
instead preserve and use the original raw bytes via Fastify's raw body support:
configure addContentTypeParser('application/json', { parseAs: 'buffer' }, ...)
to populate request.rawBody and then replace the JSON.stringify(request.body)
usage in the webhook handler to read request.rawBody (or
Buffer.from(request.rawBody) when needed) before computing/verifying the
signature.
- Around line 114-121: The /github webhook route currently disables rate
limiting entirely; instead, configure @fastify/rate-limit for that route with an
IP allowlist that fetches GitHub delivery IPs (from the GitHub /meta API) so
legitimate GitHub requests bypass limits while all other IPs are rate-limited;
ensure the X-Hub-Signature-256 signature verification runs as the first
middleware for the handler (reject invalid signatures immediately), and change
the handler for app.post('/github') to return a quick 2xx response and enqueue
payloads for asynchronous processing to avoid retries.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
.env.exampleapps/extension/CHANGELOG.mdapps/extension/package.jsonapps/extension/src/extension.tsapps/extension/src/views/index.tsapps/extension/src/views/leaderboardProvider.tsapps/extension/src/views/statsProvider.tsapps/server/prisma/schema.prismaapps/server/src/config.tsapps/server/src/routes/leaderboards.tsapps/server/src/routes/stats.tsapps/server/src/routes/webhooks.tsapps/server/src/server.tspackages/shared/src/constants.tspackages/shared/src/types.ts
🧰 Additional context used
🧬 Code graph analysis (5)
apps/extension/src/views/leaderboardProvider.ts (2)
packages/shared/src/types.ts (1)
LeaderboardEntry(209-220)apps/server/src/lib/logger.ts (1)
logger(68-68)
apps/server/src/routes/webhooks.ts (4)
apps/server/src/services/db.ts (1)
getDb(20-33)packages/shared/src/constants.ts (2)
ACHIEVEMENTS(95-128)REDIS_KEYS(35-55)apps/server/src/ws/handler.ts (1)
broadcastToUsers(405-412)apps/server/src/services/redis.ts (1)
getRedis(60-63)
apps/server/src/server.ts (3)
apps/server/src/routes/stats.ts (1)
statsRoutes(68-377)apps/server/src/routes/leaderboards.ts (1)
leaderboardRoutes(37-375)apps/server/src/routes/webhooks.ts (1)
webhookRoutes(103-348)
apps/extension/src/extension.ts (2)
apps/extension/src/views/index.ts (2)
StatsProvider(5-5)LeaderboardProvider(6-6)packages/shared/src/types.ts (3)
UserStatsDTO(244-252)LeaderboardEntry(209-220)AchievementPayload(235-241)
apps/extension/src/views/statsProvider.ts (2)
packages/shared/src/types.ts (3)
StreakInfo(177-188)WeeklyStatsDTO(191-206)AchievementDTO(164-171)apps/server/src/lib/logger.ts (1)
logger(68-68)
🪛 GitHub Actions: CI
apps/extension/src/views/index.ts
[warning] 1-1: Prettier formatting issues detected by the format:check. Run 'prettier --write' to fix.
apps/server/src/routes/stats.ts
[warning] 1-1: Prettier formatting issues detected by the format:check. Run 'prettier --write' to fix.
apps/extension/src/views/leaderboardProvider.ts
[warning] 1-1: Prettier formatting issues detected by the format:check. Run 'prettier --write' to fix.
apps/server/src/routes/webhooks.ts
[warning] 1-1: Prettier formatting issues detected by the format:check. Run 'prettier --write' to fix.
packages/shared/src/types.ts
[warning] 1-1: Prettier formatting issues detected by the format:check. Run 'prettier --write' to fix.
apps/server/src/routes/leaderboards.ts
[warning] 1-1: Prettier formatting issues detected by the format:check. Run 'prettier --write' to fix.
apps/extension/src/extension.ts
[warning] 1-1: Prettier formatting issues detected by the format:check. Run 'prettier --write' to fix.
apps/extension/src/views/statsProvider.ts
[warning] 1-1: Prettier formatting issues detected by the format:check. Run 'prettier --write' to fix.
🪛 GitHub Check: CodeQL
apps/server/src/routes/webhooks.ts
[failure] 122-184: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
🔇 Additional comments (39)
apps/extension/package.json (3)
5-5: LGTM! Appropriate version bump.The version bump from 0.2.0 to 0.3.0 correctly follows semantic versioning for the new gamification features being introduced.
44-48: LGTM! Stats view properly declared.The new "My Stats" view is correctly configured with proper ID, name, and contextual title. The placement at the top of the views list will make stats prominent in the UI.
55-59: LGTM! Leaderboard view properly declared.The new "Leaderboard" view is correctly configured with proper ID, name, and contextual title.
apps/server/prisma/schema.prisma (2)
31-33: LGTM! User relations properly extended.The new one-to-many relations for achievements and weekly stats are correctly defined and align with the new models introduced below.
39-54: The Achievement duplicate handling is already implemented at the application level—no schema change needed.Milestone achievements (STREAK_7, STREAK_30, STREAK_100) prevent duplicates in
stats.tsviafindFirst(...where: { userId, type })before creating. Event achievements (ISSUE_CLOSED, PR_MERGED) inwebhooks.tsintentionally allow repeats, handled gracefully with a try/catch fallback that logs "Likely a duplicate - that's OK". The absence of a unique constraint in the schema is intentional to support repeatable event-based achievements while milestone achievements are protected at the application level.Likely an incorrect or invalid review comment.
packages/shared/src/constants.ts (4)
10-24: LGTM! Well-designed gamification constants.The constants are well-thought-out:
STREAK_TTL_SECONDS(25 hours) provides timezone flexibilitySESSION_TTL_SECONDS(48 hours) allows session overlap across day boundariesNETWORK_ACTIVITY_TTL_SECONDS(2 minutes) is appropriate for real-time activity tracking- Clear comments explain the purpose of each constant
36-54: LGTM! Consistent Redis key naming.The expanded Redis keys are well-organized with:
- Clear section comments separating Phase 1 and Phase 2 keys
- Consistent naming patterns across all keys
- Appropriate parameterization (userId, date, metric, minute)
- Good separation of concerns (streaks, leaderboards, sessions, network activity)
91-128: LGTM! Well-designed achievement system.The achievement definitions are comprehensive and user-friendly:
- Cover multiple categories (GitHub events, streaks, time-based)
- Include engaging emojis for UI presentation
- Clear, concise descriptions
- Structure allows easy extension with new achievements
- Using
as constproperly ensures type safety
130-131: LGTM! Type-safe achievement keys.Deriving
AchievementTypeKeyfrom theACHIEVEMENTSobject keys ensures type safety and automatic updates when new achievements are added.apps/extension/src/views/index.ts (1)
5-7: LGTM!The new exports for
StatsProvider,StatsData, andLeaderboardProviderfollow the existing pattern. The Prettier warning from CI should be addressed by runningprettier --write.apps/server/src/server.ts (2)
27-30: LGTM!The new route imports are correctly organized alongside existing route imports.
203-213: LGTM!Route registration is well-organized:
statsRoutesandleaderboardRoutescorrectly placed under/api/v1with the other API routeswebhookRoutesappropriately placed at root/webhooksfor external GitHub webhook callbacksapps/extension/CHANGELOG.md (1)
8-46: LGTM!Comprehensive changelog entry documenting Phase 2 gamification features. The structure follows Keep a Changelog format and provides good technical context for the changes.
apps/extension/src/views/statsProvider.ts (4)
14-19: LGTM!The
StatsDatainterface correctly mirrorsUserStatsDTOfrom@devradar/shared, ensuring type consistency between the extension and server.
52-59: LGTM!Good UX considerations with the loading spinner state and fallback message for first-time users.
63-83: LGTM!The streak display logic is well-implemented with appropriate emoji tiers (🏆 for 30+, 🔥 for 7+, ⚡ otherwise) and clear status indicators for active/at-risk/broken states.
160-164: LGTM!Proper disposal pattern iterating through all registered disposables.
packages/shared/src/types.ts (4)
147-161: LGTM!Well-structured achievement types covering GitHub-integration achievements (ISSUE_CLOSED, PR_MERGED), streak milestones, and time-based achievements.
173-188: LGTM!The
StreakInfointerface is comprehensive with clear documentation. TheStreakStatustype provides good semantic clarity for UI state handling.
190-206: LGTM!
WeeklyStatsDTOcorrectly marksrankas optional since users may not be ranked if they have no activity.
208-252: LGTM!The remaining gamification types (
LeaderboardEntry,NetworkActivity,AchievementPayload,UserStatsDTO) are well-designed and properly documented with JSDoc comments.apps/extension/src/extension.ts (4)
21-25: LGTM!New imports are correctly added for the gamification providers and shared types.
40-43: LGTM!Instance variables for Phase 2 gamification are properly declared with appropriate types.
305-353: LGTM!The fetch methods have proper error handling with:
- Early return if no auth token
- Try-catch with graceful logging on failure
- Response status logging for non-OK responses
535-555: LGTM!The enhanced
handleAchievementmethod correctly:
- Differentiates between user's own achievements and friends' achievements
- Customizes the notification message accordingly
- Triggers a stats refresh for the user's own achievements
apps/server/src/routes/leaderboards.ts (5)
43-126: LGTM!The
/weekly/timeendpoint is well-implemented:
- Proper pagination with
ZREVRANGEbounds calculation- Efficient
WITHSCORESto avoid separate score lookups- Friend status lookup for UI hints
- Appropriate cache headers for balancing freshness and performance
131-204: LGTM!The
/weekly/commitsendpoint follows the same robust pattern as/weekly/timewith efficient parallel DB queries viaPromise.all.
209-308: LGTM!The
/friendsendpoint efficiently handles the friends-only leaderboard:
- Uses Redis pipeline for batch
ZSCORElookups- Properly handles edge cases (no friends, no scores)
- Sorts and builds rank in-memory for the small friend set
313-374: LGTM!The
/network-activityendpoint provides a lightweight heatmap aggregation:
- 5-minute sliding window for recency
- Language counting with
lang:prefix convention- Short 10-second cache for near-real-time feel while reducing load
36-38: The original concern about initialization order is unfounded.getDb()synchronously creates a PrismaClient instance without establishing a connection; the actualdb.$connect()occurs inconnectDb()which runs after route registration completes. Since all database operations within route handlers are async and execute only after the server starts listening, the connection is guaranteed to be established before any database calls occur.Likely an incorrect or invalid review comment.
apps/server/src/routes/stats.ts (3)
1-11: LGTM! Clear documentation.The module documentation clearly describes the implemented best practices including Redis pipelining, TTL-based streak expiration, and deduplication.
30-35: LGTM! Good validation schema.The
SessionPayloadSchemaproperly constrainssessionDurationto reasonable bounds (0-86400 seconds) preventing abuse.
37-46: LGTM! Correct week start calculation.The logic properly handles Sunday (day 0) by adjusting to the previous Monday, and correctly sets time to midnight UTC.
apps/server/src/routes/webhooks.ts (2)
83-100: LGTM! Correct HMAC-SHA256 signature verification.The implementation correctly uses constant-time comparison via
crypto.timingSafeEqualand properly checks buffer lengths before comparison to prevent timing attacks.
321-347: LGTM! Push event handler is straightforward.Correctly increments commit count in the weekly leaderboard and handles edge cases (zero commits, unknown users).
apps/extension/src/views/leaderboardProvider.ts (4)
14-41: LGTM! Well-structured tree item with rich tooltips.The
LeaderboardTreeItemclass properly handles medal display, score formatting, and provides a useful tooltip. The command binding for profile viewing is correctly implemented.
93-109: LGTM! Proper TreeDataProvider implementation.The class correctly implements
TreeDataProviderandDisposable, with proper disposal of the event emitter.
115-148: LGTM! Clear state handling in getChildren.The method properly handles loading, empty, header children, and root level states with appropriate UI feedback items.
180-184: LGTM! Proper disposal pattern.The dispose method correctly iterates through and disposes all tracked disposables.
| } catch (error) { | ||
| logger.error({ error, event, deliveryId }, 'Error processing GitHub webhook'); | ||
| // Still return 200 to avoid GitHub retries for app errors | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Silently swallowing errors may mask systemic issues.
Returning 200 on application errors prevents GitHub retries, but also hides bugs. Consider logging at error level (already done) and tracking error rates via metrics for alerting.
…IDs, fix log count
…ion for webhook secret
…milestone logic, add language allowlist
…duplicate checking, rate limiting
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/extension/src/extension.ts (1)
210-227: Bug: stats/leaderboard polling isn’t tied to auth state changes (login/logout).
Right now it starts only duringinitializeConnection()and is never stopped/cleared on logout via the auth listener.Proposed fix
this.authService.onAuthStateChange((isAuthenticated) => { @@ if (isAuthenticated) { this.wsClient.connect(); this.activityTracker.start(); + // Phase 2: start polling once authenticated + this.startStatsRefresh(); } else { this.wsClient.disconnect(); this.activityTracker.stop(); + // Phase 2: stop polling + clear views on logout + this.stopStatsRefresh(); } this.friendsProvider.refresh(); void this.statusBar.update(); });Also applies to: 271-287, 289-310
🤖 Fix all issues with AI agents
In @apps/extension/src/extension.ts:
- Around line 326-334: When handling non-OK responses and exceptions around the
stats fetch, you must clear the provider loading state so the UI spinner stops;
after the else branch and inside the catch replace/augment the logger.warn calls
with a call to clear loading on the stats provider (e.g., call
statsProvider.setLoading(false) or statsProvider.setIsLoading(false) depending
on your API) before returning, and apply the same fix to the other fetch block
referenced at 351-361 (the other provider that mirrors
statsProvider.updateStats) so both providers always have their isLoading flag
reset on error.
- Around line 312-362: fetchStats() and fetchLeaderboard() lack request timeouts
so stalled fetches can hang; wrap each fetch call with an AbortController, set a
10s timeout that calls controller.abort(), pass controller.signal to fetch, and
clear the timeout after the fetch completes (in finally) to avoid leaks; update
the try/catch to handle abort errors via the existing logger and ensure the
controller/timeout logic is implemented inside both fetchStats and
fetchLeaderboard around the fetch(...) invocation.
In @apps/extension/src/views/statsProvider.ts:
- Around line 13-19: StatsData duplicates the API DTO and risks drifting;
replace the local interface by importing and reusing the shared UserStatsDTO (or
the exact exported type representing the API response) instead of declaring
StatsData, update any references to StatsData (including the other duplicate at
the bottom of the file) to use UserStatsDTO, and ensure exported function
signatures/types (e.g., any getStats/fetchStats or functions that return
StatsData) reflect the imported DTO so the file stays in sync with the API
contract.
- Around line 143-149: The setLoading method currently only fires
onDidChangeTreeDataEmitter when loading is true, leaving the tree stuck on
"Loading…" if setLoading(false) is called; update setLoading(loading: boolean)
in the class that owns isLoading and onDidChangeTreeDataEmitter to always call
this.onDidChangeTreeDataEmitter.fire(undefined) after updating this.isLoading so
the view refreshes on both start and end of loading.
In @apps/server/src/config.ts:
- Around line 36-37: The GITHUB_WEBHOOK_SECRET schema is too permissive; change
the Zod validator for GITHUB_WEBHOOK_SECRET to trim whitespace and require a
stronger minimum length (e.g., z.string().trim().min(32).optional() or
z.string().trim().min(64).optional() depending on your secret policy) so
accidental leading/trailing spaces are removed and weak short secrets are
rejected; update the GITHUB_WEBHOOK_SECRET definition (the symbol
GITHUB_WEBHOOK_SECRET in the config schema) accordingly and adjust any related
docs/tests that assume the old length.
In @apps/server/src/routes/stats.ts:
- Around line 212-225: The streak-parsing logic duplicated in the stats route
should be extracted into a shared helper (e.g., parseStreakData) that accepts
the Redis hgetall result (Record<string,string>|null) and returns a StreakInfo;
implement parseStreakData to parse count and longest with parseInt (default
'0'), derive lastActiveDate, compute streakStatus via
calculateStreakStatus(lastActiveDate), and set isActiveToday as streakStatus ===
'active', then replace the inline parsing in the GET /me handler and in the code
that reads REDIS_KEYS.streakData(userId) to call parseStreakData instead.
- Around line 400-420: The /achievements route currently returns all records for
a user; add paginated query support by parsing request.query with a schema
(e.g., AchievementsQuerySchema with limit:number default 50 and optional
cursor:string), then change the db.achievement.findMany call in the route
handler to use take: query.limit + 1, optionally pass cursor:{ id: query.cursor
} and skip:1 when cursor is present, orderBy earnedAt desc, slice results to
query.limit, compute hasMore and nextCursor (use the last returned achievement
id when hasMore), and include a pagination object (hasMore and nextCursor)
alongside the mapped achievements in reply.send.
- Around line 318-323: The inline comment numbering is inconsistent around the
network activity/ language tracking block — it jumps from "3." to "5."; update
the comment sequence to be consecutive (e.g., change "5. Track language..." to
"4. Track language...") so comments reflect the actual step order near the
pipeline.hincrby(REDIS_KEYS.networkIntensity(minute), 'count', 1) and
pipeline.expire(REDIS_KEYS.networkIntensity(minute),
NETWORK_ACTIVITY_TTL_SECONDS) lines and the subsequent language-tracking logic.
- Around line 29-33: The SessionPayloadSchema currently allows unbounded
optional strings for language and project; add sensible max-length validations
(e.g., z.string().max(255)) to those fields so they cannot exceed expected size
when logged or stored: update the SessionPayloadSchema definition to replace
language: z.string().optional() and project: z.string().optional() with
length-constrained variants (keep them optional) and choose an appropriate max
(e.g., 255 or another project-specific limit).
- Around line 254-292: The Lua script STREAK_UPDATE_SCRIPT relies on os.time()
which uses the Redis server's local timezone and can miscompute daysDiff;
instead avoid server-local time math by passing a UTC "yesterday" string from
the application and do string comparisons: keep ARGV[1] as today, add ARGV[3] as
yesterday (computed by getTodayDate() - 1 day in UTC in the app), then replace
the os.time/date parsing block with a simple check: if lastDate == today return;
elseif lastDate == yesterday then newStreak = count + 1 else newStreak = 1;
update the script to read ARGV[3] and adjust any callers that load
STREAK_UPDATE_SCRIPT to pass the new yesterday argument.
In @apps/server/src/routes/webhooks.ts:
- Around line 24-27: The code currently re-serializes the request body for
signature verification which can produce different bytes; update the webhook
handlers to fail fast if RawBodyRequest.rawBody is undefined instead of
JSON.stringify fallback: in any signature verification logic (where rawBody is
inspected — referenced around lines 112-125) return an error/HTTP 400 or throw a
configured server-misconfiguration error when rawBody is missing and use the
existing Buffer in RawBodyRequest.rawBody for verification only; remove any
re-serialization code paths and ensure the route handler signature/types require
RawBodyRequest so callers/consumers know rawBody must be present.
- Around line 234-268: The read-then-create for achievements (using
db.achievement.findFirst followed by db.achievement.create) can race and cause
Prisma P2002 errors; replace the pattern with a single atomic operation: either
use db.achievement.upsert keyed on userId+type+metadata.issueNumber (or the
achievement unique constraint) with the same data from
ACHIEVEMENTS.ISSUE_CLOSED, or keep create but catch
Prisma.PrismaClientKnownRequestError and ignore P2002 (unique constraint)
errors; apply the same change for the PR_MERGED branch so concurrent webhook
deliveries don't produce noisy unique-violation errors.
- Around line 41-53: The GitHub push payload schema is currently parsing email
fields and other unnecessary PII; update GitHubPushPayloadSchema to only parse
what's needed (commits array limited to a minimal shape such as just id so you
can read commits.length, and pusher/email removed) and keep only sender.id
(remove sender.login and any email fields). Apply the same change to any other
push-payload parsing in this file (the other GitHub push schema/validator
instance) so no email or extra user fields are included in memory or logs.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
apps/extension/src/extension.tsapps/extension/src/views/leaderboardProvider.tsapps/extension/src/views/statsProvider.tsapps/server/src/config.tsapps/server/src/routes/stats.tsapps/server/src/routes/webhooks.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/extension/src/views/statsProvider.ts (2)
packages/shared/src/types.ts (3)
StreakInfo(177-188)WeeklyStatsDTO(191-206)AchievementDTO(164-171)apps/server/src/lib/logger.ts (1)
logger(68-68)
apps/server/src/routes/stats.ts (5)
apps/server/src/services/db.ts (1)
getDb(20-33)apps/server/src/services/redis.ts (1)
getRedis(60-63)packages/shared/src/constants.ts (5)
REDIS_KEYS(35-55)STREAK_TTL_SECONDS(15-15)SESSION_TTL_SECONDS(18-18)NETWORK_ACTIVITY_TTL_SECONDS(21-21)ACHIEVEMENTS(95-128)apps/server/src/lib/logger.ts (1)
logger(68-68)apps/server/src/ws/handler.ts (1)
broadcastToUsers(405-412)
apps/extension/src/extension.ts (2)
apps/extension/src/views/index.ts (2)
StatsProvider(5-5)LeaderboardProvider(6-6)packages/shared/src/types.ts (3)
UserStatsDTO(244-252)LeaderboardEntry(209-220)AchievementPayload(235-241)
🪛 GitHub Check: CodeQL
apps/server/src/routes/webhooks.ts
[failure] 112-203: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
🔇 Additional comments (9)
apps/server/src/routes/stats.ts (5)
35-74: LGTM!Good defensive measure using an allowlist to prevent unbounded Redis hash growth from arbitrary language strings. The fallback to
'other'for unknown languages is appropriate.
76-91: LGTM!Both helper functions correctly handle UTC dates. The week start calculation properly adjusts for Sunday (day 0) to get the previous Monday.
93-104: LGTM!The streak status logic correctly distinguishes between active (today), at-risk (yesterday), and broken (2+ days ago) states.
130-134: Consider whether to return partial data on Redis errors.Pipeline errors are logged but the request continues with potentially null/stale data. While this "degraded response" approach is reasonable for a stats endpoint, ensure this behavior is intentional and documented for consumers.
361-395: LGTM!Good use of
Promise.allfor parallel Redis operations. The fallback chain fortotalSecondscorrectly prioritizes live Redis data over stored DB values.apps/server/src/routes/webhooks.ts (2)
92-103: No action needed—the comment is correct. The route is registered with a/webhooksprefix inserver.ts:213(app.register(webhookRoutes, { prefix: '/webhooks' })), so the'/github'route becomesPOST /webhooks/githubas documented in the JSDoc comment.Likely an incorrect or invalid review comment.
100-111: Rate limiting is properly enforced. The@fastify/rate-limitplugin is registered in server.ts (lines 65–80) with a functional keyGenerator and global defaults, and the route-levelconfig.rateLimitoverride in webhooks.ts correctly applies per-route limits. No configuration disables or bypasses rate limiting.Likely an incorrect or invalid review comment.
apps/extension/src/extension.ts (1)
75-88: No action required —ConfigManagerproperly implementsvscode.Disposablewith adispose()method.The
ConfigManagerclass explicitly declaresimplements vscode.Disposable(line 55 ofconfigManager.ts) and includes a completedispose()implementation (line 129), so adding it to the disposables array is correct and will not throw at runtime.Likely an incorrect or invalid review comment.
apps/extension/src/views/statsProvider.ts (1)
65-79: ThestreakStatus === 'at_risk'check is already correct and matches the sharedStreakStatustype definition ('active' | 'at_risk' | 'broken'). No hardening needed here.For
weeklyStats.rank(lines 110-113): sincerankis defined asrank?: number | undefinedwith the semantic "1-indexed" (per shared type docs), an explicit type check is preferable for defensive coding:const rankDisplay = this.stats.weeklyStats.rank ? `#${String(this.stats.weeklyStats.rank)}` : '';Consider:
const rank = this.stats.weeklyStats.rank; const rankDisplay = typeof rank === 'number' ? `#${String(rank)}` : '';Likely an incorrect or invalid review comment.
…d server Extension: - Add AbortController with 10s timeout to fetchStats/fetchLeaderboard - Clear provider loading state on fetch errors to avoid stuck spinners - Fix XSS vulnerability in leaderboard tooltip (isTrusted=false, appendText) - Fix setLoading() to always fire tree refresh in both providers - Replace local StatsData interface with UserStatsDTO from shared package Server: - Strengthen GITHUB_WEBHOOK_SECRET validation (trim + min 32 chars) - Add max-length validation (255) to session language/project fields - Extract parseStreakData helper to deduplicate streak parsing logic - Fix Lua streak script timezone issue with UTC yesterday comparison - Fix comment numbering consistency in stats route - Add cursor-based pagination to /achievements endpoint - Fix streak achievements to award all applicable milestones (not just highest) - Fail fast on missing rawBody in webhooks (no re-serialization fallback) - Minimize GitHub push payload schema (remove PII: email, author, pusher) - Harden signature verification (sha256= prefix, getHeader helper, utf8) - Add delivery-id deduplication to prevent duplicate webhook processing - Fix achievement race condition with P2002 error handling Shared: - Add webhookDelivery Redis key for deduplication
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @apps/extension/src/views/leaderboardProvider.ts:
- Line 166: Extract the magic number 10 used in the leaderboard slice into a
named constant (e.g., MAX_LEADERBOARD_ENTRIES) at the module or class level,
then replace the hardcoded slice call this.leaderboard = entries.slice(0, 10)
with this.leaderboard = entries.slice(0, MAX_LEADERBOARD_ENTRIES) so the top-N
limit is configurable and self-documenting; ensure the constant is exported or
placed near the LeaderboardProvider class for clarity.
- Around line 16-24: The nested ternary that computes medal (using showMedal and
entry.rank) is hard to read; extract this into a small helper like
getMedal(rank: number): string and replace the ternary with medal = showMedal ?
this.getMedal(entry.rank) : ''. Implement getMedal to return '🥇 ' for 1, '🥈 '
for 2, '🥉 ' for 3, and `#${String(rank)} ` otherwise, and call it from the
scope where medal is assigned (referencing entry.rank and showMedal).
In @apps/server/src/routes/webhooks.ts:
- Line 286: Remove unnecessary String() calls inside template literals: change
occurrences like `Closed issue #${String(issue.number)} in
${repository.full_name}` (and the similar usage around line 380) to use direct
interpolation `#${issue.number}`; update any other template literals in this
file that wrap numeric or non-string values with String() to rely on implicit
coercion (e.g., `#${issue.number}`, `${someNumericVar}`), keeping the rest of
the template content unchanged.
In @packages/shared/src/constants.ts:
- Around line 94-134: The ACHIEVEMENTS export includes three time-based awards
(FIRST_HOUR, NIGHT_OWL, EARLY_BIRD) that are not implemented and thus create API
inconsistency; either remove these keys from the ACHIEVEMENTS object and the
derived AchievementTypeKey, or implement backend checks to award them (add logic
alongside checkStreakAchievements in stats.ts or a new function invoked where
achievements are evaluated, e.g., the same flow that awards
ISSUE_CLOSED/PR_MERGED in webhooks.ts) so the constants match actual behavior.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
apps/extension/src/extension.tsapps/extension/src/views/index.tsapps/extension/src/views/leaderboardProvider.tsapps/extension/src/views/statsProvider.tsapps/server/src/config.tsapps/server/src/routes/stats.tsapps/server/src/routes/webhooks.tspackages/shared/src/constants.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/server/src/routes/stats.ts (6)
packages/shared/src/types.ts (3)
StreakInfo(177-188)WeeklyStatsDTO(191-206)AchievementDTO(164-171)apps/server/src/services/db.ts (1)
getDb(20-33)apps/server/src/services/redis.ts (1)
getRedis(60-63)packages/shared/src/constants.ts (4)
REDIS_KEYS(35-58)STREAK_TTL_SECONDS(15-15)SESSION_TTL_SECONDS(18-18)NETWORK_ACTIVITY_TTL_SECONDS(21-21)apps/server/src/lib/logger.ts (1)
logger(68-68)apps/server/src/ws/handler.ts (1)
broadcastToUsers(405-412)
apps/extension/src/views/statsProvider.ts (3)
apps/extension/src/views/index.ts (1)
StatsProvider(5-5)packages/shared/src/types.ts (1)
UserStatsDTO(244-252)apps/server/src/lib/logger.ts (1)
logger(68-68)
apps/server/src/routes/webhooks.ts (6)
apps/server/src/services/db.ts (1)
getDb(20-33)apps/server/src/lib/logger.ts (1)
logger(68-68)apps/server/src/config.ts (1)
env(61-61)apps/server/src/services/redis.ts (1)
getRedis(60-63)packages/shared/src/constants.ts (2)
REDIS_KEYS(35-58)ACHIEVEMENTS(98-131)apps/server/src/ws/handler.ts (1)
broadcastToUsers(405-412)
🪛 GitHub Check: CodeQL
apps/server/src/routes/webhooks.ts
[failure] 131-229: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
🔇 Additional comments (25)
apps/server/src/config.ts (1)
36-41: LGTM! Webhook secret validation is secure.The optional
GITHUB_WEBHOOK_SECRETconfiguration is correctly validated with appropriate security constraints (minimum 32 characters, trimmed). The validation chain properly handles the optional nature while ensuring that if provided, the secret meets security requirements.apps/extension/src/views/index.ts (1)
5-6: LGTM! Clean re-export pattern.The new provider exports follow the existing pattern and properly expose the public API for the gamification views.
apps/extension/src/views/statsProvider.ts (3)
46-125: Well-structured stats display logic.The
getChildrenmethod effectively handles all edge cases (loading, no data, partial data) and provides clear visual feedback with appropriate icons and emojis. The streak status logic and conditional displays for best streak and top language are well implemented.
127-151: LGTM! Clean state management methods.The public API methods (
updateStats,setLoading,clear,refresh) provide clear interfaces for updating the view state and properly trigger tree refresh events.
153-157: LGTM! Proper resource cleanup.The
disposemethod correctly cleans up all registered disposables, following VS Code extension best practices.apps/server/src/routes/stats.ts (10)
28-39: LGTM! Well-designed validation schemas.The Zod schemas provide appropriate constraints:
- Session duration capped at 24 hours prevents invalid data
- String length limits prevent unbounded growth
- Pagination limits are sensible
41-80: Excellent security practice with language allowlist.The allowlist prevents unbounded Redis hash growth from arbitrary language strings. The comprehensive set covers major languages and frameworks, with a safe 'other' fallback at line 327 for unmapped languages.
82-118: LGTM! Timezone-safe date utility functions.All utility functions correctly handle UTC timezone conversions:
getWeekStart()properly calculates Monday for all days of the weekgetTodayDate()andgetYesterdayDate()safely extract date stringscalculateStreakStatus()accurately determines streak state based on last activity
120-134: LGTM! Safe parsing with proper defaults.The
parseStreakDatafunction safely handles potentially missing Redis data with appropriate defaults and null coalescing.
224-236: LGTM! Simple and correct streak retrieval.The endpoint cleanly retrieves and parses streak data using the shared helper function.
242-360: Excellent atomic session recording with robust safeguards.The endpoint demonstrates several best practices:
- Atomic streak updates: The Lua script ensures streak progression is atomic and prevents double-counting via same-day deduplication (line 274-276)
- Timezone-safe date handling: String-based date comparison in Lua (lines 274, 280) works correctly with UTC YYYY-MM-DD format
- Unbounded growth prevention: Language allowlist (line 327) with 'other' fallback protects against Redis hash bloat
- Appropriate TTLs: 50-hour streak TTL (line 301) provides generous grace period for timezone variations
- Non-blocking achievements: Fire-and-forget pattern (line 348) ensures achievement processing doesn't impact session recording latency
365-396: LGTM! Efficient weekly stats with real-time data.The endpoint effectively merges PostgreSQL base data with Redis real-time scores using
Promise.allfor parallel fetching.
401-446: LGTM! Correct cursor-based pagination.The pagination implementation properly uses the fetch-one-extra pattern to determine
hasMoreand correctly handles cursor-based navigation.
451-508: Achievement backfilling logic is sound.The function correctly checks all milestone thresholds and backfills missing achievements (e.g., a user reaching 100-day streak will get STREAK_7, STREAK_30, and STREAK_100 if they don't already have them). The broadcast to followers is appropriately scoped to achievement events.
Minor edge case: If a user is deleted between achievement creation (line 469) and user query (line 479), an orphaned achievement may be created. This is acceptable given the low probability and non-critical nature.
143-219: Achievement type assertions are safe due to proper synchronization, no changes needed.The types between Prisma schema and
AchievementDTOare properly aligned. TheAchievementTypeunion includes all values defined in theACHIEVEMENTSconstant (ISSUE_CLOSED, PR_MERGED, STREAK_7, STREAK_30, STREAK_100, FIRST_HOUR, NIGHT_OWL, EARLY_BIRD), and achievements are only created via theACHIEVEMENTS[type]pattern, which validates the type exists. The type assertion at line 204 is safe and the schema remains synchronized.packages/shared/src/constants.ts (2)
10-24: LGTM! Well-reasoned TTL values for gamification.The TTL constants are appropriately sized:
- 25-hour streak TTL provides timezone flexibility
- 48-hour session TTL enables day-overlap queries
- 2-minute network activity supports real-time heatmaps
41-43: Verify if both streak key formats are needed.Two streak-related Redis key generators are defined in lines 42-43:
userStreak: (userId: string) => 'streak:${userId}'streakData: (userId: string) => 'streak:data:${userId}'Confirm whether
userStreakis still in use elsewhere in the codebase, or if it's legacy code that can be removed. IfstreakDatais the replacement format for Phase 2, the olderuserStreakkey may be redundant.apps/extension/src/extension.ts (3)
314-348: LGTM! Proper timeout and error handling.Both
fetchStatsandfetchLeaderboardmethods correctly implement:
- AbortController with 10-second timeout
- Proper cleanup in finally block
- Graceful degradation on errors
- Appropriate logging
The timeout handling prevents indefinite hangs, and the error handling ensures the loading state is cleared even on failures.
Also applies to: 352-390
577-592: Well-structured achievement handling with user-specific logic.The updated
handleAchievementmethod correctly:
- Validates the payload structure
- Determines if the achievement belongs to the current user
- Customizes notification messaging
- Triggers a stats refresh for the current user's achievements
This provides a good user experience by immediately reflecting new achievements in the stats view.
289-300: Potential issue: multiple intervals could be created.The nullish coalescing assignment (
??=) on line 296 prevents creating a new interval only ifstatsRefreshIntervalis truthy. However, ifstartStatsRefresh()is called multiple times whenstatsRefreshIntervalis already set, the old interval won't be cleared, potentially leading to multiple concurrent refresh loops.Consider adding an explicit check and clearing any existing interval before creating a new one.
🔧 Proposed fix
private startStatsRefresh(): void { + // Clear any existing interval first + if (this.statsRefreshInterval) { + clearInterval(this.statsRefreshInterval); + } + // Initial fetch void this.fetchStats(); void this.fetchLeaderboard(); // Refresh every 60 seconds - this.statsRefreshInterval ??= setInterval(() => { + this.statsRefreshInterval = setInterval(() => { void this.fetchStats(); void this.fetchLeaderboard(); }, 60_000); }Likely an incorrect or invalid review comment.
apps/server/src/routes/webhooks.ts (5)
80-100: Excellent signature verification implementation.The signature verification correctly implements:
- HMAC-SHA256 with the secret
- Constant-time comparison using
crypto.timingSafeEqualto prevent timing attacks- Explicit UTF-8 encoding for buffer comparison
- Length validation before comparison
This is a security best practice for webhook signature verification.
122-128: Static analysis false positive - rate limiting is properly configured.The CodeQL warning about missing rate limiting is incorrect. The route handler has rate limiting configured with:
- Maximum 100 requests per minute
- Applied via the
rateLimitconfig optionThis is appropriate for webhook endpoints that may receive bursts of legitimate traffic from GitHub.
260-275: Proper idempotency checks before achievement creation.Both
handleIssuesEventandhandlePullRequestEventcorrectly query for existing achievements using specific metadata fields (issueNumber/prNumber) before attempting to create new ones. This reduces the likelihood of race conditions and unnecessary database writes.Combined with the P2002 error handling in the try-catch blocks, this provides robust protection against duplicate achievements.
Also applies to: 354-369
280-325: Well-implemented race condition handling.The achievement creation logic properly handles race conditions by:
- Checking for existing achievements first (lines 260-275, 354-369)
- Wrapping creation in try-catch
- Using the
isPrismaUniqueConstraintErrorguard to detect P2002 violations- Gracefully returning when duplicates are detected
This ensures idempotent webhook processing even under concurrent webhook deliveries.
Also applies to: 374-419
165-174: No issue found—the code correctly uses ioredis's return value semantics.The deduplication logic is correct. The project uses
ioredis, which returns"OK"whenSETwithNXsucceeds (key did not exist) andnullwhen it fails (key already exists). The checkif (setResult !== 'OK')properly detects duplicate deliveries.
| const medal = showMedal | ||
| ? entry.rank === 1 | ||
| ? '🥇 ' | ||
| : entry.rank === 2 | ||
| ? '🥈 ' | ||
| : entry.rank === 3 | ||
| ? '🥉 ' | ||
| : `#${String(entry.rank)} ` | ||
| : ''; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting medal logic for improved readability.
The nested ternary operators for medal assignment are functional but could be more maintainable as a helper function or a simple if-else chain.
♻️ Optional refactor
- const medal = showMedal
- ? entry.rank === 1
- ? '🥇 '
- : entry.rank === 2
- ? '🥈 '
- : entry.rank === 3
- ? '🥉 '
- : `#${String(entry.rank)} `
- : '';
+ const medal = showMedal ? this.getMedal(entry.rank) : '';Add a helper method:
private getMedal(rank: number): string {
if (rank === 1) return '🥇 ';
if (rank === 2) return '🥈 ';
if (rank === 3) return '🥉 ';
return `#${String(rank)} `;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/extension/src/views/leaderboardProvider.ts around lines 16 - 24, The
nested ternary that computes medal (using showMedal and entry.rank) is hard to
read; extract this into a small helper like getMedal(rank: number): string and
replace the ternary with medal = showMedal ? this.getMedal(entry.rank) : ''.
Implement getMedal to return '🥇 ' for 1, '🥈 ' for 2, '🥉 ' for 3, and
`#${String(rank)} ` otherwise, and call it from the scope where medal is
assigned (referencing entry.rank and showMedal).
|
|
||
| /** Updates the leaderboard data. */ | ||
| updateLeaderboard(entries: LeaderboardEntry[], myRank: number | null): void { | ||
| this.leaderboard = entries.slice(0, 10); // Top 10 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract magic number to a constant.
The top-10 limit is hardcoded. Consider extracting it to a named constant at the class or module level for better maintainability.
♻️ Proposed refactor
At the top of the file or class:
const MAX_LEADERBOARD_ENTRIES = 10;Then update line 166:
- this.leaderboard = entries.slice(0, 10); // Top 10
+ this.leaderboard = entries.slice(0, MAX_LEADERBOARD_ENTRIES);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/extension/src/views/leaderboardProvider.ts at line 166, Extract the
magic number 10 used in the leaderboard slice into a named constant (e.g.,
MAX_LEADERBOARD_ENTRIES) at the module or class level, then replace the
hardcoded slice call this.leaderboard = entries.slice(0, 10) with
this.leaderboard = entries.slice(0, MAX_LEADERBOARD_ENTRIES) so the top-N limit
is configurable and self-documenting; ensure the constant is exported or placed
near the LeaderboardProvider class for clarity.
| userId: user.id, | ||
| type: 'ISSUE_CLOSED', | ||
| title: achievementDef.title, | ||
| description: `Closed issue #${String(issue.number)} in ${repository.full_name}`, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Unnecessary String() conversions.
Using String() to convert numbers to strings in template literals is defensive but unnecessary, as JavaScript automatically coerces numbers to strings in template contexts.
♻️ Optional simplification
- description: `Closed issue #${String(issue.number)} in ${repository.full_name}`,
+ description: `Closed issue #${issue.number} in ${repository.full_name}`,Similarly for line 380:
- description: `Merged PR #${String(pull_request.number)} in ${repository.full_name}`,
+ description: `Merged PR #${pull_request.number} in ${repository.full_name}`,Also applies to: 380-380
🤖 Prompt for AI Agents
In @apps/server/src/routes/webhooks.ts at line 286, Remove unnecessary String()
calls inside template literals: change occurrences like `Closed issue
#${String(issue.number)} in ${repository.full_name}` (and the similar usage
around line 380) to use direct interpolation `#${issue.number}`; update any
other template literals in this file that wrap numeric or non-string values with
String() to rely on implicit coercion (e.g., `#${issue.number}`,
`${someNumericVar}`), keeping the rest of the template content unchanged.
| /** | ||
| * Achievement definitions with titles and descriptions. | ||
| * Used for creating achievements and displaying in UI. | ||
| */ | ||
| export const ACHIEVEMENTS = { | ||
| ISSUE_CLOSED: { | ||
| title: '🐛 Bug Slayer', | ||
| description: 'Closed an issue on GitHub', | ||
| }, | ||
| PR_MERGED: { | ||
| title: '🎉 Merge Master', | ||
| description: 'Merged a pull request', | ||
| }, | ||
| STREAK_7: { | ||
| title: '🔥 Week Warrior', | ||
| description: 'Maintained a 7-day coding streak', | ||
| }, | ||
| STREAK_30: { | ||
| title: '⚡ Monthly Machine', | ||
| description: 'Maintained a 30-day coding streak', | ||
| }, | ||
| STREAK_100: { | ||
| title: '🏆 Century Coder', | ||
| description: 'Maintained a 100-day coding streak', | ||
| }, | ||
| FIRST_HOUR: { | ||
| title: '⏰ First Hour', | ||
| description: 'Coded for your first full hour', | ||
| }, | ||
| NIGHT_OWL: { | ||
| title: '🦉 Night Owl', | ||
| description: 'Coded past midnight', | ||
| }, | ||
| EARLY_BIRD: { | ||
| title: '🌅 Early Bird', | ||
| description: 'Started coding before 6 AM', | ||
| }, | ||
| } as const; | ||
|
|
||
| /** Achievement type from the ACHIEVEMENTS keys. */ | ||
| export type AchievementTypeKey = keyof typeof ACHIEVEMENTS; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for time-based achievement implementations
echo "=== Checking FIRST_HOUR implementation ==="
rg "FIRST_HOUR" --type ts -C 3
echo -e "\n=== Checking NIGHT_OWL implementation ==="
rg "NIGHT_OWL" --type ts -C 3
echo -e "\n=== Checking EARLY_BIRD implementation ==="
rg "EARLY_BIRD" --type ts -C 3
echo -e "\n=== Checking all achievement award locations ==="
rg "awardAchievement|createAchievement|Achievement.*award" --type ts -C 2Repository: senutpal/devRadar
Length of output: 3204
🏁 Script executed:
#!/bin/bash
# Search for achievement creation/award logic in server
echo "=== Checking stats.ts for all achievement checks ==="
rg "checkStreakAchievements|ACHIEVEMENT" apps/server/src/services/stats.ts -C 2
echo -e "\n=== Searching for achievement creation in server ==="
rg "achievement" apps/server/src -i --type ts -l | head -20
echo -e "\n=== Checking for time-based achievement checks ==="
rg "FIRST_HOUR|NIGHT_OWL|EARLY_BIRD|midnight|6 AM|hour" apps/server/src --type ts -C 3
echo -e "\n=== Checking webhooks for achievement logic ==="
rg "award|achievement|ACHIEVEMENT" apps/server/src/routes/webhooks.ts --type ts -C 2Repository: senutpal/devRadar
Length of output: 4286
🏁 Script executed:
#!/bin/bash
# Find where streak achievements are checked
echo "=== Searching for streak achievement implementation ==="
rg "STREAK_7|STREAK_30|STREAK_100|checkStreak" apps/server/src --type ts -C 3
echo -e "\n=== Check all locations where achievements are created ==="
rg "achievement.*create\|db\.achievement\.create" apps/server/src --type ts -A 1
echo -e "\n=== List all routes/service files ==="
find apps/server/src -name "*.ts" -type f | grep -E "(routes|services)" | head -20Repository: senutpal/devRadar
Length of output: 1914
🏁 Script executed:
#!/bin/bash
# Get the full context of streak achievement implementation
echo "=== Full checkStreakAchievements function ==="
rg -A 50 "async function checkStreakAchievements" apps/server/src/routes/stats.tsRepository: senutpal/devRadar
Length of output: 1834
Remove or implement time-based achievements.
The ACHIEVEMENTS constant defines three achievements that have no implementation:
- ✅ ISSUE_CLOSED, PR_MERGED: Implemented in webhooks.ts
- ✅ STREAK_7, STREAK_30, STREAK_100: Implemented in stats.ts via
checkStreakAchievements - ❌ FIRST_HOUR, NIGHT_OWL, EARLY_BIRD: Defined but not implemented anywhere
These three time-based achievements are exported in constants and included in the type definitions but have no backend logic to award them. Either implement the achievement checks or remove them from the constants to avoid API inconsistency.
🤖 Prompt for AI Agents
In @packages/shared/src/constants.ts around lines 94 - 134, The ACHIEVEMENTS
export includes three time-based awards (FIRST_HOUR, NIGHT_OWL, EARLY_BIRD) that
are not implemented and thus create API inconsistency; either remove these keys
from the ACHIEVEMENTS object and the derived AchievementTypeKey, or implement
backend checks to award them (add logic alongside checkStreakAchievements in
stats.ts or a new function invoked where achievements are evaluated, e.g., the
same flow that awards ISSUE_CLOSED/PR_MERGED in webhooks.ts) so the constants
match actual behavior.
1 participant
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.