Fix flaky ChecklistItem test by properly awaiting act()#9616
Conversation
|
@pavelzeman: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsI understand the commands that are listed here |
pavelzeman
added
release-note-none
and removed
do-not-merge/release-note-label-needed
labels
|
@pavelzeman: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsI understand the commands that are listed here |
mm-cloud-bot
added
do-not-merge/release-note-label-needed
and removed
release-note-none
labels
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA test file for the ChecklistItem component was updated to wrap asynchronous checkbox interactions with Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
patches/react-native-navigation+7.45.0.patch (4)
263-272:⚠️ Potential issue | 🟠 MajorGuard the
NavigationActivitycast ininvalidate().The
activity()method now returns a plainActivity, butinvalidate()does an unconditional(NavigationActivity) activity()cast. While the patch handles this risk in other methods—usinginstanceofchecks (innavigator()) or try-catch blocks (inhandle()andonHostResume())—invalidate()remains unprotected. If bridge teardown runs while a non-NavigationActivity is current, this will throw aClassCastException.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/react-native-navigation`+7.45.0.patch around lines 263 - 272, The invalidate() method unconditionally casts activity() to NavigationActivity which can cause a ClassCastException; before casting, check that activity() is an instance of NavigationActivity (or null) and only call navigationActivity.onCatalystInstanceDestroy() when that check passes, otherwise skip that call; keep the call to super.invalidate() regardless. Reference: invalidate(), activity(), NavigationActivity, onCatalystInstanceDestroy(), super.invalidate().
126-228:⚠️ Potential issue | 🔴 CriticalNavigation commands will hang when no active NavigationActivity exists.
Wrapping navigator calls with
ifPresent()causes them to silently no-op when the current activity is not aNavigationActivity. For promise-based commands,NativeCommandListenernever runs, so the JavaScript promises never settle. This breaks the error-handling logic already in place atapp/screens/navigation.ts:579–590,app/screens/navigation.ts:773–790, andapp/products/calls/state/actions.ts:470–475, where the application explicitly handles promise rejections.Also applies to: lines 233–238 (navigator returns Optional.empty), lines 243–253 (handle catches ClassCastException silently).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/react-native-navigation`+7.45.0.patch around lines 126 - 228, The navigator Optional wrapping causes commands to silently no-op and leaves NativeCommandListener (and JS promises) unresolved; instead of replacing calls like navigator().setRoot(...)/push(...)/mergeOptions(...) with navigator().ifPresent(...), detect when navigator() is empty and explicitly invoke the corresponding NativeCommandListener error path so the Promise is settled: for each call site that now uses navigator().ifPresent(nav -> nav.<action>(... , new NativeCommandListener(...))), keep creating the same NativeCommandListener instance and, if navigator().isPresent() call nav.<action>(...), else call the listener's failure/reject callback (or otherwise invoke its error handling method) so the JS promise is rejected; apply this pattern for setRoot, setDefaultOptions, mergeOptions, push, setStackRoot, pop, popTo, popToRoot, showModal, dismissModal (ensure mergeOptions still runs against nav when present), dismissAllModals, showOverlay, dismissOverlay and dismissAllOverlays.
355-378:⚠️ Potential issue | 🟠 MajorInconsistent null-safety: gesture callbacks should use safe handling like other methods in the same patch.
Lines 357, 364, and 371 still force-unwrap
getEventDispatcher()!!, creating a crash risk. The patch makes the dispatcher nullable and correctly guards it with?.letinonInterceptTouchEvent()andonTouchEvent()(lines 387–389, 396–398), but the gesture callbacks don't apply the same pattern. Additionally,getEventDispatcher()itself (lines 376–378) can return null but still crashes ifUIManagerModuleis absent due to the!!force-unwrap on the module.Update the gesture callbacks to match the safe handling used elsewhere:
androidEvent?.let { getEventDispatcher()?.let { dispatcher -> mJSTouchDispatcher.onChildStartedNativeGesture(it, dispatcher) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/react-native-navigation`+7.45.0.patch around lines 355 - 378, The gesture callbacks onChildStartedNativeGesture and onChildEndedNativeGesture currently force-unwrap getEventDispatcher() (!!) and risk NPEs; change them to mirror the safe pattern used elsewhere by null-checking androidEvent and then safely obtaining the dispatcher (call getEventDispatcher() and .let on it) before calling mJSTouchDispatcher.onChildStartedNativeGesture/onChildEndedNativeGesture, and also remove the forced unwrap when retrieving the UIManagerModule inside getEventDispatcher so it can return null safely (ensure UIManagerModule lookup is null-checked before accessing eventDispatcher).
315-321:⚠️ Potential issue | 🟠 MajorUse
ime.bottominstead of hardcoded0for bottom padding.When
SOFT_INPUT_ADJUST_NOTHINGis set, the system will not pan the layout for the keyboard. The code here manually applies IME insets as padding to accommodate it, but sets bottom padding to0instead ofime.bottom. The IME inset's bottom value represents keyboard height (zero when hidden, equals keyboard height when visible). Without this value, non-root views will still be covered by the keyboard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/react-native-navigation`+7.45.0.patch around lines 315 - 321, The bottom padding is incorrectly hardcoded to 0 when applying IME insets; in the block that gets Activity via getActivity(), computes Insets ime = insets.getInsets(WindowInsetsCompat.Type.ime()), and checks softInputMode == WindowManager.LayoutParams.SOFT_INPUT_ADJUST_NOTHING && !isRoot(), change the view.setPadding call to use ime.bottom for the bottom padding instead of 0 so the keyboard height is respected, then return WindowInsetsCompat.CONSUMED as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ios/Gekidou/Sources/Gekidou/Storage/Database.swift`:
- Around line 178-180: The code currently materializes rows with
Array(iterator).map which uses SQLite.swift's non-throwing next() and can hide
SQLITE_BUSY; replace each Array(iterator).map usage (e.g., where you call let
iterator = try db.prepareRowIterator(query) and then let servers: [T] = try
Array(iterator).map { row in return try row.decode() }) with an explicit
failableNext() loop: repeatedly call try iterator.failableNext() in a while-let
loop, decode each yielded row via row.decode() and append to a local results
array, then return that array; apply the same replacement in Database.swift
(both occurrences around db.prepareRowIterator), Database+Users.swift (the line
using Array(iterator)), and the test file DatabaseSafeIterationTests.swift to
ensure every Array(iterator) materialization is converted to the failableNext()
loop.
In `@ios/Gekidou/Tests/GekidouTests/DatabaseSafeIterationTests.swift`:
- Around line 136-154: The test must not call sqlite3_close_v2(conn.handle)
because that violates Connection ownership; instead simulate a SQLite error
through the public API (e.g., create a second Connection to the same db file,
execute a locking statement like "BEGIN IMMEDIATE" on that second Connection to
force a SQLITE_LOCKED condition, then call iterator.failableNext() and assert it
throws with XCTAssertThrowsError or returns nil as appropriate). Remove the
direct sqlite3_close_v2 call and replace the loose success path in the do/catch
with a proper assertion that fails if no error is thrown; keep cleanup (try?
FileManager.default.removeItem(atPath: dbPath)) but ensure it runs after
restoring/removing the lock.
- Around line 161-198: The test testBusyTimeoutHandlesConcurrentAccess currently
uses WAL and manually-created Connection instances so readers never hit
busy-timeout; change it to open connections via the production helper
Database.openConnection(...) and create a real contention scenario (e.g., set
PRAGMA journal_mode=DELETE or ensure WAL is off, start an exclusive/IMMEDIATE
write transaction on the writer Connection to hold a lock, then attempt a read
on the reader opened through Database.openConnection with a short busyTimeout)
so the reader actually blocks and exercises the busyTimeout path; reference the
testBusyTimeoutHandlesConcurrentAccess function, the writer/reader Connection
instances, busyTimeout property, PRAGMA journal_mode setting, and
Database.openConnection(...) when making the edits.
---
Outside diff comments:
In `@patches/react-native-navigation`+7.45.0.patch:
- Around line 263-272: The invalidate() method unconditionally casts activity()
to NavigationActivity which can cause a ClassCastException; before casting,
check that activity() is an instance of NavigationActivity (or null) and only
call navigationActivity.onCatalystInstanceDestroy() when that check passes,
otherwise skip that call; keep the call to super.invalidate() regardless.
Reference: invalidate(), activity(), NavigationActivity,
onCatalystInstanceDestroy(), super.invalidate().
- Around line 126-228: The navigator Optional wrapping causes commands to
silently no-op and leaves NativeCommandListener (and JS promises) unresolved;
instead of replacing calls like
navigator().setRoot(...)/push(...)/mergeOptions(...) with
navigator().ifPresent(...), detect when navigator() is empty and explicitly
invoke the corresponding NativeCommandListener error path so the Promise is
settled: for each call site that now uses navigator().ifPresent(nav ->
nav.<action>(... , new NativeCommandListener(...))), keep creating the same
NativeCommandListener instance and, if navigator().isPresent() call
nav.<action>(...), else call the listener's failure/reject callback (or
otherwise invoke its error handling method) so the JS promise is rejected; apply
this pattern for setRoot, setDefaultOptions, mergeOptions, push, setStackRoot,
pop, popTo, popToRoot, showModal, dismissModal (ensure mergeOptions still runs
against nav when present), dismissAllModals, showOverlay, dismissOverlay and
dismissAllOverlays.
- Around line 355-378: The gesture callbacks onChildStartedNativeGesture and
onChildEndedNativeGesture currently force-unwrap getEventDispatcher() (!!) and
risk NPEs; change them to mirror the safe pattern used elsewhere by
null-checking androidEvent and then safely obtaining the dispatcher (call
getEventDispatcher() and .let on it) before calling
mJSTouchDispatcher.onChildStartedNativeGesture/onChildEndedNativeGesture, and
also remove the forced unwrap when retrieving the UIManagerModule inside
getEventDispatcher so it can return null safely (ensure UIManagerModule lookup
is null-checked before accessing eventDispatcher).
- Around line 315-321: The bottom padding is incorrectly hardcoded to 0 when
applying IME insets; in the block that gets Activity via getActivity(), computes
Insets ime = insets.getInsets(WindowInsetsCompat.Type.ime()), and checks
softInputMode == WindowManager.LayoutParams.SOFT_INPUT_ADJUST_NOTHING &&
!isRoot(), change the view.setPadding call to use ime.bottom for the bottom
padding instead of 0 so the keyboard height is respected, then return
WindowInsetsCompat.CONSUMED as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e07d31c-d8d0-4f59-81c2-2285a9913f6a
📒 Files selected for processing (10)
app/products/playbooks/screens/playbook_run/checklist/checklist_item/checklist_item.test.tsxios/Gekidou/Sources/Gekidou/Storage/Database+Channels.swiftios/Gekidou/Sources/Gekidou/Storage/Database+Mentions.swiftios/Gekidou/Sources/Gekidou/Storage/Database+System.swiftios/Gekidou/Sources/Gekidou/Storage/Database+Team.swiftios/Gekidou/Sources/Gekidou/Storage/Database+Thread.swiftios/Gekidou/Sources/Gekidou/Storage/Database+Users.swiftios/Gekidou/Sources/Gekidou/Storage/Database.swiftios/Gekidou/Tests/GekidouTests/DatabaseSafeIterationTests.swiftpatches/react-native-navigation+7.45.0.patch
Coverage Comparison Report |
pavelzeman
added
release-note-none
and removed
do-not-merge/release-note-label-needed
labels
|
@pavelzeman: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsI understand the commands that are listed here |
larkox
left a comment
larkox
left a comment
There was a problem hiding this comment.
Another PR where the actual fix got code bleeds from other PRs. The change here should be only on checklist_item.test.tsx, but I saw this same change on the "fix db" PR. Apart of that, LGTM.
You're right - I was cherry-picking the "flaky test" to get to a clean CI. I'll clean that up before merging. |
The 'renders the correct checkbox' test was flaky because it used synchronous act() to trigger async state updates. When toggleChecked() is called, it sets isChecking=true, awaits updateChecklistItem(), then sets isChecking=false. The synchronous act() doesn't flush microtasks from the async continuation, so setIsChecking(false) never executes within the act boundary, leaving the loading indicator visible. Changed both act() calls to await act(async () => ...) so React properly flushes all pending microtasks and state updates. Co-authored-by: Claude <claude@anthropic.com>
dd6c0af to
19b9f0b
Compare
3 participants
Summary
Fixes the flaky
ChecklistItem › renders the correct checkboxtest.Root Cause
The test used synchronous
act()to triggertoggleChecked(), which is an async function that callssetIsChecking(true), awaitsupdateChecklistItem(), then callssetIsChecking(false). Synchronousact()does not flush microtasks from the async continuation, sosetIsChecking(false)never ran within the act boundary — the loading indicator stayed visible and thewaitForassertion timed out.Fix
Changed both
act()calls toawait act(async () => ...)so React properly flushes all pending microtasks and state updates before the subsequent assertions.Testing
CI should confirm the test passes consistently.
Migrated from #9612 (fork → org branch)
Release Note