Console Redesign Batch 2: Tenant Detail workbench, lifecycle dialogs, impersonation ceremony - #35
Conversation
Scopes Batch 2 to the Tenant Detail page, Suspend/Archive dialogs, and Impersonation ceremony; defers Audit Log and Plans editor reskins to Batch 3. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…orkbench) 12 tasks: backend audit-scoping + mandatory-reason changes (1-3), reusable frontend infra (4-6), Suspend/Archive/Impersonation dialogs (7-9), and the Tenant Detail page assembled section-by-section (10-12). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ction Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…enant-targeted PATCH /tenants/:id/subscription now requires a non-empty reason field (400 if absent/empty) and logs the admin action with target_type="tenant", target_id=<tenantID> instead of target_type="subscription", target_id=<sub.ID>, so subscription changes surface under Task 1's new ?target_id= audit filter. The reason is included in the audit changes payload alongside old/new. Also updates an existing fixture test (TestUpdateTenantSubscriptionCreatesWhenMissing) to include a reason field, since it now falls under the mandatory-reason validation.
…riptionRequiresPlanWhenMissing Add a reason field to this test's request body so it once again reaches and exercises the plan-id-required-on-create branch, instead of getting its 400 from the (now mandatory) reason check added in the previous commit.
Implements the useScrollSpy hook that tracks which section element is currently most visible in a scrolling container, driving an anchor rail's active-link highlight. Uses IntersectionObserver on the nearest <main> ancestor for precise scroll-aware section tracking. Includes full test coverage (2 tests) for default and intersection updates. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…lendar day groupAuditLogByDay's day key is a UTC YYYY-MM-DD slice. new Date(group.day) UTC-parses that into midnight UTC, but toLocaleDateString renders in the browser's local timezone — for viewers west of UTC (e.g. US Pacific/Eastern) this rolled the displayed heading back one day relative to the entries grouped under it. Parse the day key into explicit local Date components instead so the heading always matches the entries it groups.
Manual process.env.TZ save/restore coerced an originally-unset TZ to the literal string "undefined" on cleanup (Node treats assigning undefined to a process.env key as string coercion, not deletion), leaving TZ invalid for UTC fallback in later tests sharing the same vitest worker thread. Switch to vi.stubEnv/vi.unstubAllEnvs, which restores the pre-test state correctly including the originally-unset case.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… + Subscription sections
…sers section Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…Detail workbench complete Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
OrganizationDetail.tsx calls useScrollSpy() before its data load completes, so the target section elements don't exist in the DOM on first effect run. The hook silently gave up (elements.length === 0) and never retried, leaving activeId stuck at the default forever once the real sections appeared post-load. Poll via requestAnimationFrame until the sections exist before attaching the IntersectionObserver. Adds a regression test that mounts the hook against an empty <main>, appends the sections afterward, and asserts the observer attaches and activeId updates on intersection. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…loading Native browser hash-scroll fires once at initial navigation, before the async data fetch has rendered the target section (e.g. #activity from the impersonation exit-summary's "View activity log" link). Re-run the hash scroll once loading completes so the deep link actually lands. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
github-actions
Bot
added
documentation
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe PR adds tenant-scoped audit filtering and mandatory operation reasons, then introduces a sectioned super-admin tenant workbench with lifecycle dialogs, audit activity, user data, typed confirmations, and impersonation exit summaries. ChangesTenant detail workbench
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Admin as Super-admin
participant OrganizationDetail
participant Backend as Super-admin API
participant AuditLog as Tenant audit log
Admin->>OrganizationDetail: Open tenant detail
OrganizationDetail->>Backend: Fetch stats, plans, users, and audit entries
Backend->>AuditLog: Filter entries by tenant target_id
AuditLog-->>OrganizationDetail: Return tenant activity
Admin->>OrganizationDetail: Submit lifecycle or subscription reason
OrganizationDetail->>Backend: Send operation with reason
Backend->>AuditLog: Record tenant-targeted action
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 013a3f54a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }); | ||
| const entries: Array<{ created_at: string }> = data.logs ?? []; | ||
| const since = new Date(mintedAt).getTime(); | ||
| const relevant = entries.filter((e) => new Date(e.created_at).getTime() >= since); |
There was a problem hiding this comment.
When two super-admins have overlapping support sessions for the same tenant, this audit request returns every impersonated_request row for that tenant/action. The subsequent filter only checks created_at, so the exit dialog can count another operator's changes as part of the current operator's session; filter by the parked operator's user/admin ID as well, or have the backend return a session-scoped count.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 36d22b1: fetchImpersonationSummary now filters counted rows by the acting operator's own admin_user_id (read from the user object startImpersonation never touches), so an overlapping session from a different operator is no longer counted.
| const { data } = await axios.get(`${baseURL}/api/super-admin/audit-log`, { | ||
| headers: { Authorization: `Bearer ${operatorToken}` }, | ||
| params: { target_id: tenantId, action: 'impersonated_request' }, | ||
| }); |
There was a problem hiding this comment.
For support sessions with more than the audit endpoint's default page of mutating requests, this call only receives the first page from GetAuditLog (default limit is 50) and then counts relevant.length, so the exit summary underreports the number of changes. The summary needs to request/paginate all rows for the session or use a backend count that is filtered by session start.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 36d22b1: the function now pages through the audit log (newest-first) until it hits an entry older than mintedAt or exhausts total, instead of only inspecting the first page.
| const [tenantResponse, plansResponse, auditResponse, usersResponse] = await Promise.all([ | ||
| api.get(`/api/super-admin/tenants/${id}/stats`), | ||
| api.get('/api/super-admin/plans'), | ||
| api.get(`/api/super-admin/audit-log?target_id=${id}&limit=100`), |
There was a problem hiding this comment.
For tenants with more than 100 audit rows, this unfiltered tenant audit request returns only the first page and the backend caps limit at 100, then subscriptionAudit is filtered client-side from that truncated set. A tenant with enough recent lifecycle/impersonation activity can therefore show an empty or incomplete Subscription History even though older subscription changes exist; fetch an action-filtered/paginated subscription history instead of filtering the first unfiltered page.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed this is a real limitation, but leaving it as documented scope for now rather than a quick fix: the Subscription History card intentionally reuses the single tenant-wide limit=100 fetch (shared with the Activity section) rather than issuing a dedicated action-filtered query, per this batch's design spec. A tenant with >100 total audit rows across ALL actions (not just subscription changes) could see a truncated history. Tracking as backlog rather than fixing now since a correct fix needs either server-side multi-action filtering or a second paginated request, and I'd rather not rush that into a shared fetch path without its own review pass.
| api.get(`/api/super-admin/tenants/${id}/stats`), | ||
| api.get('/api/super-admin/plans'), | ||
| api.get(`/api/super-admin/audit-log?target_id=${id}&limit=100`), | ||
| api.get(`/api/super-admin/users?tenant_id=${id}`), |
There was a problem hiding this comment.
For tenants with more than 50 users, this call relies on GetAllUsersSuper's default page_size=50, and the page ignores the returned total with no pagination controls. The Users section will silently present only the first page as if it were the complete tenant user list; request/paginate all pages or render this as a paginated table.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same reasoning as the sibling comment on this endpoint: real limitation (>50 users in one tenant would only show the first page), left as documented backlog rather than fixed now — GetAllUsersSuper already supports page/page_size, so wiring pagination into this tab is a scoped, low-risk follow-up, just not done in this pass.
| api.get(`/api/super-admin/tenants/${id}/stats`), | ||
| api.get('/api/super-admin/plans'), | ||
| api.get(`/api/super-admin/audit-log?target_id=${id}&limit=100`), | ||
| api.get(`/api/super-admin/users?tenant_id=${id}`), |
There was a problem hiding this comment.
When a user belongs to multiple tenants with different membership roles, this endpoint returns the global users.role selected in backend/internal/store/pg_store_super_admin.go, not the per-tenant user_tenants.role that the migration documents as the role within a specific organization. Using it for the tenant-scoped Users section can therefore show the wrong role (and account creation date instead of membership join date); fetch tenant membership data or extend the endpoint to return ut.role for the requested tenant.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed real — GetAllUsers's query selects u.role (the global role) even when tenant_id scopes the result, not ut.role (the per-tenant membership role from user_tenants), so a user with different roles across tenants would show the wrong one here. Deliberately NOT fixing this in the same pass as the other findings: the correct fix needs the JOIN itself scoped to the filtered tenant (LEFT JOIN user_tenants ut ON u.id = ut.user_id AND ut.tenant_id = $N) so the pre-existing cross-tenant /super-admin/users page (which uses this same function with no tenant filter, and depends on the current unscoped JOIN + SELECT DISTINCT to stay one-row-per-user) doesn't regress. That's a real query-correctness change to a shared, security-relevant endpoint and deserves its own focused change + review rather than being bundled in here. Filed as backlog.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/pages/super-admin/OrganizationDetail.tsx (1)
83-115: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSingle Promise.all failure blocks the whole page, even for non-critical data.
the implementation builds
countQueryusing the dynamically generatedwhereclause and executes it with the filterargs...aside — focusing on the frontend:loadDataawaitsstats,plans,audit-log, andusersin onePromise.all(Lines 85-90). If the audit-log or users endpoint fails transiently, the catch block never setstenant, so the whole page shows "Organization not found" (Line 228) even though the core tenant/subscription data would have loaded fine — blocking subscription/lifecycle management for an unrelated, non-critical failure.Consider
Promise.allSettled, or fetching audit-log/users independently with per-section error states, so a single non-critical failure doesn't take down the whole workbench.♻️ Suggested direction
- const [tenantResponse, plansResponse, auditResponse, usersResponse] = await Promise.all([ - api.get(`/api/super-admin/tenants/${id}/stats`), - api.get('/api/super-admin/plans'), - api.get(`/api/super-admin/audit-log?target_id=${id}&limit=100`), - api.get(`/api/super-admin/users?tenant_id=${id}`), - ]); - - setTenant(tenantResponse.data); - setPlans(plansResponse.data); - setAuditEntries(auditResponse.data.logs || []); - setUsers(usersResponse.data.users || []); + const [statsResult, plansResult, auditResult, usersResult] = await Promise.allSettled([ + api.get(`/api/super-admin/tenants/${id}/stats`), + api.get('/api/super-admin/plans'), + api.get(`/api/super-admin/audit-log?target_id=${id}&limit=100`), + api.get(`/api/super-admin/users?tenant_id=${id}`), + ]); + + if (statsResult.status === 'fulfilled') setTenant(statsResult.value.data); + if (plansResult.status === 'fulfilled') setPlans(plansResult.value.data); + if (auditResult.status === 'fulfilled') setAuditEntries(auditResult.value.data.logs || []); + if (usersResult.status === 'fulfilled') setUsers(usersResult.value.data.users || []);🤖 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 `@web/src/pages/super-admin/OrganizationDetail.tsx` around lines 83 - 115, Update loadData so the critical tenant stats and plans responses remain usable when audit-log or users requests fail independently. Replace the single-failure Promise.all handling with all-settled or equivalent per-request handling, always apply successful tenant/subscription data, and default or separately handle failed audit and users sections without triggering the page-level failure state.
🧹 Nitpick comments (3)
docs/superpowers/plans/2026-07-11-console-redesign-batch2.md (1)
204-207: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a store-level regression test for the generated SQL.
The handler tests only verify that
target_idreaches the filter map; they cannot detect an incorrect/missingWHERE target_id = ...clause inPGStore.GetAuditLog. Add a SQL-mocking or integration test before treating this filter as covered.🤖 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 `@docs/superpowers/plans/2026-07-11-console-redesign-batch2.md` around lines 204 - 207, Update Step 6 to require a store-level regression test for PGStore.GetAuditLog that verifies the generated SQL includes the target_id filter and its bound value, using the repository’s existing SQL-mocking or integration-test conventions. Include this test in the backend test command or explicitly run it before marking the backend gate complete; do not rely solely on the handler filter-passthrough assertion.web/src/pages/super-admin/OrganizationDetail.tsx (1)
148-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated error-unwrap-and-toast pattern.
The same
error as { response?: { data?: { error?: string } } }cast plus fallback-toast logic is duplicated acrossrunSuspend,runArchive,runReactivate, andimpersonate. A small helper (e.g.showApiError(error, fallbackKey)) would remove the duplication and centralize the unchecked type assertion.♻️ Suggested helper
function apiErrorMessage(error: unknown, fallback: string): string { const err = error as { response?: { data?: { error?: string } } }; return err.response?.data?.error || fallback; }🤖 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 `@web/src/pages/super-admin/OrganizationDetail.tsx` around lines 148 - 208, Extract the repeated API error extraction into a shared helper near the component, such as apiErrorMessage(error, fallback), that accepts unknown errors and returns the nested response message or fallback. Replace the duplicated casts and fallback logic in runSuspend, runArchive, runReactivate, and impersonate with this helper while preserving each handler’s existing translation key.web/src/components/ArchiveSheet.tsx (1)
23-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting shared lifecycle-confirmation logic.
ArchiveSheetandSuspendTenantDialogshare the same props interface,useTypedConfirmGateusage,close/reset pattern, optional reason textarea, and type-to-confirm block. Extracting a shareduseLifecycleConfirmhook or a sharedLifecycleConfirmBodycomponent would reduce duplication and keep the acknowledgement/checkbox logic in sync.This is deferable given the Sheet-vs-Dialog and 2-vs-1 checkbox differences, but worth tracking as the lifecycle dialog family grows.
🤖 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 `@web/src/components/ArchiveSheet.tsx` around lines 23 - 87, The shared lifecycle-confirmation flow is duplicated between ArchiveSheet and SuspendTenantDialog. Extract the common props, useTypedConfirmGate integration, close/reset behavior, optional reason handling, and type-to-confirm UI into a reusable hook or LifecycleConfirmBody component, while keeping the Sheet-vs-Dialog wrappers and archive-specific two-acknowledgement logic separate.
🤖 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 `@backend/internal/handler/super_admin.go`:
- Around line 463-467: Update the request-binding flow around c.Bind(&body) to
capture and handle its error before validating body.Reason. Return the existing
invalid-request response when binding fails, then retain the
strings.TrimSpace(body.Reason) required check for successfully bound requests;
remove the errcheck suppression.
In `@docs/superpowers/plans/2026-07-11-console-redesign-batch2.md`:
- Around line 2035-2065: Update the tenant detail loading flow around useEffect
and loadData so an in-flight request for a previous id cannot update the current
tenant’s state. Abort the prior request or track the request/tenant generation,
and verify it is still current before applying tenant, plans, audit, and
subscription state or showing load errors.
- Around line 552-559: Update the IntersectionObserver callback that calls
setActiveId so active-section selection is deterministic: do not rely on
visible[0] or only the changed entries; track the observed section
positions/state and select the section with the greatest visibility (using a
stable tie-breaker such as document order). Preserve the existing root and
rootMargin configuration.
- Around line 2033-2038: Update the component’s post-load effect around
activeSection and loadData to handle an existing URL hash after async sections
render, scrolling to the matching section once data is available, and
synchronize the URL hash as activeSection changes during scrolling. Apply the
same behavior to the corresponding logic at the additional referenced location,
preserving the existing styling use of activeSection.
- Around line 159-185: The GetAuditLog implementation must check rows.Err()
after the rows.Next() loop and return any iteration failure instead of a partial
success. Wrap query, scan, and rows.Err() failures with clear store-operation
context while preserving the existing result and JSON unmarshalling behavior.
- Around line 1700-1714: Update fetchImpersonationSummary to avoid relying on
the audit-log endpoint’s default single page: request successive pages using the
response total and offset (or use the server-side summary contract), and filter
entries by tenant, impersonated_request action, current operator identity, and
mintedAt before counting. Preserve the existing durationMinutes calculation
while ensuring actionCount includes every qualifying row.
In `@docs/superpowers/specs/2026-07-11-console-redesign-batch2-design.md`:
- Line 56: Update the subscription change-feed contract around the GET
/audit-log action filter so comma-separated update_subscription and
create_subscription values return matching records. Prefer implementing
multi-valued action support in the handler/store while preserving existing
single-action behavior; otherwise document two requests or equivalent
client-side filtering instead of treating the combined value as one exact
action.
In `@web/src/components/AuditEntryList.tsx`:
- Around line 41-46: Update the action label rendering in AuditEntryList’s Badge
to use the documented auditAction_<action> translation key through the existing
localization mechanism, rather than formatting entry.action with replace.
Preserve the current badge class and variant selection, and ensure custom action
labels resolve through translations with an appropriate fallback if no
translation exists.
- Around line 31-36: Update the date handling between groupAuditLogByDay and the
AuditEntryList day heading so grouping and row timestamps use the same timezone.
Prefer changing groupAuditLogByDay to bucket entries by the operator’s local
calendar day, preserving the existing localDate formatting in the heading and
timestamp rendering; alternatively, consistently render both using UTC.
In `@web/src/components/ImpersonationBanner.tsx`:
- Around line 44-54: Update startExit in ImpersonationBanner to clear the
existing summary before beginning each impersonation summary fetch, including
when no operator token is available. Preserve the current fetch, success, and
failure behavior so stale data is not displayed during subsequent exit attempts.
In `@web/src/lib/impersonationSummary.ts`:
- Around line 14-29: Add a finite timeout to the axios request in
fetchImpersonationSummary so a hanging audit-log endpoint rejects promptly and
allows the caller’s unavailable-summary fallback to run. Keep the existing URL,
headers, parameters, and response processing unchanged.
---
Outside diff comments:
In `@web/src/pages/super-admin/OrganizationDetail.tsx`:
- Around line 83-115: Update loadData so the critical tenant stats and plans
responses remain usable when audit-log or users requests fail independently.
Replace the single-failure Promise.all handling with all-settled or equivalent
per-request handling, always apply successful tenant/subscription data, and
default or separately handle failed audit and users sections without triggering
the page-level failure state.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-07-11-console-redesign-batch2.md`:
- Around line 204-207: Update Step 6 to require a store-level regression test
for PGStore.GetAuditLog that verifies the generated SQL includes the target_id
filter and its bound value, using the repository’s existing SQL-mocking or
integration-test conventions. Include this test in the backend test command or
explicitly run it before marking the backend gate complete; do not rely solely
on the handler filter-passthrough assertion.
In `@web/src/components/ArchiveSheet.tsx`:
- Around line 23-87: The shared lifecycle-confirmation flow is duplicated
between ArchiveSheet and SuspendTenantDialog. Extract the common props,
useTypedConfirmGate integration, close/reset behavior, optional reason handling,
and type-to-confirm UI into a reusable hook or LifecycleConfirmBody component,
while keeping the Sheet-vs-Dialog wrappers and archive-specific
two-acknowledgement logic separate.
In `@web/src/pages/super-admin/OrganizationDetail.tsx`:
- Around line 148-208: Extract the repeated API error extraction into a shared
helper near the component, such as apiErrorMessage(error, fallback), that
accepts unknown errors and returns the nested response message or fallback.
Replace the duplicated casts and fallback logic in runSuspend, runArchive,
runReactivate, and impersonate with this helper while preserving each handler’s
existing translation key.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d048a732-7de7-4c53-9213-d1d87dcf5823
📒 Files selected for processing (31)
backend/internal/handler/super_admin.gobackend/internal/handler/super_admin_impersonation_test.gobackend/internal/handler/super_admin_subscription_test.gobackend/internal/handler/super_admin_test.gobackend/internal/store/pg_store.godocs/superpowers/plans/2026-07-11-console-redesign-batch2.mddocs/superpowers/specs/2026-07-11-console-redesign-batch2-design.mdweb/src/components/ArchiveSheet.tsxweb/src/components/AuditEntryList.tsxweb/src/components/ImpersonateDialog.tsxweb/src/components/ImpersonationBanner.tsxweb/src/components/SuspendTenantDialog.tsxweb/src/components/TenantIdentityHeader.tsxweb/src/components/__tests__/ArchiveSheet.test.tsxweb/src/components/__tests__/AuditEntryList.test.tsxweb/src/components/__tests__/ImpersonateDialog.test.tsxweb/src/components/__tests__/ImpersonationBanner.test.tsxweb/src/components/__tests__/SuspendTenantDialog.test.tsxweb/src/components/__tests__/TenantIdentityHeader.test.tsxweb/src/hooks/__tests__/useScrollSpy.test.tsweb/src/hooks/__tests__/useTypedConfirmGate.test.tsweb/src/hooks/useScrollSpy.tsweb/src/hooks/useTypedConfirmGate.tsweb/src/i18n.tsweb/src/lib/__tests__/auditFormat.test.tsweb/src/lib/__tests__/impersonation.test.tsweb/src/lib/auditFormat.tsweb/src/lib/impersonation.tsweb/src/lib/impersonationSummary.tsweb/src/pages/super-admin/OrganizationDetail.tsxweb/src/pages/super-admin/__tests__/OrganizationDetail.test.tsx
| export async function fetchImpersonationSummary( | ||
| tenantId: string, | ||
| mintedAt: string, | ||
| operatorToken: string | ||
| ): Promise<ImpersonationSummary> { | ||
| const baseURL = import.meta.env.VITE_API_URL || 'http://localhost:8008'; | ||
| const { data } = await axios.get(`${baseURL}/api/super-admin/audit-log`, { | ||
| headers: { Authorization: `Bearer ${operatorToken}` }, | ||
| params: { target_id: tenantId, action: 'impersonated_request' }, | ||
| }); | ||
| const entries: Array<{ created_at: string }> = data.logs ?? []; | ||
| const since = new Date(mintedAt).getTime(); | ||
| const relevant = entries.filter((e) => new Date(e.created_at).getTime() >= since); | ||
| const durationMinutes = Math.max(0, Math.round((Date.now() - since) / 60000)); | ||
| return { durationMinutes, actionCount: relevant.length }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target document and inspect the relevant range.
wc -l docs/superpowers/plans/2026-07-11-console-redesign-batch2.md
sed -n '1670,1735p' docs/superpowers/plans/2026-07-11-console-redesign-batch2.md
# Find any implementation or references to the function name.
rg -n "fetchImpersonationSummary|impersonation summary|audit-log|impersonated_request" -S .Repository: thevladbog/idento
Length of output: 16675
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the plan section around the snippet and any nearby notes about the intended contract.
sed -n '1660,1755p' docs/superpowers/plans/2026-07-11-console-redesign-batch2.md
# Search for actual code definitions and usage sites.
rg -n "function fetchImpersonationSummary|fetchImpersonationSummary\(|audit-log|target_id: tenantId|action: 'impersonated_request'|total" -S . --glob '!docs/superpowers/plans/2026-07-11-console-redesign-batch2.md'
# Show concise file list for anything matching the function name or related audit log handling.
fd -a ".*(impersonation|audit-log|super-admin).*" .Repository: thevladbog/idento
Length of output: 23447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect backend audit-log shape and filters.
sed -n '300,390p' backend/internal/handler/super_admin.go
sed -n '1640,1695p' backend/internal/store/pg_store.go
# Inspect impersonation audit logging payload to see whether operator identity is encoded.
sed -n '1,120p' backend/internal/middleware/impersonation_audit.go
# Inspect the impersonation summary implementation in the plan and the UI consumer.
sed -n '1696,1715p' docs/superpowers/plans/2026-07-11-console-redesign-batch2.md
sed -n '46,62p' web/src/components/ImpersonationBanner.tsxRepository: thevladbog/idento
Length of output: 7152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Audit-log request handling and persistence filters.
cat -n backend/internal/handler/super_admin.go | sed -n '330,380p'
echo '---'
cat -n backend/internal/store/pg_store.go | sed -n '1620,1695p'
echo '---'
cat -n backend/internal/middleware/impersonation_audit.go | sed -n '1,120p'
echo '---'
cat -n web/src/components/ImpersonationBanner.tsx | sed -n '1,120p'Repository: thevladbog/idento
Length of output: 9733
Do not compute the exit summary from a single audit-log page. This still uses the default 50-row page and only filters by tenant/action, so longer sessions will undercount and other operators’ impersonated_request rows can be included. Page through total/offset and count only the current operator’s rows, or move this behind a server-side summary contract.
🤖 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 `@docs/superpowers/plans/2026-07-11-console-redesign-batch2.md` around lines
1700 - 1714, Update fetchImpersonationSummary to avoid relying on the audit-log
endpoint’s default single page: request successive pages using the response
total and offset (or use the server-side summary contract), and filter entries
by tenant, impersonated_request action, current operator identity, and mintedAt
before counting. Preserve the existing durationMinutes calculation while
ensuring actionCount includes every qualifying row.
There was a problem hiding this comment.
Fixed in 36d22b1 — see the reply on the corresponding impersonationSummary.ts comment (same finding, this one's against the plan document's embedded snippet rather than the shipped source).
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git log --oneline -5 36d22b1 2>/dev/null || true
git show 36d22b1 --stat 2>/dev/null || true
git show 36d22b1 -- web/src/lib/impersonationSummary.ts 2>/dev/null || true
git show 36d22b1 -- docs/superpowers/plans/2026-07-11-console-redesign-batch2.md 2>/dev/null || trueLength of output: 7228
The fix confirms what was needed: fetchImpersonationSummary now pages through the audit log via limit/offset until it reaches an entry older than mintedAt or exhausts total, and scopes actionCount to the acting operator's own admin_user_id. This addresses both concerns raised (single-page undercounting and cross-operator contamination) in the shipped source, and since this plan-document comment was against the same embedded snippet, it's resolved as well.
| const activeSection = useScrollSpy(SECTIONS.map((s) => s.id)); | ||
|
|
||
| useEffect(() => { | ||
| loadData(); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps -- load when id changes | ||
| }, [id]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore and synchronize hash navigation after async loading.
The anchor links set a hash only when clicked, while activeSection is used only for styling. There is no post-load handling for an existing #activity hash or synchronization when scrolling, so the exit-summary deep link can land at the top before sections exist.
Also applies to: 2124-2137
🤖 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 `@docs/superpowers/plans/2026-07-11-console-redesign-batch2.md` around lines
2033 - 2038, Update the component’s post-load effect around activeSection and
loadData to handle an existing URL hash after async sections render, scrolling
to the matching section once data is available, and synchronize the URL hash as
activeSection changes during scrolling. Apply the same behavior to the
corresponding logic at the additional referenced location, preserving the
existing styling use of activeSection.
There was a problem hiding this comment.
Half of this is already fixed (013a3f5, before this review): the exit-summary's #activity deep link now correctly scrolls to the section once data finishes loading, verified live. The other half — synchronizing the URL hash to activeSection as the user scrolls with the mouse wheel (not just via an anchor-rail click, which already updates the hash natively) — is a real gap against this batch's own design intent, but a nice-to-have polish item rather than a functional bug: no interaction is broken today, clicking an anchor link or landing via a deep link both work correctly. Deferring the live two-way sync as backlog.
There was a problem hiding this comment.
@thevladbog, that's a fair split — thanks for the clarification. Good to know the #activity deep-link-after-async-load path is already covered by 013a3f5. Agreed the live hash-sync-while-scrolling is polish rather than a functional bug, since both the anchor-click and deep-link paths work correctly today.
Would you like me to open a follow-up issue to track the two-way scroll-to-hash sync as backlog, so it doesn't get lost?
- ImpersonateTenant: handle c.Bind error before validating reason, instead of silently swallowing malformed JSON - GetAuditLog: check rows.Err() after iteration and wrap query/scan errors with context, instead of returning an incomplete result as success - fetchImpersonationSummary: scope the exit-summary action count to the acting operator's own admin_user_id (not all operators sharing an overlapping session), and page through the audit log instead of only inspecting the first page - ImpersonationBanner: clear the previous summary before each new exit attempt, so a stale duration/count can't flash before the fresh fetch resolves - groupAuditLogByDay: bucket by the viewer's local calendar day instead of the UTC date slice, so entries near midnight UTC no longer land under a day heading that disagrees with their own displayed local time - OrganizationDetail: guard loadData against a slow response for a previously-viewed tenant overwriting a newer tenant's state after a fast navigation between two tenant detail pages Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
Replaces the plain-Card Tenant Detail page (
OrganizationDetail.tsx) with the design brief's tenant workbench: a persistent identity header, five stacked sections behind a sticky anchor rail with scroll-spy, a lifecycle state timeline with a checkbox-gated Suspend modal / Archive side-sheet, and a ceremonial impersonation flow (mandatory-reason entry, unchanged amber banner, new exit summary with duration + action count and a deep link back into Activity).target_idfilter on the audit-log endpoint; mandatoryreason+ tenant-targeted audit logging on subscription updates; mandatoryreasonon impersonation entry — all enforced server-side, not just client-side.useScrollSpy,AuditEntryList+ day-grouping/diff-formatting utilities (reused unmodified across two call sites, and intended for Batch 3's Audit Log page),TenantIdentityHeader+useTypedConfirmGate(extracted fromConfirmActionDialog's fail-closed logic without ever modifying that file).SuspendTenantDialog,ArchiveSheet,ImpersonateDialog— each composing the shared typed-confirm gate rather than duplicating it.OrganizationDetail.tsxrebuilt section-by-section (Summary+Subscription → Lifecycle+Users → Activity+impersonation wiring).Executed via subagent-driven-development: 12 tasks, each with a fresh implementer + independent task review (spec compliance + code quality), 2 fix-and-re-review loops during implementation, plus a live-browser click-through that found and fixed 2 additional bugs (a
useScrollSpyloading-gate race, and a matching URL-hash-scroll race on the impersonation exit summary's deep link) — both verified live after landing. A final whole-branch review (opus) confirmed cross-task integration is sound and returned Ready to merge: Yes, with 6 non-blocking Minor findings recorded as backlog (see below).Design spec:
docs/superpowers/specs/2026-07-11-console-redesign-batch2-design.mdImplementation plan:
docs/superpowers/plans/2026-07-11-console-redesign-batch2.mdOut of scope (deferred to Batch 3)
Audit Log page reskin, Plans editor reskin — independent screens, no code overlap with this work.
Known gaps (documented, not fixed)
formatAuditDiff) is English-only — consistent with the design brief's explicit EN-first-console allowance.Minor backlog from final review (non-blocking)
useScrollSpypolls viarequestAnimationFrameindefinitely on the tenant-not-found/error terminal screens (harmless, low-cost, no sections ever mount there).useScrollSpy's retry-until-found pattern — both verified working, just inconsistent.admin_user_id(counts allimpersonated_requestrows for the tenant since mint time) — a real drift from the spec's documented design, matters only with overlapping multi-operator sessions.auditAction_<action>i18n key family was never added; badge labels render raw action strings — effectively covered by the accepted EN-first audit-copy gap.Test plan
go build ./... && go vet ./... && go test ./...— 146 tests passnpx tsc -b --noEmit && npx eslint . && npx vitest run— 70 tests pass, 0 type/lint errorsConfirmActionDialogflow); Impersonate (mandatory reason → banner → real mutating action logged → exit summary with real duration/count → "View activity log" deep link scrolls correctly); light/dark mode; EN/RU locale toggle🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests