ADFA-4928 create a single manager for plugins and templates - #1627
ADFA-4928 create a single manager for plugins and templates#1627hal-eisen-adfa wants to merge 16 commits into
Conversation
Adds the Compose plugin/buildFeatures/dependencies to app/build.gradle.kts, mirroring the floating-window/profiler modules' setup, plus a shared ManagerTheme composable that resolves Theme.AndroidIDE's Material3 attrs (same technique as FloatingTheme). This is the first commit of the Plugin Manager + Template Manager merge (ADR 0009 requires new screens to be Compose); the theme/build wiring lands separately from any screen code so it's independently reviewable and buildable.
Rebuilds PluginManagerActivity's screen in Jetpack Compose (ADR 0009), preserving every capability of the old RecyclerView/dialogs UI: install via SAF picker, enable/disable/uninstall, overwrite and signature-mismatch conflict handling, restart prompt, and the discover-plugins action. PluginManagerViewModel/PluginRepository are reused unchanged. The six long-press tooltip anchor points collapse to two (list items, and the screen's background/empty state) since they all showed the same TooltipTag.PLUGIN_MANAGER content anyway - verified on-device that the long-press still correctly reaches TooltipManager. Also moves two dialogs' hardcoded English strings (uninstall confirmation, plugin details labels) into string resources. Note: taken together with the prior commit, this is the buildable/ tested state; the prior commit's PluginListAdapter.kt deletion was accidentally bundled with the build-wiring commit rather than this one, so that earlier commit alone doesn't compile in isolation - only the combined history does (verified via :app:assembleV8Debug and a manual on-device pass).
Ports the parsing/model layer from appdevforall/TemplateManagerPlugin (CgtTemplateReader, TemplateMetadata/CgtFileItem, plus their unit tests) into the app module as the basis for the new Templates tab. Adds TemplateRepository/TemplateRepositoryImpl, which reimplement the plugin's install/uninstall/delete semantics as direct file operations on Environment.TEMPLATES_DIR + the Downloads folder, since the host app doesn't need IdeTemplateService's plugin-facing permission gate. Provenance (bundled/plugin/user) is inferred from the same filename convention IdeTemplateServiceImpl/PluginProjectManager already use. Adds TemplateManagerViewModel (UDF shape matching PluginManagerViewModel) and a Koin di/TemplateModule, registered in IDEApplication alongside pluginModule. No UI yet - this commit is data-layer only. CgtTemplateReaderTest needs @RunWith(RobolectricTestRunner::class): org.json.JSONObject throws "not mocked" under a plain JVM unit test, same as other app-module tests that touch real android.jar classes.
Adds the Compose UI for the Templates tab, backed by the data layer from the previous commit: TemplateListItem (card - tapping only opens the multi-template sub-list, matching the reference plugin's design), TemplateManagerDialogs (delete confirmation, file-level details, per-template details, multi-template sub-list), and TemplateManagerScreen (content composable wiring the ViewModel's uiState/uiEffect, same long-press pointerInput tooltip shim as the Plugins tab, new TooltipTag.TEMPLATE_MANAGER). TemplateManagerScreen is content-only (no Scaffold/TopAppBar/FAB) - unlike the Plugins tab there's no install-flow FAB, matching the ported plugin's passive Downloads-folder scanning. It's meant to be composed as one tab's body inside the shared manager screen; wiring the two tabs together is the next commit.
New ManagerScreen composable owns the shared Scaffold/TopAppBar/TabRow + HorizontalPager, hosting Plugins and Templates as pages (Plugins default). The FAB and discover-plugins action only render on the Plugins tab, since Templates is a passive Downloads-folder scan with no equivalent action. Refactors the old PluginManagerScreen into PluginManagerContent - a Scaffold-free content composable, matching TemplateManagerScreen's shape - so both tabs plug into ManagerScreen's single Scaffold instead of nesting their own. PluginManagerActivity now resolves both PluginManagerViewModel and TemplateManagerViewModel and renders ManagerScreen; its class name and entry points (Settings, the crash-recovery dialog) are unchanged. Updates ARCHITECTURE.md: this is the first production Compose screen in app (ADR 0009), and templates/manager is a new data-layer package. Verified end-to-end on a physical device: assembleV8Debug, installed APK, exercised both tabs from Settings -> Plugin Manager. Templates tab correctly scanned Environment.TEMPLATES_DIR + Downloads (found real pre-existing .cgt fixtures on the test device), and a full install/uninstall round-trip moved files between Downloads and TEMPLATES_DIR and refreshed the list correctly. No crashes.
…ity + docs) Finishes the previous commit: a staging mistake (a `git add` call hit a stale pathspec and aborted before reaching these files) left `81e3797ab` with only the new `ManagerScreen.kt` and a content-less file rename, referencing a `PluginManagerContent` composable that didn't exist yet in that commit alone - not independently buildable. This commit adds what was missed: the actual `PluginManagerContent.kt` refactor (Scaffold/TopAppBar/FAB stripped out, now content-only), `PluginManagerActivity.kt` wired to render `ManagerScreen` with both view models, the `ARCHITECTURE.md` updates, and the `title_manager` string. Combined history through this commit compiles (:app:compileV8DebugKotlin) and matches what was already verified end-to-end on-device in the previous message.
The Settings entry that opens the merged Plugins/Templates screen was still titled "Plugin Manager" with a summary mentioning "extensions" (the old plugin-only wording). Renamed to "Extensions Manager" with a summary reflecting both tabs it now opens: "Manage IDE plugins and templates". Verified on-device: preferences list and the opened screen both render correctly.
PluginModule's Koin factories called Context.filesDir directly, which does a real File.exists() check on every call, not just the first. That trips StrictMode's DiskReadViolation the first time the Extensions Manager screen resolves PluginRepository/PluginManagerViewModel on the main thread. Cache the resolved File once, off-main, during app startup (IDEApplication.cachedFilesDir), and have PluginModule read that instead - later reads are then a plain field access rather than a syscall. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The SAF picker launched with "*/*", showing every file regardless of type. SAF filters by MIME, not extension, and .cgp has no registered MIME type, so the closest working filter is "application/octet-stream" - what document providers report for files with an unrecognized extension. This hides files with a known type (zips, jars, images, ...) while leaving .cgp files selectable. isSupportedPluginFile() still validates the actual pick, since this is an approximation, not an exact extension filter (SAF has no such thing). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 Walkthrough
WalkthroughThe Extensions Manager replaces the legacy plugin UI with Compose. It adds template parsing, storage operations, UDF state, ViewModels, dialogs, tabs, theming, dependency injection, validation, and tests. ChangesExtensions manager
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PluginManagerActivity
participant ManagerScreen
participant TemplateManagerScreen
participant TemplateManagerViewModel
participant TemplateRepository
PluginManagerActivity->>ManagerScreen: Render manager tabs
ManagerScreen->>TemplateManagerScreen: Show Templates tab
TemplateManagerScreen->>TemplateManagerViewModel: Dispatch template event
TemplateManagerViewModel->>TemplateRepository: Load or mutate template files
TemplateRepository-->>TemplateManagerViewModel: Return Result
TemplateManagerViewModel-->>TemplateManagerScreen: Emit state and effects
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (3)
app/src/main/java/com/itsaky/androidide/viewmodels/TemplateManagerViewModel.kt (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the project logger facade.
android.util.Logand interpolated messages bypass the required structured logging contract. ReplaceTAGwithLoggerFactoryand use placeholders for dynamic values.As per coding guidelines, use SLF4J
LoggerFactorywith structured placeholders.Also applies to: 55-61, 77-82, 97-102, 124-129
🤖 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/viewmodels/TemplateManagerViewModel.kt` at line 3, Replace android.util.Log and the TAG-based logging in TemplateManagerViewModel with the project’s SLF4J LoggerFactory facade. Update all affected logging calls, including the referenced ranges, to use structured placeholder arguments instead of interpolated messages, and remove the obsolete TAG declaration/import.Source: Coding guidelines
app/src/main/java/com/itsaky/androidide/ui/models/TemplateManagerUiState.kt (1)
6-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd KDoc for the new public manager APIs.
The new public types and composables lack contract documentation. Document state ownership, effect delivery, destructive-action behavior, and caller expectations.
app/src/main/java/com/itsaky/androidide/ui/models/TemplateManagerUiState.kt#L6-L76: add KDoc for the state, event, effect, and operation contracts.app/src/main/java/com/itsaky/androidide/viewmodels/TemplateManagerViewModel.kt#L40-L49: document event dispatch behavior and threading expectations.app/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginListItem.kt#L45-L55: document plugin action and tooltip callback contracts.app/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginManagerDialogs.kt#L25-L123: document each dialog confirmation and dismissal contract.As per coding guidelines, public classes and functions require KDoc or Javadoc.
🤖 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/ui/models/TemplateManagerUiState.kt` around lines 6 - 76, Add KDoc for the public contracts in app/src/main/java/com/itsaky/androidide/ui/models/TemplateManagerUiState.kt:6-76, covering TemplateManagerUiState, TemplateManagerUiEvent, TemplateManagerUiEffect, and TemplateOperation, including state ownership, effect delivery, destructive actions, and caller expectations. Document event dispatch behavior and threading expectations for the relevant API in app/src/main/java/com/itsaky/androidide/viewmodels/TemplateManagerViewModel.kt:40-49. Document plugin action and tooltip callback contracts in app/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginListItem.kt:45-55, and add KDoc for each dialog’s confirmation and dismissal contract in app/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginManagerDialogs.kt:25-123.Source: Coding guidelines
app/src/main/java/com/itsaky/androidide/ui/compose/templates/TemplateManagerScreen.kt (1)
89-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated error-flashbar presentation in both tab bodies. Both tab contents build the same error flashbar: the
5000LversusDURATION_INDEFINITEduration heuristic, the error icon, the message, the conditional copy action with a clipboard write, andshowOnUiThread(). Only the clip label resource differs. The shared root cause is one missing helper.
app/src/main/java/com/itsaky/androidide/ui/compose/templates/TemplateManagerScreen.kt#L89-L109: replace this block with a call to a shared helper, for exampleComponentActivity.showEffectError(messageResId, formatArgs, R.string.msg_template_error_clip_label), and define the duration as a named constant.app/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginManagerContent.kt#L133-L153: replace this block with the same helper, passingR.string.msg_plugin_error_clip_label.Reuse existing helpers, extract duplicated logic, replace repeated magic values with named constants, as required by the coding guidelines.
🤖 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/ui/compose/templates/TemplateManagerScreen.kt` around lines 89 - 109, The error flashbar presentation is duplicated across both tab bodies. In app/src/main/java/com/itsaky/androidide/ui/compose/templates/TemplateManagerScreen.kt:89-109 and app/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginManagerContent.kt:133-153, extract the shared logic into a ComponentActivity helper that accepts the message resource, format arguments, and clip-label resource; replace both blocks with calls to it, using the template and plugin clip labels respectively. Define the 5000L duration as a named constant and preserve the conditional copy action and indefinite duration behavior.Source: Coding guidelines
🤖 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/activities/PluginManagerActivity.kt`:
- Around line 39-47: Update ManagerScreen’s Scaffold to use WindowInsets(0) for
contentWindowInsets, since binding.root already applies system-bar padding; keep
the activity’s existing root padding and prevent duplicate inset spacing around
the tab row, pager, and FAB.
In `@app/src/main/java/com/itsaky/androidide/app/IDEApplication.kt`:
- Around line 147-155: Ensure IDEApplication.cachedFilesDir is initialized off
the main thread before Koin can resolve PluginManagerViewModel: update
IDEApplication.cachedFilesDir and the warmup in
DeviceProtectedApplicationLoader.load() so initialization completes before
ensureKoinStarted() exposes pluginModule, and verify PluginModule uses the
already-initialized cache without triggering lazy initialization on the main
thread. Apply the required changes in
app/src/main/java/com/itsaky/androidide/app/IDEApplication.kt (lines 147-155),
app/src/main/java/com/itsaky/androidide/app/DeviceProtectedApplicationLoader.kt
(lines 137-145), and app/src/main/java/com/itsaky/androidide/di/PluginModule.kt
(lines 19-32).
In
`@app/src/main/java/com/itsaky/androidide/repositories/TemplateRepositoryImpl.kt`:
- Around line 81-84: Update the template installation logic around the file-copy
operations in TemplateRepositoryImpl so the result of item.file.delete() is
validated. If source deletion fails, remove the newly created dest copy and
return the operation’s defined failure/recovery result instead of reloading
providers or reporting success; apply the same handling to both affected
methods.
- Line 3: Replace android.util.Log usage throughout TemplateRepositoryImpl with
an SLF4J logger created via LoggerFactory. Update the referenced logging calls
to use appropriate SLF4J levels and structured `{}` placeholders with arguments
instead of string concatenation or interpolation.
- Around line 77-85: Update installTemplate and the corresponding
uninstallTemplate flow to detect an existing destination before copying and
refuse the operation unless an explicit user-confirmed replacement is provided.
Remove the unconditional overwrite behavior in File.copyTo, preserving
bundled-provenance protection and preventing unrelated same-name archives from
being replaced.
- Around line 32-36: Replace the broad runCatching usage in listTemplateFiles
and the other indicated repository I/O paths with explicit exception handling:
catch only expected file, parsing, and provider exceptions, rethrow
CancellationException, and handle unexpected failures explicitly rather than
using onFailure solely to log them. Preserve each method’s existing Result
success/failure contract and logging context.
In
`@app/src/main/java/com/itsaky/androidide/templates/manager/models/CgtFileItem.kt`:
- Around line 5-11: Document the public model contracts with KDoc for
TemplateMetadata and CgtFileItem. Describe each model’s purpose, clarify the
semantics of installed and provenance, and explain how a single archive can
contain multiple templates; retain the existing optionalTags field documentation
and add property-level KDoc where needed for these non-obvious meanings.
In `@app/src/main/java/com/itsaky/androidide/ui/compose/common/FileImage.kt`:
- Around line 35-37: Update the image-loading logic in FileImage to read source
bounds first, calculate an inSampleSize that limits the decoded bitmap to the
40.dp icon’s required dimensions, and decode using those options before
converting with asImageBitmap. Replace broad runCatching with targeted
recoverable-failure handling, while allowing CancellationException to propagate.
- Around line 30-38: Update the file-loading logic in the produceState block so
the file.exists() check is performed inside withContext(Dispatchers.IO),
alongside BitmapFactory.decodeFile(). Remove the preceding takeIf existence
check while preserving the null handling and bitmap conversion behavior.
In `@app/src/main/java/com/itsaky/androidide/ui/compose/ManagerScreen.kt`:
- Around line 71-98: The discover-plugins IconButton and install
FloatingActionButton in ManagerScreen need idetooltips long-press support. Add
the established tooltip anchor and long-press handler to both controls, using
the appropriate tooltip identifiers and preserving the existing
UrlManager.openUrl and PluginManagerUiEvent.OpenFilePicker actions.
- Around line 63-70: Replace android.R.string.cancel in ManagerScreen’s
navigationIcon contentDescription with the resources module’s cd_navigate_back
string, and add that cd_navigate_back resource with the “Navigate back” text to
its strings.xml.
In
`@app/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginListItem.kt`:
- Around line 118-143: Update the plugin action menu in PluginListItem so only
the enable/disable options remain guarded by plugin.isLoaded; render the
uninstall DropdownMenuItem for every listed plugin, preserving its existing
menuExpanded reset and onUninstall callback.
In
`@app/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginManagerContent.kt`:
- Line 7: Replace the android.util.Log import and Log.w usages in
PluginManagerContent with an SLF4J LoggerFactory logger, using structured
placeholders and an appropriate warning level. Update all referenced locations,
including the additional occurrences, while preserving the existing messages and
values.
- Around line 163-174: In the OpenFilePicker branch handling
filePickerLauncher.launch, replace the broad Exception catch with an explicit
ActivityNotFoundException catch, add the required import, and log the caught
throwable before showing the existing no-file-manager error.
- Around line 57-58: Move the content-URI filename validation out of the picker
callback and into the relevant ViewModel using a background dispatcher, ensuring
Uri.getFileName is not called on the UI thread. Update the existing effect flow
to return the validation result and have the picker handling consume that
result, while preserving the current PLUGIN_EXTENSION matching behavior.
In
`@app/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginManagerDialogs.kt`:
- Around line 126-130: Update the DetailRow composable to use a positional
detail-row format string defined in the resources module, and retrieve it with
stringResource while passing label and value as arguments. Remove the inline
"$label: $value" construction so translators can control ordering, spacing, and
punctuation.
In
`@app/src/main/java/com/itsaky/androidide/ui/compose/templates/TemplateListItem.kt`:
- Around line 82-87: The template count currently uses a fixed plural string.
Update TemplateListItem.kt lines 82-87 to import and use pluralStringResource
with R.plurals.template_contains_count and item.templates.size; replace
resources/src/main/res/values/strings.xml line 1270’s template_contains_count
string with singular and plural forms in a plurals resource.
- Around line 61-64: Update the combinedClickable usage in TemplateListItem so
single-template cards are not treated as clickable or expose tap press
semantics. Apply click handling only when item.hasMultipleTemplates and
onViewTemplates are valid, while preserving onLongPressTooltip for long-press
behavior.
In
`@app/src/main/java/com/itsaky/androidide/viewmodels/TemplateManagerViewModel.kt`:
- Around line 33-34: Change the _uiEffect channel in TemplateManagerViewModel to
use buffering so effects emitted before a collector is ready are retained, and
update the existing viewModelScope emission paths to send through the channel
without dropping results. Add a test that emits an effect before collection
begins, then starts collecting and verifies the effect is received.
In
`@app/src/test/java/com/itsaky/androidide/templates/manager/models/CgtFileItemTest.kt`:
- Around line 3-6: Enable JUnit Jupiter for app unit tests and migrate
CgtFileItemTest to org.junit.jupiter.api.Test with Truth assertions, updating
its test annotations and assertion imports/usages. In
app/src/test/java/com/itsaky/androidide/templates/manager/parsing/CgtTemplateReaderTest.kt
at lines 3-7, retain JUnit 4 and RobolectricTestRunner compatibility while
replacing only its assertion imports/usages with Truth; do not migrate its Test
annotation to Jupiter.
---
Nitpick comments:
In
`@app/src/main/java/com/itsaky/androidide/ui/compose/templates/TemplateManagerScreen.kt`:
- Around line 89-109: The error flashbar presentation is duplicated across both
tab bodies. In
app/src/main/java/com/itsaky/androidide/ui/compose/templates/TemplateManagerScreen.kt:89-109
and
app/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginManagerContent.kt:133-153,
extract the shared logic into a ComponentActivity helper that accepts the
message resource, format arguments, and clip-label resource; replace both blocks
with calls to it, using the template and plugin clip labels respectively. Define
the 5000L duration as a named constant and preserve the conditional copy action
and indefinite duration behavior.
In `@app/src/main/java/com/itsaky/androidide/ui/models/TemplateManagerUiState.kt`:
- Around line 6-76: Add KDoc for the public contracts in
app/src/main/java/com/itsaky/androidide/ui/models/TemplateManagerUiState.kt:6-76,
covering TemplateManagerUiState, TemplateManagerUiEvent,
TemplateManagerUiEffect, and TemplateOperation, including state ownership,
effect delivery, destructive actions, and caller expectations. Document event
dispatch behavior and threading expectations for the relevant API in
app/src/main/java/com/itsaky/androidide/viewmodels/TemplateManagerViewModel.kt:40-49.
Document plugin action and tooltip callback contracts in
app/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginListItem.kt:45-55,
and add KDoc for each dialog’s confirmation and dismissal contract in
app/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginManagerDialogs.kt:25-123.
In
`@app/src/main/java/com/itsaky/androidide/viewmodels/TemplateManagerViewModel.kt`:
- Line 3: Replace android.util.Log and the TAG-based logging in
TemplateManagerViewModel with the project’s SLF4J LoggerFactory facade. Update
all affected logging calls, including the referenced ranges, to use structured
placeholder arguments instead of interpolated messages, and remove the obsolete
TAG declaration/import.
🪄 Autofix
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 Plus
Run ID: 68cd254b-8078-46a4-bc86-93930cc11c63
📒 Files selected for processing (32)
ARCHITECTURE.mdapp/build.gradle.ktsapp/src/main/java/com/itsaky/androidide/activities/PluginManagerActivity.ktapp/src/main/java/com/itsaky/androidide/adapters/PluginListAdapter.ktapp/src/main/java/com/itsaky/androidide/app/DeviceProtectedApplicationLoader.ktapp/src/main/java/com/itsaky/androidide/app/IDEApplication.ktapp/src/main/java/com/itsaky/androidide/di/PluginModule.ktapp/src/main/java/com/itsaky/androidide/di/TemplateModule.ktapp/src/main/java/com/itsaky/androidide/repositories/TemplateRepository.ktapp/src/main/java/com/itsaky/androidide/repositories/TemplateRepositoryImpl.ktapp/src/main/java/com/itsaky/androidide/templates/manager/models/CgtFileItem.ktapp/src/main/java/com/itsaky/androidide/templates/manager/parsing/CgtTemplateReader.ktapp/src/main/java/com/itsaky/androidide/ui/compose/ManagerScreen.ktapp/src/main/java/com/itsaky/androidide/ui/compose/common/FileImage.ktapp/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginListItem.ktapp/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginManagerContent.ktapp/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginManagerDialogs.ktapp/src/main/java/com/itsaky/androidide/ui/compose/templates/TemplateListItem.ktapp/src/main/java/com/itsaky/androidide/ui/compose/templates/TemplateManagerDialogs.ktapp/src/main/java/com/itsaky/androidide/ui/compose/templates/TemplateManagerScreen.ktapp/src/main/java/com/itsaky/androidide/ui/compose/theme/ManagerTheme.ktapp/src/main/java/com/itsaky/androidide/ui/models/TemplateManagerUiState.ktapp/src/main/java/com/itsaky/androidide/viewmodels/TemplateManagerViewModel.ktapp/src/main/res/layout/activity_plugin_manager.xmlapp/src/main/res/layout/dialog_install_plugin.xmlapp/src/main/res/layout/item_plugin.xmlapp/src/main/res/menu/menu_plugin_manager.xmlapp/src/test/java/com/itsaky/androidide/templates/manager/models/CgtFileItemTest.ktapp/src/test/java/com/itsaky/androidide/templates/manager/parsing/CgtTemplateReaderTest.ktgradle/libs.versions.tomlidetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.ktresources/src/main/res/values/strings.xml
💤 Files with no reviewable changes (4)
- app/src/main/res/menu/menu_plugin_manager.xml
- app/src/main/res/layout/item_plugin.xml
- app/src/main/java/com/itsaky/androidide/adapters/PluginListAdapter.kt
- app/src/main/res/layout/dialog_install_plugin.xml
Code reviewFound 2 issues:
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
| item.file.copyTo(dest, overwrite = true) | ||
| item.file.delete() |
There was a problem hiding this comment.
installTemplate and uninstallTemplate discard the result of File.delete(), but deleteDownloadFile (L105-113) throws IOException when the same call fails. Worth making the three consistent.
If this delete fails, the copy in templatesDir and the original in downloadDir both survive. scanTemplates() merges the two directories, so the next refresh lists the same .cgt twice - one row with installed = true, one with installed = false. runCatching still yields Result.success, so nothing reaches the user. Lines 98-99 have the same shape in the uninstall direction, where the leftover is the store copy the user just asked to remove.
Matching deleteDownloadFile is enough - the surrounding runCatching converts it to the Result.failure the ViewModel already handles:
if (!item.file.delete()) {
throw IOException("Failed to delete ${item.file.absolutePath}")
}One caveat on the uninstall path: throwing after the restore copy succeeds leaves the template in both places, which is still the safe direction (the user's copy survives) but reports failure. A warning log there instead of a throw may fit better than in installTemplate.
Doc drift:
|
| Line | Current text | Status after this PR |
|---|---|---|
| 114 | "renders a different icon based on whether the system is in light or dark mode (PluginListAdapter.kt:61)" |
Behavior is correct. The pointer is dead. The code moved to PluginListItem.kt:69-70. |
| 142 | "Icons are decoded with Glide (PluginListAdapter.kt:69), which handles raster formats only." |
Pointer is dead, and the decoder name is now wrong. FileImage.kt:36 calls BitmapFactory.decodeFile. |
| 254 | "The selection happens in PluginListAdapter.kt:61 via isSystemInDarkMode()." |
Behavior is correct. The pointer is dead. |
This is not a pre-existing issue. The Glide sentence was true before this PR: the old adapter imported Glide and called Glide.with(pluginIcon).load(iconFile) (line 71 on stage). This PR replaces that call with BitmapFactory.decodeFile, so this PR is what makes the doc wrong. Glide itself stays in the module - TemplateListAdapter.kt still uses it, so the dependency is not orphaned.
Impact is moderate. A plugin author reads this doc to learn where to put icons and which formats to use. Both answers stay correct: BitmapFactory decodes PNG, WebP, and JPEG, and it does not decode SVG or vector XML, so the "raster formats only" rule survives the swap. Only the citations rot. The reader loses the ability to jump to the source; the reader does not build a broken plugin.
CLAUDE.md asks for the doc update in the same change:
Keep docs in step with code. When you change code, update the docs that describe it in the same change [...] so a doc never outlives the API it documents. If the doc fix is out of scope, file a ticket rather than let it drift.
This PR already follows that rule for ARCHITECTURE.md. PLUGIN_AUTHORING.md was missed.
Two ways to close it:
- Edit three lines here.
PluginListAdapter.kt:61becomesPluginListItem.kt:69;PluginListAdapter.kt:69becomesFileImage.kt:36; "Glide" becomes "BitmapFactory". - File an ADFA ticket for the doc update and link it in the PR description.
Option 1 costs less. The edit touches Markdown only, so it does not pull any Kotlin file under the Spotless ratchet.
🤖 Generated with Claude Code
Review: ADFA-4928 — single manager for plugins and templatesRead the full diff and verified against the surrounding code on OverviewReplaces the View-based Plugin Manager with a Compose two-tab "Manager" screen (Plugins | Templates) and adds a Templates feature end-to-end:
Solid work overall: UDF layering respected, the parser is deliberately Android-free and unit-tested, the KDoc explains the non-obvious calls, and the docs were updated alongside. Verified all referenced strings/drawables exist, the catalog already carried the Compose aliases, Robolectric reaches High — worth fixing before merge1. Double system-bar insets. 2. Effect collection is no longer lifecycle-scoped. 3. val dest = File(templatesDir, item.file.name)
item.file.copyTo(dest, overwrite = true)
item.file.delete()
4. Rendezvous Medium
Low / polish
Test coverageGood: Gaps:
SecurityNothing alarming. ConventionsTabs/LF and ktlint formatting look correct throughout; strings correctly land in |
Address CodeRabbit review feedback on PR #1627: - Use SLF4J logging instead of android.util.Log - Narrow runCatching to expected I/O/parsing exceptions, rethrowing CancellationException instead of swallowing it - Refuse to install/uninstall over an existing same-name destination file instead of silently overwriting it - Treat a failed source-file delete as an install/uninstall failure and roll back the copied destination file
Address CodeRabbit review feedback on PR #1627: - Warm IDEApplication.cachedFilesDir on an IO thread before Koin starts, eliminating the race where pluginModule/templateModule could resolve it on the main thread first - Bound FileImage's bitmap decode with inSampleSize and move the file-existence check inside the IO dispatcher; narrow its catch to recoverable failures and let CancellationException propagate - Move the picked plugin file's name/extension validation (a ContentResolver IPC call for content:// URIs) off the picker callback and into PluginManagerViewModel on a background dispatcher, routed back through a new ShowInstallConfirmation effect - Replace android.util.Log with SLF4J logging in PluginManagerContent - Narrow the file-picker launch catch to ActivityNotFoundException and log it instead of silently swallowing any Exception
Address CodeRabbit review feedback on PR #1627: - Avoid double system-bar insets by zeroing ManagerScreen's Scaffold contentWindowInsets, since the activity's root already applies them - Fix the back button's TalkBack announcement (was "Cancel") with a dedicated cd_navigate_back string - Wire long-press tooltips to the discover-plugins action and install FAB - Always show Uninstall for a listed plugin, even when it failed to load, so a broken plugin has a recovery action - Move the detail-row "label: value" format into a string resource so translators control ordering/punctuation - Only treat a template card as clickable when it bundles more than one template, instead of always exposing tap/press semantics - Use an Android plurals resource for the template count string instead of a fixed "templates" string - Buffer TemplateManagerViewModel's uiEffect channel and use send() instead of trySend() so effects aren't dropped before a collector is ready
Address CodeRabbit review feedback on PR #1627: document the model contracts, including the meaning of installed/provenance and the one-archive-to-many-templates relationship. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Address CodeRabbit review feedback on PR #1627 (matches the JUnit Jupiter + Truth strategy ARCHITECTURE.md already documents for unit tests, which the app module hadn't wired up yet): - Run app unit tests on the JUnit Platform, with the vintage engine so existing JUnit 4/Robolectric tests keep running unchanged - Migrate CgtFileItemTest (no Robolectric dependency) to org.junit.jupiter.api.Test with Truth assertions - Keep CgtTemplateReaderTest on JUnit 4/RobolectricTestRunner (no built-in Jupiter integration) but switch its assertions to Truth Verified all 22 app unit test classes still run under :app:testV8DebugUnitTest with 0 failures.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/itsaky/androidide/viewmodels/TemplateManagerViewModel.kt (1)
51-53: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard template reloads against stale results.
loadTemplates()launches a new coroutine for every request frominit,onEvent, and post-mutation success paths. SincetemplateRepository.listTemplateFiles()runs onDispatchers.IOwithout synchronization or a request token, a faster initial load can complete after a later mutation-triggered reload and replaceuiState.itemswith stale data. Serialize reloads or ignore results from obsolete jobs, and cover out-of-order load completion in a coroutine test.🤖 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/viewmodels/TemplateManagerViewModel.kt` around lines 51 - 53, Update loadTemplates and its callers so overlapping reload requests cannot apply stale listTemplateFiles results: serialize loads or track and discard obsolete jobs, while preserving the loading state and post-mutation refresh behavior. Add a coroutine test that completes concurrent loads out of order and verifies uiState.items retains the newest result.
🧹 Nitpick comments (2)
app/src/main/java/com/itsaky/androidide/viewmodels/TemplateManagerViewModel.kt (2)
40-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public event API.
onEventis public but has no KDoc. Document its event contract, lifecycle-bound execution, state updates, and one-shot effects.As per coding guidelines, public functions must document contracts, threading, nullability, side effects, or units.
🤖 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/viewmodels/TemplateManagerViewModel.kt` around lines 40 - 48, Document the public TemplateManagerViewModel.onEvent function with KDoc covering its accepted TemplateManagerUiEvent contract, lifecycle-bound execution, resulting state updates, and one-shot effects; do not change the event handling behavior.Source: Coding guidelines
82-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the project logger instead of
Log.Replace the structured logging calls in
app/src/main/java/com/itsaky/androidide/viewmodels/TemplateManagerViewModel.ktwith existing SLF4JLoggerFactorylogger calls and keep exceptions as throwable arguments. Also applies to lines 102 and 129.🤖 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/viewmodels/TemplateManagerViewModel.kt` at line 82, Replace the Android Log calls in TemplateManagerViewModel, including the failures near lines 82, 102, and 129, with the existing project SLF4J LoggerFactory logger. Preserve each message and pass the caught exception as the throwable argument to the logger call, removing the direct Log dependency if no longer used.Source: Coding guidelines
🤖 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/app/IDEApplication.kt`:
- Around line 199-203: Move the cachedFilesDir warmup out of the pre-branch
startup path and execute it only after the user-unlocked initialization via
DeviceProtectedApplicationLoader.load(). Ensure onCreate() does not evaluate
cachedFilesDir during Direct Boot, while preserving the existing IO-thread
warmup once credential-protected storage is available.
- Around line 199-203: Remove the blocking runBlocking call around
cachedFilesDir from Application.onCreate(). Replace it with non-blocking
initialization, or defer/guard the Koin pluginModule/templateModule resolution
so cachedFilesDir is accessed only after the Activity framework can continue
startup.
In `@app/src/main/java/com/itsaky/androidide/ui/compose/common/FileImage.kt`:
- Around line 47-52: In the icon-loading catch blocks of FileImage, add
rate-limited SLF4J warning logs for handled SecurityException and
OutOfMemoryError cases before returning null. Use the established observability
mechanism, include the failure context and exception, and do not log the file
path; preserve CancellationException propagation and placeholder fallback
behavior.
- Around line 85-93: Update the sampling loop in decodeBounded() to base
inSampleSize on the larger of bounds.outWidth and bounds.outHeight, allowing
sampling whenever that maximum dimension remains at least twice maxDimensionPx.
Preserve the existing power-of-two increments and decode options flow.
In `@app/src/main/java/com/itsaky/androidide/ui/models/PluginManagerUiState.kt`:
- Around line 55-57: Update the PluginManagerUiEffect channel to use
Channel.BUFFERED so one-time effects survive periods when the LaunchedEffect
collector is unavailable, and handle failed trySend results by logging or
reporting the delivery failure. Preserve the existing ShowInstallConfirmation
effect flow and locate the changes around the channel declaration and its send
sites.
In `@common/src/main/java/com/itsaky/androidide/utils/UriExtensions.kt`:
- Around line 11-27: Update Uri.getFileName to catch only verified recoverable
ContentResolver provider failures in the second catch, while retaining the
existing SecurityException handling; do not convert unrelated exceptions into
"Unknown File". Replace the current UriExtensions logging with a class-scoped
SLF4J logger and use it for handled failures, preserving the fallback label only
for genuinely recoverable query failures.
---
Outside diff comments:
In
`@app/src/main/java/com/itsaky/androidide/viewmodels/TemplateManagerViewModel.kt`:
- Around line 51-53: Update loadTemplates and its callers so overlapping reload
requests cannot apply stale listTemplateFiles results: serialize loads or track
and discard obsolete jobs, while preserving the loading state and post-mutation
refresh behavior. Add a coroutine test that completes concurrent loads out of
order and verifies uiState.items retains the newest result.
---
Nitpick comments:
In
`@app/src/main/java/com/itsaky/androidide/viewmodels/TemplateManagerViewModel.kt`:
- Around line 40-48: Document the public TemplateManagerViewModel.onEvent
function with KDoc covering its accepted TemplateManagerUiEvent contract,
lifecycle-bound execution, resulting state updates, and one-shot effects; do not
change the event handling behavior.
- Line 82: Replace the Android Log calls in TemplateManagerViewModel, including
the failures near lines 82, 102, and 129, with the existing project SLF4J
LoggerFactory logger. Preserve each message and pass the caught exception as the
throwable argument to the logger call, removing the direct Log dependency if no
longer used.
🪄 Autofix
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 Plus
Run ID: 5bdff30c-2ed4-4788-9852-1b49aa06cafe
📒 Files selected for processing (18)
app/build.gradle.ktsapp/src/main/java/com/itsaky/androidide/app/IDEApplication.ktapp/src/main/java/com/itsaky/androidide/repositories/TemplateRepositoryImpl.ktapp/src/main/java/com/itsaky/androidide/templates/manager/models/CgtFileItem.ktapp/src/main/java/com/itsaky/androidide/ui/compose/ManagerScreen.ktapp/src/main/java/com/itsaky/androidide/ui/compose/common/FileImage.ktapp/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginListItem.ktapp/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginManagerContent.ktapp/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginManagerDialogs.ktapp/src/main/java/com/itsaky/androidide/ui/compose/templates/TemplateListItem.ktapp/src/main/java/com/itsaky/androidide/ui/models/PluginManagerUiState.ktapp/src/main/java/com/itsaky/androidide/viewmodels/PluginManagerViewModel.ktapp/src/main/java/com/itsaky/androidide/viewmodels/TemplateManagerViewModel.ktapp/src/test/java/com/itsaky/androidide/templates/manager/models/CgtFileItemTest.ktapp/src/test/java/com/itsaky/androidide/templates/manager/parsing/CgtTemplateReaderTest.ktcommon/src/main/java/com/itsaky/androidide/utils/UriExtensions.ktgradle/libs.versions.tomlresources/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (11)
- app/build.gradle.kts
- app/src/main/java/com/itsaky/androidide/ui/compose/ManagerScreen.kt
- gradle/libs.versions.toml
- app/src/main/java/com/itsaky/androidide/ui/compose/templates/TemplateListItem.kt
- app/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginManagerDialogs.kt
- app/src/test/java/com/itsaky/androidide/templates/manager/models/CgtFileItemTest.kt
- app/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginListItem.kt
- app/src/test/java/com/itsaky/androidide/templates/manager/parsing/CgtTemplateReaderTest.kt
- app/src/main/java/com/itsaky/androidide/repositories/TemplateRepositoryImpl.kt
- app/src/main/java/com/itsaky/androidide/templates/manager/models/CgtFileItem.kt
- app/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginManagerContent.kt
Address CodeRabbit follow-up review feedback on PR #1627: - Only run the cachedFilesDir warmup eagerly in onCreate() when credential-protected storage is already unlocked - the default Context.getFilesDir() throws during Direct Boot. When locked, warm it instead from CredentialProtectedApplicationLoader.load(), which only proceeds once that storage is confirmed accessible. - Base FileImage's inSampleSize loop on the larger image dimension instead of requiring both dimensions to exceed the target, so a wide-but-short (or tall-but-narrow) image still gets downsampled - Log FileImage's swallowed SecurityException/OutOfMemoryError icon-load failures via a throttled SLF4J warning, without logging the file path - Buffer PluginManagerViewModel's uiEffect channel and use send() instead of trySend(), same fix already applied to TemplateManagerViewModel, so effects (e.g. the new ShowInstallConfirmation) aren't dropped - Narrow UriExtensions.getFileName's second catch to SecurityException/ IllegalArgumentException instead of blanket Exception, so unexpected ContentResolver failures surface instead of being silently mislabeled as "Unknown File" (and then downstream as an unsupported plugin file); switch its logging to a class-scoped SLF4J logger
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ui/compose/common/FileImage.kt`:
- Around line 106-107: Update decodeBounded() to reject maxDimensionPx values
less than or equal to zero before entering the inSampleSize loop, using the
existing error-handling contract for invalid inputs. Preserve the current
sampling behavior for positive limits and ensure FileImage() cannot trigger the
loop with a zero limit.
🪄 Autofix
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 Plus
Run ID: e7b8a4ea-29e5-4ce1-a3b2-8d71d1e8ff53
📒 Files selected for processing (5)
app/src/main/java/com/itsaky/androidide/app/CredentialProtectedApplicationLoader.ktapp/src/main/java/com/itsaky/androidide/app/IDEApplication.ktapp/src/main/java/com/itsaky/androidide/ui/compose/common/FileImage.ktapp/src/main/java/com/itsaky/androidide/viewmodels/PluginManagerViewModel.ktcommon/src/main/java/com/itsaky/androidide/utils/UriExtensions.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- common/src/main/java/com/itsaky/androidide/utils/UriExtensions.kt
- app/src/main/java/com/itsaky/androidide/app/IDEApplication.kt
- app/src/main/java/com/itsaky/androidide/viewmodels/PluginManagerViewModel.kt
| while (maxOf(bounds.outWidth, bounds.outHeight) / (inSampleSize * 2) >= maxDimensionPx) { | ||
| inSampleSize *= 2 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate FileImage.kt =="
fd -a 'FileImage\.kt$' . | sed 's#^\./##'
echo "== outline =="
file="$(fd 'FileImage\.kt$' . | head -n1)"
if [ -n "${file:-}" ]; then
ast-grep outline "$file" || true
echo "== relevant lines 60-130 =="
sed -n '60,130p' "$file" | nl -ba -v60
echo "== callers of decodeBounded/FileImage =="
rg -n "decodeBounded|FileImage\(" .
fiRepository: appdevforall/CodeOnTheGo
Length of output: 526
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="/home/jailuser/git/app/src/main/java/com/itsaky/androidide/ui/compose/common/FileImage.kt"
echo "== file ==
wc -l "$file"
echo "== lines 1-150 =="
awk '{printf "%5d\t%s\n", NR, $0}' "$file" | sed -n '1,150p'
echo "== callers of decodeBounded/FileImage =="
rg -n "decodeBounded|FileImage\(" /home/jailuser/git/app/src/main/java || true
echo "== Kotlin integer behavior probe if available =="
if command -v kotlinc >/dev/null 2>&1; then
tmp="$(mktemp /tmp/kotlin_probe_XXXXXX.kts)"
cat > "$tmp" <<'KT'
var inSampleSize = 1
val maxDimensionPx = 0
var iterations = 0
try {
while (max(4096, 4096) / (inSampleSize * 2) >= maxDimensionPx) {
inSampleSize *= 2
iterations++
if (iterations > 20) break
}
println("no exception iterations=$iterations inSampleSize=$inSampleSize")
} catch (e: ArithmeticException) {
println("ArithmeticException iterations=$iterations inSampleSize=$inSampleSize")
}
KT
kotlinc -include-runtime -d "$tmp.jar" "$tmp" >/dev/null 2>&1
java -jar "$tmp.jar"
rm "$tmp" "$tmp.jar"
else
echo "kotlinc not available"
fiRepository: appdevforall/CodeOnTheGo
Length of output: 223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="/home/jailuser/git/app/src/main/java/com/itsaky/androidide/ui/compose/common/FileImage.kt"
echo "== lines 1-150 =="
awk '{printf "%5d\t%s\n", NR, $0}' "$file" | sed -n '1,150p'
echo "== callers of decodeBounded/FileImage =="
rg -n "decodeBounded|FileImage\(" /home/jailuser/git/app/src/main/java || true
echo "== Kotlin integer behavior probe if available =="
if command -v kotlinc >/dev/null 2>&1; then
tmp="$(mktemp /tmp/kotlin_probe_XXXXXX.kts)"
cat > "$tmp" <<'KT'
var inSampleSize = 1
val maxDimensionPx = 0
var iterations = 0
try {
while (max(4096, 4096) / (inSampleSize * 2) >= maxDimensionPx) {
inSampleSize *= 2
iterations++
if (iterations > 20) break
}
println("no exception iterations=$iterations inSampleSize=$inSampleSize")
} catch (e: ArithmeticException) {
println("ArithmeticException iterations=$iterations inSampleSize=$inSampleSize")
}
KT
kotlinc -include-runtime -d "$tmp.jar" "$tmp" >/dev/null 2>&1
java -jar "$tmp.jar"
rm "$tmp" "$tmp.jar"
else
echo "kotlinc not available"
fiRepository: appdevforall/CodeOnTheGo
Length of output: 5277
🌐 Web query:
Kotlin Int overflow arithmetic exception wrap-around behavior division by zero
💡 Result:
In Kotlin, the behavior of integer arithmetic regarding overflow and division by zero is platform-dependent and generally follows the specifications of the underlying target environment [1][2]. Integer Overflow By default, Kotlin integer arithmetic (for types such as Int, Long, Short, and Byte) does not throw an exception on overflow [3][4]. Instead, operations wrap around, using modular arithmetic [5][3]. For example, adding 1 to Int.MAX_VALUE results in Int.MIN_VALUE [6][4]. The language specification notes that while overflow results are technically unspecified, they typically manifest as silent wraparound on most common platforms [1][2]. To perform checked arithmetic that throws an ArithmeticException on overflow, you can use interoperability with Java's standard library methods, such as Math.addExact, Math.subtractExact, or Math.multiplyExact [3][7]. Division by Zero The behavior of integer division by zero is not consistent across all Kotlin platforms [8][9]: 1. JVM and Kotlin/Native: Integer division by zero throws an ArithmeticException [8][9]. This aligns with standard JVM and native behavior [9]. 2. Kotlin/JS: Integer division by zero does not throw an exception [8][9]. Because Kotlin/JS maps 32-bit integers to standard JavaScript numbers (IEEE 754 floats) and emulates integer division using bitwise operations, division by zero effectively evaluates to 0 [8][9]. Due to these platform differences, it is recommended to explicitly check for a zero divisor if your code must run consistently across multiple platforms (e.g., in Kotlin Multiplatform projects) [9]. Floating-point division by zero (using Float or Double) does not throw an exception; it results in Infinity, -Infinity, or NaN, adhering to the IEEE 754 standard [8].
Citations:
- 1: https://kotlinlang.org/spec/built-in-types-and-their-semantics.html
- 2: https://kotlinlang.org/spec/pdf/sections/built-in-types-and-their-semantics.pdf
- 3: https://www.slingacademy.com/article/handling-overflow-and-underflow-in-kotlin-numbers/
- 4: https://github.com/jetbrains/kotlin-web-site/blob/master/docs/topics/numbers.md
- 5: https://youtrack.jetbrains.com/projects/KT/issues/KT-84024/Non-overflowing-integer-arithmetic-operations
- 6: https://kotlinlang.org/docs/numbers.html
- 7: https://developer.android.google.cn/reference/kotlin/java/lang/StrictMath
- 8: https://docs.syntblaze.com/kotlin/arithmetic-operators/division
- 9: https://dev.to/kotools/the-kotlin-multiplatform-division-by-zero-trap-4jpn
Guard against non-positive sampling limits.
FileImage() can produce maxDimensionPx == 0 when the default 40.dp resolves below one pixel on this density. If decodeBounded() then receives 0, the loop is not bounded by inSampleSize until overflow reaches a zero divisor and decodeFile() throws an unhandled exception. Reject maxDimensionPx <= 0 inside decodeBounded() before the loop so the API is safe from density edge cases.
🤖 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/ui/compose/common/FileImage.kt`
around lines 106 - 107, Update decodeBounded() to reject maxDimensionPx values
less than or equal to zero before entering the inSampleSize loop, using the
existing error-handling contract for invalid inputs. Preserve the current
sampling behavior for positive limits and ensure FileImage() cannot trigger the
loop with a zero limit.
- Disable the install FAB while a plugin install is in flight, so a second tap can't start a concurrent installPlugin() coroutine. The Compose ManagerScreen replaced the old Activity, which disabled the FAB via binding.fabInstallPlugin.isEnabled = !state.isInstalling; nothing carried that behavior over. - Fix PLUGIN_AUTHORING.md pointers left dangling by the PluginListAdapter.kt -> PluginListItem.kt/FileImage.kt migration. The delete-failure-handling and cachedFilesDir warmup comments from the same review were already addressed by prior commits on this branch; verified against current HEAD, no further changes needed.
hal-eisen-adfa
left a comment
hal-eisen-adfa
left a comment
There was a problem hiding this comment.
Blocking review: 5 findings from a fresh pass
These are new - none overlap the CodeRabbit threads or the two earlier review comments. Details inline; #2 has two sites (TemplateRepositoryImpl, UriExtensions).
- The long-press tooltips added to the FAB and the toolbar action almost certainly never fire.
- Two "narrow the catch" fixes turned swallowed failures into crash paths, because both callers are bare
viewModelScope.launchwith noCoroutineExceptionHandler. - The
label_valuestring-resource fix landed in the plugin dialog but not the template one. PluginManagerActivity'stry/catchno longer covers anything, sincesetContent's lambda runs afteronCreatereturns.pluginVersionLabelduplicates the testedversionLabeland already disagrees with it on blank input.
(GitHub does not allow REQUEST_CHANGES on your own PR, so this is submitted as a comment review; treat each inline as blocking.)
| modifier = | ||
| Modifier | ||
| .alpha(if (pluginUiState.isInstalling) DISABLED_ALPHA else 1f) | ||
| .pointerInput(Unit) { | ||
| detectTapGestures(onLongPress = { showTooltip() }) | ||
| }, |
There was a problem hiding this comment.
Blocking - needs a device check before merge. The long-press tooltip wired here for the FAB (118-123) and for the Discover-plugins IconButton (96-99) most likely never fires.
FloatingActionButton/IconButton apply the caller's modifier at the head of the chain and their own internal clickable at the tail of the same layout node. On PointerEventPass.Main the tail runs first, and clickable's detectTapAndPress consumes the down; detectTapGestures here then calls awaitFirstDown(requireUnconsumed = true) and never sees it. The pointerInput anchors on plain Box/Card (PluginManagerContent, TemplateManagerScreen, the single-template branch of TemplateListItem) are unaffected - no inner clickable there.
If it holds, the two long-press handlers the old activity had on fabInstallPlugin and action_discover_plugins are silently gone, and the fix for the earlier "add long-press tooltips" comment is cosmetic. PluginListItem.kt:63 already uses the pattern that works (combinedClickable(onClick = ..., onLongClick = ...)); for the FAB that means hosting the gesture on a wrapper that owns the click, or driving the tooltip off an interactionSource.
| } catch (e: CancellationException) { | ||
| throw e | ||
| } catch (e: IOException) { | ||
| logger.error("Failed to scan template files", e) | ||
| Result.failure(e) | ||
| } catch (e: SecurityException) { | ||
| logger.error("Failed to scan template files", e) | ||
| Result.failure(e) | ||
| } |
There was a problem hiding this comment.
Blocking. Narrowing this to IOException/SecurityException turned a swallowed failure into a crash path.
ZipInputStream.nextEntry (CgtTemplateReader.kt:26) throws IllegalArgumentException("MALFORMED") when an entry name is not valid UTF-8, and zip.readBytes() (CgtTemplateReader.kt:28) is unbounded, so a truncated or hostile archive can throw OutOfMemoryError. Neither is caught here, nor by parseCgtFile's IOException/JSONException, so it propagates out of withContext into TemplateManagerViewModel.loadTemplates's bare viewModelScope.launch - which has no CoroutineExceptionHandler - and reaches the uncaught handler.
downloadDir is the public Downloads folder, so this input is untrusted: a single malformed .cgt sitting there crashes the app the moment the Templates tab opens. Catch IllegalArgumentException too, preferably per-file in parseCgtFile so one bad archive is skipped instead of failing the whole scan, and cap the template.json read.
Same regression shape in UriExtensions.kt - see the other comment.
| } catch (e: SecurityException) { | ||
| log.warn("Denied access while reading URI: {}://{}", scheme, authority, e) | ||
| } catch (e: IllegalArgumentException) { | ||
| // No registered provider for this URI, or the provider rejected the query args. | ||
| log.warn("No provider could resolve URI: {}://{}", scheme, authority, e) | ||
| } |
There was a problem hiding this comment.
Blocking. The previous catch (e: Exception) became SecurityException + IllegalArgumentException, but the new caller has no guard of its own.
PluginManagerViewModel.handleFileSelected calls this inside withContext(Dispatchers.IO) in a bare viewModelScope.launch, so anything else a provider throws from query()/getString() - UnsupportedOperationException for an unsupported projection, CursorWindowAllocationException, a RuntimeException wrapping DeadObjectException - now crashes the app where the user previously just saw "unsupported file". The other caller, FileImporter.kt:40, sits inside runCatching and is unaffected, which is what makes this easy to miss.
Either keep a broad catch here (the added logging is the part that was actually asked for), or wrap the call in handleFileSelected.
| label: String, | ||
| value: String, | ||
| ) { | ||
| Text("$label: $value") |
There was a problem hiding this comment.
Blocking, small. The Text("$label: $value") comment was fixed in PluginManagerDialogs.kt:130 (stringResource(R.string.label_value, label, value)) but not here, so the two dialogs now disagree: the plugin one is localizable and RTL-correct, the template one is not. Use the same R.string.label_value.
Line 109 has the same problem plus a style violation - Text("• $tag") hardcodes an untranslatable separator and uses a non-ASCII literal in code, which CLAUDE.md's ASCII rule disallows. TemplateListItem.kt:111 (" - " + stringResource(...)) is the third instance.
| try { | ||
| super.onCreate(savedInstanceState) | ||
|
|
||
| setSupportActionBar(binding.toolbar) | ||
| supportActionBar?.apply { | ||
| title = getString(R.string.title_plugin_manager) | ||
| setDisplayHomeAsUpEnabled(true) | ||
| } | ||
|
|
||
| binding.toolbar.setNavigationOnClickListener { | ||
| onBackPressedDispatcher.onBackPressed() | ||
| binding.composeView.setContent { | ||
| ManagerTheme { | ||
| ManagerScreen( | ||
| activity = this, | ||
| pluginViewModel = pluginViewModel, | ||
| templateViewModel = templateViewModel, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| setupRecyclerView() | ||
| setupFab() | ||
| setupTooltipLongPress() | ||
| setupFeedbackButton() | ||
| observeViewModel() | ||
| } catch (e: Exception) { | ||
| // Log the error and finish the activity if something goes wrong | ||
| e.printStackTrace() |
There was a problem hiding this comment.
Blocking. This try/catch no longer covers what it exists to cover. setContent only registers the composable; the lambda body runs at first layout, after onCreate has returned, so nothing thrown from it reaches this catch and the graceful flashError(...) + finish() is dead code for that path.
It matters because both ViewModels are first resolved inside that lambda. Environment.init(...) is wrapped in runCatching in DeviceProtectedApplicationLoader, so after a partial init Environment.TEMPLATES_DIR can still be null, and TemplateModule.kt:19 passes it into TemplateRepositoryImpl's non-null File parameter: NPE during composition and a hard crash, where this block used to show the error and finish.
Resolve the ViewModels (or validate Environment) inside the try, before setContent, or handle the failure inside the composition.
| /** Matches the plugin list's version-chip truncation: `vX.Y.Z...` past three dot-segments. */ | ||
| internal fun pluginVersionLabel(version: String): String { | ||
| val segments = version.split('.') | ||
| return if (segments.size > 3) "v${segments.take(3).joinToString(".")}..." else "v$version" | ||
| } |
There was a problem hiding this comment.
Blocking, small. This duplicates versionLabel (CgtFileItem.kt:66), and the two already disagree: versionLabel("") returns "", pluginVersionLabel("") returns a bare "v", so a plugin whose manifest omits version renders a stray "v" chip. Only versionLabel has tests (CgtFileItemTest), so the untested copy is the one that will rot. Delete this and call the shared helper.
3 participants
We currently don't have any UI for adding a template to CoGo. It's a very similar idea to adding a plugin, so let's try putting them together.