[krassx]

Summary

• createReport() returns one outstanding BugseeCreatedReport, or rejects E_REPORT_CREATE_BUSY.
• upload() marks the handle dead synchronously before crossing. A created-report file attachment sends no move.
• Both native createReport paths are stubs. Android and iOS implementations are the next two PRs, which branch from here.

Test plan

• yarn test: 1644 passed, 1 skipped
• Mutations reverted: dead-after-resolve, and move: true on created-report files
• Merge after Task 8.1. Then merge 8.2b and 8.2c, which are parallel.

Made with Cursor

[cursor]

Stale comment

Deep review: Task 8.2a createReport JS API (native stubbed)

Reviewed cd79431 against the handler report proxy, toReportError / normalizeSnapshot, TurboModule spec + both module stubs, the mock, Task 8.2a in the implementation plan, and the 8.2b/8.2c contracts this slice is meant to unlock. Did not review the diff in isolation.

No P0–P3 findings.

Adversarial checks that did not produce a reportable bug:

• Dead-before-cross. CreatedReport.upload() sets dead synchronously, then crosses. Two overlapping upload() calls issue one native call; the second rejects E_REPORT_HANDLE_DEAD locally. A native rejection after that still leaves the JS handle dead, matching the spec note (“whatever the result”).
• Validation parity. update / addFileAttachment / addDataAttachment use the same validateReportPatch, normalizeFilePath, validateBase64, and validateNonEmptyString path as BugseeReportProxy. A patch with one bad field sends nothing. file:// is stripped; there is no move argument (call arity pinned at 4).
• Error mapping. Native E_REPORT_CREATE_BUSY becomes BugseeReportError with that code. Unknown rejections pass through. handleId == null treats both null and undefined as “SDK made none”.
• One-at-a-time (P5). Not enforced in JS; that is native reserve() in 8.2b/8.2c. Stubs never mint a handle, so two createReport() calls both resolve null, which is truthful. No discard API, matching the plan (release is upload() or JS reload / invalidate).
• Stubs. Android createReport resolves null; iOS resolve(nil). The five createdReport* methods reject E_REPORT_HANDLE_DEAD without touching the SDK. Selectors/signatures match existing report* methods (patchJson as JSON text, nullable mimeType, Promise resolve/reject). RN compat 0.81 / 0.83 / 0.86 / 0.87 already passed codegen + java-signatures against this spec.
• In-flight update vs upload. JS is single-threaded; ensureAlive is only re-checked at the start of each method. An update that has already yielded to native can still complete after upload() has marked dead. Same pattern as the handler proxy; 8.2b take() then makes native get() return dead. Sequential await update(); await upload() is fine.
• Handler timing / iOS attachment caps. Documented on createReport (P6/P7); JS does not pretend to normalise them. Limits are native 8.2c.

Did not run yarn test here (node_modules absent). At review time: rn-compat SUCCESS on 0.81–0.87; lint/unit/mutation, Android, and iOS jobs were still in progress.

Overall risk: Low

Merge recommendation

Safe to merge into feat/phase-8-1. This is the specified sequential slice: the public JS API and the TurboModule surface land first; Android 8.2b and iOS 8.2c replace the stubs. Do not treat this merge as a working createReport() for apps — on a device it always resolves null until those PRs land.

Related: the Android follow-up (PR 20) has a separate invalidate()/CreatedReports.clear() reload-order issue; that is native-side and not introduced here.

Most important issues to fix

None in this PR.

Positive observations

• Reuse of the handler proxy’s validators, JSON-text patch transport, and snapshot normalisation avoids a second wire shape.
• upload() dead-before-cross is tested (including the concurrent case the plan’s mutate step was written to catch).
• File attachments cannot accidentally pass move: true at the spec layer.
• ios-spec-coverage and java-signatures derive methods from the spec, so the six new native methods cannot silently miss an implementation.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep review: Task 8.2a createReport JS API (native stubbed)

Reviewed c8513e8 (base c437703 / feat/phase-8-1) against the handler report proxy, toReportError / normalizeSnapshot, TurboModule spec + both module stubs, the mock, Task 8.2a in the implementation plan, and the 8.2b/8.2c contracts this slice is meant to unlock. Did not review the diff in isolation.

Prior assessment was cd79431. Three later commits are tests only; production CreatedReport and the native stubs are unchanged.

No P0–P3 findings.

Adversarial checks that did not produce a reportable bug:

