Chore/docs retry and modal race fixes - #1289
Merged
github-actions[bot] merged 5 commits intoJul 26, 2026
Merged
Conversation
Expand the rustdoc on `FiatBridge::get_escrow_record` from nothing to a full entry: what the function is for (the read side of the receipt→escrow migration, and the dense-id path indexers use instead of receipt hashes), its parameter, all three distinct reasons it can return `None` and how to tell them apart, the fact that it cannot fail or panic, TTL and version caveats, and a worked deposit→migrate→read example. Also add a one-line doc to `EscrowRecord`, the type it returns. Cross-references use intra-doc links spelled `FiatBridge::…` rather than `Self::…`; `#[contractimpl]` copies doc comments onto generated spec items where `Self` no longer resolves, so the surrounding `Self::` links in this file all emit `broken_intra_doc_links` warnings. The links added here resolve cleanly. Docs are deliberately limited to these two items: soroban-sdk embeds doc strings in the on-chain contract spec, and the CI WASM size guard (92160 bytes) only had ~2.2 KB of headroom. Documenting the neighbouring escrow-migration entry points as well pushed the artifact over the limit. The build now sits at 91173 bytes. No behavioural changes.
Add TSDoc across `src/lib/filterTelemetry.ts`, which had one comment on a
private helper and nothing on its public surface.
- A file header explaining why filter events ride the shared
`chat:telemetry` channel instead of widening `ChatEventName`.
- `FilterEventName` and each payload interface, field by field, including
the `nextValue`/`isCleared` relationship.
- `emit`: its three guards (consent, SSR, deferred dispatch), and an
explicit note that it never throws — so delivery failures are silent and
the channel must not be used for anything the app depends on.
- `filterTelemetry` and each method, with a worked example covering a chip
toggle, a keyboard cycle, a wrap-around clear, and a bulk reset.
- Cross-references via `{@link …}` to the sibling `chatTelemetry` module.
No behavioural changes.
…et#1210) Root cause: the effect listed `onClose` in its dependency array. Every consumer passes an inline callback, so any parent re-render tore the whole modal setup down and built it again while the modal was still open. Two things raced as a result, and a third failure was silent. 1. Body scroll lock. Each setup snapshotted `document.body.style.overflow` into a per-effect local. With two modals stacked (BottomSheet over BankDetailsModal) the inner one snapshotted the outer one's 'hidden'. Closing the outer modal first unlocked the page while the inner one was still up; closing the inner one then wrote 'hidden' back to a page with no modals on it, leaving it permanently unscrollable. 2. Focus. Every re-setup re-applied initial focus, yanking the user back to the first control mid-form, and every teardown bounced focus through the opener on the way past. 3. Focus restore. `previousActive?.focus()` on a node that had since been unmounted is a no-op, silently stranding focus on the closing modal's own controls. Fix: - `onClose` is read through a ref, so the effect keys on `isOpen` alone and runs exactly once per open/close. The latest callback is still the one invoked on Escape. - The scroll lock is reference-counted at module scope: only the first acquire records the pre-modal overflow, only the last release restores it. - Focus is restored only to an opener still connected to the document; otherwise the active element is blurred so focus falls back to the body. - Error paths surface a message instead of failing silently. If the container never mounts (retried once on the next frame, for animated sheets) or holds nothing focusable and cannot take focus itself, the hook raises an error toast — overridable via a new `onError` option — telling the user the dialog is not keyboard-navigable. Reported at most once per open so a held Tab key cannot spam it. Regression tests: 14 cases in `useAccessibleModal.test.tsx`. Six of them fail against the previous implementation — the stacked-modal lock, focus stolen on re-render, focus restore to a detached opener, and the three error-path cases — and all 14 pass after the fix. No `any` casts; `pnpm typecheck` and `pnpm test:unit` are unchanged.
…1043) A message that failed to send showed an error indicator with a Retry button that no caller ever wired up, so users had to retype the message. - `useMessageRetry` (new) owns the retry policy: up to 3 automatic resends on exponential backoff (1s, 2s, 4s), a countdown to the next attempt, and a `retryNow` escape hatch. Manual retries cancel the pending timer and resend immediately without spending the automatic budget, so the remaining attempts still follow if the resend fails again. A message that recovers gets a fresh budget. Rejected and synchronously-throwing handlers are treated as ordinary failures rather than stalling the loop. - `Message` renders the retry action with a live-region status line ("Retrying automatically in Ns (attempt 1 of 3)" → "Resending…" → exhausted), disables the button while a resend is in flight, and counts attempts already recorded on the message alongside its own. - Retry content comes from `message.originalPayload.content`, falling back to `message.content`. That is what pre-populates the resend: the original text the user typed survives even though `content` is overwritten with a failure placeholder when a send fails. - `onRetry` now carries that content — `(messageId, content)` — so callers do not have to reconstruct it. Wired through `ChatMessages` to `StellarChatInterface`, which resends via `sendMessage`. - Status copy added to the en/fr/es locale files. Tests: 12 cases for the hook, 8 for the component.
|
@nonso7 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
`pnpm lint` (and therefore `pnpm build`) failed on main, and 19 unit tests failed alongside it. All of these predate the current branch. Lint: - `aiAssistant.test.ts` did not parse: the last `it()` in the abort-signal suite was never closed, so its `});` closed the `it` and left the `describe` open. Added the missing brace. - `apiSchemas.ts` stapled `status` and `response` onto a plain `Error` through two `any` casts. Replaced with an `HttpResponseError` class that declares both as typed readonly fields. - `apiSchemas.test.ts` had an unused `startTime` and an `as any` on a `setTimeout` spy. - `OfflineStatusBanner.tsx` kept a `pendingCount` state nothing read and two optimistic-update callbacks nothing called. The queued count is owned by `offlineMessageQueue` and published by `useChat`; the banner only mirrors it, so the local copies were redundant. Removed, along with the imports they were the only users of. - `OfflineStatusBanner.test.tsx` imported `renderHook` without using it. Two real bugs surfaced while fixing the above: - `withRetry` decided status retries with `error instanceof Response`. The value thrown by `fetchWithRetry` is an `Error`, never a `Response`, so that arm was dead and 500/503/429 responses were never retried — the `any` casts were what hid the status from the check. It now tests `HttpResponseError` (keeping the `Response` arm for callers that throw one directly). An explicit `AbortError` guard was also added, so an aborted request is never retried regardless of a caller's `retryableErrors`. - `OfflineStatusBanner` ignored `wasOffline`, announcing a reconnect only when it observed the offline render itself. Mounting after the connection was already back skipped the toast and never called `resetWasOffline`, leaving the hook's latch set forever. The reconnect branch now also fires on a latched `wasOffline` and consumes the latch immediately. Tests: the failures were fake-timer deadlocks. Suites installed `vi.useFakeTimers()` and then awaited code that sleeps between retries, or polled with `waitFor` while the clock was frozen — so the promise never settled and the test hit its 15s timeout. Fixed by draining timers with `vi.runAllTimersAsync()`, attaching rejection handlers before draining (an unhandled rejection otherwise escapes), and replacing frozen-clock `waitFor` calls with assertions made directly after an explicit `advanceTimersByTime`. `OfflineStatusBanner`'s pending-count test also pulled from `getQueuedMessageCount`, which the component never reads; it now drives the subscription the component actually listens to. pnpm typecheck, pnpm lint, pnpm build and pnpm test:unit (805 tests) all pass.
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
1 participant
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.
#1239 — get_escrow_record docs (lib.rs:3243)
Intent, parameter, all three distinct reasons it returns None and how to distinguish them, the fact it cannot fail or panic, TTL/version caveats, and a worked deposit→migrate→read example. Cross-refs use FiatBridge::… rather than Self::… — #[contractimpl] copies docs onto generated spec items where Self doesn't resolve, which is why the file's existing Self:: links all emit rustdoc warnings. Mine resolve cleanly.
#1247 — filterTelemetry docs (filterTelemetry.ts)
File header, every exported type/field, the emit helper's three guards, and each public method with a worked example. Documented explicitly that it never throws, so delivery failures are silent.
#1210 — useAccessibleModal race (useAccessibleModal.ts)
Root cause: onClose was an effect dependency and every consumer passes an inline callback, so the whole modal setup tore down and rebuilt on each parent re-render. Fallout: stacked modals clobbered each other's scroll-lock snapshot (page permanently unscrollable), focus was yanked back to the first control mid-form, and restoring focus to an unmounted opener silently stranded it. Fixed with a ref for onClose, a module-scope reference-counted scroll lock, connected-node-checked focus restore, and an error toast when the trap can't take hold. 14 regression tests — 6 fail against the old implementation, all 14 pass after.
#1043 — resend failed message (useMessageRetry.ts, Message.tsx:250)
3 automatic retries at 1s/2s/4s with a live countdown, plus a manual button that doesn't spend the automatic budget. Content comes from originalPayload.content — the text the user actually typed, which survives content being overwritten with the failure placeholder. onRetry now carries it, wired through ChatMessages to StellarChatInterface. 20 new tests; status copy added to en/fr/es.
Two things to flag
The contract WASM size guard drove a scope cut. soroban-sdk embeds doc strings in the on-chain spec, and CI's limit (92160 bytes) had only ~2.2 KB of headroom over main's 89929. I'd initially also documented migrate_escrow, get_migration_cursor, get_escrow_storage_version and EscrowRecord's fields — that came to 94589 bytes, over the limit. I cut back to get_escrow_record plus a one-line EscrowRecord doc, landing at 91173 bytes. Documenting the rest of the escrow area needs MAX_WASM_BYTES raised in .github/workflows/contracts.yml; I didn't touch it, since weakening a size guard as a side effect of a docs PR is your call.
Pre-existing failures on main, not from these changes. pnpm lint and pnpm build fail on apiSchemas.ts, OfflineStatusBanner.tsx, and a parse error in aiAssistant.test.ts; pnpm test:unit fails 19 tests across those same files plus CCIPBridgeModal. I verified identical failures on a stashed baseline. My files are lint-clean, pnpm typecheck passes, next build compiles successfully (it's the lint gate that fails), and contracts pass cargo test (282), cargo clippy -D warnings, and the WASM build.
Push only — I didn't open a PR against leojay-net/Stellar-Dex-Chat. Say the word and I'll open it, or four separate ones if you'd rather they track the issues individually.
Closes #1043
Closes #1210
Closes #1239
Closes #1247