From ff7a57948f0e0798aff2ceafa841002e4549fe8f Mon Sep 17 00:00:00 2001 From: Kris McGinnes Date: Thu, 18 Jun 2026 18:23:44 -0500 Subject: [PATCH 1/8] Enable no-floating-promises and make fire-and-forget explicit Turn on the oxlint typescript/no-floating-promises rule and resolve every violation across the codebase. Persisted Jotai atom writes return a background persistence promise that was previously dropped silently, hiding real failures behind intentional fire-and-forget calls. Add a fireAndForget helper that observes failures via logger.error, and apply two idioms by meaning: fireAndForget for unawaited writes whose rejection should be logged (persisted-atom setters), and bare void for genuinely inert promises (navigation, self-catching helpers, test-state seeding). Record the convention in an ADR and the TypeScript conventions skill. --- .kiro/skills/typescript/SKILL.md | 9 +++ .oxlintrc.json | 2 +- ...exeddb-not-localstorage-for-persistence.md | 2 +- ...key-diff-merge-cross-tab-reconciliation.md | 2 +- ...18-explicit-floating-promise-convention.md | 31 ++++++++ .../src/components/Button/NavButton.tsx | 2 +- .../src/components/Redirect.tsx | 2 +- .../src/components/utils/canvas/drawImage.ts | 34 ++++---- .../src/connector/LoggerConnector.test.ts | 12 +-- .../connector/queries/edgeConnectionsQuery.ts | 54 +++++++------ .../connector/queries/schemaSyncQuery.test.ts | 16 ++-- .../src/connector/queries/schemaSyncQuery.ts | 56 ++++++------- .../src/core/AppStatusLoader.tsx | 20 ++--- .../StateProvider/atomWithLocalForage.test.ts | 22 +++--- .../core/StateProvider/displayEdge.test.ts | 12 +-- .../core/StateProvider/displayVertex.test.ts | 12 +-- .../StateProvider/graphSession/storage.ts | 5 +- .../src/core/StateProvider/localDb.test.ts | 30 +++---- .../src/core/StateProvider/schema.test.ts | 36 ++++----- .../src/core/StateProvider/schema.ts | 36 +++++---- .../core/StateProvider/storageAtoms.test.ts | 4 +- .../src/core/StateProvider/userLayout.test.ts | 14 ++-- .../src/core/StateProvider/userLayout.ts | 38 +++++---- .../src/hooks/useDeleteConfig.test.ts | 10 +-- .../src/hooks/useDeleteConfig.ts | 58 ++++++++------ .../src/hooks/useExpandNode.test.ts | 10 +-- .../src/hooks/useSchemaSync.test.ts | 10 +-- .../src/hooks/useTextTransform.test.ts | 6 +- .../AvailableConnections.test.tsx | 2 +- .../AvailableConnections/ConnectionRow.tsx | 4 +- .../useImportConnectionFile.ts | 35 +++++---- .../CreateConnection/CreateConnection.tsx | 78 ++++++++++--------- .../EdgesStyling/SingleEdgeStyling.tsx | 4 +- .../internalComponents/ContextMenu.tsx | 10 ++- .../GraphViewer/useAutoOpenDetailsSidebar.ts | 21 ++--- .../src/modules/Namespaces/UserPrefixes.tsx | 59 +++++++------- .../modules/NodesStyling/NodeStyleDialog.tsx | 8 +- .../NodesStyling/SingleNodeStyling.tsx | 3 +- .../SchemaGraph/SchemaGraphToolbar.tsx | 2 +- .../SearchSidebar/QuerySearchTabContent.tsx | 2 +- .../SearchSidebar/useKeywordSearch.test.ts | 10 +-- .../src/routes/DataExplorer/DataExplorer.tsx | 13 +++- .../src/routes/Settings/SettingsGeneral.tsx | 11 ++- .../src/utils/fireAndForget.test.ts | 26 +++++++ .../graph-explorer/src/utils/fireAndForget.ts | 17 ++++ packages/graph-explorer/src/utils/index.ts | 1 + .../src/utils/testing/DbState.ts | 20 +++-- .../src/utils/testing/createCancelledError.ts | 2 +- .../src/utils/testing/persistence.test.ts | 8 +- 49 files changed, 518 insertions(+), 363 deletions(-) create mode 100644 docs/adr/20260618-explicit-floating-promise-convention.md create mode 100644 packages/graph-explorer/src/utils/fireAndForget.test.ts create mode 100644 packages/graph-explorer/src/utils/fireAndForget.ts diff --git a/.kiro/skills/typescript/SKILL.md b/.kiro/skills/typescript/SKILL.md index 4d571a364..caf93e4d2 100644 --- a/.kiro/skills/typescript/SKILL.md +++ b/.kiro/skills/typescript/SKILL.md @@ -12,6 +12,15 @@ description: TypeScript conventions and rules for Graph Explorer, including bran - Every commit should have no type errors - Use explicit type aliases instead of `ReturnType` when available (e.g., use `AppStore` instead of `ReturnType`) +## Floating Promises + +`typescript/no-floating-promises` is enabled as an error, so every unhandled promise must be marked explicitly. Pick the idiom by meaning: + +- **`fireAndForget(promise)`** (from `@/utils`) — when a rejection is meaningful and should be observed even though you don't await it. It attaches `.catch(logger.error)`. This is the idiom for persisted-atom setters (`atomWithLocalForage`-backed writes), where a failed durable write should be logged rather than silently dropped. +- **bare `void expr;`** — when the rejection is genuinely inert or already handled elsewhere: aborted `navigate()`, a refetch whose errors surface through query state, a self-catching async helper, and test-state seeding. + +See ADR `docs/adr/20260618-explicit-floating-promise-convention.md` for the full rationale. + ## Branded Types The project uses branded types from `@/utils` for type safety. These prevent accidental mixing of similar types at compile time. diff --git a/.oxlintrc.json b/.oxlintrc.json index 316c65f83..d2d860fc0 100644 --- a/.oxlintrc.json +++ b/.oxlintrc.json @@ -28,7 +28,7 @@ "jest/require-to-throw-message": "off", "jest/valid-expect": "off", "typescript/unbound-method": "off", - "typescript/no-floating-promises": "off", + "typescript/no-floating-promises": "error", "no-unused-vars": [ "error", { diff --git a/docs/adr/20260616-indexeddb-not-localstorage-for-persistence.md b/docs/adr/20260616-indexeddb-not-localstorage-for-persistence.md index e4d6eae42..2af1174e6 100644 --- a/docs/adr/20260616-indexeddb-not-localstorage-for-persistence.md +++ b/docs/adr/20260616-indexeddb-not-localstorage-for-persistence.md @@ -2,7 +2,7 @@ - **Status:** Accepted - **Date:** 2026-06-16 -- **Related:** ADR `per-key-diff-merge-cross-tab-reconciliation` builds the cross-tab fix on top of this constraint. Issue #1820. +- **Related:** ADR `per-key-diff-merge-cross-tab-reconciliation` builds the cross-tab fix on top of this constraint. ADR `explicit-floating-promise-convention` makes this write path's fire-and-forget setters explicit. Issue #1820. ## Context diff --git a/docs/adr/20260616-per-key-diff-merge-cross-tab-reconciliation.md b/docs/adr/20260616-per-key-diff-merge-cross-tab-reconciliation.md index 104a8eff4..1dbbdf398 100644 --- a/docs/adr/20260616-per-key-diff-merge-cross-tab-reconciliation.md +++ b/docs/adr/20260616-per-key-diff-merge-cross-tab-reconciliation.md @@ -2,7 +2,7 @@ - **Status:** Accepted - **Date:** 2026-06-16 -- **Related:** ADR `indexeddb-not-localstorage-for-persistence` (IndexedDB constraint) is the storage substrate this reconciliation runs on. Issue #1820 (the clobber bug); inverse of #1788 / per-tab active connection, which wants per-tab _divergence_ rather than reconciliation. +- **Related:** ADR `indexeddb-not-localstorage-for-persistence` (IndexedDB constraint) is the storage substrate this reconciliation runs on. ADR `explicit-floating-promise-convention` makes the fire-and-forget setters on this write path explicit. Issue #1820 (the clobber bug); inverse of #1788 / per-tab active connection, which wants per-tab _divergence_ rather than reconciliation. ## Context diff --git a/docs/adr/20260618-explicit-floating-promise-convention.md b/docs/adr/20260618-explicit-floating-promise-convention.md new file mode 100644 index 000000000..f7ee3165e --- /dev/null +++ b/docs/adr/20260618-explicit-floating-promise-convention.md @@ -0,0 +1,31 @@ +# ADR — Explicit handling of intentional floating promises + +- **Status:** Accepted +- **Date:** 2026-06-18 +- **Related:** ADR `per-key-diff-merge-cross-tab-reconciliation` and `indexeddb-not-localstorage-for-persistence` describe the persistence write path whose fire-and-forget nature motivates this convention. Enables the `typescript/no-floating-promises` lint rule. + +## Context + +Many call sites start a promise and never await it. The most common is a write to a localForage-backed Jotai atom (`atomWithLocalForage`): the in-memory value updates synchronously, while the durable write happens in the background and is "never awaited in production" (see the write-path comment in `core/StateProvider/atomWithLocalForage.ts`). Other cases include react-router `navigate()`, fire-and-forget query refetches, and test-state seeding. + +Until now `typescript/no-floating-promises` was off, so these reads as ordinary statements. The hazard: a genuinely-swallowed persistence failure looks identical to an intentional fire-and-forget. The user's hypothesis was that some of those silent `store.set(...)` calls were latent bugs hiding behind that ambiguity. + +## Decision + +Turn on `typescript/no-floating-promises` (`error`) so every unhandled promise must be **explicitly** marked. Use one of two idioms by meaning: + +1. **`fireAndForget(promise)`** (`@/utils`) — attaches `.catch(logger.error)`. Use when a rejection is **meaningful and should be observed** even though the caller does not await it. This is the idiom for **persisted-atom setters**: a failed durable write gets logged rather than silently dropped, and the call sites stay greppable for follow-up auditing. + +2. **bare `void expr;`** — use when a rejection is **genuinely inert** or already handled elsewhere: aborted `navigate()`, a refetch whose errors surface through query state, a self-catching async helper, and **test-state seeding** (a test seeding store state does not care whether the value lands in durable storage). + +The reconciliation contract for user-facing persistence errors (reject + surface a toast) is a **separate, in-flight effort** on the persistence branch. This ADR deliberately does **not** change the runtime behavior of persisted setters — it only makes the existing fire-and-forget explicit. When the toast contract lands, the `fireAndForget`-wrapped userStyling call sites are the ones to revisit toward a handled-reject form. + +### Alternatives considered + +**Centralize the `.catch` inside `atomWithLocalForage` and return `void`.** The writer atom could attach its own `.catch(logger.error)` and return `void`, deleting most of the ~25 `fireAndForget(set(...))` wrappers at call sites. We rejected this for now because the in-flight persistence work (reject + toast, per `per-key-diff-merge-cross-tab-reconciliation`) needs _per-call-site_ control over how a failure is surfaced — a single swallow point in the writer would have to be unwound again to differentiate "log only" from "toast the user." Keeping the decision at each call site also keeps the persisted-write sites **greppable** for the #1854 audit. Once the surfacing contract is settled, collapsing the common case back into the writer is a reasonable follow-up. + +## Consequences + +- A swallowed persistence error is now either logged (`fireAndForget`) or a deliberate, reviewed `void` — no longer invisible. +- Reviewers have a clear rule: meaningful-but-unawaited → `fireAndForget`; inert → `void`. +- The lint rule prevents silent fire-and-forget from being reintroduced anywhere in the codebase. diff --git a/packages/graph-explorer/src/components/Button/NavButton.tsx b/packages/graph-explorer/src/components/Button/NavButton.tsx index 85db9844e..97821a7ea 100644 --- a/packages/graph-explorer/src/components/Button/NavButton.tsx +++ b/packages/graph-explorer/src/components/Button/NavButton.tsx @@ -24,7 +24,7 @@ function NavButton({ to, navOptions, ...props }: NavButtonProps) { const navigate = useNavigate(); function handleClick() { - navigate(to, navOptions); + void navigate(to, navOptions); } return