Skip to content

feat(upload): info notice when notify is unset on first upload#77

Merged
rubenhensen merged 1 commit into
mainfrom
feat/upload-silent-default-notice
May 24, 2026
Merged

feat(upload): info notice when notify is unset on first upload#77
rubenhensen merged 1 commit into
mainfrom
feat/upload-silent-default-notice

Conversation

@rubenhensen
Copy link
Copy Markdown
Contributor

Summary

Closes the one failure mode neither TypeScript nor validateUploadOptions can catch: a caller who simply forgets to set notify on sealed.upload() and silently gets no recipient email.

Since notify is optional in UploadOptions, omitting it type-checks cleanly — the user only finds out via a support ticket. The validator (#76) catches wrong-shape misuse but cannot catch no shape at all.

This commit emits a one-time console.info on the first sealed.upload() call per PostGuard instance where notify is undefined:

[@e4a/pg-js] sealed.upload(): notify is unset — uploading silently
(no recipient email sent). Pass { notify: { recipients: true } } to
email recipients, or { notify: { recipients: false } } to make the
silent intent explicit and suppress this notice.

Behaviour

  • Logged via console.info (informational, not a warning).
  • Tracked per PostGuard config via WeakSet so long-running processes get one notice, not one per upload.
  • Suppressed by passing notify explicitly — either { recipients: true } (email) or { recipients: false } (silent, but with intent). Both signal "I know what I'm doing."

Test plan

  • npm test — 111/111 passing (2 new tests in tests/postguard.test.ts)
  • npm run typecheck clean
  • npm run build clean
  • Manual: ran live smoke against staging; notice fired once, upload succeeded with real UUID

Closes the one failure mode neither TypeScript nor validateUploadOptions
can catch: a caller who simply forgets to set `notify` and silently gets
no recipient email. `notify` is optional in `UploadOptions`, so leaving
it out type-checks cleanly — the user only finds out via a support
ticket that "the SDK didn't send my email".

Logs once per PostGuard config (tracked via WeakSet) so long-running
processes don't flood. Suppressed by passing `notify` explicitly:
`{ notify: { recipients: true } }` to email, or
`{ notify: { recipients: false } }` to keep silent intent explicit.
@rubenhensen rubenhensen merged commit e11942d into main May 24, 2026
5 checks passed
@rubenhensen rubenhensen deleted the feat/upload-silent-default-notice branch May 24, 2026 13:44
@github-actions
Copy link
Copy Markdown

🎉 This PR is included in version 1.10.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link
Copy Markdown
Contributor

@dobby-coder dobby-coder Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review

Small, focused PR that does what it claims: emit a one-time console.info when sealed.upload() is called with notify unset. Tests pass (111/111), typecheck clean, build clean. The change is well-scoped and the design (WeakSet keyed by config, fires before pipeline work, suppressed by any explicit notify) is sensible.

That said, several real rough edges:

  1. The notice fires before the ReadableStream payload guard, so a misconfigured upload that's about to throw still prints a misleading 'uploading silently' notice and burns the once-per-config slot. (inline)
  2. The PR description claims 'per PostGuard instance' but the WeakSet keys by config object, so two PostGuard instances built from the same config literal share the notice — divergence between intent and behaviour. (inline)
  3. The pre-existing accepts undefined opts test (tests/postguard.test.ts:75, outside this PR's diff) calls sealed.upload() with no opts and no console.info spy. With the new code, the real notice now prints to stdout during every test run — confirmed via npx vitest run --reporter verbose. It also adds the top-level test pg config to the module-level WeakSet permanently for the duration of the test process, creating implicit ordering between unrelated describe blocks. Fix: either silence console.info in that test, move the silent-default notice tests into their own test file with their own pg, or expose a test hook to reset silentDefaultNoticed.
  4. No test that two different fresh PostGuard instances each get their own notice — would catch a regression where the WeakSet is replaced with a module-scoped boolean. (inline)
  5. createEnvelope interaction (cross-cutting, outside this PR's diff): src/email/envelope.ts:62-63 calls sealed.upload(uploadOpts) with uploadOpts === undefined whenever the caller doesn't pass notify or onUploadInit. That means every createEnvelope() caller without an explicit notify now triggers the new notice — and the message string says sealed.upload(): notify is unset, which will read as confusing for high-level API users who never directly called sealed.upload() and don't know what to do with { notify: { recipients: true } } in the envelope context. Consider either (a) plumbing an option through createEnvelope to suppress the SDK-internal notice, or (b) rewording the message to be neutral about the call site (e.g. [@e4a/pg-js] uploading silently (no recipient email sent) — pass notify to email recipients).

None are showstoppers but the ordering issue, the test-pollution, and the createEnvelope interaction are concrete and worth fixing.

Test results: npm test 111/111 passing (includes 2 new tests); npm run typecheck clean; npm run build clean (1.04 MB dist/index.mjs). Verbose vitest run confirms the real notice prints to stdout from the pre-existing accepts undefined opts test.

Rule compliance

Checked against the dobby-memory rule set and the postguard-js repo notes.

  • conventional-commit-pr-titles — clean (feat(upload): ...).
  • default-prs-to-ready / flip-drafts-after-ci — clean (opened ready).
  • promised-vs-delivered — see code-review finding 2: PR body says 'per PostGuard instance' but implementation keys by config object. Either tighten the body to 'per PostGuard config' or change the key.
  • tests-required-on-fixes — satisfied; both positive and negative cases covered. Coverage gap (two fresh instances each logging) flagged separately as code-review finding 4.
  • vitest-mock-hoisting — clean; tests use inline vi.spyOn(console, 'info'), no hoisted-factory pitfalls.
  • postguard-js repo notes (signing-keys / createEnvelope) — the notes already document that createEnvelope calls toBytes() then conditionally upload(). With this PR, that upload path becomes a notice-emission path for any caller of createEnvelope who doesn't set notify — see code-review finding 5.
  • All other org-level / process / infra rules (branch cleanup, board sync, multi-org, sveltekit, WCAG, etc.) — not applicable to this diff.

Note: the PR is already merged. Findings above are still worth a follow-up commit; the test-pollution and createEnvelope ones in particular are concrete.

Comment thread src/sealed.ts
// config) to avoid spamming long-running processes; suppress by
// passing notify explicitly (true OR false counts as an explicit
// choice).
if (opts?.notify === undefined && !silentDefaultNoticed.has(this.config)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Code review] The silent-default notice fires before the ReadableStream guard at line 107. If a caller passes data: ReadableStream with no notify, this block runs first: it prints [@e4a/pg-js] sealed.upload(): notify is unset — uploading silently AND consumes the WeakSet slot, then immediately throws TypeError about ReadableStream. Two consequences: the user is told their upload is going silently when it actually errored and uploaded nothing; and any later, well-formed call on the same config will no longer see the notice (slot already used). Move the notice below the ReadableStream guard (and ideally below all synchronous precondition checks) so it only fires when the upload is actually about to start. Same argument applies to any future precondition added between line 84 and the actual encryptPipeline call.

Comment thread src/sealed.ts
* notice, so a long-running process logs it once rather than on every
* upload. Keyed by config object so each `new PostGuard(...)` gets one
* chance to be told. */
const silentDefaultNoticed = new WeakSet<PostGuardConfig>();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Code review / Rule: promised-vs-delivered] PR description says 'tracked per PostGuard config via WeakSet so long-running processes get one notice, not one per upload' and 'one notice per PostGuard instance' — but the WeakSet is keyed by the config object reference, and PostGuardBase stores the config by reference without cloning (postguard-base.ts:21). So const cfg = {...}; new PostGuard(cfg); new PostGuard(cfg) results in the second PostGuard never logging — the two instances share the slot. Likely an acceptable edge case in practice, but the JSDoc here ('each new PostGuard(...) gets one chance to be told') is literally wrong; it's one chance per config identity. Either tighten the comment and PR body to say 'per config object', or key the WeakSet by something tied to the PostGuard instance (or move the boolean onto a private field of PostGuardBase).

Comment thread tests/postguard.test.ts
});
});

