From 9cb78c9c98777e40ac1bdc2b8279b717be38446b Mon Sep 17 00:00:00 2001 From: Samuel Enocsson Date: Mon, 25 May 2026 09:54:15 +0200 Subject: [PATCH] fix: address code-review findings from pass 1 + 2 (#8) - Fix saveCourseToDB returning 0 on conflict by falling back to SELECT - Fix inactive layouts showing 'Never played' when last_played exists - Add .icon-btn.spinning to courses.css for refresh button feedback - Remove duplicate .btn-primary from courses.css (use shared.css version) - Tokenize rating tier colors into --rating-tier-{high,mid,low} CSS vars - Convert var to const/let throughout courses.js - Fix logger.error calls to use {err} object form (pino convention) - Extract RATING_TIER_HIGH/MID constants in course-layouts.ejs scriptlet - Remove dead href='#' View all link from courses.ejs (deferred) - Pass total prop explicitly from course-table.ejs to course-cards.ejs - Remove dead #search-results-info selector from mobile.css - Remove redundant .replace(/"/g, '"') from data attributes in course-table.ejs --- public/css/courses.css | 39 ++++------- public/css/mobile.css | 3 - public/css/shared.css | 5 ++ public/js/courses.js | 108 +++++++++++++++--------------- src/models/course.js | 10 ++- src/routes/courses.js | 10 +-- views/pages/courses.ejs | 2 +- views/partials/course-layouts.ejs | 12 +++- views/partials/course-table.ejs | 4 +- 9 files changed, 97 insertions(+), 96 deletions(-) diff --git a/public/css/courses.css b/public/css/courses.css index 63c4dc2..0cc0843 100644 --- a/public/css/courses.css +++ b/public/css/courses.css @@ -78,26 +78,7 @@ /* ── Buttons ──────────────────────────────────────── */ -.btn-primary { - background: var(--accent); - color: #fff; - border: 0; - height: 40px; - padding: 0 16px; - border-radius: var(--radius-sm); - font: 600 14px/1 var(--font-sans); - cursor: pointer; - white-space: nowrap; -} - -.btn-primary:hover { - filter: brightness(1.05); -} - -.btn-primary:disabled { - opacity: .6; - cursor: not-allowed; -} +/* .btn-primary is defined in shared.css — no override needed here */ .btn-pill { padding: 6px 12px; @@ -332,18 +313,18 @@ } .chip-rating--green { - color: oklch(0.55 0.15 150); - background: color-mix(in oklch, oklch(0.55 0.15 150) 10%, transparent); + color: var(--rating-tier-high); + background: color-mix(in oklch, var(--rating-tier-high) 10%, transparent); } .chip-rating--amber { - color: oklch(0.55 0.12 100); - background: color-mix(in oklch, oklch(0.55 0.12 100) 10%, transparent); + color: var(--rating-tier-mid); + background: color-mix(in oklch, var(--rating-tier-mid) 10%, transparent); } .chip-rating--orange { - color: oklch(0.55 0.10 50); - background: color-mix(in oklch, oklch(0.55 0.10 50) 10%, transparent); + color: var(--rating-tier-low); + background: color-mix(in oklch, var(--rating-tier-low) 10%, transparent); } /* ── Inactive layouts collapsible ────────────────── */ @@ -386,6 +367,12 @@ display: none; } +/* ── Icon button spin state (keyframes defined in shared.css) ─── */ + +.icon-btn.spinning { + animation: spin 0.8s linear infinite; +} + /* ── Tjing results ───────────────────────────────── */ #tjing-results { diff --git a/public/css/mobile.css b/public/css/mobile.css index f210161..ccc5c87 100644 --- a/public/css/mobile.css +++ b/public/css/mobile.css @@ -64,9 +64,6 @@ display: none; } - /* Hide search result info text on courses (mobile has section-head) */ - #search-results-info { display: none; } - /* ── Container ──────────────────────────────────── */ .container { diff --git a/public/css/shared.css b/public/css/shared.css index 7e43319..e411128 100644 --- a/public/css/shared.css +++ b/public/css/shared.css @@ -34,6 +34,11 @@ --font-sans: 'Plus Jakarta Sans', system-ui, sans-serif; --font-mono: 'JetBrains Mono', ui-monospace, monospace; + /* ── Rating tier tokens ───────────────────────── */ + --rating-tier-high: oklch(0.55 0.15 150); + --rating-tier-mid: oklch(0.55 0.12 100); + --rating-tier-low: oklch(0.55 0.10 50); + /* legacy token aliases — remove as components migrate */ --surface-0: var(--bg); --surface-1: var(--paper); diff --git a/public/js/courses.js b/public/js/courses.js index 00a21e3..1d4e09c 100644 --- a/public/js/courses.js +++ b/public/js/courses.js @@ -1,6 +1,6 @@ // ── Tab switching ────────────────────────────────── function initCourseTabs() { - var tabs = document.querySelectorAll('.action-tab'); + const tabs = document.querySelectorAll('.action-tab'); tabs.forEach(function(tab) { tab.addEventListener('click', function() { tabs.forEach(function(t) { @@ -15,8 +15,8 @@ function initCourseTabs() { pane.classList.remove('is-active'); }); - var targetId = 'tab-pane-' + tab.dataset.tab; - var pane = document.getElementById(targetId); + const targetId = 'tab-pane-' + tab.dataset.tab; + const pane = document.getElementById(targetId); if (pane) { pane.hidden = false; pane.classList.add('is-active'); @@ -27,23 +27,23 @@ function initCourseTabs() { // ── Live filter ──────────────────────────────────── function initCourseLiveFilter() { - var input = document.getElementById('course-filter-input'); + const input = document.getElementById('course-filter-input'); if (!input) return; input.addEventListener('input', function() { - var q = input.value.toLowerCase().trim(); - var rows = document.querySelectorAll('.course-row'); - var visible = 0; + const q = input.value.toLowerCase().trim(); + const rows = document.querySelectorAll('.course-row'); + let visible = 0; rows.forEach(function(row) { - var name = row.dataset.courseName || ''; - var city = row.dataset.courseCity || ''; - var match = !q || name.includes(q) || city.includes(q); + const name = row.dataset.courseName || ''; + const city = row.dataset.courseCity || ''; + const match = !q || name.includes(q) || city.includes(q); row.hidden = !match; // Keep the expanded content sibling in sync - var next = row.nextElementSibling; + const next = row.nextElementSibling; if (next && next.classList.contains('expanded-content')) { next.hidden = !match; } @@ -51,32 +51,32 @@ function initCourseLiveFilter() { if (match) visible++; }); - var visibleEl = document.getElementById('visible-count'); + const visibleEl = document.getElementById('visible-count'); if (visibleEl) visibleEl.textContent = visible; }); } // ── Count display ────────────────────────────────── function initCourseCounts() { - var grid = document.querySelector('.course-grid'); - var total = grid ? parseInt(grid.dataset.totalCount || '0', 10) : 0; - var rows = document.querySelectorAll('.course-row'); - var visible = 0; + const grid = document.querySelector('.course-grid'); + const total = grid ? parseInt(grid.dataset.totalCount || '0', 10) : 0; + const rows = document.querySelectorAll('.course-row'); + let visible = 0; rows.forEach(function(r) { if (!r.hidden) visible++; }); - var totalEl = document.getElementById('total-count'); - var visibleEl = document.getElementById('visible-count'); + const totalEl = document.getElementById('total-count'); + const visibleEl = document.getElementById('visible-count'); if (totalEl) totalEl.textContent = total; if (visibleEl) visibleEl.textContent = visible || total; } // ── Course row expand/collapse ───────────────────── function toggleCourseLayouts(courseId) { - var row = document.querySelector('.course-row[data-course-id="' + courseId + '"]'); - var content = document.getElementById('course-layouts-' + courseId); + const row = document.querySelector('.course-row[data-course-id="' + courseId + '"]'); + const content = document.getElementById('course-layouts-' + courseId); if (!row || !content) return; - var isOpen = content.classList.contains('is-open'); + const isOpen = content.classList.contains('is-open'); if (isOpen) { content.classList.remove('is-open'); @@ -86,7 +86,7 @@ function toggleCourseLayouts(courseId) { row.classList.add('row-open'); // Lazy-load layouts on first expand - var cell = content.querySelector('.expanded-cell'); + const cell = content.querySelector('.expanded-cell'); if (cell && cell.dataset.loaded !== 'true') { cell.dataset.loaded = 'true'; htmx.ajax('GET', '/partials/course-layouts/' + courseId, { target: cell, swap: 'innerHTML' }); @@ -95,17 +95,17 @@ function toggleCourseLayouts(courseId) { } // ── Mobile course card toggle ────────────────────── -var openMobileCourseId = null; +let openMobileCourseId = null; function toggleMobileCourseLayouts(courseId) { - var card = document.getElementById('m-course-' + courseId); + const card = document.getElementById('m-course-' + courseId); if (!card) return; - var isOpen = card.classList.contains('is-open'); + const isOpen = card.classList.contains('is-open'); // Close previously open card if (openMobileCourseId !== null && openMobileCourseId !== courseId) { - var prevCard = document.getElementById('m-course-' + openMobileCourseId); + const prevCard = document.getElementById('m-course-' + openMobileCourseId); if (prevCard) { prevCard.classList.remove('is-open'); prevCard.setAttribute('aria-expanded', 'false'); @@ -125,7 +125,7 @@ function toggleMobileCourseLayouts(courseId) { openMobileCourseId = courseId; // Lazy-load layouts on first expand - var container = document.getElementById('m-layouts-container-' + courseId); + const container = document.getElementById('m-layouts-container-' + courseId); if (container && container.dataset.loaded !== 'true') { htmx.ajax('GET', '/partials/course-layouts/' + courseId, { target: '#m-layouts-container-' + courseId, swap: 'innerHTML' }); container.dataset.loaded = 'true'; @@ -134,10 +134,10 @@ function toggleMobileCourseLayouts(courseId) { // ── Inactive layouts toggle ──────────────────────── function toggleInactiveLayouts(btn) { - var body = btn.nextElementSibling; + const body = btn.nextElementSibling; if (!body) return; - var isOpen = btn.classList.contains('is-open'); + const isOpen = btn.classList.contains('is-open'); btn.classList.toggle('is-open', !isOpen); btn.setAttribute('aria-expanded', String(!isOpen)); body.hidden = isOpen; @@ -145,15 +145,15 @@ function toggleInactiveLayouts(btn) { // ── Scrape courses ───────────────────────────────── async function scrapeCourses() { - var btn = document.getElementById('scrape-courses-btn'); + const btn = document.getElementById('scrape-courses-btn'); if (btn) { btn.disabled = true; btn.textContent = 'Scraping...'; } try { - var response = await fetch('/api/scrape-courses', { method: 'POST' }); - var data = await response.json(); + const response = await fetch('/api/scrape-courses', { method: 'POST' }); + const data = await response.json(); if (data.success) { alert(data.message); @@ -177,16 +177,16 @@ async function scrapeLayouts(courseId, btn) { if (btn) btn.classList.add('spinning'); try { - var response = await fetch('/api/scrape-layouts/' + courseId, { method: 'POST' }); - var data = await response.json(); + const response = await fetch('/api/scrape-layouts/' + courseId, { method: 'POST' }); + const data = await response.json(); if (response.status === 409) { alert(data.message || 'Scrape already in progress for this course. Please wait.'); } else if (data.success) { // Reload expanded layout content if currently open - var content = document.getElementById('course-layouts-' + courseId); + const content = document.getElementById('course-layouts-' + courseId); if (content && content.classList.contains('is-open')) { - var cell = content.querySelector('.expanded-cell'); + const cell = content.querySelector('.expanded-cell'); if (cell) { cell.dataset.loaded = 'true'; htmx.ajax('GET', '/partials/course-layouts/' + courseId, { target: cell, swap: 'innerHTML' }); @@ -206,12 +206,12 @@ async function scrapeLayouts(courseId, btn) { // ── Tjing search ─────────────────────────────────── async function searchTjing() { - var input = document.getElementById('tjing-search-input'); - var btn = document.getElementById('tjing-search-btn'); - var container = document.getElementById('tjing-results'); + const input = document.getElementById('tjing-search-input'); + const btn = document.getElementById('tjing-search-btn'); + const container = document.getElementById('tjing-results'); if (!input || !container) return; - var q = input.value.trim(); + const q = input.value.trim(); if (!q) return; btn.disabled = true; @@ -222,12 +222,12 @@ async function searchTjing() { } try { - var response = await fetch('/api/tjing/search?q=' + encodeURIComponent(q)); - var data; + const response = await fetch('/api/tjing/search?q=' + encodeURIComponent(q)); + let data; try { data = await response.json(); } catch (e) { - var errP = document.createElement('p'); + const errP = document.createElement('p'); errP.className = 'tjing-error'; errP.textContent = 'Invalid response from server.'; container.appendChild(errP); @@ -235,16 +235,16 @@ async function searchTjing() { } if (!response.ok || data.error) { - var errP2 = document.createElement('p'); + const errP2 = document.createElement('p'); errP2.className = 'tjing-error'; errP2.textContent = 'Error: ' + (data.error || 'Search failed'); container.appendChild(errP2); return; } - var results = data.results || []; + const results = data.results || []; if (results.length === 0) { - var noResults = document.createElement('p'); + const noResults = document.createElement('p'); noResults.className = 'tjing-error'; noResults.textContent = 'No courses found on Tjing.'; container.appendChild(noResults); @@ -252,24 +252,24 @@ async function searchTjing() { } results.forEach(function(course) { - var item = document.createElement('div'); + const item = document.createElement('div'); item.className = 'tjing-result'; - var info = document.createElement('div'); + const info = document.createElement('div'); info.className = 'tjing-result-info'; - var nameSpan = document.createElement('span'); + const nameSpan = document.createElement('span'); nameSpan.className = 'tjing-result-name'; nameSpan.textContent = course.name || ''; - var addrSpan = document.createElement('span'); + const addrSpan = document.createElement('span'); addrSpan.className = 'tjing-result-address'; addrSpan.textContent = course.address || ''; info.appendChild(nameSpan); info.appendChild(addrSpan); - var importBtn = document.createElement('button'); + const importBtn = document.createElement('button'); importBtn.className = 'btn-pill'; importBtn.textContent = 'Import'; (function(id, b) { @@ -282,7 +282,7 @@ async function searchTjing() { }); } catch (error) { console.error('Error searching Tjing:', error); - var errFallback = document.createElement('p'); + const errFallback = document.createElement('p'); errFallback.className = 'tjing-error'; errFallback.textContent = 'Failed to search Tjing.'; container.appendChild(errFallback); @@ -297,8 +297,8 @@ async function importFromTjing(tjingId, btn) { btn.textContent = 'Importing…'; try { - var response = await fetch('/api/tjing/import/' + encodeURIComponent(tjingId), { method: 'POST' }); - var data; + const response = await fetch('/api/tjing/import/' + encodeURIComponent(tjingId), { method: 'POST' }); + let data; try { data = await response.json(); } catch (e) { diff --git a/src/models/course.js b/src/models/course.js index 748c8a8..d2d38f7 100644 --- a/src/models/course.js +++ b/src/models/course.js @@ -8,8 +8,14 @@ function saveCourseToDB(courseData) { ON CONFLICT(link) DO UPDATE SET name = excluded.name, city = excluded.city, last_updated = datetime('now')`, [courseData.name, courseData.link, courseData.city], function(err) { - if (err) reject(err); - else resolve(this.lastID); + if (err) return reject(err); + // node-sqlite3 leaves lastID = 0 when ON CONFLICT triggers an UPDATE. + // Fall back to a SELECT to get the real id in that case. + if (this.lastID !== 0) return resolve(this.lastID); + db.get('SELECT id FROM courses WHERE link = ?', [courseData.link], (err2, row) => { + if (err2) reject(err2); + else resolve(row ? row.id : 0); + }); } ); }); diff --git a/src/routes/courses.js b/src/routes/courses.js index 7d5c70e..8db22b1 100644 --- a/src/routes/courses.js +++ b/src/routes/courses.js @@ -46,7 +46,7 @@ router.get('/partials/course-layouts/:courseId', async (req, res) => { const layouts = await getLayoutsForCourse(courseId); res.render('../partials/course-layouts', { layouts, courseId }); } catch (error) { - logger.error('Error loading course layouts:', error.message); + logger.error({ err: error }, 'Error loading course layouts'); res.status(500).send('
Error loading layouts
'); } }); @@ -56,7 +56,7 @@ router.get('/api/courses', async (req, res) => { const courses = await getAllCoursesFromDB(); res.json(courses); } catch (error) { - logger.error('Error fetching courses:', error.message); + logger.error({ err: error }, 'Error fetching courses'); res.status(500).json({ error: 'Failed to fetch courses' }); } }); @@ -67,7 +67,7 @@ router.get('/api/layouts/:courseId', async (req, res) => { const layouts = await getLayoutsForCourse(courseId); res.json(layouts); } catch (error) { - logger.error('Error fetching layouts:', error.message); + logger.error({ err: error }, 'Error fetching layouts'); res.status(500).json({ error: 'Failed to fetch layouts' }); } }); @@ -226,7 +226,7 @@ router.post('/api/scrape-layouts/:courseId', async (req, res) => { savedCount++; } } catch (err) { - logger.error(` Error updating layout ${layoutDataResult.name}:`, err.message); + logger.error({ err, layoutName: layoutDataResult.name }, 'Error updating layout'); } } } @@ -360,7 +360,7 @@ router.post('/api/scrape-event-results/:courseId', async (req, res) => { savedCount++; } } catch (err) { - logger.error(` Error updating layout ${ld.name}:`, err.message); + logger.error({ err, layoutName: ld.name }, 'Error updating layout'); } } } diff --git a/views/pages/courses.ejs b/views/pages/courses.ejs index d13646e..58dc207 100644 --- a/views/pages/courses.ejs +++ b/views/pages/courses.ejs @@ -23,7 +23,7 @@
Showing 0 of 0 courses - View all → + <%# "View all" link deferred — design spec includes it but functionality not yet implemented %>
diff --git a/views/partials/course-layouts.ejs b/views/partials/course-layouts.ejs index 76a866b..389a20b 100644 --- a/views/partials/course-layouts.ejs +++ b/views/partials/course-layouts.ejs @@ -12,10 +12,12 @@ inactiveLayouts.push(l); } }); + var RATING_TIER_HIGH = 970; + var RATING_TIER_MID = 940; function ratingTier(r) { if (r == null) return null; - if (r >= 970) return 'green'; - if (r >= 940) return 'amber'; + if (r >= RATING_TIER_HIGH) return 'green'; + if (r >= RATING_TIER_MID) return 'amber'; return 'orange'; } %> @@ -48,7 +50,11 @@
  • <%= l.name %> - Never played + <% if (l.last_played) { %> + Last played: <%= l.last_played %> + <% } else { %> + Never played + <% } %>
    Par <%= l.par %> diff --git a/views/partials/course-table.ejs b/views/partials/course-table.ejs index 3260237..bbf9b32 100644 --- a/views/partials/course-table.ejs +++ b/views/partials/course-table.ejs @@ -6,7 +6,7 @@ var layoutCount = course.layoutCount || 0; var activeLayoutCount = course.activeLayoutCount || 0; %> -
    " data-course-city="<%= (course.city || '').toLowerCase().replace(/"/g, '"') %>" onclick="toggleCourseLayouts(<%= course.id %>)"> +
    <%= course.name %> @@ -35,5 +35,5 @@
    <% }); %>
    -<%- include('course-cards', { courses: courses }) %> +<%- include('course-cards', { courses: courses, total: courses.length }) %> <% } %>