🏗️ PUT-1761: tell the members when something is shared with the workspace

[jfcastro92]

Closes PUT-1761. Stacked on #3729.

The bug

Sharing with a workspace notified nobody, and said it had succeeded.

ShareNotificationService.notifyShared keys every map off ResolvedShare.holderId — one user id, documented as "who to notify". A workspace share has no individual holder: the grant is a single row against the group, and holder_user_id on the index row is NULL. So counts, named and targets all skipped it and the call returned normally.

Nothing errored. The share was created correctly, resolved correctly for every member, and appeared in their listings. The failure mode was silence, which is why it needs a test that counts notifications — nothing else catches it.

The fix

#recipientsOf() expands a workspace share to its live members at announcement time. The grant stays one row; only the telling fans out.

Because #announce and #emailHolder were already inside the same per-recipient loop, expanding the recipients fans out both channels — in-app notification and email digest — with no separate email path.

TeamStore.listMemberIdsByGroupId() is the lookup, scoped to live workspaces.

The anti-spam decision

PUT-1761 listed three options and asked for a deliberate choice. Option 1: expand per member, and let the existing per-recipient budgets (10/hour, 50/day) plus the digest absorb it. Those budgets were designed for exactly this shape.

The fan-out is bounded at NOTIFY_FANOUT_CAP = 500 — far above the default 50-seat cap, so it bounds a pathological workspace rather than a real one — and it logs when it bites instead of truncating silently. A shortened fan-out otherwise reads as "everyone knows".

Wording deliberately unchanged

The ticket originally asked for the notification to name the workspace ("alice shared 3 items with Acme Design"). That was built and then reverted on review: it needed a via field threaded through ShareSender, mergeShareSender, DigestEntry and digestSubject, plus a rule for what to say when one person receives a direct share and a workspace share inside the same grouping window.

shareNotifyTitle.ts is therefore unchanged in this PR — the wording stays "shared N items with you" in both channels. The ticket has been updated to record that.

Tests

TeamShareNotify.test.ts, five tests on the two-workspace fixture:

✓ tells every member of the workspace
✓ emails every member too, not just the in-app notification
✓ does not tell the issuer about their own share
✓ does not tell members of a workspace it was not shared with
✓ does not retroactively tell a member who joins afterwards

The email leg uses a sendRaw spy, so the digest renders for real and nothing leaves the process.

Falsified, not just run. Disabling the expansion reproduces the original silent skip:

 × tells every member of the workspace
 × emails every member too, not just the in-app notification
Tests  2 failed | 3 passed (5)     <- expansion disabled
Tests  5 passed (5)                <- restored

Worth noting the three negative tests pass either way — they are trivially true when nobody is told. That is the trap this ticket warns about, and it is why the two positive tests are the ones carrying the coverage.

Full suite: 7530 passed | 26 skipped. Typecheck clean.

Note for review

listMemberIdsByGroupId is a new unbounded-by-default read on a notification path. It takes an explicit limit with a documented cap rather than relying on the seat cap, because max_seats_per_workspace is configurable and a deployment that raises it should not silently turn one share into thousands of sends.

[github-actions]

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	93.79% ⬇️ -0.01%	29022 / 30941
🔵	Statements	91.82% 🟰 ±0%	31513 / 34320
🔵	Functions	90.53% ⬆️ +0.04%	5344 / 5903
🔵	Branches	80.75% ⬆️ +0.01%	20844 / 25812

File Coverage

File	Stmts	Branches	Functions	Lines	Uncovered Lines
Changed Files
src/backend/services/share/ShareNotificationService.ts	85.91% ⬇️ -0.70%	78.91% ⬆️ +0.55%	94.59% ⬆️ +0.66%	88.46% ⬇️ -0.47%	57, 88, 90-92, 101, 114, 128, 182, 204, 212-217, 222-227, 247, 250-251, 322, 325, 388, 414-415, 436-437, 461, 472, 526, 544, 565-568, 584-587, 592, 599-603, 666-678, 694, 715-716, 728
src/backend/services/team/TeamService.ts	84.87% ⬆️ +0.62%	72.44% ⬇️ -0.36%	95.23% ⬆️ +0.11%	86.63% ⬆️ +0.65%	90-91, 111-112, 133, 201-203, 206-208, 217-229, 258-261, 283-294, 344-347, 355, 361, 398-401, 453-455, 509, 593-596, 642-644, 664
src/backend/services/user/UserAccountService.ts	93.61% ⬆️ +0.28%	84.61% ⬆️ +1.28%	100% 🟰 ±0%	97.29% ⬆️ +0.07%	71, 175, 178
src/backend/stores/team/TeamStore.ts	97.43% ⬇️ -0.76%	86.73% ⬆️ +0.66%	100% 🟰 ±0%	97.88% ⬇️ -1.15%	255, 281, 411, 460

Generated in workflow #1596 for commit 212f7a5 by the Vitest Coverage Report Action

[jfcastro92]

Review fixes

A code review across the three phase 4 PRs raised 13 issues. All 13 are fixed; three land here.

A blocked member was still told

#shareWithTeam never calls #assertNotBlocked, which the user-to-user path applies at ShareService.ts:901 (blocksAllShares(holder) || userBlock.isBlocked(holder, issuer)). Combined with this PR's fan-out, a member who had blocked the sharer — or who set "accept no shares from anyone" — received the notification and the email.

The check belongs per-recipient, not per-share: the grant is one row against the group and cannot exclude a member, but the contact is exactly what a block refuses. #recipientsOf now filters through #unblocked(), which stops both channels. It fails closed — a block-check error drops the recipient rather than announcing to someone who refused it.

Left deliberately: the member still holds the grant and can still reach the file. Excluding one member from a group grant is not representable without per-member exceptions, which this design does not have. Worth a reviewer's opinion on whether that is the right line.

The fan-out cap was off by one, and re-read per item

• members.length >= NOTIFY_FANOUT_CAP warned "some members were not told" for a workspace of exactly 500, where LIMIT 500 had truncated nothing. The store now reads one past the cap so truncation is genuinely detectable, the caller compares with >, and — this matters — slices back to the cap afterwards. Detecting truncation and enforcing it are not the same thing; without the slice it would notify one member beyond the limit.
• listMemberIdsByGroupId ran once per share row, so sharing N items with one workspace issued N identical queries. Now cached per group for the call.

Tests — all falsified

 × does not announce to a member who blocked the sharer
 × reads the member list once however many items are shared

⚠ The block test was vacuous twice before it worked, and both reasons are traps for anyone testing notifications here:

1. It first asserted a positive control on email delivery. By that point in the file the per-pair interruption budget is spent, so no digest opens and the assertion could never pass — the log says interruption budget spent, no digest open. Notifications are still written when the budget is spent (that is the documented split between what a notification says and whether it may interrupt), so the assertion moved there.
2. It then counted notification rows for an existing seat. Notifications fold into an open row rather than adding one, so the count cannot move for a seat already notified earlier in the file. It now creates a fresh member.

Full suite 7732 passed | 26 skipped, typecheck clean.

Rebased onto current main, which advanced 9 commits during this work — including PUT-1679/1681/1684/1685, the notification and events-surface changes this PR touches directly. The rebase was clean and the suite absorbed their tests.

[jfcastro92]

Local validation (post-review)

Engine: mysql 8 · teams_enabled: true · real SMTP to mailhog, no spy. Fresh database, one workspace with three activated, email-confirmed seats, plus a non-member.

The unit tests use a sendRaw spy, so real delivery, the digest's batching and the wording a member actually receives were all unproven until this run.

Both channels reach every member, and nobody else

One share with the workspace, then the notification rows:

username         notifications
p4own80144            0     <- the issuer
p4out80144            0     <- non-member
p4seat1x80144         1
p4seat2x80144         1
p4seat3x80144         1

And the mail actually delivered:

['p4seat1x80144@example.test'] -> p4own80144 shared union.txt with you
['p4seat2x80144@example.test'] -> p4own80144 shared union.txt with you
['p4seat3x80144@example.test'] -> p4own80144 shared union.txt with you

The wording is "with you", as decided

{"type": "share.received",
 "title": "p4own80144 shared 3 items with you",
 "fields": {"count": 3, "senders": [{"count": 3, "username": "p4own80144"}], ...}}

The workspace is deliberately not named — that requirement was dropped, and shareNotifyTitle.ts is unchanged in this PR. This confirms the revert was complete: had any part of it survived, the title would read "with Acme Phase4".

The anti-spam property, which is why option 1 was acceptable

Look at count: 3 above. Three separate shares produced one notification per seat, folded, rather than three. That is the folding the budget argument rests on, and it is the thing a unit test with a stubbed notification layer cannot show.

Per-seat email came out at one message per interruption window rather than one per share, for the same reason.

---

⚠ Not exercised in this run: NOTIFY_FANOUT_CAP. It is 500 and the fixture has 3 seats, so nothing approached it. Reaching it would mean provisioning 500 accounts or temporarily lowering the constant; I did neither, and would rather say so than imply coverage. The cap's off-by-one and the slice-back-after-warning are covered by unit tests.

Also not re-exercised here: the per-recipient hourly budget (10/hour). Three seats over a few shares stays well inside it.

Full suite 7733 passed | 26 skipped, typecheck clean.

[jfcastro92]

Rebased onto main (6d7be8bd6), which landed the KV share-grant series (#3690–#3693).

One collision worth naming: those commits took migration 0080 / mysql_mig_35 / postgres_mig_24 — the exact three numbers this stack's temp_password_expires_at migration held. Renumbered to 0081 / mysql_mig_36 / postgres_mig_25, registry entry [76, ['0081_temp-password-expiry.sql']], and CURRENT_SCHEMA_VERSION is now 77. Main's three files are byte-identical to what it merged; only ours moved.

Verified per commit rather than at the tip — each of the 9 typechecks on its own, so no intermediate commit is broken. Full suite 7883 passed | 26 skipped.

[Salazareo]

definitely needs caching

[Salazareo]

feel free to merge after caching fixes

[jfcastro92]

Terminology: "workspace" is now "team", everywhere

The feature said puter.teams in the namespace and "workspace" in every string, comment and error code. That divergence was deliberate at design time; it is now settled the other way, and the whole stack says team.

Applied to every commit rather than added on top, so each PR shows its own code already named correctly and no intermediate commit references a name a later one changes. All 13 commits still typecheck individually.

The contract-bearing renames

These were free to change now and expensive after release, which is why they went in this pass:

Before	After
not_the_workspace_owner	not_the_team_owner
workspace_limit_reached	team_limit_reached
workspace_deleted	team_deleted
max_workspaces_per_user	max_teams_per_user
max_seats_per_workspace	max_seats_per_team
disabled_by_workspace	disabled_by_team
createWorkspace / updateWorkspace / deleteWorkspace	createTeam / updateTeam / deleteTeam

Plus twoWorkspaces.ts → twoTeams.ts, shareWorkspaces.js → shareTeams.js, the teams-workspace-* CSS classes, and the team_workspace_deleted email template, which the mechanical pass turned into the stutter team_team_deleted and is now team_closed.

Scoped, and checked

"workspace" means other things in this repo. These were deliberately left alone, and verified untouched:

• kvShares.ts and the KV integration tests, where workspace:abc: is a key prefix in an example, not a team
• Events.md and KV.md, which use it the same way
• UIPermissionDialog tests, same
• package.json / webpack/BaseConfig.cjs / .gitignore, where it is npm workspaces

Audited three ways: every changed line mentions team or workspace (no collateral edits), no stutters or mangled words (team_team, teamspace, …), and both old config keys are gone with the new ones consistent across all 12 sites that read them.

One thing not renamed

The git branch names still say workspace (put-1740-1762-workspace-gui). Renaming a branch changes a PR's head ref and closes the PR, so they stay as they are — they are refs, not product surface.

[jfcastro92]

definitely needs caching
feel free to merge after caching fixes

Done. listMemberIdsByGroupId and getOrgSeat are now cached in Redis on a 60s TTL.

What is deliberately not cached: isMember and getByUid. Those are the hot reads — every team route calls one — but they are also the authorization reads, and a stale entry there means a removed member keeps access rather than getting one stray notification. Adding an authz cache late, without the review attention that deserves, is the wrong trade. Happy to do it as a follow-up if you want the win; it wants its own PR.

The part that took the thought

jct_user_group.user_id is ON DELETE CASCADE, so deleting an account changes membership without going through the store that caches it. A TTL alone gets that wrong for up to a minute.

An existing test caught it immediately — leaves the membership unreadable after the delete it was captured for went red as soon as the cache went in, because cascadeDelete had populated team:seat:<id> via captureSeatForBilling and nothing busted it. So the invalidation sits in UserAccountService.cascadeDelete, next to the emit that was already there, rather than in one caller:

this.services.team.emitSeatDeleted(seat);
if (seat) await this.services.team.forgetSeat(seat);

Explicit busts also on addMember, removeMember and softDelete. The TTL is the backstop for anything that reaches the table another way, not the primary mechanism.

Verification

New test walks a roster through add → cascade-delete and asserts the list tracks it. Falsified both ways:

Broke	Failed
the cascade bust (forgetSeat)	3
the addMember bust	1

Backend 7893 passed | 26 skipped, typecheck clean, all 13 commits typecheck individually.

[jfcastro92]

Follow-up: the two authorization reads are cached now too

My earlier reply said isMember and getByUid would stay uncached because a stale entry there is access, not a stray notification. That was a statement about the risk, not about whether it could be done safely — so they are cached now, with the invalidation made exhaustive rather than the read left slow.

They are the hottest reads in the feature: every team route resolves the team through getByUid and authorizes through isMember.

Every path that can change either value busts it:

Path	Busts
addMember / removeMember	the membership pair
update	the team row — name, handle and directory_enabled all live on it
softDelete	the row and every pair over it, read while still live, bounded by the fan-out cap
cascadeDelete	the pair — ON DELETE CASCADE changes membership without touching this store

The soft-delete case is the one that needed thinking about. Busting "every membership of this team" cannot be done by key pattern without a SCAN, so the member ids are read before the deleted_at write, while the team still resolves, and each pair is busted after. Bounded, and it runs once per deletion.

I also checked the weaker argument I could have leaned on: both isMember callers happen to resolve the team first and 404 if it is gone, so the soft-delete case is already covered by caller discipline. That is true today and is exactly the kind of invariant a future caller breaks silently, so the pairs are busted explicitly regardless.

Falsified individually — each bust removed on its own, only its own case fails:

pair bust on add/remove      -> 1 failed
row + pair bust on softDelete -> 1 failed
row bust on update            -> 3 failed
pair bust on cascade          -> 1 failed

Four new tests, one per path, plus the roster test from before. Backend 7897 passed | 26 skipped, typecheck clean, all 13 commits typecheck individually and every test file parses at every commit.

⚠ Placement worth knowing: these three methods (getByUid, getMembership, isMember) are main's code from phase 2, so by the usual rule the change would go in the lowest open PR. It is here instead because the soft-delete bust calls #readMemberIds, which this PR introduces — putting it lower would leave an intermediate commit referencing a method that does not exist yet. Keeping all the caching and all the invalidation in one PR also means a reviewer sees the whole mechanism at once.