mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-30 03:00:57 +00:00
21556 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Johannes Millan
|
7291f2fdc8
|
fix(backup): show the Windows Store backup path only when it exists (#9218)
* docs(sync): clarify multi-device token usage * fix(backup): show Windows Store backup path only if it exists Settings showed the MSIX-virtualized path unconditionally under process.windowsStore, without ever checking it is there. On installs where the package is not virtualized the backups sit in the real AppData\Roaming, so the settings page advertised a folder that does not exist and rendered a dead file:// link - for the backup feature, whose whole point is being findable after a disaster (#9209). AppData redirection is not a constant: it applies only to virtualized packages, and since Windows 10 1903 the OS resolves it per file. That is why #995 and #9209 report the exact opposite symptom and both are right. Probe for the directory instead of assuming, and fall back to the path we actually write to, which is correct in either case. Closes #9209 * fix(backup): pin the Windows Store backup path with a regression test Follow-up to the previous commit, addressing multi-agent review. Move the display-path decision next to the constants it reads, as getBackupDirForDisplay(). BACKUP_DIR_WINSTORE stops being exported, so the rule "verify before displaying" is enforced by the only accessor instead of being an honour-system note in a doc comment - which is the shape the 2025 fix got wrong. backup.ts already imports existsSync, so this costs no new import and matches getRelaunchExecPath in ipc-handlers/app-control.ts. Correct the doc comment: it claimed BACKUP_DIR_WINSTORE is never read, while BACKUP_LOAD_DATA accepts it as an allow-listed read root fifty lines below. Left as-is otherwise; narrowing a GHSA-x937-wf3j-88q3 guard does not belong in a display-path fix. Add electron/backup.test.cjs pinning both directions of the loop this bug has already run once: Store + LocalCache present must show it (#995), Store + LocalCache absent must fall back (#9209). Verified by sabotage - restoring the unconditional path fails the #9209 case, deleting the probe fails the #995 case, and each kills only its own test. Document the two accepted shortcuts with their ceiling and upgrade path: the hardcoded package family name (unverifiable from source, degrades gracefully), and the stale LocalCache dir left by a virtualized to full-trust flip. * docs(backup): record how the hardcoded package family name was verified Computed it from the shipped v18.15.1 AppxManifest: PublisherId is the first 8 bytes of SHA-256 over the UTF-16LE Publisher string in base32 (digits + a-z minus i/l/o/u). CN=AC30A249-AFE7-4B23-AE54-A95B4FDF8928 yields ch45amy23cdv6, matching the constant exactly - so it has not drifted, and the previous comment's claim that it cannot be verified was too pessimistic: any release artifact re-checks it without Windows. * docs: trim backup path comments and document the electron test harness Cut the duplicated MSIX explanation across the two doc blocks (-7 lines) while keeping the parts that stop this bug round-tripping a third time: the per-file 1903 semantics, the #995/#9209 link, and both shortcuts' ceilings. Add npm run test:electron to AGENTS.md. It was absent, and the harness is easy to miss because it uses electron/*.test.cjs while the rest of the repo uses .spec.ts - which tsconfig.electron.json actively excludes, so a spec dropped there silently never runs. Searching for the repo's dominant convention turns up nothing and invites the conclusion that main-process code is untestable; it is not. |
||
|
Johannes Millan
|
e581fafefb
|
test(tasks): create the notes-focus fixture inside fakeAsync (#9219)
The suite runs zoneless (src/test.ts provides provideZonelessChangeDetection
globally) and Angular's ComponentFixture.autoDetectDefault is `true` whenever
zoneless is enabled, so createComponent()/setInput() schedule an
ApplicationRef tick. Run from the async beforeEach, that tick can fire before
the spec body and execute ngAfterViewInit outside the fake zone: its delay(50)
then registers a REAL timer that tick(100) can never flush, and the focusItem
spy is installed too late to observe the call. CI hit this as an intermittent
"focusItem ... was never called" on the America/Los_Angeles run.
It also explains why the earlier tick(50) -> tick(100) hardening (
|
||
Kamil
|
c385477f93
|
fix(tools): make checkFile work on Windows (#9132)
tools/check-file.js spawned npm/npx via execFileSync, which fails on Windows: npm and npx are .cmd shims there and Node refuses to spawn .cmd without a shell since the CVE-2024-27980 fix, so every `npm run checkFile <file>` died before running prettier or lint. Invoke each tool's JS entry point with process.execPath instead - no npm/npx shims and no shell on any platform, so argv stays literal. Also fixes invocation from a subdirectory (npm run resolved package.json from cwd) and surfaces prettier diagnostics that the npm banner previously swallowed. |
||
giri256
|
e1d15212ff
|
test: reset global dialog state between specs #9100 (#9183)
Co-authored-by: p1 <giridharpavan593@gmail.com> |
||
Sai Asish Y
|
b26fb1093f
|
fix(markdown): select exactly the url when wrapping selected text in a link (#9208)
Signed-off-by: Sai Asish Y <say.apm35@gmail.com> |
||
|
Kamil
|
775d202fd4
|
test: make date-based specs timezone-independent (#9131)
create-schedule-days.spec derived its day strings via toISOString() (UTC) while createScheduleDays buckets days by local midnight; plugin-i18n-date.util.spec pinned a UTC instant while formatDateForPlugin formats in the local timezone. In timezones far enough ahead of UTC (e.g. Australia/Sydney, UTC+11), the derived dates shift back a day and 8 specs fail — one of them crashing in getTimeLeftForTask because the shifted week boundary lets a raw task literal without subTaskIds reach the budget calculation (`npm run test:tz:sydney` reproduces this on any OS). Derive day strings with getDbDateStr (local), construct the plugin spec's test date in local time, and build the raw task literal with createTestTask. The derived values are unchanged in the timezones CI runs (Europe/Berlin, America/Los_Angeles, Asia/Tokyo, UTC). This also fixes clean-master unit tests for Windows contributors in such timezones: Chromium on Windows ignores the TZ env var and uses the OS timezone, so the cross-env TZ pinning in the test scripts never reaches the Karma browser and the suite runs in the machine's local timezone. |
||
|
Johannes Millan
|
2a001e2e9b
|
test(sync): cover production batch conflict queries (#9211)
* test(sync): cover production batch conflict queries Execute the real Prisma SQL templates in PGlite and pin the conflict-policy and chunk-boundary behavior exposed by mutation testing.\n\nRefs #9205 * test(sync): strengthen batch boundary guards * test(sections): wait for menu animation before click Prevent the Add Section item from moving between mouse down and mouse up under CI load. |
||
|
Johannes Millan
|
8f5296d7c9
|
perf(supersync): disable GIN fastupdate for conflict lookups (#9213)
Prevent the entity_ids GIN pending list from adding hundreds of milliseconds to conflict probes. Bound the index lock and cleanup, and retry the exact failed migration natively without marking it applied out of band. Refs #9204 |
||
|
Johannes Millan
|
03e90c8948
|
test(supersync): cover real-prisma array winner (#9210)
Exercise the positive entity_ids result and compound key fetch against real Prisma and PostgreSQL. |
||
|
Johannes Millan
|
77557077f8
|
fix(supersync): split the conflict entity lookup to avoid a full-history scan (#9195)
* fix(supersync): split conflict entity lookup to avoid full-history scan
detectConflictForEntity ran one Prisma filter with
`OR: [{ entityId }, { entityIds: { has: entityId } }]` and
`orderBy: { serverSeq: 'desc' }`. The OR spans the
(user_id, entity_type, entity_id, server_seq) btree and the entity_ids
GIN index, and GIN cannot supply server_seq ordering — so with LIMIT 1
the planner abandoned both index paths and walked server_seq backwards
applying the OR as a filter, betting it would exit early. For an entity
with no matching rows (the first op for a new task, the most common
upload there is) that bet loses and it scans the user's whole history.
The function runs twice per operation on the default serial upload path,
so every upload paid it twice.
Production incident: 47 backends stuck on this query, longest 75 minutes,
wait_event=DataFileRead, 61 of 66 connections consumed. Prisma pool
exhaustion made every user's upload AND download fail with "Unable to
start a transaction in the given time". No index was missing or invalid.
Split into separately-indexed lookups:
- scalar branch: findFirst on {userId, entityType, entityId} — the btree
covers all equality columns plus the sort column
- array branch: aggregate({_max: {serverSeq}}) — Prisma's OFFSET 0
subquery is a planner fence that forbids the LIMIT-driven walk and
forces the GIN bitmap path
- winner fetch: findUnique on the (userId, serverSeq) unique key, only
when the array max beats the scalar
Measured on production Postgres (88k-op user, zero-match probe) after an
ANALYZE and a GIN pending-list flush: old shape 12 buffers, new shape 9
(scalar 4 + array 5). In that regime the two are comparable — the value
of this split is NOT throughput. It is that neither branch offers the
planner a LIMIT-driven early-exit bet, so it cannot degenerate into the
backward walk regardless of statistics. Even post-ANALYZE the planner
still estimates rows=96 for a zero-match `entity_ids @>` key, so the old
shape stays one autovacuum re-sample away from the incident plan.
Wall-clock is not quoted: on a live server the same old-shape query
measured 255ms and 915ms on consecutive runs while its buffer count
fell. Buffers are the stable metric here.
Semantics are unchanged: both forms return the highest-serverSeq row
across the union of the two predicates. Ties need no tie-break because
@@unique([userId, serverSeq]) makes an equal serverSeq the same row.
The legacy GLOBAL_CONFIG:misc alias path is untouched.
The batch paths (detectConflictForEntities, prefetchLatestEntityOpsForBatch)
are not exposed to this trap — no LIMIT, and DISTINCT ON forces full
evaluation — and are left unchanged.
* fix(supersync): force the GIN path with a MATERIALIZED CTE
The previous commit's array branch used `aggregate({_max: {serverSeq}})`,
relying on Prisma's `OFFSET 0` subquery as a planner fence. Production
EXPLAIN under `SET plan_cache_mode = force_generic_plan` shows that does
not work:
Aggregate
-> Index Scan using operations_user_id_entity_type_entity_id_server_seq_idx
Index Cond: ((user_id = $1) AND (entity_type = $2))
Filter: (entity_ids @> ARRAY[$3])
It scans the whole (user_id, entity_type) slice — ~88k rows for a
deep-history user. Dropping LIMIT removed the early-exit bet but did not
force the GIN path: the planner costs btree+filter at ~80 against ~875
for the GIN bitmap and takes the btree.
The root reason both earlier designs were misjudged: Prisma sends
parameterized prepared statements, so after ~5 executions Postgres builds
a GENERIC plan that cannot see parameter values. Every EXPLAIN run with
literal constants measures a custom plan production never receives. Two
designs looked correct that way and were catastrophic in production.
Array branch is now raw SQL:
WITH cand AS MATERIALIZED (
SELECT user_id, entity_type, server_seq FROM operations
WHERE entity_ids @> ARRAY[$1]::text[]
)
SELECT MAX(server_seq)::int AS "maxSeq" FROM cand
WHERE user_id = $2 AND entity_type = $3
Inside the CTE the only predicate is `entity_ids @>`, so the composite
btree has no usable leading column and GIN is the only option — the plan
is forced structurally rather than won on cost, which is what makes it
survive statistics drift. MATERIALIZED prevents the outer predicates
being pushed down, which would hand the btree back.
Adding `server_seq > <scalar>` to bound the read was evaluated and
rejected: under generic planning the value is invisible, so it lands as a
post-GIN filter and buys nothing, and placing it inside the CTE creates a
new cliff (no leading-column anchor, so cost scales with total table size
rather than the probing user's history).
Semantics unchanged: verified by differential testing old-vs-new across
20,000 randomized in-memory histories and 1,500 against real Postgres,
zero divergences, with 18 sabotage mutations confirming the harness can
fail.
Tests: mocks moved from `aggregate` to `$queryRaw` behind a shared
discriminator; all plan assertions now run under force_generic_plan; the
plan spec's block accounting no longer double-counts (Postgres buffer
counts are cumulative, so parents already include children).
* test(supersync): make the conflict-lookup plan spec able to fail
The plan spec could not catch the outage it was written for. Three
independent problems, each of which made a load-bearing property
invisible:
1. The spec seeded ONE user and ONE entity_type. In that shape the GIN
estimate (DEFAULT_CONTAIN_SEL * whole table) and the btree-slice
estimate (N / users*entity_types) are the same rows, so GIN always
wins on cost and no regression is detectable. Reseeded to 20k rows for
the probed user plus 20k across ~20k other users over 8 entity types,
which reproduces production's plan node-for-node. Seeds in ~490ms.
2. The measuring shim rebuilt the array-branch SQL from a CONSTANT, so
changing MAX(server_seq) to MIN in conflict.ts could never be
observed. It now reconstructs the SQL from the real tagged template,
putting the aggregate, the MATERIALIZED fence and the CTE shape
genuinely under test.
3. The shim ignored `select`, so dropping actionType from the array
branch was invisible — that field decides whether concurrent
time-tracking deltas merge or get rejected. It now honors `select` and
throws on an unmapped key.
Before this, all three of these mutations left the suite green:
MAX->MIN (silently accepts a write that should conflict, overwriting a
concurrent remote edit), dropping actionType (rejects a client's tracked
time), and hardcoding userId in the findUnique.
Measured after reseeding: shipped CTE 2 blocks / 0 rows filtered; the old
OR, the flat MAX, Prisma's aggregate form, and MATERIALIZED-dropped all
land at 806 blocks / 2500 filtered. One budget assertion (<100 blocks)
now catches every known route back to the incident, with a 403x margin.
All plan assertions run under force_generic_plan; plan-NAME assertions
were removed after measurement showed the old OR no longer plans as a
Backward walk at this seed.
Also corrects a false claim this change introduced. The comment justified
the cross-tenant CTE scan with "entity ids are client-generated nanoids,
so overlap is not expected". The client hard-codes globally shared ids
(TODAY, EM_URGENT, EM_IMPORTANT, KANBAN_IN_PROGRESS, INBOX_PROJECT,
EISENHOWER_MATRIX, KANBAN_DEFAULT, plus fixed GLOBAL_CONFIG keys) which
are byte-identical for every user, and updateBoard({id:'KANBAN_DEFAULT'})
routes here as a single-entity op. Measured: 3885 blocks to reach the 10
rows belonging to the probing user, versus 4 for a unique id. Correctness
is preserved by the outer WHERE user_id; cost is not bounded per-user.
The fix is a btree_gin composite index, which needs a real-Postgres
EXPLAIN (PGlite lacks the extension) and is filed separately.
Deletions: the entity-ids-conflict helper that hand-wrote SQL production
never sends and modelled the abandoned aggregate design (-71 lines), a
dead aggregate mock that would have MASKED a regression by returning an
undefined max, the server_seq-bound test that asserted a rejected design
still works (so it could never fail informatively), and four stale
comments describing a call that no longer exists.
sync-operations.spec.ts had a $queryRaw mock never wired to the array
branch; it fell through to a vestigial value that reads as null, so every
conflict assertion there ran with the array branch stubbed out. Wired up,
and all six raw-query sites now throw on an unrecognised query rather
than falling through.
* test(supersync): seed the conflict-lookup plan spec in production order
Building the GIN after the bulk load produced a pending-list-free index and
measured 2 blocks for the array branch - a state production only sees right
after a vacuum. With the index pre-existing and rows arriving one at a time,
as in production, the same query reads 140 blocks against a 14x larger index.
Seed in production order and never vacuum, so the spec measures the worst
realistic steady state, and re-derive the budget (100 -> 300) against it.
Also move the GIN-not-btree structural assertions onto the real tagged
template, and replace the five hardcoded regression shapes with a single
canary whose stated job is proving the seed still reproduces the mis-plan.
The five EXPLAINed strings that exist only in the test file and responded to
no source mutation; the budget and plan assertions on the real SQL do.
* docs(supersync): correct two false claims in the conflict-lookup comment
The cross-tenant scan vector was wrong. updateTag({id:'TODAY'}) and
updateBoard({id:'KANBAN_DEFAULT'}) are single-entity, so getStoredEntityIds
persists '{}' and they never enter the GIN under that id. The actual vector is
the bulk sortBoards action, which stores the shared board ids in entity_ids.
'Needs btree_gin, unverifiable in PGlite' was also wrong: a GIN index on
(ARRAY['u:'||user_id] || entity_ids) has a text[] operand served by the
built-in array_ops, builds in PGlite with no extension, and was measured
flat against a baseline that scales with tenant count. Recorded as a
tradeoff rather than a free win - it needs the predicate rewritten to match
and indexes every row rather than the multi-entity minority.
Drop the force_generic_plan methodology lecture duplicated from the spec
header, keeping a pointer.
Also make the #8334 mock honest: StoredRow now requires actionType and
findUnique honours select, so a dropped column changes what it returns
instead of silently yielding undefined.
* test(supersync): cover the scalar branch picking the newest op
Flipping detectConflictForEntity's scalar orderBy from 'desc' to 'asc' passed
the entire 914-test server suite. Every existing case gave an entity at most
one scalar row, so the ordering was unobservable.
The bug it hides is silent data loss: with the oldest row returned, conflict
detection compares the incoming clock against a stale one, so an op that is a
clean successor of the OLD state but CONCURRENT with the current one is
accepted and overwrites a remote edit.
Seed two scalar rows whose verdicts differ, and verify by mutation that the
test is red under 'asc' and green under 'desc'.
* docs(supersync): stop the vector-clock doc recommending the outage query
vector-clocks.md still described detectConflictForEntity as the combined
OR + orderBy filter and vouched for it - 'a BitmapOr + sort bounded by the
entity's stored version depth (op-log pruning keeps that small)'. That is the
exact false assumption behind the 2026-07-20 outage, in the authoritative doc.
Worse, the escalation it recommended (two ordered LIMIT 1 lookups) is itself a
measured-catastrophic shape: the array side still cannot order on GIN.
Also fix a test that could not fail. 'does not alias a POST-split misc write
onto tasks' probed 'tasks-v2-only', but the legacy-misc branch is entered only
for entityId === 'tasks', so it never reached the gate it claimed to guard -
breaking the gate to 'lte' left all 915 tests green. It now probes 'tasks'
under its own tenant, and is verified red under that mutation.
Correct three overclaims in the comments: production does get custom plans for
the first ~5 executions before settling generic; a partial expression GIN can
skip the single-entity majority; fastupdate=off stops new pending entries but
does not flush existing ones, and 140 blocks is not a ceiling.
* docs(supersync): pin the two fail-open invariants in the conflict lookup
Both are unreachable today and both fail toward ACCEPTING a conflicting
write if a future edit breaks them, which is silent data loss rather than
an error. No runtime guards - they could never fire, and the sync layer does
not need more unreproducible hardening. Documented at the exact spot an
editor would stand instead.
- The aggregate fold is safe only because a bare aggregate returns exactly
one row; a GROUP BY (or a revert to Prisma aggregate()) makes zero rows
possible, which reads as 'no prior op'.
- 'arrayOp ?? scalarOp' conflates 'array branch lost' with 'array row
vanished'. The second needs RepeatableRead to stay unreachable.
* docs(supersync): stop overclaiming GIN is forced; fix the batch-lookup doc
Three misleading safety claims, all in text a future contributor would rely on:
- vector-clocks.md still documented the batch lookup as the mutually exclusive
'CASE WHEN cardinality(entity_ids) > 0 THEN entity_ids ELSE ARRAY[entity_id]'.
Production unions the two columns instead, and the exclusive form is the
#8334 bug: it drops a scalar that is not a member of its own entity_ids, so
a later concurrent write to that entity is wrongly accepted - silent data
loss. The section above it was rewritten last commit; this one was missed.
- 'The CTE wins STRUCTURALLY, not on cost' overstated. Removing the competing
btree is structural; GIN being chosen is not. A sequential scan is always
available and wins for an unselective id - reproducible on PG16, and exactly
what a globally shared entity id produces. Reframed as a measured outcome,
since a confident comment asserting what the planner will do is what
preceded the outage.
- The plan spec still said production receives 'a CUSTOM plan nobody
receives'. It receives custom plans for roughly the first 5 executions
before settling generic, and this file covers only the generic mode.
* docs(supersync): correct the plan-cache and aggregate claims; match Int in tests
Round-4 review. Four inaccuracies, all in text a contributor would rely on.
- plan_cache_mode=auto does NOT unconditionally switch to generic after 5
executions: it compares the generic cost against the average custom cost
and may keep using custom plans indefinitely. Stated as an observation for
THIS statement (custom_plans=5, generic_plans=15 on production) rather than
a universal rule, in conflict.ts, the plan spec and vector-clocks.md.
Note: commit
|
||
David Vornholt
|
87b0742183
|
fix(metrics): remove productivity score drop when focus passes target (#9177)
* fix(metrics): remove productivity score drop when focus passes target The focus-progress soft-cap clamped its input to [0,1], so any focus beyond the target collapsed to a constant 0.889 credit: with impact 3, 240 min of focus scored 81 while 241 min scored 77, and the score never recovered no matter how much more focus was logged. Focus progress is now linear to the target and flat beyond it - continuous and non-decreasing - leaving overwork penalties to the sustainability score. Also removes the dead workloadLinearZeroAt parameter from calculateSustainabilityScore (ignored since the sigmoid rewrite) and its misleading call-site comments, renames DEFAULT_WORKLOAD_LINEAR_ZERO_AT to DEFAULT_MAX_WORK_MINUTES to match what it does, and fixes stale JSDoc (removed exhaustion param, outdated example values). * fix(metrics): preserve impact mismatch classification * docs(metrics): clarify productivity focus credit |
||
Syed Osama Ali Shah
|
03a326a042
|
Fix "0 years ago" for timestamps 330-364 days in the past (#9178)
humanizeTimestamp derives diffMonths and diffYears independently:
const diffMonths = Math.floor(diffDays / 30);
const diffYears = Math.floor(diffDays / 365);
For a past timestamp 330-364 days old, diffMonths is >= 11 (so the MONTHS
branch is skipped) but diffYears is still 0 (so the `diffYears === 1`
A_YEAR branch is skipped too), and it falls through to the YEARS branch with
count: 0 — rendering "0 years ago".
The 365-day sibling case already returns A_YEAR ("a year ago"), and the
future branch is coincidentally correct because Math.floor rounds the
negative diff toward -Infinity (so ~11 future months floor to -1 → abs 1).
Only the past branch, which floors ~0.9 to 0, is wrong. Widen the year check
to `diffYears <= 1` so 330-729 days map to "a year ago" and >= 730 days to
"N years ago". This only adds the broken 330-364 window; the 365-729 range
already returned A_YEAR, so there is no behavior change there.
|
||
Loi Nguyen
|
43246e45cd
|
fix(ios): decode status-zero audio assets #8880 (#9175)
Co-authored-by: Loi Nguyen <loinguyen@local> |
||
Matt Van Horn
|
abe9e8695b
|
fix(scheduled-list): parse 'Last' repeat date as local day to fix off-by-one #9127 (#9159)
The Upcoming/Scheduled page tooltip for an "Every x days" repeat rendered its
'Last' date from a YYYY-MM-DD string via formatMonthDay(new Date(str), locale).
new Date('2025-08-01') parses a date-only string as UTC midnight, which
formatMonthDay then renders in the system timezone. For any user west of UTC
(e.g. America/Los_Angeles) that instant falls on the previous local day, so
"8/1" renders as "7/31" - the reported, persistent off-by-one. The 'Next' value
is unaffected because it comes from a real epoch timestamp, not a date string.
Parse the day string with the codebase's canonical dateStrToUtcDate helper,
which builds a local start-of-day Date (already used elsewhere for exactly this).
To keep the fix unit-testable without a component TestBed, the tooltip-building
logic is extracted into a pure get-repeat-cfg-tooltip-text.util.ts (formats the
already-computed next timestamp unchanged and the last-day string via
dateStrToUtcDate), and getTooltipText() delegates to it.
Fixes #9127
Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
|
||
|
Loi Nguyen
|
70ffc348bb
|
docs(wiki): document F11 fullscreen shortcut (#9179)
Co-authored-by: Loi Nguyen <1948922+lntutor@users.noreply.github.com> |
||
|
Johannes Millan
|
66b760a8cb
|
test(planner): use distinct dates in subtask visibility spec (#9193) | ||
|
Johannes Millan
|
88b5055504
|
fix(theme): tolerate work contexts persisted without a theme (#9139) (#9145)
* fix(theme): tolerate work contexts persisted without a theme (#9139) A tag/project entity can be persisted with no `theme` at all. Validation at hydration is non-fatal, so such a snapshot loads anyway and every consumer then dereferences `undefined`, crashing the theme pipeline on every launch. Reproduced end-to-end: resolveBackground() throws on backgroundImageDark/Light and _setColorTheme() on isAutoContrast. Two layers: - resolveContextTheme() extracted from the currentTheme$ map, defaulting a missing theme to WORK_CONTEXT_DEFAULT_THEME. Single choke point for both crashing consumers. This also fixes the tag-colour branch, which spread an undefined theme into a partial `{ primary }` object. - an auto-fix branch backfilling a missing tag/project theme, so data already corrupted on disk is healed rather than merely tolerated. Unrepaired, the same corruption also dead-ended legacy migration with "Migration failed" and no way in. The auto-fix branch matches on path + `value === undefined` and deliberately NOT on `error.expected`: typia reports this as the generated name `Readonly<__type>.oNN`, whose ordinal shifts with the type graph, so an expected-based guard would silently stop firing. Verified: both fixes sabotage-tested (3 failures each with the fix reverted), full suite green in both TZ variants, and confirmed firing against real typia output and real hydration in the browser. * fix(theme): address review findings on the #9139 theme fix Follow-up to |
||
|
Johannes Millan
|
a9932f9f71
|
fix(theme): polish, atomic activation, and validator hardening (#9157)
* docs(agents): require reviews to ask if a feature earns its place * build: add output folder to gitignore because of playwright writing there * style(themes): polish built-in visual states * fix(theme): make stylesheet activation atomic * fix(theme): harden custom theme contracts * fix(theme): reject unterminated url() and exempt commented keywords Two validator correctness fixes found in review of the theme-polish branch: - Reject any url()/src() that runs to end-of-input without a closing ')'. The CSS tokenizer still emits a fetchable url-token at EOF (Syntax §4.3.6), so a theme file ending mid-url (e.g. `background:url(http://x`) beaconed on every load — the argument regex requires the ')' and missed it. Proven to fetch in Chromium. - Move the @import / image() / image-set() keyword-presence bans onto the comment-stripped view. Scanning the unstripped source rejected benign themes that merely mention these tokens in a comment; because stored themes are re-validated on every load, such a theme silently reverted to Default after updating. url()/src() stay on the unstripped view so a disguised token can never hide a later live fetch. Docs and specs updated (+5 cases: EOF-fetch payloads, line reporting, comment exemption, and real-rule-next-to-comment still rejects). * fix(theme): restore plainspace board-panel header underline The token-model pass removed the data:-URL `--underline-hand` mask and migrated its consumers to `clip-path: var(--underline-hand-shape)`, but the board-panel `header::after` consumer was missed. Its `mask: var(--underline- hand)` then resolved to an undefined variable, collapsing the hand-drawn ribbon to a solid 10px bar. Convert it to the shared clip-path polygon like the other two consumers; the percentage-based polygon scales to the box. |
||
|
Johannes Millan
|
b64f398975
|
fix(sync): recover from migration-path hydration failures via op-log replay (#9153)
* docs(agents): require reviews to ask if a feature earns its place * build: add output folder to gitignore because of playwright writing there * fix(sync): recover from migration-path hydration failures via op-log replay A throw on the snapshot migration path (metadata-validation failure, migration transform failure) previously escalated into attemptRecovery(), which refuses while a snapshot exists on disk — bricking the app to an empty store with the HYDRATION_FAILED snack on every boot, even though the op-log could rebuild the state (#9140). The same applied when a feature reducer threw on the snapshot loadAllData dispatch of non-fatally-validated migrated state (the residual class #9124's non-fatal design created). - Catch migrateSnapshotWithBackup throws: when the op-log has replayable rows, discard the unmigratable snapshot for the boot and fall into the existing replay-from-scratch path (#7892 fallback); a successful replay persists a fresh CURRENT_SCHEMA_VERSION snapshot, breaking the re-migrate loop. Empty op-log and IndexedDBOpenError keep the existing terminal handling. - Guard the snapshot loadAllData dispatch with a collector-scoped meta-reducer (mirrors the bulk-replay failure collector): a reducer throw is reported instead of erroring the NgRx state observable (which would silently drop all later dispatches), and the hydrator falls back to the same op-log replay. Pure pass-through outside hydration. - Extract _replayTailOps/_replayAllOpsFromScratch so both fallback call sites reuse the pre-dispatch replay path (replay-from-0 must never run on top of committed snapshot state — double-apply hazard). * fix(sync): harden #9140 fallback against data loss found in multi-review Multi-agent review of the previous commit surfaced three verified defects in the fallback design; all are fixed here with discriminating tests: - Never persist the fallback replay (and suppress the convergence save): for a synced client the surviving op-log is only a compaction-window tail and cursor-based sync never re-sends pruned ops, so caching the partial replay would silently overwrite the intact on-disk snapshot — the last complete local copy. Recovery now re-runs each boot (visible via a new HYDRATION_FALLBACK_RECOVERY snack) until a fixed build migrates the intact snapshot. - Reject the fallback when the store already holds meaningful data: hydrateStore() re-enters on a LIVE store via PluginAPI.reInitData(), and replay-from-0 on top would double-apply non-idempotent reducers. - Rethrow instead of booting silently empty when rows exist but every one is reducer-rejected (the lastSeq>0 gate is only a pre-filter). Also from review: corrected the guard/hydrator comments — a reducer throw never propagates through dispatch(); rxjs diverts it to an async unhandled-error report and silently tears down the state subscription (store freezes). Proven by a new real-store integration spec that also pins the guarded path end-to-end through META_REDUCERS. Registry spec now pins the guard before Phase 3; registry header documents Phase 2.5. Dropped the dead vectorClockService injection to stay under the 1200-line service cap (now exactly 1200 — the near-duplicate _replayTailOps/_replayAllOpsFromScratch pair is the follow-up to win back headroom). * fix(sync): block compaction during hydration fallback; add recovery hint Closes the two follow-ups from the #9140 review round: - Compaction (incl. emergency) now skips while the session booted via the hydration fallback: the live state may be partial (rebuilt from the surviving op tail) while the intact-but-unhydratable snapshot is still on disk — compacting would overwrite that last complete local copy AND prune the ops the next boot's recovery replays. Tracked via a session-scoped flag on HydrationStateService, set/cleared at the end of every hydrateStore() run so a later clean re-hydration (plugin reInit after a sync import) re-enables pruning. - The terminal HYDRATION_FAILED snack now tells users how to actually recover (sync / backup import) instead of only suggesting a reload (#9140 fix 2 minimum). * test(sync): e2e-reproduce the #9140 brick; require e2e repro for sync changes Adds the reproducible end-to-end artifact for this fix: a Playwright spec that seeds a real schemaVersion-1 state_cache snapshot whose v1->v2 migration transform throws, then boots twice. Verified both ways: it FAILS against the pre-fix hydrator (boot bricks, task list never appears) and passes with the fix (tasks restored via op-log replay, fallback path pinned via console log, snapshot preserved at v1 on disk). lastAppliedOpSeq is seeded past all real ops so only the fallback's replay-from-0 can restore the tasks - the assertions discriminate the recovery path, not just task visibility. Found along the way: in the web e2e env the 'will not be persisted' boot snack replaces the recovery snack (SnackService debounces opens), so snack UX is asserted at the unit level instead; Electron/PWA sessions do not show the persistence warning. AGENTS.md: new sync-correctness rule - any sync-system change must START from a reproducible failure (failing test or scripted E2E against real data shapes, not a mocked seam); hardening without an observed end-to-end failure is how the sync layer accumulates overly defensive complexity. |
||
|
Johannes Millan
|
3d15fc50e8
|
test(sync): guard against required fields added without a migration (#9143)
* docs(agents): require reviews to ask if a feature earns its place * test(sync): guard against required fields added without a migration A required field added to a persisted model without a backfill migration breaks every existing install: data already on disk lacks it, and typia rejects it. The failure is latent — hydration trusts a snapshot whose schema version matches, so it only surfaces when an unrelated bump later drags it onto the migration path. #8965 shipped in January and detonated in v18.15.0 as a deterministic boot-to-empty-store (#9124). Nothing at runtime complains at the time the mistake is made, so only a test catches it. frozen-state-v18.15.json is a frozen snapshot of the persisted state shape as written by v18.15, run through migrateState -> validateFull. It stands in for data on users' disks and must never be regenerated from current defaults. Every slice carries at least one entity: typia cannot check an entity type whose collection is empty, so a fixture with `entities: {}` would validate no matter how the model changed — and issue-provider configs, where this class historically clusters, would have been the blind spot. Sabotage-verified on two slices by adding a required field to the model AND its DEFAULT_* (which compiles clean, as #8965 did): Project and RedmineCfg both fail the guard with the exact missing-field path. Note: typia inlines the model graph into validation-fn.ts, but Angular's build cache does not invalidate that file when a type it only imports changes — a warm-cache local run can report zero errors against a stale validator. CI builds cold. Documented in the spec docblock. Refs #9125, #9124 * test(sync): close coverage holes in the frozen-state guard Multi-agent review found the guard was thinner than its own docblock claimed, and that two pieces of its guidance were wrong. Coverage — the docblock asserted "every slice carries at least one entity", but eight collections were empty, so their element types were never shape-checked at all: reminders, pluginUserData, pluginMetadata, planner.days, timeTracking.project/tag, task.attachments, metric.reflections and both archive task collections. Populated all of them. issueProvider covered only the 8 built-in keys; the 6 migrated plugin-shaped members (GITHUB, CLICKUP, GITEA, LINEAR, TRELLO, AZURE_DEVOPS) and a plugin: provider are exactly what sits on disk after the issue-provider→plugin migration, so all 15 union members are now present. Jira/OpenProject availableTransitions are populated too — that field caused one of the historical occurrences. validateFull omits archiveOld/archiveYoung (DataToValidate), so a new required ArchiveModel field could ship green. Both are now validated explicitly via validateAppDataProperty. The emptiness invariant is now enforced rather than asserted in prose: a spec walks the fixture and fails on any collection that is empty everywhere it appears (per element TYPE, not per instance; *Ids lists are exempt as primitives). It caught four gaps the manual pass missed. Guidance — the regeneration advice was backwards. A transforming migration is NOT a reason to edit the fixture: migrateState applies it during the test, and the spec still passing is the proof the migration handles real v18.15 data. Hand-editing to the post-migration shape while the pin stays at 4 would re-apply the migration to already-migrated data. Also: a failed migration no longer falls through to validate undefined, and the failure message now carries the "do NOT edit the fixture" instruction, since the message is the only thing a developer reads before reaching for the quickest fix. AGENTS.md rule 11 had three inaccuracies: the per-section config merge covers 9 of 21 sections (not all config), the blanket globalConfig auto-fix runs only inside dataRepair rather than normal hydration, and "entity slices heal nothing" ignored the generic boolean/nullable coercions. It also pointed at recreate-fallback.const.ts, where membership additionally opts a type into SPAP-14 disjoint-field auto-merge — an unrelated sync behaviour change. Rewritten instruction first, and now prefers optional typing over a backfill migration, which rule 10 discourages because it costs a schema bump. A credential sentinel guards the one path by which this file could ever leak secrets: regeneration against a real profile. Re-sabotaged after the rewrite (RedmineCfg, cold cache): still fires. Refs #9125, #9124 * test(sync): regression-lock the #9124 idle backfill The guard cited #9124 but could not catch a regression of it. With the fixture pinned at schema 4 (== CURRENT_SCHEMA_VERSION), migrateState early-returns, so no migration ran: deleting the backfill in lww-replacement-barrier-v2-to-v3.ts left the whole spec green. Adds the case that closes it — v18.14 data is the frozen state at schema 2 minus the field #8965 added, so migrating it must backfill the field and still validate. Derived from the same frozen bytes rather than a second 900-line fixture: identical coverage, no duplication. This is also the only case exercising the migration chain until the next schema bump. Sabotage-verified the way the previous cases were: removing the backfill fails with "Expected undefined to be false". Also runs prettier over the fixture — .husky/pre-commit runs pretty-quick --staged, so an unformatted fixture would have handed the next contributor a several-hundred-line reformat diff on a file whose whole value is being reviewable. Refs #9125, #9124 * test(sync): clone the frozen fixture before migrating it migrateState returns `state` BY REFERENCE when source === target (migrate.ts early-out), so migrateFrozen() handed every spec in the file a live handle on the imported module singleton. Harmless today — all consumers are read-only — but one stray mutation would corrupt the fixture for whatever ran next, and that is an order-dependent flake this repo has been bitten by before. structuredClone costs well under a millisecond on 24KB. Also records a verified detail about the stale-validator caveat: clearing only the `babel-webpack` cache namespace is NOT sufficient. Measured both ways against a live sabotage — the narrow clear still reported green against a model that genuinely broke the fixture, because the typia output is cached under `angular-webpack`. Refs #9125 * test(sync): make the vacuity check explicit instead of heuristic The walk-based version inferred which empty collections were acceptable from a regex on the path name (`*Ids` = primitive list) plus a group-by-name rule. That was wrong in a way that mattered: a future OBJECT array named `somethingIds` would have been waved through silently, which is the exact failure mode this check exists to prevent. Replaced with an explicit list of the paths that must stay populated, plus a path lookup. A missing or renamed path now resolves to undefined and reports as empty, so the check fails loudly rather than silently skipping — fail-safe in the direction that matters. This is NOT a size reduction (194 -> 196 lines); the 22 explicit paths cost about what the heuristic cost. It buys auditability — a reviewer can read exactly what is covered — and removes the misclassification risk. Sabotage-verified: emptying `reminders` and `issueProvider.entities` (what regenerating the fixture from createValidAppData would do, the likeliest way this guard dies) fails the check. Refs #9125 |
||
|
Johannes Millan
|
18daadad71
|
test(sections): scope section title locator (#9142)
* docs(agents): require reviews to ask if a feature earns its place * test(sections): scope section title locator Match section containers through their collapsible title so hovered task controls such as right_panel_open cannot redirect cross-section drag targets. |
||
|
Johannes Millan
|
a9095ca10e
|
fix(sync): converge snapshot in one boot after schema migration (#9138)
* fix(sync): converge snapshot in one boot after schema migration Users saw "Failed to load data" on every launch after upgrading. A required globalConfig field (idle.isSuppressIdleDuringFocusMode, #8965) shipped without a migration backfill, so an old schema-v2 snapshot failed the migration-path validation gate. #9124 already made that gate non-fatal (reducer-healed hydration) and backfilled the field, but the safety-net path rolls the on-disk cache back to the old schema version and only re-persists when the tail-replay branch has >10 ops — so migration + validation re-run on every launch for a not-yet-backfilled field. Persist a fresh CURRENT_SCHEMA_VERSION snapshot at the end of a migrating hydration once the live reducer-healed state re-validates, regardless of tail-op count, so the cache converges in one boot. This resolves the TODO(followup) in operation-log-snapshot.service.ts. Safety: - Gated on a per-run migration-ran flag (now reset at the top of hydrateStore to kill a latent re-entrancy footgun) plus a double-save guard, so normal boots pay nothing and no boot double-writes. - Placed after retryFailedRemoteOps so the deferred-action pipeline is drained before the phantom-change guard (#8751) runs. - Re-validates the live state before saving so an unhealed/corrupt state is never cached (Checkpoint B trusts a matching-schema snapshot unvalidated). - Routes through saveCurrentStateAsSnapshot, keeping the #8469 quiesce, #8751 phantom guard and #7892 empty-overwrite guard; on any guard skip it simply re-migrates next boot, as before. Tests: - Convergence unit tests: persist-after-migration with zero tail ops, skip when the current state is invalid, no double-save when tail replay already saved, and no save on a normal (non-migration) boot. - Bug-class guard tying the globalConfig reducer heal to the real GlobalConfigState validator, so a future required-field-without-default is caught at CI instead of at users' next launch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BfzmAJs4DY1oMGrLETTAX7 * test(sync): harden post-migration convergence coverage A safety review found the convergence suite proved the feature with a single assertion (only the zero-tail-ops test failed if the production block was deleted); the other cases were negative/guard assertions, and three claimed safety behaviors were untested. Add: - a second positive test covering convergence after a migrating full-state-op (SyncImport) boot — the branch that loads directly and never sets the Checkpoint-C save flag; - an ordering guard proving the convergence save runs AFTER retryFailedRemoteOps (so the deferred-action pipeline is drained before the phantom-change guard); - a re-entrancy guard proving a later non-migrating hydrateStore() on the same instance does not converge (the per-run _migrationRanDuringHydration reset). Verified by mutation: disabling the convergence block now fails 4 tests (previously 1). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BfzmAJs4DY1oMGrLETTAX7 * docs(sync): correct convergence double-write comment A follow-up review noted the double-save guard only tracks the hydrator's own Checkpoint-C saves, not migrateSnapshotWithBackup's step-5 persist. On the healthy backfill path (migrated snapshot validates and is persisted) with few/no tail ops, convergence still writes once more. Clarify the comment: that extra write is harmless and rare (once per schema bump per device) and its post-replay state is strictly more advanced, so it is intentionally not suppressed. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BfzmAJs4DY1oMGrLETTAX7 * fix(sync): make post-migration convergence actually converge Follow-up hardening on the convergence block, from a multi-agent review of this PR. The design and its placement are unchanged; four defects in it are fixed. 1. saveCurrentStateAsSnapshot() returned void, but silently skips the write on the #8751 phantom guard, the #7892 empty-state guard, or a caught write failure. The hydrator set snapshotPersistedDuringHydration = true purely because the call resolved, so a guard-skipped Checkpoint-C save suppressed the convergence save — leaving the old-schema cache on disk and re-migrating every boot, the exact bug this PR exists to fix. It now returns whether it wrote, and both Checkpoint-C sites assign from that. 2. The block claimed placement after retryFailedRemoteOps() drained the deferred-action pipeline. It does not: that method early-returns before its try/finally when there are no failed ops, i.e. on the common boot. Any action buffered during the replay's sync window therefore kept the phantom guard tripped and skipped the save. Drain explicitly instead (no-ops on an empty buffer). The ordering vs retryFailedRemoteOps still matters for #7700 and is unchanged. 3. _validateCurrentStateForHydration is not throw-safe and sat inside the outer try, so a throw ran attemptRecovery() — which refuses because a snapshot exists — and surfaced "Failed to load data" on an otherwise healthy boot. A best-effort cache optimisation must not be able to fail hydration; the block now has its own error boundary. 4. Corrected two inaccurate comments: the second hydrateStore() call is real, not hypothetical (PluginBridge.reInitData() is a public plugin API reaching DataInitService.reInit()), and the cost is not "once per schema bump per device" — sync-hydration.service.ts persists a state cache with no schemaVersion, which reads back as v1 and re-migrates after every legacy SYNC_IMPORT. The convergence save stamps a version and ends that loop. Adding schemaVersion at that writer is left as a separate follow-up. Tests: four added, each verified by mutation to fail only for its own fix, including two paths that previously had no coverage at all — dropping the full-replay persisted-flag assignment, and dropping the top-level DEFAULT_GLOBAL_CONFIG spread (the heal spec asserted a missing section via `idle`, which the reducer also deep-merges per-section, so it never exercised the top-level spread; retargeted to `flowtime`). Also removed the spec NOTE claiming the typia transform does not apply under `npm run test:file`. It does — both scripts use the same builder and the same src/tsconfig.spec.json, which inherits typia/lib/transform from tsconfig.base.json. Verified by running it and by sabotage-probing that the validator rejects garbage. Full suite green in both TZ variants (13190 / 13176). * docs(sync): correct the sync-hydration schemaVersion note The previous commit's comment framed sync-hydration's version-less state-cache write as an incidental defect that convergence "ends". That reading is wrong and would invite a harmful one-line "fix". Downloaded snapshot data is never schema-migrated anywhere else on the client: migrateStateIfNeeded has exactly one call site (migrateSnapshotWithBackup, on the local state_cache), and the SYNC_IMPORT op carrying the payload is stamped CURRENT_SCHEMA_VERSION at creation, so op-level migration skips it too. Since the merged payload includes downloadedMainModelData, which can originate from an older-schema client, omitting the version is the only thing that forces that data through the migration chain on the next boot. Stamping a version there would freeze old-schema remote data into a cache that Checkpoint B subsequently trusts unvalidated. Convergence is unaffected: it only stamps a version after the migration chain has actually run. Comment-only; no behaviour change. --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
aakhter
|
bd7c2f2cee
|
test(sync): pin 3-client disjoint-merge composition (shared-field reconciliation + replace-mode convergence) (#8933)
* test(sync): pin three-client convergence after a merged op loses LWW Follow-up to #8874 (the retracted-then-adjudicated composition finding): after a disjoint-merged op loses whole-op LWW to a newer overlapping edit from a third client, the remote-win holders transiently keep the merged delta's other field while the local-win holder never applied it. Convergence depends ENTIRELY on _createLocalWinUpdateOp emitting a FULL-SNAPSHOT reconciling op with a dominating clock. This spec pins that closure property end-to-end (merge -> overlapping newer edit -> whole-op LWW on both holders -> snapshot propagation): if local-win ops ever become partial deltas - the same direction the merge path moved for analogous reasons - the three clients diverge permanently and this test goes red. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(sync): reframe 3-client convergence as a focused composition test Addresses maintainer review on #8933: - Drops the '(e2e)/all-clients' framing. It is a composition test of ConflictResolutionService's emitted ops applied via a local LWW helper, not a transport e2e; the comment now states the scope and that cross-client propagation is modelled (justified by the dominating-clock assertion), not executed. Client B contributes only an input op. - Adds the two assertions that make the convergence claim real: the local-win op reconstructs C's COMPLETE reviewable entity from a bare base (full-snapshot closure), and its clock is GREATER_THAN both inputs (opC and the merged op), so it propagates as a plain non-conflicting remote op. - Updates the local resolveCapturing helper to the #8900 mixed-source-batch API (appendMixedSourceBatchSkipDuplicates); the rebase onto current master surfaced that the prior appendWithVectorClockUpdate mock no longer matches the service. SPAP-39 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(sync): scope 3-client claim to shared-field reconciliation; prove receiver-only-field divergence Addresses maintainer review on #8933. The three-client composition test previously claimed full-snapshot closure (stateA === stateC / identical entities). johannesjo gave a verified counterexample: applying C's full local-win snapshot to A cannot clear a field C never had (SuperSync JSON-serializes ops so absent fields don't travel as clears; lwwUpdateMetaReducer applies via a shallow updateOne), so A keeps it and the clients diverge — with a dominating clock, permanently. - Narrow the active composition test to the property that actually holds: present-field reconstruction + a dominating clock + shared-field reconciliation. It no longer asserts byte-identical whole-state equality. - Replace the pending() aspiration with an ENABLED test asserting today's ACTUAL behavior: run the real resolution service to get C's local-win op, JSON round-trip its payload, apply it through the PRODUCTION lwwUpdateMetaReducer, and assert A retains the receiver-only dueDay while C lacks it (divergence). Flip the dueDay assertions when the runtime reconciles receiver-only fields. The underlying runtime data-integrity bug is tracked in SPAP-43. SPAP-39 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(sync): migrate disjoint-merge spec to production op-apply seam; pin convergence Rebasing #8933 onto current master surfaced that upstream #8980/#8990 reshaped the LWW op payload (adapter `{ task: { changes } }` -> nested `{ actionPayload, entityChanges, lwwUpdateMode }`) and #9007/SPAP-43 made `_createLocalWinUpdateOp` stamp `lwwUpdateMode: 'replace'`, applied via setOne. Per johannesjo's #8933 review: - Content-model applyOp: also read the nested `actionPayload` shape so the no-receiver-only-field test reconstructs the op's carried fields correctly. (This limb only asserts WHICH fields the op transports; no clearing in play.) - Receiver-only test: build the action via the real `convertOpToAction` instead of hand-rolling it. The hand-built action spread `lwwUpdateMode` at the top level, so it never reached `action.meta` and silently took the updateOne (shallow-merge) branch — masking production. Through the real seam the 'replace' op applies via setOne: A's absorbed receiver-only `dueDay` and B's `notes` are CLEARED and the clients CONVERGE. Flipped the assertions and reframed the test from "diverge" to "converge" — the SPAP-43 replace/setOne work closed the gap the test previously (incorrectly) documented as open. Verified end-to-end: 28/28 in-file green on current master. SPAP-39 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(sync): fix stale op-log-store mock; reconcile flipped convergence comments Round-4 review fixes for #8933 after rebase onto current master: - resolveCapturing helper's createSpyObj was a third, un-maintained copy of the OperationLogStoreService mock and had rotted: renamed appendWithVectorClockUpdate -> appendWithVectorClockOverwrite (#9080) and added the missing getOpById (#9086). This was the CI-red blocker (TypeError: opLogStore.getOpById is not a function). - Flip the two describe-level headers that still carried pre-flip divergence framing. Master now stamps lwwUpdateMode: 'replace' on local-win ops and the reducer applies replace-mode via setOne, so test 2 asserts the receiver-only field IS cleared and the clients converge -- the headers said the opposite. - Add a receiver-version scope note: the disjoint merge is enabled in production (unfrozen by #9095), but only replace-mode-aware receivers converge; older clients apply the snapshot via updateOne and still diverge. 29/29 SUCCESS in conflict-resolution.disjoint-merge.spec.ts. SPAP-39 * chore(sync): scrub private tracker keys from disjoint-merge spec The spec ships to the public upstream PR and the maintainer is GitHub-only, so replace internal tracker keys with neutral descriptive wording: - top-level describe + file header: drop the key, keep "disjoint-field merge" - (a3)/(a4)/(a5) section comments: "disjoint-merge fix:" instead of the key - 3-client test comments: "the replace/setOne behavior/work" without the key No behavioural change; 29/29 SUCCESS. SPAP-39 --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
CuriousChipmunk
|
b902edb77b
|
Update community-plugins.json (#9123) | ||
Johannes Millan
|
014b789c22 | 18.15.1 | ||
|
Johannes Millan
|
c29c8e6db9
|
ci: stop running redundant unit tests on the macOS build job (#9129)
* test(sync): deflake SPAP-35 banner-refresh specs on slow CI runners The two positive live-refresh specs waited a fixed 160ms for the revision-triggered refresh (100ms auditTime window + async journal read), leaving only ~60ms of slack. On the slow/contended macOS CI runner the trailing refresh had not fired yet when the assertion ran, so it saw the banner's original counts and failed intermittently (Mac-only flake). Replace the fixed sleep with a waitFor() poll that waits for the refresh to actually land (open/dismiss call), so the assertion no longer depends on runner speed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P2k84g6MWEEG8Zx3MUShD9 * ci: drop redundant unit-test step from macOS build job The manual Mac build job ran the full Angular/Karma unit suite, which is platform-independent (headless Chrome) and already gates every PR on Linux via ci.yml → test-on-linux. Re-running it on the slow, expensive macOS runner added no coverage and only surfaced timing-sensitive test flakes. The Mac job's purpose is the signed DMG build; keep that, drop the tests. This matches existing repo precedent: the Mac Store release workflow already has its unit-test step commented out, and test-mac-dmg-build.yml runs none. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P2k84g6MWEEG8Zx3MUShD9 --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Johannes Millan
|
a91f830982
|
test(sync): deflake SPAP-35 banner-refresh specs on slow CI runners (#9128)
The two positive live-refresh specs waited a fixed 160ms for the revision-triggered refresh (100ms auditTime window + async journal read), leaving only ~60ms of slack. On the slow/contended macOS CI runner the trailing refresh had not fired yet when the assertion ran, so it saw the banner's original counts and failed intermittently (Mac-only flake). Replace the fixed sleep with a waitFor() poll that waits for the refresh to actually land (open/dismiss call), so the assertion no longer depends on runner speed. Claude-Session: https://claude.ai/code/session_01P2k84g6MWEEG8Zx3MUShD9 Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Johannes Millan
|
49db32e0ba |
Merge branch 'feat/with-the-latest-changes-i-get-your-app-4212df'
* feat/with-the-latest-changes-i-get-your-app-4212df: docs(sync): warn against bumping CURRENT_SCHEMA_VERSION unnecessarily |
||
|
Johannes Millan
|
27fb138d9b |
docs(sync): warn against bumping CURRENT_SCHEMA_VERSION unnecessarily
A schema bump is near-irreversible and hard-fences lagging post-v18.14.0 clients (frozen cursor). The existing Bump Policy only warned against bumping for changes old clients would MISAPPLY; it never said don't bump for a change old clients can TOLERATE — the gap #9009 (v4 delete-wins) fell through. Add a 'default: do NOT bump' principle to the canonical policy (A.7.11) and short cross-referencing gates at every bump-initiation point: the CURRENT_SCHEMA_VERSION doc-comment, the migration-registry checklist (step 0), AGENTS.md rule 10, and a RETROSPECTIVE note on the offending v3->v4 migration. |
||
|
Johannes Millan
|
51e6151682 | 18.15.0 | ||
|
Johannes Millan
|
5ed1f2c5d0
|
fix(supersync): prevent migrator advisory-lock leak on deploy timeout (#9126)
A timed-out `docker compose run` in deploy.sh SIGTERMs the compose CLI but can leave the migrator container (and its DB connection) running detached, which keeps Prisma's session-level migration advisory lock held and wedges the next deploy with P1002. - deploy.sh: run_migrator() names each one-off migrator container from a single MIGRATOR_NAME_PREFIX and force-removes it (inline + $$-scoped EXIT sweep) so a timed-out run can't orphan it and leak the lock; add timeout -k. - migrate-deploy.sh: recognize P1002 advisory-lock timeouts and print copy-paste diagnosis + cleanup guidance instead of a generic exit 1; never auto-terminate a backend (an active CONCURRENTLY build legitimately holds it). - tests: P1002 fake-npx case + non-vacuous recovery test; assert the name/sweep-filter prefix single-source-of-truth invariant. - env.example: document the recommended higher MIGRATION_TIMEOUT and the new force-remove-on-timeout behavior. |
||
|
Johannes Millan
|
d0b7bda01a
|
fix(sync): heal missing idle config field on schema-migration upgrade (#9124)
A v18.14 snapshot (schema v2) lacks globalConfig.idle.isSuppressIdleDuring FocusMode, which #8965 added as a required field without its own schema bump. Upgrading bumps the snapshot onto the v2->v4 migration path, whose validation gate — uniquely, the only validator that runs on the RAW snapshot before the loadAllData reducer can backfill defaults, and the only one that is fatal rather than repair-or-tolerate — rejects it. Hydration then aborts, recovery refuses because a snapshot still exists, and the app boots to an empty store, deterministically, every launch. Two fixes: - v2->v3 migration backfills the opt-in default (false) when the field is not already a boolean (never clobbering a real user choice), so the migrated snapshot validates and a clean v4 snapshot persists immediately. - The migration-path state-validation gate is now non-fatal: on failure it rolls the on-disk cache back to the pre-migration backup (never persisting an unvalidated snapshot) but returns the migrated snapshot for reducer-healed hydration, instead of throwing into disaster recovery. Genuinely corrupt state is still caught at Checkpoint C and not persisted. Metadata-validation failures remain fatal. Both paths are sabotage-tested to fail without their fix. A known follow-up (noted in code) can persist a fresh snapshot after any migration-then-valid hydration so the safety-net path converges in one boot instead of re-migrating each launch for a not-yet-backfilled field. |
||
|
Johannes Millan
|
f15a20ba8d |
fix(supersync): make interrupted CONCURRENTLY migrations recoverable
A bare CREATE INDEX CONCURRENTLY aborted mid-build (the in-image step
timeout firing at its 1800s default when a raised MIGRATION_TIMEOUT was
not forwarded, an external stop, or OOM) leaves the migration failed and
an INVALID index of the target name, wedging the deploy. Deployed images
reported only a bare "prisma migrate deploy failed (exit 143)" with no
recovery steps, so operators had no way forward.
- deploy.sh: forward MIGRATE_STEP_TIMEOUT into the migrator so a large
MIGRATION_TIMEOUT is not silently capped at the image default (1800s)
and kill a slow CREATE INDEX CONCURRENTLY early (the root cause).
- migrate-deploy.sh: normalize BusyBox `timeout` 143 -> 124 so a step
timeout hits the timeout branch, not the generic one.
- migrate-deploy.sh: emit_interrupted_recovery_hint() prints copy-paste
recovery (drop the INVALID index, roll the record back, re-run) for an
interrupted CONCURRENTLY build, from BOTH the 124 timeout branch (where
a normalized 143 -- the incident's own signal -- lands) and the generic
non-gate branch (OOM/137), plus the existing P3009 re-run path. Every
path prints guidance only and NEVER auto-resolves a bare CREATE.
- env.example: document the now-forwarded MIGRATE_STEP_TIMEOUT knob.
- tests: cover 143 normalization, the incident's 143->124 timeout-branch
recovery, the P3009 and non-gate bare-create recovery, and the
forwarded step timeout.
The bare CREATE stays bare and fail-loud on purpose: 20260613000001
ships in v18.11.0-v18.14.0 and is applied natively on healthy deploys,
so converting it to a drop-then-create shape would change its checksum
and break every DB that already applied it.
Recovery for the current wedged deploy (20260613000001):
DROP INDEX CONCURRENTLY IF EXISTS "operations_entity_ids_gin";
prisma migrate resolve --rolled-back \
20260613000001_add_operation_entity_ids_gin_index
then re-run the deploy with the step timeout raised enough for the GIN
build (and clear any idle-in-transaction blocker first).
|
||
|
Johannes Millan
|
238b91aa59
|
test(e2e): accept beforeunload prompt in shared dialog fallback (#9122)
The USE_REMOTE crash-resume spec reloads Client B mid-sync. While isSyncInProgress is set, startup.service's beforeunload handler calls preventDefault(), so Chrome raises a beforeunload prompt (the preceding button click supplies the user activation it requires). Playwright auto-accepts such prompts only when no 'dialog' listener is registered — but installDevErrorDialogHandler registers one on every page and early- returned on beforeunload, leaving the prompt unanswered. The navigation then never starts and reload() times out (observed on SuperSync shard 6/6; the earlier load→domcontentloaded de-flake could not help because navigation is blocked before any lifecycle event). Accept beforeunload in the shared fallback, restoring Playwright's default; other dialog types keep flowing to spec-specific handlers. Corrects the two crash-resume comments that encoded the wrong theory. |
||
|
Johannes Millan
|
c18055a341
|
fix(android): stop the widget labelling a stale list as Today (#9098) (#9118)
The widget renders a pre-computed snapshot Angular pushes into `widget_data`; the blob carried no expiry, so a process that stayed dead across midnight kept rendering yesterday's tasks under a hardcoded "Today" header, indefinitely. Native cannot recompute the list. TODAY_TAG membership is virtual, today's repeat instances do not exist as entities until TaskDueEffects materializes them on a day change, and overdue carry-over runs there too — so there is no persisted field a native filter could read that would give the right answer even in principle. Only running the app can produce today's list. So make the widget honest instead of wrong: Angular stamps the snapshot with `validUntil`, the instant it stops being today (start-of-next-day offset included), and `dayStr` for the label. Native's whole verdict is `WidgetData.headerFor` → `now >= validUntil`; when stale it renders the snapshot's own date instead of "Today". The list stays visible and useful, it just stops claiming to be today's. A stale snapshot whose day cannot be read says "Outdated" rather than falling back to the very lie this fixes. Shipping the boundary rather than its inputs keeps the app's calendar rules in one language. The iOS port (#8950) can consume `validUntil` unchanged instead of mirroring getDbDateStr semantics into Swift, where a non-Gregorian default locale would silently misread the day. The verdict is a pure function so it is unit-testable: this project has no Robolectric, and a decision left inside the provider could ship inverted and green. Both refresh paths rebuild the header. A push can change the day the blob describes; a tap cannot, but it re-renders at a later `now` than the last verdict was computed at — and it is the one interaction that reaches our code while the app is dead, so a tap on a new day must not redraw rows under a "Today" header. Also fixed: onUpdate re-registered the adapter with an unchanged intent, which does not re-invoke the factory's onDataSetChanged(), so the periodic update rendered whatever rows the adapter last built. `v` stays 1: the fields are additive, and parse() returns empty for any other version, so a bump would blank the widget of every install until next opened. A pre-#9098 blob has no `validUntil` and is never reported stale — unknown must not read as expired. Known bounds, documented in the plan rather than papered over: the label flips only on a push, a tap, or the inexact Doze-deferred 30-min periodic update, and the launcher paints cached views on unlock — so the lie is bounded, not eliminated. Force-stop is not a gap: the system masks a stopped package's widget entirely, so the reported symptom can only occur in the Doze band. An exact alarm at `validUntil` would close the rest; deferred on cost/scope, not because it would not work. Verified: 25/25 Kotlin across 6 timezones incl. midnight-gap zones (Santiago, Apia); 203/203 Angular in both timezones CI runs. Boundary math asserted against the real getDbDateStr rollover. Sabotage-measured, not assumed: inverting the verdict fails 5 tests, >=→> fails 4, dropping the validUntil guard fails 3, Locale.US→getDefault fails 1, dropping the day round-trip fails 1. |
||
Jon Kilroy
|
d5afe62016
|
feat(schedule): add a single-day view to the Schedule tab (#9058)
* feat(schedule): allow 'day' as a persisted time-view mode * feat(schedule): compute single-day range and header for day view * feat(schedule): add day-view toggle and single-day labels Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JzGvAaeT2TrZcEUA6fYPSZ * docs(schedule): note the day view mode in the Schedule wiki * fix(schedule): make the day-view header compact and locale-aware Address review feedback on the single-day view: - Build the day title with a locale-aware Intl skeleton instead of a hardcoded 'EEE, MMM d, yyyy' pattern, which pinned en-US field ordering. - Shrink the title responsively so it never gets ellipsis-clipped: show the full weekday + date + year on roomy widths, and fall back to a compact month + day below TABLET, where the toggle group and nav controls leave too little room. The weekday still shows in the day-column header. - Rename the .day-view-btn test hook to .e2e-day-view-btn per convention. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(schedule): rename week-month-* classes to time-view-* The toggle group now has three buttons (Day, Week, Month), so the week-month-selector / week-month-btn names no longer describe it. Rename to time-view-selector / time-view-btn. Pure CSS class rename, no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(schedule): complete time-view-* rename in zen theme The class rename left zen.css targeting .week-month-*, so its overrides matched nothing. The :not(.active) rule is the load-bearing one: its !important suppresses the toggle's hover highlight, which zen exists to strip. Co-authored-by: Jon Kilroy <jkusa@users.noreply.github.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Johannes Millan <johannes.millan@gmail.com> Co-authored-by: Jon Kilroy <jkusa@users.noreply.github.com> |
||
coyaSONG
|
5097155e25
|
feat(work-view): show custom section task counts #9068 (#9109) | ||
|
Johannes Millan
|
99f923ad6c
|
docs(sync): fix stale schema-compat docs, add bump policy, rescope Task 6 (#9119)
* docs(sync): fix stale schema docs, add bump policy, rescope Task 6 * docs(sync): add severity-triage and schema-bump rules to AGENTS.md * docs(sync): fix review findings in schema compat docs |
||
|
Johannes Millan
|
6756e9998c |
docs(sync): record fast-track simplification audit
Capture the read-only audit evidence and its verified and provisional risk/reward guides in one baseline-preserving changeset. |
||
|
Johannes Millan
|
4519c5f4d3 |
docs: correct simplification audit findings
Replace stale placements for revised scopes, separate line-level LOC categories, and neutralize superseded implementation guidance. |
||
|
Johannes Millan
|
0204210c5b |
docs: document non-sync simplification audit
Record the complete non-sync audit, two-pass candidate review, conservative reconciliation, global risk/reward order, and verified stashed implementation state. |
||
|
Johannes Millan
|
fdbc2c1a86
|
refactor(sync): centralize clock pruning in store, make merge atomic (#9107)
Follow-up to
|
||
|
Johannes Millan
|
fcbb789746
|
docs(sync): record #9105 staleness-eviction plan, improve prune snack (#9106) | ||
|
Johannes Millan
|
a224eb5fa3
|
fix(sync): keep import author in client-side clock pruning (#9096) (#9102)
The client pruned its durable vector clock with uploader-only protection, so once 21+ client ids accumulated after a full-state import, the import author's low-counter entry was evicted. Every subsequent local op then permanently failed the sync-import filter's knows-import-counter rescue and was dropped as CONCURRENT on every peer — the client-side ceiling of the server fix in #9089. - client limitVectorClockSize wrapper now takes a preserve list, matching the shared implementation - calculateRemoteClockMerge (remote merge + reducer checkpoint) preserves the latest full-state author; an in-batch full-state op supersedes the stored one, and the checkpoint resolves the author inside its transaction so a just-rejected import cannot name the protected author - snapshot save, compaction, hydrator restore, and the sync-hydration file-snapshot bootstrap protect the author on their durable-clock paths - docs: add the missing calculateRemoteClockMerge prune site, correct the stale RepairOperationService rows (repair ships the full clock), and reword the sync-core pruning comment to the real invariant |
||
|
Johannes Millan
|
b1f074705a
|
test(project): guard project navigation route detection (#9103) | ||
ghostkeni
|
67cd017bc7
|
Add critical tests for task and sync helper flows (#9079)
* test: add critical coverage for task and sync helpers Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * test(tasks): fix vacuous alert assertion and zero-delta time test - Reset devError latch and alert spy call history before orphan-subtask assertion so the test is not vacuous in a full Karma suite run - Change DAY_3 timeSpent from 60 to 90 in updateTimeSpentForTask test so totalDelta is non-zero (+30), making the accumulator mutation detectable by mutation testing - Add isLww=false short-circuit case to bulk-archive-filter spec Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> |
||
|
Johannes Millan
|
e5a9ff7866
|
fix(sync): unfreeze the disjoint-field merge to stop data loss (#9095) (#9101)
Drop disableDisjointMerge from the production conflict-resolution entry point. With the merge frozen (#9061), concurrent edits to different fields of one entity resolve by whole-entity LWW: the later side wins a full 'replace' snapshot with a dominating clock and the other side's edit is silently and permanently lost on every client (a rename dies when another device marks the task done). The journal half of the freeze stays. Wire-safe: v18.14.0 predates lwwUpdateMode entirely and applies LWW Update payloads via updateOne, so the merge's 'patch' union-delta merges cleanly on released clients (#8874's back-compat envelope design). Restores the strengthened 3.1 disjoint-merge E2E parked out of #9089 (it fails deterministically while the merge is frozen) and adds a unit regression for the exact rename-vs-done pair; the caller guard spec now pins that the merge stays enabled on the production resolve path. |
||
|
Johannes Millan
|
b50d5f6d96
|
fix(sync): dedupe surgical-sync retries and keep the import author through pruning (#9089)
* fix(sync): deduplicate surgical sync retries Persist split-file configuration and acknowledge operation IDs already committed remotely after a lost upload response. * fix(sync): preserve import author during clock pruning Keep the active causal full-state author in oversized stored clocks and reuse the stored protected IDs when classifying response-loss retries. * test(sync): harden supersync failure coverage Exercise real upload endpoints and exact operation IDs across response loss, validation failures, schema blockers, full-state boundaries, vector pruning, and concurrent edits. * fix(sync): heal a corrupt primary on duplicate-only uploads The .bak recovery path caches the CORRUPT primary's rev precisely so this cycle's conditional overwrite repairs sync-ops.json. The duplicate-retry short-circuit read that cache and returned before the write, leaving the primary corrupt whenever the recovered buffer already held every pending op. Flag the recovered entry and let those uploads fall through. Also stop synthesising a serverSeq for ops already in the buffer: the field is optional, the upload and download paths number ops differently, and a mixed batch could hand two ops the same value. * perf(sync): look the full-state author up once per upload Batch upload is off by default, so the guarded batch path was not the one serving production: the serial path queries the causal full-state author per op inside a single transaction, and a clock of 21-50 entries passes validation and trips the guard on every one of them. Memoize the author per transaction and resolve it lazily, so only an op whose clock actually overflows pays, and only once. This also retires the batch pre-scan and its loop-carried author, leaving both paths on one mechanism. Report the lookup through ProcessOperationResult so the upload summary stops under-reporting round-trips, and record why reconstructing the stored protected set loosens id-collision detection. * test(sync): wait for the committed title in renameTask renameTask blurred the textarea, slept 300ms and returned without ever checking the rename landed. Blur -> dispatch -> re-render outruns that delay on a loaded machine, so a following sync uploads without the rename op and the caller asserts against a task that was never renamed — which is what supersync 3.1 hits on CI but never locally. Wait for the new title instead, mirroring markTaskDone's done-state wait and the e2e no-waitForTimeout rule. * test(sync): dispatch focus so renameTask actually commits renameTask relied on el.focus() to emit a focus event, but these tests drive two clients as separate pages and only one page can hold focus, so on CI the event often never fires. TaskTitleComponent then keeps _isFocused=false, and resetToLastExternalValueTrigger resets tmpValue to the stored title on the next task-object emission. Blur therefore computes wasChanged=false, task.component skips update(), and the rename is silently dropped without ever becoming an op. That is supersync 3.1: client A's rename lives only in tmpValue, A syncs and uploads nothing, B uploads its done op, A downloads it, the task ref changes and the title reverts to the original — exactly the state the CI artifact captured. A real user always has real focus, so the app itself is unaffected. Dispatch focus explicitly, mirroring the synthetic input/blur already used here. Also correct the previous commit's claim: the toBeVisible wait matches tmpValue, a component-local signal rendered in both template branches, so it never observed the committed title and could not have fixed this. * docs: revert incidental prettier reformat of unrelated docs The master merge ran prettier across files it pulled in, reformatting three documents this branch has no business touching: markdown table cell padding plus *emphasis* -> _emphasis_, with no content change. handover.md documents two unrelated branches entirely. Restores them to master. vector-clocks.md keeps its edits — those are this branch's own and describe the pruning protection. * refactor(sync): drop the full-state author lookup's roundtrip accounting resolveFullStateAuthor memoizes per transaction, so the lookup it counts fires at most once per upload — the plumbing existed to report a number that is always 0 or 1, on a log line already counting dozens. The memo and its own accounting cancelled out. Removing didQuery lets resolveFullStateAuthor return string | undefined and getPruneProtectedIds return string[], instead of both carrying a tuple purely to feed the counter. uploadDbRoundtrips and the batch path's own counter are untouched. * test(sync): assert the committed store title in renameTask The focus dispatch did not fix supersync 3.1 — the shard failed again with an identical snapshot (original title, done, rename gone), so that diagnosis was wrong. Stop guessing at the trigger and make the helper able to observe the thing in question. task-title renders tmpValue, a component-local signal, in BOTH its editing and idle branches. Every DOM assertion here therefore matches as soon as the synthetic input event fires, whether or not an op was ever captured — which is why two rounds of "wait for the title" changed nothing. Read the store instead, via the __e2eTestHelpers.store hook the timeSpent helper already uses. This is a diagnostic as much as a fix: it splits the two remaining explanations. If renameTask now fails, the rename never becomes an op and the bug is in how the test drives the edit. If it passes and 3.1 still fails at the merge assertion, the op is captured and lost during sync — a real defect, and the test is right to fail. * test(sync): move the 3.1 disjoint-merge rewrite out to #9095 3.1 was the last red shard, and it turned out to be right: the store-backed renameTask passes, so the rename IS captured as an op, and the test still fails at the merge assertion — the op is committed and then lost during sync. Filed as #9095. That bug is pre-existing and cannot be reached by anything in this PR: the file-based adapter is not used by SuperSync, and the server-side author memo only engages for clocks over 20 entries where this test carries about three. 3.1 is also the only test here that exercises neither of this PR's fixes — it races a title change against a move-to-done, which is conflict resolution, not retry dedup or clock pruning. So it moves to #9095 rather than holding verified sync fixes red. The rest of the hardening stays: the fault injections whose globs never matched a real endpoint, the schema-mismatch test that asserted nothing, and the compaction suite that called an endpoint which never existed are what actually cover the fixes here. Restoring the old 3.1 puts a misleading test back, so it now carries a comment saying why it proves little and where the real one lives. The strengthened version is kept on test/issue-9095-disjoint-merge-repro. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> |
||
|
Johannes Millan
|
0238b5a729
|
fix(sync): fence in-flight sync cycles across destructive config changes (#9088)
* fix(sync): fence in-flight sync cycles across config changes (#9074) Destructive config changes (encryption enable/disable/password change, provider/account switch) blocked NEW sync cycles but neither drained nor cancelled a cycle already mid-await, which could then apply remote ops, upload, acknowledge, or advance the cursor against the new epoch/target (cross-epoch/cross-provider contamination, mixed-key server history). Fix, per the issue's KISS sketch: - Monotonic sync epoch on SyncProviderManager, bumped AFTER each provider switch / target-moving config write / bypass ingress, and at runWithSyncBlocked entry. Content-only saves do not bump. - Every cycle entry point (main sync, immediate upload, WS download, force upload) captures the epoch with its cycle-guard claim and threads it as fenceEpoch. - Provider I/O is fenced in one choke point: getOperationSyncCapable() returns a per-cycle delegate that re-asserts the epoch before every provider call (uploads, downloads, all setLastServerSeq cursor writes). Local writes (apply-lock closures incl. the full-state path, deferred acks, hydration, migration appends, rejected-ops handling, raw-rebuild resume) re-assert at the call site. - Stale completions throw SyncEpochChangedError, handled everywhere as a benign abort (no error snack; UNKNOWN_OR_CHANGED) — each abort point lands in a crash-equivalent state by design. - runWithSyncBlocked is serialized, sets the block flag FIRST, bumps the epoch, then drains the main sync AND the cycle-guard side channels (bounded, throws on timeout) — the fence cannot recall request bytes already on the wire, so destructive remote writes wait for the stale cycle to settle. ImmediateUploadService now routes through WrappedProviderService instead of a raw cast. Closes the audit findings C1-1 (ORCH-1) and C1-2 (SWITCH-2). * fix(sync): don't bump the sync epoch on first-time setup (#9074) Every conflict-dialog E2E timed out on `SyncEpochChangedError (1 → 2)`: the first-ever provider activation and the first-ever config save (no previous privateCfg, so isSyncTargetChanged reports a target change) both bumped the epoch, racing the fresh config's first sync into a spurious abort — the dialog-producing cycle itself was fenced. First-time setup has no OLD target an in-flight cycle could be running against (a pre-activation cycle sees getActiveProvider() === null and exits; a pre-first-save cycle is not ready), so these bumps fence nothing and only cause false positives. Gate them: bump on a config save only when a previous config existed, and on activation only when a previous provider was active. Real switches (X→Y, X→null) and real target moves still bump; the cache-invalidation emission keeps its true-on-first-save semantics untouched. * fix(sync): read the (provider, epoch) fence pair in one sync block (#9074) The SuperSync provider-switch E2E still aborted its first post-switch sync: the cycle captured the fence epoch at its guard claim but fetched the provider object several awaits later, so a switch completing in between handed the cycle the NEW provider with a STALE epoch — the fence then aborted a cycle that was actually running against the new target. The consistency rule is pair atomicity, not claim atomicity: a switch swaps the provider object and bumps the epoch in one synchronous block, so reading getActiveProvider() and syncEpoch in one synchronous block always yields a consistent pair (old+old aborts on the later bump; new+new proceeds). Move the capture to the provider read in the main sync and WS-download cycles (immediate upload aligned for uniformity — it was same-block already only by accident of having no await between). |
||
|
Johannes Millan
|
57cee868bf
|
fix(locale): consolidate textLocale, fix planner month label, enforce via lint (#8987) (#9065)
* refactor(locale): collapse inlined textLocale copies into the helper No behavior change: textLocale() is defined as isoTextLocale() ?? currentLocale(), which is exactly what these five sites had inlined. #9056 was based on master and so could not use the helper #9055 adds — it re-inlined the expression at focus-session, habit-tracker, worklog, scheduled-list and scheduled-date-group. Now that both have landed, collapse them so there is one canonical spelling instead of six. Caller specs mock textLocale() directly rather than the isoTextLocale/ currentLocale pair, mirroring what each SUT actually calls. * fix(locale): planner month label followed the browser locale, not the app's monthLabel passed no locale to toLocaleDateString, so the spelled-out month followed the *browser's* locale and ignored both the configured date locale and the UI language: a German browser rendered 'Juli 2026' in an English app. Same family as #8987 but reachable without the ISO option at all. Route it through textLocale(), which is the UI language under the ISO option and currentLocale() otherwise. The two existing specs computed their expected value with the same undefined locale, so they mirrored the bug and could never have caught it. They now pin a fixed app locale, deliberately not the runner's browser locale — otherwise they would pass either way. * test(lint): add require-text-locale rule and enforce it over src/app Guards the #8987 invariant that kept recurring: spelled-out weekday/month names must be formatted with textLocale(), never currentLocale() (the ISO option's 'sv' sentinel) and never the implicit browser locale. Three PRs chased this bug class site-by-site because currentLocale() is the obvious-looking default at every new call site. - Resolves `const locale = ...currentLocale()` through the scope chain: that is the shape the original bug had in plannedStartDateStr, so a rule matching only direct calls would have missed the very bug it exists to prevent. - Covers new Intl.DateTimeFormat() too — the same trap in constructor form, used at ~9 sites. Stays silent on clock times (hour/minute/dayPeriod), which must keep currentLocale() so the ISO 24h format survives. - Specs excluded: computing an expected string against an explicit locale is a legitimate test technique; the invariant is about what the product renders. - Documents its own blind spots (locale threaded through a parameter, reassigned variables, non-literal options) and pins them as valid cases, per the no-multi-entity-effect convention. Error severity is safe: textLocale() equals currentLocale() for every non-ISO option, so for spelled-out names it is never worse. Zero violations remain. * fix(lint): keep require-text-locale silent on clock-time formats The rule documented itself as staying silent on clock times, but `dayPeriod` sat in ALWAYS_SPELLED_OUT and only `.toLocaleTimeString()` was excluded — so `{ hour, minute, dayPeriod }` via `Intl.DateTimeFormat`/`toLocaleString` did fire, and its message told the reader to switch to textLocale(). Following that advice flips the ISO 24h clock to 12h: "13:05" -> "1:05 in the afternoon". The same holds for any mixed date+time options object: `{ weekday, hour }` goes from "onsdag 13:05" to "Wednesday 1:05 PM" — a Swedish name traded for a broken clock, the exact ISO regression this rule family exists to prevent. A format that mixes a spelled-out name with a clock has no single correct locale; it has to be split (names on textLocale(), clock on currentLocale(), as plannedStartDateStr does), which is more than a one-locale message can advise. So skip any options object containing `hour`, and say so. This costs a blind spot on `{ weekday, hour }` — cheaper than confidently wrong advice at `error` severity. No call site changes: `dayPeriod`/`era` have zero uses in src/, so this was latent. All four real bug shapes are still caught; src/app stays at zero violations. The two clock-time `valid` cases named dayPeriod in their comments but only ever tested `{ hour, minute }` in their code, which is how this slipped through — pin the actual shapes instead. * fix(lint): catch dateStyle in require-text-locale The rule missed `dateStyle` entirely, so the canonical #8987 shape walked straight past it: `toLocaleDateString(currentLocale(), { dateStyle: 'full' })` renders "onsdag 15 juli 2026" under the sentinel — a spelled-out weekday and month — without naming weekday or month at all. Zero call sites today, so this was latent, but guarding call sites that do not exist yet is the rule's whole job. `dateStyle` needs its own value set rather than month's: the two invert. `month: 'short'` is "Jul" (spelled out) but `dateStyle: 'short'` is "2026-07-15" (numeric), so reusing SPELLED_OUT_VALUES would have flagged dateStyle:'short' and pushed the reader to route ISO's YYYY-MM-DD through textLocale() — the mirror of the clock-time trap. Modelled as a per-field map so the inversion is stated where it can't be conflated, and pinned from both sides: 'short' as valid, 'full'/'medium'/'long' as invalid. Sabotage-verified — swapping in month's value set fails the spec. `timeStyle` joins `hour` as a clock-time field: `{ dateStyle, timeStyle }` is the mixed date+time case again ("onsdag 15 juli 2026 kl. 13:05" -> "Wednesday, July 15, 2026 at 1:05 PM"), and it carries no `hour` key for the existing guard to catch. Verified: 6/6 real bug shapes flagged, 0/6 false positives on correct usage, src/app still at zero violations. |