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

@@ -78,26 +78,7 @@
 
 /* ── Buttons ──────────────────────────────────────── */
 
-.btn-primary {
+/* .btn-primary is defined in shared.css — no override needed here */
-    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-pill {
     padding: 6px 12px;
@@ -332,18 +313,18 @@
 }
 
 .chip-rating--green {
-    color: oklch(0.55 0.15 150);
+    color: var(--rating-tier-high);
-    background: color-mix(in oklch, oklch(0.55 0.15 150) 10%, transparent);
+    background: color-mix(in oklch, var(--rating-tier-high) 10%, transparent);
 }
 
 .chip-rating--amber {
-    color: oklch(0.55 0.12 100);
+    color: var(--rating-tier-mid);
-    background: color-mix(in oklch, oklch(0.55 0.12 100) 10%, transparent);
+    background: color-mix(in oklch, var(--rating-tier-mid) 10%, transparent);
 }
 
 .chip-rating--orange {
-    color: oklch(0.55 0.10 50);
+    color: var(--rating-tier-low);
-    background: color-mix(in oklch, oklch(0.55 0.10 50) 10%, transparent);
+    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 {

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 {

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);

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;
+            const targetId = 'tab-pane-' + tab.dataset.tab;
-            var pane = document.getElementById(targetId);
+            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();
+        const q = input.value.toLowerCase().trim();
-        var rows = document.querySelectorAll('.course-row');
+        const rows = document.querySelectorAll('.course-row');
-        var visible = 0;
+        let visible = 0;
 
         rows.forEach(function(row) {
-            var name = row.dataset.courseName || '';
+            const name = row.dataset.courseName || '';
-            var city = row.dataset.courseCity || '';
+            const city = row.dataset.courseCity || '';
-            var match = !q || name.includes(q) || city.includes(q);
+            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');
+    const grid = document.querySelector('.course-grid');
-    var total = grid ? parseInt(grid.dataset.totalCount || '0', 10) : 0;
+    const total = grid ? parseInt(grid.dataset.totalCount || '0', 10) : 0;
-    var rows = document.querySelectorAll('.course-row');
+    const rows = document.querySelectorAll('.course-row');
-    var visible = 0;
+    let visible = 0;
     rows.forEach(function(r) { if (!r.hidden) visible++; });
 
-    var totalEl = document.getElementById('total-count');
+    const totalEl = document.getElementById('total-count');
-    var visibleEl = document.getElementById('visible-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 + '"]');
+    const row = document.querySelector('.course-row[data-course-id="' + courseId + '"]');
-    var content = document.getElementById('course-layouts-' + 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' });
+        const response = await fetch('/api/scrape-courses', { method: 'POST' });
-        var data = await response.json();
+        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' });
+        const response = await fetch('/api/scrape-layouts/' + courseId, { method: 'POST' });
-        var data = await response.json();
+        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');
+    const input = document.getElementById('tjing-search-input');
-    var btn = document.getElementById('tjing-search-btn');
+    const btn = document.getElementById('tjing-search-btn');
-    var container = document.getElementById('tjing-results');
+    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));
+        const response = await fetch('/api/tjing/search?q=' + encodeURIComponent(q));
-        var data;
+        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' });
+        const response = await fetch('/api/tjing/import/' + encodeURIComponent(tjingId), { method: 'POST' });
-        var data;
+        let data;
         try {
             data = await response.json();
         } catch (e) {

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);
+        if (err) return reject(err);
-        else resolve(this.lastID);
+        // 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);
+        });
       }
     );
   });

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('<div class="no-layouts">Error loading layouts</div>');
   }
 });
@@ -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');
         }
       }
     }

views/pages/courses.ejs

@@ -23,7 +23,7 @@
 
   <div class="results-bar">
     <span class="results-count">Showing <strong id="visible-count">0</strong> of <strong id="total-count">0</strong> courses</span>
-    <a class="results-link" href="#">View all &rarr;</a>
+    <%# "View all" link deferred — design spec includes it but functionality not yet implemented %>
   </div>
 
   <div id="course-table-region" hx-get="/partials/course-table" hx-trigger="load, refresh from:body" hx-swap="innerHTML"></div>

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 >= RATING_TIER_HIGH) return 'green';
-    if (r >= 940) return 'amber';
+    if (r >= RATING_TIER_MID)  return 'amber';
     return 'orange';
   }
 %>
@@ -48,7 +50,11 @@
         <li class="layout-card layout-card--inactive">
           <div class="layout-info">
             <span class="layout-name"><%= l.name %></span>
+            <% if (l.last_played) { %>
+              <span class="layout-last-played">Last played: <%= l.last_played %></span>
+            <% } else { %>
               <span class="layout-never-played">Never played</span>
+            <% } %>
           </div>
           <div class="layout-chips">
             <span class="chip chip-par">Par <%= l.par %></span>

views/partials/course-table.ejs

@@ -6,7 +6,7 @@
       var layoutCount = course.layoutCount || 0;
       var activeLayoutCount = course.activeLayoutCount || 0;
   %>
-    <div class="course-row expandable-row" data-course-id="<%= course.id %>" data-course-name="<%= (course.name || '').toLowerCase().replace(/"/g, '&quot;') %>" data-course-city="<%= (course.city || '').toLowerCase().replace(/"/g, '&quot;') %>" onclick="toggleCourseLayouts(<%= course.id %>)">
+    <div class="course-row expandable-row" data-course-id="<%= course.id %>" data-course-name="<%= (course.name || '').toLowerCase() %>" data-course-city="<%= (course.city || '').toLowerCase() %>" onclick="toggleCourseLayouts(<%= course.id %>)">
       <div class="course-cell">
         <span class="course-name"><%= course.name %></span>
         <span class="course-meta">
@@ -35,5 +35,5 @@
     </div>
   <% }); %>
 </div>
-<%- include('course-cards', { courses: courses }) %>
+<%- include('course-cards', { courses: courses, total: courses.length }) %>
 <% } %>