Fix three cases where the converter silently drops entries - #60
Conversation
Each of these makes json-to-xml-all lose data without failing, so the loss only shows up as missing strings in-game. 1. RemoveAbsorbedSourceFiles deleted the individual questdescription_* and scriptquest_* files before checking whether the combined output that replaces them would actually be written for that locale. A partially translated locale has no questdescription_final.json or scriptquest.json, so the split files were deleted and nothing was put back. On pt this removed 29 files holding roughly 20.9k translated attributes. The removal is now gated on the corresponding combined JSON being present. 2. "questdescription_famefield .xml" ships with a trailing space in its filename. That misses the key-attribute lookup, so it falls back to the default "id", finds no such attribute (the elements use questID), and every entry in the file is dropped. Fixed by trimming the lookup key. 3. questdescription_daily and questdescription_eventjp were missing from both lookup tables, which is the same failure mode as 2. Checked against Xml/string/de, the one locale that still has the famefield file: xml-to-json de reports 108 parse warnings for it before this change and 2 after, and all 106 of its questIDs now reach questdescription_final.json. Note that the data these bugs dropped is already gone from the tree as of #55 (pt has no quest/script files left, questdescription_daily is gone, and only the de copy of famefield survives). This change stops further loss but does not recover that content, which is still available in the history before #55. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesConverter updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@WeblateConverter/Converter/Converter.cs`:
- Around line 588-599: Update the cleanup flow in ProcessJsonToXml to track
whether questdescription_final.json or scriptquest.json was successfully written
with valid combined output, rather than only checking filename presence. Defer
deletion of source fragments until after the corresponding conversion/write
succeeds, and preserve the existing behavior of skipping empty or malformed JSON
without deleting its source data.
- Around line 621-624: Update IsAbsorbedByCombinedOutput so
questdescription_final.xml is explicitly excluded from the questdescription_
prefix match, while preserving absorption behavior for other source fragments
and the existing hasQuestOutput result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a2fd25f2-978f-4927-9edf-6d7af72a8a6b
📒 Files selected for processing (1)
WeblateConverter/Converter/Converter.cs
…output is written Addresses CodeRabbit's two findings on the absorbed-file cleanup: 1. RemoveAbsorbedSourceFiles ran BEFORE generation and gated on the combined JSON merely existing, so an empty questdescription_final.json/scriptquest.json (skipped) or a failed conversion still deleted the fragments with nothing written back. Cleanup now runs AFTER the loop and deletes a family's fragments only if its combined output was actually written this run. 2. questdescription_final.xml matches the questdescription_ prefix, so the cleanup would delete the combined output itself. IsAbsorbedByCombinedOutput now excludes both combined outputs explicitly. Verified end-to-end: empty combined -> fragments preserved; non-empty combined -> fragments removed and the combined output survives; json-to-xml-all + verify-all clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@WeblateConverter/Converter/Converter.cs`:
- Around line 671-677: Update ConvertQuestDescriptionFinalWithSorting to return
false when questEntries.Count is zero before saving, and return true only after
successfully saving a non-empty output. In the jsonFile handling block, assign
questFinalWritten from that helper result instead of marking it unconditionally;
keep scriptFinalWritten behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bdc741c5-2cd2-44b7-aada-f6812be21471
📒 Files selected for processing (1)
WeblateConverter/Converter/Converter.cs
Follow-up to CodeRabbit: a non-empty questdescription_final.json whose quest IDs are all unparseable produced an empty questEntries list, which (a) overwrote the existing questdescription_final.xml with an empty file and (b) still marked the output written, so the absorbed source fragments were deleted. ConvertQuestDescriptionFinalWithSorting now returns bool: on an empty result it skips the save (leaving the existing combined intact) and returns false; the caller assigns questFinalWritten from that, so cleanup runs only after a real, non-empty write. Verified: all-invalid-id JSON leaves both the combined output and its fragments untouched; json-to-xml-all + verify-all remain clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The in-place flip's RemoveAbsorbedSourceFiles deleted pt's questdescription_*/ scriptquest_* fragments even though pt has no combined questdescription_final.json/ scriptquest.json to replace them (the bug fixed in #60), so the data was lost. Restores the 29 files from 3b7ec8a (last commit before #55): ~5,864 quest entries, of which 4,755 are real Portuguese translations. Verified: they parse as valid XML, and with the #60 fix in place a full json-to-xml-all no longer deletes them (pt has no combined output). verify-all passes. Other locales (en/cn/jp/kr) are not affected — their fragments were correctly absorbed into the committed combined outputs, so their data is preserved. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The original XML->JSON dropped questdescription_famefield's 106 entries because of the trailing-space filename bug (fixed in #60), so they never made it into the combined questdescription_final.json. - en/cn/jp: add the 106 missing famefield entries (real translations, e.g. 'Cry of the Wildflower', 'The Case of the Missing Plungers'). - ko: the base-language template held Chinese for all 106 famefield keys (the missing-keys merge backfilled ko from cn when ko's own parse failed). Correct them to the real Korean from the KMS2 source (e.g. 野花的悲鸣 -> 야생화의 비명). No original key dropped, no non-famefield key changed. Verified end-to-end: json-to-xml regenerates valid XML for the recovered entries and 'verify' passes. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Three separate bugs in
WeblateConvertercausejson-to-xml-allto lose data without failing. Because nothing errors, the loss only surfaces later as missing strings in-game.Found while adopting the #55 JSON-source model on a fork, where the regenerated output was diffed against the pre-#55 tree.
1. Split quest files deleted with no replacement
RemoveAbsorbedSourceFilesdeleted everyquestdescription_*/scriptquest_*file for a locale before checking whether the combined output that absorbs them would actually be written. A partially translated locale has noquestdescription_final.jsonorscriptquest.json, so the files were deleted and nothing was put back.ptis the clear case: it only hasitemname.jsonandkorflash.json, and lost 29 files holding roughly 20.9k translated attributes. Removal is now gated on the corresponding combined JSON being present.2. Trailing space in a filename misses every lookup
questdescription_famefield .xmlships with a trailing space in its name. That misses the key-attribute lookup, falls back to the defaultid, finds no such attribute (the elements usequestID), and drops all 106 entries. Fixed by trimming the lookup key, which also guards against any other stray whitespace.3. Two files absent from the lookup tables
questdescription_dailyandquestdescription_eventjpwere in neither the element-name nor the key-attribute table, which is the same failure mode as 2. Added to both, matching the other 19questdescription_*entries.Evidence
Xml/string/deis the only locale that still has the famefield file, anddehas no JSON so it was never regenerated. Runningxml-to-json de:questdescription_final.jsonScope
This stops further loss but does not recover what is already gone. As of #55 the tree has no
ptquest/script files, noquestdescription_daily, and only thedecopy of famefield. That content is still in the history before #55 if you want it restored, which I am happy to do as a follow-up.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
questIDkey attribute.