fix(license): apply the review follow-ups on the license stack head
All checks were successful
PR Check — Frontend / frontend (pull_request) Successful in 1m47s
PR Check — Rust / rust (pull_request) Successful in 3m40s

Follow-ups of the three /pr-review passes on #386, #387 and #388, applied
on the stack head before the fast-forward merge:

- #386: pin the relay of the revoked state by revocationCheckLauncher
  (a launcher passing false instead of wasRevoked passed the whole suite),
  and say in the rate budget comment that two checks in one minute bring
  the machine-limit path to 6 requests.
- #387 S1: the fake `t` of the removal tests and of the License card tests
  marks what it translates, so a string that skipped `t` fails its test.
- #387 S2: pin that storing another key stops on Storage while the
  previous token cannot be dropped, the previous key staying.
- #387 S3: reload the license context after a failed removal too. Rust
  deletes the key first, so a token that resisted left the key gone while
  the card kept showing it under "the key was not removed".
- #388: pin that an unreadable token stays (the directory case could not
  tell, its deletion fails anyway).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
le king fu 2026-10-05 19:13:36 -04:00
parent fd90de5424
commit 2cbc886653
5 changed files with 114 additions and 35 deletions

View file

@ -1979,6 +1979,22 @@ mod tests {
assert!(dir.0.join(ACTIVATION_FILE).exists(), "the same key keeps its activation"); assert!(dir.0.join(ACTIVATION_FILE).exists(), "the same key keeps its activation");
} }
#[test]
fn store_refuses_another_key_while_the_previous_token_cannot_be_dropped() {
// Kept next to the new key, that token would name the previous license to the price
// server (#347 review, the folder of #367): the key change stops on `Storage` instead,
// and the previous key stays (#387 review).
let (enc, dec) = default_keys();
let dir = TestDir::new("store_token_not_deletable");
let old_key = base_license_with_features(&enc, vec!["balance".to_string()]);
dir.write(LICENSE_FILE, &old_key);
// A directory in place of the token: removing it as a file fails.
fs::create_dir(dir.0.join(ACTIVATION_FILE)).unwrap();
let new_key = base_license_with_features(&enc, vec![]);
assert_eq!(store_license_in(&dir.0, &new_key, &dec).unwrap_err(), LicenseError::Storage);
assert_eq!(fs::read_to_string(dir.0.join(LICENSE_FILE)).unwrap(), old_key);
}
#[test] #[test]
fn store_drops_a_token_left_without_a_key() { fn store_drops_a_token_left_without_a_key() {
let (enc, dec) = default_keys(); let (enc, dec) = default_keys();
@ -3575,6 +3591,16 @@ mod tests {
assert_eq!(remove_activation_token_if_unchanged_in(&dir.0, " token-a"), Ok(true)); assert_eq!(remove_activation_token_if_unchanged_in(&dir.0, " token-a"), Ok(true));
assert!(!dir.0.join(ACTIVATION_FILE).exists()); assert!(!dir.0.join(ACTIVATION_FILE).exists());
// A token that does not read as text: nothing says it is the one the answer is about,
// so it stays (#388 review; the directory below cannot tell, its deletion fails anyway).
dir.write(ACTIVATION_FILE, NOT_UTF8);
assert_eq!(
remove_activation_token_if_unchanged_in(&dir.0, "token-a"),
Err(LicenseError::Storage)
);
assert!(dir.0.join(ACTIVATION_FILE).exists(), "an unreadable token stays");
fs::remove_file(dir.0.join(ACTIVATION_FILE)).unwrap();
// A directory in place of the token: it cannot be read as a file. // A directory in place of the token: it cannot be read as a file.
fs::create_dir(dir.0.join(ACTIVATION_FILE)).unwrap(); fs::create_dir(dir.0.join(ACTIVATION_FILE)).unwrap();
assert_eq!( assert_eq!(

View file

@ -10,10 +10,13 @@ vi.mock("@tauri-apps/api/core", () => ({ invoke: vi.fn() }));
vi.mock("@tauri-apps/plugin-opener", () => ({ openUrl: vi.fn(() => Promise.resolve()) })); vi.mock("@tauri-apps/plugin-opener", () => ({ openUrl: vi.fn(() => Promise.resolve()) }));
let language = "fr"; let language = "fr";
// The fake `t` marks what it translates, `t(key[options])`: the assertions written in that form
// fail on a string that skipped `t` (#387 review). Substring assertions on a bare key still read
// the key inside the mark.
vi.mock("react-i18next", () => ({ vi.mock("react-i18next", () => ({
useTranslation: () => ({ useTranslation: () => ({
t: (k: string, o?: Record<string, unknown>) => t: (k: string, o?: Record<string, unknown>) =>
o ? `${k}[${Object.entries(o).map(([a, b]) => `${a}=${String(b)}`).join(",")}]` : k, o ? `t(${k}[${Object.entries(o).map(([a, b]) => `${a}=${String(b)}`).join(",")}])` : `t(${k})`,
i18n: { language }, i18n: { language },
}), }),
})); }));
@ -300,8 +303,8 @@ describe("LicenseCard machines", () => {
it("locks key entry while an activation runs, and Activate while a key is stored", () => { it("locks key entry while an activation runs, and Activate while a key is stored", () => {
// Either order would leave the previous key's token next to the new key (#364 review). // Either order would leave the previous key's token next to the new key (#364 review).
const enterKeyDisabled = /<button[^>]*disabled=""[^>]*>license\.enterKey<\/button>/; const enterKeyDisabled = /<button[^>]*disabled=""[^>]*>t\(license\.enterKey\)<\/button>/;
const activateDisabled = /<button[^>]*disabled=""[^>]*>license\.activate<\/button>/; const activateDisabled = /<button[^>]*disabled=""[^>]*>t\(license\.activate\)<\/button>/;
machines = hookView({ activation: notActivated, activating: true }); machines = hookView({ activation: notActivated, activating: true });
expect(render()).toMatch(enterKeyDisabled); expect(render()).toMatch(enterKeyDisabled);
@ -328,11 +331,11 @@ describe("LicenseCard machines", () => {
// "Remove the key" shows for any stored key, valid or revoked (#370): `hasLicense` (a paid // "Remove the key" shows for any stored key, valid or revoked (#370): `hasLicense` (a paid
// edition) would hide it for a revoked key, whose daily check is what the removal stops. // edition) would hide it for a revoked key, whose daily check is what the removal stops.
describe("LicenseCard key removal (#370)", () => { describe("LicenseCard key removal (#370)", () => {
const REMOVE = "license.removeKey.button"; const REMOVE = "t(license.removeKey.button)";
const removeDisabled = /<button[^>]*disabled=""[^>]*>license\.removeKey\.button<\/button>/; const removeDisabled = /<button[^>]*disabled=""[^>]*>t\(license\.removeKey\.button\)<\/button>/;
const enterKeyDisabled = /<button[^>]*disabled=""[^>]*>license\.enterKey<\/button>/; const enterKeyDisabled = /<button[^>]*disabled=""[^>]*>t\(license\.enterKey\)<\/button>/;
const activateDisabled = /<button[^>]*disabled=""[^>]*>license\.activate<\/button>/; const activateDisabled = /<button[^>]*disabled=""[^>]*>t\(license\.activate\)<\/button>/;
const deactivateDisabled = /<button[^>]*disabled=""[^>]*>license\.machines\.deactivate<\/button>/; const deactivateDisabled = /<button[^>]*disabled=""[^>]*>t\(license\.machines\.deactivate\)<\/button>/;
it("is offered for a valid key", () => { it("is offered for a valid key", () => {
const html = render(); const html = render();
@ -376,7 +379,7 @@ describe("LicenseCard key removal (#370)", () => {
it("while it runs, says so, and key entry, Activate and Deactivate wait", () => { it("while it runs, says so, and key entry, Activate and Deactivate wait", () => {
machines = hookView({ activation: notActivated, removing: true }); machines = hookView({ activation: notActivated, removing: true });
let html = render(); let html = render();
expect(html).toContain("license.removeKey.removing"); expect(html).toContain("t(license.removeKey.removing)");
expect(html).not.toContain(REMOVE); expect(html).not.toContain(REMOVE);
expect(html).toMatch(enterKeyDisabled); expect(html).toMatch(enterKeyDisabled);
expect(html).toMatch(activateDisabled); expect(html).toMatch(activateDisabled);
@ -396,7 +399,7 @@ describe("LicenseCard key removal (#370)", () => {
it("names a failed removal with its reason", () => { it("names a failed removal with its reason", () => {
machines = hookView({ removeError: parseLicenseError('{"code":"storage"}') }); machines = hookView({ removeError: parseLicenseError('{"code":"storage"}') });
const html = render(); const html = render();
expect(html).toContain("license.removeKey.failed[reason=license.errors.storage]"); expect(html).toContain("t(license.removeKey.failed[reason=t(license.errors.storage)])");
expect(html.split("license.removeKey.failed")).toHaveLength(2); expect(html.split("license.removeKey.failed")).toHaveLength(2);
}); });
}); });

View file

@ -601,3 +601,33 @@ describe("performRevocationCheck", () => {
expect(deps.refresh).not.toHaveBeenCalled(); expect(deps.refresh).not.toHaveBeenCalled();
}); });
}); });
// The launcher relays the revoked state the provider read: a reload follows a change only.
// Every other test of the launcher starts from a key that is not revoked (#386 review).
describe("revocationCheckLauncher", () => {
const settle = () => new Promise<void>((resolve) => setTimeout(resolve, 0));
function launcherHarness(result: RevocationCheck) {
const deps: RevocationCheckDeps = {
checkLicenseRevocation: vi.fn(async (): Promise<RevocationCheck> => result),
refresh: vi.fn(async () => {}),
logInfo: vi.fn(),
logWarn: vi.fn(),
};
return deps;
}
it("reloads when a key known as revoked is confirmed valid again", async () => {
const deps = launcherHarness({ outcome: "valid", revoked: false });
revocationCheckLauncher(deps)(true);
await settle();
expect(deps.refresh).toHaveBeenCalledTimes(1);
});
it("does not reload a key known as revoked that is still revoked", async () => {
const deps = launcherHarness({ outcome: "revoked", revoked: true });
revocationCheckLauncher(deps)(true);
await settle();
expect(deps.checkLicenseRevocation).toHaveBeenCalledTimes(1);
expect(deps.refresh).not.toHaveBeenCalled();
});
});

View file

@ -642,7 +642,7 @@ describe("useLicenseMachines wiring to the LicenseContext", () => {
await view.removeKey({ confirm, t: echo }); await view.removeKey({ confirm, t: echo });
expect(confirm).toHaveBeenCalledWith("license.removeKey.confirmRevoked"); expect(confirm).toHaveBeenCalledWith("t(license.removeKey.confirmRevoked)");
expect(invoke).toHaveBeenCalledWith("remove_license", { releaseSlot: false }); expect(invoke).toHaveBeenCalledWith("remove_license", { releaseSlot: false });
expect(context.refresh).toHaveBeenCalledTimes(1); expect(context.refresh).toHaveBeenCalledTimes(1);
expect(log.logInfo).toHaveBeenCalledWith("License: the stored key was removed"); expect(log.logInfo).toHaveBeenCalledWith("License: the stored key was removed");
@ -653,17 +653,19 @@ describe("useLicenseMachines wiring to the LicenseContext", () => {
const confirm = vi.fn(() => false); const confirm = vi.fn(() => false);
const view = renderHook(); const view = renderHook();
await view.removeKey({ confirm, t: echo }); await view.removeKey({ confirm, t: echo });
expect(confirm).toHaveBeenCalledWith("license.removeKey.confirmValid"); expect(confirm).toHaveBeenCalledWith("t(license.removeKey.confirmValid)");
expect(invoke).not.toHaveBeenCalled(); expect(invoke).not.toHaveBeenCalled();
}); });
}); });
// The removal of the stored key (#370). The fake `t` echoes the key and its options, so each // The removal of the stored key (#370). The fake `t` marks what it translates, `t(key[options])`,
// assertion names the message the user would read. // so each assertion names the message the user would read, and a string that skipped `t` (a raw
// key) fails it (#387 review).
function echo(key: string, options?: Record<string, unknown>): string { function echo(key: string, options?: Record<string, unknown>): string {
return options const params = options
? `${key}[${Object.entries(options).map(([name, value]) => `${name}=${String(value)}`).join(",")}]` ? `[${Object.entries(options).map(([name, value]) => `${name}=${String(value)}`).join(",")}]`
: key; : "";
return `t(${key}${params})`;
} }
describe("licenseMachinesReducer — removal of the key", () => { describe("licenseMachinesReducer — removal of the key", () => {
@ -776,7 +778,7 @@ describe("performRemoveLicense", () => {
it("calls nothing when the confirmation is refused", async () => { it("calls nothing when the confirmation is refused", async () => {
const { deps, events, getState } = removalHarness([false], []); const { deps, events, getState } = removalHarness([false], []);
await performRemoveLicense(deps, activated); await performRemoveLicense(deps, activated);
expect(events).toEqual(["confirm license.removeKey.confirmActivated"]); expect(events).toEqual(["confirm t(license.removeKey.confirmActivated)"]);
expect(deps.removeLicense).not.toHaveBeenCalled(); expect(deps.removeLicense).not.toHaveBeenCalled();
expect(deps.refresh).not.toHaveBeenCalled(); expect(deps.refresh).not.toHaveBeenCalled();
expect(deps.logInfo).not.toHaveBeenCalled(); expect(deps.logInfo).not.toHaveBeenCalled();
@ -788,7 +790,7 @@ describe("performRemoveLicense", () => {
const { deps, events, getState } = removalHarness([true], [removed]); const { deps, events, getState } = removalHarness([true], [removed]);
await performRemoveLicense(deps, valid); await performRemoveLicense(deps, valid);
expect(events).toEqual([ expect(events).toEqual([
"confirm license.removeKey.confirmValid", "confirm t(license.removeKey.confirmValid)",
"REMOVE_START", "REMOVE_START",
"remove(false)", "remove(false)",
"refresh", "refresh",
@ -805,7 +807,7 @@ describe("performRemoveLicense", () => {
const { deps, events } = removalHarness([true], [removed]); const { deps, events } = removalHarness([true], [removed]);
await performRemoveLicense(deps, activated); await performRemoveLicense(deps, activated);
expect(events).toEqual([ expect(events).toEqual([
"confirm license.removeKey.confirmActivated", "confirm t(license.removeKey.confirmActivated)",
"REMOVE_START", "REMOVE_START",
"remove(true)", "remove(true)",
"refresh", "refresh",
@ -820,7 +822,7 @@ describe("performRemoveLicense", () => {
const { deps, events } = removalHarness([true], [removed]); const { deps, events } = removalHarness([true], [removed]);
await performRemoveLicense(deps, revoked); await performRemoveLicense(deps, revoked);
expect(events.slice(0, 3)).toEqual([ expect(events.slice(0, 3)).toEqual([
"confirm license.removeKey.confirmRevoked", "confirm t(license.removeKey.confirmRevoked)",
"REMOVE_START", "REMOVE_START",
"remove(false)", "remove(false)",
]); ]);
@ -829,10 +831,10 @@ describe("performRemoveLicense", () => {
// Error table, second row: the server could not be reached or answered badly. Rust removed // Error table, second row: the server could not be reached or answered badly. Rust removed
// nothing; the second confirmation names the reason and warns that the slot stays occupied. // nothing; the second confirmation names the reason and warns that the slot stays occupied.
const retryable: [LicenseErrorCode, Record<string, unknown>, string][] = [ const retryable: [LicenseErrorCode, Record<string, unknown>, string][] = [
["network", {}, "license.errors.network"], ["network", {}, "t(license.errors.network)"],
["server", { status: 502 }, "license.errors.server"], ["server", { status: 502 }, "t(license.errors.server)"],
["rate_limit", { retry_after_s: 42 }, "license.errors.rateLimit[count=42]"], ["rate_limit", { retry_after_s: 42 }, "t(license.errors.rateLimit[count=42])"],
["invalid_response", {}, "license.errors.invalidResponse"], ["invalid_response", {}, "t(license.errors.invalidResponse)"],
]; ];
it.each(retryable)( it.each(retryable)(
@ -841,10 +843,10 @@ describe("performRemoveLicense", () => {
const { deps, events, getState } = removalHarness([true, true], [failure(code, extra), removed]); const { deps, events, getState } = removalHarness([true, true], [failure(code, extra), removed]);
await performRemoveLicense(deps, activated); await performRemoveLicense(deps, activated);
expect(events).toEqual([ expect(events).toEqual([
"confirm license.removeKey.confirmActivated", "confirm t(license.removeKey.confirmActivated)",
"REMOVE_START", "REMOVE_START",
"remove(true)", "remove(true)",
`confirm license.removeKey.confirmWithoutRelease[reason=${reason}]`, `confirm t(license.removeKey.confirmWithoutRelease[reason=${reason}])`,
"remove(false)", "remove(false)",
"refresh", "refresh",
"REMOVE_END", "REMOVE_END",
@ -888,14 +890,15 @@ describe("performRemoveLicense", () => {
const { deps, events, getState } = removalHarness([true], [failure(code)]); const { deps, events, getState } = removalHarness([true], [failure(code)]);
await performRemoveLicense(deps, activated); await performRemoveLicense(deps, activated);
expect(events).toEqual([ expect(events).toEqual([
"confirm license.removeKey.confirmActivated", "confirm t(license.removeKey.confirmActivated)",
"REMOVE_START", "REMOVE_START",
"remove(true)", "remove(true)",
"refresh",
"REMOVE_ERROR", "REMOVE_ERROR",
]); ]);
expect(getState().removeError?.code).toBe(code); expect(getState().removeError?.code).toBe(code);
expect(getState().removing).toBe(false); expect(getState().removing).toBe(false);
expect(deps.refresh).not.toHaveBeenCalled(); expect(deps.refresh).toHaveBeenCalledTimes(1);
expect(deps.logInfo).not.toHaveBeenCalled(); expect(deps.logInfo).not.toHaveBeenCalled();
expect(deps.logWarn).toHaveBeenCalledWith("License: the stored key could not be removed", code); expect(deps.logWarn).toHaveBeenCalledWith("License: the stored key could not be removed", code);
}); });
@ -905,9 +908,10 @@ describe("performRemoveLicense", () => {
const { deps, events, getState } = removalHarness([true, true], [failure("network")]); const { deps, events, getState } = removalHarness([true, true], [failure("network")]);
await performRemoveLicense(deps, valid); await performRemoveLicense(deps, valid);
expect(events).toEqual([ expect(events).toEqual([
"confirm license.removeKey.confirmValid", "confirm t(license.removeKey.confirmValid)",
"REMOVE_START", "REMOVE_START",
"remove(false)", "remove(false)",
"refresh",
"REMOVE_ERROR", "REMOVE_ERROR",
]); ]);
expect(getState().removeError?.code).toBe("network"); expect(getState().removeError?.code).toBe("network");
@ -916,9 +920,19 @@ describe("performRemoveLicense", () => {
it("names a removal that fails after the slot was given up", async () => { it("names a removal that fails after the slot was given up", async () => {
const { deps, events, getState } = removalHarness([true, true], [failure("network"), failure("storage")]); const { deps, events, getState } = removalHarness([true, true], [failure("network"), failure("storage")]);
await performRemoveLicense(deps, activated); await performRemoveLicense(deps, activated);
expect(events.slice(-2)).toEqual(["remove(false)", "REMOVE_ERROR"]); expect(events.slice(-3)).toEqual(["remove(false)", "refresh", "REMOVE_ERROR"]);
expect(getState().removeError?.code).toBe("storage"); expect(getState().removeError?.code).toBe("storage");
expect(deps.refresh).not.toHaveBeenCalled(); expect(deps.refresh).toHaveBeenCalledTimes(1);
expect(deps.logWarn).toHaveBeenLastCalledWith("License: the stored key could not be removed", "storage"); expect(deps.logWarn).toHaveBeenLastCalledWith("License: the stored key could not be removed", "storage");
}); });
it("reloads the license context after a failure, before naming it", async () => {
// Rust deletes the key first: a token that resisted leaves the key gone, and the card must
// follow the disk rather than keep the key under "not removed" (#387 review).
const { deps, events, getState } = removalHarness([true], [failure("storage")]);
await performRemoveLicense(deps, valid);
expect(events.slice(-2)).toEqual(["refresh", "REMOVE_ERROR"]);
expect(deps.refresh).toHaveBeenCalledTimes(1);
expect(getState().removeError?.code).toBe("storage");
});
}); });

View file

@ -29,9 +29,11 @@ import { useLicenseContext } from "../contexts/LicenseContext";
// A `license_revoked` answer, from any of the three calls, is persisted by the Rust // A `license_revoked` answer, from any of the three calls, is persisted by the Rust
// command that received it (#365): this hook only reloads the LicenseContext, so the // command that received it (#365): this hook only reloads the LicenseContext, so the
// edition (Free) and the card follow. The revocation check, made at app start and after // edition (Free) and the card follow. The revocation check, made at app start and after
// a key is entered (#373), shares that budget: with it, the machine-limit path reaches // a key is entered (#373), shares that budget: one check brings the machine-limit path to
// the 5 requests of the first minute. Removing the key of an activated machine spends one // the 5 requests of the first minute, and two checks in that minute (one at start for a
// deactivate call (#370). // previous key, one when a new key is entered) bring it to 6, so its last call can get a
// 429, which the card names as a rate-limit error. Removing the key of an activated
// machine spends one deactivate call (#370).
// //
// Loading the list may remove this machine's activation token, when the list names no // Loading the list may remove this machine's activation token, when the list names no
// activation of this machine (#367): the activation status is read again after every list // activation of this machine (#367): the activation status is read again after every list
@ -370,6 +372,9 @@ export interface RemoveLicenseDeps extends RemoveLicenseUi {
* staying occupied. Any other failure is named on the card. Each removal, and each release or * staying occupied. Any other failure is named on the card. Each removal, and each release or
* removal that failed, is written to the in-app log. After a removal the license context is * removal that failed, is written to the in-app log. After a removal the license context is
* reloaded: the card remounts without a key, and the revocation check has nothing to send. * reloaded: the card remounts without a key, and the revocation check has nothing to send.
* It is reloaded after a failure too (local reads only): Rust deletes the key first, so a
* token that could not be deleted leaves the key gone, and the card must not keep showing it
* under "the key was not removed".
*/ */
export async function performRemoveLicense( export async function performRemoveLicense(
deps: RemoveLicenseDeps, deps: RemoveLicenseDeps,
@ -392,6 +397,7 @@ export async function performRemoveLicense(
} }
if (!result.ok) { if (!result.ok) {
deps.logWarn(REMOVE_LOG.failed, result.error.code); deps.logWarn(REMOVE_LOG.failed, result.error.code);
await deps.refresh();
deps.dispatch({ type: "REMOVE_ERROR", error: result.error }); deps.dispatch({ type: "REMOVE_ERROR", error: result.error });
return; return;
} }