[5528] test(frontend): run web/oss unit tests in CI - #5567
Conversation
The 13+ unit test files colocated under web/oss/src imported from vitest but never executed: web/oss had no vitest config, no vitest dependency, and no test:unit script, so the recursive runner (pnpm -r --if-present run test:unit) skipped the package silently and the CI job still passed. Add a vitest config (colocated src/**/*.test glob, tsconfig path aliases, jsdom env, junit reporter), the vitest/coverage/jsdom dev deps, and the test scripts so the suite runs in the existing unit layer and a failing assertion fails the build. Use @vitejs/plugin-react-swc to transform JSX, since tsconfig's jsx: preserve otherwise breaks vite's parser when a test transitively imports a .tsx. Publish web/oss/test-results/junit.xml from the workflow alongside the per-package reports. All 14 files (98 tests) run and pass.
dosubot
Bot
added
the
size:XL
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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: Organization UI 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
WalkthroughThis release updates agent skills, API and mount handling, runner reliability, Agent Chat uploads and voice interactions, onboarding navigation, settings, shared entity contracts, table imports, gateway-trigger flows, tests, and package metadata. ChangesAgent skill documentation
Backend and runner behavior
Agent Chat and drives
Navigation and settings
Shared contracts and UI migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
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/hooks/usePostAuthRedirect.ts (1)
142-154: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInitialize simplified navigation before the invitation return.
Invited first-time users return at lines 133–136 before either assignment here, leaving their per-user default
false. Set the default immediately insideif (isNewUser)so every new signup receives the intended navigation baseline.Proposed fix
if (isNewUser) { + setNavSimplifiedDefault(true) if (isInvitedUser) { ... - setNavSimplifiedDefault(true) await router.push("/post-signup") ... - setNavSimplifiedDefault(true) } }web/packages/agenta-entity-ui/src/gatewayTrigger/drawers/shared/RunVersionField.tsx (1)
171-210: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExisting environment-bound schedules/subscriptions lose their version picker UI once "Deployed" is hidden. The shared root cause is in
RunVersionField.tsx:hideEnvironmentforces the revision-picker branch and removes the "Deployed" rail option unconditionally, but both drawers can still prefillbindMode = "environment"for pre-existing environment-bound entities, whose header summary continues to display "env: X" while the expanded section now shows an unrelated, unbound "Pinned" picker.
web/packages/agenta-entity-ui/src/gatewayTrigger/drawers/shared/RunVersionField.tsx#L171-L210: branch the rendered content on the actualbindMode(e.g. still show a read-only/environment summary whenbindMode === "environment") instead of lettinghideEnvironmentunconditionally force the revision branch.web/packages/agenta-entity-ui/src/gatewayTrigger/drawers/TriggerScheduleDrawer.tsx#L848-L863: once the component supports it, pass through enough context (or a read-only environment view) so an environment-bound schedule remains visible/explainable here.web/packages/agenta-entity-ui/src/gatewayTrigger/drawers/TriggerSubscriptionDrawer.tsx#L1047-L1061: same fix needed for the subscription drawer's identical call site.
🟠 Major comments (20)
docs/design/simplify-nav-new-users/plan.md-148-152 (1)
148-152: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse a genuinely user-scoped override key.
Line 149 uses the literal
"<userId>"; if implemented as written, every account in the same browser shares the Classic-mode preference. Reuse the Phase 1 scoped storage family andcreateScopedStorageKeypattern..agents/skills/add-announcement/SKILL.md-1-5 (1)
1-5: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAlign the tool allowlist with the documented workflow.
The skill allows only
Read,Edit,Grep, andGlob, but later instructs the agent to runcat,code,git add, andgit commit. Add the required tools, or explicitly mark those commands as user-run instructions; otherwise the skill cannot perform its own documented workflow..agents/skills/add-announcement/SKILL.md-200-204 (1)
200-204: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse the canonical changelog entry path.
These instructions create entries under
docs/docs/changelog, but.agents/skills/create-changelog-announcement/SKILL.mddefinesdocs/blog/entries/[feature-slug].mdxas the source consumed by the changelog index. Following this skill can create a page the changelog build does not discover..agents/skills/add-announcement/SKILL.md-219-223 (1)
219-223: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse GitButler commands instead of raw Git commands.
This workflow teaches
git addandgit commit, which bypasses the repository’s lane assignment and stack safeguards. Replace this with the documentedbut rubandbut commit <lane> --onlyflow, or clearly scope the example to non-GitButler repositories.Per coding guidelines, when GitButler workspace mode is active, use
butand verify lane scope before committing.Source: Coding guidelines
.agents/skills/create-changelog-announcement/SKILL.md-321-321 (1)
321-321: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAvoid direct peer-skill instruction access.
This line tells the skill to read another skill’s
SKILL.md, exposing prompt instructions and capabilities across skills. Inline the needed social-announcement requirements or pass sanitized guidance through an orchestrator instead.Source: Linters/SAST tools
.agents/skills/create-changelog-announcement/SKILL.md-372-384 (1)
372-384: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake the example follow the required Summary/truncate contract.
The example entry starts with front matter and the full write-up but omits
<Summary>and{/* truncate */}, even though the required structure is documented earlier. Users copying this example can produce entries without the intended index preview or page split..agents/skills/implement-feature/SKILL.md-190-191 (1)
190-191: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winResolve the contradictory push policy.
Phase 6 says to stop unless the user asks to push, but the parallel-agent section later tells agents to push each lane when a slice is done. Choose one policy, preferably explicit user approval, and apply it consistently to avoid publishing shared-workspace changes unexpectedly.
.agents/skills/plan-feature/SKILL.md-1-3 (1)
1-3: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
plan-featureas the skill nameThis skill is referenced by directory and documentation as
plan-feature, but.agents/skills/plan-feature/SKILL.mddeclaresname: planner-feature. Keep the frontmatternameconsistent with the directory to avoid ambiguity and inconsistent tooling resolution..agents/skills/update-llm-model-list/SKILL.md-37-70 (1)
37-70: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin LiteLLM for every audit command.
Both PEP 723 scripts declare unpinned
dependencies = ["litellm"], and theuvx --with litellminvocations resolve the latest release at runtime. Pin LiteLLM in the script metadata and in eachuvxcommand, updating it deliberately when regenerating the model list.Also applies to: 86-138, 174-180
Source: Linters/SAST tools
.agents/skills/update-llm-model-list/SKILL.md-50-53 (1)
50-53: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winScope the slash-fallback to Anthropic and Cohere.
exists()currently strips the first path component for anyprovider/modelentry, sounsupported/modelcan pass whenevermodelexists in LiteLLM. Restrict fallback matching to the documentedanthropic/andcohere/prefixes (lines 23-26)..agents/skills/update-llm-model-list/SKILL.md-45-46 (1)
45-46: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the “no local install” command import the local SDK.
uvx --with litellmonly installslitellmin the isolated tool environment; it does not make the repository’sagentapackage importable. The script at.agents/skills/update-llm-model-list/SKILL.md:46importsagenta.sdk.utils.assets, so the Step 1 and Step 4 commands fail in a fresh clone unless the SDK path is passed withuvx --with .or documented/installed viaPYTHONPATH.web/oss/src/components/AgentChatSlice/components/ComposerAttachments.tsx-124-140 (1)
124-140: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winClickable tiles aren't keyboard-reachable.
Chipand the image tile both userole="button"+onClickon adivwith notabIndexor key handler, so opening an attachment in the viewer is mouse-only. AddtabIndex={0}and an Enter/Space handler whenonClick/onViewis present (or render a real<button>).As per coding guidelines, "Implement light and dark appearance and interaction states for every added or changed UI element" and accessibility blockers that prevent task completion must be fixed.
♿ Proposed fix for `Chip`
<div role={onClick ? "button" : undefined} + tabIndex={onClick ? 0 : undefined} onClick={onClick} + onKeyDown={ + onClick + ? (e) => { + if (e.key === "Enter" || e.key === " ") { + e.preventDefault() + onClick() + } + } + : undefined + } className={...} >Also applies to: 339-346
Source: Coding guidelines
web/oss/src/components/AgentChatSlice/components/ComposerAttachments.tsx-185-196 (1)
185-196: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winObject-URL lifecycle keyed on array identity revokes URLs that are still in use. Both effects depend on the whole upload array, so any new array reference — including an upload
status/percenttick from the (planned) upload flow — revokes and re-creates every URL. In the composer this swapsAudioPlayer'ssrcand restarts playback mid-clip; in the drawer it revokes the blob the viewer is currently rendering. Key the effect on the file identities instead.
web/oss/src/components/AgentChatSlice/components/ComposerAttachments.tsx#L185-L196: derive a stable key from the uids (or cache URLs peroriginFileObjand revoke only removed entries) instead of depending onfiles.web/oss/src/components/AgentChatSlice/components/AttachmentViewerDrawer.tsx#L97-L105: rebuildnodeson the uid set rather than theuploadsreference.hosting/railway/oss/scripts/configure.sh-79-82 (1)
79-82: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSilently accepts an empty RSA key if
openssl genpkeyfails.Line 82 discards stderr inside a
${VAR:-$(...)}substitution, so a genpkey failure (missing/old openssl, no entropy) leavesAGENTA_STORE_JWT_PRIVATE_KEYempty. That empty value is then pushed to theapiservice at line 370 and the deploy looks successful, but store web-identity signing fails at runtime with no hint from this script. Worth failing fast here.🛡️ Proposed fix to validate the generated key
AGENTA_STORE_ACCESS_KEY="${AGENTA_STORE_ACCESS_KEY:-$(openssl rand -hex 10)}" AGENTA_STORE_SECRET_KEY="${AGENTA_STORE_SECRET_KEY:-$(openssl rand -hex 32)}" AGENTA_STORE_SIGNING_KEY="${AGENTA_STORE_SIGNING_KEY:-$(openssl rand -base64 32)}" - AGENTA_STORE_JWT_PRIVATE_KEY="${AGENTA_STORE_JWT_PRIVATE_KEY:-$(openssl genpkey -algorithm RSA -pkeyopt rsa_keygen_bits:2048 2>/dev/null)}" + if [ -z "$AGENTA_STORE_JWT_PRIVATE_KEY" ]; then + AGENTA_STORE_JWT_PRIVATE_KEY="$(openssl genpkey -algorithm RSA -pkeyopt rsa_keygen_bits:2048)" + fi + if [ -z "$AGENTA_STORE_ACCESS_KEY" ] || [ -z "$AGENTA_STORE_SECRET_KEY" ] || + [ -z "$AGENTA_STORE_SIGNING_KEY" ] || [ -z "$AGENTA_STORE_JWT_PRIVATE_KEY" ]; then + printf "Failed to resolve or generate the mount store credentials.\n" >&2 + return 1 + fiweb/oss/src/components/AgentChatSlice/components/clientTools/useConnectFlow.ts-113-129 (1)
113-129: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
finishagainstmeta.settledbefore callingsettle().
finishis the shared terminal-settle path for success, poll-abandon, timeout, and error failures.cancel/declinealready check!settledRef.current && !meta.settled, butfinishonly checks its ownsettledRef, so two mounted instances for the same parked call can race: another instance’s settle can flipmeta.settledbefore this instance’s async terminal callback returns, then this callback still callssettle()again.🔒️ Proposed fix
const finish = useCallback( (result: ConnectOutput | {errorText: string}) => { - if (settledRef.current) return + if (settledRef.current || meta.settled) return settledRef.current = true teardown() setPhase("idle") if ("errorText" in result) { setOutcome({connected: false}) settle({errorText: result.errorText}) } else { setOutcome({connected: result.connected === true}) settle({output: result as Record<string, unknown>}) } }, - [settle, teardown], + [settle, teardown, meta.settled], )web/oss/src/components/AgentChatSlice/hooks/useAttachmentUploads.ts-27-76 (1)
27-76: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
enqueuesilently no-ops for files added in the same tick, once a transport is wired.
filesRef.current(Line 33-34) only reflectsfilesas of the hook's last render. The documented/expected caller pattern (seeAgentConversation.tsxlines ~1738-1745) callssetFiles(...)and thenuploads.enqueue(...)synchronously in the same event handler —filesRef.currenthasn't caught up yet, sorun(Line 47-50) can't find the newly-addedoriginFileObjand returns early (Line 50) without starting the upload. This is currently masked byif (!upload) return(Line 46), but will silently break the very first attachment upload once a realuploadtransport is wired — the exact scenario the docstring (Line 10-13) promises "just works."Consider having
enqueueaccept the file payload directly instead of re-deriving it from a possibly-stale ref, e.g.:🐛 Proposed fix direction
export interface AttachmentUploads { - enqueue: (uids: string[]) => void + enqueue: (files: {uid: string; file: File}[]) => void retry: (uid: string) => void } @@ const run = useCallback( - (uid: string) => { + (uid: string, fileArg?: File) => { if (!upload) return - const file = filesRef.current.find((f) => f.uid === uid)?.originFileObj as - | File - | undefined + const file = + fileArg ?? + (filesRef.current.find((f) => f.uid === uid)?.originFileObj as File | undefined) if (!file) return ... }, [upload, patch], ) - const enqueue = useCallback((uids: string[]) => uids.forEach(run), [run]) + const enqueue = useCallback( + (files: {uid: string; file: File}[]) => files.forEach(({uid, file}) => run(uid, file)), + [run], + )The caller in
AgentConversation.tsxwould then passaccepted.map((f) => ({uid: ..., file: f}))instead of just uids.web/packages/agenta-ui/src/RichChatInput/ComposerSendButton.tsx-37-37 (1)
37-37: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReplace raw hex colors with theme tokens.
Line 37 hard-codes
#191a0dand#b8cb3f, which can render incorrectly across themes and violates the repository’s theme-color rule.Suggested direction
- : "!border-[var(--ag-surface-accent)] !bg-[var(--ag-surface-accent)] !text-[`#191a0d`] hover:!border-[`#b8cb3f`] hover:!bg-[`#b8cb3f`]" + : "use semantic Ant/Tailwind theme tokens for text, border, background, and hover states"As per coding guidelines, consume theme colors through semantic tokens, Tailwind utilities, or supported
var(--ag-color*)variables; raw hex colors are not allowed.Source: Coding guidelines
web/packages/agenta-ui/src/RichChatInput/RichChatInput.tsx-297-297 (1)
297-297: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDisable submission while dictation is active.
Line 297 locks the editor, but the
SendButtonremains enabled unlessdisabledis set. A user can submit during an active transcript, sending incomplete text while the dictation session continues updating the editor.Suggested fix
<SendButton onSubmit={onSubmit} forceEnabled={sendForceEnabled} - disabled={disabled} + disabled={disabled || dictating} streaming={streaming} onStop={onStop} />web/oss/src/components/Drives/driveMedia.ts-167-191 (1)
167-191: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftMove this upload behind the Fern resource accessor.
This direct Axios call violates the frontend API rule requiring per-resource accessors from
@agenta/sdk/resources. Add or extend the mount-file accessor inweb/packages/agenta-sdk/src/resources.ts, including any needed progress-capable transport support, rather than exposing a raw API call here.Source: Coding guidelines
web/oss/src/components/Drives/useMountUpload.ts-124-126 (1)
124-126: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNon-unique item
idcan collide across rapid upload batches.
`${Date.now()}-${i}-${file.name}`can collide whenupload()is invoked twice in the same millisecond (e.g., two drop targets firing synchronously) with overlapping indexiand identical file name. A collision overwrites the earlier item's entry insources.current/controllers.current, causing retry/dismiss/abort to target the wrong file and duplicate React keys initems.🐛 Proposed fix using a guaranteed-unique id
- const id = `${Date.now()}-${i}-${file.name}` + const id = crypto.randomUUID()
🟡 Minor comments (14)
docs/design/simplify-nav-new-users/plan.md-168-175 (1)
168-175: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winResolve the OSS/EE scope contradiction.
Phase 2 requires availability in both OSS and EE, but the file list and Line 198 explicitly prohibit EE changes. Either add the EE registration/files or revise the requirement to OSS-only.
Also applies to: 191-198
docs/design/simplify-nav-new-users/status.md-7-10 (1)
7-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRefresh this status note after enabling the OSS Vitest runner.
The file says both phases are implemented, then calls the work “Ready to build” and claims there is no CI-wired OSS Vitest runner. The PR objectives state that
web/ossnow has Vitest/test:unit scripts and CI JUnit collection. Update Lines 53-65 to say implementation is complete with manual QA pending, and remove or rewrite the stale Slice 0 follow-up.Also applies to: 53-65
docs/design/simplify-nav-new-users/context.md-89-90 (1)
89-90: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFormat the Phase 2 criteria as a real list.
**Phase 2** 6.keeps the heading and all criteria in one paragraph, so Markdown will not render the numbered list correctly. Also use the compound adjectivefull-page.Proposed fix
-**Phase 2** 6. The Settings → Feature flags switch flips the effective mode and survives a -reload. 7. Toggling the switch updates the sidebar without a full page reload. 8. Phase 1's -sidebar behavior is unchanged when no override is set. +**Phase 2** + +6. The Settings → Feature flags switch flips the effective mode and survives a reload. +7. Toggling the switch updates the sidebar without a full-page reload. +8. Phase 1's sidebar behavior is unchanged when no override is set.Source: Linters/SAST tools
.agents/skills/create-changelog-announcement/SKILL.md-275-285 (1)
275-285: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the sidebar example valid JSON.
The
// ... existing entriesline makes this block invalid JSON, despite the workflow requiring valid JSON. Put the ellipsis outside the fenced block or label the example as JSONC..agents/skills/update-api-docs/SKILL.md-68-68 (1)
68-68: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the verification step number.
This is the third documented step, not “Step 5,” which makes the workflow appear incomplete.
.agents/skills/update-api-docs/SKILL.md-83-86 (1)
83-86: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd language tags to prose example fences.
.agents/skills/update-api-docs/SKILL.md#L83-L86: mark the commit-message block astext..agents/skills/write-issue/SKILL.md#L67-L70: mark the checklist example astext..agents/skills/write-social-announcement/SKILL.md#L163-L218: mark each social-post example astext.Source: Linters/SAST tools
web/oss/src/components/Drives/useDriveDrop.ts-81-87 (1)
81-87: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear the spring-load timer during effect cleanup.
Unmounting or disabling the hook while a folder timer is pending still calls
onNavigate()after 700 ms. CallclearSpring()in this cleanup before removing listeners.web/oss/src/components/Drives/useDriveDrop.ts-103-152 (1)
103-152: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake disabled drop handlers inert.
enabled={false}removes window listeners, but these handlers still highlight folders, suppress browser defaults, spring-navigate, and call the no-op upload callback. Return early from each handler when disabled.web/oss/src/components/AgentChatSlice/assets/attachments.ts-83-91 (1)
83-91: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTwo-item list renders as "Images, and audio".
nouns.slice(0, -1).join(", ")leaves an empty separator for a single leading item, so the Oxford comma is emitted with only two kinds — reachable as soon askindsis narrowed by capability gating (the documented seam).🐛 Proposed fix
const sentence = nouns.length === 1 ? nouns[0] - : `${nouns.slice(0, -1).join(", ")}, and ${nouns[nouns.length - 1]}` + : nouns.length === 2 + ? `${nouns[0]} and ${nouns[1]}` + : `${nouns.slice(0, -1).join(", ")}, and ${nouns[nouns.length - 1]}` return sentence.charAt(0).toUpperCase() + sentence.slice(1)web/oss/src/components/Drives/DriveExplorer.tsx-1834-1852 (1)
1834-1852: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winA drop into an unresolvable folder is silently discarded.
When
drive.resolveMount(folder)returns nothing,uploadIntoFolderreturns without starting an upload, staging the file, or telling the user — the tile just never appears and the picked/dropped files are lost.messagefromApp.useApp()is already in scope (line 1545); surface a failure there so the intent isn't dropped on the floor.🛡️ Proposed fix
const uploadIntoFolder = useCallback( (picked: File[], folder: string) => { const resolved = drive.resolveMount(folder) - if (!resolved) return + if (!resolved) { + message.error("Couldn't resolve a destination for these files.") + return + } mountUpload.upload(picked, {and add
messageto the dependency array.web/oss/src/components/AgentChatSlice/components/MicPermissionNotice.tsx-23-25 (1)
23-25: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not update refs during render.
MicPermissionNoticewritesmessageRef.currentinside the render function and then reads it at line 32, which React discourages/forbids because pending renders can overwrite the ref without committing. Keep the fallback text in state and rendermessage ?? latchedMessageinstead.Proposed fix
-import {useRef} from "react" +import {useEffect, useState} from "react" ... - const messageRef = useRef("") - if (message) messageRef.current = message + const [latchedMessage, setLatchedMessage] = useState(message ?? "") + + useEffect(() => { + if (message) setLatchedMessage(message) + }, [message]) ... - <span className="truncate">{messageRef.current}</span> + <span className="truncate">{message ?? latchedMessage}</span>web/oss/src/components/pages/settings/Organization/General.tsx-232-233 (1)
232-233: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winHardcoded gray palette utilities won't adapt to dark mode.
text-gray-400,bg-gray-100, andborder-gray-100are fixed light-theme values; use Ant Design semantic tokens orvar(--ag-color*)-backed utilities so the option rows render correctly in both themes.As per coding guidelines: "Consume theme colors through Ant Design semantic tokens, Tailwind color utilities, or supported
var(--ag-color*)variables" and "Implement light and dark appearance and interaction states for every added or changed UI element, and verify both themes."Also applies to: 247-247
Source: Coding guidelines
web/oss/src/components/AgentChatSlice/hooks/useAudioRecorder.ts-197-215 (1)
197-215: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStale
micPermissionState()callback can clobber a newer session's state.The fire-and-forget
micPermissionState().then(...)(Line 212-214) isn't guarded against a newerstart()/dismissError()happening in between. If the user retries quickly after a denial, this stale callback can still firesetError(DISMISSED_MESSAGE)after recording has already started (or after the error was dismissed), showing a stale mic-error message over an unrelated/successful state.🐛 Proposed fix
setStatus("denied") setError(BLOCKED_MESSAGE) - micPermissionState().then((state) => { - if (state === "prompt") setError(DISMISSED_MESSAGE) - }) + micPermissionState().then((state) => { + // Only apply if nothing superseded this attempt in the meantime. + if (state === "prompt" && recRef.current === null && !cancelledRef.current) { + setError((prev) => (prev === BLOCKED_MESSAGE ? DISMISSED_MESSAGE : prev)) + } + })web/oss/src/components/Drives/driveFileSource.tsx-72-85 (1)
72-85: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKey text state to the current local file.
Switching local files can render the previous file’s text with
isPending: falseuntil the new read resolves. Store the sourceFilealongside the text, and returnundefined/pending when it differs.Proposed fix
- const [text, setText] = useState<string | undefined>(undefined) + const [text, setText] = useState<{file: File | null; value: string | undefined}>({ + file: null, + value: undefined, + }) useEffect(() => { if (!local) return let cancelled = false local.file .text() - .then((t) => !cancelled && setText(t)) - .catch(() => !cancelled && setText("")) + .then((value) => !cancelled && setText({file: local.file, value})) + .catch(() => !cancelled && setText({file: local.file, value: ""})) @@ - if (local) return {data: text, isPending: text === undefined} + if (local) { + const data = text.file === local.file ? text.value : undefined + return {data, isPending: data === undefined} + }
🧹 Nitpick comments (16)
web/oss/src/components/TestsetsTable/TestsetsTable.tsx (1)
367-368: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftDo not bypass the table-store contract with
as never.The cast only silences the generic mismatch at the
@agenta/ui/tableboundary. Use a correctly typed shared store or an explicit adapter so incompatible pagination/row contracts remain compile-time errors.web/oss/src/components/Drives/useDriveDrop.ts (1)
3-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten the new behavioral comments.
These multi-line comments describe normal hook behavior rather than a surprising constraint. Keep them to short one-line summaries.
As per coding guidelines, “Keep in-code comments to at most one short line; use longer comments only for genuinely surprising constraints such as bugs, races, or ordering requirements.”
Also applies to: 163-169
Source: Coding guidelines
web/oss/src/components/AgentChatSlice/AgentConversation.tsx (2)
1738-1749: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the upload uid from
toUploadFileinstead of re-implementing it.
addFilesrecomputes${f.name}-${f.lastModified}-${f.size}independently oftoUploadFile(line 1723); any change to the uid scheme silently desyncs the enqueue keys from the tray entries. Two identical drops also collide on the same uid, soremoveFilewould drop both rows.♻️ Reuse the mapped entries
if (accepted.length) { - setFiles((prev) => [...prev, ...accepted.map(toUploadFile)]) - uploads.enqueue(accepted.map((f) => `${f.name}-${f.lastModified}-${f.size}`)) + const mapped = accepted.map(toUploadFile) + setFiles((prev) => [...prev, ...mapped]) + uploads.enqueue(mapped.map((f) => f.uid)) }
1855-1864: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
!attachmentsSettledreturns silently — the send is swallowed with no feedback.Inert today (no uploader is wired, so nothing ever reaches
uploading/error), but once the transport lands, Enter with text plus an in-flight upload just no-ops while the composer clears. Surface the reason (or block the send affordance) rather than dropping it.api/oss/src/middlewares/prefix.py (1)
49-52: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueEvery request now pays an extra full route-table scan.
_known(scope, path)runs unconditionally on each HTTP request, and for the common case (path already known) that's one complete linear pass overself._routes()with regex matching per route, on top of Starlette's own routing pass. With a large route table this is a per-request hot-path cost.A cheap mitigation is a small memo keyed by
(method, path)of the "known" verdict, or skipping the probe when the path already ends in the shape the vast majority of routes declare.web/oss/src/components/AgentChatSlice/components/AttachmentViewerDrawer.tsx (1)
97-105: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRebuilding on
uploadsidentity revokes live object URLs mid-view.Any change to the
uploadsarray while the drawer is open (a new reference from the parent, or an upload status/percent update) tears down and re-creates every object URL, so the currently previewed blob URL is revoked underneath the viewer. Keying the rebuild on the uid set (or theoriginFileObjidentities) instead avoids the churn.♻️ Sketch
- useEffect(() => { + const uploadsKey = uploads.map((u) => u.uid).join("|") + useEffect(() => { if (!open) { setNodes(null) return } const built = buildNodes(uploads) setNodes(built) return () => built.source.forEach((v) => URL.revokeObjectURL(v.objectUrl)) - }, [open, uploads]) + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [open, uploadsKey])web/oss/src/components/AgentChatSlice/components/AudioPlayer.tsx (1)
99-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo scrub affordance and the progress bar is invisible to assistive tech.
The native element is
hidden, so play/pause is the only interaction: keyboard and screen-reader users can't seek, and the bar carries norole="progressbar"/aria-valuenow. Adding the ARIA attributes (and optionally a click/keyboard seek on the bar) closes that gap.web/oss/src/components/AgentChatSlice/components/ComposerAttachments.tsx (1)
71-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRaw
rgba(...)literals instead of theme tokens.The overlay scrim, progress track and error background use hardcoded color literals (
rgba(0,0,0,0.6),rgba(0,0,0,0.4),rgba(255,255,255,0.3)) plustext-white, which won't adapt across light/dark. Prefer Ant Design semantic tokens or supportedvar(--ag-color*)variables.As per coding guidelines, "Consume theme colors through Ant Design semantic tokens, Tailwind color utilities, or supported
var(--ag-color*)variables; do not use raw hex colors or--ag-c-*literals."Also applies to: 86-88, 102-102
Source: Coding guidelines
web/oss/src/components/Drives/DriveExplorer.tsx (1)
1727-1733: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
as unknown as MountFilefabricates a partial entity.The synthetic upload nodes only carry
path/size, and the double cast hides that from every downstream consumer of the built tree. It works today because both render paths short-circuit these nodes viapendingUploadByPath/pendingbefore touching other fields, but a future consumer readingtouchedAt(or any other required field) will getundefinedwith no compile error. Consider a narrow local type instead of casting throughunknown, e.g.Pick<MountFile, "path" | "size">on the tree-builder input.web/oss/src/components/AgentChatSlice/components/clientTools/useConnectFlow.ts (2)
36-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLeftover reviewer note in shipped code.
"NOTE for Mahmoud: confirm the bound" reads as an unresolved open question left in the comment rather than a settled design decision.
70-304: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftNo unit tests for this hook.
useConnectFlowimplements security-sensitive origin validation, popup lifecycle management, and multi-instance settle-race protection — exactly the kind of pure-ish, high-risk logic worth direct unit testing (mockingwindow.open/postMessage/timers), especially since this review found gaps in the settle guarantees the docstring promises.web/oss/src/components/AgentChatSlice/components/InteractionDock.tsx (1)
1-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim the introductory implementation comment.
Move the UX narrative to PR/docs; retain only a short constraint-oriented comment here. As per coding guidelines, “Keep in-code comments to at most one short line; use longer comments only for genuinely surprising constraints such as bugs, races, or ordering requirements.”
Source: Coding guidelines
web/oss/src/lib/helpers/dynamicEnv.ts (1)
40-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten the template-strip comment.
This documents ordinary feature behavior in a multi-line implementation comment; keep it to one short line and move rollout details to flag documentation. As per coding guidelines, keep in-code comments to at most one short line except for genuinely surprising constraints.
Source: Coding guidelines
web/oss/src/components/AgentChatSlice/components/RecordingBar.tsx (1)
66-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
aria-live="polite"on the container will announce the timer every second.The clock text changes ~1x/s inside the live region, so screen readers get a continuous stream of announcements for the whole take. Consider keeping the live region for state changes only and marking the running timer as
aria-hidden(or movingaria-liveonto the near-limit warning).♿ Proposed tweak
<span + aria-hidden className={`text-sm tabular-nums transition-colors duration-300 ${web/oss/src/components/pages/observability/components/NodeNameCell.tsx (1)
13-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSame
spanTypeStyles.unknownassumption asAvatarTreeContent.tsx.The duplicated
spanTypeStyles[type ?? "unknown"] ?? spanTypeStyles.unknownlookup would be better as a single shared resolver in../assets/constantsso the fallback contract lives in one place.web/oss/src/components/AgentChatSlice/components/VoiceInputButton.tsx (1)
138-152: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize
menuItems.
menuItemsrebuilds a JSX-bearing array on every render and is handed straight toDropdown. In a composer that re-renders frequently during streaming/dictation, wrap it inuseMemo.♻️ Suggested fix
- const menuItems: MenuProps["items"] = available.map((m) => ({ - key: m.key, - icon: modeIcon(m.key), - label: - m.key === "audio" && audioNotHeard ? ( - <span className="flex flex-col"> - <span>{MODE_LABEL[m.key]}</span> - <span className="text-[11px] text-colorTextTertiary"> - This model can't listen to audio - </span> - </span> - ) : ( - MODE_LABEL[m.key] - ), - })) + const menuItems: MenuProps["items"] = useMemo( + () => + available.map((m) => ({ + key: m.key, + icon: modeIcon(m.key), + label: + m.key === "audio" && audioNotHeard ? ( + <span className="flex flex-col"> + <span>{MODE_LABEL[m.key]}</span> + <span className="text-[11px] text-colorTextTertiary"> + This model can't listen to audio + </span> + </span> + ) : ( + MODE_LABEL[m.key] + ), + })), + [available, audioNotHeard], + )Source: Coding guidelines
| const popup = window.open( | ||
| redirectUrl, | ||
| oauthWindowName, | ||
| "width=600,height=700,popup=yes", | ||
| ) | ||
| if (!popup) { | ||
| setPhase("error") | ||
| setErrorText("Couldn’t open the connection window. Allow popups and retry.") | ||
| return | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file and inspect the relevant hook implementation around terminal branches.
target="web/oss/src/components/AgentChatSlice/components/clientTools/useConnectFlow.ts"
if [ ! -f "$target" ]; then
echo "target file not found"
fd -a 'useConnectFlow\.ts$' .
exit 1
fi
wc -l "$target"
sed -n '1,320p' "$target" | nl -ba | sed -n '1,320p'
# Identify all calls to finish() and terminal signal locations.
python3 - <<'PY'
from pathlib import Path
p=Path("web/oss/src/components/AgentChatSlice/components/clientTools/useConnectFlow.ts")
text=p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if 'finish(' in line or 'settleParkedCall' in line or 'function finish' in line or 'const finish' in line or 'runConnect' in line:
print(f"{i}: {line}")
PYRepository: Agenta-AI/agenta
Length of output: 278
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target="web/oss/src/components/AgentChatSlice/components/clientTools/useConnectFlow.ts"
if [ ! -f "$target" ]; then
echo "target file not found"
fd 'useConnectFlow\.ts$' .
exit 1
fi
echo "## file line count"
wc -l "$target"
echo "## first 320 lines with line numbers"
awk '{print NR": "$0}' "$target" | sed -n '1,320p'
echo "## relevant identifiers"
python3 - <<'PY'
from pathlib import Path
p=Path("web/oss/src/components/AgentChatSlice/components/clientTools/useConnectFlow.ts")
for i, line in enumerate(p.read_text().splitlines(), 1):
if any(token in line for token in [
'finish(', 'settleParkedCall', 'function finish', 'const finish', 'runConnect',
'window.open', 'popup_blocked', 'connected:', 'reactive: "connected"',
]):
print(f"{i}: {line}")
PYRepository: Agenta-AI/agenta
Length of output: 17801
Settle the popup-blocked path before returning.
When window.open is blocked, this is a terminal OAuth failure, but the branch only sets phase="error" and returns. For the live parked connection flow (settleParkedCall=true), finish() is never called, so the client tool never settles and the agent run can hang.
🐛 Proposed fix
if (!popup) {
setPhase("error")
setErrorText("Couldn’t open the connection window. Allow popups and retry.")
+ if (settleParkedCall) {
+ finish({
+ connected: false,
+ integration,
+ slug,
+ reason: "popup_blocked",
+ })
+ }
return
}📝 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.
| const popup = window.open( | |
| redirectUrl, | |
| oauthWindowName, | |
| "width=600,height=700,popup=yes", | |
| ) | |
| if (!popup) { | |
| setPhase("error") | |
| setErrorText("Couldn’t open the connection window. Allow popups and retry.") | |
| return | |
| } | |
| const popup = window.open( | |
| redirectUrl, | |
| oauthWindowName, | |
| "width=600,height=700,popup=yes", | |
| ) | |
| if (!popup) { | |
| setPhase("error") | |
| setErrorText("Couldn’t open the connection window. Allow popups and retry.") | |
| if (settleParkedCall) { | |
| finish({ | |
| connected: false, | |
| integration, | |
| slug, | |
| reason: "popup_blocked", | |
| }) | |
| } | |
| return | |
| } |
Railway Preview Environment
|
dosubot
Bot
added
size:M
…thub-issue-5528-3377d2 # Conflicts: # web/oss/package.json # web/pnpm-lock.yaml
1 participant
Fixes #5528.
Problem
The unit test files colocated under
web/oss/srcimported fromvitestbut never executed — not in CI, not locally.web/osshad no vitest config,vitestwas not a dependency, andpackage.jsondeclared notest:unitscript. The unit-layer runner selects packages withpnpm -r --if-present run test:unit(web/tests/playwright/scripts/run-tests.ts), so a package without that script is skipped silently and the "Run web unit tests" job still passes. The suites passed review while asserting nothing.Fix
web/oss/vitest.config.ts(new): colocated globsrc/**/*.test.{ts,tsx}(oss keeps tests next to source, unlike the@agenta/*packages undertests/unit/), the twotsconfig.jsonpath aliases (@/oss/*,@/agenta-oss-common/*),jsdomenvironment (some tests touchwindow.localStorage), and thejunitreporter writing totest-results/junit.xml.@vitejs/plugin-react-swcfor the JSX transform.tsconfig.jsonsetsjsx: "preserve"for the Next build, which leaves JSX in the source and breaks vite's parser the moment a test transitively imports a.tsx(the state-selector tests reachAlertPopup.tsxthrough the app graph). The SWC React plugin transforms JSX itself and ignores that tsconfig setting.web/oss/package.json:test/test:unit/test:watch/test:coveragescripts and dev deps (vitest,@vitest/coverage-v8,jsdom,@vitejs/plugin-react-swc).test:unitis what makes the recursive runner pick oss up..github/workflows/12-check-unit-tests.yml: publishweb/oss/test-results/junit.xmlalongside the per-package reports (oss is not underweb/packages/*).web/oss/.gitignore: ignore/test-results.Result
cd web/oss && pnpm run test:unitruns 14 files / 98 tests, all passing (the issue listed 13; a 14th,transcriptToMessages.test.ts, was added since). A failing suite exits non-zero — during setup the pre-fix parse failures produced exit code 1 while other tests passed — so a failing assertion now fails the build.Notes
web/ee/srchas no test files today, so it is unaffected; the same wiring could be added there if/when it gains tests.