refactor: Effect-ify the workers #2185
Conversation
WalkthroughThis pull request refactors network operations in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (6)
apps/workers/workers/videoWorker.ts (1)
4-4: Video worker’s migration toEither-based URL validation looks correctImporting
Eitherand switching toEither.isRight(validation)withvalidation.right.toString()for the normalized URL aligns with the newvalidateUrlreturn type; loggingvalidation.left.messageon failure keeps behavior clear while honoring the new error model.If you want more diagnostic detail when debugging URL issues, you could also log
validation.left._tag(error type) alongsidemessage.Also applies to: 111-118
apps/workers/workers/crawlerWorker.ts (1)
13-13: Crawler worker’s switch toEither-based URL validation is soundThe new
Eitherimport and usage (Either.isRight,.left.message,.right.toString()) for both sub-request blocking and main navigation correctly match the refactoredvalidateUrlcontract and preserve the intended control flow (block disallowed URLs, throw on invalid navigation targets).If you need to distinguish parse/DNS/validation failures operationally, consider also logging
validation.left._tagin addition to.message.Also applies to: 511-518, 525-535
apps/workers/network.ts (4)
70-90: DNS resolver service and cache logic look correct and safeThe
DnsResolverServiceContext tag,DnsResolverLivelayer, andDnsCache(5‑minute TTL, capacity 1000, dual A/AAAA lookup viaEffect.all+Either) form a reasonable, side‑effect‑controlled DNS resolution path, and the code correctly fails withHostAddressResolutionErrorwhen no addresses are resolved.If DNS resolution becomes a bottleneck, consider tightening
capacity/TTL or adding simple metrics around cache hit/miss rates for observability.Also applies to: 92-140
268-277:matchesNoProxyandgetProxyAgentcorrectly wrap the new Effect-based helpersThe effectful implementations—
matchesNoProxyEffect(with graceful fallback onUrlParseError) andgetProxyAgentEffect(respecting noProxy and protocol-specific proxy lists)—mirror prior behavior while centralizing parsing and validation; the exported wrappers usingEffect.runSynckeep the public API synchronous for existing callers.You might later consolidate the proxy-selection logic (HTTP vs HTTPS vs fallback) into a small helper to ease testing, but it’s fine as-is.
Also applies to: 278-310
414-483:fetchWithProxyEffect-based redirect loop appears correct and boundedThe new
fetchWithProxyusesprepareFetchOptions+buildFetchOptions, resolves an agent viagetProxyAgentEffect, validates each hop viavalidateUrlEffect, and handles redirects manually with a clearmaxRedirectslimit and aFetchErroron exhaustion; non-redirect responses or missingLocationheaders correctly terminate the loop.If you expect many redirects, you might consider logging when
maxRedirectsis hit to distinguish “too many redirects” from otherFetchErrorcases in your metrics.
157-160:UrlValidationResulttype is unused and should be removedThe exported
UrlValidationResulttype (lines 157–160) is never referenced anywhere in the codebase. ThevalidateUrlfunction correctly returnsPromise<Either<UrlValidationError, URL>>via the Effect pipeline, which is the actual contract being used by callers invideoWorker.tsandcrawlerWorker.ts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/workers/network.ts(4 hunks)apps/workers/package.json(1 hunks)apps/workers/workers/crawlerWorker.ts(3 hunks)apps/workers/workers/videoWorker.ts(2 hunks)package.json(2 hunks)tooling/typescript/base.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx,json,css,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier according to project standards
Files:
package.jsontooling/typescript/base.jsonapps/workers/workers/videoWorker.tsapps/workers/workers/crawlerWorker.tsapps/workers/package.jsonapps/workers/network.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for type safety in all source files
Files:
apps/workers/workers/videoWorker.tsapps/workers/workers/crawlerWorker.tsapps/workers/network.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Lint code using oxlint and fix issues with
pnpm lint:fix
Files:
apps/workers/workers/videoWorker.tsapps/workers/workers/crawlerWorker.tsapps/workers/network.ts
🧠 Learnings (1)
📚 Learning: 2025-11-27T16:02:47.432Z
Learnt from: CR
Repo: karakeep-app/karakeep PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:02:47.432Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Lint code using oxlint and fix issues with `pnpm lint:fix`
Applied to files:
package.json
🧬 Code graph analysis (1)
apps/workers/network.ts (1)
packages/trpc/index.ts (1)
Context(22-28)
🪛 Biome (2.1.2)
package.json
[error] 7-7: The key prepare was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: tests
- GitHub Check: typecheck
🔇 Additional comments (4)
package.json (1)
30-30: New@effect/language-servicedevDependency looks appropriateAdding
@effect/language-servicehere aligns with the new TS plugin configuration and Effect-based code in workers; no issues from this change alone.Please just confirm locally that
pnpm installplustsserver/IDE pick up the plugin without additional configuration.tooling/typescript/base.json (1)
18-23: TS config changes for Effect plugin look consistentSetting
noUncheckedIndexedAccessexplicitly and wiring@effect/language-serviceintocompilerOptions.pluginsis coherent with the new devDependency; no issues from a tooling standpoint.Please confirm in your environment that
tsc --showConfig(or your IDE) reflects the plugin and that diagnostics from the Effect language service work as expected.apps/workers/package.json (1)
22-22: Addingeffectruntime dependency is appropriateIncluding
effect@^3.19.8in workers’ dependencies matches the new Effect-based networking and validation code.After installing, please verify
pnpm-lock.yaml(or equivalent) is updated and thatpnpm --filter @karakeep/workers test/typecheckrun clean against this version.apps/workers/network.ts (1)
3-13: Error modeling withSchema.TaggedErrorandparseUrlis well-structuredThe introduction of
FetchError,UrlValidationError,UrlParseError, andHostAddressResolutionErrorplus theparseUrlhelper gives you a clear, typed error taxonomy around DNS, URL parsing, validation, and fetch failures; theEffect.tryusage cleanly wrapsnew URL(...)into a tagged error path.Please ensure downstream code (e.g., logging/metrics) treats these tagged errors as expected (e.g., by
_tagormessage) and doesn’t rely on older string-based reasons.Also applies to: 20-50
| "version": "0.1.0", | ||
| "private": true, | ||
| "scripts": { | ||
| "prepare": "effect-language-service patch", |
There was a problem hiding this comment.
Duplicate prepare script means the new command never runs
You now have two prepare keys (lines 7 and 17); only the last one (husky) is honored, so effect-language-service patch is effectively disabled. Consider merging into a single script, e.g.:
- "scripts": {
- "prepare": "effect-language-service patch",
+ "scripts": {
+ "prepare": "husky && effect-language-service patch",(or the opposite order, depending on which you prefer to run first).
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (2.1.2)
[error] 7-7: The key prepare was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
🤖 Prompt for AI Agents
In package.json around line 7, there are duplicate "prepare" scripts so only the
later husky script is executed and "effect-language-service patch" is skipped;
remove the duplicate key and merge the two commands into a single "prepare"
script that runs both commands in the desired order (e.g., run
effect-language-service patch first, then husky install) by joining them with a
shell operator (&&) or using a task runner, and delete the extra "prepare" entry
so only the combined script remains.
No description provided.