Fix repo images in authed instance case and add manifest json #332
Conversation
WalkthroughThis update centralizes and refactors token retrieval logic by introducing a new Changes
Sequence Diagram(s)sequenceDiagram
participant WebClient
participant WebAPI
participant Actions
participant Crypto
participant DB
WebClient->>WebAPI: GET /api/[domain]/repos/[repoId]/image
WebAPI->>Actions: getRepoImage(repoId, domain)
Actions->>DB: fetch repo by id/org
Actions->>Crypto: getTokenFromConfig(token, orgId, db)
Crypto->>DB: fetch secret (if needed)
Crypto-->>Actions: token string
Actions->>ExternalHost: fetch image with Authorization (if self-hosted)
ExternalHost-->>Actions: image data
Actions-->>WebAPI: image response
WebAPI-->>WebClient: image data or redirect
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/web/public/manifest.json (1)
1-14: Consider enhancing the PWA manifest for better user experience.While the current manifest is functional, consider adding these standard PWA properties for a more complete implementation:
{ "name": "Sourcebot", "short_name": "Sourcebot", + "description": "AI-powered code analysis and review platform", "display": "standalone", "start_url": "/", + "theme_color": "#000000", + "background_color": "#ffffff", + "orientation": "portrait-primary", "icons": [ + { + "src": "/logo_192.png", + "sizes": "192x192", + "type": "image/png" + }, { "src": "/logo_512.png", "sizes": "512x512", "type": "image/png" } ] }packages/web/src/app/api/[domain]/repos/[repoId]/image/route.ts (1)
5-23: Consider adding comprehensive error handling.The implementation is solid but could benefit from additional error handling:
- The direct return of
resultassumesgetRepoImagealways returns a proper Response object- No try-catch block to handle unexpected errors from the
getRepoImageaction- Consider validating that
repoIdNumis positiveexport async function GET( request: NextRequest, { params }: { params: { domain: string; repoId: string } } ) { const { domain, repoId } = params; const repoIdNum = parseInt(repoId); - if (isNaN(repoIdNum)) { + if (isNaN(repoIdNum) || repoIdNum <= 0) { return new Response("Invalid repo ID", { status: 400 }); } - const result = await getRepoImage(repoIdNum, domain); + try { + const result = await getRepoImage(repoIdNum, domain); - if (isServiceError(result)) { - return new Response(result.message, { status: result.statusCode }); - } + if (isServiceError(result)) { + return new Response(result.message, { status: result.statusCode }); + } - return result; + return result; + } catch (error) { + console.error('Error fetching repo image:', error); + return new Response("Internal server error", { status: 500 }); + } }packages/crypto/src/tokenUtils.ts (1)
5-33: Consider adding defensive error handling.The function is well-structured but could benefit from additional error handling:
- No validation that
orgIdis positive- Database query errors aren't explicitly handled
- The
decryptfunction could throw errors that aren't caughtexport const getTokenFromConfig = async (token: Token, orgId: number, db: PrismaClient) => { + if (orgId <= 0) { + throw new Error('Invalid organization ID'); + } + if ('secret' in token) { const secretKey = token.secret; - const secret = await db.secret.findUnique({ - where: { - orgId_key: { - key: secretKey, - orgId + try { + const secret = await db.secret.findUnique({ + where: { + orgId_key: { + key: secretKey, + orgId + } } - } - }); + }); + } catch (error) { + throw new Error(`Failed to query secret: ${error instanceof Error ? error.message : 'Unknown error'}`); + } if (!secret) { throw new Error(`Secret with key ${secretKey} not found for org ${orgId}`); } - const decryptedToken = decrypt(secret.iv, secret.encryptedValue); + try { + const decryptedToken = decrypt(secret.iv, secret.encryptedValue); + } catch (error) { + throw new Error(`Failed to decrypt token: ${error instanceof Error ? error.message : 'Unknown error'}`); + } return decryptedToken;packages/web/src/lib/utils.ts (1)
413-440: Improve input validation and edge case handling.The function logic is sound but could be more robust:
- Handle empty string imageUrl case
- Consider validating domain parameter
- The public hostnames list might need to include more GitLab/Gitea instances
export const getRepoImageSrc = (imageUrl: string | undefined, repoId: number, domain: string): string | undefined => { - if (!imageUrl) return undefined; + if (!imageUrl || imageUrl.trim() === '') return undefined; + + if (!domain || domain.trim() === '') { + console.warn('Invalid domain provided to getRepoImageSrc'); + return imageUrl; + } try { const url = new URL(imageUrl); // List of known public instances that don't require authentication const publicHostnames = [ 'github.com', 'gitlab.com', 'avatars.githubusercontent.com', 'gitea.com', 'bitbucket.org', + // Add more common public instances as needed ]; const isPublicInstance = publicHostnames.includes(url.hostname); if (isPublicInstance) { return imageUrl; } else { // Use the proxied route for self-hosted instances return `/api/${domain}/repos/${repoId}/image`; } } catch { // If URL parsing fails, use the original URL + console.warn('Failed to parse image URL:', imageUrl); return imageUrl; } };Consider making the public hostnames configurable.
The hardcoded list of public hostnames might need updates over time. Consider making this configurable or moving it to a constants file.
packages/web/src/actions.ts (2)
1756-1783: Consider refactoring the repetitive token retrieval logic.The token retrieval logic for different connection types follows a similar pattern. Consider extracting this into a helper function to reduce duplication and improve maintainability.
+const getAuthHeadersForConnection = async (connection: any, orgId: number, prisma: PrismaClient): Promise<Record<string, string>> => { + const config = connection.config as any; + if (!config.token) return {}; + + try { + const token = await getTokenFromConfig(config.token, orgId, prisma); + + switch (connection.connectionType) { + case 'github': + case 'gitea': + return { 'Authorization': `token ${token}` }; + case 'gitlab': + return { 'Authorization': `Bearer ${token}` }; + default: + return {}; + } + } catch (error) { + logger.warn(`Failed to get token for connection ${connection.id}:`, error); + return {}; + } +}; let authHeaders: Record<string, string> = {}; for (const { connection } of repo.connections) { - try { - if (connection.connectionType === 'github') { - const config = connection.config as unknown as GithubConnectionConfig; - if (config.token) { - const token = await getTokenFromConfig(config.token, connection.orgId, prisma); - authHeaders['Authorization'] = `token ${token}`; - break; - } - } else if (connection.connectionType === 'gitlab') { - const config = connection.config as unknown as GitlabConnectionConfig; - if (config.token) { - const token = await getTokenFromConfig(config.token, connection.orgId, prisma); - authHeaders['Authorization'] = `Bearer ${token}`; - break; - } - } else if (connection.connectionType === 'gitea') { - const config = connection.config as unknown as GiteaConnectionConfig; - if (config.token) { - const token = await getTokenFromConfig(config.token, connection.orgId, prisma); - authHeaders['Authorization'] = `token ${token}`; - break; - } - } - } catch (error) { - logger.warn(`Failed to get token for connection ${connection.id}:`, error); - } + const headers = await getAuthHeadersForConnection(connection, connection.orgId, prisma); + if (Object.keys(headers).length > 0) { + authHeaders = headers; + break; + } }
1742-1748: Extract public hostnames to a constant.Consider moving the public hostnames list to a constant for better maintainability and potential reuse.
+const PUBLIC_CODE_HOST_DOMAINS = [ + 'github.com', + 'gitlab.com', + 'avatars.githubusercontent.com', + 'gitea.com', + 'bitbucket.org', +] as const; // Only proxy images from self-hosted instances that might require authentication const imageUrl = new URL(repo.imageUrl); -const publicHostnames = [ - 'github.com', - 'gitlab.com', - 'avatars.githubusercontent.com', - 'gitea.com', - 'bitbucket.org', -]; -const isPublicInstance = publicHostnames.includes(imageUrl.hostname); +const isPublicInstance = PUBLIC_CODE_HOST_DOMAINS.includes(imageUrl.hostname);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/web/public/logo_512.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
packages/backend/src/utils.ts(2 hunks)packages/crypto/package.json(1 hunks)packages/crypto/src/index.ts(1 hunks)packages/crypto/src/tokenUtils.ts(1 hunks)packages/crypto/tsconfig.json(2 hunks)packages/web/public/manifest.json(1 hunks)packages/web/src/actions.ts(2 hunks)packages/web/src/app/[domain]/connections/[id]/components/repoListItem.tsx(2 hunks)packages/web/src/app/[domain]/repos/columns.tsx(2 hunks)packages/web/src/app/[domain]/repos/repositoryTable.tsx(2 hunks)packages/web/src/app/api/[domain]/repos/[repoId]/image/route.ts(1 hunks)packages/web/src/app/layout.tsx(1 hunks)packages/web/src/lib/utils.ts(1 hunks)packages/web/src/middleware.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (13)
packages/web/src/middleware.ts (1)
32-32: LGTM! Proper static asset exclusion for PWA files.The updated matcher correctly excludes the PWA manifest and icon files from middleware processing, ensuring they're served as static assets without interference.
packages/crypto/package.json (1)
11-12: LGTM! Appropriate dependencies for centralized token utilities.Adding the db and schemas dependencies supports the new centralized token utility functions, improving code organization and maintainability across the monorepo.
packages/crypto/src/index.ts (1)
67-68:✅ Verification successful
Verify the module path and ensure tokenUtils exists.
The export looks good for centralizing token retrieval logic. However, please confirm that:
- The
tokenUtils.jsfile exists (or if this should referencetokenUtils.ts)- The
.jsextension is correct for Node16 module resolution
🏁 Script executed:
#!/bin/bash # Description: Verify tokenUtils file exists and the export is correct # Expected: tokenUtils file should exist and export getTokenFromConfig # Check if tokenUtils file exists fd "tokenUtils" packages/crypto/src/ # Verify getTokenFromConfig is exported from tokenUtils rg -A 5 "export.*getTokenFromConfig" packages/crypto/src/Length of output: 684
tokenUtils export path and .js extension validated
Confirmed correct file and export; the
.jsextension aligns with Node16 ES module resolution and matches the compiled output.•
packages/crypto/src/tokenUtils.tsexists and exportsgetTokenFromConfig
• Including.jsin the import is required for ESM under Node16No further changes required.
packages/crypto/tsconfig.json (1)
3-19: LGTM! TypeScript configuration modernization.The updates to ES2022/ES2023 and Node16 module resolution are appropriate for supporting modern ES modules and align with the new token utilities export using
.jsextensions.packages/web/src/app/[domain]/connections/[id]/components/repoListItem.tsx (2)
3-3: LGTM! Centralizing image URL logic.Good refactoring to centralize repository image URL resolution logic.
56-58: LGTM! Consistent image rendering logic.The conditional rendering and image source usage are correctly updated to use the centralized
imageSrc.packages/web/src/app/[domain]/repos/repositoryTable.tsx (2)
28-28: LGTM! Adding repoId for image proxying support.Good addition of
repoIdto support the new centralized image handling functionality.
39-39:✅ Verification successful
Verify repoId is always available in repo data.
The mapping looks correct. Please confirm that
repo.repoIdis always present and properly typed in the repository data structure.
🏁 Script executed:
#!/bin/bash # Description: Verify repoId is properly defined in repository types # Expected: repoId should be a required field in repository interfaces # Check repository type definitions rg -A 10 -B 5 "interface.*[Rr]epo" packages/web/src/ rg -A 10 -B 5 "type.*[Rr]epo" packages/web/src/ # Verify repoId field in database schemas rg "repoId" packages/db/src/Length of output: 14732
🏁 Script executed:
#!/bin/bash # Verify that repoId is required in the repositoryQuerySchema rg -A20 -B5 "repositoryQuerySchema" packages/web/src/lib/types.tsLength of output: 753
🏁 Script executed:
#!/bin/bash # Show the definition of repositoryQuerySchema and verify repoId field rg -C5 "repositoryQuerySchema" packages/web/src/libLength of output: 1521
🏁 Script executed:
#!/bin/bash # Locate repositoryTable.tsx and display its prop types and mapping context file=$(rg -l "repositoryTable.tsx" packages/web/src) echo "Found file: $file" sed -n '1,200p' "$file"Length of output: 151
🏁 Script executed:
#!/bin/bash # Locate all repos directories under src/app and list their contents find packages/web/src/app -type d -name repos | while read dir; do echo "==> Directory: $dir" ls -1 "$dir" doneLength of output: 674
Confirmed:
repoIdis always present and correctly typed
repoIdis defined as a requirednumberin the ZodrepositoryQuerySchema(packages/web/src/lib/schemas.ts) and flows through to every component prop type:
- RetryRepoIndexButtonProps (repoId: number)
- RepoListItemProps (repoId: number)
- RepositoryColumnInfo (repoId: number)
No further changes needed.
packages/web/src/app/[domain]/repos/columns.tsx (3)
9-9: LGTM!The import is correctly added for the new utility function.
15-15: LGTM!The type extension correctly adds the required
repoIdfield for the new image source utility.
116-116: LGTM!The image source correctly uses the new utility function to handle both public and self-hosted repository images with appropriate fallback.
packages/backend/src/utils.ts (1)
5-5: Good refactoring to centralize token retrieval logic.The delegation to
getTokenFromConfigBasefrom the crypto package is a clean architectural improvement that promotes code reuse while maintaining backend-specific error handling.Also applies to: 27-41
packages/web/src/actions.ts (1)
1718-1810: Well-implemented image proxy with proper authentication handling.The implementation correctly handles both public and private repository images, includes appropriate error handling, sets proper cache headers, and respects security boundaries. The guest access allowance is appropriate for image viewing.
packages/web/src/app/[domain]/connections/[id]/components/repoListItem.tsx
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
Improvements
Refactor
Chores