Skip to content

chore(e2e): retire lost-reply workarounds obsolete since obsidian-e2e 0.10.0 - #1668

Merged
chhoumann merged 2 commits into
masterfrom
chore/e2e-retire-lost-reply-workarounds
Aug 12, 2026
Merged

chore(e2e): retire lost-reply workarounds obsolete since obsidian-e2e 0.10.0#1668
chhoumann merged 2 commits into
masterfrom
chore/e2e-retire-lost-reply-workarounds

Conversation

@chhoumann

@chhoumann chhoumann commented Aug 12, 2026

Copy link
Copy Markdown
Owner

obsidian-e2e 0.10.0 (obsidian-e2e#25, PR #26) made exec() - and everything riding it: dev.eval*, plugin.reload(), quickadd:* CLI calls - recover lost CLI replies via a nonce-registry dispatch shim, and evalJsonAsync already runs kickoff-and-poll internally with exactly-once semantics. This retires the hand-rolled defense quickadd carried for that failure mode.

Removed / updated

  • tests/e2e/apply-template-to-active-note.test.ts - applyTemplate fire-and-poll helper: replaced evalRaw + window.__qaApplyTplResult + waitFor(evalJson) polling with a single awaited evalJsonAsync call. The comment justifying the decoupling ("a long-awaited eval can stall and time out even after the in-app operation completed") describes exactly the lost-reply mode 0.10.0's recoverable exec fixes; evalJsonAsync is now that fire-and-poll pattern, managed by the package. The in-app { ok, error } catch stays so A07/A08 keep asserting on rejection messages, and the vault-index poll loop stays (index lag is real and unrelated to the transport).
  • package.json + pnpm-lock.yaml: obsidian-e2e ^0.9.0^0.10.0, the version that ships the recovery. Lockfile regenerated and verified consistent with --frozen-lockfile.

Deliberately kept

  • vitest.e2e.config.mts testTimeout: 60s / hookTimeout: 30s: predate the transport-hang era entirely (set at suite creation in test(e2e): add file collision behavior coverage #1153, March 2026) - they are generic e2e budgets, not lost-reply padding, and hooks still need headroom for the ~5s-scale recovery poll latency on top of reload/lock work.
  • Per-test 30_000 timeouts and all 10-15s WAIT_OPTS / waitForContent / waitUi polls: these wait on vault-content writes, metadata indexing, and UI state - real in-app latencies the recoverable transport does not remove.
  • exec("quickadd:run", ...) call sites: unchanged; they gain recovery automatically in 0.10.0, which explicitly keeps non-idempotent quickadd:run-style commands exactly-once.
  • A00 regression test: still pins the obsidian-e2e#18 envelope-corruption fix; unrelated to the transport recovery.

Sweep covered tests/e2e/, docs/, configs, and inline comments for fire-and-poll, window-global, evalJsonAsync-guidance, 30s/transport/lost-reply language (via git grep, avoiding the known ripgrep NUL-byte blindspot). The helper above was the only workaround found.

Verification

  • Full fresh-launch loop run twice (stop:e2e-obsidianstart --print-envtest:e2e): 49/49 e2e tests green both rounds.
  • Unit suite: 4913 passed / 37 skipped. tsc -noEmit and eslint clean.

Summary by CodeRabbit

  • Chores
    • Updated development tooling to improve compatibility and reliability.
    • Improved automated validation for template application workflows.
  • User Experience
    • No direct changes to end-user functionality or interface.

@chhoumann
chhoumann marked this pull request as ready for review August 12, 2026 21:52
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: eedbd526-8339-4377-bc78-f24bdb662428

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: dc35291f-4f16-4bbd-8aa3-433befca7d23

📥 Commits

Reviewing files that changed from the base of the PR and between 146e9b0 and 751c952.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (2)
  • package.json
  • tests/e2e/apply-template-to-active-note.test.ts

📝 Walkthrough

Walkthrough

The E2E dependency now uses version ^0.10.0. The template application helper uses evalJsonAsync and returns structured success or failure results without window-global polling.

Changes

E2E structured result handling

Layer / File(s) Summary
Direct JSON evaluation
package.json, tests/e2e/apply-template-to-active-note.test.ts
The E2E dependency uses ^0.10.0. The helper calls evalJsonAsync<ApplyResult> and returns direct { ok: true, path } or { ok: false, error } results. It removes the window-global transport and polling logic.

Estimated code review effort: 2 (Simple) | ~10 minutes

Mergeability Score: ⚪ Minimal · up to 751c9

This PR removes obsolete e2e transport workarounds and updates the supporting dependency without changing product runtime behavior; the localized test changes are merge-ready after normal checks, with no actionable merge-blocking risk remaining.

Possibly related PRs

Suggested reviewers: semantic-release-bot

Poem

I hop through JSON, neat and bright,
No window globals hide from sight.
Success or error, paths are clear,
Polling flakes no longer appear.
A tidy test brings carrots near.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: removing obsolete lost-reply workarounds after upgrading to obsidian-e2e 0.10.0.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/e2e-retire-lost-reply-workarounds

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 12, 2026

Copy link
Copy Markdown

Deploying quickadd with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5096e27
Status: ✅  Deploy successful!
Preview URL: https://b407d957.quickadd.pages.dev
Branch Preview URL: https://chore-e2e-retire-lost-reply.quickadd.pages.dev

View logs

@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: 751c9527f8

ℹ️ 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 thread pnpm-lock.yaml Outdated
@chhoumann
chhoumann merged commit 2b5c225 into master Aug 12, 2026
11 checks passed
@chhoumann
chhoumann deleted the chore/e2e-retire-lost-reply-workarounds branch August 12, 2026 22:08
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