feat: add cat profiles with personality customization#3
feat: add cat profiles with personality customization#3EunHyeokJung wants to merge 6 commits intoeunseo9311:mainfrom
Conversation
|
Follow-up discussion needed on how this should relate to This PR adds two things for the main cat only: multiple saved cat profiles and personality presets. But if Questions worth aligning on:
I left this PR scoped to main-cat-only for now, but I think the product direction between |
eunseo9311
left a comment
There was a problem hiding this comment.
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
-
Correct architectural placement.
cat_profile.rsis incrates/commit-cat-core/src/models/— exactly where domain models belong. Great job following the pattern from PR #2. -
Solid backward compatibility. Every new field uses
#[serde(default)], andnormalize()ensures data invariants hold even when deserializing legacy files. -
Good test coverage. Tests for normalization, CRUD operations, edge cases (delete last profile, invalid active ID) are all present.
-
Clean personality extraction.
personality.tsuses a data-driven approach with weighted behavior tables and per-personality message maps. Easy to extend with new personalities. -
Debounced profile name saving. The 350ms debounce with onBlur flush is a thoughtful UX detail.
-
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! 🐱
8b1d5cf to
ec31520
Compare
|
Additional UX note after local review: Because the profile strip is currently ordered as 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. |
|
Good call on flagging this. Let's go with stable tile order + active badge/ring. Reasons:
So the strip should be: Please update the ordering logic in the next push. |
|
Updated in The profile strip now keeps a stable order instead of moving the active profile toward the front. Please take another look when you have a chance. |
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
New Catappears first, the active cat appears second, and profile name edits save without relying on input blur aloneExample Image
Testing
npm run buildcargo check -p commit-catcargo test -p commit-cat --lib