Address review feedback on manual feed subscription
Some checks failed
CI / check (pull_request) Failing after 16s
CI / test (pull_request) Has been skipped
CI / audit (pull_request) Has been skipped

- Annotate the form onsubmit event type for clarity
- Disable the Subscribe button while the URL field is blank
- Rename title -> feed_title and invert the blank-title filter into a
  positively-named has_content helper, with a unit test
- Guard fetch_feed with error_for_status() so a well-formed URL that
  points at nothing (404/5xx) surfaces a clear error instead of an
  opaque feed-rs parse failure
- Collapse subscribe_feed_impl's two upsert_feed writes into one: fetch
  under a throwaway id first, then let upsert_feed be the single source
  of truth for the real feed id (existing id on re-subscribe, fresh
  otherwise), remapping fetched articles onto it before inserting. This
  also means a failed subscribe no longer leaves a placeholder row behind.
- Move the blank-URL check ahead of trimming

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XJJQb2DWZZwQ1yPiaAQQoY
This commit is contained in:
Austin Schaefer 2026-08-21 15:32:39 +02:00
parent d40c90552c
commit 8d316408b8
3 changed files with 45 additions and 13 deletions

View file

@ -7,14 +7,18 @@ use uuid::Uuid;
/// entries. `feed-rs` normalizes both RSS and Atom into a common model so
/// callers don't need to branch on feed type.
pub async fn fetch_feed(feed_url: &str, feed_id: Uuid) -> Result<(String, Vec<Article>)> {
let bytes = reqwest::get(feed_url).await?.bytes().await?;
// `.error_for_status()` turns a 404/5xx into a clear `Err` here rather
// than letting a non-feed error page fall through to a confusing
// feed-rs parse failure below.
let response = reqwest::get(feed_url).await?.error_for_status()?;
let bytes = response.bytes().await?;
let parsed = feed_rs::parser::parse(&bytes[..])?;
let title = parsed
let feed_title = parsed
.title
.as_ref()
.map(|t| t.content.clone())
.filter(|t| !t.trim().is_empty())
.filter(|t| has_content(t))
.unwrap_or_else(|| feed_url.to_string());
let articles = parsed
@ -47,7 +51,13 @@ pub async fn fetch_feed(feed_url: &str, feed_id: Uuid) -> Result<(String, Vec<Ar
})
.collect();
Ok((title, articles))
Ok((feed_title, articles))
}
/// True if `s` has any non-whitespace content — used to reject a
/// present-but-blank feed `<title>` rather than surface it as-is.
fn has_content(s: &str) -> bool {
!s.trim().is_empty()
}
/// Rough estimate from word count at ~200 wpm, used as the denominator for
@ -59,3 +69,17 @@ fn estimate_read_seconds(text: &str) -> u32 {
let words = text.split_whitespace().count() as u32;
((words.max(1) as f32 / 200.0) * 60.0) as u32
}
#[cfg(test)]
mod tests {
use super::*;
/// A blank or whitespace-only feed title should be treated the same as
/// a missing one, not surfaced as an empty string in the UI.
#[test]
fn has_content_rejects_blank_strings() {
assert!(!has_content(""));
assert!(!has_content(" \n\t"));
assert!(has_content("Rust Blog"));
}
}

View file

@ -73,7 +73,7 @@ fn SubscribeForm(on_subscribed: EventHandler<()>) -> Element {
rsx! {
form {
class: "subscribe-form",
onsubmit: move |ev| {
onsubmit: move |ev: FormEvent| {
ev.prevent_default();
submit(());
},
@ -88,7 +88,7 @@ fn SubscribeForm(on_subscribed: EventHandler<()>) -> Element {
Button {
r#type: "submit",
variant: ButtonVariant::Primary,
disabled: submitting(),
disabled: submitting() || url.read().trim().is_empty(),
if submitting() {
"Subscribing..."
} else {

View file

@ -72,16 +72,24 @@ pub async fn list_ranked_articles_impl(db: Db) -> Result<Vec<ArticleView>> {
/// already subscribed) and does an immediate first fetch so the reader
/// isn't empty until the next scheduled poll. Returns the number of
/// articles pulled in on this fetch.
///
/// Fetches under a throwaway id before writing anything, so there's exactly
/// one write to the `feeds` table (`upsert_feed` is the single source of
/// truth for the real id — the existing one on a re-subscribe, a fresh one
/// otherwise) and no half-registered row left behind if the fetch fails
/// (e.g. the URL is well-formed but points at nothing, or the feed no
/// longer exists — `fetch_feed` surfaces both as an `Err` via
/// `error_for_status`/feed-rs parse failure, which becomes the error
/// message shown in the subscribe form).
pub async fn subscribe_feed_impl(db: Db, url: String) -> Result<usize> {
anyhow::ensure!(!url.trim().is_empty(), "feed URL is required");
let url = url.trim();
anyhow::ensure!(!url.is_empty(), "feed URL is required");
// Register under a placeholder title first so the feed exists even if
// the fetch below fails (e.g. transient network error) — it'll be
// picked up by a later poll once feed polling is wired in.
let feed_id = db.upsert_feed(url, url).await?;
let (title, articles) = feedsignal_feeds::fetch_feed(url, feed_id).await?;
db.upsert_feed(url, &title).await?;
let (title, mut articles) = feedsignal_feeds::fetch_feed(url, Uuid::new_v4()).await?;
let feed_id = db.upsert_feed(url, &title).await?;
for article in &mut articles {
article.feed_id = feed_id;
}
let count = articles.len();
for article in &articles {