Skip to content

Speed review improvements #2207

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
anglilian merged 5 commits into from
Mar 28, 2026
Merged

Speed review improvements #2207

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
26c70f5
fix(speed-review): correct date comparison logic for filtering rounds
anglilian Mar 28, 2026
0ba9240
fix(speed-review): improve text overflow handling in application cards
anglilian Mar 28, 2026
a9184d5
feat(speed-review): add spacebar navigation for application details
anglilian Mar 28, 2026
1b2549d
feat(speed-review): prioritize applications with AI summaries
anglilian Mar 28, 2026
e042cb4
style: improve code formatting and readability in speed-review keyboa…
anglilian Mar 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/speed-review/src/components/ApplicationCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ export const ApplicationCard: React.FC<ApplicationCardProps> = ({ application, p
</div>

{(applicationSource ?? utmSource) && (
<div className="text-size-xs text-stone-500 space-y-0.5">
<div className="text-size-xs text-stone-500 space-y-0.5 break-words overflow-hidden">
{applicationSource && <p>Heard about us: {applicationSource}</p>}
{utmSource && <p>UTM source: {utmSource}</p>}
</div>
Expand Down
15 changes: 9 additions & 6 deletions apps/speed-review/src/components/SessionComplete.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,10 @@ export const SessionComplete: React.FC<SessionCompleteProps> = ({
const totalSecs = Math.floor(totalMs / 1000);
const mins = Math.floor(totalSecs / 60);
const secs = totalSecs % 60;
const avgSecs = rated.length > 0 ? Math.round(totalSecs / rated.length) : 0;
const avgTotalSecs = rated.length > 0 ? Math.round(totalSecs / rated.length) : 0;
const avgMins = Math.floor(avgTotalSecs / 60);
const avgSecs = avgTotalSecs % 60;
const avgDisplay = avgMins > 0 ? `${avgMins}m ${String(avgSecs).padStart(2, '0')}s` : `${avgSecs}s`;

const renderRow = (r: RatedApplication, accent: 'green' | 'red') => {
const isMoved = r.rating === 'moved-to-agisc';
Expand All @@ -83,7 +86,7 @@ export const SessionComplete: React.FC<SessionCompleteProps> = ({
else if (accent === 'green') bgColors = 'bg-green-950 border-green-800';

return (
<div key={r.id} className={`border rounded-lg px-3 py-2 ${bgColors}`}>
<div key={r.id} className={`border rounded-lg px-3 py-2 overflow-hidden ${bgColors}`}>
<div className="flex flex-col sm:flex-row sm:items-start sm:justify-between gap-1 sm:gap-2">
<div className="min-w-0">
<p className="text-size-sm font-medium text-stone-100 break-words">
Expand Down Expand Up @@ -129,7 +132,7 @@ export const SessionComplete: React.FC<SessionCompleteProps> = ({
};

return (
<div className="space-y-6">
<div className="space-y-6 overflow-hidden">
<div>
<h1 className="text-2xl font-bold text-stone-100">Session complete</h1>
<p className="text-size-sm text-stone-400 mt-1">{round}</p>
Expand Down Expand Up @@ -176,21 +179,21 @@ export const SessionComplete: React.FC<SessionCompleteProps> = ({
<p className="text-size-xs text-stone-500 mt-1">Total time</p>
</div>
<div className="bg-stone-800 border border-stone-700 rounded-lg p-4">
<p className="text-2xl font-bold text-stone-100">{avgSecs}s</p>
<p className="text-2xl font-bold text-stone-100">{avgDisplay}</p>
<p className="text-size-xs text-stone-500 mt-1">Avg per app</p>
</div>
</div>

<div className="grid md:grid-cols-2 gap-6">
<div>
<div className="min-w-0">
<h2 className="text-size-sm font-semibold uppercase tracking-wide text-green-400 mb-3">
Accept ({accepted.length})
</h2>
<div className="space-y-2">
{accepted.map((r) => renderRow(r, 'green'))}
</div>
</div>
<div>
<div className="min-w-0">
<h2 className="text-size-sm font-semibold uppercase tracking-wide text-red-400 mb-3">
Reject ({rejected.length})
</h2>
Expand Down
9 changes: 7 additions & 2 deletions apps/speed-review/src/lib/api/airtable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,8 @@ export const fetchRounds = async (): Promise<Round[]> => {
['Course - Round - Intensity', 'Status', 'fldfi2ZKsbSK6NVTV', 'First discussion'],
);

const today = new Date().toISOString().slice(0, 10);
const todayStart = new Date();
todayStart.setHours(0, 0, 0, 0);

return records
.map((r) => ({
Expand All @@ -155,7 +156,11 @@ export const fetchRounds = async (): Promise<Round[]> => {
firstDiscussion: str(r.fields['First discussion']),
}))
.filter((r) => r.name !== '')
.filter((r) => !!r.firstDiscussion && r.firstDiscussion >= today)
.filter((r) => {
if (!r.firstDiscussion) return false;
const discussionDate = new Date(r.firstDiscussion);
return discussionDate >= todayStart;
Comment on lines 148 to +162

@greptile-apps greptile-apps Bot Mar 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Timezone mismatch in date comparison

todayStart.setHours(0, 0, 0, 0) sets midnight in the server's local timezone, but new Date(r.firstDiscussion) where r.firstDiscussion is a date-only string like "2025-04-01" is parsed as UTC midnight per the ECMAScript spec.

If the server runs in a timezone behind UTC (e.g., UTC-5), todayStart is 2025-04-01T05:00:00Z while new Date('2025-04-01') is 2025-04-01T00:00:00Z, so discussionDate >= todayStart evaluates to false — meaning today's rounds would be excluded from the list.

The old string comparison approach (r.firstDiscussion >= today on two YYYY-MM-DD strings) was actually correct and timezone-safe for pure calendar-date comparisons. The new approach only works correctly when the server is running in UTC.

A consistent fix would be to stay in UTC throughout:

const todayUTC = new Date().toISOString().slice(0, 10); // "YYYY-MM-DD" UTC

return records
  ...
  .filter((r) => {
    if (!r.firstDiscussion) return false;
    return r.firstDiscussion >= todayUTC;
  })

})
.sort((a, b) => {
const aDate = a.firstDiscussion ?? '';
const bDate = b.firstDiscussion ?? '';
Expand Down
12 changes: 12 additions & 0 deletions apps/speed-review/src/pages/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,19 @@
case 'E':
handleRateRef.current('strong-yes');
break;
case ' ': {
e.preventDefault();
const details = Array.from(document.querySelectorAll('details'));
const openIndex = details.findIndex((d) => d.open);
details.forEach((d) => { d.open = false; });

Check failure on line 356 in apps/speed-review/src/pages/index.tsx

View workflow job for this annotation

GitHub Actions / ci 

Closing curly brace should be on the same line as opening curly brace or on the line after the previous block

Check failure on line 356 in apps/speed-review/src/pages/index.tsx

View workflow job for this annotation

GitHub Actions / ci 

This line has 2 statements. Maximum allowed is 1

Check failure on line 356 in apps/speed-review/src/pages/index.tsx

View workflow job for this annotation

GitHub Actions / ci 

Statement inside of curly braces should be on next line
const nextIndex = openIndex === -1 ? 0 : openIndex + 1;
if (nextIndex < details.length) {
details[nextIndex]!.open = true;
details[nextIndex]!.scrollIntoView({ behavior: 'smooth', block: 'nearest' });
Comment on lines +352 to +363

@greptile-apps greptile-apps Bot Mar 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Global DOM query for <details> elements is fragile

document.querySelectorAll('details') queries the entire document, not just the current application card. This works today because <details> elements only appear inside ApplicationCard, but if any other component (e.g. SummaryCard, a modal, or a nav component) ever adds a <details> element, those would silently be included in the cycle, causing unexpected behaviour.

A more robust approach would be to scope the query to the card container using a ref or a specific data attribute on the card's wrapper <div>:

// In the JSX, add a data attribute:
// <div data-app-details-container ...>

// In the keyboard handler:
const container = document.querySelector('[data-app-details-container]');
const details = Array.from(container?.querySelectorAll('details') ?? []);

Comment on lines +352 to +363

@greptile-apps greptile-apps Bot Mar 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 No tests for the new spacebar shortcut

The spacebar cycling behaviour (open first section → cycle to next → close all → wrap around) is non-trivial logic. Consider adding a unit or integration test that verifies the cycle order, the wrap-around when reaching the last section, and that e.preventDefault() is called to suppress browser scrolling.

Rule Used: Consider adding tests for any new functionality in... (source)

Learnt From
bluedotimpact/bluedot#956
bluedotimpact/bluedot#969
bluedotimpact/bluedot#958

}
break;

Check failure on line 362 in apps/speed-review/src/pages/index.tsx

View workflow job for this annotation

GitHub Actions / ci 

Expected blank line before this statement
}
Comment on lines +352 to +363

@coderabbitai coderabbitai Bot Mar 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix ESLint brace-style violations causing CI failure.

The pipeline is failing due to style violations flagged by ESLint:

  • Line 356: Statement inside curly braces should be on the next line
  • Line 362: Expected blank line before break statement

The navigation logic itself is sound.

🔧 Proposed fix for ESLint violations 
         case ' ': {
           e.preventDefault();
           const details = Array.from(document.querySelectorAll('details'));
           const openIndex = details.findIndex((d) => d.open);
-          details.forEach((d) => { d.open = false; });
+          details.forEach((d) => {
+            d.open = false;
+          });
           const nextIndex = openIndex === -1 ? 0 : openIndex + 1;
           if (nextIndex < details.length) {
             details[nextIndex]!.open = true;
             details[nextIndex]!.scrollIntoView({ behavior: 'smooth', block: 'nearest' });
           }
+
           break;
         }
🧰 Tools
🪛 GitHub Actions: ci_cd

[error] 356-356: ESLint (@stylistic/brace-style): 356:34 error - Statement inside of curly braces should be on next line.

🪛 GitHub Check: ci

[failure] 362-362:
Expected blank line before this statement

[failure] 356-356:
Closing curly brace should be on the same line as opening curly brace or on the line after the previous block

[failure] 356-356:
This line has 2 statements. Maximum allowed is 1

[failure] 356-356:
Statement inside of curly braces should be on next line

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@apps/speed-review/src/pages/index.tsx` around lines 352 - 363, In the
keyboard navigation switch case (case ' '), fix ESLint brace-style and spacing:
change the inline block in details.forEach((d) => { d.open = false; }); to a
multiline block so the statement is on the next line (e.g. details.forEach((d)
=> { newline d.open = false; newline });) and add a blank line immediately
before the break statement so there is an empty line separating the last
statement and break; update the code in the case ' ' block where details.forEach
and break are used.

case 'p':

Check failure on line 364 in apps/speed-review/src/pages/index.tsx

View workflow job for this annotation

GitHub Actions / ci 

Expected blank line before this statement
case 'P':
dispatch({ type: 'TOGGLE_PAUSE' });
break;
Expand Down
Loading