ADFA-4928 create a single manager for plugins and templates - #1627
Open
hal-eisen-adfa wants to merge 19 commits into
Open
ADFA-4928 create a single manager for plugins and templates#1627hal-eisen-adfa wants to merge 19 commits into
hal-eisen-adfa wants to merge 19 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.
Contributor
|
Warning Review limit reached
Next review available in: 6 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (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, file 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 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 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
Collaborator
Author
Code reviewFound 2 issues:
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
hal-eisen-adfa
commented
Aug 4, 2026
Collaborator
Author
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
Collaborator
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.
Contributor
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
Contributor
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
- 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
commented
Aug 7, 2026
hal-eisen-adfa
left a comment
hal-eisen-adfa
left a comment
Collaborator
Author
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.)
…ct collection - ManagerScreen's TopAppBar still used its default status-bar insets on top of PluginManagerActivity.onApplySystemBarInsets, which already pads the root view by the full system-bar insets (that padding doesn't consume the insets, so Compose saw them a second time). Zero out TopAppBar's windowInsets to match the Scaffold's contentWindowInsets, which was already zeroed. - PluginManagerContent and TemplateManagerScreen collected viewModel.uiEffect in a bare LaunchedEffect, so it kept collecting while the activity was stopped. A plugin install finishing while the app is backgrounded could then run DialogUtils.showRestartPrompt or a flashbar builder against a stopped activity. Wrap both collectors in repeatOnLifecycle(STARTED), matching the old repeatOnLifecycle(STARTED) pattern used elsewhere in the app.
- Long-press tooltips on the FAB and Discover-plugins IconButton never fired: pointerInput(detectTapGestures) placed on the caller-side modifier loses the down event to the button's own internal clickable, which runs first on the Main pointer pass. Drive the tooltip off the button's own MutableInteractionSource instead (press duration vs LocalViewConfiguration's longPressTimeoutMillis), which observes the same press stream the button already dispatches rather than racing it for the raw pointer event. - CgtTemplateReader.readTemplates read a zip entry's bytes unbounded, so a corrupt/hostile .cgt sitting in the public Downloads folder could OOM the app; bound the read and throw IOException past 1 MiB. parseCgtFile also didn't catch IllegalArgumentException, which ZipInputStream.nextEntry throws for a non-UTF-8 entry name - that propagated out of the bare viewModelScope.launch in TemplateManagerViewModel.loadTemplates (no CoroutineExceptionHandler) and crashed the app. Both are now handled per-file, so one bad archive is skipped instead of failing the whole scan. - UriExtensions.getFileName's catch was narrowed to SecurityException/IllegalArgumentException, but a misbehaving content provider can throw other RuntimeExceptions from query()/getString() (CursorWindowAllocationException, a wrapped DeadObjectException, ...). Broadened back to Exception, since this is a best-effort display-name lookup, not a path that should ever crash the caller. - TemplateManagerDialogs' DetailRow still built "$label: $value" with string concatenation instead of the R.string.label_value fix that landed in the plugin dialog, and the optional-tags list hardcoded a non-ASCII "*" bullet in code (CLAUDE.md's ASCII rule). Added R.string.template_optional_tag and reused R.string.label_value. TemplateListItem's status/provenance row had the same hardcoded-separator shape; extracted it to R.string.label_separator. - PluginManagerActivity's try/catch around setContent no longer caught anything: setContent only registers the composable, and its lambda (where both ViewModels first resolve via Koin) runs at first layout, after onCreate has already returned past the catch. Force-resolve both `by viewModel()` delegates inside the try, before setContent. - PluginListItem.pluginVersionLabel duplicated CgtFileItem.versionLabel and disagreed with it on blank input (a stray "v" chip vs the tested ""). Deleted the duplicate and reused the tested helper. Added a CgtTemplateReaderTest regression case covering the bounded-read cap. Verified via :app:testV8DebugUnitTest (all passing) and spotlessCheck.
Contributor
|
@hal-eisen-adfa Addressed all 6 findings from your review at 9d124ac:
Ran @coderabbitai review |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- follow-up commit ---'
git show --stat --oneline 9d124ac96
git diff 9d124ac96^ 9d124ac96 -- \
app/src/main/java/com/itsaky/androidide/ui/compose/ManagerScreen.kt \
app/src/main/java/com/itsaky/androidide/templates/manager/parsing/CgtTemplateReader.kt \
app/src/main/java/com/itsaky/androidide/repositories/TemplateRepositoryImpl.kt \
common/src/main/java/com/itsaky/androidide/utils/UriExtensions.kt \
app/src/main/java/com/itsaky/androidide/ui/compose/templates/TemplateManagerDialogs.kt \
app/src/main/java/com/itsaky/androidide/activities/PluginManagerActivity.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 \
resources/src/main/res/values/strings.xml
echo '--- structural maps ---'
for f in \
app/src/main/java/com/itsaky/androidide/ui/compose/ManagerScreen.kt \
app/src/main/java/com/itsaky/androidide/templates/manager/parsing/CgtTemplateReader.kt \
app/src/main/java/com/itsaky/androidide/repositories/TemplateRepositoryImpl.kt \
common/src/main/java/com/itsaky/androidide/utils/UriExtensions.kt \
app/src/main/java/com/itsaky/androidide/ui/compose/templates/TemplateManagerDialogs.kt \
app/src/main/java/com/itsaky/androidide/activities/PluginManagerActivity.kt \
app/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginListItem.kt \
app/src/main/java/com/itsaky/androidide/templates/manager/models/CgtFileItem.kt
do
echo "--- $f ---"
ast-grep outline "$f" --items all || true
done
echo '--- relevant source ranges ---'
for f in \
app/src/main/java/com/itsaky/androidide/ui/compose/ManagerScreen.kt \
app/src/main/java/com/itsaky/androidide/templates/manager/parsing/CgtTemplateReader.kt \
app/src/main/java/com/itsaky/androidide/repositories/TemplateRepositoryImpl.kt \
common/src/main/java/com/itsaky/androidide/utils/UriExtensions.kt \
app/src/main/java/com/itsaky/androidide/ui/compose/templates/TemplateManagerDialogs.kt \
app/src/main/java/com/itsaky/androidide/activities/PluginManagerActivity.kt \
app/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginListItem.kt \
app/src/main/java/com/itsaky/androidide/templates/manager/models/CgtFileItem.kt
do
echo "--- $f ---"
cat -n "$f"
done
echo '--- references and resource declarations ---'
rg -n -C 3 --glob '*.kt' \
'TooltipTag\.(PLUGIN_MANAGER|TEMPLATE_MANAGER)|longPressTimeoutMillis|MutableInteractionSource|versionLabel\(|pluginVersionLabel|CgtTemplateReader\.readTemplates|parseOptionalTags|getFileName\(' \
app common idetooltips || true
rg -n -C 2 'label_value|template_optional|template_status|template_provenance' \
resources/src/main/res/values/strings.xml || trueLength of output: 96508
The bounded archive read, malformed ZIP handling, filename lookup fallback, string-resource changes, ViewModel resolution boundary, and shared version formatter look correct from static inspection. You are interacting with an AI system. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/activities/PluginManagerActivity.kt (1)
39-45: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRestrict the
onCreaterecoverable failure boundary.The broad
catch (e: Exception)aroundsuper.onCreate(),setContent, ViewModel initialization, andsetupFeedbackButton()treats unrelated lifecycle or post-configuration failures as recoverable plugin-manager init errors. Catch only documented initialization failures and log handled errors withLoggerFactoryinstead ofprintStackTrace(); report handled failures through the project observability path as applicable.🤖 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/activities/PluginManagerActivity.kt` around lines 39 - 45, Narrow the try/catch in PluginManagerActivity.onCreate to cover only documented plugin-manager initialization failures, rather than super.onCreate(), setContent, and setupFeedbackButton() or unrelated lifecycle errors. Replace printStackTrace() with LoggerFactory logging and route handled failures through the project’s observability mechanism where applicable, while preserving recovery for the expected ViewModel/configuration initialization failure.Sources: Coding guidelines, Learnings
🤖 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.
Nitpick comments:
In `@app/src/main/java/com/itsaky/androidide/activities/PluginManagerActivity.kt`:
- Around line 39-45: Narrow the try/catch in PluginManagerActivity.onCreate to
cover only documented plugin-manager initialization failures, rather than
super.onCreate(), setContent, and setupFeedbackButton() or unrelated lifecycle
errors. Replace printStackTrace() with LoggerFactory logging and route handled
failures through the project’s observability mechanism where applicable, while
preserving recovery for the expected ViewModel/configuration initialization
failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a5d2a847-c635-4ac9-b992-da7da7a6680c
📒 Files selected for processing (12)
app/src/main/java/com/itsaky/androidide/activities/PluginManagerActivity.ktapp/src/main/java/com/itsaky/androidide/repositories/TemplateRepositoryImpl.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/plugins/PluginListItem.ktapp/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginManagerContent.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/test/java/com/itsaky/androidide/templates/manager/parsing/CgtTemplateReaderTest.ktcommon/src/main/java/com/itsaky/androidide/utils/UriExtensions.ktresources/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (10)
- app/src/main/java/com/itsaky/androidide/ui/compose/ManagerScreen.kt
- app/src/test/java/com/itsaky/androidide/templates/manager/parsing/CgtTemplateReaderTest.kt
- resources/src/main/res/values/strings.xml
- app/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginManagerContent.kt
- app/src/main/java/com/itsaky/androidide/ui/compose/templates/TemplateManagerDialogs.kt
- app/src/main/java/com/itsaky/androidide/repositories/TemplateRepositoryImpl.kt
- app/src/main/java/com/itsaky/androidide/ui/compose/templates/TemplateListItem.kt
- app/src/main/java/com/itsaky/androidide/ui/compose/templates/TemplateManagerScreen.kt
- app/src/main/java/com/itsaky/androidide/ui/compose/plugins/PluginListItem.kt
- app/src/main/java/com/itsaky/androidide/templates/manager/parsing/CgtTemplateReader.kt
… bound CodeRabbit flagged (2026-08-05 review, still unresolved) that decodeBounded()'s inSampleSize loop assumes maxDimensionPx > 0. If it's ever <= 0 - e.g. the 40.dp default rounding to a sub-pixel size at an unusual density - the loop condition (a non-negative quotient >= a non-positive bound) is permanently true, hanging on an unbounded doubling of inSampleSize instead of throwing. Skip the downsampling loop entirely in that case and decode at inSampleSize = 1.
hal-eisen-adfa
commented
Aug 11, 2026
hal-eisen-adfa
left a comment
hal-eisen-adfa
left a comment
Collaborator
Author
There was a problem hiding this comment.
Third pass: 2 residuals
The six findings from the last review all check out - the IllegalArgumentException catch, the 1 MiB bounded read, the broadened UriExtensions catch, the label_value/separator string resources, the pre-setContent ViewModel resolution, and the pluginVersionLabel deletion are all correct. My two earlier issue comments (the unread isInstalling, the PLUGIN_AUTHORING.md drift) are closed too.
Two things are still open, both inline:
- The
cachedFilesDirwarm-up is gated but still not wrapped inrunCatching, so a non-lock-statefilesDirfailure still exits the process. - The long-press tooltip now fires, but it doesn't suppress the button's click - a long press on the FAB shows the tooltip and opens the file picker.
(Submitted as a comment review because GitHub does not allow REQUEST_CHANGES on your own PR; treat #2 as blocking.)
| // instead runs from CredentialProtectedApplicationLoader.load(), which only proceeds once | ||
| // that storage is confirmed accessible. | ||
| if (isUserUnlocked) { | ||
| runBlocking(Dispatchers.IO) { cachedFilesDir } |
Collaborator
Author
There was a problem hiding this comment.
Still needs runCatching.
Gating on isUserUnlocked closes the Direct Boot path from my Aug-4 comment, and that was the larger half of it. What the gate does not cover is a filesDir failure that is not lock-state related - which is the failure this codebase has actually shipped a crash for. ADFA-2358 (dbb8cc05b) was IllegalArgumentException: Invalid path: /data/data/com.itsaky.androidide/files; the user is unlocked, the gate passes, and the call still throws.
Both warm-up sites are unprotected:
- Here.
runBlocking(Dispatchers.IO) { cachedFilesDir }rethrows intoonCreate, beforeensureKoinStarted(). Nothing upstack catches it, so it lands inhandleUncaughtException->exitProcess(EXIT_CODE_CRASH)(DeviceProtectedApplicationLoader.kt:166). CredentialProtectedApplicationLoader.kt:84.withContext(Dispatchers.IO) { IDEApplication.cachedFilesDir }runs insidecoroutineScope.launch(Dispatchers.Default)(IDEApplication.kt:94), a bareMainScope()with noCoroutineExceptionHandler- same destination. It also sits before the_isLoaded.compareAndSeton line 86, so a throw there leaves the flag false and the whole remainder of credential-protected init silently never runs.
What makes this worth fixing rather than arguing about: cachedFilesDir is a by lazy warm-up whose entire purpose is to keep a later main-thread read off the disk. Failing it should degrade to "the first real read pays the syscall" - never to a process exit. That is exactly why every other storage-touching call in DeviceProtectedApplicationLoader.load() is wrapped in runCatching with the comment "this may fail when running in direct boot mode, so we wrap this in runCatching and ignore errors" (lines 57, 61, 76). This block is the odd one out.
if (isUserUnlocked) {
runCatching { runBlocking(Dispatchers.IO) { cachedFilesDir } }
.onFailure { logger.warn("Failed to warm cachedFilesDir; first read will hit disk", it) }
}Swallowing it is safe in both directions: by lazy caches the value, not the failure, so a throw here does not poison later reads and the retry from CredentialProtectedApplicationLoader still works.
| } | ||
| }, | ||
| modifier = Modifier.alpha(if (pluginUiState.isInstalling) DISABLED_ALPHA else 1f), | ||
| interactionSource = rememberLongPressInteractionSource { showTooltip() }, |
Collaborator
Author
There was a problem hiding this comment.
Blocking. The tooltip fires now, but so does the action.
The interactionSource switch is the right fix for detection - it observes the same press stream the button already dispatches instead of competing for the raw pointer event, so the race from my last review is gone. It does not suppress the click, though, and Modifier.clickable has no long-press concept at all. Traced through the versions this PR builds against (material3 1.2.0, foundation 1.6.1):
IconButton.kt:85-94-modifier.<...>.clickable(onClick = onClick, interactionSource = interactionSource, ...)FloatingActionButton.kt:104-112->Surface(onClick, interactionSource)->Surface.kt:235-240-.clickable(interactionSource, indication, enabled, onClick)Clickable.kt:981routes plainclickabletodetectTapAndPress, which isawaitFirstDown()thenwaitForUpOrCancellation()thenonTap?.invoke(...)(TapGestureDetector.kt:237-255). No duration threshold anywhere in that path.
So a long press on the FAB shows the tooltip at longPressTimeoutMillis and then fires OpenFilePicker when the finger lifts; on the Discover action it shows the tooltip and then launches the browser. The pre-Compose activity did neither - setOnLongClickListener returning true consumed the event.
combinedClickable is what suppresses it: with a non-null onLongClick it routes to detectTapGestures (Clickable.kt:1019), where the long-press branch invokes onLongPress and then consumeUntilUp() with upOrCancel left null, so onTap is never reached (TapGestureDetector.kt:128-138). That is why PluginListItem.kt:58 and TemplateListItem.kt:66 are correct as written.
It cannot simply be moved onto the caller modifier here, though - FloatingActionButton/IconButton keep their own internal clickable at the tail of the chain, so you would have two competing detectors and would reintroduce the bug I filed last time. Two options that do work:
- Drop the Material wrapper where you need both gestures. Replace
IconButtonwith aBox(Modifier.size(48.dp).clip(CircleShape).combinedClickable(onClick = ..., onLongClick = ...))around theIcon, so exactly one detector owns the node. Cleanest semantically, and it keeps a single source of truth for the gesture; costs you the built-in ripple/sizing defaults, which you would restore viaindication/size. - Keep
rememberLongPressInteractionSourceand consume the next click. Have the helper set a flag when it fires and haveonClickread-and-clear it:
val suppressNextClick = remember { mutableStateOf(false) }
// in rememberLongPressInteractionSource's LaunchedEffect, after delay(longPressTimeoutMillis):
suppressNextClick.value = true
currentOnLongPress()
// at the call site:
onClick = {
if (suppressNextClick.value) {
suppressNextClick.value = false
} else {
pluginViewModel.onEvent(PluginManagerUiEvent.OpenFilePicker)
}
}The ordering is safe - the flag is set at the long-press timeout, strictly before the up event that drives onTap.
Option 1 for the IconButton and option 2 for the FAB is probably the least-churn split, since there is no drop-in replacement for a Material FAB's shape/elevation.
Either way this still wants the device check I asked for last time: the unit-test run does not exercise any of this. Worth confirming on hardware that a long press shows the tooltip and leaves the file picker closed, and that a normal tap still opens it.
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
4 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.
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.