Lane 1: roster visibility rules + hide-yourself privacy flag (gaps 04, 05, 08) by DJAscendance · Pull Request #8 · DJAscendance/ctr

DJAscendance · 2026-07-30T13:34:16Z

Closes gaps 04, 05 and 08 — three separate register entries that all land on one surface, the online roster. Targets this fork, stacked on #7 (feat/member-data-store) since the privacy flag and buddy reads need that store.

Roster rules

Visitors get the count and nothing else (gap 04). message/count.html emits the roster link only when NNM != "Visitor", so an unauthenticated caller learns how many people are online and no more.

CTR gated the whole endpoint on a session, so visitors got a 400 rather than the count they were meant to see. The response now carries returnUsers: null for a visitor — null rather than [] so the client can distinguish "not permitted to see" from "nobody online".

Serving both audiences from one endpoint needed a session probe that doesn't respond on failure: decryptSession writes a 400 as a side effect, which would pre-empt the visitor response. Hence MemberService.peekSession.

Buddies render bold (gap 05). The BU_ loop flag in message/list.html gated a <B> wrapper, and that bold name was the entire visual buddy affordance in the classic UI. Exposed as isBuddy, with isSelf alongside since the viewer's own name renders as plain text rather than a link. The client owns the markup; the API supplies flags.

Buddy matching is case-insensitive, because buddies are stored by nickname — the original's own field set includes NNK, a lowercased nickname, for exactly this reason.

Privacy flag (gap 08)

One boolean, backed by the IMS attribute. That single checkbox is the whole privacy model in the original: no per-buddy blocking, no appear-offline-to-some, no ignore list. The simplicity is the design, so this deliberately stops there.

The part worth reviewing: a hidden member appears offline, so they're excluded from the entries and from the count. Omitting the name while still counting them would leak their presence — the count would exceed the visible names and reveal that someone is hiding. There's a test named for that.

The viewer always sees themselves regardless of their own flag, so turning "hide me" on doesn't make you vanish from your own roster.

New endpoints: GET /member/get_privacy, POST /member/update_privacy. Both act only on the caller's own record.

Also in here

Centralised the online window. 5 * 60000 was written out three times in member.service. Now one constant, value unchanged — so behaviour-preserving. There's a note in-code that the original is 120 s expiry against a 30 s heartbeat (g_MsRefresh), which can't be corrected without adding the heartbeat at the same time, or active users would start dropping off the roster. That's a separate task.
getForMembers batches one attribute across many members. The roster needs every online member's privacy flag at once, and this endpoint already carries two N+1 loops (getHome, getAccessLevel per user) — it didn't need a third.
Buddy slots stay sparse. An empty slot 3 does not shift 4..9 down, because the slot index is part of the original's model. Read-only here; managing the list is a separate task.

Verification

22 new tests, covering the behaviour that would be easy to get subtly wrong:

visitor: count and no names; buddy slots never read; hidden still excluded from count
member:  buddies flagged case-insensitively; self flagged; count === entries.length
hidden:  omitted from entries; omitted from count; still visible to themselves
empty:   [] not null for a member; no attribute query for an empty member list
IMS:     only the exact value "1" reads as hidden ("0" does not)
slots:   always exactly ten; sparse, not compacted; out-of-range names ignored

Suite 15 → 37 passing, typecheck clean, 0 lint errors.

This adds a constructor dependency to MemberService, whose spec was already failing on missing MySQL — so I compared that spec's failures failure-for-failure against the merge base rather than trusting the count. Byte-identical, so the new dependency didn't break its DI.

Not in here

No UI. The isBuddy/isSelf flags and the privacy endpoints are unconsumed until the Vue side is wired.

coderabbitai · 2026-07-30T14:31:08Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6e462bd5-3fc2-4f72-b9d6-0edbec33c6f0

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

DJAscendance · 2026-07-30T14:46:46Z

@coderabbitai review

coderabbitai · 2026-07-30T14:46:51Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copilot

Pull request overview

This PR implements classic Cybertown-style online roster visibility rules (visitor count-only, buddy/self flags, and hide-yourself privacy) in the API layer, backed by the member_data attribute store and exposed through updated roster and new privacy endpoints.

Changes:

Added RosterService to build a roster view with visitor gating (entries: null), buddy/self flags, and IMS-based hidden-member filtering (including count privacy).
Added MemberDataService helpers for IMS (hide-yourself) and BU0..BU9 buddy slots, plus a batched repository read (getForMembers) to avoid per-member attribute lookups.
Updated /member/online_users to serve both visitors and members (via peekSession), plus added /member/get_privacy and /member/update_privacy endpoints.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
api/src/services/roster/roster.service.ts	New roster builder applying visitor/buddy/self/hidden rules and producing `{count, entries}`.
api/src/services/roster/roster.service.spec.ts	Unit tests covering visitor vs member behavior, hidden-member count rules, and buddy/self flags.
api/src/services/member/member.service.ts	Centralizes “online window” constant; adds `peekSession` and delegates roster building to `RosterService`.
api/src/services/member-data/member-data.service.ts	New service encapsulating IMS (privacy) and BU* buddy-slot semantics and parsing rules.
api/src/services/member-data/member-data.service.spec.ts	Unit tests for IMS semantics and buddy-slot sparsity / parsing edge cases.
api/src/services/index.ts	Exports new `MemberDataService` and `RosterService` from the services barrel.
api/src/routes/member.routes.ts	Adds `/member/get_privacy` and `/member/update_privacy` routes.
api/src/repositories/member-data/member-data.repository.ts	Adds `getForMembers` to batch-read a single attribute across many members.
api/src/controllers/member.controller.ts	Adds privacy endpoints and reworks `/member/online_users` to return visitor-safe responses and new flags.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+    const online: { id: number; username: string }[] =
+      await this.memberRepository.findOnlineUsers(activeWithin);
+
+    // One batched read rather than a per-member lookup.
+    const hiddenFlags = await this.memberDataRepository.getForMembers(
+      online.map(member => member.id),
+      MemberDataService.HIDDEN,
+    );
+    const visible = online.filter(
+      member =>
+        hiddenFlags.get(member.id) !== '1' ||
+        (!!viewerMemberId && member.id === viewerMemberId),
+    );
+
+    if (!viewerMemberId) {
+      return { count: visible.length, entries: null };
+    }
+
+    const buddies = await this.memberDataService.getBuddyNameSet(viewerMemberId);


+    it('does not query attributes for an empty member list', async () => {
+      online();
+      await service.getRoster(null, ACTIVE_SINCE);
+      expect(memberDataRepository.getForMembers).toHaveBeenCalledWith([], 'IMS');
+    });


+      if (roster.entries === null) {
+        response.status(200).json({ count: roster.count, returnUsers: null });
+        return;


Copilot review of #8. Three findings, all verified. The visitor case was broken, not just changed. This PR started returning `{ count, returnUsers: null }` to an unauthenticated caller instead of a 400, and CitizenOnlineModal assigns returnUsers straight to this.users and then calls .forEach on it, with no try/catch. So a visitor went from a rejected request that left the list empty -- degraded but working, "0 Citizens Online" -- to a TypeError on null, with the template's users.length failing too. That is worse than what it replaced. Fixed on the client rather than by softening the API. null is the right answer: it distinguishes "you may not see who is online" from "nobody is online", and the endpoint now returns a count to visitors precisely so they can be shown something. The modal records that as a canSeeUsers flag, keeps this.users an array either way, takes its heading from `count` rather than users.length, and shows "Sign in to see who is online." in place of the list. message/count.html in the original emitted the roster link only when NNM != "Visitor", so a visitor seeing a number and no names is the intended behaviour. The heading also stops saying "0 Citizen". It read `v-if="users.length > 1"`, so zero took the singular branch; `count !== 1` is the actual rule. RosterService now short-circuits when nobody is online. It was issuing the batched hidden-flag read with an empty id list, and for a member also fetching their buddy set, to build an empty roster. This endpoint is polled, so the empty case should cost nothing. The spec for that asserted the opposite of its own name. It was called 'does not query attributes for an empty member list' while asserting getForMembers HAD been called with [] -- so it documented and locked in the wasted round trip. Inverted, plus a companion test that a member with nobody online does not trigger the buddy lookup either. Verified: tsc clean apart from the pre-existing missing 'sharp' module. eslint 0 errors on the changed api files. Suite compared against a stashed baseline test-name by test-name: identical failures, no regressions; roster and member-data specs 23 -> 24. The modal's eslint errors went 21 -> 20: all pre-existing, none introduced, and the max-len on the heading line is gone because that line was rewritten. Deliberately not done: the modal still ignores the isBuddy and isSelf flags this PR added to each entry -- buddies should render bold and the viewer's own name as plain text rather than a link. The server side is done; wiring the markup is a separate change. spa/node_modules was symlinked into this worktree from the main checkout so the .vue file could be linted at all.

Closes two gaps that share the same surface, the online roster. ROSTER RULES (gaps 04, 05) Visitors get the COUNT and nothing else. message/count.html emits the roster link only when NNM != "Visitor", so an unauthenticated caller learns how many people are online and no more. CTR gated the whole endpoint on a session, so visitors got a 400 rather than the count they were meant to see. The response now carries `returnUsers: null` for a visitor -- null rather than [] so the client can tell "not permitted to see" from "nobody online". Buddies render bold: the BU_ loop flag in message/list.html gated a <B> wrapper, and that bold name was the entire visual buddy affordance in the classic UI. Exposed as isBuddy, with isSelf alongside it since the viewer's own name renders as plain text rather than a link. The client owns the markup; the API supplies flags. Serving both members and visitors from one endpoint needed a session probe that does not respond on failure -- decryptSession writes a 400 as a side effect, which would pre-empt the visitor response. Added MemberService.peekSession for that. PRIVACY FLAG (gap 08) One boolean, backed by the IMS attribute. That single checkbox is the WHOLE privacy model in the original: no per-buddy blocking, no appear-offline-to-some, no ignore list. The simplicity is the design, so this deliberately stops there. A hidden member appears OFFLINE, so they are excluded from the entries AND from the count. Omitting the name while still counting them would leak their presence, because the count would exceed the visible names and reveal that someone is hiding. Tested. The viewer always sees themselves regardless of their own flag, so turning "hide me" on does not make you vanish from your own roster. ALSO - Centralised the online window. `5 * 60000` was written out three times in member.service; now one constant. Value unchanged, so this is behaviour-preserving -- note in-code that the original is 120 s against a 30 s heartbeat, which cannot be corrected without adding the heartbeat at the same time or active users would drop off the roster. - MemberDataRepository.getForMembers batches one attribute across many members. The roster needs every online member's privacy flag at once, and this endpoint already carries two N+1s; it did not need a third. - Buddy slots are read-only here and stay SPARSE: an empty slot 3 does not shift 4..9 down, because the slot index is part of the original's model. Managing the list is a separate task. 22 tests. Suite 15 -> 37 passing, with the same 5 pre-existing DB-dependent failures -- verified failure-for-failure against the merge base, since this adds a constructor dependency to MemberService.

Three fixes from a local CodeRabbit pass, each verified against the code before applying. Buddy slot names are now matched as text, not coerced. The suffix check ran Number() first and asked whether the result was an integer in range, which is far more permissive than the ten slot names are. Number('') is 0, so an attribute named exactly 'BU' populated slot 0. Number() also reads '01' and '1e0' as 1, so BU01 and BU1e0 both collided with BU1. getByPrefix matches on prefix, so any of these can arrive from imported data, and any future BU* attribute that is not a slot would have been silently read as one. Requiring exactly one digit is the actual rule. The existing range test covered BU10 and BUxx, which the old code already rejected, so it did not catch this; added a case for the three that got through. The roster's `security` flag was dead. getAccessLevel returns string[], so `accessLevel === 'security'` compared an array against a string and could never be true -- every entry has come back security:false since the flag was introduced in 1a35fec (Jan 2025), which this PR carried forward verbatim when it rewrote getOnlineUsers. Every other consumer in the tree already uses .includes(), including getRoles four hundred lines up in this same controller. This is a visible behaviour change: security members will now actually be marked on the roster, which is what the field was added to do. getOnlineUsers no longer issues two queries per person. hasHome is now one batched findMemberIdsWithHome call, matching getDirectory directly below it. getAccessLevel is left per-entry but resolved with Promise.all instead of awaited in a loop -- it fans out into canAdmin, canLeader and a role lookup, so genuinely batching it means batching those three, and that is a larger change than this cleanup should carry. Verified: tsc clean apart from the pre-existing missing 'sharp' module, which fails identically on feat/member-data-store. eslint 0 errors. Suite compared failure-for-failure against a stashed baseline rather than by count: the same five suites fail before and after, all on MySQL connection errors needing a live database, and passing tests go 37 -> 38 with the one test added here. Deliberately not done: getAccessLevel's return type is still `any` and its shape is only knowable by reading it, which is what allowed the dead comparison to survive review. Typing it string[] would surface any other bad comparison at compile time -- worth doing, but it touches call sites outside this PR.

Copilot review of #8. Three findings, all verified. The visitor case was broken, not just changed. This PR started returning `{ count, returnUsers: null }` to an unauthenticated caller instead of a 400, and CitizenOnlineModal assigns returnUsers straight to this.users and then calls .forEach on it, with no try/catch. So a visitor went from a rejected request that left the list empty -- degraded but working, "0 Citizens Online" -- to a TypeError on null, with the template's users.length failing too. That is worse than what it replaced. Fixed on the client rather than by softening the API. null is the right answer: it distinguishes "you may not see who is online" from "nobody is online", and the endpoint now returns a count to visitors precisely so they can be shown something. The modal records that as a canSeeUsers flag, keeps this.users an array either way, takes its heading from `count` rather than users.length, and shows "Sign in to see who is online." in place of the list. message/count.html in the original emitted the roster link only when NNM != "Visitor", so a visitor seeing a number and no names is the intended behaviour. The heading also stops saying "0 Citizen". It read `v-if="users.length > 1"`, so zero took the singular branch; `count !== 1` is the actual rule. RosterService now short-circuits when nobody is online. It was issuing the batched hidden-flag read with an empty id list, and for a member also fetching their buddy set, to build an empty roster. This endpoint is polled, so the empty case should cost nothing. The spec for that asserted the opposite of its own name. It was called 'does not query attributes for an empty member list' while asserting getForMembers HAD been called with [] -- so it documented and locked in the wasted round trip. Inverted, plus a companion test that a member with nobody online does not trigger the buddy lookup either. Verified: tsc clean apart from the pre-existing missing 'sharp' module. eslint 0 errors on the changed api files. Suite compared against a stashed baseline test-name by test-name: identical failures, no regressions; roster and member-data specs 23 -> 24. The modal's eslint errors went 21 -> 20: all pre-existing, none introduced, and the max-len on the heading line is gone because that line was rewritten. Deliberately not done: the modal still ignores the isBuddy and isSelf flags this PR added to each entry -- buddies should render bold and the viewer's own name as plain text rather than a link. The server side is done; wiring the markup is a separate change. spa/node_modules was symlinked into this worktree from the main checkout so the .vue file could be linted at all.

DJAscendance requested a review from Copilot July 30, 2026 14:51

Copilot started reviewing on behalf of DJAscendance July 30, 2026 14:52 View session

Copilot AI reviewed Jul 30, 2026

View reviewed changes

DJAscendance added 3 commits July 30, 2026 11:19

DJAscendance force-pushed the feat/roster-privacy branch from fb663a1 to 1ae407f Compare July 30, 2026 15:19

Navigation Menu

Lane 1: roster visibility rules + hide-yourself privacy flag (gaps 04, 05, 08) - #8

Lane 1: roster visibility rules + hide-yourself privacy flag (gaps 04, 05, 08)#8
DJAscendance wants to merge 3 commits into
feat/member-data-storefrom
feat/roster-privacy

DJAscendance commented Jul 30, 2026

Uh oh!

coderabbitai Bot commented Jul 30, 2026 •

edited

Loading

Review skipped

Uh oh!

DJAscendance commented Jul 30, 2026

Uh oh!

coderabbitai Bot commented Jul 30, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

DJAscendance commented Jul 30, 2026

Roster rules

Privacy flag (gap 08)

Also in here

Verification

Not in here

Uh oh!

coderabbitai Bot commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

DJAscendance commented Jul 30, 2026

Uh oh!

coderabbitai Bot commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Jul 30, 2026 •

edited

Loading

coderabbitai Bot commented Jul 30, 2026 •

edited

Loading