fix(desktop): isolate preview keyboard input - #8494

💭 I'm confirming the reason discriminator fix by splitting into three error classes checks out in the diff, and now I'm scanning the changed files for import convention violations, particularly whether they use consolidated "effect" imports versus namespace subpath imports. The new error classes look structured properly with tagged error schemas and clean exports, and the IPC handler correctly uses catchTags. I'm now looking closely at the fromCause factory, which switches on the cause's _tag to classify errors — checking whether this kind of tag-based dispatch at a translation boundary fits the established convention for static factory mappers. I still need to check whether the new host errors preserve the underlying cause, or whether that omission matches existing patterns in the same file — I should pull up the actual file contents to confirm.

1. browse_code — apps/web/src/components/preview/previewAutomationErrors.ts

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

💭 I'm reviewing the new error classes in Manager.ts near line 4827, checking that the union type and type guard export follow existing conventions, and verifying whether PreviewAutomationTargetChangedError's operation field creates a redundant discriminator issue. I'm also weighing whether the new host errors should preserve cause when translating underlying failures—but since the sibling PreviewAutomationTargetNotEditableHostError already omits it, that's the established pattern in this file, so I'll skip flagging it as noise rather than a real violation. Now I'm moving on to scan the rest of Manager.ts for other potential convention issues.

3. browse_code — apps/desktop/src/preview/Manager.ts

4. browse_code — apps/desktop/src/preview/Manager.ts

💭 The imports look fine. Now I'm looking at PreviewKeyboard.ts, where resolveKeyDefinition throws a plain Error for unsupported keys inside a pure helper called from performAutomationPress, an Effect. Throwing a defect like this seems at odds with the convention of using Schema.TaggedErrorClass for service failures, so I want to check if this counts as a violation. Since makePreviewAutomationKeySequence runs inside an Effect.fn generator, a thrown Error becomes a defect that kills the fiber instead of surfacing as a typed failure — but if the key schema already restricts values to a literal union at the IPC boundary, this throw is unreachable and functions more as a defensive invariant check than a real service failure, so I'm leaning toward not flagging it. I still need to verify whether DesktopPreviewAutomationPressInputSchema actually enforces that literal key union to confirm the throw path is truly unreachable.

5. git_tools — DesktopPreviewAutomationPressInputSchema

💭 The input is validated against the literal union, so that throw path is defensively unreachable and not worth flagging. The IPC handler's catchTags usage looks correct with all four tags covered. I'm now checking how the web-side function throws the decoded error into a promise chain for classification, and whether throwing a plain object rather than an Error instance could cause issues downstream. This is a deliberate boundary design since Electron IPC can't carry typed errors, so the result union gets unwrapped by throwing a tagged struct into the existing catch-based path — consistent with how desktop errors already surface elsewhere as thrown values, so I won't flag this. I still want to check whether the call site in PreviewAutomationHosts.tsx wraps this in an Effect.tryPromise or handles it through a plain promise-based path, so I need to look at the surrounding code.

6. browse_code — apps/web/src/components/preview/PreviewAutomationHosts.tsx

💭 This promise-based host handler with a catch classifying via fromCause looks consistent with the existing design. I'm now checking the manager's semaphore usage and the preview manager's runFork helper, and I want to grep the changed files for catchTag, catchIf, and runPromise usages to make sure nothing new was introduced inconsistently.

7. git_tools — catchTag(|catchIf|runPromise|ManagedRuntime|Layer.succeed

8. git_tools — runFork|const attempt|attemptPromise

💭 These are existing patterns already following convention, so that's fine. I'm noting that the new attempt wrapping in automationPress uses operation strings consistent with existing PreviewOperationError patterns, and checking how dispatch assigns errors to a preflightError variable before failing through the effect chain. Now let me look at the full performAutomationPress code to check for any error-construction helper violations.

9. browse_code — apps/desktop/src/preview/Manager.ts

💭 The errors are constructed directly at failure boundaries with structured fields, which looks good. I'm checking the new IPC contract schemas for desktop preview automation press errors, and they mirror the desktop error classes since desktop errors can't be transported across the boundary — this duplication seems reasonable and isn't forbidden by conventions. I check whether operation fields avoid double-encoding distinctions between error tags and single-value literals — the target-changed error uses multi-value operations correctly, as do the host-side errors. The pure domain errors lacking a cause field is fine since there's no underlying failure. The IPC handler's catchTags mapping across four tags to Failure looks like good usage, and I'm now checking whether a isPreviewAutomationPressError predicate is properly exported as a public schema predicate rather than kept as a private constant. The private, unexported isPreviewAutomationPressError is used directly and isn't wrapped by a redundant function, so it doesn't violate the wrapping convention even though it's not exported. The unwrapPreviewAutomationPressResult helper unwraps a result union rather than just constructing an error, so that's also fine against the "no trivial error-constructing helper" rule. I check the web side files for any new runtime Effect calls introduced by the diff, but it looks like only the unwrap logic was added, and the desktop Manager interfaces seem fine too. I'm weighing whether a throw inside PreviewKeyboard.ts's Effect.gen body should instead use a proper TaggedErrorClass — since invalid input is already validated at the contract boundary to a literal union, this path is unreachable and functions more as an invariant guard in a pure helper module rather than a service, so I won't flag it. Finally, I'm scanning for any remaining consolidated import { X } from "effect" in touched non-test source files, noting that the new contracts test file imports Schema this way, and I want to confirm whether that matches existing conventions in other contracts files.

10. git_tools — ^import .*from "effect";

💭 This module is a pure schema file, not Effect service code, so the named-import convention doesn't apply here—not a finding. I scan the rest of the Manager diff for helper functions creating errors or message-getter switches, finding none, then move to checking the destructuring pattern. In preview.ts, the catchTags handler encodes an Effect error class instance against a TaggedStruct schema, which should work fine since the fields align—not a convention issue. I also confirm the manager's automationPress method type still includes the new errors in its union, and that my prior review comment about splitting semantically distinct failures into separate error classes has already been addressed, as noted in the "no longer relevant" comment. Now I want to verify the contracts-side duplication doesn't conflict with the guidance to keep a service's schemas/errors in one canonical module, since the desktop error classes live in Manager.ts but contracts mirrors them. The mirroring is acceptable since it's a cross-process boundary and contracts can't import from apps/desktop anyway. Checking the fromCause mapping, it uses a structural schema predicate to classify the error union, which is fine, and the switch on cause._tag falls through appropriately without needing a default since it's exhaustive. The catchTags convention about not using catchIf with schema predicates doesn't apply here since fromCause is plain classification logic, not an Effect catch. I've concluded there are no findings, but now I need to resolve a conflict in how to report this: the complete_check tool expects structured fields, while a separate instruction insists the entire final response be exactly "All clear" with no formatting. I'll treat that explicit instruction as authoritative and output the plain text "All clear" rather than invoking the structured tool.