fix(preview): improve browser recording quality - #8839

💭 I'm checking the current head to verify whether the previously flagged issues were fixed and no new ones introduced. The timeout error was addressed by converting it to a proper TaggedErrorClass, and I'm now confirming the save-artifact format issue was similarly resolved with its own tagged error class.

1. git_tools — ^import|^} from|from "effect",

2. browse_code — apps/web/src/browser/browserRecording.ts

💭 I want to look further into lines 200-500 to check how error handling is structured.

3. browse_code — apps/web/src/browser/browserRecording.ts

💭 Most of the errors in this file are pre-existing legacy patterns, not introduced by this PR. I'm focusing on what actually changed here: the new "cleanup" operation literal, the capture-media-stream logic, and the format-unavailable and capture-timeout error cases, checking each for correctness. I trace through whether the new predicate follows the schema convention properly — it's a private const used directly rather than wrapped redundantly, so it seems fine, consistent with how isBrowserRecordingOperationError is also used privately. The timeout error creation in captureTabMediaStreamWithTimeout looks correctly addressed too, so I move on to check the changes in Manager.ts's startRecording implementation.

4. git_tools — recording.measureSource|recording.warmSource|recording.getMediaSourceId|PreviewRecordingSourceSizeUnavailableError

💭 With prior findings addressed, I want to check the new preview.ts IPC method to see if it follows conventions like makeIpcMethod usage.

5. browse_code — apps/desktop/src/ipc/methods/preview.ts

💭 That looks conventional. Now I want to check how useSettings and getClientSettings handle imperative access at the web layer, since browserRecording.ts sits on that imperative boundary — I need to see if these helpers rely on runPromise or a hidden ManagedRuntime.

6. git_tools — ensureClientSettingsHydrated|export const getClientSettings|export function getClientSettings

💭 That's a pre-existing pattern used elsewhere, so it checks out. I want to see if browserSurfaceStore's acquireBrowserSurfaceActivity is part of the diff, but it doesn't appear to be included even though the test references activityByTabId, so it may already exist and I need to verify.

7. git_tools — acquireBrowserSurfaceActivity

💭 I'm checking whether the new operation literals are properly added to the PreviewOperationError list, and looking at whether the attempt/attemptPromise helpers might violate the rule against trivial wrapper helpers that just construct an error.

8. git_tools — class PreviewOperationError

💭 That's pre-existing, so no new issue there. The Effect.ignore on warmSource seems intentional given the comment about transient warmup, not a convention violation. Now I want to check the tail end of browserRecording.ts around lines 520-660 for the format-unavailable error handling and the cleanup operation.

9. browse_code — apps/web/src/browser/browserRecording.ts

💭 I'm noticing that the cleanup failure wraps an already-structured domain error into a generic "cleanup" error, losing the original error's distinction—this seems worth flagging since the convention is to pass through structured domain errors at translation boundaries rather than re-wrapping them. I want to compare against the pre-change code to see whether it previously handled the result._tag Failure and cleanupError differently, since the old operation label was "stop-media-recorder" with what looks like a similar aggregation pattern.

10. browse_code — apps/web/src/browser/browserRecording.ts

💭 I see that the "cleanup" operation literal already existed in the base code, so that's pre-existing behavior for the aggregate path. What's new is that cleanup errors from stopMediaRecorder are now reported under operation: "cleanup" rather than "stop-media-recorder". That's a slight loss of specificity, but since "cleanup" now genuinely covers two sources (recorder stop and stream stop), it fits the multi-value discriminator model and the test confirms this expectation — not a clear convention violation. The AggregateError usage also looks fine since it aggregates real errors rather than manufacturing one just to populate cause. I'm now looking at whether mapping a null hostWebContents to PreviewMainWindowClosedError is semantically appropriate, since that error's message describes a different condition than what's actually happening — possibly a mis-modeled error reuse worth flagging against the convention about caller-visible error messages accurately describing the condition. Thinking it through more, if a webview's host webContents is the main window's webContents, then a null value logically follows from the main window being destroyed, so the mapping is actually reasonable rather than a violation. I also confirm getMediaSourceId is a synchronous Electron API returning a string, so that's fine, and I'm now checking how DesktopPreviewRecordingSourceSchema is declared against existing schema patterns in that file for consistency. I'm also reviewing settings.ts to see if the Schema.Literals and withDecodingDefault usage matches existing conventions, and checking whether recording frame listeners are now dead code since the diff removed frame delivery — though I recognize dead code isn't really what this convention check is meant to catch, so I probably shouldn't flag it as a violation.

11. git_tools — recordingFrameListenersRef

💭 That lock usage in startRecording looks like a potential deadlock, but that's a correctness issue outside the scope of this Effect service convention check. The retry-then-ignore pattern silently swallows errors too, but again, conventions don't cover that. Let me check the test file's imports for Exit, Cause, and Option to confirm they're properly namespace-imported from their subpaths.

12. browse_code — apps/desktop/src/preview/Manager.test.ts

💭 Checking the imports and settings files, I don't see any remaining Effect-convention violations after the prior fixes—so I'm leaning toward outputting "All clear" since there are no findings to report. I double-check the browserRecording.ts predicate naming and it matches the existing module pattern, so that's not worth flagging. I also glance at the contracts ipc.ts schema fields for readonly consistency but nothing stands out as a violation there either.

13. browse_code — packages/contracts/src/ipc.ts

💭 Everything looks consistent with no findings, so I'll report success. Since the schema requires JSON but the instruction wants an exact final response, I'll place "All clear" in the summary field.