R&D: browserbase for the web resources...#170
R&D: browserbase for the web resources...#170bmdavis419 wants to merge 2 commits intodavis/web-resourcefrom
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
| /// <reference path="../../types/turndown-plugin-gfm.d.ts" /> | ||
|
|
||
| import { promises as fs } from 'node:fs'; |
There was a problem hiding this comment.
Bad type reference path
This /// <reference path="../../types/turndown-plugin-gfm.d.ts" /> points to apps/server/src/types/..., but from apps/server/src/resources/impls/website.ts the relative path should be ../../types/...? Actually resources/impls -> resources -> src so ../../types resolves to apps/server/src/resources/types, not apps/server/src/types. This will break TS type resolution for turndown-plugin-gfm in this file.
| /// <reference path="../../types/turndown-plugin-gfm.d.ts" /> | |
| import { promises as fs } from 'node:fs'; | |
| /// <reference path="../../types/turndown-plugin-gfm.d.ts" /> |
Adjust the path so it resolves to apps/server/src/types/turndown-plugin-gfm.d.ts (or add this declaration to a location included by TS without a triple-slash reference).
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/resources/impls/website.ts
Line: 1:3
Comment:
**Bad type reference path**
This `/// <reference path="../../types/turndown-plugin-gfm.d.ts" />` points to `apps/server/src/types/...`, but from `apps/server/src/resources/impls/website.ts` the relative path should be `../../types/...`? Actually `resources/impls` -> `resources` -> `src` so `../../types` resolves to `apps/server/src/resources/types`, not `apps/server/src/types`. This will break TS type resolution for `turndown-plugin-gfm` in this file.
```suggestion
/// <reference path="../../types/turndown-plugin-gfm.d.ts" />
```
Adjust the path so it resolves to `apps/server/src/types/turndown-plugin-gfm.d.ts` (or add this declaration to a location included by TS without a triple-slash reference).
How can I resolve this? If you propose a fix, please make it concise.| shell: false | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Render budget leaks
args.renderState.remaining -= 1 happens before confirming the render actually succeeded and produced a usable result; if renderer.render() throws or returns a cross-origin finalUrl (you return null), you still burn budget. In crawls with intermittent render failures, this will stop rendering much earlier than intended.
Consider only decrementing remaining after a successful render that passes the origin check (and/or only after you decide to accept the rendered snapshot).
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/resources/impls/website.ts
Line: 718:720
Comment:
**Render budget leaks**
`args.renderState.remaining -= 1` happens before confirming the render actually succeeded and produced a usable result; if `renderer.render()` throws or returns a cross-origin `finalUrl` (you return `null`), you still burn budget. In crawls with intermittent render failures, this will stop rendering much earlier than intended.
Consider only decrementing `remaining` after a successful render that passes the origin check (and/or only after you decide to accept the rendered snapshot).
How can I resolve this? If you propose a fix, please make it concise.| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Test detection can misfire
process.argv.includes('test') will treat any run whose argv contains the string test as a test run and disable renderer auto-loading. That can inadvertently disable rendering in normal execution (e.g. a path containing test or a user-provided arg). Prefer relying on Bun.env.BUN_TEST (or a dedicated config flag) instead of argv substring matching.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/resources/impls/website.ts
Line: 1086:1088
Comment:
**Test detection can misfire**
`process.argv.includes('test')` will treat any run whose argv contains the string `test` as a test run and disable renderer auto-loading. That can inadvertently disable rendering in normal execution (e.g. a path containing `test` or a user-provided arg). Prefer relying on `Bun.env.BUN_TEST` (or a dedicated config flag) instead of argv substring matching.
How can I resolve this? If you propose a fix, please make it concise.
Greptile Overview
Greptile Summary
@btca/bbto integrate Browserbase + puppeteer-core as an optional HTML renderer.Result.isError/manual throws.Confidence Score: 3/5
apps/server/src/resources/impls/website.tshas a broken triple-slash reference path (can break typechecking) and render budget/test-run detection logic that will behave incorrectly in real runs.Important Files Changed