Repository navigation
[simplex-desktop]: add end-to-end operator push notifications - #1307
seunlanlege merged 5 commits into
Conversation
96b5dca to
4daab16
Compare
seunlanlege
left a comment
There was a problem hiding this comment.
Reviewed the full diff plus the surrounding code. 14 comments inline — the first few are the ones I'd want addressed before merge; the tail is cleanup and docs.
Holds up well. desktopNotificationUrl correctly rejects https://evil, //evil/ and simplex://remote/ before anything reaches loadURL, and simplex://local/orders resolves through handleSimplexProtocol's SPA fallback so desktop click-through lands on the right tab. Push payloads stay encrypted end-to-end, the SSE stream and receipt endpoint sit behind the existing host/CSRF/provenance guards, and the acknowledge POST carries X-Simplex-UI. Every write goes through patchRuntimeState, so notification state will not clobber paused.
Not a comment, just a note: neither simplex nor simplex-desktop gets a version bump here (both stay at 0.16.2). That matches what #1268 and #1291 did, so I left it alone — flagging in case it was meant to move. workspace-policy.test.ts asserts the two match, so they would go together.
I did not run the suites locally (no node_modules in the review worktree). The new tests read sound, but nothing covers the wrong-error-message path or the partial-fill dedup case.
seunlanlege
left a comment
There was a problem hiding this comment.
Two follow-ups on the fixes in 246be7c — both small, neither blocking. Everything else I raised looks properly addressed, and I've resolved those threads.
Nice touches beyond what was asked: the error listener in openSseStream closes a latent write-after-end crash path on the SSE responses, and the ensureReady() retry cannot double-register listeners since every throw point in initialize() precedes the on(…) calls.
Unrelated to notifications: 002a523 ("add real-time private key validation and align status icons", issue #1231) is riding along in this branch — Wizard/Signer/Field/setup-controls plus its own changelog note. Worth splitting out if it wasn't deliberate.
seunlanlege
left a comment
There was a problem hiding this comment.
LGTM — all 14 review findings addressed across 246be7c and 4334193, every thread resolved.
Verified locally: tsc -p on base vs head introduces no new type errors (the only delta is web-push, which just isn't installed in my worktree), and the branch incidentally fixes a pre-existing bin/simplex.ts signature error. The Pick<BalanceProvider, "getSnapshot" | "on" | "off"> contract type-checks against the new emitter-backed test doubles.
Two non-blocking notes for whenever: 002a523 ("real-time private key validation", #1231) is unrelated to notifications and would be cleaner as its own PR, and neither simplex nor simplex-desktop gets a version bump here.
|
@ddboy19912 needs rebase |
There wasn’t an actual Git conflict. GitHub was using stale PR-stack metadata and incorrectly marked the PR as needing a rebase. I removed the stack association and refreshed the unchanged base branch; GitHub now reports the PR as clean and mergeable. |
Pull Request is not mergeable
Pull Request is not mergeable
Summary
Closes #1226 and #1231
Adds end-to-end operator notifications for the Simplex PWA and desktop app.
Features
Verification
git diff --checkpassed.No commit or push has been performed.