Repository navigation
fix(editor): ci stability - #14704
Merged
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughIntroduces polling-based Playwright test helpers and widespread test synchronization; rewrites scroll/click helpers to use locator.evaluate + expect.poll; hardens Electron page selection, fixtures, and CI/Linux teardown; implements staged, resilient disposal for Electron tab webContents; converts test cleanup functions to async and tracks worker termination. Changes
Sequence Diagram(s)sequenceDiagram
participant Manager as "WebContentViewsManager\nrgba(30,144,255,0.5)"
participant Content as "mainWindow.contentView\nrgba(34,139,34,0.5)"
participant WC as "webContents\nrgba(255,140,0,0.5)"
Manager->>Manager: schedule delayed cleanup (setTimeout)
Manager->>Manager: lookup & delete tabViewsMap[id]
alt view exists
Manager->>Content: removeView(view) [if present]
Manager->>WC: if destroyed? -> return
Manager->>WC: close({waitForBeforeUnload: true}) (try/catch)
WC-->>Manager: destroyed event? (wait up to 1s)
alt not destroyed
Manager->>WC: forcefullyCrashRenderer()
Manager->>WC: close({waitForBeforeUnload: false}) (try/catch)
WC-->>Manager: destroyed event? (wait)
alt still alive
Manager->>Manager: logger.warn(id, webContentsId, url)
end
end
Manager->>Content: re-check/remove view
else missing -> Manager: return early
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/affine-local/e2e/blocksuite/paragraph.spec.ts (2)
51-61: Prefer Playwright's built-intoBeVisible()/toBeHidden()matchers.Playwright's
expect(locator).toBeVisible()andexpect(locator).toBeHidden()already include auto-waiting and retry logic. Usingexpect.poll()withisVisible()is redundant and loses some benefits like clearer error messages.♻️ Suggested simplification
async function expectParagraphVisibility( paragraphs: Locator, index: number, visible: boolean ) { - await expect - .poll(async () => { - return await paragraphs.nth(index).isVisible(); - }) - .toBe(visible); + if (visible) { + await expect(paragraphs.nth(index)).toBeVisible(); + } else { + await expect(paragraphs.nth(index)).toBeHidden(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/affine-local/e2e/blocksuite/paragraph.spec.ts` around lines 51 - 61, The helper expectParagraphVisibility uses expect.poll + isVisible(), which is redundant; update it to call Playwright's built-in matchers on the locator (use paragraphs.nth(index)) and invoke expect(...).toBeVisible() when visible is true or expect(...).toBeHidden() when visible is false so Playwright's auto-waiting and better error messages are used; modify the logic in expectParagraphVisibility to choose the appropriate matcher for paragraphs.nth(index).
25-49: Consider improving error diagnostics for failed assertions.When this polling assertion fails, the error message will only indicate that the result wasn't truthy, without showing which property (
typeortext) mismatched or what the actual values were. This can make debugging flaky tests harder.Also, the file has inconsistent assertion patterns - the new helpers are only used in some tests while others (e.g., lines 211-266, 300-319) still use direct
evaluate()calls.♻️ Suggested improvement for better error messages
async function expectParagraphState( paragraphs: Locator, index: number, expectedType: string, expectedText: string ) { await expect .poll(async () => { - return await paragraphs + const result = await paragraphs .nth(index) .evaluate( - ( - block: ParagraphBlockComponent, - expected: { type: string; text: string } - ) => - block.model.props.type === expected.type && - block.model.props.text.toString() === expected.text, - { - type: expectedType, - text: expectedText, - } + (block: ParagraphBlockComponent) => ({ + type: block.model.props.type, + text: block.model.props.text.toString(), + }) ); + return result; }) - .toBeTruthy(); + .toEqual({ type: expectedType, text: expectedText }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/affine-local/e2e/blocksuite/paragraph.spec.ts` around lines 25 - 49, The expectParagraphState helper currently polls for a truthy value and loses diagnostic info on failure; update expectParagraphState (the function taking paragraphs: Locator, index, expectedType, expectedText and using ParagraphBlockComponent.evaluate) to return or assert an object containing actualType and actualText along with expectedType/expectedText (e.g., { ok, actualType, actualText, expectedType, expectedText }) so failed polls surface which property mismatched and their values, and then replace remaining direct evaluate() usages in tests (the manual evaluates at the other test blocks) to call this centralized helper so all paragraph assertions get the improved diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/kit/src/electron.ts`:
- Around line 54-61: The timeout promise started alongside closeApp() runs even
if closeApp() resolves first, risking a later call to
electronApp.process().kill(); wrap the timeout so it can be cancelled (use an
AbortController or keep the timer id) and ensure you abort/clear it when
closeApp() completes: create an AbortController (or store the setTimeout id)
before Promise.race, make the timeout promise listen to controller.signal (or be
cleared) and only attempt kill() if not aborted, and call controller.abort() (or
clearTimeout) in the success path of closeApp() so the kill() callback never
runs after cleanup; update the references in this block around closeApp(), the
timeout promise, and electronApp.process().kill() accordingly.
---
Nitpick comments:
In `@tests/affine-local/e2e/blocksuite/paragraph.spec.ts`:
- Around line 51-61: The helper expectParagraphVisibility uses expect.poll +
isVisible(), which is redundant; update it to call Playwright's built-in
matchers on the locator (use paragraphs.nth(index)) and invoke
expect(...).toBeVisible() when visible is true or expect(...).toBeHidden() when
visible is false so Playwright's auto-waiting and better error messages are
used; modify the logic in expectParagraphVisibility to choose the appropriate
matcher for paragraphs.nth(index).
- Around line 25-49: The expectParagraphState helper currently polls for a
truthy value and loses diagnostic info on failure; update expectParagraphState
(the function taking paragraphs: Locator, index, expectedType, expectedText and
using ParagraphBlockComponent.evaluate) to return or assert an object containing
actualType and actualText along with expectedType/expectedText (e.g., { ok,
actualType, actualText, expectedType, expectedText }) so failed polls surface
which property mismatched and their values, and then replace remaining direct
evaluate() usages in tests (the manual evaluates at the other test blocks) to
call this centralized helper so all paragraph assertions get the improved
diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea0f2518-74c3-4032-9833-fbc8e6ea7b24
📒 Files selected for processing (3)
tests/affine-local/e2e/blocksuite/paragraph.spec.tstests/blocksuite/e2e/utils/actions/misc.tstests/kit/src/electron.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## canary #14704 +/- ##
==========================================
- Coverage 57.35% 56.84% -0.51%
==========================================
Files 2945 2945
Lines 162765 162774 +9
Branches 23973 23833 -140
==========================================
- Hits 93346 92536 -810
- Misses 66430 67055 +625
- Partials 2989 3183 +194
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/kit/src/electron.ts (1)
169-173: Wrap pid access in try-catch for consistency.On line 171,
electronApp.process().pidis accessed before the try-catch inkillProcess(). If the process state is invalid at this point, this could throw. Consider wrapping this access defensively.🛡️ Proposed fix to wrap pid access
.then(() => { - logElectronDebug('electron close timed out; forcing kill', { - pid: electronApp.process().pid, - }); + try { + logElectronDebug('electron close timed out; forcing kill', { + pid: electronApp.process().pid, + }); + } catch {} killProcess(); })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/kit/src/electron.ts` around lines 169 - 173, The call site logs electronApp.process().pid before calling killProcess(), which can throw if the process state is invalid; wrap the pid access in a try/catch (or use a safe getter) when calling logElectronDebug so that any exception is caught and you either log a placeholder/undefined pid or the caught error, then call killProcess(); update the code around logElectronDebug and the electronApp.process().pid access to use that defensive try/catch to prevent uncaught exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/kit/src/electron.ts`:
- Around line 169-173: The call site logs electronApp.process().pid before
calling killProcess(), which can throw if the process state is invalid; wrap the
pid access in a try/catch (or use a safe getter) when calling logElectronDebug
so that any exception is caught and you either log a placeholder/undefined pid
or the caught error, then call killProcess(); update the code around
logElectronDebug and the electronApp.process().pid access to use that defensive
try/catch to prevent uncaught exceptions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/frontend/apps/electron/src/main/e2e-debug.ts`:
- Around line 21-45: collectDesktopE2EDebugState can throw when BrowserWindow or
WebContents are already destroyed; wrap all property/method reads (e.g.,
window.getTitle(), window.isFocused(), window.webContents.getURL(),
contents.getType(), contents.isLoading()) in try-catch and fall back to safe
defaults so the function never throws during teardown; iterate
BrowserWindow.getAllWindows() and webContents.getAllWebContents(), catch
per-window/content errors and populate fields like title, url, type, loading,
destroyed with safe values (or the caught error message) to ensure
collectDesktopE2EDebugState always returns a debug object even if Electron
objects are destroyed.
In `@packages/frontend/apps/electron/src/main/index.ts`:
- Line 161: The call currently eagerly evaluates collectDesktopE2EDebugState()
even when logging is disabled; change each call site (the one calling
logDesktopE2EDebug with collectDesktopE2EDebugState(), and the other occurrences
mentioned) to first check the debug flag and only compute the snapshot when
enabled—for example, guard with a predicate (e.g., if
(isDesktopE2EDebugEnabled()) { logDesktopE2EDebug('window-all-closed',
collectDesktopE2EDebugState()); }) or alter logDesktopE2EDebug to accept a thunk
and invoke it only when active; update all uses of logDesktopE2EDebug +
collectDesktopE2EDebugState accordingly.
- Around line 97-103: The destroyed handler for WebContents is calling
contents.getType() and contents.getURL() which is unsafe; instead, capture and
cache the id/type/url when the web-contents-created event fires (or before
destruction) and then use those cached values inside the
contents.on('destroyed') callback (or guard with contents.isDestroyed() before
calling methods). Update the logic around contents.on('destroyed') and
logDesktopE2EDebug to read from the cached fields you store in the
web-contents-created handler (or add an isDestroyed() check) and keep
collectDesktopE2EDebugState() usage unchanged.
In `@tests/kit/src/electron.ts`:
- Around line 151-154: The waitForAppClose helper currently only attaches
electronApp.once('close', ...) which can race with the close() call; update
waitForAppClose (and the equivalent blocks around the other occurrences) to
first check process.exitCode or whether electronApp.isClosed/closed state if
available, attach both electronApp.once('close', ...) and process.once('exit',
...) as fallbacks, and wrap the wait in a Promise.race against a short timeout
so the caller (e.g., the Promise.all that uses electronApp.close().catch() and
waitForAppClose) cannot hang if the 'close' event fired before the listener was
attached or never fires; ensure you remove both listeners on resolve to avoid
leaks and reference the waitForAppClose function and electronApp.once /
process.once symbols when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d29bd28-2de1-46e2-ae05-1671fb355cce
📒 Files selected for processing (5)
packages/frontend/apps/electron/src/main/e2e-debug.tspackages/frontend/apps/electron/src/main/helper-process.tspackages/frontend/apps/electron/src/main/index.tspackages/frontend/apps/electron/src/main/worker/pool.tstests/kit/src/electron.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/kit/src/electron.ts (1)
105-128: Minor type inconsistency in return value.
describeBrowserWindowsreturns an array on success/timeout but an object with anerrorproperty on catch. This mixed return type could cause issues if callers expect a consistent shape.Consider returning a consistent shape or using a discriminated union type.
♻️ Suggested fix for consistent return type
const describeBrowserWindows = async (electronApp: ElectronApplication) => { try { return await withDebugTimeout( electronApp.evaluate(({ BrowserWindow }) => { return BrowserWindow.getAllWindows().map(window => ({ id: window.id, title: window.getTitle(), visible: window.isVisible(), focused: window.isFocused(), destroyed: window.isDestroyed(), minimized: window.isMinimized(), bounds: window.getBounds(), webContentsId: window.webContents.id, webContentsUrl: window.webContents.getURL(), })); }), [] ); } catch (error) { - return { - error: error instanceof Error ? error.message : String(error), - }; + return []; } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/kit/src/electron.ts` around lines 105 - 128, describeBrowserWindows currently returns an array on success but an { error } object on failure; change it to always return a consistent shape (e.g., an object containing a windows array and optional error field) so callers can rely on the same structure. Update describeBrowserWindows to return { windows: [...] } from the try/withDebugTimeout path and { windows: [], error: string } (or similar) from the catch, and update any callers to read the windows property (or adopt a discriminated union if preferred) so the return type is uniform.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/kit/src/electron.ts`:
- Around line 105-128: describeBrowserWindows currently returns an array on
success but an { error } object on failure; change it to always return a
consistent shape (e.g., an object containing a windows array and optional error
field) so callers can rely on the same structure. Update describeBrowserWindows
to return { windows: [...] } from the try/withDebugTimeout path and { windows:
[], error: string } (or similar) from the catch, and update any callers to read
the windows property (or adopt a discriminated union if preferred) so the return
type is uniform.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18b23e7a-95d6-4c6b-85f8-7eb653ec298c
📒 Files selected for processing (2)
tests/affine-desktop/e2e/tabs.spec.tstests/kit/src/electron.ts
darkskygit
force-pushed
the
darksky/fix-ci
branch
from
March 22, 2026 20:00
139a5e5 to
ffa3ff9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/kit/src/electron.ts (1)
103-110: Consider adding child process 'exit' event as fallback inwaitForAppClose.The current implementation only listens to
electronApp.once('close'). A past review suggested also listening tochild.once('exit')as a fallback in case the Playwright 'close' event doesn't fire. While the Linux CI path handles this by usingwaitForProcessExitalongsidecloseEventin thePromise.all, the generalcloseWithTimeoutpath (line 143) only usescloseEvent.🔧 Optional: Add exit event fallback
const waitForAppClose = () => new Promise<void>(resolve => { if (child.exitCode !== null || child.signalCode !== null) { resolve(); return; } - electronApp.once('close', () => resolve()); + const done = () => resolve(); + electronApp.once('close', done); + child.once('exit', done); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/kit/src/electron.ts` around lines 103 - 110, The waitForAppClose promise only listens to electronApp.once('close') and should also attach a child.once('exit') fallback so the promise resolves if the child process exits but Playwright's 'close' never fires; update the waitForAppClose function to register both listeners (electronApp.once('close', onClose) and child.once('exit', onExit)) and ensure each handler removes the other listener before calling resolve (to avoid double resolution). Reference the existing waitForAppClose, child, and electronApp symbols when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/frontend/apps/electron/src/main/windows-manager/tab-views.ts`:
- Around line 456-462: The logger.warn call is reading view.webContents.id and
view.webContents.getURL() which can throw after a renderer crash; wrap the
property access in a try/catch (around the retrieval of webContents id and url
or the entire logger.warn call) so exceptions are caught during cleanup, and
supply safe fallback values (e.g., 'unknown' or null) if access fails; adjust
the block around waitForDestroyed() / view.webContents so that when
view.webContents access throws it is caught and logger.warn still executes with
the safe values (reference view.webContents, waitForDestroyed(), logger.warn,
and id).
---
Nitpick comments:
In `@tests/kit/src/electron.ts`:
- Around line 103-110: The waitForAppClose promise only listens to
electronApp.once('close') and should also attach a child.once('exit') fallback
so the promise resolves if the child process exits but Playwright's 'close'
never fires; update the waitForAppClose function to register both listeners
(electronApp.once('close', onClose) and child.once('exit', onExit)) and ensure
each handler removes the other listener before calling resolve (to avoid double
resolution). Reference the existing waitForAppClose, child, and electronApp
symbols when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 946f3353-051b-4d3f-8fd7-6905d44a69e5
📒 Files selected for processing (5)
packages/frontend/apps/electron/src/main/windows-manager/tab-views.tstests/affine-desktop/e2e/tabs.spec.tstests/affine-local/e2e/blocksuite/paragraph.spec.tstests/blocksuite/e2e/utils/actions/misc.tstests/kit/src/electron.ts
✅ Files skipped from review due to trivial changes (1)
- tests/blocksuite/e2e/utils/actions/misc.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/affine-desktop/e2e/tabs.spec.ts
darkskygit
force-pushed
the
darksky/fix-ci
branch
from
March 23, 2026 03:49
5b81b3b to
f401b47
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/frontend/apps/electron/src/main/windows-manager/tab-views.ts (1)
437-443: Log which close step failed before returning.Both catch blocks swallow the exception and exit. If this teardown path flakes again, the logs will not show whether the graceful close or the post-crash close failed.
Proposed fix
try { view.webContents.close({ waitForBeforeUnload: true, }); - } catch { + } catch (error) { + logger.warn('failed to close tab webContents before force cleanup', { + id, + error, + }); return; } @@ try { view.webContents.close({ waitForBeforeUnload: false, }); - } catch { + } catch (error) { + logger.warn('failed to close tab webContents after renderer crash', { + id, + error, + }); return; }Also applies to: 449-455
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/apps/electron/src/main/windows-manager/tab-views.ts` around lines 437 - 443, The catch blocks around view.webContents.close are swallowing errors and make it impossible to know whether the graceful close or the fallback/post-crash close failed; update both catch blocks (the one around view.webContents.close with waitForBeforeUnload: true and the subsequent post-crash close) to log the thrown error before returning — include a clear contextual message (e.g., "failed graceful close" vs "failed post-crash close") and the error details using the project's logger (or console.error if no logger is available) so callers can see which close step failed and the error stack.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/frontend/apps/electron/src/main/windows-manager/tab-views.ts`:
- Around line 394-402: The code deletes an entry from tabViewsMap without
notifying observers and calls disposeTabView without handling rejections; update
the block in the timeout to (1) remove the entry from this.tabViewsMap and then
publish the change via this.webViewsMap$.next(this.tabViewsMap) so reactive
subscribers stay in sync, and (2) invoke disposeTabView(id, view) in a try/catch
(or attach .catch) to surface/log any errors (e.g., property access on a crashed
webContents inside disposeTabView) instead of fire-and-forgetting the promise;
reference the tabViewsMap, webViewsMap$, and disposeTabView symbols when making
the changes.
In `@tests/affine-local/e2e/blocksuite/paragraph.spec.ts`:
- Around line 25-49: The poll callback in expectParagraphState currently calls
paragraphs.nth(index).evaluate(...) which can throw if the paragraph node is
transiently missing; wrap the evaluate invocation inside a try/catch in the
expect.poll callback (within expectParagraphState) and return false when an
error is caught so the poll will retry instead of failing immediately;
specifically modify the body that calls paragraphs.nth(index).evaluate(...)
(referencing expectParagraphState, paragraphs.nth, and evaluate) to catch
exceptions and return a falsy value on error while preserving the original
boolean return when evaluation succeeds.
---
Nitpick comments:
In `@packages/frontend/apps/electron/src/main/windows-manager/tab-views.ts`:
- Around line 437-443: The catch blocks around view.webContents.close are
swallowing errors and make it impossible to know whether the graceful close or
the fallback/post-crash close failed; update both catch blocks (the one around
view.webContents.close with waitForBeforeUnload: true and the subsequent
post-crash close) to log the thrown error before returning — include a clear
contextual message (e.g., "failed graceful close" vs "failed post-crash close")
and the error details using the project's logger (or console.error if no logger
is available) so callers can see which close step failed and the error stack.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 314d8090-0ee4-49df-8b94-bc89e36ac945
📒 Files selected for processing (5)
packages/frontend/apps/electron/src/main/windows-manager/tab-views.tstests/affine-desktop/e2e/tabs.spec.tstests/affine-local/e2e/blocksuite/paragraph.spec.tstests/blocksuite/e2e/utils/actions/misc.tstests/kit/src/electron.ts
✅ Files skipped from review due to trivial changes (1)
- tests/kit/src/electron.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/affine-desktop/e2e/tabs.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/kit/src/electron.ts (1)
245-252:⚠️ Potential issue | 🟠 Major
closeWithTimeout()is still unbounded on the non-Linux path.
killAfterTimeoutonly sendschild.kill()in the background; it never bounds the awaited result. IfelectronApp.close()stays pending, or if the child exits without Playwright delivering the'close'event,Promise.all([electronApp.close().catch(() => {}), closeEvent])can still hang forever, so the fixture never reachesremoveWithRetry(). Playwright documentselectronApp.process()as aChildProcessand its'close'event as process termination, while Node exposes a separate child'exit'event after the process ends, so this wait should resolve on either signal and be raced against the timeout instead of awaitingelectronApp.close()itself. (playwright.dev)Also applies to: 268-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/kit/src/electron.ts` around lines 245 - 252, The non-Linux path of closeWithTimeout can hang because it awaits electronApp.close() without bounding it; update closeWithTimeout to race the app close against a timeout and process termination events by: create waitForAppClose that listens for both the ElectronApplication 'close' event (electronApp.once('close', ...)) and the underlying ChildProcess events (child.once('close'/'exit', ...)) and resolves when either occurs, then use Promise.race between Promise.all([electronApp.close().catch(() => {}), waitForAppClose()]) and the timeout promise produced by killAfterTimeout so the whole close operation is bounded; reference functions/objects: closeWithTimeout, killAfterTimeout, waitForAppClose, electronApp.close(), and electronApp.process() (child).
🧹 Nitpick comments (1)
tests/kit/src/electron.ts (1)
180-186: Abort the loser timeout inwithTimeoutFallback().When
promisewins, the 1s fallback timer still stays live until it fires. SincewaitForElectronPage()hits this helper repeatedly across windows, successful probes can still leave a burst of active timers behind and muddy the teardown-handle dump. Node's promisifiedsetTimeoutsupports bothsignalcancellation andref: false, so you can cancel the fallback as soon as the main promise settles. (nodejs.org)Suggested refactor
const withTimeoutFallback = async <T>(promise: Promise<T>, fallback: T) => { + const controller = new AbortController(); try { - return await Promise.race([promise, setTimeout(1_000).then(() => fallback)]); + return await Promise.race([ + promise.finally(() => controller.abort()), + setTimeout(1_000, fallback, { + signal: controller.signal, + ref: false, + }), + ]); } catch { return fallback; } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/kit/src/electron.ts` around lines 180 - 186, withTimeoutFallback leaves the 1s fallback timer alive when the main promise wins; fix by creating an AbortController, pass its signal and ref:false to the promisified setTimeout call used as the fallback, run Promise.race([promise, timeoutPromise]) and in a finally block call controller.abort() to cancel the loser timer so it doesn't remain active; update the setTimeout invocation accordingly and keep this logic inside withTimeoutFallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/kit/src/electron.ts`:
- Around line 1-14: The import block is unsorted and triggers
simple-import-sort/imports; reorder imports by grouping and alphabetizing: group
built-in node imports together (e.g., ChildProcess, crypto, spawnSync,
setTimeout), then external packages alphabetically (e.g., fs-extra as fs,
playwright and its _electron alias, tree-kill, `@playwright/test` types), then
internal project imports (e.g., Package from `@affine-tools/utils/workspace`, test
as base from './playwright', removeWithRetry from './utils/utils'); keep
type-only imports (import type ...) grouped with other type imports and preserve
aliasing like "_electron as electron" and named imports like "test as base".
---
Duplicate comments:
In `@tests/kit/src/electron.ts`:
- Around line 245-252: The non-Linux path of closeWithTimeout can hang because
it awaits electronApp.close() without bounding it; update closeWithTimeout to
race the app close against a timeout and process termination events by: create
waitForAppClose that listens for both the ElectronApplication 'close' event
(electronApp.once('close', ...)) and the underlying ChildProcess events
(child.once('close'/'exit', ...)) and resolves when either occurs, then use
Promise.race between Promise.all([electronApp.close().catch(() => {}),
waitForAppClose()]) and the timeout promise produced by killAfterTimeout so the
whole close operation is bounded; reference functions/objects: closeWithTimeout,
killAfterTimeout, waitForAppClose, electronApp.close(), and
electronApp.process() (child).
---
Nitpick comments:
In `@tests/kit/src/electron.ts`:
- Around line 180-186: withTimeoutFallback leaves the 1s fallback timer alive
when the main promise wins; fix by creating an AbortController, pass its signal
and ref:false to the promisified setTimeout call used as the fallback, run
Promise.race([promise, timeoutPromise]) and in a finally block call
controller.abort() to cancel the loser timer so it doesn't remain active; update
the setTimeout invocation accordingly and keep this logic inside
withTimeoutFallback.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/kit/src/electron.ts (1)
244-288:⚠️ Potential issue | 🟠 MajorNon-Linux teardown can still hang indefinitely.
closeWithTimeout()still waits oncloseEventonly. If Playwright misses'close', the await at Line 284 never settles even after the 10s timer callschild.kill(), so this fixture can still wedge outside the Linux-specific branch. The Linux path already guards against this withprocessExit; the non-Linux path should do the same.🔧 Suggested fix
const closeWithTimeout = async () => { const closeEvent = waitForAppClose(); + const processExit = waitForProcessExit(); + const closeOrExit = Promise.race([closeEvent, processExit]); const controller = new AbortController(); const killAfterTimeout = setTimeout(10_000, undefined, { signal: controller.signal, }) - .then(() => { + .then(async () => { killProcess(); + await Promise.race([closeOrExit, setTimeout(5_000)]); }) .catch(error => { if (error instanceof Error && error.name === 'AbortError') { return; } throw error; }); try { - await Promise.all([electronApp.close().catch(() => {}), closeEvent]); + void electronApp.close().catch(() => {}); + await Promise.race([closeOrExit, killAfterTimeout]); } finally { controller.abort(); await killAfterTimeout; } };This script confirms the current non-Linux branch still depends on
closeEventalone.#!/bin/bash python - <<'PY' from pathlib import Path text = Path("tests/kit/src/electron.ts").read_text() close_start = text.index("const closeWithTimeout = async () => {") close_end = text.index("if (process.env.CI && process.platform === 'linux') {") close_block = text[close_start:close_end] print(close_block) assert "const processExit = waitForProcessExit();" not in close_block assert "Promise.all([electronApp.close().catch(() => {}), closeEvent])" in close_block print("\nVerified: the non-Linux close path still waits on closeEvent only.") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/kit/src/electron.ts` around lines 244 - 288, The non-Linux closeWithTimeout path only awaits closeEvent and can hang if Playwright misses 'close'; update closeWithTimeout to also create const processExit = waitForProcessExit() (same as the Linux branch) and include it in the awaited Promise.all so you await Promise.all([electronApp.close().catch(() => {}), closeEvent, processExit]); ensure the AbortController/killer logic remains intact and child.kill() is still invoked by the timeout so the fixture is protected by processExit as well as closeEvent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/kit/src/electron.ts`:
- Around line 244-288: The non-Linux closeWithTimeout path only awaits
closeEvent and can hang if Playwright misses 'close'; update closeWithTimeout to
also create const processExit = waitForProcessExit() (same as the Linux branch)
and include it in the awaited Promise.all so you await
Promise.all([electronApp.close().catch(() => {}), closeEvent, processExit]);
ensure the AbortController/killer logic remains intact and child.kill() is still
invoked by the timeout so the fixture is protected by processExit as well as
closeEvent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c2bedda-50af-4517-a744-70f2f553db78
📒 Files selected for processing (2)
packages/frontend/apps/electron/src/main/windows-manager/tab-views.tstests/kit/src/electron.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/frontend/apps/electron/src/main/windows-manager/tab-views.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/kit/src/electron.ts (1)
267-288:⚠️ Potential issue | 🟠 MajorPrevent non-Linux teardown from hanging on
'close'event only.In
closeWithTimeout, Line 284 waits forcloseEventonly. If'close'is missed/not emitted,killProcess()at timeout does not guarantee this await completes, so fixture teardown can hang. ReusewaitForProcessExit()as a fallback in this path too.Suggested fix
const closeWithTimeout = async () => { const closeEvent = waitForAppClose(); + const processExit = waitForProcessExit(); const controller = new AbortController(); const killAfterTimeout = setTimeout(10_000, undefined, { signal: controller.signal, }) .then(() => { killProcess(); }) .catch(error => { if (error instanceof Error && error.name === 'AbortError') { return; } throw error; }); try { - await Promise.all([electronApp.close().catch(() => {}), closeEvent]); + void electronApp.close().catch(() => {}); + await Promise.race([ + Promise.all([closeEvent, processExit]), + killAfterTimeout.then(() => + Promise.race([processExit, setTimeout(2_000)]) + ), + ]); } finally { controller.abort(); await killAfterTimeout; } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/kit/src/electron.ts` around lines 267 - 288, The teardown can hang if waitForAppClose() never emits; in closeWithTimeout update the awaited tasks so the close fallback also waits for a process exit: when calling Promise.all currently awaiting [electronApp.close().catch(() => {}), closeEvent], add waitForProcessExit() (or use Promise.race with closeEvent and waitForProcessExit) so that if the 'close' event is missed the fixture still proceeds to killProcess(); keep the AbortController/killAfterTimeout logic and still call killProcess() on timeout and controller.abort() in finally. Ensure references to closeWithTimeout, waitForAppClose, waitForProcessExit, killProcess, and electronApp.close are used to locate and modify the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/kit/src/electron.ts`:
- Around line 1-13: Reorder the import block to satisfy simple-import-sort:
group and alphabetize builtins, then externals, then internal/project imports,
with a blank line between each group; ensure type-only imports (type Page, type
ChildProcess, type ElectronApplication) remain adjacent to their corresponding
module imports and keep the aliased import "{ _electron as electron }" with
other Playwright imports; specifically reorder symbols like spawnSync,
setTimeout, crypto, fs, treeKill (builtins/external) first, then playwright and
`@playwright/test`, then `@affine-tools/utils/workspace` and local imports
"./playwright" and "./utils/utils", all sorted alphabetically within groups to
satisfy simple-import-sort/imports.
---
Duplicate comments:
In `@tests/kit/src/electron.ts`:
- Around line 267-288: The teardown can hang if waitForAppClose() never emits;
in closeWithTimeout update the awaited tasks so the close fallback also waits
for a process exit: when calling Promise.all currently awaiting
[electronApp.close().catch(() => {}), closeEvent], add waitForProcessExit() (or
use Promise.race with closeEvent and waitForProcessExit) so that if the 'close'
event is missed the fixture still proceeds to killProcess(); keep the
AbortController/killAfterTimeout logic and still call killProcess() on timeout
and controller.abort() in finally. Ensure references to closeWithTimeout,
waitForAppClose, waitForProcessExit, killProcess, and electronApp.close are used
to locate and modify the code.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/kit/src/electron.ts (1)
62-68: Process tree capture may only show the parent process.The
ps --forest -p <pid>command restricts output to the specified PID only, which won't include child processes. For full tree visibility, considerpstree -p <pid>or querying by process group. However, since this is diagnostic logging for CI flakiness investigation, the current approach is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/kit/src/electron.ts` around lines 62 - 68, The current use of spawnSync in tests/kit/src/electron.ts calling ps with ['-o', 'pid=,ppid=,pgid=,stat=,command=', '--forest', '-p', String(pid)] only restricts output to the single PID (treeResult) and can omit child processes; change the diagnostic capture to run pstree -p <pid> (or change to query the process group via ps -g $(ps -o pgid= -p <pid>) --forest) so children appear; update the spawnSync invocation referenced by spawnSync and treeResult to use the chosen command and arguments so the full process tree is logged for CI debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/kit/src/electron.ts`:
- Around line 62-68: The current use of spawnSync in tests/kit/src/electron.ts
calling ps with ['-o', 'pid=,ppid=,pgid=,stat=,command=', '--forest', '-p',
String(pid)] only restricts output to the single PID (treeResult) and can omit
child processes; change the diagnostic capture to run pstree -p <pid> (or change
to query the process group via ps -g $(ps -o pgid= -p <pid>) --forest) so
children appear; update the spawnSync invocation referenced by spawnSync and
treeResult to use the chosen command and arguments so the full process tree is
logged for CI debugging.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
blocksuite/integration-test/src/__tests__/edgeless/turbo-renderer.spec.ts (1)
24-37:⚠️ Potential issue | 🟡 MinorDouble cleanup: both
return cleanupandafterEachwill invoke cleanup.The
beforeEachreturnscleanupat line 34 (vitest uses this as teardown), andafterEachat line 37 also callscleanup. This causes cleanup to run twice per test.Remove
return cleanup;frombeforeEachsinceafterEachnow handles it:🔧 Proposed fix
beforeEach(async () => { cleanup = await setupEditor('edgeless', [ ParagraphLayoutHandlerExtension, TurboRendererConfigFactory({ painterWorkerEntry: createPainterWorker, }), ViewportTurboRendererExtension, ]); - return cleanup; }); afterEach(async () => await cleanup?.());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blocksuite/integration-test/src/__tests__/edgeless/turbo-renderer.spec.ts` around lines 24 - 37, The beforeEach currently returns the cleanup function (cleanup) which Vitest will treat as a teardown, and afterEach also calls cleanup, causing double teardown; update the beforeEach that calls setupEditor (which assigns cleanup) to not return cleanup — simply await setupEditor and assign cleanup without returning it, and keep the afterEach(async () => await cleanup?.()) as the single teardown; locate the setupEditor call and the beforeEach/afterEach blocks to make this change.
🧹 Nitpick comments (2)
blocksuite/integration-test/src/__tests__/utils/setup.ts (1)
170-170: Remove unusedwindow.storedeletion.The
delete (window as any).store;statement at line 170 serves no purpose.window.storeis never assigned anywhere in this file, is not declared in the Window interface (lines 178-184), and has no references in the codebase. This is legacy cleanup code that can be safely removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blocksuite/integration-test/src/__tests__/utils/setup.ts` at line 170, Remove the unused legacy deletion statement "delete (window as any).store;" from the setup file: locate the standalone statement and delete it, and ensure no other references to window.store or related casts remain (you may also remove any redundant casts or comments introduced for this unused value); this will clean up the orphaned legacy code without changing any other setup logic.tests/blocksuite/e2e/list.spec.ts (1)
191-195: Consider removing fixed 200ms waits before assertion calls, since assertion helpers already poll.Lines 191, 195, 209, 268, 271, 284, 612, and 619 use
waitNextFrame(page, 200)before assertions likeassertBlockChildrenIds()andassertRichTexts(). ThewaitNextFrame()function is a pure fixed-timeout wrapper (callspage.waitForTimeout()), but both assertion helpers already implement polling viaexpect.poll(). The explicit fixed waits before polling starts add unnecessary latency on fast runners without eliminating brittleness on slow CI—the polling mechanism is what provides resilience. Remove the explicit waits and rely on the assertions' built-in retry logic to handle timing variations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/blocksuite/e2e/list.spec.ts` around lines 191 - 195, Remove the fixed 200ms explicit waits that precede polling assertions: eliminate calls to waitNextFrame(page, 200) before assertions such as assertBlockChildrenIds(...) and assertRichTexts(...). The assertion helpers already use expect.poll() for retries, so delete the waitNextFrame calls (and any similar page.waitForTimeout wrappers) around interactions like pressShiftTab(page) and subsequent asserts to reduce test latency and rely on the assertions' built-in polling for timing resilience.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/blocksuite/e2e/paragraph.spec.ts`:
- Around line 91-93: The test uses setInlineRangeInInlineEditor(page, { index:
2, length: 0 }) which targets the nth [data-v-root="true"] globally and can move
the caret in the wrong inline root; change the call to target the page title
explicitly by selecting the title inline root (e.g. use a title-specific helper
or a selector for the doc-title inline root) so the caret is set inside the
title only—replace the global setInlineRangeInInlineEditor invocation with a
title-specific helper or add an overload that accepts a root selector and call
it for the doc-title before pressEnter and waitNextFrame.
In `@tests/blocksuite/e2e/utils/actions/misc.ts`:
- Around line 549-554: The visibility precheck on the inner rich-text editor
breaks readonly callers because readonly blocks may render RICH_TEXT_SELECTOR
without an editable .inline-editor; update the click logic around
getEditorHostLocator(page) and the locator (nth(i)) so that before calling
locator.click({ force: true, ... }) you check for the presence/visibility of the
inner .inline-editor and, if absent, fall back to click the rich-text host (or
the affine-selected-blocks host) instead; ensure you still pass force: true and
preserve options?.clickPosition when clicking the host so readonly blocks
receive the same click behavior.
In `@tests/blocksuite/e2e/utils/asserts.ts`:
- Around line 418-430: The polling currently reads children from the rendered
DOM element via page.evaluate(({ blockId }) =>
document.querySelector(...).model.children) which can be stale; change the
evaluator to read the source-of-truth by returning
window.doc.getBlock(blockId)?.model.children instead (keep using page.evaluate
and the same blockId param). Make the same replacement for the other polling
assertion that also queries element.model.children so both assertions read
children from window.doc.getBlock(...).model.children.
- Around line 365-378: The current page.evaluate uses
richText?.closest(`[${BLOCK_ID_ATTR}]`) which returns the nested paragraph block
for list items (type "text"); change the evaluation to resolve the owning
list/paragraph block before reading props.type: after finding the initial block
element, walk up ancestor elements with `[${BLOCK_ID_ATTR}]` until you find a
BlockComponent whose (block.model as BlockModel).props.type is not "text" (or
until no higher ancestor exists), then return that block's props.type; update
the expect.poll callback accordingly so it stabilizes on the outer block type
(e.g., "bulleted") instead of the inner paragraph.
- Around line 144-157: The test is selecting the raw inline root with
querySelectorAll('rich-text [data-v-root="true"]')[i], which can return
stale/empty editors; instead, locate the rich-text element at index i and then
read its inline root's inlineEditor.yTextDeltas. Update the polling callback
that currently references editorHost, InlineRootElement and inlineRoot to first
select editorHost?.querySelectorAll('rich-text')[i], then find the child
'[data-v-root="true"]' inside that rich-text and return its
inlineEditor?.yTextDeltas (preserving the same variable names like
inlineEditor/yTextDeltas) so the assertion reads the correct, fresh editor
instance.
---
Outside diff comments:
In `@blocksuite/integration-test/src/__tests__/edgeless/turbo-renderer.spec.ts`:
- Around line 24-37: The beforeEach currently returns the cleanup function
(cleanup) which Vitest will treat as a teardown, and afterEach also calls
cleanup, causing double teardown; update the beforeEach that calls setupEditor
(which assigns cleanup) to not return cleanup — simply await setupEditor and
assign cleanup without returning it, and keep the afterEach(async () => await
cleanup?.()) as the single teardown; locate the setupEditor call and the
beforeEach/afterEach blocks to make this change.
---
Nitpick comments:
In `@blocksuite/integration-test/src/__tests__/utils/setup.ts`:
- Line 170: Remove the unused legacy deletion statement "delete (window as
any).store;" from the setup file: locate the standalone statement and delete it,
and ensure no other references to window.store or related casts remain (you may
also remove any redundant casts or comments introduced for this unused value);
this will clean up the orphaned legacy code without changing any other setup
logic.
In `@tests/blocksuite/e2e/list.spec.ts`:
- Around line 191-195: Remove the fixed 200ms explicit waits that precede
polling assertions: eliminate calls to waitNextFrame(page, 200) before
assertions such as assertBlockChildrenIds(...) and assertRichTexts(...). The
assertion helpers already use expect.poll() for retries, so delete the
waitNextFrame calls (and any similar page.waitForTimeout wrappers) around
interactions like pressShiftTab(page) and subsequent asserts to reduce test
latency and rely on the assertions' built-in polling for timing resilience.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 73e1b789-c5d8-4512-b357-b9a33ffdc565
📒 Files selected for processing (13)
blocksuite/integration-test/src/__tests__/edgeless/layer.spec.tsblocksuite/integration-test/src/__tests__/edgeless/turbo-renderer.spec.tsblocksuite/integration-test/src/__tests__/utils/setup.tstests/blocksuite/e2e/format-bar.spec.tstests/blocksuite/e2e/hotkey/hotkey.spec.tstests/blocksuite/e2e/hotkey/multiline.spec.tstests/blocksuite/e2e/list.spec.tstests/blocksuite/e2e/paragraph.spec.tstests/blocksuite/e2e/selection/block.spec.tstests/blocksuite/e2e/selection/native.spec.tstests/blocksuite/e2e/utils/actions/misc.tstests/blocksuite/e2e/utils/asserts.tstests/kit/src/electron.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tests/blocksuite/e2e/utils/asserts.ts (3)
418-430:⚠️ Potential issue | 🟠 MajorRead children from
window.doc, not the rendered element.Lines 422-445 still poll
element.model.childrenoff the DOM component. During reparenting that lags behind the document model, so these helpers keep timing out on stale child ids/flavours.🛠️ Possible fix
export async function assertBlockChildrenIds( page: Page, blockId: string, ids: string[] ) { await expect .poll(async () => { return page.evaluate( ({ blockId }) => { - const element = document.querySelector<BlockComponent>( - `[data-block-id="${blockId}"]` - ); - return element?.model.children.map(child => child.id); + return window.doc + ?.getBlock(blockId) + ?.model.children.map(child => child.id); }, { blockId } ); }) .toEqual(ids); } @@ export async function assertBlockChildrenFlavours( page: Page, blockId: string, flavours: string[] ) { await expect .poll(async () => { return page.evaluate( ({ blockId }) => { - const element = document.querySelector<BlockComponent>( - `[data-block-id="${blockId}"]` - ); - return element?.model.children.map(child => child.flavour); + return window.doc + ?.getBlock(blockId) + ?.model.children.map(child => child.flavour); }, { blockId } ); }) .toEqual(flavours); }Also applies to: 438-450
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/blocksuite/e2e/utils/asserts.ts` around lines 418 - 430, The polling currently reads children from the rendered DOM component via element.model.children (inside the page.evaluate that receives { blockId }), which can lag during reparenting; instead read from the authoritative document model on window (e.g., window.doc.getBlock(blockId).children) within the same page.evaluate. Update both occurrences that query element?.model.children and replace them to access window.doc (or its API) to fetch the block by blockId and map its children to ids so the poll observes the canonical model rather than the rendered element.
365-378:⚠️ Potential issue | 🟠 MajorResolve the owning list block before reading
props.type.Line 371 takes the nearest
[data-block-id], which is the nested paragraph for list items. That makes this helper stabilize on"text"instead of"bulleted"during the list/heading undo path.🛠️ Possible fix
export async function assertRichTextModelType( page: Page, type: string, index = 0 ) { await expect .poll(async () => { return page.evaluate( ({ index, BLOCK_ID_ATTR }) => { const editorHost = document.querySelector('editor-host'); const richText = editorHost?.querySelectorAll('rich-text')[index]; - const block = richText?.closest<BlockComponent>(`[${BLOCK_ID_ATTR}]`); - return (block?.model as BlockModel<{ type: string }> | undefined) - ?.props.type; + const getType = (block?: BlockComponent | null) => + (block?.model as BlockModel<{ type: string }> | undefined)?.props + .type; + let block = richText?.closest<BlockComponent>(`[${BLOCK_ID_ATTR}]`); + while (block) { + const parent = block.parentElement?.closest<BlockComponent>( + `[${BLOCK_ID_ATTR}]` + ); + if (!parent || !getType(parent) || getType(block) !== 'text') { + break; + } + block = parent; + } + return getType(block); }, { index, BLOCK_ID_ATTR } ); }) .toEqual(type); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/blocksuite/e2e/utils/asserts.ts` around lines 365 - 378, The helper currently reads the nearest block's props.type (using page.evaluate with BLOCK_ID_ATTR and querying editor-host -> rich-text -> closest<BlockComponent>(`[${BLOCK_ID_ATTR}]`)), which returns the nested paragraph ("text") for list items; update the evaluation to resolve the owning/list parent block before returning props.type: after locating block (BlockComponent) check its (block.model as BlockModel)?.props.type and if it equals "text" (or is a paragraph), walk up to the next ancestor matching `[${BLOCK_ID_ATTR}]` whose (model as BlockModel)?.props.type is not "text" (or matches list/heading types like "bulleted") and return that ancestor's props.type instead; ensure you still return undefined when no suitable ancestor exists.
144-157:⚠️ Potential issue | 🟠 MajorIndex the
rich-text, not the raw inline root.Line 149 still walks
rich-text [data-v-root="true"]directly. After markdown transforms that can resolve to a stale/empty editor and keep returning[], which matches the current inline-text failures.🛠️ Possible fix
export async function assertRichTextInlineDeltas( page: Page, deltas: unknown[], i = 0 ) { await expect .poll(async () => { return page.evaluate( ([i]) => { const editorHost = document.querySelector('editor-host'); - const inlineRoot = editorHost?.querySelectorAll<InlineRootElement>( - 'rich-text [data-v-root="true"]' - )[i]; - return inlineRoot?.inlineEditor.yTextDeltas; + const richText = editorHost?.querySelectorAll<RichText>('rich-text')[i]; + return richText?.inlineEditor?.yTextDeltas; }, [i] ); }) .toEqual(deltas); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/blocksuite/e2e/utils/asserts.ts` around lines 144 - 157, The test is indexing the raw inline root directly (querySelectorAll('rich-text [data-v-root="true"]')) which can return stale/empty editors; update the page.evaluate logic used in the expect.poll call to first select the list of 'rich-text' elements, pick the i-th rich-text element, then query that element for its [data-v-root="true"] child and read inlineEditor.yTextDeltas from that child (replace the existing editorHost/querySelectorAll usage that assigns inlineRoot). This targets the rich-text container instead of the raw inline root to avoid stale matches.tests/blocksuite/e2e/paragraph.spec.ts (1)
91-93:⚠️ Potential issue | 🟠 MajorUse a title-specific range setter here.
Line 91 still goes through the generic inline-root helper, so this test can place the caret in the wrong editor as soon as another inline root appears before
doc-title.🛠️ Possible fix
- await setInlineRangeInInlineEditor(page, { index: 2, length: 0 }); + await page.evaluate(() => { + const richText = document.querySelector('doc-title rich-text'); + const inlineEditor = richText?.inlineEditor; + if (!inlineEditor) { + throw new Error('Doc title inline editor not found'); + } + inlineEditor.setInlineRange({ index: 2, length: 0 }); + }); + await waitNextFrame(page);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/blocksuite/e2e/paragraph.spec.ts` around lines 91 - 93, The test currently uses the generic helper setInlineRangeInInlineEditor which can target the wrong inline root; replace that call with the title-specific helper (e.g. setInlineRangeInTitleEditor or setInlineRangeInDocTitle) to position the caret explicitly in the doc-title inline root (use the same args: { index: 2, length: 0 }), then keep the following pressEnter(page) and waitNextFrame calls; ensure no other generic inline-root helper is used around this assertion so the caret is guaranteed to be in the title editor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@blocksuite/integration-test/src/__tests__/utils/setup.ts`:
- Around line 146-149: The teardown returned async function must be
exception-safe: ensure DOM cleanup (appElement?.remove()) always runs and that
worker termination/global deletion in cleanup() are not skipped if one await
rejects. Wrap the await cleanup() in try/catch/finally (or run cleanup() and
appElement removal inside a try/finally) so appElement?.remove() is called in
the finally block; catch and log any errors from cleanup() and rethrow or
aggregate them after removal so failures don't leave stale state. Target the
returned async teardown function and the calls to cleanup() and
appElement?.remove().
---
Duplicate comments:
In `@tests/blocksuite/e2e/paragraph.spec.ts`:
- Around line 91-93: The test currently uses the generic helper
setInlineRangeInInlineEditor which can target the wrong inline root; replace
that call with the title-specific helper (e.g. setInlineRangeInTitleEditor or
setInlineRangeInDocTitle) to position the caret explicitly in the doc-title
inline root (use the same args: { index: 2, length: 0 }), then keep the
following pressEnter(page) and waitNextFrame calls; ensure no other generic
inline-root helper is used around this assertion so the caret is guaranteed to
be in the title editor.
In `@tests/blocksuite/e2e/utils/asserts.ts`:
- Around line 418-430: The polling currently reads children from the rendered
DOM component via element.model.children (inside the page.evaluate that receives
{ blockId }), which can lag during reparenting; instead read from the
authoritative document model on window (e.g.,
window.doc.getBlock(blockId).children) within the same page.evaluate. Update
both occurrences that query element?.model.children and replace them to access
window.doc (or its API) to fetch the block by blockId and map its children to
ids so the poll observes the canonical model rather than the rendered element.
- Around line 365-378: The helper currently reads the nearest block's props.type
(using page.evaluate with BLOCK_ID_ATTR and querying editor-host -> rich-text ->
closest<BlockComponent>(`[${BLOCK_ID_ATTR}]`)), which returns the nested
paragraph ("text") for list items; update the evaluation to resolve the
owning/list parent block before returning props.type: after locating block
(BlockComponent) check its (block.model as BlockModel)?.props.type and if it
equals "text" (or is a paragraph), walk up to the next ancestor matching
`[${BLOCK_ID_ATTR}]` whose (model as BlockModel)?.props.type is not "text" (or
matches list/heading types like "bulleted") and return that ancestor's
props.type instead; ensure you still return undefined when no suitable ancestor
exists.
- Around line 144-157: The test is indexing the raw inline root directly
(querySelectorAll('rich-text [data-v-root="true"]')) which can return
stale/empty editors; update the page.evaluate logic used in the expect.poll call
to first select the list of 'rich-text' elements, pick the i-th rich-text
element, then query that element for its [data-v-root="true"] child and read
inlineEditor.yTextDeltas from that child (replace the existing
editorHost/querySelectorAll usage that assigns inlineRoot). This targets the
rich-text container instead of the raw inline root to avoid stale matches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc0d8e30-c7ef-4b10-8e65-b8bd0da46a24
📒 Files selected for processing (13)
blocksuite/integration-test/src/__tests__/edgeless/layer.spec.tsblocksuite/integration-test/src/__tests__/edgeless/turbo-renderer.spec.tsblocksuite/integration-test/src/__tests__/utils/setup.tstests/blocksuite/e2e/format-bar.spec.tstests/blocksuite/e2e/hotkey/hotkey.spec.tstests/blocksuite/e2e/hotkey/multiline.spec.tstests/blocksuite/e2e/list.spec.tstests/blocksuite/e2e/paragraph.spec.tstests/blocksuite/e2e/selection/block.spec.tstests/blocksuite/e2e/selection/native.spec.tstests/blocksuite/e2e/utils/actions/misc.tstests/blocksuite/e2e/utils/asserts.tstests/kit/src/electron.ts
✅ Files skipped from review due to trivial changes (1)
- tests/blocksuite/e2e/list.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/blocksuite/e2e/utils/actions/misc.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/blocksuite/e2e/utils/actions/edgeless.ts`:
- Around line 379-385: The boundingBox() call on shapeToolButton should be
preceded by an explicit visibility check to avoid flakiness during toolbar
animations; update the code around shapeToolButton to await
expect(shapeToolButton).toBeVisible() before calling
shapeToolButton.boundingBox(), mirroring the pattern used for squareShapeButton
and ensuring the click uses geometry only after the element is visible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5505c4a4-e0b7-4b3c-a24a-6201a8a0ec2c
📒 Files selected for processing (3)
tests/blocksuite/e2e/format-bar.spec.tstests/blocksuite/e2e/list.spec.tstests/blocksuite/e2e/utils/actions/edgeless.ts
✅ Files skipped from review due to trivial changes (1)
- tests/blocksuite/e2e/format-bar.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/blocksuite/e2e/list.spec.ts
darkskygit
force-pushed
the
darksky/fix-ci
branch
from
March 23, 2026 08:24
ab17304 to
fb69d84
Compare
Wool5443
pushed a commit
to Wool5443/AFFiNE
that referenced
this pull request
Apr 15, 2026
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Improved Electron shutdown, diagnostics and tab teardown for more reliable exits and forced cleanup on stubborn processes. * **Tests** * Added polling-based test helpers, stronger scroll/page readiness, timeout-tolerant page selection, and async cleanup/worker teardown; updated many tests to wait for UI/model updates and rendering frames. * **Bug Fixes** * Reduced flakiness by awaiting paragraph visibility, nested counts, selection/navigation stability, tab counts, post-action renders, and safer element interactions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
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.
Summary by CodeRabbit