feat(webapp): surface run cell in the run admin panel#4308
Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📜 Recent review details⏰ Context from checks skipped due to timeout. (20)
WalkthroughThe span route now authenticates with the full user, derives an admin flag, and passes both user identity and admin state to 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
7dfd4a9 to
37267ea
Compare
|
|
||
| export const loader = async ({ request, params }: LoaderFunctionArgs) => { | ||
| const userId = await requireUserId(request); | ||
| const user = await requireUser(request); |
There was a problem hiding this comment.
🟡 Opening a run's detail panel now runs an extra database lookup for every user
The span-detail resource route now looks up the full user record from the database on every load (requireUser at apps/webapp/app/routes/.../spans.$spanParam/route.tsx:111) instead of the previous cookie-only check, so every user who opens a run/span panel pays an added database round-trip, not just admins.
Impact: A frequently-hit dashboard endpoint gains a per-request database query for all users, increasing load and latency contrary to the intended admin-only cost.
Why the non-admin path changed
requireUserId (previously used here) is documented as the cookie-only fast path with "zero DB queries" on the non-impersonation branch (apps/webapp/app/services/session.server.ts:82-91). The new requireUser calls getUser → getUserById, an unconditional DB fetch (apps/webapp/app/services/session.server.ts:94-110), plus a getImpersonationId read. This resource route is loaded via fetcher whenever a span/run detail panel is opened, so the extra query runs for every viewer regardless of admin status — the PR description's claim that "the hot non-admin path is unchanged" does not hold. The admin flag genuinely requires the user row, but the added cost is now borne by all users rather than gated.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const resource = rootSpan?.resourceProperties as Record<string, any> | undefined; | ||
| const value = resource?.trigger?.cell ?? resource?.["trigger.cell"]; | ||
| cell = typeof value === "string" ? value : undefined; |
There was a problem hiding this comment.
🔍 Cell attribute source shape depends on event store implementation
The cell value is read as resource?.trigger?.cell ?? resource?.["trigger.cell"] from rootSpan.resourceProperties (apps/webapp/app/presenters/v3/SpanPresenter.server.ts:414-416). This gracefully handles both nested and flat attribute shapes. Note that resourceProperties is only populated from the $resource attributes in the ClickHouse event repository (apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts:1916-1921). If a deployment's span for the run is served by an event store path that does not populate resourceProperties, the field will always render -. This matches the PR's stated best-effort/optional intent, so it is not a bug, but worth confirming the trigger.cell attribute is actually emitted as a resource attribute (not a span attribute) in the environments this feature targets.
Was this helpful? React with 👍 or 👎 to provide feedback.
@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: |
1 participant
Adds a Cell field to the run detail page's admin-only panel that surfaces which cell a run executed on, when that information is present in the run's telemetry.
Why
For deployments that spread runs across multiple cells, it's useful to see which cell an individual run landed on. The admin panel shows region but not cell - this surfaces the cell whenever the run's telemetry carries it.
How
Display-only, sourced entirely from telemetry: if the run's execution span carries a
trigger.cellresource attribute, the panel shows it - read via the existinggetSpanpath from the event store. Nothing is read from or added to the run record or the database.-. This change only surfaces the attribute; it does not produce it.-- it never blocks or breaks the panel. Reads both the nested and flat attribute shapes to be safe.Scope
webapp-only. No package changes, no schema or database changes, no changes to the trigger hot path.
Testing
-for runs with notrigger.cellattribute (e.g. local runs, or before the span is ingested).