• Dead-before-cross. CreatedReport.upload() sets dead synchronously, then crosses. Two overlapping upload() calls issue one native call; the second rejects E_REPORT_HANDLE_DEAD locally. A native rejection after that still leaves the JS handle dead, matching the spec note (“whatever the result”).
• Validation parity. update / addFileAttachment / addDataAttachment use the same validateReportPatch, normalizeFilePath, validateBase64, and validateNonEmptyString path as BugseeReportProxy. A patch with one bad field sends nothing. file:// is stripped; there is no move argument (call arity pinned at 4).
• Error mapping. Native E_REPORT_CREATE_BUSY becomes BugseeReportError with that code. Unknown rejections pass through. handleId == null treats both null and undefined as “SDK made none”.
• One-at-a-time (P5). Not enforced in JS; that is native reserve() in 8.2b/8.2c. Stubs never mint a handle, so two createReport() calls both resolve null, which is truthful. No discard API, matching the plan (release is upload() or JS reload / invalidate).
• Stubs. Android createReport resolves null; iOS resolve(nil). The five createdReport* methods reject E_REPORT_HANDLE_DEAD without touching the SDK. Selectors/signatures match existing report* methods (patchJson as JSON text, nullable mimeType, Promise resolve/reject). RN compat 0.81 / 0.83 / 0.86 / 0.87 passed codegen + java-signatures against this spec on this SHA.
• In-flight update vs upload. JS is single-threaded; ensureAlive is only re-checked at the start of each method. An update that has already yielded to native can still complete after upload() has marked dead. Same pattern as the handler proxy; 8.2b take() then makes native get() return dead. Sequential await update(); await upload() is fine.
• Handler timing / iOS attachment caps. Documented on createReport (P6/P7); JS does not pretend to normalise them. Limits are native 8.2c.

Test-only delta after cd79431:

• Created-report coverage for omitted mime type, empty path/name, data-attachment wire, native rejection mapping, and the dead-handle message.
• upload / showReportDialog tests pin severity 5 and the exact TypeError/RangeError text.
• BGSRNSettleOnceTests race uses deadlineMs=0 (a positive deadline is a third caller into that two-party barrier) and 5s waits instead of DISPATCH_TIME_FOREVER.

Did not run yarn test here (node_modules absent). At review time: rn-compat SUCCESS on 0.81–0.87; lint/unit/mutation, Android, and iOS jobs were still pending on this SHA.

Overall risk: Low

Merge recommendation

Safe to merge into feat/phase-8-1. This is the specified sequential slice: the public JS API and the TurboModule surface land first; Android 8.2b and iOS 8.2c replace the stubs. Do not treat this merge as a working createReport() for apps — on a device it always resolves null until those PRs land.

Related: the Android follow-up (PR 20) has a separate invalidate()/CreatedReports.clear() reload-order issue; that is native-side and not introduced here.

Most important issues to fix

None in this PR.

Positive observations

• Reuse of the handler proxy’s validators, JSON-text patch transport, and snapshot normalisation avoids a second wire shape.
• upload() dead-before-cross is tested (including the concurrent case the plan’s mutate step was written to catch).
• File attachments cannot accidentally pass move: true at the spec layer.
• ios-spec-coverage and java-signatures derive methods from the spec, so the six new native methods cannot silently miss an implementation.
• The later test commits kill the mutation survivors (severity 5, exact argument-error text, attachment/rejection paths) without changing production behaviour.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep review: Task 8.2a createReport JS API (native stubbed)

Reviewed bdf2522 (base c437703 / feat/phase-8-1) against the handler report proxy, toReportError / normalizeSnapshot, TurboModule spec + both module stubs, the mock, Task 8.2a in the implementation plan, and the 8.2b/8.2c contracts this slice is meant to unlock. Did not review the diff in isolation.

Prior assessment was c8513e8. Production CreatedReport and the native stubs are unchanged since cd79431. The only new commit is a test fix.

No P0–P3 findings. No prior inline threads to resolve.

Adversarial checks that did not produce a reportable bug:

