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
30 changes: 30 additions & 0 deletions docs/groups-list-improvements/review-round-1.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
# Review round 1 — feat/groups-list-improvements

Three reviewers (code quality, security, functional/UX) ran in parallel against PR #53. All three returned a `ship` or `ship-then-fix-nits` verdict — no critical findings. Items below are tracked as accepted (acted on this round) or rejected (with rationale).

## Accepted

| # | Source | Item | Action |
|---|---|---|---|
| A1 | Code quality #2 | Sort comparator redundantly lowercases before `localeCompare` | Drop `.toLowerCase()`, pass `{ sensitivity: "base" }` to `localeCompare` |
| A2 | Code quality #3 | `new Date(joinedAt).getTime()` runs O(n log n) times during sort | Decorate-sort-undecorate: pre-compute timestamps once |
| A3 | Code quality #4 | Bare `aria-hidden` is inconsistent with `aria-hidden="true"` elsewhere | Use the explicit form |
| A4 | Code quality #5 | `e.target.value as SortMode` is an unchecked cast | Validate against `SORT_OPTIONS` before setting state |
| A5 | Code quality #10 + Functional #1 | Tooltip on wrapper div is invisible to keyboard/AT users; current sort mode not announced | Move `title` onto the `<select>` and include the current mode in `aria-label` |
| A6 | Functional #2 | Owners' Leave-button tooltip is a dead-end ("Owners can't leave the group") | Add actionable guidance: "Owners can't leave — transfer ownership in group settings first" |
| A7 | Code quality #7 | `org.displayName \|\| org.handle` is repeated 6x | Extract a local `displayLabel` const inside `renderOrgItem` |

## Rejected

| # | Source | Item | Rationale |
|---|---|---|---|
| R1 | Code quality #1 | Persist `sortMode` to `localStorage` | Out of scope. User explicitly said "by default order them from oldest to newest" — persistence is a separate product decision. The PR body lists this in "Out of scope". |
| R2 | Code quality #6 | Inline `renderOrgItem` JSX in `.map()` instead of extracting | Style preference; helper improves the JSX-block readability. No perf signal. |
| R3 | Code quality #8 + #9 + Functional #3 | Prune `accepted` tie-break from `navbar.tsx`; mark `Group.accepted` optional | Out of scope. PR body explicitly leaves the navbar untouched. `accepted` is still set by `groups/create/page.tsx` and `add-org-modal.tsx`; deeper cleanup belongs in a follow-up. |
| R4 | Security nit | Wrap `console.error("Failed to leave group:", err)` to avoid logging full Error objects | Reviewer rated "Acceptable" — no token/cookie/secret in scope. Pre-existing pattern in the file. |
| R5 | Security important | Removing UserCheck/UserX is a privacy-UX regression (no in-app way to make a previously-public membership private) | User explicitly asked to remove these buttons. Already noted in PR "Out of scope". Flag is for product, not code. |
| R6 | Functional #5 | Safari/Firefox quirks of the transparent-select pattern | Reviewer said "not worth fixing pre-merge". Will be caught by manual smoke before staging→main if it surfaces. |

## Round 2?

Per workflow rule: "Run a follow-up round only if round 1 surfaced ≥5 substantive items." Substantive items here (Important): 2 (A5/A6, the AT/keyboard improvements). The rest are nits or perf polish. **Skipping round 2.**
22 changes: 22 additions & 0 deletions docs/groups-list-improvements/review-round-2.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# Review round 2 — feat/groups-list-improvements

Two reviewers (code quality, functional/UX) ran against the round-1 fix commit. Both verdicts: `ship`. One **Important** item flagged independently by both reviewers; the rest are nits.

## Accepted

| # | Source | Item | Action |
|---|---|---|---|
| B1 | Functional Important + Code quality nit | `aria-label="Sort groups, current: <label>"` is redundant — native `<select>` already announces the selected `<option>` after the role | Revert to `aria-label="Sort groups"`. Keep the `title` attribute (different audience: sighted keyboard/mouse). |
| B2 | Both reviewers (nit) | Stale comment "reuse the lowercased label" — the label is no longer lowercased | Update comment + add a note that ES2019 `Array.prototype.sort` is stable so equal keys don't need an explicit tiebreak |
| B3 | Code quality nit | Two `as SortMode` casts inside the `onChange` validator | Extract a `isSortMode(v): v is SortMode` type predicate; `SORT_VALUES` widened to `ReadonlySet<string>` so the predicate's input doesn't need a cast |

