Skip to content

[Obsidian review blockers] - Step 4: Contain async UI work - #2786

Closed
logancyang wants to merge 1 commit into
codex/issue-285-03-obsidian-domfrom
codex/issue-285-04-async-boundaries
Closed

[Obsidian review blockers] - Step 4: Contain async UI work#2786
logancyang wants to merge 1 commit into
codex/issue-285-03-obsidian-domfrom
codex/issue-285-04-async-boundaries

Conversation

@logancyang

Copy link
Copy Markdown
Owner

Relates to logancyang/obsidian-copilot-preview#285

Why

Obsidian's review identifies floating promises and async callbacks passed to void-returning UI and plugin lifecycle boundaries. Rejections at those boundaries can otherwise escape without context, while changing every callback into ad hoc void expressions would make the error policy inconsistent.

What

This step adds one small toVoidHandler boundary helper that preserves callback arguments and reports synchronous throws and rejected promises through the existing logger. Affected React handlers, Obsidian modal callbacks, gallery lifecycle hooks, and plugin lifecycle entry points now make their fire-and-forget behavior explicit without changing the underlying async work or ordering.

Before: async UI and lifecycle callbacks rely on implicit promise disposal, with inconsistent rejection handling.

After: those same callbacks enter through an explicit void boundary that logs failures and preserves existing behavior.

Non goal

  • Change task ordering, retry behavior, cancellation, or concurrency.
  • Redesign chat, settings, Agent Mode, gallery, or modal UX.
  • Change persisted settings, chat formats, credentials, or user data schemas.
  • Add new dependencies or a new async abstraction layer.
  • Address CSS, dependency, or remaining strict-lint review findings in this stack step.
  • Reimplement or authenticate against Obsidian's private review service.

Screenshot

Not applicable — UI structure, copy, styling, and interaction outcomes are unchanged.

Risk

High

Criterion Status Reason
No behavior change, or a cosmetic/copy/docs/config change visible where it renders, or deterministic tests cover the changed behavior Async entry points span several UI surfaces; focused tests cover representative paths but not every click handler
A defect would fail CI or be obvious on first use Boundary mistakes surface as failed focused tests, build errors, or logged callback failures
A revert fully restores prior state, including persisted data No persisted data changes
No auth, permissions, secrets, or input-handling surface changes Existing handler inputs are forwarded unchanged
No public API, plugin API, message, or on-disk contract changes The helper is internal and existing data contracts are unchanged
No core-path concurrency, async-lifecycle, or state-machine changes Plugin lifecycle and chat/settings callbacks are explicitly wrapped
No hot-path behavior lacks deterministic coverage Representative chat, gallery, settings, and helper paths are covered, but not every affected handler
No new dependency Dependency manifests are unchanged
Human-only behavior stays in one feature area and surfaces quickly Manual verification spans plugin load, chat, settings, and Agent Mode

Review: start with src/utils/asyncHandler.ts and src/main.ts, then inspect representative call sites in src/components/Chat.tsx, src/agentMode/ui/AgentChatInput.tsx, and src/settings/v2/components/PlusSettings.tsx.

Verification

  1. Run npm test -- --runInBand dev/gallery/main.test.ts src/utils/asyncHandler.test.ts.
  2. Run npm test -- --runInBand src/modelManagement/ui/dialogs/ConfigureProviderDialog.test.tsx src/agentMode/ui/AgentChatInput.test.tsx src/settings/v2/components/PlusSettings.test.tsx.
  3. Run npm run build.
  4. In Obsidian, load and unload Copilot, then exercise chat send, Agent Mode input, provider configuration, and Plus settings actions; confirm outcomes are unchanged and failures are logged rather than emitted as unhandled rejections.

Note

npm run format passes. The staged-file lint gate passes with one pre-existing test-only mobile warning in the gallery fixture. Repo-wide npm run lint still reports later review families tracked by issue #285; those are intentionally isolated into subsequent stack steps.

@logancyang
logancyang force-pushed the codex/issue-285-04-async-boundaries branch from 82b7e3c to 9e2d765 Compare August 8, 2026 23:34
@logancyang
logancyang marked this pull request as ready for review August 8, 2026 23:39

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9e2d765ac7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +83 to +84
void navigator.clipboard
.readText()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Catch synchronous clipboard API failures

When this control runs in a WebView or environment where navigator.clipboard or readText is unavailable, evaluating this expression throws before a promise exists, so the chained .catch() never runs and clicking the paste button raises an uncaught exception. The previous try/catch also covered these synchronous failures; retain that boundary while explicitly discarding the returned promise.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid for this abandoned diff, but no longer applicable to the retained stack. PR #2786 was closed without merge; the current top stack at 3e257cb2 retains the async try/catch around navigator.clipboard.readText(), covering both synchronous API-access failures and rejected reads. I would not make a new change because the unsafe boundary change never shipped.

@logancyang

Copy link
Copy Markdown
Owner Author

Conservative risk disposition

Risk: High — deferred and removed from the required stack.

This PR changes async callback and lifecycle boundaries across core React UI. Those edits can affect event timing, unmount behavior, error propagation, duplicate work, or test scheduling even when the visible intent is only lint compliance.

The current findings are warning-only. Recommendation: close this PR and revisit individual warnings only with focused behavioral tests; no async/UI lifecycle changes should ride with the Obsidian review blockers.

@logancyang logancyang closed this Aug 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant