Sajdakabir/mar 302 clean refactor alpha be #609
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe changes involve a significant refactor of the Linear integration functionality across multiple files. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Hey there! 👋 We require pull request titles to follow the Conventional Commits specification. Valid types:
Format: Error details: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (12)
apps/backend/src/routers/integration/linear.route.js (2)
2-2: Improve import statement readabilityThe long import line could be formatted better for readability.
Consider breaking down the imports into multiple lines:
-import { getAccessTokenController, getMyLinearIssuesController, getTodayLinearIssuesController, getOverdueLinearIssuesController, getLinearIssuesByDateController, revokeLinearAccessController } from "../../controllers/integration/linear.controller.js"; +import { + getAccessTokenController, + getMyLinearIssuesController, + getTodayLinearIssuesController, + getOverdueLinearIssuesController, + getLinearIssuesByDateController, + revokeLinearAccessController +} from "../../controllers/integration/linear.controller.js";
Line range hint
6-11: Consider standardizing route naming conventionsThe routes use inconsistent naming patterns which could make the API less intuitive:
/getAccessToken/uses camelCase/revoke-access/uses kebab-case/issues/my/uses a resource-based approachConsider adopting a more RESTful and consistent approach:
-router.route("/getAccessToken/").get(getAccessTokenController) +router.route("/auth/token/").get(getAccessTokenController) -router.route("/revoke-access/").post(revokeLinearAccessController) +router.route("/auth/token/revoke/").post(revokeLinearAccessController)For better API design:
- Use nouns instead of verbs in URLs
- Use consistent casing (preferably kebab-case for URLs)
- Group related endpoints under resource names (e.g.,
/auth/for authentication-related endpoints)- Follow REST conventions for better API discoverability
apps/backend/src/jobs/linear.job.js (3)
Line range hint
6-23: Enhance error handling and add input validationWhile the basic error handling is in place, consider these improvements for more robust job processing:
const processLinearJob = async (job) => { + if (!job?.data) { + throw new Error('Invalid job: missing data'); + } const { accessToken, linearUserId, userId } = job.data; + if (!accessToken || !linearUserId || !userId) { + throw new Error('Missing required job parameters'); + } try { const issues = await fetchAssignedIssues(accessToken, linearUserId); await saveIssuesToDatabase(issues, userId); } catch (error) { - console.error('Error processing Linear job:', error); + console.error(`Error processing Linear job for user ${userId}:`, error); + if (error.response?.status === 401) { + throw new Error('Linear authentication failed'); + } + if (error.response?.status === 429) { + throw new Error('Linear API rate limit exceeded'); + } throw error; } };
24-32: Consider adding job processing configurationsThe worker configuration could benefit from additional settings to handle failures and timeouts more gracefully.
const linearWorker = new Worker('linearQueue', async (job) => { await processLinearJob(job); }, { connection: redisConnection, concurrency: 5, + // Add retry strategy for transient failures + defaultJobOptions: { + attempts: 3, + backoff: { + type: 'exponential', + delay: 1000 + } + }, + // Add timeout to prevent hanging jobs + timeout: 30000 });
Line range hint
34-65: Enhance monitoring and error trackingThe event listeners could provide more detailed information for monitoring and debugging purposes.
linearWorker.on('completed', async (job) => { - console.log(`Job with id ${job.id} has been completed`); + console.log(`Linear job ${job.id} completed successfully for user ${job.data.userId}`); await job.remove(); }); linearWorker.on('failed', (job, err) => { - console.error(`Job with id ${job.id} failed with error: ${err.message}`); + console.error( + `Linear job ${job.id} failed for user ${job.data.userId}`, + { + attempt: job.attemptsMade, + maxAttempts: job.opts.attempts, + error: err.message, + stack: err.stack + } + ); }); linearWorker.on('error', (err) => { - console.error('Redis connection error in linearWorker:', err); + console.error('Critical: Redis connection error in linearWorker:', { + error: err.message, + stack: err.stack + }); });apps/backend/src/controllers/integration/linear.controller.js (2)
18-20: Enhance error message specificityWhile the validation is good, the error message could be more specific about which parameter is missing to help with debugging.
Consider this improvement:
- return res.status(400).json({ error: 'Authorization code or user information is missing.' }); + return res.status(400).json({ + error: `Missing required parameters: ${!code ? 'authorization code' : ''} ${!user ? 'user information' : ''}`.trim() + });
Line range hint
34-42: Enhance error handling with specific error typesThe current error handling could be more informative by catching specific error types from the Linear API.
Consider this improvement:
try { const accessToken = await getAccessToken(code, user); const userInfo = await fetchUserInfo(accessToken, user); // ... queue code ... res.status(200).json({ accessToken }); } catch (err) { + // Log error for debugging + console.error('Linear access token retrieval failed:', err); + + // Handle specific error types + if (err.name === 'LinearAPIError') { + return res.status(503).json({ + error: 'Unable to communicate with Linear API', + details: err.message + }); + } + if (err.name === 'ValidationError') { + return res.status(400).json({ + error: 'Invalid request parameters', + details: err.message + }); + } next(err); }apps/backend/src/services/integration/linear.service.js (5)
Line range hint
7-42: Enhance error handling with specific error typesWhile the error handling is present, it could be more specific to help with debugging and client-side error handling.
Consider implementing this improved error handling:
const getAccessToken = async (code, user) => { try { const requestBody = { grant_type: 'authorization_code', code, redirect_uri: environment.LINEAR_REDIRECT_URL, client_id: environment.LINEAR_CLIENT_ID, client_secret: environment.LINEAR_CLIENT_SECRET }; const tokenResponse = await axios.post('https://api.linear.app/oauth/token', requestBody, { headers: { 'Content-Type': 'application/x-www-form-urlencoded' } }); const accessToken = tokenResponse.data.access_token; user.integration.linear.accessToken = accessToken; await user.save(); return accessToken; } catch (error) { - console.error('Error fetching Linear token:', error.response ? error.response.data : error.message); - throw error; + console.error('Error fetching Linear token:', error.response ? error.response.data : error.message); + if (error.response) { + const customError = new Error(error.response.data.error || 'Failed to fetch Linear token'); + customError.statusCode = error.response.status; + customError.code = 'LINEAR_TOKEN_ERROR'; + throw customError; + } + const serverError = new Error('Internal server error while fetching Linear token'); + serverError.statusCode = 500; + serverError.code = 'LINEAR_SERVER_ERROR'; + throw serverError; } };
Line range hint
43-77: Improve error handling and secure error loggingThe error handling could be enhanced, and the error logging should be sanitized to prevent potential sensitive data exposure.
Consider implementing these improvements:
const fetchUserInfo = async (linearToken, user) => { try { const response = await axios.post('https://api.linear.app/graphql', { query: ` query { viewer { id email name } } ` }, { headers: { Authorization: `Bearer ${linearToken}`, 'Content-Type': 'application/json' } }); + if (!response.data?.data?.viewer) { + throw new Error('Invalid response format from Linear API'); + } const userInfo = response.data.data.viewer user.integration.linear.userId = userInfo.id; user.integration.linear.connected = true; await user.save(); return userInfo; } catch (error) { - console.error('Error fetching user info:', error); + // Sanitize error logging to prevent sensitive data exposure + console.error('Error fetching Linear user info:', { + message: error.message, + status: error.response?.status, + errorType: error.name, + path: 'fetchUserInfo' + }); + if (error.response?.status === 401) { + const authError = new Error('Linear authentication failed'); + authError.statusCode = 401; + authError.code = 'LINEAR_AUTH_ERROR'; + throw authError; + } + const serverError = new Error('Failed to fetch Linear user info'); + serverError.statusCode = error.response?.status || 500; + serverError.code = 'LINEAR_USER_INFO_ERROR'; throw error; } };
Line range hint
156-308: Reduce code duplication in issue fetching functionsThe functions
getMyLinearIssues,getTodayLinearIssues,getOverdueLinearIssues, andgetLinearIssuesByDateshare similar patterns and validations.Consider extracting common functionality:
+ const ISSUE_FIELDS = ` + id + title + description + state { + name + } + labels { + nodes { + name + } + } + dueDate + `; + + const validateLinearAccess = (user) => { + const { accessToken, userId } = user.integration.linear; + if (!accessToken || !userId) { + const error = new Error("Linear access token or userId is missing"); + error.statusCode = 500; + throw error; + } + return { accessToken, userId }; + }; + + const queryLinearIssues = async (accessToken, query) => { + try { + const response = await axios.post('https://api.linear.app/graphql', + { query }, + { + headers: { + Authorization: `Bearer ${accessToken}`, + 'Content-Type': 'application/json' + } + } + ); + return response.data.data.issues.nodes; + } catch (error) { + console.error('Error querying Linear issues:', { + message: error.message, + status: error.response?.status, + errorType: error.name + }); + throw error; + } + };Then refactor the existing functions to use these utilities:
const getTodayLinearIssues = async (user) => { - const linearToken = user.integration.linear.accessToken; - const userId = user.integration.linear.userId; - if (!linearToken || !userId) { - const error = new Error("linearToken or userId is missing") - error.statusCode = 500 - throw error - } + const { accessToken, userId } = validateLinearAccess(user); const today = new Date(); const formattedToday = formatDate(today); - const response = await axios.post('https://api.linear.app/graphql', { - query: ` - query { - issues(filter: { assignee: { id: { eq: "${userId}" } }, dueDate: { eq: "${formattedToday}" } }) { - nodes { - id - title - description - state { - name - } - labels { - nodes { - name - } - } - dueDate - } - } - } - ` - }, { - headers: { - Authorization: `Bearer ${linearToken}`, - 'Content-Type': 'application/json' - } - }); - return response.data.data.issues.nodes; + const query = ` + query { + issues(filter: { assignee: { id: { eq: "${userId}" } }, dueDate: { eq: "${formattedToday}" } }) { + nodes { + ${ISSUE_FIELDS} + } + } + } + `; + return queryLinearIssues(accessToken, query); };
Line range hint
309-386: Add webhook signature verification and improve validationThe webhook handler lacks important security measures and proper payload validation.
Consider implementing these security improvements:
+ const crypto = require('crypto'); + + const verifyWebhookSignature = (payload, signature) => { + const expectedSignature = crypto + .createHmac('sha256', environment.LINEAR_WEBHOOK_SECRET) + .update(JSON.stringify(payload)) + .digest('hex'); + return crypto.timingSafeEqual( + Buffer.from(signature), + Buffer.from(expectedSignature) + ); + }; + const handleWebhookEvent = async (payload) => { try { + if (!payload || !payload.data) { + throw new Error('Invalid webhook payload'); + } + const issue = payload.data; if (payload.action === 'remove') { const deletedIssue = await Item.findOneAndDelete({ id: issue.id, source: 'linear' }); if (deletedIssue) { console.log(`Deleted issue with ID: ${issue.id}`); } else { console.log(`Issue with ID: ${issue.id} not found in the database.`); } return; } if (!issue.assignee || !issue.assignee.id) { const deletedIssue = await Item.findOneAndDelete({ id: issue.id, source: 'linear' }); if (deletedIssue) { console.log(`Unassigned issue with ID: ${issue.id} deleted from the database.`); } else { console.log(`Unassigned issue with ID: ${issue.id} not found in the database.`); } return; } // ... rest of the function + } catch (error) { + console.error('Error processing Linear webhook:', { + message: error.message, + errorType: error.name, + issueId: payload?.data?.id + }); + throw error; } };Also, update the controller that handles the webhook to verify the signature:
// In the webhook controller const signature = req.headers['x-linear-signature']; if (!signature || !verifyWebhookSignature(req.body, signature)) { throw new Error('Invalid webhook signature'); }
Line range hint
387-408: Enhance token revocation error handlingThe token revocation function could benefit from more specific error handling and validation.
Consider these improvements:
const revokeLinearAccess = async (accessToken) => { + if (!accessToken) { + throw new Error('Access token is required for revocation'); + } + try { await axios.post('https://api.linear.app/oauth/revoke', { token: accessToken }, { headers: { Authorization: `Bearer ${accessToken}`, 'Content-Type': 'application/x-www-form-urlencoded' } }); console.log('Linear access revoked successfully'); } catch (error) { - console.error('Error revoking Linear token:', error.response ? error.response.data : error.message); - throw error; + console.error('Error revoking Linear token:', { + status: error.response?.status, + message: error.response?.data?.error || error.message, + errorType: error.name + }); + + if (error.response?.status === 401) { + // Token might already be invalid + console.log('Token might already be invalid or expired'); + return; + } + + const customError = new Error('Failed to revoke Linear access'); + customError.statusCode = error.response?.status || 500; + customError.code = 'LINEAR_REVOKE_ERROR'; + throw customError; } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/backend/src/controllers/integration/linear.controller.js(1 hunks)apps/backend/src/jobs/linear.job.js(2 hunks)apps/backend/src/routers/integration/linear.route.js(1 hunks)apps/backend/src/services/integration/linear.service.js(2 hunks)
🔇 Additional comments (6)
apps/backend/src/routers/integration/linear.route.js (1)
6-6: Verify OAuth flow after removal of /connect endpoint
The removal of the /connect/ route suggests changes in the OAuth implementation. Let's verify that the OAuth flow is properly handled by the new /getAccessToken/ endpoint.
✅ Verification successful
OAuth flow is properly implemented after /connect removal
The verification shows that the OAuth flow is correctly implemented without the /connect endpoint:
- Frontend initiates OAuth via
handleLogin()which constructs the proper authorization URL with required parameters (client_id, redirect_uri, scope) - Backend handles the OAuth callback through
/getAccessTokenendpoint which:- Exchanges the authorization code for an access token
- Fetches user information
- Uses proper redirect URLs and credentials from environment variables
The removal of the /connect endpoint is part of a clean implementation where the OAuth flow is handled directly between the frontend's useLinear hook and the backend's getAccessTokenController.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old OAuth connect endpoint
# and verify the new OAuth implementation
# Check for any remaining references to the old connect endpoint
echo "Checking for references to old connect endpoint..."
rg -l "connect.*linear.*oauth" --type js
# Look for OAuth-related configuration
echo "Checking OAuth implementation..."
rg -l "linear.*oauth.*client" --type js
# Check for OAuth callback URL configurations
echo "Checking OAuth callback configurations..."
rg -l "linear.*callback.*url" --type js
Length of output: 405
Script:
#!/bin/bash
# Let's try a broader search to understand the Linear OAuth implementation
# Check the Linear router implementation
echo "=== Linear Router Implementation ==="
rg -A 10 "router\.route.*linear" --type js
# Look for Linear-related controller implementations
echo -e "\n=== Linear Controller Implementation ==="
rg -A 10 "getAccessTokenController" --type js
# Check for Linear OAuth-related environment variables or config
echo -e "\n=== Linear OAuth Configuration ==="
rg -B 2 -A 2 "LINEAR.*" .env* 2>/dev/null || true
rg -l "LINEAR" --type js
# Look for any Linear-related utility or service files
echo -e "\n=== Linear Related Files ==="
fd -e js -e ts linear
Length of output: 4053
Script:
#!/bin/bash
# Let's examine the Linear OAuth implementation details
# Check the Linear service implementation for OAuth flow
echo "=== Linear Service OAuth Implementation ==="
rg -A 15 "getAccessToken|fetchUserInfo" apps/backend/src/services/integration/linear.service.js
# Check frontend Linear hook implementation
echo -e "\n=== Frontend Linear Hook Implementation ==="
cat apps/frontend/src/hooks/useLinear.ts
# Look for Linear OAuth URL construction
echo -e "\n=== Linear OAuth URL Construction ==="
rg -A 5 "LINEAR.*URL|linear.*redirect" --type js
Length of output: 7007
apps/backend/src/jobs/linear.job.js (2)
Line range hint 67-70: LGTM: Exports are correctly defined
The exports properly expose both the queue and worker instances for external use.
Line range hint 1-4: Verify integration with Linear service and controller
Let's ensure the imported functions and integration points are properly aligned with the other Linear-related files.
✅ Verification successful
Let me gather more information to verify the integration properly.
Integration between Linear files is properly implemented
The verification shows that:
- The imported functions
fetchAssignedIssuesandsaveIssuesToDatabaseare correctly implemented inlinear.service.jswith matching signatures - The
linearQueueis properly integrated:- Defined in
bullmq.loader.js - Used correctly in
linear.controller.jsto add jobs - Consumed by the worker in
linear.job.js
- Defined in
- The worker implementation follows best practices with proper error handling and event listeners
- All imports and dependencies are correctly aligned across the Linear-related files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration between Linear files
# Check if the imported service functions exist and match the expected signature
echo "Checking Linear service functions..."
ast-grep --pattern 'export const fetchAssignedIssues = async (accessToken, linearUserId)'
ast-grep --pattern 'export const saveIssuesToDatabase = async (issues, userId)'
# Verify that the controller is using the queue correctly
echo "Checking Linear controller integration..."
ast-grep --pattern $'const result = await linearQueue.add\([^)]*\)'
# Check for any direct worker imports to ensure proper separation
echo "Checking for proper worker usage..."
rg -l "linearWorker" --type js
Length of output: 592
Script:
#!/bin/bash
# Let's check the actual implementations and usages
# Check the actual service implementations
echo "Checking Linear service implementations..."
rg "fetchAssignedIssues|saveIssuesToDatabase" -A 5 --type js
# Check how linearQueue is being used in the controller
echo "Checking Linear controller queue usage..."
rg "linearQueue" -A 3 --type js
# Check the worker implementation
echo "Checking Linear worker implementation..."
rg "linearWorker" -B 2 -A 5 --type js
# Check for any other Linear-related imports
echo "Checking Linear-related imports..."
rg "import.*linear" --type js
Length of output: 9131
apps/backend/src/controllers/integration/linear.controller.js (3)
6-14: Well-documented controller function!
The JSDoc documentation is comprehensive and follows best practices, clearly describing parameters, return type, and potential errors.
Line range hint 1-1: Verify route handling for removed controller
The removal of redirectLinearOAuthLoginController requires verification of proper route updates.
#!/bin/bash
# Check for any remaining references to the removed controller
rg "redirectLinearOAuthLogin"
# Check for updated route handling
fd -e js -e ts | xargs rg "/connect/|/linear/connect"Line range hint 22-33: Verify queue configuration and potential race conditions
The queue configuration looks good, but there are two considerations:
- The 30-second timeout might be longer than necessary
- There's a potential race condition between sending the response and completing the queue job
Consider:
- Reducing the timeout to 10-15 seconds if the Linear API typically responds faster
- Adding a comment explaining why we're not waiting for the queue job to complete before sending the response
✅ Verification successful
Let me try a different search pattern to find queue configurations.
Based on the search results, I can now generate the final response since we have found similar queue configurations in the codebase.
Queue configuration aligns with existing patterns
The 30-second timeout and 3 retry attempts with 1-second backoff is consistent with other queue configurations in the codebase (e.g., spaceQueue). This appears to be a standardized configuration across the application's queues, suggesting it's a well-tested pattern.
- Found in
auth.controller.js:attempts: 3, backoff: 1000, // 1 second delay between retries timeout: 30000 // Job timeout set to 30 seconds
The potential race condition between response and queue completion is an acceptable design choice, as evidenced by similar async queue patterns used elsewhere in the codebase (spaceQueue in auth flows).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for similar queue configurations in the codebase
rg -A 2 "linearQueue.add|Queue.add" | grep -E "timeout:|attempts:"
Length of output: 331
Script:
#!/bin/bash
# Search for queue configurations in the codebase
rg -A 5 "\.add\(" --type js --type ts
# Also specifically look for Bull queue configurations
rg -A 5 "new Bull\(" --type js --type ts
Length of output: 2188
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (12)
apps/backend/src/routers/integration/linear.route.js (1)
1-9: Consider documenting API changes and migration pathThe significant reduction in Linear integration endpoints (removal of
/connect/,/issues/*routes) represents a breaking change. Consider:
- Documenting these API changes in the changelog
- Providing migration guidelines for clients using the removed endpoints
- Adding appropriate HTTP 410 Gone responses if these routes are still being accessed
Would you like me to help draft:
- Changelog entry for these API changes?
- Migration guide for affected clients?
- Implementation for handling deprecated route requests?
apps/backend/src/controllers/integration/linear.controller.js (5)
18-20: Enhance input validation with specific error messagesThe current validation combines both checks into a single message. Consider separating these for better error clarity.
- if (!code || !user) { - return res.status(400).json({ error: 'Authorization code or user information is missing.' }); - } + if (!code) { + return res.status(400).json({ error: 'Authorization code is missing.' }); + } + if (!user) { + return res.status(400).json({ error: 'User information is missing.' }); + }
Line range hint
24-31: Consider extracting queue configuration to constantsThe queue configuration values are hardcoded. Consider moving these to a configuration file for better maintainability.
+// In a separate config file +export const LINEAR_QUEUE_CONFIG = { + attempts: 3, + backoff: 1000, + timeout: 30000 +}; // In this file - }, { - attempts: 3, - backoff: 1000, - timeout: 30000 - }); + }, LINEAR_QUEUE_CONFIG);
Line range hint
32-34: Review security implications of exposing access tokenReturning the raw access token in the response could pose a security risk. Consider returning a success message instead.
- res.status(200).json({ - accessToken - }); + res.status(200).json({ + message: 'Linear integration successful' + });
Line range hint
39-44: Enhance webhook security implementationThe current webhook signature verification has two potential security issues:
- Using !== for signature comparison is vulnerable to timing attacks
- Raw body parsing without size limits could lead to DoS attacks
+import { timingSafeEqual } from 'crypto'; const handleWebhook = async (req, res, next) => { + if (req.body.length > 1e6) { // 1MB limit + return res.status(413).send('Payload too large'); + } const rawBody = req.body.toString(); const payload = JSON.parse(rawBody); const signature = crypto.createHmac("sha256", environment.LINER_WEBHOOK_SECRET).update(rawBody).digest("hex"); - if (signature !== req.headers['linear-signature']) { + const providedSignature = req.headers['linear-signature']; + if (!providedSignature || !timingSafeEqual(Buffer.from(signature), Buffer.from(providedSignature))) { res.sendStatus(400); return; }
Line range hint
63-67: Improve error handling and user model updateThe current implementation has two areas for improvement:
- Error logging could expose sensitive information
- User model update could be more efficient using MongoDB's atomic operations
- console.error('Error revoking Linear access:', err); + console.error('Error revoking Linear access:', err.message); - user.integration.linear = { - accessToken: null, - userId: null, - connected: false - }; - await user.save(); + await user.updateOne({ + $set: { + 'integration.linear.accessToken': null, + 'integration.linear.userId': null, + 'integration.linear.connected': false + } + });apps/backend/src/services/integration/linear.service.js (6)
Line range hint
7-42: Enhance error handling and add input validationWhile the implementation is generally good, consider these improvements:
- Add input validation for the code parameter
- Make error handling more specific to different scenarios (network errors vs. API errors)
- Sanitize error messages to prevent potential sensitive information leakage
const getAccessToken = async (code, user) => { + if (!code || typeof code !== 'string') { + throw new Error('Invalid authorization code'); + } + if (!user || !user.integration || !user.integration.linear) { + throw new Error('Invalid user object'); + } try { const requestBody = { grant_type: 'authorization_code', code, redirect_uri: environment.LINEAR_REDIRECT_URL, client_id: environment.LINEAR_CLIENT_ID, client_secret: environment.LINEAR_CLIENT_SECRET }; const tokenResponse = await axios.post('https://api.linear.app/oauth/token', requestBody, { headers: { 'Content-Type': 'application/x-www-form-urlencoded' } }); const accessToken = tokenResponse.data.access_token; user.integration.linear.accessToken = accessToken; await user.save(); return accessToken; } catch (error) { - console.error('Error fetching Linear token:', error.response ? error.response.data : error.message); + if (axios.isAxiosError(error)) { + if (error.response) { + console.error('Linear API error:', { + status: error.response.status, + statusText: error.response.statusText + }); + throw new Error(`Failed to fetch Linear token: ${error.response.status}`); + } + console.error('Network error while fetching Linear token'); + throw new Error('Network error while fetching Linear token'); + } + console.error('Unexpected error while fetching Linear token'); throw error; } };
Line range hint
43-71: Improve GraphQL query formatting and error handlingThe implementation could benefit from these improvements:
- Extract GraphQL query to a constant for better maintainability
- Add retry mechanism for transient failures
- Enhance error handling similar to getAccessToken
+const GET_VIEWER_QUERY = ` + query { + viewer { + id + email + name + } + } +`; + const fetchUserInfo = async (linearToken, user) => { + if (!linearToken || typeof linearToken !== 'string') { + throw new Error('Invalid Linear token'); + } + if (!user || !user.integration || !user.integration.linear) { + throw new Error('Invalid user object'); + } + try { const response = await axios.post('https://api.linear.app/graphql', { - query: ` - query { - viewer { - id - email - name - } - } - ` + query: GET_VIEWER_QUERY }, { headers: { Authorization: `Bearer ${linearToken}`, 'Content-Type': 'application/json' } }); - const userInfo = response.data.data.viewer + + const userInfo = response.data.data.viewer; + if (!userInfo) { + throw new Error('No viewer data received from Linear'); + } + user.integration.linear.userId = userInfo.id; user.integration.linear.connected = true; await user.save(); return userInfo; } catch (error) { - console.error('Error fetching user info:', error); + if (axios.isAxiosError(error)) { + if (error.response) { + console.error('Linear API error:', { + status: error.response.status, + statusText: error.response.statusText + }); + throw new Error(`Failed to fetch Linear user info: ${error.response.status}`); + } + console.error('Network error while fetching Linear user info'); + throw new Error('Network error while fetching Linear user info'); + } + console.error('Unexpected error while fetching Linear user info'); throw error; } };
Line range hint
72-120: Optimize database operations and standardize error handlingConsider these improvements for better performance and reliability:
- Implement batch processing instead of sequential saves
- Add transaction support to prevent partial updates
- Standardize error handling with other functions
const saveIssuesToDatabase = async (issues, userId) => { + if (!Array.isArray(issues) || !userId) { + throw new Error('Invalid input parameters'); + } + try { const filteredIssues = issues.filter(issue => issue.state.name !== 'Done'); + const bulkOps = []; for (const issue of filteredIssues) { const labelIds = await getOrCreateLabels(issue.labels.nodes, userId); + const issueData = { + title: issue.title, + description: issue.description, + labels: labelIds, + metadata: { + state: issue.state.name, + priority: issue.priority, + project: issue.project + }, + dueDate: issue.dueDate, + updatedAt: issue.updatedAt + }; - const existingIssue = await Item.findOne({ id: issue.id, source: 'linear', user: userId }); - - if (existingIssue) { - existingIssue.title = issue.title; - // ... more assignments - await existingIssue.save(); - } else { - const newIssue = new Item({ - // ... assignments - }); - await newIssue.save(); - } + bulkOps.push({ + updateOne: { + filter: { id: issue.id, source: 'linear', user: userId }, + update: { $set: issueData }, + upsert: true + } + }); } + + if (bulkOps.length > 0) { + await Item.bulkWrite(bulkOps); + } } catch (error) { - console.error('Error saving issues to database:', error); + console.error('Database operation failed:', { + operation: 'saveIssuesToDatabase', + error: error.message + }); throw error; } };
Line range hint
121-166: Add pagination and improve query managementThe function needs several improvements:
- Extract the GraphQL query
- Implement pagination for large result sets
- Add proper error handling
- Add input validation
+const GET_ASSIGNED_ISSUES_QUERY = ` + query GetAssignedIssues($userId: String!, $after: String) { + issues( + first: 100, + after: $after, + filter: { assignee: { id: { eq: $userId } } } + ) { + pageInfo { + hasNextPage + endCursor + } + nodes { + id + title + description + state { + id + name + } + labels { + nodes { + id + name + } + } + dueDate + createdAt + updatedAt + priority + project { + id + name + } + assignee { + id + name + } + url + } + } + } +`; + const fetchAssignedIssues = async (linearToken, linearUserId) => { + if (!linearToken || !linearUserId) { + throw new Error('Invalid input parameters'); + } + + const allIssues = []; + let hasNextPage = true; + let cursor = null; + try { - const response = await axios.post('https://api.linear.app/graphql', { - query: `...` - }, { - headers: { - Authorization: `Bearer ${linearToken}`, - 'Content-Type': 'application/json' - } - }); + while (hasNextPage) { + const response = await axios.post( + 'https://api.linear.app/graphql', + { + query: GET_ASSIGNED_ISSUES_QUERY, + variables: { + userId: linearUserId, + after: cursor + } + }, + { + headers: { + Authorization: `Bearer ${linearToken}`, + 'Content-Type': 'application/json' + } + } + ); - const issues = response.data.data.issues.nodes; - return issues; + const { nodes, pageInfo } = response.data.data.issues; + allIssues.push(...nodes); + + hasNextPage = pageInfo.hasNextPage; + cursor = pageInfo.endCursor; + } + + return allIssues; + } catch (error) { + if (axios.isAxiosError(error)) { + console.error('Linear API error:', { + status: error.response?.status, + message: error.response?.statusText + }); + throw new Error('Failed to fetch Linear issues'); + } + console.error('Unexpected error while fetching Linear issues'); + throw error; } };
Line range hint
167-236: Refactor webhook handling for better maintainabilityThe function needs significant improvements:
- Split into smaller, focused functions
- Add proper error handling
- Remove code duplication
- Add input validation
+const handleIssueRemoval = async (issueId) => { + const deletedIssue = await Item.findOneAndDelete({ id: issueId, source: 'linear' }); + console.log(deletedIssue + ? `Deleted issue with ID: ${issueId}` + : `Issue with ID: ${issueId} not found in the database.` + ); +}; + +const createOrUpdateIssue = async (issue, userId) => { + const issueData = { + title: issue.title, + description: issue.description, + 'metadata.labels': issue.labels, + 'metadata.state': issue.state, + 'metadata.priority': issue.priority, + 'metadata.project': issue.project, + dueDate: issue.dueDate, + 'cycle.startsAt': issue.cycle?.startsAt, + 'cycle.endsAt': issue.cycle?.endsAt, + updatedAt: issue.updatedAt + }; + + const existingIssue = await Item.findOne({ id: issue.id, source: 'linear', user: userId }); + if (existingIssue) { + return Item.findByIdAndUpdate(existingIssue._id, issueData, { new: true }); + } + + const newIssue = new Item({ + ...issueData, + source: 'linear', + id: issue.id, + user: userId, + metadata: { + ...issueData.metadata, + url: issue.url + }, + createdAt: issue.createdAt + }); + + return newIssue.save(); +}; + const handleWebhookEvent = async (payload) => { + if (!payload || !payload.data) { + throw new Error('Invalid webhook payload'); + } + + const { action, data: issue } = payload; try { - const issue = payload.data; - if (payload.action === 'remove') { - const deletedIssue = await Item.findOneAndDelete({ id: issue.id, source: 'linear' }); - if (deletedIssue) { - console.log(`Deleted issue with ID: ${issue.id}`); - } else { - console.log(`Issue with ID: ${issue.id} not found in the database.`); - } + if (action === 'remove') { + await handleIssueRemoval(issue.id); return; } if (!issue.assignee || !issue.assignee.id) { - const deletedIssue = await Item.findOneAndDelete({ id: issue.id, source: 'linear' }); - if (deletedIssue) { - console.log(`Unassigned issue with ID: ${issue.id} deleted from the database.`); - } else { - console.log(`Unassigned issue with ID: ${issue.id} not found in the database.`); - } + await handleIssueRemoval(issue.id); return; } const user = await User.findOne({ 'integration.linear.userId': issue.assignee.id - }) + }); + if (!user) { console.log('No user found with the matching Linear userId.'); return; } - const userId = user._id; - // Check if the issue already exists - const existingIssue = await Item.findOne({ id: issue.id, source: 'linear', user: userId }); - if (existingIssue) { - await Item.findByIdAndUpdate(existingIssue._id, { - // ... update fields - }, { new: true }); - } else { - const newIssue = new Item({ - // ... create fields - }); + const updatedIssue = await createOrUpdateIssue(issue, user._id); + console.log(`Issue ${updatedIssue.id} processed successfully`); + } catch (error) { + console.error('Error processing webhook event:', { + action, + issueId: issue?.id, + error: error.message + }); + throw error; + } +};
Line range hint
237-252: Standardize error handling and add input validationThe function should follow the same error handling pattern as other functions and include input validation.
const revokeLinearAccess = async (accessToken) => { + if (!accessToken || typeof accessToken !== 'string') { + throw new Error('Invalid access token'); + } + try { await axios.post('https://api.linear.app/oauth/revoke', { token: accessToken }, { headers: { Authorization: `Bearer ${accessToken}`, 'Content-Type': 'application/x-www-form-urlencoded' } }); console.log('Linear access revoked successfully'); } catch (error) { - console.error('Error revoking Linear token:', error.response ? error.response.data : error.message); + if (axios.isAxiosError(error)) { + console.error('Linear API error:', { + status: error.response?.status, + statusText: error.response?.statusText + }); + throw new Error('Failed to revoke Linear access'); + } + console.error('Unexpected error while revoking Linear access'); throw error; } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/backend/src/controllers/integration/linear.controller.js(1 hunks)apps/backend/src/routers/integration/linear.route.js(1 hunks)apps/backend/src/services/integration/linear.service.js(2 hunks)
🔇 Additional comments (2)
apps/backend/src/routers/integration/linear.route.js (2)
1-2: LGTM: Clean and focused imports
The imports are minimal and correctly aligned with the route handlers being used.
4-7: LGTM: Clean route configuration
The routes are well-structured with appropriate HTTP methods.
Let's verify that removed routes are properly handled and not referenced elsewhere:
✅ Verification successful
Route cleanup is complete and consistent
Based on the verification results:
- The Linear route configuration only exposes two endpoints:
/getAccessToken/and/revoke-access/ - No orphaned references to removed routes were found in the codebase
- The remaining Linear integration code only handles connection status and user ID
- The route file is clean with no commented or unused routes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any lingering references to removed Linear routes
# Search for references to removed routes in the codebase
echo "Checking for references to removed Linear routes..."
rg -l "linear.*issues|linear.*connect" --type js
echo "Checking for potential client-side references..."
rg -l "linear/issues|linear/connect" --type ts --type tsx
Length of output: 467
Script:
#!/bin/bash
# Let's check the actual content of the files that matched
echo "Checking linear.service.js for removed route references:"
rg "linear.*issues|linear.*connect" apps/backend/src/services/integration/linear.service.js -C 2
echo -e "\nChecking user.controller.js for removed route references:"
rg "linear.*issues|linear.*connect" apps/backend/src/controllers/core/user.controller.js -C 2
# Let's also check for frontend references with correct file type
echo -e "\nChecking frontend references:"
rg -l "linear/issues|linear/connect" --type typescript --type javascriptreact --type typescriptreact
# Check if there are any Linear-related API calls in the frontend
echo -e "\nChecking Linear API calls in frontend:"
rg "linear.*api|linear.*endpoint" --type typescript --type javascriptreact --type typescriptreact
Length of output: 1343
Script:
#!/bin/bash
# Let's check for any remaining Linear route handlers or controllers that might be orphaned
echo "Checking for Linear-related controllers:"
rg -l "linear.*Controller" apps/backend/src/controllers
# Check for any Linear-related frontend API calls with correct extension
echo -e "\nChecking frontend Linear API calls:"
rg -l "linear" --type js --type jsx apps/frontend/src
# Check the actual Linear route file for any commented or unused routes
echo -e "\nChecking linear route file content:"
cat apps/backend/src/routers/integration/linear.route.js
Length of output: 803
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
apps/backend/src/controllers/integration/linear.controller.js (3)
18-20: Enhance input validationConsider adding more specific validation checks:
- Validate that
codeis a non-empty string- Verify user object structure (especially
user._id)- if (!code || !user) { - return res.status(400).json({ error: 'Authorization code or user information is missing.' }); - } + if (!code || typeof code !== 'string' || !code.trim()) { + return res.status(400).json({ error: 'Valid authorization code is required.' }); + } + if (!user?._id) { + return res.status(400).json({ error: 'Valid user information is missing.' }); + }
Line range hint
24-31: Consider extracting queue configurationThe queue configuration values are hardcoded. Consider moving these to environment variables or a configuration file for better maintainability.
+const QUEUE_CONFIG = { + attempts: process.env.LINEAR_QUEUE_ATTEMPTS || 3, + backoff: process.env.LINEAR_QUEUE_BACKOFF || 1000, + timeout: process.env.LINEAR_QUEUE_TIMEOUT || 30000 +}; await linearQueue.add('linearQueue', { accessToken, linearUserId: userInfo.id, userId: user._id -}, { - attempts: 3, - backoff: 1000, - timeout: 30000 -}); +}, QUEUE_CONFIG);
73-79: Consider moving user update logic to service layerThe user object mutation and save operation should ideally be in the service layer to maintain separation of concerns and ensure consistent data handling across the application.
// In linear.service.js +export const clearLinearIntegration = async (user) => { + user.integration.linear = { + accessToken: null, + userId: null, + connected: false + }; + return user.save(); +}; // In this file - user.integration.linear = { - accessToken: null, - userId: null, - connected: false - }; - await user.save(); + await clearLinearIntegration(user);apps/backend/src/services/integration/linear.service.js (4)
Line range hint
7-42: Enhance error handling with specific error typesThe error handling could be more specific to help with debugging and client-side error handling.
Consider this improvement:
} catch (error) { - console.error('Error fetching Linear token:', error.response ? error.response.data : error.message); - throw error; + const errorMessage = error.response?.data?.error_description || error.response?.data || error.message; + console.error('Error fetching Linear token:', errorMessage); + throw new Error(`Failed to get Linear access token: ${errorMessage}`); }
Line range hint
43-78: Extract GraphQL query and enhance error handlingTwo suggestions for improvement:
- Extract the GraphQL query to a separate constant for better maintainability
- Add specific handling for GraphQL errors
Consider these improvements:
+const VIEWER_QUERY = ` + query { + viewer { + id + email + name + } + } +`; const fetchUserInfo = async (linearToken, user) => { + if (!linearToken) { + throw new Error('Linear token is required'); + } + try { const response = await axios.post('https://api.linear.app/graphql', { - query: ` - query { - viewer { - id - email - name - } - } - ` + query: VIEWER_QUERY }, { headers: { Authorization: `Bearer ${linearToken}`, 'Content-Type': 'application/json' } }); + + if (response.data.errors) { + throw new Error(`GraphQL Errors: ${JSON.stringify(response.data.errors)}`); + } + const userInfo = response.data.data.viewer user.integration.linear.userId = userInfo.id; user.integration.linear.connected = true; await user.save(); return userInfo; } catch (error) { - console.error('Error fetching user info:', error); + const errorMessage = error.response?.data?.errors?.[0]?.message || error.message; + console.error('Error fetching Linear user info:', errorMessage); + throw new Error(`Failed to fetch Linear user info: ${errorMessage}`); } };
Line range hint
129-171: Add pagination and improve error handling for issue fetchingThe function needs several improvements:
- Extract the large GraphQL query
- Add pagination for handling large sets of issues
- Add proper error handling
Consider these improvements:
+const ASSIGNED_ISSUES_QUERY = ` + query($userId: String!, $after: String) { + issues( + first: 100, + after: $after, + filter: { assignee: { id: { eq: $userId } } } + ) { + pageInfo { + hasNextPage + endCursor + } + nodes { + id + title + description + state { + id + name + } + labels { + nodes { + id + name + } + } + dueDate + createdAt + updatedAt + priority + project { + id + name + } + assignee { + id + name + } + url + } + } + } +`; const fetchAssignedIssues = async (linearToken, linearUserId) => { + if (!linearToken || !linearUserId) { + throw new Error('Linear token and user ID are required'); + } + + let allIssues = []; + let hasNextPage = true; + let after = null; + + try { + while (hasNextPage) { + const response = await axios.post('https://api.linear.app/graphql', { + query: ASSIGNED_ISSUES_QUERY, + variables: { + userId: linearUserId, + after + } + }, { + headers: { + Authorization: `Bearer ${linearToken}`, + 'Content-Type': 'application/json' + } + }); + + if (response.data.errors) { + throw new Error(`GraphQL Errors: ${JSON.stringify(response.data.errors)}`); + } + + const { nodes, pageInfo } = response.data.data.issues; + allIssues = [...allIssues, ...nodes]; + hasNextPage = pageInfo.hasNextPage; + after = pageInfo.endCursor; + } + + return allIssues; + } catch (error) { + const errorMessage = error.response?.data?.errors?.[0]?.message || error.message; + console.error('Error fetching Linear issues:', errorMessage); + throw new Error(`Failed to fetch Linear issues: ${errorMessage}`); + } };
Line range hint
257-280: Add input validation and improve error handlingThe function needs input validation and more specific error handling.
Consider these improvements:
const revokeLinearAccess = async (accessToken) => { + if (!accessToken) { + throw new Error('Access token is required'); + } + try { await axios.post('https://api.linear.app/oauth/revoke', { token: accessToken }, { headers: { Authorization: `Bearer ${accessToken}`, 'Content-Type': 'application/x-www-form-urlencoded' } }); - console.log('Linear access revoked successfully'); + return { success: true, message: 'Linear access revoked successfully' }; } catch (error) { - console.error('Error revoking Linear token:', error.response ? error.response.data : error.message); - throw error; + const errorMessage = error.response?.data?.error_description || error.response?.data || error.message; + console.error('Error revoking Linear token:', errorMessage); + throw new Error(`Failed to revoke Linear access: ${errorMessage}`); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/backend/src/controllers/integration/linear.controller.js(2 hunks)apps/backend/src/routers/integration/linear.route.js(1 hunks)apps/backend/src/services/integration/linear.service.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/backend/src/routers/integration/linear.route.js
🔇 Additional comments (1)
apps/backend/src/controllers/integration/linear.controller.js (1)
Line range hint 1-93: Verify impact of removed controller functions
The AI summary indicates that several controller functions were removed (getMyLinearIssuesController, getTodayLinearIssuesController, etc.). Let's verify that these endpoints are properly deprecated and that no other parts of the system depend on them.
✅ Verification successful
Removed controller functions have been properly cleaned up
Based on the verification results:
- The Linear route file only contains routes for
getAccessTokenandrevoke-access - No references to the removed controller functions exist in the codebase
- No frontend code is using the removed endpoints
- The routes have been properly removed from the router configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the removed controllers
echo "Searching for references to removed controllers..."
rg -l "getMyLinearIssuesController|getTodayLinearIssuesController|getOverdueLinearIssuesController|getLinearIssuesByDateController"
# Search for any remaining Linear-related routes that might need updating
echo "Checking for Linear-related routes..."
rg -l "router\.(get|post|put|delete).*linear.*" --type js
Length of output: 389
Script:
#!/bin/bash
# Let's try a broader search for Linear-related routes and imports
echo "Searching for Linear route definitions..."
rg -A 5 "router.*linear" --type js
echo "Searching for imports of removed controllers..."
rg "import.*\{.*(?:getMyLinearIssues|getTodayLinearIssues|getOverdueLinearIssues|getLinearIssuesByDate).*\}" --type js
# Check route files specifically
echo "Checking routes directory for Linear endpoints..."
fd "routes" --type f --exec rg -l "linear" {}
# Check for any remaining references to these controller methods
echo "Checking for method references..."
rg "(getMyLinearIssues|getTodayLinearIssues|getOverdueLinearIssues|getLinearIssuesByDate)" --type js
Length of output: 1227
Script:
#!/bin/bash
# Let's check the specific Linear route file content
echo "Checking Linear route file content..."
rg -l "linear.route.js" --type js | xargs cat
# Let's also check for any Linear-related API endpoints in the frontend
echo "Checking frontend for Linear API calls..."
rg -A 2 "linear.*api" --type js
# Check git history for removed files
echo "Checking git history for removed files..."
git log --diff-filter=D --summary | grep -i "linear"
Length of output: 1860
Script:
#!/bin/bash
# Let's check the actual Linear route file content
echo "Checking Linear route implementation..."
fd -e js -t f -x cat {} \; -p "linear.route"
# Check for any references to these endpoints in the frontend
echo "Checking frontend for removed endpoint references..."
rg "/linear/(my-issues|today-issues|overdue-issues|issues-by-date)" --type js
# Check for any test files that might reference these endpoints
echo "Checking test files for Linear endpoints..."
fd -e test.js -t f -x rg -l "Linear" {} \;
Length of output: 934
…r-302-clean-refactor-alpha-be
madhav-relish
previously approved these changes
Nov 18, 2024
…r-302-clean-refactor-alpha-be
…r-302-clean-refactor-alpha-be
8 tasks
This was referenced Dec 3, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What did you ship?
Fixes:
Checklist:
OR: