⚡ Bolt: Memoize filtered analyzed user messages to prevent O(N) re-filtering on every render#337
Conversation
…ltering on every render Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideMemoizes the filtered subset of user messages with mental health analysis and reuses it in render logic to avoid repeated O(N) filtering and condition checks on every render. Sequence diagram for memoized analyzedUserMessages during React rendersequenceDiagram
participant ReactRenderer
participant MentalHealthChatDemo
participant useMemo_analyzedUserMessages
participant MessagesArray
ReactRenderer->>MentalHealthChatDemo: render(messages)
MentalHealthChatDemo->>useMemo_analyzedUserMessages: getAnalyzedUserMessages(messages)
alt messages_reference_changed
useMemo_analyzedUserMessages->>MessagesArray: filter(role === user && mentalHealthAnalysis && !isProcessing)
MessagesArray-->>useMemo_analyzedUserMessages: analyzedUserMessages
useMemo_analyzedUserMessages-->>MentalHealthChatDemo: new analyzedUserMessages
else messages_reference_unchanged
useMemo_analyzedUserMessages-->>MentalHealthChatDemo: cached analyzedUserMessages
end
MentalHealthChatDemo->>MentalHealthChatDemo: renderAnalyzedHistory(analyzedUserMessages.slice(-2))
MentalHealthChatDemo->>MentalHealthChatDemo: renderEmptyStateIf(analyzedUserMessages.length === 0)
MentalHealthChatDemo-->>ReactRenderer: rendered_output
Flow diagram for rendering logic using analyzedUserMessagesflowchart TD
A_start[Start render] --> B_getAnalyzed[Use analyzedUserMessages from useMemo]
B_getAnalyzed --> C_haveAnalyzed{analyzedUserMessages.length > 0}
C_haveAnalyzed -->|Yes| D_slice[Take last two analyzedUserMessages]
D_slice --> E_renderList[Render analysis cards for sliced messages]
C_haveAnalyzed -->|No| F_renderEmpty[Render empty state Card]
E_renderList --> G_end[End render]
F_renderEmpty --> G_end[End render]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughIntroduced a memoized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Memoizing the filtered messages is a good improvement, but it relies on messages being updated immutably (otherwise useMemo can return stale results). Also, getAnalysisHistory() still performs a separate filter/map pass over messages; now that analyzedUserMessages exists, you can centralize the criteria and avoid redundant work.
Additional notes (1)
- Performance |
src/components/MentalHealthChatDemo.tsx:386-394
Now thatanalyzedUserMessagesexists, you can likely eliminate an extra pass overmessagesby deriving analysis history from it (and potentially dropgetAnalysisHistoryentirely). This keeps the “what counts as analyzed” criteria centralized and avoids redundant filtering work whenmessageschanges.
Summary of changes
Summary
- Added a memoized
analyzedUserMessagesarray viauseMemoto cachemessages.filter(...)results. - Updated the Live Analysis Results UI to use
analyzedUserMessagesinstead of inline.filter(...)and replaced an inline.some(...)check withanalyzedUserMessages.length === 0.
Key code paths touched
- Memoization added near
getAnalysisHistory()(useMemo(() => messages.filter(...), [messages])). - Rendering logic now slices/maps
analyzedUserMessages(.slice(-2).map(...)) and uses it for the empty-state condition.
| const analyzedUserMessages = useMemo(() => { | ||
| return messages.filter( | ||
| (m) => m.role === 'user' && m.mentalHealthAnalysis && !m.isProcessing, | ||
| ) | ||
| }, [messages]) |
There was a problem hiding this comment.
useMemo only helps if messages changes by reference when updated. If messages is ever mutated in-place (e.g., messages.push(...) followed by a state set with the same array), this memo can go stale and the UI will fail to update correctly.
Suggestion
Ensure all messages updates are immutable (create a new array each time). For example:
setMessages((prev) => [...prev, nextMessage])Reply with "@CharlieHelps yes please" if you'd like me to add a commit that audits the messages update sites in this component to guarantee immutable updates.
…fig/playwright.config.ts Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Memoizes the subset of user messages that have completed mental health analysis to avoid repeated inline O(N) filtering/existence checks on each render, improving performance as chat histories grow.
Changes:
- Added a
useMemo-backedanalyzedUserMessagesderived frommessages. - Replaced inline
messages.filter(...)rendering logic with the memoized array. - Replaced inline
messages.some(...)empty-state check with a length check on the memoized array.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/components/MentalHealthChatDemo.tsx | Introduces memoized analyzedUserMessages and reuses it for rendering and empty-state logic to reduce render-time work. |
| .github/workflows/browser-tests.yml | Adjusts workflow quoting/formatting and adds an explicit Playwright --config argument for browser tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
…ltering on every render Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/browser-tests.yml">
<violation number="1">
P2: Removing the explicit `--config=config/playwright.config.ts` means Playwright will not load the intended config (there is no root `playwright.config.*`). This can cause the browser tests to run with incorrect defaults or fail to resolve settings from the config.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…ltering on every render Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
…n browser-tests.yml Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="config/test-results/results.json">
<violation number="1" location="config/test-results/results.json:1">
P2: Do not commit generated Playwright JSON results; this file is runtime test output and should be excluded from version control.</violation>
</file>
<file name="config/test-results/junit.xml">
<violation number="1" location="config/test-results/junit.xml:1">
P2: Do not commit generated JUnit test-result artifacts; they cause noisy diffs and can leave stale CI status data in version control.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
config/test-results/results.json (1)
1-467: Avoid checking in generated Playwright result snapshots.This file is machine-generated and bakes in absolute paths, timestamps, and run-specific outcomes. Keeping it under version control will create noisy diffs and stale review signal; it is better kept as a CI artifact and ignored from git.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/test-results/results.json` around lines 1 - 467, The committed file config/test-results/results.json is a generated Playwright test artifact (see "json" reporter with "outputFile": "test-results/results.json") and should be removed from source control; delete config/test-results/results.json from the repo, add an appropriate ignore rule (e.g., config/test-results/ or test-results/*.json) to .gitignore so future runs aren’t committed, and commit the removal; optionally ensure CI uploads the Playwright report artifact instead of checking it in.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/browser-tests.yml:
- Around line 59-66: The workflow step "Run browser tests" currently uses
config/playwright.config.ts which writes Playwright reporter outputs to
config/test-results/results.json, but the subsequent upload step still targets
public/test-results/ so the report job misses files; update the upload action to
include the config/test-results/ directory (or change the Playwright reporter
path to public/test-results/) so the artifacts produced by the Playwright
reporter are uploaded and available to the report job, making sure to reference
the Playwright config at config/playwright.config.ts and the upload step that
currently points to public/test-results/.
- Around line 59-66: The workflow's test-report parsing expects Mocha-style
.failures/.passes arrays but Playwright emits a JSON with a "stats" object;
update the report generation logic (the step that reads FAILED, PASSED, SKIPPED,
TOTAL_TESTS after the "Run browser tests" job) to produce Playwright JSON (add
--reporter=json, e.g. --reporter=json=playwright-report.json to the pnpm exec
playwright command) and replace any jq/grep reads of .failures and .passes with
reads of .stats.unexpected (FAILED), .stats.expected (PASSED), and
.stats.skipped (SKIPPED), then compute TOTAL_TESTS from those three fields;
ensure variable names FAILED, PASSED, SKIPPED, TOTAL_TESTS are used consistently
when rendering the final status.
In `@config/playwright.config.ts`:
- Around line 73-74: The testIgnore glob no longer matches because testDir is
set to '../tests'—update the testIgnore entry in the config (the testIgnore
array in playwright.config.ts) to match paths relative to the new test root;
replace 'tests/accessibility/**' with 'accessibility/**' (or use a more
defensive '**/accessibility/**') so accessibility tests are actually excluded
when testDir is '../tests'.
---
Nitpick comments:
In `@config/test-results/results.json`:
- Around line 1-467: The committed file config/test-results/results.json is a
generated Playwright test artifact (see "json" reporter with "outputFile":
"test-results/results.json") and should be removed from source control; delete
config/test-results/results.json from the repo, add an appropriate ignore rule
(e.g., config/test-results/ or test-results/*.json) to .gitignore so future runs
aren’t committed, and commit the removal; optionally ensure CI uploads the
Playwright report artifact instead of checking it in.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc6250e8-54f0-44c9-82b1-0b70acaa2609
📒 Files selected for processing (5)
.github/workflows/browser-tests.ymlconfig/playwright-report/index.htmlconfig/playwright.config.tsconfig/test-results/junit.xmlconfig/test-results/results.json
✅ Files skipped from review due to trivial changes (1)
- config/test-results/junit.xml
.github/workflows/browser-tests.yml
Outdated
| - name: Run browser tests | ||
| run: | | ||
| export NODE_ENV=test | ||
| export DISABLE_AUTH=true | ||
| export DISABLE_WEB_FONTS=true | ||
| export SKIP_MSW=true | ||
| export BASE_URL=http://localhost:4323 | ||
| pnpm exec playwright test tests/browser/auth.spec.ts tests/browser/cross-browser-compatibility.spec.ts --project=chromium --max-failures=10 --workers=1 | ||
| pnpm exec playwright test tests/browser/auth.spec.ts tests/browser/cross-browser-compatibility.spec.ts --config=config/playwright.config.ts --project=chromium --max-failures=10 --workers=1 |
There was a problem hiding this comment.
Upload config/test-results/ or the report job will miss the reporter output.
Line 66 now activates config/playwright.config.ts, and this PR already shows the JSON reporter writing to config/test-results/results.json. Lines 74-78 still upload public/test-results/, so the downstream report job never receives the Playwright JSON/JUnit files it is supposed to summarize.
📦 Minimal fix
- name: Upload test results
if: always()
uses: actions/upload-artifact@v7.0.0
with:
name: browser-test-results-${{ github.run_id }}
path: |
playwright-report/
+ config/test-results/
public/test-results/Also applies to: 70-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/browser-tests.yml around lines 59 - 66, The workflow step
"Run browser tests" currently uses config/playwright.config.ts which writes
Playwright reporter outputs to config/test-results/results.json, but the
subsequent upload step still targets public/test-results/ so the report job
misses files; update the upload action to include the config/test-results/
directory (or change the Playwright reporter path to public/test-results/) so
the artifacts produced by the Playwright reporter are uploaded and available to
the report job, making sure to reference the Playwright config at
config/playwright.config.ts and the upload step that currently points to
public/test-results/.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
jq '{
has_passes: has("passes"),
has_failures: has("failures"),
stats: .stats
}' config/test-results/results.jsonRepository: daggerstuff/pixelated
Length of output: 288
🏁 Script executed:
cat -n .github/workflows/browser-tests.yml | sed -n '139,181p'Repository: daggerstuff/pixelated
Length of output: 2765
🏁 Script executed:
cat -n .github/workflows/browser-tests.yml | sed -n '59,66p'Repository: daggerstuff/pixelated
Length of output: 551
Update Playwright JSON parsing in test report generation.
Line 66 uses Playwright's test runner, which outputs results to a JSON schema with .stats.expected, .stats.unexpected, and .stats.skipped, but lines 147–148 still parse Mocha-style .failures and .passes arrays that don't exist in the Playwright output. With the current code, FAILED and PASSED both become 0, making TOTAL_TESTS 0, and lines 177–180 incorrectly render "All Tests Passed" even when tests were skipped or the test run produced no parseable results.
Replace the jq and grep parsing with Playwright's actual schema:
Suggested fix
- FAILED=$(jq -r 'try (.failures // []) | length' "$json_file" 2>/dev/null || echo "0")
- PASSED=$(jq -r 'try (.passes // []) | length' "$json_file" 2>/dev/null || echo "0")
+ FAILED=$(jq -r 'try (.stats.unexpected // 0)' "$json_file" 2>/dev/null || echo "0")
+ PASSED=$(jq -r 'try (.stats.expected // 0)' "$json_file" 2>/dev/null || echo "0")
+ SKIPPED=$(jq -r 'try (.stats.skipped // 0)' "$json_file" 2>/dev/null || echo "0")
else
- FAILED=$(grep -o '"failures":\s*\[[^]]*\]' "$json_file" 2>/dev/null | wc -l || echo "0")
- PASSED=$(grep -o '"passes":\s*\[[^]]*\]' "$json_file" 2>/dev/null | wc -l || echo "0")
+ FAILED=$(grep -o '"unexpected":\s*[0-9]\+' "$json_file" 2>/dev/null | grep -o '[0-9]\+' | head -1 || echo "0")
+ PASSED=$(grep -o '"expected":\s*[0-9]\+' "$json_file" 2>/dev/null | grep -o '[0-9]\+' | head -1 || echo "0")
+ SKIPPED=$(grep -o '"skipped":\s*[0-9]\+' "$json_file" 2>/dev/null | grep -o '[0-9]\+' | head -1 || echo "0")
fi
ISSUE_COUNT=$((ISSUE_COUNT + FAILED))
PASSED_TESTS=$((PASSED_TESTS + PASSED))
- TOTAL_TESTS=$((TOTAL_TESTS + FAILED + PASSED))
+ TOTAL_TESTS=$((TOTAL_TESTS + FAILED + PASSED + SKIPPED))
done
...
- if [ "$ISSUE_COUNT" -gt 0 ]; then
+ if [ "$TOTAL_TESTS" -eq 0 ]; then
+ echo '<div class="summary">' >> report.html
+ echo '<h2>⚠️ No Executed Tests</h2>' >> report.html
+ echo '<p>Playwright produced no runnable test results.</p>' >> report.html
+ echo '</div>' >> report.html
+ elif [ "$ISSUE_COUNT" -gt 0 ]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/browser-tests.yml around lines 59 - 66, The workflow's
test-report parsing expects Mocha-style .failures/.passes arrays but Playwright
emits a JSON with a "stats" object; update the report generation logic (the step
that reads FAILED, PASSED, SKIPPED, TOTAL_TESTS after the "Run browser tests"
job) to produce Playwright JSON (add --reporter=json, e.g.
--reporter=json=playwright-report.json to the pnpm exec playwright command) and
replace any jq/grep reads of .failures and .passes with reads of
.stats.unexpected (FAILED), .stats.expected (PASSED), and .stats.skipped
(SKIPPED), then compute TOTAL_TESTS from those three fields; ensure variable
names FAILED, PASSED, SKIPPED, TOTAL_TESTS are used consistently when rendering
the final status.
config/playwright.config.ts
Outdated
| testDir: '../tests', | ||
| testIgnore: ['tests/accessibility/**'], |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Playwright config, how are testIgnoreglob patterns resolved whentestDiris set? IftestDiris../tests, will testIgnore: ['tests/accessibility/']still ignoretests/accessibility/accessibility-audit.spec.ts, or should the pattern be accessibility///accessibility/ instead?
💡 Result:
In Playwright config, testIgnore glob patterns are matched against the absolute file path of candidate test files discovered by recursively scanning the testDir directory. The patterns are standard glob patterns (using minimatch), where matches any number of directories, * matches filenames without path separator, etc. For testDir: '../tests', the absolute path of tests/accessibility/accessibility-audit.spec.ts (assuming config at root and tests sibling) is something like /path/to/project/tests/accessibility/accessibility-audit.spec.ts. - testIgnore: ['tests/accessibility/'] will NOT match because it expects a subpath starting with tests/ directly under root, but the path from testDir starts with accessibility/. - Use testIgnore: ['accessibility/'] to match paths under tests/accessibility/. - /accessibility/ would also work, matching anywhere under testDir. Official docs confirm: "Matching is performed against the absolute file path. Strings are treated as glob patterns. For example, '/test-assets/' will ignore any files in the test-assets directory." Examples use patterns like *test-assets or /test-assets/ that work regardless of testDir because they use or simple *. Secondary source notes patterns are "relative to the project's testDir", meaning they effectively match the subpath under testDir when using non-anchored globs, but technically against full absolute path.
Citations:
- 1: https://playwright.dev/docs/test-configuration
- 2: https://playwright.dev/docs/api/class-testconfig
- 3: https://playwright.dev/docs/next/api/class-testproject
- 4: https://playwright.dev/docs/api/class-testproject
Update testIgnore glob for the new test directory.
Line 73 sets testDir: '../tests', but Line 74 uses testIgnore: ['tests/accessibility/**']. Playwright matches testIgnore patterns against the absolute path of discovered test files. With the new test root, paths will start from accessibility/, not tests/accessibility/, so the current pattern won't exclude accessibility tests as intended.
♻️ Minimal fix
- testIgnore: ['tests/accessibility/**'],
+ testIgnore: ['accessibility/**'],📝 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.
| testDir: '../tests', | |
| testIgnore: ['tests/accessibility/**'], | |
| testDir: '../tests', | |
| testIgnore: ['accessibility/**'], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/playwright.config.ts` around lines 73 - 74, The testIgnore glob no
longer matches because testDir is set to '../tests'—update the testIgnore entry
in the config (the testIgnore array in playwright.config.ts) to match paths
relative to the new test root; replace 'tests/accessibility/**' with
'accessibility/**' (or use a more defensive '**/accessibility/**') so
accessibility tests are actually excluded when testDir is '../tests'.
…ltering on every render Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
There was a problem hiding this comment.
3 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="config/playwright.config.ts">
<violation number="1">
P1: `testDir` now points to `config/tests`, so Playwright will miss the repository’s actual test suite under top-level `tests/`.</violation>
</file>
<file name=".github/workflows/browser-tests.yml">
<violation number="1">
P1: The Playwright run no longer points to the repo’s main config file, so CI skips the intended shared test configuration.</violation>
<violation number="2">
P2: Using `sleep 15` for server readiness is flaky; replace it with an actual health check loop before running tests.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…ltering on every render Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
💡 What: Memoize the filtering of
messagesforanalyzedUserMessagesusinguseMemoand replace inlinefilterandsomecalls with the memoized array.🎯 Why: The component previously performed O(N) operations by iterating over the
messagesarray inline on every render to find messages withmentalHealthAnalysis. This creates a rendering bottleneck as the chat session grows.📊 Impact: Prevents unnecessary O(N) re-filtering and O(N) condition checks on every React render cycle.
🔬 Measurement: Profile the component rendering speed when the
messagesarray becomes very large; the render time overhead from re-filtering will be eliminated.PR created automatically by Jules for task 5867982123106047137 started by @daggerstuff
Summary by Sourcery
Enhancements:
Summary by cubic
Memoized filtering of analyzed user messages in
MentalHealthChatDemoto avoid O(N) re-filtering on each render and keep the UI smooth. CI runsplaywrightwith the shared config, publishes reports, and useswebServerfor preview to avoid port collisions.Refactors
analyzedUserMessageswithuseMemoand reused it for recent analysis and empty-state checks.Bug Fixes
playwrightwith--config=config/playwright.config.ts; use thetestsdir and letwebServerhandle build/preview to prevent port conflicts.config/playwright-reportandconfig/test-resultsfor CI artifacts.Written for commit 85f0d5c. Summary will update on new commits.
Summary by CodeRabbit