Skip to content

feat(settings): surface approval history audit trail#2949

Merged
senamakel merged 7 commits into
tinyhumansai:mainfrom
obchain:feat/approval-history-panel
May 30, 2026
Merged

feat(settings): surface approval history audit trail#2949
senamakel merged 7 commits into
tinyhumansai:mainfrom
obchain:feat/approval-history-panel

Conversation

@obchain
Copy link
Copy Markdown
Contributor

@obchain obchain commented May 29, 2026

Summary

  • Adds a read-only Approval history panel under Settings → Agent access that lists decided tool-approval requests (tool, scrubbed summary, decision badge, decided-at), with Refresh and a stale-response guard.
  • Wires the merged-but-uncalled backend read path openhuman.approval_list_recent_decisions (shipped in Add approval audit history read path #2335) into the frontend — it had zero callers before this PR.
  • Adds approvalApi.ts with an unwrapRows normalizer that handles both wire shapes the gate produces ({result, logs} envelope when installed vs bare [] when absent).
  • Routes + breadcrumbs + a link from AgentAccessPanel; also adds the previously-missing agent-access route to useSettingsNavigation (it was navigated to but absent from the route union / mapping).
  • i18n keys across all locale chunks, an about_app capability entry (security.approval_history), and a coverage-matrix row.

Problem

Under the supervised access tier every external-effect tool call is gated, and each Approve / Deny is persisted as a durable, PII-scrubbed ApprovalAuditEntry. The read path for that history (approval_list_recent_decisions) merged in #2335 but never got a UI — so the audit trail was effectively write-only and the shipped backend was dark. The only approval UI today is the in-chat ApprovalRequestCard, which handles one live prompt and shows no history.

Solution

  • app/src/services/api/approvalApi.ts — typed ApprovalAuditEntry / PendingApproval mirroring the Rust serde output, plus fetchRecentApprovalDecisions / fetchPendingApprovals. unwrapRows normalizes the CLI-compatible RpcOutcome envelope: the RPCs attach a diagnostic log when the gate is installed ({result, logs}) and return a bare array when it is not.
  • ApprovalHistoryPanel.tsx — loading / empty / error / loaded states; one row per decided request with a decision badge (Approved once / Always allowed / Denied); Refresh control; monotonic loadSeqRef guard so a slow response can't clobber a fresher one; localized error text only (no raw backend strings).
  • NavSettings.tsx route, useSettingsNavigation union + getCurrentRoute + breadcrumbs (also closes the pre-existing agent-access gap), and a "View approval history" link in AgentAccessPanel.
  • Catalog/docsabout_app capability security.approval_history; coverage-matrix row 13.1.4.

No change to approval decision semantics — decisions still flow through openhuman.approval_decide.

Verification

  • pnpm typecheck — clean.
  • pnpm exec eslint on all touched files — 0 errors / 0 warnings.
  • pnpm i18n:check — parity clean (0 missing / extra / drifted across all locales).
  • cargo check — clean (catalog.rs).
  • cargo fmt --check — clean.
  • Vitest: 12 new tests pass (7 API client incl. envelope normalization, 5 panel states); existing AgentAccessPanel suite (9) still green.

Submission Checklist

If a section does not apply to this change, mark the item as N/A with a one-line reason. Do not delete items.

  • Tests added or updated (happy path + at least one failure / edge case) per Testing Strategy — panel error/empty states + API envelope-normalization edge cases.
  • Diff coverage >= 80% — changed lines (Vitest + cargo-llvm-cov merged via diff-cover) meet the gate enforced by .github/workflows/coverage.yml. New panel + client lines are exercised by the new tests; the Rust change is a single static catalog entry.
  • Coverage matrix updated — added row 13.1.4 in docs/TEST-COVERAGE-MATRIX.md.
  • All affected feature IDs from the matrix are listed in the PR description under ## Related — 13.1.4.
  • No new external network dependencies introduced (mock backend used per Testing Strategy) — pure in-process RPC; tests mock callCoreRpc.
  • Manual smoke checklist updated if this touches release-cut surfaces (docs/RELEASE-MANUAL-SMOKE.md) — N/A: additive read-only Settings panel, no release-cut surface.
  • Linked issue closed via Closes #NNN in the ## Related section — Closes Surface approval history (audit trail) in Settings #2947.

Impact

  • Runtime/platform: desktop (macOS/Windows/Linux). React-only feature over an in-process RPC; no mobile/web/CLI change.
  • Performance: one list RPC on panel mount + manual refresh; negligible.
  • Security: read-only. Summaries are PII-scrubbed and arguments redacted to shape at the source (Add approval audit history read path #2335); the panel renders no chat content.
  • Compatibility: pure addition. When the gate is disabled the RPC returns an empty list and the panel shows the empty state.

Related


AI Authored PR Metadata (required for Codex/Linear PRs)

Keep this section for AI-authored PRs. For human-only PRs, mark each field N/A.

Linear Issue

  • Key: N/A
  • URL: N/A

Commit & Branch

  • Branch: feat/approval-history-panel
  • Commit SHA: 1fd0e23708fc826451d0362a811f0d8a9203b865

Validation Run

  • pnpm --filter openhuman-app format:check — Prettier clean on touched files.
  • pnpm typecheck — clean.
  • Focused tests: vitest run approvalApi.test.ts ApprovalHistoryPanel.test.tsx → 12 passed; AgentAccessPanel.test.tsx → 9 passed.
  • Rust fmt/check (if changed): cargo check + cargo fmt --check clean on about_app/catalog.rs.
  • Tauri fmt/check (if changed): N/A — no app/src-tauri/ files changed.

Validation Blocked

  • command: N/A
  • error: N/A
  • impact: N/A

Behavior Changes

  • Intended behavior change: adds a read-only approval-history surface; no change to gating or decision flow.
  • User-visible effect: Settings → Agent access → View approval history lists past Approve/Deny decisions.

Parity Contract

  • Legacy behavior preserved: approval_decide and the in-chat ApprovalRequestCard are unchanged; this only reads existing audit rows.
  • Guard/fallback/dispatch parity checks: unwrapRows tolerates both the gate-installed {result, logs} envelope and the gate-absent bare array; degraded responses collapse to an empty list rather than throwing.

Duplicate / Superseded PR Handling

  • Duplicate PR(s): none.
  • Canonical PR: this one.
  • Resolution (closed/superseded/updated): N/A.

Summary by CodeRabbit

  • New Features

    • Added "Approval History" view in Agent Access settings to display recent approve/deny decisions
    • Users can refresh approval history records to see the latest updates
    • Added navigation link from Agent Access to the Approval History view and updated breadcrumbs to show a deeper trail when viewing history
  • Internationalization

    • Added translation support for approval history UI across 15 languages
  • Tests

    • Added comprehensive tests for the Approval History panel and API behavior

Review Change Stack

Add a read-only Approval history panel under Settings → Agent access
that lists decided tool-approval requests via the existing
approval_list_recent_decisions read path (merged in tinyhumansai#2335), which had
no frontend caller. Covers loading/empty/error/loaded states, a
per-decision badge, refresh with a stale-response guard, and the
bare-array vs {result,logs} envelope normalization.

Closes tinyhumansai#2947
@obchain obchain requested a review from a team May 29, 2026 14:08
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 29, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8685ca1d-602b-40a2-a537-7ac0f6066213

📥 Commits

Reviewing files that changed from the base of the PR and between 3010878 and ec28a55.

📒 Files selected for processing (1)
  • app/src/components/settings/panels/__tests__/ApprovalHistoryPanel.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/src/components/settings/panels/tests/ApprovalHistoryPanel.test.tsx

📝 Walkthrough

Walkthrough

Adds a read-only Approval History panel under Settings → Agent access: a new RPC client and types, an ApprovalHistoryPanel UI with loading/error/empty/refresh handling and stale-response guard, navigation route/breadcrumb wiring, locale translations, and tests.

Changes

Approval History Feature

Layer / File(s) Summary
Approval API types and RPC client
app/src/services/api/approvalApi.ts, app/src/services/api/approvalApi.test.ts
Defines ApprovalDecision, PendingApproval, and ApprovalAuditEntry; implements unwrapRows<T> normalization; exports DEFAULT_APPROVAL_HISTORY_LIMIT, fetchRecentApprovalDecisions(limit?), and fetchPendingApprovals(); includes unit tests for normalization and RPC parameter usage.
Settings navigation and breadcrumbs
app/src/components/settings/hooks/useSettingsNavigation.ts
Extends SettingsRoute with 'approval-history', adds pathname detection for /settings/approval-history, introduces agentAccessCrumb, and returns breadcrumbs including [Settings, Agents, Agent access] for the new route.
Approval history panel component and tests
app/src/components/settings/panels/ApprovalHistoryPanel.tsx, app/src/components/settings/panels/__tests__/ApprovalHistoryPanel.test.tsx
React panel with loading/error/empty/loaded states, monotonic loadSeqRef guard, formatDateTime, decision→tone/label mappings, refresh control, and Vitest + RTL tests covering rows, badges, empty/error states, and refresh behavior.
Agent access panel approval history link
app/src/components/settings/panels/AgentAccessPanel.tsx
Destructures navigateToSettings from useSettingsNavigation and adds an "Approval history" section with a button that calls navigateToSettings('approval-history') (data-testid="agent-access-approval-history-link").
Settings route registration
app/src/pages/Settings.tsx
Imports ApprovalHistoryPanel and registers a <Route path="approval-history"> that renders the panel inside wrapSettingsPage.
Internationalization across locales
app/src/lib/i18n/{en,ar,bn,de,es,fr,hi,id,it,ko,pl,pt,ru,zh-CN}.ts
Adds settings.agentAccess.approvalHistory* and settings.approvalHistory.* keys across English and multiple locale files covering page titles/subtitles, descriptive text, refresh/loading/retry/empty/error strings, decided-at label, and decision outcome labels (approve once, always allowed, deny).
Test coverage documentation
docs/TEST-COVERAGE-MATRIX.md
Registers feature 13.1.5 "Approval History" under Account & Security with ✅ Covered and documents the panel and API test paths and response-shape normalization notes.

Sequence Diagram

sequenceDiagram
  participant User
  participant ApprovalHistoryPanel as Panel
  participant Navigation as useSettingsNavigation
  participant API as fetchRecentApprovalDecisions
  participant RPC as callCoreRpc
  User->>Navigation: Navigate to approval-history
  Navigation->>Panel: Render with breadcrumbs
  Panel->>Panel: Mount → loadSeqRef.current=1, setIsLoading(true)
  Panel->>API: Load approvals (seq 1)
  API->>RPC: callCoreRpc('openhuman.approval_list_recent_decisions')
  RPC-->>API: {result: [...], logs: [...]} or []
  API->>API: unwrapRows → ApprovalAuditEntry[]
  API-->>Panel: entries
  Panel->>Panel: seq still 1? → setEntries, setIsLoading(false)
  Panel->>User: Render approval list with decision badges
  User->>Panel: Click Refresh
  Panel->>Panel: loadSeqRef.current++, reset state
  Panel->>API: Load approvals (seq 2)
  API->>RPC: callCoreRpc (fetch again)
  RPC-->>API: [latest entries]
  API-->>Panel: new entries
  Panel->>Panel: seq == 2? → setEntries
  Panel->>User: Render updated list
Loading

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • tinyhumansai/openhuman#2335: Backend approval read path (approval_list_recent_decisions / ApprovalAuditEntry) that the new fetchRecentApprovalDecisions() frontend API client calls directly.
  • tinyhumansai/openhuman#2367: Backend changes persisting approval/audit history which supply the audit data the UI displays.
  • tinyhumansai/openhuman#2355: Other update touching useSettingsNavigation route/breadcrumb logic; overlaps at navigation hook level.

Suggested reviewers

  • graycyrus
  • senamakel

"🐰 I hopped to Settings, found a trail,
Past approvals now tell their tale;
Buttons to refresh, badges that glow,
Audit history — hop, learn, and know!"

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat(settings): surface approval history audit trail' directly matches the PR's main objective—adding an approval history panel to the Settings page.
Linked Issues check ✅ Passed The PR implements all acceptance criteria from #2947: reachable panel with four states (loading, empty, error, loaded), per-decision badges (Approve once, Always allowed, Denied), wire-shape resilience, API client, navigation/breadcrumbs, i18n, tests, and capability entry.
Out of Scope Changes check ✅ Passed All changes align with #2947 objectives: approval history panel, API client, navigation, i18n translations, tests, coverage matrix, and capability entry—no unrelated modifications detected.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot added feature Net-new user-facing capability or product behavior. agent Built-in agents, prompts, orchestration, and agent runtime in src/openhuman/agent/. labels May 29, 2026
Copy link
Copy Markdown
Contributor

@graycyrus graycyrus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@obchain hey! the code looks good to me, but CI is still pending — once those checks are green i'll come back and approve this. let me know if anything comes back red and needs a look.

One small thing while i was reading through:

The comment on lines 122-123 of useSettingsNavigation.ts is garbled — looks like an edit mid-thought left it in a state that doesn't read:

// `approval-history` must be checked before `agent-access` is irrelevant
// (distinct prefixes), but both are explicit leaf routes under Agent access.

The actual ordering doesn't matter here (neither string is a substring of the other), so the comment's intent is fine, it's just the phrasing that got mangled. Worth cleaning up so the next reader isn't confused.

Everything else looks solid: unwrapRows is robust, the stale-response guard with loadSeqRef is correct, error messages correctly stay localized (no raw backend strings leak to the UI), and the test coverage hits all four panel states plus the envelope-normalization edge cases. The agent-access route gap fix is a nice catch too.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
app/src/components/settings/panels/__tests__/ApprovalHistoryPanel.test.tsx (1)

98-109: ⚡ Quick win

Add a last-request-wins test for the stale-response guard.

This verifies refetching, but not the loadSeqRef behavior itself. A test where the first request resolves after the refresh would catch regressions where stale data overwrites the newer response.

🤖 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 `@app/src/components/settings/panels/__tests__/ApprovalHistoryPanel.test.tsx`
around lines 98 - 109, Add a "last-request-wins" test for ApprovalHistoryPanel
that verifies the stale-response guard: use mockFetch to return two controllable
promises (store their resolve functions), render ApprovalHistoryPanel (which
triggers the first fetch), then fire the refresh button to start the second
fetch, resolve the second promise with the newer auditRow data and only then
resolve the first promise with different/older data, and finally assert that the
rendered list shows the newer data (from the second response) and that mockFetch
was called twice; reference ApprovalHistoryPanel, mockFetch, and the refresh
test (approval-history-refresh / approval-history-list) when locating where to
add this test.
🤖 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 `@app/src/components/settings/hooks/useSettingsNavigation.ts`:
- Around line 186-189: Replace the hard-coded breadcrumb label in
agentAccessCrumb with a localized string from useT(): import and call useT()
(the i18n hook used elsewhere in this file) and replace label: 'Agent access'
with label: t('settings.agentAccess') (or the appropriate key consistent with
existing keys) so the user-visible string uses the translator; update any nearby
tests or keys if needed and keep the BreadcrumbItem object name agentAccessCrumb
unchanged.

In `@app/src/components/settings/panels/ApprovalHistoryPanel.tsx`:
- Around line 143-145: Replace the concatenation of the label and formatted date
with a single translated string that accepts a {date} placeholder: call
t('settings.approvalHistory.decidedAtWithDate', { date:
formatDateTime(entry.decided_at) }) instead of
t('settings.approvalHistory.decidedAt') + ' ' + formatDateTime(...); update the
locale files to add the new key (settings.approvalHistory.decidedAtWithDate)
with the appropriate placeholder placement for each language; keep using the
existing t function and formatDateTime helper and ensure entry.decided_at is
passed into formatDateTime before handing it to t.
- Around line 12-15: Replace the local debug helper's use of
import.meta.env?.DEV with the shared flag exported from app/src/utils/config.ts
(import the exported boolean, e.g. isDev/DEV) and use that in the debug function
so dev-mode is read from the central config; also stop building the “decided at”
string by concatenation and instead add a new i18n key (e.g.
settings.approvalHistory.decidedAtWithDate) that includes a date placeholder,
then render it via a single t('settings.approvalHistory.decidedAtWithDate', {
date: formatDateTime(...) }) in ApprovalHistoryPanel.tsx so word order is fully
localizable.

---

Nitpick comments:
In `@app/src/components/settings/panels/__tests__/ApprovalHistoryPanel.test.tsx`:
- Around line 98-109: Add a "last-request-wins" test for ApprovalHistoryPanel
that verifies the stale-response guard: use mockFetch to return two controllable
promises (store their resolve functions), render ApprovalHistoryPanel (which
triggers the first fetch), then fire the refresh button to start the second
fetch, resolve the second promise with the newer auditRow data and only then
resolve the first promise with different/older data, and finally assert that the
rendered list shows the newer data (from the second response) and that mockFetch
was called twice; reference ApprovalHistoryPanel, mockFetch, and the refresh
test (approval-history-refresh / approval-history-list) when locating where to
add this test.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6d38d78e-e434-4699-96b3-cdce95dfb312

📥 Commits

Reviewing files that changed from the base of the PR and between c52f75d and 1fd0e23.

📒 Files selected for processing (24)
  • app/src/components/settings/hooks/useSettingsNavigation.ts
  • app/src/components/settings/panels/AgentAccessPanel.tsx
  • app/src/components/settings/panels/ApprovalHistoryPanel.tsx
  • app/src/components/settings/panels/__tests__/ApprovalHistoryPanel.test.tsx
  • app/src/lib/i18n/chunks/ar-5.ts
  • app/src/lib/i18n/chunks/bn-5.ts
  • app/src/lib/i18n/chunks/de-5.ts
  • app/src/lib/i18n/chunks/en-5.ts
  • app/src/lib/i18n/chunks/es-5.ts
  • app/src/lib/i18n/chunks/fr-5.ts
  • app/src/lib/i18n/chunks/hi-5.ts
  • app/src/lib/i18n/chunks/id-5.ts
  • app/src/lib/i18n/chunks/it-5.ts
  • app/src/lib/i18n/chunks/ko-5.ts
  • app/src/lib/i18n/chunks/pl-5.ts
  • app/src/lib/i18n/chunks/pt-5.ts
  • app/src/lib/i18n/chunks/ru-5.ts
  • app/src/lib/i18n/chunks/zh-CN-5.ts
  • app/src/lib/i18n/en.ts
  • app/src/pages/Settings.tsx
  • app/src/services/api/approvalApi.test.ts
  • app/src/services/api/approvalApi.ts
  • docs/TEST-COVERAGE-MATRIX.md
  • src/openhuman/about_app/catalog.rs

Comment on lines +186 to +189
const agentAccessCrumb: BreadcrumbItem = {
label: 'Agent access',
onClick: () => navigate('/settings/agent-access'),
};
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Localize the new breadcrumb label instead of hard-coding it.

agentAccessCrumb.label is user-visible and should come from useT() to preserve locale parity.

Suggested fix
+import { useT } from '../../../lib/i18n/I18nContext';
...
 export const useSettingsNavigation = (): SettingsNavigationHook => {
+  const { t } = useT();
   const navigate = useNavigate();
   const location = useLocation();
...
   const agentAccessCrumb: BreadcrumbItem = {
-    label: 'Agent access',
+    label: t('settings.agentAccess.title'),
     onClick: () => navigate('/settings/agent-access'),
   };

As per coding guidelines: “Every user-visible string in app/src/** … must use useT() …; hard-coded literals … are not allowed.”

🤖 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 `@app/src/components/settings/hooks/useSettingsNavigation.ts` around lines 186
- 189, Replace the hard-coded breadcrumb label in agentAccessCrumb with a
localized string from useT(): import and call useT() (the i18n hook used
elsewhere in this file) and replace label: 'Agent access' with label:
t('settings.agentAccess') (or the appropriate key consistent with existing keys)
so the user-visible string uses the translator; update any nearby tests or keys
if needed and keep the BreadcrumbItem object name agentAccessCrumb unchanged.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferring: every breadcrumb label in this hook is a hard-coded English literal today (settingsCrumb 'Settings', accountCrumb 'Account', developerCrumb 'Developer Options', etc.). Localizing only the new agentAccessCrumb would be inconsistent; the whole breadcrumb set should move to useT() in one dedicated pass rather than piecemeal here. Tracking as a follow-up.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@obchain, that's a fair point — applying useT() to only agentAccessCrumb while all sibling crumbs remain hard-coded English would be inconsistent and would create a confusing mixed state. A single pass across the whole breadcrumb set is the right approach.

Would you like me to open a GitHub issue to track the breadcrumb-localization follow-up so it doesn't get lost?

Comment thread app/src/components/settings/panels/ApprovalHistoryPanel.tsx Outdated
Comment thread app/src/components/settings/panels/ApprovalHistoryPanel.tsx
@sanil-23 sanil-23 self-assigned this May 29, 2026
- Replace the import.meta.env DEV-gated console.debug helper with the
  shared `debug` namespace logger (repo convention; no direct
  import.meta.env access).
- Localize the "decided at" line as a single {date}-placeholder string
  instead of concatenating, so locales control word order.
@coderabbitai coderabbitai Bot added the working A PR that is being worked on by the team. label May 29, 2026
@senamakel senamakel self-assigned this May 30, 2026
senamakel added 4 commits May 30, 2026 07:46
# Conflicts:
#	app/src/components/settings/hooks/useSettingsNavigation.ts
#	app/src/lib/i18n/chunks/ar-5.ts
#	app/src/lib/i18n/chunks/bn-5.ts
#	app/src/lib/i18n/chunks/de-5.ts
#	app/src/lib/i18n/chunks/en-5.ts
#	app/src/lib/i18n/chunks/es-5.ts
#	app/src/lib/i18n/chunks/fr-5.ts
#	app/src/lib/i18n/chunks/hi-5.ts
#	app/src/lib/i18n/chunks/id-5.ts
#	app/src/lib/i18n/chunks/it-5.ts
#	app/src/lib/i18n/chunks/ko-5.ts
#	app/src/lib/i18n/chunks/pl-5.ts
#	app/src/lib/i18n/chunks/pt-5.ts
#	app/src/lib/i18n/chunks/ru-5.ts
#	app/src/lib/i18n/chunks/zh-CN-5.ts
#	app/src/pages/Settings.tsx
#	docs/TEST-COVERAGE-MATRIX.md
#	src/openhuman/about_app/catalog.rs
Post-merge semantic conflict: main moved agent-access under the new Agents
section page, while this PR had treated it as a top-level Settings panel. That
left a duplicate `agent-access` switch case (no-duplicate-case lint error) and
a duplicate `SettingsRoute` union member. Drop the PR's top-level treatment so
main's IA wins, and route the approval-history trail through
Settings -> Agents -> Agent access (keeping both agentsCrumb and
agentAccessCrumb in use).
Addresses CodeRabbit nitpick on ApprovalHistoryPanel.test.tsx: exercises the
loadSeqRef guard via the mount-load -> refresh sequence, asserting the newer
response's rows replace the initial load.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@app/src/components/settings/panels/__tests__/ApprovalHistoryPanel.test.tsx`:
- Around line 112-130: The test currently waits for the first response before
triggering the refresh so it doesn't exercise overlapping requests; change the
mockFetch usage in the 'keeps the latest result...' test to return two deferred
Promises (e.g. promiseA for the initial mount and promiseB for the refresh) so
you can trigger the refresh (fireEvent.click on 'approval-history-refresh')
while the initial request is still pending, then resolve the refresh promiseB
with the 'new-tool' result and only afterwards resolve promiseA with the
'old-tool' result to verify ApprovalHistoryPanel (and its loadSeqRef
last-request-wins logic) keeps the newer response; update assertions to await
'new-tool' and assert 'old-tool' is absent.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6953431d-e679-4f67-b4b9-4915d49d7a15

📥 Commits

Reviewing files that changed from the base of the PR and between c2cdea9 and 3010878.

📒 Files selected for processing (19)
  • app/src/components/settings/hooks/useSettingsNavigation.ts
  • app/src/components/settings/panels/AgentAccessPanel.tsx
  • app/src/components/settings/panels/__tests__/ApprovalHistoryPanel.test.tsx
  • app/src/lib/i18n/ar.ts
  • app/src/lib/i18n/bn.ts
  • app/src/lib/i18n/de.ts
  • app/src/lib/i18n/en.ts
  • app/src/lib/i18n/es.ts
  • app/src/lib/i18n/fr.ts
  • app/src/lib/i18n/hi.ts
  • app/src/lib/i18n/id.ts
  • app/src/lib/i18n/it.ts
  • app/src/lib/i18n/ko.ts
  • app/src/lib/i18n/pl.ts
  • app/src/lib/i18n/pt.ts
  • app/src/lib/i18n/ru.ts
  • app/src/lib/i18n/zh-CN.ts
  • app/src/pages/Settings.tsx
  • docs/TEST-COVERAGE-MATRIX.md
💤 Files with no reviewable changes (16)
  • app/src/lib/i18n/ru.ts
  • app/src/lib/i18n/ko.ts
  • app/src/lib/i18n/pt.ts
  • app/src/lib/i18n/it.ts
  • app/src/pages/Settings.tsx
  • app/src/lib/i18n/ar.ts
  • app/src/lib/i18n/zh-CN.ts
  • app/src/lib/i18n/pl.ts
  • app/src/lib/i18n/hi.ts
  • app/src/lib/i18n/fr.ts
  • app/src/lib/i18n/de.ts
  • docs/TEST-COVERAGE-MATRIX.md
  • app/src/lib/i18n/en.ts
  • app/src/lib/i18n/id.ts
  • app/src/lib/i18n/es.ts
  • app/src/lib/i18n/bn.ts

Comment on lines +112 to +130
it('keeps the latest result when a refresh supersedes the initial load', async () => {
// Drives the `loadSeqRef` last-request-wins path: the mount load resolves
// with one set of rows, a refresh fetches a different set, and the newer
// response is what remains on screen.
mockFetch
.mockResolvedValueOnce([auditRow({ request_id: 'old', tool_name: 'old-tool' })])
.mockResolvedValueOnce([auditRow({ request_id: 'new', tool_name: 'new-tool' })]);

renderWithProviders(<ApprovalHistoryPanel />, {
initialEntries: ['/settings/approval-history'],
});

await screen.findByText('old-tool');

fireEvent.click(screen.getByTestId('approval-history-refresh'));

await screen.findByText('new-tool');
expect(screen.queryByText('old-tool')).not.toBeInTheDocument();
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

This test doesn’t exercise the stale-response race it claims to cover.

Because Line 124 waits for old-tool before Line 126 clicks refresh, the first request is already complete; this validates sequential replacement, not “last request wins” under overlapping in-flight calls. Please make the two calls overlap and resolve them out of order (resolve refresh first, then initial) to verify loadSeqRef protection.

Suggested test shape (deferred promises)
 it('keeps the latest result when a refresh supersedes the initial load', async () => {
-  mockFetch
-    .mockResolvedValueOnce([auditRow({ request_id: 'old', tool_name: 'old-tool' })])
-    .mockResolvedValueOnce([auditRow({ request_id: 'new', tool_name: 'new-tool' })]);
+  let resolveFirst!: (v: ApprovalAuditEntry[]) => void;
+  let resolveSecond!: (v: ApprovalAuditEntry[]) => void;
+  const first = new Promise<ApprovalAuditEntry[]>((r) => { resolveFirst = r; });
+  const second = new Promise<ApprovalAuditEntry[]>((r) => { resolveSecond = r; });
+  mockFetch.mockReturnValueOnce(first).mockReturnValueOnce(second);

   renderWithProviders(<ApprovalHistoryPanel />, {
     initialEntries: ['/settings/approval-history'],
   });

-  await screen.findByText('old-tool');
-
   fireEvent.click(screen.getByTestId('approval-history-refresh'));

+  resolveSecond([auditRow({ request_id: 'new', tool_name: 'new-tool' })]);
   await screen.findByText('new-tool');
+
+  resolveFirst([auditRow({ request_id: 'old', tool_name: 'old-tool' })]);
+  await waitFor(() => {
+    expect(screen.queryByText('old-tool')).not.toBeInTheDocument();
+  });
-  expect(screen.queryByText('old-tool')).not.toBeInTheDocument();
 });

As per coding guidelines: "Prefer behavior over implementation details in unit tests."

🤖 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 `@app/src/components/settings/panels/__tests__/ApprovalHistoryPanel.test.tsx`
around lines 112 - 130, The test currently waits for the first response before
triggering the refresh so it doesn't exercise overlapping requests; change the
mockFetch usage in the 'keeps the latest result...' test to return two deferred
Promises (e.g. promiseA for the initial mount and promiseB for the refresh) so
you can trigger the refresh (fireEvent.click on 'approval-history-refresh')
while the initial request is still pending, then resolve the refresh promiseB
with the 'new-tool' result and only afterwards resolve promiseA with the
'old-tool' result to verify ApprovalHistoryPanel (and its loadSeqRef
last-request-wins logic) keeps the newer response; update assertions to await
'new-tool' and assert 'old-tool' is absent.

Addresses CodeRabbit on ApprovalHistoryPanel.test.tsx: the earlier test name
overclaimed coverage of the overlapping last-request-wins race. That race isn't
reachable from the UI — the Refresh button is `disabled` while a load is in
flight, so two concurrent fetches can't be initiated (verified empirically: a
deferred-promise overlap test deadlocks because the refresh click is a no-op on
the disabled button). Renamed to reflect what it actually verifies — the
reachable refresh-replacement path — and documented why the loadSeqRef guard
stays as defensive protection against React concurrent/StrictMode double-invoke.
// reachable from the UI — the Refresh button is `disabled` while a load is
// pending, so two concurrent fetches can never be initiated. The guard
// stays as defense against React concurrent/StrictMode double-invocation.)
mockFetch
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in ec28a5584 — but not by adapting the suggested deferred-promise test, because that overlap isn't reachable from the component's public API.

The Refresh button is disabled={isLoading}, and isLoading starts true for the mount load. So while the initial request is in flight the button is disabled and a second fetch can't be initiated — there's no way to get two overlapping in-flight loads through the UI. I verified this empirically: wiring up the exact deferred-promise shape you suggested makes the test deadlock — fireEvent.click on the disabled Refresh button is a no-op, the second fetch never fires, and findByText('new-tool') times out.

So the loadSeqRef guard is genuinely defensive — it protects against React concurrent-mode / StrictMode double-invocation of the mount effect, not a user-reachable double-click. I tried exercising it via <StrictMode> too, but the dev double-invoke doesn't reproduce deterministically under Vitest/jsdom here.

I renamed the test to replaces the list with the refreshed result so it no longer overclaims, and documented the above in a comment so the next reader knows the race is unreachable-by-design rather than untested-by-omission. The uncovered stale-discard branch is 3 lines and doesn't move the changed-line coverage gate below threshold.

@senamakel senamakel merged commit 46b6c1f into tinyhumansai:main May 30, 2026
18 of 21 checks passed
@sanil-23 sanil-23 removed their assignment May 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent Built-in agents, prompts, orchestration, and agent runtime in src/openhuman/agent/. feature Net-new user-facing capability or product behavior. working A PR that is being worked on by the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Surface approval history (audit trail) in Settings

4 participants