Skip to content

Add 'now' parameter to /sessions endpoint for real-time filtering#3

Merged
ThomasHoussin merged 3 commits intomasterfrom
claude/add-sessions-now-parameter-01KjFdnJkCpLB63MuZi26rPH
Nov 21, 2025
Merged

Add 'now' parameter to /sessions endpoint for real-time filtering#3
ThomasHoussin merged 3 commits intomasterfrom
claude/add-sessions-now-parameter-01KjFdnJkCpLB63MuZi26rPH

Conversation

@ThomasHoussin
Copy link
Owner

Features:

  • New query parameter now=true returns sessions happening now or starting soon
  • Ongoing: Sessions currently in progress (startTime <= now <= endTime)
  • Upcoming: Sessions starting within next 30 minutes
  • Uses Paris timezone (Europe/Paris) via zoneinfo (Python 3.9+)
  • Fallback to 20-minute duration for sessions with invalid endTime

Implementation:

  • Added parse_session_datetime() to parse session dates/times
  • Added get_paris_now() for Paris timezone-aware current time
  • Added filter_sessions_by_now() for real-time session filtering
  • Response includes currentTime, ongoing, and upcoming session groups
  • No caching for accuracy (sessions transition states quickly)

Documentation:

  • Updated llms.txt with new parameter details and examples
  • Added response format examples for now=true
  • Added common query pattern for "What's happening now?"

Technical notes:

  • Uses zoneinfo (stdlib) instead of pytz (no external dependencies)
  • Returns different response structure when now=true
  • Other filters (date, stage, search) ignored when now=true

Features:
- New query parameter `now=true` returns sessions happening now or starting soon
- Ongoing: Sessions currently in progress (startTime <= now <= endTime)
- Upcoming: Sessions starting within next 30 minutes
- Uses Paris timezone (Europe/Paris) via zoneinfo (Python 3.9+)
- Fallback to 20-minute duration for sessions with invalid endTime

Implementation:
- Added parse_session_datetime() to parse session dates/times
- Added get_paris_now() for Paris timezone-aware current time
- Added filter_sessions_by_now() for real-time session filtering
- Response includes currentTime, ongoing, and upcoming session groups
- No caching for accuracy (sessions transition states quickly)

Documentation:
- Updated llms.txt with new parameter details and examples
- Added response format examples for now=true
- Added common query pattern for "What's happening now?"

Technical notes:
- Uses zoneinfo (stdlib) instead of pytz (no external dependencies)
- Returns different response structure when now=true
- Other filters (date, stage, search) ignored when now=true
Add required permissions to CodeQL workflow:
- security-events: write (to upload scan results)
- contents: read (to checkout code)
- actions: read (to access workflow metadata)

This fixes the "Resource not accessible by integration" error when
uploading CodeQL analysis results to GitHub Security.
@claude
Copy link

claude bot commented Nov 20, 2025

Pull Request Review: Add 'now' parameter to /sessions endpoint

Overview

This PR adds real-time session filtering capability to help users find sessions happening now or starting soon. The implementation is well-structured and adds valuable functionality for conference attendees.

✅ Strengths

1. Code Quality

  • Clean separation of concerns: New functions (parse_session_datetime, get_paris_now, filter_sessions_by_now) are well-isolated and reusable
  • Good error handling: Safe fallback for sessions with invalid end times (20-minute default duration)
  • Type hints: Proper use of type annotations (datetime | None)
  • DRY principle: Time parsing logic is reused from existing parse_time() function

2. Documentation

  • Excellent LLM-friendly documentation in llms.txt with clear examples
  • Well-documented behavior when now=true (ignores other filters)
  • Comprehensive PR description

3. Security Enhancement

  • Added GitHub Actions permissions in CodeQL workflow (security best practice)

🔍 Potential Issues & Suggestions

1. Code Duplication in Time Parsing (Medium Priority)

Location: handler.py:89-104 and handler.py:71-86

Both parse_session_datetime() and parse_time() contain duplicate time parsing logic:

# In parse_session_datetime (lines 96-104)
parts = time_str.replace("AM", "").replace("PM", "").strip().split(":")
hours = int(parts[0])
minutes = int(parts[1]) if len(parts) > 1 else 0

if "PM" in time_str and hours != 12:
    hours += 12
elif "AM" in time_str and hours == 12:
    hours = 0

Recommendation: Extract to shared function to reduce maintenance burden.

2. Date Parsing Brittleness (Medium Priority)

Location: handler.py:107

The hardcoded date format "%b %d, %Y" (e.g., "Nov 25, 2025") depends on the exact format from the data source.

Concern: If the scraper or data source changes date format, this will silently fail and return None.

Recommendations:

  • Add logging when date parsing fails to aid debugging
  • Consider supporting multiple date formats as fallback
  • Add validation during data ingestion

3. Timezone Assumption (Low Priority)

Location: handler.py:108

The code assumes all session times are in Paris timezone, but this isn't validated in the data.

