Conversation
- Change `bun dev` to run desktop, api, and web apps instead of just desktop - Remove redundant `dev:app` script 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughThe PR removes the ClientLogosSection component and its associated CLIENT_LOGOS constant from the marketing app folder, while updating TrustedBySection to use a Marquee-based logo rendering with an expanded data structure. Development scripts in package.json were restructured to apply Turbo filters across api, web, and desktop instead of desktop only, with added marketing-specific script. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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 |
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
![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 (2)
apps/marketing/src/app/components/TrustedBySection/TrustedBySection.tsx (2)
7-31: Extract the logo type definition for better maintainability.The inline type assertion makes the code harder to maintain. Consider extracting the type to a separate definition at the file or module level.
🔎 Proposed refactor
+type ClientLogo = { + name: string; + logo: string; + height: number; + marginTop?: number; + borderRadius?: number; + text?: string; +}; + -const CLIENT_LOGOS = [ +const CLIENT_LOGOS: ClientLogo[] = [ { name: "cadra", logo: "/logos/cadra.png", height: 38, text: "Cadra" }, { name: "onlook", logo: "/logos/onlook.svg", height: 38 }, { name: "ycombinator", logo: "/logos/yc.png", height: 44 }, { name: "scribe", logo: "/logos/scribe.svg", height: 38 }, { name: "adamcad", logo: "/logos/adam.svg", height: 32, marginTop: -5 }, { name: "amazon", logo: "/logos/amazon.png", height: 32, marginTop: 20 }, { name: "google", logo: "/logos/google.svg", height: 32, marginTop: 10 }, { name: "servicenow", logo: "/logos/servicenow.svg", height: 24 }, { name: "mastra", logo: "/logos/mastra.svg", height: 28, text: "Mastra" }, { name: "trainloop", logo: "/logos/trainloop.jpeg", height: 38, borderRadius: 10, text: "Trainloop", }, -] as { - name: string; - logo: string; - height: number; - marginTop?: number; - borderRadius?: number; - text?: string; -}[]; +];
74-78: Remove unnecessary optional chaining.The optional chaining operator on
client?.borderRadiusandclient?.marginTopis unnecessary sinceclientis guaranteed to exist within the map callback.🔎 Proposed fix
style={{ height: client.height, - borderRadius: client?.borderRadius ?? 0, - marginTop: client?.marginTop ?? 0, + borderRadius: client.borderRadius ?? 0, + marginTop: client.marginTop ?? 0, }}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
apps/marketing/public/logos/adam.pngis excluded by!**/*.pngapps/marketing/public/logos/adam.svgis excluded by!**/*.svgapps/marketing/public/logos/amazon.pngis excluded by!**/*.pngapps/marketing/public/logos/cadra.pngis excluded by!**/*.pngapps/marketing/public/logos/google.svgis excluded by!**/*.svgapps/marketing/public/logos/mastra.svgis excluded by!**/*.svgapps/marketing/public/logos/onlook.svgis excluded by!**/*.svgapps/marketing/public/logos/scribe.svgis excluded by!**/*.svgapps/marketing/public/logos/servicenow.svgis excluded by!**/*.svgapps/marketing/public/logos/trainloop.jpegis excluded by!**/*.jpegapps/marketing/public/logos/yc.pngis excluded by!**/*.png
📒 Files selected for processing (5)
apps/marketing/src/app/components/ClientLogosSection/ClientLogosSection.tsx(0 hunks)apps/marketing/src/app/components/ClientLogosSection/constants.ts(0 hunks)apps/marketing/src/app/components/ClientLogosSection/index.ts(0 hunks)apps/marketing/src/app/components/TrustedBySection/TrustedBySection.tsx(2 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (3)
- apps/marketing/src/app/components/ClientLogosSection/index.ts
- apps/marketing/src/app/components/ClientLogosSection/ClientLogosSection.tsx
- apps/marketing/src/app/components/ClientLogosSection/constants.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid using any type in TypeScript - maintain type safety unless absolutely necessary
Files:
apps/marketing/src/app/components/TrustedBySection/TrustedBySection.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run Biome for formatting, linting, import organization, and safe fixes at the root level using bun run lint:fix
Files:
apps/marketing/src/app/components/TrustedBySection/TrustedBySection.tsx
**/{components,features}/**/[!.]*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Organize project structure with one folder per component: ComponentName/ComponentName.tsx with index.ts barrel export
Files:
apps/marketing/src/app/components/TrustedBySection/TrustedBySection.tsx
**/{components,features}/**/*.{ts,tsx,test.ts,test.tsx,stories.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Files:
apps/marketing/src/app/components/TrustedBySection/TrustedBySection.tsx
**/*.{tsx,css}
📄 CodeRabbit inference engine (AGENTS.md)
Use React + TailwindCSS v4 + shadcn/ui for UI development
Files:
apps/marketing/src/app/components/TrustedBySection/TrustedBySection.tsx
**/{components,features}/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
**/{components,features}/**/*.tsx: Nest components in parent's components/ folder if used only once, promote to highest shared parent's components/ if used 2+ times
Use one component per file - do not combine multiple components in a single file
Files:
apps/marketing/src/app/components/TrustedBySection/TrustedBySection.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Docs
- GitHub Check: Deploy API
- GitHub Check: Deploy Web
- GitHub Check: Build
🔇 Additional comments (4)
package.json (1)
18-22: LGTM! Improved developer experience.The expanded
devscript now runs API, web, and desktop together, streamlining the development workflow. The newdev:marketingscript enables isolated marketing app development, which is helpful for focused work on that package.apps/marketing/src/app/components/TrustedBySection/TrustedBySection.tsx (3)
33-54: LGTM! Well-structured component layout.The motion wrapper and section structure follow good practices with appropriate animation timing and viewport detection.
71-72: Verify aspect ratios with fixed width.Setting a fixed
width={160}while heights vary (24-44px) may cause aspect ratio distortion if the original logos don't match these dimensions. Next.js Image will maintain aspect ratio withobject-contain, but verify visually that logos aren't stretched or compressed.Consider using only the
heightprop and letting Next.js calculate the width automatically to preserve natural aspect ratios:<Image src={client.logo} alt={client.name} - width={160} - height={client.height} + height={client.height} + width={0} className="object-contain w-auto" style={{ height: client.height, borderRadius: client.borderRadius ?? 0, marginTop: client.marginTop ?? 0, }} />Or remove both width and height from props and control via CSS only if the images are static assets that Next.js can analyze at build time.
4-5: All dependencies and logo assets are present. Thereact-fast-marqueepackage (v^1.6.5) is installed, and all 10 logo files are present in the public/logos directory.
| key={setId} | ||
| className="flex gap-12 sm:gap-16 md:gap-24 items-center" | ||
| key={client.name} | ||
| className="flex items-center justify-center opacity-60 hover:opacity-100 transition-opacity cursor-pointer whitespace-nowrap h-14 gap-2" |
There was a problem hiding this comment.
Remove misleading cursor-pointer class.
The cursor-pointer class suggests interactivity, but there's no click handler attached to these logo elements. This creates a misleading user experience.
🔎 Proposed fix
<div
key={client.name}
- className="flex items-center justify-center opacity-60 hover:opacity-100 transition-opacity cursor-pointer whitespace-nowrap h-14 gap-2"
+ className="flex items-center justify-center opacity-60 hover:opacity-100 transition-opacity whitespace-nowrap h-14 gap-2"
>📝 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.
| className="flex items-center justify-center opacity-60 hover:opacity-100 transition-opacity cursor-pointer whitespace-nowrap h-14 gap-2" | |
| <div | |
| key={client.name} | |
| className="flex items-center justify-center opacity-60 hover:opacity-100 transition-opacity whitespace-nowrap h-14 gap-2" | |
| > |
🤖 Prompt for AI Agents
In apps/marketing/src/app/components/TrustedBySection/TrustedBySection.tsx
around line 66, the element includes the CSS class "cursor-pointer" which
falsely indicates interactive behavior; remove "cursor-pointer" from the
className string (or if these logos should be interactive, add an appropriate
click handler and keyboard accessibility attributes instead). Update the
className to drop only "cursor-pointer" and run a quick visual/accessibility
check to confirm cursor and focus behavior are now correct.
| <Image | ||
| src={client.logo} | ||
| alt={client.name} | ||
| width={160} | ||
| height={client.height} | ||
| className="object-contain w-auto" |
There was a problem hiding this comment.
Improve alt text for better accessibility.
The alt text currently uses lowercase identifiers (e.g., "cadra", "onlook") which are not descriptive for screen readers. Consider using more descriptive text like "Cadra logo" or storing a proper display name in the CLIENT_LOGOS array.
🔎 Proposed fix
Option 1: Add displayName to CLIENT_LOGOS and use it for alt text:
const CLIENT_LOGOS: ClientLogo[] = [
- { name: "cadra", logo: "/logos/cadra.png", height: 38, text: "Cadra" },
+ { name: "cadra", displayName: "Cadra", logo: "/logos/cadra.png", height: 38, text: "Cadra" },
// ... update other entries
];
<Image
src={client.logo}
- alt={client.name}
+ alt={`${client.displayName} logo`}
width={160}
height={client.height}
className="object-contain w-auto"
style={{
height: client.height,
borderRadius: client.borderRadius ?? 0,
marginTop: client.marginTop ?? 0,
}}
/>Option 2: Use text field if available, fallback to formatted name:
<Image
src={client.logo}
- alt={client.name}
+ alt={`${client.text || client.name.charAt(0).toUpperCase() + client.name.slice(1)} logo`}
width={160}
height={client.height}
className="object-contain w-auto"
style={{
height: client.height,
borderRadius: client.borderRadius ?? 0,
marginTop: client.marginTop ?? 0,
}}
/>📝 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.
| <Image | |
| src={client.logo} | |
| alt={client.name} | |
| width={160} | |
| height={client.height} | |
| className="object-contain w-auto" | |
| <Image | |
| src={client.logo} | |
| alt={`${client.text || client.name.charAt(0).toUpperCase() + client.name.slice(1)} logo`} | |
| width={160} | |
| height={client.height} | |
| className="object-contain w-auto" |
🤖 Prompt for AI Agents
In apps/marketing/src/app/components/TrustedBySection/TrustedBySection.tsx
around lines 68 to 73, the Image alt prop uses raw lowercase identifiers
(client.name) which are not descriptive for screen readers; update alt text to a
human-friendly label by using a displayName property on each CLIENT_LOGOS entry
(e.g., "Cadra logo") or, if no displayName exists, derive a fallback like
`${client.name} logo` with proper capitalization; change the data source or
construct the formatted string at render time and ensure all CLIENT_LOGOS
entries include the displayName to keep alt text consistent and accessible.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.