Skip to content

feat(api): bridge terminal titlebar actions - #65

Merged
kshivang merged 5 commits into
mainfrom
feature/terminal-titlebar-actions
Sep 28, 2026
Merged

kshivang merged 5 commits into
mainfrom
feature/terminal-titlebar-actions

Conversation

@kshivang

@kshivang kshivang commented Sep 27, 2026 •

Copy link
Copy Markdown
Collaborator

Add an optional UI-thread terminal titlebar bridge. Hosts claim individual windows; plugins register a window-content provider that owns controls and dialogs independently of terminal composition. Provider identity and window ID key composition lifetime, while owner tokens isolate published actions and disposal clears them. Unclaimed windows retain existing floating controls.

The bridge also carries owner-scoped, window-specific live-call content. The host can place it in its existing overlay layer; removing the publisher clears the call panel without affecting the other titlebar actions.

This adds new types in the shared UI package without changing existing API constructors or interfaces. BossConsole carries the matching parent-first implementation. Release source remains 1.0.94 because release CI increments before publishing 1.0.95. Local next-version builds use -PlocalApiVersion=1.0.95. Host-implemented types are annotated and document host-version gating.

Owners use identity and explicit publication order; unchanged effects preserve ownership, and provider/call-panel composition keys isolate state across owners. Validation: apiCheck, test, and buildPluginJar pass, including three bridge regression tests for arbitration, removal, and composition state.

@claude

claude Bot commented Sep 27, 2026

Copy link
Copy Markdown

Review: terminal titlebar action bridge

This is a small, additive change, and it follows the same design as BrowserTitleBarBridge. The new types are only added, existing ones are untouched. The version is changed only in build.gradle.kts, the file ends with a newline, and it uses only Compose Multiplatform APIs. There are a few things to fix before merging, most important first.

1. 🔴 The API dump is not updated, so apiCheck / CI will probably fail

This PR adds two public types (TerminalTitleBarAction and TerminalTitleBarBridge), but api/boss-plugin-api.api is unchanged. The binary-compatibility-validator fails ./gradlew build when public declarations are missing from the dump. Run ./gradlew apiDump and commit the result. I couldn't run apiCheck locally in this review, so please check that CI passes.

2. 🟠 actions() picks an arbitrary entry when a window has more than one active entry

entries.values.lastOrNull { it.windowId == windowId && it.active }

mutableStateMapOf is a SnapshotStateMap, which is backed by a persistent hash map. Its iteration order is not insertion order, so "last" has no meaning here. Also, calling publish again for an owner that already exists doesn't move that entry anywhere. As a result, whenever two active entries share a windowId, the titlebar may show either terminal's actions, and the choice can change as the map changes. This can happen when:

  • two terminals are visible in a split view and both report active, or
  • a new composition publishes before the old one's onDispose runs, such as when switching tabs or moving a tab between windows.

Possible fixes:

  • Keep an explicit order, for example a monotonic seq: Long on Entry, and pick maxByOrNull { it.seq }. Or
  • Define "at most one active owner per window" in the contract: publish(active = true) clears active on the window's other entries.

Whichever you choose, document the rule in the KDoc so that host and plugin agree on it.

3. 🟡 Missing @HostImplemented and the evolution notes

The description says BossConsole ships a matching parent-first implementation. That means the host's compiled copy shadows this one, and any member change later will need a host release and minBossVersion gating. BrowserTitleBarBridge is marked @HostImplemented and documents this ("Future member additions need a host release…", "Preserve this constructor…"). Please do the same here, on both the object and TerminalTitleBarAction. TerminalTitleBarAction has a 6-argument constructor with no defaults, so the "add compatible overloads instead" note matters for it.

