diff --git a/.serena/memories/frontend/workspace-state-persistence-subtleties.md b/.serena/memories/frontend/workspace-state-persistence-subtleties.md index b07fee51d3..be612121e1 100644 --- a/.serena/memories/frontend/workspace-state-persistence-subtleties.md +++ b/.serena/memories/frontend/workspace-state-persistence-subtleties.md @@ -24,7 +24,7 @@ - Persistence is skipped in version preview/read-only mode or without edit permission. - Save failures split transient vs terminal (`transient-error?`: the repo retryable types `:network`/`:offline`/`:bad-gateway`/`:service-unavailable` plus `:invalid-save-response`; everything else is terminal). Terminal keeps the `:error` halt + `flash-persistence` path; transient enters a `:retrying` episode: the head commit stays queued and resends with backoff 2s/8s/20s (3 retries, then today's terminal path). Resends rotate the `::request-id` stamp only when the old request left `active-requests`, carry the same `:commit-id`, and never double-send while one request is in flight (`:in-flight` stays silent). Retry timers carry the episode token; a superseded token stays silent. Status stays `:retrying` through re-entries (`next-status` refuses `:pending`/`:saving` from it); waiters (`wait-persisted-or-error`) wait through it and reject only on `:error`. - One reconnect notice per episode: sticky toast tagged `:persistence-reconnecting` (single-toast store, re-show replaces), hidden by tag on drain (`:saved`) and on terminal failure; recovery is silent. Header indicator has a `:retrying` state (`workspace.header.retrying`). -- Resume triggers: backoff timer, the browser `online` event (guarded by `exists? js/window`; re-enters the runner only for a live `:retrying` episode), and new local edits (`append-commit` re-enters the runner during `:retrying`; the emission retires the stuck runner via its stopper). +- Resume triggers: backoff timer and the browser `online` event (guarded by `exists? js/window`; re-enters the runner only for a live `:retrying` episode). New local edits during `:retrying` only join the queue: the episode keeps its `:run-id`, so `append-commit` does not re-enter, and the live runner (still waiting on the head's `commit-persisted`) sends them after the head. Re-entering on edits would bypass the backoff and spend an attempt per batched commit. - Tests instant-trigger retries by stubbing `rx/timer` (recording delays to assert the schedule); dynamic bindings do not survive `await` continuations, so no dynamic var for delays. - Undo transactions can stay open only temporarily; timed-out pending transactions are force-committed after about 20s. Undo entries are capped at 50. - Undo/redo are ignored while a normal editor/drawing interaction is active, except grid-layout edition handles undo through this path. diff --git a/frontend/src/app/main/data/persistence.cljs b/frontend/src/app/main/data/persistence.cljs index f2c6cfafeb..a92b4e1ae3 100644 --- a/frontend/src/app/main/data/persistence.cljs +++ b/frontend/src/app/main/data/persistence.cljs @@ -232,14 +232,11 @@ ptk/WatchEvent (watch [_ state _] (let [pstate (:persistence state)] - ;; A new edit during `:retrying` re-enters the runner even though - ;; the run id no longer matches: the previous runner is stuck - ;; behind the failed head, and the emission itself retires it - ;; through its stopper. Concurrent re-entries collapse to a - ;; single send via the in-flight guard in `attempt-state`. + ;; A `:retrying` episode keeps its run id, so a new edit only + ;; joins the queue: the live runner sends it after the head, and + ;; resends stay on the backoff schedule and its attempt budget. (when (and (not= :error (:status pstate)) - (or (= run-id (:run-id pstate)) - (= :retrying (:status pstate)))) + (= run-id (:run-id pstate))) (rx/of (update-status :saving) (run-persistence-task)))))))) diff --git a/frontend/src/app/main/errors.cljs b/frontend/src/app/main/errors.cljs index ceda6f90db..51b52afda9 100644 --- a/frontend/src/app/main/errors.cljs +++ b/frontend/src/app/main/errors.cljs @@ -400,6 +400,12 @@ (dom/trigger-download-uri "report" "text/plain" uri) (ts/schedule-on-idle #(wapi/revoke-uri uri)))) +(defn- report-format + "Payload format for `cause`: compact for environment failures, full + otherwise." + [cause] + (if (environment-error? cause) :compact :full)) + (defn- emit-flash-report! "Reserves, generates and emits the flash report. Returns the generated report string, or nil when nothing is emitted (non-exception cause, @@ -410,7 +416,7 @@ :handled "handled-exception" :unhandled "unhandled-exception" :silent nil)] - (let [format (if (environment-error? cause) :compact :full) + (let [format (report-format cause) report-hint (ex/get-hint cause)] (when (and (string? report-hint) (not (str/empty? report-hint))) (when-let [{:keys [occurrences]} (reserve! event-name cause)] @@ -432,6 +438,9 @@ The report is reserved before being generated, so repeated errors that fall inside the governor window do not pay the report-building cost. + With `:report-link?` the toast always carries a download link: when no + report is emitted (suppressed repeat, empty hint, `:silent`), one is + generated for the link alone, without emitting it. The whole body (report pipeline first, toast after) runs inside a single `ts/schedule` callback: nothing report- or toast-related executes @@ -457,19 +466,23 @@ (fn [resolve _reject] (ts/schedule (fn [] - (let [report (try (emit-flash-report! type cause) - (catch :default err - (.error js/console "error on emitting report" err) - nil))] + (let [report (try (emit-flash-report! type cause) + (catch :default err + (.error js/console "error on emitting report" err) + nil)) + link-report (when report-link? + (or report + (when (ex/exception? cause) + (generate-report cause {:format (report-format cause)}))))] (try (st/emit! (ntf/show (cond-> {:content (or ^boolean hint (tr "errors.generic")) :type :toast :level :error :timeout timeout} - (and report-link? report) + link-report (assoc :links [{:label (tr "labels.download" "report.txt") - :callback (partial download-report! report)}])))) + :callback (partial download-report! link-report)}])))) (catch :default err (.error js/console "error on emitting toast" err))) (resolve report))))))) diff --git a/frontend/test/frontend_tests/data/persistence_retry_test.cljs b/frontend/test/frontend_tests/data/persistence_retry_test.cljs index 5da357242f..87b5745624 100644 --- a/frontend/test/frontend_tests/data/persistence_retry_test.cljs +++ b/frontend/test/frontend_tests/data/persistence_retry_test.cljs @@ -179,9 +179,9 @@ (fn [_ _] (rx/throw (ex-info "offline" {:type :offline}))))))) ;; Scenario: a retry timer fires while the replacement request is still in -;; flight. The timer is driven by hand: fail the first send, queue a second -;; edit so its run resends (hanging), then fire the pending timer. Proves: -;; the firing is skipped instead of double-sending the same commit. +;; flight. The timer is driven by hand: fail the first send, let the +;; online signal resend the head (hanging), then fire the pending timer. +;; Proves: the firing is skipped instead of double-sending the same commit. (t/deftest ^:async retry-skips-resend-while-previous-request-is-in-flight (let [calls (atom 0) delays (atom []) @@ -198,11 +198,10 @@ (await (async/wait-for #(= :retrying (get-in @store [:persistence :status])) "transient failure retries")) (t/is (= 1 (count @requests))) - ;; Phase 2 — a new edit re-enters the runner under the live - ;; episode (status stays :retrying) and resends the head, - ;; hanging in flight. - (ptk/emit! store (local-commit file-id) ::dps/force-persist) - (await (async/wait-for #(= 2 (count @requests)) "second edit resends")) + ;; Phase 2 — the online signal resends the head under the live + ;; episode (status stays :retrying), hanging in flight. + (ptk/emit! store (#'dps/resume-on-online)) + (await (async/wait-for #(= 2 (count @requests)) "online resends")) (t/is (= :retrying (get-in @store [:persistence :status]))) (t/is (= 1 (get-in @store [:persistence :attempts]))) ;; Phase 3 — the pending timer fires into the in-flight request: @@ -211,7 +210,7 @@ (await (async/settle)) (t/is (= 2 (count @requests)) "no double-send while in flight") (t/is (= :retrying (get-in @store [:persistence :status]))) - (t/is (= 2 (count (get-in @store [:persistence :queue])))) + (t/is (= 1 (count (get-in @store [:persistence :queue])))) (rx/end! timer-s)))) (fn [_ _] (if (= 1 (swap! calls inc)) @@ -242,6 +241,51 @@ (t/is (nil? (get-in @store [:persistence :error])))))) (fn [_ _] (rx/throw (ex-info "offline" {:type :offline})))))) +;; Scenario: a new edit lands while the episode waits on its backoff timer. +;; The edit only joins the queue: nothing is sent and no attempt is spent +;; until the timer fires. The retry then saves the head and the runner +;; sends the new edit after it. Proves: edits during an episode never +;; bypass the backoff nor consume the retry budget. +(t/deftest ^:async new-edit-during-retry-waits-for-the-backoff + (let [calls (atom 0) + delays (atom []) + timer-s (rx/subject)] + (await + (with-persistence + (^:async fn [{:keys [file-id requests store]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub (fn [ms] (swap! delays conj ms) timer-s))} + ;; Phase 1 — first send fails transiently; the retry pends on + ;; the hand-fired timer. + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= :retrying (get-in @store [:persistence :status])) + "transient failure retries")) + (t/is (= 1 (count @requests))) + ;; Phase 2 — a new edit joins the queue without resending. + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= 2 (count (get-in @store [:persistence :queue]))) + "second edit is queued")) + (await (async/settle)) + (t/is (= 1 (count @requests)) "the edit does not resend the head") + (t/is (= 1 (get-in @store [:persistence :attempts])) "no attempt is spent") + (t/is (= [2000] @delays) "the pending backoff is kept") + (t/is (= :retrying (get-in @store [:persistence :status]))) + ;; Phase 3 — the timer fires: the head saves, then the new edit. + (rx/push! timer-s :tick) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "both edits save")) + (t/is (= 3 (count @requests)) "failed send, retry, then the new edit") + (let [[first-id retry-id edit-id] (map (comp :commit-id second) @requests)] + (t/is (= first-id retry-id) "the retry resends the head") + (t/is (not= retry-id edit-id) "the new edit is sent after the head")) + (rx/end! timer-s)))) + (fn [_ _] + (if (= 1 (swap! calls inc)) + (rx/throw (ex-info "offline" {:type :offline})) + (rx/of {:revn @calls}))))))) + ;; Scenario: a transient failure raises the reconnect notice; the retry ;; then succeeds. The timer is driven by hand so the test observes the ;; episode mid-flight: one visible notice (the store holds a single toast, @@ -342,11 +386,11 @@ (filter #(= ::ev/event (ptk/type %)) events)) ;; Scenario: the `online` event arrives while a retry request is already in -;; flight. The first send fails transiently, a new edit re-enters and -;; resends (hanging), then connectivity reports back mid-flight. Proves: -;; the online entry point honors the in-flight guard instead of -;; double-sending — the same guard as the retry-timer path, through -;; `resume-on-online` -> `run-persistence-task`. +;; flight. The first send fails transiently, the retry timer resends +;; (hanging), then connectivity reports back mid-flight. Proves: the online +;; entry point honors the in-flight guard instead of double-sending — the +;; same guard as the retry-timer path, through `resume-on-online` -> +;; `run-persistence-task`. (t/deftest ^:async online-event-does-not-resend-an-in-flight-request (let [calls (atom 0) timer-s (rx/subject)] @@ -362,10 +406,10 @@ (await (async/wait-for #(= :retrying (get-in @store [:persistence :status])) "transient failure retries")) (t/is (= 1 (count @requests))) - ;; Phase 2 — a new edit re-enters the runner under the live - ;; episode and resends the head, hanging in flight. - (ptk/emit! store (local-commit file-id) ::dps/force-persist) - (await (async/wait-for #(= 2 (count @requests)) "second edit resends")) + ;; Phase 2 — the retry timer fires and resends the head, + ;; hanging in flight. + (rx/push! timer-s :tick) + (await (async/wait-for #(= 2 (count @requests)) "retry resends")) (t/is (= :retrying (get-in @store [:persistence :status]))) ;; Phase 3 — online arrives mid-flight: silent, never a third send. (ptk/emit! store (#'dps/resume-on-online)) diff --git a/frontend/test/frontend_tests/errors_governor_test.cljs b/frontend/test/frontend_tests/errors_governor_test.cljs index add35d862c..c0998a7280 100644 --- a/frontend/test/frontend_tests/errors_governor_test.cljs +++ b/frontend/test/frontend_tests/errors_governor_test.cljs @@ -357,3 +357,47 @@ tm/schedule (mock/stub (fn [f] (mock/asap f)))} (await (errors/flash :cause cause :type :handled)) (t/is (= 1 (count @events)) "only the toast survives a reporting failure"))))) + +(defn- toast-links + "The download links of every toast collected through the `st/emit!` + double, one entry per toast." + [events] + (->> events + (remove #(= ::ev/event (ptk/type %))) + (map #(get-in (ptk/update % {}) [:notification :links])))) + +(t/deftest ^:async repeated-flash-keeps-the-report-download-link + ;; Scenario: the same save failure flashes twice inside the governor + ;; window with a report link. The repeat is not emitted, but its toast + ;; still offers the report. Proves: the governor bounds emission, never + ;; the user's download. + (let [generated (atom 0) + cause (error-cause :type :validation :hint "save failed") + events (atom [])] + (await + (mock/with-mocks* + {st/format-last-events (mock/stub (fn [& _] (swap! generated inc) "report")) + st/emit! (mock/stub (fn [& emitted] (swap! events into emitted))) + rt/get-current-href (constantly "https://penpot.example.com/#/workspace") + tm/schedule (mock/stub (fn [f] (mock/asap f)))} + (dotimes [_ 2] + (await (errors/flash :cause cause :type :handled :report-link? true))) + (t/is (= 1 (count (report-events @events))) "the repeat is not emitted") + (t/is (= [1 1] (map count (toast-links @events))) "both toasts carry the link") + (t/is (= 2 @generated) "the repeat builds its report for the link only"))))) + +(t/deftest ^:async unreportable-flash-keeps-the-report-download-link + ;; Scenario: a failure with an empty hint flashes with a report link. It + ;; cannot be emitted, but its toast still offers the report. Proves: the + ;; link does not depend on emission. + (let [cause (ex-info "" {:type :validation}) + events (atom [])] + (await + (mock/with-mocks* + {st/format-last-events (mock/stub (fn [& _] "report")) + st/emit! (mock/stub (fn [& emitted] (swap! events into emitted))) + rt/get-current-href (constantly "https://penpot.example.com/#/workspace") + tm/schedule (mock/stub (fn [f] (mock/asap f)))} + (await (errors/flash :cause cause :type :handled :report-link? true)) + (t/is (empty? (report-events @events)) "nothing is emitted") + (t/is (= [1] (map count (toast-links @events))) "the toast carries the link")))))