feat(webapp): dashboard agent — UI - #4529
Conversation
|
|
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: Repository 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:
WalkthroughThe pull request expands the Dashboard Agent into a page-aware chat experience. It adds shared channel routing, fullscreen controls, chat history, quotas, transcript handling, structured view blocks, investigations, reports, suggested prompts, and route metadata. It removes selected page-header Docs controls and adds investigation actions for failed runs, waiting runs, and degraded queues. Tests cover routing, prompts, rendering, transcript state, quotas, navigation, accessibility, and report parity. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
bd4d4a0 to
887f5b6
Compare
887f5b6 to
17a0f07
Compare
2460d14 to
9447128
Compare
ddb9f3c to
88efcf7
Compare
Observability mapAs of Nothing in this pull request moves the report any more. The findings an earlier push reported are gone. The score and findings here are report-only and never gate the merge. Separately, a required test suite keeps this tool's symbol and route lists in sync with the code they name, and can fail a pull request that renames or removes a symbol they reference, or that adds the first route with a segment they anticipate. Each failure names the list to edit. The rules and their reasons: internal-packages/observability-map/README.md. |
88efcf7 to
83ae7f2
Compare
| vi.mock("~/services/dashboardAgentDb.server", () => ({ dashboardAgentDb: undefined })); | ||
|
|
||
| const { sweepDashboardAgentInvestigations } = | ||
| await import("~/services/dashboardAgentInvestigationSweep.server"); |
There was a problem hiding this comment.
🟡 New test mocks an application module instead of using a real container, against the repository's testing rule
The new sweep test replaces a real module with a fake one (vi.mock("~/services/dashboardAgentDb.server", …) at apps/webapp/test/dashboardAgentInvestigationSweepCard.test.ts:19), which the repository's testing rules forbid.
Impact: The test suite diverges from the project's stated testing approach, so the guidance stops being reliable for future contributors.
Rules involved
AGENTS.md, "Testing": "We use vitest exclusively. Never mock anything - use testcontainers instead." The test mocks ~/services/dashboardAgentDb.server to avoid opening a pool; the repo's @internal/testcontainers helpers (postgresTest, containerTest) are the sanctioned way to do this.
The mock also forces a dynamic import (const { sweepDashboardAgentInvestigations } = await import("~/services/dashboardAgentInvestigationSweep.server"), lines 21-22), which AGENTS.md's "Imports" section asks to avoid in favour of static imports.
Prompt for agents
`apps/webapp/test/dashboardAgentInvestigationSweepCard.test.ts` uses `vi.mock` on `~/services/dashboardAgentDb.server` purely so that importing `dashboardAgentInvestigationSweep.server` does not open a connection pool, and then has to use `await import(...)` because of the mock ordering. AGENTS.md forbids mocking (use `@internal/testcontainers` instead) and prefers static imports. Since every write in this test is already injected through the `listStale`/`settleAndClose` dependencies, the cleanest fix is to move the pure, dependency-injected `sweepDashboardAgentInvestigations` function into a module that does not import the datastore singleton at all, so the test can import it statically with no mock. Alternatively, run the test against a real Postgres via `postgresTest`/`containerTest`.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
The mock is dashboardAgentDb: undefined and it stands in for nothing under test — every behavioural dependency in that suite is injected. It exists only to neutralise an import-time side effect: singleton() is eager, so importing the module constructs a Postgres pool. The database half is covered by dashboardAgentInvestigationSweep.test.ts, a real container test with no mocks at all. Removing the stub properly means making the singleton lazy — 26 references across 8 files — which is a refactor well beyond this finding, so it stays with the reason recorded in the test.
83ae7f2 to
00a22c6
Compare
92efdcd to
2481567
Compare
The panel, the page-context marks on the pages the agent reads, and the entry points.
Ask AI (Kapa) owns the two entry points it had before the dashboard agent replaced it: Cmd-I, and the `?aiHelp=` deep link the CLI's "Get a fix for this error using AI" line points at. `AskAIRoot` mounts in the `_app` layout, above every signed-in page, so Cmd-I reaches it from org-level pages too and the dialog outlives whatever opened it. The agent no longer reads deep links at all: nothing produced its `?ask=` param except the CLI redirect, and both readers consume the param, so a live agent reader would always beat Kapa to it. It stays the fall-through — where Kapa cannot open (self-hosted, or no website id), both channels land on the agent instead of dead-ending.
…the pipeline emits The fixture still set `facts.staleReason`, renamed to `untrustworthyReason` three commits before the caveat started reading it, so the branch's only trust snapshot fell back to "could not be verified" for a report whose reason is known.
The scheduled example lost its import line to the standard one, so copying it gave code that does not compile.
Without org/project/env context the run button rendered but did nothing.
Shortcuts can now ask for the browser default to be prevented, and the agent's keystroke does.
Selecting a stored chat with no messages dropped you into a fresh draft, as if the chat had been deleted.
Radix tooltip content is not the accessible name of its trigger, so the icon-only ask-agent button and the two deploy docs links announced as unnamed controls. Name them explicitly and pass asChild so the tooltip trigger stops wrapping them in a second button. Adds a source scan that fails on the next SimpleTooltip with an unnamed or double-wrapped control, with the pre-existing sites baselined.
A limit of 0 is zero capacity, not saturation: running >= 0 holds for every queue, so any backlog marked the queue degraded and offered Investigate, while the agent's own suggested prompt stayed silent. One predicate now decides it for the queue detail page, the queues list badge and the page mappers.
Retry appended the last user message again, so the failed turn stayed in the transcript and its text was sent twice. It now regenerates once the agent has started answering, and otherwise re-sends the failed turn under its own id.
…e reader's clock Bar timestamps came from Date.now() during render, so the same bar reported a different time on every re-render and a server pass disagreed with the client. They now come from the view model's generatedAt, which the schema already describes as the timestamp the renderer must not invent. Moves the arithmetic into report-spark.ts to keep it clock-free and testable, and drops the unreachable Math.max on the slice end while doing so.
ViewBlocks looked each surviving block's index up with indexOf inside the render loop: quadratic, and two occurrences of the same block object both answered with the first index, so they collided on one React key. latestRevisionEntries carries each survivor's position out instead.
setSearchParams only starts the navigation that drops the param, so a render before it commits saw the question again and asked it a second time. The reader now records what it sent and forgets it once the URL no longer carries it, so a later visit with the same question still works.
A request still in flight at unmount rejected afterwards, and the catch scheduled a retry that fetched again and set state for a component that was gone. The hook tracks whether it is still mounted and neither records nor reschedules once it is not.
The character counter's live region only entered the DOM at the warning point, and several screen readers only announce updates for a region that was already there; it is now always mounted and empty until there is something to say. The history trigger's aria-label replaced the chat title it shows, so a speech-input user could not activate it by the words on it. The title now leads the accessible name.
…anel justify-center on a scrolling column overflows equally in both directions, and nothing can scroll back past the origin, so at the docked panel's narrowest the heading and composer were unreachable. The child centres with m-auto, which gives its space up once there is none to spare.
new URL(environmentPath, origin) ignores origin when the path is absolute, and the result goes straight into redirect(). Today's only caller passes a builder-generated internal path, so this closes the gap rather than a hole.
A preview branch named `env` put a second `env` segment in the path and `lastIndexOf` picked it, shifting every index derived from it.
… keystroke Between the warning point and the limit the region announced a new count per character; it now steps in 200s and names the limit on reaching it.
…review fixes - `_app`: `AskAIRoot` is a sibling of the app, not a wrapper. Its Kapa provider mounts client-only, so wrapping the outlet remounted the whole signed-in tree once per page load on cloud. Entry points reach it through an open-request bridge instead of a render prop. - The agent no longer hands a `trigger://source/...` target to `navigate`: a resolved GitHub URL opens in a new tab, and only a root-relative path is routed. - Chat history reloads coalesce without answering a request with data fetched before it, so a new chat and its title land. - Help & Feedback and the shortcuts sheet offer each AI surface only where the reader has it: Ask AI is back for users without agent access. - Restore the `AskAgentButton` mount on the deploy blank states, dropped in the PR split. - `agent-shortcuts.test.ts`: import ⌘I from `ask-ai-channels`.
…rface Where neither Ask AI nor the agent can open, the redirect left `?aiHelp=` on the environment page with nothing to read it. Send the question to the docs instead.
The transcript perf fix had two halves. The winners half kept its identity because a test pinned it; the strip cache had none, so rewriting the component into chat primitives quietly brought back the rebuild-every-render version, and every tool-calling turn re-rendered on each streamed token again. Restore the cache and pin it the same way. orderTranscript is not implicated: it returns a fresh array but the same message objects, which is what the turns memoize on.
2481567 to
d87ca1d
Compare
Stacked on #4418. Merge that first.
The dashboard agent's UI: the side panel, the marks that tell it which page you're on, and the entry points. #4418 works without this — the system is simply invisible.
What's inside
handle.agentPageContexton 47 routes, ~20 lines each.?aiHelp=links keep working.Notes
canAccessDashboardAgent; no behavior change with the flag off.