## Rejected

| # | Source | Item | Rationale |
|---|---|---|---|
| C1 | Functional nit | Restructure the NaN sentinel: `const d = …; ts = d && !Number.isNaN(d.getTime()) ? d.getTime() : null` | Equivalent behavior, style preference. Current form is two lines and self-documents via `Number.isNaN(t) ? null : t`. |
| C2 | Code quality nit | Trim the `?? ""` defensive fallback on `currentSortLabel` | `?? ""` is harmless and keeps the type as `string` rather than `string \| undefined`, simplifying interpolation. Defensive but cheap. |

## Round 3?

Substantive items in round 2: 1 (B1). Below the ≥5 threshold for a follow-up round per the workflow rule. Both reviewers explicitly returned `ship` verdicts. **Stopping here.**
122 changes: 47 additions & 75 deletions src/app/globals.css
Original file line number Diff line number Diff line change
Expand Up @@ -4062,6 +4062,47 @@ h1, h2, h3, h4, h5, h6 {
border-radius: var(--radius);
}

.org-list__header-right {
display: flex;
align-items: center;
gap: 8px;
}

.org-list__sort-icon-btn {
position: relative;
display: inline-flex;
align-items: center;
justify-content: center;
width: 28px;
height: 28px;
border-radius: var(--radius);
color: var(--color-mid-gray);
cursor: pointer;
transition: background var(--transition-fast), color var(--transition-fast);
}

.org-list__sort-icon-btn:hover {
background: var(--color-off-white);
color: var(--color-primary);
}

.org-list__sort-icon-btn:focus-within {
outline: 2px solid var(--color-accent);
outline-offset: 2px;
}

.org-list__sort-icon-select {
position: absolute;
inset: 0;
width: 100%;
height: 100%;
opacity: 0;
cursor: pointer;
appearance: none;
border: none;
background: transparent;
}

.org-list__items {
display: flex;
flex-direction: column;
Expand Down Expand Up @@ -4111,6 +4152,12 @@ h1, h2, h3, h4, h5, h6 {
white-space: nowrap;
}

.org-list__item-meta {
font-size: 0.6875rem;
color: var(--color-mid-gray);
margin-top: 2px;
}

.org-list__item-role {
font-size: 0.6875rem;
font-weight: 500;
Expand All @@ -4135,81 +4182,6 @@ h1, h2, h3, h4, h5, h6 {
justify-content: center;
}

.org-list__divider {
display: flex;
align-items: center;
gap: 12px;
padding: 16px 0 8px;
margin-top: 4px;
}

.org-list__divider::before,
.org-list__divider::after {
content: "";
flex: 1;
height: 1px;
background: var(--border-subtle);
}

.org-list__divider-text {
font-size: 0.75rem;
color: var(--color-mid-gray);
white-space: nowrap;
}

.org-list__accept-btn {
display: flex;
align-items: center;
justify-content: center;
width: 28px;
height: 28px;
border: 1px solid var(--color-success-text);
border-radius: var(--radius);
background: transparent;
color: var(--color-success-text);
cursor: pointer;
transition: background 0.15s, color 0.15s;
}

.org-list__accept-btn:hover {
background: var(--color-success-text);
color: #fff;
}

.org-list__accept-btn:disabled {
opacity: 0.5;
cursor: not-allowed;
}

.org-list__accept-btn:disabled:hover {
background: transparent;
color: var(--color-success-text);
}

.org-list__remove-public-btn {
display: flex;
align-items: center;
justify-content: center;
width: 28px;
height: 28px;
border: 1px solid var(--border-default);
border-radius: var(--radius);
background: transparent;
color: var(--color-mid-gray);
cursor: pointer;
transition: background 0.15s, color 0.15s, border-color 0.15s;
}

.org-list__remove-public-btn:hover {
border-color: var(--color-warning-text, #b45309);
color: var(--color-warning-text, #b45309);
}

.org-list__remove-public-btn:disabled {
opacity: 0.5;
cursor: not-allowed;
}

.org-list__leave-btn {
display: flex;
align-items: center;
Expand Down
Loading