feat: support ticketing dashboard with live chat and department RBAC

[vendz]

Summary

• New admin dashboard (admin/ticket/tickets.html + tickets.js) for the chat-based support ticketing system: ticket list with polling refresh, live chat drawer over SSE, status updates, department-scoped filtering.
• Department RBAC: TICKET_ROLE_SERVICE_MAP mirrors the backend's role→service mapping so each department admin (maintenanceAdmin, housekeepingAdmin, foodAdmin, travelAdmin, officeAdmin) only sees their own tickets; superAdmin sees everything. Server-side enforcement lives in the backend PR — this is UX-level filtering plus the shared "Support Tickets" home button wired into adminhome.html for the relevant roles.
• SSE is consumed via a hand-rolled fetch + ReadableStream reader (vanilla JS admin panel can't use browser EventSource since it can't set the Authorization header), with reconnect/watchdog handling for dropped connections.

Key pieces

• admin/ticket/tickets.js — list/detail/stream logic, scroll-position-preserving message rendering, stale-response race protection when switching tickets quickly.
• style/js/roleCheck.js — shared getRoles() helper (previously each page parsed sessionStorage roles independently).
• style/js/htmlEscape.js — shared escapeHtml() (previously duplicated across 3 files).
• admin/adminhome.html — added "Support Tickets" button for relevant department roles.

Testing

• Manual end-to-end pass via local static server + local backend: opening tickets, live message delivery, status changes reflecting instantly on the app side, role-based filtering per department, reconnect after simulated drops.
• Went through a full /code-review pass (xhigh effort, 15 verified findings) — all fixed. Followed by a /simplify cleanup pass — all findings applied or explicitly skipped as false positives.

Test plan

• Confirm each department admin role is actually assigned to the right prod accounts (ops task, not code)
• Smoke test in a real browser against the deployed backend (local testing used a temporary local API override, reverted before commit)

[claude]

Claude finished @vendz's task in 5m 47s —— View job

---

Code Review — Support Ticketing Dashboard

Nice work — this is a well-structured feature. The SSE handling (fetch + ReadableStream with watchdog/reconnect), the stale-response race guards when switching tickets quickly, scroll-position preservation, message dedup by data-msg-id, and the consistent escapeHtml() on all innerHTML sinks (with textContent for the debug panel) are all done carefully, and the inline comments explaining why are genuinely helpful. XSS surface looks well covered.

The main theme in my findings is HTTP error handling: several fetch calls don't check response.ok, which both introduces bugs and deviates from the repo convention (existing code checks response.ok + surfaces result.message — e.g. admin/food/plateCount.js, admin/maintenance/updateRequest.js, admin/travel/updateBookingStatus.js).

Findings (posted inline):

• 🔴 sendReply treats a failed send as success — clears the textarea + reloads on 4xx/5xx, silently losing the admin's message. → inline
• 🟠 updateStatus reports success even when the PATCH fails. → inline
• 🟠 Stream never checks resp.ok — a 401/403/500 causes an indefinite 3s reconnect storm (and a TypeError if resp.body is null). Consider not retrying on 4xx. → inline
• 🟠 loadTicketDetails has no res.ok/data guard — an error response makes populateDrawer(undefined) throw and opens an empty drawer with no error. → inline
• 🟠 Status dropdown is missing closed — opening a Closed ticket renders a blank select and Update Status can PATCH status: "". → inline
• 🟡 Unread-tracking key desync — the URI-encoded id becomes currentTicketId while the table reads unread state with the raw id; harmless for int/UUID ids, fragile otherwise. → inline

Minor notes (no inline comment needed):

• serviceFilter has an "IT Support" option (tickets.html:289) that has no entry in TICKET_ROLE_SERVICE_MAP and isn't in the map you say mirrors the backend — worth confirming it's a real backend service value, otherwise it's a dead option only superAdmin ever sees.
• In the stream loop, a single malformed frame makes JSON.parse (tickets.js:484) throw out of the read loop → reconnect + buffer loss. Wrapping the per-frame parse in try/catch would make one bad frame non-fatal.
• getRoles() / escapeHtml() extraction and reuse across pages is a good cleanup. 👍

None of these are blocking on their own, but the sendReply data-loss one is worth fixing before merge. I did not verify the backend role→service map or endpoints (separate PR) — the frontend-side mirror looks internally consistent aside from the "IT Support" note.

---

