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
250 changes: 250 additions & 0 deletions .claude/PRPs/issues/completed/issue-397.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,250 @@
Hello tbrandenburg!

Thanks for your feedback!

Here are the tools I am offering:

**Commands:**
/ghar-commit-push
/ghar-issue-fix
/ghar-issue-investigate
/ghar-maintainability-review
/ghar-resolve-ci-errors
/ghar-review
/ghar-security-check
/ghar-stale-check

**Agents:**
*review

**Usage:**
- Use commands directly: `/command-name`
- Use agents with commands: `*agent-name /command-name PROMPT`

Example: `*review /security-review Please check this code for vulnerabilities`
# Investigation: sidebar-version margin-top uses magic value that may break with header style changes

**Issue**: #397 (https://github.com/tbrandenburg/made/issues/397)
**Type**: BUG
**Investigated**: 2026-04-28T12:00:00Z

### Assessment

| Metric | Value | Reasoning |
| ---------- | ------ | ----------------------------------------------------------------------------------------------------------------------------------- |
| Severity | LOW | Cosmetic CSS issue that only manifests if header styles change; no current user impact since .sidebar-version doesn't exist yet. |
| Complexity | LOW | Single CSS file change (sidebar.css); no integration points or architectural changes needed. |
| Confidence | HIGH | The problematic CSS is explicitly quoted in the issue (#300's proposal); the fix is clearly specified and the codebase patterns are clear. |

---

## Problem Statement

The `.sidebar-version` CSS rule proposed in issue #300 uses `margin-top: -1rem` to tighten spacing with the `.sidebar-header` above it. This negative margin is a magic value that depends implicitly on the header's bottom padding/margin. If `.sidebar-header` styles change (font-size, padding, spacing), the version label could overlap the header or leave an unexpected gap.

---

## Analysis

### Root Cause / Change Rationale

The root cause is a CSS design choice that creates an implicit coupling between `.sidebar-version` and `.sidebar-header` via a negative margin. The fix is to make spacing explicit by controlling it on `.sidebar-header` with `padding-bottom` and removing the negative margin from `.sidebar-version`.

### Evidence Chain

WHY: `.sidebar-version` may overlap `.sidebar-header` or leave unexpected gap if header styles change
↓ BECAUSE: `.sidebar-version` uses `margin-top: -1rem` which is a magic value tied to current header spacing
Evidence: Issue #397 body - `.sidebar-version { margin-top: -1rem; }`

↓ BECAUSE: The negative margin compensates for unspecified bottom spacing on `.sidebar-header`
Evidence: `packages/frontend/src/styles/sidebar.css:15-18` - `.sidebar-header` has no explicit bottom padding/margin

↓ ROOT CAUSE: Spacing between header and version is controlled by a negative margin on the child rather than explicit padding on the parent
Evidence: Issue #397 proposed solution - move spacing control to `.sidebar-header` with `padding-bottom: 0.25rem`

### Affected Files

| File | Lines | Action | Description |
| ----------------------------------------- | ----- | ------ | -------------------------------------------------------------------------------- |
| `packages/frontend/src/styles/sidebar.css` | 15-18 | UPDATE | Add `padding-bottom: 0.25rem` to `.sidebar-header` |
| `packages/frontend/src/styles/sidebar.css` | NEW | UPDATE | Add `.sidebar-version` class without negative margin (when version feature added) |

### Integration Points

- `.sidebar-header` is used in `packages/frontend/src/components/Sidebar.tsx:38`
- The version display will be added to the Sidebar component (per issue #300)
- No other files depend on these CSS classes currently

### Git History

- **Introduced**: Not yet introduced (issue #300 proposed the CSS, not yet merged)
- **Last modified**: `21512a2` - Merge pull request #394 (sidebar.css not changed)
- **Implication**: This is a preemptive fix for code that doesn't exist yet; apply when implementing issue #300

---

## Implementation Plan

### Step 1: Add padding-bottom to .sidebar-header

**File**: `packages/frontend/src/styles/sidebar.css`
**Lines**: 15-18
**Action**: UPDATE

**Current code:**

```css
.sidebar-header {
font-weight: 700;
font-size: 1.5rem;
}
```

**Required change:**

```css
.sidebar-header {
font-weight: 700;
font-size: 1.5rem;
padding-bottom: 0.25rem;
}
```

**Why**: Explicitly controls bottom spacing of header, making it resilient to style changes.

---

### Step 2: Add .sidebar-version class without negative margin

**File**: `packages/frontend/src/styles/sidebar.css`
**Lines**: After `.sidebar-header` block
**Action**: UPDATE (add new rule)

**Current code:**

```css
.sidebar-header {
font-weight: 700;
font-size: 1.5rem;
padding-bottom: 0.25rem;
}
```

**Required change:**

```css
.sidebar-header {
font-weight: 700;
font-size: 1.5rem;
padding-bottom: 0.25rem;
}

.sidebar-version {
font-size: 0.7rem;
text-align: center;
color: var(--muted);
margin-top: 0;
margin-bottom: 0.5rem;
}
```

**Why**: Removes the magic `margin-top: -1rem` value and makes spacing explicit. The `margin-bottom: 0.5rem` provides consistent spacing below the version label.

---

### Step 3: Add version display to Sidebar component (when implementing #300)

**File**: `packages/frontend/src/components/Sidebar.tsx`
**Lines**: After line 38
**Action**: UPDATE

**Current code:**

```tsx
<div className="sidebar-header">MADE</div>
<ul>
```

**Required change:**

```tsx
<div className="sidebar-header">MADE</div>
<div className="sidebar-version">{version}</div>
<ul>
```

**Why**: Displays the version number in the sidebar with properly spaced CSS.

---

## Patterns to Follow

**From codebase - mirror these exactly:**

```css
/* SOURCE: packages/frontend/src/styles/sidebar.css:29-48 */
.nav-link {
display: flex;
align-items: center;
gap: 0.75rem;
padding: 0.75rem 1rem;
border-radius: 0.9rem;
color: var(--muted);
transition: all 0.2s ease;
}
```

CSS patterns use `var(--muted)` for secondary text and consistent spacing values (0.25rem, 0.5rem, 0.75rem, 1rem).

---

## Edge Cases & Risks

| Risk/Edge Case | Mitigation |
| --------------------------------- | --------------------------------------------------------------------- |
| `.sidebar-version` doesn't exist yet | Apply this fix when implementing issue #300 (version visibility) |
| Version text could be long | `text-align: center` handles wrapping; consider `overflow: hidden` |
| Multiple versions displayed | Class is singular; if multiple needed, refactor to use `sidebar-versions` |

---

## Validation

### Automated Checks

```bash
cd packages/frontend && npm run lint
cd packages/frontend && npm run build
```

### Manual Verification

1. Open the app and verify sidebar header "MADE" has consistent spacing below it
2. Verify version label (when added) doesn't overlap with header
3. Change `.sidebar-header` font-size or padding and confirm no overlap occurs

---

## Scope Boundaries

**IN SCOPE:**

- Updating `.sidebar-header` CSS with explicit `padding-bottom`
- Adding `.sidebar-version` CSS without negative margin
- Ensuring spacing is resilient to header style changes

**OUT OF SCOPE (do not touch):**

- Implementing the full version display feature (issue #300)
- Adding version API endpoint (backend)
- Changing other sidebar styles (.nav-link, .sidebar ul, etc.)
- Modifying Sidebar.tsx component (defer to #300 implementation)

---

## Metadata

- **Investigated by**: GHAR
- **Timestamp**: 2026-04-28T12:00:00Z
- **Artifact**: `.ghar/issues/issue-397.md`
- **Related Issues**: #300 (version visibility feature)
4 changes: 3 additions & 1 deletion packages/frontend/src/styles/sidebar.css
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
.sidebar-header {
font-weight: 700;
font-size: 1.5rem;
padding-bottom: 0.25rem;
}

.sidebar ul {
Expand Down Expand Up @@ -65,5 +66,6 @@
font-size: 0.7rem;
text-align: center;
color: var(--muted);
margin-top: -1rem;
margin-top: 0;
margin-bottom: 0.5rem;
}