|
| 1 | +# PR #769 — Status, Risk Analysis, and Test Plan |
| 2 | + |
| 3 | +**Branch:** `flow-real-types` → `main` (rebased onto post-#767 main, now mergeable) |
| 4 | +**Scope:** 167 files changed, 7 commits |
| 5 | + |
| 6 | +--- |
| 7 | + |
| 8 | +## 1. Current state |
| 9 | + |
| 10 | +| Metric | Before (#767 merged) | After #769 | |
| 11 | +|---|---|---| |
| 12 | +| `flow check` errors | 0 | **0** | |
| 13 | +| Suppressions (`$FlowIgnore` / `$FlowFixMe`) | 560 | **18** | |
| 14 | +| Jest | 200 suites / 4,644 | **200 suites / 4,644** | |
| 15 | + |
| 16 | +**97% of remaining Flow suppressions removed.** Of the 18 left, 3 aren't real suppressions at all (two sit inside commented-out code; one is the literal text `$FlowIgnore[prop-missing]` inside a prose comment in `__mocks__/asNPTypes.js`). So **15 live suppressions remain**, each narrowed to a specific error code and carrying a comment explaining why it's still there. |
| 17 | + |
| 18 | +The rebase dropped 6 commits that are now redundant (the multi-level indent fix and its docblock, both shipped in #770; the `insertTodos` revert and its correction, now obsolete; and a `[DIAG]` logging add/remove pair that nets to zero). |
| 19 | + |
| 20 | +--- |
| 21 | + |
| 22 | +## 2. Where there is **zero** risk |
| 23 | + |
| 24 | +This is the majority of the PR, and the claim is mechanical rather than a matter of judgement. |
| 25 | + |
| 26 | +**118 of 167 files (71%) produce byte-identical JavaScript.** |
| 27 | + |
| 28 | +Babel erases Flow types at build time, so a change that only touches annotations *cannot* alter runtime behaviour. `scripts/flow-emit-audit.js` proves this per-file: it runs the Babel transform on each changed file at `main` and again on this branch, and diffs the emitted JS. For 118 files the output is identical byte-for-byte. |
| 29 | + |
| 30 | +**A further 4 files differ only by a bare `;`** — an empty statement, a no-op: |
| 31 | + |
| 32 | +- `helpers/react/reactDev.js` |
| 33 | +- `jgclark.Dashboard/src/react/components/Section/useSectionSortAndFilter.jsx` |
| 34 | +- `np.Shared/src/react/Root.jsx` |
| 35 | +- `np.Templating/__tests__/NPTemplateRunner.test.js` |
| 36 | + |
| 37 | +The repo omits semicolons, so a statement starting with `(` continues the previous line. Inline casts at statement start must be written `;(foo: any).bar()`. That leading `;` survives type erasure as an empty statement. |
| 38 | + |
| 39 | +**So 122 of 167 files carry no behavioural risk whatsoever, and this is verifiable by re-running the audit rather than by trusting review.** |
| 40 | + |
| 41 | +--- |
| 42 | + |
| 43 | +## 3. Where the risk actually is |
| 44 | + |
| 45 | +**45 files, 489 changed lines of emitted JavaScript.** I reviewed **100% of those lines at the emitted-JS level** — not the source diff. That matters: emitted JS has comments and type annotations stripped, so nothing can hide in a plausible-looking comment, and a "type-only" change that isn't shows up immediately. |
| 46 | + |
| 47 | +The 45 files break into three tiers. |
| 48 | + |
| 49 | +### Tier A — Mechanically equivalent refactors (no behaviour change) |
| 50 | + |
| 51 | +Verified equivalent, usually by checking the thing the change depended on rather than assuming: |
| 52 | + |
| 53 | +| File | Change | Why it's safe | |
| 54 | +|---|---|---| |
| 55 | +| `helpers/dev.js` | `timeEnd - timeStart` → `Number(timeEnd) - Number(timeStart)` | `Number()` is exactly what `-` does to each operand. **Important:** `.getTime()` would have been wrong — `helpers/promisePolyfill.js:161,183` pass `Date.now()` (a number) into a param annotated `Date`, so `.getTime()` would throw. | |
| 56 | +| `helpers/note.js`, `helpers/NPnote.js` | 6 sorts → `Number(a) - Number(b)` | Same reasoning. An agent originally wrote `.getTime()` here; I reverted that because it adds a crash path the original `-` never had. | |
| 57 | +| `dwertheimer.EventAutomations/src/config.js` | `validateConfigProperties` now gets a copy, result discarded | Verified it only *reads* `config[v]` — no writes anywhere in the function. | |
| 58 | +| `dbludeau.TodoistNoteplanSync` | setters write `setup.x` not `this.x` | Verified `setup` is never spread or copied, so `this` was always `setup`. | |
| 59 | +| `helpers/urls.js`, `jgclark.SearchExtensions/src/searchTriggers.js` | `while ((m = re.exec(s)))` restructured | Same `exec` call order and count. | |
| 60 | +| `dwertheimer.TaskAutomations/src/NPTaskScanAndProcess.js` | `hasOwnProperty` → truthiness | Values are always non-empty arrays; `[]` is truthy anyway. | |
| 61 | +| `dwertheimer.Forms/src/formSubmission.js` | `for…in` + `hasOwnProperty` → `Object.keys`; `Function.apply` → `new Function` | Identical semantics. | |
| 62 | +| `jgclark.SearchExtensions/src/flexiSearch.js` | prefs wrapped in `String()` | Verified the values are only substituted into template strings, never used in boolean context (`"false"` would be truthy). | |
| 63 | +| `helpers/config.js`, `helpers/NPConfiguration.js`, `helpers/content.js`, `dwertheimer.TaskSorting/src/tagTasks.js`, `np.Templating/lib/support/modules/data/service.js` | local refinements, `const` destructuring, explicit branches | Equivalent including null/array edge cases. | |
| 64 | + |
| 65 | +### Tier B — Real bug fixes (behaviour changes, but the old behaviour was broken) |
| 66 | + |
| 67 | +These *do* change behaviour. In each case the previous behaviour was a crash, an exception swallowed into a misleading message, or a silently wrong value. |
| 68 | + |
| 69 | +**Three functions that did not exist** — confirmed absent by grep, not inferred: |
| 70 | + |
| 71 | +| Call site | Was | Now | |
| 72 | +|---|---|---| |
| 73 | +| `np.Templating/src/Templating.js` | `NPTemplating.getTemplate()` — **threw a TypeError every time**, so meeting-note templating was broken | `getTemplateContent()` | |
| 74 | +| `np.Templating/lib/TemplatingEngine.js` | `this.getTemplateConfig()` — no such method | `this.templateConfig` | |
| 75 | +| `np.Templating/lib/rendering/index.js` | re-exported `renderTemplate` — resolved to `undefined` | `renderTemplateByName` | |
| 76 | + |
| 77 | +**Crashes:** |
| 78 | + |
| 79 | +- `helpers/NPParagraph.js` `paragraphMatches()` called `.startsWith()` on non-string fields (`lineIndex`, `indents`, `date`) → TypeError. Now type-guarded. |
| 80 | +- `np.Tidy/src/tidyMain.js` `removeBlankNotes()` compared against the **string** `'undefined'`, so the guard was always true and a note with undefined content threw mid-filter. |
| 81 | +- `jgclark.Dashboard` move-day/move-week handlers wrote `p.content` then used `p.note`, which really is `?TNote` — a genuine null dereference the suppression was hiding. |
| 82 | +- `np.Templating/lib/support/modules/quote.js` resolved `undefined` out of a `Promise<string>` that callers assign to `const verse: string`. |
| 83 | + |
| 84 | +**Silently wrong values:** |
| 85 | + |
| 86 | +- `jgclark.Dashboard/src/reactMain.js` returned `{}` on error, so every setting read as `undefined`. Now returns `null` and the caller keeps the loaded settings. |
| 87 | +- `helpers/NPWindows.js` `getStoredWindowRect()` returned a malformed preference as a fake `Rect`. |
| 88 | +- `dwertheimer.Forms` `FormView` passed a tuple-taking `sendToPlugin` into context. |
| 89 | +- `np.Templating/lib/support/modules/prompts/handlers/index.js` called `promptDateInterval('', message, default)` against a `(message, defaultValue)` signature — the user saw an empty prompt and the default was dropped. |
| 90 | +- `helpers/NPDateStrings.js` — a `null` week threw, and the outer catch discarded **the entire relative-date list**. Now that one week is logged and skipped. |
| 91 | + |
| 92 | +### Tier C — Judgement calls that need a human decision |
| 93 | + |
| 94 | +**These are the only places where reasonable people could disagree.** Everything else is either provably inert or a fix to something demonstrably broken. |
| 95 | + |
| 96 | +1. **`jgclark.SearchExtensions/src/searchHelpers.js` — `makeAnySyncs()` returns `input` instead of `null` on error.** *(@jgclark's call.)* The old behaviour wasn't a design: `null` escaped a `Promise<resultOutputType>` into `saveSearch()`, which reads `.resultCount` and threw — the whole search visibly failed. Now a failure while adding blockIDs costs only the sync links. The alternative (rethrow, so the user gets an empty result set) is equally defensible. |
| 97 | + |
| 98 | +2. **Heading levels clamped to 1–8** (`helpers/userInput.js`, `helpers/paragraph.js`). A computed level of `0`, `NaN` or `12` previously went raw to `insertHeading()`; it's now rounded and clamped (`NaN` → 2). Only affects out-of-range input. |
| 99 | + |
| 100 | +3. **`dwertheimer.TaskAutomations/src/NPOverdueReact.js` — field whitelist.** `para[field] = row[field]` became explicit `type`/`content` branches; anything else logs an error and is skipped. Verified only those two fields are ever sent, but a third would now be dropped rather than blind-assigned. |
| 101 | + |
| 102 | +4. **`helpers/sorting.js` `fieldSorter` — two non-`Date` objects now compare equal.** I found this during review; the agent's claim that only strings/numbers reach that line was wrong. `isNaN()` coerces, so Dates and booleans still become numbers — only plain objects are affected, and the fields actually sorted are `content`/`index`/`date`/`priority`. Very unlikely to fire, but it *is* a divergence neither Flow nor the tests would catch. |
| 103 | + |
| 104 | +5. **`dwertheimer.EventAutomations/src/events.js` — now skips untitled/undated events.** Worth knowing but **not testable**: its wrappers are exported but appear in neither `index.js` nor `plugin.json`, so the command is unreachable. |
| 105 | + |
| 106 | +--- |
| 107 | + |
| 108 | +## 4. How we know this is safe |
| 109 | + |
| 110 | +Four independent mechanisms, in decreasing order of strength: |
| 111 | + |
| 112 | +1. **Emitted-JS audit (mechanical, re-runnable).** `node scripts/flow-emit-audit.js --base origin/main` — proves 118 files cannot behave differently. This is the single most important artifact in the PR: it converts "trust the review" into "check the tool." |
| 113 | +2. **Full test suite.** 200 suites / 4,644 tests green. Note this is a *floor*, not a ceiling — coverage is thin in exactly the plugin code these changes touch. |
| 114 | +3. **Line-by-line review of the entire behaviour-changing surface.** All 489 emitted lines across 45 files, not a sample. Six changes looked like defects and were chased individually; five turned out fine (and could only be known so by checking), one was a real divergence (Tier C item 4). |
| 115 | +4. **Dead-suppression sweep.** Placeholder substitution — swap each suppression for a same-line-count placeholder so no line numbers shift, run Flow, see which covered lines actually error. Confirms no remaining suppression is inert (an inert suppression is worse than none: it hides future errors on that line). |
| 116 | + |
| 117 | +### Things I got wrong during this work, and how they were caught |
| 118 | + |
| 119 | +Stated plainly, because it calibrates how much to trust the rest: |
| 120 | + |
| 121 | +- I claimed a Babel upgrade would unlock blocked syntax. **Wrong** — tested 4 versions, all fail. Corrected with a tested matrix. |
| 122 | +- An agent's `chroma-js` change traded 3 errors for 5. Caught by re-measuring; reverted. |
| 123 | +- A `checkType.js` mapped-type rewrite turned 19 suites red — Babel can't parse mapped types at any version. Reverted. |
| 124 | +- Three incidental behaviour changes slipped into a "type-only" commit. Caught **by my own emit audit**, not by review. |
| 125 | +- I twice attributed observed indent-flattening to a code path that never executed, and wrote the false conclusion into a commit message. Caught by @dwertheimer's logs; the fix that survived is the one he insisted on re-applying. |
| 126 | + |
| 127 | +The pattern: **the mechanical checks caught what review missed.** That's why the audit matters more than my summary. |
| 128 | + |
| 129 | +--- |
| 130 | + |
| 131 | +## 5. What a human should test before merging |
| 132 | + |
| 133 | +Ordered by value. Items 1–2 are the ones I'd actually insist on. |
| 134 | + |
| 135 | +### 1. Meeting notes (np.Templating) — **highest value** |
| 136 | +`Create Meeting Note using Meeting Note Template`. |
| 137 | +This path **threw a TypeError on every invocation** before this PR (`NPTemplating.getTemplate` doesn't exist). Expect it to work now. If it still fails, the failure is downstream of what was fixed. |
| 138 | + |
| 139 | +### 2. Dashboard settings survive a perspective switch |
| 140 | +`Show Dashboard`, then switch perspectives several times including back to default. |
| 141 | +✅ Settings persist. ❌ Settings blank out, or the window fails to open. Check the log for `carry on with the existing settings` or `Couldn't assemble the initial data`. |
| 142 | +*Why:* `reactMain` used to return `{}` on error, silently replacing every setting with `undefined`. |
| 143 | + |
| 144 | +### 3. Search with sync'd results (jgclark) |
| 145 | +Settings: result style **NotePlan**, `syncOpenResultItems` **on**. Run `search` for a term matching several open tasks. |
| 146 | +✅ Results carry sync IDs (`^abc123`) exactly as before. The happy path must be untouched — that's the main thing to confirm; the changed path only fires on an error that already broke the search. |
| 147 | + |
| 148 | +### 4. Overdue review edits |
| 149 | +`Review overdue tasks (by Task)`. Edit a task's text; complete/cancel a task. |
| 150 | +✅ Both work. Check log for `unsupported field` — its absence confirms only `type`/`content` are ever sent. |
| 151 | + |
| 152 | +### 5. Relative-date lists |
| 153 | +Any chooser showing relative dates (e.g. `Show Dashboard`, or a template using them). |
| 154 | +✅ `this week`, `last week`, `next week`, `2 weeks ago` … `10 weeks' time` all present. ❌ Whole categories missing → check log for `returned null for week offset`. |
| 155 | + |
| 156 | +### 6. Window position restore |
| 157 | +Open a plugin window, move/resize, close, reopen. ✅ Restored. If the log shows `isn't a valid Rect`, you had a corrupt stored pref — the intended fix, but worth knowing it fired. |
| 158 | + |
| 159 | +### 7. Rename a note |
| 160 | +`rename note` on a normal note ✅; on a title-less note ✅ stops cleanly rather than offering `"undefined"`. |
| 161 | + |
| 162 | +**Not testable — dead code, no action needed:** `events.js` (unreachable — not in `index.js` or `plugin.json`), `TemplatingEngine.getDefaultFormat()` (marked "should never be called"), the `renderTemplate` barrel re-export (nothing imports it). |
| 163 | + |
| 164 | +--- |
| 165 | + |
| 166 | +## 6. Known pre-existing bugs found but deliberately NOT fixed |
| 167 | + |
| 168 | +Listed so they aren't mistaken for regressions during testing: |
| 169 | + |
| 170 | +- **`helpers/calendar.js` `keepTodayPortionOnly()`** claims `Array<TCalendarItem>` but returns hand-built plain objects with no `id`, no `isRecurring`, no methods. Any caller reading `.id` or calling `.findLinkedFilenames()` on a multi-day event gets `undefined` or a crash. Four files across three plugins consume it, and `helpers/__tests__/calendar.test.js` asserts `toEqual()` on the plain-object shape — so the test currently locks in the wrong contract. |
| 171 | +- **`helpers/NPnote.js` `chooseNoteV2()`** can return a fake note: pick a future calendar date with no note yet and you get a stub with only `title`/`type`/`changedDate`. |
| 172 | +- **`helpers/urls.js` `findProjectNoteUrlInText()`** — the `allowCalendarNotes` parameter is dead; the filtered result is computed then discarded. |
| 173 | +- **`jgclark.SearchExtensions/flexiSearch.js`** — `String(x) ?? 'quick'` dead defaults: `String()` never returns null, so unset preferences become the literal string `"undefined"`. |
| 174 | +- **`dbludeau.TodoistNoteplanSync`** — `teamAccount` is both a data property and a setter; the setter wins, so reads return `undefined`. Appears to be dead code. |
| 175 | +- **`helpers/sorting.js` `fieldSorter`** — declared `(a: string, b: string)` but always called with objects; also compares against the *string* `'NaN'`. |
| 176 | + |
| 177 | +--- |
| 178 | + |
| 179 | +## 7. Separate finding: NotePlan API bug (not ours, not fixable by plugins) |
| 180 | + |
| 181 | +**Space-indentation is invisible to plugins.** Verified against a note whose bytes on disk were confirmed: |
| 182 | + |
| 183 | +| Line | On disk | API `indents` | API `rawContent` | |
| 184 | +|---|---|---|---| |
| 185 | +| tab child | 1 tab | **1** ✓ | `"\t* A child ONE-TAB"` ✓ | |
| 186 | +| tab grandchild | 2 tabs | **2** ✓ | tabs preserved ✓ | |
| 187 | +| 2-space child | 2 spaces | **0** ✗ | spaces stripped | |
| 188 | +| 8-space grandchild | 8 spaces | **0** ✗ | spaces stripped | |
| 189 | + |
| 190 | +`rawContent` is reconstructed from marker + `indents` rather than being the literal file bytes (a `- [ ]` on disk comes back as `* `), so once the parser declines to count leading spaces the indentation is gone from *both* fields before any plugin runs. |
| 191 | + |
| 192 | +**The consequence is data loss, not display:** any plugin that deletes and re-inserts paragraphs writes back the stripped version and permanently flattens a space-indented file. Repro note is `DELETEME/indent-api-test.md`; section D shows both behaviours under a single parent. Needs raising with @EduardMe. |
0 commit comments