From 4e5699dc891197262046ed980fb1606dc22e6912 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Sun, 23 Aug 2026 06:05:50 +0800 Subject: [PATCH] fix(config): say what is wrong with a config file, and where MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three different mistakes, one answer: { "shell": "/nonexistent/shell" } -> NOT VALID JSON { "font_size": 12,, } -> NOT VALID JSON { "font_size": "big" } -> NOT VALID JSON Two of those *are* valid JSON. They are valid JSON in the wrong shape — a string where a struct goes, a string where a number goes — which is a different mistake with a different fix, and telling that reader their file is not valid JSON sends them hunting for a missing comma that is not missing. serde has already worked out the answer. It names the field, the type it wanted, and the line and column. That went to `log::warn!` and nowhere else, and there is no log unless `TTY7_LOG` is set, so on a default install it went nowhere at all — which is the same shape as the keybinding faults and the clamped settings this tree has already fixed. Now: DOES NOT FIT — invalid type: string "big", expected f32 at line 1 column 20 NOT VALID JSON — key must be a string at line 1 column 19 NOT VALID JSON — EOF while parsing an object at line 1 column 17 `parse_fault` is a helper rather than a field on `LoadOutcome`, which is `Copy` and crosses several call sites that only want the verdict; this is asked once, by a diagnostic, about a file already on disk. The window's notice appends the same detail on its own line — deliberately after the translated sentence rather than inside it, because serde's message is English that is not ours to translate and a placeholder would leave a raw parser string in the middle of a localized one. The notice's own wording needed no change: it says "could not be parsed", which was true of all three. --- CHANGELOG.md | 12 ++++ crates/tty7-cli/src/commands.rs | 26 +++++++-- crates/tty7-core/src/core/config.rs | 89 ++++++++++++++++++++++++++++- src/main.rs | 16 +++++- 4 files changed, 136 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6615845c..5557b500 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -153,6 +153,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **A config file that will not load now says what is wrong and where.** + `doctor` reported "NOT VALID JSON" for three different mistakes, and two of + them were valid JSON: `"font_size": "big"`, or a string where an object goes, + is well-formed JSON that does not fit the shape — a reader told their file + is not valid JSON goes hunting for a missing comma that is not missing. + serde already answers this, naming the field, the type it wanted and the line + and column, and that answer went only to a `log::warn!` — which is nowhere, + since there is no log unless `TTY7_LOG` is set. The two are now told apart, + and both carry the detail: `DOES NOT FIT — invalid type: string "big", + expected f32 at line 1 column 20`. The window's notice appends it too, on its + own line after the translated sentence. + - **A workspace or tab name is stored as one line.** `tty7 tab rename @1 $'one\ntwo'` kept the newline verbatim, and a name is only ever drawn as a label — the layout breaks a label on a newline whatever its wrapping says, diff --git a/crates/tty7-cli/src/commands.rs b/crates/tty7-cli/src/commands.rs index d90c3418..f19c336e 100644 --- a/crates/tty7-cli/src/commands.rs +++ b/crates/tty7-cli/src/commands.rs @@ -1776,11 +1776,27 @@ fn doctor(ctx: &Context, backend: &mut dyn Backend) -> Result { let (config_state, config_ok) = match loaded.1 { tty7_core::core::config::LoadOutcome::Parsed => ("ok".to_string(), true), tty7_core::core::config::LoadOutcome::Absent => ("none yet — the defaults are the config".to_string(), true), - tty7_core::core::config::LoadOutcome::Quarantined => ( - "NOT VALID JSON — kept aside as config.json.corrupt; running on defaults and not saving" - .to_string(), - false, - ), + tty7_core::core::config::LoadOutcome::Quarantined => { + // What failed and where, not just that something did. serde + // already names the field, the type it wanted and the line and + // column; that went to a log nobody has switched on. And a file + // that is valid JSON in the wrong shape — `"font_size": "big"`, a + // string where an object goes — is a different mistake from a + // missing comma, so calling both "not valid JSON" sent half the + // readers hunting for punctuation that was not wrong. + let fault = tty7_core::core::config::parse_fault(); + let what = match &fault { + Some(f) if f.malformed => format!("NOT VALID JSON — {}", f.detail), + Some(f) => format!("DOES NOT FIT — {}", f.detail), + None => "NOT USABLE".to_string(), + }; + ( + format!( + "{what}; kept aside as config.json.corrupt, running on defaults and not saving" + ), + false, + ) + } tty7_core::core::config::LoadOutcome::Unreadable => ( "UNREADABLE — running on defaults and not saving" .to_string(), diff --git a/crates/tty7-core/src/core/config.rs b/crates/tty7-core/src/core/config.rs index cfb1b1a3..daf96fdb 100644 --- a/crates/tty7-core/src/core/config.rs +++ b/crates/tty7-core/src/core/config.rs @@ -917,7 +917,7 @@ impl Config { } } - fn path() -> Option { + pub(crate) fn path() -> Option { config_path("config.json") } } @@ -970,6 +970,47 @@ pub fn config_path(file: &str) -> Option { Some(config_dir()?.join(file)) } +/// Why the config file did not parse, in serde's own words. +/// +/// [`LoadOutcome::Quarantined`] says only *that* it failed, and that is all +/// most callers need. `doctor` needs the rest, because the reader is standing +/// in front of a file they have to fix and the answer is already known: serde +/// names the field, the type it wanted, and the line and column. +/// +/// Until now that went to `log::warn!` and nowhere else — and there is no log +/// unless `TTY7_LOG` is set, so in practice it went nowhere. +/// +/// Read again rather than carried on the outcome: the outcome is `Copy` and +/// crosses several call sites that only want the verdict, and this is asked +/// once, by a diagnostic command, about a file that is already on disk. +pub fn parse_fault() -> Option { + let path = Config::path()?; + let text = std::fs::read_to_string(&path).ok()?; + let error = serde_json::from_str::(strip_bom(&text)).err()?; + Some(ParseFault { + malformed: matches!( + error.classify(), + serde_json::error::Category::Syntax | serde_json::error::Category::Eof + ), + detail: error.to_string(), + }) +} + +/// What [`parse_fault`] found. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ParseFault { + /// True when the bytes are not JSON at all — a missing comma, an unclosed + /// brace. False when they *are* JSON and simply do not fit the shape: + /// `"font_size": "big"`, or a string where an object goes. Those are + /// different mistakes and they send the reader to different places, so + /// calling both "not valid JSON" sends half of them hunting for a comma + /// that is not missing. + pub malformed: bool, + /// serde's message, which already carries the field, the expected type, + /// and `at line N column M`. + pub detail: String, +} + pub fn strip_bom(text: &str) -> &str { text.strip_prefix('\u{FEFF}').unwrap_or(text) } @@ -2508,6 +2549,52 @@ mod tests { assert!(cfg.keybindings.is_empty()); } + /// A config that will not load says what is wrong with it and where. + /// + /// Two mistakes, and they send the reader to different places. A missing + /// comma is not JSON. `"font_size": "big"`, or a string where an object + /// goes, *is* JSON — it simply does not fit — and telling that reader + /// their file is "not valid JSON" sends them hunting for punctuation that + /// is not wrong. `doctor` said exactly that for both, and serde's answer, + /// which names the field, the type it wanted and the line and column, went + /// to a `log::warn!` nobody has switched on. + #[test] + fn a_config_that_will_not_load_says_what_and_where() { + let _guard = lock_config_file(); + pin_config_dir(); + let path = Config::path().expect("a pinned config dir"); + + std::fs::write(&path, br#"{ "font_size": 12,, }"#).unwrap(); + let fault = parse_fault().expect("a file that does not parse has a fault"); + assert!(fault.malformed, "a stray comma is not JSON: {fault:?}"); + assert!( + fault.detail.contains("line 1 column"), + "the reader needs the place: {fault:?}" + ); + + std::fs::write(&path, br#"{ "font_size": "big" }"#).unwrap(); + let fault = parse_fault().expect("a shape mismatch is a fault too"); + assert!( + !fault.malformed, + "this is valid JSON that does not fit, not broken JSON: {fault:?}" + ); + assert!( + fault.detail.contains("f32") && fault.detail.contains("line 1 column"), + "the reader needs the type and the place: {fault:?}" + ); + + std::fs::write(&path, br#"{ "font_size": 12"#).unwrap(); + assert!( + parse_fault().is_some_and(|f| f.malformed), + "a truncated file is not JSON either" + ); + + std::fs::write(&path, br#"{ "font_size": 12 }"#).unwrap(); + assert_eq!(parse_fault(), None, "a file that loads has no fault"); + + let _ = std::fs::remove_file(&path); + } + fn pin_config_dir() { let dir = std::env::temp_dir().join(format!("tty7-covtest-{}", std::process::id())); std::fs::create_dir_all(&dir).ok(); diff --git a/src/main.rs b/src/main.rs index ab783fca..f2af1c71 100644 --- a/src/main.rs +++ b/src/main.rs @@ -212,8 +212,22 @@ fn notify_config_load_failed( let Some(handle) = crate::ui::windows::WindowRegistry::window_for(cx, workspace) else { return; }; + // The sentence says what happened; serde says what to fix. It already + // names the field, the type it wanted, and the line and column, and until + // now it went only to `log::warn!` — which is to say nowhere, since there + // is no log unless `TTY7_LOG` is set. "Fix it and tty7 reloads" is not + // much help to somebody who cannot see which line is wrong. + // + // Appended rather than folded into the translated string: it is serde's + // English and it is not ours to translate, and a placeholder would put a + // raw parser message in the middle of a localized sentence. On its own + // line after it, it reads as what it is — the parser's own words. + let body = match crate::core::config::parse_fault() { + Some(fault) => format!("{}\n\n{}", crate::ui::i18n::t(key), fault.detail), + None => crate::ui::i18n::t(key).to_string(), + }; let _ = handle.update(cx, |_, window, cx| { - window.push_notification(crate::ui::i18n::t(key), cx); + window.push_notification(body, cx); }); }