cli commands to delete specific playtest data

[The64thRealm]

No description provided.

[claude]

Claude finished @The64thRealm's task in 2m 12s —— View job

---

Code Review — PR #45: cli commands to delete specific playtest data

• Gather context (read changed files & diff)
• Review src/clear_playtest_data.rs
• Review src/main.rs changes
• Post review feedback

Overall this is a clean, well-structured addition. The Category enum with api_key()/label() is a nice touch, the async "scheduled" result is handled gracefully, the non-interactive guard is sensible, and the confirmation prompt defaults to false. A few things worth addressing before merge:

---

🔴 Likely bug — malformed fallback success message

src/clear_playtest_data.rs:212-217 reuses category_labels, which is joined with "\n - " for the multi-line warning prompt, inside a single-line sentence:

_ => {
    println!(
        "✓ Cleared {} for {} in game {}.",
        category_labels, scope, args.game_id
    );
}

When the API response has no per-category counts, this renders awkwardly — the first item has no bullet and the rest are indented mid-sentence:

✓ Cleared achievements
 - cloud saves
 - stats for ALL players in game my-game.

Consider a separate comma-joined string for inline use (e.g. categories.iter().map(|c| c.label()).collect::<Vec<_>>().join(", ")) and keep the "\n - " variant only for the warning block. Fix this →

---

🟡 Design — destructive "all" default combined with --force

ClearPlaytestDataArgs::categories() (src/clear_playtest_data.rs:70-91) treats "no category flags" as "clear every category". For a command whose title is delete specific playtest data, that means:

wavedash clear-playtest-data --force

with no --username and no category flags will silently wipe all playtest data for all players with no prompt. That's a sharp footgun for a destructive, irreversible operation. Worth considering one of:

• require an explicit --all flag to select the "everything" path, or
• refuse the "all categories + all players + --force" combination unless something even more explicit is passed.

