Ticket Workspace PR Review - edmozley/freeitsm GitHub Wiki

Ticket workspace, task editor and ticket table: review notes (#211-#213)

✅ Done - merged, and released in 4.3.0 on 27 October 2026: @Dr4inder's three commits, each merged as its own pull request, followed by one review commit of mine. This page is the record of the review.

Written for @Dr4inder, who built all three, and for anyone - or any AI coding assistant - contributing to FreeITSM next. Same shape as the other reviews: your code does X, the problem is Y, the approach now is Z, then a verdict: keep, change or leave out. Lessons that keep coming back are collected in Contributing.

PR	What	Verdict
#211	The ticket workspace: install setting, personal switch, audited Make workspace flag, one renderer	Merge with fixes
#212	A linked task's own editor inside the workspace's Work tab	Merge with fixes (one real bug)
#213	The ticket table: server-side paging, saved views, a CSV export behind a setting	Merge with fixes (mostly style)
#206	The original draft, all three in one	Closed - replaced by the three above

How it got here

The first version was one draft pull request, #206: a good idea, careful security, and three features in one branch, 49 commits behind main. Rather than review it as it was, I wrote down how it should fit - the design page - with four open decisions, and asked for three pull requests on current main.

Every point on that page came back as asked: the three settings with Off as the default, the personal switch, Make workspace hidden in Every ticket mode, the badge, the export setting, one renderer instead of moving controls after drawing, and SQL columns as parameters instead of str_replace(). All four of my recommendations were accepted. That is the reason this review is short: when the design is agreed first, the review is about the code, not the idea.

How I checked it. The PRs' own suites need a database they can write to, and my development database is real data - so I made a throwaway install: a git worktree of the merged branch under the web root, its own config.php, and a fresh database loaded from database/freeitsm.sql. All three suites ran there - 226 checks, including three that drive a real Chrome through the real page - before my changes and again after, both times green. The recipe is in the workspace developer guide.

Keep - nearly all of it

One renderer, done properly. displayEmail() now defines every control once and chooses where it goes - and the classic pane is drawn by the same code, in the same order. A control added tomorrow appears in both layouts with nothing else to do. The TRAP: comment you put there is exactly the right one.
The service does the auditing. The workspace flag is validated, guarded until Database Verification has run, audited server-side whoever sends it, and a flag-only change returns before workflows. Your TRAP: - test what actually changed, not which keys were sent - catches the case I would have missed: a client resending every field with only the flag different.
The task embed's checks. taskEmbedCanOpen() walks the subtask chain to the linked root, checking access at every step, with a loop guard; api/tasks/get.php re-checks on every read; the frame and the ticket check origin, sender window and context on every message. Writes go through the ordinary Tasks endpoints, so there is no second permission model to keep in step.
Unsaved work. canLeave(), holdRender() / flushRender(), ticketReadGuard() and the bridge's leave handshake between them mean no draft, half-typed field or task edit is lost to a click, a layout switch or a reload. That is a lot of care for an edge most contributions never think about.
One query for the list and the table. Moving the inbox query into includes/ticket_list.php, with its comments, and passing the table's columns in rather than splicing them into SQL.
The export. A setting (Administrators only by default), checked on every request; formula neutralisation; one snapshot across batches; a buffer so a failure never sends half a file; and the distinction between UTC timestamps and wall-clock times - which you marked with a TRAP:.
The tests. Positive controls (a real workflow that does fire on a mixed change), failing-base controls, fixtures created by id and removed in finally, and browser tests in PHP with no Node.js - the Contributing rule from PR #198, followed.

Change

#	Finding	PR	Verdict
1	`crypto.randomUUID()` only exists on HTTPS	#212	Change - a bug
2	The live tests assumed a web-root install	all	Change
3	Dense code and missing comments	#213 mostly	Change
4	The export policy was a string list in four places	#213	Change
5	Two different validation blocks for two enum settings	#211, #213	Change
6	Small things	all	Change

1. crypto.randomUUID(). Your code gave each "may I close you?" question to the task frame a UUID. The problem: browsers only provide crypto.randomUUID() in a secure context - HTTPS, or localhost. Your test machine was localhost, so it worked; on an install reached as http://helpdesk/ inside a network - which is common for FreeITSM - it throws. taskCanLeave() would fail, and with it canLeave(), so no ticket could be left while a task was open. Now: a counter ('leave-' + ++leaveRequestSeq). The id only has to tell this page's own questions apart, so it needs no randomness. A TRAP: comment says why. The same rule already applies to navigator.clipboard, which is why assets/js/clipboard.js checks window.isSecureContext.

2. Web-root URLs in the tests. Your tests logged in with fetch('/auth/login.php') and checked the case report's path was exactly /tickets/case-report.php. The problem: my install is at http://localhost/freeitsm-app/, so the login went to a page that does not exist and every live test failed with "currentEmail is not defined" - the page never loaded, which looks like a code bug and is not. Now: the login is relative to the page the browser is on, and the path check uses endsWith(). FreeITSM supports being installed in a subfolder (BASE_URL), so tests have to as well.

3. House style. Your code in includes/ticket_list.php (the new half), api/tickets/table.php, api/tickets/table_export.php, assets/js/tickets-table.js and the new parts of assets/js/data-table.js was written several statements to a line with no spaces - $page=$string('page','1'); $size=$string('page_size','50'); - and with one-line docblocks. The problem: every file around it is laid out one statement per line, with comments that say why, and the next person to change it (often me, often an AI assistant) needs that more than the author did. #211 and #212 were closer, but the two most subtle pieces - the window.fetch wrappers in assets/js/ticket-workspace.js and assets/js/task-embed-bridge.js - had no explanation at all. Now: reformatted to match, with file headers, docblocks, and TRAP: comments on both fetch wrappers. The three separate maps of the six filter columns (filter SQL, facet lookups, row fields) are one map, ticketListFilters(). Behaviour is unchanged - the same 226 checks pass. I also put back the comments #211 dropped from handleEmailRowClick() ("⌘ on a Mac does the same job", "the anchor deliberately STAYS PUT"): a rewrite should carry the old comments across unless they have become untrue.

4. An enum for the export policy. Your code listed ['admins', 'ticket_users', 'nobody'] in the settings save, the settings page's PHP, its JavaScript and ticketListCanExport(). The problem: since 4.0, a closed list the code owns is an enum (Contributing - Fixed lists are enums) - you had done exactly that for TicketWorkspaceMode in #211. Now: TicketListExport, beside it.

5. One validation block. The two PRs each added their own check to api/settings/save_system_settings.php, in different shapes (a foreach with an if inside, and an in_array()). Now: one table of setting key => enum, checked before any key is written, so a combined save with one bad value is refused whole.

6. Small things.

The two new settings were at opposite ends of the General tab; they sit together now.
The settings page's loader had been inserted between the "Absent (never saved) means ON" comment and the line it describes.
saveNote, sendEmail and sendChannelMessage were wrapped in try { ... } finally { ticketPendingWrites--; ... } with their whole bodies left unindented. They now call trackTicketWrite(), and the original bodies are untouched, so the diff against main is tiny.
catch (e) { ... } /* silent — ... */ } on one line, and a if (!data.success) return; straight after a throw on the same condition.
assets/css/ticket-workspace.css had two identical @container blocks; merged.
The preferences heading said Tickets for a switch that is about the workspace.
api/tickets/assign_ticket.php gained requireModuleWriteJson('tickets'). Tickets has no Read only level, so today it always passes - I kept it, with a comment saying so, because it is the right question and starts working the day Tickets gets one. The csrfRequireWriteMethod() you added beside it brings it in line with every other ticket write.

