477 add repetition field to popup and banner #481
Conversation
- Modified the banner preview section to accommodate extra space. - Improved UI consistency across creation pages.
|
Warning Rate limit exceeded@erenfn has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 59 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis pull request introduces a comprehensive enhancement to the banner and popup functionality by adding a new Changes
Possibly related issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
backend/src/test/e2e/popup.test.mjs (1)
Line range hint
1-600: Heads up! We need test cases for the happy path too!While we've got test coverage for validation errors, we should also verify that valid repetitionType values are handled correctly in successful popup creation and updates.
Add these test cases:
- Verify successful popup creation with each valid repetitionType value
- Verify successful popup updates that change the repetitionType
- Verify the repetitionType field is correctly returned in GET responses
+ it("should handle valid repetitionType values correctly", async () => { + const validTypes = ["show_once", "show_every_visit"]; + for (const type of validTypes) { + const res = await chai.request + .execute(app) + .post("/api/popup/add_popup") + .set("Authorization", `Bearer ${token}`) + .send(popup().withRepetitionType(type).build()); + expect(res).to.have.status(201); + expect(res.body.repetitionType).to.equal(type); + } + });
🧹 Nitpick comments (11)
frontend/src/scenes/banner/BannerPageComponents/BannerPreview/BannerPreview.module.scss (1)
65-65: Hold up, these dimensions are all over the place, eh?I notice a few things that could use a second look:
- The width jumps from 80% to 95% between sections - might be jarring
- Heights vary without clear visual hierarchy (5.4rem -> 4rem -> 3rem)
Consider:
- Using consistent width increments
- Creating a more natural height progression
Here's a suggestion to make it smoother:
.bannerTwo { - width: 80%; - height: 5.4rem; + width: 85%; + height: 5rem; } .bannerThree { width: 95%; - height: 4rem; + height: 4.5rem; } .bannerFour { width: 95%; - height: 3rem; + height: 4rem; }Also applies to: 69-71, 73-75
backend/src/utils/guide.helper.js (2)
34-37: Yo dawg, the validation function looks solid, but let's make it more robust!The function correctly validates the repetition options, but we could enhance error handling by including the valid options in the error message.
const validateRepetitionOption = (value) => { const validActions = ['show only once', 'show every visit']; - return validActions.includes(value); + return validActions.includes(value?.toLowerCase?.()); };
39-43: Mom's spaghetti says we need better error messages!The error message could be more helpful by including the valid options.
const ensureValidRepetitionOption = (value) => { if (!validateRepetitionOption (value)) { - throw new Error('Invalid repetition option selected'); + throw new Error('Invalid repetition option selected. Valid options are: "show only once" and "show every visit"'); } };frontend/src/scenes/popup/PopupPageComponents/PopupContent/PopupContent.jsx (1)
41-46: There's UI on his sweater already, let's make it steady!Consider these improvements to the repetition dropdown section:
- Extract hardcoded strings to constants:
const REPETITION_OPTIONS = { ONCE: 'Show only once', EVERY_VISIT: 'Show every visit' };
- Move inline styles to the SCSS module:
- <h2 style={{marginTop: '1.5rem'}}>Repetition</h2> + <h2 className={styles.sectionHeader}>Repetition</h2>frontend/src/scenes/banner/BannerPageComponents/BannerLeftContent/BannerLeftContent.jsx (2)
15-16: Consider using PropTypes.oneOf for buttonRepetitionYo! The PropTypes validation could be more specific by using
PropTypes.oneOfto restrict the possible values.- buttonRepetition: PropTypes.string + buttonRepetition: PropTypes.oneOf(['Show only once', 'Show every visit'])Also applies to: 97-98
42-47: Extract repetition options as constantsMom's spaghetti! These magic strings should be extracted as constants to maintain consistency and prevent typos.
+const REPETITION_OPTIONS = { + ONCE: 'Show only once', + EVERY_VISIT: 'Show every visit' +}; <DropdownList - actions={['Show only once', 'Show every visit']} + actions={Object.values(REPETITION_OPTIONS)} onActionChange={handleRepetitionChange} selectedActionString={buttonRepetition} />backend/src/models/Popup.js (1)
34-36: Rename validation function for clarityPalms are sweaty! The validation function name
isValidActiondoesn't match its purpose of validating repetition type.validate: { - isValidAction(value) { + isValidRepetitionType(value) { ensureValidRepetitionOption(value); }, }backend/src/controllers/banner.controller.js (1)
Line range hint
34-38: Yo! Let's make these error messages consistent, fam!The error messages for repetitionType validation differ between addBanner and editBanner:
- addBanner: "Invalid value entered"
- editBanner: "Invalid value for repetition"
Let's keep it consistent across both methods.
- return res.status(400).json({ - errors: [{ msg: "Invalid value entered" }], - }); + return res.status(400).json({ + errors: [{ msg: "Invalid value for repetition" }], + });Also applies to: 135-139
backend/src/controllers/popup.controller.js (1)
39-40: Yo dawg, these error messages need to be more specific!The current error message combines multiple validation failures into one message. Consider separating them for better error handling:
- errors: [{ msg: "Invalid value for popupSize or closeButtonAction or repetitionType" }], + errors: [ + !validatePopupSize(popupSize) && { msg: "Invalid value for popupSize" }, + !validateCloseButtonAction(closeButtonAction) && { msg: "Invalid value for closeButtonAction" }, + !validateRepetitionOption(repetitionType) && { msg: "Invalid value for repetition" } + ].filter(Boolean),frontend/src/scenes/popup/CreatePopupPage.jsx (2)
43-43: Yo! Let's not keep that string hanging like mom's spaghetti!The 'Show only once' string is hardcoded. Consider moving it to a constants file for better maintainability.
+const DEFAULT_BUTTON_REPETITION = 'Show only once'; + - const [buttonRepetition, setButtonRepetition] = useState('Show only once') + const [buttonRepetition, setButtonRepetition] = useState(DEFAULT_BUTTON_REPETITION)
126-126: Keep that case conversion consistent, yo!Converting buttonRepetition to lowercase only at save time could lead to inconsistencies. Consider handling case conversion when setting the state:
- repetitionType: buttonRepetition.toLowerCase(), + repetitionType: buttonRepetition, // And update the state setter: - const [buttonRepetition, setButtonRepetition] = useState('Show only once') + const [buttonRepetition, setButtonRepetition] = useState('show only once') + const handleRepetitionChange = (value) => setButtonRepetition(value.toLowerCase())
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
backend/src/controllers/banner.controller.js(5 hunks)backend/src/controllers/popup.controller.js(5 hunks)backend/src/models/Banner.js(2 hunks)backend/src/models/Popup.js(2 hunks)backend/src/test/e2e/auth.test.mjs(2 hunks)backend/src/test/e2e/popup.test.mjs(4 hunks)backend/src/test/mocks/banner.mock.js(12 hunks)backend/src/test/mocks/popup.mock.js(2 hunks)backend/src/test/unit/controllers/popup.test.js(2 hunks)backend/src/utils/guide.helper.js(1 hunks)frontend/public/svg/utilityIcons.jsx(3 hunks)frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftApperance.jsx(1 hunks)frontend/src/scenes/banner/BannerPageComponents/BannerLeftContent/BannerLeftContent.jsx(3 hunks)frontend/src/scenes/banner/BannerPageComponents/BannerPreview/BannerPreview.jsx(1 hunks)frontend/src/scenes/banner/BannerPageComponents/BannerPreview/BannerPreview.module.scss(2 hunks)frontend/src/scenes/banner/CreateBannerPage.jsx(4 hunks)frontend/src/scenes/popup/CreatePopupPage.jsx(4 hunks)frontend/src/scenes/popup/PopupPageComponents/PopupAppearance/PopupAppearance.module.scss(1 hunks)frontend/src/scenes/popup/PopupPageComponents/PopupContent/PopupContent.jsx(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- frontend/src/scenes/banner/BannerPageComponents/BannerLeftAppearance/BannerLeftApperance.jsx
- frontend/public/svg/utilityIcons.jsx
- frontend/src/scenes/popup/PopupPageComponents/PopupAppearance/PopupAppearance.module.scss
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (8)
frontend/src/scenes/banner/BannerPageComponents/BannerPreview/BannerPreview.jsx (1)
37-37: Yo, nice addition of the new banner section!The new bannerFour div looks good, eh? Just make sure it maintains proper spacing with the other banner sections above it.
Let's check if this plays nice with different screen sizes:
✅ Verification successful
Yo, I checked the styles and they're straight fire! 🔥
The bannerFour styling is perfectly aligned with the other banners, eh? Got the same background color, proper spacing with that 1rem margin-bottom, and it's rockin' that 95% width just like bannerThree. Everything's as clean as mom's spaghetti!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for responsive design tests rg -l "getByTestId.*bannerFour.*resize|@media.*bannerFour" --type css --type jsxLength of output: 110
Script:
#!/bin/bash # Find the associated style module and check its contents fd -e module.css -e css bannerPreview --exec cat {} # Look for responsive design patterns in the component rg -t js -t jsx "bannerFour|@media" frontend/src/scenes/banner/Length of output: 149
Script:
#!/bin/bash # List all files in the banner preview directory to understand structure ls -R frontend/src/scenes/banner/BannerPageComponents/BannerPreview/ # Try to find and show content of any CSS/style files find frontend/src/scenes/banner/BannerPageComponents/BannerPreview/ -type f -name "*.css" -o -name "*.scss" -exec cat {} \;Length of output: 1788
frontend/src/scenes/banner/BannerPageComponents/BannerPreview/BannerPreview.module.scss (1)
49-50: Beauty addition to the selector list!Good grouping of similar elements, keeping the code DRY. 👍
backend/src/test/mocks/popup.mock.js (1)
6-6: Palms are sweaty but these mock changes are ready!The mock builder implementation is solid:
- Default value is properly set
- Test helper methods follow the established pattern
- Covers both missing and invalid cases
Also applies to: 71-79
frontend/src/scenes/popup/PopupPageComponents/PopupContent/PopupContent.jsx (1)
35-37: The handler's calm and ready!The repetition change handler looks good and follows the component's patterns.
backend/src/test/mocks/banner.mock.js (1)
48-56: LGTM! Comprehensive test coverage for edge casesKnees weak! The test helpers cover both missing and invalid repetition types, which is crucial for thorough testing.
frontend/src/scenes/banner/CreateBannerPage.jsx (1)
24-24: Verify case sensitivity handlingArms are heavy! The
toLowerCase()transformation might cause validation issues if the backend expects specific casing.Also applies to: 68-68
✅ Verification successful
Yo, the case transformation is actually on point! 🍝
The backend expects these exact strings (case-sensitive):
- 'show only once'
- 'show every visit'
So that
toLowerCase()is saving our spaghetti from hitting the fan! Keep it in there, it's doing the right thing.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check backend validation for repetition type casing ast-grep --pattern 'ensureValidRepetitionOption($_)'Length of output: 213
Script:
#!/bin/bash # Find implementation of ensureValidRepetitionOption ast-grep --pattern 'function ensureValidRepetitionOption($_) { $$$ }' # Also search with const/let/var in case it's not a function declaration ast-grep --pattern '$_ ensureValidRepetitionOption = $_' # Backup search with ripgrep in case it's defined differently rg "ensureValidRepetitionOption" -A 5Length of output: 1897
Script:
#!/bin/bash # Find validateRepetitionOption implementation ast-grep --pattern 'const validateRepetitionOption = $_' # Backup search with ripgrep rg "validateRepetitionOption" -B 2 -A 5Length of output: 5270
backend/src/controllers/banner.controller.js (1)
3-3: LGTM! Validation implementation is consistent.The addition of repetitionType validation follows the established pattern and maintains consistency with existing validation checks.
Also applies to: 17-17, 32-33
backend/src/test/e2e/auth.test.mjs (1)
363-366: Yo! These changes seem to be in the wrong PR!The updates to auth error messages appear unrelated to the PR's objective of adding repetition field to popup and banner. Consider moving these changes to a separate PR for better traceability.
Also applies to: 377-380
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Looks good - thanks @MandeepPaul ! |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Describe your changes
Issue number
#477
Please ensure all items are checked off before requesting a review:
Preview