• Gather context (diff, changed files, repo conventions)
• Review admin/ticket/tickets.js (list/detail/SSE stream logic)
• Review admin/ticket/tickets.html
• Review style/js/roleCheck.js + style/js/htmlEscape.js
• Review admin/adminhome.html
• Post inline findings + summary

[claude]

Bug (high): a failed send is treated as success and silently loses the admin's message.

fetch only rejects on network errors, not on HTTP 4xx/5xx. If the POST returns e.g. 403/500, execution still falls through to messageBox.value = '' and loadTicketDetails(), so the textarea is cleared and no error is shown — the typed reply is gone. The catch only fires for network failures.

This also deviates from the repo convention of checking response.ok and surfacing result.message (see admin/maintenance/updateRequest.js, admin/food/plateCount.js).

Suggested change

	await fetch(`${CONFIG.basePath}/tickets/${currentTicketId}/messages`, {
	method: 'POST',
	headers: authHeaders(true),
	body: JSON.stringify({ message })
	});
	messageBox.value = '';
	await loadTicketDetails();
	const res = await fetch(`${CONFIG.basePath}/tickets/${currentTicketId}/messages`, {
	method: 'POST',
	headers: authHeaders(true),
	body: JSON.stringify({ message })
	});
	if (!res.ok) throw new Error('Request failed');
	messageBox.value = '';
	await loadTicketDetails();

[claude]

Bug (medium): status update reports success even when the PATCH fails.

Same issue as sendReply — an HTTP 4xx/5xx doesn't reject the promise, so the code proceeds to reload as if the status changed. The admin sees no error and believes the status was updated. Guard on res.ok to keep it consistent with the rest of the admin panel.

Suggested change

	await fetch(`${CONFIG.basePath}/tickets/${currentTicketId}/status`, {
	method: 'PATCH',
	headers: authHeaders(true),
	body: JSON.stringify({ status })
	});
	const res = await fetch(`${CONFIG.basePath}/tickets/${currentTicketId}/status`, {
	method: 'PATCH',
	headers: authHeaders(true),
	body: JSON.stringify({ status })
	});
	if (!res.ok) throw new Error('Request failed');

[claude]

Bug (medium): the stream never checks resp.ok, causing a reconnect storm on auth/permission errors.

If the stream request comes back non-2xx (expired token → 401, forbidden ticket/department → 403, server error → 500), the response still has a body, so resp.body.getReader() reads the small error JSON, finds no data: frame, hits done, and calls scheduleStreamReconnect(). Since currentTicketId still matches, this retries every 3s indefinitely against a request that will never succeed — hammering the backend with an expired/forbidden token. Also, if resp.body is ever null, line 467 throws TypeError (then reconnect-loops the same way).

Minimal guard below. Beyond this, consider not retrying on client (4xx) errors in scheduleStreamReconnect so an expired token or forbidden ticket stops the loop instead of storming.

Suggested change

	const resp = await fetch(`${CONFIG.basePath}/tickets/${ticketId}/stream`, {
	headers: authHeaders(),
	signal: controller.signal
	});
	const reader = resp.body.getReader();
	const resp = await fetch(`${CONFIG.basePath}/tickets/${ticketId}/stream`, {
	headers: authHeaders(),
	signal: controller.signal
	});
	if (!resp.ok || !resp.body) throw new Error(`Stream failed: ${resp.status}`);
	const reader = resp.body.getReader();

[claude]

Bug (medium): no res.ok/result.data guard — an error response makes populateDrawer(undefined) throw.

On any non-2xx (or a 200 without data), result.data is undefined, and populateDrawer(ticket) immediately dereferences ticket.id → TypeError, swallowed by the outer catch. Net effect: openTicket() still awaits this successfully and opens an empty drawer with no error shown. The list fetch (fetchTickets, line 103) has the same missing check but degrades to an empty table instead of throwing.

Suggested change

	const res = await fetch(`${CONFIG.basePath}/tickets/${targetTicketId}`, {
	headers: authHeaders()
	});
	const result = await res.json();
	const ticket = result.data;
	const res = await fetch(`${CONFIG.basePath}/tickets/${targetTicketId}`, {
	headers: authHeaders()
	});
	if (!res.ok) throw new Error('Request failed');
	const result = await res.json();
	const ticket = result.data;

[claude]

Bug (medium): the status dropdown is missing closed, so closed tickets misbehave.

