Persistent named palettes with tabbed drawer, export, and UX polish

[superhighfives]

Replaces the flat auto-history drawer with a tabbed palette system: a persistent auto-history palette at index 0, plus user-created named palettes. Several behavioral bugs were caught and fixed across iterative review.

Data layer

• New Palette model (id, name?, pairs, createdAt); auto-history is palettes[0] where name == nil
• migrateHistoryToPalettes() merges legacy colorHistory into palettes[0].pairs rather than overwriting
• Defaults[.activePaletteIndex] tracks the active tab; clamped to 0 on out-of-range

Palette management (Eyedroppers)

• savePalette, renamePalette, deletePalette, addCurrentToPalette, deleteChipFromPalette
• swap() only calls pushUndo() when on auto-history (index 0) — named-palette swaps no longer corrupt the undo stack
• savePalette sets activeHistoryID = current.id so the new chip is selected immediately
• deleteChipFromPalette nils activeHistoryID when the removed pair was active
• deletePalette calls restoreAutoHistorySelection() after clamping to index 0, and decrements activePaletteIndex when deleting a palette before the active one

Drawer UI

• PaletteTabBar with inline create/rename via PaletteNameField; auto-history tab shows a clock icon
• Tab switch uses directional vertical slide — slideDirection is set via @Binding in the button action before activePaletteIndex changes, avoiding the one-frame-behind issue that onChange caused
• isAutoHistory derived from activePalette?.isAutoHistory ?? false; activePaletteIndex is clamped back to 0 async when out of range
• Context-menu remove label switches between textHistoryRemove and textPaletteRemoveChip based on isAutoHistory

Export

• Exporter.paletteToJSON serialises pairs; auto-history entries include an ISO 8601 date field
• ISO8601DateFormatter hoisted out of the pairs.map closure (one allocation per export, not per pair)
• Filename strips /, :, \ before NSSavePanel; write errors surface via NSAlert

UI polish

• Toolbar tooltips include keyboard shortcuts: preview (P), compliance (C), history (H)
• Compliance button: checkmark.shield.fill when active vs checkmark.shield when inactive

[Copilot]

Pull request overview

This PR updates Pika’s palette/history UX by introducing persistent palettes (including JSON export), improving keyboard navigation and selection behavior, and polishing several UI interactions/animations (preview pill stroke, swap hover/rotation, drawer sizing), plus a new “compliance” toggle.

Changes:

• Add palette model + Defaults storage, migrate existing history, and update history drawer UI into a palette-tabbed drawer with inline naming/rename/delete and add-to-palette actions.
• Add JSON export for palettes/history via menu items/context menus and an export save panel flow.
• Add compliance visibility toggle (menu + keyboard shortcut) and multiple UI/animation refinements (preview pill strokeBorder, swap hover cleanup, window sizing).

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
Pika/Views/NavigationMenuItems.swift	Adds palette creation/export and compliance toggle actions to the menu.
Pika/Views/NavigationMenu.swift	Adds toolbar compliance toggle and keyboard shortcut registration.
Pika/Views/HelpView.swift	Documents the new compliance shortcut.
Pika/Views/ContentView.swift	Wires compliance visibility + palette export save panel handling.
Pika/Views/ColorPreview.swift	Fixes preview pill stroke clipping and adjusts swap preview animations/behavior.
Pika/Views/ColorPickers.swift	Removes clipping to prevent UI elements from being cut off.
Pika/Views/ColorHistoryDrawer.swift	Reworks history drawer into palette UI (tabs, inline naming, add/clear behavior).
Pika/Services/WindowCoordinator.swift	Adjusts window minimum/ideal height to fit updated UI.
Pika/Services/Eyedroppers.swift	Moves history storage to palettes, adds palette management, updates navigation/selection logic.
Pika/Services/Exporter.swift	Adds palette/history JSON export helper.
Pika/Services/ColorPair.swift	Introduces Palette model to support palette persistence.
Pika/Constants/Defaults.swift	Adds new defaults for palettes, active palette index, and compliance visibility.
Pika/Constants/Constants.swift	Adds notifications + localized text keys for palettes/compliance/export.
Pika/ButtonStyles/SwapButtonStyle.swift	Improves hover/task cleanup when swap UI hides.
Pika/Assets/en.lproj/Localizable.strings	Adds/updates strings for palettes, export, and compliance toggle.
Pika/AppDelegate.swift	Adds migration from colorHistory to palettes and compliance toggle action.

