Conversation
New TshirtAds component renders three fixed-position display-ad-style slots (160x600 side rails, 728x90 desktop bottom banner, 320x50 mobile bottom banner) via ReactDOM portals. Each slot cycles through its image pool on every page load with non-overlapping random selection. Clicks and dismissals fire GA4 events tagged with ad_position. Dismissal persists 7 days via localStorage. Side rails hidden under 1280px, bottom banner hidden under 768px, mobile banner hidden at 768px+. Includes 4 IAB-compliant 160x600 t-shirt mockup creatives. Updates .gitignore to whitelist public/ads/ so creatives ship with the build, while keeping public/ads/_originals/ local-only. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
At viewports >= 1280px the 160x600 side rails occupy 0-176px from each edge, but .content-section and footer were sized at max-width: 80%, which only clears 10% on each side. Below ~1760px viewport width the content slid underneath the ads. Cap .content-section and footer at min(80%, calc(100% - 400px)) inside a min-width: 1280px media query so they always leave at least 200px on each side for the ads plus breathing room. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe PR integrates a dismissible Fourthwall ad component into dict2json.com. It adds ChangesFourthwall T-shirt Ads Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
- Drop stale TODO comment in TshirtAds.js — the Fourthwall URL is set - Remove plan file plans/generic-dancing-goose.md; intermediate work product shouldn't ship to main Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/TshirtAds.js (2)
116-122: ⚡ Quick winConsider adding srcset for retina displays.
The plan document specifies supporting
@2xretina variants viasrcset, but the current implementation only uses thesrcattribute. This may result in lower quality images on high-DPI displays.📱 Proposed enhancement for retina support
<img className="tshirt-ad__image" src={`/ads/${slot.filename}`} + srcSet={`/ads/${slot.filename} 1x, /ads/${slot.filename.replace(/\.(png|jpg)$/, '`@2x`.$1')} 2x`} alt={TSHIRT_ALT} width={slot.width} height={slot.height} />Note: This assumes
@2xvariants follow the naming patternfilename@2x.extension. You may need to adjust the IMAGE_POOLS structure to explicitly track 1x and 2x variants if the naming differs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/TshirtAds.js` around lines 116 - 122, The image element in TshirtAds.js uses only src (src={`/ads/${slot.filename}`}) and lacks a srcset for `@2x` retina variants; update the <img> rendering logic in the TshirtAds component to generate and include a srcset that points to the 2x filename variant (e.g., derive `${base}`@2x`.${ext}` from slot.filename) alongside the existing src, keep alt={TSHIRT_ALT} and width/height as-is, and ensure the srcset string uses the "2x" descriptor so high-DPI displays load the `@2x` asset.
4-5: TODO: Fourthwall product URL needs to be finalized.The placeholder URL should be replaced with the actual product URL before deployment. The plan document lists this as an open item requiring user input.
Do you have the final Fourthwall product URL ready, or would you like me to help verify this is the correct one?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/TshirtAds.js` around lines 4 - 5, Replace the placeholder FOURTHWALL_URL constant in TshirtAds.js with the final Fourthwall product URL (update the value of FOURTHWALL_URL to the production product link) or, if you prefer configurability, refactor FOURTHWALL_URL into a runtime configuration/env var (e.g., process.env.FOURTHWALL_URL) and ensure the component uses that symbol so the correct URL can be provided without code changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/components/TshirtAds.js`:
- Around line 116-122: The image element in TshirtAds.js uses only src
(src={`/ads/${slot.filename}`}) and lacks a srcset for `@2x` retina variants;
update the <img> rendering logic in the TshirtAds component to generate and
include a srcset that points to the 2x filename variant (e.g., derive
`${base}`@2x`.${ext}` from slot.filename) alongside the existing src, keep
alt={TSHIRT_ALT} and width/height as-is, and ensure the srcset string uses the
"2x" descriptor so high-DPI displays load the `@2x` asset.
- Around line 4-5: Replace the placeholder FOURTHWALL_URL constant in
TshirtAds.js with the final Fourthwall product URL (update the value of
FOURTHWALL_URL to the production product link) or, if you prefer
configurability, refactor FOURTHWALL_URL into a runtime configuration/env var
(e.g., process.env.FOURTHWALL_URL) and ensure the component uses that symbol so
the correct URL can be provided without code changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d896e08c-80b6-4342-8abd-335e1e084500
⛔ Files ignored due to path filters (6)
public/ads/horizontal-mobile.jpgis excluded by!**/*.jpgpublic/ads/horizontal.jpgis excluded by!**/*.jpgpublic/ads/unisex-staple-t-shirt-black-front-6a0df0764472a.pngis excluded by!**/*.pngpublic/ads/unisex-staple-t-shirt-black-left-front-6a0df07642e27.pngis excluded by!**/*.pngpublic/ads/unisex-staple-t-shirt-black-right-front-6a0df0763f3d0.pngis excluded by!**/*.pngpublic/ads/unisex-staple-t-shirt-black-right-front-6a0df07641fa5.pngis excluded by!**/*.png
📒 Files selected for processing (6)
.gitignoreplans/generic-dancing-goose.mdsrc/App.jssrc/components/TshirtAds.jssrc/styles/ads.csssrc/styles/layout.css
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
Release Notes
New Features