ADFA-3653: Expand IdeEditorService plugin API with buffer reads, mutations, and file-change events#1194
Merged
Conversation
…tions, and file-change events
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 40 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a process-wide editor event dispatcher, an EditorProvider implementation that exposes rich editor state/mutations to plugins, plugin-api types/methods for editor access, and plugin-manager wiring to register/swap an active editor provider at runtime. Changes
Sequence DiagramsequenceDiagram
participant Activity as EditorHandlerActivity
participant Events as EditorEvents
participant Provider as EditorProviderImpl
participant PluginMgr as PluginManager
participant Plugin as Plugin Code
Activity->>Provider: instantiate EditorProviderImpl(this)
Activity->>PluginMgr: setEditorProvider(provider)
PluginMgr->>Provider: bind delegating provider / replay buffered callbacks
Provider->>Events: register internal listener
Plugin->>PluginMgr: addFileChangeListener(cb)
PluginMgr->>Provider: addFileChangeCallback(cb)
Events->>Provider: notifyFileChanged(file)
Provider->>Plugin: invoke plugin callback(file)
Activity->>Events: notifyFileChanged(null|file)
PluginMgr->>Provider: setEditorProvider(null)
Provider->>Events: remove listener
Provider->>Provider: dispose()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.kt (2)
88-101: Orphaned/duplicate KDoc blocks.Two adjacent KDoc comments are stacked on
pendingFileChangeCallbacks. The first one (“Stable provider handed to every plugin's [IdeEditorServiceImpl]…”) clearly documentsdelegatingEditorProviderand should be moved directly above line 101. As written, the first block is attached to the wrong symbol and Dokka will likely drop it.✏️ Proposed fix
- /** - * Stable provider handed to every plugin's [IdeEditorServiceImpl]. Each call delegates to - * the currently-set [editorProvider] (or returns a no-op default when none is wired). This - * lets the editor activity register/unregister a real provider over its lifecycle without - * having to rebuild already-loaded plugin services. - */ /** * Callbacks added by plugins before any real provider was set. We hold them here and * replay onto a real provider when one is registered, so listener registration doesn't * silently drop on the floor during the early-boot window. */ private val pendingFileChangeCallbacks = java.util.concurrent.CopyOnWriteArraySet<(File?) -> Unit>() + /** + * Stable provider handed to every plugin's [IdeEditorServiceImpl]. Each call delegates to + * the currently-set [editorProvider] (or returns a no-op default when none is wired). This + * lets the editor activity register/unregister a real provider over its lifecycle without + * having to rebuild already-loaded plugin services. + */ private val delegatingEditorProvider = object : IdeEditorServiceImpl.EditorProvider {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.kt` around lines 88 - 101, The first KDoc block describing the "Stable provider handed to every plugin's [IdeEditorServiceImpl]" is misplaced above pendingFileChangeCallbacks; move that KDoc so it sits immediately above the declaration of delegatingEditorProvider (the object implementing IdeEditorServiceImpl.EditorProvider) and leave the second KDoc above pendingFileChangeCallbacks which documents the callback set, ensuring each comment is attached to the correct symbol (pendingFileChangeCallbacks and delegatingEditorProvider) so Dokka picks them up correctly.
140-147: Double-registration viaaddFileChangeCallback.When
addFileChangeCallbackis invoked while a provider is already attached, the callback is added topendingFileChangeCallbacksand forwarded tocurrent(). A subsequentsetEditorProvider(newProvider)then replays all pending callbacks onto the new provider — including ones that were already added when the original provider was live. That's fine on the new provider (since it's a fresh target), but on provider swapsetEditorProvideralso doesprevious.removeFileChangeCallback(cb)which should cleanly detach them from the old one. Worth verifying thatEditorProviderImpl.removeFileChangeCallbackis idempotent when called with an unknown callback (it is —CopyOnWriteArrayList.removeis a no-op). No functional bug, but the "pending" name is now slightly misleading since it also contains already-attached callbacks; consider renaming toregisteredFileChangeCallbacksfor clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.kt` around lines 140 - 147, The field pendingFileChangeCallbacks is misleading because addFileChangeCallback both registers and forwards callbacks to current(), causing the list to contain callbacks already attached to an active provider; rename pendingFileChangeCallbacks to registeredFileChangeCallbacks and update all usages (addFileChangeCallback, removeFileChangeCallback, setEditorProvider, and any replay logic) to use the new name so semantics are clearer, and ensure EditorProviderImpl.removeFileChangeCallback usage remains unchanged (it's idempotent) while updating variable references accordingly.plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImpl.kt (1)
282-282: EmptyreadPermissions/writePermissionswould short-circuit the gate.
hasAllreturnstruewhen the required set is empty (Set.all {}vacuously true). If a caller ever constructsIdeEditorServiceImplwith an emptyreadPermissions/writePermissions(e.g. a test or misconfigured wiring), all permission checks silently pass. The defaults in the constructor prevent this today, but a defensiverequire(readPermissions.isNotEmpty())(or an explicit comment documenting the intent) would prevent foot-guns later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImpl.kt` at line 282, The hasAll function currently returns true for an empty required set (vacuous truth), so if readPermissions or writePermissions are ever constructed empty permission checks will incorrectly pass; update the IdeEditorServiceImpl constructor to enforce non-empty permission sets by adding a defensive require(readPermissions.isNotEmpty()) and require(writePermissions.isNotEmpty()) (or equivalent assertions) so hasAll cannot be called with empty requirement sets, and optionally add a short comment next to hasAll documenting that it expects non-empty required sets.plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt (2)
34-47: Clarify inclusivity convention forSelectionRange.The kdoc says "Inclusive of start, exclusive of end (matches the underlying editor)". In practice, callers will produce ranges from cursor positions where
endis atrightLine/rightColumn— please confirm whether(endLine, endColumn)is interpreted as a zero-width position just past the last selected character (exclusive), and document explicitly whatendColumn == 0means (i.e., a selection that ends at the start ofendLineincluding the newline ofendLine-1). Plugin authors building edit operations onreplaceRangewill need this to be unambiguous; right nowEditorProviderImpl.replaceRangepasses these through to sora'sContent.replace(startLine, startCol, endLine, endCol, ...)which has its own (inclusive-start, exclusive-end) semantics — please add a sentence pinning the contract so plugin authors don't have to reverse-engineer sora.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt` around lines 34 - 47, Update the KDoc for SelectionRange to explicitly state that (endLine,endColumn) is an exclusive end position (i.e., a zero-width position immediately after the last selected character), that endColumn == 0 means the selection ends at the start of endLine and therefore includes the newline character of endLine-1 when applicable, and that this contract (inclusive start, exclusive end) matches the behavior used by EditorProviderImpl.replaceRange which forwards values to sora's Content.replace(startLine,startCol,endLine,endCol,...); edit the KDoc on the SelectionRange data class (and optionally reference CursorPosition) to pin this contract so plugin authors do not need to reverse-engineer sora.
57-119: Public API surface — document threading contract.The mutation methods (
openFile,saveCurrentFile,insertTextAtCursor,replaceSelection,appendToLine, …) are backed byEditorProviderImpl.onMain { … }which blocks the caller on aCountDownLatchwhen invoked off the main thread. Plugin authors calling these from a UI thread, a coroutine dispatcher, or under a lock need to know:
- These calls are synchronous and will block until the main thread has processed them.
saveCurrentFile()is not synchronous in the current impl — it dispatches tolifecycleScope.launchand returnstrueimmediately (seeEditorProviderImpl.saveCurrentFile), so the Boolean does not reflect whether the save actually succeeded. That's inconsistent with the others and deserves a doc note here.Consider adding a KDoc block to the interface summarising thread/async semantics so plugin authors don't guess.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt` around lines 57 - 119, Add a KDoc to the IdeEditorService interface that clearly documents the threading/async contract: state that read methods are safe for callers but mutation methods (list the mutation methods by name: openFile, openFileAt, saveCurrentFile, insertTextAtCursor, replaceSelection, appendToLine, prependToLine, replaceLine, insertLineBefore, deleteLine, replaceRange) are dispatched to the main editor thread via EditorProviderImpl.onMain and therefore block the caller by waiting on a CountDownLatch when invoked off the main thread; explicitly call out the inconsistency that saveCurrentFile does not block in the current implementation (see EditorProviderImpl.saveCurrentFile which uses lifecycleScope.launch and returns true immediately) so its Boolean return value does not indicate completion, and advise plugin authors to treat saveCurrentFile as asynchronous until the implementation is changed.app/src/main/java/com/itsaky/androidide/app/EditorProviderImpl.kt (1)
236-245:deleteLinesemantics for final line.When deleting the last line in the buffer (
line == lineCount - 1andline > 0), the code deletes from(line - 1, colCount(line-1))to(line, colCount(line)). That removes the trailing newline that terminatedline - 1plus the text online, which is conventional. But when the buffer has a single line (line == 0 && lineCount == 1), theelsebranch clears the line content in place (leaving an empty single line). That's a reasonable behaviour but worth documenting sincedeleteLinereturningtrueon a 1-line buffer leaves a non-empty result (the empty line) — different from "file now contains nothing at all". A plugin author may expect the buffer to shrink to 0 lines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/app/EditorProviderImpl.kt` around lines 236 - 245, The deleteLine implementation (deleteLine calling lineEdit) behaves differently for the final line vs. a single-line buffer: when deleting the only line it clears its contents but leaves a single empty line rather than producing a zero-line buffer; update the code and docs to make this explicit and consistent — either change the else branch in deleteLine (the lambda passed to lineEdit) so that when line == 0 && lineCount == 1 you remove the line from the buffer (shrink to 0 lines) instead of deleting its contents in-place, or add KDoc above deleteLine explaining the current semantics (that deleting the sole line yields an empty single line) and add a unit test; reference the deleteLine method and the lineEdit call when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/itsaky/androidide/app/EditorEvents.kt`:
- Around line 66-71: notifyFileClosed currently clears lastActiveFile but leaves
lastNotifiedPath stale, which can cause future reopen events to be deduplicated;
update the notifyFileClosed function to also reset lastNotifiedPath (set it to
null or empty as your dedupe logic expects) whenever you clear lastActiveFile so
that listeners will fire correctly even if notifyFileChanged is not called
(refer to notifyFileClosed, lastActiveFile, lastNotifiedPath and the
notifyFileChanged/_displayedFile observer in EditorHandlerActivity to locate the
related logic).
In `@app/src/main/java/com/itsaky/androidide/app/EditorProviderImpl.kt`:
- Around line 174-185: openFile and openFileAt currently dispatch asynchronous
opens (activity.openFileAsync, activity.openFileAndSelect) and optimistically
return true; change them to call and await the actual suspend open operation on
the main dispatcher and return its real boolean result instead of always true.
Specifically, in EditorProviderImpl.openFile call the activity's suspend
openFile (or the suspend variant of openFileAsync) via
withContext(Dispatchers.Main) and return that boolean; similarly in openFileAt
call and await activity.openFileAndSelect (suspend) with the computed Range and
return its result; ensure you still null-check activity() and preserve
Position/Range construction, and handle delegateFileOpen behavior by returning
the underlying result rather than assuming success.
- Around line 298-316: The onMain helper currently calls latch.await() with no
timeout which can block forever; change it to use latch.await(timeout,
TimeUnit.SECONDS) (or another configured TimeUnit) and handle the boolean
result: if await returns false, set or throw a TimeoutException (or wrap it in
an appropriate runtime exception) so callers get a clear failure instead of
hanging. Update the logic around errorRef/resultRef to surface that timeout
(e.g., set errorRef to the TimeoutException when await times out, then follow
the existing errorRef.get()?.let { throw it } path) and pick a sensible timeout
constant; keep the existing finally/countDown behavior and retain the `@Suppress`
unchecked cast handling for resultRef.
- Around line 187-195: saveCurrentFile() currently schedules the save
asynchronously (uses activity.lifecycleScope.launch) and returns true
immediately, so the Boolean misleads callers; change
EditorProviderImpl.saveCurrentFile to perform the save synchronously on the main
thread so the return value reflects completion: replace the
lifecycleScope.launch path with an onMain { ... } call and invoke
activity.saveResult(index, SaveResult()) inside it while blocking until it
completes (e.g., wrap the save call in runBlocking or switch saveResult to a
synchronous API) so callers of saveCurrentFile() can rely on the returned
Boolean; reference: EditorProviderImpl.saveCurrentFile, activity(),
activity.saveResult(...), and current lifecycleScope.launch usage.
In `@plugin-api/build.gradle.kts`:
- Around line 9-13: The plugin-api change raises compileSdk/minSdk and requires
updating the dependent plugins to match: update the minSdk property in
markdown-preview-plugin, keystore-generator-plugin, and apk-viewer-plugin to at
least 28 (e.g., change minSdk = 26 to minSdk = 28) in each plugin's
build.gradle.kts so manifest merging won't fail; ensure any module-level
defaultConfig/minSdk declarations or productFlavor overrides in those plugins
are adjusted likewise and run a build to verify no further manifest/AGP
conflicts remain.
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImpl.kt`:
- Around line 311-328: The current isFileAccessAllowedDefault uses
canonicalPath.startsWith(it) which allows sibling matches; change it to compare
against canonicalized allowlist entries and ensure the match is an exact path
segment by canonicalizing each defaultAllowedPaths entry (use
File(path).canonicalPath) and then check either canonicalPath == allowedEntry or
canonicalPath.startsWith(allowedEntry + File.separator); update
defaultAllowedPaths to produce canonical paths lazily and make
isFileAccessAllowedDefault iterate those canonical entries when validating.
---
Nitpick comments:
In `@app/src/main/java/com/itsaky/androidide/app/EditorProviderImpl.kt`:
- Around line 236-245: The deleteLine implementation (deleteLine calling
lineEdit) behaves differently for the final line vs. a single-line buffer: when
deleting the only line it clears its contents but leaves a single empty line
rather than producing a zero-line buffer; update the code and docs to make this
explicit and consistent — either change the else branch in deleteLine (the
lambda passed to lineEdit) so that when line == 0 && lineCount == 1 you remove
the line from the buffer (shrink to 0 lines) instead of deleting its contents
in-place, or add KDoc above deleteLine explaining the current semantics (that
deleting the sole line yields an empty single line) and add a unit test;
reference the deleteLine method and the lineEdit call when making the change.
In
`@plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt`:
- Around line 34-47: Update the KDoc for SelectionRange to explicitly state that
(endLine,endColumn) is an exclusive end position (i.e., a zero-width position
immediately after the last selected character), that endColumn == 0 means the
selection ends at the start of endLine and therefore includes the newline
character of endLine-1 when applicable, and that this contract (inclusive start,
exclusive end) matches the behavior used by EditorProviderImpl.replaceRange
which forwards values to sora's
Content.replace(startLine,startCol,endLine,endCol,...); edit the KDoc on the
SelectionRange data class (and optionally reference CursorPosition) to pin this
contract so plugin authors do not need to reverse-engineer sora.
- Around line 57-119: Add a KDoc to the IdeEditorService interface that clearly
documents the threading/async contract: state that read methods are safe for
callers but mutation methods (list the mutation methods by name: openFile,
openFileAt, saveCurrentFile, insertTextAtCursor, replaceSelection, appendToLine,
prependToLine, replaceLine, insertLineBefore, deleteLine, replaceRange) are
dispatched to the main editor thread via EditorProviderImpl.onMain and therefore
block the caller by waiting on a CountDownLatch when invoked off the main
thread; explicitly call out the inconsistency that saveCurrentFile does not
block in the current implementation (see EditorProviderImpl.saveCurrentFile
which uses lifecycleScope.launch and returns true immediately) so its Boolean
return value does not indicate completion, and advise plugin authors to treat
saveCurrentFile as asynchronous until the implementation is changed.
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.kt`:
- Around line 88-101: The first KDoc block describing the "Stable provider
handed to every plugin's [IdeEditorServiceImpl]" is misplaced above
pendingFileChangeCallbacks; move that KDoc so it sits immediately above the
declaration of delegatingEditorProvider (the object implementing
IdeEditorServiceImpl.EditorProvider) and leave the second KDoc above
pendingFileChangeCallbacks which documents the callback set, ensuring each
comment is attached to the correct symbol (pendingFileChangeCallbacks and
delegatingEditorProvider) so Dokka picks them up correctly.
- Around line 140-147: The field pendingFileChangeCallbacks is misleading
because addFileChangeCallback both registers and forwards callbacks to
current(), causing the list to contain callbacks already attached to an active
provider; rename pendingFileChangeCallbacks to registeredFileChangeCallbacks and
update all usages (addFileChangeCallback, removeFileChangeCallback,
setEditorProvider, and any replay logic) to use the new name so semantics are
clearer, and ensure EditorProviderImpl.removeFileChangeCallback usage remains
unchanged (it's idempotent) while updating variable references accordingly.
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImpl.kt`:
- Line 282: The hasAll function currently returns true for an empty required set
(vacuous truth), so if readPermissions or writePermissions are ever constructed
empty permission checks will incorrectly pass; update the IdeEditorServiceImpl
constructor to enforce non-empty permission sets by adding a defensive
require(readPermissions.isNotEmpty()) and require(writePermissions.isNotEmpty())
(or equivalent assertions) so hasAll cannot be called with empty requirement
sets, and optionally add a short comment next to hasAll documenting that it
expects non-empty required sets.
🪄 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: c85b567c-3f1b-4a7c-8797-1b65cbb4f43d
📒 Files selected for processing (7)
app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.ktapp/src/main/java/com/itsaky/androidide/app/EditorEvents.ktapp/src/main/java/com/itsaky/androidide/app/EditorProviderImpl.ktplugin-api/build.gradle.ktsplugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImpl.kt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/src/main/java/com/itsaky/androidide/app/EditorEvents.kt (1)
54-59: Silent listener failures hurt diagnosability.
catch (_: Exception) {}swallows every listener error with no log line. When a plugin'sFileChangeListenermisbehaves, there'll be no trace at all — not even in debug. Consider aLog.w(TAG, "listener failed", e)so plugin authors (and your own CI) can see it.🔧 Suggested change
fileChangeListeners.forEach { listener -> try { listener(file) - } catch (_: Exception) { + } catch (e: Exception) { + Log.w(TAG, "file-change listener threw", e) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/app/EditorEvents.kt` around lines 54 - 59, The silent catch in the fileChangeListeners.forEach block (in EditorEvents.kt) hides plugin exceptions; replace the empty catch with logging the Throwable so failures are visible: catch the exception (e) from listener(file) and call Log.w(TAG, "FileChangeListener failed for file: " + file, e) (or use your processLogger) so the error and context are recorded, but still do not rethrow to preserve existing behavior.plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt (1)
49-55: Document the threading contract ofFileChangeListener.Plugin authors will want to know which thread
onFileChangedfires on before deciding whether it's safe to touch UI / do I/O in the callback. GivenEditorEvents.notifyFileChangedis invoked fromEditorHandlerActivity's_displayedFileobserver, this is effectively main-thread — worth stating so consumers don't defensivelypostor block.🔧 Suggested KDoc
/** * Notified when the user switches between open files. The new active file is passed, - * or null if all files are closed. + * or null if all files are closed. + * + * Callbacks are delivered on the IDE's main thread. Do not perform blocking I/O here; + * dispatch to a background scope if needed. */ fun interface FileChangeListener { fun onFileChanged(file: File?) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt` around lines 49 - 55, Add KDoc to the FileChangeListener interface clarifying its threading contract: state that FileChangeListener.onFileChanged is invoked on the main (UI) thread because EditorEvents.notifyFileChanged is called from EditorHandlerActivity's _displayedFile observer; instruct plugin authors that they must dispatch to background threads for heavy I/O or use main-thread constructs for UI work as appropriate. Mention the specific symbols FileChangeListener, onFileChanged, EditorEvents.notifyFileChanged, and EditorHandlerActivity._displayedFile in the doc so readers can trace why the callback is main-thread.plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImpl.kt (1)
76-79: Consider exposingdispose()on theIdeEditorServiceinterface to clarify lifecycle requirements.
dispose()is currently only accessible on the concreteIdeEditorServiceImpltype, not through the interface contract. WhilePluginManagerdoes correctly calldispose()during plugin unload via an explicit type check, this pattern is fragile—external code cannot reliably dispose instances without knowing the concrete type. Addingdispose()to the interface (or implementingAutoCloseable) would make the cleanup contract explicit and prevent accidental listener leaks if instances are ever passed outside the plugin manager context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImpl.kt` around lines 76 - 79, Add the cleanup API to the interface so callers can dispose without knowing the concrete type: add a dispose() method to the IdeEditorService interface (or have the interface extend AutoCloseable and implement close() in IdeEditorServiceImpl) and move the current cleanup logic from IdeEditorServiceImpl.dispose() (removing editorProvider.removeFileChangeCallback(internalFileChangeCallback) and fileChangeListeners.clear()) to the interface implementation; update PluginManager to call the interface method instead of type-checking the concrete class. Ensure method signatures and documentation reflect the lifecycle requirement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/itsaky/androidide/app/EditorProviderImpl.kt`:
- Around line 233-236: insertLineBefore currently calls insert(line, 0, payload)
but allows line == content.lineCount via lineEdit(..., existing = false), which
causes Content.insert to throw for end-of-buffer indices; update
insertLineBefore to special-case the end-of-buffer: if line ==
content.lineCount, call insert at the end of the last line (use the last line's
length as the column) or use an append/insert-at-end API instead, otherwise keep
the existing insert(line, 0, payload) path; locate this logic in
insertLineBefore and adjust the branch that invokes insert via lineEdit to avoid
passing lineCount into Content.insert.
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImpl.kt`:
- Around line 325-333: The /tmp entry in defaultAllowedPaths is incorrect:
replace the hardcoded singular "/tmp/CodeOnTheGoProject" with a path that uses
the Environment.PROJECTS_FOLDER value (the local variable projects) so it
matches the other entries; update the list inside the defaultAllowedPaths lazy
block to build the /tmp entry using "/tmp/$projects" (then let the existing map
{ runCatching { File(it).canonicalPath }... } handle canonicalization) so
canonicalized allowed paths align with actual project directories.
---
Nitpick comments:
In `@app/src/main/java/com/itsaky/androidide/app/EditorEvents.kt`:
- Around line 54-59: The silent catch in the fileChangeListeners.forEach block
(in EditorEvents.kt) hides plugin exceptions; replace the empty catch with
logging the Throwable so failures are visible: catch the exception (e) from
listener(file) and call Log.w(TAG, "FileChangeListener failed for file: " +
file, e) (or use your processLogger) so the error and context are recorded, but
still do not rethrow to preserve existing behavior.
In
`@plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.kt`:
- Around line 49-55: Add KDoc to the FileChangeListener interface clarifying its
threading contract: state that FileChangeListener.onFileChanged is invoked on
the main (UI) thread because EditorEvents.notifyFileChanged is called from
EditorHandlerActivity's _displayedFile observer; instruct plugin authors that
they must dispatch to background threads for heavy I/O or use main-thread
constructs for UI work as appropriate. Mention the specific symbols
FileChangeListener, onFileChanged, EditorEvents.notifyFileChanged, and
EditorHandlerActivity._displayedFile in the doc so readers can trace why the
callback is main-thread.
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImpl.kt`:
- Around line 76-79: Add the cleanup API to the interface so callers can dispose
without knowing the concrete type: add a dispose() method to the
IdeEditorService interface (or have the interface extend AutoCloseable and
implement close() in IdeEditorServiceImpl) and move the current cleanup logic
from IdeEditorServiceImpl.dispose() (removing
editorProvider.removeFileChangeCallback(internalFileChangeCallback) and
fileChangeListeners.clear()) to the interface implementation; update
PluginManager to call the interface method instead of type-checking the concrete
class. Ensure method signatures and documentation reflect the lifecycle
requirement.
🪄 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: 3a69dc82-ed4d-4790-9882-5df889d242de
📒 Files selected for processing (4)
app/src/main/java/com/itsaky/androidide/app/EditorEvents.ktapp/src/main/java/com/itsaky/androidide/app/EditorProviderImpl.ktplugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeServices.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeEditorServiceImpl.kt
dara-abijo-adfa
approved these changes
Apr 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Screen.Recording.2026-04-16.at.23.24.51.mov