Discourse PR #32 — Live Benchmark

Golden Comments (3/3 found)

#	Comment	Severity	Status	FriendlyReviewer Detail
#1	The `findMembers()` call is now asynchronous and unhandled. The controller may not have member data immediately available, creating a race condition.	HIGH	✓ Found	"`setupController` fires `model.findMembers()` without returning/awaiting the promise. The members list and pagination state populate asynchronously, allowing the template to render an empty list that later "pops in," and any AJAX failure results in an unhandled rejection" (admin_group_route.js L13)
#2	In the next action, capping the next offset at user_count can produce an empty page (e.g., total equal to limit results in offset == total, showing 2/2 with no members). This can cause confusing UX on the last page.	MEDIUM	✓ Found	"Offset clamping against `user_count` uses a possibly-stale/zero value (initial default 0), which can silently clamp a legitimate paginated request to 0 or to an incorrect value on first load. The clamp also permits `offset === user_count`, returning an empty page" (group.js L23)
#3	HTTP method mismatch in `.remove_member` - test uses PUT but remove_member action expects DELETE	MEDIUM	✓ Found	"The `remove_member` automatic-group spec uses `xhr :put` but the route is `delete "members"`; the sibling success test correctly uses `xhr :delete`" (groups_controller_spec.rb L115)

Supplementary Findings (15 findings)

Finding	File	Severity	Legitimate?
`findMembers` returns `undefined` when the group name is empty instead of a resolved promise, breaking the promise contract used by `addMembers`/`removeMember` and any caller that chains `.then()`	group.js:21	HIGH	✓ Real contract break: any caller chaining on the promise crashes; not covered by the dataset
`add_members` expects a comma-separated String via `.split` but the client sends an Array — Rails parses an Array → `NoMethodError` → HTTP 500; the spec only passes a pre-joined string	admin/groups_controller.rb:71	HIGH	✓ Real: the actual client payload crashes the endpoint; the spec masks it
The `limit` parameter has no upper bound — `?limit=1000000` retrieves every member in one request, defeating pagination and creating a DoS vector	groups_controller.rb:23	HIGH	✓ Real DoS vector on a public endpoint; no clamping anywhere
Inside `addMembers` and `removeMember`, the `self.findMembers()` reload is not returned and has no failure handler — the caller's promise resolves before the reload completes and AJAX rejections are unhandled	group.js:47	MEDIUM	✓ Real unhandled-async issue; the missing semicolon part is cosmetic
`remove_member` uses `group.users.delete(user_id)`, bypassing `GroupUser` destroy callbacks — the `user_count` counter cache is not decremented and stays stale	admin/groups_controller.rb:90	MEDIUM	✓ Real: `user_count` diverges from actual membership after every removal
`add_members` does not dedup against current membership — re-adding an existing member violates the `(group_id, user_id)` unique index, raising `RecordNotUnique` → 500	admin/groups_controller.rb:71	MEDIUM	✓ Real: a duplicate insert crashes with 500 instead of a clean error
Negative `limit`/`offset` values are passed directly into the query builder — PostgreSQL rejects `LIMIT must not be negative`, causing a 500	groups_controller.rb:24	MEDIUM	✓ Real: trivial to trigger, no input validation
`total = group.users.count` and the member page are two separate, non-atomic queries — concurrent mutations can make `meta.total` mismatch the page and skip/duplicate rows	groups_controller.rb:26	MEDIUM	✓ Borderline: a real race, but requires concurrent membership writes to manifest
`.next, .previous` colors use raw hex literals instead of theme SCSS variables — breaks custom/dark themes	admin_base.scss:472	MEDIUM	✓ Real theme inconsistency; cosmetic in severity
New fixed widths (`350px`/`190px`) have no responsive handling — they overflow the container on a 320px viewport	admin_base.scss:174	MEDIUM	✓ Real on small viewports; low impact elsewhere
`update` ignores `name` for automatic groups but still applies `alias_level` and `visible` changes	admin/groups_controller.rb:37	LOW	✓ Borderline: likely intended (no membership bypass), worth confirming
The server reflects `meta.offset` as the raw value sent by the client — relies on a cooperative client, any other client can get a valid-looking empty page	groups_controller.rb:27	LOW	✓ Borderline: harmless today, fragile by contract
The new block uses `!important` on three declarations (`width: 100% !important`, `margin-right: 0 !important`, `color: #333 !important`), fighting the cascade	admin_base.scss:464	LOW	✓ Real style-hygiene issue; makes future overrides fragile
The new `.groups` block is inserted at the top of the file, far from the `// Groups area` banner and the main `.groups` block (~line 462)	admin_base.scss:164	LOW	✓ Borderline: organization only, no behavior impact
A pre-existing `.row.groups input { width: 500px }` / `#group-users { width: 600px }` block now coexists with the new 350px rule — two scattered, inconsistent width rules	admin_base.scss:1054	LOW	✓ Real: conflicting width rules for the same element

