fix(preset): stop non-ASCII preset names from colliding on disk - #275
Merged
Conversation
`sanitize_filename` mapped every byte outside `[A-Za-z0-9-_]` to `_`, so every ZH_CN preset name collapsed onto the same file: a Chinese-locale user effectively had one preset slot and each save destroyed the previous one. Percent-encode the disallowed bytes instead. The encoding is injective (`%` is itself escaped), stable, reversible, and the identity on names the old scheme already left alone. Existing installs keep working because a preset is identified by the `name` field inside its JSON, never by its filename: `Manager` now records the path each preset was loaded from and reuses it when saving or deleting, so files written under the old scheme are updated in place rather than orphaned. Only presets that do not exist yet get a freshly encoded name. Pathologically long names are truncated on an escape boundary and suffixed with an FNV-1a digest so they stay under the filesystem's per-component limit without colliding. Also wire up the overwrite-confirmation dialog, which was fully built but had no call sites — saving over an existing preset now asks first. `PresetMessage::SaveConfirmed` keeps confirming from re-triggering the prompt; `Update` stays exempt, since overwriting the selected preset is the point. Tests: sanitize_filename injectivity (ZH_CN, punctuation-only diffs, percent, length bounds) and identity on already-safe names; a save -> load round trip and legacy-filename save/delete through a temp dir; and a serde guard pinning the on-disk key of all 12 `StageConfig` variants, which previously had round-trip coverage for exactly one.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes preset filename collisions for non-ASCII (notably ZH_CN) preset names by switching from a lossy sanitization scheme to an injective percent-encoding scheme, while preserving backward compatibility for existing on-disk preset files by tracking the original load path per preset name. It also wires up the previously-unused overwrite confirmation dialog for “Save As” operations and adds regression tests (including a serde “wire name” guard for all StageConfig variants).
Changes:
- Replace lossy filename sanitization with percent-encoding + bounded-length stems (with stable FNV-1a suffix on truncation) and reuse legacy filenames via
Managerpath tracking. - Wire the overwrite-confirmation UI by splitting
PresetMessage::SavevsSaveConfirmedand plumbing it through the standalone app. - Add tests for filename injectivity/back-compat round trips and pin
StageConfig’s externally-tagged JSON keys for all registered stages.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| rustortion-ui/src/messages/preset.rs | Adds SaveConfirmed message to separate confirmed overwrites from normal saves. |
| rustortion-ui/src/handlers/preset.rs | Prompts on overwrite for Save, and handles SaveConfirmed to proceed without re-prompting. |
| rustortion-ui/src/components/preset_bar.rs | Uses an explicit overwrite target and emits SaveConfirmed on confirmation. |
| rustortion-standalone/src/gui/app.rs | Treats SaveConfirmed the same as other preset select/save events for persistence flow. |
| rustortion-core/src/preset/stage_config.rs | Adds tests that pin stable serde wire keys for all StageConfig variants. |
| rustortion-core/src/preset/manager.rs | Implements percent-encoded filename stems, stable hashing for truncation, and tracks original preset file paths for back-compat saves/deletes. |
| CLAUDE.md | Updates project notes to reflect the new preset filename scheme and remaining REV-7 concerns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+149
to
+157
| fn path_for(&self, preset_name: &str) -> PathBuf { | ||
| self.preset_paths.get(preset_name).map_or_else( | ||
| || { | ||
| self.presets_dir | ||
| .join(format!("{}.json", sanitize_filename(preset_name))) | ||
| }, | ||
| Clone::clone, | ||
| ) | ||
| } |
`path_for` fell back to the encoded name whenever a preset was not in
`preset_paths`, and `save_preset` wrote there without checking whether the
file already belonged to something else. The overwrite dialog did not
help: it gates on `preset_exists`, which compares names, not paths.
That is reachable with the presets we ship. The old lossy scheme emitted
stems drawn from exactly `[A-Za-z0-9-_]`, which is precisely the set the
new encoding maps to identity, so every shipped file whose internal
`name` differs from its stem is armed by the literal stem as a preset
name. On a fresh install: Save As -> `High_Gain` -> Save -> no prompt ->
`presets/High_Gain.json` (name `High Gain`) is gone. Same for `Clean_Eb`,
`Clean_Overdrive`, `Djent_Eb` and `Funky_Bass`. Pre-existing on main, but
it is the defect REV-7 exists to remove, so close it here.
`path_for` now refuses any candidate that exists on disk or that another
loaded preset claims, stepping aside to the digest-suffixed stem the
truncation path already uses. Disambiguation is silent: filenames carry
no user-visible identity — the `name` field inside the JSON does — so
`High_Gain` landing on `High_Gain-<digest>.json` while `High Gain` keeps
its file is both the least surprising outcome and strictly
data-preserving. Existing installs are untouched, since a preset that is
already on disk still short-circuits to its recorded path. The name-level
overwrite prompt is a different question and stays as it is.
`delete_preset` had the mirror of the same bug: `delete_preset("My_Preset")`
against a store holding only `My Preset` computed `My_Preset.json`, found
it, deleted it and returned `Ok`. It now only removes a file it can
positively attribute to the named preset, and reports "Preset file not
found" otherwise.
Two standalone-shell fixes around the newly live prompt:
- Cancelling it no longer persists the new name. `Save` used to be
unconditionally treated as a save, so settings.json was written with
the pending name before the user answered; clicking No left the next
launch opening a preset that was never saved. Persist only when the
handler's own selection actually moved.
- `any_dialog_visible` now covers the prompt, so hotkeys are blocked
while it is up. A preset-switch hotkey pressed between Save and Yes
swapped the whole chain out, and the confirmation then wrote the newly
loaded chain over the confirmed name.
`sanitize_filename` no longer claims unconditional injectivity: `-` and
`0-9a-f` are unreserved, so a truncated digest-suffixed stem is itself a
valid identity encoding, and `"x".repeat(500)` collides with a preset
named literally `xxx…x-b6822c71b0501b75`. The occupancy check, not the
encoding, is what keeps two presets off one file. CLAUDE.md also had the
preset directory wrong: it is `preset_dir`, defaulting to `./presets`,
and the shipped `presets/` is that live writable directory — which is
what made this reachable at all.
Tests: the save and delete holes above; a long name and its digest-stem
namesake surviving as separate files; `sanitize_filename("")` vs `"%"`,
which had a justifying comment and no coverage; and `stable_hash` pinned
to literals, so the cross-toolchain stability that motivated hand-rolling
FNV-1a is actually guarded rather than only checked within one process.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the REV-7 half of the v0.3.0 gate, with TEST-7.
The bug
sanitize_filenamemapped every byte outside[A-Za-z0-9-_]to_, with no collision check, and bothsave_presetanddelete_presetwent through it. Every ZH_CN preset name therefore collapsed onto the same file: a Chinese-locale user effectively had one preset slot, and each save destroyed the previous one. ZH_CN is a shipped locale.The display name was never affected — presets are identified by the
namefield inside the JSON — so this looked like presets silently vanishing rather than anything visibly wrong.Separately, the overwrite-confirmation dialog was fully built in
preset_bar.rsbutshow_overwrite_confirmation()had zero call sites, so saves always overwrote silently.The fix
Percent-encode the disallowed bytes instead. The encoding is injective (
%is itself escaped as%25), stable, and the identity on names the old scheme already left alone.Backward compatibility. Percent-encoding alone would orphan existing files: a legacy
My_Preset.json(name"My Preset") would get saved to a newMy%20Preset.json, leaving the old file behind — the preset would then appear twice and delete would miss. SoManagernow records the path each preset was loaded from and reuses it on save/delete. Files written by older versions are updated in place; only presets that don't exist yet get a freshly encoded name.Pathologically long names are truncated on an escape-unit boundary (never mid-
%XX) and suffixed with an FNV-1a digest — hand-rolled becauseDefaultHasher's algorithm is explicitly unstable across Rust releases and these values land in filenames.The overwrite dialog is now wired up.
PresetMessage::SaveConfirmedis kept separate fromSaveso confirming can't re-trigger the prompt.Updatestays exempt — overwriting the selected preset is its purpose. No new UI strings, so no i18n work:overwrite_preset/yes/noalready exist in EN and ZH_CN.Known residual
Names differing only in ASCII case still share a file on case-insensitive filesystems. Encoding case would have changed the filename of every existing preset — a far worse trade. Preset writing only happens in the standalone, whose release targets are both case-sensitive Linux.
Out of scope
Deliberately deferred to v0.4.0, per the gate scoping: the non-atomic
fs::write(crash mid-write leaves truncated JSON) and the missing UI error surface for save/delete failures. Both are narrower windows than the collision.Tests
+12.
sanitize_filenameinjectivity across ZH_CN names, punctuation-only differences,%, and length bounds, plus identity on already-safe names. A save -> load round trip and legacy-filename save/delete through a temp dir. And a serde guard pinning the on-disk key of all 12StageConfigvariants — previously exactly one had round-trip coverage, so a serde rename would have silently broken every user's presets.Verified on the integration branch with all four gate items: 302 passed, 0 failed, lint clean.