---

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

[superhighfives]

Want to do a follow up review, @copilot?

[Copilot]

Want to do a follow up review, @copilot?

Sure! Here's the follow-up review on the current state of the code:

---

✅ All previous feedback addressed correctly

The seven issues from the first pass are cleanly resolved. Here are three new observations:

---

1. savePalette doesn't set activeHistoryID — swap won't update the new palette entry

After savePalette(name:) creates a new palette and switches to it, activeHistoryID still holds whatever ID it had before. The new palette's only pair (current) is never selected, so:

• No chip is highlighted on the new palette tab
• Triggering swap won't update the stored hex in that pair (the guard let id = activeHistoryID in swap() finds no match)

Fix: add activeHistoryID = current.id at the end of savePalette:

func savePalette(name: String) {
    let current = ColorPair(id: UUID(), ...)
    ...
    Defaults[.activePaletteIndex] = palettes.count - 1
    activeHistoryID = current.id   // ← add this
}

---

2. applyEntry records to auto-history on every palette navigation

When activePaletteIndex != 0, applyEntry(_:) calls recordHistory() before setting activeHistoryID. This means clicking or keyboard-navigating through a named palette adds every visited pair to auto-history. The dedup guard stops consecutive identical pairs, but browsing through a varied palette will populate auto-history. Whether this is intentional (palette browsing leaves a trail) is worth confirming.

---

3. deleteChipFromPalette leaves a stale activeHistoryID

If the user right-click-removes the currently highlighted chip from a named palette, activeHistoryID keeps pointing to the deleted pair's ID. hasActiveSelection returns false so navigation is unaffected, but the stale ID means a subsequent swap would still attempt to find—and fail to find—the deleted pair. Fix: nil out activeHistoryID when removing the active chip:

func deleteChipFromPalette(paletteIndex: Int, pairID: UUID) {
    var palettes = Defaults[.palettes]
    guard paletteIndex > 0, paletteIndex < palettes.count else { return }
    palettes[paletteIndex].pairs.removeAll { $0.id == pairID }
    Defaults[.palettes] = palettes
    if activeHistoryID == pairID { activeHistoryID = nil }   // ← add this
}

---

Items 1 and 3 are clear bugs worth fixing; item 2 is worth a deliberate decision either way.

[superhighfives]

Thanks for the follow-up review! Fixed #1 and #3:

1. savePalette selection — activeHistoryID = current.id added so the new palette's chip is highlighted and swap works immediately.
2. applyEntry auto-history — Leaving as-is for now (the consecutive dedup is sufficient).
3. deleteChipFromPalette stale ID — activeHistoryID is now nilled out when the deleted chip was the active one.

[superhighfives]

One more time, @copilot!

[Copilot]

One more time, @copilot!

Here's the third-pass review:

---

✅ Previous items resolved

Both fixes from the last round look correct — activeHistoryID = current.id in savePalette and the nil-out in deleteChipFromPalette are clean.

---

New observations

1. slideTransition direction is always one change behind

slideDirection is updated in .onChange(of: activePaletteIndex), but SwiftUI evaluates the view body (and captures the transition) before onChange fires. So when the user taps a palette tab:

1. SwiftUI re-renders the body, reading the old slideDirection
2. The .id(activePalette?.id)/.transition(slideTransition) captures the stale direction
3. onChange fires and updates slideDirection — too late for this animation, too early for the next

The practical effect: every transition shows the direction of the previous index change, not the current one.

Fix: set slideDirection in the same action closure that sets activePaletteIndex, rather than in onChange. The simplest approach is to pass $slideDirection as a @Binding into PaletteTabBar and set it in the tab button action before mutating activePaletteIndex:

// in paletteTab(palette:index:)
Button(action: {
    slideDirection = index > activePaletteIndex ? .down : .up  // ← set first
    activePaletteIndex = index
    if index == 0 { eyedroppers.restoreAutoHistorySelection() }
})

