Skip to content

feat(browser): add optional native title-bar bridge - #64

Merged
kshivang merged 3 commits into
mainfrom
feature/native-browser-titlebar
Sep 26, 2026
Merged

kshivang merged 3 commits into
mainfrom
feature/native-browser-titlebar

Conversation

@kshivang

@kshivang kshivang commented Sep 26, 2026 •

Copy link
Copy Markdown
Collaborator

Adds an optional browser/title-bar handshake so a host can present navigation, URL editing, autocomplete, and the browser's existing suggestion content in native chrome. The browser retains its in-pane toolbar until its window or handle is explicitly hosted; unhosting restores the fallback.

The new API types are additive and UI-thread scoped. Handle publications use owner identity so disposal of an old composition cannot unregister its replacement.

Validation: ./gradlew build passed, including apiCheck and 120 tests. The committed ABI dump includes the new declarations.

Release this before the dependent Fluck Browser and BossConsole PRs. main remains version 1.0.93 because the release workflow increments before publishing; this change is intended for 1.0.94. Recheck that version if another API release lands first.

Host integration: risa-labs-inc/BossConsole#1736. Merge the API release before the browser and host integrations.

@claude

claude Bot commented Sep 26, 2026

Copy link
Copy Markdown

Review: optional native title-bar bridge

The change is small and purely additive. The ABI dump is regenerated, the version is changed only in the build.gradle.kts changelog comment (version correctly stays at 1.0.93 because the release workflow bumps it), plugin.json is untouched, the file ends with a newline, and it only uses Compose Multiplatform runtime APIs. Using owner identity so an old composition's remove can't unregister its replacement is a good choice. My main concerns are about the cross-classloader contract and lifecycle, which this repo's AGENTS.md treats as the hard part.

Potential bugs / design issues

1. The singleton only works if host and plugin resolve the same Class. Please confirm and document which copy runs.
BrowserTitleBarBridge is a stateful object. The handshake works only if the browser plugin and BossConsole see the same instance. BossOverlayHost documents this explicitly: which copy executes, which package is parent-first shared, and which gate a plugin needs. Here, if BossConsole's dependent PR compiles against its own copy while the plugin gets its copy from the ApiClassLoader, or the reverse after an api hot-swap, you get two separate maps. The browser then keeps its in-pane toolbar forever, and nothing errors. Please:

  • confirm whether ai.rever.boss.plugin.browser. is in the host's shared, parent-first packages, and which copy is authoritative once BossConsole compiles this in;
  • add a KDoc block like the one on BossOverlayHost saying which copy runs on an old host (the api jar, where host() is never called, so the fallback toolbar stays, which is the right degradation) and on a new host;
  • if BossConsole will compile this type in, mark BrowserTitleBarBridge @HostImplemented. After that, any member change needs a host release and minBossVersion, and future contributors should see that.

2. remove() also drops the host's claim (hosts.remove(handleId)).
When a DisposableEffect key changes, or the browser composition is recreated for the same handle, the old owner's onDispose usually runs before the new owner's publish. In that order the owner check passes, and the host's focus callback is deleted even though the host still shows native chrome for that handle. The new composition then sees isHosted == false and draws its fallback toolbar as well, until the host happens to call host() again. Either keep the hosts entry independent of browser publications (the host owns it and clears it with host(id, null)), or state clearly that the host must re-claim after every re-publish. The first option seems more robust.

3. Lifecycle leak on plugin unload or hot-swap.
The global maps hold strong references to owner, to BrowserTitleBarState lambdas (which capture browser/Compose objects), and to @Composable suggestion content. All of these belong to the plugin classloader. If a plugin is unloaded without every composition running its onDispose (crash, forced unload, the api-swap unload-all), the entries pin the old PluginClassLoader. Consider a removeAll(owner) or a per-plugin clear, and say in the KDoc that callers must pair every publish with remove in onDispose.

4. onCommand: (String) -> Unit is a stringly typed protocol with no documented vocabulary.
The host and browser live in different repos, so the valid command strings (Enter, Escape, Up/Down, Tab…?) are the real contract. It should be pinned in this API with constants or KDoc listing the values. Otherwise the two sides can drift silently, and adding a command later is invisible to the ABI check.

API evolution

  • BrowserAddressBarState has no defaulted parameters, and BrowserTitleBarState defaults only the last two. Adding a field later changes the constructor signature, and the validator rejects any plugin built against the old one as a whole. Consider leaving an evolution path now, for example a KDoc note that new fields must come with a secondary constructor that keeps the old signature, or construction through a builder/factory. This is cheap to do before the first release and expensive afterwards (see the note on BossOverlayHost.modalRenderer about widening signatures after release).
  • revision: Int and hasSelectedSuggestion have no documented semantics. What does the host do when revision changes? Does it reset its text field selection? One line each would help the BossConsole implementer.

Code quality (minor)

  • windows is mutableStateMapOf<String, Boolean>(), but it only ever stores true. A snapshot-state set (for example mutableStateMapOf<String, Unit>()) matches its meaning better, while hostWindow(id, enabled) keeps its Boolean API.
  • private val hosts is declared between methods. Group it with entries and windows at the top.
  • The file declares BrowserAddressBarState after the bridge that uses it, and neither the bridge's public functions nor the relationship between windowId and handleId have KDoc. Please add a short paragraph explaining when a browser should hide its toolbar (isWindowHosted(window) || isHosted(handle)?) and who calls which function (host: hostWindow/host/focus; plugin: publish/remove/isHosted).
  • "UI-thread-only" is not enforced. That's fine for snapshot state, but mention that focus() runs the callback synchronously on the caller's thread.

Test coverage

No tests are added, although the ownership logic is pure and easy to pin. This repo already tests similar contracts (OverlayRoutingTest, BrowserMenuContextContractTest). Suggested cases:

  • remove with a stale owner after a new publish leaves the new entry in place;
  • remove with the current owner clears the state (and pins whatever you decide for hosts in point 2);
  • host(id, null) un-hosts, and focus returns false for an unhosted handle and true (running the callback) for a hosted one;
  • hostWindow(id, false) clears the flag.

Security / performance

No security concerns: it's in-process state with no I/O. Performance is fine. Snapshot maps mean host chrome recomposes on each publish, so publishers should avoid re-publishing identical state every frame. Because the state classes are not data classes there is no structural equality, and every publish is a change.

Summary

Points 1–3 should be resolved or explicitly documented before release, since this is the first release of the type and later changes need coordinated releases. Point 1 is the one that could make the whole feature silently fail. The rest is polish. The release-ordering note in the description (this lands before the Fluck Browser and BossConsole PRs, targeting 1.0.94) looks right.

@claude

claude Bot commented Sep 26, 2026

Copy link
Copy Markdown

Review: feat(browser): add optional native title-bar bridge

This is a small, additive change and it follows the repo conventions: version touched only in build.gradle.kts, plugin.json left alone, ABI dump regenerated, new file ends with a newline, and it imports only Compose Multiplatform runtime. Owner identity on publish/remove is a good idea. My main concern is that the ABI shape will be hard to evolve. There are also a few ownership edge cases.

1. Constructor-shaped state classes will freeze on the first release (evolution risk)

BrowserTitleBarState (12 params) and BrowserAddressBarState (12 params, no defaults at all) are plain classes whose whole contract is the primary constructor. The ABI dump pins exactly one JVM signature per class. For example:

<init>(Ljava/lang/String;IILjava/lang/String;ZZILkotlin/jvm/functions/Function3;...)V

Adding any field later (a canShare, a security/lock indicator, a favicon, a zoom...) changes that signature. Old callers then get NoSuchMethodError, the validator rejects them, and apiCheck fails. That's the same trap AGENTS.md describes for data classes ("Never evolve ... data classes across the boundary"). A title-bar model feels very likely to grow.

Options, in rough order of preference:

  • Make them interfaces with default members (val share: (() -> Unit)? get() = null). The browser implements them and the host reads them. New members get default bodies, which is exactly the additive rule this repo already enforces.
  • Or keep the classes but add an explicit forward-compat hatch now, like AiModelPricing.extras from 1.0.92. You'd also need to commit to adding secondary constructors rather than touching the primary one.

This PR is the cheapest point to decide, because after 1.0.94 ships the shape is permanent.

2. remove() clears the host's registration, which is owned by someone else

fun remove(handleId: String, owner: Any) {
    if (entries[handleId]?.owner === owner) {
        entries.remove(handleId)
        hosts.remove(handleId)   // <- host-owned, removed on the plugin's owner token
    }
}

