feat(userspace): install-time permission consent UI (App Runtime M3)#1579
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds an app-import flow to StoreApp: an ChangesImport and consent flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ImportAppButton
participant InstallAPI
participant PermissionConsent
participant PermissionsAPI
User->>ImportAppButton: select and upload app file
ImportAppButton->>InstallAPI: installUserspaceApp(file)
InstallAPI-->>ImportAppButton: result (needs_consent, permissions)
ImportAppButton->>ImportAppButton: emitAppEvent(USERSPACE_APPS_CHANGED)
alt needs_consent
ImportAppButton->>PermissionConsent: render dialog with requested permissions
User->>PermissionConsent: click Allow
PermissionConsent->>PermissionsAPI: POST granted permissions
PermissionsAPI-->>PermissionConsent: response
PermissionConsent-->>ImportAppButton: onAllow(granted)
ImportAppButton->>ImportAppButton: emitAppEvent(USERSPACE_APPS_CHANGED)
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| * Returns null if the row doesn't exist or the request fails. */ | ||
| export async function fetchUserspaceAppRow(appId: string): Promise<UserspaceAppRow | null> { | ||
| try { | ||
| const res = await fetch("/api/userspace-apps"); |
There was a problem hiding this comment.
WARNING: fetchUserspaceAppRow downloads the entire userspace-app list to find a single row by id.
At scale this is wasteful (over-fetching every app's metadata/icons) and unnecessary work on the no-consent path (see ImportAppButton.tsx comment). Use a dedicated endpoint such as GET /api/userspace-apps/{app_id} that returns one row, mirroring the existing POST /api/userspace-apps/{app_id}/permissions route.
| const res = await fetch("/api/userspace-apps"); | |
| const res = await fetch(`/api/userspace-apps/${encodeURIComponent(appId)}`); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| setError(null); | ||
| try { | ||
| const result: InstallResult = await installUserspaceApp(file); | ||
| const row = await fetchUserspaceAppRow(result.app_id); |
There was a problem hiding this comment.
WARNING: fetchUserspaceAppRow is invoked on every install, including installs where result.needs_consent is false and no dialog will be shown.
The fetched row is only consumed when result.needs_consent === true (to populate appName and alreadyGranted). For the common all-free-capabilities case this is a wasted network roundtrip plus an unnecessary list query (see the full-list scan concern on userspace-apps.ts:106).
| const row = await fetchUserspaceAppRow(result.app_id); | |
| if (result.needs_consent) { | |
| const row = await fetchUserspaceAppRow(result.app_id); | |
| // The row is now enabled and visible regardless of consent -- only its | |
| // gated capabilities are withheld until the user answers below. | |
| emitAppEvent(USERSPACE_APPS_CHANGED); | |
| setConsent({ |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // The row is now enabled and visible regardless of consent -- only its | ||
| // gated capabilities are withheld until the user answers below. | ||
| emitAppEvent(USERSPACE_APPS_CHANGED); | ||
| if (result.needs_consent) { |
There was a problem hiding this comment.
SUGGESTION: Race condition / silent fallback.
installUserspaceApp resolves before the backend row is necessarily visible to a subsequent GET /api/userspace-apps (depending on commit/visibility semantics, replication, or cache). If the row is missing here, the consent dialog silently shows result.app_id as the app name instead of the user-facing name, and alreadyGranted defaults to [] -- so any re-consent after an update flow could appear to revoke earlier grants and resubmit them as 'new'.
Two concrete fixes worth considering: (a) ask the backend to include name and permissions_granted directly in the InstallResult for the fresh-install case (PR description notes this gap already exists), or (b) retry fetchUserspaceAppRow once with a short delay when row === null.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| * them. Mirrors tinyagentos/userspace/capabilities.py GATED_CAPS exactly -- | ||
| * keep in sync if the backend vocabulary changes. | ||
| */ | ||
| export const GATED_CAPS = new Set(["app.net", "app.agent", "app.llm", "app.memory"]); |
There was a problem hiding this comment.
WARNING: GATED_CAPS is a hand-maintained mirror of tinyagentos/userspace/capabilities.py. The doc comment even calls this out: 'keep in sync if the backend vocabulary changes.' Nothing enforces that.
Because this set directly controls whether the user is asked for consent (a security-relevant decision), divergence between frontend and backend would mean either (a) a gated capability silently slips through without a consent prompt, or (b) a non-gated capability forces an unnecessary prompt. Either is bad, and (a) is the dangerous one.
Options:
- Have the backend expose
GET /api/userspace-apps/capabilitiesreturninggated_caps+capability_descriptionsand consume it inPermissionConsent. - At minimum, add a CI check (or a test) that imports the python source or a generated JSON and asserts the frontend set matches.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit 7059445)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 7059445)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (6 files)
Reviewed by minimax-m3 · Input: 36.9K · Output: 4.6K · Cached: 202K |
Adds the PermissionConsent dialog for userspace app installs: lists each
requested capability with a plain-English description mirrored from
tinyagentos/userspace/capabilities.py, flags the gated ones (app.net,
app.agent, app.llm, app.memory), and Allow/Deny grants or skips them via
POST /api/userspace-apps/{app_id}/permissions.
Wires it into the Store's Apps tab via a new ImportAppButton that installs
a .taosapp package and shows the dialog whenever the install response sets
needs_consent.
Docs-Reviewed: frontend-only Store components (install-consent dialog + .taosapp import button); no README app-list or route surface change, userspace install/consent tracked under App Runtime epic #196
jaylfc
force-pushed
the
feat/userspace-consent-ui
branch
from
7059445 to
1e8494c
Compare
| } catch (e) { | ||
| setError(e instanceof Error ? e.message : "Install failed"); | ||
| } finally { | ||
| setBusy(false); |
There was a problem hiding this comment.
SUGGESTION: setBusy(false) in finally runs as soon as the consent dialog is shown, leaving the Import button enabled while the dialog is open.
Concrete failure mode:
- User picks package A -> install succeeds -> consent dialog A is shown, Import button is re-enabled.
- User clicks Import again and picks package B ->
setBusy(true), install runs in parallel with the open dialog. handleFilefor B resumes, callssetConsent(...)which overwrites dialog A's state.- User answers the (now-B) dialog -- the POST grants the cap set the backend returned for B, but dialog A was already shown -- and the first app's pending grant never reaches the server.
Suggested fix: keep busy true (or track a separate consentOpen flag) until the dialog is closed (onAllow / onDeny)
| setBusy(false); | |
| setBusy(false); |
to keep the existing behavior; or gate the button on consent === null while the dialog is open.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
desktop/src/apps/StoreApp/PermissionConsent.test.tsx (1)
43-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for the error path and backdrop-click dismissal.
The success path for Allow and the button-based Deny are covered, but the
catchbranch inhandleAllow(error banner + re-enabling buttons) and the backdrop-click-to-deny path aren't exercised by any test here.🤖 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 `@desktop/src/apps/StoreApp/PermissionConsent.test.tsx` around lines 43 - 88, The PermissionConsent tests only cover the Allow success path and the Deny button path, so add cases for the missing branches in PermissionConsent.handleAllow and the backdrop dismissal behavior. In PermissionConsent.test.tsx, add a test that stubs fetch to reject or return a non-ok response, then verifies the error banner appears and the action buttons are re-enabled after the catch path runs. Also add a test that simulates clicking the dialog backdrop and asserts it triggers the same deny flow as onDeny, using the PermissionConsent component and its allow/deny controls to locate the relevant behavior.desktop/src/lib/userspace-apps.ts (1)
100-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider sharing fetch logic with
fetchUserspaceApps.
fetchUserspaceAppRowduplicates thefetch("/api/userspace-apps")+ parse + error-swallow logic already present infetchUserspaceApps(Lines 115-124). Extracting a sharedfetchUserspaceAppRows()helper would avoid the two implementations drifting (e.g., one adding auth headers or error handling the other misses).♻️ Suggested refactor
+async function fetchAllUserspaceAppRows(): Promise<UserspaceAppRow[]> { + const res = await fetch("/api/userspace-apps"); + if (!res.ok) return []; + return (await res.json()) as UserspaceAppRow[]; +} + export async function fetchUserspaceAppRow(appId: string): Promise<UserspaceAppRow | null> { try { - const res = await fetch("/api/userspace-apps"); - if (!res.ok) return null; - const rows = (await res.json()) as UserspaceAppRow[]; - return rows.find((r) => r.app_id === appId) ?? null; + const rows = await fetchAllUserspaceAppRows(); + return rows.find((r) => r.app_id === appId) ?? null; } catch { return null; } } export async function fetchUserspaceApps(): Promise<AppManifest[]> { - let rows: UserspaceAppRow[]; try { - const res = await fetch("/api/userspace-apps"); - if (!res.ok) return []; - rows = (await res.json()) as UserspaceAppRow[]; + const rows = await fetchAllUserspaceAppRows(); + return rows.filter((r) => r.enabled).map(toAppManifest); } catch { return []; } - return rows.filter((r) => r.enabled).map(toAppManifest); }🤖 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 `@desktop/src/lib/userspace-apps.ts` around lines 100 - 114, Refactor the duplicated `/api/userspace-apps` fetch-and-parse logic in `fetchUserspaceAppRow` so it shares the same implementation as `fetchUserspaceApps`. Extract a reusable helper like `fetchUserspaceAppRows()` and have both `fetchUserspaceApps` and `fetchUserspaceAppRow` call it, keeping the current null-on-failure behavior while centralizing any future auth header, parsing, or error-handling changes in one place.
🤖 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 `@desktop/src/apps/StoreApp/ImportAppButton.tsx`:
- Around line 32-44: The consent flow in ImportAppButton after
installUserspaceApp should not proceed with an empty permission baseline when
fetchUserspaceAppRow() returns null. Update the logic around setConsent so that
a missing row is treated as a lookup failure (for example by blocking consent,
retrying the fetch, or surfacing an error) instead of defaulting alreadyGranted
to an empty array, and keep the existing row?.permissions_granted path tied to
the consent setup.
In `@desktop/src/apps/StoreApp/PermissionConsent.tsx`:
- Around line 79-88: Guard the dismissal paths in PermissionConsent so the modal
cannot be closed while a grant request is in flight. In PermissionConsent’s
portal container, update the backdrop click and Escape handling to check the
submitting state before calling onDeny(), matching the existing disabled
behavior on the Allow/Deny actions. Keep the fix localized to PermissionConsent
and its submit flow so grantUserspacePermissions cannot resolve after the dialog
has been dismissed.
---
Nitpick comments:
In `@desktop/src/apps/StoreApp/PermissionConsent.test.tsx`:
- Around line 43-88: The PermissionConsent tests only cover the Allow success
path and the Deny button path, so add cases for the missing branches in
PermissionConsent.handleAllow and the backdrop dismissal behavior. In
PermissionConsent.test.tsx, add a test that stubs fetch to reject or return a
non-ok response, then verifies the error banner appears and the action buttons
are re-enabled after the catch path runs. Also add a test that simulates
clicking the dialog backdrop and asserts it triggers the same deny flow as
onDeny, using the PermissionConsent component and its allow/deny controls to
locate the relevant behavior.
In `@desktop/src/lib/userspace-apps.ts`:
- Around line 100-114: Refactor the duplicated `/api/userspace-apps`
fetch-and-parse logic in `fetchUserspaceAppRow` so it shares the same
implementation as `fetchUserspaceApps`. Extract a reusable helper like
`fetchUserspaceAppRows()` and have both `fetchUserspaceApps` and
`fetchUserspaceAppRow` call it, keeping the current null-on-failure behavior
while centralizing any future auth header, parsing, or error-handling changes in
one place.
🪄 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: CHILL
Plan: Pro Plus
Run ID: c43f9d74-ec39-4d7a-9479-f59fbdd05d57
📒 Files selected for processing (6)
desktop/src/apps/StoreApp/ImportAppButton.test.tsxdesktop/src/apps/StoreApp/ImportAppButton.tsxdesktop/src/apps/StoreApp/PermissionConsent.test.tsxdesktop/src/apps/StoreApp/PermissionConsent.tsxdesktop/src/apps/StoreApp/index.tsxdesktop/src/lib/userspace-apps.ts
| const result: InstallResult = await installUserspaceApp(file); | ||
| const row = await fetchUserspaceAppRow(result.app_id); | ||
| // The row is now enabled and visible regardless of consent -- only its | ||
| // gated capabilities are withheld until the user answers below. | ||
| emitAppEvent(USERSPACE_APPS_CHANGED); | ||
| if (result.needs_consent) { | ||
| setConsent({ | ||
| appId: result.app_id, | ||
| appName: row?.name ?? result.app_id, | ||
| requested: result.new_permissions, | ||
| alreadyGranted: row?.permissions_granted ?? [], | ||
| }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first
ast-grep outline desktop/src/apps/StoreApp/ImportAppButton.tsx --view expanded
ast-grep outline desktop/src/lib/userspace-apps.ts --view expanded
# Locate the consent flow and permission update path
rg -n "setConsent|alreadyGranted|needs_consent|new_permissions|permissions_granted|Allow|grant" desktop/src -S
# Show the relevant slices with line numbers
sed -n '1,220p' desktop/src/apps/StoreApp/ImportAppButton.tsx
printf '\n--- userspace-apps.ts ---\n'
sed -n '1,220p' desktop/src/lib/userspace-apps.tsRepository: jaylfc/taOS
Length of output: 38693
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the consent component and its tests
sed -n '1,220p' desktop/src/apps/StoreApp/PermissionConsent.tsx
printf '\n--- PermissionConsent.test.tsx ---\n'
sed -n '1,220p' desktop/src/apps/StoreApp/PermissionConsent.test.tsx
# Check any tests around ImportAppButton behavior
printf '\n--- ImportAppButton.test.tsx ---\n'
sed -n '1,220p' desktop/src/apps/StoreApp/ImportAppButton.test.tsx
# Find any backend/API docs or code comments describing replacement vs merge semantics
rg -n "replace.*permissions|permissions column|alreadyGranted|granted.*merge|granted.*replace|new_permissions|grantUserspacePermissions|permissions/" desktop/src -SRepository: jaylfc/taOS
Length of output: 16285
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the backend handler for userspace app permission grants
rg -n "/userspace-apps/.*/permissions|grantUserspacePermissions|permissions_granted|granted" . -S --glob '!desktop/src/**'
# Narrow to likely Python/backend files if present
fd -a -t f 'userspace' .
fd -a -t f 'permissions' . | sed -n '1,120p'Repository: jaylfc/taOS
Length of output: 47510
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '288,312p' tinyagentos/routes/userspace_apps.py
printf '\n--- related test ---\n'
sed -n '372,430p' tests/test_routes_userspace_apps.pyRepository: jaylfc/taOS
Length of output: 3142
Handle a failed row lookup before opening consent
fetchUserspaceAppRow() returning null here falls back to alreadyGranted: [], and the Allow path posts the merged list as the full permission set. On a re-consent/update flow, that can overwrite permissions_granted with only the newly requested caps and silently drop earlier grants. Surface the lookup failure or block/retry instead of proceeding with an empty baseline.
🤖 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 `@desktop/src/apps/StoreApp/ImportAppButton.tsx` around lines 32 - 44, The
consent flow in ImportAppButton after installUserspaceApp should not proceed
with an empty permission baseline when fetchUserspaceAppRow() returns null.
Update the logic around setConsent so that a missing row is treated as a lookup
failure (for example by blocking consent, retrying the fetch, or surfacing an
error) instead of defaulting alreadyGranted to an empty array, and keep the
existing row?.permissions_granted path tied to the consent setup.
| return createPortal( | ||
| <div | ||
| className="fixed inset-0 z-50 flex items-center justify-center bg-black/50" | ||
| onClick={(e) => { | ||
| if (e.target === e.currentTarget) onDeny(); | ||
| }} | ||
| onKeyDown={(e) => { | ||
| if (e.key === "Escape") onDeny(); | ||
| }} | ||
| > |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard backdrop-click/Escape dismissal while a grant request is in flight.
The Allow/Deny buttons are disabled during submitting, but the backdrop click and Escape-key paths are not. A user can click Allow, then hit Escape or click outside before the POST resolves — the dialog unmounts via onDeny() while the in-flight grantUserspacePermissions request is still pending, and its resolution can later call onAllow(granted) on an already-dismissed flow (or update state on an unmounted component). This creates ambiguous "denied vs. actually granted" outcomes for a security-sensitive permission grant.
🔒 Proposed fix
<div
className="fixed inset-0 z-50 flex items-center justify-center bg-black/50"
onClick={(e) => {
- if (e.target === e.currentTarget) onDeny();
+ if (submitting) return;
+ if (e.target === e.currentTarget) onDeny();
}}
onKeyDown={(e) => {
- if (e.key === "Escape") onDeny();
+ if (submitting) return;
+ if (e.key === "Escape") onDeny();
}}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return createPortal( | |
| <div | |
| className="fixed inset-0 z-50 flex items-center justify-center bg-black/50" | |
| onClick={(e) => { | |
| if (e.target === e.currentTarget) onDeny(); | |
| }} | |
| onKeyDown={(e) => { | |
| if (e.key === "Escape") onDeny(); | |
| }} | |
| > | |
| return createPortal( | |
| <div | |
| className="fixed inset-0 z-50 flex items-center justify-center bg-black/50" | |
| onClick={(e) => { | |
| if (submitting) return; | |
| if (e.target === e.currentTarget) onDeny(); | |
| }} | |
| onKeyDown={(e) => { | |
| if (submitting) return; | |
| if (e.key === "Escape") onDeny(); | |
| }} | |
| > |
🤖 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 `@desktop/src/apps/StoreApp/PermissionConsent.tsx` around lines 79 - 88, Guard
the dismissal paths in PermissionConsent so the modal cannot be closed while a
grant request is in flight. In PermissionConsent’s portal container, update the
backdrop click and Escape handling to check the submitting state before calling
onDeny(), matching the existing disabled behavior on the Allow/Deny actions.
Keep the fix localized to PermissionConsent and its submit flow so
grantUserspacePermissions cannot resolve after the dialog has been dismissed.
… badge + slider render (#1589) * feat(userspace): install-time permission consent dialog (#1579) Adds the PermissionConsent dialog for userspace app installs: lists each requested capability with a plain-English description mirrored from tinyagentos/userspace/capabilities.py, flags the gated ones (app.net, app.agent, app.llm, app.memory), and Allow/Deny grants or skips them via POST /api/userspace-apps/{app_id}/permissions. Wires it into the Store's Apps tab via a new ImportAppButton that installs a .taosapp package and shows the dialog whenever the install response sets needs_consent. Docs-Reviewed: frontend-only Store components (install-consent dialog + .taosapp import button); no README app-list or route surface change, userspace install/consent tracked under App Runtime epic #196 * fix(providers): report true provider status; drop contradictory Error badge + slider render (#1578) BackendCatalog.get_lifecycle_state() fabricated "running" for any backend LifecycleManager never started/stopped (the default for a manually-added, auto_manage=false provider), so the Providers dialog rendered that fake "Running" pill next to the real, live-probed "Error" status -- a contradiction with no basis in fact. Only surface lifecycle_state when a backend is actually under LifecycleManager control. Separately, the Add Provider wizard's rkllama default URL was still the pre-beta.20 legacy port (localhost:8080); the installer's real default is 7833, so a wizard-added rkllama entry silently pointed at a dead port and was truthfully (not falsely) reported unreachable. The slider/toggle thumbs were absolutely positioned with only a top offset and no left anchor, so their un-transformed position depended on the browser's default static-position fallback instead of the button's edge -- overflowing the track entirely in the "on" state. Anchor with left-0.5 to match the existing top-0.5.
1 participant
Summary
PermissionConsent— the install-time consent dialog for userspace apps. Lists each requested capability with a plain-English description mirrored fromtinyagentos/userspace/capabilities.py, visually flags the gated ones (app.net,app.agent,app.llm,app.memory), and Allow/Deny posts the granted set (or nothing) toPOST /api/userspace-apps/{app_id}/permissions.ImportAppButtonin the Store's Apps tab (next to taOS Apps) so a.taosapppackage can actually be installed from the UI, and wires the dialog to show whenever the install response hasneeds_consent: true.fetchUserspaceAppRowtolib/userspace-apps.tsso the dialog can show the app's real name and merge with any already-granted permissions (the permissions column is replaced wholesale server-side, not patched).Completes the M3 frontend for the App Runtime epic (#196); backend permission plumbing was already on
dev.Test plan
npx vitest run src/apps/StoreApp/PermissionConsent.test.tsx src/apps/StoreApp/ImportAppButton.test.tsx— 8 passednpx vitest run(full desktop suite) — 2365 passed, 285 filesnpm run build(tsc -b && vite build) — cleanSummary by CodeRabbit
.taosappor.zipfiles directly.