Skip to content

fix(preview): validate popup URLs before loading - #6156

Closed
playb0t wants to merge 1 commit into
pingdotgg:mainfrom
playb0t:fix/preview-popup-url-validation
Closed

playb0t wants to merge 1 commit into
pingdotgg:mainfrom
playb0t:fix/preview-popup-url-validation

fix(preview): validate popup URLs before loading

adf1132
Select commit
Loading
Failed to load commit list.
MacroscopeApp / Macroscope - Effect Service Conventions succeeded Aug 11, 2026 in 59s

All clear

All clear

Details

Note

Your check run agent prompt is: .macroscope/check-run-agents/effect-service-conventions.md
More information about how Check Run Agents work can be found in our Help Center.


🔍 Agent activity — reasoning, no tool calls

💭 Looking at the error handling pattern in setWindowOpenHandler—there's a bare try/catch that silently swallows a domain error, which goes against the convention of using Effect.catchTags for statically known failures. Though this is at a native callback boundary where imperative error handling is acceptable, the codebase already has an attempt helper and an isPreviewUrlNormalizationError predicate that could be leveraged here instead.… I'm looking at wrapping the normalizePreviewUrl call in error handling—either with a try/catch block or using the repo's attempt helper from Manager.ts. I need to check how that helper models errors and whether the convention favors keeping failures typed in Effect rather than silently swallowing them in an imperative catch.… I'm noticing the current approach uses a raw try/catch that silently fails, whereas the convention would be to use the attempt helper for synchronous operations that throw, which would properly type the error as PreviewOperationError. The convention docs don't explicitly flag try/catch as a violation, but the handler needs to return synchronously, so I'm weighing whether the idiomatic pattern applies here or if the constraint changes things. Looking at the actual behavior, both the try/catch and Effect.ignore discard the failure without logging, so there's parity there. The question is whether this counts as a concrete convention violation or just a style preference—and given the instructions to only report concrete violations, I'm leaning toward this being acceptable, though a reviewer might reasonably push back on it. I need to format the response as JSON per the schema requirements, so I'll structure it with the success state and "All clear" as the summary, even though the instruction asks for that exact phrase on one line.…


Agent Credits: 51 credits