Things I checked that were fine

Classic-pane behaviour. Several section loaders now throw on success: false where they used to return quietly. In the reading pane that only adds a console line on a genuine error - the endpoints return success: false only for real failures, never for "nothing to show" (time tracking switched off is success: true, disabled: true).
The REST spec. The requestBodies entry for PATCH /tickets/{id} types only workspace but keeps additionalProperties: true, so no documented field was lost. php api/v1/lib/openapi_check.php passes.
PHP 7.4 syntax in directly-run files. None - the enums and match are all under includes/.
Cache-busting. Every changed script and stylesheet is loaded with a version newer than main's.
Merging the three. #212 and #213 conflict in five files. Your own integration branch had resolved them; mine matched it exactly.

For next time

Secure-context APIs - crypto.randomUUID(), navigator.clipboard, service workers - are missing on plain HTTP. Check window.isSecureContext, or use something that does not need one. Now in Contributing.
Tests must work on a subfolder install. Build every URL from FREEITSM_TEST_URL, or relative to the page. Now in Contributing.
Match the layout of the file you are in - one statement per line, spaces around operators, a docblock that says why. Compact code costs the reviewer and every reader after.
Wrapping a global (window.fetch, window.showAuditHistory) is sometimes the right call, as it was here - but it deserves the biggest comment in the file, because nobody looking at a fetch() call elsewhere will guess it is being counted.
Everything else here is the standard to copy: the design agreed first, three focused pull requests, one renderer, server-side checks for anything the browser hides, and tests with positive and failing-base controls.