test(intelligence): backfill memory-tab unit tests (#1870)#2064
Conversation
…nyhumansai#1870) Issue tinyhumansai#1870 — PR tinyhumansai#1518 (i18n) surfaced a coverage gap: many intelligence/ memory components had no tests at all. Add per-component RTL + Vitest suites for the Memory tab group so the coverage gate stops blocking on these files. Coverage added for: - ActionableCard - IntelligenceDreamsTab - IntelligenceMemoryTab - MemoryEmptyPlaceholder - MemoryGraph (openUrl mock, summary deep-link assertions) - MemoryHeatmap (event totals, hover tooltip, loading) - MemoryInsights (category grouping, expand/collapse, badges) - MemoryNavigator (search, section toggle, source/entity selection) - MemoryResultList (time-bucket grouping, active row, click) - MemoryStatsBar (number/byte/time-ago formatting, '--' fallbacks) All 171 tests in app/src/components/intelligence/ pass; typecheck + lint pass on changed files.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds Vitest + React Testing Library test suites across intelligence and memory UI components, covering rendering, user interactions, time-bucketing, visualization behaviors, and relation/grouping logic. ChangesIntelligence Component Test Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ 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
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
app/src/components/intelligence/MemoryEmptyPlaceholder.test.tsx (1)
15-21: ⚡ Quick winReduce structure-coupled checks in the conditional-mount test.
Lines 18-21 verify exact tags (
DIV,h2,p), which are likely to churn without behavioral impact. Prefer role/text-based assertions scoped to the testid container.As per coding guidelines: "Prefer testing behavior over implementation details."
🤖 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/intelligence/MemoryEmptyPlaceholder.test.tsx` around lines 15 - 21, The test in MemoryEmptyPlaceholder.test.tsx is too coupled to DOM structure; instead of asserting root.tagName and querying 'h2'/'p', use role/text-based assertions scoped to the container returned by screen.getByTestId('memory-empty-placeholder') (root) — for example use within(root).getByRole('heading', { level: 2 }) or within(root).getByText(...) and within(root).getByText(...) for the body copy; remove the expect(root.tagName).toBe('DIV') and the querySelector assertions and replace them with these behavior-focused checks.app/src/components/intelligence/IntelligenceDreamsTab.test.tsx (1)
11-12: ⚡ Quick winPrefer behavior assertions over DOM tag-count assertions.
Line 12 couples the test to internal markup (
<p>count), which is brittle. Assert rendered copy/visible content instead.As per coding guidelines: "Prefer testing behavior over implementation details."
🤖 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/intelligence/IntelligenceDreamsTab.test.tsx` around lines 11 - 12, The test currently asserts internal markup by counting <p> elements via expect(document.querySelectorAll('p')).toHaveLength(2);; replace that with behavior-oriented assertions that verify the actual rendered copy is visible—use the IntelligenceDreamsTab test utilities (e.g., screen.getByText or getByRole/getByLabelText) to assert the specific body and footer strings are present and visible (e.g., expect(screen.getByText('...')).toBeInTheDocument()/toBeVisible()) instead of counting DOM tags.app/src/components/intelligence/MemoryStatsBar.test.tsx (1)
60-70: ⚡ Quick winMake the relative-time test deterministic.
Line 61 depends on live wall-clock time. Freeze time with
vi.useFakeTimers()+vi.setSystemTime(...)to avoid time-sensitive flake in CI.As per coding guidelines: "Keep tests deterministic: avoid ... time-sensitive flakes."
🤖 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/intelligence/MemoryStatsBar.test.tsx` around lines 60 - 70, The test in MemoryStatsBar.test.tsx uses Date.now() (const now) making it time-dependent; make it deterministic by installing fake timers and setting a fixed system time before computing `now` (use vi.useFakeTimers() and vi.setSystemTime(fixedTimestamp)) and restore real timers after the test (vi.useRealTimers()); apply this setup/teardown around the test that renders <MemoryStatsBar ... oldestDocTimestamp={now - 3600 * 5} newestDocTimestamp={now - 60 * 10} /> so the expectations for "5h ago" and "10m ago" are stable in CI.app/src/components/intelligence/MemoryResultList.test.tsx (1)
56-56: ⚡ Quick winPrefer role/state assertions over DOM/CSS internals.
Line 56 and Lines 68-72 currently depend on
data-*selectors andis-activeclass. These are brittle against markup/style refactors; use accessibility state assertions as primary contract.Suggested patch
- expect(document.querySelectorAll('button[data-chunk-id]').length).toBe(4); + expect(screen.getAllByRole('button', { pressed: false })).toHaveLength(4); @@ - const { container } = render( + render( <MemoryResultList chunks={chunks} selectedChunkId="b" onSelectChunk={() => {}} /> ); - const active = container.querySelector('button[data-chunk-id="b"]') as HTMLButtonElement; - expect(active).not.toBeNull(); - expect(active.getAttribute('aria-pressed')).toBe('true'); - expect(active.className).toContain('is-active'); + const active = screen.getByRole('button', { pressed: true }); + expect(active).toHaveAttribute('data-chunk-id', 'b');As per coding guidelines, "Prefer testing behavior over implementation details."
Also applies to: 68-72
🤖 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/intelligence/MemoryResultList.test.tsx` at line 56, Tests in MemoryResultList.test.tsx currently assert DOM internals (button[data-chunk-id] and .is-active) which is brittle; update the assertions to use accessibility queries and state checks instead: replace document.querySelectorAll('button[data-chunk-id]') with screen.getAllByRole('button', ...) or getAllByRole('button') filtered by accessible name to count chunk buttons, and replace checks for the is-active class with assertions on an accessibility state (e.g., toHaveAttribute('aria-pressed' or 'aria-current' or 'aria-selected' set to "true") or toHaveAccessibleName) so the tests verify behavior/state (use MemoryResultList.test.tsx and the specific expectations referencing data-chunk-id and is-active).app/src/components/intelligence/MemoryInsights.test.tsx (2)
26-26: ⚡ Quick winPrefer semantic queries over CSS class selectors.
Querying by
.animate-pulsetests an implementation detail that could change if styling is refactored. Consider using semantic queries likegetByRole('status'), checking foraria-busy, or adding adata-testidto the skeleton container.♻️ Example with semantic query
- expect(container.querySelectorAll('.animate-pulse').length).toBeGreaterThanOrEqual(3); + expect(screen.getAllByRole('status').length).toBeGreaterThanOrEqual(3);Or if the skeleton has a container with accessible attributes:
- expect(container.querySelectorAll('.animate-pulse').length).toBeGreaterThanOrEqual(3); + expect(screen.getByLabelText(/loading/i)).toBeInTheDocument();As per coding guidelines: "Prefer testing behavior over implementation details."
🤖 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/intelligence/MemoryInsights.test.tsx` at line 26, The test in MemoryInsights.test.tsx is querying the implementation-specific CSS class '.animate-pulse'; replace that with a semantic or stable selector: update the skeleton component (used by MemoryInsights or its test render) to expose an accessibility attribute or test id (e.g., role="status" or aria-busy="true" or data-testid="memory-insights-skeleton"), then change the assertion in the test to queryByRole/getByRole('status') or getByTestId('memory-insights-skeleton') and assert its presence/quantity instead of using container.querySelectorAll('.animate-pulse').length.
113-113: 💤 Low valueConsider querying badge independently rather than via
parentElement.Accessing
.parentElementcouples the test to the specific DOM structure. If the component wraps the subject differently, this assertion will break. Consider querying for the badge directly (e.g., by role or test ID) or verifying both subject and badge appear without assuming their container relationship.♻️ Alternative approach
If badges have a specific role or test ID:
- const subj = screen.getByText('Alice'); - expect(within(subj.parentElement as HTMLElement).getByText(/person/i)).toBeInTheDocument(); + expect(screen.getByText('Alice')).toBeInTheDocument(); + expect(screen.getByText(/person/i)).toBeInTheDocument();Or scope more broadly to the entire relation item if it has a container with test ID.
As per coding guidelines: "Prefer testing behavior over implementation details."
🤖 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/intelligence/MemoryInsights.test.tsx` at line 113, The assertion is coupling to DOM structure by using within(subj.parentElement); update the test in MemoryInsights.test.tsx to query the badge directly instead of navigating to subj.parentElement — for example locate the badge by role, accessible name, or a data-testid (e.g., queryByRole('status')/getByTestId('relation-badge') or scope to the relation container element like getByTestId('relation-item') and then assert the badge exists) and then assert that it is in the document; replace the within(subj.parentElement as HTMLElement).getByText(/person/i) call with a direct query for the badge and keep the existing subj assertion.
🤖 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/intelligence/IntelligenceMemoryTab.test.tsx`:
- Around line 86-90: The test currently only checks for a level-2 heading which
is ambiguous across different state branches; update the tests that call
renderTab (e.g., the 'renders the loading state when itemsLoading and no memory
data yet' test and the other mentioned tests) to assert the specific state text
or i18n key that each branch renders (for example assert the exact loading
heading text/key when itemsLoading is true, the empty-state text/key when there
are no memories, and the error/state-specific text for other branches) instead
of only checking getByRole('heading', { level: 2 }), so each branch uses a
distinct, branch-specific string assertion to detect regressions.
- Around line 97-103: The test currently clicks the analyze button but only
asserts its presence; update the test to assert that the analyze action was
invoked by mocking/spying on handleAnalyzeNow (or the prop/function passed into
renderTab) and verifying it was called after fireEvent.click(btn). Specifically,
modify the test that uses renderTab({ usingMemoryData: false }) to provide a
jest.fn() spy for handleAnalyzeNow (or the callback prop used by the component)
and replace the final expect(btn).toBeInTheDocument() with an assertion like
expect(handleAnalyzeNowSpy).toHaveBeenCalled()/toHaveBeenCalledTimes(1) to
confirm the analyze action was scheduled.
In `@app/src/components/intelligence/MemoryInsights.test.tsx`:
- Around line 90-92: Replace the brittle container.querySelector('button') usage
in MemoryInsights.test.tsx: locate the header button using an accessible role
query like screen.getByRole('button', { name:
/<CATEGORY_HEADER_TEXT_OR_PATTERN>/i }) instead of querySelector, assign that to
the headerBtn variable (or inline use) and call fireEvent.click on it; update
the name/pattern to match the actual accessible label of the category header
button so the test targets the intended element (replace headerBtn lookup and
subsequent expect/click accordingly).
In `@app/src/components/intelligence/MemoryNavigator.test.tsx`:
- Line 15: Tests in MemoryNavigator.test.tsx use Date.now() for timestamp_ms
(and other assertions) causing flaky behavior; fix by freezing system time for
the test suite: in the test file (around tests that reference timestamp_ms and
other Date.now() usages) call the test framework's fake-timer API (e.g.,
jest.useFakeTimers('modern') / jest.setSystemTime(fixedDate) or equivalent)
before tests run and restore with jest.useRealTimers() after, so all Date.now()
calls return a deterministic fixedDate; apply this in setup/teardown
(beforeAll/afterAll or beforeEach/afterEach) so functions and assertions
referencing timestamp_ms, and any code paths exercised at lines 30 and 155-159,
are stable.
- Around line 174-176: The tests in MemoryNavigator.test.tsx currently assert
any occurrence of "1" or "2" in the entire memory-navigator text which is
brittle; update the assertions to target the specific labeled counters instead
of generic digits. Locate the element retrieved with
screen.getByTestId('memory-navigator') and replace
expect(text).toMatch(/1/)/expect(text).toMatch(/2/) with assertions that match
the labeled counter strings (for example use screen.getByText or
within(screen.getByTestId('memory-navigator')).getByText to assert a pattern
like /Saved memories:\s*1\b/ and /Viewed memories:\s*2\b/ or target specific
counter elements/classes if present, e.g., getByTestId('saved-counter') and
getByTestId('viewed-counter') and assert their text equals "1" and "2"
respectively. Ensure the replacement uses semantic label-based regexes or
explicit element selectors so the test verifies behavior not incidental digits.
In `@app/src/components/intelligence/MemoryResultList.test.tsx`:
- Line 15: The tests reference live time via timestamp_ms and the date-bucket
setup which makes them flaky; fix by freezing system time with Vitest's fake
timers at the start of the suite (use vi.useFakeTimers() and
vi.setSystemTime(fixedDate) in a beforeAll or beforeEach) and restoring real
timers at teardown (vi.useRealTimers() in afterAll/afterEach); apply this to the
tests that set timestamp_ms and the bucket setup blocks so Date.now() and bucket
boundaries are deterministic during the suite.
---
Nitpick comments:
In `@app/src/components/intelligence/IntelligenceDreamsTab.test.tsx`:
- Around line 11-12: The test currently asserts internal markup by counting <p>
elements via expect(document.querySelectorAll('p')).toHaveLength(2);; replace
that with behavior-oriented assertions that verify the actual rendered copy is
visible—use the IntelligenceDreamsTab test utilities (e.g., screen.getByText or
getByRole/getByLabelText) to assert the specific body and footer strings are
present and visible (e.g.,
expect(screen.getByText('...')).toBeInTheDocument()/toBeVisible()) instead of
counting DOM tags.
In `@app/src/components/intelligence/MemoryEmptyPlaceholder.test.tsx`:
- Around line 15-21: The test in MemoryEmptyPlaceholder.test.tsx is too coupled
to DOM structure; instead of asserting root.tagName and querying 'h2'/'p', use
role/text-based assertions scoped to the container returned by
screen.getByTestId('memory-empty-placeholder') (root) — for example use
within(root).getByRole('heading', { level: 2 }) or within(root).getByText(...)
and within(root).getByText(...) for the body copy; remove the
expect(root.tagName).toBe('DIV') and the querySelector assertions and replace
them with these behavior-focused checks.
In `@app/src/components/intelligence/MemoryInsights.test.tsx`:
- Line 26: The test in MemoryInsights.test.tsx is querying the
implementation-specific CSS class '.animate-pulse'; replace that with a semantic
or stable selector: update the skeleton component (used by MemoryInsights or its
test render) to expose an accessibility attribute or test id (e.g.,
role="status" or aria-busy="true" or data-testid="memory-insights-skeleton"),
then change the assertion in the test to queryByRole/getByRole('status') or
getByTestId('memory-insights-skeleton') and assert its presence/quantity instead
of using container.querySelectorAll('.animate-pulse').length.
- Line 113: The assertion is coupling to DOM structure by using
within(subj.parentElement); update the test in MemoryInsights.test.tsx to query
the badge directly instead of navigating to subj.parentElement — for example
locate the badge by role, accessible name, or a data-testid (e.g.,
queryByRole('status')/getByTestId('relation-badge') or scope to the relation
container element like getByTestId('relation-item') and then assert the badge
exists) and then assert that it is in the document; replace the
within(subj.parentElement as HTMLElement).getByText(/person/i) call with a
direct query for the badge and keep the existing subj assertion.
In `@app/src/components/intelligence/MemoryResultList.test.tsx`:
- Line 56: Tests in MemoryResultList.test.tsx currently assert DOM internals
(button[data-chunk-id] and .is-active) which is brittle; update the assertions
to use accessibility queries and state checks instead: replace
document.querySelectorAll('button[data-chunk-id]') with
screen.getAllByRole('button', ...) or getAllByRole('button') filtered by
accessible name to count chunk buttons, and replace checks for the is-active
class with assertions on an accessibility state (e.g.,
toHaveAttribute('aria-pressed' or 'aria-current' or 'aria-selected' set to
"true") or toHaveAccessibleName) so the tests verify behavior/state (use
MemoryResultList.test.tsx and the specific expectations referencing
data-chunk-id and is-active).
In `@app/src/components/intelligence/MemoryStatsBar.test.tsx`:
- Around line 60-70: The test in MemoryStatsBar.test.tsx uses Date.now() (const
now) making it time-dependent; make it deterministic by installing fake timers
and setting a fixed system time before computing `now` (use vi.useFakeTimers()
and vi.setSystemTime(fixedTimestamp)) and restore real timers after the test
(vi.useRealTimers()); apply this setup/teardown around the test that renders
<MemoryStatsBar ... oldestDocTimestamp={now - 3600 * 5} newestDocTimestamp={now
- 60 * 10} /> so the expectations for "5h ago" and "10m ago" are stable in CI.
🪄 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: 974ffc8f-57c2-4d01-8ec8-d5855db7bf05
📒 Files selected for processing (10)
app/src/components/intelligence/ActionableCard.test.tsxapp/src/components/intelligence/IntelligenceDreamsTab.test.tsxapp/src/components/intelligence/IntelligenceMemoryTab.test.tsxapp/src/components/intelligence/MemoryEmptyPlaceholder.test.tsxapp/src/components/intelligence/MemoryGraph.test.tsxapp/src/components/intelligence/MemoryHeatmap.test.tsxapp/src/components/intelligence/MemoryInsights.test.tsxapp/src/components/intelligence/MemoryNavigator.test.tsxapp/src/components/intelligence/MemoryResultList.test.tsxapp/src/components/intelligence/MemoryStatsBar.test.tsx
- IntelligenceMemoryTab: each branch asserts its own English heading text instead of a level-2 heading existence check; the analyze-now test now verifies handleAnalyzeNow was actually invoked. - IntelligenceDreamsTab: assert visible body/footer copy instead of counting <p> tags. - MemoryEmptyPlaceholder: assert heading + body via within(testid), drop the tagName + querySelector coupling. - MemoryInsights: locate the expand header via role+name "Known Facts"; query the entity-type badge with within(subj) instead of reaching through parentElement. - MemoryNavigator: freeze the clock with vi.useFakeTimers in beforeEach so today/this-week bucket counts and timestamp defaults are deterministic; assert "Today 1"/"This Week 2" labels instead of loose /1/, /2/ digit matches. - MemoryResultList: freeze the clock; replace data-chunk-id + is-active class queries with aria-pressed role queries. - MemoryStatsBar: freeze the clock around the relative time-ago test so "5h ago" / "10m ago" stay stable in CI. Skipped per "minimal" guidance: - MemoryInsights skeleton .animate-pulse: requires touching production code to add a test-id; consistent with the existing pattern in the rest of this test file and MemoryStatsBar.
graycyrus
left a comment
There was a problem hiding this comment.
Walkthrough
Solid test backfill for the Intelligence/Memory tab group. All 10 files are well-structured RTL+Vitest suites that follow the existing sibling test patterns (IntelligenceCallsTab.test.tsx, MemorySyncConnections.test.tsx). The author addressed all of CodeRabbit's actionable feedback across two fix commits, including switching to semantic role queries and freezing time in date-dependent tests. No production code changes, no scope drift — this cleanly covers group 1 from #1870.
Two minor consistency notes below; nothing blocking.
Change Summary
| File | Change type | Description |
|---|---|---|
ActionableCard.test.tsx |
Added | Tests render, callbacks (complete/dismiss/snooze), new-badge, sourceLabel |
IntelligenceDreamsTab.test.tsx |
Added | Tests title/description/coming-soon copy, decorative SVG |
IntelligenceMemoryTab.test.tsx |
Added | Tests search/filter inputs, loading/analyzing/empty states, time-group rendering |
MemoryEmptyPlaceholder.test.tsx |
Added | Tests empty title and hint copy |
MemoryGraph.test.tsx |
Added | Tests empty state, SVG rendering, Obsidian deep-link, tooltip, click gating |
MemoryHeatmap.test.tsx |
Added | Tests skeleton, SVG grid, event counting, tooltip, ms-precision timestamps |
MemoryInsights.test.tsx |
Added | Tests skeleton, empty state, predicate grouping, sort order, expand/collapse, entity badges |
MemoryNavigator.test.tsx |
Added | Tests search input, source/entity toggle, section collapse, chunk counting |
MemoryResultList.test.tsx |
Added | Tests empty state, time-bucket sections, aria-pressed, click handler, sender label, id fallback |
MemoryStatsBar.test.tsx |
Added | Tests number formatting, byte formatting, null placeholders, docs-today, relative time, skeleton |
| function makeItem(overrides: Partial<ActionableItem> = {}): ActionableItem { | ||
| const now = new Date(); | ||
| return { | ||
| id: 'item-1', |
There was a problem hiding this comment.
[minor] The makeItem() factory uses live Date.now() for createdAt/updatedAt, and the test at line 15 asserts /10 mins ago/i. The other test files in this PR (MemoryNavigator, MemoryResultList, MemoryStatsBar) all freeze time with vi.useFakeTimers() + vi.setSystemTime() for exactly this kind of relative-time assertion — and the snooze test in this same file (line 62) already uses fake timers.
It's unlikely to flake in practice (the window between factory creation and assertion is microseconds), but freezing time here too would make the pattern consistent across the PR.
// suggestion: add beforeEach/afterEach at describe scope
const FROZEN_NOW = new Date('2026-03-04T12:00:00.000Z');
beforeEach(() => { vi.useFakeTimers(); vi.setSystemTime(FROZEN_NOW); });
afterEach(() => { vi.useRealTimers(); });| // Tooltip is rendered as a fixed-positioned div with z-50. | ||
| expect(container.querySelector('.fixed.z-50')).not.toBeNull(); | ||
| fireEvent.mouseLeave(rect!); | ||
| expect(container.querySelector('.fixed.z-50')).toBeNull(); |
There was a problem hiding this comment.
[minor] The tooltip assertion uses .fixed.z-50 (Tailwind utility classes) to locate the tooltip element. This is the same class-coupling pattern that CodeRabbit flagged for .animate-pulse in the skeleton tests. If the component switches to a different positioning strategy or z-index token, this breaks without any behavioral change.
If the component exposes a data-testid or role for the tooltip (or could be updated to), a semantic query would be more resilient. Not blocking — just noting the consistency gap.
Summary
Backfills unit tests for the Intelligence/Memory tab group flagged by issue #1870 — the coverage gap that PR #1518 (Chinese i18n) surfaced via the changed-lines coverage gate.
This PR takes group 1 (Intelligence/Memory tab) from the suggested per-domain decomposition in #1870. It adds RTL + Vitest suites for every component in that group that had 0–26% coverage:
No production code changes — only new `*.test.tsx` files.
Test plan
Closes part of #1870 — five more domain groups (settings panels, debug panels, top-level pages, onboarding, etc.) remain and should land in follow-up PRs as suggested in the issue.
Summary by CodeRabbit