entries is owned by the browser plugin, but hosts is owned by the host window (it has its own owner token in host(handleId, owner, focus)). If the browser composition is disposed and recreated for the same handle (pane move, split, recomposition keyed differently), and the host doesn't re-call host(...), then isHosted() goes false. The browser shows its in-pane toolbar again while native chrome may still be showing, or the reverse. That breaks the PR's own guarantee that "a disposed window cannot unregister the focus handler of a newer host". Suggestions:

  • Leave hosts alone in remove() and let the host unregister with its own owner. Or document clearly that the host must re-assert host() whenever state(handleId) goes from null to non-null.
  • Either way, a unit test for "publish A → publish B → remove A keeps B" and "remove entry doesn't drop host" would pin this down.

3. The "legacy" overload on a brand-new type

host(handleId, focus) uses one shared legacyHostOwner, so every 2-arg caller can unregister every other 2-arg caller's handler (host(id, null) removes whatever legacy host is there). Nothing is legacy yet because this type doesn't exist on main. If the dependent BossConsole PR was written against an earlier draft, I'd update it to the owner-aware overload and drop this one before it becomes permanent ABI. Once released it can never be removed.

4. Any plugin can overwrite any handle's state (URL spoofing in trusted chrome)

publish() overwrites unconditionally, and BrowserTitleBarBridge is a singleton in the shared ApiClassLoader that every plugin can see. So any loaded plugin that knows or guesses a handleId can replace the URL, the navigate callback, and the @Composable suggestions slot that the host draws in native window chrome. Users tend to trust the title-bar URL more than in-pane content, which makes it a phishing surface. The plugin model is largely trusted today, so this isn't a blocker, but consider:

  • first-publisher-wins: reject publish when a different owner already holds the handle, unless it has been removed; or
  • having the host check that the publisher is the plugin that owns the BrowserHandle (for example, route publication through the handle or PluginContext instead of a global object).

5. Host/plugin singleton identity: please confirm the loading path

The handshake only works if the host and the browser plugin see the same BrowserTitleBarBridge class and so the same mutableStateMapOfs. Before BossConsole pins 1.0.94, the host can only reach it through the ApiClassLoader. After the pin, the host's compiled-in copy is resolved parent-first. Both paths should yield one instance, but it's worth a quick check in the BossConsole PR that the api package isn't also bundled into the host's own classloader in a way that creates a second copy. Because the host will compile against this object, consider marking it @HostImplemented (as BrowserHandle is). Future member changes would then be flagged as needing a host release and minBossVersion.