Recommendation: Document this assumption in the code comments and data schema.

4. Caching Implications (High Priority - Functionality)

Location: Global cache variables at top of file

The PR description mentions "No caching for accuracy" but the code still uses global caching for session data:

_sessions_cache: list[dict] | None = None  # Line 16

def get_sessions() -> list[dict]:
    global _sessions_cache
    if _sessions_cache is None:
        data = load_json_from_s3(f"{DATA_PREFIX}/sessions.json")
        _sessions_cache = data.get("sessions", [])
    return _sessions_cache

Issue: Lambda with SnapStart will cache session data across invocations. For now=true queries, this means:

  • A session that ended may still show as "ongoing" until the Lambda instance is replaced
  • New Lambda instances may have stale data from SnapStart snapshots

Recommendations:

  1. Option A (Recommended): Add cache TTL for time-sensitive queries:
    _sessions_cache_time: datetime | None = None
    CACHE_TTL_SECONDS = 60  # 1 minute TTL for now queries
  2. Option B: Disable caching entirely for now=true queries (may impact performance)
  3. Option C: Add a response header indicating cache age to help users understand data freshness

5. Edge Case: Cross-Day Sessions (Low Priority)

Location: handler.py:129

If a session crosses midnight (unlikely but possible), the current logic may not handle it correctly since parse_session_datetime doesn't consider multi-day events.

Recommendation: Document this limitation or add validation.

6. Missing Unit Tests (High Priority)

Observation: No test files visible in the PR.

Recommendations:
For a time-sensitive feature, tests are critical:

  • Test parsing various date/time formats
  • Test boundary conditions (sessions starting exactly at now, ending exactly at now)
  • Test timezone edge cases
  • Mock datetime.now() to test different times of day
  • Test the 20-minute fallback duration logic

7. Code Style: Nested Function (Low Priority)

Location: handler.py:278-290

The format_session() function is defined inside the handler only for the now=true branch, but identical logic exists in the else branch (lines 311-325).

Recommendation: Extract to module-level function to avoid duplication and improve testability.

8. Performance Consideration (Low Priority)

Location: handler.py:128-145

The function iterates through all 243 sessions and parses datetime strings for each. For the dataset size, this is acceptable, but consider:

  • If the conference grows significantly, add indexing or pre-processing
  • The datetime parsing happens on every request (due to filtering after cache retrieval)

🔒 Security Analysis

No security concerns identified:

  • Input validation is implicit (timezone-aware datetime parsing)
  • No SQL injection risk (no database)
  • No XSS risk (JSON API)
  • Proper CORS headers maintained

🎯 Performance Considerations

Current Impact: Minimal

  • Additional datetime parsing adds ~O(n) operations where n=243
  • Using stdlib zoneinfo (no external dependencies) ✅
  • Response size is reasonable (only returns relevant sessions)

SnapStart Compatibility: ⚠️ See caching issue above

📋 Test Coverage

Status: ⚠️ No tests included

Recommended Test Cases:

  1. Valid now=true query returns ongoing/upcoming sessions
  2. Invalid date formats are handled gracefully
  3. Sessions with missing endTime use 20-minute fallback
  4. Timezone correctness (sessions in Paris time)
  5. Boundary conditions (session ending exactly now)
  6. Empty results when no sessions match

🎨 Best Practices Compliance

✅ Follows Python conventions (PEP 8)
✅ Type hints used consistently
✅ Error handling with try/except
✅ Clear function documentation
⚠️ Could benefit from docstring examples
⚠️ Missing unit tests

🚀 Recommendations for Next Steps

  1. Before Merge (High Priority):

    • Address caching issue for time-sensitive data
    • Add basic unit tests for datetime parsing
    • Consider adding logging for parse failures
  2. After Merge (Medium Priority):

    • Refactor duplicate time parsing logic
    • Add comprehensive test coverage
    • Monitor API for parsing failures in production
  3. Future Enhancements (Low Priority):

    • Add minutes parameter to customize "upcoming" window (currently hardcoded to 30)
    • Consider adding past sessions (recently ended)
    • Add response header with data freshness timestamp

✨ Conclusion

This is a well-implemented feature that adds significant value for conference attendees. The code is clean, well-documented, and follows best practices. The main concern is the caching behavior with SnapStart, which could lead to stale "current" session data. Once the caching issue is addressed and basic tests are added, this PR would be ready to merge.

Recommendation: Approve with minor changes requested (caching + tests)


Review generated with attention to the CLAUDE.md project guidelines and Python best practices.

@ThomasHoussin ThomasHoussin changed the base branch from master to claude/update-llm-instructions-01RSD26PcPnWiAGrGSrVGyrc November 20, 2025 16:37
@ThomasHoussin ThomasHoussin changed the base branch from claude/update-llm-instructions-01RSD26PcPnWiAGrGSrVGyrc to master November 20, 2025 16:38
Performance optimization for the 'now' parameter:
- Pre-parse all session datetimes once at Lambda cold start
- Store parsed datetime objects in _start_dt and _end_dt fields
- Reuse cached datetimes across all invocations

Measured performance improvement:
- WITHOUT cache: ~9.20ms per request (8.97ms parsing + 0.23ms filtering)
- WITH cache: ~0.23ms per request (filtering only)
- Result: 97.6% faster per request after cold start

With SnapStart, the 8.97ms parsing cost is amortized across thousands
of Lambda invocations, maximizing cold start optimization benefits.

Technical implementation:
- get_sessions() now enriches sessions with _start_dt and _end_dt
- filter_sessions_by_now() uses pre-parsed datetimes instead of parsing
- Internal fields (_start_dt, _end_dt) not exposed in API responses
@claude
Copy link

claude bot commented Nov 20, 2025

PR Review: Add 'now' parameter to /sessions endpoint

Summary

This PR adds a valuable real-time filtering feature to the /sessions endpoint with the now=true parameter. The implementation is generally solid with good performance optimizations leveraging SnapStart.


✅ Strengths

  1. Excellent Performance Optimization: Pre-parsing datetimes at cold start and caching them in session objects is a smart use of SnapStart. The 97.6% performance improvement is impressive.

  2. Good Fallback Logic: The 20-minute fallback duration for sessions with invalid endTime (handler.py:157) is practical and based on data analysis.

  3. Clean API Design: The response structure with ongoing and upcoming categories is intuitive and well-documented.

  4. Comprehensive Documentation: The llms.txt updates clearly explain the new feature with examples.

  5. Security Fix: The CodeQL workflow permissions fix is a good catch.


🐛 Potential Bugs

1. Datetime Parsing Duplication (handler.py:103-126)

The parse_session_datetime() function duplicates time parsing logic from parse_time() (lines 85-100). This creates maintenance burden and potential inconsistencies.

Recommendation: Refactor to reuse parse_time() or extract shared logic.

2. Edge Case: Sessions with endTime == startTime (handler.py:155)

Sessions where endTime <= startTime are treated as invalid, but endTime == startTime might be legitimate (e.g., a 5-minute announcement). The fallback adds 20 minutes, which could incorrectly classify finished sessions as ongoing.

3. Timezone Assumption Not Documented

The code hardcodes Europe/Paris timezone (handler.py:122), but this assumption isn't documented. Recommend adding a module-level constant CONFERENCE_TIMEZONE.


⚡ Performance Considerations

1. Cache Pollution with Internal Fields

Adding _start_dt and _end_dt to cached session objects increases memory footprint. For 243 sessions × 2 datetime objects, this is negligible, but worth documenting.

2. Unnecessary get_paris_now() Calls

The code calls get_paris_now() twice (lines 140, 311). Recommend calling once and passing as parameter to improve testability.


🔒 Security Concerns

✅ Input validation on now parameter is safe with .lower() in ["true", "1", "yes"]

⚠️ Exception swallowing (handler.py:125) - parse_session_datetime() silently returns None on any exception. Recommend adding logging for debugging.


🧪 Test Coverage

Critical Issue: ❌ No tests found in the repository

This PR introduces complex datetime logic with edge cases:

  • Invalid/missing startTime/endTime
  • Sessions ending before they start
  • Boundary conditions (exactly 30 minutes away, exactly now)
  • Timezone handling
  • AM/PM parsing edge cases (12:00 AM, 12:00 PM)

Strongly recommend adding unit tests for:

  1. parse_session_datetime() with various date/time formats
  2. filter_sessions_by_now() with different time scenarios
  3. Edge cases with missing data and invalid times

📝 Code Quality

1. Nested Function Definition (handler.py:297-309)

The format_session() function is defined inside the handler. Recommend extracting to module level to eliminate duplication (same logic at lines 330-343) and improve testability.

2. Magic Numbers

Several magic numbers should be extracted to constants:

  • 30 (minutes for upcoming, handler.py:141)
  • 20 (minutes fallback duration, handler.py:157)
  • 720 (minutes in filter_sessions(), handler.py:200, 205)

🎉 Conclusion

This is a well-implemented feature with smart performance optimizations.

Main concerns:

  1. Critical: No test coverage for datetime parsing logic
  2. Important: Code duplication in parsing logic and session formatting
  3. Minor: Magic numbers and minor edge case handling

Recommendation: Approve with changes

  • Add unit tests before merging
  • Refactor duplicated code
  • Extract magic numbers to constants

The SnapStart optimization alone makes this PR valuable. Great work! 🚀


Reviewed by: Claude Code | Files Changed: 3 (+200 / -27 lines) | Commits: 3

@ThomasHoussin ThomasHoussin merged commit 301a41c into master Nov 21, 2025
4 checks passed
@ThomasHoussin ThomasHoussin deleted the claude/add-sessions-now-parameter-01KjFdnJkCpLB63MuZi26rPH branch November 21, 2025 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants