Skip to content

Life: Fullscreen grid with collapsible sidebar - #184

Merged
evanmcfarland merged 3 commits into
masterfrom
feature/life-fullscreen-sidebar
Dec 15, 2025
Merged

Life: Fullscreen grid with collapsible sidebar#184
evanmcfarland merged 3 commits into
masterfrom
feature/life-fullscreen-sidebar

Conversation

@evanmcfarland

Copy link
Copy Markdown
Member

Summary

  • Grid now takes full screen height (except page header)
  • Combined info + pattern selector in collapsible left sidebar (desktop)
  • Bottom bar for mobile devices
  • Sidebar state persisted to localStorage

Test plan

  • Visit /life route on desktop - sidebar should be visible on left
  • Collapse sidebar - grid expands, state persists on refresh
  • Test on mobile viewport - bottom bar should appear instead
  • Verify zoom controls still work in top-right overlay
  • Verify pattern selection and cell placement still work

Deployed to mainnet:

🤖 Generated with Claude Code

evanmcfarland and others added 3 commits December 15, 2025 11:35
- Grid now takes full screen height (except page header)
- Combined info + pattern selector in collapsible left sidebar (desktop)
- Bottom bar for mobile devices with expandable pattern selection
- Sidebar collapse state persisted to localStorage

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Signed-off-by: Evan McFarland <62383100+evanmcfarland@users.noreply.github.com>
@evanmcfarland
evanmcfarland merged commit 3ef2bdf into master Dec 15, 2025
1 check passed
@claude

claude Bot commented Dec 15, 2025

Copy link
Copy Markdown

Code Review: Life Fullscreen Grid with Collapsible Sidebar

🚨 Critical Issues (P0 - Must Fix)

1. Duplicate and Broken JSX Structure (Lines 910-1063)

The most critical issue: the component has duplicate and conflicting render code that will cause runtime errors.

Problem:

  • Line 912: Opens outer container <div className="flex flex-col h-[calc(100vh-80px)]">
  • Line 914: Opens ANOTHER container <div className="flex flex-col h-[calc(100vh-120px)]"> (different height!)
  • Lines 916-958: Old header code that should have been removed
  • Lines 1004-1061: Duplicate canvas and zoom controls that conflict with lines 973-1003
  • The closing tags don't match the opening tags properly

Impact: This code will either fail to compile or render incorrectly with duplicate elements.

Fix Required: Remove the entire duplicate section (lines 914-1061). The correct structure should be:

return (
  <div className="flex flex-col h-[calc(100vh-80px)]">
    {error && <div>...</div>}
    <div className="flex flex-1 min-h-0">
      <Sidebar />
      <div className="flex-1 flex flex-col relative bg-black">
        {/* Zoom controls */}
        {/* Help text */}
        {/* Canvas */}
      </div>
    </div>
    <MobileBottomBar />
  </div>
);

2. Orphaned/Unreachable Code (Lines 1004-1018)

Lines 1004-1018 contain fragment HTML outside any container - this appears to be leftover code from the old pattern selector that wasn't fully removed.


⚠️ Major Issues (P1 - Should Fix)

3. Missing State Variables

The code references variables that are never defined:

  • currentGameId - used in lines 376, 584, 601, 611, 625, 627
  • games - set in line 242 via setGames() but never declared
  • newGameName - used in lines 256, 257, 276
  • mode - used in lines 275, 309, 320, 425

Impact: TypeScript compilation will fail. These appear to be remnants from a lobby system that was removed.

Fix: Either:

  • Add these state variables back
  • Remove the functions that use them (handleCreateGame, handleJoinGame, handleLeaveGame, fetchGames)
  • Or stub them out if they're needed for future features

4. Placement Error Toast Not Displayed

The placementError state is set (lines 578-595) but the error toast overlay at lines 1026-1031 is in the duplicate/broken section and won't render.

Fix: Add the placement error toast inside the correct canvas container (around line 985).


🔧 Code Quality Issues (P2 - Nice to Have)

5. Inconsistent Height Values

  • Login screen: h-[calc(100vh-80px)] (line 892)
  • Game view: h-[calc(100vh-80px)] (line 912)
  • But also: h-[calc(100vh-120px)] (line 914) in duplicate section

The PLAN document mentions changing from 120px to 80px, but the duplicate code still has 120px. Once duplicates are removed, ensure consistent 80px offset throughout.

6. Sidebar Component Performance

The Sidebar and MobileBottomBar components are defined inside the main component (lines 651-887), causing them to be recreated on every render.

Fix: Move these to useMemo or extract as separate components outside the main component.

7. Missing Error Handling

  • The game loop (lines 364-425) catches errors but only logs them to console
  • Network failures won't be visible to users
  • Consider showing connection status or retry UI

8. Accessibility Issues

  • Sidebar toggle button (line 664) only has a chevron symbol, no aria-label
  • Mobile expand button (line 807) same issue
  • Canvas has no keyboard navigation support

✅ What's Good

  1. LocalStorage Persistence: Sidebar state persistence is well implemented (lines 184-196)
  2. Responsive Design: Good use of Tailwind's lg: breakpoint for desktop/mobile split
  3. Component Organization: Sidebar and MobileBottomBar separation is clean conceptually
  4. Smooth Transitions: transition-all duration-300 on sidebar is nice UX
  5. Color Consistency: Good use of player color constants throughout

🧪 Testing Concerns

Given the duplicate code issue, this PR cannot be tested as-is - it likely won't build or will crash at runtime. The test plan checklist is good but premature until P0 issues are fixed.


📋 Security Review

No security concerns identified. The code:

  • Properly handles authentication via Internet Identity
  • Uses the IC's VRF for randomness (good for fairness)
  • No XSS vulnerabilities in the JSX
  • LocalStorage usage is safe (only storing UI preferences)

🎯 Recommended Action Plan

  1. Immediately: Remove duplicate code (lines 914-1061)
  2. Before merge: Fix missing state variables or remove unused functions
  3. Before merge: Add placement error toast to correct location
  4. Post-merge: Address accessibility and performance optimizations

Summary

Recommendation: ❌ Request Changes

While the UI redesign concept is solid, the implementation has critical structural issues that will prevent it from working. The duplicate render code must be fixed before this can be merged to production.

The good news: once the duplicate code is removed, the core implementation (sidebar, mobile bar, localStorage) appears sound. This should be a straightforward fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant