fix(apps): sync installed_apps + installed_meta on MCP/CLI install - #88
Merged
KrasimirKralev merged 4 commits intoApr 19, 2026
Merged
KrasimirKralev merged 4 commits into
KrasimirKralev merged 4 commits into
Conversation
…stall Installs triggered via the MCP `app_install` tool (or CLI) landed the skill on disk, reloaded the gateway, and returned success — but the desktop never saw them. The Store UI's 'Installed' tab and the desktop icon grid both read from the `installed_apps` / `installed_meta` preferences, and those were only written by the AppStore React component as a client-side side-effect of the UI install flow. Off-desktop installs (MCP, CLI) flew under that radar. The install route now writes both preferences as the server-side source of truth. Metadata (name, color, iconUrl) is derived by looking up the Store listing for the appId; the category → color map that the UI uses is extracted to `src/lib/store-categories.ts` so both sides stay in step. Falls back to a title-cased slug and a neutral color when the Store API is unreachable. Uninstall mirrors the delete. Re-installs short-circuit — if the app is already listed and has meta, no Store fetch and no config write, so MCP retries don't thrash the upstream API. The desktop still needs a page refresh to see new installs (the React state is only seeded on mount). Live-refresh via SSE is deferred to a follow-up PR. Live-tested on Jetson: Gemma-initiated MCP `app_install` → skill on disk → preferences updated → skill appears on desktop + Store tab after refresh. Covers both fresh installs and already-installed (backfill) cases.
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 53 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCentralized store category metadata and an InstalledMeta type; installation now looks up remote store metadata (with fallback) and syncs Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant InstallAPI as Install API
participant StoreAPI as Store API
participant ConfigStore as Config Store
participant Gateway as Gateway
Client->>InstallAPI: POST /setup-api/apps/install
InstallAPI->>InstallAPI: Run 'openclaw skills install'
alt install succeeds
InstallAPI->>StoreAPI: Fetch app metadata (timeout: 8s)
alt Store metadata available
StoreAPI-->>InstallAPI: Return metadata (name, category, icon)
else lookup fails
InstallAPI->>InstallAPI: Build deterministic InstalledMeta fallback
end
InstallAPI->>ConfigStore: getAll(pref:installed_apps, pref:installed_meta)
ConfigStore-->>InstallAPI: Return existing prefs
InstallAPI->>InstallAPI: Merge/append appId and set metadata if missing
InstallAPI->>ConfigStore: setMany(updated prefs)
ConfigStore-->>InstallAPI: Persisted
InstallAPI->>Gateway: reloadGateway()
InstallAPI-->>Client: Success
else install fails
InstallAPI-->>Client: Error
end
sequenceDiagram
participant Client as Client
participant UninstallAPI as Uninstall API
participant FS as File System
participant ConfigStore as Config Store
participant Gateway as Gateway
Client->>UninstallAPI: POST /setup-api/apps/uninstall
UninstallAPI->>FS: Remove skill directory
UninstallAPI->>FS: Delete app icon
UninstallAPI->>ConfigStore: getAll(pref:installed_apps, pref:installed_meta)
ConfigStore-->>UninstallAPI: Return existing prefs
UninstallAPI->>UninstallAPI: Filter appId out of installed_apps
UninstallAPI->>UninstallAPI: Delete appId key from installed_meta
alt prefs changed
UninstallAPI->>ConfigStore: setMany(updated prefs)
ConfigStore-->>UninstallAPI: Persisted
else no changes needed
UninstallAPI->>UninstallAPI: Skip persistence
end
UninstallAPI->>Gateway: reloadGateway()
UninstallAPI-->>Client: Success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/setup-api/apps/install/route.ts`:
- Around line 99-125: Add a short inline comment above the re-install
short-circuit explaining that when lookupStoreMeta returns the title-cased
fallback it becomes sticky in pref:installed_meta (so subsequent re-installs
will not refresh real store metadata), and if you prefer automatic refresh
instead of a comment, change the condition in the if (!alreadyListed ||
!metaMap[appId]) check to also detect the fallback (e.g. add an isFallback check
comparing meta.color === DEFAULT_CATEGORY_COLOR && meta.name ===
titleCaseFromSlug(appId)) and allow a refetch by treating fallback as missing;
reference lookupStoreMeta, pref:installed_meta, pref:installed_apps,
configSetMany, DEFAULT_CATEGORY_COLOR and titleCaseFromSlug when making the
change.
- Around line 10-14: The titleCaseFromSlug function can return an empty string
for slugs made only of separators (e.g., "---" or "_"), so update
titleCaseFromSlug to split on both hyphens and underscores (e.g., split by
/[-_]+/), filter falsy parts, map to capitalized words, and if the resulting
array is empty return the original raw slug instead of "". Change the
implementation referenced by titleCaseFromSlug so InstalledMeta.name never
becomes an empty string when the appId regex allows separator-only values.
- Around line 16-40: In lookupStoreMeta, return the remote Store icon URL as the
InstalledMeta.iconUrl fallback instead of the local
`/setup-api/apps/icon/${appId}` path: when a matching store app is found use
iconUrl: `${STORE_ICONS_BASE}/${match.slug}.png` (or if using fallback when no
match, set iconUrl to that same STORE_ICONS_BASE path using appId/slug), while
keeping name, color, DEFAULT_CATEGORY_COLOR, and CATEGORY_COLORS logic intact;
update references to InstalledMeta and ensure STORE_ICONS_BASE is used so the
client (and InstalledAppIcon) has a remote second source if local icon download
failed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9ce3772a-64c1-43dc-b7c8-0333a6cc1f4a
📒 Files selected for processing (5)
src/app/page.tsxsrc/app/setup-api/apps/install/route.tssrc/app/setup-api/apps/uninstall/route.tssrc/components/AppStore.tsxsrc/lib/store-categories.ts
…apps-install-preferences-sync
- titleCaseFromSlug: guard against all-separator slugs ("---" → "")
by splitting on /[-_]+/ and falling back to raw slug when result is
empty. Matches the appId validator regex which permits underscores.
- lookupStoreMeta: use remote Store icon URL as meta iconUrl so the
client <InstalledAppIcon> has a second source when the local icon
download failed. Matches AppStore.tsx's apiToStoreApp shape so both
UI and MCP install paths produce identical meta.
- Re-install short-circuit: document the sticky-fallback tradeoff
inline so future readers don't accidentally re-introduce a refresh
path without understanding why it was avoided.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/setup-api/apps/install/route.ts`:
- Around line 142-144: The catch that currently only logs configSetMany failures
(inside the install route handling installed_apps/installed_meta updates) must
surface the error to callers: update the catch block around the configSetMany
call to either return a non-2xx response (throw an HttpError or use Response
with status 500) or modify the JSON response to set ok: false and include a
preferenceSyncError field with the error message (use err instanceof Error ?
err.message : String(err)); ensure this change is applied for both catch sites
handling preference sync so callers (MCP/CLI) can detect the failure instead of
receiving ok: true.
- Around line 40-46: Remote categories must be validated before indexing
CATEGORY_COLORS to avoid inherited keys or malicious values being used for
InstalledMeta.color; in the route where you compute color (using data, match,
appId), check that match.category is a non-empty string and that
Object.prototype.hasOwnProperty.call(CATEGORY_COLORS, match.category) (and
optionally that the looked-up value is a valid string/color) before using
CATEGORY_COLORS[match.category]; if the validation fails, use
DEFAULT_CATEGORY_COLOR instead.
- Around line 132-140: When adding an app where alreadyListed is false but
metaMap[appId] already exists, avoid refetching and overwriting that existing
metadata; change the logic in the branch around lookupStoreMeta/configSetMany so
you only call lookupStoreMeta(appId) and set "pref:installed_meta" when
metaMap[appId] is undefined, otherwise only add appId to "pref:installed_apps"
(i.e. build nextUpdates to reuse existing metaMap when present and only include
the fetched storeMeta when missing), then call configSetMany(nextUpdates).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e5322a2b-2cd7-421c-83c7-dad87e23009d
📒 Files selected for processing (1)
src/app/setup-api/apps/install/route.ts
Three fixes on `src/app/setup-api/apps/install/route.ts`: 1. Surface preferenceSyncError to callers (API contract) Previously the try/catch around config-store writes only logged the failure and still returned ok:true, so MCP/CLI callers couldn't tell whether the desktop would actually see the install. Now the response carries `preferenceSyncError` and `ok` flips false when it's set. 2. hasOwnProperty guard on CATEGORY_COLORS lookup (hardening) Malicious or unexpected `category` value from openclawhardware.dev (e.g. '__proto__', 'constructor') would otherwise resolve to an inherited property. Validate as string + ownProperty before indexing. 3. Don't overwrite existing meta on partial-state re-install (logic) Old branch condition `!alreadyListed || !metaMap[appId]` refetched and overwrote meta whenever the list entry was missing, even when meta was present. Split into two independent checks: only fetch Store meta when meta is absent; only append to list when not listed. Also update `src/tests/routes/apps/install.test.ts` so the config-store mock exports getAll/setMany (the route now uses them). 24/24 pass.
Contributor
Author
|
Addressed all 3 findings from the second review on 7a1f869: surface preferenceSyncError to callers + flip |
|
✅ Actions performedReview triggered.
|
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
1 participant
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.
Summary
Installs triggered via the MCP
app_installtool (or CLI) landed the skill on disk but were invisible on the desktop and in the Store's Installed tab. The Store UI filters "Installed" by theinstalled_appspreference, and the desktop filters icons by bothinstalled_appsANDinstalled_meta; those were only written as a client-side side-effect of the AppStore UI install flow. Off-desktop installs flew under that radar.This PR makes the server-side routes the source of truth:
src/app/setup-api/apps/install/route.tsnow writes bothpref:installed_apps(id) andpref:installed_meta(name + color + iconUrl)src/app/setup-api/apps/uninstall/route.tsmirrors the deletesrc/lib/store-categories.ts(new) holds the category→color map previously inlined inAppStore.tsx, so client and server stay in stepsrc/app/page.tsxandsrc/components/AppStore.tsximport the sharedInstalledMetatype / colour mapBehaviour
Scope / non-goals
Live refresh is not in this PR. The desktop's React state is seeded on mount, so newly installed skills appear after a hard refresh (Ctrl+Shift+R). Wiring server→client push (SSE) is a deferred follow-up — kept this PR small and focused on the correctness fix.
Test plan
app_installvia Gemma chat: skill on disk + preferences populated (verified on Jetson at 192.168.1.57)Summary by CodeRabbit
New Features
Bug Fixes
Improvements