Lane 1: add member_data, the per-member attribute store - #7
Open
DJAscendance wants to merge 2 commits into
Open
Conversation
Lane 1 foundation. In CS 4.x this is the MD / memdata table, keyed by member id,
and it is where the buddy list actually lives -- ten slots BU0..BU9 holding
NICKNAMES, settled by writing a buddy on a live 4.1 server and diffing the data
files. Buddies are not a join table and not in groups/groupmem; those back the
Group entries in the access-rights model instead. sqserver.sql names this table
Member_Data.
Generic key/value rather than a column per feature, because that is what the
original is: buddy slots, the hide-yourself privacy flag (IMS) and similar state
are all named attributes there. Adding a feature should not need a migration.
Deliberately NOT normalised into a friend table. The ten-slot nickname-keyed shape
is the fidelity target, and "improving" it into (member_id, friend_member_id) rows
would lose two behaviours the original has: a slot can name someone who does not
exist or who later renames, and the slot INDEX is meaningful and stable.
Two details worth keeping:
- An empty or null value DELETES the row rather than storing ''. Otherwise
"unset" has two representations and a cleared buddy slot reads back as '' from
one path and null from another.
- getByPrefix escapes LIKE metacharacters, so getByPrefix('X_') cannot also match
'XA1'. Reading a whole family ('BU') is the normal access pattern and a caller
should not be able to widen it accidentally.
value is text, not json: MySQL 5.7 is the target and every value the original
stores here is a short scalar.
Verified against a throwaway MySQL 5.7: upsert replaces rather than duplicating
(1 row, not 2); set('') leaves 0 rows and reads back null; ten BU slots round-trip
and a BU prefix read excludes IMS; getByPrefix('X_') returns X_1 and not XA1;
setMany applies a mixed set+unset atomically.
No unit spec: this layer is thin query glue, so mocking knex would mostly assert
the mock. The behaviour that matters -- upsert, unset semantics, prefix escaping --
was exercised against a real database instead. Services built on it (buddy list,
privacy flag) carry real logic and will get specs.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
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. Comment |
There was a problem hiding this comment.
Pull request overview
Adds the foundational per-member attribute store (“CS 4.x MD / memdata”) to the API so future social-layer features (buddy slots BU0..BU9, privacy flags like IMS, etc.) can persist member-scoped state without adding new columns/migrations per feature.
Changes:
- Introduces the
member_dataDB table via a new Knex migration (unique key on(member_id, name)). - Adds a
MemberDatamodel,Db.memberDataaccessor, and re-exports for consumption across the API. - Adds
MemberDataRepositorywith get/set APIs, prefix reads, and transactional multi-set with “empty/null unsets” semantics.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| api/src/types/models/member-data.model.ts | Adds the MemberData row shape for typed access to member_data. |
| api/src/types/models/index.ts | Re-exports MemberData from the models barrel. |
| api/src/repositories/member-data/member-data.repository.ts | Implements read/write operations for per-member named attributes. |
| api/src/repositories/index.ts | Re-exports MemberDataRepository from the repositories barrel. |
| api/src/db/db.class.ts | Adds a typed memberData table accessor on Db. |
| api/db/migrations/20260730140000_create_member_data.ts | Creates the member_data table and its constraints/indexing. |
Comments suppressed due to low confidence (1)
api/src/repositories/member-data/member-data.repository.ts:91
- Same as
set():valuesis typed asRecord<string, string | null>, so filtering forvalue === undefinedis unreachable from the type system and suggestsundefinedis supported. Consider removing theundefinedchecks (or widening the type if that’s intentional) to keep the API contract clear.
.filter(([, value]) => value === null || value === undefined || value === '')
.map(([name]) => name);
const toUpsert = entries
.filter(([, value]) => !(value === null || value === undefined || value === ''))
.map(([name, value]) => ({ member_id: memberId, name, value: value as string }));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .map(([name, value]) => ({ member_id: memberId, name, value: value as string })); | ||
|
|
||
| if (toDelete.length) { | ||
| await trx('member_data').where('member_id', memberId).whereIn('name', toDelete).del(); |
Comment on lines
+14
to
+16
| export class MemberDataRepository { | ||
| constructor(private db: Db) {} | ||
|
|
Comment on lines
+45
to
+49
| // One row per (member, attribute). Writes are upserts against this. | ||
| table.unique(['member_id', 'name']); | ||
| // Reads are almost always "all attributes for this member", or a prefix scan of one | ||
| // family (BU%), so member_id leads. | ||
| table.index(['member_id', 'name']); |
Comment on lines
+66
to
+70
| public async set(memberId: number, name: string, value: string | null): Promise<void> { | ||
| if (value === null || value === undefined || value === '') { | ||
| await this.unset(memberId, name); | ||
| return; | ||
| } |
Copilot review of #7. Three of four findings; the fourth did not reproduce. The repository had no spec despite carrying the non-obvious behaviour of this PR -- unset-on-empty, LIKE escaping, and the transactional setMany. member.repository.spec.ts is the precedent, so this follows it. 19 tests: get returning null for a missing row, getAll reducing to name -> value, getByPrefix's escaping (including a literal backslash), set deleting rather than storing an empty value for each of null / '' / undefined, and setMany's upsert half, delete half, mixed batch, no-op on {}, and the fact that it does not issue a pointless delete when nothing is cleared. The spec builds a local chainable mock instead of using @spec/mocks. This repository calls db.knex('member_data') as a FUNCTION and awaits the builder, whereas the shared mockDb exposes knex as a plain object whose builder is not thenable -- awaiting it yields the builder rather than rows. Reshaping a mock that every other repository spec depends on, to add coverage for one new repository, is a worse trade than keeping the change contained here. Two traps worth recording, since both produced confidently wrong tests before they were caught. Returning the thenable builder from an async helper makes the runtime await it, so the caller gets the query result instead of the builder it wanted to assert on -- it is now returned wrapped. And a `result: any = []` default parameter swallows an explicit undefined, so the "no row" test was really asserting against a truthy empty array; useBuilder takes rest args and checks length instead. The migration no longer declares index(['member_id', 'name']) alongside unique(['member_id', 'name']). A UNIQUE constraint IS a btree index in MySQL, and because member_id leads it already serves both "all attributes for this member" and the BU% prefix scan. The second declaration was the same index twice: extra storage and extra write cost on every upsert for no query the optimiser could not already satisfy. set and setMany now type value as `string | null | undefined`. The undefined branch was flagged as unreachable, which is true of the declared type and false of reality: these values arrive from request bodies and dynamically built objects where a missing key is undefined long before it meets a typed boundary. Widening the type is the honest fix; deleting the guard would have removed real runtime protection to satisfy a signature that was wrong. Not reproduced: the report that the setMany delete chain exceeds the 100-character limit. No line in the file is over 100 -- that one is 93 -- and eslint reports nothing on it. Left alone. Verified: tsc clean apart from the pre-existing missing 'sharp' module. eslint 0 errors. Full suite compared against a stashed baseline test-name by test-name: identical failures, no regressions. Note the new spec is untracked and so was present in both runs; the baseline still covers the migration and repository changes, which were stashed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lane 1 foundation. Targets this fork's
master, deliberately notCybertownRevival/ctr.Adds
member_data, the per-member attribute store everything else in the social layer hangs off.Why this shape
In CS 4.x this is the MD / memdata table, keyed by member id, and it is where the buddy list actually lives — ten slots
BU0..BU9holding nicknames. That was settled empirically: writing a buddy on a live 4.1 server and diffing the data files. Buddies are not a join table and not ingroups/groupmem— those back the Group entries in the access-rights model instead.sqserver.sqlnames this tableMember_Data.Generic key/value rather than a column per feature, because that is what the original is: buddy slots, the hide-yourself privacy flag (
IMS) and similar state are all named attributes there. Adding a feature shouldn't need a migration.Deliberately not normalised into a friend table. The ten-slot nickname-keyed shape is the fidelity target. "Improving" it into
(member_id, friend_member_id)rows would lose two behaviours the original has: a slot can name someone who doesn't exist or who later renames, and the slot index is meaningful and stable.Two details worth reviewing
''. Otherwise "unset" has two representations, and a cleared buddy slot reads back as''from one path andnullfrom another.getByPrefixescapes LIKE metacharacters, sogetByPrefix('X_')cannot also matchXA1. Reading a whole family ('BU') is the normal access pattern and a caller shouldn't be able to widen it by accident.valueistext, notjson— MySQL 5.7 is the target and every value the original stores here is a short scalar.Verification
Against a throwaway MySQL 5.7 through the real migration:
Typecheck and lint clean; suite unchanged from the merge base (5 pre-existing DB-dependent failures, 15 passing).
No unit spec, on purpose. This layer is thin query glue, so mocking knex would mostly assert the mock. The behaviour that matters — upsert, unset semantics, prefix escaping — was exercised against a real database instead. The services built on top (buddy list, privacy flag) carry real logic and will get specs.
What is NOT in here
Nothing consumes the store yet. The buddy list, IM and privacy flag are separate tasks, and several of them touch
spa/server.js— which is being reworked by CybertownRevival#412 / fork #5, so they need to be branched on top of that rather than onmaster.