feat(ambient-ui): Resources/Config tabs, session table enhancements, navigation overhaul#1638
Merged
Merged
Conversation
…tail Enable the remaining two disabled tabs in the session detail view: - Resources tab: shows repositories merged with reconciliation state (clone status badges, active branch, cloned timestamp), MCP servers placeholder, empty state for sessions with no resources - Details tab: shows configuration metadata (model, temperature, max tokens, timeout, workflow), prompt with truncation/expand, environment variables, registered annotations with icons from the annotation registry, raw annotations table, and labels Extends DomainSession with repos, reconciledRepos, conditions, environmentVariables, labels, temperature, maxTokens, timeout, workflowId, prompt, and sdkRestartCount fields parsed from the SDK Session type. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Rename tabs: Phase→Overview, Details→Config, add icons to all tabs - Default to Logs tab instead of Overview - Eliminate data overlap between Overview and Config tabs - Unify registered/raw annotations into single list - Mask secret-looking env var values with reveal toggle - Remove dead MCP Servers placeholder - Make repo URLs clickable links with tooltips - Fix hardcoded colors to use theme tokens - Add phase-colored timeline dots - Extract shared MetaRow component - Add section counts, responsive grid, consistent missing values - Show prompt char count on expand toggle Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Rename "Fleet" → "Sessions" in all user-facing labels - Fix breadcrumbs to show session name on detail pages - Fix sidebar active state on child routes (startsWith) - Remove disabled "Coming soon" sidebar items - Fix session header sticky offset (top-14) - Rename DetailsTab → ConfigTab for naming consistency - Add Ambient wordmark to sidebar header - Demote heading sizes, fix spacing, standardize icons Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fetch agent names from API and resolve ID→name in fleet table - Fix duration: Running sessions compute from start to now, terminal sessions only use completionTime when it's after startTime - Fix last activity: Running sessions show "Active now", terminal sessions show relative time from completionTime - Fix breadcrumb to show session display name instead of raw ID Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rename the URL path from /{projectId}/fleet to /{projectId}/sessions
to match the user-facing "Sessions" label. Updates all route references,
sidebar href, breadcrumb logic, and navigation links. Also fixes
timeline dot/line alignment in overview tab.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nd filtering - Add column sorting with smart default (Failed first, then Running) - Pin Chat column to right edge with shadow separator - Make summary bar phase counts clickable for filtering - Summary bar reflects filtered state when search is active - Precise duration format (2h 03m instead of "about 2 hours") - Differentiate "Active now" / "Starting..." / "Stopping..." states - Add visual containment to summary bar - Improve row hover states and keyboard navigation - Add Chat column header icon - Drop uppercase table headers for ops density - Fix hardcoded colors to use semantic tokens Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
Contributor
📝 WalkthroughWalkthroughThis PR restructures the session dashboard by migrating from fleet-centric to sessions-centric navigation. It expands ChangesSession Dashboard Restructure
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
components/ambient-ui/src/components/nav-header.tsx (1)
95-119: 💤 Low valueTwo adjacent breadcrumbs resolve to the same href on the detail view.
On a session detail page the project crumb (Line 100) and the mapped "Sessions" crumb (Line 112) both link to
/${projectId}/sessions, so users see two consecutive links pointing to the same destination. Functional, just slightly redundant — consider rendering the "Sessions" crumb as a plainBreadcrumbPagewhen it would duplicate the parent target.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/ambient-ui/src/components/nav-header.tsx` around lines 95 - 119, The two breadcrumb entries render links to the same href when on a session detail: adjust the conditional that renders the mapped "Sessions" crumb (the block using mappedPageName, sessionName, BreadcrumbLink/BreadcrumbPage) so that when projectId is present and the mapped crumb's href would be `/${projectId}/sessions` (the same as the project crumb) you render a non-link BreadcrumbPage instead of a BreadcrumbLink; update the conditional around the mappedPageName branch to check the duplicate-target case (using projectId, mappedPageName and sessionName) and choose BreadcrumbPage when duplicates would occur, otherwise keep the existing BreadcrumbLink behavior.components/ambient-ui/src/app/(dashboard)/[projectId]/sessions/_components/fleet-columns.tsx (1)
73-79: ⚡ Quick winWire the phase column to the registered
phaseEqualsfilterFn.
fleet-table.tsxregistersfilterFns.phaseEqualsbut this column never references it, so it's dead code. Phase filtering currently falls back to TanStack'sauto/includesString(substring) matcher. It works today only because no phase name is a substring of another — fragile if a phase label ever changes.♻️ Use exact-match filterFn
col.accessor('phase', { header: 'Phase', cell: info => <PhaseBadge phase={info.getValue()} />, size: 130, enableSorting: true, sortingFn: phaseSortingFn, + filterFn: 'phaseEquals', }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/ambient-ui/src/app/`(dashboard)/[projectId]/sessions/_components/fleet-columns.tsx around lines 73 - 79, The Phase column defined in col.accessor('phase', ...) is not wired to the registered phaseEquals filter, so filtering uses the generic substring matcher; update the column to use the exact-match filter by adding the filterFn reference (either the string key "phaseEquals" or the actual phaseEquals function imported/registered in fleet-table.tsx) to the column definition (i.e., add filterFn: 'phaseEquals' or filterFn: phaseEquals alongside header/cell/size/sortingFn) so the PhaseBadge column uses the exact-match filter.components/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/__tests__/resources-tab.test.tsx (1)
58-71: ⚡ Quick winFixtures use identical URLs on both sides, so the merge path is never exercised against a
.gitmismatch. Consider a case wherereposandreconciledReposdiffer only by the.gitsuffix to lock in the merge behavior (see related comment onresources-tab.tsx).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/ambient-ui/src/app/`(dashboard)/[projectId]/sessions/[sessionId]/_components/__tests__/resources-tab.test.tsx around lines 58 - 71, The test fixtures currently use identical repo URLs so the merge-with-.git-suffix logic in ResourcesTab isn't exercised; update the test data used by this spec (the REPO and RECONCILED fixtures passed into makeSession in the test) so their URL fields differ only by the ".git" suffix (e.g., one URL ends with ".git" and the other does not) to trigger the merge path in resources-tab.tsx, then keep the same render/assertions to verify the merged result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/ambient-ui/src/adapters/mappers.ts`:
- Around line 118-120: numberOrNull currently treats 0 as null which discards a
valid temperature=0; change the logic so numberOrNull returns null only for
undefined or null (do not collapse 0), and introduce a separate helper (e.g.
numberNullIfZero) that preserves the original behavior for fields that should
treat 0 as unset (maxTokens/timeout). Update callers: use numberOrNull for
temperature mapping and use numberNullIfZero for maxTokens/timeout (or rename
functions to match intent) while keeping the function names numberOrNull and the
new helper referenced in the mapper code.
- Around line 73-112: The parser functions parseRepos, parseReconciledRepos, and
parseConditions currently cast each array item to Record<string, unknown> and
access properties directly, which throws if an item is null or a primitive;
update each function to first check that item is a non-null object (e.g., typeof
item === "object" && item !== null) before reading properties from it, and for
invalid entries return sensible defaults or skip them (e.g., map to null/empty
fields or filter them out) so malformed parseJsonArray outputs like [null] or
[1,2] do not cause a runtime TypeError; apply this guard consistently inside
parseRepos, parseReconciledRepos, and parseConditions when handling r / c and
preserve existing field coercions (String(...), Boolean(...), VALID_* checks).
In
`@components/ambient-ui/src/app/`(dashboard)/[projectId]/sessions/[sessionId]/_components/resources-tab.tsx:
- Around line 32-68: mergeRepos is matching on raw url strings so ".../repo.git"
vs ".../repo" won't join; normalize URLs (e.g., strip a trailing ".git"
case-insensitively and optional trailing slash) before inserting into
reconciledByUrl Map and the seen Set and when checking matches. Concretely, add
a small normalization step (normalizeUrl) and use normalizedUrl as the Map key
and Set values while still using the original repo.url or reconciled.url for the
returned MergedRepo.url field and for name/branch/status lookups (use
reconciledByUrl.get(normalizedUrl) and seen.has(normalizedUrl)).
In
`@components/ambient-ui/src/app/`(dashboard)/[projectId]/sessions/[sessionId]/page.tsx:
- Around line 24-27: The useState initializer for activeTab (in
SessionDetailPage) reads window on the client but returns 'overview' on the
server, causing a hydration mismatch when the incoming URL has ?tab=...; change
to initialize activeTab to a stable server value (e.g., 'overview') and then
update it on mount: in a useEffect (or via next/navigation's useSearchParams in
a client hook) read the URL's search param and call setActiveTab to the real tab
value; update references to activeTab/setActiveTab only—do not access window
during initial render—to ensure server and client markup match and avoid
hydration warnings.
---
Nitpick comments:
In
`@components/ambient-ui/src/app/`(dashboard)/[projectId]/sessions/_components/fleet-columns.tsx:
- Around line 73-79: The Phase column defined in col.accessor('phase', ...) is
not wired to the registered phaseEquals filter, so filtering uses the generic
substring matcher; update the column to use the exact-match filter by adding the
filterFn reference (either the string key "phaseEquals" or the actual
phaseEquals function imported/registered in fleet-table.tsx) to the column
definition (i.e., add filterFn: 'phaseEquals' or filterFn: phaseEquals alongside
header/cell/size/sortingFn) so the PhaseBadge column uses the exact-match
filter.
In
`@components/ambient-ui/src/app/`(dashboard)/[projectId]/sessions/[sessionId]/_components/__tests__/resources-tab.test.tsx:
- Around line 58-71: The test fixtures currently use identical repo URLs so the
merge-with-.git-suffix logic in ResourcesTab isn't exercised; update the test
data used by this spec (the REPO and RECONCILED fixtures passed into makeSession
in the test) so their URL fields differ only by the ".git" suffix (e.g., one URL
ends with ".git" and the other does not) to trigger the merge path in
resources-tab.tsx, then keep the same render/assertions to verify the merged
result.
In `@components/ambient-ui/src/components/nav-header.tsx`:
- Around line 95-119: The two breadcrumb entries render links to the same href
when on a session detail: adjust the conditional that renders the mapped
"Sessions" crumb (the block using mappedPageName, sessionName,
BreadcrumbLink/BreadcrumbPage) so that when projectId is present and the mapped
crumb's href would be `/${projectId}/sessions` (the same as the project crumb)
you render a non-link BreadcrumbPage instead of a BreadcrumbLink; update the
conditional around the mappedPageName branch to check the duplicate-target case
(using projectId, mappedPageName and sessionName) and choose BreadcrumbPage when
duplicates would occur, otherwise keep the existing BreadcrumbLink behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ef5af021-7884-474f-9d09-b32a38236892
📒 Files selected for processing (44)
components/ambient-ui/src/adapters/__tests__/mappers.test.tscomponents/ambient-ui/src/adapters/mappers.tscomponents/ambient-ui/src/app/(dashboard)/[projectId]/fleet/[sessionId]/_components/phase-tab.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/fleet/_components/__tests__/fleet-summary.test.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/fleet/_components/fleet-columns.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/fleet/_components/fleet-summary.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/fleet/_components/fleet-table.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/__tests__/config-tab.test.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/__tests__/event-type-badge.test.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/__tests__/logs-tab.test.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/__tests__/resources-tab.test.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/chat-tab.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/config-tab.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/event-announcer.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/event-row.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/event-summary-banner.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/event-type-badge.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/live-tail-indicator.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/logs-tab.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/meta-row.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/overview-tab.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/resources-tab.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/session-header.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/page.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/_components/__tests__/fleet-summary.test.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/_components/__tests__/phase-badge.test.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/_components/fleet-columns.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/_components/fleet-summary.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/_components/fleet-table.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/_components/phase-badge.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/page.tsxcomponents/ambient-ui/src/app/(dashboard)/layout.tsxcomponents/ambient-ui/src/app/(dashboard)/page.tsxcomponents/ambient-ui/src/components/app-sidebar.tsxcomponents/ambient-ui/src/components/chat-sidebar.tsxcomponents/ambient-ui/src/components/empty-state.tsxcomponents/ambient-ui/src/components/nav-header.tsxcomponents/ambient-ui/src/components/project-selector.tsxcomponents/ambient-ui/src/components/ui/table.tsxcomponents/ambient-ui/src/domain/types.tscomponents/ambient-ui/src/lib/__tests__/format-timestamp.test.tscomponents/ambient-ui/src/lib/format-timestamp.tscomponents/ambient-ui/src/queries/query-keys.tscomponents/ambient-ui/src/queries/use-agents.ts
💤 Files with no reviewable changes (5)
- components/ambient-ui/src/app/(dashboard)/[projectId]/fleet/_components/tests/fleet-summary.test.tsx
- components/ambient-ui/src/app/(dashboard)/[projectId]/fleet/[sessionId]/_components/phase-tab.tsx
- components/ambient-ui/src/app/(dashboard)/[projectId]/fleet/_components/fleet-table.tsx
- components/ambient-ui/src/app/(dashboard)/[projectId]/fleet/_components/fleet-summary.tsx
- components/ambient-ui/src/app/(dashboard)/[projectId]/fleet/_components/fleet-columns.tsx
Comment on lines
+73
to
+112
| function parseRepos(raw: string): DomainRepo[] { | ||
| return parseJsonArray(raw).map((item) => { | ||
| const r = item as Record<string, unknown> | ||
| return { | ||
| url: String(r.url ?? ''), | ||
| branch: r.branch ? String(r.branch) : null, | ||
| name: r.name ? String(r.name) : null, | ||
| autoPush: Boolean(r.autoPush), | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| function parseReconciledRepos(raw: string): DomainReconciledRepo[] { | ||
| return parseJsonArray(raw).map((item) => { | ||
| const r = item as Record<string, unknown> | ||
| const status = String(r.status ?? '') | ||
| return { | ||
| url: String(r.url ?? ''), | ||
| name: r.name ? String(r.name) : null, | ||
| status: VALID_REPO_STATUSES.has(status) ? (status as ReconciledRepoStatus) : null, | ||
| currentActiveBranch: r.currentActiveBranch ? String(r.currentActiveBranch) : null, | ||
| defaultBranch: r.defaultBranch ? String(r.defaultBranch) : null, | ||
| clonedAt: r.clonedAt ? String(r.clonedAt) : null, | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| function parseConditions(raw: string): DomainCondition[] { | ||
| return parseJsonArray(raw).map((item) => { | ||
| const c = item as Record<string, unknown> | ||
| const status = String(c.status ?? 'Unknown') | ||
| return { | ||
| type: String(c.type ?? ''), | ||
| status: VALID_CONDITION_STATUSES.has(status) ? (status as ConditionStatus) : 'Unknown', | ||
| reason: c.reason ? String(c.reason) : null, | ||
| message: c.message ? String(c.message) : null, | ||
| lastTransitionTime: c.lastTransitionTime ? String(c.lastTransitionTime) : null, | ||
| } | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against non-object array entries before property access.
parseJsonArray can legitimately return arrays containing null or primitives (e.g. backend emits "[null]" or "[1,2]"). Here const r = item as Record<string, unknown> is a compile-time-only cast; at runtime r.url on a null item throws TypeError, and the .map isn't wrapped in try/catch — so a malformed repos/reconciled_repos/conditions payload crashes the render of the Resources/Config tabs.
🛡️ Suggested guard (apply to all three parsers)
+function isRecord(v: unknown): v is Record<string, unknown> {
+ return typeof v === 'object' && v !== null && !Array.isArray(v)
+}
+
function parseRepos(raw: string): DomainRepo[] {
- return parseJsonArray(raw).map((item) => {
- const r = item as Record<string, unknown>
- return {
+ return parseJsonArray(raw)
+ .filter(isRecord)
+ .map((r) => ({
url: String(r.url ?? ''),
branch: r.branch ? String(r.branch) : null,
name: r.name ? String(r.name) : null,
autoPush: Boolean(r.autoPush),
- }
- })
+ }))
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/ambient-ui/src/adapters/mappers.ts` around lines 73 - 112, The
parser functions parseRepos, parseReconciledRepos, and parseConditions currently
cast each array item to Record<string, unknown> and access properties directly,
which throws if an item is null or a primitive; update each function to first
check that item is a non-null object (e.g., typeof item === "object" && item !==
null) before reading properties from it, and for invalid entries return sensible
defaults or skip them (e.g., map to null/empty fields or filter them out) so
malformed parseJsonArray outputs like [null] or [1,2] do not cause a runtime
TypeError; apply this guard consistently inside parseRepos,
parseReconciledRepos, and parseConditions when handling r / c and preserve
existing field coercions (String(...), Boolean(...), VALID_* checks).
Comment on lines
+118
to
+120
| function numberOrNull(value: number): number | null { | ||
| return value === 0 || value === undefined || value === null ? null : value | ||
| } |
Contributor
There was a problem hiding this comment.
numberOrNull discards a valid temperature of 0.
temperature: 0 is a meaningful deterministic setting, but numberOrNull collapses it to null (same as unset). It's correct for maxTokens/timeout, but for temperature 0 should be preserved. Consider a dedicated nullable-numeric that only nulls undefined/null for temperature.
Also applies to: 132-132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/ambient-ui/src/adapters/mappers.ts` around lines 118 - 120,
numberOrNull currently treats 0 as null which discards a valid temperature=0;
change the logic so numberOrNull returns null only for undefined or null (do not
collapse 0), and introduce a separate helper (e.g. numberNullIfZero) that
preserves the original behavior for fields that should treat 0 as unset
(maxTokens/timeout). Update callers: use numberOrNull for temperature mapping
and use numberNullIfZero for maxTokens/timeout (or rename functions to match
intent) while keeping the function names numberOrNull and the new helper
referenced in the mapper code.
Comment on lines
+32
to
+68
| function mergeRepos( | ||
| repos: DomainRepo[], | ||
| reconciledRepos: DomainReconciledRepo[], | ||
| ): MergedRepo[] { | ||
| const reconciledByUrl = new Map( | ||
| reconciledRepos.map(r => [r.url, r]), | ||
| ) | ||
|
|
||
| const seen = new Set<string>() | ||
| const result: MergedRepo[] = [] | ||
|
|
||
| for (const repo of repos) { | ||
| seen.add(repo.url) | ||
| const reconciled = reconciledByUrl.get(repo.url) | ||
| result.push({ | ||
| url: repo.url, | ||
| name: reconciled?.name ?? repo.name ?? baseNameFromUrl(repo.url), | ||
| branch: reconciled?.currentActiveBranch ?? repo.branch ?? null, | ||
| status: reconciled?.status ?? null, | ||
| clonedAt: reconciled?.clonedAt ?? null, | ||
| }) | ||
| } | ||
|
|
||
| for (const reconciled of reconciledRepos) { | ||
| if (!seen.has(reconciled.url)) { | ||
| result.push({ | ||
| url: reconciled.url, | ||
| name: reconciled.name ?? baseNameFromUrl(reconciled.url), | ||
| branch: reconciled.currentActiveBranch ?? null, | ||
| status: reconciled.status, | ||
| clonedAt: reconciled.clonedAt, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| return result | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate the reviewed file and inspect the relevant section
FILE="components/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/resources-tab.tsx"
echo "=== File: $FILE ==="
sed -n '1,200p' "$FILE" | nl -ba | sed -n '20,140p'
# 2) Find baseNameFromUrl definition/usage
echo "=== baseNameFromUrl search ==="
rg -n "baseNameFromUrl\\b" -S . | head -n 50
# 3) Find DomainRepo / DomainReconciledRepo types
echo "=== DomainRepo / DomainReconciledRepo search ==="
rg -n "type\\s+DomainRepo\\b|interface\\s+DomainRepo\\b|DomainRepo\\b" -S components/ambient-ui/src | head -n 50
rg -n "type\\s+DomainReconciledRepo\\b|interface\\s+DomainReconciledRepo\\b|DomainReconciledRepo\\b" -S components/ambient-ui/src | head -n 50
# 4) Search for any URL normalization that strips ".git"
echo "=== .git normalization search ==="
rg -n "\\.git\\b|replace\\(.*\\.git\\$|strip.*git|normalize.*url" -S components/ambient-ui/src | head -n 80Repository: ambient-code/platform
Length of output: 245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
REPO_ROOT="$(pwd)"
FILE="components/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/resources-tab.tsx"
echo "=== sanity: file exists? ==="
if [ -f "$FILE" ]; then echo "FOUND: $FILE"; else echo "MISSING: $FILE"; exit 1; fi
echo "=== reviewed section (lines 1-140 with numbers via awk) ==="
awk '{printf "%6d\t%s\n", NR, $0}' "$FILE" | sed -n '1,160p'
echo "=== baseNameFromUrl search (definition + nearby) ==="
rg -n "function\\s+baseNameFromUrl\\b|\\bbaseNameFromUrl\\b" -S components/ambient-ui/src > /tmp/baseName_hits.txt || true
head -n 80 /tmp/baseName_hits.txt
echo "--- showing context for first few hits ---"
awk 'NR<=8{print}' /tmp/baseName_hits.txt | cut -f1 | while read -r loc; do
: # noop
done
# Show file+line context for the first match only (avoid big output)
FIRST_MATCH="$(head -n 1 /tmp/baseName_hits.txt || true)"
echo "=== first match: $FIRST_MATCH ==="
if [ -n "$FIRST_MATCH" ]; then
path="$(echo "$FIRST_MATCH" | cut -d: -f1)"
line="$(echo "$FIRST_MATCH" | cut -d: -f2)"
awk -v L="$line" 'NR>=L-30 && NR<=L+30 {printf "%6d\t%s\n", NR, $0}' "$path" | sed -n '1,100p'
fi
echo "=== DomainRepo / DomainReconciledRepo types search ==="
rg -n "DomainRepo\\b|DomainReconciledRepo\\b" -S components/ambient-ui/src | head -n 120
echo "=== .git normalization search ==="
rg -n "\\.git\\b|normalize.*url|strip.*git|replace\\([^)]*\\.git" -S components/ambient-ui/src | head -n 120Repository: ambient-code/platform
Length of output: 13443
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show Domain types
echo "=== domain/types.ts (relevant) ==="
awk 'NR>=1 && NR<=120{print NR "\t" $0}' components/ambient-ui/src/domain/types.ts | sed -n '1,120p'
# Show mappers normalization (if any)
echo "=== adapters/mappers.ts (relevant) ==="
# Print around parseRepos and parseReconciledRepos
rg -n "function\\s+parseRepos\\b|function\\s+parseReconciledRepos\\b" components/ambient-ui/src/adapters/mappers.ts
python3 - <<'PY'
import re, itertools, pathlib
path = pathlib.Path("components/ambient-ui/src/adapters/mappers.ts")
txt = path.read_text().splitlines()
def find_lines(pattern):
for i,line in enumerate(txt, start=1):
if re.search(pattern, line):
return i
return None
for pat in [r"function\s+parseRepos\b", r"function\s+parseReconciledRepos\b"]:
ln = find_lines(pat)
print("pattern", pat, "line", ln)
PY
python3 - <<'PY'
import pathlib, re
path = pathlib.Path("components/ambient-ui/src/adapters/mappers.ts")
lines = path.read_text().splitlines()
def show_around(needle, radius=80):
for i,l in enumerate(lines, start=1):
if re.search(needle, l):
start=max(1,i-radius)
end=min(len(lines), i+radius)
print(f"\n--- {needle} around line {i} ({start}-{end}) ---")
for j in range(start,end+1):
print(f"{j}\t{lines[j-1]}")
return
show_around(r"function\s+parseRepos\b")
show_around(r"function\s+parseReconciledRepos\b")
PY
# Show tests (relevant)
echo "=== resources-tab.test.tsx (relevant) ==="
python3 - <<'PY'
import pathlib
path = pathlib.Path("components/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/__tests__/resources-tab.test.tsx")
lines = path.read_text().splitlines()
# print whole file if <=200 lines else subset
print("line_count", len(lines))
limit=220
for i,l in enumerate(lines[:limit], start=1):
print(f"{i}\t{l}")
PYRepository: ambient-code/platform
Length of output: 18677
Normalize repo URLs when merging to avoid .git-suffix mismatches.
mergeRepos matches using raw url strings (Map/Set keys), but baseNameFromUrl() already strips a trailing .git for display, and parseRepos/parseReconciledRepos pass url through unchanged—so if one side includes .../repo.git and the other is .../repo, the reconciled data won’t join and you can render duplicate rows / wrong statuses.
♻️ Suggested normalization
+function normalizeUrl(url: string): string {
+ return url.replace(/\.git$/, '').replace(/\/$/, '')
+}
+
function mergeRepos(
repos: DomainRepo[],
reconciledRepos: DomainReconciledRepo[],
): MergedRepo[] {
const reconciledByUrl = new Map(
- reconciledRepos.map(r => [r.url, r]),
+ reconciledRepos.map(r => [normalizeUrl(r.url), r]),
)
const seen = new Set<string>()
const result: MergedRepo[] = []
for (const repo of repos) {
- seen.add(repo.url)
- const reconciled = reconciledByUrl.get(repo.url)
+ seen.add(normalizeUrl(repo.url))
+ const reconciled = reconciledByUrl.get(normalizeUrl(repo.url))
result.push({
url: repo.url,
name: reconciled?.name ?? repo.name ?? baseNameFromUrl(repo.url),
branch: reconciled?.currentActiveBranch ?? repo.branch ?? null,
status: reconciled?.status ?? null,
clonedAt: reconciled?.clonedAt ?? null,
})
}
for (const reconciled of reconciledRepos) {
- if (!seen.has(reconciled.url)) {
+ if (!seen.has(normalizeUrl(reconciled.url))) {
result.push({
url: reconciled.url,
name: reconciled.name ?? baseNameFromUrl(reconciled.url),
branch: reconciled.currentActiveBranch ?? null,
status: reconciled.status,
clonedAt: reconciled.clonedAt,
})
}
}
return result
}📝 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
| function mergeRepos( | |
| repos: DomainRepo[], | |
| reconciledRepos: DomainReconciledRepo[], | |
| ): MergedRepo[] { | |
| const reconciledByUrl = new Map( | |
| reconciledRepos.map(r => [r.url, r]), | |
| ) | |
| const seen = new Set<string>() | |
| const result: MergedRepo[] = [] | |
| for (const repo of repos) { | |
| seen.add(repo.url) | |
| const reconciled = reconciledByUrl.get(repo.url) | |
| result.push({ | |
| url: repo.url, | |
| name: reconciled?.name ?? repo.name ?? baseNameFromUrl(repo.url), | |
| branch: reconciled?.currentActiveBranch ?? repo.branch ?? null, | |
| status: reconciled?.status ?? null, | |
| clonedAt: reconciled?.clonedAt ?? null, | |
| }) | |
| } | |
| for (const reconciled of reconciledRepos) { | |
| if (!seen.has(reconciled.url)) { | |
| result.push({ | |
| url: reconciled.url, | |
| name: reconciled.name ?? baseNameFromUrl(reconciled.url), | |
| branch: reconciled.currentActiveBranch ?? null, | |
| status: reconciled.status, | |
| clonedAt: reconciled.clonedAt, | |
| }) | |
| } | |
| } | |
| return result | |
| } | |
| function normalizeUrl(url: string): string { | |
| return url.replace(/\.git$/, '').replace(/\/$/, '') | |
| } | |
| function mergeRepos( | |
| repos: DomainRepo[], | |
| reconciledRepos: DomainReconciledRepo[], | |
| ): MergedRepo[] { | |
| const reconciledByUrl = new Map( | |
| reconciledRepos.map(r => [normalizeUrl(r.url), r]), | |
| ) | |
| const seen = new Set<string>() | |
| const result: MergedRepo[] = [] | |
| for (const repo of repos) { | |
| seen.add(normalizeUrl(repo.url)) | |
| const reconciled = reconciledByUrl.get(normalizeUrl(repo.url)) | |
| result.push({ | |
| url: repo.url, | |
| name: reconciled?.name ?? repo.name ?? baseNameFromUrl(repo.url), | |
| branch: reconciled?.currentActiveBranch ?? repo.branch ?? null, | |
| status: reconciled?.status ?? null, | |
| clonedAt: reconciled?.clonedAt ?? null, | |
| }) | |
| } | |
| for (const reconciled of reconciledRepos) { | |
| if (!seen.has(normalizeUrl(reconciled.url))) { | |
| result.push({ | |
| url: reconciled.url, | |
| name: reconciled.name ?? baseNameFromUrl(reconciled.url), | |
| branch: reconciled.currentActiveBranch ?? null, | |
| status: reconciled.status, | |
| clonedAt: reconciled.clonedAt, | |
| }) | |
| } | |
| } | |
| return result | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/ambient-ui/src/app/`(dashboard)/[projectId]/sessions/[sessionId]/_components/resources-tab.tsx
around lines 32 - 68, mergeRepos is matching on raw url strings so
".../repo.git" vs ".../repo" won't join; normalize URLs (e.g., strip a trailing
".git" case-insensitively and optional trailing slash) before inserting into
reconciledByUrl Map and the seen Set and when checking matches. Concretely, add
a small normalization step (normalizeUrl) and use normalizedUrl as the Map key
and Set values while still using the original repo.url or reconciled.url for the
returned MergedRepo.url field and for name/branch/status lookups (use
reconciledByUrl.get(normalizedUrl) and seen.has(normalizedUrl)).
Comment on lines
24
to
27
| const [activeTab, setActiveTab] = useState(() => { | ||
| if (typeof window === 'undefined') return 'phase' | ||
| return new URL(window.location.href).searchParams.get('tab') ?? 'phase' | ||
| if (typeof window === 'undefined') return 'overview' | ||
| return new URL(window.location.href).searchParams.get('tab') ?? 'overview' | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="components/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/page.tsx"
echo "== file stats =="
wc -l "$FILE"
echo
echo "== top of file =="
sed -n '1,120p' "$FILE"
echo
echo "== relevant lines around state initializer =="
sed -n '15,60p' "$FILE"
echo
echo "== search for tab param usage in file =="
rg -n "tab|searchParams\.get\(['\"]tab['\"]\]|\\?tab=" "$FILE" || trueRepository: ambient-code/platform
Length of output: 5386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo search for ?tab= deep links =="
rg -n "\\?tab=" components/ambient-ui/src || true
echo
echo "== repo search for 'tab' query param plumbing =="
rg -n "searchParams\\.get\\(['\"]tab['\"]\\)|get\\(['\"]tab['\"]\\)|\\btab\\b.*searchParams|URLSearchParams\\(.*tab|tab=" components/ambient-ui/src || trueRepository: ambient-code/platform
Length of output: 580
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo-wide search for '?tab=' deep links =="
rg -n "\\?tab=" . || true
echo
echo "== repo-wide search for URLSearchParams('tab') usage =="
rg -n "URLSearchParams\\(|searchParams\\.set\\(['\"]tab['\"]\\)|searchParams\\.get\\(['\"]tab['\"]\\)" . || true
echo
echo "== repo-wide search for 'tab' query param routing =="
rg -n "tab['\"]\\s*:\\s*['\"]|params.*tab|query.*tab|\\btab\\b.*searchParams" . || trueRepository: ambient-code/platform
Length of output: 3834
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== search for explicit tab values in URLs =="
rg -n "tab=(overview|logs|resources|config|chat)" . || true
echo
echo "== search for 'sessions' links that include tab= =="
rg -n "sessions/[^\\s'\"\\)]*\\?tab=" . || true
echo
echo "== search for query param building for tab in frontend =="
rg -n "searchParams\\.set\\(['\"]tab['\"]\\)|URL\\(.+\\)\\..*searchParams.*tab" components/ambient-ui/src components/frontend/src || trueRepository: ambient-code/platform
Length of output: 520
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find Tabs component implementation
fd -a "tabs.tsx" components/ambient-ui/src || true
fd -a "tabs.*" components/ambient-ui/src/components || true
fd -a "components/ui/tabs" -t f . || true
echo "== Search for 'TabsContent' definition =="
rg -n "export (function|const) TabsContent|TabsContent" components/ambient-ui/src/components components/ambient-ui/src -S || true
echo "== Search for 'Tabs value=' usage =="
rg -n "TabsContent\\b|TabsTrigger\\b|Tabs\\b" components/ambient-ui/src/components/ui -S || trueRepository: ambient-code/platform
Length of output: 2296
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="components/ambient-ui/src/components/ui/tabs.tsx"
echo "== file stats =="
wc -l "$FILE"
echo
echo "== tabs.tsx content (numbered) lines 1-140 =="
sed -n '1,140p' "$FILE"Repository: ambient-code/platform
Length of output: 1989
Potential hydration mismatch from ?tab query param in session page
SessionDetailPage is a use client component, but its useState initializer returns 'overview' on the server and then reads window.location.searchParams.get('tab') on the client. If the URL is opened with ?tab= (e.g., ...?tab=logs), the first hydrated render can differ from the server HTML, triggering hydration warnings/content flash. Consider initializing via useSearchParams/router state or updating activeTab in a useEffect after mount. There’s no evidence in-repo that session URLs for this page are generated with ?tab= elsewhere—?tab is set only when users change tabs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/ambient-ui/src/app/`(dashboard)/[projectId]/sessions/[sessionId]/page.tsx
around lines 24 - 27, The useState initializer for activeTab (in
SessionDetailPage) reads window on the client but returns 'overview' on the
server, causing a hydration mismatch when the incoming URL has ?tab=...; change
to initialize activeTab to a stable server value (e.g., 'overview') and then
update it on mount: in a useEffect (or via next/navigation's useSearchParams in
a client hook) read the URL's search param and call setActiveTab to the real tab
value; update references to activeTab/setActiveTab only—do not access window
during initial render—to ensure server and client markup match and avoid
hydration warnings.
Contributor
Merge Queue Status
This pull request spent 1 minute 44 seconds in the queue, including 20 seconds running CI. Required conditions to merge |
4 tasks
mergify Bot
pushed a commit
that referenced
this pull request
Jun 3, 2026
## Summary Follow-up to #1638 addressing CodeRabbit review comments. - Guard JSON array parsers against null/primitive entries with `isRecord` type guard - Preserve `temperature=0` as a valid deterministic setting (only null `maxTokens`/`timeout` at zero) - Normalize repo URLs when merging config + reconciled repos to avoid `.git` suffix mismatches - Fix hydration mismatch: initialize tab state with stable server default, sync from URL `?tab=` param in `useEffect` ## Test plan - [ ] 323 vitest tests pass - [ ] `npm run build` passes with 0 errors, 0 warnings - [ ] Session with `temperature: 0` displays "0" in Config tab (not "—") - [ ] Repos with `.git` suffix match reconciled repos without suffix 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed LLM temperature configuration to preserve zero values instead of treating them as null. * **Improvements** * Repository URLs are now normalized, treating URLs with `.git` suffixes and trailing slashes as identical entries. * Enhanced session detail page tab selection logic for improved navigation consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Completes the session detail view and significantly improves the sessions table and navigation based on iterative UX critique from Steve Krug, Don Norman, and Refactoring UI perspectives.
Session Detail Tabs
Sessions Table
2h 03minstead of "about 2 hours")Navigation
Domain Model
DomainSessionwith repos, reconciledRepos, conditions, environmentVariables, labels, temperature, maxTokens, timeout, workflowId, prompt, sdkRestartCountuseAgentNameshook for agent ID to name resolutionTest plan
cd components/ambient-ui && npx vitest run)npm run buildpasses with 0 errors, 0 warnings🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
/sessionspath instead of/fleet.Refactor