Repository navigation
Postcss hotfix - #17
Merged
Merged
Conversation
This was referenced Aug 25, 2026
Merged
roncodes
added a commit
that referenced
this pull request
Aug 25, 2026
The comments indexed findings under an earlier scheme running to #160, which this file restarted at #1. That numbering still appears in source comments — full-calendar-test.js cites "DEFECTS.md #94 for Leaflet" — and resolved to nothing once the comments went. Appendix A maps the old numbering to what actually happened. I verified each against current source rather than copying the claims across, and every finding in that set has since been resolved: filter/multi-option's task-called-as-a- function, overlay.resize clamping the wrong dimension, custom-field/form (the file is gone), set-height's "px", leaflet's never-set `initialized`, transition-to's self-satisfying assert, resource-context-panel's validate order, the money arm, and the whole dead-member list. #148 in particular — the three query-builder validate* actions nothing performed, so a panel kept sorting by deselected columns — is wired to {{did-update}} in all three panels now. Appendix B keeps the seven categories the remaining gaps fall into, including the distinction that cost the most time: a default on a plain function is real reachable surface, a default on a framework-invoked signature is dead, and istanbul's branch map cannot tell them apart. Category 7 is annotated with what has happened since — most of the "harness question" files turned out to be drivable, so it is now flagged as unverified-until-tried. Appendix C keeps the habits, led by the one that keeps proving itself: a green test is not evidence that the branch you aimed at ran. Nothing is imported as an open item. The tracker's open list is unchanged at #1-#17.
roncodes
added a commit
that referenced
this pull request
Aug 25, 2026
Statements, functions and lines to 100%; branches 78.49% -> 87.09%. Eleven
tests, several of which are worth having whatever the coverage number says:
- Drag and resize gestures reach the data model. Driven with real
PointerEvents through interact.js, end to end from gesture to saved
template. moveElement and resizeElement had never executed.
- The undo history caps at fifty steps. Real user-visible behaviour that no
test touched.
- Reordering two layers leaves a third untouched. The existing reorder tests
use two elements, so the map()'s fall-through was structurally unreachable
from them.
- An unrecognised paper size keeps its stored dimensions instead of blanking
them.
- Six optional-argument paths: save, preview, contextSchemas, and the
ember-data record shapes.
Four of my own tests failed first, each a different wrong assumption, all worth
recording:
- A live reference where I needed a value. moveElement mutates in place and
savedTemplate() hands back those same objects, so the "before" values moved
with the result and the assertion could not fail.
- interact's resizable sets edges from the pointer's ABSOLUTE position, not a
delta, so dragging relative to the handle's centre shrank the element.
- The uuid/id fallback only exists on the ember-data path; a plain object is
cloned wholesale, so plain fixtures could never reach it.
- Selection runs through interact's tap handler, not a bare click(), so the
rotate button stayed disabled.
One assertion is deliberately weaker than it looks: the resize test asserts the
model changed rather than exact arithmetic, because Ember's test container
scales its contents (a 200x40 element measures 100x20) and resizeElement works
from interact's measured rect. Encoding that scale would be precise and brittle.
moveElement works from deltas, so its numbers are asserted exactly.
Twelve documented ignores, each naming what makes the site unreachable:
constructor-pre-empted @Tracked initializers, uuid lookups that can only receive
a rendered element, and the undo/redo guards behind disabled={{not @canundo}}
with no keyboard shortcut.
Also logs DEFECTS #20: attach/popover adds document-level click/touchend and
keydown listeners and never removes them. removeEventListeners() is correct but
its only caller is the first line of initializeAttacher(), which runs once from
{{did-insert}} while the listener maps are still empty — so the removal loops
are dead code and every destroyed popover leaks. Same shape as the former #17,
except the method itself needs no repair, only a destructor to call it. Left
open: it changes teardown on a component used across the app.
Full suite: 5212 pass, 0 fail, 0 skip.
96.48% statements, 93.62% branches, 98.80% functions, 96.61% lines.
roncodes
added a commit
that referenced
this pull request
Aug 25, 2026
The component registers handlers on `document`, not just on its target:
click/touchend for clickout, keydown for escapekey (on by default), and
mousemove for interactive attachments.
removeEventListeners() was already correct — it stores each handler and passes
the stored reference back to removeEventListener, so there is no .bind()
mismatch like the full-calendar leak had. The problem was purely that nothing
called it. Its only caller was the first line of initializeAttacher(), which
runs once from {{did-insert}} while the listener maps are still empty, so it
removed nothing and its loops were dead code.
Every popover that was rendered and destroyed therefore left its document
handlers registered for the lifetime of the page, each still running
hideOnClickOut against a destroyed component. A route that renders many
popovers accumulates them.
Fixed by calling removeEventListeners() from willDestroy(). The docblock records
why the method looked fine while leaking, and notes that useCapture has to match
between add and remove or the removal silently no-ops.
Five tests. The two for the leak assert the observable consequence — dispatch
the click and escape events a leaked handler would answer, after the component
is gone — rather than asserting that a method ran, which would have passed
against the broken version too. The other three cover the isDestroyed guards by
destroying the component mid-delay, which is the teardown race those guards are
for.
The removal loops go from dead code to covered as a consequence, which is the
honest way to close those lines: ignoring them would have documented a bug as if
it were a design.
attach/popover.js: 39 uncovered sites down to 27.
Second leak found this way. Uncovered teardown code has now twice meant a
missing destructor rather than a missing test (full-calendar #17, this one), so
that check is now in the loop brief.
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
1 participant
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.
No description provided.