Skip to content

feat: add cat profiles with personality customization#3

Open
EunHyeokJung wants to merge 6 commits intoeunseo9311:mainfrom
EunHyeokJung:feat/cat-profile
Open

feat: add cat profiles with personality customization#3
EunHyeokJung wants to merge 6 commits intoeunseo9311:mainfrom
EunHyeokJung:feat/cat-profile

Conversation

@EunHyeokJung
Copy link
Copy Markdown
Contributor

@EunHyeokJung EunHyeokJung commented Apr 2, 2026

Summary

This PR adds persistent cat profiles with personality presets, lets users switch the active desktop cat, and refreshes the cat settings UI around a profile-first flow.

Changes

  • added persisted cat profiles with active profile selection and normalization for legacy data
  • added Tauri commands for cat profile create, update, delete, and activation
  • wired the active profile into the desktop cat state so color and personality update live
  • added personality presets that affect speech bubbles, idle behavior weights, coding chatter cadence, and AI chat tone
  • replaced the cat settings section with a horizontal profile strip, profile editor, and profile activation flow
  • polished the profile strip UX so New Cat appears first, the active cat appears second, and profile name edits save without relying on input blur alone

Example Image

image

Testing

  • npm run build
  • cargo check -p commit-cat
  • cargo test -p commit-cat --lib

@EunHyeokJung EunHyeokJung marked this pull request as ready for review April 2, 2026 17:55
@EunHyeokJung
Copy link
Copy Markdown
Contributor Author

Follow-up discussion needed on how this should relate to Companions.

This PR adds two things for the main cat only: multiple saved cat profiles and personality presets. But if Companions is effectively the feature for showing multiple cats on screen, we should decide whether these systems should be connected in a later iteration.

Questions worth aligning on:

  • Should each companion be assigned one of the saved cat profiles?
  • Or should profiles remain main-cat-only, while companions keep the current lightweight behavior?
  • If they should connect, what is the source of truth when profile count and companion count differ?

I left this PR scoped to main-cat-only for now, but I think the product direction between Cat Profiles and Companions should be clarified before building on top of either.

Copy link
Copy Markdown
Owner

@eunseo9311 eunseo9311 left a comment

Choose a reason for hiding this comment

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

Review: feat: add cat profiles with personality customization

Nice feature addition! The personality system is well-structured and the profile-first flow makes sense. Here are findings from the review.


👍 Things Done Well

  1. Correct architectural placement. cat_profile.rs is in crates/commit-cat-core/src/models/ — exactly where domain models belong. Great job following the pattern from PR #2.

  2. Solid backward compatibility. Every new field uses #[serde(default)], and normalize() ensures data invariants hold even when deserializing legacy files.

  3. Good test coverage. Tests for normalization, CRUD operations, edge cases (delete last profile, invalid active ID) are all present.

  4. Clean personality extraction. personality.ts uses a data-driven approach with weighted behavior tables and per-personality message maps. Easy to extend with new personalities.

  5. Debounced profile name saving. The 350ms debounce with onBlur flush is a thoughtful UX detail.

  6. Event-driven profile propagation. Using emit("cat-profile:changed") for cross-window communication is the right pattern.


🔴 Critical — Potential panic in active_cat_profile methods

pub fn active_cat_profile(&self) -> &CatProfile {
    self.cat_profiles
        .iter()
        .find(|profile| profile.id == self.active_cat_profile_id)
        .unwrap_or(&self.cat_profiles[0])  // panics if vec is empty
}

Both active_cat_profile() and active_cat_profile_mut() fall back to self.cat_profiles[0], which will panic with index-out-of-bounds if cat_profiles is empty. While normalize() is designed to prevent this, these methods are public and can be called before normalization.

Fix: Use .first() or return Option:

pub fn active_cat_profile(&self) -> Option<&CatProfile> {
    self.cat_profiles
        .iter()
        .find(|p| p.id == self.active_cat_profile_id)
        .or_else(|| self.cat_profiles.first())
}

🟡 High — Profile ID collision risk

new_profile_id() uses nanosecond timestamp only. Rapid sequential calls can produce identical IDs on platforms where SystemTime resolution is coarser than nanoseconds.

Fix: Add a random component:

fn new_profile_id() -> String {
    let ts = SystemTime::now().duration_since(UNIX_EPOCH).map(|d| d.as_nanos()).unwrap_or(0);
    let rand: u32 = rand::random();
    format!("cat-profile-{}-{}", ts, rand)
}

🟡 High — Rebase needed onto current main

This PR will have merge conflicts in at least 4 files:

  • crates/commit-cat-core/src/models/settings.rs (birthday, hat fields)
  • src/components/settings/Settings.tsx (hat inventory, birthday UI, companions selector)
  • src/components/cat/Cat.tsx (hat rendering, grab sprite, item debug)
  • src/stores/catStore.ts (hat state, companions)

Please rebase onto current main before merge.


🟢 Medium — Missing migration for legacy catColor

The PR removes catColor from the frontend SettingsPayload but doesn't migrate the existing settings.catColor value into the default profile's color. Users will lose their previous color choice on upgrade.

Fix: In normalize(), check if legacy settings.cat_color exists and apply it to the default profile.


Overall this is a solid contribution — the personality system adds real character to the cats. The main blockers are the panic risk and rebase. Looking forward to the updated version! 🐱

@EunHyeokJung
Copy link
Copy Markdown
Contributor Author

Additional UX note after local review:

Because the profile strip is currently ordered as New Cat -> active profile -> the rest, clicking a different profile causes the newly active tile to jump into the second slot immediately. Functionally it works, but the visual movement is a little abrupt.

I left the current ordering in place for now, but I think it would be good to align on whether we want to keep this "active profile pinned near the front" behavior, or preserve stable tile order and show active state only with a badge/ring.

@EunHyeokJung EunHyeokJung requested a review from eunseo9311 April 8, 2026 18:26
@eunseo9311
Copy link
Copy Markdown
Owner

Good call on flagging this. Let's go with stable tile order + active badge/ring.

Reasons:

  • Users build spatial memory of where each profile is. Shuffling tiles on activation breaks that.
  • A green ring or "Active" badge on the tile is clear enough — no need for positional emphasis.
  • Stable order also simplifies the code (no re-sorting on every activation).

So the strip should be: [New Cat] [Profile 1] [Profile 2] [Profile 3] ... — always in creation order, with the active one highlighted via a visual indicator (border ring or badge), not by position.

Please update the ordering logic in the next push.

@EunHyeokJung
Copy link
Copy Markdown
Contributor Author

Updated in c8fcaeb.

The profile strip now keeps a stable order instead of moving the active profile toward the front. New Cat stays first, the rest remain in their existing order, and the active profile is indicated by the badge rather than position.

Please take another look when you have a chance.

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.

2 participants