ADFA-4399: Floating windows feature#1456
Conversation
…on (#1449) feat(ADFA-4400): add floating-window module with docking model and overlay foundation
* feat(ADFA-4400): add floating-window module with docking model and overlay foundation * feat(ADFA-4401): add floating window chrome * ADFA-4401: localize floating window chrome accessibility labels
* feat(ADFA-4400): add floating-window module with docking model and overlay foundation * feat(ADFA-4401): add floating window chrome * feat(ADFA-4402): add per-window controller and foreground service
* feat(ADFA-4400): add floating-window module with docking model and overlay foundation * feat(ADFA-4401): add floating window chrome * feat(ADFA-4402): add per-window controller and foreground service * feat(ADFA-4403): undock and redock editor and plugin tabs
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
📝 Walkthrough
WalkthroughThe PR adds a floating-window module, overlay runtime, tab adapters, and editor activity wiring that lets file and plugin tabs be undocked into a foreground service and later redocked or closed through docking events. ChangesFloating window docking
Sequence Diagram(s)sequenceDiagram
participant EditorHandlerActivity
participant IdeFloatingTabController
participant DockingManager
participant FloatingTabService
participant FloatingWindow
EditorHandlerActivity->>IdeFloatingTabController: undockFileTab(fileIndex) / undockPluginTab(tabId, position)
IdeFloatingTabController->>DockingManager: undock(content, WindowBounds)
IdeFloatingTabController->>FloatingTabService: ensureRunning(context)
FloatingTabService->>DockingManager: collect windows and reconcile
DockingManager-->>FloatingTabService: windows updates
FloatingTabService->>FloatingWindow: create and show
DockingManager-->>IdeFloatingTabController: DockingEvent.Redock / Close
IdeFloatingTabController->>EditorHandlerActivity: save/release and reopen or select tab
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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
`@app/src/main/java/com/itsaky/androidide/editor/floating/EditorPanelDockableContent.kt`:
- Around line 75-78: The teardown currently only happens in release(), but the
floating-window lifecycle expects EditorPanelDockableContent to clean up in
onDestroyView(). Add the DockableContent.onDestroyView() implementation to
invoke the same CodeEditorView.close() and null-out logic used by release(), so
the editor is always released even when the overlay is destroyed outside the
redock flow.
In
`@app/src/main/java/com/itsaky/androidide/editor/floating/IdeFloatingTabController.kt`:
- Around line 65-70: In IdeFloatingTabController’s floating-tab flow, the
original tab is being removed too early, before the floating copy is confirmed
to exist. Reorder the logic so remove() is only called after
DockingManager.undock and FloatingTabService.ensureRunning complete
successfully, or gate it behind a successful floating-host creation path, so the
docked tab can be restored if the floating setup fails.
- Around line 44-47: The undock flow in IdeFloatingTabController.launch
currently proceeds to mark the panel saved and close the file unconditionally
after panel.save(). Update this path so the save result is checked first; if
panel.save() returns false, stop the coroutine flow and do not call
panel.markAsSaved() or activity.closeFile(...). Keep the existing save/close
sequence only when the save succeeds.
- Around line 29-33: The event collection in IdeFloatingTabController.start is
tied to activity.lifecycleScope, so DockingManager.events can be missed when the
activity is recreated or inactive. Move the collection out of the activity
lifecycle (for example, use an application-wide scope) or change
DockingManager.events to a MutableSharedFlow with replay = 1 so new subscribers
still receive the latest Redock/Close event. Keep the fix centered around
start() and DockingManager.events.
In
`@app/src/main/java/com/itsaky/androidide/editor/floating/PluginTabDockableContent.kt`:
- Around line 37-39: `PluginTabDockableContent.createContent` currently returns
`overlayHost.view` even when `PluginEditorTabManager.newTabFragment(tabId)` is
null, which bypasses the existing failure UI. Update the flow so the
`overlayHost` only returns its normal view after `setFragment` succeeds, and
otherwise switch to the fallback error view already implemented for load
failures. Keep the null-handling logic centered around `newTabFragment(tabId)`
and `overlayHost` so a missing plugin fragment is treated as an actual failure.
In `@floating-window/src/main/AndroidManifest.xml`:
- Around line 6-8: Add the missing foreground-service declaration for
FloatingTabService in AndroidManifest.xml: the service that calls
startForeground/startForegroundService needs
android:foregroundServiceType="specialUse" on the <service> entry. Also add the
matching android.permission.FOREGROUND_SERVICE_SPECIAL_USE uses-permission
alongside the other manifest permissions. Update the existing FloatingTabService
declaration rather than introducing a new service entry.
In
`@floating-window/src/main/java/com/itsaky/androidide/floating/model/DockingManager.kt`:
- Around line 28-29: The event flow in `DockingManager.dock()` and
`DockingManager.close()` can desynchronize `_windows` from the UI because state
is removed before `MutableSharedFlow.tryEmit()` is confirmed. Update these
methods so the removal only happens after a successful emission, or handle the
`false` return from `_events.tryEmit(...)` by aborting the mutation and
preserving the tab in `_windows`. If the caller can suspend, prefer using a
coroutine-backed `emit` path; otherwise ensure `DockingEvent` delivery is
guaranteed before mutating state.
In
`@floating-window/src/main/java/com/itsaky/androidide/floating/service/FloatingTabService.kt`:
- Around line 89-97: The undock recovery in FloatingTabService is marking a tab
as floating before FloatingWindow.show() has actually succeeded, and the failure
path closes the tab instead of restoring it. Update the attach flow so
FloatingWindow.show() reports success/failure, only store the window in windows
after a successful attach, and on exceptions redock the tab through
DockingManager rather than calling DockingManager.close; use FloatingTabService,
FloatingWindow.show(), and the existing windows/failed handling to keep the tab
recoverable.
In
`@floating-window/src/main/java/com/itsaky/androidide/floating/ui/FloatingWindowChrome.kt`:
- Around line 251-261: The `combinedClickable` click handler in
`FloatingWindowChrome.kt` is swallowing all failures from
`DockAction.onInvoke()` by using `runCatching`, which also hides cancellation
and unexpected errors. Replace the broad catch with narrow handling for only the
expected failure path around the `action.onInvoke()` call, and let other
exceptions propagate or be logged. Keep the existing confirm/`ACTION_CONFIRM_MS`
behavior intact in the `onClick` coroutine.
- Around line 112-116: The resize grip in FloatingWindowChrome is still shown
even when the window is maximized, but FloatingWindow.resizeBy() does nothing
outside WindowMode.NORMAL, so hide this dead UI in non-normal modes. Update the
ResizeHandle rendering in FloatingWindowChrome to be conditional on the current
window mode, using the same state or mode source already available to the
composable, so the handle is only visible when resizing is actually supported.
In
`@floating-window/src/main/java/com/itsaky/androidide/floating/window/FloatingWindow.kt`:
- Around line 98-104: The show() method in FloatingWindow currently swallows
addView() failures via runCatching, which prevents
FloatingTabService.reconcile() from detecting and recovering from a failed show.
Remove the exception suppression in show(), keep the existing
host.attach(rootView) and windowManager.addView(rootView, params) flow, and let
addView() errors propagate so the service can retry or close the window; keep
the added flag update only on success.
In
`@floating-window/src/main/java/com/itsaky/androidide/floating/window/InitialBounds.kt`:
- Around line 22-27: The cascaded window position logic in
InitialBounds.calculate/WindowBounds only clamps to zero, so later cascade steps
can place windows beyond the visible viewport. Update the x and y calculations
to clamp within both bounds: keep the current centered base plus cascade step,
but also cap each coordinate at the maximum visible position derived from
metrics.widthPixels/heightPixels minus the window size before returning
WindowBounds, so OverlayLayoutParams.create receives on-screen coordinates.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 395f6d7f-d1d9-4b3e-8438-cc1c54f34def
📒 Files selected for processing (26)
app/build.gradle.ktsapp/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.ktapp/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.ktapp/src/main/java/com/itsaky/androidide/editor/floating/EditorPanelDockableContent.ktapp/src/main/java/com/itsaky/androidide/editor/floating/IdeFloatingTabController.ktapp/src/main/java/com/itsaky/androidide/editor/floating/PluginTabDockableContent.ktfloating-window/build.gradle.ktsfloating-window/src/main/AndroidManifest.xmlfloating-window/src/main/java/com/itsaky/androidide/floating/fragment/OverlayFragmentHost.ktfloating-window/src/main/java/com/itsaky/androidide/floating/model/DockAction.ktfloating-window/src/main/java/com/itsaky/androidide/floating/model/DockableContent.ktfloating-window/src/main/java/com/itsaky/androidide/floating/model/DockingEvent.ktfloating-window/src/main/java/com/itsaky/androidide/floating/model/DockingManager.ktfloating-window/src/main/java/com/itsaky/androidide/floating/model/FloatingTab.ktfloating-window/src/main/java/com/itsaky/androidide/floating/permission/OverlayPermission.ktfloating-window/src/main/java/com/itsaky/androidide/floating/service/FloatingTabService.ktfloating-window/src/main/java/com/itsaky/androidide/floating/ui/FloatingTheme.ktfloating-window/src/main/java/com/itsaky/androidide/floating/ui/FloatingWindowChrome.ktfloating-window/src/main/java/com/itsaky/androidide/floating/window/FloatingWindow.ktfloating-window/src/main/java/com/itsaky/androidide/floating/window/FloatingWindowHost.ktfloating-window/src/main/java/com/itsaky/androidide/floating/window/FloatingWindowState.ktfloating-window/src/main/java/com/itsaky/androidide/floating/window/InitialBounds.ktfloating-window/src/main/java/com/itsaky/androidide/floating/window/OverlayLayoutParams.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/ui/PluginEditorTabManager.ktresources/src/main/res/values/strings.xmlsettings.gradle.kts
| fun release() { | ||
| editorView?.close() | ||
| editorView = null | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Implement the DockableContent.onDestroyView() teardown path.
This class only exposes release(), but the floating-window contract tears content down via onDestroyView(). If the overlay is destroyed outside the activity-driven redock flow, CodeEditorView.close() never runs and the editor instance stays alive longer than intended.
🤖 Prompt for 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.
In
`@app/src/main/java/com/itsaky/androidide/editor/floating/EditorPanelDockableContent.kt`
around lines 75 - 78, The teardown currently only happens in release(), but the
floating-window lifecycle expects EditorPanelDockableContent to clean up in
onDestroyView(). Add the DockableContent.onDestroyView() implementation to
invoke the same CodeEditorView.close() and null-out logic used by release(), so
the editor is always released even when the overlay is destroyed outside the
redock flow.
| fun start() { | ||
| activity.lifecycleScope.launch { | ||
| DockingManager.events.collect(::onEvent) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
ast-grep run --pattern 'class DockingManager' --lang kotlinRepository: appdevforall/CodeOnTheGo
Length of output: 162
🏁 Script executed:
fd -t f "DockingManager.kt" .Repository: appdevforall/CodeOnTheGo
Length of output: 249
🏁 Script executed:
cat -n ./floating-window/src/main/java/com/itsaky/androidide/floating/model/DockingManager.kt | head -100Repository: appdevforall/CodeOnTheGo
Length of output: 4045
🏁 Script executed:
node - <<'JS'
const fs = require('fs');
// Simulating the default behavior of MutableSharedFlow
// In Kotlin Coroutines:
// public constructor(replay: Int = 0, extraBufferCapacity: Int = 0, onBufferOverflow: BufferOverflow = SUSPEND)
// The review states replay=0 (default) which means no history is saved.
console.log("Verification: MutableSharedFlow defaults to replay=0");
console.log("Conclusion: Events emitted while no collector is active are lost (unless buffered, but not replayed to new collectors).");
JSRepository: appdevforall/CodeOnTheGo
Length of output: 335
🏁 Script executed:
cat -n ./app/src/main/java/com/itsaky/androidide/editor/floating/IdeFloatingTabController.kt | head -50Repository: appdevforall/CodeOnTheGo
Length of output: 2242
Do not bind docking-event reconciliation to the activity lifecycle.
DockingManager is a singleton, and its events property is a MutableSharedFlow configured with extraBufferCapacity = 16 but no replay history (replay=0).
Since IdeFloatingTabController is instantiated per EditorHandlerActivity and collects events within activity.lifecycleScope, any Redock or Close event emitted by the floating window service while the activity is destroyed (e.g., configuration change, background navigation) or before the new instance starts will be lost forever. The new controller instance will not receive these past events, causing the editor state to drift from the floating window state.
Decouple the collection from the activity lifecycle (e.g., move to Application scope) or configure the MutableSharedFlow with replay = 1 to ensure the latest event is delivered to new subscribers.
🤖 Prompt for 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.
In
`@app/src/main/java/com/itsaky/androidide/editor/floating/IdeFloatingTabController.kt`
around lines 29 - 33, The event collection in IdeFloatingTabController.start is
tied to activity.lifecycleScope, so DockingManager.events can be missed when the
activity is recreated or inactive. Move the collection out of the activity
lifecycle (for example, use an application-wide scope) or change
DockingManager.events to a MutableSharedFlow with replay = 1 so new subscribers
still receive the latest Redock/Close event. Keep the fix centered around
start() and DockingManager.events.
| activity.lifecycleScope.launch { | ||
| panel.save() | ||
| panel.markAsSaved() | ||
| activity.closeFile(fileIndex) {} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Abort the undock if panel.save() fails.
This path clears the dirty state and closes the docked editor unconditionally. If save() returns false, the tab is still marked saved and removed, which can discard unsaved edits.
Suggested fix
activity.lifecycleScope.launch {
- panel.save()
+ if (!panel.save()) {
+ return@launch
+ }
panel.markAsSaved()
activity.closeFile(fileIndex) {}
DockingManager.undock(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| activity.lifecycleScope.launch { | |
| panel.save() | |
| panel.markAsSaved() | |
| activity.closeFile(fileIndex) {} | |
| activity.lifecycleScope.launch { | |
| if (!panel.save()) { | |
| return@launch | |
| } | |
| panel.markAsSaved() | |
| activity.closeFile(fileIndex) {} |
🤖 Prompt for 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.
In
`@app/src/main/java/com/itsaky/androidide/editor/floating/IdeFloatingTabController.kt`
around lines 44 - 47, The undock flow in IdeFloatingTabController.launch
currently proceeds to mark the panel saved and close the file unconditionally
after panel.save(). Update this path so the save result is checked first; if
panel.save() returns false, stop the coroutine flow and do not call
panel.markAsSaved() or activity.closeFile(...). Keep the existing save/close
sequence only when the save succeeds.
| remove() | ||
| DockingManager.undock( | ||
| PluginTabDockableContent(tabId, title), | ||
| InitialBounds.cascaded(activity, undockCounter++), | ||
| ) | ||
| FloatingTabService.ensureRunning(activity.applicationContext) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
The docked plugin tab is removed before the floating copy is known-good.
remove() runs first, but the floating plugin content can still fail later during fragment creation/hosting. In that case the original tab is already gone and there is nothing left to redock back to.
🤖 Prompt for 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.
In
`@app/src/main/java/com/itsaky/androidide/editor/floating/IdeFloatingTabController.kt`
around lines 65 - 70, In IdeFloatingTabController’s floating-tab flow, the
original tab is being removed too early, before the floating copy is confirmed
to exist. Reorder the logic so remove() is only called after
DockingManager.undock and FloatingTabService.ensureRunning complete
successfully, or gate it behind a successful floating-host creation path, so the
docked tab can be restored if the floating setup fails.
| overlayHost.start() | ||
| PluginEditorTabManager.getInstance().newTabFragment(tabId)?.let(overlayHost::setFragment) | ||
| overlayHost.view |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Treat a null plugin fragment as a load failure.
PluginEditorTabManager.newTabFragment(tabId) can return null, but this path still returns overlayHost.view. That leaves the user with an empty floating window instead of the fallback error view you already implemented.
Suggested fix
- overlayHost.start()
- PluginEditorTabManager.getInstance().newTabFragment(tabId)?.let(overlayHost::setFragment)
- overlayHost.view
+ overlayHost.start()
+ val fragment = PluginEditorTabManager.getInstance().newTabFragment(tabId)
+ ?: run {
+ overlayHost.destroy()
+ fragmentHost = null
+ return errorView(context)
+ }
+ overlayHost.setFragment(fragment)
+ overlayHost.view📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| overlayHost.start() | |
| PluginEditorTabManager.getInstance().newTabFragment(tabId)?.let(overlayHost::setFragment) | |
| overlayHost.view | |
| overlayHost.start() | |
| val fragment = PluginEditorTabManager.getInstance().newTabFragment(tabId) | |
| ?: run { | |
| overlayHost.destroy() | |
| fragmentHost = null | |
| return errorView(context) | |
| } | |
| overlayHost.setFragment(fragment) | |
| overlayHost.view |
🤖 Prompt for 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.
In
`@app/src/main/java/com/itsaky/androidide/editor/floating/PluginTabDockableContent.kt`
around lines 37 - 39, `PluginTabDockableContent.createContent` currently returns
`overlayHost.view` even when `PluginEditorTabManager.newTabFragment(tabId)` is
null, which bypasses the existing failure UI. Update the flow so the
`overlayHost` only returns its normal view after `setFragment` succeeds, and
otherwise switch to the fallback error view already implemented for load
failures. Keep the null-handling logic centered around `newTabFragment(tabId)`
and `overlayHost` so a missing plugin fragment is treated as an actual failure.
| try { | ||
| val window = FloatingWindow(newWindowContext(), tab) | ||
| windows[tab.id] = window | ||
| window.show() | ||
| } catch (t: Throwable) { | ||
| log.error("Failed to create floating window for {}; dropping it", tab.id, t) | ||
| failed.add(tab.id) | ||
| windows.remove(tab.id)?.let { runCatching { it.dismiss() } } | ||
| DockingManager.close(tab.id) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Recover failed undocks by redocking, and only after a successful attach.
This path records the FloatingWindow before show() has proven that the overlay was added, and the fallback turns hosting errors into DockingManager.close(...). That means a failed attach either leaves the tab stuck as “floating” with no visible window, or permanently closes the underlying tab instead of restoring it to the editor.
Proposed direction
try {
val window = FloatingWindow(newWindowContext(), tab)
- windows[tab.id] = window
- window.show()
+ if (window.show()) {
+ windows[tab.id] = window
+ } else {
+ failed.add(tab.id)
+ DockingManager.dock(tab.id)
+ }
} catch (t: Throwable) {
log.error("Failed to create floating window for {}; dropping it", tab.id, t)
failed.add(tab.id)
windows.remove(tab.id)?.let { runCatching { it.dismiss() } }
- DockingManager.close(tab.id)
+ DockingManager.dock(tab.id)
}This also needs FloatingWindow.show() to report success/failure instead of swallowing addView(...) errors.
🤖 Prompt for 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.
In
`@floating-window/src/main/java/com/itsaky/androidide/floating/service/FloatingTabService.kt`
around lines 89 - 97, The undock recovery in FloatingTabService is marking a tab
as floating before FloatingWindow.show() has actually succeeded, and the failure
path closes the tab instead of restoring it. Update the attach flow so
FloatingWindow.show() reports success/failure, only store the window in windows
after a successful attach, and on exceptions redock the tab through
DockingManager rather than calling DockingManager.close; use FloatingTabService,
FloatingWindow.show(), and the existing windows/failed handling to keep the tab
recoverable.
| ResizeHandle( | ||
| onResize = onResize, | ||
| onResizeStopped = onResizeStopped, | ||
| modifier = Modifier.align(Alignment.BottomEnd), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Hide the resize grip while maximized.
FloatingWindow.resizeBy() is a no-op outside WindowMode.NORMAL, so this handle becomes dead UI in maximized mode.
Suggested change
- ResizeHandle(
- onResize = onResize,
- onResizeStopped = onResizeStopped,
- modifier = Modifier.align(Alignment.BottomEnd),
- )
+ if (!maximized) {
+ ResizeHandle(
+ onResize = onResize,
+ onResizeStopped = onResizeStopped,
+ modifier = Modifier.align(Alignment.BottomEnd),
+ )
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ResizeHandle( | |
| onResize = onResize, | |
| onResizeStopped = onResizeStopped, | |
| modifier = Modifier.align(Alignment.BottomEnd), | |
| ) | |
| if (!maximized) { | |
| ResizeHandle( | |
| onResize = onResize, | |
| onResizeStopped = onResizeStopped, | |
| modifier = Modifier.align(Alignment.BottomEnd), | |
| ) | |
| } |
🤖 Prompt for 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.
In
`@floating-window/src/main/java/com/itsaky/androidide/floating/ui/FloatingWindowChrome.kt`
around lines 112 - 116, The resize grip in FloatingWindowChrome is still shown
even when the window is maximized, but FloatingWindow.resizeBy() does nothing
outside WindowMode.NORMAL, so hide this dead UI in non-normal modes. Update the
ResizeHandle rendering in FloatingWindowChrome to be conditional on the current
window mode, using the same state or mode source already available to the
composable, so the handle is only visible when resizing is actually supported.
| .combinedClickable( | ||
| onLongClick = { action.onLongPress?.invoke(view) }, | ||
| onClick = { | ||
| scope.launch { | ||
| val confirm = runCatching { action.onInvoke() }.getOrDefault(false) | ||
| if (confirm && action.confirmIconRes != null) { | ||
| confirmed = true | ||
| delay(ACTION_CONFIRM_MS) | ||
| confirmed = false | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don’t swallow all DockAction failures.
runCatching { action.onInvoke() } turns every thrown Throwable into false, so button failures disappear silently and coroutine cancellation/errors get eaten too. Catch only the specific expected exception here, or let unexpected failures surface/log.
Based on learnings, this project prefers narrow exception handling in Kotlin crash paths instead of broad catch-alls.
🤖 Prompt for 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.
In
`@floating-window/src/main/java/com/itsaky/androidide/floating/ui/FloatingWindowChrome.kt`
around lines 251 - 261, The `combinedClickable` click handler in
`FloatingWindowChrome.kt` is swallowing all failures from
`DockAction.onInvoke()` by using `runCatching`, which also hides cancellation
and unexpected errors. Replace the broad catch with narrow handling for only the
expected failure path around the `action.onInvoke()` call, and let other
exceptions propagate or be logged. Keep the existing confirm/`ACTION_CONFIRM_MS`
behavior intact in the `onClick` coroutine.
Source: Learnings
| fun show() { | ||
| if (added) return | ||
| host.attach(rootView) | ||
| runCatching { windowManager.addView(rootView, params) } | ||
| .onSuccess { added = true } | ||
| .onFailure { log.error("Failed to add floating window {}", id, it) } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Let show() fail fast when addView() fails.
FloatingTabService.reconcile() only recovers from creation/show failures if show() throws. Swallowing the addView() exception here leaves the tab undocked but invisible, because the service keeps this instance in its windows map with added = false and never retries or closes it.
Suggested change
fun show() {
if (added) return
host.attach(rootView)
- runCatching { windowManager.addView(rootView, params) }
- .onSuccess { added = true }
- .onFailure { log.error("Failed to add floating window {}", id, it) }
+ try {
+ windowManager.addView(rootView, params)
+ added = true
+ } catch (t: Throwable) {
+ host.destroy()
+ tab.content.onDestroyView()
+ throw t
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun show() { | |
| if (added) return | |
| host.attach(rootView) | |
| runCatching { windowManager.addView(rootView, params) } | |
| .onSuccess { added = true } | |
| .onFailure { log.error("Failed to add floating window {}", id, it) } | |
| } | |
| fun show() { | |
| if (added) return | |
| host.attach(rootView) | |
| try { | |
| windowManager.addView(rootView, params) | |
| added = true | |
| } catch (t: Throwable) { | |
| host.destroy() | |
| tab.content.onDestroyView() | |
| throw t | |
| } | |
| } |
🤖 Prompt for 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.
In
`@floating-window/src/main/java/com/itsaky/androidide/floating/window/FloatingWindow.kt`
around lines 98 - 104, The show() method in FloatingWindow currently swallows
addView() failures via runCatching, which prevents
FloatingTabService.reconcile() from detecting and recovering from a failed show.
Remove the exception suppression in show(), keep the existing
host.attach(rootView) and windowManager.addView(rootView, params) flow, and let
addView() errors propagate so the service can retry or close the window; keep
the added flag update only on success.
| val step = (CASCADE_STEP_DP * metrics.density).roundToInt() * (index % CASCADE_WRAP) | ||
| val baseX = (metrics.widthPixels - width) / 2 | ||
| val baseY = (metrics.heightPixels - height) / 2 | ||
| return WindowBounds( | ||
| x = (baseX + step).coerceAtLeast(0), | ||
| y = (baseY + step).coerceAtLeast(0), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clamp cascaded positions to the visible viewport.
These coordinates only guard against negative values. On typical phone widths, the later cascade steps can push x/y past screenSize - windowSize, and OverlayLayoutParams.create(...) applies them as-is, so new windows start partially off-screen.
Proposed fix
val step = (CASCADE_STEP_DP * metrics.density).roundToInt() * (index % CASCADE_WRAP)
val baseX = (metrics.widthPixels - width) / 2
val baseY = (metrics.heightPixels - height) / 2
+ val maxX = (metrics.widthPixels - width).coerceAtLeast(0)
+ val maxY = (metrics.heightPixels - height).coerceAtLeast(0)
return WindowBounds(
- x = (baseX + step).coerceAtLeast(0),
- y = (baseY + step).coerceAtLeast(0),
+ x = (baseX + step).coerceIn(0, maxX),
+ y = (baseY + step).coerceIn(0, maxY),
width = width,
height = height,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val step = (CASCADE_STEP_DP * metrics.density).roundToInt() * (index % CASCADE_WRAP) | |
| val baseX = (metrics.widthPixels - width) / 2 | |
| val baseY = (metrics.heightPixels - height) / 2 | |
| return WindowBounds( | |
| x = (baseX + step).coerceAtLeast(0), | |
| y = (baseY + step).coerceAtLeast(0), | |
| val step = (CASCADE_STEP_DP * metrics.density).roundToInt() * (index % CASCADE_WRAP) | |
| val baseX = (metrics.widthPixels - width) / 2 | |
| val baseY = (metrics.heightPixels - height) / 2 | |
| val maxX = (metrics.widthPixels - width).coerceAtLeast(0) | |
| val maxY = (metrics.heightPixels - height).coerceAtLeast(0) | |
| return WindowBounds( | |
| x = (baseX + step).coerceIn(0, maxX), | |
| y = (baseY + step).coerceIn(0, maxY), |
🤖 Prompt for 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.
In
`@floating-window/src/main/java/com/itsaky/androidide/floating/window/InitialBounds.kt`
around lines 22 - 27, The cascaded window position logic in
InitialBounds.calculate/WindowBounds only clamps to zero, so later cascade steps
can place windows beyond the visible viewport. Update the x and y calculations
to clamp within both bounds: keep the current centered base plus cascade step,
but also cap each coordinate at the maximum visible position derived from
metrics.widthPixels/heightPixels minus the window size before returning
WindowBounds, so OverlayLayoutParams.create receives on-screen coordinates.
No description provided.