4. 🟡 Unclear contract details

  • symbol vs icon: the reader can't tell what symbol is for. Is it an SF Symbol name for the native macOS titlebar, with icon as the Compose fallback? One line of KDoc would fix this.
  • active appears both per entry and per action (TerminalTitleBarAction.active), with different meanings: "focused terminal" vs "toggled state". Consider renaming one, e.g. selected/checked on the action, or document both.
  • Cleanup responsibility: the browser bridge spells out who must call remove or hostWindow(false), and when (e.g. "Plugins pair every publish with remove during composition disposal, including unload"). The same needs saying here. Otherwise, after the plugin unloads (hot-swap), its entries keep references to plugin lambdas and ImageVectors, which can pin the old plugin classloader.
  • hostWindow has no owner token: a disposed old window can unclaim a newer window that reuses the same windowId. BrowserTitleBarBridge.host had to add an owner overload later to fix exactly this. It's cheaper to add it now, since after release a new member means a host release.

5. 🟡 No tests in this repo

The description says the behaviour is tested in the host integration. But this repo already has BrowserTitleBarBridgeTest, and a matching TerminalTitleBarBridgeTest would be cheap and protect the API-jar copy that older hosts load. Suggested cases:

  • an unclaimed window reports isHosted == false and actions is empty
  • inactive entries contribute nothing
  • windows are isolated from each other
  • remove(owner) clears only that owner's entry
  • a test for whatever rule you pick in item 2 (which entry wins when several are active)

Nits

  • Entry doesn't need to be a data class. Its actions list contains lambdas, so generated equals is meaningless. A plain private class matches the browser bridge.
  • Security and performance: nothing to worry about. Everything is in-process, UI-thread state, and each map has one entry per window or terminal.

Overall the direction looks good. Items 1 and 2 are the ones I'd treat as blocking.

@claude

claude Bot commented Sep 27, 2026

Copy link
Copy Markdown

Review: feat(api): bridge terminal titlebar actions

The change is small and additive, and it follows the BrowserTitleBarBridge pattern from 1.0.94: only new types, apiDump regenerated, version bumped only in build.gradle.kts, the new file ends with a newline, and there is nothing Android-specific. TerminalTitleBarAction is a plain class, not a data class, which is the right call for this boundary. A few points before merge, the first two being the ones that matter most.

1. lastOrNull does not mean "most recently published" (correctness)

entries.values.lastOrNull { it.windowId == windowId && it.active }: mutableStateMapOf is a SnapshotStateMap backed by a persistent hash map, so iteration order follows hash order, not insertion order. Re-publishing an existing owner also leaves its position unchanged. If a window ever has two active entries, the titlebar shows whichever one hashes last, and that can flip between runs. Two active entries can happen with split panes, or for a frame during a focus handoff when the old terminal has not published active = false yet.

Suggestion: keep a monotonically increasing sequence number in Entry, set on each publish (or only when active goes false→true), and pick maxByOrNull { it.seq }. Or document that at most one active owner per window is a hard contract and say which one wins if it is broken.

2. Host copy vs. API-jar copy: which singleton runs?

The PR description says "BossConsole carries the matching parent-first implementation", but this object:

  • sits in ai.rever.boss.plugin.ui, not in browser. The browser bridge KDoc explicitly says "The browser package is shared parent-first".
  • has no @HostImplemented annotation and none of the KDoc that BrowserTitleBarBridge uses to explain which copy runs.

This bridge only works if the host and every terminal plugin resolve the same TerminalTitleBarBridge class. If the host compiles its own copy but ai.rever.boss.plugin.ui is not delegated parent-first to the host, you get two singletons. The host calls hostWindow on its copy, the plugin calls isHosted/publish on the ApiClassLoader copy, and both sides fail quietly: floating controls stay visible and the titlebar stays empty. Please confirm how ui is delegated in BossConsole. Then:

  • add @HostImplemented, since future member additions will need a host release plus a minBossVersion gate, the same as the browser bridge;
  • copy the browser bridge contract KDoc: UI-thread only, pair every publish with a remove on disposal, hosts release window claims on disposal, minApiVersion 1.0.95 for the types and a supporting host for native hosting.

3. Owner key uses equals, not identity

