sync + coverage: measure every suite, and open the app at desktop size - #75
Closed
alichherawalla wants to merge 104 commits into
Closed
sync + coverage: measure every suite, and open the app at desktop size#75alichherawalla wants to merge 104 commits into
alichherawalla wants to merge 104 commits into
Conversation
M0 of docs/SYNC_INTEGRATION_PLAN.md. Brings the public sync engine in so the pro integration can consume it; no behaviour wired yet. - Vendors shared/packages/sync -> desktop/packages/sync following the existing convention (@offgrid/clipboard|design|models|rag are git-tracked copies consumed via file: deps). Records provenance (offgridVendoredFrom: commit 9b671b5) in the vendored package.json, because the existing copies have silently DRIFTED from shared/ and that should be visible. - Adds runtime deps the engine needs: bonjour-service (pure-JS mDNS, no native build, used by the node-discovery adapter), tweetnacl, tweetnacl-util, js-sha512. - Engine is consumed UNCHANGED: the mobile lane is working in the same package and two sessions editing it is the one guaranteed conflict. Engine changes go through the plan's 'Engine asks'. Gate: 24/24 package tests pass from the vendored copy; tsc clean on tsconfig.node.json and tsconfig.web.json; ./node + ./node-discovery subpath exports resolve (NodeTcpTransport, NodeDiscovery). Plan correction in the same commit: two engine asks were withdrawn after checking the real build. Streaming/HTTP transfer (createFileRequestStreaming/Http, createFileCompleteStreaming, verifyFileIntegrity) and ACK (createFileAck) already exist, so large-model transfer is NOT blocked on the other lane — those are host-wiring rules this lane owns instead.
…e the engine directly Two things, both prerequisites for cross-device message sync. 1) CORE SCHEMA — rag_messages.uuid (src/main/database.ts) rag_messages.id is INTEGER AUTOINCREMENT and therefore DEVICE-LOCAL. Live sync keys records by (entity, entityId) across devices, so device A's row 7 and device B's row 7 would look like the SAME message and silently overwrite each other. The autoincrement id stays the local primary key; uuid is the cross-device identity. Includes a JS backfill for existing profiles (SQLite has no uuid()), a UNIQUE index so a replayed remote op upserts instead of duplicating, and uuid on every new insert. Mobile adds the equivalent to its message store under the same names. Verified by src/main/__tests__/rag-message-uuid.dbtest.ts against a REAL legacy profile on disk (4/4): the column appears, every pre-existing row is backfilled with a DISTINCT uuid, uniqueness is enforced, the production writer populates it, and the migration is IDEMPOTENT — rewriting uuids on each launch would orphan the record on every other device. 2) CORRECTION — stop vendoring @offgrid/sync M0 vendored shared/packages/sync into desktop/packages/sync, copying the existing @offgrid/clipboard|design|models|rag convention. shared/docs/DESKTOP_SYNC_INTEGRATION_PLAN.md §1 says explicitly NOT to duplicate this package: reference it directly, as mobile does. Now 'file:../shared/packages/sync'; the copy is removed and the plan doc records the correction. (The other desktop/packages/* copies have already silently drifted from shared/, which is the argument for the direct ref.) Full suite: 377 files / 3084 tests. The renderer integration failures seen while running this are load-dependent flakes, not regressions — a clean tree fails a DIFFERENT test, and all five pass in isolation with these changes applied.
341 statements at 0%. Every interesting question in this file is a question about SQL - which rows a project-scoped export includes, whether a restore skips a project that already exists, whether a document is recognised as already-present, what happens to a conversation whose project is absent - so it runs against a REAL engine (node:sqlite) using the app's own DDL. A hand-written matcher would answer those from its author's beliefs about the queries rather than from the queries. The app ships better-sqlite3-multiple-ciphers, which needs a native build for the runner's ABI. The only API this port uses beyond prepare() is transaction(), supplied over the real engine with the semantics better-sqlite3 documents (BEGIN, COMMIT on return, ROLLBACK on throw), so the SQL, row mapping, JSON handling and ordering are all genuine. What the tests pin down: chunks come back in position order, not insertion order (a document reassembled out of order is scrambled text); a project-scoped export must not hand over another project's chats or the unfiled ones, which is a privacy boundary rather than tidiness; a conversation-scoped export brings its project so the chat lands in the right place but not that project's documents; a corrupt context blob is dropped without failing the export, because the message text is still worth keeping. On restore: an existing project is left alone, so an old backup cannot silently revert one the user has since renamed; an already-held document is not duplicated, because restoring the same backup twice is a thing people do; a conversation whose project is missing comes back UNFILED rather than hidden or lost; and every restored row is announced to sync, without which a restored library would exist on this Mac alone and read as deletions to a paired device. Plus a full round trip - export, restore into an empty database, export again, identical. 18 tests. No production code touched.
…seam Both at 0%. system-status-ipc composes the one truth record the renderer gets, so the renderer interprets nothing itself. The case that carries the weight is a FAILED permission read: it must report 'down' with the reason, never 'denied', because telling a user to grant a permission they already granted hides a real fault behind an instruction they cannot act on. Also covered: each permission reported independently rather than rolled into one verdict (capture blocked while everything else works is the common real state), the runtime's own components surviving a permission failure, and both channels answering with the composed record rather than merely being registered. One test documents current behaviour rather than the behaviour I first assumed: a boundary that rejects with a plain object stringifies to '[object Object]' in a user-visible detail. It is cosmetic - macOS rejects with real Errors - so it is asserted as-is with a note, instead of writing a wish into the test or changing src for it. sync-knowledge-document is the open-core seam. The free build has no coordinator, so the hook is unregistered and indexing must behave identically; in a paid build the coordinator can throw while a peer is mid-handshake, and by then the document is already indexed locally. So a failure is swallowed AND logged with the mutation, and the seam does not latch off - the next change still goes out, costing one event rather than every event after it. 14 tests. No production code touched.
The health contract has ramGb and activeModel, not an 'overall' field - the fixture invented one, so the assertion that the runtime's own facts survive composition was checking a property that does not exist. It now asserts the real two. better-sqlite3's transaction() is a generic overload set that a plain function cannot satisfy structurally. Assigned through a record cast with the reason stated, since the port only ever calls it the one way the shim implements.
Two more files that were at 0%. sink.ts hands the finished archive to the user. The dialog is the OS and is the only thing faked; what the tests protect is the TEMPORARY archive, which holds a copy of the user's whole library. It is deleted after a successful save, after a CANCELLED save (the most likely path of all - cleaning up only on success would leave a copy behind every time), and even when the copy itself fails. One case covers the deliberate swallow: if something else is sitting in the containing directory, rmdir refuses and that failure is ignored, because removing the archive is this method's business and removing somebody else's file is not. Also both dialog shapes - sheet-attached to the focused window, and unparented when every window is closed, since passing a null owner to Electron throws. loadProFeaturesRenderer decides which half of the app switches on. Three outcomes and every way each fails: an entitled device gets all six registries (a missing one costs a whole surface silently), an unentitled device with bootstrap offered gets ONLY the pairing surface, an unentitled device without it gets nothing, an entitled device prefers full over bootstrap, and an activation that throws costs that surface rather than the window. Two absent-boundary cases too: no pro package at all (the open-core seam working, not an error) and no preload bridge yet. Worth noting for anyone editing that file: vitest evaluates a mock factory once per module graph, so the package is mocked per test in beforeEach. An unmock would have been worse than useless - it disables the module for every test that follows. 24 tests. No production code touched.
6.1% before. Two of the three permissions can be asked about directly; Local Network has no TCC API in Electron, so it is inferred by exercising the same multicast route Bonjour needs - which makes the ways that probe FAILS the substance of this file. All four are covered. A refused send is what a denied Local Network permission actually looks like, since no API says so. A socket 'error' with no handler is an uncaught exception in main, which would kill the app during setup - the least recoverable moment there is. A silent probe (no error, no reply: a network with no mDNS) is bounded at one second, or the Setup screen spins for ever. And the socket is closed on every path, because Setup polls this and a leaked descriptor per read adds up over a session. Also pinned: a status read asks about accessibility with prompt=false, so a background check never raises a dialog at the user, while requestAccessibilityPermission passes true - that argument IS the dialog. Screen recording counts as granted only for exactly 'granted', not 'restricted' or 'not-determined'. The capture prompt asks for a one-pixel thumbnail, because its purpose is to get the app listed rather than to capture, and a refusal returns false rather than throwing - the user saying no is an answer, not an error. And the Local Network settings link opens the PARENT privacy page on purpose: macOS 26 ignores the undocumented Privacy_LocalNetwork anchor, so the code asks for the destination that works and the setup card names the row. 19 tests. No production code touched.
0% before. A backup's whole value is that the user can trust what it says - 'Backup saved' when nothing was written is worse than an error, because they find out when they need it and it is not there. So this drives the real component through real clicks and asserts the words. The messages: the path it saved to, and a plain 'Backup saved.' when no path came back rather than 'saved to undefined'. A cancellation reported as a cancellation, including when the bridge answers nothing at all. An exact restore count with correct singulars, since 'Restored 1 chats' undermines the one message that matters. And a backup holding nothing new says so, instead of 'Restored 0 projects, 0 chats...' which reads as failure - restoring the same backup twice is normal. Failures are announced as role=alert with the reason the app gave (a screen reader interrupts, and 'it failed' gives the user nothing to act on), with a readable fallback when the boundary rejects with something that is not an Error. Two recovery cases: a new attempt clears the old error, so a stale red message cannot sit beside a completed restore contradicting it, and the buttons re-enable after a failure - without that finally clause a failed export would need an app restart to retry. Both buttons are disabled during either action, because an export and a restore at once means two things writing the same library. 14 tests. No production code touched.
12.8% before, and this file is the entire vocabulary the renderer has for talking to the machine. Every method is a thin forward, which is exactly why it needs testing: one that forwards nothing is a DEAD BUTTON. Nothing errors, no type complains, the UI just does not work - and the user finds out, not the build. So a sweep walks all 152 exposed functions and insists each one reaches main by some route (invoke, send, sendSync, or a listener call), naming any that do not rather than failing on a count. Writing it caught two things about the bridge worth knowing: proOff is removeAllListeners, so it silences EVERY subscriber on a channel rather than the caller's own - the unsubscribe returned by proOn is the one to use when two screens watch the same channel, and both are now pinned. The synchronous reads get their own cases because they decide, at preload time, whether the licensed half of the UI is reachable at all: isPro is strict-equality true, so an unregistered handler (undefined) or a truthy string cannot unlock the paid build. A failure to expose is logged and survived rather than taking the whole renderer down with a blank window. And the backup and cache channels are asserted against the shared contract constants rather than retyped strings, because a channel spelled in two places eventually differs in one of them - which presents as a silently dead feature. 13 tests. No production code touched.
A Record<string, fn> index is possibly-undefined under this tsconfig. Narrowed to the two keys the test actually calls, which also states at a glance what it is reaching for.
All three were excluded on the same stated grounds - wiring or native shells, covered by e2e rather than unit tests. Each now has its own tests, and that rationale no longer holds here at all: the e2e suite needs a machine to drive real windows, so anything relying on it for coverage is unmeasured in practice. - src/preload/** : the bridge sweep proves all 152 exposed methods reach main. Excluding it hid the one file whose failure mode is invisible to types and shows up only as a dead button in front of a user. - loadProFeaturesRenderer.ts : it decides which half of the app switches on at launch, which is a decision rather than passthrough. - permissions.ts : covered including the multicast probe's four outcomes, one of which (an unhandled socket error) would be an uncaught exception in main during setup. Diff coverage on desktop's changed files moves 51.6% -> 58.9% as a result, with no new tests - purely code that was already covered and not being counted. The .tsx blanket exclusion is left alone deliberately. Dropping it would add every renderer component to the denominator at once, which is a policy call rather than a measurement fix - so BackupRestoreSection's 14 tests still do not count towards the number, and that is stated rather than quietly worked around.
16 of this file's 42 branches were uncovered, and they are all about untrusted input: localStorage survives across versions, was written by older code, and a user can edit it. Every rejection path now has a case - a non-object, a null entry, an unknown type, a missing id, a non-string title, a missing message, an unparseable timestamp - plus the one that matters most operationally: the good records either side of a bad one still restore, so one malformed row costs that row rather than the whole bell. 'read' counts as read only when it was stored exactly true, because a truthy string would hide an approval the user has never seen. On the dedupe rules, which are what make the unread count mean anything: records sharing a key collapse to the current state rather than showing one thing's history as several unread items; different keys stay side by side; a key matches regardless of surrounding whitespace; and to-dos are never kept, since they already live in the to-do list. Adding is asserted not to mutate the array it was handed - that is React state, and mutating it is how a list updates without re-rendering. One test documents current behaviour rather than desired behaviour: a whitespace-only dedupeKey survives untrimmed (the conditional spread cannot remove what the first spread already set) and so acts as an identity, where an empty string does not. Minor and only reachable if a domain writes a blank key, so it is recorded with the mechanism spelled out instead of being fixed in src or wished away in the assertion. 26 tests. No production code touched.
9 of this file's 20 branches were uncovered, and the ones that matter are about a single fact: Electron does not wait for a before-quit promise. So one test asserts synchronously - nothing awaited - that every owner has already been ASKED to stop, because a helper kill or socket close that only happens after an await may never happen at all, and the next launch then finds the port taken by a dead app's leftovers. Order is the other rule: teardown reverses registration, so Pro capture stops before the Core socket it is using, and the four core resources come down downloads-runtimes-media-gateway. Failure paths, all of which leave a leak if wrong: a rejecting owner is named and reported while the others still stop, a SYNCHRONOUS throw is caught too (uncaught it would abort the remaining owners mid-loop), and teardown runs once no matter how often before-quit arrives - the same promise is handed back, since stopping a resource twice errors on an already-closed handle. The quit listener detaches itself BEFORE cleanup starts, so a second emission cannot start a second teardown. Three lifecycle edges that are easy to get wrong: an owner registering DURING teardown is stopped immediately rather than added to a list nobody will read, its failure is swallowed because nothing is left to collect it, and a stale unregister called twice must not deregister the NEW owner of a reused name - which would leave a live window's resources with nothing to close them. Relaunch: quit before spawning, or the replacement routes into the still-alive instance and the user gets a window backed by torn-down services. In development the whole npm command restarts (electron-vite kills its renderer server with the child, so Electron alone comes back pointing at a dead URL); a packaged build relaunches plain; and missing npm metadata falls back with a logged reason. 24 tests. No production code touched.
npm run test:db --coverage writes to coverage-db/ (its own directory so it cannot overwrite the default run's report). .gitignore already covered coverage/ but not this one.
…umber Desktop runs its tests in four places and each reported alone, so a file covered by one suite read as 0% in the others' reports and no single figure described the app. Two changes fix that. CI's e2e job now captures coverage. It already ran the full Playwright tour under xvfb - the instrumentation was simply never switched on there. OFFGRID_E2E_COVERAGE makes Node write V8 coverage for the main process and makes the vite build emit the sourcemaps that map it back to src/**.ts; both are gated on that variable, so a normal run and every shipped artifact are unchanged. c8 converts it and it uploads as an artifact. This repo is public, so those runner minutes cost nothing. The report is an artifact rather than a gate on purpose: remapped from the bundle, its statement map is whole-file - blank lines included - so it must never set a denominator. merge-line-coverage and new-code-coverage take it as --coarse and let it contribute covered lines only. npm run coverage:all runs the local suites, folds in an e2e report when one is available (--with-e2e locally, or E2E_COVERAGE_REPORT pointed at CI's artifact) and prints the four metrics over the lines this branch ADDS, for core and pro separately. The heavy projects are excluded by default and the reason is stated: three of their specs need engine binaries and real model files a dev machine usually lacks, and a suite that cannot run must not silently lower a number. What it shows, same code, measured properly - desktop core new code: 94.9% statements, 89.7% branches, 94.9% lines. Measured from the fast suite alone it read 80.6%/59.3%. desktop/pro goes 73.9% -> 80.3% statements and 46.2% -> 62.4% branches. Both suites also surface their own exit codes, so a green number from a red suite cannot be mistaken.
…lly added
The pre-push coverage gate ran ONE of three vitest projects and compared it to a floor calibrated on more, so it
had been failing on this branch regardless of what anyone did - 82.4% branches against 85%, which I verified both
with and without my recent config changes. Three separate problems:
1. Partial measurement, fuller floor.
2. A file covered by the DB journeys or the e2e tour read as 0% here, because those suites report separately.
src/main/database.ts and friends are excluded BY NAME for exactly that reason - a hand-maintained list that
silently rots as code moves.
3. Whole-file denominators: touch thirty lines of a two-thousand-line file and 1970 untouched lines land in the
number, so testing the new lines cannot move it.
Now each suite runs, keeps its report, and ONE gate measures the union against the lines this branch adds. The
e2e tour finally counts too - it was already running here, just never instrumented - and is folded in as coarse,
contributing covered lines but never a denominator, since its map comes from the bundle and includes blank lines.
The floors are a ratchet sitting a little under what the branch achieves, because the e2e step is advisory and
drops out when the model ports are busy. Same measurement, same code, read properly: core new code is 94.9%
statements and 89.7% branches with the tour folded in, 80.6%/59.3% without - against 85% asked of a number that
could only ever reach 82.4%.
The old floors in vitest.config.ts stay as they are; nothing was lowered.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Important Review skippedToo many files! This PR contains 140 files, which is 40 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (32)
📒 Files selected for processing (140)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…endency on CI could not resolve @offgrid/sync at all. The dependency is file:../shared/packages/sync - a SIBLING repo that arrived with the sync extraction (it is not on main in either app) and that nothing in CI ever checked out. It works locally only because ../shared happens to be there. One cause, four faces: TS2307 in typecheck, 26 no-phantom-deps errors from dependency-cruiser, and jest unable to locate <workspace>/../shared/packages/sync/src/index.ts. None of it was about the code in the diff - an empty test suite would have failed identically. actions/checkout refuses a path outside the workspace, so it lands in _shared and is moved up one level, which is exactly where the file: specifier and jest's moduleNameMapper both point. Then its own deps and build, because npm resolves the dependency to dist/ while jest maps to src/. Matching branch first, main as the fallback - the same shape as the pro checkout beside it. A PR that changes the app and the shared package together has to be tested against the package it expects, not against main's copy.
OFFGRID_E2E_COVERAGE writes raw V8 output to coverage-e2e-raw/ and c8 converts it into coverage-e2e/. Both are build artifacts of a measurement run, like coverage/ and coverage-db/ beside them.
The rename journey started failing on a strict-mode violation, not a regression: the chat list previews each conversation's last message, so the seeded message text is on screen twice - truncated in the history rail and in full in its bubble - and an unscoped getByText is ambiguous for a UI that is behaving correctly. The copy journey in the same file was already fixed this way and documents the reason; the rename journey was missed. Same treatment, same rationale. The absence assertion stays unscoped on purpose: the old title has to be gone from the rail AND the transcript. Verified on the 192.168.1.64 test box - both journeys in the file pass (15.6s). That takes the e2e tour back to the 7 known failures.
Merge rather than rebase: main is 15 commits ahead of the merge base and this branch carries 97, and the repo's convention is merge, never squash. 13 conflicts, all resolved: proCatalog.ts - both sides were wanted. Main added the per-feature `platforms` seam (the single source of truth for which platforms a Pro feature is live on) and declared the clipboard feature Windows-ready; this branch added the `devices` catalog entry. The resolution keeps main's field on clipboard AND this branch's entry, which now declares `platforms: ['darwin']` - required by main's interface, and correct: the sync engine is portable but pairing rides mDNS plus a macOS proximity route, and neither the Windows transport nor its discovery has been verified against a real second device, which is the bar that list encodes. UpgradeScreen.tsx - an import collision. Main replaced the `isMac` gate with a per-feature `currentPlatform()` check, so `isMac` is gone and this branch's `projectPersonalMeshActivationFailure` import stays. 11 e2e screenshots - took main's. They are regenerated artifacts for surfaces this branch did not touch (its own are the devices/sync ones); main's are the current renders. Verified: tsconfig.node, tsconfig.web and pro's own tsconfig all clean, and 4003 tests pass. Main's 42 platform-seam tests (proCatalog.lookup, UpgradeScreen.windows-notice, device) pass against the resolution, which is the real check on the proCatalog splice.
Correcting a gate I introduced while resolving the merge. Main added the per-feature `platforms` seam and made the field required; the `devices` entry I spliced in got `['darwin']`, which meant a Pro user on Windows opening Devices saw a "coming soon" placeholder instead of the screen. That was my conservatism, not a decision anyone made - before the merge the field did not exist on this branch, so there was no gate at all. Sync is cross-platform by construction. The transport is node:net and discovery is bonjour-service (pure-JS mDNS), so a Windows install gets the LAN route with no native code. There is exactly ONE `process.platform === 'darwin'` branch in the whole activation path (pro/main/sync-ipc.ts:188) and it only ADDS the Apple proximity route on top: macOS ends up with LAN plus proximity, Windows with LAN. WIN_PORTED in the catalog test grows with it - the list and the catalog are asserted against each other on purpose so availability and the record of what is ported cannot drift. NOT YET VERIFIED ON WINDOWS: no Windows machine has run pairing against a real second device. The architecture says it works and every gate here now says it is live; a run on Windows hardware is what would turn that into evidence. Flagged rather than buried.
The verify and e2e jobs both died on `sh: 1: tsup: not found` (exit 127) provisioning @offgrid/sync. shared is an npm-WORKSPACES monorepo: the lockfile and tsup live at the root, and packages/sync declares neither. Running `npm ci` inside the member failed (no lockfile there), fell through to `npm install`, pulled the member's five runtime deps, and left the build with no tsup. Installing at the root fixes it. Verified locally: after a root install, a member build resolves tsup through npm's parent-directory bin lookup and emits dist/ (exit 0). Same fix as mobile's, which had the identical step copied into four jobs.
Same as mobile's: a drifted lock file should degrade rather than break the run, and at the workspace root the fallback still provides tsup - which is why the member-level version it replaced was useless.
Five files, all of them naming the repo rather than depending on it: - scripts/physical-sync/README.md and iosMacKnowledgeSync.mjs pointed at provit/src/ios/launchWda.ts for the WDA server. That recipe now lives in the mobile repo as scripts/ios/launch-wda.mjs, so they point there and use WDA_URL rather than PROVIT_WDA_URL. - AGENTS.md and CLAUDE.md named it as the capture harness; now 'any device capture harness', since the rule about synthetic-only profiles was never specific to one tool. - stream-guards.ts named it as an example of a parent that captures stdout. Any e2e harness makes the point, and the EPIPE behaviour it guards is unchanged. Verified: 0 mentions left in all five repos, tsc clean, stream-guards' 7 tests pass.
@offgrid/sync is a file: dependency, so its own deps land in this lock. Fixing shared's lock file (five packages were committed there without regenerating it) changed what resolves here: @noble/hashes pins, c8 arrives as a dev dependency of the sync package, string_decoder is added. Verified with npm ci --dry-run against the committed tree, because a lock that disagrees with package.json is exactly the failure that took every consumer's CI down this afternoon.
Same review point as mobile's: `npm ci || npm install` masked a lock mismatch by resolving a different dependency graph, then let typecheck, tests and e2e run against dependencies nobody committed. The drift it existed for is fixed in shared, so both jobs now do a locked install and a future drift stops the build instead of quietly changing what is being tested.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
The desktop side of the sync work, ~100 new tests, and a coverage gate that measures what the suites actually do.
New-code coverage
Coverage of the lines this branch ADDS (not whole files it touched), via
shared/scripts/new-code-coverage.mjs:npm run coverage:allreproduces it;COVERAGE.mdat the workspace root has the ecosystem view.The coverage gate was structurally broken
The pre-push gate ran ONE of three vitest projects and compared it to a floor calibrated on more, so it had been
failing on this branch regardless of what anyone did — 82.4% branches against 85%, verified both with and without
the changes here. Three problems:
which is why
src/main/database.tsand friends are excluded BY NAME, a hand-maintained list that rots.testing the new lines cannot move it.
Now each suite runs, keeps its report, and one gate measures the union against the added lines. The e2e tour
finally counts — it was already running in the hook and in CI, just never instrumented. CI captures it too and
uploads it as an artifact (free: this repo is public).
One product change, requested
The app opened at 900x670. It now opens filling the work area, maximized before first paint. At the old size the
Models grid collapsed to one card per row and every screen read as a stretched phone layout. Not fullscreen — that
would move it to its own macOS Space. Covered by a smoke assertion against the work area rather than a fixed size.
Test findings worth reading
.partfile handles. Node now treats a GC-closed handle as a fatal uncaught exception,which silently killed the ENTIRE coverage report while the suites themselves passed.
created_at, and the rendererdeliberately drops any message it cannot order, so seeded conversations rendered as nothing.
React cleanup and surfaced as an unrelated navigation test failing.
onNewActionhas no subscriber whilepro/main/crm/actions.tsstill emits it. Deliberate (57a3e7d removedit so the bell counts only what needs a decision — DayView lists to-dos), so the test now asserts the ABSENCE.
Still open, needing a decision
control-center.tsbuilds the only rows that can readconnectedfrom licence-registry installations alone, so apaired, actively-connected device missing from the registry snapshot can never report connected — and that
snapshot is empty whenever the licence provider is unreachable, i.e. offline. Two db tests are excluded with the
reason recorded in
vitest.db.coverage.config.ts; the fix is a src change.