Discourse / PR #7

scale-color $lightness must use $secondary for dark themes

marchugon/discourse-cursor-fr-benchmark#11
Golden recall: 66.7% Extra findings: 14

Golden Comments (1/3 found, 1 partial)

# Comment Severity Status FriendlyReviewer Detail
#1 .topic-meta-data h5 a: original $lightness: 30% changed to 70% for the light theme — dramatic inversion LOW ✗ Missed FriendlyReviewer did not specifically identify this selector.
#2 desktop/user.scss + mobile/user.scss: $primary changed from 30% to 50% for the light theme — should preserve original LOW ✓ Found FriendlyReviewer found both occurrences: "original scale-color($primary, $lightness: 30%) was changed to scale-color($primary, $lightness: 50%)" with fix suggestion (desktop/user.scss:520 + mobile/user.scss:495).
#3 topic-post.scss: original $lightness: 70% replaced by 30% in the light theme — text significantly darker LOW ⚠ Partial FriendlyReviewer found the same pattern (70%→30%) in mobile/modal.scss but not in the specific topic-post.scss file referenced by the golden comment.

Supplementary Findings (15 findings)

Finding File Severity Legitimate?
MISSED CONVERSION: .topic-list.categories .category .badge-notification without dark-light-choose _topic-list.scss:93 HIGH ✓ Real bug — dark theme contrast break
BUG: .custom-message-length light theme uses 30% instead of original 70% — visual regression mobile/modal.scss:99 HIGH ✓ Matches golden #3 pattern
Light-theme color shift: $primary 30%→50% for .group-member-info .name desktop/user.scss:520 HIGH ✓ Matches golden #2
Same issue: $primary 30%→50% in the mobile variant mobile/user.scss:495 HIGH ✓ Matches golden #2
Pre-existing: font-size: 15 without CSS unit — ignored by browsers mobile/login.scss:34 HIGH ✓ Real bug
Pre-existing: float: auto is invalid — should be float: none common/base/login.scss:36 HIGH ✓ Real bug
.topic-map h3: desktop uses (20%, 80%) but mobile uses (50%, 50%) — inconsistent desktop/topic-post.scss:321 MEDIUM ✓ Cross-breakpoint inconsistency
Same inconsistency: mobile 50%/50% vs desktop 20%/80% mobile/topic-post.scss:182 MEDIUM ✓ Cross-breakpoint inconsistency
.user-field label width 92px vs 85px between breakpoints common/base/login.scss:22 MEDIUM ✓ Layout discrepancy
.user-field input[type=text] width 220px vs 184px common/base/login.scss:25 MEDIUM ✓ Layout discrepancy
.controls margin-left 92px vs 85px — should match label widths common/base/login.scss:30 MEDIUM ✓ Layout discrepancy
#new-account-link: <a> in mobile vs <button> in base — styles not applied mobile/login.scss:24 MEDIUM ✓ Functional issue
.user-title hardcoded #aaa overrides dark-light-choose — dark theme broken mobile/topic-post.scss:508 MEDIUM ✓ Dark theme regression
.post-controls button.create without dark-light-choose on mobile mobile/topic-post.scss:128 MEDIUM ✓ Inconsistent dark theme support
.topic-map .buttons .btn without dark-light-choose on mobile mobile/topic-post.scss:274 MEDIUM ✓ Inconsistent dark theme support

Honest Analysis

Golden #2 was found perfectly by FriendlyReviewer — both desktop/user.scss:520 and mobile/user.scss:495 with exactly the same analysis as the golden comment. Golden #3 was partially covered: FriendlyReviewer identified the same inversion pattern (70%→30%) in mobile/modal.scss, though not in the specific topic-post.scss file mentioned by the benchmark. Golden #1 (the specific .topic-meta-data h5 a selector) was missed entirely.

FriendlyReviewer discovered a missed dark-light-choose conversion in _topic-list.scss that would cause incorrect contrast in dark themes — a real supplementary bug. It also found 6 pre-existing issues in login styles, including invalid CSS (font-size: 15 without unit, float: auto) that are genuine bugs. The review identified numerous desktop/mobile inconsistencies across login and topic-post styles, many of which represent actual layout problems.

Severity inflation is notable: all findings FriendlyReviewer classified as HIGH would be LOW in the golden benchmark. This is consistent with how FriendlyReviewer treats CSS/scaling inconsistancies — they get the highest severity because they can cause visible breakage, whereas the benchmark considers them cosmetic.

FriendlyReviewer's full review

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

friendly-reviewer bot commented now