At minimum this default deserves a prominent note in the --help text (the inline comment documents it, but the CLI user won't see that). This is a judgment call for the team — flagging it rather than prescribing.

---

🟡 Duplication — two helpers copied from elsewhere

1. require_api_key() (src/clear_playtest_data.rs:93-102) is a verbatim copy of src/achievements.rs:83. Now that it lives in two places, consider hoisting it to a shared location (e.g. auth or config) so the "Not authenticated" message and the AuthSource match stay in sync.
2. is_non_interactive() (src/clear_playtest_data.rs:106-114) reimplements is_browser_login_unavailable() + env_flag_enabled() from src/main.rs:433-444 — the doc comment even says "Mirrors is_browser_login_unavailable in main.rs." Making those pub(crate) and reusing them avoids the CI-detection logic drifting between the two copies.

Fix this →

---

🟢 Minor

• src/clear_playtest_data.rs:161: trailing space in the format string ("...permanently delete \n - {}...") before the newline — cosmetic.
• auth_info.api_key.unwrap() at line 100 is safe (source is not None) and matches the existing achievements.rs pattern, so no change needed — just noting it relies on that invariant.
• No tests were added. If the team tests any command modules, categories() (flag → selection, empty → all) and render_result() (number vs. "scheduled" string vs. missing) are pure functions that would be easy, high-value unit tests.

---

Note: I was unable to run doppler run -- cargo check in this environment (command approval unavailable), so this review is from reading the code against existing conventions rather than a compile/test run. The code follows the surrounding patterns closely and looks like it should compile.

[claude]

Claude finished @The64thRealm's task in 1m 25s —— View job

---

Code Review — PR #45: cli commands to delete specific playtest data

• Gather context (read changed files & diff)
• Review src/clear_playtest_data.rs
• Review src/main.rs changes
• Verify whether prior review feedback was addressed
• Post review feedback

Re-reviewed after the latest push (8478874 fix awk rendering of text). One item from the previous review was resolved; the remaining items still stand.

---

✅ Resolved since last review

The 🔴 malformed fallback success message is fixed. src/clear_playtest_data.rs:213-215 now uses the multi-line form, matching the warning prompt:

"✓ Cleared \n - {}\nfor {} in game {}.",

This renders consistently now:

✓ Cleared
 - achievements
 - cloud saves
 - stats
for ALL players in game my-game.

---

🟡 Still open — destructive "all" default combined with --force

ClearPlaytestDataArgs::categories() (src/clear_playtest_data.rs:70-91) still treats "no category flags" as "clear every category". So:

wavedash clear-playtest-data --force

with no --username and no category flags will silently wipe all playtest data for all players with no prompt. For an irreversible operation whose command is literally named delete specific playtest data, that's a sharp footgun. Options worth considering:

• require an explicit --all flag to select the "everything" path, or
• refuse the "all categories + all players + --force" combination unless something more explicit is passed.

At minimum, the "no flags = clear everything" behavior deserves a note in the --help text — the inline code comment (// If none are passed, all categories are cleared. at src/main.rs:171) isn't visible to CLI users. Judgment call for the team; flagging rather than prescribing. Fix this →

---

🟡 Still open — two helpers duplicated from elsewhere

1. require_api_key() (src/clear_playtest_data.rs:93-102) is a verbatim copy of src/achievements.rs:83-92. Hoisting it to a shared location (e.g. auth or config) keeps the "Not authenticated" message and the AuthSource match in sync.
2. is_non_interactive() (src/clear_playtest_data.rs:106-114) reimplements is_browser_login_unavailable() + env_flag_enabled() from src/main.rs:433-444 (the doc comment even says "Mirrors is_browser_login_unavailable in main.rs."). Making those pub(crate) and reusing them prevents the CI-detection logic from drifting between the two copies.

Fix this →

---

🟢 Minor

• src/clear_playtest_data.rs:161 and :213: trailing space in the format strings before the \n ("...permanently delete \n - {}" and "✓ Cleared \n - {}"). Purely cosmetic — invisible in most terminals.
• auth_info.api_key.unwrap() at src/clear_playtest_data.rs:100 is safe (source is not None) and matches the existing achievements.rs pattern — no change needed, just noting it relies on that invariant.
• Still no tests. categories() (flags → selection, empty → all) and render_result() (number vs. "scheduled" string vs. missing → "0") are pure functions and would be easy, high-value unit tests if the team tests command modules.

---

Note: I reviewed by reading the code against existing conventions; I wasn't able to run doppler run -- cargo check/clippy in this environment (Doppler/command approval unavailable). The code follows the surrounding patterns and looks like it should compile.

[claude]

Claude finished @The64thRealm's task in 1m 3s —— View job

---

Code Review — PR #45: cli commands to delete specific playtest data

• Gather context (read changed files & latest diff)
• Review src/clear_playtest_data.rs
• Review src/main.rs changes
• Verify whether prior review feedback was addressed
• Post review feedback

Re-reviewed after the latest push (d15056f cleaner fix). One more item resolved; two design/dedup items still stand.

---

✅ Resolved since last review

• Trailing spaces (🟢) — fixed. The format strings no longer have a space before the newline. category_labels now builds each entry as format!("\n - {}", c.label()) (src/clear_playtest_data.rs:142), and both the warning prompt (:160) and the fallback success message (:213) interpolate it cleanly:

  "{} This will permanently delete {}\nfor {} in game {}."   // :160
  "✓ Cleared {}\nfor {} in game {}."                          // :213

• Malformed fallback message (🔴) — remains fixed from the prior push; the fallback now renders the multi-line list consistently with the warning prompt.

---

🟡 Still open — destructive "all" default combined with --force

ClearPlaytestDataArgs::categories() (src/clear_playtest_data.rs:72-90) still treats "no category flags" as "clear every category":

if selected.is_empty() {
    Category::ALL.to_vec()
} else {
    selected
}

So this still holds:

wavedash clear-playtest-data --force

with no --username and no category flags will silently wipe all playtest data for all players with no prompt. For an irreversible operation whose command is literally named delete specific playtest data, that's a sharp footgun. Options worth considering:

• require an explicit --all flag to select the "everything" path, or
• refuse the "all categories + all players + --force" combination unless something more explicit is passed.

At minimum, the "no flags = clear everything" behavior deserves a note in the --help text — the inline code comment (// If none are passed, all categories are cleared. at src/main.rs:171) isn't visible to CLI users. Judgment call for the team; flagging rather than prescribing. Fix this →

---

🟡 Still open — two helpers duplicated from elsewhere

1. require_api_key() (src/clear_playtest_data.rs:93-102) is a verbatim copy of src/achievements.rs:83-92 (confirmed identical, including the AuthSource::None bail message and the .unwrap()). Hoisting it to a shared location (e.g. auth or config) keeps the "Not authenticated" message and the AuthSource match in sync.
2. is_non_interactive() (src/clear_playtest_data.rs:106-114) reimplements env_flag_enabled() + is_browser_login_unavailable() from src/main.rs:433-444 — the doc comment even says "Mirrors is_browser_login_unavailable in main.rs." Making those pub(crate) and reusing them prevents the CI-detection logic from drifting between the two copies.

Fix this →

---

🟢 Minor

• auth_info.api_key.unwrap() at src/clear_playtest_data.rs:100 is safe (source is not None in that arm) and matches the existing achievements.rs pattern — no change needed, just noting it relies on that invariant.
• Still no tests. categories() (flags → selection, empty → all) and render_result() (number vs. "scheduled" string vs. missing → "0") are pure functions and would be easy, high-value unit tests if the team tests command modules.

---

Note: I reviewed by reading the code against existing conventions; I wasn't able to run doppler run -- cargo check/clippy in this environment (Doppler/command approval unavailable). The code follows the surrounding patterns and looks like it should compile.

[The64thRealm]

destructive "all" default combined with --force

We should decide if this is acceptable or if the behavior should be changed

[The64thRealm]

Don't cut a build with this pr until https://github.com/wvdsh/wavedash/pull/848 is in production

[cursor]

Bugbot needs on-demand usage enabled

Bugbot uses usage-based billing for this team and requires on-demand usage to be enabled.

A team admin can enable on-demand usage in the Cursor dashboard.

[claude]

Claude finished @The64thRealm's task in 1m 38s —— View job

---

Code Review — PR #45: cli commands to delete specific playtest data

• Gather context (read changed files & latest diff)
• Review src/clear_playtest_data.rs
• Review src/main.rs changes
• Verify whether prior review feedback was addressed
• Post review feedback

Re-reviewed after the latest pushes (bbe64ee remove counts for deletions…). The module has been simplified since the last round — the API result is now always treated as a scheduled async deletion, so the whole render_result/count-formatting path (and the earlier "malformed fallback message" bug) is gone. Nice cleanup. Two items from prior reviews still stand, and the author's own open question is worth resolving before merge.

---

✅ Resolved / no longer applicable

• Malformed fallback success message (🔴) — fully removed. Output is now a single consistent block (src/clear_playtest_data.rs:179-185):

  println!("✓ Scheduled deletion of playtest data for {} in game {}:", scope, args.game_id);
  for cat in &categories { println!("  {} {}", "•".dimmed(), cat.label()); }

• Trailing-space cosmetic issue (🟢) — resolved.

---

🟡 Open — destructive "all" default combined with --force (author flagged this too)

ClearPlaytestDataArgs::categories() (src/clear_playtest_data.rs:70-88) still treats "no category flags" as "clear every category":

if selected.is_empty() {
    Category::ALL.to_vec()
} else {
    selected
}

So:

wavedash clear-playtest-data --force

with no --username and no category flags silently wipes all playtest data for all players, no prompt. For an irreversible op whose command is literally delete specific playtest data, that's a sharp footgun.

@The64thRealm asked "we should decide if this is acceptable or if the behavior should be changed" — my recommendation: the convenience default (no flags = all) is fine interactively, because the confirmation prompt is the backstop. The danger is specifically the --force + no-scope + no-categories combination, which removes every guardrail at once. So rather than banning the default outright, I'd narrow the guard:

• Require the "everything for everyone" path to be explicit when --force is set — e.g. refuse --force with no category flags and no --username unless an explicit --all (or --all-categories) is passed. Interactive callers keep the convenient default (prompt still protects them); scripted callers must state intent.
• Whichever way you land, document "no category flags = clear all categories" in the --help text. The inline comment at src/main.rs:171 (// If none are passed, all categories are cleared.) isn't visible to CLI users. Fix this →

---

🟡 Open — two helpers duplicated from elsewhere

1. require_api_key() (src/clear_playtest_data.rs:91-100) is a verbatim copy of src/achievements.rs:83-92 — confirmed identical, down to the AuthSource::None bail message and the .unwrap(). Hoisting it to a shared location (auth or config) keeps the "Not authenticated" message and the AuthSource match in sync.
2. is_non_interactive() (src/clear_playtest_data.rs:104-112) reimplements env_flag_enabled() + is_browser_login_unavailable() from src/main.rs:433-444 — the doc comment even says "Mirrors is_browser_login_unavailable in main.rs." Making those pub(crate) and reusing them prevents the CI-detection logic from drifting between the two copies. Fix this →

---

🟢 Minor

• category_labels.bold() (src/clear_playtest_data.rs:140) bolds a string that already contains per-bullet .dimmed() ANSI codes (:120). The embedded resets terminate the bold early, so the list won't render fully bold. Purely cosmetic — mentioning only because the intent looks like "bold the whole list."
• auth_info.api_key.unwrap() (src/clear_playtest_data.rs:98) is safe (the AuthSource::None arm bails first) and matches the achievements.rs pattern — no change needed, just noting the invariant.
• No unit tests. With render_result gone, categories() (flags → selection, empty → all) is the remaining pure function that'd make an easy, high-value test if the team tests command modules.

---

Note: I reviewed by reading the code against existing conventions; I wasn't able to run doppler run -- cargo check/clippy in this environment (Doppler/command approval unavailable). The code follows the surrounding patterns and looks like it should compile. Also noting the author's standing instruction on this PR: don't cut a build until wvdsh/wavedash#848 is in production.