Skip to content

Circular sleep chart displays as if it is a clock#2

Open
Tom4259 wants to merge 10 commits intoDanielJamesTronca:mainfrom
Tom4259:main
Open

Circular sleep chart displays as if it is a clock#2
Tom4259 wants to merge 10 commits intoDanielJamesTronca:mainfrom
Tom4259:main

Conversation

@Tom4259
Copy link

@Tom4259 Tom4259 commented Oct 31, 2025

For example sleeping at 11pm will have the bed icon just to the left of the top middle, and waking up at 7am will show just to the left of the bottom middle

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adjusts the circular sleep chart so its start/end indicators align with clock positions (e.g., sleep starting at ~11pm appears near 11 o’clock), alongside several UI/style and API enhancements.

Changes:

  • Rotates circular chart segments based on the first sample’s start time; updates iconography and default threshold hours.
  • Updates legend layout to be more responsive and optionally hide durations; adds a new .timelineNoDurations style.
  • Improves timeline connectors (line width/opacity/offset and stage-to-stage color gradient) and adds Codable conformance to core models; bumps minimum platform versions.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
Sources/SleepChartKit/Utils/SleepChartConstants.swift Tweaks legend spacing and connector styling constants.
Sources/SleepChartKit/Services/SleepStageColorProvider.swift Changes default unspecified-stage color and introduces a custom color provider.
Sources/SleepChartKit/Models/SleepStage.swift Adds Codable conformance.
Sources/SleepChartKit/Models/SleepSample.swift Adds Codable conformance.
Sources/SleepChartKit/Models/SleepChartStyle.swift Adds .timelineNoDurations style.
Sources/SleepChartKit/Components/SleepTimelineGraph.swift Adds stage-aware gradient connectors and adjusts connector geometry.
Sources/SleepChartKit/Components/SleepLegendView.swift Switches to ViewThatFits + LazyHGrid approach; adds hideDurations.
Sources/SleepChartKit/Components/SleepCircularChartView.swift Rotates start angle based on sleep start time; changes default threshold and start icon.
Sources/SleepChartKit/Components/SleepChartView.swift Wires .timelineNoDurations into legend rendering.
Package.swift Raises minimum supported platform versions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +107 to +109
//making the chart look like the sleep starts at it would on a clock
let sleepStart = samples.first!.startDate

Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In sleepSegments, samples.first! is force-unwrapped even though the method already guards against an empty array. Consider using the already-validated first sample from the guard (or guard let sleepStart = samples.first?.startDate else { ... }) to avoid unnecessary force unwraps and keep the control flow clearer.

