v0.4.7: Mobile Navigation & Code Quality Improvements #56

2025-11-14T14:29:59-05:00

bscott commented

2025-11-14 14:29:59 -05:00

(Migrated from github.com)

🎉 What's New

Hamburger Menu: Implemented responsive hamburger menu for mobile devices
Mobile Menu Overlay: Slide-in panel with backdrop for easy navigation
Full Navigation Access: All pages (Dashboard, Subscriptions, Analytics, Calendar, Settings) now have mobile menu support
Add Subscription: Accessible directly from mobile menu
Keyboard Support: Escape key closes mobile menu
Body Scroll Lock: Prevents background scrolling when menu is open
Dark Mode: Full dark mode support for mobile menu

🔧 Code Quality Improvements (Resolves #37)

Date Parsing Refactor: Extracted duplicated date parsing logic into reusable helper function
Error Logging: Added logging for invalid date format submissions
Code Reduction: Reduced code duplication by 83% (36 lines → 6 lines)
Maintainability: Centralized date parsing logic for easier maintenance and consistency
Unit Tests: Added comprehensive test coverage for parseDatePtr() function

🎨 UI Improvements

Updated README with mobile screenshot
Updated dashboard screenshot with current dark mode view
Consistent mobile experience across all pages
Improved mobile responsiveness

🔧 Technical Details

Updated templates: analytics.html, calendar.html, dashboard.html, settings.html, subscriptions.html
Added mobile menu JavaScript functions
Responsive breakpoints using Tailwind CSS md: classes
Created parseDatePtr() helper function in subscription handlers
Refactored CreateSubscription and UpdateSubscription to use centralized date parsing
Added internal/handlers/subscription_test.go with unit tests

✅ Testing

Playwright tests completed for desktop and mobile viewports
Unit tests added and passing for date parsing functionality
Manual testing verified on all pages

Closes #39
Closes #37

## 🎉 What's New ### 📱 Mobile Navigation (Resolves #39) - **Hamburger Menu**: Implemented responsive hamburger menu for mobile devices - **Mobile Menu Overlay**: Slide-in panel with backdrop for easy navigation - **Full Navigation Access**: All pages (Dashboard, Subscriptions, Analytics, Calendar, Settings) now have mobile menu support - **Add Subscription**: Accessible directly from mobile menu - **Keyboard Support**: Escape key closes mobile menu - **Body Scroll Lock**: Prevents background scrolling when menu is open - **Dark Mode**: Full dark mode support for mobile menu ### 🔧 Code Quality Improvements (Resolves #37) - **Date Parsing Refactor**: Extracted duplicated date parsing logic into reusable helper function - **Error Logging**: Added logging for invalid date format submissions - **Code Reduction**: Reduced code duplication by 83% (36 lines → 6 lines) - **Maintainability**: Centralized date parsing logic for easier maintenance and consistency - **Unit Tests**: Added comprehensive test coverage for `parseDatePtr()` function ### 🎨 UI Improvements - Updated README with mobile screenshot - Updated dashboard screenshot with current dark mode view - Consistent mobile experience across all pages - Improved mobile responsiveness ## 🔧 Technical Details - Updated templates: analytics.html, calendar.html, dashboard.html, settings.html, subscriptions.html - Added mobile menu JavaScript functions - Responsive breakpoints using Tailwind CSS md: classes - Created `parseDatePtr()` helper function in subscription handlers - Refactored `CreateSubscription` and `UpdateSubscription` to use centralized date parsing - Added `internal/handlers/subscription_test.go` with unit tests ## ✅ Testing - Playwright tests completed for desktop and mobile viewports - Unit tests added and passing for date parsing functionality - Manual testing verified on all pages ## 📝 Related Issues - Closes #39 - Closes #37

🎉 1

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed

2025-11-14 14:34:38 -05:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull Request Overview

This PR adds mobile navigation with a hamburger menu and refactors date parsing logic to improve code quality. The mobile menu provides full navigation access on mobile devices with keyboard support and dark mode compatibility.

Key Changes

Mobile hamburger menu implementation across all 5 pages with slide-in navigation panel
Refactored date parsing into a centralized parseDatePtr() helper function with comprehensive unit tests
Updated README with mobile and calendar screenshots

Reviewed Changes

Copilot reviewed 8 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
templates/subscriptions.html	Added mobile menu overlay, hamburger button, and JavaScript handlers
templates/settings.html	Added mobile menu overlay, hamburger button, and JavaScript handlers
templates/dashboard.html	Added mobile menu overlay, hamburger button, and JavaScript handlers
templates/calendar.html	Added mobile menu overlay, hamburger button, and JavaScript handlers
templates/analytics.html	Added mobile menu overlay, hamburger button, and JavaScript handlers
internal/handlers/subscription.go	Added `parseDatePtr()` helper function to centralize date parsing logic
internal/handlers/subscription_test.go	Added comprehensive unit tests for `parseDatePtr()` function
README.md	Added mobile and calendar screenshots, updated feature descriptions
mobile-screenshot.png	New mobile view screenshot
calendar-screenshot.png	New calendar view screenshot

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

## Pull Request Overview This PR adds mobile navigation with a hamburger menu and refactors date parsing logic to improve code quality. The mobile menu provides full navigation access on mobile devices with keyboard support and dark mode compatibility. ### Key Changes - Mobile hamburger menu implementation across all 5 pages with slide-in navigation panel - Refactored date parsing into a centralized `parseDatePtr()` helper function with comprehensive unit tests - Updated README with mobile and calendar screenshots ### Reviewed Changes Copilot reviewed 8 out of 11 changed files in this pull request and generated 3 comments. <details> <summary>Show a summary per file</summary> | File | Description | | ---- | ----------- | | templates/subscriptions.html | Added mobile menu overlay, hamburger button, and JavaScript handlers | | templates/settings.html | Added mobile menu overlay, hamburger button, and JavaScript handlers | | templates/dashboard.html | Added mobile menu overlay, hamburger button, and JavaScript handlers | | templates/calendar.html | Added mobile menu overlay, hamburger button, and JavaScript handlers | | templates/analytics.html | Added mobile menu overlay, hamburger button, and JavaScript handlers | | internal/handlers/subscription.go | Added `parseDatePtr()` helper function to centralize date parsing logic | | internal/handlers/subscription_test.go | Added comprehensive unit tests for `parseDatePtr()` function | | README.md | Added mobile and calendar screenshots, updated feature descriptions | | mobile-screenshot.png | New mobile view screenshot | | calendar-screenshot.png | New calendar view screenshot | </details> --- 💡 <a href="/bscott/subtrackr/new/main/.github/instructions?filename=*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

templates/subscriptions.html

The mobile menu doesn't restore body scroll when navigating to a different page via the menu links. The onclick="closeMobileMenu()" calls will execute, but since the page is navigating away, the body scroll restoration (document.body.style.overflow = '') may not complete. This could leave the body scroll locked if the user navigates and then uses the browser back button. Consider adding scroll restoration on page load or using CSS classes instead of inline styles.

The mobile menu doesn't restore body scroll when navigating to a different page via the menu links. The `onclick="closeMobileMenu()"` calls will execute, but since the page is navigating away, the body scroll restoration (`document.body.style.overflow = ''`) may not complete. This could leave the body scroll locked if the user navigates and then uses the browser back button. Consider adding scroll restoration on page load or using CSS classes instead of inline styles.

templates/subscriptions.html

The mobile menu HTML structure is duplicated across all 5 template files. This 59-line block (lines 101-159) is identical in subscriptions.html, settings.html, dashboard.html, calendar.html, and analytics.html except for the active page indicator. Consider extracting this into a reusable template partial/component to reduce duplication and improve maintainability.

templates/subscriptions.html

The mobile menu JavaScript code is duplicated across all 5 template files (subscriptions.html, settings.html, dashboard.html, calendar.html, analytics.html). Consider extracting this into a shared JavaScript file to follow DRY principles and make maintenance easier. The duplicated code includes:

openMobileMenu() function
closeMobileMenu() function
Event listener setup
Escape key handler

This would reduce code duplication and ensure consistent behavior across all pages.

The mobile menu JavaScript code is duplicated across all 5 template files (subscriptions.html, settings.html, dashboard.html, calendar.html, analytics.html). Consider extracting this into a shared JavaScript file to follow DRY principles and make maintenance easier. The duplicated code includes: - `openMobileMenu()` function - `closeMobileMenu()` function - Event listener setup - Escape key handler This would reduce code duplication and ensure consistent behavior across all pages.

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed

2025-11-14 14:44:24 -05:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull Request Overview

Copilot reviewed 8 out of 11 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

## Pull Request Overview Copilot reviewed 8 out of 11 changed files in this pull request and generated 3 comments. --- 💡 <a href="/bscott/subtrackr/new/main/.github/instructions?filename=*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

templates/subscriptions.html

Significant indentation inconsistency detected. Lines 316-318 and 343-365 have incorrect indentation that breaks from the established pattern. These closing tags should align with their opening tags at the proper indentation level. This makes the code harder to read and maintain.

                                {{end}}
                            </div>
                        </div>

Significant indentation inconsistency detected. Lines 316-318 and 343-365 have incorrect indentation that breaks from the established pattern. These closing tags should align with their opening tags at the proper indentation level. This makes the code harder to read and maintain. ```suggestion {{end}} </div> </div> ```

templates/subscriptions.html

Significant indentation inconsistency detected. Lines 343-365 have incorrect indentation for button elements. These buttons should be properly indented to align with their parent container structure for better code readability and maintainability.

templates/subscriptions.html

Indentation inconsistency in the "no subscriptions" empty state section. Lines 378-393 should maintain consistent indentation with the rest of the template structure for better readability.

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed

2025-11-14 14:53:37 -05:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull Request Overview

Copilot reviewed 8 out of 11 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

## Pull Request Overview Copilot reviewed 8 out of 11 changed files in this pull request and generated 2 comments. --- 💡 <a href="/bscott/subtrackr/new/main/.github/instructions?filename=*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

internal/handlers/subscription_test.go

					
				@ -0,0 +1,108 @@

				package handlers

The comment mentions 'ignore time component differences' but the test only compares Year, Month, and Day fields. This works correctly for the parseDatePtr function which uses UTC with zero time components. However, the comment could be clearer by stating 'Compare date components only (Year, Month, Day) as parseDatePtr returns UTC dates with zero time components'.

					// Compare date components only (Year, Month, Day) as parseDatePtr returns UTC dates with zero time components

The comment mentions 'ignore time component differences' but the test only compares Year, Month, and Day fields. This works correctly for the `parseDatePtr` function which uses UTC with zero time components. However, the comment could be clearer by stating 'Compare date components only (Year, Month, Day) as parseDatePtr returns UTC dates with zero time components'. ```suggestion // Compare date components only (Year, Month, Day) as parseDatePtr returns UTC dates with zero time components ```

templates/subscriptions.html

The mobile menu JavaScript code (openMobileMenu, closeMobileMenu, event handlers) is duplicated across all 5 template files. Consider extracting this into a shared JavaScript file (e.g., /static/js/mobile-menu.js) to reduce code duplication and improve maintainability. This would make future updates and bug fixes easier to manage.

The mobile menu JavaScript code (openMobileMenu, closeMobileMenu, event handlers) is duplicated across all 5 template files. Consider extracting this into a shared JavaScript file (e.g., `/static/js/mobile-menu.js`) to reduce code duplication and improve maintainability. This would make future updates and bug fixes easier to manage.

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed

2025-11-14 15:02:29 -05:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull Request Overview

Copilot reviewed 9 out of 12 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

templates/calendar.html:363

The Escape key handler is duplicated between mobile-menu.js and the inline script. In calendar.html, the inline script checks for mobile menu state before closing it (lines 354-357), which may cause issues since mobile-menu.js already handles this (lines 32-44). The inline handler should be removed to avoid duplicate event listeners and potential race conditions.

        // Close mobile menu on escape key
        document.addEventListener('keydown', function(e) {
            if (e.key === 'Escape') {
                const mobileMenu = document.getElementById('mobile-menu');
                if (!mobileMenu.classList.contains('hidden')) {
                    closeMobileMenu();
                }
                // Also close modal if open
                const modal = document.getElementById('modal');
                if (modal) {
                    modal.classList.add('hidden');
                }
            }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

## Pull Request Overview Copilot reviewed 9 out of 12 changed files in this pull request and generated 3 comments. <details> <summary>Comments suppressed due to low confidence (1)</summary> **templates/calendar.html:363** * The Escape key handler is duplicated between `mobile-menu.js` and the inline script. In `calendar.html`, the inline script checks for mobile menu state before closing it (lines 354-357), which may cause issues since `mobile-menu.js` already handles this (lines 32-44). The inline handler should be removed to avoid duplicate event listeners and potential race conditions. ``` // Close mobile menu on escape key document.addEventListener('keydown', function(e) { if (e.key === 'Escape') { const mobileMenu = document.getElementById('mobile-menu'); if (!mobileMenu.classList.contains('hidden')) { closeMobileMenu(); } // Also close modal if open const modal = document.getElementById('modal'); if (modal) { modal.classList.add('hidden'); } } ``` </details> --- 💡 <a href="/bscott/subtrackr/new/main/.github/instructions?filename=*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

templates/settings.html

					
				@ -669,6 +748,8 @@

				    </div>

The Escape key handler is duplicated between mobile-menu.js and the inline script. In settings.html, the inline script checks for mobile menu state before closing it (lines 763-766), which may cause issues since mobile-menu.js already handles this (lines 32-44). The inline handler should be removed to avoid duplicate event listeners and potential race conditions.

        // Close modal on escape key
        document.addEventListener('keydown', function(e) {
            if (e.key === 'Escape') {
                // Close modal if open

The Escape key handler is duplicated between `mobile-menu.js` and the inline script. In `settings.html`, the inline script checks for mobile menu state before closing it (lines 763-766), which may cause issues since `mobile-menu.js` already handles this (lines 32-44). The inline handler should be removed to avoid duplicate event listeners and potential race conditions. ```suggestion // Close modal on escape key document.addEventListener('keydown', function(e) { if (e.key === 'Escape') { // Close modal if open ```

templates/subscriptions.html

					
				@ -80,6 +98,67 @@

				            </div>

The inline onclick handler closeMobileMenu(); document.getElementById('modal').classList.remove('hidden'); creates a race condition. The menu closing animation may interfere with the modal opening. Consider using a setTimeout or waiting for the menu close transition to complete before opening the modal.

The inline onclick handler `closeMobileMenu(); document.getElementById('modal').classList.remove('hidden');` creates a race condition. The menu closing animation may interfere with the modal opening. Consider using a setTimeout or waiting for the menu close transition to complete before opening the modal.

web/static/js/mobile-menu.js

					
				@ -0,0 +15,4 @@

				        mobileMenu.classList.add('hidden');

				        document.body.style.overflow = ''; // Restore body scroll

				    }

				}

[nitpick] The openMobileMenu() and closeMobileMenu() functions are declared globally but are only called inline in HTML templates. This creates tight coupling between the JS module and HTML. Consider using data attributes and event delegation instead, or explicitly exporting these functions if they need to be globally accessible.

[nitpick] The `openMobileMenu()` and `closeMobileMenu()` functions are declared globally but are only called inline in HTML templates. This creates tight coupling between the JS module and HTML. Consider using data attributes and event delegation instead, or explicitly exporting these functions if they need to be globally accessible.

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed

2025-11-14 15:27:55 -05:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull Request Overview

Copilot reviewed 9 out of 12 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

## Pull Request Overview Copilot reviewed 9 out of 12 changed files in this pull request and generated 3 comments. --- 💡 <a href="/bscott/subtrackr/new/main/.github/instructions?filename=*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

templates/subscriptions.html

					
				@ -80,6 +98,67 @@

				            </div>

The setTimeout with a 100ms delay (line 146) is a code smell that suggests a race condition or timing issue. This approach is fragile and could fail on slower devices or different browsers.

Consider using a callback-based approach or ensuring the menu close completes before opening the modal, rather than relying on an arbitrary timeout.

The `setTimeout` with a 100ms delay (line 146) is a code smell that suggests a race condition or timing issue. This approach is fragile and could fail on slower devices or different browsers. Consider using a callback-based approach or ensuring the menu close completes before opening the modal, rather than relying on an arbitrary timeout.

web/static/js/mobile-menu.js

					
				@ -0,0 +1,62 @@

				// Mobile menu functions for responsive navigation

The Escape key handler in the mobile menu JavaScript also handles closing modals. However, this creates a dual responsibility where pressing Escape will close both the mobile menu AND any open modal simultaneously (lines 32-43). This could lead to unexpected behavior if both are open.

Consider separating the concerns or adding logic to close only the topmost element (either the mobile menu OR the modal, not both).

            const modal = document.getElementById('modal');
            // If modal is open, close it; else if mobile menu is open, close it
            if (modal && !modal.classList.contains('hidden')) {
                modal.classList.add('hidden');
            } else {
                const mobileMenu = document.getElementById('mobile-menu');
                if (mobileMenu && !mobileMenu.classList.contains('hidden')) {
                    closeMobileMenu();
                }

The Escape key handler in the mobile menu JavaScript also handles closing modals. However, this creates a dual responsibility where pressing Escape will close both the mobile menu AND any open modal simultaneously (lines 32-43). This could lead to unexpected behavior if both are open. Consider separating the concerns or adding logic to close only the topmost element (either the mobile menu OR the modal, not both). ```suggestion const modal = document.getElementById('modal'); // If modal is open, close it; else if mobile menu is open, close it if (modal && !modal.classList.contains('hidden')) { modal.classList.add('hidden'); } else { const mobileMenu = document.getElementById('mobile-menu'); if (mobileMenu && !mobileMenu.classList.contains('hidden')) { closeMobileMenu(); } ```

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed

2025-11-14 17:22:58 -05:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull Request Overview

Copilot reviewed 9 out of 12 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

## Pull Request Overview Copilot reviewed 9 out of 12 changed files in this pull request and generated no new comments. --- 💡 <a href="/bscott/subtrackr/new/main/.github/instructions?filename=*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

bscott/subtrackr!56

				`@ -0,0 +1,62 @@`
				`// Mobile menu functions for responsive navigation`

Rows
Columns