Enhance commit message suggestion engine and improve repo health UX#66
Conversation
Princelad
merged commit afd8b3d
into
Pre-Usable-Release-(stabilization-+-onboarding)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f06b3f28f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Err(err) = app.store.load_from_json(wd) { | ||
| startup_diagnostics.push(format!(".forge load diagnostic: {}", err)); | ||
| } |
There was a problem hiding this comment.
When app.store.load_from_json(wd) fails here, startup only records a diagnostic and continues, but later in the same App::new flow the auto-populate path still calls save_to_json, which rewrites .forge/modules.json, .forge/developers.json, and schema.json from in-memory defaults. In repos where .forge/schema.json is malformed or from a newer version, this turns a recoverable load error into silent data loss by overwriting existing persisted project data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds stronger startup-time configuration/persistence diagnostics and improves UX around repository “health” by making schema/migration/keybinding issues visible and providing a default keymap profile users can copy to override bindings.
Changes:
- Aggregate startup diagnostics for
.forgemigration/load, settings validation, and keybindings load. - Introduce
.forgeschema manifest + migration routine and persist schema version on save. - Generate a default keybindings profile file and document keymap override workflow.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.rs | Collects and surfaces startup diagnostics; adds explicit settings JSON schema validation. |
| src/key_handler.rs | Tightens keybindings TOML parsing, writes a default keymap profile, and adds tests. |
| src/data.rs | Adds .forge schema manifest versioning/migration and tests migration behavior. |
| README.md | Documents keymap overrides via .forge/keybindings.default.toml → .forge/keybindings.toml. |
| PROGRESS.md | Marks configuration/persistence tasks as completed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| format!( | ||
| "Invalid keybindings schema in {}: {}\nHint: use [bindings] table with action = \"Key\" entries", | ||
| path.display(), | ||
| err | ||
| ), | ||
| ) |
There was a problem hiding this comment.
The keybindings parse error message includes a newline (\nHint: ...). Since this message is later surfaced in the single-line status bar, the newline can break or truncate the UI rendering. Consider keeping the error string single-line (e.g., replace the newline with a | Hint: ... suffix) or emitting the hint separately.
| match data::Store::migrate_forge_schema(wd) { | ||
| Ok(Some(msg)) => startup_diagnostics.push(msg), | ||
| Ok(None) => {} | ||
| Err(err) => startup_diagnostics | ||
| .push(format!(".forge migration diagnostic: {}", err)), | ||
| } | ||
| if let Err(err) = app.store.load_from_json(wd) { | ||
| startup_diagnostics.push(format!(".forge load diagnostic: {}", err)); | ||
| } | ||
| match app.load_settings_from(wd) { | ||
| Ok(Some(settings)) => { | ||
| app.settings = settings; | ||
| } | ||
| Ok(None) => {} | ||
| Err(err) => { | ||
| startup_diagnostics | ||
| .push(format!("settings schema diagnostic: {}", err)); | ||
| } | ||
| } | ||
|
|
||
| if let Err(err) = key_handler::ensure_default_keybindings_profile(wd) { | ||
| startup_diagnostics | ||
| .push(format!("default keymap profile diagnostic: {}", err)); | ||
| } | ||
|
|
||
| if let Err(err) = app.key_handler.load_keybindings_from(wd) { | ||
| let msg = error(&format!("Failed to load keybindings: {}", err)); | ||
| startup_diagnostics.push(format!("keybindings schema diagnostic: {}", err)); | ||
| } | ||
|
|
||
| if !startup_diagnostics.is_empty() { | ||
| let msg = error(&format!( | ||
| "Startup configuration diagnostics | {}", | ||
| startup_diagnostics.join(" | ") | ||
| )); |
There was a problem hiding this comment.
startup_diagnostics mixes successful diagnostics (e.g., a schema migration message) with actual failures, but the combined message is always wrapped with error(...). This will present successful migrations as an error in the status bar. Consider using info(...)/success(...) for non-error diagnostics, or separating warnings/errors from informational startup notes.
| use std::io::Read; | ||
|
|
||
| let dir = workdir.join(".forge"); | ||
| let _ = Self::migrate_forge_schema(workdir)?; |
There was a problem hiding this comment.
Forge schema migration is invoked twice on startup: once explicitly in App::new and again inside Store::load_from_json (which currently discards the migration message). This duplicates work and makes it harder to reason about when/where migration messaging should be surfaced. Consider performing migration in only one place (e.g., have load_from_json return the migration message, or remove the internal call and keep the startup-level call).
| let _ = Self::migrate_forge_schema(workdir)?; |
| if !object | ||
| .get("notifications") | ||
| .is_some_and(serde_json::Value::is_boolean) | ||
| { | ||
| errors.push("notifications must be a boolean".to_string()); | ||
| } | ||
|
|
||
| if !object | ||
| .get("autosync") | ||
| .is_some_and(serde_json::Value::is_boolean) | ||
| { | ||
| errors.push("autosync must be a boolean".to_string()); |
There was a problem hiding this comment.
When notifications/autosync are missing from settings.json, the validation error currently says they "must be a boolean", which is misleading compared to the more specific "missing required field" used for theme. Consider distinguishing between "missing" vs "wrong type" for these fields to make diagnostics actionable.
| if !object | |
| .get("notifications") | |
| .is_some_and(serde_json::Value::is_boolean) | |
| { | |
| errors.push("notifications must be a boolean".to_string()); | |
| } | |
| if !object | |
| .get("autosync") | |
| .is_some_and(serde_json::Value::is_boolean) | |
| { | |
| errors.push("autosync must be a boolean".to_string()); | |
| match object.get("notifications") { | |
| Some(value) if value.is_boolean() => {} | |
| Some(_) => errors.push("notifications must be a boolean".to_string()), | |
| None => errors.push("missing required field: notifications".to_string()), | |
| } | |
| match object.get("autosync") { | |
| Some(value) if value.is_boolean() => {} | |
| Some(_) => errors.push("autosync must be a boolean".to_string()), | |
| None => errors.push("missing required field: autosync".to_string()), |
No description provided.