Code review and fixes #44

Merged
jason merged 2 commits from cpas-checking into master 2026-03-18 16:01:40 -05:00
10 changed files with 102 additions and 18 deletions

4
.vscode/settings.json vendored Normal file
View File

@@ -0,0 +1,4 @@
{
"giteaActions.baseUrl": "https://git.alwisp.com",
"giteaActions.discovery.mode": "allAccessible"
}

View File

@@ -6,6 +6,8 @@ import ToastProvider from './components/ToastProvider';
import './styles/mobile.css'; import './styles/mobile.css';
const REPO_URL = 'https://git.alwisp.com/jason/cpas'; const REPO_URL = 'https://git.alwisp.com/jason/cpas';
// TODO [CLEANUP #18]: DevTicker is a dev vanity widget that ships to prod.
// Either gate with `import.meta.env.DEV` or remove from the footer.
const PROJECT_START = new Date('2026-03-06T11:33:32-06:00'); const PROJECT_START = new Date('2026-03-06T11:33:32-06:00');
function elapsed(from) { function elapsed(from) {
@@ -101,7 +103,9 @@ const tabs = [
{ id: 'violation', label: '+ New Violation' }, { id: 'violation', label: '+ New Violation' },
]; ];
// Responsive utility hook // TODO [MAJOR #8]: Move to src/hooks/useMediaQuery.js — this hook is duplicated
// verbatim in Dashboard.jsx. Also remove `matches` from the useEffect dep array
// (it changes inside the effect, which can cause a loop on strict-mode mount).
function useMediaQuery(query) { function useMediaQuery(query) {
const [matches, setMatches] = useState(false); const [matches, setMatches] = useState(false);
useEffect(() => { useEffect(() => {
@@ -237,6 +241,8 @@ export default function App() {
return ( return (
<ToastProvider> <ToastProvider>
{/* TODO [MAJOR #9]: Inline <style> tags re-inject on every render and duplicate
the same block from Dashboard.jsx. Move all shared mobile CSS to mobile.css */}
<style>{mobileStyles}</style> <style>{mobileStyles}</style>
<div style={s.app}> <div style={s.app}>
<nav style={s.nav} className="app-nav"> <nav style={s.nav} className="app-nav">
@@ -248,7 +254,8 @@ export default function App() {
<div className="nav-tabs"> <div className="nav-tabs">
{tabs.map(t => ( {tabs.map(t => (
<button key={t.id} style={s.tab(tab === t.id)} className="nav-tab" onClick={() => setTab(t.id)}> <button key={t.id} style={s.tab(tab === t.id)} className="nav-tab" onClick={() => setTab(t.id)}>
{isMobile ? t.label.replace('📊 ', '📊 ').replace('+ New ', '+ ') : t.label} {/* TODO [MINOR #17]: first .replace('📊 ', '📊 ') replaces string with itself — no-op. Remove it. */}
{isMobile ? t.label.replace('📊 ', '📊 ').replace('+ New ', '+ ') : t.label}
</button> </button>
))} ))}
</div> </div>

View File

@@ -112,6 +112,9 @@ export default function AuditLog({ onClose }) {
const [filterAction, setFilterAction] = useState(''); const [filterAction, setFilterAction] = useState('');
const LIMIT = 50; const LIMIT = 50;
// TODO [MAJOR #5]: `offset` in useCallback deps causes the callback to be
// re-created on each load-more, which triggers the filterType/filterAction
// useEffect unexpectedly. Track offset in a useRef instead.
const load = useCallback((reset = false) => { const load = useCallback((reset = false) => {
setLoading(true); setLoading(true);
const o = reset ? 0 : offset; const o = reset ? 0 : offset;
@@ -121,7 +124,8 @@ export default function AuditLog({ onClose }) {
axios.get('/api/audit', { params }) axios.get('/api/audit', { params })
.then(r => { .then(r => {
const data = r.data; const data = r.data;
// Client-side action filter (cheap enough at this scale) // TODO [MINOR]: client-side action filter means server still fetches LIMIT
// rows before filtering — add server-side `action` param to /api/audit.
const filtered = filterAction ? data.filter(e => e.action === filterAction) : data; const filtered = filterAction ? data.filter(e => e.action === filterAction) : data;
setEntries(prev => reset ? filtered : [...prev, ...filtered]); setEntries(prev => reset ? filtered : [...prev, ...filtered]);
setHasMore(data.length === LIMIT); setHasMore(data.length === LIMIT);

View File

@@ -29,7 +29,8 @@ function isAtRisk(points) {
return boundary !== null && (boundary - points) <= AT_RISK_THRESHOLD; return boundary !== null && (boundary - points) <= AT_RISK_THRESHOLD;
} }
// Media query hook // TODO [MAJOR #8]: Same hook is defined in App.jsx — extract to src/hooks/useMediaQuery.js
// Also: `matches` in the dep array can cause a loop on strict-mode initial mount.
function useMediaQuery(query) { function useMediaQuery(query) {
const [matches, setMatches] = useState(false); const [matches, setMatches] = useState(false);
useEffect(() => { useEffect(() => {
@@ -186,6 +187,7 @@ export default function Dashboard() {
return ( return (
<> <>
{/* TODO [MAJOR #9]: Same mobileStyles block exists in App.jsx. Move to mobile.css */}
<style>{mobileStyles}</style> <style>{mobileStyles}</style>
<div style={s.wrap} className="dashboard-wrap"> <div style={s.wrap} className="dashboard-wrap">
<div style={s.header} className="dashboard-header"> <div style={s.header} className="dashboard-header">

View File

@@ -103,13 +103,12 @@ export default function EmployeeModal({ employeeId, onClose }) {
const load = useCallback(() => { const load = useCallback(() => {
setLoading(true); setLoading(true);
Promise.all([ Promise.all([
axios.get('/api/employees'), axios.get(`/api/employees/${employeeId}`),
axios.get(`/api/employees/${employeeId}/score`), axios.get(`/api/employees/${employeeId}/score`),
axios.get(`/api/violations/employee/${employeeId}?limit=100`), axios.get(`/api/violations/employee/${employeeId}?limit=100`),
]) ])
.then(([empRes, scoreRes, violRes]) => { .then(([empRes, scoreRes, violRes]) => {
const emp = empRes.data.find((e) => e.id === employeeId); setEmployee(empRes.data || null);
setEmployee(emp || null);
setScore(scoreRes.data); setScore(scoreRes.data);
setViolations(violRes.data); setViolations(violRes.data);
}) })

View File

@@ -1,5 +1,6 @@
import React, { useState } from 'react'; import React, { useState } from 'react';
import axios from 'axios'; import axios from 'axios';
import { useToast } from './ToastProvider';
const s = { const s = {
wrapper: { marginTop: '20px' }, wrapper: { marginTop: '20px' },
@@ -53,14 +54,23 @@ export default function EmployeeNotes({ employeeId, initialNotes, onSaved }) {
const [draft, setDraft] = useState(initialNotes || ''); const [draft, setDraft] = useState(initialNotes || '');
const [saved, setSaved] = useState(initialNotes || ''); const [saved, setSaved] = useState(initialNotes || '');
const [saving, setSaving] = useState(false); const [saving, setSaving] = useState(false);
const [saveErr, setSaveErr] = useState('');
const toast = useToast();
const handleSave = async () => { const handleSave = async () => {
setSaving(true); setSaving(true);
setSaveErr('');
try { try {
await axios.patch(`/api/employees/${employeeId}/notes`, { notes: draft }); await axios.patch(`/api/employees/${employeeId}/notes`, { notes: draft });
setSaved(draft); setSaved(draft);
setEditing(false); setEditing(false);
if (onSaved) onSaved(draft); if (onSaved) onSaved(draft);
} catch (err) {
const msg = err.response?.data?.error || err.message || 'Failed to save notes';
setSaveErr(msg);
toast.error('Notes save failed: ' + msg);
// Keep editing open so the user doesn't lose their changes
} finally { } finally {
setSaving(false); setSaving(false);
} }
@@ -130,6 +140,11 @@ export default function EmployeeNotes({ employeeId, initialNotes, onSaved }) {
placeholder="Free-text notes — one per line or comma-separated. Does not affect CPAS scoring." placeholder="Free-text notes — one per line or comma-separated. Does not affect CPAS scoring."
autoFocus autoFocus
/> />
{saveErr && (
<div style={{ fontSize: '12px', color: '#ff7070', marginBottom: '6px' }}>
{saveErr}
</div>
)}
<div style={s.actions}> <div style={s.actions}>
<button style={s.saveBtn} onClick={handleSave} disabled={saving}> <button style={s.saveBtn} onClick={handleSave} disabled={saving}>
{saving ? 'Saving…' : 'Save Notes'} {saving ? 'Saving…' : 'Save Notes'}

View File

@@ -1,8 +1,8 @@
import React, { useEffect, useState } from 'react'; import React, { useEffect, useState } from 'react';
import axios from 'axios'; import axios from 'axios';
// Tier thresholds used to compute what tier an employee would drop to // TODO [MINOR #10]: This TIER_THRESHOLDS array duplicates tiers defined in CpasBadge.jsx
// after a given violation rolls off. // and Dashboard.jsx. Export TIERS from CpasBadge.jsx and import here instead.
const TIER_THRESHOLDS = [ const TIER_THRESHOLDS = [
{ min: 30, label: 'Separation', color: '#ff1744' }, { min: 30, label: 'Separation', color: '#ff1744' },
{ min: 25, label: 'Final Decision', color: '#ff6d00' }, { min: 25, label: 'Final Decision', color: '#ff6d00' },

View File

@@ -78,14 +78,12 @@ export default function NegateModal({ violation, onConfirm, onCancel }) {
}); });
}; };
// FIX: overlay click only closes on backdrop, NOT modal children
const handleOverlayClick = (e) => { const handleOverlayClick = (e) => {
if (e.target === e.currentTarget && onCancel) onCancel(); if (e.target === e.currentTarget && onCancel) onCancel();
}; };
return ( return (
<div style={s.overlay} onClick={handleOverlayClick}> <div style={s.overlay} onClick={handleOverlayClick}>
{/* FIX: stopPropagation prevents modal clicks from bubbling to overlay */}
<div style={s.modal} onClick={(e) => e.stopPropagation()}> <div style={s.modal} onClick={(e) => e.stopPropagation()}>
<div style={s.header}> <div style={s.header}>

View File

@@ -35,6 +35,7 @@ const s = {
const EMPTY_FORM = { const EMPTY_FORM = {
employeeId: '', employeeName: '', department: '', supervisor: '', witnessName: '', employeeId: '', employeeName: '', department: '', supervisor: '', witnessName: '',
violationType: '', incidentDate: '', incidentTime: '', violationType: '', incidentDate: '', incidentTime: '',
// TODO [MAJOR #6]: `amount` and `minutesLate` are rendered but never sent to the API
amount: '', minutesLate: '', location: '', additionalDetails: '', points: 1, amount: '', minutesLate: '', location: '', additionalDetails: '', points: 1,
acknowledgedBy: '', acknowledgedDate: '', acknowledgedBy: '', acknowledgedDate: '',
}; };
@@ -43,7 +44,7 @@ export default function ViolationForm() {
const [employees, setEmployees] = useState([]); const [employees, setEmployees] = useState([]);
const [form, setForm] = useState(EMPTY_FORM); const [form, setForm] = useState(EMPTY_FORM);
const [violation, setViolation] = useState(null); const [violation, setViolation] = useState(null);
const [status, setStatus] = useState(null); const [status, setStatus] = useState(null); // TODO [MAJOR #7]: remove — toast covers this
const [lastViolId, setLastViolId] = useState(null); const [lastViolId, setLastViolId] = useState(null);
const [pdfLoading, setPdfLoading] = useState(false); const [pdfLoading, setPdfLoading] = useState(false);
@@ -108,6 +109,7 @@ export default function ViolationForm() {
setEmployees(empList.data); setEmployees(empList.data);
toast.success(`Violation #${newId} recorded — click Download PDF to save the document.`); toast.success(`Violation #${newId} recorded — click Download PDF to save the document.`);
// TODO [MAJOR #7]: remove setStatus — toast above already covers this message
setStatus({ ok: true, msg: `✓ Violation #${newId} recorded — click Download PDF to save the document.` }); setStatus({ ok: true, msg: `✓ Violation #${newId} recorded — click Download PDF to save the document.` });
setForm(EMPTY_FORM); setForm(EMPTY_FORM);
setViolation(null); setViolation(null);

View File

@@ -11,6 +11,10 @@ app.use(cors());
app.use(express.json()); app.use(express.json());
app.use(express.static(path.join(__dirname, 'client', 'dist'))); app.use(express.static(path.join(__dirname, 'client', 'dist')));
// TODO [CRITICAL #1]: No authentication on any route. Add an auth middleware
// (e.g. express-session + password, or JWT) before all /api/* routes.
// Anyone on the network can currently create, delete, or negate violations.
// ── Demo static route ───────────────────────────────────────────────────────── // ── Demo static route ─────────────────────────────────────────────────────────
// Serves the standalone stakeholder demo page at /demo/index.html // Serves the standalone stakeholder demo page at /demo/index.html
// Must be registered before the SPA catch-all below. // Must be registered before the SPA catch-all below.
@@ -49,6 +53,13 @@ app.get('/api/employees', (req, res) => {
res.json(rows); res.json(rows);
}); });
// GET /api/employees/:id — single employee record
app.get('/api/employees/:id', (req, res) => {
const emp = db.prepare('SELECT id, name, department, supervisor, notes FROM employees WHERE id = ?').get(req.params.id);
if (!emp) return res.status(404).json({ error: 'Employee not found' });
res.json(emp);
});
app.post('/api/employees', (req, res) => { app.post('/api/employees', (req, res) => {
const { name, department, supervisor } = req.body; const { name, department, supervisor } = req.body;
if (!name) return res.status(400).json({ error: 'name is required' }); if (!name) return res.status(400).json({ error: 'name is required' });
@@ -58,6 +69,9 @@ app.post('/api/employees', (req, res) => {
db.prepare('UPDATE employees SET department = COALESCE(?, department), supervisor = COALESCE(?, supervisor) WHERE id = ?') db.prepare('UPDATE employees SET department = COALESCE(?, department), supervisor = COALESCE(?, supervisor) WHERE id = ?')
.run(department || null, supervisor || null, existing.id); .run(department || null, supervisor || null, existing.id);
} }
// TODO [MINOR #16]: Spreading `existing` then overwriting with possibly-undefined
// `department`/`supervisor` returns `undefined` for unset fields.
// Re-query after update or only spread defined values.
return res.json({ ...existing, department, supervisor }); return res.json({ ...existing, department, supervisor });
} }
const result = db.prepare('INSERT INTO employees (name, department, supervisor) VALUES (?, ?, ?)') const result = db.prepare('INSERT INTO employees (name, department, supervisor) VALUES (?, ?, ?)')
@@ -290,6 +304,17 @@ app.post('/api/violations', (req, res) => {
// PATCH /api/violations/:id/amend — edit mutable fields; logs a diff per changed field // PATCH /api/violations/:id/amend — edit mutable fields; logs a diff per changed field
const AMENDABLE_FIELDS = ['incident_time', 'location', 'details', 'submitted_by', 'witness_name', 'acknowledged_by', 'acknowledged_date']; const AMENDABLE_FIELDS = ['incident_time', 'location', 'details', 'submitted_by', 'witness_name', 'acknowledged_by', 'acknowledged_date'];
// Pre-build one prepared UPDATE statement per amendable field combination is not
// practical (2^n combos), so instead we validate columns against the static
// whitelist and build the clause only from known-safe names at startup.
// The whitelist itself is the guard; no user-supplied column name ever enters SQL.
const AMEND_UPDATE_STMTS = Object.fromEntries(
AMENDABLE_FIELDS.map(f => [
f,
db.prepare(`UPDATE violations SET ${f} = ? WHERE id = ?`)
])
);
app.patch('/api/violations/:id/amend', (req, res) => { app.patch('/api/violations/:id/amend', (req, res) => {
const id = parseInt(req.params.id); const id = parseInt(req.params.id);
const { changed_by, ...updates } = req.body; const { changed_by, ...updates } = req.body;
@@ -307,18 +332,14 @@ app.patch('/api/violations/:id/amend', (req, res) => {
} }
const amendTransaction = db.transaction(() => { const amendTransaction = db.transaction(() => {
// Build UPDATE
const setClauses = Object.keys(allowed).map(k => `${k} = ?`).join(', ');
const values = [...Object.values(allowed), id];
db.prepare(`UPDATE violations SET ${setClauses} WHERE id = ?`).run(...values);
// Insert an amendment record per changed field
const insertAmendment = db.prepare(` const insertAmendment = db.prepare(`
INSERT INTO violation_amendments (violation_id, changed_by, field_name, old_value, new_value) INSERT INTO violation_amendments (violation_id, changed_by, field_name, old_value, new_value)
VALUES (?, ?, ?, ?, ?) VALUES (?, ?, ?, ?, ?)
`); `);
for (const [field, newVal] of Object.entries(allowed)) { for (const [field, newVal] of Object.entries(allowed)) {
const oldVal = violation[field]; const oldVal = violation[field];
// Use the pre-built statement for this field — no runtime interpolation
AMEND_UPDATE_STMTS[field].run(newVal, id);
if (String(oldVal) !== String(newVal)) { if (String(oldVal) !== String(newVal)) {
insertAmendment.run(id, changed_by || null, field, oldVal ?? null, newVal ?? null); insertAmendment.run(id, changed_by || null, field, oldVal ?? null, newVal ?? null);
} }
@@ -391,6 +412,38 @@ app.delete('/api/violations/:id', (req, res) => {
res.json({ success: true }); res.json({ success: true });
}); });
// ── Violation counts per employee ────────────────────────────────────────────
// GET /api/employees/:id/violation-counts
// Returns { violation_type: count } for the rolling 90-day window (non-negated).
app.get('/api/employees/:id/violation-counts', (req, res) => {
const rows = db.prepare(`
SELECT violation_type, COUNT(*) AS count
FROM violations
WHERE employee_id = ?
AND negated = 0
AND incident_date >= DATE('now', '-90 days')
GROUP BY violation_type
`).all(req.params.id);
const result = {};
for (const r of rows) result[r.violation_type] = r.count;
res.json(result);
});
// GET /api/employees/:id/violation-counts/alltime
// Returns { violation_type: { count, max_points_used } } across all time (non-negated).
app.get('/api/employees/:id/violation-counts/alltime', (req, res) => {
const rows = db.prepare(`
SELECT violation_type, COUNT(*) AS count, MAX(points) AS max_points_used
FROM violations
WHERE employee_id = ?
AND negated = 0
GROUP BY violation_type
`).all(req.params.id);
const result = {};
for (const r of rows) result[r.violation_type] = { count: r.count, max_points_used: r.max_points_used };
res.json(result);
});
// ── Audit log ──────────────────────────────────────────────────────────────── // ── Audit log ────────────────────────────────────────────────────────────────
app.get('/api/audit', (req, res) => { app.get('/api/audit', (req, res) => {
const limit = Math.min(parseInt(req.query.limit) || 100, 500); const limit = Math.min(parseInt(req.query.limit) || 100, 500);