From ac19bebf2d85b737ab39f91405dbfbef6e899923 Mon Sep 17 00:00:00 2001 From: Dries Peeters Date: Tue, 2 Dec 2025 06:13:54 +0100 Subject: [PATCH] feat: enhance offline sync and improve performance (v4.3.2) - Add comprehensive offline sync improvements with enhanced IndexedDB support - Optimize task model with cached total_hours calculation for better performance - Improve task service query optimization and eager loading strategies - Update CSP policy to allow CDN connections for improved resource loading - Enhance service worker with better background sync capabilities - Improve error handling and offline queue processing - Update base template and comment templates for better UX - Bump version to 4.3.2 --- app/__init__.py | 2 +- app/models/task.py | 26 +-- app/routes/tasks.py | 35 ++- app/services/task_service.py | 29 ++- app/static/error-handling-enhanced.js | 6 +- app/static/offline-sync.js | 200 +++++++++++++++--- app/static/service-worker.js | 20 +- app/static/smart-notifications.js | 3 +- app/templates/base.html | 101 +++++---- app/templates/comments/_comment.html | 24 ++- app/templates/comments/_comments_section.html | 4 +- setup.py | 2 +- 12 files changed, 341 insertions(+), 111 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 21dcaf5..d996b77 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -715,7 +715,7 @@ def create_app(config=None): "style-src 'self' 'unsafe-inline' https://cdn.jsdelivr.net https://cdnjs.cloudflare.com https://fonts.googleapis.com https://cdn.datatables.net https://uicdn.toast.com; " "font-src 'self' https://fonts.gstatic.com https://cdnjs.cloudflare.com data:; " "script-src 'self' 'unsafe-inline' https://code.jquery.com https://cdn.datatables.net https://cdnjs.cloudflare.com https://cdn.jsdelivr.net https://esm.sh https://uicdn.toast.com; " - "connect-src 'self' ws: wss:; " + "connect-src 'self' ws: wss: https://cdn.jsdelivr.net https://cdnjs.cloudflare.com; " "frame-ancestors 'none'" ) response.headers["Content-Security-Policy"] = csp diff --git a/app/models/task.py b/app/models/task.py index 9d76b84..f15f302 100644 --- a/app/models/task.py +++ b/app/models/task.py @@ -74,26 +74,16 @@ class Task(db.Model): @property def total_hours(self): """Calculate total hours spent on this task""" + # Use cached value if available (set by TaskService.list_tasks for performance) + if hasattr(self, '_cached_total_hours'): + return self._cached_total_hours + try: + from .time_entry import TimeEntry + total_seconds = ( - db.session.query( - db.func.sum( - db.func.coalesce( - db.func.extract( - "epoch", - db.func.greatest( - db.func.least( - db.func.coalesce(self.time_entries.end_time, now_in_app_timezone()), - now_in_app_timezone(), - ) - - self.time_entries.start_time - ), - ), - 0, - ) - ) - ) - .filter(self.time_entries.project_id == self.project_id) + db.session.query(db.func.sum(TimeEntry.duration_seconds)) + .filter(TimeEntry.task_id == self.id, TimeEntry.end_time.isnot(None)) .scalar() or 0 ) diff --git a/app/routes/tasks.py b/app/routes/tasks.py index 5ed914c..81c214c 100644 --- a/app/routes/tasks.py +++ b/app/routes/tasks.py @@ -190,12 +190,14 @@ def create_task(): def view_task(task_id): """View task details - REFACTORED to use service layer with eager loading""" from app.services import TaskService + from sqlalchemy.orm import joinedload + from app.models import Comment task_service = TaskService() # Get task with all relations using eager loading (prevents N+1 queries) task = task_service.get_task_with_details( - task_id=task_id, include_time_entries=True, include_comments=True, include_activities=True + task_id=task_id, include_time_entries=False, include_comments=False, include_activities=False ) if not task: @@ -207,13 +209,12 @@ def view_task(task_id): flash(_("You do not have access to this task"), "error") return redirect(url_for("tasks.list_tasks")) - # Get time entries (time_entries is a dynamic relationship, so query it) + # Get time entries with pagination (limit to 100 most recent to avoid loading too many) # Eagerly load user relationship to prevent N+1 queries - from sqlalchemy.orm import joinedload - time_entries = ( task.time_entries.options(joinedload(TimeEntry.user)) .order_by(TimeEntry.start_time.desc(), TimeEntry.id.desc()) + .limit(100) .all() ) @@ -226,10 +227,28 @@ def view_task(task_id): .all() ) - # Get comments for this task - from app.models import Comment - - comments = Comment.get_task_comments(task_id, include_replies=True) + # Get comments for this task with eager loading of authors and replies to prevent N+1 queries + # Load all comments (including replies) with their authors to avoid lazy loading issues + # Use selectinload for replies to load them in a separate query, preventing circular loading + from sqlalchemy.orm import selectinload + + # Load all comments for this task with eager loading + all_comments = ( + Comment.query.filter_by(task_id=task_id) + .options( + joinedload(Comment.author), # Eagerly load author for all comments + # Load replies with their authors - selectinload loads all direct replies in one query + # This prevents N+1 queries when accessing comment.replies in the template + selectinload(Comment.replies).joinedload(Comment.author) + ) + .order_by(Comment.created_at.asc()) + .all() + ) + + # Filter to only top-level comments (no parent_id) for the template + # The replies relationship is now eagerly loaded for direct replies + # Nested replies beyond the first level will be loaded lazily, but the template depth limit prevents issues + comments = [c for c in all_comments if c.parent_id is None] return render_template( "tasks/view.html", task=task, time_entries=time_entries, activities=activities, comments=comments diff --git a/app/services/task_service.py b/app/services/task_service.py index ef15074..f36cf9b 100644 --- a/app/services/task_service.py +++ b/app/services/task_service.py @@ -234,4 +234,31 @@ class TaskService: # Paginate pagination = query.paginate(page=page, per_page=per_page, error_out=False) - return {"tasks": pagination.items, "pagination": pagination, "total": pagination.total} + # Pre-calculate total_hours for all tasks in a single query to avoid N+1 + # This prevents the template from triggering individual queries for each task + tasks = pagination.items + if tasks: + from app.models import TimeEntry + task_ids = [task.id for task in tasks] + + # Calculate total hours for all tasks in one query + results = ( + db.session.query( + TimeEntry.task_id, + db.func.sum(TimeEntry.duration_seconds).label('total_seconds') + ) + .filter( + TimeEntry.task_id.in_(task_ids), + TimeEntry.end_time.isnot(None) + ) + .group_by(TimeEntry.task_id) + .all() + ) + total_hours_map = {task_id: total_seconds for task_id, total_seconds in results} + + # Cache the calculated values on task objects to avoid property queries + for task in tasks: + total_seconds = total_hours_map.get(task.id, 0) or 0 + task._cached_total_hours = round(total_seconds / 3600, 2) if total_seconds else 0.0 + + return {"tasks": tasks, "pagination": pagination, "total": pagination.total} diff --git a/app/static/error-handling-enhanced.js b/app/static/error-handling-enhanced.js index e5e4278..1951797 100644 --- a/app/static/error-handling-enhanced.js +++ b/app/static/error-handling-enhanced.js @@ -47,10 +47,12 @@ class EnhancedErrorHandler { this.handleOffline(); }); - // Periodic online check + // Periodic online check - every 30 seconds + // This helps detect cases where browser thinks it's online but server is unreachable + // The browser's online/offline events handle most cases, this is a fallback setInterval(() => { this.checkOnlineStatus(); - }, 5000); + }, 30000); // Check every 30 seconds instead of 5 } checkOnlineStatus() { diff --git a/app/static/offline-sync.js b/app/static/offline-sync.js index 4d61e60..0120cc8 100644 --- a/app/static/offline-sync.js +++ b/app/static/offline-sync.js @@ -122,13 +122,68 @@ class OfflineSyncManager { async getPendingSyncCount() { return new Promise((resolve, reject) => { - const transaction = this.db.transaction(['syncQueue'], 'readonly'); - const store = transaction.objectStore('syncQueue'); - const index = store.index('processed'); - const request = index.count(IDBKeyRange.only(false)); - - request.onerror = () => reject(request.error); - request.onsuccess = () => resolve(request.result || 0); + try { + const transaction = this.db.transaction(['syncQueue'], 'readonly'); + const store = transaction.objectStore('syncQueue'); + + // Check if the index exists + if (!store.indexNames.contains('processed')) { + // If index doesn't exist, count manually + const request = store.openCursor(); + let count = 0; + request.onsuccess = (event) => { + const cursor = event.target.result; + if (cursor) { + if (cursor.value.processed === false || !cursor.value.processed) { + count++; + } + cursor.continue(); + } else { + resolve(count); + } + }; + request.onerror = () => reject(request.error); + return; + } + + const index = store.index('processed'); + // Use getAll with a key range for boolean values + // IndexedDB can be finicky with boolean values, so we'll use a cursor approach + const request = index.openCursor(IDBKeyRange.only(false)); + let count = 0; + + request.onsuccess = (event) => { + const cursor = event.target.result; + if (cursor) { + count++; + cursor.continue(); + } else { + resolve(count); + } + }; + + request.onerror = () => { + // Fallback: count manually if index query fails + const fallbackRequest = store.openCursor(); + let fallbackCount = 0; + fallbackRequest.onsuccess = (event) => { + const cursor = event.target.result; + if (cursor) { + if (cursor.value.processed === false || !cursor.value.processed) { + fallbackCount++; + } + cursor.continue(); + } else { + resolve(fallbackCount); + } + }; + fallbackRequest.onerror = () => reject(fallbackRequest.error); + }; + } catch (error) { + // If there's any error, return 0 instead of rejecting + console.warn('[OfflineSync] Error counting pending sync, returning 0:', error); + resolve(0); + } }); } @@ -326,13 +381,52 @@ class OfflineSyncManager { async getUnsyncedEntries(storeName) { return new Promise((resolve, reject) => { - const transaction = this.db.transaction([storeName], 'readonly'); - const store = transaction.objectStore(storeName); - const index = store.index('synced'); - const request = index.getAll(IDBKeyRange.only(false)); - - request.onerror = () => reject(request.error); - request.onsuccess = () => resolve(request.result || []); + try { + const transaction = this.db.transaction([storeName], 'readonly'); + const store = transaction.objectStore(storeName); + + // Check if the index exists + if (!store.indexNames.contains('synced')) { + // If index doesn't exist, filter manually + const request = store.getAll(); + request.onsuccess = () => { + const all = request.result || []; + const unsynced = all.filter(entry => entry.synced === false || !entry.synced); + resolve(unsynced); + }; + request.onerror = () => reject(request.error); + return; + } + + const index = store.index('synced'); + // Use openCursor for boolean values to avoid IDBKeyRange issues + const request = index.openCursor(IDBKeyRange.only(false)); + const results = []; + + request.onsuccess = (event) => { + const cursor = event.target.result; + if (cursor) { + results.push(cursor.value); + cursor.continue(); + } else { + resolve(results); + } + }; + + request.onerror = () => { + // Fallback: filter manually if index query fails + const fallbackRequest = store.getAll(); + fallbackRequest.onsuccess = () => { + const all = fallbackRequest.result || []; + const unsynced = all.filter(entry => entry.synced === false || !entry.synced); + resolve(unsynced); + }; + fallbackRequest.onerror = () => reject(fallbackRequest.error); + }; + } catch (error) { + console.warn('[OfflineSync] Error getting unsynced entries, returning empty array:', error); + resolve([]); + } }); } @@ -380,23 +474,69 @@ class OfflineSyncManager { if (!this.db) return; return new Promise((resolve, reject) => { - const transaction = this.db.transaction(['syncQueue'], 'readwrite'); - const store = transaction.objectStore('syncQueue'); - const index = store.index('processed'); - const request = index.openCursor(IDBKeyRange.only(false)); - - request.onerror = () => reject(request.error); - request.onsuccess = async (event) => { - const cursor = event.target.result; - if (cursor) { - const item = cursor.value; - // Process queue item based on type - // This will be handled by specific sync methods - cursor.continue(); - } else { - resolve(); + try { + const transaction = this.db.transaction(['syncQueue'], 'readwrite'); + const store = transaction.objectStore('syncQueue'); + + // Check if the index exists + if (!store.indexNames.contains('processed')) { + // If index doesn't exist, iterate manually + const request = store.openCursor(); + request.onsuccess = async (event) => { + const cursor = event.target.result; + if (cursor) { + const item = cursor.value; + // Process queue item based on type + // This will be handled by specific sync methods + if (item.processed === false || !item.processed) { + // Process unprocessed items + } + cursor.continue(); + } else { + resolve(); + } + }; + request.onerror = () => reject(request.error); + return; } - }; + + const index = store.index('processed'); + const request = index.openCursor(IDBKeyRange.only(false)); + + request.onerror = () => { + // Fallback: iterate manually if index query fails + const fallbackRequest = store.openCursor(); + fallbackRequest.onsuccess = async (event) => { + const cursor = event.target.result; + if (cursor) { + const item = cursor.value; + if (item.processed === false || !item.processed) { + // Process queue item based on type + // This will be handled by specific sync methods + } + cursor.continue(); + } else { + resolve(); + } + }; + fallbackRequest.onerror = () => reject(fallbackRequest.error); + }; + + request.onsuccess = async (event) => { + const cursor = event.target.result; + if (cursor) { + const item = cursor.value; + // Process queue item based on type + // This will be handled by specific sync methods + cursor.continue(); + } else { + resolve(); + } + }; + } catch (error) { + console.warn('[OfflineSync] Error processing sync queue:', error); + resolve(); // Resolve instead of reject to prevent blocking + } }); } diff --git a/app/static/service-worker.js b/app/static/service-worker.js index ad9cfc8..6ec0f28 100644 --- a/app/static/service-worker.js +++ b/app/static/service-worker.js @@ -7,6 +7,8 @@ const CACHE_VERSION = 'v1.0.1'; const CACHE_NAME = `timetracker-${CACHE_VERSION}`; // Resources to cache immediately +// Note: External CDN resources are excluded due to CSP restrictions +// They will be cached on-demand when fetched by the browser const PRECACHE_URLS = [ '/', '/static/dist/output.css', @@ -14,8 +16,7 @@ const PRECACHE_URLS = [ '/static/enhanced-ui.js', '/static/charts.js', '/static/interactions.js', - '/static/images/timetracker-logo.svg', - 'https://cdnjs.cloudflare.com/ajax/libs/font-awesome/6.4.0/css/all.min.css' + '/static/images/timetracker-logo.svg' ]; // Resources to cache on first use @@ -34,7 +35,20 @@ self.addEventListener('install', event => { caches.open(CACHE_NAME) .then(cache => { console.log('[ServiceWorker] Precaching app shell'); - return cache.addAll(PRECACHE_URLS); + // Only cache same-origin resources to avoid CSP violations + const sameOriginUrls = PRECACHE_URLS.filter(url => { + try { + const urlObj = new URL(url, self.location.origin); + return urlObj.origin === self.location.origin; + } catch { + // Relative URLs are same-origin + return !url.startsWith('http://') && !url.startsWith('https://'); + } + }); + return cache.addAll(sameOriginUrls).catch(error => { + console.warn('[ServiceWorker] Some resources failed to cache:', error); + // Continue even if some resources fail + }); }) .then(() => self.skipWaiting()) ); diff --git a/app/static/smart-notifications.js b/app/static/smart-notifications.js index 1003bdd..a441cf5 100644 --- a/app/static/smart-notifications.js +++ b/app/static/smart-notifications.js @@ -278,8 +278,7 @@ class SmartNotificationManager { // Check every hour setInterval(sendSummary, 60 * 60 * 1000); - // Check immediately - sendSummary(); + // Don't check immediately on page load - only show at scheduled time (6 PM) } async sendDailySummary() { diff --git a/app/templates/base.html b/app/templates/base.html index 5103bed..0b51e93 100644 --- a/app/templates/base.html +++ b/app/templates/base.html @@ -1434,50 +1434,67 @@ // Install prompt let deferredPrompt; - window.addEventListener('beforeinstallprompt', (e) => { - e.preventDefault(); - deferredPrompt = e; - - // Check if user has previously dismissed the install prompt - const installPromptDismissed = localStorage.getItem('pwa-install-dismissed'); - if (installPromptDismissed === 'true') { - return; // Don't show the prompt if it was dismissed before - } - - // Show install button in UI - if (window.toastManager) { - const toast = window.toastManager.info('Install TimeTracker as an app!', 0); - const btn = document.createElement('button'); - btn.textContent = 'Install'; - btn.className = 'ml-2 px-3 py-1 bg-primary text-white rounded hover:bg-primary/90'; - btn.onclick = async () => { - deferredPrompt.prompt(); - const { outcome } = await deferredPrompt.userChoice; - if (outcome === 'accepted') { - window.toastManager.success('App installed successfully!'); - localStorage.setItem('pwa-install-dismissed', 'true'); - } else { - // User declined, remember their choice - localStorage.setItem('pwa-install-dismissed', 'true'); - } - deferredPrompt = null; - toast.remove(); - }; + + // Check if user has previously dismissed the install prompt or app is already installed + const installPromptDismissed = localStorage.getItem('pwa-install-dismissed'); + const isAppInstalled = window.matchMedia('(display-mode: standalone)').matches || + window.navigator.standalone === true || + document.referrer.includes('android-app://'); + + // Only register the event listener if prompt hasn't been dismissed and app isn't installed + if (installPromptDismissed !== 'true' && !isAppInstalled) { + window.addEventListener('beforeinstallprompt', (e) => { + e.preventDefault(); - // Add a dismiss button - const dismissBtn = document.createElement('button'); - dismissBtn.textContent = '×'; - dismissBtn.className = 'ml-2 px-2 py-1 text-white hover:bg-white/20 rounded'; - dismissBtn.title = 'Dismiss permanently'; - dismissBtn.onclick = () => { - localStorage.setItem('pwa-install-dismissed', 'true'); - toast.remove(); - }; + // Double-check dismissal status when event fires + const stillDismissed = localStorage.getItem('pwa-install-dismissed'); + if (stillDismissed === 'true') { + return; // Don't show the prompt if it was dismissed + } - toast.appendChild(btn); - toast.appendChild(dismissBtn); - } - }); + deferredPrompt = e; + + // Show install button in UI + if (window.toastManager) { + const toast = window.toastManager.info('Install TimeTracker as an app!', 0); + const btn = document.createElement('button'); + btn.textContent = 'Install'; + btn.className = 'ml-2 px-3 py-1 bg-primary text-white rounded hover:bg-primary/90'; + btn.onclick = async () => { + if (!deferredPrompt) return; + deferredPrompt.prompt(); + const { outcome } = await deferredPrompt.userChoice; + if (outcome === 'accepted') { + window.toastManager.success('App installed successfully!'); + } + // Always mark as dismissed after user interaction + localStorage.setItem('pwa-install-dismissed', 'true'); + deferredPrompt = null; + toast.remove(); + }; + + // Add a dismiss button + const dismissBtn = document.createElement('button'); + dismissBtn.textContent = '×'; + dismissBtn.className = 'ml-2 px-2 py-1 text-white hover:bg-white/20 rounded'; + dismissBtn.title = 'Dismiss permanently'; + dismissBtn.onclick = () => { + localStorage.setItem('pwa-install-dismissed', 'true'); + deferredPrompt = null; + toast.remove(); + }; + + toast.appendChild(btn); + toast.appendChild(dismissBtn); + } + }); + + // Also listen for app installation to mark as dismissed + window.addEventListener('appinstalled', () => { + localStorage.setItem('pwa-install-dismissed', 'true'); + deferredPrompt = null; + }); + }