nginx fixes to get us out of 500-land

[kevinschaper]

No description provided.

[netlify]

✅ Deploy Preview for monarch-app canceled.

Name	Link
🔨 Latest commit	d3c3a67
🔍 Latest deploy log	https://app.netlify.com/projects/monarch-app/deploys/698cdea4f6420a0007e7871a

[claude]

PR Review: nginx fixes to get us out of 500-land

Summary

This PR refactors the nginx configuration to use map-based conditional logic instead of nested if statements, which is a significant improvement for nginx best practices.

✅ Positive Changes

1. Proper use of nginx map directive

• The new map "$is_bot:$is_entity_path" $serve_meta (lines 32-35) is the idiomatic nginx approach
• This eliminates the problematic string concatenation pattern (set $serve_meta "B" then set $serve_meta "${serve_meta}E")
• Maps are evaluated during configuration load, making this more performant than runtime if-checks

2. Cleaner location block

• Removed the nested if statements which are known to cause issues in nginx
• The proxy headers are now set unconditionally before the if statement, which is more reliable
• Better comments explaining the logic flow

3. Fixes potential 500 errors

• The old pattern of nested if statements can cause unexpected behavior in nginx due to the "if is evil" problem
• The map-based approach is deterministic and won't trigger internal nginx quirks

🔍 Code Quality Observations

Good:

• The regex pattern ~^/[A-Za-z_]+:[A-Za-z0-9_.-]+$ for entity paths looks correct for CURIE format
• Comments are clear and helpful
• Code is more maintainable and follows nginx conventions

Minor consideration:

• The proxy headers (lines 51-54) are now set even for requests that will serve static files via try_files
  • This is harmless but slightly inefficient
  • These headers only matter when proxying, but nginx will ignore them for static file serving
  • Consider moving them inside the if block if you want to be perfectly precise, though the current approach is acceptable

🔒 Security

✅ No security concerns identified:

• Proxy headers are set correctly for forwarding client information
• No user input is directly interpolated into variables
• The regex patterns are properly anchored with ^ and $

⚡ Performance

✅ Performance improvements:

• Map evaluation is faster than runtime string operations
• Reduced conditional logic complexity in the request processing path
• The break flag in the rewrite prevents unnecessary additional rewrites

🧪 Testing Recommendations

Since this changes critical routing logic, I recommend testing:

1. Bot behavior on entity pages: Verify bots get proxied to /v3/api/meta/{curie}

   curl -H "User-Agent: Twitterbot/1.0" http://localhost/MONDO:0005148

2. Regular users on entity pages: Should get the SPA

   curl http://localhost/MONDO:0005148

3. Bots on non-entity pages: Should get the SPA

   curl -H "User-Agent: Twitterbot/1.0" http://localhost/about

4. Check for 500 errors: Monitor nginx error logs during deployment

   docker logs <nginx-container> 2>&1 | grep -i error

📝 Summary

Verdict: ✅ Approve

This is a solid improvement that follows nginx best practices and should resolve the 500 errors mentioned in the title. The refactoring from nested if statements to a map-based approach is exactly the right fix for this type of conditional logic in nginx.

Recommendation: Merge after verifying the bot detection still works as expected in your staging environment.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.51%. Comparing base (3453d6a) to head (d3c3a67).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1255   +/-   ##
=======================================
  Coverage   73.51%   73.51%           
=======================================
  Files          95       95           
  Lines        3417     3417           
=======================================
  Hits         2512     2512           
  Misses        905      905           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.