Add schedule image digestion via Claude Vision API #208

Merged

ldraney merged 2 commits from 204-schedule-digest into main 2026-06-14 02:31:15 +00:00

Conversation 2 Commits 2 Files changed 14 +1015 -2

ldraney commented 2026-06-13 22:16:29 +00:00

Owner

Copy link

Summary

Photograph a printed weekly schedule and let Claude Vision extract property names per day, fuzzy-match them against existing Property records, then create WorkQueueItems after user confirmation. Handles the boss's column-overflow pattern where one day's list spills into the next column.

Changes

• Gemfile -- added gem "anthropic" for Claude Vision API access
• app/services/schedule_digester.rb -- new service orchestrating Vision API call, JSON parsing, and Levenshtein-based fuzzy matching against Property.client_name
• db/migrate/20260613000000_add_week_start_to_uploads.rb -- adds week_start:date to uploads table
• app/models/upload.rb -- added vision_compatible? helper and VISION_MEDIA_TYPES constant
• app/controllers/uploads_controller.rb -- added digest and confirm_digest actions with error handling for API/parse failures
• app/views/uploads/digest.html.erb -- confirmation UI showing exact matches (green), fuzzy matches (yellow), and unmatched names (gray/skipped)
• app/views/uploads/show.html.erb -- added week date picker and "Digest Schedule" button
• config/routes.rb -- added digest and confirm_digest member routes under uploads
• app/assets/stylesheets/application.css -- digest component styles following existing design tokens
• .env.example -- added ANTHROPIC_API_KEY placeholder
• spec/services/schedule_digester_spec.rb -- 24 unit tests for Levenshtein, matching, date math, item creation, and response parsing
• spec/requests/uploads_spec.rb -- 18 request specs covering digest actions, error handling, and duplicate skipping

Test Plan

• bundle exec rspec -- 342 examples, 0 failures
• Verify "Digest Schedule" button appears on upload show page
• Upload a schedule photo, click digest, confirm matched properties are correct
• Verify duplicate WorkQueueItems are skipped without error
• Verify unmatched names are displayed but never create Property records

Review Checklist

• Tests pass (342 examples, 0 failures)
• No unrelated changes
• No new Property records created (zero-bloat constraint)
• Duplicate WorkQueueItems handled gracefully
• API errors surfaced to user with retry option
• CSS follows existing design token patterns

Related Notes

None

Related

Closes #204

## Summary Photograph a printed weekly schedule and let Claude Vision extract property names per day, fuzzy-match them against existing Property records, then create WorkQueueItems after user confirmation. Handles the boss's column-overflow pattern where one day's list spills into the next column. ## Changes - `Gemfile` -- added `gem "anthropic"` for Claude Vision API access - `app/services/schedule_digester.rb` -- new service orchestrating Vision API call, JSON parsing, and Levenshtein-based fuzzy matching against Property.client_name - `db/migrate/20260613000000_add_week_start_to_uploads.rb` -- adds `week_start:date` to uploads table - `app/models/upload.rb` -- added `vision_compatible?` helper and VISION_MEDIA_TYPES constant - `app/controllers/uploads_controller.rb` -- added `digest` and `confirm_digest` actions with error handling for API/parse failures - `app/views/uploads/digest.html.erb` -- confirmation UI showing exact matches (green), fuzzy matches (yellow), and unmatched names (gray/skipped) - `app/views/uploads/show.html.erb` -- added week date picker and "Digest Schedule" button - `config/routes.rb` -- added `digest` and `confirm_digest` member routes under uploads - `app/assets/stylesheets/application.css` -- digest component styles following existing design tokens - `.env.example` -- added ANTHROPIC_API_KEY placeholder - `spec/services/schedule_digester_spec.rb` -- 24 unit tests for Levenshtein, matching, date math, item creation, and response parsing - `spec/requests/uploads_spec.rb` -- 18 request specs covering digest actions, error handling, and duplicate skipping ## Test Plan - `bundle exec rspec` -- 342 examples, 0 failures - Verify "Digest Schedule" button appears on upload show page - Upload a schedule photo, click digest, confirm matched properties are correct - Verify duplicate WorkQueueItems are skipped without error - Verify unmatched names are displayed but never create Property records ## Review Checklist - [x] Tests pass (342 examples, 0 failures) - [x] No unrelated changes - [x] No new Property records created (zero-bloat constraint) - [x] Duplicate WorkQueueItems handled gracefully - [x] API errors surfaced to user with retry option - [x] CSS follows existing design token patterns ## Related Notes None ## Related Closes #204

ldraney added 1 commit 2026-06-13 22:16:29 +00:00

Add schedule image digestion via Claude Vision API (#204) ... a112c64495

Photograph a printed weekly schedule and let Claude Vision extract
property names per day, fuzzy-match them against existing Property
records, then create WorkQueueItems after user confirmation.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented 2026-06-13 22:18:34 +00:00

Author

Owner

Copy link

PR #208 Review

DOMAIN REVIEW

Stack: Ruby on Rails 8.1, PostgreSQL, ActiveStorage, ERB views, vanilla CSS, RSpec.

Service object pattern (ScheduleDigester)

The service is well-structured: single public #call method, dependency injection for the API client (client: parameter), clear separation between extraction, parsing, and matching. The class-level create_work_queue_items method that handles persistence is a clean boundary.

One notable code smell in match_name (lines ~114-126 of the service): the two branches of the ambiguous-match conditional produce identical results:

if scored[0][:distance] < scored[1][:distance]
  { match: :fuzzy, property: scored[0][:property], candidates: scored.map { |s| s[:property] } }
else
  { match: :fuzzy, property: scored[0][:property], candidates: scored.map { |s| s[:property] } }
end

The comment says "if the best match is clearly better (distance gap >= 2), pick it" but the implementation ignores the gap entirely. Both branches do the same thing, and the threshold of 2 mentioned in the comment is not applied. This is dead logic -- the if/else can be collapsed to a single expression. This is not a blocker but it is misleading: a future reader might trust the comment and believe ambiguous ties are handled differently.

Levenshtein implementation

The O(n*m) matrix implementation is correct for the use case (short property names). No issues with the algorithm itself. The threshold of 3 is reasonable for landscaping client names.

Fuzzy matching edge case: normalize strips all non-alphanumeric characters. A name like "O'Brien" becomes "obrien" (7 chars). If a property is "OBrien" it normalizes to "obrien" -- exact match. If a property is stored as "O'Brien" it also normalizes to "obrien" -- exact match. This is actually desirable behavior. Good.

Controller security

• The controller inherits require_role :lead, :admin, :super_admin at the class level (line 2 of current uploads_controller.rb). The new digest and confirm_digest actions are NOT excluded from this filter, so they are properly role-gated. Correct.
• Authentication is enforced via ApplicationController's before_action :authenticate_user!. Correct.
• Upload.find(params[:id]) will raise ActiveRecord::RecordNotFound (which Rails maps to 404) for invalid IDs. Correct.
• parse_week_start has a rescue for Date::Error, falling back to current Monday. Defensive. Good.
• build_day_assignments whitelists day names via ScheduleDigester::DAYS.each, preventing arbitrary keys from reaching the service. The to_i.reject(&:zero?) pattern prevents non-numeric property IDs. Correct.

API key handling

• ENV.fetch("ANTHROPIC_API_KEY") will raise KeyError at runtime if the env var is missing. This is good -- fail fast rather than sending a nil API key. However, this error is NOT caught by the rescue clauses in the digest action (they only catch ApiError and ParseError). A missing API key will produce an unhandled KeyError with a 500 error. This should either be caught or documented as a deploy requirement.

View safety (XSS, CSRF)

• All user-facing text (entry[:name], entry[:property].client_name) is output via ERB <%= %> which auto-escapes HTML. No raw output. Safe.
• form_with generates CSRF tokens by default in Rails. Both forms use form_with. Safe.
• The digest form uses method: :post for both digest and confirm_digest. Correct -- these are state-changing operations.
• Property IDs in hidden fields come from the DB and are integers. No injection risk.

Migration

• add_column :uploads, :week_start, :date -- simple, reversible, no data loss. Correct.
• The column is nullable, which is appropriate since existing uploads won't have a week_start.

Schema drift note: The db/schema.rb diff introduces a daily_notes table that is NOT part of this PR's migration. This is a side effect of schema regeneration picking up a previously merged migration. The PR itself is clean -- this is just schema.rb noise.

Zero-bloat constraint

• Confirmed: no Property.create or Property.new calls anywhere in the service or controller. Unmatched names are displayed with "No match -- skipped" and never persisted. Constraint satisfied.

Duplicate handling

• WorkQueueItem has a uniqueness validation at the model level (validates :property_id, uniqueness: { scope: :work_date }) and a unique DB index (index_work_queue_items_on_work_date_and_property_id).
• The service uses find_by to detect existing items before create!. This is a check-then-act pattern that has a theoretical race condition (two concurrent confirm_digest requests for the same day/property), but the unique index provides a DB-level safety net -- the second create! would raise ActiveRecord::RecordNotUnique. That exception is NOT caught, so a race would produce a 500. Extremely unlikely in practice for a single-user-at-a-time landscaping app, but worth noting.

Test coverage

• 24 unit tests for the service covering: Levenshtein, name matching (exact, fuzzy, none, blank, ambiguous), date_for_day, create_work_queue_items (create, skip, positions, empty).
• 18 request specs covering: digest success, API error, parse error, week_start persistence, default week_start, confirm_digest creation, duplicate skipping, empty assignments, missing assignments param.
• Response parsing tests cover: valid JSON, markdown-fenced JSON, invalid JSON, non-object JSON, missing days.
• Coverage is thorough. Happy path, error paths, and edge cases are all present.

BLOCKERS

None.

NITS

1. Dead conditional in match_name (service line ~114-126): The if scored[0][:distance] < scored[1][:distance] branch and its else produce identical output. The comment about a "distance gap >= 2" is not implemented. Either implement the intended disambiguation logic or collapse to a single expression and update the comment.

2. Hardcoded color #fffbeb in CSS (.digest-item-fuzzy) and #f59e0b (.digest-badge-fuzzy): Every other color in the component uses design tokens (var(--color-success), var(--color-border), etc.). These two should be var(--color-warning-light) and var(--color-warning) or similar tokens for consistency. If those tokens don't exist yet, add them.

3. Unhandled KeyError: If ANTHROPIC_API_KEY is not set, ENV.fetch raises KeyError which is not caught by the rescue clauses. Consider either: (a) adding rescue KeyError to the digest action with a user-friendly message, or (b) adding a model-level or initializer check that fails at boot time.

4. Race condition on duplicate detection: find_by + create! without a rescue ActiveRecord::RecordNotUnique. The unique index saves data integrity, but the unrescued exception means a 500 on concurrent duplicate. Consider wrapping in a rescue or using find_or_create_by.

5. Model file (upload.rb) missing the new column: The diff adds VISION_MEDIA_TYPES and vision_compatible? to the model, but the model in the diff does not have week_start in any attribute whitelist or validation. This is fine for Rails (dynamic attributes), but a comment noting week_start is set by the controller would help future readers.

6. daily_notes in schema.rb: This table appears in the schema diff but has no corresponding migration in this PR. If this is leftover from a prior merge, the schema.rb change is correct but noisy. Not a blocker.

SOP COMPLIANCE

• PR body has: ## Summary, ## Changes, ## Test Plan, ## Related
• Tests exist and cover new functionality (42 new examples)
• No secrets, .env files, or credentials committed (.env.example only has placeholder)
• No unnecessary file changes -- all 14 changed files are directly related to the feature
• Commit messages are descriptive

PROCESS OBSERVATIONS

• Deployment risk: Low. The migration is additive (nullable column). The Anthropic gem is a new dependency but well-maintained. No breaking changes to existing functionality.
• Change failure risk: Low. The feature is isolated behind a button on the upload show page. Existing upload functionality is unchanged.
• Documentation: The .env.example update documents the new env var requirement. The service has thorough inline documentation. The PR body is comprehensive.
• API cost awareness: Each digest call sends a full image to Claude Vision API. No rate limiting or cost tracking is implemented. For a small landscaping operation this is fine, but worth monitoring.

VERDICT: APPROVED

Solid, well-tested feature with proper role enforcement, XSS safety, and defensive error handling. The nits are all non-blocking improvements -- the dead conditional is the most worth addressing for code clarity, but none block merge.

## PR #208 Review ### DOMAIN REVIEW **Stack:** Ruby on Rails 8.1, PostgreSQL, ActiveStorage, ERB views, vanilla CSS, RSpec. **Service object pattern (ScheduleDigester)** The service is well-structured: single public `#call` method, dependency injection for the API client (`client:` parameter), clear separation between extraction, parsing, and matching. The class-level `create_work_queue_items` method that handles persistence is a clean boundary. One notable code smell in `match_name` (lines ~114-126 of the service): the two branches of the ambiguous-match conditional produce **identical results**: ```ruby if scored[0][:distance] < scored[1][:distance] { match: :fuzzy, property: scored[0][:property], candidates: scored.map { |s| s[:property] } } else { match: :fuzzy, property: scored[0][:property], candidates: scored.map { |s| s[:property] } } end ``` The comment says "if the best match is clearly better (distance gap >= 2), pick it" but the implementation ignores the gap entirely. Both branches do the same thing, and the threshold of 2 mentioned in the comment is not applied. This is dead logic -- the `if/else` can be collapsed to a single expression. This is not a blocker but it is misleading: a future reader might trust the comment and believe ambiguous ties are handled differently. **Levenshtein implementation** The O(n*m) matrix implementation is correct for the use case (short property names). No issues with the algorithm itself. The threshold of 3 is reasonable for landscaping client names. **Fuzzy matching edge case**: `normalize` strips all non-alphanumeric characters. A name like "O'Brien" becomes "obrien" (7 chars). If a property is "OBrien" it normalizes to "obrien" -- exact match. If a property is stored as "O'Brien" it also normalizes to "obrien" -- exact match. This is actually desirable behavior. Good. **Controller security** - The controller inherits `require_role :lead, :admin, :super_admin` at the class level (line 2 of current `uploads_controller.rb`). The new `digest` and `confirm_digest` actions are NOT excluded from this filter, so they are properly role-gated. Correct. - Authentication is enforced via `ApplicationController`'s `before_action :authenticate_user!`. Correct. - `Upload.find(params[:id])` will raise `ActiveRecord::RecordNotFound` (which Rails maps to 404) for invalid IDs. Correct. - `parse_week_start` has a rescue for `Date::Error`, falling back to current Monday. Defensive. Good. - `build_day_assignments` whitelists day names via `ScheduleDigester::DAYS.each`, preventing arbitrary keys from reaching the service. The `to_i.reject(&:zero?)` pattern prevents non-numeric property IDs. Correct. **API key handling** - `ENV.fetch("ANTHROPIC_API_KEY")` will raise `KeyError` at runtime if the env var is missing. This is good -- fail fast rather than sending a nil API key. However, this error is NOT caught by the rescue clauses in the `digest` action (they only catch `ApiError` and `ParseError`). A missing API key will produce an unhandled `KeyError` with a 500 error. This should either be caught or documented as a deploy requirement. **View safety (XSS, CSRF)** - All user-facing text (`entry[:name]`, `entry[:property].client_name`) is output via ERB `<%= %>` which auto-escapes HTML. No raw output. Safe. - `form_with` generates CSRF tokens by default in Rails. Both forms use `form_with`. Safe. - The digest form uses `method: :post` for both `digest` and `confirm_digest`. Correct -- these are state-changing operations. - Property IDs in hidden fields come from the DB and are integers. No injection risk. **Migration** - `add_column :uploads, :week_start, :date` -- simple, reversible, no data loss. Correct. - The column is nullable, which is appropriate since existing uploads won't have a week_start. **Schema drift note**: The `db/schema.rb` diff introduces a `daily_notes` table that is NOT part of this PR's migration. This is a side effect of schema regeneration picking up a previously merged migration. The PR itself is clean -- this is just schema.rb noise. **Zero-bloat constraint** - Confirmed: no `Property.create` or `Property.new` calls anywhere in the service or controller. Unmatched names are displayed with "No match -- skipped" and never persisted. Constraint satisfied. **Duplicate handling** - `WorkQueueItem` has a uniqueness validation at the model level (`validates :property_id, uniqueness: { scope: :work_date }`) and a unique DB index (`index_work_queue_items_on_work_date_and_property_id`). - The service uses `find_by` to detect existing items before `create!`. This is a check-then-act pattern that has a theoretical race condition (two concurrent confirm_digest requests for the same day/property), but the unique index provides a DB-level safety net -- the second `create!` would raise `ActiveRecord::RecordNotUnique`. That exception is NOT caught, so a race would produce a 500. Extremely unlikely in practice for a single-user-at-a-time landscaping app, but worth noting. **Test coverage** - 24 unit tests for the service covering: Levenshtein, name matching (exact, fuzzy, none, blank, ambiguous), date_for_day, create_work_queue_items (create, skip, positions, empty). - 18 request specs covering: digest success, API error, parse error, week_start persistence, default week_start, confirm_digest creation, duplicate skipping, empty assignments, missing assignments param. - Response parsing tests cover: valid JSON, markdown-fenced JSON, invalid JSON, non-object JSON, missing days. - Coverage is thorough. Happy path, error paths, and edge cases are all present. ### BLOCKERS None. ### NITS 1. **Dead conditional in `match_name`** (service line ~114-126): The `if scored[0][:distance] < scored[1][:distance]` branch and its `else` produce identical output. The comment about a "distance gap >= 2" is not implemented. Either implement the intended disambiguation logic or collapse to a single expression and update the comment. 2. **Hardcoded color `#fffbeb`** in CSS (`.digest-item-fuzzy`) and `#f59e0b` (`.digest-badge-fuzzy`): Every other color in the component uses design tokens (`var(--color-success)`, `var(--color-border)`, etc.). These two should be `var(--color-warning-light)` and `var(--color-warning)` or similar tokens for consistency. If those tokens don't exist yet, add them. 3. **Unhandled `KeyError`**: If `ANTHROPIC_API_KEY` is not set, `ENV.fetch` raises `KeyError` which is not caught by the `rescue` clauses. Consider either: (a) adding `rescue KeyError` to the digest action with a user-friendly message, or (b) adding a model-level or initializer check that fails at boot time. 4. **Race condition on duplicate detection**: `find_by` + `create!` without a `rescue ActiveRecord::RecordNotUnique`. The unique index saves data integrity, but the unrescued exception means a 500 on concurrent duplicate. Consider wrapping in a rescue or using `find_or_create_by`. 5. **Model file (`upload.rb`) missing the new column**: The diff adds `VISION_MEDIA_TYPES` and `vision_compatible?` to the model, but the model in the diff does not have `week_start` in any attribute whitelist or validation. This is fine for Rails (dynamic attributes), but a comment noting `week_start` is set by the controller would help future readers. 6. **`daily_notes` in schema.rb**: This table appears in the schema diff but has no corresponding migration in this PR. If this is leftover from a prior merge, the schema.rb change is correct but noisy. Not a blocker. ### SOP COMPLIANCE - [x] PR body has: ## Summary, ## Changes, ## Test Plan, ## Related - [x] Tests exist and cover new functionality (42 new examples) - [x] No secrets, .env files, or credentials committed (`.env.example` only has placeholder) - [x] No unnecessary file changes -- all 14 changed files are directly related to the feature - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - **Deployment risk**: Low. The migration is additive (nullable column). The Anthropic gem is a new dependency but well-maintained. No breaking changes to existing functionality. - **Change failure risk**: Low. The feature is isolated behind a button on the upload show page. Existing upload functionality is unchanged. - **Documentation**: The `.env.example` update documents the new env var requirement. The service has thorough inline documentation. The PR body is comprehensive. - **API cost awareness**: Each digest call sends a full image to Claude Vision API. No rate limiting or cost tracking is implemented. For a small landscaping operation this is fine, but worth monitoring. ### VERDICT: APPROVED Solid, well-tested feature with proper role enforcement, XSS safety, and defensive error handling. The nits are all non-blocking improvements -- the dead conditional is the most worth addressing for code clarity, but none block merge.

ldraney added 1 commit 2026-06-13 22:19:52 +00:00

Address review nits: dead conditional, KeyError, race condition, design tokens ... 2f97a3d89b

- Collapse duplicate if/else branches in fuzzy match_name logic
- Catch missing ANTHROPIC_API_KEY as ApiError instead of raw KeyError
- Rescue ActiveRecord::RecordNotUnique on concurrent duplicate inserts
- Replace hardcoded hex colors with --color-warning design tokens

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented 2026-06-14 02:00:18 +00:00

Author

Owner

Copy link

PR #208 Review

DOMAIN REVIEW

Stack identified: Ruby on Rails 8.1, ActiveRecord, ActiveStorage, Anthropic Ruby SDK, ERB views, vanilla CSS, RSpec.

Ruby / Rails quality:

1. Service object architecture -- ScheduleDigester is well-structured: single responsibility, dependency-injectable client for testing, clear public API. Good separation between API call, parsing, matching, and item creation.

2. Levenshtein implementation -- Correct O(m*n) dynamic programming. Threshold of 3 is reasonable for landscaping client names (short strings). No performance concern since the property list is small (loaded with .to_a).

3. Controller -- digest and confirm_digest follow the existing controller patterns (find-by-id, redirect with flash). Error rescue for ApiError and ParseError is solid. The parse_week_start fallback to current Monday on Date::Error is defensive and correct.

4. Duplicate handling -- Double-defense: find_by check + rescue ActiveRecord::RecordNotUnique. This is correct given that the WorkQueueItem model validates uniqueness: { scope: :work_date } on property_id.

5. Migration -- Simple add_column :uploads, :week_start, :date. Non-breaking, nullable. Good.

6. Model additions -- Upload#vision_compatible? and VISION_MEDIA_TYPES are clean. Note that image/gif is in VISION_MEDIA_TYPES but not in ALLOWED_CONTENT_TYPES -- this means vision_compatible? will never return true for GIFs since they can't be uploaded. Not a bug (the check is correct), but GIF in the VISION list is dead code.

7. CSS -- Uses existing design tokens (--spacing-*, --color-*, --radius). No magic numbers. New --color-warning and --color-warning-light CSS custom properties added to the root -- these are good additions to the design token system.

Security review:

8. API key handling -- ENV.fetch("ANTHROPIC_API_KEY") with custom error. Key never logged or exposed in views. .env.example has placeholder only. Good.

9. Input validation -- build_day_assignments validates input type (ActionController::Parameters or Hash), filters to known day names via ScheduleDigester::DAYS.each, converts IDs with .to_i.reject(&:zero?). This prevents injection of arbitrary keys or non-integer IDs.

10. Authorization -- The controller inherits require_role :lead, :admin, :super_admin from line 2 of UploadsController. The new digest and confirm_digest actions are properly covered by this class-level guard. Good.

Test coverage:

11. Unit tests (schedule_digester_spec.rb, 24 examples) -- Covers: Levenshtein edge cases, exact/fuzzy/no match, date math, WorkQueueItem creation, duplicate skipping, position assignment, response parsing (valid JSON, markdown-fenced JSON, invalid JSON, non-object JSON, partial days), API error paths.

12. Request specs (uploads_spec.rb, 18 examples) -- Covers: digest success, API failure redirect, parse failure redirect, week_start persistence, default week_start, confirm_digest creation, duplicate skipping, empty assignment redirect, missing assignments param.

13. Mock strategy -- Uses instance_double for the service and double for the Anthropic client chain. The client is injected via constructor, avoiding env-dependent tests. Clean.

BLOCKERS

None found. The code is well-structured, properly authorized, has strong test coverage (42 new specs), handles errors defensively, and follows existing patterns.

NITS

1. Dead media type (non-blocking) -- VISION_MEDIA_TYPES includes image/gif but ALLOWED_CONTENT_TYPES does not include GIF, so vision_compatible? will never return true for a GIF upload. Consider removing image/gif from VISION_MEDIA_TYPES to avoid confusion, or adding a comment explaining it's there for future-proofing.

2. Model constant in controller (non-blocking) -- ScheduleDigester::DAYS is referenced directly in UploadsController#build_day_assignments. This is fine for now but creates a coupling. If the service were ever extracted to a gem, the controller would need updating. Minor.

3. Radio button override UX (non-blocking) -- The digest.html.erb view shows radio buttons for alternative fuzzy match candidates (override_<day>_<idx>), but those radio button values are never processed in confirm_digest. The form only reads assignments[<day>][] from the checkboxes. If a user selects an alternative candidate via radio, it has no effect. This is a UX gap -- the radio buttons suggest a choice that isn't wired up.

4. schema.rb includes daily_notes table (non-blocking) -- The diff to db/schema.rb adds a daily_notes table that isn't part of this PR's migration. This appears to come from another branch's migration being present locally. The PR's own migration (add_week_start_to_uploads) is correct, but the schema.rb diff is noisy.

SOP COMPLIANCE

• Branch named after issue (204-schedule-digest references #204)
• PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related)
• Closes #204 present in PR body
• Related Notes section says "None" -- no plan slug referenced. Acceptable if no plan exists, but worth noting.
• Tests exist and pass (342 examples, 0 failures per PR body)
• No secrets committed (.env.example has placeholder only)
• No unrelated file changes (the schema.rb noise from daily_notes is a schema rebuild artifact, not scope creep)
• Commit messages appear descriptive (single commit inferred from PR title)

PROCESS OBSERVATIONS

• Change failure risk: LOW -- New feature, no modifications to existing core models. WorkQueueItem creation uses existing model validations. API failures are gracefully caught.
• Rollback safety: HIGH -- The only schema change is a nullable week_start column. Removing the feature would just mean dead column, no data loss.
• Documentation gap -- No docs added for the new feature workflow. A brief section in a usage guide or README about "How to digest a schedule" would help onboarding. Non-blocking.

VERDICT: APPROVED

## PR #208 Review ### DOMAIN REVIEW **Stack identified:** Ruby on Rails 8.1, ActiveRecord, ActiveStorage, Anthropic Ruby SDK, ERB views, vanilla CSS, RSpec. **Ruby / Rails quality:** 1. **Service object architecture** -- `ScheduleDigester` is well-structured: single responsibility, dependency-injectable client for testing, clear public API. Good separation between API call, parsing, matching, and item creation. 2. **Levenshtein implementation** -- Correct O(m*n) dynamic programming. Threshold of 3 is reasonable for landscaping client names (short strings). No performance concern since the property list is small (loaded with `.to_a`). 3. **Controller** -- `digest` and `confirm_digest` follow the existing controller patterns (find-by-id, redirect with flash). Error rescue for `ApiError` and `ParseError` is solid. The `parse_week_start` fallback to current Monday on `Date::Error` is defensive and correct. 4. **Duplicate handling** -- Double-defense: `find_by` check + `rescue ActiveRecord::RecordNotUnique`. This is correct given that the `WorkQueueItem` model validates `uniqueness: { scope: :work_date }` on `property_id`. 5. **Migration** -- Simple `add_column :uploads, :week_start, :date`. Non-breaking, nullable. Good. 6. **Model additions** -- `Upload#vision_compatible?` and `VISION_MEDIA_TYPES` are clean. Note that `image/gif` is in `VISION_MEDIA_TYPES` but not in `ALLOWED_CONTENT_TYPES` -- this means `vision_compatible?` will never return true for GIFs since they can't be uploaded. Not a bug (the check is correct), but GIF in the VISION list is dead code. 7. **CSS** -- Uses existing design tokens (`--spacing-*`, `--color-*`, `--radius`). No magic numbers. New `--color-warning` and `--color-warning-light` CSS custom properties added to the root -- these are good additions to the design token system. **Security review:** 8. **API key handling** -- `ENV.fetch("ANTHROPIC_API_KEY")` with custom error. Key never logged or exposed in views. `.env.example` has placeholder only. Good. 9. **Input validation** -- `build_day_assignments` validates input type (`ActionController::Parameters` or `Hash`), filters to known day names via `ScheduleDigester::DAYS.each`, converts IDs with `.to_i.reject(&:zero?)`. This prevents injection of arbitrary keys or non-integer IDs. 10. **Authorization** -- The controller inherits `require_role :lead, :admin, :super_admin` from line 2 of `UploadsController`. The new `digest` and `confirm_digest` actions are properly covered by this class-level guard. Good. **Test coverage:** 11. **Unit tests** (`schedule_digester_spec.rb`, 24 examples) -- Covers: Levenshtein edge cases, exact/fuzzy/no match, date math, WorkQueueItem creation, duplicate skipping, position assignment, response parsing (valid JSON, markdown-fenced JSON, invalid JSON, non-object JSON, partial days), API error paths. 12. **Request specs** (`uploads_spec.rb`, 18 examples) -- Covers: digest success, API failure redirect, parse failure redirect, week_start persistence, default week_start, confirm_digest creation, duplicate skipping, empty assignment redirect, missing assignments param. 13. **Mock strategy** -- Uses `instance_double` for the service and `double` for the Anthropic client chain. The client is injected via constructor, avoiding env-dependent tests. Clean. ### BLOCKERS None found. The code is well-structured, properly authorized, has strong test coverage (42 new specs), handles errors defensively, and follows existing patterns. ### NITS 1. **Dead media type** (non-blocking) -- `VISION_MEDIA_TYPES` includes `image/gif` but `ALLOWED_CONTENT_TYPES` does not include GIF, so `vision_compatible?` will never return true for a GIF upload. Consider removing `image/gif` from `VISION_MEDIA_TYPES` to avoid confusion, or adding a comment explaining it's there for future-proofing. 2. **Model constant in controller** (non-blocking) -- `ScheduleDigester::DAYS` is referenced directly in `UploadsController#build_day_assignments`. This is fine for now but creates a coupling. If the service were ever extracted to a gem, the controller would need updating. Minor. 3. **Radio button override UX** (non-blocking) -- The `digest.html.erb` view shows radio buttons for alternative fuzzy match candidates (`override_<day>_<idx>`), but those radio button values are never processed in `confirm_digest`. The form only reads `assignments[<day>][]` from the checkboxes. If a user selects an alternative candidate via radio, it has no effect. This is a UX gap -- the radio buttons suggest a choice that isn't wired up. 4. **`schema.rb` includes `daily_notes` table** (non-blocking) -- The diff to `db/schema.rb` adds a `daily_notes` table that isn't part of this PR's migration. This appears to come from another branch's migration being present locally. The PR's own migration (`add_week_start_to_uploads`) is correct, but the schema.rb diff is noisy. ### SOP COMPLIANCE - [x] Branch named after issue (`204-schedule-digest` references #204) - [x] PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related) - [x] `Closes #204` present in PR body - [ ] Related Notes section says "None" -- no plan slug referenced. Acceptable if no plan exists, but worth noting. - [x] Tests exist and pass (342 examples, 0 failures per PR body) - [x] No secrets committed (`.env.example` has placeholder only) - [x] No unrelated file changes (the `schema.rb` noise from `daily_notes` is a schema rebuild artifact, not scope creep) - [x] Commit messages appear descriptive (single commit inferred from PR title) ### PROCESS OBSERVATIONS - **Change failure risk: LOW** -- New feature, no modifications to existing core models. WorkQueueItem creation uses existing model validations. API failures are gracefully caught. - **Rollback safety: HIGH** -- The only schema change is a nullable `week_start` column. Removing the feature would just mean dead column, no data loss. - **Documentation gap** -- No docs added for the new feature workflow. A brief section in a usage guide or README about "How to digest a schedule" would help onboarding. Non-blocking. ### VERDICT: APPROVED

ldraney added 1 commit 2026-06-14 02:07:30 +00:00

Merge branch 'main' into 204-schedule-digest b4ac9db150

ldraney force-pushed 204-schedule-digest from b4ac9db150 to cac79c3b67 2026-06-14 02:31:05 +00:00 Compare

ldraney merged commit 058fa49cfa into main 2026-06-14 02:31:15 +00:00

ldraney deleted branch 204-schedule-digest 2026-06-14 02:31:15 +00:00

ldraney referenced this pull request from a commit 2026-06-14 02:31:16 +00:00

Add schedule image digestion via Claude Vision API (#208)

ldraney referenced this pull request 2026-06-14 02:37:49 +00:00

Add assign-to-crew and add-to-queue buttons on Recent and Assigned sections #211

ldraney referenced this pull request 2026-06-14 02:50:05 +00:00

Schedule digest: feature flag gate + Week tab upload button #212

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

ldraney/landscaping-assistant!208

No description provided.