Feat/backend implementation phase 1.2 #12
Conversation
- Add Fastify 5 HTTP server with CORS, Helmet, JWT, rate limiting - Integrate Prisma 7 with PostgreSQL adapter and ESM support - Add Redis service with separate pub/sub clients for presence - Implement WebSocket handler with JWT auth and heartbeat protocol - Create GitHub OAuth authentication flow - Add REST routes: /auth, /users, /friends with full CRUD - Set up structured Pino logging with sensitive data redaction - Add hierarchical error system distinguishing operational errors - Configure graceful shutdown with connection cleanup
- Add CSRF protection, token blacklisting, and rate limiting - Optimize Redis presence updates and mutual friends query - Fix Prisma 7 schema config and add generate step to CI - Resolve ESLint errors and add proper type validations
- Add DATABASE_URL and SHADOW_DATABASE_URL placeholders for CI - Fix unnecessary type assertion in auth.ts token decoding - Prisma 7's config loader requires env vars even for generate
Changed dependsOn from "topo" to "^build" so that workspace packages like @devradar/shared are compiled before linting packages that depend on them.
Core Extension Files: - extension.ts: Main activation with command registration - authService.ts: GitHub OAuth flow with URI handler - activityTracker.ts: Activity detection with intelligent debouncing - wsClient.ts: WebSocket client with reconnection & message queue - friendsProvider.ts: TreeView for grouped friends list - statusBarItem.ts: Status bar integration Configuration & Commands: - Activation events (onStartupFinished) - Commands: login, logout, togglePrivacy, poke, refreshFriends - Configuration properties for server URL, privacy, blacklists - Sidebar views container with Friends and Activity views Assets & Media: - Extension icon (SVG) - Friends view icon - Activity view icon
![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.
📝 WalkthroughWalkthroughAdds a new DevRadar VS Code extension (auth, presence, activity, friends, UI) plus server-side OAuth, user/friend APIs, WebSocket presence handling, Prisma schema, shared message-type updates, and supporting dev/config files. Changes
Sequence Diagram(s)sequenceDiagram
actor User as VSCode User
participant Ext as Extension
participant Auth as AuthService
participant Browser as External Browser
participant Server as Auth Server
participant DB as Database
User->>Ext: Trigger "Login"
Ext->>Auth: login()
Auth->>Browser: open GitHub OAuth URL
User->>Browser: Authenticate
Browser->>Server: GET /callback?code=...&state=...
Server->>Server: validate state, exchange code
Server->>DB: upsert user
Server-->>Browser: redirect vscode://...?token=...
Browser->>Ext: URI handler with token
Ext->>Auth: handle callback (store token)
Auth->>Server: fetch user profile
Server-->>Auth: UserDTO
Auth->>Ext: emit auth state
Ext->>User: update UI (views/status)
sequenceDiagram
participant ExtA as Extension (Client A)
participant WS as WebSocket Server
participant Redis as Redis Pub/Sub
participant ExtB as Extension (Client B)
ExtA->>WS: Connect with JWT
WS->>WS: authenticate, load friends
WS->>Redis: subscribe to friend channels
WS-->>ExtA: send CONNECTED + FRIEND_STATUS
ExtA->>WS: send STATUS_UPDATE (activity)
WS->>WS: persist presence
WS->>Redis: publish to friend channels
Redis->>WS: route to subscribers
WS-->>ExtB: deliver FRIEND_STATUS
ExtB->>User: update friends view / activity feed
sequenceDiagram
participant Editor as VSCode Editor
participant Tracker as ActivityTracker
participant Config as ConfigManager
participant WSClient as WebSocketClient
participant Server as WebSocket Server
Editor->>Tracker: text change / focus / debug events
Tracker->>Tracker: debounce, compute intensity & session
Tracker->>Config: check privacy & blacklists
loop heartbeat interval
Tracker->>WSClient: sendStatusUpdate(status, activity)
WSClient->>Server: STATUS_UPDATE message
Server->>Server: update presence, publish
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
Fix all issues with AI Agents 🤖
In @apps/extension/.vscode/tasks.json:
- Around line 15-22: The "pnpm: dev" task lacks a problem matcher so TypeScript
errors won't surface in the VS Code Problems panel; update the task (label
"pnpm: dev") by adding the built-in "$tsc-watch" problem matcher to its
problemMatcher array (e.g., "problemMatcher": ["$tsc-watch"]) so background
TypeScript compilation errors are captured while "isBackground": true is set.
In @apps/extension/package.json:
- Around line 132-141: The configuration defaults for devradar.serverUrl and
devradar.wsUrl currently point to localhost; update documentation and
configuration to include production examples and guidance: add a short section
to the README describing recommended production URLs and environment-specific
overrides, and modify package.json comments or schema descriptions for
devradar.serverUrl and devradar.wsUrl to mention how to set them in production
(e.g., use env-substitution, CI/CD secrets, or workspace settings) so consumers
know the intended production values and how to override the localhost defaults.
- Around line 193-199: The default heartbeat interval
"devradar.heartbeatInterval" is set to 30000ms (30s); change the default to a
higher value (e.g., 45000 or 60000) if you want fewer heartbeats in production
and update the JSON schema "default" for that setting accordingly, and add or
update the "description" to document the trade-off between responsiveness and
server load (mention the new default and recommend tuning for production vs.
dev).
- Around line 1-239: The ESLint parser warning is caused by eslint.config.js
being interpreted as ESM while the extension build uses CommonJS; rename
eslint.config.js to eslint.config.cjs (or alternatively set "type": "module" in
package.json) to resolve the ambiguity—locate the ESLint config file
(eslint.config.js) and rename it to eslint.config.cjs so it matches the CommonJS
build (see the build script using --format=cjs in the "build" script) and re-run
the pipeline.
- Line 31: package.json references four media assets ("icon.png", "icon.svg",
"friends.svg", "activity.svg") but the media directory and files are missing;
either add the media directory and create those files (appropriate sizes/formats
for extension icons and SVGs) and commit them, or remove the corresponding keys
from package.json to avoid broken references; ensure the chosen approach is
consistent (if adding files, update .npmignore/packaging so media/ is included;
if removing, remove all four entries) and verify the extension loads without
missing asset errors.
- Around line 9-12: The package.json "repository" URL currently points to
"https://github.com/devradar/devradar"; update the "url" field under the
"repository" object to the correct canonical repo URL (e.g.,
"https://github.com/senutpal/devRadar") or the intended upstream fork URL to
reflect the actual source; locate the "repository" object in
apps/extension/package.json and replace the existing value for "url"
accordingly, and ensure the "type" remains "git".
In @apps/extension/README.md:
- Line 15: The README currently instructs users to "Install from the VS Code
Marketplace", but package.json sets "private": true which prevents publishing;
update either the package.json or the README: either remove or set "private" to
false in package.json to allow publishing to the Marketplace, or modify the
README installation text to reflect that the extension is not published (e.g.,
provide local install instructions or indicate development-only status). Ensure
references to "DevRadar" and the Marketplace link are adjusted accordingly so
documentation matches the package.json "private" setting.
- Around line 28-36: Update the settings table in README.md so it lists all 12
configuration keys with exact defaults from package.json: devradar.privacyMode
(false), devradar.showFileName (true), devradar.showProject (true),
devradar.showLanguage (true), devradar.showStatusBarItem (true),
devradar.enableNotifications (true), devradar.serverUrl
("https://devradar.example.com" — use the exact string in package.json),
devradar.wsUrl ("wss://devradar.example.com" — use the exact string in
package.json), devradar.blacklistedFiles ([".env", ".env.*", "*.pem", "*.key",
"*.secret", "**/secrets/**", "**/credentials/**"]),
devradar.blacklistedWorkspaces ([] or the exact array in package.json),
devradar.idleTimeout (300000), and devradar.heartbeatInterval (60000 or the
exact value in package.json); ensure each row shows the correct Default and a
brief Description matching the current package.json metadata.
In @apps/extension/src/extension.ts:
- Around line 274-298: The current handlePoke logic only checks for object shape
but not that userId is a string; add a runtime type guard (e.g.,
isFriendItem(item): item is { userId: string; username?: string }) that verifies
item is non-null object and typeof item.userId === 'string', then replace the
existing inline shape check in handlePoke to use isFriendItem and call
sendPoke(item.userId) only when the guard passes; keep the quick-pick branch for
non-friend items unchanged and ensure casts are removed in favor of the guard.
- Around line 304-312: In sendPoke, validate and sanitize the user input from
showInputBox before calling this.wsClient.sendPoke: trim the message, reject
empty or whitespace-only inputs, enforce a maximum length (e.g., 256–500 chars)
and truncate or reject messages that exceed it, strip or replace dangerous
control characters (newlines, non-printables) and any obvious HTML/JS snippets,
and show an appropriate vscode.window.showErrorMessage or a warning prompt when
the input is invalid; only call this.wsClient.sendPoke(userId, message) after
these checks pass.
In @apps/extension/src/services/activityTracker.ts:
- Around line 465-483: The dedupe logic in isDuplicateActivity currently only
compares fileName/language/project/workspace so heartbeat updates with
increasing sessionDuration/intensity are suppressed; update isDuplicateActivity
to also consider sessionDuration and intensity (e.g., compare
this.lastSentActivity.sessionDuration !== activity.sessionDuration ||
this.lastSentActivity.intensity !== activity.intensity) or alternately add a
parameter (e.g., allowHeartbeat: boolean) and bypass deduplication when
allowHeartbeat is true so heartbeats always send when sessionDuration/intensity
change while preserving dedupe for event-driven updates; adjust callers to pass
the heartbeat flag if you choose the latter.
In @apps/extension/src/services/wsClient.ts:
- Around line 107-117: The code currently appends the auth token to the
WebSocket URL (token and wsUrl) which exposes credentials; instead create the
WebSocket without the token (use wsUrl alone) and perform authentication after
the connection opens by sending the token in the first WebSocket message (e.g.
in the 'open' handler inside setupWebSocketHandlers or a new sendAuth message),
or for non-browser clients set an Authorization header during the upgrade if
supported; remove any code that builds a URL with ?token=..., avoid logging the
token, and ensure you still call setupWebSocketHandlers() and
scheduleReconnect() on errors while using wss:// and short-lived or scoped
socket tokens where applicable.
In @apps/extension/src/utils/configManager.ts:
- Around line 153-162: The matchGlob method poses a ReDoS risk by converting
user patterns into vulnerable regexes (notably replacing '**' with '.*');
replace the hand-rolled logic in matchGlob with a proven glob matcher (e.g., use
minimatch or picomatch to test text against pattern inside matchGlob) or, if
avoiding new deps, add strict validation in the config update path to reject
dangerous patterns (e.g., disallow nested/ambiguous wildcards) before they are
saved; update references to matchGlob so callers continue to work and ensure any
new dependency is added to package.json and imported where matchGlob is defined.
In @apps/extension/src/utils/logger.ts:
- Around line 63-70: The error method currently calls this.log(LogLevel.ERROR,
message, error) but the private log(...) implementation only writes "Error:
${message}" to the OutputChannel; update error(...) and/or log(...) so the
OutputChannel also receives the error stack when available (use error instanceof
Error ? error.stack : String(error) or a formatted message combining message and
stack) and ensure the same formatted output is written to the OutputChannel as
the console.error path; reference the error(message: string, error?: unknown)
method, the private log(...) method, and LogLevel.ERROR when making this change.
- Around line 27-37: The constructor currently mutates the global
Logger.minLevel on every new instance; move the environment-based default log
level logic out of the constructor and into a one-time static initialization
(e.g., a static initializer block or set the default at declaration) so
Logger.minLevel is initialized once based on process.env.NODE_ENV or
process.env.DEVRADAR_DEBUG and subsequent creations of Logger(...) do not
override it; ensure Logger.setLevel() still updates the static minLevel and that
the constructor only assigns this.prefix and creates the shared outputChannel.
- Around line 122-124: Add a static cleanup method to the logger class to
dispose the shared OutputChannel: implement Logger.disposeShared() (or
Logger.disposeSharedOutputChannel()) to check for the shared OutputChannel
instance and call its dispose() then null it; keep the instance dispose() as a
no-op. Call Logger.disposeShared() from the extension deactivate() function to
ensure the shared channel is released when the extension deactivates.
In @apps/extension/src/views/activityProvider.ts:
- Around line 195-215: createPokeActivity lacks runtime type checks for the poke
payload; validate that payload is a non-null object and that poke.fromUserId is
a string (reject/null if not), and ensure poke.fromUsername and poke.message are
either strings or undefined before using them; replace the current conditional
assignment with a single safe assignment that only sets event.username when
fromUserId is valid and fromUsername is a string, and use the validated
fromUserId when constructing id and userId to avoid runtime errors (mirror the
validation pattern used in createFriendStatusActivity for reference).
- Around line 163-190: The createFriendStatusActivity function needs stronger
runtime checks and a simpler username assignment: validate that payload.userId
and payload.status exist and are strings (e.g., check typeof (payload as
any).userId === 'string' and typeof (payload as any).status === 'string') and
only proceed if status is 'online' or 'offline'; compute username once with
const username = typeof (payload as any).username === 'string' ? (payload as
any).username : (payload as any).userId to simplify the conditional, and only
add event.username when the original username is a string; update references to
status/userId/username in the function accordingly (look for
createFriendStatusActivity, status, username, event).
In @apps/extension/src/views/friendsProvider.ts:
- Around line 227-235: The constructor currently accepts an unused _wsClient
parameter; either remove _wsClient from the constructor signature and all
instantiation sites (e.g., where FriendsProvider is constructed) to reduce API
surface, or keep it but replace the void statement with a clear TODO comment
describing the intended future use (including a tracking issue or expected API
calls) so its presence is justified; update all callers and any tests to match
the new signature if you remove it, or add the explanatory comment adjacent to
_wsClient in the constructor if you keep it.
- Around line 387-400: The fetchFriends method is a stub that is called by
refresh() but never actually refreshes data; either implement it to perform an
authenticated fetch and update the local friends state or remove the stub and
its call from refresh() (or add a TODO with a tracking issue). If you implement
it, use this.authService.getToken() to get the token, call the existing HTTP
client or API method to GET the friends list (e.g., GET /friends with
Authorization: Bearer <token>), update the provider's friends state and emit any
change events, and handle errors via this.logger.error; if you opt to keep
WebSocket-only behavior, delete fetchFriends and remove its invocation in
refresh(), or replace the body with a clear TODO comment referencing the issue
ID so maintainers know why it's a stub.
- Around line 351-373: handleFriendStatus currently unsafely casts payload to
UserStatus and ignores updates for unknown users; validate the payload shape
before using it and decide how to handle non-friend updates. Update
handleFriendStatus to: 1) perform a runtime check on payload (ensure it's an
object with userId, status, updatedAt and optional activity) before assigning to
a UserStatus variable; 2) guard all accesses (userId, status, updatedAt,
activity) behind that validation; 3) keep the existing update branch for entries
in this.friends (using buildActivityInfo, lastUpdated, this.friends.set and
this.onDidChangeTreeDataEmitter.fire and logger.debug) and add a clear path for
non-existing users (either ignore explicitly with a debug log or create a new
FriendInfo and insert it) based on intended behavior.
In @apps/extension/src/views/statusBarItem.ts:
- Around line 29-30: The inline comment for the status bar creation is
incorrect: it says "shown on the left" while the code calls
vscode.window.createStatusBarItem(vscode.StatusBarAlignment.Right, 100); update
the comment to accurately state that the item is shown on the right (e.g.,
"Create status bar item with high priority (shown on the right, near the right
edge)") or, if you intended it on the left, change StatusBarAlignment.Right to
StatusBarAlignment.Left so the code and comment match; reference the
statusBarItem creation using createStatusBarItem and StatusBarAlignment.Right in
your change.
In @apps/server/src/routes/auth.ts:
- Line 48: The exported function authRoutes is declared async but contains no
awaits, causing the @typescript-eslint/require-await lint error; either remove
the async keyword from the function declaration (export function
authRoutes(...): Promise<void> or adjust return type) or, if you intend to
perform async work later, keep async and add a short inline comment above
authRoutes explaining that it is intentionally async for future awaits (and add
a // eslint-disable-next-line @typescript-eslint/require-await only if
necessary). Ensure references to authRoutes and its signature are updated
accordingly.
- Around line 31-43: The GitHubAuthQuerySchema's redirect_uri validation
currently allows "http://" and redundantly checks for empty strings while also
using .optional(); update the schema so redirect_uri only allows vscode schemes
or secure "https://" in production (or add a clear comment if "http" is
intentionally allowed for local dev), and simplify the validation by removing
the "!uri" check—instead handle undefined via .optional() or run the refine only
when uri is defined; refer to GitHubAuthQuerySchema and the redirect_uri refine
to locate and change the logic accordingly.
In @apps/server/src/routes/users.ts:
- Line 73: The exported function userRoutes is declared async but contains no
awaits, triggering @typescript-eslint/require-await; either remove the async
keyword from the signature (change "export async function userRoutes(...)" to
"export function userRoutes(...)") or, if you intentionally keep it for
consistency/future async work, add a one-line comment above the function
explaining that intent or add a targeted disable: /* eslint-disable-next-line
@typescript-eslint/require-await */ so the linter is satisfied; update the
symbol userRoutes accordingly and ensure callers still work with a non-async
function if you remove async.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/server/src/routes/users.ts (1)
1-8: Update JSDoc comments to reflect new route paths.The file header comments still reference the old paths (e.g.,
GET /users/me), but the actual routes have been updated to simplified paths (e.g.,GET /me). Update the documentation to match the current implementation or clarify that these are the public-facing paths after prefix registration.🔎 Proposed fix
/** * User Routes * * User profile management: - * - GET /users/me - Get current user - * - GET /users/:id - Get user by ID - * - PATCH /users/me - Update current user + * - GET /me - Get current user + * - GET /:id - Get user by ID + * - PATCH /me - Update current user + * + * Note: These routes are registered with a /users prefix */apps/server/src/routes/auth.ts (1)
1-7: Update JSDoc comments to reflect new route paths.The file header references old paths (
/auth/github,/auth/callback), but routes have been updated to simplified paths (/github,/callback). Update documentation to match the implementation or clarify the public-facing paths after prefix registration.🔎 Proposed fix
/** * Authentication Routes * * Handles GitHub OAuth flow: - * - GET /auth/github - Redirect to GitHub login - * - GET /auth/callback - Handle OAuth callback + * - GET /github - Redirect to GitHub login + * - GET /callback - Handle OAuth callback + * + * Note: These routes are registered with an /auth prefix */
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
apps/extension/.gitignoreapps/extension/.vscode/launch.jsonapps/extension/.vscode/tasks.jsonapps/extension/.vscodeignoreapps/extension/CHANGELOG.mdapps/extension/README.mdapps/extension/eslint.config.jsapps/extension/package.jsonapps/extension/src/extension.tsapps/extension/src/services/activityTracker.tsapps/extension/src/services/authService.tsapps/extension/src/services/index.tsapps/extension/src/services/wsClient.tsapps/extension/src/utils/configManager.tsapps/extension/src/utils/index.tsapps/extension/src/utils/logger.tsapps/extension/src/views/activityProvider.tsapps/extension/src/views/friendsProvider.tsapps/extension/src/views/index.tsapps/extension/src/views/statusBarItem.tsapps/extension/tsconfig.jsonapps/server/src/routes/auth.tsapps/server/src/routes/users.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/extension/src/views/activityProvider.ts (3)
apps/extension/src/views/index.ts (2)
ActivityEvent(6-6)ActivityProvider(6-6)apps/extension/src/services/wsClient.ts (1)
WebSocketClient(42-380)apps/extension/src/utils/logger.ts (1)
Logger(22-125)
apps/server/src/routes/users.ts (1)
apps/server/src/services/db.ts (1)
getDb(25-39)
apps/extension/src/views/statusBarItem.ts (2)
apps/extension/src/services/wsClient.ts (1)
ConnectionState(29-29)apps/extension/src/utils/logger.ts (1)
Logger(22-125)
apps/extension/src/utils/configManager.ts (1)
apps/extension/src/utils/index.ts (2)
DevRadarConfig(6-6)ConfigManager(5-5)
🪛 ast-grep (0.40.3)
apps/extension/src/utils/configManager.ts
[warning] 159-159: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${regexPattern}$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 GitHub Actions: CI
apps/extension/package.json
[warning] 1-1: ESLint/Node warning: Module type of eslint.config.js is not specified and it doesn't parse as CommonJS. Reparsing as ES module. To eliminate this warning, add "type": "module" to package.json.
apps/server/src/routes/auth.ts
[error] 48-48: ESLint: Async function 'authRoutes' has no 'await' expression. @typescript-eslint/require-await.
apps/extension/eslint.config.js
[warning] 1-1: Module type of eslint.config.js is not specified and it doesn't parse as CommonJS. Reparsing as ES module; add "type": "module" to package.json to eliminate the warning.
🪛 GitHub Check: Build and Test
apps/server/src/routes/auth.ts
[failure] 48-48:
Async function 'authRoutes' has no 'await' expression
apps/server/src/routes/users.ts
[failure] 73-73:
Async function 'userRoutes' has no 'await' expression
🔇 Additional comments (26)
apps/extension/.gitignore (1)
1-15: LGTM! Standard gitignore patterns for VS Code extension.The gitignore file appropriately excludes build outputs, test artifacts, node modules, packages, and logs from version control.
apps/extension/.vscodeignore (1)
1-13: LGTM! Appropriate packaging exclusions.The vscodeignore file correctly excludes development artifacts, source files, and configuration files from the extension package, ensuring only necessary compiled outputs are distributed.
apps/extension/tsconfig.json (1)
1-37: Excellent strict TypeScript configuration with one caveat.The TypeScript configuration follows best practices with comprehensive strictness flags, declaration generation, and source maps enabled. However, the
"module": "CommonJS"setting (line 7) conflicts with the ES module syntax used ineslint.config.js, as noted in the review of that file.apps/extension/CHANGELOG.md (1)
1-21: LGTM! Well-structured changelog following best practices.The changelog follows the Keep a Changelog format and Semantic Versioning conventions. The initial release (0.1.0) is comprehensively documented with all major features listed.
apps/server/src/routes/auth.ts (2)
62-77: LGTM! Strong cookie security configuration.The cookie settings appropriately use
httpOnly,securein production,sameSite: 'lax'for OAuth compatibility, and a reasonable 10-minute expiry. This provides good protection against XSS and CSRF attacks.
154-169: LGTM! VS Code OAuth redirect flow is well-implemented.The implementation correctly:
- Retrieves and validates the redirect URI from the cookie
- Clears the cookie before redirecting
- Validates VS Code URI schemes
- Passes the token as a query parameter (standard for custom URI schemes)
- Logs the redirect for debugging
- Falls back to JSON response for non-VS Code clients
apps/extension/src/utils/logger.ts (1)
75-106: LGTM! Robust log formatting and serialization.The implementation handles various data types appropriately with:
- Comprehensive type checking (Error, objects, primitives)
- Safe JSON serialization with try-catch
- Clear fallback for non-serializable data
- Well-formatted output with timestamp, level, and prefix
apps/extension/.vscode/launch.json (2)
14-22: LGTM!The "No Build" configuration is useful for faster iteration during development when artifacts are already built.
12-12: The preLaunchTask reference is correctly configured.The task labeled
"pnpm: build"is properly defined in.vscode/tasks.jsonwith all required properties (type, command, args, problemMatcher). The launch configuration will work as intended.Likely an incorrect or invalid review comment.
apps/extension/src/utils/index.ts (1)
1-7: LGTM!Clean barrel export consolidating utility exports. The type-only export for
DevRadarConfigis correctly specified.apps/extension/src/services/index.ts (1)
1-7: LGTM!Clean barrel export consolidating service exports. The inline type exports are correctly specified.
apps/extension/src/views/index.ts (1)
1-7: LGTM!Clean barrel export consolidating view exports. The inline type exports are correctly specified.
apps/extension/package.json (6)
13-29: LGTM!The engine version
^1.85.0is reasonable, andonStartupFinishedactivation ensures minimal impact on VS Code startup performance.
33-64: LGTM!Views and view containers are properly configured. Ensure the extension sets the
devradar.isAuthenticatedcontext key to control the welcome view visibility.
65-107: LGTM!Commands are well-defined with appropriate icons and categories. The command palette integration will work correctly.
162-177: LGTM! Good privacy defaults.The
blacklistedFilesarray includes sensible defaults for preventing sensitive files (.env, *.pem, *.key, etc.) from being broadcast. This provides important privacy protection out of the box.
213-223: LGTM!The build scripts are properly configured. The esbuild configuration correctly bundles the extension, externalizes the vscode module, and targets Node.js with CommonJS format.
225-238: LGTM!Dependencies are well-organized with appropriate use of workspace protocol for the monorepo. The devDependencies include all necessary tooling for building and linting.
apps/extension/src/views/activityProvider.ts (1)
100-102: LGTM - Reserved parameter pattern.The unused logger parameter with void statement is an acceptable pattern for maintaining a consistent constructor signature while reserving the parameter for future use.
apps/extension/src/services/wsClient.ts (2)
239-256: LGTM - Robust heartbeat implementation.The heartbeat mechanism properly detects dead connections by checking for pong responses within a reasonable timeout (2x heartbeat interval) and cleanly closes stale connections. The custom close code (4000) correctly triggers reconnection logic.
271-302: LGTM - Well-implemented reconnection strategy.The exponential backoff with jitter is correctly implemented. The jitter calculation prevents thundering herd issues, and the max attempts guard prevents infinite reconnection loops. User notification on failure is a good UX touch.
apps/extension/src/utils/configManager.ts (1)
30-51: LGTM - Strong security-focused defaults.The default configuration includes comprehensive blacklist patterns for sensitive files (.env, *.pem, *.key, secrets/, credentials/), which helps prevent accidental exposure of credentials in activity tracking.
apps/extension/src/services/authService.ts (1)
244-311: LGTM: URI handler is well-implemented.The
DevRadarUriHandlercorrectly implements the OAuth callback flow with:
- Flexible path matching for callback URIs
- Proper timeout handling
- Clear logging for debugging
- Appropriate error propagation
The promise-based callback mechanism with timeout ensures the login flow won't hang indefinitely if the callback doesn't arrive.
apps/extension/src/views/friendsProvider.ts (1)
37-178: LGTM: Excellent tree item implementation.The
FriendTreeItemclass provides a rich, well-structured UI with:
- Detailed markdown tooltips showing friend status and activity
- Context-aware descriptions based on current activity
- Proper use of VS Code theme colors for status indicators
- Clean formatting helpers for duration display
The code follows VS Code extension best practices and provides good UX.
apps/extension/src/services/activityTracker.ts (2)
151-214: LGTM: Well-designed event handling strategy.The event listener registration demonstrates thoughtful design:
- Immediate updates for significant events (editor changes, debug start/stop, window focus)
- Debounced updates for high-frequency events (text changes) to avoid spam
- Activity recording only for low-impact events (selection changes)
- Smart idle detection on window blur
This approach effectively balances real-time updates with performance and network efficiency.
323-378: LGTM: Comprehensive privacy controls.The
buildActivityPayloadmethod implements robust privacy features:
- Privacy mode support (lines 325-329)
- File-level blacklisting with selective field exposure (lines 345-353)
- Workspace-level blacklisting (lines 355-359)
- Granular control over which fields to share based on settings (lines 364-376)
The implementation correctly handles various privacy scenarios while maintaining useful activity tracking.
| import config from '@devradar/eslint-config'; | ||
|
|
||
| export default config; |
There was a problem hiding this comment.
Fix the module type configuration issue.
The pipeline warning indicates that the module type is not specified in package.json, causing Node.js to initially parse this file as CommonJS before reparsing it as an ES module. This creates unnecessary overhead and potential issues.
Additionally, there's an inconsistency: this file uses ES module syntax (import/export) while tsconfig.json specifies "module": "CommonJS". This mismatch can lead to module resolution issues.
🔎 Recommended solutions
Option 1 (Recommended): Add "type": "module" to package.json and update TypeScript config:
In apps/extension/package.json:
{
"type": "module"
}In apps/extension/tsconfig.json:
- "module": "CommonJS",
+ "module": "ES2022",Option 2: Rename this file to eslint.config.mjs to explicitly mark it as an ES module:
mv apps/extension/eslint.config.js apps/extension/eslint.config.mjsOption 3: Convert to CommonJS syntax:
-import config from '@devradar/eslint-config';
+const config = require('@devradar/eslint-config');
-export default config;
+module.exports = config;Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: CI
[warning] 1-1: Module type of eslint.config.js is not specified and it doesn't parse as CommonJS. Reparsing as ES module; add "type": "module" to package.json to eliminate the warning.
| { | ||
| "name": "devradar", | ||
| "displayName": "DevRadar", | ||
| "description": "Discord Status for VS Code - See what your friends are coding in real-time", | ||
| "version": "0.1.0", | ||
| "publisher": "devradar", | ||
| "license": "MIT", | ||
| "private": true, | ||
| "repository": { | ||
| "type": "git", | ||
| "url": "https://github.com/devradar/devradar" | ||
| }, | ||
| "engines": { | ||
| "vscode": "^1.85.0" | ||
| }, | ||
| "categories": [ | ||
| "Other", | ||
| "Visualization" | ||
| ], | ||
| "keywords": [ | ||
| "presence", | ||
| "friends", | ||
| "social", | ||
| "status", | ||
| "coding" | ||
| ], | ||
| "activationEvents": [ | ||
| "onStartupFinished" | ||
| ], | ||
| "main": "./dist/extension.js", | ||
| "icon": "media/icon.png", | ||
| "contributes": { | ||
| "viewsContainers": { | ||
| "activitybar": [ | ||
| { | ||
| "id": "devradar", | ||
| "title": "DevRadar", | ||
| "icon": "media/icon.svg" | ||
| } | ||
| ] | ||
| }, | ||
| "views": { | ||
| "devradar": [ | ||
| { | ||
| "id": "devradar.friends", | ||
| "name": "Friends", | ||
| "icon": "media/friends.svg", | ||
| "contextualTitle": "DevRadar Friends" | ||
| }, | ||
| { | ||
| "id": "devradar.activity", | ||
| "name": "Activity", | ||
| "icon": "media/activity.svg", | ||
| "contextualTitle": "DevRadar Activity" | ||
| } | ||
| ] | ||
| }, | ||
| "viewsWelcome": [ | ||
| { | ||
| "view": "devradar.friends", | ||
| "contents": "Welcome to DevRadar! 🚀\n\nConnect with your developer friends and see what they're coding in real-time.\n\n[Login with GitHub](command:devradar.login)", | ||
| "when": "!devradar.isAuthenticated" | ||
| } | ||
| ], | ||
| "commands": [ | ||
| { | ||
| "command": "devradar.login", | ||
| "title": "Login with GitHub", | ||
| "category": "DevRadar", | ||
| "icon": "$(github)" | ||
| }, | ||
| { | ||
| "command": "devradar.logout", | ||
| "title": "Logout", | ||
| "category": "DevRadar", | ||
| "icon": "$(sign-out)" | ||
| }, | ||
| { | ||
| "command": "devradar.togglePrivacy", | ||
| "title": "Toggle Privacy Mode", | ||
| "category": "DevRadar", | ||
| "icon": "$(eye-closed)" | ||
| }, | ||
| { | ||
| "command": "devradar.poke", | ||
| "title": "Poke Friend", | ||
| "category": "DevRadar", | ||
| "icon": "$(bell)" | ||
| }, | ||
| { | ||
| "command": "devradar.refreshFriends", | ||
| "title": "Refresh Friends", | ||
| "category": "DevRadar", | ||
| "icon": "$(refresh)" | ||
| }, | ||
| { | ||
| "command": "devradar.viewProfile", | ||
| "title": "View Profile", | ||
| "category": "DevRadar", | ||
| "icon": "$(account)" | ||
| }, | ||
| { | ||
| "command": "devradar.setStatus", | ||
| "title": "Set Status", | ||
| "category": "DevRadar", | ||
| "icon": "$(broadcast)" | ||
| } | ||
| ], | ||
| "menus": { | ||
| "view/title": [ | ||
| { | ||
| "command": "devradar.refreshFriends", | ||
| "when": "view == devradar.friends", | ||
| "group": "navigation" | ||
| } | ||
| ], | ||
| "view/item/context": [ | ||
| { | ||
| "command": "devradar.poke", | ||
| "when": "view == devradar.friends && viewItem == friend-online", | ||
| "group": "inline" | ||
| }, | ||
| { | ||
| "command": "devradar.viewProfile", | ||
| "when": "view == devradar.friends" | ||
| } | ||
| ] | ||
| }, | ||
| "configuration": { | ||
| "title": "DevRadar", | ||
| "properties": { | ||
| "devradar.serverUrl": { | ||
| "type": "string", | ||
| "default": "http://localhost:3000", | ||
| "description": "DevRadar server URL" | ||
| }, | ||
| "devradar.wsUrl": { | ||
| "type": "string", | ||
| "default": "ws://localhost:3000/ws", | ||
| "description": "DevRadar WebSocket URL" | ||
| }, | ||
| "devradar.privacyMode": { | ||
| "type": "boolean", | ||
| "default": false, | ||
| "description": "Enable privacy mode to hide your activity from friends" | ||
| }, | ||
| "devradar.showFileName": { | ||
| "type": "boolean", | ||
| "default": true, | ||
| "description": "Show the current file name in your status" | ||
| }, | ||
| "devradar.showProject": { | ||
| "type": "boolean", | ||
| "default": true, | ||
| "description": "Show the current project name in your status" | ||
| }, | ||
| "devradar.showLanguage": { | ||
| "type": "boolean", | ||
| "default": true, | ||
| "description": "Show the current programming language in your status" | ||
| }, | ||
| "devradar.blacklistedFiles": { | ||
| "type": "array", | ||
| "default": [ | ||
| ".env", | ||
| ".env.*", | ||
| "*.pem", | ||
| "*.key", | ||
| "*.secret", | ||
| "**/secrets/**", | ||
| "**/credentials/**" | ||
| ], | ||
| "items": { | ||
| "type": "string" | ||
| }, | ||
| "description": "File patterns to never broadcast (for privacy)" | ||
| }, | ||
| "devradar.blacklistedWorkspaces": { | ||
| "type": "array", | ||
| "default": [], | ||
| "items": { | ||
| "type": "string" | ||
| }, | ||
| "description": "Workspace folder names to never broadcast activity from" | ||
| }, | ||
| "devradar.idleTimeout": { | ||
| "type": "number", | ||
| "default": 300000, | ||
| "minimum": 60000, | ||
| "maximum": 3600000, | ||
| "description": "Time in milliseconds before being marked as idle (default: 5 minutes)" | ||
| }, | ||
| "devradar.heartbeatInterval": { | ||
| "type": "number", | ||
| "default": 30000, | ||
| "minimum": 10000, | ||
| "maximum": 60000, | ||
| "description": "Heartbeat interval in milliseconds (default: 30 seconds)" | ||
| }, | ||
| "devradar.enableNotifications": { | ||
| "type": "boolean", | ||
| "default": true, | ||
| "description": "Show notifications when friends come online or poke you" | ||
| }, | ||
| "devradar.showStatusBarItem": { | ||
| "type": "boolean", | ||
| "default": true, | ||
| "description": "Show DevRadar status in the status bar" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "scripts": { | ||
| "vscode:prepublish": "pnpm run build", | ||
| "build": "esbuild src/extension.ts --bundle --outfile=dist/extension.js --external:vscode --format=cjs --platform=node --minify", | ||
| "watch": "esbuild src/extension.ts --bundle --outfile=dist/extension.js --external:vscode --format=cjs --platform=node --sourcemap --watch", | ||
| "dev": "pnpm run watch", | ||
| "check-types": "tsc --noEmit", | ||
| "lint": "eslint .", | ||
| "lint:fix": "eslint . --fix", | ||
| "package": "vsce package --no-dependencies", | ||
| "publish": "vsce publish --no-dependencies", | ||
| "clean": "rimraf dist out" | ||
| }, | ||
| "dependencies": { | ||
| "@devradar/shared": "workspace:*" | ||
| }, | ||
| "devDependencies": { | ||
| "@devradar/eslint-config": "workspace:*", | ||
| "@devradar/tsconfig": "workspace:*", | ||
| "@types/node": "^22.0.0", | ||
| "@types/vscode": "^1.85.0", | ||
| "@vscode/vsce": "^3.0.0", | ||
| "esbuild": "^0.24.0", | ||
| "eslint": "^9.39.2", | ||
| "rimraf": "^6.1.2", | ||
| "typescript": "^5.9.3" | ||
| } | ||
| } |
There was a problem hiding this comment.
Resolve the ESLint module type warning.
The pipeline reports that eslint.config.js is being parsed as ES module because the module type isn't specified. You have two options:
- Add
"type": "module"to package.json if you want to use ES modules throughout (but note your extension uses CommonJS format) - Rename
eslint.config.jstoeslint.config.cjsto explicitly mark it as CommonJS and avoid the ambiguity
Option 2 is recommended since your extension build uses CommonJS format (--format=cjs in line 215).
🔎 Recommended fix: Verify and rename ESLint config
#!/bin/bash
# Check if eslint.config.js exists and suggest renaming
if [ -f "apps/extension/eslint.config.js" ]; then
echo "✓ Found eslint.config.js"
echo ""
echo "Recommended action: Rename to eslint.config.cjs to match CommonJS format"
echo "Command: mv apps/extension/eslint.config.js apps/extension/eslint.config.cjs"
else
echo "✗ eslint.config.js not found in apps/extension/"
echo "The file may be at the workspace root or elsewhere"
fd -t f "eslint.config.js"
fi🧰 Tools
🪛 GitHub Actions: CI
[warning] 1-1: ESLint/Node warning: Module type of eslint.config.js is not specified and it doesn't parse as CommonJS. Reparsing as ES module. To eliminate this warning, add "type": "module" to package.json.
🤖 Prompt for AI Agents
In @apps/extension/package.json around lines 1-239, The ESLint parser warning is
caused by eslint.config.js being interpreted as ESM while the extension build
uses CommonJS; rename eslint.config.js to eslint.config.cjs (or alternatively
set "type": "module" in package.json) to resolve the ambiguity—locate the ESLint
config file (eslint.config.js) and rename it to eslint.config.cjs so it matches
the CommonJS build (see the build script using --format=cjs in the "build"
script) and re-run the pipeline.
| "onStartupFinished" | ||
| ], | ||
| "main": "./dist/extension.js", | ||
| "icon": "media/icon.png", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that required media assets exist
cd apps/extension 2>/dev/null || { echo "Directory not found"; exit 1; }
echo "=== Checking for required media assets ==="
for file in media/icon.png media/icon.svg media/friends.svg media/activity.svg; do
if [ -f "$file" ]; then
echo "✓ Found: $file"
else
echo "✗ Missing: $file"
fi
done
echo ""
echo "=== Listing all files in media directory ==="
if [ -d "media" ]; then
ls -la media/
else
echo "media directory does not exist"
fiRepository: senutpal/devRadar
Length of output: 291
Create required media assets.
The extension references four media files (icon.png, icon.svg, friends.svg, activity.svg) in package.json (lines 31, 38, 47, 53), but the media directory and all referenced files are missing. Create these assets or remove the references from package.json before packaging the extension.
🤖 Prompt for AI Agents
In @apps/extension/package.json around line 31, package.json references four
media assets ("icon.png", "icon.svg", "friends.svg", "activity.svg") but the
media directory and files are missing; either add the media directory and create
those files (appropriate sizes/formats for extension icons and SVGs) and commit
them, or remove the corresponding keys from package.json to avoid broken
references; ensure the chosen approach is consistent (if adding files, update
.npmignore/packaging so media/ is included; if removing, remove all four
entries) and verify the extension loads without missing asset errors.
| const GitHubAuthQuerySchema = z.object({ | ||
| redirect_uri: z | ||
| .string() | ||
| .refine( | ||
| (uri) => | ||
| !uri || | ||
| uri.startsWith('vscode://') || | ||
| uri.startsWith('vscode-insiders://') || | ||
| uri.startsWith('http'), | ||
| 'Invalid redirect URI scheme' | ||
| ) | ||
| .optional(), | ||
| }); |
There was a problem hiding this comment.
Review redirect_uri validation for security and clarity.
The validation allows http:// schemes in addition to VS Code schemes. Consider:
-
Security concern:
http://redirects in production could be vulnerable to token interception. If this is intended for local development only, add a comment or restrict tohttps://in production environments. -
Validation redundancy: The refine at line 36 checks
!uriwhich allows empty strings, but the field is already marked.optional()at line 42. Consider simplifying the validation.
🔎 Suggested improvements
const GitHubAuthQuerySchema = z.object({
redirect_uri: z
.string()
+ .optional()
.refine(
(uri) =>
- !uri ||
- uri.startsWith('vscode://') ||
+ uri.startsWith('vscode://') ||
uri.startsWith('vscode-insiders://') ||
- uri.startsWith('http'),
+ (uri.startsWith('https://') || (uri.startsWith('http://') && !isProduction)),
- 'Invalid redirect URI scheme'
+ 'Invalid redirect URI scheme (only vscode://, vscode-insiders://, or https:// allowed in production)'
- )
- .optional(),
+ ),
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/server/src/routes/auth.ts around lines 31-43, The
GitHubAuthQuerySchema's redirect_uri validation currently allows "http://" and
redundantly checks for empty strings while also using .optional(); update the
schema so redirect_uri only allows vscode schemes or secure "https://" in
production (or add a clear comment if "http" is intentionally allowed for local
dev), and simplify the validation by removing the "!uri" check—instead handle
undefined via .optional() or run the refine only when uri is defined; refer to
GitHubAuthQuerySchema and the redirect_uri refine to locate and change the logic
accordingly.
- add $tsc-watch problem matcher to dev task in tasks.json - rename eslint.config.js to .cjs to fix module type issues - update repository url to point to senutpal/devRadar - increase default heartbeat interval to 60s - update server/ws url descriptions to mention env overrides - generate missing icon.png asset - update README installation steps for local development - synchronize README settings table with package.json defaults
- Add type guards and runtime validation for poke functionality - Validate and sanitize user input (max 500 chars, strip control chars/HTML) - Add strict runtime checks for friend status and poke activities Security: - Remove auth token from WebSocket URL query string - Implement WebSocket authentication via initial message - Handle auth failure with automatic logout - Replace vulnerable glob regex with minimatch library Reliability: - Update activity deduplication to consider session duration and intensity - Add periodic token revalidation - Fix logger static initialization - Improve error logging with stack traces - Add proper resource cleanup on deactivation Dependencies: - Add minimatch for safe glob matching
- Fix status bar comment to correctly state "shown on the right" - Improve redirect_uri validation to enforce https in production - Allow http:// only in development with explanatory comment - Remove async from authRoutes and userRoutes (no awaits needed)
senutpal
force-pushed
the
feat/backend-implementation-phase-1.2
branch
from
3d288d0 to
1b04e17
Compare
There was a problem hiding this comment.
Actionable comments posted: 33
🤖 Fix all issues with AI Agents
In @apps/extension/README.md:
- Around line 35-38: The README settings table contains duplicated header and
separator rows; remove the duplicate pair so there is only one header row "|
Setting | Default | Description |" followed immediately by a single separator
row "| --- | --- | --- |" and then the table entries; update the remaining table
rows accordingly in apps/extension/README.md to ensure the markdown table
renders correctly.
In @apps/extension/src/extension.ts:
- Around line 333-335: Add a comment suppressing the Biome lint false positive
and documenting intent above the control-character regex used to build
sanitizedMessage; specifically, next to the line where sanitizedMessage is
assigned (which uses trimmedMessage.replace(/[\u0000-\u001f\u007f-\u009f]/g,
'')...), add a brief suppression comment that states the regex is intentionally
removing control characters and that the Biome warning can be ignored.
- Around line 351-360: Replace the inline object/type checks in
handleViewProfile with the existing isFriendItem type guard: change the
signature to accept unknown, call isFriendItem(item) to narrow the type, then
use item.username and item.userId accordingly (e.g., construct the GitHub URL
from item.username and open it). Ensure you import/retain the isFriendItem
function and remove the manual cast to { userId: string; username?: string } so
the guard provides the narrowing.
In @apps/extension/src/services/authService.ts:
- Around line 204-225: The validateToken method performs side effects (it sets
this.currentUser and calls this.context.globalState.update with USER_KEY)
despite its name implying a pure check; rename the method to
validateAndRefreshToken or document its behavior and update all callers and
tests to use the new name (or keep the name but add JSDoc noting the side
effects), and ensure references to validateToken, currentUser,
context.globalState.update, and USER_KEY are updated accordingly so callers
expect the refresh/persist behavior.
- Around line 283-321: The current loose matching in handleUri (using
uri.path.includes('auth/callback') || uri.path.includes('callback')) can accept
unintended paths; tighten it by replacing the includes checks with a stricter
match against the path (e.g., check for exact segment or end-of-path like path
=== '/auth/callback' or use a RegExp such as /^\/auth\/callback(?:$|\/)/ or
/^\/callback(?:$|\/)/ to allow only the intended callback routes), apply the new
check where the if currently inspects uri.path, and keep the existing
token/error handling and resolver clearing logic unchanged.
In @apps/extension/src/services/wsClient.ts:
- Around line 190-208: The code sets connectionState to 'connected' immediately
in the ws.onopen handler before server-side auth is confirmed; change this so
ws.onopen only sends the AUTH (via send('AUTH', { token })) and starts an auth
timeout, and move setConnectionState('connected'), reset of reconnectAttempts,
lastPongTime, startHeartbeat(), and flushMessageQueue() into the AUTH_SUCCESS
path inside the message handler (the handler that processes incoming messages
and looks for the 'AUTH_SUCCESS' event); also ensure the auth timeout will call
disconnect() / set an unauthenticated state if AUTH_SUCCESS isn't received in
time and preserve the existing token-missing branch behavior.
In @apps/extension/src/utils/configManager.ts:
- Around line 67-71: The constructor of ConfigManager initializes this.cache via
loadConfig() but never subscribes to vscode.workspace.onDidChangeConfiguration,
so external changes leave the cache stale; in the ConfigManager constructor
register a listener with vscode.workspace.onDidChangeConfiguration that calls
this.reload() (or emits via this.onConfigChangeEmitter) when relevant
configuration sections change, push that disposable into this.disposables, and
ensure the listener is disposed in the existing dispose flow to avoid leaks.
In @apps/extension/src/views/activityProvider.ts:
- Around line 257-271: The createConflictActivity function lacks validation for
the payload fields and can throw if editors is missing or not an array; update
createConflictActivity to verify payload.fileHash is a non-empty string and
payload.editors is an array (Array.isArray) before using .length, return null if
validation fails, and use a safe count (e.g., editors.length ?? 0) when building
the description; reference the createConflictActivity function and the conflict
variable when making these checks and the early return.
In @apps/extension/src/views/friendsProvider.ts:
- Around line 290-334: The response is currently cast to { data: any[] } in
fetchFriends; replace this with a properly declared API response type (e.g., an
interface like FriendResponse { data: FriendDTO[] } and FriendDTO matching the
fields used to construct FriendInfo) and use that type in the JSON cast inside
fetchFriends; update mapping to use FriendDTO (or validate required fields) when
building FriendInfo and preserve lastUpdated logic so you avoid untyped any[]
usage and make the contract explicit for future changes.
- Around line 170-178: The formatDuration function currently returns "0m" for
durations under 60 seconds; update the logic in formatDuration to compute the
seconds remainder (seconds % 60) and when total minutes === 0 return a seconds
string like "Xs"; for minute-level durations include seconds as optional "Mm Ss"
if desired (e.g., when seconds>0) and keep the existing "Hh Mm" output when
hours>0; modify the method name formatDuration to use these computed minutes and
seconds values so short sessions show seconds instead of "0m".
- Line 236: The FriendsProvider class implements
vscode.TreeDataProvider<FriendTreeItem | GroupTreeItem> but lacks the required
methods getTreeItem and getChildren; implement getTreeItem(element:
FriendTreeItem | GroupTreeItem): vscode.TreeItem to return the TreeItem
representation (delegate to element when it already extends TreeItem or
construct one using its label/collapsibleState/iconPath/command) and implement
async getChildren(element?: GroupTreeItem | FriendTreeItem):
Promise<Array<FriendTreeItem | GroupTreeItem>> to return root groups when
element is undefined and the group's children when a GroupTreeItem is passed
(use existing data structures and methods in FriendsProvider that populate
friends/groups), and ensure to fire this._onDidChangeTreeData when data changes
via the provider’s existing event emitter.
- Line 262: The FriendsProvider class calls this.buildActivityInfo(...) but the
method is missing; implement a private buildActivityInfo(activity: any):
ActivityInfo method in FriendsProvider that maps the incoming activity payload
to an ActivityInfo object with properties fileName, language, project, and
sessionDuration (derive sessionDuration from activity.duration or compute from
start/end if present), return undefined/null only if activity is falsy, and
ensure callers at the lines where newActivity is created (calls at
status.activity) and the other invocation expect the same structure; add
appropriate null checks and type conversions to avoid runtime errors.
In @apps/extension/src/views/statusBarItem.ts:
- Around line 48-51: The update() method can suffer from out-of-order async
completions; to fix it add a sequence guard: introduce a numeric updateCounter
property on the class, increment it at the start of update() and capture the
current id in a local variable, then await this.authService.isAuthenticated()
and only assign to this.isAuthenticated and call this.render() if the captured
id still equals the latest updateCounter; this ensures stale results from
earlier calls don’t overwrite newer state while leaving
authService.isAuthenticated(), update(), isAuthenticated, render() usage intact.
In @apps/server/prisma/schema.prisma:
- Around line 7-9: The Prisma datasource block for "db" is missing a connection
URL; update the datasource db definition (datasource db { ... }) to include a
URL loaded from an environment variable (e.g., use env("DATABASE_URL")) or a
directUrl so Prisma can connect; ensure you add the url (or directUrl) entry
inside the datasource block and document/set the corresponding DATABASE_URL env
var in the environment.
In @apps/server/src/routes/auth.ts:
- Around line 191-210: The MessageType union in packages/shared/src/types.ts is
missing the 'AUTH' variant causing type errors when wsClient.ts calls
this.send('AUTH', { token }); update the MessageType union to include 'AUTH' (or
change the wsClient send call to use an existing allowed type) and ensure the
send() signature/type aliases (MessageType, any related enums or type aliases)
reflect this addition so the call site this.send('AUTH', ...) type-checks
correctly.
In @apps/server/src/routes/friends.ts:
- Around line 81-101: The current cast "as Promise<FollowWithUser[]>" on the
db.follow.findMany call bypasses Prisma's type inference and is fragile; remove
that assertion and instead use Prisma's generated payload type to ensure the
query shape matches the type (e.g., replace the cast with a typed declaration
using Prisma.FollowGetPayload<{ include: { following: true } }>[] or use a
`satisfies` expression against that payload type) targeting the
db.follow.findMany call and the FollowWithUser reference so the returned shape
is statically checked rather than forcibly asserted.
- Around line 276-297: Replace the two-step find-and-delete with a single db
operation: call db.follow.deleteMany with the same composite key filter
(followerId: userId, followingId: targetUserId), then check the returned count
(or affected rows) and throw NotFoundError('Follow relationship') if count is 0;
remove the prior db.follow.findUnique and db.follow.delete calls and use the
deleteMany result to determine success to avoid the extra round-trip and race
conditions.
- Around line 73-76: The current code silently falls back to defaults when
PaginationQuerySchema.safeParse(request.query) fails; update the handler to
explicitly handle invalid pagination by checking paginationResult.success and
either (a) return a 400 validation error response with details from
paginationResult.error or (b) at minimum log a warning including request.query
and paginationResult.error before applying defaults; reference
PaginationQuerySchema, paginationResult, request.query and the derived
page/limit variables to locate and update the logic so malformed params (e.g.,
page=-1 or limit=abc) are not silently ignored.
In @apps/server/src/routes/users.ts:
- Around line 254-258: Remove the orphaned JSDoc block for "GET /users/search"
at the end of the file (the /** ... */ comment) that is duplicated; delete that
JSDoc comment and the stray closing brace after it so only the real route
documentation and its implementation remain (look for the /** GET /users/search
*/ block near the file end in apps/server/src/routes/users.ts).
- Around line 107-114: The JSDoc for "GET /users/:id" is misplaced above the
"/users/search" route; move that comment so it sits immediately above the route
handler for GET /users/:id (or delete it if redundant) so it no longer documents
GET /users/search — locate the orphaned comment and reposition it directly above
the router.get('/users/:id', ...) handler (or remove it) and ensure the GET
/users/search block only has its own appropriate comment.
In @apps/server/src/services/github.ts:
- Around line 197-210: The functions getUserById and getUserByGitHubId currently
use redundant type assertions ("as Promise<User | null>") on the results of
db.user.findUnique; remove those assertions and return the findUnique call
directly (i.e., keep the same signature but return db.user.findUnique({...})
without casting) so Prisma's inferred types are used and the code is cleaner.
- Around line 87-97: Validate that the parsed TokenResponse contains a non-empty
data.access_token before returning: if access_token is missing or falsy, log an
error with the full response (use logger.error and include data and any relevant
fields like data.scope) and throw an AuthenticationError with a clear message
(e.g., 'Missing access_token in GitHub response'), otherwise return
data.access_token; update the code around the response.json() handling and the
existing error branch that uses TokenResponse, data, logger.error, and
AuthenticationError.
In @apps/server/src/ws/handler.ts:
- Around line 486-493: The broadcastToUsers function's public API uses a loose
string for the message type; change the signature of broadcastToUsers(userIds:
string[], type: string, payload: unknown): void to use the strongly typed
OutboundMessageType instead of string, update any internal references and
callers to pass an OutboundMessageType, and ensure OutboundMessageType is
imported or exported where broadcastToUsers is defined so the public API surface
is correctly typed (keep send's signature consistent if it already expects
OutboundMessageType).
- Around line 161-175: The current loop subscribes per-friend using ws.friendIds
and REDIS_KEYS.presenceChannel which doesn't scale; change to a pattern-based
subscription: subscribe once with PSUBSCRIBE (e.g., pattern "presence:*") via
subscriber.psubscribe and maintain a server-side channelSubscriptions or
patternSubscriptions map to track which userIds care about which friendId
channels, then filter incoming pmessage events by extracting the friendId from
the channel name and forwarding only to ws.userId when friendId is in
ws.friendIds; update any unsubscribe logic to remove userId from the tracked set
and only call punsubscribe when no users remain for the pattern.
- Around line 269-273: The poke authorization uses the stale in-memory
ws.friendIds snapshot set at connection time (populated where the socket is
initialized), so refresh friends before the check in the poke handler: replace
the direct read of ws.friendIds in the block that calls sendError with an
up-to-date fetch (e.g. call the existing user/friend service method like
fetchFriendIds/getFriendIds for ws.userId) or first apply any pending
subscription updates; alternatively implement SUBSCRIBE/UNSUBSCRIBE message
handling to update ws.friendIds on-demand and/or add a periodic refresh for
ws.friendIds so the poke check uses fresh data rather than the connection-time
snapshot.
- Around line 391-397: The code currently reads the JWT from request.query into
token (token variable) and rejects if missing; replace this with a flow that
accepts only short-lived, single-use WebSocket tokens: call a new/updated
validateWebSocketToken(token) function from the connection handler to verify TTL
and uniqueness, then immediately call invalidateWebSocketToken(token) upon
successful validation; also ensure any logging of request.url or request.query
excludes the token (strip query param before logging) and add an inline comment
and documentation noting the security tradeoff and recommending proxy config to
drop query params from access logs, leaving ws.close(WsCloseCodes.UNAUTHORIZED,
'Token required') behavior unchanged for missing/invalid tokens.
- Around line 356-366: The current setTimeout-based grace period can leave stale
presence when the server restarts; replace or supplement this with Redis-side
TTL on the presence keys (set an expiry when creating/updating presence) so keys
auto-expire if the process dies, and keep or shorten the in-process grace logic
in the ws disconnect handler (references: ws.userId, connections, setTimeout) to
only handle fast reconnects. Also ensure deletePresence calls surface failures
by awaiting the promise and logging errors (reference: deletePresence and
log.debug) instead of swallowing them; log a descriptive error on rejection so
cleanup problems are visible.
- Around line 78-92: The send function is bypassing TypeScript checks by taking
type: string and casting it to WsMessage['type']; change the signature to accept
type: WsMessage['type'] (or a generic constrained to WsMessage['type']) so the
compiler enforces valid message types, remove the manual cast inside send, and
update any call sites that pass arbitrary strings to use the allowed
WsMessage['type'] values (or cast at the call site only when truly necessary).
In @apps/server/src/ws/types.ts:
- Around line 68-71: HeartbeatPayload currently defines status and activity but
the handler uses HeartbeatMessageSchema which expects { ping?: boolean }, so
reconcile them: either update the HeartbeatPayload interface to match the
handler schema (e.g., define optional ping?: boolean) or remove/rename the
unused HeartbeatPayload and replace its references with the correct schema type
used by the handler (HeartbeatMessageSchema); search for usages of
HeartbeatPayload in apps/server/src/ws and update any type annotations to the
chosen shape and ensure exports/imports remain correct.
- Around line 76-80: The local PokePayload interface in ws/types.ts is
inconsistent with the shared PokePayload (from the shared package) because
fromUserId is optional locally but required in the shared type; fix by either:
(a) replacing the local declaration with a direct re-export or import of the
shared PokePayload to ensure a single source of truth, (b) if semantics differ,
rename the local type (e.g., InboundPokePayload or OutboundPokePayload) and
document the difference, or (c) make the local PokePayload match the shared
shape (make fromUserId required) so both types align; update any usages of
PokePayload accordingly (functions, handlers, and type imports) to refer to the
chosen type.
- Around line 36-42: InboundMessageType declares 'PING' but the handler's
InboundMessageSchema in handler.ts lacks it causing validation failures; update
the handler to support PING by adding 'PING' to the InboundMessageSchema array
and implement handling logic in the handler function (or if PING is not used,
remove 'PING' from the InboundMessageType union instead) — locate the symbols
InboundMessageType, InboundMessageSchema and the handler function in handler.ts
to make the change.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
apps/extension/media/activity.svgis excluded by!**/*.svgapps/extension/media/friends.svgis excluded by!**/*.svgapps/extension/media/icon.svgis excluded by!**/*.svg
📒 Files selected for processing (23)
apps/extension/.vscode/tasks.jsonapps/extension/README.mdapps/extension/eslint.config.cjsapps/extension/package.jsonapps/extension/src/extension.tsapps/extension/src/services/activityTracker.tsapps/extension/src/services/authService.tsapps/extension/src/services/wsClient.tsapps/extension/src/utils/configManager.tsapps/extension/src/utils/logger.tsapps/extension/src/views/activityProvider.tsapps/extension/src/views/friendsProvider.tsapps/extension/src/views/statusBarItem.tsapps/server/prisma/schema.prismaapps/server/src/routes/auth.tsapps/server/src/routes/friends.tsapps/server/src/routes/users.tsapps/server/src/services/db.tsapps/server/src/services/github.tsapps/server/src/ws/handler.tsapps/server/src/ws/types.tspackages/shared/src/types.tspackages/shared/src/validators.ts
🧰 Additional context used
🧬 Code graph analysis (8)
apps/server/src/routes/friends.ts (5)
apps/server/src/services/db.ts (1)
getDb(21-34)packages/shared/src/validators.ts (1)
PaginationQuerySchema(91-94)apps/server/src/services/redis.ts (1)
getPresences(246-271)apps/server/src/lib/errors.ts (3)
ValidationError(75-89)ConflictError(142-153)NotFoundError(129-137)apps/server/src/lib/logger.ts (1)
logger(79-79)
apps/server/src/routes/auth.ts (5)
apps/server/src/config.ts (1)
isProduction(80-80)apps/server/src/services/github.ts (2)
getGitHubAuthUrl(43-56)authenticateWithGitHub(151-189)apps/server/src/lib/logger.ts (1)
logger(79-79)apps/server/src/lib/errors.ts (1)
ValidationError(75-89)apps/server/src/services/redis.ts (1)
blacklistToken(285-293)
apps/extension/src/views/statusBarItem.ts (1)
apps/extension/src/services/wsClient.ts (1)
ConnectionState(29-29)
apps/server/src/services/db.ts (3)
apps/server/src/config.ts (2)
env(75-75)isDevelopment(85-85)apps/server/src/lib/logger.ts (1)
logger(79-79)apps/extension/src/utils/logger.ts (1)
error(65-73)
apps/extension/src/services/authService.ts (3)
apps/extension/src/services/index.ts (2)
AuthState(5-5)AuthService(5-5)packages/shared/src/types.ts (1)
UserDTO(90-100)apps/extension/src/utils/logger.ts (2)
Logger(22-138)error(65-73)
apps/extension/src/services/activityTracker.ts (3)
packages/shared/src/types.ts (3)
UserStatusType(4-4)ActivityPayload(45-62)Intensity(40-40)apps/extension/src/services/wsClient.ts (1)
WebSocketClient(42-405)apps/extension/src/utils/configManager.ts (1)
ConfigManager(57-162)
apps/server/src/ws/types.ts (1)
packages/shared/src/types.ts (2)
UserStatusType(4-4)PokePayload(105-109)
apps/extension/src/services/wsClient.ts (4)
packages/shared/src/types.ts (5)
WebSocketMessage(78-85)MessageType(19-27)UserStatusType(4-4)ActivityPayload(45-62)PokePayload(105-109)apps/extension/src/services/authService.ts (1)
AuthService(32-267)apps/extension/src/utils/logger.ts (1)
error(65-73)packages/shared/src/constants.ts (2)
HEARTBEAT_INTERVAL_MS(10-10)WS_RECONNECT_CONFIG(20-25)
🪛 Biome (2.1.2)
apps/extension/eslint.config.cjs
[error] 1-1: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 2-3: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
apps/extension/src/extension.ts
[error] 334-334: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 334-334: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (62)
apps/extension/.vscode/tasks.json (1)
1-24: LGTM! Task configuration is properly set up.Both tasks are correctly configured with appropriate problem matchers:
$tscfor the build task and$tsc-watchfor the background dev task. This ensures TypeScript errors will surface in the VS Code Problems panel during development.packages/shared/src/validators.ts (3)
1-1: LGTM! Standard Zod import path.The import has been updated to use the standard
zodpackage path, which is the correct approach for current Zod versions.
29-29: LGTM! PONG message type added for WebSocket heartbeat pattern.The addition of
'PONG'to the message type schema aligns with standard WebSocket ping-pong patterns for connection health monitoring, and is consistent with the corresponding type definition inpackages/shared/src/types.ts.
61-61: LGTM! Proper UUID validation for optional correlationId.The correlationId field correctly uses
string().uuid().optional()to maintain UUID format validation while allowing the field to be omitted, which is appropriate for request-response correlation patterns.packages/shared/src/types.ts (1)
26-27: LGTM! PONG message type added to support WebSocket heartbeat.The addition of
'PONG'to theMessageTypeunion is consistent with the validator changes inpackages/shared/src/validators.tsand supports the WebSocket heartbeat/pong pattern for connection health monitoring.apps/extension/package.json (3)
9-12: Repository URL is correctly configured.The repository URL now correctly points to
https://github.com/senutpal/devRadar, matching the actual repository location.
193-199: Heartbeat interval default is now appropriate.The default
heartbeatIntervalhas been set to 60000ms (1 minute), which is a reasonable balance between responsiveness and server load. The configuration allows users to tune it between 10-60 seconds based on their needs.
225-227: Workspace dependency setup looks correct.The workspace-scoped dependencies (
@devradar/shared,@devradar/eslint-config,@devradar/tsconfig) are properly configured for a monorepo structure.apps/server/src/routes/friends.ts (2)
107-123: Friend data transformation is well-structured.The mapping correctly handles privacy mode by returning
'incognito'status and omitting activity whenprivacyModeis true. The fallback to'offline'for missing presence is appropriate.
322-336: Efficient mutual friends query.The single query using
ANDwith nestedsomeclauses is an efficient approach to find mutual friends. Good use of Prisma's relational filtering.apps/server/src/services/github.ts (2)
65-107: Timeout and error handling is well-implemented.Good use of
AbortSignal.timeout()for preventing hanging requests, and proper re-throwing ofAuthenticationErrorwhile wrapping other errors appropriately.
161-176: User upsert logic is correct.The upsert correctly creates new users or updates existing ones based on
githubId. The fields being updated (username,displayName,avatarUrl,apps/server/src/routes/users.ts (3)
73-74: Function signature is now correctly synchronous.The
userRoutesfunction is no longerasync, which resolves the previous ESLint warning about async functions without await expressions.
43-68: Good defensive programming with tier validation.The
toUserDTOfunction validates tier values at runtime and provides a safe default of'FREE'for invalid database values. This protects against data corruption or schema drift.
185-197: Privacy mode handling is well-implemented.When a user has
privacyModeenabled, the response correctly returns a limited profile withstatus: 'incognito'and omits sensitive data like follower counts and activity.apps/extension/src/extension.ts (3)
274-281: Type guard implementation addresses previous concern.The
isFriendItemtype guard properly validates thatitemis a non-null object with auserIdproperty of typestring, which addresses the previous feedback about runtime type validation.
315-339: Poke message validation and sanitization implemented.Good implementation of input validation with a 500-character limit, user cancellation handling, and sanitization to remove control characters and HTML tags. This addresses the previous feedback.
477-495: Singleton pattern and lifecycle management is well-structured.The module-level singleton with proper disposal on deactivation, including the static
Logger.disposeShared()call, ensures clean resource management.apps/server/src/services/db.ts (4)
21-34: Database singleton initialization is well-implemented.The lazy initialization pattern with the PrismaPg adapter is correct. The conditional logging (verbose in development, errors-only in production) is a good practice.
40-50: Connection error handling is appropriate.Using
logger.fatalfor database connection failures and re-throwing the error allows the caller (typically server startup) to handle the failure appropriately, such as terminating the process.
56-62: Disconnection correctly resets singleton state.Setting
prisma = nullafter disconnection ensures a fresh client is created on the nextgetDb()call, which is important for graceful shutdown and restart scenarios.
69-76: Health check appropriately swallows errors.Returning
falseinstead of throwing on health check failure is the correct pattern for health endpoints, allowing the caller to report unhealthy status without crashing.apps/server/prisma/schema.prisma (5)
32-43: Follow model relations and constraints are well-designed.The composite primary key
@@id([followerId, followingId])prevents duplicate follows, and cascade deletes ensure referential integrity when users are deleted. Both foreign key columns are properly indexed.
45-59: Team model structure is appropriate.The unique
slugconstraint enables URL-friendly team identifiers, and cascade delete on owner relation ensures teams are removed when owners are deleted. Consider whether orphaning teams to a system user might be preferable in some cases.
61-74: TeamMember model correctly prevents duplicate memberships.The
@@unique([userId, teamId])constraint properly prevents a user from being added to the same team multiple times.
1-5: Generator configuration is correct for ESM output.The generator outputs to a local path with ESM module format, which aligns with the server's module configuration.
11-30: This review comment is based on an incorrect assumption. The username field should not be marked as unique.The system correctly uses
githubIdandidas unique identifiers (verified at findUnique calls). Username is intentionally mutable and synced from GitHub on each authentication. The index on username exists to support the search functionality (case-insensitive contains search), not to enforce uniqueness. Users can update their username when they change their GitHub login, and the system is designed to handle this without conflicts. No documentation is needed as this is working as intended.Likely an incorrect or invalid review comment.
apps/server/src/routes/auth.ts (3)
1-44: Schema definitions and route setup look good.The
GitHubAuthQuerySchemanow properly restrictshttp://to development environments only, addressing the earlier security concern. The function is correctly declared synchronous.
54-85: LGTM - OAuth initiation flow is secure.The CSRF state generation, secure cookie settings, and proper redirect handling follow OAuth best practices.
216-247: Logout gracefully handles edge cases.The try-catch with graceful degradation for token blacklisting is appropriate—logout should succeed even if blacklisting fails. The dynamic import comment explains the circular dependency avoidance.
apps/extension/src/views/statusBarItem.ts (3)
21-35: Constructor parameters reserved for future use.The explicit
voidstatements for unused parameters are clear. Consider using TypeScript's underscore prefix convention consistently (already done) and adding a brief TODO if these will be used soon.
56-97: Render logic handles all connection states correctly.The state machine covers all
ConnectionStatevalues with appropriate icons, text, and background colors. The fallback todefaultfordisconnectedis defensive.
102-116: Trusted MarkdownString is required for command links.Setting
isTrusted = trueandsupportHtml = trueis necessary for the[command:...]links to work. This is safe since the content is fully controlled by the extension.apps/extension/src/views/activityProvider.ts (4)
28-83: ActivityTreeItem implementation is well-structured.The icon mapping and relative time formatting are comprehensive. The
formatTimemethod handles edge cases appropriately.
163-198: Type validation properly implemented.The runtime checks for
userIdandstatusbeing strings address the earlier review concern. The optionalusernamehandling is correct.
203-233: Poke activity creation has proper validation.The
fromUserIdstring check and optional field handling are implemented correctly.
276-306: Activity management and disposal are correct.The
unshift+slicepattern maintains the most recent activities efficiently, and the disposal loop properly cleans up resources.apps/extension/src/utils/configManager.ts (3)
1-52: Configuration schema and defaults are well-defined.The default blacklist patterns for sensitive files (
.env,*.pem,*.key, etc.) are security-conscious defaults.
150-155: ReDoS concern addressed with minimatch.The
matchGlobimplementation now uses theminimatchlibrary with{ dot: true }, which safely handles glob patterns and hidden files.
90-99: Cache updated before persistence confirmed.If
config.update()fails (line 96), the local cache at line 97 will already reflect the new value, causing inconsistency between persisted and in-memory state.🔎 Proposed fix
async update<K extends keyof DevRadarConfig>( key: K, value: DevRadarConfig[K], target: vscode.ConfigurationTarget = vscode.ConfigurationTarget.Global ): Promise<void> { const config = vscode.workspace.getConfiguration('devradar'); await config.update(key, value, target); - this.cache[key] = value; + this.cache = this.loadConfig(); // Reload to ensure consistency this.onConfigChangeEmitter.fire(this.cache); }Likely an incorrect or invalid review comment.
apps/extension/src/services/wsClient.ts (3)
107-116: Security improvement: Token no longer passed in URL.The WebSocket now connects without credentials in the URL and authenticates via a post-connection message, addressing the earlier security concern about tokens in query parameters.
261-291: Heartbeat implementation is robust.The dead connection detection using
lastPongTimecomparison and the proper cleanup instopHeartbeatare well implemented.
293-327: Reconnection logic with exponential backoff and jitter is correct.The jitter implementation (±25%) helps prevent thundering herd issues when many clients reconnect simultaneously.
apps/extension/src/utils/logger.ts (4)
27-32: Log level initialization moved to static block.The static initialization block ensures the log level is evaluated once based on environment variables, addressing the earlier concern about per-instance re-evaluation.
65-73: Error logging now includes stack traces in OutputChannel.The
errormethod now formats the error with stack trace for the OutputChannel (line 67), while also logging to console for debugging. This addresses the earlier inconsistency concern.
78-109: Log formatting handles various data types well.The fallback cases for unserializable data are appropriate. The empty catch at line 103 could silently swallow serialization errors, but the
[Unable to serialize data]message is an acceptable fallback.
125-137: Static dispose method added for proper cleanup.The
disposeShared()method addresses the earlier concern about OutputChannel cleanup during extension deactivation.apps/extension/src/services/authService.ts (4)
44-63: Periodic token validation addresses session staleness.The 5-minute validation interval (lines 55-62) ensures that expired or revoked tokens are detected within a reasonable timeframe, addressing the earlier concern about stale tokens.
81-118: isAuthenticated() optimistic return is acceptable with periodic validation.The method returns
trueimmediately if a token exists in memory (line 89), relying on the background periodic validation to catch invalid tokens. This is a reasonable trade-off between latency and correctness.
137-174: OAuth login flow is well-implemented.The flow correctly uses VS Code's external URI handling, waits for callback with timeout, and stores credentials securely.
326-339: waitForCallback timeout handling is correct.The Promise-based callback pattern with timeout ensures the login flow doesn't hang indefinitely.
apps/server/src/ws/handler.ts (2)
63-63: Single connection per user may cause issues with multiple devices/tabs.The
connectionsMap usesuserIdas the key, allowing only one connection per user. If a user opens the app in multiple browser tabs or devices, only the latest connection will receive messages, and the previous one will be forcibly closed (line 423). This may be intentional, but consider whether users expect multi-device support.If multi-device support is intended in the future, consider changing to
Map<string, Set<AuthenticatedWebSocket>>to track multiple connections per user.
38-43: LGTM!The layered validation approach is well-designed: envelope validation first, then type-specific payload validation in individual handlers.
apps/extension/src/services/activityTracker.ts (6)
466-491: Past review concern has been addressed.The
isDuplicateActivitymethod now includes checks forsessionDurationandintensitychanges (lines 479-483), which addresses the previous review concern about heartbeat updates being suppressed.
445-460: LGTM!The intensity calculation is well-implemented with appropriate guards against division by very small windows.
493-499: LGTM!Clean disposal pattern that properly stops tracking and cleans up all VS Code event subscriptions.
323-378: LGTM!Privacy handling is well-implemented with appropriate levels of data masking for privacy mode, blacklisted files, and blacklisted workspaces.
247-260: LGTM!Debounce implementation correctly clears existing timeouts before scheduling new ones.
265-273: VerifyheartbeatIntervalconfiguration is in milliseconds.The
heartbeatIntervalfromconfigManageris passed directly tosetInterval(), which expects milliseconds. If the config value is in seconds (e.g.,30meaning "30 seconds"), the heartbeat would fire every 30 milliseconds instead. Check the configuration source and type definition forconfigManager.get('heartbeatInterval')to confirm it returns milliseconds, or apply a conversion factor (e.g.,interval * 1000) if needed.apps/server/src/ws/types.ts (1)
112-143: LGTM!Close codes are well-organized with both standard WebSocket codes and custom application codes (4xxx range) with corresponding human-readable reasons.
apps/extension/src/views/friendsProvider.ts (2)
228-234: Past concern addressed:_wsClientparameter removed.The constructor no longer includes the unused
_wsClientparameter that was flagged in a previous review.
241-276: Past concern addressed: Payload validation implemented.The
handleFriendStatusmethod now properly validates the payload structure (lines 246-255) before using it, addressing the previous review concern about unsafe type assertions.
| | Setting | Default | Description | | ||
| | -------------------------------- | -------------------------------------------- | ----------------------------------------------------------- | | ||
| | Setting | Default | Description | | ||
| | -------------------------------- | -------------------------------------------- | ----------------------------------------------------------- | |
There was a problem hiding this comment.
Remove duplicate table header rows.
The settings table contains duplicate header rows that will render incorrectly.
🔎 Proposed fix
| Setting | Default | Description |
| -------------------------------- | -------------------------------------------- | ----------------------------------------------------------- |
-| Setting | Default | Description |
-| -------------------------------- | -------------------------------------------- | ----------------------------------------------------------- |
| `devradar.privacyMode` | `false` | Hide your activity from friends |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | Setting | Default | Description | | |
| | -------------------------------- | -------------------------------------------- | ----------------------------------------------------------- | | |
| | Setting | Default | Description | | |
| | -------------------------------- | -------------------------------------------- | ----------------------------------------------------------- | | |
| | Setting | Default | Description | | |
| | -------------------------------- | -------------------------------------------- | ----------------------------------------------------------- | | |
| | `devradar.privacyMode` | `false` | Hide your activity from friends | |
🤖 Prompt for AI Agents
In @apps/extension/README.md around lines 35 - 38, The README settings table
contains duplicated header and separator rows; remove the duplicate pair so
there is only one header row "| Setting | Default | Description |" followed
immediately by a single separator row "| --- | --- | --- |" and then the table
entries; update the remaining table rows accordingly in apps/extension/README.md
to ensure the markdown table renders correctly.
| const sanitizedMessage = trimmedMessage | ||
| .replace(/[\u0000-\u001f\u007f-\u009f]/g, '') // Remove control chars | ||
| .replace(/<[^>]*>/g, ''); // Remove HTML tags |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Control character regex is intentional - suppress Biome warning.
The regex /[\u0000-\u001f\u007f-\u009f]/g is correctly used to sanitize control characters from user input. The Biome lint warning is a false positive in this context. Consider adding a suppression comment to document the intent.
🔎 Proposed fix
const trimmedMessage = message.trim();
// Sanitize: strip control characters (except simple whitespace) and HTML-like tags
+ // biome-ignore lint/suspicious/noControlCharactersInRegex: Intentionally removing control chars for sanitization
const sanitizedMessage = trimmedMessage
.replace(/[\u0000-\u001f\u007f-\u009f]/g, '') // Remove control chars
.replace(/<[^>]*>/g, ''); // Remove HTML tags🧰 Tools
🪛 Biome (2.1.2)
[error] 334-334: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 334-334: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🤖 Prompt for AI Agents
In @apps/extension/src/extension.ts around lines 333 - 335, Add a comment
suppressing the Biome lint false positive and documenting intent above the
control-character regex used to build sanitizedMessage; specifically, next to
the line where sanitizedMessage is assigned (which uses
trimmedMessage.replace(/[\u0000-\u001f\u007f-\u009f]/g, '')...), add a brief
suppression comment that states the regex is intentionally removing control
characters and that the Biome warning can be ignored.
| private handleViewProfile(item: unknown): void { | ||
| if (item && typeof item === 'object' && 'userId' in item) { | ||
| const friendItem = item as { userId: string; username?: string }; | ||
|
|
||
| if (friendItem.username) { | ||
| const url = `https://github.com/${friendItem.username}`; | ||
| void vscode.env.openExternal(vscode.Uri.parse(url)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Apply consistent type guard pattern for handleViewProfile.
The handleViewProfile method uses inline type narrowing similar to the pattern previously flagged in handlePoke. Consider reusing the isFriendItem type guard here for consistency.
🔎 Proposed fix
private handleViewProfile(item: unknown): void {
- if (item && typeof item === 'object' && 'userId' in item) {
- const friendItem = item as { userId: string; username?: string };
-
- if (friendItem.username) {
- const url = `https://github.com/${friendItem.username}`;
+ if (this.isFriendItem(item)) {
+ if (item.username) {
+ const url = `https://github.com/${item.username}`;
void vscode.env.openExternal(vscode.Uri.parse(url));
}
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/extension/src/extension.ts around lines 351 - 360, Replace the inline
object/type checks in handleViewProfile with the existing isFriendItem type
guard: change the signature to accept unknown, call isFriendItem(item) to narrow
the type, then use item.username and item.userId accordingly (e.g., construct
the GitHub URL from item.username and open it). Ensure you import/retain the
isFriendItem function and remove the manual cast to { userId: string; username?:
string } so the guard provides the narrowing.
| private async validateToken(token: string): Promise<boolean> { | ||
| try { | ||
| const serverUrl = this.configManager.get('serverUrl'); | ||
| const response = await fetch(`${serverUrl}/api/v1/users/me`, { | ||
| headers: { | ||
| Authorization: `Bearer ${token}`, | ||
| }, | ||
| }); | ||
|
|
||
| if (response.ok) { | ||
| const user = (await response.json()) as UserDTO; | ||
| this.currentUser = user; | ||
| await this.context.globalState.update(USER_KEY, JSON.stringify(user)); | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } catch (error) { | ||
| this.logger.warn('Token validation failed', error); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
validateToken has side effects that may be unexpected.
The method updates currentUser and persists to globalState on success. While this keeps user data fresh, the method name suggests a pure validation check. Consider documenting this behavior or renaming to validateAndRefreshToken.
🤖 Prompt for AI Agents
In @apps/extension/src/services/authService.ts around lines 204 - 225, The
validateToken method performs side effects (it sets this.currentUser and calls
this.context.globalState.update with USER_KEY) despite its name implying a pure
check; rename the method to validateAndRefreshToken or document its behavior and
update all callers and tests to use the new name (or keep the name but add JSDoc
noting the side effects), and ensure references to validateToken, currentUser,
context.globalState.update, and USER_KEY are updated accordingly so callers
expect the refresh/persist behavior.
| handleUri(uri: vscode.Uri): vscode.ProviderResult<void> { | ||
| this.logger.info('Received callback URI', { | ||
| path: uri.path, | ||
| query: uri.query, | ||
| scheme: uri.scheme, | ||
| authority: uri.authority, | ||
| }); | ||
|
|
||
| // Check if this is an auth callback (handle both /auth/callback and variations) | ||
| if (uri.path.includes('auth/callback') || uri.path.includes('callback')) { | ||
| const params = new URLSearchParams(uri.query); | ||
| const token = params.get('token'); | ||
| const error = params.get('error'); | ||
|
|
||
| this.logger.debug('Processing auth callback', { hasToken: !!token, hasError: !!error }); | ||
|
|
||
| if (this.callbackResolve) { | ||
| if (this.callbackTimeout) { | ||
| clearTimeout(this.callbackTimeout); | ||
| } | ||
|
|
||
| if (token) { | ||
| this.logger.info('Token received, completing login'); | ||
| this.callbackResolve({ success: true, token }); | ||
| } else { | ||
| this.logger.warn('No token in callback', { error }); | ||
| this.callbackResolve({ | ||
| success: false, | ||
| error: error ?? 'Unknown error', | ||
| }); | ||
| } | ||
| this.callbackResolve = null; | ||
| } else { | ||
| this.logger.warn('Received callback but no resolver waiting'); | ||
| } | ||
| } else { | ||
| this.logger.debug('Ignoring non-auth URI', { path: uri.path }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Loose URI path matching could accept unintended callbacks.
The condition at line 292 uses includes('auth/callback') || includes('callback') which would match any URI containing "callback" anywhere in the path (e.g., /some/callback/other).
🔎 Proposed fix for stricter path matching
- if (uri.path.includes('auth/callback') || uri.path.includes('callback')) {
+ if (uri.path === '/auth/callback' || uri.path.endsWith('/auth/callback')) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| handleUri(uri: vscode.Uri): vscode.ProviderResult<void> { | |
| this.logger.info('Received callback URI', { | |
| path: uri.path, | |
| query: uri.query, | |
| scheme: uri.scheme, | |
| authority: uri.authority, | |
| }); | |
| // Check if this is an auth callback (handle both /auth/callback and variations) | |
| if (uri.path.includes('auth/callback') || uri.path.includes('callback')) { | |
| const params = new URLSearchParams(uri.query); | |
| const token = params.get('token'); | |
| const error = params.get('error'); | |
| this.logger.debug('Processing auth callback', { hasToken: !!token, hasError: !!error }); | |
| if (this.callbackResolve) { | |
| if (this.callbackTimeout) { | |
| clearTimeout(this.callbackTimeout); | |
| } | |
| if (token) { | |
| this.logger.info('Token received, completing login'); | |
| this.callbackResolve({ success: true, token }); | |
| } else { | |
| this.logger.warn('No token in callback', { error }); | |
| this.callbackResolve({ | |
| success: false, | |
| error: error ?? 'Unknown error', | |
| }); | |
| } | |
| this.callbackResolve = null; | |
| } else { | |
| this.logger.warn('Received callback but no resolver waiting'); | |
| } | |
| } else { | |
| this.logger.debug('Ignoring non-auth URI', { path: uri.path }); | |
| } | |
| } | |
| handleUri(uri: vscode.Uri): vscode.ProviderResult<void> { | |
| this.logger.info('Received callback URI', { | |
| path: uri.path, | |
| query: uri.query, | |
| scheme: uri.scheme, | |
| authority: uri.authority, | |
| }); | |
| // Check if this is an auth callback (handle both /auth/callback and variations) | |
| if (uri.path === '/auth/callback' || uri.path.endsWith('/auth/callback')) { | |
| const params = new URLSearchParams(uri.query); | |
| const token = params.get('token'); | |
| const error = params.get('error'); | |
| this.logger.debug('Processing auth callback', { hasToken: !!token, hasError: !!error }); | |
| if (this.callbackResolve) { | |
| if (this.callbackTimeout) { | |
| clearTimeout(this.callbackTimeout); | |
| } | |
| if (token) { | |
| this.logger.info('Token received, completing login'); | |
| this.callbackResolve({ success: true, token }); | |
| } else { | |
| this.logger.warn('No token in callback', { error }); | |
| this.callbackResolve({ | |
| success: false, | |
| error: error ?? 'Unknown error', | |
| }); | |
| } | |
| this.callbackResolve = null; | |
| } else { | |
| this.logger.warn('Received callback but no resolver waiting'); | |
| } | |
| } else { | |
| this.logger.debug('Ignoring non-auth URI', { path: uri.path }); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @apps/extension/src/services/authService.ts around lines 283 - 321, The
current loose matching in handleUri (using uri.path.includes('auth/callback') ||
uri.path.includes('callback')) can accept unintended paths; tighten it by
replacing the includes checks with a stricter match against the path (e.g.,
check for exact segment or end-of-path like path === '/auth/callback' or use a
RegExp such as /^\/auth\/callback(?:$|\/)/ or /^\/callback(?:$|\/)/ to allow
only the intended callback routes), apply the new check where the if currently
inspects uri.path, and keep the existing token/error handling and resolver
clearing logic unchanged.
| // Extract token from query string | ||
| const token = (request.query as Record<string, string>).token; | ||
|
|
||
| if (!token) { | ||
| ws.close(WsCloseCodes.UNAUTHORIZED, 'Token required'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Token in query string poses security risks.
JWT tokens in query parameters can be logged in server access logs, proxy logs, and browser history. While this is a known limitation for WebSocket authentication (browsers can't set custom headers for the initial handshake), consider:
- Using short-lived, single-use tokens specifically for WebSocket connection
- Configuring server/proxy to exclude query parameters from access logs
- Documenting this security consideration
🤖 Prompt for AI Agents
In @apps/server/src/ws/handler.ts around lines 391 - 397, The code currently
reads the JWT from request.query into token (token variable) and rejects if
missing; replace this with a flow that accepts only short-lived, single-use
WebSocket tokens: call a new/updated validateWebSocketToken(token) function from
the connection handler to verify TTL and uniqueness, then immediately call
invalidateWebSocketToken(token) upon successful validation; also ensure any
logging of request.url or request.query excludes the token (strip query param
before logging) and add an inline comment and documentation noting the security
tradeoff and recommending proxy config to drop query params from access logs,
leaving ws.close(WsCloseCodes.UNAUTHORIZED, 'Token required') behavior unchanged
for missing/invalid tokens.
| export function broadcastToUsers(userIds: string[], type: string, payload: unknown): void { | ||
| for (const userId of userIds) { | ||
| const ws = connections.get(userId); | ||
| if (ws && ws.readyState === ws.OPEN) { | ||
| send(ws, type, payload); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Public API has loose typing.
Similar to the send function, broadcastToUsers accepts type: string which bypasses type safety. Since this is part of the public API, consider using OutboundMessageType for the type parameter.
🤖 Prompt for AI Agents
In @apps/server/src/ws/handler.ts around lines 486 - 493, The broadcastToUsers
function's public API uses a loose string for the message type; change the
signature of broadcastToUsers(userIds: string[], type: string, payload:
unknown): void to use the strongly typed OutboundMessageType instead of string,
update any internal references and callers to pass an OutboundMessageType, and
ensure OutboundMessageType is imported or exported where broadcastToUsers is
defined so the public API surface is correctly typed (keep send's signature
consistent if it already expects OutboundMessageType).
| export type InboundMessageType = | ||
| | 'HEARTBEAT' | ||
| | 'POKE' | ||
| | 'SUBSCRIBE' | ||
| | 'UNSUBSCRIBE' | ||
| | 'STATUS_UPDATE' | ||
| | 'PING'; |
There was a problem hiding this comment.
'PING' in InboundMessageType is not handled in the handler.
InboundMessageType includes 'PING' but the handler's InboundMessageSchema (in handler.ts, line 39) only includes ['HEARTBEAT', 'POKE', 'SUBSCRIBE', 'UNSUBSCRIBE', 'STATUS_UPDATE']. This inconsistency means PING messages would fail validation. Either add 'PING' handling to the handler or remove it from this type.
🤖 Prompt for AI Agents
In @apps/server/src/ws/types.ts around lines 36 - 42, InboundMessageType
declares 'PING' but the handler's InboundMessageSchema in handler.ts lacks it
causing validation failures; update the handler to support PING by adding 'PING'
to the InboundMessageSchema array and implement handling logic in the handler
function (or if PING is not used, remove 'PING' from the InboundMessageType
union instead) — locate the symbols InboundMessageType, InboundMessageSchema and
the handler function in handler.ts to make the change.
| export interface HeartbeatPayload { | ||
| status: UserStatusType; | ||
| activity?: SharedActivityPayload | undefined; | ||
| } |
There was a problem hiding this comment.
HeartbeatPayload doesn't match handler schema.
This HeartbeatPayload type expects status and activity fields, but the handler's HeartbeatMessageSchema (handler.ts, line 48-50) validates for { ping?: boolean }. These definitions are inconsistent - the type definition here appears unused or incorrect.
🤖 Prompt for AI Agents
In @apps/server/src/ws/types.ts around lines 68 - 71, HeartbeatPayload currently
defines status and activity but the handler uses HeartbeatMessageSchema which
expects { ping?: boolean }, so reconcile them: either update the
HeartbeatPayload interface to match the handler schema (e.g., define optional
ping?: boolean) or remove/rename the unused HeartbeatPayload and replace its
references with the correct schema type used by the handler
(HeartbeatMessageSchema); search for usages of HeartbeatPayload in
apps/server/src/ws and update any type annotations to the chosen shape and
ensure exports/imports remain correct.
| export interface PokePayload { | ||
| toUserId: string; | ||
| fromUserId?: string | undefined; | ||
| message?: string | undefined; | ||
| } |
There was a problem hiding this comment.
PokePayload differs from shared type.
The local PokePayload has fromUserId as optional, but the shared PokePayload in packages/shared/src/types.ts (lines 104-108) defines it as required. This inconsistency could cause type mismatches. Consider either:
- Aligning with the shared type
- Re-exporting from shared instead of redefining
- Using distinct names if the semantics differ (e.g.,
InboundPokePayloadvsOutboundPokePayload)
🤖 Prompt for AI Agents
In @apps/server/src/ws/types.ts around lines 76 - 80, The local PokePayload
interface in ws/types.ts is inconsistent with the shared PokePayload (from the
shared package) because fromUserId is optional locally but required in the
shared type; fix by either: (a) replacing the local declaration with a direct
re-export or import of the shared PokePayload to ensure a single source of
truth, (b) if semantics differ, rename the local type (e.g., InboundPokePayload
or OutboundPokePayload) and document the difference, or (c) make the local
PokePayload match the shared shape (make fromUserId required) so both types
align; update any usages of PokePayload accordingly (functions, handlers, and
type imports) to refer to the chosen type.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Add AUTH/AUTH_SUCCESS to MessageType for WebSocket authentication - Implement missing TreeDataProvider methods in FriendsProvider - Fix exactOptionalPropertyTypes compatibility in buildActivityInfo - Install minimatch dependency, fix ESLint config extension - Remove async from sync handleHeartbeat function
senutpal
force-pushed
the
feat/backend-implementation-phase-1.2
branch
from
697e556 to
d5bd1bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @apps/extension/package.json:
- Around line 193-199: DEFAULT_CONFIG in
apps/extension/src/utils/configManager.ts currently sets heartbeatInterval to
30000 while package.json schema default is 60000; update the heartbeatInterval
value in DEFAULT_CONFIG to 60000 to match the "devradar.heartbeatInterval"
schema so VS Code and runtime defaults are consistent (refer to the
DEFAULT_CONFIG constant and the heartbeatInterval key in configManager.ts).
In @apps/extension/src/extension.ts:
- Around line 331-341: The HTML-stripping regex in the sanitizedMessage creation
is single-pass and misses nested/split tags; change the logic that builds
sanitizedMessage (after trimmedMessage and control-char removal) to repeatedly
apply the .replace(/<[^>]*>/g, '') operation until the string stops changing
(e.g., loop comparing previous and next) so all nested or fragmented tags are
removed before calling this.wsClient.sendPoke(userId, sanitizedMessage); keep
the control-char removal step and preserve the trimmedMessage variable and
sendPoke call names for locating the change.
In @apps/extension/src/utils/configManager.ts:
- Around line 67-71: The class currently initializes its cache with
this.loadConfig() in the constructor and registers this.onConfigChangeEmitter
into disposables but does not subscribe to
vscode.workspace.onDidChangeConfiguration internally; since extension.ts calls
configManager.reload() externally, either add an internal subscription in the
constructor to vscode.workspace.onDidChangeConfiguration that calls
this.reload() (and ensure you push that subscription into this.disposables) or
add a clear JSDoc/comment on the constructor noting that configuration change
handling is performed externally via extension.ts and that reload() must be
invoked there; reference the constructor, loadConfig, disposables,
onConfigChangeEmitter and the reload() method when applying the change.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/extension/eslint.config.mjsapps/extension/package.jsonapps/extension/src/extension.tsapps/extension/src/services/authService.tsapps/extension/src/utils/configManager.tsapps/extension/src/views/friendsProvider.tsapps/server/src/ws/handler.tspackages/shared/src/types.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/extension/src/extension.ts (7)
apps/extension/src/utils/logger.ts (2)
Logger(22-138)error(65-73)apps/extension/src/utils/index.ts (1)
Logger(7-7)apps/extension/src/services/authService.ts (2)
AuthService(32-271)isAuthenticated(85-122)apps/extension/src/services/index.ts (3)
AuthService(5-5)WebSocketClient(6-6)ActivityTracker(7-7)apps/extension/src/services/wsClient.ts (1)
WebSocketClient(42-405)apps/extension/src/views/statusBarItem.ts (1)
StatusBarManager(16-135)packages/shared/src/types.ts (1)
UserStatusType(4-4)
apps/extension/src/services/authService.ts (3)
packages/shared/src/types.ts (1)
UserDTO(92-102)apps/extension/src/utils/configManager.ts (1)
ConfigManager(57-162)apps/extension/src/utils/logger.ts (2)
Logger(22-138)error(65-73)
apps/server/src/ws/handler.ts (2)
apps/server/src/ws/types.ts (1)
AuthenticatedWebSocket(13-31)apps/extension/src/services/wsClient.ts (1)
send(138-156)
apps/extension/src/views/friendsProvider.ts (2)
packages/shared/src/types.ts (4)
UserStatusType(4-4)ActivityPayload(47-64)UserStatus(69-75)UserDTO(92-102)apps/extension/src/utils/logger.ts (1)
Logger(22-138)
🪛 Biome (2.1.2)
apps/extension/src/extension.ts
[error] 335-335: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 335-335: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🪛 GitHub Check: CodeQL
apps/extension/src/extension.ts
[failure] 334-336: Incomplete multi-character sanitization
This string may still contain <script, which may cause an HTML element injection vulnerability.
🔇 Additional comments (13)
apps/extension/eslint.config.mjs (1)
1-3: LGTM! Previous module type issue resolved.The file has been correctly renamed to
.mjs, which explicitly marks it as an ES module and resolves the previous pipeline warning about module type not being specified. The implementation correctly forwards the shared ESLint configuration.packages/shared/src/types.ts (1)
19-29: LGTM! MessageType union expansion aligns with the new auth and heartbeat flows.The new
AUTHandAUTH_SUCCESStypes support the WebSocket authentication handshake, andPONGcomplements the existingHEARTBEATfor the ping-pong protocol.apps/server/src/ws/handler.ts (3)
227-251: LGTM! The STATUS_UPDATE handler is well-structured.Proper Zod validation before processing, and Redis presence update with the correct payload structure. The implementation integrates cleanly with the existing message handling flow.
209-225: LGTM! Simplified heartbeat handler with PONG response.Converting to synchronous is appropriate since there are no async operations. The PONG response aligns with the new message type added to the shared types.
312-334: LGTM! Message routing updated correctly.The
STATUS_UPDATEcase routes to the new handler, and the synchronoushandleHeartbeatcall is appropriate. TheNOT_IMPLEMENTEDresponse for dynamic subscriptions is a reasonable placeholder.apps/extension/src/extension.ts (2)
64-85: LGTM! Clean activation flow with proper error handling.The activation sequence correctly initializes tree views, commands, and event listeners before establishing the connection. Error logging and user notification are appropriately handled.
274-281: LGTM! Proper type guard implementation.The
isFriendItemtype guard correctly validates both object shape anduserIdtype, addressing the previous review concern about runtime type safety.apps/extension/src/utils/configManager.ts (1)
153-155: LGTM! ReDoS vulnerability addressed with minimatch.Using
minimatchinstead of custom regex pattern construction properly addresses the previous ReDoS concern.apps/extension/src/services/authService.ts (2)
55-67: LGTM! Periodic token validation addresses stale session concerns.The 5-minute background revalidation interval ensures tokens are checked periodically without blocking operations. The disposable pattern for cleanup is properly implemented.
141-178: LGTM! OAuth login flow is well-structured.The flow correctly handles external browser OAuth, timeout-based callback waiting, secure token storage, and proper event emission on success.
apps/extension/src/views/friendsProvider.ts (3)
231-268: LGTM! TreeDataProvider methods properly implemented.
getTreeItemandgetChildrenare correctly implemented. The grouping logic organizes friends by status with appropriate counts, and the filtering works correctly for each group.
280-294: LGTM! buildActivityInfo properly normalizes activity payload.The method correctly handles optional fields and creates the expected
FriendInfo['activity']structure.
299-334: LGTM! Payload validation added before type assertion.The validation now checks for required fields (
userId,status,updatedAt) before casting, addressing the previous concern about unsafe type assertions.
| "devradar.heartbeatInterval": { | ||
| "type": "number", | ||
| "default": 60000, | ||
| "minimum": 10000, | ||
| "maximum": 60000, | ||
| "description": "Heartbeat interval in milliseconds (default: 60 seconds; lower values increase server load)" | ||
| }, |
There was a problem hiding this comment.
Heartbeat interval default mismatch between package.json and configManager.ts.
The heartbeatInterval default here is 60000 (60 seconds), but DEFAULT_CONFIG in apps/extension/src/utils/configManager.ts (line 49) sets it to 30_000 (30 seconds). Since VS Code reads from package.json schema defaults when no user config exists, this inconsistency could cause confusion. Align these values.
🔎 Proposed fix in configManager.ts
const DEFAULT_CONFIG: DevRadarConfig = {
// ...
- heartbeatInterval: 30_000, // 30 seconds
+ heartbeatInterval: 60_000, // 60 seconds
// ...
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/extension/package.json around lines 193 - 199, DEFAULT_CONFIG in
apps/extension/src/utils/configManager.ts currently sets heartbeatInterval to
30000 while package.json schema default is 60000; update the heartbeatInterval
value in DEFAULT_CONFIG to 60000 to match the "devradar.heartbeatInterval"
schema so VS Code and runtime defaults are consistent (refer to the
DEFAULT_CONFIG constant and the heartbeatInterval key in configManager.ts).
| const trimmedMessage = message.trim(); | ||
| // Sanitize: strip control characters (except simple whitespace) and HTML-like tags | ||
| /* eslint-disable no-control-regex */ | ||
| const sanitizedMessage = trimmedMessage | ||
| .replace(/[\u0000-\u001f\u007f-\u009f]/g, '') // Remove control chars | ||
| .replace(/<[^>]*>/g, ''); // Remove HTML tags | ||
| /* eslint-enable no-control-regex */ | ||
|
|
||
| this.wsClient.sendPoke(userId, sanitizedMessage); | ||
| void vscode.window.showInformationMessage('DevRadar: Poke sent! 👋'); | ||
| } |
There was a problem hiding this comment.
Incomplete HTML sanitization allows injection via nested tags.
The single-pass regex .replace(/<[^>]*>/g, '') doesn't handle nested or split tags. For example, <scr<script>ipt>alert(1)</script> becomes <script>alert(1) after one pass. Apply the replacement iteratively until no more tags remain.
🔎 Proposed fix
const trimmedMessage = message.trim();
// Sanitize: strip control characters (except simple whitespace) and HTML-like tags
/* eslint-disable no-control-regex */
+ // biome-ignore lint/suspicious/noControlCharactersInRegex: Intentionally removing control chars for sanitization
+ let sanitizedMessage = trimmedMessage.replace(/[\u0000-\u001f\u007f-\u009f]/g, '');
+ // Iteratively remove HTML tags to handle nested/split tags like <scr<script>ipt>
+ let previousLength: number;
+ do {
+ previousLength = sanitizedMessage.length;
+ sanitizedMessage = sanitizedMessage.replace(/<[^>]*>/g, '');
+ } while (sanitizedMessage.length < previousLength);
- const sanitizedMessage = trimmedMessage
- .replace(/[\u0000-\u001f\u007f-\u009f]/g, '') // Remove control chars
- .replace(/<[^>]*>/g, ''); // Remove HTML tags
/* eslint-enable no-control-regex */🧰 Tools
🪛 Biome (2.1.2)
[error] 335-335: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 335-335: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🪛 GitHub Check: CodeQL
[failure] 334-336: Incomplete multi-character sanitization
This string may still contain <script, which may cause an HTML element injection vulnerability.
🤖 Prompt for AI Agents
In @apps/extension/src/extension.ts around lines 331 - 341, The HTML-stripping
regex in the sanitizedMessage creation is single-pass and misses nested/split
tags; change the logic that builds sanitizedMessage (after trimmedMessage and
control-char removal) to repeatedly apply the .replace(/<[^>]*>/g, '') operation
until the string stops changing (e.g., loop comparing previous and next) so all
nested or fragmented tags are removed before calling
this.wsClient.sendPoke(userId, sanitizedMessage); keep the control-char removal
step and preserve the trimmedMessage variable and sendPoke call names for
locating the change.
| constructor() { | ||
| this.cache = this.loadConfig(); | ||
|
|
||
| this.disposables.push(this.onConfigChangeEmitter); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Configuration change subscription handled externally.
While this class doesn't subscribe to vscode.workspace.onDidChangeConfiguration internally, the subscription is handled in apps/extension/src/extension.ts (lines 146-153) which calls configManager.reload(). This external pattern works but consider documenting it or internalizing the subscription for better encapsulation.
🤖 Prompt for AI Agents
In @apps/extension/src/utils/configManager.ts around lines 67 - 71, The class
currently initializes its cache with this.loadConfig() in the constructor and
registers this.onConfigChangeEmitter into disposables but does not subscribe to
vscode.workspace.onDidChangeConfiguration internally; since extension.ts calls
configManager.reload() externally, either add an internal subscription in the
constructor to vscode.workspace.onDidChangeConfiguration that calls
this.reload() (and ensure you push that subscription into this.disposables) or
add a clear JSDoc/comment on the constructor noting that configuration change
handling is performed externally via extension.ts and that reload() must be
invoked there; reference the constructor, loadConfig, disposables,
onConfigChangeEmitter and the reload() method when applying the change.
1 participant
Summary by CodeRabbit
New Features
Documentation
UX
✏️ Tip: You can customize this high-level summary in your review settings.