Repository navigation
Refactoring: bug fixes, less duplication, cleaner architecture - #44
Merged
Merged
Conversation
…date
ItemSort compared two completed entries with the same completion date and
start with "||" instead of "&&" and so said "less" in both directions: their
order was arbitrary (in practice reversed). It is now a chain of small key
comparisons with the same intended order (open before completed; open by
start, due date, description; completed by completion date descending,
start, description; missing dates last) and the id as final tie-break, so it
is a consistent total order.
toggleCompleted compared the boolean from prop('checked') with the string
'checked', so after a failed update the checkbox was always set to checked,
and a transport error did not revert it at all. Both failure paths now put
the checkbox back to the state of the unchanged item.
refresh() hid the "working" indicator right after starting the asynchronous
reload. reload() now returns a promise for its requests (also resolved when
one fails) and the indicator is hidden when they are finished.
updateProgress no longer shows NaN% widths when there are no entries, and
reactivate checks for an unknown id like its sibling functions.
Add UI tests for the order of completed entries, the checkbox after a failed
update and (as a guard for the earlier fix) "do today" and tap-to-edit on a
todo added without reloading.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AFJFMMtwhGjknLf1d28Trw
Remove the copies in the query endpoints, without changing any answer or status code: - queries/endpoint.php holds the start of the ten modifying endpoints (authenticate, POST and CSRF check, database); queries/todo-input.php holds the checks of the description, dates and tags of enter.php and update.php (todoInputError() returns the language key or null). - rejectRequest() replaces the echo-and-die pairs, requireSingleRow() the "no row / more than one row / success" blocks. It stops with the message unless exactly one row changed, so update.php still saves the tags and reactivate-one.php still copies the entry only on success. - query-lists.php and query-tags.php use the existing jsonQueryResults() instead of their own copy of the loop. - todoConfigPath() finds config.php (or TODO_CONFIG) for lang.php and migrate.php; db.php no longer loads it a second time, lang.php has already done so before db.php is reached. - dbQueryOrDie() is called dbQuery() (it never died); drop the $db->close() calls at the end of the requests, a stale TODO comment and the redundant check in date.php (and fix its indentation). - The pages use the configured language in <html lang> and a relative favicon URL, so that the application also works in a sub-directory. - The new include-only files are denied in .htaccess, nginx.conf.sample and tests/apache-access.sh. The checks of enter.php and update.php now run in one order (description, tags, dates; update.php then checks the notes and the rest), so a request with several invalid fields can get another message than before. Each single invalid field gets the same message. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AFJFMMtwhGjknLf1d28Trw
The files which are only included by other scripts are denied by hand in .htaccess, in nginx.conf.sample and in tests/apache-access.sh. The lists had already drifted (the Apache test missed date.php and todo-list-query.php), and nothing noticed a new helper which was not listed anywhere. tests/deny-lists.test.js derives the files from the code: the endpoints are the queries/<name>.php URLs in the client JavaScript (*.js in the root and in js/), the helpers are the other queries/*.php files; the root pages are index.php and the files it references with src/href, the include-only files are the other root .php files except config*.php. It compares them with the FilesMatch lines of .htaccess and the locations of nginx.conf.sample (missing and extra names are reported separately) and checks that every endpoint and page is a file. tests/apache-access.sh computes the same lists from the checkout instead of the hand-written entries: every helper and every include-only root file must give 403 (also with a trailing path), every page and endpoint 401 without and 200 with credentials. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AFJFMMtwhGjknLf1d28Trw
Recurrence: reactivate.php and reactivate-temp.php (a temporary table shared through include scope) become functions in queries/recurrence.php: reactivateDue(), reactivateOne() and copyRecurringEntry(). The copy claims the entry in recurringCopied and copies it from the live row inside one transaction; if the row no longer qualifies the claim is rolled back. The database user no longer needs CREATE TEMPORARY TABLES (README.md updated, which also points to scripts/migrate.php instead of sql/update-*.sql). Transactions: withTransaction() in db.php wraps the writes of enter.php, update.php, delete-tag.php, merge-tag.php and copyRecurringEntry(). The entry and its tags are now saved together; updateTags() no longer opens a transaction of its own. A version conflict still answers VERSION_CONFLICT and changes nothing. No more globals: todoUserId() replaces $curUserID, todoDb() the global $db, and todoConfig() the variables of config.php (read inside a function). MAX_TODO_LENGTH and MAX_NOTES_LENGTH move to queries/limits.php, which PHPStan loads as bootstrap file, so the ignoreErrors entry and checkMaybeUndefinedVariables: false are gone from phpstan.neon. limits.php is in both deny lists; reactivate and reactivate-temp are removed from them. log.js.php becomes a static log.js that reads the start text from $T() at runtime (the translation was put into a JS string unescaped before); index.php loads lang-js.php first. lang-js.php sends text/javascript. tests/auth.test.js no longer lists log.js.php, which does not exist any more. checkDateStr() used $ and accepted a date with a trailing newline; it uses \z now (test added with tests for the version conflict, a database error in the middle of a request and the not possible reactivation). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AFJFMMtwhGjknLf1d28Trw
Requests: postAction(url, data) sends a modifying request and returns a
promise which resolves with the answer on success and is rejected with
{ transport: true } (request failed) or { message } (the server answered
with an error text). It is the one place that decides what a success is
(an integer answer; reactivate-one.php, which answers with a text, is
handled explicitly), and reportFailure() logs and shows the failure with
exactly the texts used before. All modifying requests use them, so the
ten hand-written $.ajax calls with three different success checks are
gone, and isInt(). setTrashed(id, trashed) replaces trashItem/restoreItem
and trashLocally/restoreLocally, tagDialogAction() is the shared path of
the save/delete/merge buttons of the tag dialog, and refreshView()
replaces the repeated renderTable(); updateProgress(); pairs. The only
visible difference: after deleting a tag, reloadTagList() now runs before
refresh() like for the other two tag actions (two independent requests).
State: the shared currentlyModified is gone, toggleCompleted and
storeItemRemote keep their item in the closure, so overlapping requests no
longer overwrite each other. The implicit globals (toggledItem, result,
line, idx, merge_tagname) are declared, tagList is declared once at the
top of todo.js. The Tagify instances are kept in variables (filterTagify,
modifyTagify, mergeTagify, tagListTagify) instead of reading
element.__tagify.
addItem no longer renames five element ids by hand: the item gets its id
and the table is rendered again, so the tap handler of a row can use the
id of its item.
Dates: parseDate, parseDay, formatDate, fillStr (now padStart),
getUTCDate, dateDiffInDays and addDays move to dates.js, loaded before
todo.js.
Dead code: printItem and its double-click debug alert, the commented-out
insertion loop and autocomplete block, the update of the non-existing
#enter_effort, unused Tagify variables, the unused hidden inputs
#filter_tags, #modify_tags and #merge_tag, and the "IE" comment in log.js.
CSS: the shared class iconButton replaces the selector list of the eight
icon button classes in the three rules of todo.css (screenshots of wide
and narrow layout, hover and dialogs are pixel-identical apart from
antialiasing noise in one dialog corner).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AFJFMMtwhGjknLf1d28Trw
Descriptions, notes and tag names were run through htmlentities() before
they were stored, which encodes all 253 entities of HTML 4.01 ("Straße" was
stored as "Straße"). The client had to decode and re-escape them
everywhere, the length limits counted "&" as 5 characters, and every
other consumer of the data saw entities. The database now holds plain text
and the client escapes it when it inserts it into HTML (.text(), .val() and
Tagify take plain text as it is). encodeInput() and html_entity_decode() are
gone, the limits (255 / 10000 / 255) count the characters of the text, and
the messages for too long input no longer mention special characters.
Input which is not valid UTF-8 is refused with "Invalid parameters!"
(htmlentities() used to turn it into an empty string, now the database would
refuse it).
Admins have to run the migration right after updating the program:
php scripts/migrate.php. Migration 016 decodes the descriptions, notes, tag
names and list names with html_entity_decode(), the exact inverse of the old
encoding, in one transaction (applied completely or not at all). Tags which
become equal (case-insensitive, like "ä" and "ä") are merged into the
one with the lowest id, as 014 does; the account needs the right to create
temporary tables. The runner accepts NNN-name.php migrations next to the SQL
ones: a file that returns a function which gets the connection, because the
inverse of htmlentities does not fit in SQL. install.sql has db.version 16
(no schema change); tests/fixtures/install-v15.sql is the layout before.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AFJFMMtwhGjknLf1d28Trw
The endpoints used to answer every request with HTTP 200 and a text: "1",
an id, a count or a message, and the client had to guess from the text
whether it was a success. Now the HTTP status says it and the body is JSON:
- Success is 200 and {"ok": true}, plus the new "id" (enter.php), the
number of removed assignments as "deleted" (delete-tag.php), the new
"version" (complete.php, update.php) or the "message" of
reactivate-one.php.
- An error is {"error": "<translated message>"} with the status:
403 access denied and invalid CSRF token, 405 not a POST request,
409 conflict with the current data (version conflict, deleted entry,
tag exists, reactivation not possible, no or more than one row changed),
422 invalid input, 500 database error. rejectRequest() takes the status
at every call site; requireSingleRow() answers 409.
- queries/response.php holds the shared helpers (sendJson, sendOk,
exitWithError) for session.php and db.php; it is include-only and in the
deny lists of .htaccess and nginx.conf.sample.
- failRequest() answers JSON only for endpoints, which define
TODO_API_REQUEST (endpoint.php, the query-*.php scripts) before anything
can fail; statistik.php keeps its plain text 500.
The client's postAction() resolves with the parsed JSON on 2xx and rejects
with { message } for an error status with a JSON "error", otherwise with
{ transport: true }; the id and version of the answer are used. A refused
reactivation is still logged as result without an alert. The GET requests
for lists, todos and tags report failures (new ERROR_WHILE_LOADING) instead
of ignoring them or showing invalid data. Two small fixes: a failed trash
or restore is undone after a transmission error as well, and a failed
creation only removes the new entry if it is still in the list.
The tests check status, content type and JSON (lib.js: assertOk,
assertError, createdId) and have new tests for the statuses, the database
error (JSON for endpoints, plain text for statistik.php) and the client
behaviour on failed requests.
deleteLocally() ignores an index of -1 (an entry which is not in the list any more,
e.g. after a reload): splice(-1, 1) removed the last entry instead, after a failed
creation or after moving an entry to another list.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AFJFMMtwhGjknLf1d28Trw
untilAlert() waited for the first dialog of any kind. Reactivating shows a confirm() first and the alert only after the (failing) request, so the test 'reactivating reports the answer of the server in the log' checked the alerts before the alert came. With the headless Chromium of CI it lost that race in every job; it now waits for a dialog of the type alert. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AFJFMMtwhGjknLf1d28Trw
statistik.php is now statistics.php (the only German file name). The web server redirects the old address with 301 (RedirectMatch in .htaccess, a location in nginx.conf.sample, a check in tests/apache-access.sh), not a PHP file, which would count as an include-only file. The page used to start its HTML before the queries ran, so under a server without output buffering a database error could no longer change the status. Now queries/statistics-data.php (include-only, in both deny lists) computes the data of all 8 charts first, and statistics.php renders them afterwards: a database error answers a plain-text 500 under every server configuration. One query shape per chart: SELECT <bucket> AS bucket, COUNT(*) FROM todo WHERE <date column> >= ? AND deleted = 0 AND list in the lists of the user GROUP BY bucket (the year chart without the lower bound; NULL is the "not finished" bucket). The buckets (30 days, 26 ISO weeks, 24 months, the years present) are generated in PHP from UTC_DATE() of the database, and missing ones are filled with 0. The three SQL shapes and the hand-written digit tables are gone; the range on the date column stays sargable (the statistics test of tests/performance.test.js reads 19,275 rows, was 22,039, limit unchanged). Same numbers, with these differences (found by comparing with the old code on 1,500 simulated dates, see below): - Weeks are grouped by ISO year and week. The old grouping used the calendar year with WEEK(date, 3): around new year a week was split in two bars (53, 53) or, with today before Jan 1 in the week of Jan 1, shown at the start of the chart instead of the end, and the oldest week dropped. - Maximum, average and minimum are those of the bars shown. The old code also counted the oldest bucket which it then cut off (the 27th week, the 25th month, the 31st day at exactly midnight) in the sum, the maximum and the minimum, but divided by the number of bars shown. - The "not finished" bucket is never cut off: it sorted first and was the first to be dropped when there were more than 14 buckets (more than 13 years of entries). The years shown are the last 14. - A bar is the integer part of 100 * count / maximum high (no floating point error: 99 for the highest bar). - Entries dated after today are counted in the bar of the current week (as they were in the one of the current month); the app does not create any. Day, month and year charts: identical labels and counts on every simulated date. Weekly charts differ only where a day of the window has another calendar year than the ISO year of its week. The bars are inline SVG (rect and text with width, height, x and y attributes, no style attribute); the colors are in todo.css. Every bar has data-key and data-count, a chart data-chart (completion-day, ...), so tests can read them; the bars have a tooltip. Labels which do not fit below a bar (the dates of the daily chart) are turned instead of broken into pieces. On narrow screens the chart scrolls in its own box and the maximum, average and minimum move below it. The statistics were the last user of style attributes, so style-src of the Content-Security-Policy is 'self' only (jQuery UI and Tagify set styles with JavaScript, which is allowed). The UI tests and a walkthrough of the dialogs, tags, log and statistics at 1280 and 400 pixels show no violation. tests/statistics.test.js checks the charts against known entries (today, 8 days ago, previous month, two years ago, never completed, in the trash, of another user, edges of weeks, months and years) with a second implementation of the buckets, and the generated periods and the SQL keys around new year with the php command line. tests/ui.test.js checks the page for CSP violations, style attributes and the size of the bars. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AFJFMMtwhGjknLf1d28Trw
The yearly chart of completed entries showed the hard-coded English label "not finished" also in the German interface; it is the language key STAT_NOT_FINISHED now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AFJFMMtwhGjknLf1d28Trw
MySQL 8.0 read all entries of the user's lists through list_id for each of the six charts of days, weeks and months when its statistics of the table were old (as in the tests, after many entries were added at once): the statistics page read 46,316 rows of a table of 10,000 entries and failed the performance test, which MariaDB passed. The date index is forced for the range now: 29,223 rows on MySQL 8.0, the counts are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AFJFMMtwhGjknLf1d28Trw
todo.js (1,000 lines of global functions and variables) and init.js
become modules in js/, loaded by index.php through one
<script type="module" src="js/main.js">. jQuery, jQuery UI, lang-js.php
($T) and Tagify stay classic scripts before it, and the modules use them
as globals. A module runs after the document is parsed, so the code of the
two $(document).ready handlers runs at the top level of main.js, in the
same order as before (the former todo.js handler first, then init.js).
Nothing was rewritten: functions were moved, got export/import, and the
global variables became properties of one state object. The CSP is
unchanged (module scripts from 'self').
Modules (what imports what):
- state.js: the shared state (itemList, lists, tagList, reloadData, the
four Tagify instances) as the one exported object "state", because an
importer cannot assign to an imported binding; narrowScreenQuery is a
constant next to it.
- dates.js, html.js (escapeHtml, textToHtml): no imports. html.js is not
part of render.js, so that log.js does not depend on the renderer.
- log.js -> html.js; api.js -> log.js (postAction, reportFailure,
reportLoadFailure, the CSRF header of $.ajaxSetup as setupAjax()).
- items.js -> state: Todo, copyTodo, ItemSort, findItem, getTodoWithTag.
- actions.js: the *Locally functions and the actions (complete, trash,
reactivate, enter, store, empty the trash, do today).
- load.js: reload, loadTodos, refresh, reloadTagList.
- render.js: table, progress bar, lists, tag list, the HTML builders.
- dialogs.js: the dialogs of an entry, a tag and the log, the resize
handler for open dialogs.
- main.js: imports all of them and does what init.js and the ready handler
of todo.js did (event wiring, the Tagify instances, the first load).
render.js, actions.js, dialogs.js and load.js import each other (the
buttons of the rows call the actions, which show the result by calling
render.js); that is the only cycle, and the modules only call each other's
functions when an event happens, never while they are loaded.
Test hook: main.js sets window.todoApp = { state, ItemSort, logItems,
reloadTagList } for the UI tests and the browser console; the application
does not use it, and the old globals are gone. tests/ui.test.js reads
todoApp.state.itemList etc. instead of itemList etc. (only that changed;
the openPage() default "ready" waits for todoApp).
Caching: assetUrl() puts the modification time only into the URL of
js/main.js; the modules it imports are fetched by their plain URLs and a
browser could use old ones after an update (an import map with versions
would need an inline script, which the CSP forbids). So the web server
sends "Cache-Control: no-cache" for js/*.js: the browser asks every time
and gets a cheap 304 if the file did not change. .htaccess does that with
an <If> block in the mod_headers section, nginx.conf.sample with a
location /todo/js/ and "expires -1" (add_header there would stop the
inheritance of the security headers; a plain prefix location leaves the
regular expression locations, and so the deny rules, in force).
tests/apache-access.sh checks every module (401/200, JavaScript content
type, Cache-Control: no-cache, also on a 304 answer, and that todo.css is
not affected); tests/container.sh checks js/main.js in the image.
Checked with a browser behind Apache: with a warm cache and a changed
js/log.js the browser sends conditional requests for all modules and loads
only the new log.js; without the rule it never asked again.
The deny-list test and tests/apache-access.sh already scan js/ for the
endpoints (13, as before). The Dockerfile copies the whole checkout, and
js/ is not in .dockerignore.
nginx.conf.sample: the location for js/ repeats the HTTPS redirect, as nginx does not
inherit if/return into nested locations (checked with nginx 1.24: plain HTTP requests
of the modules were answered with 200 instead of the redirect).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AFJFMMtwhGjknLf1d28Trw
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An analysis of the application for duplication, code smells and architecture, implemented step by step. Each item is one commit, made by its own agent in a separate sub-session, then reviewed and tested before the next one started. Three fixes came out of CI and review along the way.
What changed
9db8e3d)ItemSortwas inconsistent for completed entries with the same completion date and start (||instead of&&).NaN%progress bar on an empty list.133c474)enter.phpandupdate.php.rejectRequest()andrequireSingleRow().<html lang>from the configured language, and a relative favicon URL.53c8e9a)tests/deny-lists.test.jsderives the include-only PHP files from the code and checks.htaccessandnginx.conf.sample.tests/apache-access.shderives its lists too; the old hand-written list had already drifted.b099806)CREATE TEMPORARY TABLESgrant.withTransaction(): an entry and its tags are saved together.todoUserId(),todoDb()andtodoConfig()replace the globals, so PHPStan no longer needs its exceptions.log.js.checkDateStr()no longer accepts a trailing newline.bf74322)postAction()for all modifying requests.setTrashed()and a shared tag-dialog path.currentlyModifiedor implicit globals.alert()on double-click)..iconButtonclass.3921aa1)sql/migrations/016-plain-text.php) decodes the existing data withhtml_entity_decode(), the exact inverse of the oldhtmlentities(), which encoded 253 characters incl. umlauts. It runs in one transaction and merges tags that become equal.scripts/migrate.phpnow also runs.phpmigrations.9a5dd28){"ok":true,…}or{"error":"…"}with 403/405/409/422/500; the statistics page keeps a plain-text 500.deleteLocally(-1)no longer removes the last entry.d5c45c9)statistik.php→statistics.php, with a 301 redirect in.htaccess/ nginx.style-src 'self'(no'unsafe-inline').f616f85: the "not finished" label is translated.bbd5d79)js/(main.js→state,api,items,actions,render,dialogs,load,log,dates,html), without a build step.window.todoApp, instead of globals.Cache-Control: no-cache(.htaccess, nginx), because an import map would need an inline script, which the CSP forbids.Fixes found by CI and review
1e3b2b6: the UI tests waited for the first dialog instead of the alert. Reactivating shows aconfirm()first, so the test checked too early. CI's headless Chromium lost that race in every job; local runs now use the headless shell too.a85c9e8: with stale table statistics, MySQL 8.0 read all entries per chart (46,316 rows instead of ≤ 35,000). The statistics queries now force the date index: 29,223 rows on MySQL, and the counts are unchanged.bbd5d79,nginx.conf.samplerepeats the HTTPS redirect in the newlocation /todo/js/, because nginx does not inheritif/returninto nested locations. This was checked with nginx 1.24: before the fix, modules were served over plain HTTP. The deny rules and thestatistik.phpredirect of the sample were verified too.Upgrading
php scripts/migrate.phpright after deploying (migration 016). The new code expects plain text, so an unmigrated database would show entities likeäliterally. The migration account needsCREATE TEMPORARY TABLESfor 016; the application's own user no longer needs it.config.phpis read inside a function. The sample config, the Docker-generated config and the test configs work unchanged. A hand-writtenconfig.phpmust only set the documented variables.statistik.phpwith an absolutehttp://Location. Browsers that have seen the site's HSTS header (set by.htaccess) upgrade it, and nginx/Caddy rewrite it too.php -S, the browser may keep old modules until a hard reload, since only Apache/nginx sendno-cacheforjs/.For a decision (not changed)
trash.phpdoes not incrementversion, andupdate.phplets a deleted entry be edited whilecomplete.phprefuses it. The optimistic-locking and "deleted" rules differ between the endpoints.todoUserId()is always 0, but every query checks ownership. Multi-user support is half implemented.reload()POSTsreactivate-due.phpon every list switch and "load more" (a write on every read; once a day would be enough).Testing
php -l, PHPStan, the fullnpm test(99 tests), the PHP server log free of warnings, andtests/apache-access.sh.</&stored as typed, edit, complete, trash/restore, tag dialog, statistics, with no console errors or CSP violations.🤖 Generated with Claude Code
https://claude.ai/code/session_01AFJFMMtwhGjknLf1d28Trw