Copilot uses AI. Check for mistakes.
Comment on lines +130 to 132
public var body: some View {
HStack(spacing: 4) {
// Color indicator circle
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LegendItem uses a hard-coded HStack(spacing: 4) while overall legend spacing is now controlled by SleepChartConstants.legendItemSpacing. Consider introducing a dedicated constant for “within-item” spacing (or reusing an existing one) instead of a magic number so layout tuning remains centralized.

Copilot uses AI. Check for mistakes.
from: prevRect,
to: currentRect
to: currentRect,
fromStage: previousStage,
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the connector rendering call, previousStage is already unwrapped into prevStage, but the code still passes previousStage (optional) into fromStage:. This makes the API/typing noisier than necessary. Consider passing prevStage and changing renderStageConnector to take non-optional SleepStage parameters (since the connector is only drawn when both stages exist).

Suggested change
fromStage: previousStage,
fromStage: prevStage,

Copilot uses AI. Check for mistakes.
context.stroke(
connectorPath,
with: .color(.gray.opacity(SleepChartConstants.connectorOpacity)),
with: .linearGradient(gradient, startPoint: controlPoint1, endPoint: controlPoint2),
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linear gradient for the connector stroke uses controlPoint1/controlPoint2 as the gradient’s start/end points. That makes the gradient direction dependent on curve control points rather than the actual connector endpoints, which can look incorrect when stage durations/positions vary. Consider using startPoint and endPoint for the gradient vector so the color transition aligns with the connector’s start/end stages.

Suggested change
with: .linearGradient(gradient, startPoint: controlPoint1, endPoint: controlPoint2),
with: .linearGradient(gradient, startPoint: startPoint, endPoint: endPoint),

Copilot uses AI. Check for mistakes.
Comment on lines +9 to +12
.iOS(.v16),
.macOS(.v13),
.watchOS(.v9),
.tvOS(.v16)
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR bumps the minimum deployment targets (e.g., iOS 15 → 16, macOS 12 → 13). That’s a breaking change for package consumers; please confirm it’s required (e.g., for ViewThatFits) and call it out explicitly in the PR description / release notes so downstream apps can plan accordingly.

Suggested change
.iOS(.v16),
.macOS(.v13),
.watchOS(.v9),
.tvOS(.v16)
.iOS(.v15),
.macOS(.v12),
.watchOS(.v8),
.tvOS(.v15)

Copilot uses AI. Check for mistakes.
Comment on lines +107 to +118
//making the chart look like the sleep starts at it would on a clock
let sleepStart = samples.first!.startDate

var h: Double = Double(Calendar.current.component(.hour, from: sleepStart))
if h >= 12 { h -= 12 }

let m: Double = Double(Calendar.current.component(.minute, from: sleepStart))

var difference: Double = (360 / 12) * h
difference += (360 / 12) * (m / 60)

currentAngle += difference
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new “rotate chart based on sleep start time” logic changes the circular chart’s core positioning behavior, but there’s no unit coverage validating the resulting start angle (existing tests only assert the view can be created). Consider extracting the angle/segment calculation into a testable helper (internal function/type) and adding tests for representative start times (e.g., 23:00 → near 11 o’clock, 07:00 → near 7 o’clock).

Copilot uses AI. Check for mistakes.
return .indigo
case .asleepUnspecified:
return .purple
return Color(UIColor.lightGray)
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DefaultSleepStageColorProvider uses UIColor.lightGray, but this package declares macOS/watchOS support where UIColor isn’t available. This will fail to compile on non-UIKit platforms; use a cross-platform Color (e.g., .gray/.secondary) or wrap in #if canImport(UIKit)/#elseif canImport(AppKit) with UIColor/NSColor as appropriate.

Suggested change
return Color(UIColor.lightGray)
return .gray

Copilot uses AI. Check for mistakes.
Comment on lines 69 to 72
showLabels: Bool = true,
showIcons: Bool = true,
thresholdHours: Double = 9.0
thresholdHours: Double = 12
) {
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default thresholdHours value was changed to 12, but the stored property comment and initializer docs still say the default is 9 hours. Please update the documentation to match the new default (and/or explain why 12 is now the default).

Copilot uses AI. Check for mistakes.
Comment on lines +51 to +77
var awakeColour: Color
var remColour: Color
var coreColour: Color
var deepColour: Color
var unspecifiedColour: Color
var inBedColour: Color

public init(awake: Color? = nil, REM: Color? = nil, core: Color? = nil, deep: Color? = nil, unspecified: Color? = nil, inBed: Color? = nil) {

self.awakeColour = awake ?? .orange
self.remColour = REM ?? .cyan
self.coreColour = core ?? .blue
self.deepColour = deep ?? .indigo
self.unspecifiedColour = unspecified ?? .purple
self.inBedColour = inBed ?? .gray
}

public func color(for stage: SleepStage) -> Color {

switch stage {

case .awake: return awakeColour
case .asleepREM: return remColour
case .asleepCore: return coreColour
case .asleepDeep: return deepColour
case .asleepUnspecified: return unspecifiedColour
case .inBed: return inBedColour
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CustomSleepStageColorProvider exposes a public API with inconsistent naming (awakeColour/remColour vs the rest of the codebase’s Color terminology) and a parameter label REM that breaks Swift API design guidelines (capitalized acronym as an external label is unusual). Consider renaming to awakeColor, remColor, etc., and using a rem: parameter label for consistency and discoverability.

Suggested change
var awakeColour: Color
var remColour: Color
var coreColour: Color
var deepColour: Color
var unspecifiedColour: Color
var inBedColour: Color
public init(awake: Color? = nil, REM: Color? = nil, core: Color? = nil, deep: Color? = nil, unspecified: Color? = nil, inBed: Color? = nil) {
self.awakeColour = awake ?? .orange
self.remColour = REM ?? .cyan
self.coreColour = core ?? .blue
self.deepColour = deep ?? .indigo
self.unspecifiedColour = unspecified ?? .purple
self.inBedColour = inBed ?? .gray
}
public func color(for stage: SleepStage) -> Color {
switch stage {
case .awake: return awakeColour
case .asleepREM: return remColour
case .asleepCore: return coreColour
case .asleepDeep: return deepColour
case .asleepUnspecified: return unspecifiedColour
case .inBed: return inBedColour
var awakeColor: Color
var remColor: Color
var coreColor: Color
var deepColor: Color
var unspecifiedColor: Color
var inBedColor: Color
public init(awake: Color? = nil, rem: Color? = nil, core: Color? = nil, deep: Color? = nil, unspecified: Color? = nil, inBed: Color? = nil) {
self.awakeColor = awake ?? .orange
self.remColor = rem ?? .cyan
self.coreColor = core ?? .blue
self.deepColor = deep ?? .indigo
self.unspecifiedColor = unspecified ?? .purple
self.inBedColor = inBed ?? .gray
}
@available(*, deprecated, message: "Use init(awake:rem:core:deep:unspecified:inBed:) instead.")
public init(awake: Color? = nil, REM: Color? = nil, core: Color? = nil, deep: Color? = nil, unspecified: Color? = nil, inBed: Color? = nil) {
self.init(
awake: awake,
rem: REM,
core: core,
deep: deep,
unspecified: unspecified,
inBed: inBed
)
}
public func color(for stage: SleepStage) -> Color {
switch stage {
case .awake: return awakeColor
case .asleepREM: return remColor
case .asleepCore: return coreColor
case .asleepDeep: return deepColor
case .asleepUnspecified: return unspecifiedColor
case .inBed: return inBedColor

Copilot uses AI. Check for mistakes.
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