refactor(rewards): update referral sharing functionality and add tests#834
Closed
YellowSnnowmann wants to merge 4 commits intotinyhumansai:mainfrom
Closed
refactor(rewards): update referral sharing functionality and add tests#834YellowSnnowmann wants to merge 4 commits intotinyhumansai:mainfrom
YellowSnnowmann wants to merge 4 commits intotinyhumansai:mainfrom
Conversation
- Refactored the ReferralRewardsSection component to copy only the referral code instead of the referral link, enhancing clarity for users. - Updated the share functionality to include a direct download link for the app, improving the sharing experience. - Added a new test suite for ReferralRewardsSection to ensure the correct behavior of the copy and share functionalities. - Introduced a utility function for opening URLs, improving code reusability across components.
Contributor
📝 WalkthroughWalkthroughLarge feature-and-infra update: adds native OS notification plumbing (frontend, Tauri bridge, services, Redux, UI), notification routing/settings, skills UI and API client, global memory-tree digest/recap/registry/sealing, connected-identities prompt plumbing, many tests and E2E specs, onboarding simplifications, assorted Rust refactors, and multiple docs/CI/workflow additions. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/components/rewards/ReferralRewardsSection.tsx (1)
93-133:⚠️ Potential issue | 🟡 MinorAdd namespaced debug logs for copy/share branches and failures.
This change introduces multiple external call branches (
navigator.clipboard,navigator.share) and error paths, but these paths currently have no diagnostic logging.As per coding guidelines: “Add substantial development-oriented logging at entry/exit points, branch decisions, external calls, retries/timeouts, state transitions, and error paths.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/rewards/ReferralRewardsSection.tsx` around lines 93 - 133, Add namespaced debug logging to the handleCopy and handleShare flows: log entry/exit of handleCopy and handleShare, log branch decisions (missing referralCodeToCopy, navigator.share present/absent), log before external calls (navigator.clipboard.writeText and navigator.share) with the referral code or shareText (sanitized if needed), and log success and failure paths including caught errors and their .name/.message; also log fallback attempts (clipboard fallback after share failure) and the setCopyHint state changes. Target the functions handleCopy and handleShare and place logs at each branch, external call, retry/fallback, and in every catch block so developers can trace decisions and errors.
🧹 Nitpick comments (1)
app/src/components/rewards/__tests__/ReferralRewardsSection.test.tsx (1)
64-89: Add fallback-path tests fornavigator.shareunavailable/rejection.The new share behavior changed fallback semantics, but this suite currently verifies only the Web Share happy path.
As per coding guidelines: “Ship unit tests and coverage for behavior you are adding or changing before building additional features on top.”✅ Suggested test additions
+ it('falls back to clipboard when Web Share API is unavailable', async () => { + Object.defineProperty(window.navigator, 'share', { + value: undefined, + configurable: true, + writable: true, + }); + + mocks.mockReferralApi.getStats.mockResolvedValueOnce({ + referralCode: 'GQ9F7LEV', + referralLink: 'https://tinyhumans.ai/signup?ref=GQ9F7LEV', + totals: { totalRewardUsd: 10, pendingCount: 0, convertedCount: 2 }, + referrals: [], + canApplyReferral: true, + appliedReferralCode: null, + }); + + render(<ReferralRewardsSection />); + fireEvent.click(await screen.findByRole('button', { name: 'Share' })); + + await waitFor(() => { + expect(writeText).toHaveBeenCalledWith( + [ + 'Join me on OpenHuman.', + 'Referral code: GQ9F7LEV', + 'Download OpenHuman: https://github.com/tinyhumansai/openhuman/releases/latest', + ].join('\n') + ); + }); + }); + + it('falls back to clipboard when Web Share rejects (non-abort)', async () => { + share.mockRejectedValueOnce(new Error('Share not allowed')); + + mocks.mockReferralApi.getStats.mockResolvedValueOnce({ + referralCode: 'GQ9F7LEV', + referralLink: 'https://tinyhumans.ai/signup?ref=GQ9F7LEV', + totals: { totalRewardUsd: 10, pendingCount: 0, convertedCount: 2 }, + referrals: [], + canApplyReferral: true, + appliedReferralCode: null, + }); + + render(<ReferralRewardsSection />); + fireEvent.click(await screen.findByRole('button', { name: 'Share' })); + + await waitFor(() => { + expect(writeText).toHaveBeenCalled(); + }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/rewards/__tests__/ReferralRewardsSection.test.tsx` around lines 64 - 89, The tests only cover the successful Web Share path; add two fallback-path tests for ReferralRewardsSection: (1) simulate navigator.share being undefined by deleting or setting global.navigator.share = undefined and assert the component uses the fallback (e.g., calls navigator.clipboard.writeText or triggers the expected fallback UI/notification) after clicking the Share button; (2) simulate navigator.share existing but rejecting by mocking share to reject (share.mockRejectedValueOnce(new Error('fail'))) and assert the same fallback behavior occurs. In both tests use mocks.mockReferralApi.getStats.mockResolvedValueOnce(...) to provide the referralCode/referralLink and locate the share button via screen.findByRole('button', { name: 'Share' }) before firing the click and awaiting the fallback assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/src/components/rewards/ReferralRewardsSection.tsx`:
- Around line 93-133: Add namespaced debug logging to the handleCopy and
handleShare flows: log entry/exit of handleCopy and handleShare, log branch
decisions (missing referralCodeToCopy, navigator.share present/absent), log
before external calls (navigator.clipboard.writeText and navigator.share) with
the referral code or shareText (sanitized if needed), and log success and
failure paths including caught errors and their .name/.message; also log
fallback attempts (clipboard fallback after share failure) and the setCopyHint
state changes. Target the functions handleCopy and handleShare and place logs at
each branch, external call, retry/fallback, and in every catch block so
developers can trace decisions and errors.
---
Nitpick comments:
In `@app/src/components/rewards/__tests__/ReferralRewardsSection.test.tsx`:
- Around line 64-89: The tests only cover the successful Web Share path; add two
fallback-path tests for ReferralRewardsSection: (1) simulate navigator.share
being undefined by deleting or setting global.navigator.share = undefined and
assert the component uses the fallback (e.g., calls
navigator.clipboard.writeText or triggers the expected fallback UI/notification)
after clicking the Share button; (2) simulate navigator.share existing but
rejecting by mocking share to reject (share.mockRejectedValueOnce(new
Error('fail'))) and assert the same fallback behavior occurs. In both tests use
mocks.mockReferralApi.getStats.mockResolvedValueOnce(...) to provide the
referralCode/referralLink and locate the share button via
screen.findByRole('button', { name: 'Share' }) before firing the click and
awaiting the fallback assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c826ec5-ff87-4ee7-9527-1d9158110c2c
📒 Files selected for processing (5)
app/src/components/rewards/ReferralRewardsSection.tsxapp/src/components/rewards/RewardsCommunityTab.tsxapp/src/components/rewards/__tests__/ReferralRewardsSection.test.tsxapp/src/pages/__tests__/Rewards.test.tsxapp/src/test/setup.ts
- Introduced a new �]2;��]1;� module containing a function that normalizes whitespace in thread titles. - Updated to utilize the new function, enhancing title sanitization. - Added unit tests for the function to ensure correct behavior across various input scenarios.
…hreads" This reverts commit 558a573.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/openhuman/memory/tree/score/extract/types.rs (1)
198-214:⚠️ Potential issue | 🟡 MinorTest coverage gap:
EntityKind::Topicmissing from round-trip test.The
entity_kind_round_triptest doesn't include the newly addedTopicvariant, which could allow serialization/parsing regressions to go undetected.💚 Proposed fix
fn entity_kind_round_trip() { for k in [ EntityKind::Email, EntityKind::Url, EntityKind::Handle, EntityKind::Hashtag, EntityKind::Person, EntityKind::Organization, EntityKind::Location, EntityKind::Event, EntityKind::Product, EntityKind::Misc, + EntityKind::Topic, ] { assert_eq!(EntityKind::parse(k.as_str()).unwrap(), k); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/memory/tree/score/extract/types.rs` around lines 198 - 214, The entity_kind_round_trip test omits the new EntityKind::Topic variant, so update the entity_kind_round_trip test to include EntityKind::Topic in the array of variants being iterated; ensure you still call k.as_str() and EntityKind::parse(...) unwrap equality as done for other variants so Topic round-trips through as_str and parse correctly.src/openhuman/agent/triage/envelope.rs (1)
22-40:⚠️ Potential issue | 🟡 MinorAdd a direct test for the new
WebviewIntegrationslug.This adds a new triage source, but the suite still only locks down the
Composiopath. A tinyTriggerSource::WebviewIntegration { .. }.slug() == "webview"test would catch regressions in the new routing label cheaply.Based on learnings: "Ship enough unit tests and coverage for behavior being added or changed before building additional features on top—treat untested code as incomplete"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/agent/triage/envelope.rs` around lines 22 - 40, Add a unit test that verifies TriggerSource::WebviewIntegration { provider: ..., account_id: ... }.slug() returns "webview"; locate the slug() implementation for TriggerSource and add a small test (e.g., in the same file's tests module or the existing tests for slug) that constructs a WebviewIntegration variant and asserts equality with "webview" to prevent regressions in the new routing label.
🟡 Minor comments (26)
src/openhuman/config/schema/load.rs-258-261 (1)
258-261:⚠️ Potential issue | 🟡 MinorTrim
OPENHUMAN_WORKSPACEbefore validating emptiness.At Line 259,
is_empty()allows whitespace-only values (e.g.," "), which can be interpreted as an unintended path. Treat trimmed-empty values as absent.💡 Suggested patch
- if let Some(custom_workspace) = env.get("OPENHUMAN_WORKSPACE") { - if !custom_workspace.is_empty() { + if let Some(custom_workspace) = env.get("OPENHUMAN_WORKSPACE") { + let custom_workspace = custom_workspace.trim(); + if !custom_workspace.is_empty() { let (openhuman_dir, workspace_dir) = resolve_config_dir_for_workspace(&PathBuf::from(custom_workspace)); return Ok(( openhuman_dir, workspace_dir, ConfigResolutionSource::EnvWorkspace, )); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/config/schema/load.rs` around lines 258 - 261, The code reads OPENHUMAN_WORKSPACE into custom_workspace and checks custom_workspace.is_empty(), which misses whitespace-only values; trim the environment string before validating emptiness (e.g., let trimmed = custom_workspace.trim()), use trimmed.is_empty() for the guard, and pass the trimmed value (not the raw custom_workspace) into resolve_config_dir_for_workspace (or into PathBuf::from) so whitespace-only inputs are treated as absent and valid paths get used.app/src/providers/__tests__/CoreStateProvider.test.tsx-106-112 (1)
106-112:⚠️ Potential issue | 🟡 MinorStabilize context access to avoid intermittent
ctx!crashes.
ctxis captured viauseEffect, but it’s invoked withctx!in async flows. In slower CI timing, this can be undefined momentarily and flake.Suggested hardening
let ctx: CoreStateContextValue | undefined; @@ render( <CoreStateProvider> @@ </CoreStateProvider> ); + await waitFor(() => expect(ctx).toBeDefined()); @@ - await act(async () => { - await ctx!.refreshTeamMembers('team-u1'); - await ctx!.refreshTeamInvites('team-u1'); - }); + await act(async () => { + const current = ctx; + if (!current) throw new Error('CoreState context was not captured'); + await current.refreshTeamMembers('team-u1'); + await current.refreshTeamInvites('team-u1'); + });Apply the same guard pattern to other
ctx!.refresh()calls in this file.Based on learnings: Keep test files deterministic and avoid hidden global state/flakes.
Also applies to: 121-124, 145-151, 161-163, 176-182, 191-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/providers/__tests__/CoreStateProvider.test.tsx` around lines 106 - 112, Tests capture `ctx` via the Consumer but call ctx! (non-null assertion) directly causing intermittent undefined crashes; change every direct ctx! usage (e.g., calls to ctx!.refresh()) to a safe guard that waits for or asserts ctx is defined before invoking methods: after rendering <CoreStateProvider>/<Consumer> use a test-friendly wait/assert (for example waitFor(() => expect(ctx).toBeDefined()) or an explicit if (!ctx) throw/wait) and then call ctx.refresh() (optionally wrapped in act())—apply this pattern to all occurrences of ctx!.refresh() and other ctx! calls in this test file (around the Consumer captureCtx usage and the ranges noted)..claude/agents/pr-manager.md-1-241 (1)
1-241:⚠️ Potential issue | 🟡 MinorConsolidate duplicated agent specs: two files are mirrored across
.agents/agents/and.claude/agents/.This PR introduces duplication across both agent directories:
pr-manager.md(245 lines in.agents/, 241 in.claude/)pr-manager-lite.md(195 lines in.agents/, 197 in.claude/)
.agents/agents/contains only these 2 files, while.claude/agents/has 14 files (the full suite). This suggests.agents/agents/may be a legacy or subset directory.Recommendation: If
.claude/agents/is the primary agent repository, remove.agents/agents/entirely and document the canonical location inREADME.mdorAGENTS.md. If.agents/serves a distinct purpose, clearly document when each directory is used and consolidate the shared specs to a single source of truth to avoid sync drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/agents/pr-manager.md around lines 1 - 241, The PR contains duplicated agent spec files (pr-manager.md and pr-manager-lite.md) present in both `.agents/agents/` and `.claude/agents/`; pick a single canonical location (suggest `.claude/agents/`), remove the duplicates from the other directory (`.agents/agents/`), update the repo documentation (README.md or AGENTS.md) to declare `.claude/agents/` as the canonical agent spec location and note any legacy purpose of `.agents/`, then run format/lint and commit the change with a message like "chore(docs): consolidate agent specs to .claude/agents (remove duplicates)".packages/homebrew-core/openhuman.rb-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorRendered formula appears out of sync with the template.
Line 3 differs from
packages/homebrew-core/openhuman.rb.in(homepage URL). Since this file is generated from the template, this will cause avoidable churn on the next render; please align one source of truth and regenerate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/homebrew-core/openhuman.rb` at line 3, The rendered formula's homepage value (homepage "https://tinyhumans.ai/openhuman") is out of sync with the template openhuman.rb.in; update the source of truth (either modify the template openhuman.rb.in to match the intended homepage or change the rendered file to match the template), then re-run the renderer/generator to regenerate packages/homebrew-core/openhuman.rb so both files align and avoid future churn.scripts/release/render-homebrew-core-formula.sh-15-18 (1)
15-18:⚠️ Potential issue | 🟡 MinorValidate tag format before deriving
VERSIONand download URL.Line 15 accepts any string; passing
0.52.27(nov) makes Line 18 fetch a likely-invalid tag URL and fail later with a less actionable error.💡 Proposed fix
TAG="${1:?Usage: render-homebrew-core-formula.sh <tag> [output_path]}" OUT="${2:-$REPO_ROOT/packages/homebrew-core/openhuman.rb}" +if [[ ! "$TAG" =~ ^v[0-9]+(\.[0-9]+)*([.-][0-9A-Za-z]+)?$ ]]; then + echo "Invalid tag '$TAG'. Expected format like v0.52.27" >&2 + exit 1 +fi VERSION="${TAG#v}" SOURCE_URL="https://github.com/tinyhumansai/openhuman/archive/refs/tags/${TAG}.tar.gz"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release/render-homebrew-core-formula.sh` around lines 15 - 18, Validate TAG before deriving VERSION and SOURCE_URL: after the TAG assignment in render-homebrew-core-formula.sh, check that TAG matches the expected prefixed-semver pattern (e.g., starts with "v" and follows semver like vX.Y.Z) and if not print a clear usage/error message and exit non‑zero; only compute VERSION="${TAG#v}" and SOURCE_URL after the check. Reference variables: TAG, VERSION, SOURCE_URL.app/src/pages/Rewards.tsx-28-51 (1)
28-51:⚠️ Potential issue | 🟡 MinorRetry fetches are not race/cancellation safe.
On line 68,
handleRetrycallsloadRewards()without a signal, so retries lack cancellation protection unlike the mount-triggered call (line 55). If the user navigates away while a retry is in-flight, the response will still update component state after unmount, causing state-update warnings.While the retry button is correctly disabled during loading (line 155 in RewardsCommunityTab), this only prevents overlapping concurrent requests—not the unmount race condition when users navigate away mid-retry.
💡 Suggested fix (request id + mounted ref)
-import { useCallback, useEffect, useState } from 'react'; +import { useCallback, useEffect, useRef, useState } from 'react'; @@ const Rewards = () => { + const mountedRef = useRef(true); + const requestIdRef = useRef(0); @@ - const loadRewards = useCallback(async (signal?: { cancelled: boolean }) => { + const loadRewards = useCallback(async () => { + const requestId = ++requestIdRef.current; console.debug('[rewards] fetching snapshot'); setIsLoading(true); setError(null); try { const result = await rewardsApi.getMyRewards(); - if (signal?.cancelled) return; + if (!mountedRef.current || requestId !== requestIdRef.current) return; setSnapshot(result); @@ } catch (err) { const message = errorMessage(err); console.debug('[rewards] snapshot load failed', message); - if (signal?.cancelled) return; + if (!mountedRef.current || requestId !== requestIdRef.current) return; setSnapshot(null); setError(message); } finally { - if (!signal?.cancelled) { + if (mountedRef.current && requestId === requestIdRef.current) { setIsLoading(false); } } }, []); @@ useEffect(() => { - const signal = { cancelled: false }; - void loadRewards(signal); - return () => { - signal.cancelled = true; - }; + void loadRewards(); + return () => { + mountedRef.current = false; + requestIdRef.current += 1; + }; }, [loadRewards]); @@ const handleRetry = useCallback(() => { console.debug('[rewards] retry requested'); void loadRewards(); }, [loadRewards]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/Rewards.tsx` around lines 28 - 51, The retry path calls loadRewards() without a cancellation token causing state updates after unmount; modify handleRetry to provide a cancellable signal (or generate a unique requestId and compare against a mounted/request ref) when calling loadRewards so the in-flight retry can be ignored if cancelled/unmounted; ensure loadRewards continues to check signal?.cancelled (and/or requestId match) before calling setSnapshot, setError, and setIsLoading so no state updates occur after unmount..claude/memory.md-110-110 (1)
110-110:⚠️ Potential issue | 🟡 MinorMinor docs wording fix: use “Markdown” (proper noun).
Line 110 uses “markdown”; please capitalize to “Markdown” for consistency with technical docs style.
Suggested docs patch
-- **Connected identities loader/renderer** — `src/openhuman/composio/providers/profile.rs` contains `load_connected_identities()` (reads `skill:*` facets) and `render_connected_identities_section()` (formats markdown for prompt injection). Keep rendering logic there, not in prompt modules. +- **Connected identities loader/renderer** — `src/openhuman/composio/providers/profile.rs` contains `load_connected_identities()` (reads `skill:*` facets) and `render_connected_identities_section()` (formats Markdown for prompt injection). Keep rendering logic there, not in prompt modules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/memory.md at line 110, Tiny docs wording fix: change the lowercase "markdown" to the proper noun "Markdown" in the docs text referenced (the Connected identities loader/renderer description that mentions render_connected_identities_section and load_connected_identities). Locate the sentence that reads "formats markdown for prompt injection" and update it to "formats Markdown for prompt injection" to match technical documentation style.src/core/event_bus/events.rs-228-232 (1)
228-232:⚠️ Potential issue | 🟡 MinorAdd the new Composio variant to the domain-mapping test table.
Line 228 introduces
ComposioConnectionDeleted, butall_variants_have_correct_domaindoes not include it yet, so the “all variants” guard is now incomplete.Based on learnings: Ship unit tests and coverage for behavior you are adding or changing before building additional features on top.✅ Suggested test addition
( DomainEvent::ComposioConnectionCreated { toolkit: "gmail".into(), connection_id: "conn-1".into(), connect_url: "https://backend.composio.dev/connect/abc".into(), }, "composio", ), + ( + DomainEvent::ComposioConnectionDeleted { + toolkit: "gmail".into(), + connection_id: "conn-1".into(), + }, + "composio", + ), ( DomainEvent::ComposioActionExecuted { tool: "GMAIL_SEND_EMAIL".into(), success: true, error: None,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/event_bus/events.rs` around lines 228 - 232, The domain-mapping test all_variants_have_correct_domain is missing the new enum variant ComposioConnectionDeleted; update that test to include ComposioConnectionDeleted { toolkit: String::from("..."), connection_id: String::from("...") } in the list of variants and add its expected domain entry (use the same domain string used by other Composio variants) so the “all variants” guard covers the new variant and the test compiles.app/src/components/skills/CreateSkillModal.tsx-299-302 (1)
299-302:⚠️ Potential issue | 🟡 MinorAvoid Unix-only path text in a cross-platform dialog.
~/.openhuman/...is wrong on Windows, so this helper copy will mislead part of the shipped desktop audience. Prefer platform-neutral wording here, or render the actual resolved path from the core.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/skills/CreateSkillModal.tsx` around lines 299 - 302, The copy in CreateSkillModal that uses "~/.openhuman/skills/<slug>/SKILL.md" is Unix-specific and can mislead Windows users; update the UI text where scope is used (the JSX block in CreateSkillModal around the paragraph using scope === 'user') to use platform-neutral wording such as "your user profile's OpenHuman skills folder" or "the workspace's OpenHuman skills folder", or fetch and show the actual resolved path from the core API (e.g., call the existing path-resolving function and render its result) so the dialog displays either neutral phrasing or the real platform-specific path instead of "~/.openhuman/...".app/src/components/skills/SkillResourceTree.tsx-96-107 (1)
96-107:⚠️ Potential issue | 🟡 MinorExpose the current selection semantically.
The selected resource is indicated only by color. Add a semantic state such as
aria-pressed={isSelected}so keyboard and screen-reader users can tell which file is active.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/skills/SkillResourceTree.tsx` around lines 96 - 107, The button that triggers onSelect in SkillResourceTree currently indicates selection only by color; add a semantic state by including aria-pressed={isSelected} (ensure you pass the boolean isSelected, not a string) on the same button element (the one using log, path, onSelect and isSelected) so screen readers and keyboard users can perceive the active selection.app/test/wdio.conf.ts-108-116 (1)
108-116:⚠️ Potential issue | 🟡 MinorGuard artifact capture failures inside
afterTest.If artifact capture throws, the hook can add noisy secondary failures on top of the real test failure. Wrap the call in
try/catchand log a warning.🛡️ Suggested hardening
afterTest: async function ( test: { title: string; parent?: string }, _context: unknown, result: { passed: boolean; error?: Error } ) { if (result.passed) return; const name = [test.parent, test.title].filter(Boolean).join(' ').trim() || test.title; - await captureFailureArtifacts(name); + try { + await captureFailureArtifacts(name); + } catch (artifactError) { + console.warn(`[e2e-artifacts] failed to capture artifacts for "${name}"`, artifactError); + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/test/wdio.conf.ts` around lines 108 - 116, In the afterTest hook, guard the call to captureFailureArtifacts so artifact capture failures don't mask the original test failure: wrap the await captureFailureArtifacts(name) call in a try/catch inside afterTest (the function handling test:{title,parent} and result:{passed,error}); on catch, log a warning with the error (e.g., using the project's logger or console.warn) and do not rethrow so the original test failure remains the primary result.src/openhuman/agent/agents/welcome/prompt.rs-29-33 (1)
29-33:⚠️ Potential issue | 🟡 MinorPlease add one test for non-empty
connected_identities_md.The new branch is only exercised with empty context in this file. Add a positive-path assertion to prevent regressions in prompt composition.
✅ Suggested test addition
#[test] fn build_returns_nonempty_body() { let body = build(&ctx_with(&[])).unwrap(); assert!(!body.is_empty()); assert!(!body.contains("## Connected Integrations")); } +#[test] +fn build_includes_connected_identities_when_present() { + let mut ctx = ctx_with(&[]); + ctx.connected_identities_md = + "## Connected Identities\n\n- **gmail** — Jane Doe".to_string(); + + let body = build(&ctx).unwrap(); + assert!(body.contains("## Connected Identities")); + assert!(body.contains("- **gmail** — Jane Doe")); +} + #[test] fn build_lists_only_connected_integrations() {Based on learnings: "Ship enough unit tests and coverage for behavior being added or changed before building additional features on top—treat untested code as incomplete".
Also applies to: 123-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/agent/agents/welcome/prompt.rs` around lines 29 - 33, Add a unit test that exercises the non-empty connected_identities_md branch by constructing a Context (or the same struct used in prompt composition) with connected_identities_md set to a non-empty string, calling the prompt composition function that uses ctx.connected_identities_md (look for the function in prompt.rs that builds `out` and uses `identities`, `out.push_str`), and assert the returned prompt contains the trimmed identities followed by a double newline; this positive-path test should prevent regressions where the `if !identities.trim().is_empty()` branch is skipped.src/openhuman/agent/agents/orchestrator/prompt.rs-30-34 (1)
30-34:⚠️ Potential issue | 🟡 MinorAdd a direct test for the new connected-identities branch.
The new branch is added, but this suite only initializes
connected_identities_mdas empty. Please add one assertion for non-empty content to lock behavior and prevent silent regressions.✅ Suggested test addition
#[test] fn build_returns_nonempty_body() { let body = build(&ctx_with(&[])).unwrap(); assert!(!body.is_empty()); assert!(!body.contains("## Delegation Guide")); } +#[test] +fn build_includes_connected_identities_when_present() { + let mut ctx = ctx_with(&[]); + ctx.connected_identities_md = + "## Connected Identities\n\n- **notion** — Jane Doe".to_string(); + + let body = build(&ctx).unwrap(); + assert!(body.contains("## Connected Identities")); + assert!(body.contains("- **notion** — Jane Doe")); +} + #[test] fn build_emits_delegation_guide_with_spawn_snippet() {Based on learnings: "Ship enough unit tests and coverage for behavior being added or changed before building additional features on top—treat untested code as incomplete".
Also applies to: 110-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/agent/agents/orchestrator/prompt.rs` around lines 30 - 34, Add a unit test that exercises the new connected-identities branch by initializing ctx.connected_identities_md with non-empty content and asserting the prompt builder (the code that reads ctx.connected_identities_md and pushes into out) includes the trimmed content and the expected trailing "\n\n"; specifically, create a test that sets connected_identities_md to a non-empty string, invokes the function that produces the prompt (the code around connected_identities_md usage), and asserts the output contains that trimmed string and the two newlines so this behavior is locked and prevents regressions (also add a corresponding assertion for the similar case referenced at the other occurrence around line 110).docs/AGENT-OBSERVABILITY.md-19-35 (1)
19-35:⚠️ Potential issue | 🟡 MinorAdd a language identifier to the fenced artifact-tree block.
The code fence at Line 19 lacks a language tag (
MD040).Proposed fix
-``` +```text app/test/e2e/artifacts/<ISO-timestamp>-agent-review/ 01-welcome.png @@ failure-<test>.source.xml # only on failure meta.json # run metadata + checkpoint index</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/AGENT-OBSERVABILITY.mdaround lines 19 - 35, The fenced artifact-tree
block in AGENT-OBSERVABILITY.md is missing a language identifier which triggers
MD040; update the triple-backtick fence that precedes the artifact list (the
block starting with "app/test/e2e/artifacts/-agent-review/") to
include a language tag such as text (i.e. changetotext) so the markdown
linter recognizes the code fence correctly.</details> </blockquote></details> <details> <summary>app/scripts/e2e-agent-review.sh-26-30 (1)</summary><blockquote> `26-30`: _⚠️ Potential issue_ | _🟡 Minor_ **Guard `--label` against missing value.** With `set -u`, `--label` at the end of args throws an unhelpful unbound-variable error. Add an explicit value check and fail with a clear message. <details> <summary>Proposed fix</summary> ```diff while [ $# -gt 0 ]; do case "$1" in --skip-build) SKIP_BUILD=1; shift ;; - --label) LABEL="$2"; shift 2 ;; + --label) + if [ $# -lt 2 ]; then + echo "Missing value for --label" >&2 + exit 2 + fi + LABEL="$2" + shift 2 + ;; -h|--help) sed -n '2,14p' "$0"; exit 0 ;; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@app/scripts/e2e-agent-review.sh` around lines 26 - 30, The --label) case must guard against a missing value before assigning LABEL="$2"; update the case arm for --label) to first verify that a value exists (e.g. ensure there are at least two positional args or that "$2" is non-empty and not another option), and if absent print a clear error to stderr like "Missing value for --label" and exit with a non-zero status; only after the check perform LABEL="$2" and shift 2 so the script no longer triggers an unbound-variable error when --label is the last argument. ``` </details> </blockquote></details> <details> <summary>app/test/e2e/helpers/artifacts.ts-50-52 (1)</summary><blockquote> `50-52`: _⚠️ Potential issue_ | _🟡 Minor_ **No-op `.replace('Z', 'Z')` in `nowStamp`.** This replace does nothing—likely a leftover from editing. Remove it or clarify intent. <details> <summary>🧹 Proposed fix</summary> ```diff function nowStamp(): string { - return new Date().toISOString().replace(/[:.]/g, '-').replace('Z', 'Z'); + return new Date().toISOString().replace(/[:.]/g, '-'); } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@app/test/e2e/helpers/artifacts.ts` around lines 50 - 52, The nowStamp function contains a no-op .replace('Z', 'Z') which should be removed; update the nowStamp function (the function named nowStamp in this file) to return new Date().toISOString().replace(/[:.]/g, '-') without the redundant .replace('Z', 'Z'), or if a specific timezone marker change was intended, implement that explicit transformation instead. ``` </details> </blockquote></details> <details> <summary>src/openhuman/composio/providers/profile.rs-224-235 (1)</summary><blockquote> `224-235`: _⚠️ Potential issue_ | _🟡 Minor_ **`normalize_token` can return an empty string if input contains only special characters.** If `raw` is something like `"!!!"`, the result after `trim_matches('_')` is `""`. An empty identifier would produce malformed keys like `skill:gmail::email`. Consider defaulting to a sentinel (e.g., `"_"`) or returning `Option<String>`. <details> <summary>🛡️ Proposed defensive guard</summary> ```diff fn normalize_token(raw: &str) -> String { let mut out = String::with_capacity(raw.len()); for ch in raw.chars() { let lower = ch.to_ascii_lowercase(); if lower.is_ascii_alphanumeric() || lower == '-' || lower == '_' { out.push(lower); } else { out.push('_'); } } - out.trim_matches('_').to_string() + let trimmed = out.trim_matches('_'); + if trimmed.is_empty() { + "_".to_string() + } else { + trimmed.to_string() + } } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/openhuman/composio/providers/profile.rs` around lines 224 - 235, normalize_token can return an empty string for inputs of only non-alphanumerics; update the function (normalize_token) so it never returns an empty string by defaulting to a sentinel (e.g., "_") when the trimmed result is empty (replace the final trim_matches('_').to_string() with logic that checks if the result is empty and returns "_" instead), or alternatively change the signature to return Option<String> and propagate None where callers (e.g., key-building code that produces "skill:gmail::email") can handle it; pick one approach and implement the corresponding changes to callers to avoid producing malformed keys. ``` </details> </blockquote></details> <details> <summary>src/openhuman/composio/providers/profile.rs-6-7 (1)</summary><blockquote> `6-7`: _⚠️ Potential issue_ | _🟡 Minor_ **Stale doc comment: references `FacetType::Context` but code uses `FacetType::Skill`.** The module doc states facets are keyed as `FacetType::Context`, but the implementation (line 73) and `load_connected_identities` (line 121) use `FacetType::Skill`. <details> <summary>📝 Proposed fix</summary> ```diff -//! Each non-`None` field becomes a [`FacetType::Context`] facet keyed +//! Each non-`None` field becomes a [`FacetType::Skill`] facet keyed ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/openhuman/composio/providers/profile.rs` around lines 6 - 7, The module-level doc comment is stale: it refers to facets as `FacetType::Context` while the implementation (`FacetType::Skill` used in the code around `load_connected_identities` and the facet creation at the `FacetType::Skill` usage) actually uses `Skill`; update the top doc comment to say `FacetType::Skill` (and adjust any example key templates or wording to match `skill:{toolkit}:{identifier}:{field}`/confidence 0.95) so the documentation matches the behavior in the functions that construct skill facets such as the facet-creation code and `load_connected_identities`. ``` </details> </blockquote></details> <details> <summary>app/src-tauri/src/webview_accounts/ua_spoof.js-101-155 (1)</summary><blockquote> `101-155`: _⚠️ Potential issue_ | _🟡 Minor_ **Move the reinjection guard before the first notification shim.** `window.__OH_NOTIF_SHIM` is checked only after `navigator.permissions` and `PushManager.prototype.permissionState` have already been wrapped. On repeated injections, those earlier wrappers stack because `_rq` binds to the previously wrapped `query`, which adds avoidable indirection and can drift behavior over time. <details> <summary>🔧 Suggested fix</summary> ```diff try { delete window.safari; } catch (_) {} + if (window.__OH_NOTIF_SHIM) { + return; + } + window.__OH_NOTIF_SHIM = true; + // navigator.permissions — return "granted" for notifications queries. // Simple property assignment on Blink platform objects (like Permissions) // is silently ignored; Object.defineProperty on the navigator itself works // because navigator exposes configurable getters (same mechanism used above // for navigator.userAgent). try { @@ - if (window.__OH_NOTIF_SHIM) { - return; - } try { @@ - window.__OH_NOTIF_SHIM = true; })(); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@app/src-tauri/src/webview_accounts/ua_spoof.js` around lines 101 - 155, Move the reinjection guard check for window.__OH_NOTIF_SHIM to the top of the script so the function exits before any notification-related shims are applied; specifically, ensure the guard is evaluated before wrapping navigator.permissions (where _rq is bound) and before altering PushManager.prototype.permissionState, so repeated injections return early and avoid stacking wrappers that bind to previously wrapped functions. ``` </details> </blockquote></details> <details> <summary>app/src/AppRoutes.tsx-125-132 (1)</summary><blockquote> `125-132`: _⚠️ Potential issue_ | _🟡 Minor_ **Remove duplicate `/notifications` route declaration.** Line 125 redefines the same protected route already declared at Line 89. Keep a single route entry to avoid split edits and route table drift. <details> <summary>Suggested fix</summary> ```diff - <Route - path="/notifications" - element={ - <ProtectedRoute requireAuth={true}> - <Notifications /> - </ProtectedRoute> - } - /> ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@app/src/AppRoutes.tsx` around lines 125 - 132, The file contains a duplicate Route for path="/notifications" wrapped with ProtectedRoute and rendering <Notifications />; remove the redundant Route block so only a single Route with path="/notifications" using ProtectedRoute (and rendering Notifications) remains in the routes array/JSX (locate the duplicate Route declarations that reference path="/notifications", ProtectedRoute, and Notifications and delete the extra one). ``` </details> </blockquote></details> <details> <summary>app/src/components/settings/panels/NotificationsPanel.tsx-46-50 (1)</summary><blockquote> `46-50`: _⚠️ Potential issue_ | _🟡 Minor_ **Missing error handling for `setGlobalDnd` failure.** `handleDndToggle` optimistically updates local state before the async call completes. If `setGlobalDnd(next)` fails, the UI will show the toggled state while the backend remains unchanged, creating a mismatch. Consider either: 1. Rollback on error: catch the error and revert `dnd` to its previous value 2. Pessimistic update: await the service call before updating state <details> <summary>Proposed fix with error rollback</summary> ```diff const handleDndToggle = async () => { const next = !dnd; setDnd(next); - await setGlobalDnd(next); + try { + await setGlobalDnd(next); + } catch (err) { + console.error('[notifications] failed to set global DND:', err); + setDnd(!next); // rollback + } }; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@app/src/components/settings/panels/NotificationsPanel.tsx` around lines 46 - 50, handleDndToggle currently updates local state optimistically then awaits setGlobalDnd(next), which can lead to UI/backend mismatch on failure; change it to either perform a pessimistic update (await setGlobalDnd(next) before calling setDnd) or implement rollback: store the previous value (const prev = dnd), setDnd(next), then wrap await setGlobalDnd(next) in try/catch and on error call setDnd(prev) and surface/log the error; update logic is inside handleDndToggle and references setGlobalDnd, setDnd, and dnd. ``` </details> </blockquote></details> <details> <summary>app/src/components/settings/panels/NotificationsPanel.tsx-35-40 (1)</summary><blockquote> `35-40`: _⚠️ Potential issue_ | _🟡 Minor_ **Promise rejection is unhandled in useEffect.** If `getBypassPrefs()` rejects, the error is swallowed and `dndLoading` remains `true` indefinitely, leaving users stuck on the loading placeholder. <details> <summary>Proposed fix with error handling</summary> ```diff useEffect(() => { - getBypassPrefs().then(prefs => { - if (prefs) setDnd(prefs.global_dnd); - setDndLoading(false); - }); + getBypassPrefs() + .then(prefs => { + if (prefs) setDnd(prefs.global_dnd); + }) + .catch(err => { + console.error('[notifications] failed to load bypass prefs:', err); + }) + .finally(() => { + setDndLoading(false); + }); }, []); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@app/src/components/settings/panels/NotificationsPanel.tsx` around lines 35 - 40, The useEffect calling getBypassPrefs() does not handle rejections so a failure leaves dndLoading true; update the effect to handle errors (either by using an async IIFE with try/catch/finally or by appending .catch and .finally) so that on error you log or report the error and always call setDndLoading(false); ensure you still call setDnd(prefs.global_dnd) when prefs is present. Target the useEffect block and the getBypassPrefs() call, and make sure setDnd, setDndLoading are invoked appropriately in the error and finalization paths. ``` </details> </blockquote></details> <details> <summary>app/src/components/notifications/NotificationCard.tsx-8-16 (1)</summary><blockquote> `8-16`: _⚠️ Potential issue_ | _🟡 Minor_ **Guard `relativeTime()` against invalid or future timestamps.** A bad `received_at` value will render `NaNs ago`, and clock skew can produce negative strings like `-12s ago`. Clamp/validate the parsed time before formatting so malformed notification data does not leak into the UI. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@app/src/components/notifications/NotificationCard.tsx` around lines 8 - 16, Fix relativeTime by validating and clamping the parsed timestamp: inside function relativeTime(isoString: string) parse the date once, check isNaN(parsed.getTime()) and treat invalid timestamps as "just now" (or 0 diff), and if the computed diff is negative (future timestamp due to clock skew) clamp it to 0 before converting to seconds/minutes/hours/days; update the return formatting to use the clamped/validated diff so you never return "NaN" or negative values. ``` </details> </blockquote></details> <details> <summary>app/src/services/webviewAccountService.ts-135-146 (1)</summary><blockquote> `135-146`: _⚠️ Potential issue_ | _🟡 Minor_ **Set `permissionChecked` only after the full round-trip succeeds.** Right now the flag flips to `true` before `webview_notification_permission_request()` runs. If that second invoke fails once, every later account open skips permission discovery for the rest of the session and never retries. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@app/src/services/webviewAccountService.ts` around lines 135 - 146, The function ensureNotificationPermission currently sets permissionChecked = true immediately after the first invoke, causing failures in the subsequent request to be masked; change the logic in ensureNotificationPermission so that permissionChecked is set only after the full round-trip succeeds: keep setting it immediately and returning if the first invoke('webview_notification_permission_state') returns 'granted', but if state !== 'granted' only set permissionChecked = true after invoke('webview_notification_permission_request') completes successfully (and log the result); if the request throws, do not flip permissionChecked so future calls will retry. Ensure you update the code paths around the invoke calls and the permissionChecked assignment accordingly. ``` </details> </blockquote></details> <details> <summary>app/src/lib/nativeNotifications/service.ts-83-100 (1)</summary><blockquote> `83-100`: _⚠️ Potential issue_ | _🟡 Minor_ **Add coverage for the new `chat_error` and `disconnect` branches.** The companion suite only validates `chat_done`, but these two paths build different categories, titles, and bodies. A regression here will currently slip through without any test signal. Based on learnings: Ship enough unit tests and coverage for behavior being added or changed before building additional features on top. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@app/src/lib/nativeNotifications/service.ts` around lines 83 - 100, Add unit tests that exercise the chatErrorListener and disconnectListener branches to assert dispatchAndMaybeBanner is called with the expected payloads: for chatErrorListener (function chatErrorListener) verify category 'system', id pattern starts with "chat_error:", title "Agent error", body contains the truncated p.message (or default message) and deepLink '/chat'; for disconnectListener (function disconnectListener) verify category 'system', id starts with "socket_disconnect:", title "Connection lost", and body contains the truncated reason string. Mock or spy on dispatchAndMaybeBanner to capture the dispatched payloads and include tests for both normal and fallback cases (missing message/thread_id/request_id and non-string reason) so these branches are covered. ``` </details> </blockquote></details> <details> <summary>app/src/lib/nativeNotifications/__tests__/service.test.ts-15-21 (1)</summary><blockquote> `15-21`: _⚠️ Potential issue_ | _🟡 Minor_ **Restore `document.hasFocus` between tests.** `vi.clearAllMocks()` does not undo the `spyOn(document, 'hasFocus')` implementation, so the mocked focus state can leak into later cases and other suites. Add `vi.restoreAllMocks()` in `afterEach` or at the start of `beforeEach`. Based on learnings: Keep test files deterministic: avoid real network calls, time-sensitive flakes, or hidden global state. Also applies to: 47-55 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@app/src/lib/nativeNotifications/__tests__/service.test.ts` around lines 15 - 21, The test suite leaves a spy on document.hasFocus in place because vi.clearAllMocks() doesn't restore spy implementations; add vi.restoreAllMocks() either at the start of beforeEach (alongside __resetForTests() and vi.clearAllMocks()) or in an afterEach to fully restore spyOn(document, 'hasFocus') between tests; apply the same change for the other test block mentioned (the section around the existing spy usage at lines ~47-55) so no focus/mock state leaks across tests. ``` </details> </blockquote></details> </blockquote></details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `4e5c62e1-eccc-4db5-87f6-15242501f050` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 558a573a3221bf9b18fcc46ebaab5efa838a2676 and 1135fcb08c5a31ddf7424e48351d2a74e1a4929c. </details> <details> <summary>⛔ Files ignored due to path filters (2)</summary> * `Cargo.lock` is excluded by `!**/*.lock` * `app/src-tauri/Cargo.lock` is excluded by `!**/*.lock` </details> <details> <summary>📒 Files selected for processing (243)</summary> * `.agents/agents/pr-manager-lite.md` * `.agents/agents/pr-manager.md` * `.claude/agents/pr-manager-lite.md` * `.claude/agents/pr-manager.md` * `.claude/agents/pr-reviewer.md` * `.claude/memory.md` * `.claude/rules/03-platform-setup-windows.md` * `.claude/rules/04-platform-setup-macos.md` * `.claude/rules/05-platform-setup-android.md` * `.claude/rules/06-platform-setup-ios.md` * `.claude/rules/07-rust-backend-guide.md` * `.claude/rules/08-frontend-guide.md` * `.claude/rules/09-permissions-capabilities.md` * `.claude/rules/12-design-system.md` * `.claude/rules/13-backend-auth-implementation.md` * `.claude/rules/14-deep-link-platform-guide.md` * `.claude/rules/15-settings-modal-system.md` * `.claude/rules/16-macos-background-execution.md` * `.claude/rules/17-skills-memory-inference-flow.md` * `.env.example` * `.github/workflows/e2e-agent-review.yml` * `.github/workflows/installer-smoke.yml` * `.gitignore` * `AGENTS.md` * `CLAUDE.md` * `PLAN.md` * `app/.env.example` * `app/package.json` * `app/scripts/e2e-agent-review.sh` * `app/scripts/e2e-run-all-flows.sh` * `app/src-tauri/Cargo.toml` * `app/src-tauri/capabilities/default.json` * `app/src-tauri/src/cdp/emulation.rs` * `app/src-tauri/src/cdp/session.rs` * `app/src-tauri/src/cdp/target.rs` * `app/src-tauri/src/lib.rs` * `app/src-tauri/src/webview_accounts/mod.rs` * `app/src-tauri/src/webview_accounts/ua_spoof.js` * `app/src-tauri/vendor/tauri-cef` * `app/src/App.tsx` * `app/src/AppRoutes.tsx` * `app/src/components/BottomTabBar.tsx` * `app/src/components/ErrorFallbackScreen.tsx` * `app/src/components/OnboardingOverlay.tsx` * `app/src/components/__tests__/OnboardingOverlay.test.tsx` * `app/src/components/daemon/ServiceBlockingGate.tsx` * `app/src/components/daemon/__tests__/ServiceBlockingGate.test.tsx` * `app/src/components/notifications/NotificationCard.tsx` * `app/src/components/notifications/NotificationCenter.tsx` * `app/src/components/rewards/RewardsCommunityTab.tsx` * `app/src/components/settings/SettingsHome.tsx` * `app/src/components/settings/hooks/useSettingsNavigation.ts` * `app/src/components/settings/panels/ConnectionsPanel.tsx` * `app/src/components/settings/panels/NotificationRoutingPanel.tsx` * `app/src/components/settings/panels/NotificationsPanel.tsx` * `app/src/components/settings/panels/PrivacyPanel.tsx` * `app/src/components/settings/panels/RecoveryPhrasePanel.tsx` * `app/src/components/settings/panels/__tests__/ConnectionsPanel.test.tsx` * `app/src/components/settings/panels/__tests__/PrivacyPanel.test.tsx` * `app/src/components/settings/panels/__tests__/RecoveryPhrasePanel.test.tsx` * `app/src/components/skills/CreateSkillModal.tsx` * `app/src/components/skills/InstallSkillDialog.tsx` * `app/src/components/skills/SkillDetailDrawer.tsx` * `app/src/components/skills/SkillResourcePreview.tsx` * `app/src/components/skills/SkillResourceTree.tsx` * `app/src/components/skills/__tests__/CreateSkillModal.test.tsx` * `app/src/components/skills/__tests__/InstallSkillDialog.test.tsx` * `app/src/components/skills/__tests__/SkillDetailDrawer.test.tsx` * `app/src/components/skills/__tests__/SkillResourcePreview.test.tsx` * `app/src/components/ui/Button.test.tsx` * `app/src/components/ui/Button.tsx` * `app/src/features/privacy/WhatLeavesLink.tsx` * `app/src/features/privacy/WhatLeavesMyComputerSheet.test.tsx` * `app/src/features/privacy/WhatLeavesMyComputerSheet.tsx` * `app/src/features/privacy/whatLeavesItems.ts` * `app/src/hooks/__tests__/useDaemonLifecycle.test.ts` * `app/src/lib/nativeNotifications/__tests__/service.test.ts` * `app/src/lib/nativeNotifications/index.ts` * `app/src/lib/nativeNotifications/service.ts` * `app/src/lib/nativeNotifications/tauriBridge.ts` * `app/src/lib/webviewNotifications/service.ts` * `app/src/pages/Notifications.tsx` * `app/src/pages/Rewards.tsx` * `app/src/pages/Settings.tsx` * `app/src/pages/Skills.tsx` * `app/src/pages/__tests__/Rewards.test.tsx` * `app/src/pages/onboarding/Onboarding.tsx` * `app/src/pages/onboarding/components/OnboardingNextButton.tsx` * `app/src/pages/onboarding/steps/ContextGatheringStep.tsx` * `app/src/pages/onboarding/steps/WelcomeStep.tsx` * `app/src/pages/onboarding/steps/__tests__/ContextGatheringStep.test.tsx` * `app/src/pages/onboarding/steps/__tests__/WelcomeStep.test.tsx` * `app/src/providers/__tests__/ChatRuntimeProvider.test.tsx` * `app/src/providers/__tests__/CoreStateProvider.test.tsx` * `app/src/providers/__tests__/SocketProvider.test.tsx` * `app/src/services/__tests__/coreRpcClient.test.ts` * `app/src/services/api/__tests__/skillsApi.test.ts` * `app/src/services/api/skillsApi.ts` * `app/src/services/notificationService.ts` * `app/src/services/webviewAccountService.ts` * `app/src/store/__tests__/accountsSlice.core.test.ts` * `app/src/store/__tests__/deepLinkAuthState.test.ts` * `app/src/store/__tests__/notificationSlice.test.ts` * `app/src/store/__tests__/threadSlice.test.ts` * `app/src/store/index.ts` * `app/src/store/notificationSlice.ts` * `app/src/store/notificationsSlice.ts` * `app/src/types/notifications.ts` * `app/src/utils/config.ts` * `app/src/utils/tauriCommands/aboutApp.ts` * `app/src/utils/tauriCommands/index.ts` * `app/test/e2e/helpers/artifacts.ts` * `app/test/e2e/specs/agent-review.spec.ts` * `app/test/e2e/specs/cron-jobs-flow.spec.ts` * `app/test/e2e/specs/login-flow.spec.ts` * `app/test/e2e/specs/webhooks-ingress-flow.spec.ts` * `app/test/e2e/specs/webhooks-tunnel-flow.spec.ts` * `app/test/wdio.conf.ts` * `docs/AGENT-OBSERVABILITY.md` * `docs/E2E-TESTING.md` * `docs/SUMMARY.md` * `docs/homebrew-core.md` * `docs/install.md` * `docs/referral-doc.md` * `packages/homebrew-core/openhuman.rb` * `packages/homebrew-core/openhuman.rb.in` * `scripts/install.sh` * `scripts/release/render-homebrew-core-formula.sh` * `scripts/setup-dev-codesign.sh` * `src/core/agent_cli.rs` * `src/core/all.rs` * `src/core/cli.rs` * `src/core/dispatch.rs` * `src/core/event_bus/events.rs` * `src/core/jsonrpc.rs` * `src/core/mod.rs` * `src/core/screen_intelligence_cli.rs` * `src/openhuman/about_app/catalog.rs` * `src/openhuman/about_app/mod.rs` * `src/openhuman/about_app/types.rs` * `src/openhuman/agent/agents/archivist/prompt.rs` * `src/openhuman/agent/agents/code_executor/prompt.rs` * `src/openhuman/agent/agents/critic/prompt.rs` * `src/openhuman/agent/agents/integrations_agent/prompt.rs` * `src/openhuman/agent/agents/loader.rs` * `src/openhuman/agent/agents/morning_briefing/prompt.rs` * `src/openhuman/agent/agents/orchestrator/prompt.rs` * `src/openhuman/agent/agents/planner/prompt.rs` * `src/openhuman/agent/agents/researcher/prompt.rs` * `src/openhuman/agent/agents/summarizer/prompt.rs` * `src/openhuman/agent/agents/tool_maker/prompt.rs` * `src/openhuman/agent/agents/tools_agent/prompt.rs` * `src/openhuman/agent/agents/trigger_reactor/prompt.rs` * `src/openhuman/agent/agents/trigger_triage/prompt.rs` * `src/openhuman/agent/agents/welcome/prompt.rs` * `src/openhuman/agent/debug/dump_writer.rs` * `src/openhuman/agent/debug/mod.rs` * `src/openhuman/agent/harness/definition.rs` * `src/openhuman/agent/harness/session/turn.rs` * `src/openhuman/agent/harness/subagent_runner/mod.rs` * `src/openhuman/agent/harness/subagent_runner/ops.rs` * `src/openhuman/agent/harness/subagent_runner/tool_prep.rs` * `src/openhuman/agent/mod.rs` * `src/openhuman/agent/prompts/connected_identities.rs` * `src/openhuman/agent/prompts/mod.rs` * `src/openhuman/agent/prompts/types.rs` * `src/openhuman/agent/triage/envelope.rs` * `src/openhuman/agent/triage/evaluator.rs` * `src/openhuman/billing/ops.rs` * `src/openhuman/composio/ops.rs` * `src/openhuman/composio/providers/gmail/provider.rs` * `src/openhuman/composio/providers/mod.rs` * `src/openhuman/composio/providers/notion/provider.rs` * `src/openhuman/composio/providers/profile.rs` * `src/openhuman/composio/providers/traits.rs` * `src/openhuman/composio/providers/types.rs` * `src/openhuman/config/schema/load.rs` * `src/openhuman/context/mod.rs` * `src/openhuman/learning/prompt_sections.rs` * `src/openhuman/memory/tree/global_tree/digest.rs` * `src/openhuman/memory/tree/global_tree/mod.rs` * `src/openhuman/memory/tree/global_tree/recap.rs` * `src/openhuman/memory/tree/global_tree/registry.rs` * `src/openhuman/memory/tree/global_tree/seal.rs` * `src/openhuman/memory/tree/ingest.rs` * `src/openhuman/memory/tree/mod.rs` * `src/openhuman/memory/tree/score/extract/llm.rs` * `src/openhuman/memory/tree/score/extract/mod.rs` * `src/openhuman/memory/tree/score/extract/regex.rs` * `src/openhuman/memory/tree/score/extract/types.rs` * `src/openhuman/memory/tree/score/mod.rs` * `src/openhuman/memory/tree/score/resolver.rs` * `src/openhuman/memory/tree/score/signals/mod.rs` * `src/openhuman/memory/tree/score/signals/ops.rs` * `src/openhuman/memory/tree/score/signals/types.rs` * `src/openhuman/memory/tree/score/store.rs` * `src/openhuman/memory/tree/source_tree/bucket_seal.rs` * `src/openhuman/memory/tree/source_tree/flush.rs` * `src/openhuman/memory/tree/source_tree/mod.rs` * `src/openhuman/memory/tree/source_tree/registry.rs` * `src/openhuman/memory/tree/source_tree/store.rs` * `src/openhuman/memory/tree/source_tree/summariser/inert.rs` * `src/openhuman/memory/tree/source_tree/summariser/mod.rs` * `src/openhuman/memory/tree/source_tree/types.rs` * `src/openhuman/memory/tree/store.rs` * `src/openhuman/memory/tree/topic_tree/backfill.rs` * `src/openhuman/memory/tree/topic_tree/curator.rs` * `src/openhuman/memory/tree/topic_tree/hotness.rs` * `src/openhuman/memory/tree/topic_tree/mod.rs` * `src/openhuman/memory/tree/topic_tree/registry.rs` * `src/openhuman/memory/tree/topic_tree/routing.rs` * `src/openhuman/memory/tree/topic_tree/store.rs` * `src/openhuman/memory/tree/topic_tree/types.rs` * `src/openhuman/mod.rs` * `src/openhuman/notifications/mod.rs` * `src/openhuman/notifications/rpc.rs` * `src/openhuman/notifications/schemas.rs` * `src/openhuman/notifications/store.rs` * `src/openhuman/notifications/types.rs` * `src/openhuman/screen_intelligence/cli/capture.rs` * `src/openhuman/screen_intelligence/cli/doctor.rs` * `src/openhuman/screen_intelligence/cli/mod.rs` * `src/openhuman/screen_intelligence/cli/server.rs` * `src/openhuman/screen_intelligence/cli/session.rs` * `src/openhuman/screen_intelligence/engine.rs` * `src/openhuman/screen_intelligence/mod.rs` * `src/openhuman/skills/mod.rs` * `src/openhuman/skills/ops.rs` * `src/openhuman/skills/schemas.rs` * `src/openhuman/team/ops.rs` * `src/openhuman/text_input/cli.rs` * `src/openhuman/text_input/mod.rs` * `src/openhuman/threads/mod.rs` * `src/openhuman/threads/ops.rs` * `src/openhuman/threads/schemas.rs` * `src/openhuman/threads/title.rs` * `src/openhuman/tool_timeout/mod.rs` * `src/openhuman/tree_summarizer/cli.rs` * `src/openhuman/tree_summarizer/mod.rs` * `src/openhuman/voice/cli.rs` * `src/openhuman/voice/mod.rs` * `src/rpc/mod.rs` * `tests/json_rpc_e2e.rs` </details> <details> <summary>💤 Files with no reviewable changes (2)</summary> * src/openhuman/context/mod.rs * src/core/screen_intelligence_cli.rs </details> <details> <summary>✅ Files skipped from review due to trivial changes (41)</summary> * .gitignore * .claude/rules/15-settings-modal-system.md * app/src/pages/onboarding/components/OnboardingNextButton.tsx * src/openhuman/agent/agents/tool_maker/prompt.rs * app/src/utils/tauriCommands/index.ts * .claude/rules/05-platform-setup-android.md * src/openhuman/agent/agents/planner/prompt.rs * .claude/rules/16-macos-background-execution.md * src/openhuman/composio/providers/mod.rs * .claude/rules/06-platform-setup-ios.md * app/src/components/settings/SettingsHome.tsx * .github/workflows/installer-smoke.yml * app/src/lib/nativeNotifications/index.ts * .claude/rules/09-permissions-capabilities.md * .claude/rules/12-design-system.md * src/openhuman/agent/harness/subagent_runner/mod.rs * .claude/rules/04-platform-setup-macos.md * .claude/rules/07-rust-backend-guide.md * src/openhuman/agent/mod.rs * app/.env.example * app/scripts/e2e-run-all-flows.sh * .claude/rules/17-skills-memory-inference-flow.md * .claude/rules/03-platform-setup-windows.md * docs/SUMMARY.md * .claude/rules/08-frontend-guide.md * docs/homebrew-core.md * .env.example * app/src-tauri/vendor/tauri-cef * .claude/rules/14-deep-link-platform-guide.md * docs/E2E-TESTING.md * src/openhuman/memory/tree/mod.rs * .claude/rules/13-backend-auth-implementation.md * src/openhuman/about_app/mod.rs * src/openhuman/composio/providers/types.rs * docs/install.md * app/src/utils/config.ts * app/src/features/privacy/whatLeavesItems.ts * app/src-tauri/src/cdp/emulation.rs * app/src/types/notifications.ts * scripts/setup-dev-codesign.sh * src/openhuman/agent/harness/subagent_runner/tool_prep.rs </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (2)</summary> * app/src/components/rewards/RewardsCommunityTab.tsx * app/src/pages/__tests__/Rewards.test.tsx </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+495
to
+529
| <claude-mem-context> | ||
| # Memory Context | ||
|
|
||
| # [openhuman] recent context, 2026-04-22 9:52am PDT | ||
|
|
||
| Legend: 🎯session 🔴bugfix 🟣feature 🔄refactor ✅change 🔵discovery ⚖️decision | ||
| Format: ID TIME TYPE TITLE | ||
| Fetch details: get_observations([IDs]) | Search: mem-search skill | ||
|
|
||
| Stats: 20 obs (8,333t read) | 593,112t work | 99% savings | ||
|
|
||
| ### Apr 22, 2026 | ||
| 2848 9:07a ✅ openhuman: All Three Review Branches Pushed to Fork Successfully | ||
| 2849 " 🔵 openhuman review-daemon-lifecycle: Two Post-Push Issues — Unstaged Prettier Changes + Missing tauri-cef Vendor | ||
| 2851 9:08a ✅ openhuman daemon lifecycle: Prettier Format Committed as Follow-Up | ||
| 2855 9:09a ✅ openhuman: All Three Review Branches Fully Pushed — PRs Ready to Open | ||
| 2857 9:10a 🔵 openhuman: GitHub Connector Cannot Create PRs to tinyhumansai/openhuman — 403 Forbidden | ||
| 2858 9:11a 🔵 openhuman webhooks-ingress: Session Stalled — Instruction Not Processed After 10+ Minutes | ||
| 2860 " 🔵 openhuman webhooks: WebhooksDebugPanel Architecture for E2E Smoke Spec | ||
| 2861 9:13a 🔵 openhuman webhooks-ingress: Full Spec Surface Mapped — RPC Log Strings + UI Navigation Path | ||
| 2866 9:15a 🟣 openhuman webhooks-ingress: webhooks-ingress-flow.spec.ts Written | ||
| 2869 9:18a ⚖️ openhuman Memory Refactor Plan: Trait Shape, L1 Pointer, and Missing Pieces | ||
| 2871 " 🔵 openhuman Memory Architecture: Auto-Inject Pattern Has 3 Separate Implementations | ||
| 2873 9:31a 🟣 openhuman: Draft PR Opened — Config Runtime Dir Refactor for Testability | ||
| 2874 9:32a 🟣 openhuman: 3 More Draft PRs Opened — Threads Schema, Daemon Lifecycle, Webhooks E2E | ||
| 2875 9:33a 🔵 openhuman Memory Namespace: 3 Auto-Inject Sites, Not 1 | ||
| 2876 " ⚖️ openhuman Memory Refactor: Breaking Trait Change + Flag-Off + ToolDiscovery Hybrid | ||
| 2877 " ✅ Memory Namespace Refactor Plan Written to docs/plans/memory-namespace-refactor.md | ||
| 2879 9:34a 🔵 openhuman Memory Trait: 15 Impls, Not 14; MemoryRecalled Has No Live Emit Site | ||
| 2880 " 🔵 openhuman SQLite Schema: memory_docs Already Has namespace Column; Migration Scope Minimal | ||
| 2881 " 🔵 openhuman Memory Trait Current Signatures: No Namespace Param on Any Method | ||
| 2882 " 🔵 openhuman Eval Infra: Does Not Exist; Phase D Requires Bootstrap from Scratch | ||
|
|
||
| Access 593k tokens of past work via get_observations([IDs]) or mem-search skill. | ||
| </claude-mem-context> No newline at end of file |
Contributor
There was a problem hiding this comment.
Embedding session-specific memory context in canonical documentation creates maintenance issues.
The <claude-mem-context> block contains dated, session-specific memory observations (Apr 22, 2026) embedded directly in AGENTS.md, the canonical agent instruction document. This pattern has several problems:
- Rapid staleness: The observations and statistics will become outdated with every new session
- Merge conflicts: Multiple PRs updating this section simultaneously will conflict
- Documentation pollution:
AGENTS.mdshould define stable patterns and rules, not carry ephemeral session state - Version control noise: Memory updates will generate commit churn in the main documentation file
Recommendation: Keep memory context external (.claude/memory/, separate files, or runtime-injected) rather than embedding it in the repository's source-controlled agent documentation.
If this is intentional for a specific workflow, please document the rationale and maintenance strategy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` around lines 495 - 529, The AGENTS.md file contains an ephemeral
<claude-mem-context> block (session-specific observations/stats) that will
stale, create merge conflicts, and pollute docs; remove this embedded block and
instead move session memory to an external, non-source-controlled location
(e.g., .claude/memory/ or per-session files) or make it runtime-injected, update
any references to <claude-mem-context> to load from that external source, and if
keeping it in-repo is intentional add a short rationale and maintenance strategy
to AGENTS.md explaining why and how it will be updated.
Comment on lines
+318
to
+343
| if let Some(state) = app.try_state::<WebviewAccountsState>() { | ||
| let prefs = state.notification_bypass.lock().unwrap().clone(); | ||
| if prefs.global_dnd { | ||
| log::debug!( | ||
| "[notify-bypass][{}] suppressed global_dnd provider={}", | ||
| account_id, | ||
| provider | ||
| ); | ||
| return; | ||
| } | ||
| if prefs.muted_accounts.contains(account_id) { | ||
| log::debug!( | ||
| "[notify-bypass][{}] suppressed muted_account provider={}", | ||
| account_id, | ||
| provider | ||
| ); | ||
| return; | ||
| } | ||
| if prefs.bypass_when_focused && prefs.focused_account.as_deref() == Some(account_id) { | ||
| log::debug!( | ||
| "[notify-bypass][{}] suppressed focused_account provider={}", | ||
| account_id, | ||
| provider | ||
| ); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Bypass checks are suppressing too much here.
These early returns happen before webview-notification:fired, so DND/mute/focused bypasses currently drop the in-app notification-center event as well as the OS toast. The focused-account branch is also missing a real main-window focus check, so the same account can stay suppressed even while the app is in the background.
Also applies to: 379-406
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src-tauri/src/webview_accounts/mod.rs` around lines 318 - 343, The bypass
logic in WebviewAccountsState (notification_bypass) currently returns early when
prefs.global_dnd, prefs.muted_accounts, or prefs.bypass_when_focused with
prefs.focused_account matches account_id, causing the code to drop the in-app
"webview-notification:fired" event as well as the OS toast; move or refactor
these suppression checks so the webview-notification:fired event is always
emitted first, then suppress the OS/desktop toast afterwards. Also tighten the
focused-account branch: replace the simple string-equality check against
prefs.focused_account with an actual main-window focus check (e.g. verify the
Tauri main window is focused before treating focused_account as a bypass). Apply
the same change for the second identical block around lines 379-406 that uses
the same prefs.* checks.
Comment on lines
+28
to
+50
| useEffect(() => { | ||
| void Promise.all( | ||
| providers.map(async provider => { | ||
| const s = await getNotificationSettings(provider); | ||
| return [provider, s] as const; | ||
| }) | ||
| ) | ||
| .then(rows => { | ||
| const next: Record< | ||
| string, | ||
| { enabled: boolean; importance_threshold: number; route_to_orchestrator: boolean } | ||
| > = {}; | ||
| rows.forEach(([provider, s]) => { | ||
| next[provider] = { | ||
| enabled: s.enabled, | ||
| importance_threshold: s.importance_threshold, | ||
| route_to_orchestrator: s.route_to_orchestrator, | ||
| }; | ||
| }); | ||
| setSettings(next); | ||
| }) | ||
| .catch(() => {}); | ||
| }, [providers]); |
Contributor
There was a problem hiding this comment.
Don't render editable fallback settings when the initial load fails.
If any getNotificationSettings() call rejects, the panel silently falls back to the hard-coded defaults from the render path. That makes a backend failure look like real state, and the next user edit can overwrite the saved settings with bogus values. Surface a load error and keep the controls disabled until the initial fetch succeeds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/settings/panels/NotificationRoutingPanel.tsx` around lines
28 - 50, When the initial fetch in the NotificationRoutingPanel useEffect (the
Promise.all of getNotificationSettings over providers) fails, do not fall back
to editable defaults; instead detect failure (use Promise.allSettled or catch
the rejection) and set a load error flag (e.g., loadError state) while leaving
settings unset (e.g., keep settings as undefined/null) so the UI stays disabled;
only call setSettings(...) when all requests succeed. Update render logic that
uses settings (and the settings variable) to show a load error message and keep
controls disabled until settings is populated and loadError is false. Ensure you
reference the useEffect, getNotificationSettings, providers, setSettings, and
settings variables when making these changes.
Comment on lines
+52
to
+72
| const updateSetting = async ( | ||
| provider: string, | ||
| patch: Partial<{ | ||
| enabled: boolean; | ||
| importance_threshold: number; | ||
| route_to_orchestrator: boolean; | ||
| }> | ||
| ) => { | ||
| const current = settings[provider] ?? { | ||
| enabled: true, | ||
| importance_threshold: 0, | ||
| route_to_orchestrator: true, | ||
| }; | ||
| const next = { ...current, ...patch }; | ||
| setSettings(prev => ({ ...prev, [provider]: next })); | ||
| try { | ||
| await setNotificationSettings({ provider, ...next }); | ||
| } catch (err) { | ||
| setSettings(prev => ({ ...prev, [provider]: current })); | ||
| throw err; | ||
| } |
Contributor
There was a problem hiding this comment.
The optimistic rollback is race-prone for overlapping saves.
current is captured before the async write starts, so a failed older request can roll a provider back after a newer request already succeeded. This is especially easy to hit on the slider because it saves on every change. Track a per-provider request version, debounce the slider, or only revert if the failing request is still the latest one.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/settings/panels/NotificationRoutingPanel.tsx` around lines
52 - 72, The optimistic rollback in updateSetting is race-prone because it
captures current and may revert a newer successful change when an older async
save fails; modify updateSetting to use a per-provider request sequence (e.g., a
ref or a version map keyed by provider) that you increment before firing
setNotificationSettings, attach the current version to the pending request, and
on error only revert settings if the stored latest version still equals that
request's version; reference updateSetting, setSettings,
setNotificationSettings, and settings when implementing the per-provider version
check (alternatively debounce the slider but ensure the rollback guard uses the
request/version comparison).
Comment on lines
+150
to
+159
| try { | ||
| const created = await skillsApi.createSkill(payload); | ||
| log('submit-ok id=%s', created.id); | ||
| onCreated(created); | ||
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| log('submit-err %s', message); | ||
| setError(message); | ||
| setSubmitting(false); | ||
| } |
Contributor
There was a problem hiding this comment.
Clear submitting on success, or close the modal here.
The success path never resets submitting. If onCreated does not synchronously unmount this component, every close path remains disabled and the dialog stays stuck in "Creating…".
Possible fix
try {
const created = await skillsApi.createSkill(payload);
log('submit-ok id=%s', created.id);
+ setSubmitting(false);
onCreated(created);
} catch (err) {📝 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
| try { | |
| const created = await skillsApi.createSkill(payload); | |
| log('submit-ok id=%s', created.id); | |
| onCreated(created); | |
| } catch (err) { | |
| const message = err instanceof Error ? err.message : String(err); | |
| log('submit-err %s', message); | |
| setError(message); | |
| setSubmitting(false); | |
| } | |
| try { | |
| const created = await skillsApi.createSkill(payload); | |
| log('submit-ok id=%s', created.id); | |
| setSubmitting(false); | |
| onCreated(created); | |
| } catch (err) { | |
| const message = err instanceof Error ? err.message : String(err); | |
| log('submit-err %s', message); | |
| setError(message); | |
| setSubmitting(false); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/skills/CreateSkillModal.tsx` around lines 150 - 159, The
submit success branch in CreateSkillModal never clears the submitting state, so
if onCreated doesn't unmount the modal the UI remains stuck; after the await
skillsApi.createSkill(payload) and before or immediately after calling
onCreated(created), call setSubmitting(false) (or close the modal) to reset the
submitting flag so the dialog can be closed and buttons re-enabled; update the
success path around the created handling in the submit routine to ensure
setSubmitting(false) is always executed on success.
Comment on lines
63
to
+69
| "screen-intelligence" => { | ||
| crate::core::screen_intelligence_cli::run_screen_intelligence_command(&args[1..]) | ||
| crate::openhuman::screen_intelligence::cli::run_screen_intelligence_command(&args[1..]) | ||
| } | ||
| "voice" | "dictate" => run_voice_server_command(&args[1..]), | ||
| "text-input" => crate::core::text_input_cli::run_text_input_command(&args[1..]), | ||
| "voice" | "dictate" => crate::openhuman::voice::cli::run_standalone_subcommand(&args[1..]), | ||
| "text-input" => crate::openhuman::text_input::cli::run_text_input_command(&args[1..]), | ||
| "tree-summarizer" => { | ||
| crate::core::tree_summarizer_cli::run_tree_summarizer_command(&args[1..]) | ||
| crate::openhuman::tree_summarizer::cli::run_tree_summarizer_command(&args[1..]) |
Contributor
There was a problem hiding this comment.
Registry-only CLI routing is still being bypassed here.
These direct domain dispatches keep src/core/cli.rs responsible for product-specific subcommands, which is exactly the coupling the repo rules forbid. Please move this routing behind the controller registry / registered handlers instead of growing more hard-coded branches here.
As per coding guidelines: "Do not add domain branches in src/core/cli.rs or src/core/jsonrpc.rs. Expose features to CLI and JSON-RPC only via the controller registry."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/cli.rs` around lines 63 - 69, The top-level CLI match in cli.rs is
directly dispatching product domains (run_screen_intelligence_command,
run_standalone_subcommand, run_text_input_command, run_tree_summarizer_command)
which bypasses the controller registry; refactor by removing these hard-coded
branches and instead look up and invoke the appropriate registered
controller/handler from the controller registry (use the registry's
lookup/dispatch API) so CLI routing delegates to registered handlers; update the
match to call the registry dispatch method with the subcommand name and args and
ensure each domain command is registered with the registry instead of being
called directly.
Comment on lines
+83
to
+90
| if let Some(existing) = find_existing_daily(config, &global.id, day_start, day_end)? { | ||
| log::info!( | ||
| "[global_tree::digest] daily already exists for {day} id={} — skipping", | ||
| existing.id | ||
| ); | ||
| return Ok(DigestOutcome::Skipped { | ||
| existing_id: existing.id, | ||
| }); |
Contributor
There was a problem hiding this comment.
Don't skip repair when the daily row exists but never reached the buffer.
If Lines 171-187 commit the SummaryNode and append_daily_and_cascade then fails, the next run hits Lines 83-90 and returns Skipped without ever enqueueing that daily id into L0. From there, weekly/monthly sealing is permanently short by one day. Either make the initial insert + L0 append atomic, or have the existing-row path reconcile buffer/cascade state before returning Skipped.
Also applies to: 171-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/memory/tree/global_tree/digest.rs` around lines 83 - 90, The
current early-return in the find_existing_daily path causes completed-summary
rows that never reached the L0 buffer to be treated as fully done; update the
existing-row branch in digest.rs (the find_existing_daily and
DigestOutcome::Skipped path) to verify/reconcile buffer+cascade state before
returning: after finding an existing daily row, check whether it was
enqueued/appended (e.g. via the same marker/state used by
append_daily_and_cascade) and if not, either (a) enqueue the daily id into L0 /
call append_daily_and_cascade for that id, or (b) perform the insert+L0-append
in a transaction so both succeed or both roll back; implement the reconciliation
logic in the existing-row code path so it enqueues missing daily ids or
completes cascade sealing before returning Skipped.
Comment on lines
+287
to
+290
| let chosen_id = match intersecting_id { | ||
| Some(id) => Some(id), | ||
| None => source_tree.root_id.clone(), | ||
| }; |
Contributor
There was a problem hiding this comment.
Stale source roots will make every later day look populated.
Line 289 falls back to source_tree.root_id even when no summary intersects [day_start, day_end). That means a source with only old activity still contributes to future digests, so EmptyDay stops being reachable once any source has a root summary. Return None here until there is day-overlapping material, or implement the raw-L0 fallback described in the docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/memory/tree/global_tree/digest.rs` around lines 287 - 290, The
current match that sets chosen_id uses source_tree.root_id when intersecting_id
is None, causing stale roots to appear in future digests; change the logic in
digest.rs so that when intersecting_id is None you return None for chosen_id (or
implement the documented raw-L0 fallback) instead of falling back to
source_tree.root_id, i.e., update the match around intersecting_id/chosen_id to
only populate chosen_id when there is day-overlapping material.
Comment on lines
+67
to
+76
| let covering = pick_covering(&all_at_level, window_start, now); | ||
| if covering.is_empty() { | ||
| continue; | ||
| } | ||
| log::debug!( | ||
| "[global_tree::recap] using level={} summaries={}", | ||
| level, | ||
| covering.len() | ||
| ); | ||
| return Ok(Some(assemble_recap(&covering, level))); |
Contributor
There was a problem hiding this comment.
Don't stop at an old higher-level summary when fresher lower-level nodes exist.
pick_covering() returns the latest summary when nothing at level overlaps the requested window, and Lines 71-76 immediately return it. With an old L1 plus recent L0 dailies, a 7-day recap will ignore the recent activity and surface stale weekly text instead of falling back to L0. Only use the "latest summary" fallback after lower levels have also failed to provide overlap.
Also applies to: 104-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/memory/tree/global_tree/recap.rs` around lines 67 - 76, The
code currently returns the "latest summary" from pick_covering immediately when
covering isn't empty, which lets an old higher-level summary override fresher
lower-level nodes; change the logic in the loop that calls pick_covering so that
if pick_covering returned only the latest-summary fallback (i.e., a
non-overlapping summary) you do not return it immediately but instead continue
scanning lower levels for overlapping nodes, and only after all lower levels
have been checked and none provided overlap should you assemble_recap and return
that latest-summary fallback; apply the same fix to the analogous block handling
levels around the assemble_recap call at the other location mentioned (the block
at lines 104-120) and use the pick_covering result (covering) and level
variables to distinguish "exact overlap" vs "latest-summary fallback" before
returning.
Comment on lines
+64
to
+66
| /// Transactionally append a single summary id to the buffer at | ||
| /// `(tree_id, level)`. Idempotent on the `(tree_id, level, item_id)` tuple | ||
| /// so retries of a partially-applied digest don't double-count. |
Contributor
There was a problem hiding this comment.
The retry guarantee breaks after the first seal.
Line 78 only de-dupes against the current buffer contents. Once a threshold seal runs, Lines 253-263 clear that buffer and move the aggregate upward, so retrying the same daily_summary.id appends it again even though it's already included in the sealed parent summary. That double-counts the day and can trigger premature higher-level seals. Persist this idempotency check outside live buffer state before claiming retries are safe.
Also applies to: 75-94, 253-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/memory/tree/global_tree/seal.rs` around lines 64 - 66, The
current idempotency (in the append routine around lines 75-94) only checks the
in-memory/current buffer and is lost when the sealing path (lines ~253-263)
clears the buffer, allowing duplicate appends after a seal; fix by persisting
seen-summary ids outside the transient buffer: add a durable "seen" check and
record (e.g., a sealed_summary_ids set/row) that is consulted in
append_summary_to_buffer (or the method that currently de-dupes the buffer) and
updated transactionally when claiming/appending or when performing the seal
move-up operation; ensure the seal path also marks moved summary ids as
persisted so retries will be no-ops, and make all checks/updates atomic in the
same transaction to avoid race conditions.
Member
|
bro we cannot have PRs like this... 240 files is almost impossible to review. and there's many breaking changes. pls break into smaller PRs |
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
2 participants
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
LATEST_APP_DOWNLOAD_URL)navigator.shareis unavailable or fails → copy full share text to clipboardhandleCopyfallbackReferralRewardsSectionVitest suiteRewards.test.tsxcoverage for copy/share/fallback flowsProblem
Previous implementation relied on
referralLink:Fallback issue:
handleCopy()was usedSolution
Standardize sharing around
referralCode:Create unified
shareText:Remove referral link display from UI:
Centralize download URL:
LATEST_APP_DOWNLOAD_URLmoved toconfig.tsSubmission Checklist
Unit tests
ReferralRewardsSection.test.tsxRewards.test.tsxE2E / Integration
Doc comments
Inline comments
Impact
Platform
Backend
referralCodealready supportedCompatibility
referralLinkremoved from UI only (still available backend-side)UX
Related
Summary by CodeRabbit
New Features
Updates
Tests