Repository navigation
Catalog UI 4/5: one install action through the main-process API, with offline and error states (#18) - #35
Conversation
…error states (#18) - InstallError marks connectivity failures (networkRequired) so the UI can say 'a network connection is required' and keep the selection for retry, instead of presenting a broken install; BatchItemResult and the installFromCatalog answer propagate the flag. - installFromCatalog refuses entries this build cannot run, with the same compatibilityFor() reason the cards show, before anything downloads. - listCatalog re-attaches the lifecycle view (installed/pinned/disabled/ updateStatus/updateAvailable/downgradeVersions) that Catalog UI 1/5 dropped, reviving the install list's Update/Pin/Disable controls. - The install plan note shows the final resolved set (names, install order, transfer size, pruned conflicts, network requirement) before confirmation; required dependencies are adopted and locked; pruned entries untick. - Failed installs keep the failed selection for a one-click retry and list every failure; a successful install clears the set.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Minor suggestions inline — nothing blocking.
Reviewed changes — Catalog UI 4/5: one install action through the main-process API, with offline and error states, plus a listCatalog() regression fix.
- Install gate in main —
installFromCatalognow resolves throughplanCatalogInstall(...).outstanding, refuses incompatible entries with the samecompatibilityFor()verdict the cards show, and reports a top-levelnetworkRequiredwhen every failure is a connect-only one. - Installer classification —
InstallError/BatchItemResultcarrynetworkRequiredfor refused sockets and interrupted streams, but not for HTTP/integrity rejections. - Screen behavior — the plan note prices the resolved set and names what each required entry pulled in and what the conflict pass pruned; dependencies are auto-ticked and locked; failed runs keep the selection and list every failure; the screen still never writes plugin files itself.
listCatalog()regression fix — the lifecycle view (installed,pinned,disabled,updateStatus,downgradeVersions, …) is re-attached so the per-plugin controls in the install list work again.- Tests + docs — new offline/mixed/success/incompatible/pricing/bridge-only cases in both suites;
docs/PLUGIN_CATALOG.mddocuments the new contract.
Verified locally: tsc --noEmit clean, and both touched suites plus the selection/compat/lifecycle/precedence suites pass (89 + 98 tests).
big-pickle (free) ｜ 𝕏
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous Review Summary (commit 23eeb14)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 23eeb14)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (7 files)
Reviewed by free · Input: 591.3K · Output: 60.9K · Cached: 220.7K |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| success: boolean; | ||
| message: string; | ||
| /** The failure was a connectivity problem, so as a whole it is retryable. */ | ||
| networkRequired?: boolean; |
There was a problem hiding this comment.
SUGGESTION: Consider adding JSDoc for networkRequired clarity
The networkRequired field on BatchItemResult indicates a connectivity problem that is retryable. The comment says "so as a whole it is retryable" but the field is per-item. Consider clarifying: "If true, this failure was a connectivity problem and the item is retryable."
| networkRequired?: boolean; | |
| /** If true, this failure was a connectivity problem and the item is retryable. */ |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| export interface CatalogInstallResult { | ||
| success: boolean; | ||
| message: string; | ||
| results: unknown[]; |
There was a problem hiding this comment.
SUGGESTION: TypeScript type precision - results should be BatchItemResult[] instead of unknown[]
The CatalogInstallResult.results field is typed as unknown[] but the function returns BatchItemResult[]. Using the concrete type improves type safety and enables IDE autocomplete for callers.
| results: unknown[]; | |
| results: BatchItemResult[]; |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
| catalogDependencyInfo.classList.add('hidden'); | ||
| catalogDependencyInfo.textContent = ''; | ||
| } catch (e) { |
There was a problem hiding this comment.
WARNING: Resolver errors silently hide the dependency info without user feedback
When plugins.resolveCatalog(ids) fails, the catch block hides catalogDependencyInfo but shows no error message. The user sees the plan note disappear with no indication of why. At minimum, log the error to console for debugging; ideally, show a transient message like "Could not resolve selection — try again."
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
…on, resolver error surfaced - mirrorPlanOntoRows: a resolver-pruned entry is unticked before any lock is applied, and pruned dependents no longer adopt or lock their own dependencies behind them; the 'Required by the selection' line never names a pruned entry. - Adoption iterates to a fixed point, so a transitive chain (A requires B requires C) lands on the rows in the same resolve instead of one per round trip. - A resolveCatalog failure now says 'Could not resolve the selection. Try again before installing.' instead of silently hiding the note. - CatalogInstallResult.results typed as BatchItemResult[]; per-item networkRequired JSDoc clarified.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — Incremental review since last Pullfrog review (23eeb14..f55d5ac).
- Resolver conflicts take precedence over locks.
mirrorPlanOntoRowsnow computesprunedIdsfrom the plan's conflicts and removes pruned entries from the selection before lock computation;lockedBychecks only apply to non-pruned entries so a dropped entry is never adopted or locked behind its dependent. - Transitive dependencies are adopted in one pass. A fixed-point loop re-runs
planLocksas newly adopted dependencies bring in their own deps, so the full chain appears checked and locked immediately and the plan note accurately describes who requires what. - Plan note construction is cleaner. The note now groups dropped entries under "Will not install" using named entries, filters pruned IDs out of the locked list, and surfaces a concrete resolver failure message if the plan cannot be read.
The tests added in the incremental commit verify the pruned-beats-locked behavior, transitive adoption, and the note text; no behavior changes outside the renderer's plan mirroring.
big-pickle (free) ｜ 𝕏
Part of #4. Closes #18 — Catalog UI 4/5. Accessibility pass follows in Catalog UI 5/5 (#19).
Changes
plugins.installCatalogand nothing else; main resolves (planCatalogInstall) and runs the batch. No new write path exists in the renderer — see the verification note below.Install set (N): …), which entry pulled each dependency in (Required by the selection: Core (required by Practice Alpha).), what the conflict pass pruned (Will not install: …), the pinned transfer size (2 downloads (~466 KB)), and that installing needs a network connection.plugins:resolveCatalognow also returnsdownloadBytes(pinned sizes of the outstanding set).InstallErrorfailures that a reconnect fixes (socket refused on fetch, stream interrupted mid-download) are typednetworkRequiredand propagated throughBatchItemResultand theinstallFromCataloganswer. When every failure is that kind, the screen reports "A network connection is required to install plugins. Reconnect and try again — your selection is kept." The catalog section also states up front that installing downloads from the network while the catalog itself stays browsable offline (it reads from a bundled file).onInstallProgressand are unchanged.listCatalog()stopped attaching the lifecycle view (installed,pinned,disabled,updateStatus,updateAvailable,downgradeVersions), which left every Update/Pin/Downgrade/Enable/Uninstall control in the install list dead — only the checkboxes worked. Re-attached (with thecanRollback: !disabled && hasBackuprefinement from the pre-1/5 code).Acceptance criteria addressed
screen.jscontains norequire(/module/ipcRenderer/child_process/fs.*/process.exit|env|cwd, and that everyplugins.<method>(call name in the screen is a method the preload bridge actually defines (installCatalog,resolveCatalog, …). Structurally the renderer runs withnodeIntegration: false,contextIsolation: true, and can only reachwindow.feedBackDesktop.*. The main process stays the only writer (installer staging + atomic swap, as documented indocs/PLUGIN_CATALOG.md).Verification
npm run typecheck— clean.npm test— 306 passed, 6 skipped; the only failure is the JUCE-submodule suite, which a clean checkout cannot populate (pre-existing).require()smuggled into the screen) — each made the relevant test fail.