Skip to content

Commit b67229c

Browse files
shrsvshrsv
committed
Implement 'All Clear' Review State
LiveReview Pre-Commit Check: ran (iter:2, coverage:66%)
1 parent ce88c5a commit b67229c

5 files changed

Lines changed: 146 additions & 4 deletions

File tree

‎internal/staticserve/static/app.js‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import { getToolbar } from './components/Toolbar.js';
1515
import { getCommentNav } from './components/CommentNav.js';
1616
import { UsageBanner } from './components/UsageBanner.js';
1717
import { buildPerformanceSnapshot, getFirstRenderTime, getLoadingActivityMessage, getPerformanceNow, recordFirstRenderTime } from './components/review_performance_state.mjs';
18+
import { shouldShowAllClear } from './components/review_outcome_state.mjs';
1819

1920
let domReadyStartMs = null;
2021

@@ -639,6 +640,8 @@ async function initApp() {
639640
const summary = reviewData?.summary || '';
640641
const files = reviewData?.Files || [];
641642
const totalComments = files.reduce((sum, file) => sum + (file.CommentCount || 0), 0);
643+
const errorSummary = reviewData?.errorSummary || '';
644+
const showAllClear = shouldShowAllClear({ status, totalComments, errorSummary });
642645
const firstCommentRenderMs = getFirstRenderTime(commentRenderTimes);
643646
const performanceSnapshot = buildPerformanceSnapshot({
644647
baselineMs: reviewStartMsRef.current,
@@ -887,11 +890,12 @@ async function initApp() {
887890
888891
<${UsageBanner} endpoint="/api/runtime/usage-chip" />
889892
890-
${summary && summary.trim() && status !== 'in_progress' && html`
893+
${(showAllClear || (summary && summary.trim())) && status !== 'in_progress' && html`
891894
<${Summary}
892895
markdown=${summary}
893896
status=${status}
894-
errorSummary=${reviewData?.errorSummary || ''}
897+
errorSummary=${errorSummary}
898+
showAllClear=${showAllClear}
895899
/>
896900
`}
897901

‎internal/staticserve/static/components/Summary.js‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ function renderSafeMarkdown(container, markdown) {
9393
export async function createSummary() {
9494
const { html, useEffect, useRef } = await waitForPreact();
9595

96-
return function Summary({ markdown, status, errorSummary }) {
96+
return function Summary({ markdown, status, errorSummary, showAllClear }) {
9797
const contentRef = useRef(null);
9898

9999
useEffect(() => {
@@ -104,6 +104,15 @@ export async function createSummary() {
104104

105105
return html`
106106
<div class="summary" id="summary-content">
107+
${showAllClear && html`
108+
<div class="summary-all-clear" role="status" aria-live="polite">
109+
<div class="summary-all-clear-icon" aria-hidden="true">✓</div>
110+
<div class="summary-all-clear-copy">
111+
<strong class="summary-all-clear-title">Good to go</strong>
112+
<p class="summary-all-clear-text">This review finished without any review comments. No issues were found in the reviewed diff.</p>
113+
</div>
114+
</div>
115+
`}
107116
${isError && html`
108117
<div style="padding: 16px; background: #fef2f2; border: 1px solid #fecaca; border-radius: 6px; color: #991b1b; margin-bottom: 16px;">
109118
<strong style="display: block; margin-bottom: 8px; font-size: 16px;">⚠️ Error Details:</strong>
@@ -112,7 +121,7 @@ export async function createSummary() {
112121
</pre>
113122
</div>
114123
`}
115-
<div ref=${contentRef}></div>
124+
<div ref=${contentRef} style=${markdown && markdown.trim() ? '' : 'display: none;'}></div>
116125
</div>
117126
`;
118127
};

‎internal/staticserve/static/components/review_outcome_state.mjs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
export function shouldShowAllClear({ status, totalComments, errorSummary }) {
2+
const normalizedStatus = typeof status === 'string' ? status.trim().toLowerCase() : '';
3+
const parsedComments = Number(totalComments);
4+
const normalizedComments = Number.isFinite(parsedComments) ? parsedComments : 0;
5+
const hasError = typeof errorSummary === 'string'
6+
? errorSummary.trim() !== ''
7+
: errorSummary != null;
8+
9+
return normalizedStatus === 'completed' && !hasError && normalizedComments === 0;
10+
}

‎internal/staticserve/static/components/review_outcome_state.test.mjs‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
import test from 'node:test';
2+
import assert from 'node:assert/strict';
3+
4+
import { shouldShowAllClear } from './review_outcome_state.mjs';
5+
6+
test('shouldShowAllClear returns true for completed reviews with zero comments and no error', () => {
7+
assert.equal(shouldShowAllClear({
8+
status: 'completed',
9+
totalComments: 0,
10+
errorSummary: '',
11+
}), true);
12+
});
13+
14+
test('shouldShowAllClear returns false when comments exist', () => {
15+
assert.equal(shouldShowAllClear({
16+
status: 'completed',
17+
totalComments: 1,
18+
errorSummary: '',
19+
}), false);
20+
});
21+
22+
test('shouldShowAllClear returns false when an error summary is present', () => {
23+
assert.equal(shouldShowAllClear({
24+
status: 'completed',
25+
totalComments: 0,
26+
errorSummary: 'Review failed',
27+
}), false);
28+
});
29+
30+
test('shouldShowAllClear returns false while review is still running', () => {
31+
assert.equal(shouldShowAllClear({
32+
status: 'in_progress',
33+
totalComments: 0,
34+
errorSummary: '',
35+
}), false);
36+
});
37+
38+
test('shouldShowAllClear normalizes string inputs and whitespace-only error summaries', () => {
39+
assert.equal(shouldShowAllClear({
40+
status: ' Completed ',
41+
totalComments: '0',
42+
errorSummary: ' ',
43+
}), true);
44+
});
45+
46+
test('shouldShowAllClear treats missing or non-numeric comment counts as zero', () => {
47+
assert.equal(shouldShowAllClear({
48+
status: 'completed',
49+
totalComments: undefined,
50+
errorSummary: undefined,
51+
}), true);
52+
53+
assert.equal(shouldShowAllClear({
54+
status: 'completed',
55+
totalComments: 'not-a-number',
56+
errorSummary: '',
57+
}), true);
58+
});
59+
60+
test('shouldShowAllClear rejects non-string error payloads', () => {
61+
assert.equal(shouldShowAllClear({
62+
status: 'completed',
63+
totalComments: 0,
64+
errorSummary: { message: 'boom' },
65+
}), false);
66+
});
67+
68+
test('shouldShowAllClear rejects missing status even when counts are zero', () => {
69+
assert.equal(shouldShowAllClear({
70+
status: null,
71+
totalComments: 0,
72+
errorSummary: '',
73+
}), false);
74+
});

‎internal/staticserve/static/styles.css‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,7 @@
127127
--status-success-bg: rgba(76, 175, 80, 0.15);
128128
--status-success-border: rgba(76, 175, 80, 0.4);
129129
--status-success-text: #89d185;
130+
--status-success-icon-bg: rgba(76, 175, 80, 0.22);
130131
--status-error-bg: rgba(241, 76, 76, 0.15);
131132
--status-error-border: rgba(241, 76, 76, 0.4);
132133
--status-error-text: #f48771;
@@ -801,6 +802,50 @@ body {
801802
border-radius: 4px;
802803
}
803804

805+
.summary-all-clear {
806+
display: flex;
807+
align-items: flex-start;
808+
gap: 12px;
809+
padding: 14px 16px;
810+
margin-bottom: 12px;
811+
background: var(--status-success-bg);
812+
border: 1px solid var(--status-success-border);
813+
border-radius: 6px;
814+
}
815+
816+
.summary-all-clear-icon {
817+
width: 28px;
818+
height: 28px;
819+
border-radius: 999px;
820+
display: inline-flex;
821+
align-items: center;
822+
justify-content: center;
823+
background: var(--status-success-icon-bg);
824+
color: var(--accent-green-light);
825+
font-size: 16px;
826+
font-weight: 700;
827+
flex: 0 0 auto;
828+
}
829+
830+
.summary-all-clear-copy {
831+
min-width: 0;
832+
}
833+
834+
.summary-all-clear-title {
835+
display: block;
836+
margin-bottom: 4px;
837+
color: var(--accent-green-light);
838+
font-size: 16px;
839+
font-weight: 700;
840+
}
841+
842+
.summary-all-clear-text {
843+
margin: 0;
844+
color: var(--text-secondary);
845+
font-size: 13px;
846+
line-height: 1.5;
847+
}
848+
804849
.summary h1 { font-size: 16px; font-weight: 600; margin-bottom: 10px; margin-top: 12px; color: var(--text-primary); }
805850
.summary h1:first-child { margin-top: 0; }
806851
.summary h2 { font-size: 14px; font-weight: 600; margin-bottom: 8px; margin-top: 10px; color: var(--text-secondary); }

0 commit comments

Comments
 (0)