• Dead-before-cross. CreatedReport.upload() sets dead synchronously, then crosses. Two overlapping upload() calls issue one native call; the second rejects E_REPORT_HANDLE_DEAD locally. A native rejection after that still leaves the JS handle dead, matching the spec note (“whatever the result”).
• Validation parity. update / addFileAttachment / addDataAttachment use the same validateReportPatch, normalizeFilePath, validateBase64, and validateNonEmptyString path as BugseeReportProxy. A patch with one bad field sends nothing. file:// is stripped; there is no move argument (call arity pinned at 4).
• Error mapping. Native E_REPORT_CREATE_BUSY becomes BugseeReportError with that code. Unknown rejections pass through. handleId == null treats both null and undefined as “SDK made none”.
• One-at-a-time (P5). Not enforced in JS; that is native reserve() in 8.2b/8.2c. Stubs never mint a handle, so two createReport() calls both resolve null, which is truthful. No discard API, matching the plan (release is upload() or JS reload / invalidate).
• Stubs. Android createReport resolves null; iOS resolve(nil). The five createdReport* methods reject E_REPORT_HANDLE_DEAD without touching the SDK. Selectors/signatures match existing report* methods (patchJson as JSON text, nullable mimeType, Promise resolve/reject). RN compat 0.81 / 0.83 / 0.86 / 0.87 passed codegen + java-signatures against this spec on this SHA.
• In-flight update vs upload. JS is single-threaded; ensureAlive is only re-checked at the start of each method. An update that has already yielded to native can still complete after upload() has marked dead. Same pattern as the handler proxy; 8.2b take() then makes native get() return dead. Sequential await update(); await upload() is fine.
• Handler timing / iOS attachment caps. Documented on createReport (P6/P7); JS does not pretend to normalise them. Limits are native 8.2c.

Delta since c8513e8 (test(ios): keep each settle-once window on its own release):

• testTwoConcurrentCallsSettleOnce now has a separate enter/release semaphore pair per RunRaceWindow (before the once-flag load, then after it). A shared go let the faster caller leave window 0, enter window 1, and consume the other thread’s release, so that caller never reached window 1 (CI timed out).
• Per-thread _Thread_local phase maps each fire()’s two visits onto those two windows. That is correct for a single run of this test (each concurrent caller hits the hook twice on its own GCD worker). Production BGSRNSettleOnce is unchanged.

Did not run yarn test here (node_modules absent). At review time on this SHA: rn-compat SUCCESS on 0.81–0.87; lint/unit/mutation, Android, and iOS jobs were still in progress. The earlier mutation failure on cd79431 (score 94.57 vs break 95, CreatedReport.ts survivors) is what the later JS test commits were written to kill.

Overall risk: Low

Merge recommendation

Safe to merge into feat/phase-8-1. This is the specified sequential slice: the public JS API and the TurboModule surface land first; Android 8.2b and iOS 8.2c replace the stubs. Do not treat this merge as a working createReport() for apps — on a device it always resolves null until those PRs land.

Related: the Android follow-up (PR 20) has a separate invalidate()/CreatedReports.clear() reload-order issue; that is native-side and not introduced here.

Most important issues to fix

None in this PR.

Positive observations

• Reuse of the handler proxy’s validators, JSON-text patch transport, and snapshot normalisation avoids a second wire shape.
• upload() dead-before-cross is tested (including the concurrent case the plan’s mutate step was written to catch).
• File attachments cannot accidentally pass move: true at the spec layer.
• ios-spec-coverage and java-signatures derive methods from the spec, so the six new native methods cannot silently miss an implementation.
• The per-window settle-once barrier matches how RunRaceWindow is actually invoked and removes a real CI timeout mode without weakening the double-settle assertion.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep review: Task 8.2a after retarget to main

Reviewed 3d2c8e9 (Merge main into feat/phase-8-2a) against current main (669b492, which is merged #18). Prior assessment was bdf2522 onto feat/phase-8-1.

The unique 8.2a surface is unchanged. git diff bdf2522 HEAD for CreatedReport.ts, NativeBugsee.ts, index.ts, types, errors, and created-report.test.ts is empty. Versus main, GitHub’s file list is the original 11-file 8.2a slice (stubs + JS API + tests). The merge kept this branch’s createReport / createdReport* stubs and took main’s split ExceptionExecutors (HANDLED vs UNHANDLED) plus the #18 dialog fix (omitted iOS summary/description stay nil).

No P0–P3 findings. No prior inline threads to resolve.

Adversarial checks on the merge that did not produce a reportable bug:

• Stubs survived. Android still promise.resolve(null) / rejectHandleDead; iOS still resolve(nil) / BGSRNRejectHandleDead. No duplicate methods, no dropped @Overrides.
• Exception path not mixed into createReport. Handled/unhandled still post onto ExceptionExecutors; createReport does not call the SDK.
• JS contract. Dead-before-cross upload(), validator parity with the handler proxy, handleId == null as none, E_REPORT_CREATE_BUSY mapping, no move on created-report files — all as previously reviewed.

Did not run yarn test here. CI on bdf2522 was SUCCESS; a new ci run is queued on this merge SHA.

Overall risk: Low

Merge recommendation

Safe to merge into main. Same sequential slice as before: public JS API + TurboModule stubs. On a device createReport() still resolves null until 8.2b/8.2c land. #18’s dialog-nil fix is already on main and is present on this head.

Related residual remains native-side on PR 20 (invalidate() / CreatedReports.clear()), not introduced here.

Most important issues to fix

None in this PR.

Positive observations

• Merge resolution is mechanical and correct: 8.2a stubs on top of main’s exception-executor split, no conflict leftovers.
• Retargeting onto merged #18 shrinks the GitHub diff to the actual 8.2a files instead of re-shipping Phase 7 / 8.1.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Deep review: Task 8.2a createReport JS API (native stubbed)

Reviewed 3d2c8e9 (merge of main / 669b492 into feat/phase-8-2a; production 8.2a still cd79431) against the handler report proxy, toReportError / normalizeSnapshot, TurboModule spec + both module stubs, the mock, Task 8.2a in the implementation plan, and the 8.2b/8.2c contracts this slice unlocks. Did not review the diff in isolation.

Prior assessment was bdf2522. No GitHub inline threads to resolve.

No P0–P3 findings.

Adversarial checks that did not produce a reportable bug:

• Dead-before-cross. CreatedReport.upload() sets dead synchronously, then crosses. Two overlapping upload() calls issue one native call; the second rejects E_REPORT_HANDLE_DEAD locally. A native rejection after that still leaves the JS handle dead, matching the spec (“whatever the result”).
• Validation parity. update / addFileAttachment / addDataAttachment use the same validateReportPatch, normalizeFilePath, validateBase64, and validateNonEmptyString path as BugseeReportProxy. A patch with one bad field sends nothing. file:// is stripped; there is no move argument (call arity pinned at 4).
• Error mapping. CreateBusy is in ReportErrorCode, so toReportError maps native E_REPORT_CREATE_BUSY. Unknown rejections pass through. handleId == null treats both null and undefined as “SDK made none”.
• One-at-a-time (P5). Not enforced in JS; that is native reserve() in 8.2b/8.2c. Stubs never mint a handle, so two createReport() calls both resolve null, which is truthful. No discard API, matching the plan (release is upload() or JS reload / invalidate).
• Stubs. Android createReport resolves null; iOS resolve(nil). The five createdReport* methods reject E_REPORT_HANDLE_DEAD without touching the SDK. Selectors/signatures match existing report* methods (patchJson as JSON text, nullable mimeType). RN compat 0.81 / 0.83 / 0.86 / 0.87 passed codegen + java-signatures against this spec on this SHA.
• In-flight update vs upload. JS is single-threaded; ensureAlive is only re-checked at the start of each method. An update that has already yielded to native can still complete after upload() has marked dead. Same pattern as the handler proxy; 8.2b take() then makes native get() return dead. Sequential await update(); await upload() is fine.
• Handler timing / iOS attachment caps. Documented on createReport (P6/P7); JS does not pretend to normalise them. Limits are native 8.2c.
• Merge of main. createReport stubs from this branch are intact (one of each method, no duplicates). Main’s split ExceptionExecutors (handled vs unhandled queues), beta3 unhandled-recovery comment, and showReportDialog omitted fields as nil (not @"") were kept. Those are already on main; this merge does not regress them.

Did not run yarn test here. At review time on this SHA: rn-compat SUCCESS on 0.81–0.87; lint/unit/mutation, Android, and iOS jobs were still pending.

Overall risk: Low

Merge recommendation

Safe to merge into main. Task 8.1 is already there (669b492). This is the specified sequential slice: the public JS API and the TurboModule surface land first; Android 8.2b and iOS 8.2c replace the stubs. Do not treat this merge as a working createReport() for apps — on a device it always resolves null until those PRs land.

Related: the Android follow-up (PR 20) has a separate invalidate()/CreatedReports.clear() reload-order issue; that is native-side and not introduced here.

Most important issues to fix

None in this PR.

Positive observations

• Merge resolution is the right split: keep this branch’s createReport surface, take main’s already-shipped exception-executor split and dialog-nil behaviour.
• Reuse of the handler proxy’s validators, JSON-text patch transport, and snapshot normalisation avoids a second wire shape.
• upload() dead-before-cross is tested (including the concurrent case the plan’s mutate step was written to catch).
• File attachments cannot accidentally pass move: true at the spec layer.
• ios-spec-coverage and java-signatures derive methods from the spec, so the six new native methods cannot silently miss an implementation.

  

_{Sent by Cursor Automation: Bugsee code review}