6. Minor / readability

  • Declaration order in the object is interleaved: hostWindow/windows sit between entries and the Host/hosts declarations. Grouping (state entries → hosts → windows) would read more easily. BrowserAddressBarState is declared after the object that references it only indirectly. Moving it next to BrowserTitleBarState would help.
  • windows stores only true, so it's really a set. That's fine, but isWindowHosted could just be windowId in windows.
  • focus() calls a foreign lambda with no guard. If a host's focus handler throws, the exception propagates into the browser plugin's key handling. A runCatching there is cheap insurance.
  • The KDoc says "UI-thread-only" but nothing enforces it. Fine for now, but it's worth saying which thread each lambda (navigate, onEdit, suggestions, ...) is invoked on from the host side.
  • The revision field's semantics aren't documented. Is it monotonic per edit, and does the host use it to discard stale selection updates? A one-line comment would help implementers on both sides.
  • Changelog: the other entries explain why the shape is what it is and which evolution rules apply ("New types only: ApiClassLoader/minApiVersion, no host release"). A sentence here on the gating (minApiVersion 1.0.94 for the browser, host pin for BossConsole) would keep that consistent. (Separately, main has no 1.0.93 entry. That isn't this PR's fault, but it's easy to fix alongside.)

Test coverage

No tests were added. The ownership logic is pure and easy to unit-test, and src/test/kotlin/ai/rever/boss/plugin/browser/ already has contract tests. Suggested cases:

  • publish(owner A) → publish(owner B) → remove(A) keeps B's state
  • host(owner X) → host(owner Y) → host(X, null) keeps Y
  • remove on a non-matching owner is a no-op
  • focus() returns false when unhosted, and true plus invokes the handler when hosted
  • hostWindow enable/disable round-trip

Performance

Negligible. Snapshot-state maps keyed by handle id are the right tool here, and reads recompose only the observers of the specific keys.


Summary: The direction is good. Before merging, I'd resolve #1 (evolvable shape) and #3 (drop the legacy overload), because both become permanent ABI the moment 1.0.94 is released. I'd also fix or document #2. #4–#6 are follow-ups.

@claude

claude Bot commented Sep 26, 2026

Copy link
Copy Markdown

Review: optional native title-bar bridge

The design holds up. Using owner identity on publish/remove and on the owner-aware host overload is the right fix for the race where an old composition, on dispose, removes its replacement. The tests cover it. The changes are additive, the ABI dump is committed, the version is edited only in build.gradle.kts (the changelog comment is there and plugin.json is untouched), and both new files end with a newline. My comments are below. Most of them matter because this is a @HostImplemented object: once 1.0.94 ships, its members are frozen until the next BossConsole release.

Worth settling before release (hard to undo later)

  1. Drop the "legacy" host(handleId, focus) overload. This API is brand new, so nothing needs the legacy form yet. Every caller of that overload shares one legacyHostOwner, which quietly turns off the owner protection the other overload exists to provide. Two windows that both use it will race each other on disposal in exactly the way this PR is meant to prevent. Because the object is @HostImplemented, removing the overload later would take a host release. I recommend shipping only the owner-aware host(handleId, owner, focus) and updating the first test to use it.

  2. hostWindow(windowId, enabled) has no owner, unlike everything else here. An old window cleanup (hostWindow(id, false)) will unhost a newer claim on the same id. If window ids are guaranteed unique for the process lifetime, say so in the KDoc. If not, add an owner parameter now, for the same reason as point 1.

  3. Address-bar commands are plain strings. "submit", "next", "previous", "accept", "right", "cancel" and "delete" are only listed in KDoc, so a typo on either side gets silently ignored. A small new object BrowserAddressBarCommands { const val SUBMIT = "submit"; ... } is additive, and because const values are inlined it costs nothing at the classloader boundary. Future commands can be added to it without touching the host-implemented type.

Correctness and robustness

  1. Plugin unload can leak its classloader. On a newer host the singleton lives in the host classloader. Every Entry holds lambdas (navigate, onEdit, the @Composable suggestions, and so on) that capture the browser plugin classes. If the browser skips remove during unload (a crash in dispose, or a composition that never gets disposed during a hot-swap), the whole plugin classloader stays pinned. The KDoc places this on plugins. It would be more robust for the host integration (BossConsole#1736) to also clear entries on plugin unload, for example by keying on the owner classloader, or at least to mention it in the host PR.

  2. The @Composable () -> Unit field in BrowserAddressBarState means the host composes browser-plugin UI inside its own composition. That only works while the host and the plugin share a single Compose runtime and compatible Compose compiler ABIs (the dump shows it as Function2 of (Composer, Int)). That is true today, but it is an implicit contract. A sentence in the KDoc would help anyone upgrading Compose later.

  3. navigate(String) takes text the host collected. The browser side, not the bridge, should keep applying its usual URL and scheme handling (javascript:, file:, etc.) to this input, just as it does for its in-pane bar. This is a note for the Fluck Browser PR; nothing needs to change here.

  4. Threading. Writing to mutableStateMapOf from a background thread outside a snapshot will not crash, but it can race. The "UI-thread-only" KDoc is the right contract. Consider stating it on publish/remove/host individually as well, since those are the entry points people will read.

Code quality nits

  • Inside the object, the private Host class, hosts, and legacyHostOwner are declared between public functions. Grouping all private state at the top would make the object easier to scan.
  • BrowserAddressBarState is declared after the bridge that references it, while BrowserTitleBarState comes first. Putting both state classes before the bridge would read more naturally.
  • BrowserTitleBarState has default arguments, but BrowserAddressBarState has none. That is fine, but the "add compatible overloads" rule now covers the synthetic $default constructor as well. Keep that in mind when extending it.
  • There is a trailing blank line before the closing } in the test class.

Test coverage

  • Nothing tests hostWindow or isWindowHosted. It would be good to cover enable/disable and the fact that it is independent of handle hosting.
  • Nothing tests how the legacy and owner-aware host overloads interact, for example that a legacy host(id, null) does not remove an owner-based registration. This goes away if you take point 1.
  • In the first test, the legacy host registration is not cleaned up in finally if an assertion fails partway through. That is harmless today because ids are unique per test, but it is easy to fix.
  • The tests run against a process-global singleton. Unique ids per test keep them isolated, and it would help to keep that convention (maybe with a comment) as more tests are added.

Summary

The approach is solid and the change is additive. I recommend resolving points 1-3 before cutting 1.0.94, because after that the shape of this @HostImplemented object can only change with a host release. The rest can go in follow-ups or the dependent PRs. As the description says, check the 1.0.94 target again if another API release merges first.

@kshivang
kshivang merged commit 4f04644 into main Sep 26, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant