-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Update branding assets and dependencies #75
Conversation
- Replace favicon and app icons with new design - Update web manifest with maskable icons - Add AWS S3 and Lodash type dependencies - Modify layout and component styling - Seed evidence requirements in database
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request updates several front-end assets and components alongside a back-end seed script. The web manifest files for both apps have been completely replaced with new JSON structures (including updated names and icon paths). In addition, unused font imports have been removed and replaced in layout files, several UI components have small styling or icon modifications, and a new asynchronous database seeding function for evidence data has been introduced to enhance the initialization routine. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as main()
participant Seed as seedEvidence()
participant DB as Prisma
Main->>Seed: Call seedEvidence()
Seed->>DB: Query for evidence requirements
DB-->>Seed: Return evidence list
loop For each evidence requirement
Seed->>DB: Upsert evidence record
DB-->>Seed: Upsert result
end
Seed-->>Main: Seeding complete
Possibly related PRs
Poem
✨ Finishing Touches
🪧 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/db/prisma/seed.js (2)
6-7
: Combine duplicate imports from @prisma/client.Consider combining both imports into a single import statement to improve code maintainability.
-const client_1 = require("@prisma/client"); -const client_2 = require("@prisma/client"); +const { PrismaClient, RequirementType } = require("@prisma/client");
284-308
: Consider performance and data integrity improvements.While the implementation is functional, consider the following improvements:
- Use
prisma.$transaction
for batch processing of upserts to improve performance- Add data validation beyond Prisma's built-in checks
- Consider cleaning up orphaned evidence records
- Add try-catch blocks for error handling within the function
Here's a suggested implementation with these improvements:
async function seedEvidence() { + try { const evidenceRequirements = await prisma.controlRequirement.findMany({ where: { type: client_2.RequirementType.evidence, }, }); console.log(`🔄 Processing ${evidenceRequirements.length} evidences`); + + // Prepare batch operations + const operations = evidenceRequirements.map(evidence => { + // Basic validation + if (!evidence.name?.trim()) { + console.log(` ⚠️ Skipping evidence with empty name: ${evidence.id}`); + return null; + } + + return prisma.evidence.upsert({ + where: { id: evidence.id }, + update: { + name: evidence.name, + description: evidence.description, + }, + create: { + id: evidence.id, + name: evidence.name, + description: evidence.description, + }, + }); + }).filter(Boolean); + + // Execute batch operations + await prisma.$transaction(operations); + + // Clean up orphaned records + await prisma.evidence.deleteMany({ + where: { + id: { + notIn: evidenceRequirements.map(e => e.id) + } + } + }); + } catch (error) { + console.error(`❌ Error seeding evidence:`, error); + throw error; // Re-throw to be caught by main's error handler + } }apps/app/src/components/upload-dialog.tsx (1)
59-61
: Consider keeping the file count display.While the Cloud icon addition improves visual clarity, removing the file count might reduce user feedback. Consider keeping both the icon and file count.
<Button variant="action"> <Cloud className="h-4 w-4" /> {t("common.attachments.upload")} + {files.length > 0 && ` (${files.length})`} </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (27)
apps/app/public/android-chrome-192x192.png
is excluded by!**/*.png
apps/app/public/android-chrome-512x512.png
is excluded by!**/*.png
apps/app/public/apple-touch-icon.png
is excluded by!**/*.png
apps/app/public/favicon-16x16.png
is excluded by!**/*.png
apps/app/public/favicon-32x32.png
is excluded by!**/*.png
apps/app/public/favicon-96x96.png
is excluded by!**/*.png
apps/app/public/favicon.ico
is excluded by!**/*.ico
apps/app/public/favicon.svg
is excluded by!**/*.svg
apps/app/public/web-app-manifest-192x192.png
is excluded by!**/*.png
apps/app/public/web-app-manifest-512x512.png
is excluded by!**/*.png
apps/web/public/android-chrome-192x192.png
is excluded by!**/*.png
apps/web/public/android-chrome-512x512.png
is excluded by!**/*.png
apps/web/public/apple-touch-icon.png
is excluded by!**/*.png
apps/web/public/comp-logo-black.png
is excluded by!**/*.png
apps/web/public/comp-logo-white.png
is excluded by!**/*.png
apps/web/public/favicon-16x16.png
is excluded by!**/*.png
apps/web/public/favicon-32x32.png
is excluded by!**/*.png
apps/web/public/favicon-96x96.png
is excluded by!**/*.png
apps/web/public/favicon.ico
is excluded by!**/*.ico
apps/web/public/favicon.svg
is excluded by!**/*.svg
apps/web/public/fonts/GeneralSans-Variable.ttf
is excluded by!**/*.ttf
apps/web/public/fonts/GeneralSans-VariableItalic.ttf
is excluded by!**/*.ttf
apps/web/public/logo.png
is excluded by!**/*.png
apps/web/public/web-app-manifest-192x192.png
is excluded by!**/*.png
apps/web/public/web-app-manifest-512x512.png
is excluded by!**/*.png
bun.lock
is excluded by!**/*.lock
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (9)
apps/app/public/site.webmanifest
(1 hunks)apps/app/src/app/[locale]/layout.tsx
(0 hunks)apps/app/src/components/risks/risk-comments.tsx
(1 hunks)apps/app/src/components/risks/tasks/task-attachment.tsx
(1 hunks)apps/app/src/components/risks/tasks/task-comments.tsx
(1 hunks)apps/app/src/components/upload-dialog.tsx
(2 hunks)apps/web/public/site.webmanifest
(1 hunks)apps/web/src/app/layout.tsx
(2 hunks)packages/db/prisma/seed.js
(3 hunks)
💤 Files with no reviewable changes (1)
- apps/app/src/app/[locale]/layout.tsx
✅ Files skipped from review due to trivial changes (2)
- apps/app/src/components/risks/risk-comments.tsx
- apps/app/src/components/risks/tasks/task-attachment.tsx
🔇 Additional comments (9)
packages/db/prisma/seed.js (1)
38-40
: LGTM!The changes maintain consistency with the existing code structure and include appropriate logging.
apps/app/public/site.webmanifest (1)
1-21
:❓ Verification inconclusive
Verify that the web app manifest icons exist.
The manifest structure looks good and aligns with PWA standards. Let's verify the existence and dimensions of the new icon files.
🏁 Script executed:
#!/bin/bash # Description: Check if the new web app manifest icons exist and have correct dimensions # Note: Using ImageMagick's identify command to check image dimensions for icon in "web-app-manifest-192x192.png" "web-app-manifest-512x512.png"; do if [ -f "apps/app/public/$icon" ]; then echo "✓ Found $icon" identify "apps/app/public/$icon" else echo "✗ Missing $icon" fi doneLength of output: 743
Manual Verification Required: Icon Dimensions
The manifest file’s structure is correct, and both icon files are present at the expected locations. However, the automated check for image dimensions couldn’t be completed because the ImageMagick
identify
command isn’t available in this environment. Please manually verify that:
apps/app/public/web-app-manifest-192x192.png
is exactly 192x192 pixels.apps/app/public/web-app-manifest-512x512.png
is exactly 512x512 pixels.Once you confirm the image dimensions (using a locally-installed version of ImageMagick or another reliable tool), you can update this review comment accordingly.
apps/app/src/components/risks/tasks/task-comments.tsx (1)
31-32
: LGTM! Icon addition enhances button visibility.The addition of the MessageSquare icon improves the visual hierarchy and provides better context for the action button.
apps/app/src/components/upload-dialog.tsx (1)
18-18
: LGTM! New icon import.The Cloud icon import aligns with the UI enhancement goals.
apps/web/public/site.webmanifest (1)
1-21
:❓ Verification inconclusive
Verify that the web app manifest icons exist.
The manifest structure is consistent with the app manifest. Let's verify the existence and dimensions of the new icon files.
🏁 Script executed:
#!/bin/bash # Description: Check if the new web app manifest icons exist and have correct dimensions # Note: Using ImageMagick's identify command to check image dimensions for icon in "web-app-manifest-192x192.png" "web-app-manifest-512x512.png"; do if [ -f "apps/web/public/$icon" ]; then echo "✓ Found $icon" identify "apps/web/public/$icon" else echo "✗ Missing $icon" fi doneLength of output: 743
Manifest Icon Verification: Manual Dimensions Check Required
- The manifest file references two icons located at
apps/web/public/web-app-manifest-192x192.png
andapps/web/public/web-app-manifest-512x512.png
, and the existence of these files has been confirmed.- The automated script attempted to verify dimensions using ImageMagick's
identify
command but encountered a "command not found" error.- Action: Please manually confirm that the image dimensions are indeed 192x192 and 512x512. You may run a Python script with Pillow (if available) or verify using a local environment where the necessary image tools are installed.
apps/web/src/app/layout.tsx (4)
1-1
: LGTM! Good choice using local fonts.The shift to local fonts improves performance, reliability, and GDPR compliance by eliminating external font requests.
Also applies to: 9-9
35-39
: LGTM! Clean font class implementation.The className implementation correctly combines both font variables using template literals.
19-23
:✅ Verification successful
Verify font file existence.
The font configuration is well-structured with variable font support and proper loading strategy.
Let's verify the font file:
🏁 Script executed:
#!/bin/bash # Description: Check if the font file exists fd "GeneralSans-Variable.ttf" apps/web/public/fonts/Length of output: 99
Font configuration verified and approved.
The font file
apps/web/public/fonts/GeneralSans-Variable.ttf
exists as expected, and the configuration inapps/web/src/app/layout.tsx
(lines 19-23) is correctly set up with variable font support and theswap
display for optimized loading.
14-16
:❓ Verification inconclusive
Verify favicon size and manifest consistency.
The favicon size of 96x96 pixels might be insufficient for modern web standards. Consider using multiple sizes for better device compatibility.
Let's verify the icon setup:
🏁 Script executed:
#!/bin/bash # Description: Check favicon existence and web manifest consistency # Check if favicon exists fd "favicon-96x96.png" apps/web/public/ # Check web manifest for icon definitions fd "site.webmanifest" apps/web/public/ --exec cat {}Length of output: 564
Icon Update Verification – Please Review Consistency Across Icon Assets
- The updated favicon (
/favicon-96x96.png
) is confirmed to exist inapps/web/public/
.- The web manifest (
apps/web/public/site.webmanifest
) still defines only larger icons (192x192
and512x512
).- Verify that using a single 96x96 icon in the layout meets all device and browser requirements. If broader compatibility is desired, consider either:
- Including additional favicon sizes (e.g., 16x16, 32x32) alongside the 96x96 icon, or
- Updating the manifest to reference the new favicon if applicable.
Summary by CodeRabbit
New Features
Style