Skip to content

Conversation

@razvanMiu
Copy link
Contributor

No description provided.

@razvanMiu razvanMiu requested review from avoinea and zotya October 13, 2025 07:00
@razvanMiu razvanMiu changed the base branch from master to develop October 13, 2025 07:03
Copy link
Member

@avoinea avoinea left a comment

Choose a reason for hiding this comment

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

⏺ 🚨 Critical Issues Found - NOT SAFE for Production

I've analyzed the changes and found several critical issues that could cause production crashes. Here's my detailed assessment:

Critical Problems:

  1. Crash Risk: Undefined Content Object (server.jsx:342-352)
  const content = store.getState().content.data;
  res.set({
    ...getDateHeader('Last-Modified', content.effective),
    'X-Page-Title': content.title,
    'X-Page-Description': content.description,
    'X-Page-Type': content['@type'],
  });

Problem: If content.data is undefined or null, accessing content.title, content.description, etc. will throw TypeError: Cannot read property 'title' of undefined and crash the server.

When this happens:

  • Empty Redux state
  • Failed API responses
  • Edge cases during SSR initialization
  • Race conditions in asyncConnect

⏺ 2. Invalid Date Header Values (server.jsx:60-65)

  function getDateHeader(name, date) {
    if (date) {
      return { [name]: new Date(date).toUTCString() };
    }
    return {};
  }

Problems:

  • If date is an invalid date string, new Date(date).toUTCString() returns "Invalid Date" which is not a valid HTTP header value
  • Empty strings are truthy but produce invalid dates
  • No validation of the resulting date
  1. Missing Headers on Error Paths

The code only sets these headers on the successful path (line 342), but not on:

  • Error pages (lines 315-340)
  • Redirects (line 314)
  • The global error handler (lines 87-113)

This creates inconsistent behavior for monitoring/indexing tools.

⏺ Specific Crash Scenarios:

  1. Homepage or pages without content.data:
    - Some pages might not have content.data populated
    - Result: TypeError → 500 error → site down
  2. API timeout/failure:
    - If backend API is slow/down, content.data may be undefined
    - Result: Server crashes instead of gracefully showing error
  3. Invalid date formats from backend:
    - Backend sends malformed dates
    - Result: "Invalid Date" in HTTP headers → proxies/CDNs may reject
  4. Missing properties:
    - Content without title, description, or @type
    - Result: undefined values in headers

Backward Compatibility:

✅ Good: Adding headers is backward compatible (doesn't break existing functionality)
❌ Bad: The crash risk means this could break currently working pages

⏺ Recommendation: 🛑 DO NOT MERGE

This needs defensive coding. Here's what should be fixed:

  function getDateHeader(name, date) {
    if (!date) return {};
    try {
      const dateObj = new Date(date);
      // Check if date is valid
      if (isNaN(dateObj.getTime())) return {};
      return { [name]: dateObj.toUTCString() };
    } catch (e) {
      return {};
    }
  }

  // In the main handler (line 342):
  const content = store.getState().content?.data;
  if (content) {
    res.set({
      ...getDateHeader('Last-Modified', content.effective),
      ...getDateHeader('Expires', content.expires),
      ...getDateHeader('X-Created-At', content.created),
      ...getDateHeader('X-Updated-At', content.modified),
      ...getDateHeader('X-Published-At', content.effective),
      ...(content.title && { 'X-Page-Title': content.title }),
      ...(content.description && { 'X-Page-Description': content.description }),
      ...(content['@type'] && { 'X-Page-Type': content['@type'] }),
    });
  }

Testing Recommendations:

Before deploying, test:

  1. Homepage with no content.data
  2. 404 pages
  3. API backend down/timeout scenarios
  4. Content with missing fields (no title, description, dates)
  5. Content with invalid date formats
  6. High-traffic load testing

⏺ Summary:

Aspect Status Risk Level
Null safety ❌ Missing HIGH
Invalid date handling ❌ Missing MEDIUM
Backward compatibility ⚠️ Partial MEDIUM
Header validation ❌ Missing LOW
Overall ❌ NOT SAFE HIGH

The feature itself (adding headers for AI indexers) is good, but the implementation lacks defensive programming. The most critical issue is the missing null check on content which will cause immediate
crashes in production when content.data is undefined.

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.

3 participants