refactor(i18n): store reviewed-unchanged ids as a flat list (#3379)

tools/i18n_auto/src/config.rs

@@ -17,21 +17,8 @@ pub const DEFAULT_REQUEST_TIMEOUT_SECONDS: f64 = 300.0;
 pub const DEFAULT_PROGRESS_INTERVAL_SECONDS: f64 = 20.0;
 pub const GUIDANCE_EXCERPT_CHAR_LIMIT: usize = 1600;
 
-pub const AUTO_I18N_UNCHANGED_COMMENT: &str = "# auto-i18n: reviewed unchanged";
-pub const AUTO_I18N_LEGACY_UNCHANGED_COMMENT: &str = "#. auto-i18n: reviewed unchanged";
-pub const AUTO_I18N_COMMENT_PREFIX: &str = "auto-i18n:";
 pub const AUTO_I18N_REVIEWED_UNCHANGED_FILE: &str = "auto-i18n-reviewed-unchanged.json";
 
-pub fn is_auto_i18n_unchanged_comment(comment: &str) -> bool {
-    let text = comment
-        .trim()
-        .strip_prefix("#. ")
-        .or_else(|| comment.trim().strip_prefix("# "))
-        .unwrap_or_else(|| comment.trim())
-        .trim();
-    text == "auto-i18n: reviewed unchanged"
-}
-
 pub type EnvOverlay = HashMap<String, String>;
 
 pub fn default_app_dir() -> PathBuf {

tools/i18n_auto/src/po.rs

@@ -3,10 +3,6 @@
 use anyhow::{Context, Result, bail};
 use regex::Regex;
 
-use crate::config::{
-    AUTO_I18N_COMMENT_PREFIX, AUTO_I18N_UNCHANGED_COMMENT, is_auto_i18n_unchanged_comment,
-};
-
 #[derive(Clone, Debug, Default, Eq, PartialEq)]
 pub struct Entry {
     pub comments: Vec<String>,
@@ -162,11 +158,7 @@ fn rebuild_block_allow_replacing(
     block: &str,
     translation_map: &[(String, &Translation)],
 ) -> Result<String> {
-    let lines = block
-        .split('\n')
-        .filter(|line| !is_auto_i18n_unchanged_comment(line))
-        .map(str::to_string)
-        .collect::<Vec<_>>();
+    let lines = block.split('\n').map(str::to_string).collect::<Vec<_>>();
     let Some(msgid_range) = field_range(&lines, "msgid") else {
         return Ok(block.to_string());
     };
@@ -185,11 +177,7 @@ fn rebuild_block_allow_replacing(
         return Ok(block.to_string());
     };
 
-    let mut next_lines = Vec::new();
-    if translation.reviewed_unchanged {
-        next_lines.push(AUTO_I18N_UNCHANGED_COMMENT.to_string());
-    }
-    next_lines.extend_from_slice(&lines[..msgstr_range.0]);
+    let mut next_lines = lines[..msgstr_range.0].to_vec();
     next_lines.push(format!("msgstr \"{}\"", escape_po(&translation.msgstr)));
     next_lines.extend_from_slice(&lines[msgstr_range.1..]);
     Ok(next_lines.join("\n"))
@@ -250,11 +238,7 @@ pub fn extract_translator_comments(entry: &Entry) -> Vec<String> {
         .iter()
         .filter_map(|line| line.strip_prefix("#. "))
         .map(str::trim)
-        .filter(|comment| {
-            !comment.is_empty()
-                && !is_auto_i18n_comment(comment)
-                && !is_placeholder_comment(comment)
-        })
+        .filter(|comment| !comment.is_empty() && !is_placeholder_comment(comment))
         .map(str::to_string)
         .collect()
 }
@@ -270,10 +254,6 @@ pub fn extract_placeholder_hints(entry: &Entry) -> Vec<String> {
         .collect()
 }
 
-pub fn is_auto_i18n_comment(comment: &str) -> bool {
-    comment.to_lowercase().starts_with(AUTO_I18N_COMMENT_PREFIX)
-}
-
 pub fn is_placeholder_comment(comment: &str) -> bool {
     comment.to_lowercase().starts_with("placeholder {") && comment.contains("}:")
 }
@@ -290,7 +270,7 @@ mod tests {
 		 msgctxt \"welcome title\"\n\
 		 msgid \"Hello \\\"world\\\"\"\n\
 		 msgstr \"\"\n\n\
-		 #. auto-i18n: reviewed unchanged\n\
+		 #. Delete button label.\n\
 		 #: src/example.tsx:2\n\
 		 msgctxt \"verb\"\n\
 		 msgid \"Delete\"\n\
@@ -330,7 +310,7 @@ mod tests {
     }
 
     #[test]
-    fn rebuilds_by_context_and_manages_unchanged_markers() {
+    fn rebuilds_by_context() {
         let rebuilt = rebuild_po_allow_replacing(
             sample_po(),
             &[
@@ -340,62 +320,28 @@ mod tests {
                     "Bonjour \"monde\"",
                 ),
                 Translation::new(Some("verb".to_string()), "Delete", "Supprimer"),
-                Translation {
-                    msgctxt: Some("keyboard key".to_string()),
-                    msgid: "Delete".to_string(),
-                    msgstr: "Delete".to_string(),
-                    reviewed_unchanged: true,
-                    notes: String::new(),
-                },
+                Translation::new(Some("keyboard key".to_string()), "Delete", "Suppr"),
             ],
         )
         .unwrap();
         let entries = parse_po(&rebuilt).unwrap();
-        assert_eq!(
+        let msgstr = |msgctxt: &str| {
             entries
                 .iter()
-                .find(|entry| entry.msgctxt.as_deref() == Some("welcome title"))
+                .find(|entry| entry.msgctxt.as_deref() == Some(msgctxt))
                 .unwrap()
-                .msgstr,
-            "Bonjour \"monde\""
-        );
-        assert_eq!(
-            entries
-                .iter()
-                .find(|entry| entry.msgctxt.as_deref() == Some("verb"))
-                .unwrap()
-                .msgstr,
-            "Supprimer"
-        );
-        let keyboard = entries
-            .iter()
-            .find(|entry| entry.msgctxt.as_deref() == Some("keyboard key"))
-            .unwrap();
-        assert_eq!(keyboard.msgstr, "Delete");
-        assert!(
-            keyboard
-                .comments
-                .contains(&AUTO_I18N_UNCHANGED_COMMENT.to_string())
-        );
-        assert!(
-            !keyboard
-                .comments
-                .contains(&crate::config::AUTO_I18N_LEGACY_UNCHANGED_COMMENT.to_string())
-        );
-        assert!(
-            !entries
-                .iter()
-                .find(|entry| entry.msgctxt.as_deref() == Some("verb"))
-                .unwrap()
-                .comments
-                .contains(&AUTO_I18N_UNCHANGED_COMMENT.to_string())
-        );
+                .msgstr
+                .clone()
+        };
+        assert_eq!(msgstr("welcome title"), "Bonjour \"monde\"");
+        assert_eq!(msgstr("verb"), "Supprimer");
+        assert_eq!(msgstr("keyboard key"), "Suppr");
     }
 
     #[test]
-    fn untouched_blocks_keep_reviewed_unchanged_markers() {
+    fn untouched_blocks_stay_byte_identical() {
         let content = format!(
-            "{}\n{AUTO_I18N_UNCHANGED_COMMENT}\n#: src/example.tsx:5\nmsgid \"Audio\"\nmsgstr \"Audio\"\n",
+            "{}\n#. Audio settings tab.\n#: src/example.tsx:5\nmsgid \"Audio\"\nmsgstr \"Audio\"\n",
             sample_po()
         );
         let rebuilt = rebuild_po_allow_replacing(
@@ -412,15 +358,9 @@ mod tests {
             .iter()
             .find(|entry| entry.msgctxt.as_deref() == Some("verb"))
             .unwrap();
-        assert_eq!(
-            verb.comments,
-            vec![crate::config::AUTO_I18N_LEGACY_UNCHANGED_COMMENT.to_string()]
-        );
+        assert_eq!(verb.comments, vec!["#. Delete button label.".to_string()]);
         let audio = entries.iter().find(|entry| entry.msgid == "Audio").unwrap();
-        assert_eq!(
-            audio.comments,
-            vec![AUTO_I18N_UNCHANGED_COMMENT.to_string()]
-        );
+        assert_eq!(audio.comments, vec!["#. Audio settings tab.".to_string()]);
         assert_eq!(
             entries
                 .iter()
@@ -436,11 +376,9 @@ mod tests {
     }
 
     #[test]
-    fn reset_clears_msgstr_and_removes_reviewed_markers() {
+    fn reset_clears_msgstr() {
         let reset = reset_po_translations(sample_po()).unwrap();
         let entries = parse_po(&reset).unwrap();
         assert!(entries.iter().all(|entry| entry.msgstr.is_empty()));
-        assert!(!reset.contains(AUTO_I18N_UNCHANGED_COMMENT));
-        assert!(!reset.contains(crate::config::AUTO_I18N_LEGACY_UNCHANGED_COMMENT));
     }
 }

tools/i18n_auto/src/reviewed_unchanged.rs

@@ -10,13 +10,14 @@ use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH};
 use anyhow::{Context, Result, bail};
 use serde::{Deserialize, Serialize};
 
-const REVIEWED_UNCHANGED_VERSION: u32 = 1;
 const LOCK_TIMEOUT: Duration = Duration::from_secs(30);
 const LOCK_POLL_INTERVAL: Duration = Duration::from_millis(50);
 
+type Locales = BTreeMap<String, Vec<ReviewedUnchangedEntry>>;
+
 #[derive(Clone, Debug, Deserialize, Eq, Ord, PartialEq, PartialOrd, Serialize)]
+#[serde(from = "StoredEntry", into = "StoredEntry")]
 pub struct ReviewedUnchangedEntry {
-    #[serde(default, skip_serializing_if = "Option::is_none")]
     pub msgctxt: Option<String>,
     pub msgid: String,
 }
@@ -30,25 +31,40 @@ impl ReviewedUnchangedEntry {
     }
 }
 
-#[derive(Clone, Debug, Deserialize, Serialize)]
-struct ReviewedUnchangedFile {
-    version: u32,
-    #[serde(default)]
-    locales: BTreeMap<String, Vec<ReviewedUnchangedEntry>>,
+#[derive(Deserialize, Serialize)]
+#[serde(untagged)]
+enum StoredEntry {
+    Msgid(String),
+    WithContext(String, String),
 }
 
-impl Default for ReviewedUnchangedFile {
-    fn default() -> Self {
-        Self {
-            version: REVIEWED_UNCHANGED_VERSION,
-            locales: BTreeMap::new(),
+impl From<StoredEntry> for ReviewedUnchangedEntry {
+    fn from(stored: StoredEntry) -> Self {
+        match stored {
+            StoredEntry::Msgid(msgid) => Self {
+                msgctxt: None,
+                msgid,
+            },
+            StoredEntry::WithContext(msgctxt, msgid) => Self {
+                msgctxt: Some(msgctxt),
+                msgid,
+            },
+        }
+    }
+}
+
+impl From<ReviewedUnchangedEntry> for StoredEntry {
+    fn from(entry: ReviewedUnchangedEntry) -> Self {
+        match entry.msgctxt {
+            Some(msgctxt) => Self::WithContext(msgctxt, entry.msgid),
+            None => Self::Msgid(entry.msgid),
         }
     }
 }
 
 pub struct ReviewedUnchangedStore {
     path: PathBuf,
-    data: ReviewedUnchangedFile,
+    data: Locales,
     dirty_locales: BTreeSet<String>,
 }
 
@@ -70,14 +86,13 @@ impl ReviewedUnchangedStore {
     pub fn contains(&self, locale: &str, msgctxt: Option<&str>, msgid: &str) -> bool {
         let needle = ReviewedUnchangedEntry::new(msgctxt, msgid);
         self.data
-            .locales
             .get(locale)
             .is_some_and(|entries| entries.binary_search(&needle).is_ok())
     }
 
     pub fn mark(&mut self, locale: &str, msgctxt: Option<&str>, msgid: &str) {
         let entry = ReviewedUnchangedEntry::new(msgctxt, msgid);
-        let entries = self.data.locales.entry(locale.to_string()).or_default();
+        let entries = self.data.entry(locale.to_string()).or_default();
         match entries.binary_search(&entry) {
             Ok(_) => {}
             Err(index) => {
@@ -89,7 +104,7 @@ impl ReviewedUnchangedStore {
 
     pub fn unmark(&mut self, locale: &str, msgctxt: Option<&str>, msgid: &str) {
         let entry = ReviewedUnchangedEntry::new(msgctxt, msgid);
-        let Some(entries) = self.data.locales.get_mut(locale) else {
+        let Some(entries) = self.data.get_mut(locale) else {
             return;
         };
         let Ok(index) = entries.binary_search(&entry) else {
@@ -97,14 +112,13 @@ impl ReviewedUnchangedStore {
         };
         entries.remove(index);
         if entries.is_empty() {
-            self.data.locales.remove(locale);
+            self.data.remove(locale);
         }
         self.dirty_locales.insert(locale.to_string());
     }
 
     pub fn locales(&self) -> impl Iterator<Item = (&str, &[ReviewedUnchangedEntry])> {
         self.data
-            .locales
             .iter()
             .map(|(locale, entries)| (locale.as_str(), entries.as_slice()))
     }
@@ -114,7 +128,7 @@ impl ReviewedUnchangedStore {
         locale: &str,
         mut keep: impl FnMut(&ReviewedUnchangedEntry) -> bool,
     ) -> usize {
-        let Some(entries) = self.data.locales.get_mut(locale) else {
+        let Some(entries) = self.data.get_mut(locale) else {
             return 0;
         };
         let before = entries.len();
@@ -124,14 +138,14 @@ impl ReviewedUnchangedStore {
             return 0;
         }
         if entries.is_empty() {
-            self.data.locales.remove(locale);
+            self.data.remove(locale);
         }
         self.dirty_locales.insert(locale.to_string());
         removed
     }
 
     pub fn clear_locale(&mut self, locale: &str) {
-        if self.data.locales.remove(locale).is_some() {
+        if self.data.remove(locale).is_some() {
             self.dirty_locales.insert(locale.to_string());
         }
     }
@@ -147,14 +161,14 @@ impl ReviewedUnchangedStore {
         let _lock = SidecarLock::acquire(&self.path)?;
         let mut merged = read_store_file(&self.path)?;
         for locale in &self.dirty_locales {
-            match self.data.locales.get(locale) {
+            match self.data.get(locale) {
                 Some(entries) if !entries.is_empty() => {
                     let mut entries = entries.clone();
                     normalize_entries(&mut entries);
-                    merged.locales.insert(locale.clone(), entries);
+                    merged.insert(locale.clone(), entries);
                 }
                 _ => {
-                    merged.locales.remove(locale);
+                    merged.remove(locale);
                 }
             }
         }
@@ -165,29 +179,22 @@ impl ReviewedUnchangedStore {
     }
 }
 
-fn read_store_file(path: &Path) -> Result<ReviewedUnchangedFile> {
+fn read_store_file(path: &Path) -> Result<Locales> {
     let content = match fs::read_to_string(path) {
         Ok(content) => content,
         Err(error) if error.kind() == ErrorKind::NotFound => {
-            return Ok(ReviewedUnchangedFile::default());
+            return Ok(Locales::new());
         }
         Err(error) => {
             return Err(error).with_context(|| format!("failed to read {}", path.display()));
         }
     };
-    let mut data = serde_json::from_str::<ReviewedUnchangedFile>(&content)
+    let mut data = serde_json::from_str::<Locales>(&content)
         .with_context(|| format!("failed to parse {}", path.display()))?;
-    if data.version != REVIEWED_UNCHANGED_VERSION {
-        bail!(
-            "unsupported reviewed-unchanged sidecar version {} in {}",
-            data.version,
-            path.display()
-        );
-    }
-    for entries in data.locales.values_mut() {
+    for entries in data.values_mut() {
         normalize_entries(entries);
     }
-    data.locales.retain(|_, entries| !entries.is_empty());
+    data.retain(|_, entries| !entries.is_empty());
     Ok(data)
 }
 
@@ -196,10 +203,28 @@ fn normalize_entries(entries: &mut Vec<ReviewedUnchangedEntry>) {
     entries.dedup();
 }
 
-fn write_store_file(path: &Path, data: &ReviewedUnchangedFile) -> Result<()> {
-    let mut content = serde_json::to_string_pretty(data)
-        .with_context(|| format!("failed to serialize {}", path.display()))?;
-    content.push('\n');
+fn render_store(data: &Locales) -> Result<String> {
+    let mut locales = Vec::with_capacity(data.len());
+    for (locale, entries) in data {
+        let mut lines = Vec::with_capacity(entries.len());
+        for entry in entries {
+            lines.push(format!("\t\t{}", serde_json::to_string(entry)?));
+        }
+        locales.push(format!(
+            "\t{}: [\n{}\n\t]",
+            serde_json::to_string(locale)?,
+            lines.join(",\n")
+        ));
+    }
+    if locales.is_empty() {
+        return Ok("{}\n".to_string());
+    }
+    Ok(format!("{{\n{}\n}}\n", locales.join(",\n")))
+}
+
+fn write_store_file(path: &Path, data: &Locales) -> Result<()> {
+    let content =
+        render_store(data).with_context(|| format!("failed to serialize {}", path.display()))?;
     let timestamp = SystemTime::now()
         .duration_since(UNIX_EPOCH)
         .unwrap_or_default()

tools/i18n_auto/src/runner.rs

@@ -15,8 +15,8 @@ use crate::config::{
     DEFAULT_OPENROUTER_BASE_URL, DEFAULT_OPENROUTER_FALLBACK_MODELS,
     DEFAULT_OPENROUTER_HTTP_REFERER, DEFAULT_OPENROUTER_MODEL, DEFAULT_OPENROUTER_PROVIDER_SORT,
     DEFAULT_PROGRESS_INTERVAL_SECONDS, DEFAULT_REQUEST_TIMEOUT_SECONDS, DEFAULT_STRING_CONCURRENCY,
-    EnvOverlay, SOURCE_LOCALE, default_app_dir, env_value, is_auto_i18n_unchanged_comment,
-    locales_dir, positive_float_env, positive_int_env, trim_trailing_slash,
+    EnvOverlay, SOURCE_LOCALE, default_app_dir, env_value, locales_dir, positive_float_env,
+    positive_int_env, trim_trailing_slash,
 };
 use crate::json_catalog::{
     StaticJsonCatalogConfig, read_static_json_entries, rebuild_static_json_allow_replacing,
@@ -594,10 +594,6 @@ fn should_translate_entry_with_reviewed_unchanged(
     if args.refresh_source_equal
         && refreshes_source_equal_locale(locale, args)
         && entry.msgstr == entry.msgid
-        && !entry
-            .comments
-            .iter()
-            .any(|comment| is_auto_i18n_unchanged_comment(comment))
         && !reviewed_unchanged
     {
         return !should_keep_unchanged(&entry.msgid, locale);