The list filter (line 283) lets an admin filter to Closed tickets and open one. When populateDrawer runs statusUpdateSelect.value = ticket.status (tickets.js:251) with ticket.status === 'closed', there is no matching <option>, so the select renders blank (selectedIndex -1). If the admin then clicks Update Status, document.getElementById('statusUpdateSelect').value returns '' and the code PATCHes status: "".

Add the missing option so closed tickets display correctly:

Suggested change

	<select id="statusUpdateSelect" class="form-control">
	<option value="open">Open</option>
	<option value="in progress">In Progress</option>
	<option value="resolved">Resolved</option>
	</select>
	<select id="statusUpdateSelect" class="form-control">
	<option value="open">Open</option>
	<option value="in progress">In Progress</option>
	<option value="resolved">Resolved</option>
	<option value="closed">Closed</option>
	</select>

If admins are intentionally not allowed to (re)set closed, instead guard updateStatus() to bail when the selected value is empty.

[claude]

Latent bug (low, id-format dependent): the encoded id becomes currentTicketId, desyncing the unread-tracking keys.

openTicket sets currentTicketId = ticketId from this URI-encoded value, and setLastSeen/getLastSeen in loadTicketDetails/appendMessage key off it. But renderTicketTable reads getLastSeen(t.id) with the raw id (line 125). If a ticket id ever contains a character encodeURIComponent changes (space, #, /, etc.), the "seen" timestamp is stored under one key and read under another, so the unread dot never clears.

Harmless for plain integer / UUID ids (no-op encoding), but fragile. Cleaner: keep currentTicketId raw and apply encodeURIComponent only where it's interpolated into a fetch URL. If you keep it as-is, at least add a note that ids must be encoding-safe.

[claude]

Re-review after new commits

The PR was updated (latest commit fix(tickets): address PR review findings). I'm re-reviewing the current state to confirm the earlier findings are resolved and to check for anything new.

• Gather context (diff vs origin/dev, changed files, repo conventions)
• Verify the 6 prior findings were addressed
• Check for any new issues introduced by the fixes
• Post inline findings (if any) + summary

View job run

[claude]

Claude finished @vendz's task in 4m 30s —— View job

---

Re-review after new commits

The PR grew substantially since the last review pass — 12 new commits added a media-attachments feature (image/video upload + display) and remapped the RBAC model to a 12-department taxonomy, on top of the fixes for the prior findings. Verified both.

• Gather context (diff vs origin/dev, current file state, repo conventions)
• Verify the 6 prior findings were addressed
• Check for any new issues introduced by fixes/new commits
• Post inline findings + summary

Prior findings — all confirmed fixed:

• ✅ sendReply now checks res.ok before clearing the composer (tickets.js:857)
• ✅ updateStatus now checks res.ok before reloading (tickets.js:893)
• ✅ Stream fetch now checks resp.ok || !resp.body and stops retrying on 4xx instead of storming every 3s (tickets.js:954, 1036)
• ✅ loadTicketDetails now guards on res.ok and missing ticket data, and surfaces a failure via alert() in openTicket instead of opening an empty drawer (tickets.js:438, 410)
• ✅ Status dropdown now has a disabled closed option so a closed ticket's select shows correctly instead of blank, and the whole composer/status control is locked when status === 'closed' (tickets.html:572, tickets.js:513)
• ✅ Unread-tracking desync fixed — openTicket now decodeURIComponents the id before assigning currentTicketId, so it matches the raw id used by the table's getLastSeen(t.id) (tickets.js:384)
• ✅ Both minor notes from the first pass are also resolved: the stream's per-frame JSON.parse is now wrapped in try/catch (tickets.js:979), and the dead "IT Support" filter option is gone, replaced by an IT: [] entry in the new 12-department map that's correctly superAdmin-only.

New finding (posted inline):

• 🟡 tickets.js:828 — sendReply() captures pendingReplyFiles by reference, not a copy. Since attachImageBtn stays enabled during an in-flight send, a file picked while a previous send is still uploading gets swept into that in-flight send's attachment list with no uploaded key, which either 400s the send or silently drops the image the admin meant for their next message. Fix is a one-line .slice().

Everything else in the new media/RBAC code (auth'd fetch → object URL lifecycle, render-generation guards against stale re-renders, Promise.all-parallelized uploads with idempotent retry via _uploadedKey, department→role service map mirroring, escapeHtml/textContent usage on all attachment-related DOM writes) held up under review — no other correctness or XSS issues found.

[claude]

Claude Code is working…

I'll analyze this and get back to you.

View job run