feat(registerApp): pass requiredPermissions to apps.json#1148
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR enhances profiler reporting with concurrent fetch deduplication, gates auth flow via waitlist phase, refactors canvas edge-state synchronization for direct ReactFlow updates, integrates config-driven subscription flow in VS Code, adjusts cProfile tree analysis, and conditionally emits app permissions in manifests. ChangesProfiler Report Fetching, Caching, and Copy Feedback
Auth Bootstrap and Waitlist Access Flow
Canvas Edge Ownership and ReactFlow State Synchronization
Graph Edge and Component Serialization
VS Code Config-Driven Subscription Flow
Server Profiler and Manifest Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID. Do not edit or delete. |
![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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/shared-ui/src/components/canvas/context/FlowGraphContext.tsx (1)
845-854:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEdge removal in
onNodesDeletedoes not notify the host.When nodes are deleted,
onNodesDeleteremoves connected edges viasetEdges, but unlikeonEdgesChange, it never callsonContentUpdated. This creates a silent mutation where the host's project state diverges from the canvas.Consider notifying the host after filtering edges:
const onNodesDelete = useCallback( (deletedNodes: FlowNode[]) => { // Remove all edges connected to any of the deleted nodes in one pass const connected = getConnectedEdges(deletedNodes, edges); const connectedSet = new Set(connected.map((e) => e.id)); const remaining = edges.filter((e) => !connectedSet.has(e.id)); setEdges(remaining); + if (connected.length > 0) { + onContentUpdated(); + } }, - [edges, setEdges] + [edges, setEdges, onContentUpdated] );🤖 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 `@packages/shared-ui/src/components/canvas/context/FlowGraphContext.tsx` around lines 845 - 854, onNodesDelete currently filters out edges connected to deleted nodes using getConnectedEdges and setEdges but never notifies the host; update onNodesDelete (the function that calls getConnectedEdges, creates connectedSet and calls setEdges) to call onContentUpdated after updating edges so the host receives the new project state (pass the new edges/content payload the same way onEdgesChange does). Ensure you reference the same edge list produced in remaining and invoke onContentUpdated with that updated state immediately after setEdges to avoid a silent divergence.
🤖 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 `@apps/profiler-ui/src/views/ProfilerView.tsx`:
- Around line 292-293: fetchReport suffers a stale-response race: an earlier
network response can arrive after state was cleared/target changed and overwrite
the UI; fix by associating each fetchReport call with a unique token or
AbortController (e.g., increment a requestId or create an AbortController in the
scope where fetchReport is called), store the current token on the component (or
ref), pass it into fetchReport, and before applying any setState in fetchReport
verify the token matches the current token (or that the request was not aborted)
and that the selected target/session still matches; apply the same guard pattern
to all locations invoking fetchReport (and any other async handlers referenced
by statusIntervalRef / tasksIntervalRef) so only the latest response can update
the tree/report.
In `@apps/profiler-ui/src/views/visualizations/ReportText.tsx`:
- Around line 101-104: The fallback copy path in ReportText.tsx uses
document.execCommand('copy') but ignores its boolean return value, causing
setCopied(true) even on failure; update the copy logic in the ReportText
component to check the result of document.execCommand('copy') and only call
setCopied(true) (and remove the textarea) when it returns true, otherwise handle
failure by not setting copied and attempting a navigator.clipboard.writeText
fallback (with proper try/catch) or setting an error state so the UI does not
display "Copied!" erroneously.
In `@apps/shell-ui/src/components/layout/Shell.tsx`:
- Around line 246-251: The code treats a missing ConnectResult.waitlisted as
false and can wrongly exit waitlist mode; update the conditional inside the
mountedRef.current block (where setIdentity(result) is called and renderPhase is
checked) to only transition out of 'waitlisted' when waitlisted is explicitly
false (e.g., check result.waitlisted === false or presence + false) rather than
using !result.waitlisted so partial/omitted updates don't flip the user out of
waitlist; keep the existing checks for renderPhase and use
setRenderPhase('shell') only when the explicit boolean indicates removal from
the waitlist.
In `@apps/vscode/src/providers/AccountProvider.ts`:
- Around line 860-883: The fallback message in handleSubscribe is inconsistent
with the attempted action (it opens a new untitled pipeline file via
vscode.commands.executeCommand('workbench.action.files.newUntitledFile', {
languageId: 'rocketride-pipeline' })); update the fallback shown in the catch
block to instruct the user to create/open a new pipeline file or open a new
untitled .rrpipe pipeline file (mirror the "new untitled file" action) so the
message accurately reflects what the code tried to do.
In `@packages/ai/src/ai/common/cprofile_manager.py`:
- Around line 349-359: The min_pct threshold uses the max of per-entry cumtime
causing hotspots to dominate; in the cprofile_manager logic replace the max
aggregation with a sum of ct across stats_data (i.e., compute total_time =
sum(ct for each entry)) and keep total_calls as the sum of nc so min_time =
total_time * (min_pct/100.0) is based on the full tree time; update the same
pattern found in the later block that computes min_time (the second occurrence
around the other aggregation) to use summed ct instead of max to ensure pruning
and reported totals are relative to the entire tree.
- Around line 390-399: The code currently only promotes phantom callers to roots
when root_keys is empty; change the logic so phantom-callers are always promoted
(or merged) into root_keys instead of being dropped when normal roots exist:
compute phantom_callers as set(children_map.keys()) - set(stats_data.keys()),
build promoted by collecting child_key from children_map entries (same loop
using children_map and _c_cc/_c_nc/_c_tt/_c_ct), then set root_keys =
list(set(root_keys) | promoted) (or extend root_keys with promoted while
avoiding duplicates) so phantom-rooted branches are preserved alongside normal
roots.
In
`@packages/shared-ui/src/components/canvas/components/node/node-component/run-button/RunButton.tsx`:
- Line 76: Remove the debug console.log statements in the RunButton component:
delete the console.log calls that print `[RunButton] clicked nodeId=${nodeId}
isRunning=${isRunning} isSubscribed=${isSubscribed}
onRunPipeline=${!!onRunPipeline}` (and the similar one at line 86) inside the
RunButton.tsx event handlers; if runtime diagnostics are required, replace them
with the project's logging utility (use the shared logger) or conditional
debug-level logging rather than console.log, keeping the logic in
onClick/onRunPipeline handlers and preserving nodeId, isRunning, isSubscribed,
and onRunPipeline usage.
In `@packages/shared-ui/src/components/canvas/context/FlowGraphContext.tsx`:
- Around line 1095-1102: The useMemo dependency array includes stable references
that can be removed to reduce verbosity: remove canvasRef, setTempNode,
setEditingNodeId, setQuickAddState, and setConfigSnackbar from the dependency
list inside the useMemo that builds the flow graph context (the dependency array
that currently lists canvasRef, nodes, edges, nodeMap, setNodes, setEdges,
onNodesChange, onEdgesChange, onEdgeConnect, isValidConnection, onDragOver,
onDrop, onNodeDragStop, addNode, updateNode, deleteNode, onNodesDelete,
tempNode, setTempNode, focusOnNode, editingNodeId, setEditingNodeId,
onContentUpdated, loadData, loadCanvas, isFlowReady, quickAddState,
setQuickAddState, configSnackbar, setConfigSnackbar); keep only the actual
changing values (nodes, edges, nodeMap, tempNode, editingNodeId, quickAddState,
configSnackbar and the callback refs that may change) and leave stable
refs/setters out.
- Around line 689-694: In FlowGraphContext.tsx remove the no-op iteration that
causes the ESLint no-empty error: delete the for (const n of nodes) { const nd =
n.data as any; if (nd.input?.length || nd.control?.length) { } } block (or
replace it with meaningful logging if you intended to keep the check); this
removes the empty loop over nodes and resolves the lint warning without changing
behavior elsewhere in the FlowGraphContext component.
- Around line 1016-1017: In FlowGraphContext.tsx remove the empty else branch
that remains at the end of the conditional (the bare "else { }" shown in the
diff) to satisfy ESLint no-empty; update the surrounding conditional in the
FlowGraphContext component (or the enclosing function where the if/else lives)
by deleting the else block entirely and, if needed, consolidate any remaining
logic into the if branch or add a meaningful fallback action instead of leaving
an empty block.
- Around line 565-567: The empty conditional block after computing removes
(const removes = changes.filter((c) => c.type === 'remove'); if (removes.length
> 0) { }) should be removed to satisfy ESLint no-empty; either delete the entire
if (removes.length > 0) { } statement or implement the intended handling for
removed changes inside the FlowGraphContext code path where changes and removes
are computed so the removes variable is used (refer to the removes and changes
identifiers to locate the spot).
In `@packages/shared-ui/src/components/canvas/hooks/useTemplateInstantiator.ts`:
- Line 104: Remove the debug console.log statements from the
useTemplateInstantiator hook: delete the console.log call that prints
"[TemplateInstantiator] post-ready: notifying host, nodes=..., edges=...,
components=..., docRevision=..." and the other debug console.log further down in
the same file; if telemetry is needed replace them with the project's logger
(e.g. processLogger.debug or similar) or remove entirely so no console.* calls
remain in useTemplateInstantiator.
In `@packages/shared-ui/src/components/canvas/util/graph.ts`:
- Around line 244-246: The variable nodesWithConnections is computed but never
used; update the loop in the same scope so it iterates over nodesWithConnections
instead of nodes (or remove the unused filter if unnecessary) to avoid dead code
and ensure only nodes with data.control or data.input are processed—look for the
const nodesWithConnections = nodes.filter(...) and the subsequent for (const
node of nodes) loop in graph.ts and change the loop to for (const node of
nodesWithConnections) or delete the filter if not needed.
- Around line 328-331: The classType extraction for invoke edges is fragile
because edge.sourceHandle.split('.').at(1) truncates any dots inside the class
name; update the parsing in the block where edge.sourceHandle is checked (the
branch using edge.sourceHandle?.startsWith('invoke-source') and the control.push
call) to take everything after the first dot instead of only the second segment
— e.g. remove the prefix up to the first '.' and treat the remainder as
classType (or use split('.').slice(1).join('.') behavior) before pushing {
classType, from: edge.source }.
- Around line 332-336: The current extraction of the lane uses
edge.sourceHandle?.split('-')?.at(1), which discards any additional '-'
characters in the lane name; change the logic that computes lane (the variable
used before input.push({ lane, from: edge.source })) to preserve everything
after the first '-' (e.g. find the first '-' with indexOf and take substring
from that point +1, or split and join the remainder with slice(1).join('-')) so
multi-dash lane identifiers are not truncated.
---
Outside diff comments:
In `@packages/shared-ui/src/components/canvas/context/FlowGraphContext.tsx`:
- Around line 845-854: onNodesDelete currently filters out edges connected to
deleted nodes using getConnectedEdges and setEdges but never notifies the host;
update onNodesDelete (the function that calls getConnectedEdges, creates
connectedSet and calls setEdges) to call onContentUpdated after updating edges
so the host receives the new project state (pass the new edges/content payload
the same way onEdgesChange does). Ensure you reference the same edge list
produced in remaining and invoke onContentUpdated with that updated state
immediately after setEdges to avoid a silent divergence.
🪄 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: c913d9c8-4b6d-40e0-979e-e052db246a11
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (35)
apps/profiler-ui/package.jsonapps/profiler-ui/src/views/ProfilerView.tsxapps/profiler-ui/src/views/visualizations/FlameGraph.tsxapps/profiler-ui/src/views/visualizations/ReportText.tsxapps/profiler-ui/src/views/visualizations/StatsTable.tsxapps/profiler-ui/src/views/visualizations/SunburstChart.tsxapps/profiler-ui/src/views/visualizations/types.tsapps/shell-ui/src/components/layout/Shell.tsxapps/shell-ui/src/connection/connection.tsapps/shell-ui/src/lib/appLoader.tsapps/vscode/src/providers/AccountProvider.tsapps/vscode/src/providers/SettingsProvider.tsapps/vscode/src/providers/SidebarProvider.tsapps/vscode/src/providers/views/Account/AccountWebview.tsxapps/vscode/src/providers/views/Settings/SettingsWebview.tsxapps/vscode/src/providers/views/Sidebar/SidebarWebview.tsxpackages/ai/src/ai/account/models.pypackages/ai/src/ai/common/cprofile_manager.pypackages/ai/src/ai/modules/task/commands/cmd_account.pypackages/ai/src/ai/modules/task/commands/cmd_cprofile.pypackages/ai/src/ai/web/server.pypackages/client-python/src/rocketride/mixins/cprofile.pypackages/client-python/src/rocketride/types/client.pypackages/client-typescript/src/client/client.tspackages/client-typescript/src/client/types/client.tspackages/client-typescript/src/client/types/cprofile.tspackages/shared-ui/src/components/canvas/components/edge/FlowEdge.tsxpackages/shared-ui/src/components/canvas/components/node/node-component/run-button/RunButton.tsxpackages/shared-ui/src/components/canvas/context/FlowGraphContext.tsxpackages/shared-ui/src/components/canvas/context/FlowProjectContext.tsxpackages/shared-ui/src/components/canvas/hooks/useTemplateInstantiator.tspackages/shared-ui/src/components/canvas/util/graph.tspackages/shared-ui/src/modules/account/AccountView.tsxpackages/shared-ui/src/modules/account/components/BillingPanel.tsxscripts/lib/registerApp.js
💤 Files with no reviewable changes (1)
- apps/shell-ui/src/lib/appLoader.ts
Rod-Christensen
added a commit
that referenced
this pull request
Jun 8, 2026
- ProfilerView: guard fetchReport with fetchIdRef to prevent stale async
responses from overwriting UI when target changes mid-flight
- ReportText: check execCommand('copy') return value before showing
"Copied!" feedback
- Shell: use `result.waitlisted === false` instead of `!result.waitlisted`
so omitted/undefined fields don't wrongly exit waitlist mode
- AccountProvider: align fallback message with the actual action (create
or open a pipeline file, not specifically .rrpipe)
- cprofile_manager: use sum (not max) for total_time so min_pct threshold
is relative to full tree; always promote phantom-caller roots instead
of only when no normal roots exist
- RunButton, useTemplateInstantiator: remove debug console.log statements
- FlowGraphContext: remove empty blocks/iterations (ESLint no-empty),
notify host via onContentUpdated in onNodesDelete, trim stable refs
from useMemo dependency array
- graph.ts: iterate nodesWithConnections instead of unused filter; use
robust parsing for classType (dots) and lane (dashes) in sourceHandle
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Apps can now declare a requiredPermissions array in their appManifest. The build-time registration script forwards this field into apps.json entries so the server can persist it and enforce permission-based visibility filtering at runtime. Part of the server-side app permission filtering feature — apps that declare requiredPermissions will only be returned to users who hold ALL listed sysPermission strings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- ProfilerView: guard fetchReport with fetchIdRef to prevent stale async
responses from overwriting UI when target changes mid-flight
- ReportText: check execCommand('copy') return value before showing
"Copied!" feedback
- Shell: use `result.waitlisted === false` instead of `!result.waitlisted`
so omitted/undefined fields don't wrongly exit waitlist mode
- AccountProvider: align fallback message with the actual action (create
or open a pipeline file, not specifically .rrpipe)
- cprofile_manager: use sum (not max) for total_time so min_pct threshold
is relative to full tree; always promote phantom-caller roots instead
of only when no normal roots exist
- RunButton, useTemplateInstantiator: remove debug console.log statements
- FlowGraphContext: remove empty blocks/iterations (ESLint no-empty),
notify host via onContentUpdated in onNodesDelete, trim stable refs
from useMemo dependency array
- graph.ts: iterate nodesWithConnections instead of unused filter; use
robust parsing for classType (dots) and lane (dashes) in sourceHandle
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rod-Christensen
force-pushed
the
feat/hide-sysapps
branch
from
6494e38 to
c773ddb
Compare
9 tasks
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
requiredPermissionsmanifest field from apppackage.jsonthroughregisterApp.jsintoapps.jsonentriesTest plan
requiredPermissions: ["sys.admin"]in its manifestapps.jsoncontains the field after./builder <app>:build🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements