Speed review improvements#2207
Conversation
Changed the date filtering logic in fetchRounds to properly compare dates at midnight rather than using ISO string comparison. This ensures rounds are correctly filtered based on whether their discussion date is today or in the future, preventing potential timezone-related issues.
Add proper CSS classes to handle long text content that was causing layout issues. Applied break-words and overflow-hidden to application source fields and session complete components to prevent horizontal scrolling and maintain responsive layout.
Add keyboard shortcut (spacebar) to cycle through application details sections sequentially. When pressed, it closes the current open detail and opens the next one, with smooth scrolling to keep it in view. This improves keyboard navigation efficiency during the review process.
render
Bot
temporarily deployed
to
speed-review-improvements - bluedot-preview PR #2207
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis pull request updates the speed-review application across multiple components and API layers. The ApplicationCard and SessionComplete components receive Tailwind styling adjustments for improved text wrapping and overflow handling. The SessionComplete component's average duration display is refactored to show a minutes-and-seconds format rather than rounded seconds. The Airtable API layer changes the date filtering logic to use a local midnight timestamp instead of ISO date strings, and reorders fetched applications by prioritising those with aiSummary fields. A spacebar keyboard shortcut is added to enable sequential navigation through details elements during review. Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
Modified the application fetching logic to display applications with AI summaries first, followed by those without. Both groups are still shuffled randomly within their respective categories to maintain fairness while improving reviewer efficiency.
…rd handler Add consistent spacing and formatting to the keyboard event handler for better code readability. Ensures proper line breaks after control flow statements and maintains consistent code style.
Greptile SummaryThis PR brings a collection of UX improvements to the speed-review app: overflow/layout fixes across Key changes:
Confidence Score: 4/5Safe to merge for UTC-hosted deployments (e.g. Vercel); the date comparison regression only surfaces on non-UTC server environments. The date filtering change introduces a real behavioral regression (today's rounds being excluded) on non-UTC servers. In practice Vercel runs in UTC so production is likely unaffected, but the logic is demonstrably wrong for local development in non-UTC timezones. All other changes are clean layout and UX improvements with no correctness issues. apps/speed-review/src/lib/api/airtable.ts — the date timezone mismatch warrants attention before merging. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Spacebar pressed] --> B{Target is input/textarea/select?}
B -- Yes --> Z[Ignore]
B -- No --> C[e.preventDefault]
C --> D[querySelectorAll 'details' globally]
D --> E{Any section open?}
E -- No --> F[nextIndex = 0]
E -- Yes --> G[openIndex = index of open section]
G --> H[Close all sections]
H --> I[nextIndex = openIndex + 1]
F --> J{nextIndex < details.length?}
I --> J
J -- No --> K[All closed — cycle complete]
J -- Yes --> L[Open details at nextIndex]
L --> M[scrollIntoView smooth]
Reviews (1): Last reviewed commit: "feat(speed-review): add spacebar navigat..." | Re-trigger Greptile |
render
Bot
temporarily deployed
to
speed-review-improvements - bluedot-preview PR #2207
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -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; | |||
There was a problem hiding this comment.
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;
})| case ' ': { | ||
| e.preventDefault(); | ||
| const details = Array.from(document.querySelectorAll('details')); | ||
| const openIndex = details.findIndex((d) => d.open); | ||
| 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' }); |
There was a problem hiding this comment.
<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') ?? []);| case ' ': { | ||
| e.preventDefault(); | ||
| const details = Array.from(document.querySelectorAll('details')); | ||
| const openIndex = details.findIndex((d) => d.open); | ||
| 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' }); |
There was a problem hiding this comment.
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/speed-review/src/lib/api/airtable.ts (1)
148-163: Consider timezone consistency for server-side date filtering.The change correctly parses dates and compares against local midnight, which is an improvement over raw ISO string comparison. However,
todayStartis set to the server's local midnight, which may differ from users' local dates depending on the server's timezone configuration.If users in different timezones should see the same rounds based on a consistent reference (e.g., UTC), consider using UTC-based comparison:
const todayStart = new Date(); todayStart.setUTCHours(0, 0, 0, 0); // and compare using UTC-based parsingIf server-local time is the intended behaviour (e.g., the organisation operates in a single timezone), the current implementation is appropriate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/speed-review/src/lib/api/airtable.ts` around lines 148 - 163, The current filtering uses server-local midnight via todayStart which can yield inconsistent results across user timezones; update the logic in the mapping/filter block (symbols: todayStart, firstDiscussion, and the final .filter that constructs discussionDate) to use a UTC-based start-of-day when you need timezone-consistent results: create todayStart using UTC (setUTCHours(0,0,0,0)) and compare discussionDate using UTC-aware parsing/values (e.g., use Date UTC timestamps or ensure discussionDate is parsed/normalized to UTC) so the comparison discussionDate >= todayStart is done in the same timezone reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/speed-review/src/pages/index.tsx`:
- Around line 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.
---
Nitpick comments:
In `@apps/speed-review/src/lib/api/airtable.ts`:
- Around line 148-163: The current filtering uses server-local midnight via
todayStart which can yield inconsistent results across user timezones; update
the logic in the mapping/filter block (symbols: todayStart, firstDiscussion, and
the final .filter that constructs discussionDate) to use a UTC-based
start-of-day when you need timezone-consistent results: create todayStart using
UTC (setUTCHours(0,0,0,0)) and compare discussionDate using UTC-aware
parsing/values (e.g., use Date UTC timestamps or ensure discussionDate is
parsed/normalized to UTC) so the comparison discussionDate >= todayStart is done
in the same timezone reference.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e047424c-3a8f-4982-9f5d-0241ca99365f
📒 Files selected for processing (4)
apps/speed-review/src/components/ApplicationCard.tsxapps/speed-review/src/components/SessionComplete.tsxapps/speed-review/src/lib/api/airtable.tsapps/speed-review/src/pages/index.tsx
| case ' ': { | ||
| e.preventDefault(); | ||
| const details = Array.from(document.querySelectorAll('details')); | ||
| const openIndex = details.findIndex((d) => d.open); | ||
| 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; | ||
| } |
There was a problem hiding this comment.
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
breakstatement
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.
1 participant
Description
Issue
Fixes #
Developer checklist
Screenshot