use pi-scout agent to find smells and suggest refactorings

context.md

@@ -0,0 +1,172 @@
+# Code Context
+
+## Files Retrieved
+List with exact line ranges:
+1. `app/test/firehose_web/controllers/blog_test.exs` (lines 1-128) - Comprehensive blog controller tests covering HTML and JSON API endpoints for engineering blog and release notes
+2. `app/lib/firehose_web/controllers/blog_controller.ex` (lines 1-79) - Blog controller with pagination, 404 handling, and input validation
+3. `app/test/support/conn_case.ex` (lines 1-38) - Test case template for connection tests
+4. `app/lib/firehose/blogs/engineering_blog.ex` (lines 1-7) - Engineering blog module configuration
+5. `app/lib/firehose/blogs/release_notes.ex` (lines 1-7) - Release notes blog module configuration
+
+## Key Code
+
+### Test Organization
+```elixir
+# Current structure has 4 describe blocks:
+describe "engineering blog (HTML)"     # 3 tests
+describe "input validation"           # 5 tests (newly added in last commit)
+describe "release notes blog (HTML)"  # 2 tests
+describe "engineering blog (JSON API)" # 4 tests  
+describe "release notes blog (JSON API)" # 3 tests
+```
+
+### Input Validation Logic (blog_controller.ex, lines 68-76)
+```elixir
+defp parse_page(nil), do: 1
+defp parse_page(str) do
+  case Integer.parse(str) do
+    {page, ""} when page > 0 -> page
+    _ -> 1
+  end
+end
+```
+
+### Test Coverage Added in Last Commit
+```elixir
+describe "input validation" do
+  test "GET /blog/nonexistent returns 404", %{conn: conn} do
+    conn = get(conn, "/blog/nonexistent")
+    assert html_response(conn, 404)
+  end
+
+  test "GET /blog/engineering?page=abc falls back to page 1", %{conn: conn} do
+    conn = get(conn, "/blog/engineering?page=abc")
+    assert html_response(conn, 200) =~ "Engineering Blog"
+  end
+
+  test "GET /blog/engineering?page=-1 falls back to page 1", %{conn: conn} do
+    conn = get(conn, "/blog/engineering?page=-1")
+    assert html_response(conn, 200) =~ "Engineering Blog"
+  end
+
+  test "GET /blog/engineering?page=0 falls back to page 1", %{conn: conn} do
+    conn = get(conn, "/blog/engineering?page=0")
+    assert html_response(conn, 200) =~ "Engineering Blog"
+  end
+
+  test "GET /blog/engineering/nonexistent-post returns 404", %{conn: conn} do
+    assert_raise Blogex.NotFoundError, fn ->
+      get(conn, "/blog/engineering/nonexistent-post")
+    end
+  end
+end
+```
+
+## Architecture
+The application uses:
+- **Blogex** library for blog functionality (engineering blog and release notes)
+- **Phoenix** framework for web endpoints
+- **ConnCase** test helper for connection testing
+- Two blog types: `Firehose.EngineeringBlog` and `Firehose.ReleaseNotes`
+- Pagination through `blog.paginate(page)` method
+- 404 handling via `Blogex.NotFoundError` exception
+
+## Start Here
+Which file to look at first and why:
+
+**Start with `app/lib/firehose_web/controllers/blog_controller.ex`**
+
+Why: This is the central controller that handles all blog requests. Understanding its structure (especially the `parse_page/1` function and `resolve_blog/2` plug) provides context for why the validation tests were added and how input handling works across both HTML and JSON endpoints.
+
+## Code Smells & Refactoring Suggestions
+
+### Smell 1: Repetitive Validation Tests
+**Issue**: Four tests for page parameter validation (`page=abc`, `-1`, `0`, and valid values) are highly repetitive with identical assertions.
+
+**Refactoring Suggestion**: Use parameterized tests or test helpers:
+```elixir
+# Test helper approach
+test_page_fallback("page=abc", "abc")
+test_page_fallback("page=-1", "-1") 
+test_page_fallback("page=0", "0")
+
+defp test_page_fallback(query_param, expected_page) do
+  conn = get(conn, "/blog/engineering?#{query_param}")
+  assert html_response(conn, 200) =~ "Engineering Blog"
+end
+```
+
+### Smell 2: Missing Negative Test Coverage
+**Issue**: Tests don't verify what happens when invalid blog_id is provided (e.g., `/blog/invalid-blog`).
+
+**Refactoring Suggestion**: Add test for unknown blog:
+```elixir
+test "GET /blog/unknown returns 404", %{conn: conn} do
+  conn = get(conn, "/blog/unknown")
+  assert html_response(conn, 404)
+end
+```
+
+### Smell 3: Inconsistent Test Naming
+**Issue**: Some tests use hyphenated slugs (`v0-1-0`), others use different formats. The naming doesn't clearly indicate what's being tested.
+
+**Refactoring Suggestion**: Standardize naming:
+```elixir
+# Instead of: "GET /blog/releases/v0-1-0 returns HTML post"
+test "GET /blog/releases/:slug returns a release post", %{conn: conn} do
+```
+
+### Smell 4: Redundant Layout Assertions
+**Issue**: Multiple tests assert the same "firehose" string appears in response, testing layout presence.
+
+**Refactoring Suggestion**: Create a shared test helper:
+```elixir
+defp assert_has_app_layout(body), 
+  do: assert body =~ "firehose"
+
+# Then in tests: assert_has_app_layout(body)
+```
+
+### Smell 5: Test Order Doesn't Follow Flow
+**Issue**: Tests are grouped by endpoint but validation tests (which should be first for defensive programming) are in the middle.
+
+**Refactoring Suggestion**: Reorder to follow natural request flow:
+1. Input validation (404s, invalid params)
+2. Success cases (index, show, tag)
+3. Edge cases (pagination, RSS feeds)
+
+### Smell 6: No Test for Controller-Level Error Handling
+**Issue**: The controller uses `halt()` in the resolve_blog plug, but there's no test verifying this behavior.
+
+**Refactoring Suggestion**: Add test:
+```elixir
+test "GET /blog/:blog_id with invalid blog halts request", %{conn: conn} do
+  conn = get(conn, "/blog/invalid")
+  assert conn.halted
+end
+```
+
+### Smell 7: Mixed Response Types Without Clear Separation
+**Issue**: HTML tests use `html_response/2`, JSON tests use `json_response/2`, but there's no helper to verify content type before parsing.
+
+**Refactoring Suggestion**: Create response helpers:
+```elixir
+defp assert_html(conn, status), do: assert html_response(conn, status) != ""
+defp assert_json(conn, status), do: assert json_response(conn, status) != %{}
+```
+
+### Smell 8: No Test for Concurrent Requests or Edge Cases
+**Issue**: Missing tests for:
+- Empty page parameter (`?page=`)
+- Very large page numbers
+- Special characters in slug/tag parameters
+
+**Refactoring Suggestion**: Add edge case tests to validation describe block.
+
+### Overall Recommendations
+1. **Extract test helpers** to reduce duplication (especially for page validation)
+2. **Standardize test naming** conventions across all blog types
+3. **Add positive test** for valid page numbers (currently missing)
+4. **Consider property-based testing** for input validation scenarios
+5. **Add performance tests** if pagination is used heavily
+6. **Create integration tests** that verify end-to-end flows