fix: explain empty consolidation viewer states#812
Open
mturac wants to merge 2 commits into
Open
Conversation
|
@mturac is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe viewer UI's empty-state messaging for consolidation tabs is redesigned to show gate requirements and current input counts. New CSS styles and helper functions render standardized cards across all consolidation sections. Dashboard, Lessons, and Crystals views display their respective gate criteria and suggested actions. A test suite validates the empty-state content presence. ChangesConsolidation tier empty-state messaging
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Poem
🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/viewer/index.html (1)
1427-1427: ⚡ Quick winEscape
currentbefore injecting into HTML.
currentis inserted directly while other fields are escaped. Keeping this escaped avoids future XSS footguns if the helper is reused with non-numeric input.🛡️ Proposed fix
- '<div class="empty-lead">Current count: ' + current + '. This is an input gate, not a broken tab.</div>' + + '<div class="empty-lead">Current count: ' + esc(current) + '. This is an input gate, not a broken tab.</div>' +🤖 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 `@src/viewer/index.html` at line 1427, The snippet injects the variable current directly into HTML; escape current using the same HTML-escaping helper used elsewhere (e.g., escapeHtml or the existing escaping utility) before concatenation so the line becomes the escaped value for current inside the '<div class="empty-lead">…</div>' string; update the construction that produces that string to call the escape helper on current (or coerce to Number if it must be numeric) to prevent XSS.test/viewer-consolidation-empty-state.test.ts (1)
12-24: ⚡ Quick winAdd an assertion for insights empty-state guidance.
This regression test covers four tiers but skips insights, which is part of the stated consolidation empty-state scope.
✅ Suggested assertion addition
expect(html).toContain("Crystals come from crystallize actions"); expect(html).toContain("mem::auto-crystallize"); + expect(html).toContain("Insights are waiting for consolidation inputs");🤖 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 `@test/viewer-consolidation-empty-state.test.ts` around lines 12 - 24, Add an assertion in the viewer-consolidation-empty-state.test.ts near the other expects that verifies the rendered html (variable html) includes the insights empty-state guidance text; follow the same pattern as the existing lines (e.g., the expect(html).toContain(...) checks for "Semantic facts...", "Procedures...", "Lessons...", "Crystals...") and add one expect(html).toContain(...) that matches the canonical insights guidance string shown in the UI so insights are covered by this consolidation empty-state test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/viewer/index.html`:
- Around line 1419-1424: The shared title lookup for the empty-state map (the
var title = { ... }[kind] || 'Nothing to show yet') is missing an insights key
so when kind === 'insights' it falls back to the generic copy; add an
"insights": '<appropriate insights message>' entry to the object literal
alongside semantic, procedural, lessons, and crystals so the lookup returns the
intended insights-specific string when kind is "insights".
---
Nitpick comments:
In `@src/viewer/index.html`:
- Line 1427: The snippet injects the variable current directly into HTML; escape
current using the same HTML-escaping helper used elsewhere (e.g., escapeHtml or
the existing escaping utility) before concatenation so the line becomes the
escaped value for current inside the '<div class="empty-lead">…</div>' string;
update the construction that produces that string to call the escape helper on
current (or coerce to Number if it must be numeric) to prevent XSS.
In `@test/viewer-consolidation-empty-state.test.ts`:
- Around line 12-24: Add an assertion in the
viewer-consolidation-empty-state.test.ts near the other expects that verifies
the rendered html (variable html) includes the insights empty-state guidance
text; follow the same pattern as the existing lines (e.g., the
expect(html).toContain(...) checks for "Semantic facts...", "Procedures...",
"Lessons...", "Crystals...") and add one expect(html).toContain(...) that
matches the canonical insights guidance string shown in the UI so insights are
covered by this consolidation empty-state test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a6d9aca-fc32-4729-8509-93d04f97952b
📒 Files selected for processing (2)
src/viewer/index.htmltest/viewer-consolidation-empty-state.test.ts
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
1 participant
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
Tests
Closes #754
Summary by CodeRabbit
New Features
Tests