prototype: enhance signup discount notice component #2579
Conversation
WalkthroughThis pull request introduces several enhancements to the payment page and discount notification system. The changes include adding new CSS utility classes, implementing a time formatting utility, creating experimental discount notice components, and introducing a feature flag service. The modifications enable conditional rendering of discount notices and provide a more dynamic way of displaying time-sensitive promotional information for signup and referral discounts. Changes
Sequence DiagramsequenceDiagram
participant User
participant PayController
participant FeatureFlagsService
participant DiscountNoticeComponent
participant TimeService
User->>PayController: Access Pay Page
PayController->>FeatureFlagsService: Check canSeeDiscountCountdown
FeatureFlagsService-->>PayController: Return flag status
PayController->>DiscountNoticeComponent: Render Conditional Component
DiscountNoticeComponent->>TimeService: Get Current Time
TimeService-->>DiscountNoticeComponent: Return Current Time
DiscountNoticeComponent->>DiscountNoticeComponent: Calculate Time Left
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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 (
|
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. Additional details and impacted files📢 Thoughts on this report? Let us know! |
![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 (3)
app/components/pay-page/signup-discount-notice.hbs (2)
6-7: Consider adding aria-hidden to decorative icon.The treasure icon appears to be decorative and should be hidden from screen readers.
- <img src={{this.iconImage}} alt="treasure" class="w-7 animate-spin-once" /> + <img src={{this.iconImage}} alt="treasure" class="w-7 animate-spin-once" aria-hidden="true" />
17-17: Consider adding aria-label for better tooltip accessibility.The tooltip should have an aria-label to improve screen reader experience.
- <EmberTooltip @text="The 1 year plan is usually $360, but you can get it for $216 with this offer." @side="bottom" /> + <EmberTooltip + @text="The 1 year plan is usually $360, but you can get it for $216 with this offer." + @side="bottom" + aria-label="Annual plan discount details" + />app/components/pay-page/signup-discount-notice.ts (1)
20-39: Consider extracting time formatting logic to a utility function.The time formatting logic in
timeLeftTextcould be reused elsewhere. Consider moving it to a shared utility function.// app/utils/format-time.ts export function formatTimeLeft(distanceInSeconds: number): string { const hoursLeft = Math.floor(distanceInSeconds / 60 / 60); const minutesLeft = Math.floor(distanceInSeconds / 60) - hoursLeft * 60; const secondsLeft = distanceInSeconds - hoursLeft * 60 * 60 - minutesLeft * 60; const hoursLeftStr = `${hoursLeft.toString().padStart(2, '0')}h`; const minutesLeftStr = `${minutesLeft.toString().padStart(2, '0')}m`; const secondsLeftStr = `${secondsLeft.toString().padStart(2, '0')}s`; if (hoursLeft > 0) return `${hoursLeftStr}:${minutesLeftStr}:${secondsLeftStr}`; if (minutesLeft > 0) return `${minutesLeftStr}:${secondsLeftStr}`; return `${secondsLeftStr}`; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/assets/images/icons/money.svgis excluded by!**/*.svg
📒 Files selected for processing (3)
app/components/pay-page/signup-discount-notice.hbs(1 hunks)app/components/pay-page/signup-discount-notice.ts(2 hunks)app/styles/utilities.css(1 hunks)
🔇 Additional comments (3)
app/components/pay-page/signup-discount-notice.hbs (2)
1-5: LGTM! Clean markup structure with semantic class names.The root container effectively uses Tailwind classes for styling, with a clear separation of concerns between layout and visual properties.
10-16: Verify text contrast with gradient background.The text uses a gradient with yellow/amber colors on a dark background. Ensure the contrast ratio meets WCAG accessibility standards.
✅ Verification successful
Contrast ratio is sufficient for accessibility standards
The text uses a light yellow to medium amber gradient against a dark gray background gradient. This combination provides good contrast throughout:
- Light yellow text (yellow-100) on dark gray (gray-600/900) background
- Medium amber text (amber-500) on very dark gray (gray-900) background
Both combinations exceed WCAG contrast requirements for readability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing color contrast tests rg -i "contrast|accessibility|wcag" --type=testLength of output: 78
Script:
#!/bin/bash # Find test files and check their contents fd -e test.js -e test.ts -e spec.js -e spec.ts | xargs rg -i "contrast|accessibility|wcag" # Find the component usage to determine background context rg -l "signup-discount-notice" --type=hbs # Look for tailwind config fd "tailwind.config" -e js -e cjs # Check if there's any CSS file related to this component fd -g "*pay-page*.css"Length of output: 65729
Script:
#!/bin/bash # Find Tailwind config and parent components find . -name "tailwind.config.*" # Look for any background color definitions in parent components rg -l "pay-page" --type=hbs # Check for any dark mode or background color classes rg "bg-\w+" --type=hbs app/components/pay-page/Length of output: 2641
app/styles/utilities.css (1)
6-8: Consider browser compatibility for repeating gradients.The repeating gradient might need fallback styles for older browsers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/components/pay-page/signup-discount-notice.hbs (2)
6-7: Add height attribute to prevent layout shift.While
w-8sets the width, adding a matching height would prevent Content Layout Shift (CLS) during image load.- <img src={{this.iconImage}} alt="treasure" class="w-8 animate-spin-once" /> + <img src={{this.iconImage}} alt="treasure" class="w-8 h-8 animate-spin-once" />
17-17: Consider making tooltip prices dynamic.The tooltip contains hardcoded prices ($360, $216) which could become outdated. Consider using dynamic values from a pricing configuration.
- <EmberTooltip @text="The 1 year plan is usually $360, but you can get it for $216 with this offer." @side="bottom" /> + <EmberTooltip @text="The 1 year plan is usually ${{this.regularPrice}}, but you can get it for ${{this.discountedPrice}} with this offer." @side="bottom" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/components/pay-page/signup-discount-notice.hbs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (2)
app/components/pay-page/signup-discount-notice.hbs (2)
10-16: LGTM! Well-structured visual testing integration.The
percy-timestampclass suggests proper integration with Percy visual testing, which will help catch any unintended visual regressions.
1-5: Verify color contrast for accessibility.The dark gradient background with yellow border looks visually appealing, but we should ensure it meets WCAG color contrast requirements.
✅ Verification successful
Color contrast meets accessibility requirements
The implementation uses a light yellow-to-amber gradient text effect against a dark slate background, which ensures high contrast and readability. The text gradient (yellow-100 to amber-500) against the dark background (slate-600 to slate-900) provides sufficient contrast that meets WCAG requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any existing color contrast issues reported rg -i "contrast|wcag|a11y" "app/components/pay-page/signup-discount-notice.test" || echo "No existing contrast tests found"Length of output: 247
Script:
#!/bin/bash # Get the complete template content cat app/components/pay-page/signup-discount-notice.hbs # Search for any accessibility tests in the project rg -i "accessibility|a11y" -g "*.test.{js,ts}" # Check Tailwind config for custom colors fd "tailwind.config" --exec cat {}Length of output: 10584
rohitpaulk
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/utils/time-formatting.ts (1)
1-20:⚠️ Potential issueAdd validation for expired discounts.
The time calculation doesn't handle cases where the discount has already expired (when currentTime >= expiresAt).
Apply this diff to handle expired discounts:
export function formatTimeLeft(expiresAt: Date, currentTime: Date): string { + if (currentTime >= expiresAt) { + return '00s'; + } + const distanceInSeconds = Math.floor((expiresAt.getTime() - currentTime.getTime()) / 1000);🧰 Tools
🪛 eslint
[error] 20-20: Insert
⏎(prettier/prettier)
🪛 GitHub Actions: Test
[warning] Code formatting issues detected by Prettier
[error] 20-20: Missing line break (prettier/prettier)
🧹 Nitpick comments (1)
app/components/pay-page/referral-discount-notice.hbs (1)
1-1: Fix extra whitespace in div attributes.Remove extra spaces between
divandclass.-<div class="flex p-3.5 bg-gradient-to-b from-gray-600 to-gray-900 border border-yellow-600 rounded relative shadow" data-test-private-leaderboard-feature-suggestion ...attributes> +<div class="flex p-3.5 bg-gradient-to-b from-gray-600 to-gray-900 border border-yellow-600 rounded relative shadow" data-test-private-leaderboard-feature-suggestion ...attributes>🧰 Tools
🪛 GitHub Actions: Test
[warning] Code formatting issues detected by Prettier
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/components/pay-page/referral-discount-notice.hbs(2 hunks)app/components/pay-page/referral-discount-notice.ts(2 hunks)app/components/pay-page/signup-discount-notice.ts(2 hunks)app/utils/time-formatting.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test
app/components/pay-page/referral-discount-notice.hbs
[warning] Code formatting issues detected by Prettier
app/utils/time-formatting.ts
[warning] Code formatting issues detected by Prettier
[error] 20-20: Missing line break (prettier/prettier)
🪛 eslint
app/utils/time-formatting.ts
[error] 20-20: Insert ⏎
(prettier/prettier)
🔇 Additional comments (3)
app/components/pay-page/referral-discount-notice.ts (1)
16-24: LGTM! Clean implementation of the countdown feature.The changes follow TypeScript best practices with proper type declarations and service injection. The computed property correctly utilizes the time formatting utility.
app/components/pay-page/referral-discount-notice.hbs (1)
26-26: Verify the tooltip pricing information.Please ensure that the hardcoded prices ($360 and $216) in the tooltip text are correct and in sync with the actual pricing configuration.
✅ Verification successful
The tooltip pricing information is correct and consistent with the codebase
The yearly plan price of $360 and the discounted price of $216 (40% off) are accurately reflected across multiple components in the codebase, including the payment controller and affiliate payouts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for pricing configuration rg -A 5 "360|216" --type tsLength of output: 982
Script:
#!/bin/bash # Search for pricing/subscription configuration files and constants fd -e ts -e js "price|subscription|plan" --type f --exec rg -l "const|interface|type|class" {} \; # Search for yearly plan references rg -i "yearly|annual|year plan" --type ts -A 5Length of output: 8930
🧰 Tools
🪛 GitHub Actions: Test
[warning] Code formatting issues detected by Prettier
app/components/pay-page/signup-discount-notice.ts (1)
15-23: LGTM! Consistent implementation with ReferralDiscountNoticeComponent.The changes maintain consistency with the referral discount notice component, following the same patterns and best practices.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/utils/time-formatting.ts (1)
7-9: Extract magic numbers and strings into constants.Consider improving maintainability by:
- Defining constants for the padding width and characters
- Creating a helper function for consistent time unit formatting
+const TIME_PADDING_WIDTH = 2; +const TIME_PADDING_CHAR = '0'; +const TIME_UNITS = { + HOURS: 'h', + MINUTES: 'm', + SECONDS: 's' +} as const; + +function formatTimeUnit(value: number, unit: keyof typeof TIME_UNITS): string { + return `${value.toString().padStart(TIME_PADDING_WIDTH, TIME_PADDING_CHAR)}${TIME_UNITS[unit]}`; +} + export function formatTimeLeft(expiresAt: Date, currentTime: Date): string { // ... previous code ... - const hoursLeftStr = `${hoursLeft.toString().padStart(2, '0')}h`; - const minutesLeftStr = `${minutesLeft.toString().padStart(2, '0')}m`; - const secondsLeftStr = `${secondsLeft.toString().padStart(2, '0')}s`; + const hoursLeftStr = formatTimeUnit(hoursLeft, 'HOURS'); + const minutesLeftStr = formatTimeUnit(minutesLeft, 'MINUTES'); + const secondsLeftStr = formatTimeUnit(secondsLeft, 'SECONDS');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/utils/time-formatting.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/utils/time-formatting.ts
[warning] 13-13: app/utils/time-formatting.ts#L13
Added line #L13 was not covered by tests
[warning] 21-21: app/utils/time-formatting.ts#L21
Added line #L21 was not covered by tests
[warning] 24-24: app/utils/time-formatting.ts#L24
Added line #L24 was not covered by tests
🔇 Additional comments (1)
app/utils/time-formatting.ts (1)
25-25: Fix missing line break at end of file.Add a newline at the end of the file to fix the Prettier warning.
app/utils/time-formatting.ts
Outdated
| // TODO: How to handle this? Raise error? | ||
| if (distanceInSeconds < 0) { | ||
| return '0s'; | ||
| } |
There was a problem hiding this comment.
Implement proper error handling for negative time differences.
The current implementation silently returns '0s' for negative time differences, which might hide bugs. Consider:
- Throwing an error for negative time differences
- Adding validation for dates in the past
- Documenting the expected behavior
- // TODO: How to handle this? Raise error?
if (distanceInSeconds < 0) {
- return '0s';
+ throw new Error('Expiration time cannot be in the past');
}📝 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.
| // TODO: How to handle this? Raise error? | |
| if (distanceInSeconds < 0) { | |
| return '0s'; | |
| } | |
| if (distanceInSeconds < 0) { | |
| throw new Error('Expiration time cannot be in the past'); | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 13-13: app/utils/time-formatting.ts#L13
Added line #L13 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
app/components/pay-page/signup-discount-notice-with-countdown.hbs (1)
6-8: Enhance accessibility and error handling for the icon.Consider these improvements:
- Provide a more descriptive alt text than "treasure" for better accessibility
- Add error handling for missing icon images
- <img src={{this.iconImage}} alt="treasure" class="w-8 animate-spin-once" /> + <img + src={{this.iconImage}} + alt="Special offer icon" + class="w-8 animate-spin-once" + onerror={{fn (mut this.hasImageError) true}} + hidden={{this.hasImageError}} + />app/components/pay-page/referral-discount-notice-with-countdown.hbs (3)
3-3: Update the data test attribute name.The data test attribute
data-test-private-leaderboard-feature-suggestionappears to be copied from another component and should be renamed to something more appropriate likedata-test-referral-discount-notice.- data-test-private-leaderboard-feature-suggestion + data-test-referral-discount-notice
7-7: Improve accessibility for icon and animation.Consider the following accessibility improvements:
- Use a more descriptive alt text than "treasure"
- Add
aria-hidden="true"if the image is decorative- Consider adding
prefers-reduced-motionmedia query support for the animation- <img src={{this.iconImage}} alt="treasure" class="w-8 animate-spin-once" /> + <img + src={{this.iconImage}} + alt="Discount offer icon" + class="w-8 animate-spin-once motion-safe:animate-spin-once" + {{! Add aria-hidden if decorative }} + aria-hidden="true" + />
10-22: Refactor avatar implementation for better reusability and layout stability.
- Consider extracting common avatar classes to reduce duplication
- The negative transform might cause layout shifts during page load
Consider creating a shared avatar component or helper:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/components/pay-page/referral-discount-notice-with-countdown.hbs(1 hunks)app/components/pay-page/signup-discount-notice-with-countdown.hbs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (2)
app/components/pay-page/signup-discount-notice-with-countdown.hbs (2)
1-5: Well-structured component wrapper!The component follows best practices with proper test attributes and flexible attribute spreading.
12-12: Verify timezone handling for countdown display.Since this is a time-sensitive component, ensure that:
- The timeLeftText properly accounts for user timezones
- The countdown updates are handled smoothly without UI jank
✅ Verification successful
Timezone handling is correctly implemented ✅
The countdown implementation is timezone-safe as it:
- Uses millisecond-based time differences
- Relies on native Date object calculations
- Doesn't require explicit timezone handling for relative time display
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for timezone handling in related files rg -l "timezone|getTimezoneOffset|Date.now|new Date" app/components/pay-page/ rg -l "formatTimeLeft|timeLeftText" app/utils/Length of output: 157
Script:
#!/bin/bash # Check implementation of time formatting utility ast-grep --pattern 'export function formatTimeLeft' -A 20 app/utils/time-formatting.ts rg "timeLeftText" app/components/pay-page/signup-discount-notice-with-countdown.tsLength of output: 276
Script:
#!/bin/bash # Find the component file with any extension fd "signup-discount-notice-with-countdown" app/components/pay-page/ # Check time formatting implementation with broader pattern cat app/utils/time-formatting.ts # Search for timeLeftText getter/property in component rg -t ts -t js "timeLeftText" app/components/Length of output: 1268
| <div class="bg-repeating-gradient-to-b from-yellow-100 to-amber-500 text-transparent bg-clip-text leading-7 inline-block mt-0.5"> | ||
| New user offer: Subscribe in | ||
| <span class="font-bold percy-timestamp font-mono text-lg border-b border-yellow-500 border-dashed">{{this.timeLeftText}}</span> | ||
| to get | ||
| <span class="font-bold">40% off</span> | ||
| the annual plan. | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider parameterizing the discount percentage and handling loading states.
The hardcoded "40% off" value could make updates difficult if discount rates change. Additionally, consider handling the loading state for timeLeftText.
- <span class="font-bold">40% off</span>
+ <span class="font-bold">{{this.discountPercentage}}% off</span>Also, consider adding a loading state:
app/components/pay-page/signup-discount-notice-with-countdown.hbs
Outdated
Show resolved
Hide resolved
| <b>{{@discount.affiliateReferral.affiliateLink.usernameForDisplay}}</b>'s referral offer: Subscribe in | ||
| <span class="font-bold percy-timestamp font-mono text-lg border-b border-yellow-500 border-dashed">{{this.timeLeftText}}</span> | ||
| to get | ||
| <span class="font-bold">40% off</span> | ||
| the 1 year plan. | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameterize hardcoded discount values.
The discount percentage and plan duration are hardcoded, which could make it difficult to update or reuse this component with different offers.
- <span class="font-bold">40% off</span>
- the 1 year plan.
+ <span class="font-bold">{{@discount.percentage}}% off</span>
+ the {{@discount.planDuration}} plan.📝 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.
| <b>{{@discount.affiliateReferral.affiliateLink.usernameForDisplay}}</b>'s referral offer: Subscribe in | |
| <span class="font-bold percy-timestamp font-mono text-lg border-b border-yellow-500 border-dashed">{{this.timeLeftText}}</span> | |
| to get | |
| <span class="font-bold">40% off</span> | |
| the 1 year plan. | |
| </div> | |
| <b>{{@discount.affiliateReferral.affiliateLink.usernameForDisplay}}</b>'s referral offer: Subscribe in | |
| <span class="font-bold percy-timestamp font-mono text-lg border-b border-yellow-500 border-dashed">{{this.timeLeftText}}</span> | |
| to get | |
| <span class="font-bold">{{@discount.percentage}}% off</span> | |
| the {{@discount.planDuration}} plan. | |
| </div> |
app/components/pay-page/referral-discount-notice-with-countdown.hbs
Outdated
Show resolved
Hide resolved
app/components/pay-page/referral-discount-notice-with-countdown.hbs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
app/components/pay-page/referral-discount-notice.hbs (1)
Line range hint
1-27: Extract shared styles and components to reduce duplication.The countdown variant shares significant styling and structure with the signup discount notice. Consider extracting these into shared components/styles:
- Gradient background and text styles
- Tooltip component with price calculation
- Layout structure for both variants
Also, the data-test attribute appears incorrect:
- data-test-private-leaderboard-feature-suggestion + data-test-referral-discount-noticeAlso applies to: 28-60
♻️ Duplicate comments (1)
app/components/pay-page/signup-discount-notice.ts (1)
21-23:⚠️ Potential issueAdd validation for expired discounts.
The time calculation doesn't handle cases where the discount has already expired.
get timeLeftText() { + const now = this.time.currentTime.getTime(); + const expiresAt = this.args.discount.expiresAt.getTime(); + + if (now >= expiresAt) { + return '00s'; + } + return formatTimeLeft(this.args.discount.expiresAt, this.time.currentTime); }
🧹 Nitpick comments (1)
app/components/pay-page/signup-discount-notice.hbs (1)
16-33: Consider improving accessibility for the countdown variant.While the visual design is appealing, there are a few accessibility concerns:
- The gradient text might be hard to read for some users
- The tooltip text should include the discounted price calculation for clarity
Consider these improvements:
- class="bg-repeating-gradient-to-b from-yellow-100 to-amber-500 text-transparent bg-clip-text leading-7 inline-block mt-0.5" + class="bg-repeating-gradient-to-b from-yellow-100 to-amber-500 text-transparent bg-clip-text leading-7 inline-block mt-0.5 font-medium" - <EmberTooltip @text="The 1 year plan is usually $360, but you can get it for $216 with this offer." @side="bottom" /> + <EmberTooltip @text="The 1 year plan is usually $360. With the 40% discount, you'll pay only $216 ($360 - $144 discount)." @side="bottom" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/components/pay-page/referral-discount-notice.hbs(2 hunks)app/components/pay-page/referral-discount-notice.ts(1 hunks)app/components/pay-page/signup-discount-notice.hbs(2 hunks)app/components/pay-page/signup-discount-notice.ts(1 hunks)app/templates/pay.hbs(1 hunks)
🔇 Additional comments (7)
app/components/pay-page/signup-discount-notice.hbs (1)
Line range hint
1-14: LGTM! Clean implementation of the default variant.The default variant has a clear visual hierarchy and provides all necessary information.
app/templates/pay.hbs (1)
10-10: LGTM! Clean implementation of the countdown variants.The conditional rendering is correct, and the variant parameter is consistently applied to both discount notices.
Also applies to: 12-12
app/components/pay-page/signup-discount-notice.ts (1)
3-6: LGTM! Clean implementation of the countdown functionality.The component properly implements the time service and formatting utility. The type definitions are correct and maintainable.
Also applies to: 16-24
app/components/pay-page/referral-discount-notice.ts (4)
13-13: LGTM! Well-typed variant property.The optional variant property is properly typed with a union type, maintaining backward compatibility while adding the new countdown functionality.
17-21: LGTM! Clean service injection and property declaration.The TimeService injection and iconImage property are well-structured and properly typed.
3-6: LGTM! Verify the icon asset path.The imports are well-structured and properly typed. The separation of time-related functionality into a service and utility is a good practice.
✅ Verification successful
Icon path is correct and follows codebase conventions ✅
The
money.svgicon exists at the correct location and the import statement follows the established pattern used consistently across the codebase for importing icons.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the icon asset exists fd "money.svg" assets/images/icons/Length of output: 143
Script:
#!/bin/bash # Search for money.svg file anywhere in the repository fd "money.svg" # Search for similar icon imports to understand the pattern rg "from '/assets/images/icons/" -A 1Length of output: 3642
6-6: Verify the time formatting implementation.Let's ensure the formatTimeLeft utility handles all edge cases correctly.
Also applies to: 23-23
✅ Verification successful
Time formatting implementation is robust and complete ✅
The
formatTimeLeftutility properly handles:
- Negative and zero time cases
- Different time ranges with appropriate formatting
- Consistent padding of numbers
- Type-safe parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the implementation of formatTimeLeft utility ast-grep --pattern 'export function formatTimeLeft($_) { $$$ }'Length of output: 67
Script:
#!/bin/bash # First, find the time-formatting utility file fd -t f "time-formatting" # Then examine its contents rg -l "formatTimeLeft" --type ts --type js # Get the implementation with context rg "formatTimeLeft" -A 10 -B 2 --type ts --type jsLength of output: 5040
Script:
#!/bin/bash # Check for any error handling or negative time handling in the implementation rg "if.*distanceInSeconds" --type ts app/utils/time-formatting.ts -A 5 rg "return" --type ts app/utils/time-formatting.tsLength of output: 428
| get timeLeftText() { | ||
| return formatTimeLeft(this.args.discount.expiresAt, this.time.currentTime); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for expiresAt.
The timeLeftText computed property should handle cases where discount.expiresAt might be undefined.
get timeLeftText() {
+ if (!this.args.discount?.expiresAt) {
+ return '';
+ }
return formatTimeLeft(this.args.discount.expiresAt, this.time.currentTime);
}📝 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.
| get timeLeftText() { | |
| return formatTimeLeft(this.args.discount.expiresAt, this.time.currentTime); | |
| } | |
| get timeLeftText() { | |
| if (!this.args.discount?.expiresAt) { | |
| return ''; | |
| } | |
| return formatTimeLeft(this.args.discount.expiresAt, this.time.currentTime); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/services/feature-flags.ts (1)
21-23: LGTM! Consider adding JSDoc for consistency.The implementation follows the established pattern and properly handles null checks. Consider adding JSDoc documentation to match other feature flag getters.
+ /** + * Returns whether the current user can see the discount countdown. + * Enabled for staff users and users with the 'can-see-discount-countdown' feature flag set to 'test'. + */ get canSeeDiscountCountdown(): boolean {app/controllers/pay.ts (2)
39-41: LGTM! Consider adding TypeScript return type.The computed property implementation is clean and follows the single responsibility principle.
- get canSeeDiscountCountdown() { + get canSeeDiscountCountdown(): boolean {
39-41: Consider adding analytics tracking for feature usage.Since this is a new user-facing feature, it would be valuable to track its usage and impact. Consider adding analytics events when the countdown is displayed and when users interact with it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/controllers/pay.ts(2 hunks)app/services/feature-flags.ts(1 hunks)app/templates/pay.hbs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/templates/pay.hbs
🔇 Additional comments (2)
app/controllers/pay.ts (2)
11-11: LGTM! Import follows TypeScript best practices.The type import is properly defined and follows the existing pattern.
18-18: LGTM! Service injection follows established pattern.The feature flags service is properly injected using the decorator pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/components/pay-page/signup-discount-notice.hbs (4)
4-4: Add aria-label to the clock icon for better accessibility.The clock icon should have an aria-label to improve accessibility for screen readers.
- {{svg-jar "clock" class="w-6 fill-current text-blue-400 animate-pulse"}} + {{svg-jar "clock" class="w-6 fill-current text-blue-400 animate-pulse" aria-label="Limited time offer"}}
22-22: Document the animate-spin-once class behavior.The animation class
animate-spin-onceshould be documented to help other developers understand its behavior and duration.Consider adding a comment above this line explaining the animation behavior:
+ {{!-- animate-spin-once: Rotates the image 360 degrees once on component mount --}} <img src={{this.iconImage}} alt="treasure" class="w-8 animate-spin-once" />
32-32: Enhance tooltip accessibility and maintainability.The tooltip implementation could be improved in several ways:
- Add keyboard navigation support
- Extract the hardcoded prices to computed properties
- <EmberTooltip @text="The 1 year plan is usually $360, but you can get it for $216 with this offer." @side="bottom" /> + <EmberTooltip + @text={{this.tooltipText}} + @side="bottom" + @showOn="mouseenter focus" + @hideOn="mouseleave blur" + @role="tooltip" + @tabindex="0" + />Consider adding these computed properties to your component class:
@tracked get tooltipText() { const regularPrice = 360; const discountedPrice = regularPrice * 0.6; // 40% off return `The 1 year plan is usually $${regularPrice}, but you can get it for $${discountedPrice} with this offer.`; }
1-34: Consider extracting common styles and making values configurable.The template has duplicated values and styles that could be made more maintainable:
- The 40% discount value is hardcoded in both variants
- Common styles like padding and border radius could be extracted
Consider:
- Making the discount percentage a component argument:
- <span class="font-semibold">40% off</span> + <span class="font-semibold">{{@discountPercentage}}% off</span>
- Creating a shared CSS class for common styles:
.discount-notice-base { @apply flex p-3 rounded shadow-sm relative; }Then update the template:
- <div class="flex p-3 bg-blue-100 border border-blue-300 rounded shadow-sm relative" + <div class="discount-notice-base bg-blue-100 border border-blue-300"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/components/pay-page/referral-discount-notice.hbs(1 hunks)app/components/pay-page/signup-discount-notice.hbs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/pay-page/referral-discount-notice.hbs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (1)
app/components/pay-page/signup-discount-notice.hbs (1)
9-9: Add null check for @discount.expiresAt.The template should handle cases where the expiration date is not available to prevent runtime errors.
- <span class="font-semibold percy-timestamp">{{date-format @discount.expiresAt format="PPPp"}}</span> + {{#if @discount.expiresAt}} + <span class="font-semibold percy-timestamp">{{date-format @discount.expiresAt format="PPPp"}}</span> + {{else}} + <span class="font-semibold">limited time</span> + {{/if}}
Bundle ReportChanges will increase total bundle size by 7.78kB (0.02%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/components/pay-page/referral-discount-notice.ts (1)
22-24:⚠️ Potential issueAdd error handling for expiresAt property.
The timeLeftText getter needs to handle cases where discount.expiresAt might be undefined.
get timeLeftText() { + if (!this.args.discount?.expiresAt) { + return ''; + } return formatTimeLeft(this.args.discount.expiresAt, this.time.currentTime); }tests/unit/utils/time-formatting-test.ts (1)
7-41:⚠️ Potential issueFix formatting to comply with pipeline checks.
Multiple formatting issues were detected by the pipeline.
Apply this diff to fix the formatting:
- // Test hours:minutes:seconds format - assert.strictEqual( - formatTimeLeft(new Date('2024-01-01T02:34:56Z'), currentTime), - '02h:34m:56s', - 'formats hours, minutes and seconds correctly' - ); + assert.strictEqual(formatTimeLeft(new Date('2024-01-01T02:34:56Z'), currentTime), '02h:34m:56s', 'formats hours, minutes and seconds correctly'); // Test minutes:seconds format - assert.strictEqual( - formatTimeLeft(new Date('2024-01-01T00:34:56Z'), currentTime), - '34m:56s', - 'formats minutes and seconds correctly when hours is 0' - ); + assert.strictEqual(formatTimeLeft(new Date('2024-01-01T00:34:56Z'), currentTime), '34m:56s', 'formats minutes and seconds correctly when hours is 0'); // Test seconds only format - assert.strictEqual( - formatTimeLeft(new Date('2024-01-01T00:00:45Z'), currentTime), - '45s', - 'formats seconds only when hours and minutes are 0' - ); + assert.strictEqual(formatTimeLeft(new Date('2024-01-01T00:00:45Z'), currentTime), '45s', 'formats seconds only when hours and minutes are 0'); // Test negative time difference - assert.strictEqual( - formatTimeLeft(new Date('2023-12-31T23:59:59Z'), currentTime), - '00s', - 'returns 00s for expired time' - ); + assert.strictEqual(formatTimeLeft(new Date('2023-12-31T23:59:59Z'), currentTime), '00s', 'returns 00s for expired time'); // Test zero seconds - assert.strictEqual( - formatTimeLeft(new Date('2024-01-01T00:00:00Z'), currentTime), - '00s', - 'formats zero seconds correctly' - ); + assert.strictEqual(formatTimeLeft(new Date('2024-01-01T00:00:00Z'), currentTime), '00s', 'formats zero seconds correctly');🧰 Tools
🪛 eslint
[error] 7-7: Delete
····(prettier/prettier)
[error] 9-13: Replace
⏎······formatTimeLeft(new·Date('2024-01-01T02:34:56Z'),·currentTime),⏎······'02h:34m:56s',⏎······'formats·hours,·minutes·and·seconds·correctly'⏎····withformatTimeLeft(new·Date('2024-01-01T02:34:56Z'),·currentTime),·'02h:34m:56s',·'formats·hours,·minutes·and·seconds·correctly'(prettier/prettier)
[error] 19-19: Insert
,(prettier/prettier)
[error] 23-27: Replace
⏎······formatTimeLeft(new·Date('2024-01-01T00:00:45Z'),·currentTime),⏎······'45s',⏎······'formats·seconds·only·when·hours·and·minutes·are·0'⏎····withformatTimeLeft(new·Date('2024-01-01T00:00:45Z'),·currentTime),·'45s',·'formats·seconds·only·when·hours·and·minutes·are·0'(prettier/prettier)
[error] 30-34: Replace
⏎······formatTimeLeft(new·Date('2023-12-31T23:59:59Z'),·currentTime),⏎······'00s',⏎······'returns·00s·for·expired·time'⏎····withformatTimeLeft(new·Date('2023-12-31T23:59:59Z'),·currentTime),·'00s',·'returns·00s·for·expired·time'(prettier/prettier)
[error] 37-41: Replace
⏎······formatTimeLeft(new·Date('2024-01-01T00:00:00Z'),·currentTime),⏎······'00s',⏎······'formats·zero·seconds·correctly'⏎····withformatTimeLeft(new·Date('2024-01-01T00:00:00Z'),·currentTime),·'00s',·'formats·zero·seconds·correctly'(prettier/prettier)
🪛 GitHub Actions: Test
[error] 7-7: Prettier formatting error: Delete extra whitespace
[error] 9-9: Prettier formatting error: Incorrect line breaks and spacing in formatTimeLeft test case
[error] 19-19: Prettier formatting error: Missing comma
[error] 23-23: Prettier formatting error: Incorrect line breaks and spacing in seconds-only format test case
[error] 30-30: Prettier formatting error: Incorrect line breaks and spacing in expired time test case
[error] 37-37: Prettier formatting error: Incorrect line breaks and spacing in zero seconds test case
🧹 Nitpick comments (1)
tests/unit/utils/time-formatting-test.ts (1)
1-43: Consider adding more edge cases to the test suite.While the current test cases cover basic scenarios, consider adding tests for:
- Invalid date inputs
- Null/undefined inputs
- Large time differences (e.g., days/months)
Example test cases:
// Test invalid inputs assert.strictEqual(formatTimeLeft(null, currentTime), '', 'handles null input gracefully'); assert.strictEqual(formatTimeLeft(undefined, currentTime), '', 'handles undefined input gracefully'); // Test large time differences assert.strictEqual( formatTimeLeft(new Date('2024-01-02T00:00:00Z'), currentTime), '24h:00m:00s', 'formats full day correctly' );🧰 Tools
🪛 eslint
[error] 7-7: Delete
····(prettier/prettier)
[error] 9-13: Replace
⏎······formatTimeLeft(new·Date('2024-01-01T02:34:56Z'),·currentTime),⏎······'02h:34m:56s',⏎······'formats·hours,·minutes·and·seconds·correctly'⏎····withformatTimeLeft(new·Date('2024-01-01T02:34:56Z'),·currentTime),·'02h:34m:56s',·'formats·hours,·minutes·and·seconds·correctly'(prettier/prettier)
[error] 19-19: Insert
,(prettier/prettier)
[error] 23-27: Replace
⏎······formatTimeLeft(new·Date('2024-01-01T00:00:45Z'),·currentTime),⏎······'45s',⏎······'formats·seconds·only·when·hours·and·minutes·are·0'⏎····withformatTimeLeft(new·Date('2024-01-01T00:00:45Z'),·currentTime),·'45s',·'formats·seconds·only·when·hours·and·minutes·are·0'(prettier/prettier)
[error] 30-34: Replace
⏎······formatTimeLeft(new·Date('2023-12-31T23:59:59Z'),·currentTime),⏎······'00s',⏎······'returns·00s·for·expired·time'⏎····withformatTimeLeft(new·Date('2023-12-31T23:59:59Z'),·currentTime),·'00s',·'returns·00s·for·expired·time'(prettier/prettier)
[error] 37-41: Replace
⏎······formatTimeLeft(new·Date('2024-01-01T00:00:00Z'),·currentTime),⏎······'00s',⏎······'formats·zero·seconds·correctly'⏎····withformatTimeLeft(new·Date('2024-01-01T00:00:00Z'),·currentTime),·'00s',·'formats·zero·seconds·correctly'(prettier/prettier)
🪛 GitHub Actions: Test
[error] 7-7: Prettier formatting error: Delete extra whitespace
[error] 9-9: Prettier formatting error: Incorrect line breaks and spacing in formatTimeLeft test case
[error] 19-19: Prettier formatting error: Missing comma
[error] 23-23: Prettier formatting error: Incorrect line breaks and spacing in seconds-only format test case
[error] 30-30: Prettier formatting error: Incorrect line breaks and spacing in expired time test case
[error] 37-37: Prettier formatting error: Incorrect line breaks and spacing in zero seconds test case
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/assets/images/icons/money.svgis excluded by!**/*.svg
📒 Files selected for processing (10)
app/components/pay-page/referral-discount-notice.hbs(1 hunks)app/components/pay-page/referral-discount-notice.ts(1 hunks)app/components/pay-page/signup-discount-notice.hbs(1 hunks)app/components/pay-page/signup-discount-notice.ts(1 hunks)app/controllers/pay.ts(2 hunks)app/services/feature-flags.ts(1 hunks)app/styles/utilities.css(1 hunks)app/templates/pay.hbs(1 hunks)app/utils/time-formatting.ts(1 hunks)tests/unit/utils/time-formatting-test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- app/services/feature-flags.ts
- app/styles/utilities.css
- app/controllers/pay.ts
- app/utils/time-formatting.ts
- app/components/pay-page/signup-discount-notice.hbs
- app/components/pay-page/referral-discount-notice.hbs
- app/components/pay-page/signup-discount-notice.ts
- app/templates/pay.hbs
🧰 Additional context used
🪛 eslint
tests/unit/utils/time-formatting-test.ts
[error] 7-7: Delete ····
(prettier/prettier)
[error] 9-13: Replace ⏎······formatTimeLeft(new·Date('2024-01-01T02:34:56Z'),·currentTime),⏎······'02h:34m:56s',⏎······'formats·hours,·minutes·and·seconds·correctly'⏎···· with formatTimeLeft(new·Date('2024-01-01T02:34:56Z'),·currentTime),·'02h:34m:56s',·'formats·hours,·minutes·and·seconds·correctly'
(prettier/prettier)
[error] 19-19: Insert ,
(prettier/prettier)
[error] 23-27: Replace ⏎······formatTimeLeft(new·Date('2024-01-01T00:00:45Z'),·currentTime),⏎······'45s',⏎······'formats·seconds·only·when·hours·and·minutes·are·0'⏎···· with formatTimeLeft(new·Date('2024-01-01T00:00:45Z'),·currentTime),·'45s',·'formats·seconds·only·when·hours·and·minutes·are·0'
(prettier/prettier)
[error] 30-34: Replace ⏎······formatTimeLeft(new·Date('2023-12-31T23:59:59Z'),·currentTime),⏎······'00s',⏎······'returns·00s·for·expired·time'⏎···· with formatTimeLeft(new·Date('2023-12-31T23:59:59Z'),·currentTime),·'00s',·'returns·00s·for·expired·time'
(prettier/prettier)
[error] 37-41: Replace ⏎······formatTimeLeft(new·Date('2024-01-01T00:00:00Z'),·currentTime),⏎······'00s',⏎······'formats·zero·seconds·correctly'⏎···· with formatTimeLeft(new·Date('2024-01-01T00:00:00Z'),·currentTime),·'00s',·'formats·zero·seconds·correctly'
(prettier/prettier)
🪛 GitHub Actions: Test
tests/unit/utils/time-formatting-test.ts
[error] 7-7: Prettier formatting error: Delete extra whitespace
[error] 9-9: Prettier formatting error: Incorrect line breaks and spacing in formatTimeLeft test case
[error] 19-19: Prettier formatting error: Missing comma
[error] 23-23: Prettier formatting error: Incorrect line breaks and spacing in seconds-only format test case
[error] 30-30: Prettier formatting error: Incorrect line breaks and spacing in expired time test case
[error] 37-37: Prettier formatting error: Incorrect line breaks and spacing in zero seconds test case
🔇 Additional comments (2)
app/components/pay-page/referral-discount-notice.ts (2)
3-6: LGTM! Imports are well-organized.The new imports are correctly structured and necessary for the component's enhanced functionality.
13-13: LGTM! Type definition is clear.The variant type is well-defined with explicit options.
…t for dynamic layouts
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
app/components/pay-page/experimental-signup-discount-notice.hbs (3)
2-5: Consider adding a more descriptive data-test attribute or additional ARIA attributes for improved testability and accessibility.
By specifying a more meaningful test attribute (e.g.,data-test-newuser-discount) or adding ARIA labels, you can enhance both automated testing and the overall accessibility of the component.
7-7: Use more descriptive alt text for improved accessibility.
Currently, the alt attribute is set to "treasure." If this image is purely decorative, addalt="". If it conveys meaning, leverage more specific text (e.g.,"Promotional discount icon") to help screen readers.
10-16: Allow dynamic discount values rather than hardcoding 40% for maintainability.
If you plan to offer multiple discount rates, consider binding the displayed discount percentage to a property from the backing code (e.g., the model).- <span class="font-bold">40% off</span> + <span class="font-bold">{{@discount.discountPercentage}} off</span>app/components/pay-page/experimental-referral-discount-notice.ts (1)
1-6: Consider extracting shared logic from both discount notice components to avoid duplication.
The structure and usage offormatTimeLeft,@service time: TimeService, and the discount data model closely mirror the signup discount notice. Refactoring into a shared base class or utility can reduce maintenance overhead.app/components/pay-page/experimental-referral-discount-notice.hbs (1)
10-23: Extract the discount percentage from the model or pass it as an argument for easier reuse.
Similar to the signup discount notice, avoid hardcoding “40% off” in the template. Make it dynamic so you can adjust the discount without modifying multiple files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/components/pay-page/experimental-referral-discount-notice.hbs(1 hunks)app/components/pay-page/experimental-referral-discount-notice.ts(1 hunks)app/components/pay-page/experimental-signup-discount-notice.hbs(1 hunks)app/components/pay-page/experimental-signup-discount-notice.ts(1 hunks)app/templates/pay.hbs(1 hunks)tests/unit/utils/time-formatting-test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/templates/pay.hbs
- tests/unit/utils/time-formatting-test.ts
🔇 Additional comments (4)
app/components/pay-page/experimental-signup-discount-notice.ts (2)
16-17: Good approach for maintaining a dedicated icon expression.
Having a directiconImageproperty and referencing the imported asset helps keep the code clear and flexible for potential future changes.
20-23: Validate expiration logic and guard against null or invalid dates.
Ifthis.args.discount.expiresAtcan potentially benullor malformed, consider adding a check to avoid runtime errors.get timeLeftText() { - return formatTimeLeft(this.args.discount.expiresAt, this.time.currentTime); + if (!this.args.discount.expiresAt) { + return 'N/A'; + } + return formatTimeLeft(this.args.discount.expiresAt, this.time.currentTime); }app/components/pay-page/experimental-referral-discount-notice.ts (1)
19-23: Verify handling of expired discounts to ensure correct user experience.
Ifthis.args.discount.expiresAtis already past, confirm that the UI gracefully handles this case (e.g., displaying an expired message instead of time left).app/components/pay-page/experimental-referral-discount-notice.hbs (1)
24-31: Looks good at first glance!
The tooltip content and container styling are consistent with the rest of the component. The usage ofEmberTooltipis appropriate for clarifying the discount details.
|
Ready to merge. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/components/pay-page/experimental-signup-discount-notice.ts (1)
1-29: Consider extracting common functionality into a base component.This component shares significant code with
experimental-referral-discount-notice.ts. Consider:
- Creating a base discount notice component
- Extending it for specific discount types
Here's how you could refactor this:
// app/components/pay-page/base-discount-notice.ts import Component from '@glimmer/component'; import type PromotionalDiscountModel from 'codecrafters-frontend/models/promotional-discount'; import { service } from '@ember/service'; import type TimeService from 'codecrafters-frontend/services/time'; import { formatTimeDurationForCoundown } from 'codecrafters-frontend/utils/time-formatting'; interface BaseSignature { Element: HTMLDivElement; Args: { discount: PromotionalDiscountModel; }; } export default class BaseDiscountNoticeComponent extends Component<BaseSignature> { @service declare time: TimeService; get timeLeftText() { return formatTimeDurationForCoundown(this.args.discount.expiresAt, this.time.currentTime); } } // app/components/pay-page/experimental-signup-discount-notice.ts import BaseDiscountNoticeComponent from './base-discount-notice'; import iconImage from '/assets/images/icons/money.svg'; export default class SignupDiscountNoticeComponent extends BaseDiscountNoticeComponent { iconImage = iconImage; }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 17-17: app/components/pay-page/experimental-signup-discount-notice.ts#L17
Added line #L17 was not covered by tests
[warning] 22-22: app/components/pay-page/experimental-signup-discount-notice.ts#L22
Added line #L22 was not covered by testsapp/utils/time-formatting.ts (2)
5-7: Improve variable naming and calculation efficiency.The time calculations could be more efficient and clearer:
- Use more descriptive variable names
- Use modulo operations for cleaner calculations
- const hoursLeft = Math.floor(distanceInSeconds / 60 / 60); - const minutesLeft = Math.floor(distanceInSeconds / 60) - hoursLeft * 60; - const secondsLeft = distanceInSeconds - hoursLeft * 60 * 60 - minutesLeft * 60; + const totalHours = Math.floor(distanceInSeconds / 3600); + const totalMinutes = Math.floor((distanceInSeconds % 3600) / 60); + const remainingSeconds = distanceInSeconds % 60;
13-22: Consider adding debug context to Sentry message.The Sentry message could include more context about where the negative time occurred.
Sentry.captureMessage(`Received negative time duration for discount countdown notice`, { extra: { expiryDate: laterDate, currentDate: earlierDate, + timeDifference: distanceInSeconds, + component: 'time-formatting-util', }, });tests/unit/utils/time-formatting-test.ts (1)
9-34: Maintain consistent assertion formatting style.The test file mixes multi-line and single-line assertion styles. Consider using a consistent style throughout.
- assert.strictEqual( - formatTimeDurationForCoundown(new Date('2024-01-01T02:34:56Z'), currentTime), - '02h:34m:56s', - 'formats hours, minutes and seconds correctly', - ); + assert.strictEqual( + formatTimeDurationForCoundown(new Date('2024-01-01T02:34:56Z'), currentTime), + '02h:34m:56s', + 'formats hours, minutes and seconds correctly' + ); // Test minutes:seconds format - assert.strictEqual(formatTimeDurationForCoundown(new Date('2023-12-31T23:59:59Z'), currentTime), '00s', 'returns 00s for expired time'); + assert.strictEqual( + formatTimeDurationForCoundown(new Date('2023-12-31T23:59:59Z'), currentTime), + '00s', + 'returns 00s for expired time' + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/components/pay-page/experimental-referral-discount-notice.ts(1 hunks)app/components/pay-page/experimental-signup-discount-notice.ts(1 hunks)app/templates/pay.hbs(1 hunks)app/utils/time-formatting.ts(1 hunks)tests/unit/utils/time-formatting-test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/templates/pay.hbs
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/components/pay-page/experimental-referral-discount-notice.ts
[warning] 18-18: app/components/pay-page/experimental-referral-discount-notice.ts#L18
Added line #L18 was not covered by tests
[warning] 23-23: app/components/pay-page/experimental-referral-discount-notice.ts#L23
Added line #L23 was not covered by tests
app/components/pay-page/experimental-signup-discount-notice.ts
[warning] 17-17: app/components/pay-page/experimental-signup-discount-notice.ts#L17
Added line #L17 was not covered by tests
[warning] 22-22: app/components/pay-page/experimental-signup-discount-notice.ts#L22
Added line #L22 was not covered by tests
🔇 Additional comments (1)
app/components/pay-page/experimental-referral-discount-notice.ts (1)
1-30: Add test coverage for the component.The component lacks test coverage, particularly for the
timeLeftTextcomputed property and icon image usage.Run this script to verify test coverage:
#!/bin/bash # Check for existing component tests echo "=== Component Tests ===" fd -e test.ts -e spec.ts . tests/integration/components/pay-page | xargs rg "ReferralDiscountNoticeComponent" echo -e "\n=== Component Usage ===" rg "PayPage::ExperimentalReferralDiscountNotice" app/templates/🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 18-18: app/components/pay-page/experimental-referral-discount-notice.ts#L18
Added line #L18 was not covered by tests
[warning] 23-23: app/components/pay-page/experimental-referral-discount-notice.ts#L23
Added line #L23 was not covered by tests
| import Component from '@glimmer/component'; | ||
| import PromotionalDiscountModel from 'codecrafters-frontend/models/promotional-discount'; | ||
| import iconImage from '/assets/images/icons/money.svg'; | ||
| import { service } from '@ember/service'; | ||
| import type TimeService from 'codecrafters-frontend/services/time'; | ||
| import { formatTimeDurationForCoundown } from 'codecrafters-frontend/utils/time-formatting'; | ||
| interface Signature { | ||
| Element: HTMLDivElement; | ||
|
|
||
| Args: { | ||
| discount: PromotionalDiscountModel; | ||
| }; | ||
| } | ||
|
|
||
| export default class SignupDiscountNoticeComponent extends Component<Signature> { | ||
| iconImage = iconImage; | ||
|
|
||
| @service declare time: TimeService; | ||
|
|
||
| get timeLeftText() { | ||
| return formatTimeDurationForCoundown(this.args.discount.expiresAt, this.time.currentTime); | ||
| } | ||
| } | ||
|
|
||
| declare module '@glint/environment-ember-loose/registry' { | ||
| export default interface Registry { | ||
| 'PayPage::ExperimentalSignupDiscountNotice': typeof SignupDiscountNoticeComponent; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add integration tests for the experimental signup discount notice component
The core time formatting utility is well-tested, but we need integration tests to verify:
- TimeService integration and countdown updates
- Proper handling of discount expiration times
- Icon rendering and component layout
Example test locations:
- Create
tests/integration/components/pay-page/experimental-signup-discount-notice-test.ts - Reference
tests/integration/helpers/date-from-now-test.jsfor TimeService testing patterns
🔗 Analysis chain
Add test coverage for the component.
The component lacks test coverage, particularly for the timeLeftText computed property and icon image usage.
Run this script to verify test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing component tests
echo "=== Component Tests ==="
fd -e test.ts -e spec.ts . tests/integration/components/pay-page | xargs rg "SignupDiscountNoticeComponent"
echo -e "\n=== Component Usage ==="
rg "PayPage::ExperimentalSignupDiscountNotice" app/templates/
Length of output: 1103
Script:
#!/bin/bash
# Search for any test files related to discount notice
echo "=== Test Files ==="
fd -e test.ts -e spec.ts . tests/ | xargs rg -l "discount.*notice"
echo -e "\n=== Original Component Content ==="
cat app/components/pay-page/signup-discount-notice.ts
echo -e "\n=== Component Differences ==="
diff app/components/pay-page/signup-discount-notice.ts app/components/pay-page/experimental-signup-discount-notice.ts
Length of output: 2037
Script:
#!/bin/bash
# Check time-formatting utility tests and similar components
echo "=== Time Formatting Tests ==="
fd -e test.ts -e spec.ts . tests/ | xargs rg "formatTimeDurationForCoundown"
echo -e "\n=== Similar Component Tests ==="
fd -e test.ts -e spec.ts . tests/ | xargs rg -l "TimeService"
Length of output: 2174
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 17-17: app/components/pay-page/experimental-signup-discount-notice.ts#L17
Added line #L17 was not covered by tests
[warning] 22-22: app/components/pay-page/experimental-signup-discount-notice.ts#L22
Added line #L22 was not covered by tests
…rate expiration checks
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/models/promotional-discount.ts (1)
16-17: Injecting the service is consistent with Ember conventions
Declaring thetimeservice using@serviceis aligned with the Ember framework's recommended patterns. Make sure all consumers of this model are prepared to handle cases where this might be undefined or mocked in tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/models/promotional-discount.ts(2 hunks)
🔇 Additional comments (1)
app/models/promotional-discount.ts (1)
4-4: Type-only import usage is good
Usingimport typefor theTimeServiceis a neat approach to keep the codebase type-safe and ensure no side effects occur during import.
app/models/promotional-discount.ts
Outdated
| // short-circuit, prevent re-computation | ||
| if (this.expiresAt < new Date()) { | ||
| return false; | ||
| } | ||
|
|
||
| return this.expiresAt < this.time.currentTime; |
There was a problem hiding this comment.
Reevaluate the expiration logic
Currently, if expiresAt is in the past (expiresAt < new Date()), the code returns false, indicating “not expired.” This appears counterintuitive. Typically, an expiration time in the past means it should be considered expired. Consider flipping the condition or adjusting the naming to match the intended behavior.
Below is a suggested diff demonstrating a more conventional approach to “expired”:
- if (this.expiresAt < new Date()) {
- return false;
- }
- return this.expiresAt < this.time.currentTime;
+ if (this.expiresAt < this.time.currentTime) {
+ return true; // it's expired
+ }
+ return false; // not expired📝 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.
| // short-circuit, prevent re-computation | |
| if (this.expiresAt < new Date()) { | |
| return false; | |
| } | |
| return this.expiresAt < this.time.currentTime; | |
| if (this.expiresAt < this.time.currentTime) { | |
| return true; // it's expired | |
| } | |
| return false; // not expired |
Prototype for https://linear.app/codecrafters/issue/CC-1595/add-discount-countdown.
Summary by CodeRabbit
Release Notes
New Features
UI/UX Improvements
Technical Enhancements
Testing
This is part 1 of 2 in a stack made with GitButler: