Fix missing task groups and duplicate click listener in All Tasks view#959
Open
Ridanshi wants to merge 1 commit into
Open
Fix missing task groups and duplicate click listener in All Tasks view#959Ridanshi wants to merge 1 commit into
Ridanshi wants to merge 1 commit into
Conversation
Three independent bugs were causing the All Tasks view to be broken: 1. Automatic Semicolon Insertion trap in renderTasks: the innerHTML assignment for the all-tasks / archived view ended line 725 with a bare `)` and placed the `+` operator at the start of line 726. JavaScript's ASI rules can insert a semicolon after the first renderGroup call, discarding the concatenation of the This week and Completed groups. Fixed by moving every `+` to the end of its line so the statement continuation is unambiguous. 2. Duplicate `const calendarDownloadBtn` declaration: the variable was declared at module scope on both line 89 and line 1384. Duplicate `const` at the same scope is a SyntaxError in ES modules (which are always strict), preventing the entire module from parsing. The redundant declaration at line 1384 is removed; the `if (calendarDownloadBtn)` guard at line 1386 continues to use the binding from line 89. 3. addItemsBtn duplicate listener risk: the click handler was registered inside the DOMContentLoaded callback while all similar paste-workflow handlers (extractBtn, clearBtn, etc.) live at module scope. Moving it outside DOMContentLoaded and switching to an `onclick` property assignment (rather than addEventListener) guarantees only one handler can fire per click even if the registration path is accidentally executed more than once. Adds tests/taskRendering.test.js with 19 tests covering task classification, renderGroup HTML output, all-three-groups rendering, archived view, event-handler insertion counts, onclick single-handler semantics, and regression cases for both the ASI and duplicate- insertion bugs. All 19 new tests and 5 existing ICS tests pass.
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.
Closes #353
Root cause analysis
Three independent bugs in
js/app.jscombined to break the All Tasks view:Bug 1 — ASI trap in
renderTasks(missing task groups)The
innerHTMLassignment for the all-tasks / archived branch ended line 725 with a bare)and placed the+operator at the start of line 726:Because the statement up to and including the first
renderGroup(...)call is a syntactically completeExpressionStatement, JavaScript's Automatic Semicolon Insertion can insert a semicolon after that line. TheThis weekandCompletedgroups are then evaluated as a separate, discarded expression — they are never appended toinnerHTML.Fix: move every
+to the end of its line so the continuation is always unambiguous:Bug 2 — Duplicate
const calendarDownloadBtndeclaration (SyntaxError)calendarDownloadBtnwas declared withconstat module scope on both line 89 and line 1384. Duplicateconstbindings in the same scope are aSyntaxErrorin ES modules (which are always strict mode). This prevents the entire module from parsing, making the app completely non-functional in modern environments.Fix: remove the redundant declaration at line 1384. The
if (calendarDownloadBtn)guard on line 1386 continues to use the binding from line 89.Bug 3 — Duplicate
addItemsBtnclick listener riskThe
addItemsBtnclick handler was registered inside theDOMContentLoadedcallback while all similar paste-workflow handlers (extractBtn,clearBtn,pasteInput) are registered at module scope. This inconsistency created the risk that the handler could be registered from both execution contexts.Fix: move the handler outside
DOMContentLoaded(consistent with the other paste-workflow listeners) and switch fromaddEventListenerto anonclickproperty assignment. Anonclickassignment guarantees only one handler fires per click regardless of how many times the registration code is reached.Files modified
js/app.js— three targeted changes as described abovetests/taskRendering.test.js(new) — 19 testsTests added (
tests/taskRendering.test.js)Tests use Node's built-in
node:testrunner (same runner as the existing ICS / CSV test suites). Pure functions are tested without DOM dependencies.Coverage:
renderGroup: empty array →'', non-empty → correct HTML structure, per-task card countclearExtracted; regression documenting double-insertion when guard is absentonclicksemantics: second assignment overwrites first, single handler fires per click+pattern vs. ASI-truncated pattern; task counts per bucketTest results