entries is keyed by Any, so it uses equals/hashCode. The browser bridge deliberately compares owners with ===. If a plugin passes a data class or string as the owner, two terminals could overwrite each other, and one terminal could remove the other one entry. Either document that owners must be unique identity tokens (e.g. remember { Any() }), or make that the only safe usage. One option is publish(...) returning an opaque handle the caller passes back to remove.

4. Smaller points

  • symbol vs icon: the KDoc should say what each is for. I assume symbol is an SF Symbol name for the native titlebar and icon is the Compose fallback. Also say whether label is the tooltip or the accessibility text. Once released, the constructor cannot change, so add "Preserve this constructor; add compatible overloads instead" like the browser types have.
  • Callback threading: say that onClick is invoked on the UI thread, so terminal plugins do not marshal to it again.
  • Changelog comment: // Next release (1.0.94): ... is now out of date next to the 1.0.95 line. Reword it to // 1.0.94: ..., and mention that 1.0.95 is new types only (ApiClassLoader/minApiVersion) apart from the host-side parent-first copy. Also check the number against the release workflow auto-bump before merging; earlier blocks in this file record numbers being taken by other merges.
  • hostWindow(false) does not drop that window entries. That is harmless because owners remove on disposal, but a sentence saying so would stop someone later adding cleanup that races with the plugin.

Tests

This repo has no tests for the bridge; the PR says coverage is in the host integration. actions() is pure logic and cheap to unit-test here. Covering "two active owners in one window → deterministic winner", "inactive owner is ignored" and "remove drops only that owner" would have caught point 1.

Performance and security

No concerns. Map sizes are about one entry per terminal, and snapshot-state reads give correct recomposition. The bridge exposes nothing across plugin trust boundaries that the existing browser bridge does not already.

Overall this looks good once the ordering behaviour and the parent-first/@HostImplemented question are sorted out. 👍

@claude

claude Bot commented Sep 27, 2026

Copy link
Copy Markdown

Review: feat(api): bridge terminal titlebar actions

Thanks for this. The API surface is small, only adds things, and apiDump has been regenerated. TerminalTitleBarAction is a plain class rather than a data class, which is right for crossing the boundary. The file ends with a newline, and plugin.json is untouched. I have a few concerns, mostly about how this compares with the BrowserTitleBarBridge that shipped in 1.0.94.

🔴 Potential bugs

1. lastOrNull() on a SnapshotStateMap doesn't give a defined order.
Content() uses providers.entries.lastOrNull() and actions() uses entries.values.lastOrNull { … }. mutableStateMapOf is backed by a persistent hash map, so iteration order isn't insertion order. The code reads as if the most recent registration or publication wins, but when there are two or more owners the winner depends on the owners' hash codes. With identity-hashed owner tokens that can change from one run to the next. For example:

  • Two terminal tabs in the same window both publish active = true for a moment during a tab switch. Which action set the titlebar shows is then arbitrary, not the one published last.
  • A plugin reloads, and the old provider hasn't been unregistered before the new one registers. Either one may render.

Options: keep an explicit ordering (a monotonically increasing sequence number in Entry and in the provider value, then take maxByOrNull { it.seq }). Or make the contract "at most one active entry per window" and have publish(active = true) clear active on the other entries for that windowId. Keying entries by windowId, as the browser bridge does by handleId, would also remove the ambiguity.

2. remove() and unregisterProvider() don't check the owner the way the browser bridge does.
In BrowserTitleBarBridge, remove(handleId, owner) only removes an entry if owner still owns it, and host(...) guards against "a disposed window cannot unregister the focus handler of a newer host". Here, entries and providers are keyed by owner, so stale-owner removal is mostly safe for entries. But nothing stops one plugin from taking over the titlebar of every hosted window by calling registerProvider, since the last provider wins, if bug 1 is fixed. Please document who may call registerProvider: host only, or the terminal plugin only. If only one provider is expected, consider rejecting or logging a second concurrent registration.

3. @HostImplemented is missing, which conflicts with the PR description.
The description says "BossConsole carries the matching parent-first implementation." If the host compiles this object in and serves it parent-first, it's exactly the case AGENTS.md describes: later member changes are shadowed by the host's copy and need a BossConsole release plus minBossVersion. BrowserTitleBarBridge is marked @HostImplemented and has a KDoc block explaining which copy runs on old and new hosts. This object needs the same annotation and similar docs. Please also confirm that ai.rever.boss.plugin.ui is actually one of the packages the host shares parent-first. Otherwise the plugin and the host each get their own TerminalTitleBarBridge singleton, with separate state maps, and the handshake never connects.

🟡 Design and code quality

  • Contract docs are thin. The browser bridge's KDoc covers threading, the pairing of publish and remove during disposal (including unload), what the host must release, and how old hosts fall back. For this bridge, please document:
    • that plugins must remove(owner) and unregisterProvider(owner) on disposal and unload. Otherwise entries and closures that hold onClick and captured terminal state leak across a plugin hot-swap.
    • that hosts must call hostWindow(id, false) when a window closes, or windows grows without bound.
    • how active is supposed to be used (at most one per window?).
    • the version gating: minApiVersion = 1.0.95 for the types, plus whatever host version supports native hosting.
  • Member order: windows and entries are declared halfway through the object, after Content(). Putting all state at the top, as BrowserTitleBarBridge does, would make it easier to read.
  • windows is a Map<String, Boolean> used as a set. This matches the browser bridge, so it's fine for consistency, but a state set or mutableStateMapOf<String, Unit> would say what it means.
  • Keep the constructor stable. Please add the same note the browser bridge has ("Preserve this constructor when extending the contract; add compatible overloads instead") to TerminalTitleBarAction. It's a final class with a public constructor in the API dump, so adding a parameter later would break binary compatibility.
  • Thread safety: "UI-thread-only" is only documented. Snapshot state writes from other threads won't crash, but callers that publish from a coroutine on Dispatchers.Default could hit snapshot write conflicts. Consider saying explicitly that callers must publish from the Compose or UI thread.

🟡 Version

  • Main is at 1.0.94, from the bump commit 4ebf191. AGENTS.md says the release workflow bump-pushes before building. Please check whether a manual bump to 1.0.95 in the PR leads CI to release 1.0.96. If it does, the new changelog comment and any minApiVersion = 1.0.95 pins in BossConsole or the terminal plugin will be off by one. The browser bridge PR seems to have added only the changelog comment and left the bump to CI.
  • The line above still says // Next release (1.0.94): …. Now that 1.0.94 has shipped, it should be reworded (for example // 1.0.94: optional native browser title-bar navigation bridge.).

🟢 Security and performance

  • Performance is fine: small maps, and reads are O(n) over the owners. Content() reads providers and windows state, so recomposition is scoped correctly.
  • On security, there's no sensitive data. The only concern is the provider takeover described in bug 2.

🧪 Test coverage

This repo has no tests for the bridge. The description says the behaviour is tested in the host integration, but that tests the host's parent-first copy, not this jar's. A small unit test here would help and would have caught bug 1. It could cover publishing from two owners, selection of the active entry, windows being isolated, and remove clearing an entry. Tests can drive SnapshotStateMap without a composition by using Snapshot.withMutableSnapshot, or by reading it directly.

Summary

The idea is sound and it follows the shape of the browser bridge. Before merging I'd ask for:

  1. deterministic selection instead of lastOrNull() on a hash-ordered map,
  2. @HostImplemented plus the parent-first and fallback KDoc, and confirmation that the ui package is shared parent-first,
  3. a check of the version bump against the release workflow.

@claude

claude Bot commented Sep 28, 2026

Copy link
Copy Markdown

Review: feat(api): bridge terminal titlebar actions

The change is small and additive, and it follows the pattern set by BrowserTitleBarBridge: a plain class rather than a data class for the carrier, an object holding snapshot state, and unclaimed windows falling back to the floating controls. apiCheck should pass since nothing existing changes, and the new file ends with a newline. I found a few issues. The first two should be fixed before merge.

