fix: proper 404s + JSON profile pages - #60
Open
jb55 wants to merge 3 commits into
Open
Conversation
When serve() returned Err (e.g. Error::NotFound for a missing note), hyper's serve_connection aborted the connection without writing any HTTP response. A reverse proxy in front then translated the dropped upstream connection into a 502/503, so a routine "note not found" surfaced to clients as a 503 instead of a 404. Catch the error in the service closure and map it to a real response: NotFound -> 404, everything else -> 500. 4xx are logged at debug since unpropagated notes and bad ids are routine; 5xx still log at error. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the bare "Profile not found :(" text and plain-text "Invalid url"
response with a shared Damus-styled 404 page (render_not_found_html /
not_found_response). The main error handler now serves this page for
Error::NotFound, and the nip19-parse-failure and profile-missing paths
reuse it too, so every 404 gets consistent branding and a link home.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Implement serve_profile_json (previously a "todo: profile json" stub).
It returns the profile metadata broken out from nostrdb's parsed profile
record — pubkey, name, display_name, about, picture, banner, website,
nip05, lud06, lud16, lnurl (empty fields omitted) — plus a recent_notes
array (the same 12-note newest-first feed the HTML profile page shows).
Also serve JSON-shaped 404s on the .json endpoints instead of the HTML
404 page: not_found_json returns {"error":"not_found","message":...}.
The main error handler picks JSON vs HTML based on the request path, and
the invalid-bech32 path in serve() does the same. Drop the last inline
"Profile not found :(" in favor of the shared profile_not_found helper.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds a JSON-serialized profile endpoint with refactored 404 response helpers. New ChangesProfile JSON Endpoint and 404 Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/html.rs`:
- Around line 415-423: The current code swallows errors from ndb.query(...)
causing empty feeds instead of surfacing backend failures; change the block that
starts with ndb.query(...) to propagate query errors (use the ? operator or
return Err(...) from the enclosing function when ndb.query fails) rather than
using if let Ok(...). Keep the existing sorting and JSON parsing loop
(serde_json::from_str, result.note.json()) but ensure the call to
ndb.query(&txn, &[notes_filter], PROFILE_FEED_RECENT_LIMIT as i32) is assigned
with a fallible binding (e.g., let results = ndb.query(...) ?;) so query
failures are returned to the caller.
In `@src/main.rs`:
- Around line 726-731: In the generic error arm (`_ => { ... }`) where `err` is
currently interpolated into the response body, keep logging the full `err`
server-side but stop returning it to clients; instead detect if the request
expects JSON (inspect the request's Accept header or the request path/extension)
and, for JSON requests, return a 500 with a generic JSON body (e.g.
{"error":"internal_server_error"}) and Content-Type application/json, otherwise
return a generic plain-text 500 message; update the `Response::builder()` call
that currently uses `Bytes::from(format!("{}\n", err))` to produce the sanitized
payload while leaving the `error!` log unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+415
to
+423
| if let Ok(mut results) = ndb.query(&txn, &[notes_filter], PROFILE_FEED_RECENT_LIMIT as i32) { | ||
| results.sort_by_key(|result| result.note.created_at()); | ||
| results.reverse(); | ||
| for result in &results { | ||
| if let Ok(note_value) = serde_json::from_str::<Value>(&result.note.json()?) { | ||
| recent_notes.push(note_value); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Propagate recent-note query failures instead of returning an empty feed.
Lines 415-423 swallow ndb.query(...) errors and still return 200 with recent_notes: []. That makes a backend failure indistinguishable from a genuinely empty profile, so API clients can cache incomplete data as truth.
Suggested fix
- if let Ok(mut results) = ndb.query(&txn, &[notes_filter], PROFILE_FEED_RECENT_LIMIT as i32) {
- results.sort_by_key(|result| result.note.created_at());
- results.reverse();
- for result in &results {
- if let Ok(note_value) = serde_json::from_str::<Value>(&result.note.json()?) {
- recent_notes.push(note_value);
- }
- }
- }
+ let mut results = ndb.query(&txn, &[notes_filter], PROFILE_FEED_RECENT_LIMIT as i32)?;
+ results.sort_by_key(|result| result.note.created_at());
+ results.reverse();
+ for result in &results {
+ if let Ok(note_value) = serde_json::from_str::<Value>(&result.note.json()?) {
+ recent_notes.push(note_value);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if let Ok(mut results) = ndb.query(&txn, &[notes_filter], PROFILE_FEED_RECENT_LIMIT as i32) { | |
| results.sort_by_key(|result| result.note.created_at()); | |
| results.reverse(); | |
| for result in &results { | |
| if let Ok(note_value) = serde_json::from_str::<Value>(&result.note.json()?) { | |
| recent_notes.push(note_value); | |
| } | |
| } | |
| } | |
| let mut results = ndb.query(&txn, &[notes_filter], PROFILE_FEED_RECENT_LIMIT as i32)?; | |
| results.sort_by_key(|result| result.note.created_at()); | |
| results.reverse(); | |
| for result in &results { | |
| if let Ok(note_value) = serde_json::from_str::<Value>(&result.note.json()?) { | |
| recent_notes.push(note_value); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/html.rs` around lines 415 - 423, The current code swallows errors from
ndb.query(...) causing empty feeds instead of surfacing backend failures; change
the block that starts with ndb.query(...) to propagate query errors (use the ?
operator or return Err(...) from the enclosing function when ndb.query fails)
rather than using if let Ok(...). Keep the existing sorting and JSON parsing
loop (serde_json::from_str, result.note.json()) but ensure the call to
ndb.query(&txn, &[notes_filter], PROFILE_FEED_RECENT_LIMIT as i32) is assigned
with a fallible binding (e.g., let results = ndb.query(...) ?;) so query
failures are returned to the caller.
Comment on lines
+726
to
+731
| _ => { | ||
| error!("serve error (500): {}", err); | ||
| Response::builder() | ||
| .status(StatusCode::INTERNAL_SERVER_ERROR) | ||
| .body(Full::new(Bytes::from(format!("{}\n", err)))) | ||
| .expect("building error response") |
There was a problem hiding this comment.
Don't expose raw internal errors on .json requests.
Lines 726-731 return format!("{}\n", err) for every 500. That leaks internal error details to clients, and .json endpoints stop being JSON on the error path. Log the real error server-side and return a generic structured 500 instead.
Suggested fix
- _ => {
- error!("serve error (500): {}", err);
- Response::builder()
- .status(StatusCode::INTERNAL_SERVER_ERROR)
- .body(Full::new(Bytes::from(format!("{}\n", err))))
- .expect("building error response")
- }
+ _ => {
+ error!("serve error (500): {}", err);
+ if wants_json {
+ Response::builder()
+ .status(StatusCode::INTERNAL_SERVER_ERROR)
+ .header(
+ header::CONTENT_TYPE,
+ "application/json; charset=utf-8",
+ )
+ .body(Full::new(Bytes::from(
+ r#"{"error":"internal_server_error","message":"Internal server error"}"#,
+ )))
+ .expect("building error response")
+ } else {
+ Response::builder()
+ .status(StatusCode::INTERNAL_SERVER_ERROR)
+ .header(
+ header::CONTENT_TYPE,
+ "text/plain; charset=utf-8",
+ )
+ .body(Full::new(Bytes::from(
+ "Internal server error\n",
+ )))
+ .expect("building error response")
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _ => { | |
| error!("serve error (500): {}", err); | |
| Response::builder() | |
| .status(StatusCode::INTERNAL_SERVER_ERROR) | |
| .body(Full::new(Bytes::from(format!("{}\n", err)))) | |
| .expect("building error response") | |
| _ => { | |
| error!("serve error (500): {}", err); | |
| if wants_json { | |
| Response::builder() | |
| .status(StatusCode::INTERNAL_SERVER_ERROR) | |
| .header( | |
| header::CONTENT_TYPE, | |
| "application/json; charset=utf-8", | |
| ) | |
| .body(Full::new(Bytes::from( | |
| r#"{"error":"internal_server_error","message":"Internal server error"}"#, | |
| ))) | |
| .expect("building error response") | |
| } else { | |
| Response::builder() | |
| .status(StatusCode::INTERNAL_SERVER_ERROR) | |
| .header( | |
| header::CONTENT_TYPE, | |
| "text/plain; charset=utf-8", | |
| ) | |
| .body(Full::new(Bytes::from( | |
| "Internal server error\n", | |
| ))) | |
| .expect("building error response") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main.rs` around lines 726 - 731, In the generic error arm (`_ => { ...
}`) where `err` is currently interpolated into the response body, keep logging
the full `err` server-side but stop returning it to clients; instead detect if
the request expects JSON (inspect the request's Accept header or the request
path/extension) and, for JSON requests, return a 500 with a generic JSON body
(e.g. {"error":"internal_server_error"}) and Content-Type application/json,
otherwise return a generic plain-text 500 message; update the
`Response::builder()` call that currently uses `Bytes::from(format!("{}\n",
err))` to produce the sanitized payload while leaving the `error!` log
unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes spurious 503s on missing notes and rounds out the JSON API.
1. Return proper HTTP status codes instead of aborting the connection
When
serve()returnedErr(e.g.Error::NotFoundfor a missing note), hyper'sserve_connectionaborted the connection without writing any HTTP response. A reverse proxy in front then turned the dropped upstream connection into a 502/503, so a routine "note not found" surfaced to clients as a 503 instead of a 404. The service closure now catches the error and maps it to a real response:NotFound → 404, everything else → 500. 4xx log atdebug(unpropagated notes / bad ids are routine); 5xx still log aterror.2. Styled 404 page
A shared, Damus-themed 404 page (
not_found_response) replaces the bare"Profile not found :("text and plain-text"Invalid url". Used by the missing-note/profile, invalid-bech32, and profile-missing paths.3. JSON profile pages + JSON 404s
serve_profile_json(previously a"todo: profile json"stub) returns the parsed profile metadata from nostrdb —pubkey,name,display_name,about,picture,banner,website,nip05,lud06,lud16,lnurl(empty fields omitted) — plus arecent_notesarray (the same 12-note newest-first feed the HTML page shows). The.jsonendpoints now return JSON-shaped 404s ({"error":"not_found","message":...}) instead of the HTML 404 page.Testing
cargo test— all 34 pass.json→ 404application/json; real profile.json→ broken-out metadata + 12 recent notes; missing note HTML → styled 404; homepage → 200🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements
.jsonsuffix.