-
Notifications
You must be signed in to change notification settings - Fork 42
Enhanced ExpenseFlow: Added navigation sections, improved typography,… #50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
… fixed layout, and GitHub Pages compatibility - Added complete Analytics, Goals, and Settings sections with full functionality - Enhanced typography with Poppins, Space Grotesk, and Inter fonts - Fixed overlapping layout issues and improved spacing - Added comprehensive Mermaid.js flowcharts in README - Enhanced feature request template with detailed sections - Fixed all file paths for GitHub Pages compatibility - Added .gitignore, .nojekyll, and DEPLOYMENT.md - Improved service worker and manifest.json paths - All navigation links now work perfectly - Better responsive design and mobile support
…racker UI and functionality
|
@Kadiwalhussain is attempting to deploy a commit to the Renu's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request enhances ExpenseFlow with comprehensive navigation sections (Analytics, Goals, Settings), improved typography using multiple Google Fonts families, layout fixes, and improved GitHub Pages compatibility through relative path configurations.
Changes:
- Added three new sections (Analytics, Goals, Settings) with full functionality including data visualization, goal tracking, and user preferences
- Enhanced typography with Poppins, Space Grotesk, and Inter font families
- Fixed GitHub Pages compatibility by updating all paths to relative notation in service worker, manifest, and JavaScript
- Added comprehensive documentation including deployment guide, enhanced README with Mermaid flowcharts, and detailed feature request template
- Improved responsive design and mobile support
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| trackerscript.js | Added section navigation system, analytics functions, goals management, settings functionality, and improved service worker registration |
| sw.js | Updated cache paths to use relative notation for GitHub Pages compatibility |
| manifest.json | Changed start_url and scope to relative paths for better portability |
| index.html | Added Analytics, Goals, and Settings sections with complete HTML structure and updated footer links |
| expensetracker.css | Enhanced typography with new font variables, added styles for new sections, improved responsive design |
| README.md | Added comprehensive Mermaid flowcharts, detailed usage guide, architecture diagrams, and expanded documentation |
| DEPLOYMENT.md | New deployment guide with GitHub Pages instructions and troubleshooting |
| .gitignore | New file with comprehensive ignore patterns |
| .nojekyll | New file to disable Jekyll processing on GitHub Pages |
| .github/ISSUE_TEMPLATE/feature_request.md | Enhanced template with detailed sections and structured format |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const transactionType = type.value || 'transaction'; | ||
| showNotification(`${transactionType.charAt(0).toUpperCase() + transactionType.slice(1)} added successfully!`, 'success'); |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The success notification is attempting to use type.value after the form fields have been cleared on line 318. This will always result in an empty string being passed to the notification function, causing the message to be "Transaction added successfully!" instead of the intended "Income added successfully!" or "Expense added successfully!". Store the type.value in a variable before clearing the form fields.
| <button class="goal-delete" onclick="deleteGoal(${index})"> | ||
| <i class="fas fa-trash"></i> | ||
| </button> |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inline onclick handler uses deleteGoal with an index parameter. If the goals array is modified (e.g., a goal is deleted) while the user is viewing the goals, the indices will shift but the rendered HTML won't update immediately. This can cause the wrong goal to be deleted. Use event delegation or store the goal ID instead of the index for safer deletion.
| <h3>${goal.title}</h3> | ||
| <button class="goal-delete" onclick="deleteGoal(${index})"> | ||
| <i class="fas fa-trash"></i> | ||
| </button> | ||
| </div> | ||
| <div class="goal-progress"> | ||
| <div class="progress-bar"> | ||
| <div class="progress-fill" style="width: ${Math.min(progress, 100)}%"></div> | ||
| </div> | ||
| <div class="progress-text"> | ||
| <span>₹${goal.current.toFixed(2)} / ₹${goal.amount.toFixed(2)}</span> | ||
| <span>${progress.toFixed(1)}%</span> | ||
| </div> | ||
| </div> | ||
| <div class="goal-details"> | ||
| <div class="goal-detail-item"> | ||
| <i class="fas fa-calendar"></i> | ||
| <span>${daysRemaining > 0 ? `${daysRemaining} days left` : 'Deadline passed'}</span> | ||
| </div> | ||
| <div class="goal-detail-item"> | ||
| <i class="fas fa-rupee-sign"></i> | ||
| <span>₹${remaining.toFixed(2)} remaining</span> | ||
| </div> | ||
| </div> | ||
| ${goal.description ? `<p class="goal-description">${goal.description}</p>` : ''} |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal.title and goal.description are directly interpolated into HTML without sanitization. This creates a Cross-Site Scripting (XSS) vulnerability. If a user enters HTML or JavaScript code in the title or description, it will be executed when the goals are displayed. Use textContent or sanitize the input before rendering.
|
|
||
| const title = document.getElementById('goal-title').value; | ||
| const amount = parseFloat(document.getElementById('goal-amount').value); | ||
| const current = parseFloat(document.getElementById('goal-current').value) || 0; | ||
| const deadline = document.getElementById('goal-deadline').value; | ||
| const description = document.getElementById('goal-description').value; | ||
|
|
||
| if (!title || !amount || !deadline) { | ||
| showNotification('Please fill in all required fields', 'error'); | ||
| return; | ||
| } |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal validation only checks for presence of required fields but doesn't validate the values. The amount and current values should be checked to ensure they're positive numbers, and the deadline should be validated to ensure it's a valid date (and ideally in the future). Additionally, there's no maximum length validation for title and description which could lead to display issues.
| const title = document.getElementById('goal-title').value; | |
| const amount = parseFloat(document.getElementById('goal-amount').value); | |
| const current = parseFloat(document.getElementById('goal-current').value) || 0; | |
| const deadline = document.getElementById('goal-deadline').value; | |
| const description = document.getElementById('goal-description').value; | |
| if (!title || !amount || !deadline) { | |
| showNotification('Please fill in all required fields', 'error'); | |
| return; | |
| } | |
| const MAX_TITLE_LENGTH = 100; | |
| const MAX_DESCRIPTION_LENGTH = 1000; | |
| const rawTitle = document.getElementById('goal-title').value; | |
| const rawAmount = document.getElementById('goal-amount').value; | |
| const rawCurrent = document.getElementById('goal-current').value; | |
| const rawDeadline = document.getElementById('goal-deadline').value; | |
| const rawDescription = document.getElementById('goal-description').value; | |
| const title = rawTitle.trim(); | |
| const amount = parseFloat(rawAmount); | |
| const current = rawCurrent === '' ? 0 : parseFloat(rawCurrent); | |
| const deadline = rawDeadline.trim(); | |
| const description = rawDescription.trim(); | |
| // Basic required field validation | |
| if (!title || !deadline) { | |
| showNotification('Please fill in all required fields', 'error'); | |
| return; | |
| } | |
| // Numeric validation for amount and current | |
| if (Number.isNaN(amount) || amount <= 0) { | |
| showNotification('Goal amount must be a positive number', 'error'); | |
| return; | |
| } | |
| if (Number.isNaN(current) || current < 0) { | |
| showNotification('Current savings must be a number greater than or equal to 0', 'error'); | |
| return; | |
| } | |
| if (current > amount) { | |
| showNotification('Current savings cannot exceed the goal amount', 'error'); | |
| return; | |
| } | |
| // Deadline validation: valid date and in the future | |
| const deadlineDate = new Date(deadline); | |
| if (Number.isNaN(deadlineDate.getTime())) { | |
| showNotification('Please enter a valid deadline date', 'error'); | |
| return; | |
| } | |
| // Consider the entire deadline day as valid | |
| const endOfDeadlineDay = new Date(deadlineDate.getTime()); | |
| endOfDeadlineDay.setHours(23, 59, 59, 999); | |
| const now = new Date(); | |
| if (endOfDeadlineDay <= now) { | |
| showNotification('Deadline must be a future date', 'error'); | |
| return; | |
| } | |
| // Length validation for title and description | |
| if (title.length > MAX_TITLE_LENGTH) { | |
| showNotification(`Title must be at most ${MAX_TITLE_LENGTH} characters`, 'error'); | |
| return; | |
| } | |
| if (description.length > MAX_DESCRIPTION_LENGTH) { | |
| showNotification(`Description must be at most ${MAX_DESCRIPTION_LENGTH} characters`, 'error'); | |
| return; | |
| } |
| link.href = URL.createObjectURL(blob); | ||
| link.download = `expenseflow-backup-${new Date().toISOString().split('T')[0]}.json`; | ||
| link.click(); |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The export data functionality doesn't clean up the created object URL after the download is triggered. This can lead to memory leaks, especially if users export data multiple times in a session. Add URL.revokeObjectURL(link.href) after triggering the click, or use a timeout to clean it up.
| link.href = URL.createObjectURL(blob); | |
| link.download = `expenseflow-backup-${new Date().toISOString().split('T')[0]}.json`; | |
| link.click(); | |
| const objectUrl = URL.createObjectURL(blob); | |
| link.href = objectUrl; | |
| link.download = `expenseflow-backup-${new Date().toISOString().split('T')[0]}.json`; | |
| link.click(); | |
| URL.revokeObjectURL(objectUrl); |
| font-size: 0.95rem; | ||
| letter-spacing: 0.3px; | ||
| padding: 0.5rem 1rem; | ||
| border-radius: 0.5rem; | ||
| transition: all 0.3s ease; | ||
| position: relative; | ||
| text-transform: uppercase; | ||
| font-size: 0.85rem; |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The font-size property is defined twice for .nav-link (lines 124 and 131), with different values (0.95rem and 0.85rem). The second declaration will override the first, making line 124 redundant. Remove the duplicate font-size declaration on line 124.
| setTimeout(() => updateAnalytics(), 100); | ||
| } | ||
|
|
||
| // Update goals if navigating to goals section | ||
| if (sectionId === 'goals') { | ||
| setTimeout(() => displayGoals(), 100); |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The showSection function uses setTimeout with a hardcoded 100ms delay to update analytics and goals. This arbitrary delay can lead to race conditions or unnecessary waiting. Consider using a callback or Promise-based approach, or call these functions directly since they should be safe to execute immediately.
| setTimeout(() => updateAnalytics(), 100); | |
| } | |
| // Update goals if navigating to goals section | |
| if (sectionId === 'goals') { | |
| setTimeout(() => displayGoals(), 100); | |
| updateAnalytics(); | |
| } | |
| // Update goals if navigating to goals section | |
| if (sectionId === 'goals') { | |
| displayGoals(); |
| // Get the base path for service worker registration | ||
| const basePath = window.location.pathname.split('/').slice(0, -1).join('/') || '.'; | ||
| const swPath = basePath === '.' ? './sw.js' : `${basePath}/sw.js`; | ||
|
|
||
| navigator.serviceWorker.register(swPath, { scope: basePath === '.' ? './' : basePath }) |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The service worker registration scope configuration may cause issues. When basePath is not '.', you're setting the scope to the base path directory but registering at ${basePath}/sw.js. This is unnecessarily complex and could fail if the page is accessed at different URL depths. Consider using a simpler approach with relative paths only (e.g., always use './sw.js' with scope './') since the manifest.json already uses relative paths.
| // Hook into transaction updates to refresh analytics | ||
| const originalUpdateLocalStorage = updateLocalStorage; | ||
| updateLocalStorage = function() { | ||
| originalUpdateLocalStorage(); | ||
| if (document.getElementById('analytics') && document.getElementById('analytics').classList.contains('active')) { | ||
| updateAnalytics(); | ||
| } | ||
| }; |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function is reassigning the global updateLocalStorage function by wrapping the original. This is a fragile pattern that could cause issues if updateLocalStorage is called before DOMContentLoaded fires, or if other code also tries to wrap this function. Consider using event dispatching or a callback system instead of function wrapping.
| // Hook into transaction updates to refresh analytics | |
| const originalUpdateLocalStorage = updateLocalStorage; | |
| updateLocalStorage = function() { | |
| originalUpdateLocalStorage(); | |
| if (document.getElementById('analytics') && document.getElementById('analytics').classList.contains('active')) { | |
| updateAnalytics(); | |
| } | |
| }; | |
| // Observe transaction list changes to refresh analytics | |
| if (typeof MutationObserver !== 'undefined' && list) { | |
| const analyticsObserver = new MutationObserver(() => { | |
| const analyticsSection = document.getElementById('analytics'); | |
| if (analyticsSection && analyticsSection.classList.contains('active')) { | |
| updateAnalytics(); | |
| } | |
| }); | |
| analyticsObserver.observe(list, { childList: true, subtree: false }); | |
| } |
| './expensetracker.css', | ||
| './trackerscript.js', | ||
| './manifest.json', | ||
| 'https://fonts.googleapis.com/css2?family=Inter:wght@300;400;500;600;700&display=swap', |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The urlsToCache array includes a Google Fonts URL but the HTML file now loads three different font families (Poppins, Space Grotesk, and Inter). The cached fonts URL only includes Inter, which means the other fonts won't be cached and will fail to load when offline. Update the cached URL to match the one in index.html.
| 'https://fonts.googleapis.com/css2?family=Inter:wght@300;400;500;600;700&display=swap', | |
| 'https://fonts.googleapis.com/css2?family=Inter:wght@300;400;500;600;700&family=Poppins:wght@300;400;500;600;700&family=Space+Grotesk:wght@300;400;500;600;700&display=swap', |
|
resolve conflict @Kadiwalhussain |
|
@Kadiwalhussain resolve conflict |
|
@Renu-code123 Done please check. |
|
@Kadiwalhussain resolve conflict |
|
@Renu-code123 Hi, I’m currently busy participating in a few hackathons, which is why I’m not able to address this right now. I’ll resolve it as soon as possible. Thank you for your patience. |
… fixed layout, and GitHub Pages compatibility