Conversation
|
Updated 1:24 PM PT - Jul 2nd, 2026
❌ @robobun, your commit 65caf03 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30755That installs a local version of the PR into your bun-30755 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesWebView Navigation API Rename
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — types-only rename matching the runtime's goBack/goForward, with a fixture to lock it in.
Extended reasoning...
Overview
This PR renames two method declarations in the Bun.WebView interface in packages/bun-types/bun.d.ts from back()/forward() to goBack()/goForward(), and adds a new type-fixture file test/integration/bun-types/fixture/webview.ts that asserts the corrected names type-check as Promise<void>.
Verification
I confirmed the runtime prototype table in src/runtime/webview/JSWebViewPrototype.cpp (lines 55-56) registers the JS-visible methods as "goBack"_s and "goForward"_s, and all backend code (JSWebView.cpp, ChromeBackend, WebKitBackend, WebViewHost) uses the same naming. The .d.ts was the only divergent location, so the rename is correct. The new fixture also references view.navigate() and view.reload(), both of which exist in the declaration file, and it uses the same expectType(...).is<...>() helper from ./utilities that other fixtures in the directory use.
Security risks
None. This is a pure ambient type-declaration change plus a compile-time-only test fixture (declare const view). No runtime code paths, auth, crypto, or I/O are touched.
Level of scrutiny
Low. The change is mechanical (two identifier renames in a .d.ts), self-contained, and directly verifiable against the C++ prototype registration. The added fixture is regression coverage that would fail the bun-types integration test if the names ever drift again.
Other factors
No bugs were reported by the bug-hunting system, there are no prior reviewer comments to address, and the PR timeline contains only the CI build notification. The fixture follows existing conventions in test/integration/bun-types/fixture/.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/integration/bun-types/bun-types.test.ts`:
- Around line 551-564: The test validates goBack() and goForward() return
Promise<void> but doesn't assert that the old names back() and forward() are
rejected, so reintroducing them wouldn't fail the test; update the test file
(the typeTest case for "goBack() and goForward() exist and return Promise<void>"
in webview-30754.ts) to add negative type assertions using `@ts-expect-error`
before calling view.back() and view.forward() to ensure those methods produce
type errors, leaving the existing expectType checks for view.goBack() and
view.goForward() unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e6bbbc9-db81-4346-9480-80803fa9e56f
📒 Files selected for processing (1)
test/integration/bun-types/bun-types.test.ts
There was a problem hiding this comment.
LGTM — types-only rename matching the runtime, with a regression test.
Extended reasoning...
Overview
This PR renames two method declarations on Bun.WebView in packages/bun-types/bun.d.ts from back()/forward() to goBack()/goForward(), and adds a typeTest block in test/integration/bun-types/bun-types.test.ts that pins the corrected names. I verified the runtime in src/runtime/webview/JSWebViewPrototype.cpp registers "goBack"/"goForward" on the prototype and that test/js/bun/webview/webview-chrome.test.ts already calls them, so the .d.ts was the only place out of sync.
Security risks
None. This is a pure TypeScript ambient declaration rename plus a type-level test fixture; no runtime code, auth, crypto, or I/O is touched.
Level of scrutiny
Low. The change is mechanical (two identifier renames in a .d.ts), self-contained, and the added test follows the exact typeTest pattern already used elsewhere in the same file (e.g., the bun:bundle feature() and Bunland JSX blocks), including the expectType(...).is<...>() helper and filtering diagnostics by file prefix.
Other factors
The robobun CI failures are all unrelated Rust lint/build issues (HiveArrayFallback deprecations, unused imports, unreachable pub items, musl build agent failure) in files this PR does not touch — a .d.ts/test-only change cannot cause those, so they are pre-existing on the base branch. No human reviewer comments are outstanding, and the bug-hunting system found no issues.
|
just a follow up, will this be included in the next release? |
|
Good question. One thing worth knowing in the meantime: the runtime already ships The PR is open and waiting on a maintainer to review and merge. Once it lands, the corrected types go out with the next |
50ab9ee to
5dcb86e
Compare
There was a problem hiding this comment.
LGTM — types-only rename to match the runtime's goBack()/goForward(), with a regression type-test.
Extended reasoning...
Overview
This PR renames two method declarations on Bun.WebView in packages/bun-types/bun.d.ts from back()/forward() to goBack()/goForward(), and adds a typeTest case in test/integration/bun-types/bun-types.test.ts that pins the names (positive expectType checks plus @ts-expect-error on the legacy names). I verified against the runtime: src/runtime/webview/JSWebViewPrototype.cpp registers "goBack" and "goForward" on the prototype, so the .d.ts was the only place that disagreed. No native code changes.
Security risks
None. This is a pure TypeScript declaration rename plus a type-level test fixture. There is no runtime behavior change, no auth/crypto/permissions surface, and no data handling.
Level of scrutiny
Low. The change is mechanical (two identifiers renamed to match an already-shipping runtime API) and self-contained. The added test follows the established typeTest pattern used throughout the same file and filters diagnostics to its own fixture, so it cannot perturb other cases. The one Buildkite failure is in build-rust, which this PR does not touch and is unrelated to a .d.ts/test-only change.
Other factors
CodeRabbit's only suggestion (negative assertions via @ts-expect-error) was applied in 5dcb86e and the thread is resolved. No CODEOWNERS entry covers packages/bun-types or this test file. No prior review from me on this PR. The bug-hunting system found no issues.
14374c2 to
110cdf5
Compare
207be72 to
b070b10
Compare
|
@robobun instead of test/integration/bun-types/webview-goback.test.ts, delete it and just update the bun-types integration test. ignore the gate failure |
The runtime registers goBack/goForward on the WebView prototype (src/runtime/webview/JSWebViewPrototype.cpp:55-56), but the type declarations advertised back/forward. Users trusting the types hit "TypeError: view.back is not a function" at runtime, while the actual goBack() call reported a missing property in the IDE. Fixes #30754
eeb571b to
661743c
Compare
|
Done. Deleted Also rebased onto current |
There was a problem hiding this comment.
LGTM — the two-line .d.ts rename and the typeTest case both look correct; just re-sync the PR description's Verification section per the inline note before merge.
Extended reasoning...
Overview
This PR renames two method declarations in packages/bun-types/bun.d.ts (back()→goBack(), forward()→goForward()) so the published types match what Bun.WebView actually registers on its prototype in JSWebViewPrototype.cpp. It also adds a typeTest case to test/integration/bun-types/bun-types.test.ts that pins the new names via expectType(...).is<Promise<void>>() and rejects the legacy names via @ts-expect-error. Two files, ~30 lines total.
Security risks
None. This is a type-declaration-only change with no runtime code path, no native code, no I/O, no external input handling.
Level of scrutiny
Low. The .d.ts edit is a two-identifier rename whose correctness is verifiable by grepping the C++ prototype table cited in the description. The test addition is a copy of the established typeTest pattern used by the immediately preceding bun:bundle feature() block in the same file (same files: fixture shape, same expectedEmptyInterfacesWhenNoDOM, same diagnostics.filter(d => d.line?.startsWith(...)) → toEqual([]) assertion). No new mechanism, no new harness.
Other factors
A maintainer reviewed on 2026-07-02 and requested folding the standalone test into bun-types.test.ts; that was done exactly as asked, and the author confirmed the suite passes (13/13) and fails when the rename is reverted. All prior review threads (CodeRabbit's negative-assertion request, my earlier Windows-symlink / stderr / stale-description notes on the since-deleted standalone test) are resolved and no longer apply to the current diff.
The one remaining finding is a metadata nit: the PR description's Verification section still describes the deleted webview-goback.test.ts / workflow wiring rather than the current typeTest in bun-types.test.ts. That's flagged inline and is a description-only edit — it doesn't affect the correctness of what merges into the tree, so I'm approving the code while surfacing it.
Adds a bun-types fixture that the integration suite type-checks: it asserts goBack()/goForward() resolve as Promise<void>, and marks back()/forward() with @ts-expect-error so reintroducing either legacy name fails the typecheck.
661743c to
65caf03
Compare
There was a problem hiding this comment.
LGTM — types-only rename matching the runtime, with the fixture placed where alii asked.
Extended reasoning...
Overview
Two-file change: renames back()/forward() to goBack()/goForward() on Bun.WebView in packages/bun-types/bun.d.ts, and adds test/integration/bun-types/fixture/webview.ts following the existing fixture pattern (imports expectType from ./utilities, asserts return types, and uses @ts-expect-error to lock out the legacy names). Verified against src/runtime/webview/WebViewHost.cpp and ObjCRuntime.cpp — the runtime does register goBack/goForward, so the .d.ts was the only deviation.
Security risks
None. Type declarations only; no runtime, auth, crypto, or permission code touched.
Level of scrutiny
Low. This is a mechanical .d.ts fix to match already-shipped runtime behavior, and per CLAUDE.md, packages/bun-types/**/*.d.ts edits don't touch compiled code. The fixture is picked up automatically by bun-types.test.ts (which copies the whole fixture/ dir into its temp project), so no wiring changes are needed.
Other factors
The PR went through several test-shape iterations; the maintainer's final request ("it should be in the fixture dir, not as a new test") was implemented in 65caf03, and all prior review threads (mine and CodeRabbit's) are resolved. The PR description's Verification section now accurately describes the fixture approach. No bugs were found by the bug hunter on the current revision.
|
Superseded by #39608, which fixes all of the open |
3 participants
Fixes #30754
What
Bun.WebViewregistersgoBack/goForwardon its prototype, but thetype declarations in
@types/bunadvertised them asback/forward.Following the types got you
TypeError: view.back is not a functionatruntime; calling the real
view.goBack()got you a "property does notexist" error in the IDE.
Cause
Method registration in
src/runtime/webview/JSWebViewPrototype.cpp:{ "goBack"_s, ..., jsWebViewProtoFuncBack, 0 }, { "goForward"_s, ..., jsWebViewProtoFuncForward, 0 },All internal C++ code (
WebViewHost::goBack,CDP::Ops::goBack,WK::Ops::goBack) and existing tests (webview-chrome.test.ts) usegoBack/goForward. The.d.tswas the only place that deviated.Fix
Rename the two methods in
packages/bun-types/bun.d.tsto match theruntime. This is a types-only change, no native behavior changes.
Verification
Adds
test/integration/bun-types/fixture/webview.ts, which the bun-typesintegration suite (
test/integration/bun-types/bun-types.test.ts)type-checks along with the other fixtures. It asserts
goBack()/goForward()resolve asPromise<void>, and marksback()/forward()with
@ts-expect-errorso reintroducing either legacy name also fails.Reverting the
bun.d.tsrename makes the fixture fail to type-check(
goBack/goForwarderror with TS2551, and the@ts-expect-errordirectives become unused, TS2578), so the suite turns red.