Hi there 👏

🌥️ Tech
6 high, 9 medium, 6 low

The Merge Request correctly implements dark theme support by wrapping scale-color($primary, $lightness: X%) calls with dark-light-choose() using $secondary as the dark-theme counterpart. The inversion pattern (secondary lightness = 100 - primary lightness) is consistently applied across most files. However, several critical issues were identified: a missed conversion in _topic-list.scss that will cause incorrect contrast in dark themes, a light-theme visual regression in mobile/modal.scss where the original lightness value was accidentally changed, and unintentional light-theme color shifts in desktop/user.scss and mobile/user.scss. Additionally, multiple pre-existing inconsistencies and bugs were flagged in login and topic-post styles, including invalid CSS units and values, divergent layout approaches, and inconsistent color values between desktop and mobile. A redundant dark-light-choose wrapper in buttons.css.scss should be simplified.

These might need a close look

  • 🔴 app/assets/stylesheets/common/base/_topic-list.scss (L93)
    MISSED CONVERSION: .topic-list.categories .category .badge-notification uses bare scale-color($primary, $lightness: 50%) without dark-light-choose wrapping.
  • 🔴 app/assets/stylesheets/mobile/modal.scss (L99)
    BUG: The .custom-message-length light-theme variant incorrectly uses scale-color($primary, $lightness: 30%) instead of the original 70%.
  • 🔴 app/assets/stylesheets/desktop/user.scss (L520)
    Unintentional light-theme color change: original 30% was changed to 50% for .group-member-info .name.
  • 🔴 app/assets/stylesheets/mobile/user.scss (L495)
    Same issue as desktop/user.scss line 520: original 30% changed to 50%.
  • 🔴 app/assets/stylesheets/mobile/login.scss (L34)
    Pre-existing bug: font-size: 15 missing CSS unit; browsers will ignore.
  • 🔴 app/assets/stylesheets/common/base/login.scss (L36)
    Pre-existing bug: float: auto is invalid; should be float: none.

Worth checking

  • 🟡 app/assets/stylesheets/desktop/topic-post.scss (L321)
    INCONSISTENCY: .topic-map h3 uses (20%, 80%) on desktop but mobile uses (50%, 50%).
  • 🟡 app/assets/stylesheets/mobile/topic-post.scss (L182)
    INCONSISTENCY: .topic-map h3 uses 50%/50% on mobile while desktop uses 20%/80%.
  • 🟡 app/assets/stylesheets/common/base/login.scss (L22)
    INCONSISTENCY: .user-field label width is 92px in common/base but 85px in mobile.
  • 🟡 app/assets/stylesheets/common/base/login.scss (L25)
    INCONSISTENCY: .user-field input[type=text] width is 220px vs 184px.
  • 🟡 app/assets/stylesheets/common/base/login.scss (L30)
    INCONSISTENCY: .controls margin-left is 92px vs 85px.
  • 🟡 app/assets/stylesheets/mobile/login.scss (L24)
    INCONSISTENCY: #new-account-link uses <a> in mobile vs <button> in base.
  • 🟡 app/assets/stylesheets/mobile/topic-post.scss (L508)
    DARK THEME BREAK: .user-title hardcoded #aaa overrides common's dark-light-choose.
  • 🟡 app/assets/stylesheets/mobile/topic-post.scss (L128)
    INCONSISTENCY: .post-controls button.create without dark-light-choose on mobile.
  • 🟡 app/assets/stylesheets/mobile/topic-post.scss (L274)
    INCONSISTENCY: .topic-map .buttons .btn without dark-light-choose on mobile.

Small things (take or leave)

  • 🔵 app/assets/stylesheets/common/components/buttons.css.scss (L85)
    Redundant dark-light-choose: both args are identical.
  • 🔵 app/assets/stylesheets/common/base/login.scss (L17)
    INCONSISTENCY: .user-field.confirm margin-top 20px vs 10px.
  • 🔵 app/assets/stylesheets/desktop/login.scss (L5)
    INCONSISTENCY: #login-buttons button min-width/margin desktop-only.
  • 🔵 app/assets/stylesheets/mobile/topic-post.scss (L270)
    INCONSISTENCY: .topic-map .domain (75%,25%) mobile vs (50%,50%) desktop.
  • 🔵 app/assets/stylesheets/mobile/topic-post.scss (L49)
    generic button (50%,50%) mobile vs (75%,25%) desktop.
  • 🔵 app/assets/stylesheets/desktop/topic-list.scss (L55)
    Extra space before closing brace.