Honest Analysis

FriendlyReviewer found all three golden comments of this PR — recall 100%. Golden #1 (HIGH) is covered by the candidate on admin_group_route.js L13, which describes exactly the race the dataset flags: setupController fires model.findMembers() without awaiting it, so the members list and pagination render empty and "pop in" later, with unhandled rejections on failure. Golden #2 maps to the offset === user_count clamp in group.js L23 — the empty-last-page case the dataset calls out. Golden #3 is the xhr :put vs delete "members" mismatch in the remove_member spec (groups_controller_spec.rb L115).

Where the review goes beyond the dataset is on the server side: three high-severity findings that are not in the goldens — findMembers returning undefined for empty names (a broken promise contract), add_members crashing with a 500 on the real client's array payload (the spec only passes a pre-joined string), and an unbounded limit that turns pagination into a DoS vector. The first two are directly about the core feature, so the "should not be merged" verdict is well supported.

The honest caveat is noise: five of the fifteen supplementary findings are SCSS hygiene (raw hex colors, !important, block placement, conflicting width rules). They are individually correct — the hex literals genuinely break dark themes — but they inflate the finding count and dilute the signal. The three genuine blockers plus the stale user_count counter cache are the review's real value, and any of them alone would have justified the verdict.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech	🌥️ Feat
3 high, 10 medium, 5 low	1 issue

The MR introduces server-side pagination and admin member management for groups, but it ships with several contract regressions and functional bugs that block a clean merge. The client-side findMembers promise contract is broken (returns undefined for empty names, and member-list reloads are fired without chaining or error handling), the admin add_members action fails against the real client's array payload (the spec only exercises a string), and remove_member leaves the user_count counter cache stale. On the public listing side, the limit parameter is unbounded and negative values cause 500s. The SCSS work also needs cleanup (hard-coded colors, !important overuse, missing responsive handling). Several high-severity issues directly affect the core feature, so this MR should not be merged until they are resolved.

These might need a close look

🔴 app/assets/javascripts/discourse/models/group.js (L21)
findMembers returns undefined when the group name is empty instead of a resolved promise (Ember.RSVP.resolve()). This breaks the promise contract used by addMembers/removeMember and any caller that chains .then().
🔴 app/controllers/admin/groups_controller.rb (L71)
add_members expects params[:usernames] to be a comma-separated String via .split, but the client sends an Array. jQuery serializes arrays with bracket notation, so Rails parses an Array → NoMethodError: undefined method 'split' for Array → HTTP 500 (or only one member added if flattened). The spec passes a pre-joined string and doesn't catch this. Normalize input to accept both Array and String.
🔴 app/controllers/groups_controller.rb (L23)
The limit parameter has no upper bound; ?limit=1000000 retrieves every member in one request, defeating pagination and creating a DoS vector. Clamp limit (e.g. min(max(1, limit), 100)).

Worth checking

