Progress bar added

[madebybye]

No description provided.

[claude]

Pull Request Review: Progress Bar Added

Summary

This PR adds visual progress bars to the middle divider and implements a danger state (red color) when a player is 30+ points behind, indicating they're at risk of being skunked. It also fixes the skunk animation orientation to display correctly for the skunked player.

---

✅ Strengths

1. Good Feature Addition: The progress bars provide excellent visual feedback for game state
2. Consistent Danger Threshold: Using the same 30-point threshold as the existing skunkDifference constant maintains consistency
3. Proper State Management: The skunkedPlayer property is correctly managed throughout the game lifecycle
4. Clean UI Implementation: The progress bars integrate well into the existing design

---

🐛 Potential Issues

1. Hardcoded Magic Number (Cribbage Scorer/ContentView.swift:116)

let dangerColor = Color(red: 0xEE / 255.0, green: 0x00 / 255.0, blue: 0x00 / 255.0)

The hex color #EE0000 is hardcoded. Consider extracting this to a constant or extension for better maintainability:

extension Color {
    static let dangerRed = Color(red: 0xEE / 255.0, green: 0x00 / 255.0, blue: 0x00 / 255.0)
}

2. Duplicated Danger Logic (Cribbage Scorer/ContentView.swift:119-125)

The danger calculation is duplicated in two computed properties. Consider extracting to a helper method in GameViewModel:

// In GameViewModel
func isDangerOfSkunk(player: Int) -> Bool {
    let playerScore = player == 1 ? player1MainScore : player2MainScore
    let opponentScore = player == 1 ? player2MainScore : player1MainScore
    return opponentScore - playerScore >= skunkDifference
}

3. Fallback Value Risk (Cribbage Scorer/ContentView.swift:205)

SkunkView(skunkedPlayer: viewModel.skunkedPlayer ?? 2)

The fallback to player 2 is arbitrary. This should never be nil when showSkunk is true, but if it is, the fallback could show incorrect orientation. Consider:

• Making skunkedPlayer non-optional and always set when showSkunk is true
• Or add a guard/assertion to ensure it's never nil when used

4. Progress Bar Calculation Duplication (Cribbage Scorer/ContentView.swift:151-172)

The progress bar geometry calculation is duplicated. Consider extracting to a reusable view:

struct ProgressBar: View {
    let score: Int
    let maxScore: Int
    let backgroundColor: Color
    let foregroundColor: Color
    
    var body: some View {
        GeometryReader { geometry in
            HStack(spacing: 0) {
                Rectangle()
                    .fill(foregroundColor)
                    .frame(width: geometry.size.width * CGFloat(min(score, maxScore)) / CGFloat(maxScore))
                Spacer(minLength: 0)
            }
        }
        .background(backgroundColor)
    }
}

---

⚡ Performance Considerations

1. Computed Properties in Body: The player1InDanger and player2InDanger computed properties are recalculated on every view update. Since they depend on @Published properties, this is acceptable and follows SwiftUI best practices.

2. GeometryReader: Using GeometryReader for progress bars is appropriate here, though it does add slight layout overhead. The implementation is clean.

3. Progress Bar Updates: The progress bars will redraw whenever scores change, which is expected behavior. No concerns here.

---

🔒 Security Concerns

No security issues identified. This is UI-only code with no:

• User input validation requirements
• Network communication
• Data persistence vulnerabilities
• Authentication/authorization concerns

---

🧪 Test Coverage

Note: No tests are included in this PR. Consider adding:

1. Unit Tests for Danger State Logic:

   • Test player1InDanger when player 2 is 30+ points ahead
   • Test player2InDanger when player 1 is 30+ points ahead
   • Test neither player in danger when difference < 30

2. ViewModel Tests:

   • Test skunkedPlayer is set correctly when skunk occurs
   • Test skunkedPlayer is cleared on game reset
   • Test skunk detection at exactly 30 point difference (boundary case)

3. UI Tests (if applicable):

   • Verify progress bar width changes with score
   • Verify danger color appears at correct threshold
   • Verify skunk view orientation for each player

---

📋 Recommendations

High Priority:

1. Fix the skunkedPlayer ?? 2 fallback to ensure it's never nil when used
2. Extract duplicated danger logic to GameViewModel

Medium Priority:
3. Extract the dangerColor to a Color extension or theme constant
4. Create a reusable ProgressBar view component

Low Priority:
5. Add unit tests for the new danger state logic
6. Consider adding accessibility labels to progress bars for VoiceOver support

---

📊 Version Bump

The version bump from 2.0.0 to 2.1.0 is appropriate for a minor feature addition. This follows semantic versioning correctly.

---

Overall, this is a solid PR that adds a nice UX improvement to the game! The main suggestions are around reducing code duplication and improving robustness with the optional handling. Great work! 🎉