refactor(frontend): the OSS chat slice runs on @agenta/chat, off antd and antd-x - #5871
Draft
ardaerzin wants to merge 2 commits into
Draft
refactor(frontend): the OSS chat slice runs on @agenta/chat, off antd and antd-x#5871ardaerzin wants to merge 2 commits into
ardaerzin wants to merge 2 commits into
Conversation
Contributor
Author
|
@coderabbitai review |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Action performedReview finished.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) 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:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe agent chat now uses shared Agenta chat, state, skin, and UI packages. It replaces local markdown, attachment, approval, session, and Ant Design implementations while updating session mounting, split-pane behavior, message rendering, and application-level styling. ChangesAgent chat migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AgentChatPanel
participant AgentConversation
participant ChatComposer
participant ChatSkin
User->>AgentChatPanel: select session
AgentChatPanel->>AgentConversation: reveal visited conversation
AgentConversation->>ChatComposer: render shared composer
User->>ChatComposer: submit message or approval response
ChatComposer->>AgentConversation: send chat action
AgentConversation->>ChatSkin: resolve shared tool or approval rendering
ChatSkin-->>AgentConversation: return rendered interaction
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/oss/src/components/AgentChatSlice/AgentChatPanel.tsx (1)
100-110: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winConfirm the pending-session effect cannot double-fire.
The ref-based consumption guard was removed. The effect now calls
adoptSessionoraddSessionand thensetPendingOpen(null). Under React StrictMode the mount effect runs twice with the same render closure, sopendingOpenForScopeis still non-null on the second run. In the no-id branch this callsaddSession()twice and can leave a stray blank tab. The comment on Line 95 still states "Consumed once".🛡️ Proposed guard
+ const consumedPendingRef = useRef<string | null>(null) useEffect(() => { if (!pendingOpenForScope) return + const key = `${pendingOpenForScope.appId}:${pendingOpenForScope.sessionId ?? ""}` + if (consumedPendingRef.current === key) return + consumedPendingRef.current = key if (pendingOpenForScope.sessionId) {web/oss/src/components/AgentChatSlice/hooks/useFirstRunSeed.ts (1)
107-124: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore an attachment gate, or the seeded first turn can be dropped.
attachmentsSettledno longer gates the auto-send. Seed files are staged asynchronously at Line 57 throughonSeedFiles, which isattachments.addFilesinAgentConversation. The auto-send now fires as soon as the model and overlay conditions hold, while those files are still uploading.The consumer then drops the turn:
web/oss/src/components/AgentChatSlice/AgentConversation.tsxLine 430:if (!attachmentsSettled) returnreturns early and schedules no retry.- Line 115 here sets
autoStartedSeedRef.current = truebefore the submit, so the effect never fires again.The path is reachable:
useStartAgentSessionsendsseedFileswithautoSend: true. The result is a silently lost first message.Either restore the parameter and gate, or set the latch only after the submit is accepted.
🐛 Proposed fix — re-gate on attachment settlement
handleSubmitRef, onSeedFiles, + attachmentsSettled = true, }: { @@ onSeedFiles?: (files: File[]) => void + /** Seeded files are staged asynchronously; hold the auto-send until they settle. */ + attachmentsSettled?: boolean }) => { @@ if ((!seedWasBlockedRef.current && !firstRunAutoSend) || messagesCount > 0) return // Hold the auto-send until the build-kit overlay settles (or the 10s bound elapses). if (!overlayReady && !overlayWaitElapsed) return + // Seeded files upload after staging; sending now would be refused by `handleSubmit`. + if (!attachmentsSettled) return autoStartedSeedRef.current = true handleSubmitRef.current(firstRunPrompt) }, [ firstRunPrompt, firstRunAutoSend, modelBlocked, messagesCount, overlayReady, overlayWaitElapsed, + attachmentsSettled, ])Then pass it back in
web/oss/src/components/AgentChatSlice/AgentConversation.tsx:modelBlocked, handleSubmitRef, + attachmentsSettled,
🧹 Nitpick comments (4)
web/oss/src/components/AgentChatSlice/assets/markdown.tsx (1)
211-215: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize
Markdownor remove the memoization claim.
Markdownis a plain component. During a streamed message update, settled Markdown parts can render again and invoke Streamdown with unchangedcontentandclassName. WrapMarkdowninReact.memoso the optimization described here is active.Verify with React Profiler that settled message parts do not re-render for each text delta.
As per coding guidelines, “Minimize React re-renders with
useMemo,useCallback, andReact.memowhere appropriate.”Source: Coding guidelines
web/oss/src/components/AgentChatSlice/components/AgentTranscript.tsx (1)
100-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace static inline styles with Tailwind utilities.
heightandflexShrinkare constants. UseclassName="h-14 shrink-0"at both sites.Proposed change
-<div style={{height: 56, flexShrink: 0}} /> +<div className="h-14 shrink-0" />As per coding guidelines, “Prefer Tailwind utility classes over CSS-in-JS or separate CSS files.”
Also applies to: 146-146
Source: Coding guidelines
web/packages/agenta-chat/tests/unit/assets/transcriptToMessages.test.ts (1)
45-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting the round trip through
getPendingApprovals.The test verifies that replay emits the
data-approval-manifestpart. It does not verify thatgetPendingApprovalsreads that part back ontoPendingApproval.manifest. The two halves are the actual contract this change introduces. One extra assertion locks both sides against a key rename indata.♻️ Proposed additional assertion
expect(data).toBeDefined() expect(data.toolCallId).toBe("tool-1") expect(data.manifest).toEqual(manifest) + // The dock reads the manifest back off the sibling part — lock that key contract too. + expect(getPendingApprovals(messages ?? [])[0]?.manifest).toEqual(manifest) })This requires importing
getPendingApprovalsfrom the model module.web/oss/src/components/AgentChatSlice/AgentConversation.tsx (1)
591-592: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the stale modal-holder comment.
The Ant Design modal context holder was removed here. The comment still states that themed confirm dialogs mount through this holder, and it now sits above
quickLookHost, which is unrelated. Delete or reword the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: e545c563-bdfe-4f63-b462-72b0a6f6d1ff
📒 Files selected for processing (100)
web/ee/src/pages/_app.tsxweb/oss/src/components/AgentChatSlice/AgentChatPanel.tsxweb/oss/src/components/AgentChatSlice/AgentConversation.tsxweb/oss/src/components/AgentChatSlice/assets/AgentChatTransport.tsweb/oss/src/components/AgentChatSlice/assets/attachmentMedia.tsweb/oss/src/components/AgentChatSlice/assets/attachmentTransport.test.tsweb/oss/src/components/AgentChatSlice/assets/attachmentTransport.tsweb/oss/src/components/AgentChatSlice/assets/attachments.tsweb/oss/src/components/AgentChatSlice/assets/contextBudget.tsweb/oss/src/components/AgentChatSlice/assets/files.test.tsweb/oss/src/components/AgentChatSlice/assets/files.tsweb/oss/src/components/AgentChatSlice/assets/loadSession.tsweb/oss/src/components/AgentChatSlice/assets/markdown.tsxweb/oss/src/components/AgentChatSlice/assets/messageParts.tsweb/oss/src/components/AgentChatSlice/assets/rewind.tsweb/oss/src/components/AgentChatSlice/assets/runError.tsweb/oss/src/components/AgentChatSlice/assets/sessionOpenTarget.test.tsweb/oss/src/components/AgentChatSlice/assets/sessionOpenTarget.tsweb/oss/src/components/AgentChatSlice/assets/toolDisplay.tsweb/oss/src/components/AgentChatSlice/assets/toolFormat.tsweb/oss/src/components/AgentChatSlice/assets/trace.tsweb/oss/src/components/AgentChatSlice/assets/transcriptToMessages.test.tsweb/oss/src/components/AgentChatSlice/assets/transcriptToMessages.tsweb/oss/src/components/AgentChatSlice/components/AgentChatEmptyState.tsxweb/oss/src/components/AgentChatSlice/components/AgentChatSkeleton.tsxweb/oss/src/components/AgentChatSlice/components/AgentComposerDock.tsxweb/oss/src/components/AgentChatSlice/components/AgentMessage.tsxweb/oss/src/components/AgentChatSlice/components/AgentTranscript.tsxweb/oss/src/components/AgentChatSlice/components/AgentTurn.tsxweb/oss/src/components/AgentChatSlice/components/ApprovalDock.tsxweb/oss/src/components/AgentChatSlice/components/AttachmentViewerDrawer.tsxweb/oss/src/components/AgentChatSlice/components/AudioPlayer.tsxweb/oss/src/components/AgentChatSlice/components/ComposerAttachments.tsxweb/oss/src/components/AgentChatSlice/components/ConnectModelBanner.tsxweb/oss/src/components/AgentChatSlice/components/ContextBudgetIndicator.tsxweb/oss/src/components/AgentChatSlice/components/Inspector/EventRow.tsxweb/oss/src/components/AgentChatSlice/components/Inspector/InspectSessionButton.tsxweb/oss/src/components/AgentChatSlice/components/Inspector/Inspector.tsxweb/oss/src/components/AgentChatSlice/components/Inspector/InspectorDrawer.tsxweb/oss/src/components/AgentChatSlice/components/Inspector/LensRail.tsxweb/oss/src/components/AgentChatSlice/components/Inspector/lenses/ContextLens.tsxweb/oss/src/components/AgentChatSlice/components/Inspector/lenses/ResponseLens.tsxweb/oss/src/components/AgentChatSlice/components/Inspector/lenses/TimelineLens.tsxweb/oss/src/components/AgentChatSlice/components/InteractionDock.tsxweb/oss/src/components/AgentChatSlice/components/QueuedMessages.tsxweb/oss/src/components/AgentChatSlice/components/RecordingBar.tsxweb/oss/src/components/AgentChatSlice/components/RightPanel/RightPanelSplit.tsxweb/oss/src/components/AgentChatSlice/components/SessionHistoryMenu.tsxweb/oss/src/components/AgentChatSlice/components/SessionRail.tsxweb/oss/src/components/AgentChatSlice/components/SessionTabLabel.tsxweb/oss/src/components/AgentChatSlice/components/SessionTagBar.tsxweb/oss/src/components/AgentChatSlice/components/ToolActivity.tsxweb/oss/src/components/AgentChatSlice/components/TranscriptPlaceholder.tsxweb/oss/src/components/AgentChatSlice/components/VoiceInputButton.tsxweb/oss/src/components/AgentChatSlice/components/approvals/registry.tsxweb/oss/src/components/AgentChatSlice/components/clientTools/ConnectToolWidget.tsxweb/oss/src/components/AgentChatSlice/components/clientTools/ElicitationWidget.tsxweb/oss/src/components/AgentChatSlice/components/clientTools/UnhandledClientTool.tsxweb/oss/src/components/AgentChatSlice/components/clientTools/index.tsweb/oss/src/components/AgentChatSlice/components/clientTools/meta.tsweb/oss/src/components/AgentChatSlice/components/clientTools/registry.tsxweb/oss/src/components/AgentChatSlice/components/clientTools/types.tsweb/oss/src/components/AgentChatSlice/components/clientTools/useConnectFlow.tsweb/oss/src/components/AgentChatSlice/hooks/useAgentChatQueue.tsweb/oss/src/components/AgentChatSlice/hooks/useAgentChatSession.tsweb/oss/src/components/AgentChatSlice/hooks/useAgentModelKeyStatus.tsweb/oss/src/components/AgentChatSlice/hooks/useAttachmentUploads.test.tsweb/oss/src/components/AgentChatSlice/hooks/useAttachmentUploads.tsweb/oss/src/components/AgentChatSlice/hooks/useComposerAttachments.tsweb/oss/src/components/AgentChatSlice/hooks/useComposerDraft.tsweb/oss/src/components/AgentChatSlice/hooks/useFileActivityDetector.tsweb/oss/src/components/AgentChatSlice/hooks/useFirstRunSeed.tsweb/oss/src/components/AgentChatSlice/hooks/useOnboardingChat.tsweb/oss/src/components/AgentChatSlice/hooks/useOpenAgentSession.tsweb/oss/src/components/AgentChatSlice/hooks/useSessionActions.tsxweb/oss/src/components/AgentChatSlice/hooks/useSessionHydration.tsweb/oss/src/components/AgentChatSlice/hooks/useStartAgentSession.tsweb/oss/src/components/AgentChatSlice/hooks/useTurnInspector.tsweb/oss/src/components/AgentChatSlice/hooks/useVirtuosoTranscript.tsxweb/oss/src/components/AgentChatSlice/state/expandState.tsweb/oss/src/components/AgentChatSlice/state/liveness.tsweb/oss/src/components/AgentChatSlice/state/pendingSessionOpen.tsweb/oss/src/components/AgentChatSlice/state/sessionEphemera.tsweb/oss/src/components/AgentChatSlice/state/sessions.tsweb/oss/src/components/Drives/ContextRail.tsxweb/oss/src/components/Drives/configDrive.tsweb/oss/src/components/Layout/ErrorFallback.tsxweb/oss/src/components/Layout/Layout.tsxweb/oss/src/components/SessionInspector/tabs/StatesTab.tsxweb/oss/src/components/SessionInspector/tabs/StreamsTab.tsxweb/oss/src/hooks/useAlwaysAllowTool.tsxweb/oss/src/pages/_app.tsxweb/oss/src/pages/w/[workspace_id]/p/[project_id]/apps/[app_id]/overview/index.tsxweb/oss/src/styles/auth.cssweb/oss/src/styles/globals.cssweb/packages/agenta-chat/src/assets/transcriptToMessages.tsweb/packages/agenta-chat/src/model/approvals.tsweb/packages/agenta-chat/src/skin/registry.tsweb/packages/agenta-chat/src/skin/types.tsweb/packages/agenta-chat/tests/unit/assets/transcriptToMessages.test.ts
💤 Files with no reviewable changes (28)
- web/oss/src/components/AgentChatSlice/components/AudioPlayer.tsx
- web/oss/src/components/AgentChatSlice/assets/files.test.ts
- web/oss/src/components/AgentChatSlice/components/clientTools/types.ts
- web/oss/src/components/AgentChatSlice/assets/toolFormat.ts
- web/oss/src/components/AgentChatSlice/components/clientTools/index.ts
- web/oss/src/components/AgentChatSlice/hooks/useAgentModelKeyStatus.ts
- web/oss/src/components/AgentChatSlice/state/pendingSessionOpen.ts
- web/oss/src/components/AgentChatSlice/assets/loadSession.ts
- web/oss/src/components/AgentChatSlice/assets/rewind.ts
- web/oss/src/components/AgentChatSlice/assets/messageParts.ts
- web/oss/src/components/AgentChatSlice/state/expandState.ts
- web/oss/src/components/AgentChatSlice/assets/sessionOpenTarget.ts
- web/oss/src/components/AgentChatSlice/assets/AgentChatTransport.ts
- web/oss/src/components/AgentChatSlice/assets/files.ts
- web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.test.ts
- web/oss/src/components/AgentChatSlice/assets/runError.ts
- web/oss/src/components/AgentChatSlice/hooks/useComposerAttachments.ts
- web/oss/src/components/AgentChatSlice/assets/sessionOpenTarget.test.ts
- web/oss/src/components/AgentChatSlice/hooks/useAgentChatQueue.ts
- web/oss/src/components/AgentChatSlice/assets/attachmentTransport.ts
- web/oss/src/components/AgentChatSlice/assets/trace.ts
- web/oss/src/styles/auth.css
- web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.ts
- web/oss/src/components/AgentChatSlice/components/ComposerAttachments.tsx
- web/oss/src/components/AgentChatSlice/assets/attachments.ts
- web/oss/src/components/AgentChatSlice/assets/attachmentTransport.test.ts
- web/oss/src/components/AgentChatSlice/hooks/useAttachmentUploads.ts
- web/oss/src/components/AgentChatSlice/hooks/useAttachmentUploads.test.ts
Comment on lines
+288
to
+290
| ) : TEMPLATE_STRIP_MODE ? null : ( // Strip era: the composer-docked strip replaces the starter pills. | ||
| <div className="flex flex-col items-start gap-1.5"> | ||
| <Text type="secondary" className="!text-xs"> | ||
| Try | ||
| </Text> | ||
| <span className="text-[11px] text-colorTextSecondary">Try</span> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep starter actions when the template strip is absent.
When TEMPLATE_STRIP_MODE is enabled, Line 288 removes BUILD_STARTERS in every build-mode empty state. AgentComposerDock renders the replacement strip only for isFreshAgentRevision and removes it after a commit. A blank session for an existing agent then has no template strip and no starter actions.
Pass explicit strip visibility or eligibility to this component. Render BUILD_STARTERS when the strip is not rendered.
Comment on lines
+97
to
+100
| {/* Hard end-of-conversation clearance: the LAST message always | ||
| has 200px of real content below it, so its action lane can | ||
| never rest against the bottom edge or inside the fade. */} | ||
| <div style={{height: 56, flexShrink: 0}} /> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the end-clearance comment.
The comment states that the last message has 200px below it. This element reserves 56px, and the surrounding pb-24 adds 96px. Update the comment to one short and accurate line.
As per coding guidelines, “Keep in-code comments to at most one short line.”
Source: Coding guidelines
Comment on lines
+22
to
+33
| <SimpleTooltip title={open ? "Hide inspector" : "Inspect session"}> | ||
| <Button | ||
| type={open ? "primary" : "text"} | ||
| size="small" | ||
| icon={<MagnifyingGlass size={14} />} | ||
| variant={open ? "default" : "ghost"} | ||
| size="icon-sm" | ||
| disabled={!sessionId} | ||
| onClick={() => sessionId && toggleSession(sessionId)} | ||
| aria-label="Inspect session" | ||
| aria-pressed={open} | ||
| /> | ||
| </Tooltip> | ||
| > | ||
| <MagnifyingGlass size={14} /> | ||
| </Button> | ||
| </SimpleTooltip> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep an enabled tooltip trigger for the disabled button.
When sessionId is null, Button is disabled. A disabled button cannot trigger SimpleTooltip. Wrap the button in an enabled span, as SessionRail.tsx does for its disabled new-session action.
Proposed fix
<SimpleTooltip title={open ? "Hide inspector" : "Inspect session"}>
+ <span className="inline-flex">
<Button
variant={open ? "default" : "ghost"}
size="icon-sm"
disabled={!sessionId}
onClick={() => sessionId && toggleSession(sessionId)}
aria-label="Inspect session"
aria-pressed={open}
>
<MagnifyingGlass size={14} />
</Button>
+ </span>
</SimpleTooltip>📝 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.
Suggested change
| <SimpleTooltip title={open ? "Hide inspector" : "Inspect session"}> | |
| <Button | |
| type={open ? "primary" : "text"} | |
| size="small" | |
| icon={<MagnifyingGlass size={14} />} | |
| variant={open ? "default" : "ghost"} | |
| size="icon-sm" | |
| disabled={!sessionId} | |
| onClick={() => sessionId && toggleSession(sessionId)} | |
| aria-label="Inspect session" | |
| aria-pressed={open} | |
| /> | |
| </Tooltip> | |
| > | |
| <MagnifyingGlass size={14} /> | |
| </Button> | |
| </SimpleTooltip> | |
| <SimpleTooltip title={open ? "Hide inspector" : "Inspect session"}> | |
| <span className="inline-flex"> | |
| <Button | |
| variant={open ? "default" : "ghost"} | |
| size="icon-sm" | |
| disabled={!sessionId} | |
| onClick={() => sessionId && toggleSession(sessionId)} | |
| aria-label="Inspect session" | |
| aria-pressed={open} | |
| > | |
| <MagnifyingGlass size={14} /> | |
| </Button> | |
| </span> | |
| </SimpleTooltip> |
Comment on lines
+1
to
+5
| /** | ||
| * "Always allow this tool" for the approval card. | ||
| * | ||
| * Config write-through into the draft agent config; `buildAgentRequest` reads the draft, so a grant | ||
| * takes effect on the paused run's resume and every future run, and a commit carries it to triggers. | ||
| * Two fields, routed by tool class: | ||
| * - a gateway / custom-function tool has a `tools[]` entry → per-tool `permission: "allow"` | ||
| * (`specPermission`, the highest-precedence gate). Checked FIRST — it outranks any rule, and a | ||
| * verbatim rule pattern would otherwise also match its slug. | ||
| * - any other harness tool (`bash`, `Terminal`, `Write`, …) has no enforceable per-tool permission | ||
| * → an allow-rule in `harness.permissions.allow`, keyed by the gate name VERBATIM: the runner | ||
| * matches `pattern === gate.toolName`, and that string is exactly what the card shows (the | ||
| * runner stamps it as `resolvedName`, which the egress prefers). Never canonicalize it, except | ||
| * the seven Pi built-ins, which the runner matches case-insensitively (see `gateRulePattern`). | ||
| * Platform ops (`commit_revision`, schedules), client tools, and MCP tools return `eligible: false` | ||
| * and always stay gated (see `gateRulePattern`). | ||
| * | ||
| * On grant we raise a single draft-change signal that the config pane consumes two ways: the section | ||
| * it landed in pulses for attention, and a contained banner (`AlwaysAllowedNotice`) offers Undo — | ||
| * both kept inside the config panel where the change is, rather than a floating toast. `revoke` is | ||
| * the exact inverse (`"ask"` for tools, `allowed:false` for harness rules). | ||
| * Moved to the chat package so mobile shares the identical always-allow behavior; this path | ||
| * survives as a re-export for the app-layer call sites. | ||
| */ | ||
| export function useAlwaysAllowTool(entityId?: string) { | ||
| const config = useAtomValue( | ||
| useMemo(() => workflowMolecule.selectors.configuration(entityId ?? ""), [entityId]), | ||
| ) | ||
| // Latest config for the deferred Undo click, so it never reverts against a stale snapshot. | ||
| const configRef = useRef(config) | ||
| configRef.current = config | ||
| const setConfiguration = useSetAtom(workflowMolecule.actions.updateConfiguration) | ||
| // Marks the config section this grant lands in so it can pulse for attention — the user | ||
| // acted here in the dock, but the write shows up over in the (maybe off-screen) config pane. | ||
| const raiseDraftSignal = useSetAtom(draftConfigChangeSignalAtom) | ||
|
|
||
| const infoFor = useCallback( | ||
| (toolName: string): ToolGrantInfo => { | ||
| if (!entityId) return INELIGIBLE | ||
| // Gateway / custom-function tools carry a per-tool `permission` in `tools[]`. First: | ||
| // it outranks a rule, and a verbatim rule pattern would also match its slug. | ||
| const tool = findGrantableTool(config, toolName) | ||
| if (tool) return {eligible: true, alreadyAllowed: tool.permission === "allow"} | ||
| // Any other harness tool (bash, Terminal, Write, …) → `harness.permissions.allow`. | ||
| const harnessTool = findGrantableHarnessTool(config, toolName) | ||
| if (harnessTool) return {eligible: true, alreadyAllowed: harnessTool.allowed} | ||
| // Platform ops (commit_revision, schedules), client tools, MCP → never grantable. | ||
| return INELIGIBLE | ||
| }, | ||
| [entityId, config], | ||
| ) | ||
|
|
||
| // Inverse of grant: put the tool back to gated. Reads the LATEST config (ref), since Undo fires | ||
| // seconds after the grant and the draft may have moved on. | ||
| const revoke = useCallback( | ||
| (toolName: string): boolean => { | ||
| if (!entityId) return false | ||
| const cfg = configRef.current | ||
| // Same routing as `grant` — `tools[]` first, then the harness allow-rule. | ||
| const tool = findGrantableTool(cfg, toolName) | ||
| const pattern = tool ? null : gateRulePattern(toolName) | ||
| const next = tool | ||
| ? withToolPermission(cfg, toolName, "ask") | ||
| : pattern | ||
| ? withHarnessToolAllow(cfg, pattern, false) | ||
| : null | ||
| if (!next) return false | ||
| setConfiguration(entityId, next) | ||
| return true | ||
| }, | ||
| [entityId, setConfiguration], | ||
| ) | ||
|
|
||
| const grant = useCallback( | ||
| (toolName: string): boolean => { | ||
| if (!entityId) return false | ||
| // Route to the field that matches the gate's tool class (see infoFor). `tools[]` first: | ||
| // its per-tool permission outranks a rule, and a verbatim pattern would match its slug. | ||
| const tool = findGrantableTool(config, toolName) | ||
| const pattern = tool ? null : gateRulePattern(toolName) | ||
| const next = tool | ||
| ? withToolPermission(config, toolName, "allow") | ||
| : pattern | ||
| ? withHarnessToolAllow(config, pattern, true) | ||
| : null | ||
| if (!next) return false | ||
| setConfiguration(entityId, next) | ||
| // A harness allow-rule writes `harness.permissions`, which surfaces in the Advanced → | ||
| // Permissions group (and classifies as an "advanced" draft change); gateway/custom-function | ||
| // tools write `tools[]`, surfaced in the Tools section. Pulse the section the change lands in. | ||
| raiseDraftSignal({ | ||
| revisionId: entityId, | ||
| sectionKeys: [tool ? "tools" : "advanced"], | ||
| origin: "approval-dock", | ||
| summary: `Always allow ${toolName}`, | ||
| // Friendly display (matches the approval card) — a gateway tool's raw name is a slug. | ||
| label: resolveToolDisplay(toolName).label, | ||
| toolName, | ||
| at: Date.now(), | ||
| }) | ||
| return true | ||
| }, | ||
| [entityId, config, setConfiguration, raiseDraftSignal], | ||
| ) | ||
|
|
||
| return {infoFor, grant, revoke} | ||
| } | ||
| export {useAlwaysAllowTool, type ToolGrantInfo} from "@agenta/chat/hooks" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Shorten the compatibility comment.
Keep this comment to one short line. The re-export does not need a multi-line migration explanation.
Proposed change
-/**
- * Moved to the chat package so mobile shares the identical always-allow behavior; this path
- * survives as a re-export for the app-layer call sites.
- */
+/** Compatibility re-export for app-layer call sites. */As per coding guidelines, keep in-code comments to at most one short line unless they document a surprising constraint.
📝 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.
Suggested change
| /** | |
| * "Always allow this tool" for the approval card. | |
| * | |
| * Config write-through into the draft agent config; `buildAgentRequest` reads the draft, so a grant | |
| * takes effect on the paused run's resume and every future run, and a commit carries it to triggers. | |
| * Two fields, routed by tool class: | |
| * - a gateway / custom-function tool has a `tools[]` entry → per-tool `permission: "allow"` | |
| * (`specPermission`, the highest-precedence gate). Checked FIRST — it outranks any rule, and a | |
| * verbatim rule pattern would otherwise also match its slug. | |
| * - any other harness tool (`bash`, `Terminal`, `Write`, …) has no enforceable per-tool permission | |
| * → an allow-rule in `harness.permissions.allow`, keyed by the gate name VERBATIM: the runner | |
| * matches `pattern === gate.toolName`, and that string is exactly what the card shows (the | |
| * runner stamps it as `resolvedName`, which the egress prefers). Never canonicalize it, except | |
| * the seven Pi built-ins, which the runner matches case-insensitively (see `gateRulePattern`). | |
| * Platform ops (`commit_revision`, schedules), client tools, and MCP tools return `eligible: false` | |
| * and always stay gated (see `gateRulePattern`). | |
| * | |
| * On grant we raise a single draft-change signal that the config pane consumes two ways: the section | |
| * it landed in pulses for attention, and a contained banner (`AlwaysAllowedNotice`) offers Undo — | |
| * both kept inside the config panel where the change is, rather than a floating toast. `revoke` is | |
| * the exact inverse (`"ask"` for tools, `allowed:false` for harness rules). | |
| * Moved to the chat package so mobile shares the identical always-allow behavior; this path | |
| * survives as a re-export for the app-layer call sites. | |
| */ | |
| export function useAlwaysAllowTool(entityId?: string) { | |
| const config = useAtomValue( | |
| useMemo(() => workflowMolecule.selectors.configuration(entityId ?? ""), [entityId]), | |
| ) | |
| // Latest config for the deferred Undo click, so it never reverts against a stale snapshot. | |
| const configRef = useRef(config) | |
| configRef.current = config | |
| const setConfiguration = useSetAtom(workflowMolecule.actions.updateConfiguration) | |
| // Marks the config section this grant lands in so it can pulse for attention — the user | |
| // acted here in the dock, but the write shows up over in the (maybe off-screen) config pane. | |
| const raiseDraftSignal = useSetAtom(draftConfigChangeSignalAtom) | |
| const infoFor = useCallback( | |
| (toolName: string): ToolGrantInfo => { | |
| if (!entityId) return INELIGIBLE | |
| // Gateway / custom-function tools carry a per-tool `permission` in `tools[]`. First: | |
| // it outranks a rule, and a verbatim rule pattern would also match its slug. | |
| const tool = findGrantableTool(config, toolName) | |
| if (tool) return {eligible: true, alreadyAllowed: tool.permission === "allow"} | |
| // Any other harness tool (bash, Terminal, Write, …) → `harness.permissions.allow`. | |
| const harnessTool = findGrantableHarnessTool(config, toolName) | |
| if (harnessTool) return {eligible: true, alreadyAllowed: harnessTool.allowed} | |
| // Platform ops (commit_revision, schedules), client tools, MCP → never grantable. | |
| return INELIGIBLE | |
| }, | |
| [entityId, config], | |
| ) | |
| // Inverse of grant: put the tool back to gated. Reads the LATEST config (ref), since Undo fires | |
| // seconds after the grant and the draft may have moved on. | |
| const revoke = useCallback( | |
| (toolName: string): boolean => { | |
| if (!entityId) return false | |
| const cfg = configRef.current | |
| // Same routing as `grant` — `tools[]` first, then the harness allow-rule. | |
| const tool = findGrantableTool(cfg, toolName) | |
| const pattern = tool ? null : gateRulePattern(toolName) | |
| const next = tool | |
| ? withToolPermission(cfg, toolName, "ask") | |
| : pattern | |
| ? withHarnessToolAllow(cfg, pattern, false) | |
| : null | |
| if (!next) return false | |
| setConfiguration(entityId, next) | |
| return true | |
| }, | |
| [entityId, setConfiguration], | |
| ) | |
| const grant = useCallback( | |
| (toolName: string): boolean => { | |
| if (!entityId) return false | |
| // Route to the field that matches the gate's tool class (see infoFor). `tools[]` first: | |
| // its per-tool permission outranks a rule, and a verbatim pattern would match its slug. | |
| const tool = findGrantableTool(config, toolName) | |
| const pattern = tool ? null : gateRulePattern(toolName) | |
| const next = tool | |
| ? withToolPermission(config, toolName, "allow") | |
| : pattern | |
| ? withHarnessToolAllow(config, pattern, true) | |
| : null | |
| if (!next) return false | |
| setConfiguration(entityId, next) | |
| // A harness allow-rule writes `harness.permissions`, which surfaces in the Advanced → | |
| // Permissions group (and classifies as an "advanced" draft change); gateway/custom-function | |
| // tools write `tools[]`, surfaced in the Tools section. Pulse the section the change lands in. | |
| raiseDraftSignal({ | |
| revisionId: entityId, | |
| sectionKeys: [tool ? "tools" : "advanced"], | |
| origin: "approval-dock", | |
| summary: `Always allow ${toolName}`, | |
| // Friendly display (matches the approval card) — a gateway tool's raw name is a slug. | |
| label: resolveToolDisplay(toolName).label, | |
| toolName, | |
| at: Date.now(), | |
| }) | |
| return true | |
| }, | |
| [entityId, config, setConfiguration, raiseDraftSignal], | |
| ) | |
| return {infoFor, grant, revoke} | |
| } | |
| export {useAlwaysAllowTool, type ToolGrantInfo} from "@agenta/chat/hooks" | |
| /** Compatibility re-export for app-layer call sites. */ | |
| export {useAlwaysAllowTool, type ToolGrantInfo} from "`@agenta/chat/hooks`" |
Source: Coding guidelines
The composer-docked template strip and the empty state's starter pills each re-derived their own visibility, and the two conditions did not agree: the empty state dropped the pills for the whole of TEMPLATE_STRIP_MODE, while the strip also required a fresh agent revision. A blank session on an existing agent (version >= 2), and every session while the revision query is pending, therefore rendered neither — an empty state with nothing to click. AgentConversation now owns the one flag and hands it to both surfaces, so exactly one of the strip and the pills is up. Also: the disabled "Inspect session" button gets the enabled-span tooltip trigger the session rail already uses (a disabled button fires no pointer events, so its tooltip never opened), the transcript's end-of-conversation spacer comment states the real numbers, and the always-allow re-export loses its three-line preamble.
ardaerzin
force-pushed
the
pkg/chat-engine
branch
from
August 10, 2026 14:58
9a75085 to
954118f
Compare
ardaerzin
force-pushed
the
oss/chat-on-shared-engine
branch
from
August 10, 2026 14:58
92cc9cf to
04e5416
Compare
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.
The OSS chat slice stops owning a chat runtime and composes
@agenta/chatinstead. That alsoremoves its antd and antd-x dependency — the slice renders on the shared, antd-free components.
Largest diff in the lower half of the stack (100 files) but almost all of it is deletion and
re-pointing: the logic moved in the lane below, this lane just stops duplicating it.
Not run in a browser — static gates only (
pnpm lint-fix24/24,tsc --noEmitcleanfor
@agenta/shared,ui,entities,entity-ui,settings-ui,oss,ee,mobile).Stacked on
pkg/chat-engine; review only this lane's diff.