🔴 1. "Last registered" is not insertion order: mutableStateMapOf is hash-ordered

Content, CallBarContent and actions all pick a winner with .lastOrNull { … } over providers.entries / callBars.values / entries.values. SnapshotStateMap is backed by a PersistentHashMap, so iteration order comes from the key hashes, not from insertion order. The owners are Any, which usually means identity hash codes, so the order is effectively random:

  • If two providers are registered (for example across a plugin reload, before the old one unregisters), which one renders is arbitrary. The winner can also change whenever a third key is added, because the trie gets reshaped.
  • If two terminals in the same window both publish active = true for a moment (focus handoff, where the new owner publishes before the old one republishes inactive), actions(windowId) can return the wrong terminal's buttons, and those buttons fire callbacks on a terminal that isn't focused.
  • The same applies to CallBarContent when two call bars target one window.

If "most recent wins" is the intended rule, store an explicit sequence number in each entry (private var seq = 0L, stamped on publish/register) and use maxByOrNull { it.seq }. The other option is to keep a single slot per window. Either way, the ordering needs to be deterministic.

🔴 2. Missing @HostImplemented (and the lifecycle KDoc its sibling has)

The PR description says "BossConsole carries the matching parent-first implementation", so the host compiles this object in, and the host's copy will shadow any later member change. Under the evolution rules in AGENTS.md, that makes it @HostImplemented, the same as BrowserTitleBarBridge. Without the annotation, a future contributor will reasonably assume a new member can ship through minApiVersion alone. Their plugin would then be rejected by the BinaryCompatibilityValidator on a host whose pinned copy lacks that member.

BrowserTitleBarBridge's KDoc also records the contract: which API version adds the types, that native hosting needs a supporting host, that future members need minBossVersion, and that every publish is paired with remove on disposal/unload. The same content belongs here. For this bridge, consumers need to be told:

  • to call unregisterProvider / remove / removeCallBar in dispose() / DisposableEffect, and
  • that the host must call hostWindow(id, false) when a window closes.

🟠 3. Version bump may skip a number

AGENTS.md and the comment block in build.gradle.kts both say: "Release CI bump-pushes before building, so main's version below is the version already released and this merge cuts the next one." main is at 1.0.94, and that version was released by 4ebf191 🔖 Bump version to 1.0.94. Leaving version = "1.0.94" should make this merge release 1.0.95. Setting it to 1.0.95 by hand looks likely to produce 1.0.96, which would leave BossConsole's pin (and any minApiVersion = 1.0.95) pointing at a version whose jar doesn't have these types. Please check this against the shared plugin-release.yml. If it does bump, revert the version = line and keep only the changelog comment. The stale "Next release (1.0.94)" line should also be rewritten as a plain 1.0.94: entry to match the rest of the log.

🟠 4. Classloader retention across plugin unload / hot swap

Every map is keyed by, and holds, plugin-supplied Any owners and lambdas (@Composable content, onClick) inside a long-lived host-side singleton. If a plugin misses a remove* during unload, which is easy to do for the call bar, the object keeps a strong reference to the plugin's classloader for the rest of the session, and the host keeps showing buttons that call into a disposed plugin. Same for windows: a window that closes without hostWindow(false) leaks an entry. Some options:

  • document the pairing requirement (see ✨ Add LocalIsPanelActive CompositionLocal #2),
  • have the host clear entries whose owner's class came from the unloading plugin's classloader during unload-all, and/or
  • clear entries/callBars for a window when hostWindow(id, false) is called.

🟡 5. CallBarContent should key on owner, as Content does

Content wraps the provider in key(owner, windowId), but CallBarContent calls content() directly. When the selected call bar switches from one owner to another and both come from the same composable lambda source (two terminals running the same plugin code), the second owner reuses the first one's remember slots: text-field state, dialogs, animation state. Carry the owner in CallBar and wrap the call in key(owner) { … }.

🟡 6. Minor / API shape

  • publishCallBar already de-duplicates on content !==, but a lambda built inline during composition gets a new identity on each recomposition unless it is remembered. Each publish would then write state mid-composition and could cause an extra recompose. It would help to say in the KDoc that callers should publish from a DisposableEffect/SideEffect with a stable lambda. publish has no de-duplication at all, and List<TerminalTitleBarAction> with lambda fields never compares equal, so the same advice applies.
  • windows could be a set (or a SnapshotStateMap<String, Unit>). The Boolean value is never false.
  • The declaration order mixes providers, call bars, windows and entries. Putting all state at the top, then the three groups each with a one-line doc, would make the three independent mechanisms easier to follow. The PR description doesn't mention the call-bar mechanism at all.
  • TerminalTitleBarAction has both symbol: String and icon: ImageVector. A short doc saying which one a native (non-Compose) titlebar uses (presumably symbol as an SF Symbol name?) would help implementers. Add the "preserve this constructor; add compatible overloads" note the browser types carry.

Test coverage

There are no tests in this repo. The description says behaviour is tested in the host integration, but that suite won't catch regressions here, where the class actually lives. src/test/kotlin/ai/rever/boss/plugin/ui/OverlayRoutingTest.kt already sets a precedent for plain-JVM tests of this package, and snapshot state maps work without a composition. Cheap cases to add:

  • actions() is window-isolated and ignores inactive owners,
  • remove(owner) clears only that owner,
  • selection among multiple active owners is deterministic (this case would currently fail; see Preserve POST body across popup→tab handoff #1),
  • hostWindow(id, false) makes isHosted false.

Conventions checklist

Nice work keeping the fallback path (unclaimed window → floating controls) as the default. Fixing #1–#3 would make this ready to merge.

@claude

claude Bot commented Sep 28, 2026

Copy link
Copy Markdown

Small correction to my review above: under Test coverage, "There are no tests in this repo" should read "This PR adds no tests". The repo does have a JVM test suite, including OverlayRoutingTest in the same ui package, which is why I suggested adding cases there.

@claude

claude Bot commented Sep 28, 2026

Copy link
Copy Markdown

Review: feat(api): bridge terminal titlebar actions

Overall this is a well-contained change. It adds only new types in ai.rever.boss.plugin.ui, does not touch existing constructors or interfaces, updates the ABI dump, and ships tests. It follows the pattern set by BrowserTitleBarBridge in 1.0.94: host-owned window claims, owners compared by identity, UI-thread-only access, and unclaimed windows keep the existing behaviour. Some notes below, roughly most important first.

Potential bugs / behavioural gotchas

  1. "Unchanged effects do not steal ownership" only holds if callers reuse the same action instances.
    TerminalOwners.put skips the update when values[index].value == value. Entry and CallBar are data classes, but TerminalTitleBarAction is a plain class that compares by identity, and CallBar.content is a lambda. So a plugin that builds listOf(TerminalTitleBarAction(...)) inside a LaunchedEffect/SideEffect produces a new value every time, even with stable callbacks. Each such publish then:

    • moves that owner to the end of the list, so it takes ownership from whoever published most recently, and
    • changes the snapshot list, which recomposes everything that reads actions(), hasCallBar() or CallBarContent().

    The test only passes because it reuses the same a list. Either:

    • say in the KDoc that callers must remember the action list and call-bar lambda, or
    • compare by field (id, label, symbol, active, and onClick by identity). Making TerminalTitleBarAction a data class isn't an option under the evolution rules, so this would be a private comparison in the bridge.

    Right now "Publish from effects with stable callbacks" reads as if stable callbacks are enough, and they aren't.

  2. Window claims have no owner. hostWindow(id, false) removes the claim no matter who made it. If two host surfaces (for example a window being recreated) claim the same windowId and the old one is disposed after the new one claims it, the new claim is dropped. BrowserTitleBarBridge.host(handleId, owner, focus) already fixed this for focus handlers ("A disposed window cannot unregister the focus handler of a newer host"). Because this is @HostImplemented, adding an owner overload later needs a BossConsole release. It would be cheaper to add hostWindow(windowId, owner, enabled) now, while the type is new.

  3. Content() ignores which window a provider belongs to. It picks providers.last() for every hosted window. That's fine for "one terminal plugin registers one provider", but if two providers overlap (for example during a hot swap, where unload/reload order isn't guaranteed), every window switches to the newest one. That matches the KDoc, so this is just something to be aware of.

Build / versioning

  1. localApiVersion override. providers.gradleProperty("localApiVersion").orNull?.let { version = it } is handy locally, but:
    • it is a second place that writes version. AGENTS.md says build.gradle.kts's version = "..." is the single source of truth, so please add a short note to AGENTS.md's Version Management section so it isn't a hidden override;
    • please confirm the shared plugin-release.yml bump step matches only the version = "x.y.z" line (for example with an anchored regex) and won't touch or trip on the { version = it } line;
    • a jar built with -PlocalApiVersion=1.0.95 from an unreleased tree will have the same version as the real 1.0.95 release. Dropping it into ~/.boss/plugins/ could shadow or confuse the real release. A suffix convention (1.0.95-local) might be safer, if the host's version comparison accepts it.
  2. The version line correctly stays at 1.0.94 (release CI bumps it), and plugin.json is untouched. ✅ Small wording nit: the new comment "1.0.95 adds …" could follow the old convention, "Next release (1.0.95): …", so it's clear this hasn't been released yet.

Code quality

  1. The @HostImplemented annotation is inconsistent. TerminalTitleBarAction has it, but the equivalent BrowserTitleBarState doesn't. If the rule is "any type the host compiles in", BrowserTitleBarState probably needs it too. Otherwise consider leaving it off the value class so the two bridges stay consistent.
  2. windows is a mutableStateMapOf<String, Boolean> that only ever stores true. That copies the browser bridge, so it's fine for consistency.
  3. @Suppress("TooManyFunctions") sits between @HostImplemented and the comment. It works, but putting the comment above both annotations reads more naturally.
  4. Test file: the kotlinx.coroutines.runBlocking import is in the middle of the androidx.* imports. ktlint's lexicographic import-ordering rule will flag this if it's enabled.

Tests

The tests are a good start and cover ordering, identity versus equals owners, active filtering, window isolation, and that call-bar cleanup can't revoke a host claim. Suggestions:

  • Each step calls composition.setContent { ... } again, so the tests check the first composition, not reactive recomposition when the state list changes. That reactive path is what the host depends on. Consider one setContent, then Snapshot.sendApplyNotifications() and awaiting the recomposer (or using runRecomposeAndApplyChanges in a child job) after each mutation.
  • Add a test that Content() renders nothing for an unclaimed window (the fallback path for unsupported hosts).
  • Add a test for item 1: republishing with fresh but equivalent TerminalTitleBarAction instances. That documents whichever behaviour you choose.
  • All tests share global singleton state. The finally cleanup handles this, but a leftover from one failing test can cascade into others. A small @AfterTest that clears the known owners/windows would make that more robust.

Conventions checklist

  • Compose Multiplatform APIs only (androidx.compose.runtime/ui.graphics, no Android APIs) ✅
  • No PluginContext provider access is added, so the null-provider rule doesn't apply. Unsupported hosts fall back to floating controls ✅
  • Version changed only in build.gradle.kts, plugin.json untouched ✅ (see item 4 about the override)
  • Both new Kotlin files end with a newline ✅
  • ABI dump updated. The change is additive and new-type-only ✅

Security / performance

  • No security concerns. Only in-process UI callbacks, no I/O.
  • Performance is fine for the expected size (a handful of owners, linear scans). The only real cost is the unnecessary recomposition described in item 1.

Nice work overall. Item 1 (a docs or equality fix) and item 2 (owner-scoped window claims, while the type is still new) are the ones I'd address before merging, since both become expensive to change once a host ships its compiled-in copy.

@kshivang
kshivang merged commit 25e730a into main Sep 28, 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