🟡 app/assets/javascripts/discourse/models/group.js (L47)
Inside addMembers (line 58) and removeMember (line 47), the self.findMembers() reload is not returned and has no failure handler. The caller's promise resolves before the reload completes, and AJAX rejections are unhandled. Also missing trailing semicolon after the .then(...) close on line 58.
🟡 app/assets/javascripts/admin/routes/admin_group_route.js (L13)
setupController fires model.findMembers() without returning/awaiting the promise. The members list and pagination state populate asynchronously, allowing the template to render an empty list that later "pops in," and any AJAX failure results in an unhandled rejection. Consider an afterModel await or explicit rejection handling.
🟡 app/assets/javascripts/discourse/models/group.js (L23)
Offset clamping against user_count uses a possibly-stale/zero value (initial default 0), which can silently clamp a legitimate paginated request to 0 or to an incorrect value on first load. The clamp also permits offset === user_count, returning an empty page. Drive it off the just-fetched meta.total or rely on the server.
🟡 app/controllers/admin/groups_controller.rb (L90)
remove_member uses group.users.delete(user_id), which bypasses GroupUser destroy callbacks, so the counter_cache: "user_count" is not decremented. group.user_count stays stale while group.users.count reflects the truth, breaking index/create serializations. Delete via the join model (e.g. GroupUser.where(...).destroy_all) so callbacks run.
🟡 spec/controllers/admin/groups_controller_spec.rb (L115)
The remove_member automatic-group spec uses xhr :put but the route is delete "members"; the sibling success test correctly uses xhr :delete. Also hard-codes group_id: 1 (and id: 1 in .update line 52, group_id: 1 in .add_members line 94), assuming the seeded automatic group is id 1. Fabricate an automatic group instead to avoid brittle assumptions.
🟡 app/controllers/admin/groups_controller.rb (L71)
add_members does not dedup against current membership; re-adding an existing member creates a duplicate join row violating the (group_id, user_id) unique index, raising RecordNotUnique → 500. Guard against already-present members and add a spec for the "add existing member" case.
🟡 app/controllers/groups_controller.rb (L24)
Negative limit/offset values are passed directly into the query builder; PostgreSQL rejects LIMIT must not be negative, causing a 500. Validate limit >= 1 and offset >= 0 (clamp or return 400/422).
🟡 app/controllers/groups_controller.rb (L26)
total = group.users.count and the member page are two separate, non-atomic queries. Concurrent membership mutations can cause meta.total to not match the page contents, and successive pages can skip/duplicate rows. Fetch a consistent snapshot or document total as best-effort.
🟡 app/assets/stylesheets/common/admin/admin_base.scss (L472)
.next, .previous { color: #333 !important; ... &.disabled { color: #aaa !important; } } uses raw hex literals instead of theme SCSS variables. This breaks custom/dark themes and is inconsistent with the rest of the file. Use $primary / scale-color helpers.
🟡 app/assets/stylesheets/common/admin/admin_base.scss (L174)
The new .groups .form-horizontal input { width: 350px } and .item { width: 190px } have no responsive handling. On a 320px viewport they overflow the container. Add rules to the existing mobile media query (e.g. width ~100%) to avoid breaking the mobile admin layout.

Small things (take or leave)

🔵 app/controllers/admin/groups_controller.rb (L37)
update ignores name for automatic groups but still applies alias_level and visible changes. This is likely intended (no membership bypass), but confirm the UI never offers alias_level/visible editing on automatic groups.
🔵 app/controllers/groups_controller.rb (L27)
The server reflects meta.offset as the raw value sent by the client. This is consistent today but relies on a cooperative client; any other client can send an out-of-range offset and get a valid-looking empty page. Clamp/validate offset independently.
🔵 app/assets/stylesheets/common/admin/admin_base.scss (L464)
The new block uses !important on three declarations (width: 100% !important, margin-right: 0 !important, color: #333 !important). This fights the cascade and makes future overrides fragile. Prefer scoping/specificity; the mobile override at ~line 1338 already handles the mobile case.
🔵 app/assets/stylesheets/common/admin/admin_base.scss (L164)
The .groups, .badges { .form-horizontal ... } block is inserted at the top of the file, far from the // Groups area banner and the main .groups block (~line 462). This splits group styling across distant locations; consolidate under the appropriate banner.
🔵 app/assets/stylesheets/common/admin/admin_base.scss (L1054)
A pre-existing .row.groups input { width: 500px } / #group-users { width: 600px } block now coexists with the new .groups .form-horizontal input { width: 350px }, producing two scattered, inconsistent width rules. Reconcile to keep future edits consistent.

✅ Feature-level checklist

Not there yet

❌ Proper handling of group memberships (per PR summary "FIX: proper handling of group memberships")
The core member-management feature is not correctly handled: add_members fails on the client's array payload (HTTP 500), remove_member leaves the user_count counter cache stale, and the client findMembers promise contract is broken for empty names. These directly prevent reliable group membership management. Additional pagination validation gaps (unbounded/negative limit) also need addressing. Multiple findings (lines 21, 47, 71, 90 of the relevant files) block the requirement.

FIX: proper handling of group memberships