---

2. deletePalette leaves activeHistoryID stale after auto-clamping to index 0

When the active palette is deleted, deletePalette clamps activePaletteIndex to 0 (auto-history), but activeHistoryID still points to a pair from the deleted palette. Unlike deleteChipFromPalette (which now nils the ID), deletePalette doesn't call restoreAutoHistorySelection() or reset the ID. The result: the auto-history tab shows no chip selected and the first keyboard navigation jumps to first/last rather than the most recent auto-history entry.

Fix: after clamping activePaletteIndex to 0 in deletePalette, call restoreAutoHistorySelection():

func deletePalette(at index: Int) {
    ...
    if Defaults[.activePaletteIndex] >= palettes.count {
        Defaults[.activePaletteIndex] = 0
        restoreAutoHistorySelection()   // ← add this
    }
}

---

3. ISO8601DateFormatter() allocated per-pair in paletteToJSON

Inside Exporter.paletteToJSON, a new ISO8601DateFormatter() is constructed on every iteration of pairs.map. ISO8601DateFormatter is expensive to instantiate. Hoist it out of the closure:

let formatter = ISO8601DateFormatter()
let entries: [[String: String]] = pairs.map { pair in
    var entry: [String: String] = [...]
    if isHistory { entry["date"] = formatter.string(from: pair.date) }
    return entry
}

---

Items 1 and 2 are behavioral bugs worth fixing before merge; item 3 is a minor performance fix.

[superhighfives]

All three addressed:

1. Slide direction timing — slideDirection is now set in the PaletteTabBar button action before activePaletteIndex changes, via a @Binding. Removed the stale onChange handler.
2. deletePalette stale selection — restoreAutoHistorySelection() is now called after clamping to index 0.
3. ISO8601DateFormatter per-pair — hoisted out of pairs.map so it's only allocated once.

[superhighfives]

Last one I promise, @copilot.

[Copilot]

Last one I promise, @copilot.

Here's the fourth-pass review:

---

✅ All three previous items resolved cleanly

The slide-direction binding, deletePalette selection restore, and the formatter hoist all look correct.

---

New observations

1. deletePalette doesn't adjust activePaletteIndex when deleting a tab before the active one

Only the "out of bounds" case is handled (clamp to 0). If the user deletes palette at index 2 while index 3 is active, after removal the array is [0, 1, 3, 4] but activePaletteIndex is still 3 — so the UI shifts to what was previously palette 4 instead of staying on the intended one:

func deletePalette(at index: Int) {
    ...
    let currentIndex = Defaults[.activePaletteIndex]
    if currentIndex >= palettes.count {
        Defaults[.activePaletteIndex] = 0
        restoreAutoHistorySelection()
    } else if currentIndex > index {
        Defaults[.activePaletteIndex] = currentIndex - 1   // ← keep pointing at the same palette
    }
}

---

2. savePalette bypasses the slideDirection binding

savePalette writes Defaults[.activePaletteIndex] directly, skipping the tab-button action that now sets slideDirection. A new palette is always appended at the highest index so .down is always the right direction — but if the last tab navigation was .up, the creation animation will use the stale .up direction. Fix: set slideDirection to .down inside savePalette, or pass a closure/callback that the drawer can wire to the binding.

---

3. VerticalDirection is nested in ColorHistoryDrawer but referenced from PaletteTabBar

PaletteTabBar now holds a @Binding var slideDirection: ColorHistoryDrawer.VerticalDirection, which creates a structural dependency from PaletteTabBar on the parent view's private enum. Moving VerticalDirection to the file scope (or a shared type) would remove this coupling — minor but worth tidying.

---

Item 1 is a behavioral bug (active palette jumps to the wrong one); items 2 and 3 are lower-priority polish.

[superhighfives]

All three fixed:

1. deletePalette index shift — when deleting a tab before the active one, activePaletteIndex is now decremented to keep pointing at the same palette.
2. savePalette slide direction — slideDirection is set to .down before calling savePalette so the animation is always correct for appended palettes.
3. VerticalDirection scope — moved to file scope, removing the coupling between PaletteTabBar and ColorHistoryDrawer.