Feat : Meetup visual save the date with speakers and topic #485
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughA new 1920×1080 React asset generator was added at Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Renderer as ScheduleShort Renderer
participant Fetchers as Data Fetchers
participant Composer as Composer/Frame
Client->>Renderer: call renderer({ params: { id } })
Renderer->>Fetchers: getEventData(id)
Renderer->>Fetchers: getAstroImageBase64(post.cover)
Renderer->>Fetchers: fetch co-organizers logos
Renderer->>Fetchers: getTalkData (up to 3) + speaker avatars
Fetchers-->>Renderer: event, images, talks, avatars
Renderer->>Composer: assemble Frame (1920×1080)
Composer->>Composer: render BgImage, left info column
Composer->>Composer: render talks list, avatars, footer
Composer-->>Renderer: JSX image
Renderer-->>Client: return rendered asset
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pages/events/[id]/assets/_schedule-short.tsx (1)
176-255: Consider handling for empty talks array.When
talksis empty (fewer than 3 talks or none at all), the right panel will render nothing. This might create an unbalanced layout.Consider adding a conditional message or adjusting the layout when no talks are available:
> + {talks.length === 0 ? ( + <div style={{ + fontSize: 36, + opacity: 0.7, + fontStyle: 'italic' + }}> + Schedule coming soon + </div> + ) : ( {await Promise.all( talks.map(async (talk) => ( // ... existing talk rendering )), )} + )} </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pages/events/[id]/assets/_schedule-short.tsx(1 hunks)src/routes.gen.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/events/[id]/assets/_schedule-short.tsx (7)
src/generated-assets/image.ts (2)
AssetImageConfig(11-15)getAstroImageBase64(171-174)src/pages/events/[id]/talks/[talkId]/assets/_utils.ts (1)
getTalkData(4-30)src/generated-assets/components/Frame.tsx (1)
Frame(4-35)src/generated-assets/components/BgImage.tsx (1)
BgImage(1-63)src/generated-assets/components/LogoWithFriends.tsx (1)
LogoWithFriends(6-63)src/generated-assets/theme.ts (1)
COLORS(1-6)src/lib/events.ts (2)
getEventDisplayType(329-336)getEventDisplayDate(315-328)
🔇 Additional comments (8)
src/routes.gen.ts (1)
36-36: LGTM! Auto-generated route entry is correctly added.The new
_schedule-shortroute is properly integrated into the assets configuration and follows the existing pattern for similar asset routes.src/pages/events/[id]/assets/_schedule-short.tsx (7)
1-12: LGTM! Imports are well-organized.All necessary components, utilities, and assets are properly imported with clear naming.
14-17: LGTM! Standard Full HD configuration.The 1920×1080 resolution is appropriate for social media and digital display assets.
196-215: Nice touch with overlapping speaker avatars!The overlapping avatar effect (line 209:
marginLeft: index === 0 ? 0 : -25) creates a visually appealing presentation for multi-speaker talks, and the fallback topeoplePlaceholderensures the layout doesn't break.
276-278: Verify data consistency for footer location.The footer displays city and country names, but there's no guarantee these values exist if they were conditionally rendered above (e.g., line 83).
Please confirm that
event.data._computed.cityandevent.data._computed.countryare always populated for events, or add optional chaining here as well:- {event.data._computed.city?.data.name},{" "} - {event.data._computed.country?.data.name} + {event.data._computed.city?.data.name ?? 'TBD'},{" "} + {event.data._computed.country?.data.name ?? 'TBD'}
298-298: LGTM! Standard default export pattern.The default export correctly applies the configuration to the component function.
83-84: Now let me check the specific context around lines 83-84 to understand if rendering nothing is the intended behavior:
The code correctly uses optional chaining (
?.) which short-circuits and evaluates to undefined instead of throwing an error whencityis undefined. When returning undefined in React, these are all valid React nodes and are ignored when rendering the DOM. This means ifcitydata is unavailable, the element simply renders empty text, which is the intended behavior. The same pattern is consistently applied across 25+ asset generation files in the codebase, confirming this is an established, correct approach for handling potentially undefined nested properties.Likely an incorrect or invalid review comment.
19-32: The original review comment is incorrect and should be ignored.After verification:
Error handling exists: Both
getEventDataandgetTalkDataexplicitly throwNotFoundAssetErrorwhen data is not found (lines 13-14 ingetEventDataand lines 7-8 ingetTalkData). This is proper error handling for build-time asset generation, where errors should propagate to fail the build.Empty talks is safe: Calling
slice(0, 3)on an empty array returns an empty array, andPromise.all([])resolves safely. No edge case handling needed here.Try-catch wrapping is inappropriate: In an asset generation context (as this code is in the
/assetsfolder), throwing errors is the intended pattern. Wrapping in try-catch would suppress legitimate build failures when required data is missing.The utilities are correctly designed to let errors propagate in the build pipeline rather than silently failing.
Likely an incorrect or invalid review comment.
|
1- in my opinion the new section is not coherent with the left side text because style is different . |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/pages/events/[id]/assets/_schedule-short.tsx (1)
176-212: Consider extracting nested Promise.all logic for better readability.The nested
await Promise.allcalls (lines 176 and 193) work correctly but could be refactored for improved readability. Consider extracting the speaker avatar fetching logic into a separate function.Example refactor:
// Before the return statement const talksWithAvatars = await Promise.all( talks.map(async (talk) => ({ ...talk, speakerAvatars: await Promise.all( talk.__speakers.map(async (speaker) => ({ speaker, image: await getAstroImageBase64( speaker.data.avatar ?? peoplePlaceholder ), })) ), })) );Then use
talksWithAvatarsin the JSX without the nested async operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/events/[id]/assets/_schedule-short.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/events/[id]/assets/_schedule-short.tsx (7)
src/generated-assets/image.ts (2)
AssetImageConfig(11-15)getAstroImageBase64(171-174)src/pages/events/[id]/talks/[talkId]/assets/_utils.ts (1)
getTalkData(4-30)src/generated-assets/components/Frame.tsx (1)
Frame(4-35)src/generated-assets/components/BgImage.tsx (1)
BgImage(1-63)src/generated-assets/components/LogoWithFriends.tsx (1)
LogoWithFriends(6-63)src/generated-assets/theme.ts (1)
COLORS(1-6)src/lib/events.ts (2)
getEventDisplayType(329-336)getEventDisplayDate(315-328)
🔇 Additional comments (1)
src/pages/events/[id]/assets/_schedule-short.tsx (1)
145-145: No issues found—textWrap: "balance"is fully supported.The project uses Satori 0.18.3, which explicitly supports textWrap with the "balance" option. The asset generation environment has no compatibility issues with this CSS property.
| <div | ||
| style={{ | ||
| display: "flex", | ||
| flexDirection: "column", | ||
| gap: 24, | ||
| }} | ||
| > |
There was a problem hiding this comment.
Add a title for the talks section as suggested in PR feedback.
PR comment from houssembaltii recommends adding a title to explain that these are the talks featured in the meetup. This would improve clarity for viewers.
Consider adding a header above the talks list:
<div
style={{
display: "flex",
flexDirection: "column",
gap: 24,
}}
>
+ <div
+ style={{
+ fontSize: 48,
+ fontWeight: 500,
+ textTransform: "uppercase",
+ color: COLORS.primary,
+ marginBottom: 12,
+ }}
+ >
+ Featured Talks
+ </div>
{await Promise.all(📝 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.
| <div | |
| style={{ | |
| display: "flex", | |
| flexDirection: "column", | |
| gap: 24, | |
| }} | |
| > | |
| <div | |
| style={{ | |
| display: "flex", | |
| flexDirection: "column", | |
| gap: 24, | |
| }} | |
| > | |
| <div | |
| style={{ | |
| fontSize: 48, | |
| fontWeight: 500, | |
| textTransform: "uppercase", | |
| color: COLORS.primary, | |
| marginBottom: 12, | |
| }} | |
| > | |
| Featured Talks | |
| </div> |
🤖 Prompt for AI Agents
In src/pages/events/[id]/assets/_schedule-short.tsx around lines 169 to 175,
there's a talks list container rendered without a section title; add a clear
header above this div (e.g., an accessible <h3> or <h2> with text like "Featured
Talks" or "Talks") to indicate these are the talks featured in the meetup, style
it consistently with existing page headings, and ensure semantic/accessible
markup (appropriate heading level and aria attributes if needed).
| <div | ||
| style={{ | ||
| display: "flex", | ||
| flexDirection: "column", | ||
| gap: 8, | ||
| minWidth: 0, | ||
| }} | ||
| > |
There was a problem hiding this comment.
Reduce spacing between talk title and speaker name as suggested in PR feedback.
PR comment from houssembaltii recommends reducing the gap between the talk title and speaker name. The current gap of 8px (line 218) could be decreased for a tighter visual grouping.
Apply this diff to reduce the spacing:
<div
style={{
display: "flex",
flexDirection: "column",
- gap: 8,
+ gap: 4,
minWidth: 0,
}}
>🤖 Prompt for AI Agents
In src/pages/events/[id]/assets/_schedule-short.tsx around lines 214 to 221 the
vertical gap between the talk title and speaker name is set to 8px; reduce that
gap to 4px to tighten the visual grouping by updating the style.gap value from 8
to 4 on the containing div.
|
No background here? As we are on a "lighter" side of the image, it can be hard to read. 
|
Try with the "yellow" for the title maybe? 
And do |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/pages/events/[id]/assets/_schedule-short.tsx (3)
30-32: The existing comment on empty talks handling remains valid.The past review comment correctly identifies that when
event.__talksis empty, the right column renders without content, creating layout imbalance. This issue should be addressed as previously suggested.
169-175: The existing comments on this section remain valid.Past review comments correctly identify:
- Style coherence issue between left and right sections (per PR feedback)
- Missing section title to explain these are featured talks
These concerns should be addressed as previously suggested.
214-221: The existing comment on spacing remains valid.The past review comment correctly suggests reducing the gap from 8px to 4px for tighter visual grouping between talk title and speaker name, per PR feedback.
🧹 Nitpick comments (1)
src/pages/events/[id]/assets/_schedule-short.tsx (1)
237-250: Consider adjusting opacity per PR feedback.PR feedback from yoannfleurydev suggests using
opacity: 0.7for the speaker names to improve contrast. The current value is0.9, which may not provide sufficient distinction from the talk title above.Apply this diff to match the PR suggestion:
<div style={{ display: "flex", fontSize: 32, fontWeight: 500, lineHeight: 1.2, - opacity: 0.9, + opacity: 0.7, color: "lightgray", }} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/events/[id]/assets/_schedule-short.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/events/[id]/assets/_schedule-short.tsx (7)
src/generated-assets/image.ts (2)
AssetImageConfig(11-15)getAstroImageBase64(171-174)src/pages/events/[id]/talks/[talkId]/assets/_utils.ts (1)
getTalkData(4-30)src/generated-assets/components/Frame.tsx (1)
Frame(4-35)src/generated-assets/components/BgImage.tsx (1)
BgImage(1-63)src/generated-assets/components/LogoWithFriends.tsx (1)
LogoWithFriends(6-63)src/generated-assets/theme.ts (1)
COLORS(1-6)src/lib/events.ts (2)
getEventDisplayType(329-336)getEventDisplayDate(315-328)
🔇 Additional comments (5)
src/pages/events/[id]/assets/_schedule-short.tsx (5)
1-17: LGTM: Imports and configuration are well-structured.The imports are properly organized and the asset configuration follows the standard pattern for generating 1920×1080 images.
34-53: LGTM: Frame and container structure is solid.The layout correctly positions the background image and ensures content appears above it with appropriate z-indexing.
62-167: LGTM: Left column event information is well-implemented.The code properly handles optional fields (city, location) with safe navigation and conditional rendering. The bold styling with shadows and primary color creates strong visual hierarchy.
258-290: LGTM: Footer layout and content are appropriate.The footer provides essential location and website information with proper styling hierarchy via opacity.
297-297: LGTM: Default export follows standard pattern.
Closes #428
Examples of result
Summary by CodeRabbit