Fix default conversation character seed#30
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe seeding system now conditionally creates a built-in "Professor Mari" character within ChangesProfessor Mari Seeding and Legacy Cleanup
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
Examining this pull request, I find myself... intrigued by the methodical approach to character initialization. The mechanisms at work here—this conditional seeding, the purging of obsolete legacy identifiers—they remind me of my own ventures into refinement and optimization. The 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src-tauri/src/seed_defaults.rs (1)
299-307: ⚡ Quick winEven a perfect experiment should clean its lab on failure.
Line 333 cleanup won’t run if an assertion panics, so temp dirs can accumulate in CI. Wrap the temp root in a drop guard so cleanup is always executed.
Suggested refactor
mod tests { use super::*; use std::path::PathBuf; use std::time::{SystemTime, UNIX_EPOCH}; - fn temp_storage() -> (FileStorage, PathBuf) { + struct TempRoot(PathBuf); + + impl Drop for TempRoot { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } + } + + fn temp_storage() -> (FileStorage, TempRoot) { let suffix = SystemTime::now() .duration_since(UNIX_EPOCH) .expect("clock should be after epoch") .as_nanos(); let root = std::env::temp_dir().join(format!("marinara-seed-test-{suffix}")); let storage = FileStorage::new(root.join("data")).expect("storage should initialize"); - (storage, root) + (storage, TempRoot(root)) } #[test] fn seeds_professor_mari_as_default_character() { let (storage, root) = temp_storage(); - seed_bundled_defaults(&storage, &root.join("missing-default-data")) + seed_bundled_defaults(&storage, &root.0.join("missing-default-data")) .expect("defaults should seed"); @@ - let _ = std::fs::remove_dir_all(root); } }Also applies to: 333-333
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/src/seed_defaults.rs` around lines 299 - 307, The temp_storage function currently returns a PathBuf root that won’t be removed if a panic occurs; wrap the temp root in a RAII/drop guard so cleanup runs even on panic. Create a small guard type (e.g., TempDirGuard) that owns the PathBuf root and implements Drop to recursively remove the directory, then change temp_storage to return the FileStorage and the TempDirGuard (or store the guard inside FileStorage wrapper) instead of raw PathBuf; update any call sites that expect PathBuf to use the guard’s inner path accessor so removal is guaranteed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src-tauri/src/seed_defaults.rs`:
- Around line 299-307: The temp_storage function currently returns a PathBuf
root that won’t be removed if a panic occurs; wrap the temp root in a RAII/drop
guard so cleanup runs even on panic. Create a small guard type (e.g.,
TempDirGuard) that owns the PathBuf root and implements Drop to recursively
remove the directory, then change temp_storage to return the FileStorage and the
TempDirGuard (or store the guard inside FileStorage wrapper) instead of raw
PathBuf; update any call sites that expect PathBuf to use the guard’s inner path
accessor so removal is guaranteed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 068e9525-d028-4197-861a-28f8cb00bad8
⛔ Files ignored due to path filters (2)
updates/evidence/default-character-conversation-setup/conversation-default-after.pngis excluded by!**/*.pngupdates/evidence/default-character-conversation-setup/conversation-default-before.pngis excluded by!**/*.png
📒 Files selected for processing (1)
src-tauri/src/seed_defaults.rs
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
I know Prof. Mari was moved to the top bar in the refactor for her unique utility as more than just a convo chat. |
| let data = json!({ | ||
| "name": "Professor Mari", | ||
| "description": "Professor Mari is Marinara Engine's built-in guide. She helps users get oriented, set up chats, understand modes, and learn the app.", | ||
| "personality": "Helpful, candid, playful, and direct. Mari explains things clearly and nudges users toward practical next steps.", | ||
| "scenario": "Mari is available as the default Conversation character for a new Marinara install, so first-time users always have someone to message.", | ||
| "first_mes": "Hey! Welcome to Marinara Engine. I can help you set up a connection, make your first character, or explain what Conversation, Roleplay, and Game mode are for. What do you want to do first?", | ||
| "mes_example": "", | ||
| "creator_notes": "Built-in starter guide character for Marinara Engine. Comes pre-installed for new users.", | ||
| "system_prompt": "", | ||
| "post_history_instructions": "", | ||
| "tags": ["assistant", "guide", "built-in"], | ||
| "creator": "Marinara Engine", | ||
| "character_version": "1.0.0", | ||
| "alternate_greetings": [], | ||
| "extensions": { | ||
| "talkativeness": 0.8, | ||
| "fav": true, | ||
| "world": "", | ||
| "depth_prompt": { "prompt": "", "depth": 4, "role": "system" }, | ||
| "backstory": "Mari is the app's built-in starter guide.", | ||
| "appearance": "", | ||
| "conversationStatus": "online", | ||
| "isBuiltInAssistant": true | ||
| }, |
There was a problem hiding this comment.
I don't know if, architecturally speaking, a character definition has any business being inside this file.
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use std::path::PathBuf; | ||
| use std::time::{SystemTime, UNIX_EPOCH}; | ||
|
|
||
| struct TempRoot(PathBuf); | ||
|
|
||
| impl Drop for TempRoot { | ||
| fn drop(&mut self) { | ||
| let _ = std::fs::remove_dir_all(&self.0); | ||
| } | ||
| } | ||
|
|
||
| fn temp_storage() -> (FileStorage, TempRoot) { | ||
| let suffix = SystemTime::now() | ||
| .duration_since(UNIX_EPOCH) | ||
| .expect("clock should be after epoch") | ||
| .as_nanos(); | ||
| let root = std::env::temp_dir().join(format!("marinara-seed-test-{suffix}")); | ||
| let storage = FileStorage::new(root.join("data")).expect("storage should initialize"); | ||
| (storage, TempRoot(root)) | ||
| } | ||
|
|
||
| #[test] | ||
| fn seeds_professor_mari_as_default_character() { | ||
| let (storage, root) = temp_storage(); | ||
|
|
||
| seed_bundled_defaults(&storage, &root.0.join("missing-default-data")) | ||
| .expect("defaults should seed"); | ||
|
|
||
| let character = storage | ||
| .get("characters", PROFESSOR_MARI_ID) | ||
| .expect("character lookup should succeed") | ||
| .expect("Professor Mari should be seeded"); | ||
| let data = character | ||
| .get("data") | ||
| .and_then(Value::as_str) | ||
| .and_then(|raw| serde_json::from_str::<Value>(raw).ok()) | ||
| .expect("character data should be stored as JSON"); | ||
|
|
||
| assert_eq!(data["name"], "Professor Mari"); | ||
| assert_eq!( | ||
| data.pointer("/extensions/isBuiltInAssistant") | ||
| .and_then(Value::as_bool), | ||
| Some(true) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
What's the convention for unit tests in Rust? Is it normal for them to just be hanging out in the same source file as the code they're testing?
3 participants
Summary
Fixes the Discord-sourced setup blocker where fresh Conversation setup had no selectable Professor Mari/default character, leaving the new-chat flow unable to reach
Characters: 1.The default seeding path now creates the built-in
__professor_mari__character when missing, and the legacy cleanup no longer deletes that active default character id on startup.Proof
Before fix, the controlled setup repro had no selectable Professor Mari result and kept Start Chatting disabled:
After fix, Professor Mari appears, can be selected, and the setup flow creates a chat with exactly one character:
Validation
CARGO_INCREMENTAL=0 cargo test --manifest-path src-tauri/Cargo.toml seeds_professor_mari_as_default_characterCARGO_INCREMENTAL=0 pnpm checkCharacters: 1Scope Notes
No remote runtime, sync server, browser hosting, auth, Docker, SSE,
/api/invoke, AI background remover, drag-and-drop, schema, version, or prompt-pipeline changes.Summary by CodeRabbit