refactor: decompose CSS, unify dark mode, extract shared modules
Structural cleanup based on thermo-nuclear code quality review: - Extract shared color-picker.css and color-popover.css from badges.css (611→85 lines) - Extract shared lib/presets.js for preset CRUD used by popup and popover - Unify dark mode to single applyDarkModeClass on <html>, removing 4 scattered functions - Fix stale closure in startPostObserver, data-driven reload key check - Remove dead exports, getHelpers wrapper, duplicated colorToHex - Replace DOM expando properties with state array in popup.js Net: -318 lines of duplication, no file over 401 lines. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -0,0 +1,51 @@
|
||||
# Thermo-Nuclear Code Quality Review — Reddit Tweaks Extension
|
||||
|
||||
**Date:** 2026-08-28 01:08
|
||||
**Task:** Deep code quality audit of the full codebase using thermo-nuclear review criteria
|
||||
|
||||
## Verdict
|
||||
|
||||
Not approved — several structural regressions and missed simplification opportunities identified.
|
||||
|
||||
## Key Findings
|
||||
|
||||
### Critical
|
||||
|
||||
1. **~200 lines of CSS duplicated** between `content/styles/badges.css` (lines 409–611) and `popup/popup.css` (lines 287–489). Color picker widget and preset badge styles are copy-pasted verbatim. Fix: extract shared `color-picker.css`.
|
||||
|
||||
2. **`badges.css` (611 lines) conflates three concerns** — badge styling (~85 lines), color popover chrome (~230 lines), and color picker widget (~150 lines). Should be decomposed into separate files.
|
||||
|
||||
### High
|
||||
|
||||
3. **Preset management logic duplicated** across `popup.js` (`renderPopupPresets`, `addPopupPreset`, `deletePopupPreset`) and `color-popover.js` (`renderPresets`, `saveAsPreset`, `removePreset`). Nearly identical CRUD pattern. Fix: extract shared `presets.js` module.
|
||||
|
||||
4. **`colorToHex` identity function duplicated** in both `popup.js` and `color-popover.js`.
|
||||
|
||||
### Medium
|
||||
|
||||
5. **Dark mode class management scattered** across 4 files with inconsistent scoping — `cards.js` targets `<html>`, `badges.js` targets each container, `color-popover.js` targets itself. Code-judo: apply `rt-dark` once on `<html>` and let CSS cascade handle the rest.
|
||||
|
||||
6. **DOM expando properties** (`entry._colorState`, `entry._nameInput`, etc.) used as state management in `popup.js`. Should use plain objects.
|
||||
|
||||
7. **Stale closure** in `startPostObserver` — line 47 uses captured `settings` instead of freshly-loaded `updated`.
|
||||
|
||||
### Low
|
||||
|
||||
8. **Dead exports** (`isDarkMode`, `getColorPrefix`) on `badgeTweak` object.
|
||||
9. **Verbose storage change listener** — four repetitive conditions should be data-driven.
|
||||
10. **`getHelpers()` wrapper** in `color-popover.js` adds pointless indirection.
|
||||
|
||||
## Recommended Priority Order
|
||||
|
||||
1. Extract shared `color-picker.css` (~200 lines deleted)
|
||||
2. Split `badges.css` into badges, popover, picker
|
||||
3. Extract shared preset module
|
||||
4. Unify dark mode to single `html.rt-dark` class
|
||||
5. Fix stale closure bug
|
||||
6. Clean up dead exports, verbose conditions, and unnecessary wrappers
|
||||
|
||||
## Lessons Learned
|
||||
|
||||
- The thermo-nuclear review skill is useful for catching CSS duplication that would otherwise grow silently — CSS files don't get the same refactoring attention as JS.
|
||||
- Browser extension architecture (content scripts vs popup) makes code sharing harder since there's no module system, but shared files loaded via both manifest.json and popup.html `<script>`/`<link>` tags work fine.
|
||||
- IIFE-scoped modules on a global namespace are a reasonable pattern for small extensions but make duplication easy to miss since there's no import graph to inspect.
|
||||
@@ -0,0 +1,80 @@
|
||||
# Thermo-Nuclear Code Quality Refactor — Reddit Tweaks Extension
|
||||
|
||||
**Date:** 2026-08-28 01:23
|
||||
**Task:** Implement all 10 findings from the thermo-nuclear code quality review
|
||||
|
||||
## Changes Made
|
||||
|
||||
### 1. CSS Deduplication — Extracted shared `color-picker.css` (NEW FILE)
|
||||
- Created `content/styles/color-picker.css` (259 lines) containing color picker widget styles and preset badge styles
|
||||
- This file is now shared between the content script (via manifest.json) and the popup (via `<link>` tag)
|
||||
- Eliminated ~200 lines of identical CSS that was previously duplicated between `badges.css` and `popup.css`
|
||||
|
||||
### 2. CSS Decomposition — Extracted `color-popover.css` (NEW FILE)
|
||||
- Created `content/styles/color-popover.css` (256 lines) with popover chrome, buttons, dark mode
|
||||
- Trimmed `badges.css` from 611 lines to 85 lines (badges-only)
|
||||
- Trimmed `popup.css` from 489 lines to 220 lines (popup-specific only)
|
||||
- Updated dark mode selectors from `.rt-color-popover.rt-dark` to `.rt-dark .rt-color-popover` (ancestor form)
|
||||
|
||||
### 3. Shared Preset Module — Extracted `lib/presets.js` (NEW FILE)
|
||||
- Created `lib/presets.js` (57 lines) with `render()`, `save()`, `delete()` functions
|
||||
- Refactored `popup.js` and `color-popover.js` to use shared module
|
||||
- Eliminated ~60 lines of duplicated preset rendering and CRUD logic
|
||||
|
||||
### 4. Dark Mode Unification
|
||||
- Added `applyDarkModeClass(settings)` to `lib/settings.js`
|
||||
- Removed per-container dark class toggling from `badges.js` `applyColorsToContainer()`
|
||||
- Removed `applyDarkMode()` from `cards.js` (was redundant with shared function)
|
||||
- Removed `applyDarkModeToPopover()` and `refreshTheme()` from `color-popover.js`
|
||||
- `main.js` now calls `applyDarkModeClass()` centrally in init, dark-mode observer, and storage listener
|
||||
|
||||
### 5. Stale Closure Fix
|
||||
- Fixed `startPostObserver` in `main.js` to use freshly-loaded `updated` settings instead of stale captured `settings`
|
||||
|
||||
### 6. Data-Driven Storage Listener
|
||||
- Replaced 4 verbose `changes.X && changes.X.newValue !== ...` conditions with `RELOAD_KEYS.some()`
|
||||
|
||||
### 7. Dead Export Cleanup
|
||||
- Removed `isDarkMode` and `getColorPrefix` re-exports from `badgeTweak` object in `badges.js`
|
||||
|
||||
### 8. Deleted `getHelpers()` Wrapper
|
||||
- Removed unnecessary indirection in `color-popover.js`, replaced with direct `window.RedditTweaks.*` calls
|
||||
|
||||
### 9. Inlined `colorToHex`
|
||||
- Removed duplicated `colorToHex` function from both `popup.js` and `color-popover.js`
|
||||
- Replaced all call sites with inline `val || "#ffffff"`
|
||||
|
||||
### 10. DOM Expando Refactor
|
||||
- Replaced `entry._colorState`, `entry._nameInput`, etc. DOM expando pattern in `popup.js`
|
||||
- Introduced `subredditEntries` array of plain state objects
|
||||
- `collectSubredditOverrides()` now iterates the state array instead of querying the DOM
|
||||
|
||||
## Files Changed
|
||||
- `content/main.js` — stale closure fix, data-driven reload keys, central dark mode calls
|
||||
- `content/tweaks/badges.js` — removed dark class toggle, dead exports, refreshTheme call
|
||||
- `content/tweaks/cards.js` — removed applyDarkMode, use shared function
|
||||
- `content/tweaks/color-popover.js` — removed getHelpers, colorToHex, applyDarkModeToPopover, refreshTheme, preset duplication
|
||||
- `lib/settings.js` — added applyDarkModeClass
|
||||
- `popup/popup.js` — inlined colorToHex, used shared presets, state array instead of expandos
|
||||
- `popup/popup.css` — trimmed to popup-only styles
|
||||
- `content/styles/badges.css` — trimmed to badge-only styles (611 → 85 lines)
|
||||
- `manifest.json` — added new CSS and JS files
|
||||
- `popup/popup.html` — added links to new CSS and JS files
|
||||
|
||||
## New Files
|
||||
- `content/styles/color-picker.css` — shared picker + preset badge styles
|
||||
- `content/styles/color-popover.css` — popover chrome styles
|
||||
- `lib/presets.js` — shared preset CRUD module
|
||||
|
||||
## Net Impact
|
||||
- **Before:** 3,180 lines across 12 source files
|
||||
- **After:** 2,862 lines across 15 source files
|
||||
- **Deleted:** ~318 lines of duplication and dead code
|
||||
- No file exceeds 401 lines (previously badges.css was 611)
|
||||
- Dark mode is now managed from a single source instead of 4 scattered functions
|
||||
|
||||
## Lessons Learned
|
||||
- CSS duplication across content script and popup contexts is easy to miss because there's no import graph. Shared CSS files referenced by both manifest.json and popup.html `<link>` work well.
|
||||
- Dark mode class management is a natural candidate for centralization — scattered per-element toggling creates CSS selector complexity that cascading from a single ancestor eliminates.
|
||||
- DOM expando properties work but are an anti-pattern that makes code harder to reason about. A separate state array with DOM references is only marginally more code but significantly clearer.
|
||||
- The `colorToHex` "conversion" function was really just null-coalescing — renaming or inlining it makes the intent obvious.
|
||||
Reference in New Issue
Block a user