describe('silent-default notice', () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Code review] Test coverage gap: there is no test that asserts two different fresh PostGuard instances each get their own notice. Both new tests use a single instance (or set notify explicitly on the second instance). A regression where someone replaces the WeakSet with a module-scoped boolean (let noticed = false) would pass both tests. Add a third case: build two fresh PostGuards, call .upload() on each with no notify, assert console.info was called exactly twice. Also worth covering: the same instance calling .upload() with explicit notify after an unset upload (should not double-log).

Comment thread src/sealed.ts
// config) to avoid spamming long-running processes; suppress by
// passing notify explicitly (true OR false counts as an explicit
// choice).
if (opts?.notify === undefined && !silentDefaultNoticed.has(this.config)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Code review — nit] opts?.notify === undefined treats notify: {} (empty object) as 'explicit' and suppresses the notice. An empty notify object behaves identically to omitting notify (no recipients emailed, no sender confirmation), so the user gets the same silent footgun but no warning. Either tighten the condition to also fire when notify is set but has no actionable keys, or document this corner case. Low impact — most callers won't construct { notify: {} } accidentally.

Comment thread src/sealed.ts
if (opts?.notify === undefined && !silentDefaultNoticed.has(this.config)) {
silentDefaultNoticed.add(this.config);
console.info(
'[@e4a/pg-js] sealed.upload(): notify is unset — uploading silently ' +
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Code review — nit] The log string uses a Unicode em dash () and curly punctuation. Most modern terminals handle this fine, but some legacy log aggregators / non-UTF-8 stream destinations mangle it. Minor — the existing validateUploadOptions error messages do the same, so consistent with codebase style.

Comment thread src/sealed.ts
// choice).
if (opts?.notify === undefined && !silentDefaultNoticed.has(this.config)) {
silentDefaultNoticed.add(this.config);
console.info(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Code review — nit] This is the SDK's first non-WASM-shim console.* call (grep confirms: only the wasm-bindgen-generated shim emits console output otherwise). Adding library log output is a real product decision — some users embed pg-js in apps with forwarded console output and won't appreciate an opinionated info line they can't silence except by passing notify. The message itself documents the workaround ({ notify: { recipients: false } }), so this is acceptable, but worth confirming the maintainer is OK with pg-js being a library that logs to console at all going forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant