Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 0 additions & 14 deletions client/page.js
Original file line number Diff line number Diff line change
Expand Up @@ -201,20 +201,6 @@ class MainPage extends RuntimeEnv {
for (let m of ["runtime", "core", "sprite", "locale", "main"]) {
bundles[m] = data.app.manifest[`${m}.js`]
}
// The app's stylesheet, when the UI build emits one.
//
// The UI used to ship every rule inside the JS (webpack style-loader) and
// inject ~92 <style> tags at runtime, so this page linked no app CSS at
// all. Chrome consults one RuleSet per stylesheet for every element it
// restyles, which made a single style recalculation cost 12-30us per
// element instead of well under 1us. The UI now extracts one merged
// `styles.<hash>.css` and lists it in the same manifest these bundles
// come from.
//
// OPTIONAL ON PURPOSE, so this server is safe to deploy on its own: a UI
// build that predates the change has no `styles.css` key, nothing is
// linked, and the runtime injection keeps working exactly as before.
data.styles = data.app.manifest["styles.css"] || null;
} else {
for (let m of ["core", "sprite", "locale", "entry"]) {
bundles[m] = data.app[m]
Expand Down
3 changes: 0 additions & 3 deletions client/templates/index.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,6 @@
<link rel="icon" href="<%= icon %>?v=3" type="image/svg+xml">
<link rel="stylesheet" href="/-/static/styles/loader.css?v=1.0.3">
<link rel="stylesheet" href="/-/static/fonts/Armin-Grotesk/stylesheet.css">
<% if (typeof(styles) !== "undefined" && styles) { %>
<link rel="stylesheet" href="<%= styles %>">
<% } %>

<script>
<%= renderer.include('bootstrap.js.tpl') %>
Expand Down
45 changes: 44 additions & 1 deletion service/lib/notify-member-joined.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,47 @@
}
}

module.exports = { notifyMemberJoined };
/**
* Tell every online member of a hub that EXISTING memberships changed — a role
* was set (hub.set_privilege) or members were removed (hub.delete_contributor)
* — so open permission matrices refetch.
*
* Both services already push to the member being changed, and only to them:
* that drives the changed member's own windows. The matrices belong to
* everybody ELSE looking at the list, and before this nothing reached them —
* an admin kept seeing the old role, or the removed member, until they
* reopened the panel.
*
* One push per request, sent after the per-member writes, so a refetch reads
* committed rows. The acting admin receives it too and simply refetches what
* they already drew — no echo guard, for the reason given on
* notifyMemberJoined above.
*
* Never throws: every call site has already committed the change.
*
* @param {object} svc service instance (needs .yp, .payload, .warn)
* @param {string} hub_id hub whose membership changed
* @param {object} change
* @param {string} change.change "privilege" | "removed"
* @param {string|string[]} change.users the affected member ids
*/
async function notifyMembersChanged(svc, hub_id, { change, users } = {}) {
if (!svc || !hub_id) return;
try {
const dest = toArray(await svc.yp.await_proc("entity_sockets", hub_id));
if (isEmpty(dest)) return;
await RedisStore.sendData(
svc.payload(
{ hub_id, change, users: toArray(users) },
{ service: "hub.members_changed" }
),
dest
);
} catch (e) {
if (svc.warn) {
svc.warn("[notifyMembersChanged] failed for hub", hub_id, e && e.message);

Check warning on line 94 in service/lib/notify-member-joined.js

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Prefer using an optional chain expression instead, as it's more concise and easier to read.

See more on https://sonarcloud.io/project/issues?id=drumee_server-team&issues=AaCeBqjBuNydAlqVZsH7&open=AaCeBqjBuNydAlqVZsH7&pullRequest=218
}
}
}

module.exports = { notifyMemberJoined, notifyMembersChanged };
16 changes: 15 additions & 1 deletion service/private/hub.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const {
ID_NOT_FOUND,
} = Constants;
const { resolve } = require("path");
const { notifyMemberJoined } = require("../lib/notify-member-joined");
const { notifyMemberJoined, notifyMembersChanged } = require("../lib/notify-member-joined");
const { butlerFrom } = require("../lib/mail-sender");
const { mailFailure } = require("../lib/mail-result");
const { resolveHubInviteName } = require("../lib/hub-invite-name");
Expand Down Expand Up @@ -2661,6 +2661,13 @@ class __private_hub extends Hub {
// Avg team size has to fall when people leave, not only rise when they
// join — a rollup refreshed on one side only climbs forever.
await this._trackWorkspaceMembers(hub_id);
// media.remove and hub.member_removed above went to the removed members
// only. Every remaining member with the permission matrix open still
// showed them; tell the hub, last, so the refetch reads the final list.
await notifyMembersChanged(this, hub_id, {
change: "removed",
users: members,
});
}
users = await this._members_by_type("not_owner", 1);
this.output.list(users);
Expand Down Expand Up @@ -2796,6 +2803,13 @@ class __private_hub extends Hub {
let sockets = await this.yp.await_proc("user_sockets", uid);
await RedisStore.sendData(this.payload(hub), sockets);
}
// The pushes above reach only the members being changed, for their own
// windows. Everybody else with the permission matrix open is told here,
// once, after every write has landed.
await notifyMembersChanged(this, this.hub.get(Attr.id), {
change: "privilege",
users,
});
Comment on lines +2809 to +2812

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Broadcast single-member privilege changes too

When callers use the ACL-exposed hub.set_member_privilege endpoint—for example, to update one member with an expiry—the adjacent implementation writes permission_grant and returns the refreshed list without invoking notifyMembersChanged; only set_privilege reaches this new call. Those updates therefore still leave every other open permission matrix stale, so the single-member privilege writer should emit the same broadcast.

Useful? React with 👍 / 👎.

this.output.data(users);
}

Expand Down
17 changes: 14 additions & 3 deletions service/private/payment.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,14 +142,25 @@
let stripe = null;
try { stripe = this._stripe(); } catch (e) { stripe = null; }
if (stripe) {
for (const p of plans) {
if (!p || !p.stripe_price_id) continue;
// IN PARALLEL. This was `await` inside a for-loop, so the catalog cost
// one Stripe round trip per priced row, end to end: 6 active usd rows
// measured 1919 ms against ~180 ms for every other payment call, and the
// Billing page cannot decide whether a promotion is real until it lands
// — so the banner, the strikes and the modal all waited on it. Fanning
// the lookups out takes it to roughly the cost of one call.
//
// Each task keeps its OWN try/catch, so this behaves exactly as before
// on a bad price id: that row simply has no amount and the FE falls back
// to its offline figure. Promise.all can never reject here, which is
// what stops one dead price id from emptying the whole catalog.
await Promise.all(plans.map(async (p) => {
if (!p || !p.stripe_price_id) return;

Check warning on line 157 in service/private/payment.js

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Prefer using an optional chain expression instead, as it's more concise and easier to read.

See more on https://sonarcloud.io/project/issues?id=drumee_server-team&issues=AaCeBqeHuNydAlqVZsH6&open=AaCeBqeHuNydAlqVZsH6&pullRequest=218
try {
const price = await stripe.prices.retrieve(p.stripe_price_id);
p.amount = price.unit_amount; // minor units (cents)
p.currency = price.currency || p.currency;
} catch (e) { /* leave amount unset on lookup failure */ }
}
}));
}
this.output.data({ plans });
}
Expand Down
133 changes: 133 additions & 0 deletions test/members-changed-push.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
// hub.set_privilege and hub.delete_contributor tell the WHOLE hub.
//
// Before this, both pushed only to the member being changed: set_privilege
// sent { privilege, hub_id, area } to that member's sockets, and
// delete_contributor sent media.remove + hub.member_removed to the removed
// member's. Every other admin with the permission matrix open kept the old row
// until they reopened the panel.
//
// Each now ends with ONE hub.members_changed broadcast to the hub's sockets,
// after the per-member work, so the matrices refetch a committed state. The
// existing per-member pushes are unchanged — the changed member's own windows
// still rely on them.
//
// Run: node --test test/members-changed-push.test.js
const assert = require("node:assert/strict");
const test = require("node:test");

global.myDrumee = { arch: "pod", useEmail: 0 };
global.verbosity = 0;
global.debug = {};

const { RedisStore, Attr } = require("@drumee/server-essentials");
const HubPrivate = require("../service/private/hub");

// hub.js and notify-member-joined.js both destructure this same RedisStore
// object, so patching the method reaches every send without a Redis.
const sent = [];
RedisStore.sendData = async (payload, dest) => {
sent.push({ payload, dest });
};

const HUB_SOCKETS = [{ socket_id: "s-admin-a" }, { socket_id: "s-admin-b" }];
const HUB = {
[Attr.id]: "hub-1",
[Attr.hub_id]: "hub-1",
[Attr.area]: "private",
[Attr.db_name]: "hub_db",
[Attr.profile]: { name: "Design team" },
[Attr.settings]: {},
};

function fakeService({ users, privilege }) {
const procs = [];
return {
procs,
uid: "admin-a",
input: {
need: (k) => (k === Attr.users ? users : undefined),
use: (k) => (k === Attr.privilege ? privilege : undefined),
get: () => undefined,
},
hub: { get: (k) => HUB[k] },
db: {
await_proc: async (name, ...args) => {
procs.push(["db", name, ...args]);
return name === "mfs_home" ? { chat_upload_id: "chat-up" } : {};
},
},
yp: {
await_proc: async (name, ...args) => {
procs.push(["yp", name, ...args]);
if (name === "user_sockets") return [{ socket_id: `s-${args[0]}` }];
if (name === "entity_sockets") return HUB_SOCKETS;
if (name === "get_entity") return { db_name: `db_${args[0]}` };
return [];
},
},
payload: (data, options) => ({ data, options }),
output: { data: () => {}, list: () => {} },
warn: () => {},
granted_node: () => ({}),
_actor_name: () => "Admin A",
_unassign_tasks: async () => {},
_broadcast_task_unassign: async () => {},
_trackWorkspaceMembers: async () => {},
_members_by_type: async () => [],
};
}

const broadcasts = () =>
sent.filter((s) => s.payload.options?.service === "hub.members_changed");

test("set_privilege broadcasts one hub.members_changed to the hub, last", async () => {
sent.length = 0;
const svc = fakeService({ users: ["u1", "u2"], privilege: 15 });
await HubPrivate.prototype.set_privilege.call(svc);

const b = broadcasts();
assert.equal(b.length, 1, "one broadcast for the whole request");
assert.deepEqual(b[0].payload.data, {
hub_id: "hub-1",
change: "privilege",
users: ["u1", "u2"],
});
assert.deepEqual(b[0].dest, HUB_SOCKETS);
assert.equal(sent.at(-1), b[0],
"sent after every per-member write, so a refetch reads committed rows");

// The changed members' own live-privilege pushes are still there.
const own = sent.filter((s) => s !== b[0]);
assert.deepEqual(own.map((s) => s.dest), [[{ socket_id: "s-u1" }], [{ socket_id: "s-u2" }]]);
assert.ok(own.every((s) => s.payload.data.privilege === 15));
});

test("delete_contributor broadcasts one hub.members_changed for the removed members", async () => {
sent.length = 0;
// The acting admin in the list is skipped by delete_contributor itself and
// must not be reported as removed.
const svc = fakeService({ users: ["u1", "admin-a", "u2"] });
await HubPrivate.prototype.delete_contributor.call(svc);

const b = broadcasts();
assert.equal(b.length, 1);
assert.deepEqual(b[0].payload.data, {
hub_id: "hub-1",
change: "removed",
users: ["u1", "u2"],
});
assert.deepEqual(b[0].dest, HUB_SOCKETS);

// The removed members' own notices are unchanged.
const removedNotices = sent.filter(
(s) => s.payload.options?.service === "hub.member_removed",
);
assert.deepEqual(removedNotices.map((s) => s.dest), [[{ socket_id: "s-u1" }], [{ socket_id: "s-u2" }]]);
});

test("delete_contributor with only the admin themself broadcasts nothing", async () => {
sent.length = 0;
const svc = fakeService({ users: ["admin-a"] });
await HubPrivate.prototype.delete_contributor.call(svc);
assert.equal(broadcasts().length, 0, "no membership changed, so no matrix needs a refetch");
});
110 changes: 110 additions & 0 deletions test/notify-members-changed.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
// The "someone's membership changed" broadcast behind every open permission
// matrix.
//
// THE REPORT: an admin changes a member's role (or removes them) and every
// OTHER admin with the matrix open keeps seeing the old row until they reopen
// the panel. hub.set_privilege and hub.delete_contributor only ever pushed to
// the member being changed, and the panels only refetch on hub.member_joined —
// so nothing reached the people looking at the list.
//
// notifyMembersChanged is the missing push. These lock what it sends, to whom,
// and that it can never fail the write it follows.
//
// Run: node --test test/notify-members-changed.test.js
const assert = require("node:assert/strict");
const test = require("node:test");

// Stub the two things the module reaches for before requiring it — the same
// shape revenue-live.test.js uses. toArray lives under `utils` here because
// that is where notify-member-joined.js reads it from.
const sent = [];
require.cache[require.resolve("@drumee/server-essentials")] = {
exports: {
RedisStore: {
sendData: async (payload, dest) => {
sent.push({ payload, dest });
},
},
utils: {
toArray: (v) => (Array.isArray(v) ? v : v == null ? [] : [v]),
},
},
};

const { notifyMembersChanged } = require("../service/lib/notify-member-joined");

const SOCKETS = [
{ socket_id: "s-admin-a", uid: "admin-a" },
{ socket_id: "s-admin-b", uid: "admin-b" },
];

function ctx({ rows = SOCKETS, fail = false } = {}) {
const calls = [];
const warnings = [];
return {
calls,
warnings,
yp: {
await_proc: async (name, arg) => {
calls.push([name, arg]);
if (fail) throw new Error("db down");
return rows;
},
},
payload: (data, options) => ({ data, options }),
warn: (...args) => warnings.push(args),
};
}

test("sends ONE hub.members_changed push to every online member of the hub", async () => {
sent.length = 0;
const svc = ctx();
await notifyMembersChanged(svc, "hub-1", {
change: "privilege",
users: ["u1", "u2"],
});

assert.deepEqual(svc.calls, [["entity_sockets", "hub-1"]],
"the audience is the hub's sockets, not the changed member's own");
assert.equal(sent.length, 1, "one push per request, not one per user");
assert.equal(sent[0].payload.options.service, "hub.members_changed");
assert.deepEqual(sent[0].payload.data, {
hub_id: "hub-1",
change: "privilege",
users: ["u1", "u2"],
});
assert.deepEqual(sent[0].dest, SOCKETS);
});

test("a single user id is sent as a one-item list", async () => {
sent.length = 0;
await notifyMembersChanged(ctx(), "hub-1", { change: "removed", users: "u9" });
assert.deepEqual(sent[0].payload.data.users, ["u9"]);
});

test("nobody online: nothing is sent", async () => {
sent.length = 0;
await notifyMembersChanged(ctx({ rows: [] }), "hub-1", {
change: "removed",
users: ["u1"],
});
assert.equal(sent.length, 0);
});

test("no hub id: does nothing at all", async () => {
sent.length = 0;
const svc = ctx();
await notifyMembersChanged(svc, null, { change: "privilege", users: ["u1"] });
assert.equal(svc.calls.length, 0);
assert.equal(sent.length, 0);
});

test("never throws — the role change or removal has already committed", async () => {
sent.length = 0;
const svc = ctx({ fail: true });
await assert.doesNotReject(
notifyMembersChanged(svc, "hub-1", { change: "privilege", users: ["u1"] }),
);
assert.equal(sent.length, 0);
assert.equal(svc.warnings.length, 1, "the failure is logged, not swallowed silently");
});
Loading