feat: Renewal reminder emails and improved subscription notes display (v0.4.8) #58
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "v0.4.8"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
🎉 New Features
Renewal Reminder Emails
Improved Subscription Notes Display
🔧 Technical Changes
SendRenewalReminder()method to EmailServiceGetSubscriptionsNeedingReminders()method to SubscriptionService🧪 Testing
📝 Files Changed
cmd/server/main.go- Scheduler implementationinternal/service/email.go- Email reminder methodinternal/service/subscription.go- Reminder query methodinternal/service/renewal_reminder_test.go- Test suitetemplates/subscriptions.html- Tooltip UItemplates/subscription-list.html- Tooltip UISee RELEASE_NOTES_v0.4.8.md for full details.
Pull Request Overview
This PR adds automatic renewal reminder emails and improves the subscription notes display UI. The renewal reminder feature includes a background scheduler that checks daily for upcoming renewals and sends email notifications. The notes display has been enhanced with hover tooltips instead of separate table rows for a cleaner interface.
Key changes:
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The ticker is never stopped, causing a resource leak. The ticker should be stored and stopped when the application shuts down (e.g., using context cancellation or graceful shutdown). Additionally, the scheduler runs every 24 hours from startup, not at midnight as mentioned in the comment on line 338.
Duplicate reminders will be sent for the same subscription on consecutive days if it remains within the reminder window. For example, a subscription renewing in 5 days will receive a reminder today, tomorrow (4 days), the next day (3 days), etc. Consider tracking when the last reminder was sent to avoid sending duplicate reminders for the same renewal period.
@ -161,0 +173,4 @@<div id="note-tooltip-{{.ID}}" class="absolute right-0 bottom-full mb-2 w-auto min-w-0 p-1.5 bg-gray-900 dark:bg-gray-700 text-white dark:text-gray-100 text-xs rounded-lg shadow-lg opacity-0 invisible group-hover:opacity-100 group-hover:visible group-focus-within:opacity-100 group-focus-within:visible transition-all duration-200 z-10 whitespace-nowrap"><p>{{.Notes}}</p><div class="absolute top-full right-4 w-0 h-0 border-l-4 border-r-4 border-t-4 border-transparent border-t-gray-900 dark:border-t-gray-700"></div></div>The button lacks an accessible label for screen readers. The tooltip on hover is not accessible to keyboard-only users or screen reader users. Add an aria-label attribute to the button and consider making the tooltip keyboard-accessible using aria-describedby or implementing keyboard focus behavior.
@ -341,38 +341,49 @@</td>The button lacks an accessible label for screen readers. The tooltip on hover is not accessible to keyboard-only users or screen reader users. Add an aria-label attribute to the button and consider making the tooltip keyboard-accessible using aria-describedby or implementing keyboard focus behavior.
Pull Request Overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@ -323,3 +325,91 @@ func setupRoutes(router *gin.Engine, handler *handlers.SubscriptionHandler, settv1.GET("/export/json", handler.ExportJSON)The scheduler will send duplicate reminders every day for the same subscription if it remains within the reminder window. For example, a subscription renewing in 5 days will receive a reminder today, tomorrow (4 days), the next day (3 days), etc.
Consider adding a tracking mechanism (e.g.,
last_reminder_sent_atfield on the Subscription model) to ensure each subscription only receives one reminder per renewal cycle. Alternatively, only send reminders whendaysUntilequals a specific value (e.g., exactly 7 days, 3 days, and 1 day).The scheduler doesn't run at midnight as the comment states. It runs 24 hours after the application starts. If the app starts at 2 PM, reminders will be sent at 2 PM daily, not at midnight.
To run at midnight, consider using a proper cron-like scheduler or calculating the time until the next midnight:
@ -212,0 +220,4 @@data := AlertData{Subscription: subscription,CurrencySymbol: currencySymbol,}The email template hardcodes the
$currency symbol, which doesn't respect the user's configured currency preference. This is inconsistent with the rest of the application which uses the currency setting fromSettingsService.GetCurrencySymbol().Consider passing the currency symbol to the email template:
Then use
{{.CurrencySymbol}}in the template instead of$.@ -93,3 +94,46 @@ func (s *SubscriptionService) GetStats() (*models.Stats, error) {func (s *SubscriptionService) GetAllCategories() ([]models.Category, error) {The calculation
int(sub.RenewalDate.Sub(now).Hours() / 24)truncates hours to days, which could lead to incorrect day counts. For example, if a renewal is in 23 hours (0.95 days), it will be counted as 0 days, but if it's in 25 hours (1.04 days), it will be counted as 1 day.Consider using
time.Until()with proper rounding:Or for more precise date-based calculation:
@ -161,0 +164,4 @@class="text-gray-400 dark:text-gray-500 hover:text-gray-600 dark:hover:text-gray-300 transition-colors duration-150"aria-label="View note"aria-describedby="note-tooltip-{{.ID}}"title="View note">The button has
aria-describedby="note-tooltip-{{.ID}}"pointing to the tooltip div, but this is semantically incorrect. Thearia-describedbyattribute should reference descriptive text, not interactive content. Additionally, the button doesn't actually do anything when clicked - it only works on hover.Consider either:
<span>withrole="img") if it's purely visualaria-labelalone withoutaria-describedbyif the tooltip is purely supplementary@ -341,38 +341,49 @@</td>The tooltip uses
whitespace-nowrapwhich prevents text wrapping and could cause very long notes to overflow off-screen or be cut off. This creates a poor user experience for subscriptions with lengthy notes.Consider either:
whitespace-nowrapand setting amax-width(e.g.,max-w-xsormax-w-sm) to allow wrappingoverflow-hidden text-ellipsiswith a reasonable max-width to truncate long notes with ellipsis@ -349,0 +351,4 @@<svg class="w-4 h-4" fill="none" stroke="currentColor" viewBox="0 0 24 24"><path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M15 12a3 3 0 11-6 0 3 3 0 016 0z"></path><path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M2.458 12C3.732 7.943 7.523 5 12 5c4.478 0 8.268 2.943 9.542 7-1.274 4.057-5.064 7-9.542 7-4.477 0-8.268-2.943-9.542-7z"></path></svg>The button has
aria-describedby="note-tooltip-{{.ID}}"pointing to the tooltip div, but this is semantically incorrect. Thearia-describedbyattribute should reference descriptive text, not interactive content. Additionally, the button doesn't actually do anything when clicked - it only works on hover.Consider either:
<span>withrole="img") if it's purely visualaria-labelalone withoutaria-describedbyif the tooltip is purely supplementaryPull Request Overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@ -49,0 +57,4 @@GIT_COMMIT=$(git rev-parse --short HEAD)echo "tag=$GIT_TAG" >> $GITHUB_OUTPUTecho "commit=$GIT_COMMIT" >> $GITHUB_OUTPUTThe workflow uses
git describe --tagsbut the checkout action doesn't fetch tags or full git history. By default,actions/checkout@v4only fetches the last commit withfetch-depth: 1. This will causegit describe --tags --abbrev=0to fail and always return "dev".Add
fetch-depth: 0to the checkout step to fetch full git history and tags:@ -56,0 +64,4 @@GIT_COMMIT=$(git rev-parse --short HEAD)echo "tag=$GIT_TAG" >> $GITHUB_OUTPUTecho "commit=$GIT_COMMIT" >> $GITHUB_OUTPUTThe workflow uses
git describe --tagsbut the checkout action doesn't fetch tags or full git history. By default,actions/checkout@v4only fetches the last commit withfetch-depth: 1. This will causegit describe --tags --abbrev=0to fail and always return "dev".Add
fetch-depth: 0to the checkout step to fetch full git history and tags:@ -323,3 +325,91 @@ func setupRoutes(router *gin.Engine, handler *handlers.SubscriptionHandler, settv1.GET("/export/json", handler.ExportJSON)This implementation will send duplicate reminders daily for the same subscription until it renews. For example, if a subscription renews in 5 days, it will receive a reminder today (5 days), tomorrow (4 days), the next day (3 days), etc.
Consider tracking which reminders have been sent (e.g., adding a
last_reminder_sentfield to the subscription model) to avoid sending multiple reminders for the same renewal period, or only send reminders at specific intervals (e.g., only when exactly 7, 3, and 1 days remain).@ -93,3 +94,46 @@ func (s *SubscriptionService) GetStats() (*models.Stats, error) {func (s *SubscriptionService) GetAllCategories() ([]models.Category, error) {The days calculation using
int(sub.RenewalDate.Sub(now).Hours() / 24)is imprecise and can lead to off-by-one errors due to floating-point truncation. For example, 23.5 hours will be calculated as 0 days when it should be considered as "renewing today".Consider using a more precise date-based calculation:
@ -349,0 +361,4 @@{{end}}<buttonhx-get="/form/subscription/{{.ID}}"hx-target="#modal-content"The tooltip button is not keyboard accessible. The button element doesn't handle keyboard interactions (Enter/Space keys) and relies solely on CSS
:hoverfor tooltip display. Users navigating with keyboards won't be able to view the tooltip content.Consider adding JavaScript to handle keyboard events or using a
tabindex="0"along with:focuspseudo-class in addition to:hoverfor the tooltip display. Thegroup-focus-withinclass is present but may not work as expected without proper focus handling on the button.Pull Request Overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The release notes claim "Auto-sizing tooltip: Tooltip width automatically adjusts to match the note text length" (line 15), but the implementation uses
whitespace-nowrapwhich prevents wrapping and could cause the tooltip to extend beyond the viewport for very long notes. This is misleading as it suggests the tooltip intelligently sizes itself, when in reality it grows indefinitely with the text length.@ -323,3 +325,91 @@ func setupRoutes(router *gin.Engine, handler *handlers.SubscriptionHandler, settv1.GET("/export/json", handler.ExportJSON)There's no mechanism to prevent sending duplicate reminder emails for the same subscription on the same day. If the server restarts multiple times in a day, or if the scheduler function is triggered multiple times, users could receive multiple reminder emails for the same subscription.
Consider adding a "last_reminder_sent" timestamp field to track when reminders were last sent, or implement a daily deduplication mechanism using a cache or database flag.
The scheduler runs every 24 hours from server startup time, not at a specific time of day (e.g., midnight). This means if the server starts at 3 PM, reminders will be sent at 3 PM daily instead of at a consistent time like midnight.
Consider using a proper cron-like scheduler or calculating the time until the next midnight:
@ -96,0 +111,4 @@result := make(map[*models.Subscription]int)for i := range subscriptions {sub := &subscriptions[i]The
GetUpcomingRenewalsrepository method doesn't preload theCategoryrelationship, which could cause issues when the email template tries to access.Category.Namein line 261 of email.go. Other similar repository methods likeGetActiveSubscriptionsandGetCancelledSubscriptionsuse.Preload("Category").Add
.Preload("Category")to the query in the repository method:[nitpick] The function returns "dev" as a fallback when both Version and GitCommit are empty or their default values, but this could mask deployment issues where version information fails to be injected during build.
Consider logging a warning when falling back to "dev" in production environments, or returning an error to make version injection failures more visible during deployment.
Pull Request Overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
internal/repository/subscription.go:248
GetUpcomingRenewalsmethod doesn't preload the Category association, but the email template inSendRenewalReminder(line 261 of email.go) accesses.Category.Name. This will cause N+1 query issues when sending multiple reminders. Add.Preload(\"Category\")to the query chain, similar toGetActiveSubscriptions()at line 226.💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The release notes incorrectly state 'No database migrations required', but the code includes a new migration function
migrateReminderTrackingininternal/database/migrations.gothat adds two new columns (last_reminder_sentandlast_reminder_renewal_date) to the subscriptions table.The comment says 'run daily at midnight' but the ticker runs every 24 hours from the server start time, not at a specific time like midnight. If the server starts at 3 PM, reminders will run at 3 PM daily. Update the comment to accurately reflect this behavior, or implement actual midnight scheduling if that's the intended design.
@ -349,0 +362,4 @@<buttonhx-get="/form/subscription/{{.ID}}"hx-target="#modal-content"hx-trigger="click"The tooltip HTML structure (lines 344-361) is duplicated identically in both
subscriptions.htmlandsubscription-list.html(lines 161-178). Consider extracting this into a reusable template partial or component to improve maintainability and ensure consistency if changes are needed in the future.Pull Request Overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Missing documentation for the new
HighCostThresholdfield in the settings. The UI change adds a configurable threshold input, but this feature is not mentioned in the release notes or feature list. Users upgrading to v0.4.8 won't be aware of this new capability.The scheduler spawns two separate goroutines (lines 333 and 340) that could potentially run
checkAndSendRenewalRemindersconcurrently. If the initial check (after 30 seconds) takes longer than expected, it might still be running when the ticker fires. While the current implementation has panic recovery, concurrent execution could lead to duplicate reminder emails being sent. Consider using a mutex or checking if a reminder run is already in progress before starting a new one.The migration silently continues even if adding columns fails (using
log.Printfwith "Note:" prefix). This could lead to runtime errors if the application expects these columns but they don't exist. Consider returning the error or at least using a more prominent warning level, especially since these fields are critical for the renewal reminder feature to work correctly.@ -209,1 +209,4 @@case "threshold":thresholdStr := c.PostForm("high_cost_threshold")if threshold, err := strconv.ParseFloat(thresholdStr, 64); err == nil && threshold >= 0 && threshold <= 10000 {[nitpick] The validation allows threshold values between 0 and 10000, but a threshold of 0 doesn't make practical sense (every subscription would be considered high-cost). Consider using a minimum value of 0.01 or 1.0 instead, or document that 0 effectively disables high-cost alerts.
@ -83,6 +83,35 @@ func (h *SubscriptionHandler) enrichWithCurrencyConversion(subscriptions []modelreturn resultThe error handling on line 104 falls back to direct comparison when currency conversion fails, but this could produce incorrect results. If a subscription's monthly cost is 40 EUR and the threshold is 50 USD, failing to convert would compare 40 > 50 (false), when the actual converted value might be 43 USD > 50 USD (still false) or vice versa depending on rates. Consider either returning an error or using a more conservative approach (e.g., always treating as high-cost when conversion fails, to avoid missing alerts).
@ -26,6 +26,8 @@ type Subscription struct {Notes string `json:"notes" gorm:""`[nitpick] The comment on line 29 says this field "Tracks when last reminder was sent for current renewal date" but it actually tracks when the last reminder was sent, regardless of renewal date. The field that tracks which renewal date the reminder was for is
LastReminderRenewalDate. Consider clarifying this comment to: "Tracks when the last reminder was sent" to avoid confusion.@ -212,0 +220,4 @@data := AlertData{Subscription: subscription,CurrencySymbol: currencySymbol,}The email template uses a hard-coded dollar sign ($) on line 259, but the application supports multiple currencies through the
CurrencySymbolsetting. The email should use the user's configured currency symbol instead of assuming USD. Consider passing the currency symbol to the template or using a currency-aware formatting function.@ -0,0 +89,4 @@},},expectedCount: 1,description: "Should find subscription renewing today (within 24 hours)",[nitpick] The test description on line 92 says "Should find subscription renewing today (within 24 hours)" but the actual test creates a subscription renewing in 12 hours. The logic in the service would calculate this as
int(12 hours / 24) = 0 days, which is correct. However, this highlights that the "0 days" case means "renews within the next 24 hours" rather than "renews today" in calendar terms. Consider clarifying the test description to reflect this hour-based calculation rather than day-based.@ -96,0 +119,4 @@// Calculate days until renewal using proper date arithmetic// Use time.Until for more accurate calculation (handles timezone differences better)daysUntil := int(time.Until(*sub.RenewalDate).Hours() / 24)[nitpick] The function iterates through all subscriptions returned by
GetUpcomingRenewals()and creates pointers to array elements (sub := &subscriptions[i]). These pointers are then stored in the result map. This is safe, but note that the pointers reference elements in thesubscriptionsslice which lives on the stack/heap within this function. Since the map is returned, the slice won't be garbage collected. Consider whether it would be clearer to haveGetUpcomingRenewals()return[]*models.Subscriptiondirectly, or document this pointer behavior.@ -96,0 +134,4 @@result[sub] = daysUntil}}The duplicate check on lines 127-132 verifies if a reminder was already sent for the same renewal date, but this logic may fail if the renewal date changes slightly (e.g., by a few hours due to timezone adjustments). The
Equal()method checks exact timestamp equality. Consider comparing just the date portion (year, month, day) instead of the full timestamp to handle cases where the renewal date time component might vary slightly.@ -8,2 +6,4 @@// Version is the semantic version tag that will be set at build timeVersion = "dev")[nitpick] The logic prioritizes semantic version over git commit, but the default value for
Versionchanged from "v0.4.5" to "dev". This means existing deployments or builds without proper version injection will show "dev" instead of a meaningful version. Consider keeping a fallback to a last-known version (e.g., "v0.4.8") or document that proper build-time version injection is now required.Pull Request Overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@ -349,0 +352,4 @@<path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M15 12a3 3 0 11-6 0 3 3 0 016 0z"></path><path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M2.458 12C3.732 7.943 7.523 5 12 5c4.478 0 8.268 2.943 9.542 7-1.274 4.057-5.064 7-9.542 7-4.477 0-8.268-2.943-9.542-7z"></path></svg></button>The tooltip implementation could benefit from improved keyboard accessibility. Currently, the button triggers the tooltip on hover/focus (via CSS), but screen reader users and keyboard-only users may have difficulty accessing the tooltip content.
Consider the following improvements:
type="button"to the button to prevent form submission if placed in a form contextrole="tooltip"to the tooltip div for better semantic meaningExample improvement:
@ -212,0 +220,4 @@data := AlertData{Subscription: subscription,CurrencySymbol: currencySymbol,}@copilot open a new pull request to apply changes based on this feedback
@bscott I've opened a new pull request, #59, to work on those changes. Once the pull request is ready, I'll request review from you.
@ -212,0 +220,4 @@data := AlertData{Subscription: subscription,CurrencySymbol: currencySymbol,}@copilot open a new pull request to apply changes based on this feedback
@bscott I've opened a new pull request, #60, to work on those changes. Once the pull request is ready, I'll request review from you.
@ -56,0 +64,4 @@GIT_COMMIT=$(git rev-parse --short HEAD)echo "tag=$GIT_TAG" >> $GITHUB_OUTPUTecho "commit=$GIT_COMMIT" >> $GITHUB_OUTPUT@copilot open a new pull request to apply changes based on this feedback
@bscott I've opened a new pull request, #61, to work on those changes. Once the pull request is ready, I'll request review from you.
@ -49,0 +57,4 @@GIT_COMMIT=$(git rev-parse --short HEAD)echo "tag=$GIT_TAG" >> $GITHUB_OUTPUTecho "commit=$GIT_COMMIT" >> $GITHUB_OUTPUT@copilot open a new pull request to apply changes based on this feedback
@bscott I've opened a new pull request, #62, to work on those changes. Once the pull request is ready, I'll request review from you.