From f27f85e1830feb88fd8d23335dea329865760592 Mon Sep 17 00:00:00 2001 From: Soumya Ray Date: Wed, 24 Dec 2025 17:16:31 +0800 Subject: [PATCH 1/6] chore: update version management of claude code files --- .claude/active/CLAUDE.feature-smartcache.md | 423 -------------------- .gitignore | 2 +- .claude/CLAUDE.md => CLAUDE.md | 0 3 files changed, 1 insertion(+), 424 deletions(-) delete mode 100644 .claude/active/CLAUDE.feature-smartcache.md rename .claude/CLAUDE.md => CLAUDE.md (100%) diff --git a/.claude/active/CLAUDE.feature-smartcache.md b/.claude/active/CLAUDE.feature-smartcache.md deleted file mode 100644 index c2608a3..0000000 --- a/.claude/active/CLAUDE.feature-smartcache.md +++ /dev/null @@ -1,423 +0,0 @@ -# Feature Plan: Smart Cache - Project-Level Caching with Subfolder Extraction - -## Overview - -Cache entire project appraisals once, then extract subfolder contributions from cache on subsequent requests without calling the worker again. - -## Current Architecture - -``` -Client → API → Check Redis cache (folder-specific key) - ↓ (hit) - Return cached JSON - ↓ (miss) - Send AppraisalRequest to SQS → Return 202 Processing - ↓ - Worker receives request - ↓ - Clone if needed - ↓ - Appraise FOLDER only - ↓ - Store JSON in Redis (folder key) - ↓ - Notify via Faye -``` - -**Current cache key format**: `appraisal:{owner}/{project}/{folder_path}` - -**Problem**: Each subfolder requires a separate worker appraisal, even though: -- The worker clones the entire repo anyway -- Git blame runs on all files during appraisal -- Subfolder data is a subset of root appraisal data - -## Proposed Architecture - -``` -Client → API → Check Redis cache (project root key) - ↓ (hit) - Extract subfolder from cached root → Return JSON - ↓ (miss) - Send AppraisalRequest to SQS (always root) → Return 202 Processing - ↓ - Worker receives request - ↓ - Clone if needed - ↓ - Appraise ROOT folder - ↓ - Store JSON in Redis (root key only) - ↓ - Notify via Faye -``` - -**New cache key format**: `appraisal:{owner}/{project}/` (root only) - -**Benefits**: -- Single worker request caches all subfolder data -- Subsequent subfolder requests are cache hits -- Reduced SQS messages, worker load, and latency - -## Key Changes - -### 1. Worker Changes - -- **Modify**: Always appraise root folder (ignore folder_path in request) -- **Modify**: Cache key always uses root path (empty string) -- **Keep**: Progress reporting unchanged - -### 2. Web API Changes - -- **Add**: Subfolder extraction logic from cached root appraisal -- **Modify**: Cache lookup uses root key, then extracts subfolder -- **Modify**: AppraisalRequest always requests root (folder_path = "") -- **Add**: New domain/infrastructure for subfolder extraction - -### 3. Cache Strategy - -- **Single key per project**: `appraisal:{owner}/{project}/` -- **Value**: Full project `FolderContributions` JSON (root) -- **TTL**: 1 day (unchanged) -- **Extraction**: API extracts subfolder from cached root on demand - -## Discussion Points - -### Feasibility Assessment - -**Pros:** - -1. ⚠️ **Reduced worker calls**: One appraisal serves all subfolder requests -2. ⚠️ **Lower latency**: Subfolders served from cache after first request -3. ⚠️ **Simpler cache model**: One key per project vs many keys per folder -4. ⚠️ **Less SQS traffic**: Fewer worker requests - -**Concerns to Address:** - -1. ~~⚠️ **Cache size**: Root appraisal JSON larger than subfolder-only~~ **RESOLVED: ~30KB is practical** - -2. ~~⚠️ **Extraction complexity**: How to extract subfolder from root?~~ **RESOLVED: Navigate tree via Roar deserialization** - -3. ~~⚠️ **First request latency**: Root appraisal takes longer than subfolder~~ **ACCEPTED: Same as current (full clone happens anyway)** - -4. ⚠️ **Subfolder not found**: What if requested subfolder doesn't exist? - - Return nil from extraction, service returns 404 - -5. ~~⚠️ **Representer changes**: How to serialize extracted subfolder?~~ **RESOLVED: Re-serialize OpenStruct subtree via representer** - -## Implementation Phases - -### Phase 1: Analysis & Design ✅ - -- [x] Analyze `FolderContributions` structure for subfolder extraction -- [x] Measure typical root appraisal JSON sizes (~30KB practical) -- [x] Design subfolder extraction algorithm (tree traversal via Roar deserialization) -- [x] Decide on location for extraction logic (Representer) -- [x] Document decisions in this file - -### Phase 2: Refactor - Create `Request::Appraisal` and `Messaging::AppraisalJob` ✅ - -**Replace `Request::ProjectPath` with `Request::Appraisal`:** - -- [x] Create `Request::Appraisal` in `app/application/requests/` - - Subsumes `ProjectPath` functionality (owner_name, project_name, folder_name) - - Add `project_fullname` method - - Add `cache_key` method (single source of truth) - - Add `root_request?` helper -- [x] Update controller to use `Request::Appraisal` instead of `ProjectPath` -- [x] Update `Service::FetchOrRequestAppraisal` to use `request.cache_key` -- [x] Remove duplicate `appraisal_cache_key` helper from service -- [x] Delete old `Request::ProjectPath` -- [x] Add unit tests for `Request::Appraisal` - -**Consolidate worker job DTO:** - -- [x] Create `Messaging::AppraisalJob` in `app/infrastructure/messaging/` -- [x] Create `Representer::AppraisalJob` (replaces `AppraisalRequest`) -- [x] Update service to use `Messaging::AppraisalJob` -- [x] Update worker `JobReporter` to use new naming -- [x] Delete legacy `Response::CloneRequest` and `Response::AppraisalRequest` -- [x] Delete legacy `Representer::CloneRequest` and `Representer::AppraisalRequest` -- [x] Update tests and documentation -- [x] Verify all existing tests pass (88 tests, 0 failures) - -### Phase 3: Representer Subfolder Extraction ✅ - -**Add extraction methods to `Representer::FolderContributions`:** - -- [x] Add `find_subfolder(root_ostruct, folder_path)` - tree traversal helper -- [x] Add `extract_subfolder(json_string, folder_path)` - returns OpenStruct or nil -- [x] Add `extract_subfolder_json(json_string, folder_path)` - returns JSON string or nil -- [x] Handle edge cases: root path (empty string), path not found, trailing slashes -- [x] Unit tests for extraction methods (16 tests in `folder_contributions_extraction_spec.rb`) - -### Phase 4: Smart Cache Key and Worker Request ✅ - -**API owns smart cache strategy; worker remains general-purpose:** - -- [x] Update `Request::Appraisal#cache_key` for root-only behavior - - `cache_key` now always returns root key: `appraisal:{owner}/{project}/` - - `folder_name` still available for subfolder extraction -- [x] Update `FetchOrRequestAppraisal` to always send root to worker - - Added `ROOT_FOLDER_PATH = ''` constant in API service - - Worker receives root path, appraises faithfully -- [x] Worker unchanged - remains general-purpose (appraises whatever folder it's told) -- [x] Update unit tests for smart cache behavior - -### Phase 5: API Service Subfolder Extraction ✅ - -**Modify `FetchOrRequestAppraisal` to extract subfolders from cache:** - -- [x] On cache hit: extract requested subfolder from cached root JSON - - Added `extract_subfolder` and `rebuild_appraisal_json` helper methods - - Root requests return cached JSON unchanged - - Subfolder requests extract and rebuild JSON with correct `folder_path` -- [x] Handle subfolder-not-found (return 404) - - Returns `NO_FOLDER_ERR` with `:not_found` status -- [x] Update service unit/integration tests - - Added 2 integration tests: subfolder extraction (happy) and subfolder not found (sad) - -### Phase 6: Cleanup & Testing ✅ - -- [x] End-to-end acceptance tests for smart cache flow -- [x] Test scenarios: root request, subfolder request, nested subfolder, invalid path -- [x] Verify old folder-specific cache keys are ignored (TTL expiration) -- [x] Update `.claude/CLAUDE.md` documentation with new architecture -- [x] Update session log with completion status - -## Session Log - -### Session 1 - -- Initial discussion of smart cache feature -- Updated CLAUDE.md to reflect current project state -- Created this planning document -- Answered all 6 Open Questions: - - Q1: FolderContributions has nested tree structure; JSON preserves it via recursive representer - - Q2: Measured JSON sizes (~30KB for root project); root-only caching is practical - - Q3: Extraction logic in Representer (presentation layer) - keeps entity worker-only - - Q4: Tree traversal is O(depth); Roar `from_json` handles nested deserialization automatically - - Q5: Cache invalidation unchanged; smart cache simplifies (one key per project) - - Q6: Create `Request::Appraisal` subsuming `ProjectPath` with cache key methods -- Made 5 decisions (see Decisions Made section) -- Refined Implementation Phases (6 phases) with Phase 2 as refactor -- Committed: `8c020a3` - docs: complete Phase 1 planning for smart cache feature -- **Status**: Phase 1 COMPLETE - -### Session 2 - -- Implemented Phase 2 refactoring: - - Created `Request::Appraisal` (replaces `Request::ProjectPath`) - - Created `Messaging::AppraisalJob` DTO (replaces `Response::AppraisalRequest`) - - Created `Representer::AppraisalJob` (replaces `Representer::AppraisalRequest`) - - Updated controller, service, and worker to use new objects - - Deleted legacy code: `CloneRequest`, `AppraisalRequest` (response/representer) - - Updated CLAUDE.md documentation - - All 88 tests passing -- Restructured test tasks: - - `rake spec` now runs only unit + integration tests (77 tests, no worker required) - - `rake spec:all` runs all tests including acceptance (88 tests, requires worker) - - `bash spec/acceptance_tests` starts worker and calls `spec:all` - - Updated CLAUDE.md testing documentation -- Committed: `cfac067` - tests: restructure test tasks for worker-independent development -- **Status**: Phase 2 COMPLETE - -### Session 3 - -- Implemented Phase 3 subfolder extraction: - - Added `extract_subfolder(json_string, folder_path)` class method to `Representer::FolderContributions` - - Added `extract_subfolder_json(json_string, folder_path)` for JSON string output - - Added private `find_subfolder` tree traversal helper - - Handles edge cases: root path, nil, trailing/leading slashes, non-existent folders, error appraisals - - Created JSON fixtures in `spec/fixtures/json/` (not wiped by `vcr:wipe`) - - Added 16 unit tests in `folder_contributions_extraction_spec.rb` - - All 93 tests passing (77 original + 16 new) -- Implemented Phase 4 smart cache (API-owned strategy): - - `Request::Appraisal#cache_key` now always returns root key - - `FetchOrRequestAppraisal` always sends root folder_path to worker - - Worker unchanged - remains general-purpose (appraises whatever it's told) - - Design: API owns smart cache strategy; worker stays simple - - Updated unit tests for smart cache behavior - - All 93 tests passing -- **Status**: Phase 4 COMPLETE; ready for Phase 5 - -### Session 4 - -- Implemented Phase 5 API service subfolder extraction: - - Added `extract_subfolder(cached_json, folder_name)` helper to `FetchOrRequestAppraisal` - - Added `rebuild_appraisal_json` helper for JSON reconstruction - - Root requests return cached JSON unchanged - - Subfolder requests extract, rebuild JSON with updated `folder_path` and `folder` - - Returns 404 when subfolder not found in cached root - - Design decision: Keep extraction as helper method (not separate step) - it's an implementation detail of cache lookup -- Added 2 integration tests: - - HAPPY: extract subfolder from cached root on cache hit - - SAD: return 404 when subfolder not found in cache -- All 95 tests passing (93 original + 2 new integration) -- **Status**: Phase 5 COMPLETE; ready for Phase 6 - -### Session 5 - -- Refactored `FetchOrRequestAppraisal` to separate caching from folder extraction: - - Renamed `check_cache` → `check_project_appraisal_cache` - - Created separate step `extract_folder_from_appraisal_on_cache_hit` - - Renamed `request_appraisal_worker` → `request_appraisal_worker_on_cache_miss` - - Key insight: "folder not found" is a bad request (404), not a cache miss -- Moved `rebuild_appraisal_json` to `Representer::Appraisal.rebuild_with_extracted_folder` - - Fixed `::JSON` namespace issue (avoids conflict with `Representable::JSON`) -- Implemented Phase 6 acceptance tests: - - Added smart cache test: extract multiple subfolders from cached root - - Added invalid subfolder test: returns 404 after root is cached - - Used actual YPBT-app folders (spec, models, services) -- Verified JSON fixtures in `spec/fixtures/json/` are correct and in use: - - `sample_appraisal.json`: used in 3 integration tests + 13 unit tests - - `error_appraisal.json`: used in 1 unit test (error handling) -- Updated CLAUDE.md documentation with smart cache architecture: - - Updated Worker-Based Appraisal Architecture diagram - - Updated cache key format documentation (root-only) - - Updated FetchOrRequestAppraisal step names - - Updated data flow example for viewing contributions -- All 107 tests passing -- **Status**: Phase 6 COMPLETE; Smart Cache feature COMPLETE - ---- - -## Open Questions - -1. ~~How is `FolderContributions` structured? Can subfolders be extracted?~~ **ANSWERED: See Decision 1** -2. ~~What is the typical JSON size for root vs subfolder appraisals?~~ **ANSWERED: ~30KB root; practical for Redis** -3. ~~Should extraction happen in domain layer or infrastructure?~~ **ANSWERED: See Decision 2** -4. ~~How to handle deep nested paths efficiently?~~ **ANSWERED: See Decision 3** -5. ~~Cache invalidation strategy unchanged?~~ **ANSWERED: See Decision 4** -6. ~~Should cache-key generation be consolidated with request parsing?~~ **ANSWERED: See Decision 5** - ---- - -## Decisions Made - -### Decision 1: Navigate Cached Tree for Subfolder Extraction - -- JSON preserves nested `FolderContributions` structure via recursive representer -- Subfolder extraction will **navigate the deserialized tree** (not filter/reconstruct) -- **Rationale**: Leverages existing JSON structure; no reconstruction needed - -### Decision 2: Extraction Logic in Presentation Layer (Representer) - -- Extraction logic will reside in `Representer::FolderContributions` as class methods -- Tentative interface: - - `extract_subfolder(json_string, folder_path)` → OpenStruct or nil - - `extract_subfolder_json(json_string, folder_path)` → JSON string or nil -- **Rationale**: - - Representer already knows the JSON shape - - This is representation logic, not business logic - - Keeps service layer thin - - Avoids needing `FolderContributions` entity in API (stays worker-only) -- Alternative considered: Domain entity - rejected because (a) would require API to have the entity, (b) extraction is representation concern not business logic -- **⚠️ Concern noted**: Representer-based traversal may be brittle to changes in the entity structure. If `FolderContributions` entity changes how it organizes subfolders or paths, the representer extraction logic could break. If this becomes an issue, reconsider having the entity own the traversal logic (would require sharing entity between API and worker). - -### Decision 3: Roar Deserialization is Sufficient - -- Roar's `from_json` already handles nested deserialization automatically -- Deserializes to `OpenStruct` tree (not domain entities) - sufficient for traversal -- No mapper needed (JSON → Entity conversion would be extra work with no benefit) -- API only needs to pass through JSON to client; OpenStruct sufficient for subfolder lookup -- **Verified**: Tested with cached `YPBT-app` project - full nested tree deserializes correctly - -### Decision 4: Cache Invalidation Strategy Unchanged - -- TTL-based expiration remains: 1 day for success, 10 seconds for errors -- Manual wipe via `rake cache:wipe` unchanged -- Smart cache **simplifies** invalidation: one key per project instead of many -- No automatic invalidation on repo updates (deferred to future feature) -- **Future considerations** (out of scope for this feature): - - Webhook-based invalidation on GitHub push - - Manual wipe + re-appraisal workflow - -### Decision 5: Create `Request::Appraisal` Subsuming `ProjectPath` - -- New `Request::Appraisal` object replaces `Request::ProjectPath` -- **Naming**: `Request::Appraisal` (not `Request::AppraisalRequest` - avoids redundancy) - - Clean pairing: `Request::Appraisal` (what client asks for) vs `Value::Appraisal` (what worker produces) -- Owns cache key generation as single source of truth -- Provides: - - `owner_name`, `project_name`, `folder_name` (from ProjectPath) - - `project_fullname` method - - `cache_key` method (currently folder-specific, will become root-only) - - `root_request?` helper -- **Rationale**: - - Semantic clarity: Request (what is asked for) vs Appraisal (result of work) - - Removes duplicate cache key logic from service - - Centralizes "what is being requested" concept - - Stays in application layer where request objects belong -- **Implementation**: Completed in Phase 2 -- **Note**: `Value::Appraisal#cache_key` remains for worker use (result caching) - -### Decision 6: Create `Messaging::AppraisalJob` for Worker Queue - -- New `Messaging::AppraisalJob` DTO replaces `Response::AppraisalRequest` -- **Naming**: "Job" clarifies it's a work payload, not a response -- **Location**: `app/infrastructure/messaging/` (alongside Queue) -- **Pattern**: Data Transfer Object (DTO) for SQS transport -- Serialized via `Representer::AppraisalJob` -- **Cleanup**: Deleted legacy `CloneRequest` and associated representer - ---- - -## Quick Reference - -### Current Cache Key Format - -``` -appraisal:{owner}/{project}/{folder_path} -``` - -Examples: -- Root: `appraisal:ISS-SOA/codepraise-api/` -- Subfolder: `appraisal:ISS-SOA/codepraise-api/app/domain/` - -### Proposed Cache Key Format - -``` -appraisal:{owner}/{project}/ -``` - -Examples: -- All requests use: `appraisal:ISS-SOA/codepraise-api/` - -### Key Files to Modify - -**Worker:** - -- `workers/application/services/appraise_project.rb` -- `app/domain/contributions/values/appraisal.rb` - -**API:** - -- `app/application/services/fetch_or_request_appraisal.rb` -- New: subfolder extraction logic (location TBD) - -**Tests:** - -- `spec/tests/unit/appraisal_spec.rb` -- `spec/tests/integration/services_spec.rb` - ---- - -## Notes - -### Commit Practices - -- **Summarize changes before requesting commit permission** - provide brief summary by folder/file for user review BEFORE asking to commit -- **Separate concerns into distinct commits** - e.g., test setup changes vs feature changes -- **User is author, Claude is co-author** - use `Co-Authored-By: Claude ` -- **Use conventional commit messages** - `feat:`, `fix:`, `refactor:`, `docs:`, etc. - - `feat:` only for changes to external API/service features - - `refactor:` for internal changes (new domain objects, infrastructure, etc.) - -### Implementation Practices - -- **Seek consent before moving to next phase** - summarize completed work, commit, then ask to proceed -- **Include coverage report in commits** - after successful tests, amend `coverage/.resultset.json` to coding commits - -### Planning Practices - -- **IMPORTANT: Pause after each question** - wait for explicit user approval before proceeding to the next question during planning discussions diff --git a/.gitignore b/.gitignore index 75df40c..6b8de6c 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,6 @@ .claude/_* .claude/settings.local.json -CLAUDE.local.md +CLAUDE.*.md _snippets/ .bundle/ config/secrets.yml diff --git a/.claude/CLAUDE.md b/CLAUDE.md similarity index 100% rename from .claude/CLAUDE.md rename to CLAUDE.md From e60e714f1cd2d1e4c011e00ebb1410faf8aca5d8 Mon Sep 17 00:00:00 2001 From: Soumya Ray Date: Wed, 24 Dec 2025 23:48:16 +0800 Subject: [PATCH 2/6] docs: add planning document for progress reporting refactoring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Simplify .gitignore to only ignore CLAUDE.local.md (allows tracking branch-specific planning docs) - Add CLAUDE.refactor-progress-reporting.md with refactoring plan: - Rename ProgressPublisher → FayeServer - Create ProgressMapper for symbol → percentage mapping - Create CloneMapper for git output parsing - Eliminate JobReporter (merge into ProgressMapper + controller) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- .gitignore | 2 +- CLAUDE.refactor-progress-reporting.md | 306 ++++++++++++++++++++++++++ 2 files changed, 307 insertions(+), 1 deletion(-) create mode 100644 CLAUDE.refactor-progress-reporting.md diff --git a/.gitignore b/.gitignore index 6b8de6c..75df40c 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,6 @@ .claude/_* .claude/settings.local.json -CLAUDE.*.md +CLAUDE.local.md _snippets/ .bundle/ config/secrets.yml diff --git a/CLAUDE.refactor-progress-reporting.md b/CLAUDE.refactor-progress-reporting.md new file mode 100644 index 0000000..ffe41fb --- /dev/null +++ b/CLAUDE.refactor-progress-reporting.md @@ -0,0 +1,306 @@ +# Refactoring Plan: Progress Reporting Architecture (refactor-progress-reporting branch) + +## Overview + +Refactor the worker's progress reporting to use symbolic messages instead of numeric percentages, with a dedicated mapper translating symbols to percentages for Faye publication. + +## Current Architecture + +``` +Service::AppraiseProject + ↓ (numeric percentages: 15, 25, 50, 85, 100) +JobReporter.progress_callback + ↓ (pass-through) +ProgressPublisher.publish + ↓ (HTTP POST) +Faye Server → Client +``` + +**Current Issues:** + +1. `Service::AppraiseProject` is tightly coupled to percentage values (15, 25, 50, etc.) +2. `JobReporter` is just a thin wrapper - doesn't add value beyond parsing job JSON +3. Percentage mapping logic is duplicated: + - `AppraisalMonitor::PHASES` hash in `presentation/values/progress_monitor.rb` + - Inline values in `Service::AppraiseProject` (15, 25, 40, 45, 50, 55, 85, 90, 100) + - `scale_clone_progress` method also has its own mapping +4. Progress stages are not clearly named - just raw numbers +5. `CloneMonitor` is marked as "legacy" but still exists + +## Proposed Architecture + +```text +Worker Controller + ↓ (deserializes job JSON, creates ProgressMapper) +Service::AppraiseProject + ↓ (symbolic progress: :started, :clone_receiving, :appraising, etc.) + ↓ (git output lines go through CloneMapper first) +ProgressMapper (in workers/infrastructure/faye/) + ↓ (maps symbols → percentages) +FayeServer (in workers/infrastructure/faye/) + ↓ (HTTP POST) +Faye Server → Client +``` + +**Key Changes:** + +1. **Service sends symbols**: `progress.call(:started)` instead of `progress.call(15)` +2. **CloneMapper parses git output**: Converts git clone lines → symbols (`:clone_receiving`, etc.) +3. **ProgressMapper is single source of truth**: All symbol → percentage mappings in one place +4. **FayeServer is pure infrastructure**: Just HTTP POST, no business logic +5. **JobReporter eliminated**: Responsibilities split between controller (parsing) and ProgressMapper (reporting) + +## Discussion Points + +### Benefits + +1. **Service is domain-focused**: Uses meaningful symbols like `:cloning`, `:appraising` +2. **Single source of truth**: All percentage mappings in one place (ProgressMapper) +3. **Testability**: Service tests don't need to know about percentages +4. **Flexibility**: Easy to change percentage distribution without touching service +5. **Clean architecture**: Proper layer separation (domain → presentation → infrastructure) + +### Layer Placement + +**Option A: Mapper in Presentation Layer** +- `workers/presentation/mappers/progress_mapper.rb` +- Aligns with current `progress_monitor.rb` location in presentation +- Translation from domain symbols to external format is a presentation concern + +**Option B: Mapper in Application Layer** +- `workers/application/requests/progress_mapper.rb` +- Keeps request handling together (JobReporter was a request object) +- Application layer coordinates between domain and infrastructure + +**Recommendation**: Option A - Presentation layer, because: +- Mapping domain concepts to external representation IS a presentation concern +- Similar to how Representers map entities to JSON +- Infrastructure (gateway) stays pure HTTP + +### Symbol Design + +**Current numeric stages in Service::AppraiseProject:** +```ruby +clone_repo: + 15 → STARTED + 50 → Skip clone (already exists) + 25, 35, 40, 45, 50 → Clone progress stages + +appraise_contributions: + 55 → Starting appraisal + 85 → Appraisal complete + +cache_result: + 90 → Caching + 100 → FINISHED +``` + +**Proposed symbols:** +```ruby +# Main phases +:started # 15 - Worker began processing +:clone_started # 20 - Beginning clone +:clone_receiving # 40 - Receiving objects from remote +:clone_resolving # 45 - Resolving deltas +:clone_done # 50 - Clone complete (or skipped) +:appraise_started # 55 - Beginning git blame analysis +:appraise_done # 85 - Analysis complete +:cache_started # 90 - Storing in Redis +:finished # 100 - All done +``` + +### Git Clone Progress Handling + +**Current approach**: `scale_clone_progress(line)` parses git output and maps keywords: +```ruby +{ 'Cloning' => 25, 'remote' => 35, 'Receiving' => 40, 'Resolving' => 45, 'Checking' => 50 } +``` + +**Options:** + +A. **Keep in Service**: Service parses git output, sends fine-grained symbols +B. **Move to Mapper**: Service sends `:clone_progress` with raw line, mapper parses +C. **Simplify**: Only report major phases, skip line-by-line git progress + +**Recommendation**: Option C - Simplify. The line-by-line progress adds complexity without significant UX benefit. Report: `:clone_started`, `:clone_done`. + +## Implementation Phases + +### Phase 1: Restructure Faye Infrastructure + +**Rename folder**: `workers/infrastructure/messaging/` → `workers/infrastructure/faye/` + +**Rename file**: `progress_publisher.rb` → `faye_server.rb` + +- [ ] Rename class `ProgressPublisher` → `FayeServer` +- [ ] Keep same interface: `publish(message)` +- [ ] This is pure infrastructure - no logic changes + +**New file**: `workers/infrastructure/faye/progress_mapper.rb` + +- [ ] Create `Appraiser::ProgressMapper` class +- [ ] Define `PHASES` hash (single source of truth for all symbol → percentage mappings) +- [ ] Include all phases: `:started`, `:clone_receiving`, `:clone_resolving`, `:clone_done`, `:appraising`, `:appraise_done`, `:caching`, `:finished` +- [ ] Implement `map(symbol) → percentage` method +- [ ] Accept FayeServer in constructor for dependency injection +- [ ] Implement `report(symbol)` that maps and publishes via FayeServer + +**New file**: `workers/infrastructure/git/mappers/clone_mapper.rb` + +- [ ] Create `CloneMapper` class (or module) +- [ ] Parse git clone output lines into symbols +- [ ] Map: `"Receiving..."` → `:clone_receiving`, `"Resolving..."` → `:clone_resolving`, etc. +- [ ] Return symbol that ProgressMapper can translate to percentage + +### Phase 2: Update Service to Use Symbols + +**Modify**: `workers/application/services/appraise_project.rb` + +- [ ] Change all `input[:progress].call(15)` → `input[:progress].call(:started)` +- [ ] Remove `scale_clone_progress` method +- [ ] Use CloneMapper to convert git output lines to symbols +- [ ] Use symbols throughout: `:started`, `:clone_receiving`, `:clone_done`, `:appraising`, etc. + +### Phase 3: Update Worker Controller + +**Modify**: `workers/application/controllers/worker.rb` + +- [ ] Move job JSON deserialization from JobReporter to controller +- [ ] Create ProgressMapper with FayeServer and channel_id +- [ ] Pass `progress_mapper.progress_callback` to service +- [ ] Update `report_each_second` logic to use ProgressMapper with `:finished` symbol + +### Phase 4: Cleanup + +- [ ] Delete `workers/application/requests/job_reporter.rb` +- [ ] Delete `workers/presentation/values/progress_monitor.rb` + - `CloneMonitor` module (marked as legacy) + - `AppraisalMonitor` module (replaced by ProgressMapper) +- [ ] Update `require_worker.rb` for new file locations +- [ ] Update tests for new symbol-based interface + +## Open Questions + +1. ~~Should we keep fine-grained git clone progress, or simplify to just start/done?~~ **DECIDED: Keep fine-grained via CloneMapper** +2. ~~JobReporter: combine with ProgressMapper, or keep separate?~~ **DECIDED: Eliminate JobReporter, merge reporting into ProgressMapper** +3. ~~Should ProgressMapper be in presentation or application layer?~~ **DECIDED: Infrastructure (with FayeServer)** +4. ~~Keep AppraisalMonitor::PHASES as the source of truth, or define in ProgressMapper?~~ **DECIDED: ProgressMapper is the single source of truth** + +All open questions resolved! + +--- + +## Decisions Made + +### Decision 1: Infrastructure Gateway Naming + +- Rename `ProgressPublisher` → `FayeServer` +- File: `workers/infrastructure/messaging/progress_publisher.rb` → `faye_server.rb` +- **Rationale**: "Server" better describes the role as an infrastructure gateway to the Faye server + +### Decision 2: Faye Infrastructure Organization + +- Rename folder: `workers/infrastructure/messaging/` → `workers/infrastructure/faye/` +- Place both `FayeServer` (gateway) and `ProgressMapper` (mapper) in this folder +- Structure: + + ```text + workers/infrastructure/faye/ + ├── faye_server.rb # Gateway - HTTP POST to Faye + └── progress_mapper.rb # Mapper - symbols → percentages + ``` + +- **Rationale**: Faye is infrastructure; the mapper translates domain symbols for this specific infrastructure concern. Keeping gateway + mapper together follows the pattern in `workers/infrastructure/git/` (gateway + mappers) + +### Decision 4: Eliminate JobReporter + +- Delete `workers/application/requests/job_reporter.rb` +- Move job JSON deserialization to Worker controller (or service's first step) +- Move `report()`, `report_each_second()`, `progress_callback()` to ProgressMapper +- **Rationale**: JobReporter has no distinct responsibility: + - Its reporting methods belong in ProgressMapper + - Its JSON deserialization is trivial (3 lines) and can live in controller/service + +**Before:** +```text +Worker.perform(request_json) + ↓ +JobReporter.new(request_json, config) # parses + creates publisher + ↓ +Service::AppraiseProject.call(progress: job.progress_callback) +``` + +**After:** +```text +Worker.perform(request_json) + ↓ +Deserialize job (in controller or service step) + ↓ +ProgressMapper.new(config, channel_id) # created with FayeServer + ↓ +Service::AppraiseProject.call(progress: mapper.progress_callback) +``` + +### Decision 3: Git Clone Progress with CloneMapper + +- Create `CloneMapper` to parse git clone output lines into symbols +- Location: `workers/infrastructure/git/mappers/clone_mapper.rb` (alongside other git mappers) +- `CloneMapper` converts git output → symbols (`:clone_receiving`, `:clone_resolving`, etc.) +- `ProgressMapper` remains the single source of truth for all symbol → percentage mappings +- Flow: + + ```text + Git clone output line (e.g., "Receiving objects: 50%...") + ↓ + CloneMapper.map(line) → :clone_receiving + ↓ + ProgressMapper.report(:clone_receiving) → 40% + ↓ + FayeServer.publish("40") + ``` + +- **Rationale**: Separates concerns - git parsing stays with git infrastructure, percentage mapping stays centralized in ProgressMapper + +--- + +## Session Log + +### Session 1 + +- Initial discussion of refactoring goals +- Created this planning document +- Reviewed current architecture: + - `workers/application/requests/job_reporter.rb` - thin wrapper around ProgressPublisher + - `workers/infrastructure/messaging/progress_publisher.rb` - HTTP POST to Faye + - `workers/application/services/appraise_project.rb` - uses numeric percentages + - `workers/presentation/values/progress_monitor.rb` - CloneMonitor (legacy) + AppraisalMonitor +- Identified issues: + - Duplicate percentage mappings in multiple places + - Service coupled to numeric values + - JobReporter adds little value +- **Status**: Initial planning, awaiting user feedback on proposed approach + +--- + +## Notes + +### Post-Implementation Cleanup + +After final implementation and testing: + +- [ ] Add `CLAUDE.refactor-progress-reporting.md` to `.gitignore` +- [ ] Move this file to `.claude/_archive/CLAUDE.refactor-progress-reporting.md` + +### Commit Practices + +- **Summarize changes before requesting commit permission** - provide brief summary by folder/file for user review BEFORE asking to commit +- **Separate concerns into distinct commits** - e.g., infrastructure rename vs service logic changes +- **User is author, Claude is co-author** - use `Co-Authored-By: Claude ` +- **Use conventional commit messages** - `feat:`, `fix:`, `refactor:`, `docs:`, etc. + - `refactor:` for internal changes (this refactoring) + - `feat:` only for changes to external API/service features + +### Implementation Practices + +- **Seek consent before moving to next phase** - summarize completed work, commit, then ask to proceed +- **Include coverage report in commits** - after successful tests, amend `coverage/.resultset.json` to coding commits From bf417d8f65e637571fcc6f40cec5a2b5623f5f64 Mon Sep 17 00:00:00 2001 From: Soumya Ray Date: Thu, 25 Dec 2025 00:26:02 +0800 Subject: [PATCH 3/6] refactor: restructure Faye infrastructure (Phase 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename workers/infrastructure/messaging/ → faye/ - Rename ProgressPublisher → FayeServer (pure HTTP gateway) - Add ProgressMapper for symbol → percentage mapping (single source of truth) - Add CloneMapper for git output → symbols parsing - Use -ing suffix for action phases: cloning_*, appraising_*, caching_* - Update require_worker.rb comment All tests pass: 95 runs, 198 assertions, 90.48% coverage 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- CLAUDE.refactor-progress-reporting.md | 34 +- coverage/.resultset.json | 774 +++++++++--------- require_worker.rb | 2 +- .../faye_server.rb} | 4 +- .../infrastructure/faye/progress_mapper.rb | 50 ++ .../git/mappers/clone_mapper.rb | 29 + 6 files changed, 498 insertions(+), 395 deletions(-) rename workers/infrastructure/{messaging/progress_publisher.rb => faye/faye_server.rb} (89%) create mode 100644 workers/infrastructure/faye/progress_mapper.rb create mode 100644 workers/infrastructure/git/mappers/clone_mapper.rb diff --git a/CLAUDE.refactor-progress-reporting.md b/CLAUDE.refactor-progress-reporting.md index ffe41fb..1ef334b 100644 --- a/CLAUDE.refactor-progress-reporting.md +++ b/CLAUDE.refactor-progress-reporting.md @@ -126,31 +126,31 @@ C. **Simplify**: Only report major phases, skip line-by-line git progress ## Implementation Phases -### Phase 1: Restructure Faye Infrastructure +### Phase 1: Restructure Faye Infrastructure ✅ **Rename folder**: `workers/infrastructure/messaging/` → `workers/infrastructure/faye/` **Rename file**: `progress_publisher.rb` → `faye_server.rb` -- [ ] Rename class `ProgressPublisher` → `FayeServer` -- [ ] Keep same interface: `publish(message)` -- [ ] This is pure infrastructure - no logic changes +- [x] Rename class `ProgressPublisher` → `FayeServer` +- [x] Keep same interface: `publish(message)` +- [x] This is pure infrastructure - no logic changes **New file**: `workers/infrastructure/faye/progress_mapper.rb` -- [ ] Create `Appraiser::ProgressMapper` class -- [ ] Define `PHASES` hash (single source of truth for all symbol → percentage mappings) -- [ ] Include all phases: `:started`, `:clone_receiving`, `:clone_resolving`, `:clone_done`, `:appraising`, `:appraise_done`, `:caching`, `:finished` -- [ ] Implement `map(symbol) → percentage` method -- [ ] Accept FayeServer in constructor for dependency injection -- [ ] Implement `report(symbol)` that maps and publishes via FayeServer +- [x] Create `Appraiser::ProgressMapper` class +- [x] Define `PHASES` hash (single source of truth for all symbol → percentage mappings) +- [x] Include all phases: `:started`, `:clone_receiving`, `:clone_resolving`, `:clone_done`, `:appraising`, `:appraise_done`, `:caching`, `:finished` +- [x] Implement `map(symbol) → percentage` method +- [x] Accept FayeServer in constructor for dependency injection +- [x] Implement `report(symbol)` that maps and publishes via FayeServer **New file**: `workers/infrastructure/git/mappers/clone_mapper.rb` -- [ ] Create `CloneMapper` class (or module) -- [ ] Parse git clone output lines into symbols -- [ ] Map: `"Receiving..."` → `:clone_receiving`, `"Resolving..."` → `:clone_resolving`, etc. -- [ ] Return symbol that ProgressMapper can translate to percentage +- [x] Create `CloneMapper` module +- [x] Parse git clone output lines into symbols +- [x] Map: `"Receiving..."` → `:clone_receiving`, `"Resolving..."` → `:clone_resolving`, etc. +- [x] Return symbol that ProgressMapper can translate to percentage ### Phase 2: Update Service to Use Symbols @@ -241,6 +241,12 @@ ProgressMapper.new(config, channel_id) # created with FayeServer Service::AppraiseProject.call(progress: mapper.progress_callback) ``` +### Decision 5: Symbol Naming Convention + +- Use `-ing` suffix for ongoing action phases: `cloning_*`, `appraising_*`, `caching_*` +- Symbols: `:started`, `:cloning_started`, `:cloning_remote`, `:cloning_receiving`, `:cloning_resolving`, `:cloning_done`, `:appraising_started`, `:appraising_done`, `:caching_started`, `:finished` +- **Rationale**: Consistent naming that describes ongoing actions; more readable and descriptive + ### Decision 3: Git Clone Progress with CloneMapper - Create `CloneMapper` to parse git clone output lines into symbols diff --git a/coverage/.resultset.json b/coverage/.resultset.json index 2aa2c7e..b17c15e 100644 --- a/coverage/.resultset.json +++ b/coverage/.resultset.json @@ -41,7 +41,7 @@ 4, null, 1, - 24, + 26, null, null ] @@ -227,16 +227,16 @@ 1, null, 1, - 18, + 12, null, null, 1, - 19, + 5, null, null, 1, null, - 20, + 13, null, null, null, @@ -262,7 +262,7 @@ 1, null, 1, - 77, + 49, null, null, null, @@ -284,7 +284,7 @@ null, 1, 1, - 60, + 24, null, null, null, @@ -299,7 +299,7 @@ null, null, 1, - 19, + 7, null, null, null, @@ -310,11 +310,11 @@ null, null, 1, - 67, + 43, null, null, 1, - 79, + 51, null, null, null, @@ -343,7 +343,7 @@ 1, null, 1, - 76, + 48, null, null, null, @@ -391,11 +391,11 @@ null, null, 1, - 39, + 13, null, null, 1, - 19, + 12, null, null, null, @@ -419,9 +419,9 @@ null, null, 1, - 164, + 76, null, - 164, + 76, null, null, null, @@ -430,13 +430,13 @@ null, null, 1, - 41, - 123, + 19, + 57, null, null, null, 1, - 76, + 48, null, null, null, @@ -462,23 +462,23 @@ null, null, null, - 38, + 12, null, null, null, null, - 38, + 12, null, null, 1, - 5, - 4, - 4, + 3, + 2, + 2, null, null, null, 1, - 19, + 12, null, null, 1, @@ -487,21 +487,21 @@ null, null, 1, - 19, - 19, + 12, + 12, null, null, 1, - 19, + 12, null, - 19, - 19, + 12, + 12, null, null, 1, - 76, + 36, null, - 41, + 19, null, null, null, @@ -512,21 +512,21 @@ null, 1, 1, - 19, + 12, null, null, 1, - 19, + 12, null, null, 1, - 19, + 12, null, - 19, - 19, + 12, + 12, null, - 19, - 57, + 12, + 36, null, null, null, @@ -549,45 +549,45 @@ 1, null, 1, - 61, + 45, null, null, 1, - 33, + 24, null, null, null, null, 1, - 28, + 21, null, null, null, 1, 1, - 33, - 33, + 24, + 24, null, null, 1, - 33, + 24, null, null, null, null, 1, 1, - 61, + 45, null, null, 1, - 61, + 45, null, null, null, null, - 60, - 60, + 44, + 44, null, null, null, @@ -603,11 +603,11 @@ null, null, 1, - 60, + 44, null, null, 1, - 4, + 2, null, null, null, @@ -625,29 +625,29 @@ null, 1, 1, - 28, - 28, - 28, + 21, + 21, + 21, null, null, 1, - 28, - 84, + 21, + 63, null, null, null, 1, - 112, + 84, null, null, null, 1, 1, - 112, + 84, null, null, 1, - 112, + 84, null, null, null, @@ -658,15 +658,15 @@ 1, null, 1, - 112, + 84, null, null, 1, - 112, + 84, null, null, 1, - 112, + 84, null, null, null, @@ -685,31 +685,31 @@ null, 1, 1, - 33, - 33, - 33, + 24, + 24, + 24, null, null, 1, - 33, - 28, + 24, + 21, null, null, 1, - 28, + 21, null, null, null, 1, 1, - 28, - 28, + 21, + 21, null, null, null, null, 1, - 28, + 21, null, null, null, @@ -722,31 +722,31 @@ null, null, 1, - 28, + 21, null, null, 1, - 28, + 21, null, null, 1, - 28, + 21, null, null, 1, - 28, + 21, null, null, 1, - 28, + 21, null, null, 1, - 28, + 21, null, null, 1, - 28, + 21, null, null, null, @@ -781,13 +781,13 @@ 1, null, 1, - 4, - 4, + 0, + 0, null, null, null, null, - 4, + 0, null, null, null, @@ -795,7 +795,7 @@ null, null, 1, - 4, + 0, null, null, null, @@ -865,17 +865,17 @@ 1, null, 1, - 15, + 9, null, null, 1, null, 1, - 15, + 9, null, null, 1, - 15, + 9, null, null, null, @@ -945,10 +945,10 @@ null, null, 1, - 5, - 5, - 5, - 5, + 1, + 1, + 1, + 1, null, 0, null, @@ -993,15 +993,15 @@ null, null, 1, - 23, + 18, null, - 21, - 21, + 16, + 16, null, - 19, - 19, + 14, + 14, null, - 17, + 12, null, null, null, @@ -1020,21 +1020,21 @@ null, null, 1, - 21, - 20, + 16, + 15, null, null, - 19, - 19, + 14, + 14, null, - 19, + 14, null, 1, null, null, null, 1, - 75, + 37, null, null, null, @@ -1042,22 +1042,22 @@ null, null, 1, - 23, + 18, null, + 18, 23, - 56, null, null, - 56, + 23, null, null, - 44, + 15, 6, 6, null, null, null, - 6, + 5, null, null, null, @@ -1222,7 +1222,7 @@ null, null, 1, - 13, + 0, null, null, null, @@ -1288,9 +1288,9 @@ null, 1, 1, - 34, + 14, null, - 33, + 13, null, null, null, @@ -1326,82 +1326,82 @@ null, null, 1, - 20, + 0, null, null, - 20, - 1, + 0, + 0, null, - 1, + 0, null, null, null, - 1, - 1, + 0, + 0, null, null, - 19, - 19, - 19, + 0, + 0, + 0, null, - 16, + 0, null, - 14, + 0, null, - 14, + 0, null, null, null, - 14, + 0, null, null, null, null, null, - 14, + 0, null, - 7, - 7, + 0, + 0, null, null, null, - 7, - 7, + 0, + 0, null, - 7, + 0, null, null, null, - 2, - 2, + 0, + 0, null, null, null, - 2, - 1, - 1, + 0, + 0, + 0, null, null, - 1, - 1, - 1, + 0, + 0, + 0, null, null, null, - 3, + 0, null, - 3, - 3, - 3, + 0, + 0, + 0, null, - 3, - 1, - 1, + 0, + 0, + 0, null, null, - 2, - 2, - 2, + 0, + 0, + 0, null, null, null, @@ -1452,25 +1452,25 @@ 1, null, 1, - 27, - 27, - 27, - 27, + 13, + 13, + 13, + 13, null, null, 1, null, 1, - 14, + 6, null, null, 1, - 18, + 6, null, null, null, 1, - 17, + 5, null, null, null, @@ -1497,16 +1497,16 @@ 1, null, 1, - 6, + 3, null, null, null, 1, - 6, + 3, null, null, null, - 1, + 0, null, null, null, @@ -1516,13 +1516,13 @@ null, null, 1, - 6, + 3, null, null, null, null, 1, - 5, + 3, null, null, null, @@ -1556,25 +1556,25 @@ null, null, 1, - 13, + 4, 1, null, - 12, - null, - 10, - null, 3, null, + 3, null, 1, null, - 10, - 9, null, 1, null, - 10, - null, + 3, + 2, + null, + 1, + null, + 3, + null, 0, 0, null, @@ -1582,15 +1582,15 @@ null, null, 1, - 12, + 3, null, null, null, - 3, + 1, null, null, 1, - 13, + 4, null, null, null, @@ -1628,37 +1628,37 @@ null, null, 1, - 20, + 6, null, null, null, - 20, - 18, + 6, + 5, null, - 2, + 1, null, null, 0, null, null, 1, - 18, - 2, + 5, + 1, null, - 16, + 4, null, null, null, 1, - 16, - 16, + 4, + 4, null, - 16, - 11, - 11, + 4, + 3, + 3, null, null, - 16, + 4, null, null, 0, @@ -1667,27 +1667,27 @@ null, 1, null, - 16, + 4, null, - 11, - 11, + 3, + 3, null, null, - 11, + 3, null, - 9, - 9, + 2, + 2, null, null, 1, null, - 14, + 3, null, null, - 5, + 1, null, null, - 5, + 1, null, null, null, @@ -1702,11 +1702,11 @@ 1, null, 1, - 5, + 1, null, null, null, - 5, + 1, null, null, 1, @@ -1717,14 +1717,14 @@ null, 1, null, - 11, + 3, null, null, - 7, - 7, + 2, + 2, null, null, - 5, + 1, null, null, null, @@ -1752,19 +1752,19 @@ null, null, 1, - 6, - 6, - 5, + 3, + 3, + 3, null, - 1, + 0, null, null, null, 1, - 5, - 5, - 5, - 5, + 3, + 3, + 3, + 3, null, 0, null, @@ -2397,6 +2397,97 @@ null ] }, + "/Users/soumyaray/Sync/Dropbox/ossdev/projects/codepraise/api-codepraise/workers/infrastructure/faye/faye_server.rb": { + "lines": [ + null, + null, + 1, + null, + 1, + null, + 1, + 1, + 0, + 0, + null, + null, + 1, + 0, + 0, + 0, + null, + null, + null, + null, + 0, + null, + 0, + null, + null, + 1, + null, + 1, + 0, + null, + null, + null, + null + ] + }, + "/Users/soumyaray/Sync/Dropbox/ossdev/projects/codepraise/api-codepraise/workers/infrastructure/faye/progress_mapper.rb": { + "lines": [ + null, + null, + 1, + null, + null, + 1, + 1, + null, + null, + null, + null, + null, + null, + null, + null, + null, + null, + null, + null, + 1, + 0, + null, + null, + null, + 1, + 0, + 0, + null, + null, + null, + null, + 1, + 0, + 0, + null, + null, + null, + 1, + 0, + 0, + 0, + null, + null, + null, + null, + 1, + 0, + null, + null, + null + ] + }, "/Users/soumyaray/Sync/Dropbox/ossdev/projects/codepraise/api-codepraise/workers/infrastructure/git/gateway/git_command.rb": { "lines": [ null, @@ -2408,16 +2499,16 @@ 1, null, 1, - 66, - 66, - 66, - 66, + 65, + 65, + 65, + 65, null, null, 1, - 2, - 2, - 2, + 1, + 1, + 1, null, null, 1, @@ -2432,21 +2523,21 @@ null, null, 1, - 2, - 2, + 1, + 1, null, null, 1, - 2, - 2, + 1, + 1, null, null, 1, - 132, + 130, null, null, 1, - 66, + 65, null, null, null, @@ -2457,8 +2548,8 @@ null, null, 1, - 1, - 5, + 0, + 0, null, null, null, @@ -2497,6 +2588,39 @@ null ] }, + "/Users/soumyaray/Sync/Dropbox/ossdev/projects/codepraise/api-codepraise/workers/infrastructure/git/mappers/clone_mapper.rb": { + "lines": [ + null, + null, + 1, + null, + null, + 1, + 1, + null, + null, + null, + null, + null, + null, + null, + null, + null, + 1, + 0, + 0, + null, + 0, + null, + null, + null, + 1, + 0, + null, + null, + null + ] + }, "/Users/soumyaray/Sync/Dropbox/ossdev/projects/codepraise/api-codepraise/workers/infrastructure/git/mappers/contributions_mapper.rb": { "lines": [ null, @@ -2812,9 +2936,9 @@ null, null, 1, - 3, - 3, - 3, + 2, + 2, + 2, null, null, 1, @@ -2826,14 +2950,14 @@ null, null, 1, - 6, + 4, null, null, 1, - 1, - 1, + 0, + 0, null, - 6, + 0, null, null, null @@ -2860,13 +2984,13 @@ 1, null, 1, - 3, - 3, + 2, + 2, null, null, 1, - 6, - 1, + 0, + 0, null, null, 1, @@ -2885,7 +3009,7 @@ null, null, 1, - 13, + 11, null, null, 1, @@ -2924,15 +3048,15 @@ 1, null, 1, - 3, + 2, null, null, 1, - 3, + 2, null, null, 1, - 1, + 0, null, null, null, @@ -2952,49 +3076,12 @@ null, 1, 1, - 12, - 17, - null, - null, - 1, - 17, - null, - null, - null, - null - ] - }, - "/Users/soumyaray/Sync/Dropbox/ossdev/projects/codepraise/api-codepraise/workers/infrastructure/messaging/progress_publisher.rb": { - "lines": [ - null, - null, - 1, - null, - 1, - null, - 1, - 1, 0, 0, null, null, 1, 0, - 0, - 0, - null, - null, - null, - null, - 0, - null, - 0, - null, - null, - 1, - null, - 1, - 0, null, null, null, @@ -3082,23 +3169,23 @@ 1, null, 1, - 7, - 7, - 7, - 7, - 7, - 7, + 6, + 6, + 6, + 6, + 6, + 6, null, null, null, null, 1, - 35, - 2795, - 2795, + 23, + 1303, + 1303, null, null, - 35, + 23, null, null, null, @@ -3107,7 +3194,7 @@ null, null, 1, - 35, + 23, null, null ] @@ -3121,101 +3208,10 @@ null, 1, null, - 27, - 27, - 27, - 27, - null, - null - ] - }, - "/Users/soumyaray/Sync/Dropbox/ossdev/projects/codepraise/api-codepraise/spec/helpers/cache_helper.rb": { - "lines": [ - null, - null, - null, - null, - null, - null, - 1, - null, - 1, - 44, - null, - null, - null, - 1, - 64, - 64, - null, - null - ] - }, - "/Users/soumyaray/Sync/Dropbox/ossdev/projects/codepraise/api-codepraise/spec/tests/integration/layers/domain_contributions_spec.rb": { - "lines": [ - null, - null, - 1, - 1, - 1, - null, - 1, - 1, - null, - 1, - 2, - 2, - null, - 2, - null, - null, - null, - 2, - null, - null, - 2, - 2, - null, - null, - 1, - 2, - null, - null, - 1, - 1, - 1, - 1, - null, - 1, - 1, - 1, - null, - null, - 1, - null, - null, - 1, - null, - null, - 1, - null, - null, - null, - 1, - 1, - null, - 1, - null, - 1, - null, - 1, - 1, - null, - 1, - 1, - null, - 1, - 1, + 15, + 15, + 15, + 15, null, null ] @@ -3695,6 +3691,28 @@ null ] }, + "/Users/soumyaray/Sync/Dropbox/ossdev/projects/codepraise/api-codepraise/spec/helpers/cache_helper.rb": { + "lines": [ + null, + null, + null, + null, + null, + null, + 1, + null, + 1, + 20, + null, + null, + null, + 1, + 40, + 40, + null, + null + ] + }, "/Users/soumyaray/Sync/Dropbox/ossdev/projects/codepraise/api-codepraise/spec/tests/integration/services/list_projects_spec.rb": { "lines": [ null, @@ -4727,6 +4745,6 @@ ] } }, - "timestamp": 1766550348 + "timestamp": 1766593526 } } diff --git a/require_worker.rb b/require_worker.rb index 7ae1c41..f28d55b 100644 --- a/require_worker.rb +++ b/require_worker.rb @@ -3,7 +3,7 @@ # Requires all ruby files in specified worker folders # Worker has its own DDD layers parallel to the API: # - domain: contributions entities/values/lib -# - infrastructure: git gateway/repositories/mappers, messaging +# - infrastructure: git gateway/repositories/mappers, faye server/mapper # - presentation: progress monitor values # - application: controllers, services, requests # Params: diff --git a/workers/infrastructure/messaging/progress_publisher.rb b/workers/infrastructure/faye/faye_server.rb similarity index 89% rename from workers/infrastructure/messaging/progress_publisher.rb rename to workers/infrastructure/faye/faye_server.rb index beef60b..58512fa 100644 --- a/workers/infrastructure/messaging/progress_publisher.rb +++ b/workers/infrastructure/faye/faye_server.rb @@ -3,8 +3,8 @@ require 'http' module Appraiser - # Publishes progress as percent to Faye endpoint - class ProgressPublisher + # Gateway to Faye server for publishing progress messages + class FayeServer def initialize(config, channel_id) @config = config @channel_id = channel_id diff --git a/workers/infrastructure/faye/progress_mapper.rb b/workers/infrastructure/faye/progress_mapper.rb new file mode 100644 index 0000000..101ec39 --- /dev/null +++ b/workers/infrastructure/faye/progress_mapper.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +module Appraiser + # Maps progress symbols to percentages and publishes via FayeServer + # Single source of truth for all progress phase percentages + class ProgressMapper + PHASES = { + started: 15, + cloning_started: 20, + cloning_remote: 35, + cloning_receiving: 40, + cloning_resolving: 45, + cloning_done: 50, + appraising_started: 55, + appraising_done: 85, + caching_started: 90, + finished: 100 + }.freeze + + def initialize(faye_server) + @faye_server = faye_server + end + + # Map a progress symbol to its percentage + def map(symbol) + PHASES.fetch(symbol) do + raise ArgumentError, "Unknown progress phase: #{symbol}" + end + end + + # Report progress by mapping symbol to percentage and publishing + def report(symbol) + percentage = map(symbol) + @faye_server.publish(percentage.to_s) + end + + # Report the same symbol repeatedly for a number of seconds + def report_each_second(seconds, symbol) + seconds.times do + sleep(1) + report(symbol) + end + end + + # Returns a proc that can be passed to services for progress reporting + def progress_callback + ->(symbol) { report(symbol) } + end + end +end diff --git a/workers/infrastructure/git/mappers/clone_mapper.rb b/workers/infrastructure/git/mappers/clone_mapper.rb new file mode 100644 index 0000000..ce2551c --- /dev/null +++ b/workers/infrastructure/git/mappers/clone_mapper.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module CodePraise + # Maps git clone output lines to progress symbols + # Used by Service::AppraiseProject to report fine-grained clone progress + module CloneMapper + CLONE_PATTERNS = { + /^Cloning/i => :cloning_started, + /^remote:/i => :cloning_remote, + /^Receiving/i => :cloning_receiving, + /^Resolving/i => :cloning_resolving, + /^Checking/i => :cloning_done + }.freeze + + # Map a git clone output line to a progress symbol + # Returns nil if line doesn't match any known pattern + def self.map(line) + CLONE_PATTERNS.each do |pattern, symbol| + return symbol if line.match?(pattern) + end + nil + end + + # Map a git clone output line, returning a default symbol if no match + def self.map_or_default(line, default = :cloning_started) + map(line) || default + end + end +end From d35111fec63d6a8537c35ae751e7d0eb97cd1787 Mon Sep 17 00:00:00 2001 From: Soumya Ray Date: Thu, 25 Dec 2025 00:40:59 +0800 Subject: [PATCH 4/6] refactor: implement symbol-based progress reporting (Phases 2-4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Update Service::AppraiseProject to use symbols instead of percentages - Use CloneMapper to convert git clone output to progress symbols - Update Worker controller to use ProgressMapper instead of JobReporter - Delete obsolete JobReporter and progress_monitor files - Update CLAUDE.md to reflect new architecture - Replace scale_clone_progress tests with CloneMapper tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- CLAUDE.md | 26 ++-- CLAUDE.refactor-progress-reporting.md | 31 ++-- coverage/.resultset.json | 137 +++++------------- spec/tests/unit/worker_appraise_spec.rb | 47 +++--- workers/application/controllers/worker.rb | 27 +++- workers/application/requests/job_reporter.rb | 36 ----- .../application/services/appraise_project.rb | 38 ++--- .../presentation/values/progress_monitor.rb | 64 -------- 8 files changed, 121 insertions(+), 285 deletions(-) delete mode 100644 workers/application/requests/job_reporter.rb delete mode 100644 workers/presentation/values/progress_monitor.rb diff --git a/CLAUDE.md b/CLAUDE.md index 09c4798..eeef66a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -367,12 +367,10 @@ workers/ ├── domain/contributions/ # Entities, values, lib for git analysis ├── infrastructure/ │ ├── git/ # Git gateway, repositories, mappers -│ └── messaging/ # Faye progress publisher -├── presentation/values/ # Progress monitor calculations +│ └── faye/ # Faye server gateway and progress mapper ├── application/ │ ├── controllers/worker.rb # Shoryuken entry point -│ ├── services/appraise_project.rb -│ └── requests/job_reporter.rb +│ └── services/appraise_project.rb └── shoryuken*.yml # Queue configs ``` @@ -388,10 +386,7 @@ workers/ - Clones repo if not exists locally - Runs git blame analysis via `Mapper::Contributions` - Stores serialized JSON in Redis with TTL - -- `requests/job_reporter.rb`: Manages progress reporting - - Deserializes AppraisalJob from JSON - - Publishes progress updates to Faye channel + - Reports progress via symbols (`:started`, `:cloning_done`, `:appraising_started`, etc.) **Worker Infrastructure Layer (`workers/infrastructure/`):** @@ -408,14 +403,13 @@ workers/ - `FileContributionsMapper`: Maps file data to `Entity::FileContributions` - `BlameContributor`: Maps git contributor info to `Entity::Contributor` - `PorcelainParser`: Parses git blame porcelain format -- `messaging/progress_publisher.rb`: Sends progress to Faye + - `CloneMapper`: Maps git clone output lines to progress symbols +- `faye/faye_server.rb`: Gateway to Faye server - POSTs JSON messages to `/faye` endpoint - Channel format: `/{request_id}` - -**Worker Presentation Layer (`workers/presentation/`):** - -- `values/progress_monitor.rb`: Tracks progress phases - - `AppraisalMonitor`: Full appraisal progress (15-50% clone, 55-85% appraise, 90-100% cache) +- `faye/progress_mapper.rb`: Maps progress symbols to percentages + - Single source of truth for all symbol → percentage mappings + - Symbols: `:started`, `:cloning_*`, `:appraising_*`, `:caching_*`, `:finished` **Configuration Files:** @@ -655,11 +649,9 @@ Tests are organized by scope in `spec/tests/`: - Contributions domain (git analysis) → `workers/domain/contributions/` - Git operations → `workers/infrastructure/git/` -- Faye messaging → `workers/infrastructure/messaging/` -- Progress tracking → `workers/presentation/values/` +- Faye gateway and progress mapping → `workers/infrastructure/faye/` - Worker entry point → `workers/application/controllers/worker.rb` - Worker services → `workers/application/services/` -- Request handling → `workers/application/requests/` **Adding a New API Endpoint:** 1. Define service object in `app/application/services/` using Dry::Transaction diff --git a/CLAUDE.refactor-progress-reporting.md b/CLAUDE.refactor-progress-reporting.md index 1ef334b..367c500 100644 --- a/CLAUDE.refactor-progress-reporting.md +++ b/CLAUDE.refactor-progress-reporting.md @@ -152,32 +152,33 @@ C. **Simplify**: Only report major phases, skip line-by-line git progress - [x] Map: `"Receiving..."` → `:clone_receiving`, `"Resolving..."` → `:clone_resolving`, etc. - [x] Return symbol that ProgressMapper can translate to percentage -### Phase 2: Update Service to Use Symbols +### Phase 2: Update Service to Use Symbols ✅ **Modify**: `workers/application/services/appraise_project.rb` -- [ ] Change all `input[:progress].call(15)` → `input[:progress].call(:started)` -- [ ] Remove `scale_clone_progress` method -- [ ] Use CloneMapper to convert git output lines to symbols -- [ ] Use symbols throughout: `:started`, `:clone_receiving`, `:clone_done`, `:appraising`, etc. +- [x] Change all `input[:progress].call(15)` → `input[:progress].call(:started)` +- [x] Remove `scale_clone_progress` method +- [x] Use CloneMapper to convert git output lines to symbols +- [x] Use symbols throughout: `:started`, `:cloning_receiving`, `:cloning_done`, `:appraising_started`, etc. +- [x] Update tests: Replace `scale_clone_progress` tests with `CloneMapper` tests -### Phase 3: Update Worker Controller +### Phase 3: Update Worker Controller ✅ **Modify**: `workers/application/controllers/worker.rb` -- [ ] Move job JSON deserialization from JobReporter to controller -- [ ] Create ProgressMapper with FayeServer and channel_id -- [ ] Pass `progress_mapper.progress_callback` to service -- [ ] Update `report_each_second` logic to use ProgressMapper with `:finished` symbol +- [x] Move job JSON deserialization from JobReporter to controller +- [x] Create ProgressMapper with FayeServer and channel_id +- [x] Pass `progress_mapper.progress_callback` to service +- [x] Update `report_each_second` logic to use ProgressMapper with `:finished` symbol -### Phase 4: Cleanup +### Phase 4: Cleanup ✅ -- [ ] Delete `workers/application/requests/job_reporter.rb` -- [ ] Delete `workers/presentation/values/progress_monitor.rb` +- [x] Delete `workers/application/requests/job_reporter.rb` +- [x] Delete `workers/presentation/values/progress_monitor.rb` - `CloneMonitor` module (marked as legacy) - `AppraisalMonitor` module (replaced by ProgressMapper) -- [ ] Update `require_worker.rb` for new file locations -- [ ] Update tests for new symbol-based interface +- [x] `require_worker.rb` already updated in Phase 1 +- [x] Tests updated in Phase 2 (CloneMapper tests replace scale_clone_progress tests) ## Open Questions diff --git a/coverage/.resultset.json b/coverage/.resultset.json index b17c15e..250abb1 100644 --- a/coverage/.resultset.json +++ b/coverage/.resultset.json @@ -41,7 +41,7 @@ 4, null, 1, - 26, + 25, null, null ] @@ -284,7 +284,7 @@ null, 1, 1, - 24, + 20, null, null, null, @@ -310,11 +310,11 @@ null, null, 1, - 43, + 35, null, null, 1, - 51, + 43, null, null, null, @@ -2607,15 +2607,15 @@ null, null, 1, - 0, - 0, + 8, + 30, null, - 0, + 3, null, null, null, 1, - 0, + 2, null, null, null @@ -3088,74 +3088,6 @@ null ] }, - "/Users/soumyaray/Sync/Dropbox/ossdev/projects/codepraise/api-codepraise/workers/presentation/values/progress_monitor.rb": { - "lines": [ - null, - null, - 1, - null, - null, - 1, - 1, - null, - null, - null, - null, - null, - null, - null, - null, - null, - 1, - 0, - null, - null, - 1, - 0, - null, - null, - 1, - 0, - null, - null, - 1, - 0, - null, - null, - 1, - 0, - null, - null, - null, - null, - null, - null, - null, - null, - 1, - 1, - null, - null, - null, - null, - null, - null, - null, - null, - null, - null, - null, - 1, - 0, - null, - null, - 1, - 0, - null, - null, - null - ] - }, "/Users/soumyaray/Sync/Dropbox/ossdev/projects/codepraise/api-codepraise/spec/helpers/vcr_helper.rb": { "lines": [ null, @@ -3181,8 +3113,8 @@ null, 1, 23, - 1303, - 1303, + 1228, + 1228, null, null, 23, @@ -3702,13 +3634,13 @@ 1, null, 1, - 20, + 16, null, null, null, 1, - 40, - 40, + 32, + 32, null, null ] @@ -4444,11 +4376,11 @@ null, 1, 1, - 7, - 7, + 3, + 3, null, null, - 7, + 3, null, null, null, @@ -4463,12 +4395,12 @@ null, null, null, - 7, - 7, + 3, + 3, null, null, 1, - 7, + 3, null, null, 1, @@ -4505,7 +4437,8 @@ null, null, null, - 1, + null, + null, 1, 1, 1, @@ -4513,18 +4446,30 @@ null, 1, 1, + null, + null, + 1, 1, null, null, 1, 1, + null, + null, + 1, 1, null, null, 1, 1, + null, + null, 1, + 1, + null, null, + 1, + 1, null, null, null, @@ -4701,20 +4646,6 @@ null, null, 1, - 4, - null, - null, - null, - null, - null, - null, - null, - 4, - 4, - null, - null, - null, - 1, 3, 4, null, @@ -4745,6 +4676,6 @@ ] } }, - "timestamp": 1766593526 + "timestamp": 1766594344 } } diff --git a/spec/tests/unit/worker_appraise_spec.rb b/spec/tests/unit/worker_appraise_spec.rb index aff4764..643ee6b 100644 --- a/spec/tests/unit/worker_appraise_spec.rb +++ b/spec/tests/unit/worker_appraise_spec.rb @@ -67,26 +67,39 @@ end end - describe 'scale_clone_progress helper' do - it 'should scale Cloning to 25' do - service = Appraiser::Service::AppraiseProject.new - _(service.send(:scale_clone_progress, 'Cloning into...')).must_equal 25 - end +end - it 'should scale Receiving to 40' do - service = Appraiser::Service::AppraiseProject.new - _(service.send(:scale_clone_progress, 'Receiving objects: 50%')).must_equal 40 - end +describe 'Unit test of CodePraise::CloneMapper' do + it 'should map Cloning to :cloning_started' do + _(CodePraise::CloneMapper.map('Cloning into...')).must_equal :cloning_started + end - it 'should scale Checking to 50' do - service = Appraiser::Service::AppraiseProject.new - _(service.send(:scale_clone_progress, 'Checking connectivity...')).must_equal 50 - end + it 'should map remote: to :cloning_remote' do + _(CodePraise::CloneMapper.map('remote: Counting objects')).must_equal :cloning_remote + end - it 'should default unknown stages to 30' do - service = Appraiser::Service::AppraiseProject.new - _(service.send(:scale_clone_progress, 'Unknown stage')).must_equal 30 - end + it 'should map Receiving to :cloning_receiving' do + _(CodePraise::CloneMapper.map('Receiving objects: 50%')).must_equal :cloning_receiving + end + + it 'should map Resolving to :cloning_resolving' do + _(CodePraise::CloneMapper.map('Resolving deltas: 100%')).must_equal :cloning_resolving + end + + it 'should map Checking to :cloning_done' do + _(CodePraise::CloneMapper.map('Checking connectivity...')).must_equal :cloning_done + end + + it 'should return nil for unknown lines' do + _(CodePraise::CloneMapper.map('Unknown stage')).must_be_nil + end + + it 'should return default for unknown lines with map_or_default' do + _(CodePraise::CloneMapper.map_or_default('Unknown stage')).must_equal :cloning_started + end + + it 'should return custom default when specified' do + _(CodePraise::CloneMapper.map_or_default('Unknown', :custom)).must_equal :custom end end diff --git a/workers/application/controllers/worker.rb b/workers/application/controllers/worker.rb index 537ef23..099bd96 100644 --- a/workers/application/controllers/worker.rb +++ b/workers/application/controllers/worker.rb @@ -31,9 +31,11 @@ def self.config = Figaro.env Shoryuken.sqs_client_receive_message_opts = { wait_time_seconds: 20 } shoryuken_options queue: config.WORKER_QUEUE_URL, auto_delete: true - def perform(_sqs_msg, request) - job = JobReporter.new(request, Worker.config) - perform_appraisal(job) + def perform(_sqs_msg, request_json) + job = deserialize_job(request_json) + progress_mapper = build_progress_mapper(job.id) + + perform_appraisal(job, progress_mapper) rescue CodePraise::GitRepo::Errors::CannotOverwriteLocalGitRepo # worker should crash fail early - only catch errors we expect! puts 'CLONE EXISTS -- ignoring request' @@ -41,19 +43,30 @@ def perform(_sqs_msg, request) private - def perform_appraisal(job) + def deserialize_job(request_json) + CodePraise::Representer::AppraisalJob + .new(OpenStruct.new) + .from_json(request_json) + end + + def build_progress_mapper(channel_id) + faye_server = FayeServer.new(Worker.config, channel_id) + ProgressMapper.new(faye_server) + end + + def perform_appraisal(job, progress_mapper) gitrepo = CodePraise::GitRepo.new(job.project, Worker.config) Service::AppraiseProject.new.call( project: job.project, - folder_path: job.folder_path, + folder_path: job.folder_path || '', config: Worker.config, gitrepo: gitrepo, - progress: job.progress_callback + progress: progress_mapper.progress_callback ) # Keep sending finished status to any latecoming subscribers - job.report_each_second(5) { AppraisalMonitor.finished_percent } + progress_mapper.report_each_second(5, :finished) end end end diff --git a/workers/application/requests/job_reporter.rb b/workers/application/requests/job_reporter.rb deleted file mode 100644 index 1a3b2ab..0000000 --- a/workers/application/requests/job_reporter.rb +++ /dev/null @@ -1,36 +0,0 @@ -# frozen_string_literal: true - -# Note: ProgressPublisher is loaded via require_worker (infrastructure/messaging) - -module Appraiser - # Reports job progress to client - class JobReporter - attr_reader :project, :folder_path - - def initialize(job_json, config) - job = CodePraise::Representer::AppraisalJob - .new(OpenStruct.new) - .from_json(job_json) - - @project = job.project - @folder_path = job.folder_path || '' - @publisher = ProgressPublisher.new(config, job.id) - end - - def report(msg) - @publisher.publish msg - end - - def report_each_second(seconds, &operation) - seconds.times do - sleep(1) - report(operation.call) - end - end - - # Returns a proc that can be passed to services for progress reporting - def progress_callback - ->(percent) { report(percent.to_s) } - end - end -end diff --git a/workers/application/services/appraise_project.rb b/workers/application/services/appraise_project.rb index 1db3382..3ba9016 100644 --- a/workers/application/services/appraise_project.rb +++ b/workers/application/services/appraise_project.rb @@ -19,7 +19,7 @@ class AppraiseProject # folder_path: String path to folder being appraised # config: Worker config with Redis URL # gitrepo: GitRepo instance - # progress: Proc to report progress + # progress: Proc to report progress (accepts symbols, not percentages) def prepare_inputs(input) # Convert OpenStruct project to Entity::Project for Value::Appraisal @@ -31,15 +31,15 @@ def prepare_inputs(input) end def clone_repo(input) - input[:progress].call(15) # STARTED + input[:progress].call(:started) if input[:gitrepo].exists_locally? - input[:progress].call(50) # Skip to post-clone + input[:progress].call(:cloning_done) else input[:gitrepo].clone_locally do |line| - # Scale clone progress from 15 to 50 - percent = scale_clone_progress(line) - input[:progress].call(percent) + # Use CloneMapper to convert git output to progress symbol + symbol = CodePraise::CloneMapper.map_or_default(line) + input[:progress].call(symbol) end end @@ -60,13 +60,13 @@ def appraise_contributions(input) # Skip if already errored in clone step return Success(input) if input[:appraisal]&.error? - input[:progress].call(55) # Starting appraisal + input[:progress].call(:appraising_started) folder = CodePraise::Mapper::Contributions .new(input[:gitrepo]) .for_folder(input[:folder_path]) - input[:progress].call(85) # Appraisal complete + input[:progress].call(:appraising_done) # Build successful appraisal value object input[:appraisal] = CodePraise::Value::Appraisal.success( @@ -90,7 +90,7 @@ def appraise_contributions(input) end def cache_result(input) - input[:progress].call(90) # Caching + input[:progress].call(:caching_started) appraisal = input[:appraisal] json = CodePraise::Representer::Appraisal.new(appraisal).to_json @@ -98,32 +98,18 @@ def cache_result(input) cache = CodePraise::Cache::Remote.new(input[:config]) cache.set(appraisal.cache_key, json, ttl: appraisal.ttl) - input[:progress].call(100) # FINISHED + input[:progress].call(:finished) Success(input) rescue StandardError => e - # Cache failure - still report 100% so client can retry + # Cache failure - still report finished so client can retry puts "CACHE ERROR: #{e.message}" - input[:progress].call(100) + input[:progress].call(:finished) Success(input) end private - # Scale git clone progress (15-50 range) - def scale_clone_progress(line) - clone_stages = { - 'Cloning' => 25, - 'remote' => 35, - 'Receiving' => 40, - 'Resolving' => 45, - 'Checking' => 50 - } - - first_word = line.match(/^[A-Za-z]+/).to_s - clone_stages[first_word] || 30 - end - # Convert OpenStruct project (from JSON) to Entity::Project def build_project_entity(project_ostruct) owner = build_member_entity(project_ostruct.owner) diff --git a/workers/presentation/values/progress_monitor.rb b/workers/presentation/values/progress_monitor.rb deleted file mode 100644 index cfd7ffd..0000000 --- a/workers/presentation/values/progress_monitor.rb +++ /dev/null @@ -1,64 +0,0 @@ -# frozen_string_literal: true - -module Appraiser - # Infrastructure to clone while yielding progress - # Legacy module for backwards compatibility - module CloneMonitor - CLONE_PROGRESS = { - 'STARTED' => 15, - 'Cloning' => 30, - 'remote' => 70, - 'Receiving' => 85, - 'Resolving' => 95, - 'Checking' => 100, - 'FINISHED' => 100 - }.freeze - - def self.starting_percent - CLONE_PROGRESS['STARTED'].to_s - end - - def self.finished_percent - CLONE_PROGRESS['FINISHED'].to_s - end - - def self.progress(line) - CLONE_PROGRESS[first_word_of(line)].to_s - end - - def self.percent(stage) - CLONE_PROGRESS[stage].to_s - end - - def self.first_word_of(line) - line.match(/^[A-Za-z]+/).to_s - end - end - - # Progress phases for full appraisal flow (clone + appraise + cache) - # Progress distribution: - # Clone: 0-50% (or skip if already cloned) - # Appraise: 50-90% - # Cache: 90-100% - module AppraisalMonitor - PHASES = { - started: 15, - cloning: 25, - clone_receiving: 40, - clone_resolving: 45, - clone_done: 50, - appraising: 55, - appraise_done: 85, - caching: 90, - finished: 100 - }.freeze - - def self.starting_percent - PHASES[:started].to_s - end - - def self.finished_percent - PHASES[:finished].to_s - end - end -end From 470f641c52f77ea87db3c0d1a634da9f0815f6de Mon Sep 17 00:00:00 2001 From: Soumya Ray Date: Thu, 25 Dec 2025 00:47:14 +0800 Subject: [PATCH 5/6] refactor: pass ProgressMapper object directly instead of callback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Service uses input[:progress].report(:symbol) instead of input[:progress].call(:symbol) - Remove progress_callback method from ProgressMapper - Cleaner interface with explicit method calls 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- coverage/.resultset.json | 617 ++++++++++-------- workers/application/controllers/worker.rb | 2 +- .../application/services/appraise_project.rb | 18 +- .../infrastructure/faye/progress_mapper.rb | 4 - 4 files changed, 351 insertions(+), 290 deletions(-) diff --git a/coverage/.resultset.json b/coverage/.resultset.json index 250abb1..e7bbd58 100644 --- a/coverage/.resultset.json +++ b/coverage/.resultset.json @@ -227,16 +227,16 @@ 1, null, 1, - 12, + 18, null, null, 1, - 5, + 19, null, null, 1, null, - 13, + 20, null, null, null, @@ -262,7 +262,7 @@ 1, null, 1, - 49, + 77, null, null, null, @@ -284,7 +284,7 @@ null, 1, 1, - 20, + 56, null, null, null, @@ -299,7 +299,7 @@ null, null, 1, - 7, + 19, null, null, null, @@ -310,11 +310,11 @@ null, null, 1, - 35, + 59, null, null, 1, - 43, + 71, null, null, null, @@ -343,7 +343,7 @@ 1, null, 1, - 48, + 76, null, null, null, @@ -391,11 +391,11 @@ null, null, 1, - 13, + 39, null, null, 1, - 12, + 19, null, null, null, @@ -419,9 +419,9 @@ null, null, 1, - 76, + 164, null, - 76, + 164, null, null, null, @@ -430,13 +430,13 @@ null, null, 1, - 19, - 57, + 41, + 123, null, null, null, 1, - 48, + 76, null, null, null, @@ -462,23 +462,23 @@ null, null, null, - 12, + 38, null, null, null, null, - 12, + 38, null, null, 1, - 3, - 2, - 2, + 5, + 4, + 4, null, null, null, 1, - 12, + 19, null, null, 1, @@ -487,21 +487,21 @@ null, null, 1, - 12, - 12, + 19, + 19, null, null, 1, - 12, + 19, null, - 12, - 12, + 19, + 19, null, null, 1, - 36, + 76, null, - 19, + 41, null, null, null, @@ -512,21 +512,21 @@ null, 1, 1, - 12, + 19, null, null, 1, - 12, + 19, null, null, 1, - 12, + 19, null, - 12, - 12, + 19, + 19, null, - 12, - 36, + 19, + 57, null, null, null, @@ -549,45 +549,45 @@ 1, null, 1, - 45, + 61, null, null, 1, - 24, + 33, null, null, null, null, 1, - 21, + 28, null, null, null, 1, 1, - 24, - 24, + 33, + 33, null, null, 1, - 24, + 33, null, null, null, null, 1, 1, - 45, + 61, null, null, 1, - 45, + 61, null, null, null, null, - 44, - 44, + 60, + 60, null, null, null, @@ -603,11 +603,11 @@ null, null, 1, - 44, + 60, null, null, 1, - 2, + 4, null, null, null, @@ -625,29 +625,29 @@ null, 1, 1, - 21, - 21, - 21, + 28, + 28, + 28, null, null, 1, - 21, - 63, + 28, + 84, null, null, null, 1, - 84, + 112, null, null, null, 1, 1, - 84, + 112, null, null, 1, - 84, + 112, null, null, null, @@ -658,15 +658,15 @@ 1, null, 1, - 84, + 112, null, null, 1, - 84, + 112, null, null, 1, - 84, + 112, null, null, null, @@ -685,31 +685,31 @@ null, 1, 1, - 24, - 24, - 24, + 33, + 33, + 33, null, null, 1, - 24, - 21, + 33, + 28, null, null, 1, - 21, + 28, null, null, null, 1, 1, - 21, - 21, + 28, + 28, null, null, null, null, 1, - 21, + 28, null, null, null, @@ -722,31 +722,31 @@ null, null, 1, - 21, + 28, null, null, 1, - 21, + 28, null, null, 1, - 21, + 28, null, null, 1, - 21, + 28, null, null, 1, - 21, + 28, null, null, 1, - 21, + 28, null, null, 1, - 21, + 28, null, null, null, @@ -781,13 +781,13 @@ 1, null, 1, - 0, - 0, + 4, + 4, null, null, null, null, - 0, + 4, null, null, null, @@ -795,7 +795,7 @@ null, null, 1, - 0, + 4, null, null, null, @@ -865,17 +865,17 @@ 1, null, 1, - 9, + 15, null, null, 1, null, 1, - 9, + 15, null, null, 1, - 9, + 15, null, null, null, @@ -945,10 +945,10 @@ null, null, 1, - 1, - 1, - 1, - 1, + 5, + 5, + 5, + 5, null, 0, null, @@ -993,15 +993,15 @@ null, null, 1, - 18, + 23, null, - 16, - 16, + 21, + 21, null, - 14, - 14, + 19, + 19, null, - 12, + 17, null, null, null, @@ -1020,21 +1020,21 @@ null, null, 1, - 16, - 15, + 21, + 20, null, null, - 14, - 14, + 19, + 19, null, - 14, + 19, null, 1, null, null, null, 1, - 37, + 75, null, null, null, @@ -1042,22 +1042,22 @@ null, null, 1, - 18, + 23, null, - 18, 23, + 56, null, null, - 23, + 56, null, null, - 15, + 44, 6, 6, null, null, null, - 5, + 6, null, null, null, @@ -1222,7 +1222,7 @@ null, null, 1, - 0, + 13, null, null, null, @@ -1288,9 +1288,9 @@ null, 1, 1, - 14, + 34, null, - 13, + 33, null, null, null, @@ -1326,82 +1326,82 @@ null, null, 1, - 0, + 20, null, null, - 0, - 0, + 20, + 1, null, - 0, + 1, null, null, null, - 0, - 0, + 1, + 1, null, null, - 0, - 0, - 0, + 19, + 19, + 19, null, - 0, + 16, null, - 0, + 14, null, - 0, + 14, null, null, null, - 0, + 14, null, null, null, null, null, - 0, + 14, null, - 0, - 0, + 7, + 7, null, null, null, - 0, - 0, + 7, + 7, null, - 0, + 7, null, null, null, - 0, - 0, + 2, + 2, null, null, null, - 0, - 0, - 0, + 2, + 1, + 1, null, null, - 0, - 0, - 0, + 1, + 1, + 1, null, null, null, - 0, + 3, null, - 0, - 0, - 0, + 3, + 3, + 3, null, - 0, - 0, - 0, + 3, + 1, + 1, null, null, - 0, - 0, - 0, + 2, + 2, + 2, null, null, null, @@ -1452,25 +1452,25 @@ 1, null, 1, - 13, - 13, - 13, - 13, + 27, + 27, + 27, + 27, null, null, 1, null, 1, - 6, + 14, null, null, 1, - 6, + 18, null, null, null, 1, - 5, + 17, null, null, null, @@ -1497,16 +1497,16 @@ 1, null, 1, - 3, + 6, null, null, null, 1, - 3, + 6, null, null, null, - 0, + 1, null, null, null, @@ -1516,13 +1516,13 @@ null, null, 1, - 3, + 6, null, null, null, null, 1, - 3, + 5, null, null, null, @@ -1556,24 +1556,24 @@ null, null, 1, - 4, + 13, 1, null, - 3, + 12, null, - 3, + 10, null, - 1, + 3, null, null, 1, null, - 3, - 2, + 10, + 9, null, 1, null, - 3, + 10, null, 0, 0, @@ -1582,15 +1582,15 @@ null, null, 1, - 3, + 12, null, null, null, - 1, + 3, null, null, 1, - 4, + 13, null, null, null, @@ -1628,37 +1628,37 @@ null, null, 1, - 6, + 20, null, null, null, - 6, - 5, + 20, + 18, null, - 1, + 2, null, null, 0, null, null, 1, - 5, - 1, + 18, + 2, null, - 4, + 16, null, null, null, 1, - 4, - 4, + 16, + 16, null, - 4, - 3, - 3, + 16, + 11, + 11, null, null, - 4, + 16, null, null, 0, @@ -1667,27 +1667,27 @@ null, 1, null, - 4, + 16, null, - 3, - 3, + 11, + 11, null, null, - 3, + 11, null, - 2, - 2, + 9, + 9, null, null, 1, null, - 3, + 14, null, null, - 1, + 5, null, null, - 1, + 5, null, null, null, @@ -1702,11 +1702,11 @@ 1, null, 1, - 1, + 5, null, null, null, - 1, + 5, null, null, 1, @@ -1717,14 +1717,14 @@ null, 1, null, - 3, + 11, null, null, - 2, - 2, + 7, + 7, null, null, - 1, + 5, null, null, null, @@ -1752,19 +1752,19 @@ null, null, 1, - 3, - 3, - 3, + 6, + 6, + 5, null, - 0, + 1, null, null, null, 1, - 3, - 3, - 3, - 3, + 5, + 5, + 5, + 5, null, 0, null, @@ -2481,10 +2481,6 @@ null, null, null, - 1, - 0, - null, - null, null ] }, @@ -2499,16 +2495,16 @@ 1, null, 1, - 65, - 65, - 65, - 65, + 66, + 66, + 66, + 66, null, null, 1, - 1, - 1, - 1, + 2, + 2, + 2, null, null, 1, @@ -2523,21 +2519,21 @@ null, null, 1, - 1, - 1, + 2, + 2, null, null, 1, - 1, - 1, + 2, + 2, null, null, 1, - 130, + 132, null, null, 1, - 65, + 66, null, null, null, @@ -2548,8 +2544,8 @@ null, null, 1, - 0, - 0, + 1, + 5, null, null, null, @@ -2936,9 +2932,9 @@ null, null, 1, - 2, - 2, - 2, + 3, + 3, + 3, null, null, 1, @@ -2950,14 +2946,14 @@ null, null, 1, - 4, + 6, null, null, 1, - 0, - 0, + 1, + 1, null, - 0, + 6, null, null, null @@ -2984,13 +2980,13 @@ 1, null, 1, - 2, - 2, + 3, + 3, null, null, 1, - 0, - 0, + 6, + 1, null, null, 1, @@ -3009,7 +3005,7 @@ null, null, 1, - 11, + 13, null, null, 1, @@ -3048,15 +3044,15 @@ 1, null, 1, - 2, + 3, null, null, 1, - 2, + 3, null, null, 1, - 0, + 1, null, null, null, @@ -3076,12 +3072,12 @@ null, 1, 1, - 0, - 0, + 12, + 17, null, null, 1, - 0, + 17, null, null, null, @@ -3101,23 +3097,23 @@ 1, null, 1, - 6, - 6, - 6, - 6, - 6, - 6, + 7, + 7, + 7, + 7, + 7, + 7, null, null, null, null, 1, - 23, - 1228, - 1228, + 35, + 2860, + 2860, null, null, - 23, + 35, null, null, null, @@ -3126,7 +3122,7 @@ null, null, 1, - 23, + 35, null, null ] @@ -3140,10 +3136,101 @@ null, 1, null, - 15, - 15, - 15, - 15, + 27, + 27, + 27, + 27, + null, + null + ] + }, + "/Users/soumyaray/Sync/Dropbox/ossdev/projects/codepraise/api-codepraise/spec/helpers/cache_helper.rb": { + "lines": [ + null, + null, + null, + null, + null, + null, + 1, + null, + 1, + 40, + null, + null, + null, + 1, + 56, + 56, + null, + null + ] + }, + "/Users/soumyaray/Sync/Dropbox/ossdev/projects/codepraise/api-codepraise/spec/tests/integration/layers/domain_contributions_spec.rb": { + "lines": [ + null, + null, + 1, + 1, + 1, + null, + 1, + 1, + null, + 1, + 2, + 2, + null, + 2, + null, + null, + null, + 2, + null, + null, + 2, + 2, + null, + null, + 1, + 2, + null, + null, + 1, + 1, + 1, + 1, + null, + 1, + 1, + 1, + null, + null, + 1, + null, + null, + 1, + null, + null, + 1, + null, + null, + null, + 1, + 1, + null, + 1, + null, + 1, + null, + 1, + 1, + null, + 1, + 1, + null, + 1, + 1, null, null ] @@ -3623,28 +3710,6 @@ null ] }, - "/Users/soumyaray/Sync/Dropbox/ossdev/projects/codepraise/api-codepraise/spec/helpers/cache_helper.rb": { - "lines": [ - null, - null, - null, - null, - null, - null, - 1, - null, - 1, - 16, - null, - null, - null, - 1, - 32, - 32, - null, - null - ] - }, "/Users/soumyaray/Sync/Dropbox/ossdev/projects/codepraise/api-codepraise/spec/tests/integration/services/list_projects_spec.rb": { "lines": [ null, @@ -4676,6 +4741,6 @@ ] } }, - "timestamp": 1766594344 + "timestamp": 1766594799 } } diff --git a/workers/application/controllers/worker.rb b/workers/application/controllers/worker.rb index 099bd96..4afb7b3 100644 --- a/workers/application/controllers/worker.rb +++ b/workers/application/controllers/worker.rb @@ -62,7 +62,7 @@ def perform_appraisal(job, progress_mapper) folder_path: job.folder_path || '', config: Worker.config, gitrepo: gitrepo, - progress: progress_mapper.progress_callback + progress: progress_mapper ) # Keep sending finished status to any latecoming subscribers diff --git a/workers/application/services/appraise_project.rb b/workers/application/services/appraise_project.rb index 3ba9016..157dd38 100644 --- a/workers/application/services/appraise_project.rb +++ b/workers/application/services/appraise_project.rb @@ -19,7 +19,7 @@ class AppraiseProject # folder_path: String path to folder being appraised # config: Worker config with Redis URL # gitrepo: GitRepo instance - # progress: Proc to report progress (accepts symbols, not percentages) + # progress: ProgressMapper instance to report progress via symbols def prepare_inputs(input) # Convert OpenStruct project to Entity::Project for Value::Appraisal @@ -31,15 +31,15 @@ def prepare_inputs(input) end def clone_repo(input) - input[:progress].call(:started) + input[:progress].report(:started) if input[:gitrepo].exists_locally? - input[:progress].call(:cloning_done) + input[:progress].report(:cloning_done) else input[:gitrepo].clone_locally do |line| # Use CloneMapper to convert git output to progress symbol symbol = CodePraise::CloneMapper.map_or_default(line) - input[:progress].call(symbol) + input[:progress].report(symbol) end end @@ -60,13 +60,13 @@ def appraise_contributions(input) # Skip if already errored in clone step return Success(input) if input[:appraisal]&.error? - input[:progress].call(:appraising_started) + input[:progress].report(:appraising_started) folder = CodePraise::Mapper::Contributions .new(input[:gitrepo]) .for_folder(input[:folder_path]) - input[:progress].call(:appraising_done) + input[:progress].report(:appraising_done) # Build successful appraisal value object input[:appraisal] = CodePraise::Value::Appraisal.success( @@ -90,7 +90,7 @@ def appraise_contributions(input) end def cache_result(input) - input[:progress].call(:caching_started) + input[:progress].report(:caching_started) appraisal = input[:appraisal] json = CodePraise::Representer::Appraisal.new(appraisal).to_json @@ -98,13 +98,13 @@ def cache_result(input) cache = CodePraise::Cache::Remote.new(input[:config]) cache.set(appraisal.cache_key, json, ttl: appraisal.ttl) - input[:progress].call(:finished) + input[:progress].report(:finished) Success(input) rescue StandardError => e # Cache failure - still report finished so client can retry puts "CACHE ERROR: #{e.message}" - input[:progress].call(:finished) + input[:progress].report(:finished) Success(input) end diff --git a/workers/infrastructure/faye/progress_mapper.rb b/workers/infrastructure/faye/progress_mapper.rb index 101ec39..3e489de 100644 --- a/workers/infrastructure/faye/progress_mapper.rb +++ b/workers/infrastructure/faye/progress_mapper.rb @@ -42,9 +42,5 @@ def report_each_second(seconds, symbol) end end - # Returns a proc that can be passed to services for progress reporting - def progress_callback - ->(symbol) { report(symbol) } - end end end From bbd442faa6a1b4bd309163c274fb0d14edfacf3a Mon Sep 17 00:00:00 2001 From: Soumya Ray Date: Thu, 25 Dec 2025 00:55:06 +0800 Subject: [PATCH 6/6] docs: archive progress reporting planning document MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moved to .claude/_archive/ (gitignored) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- CLAUDE.refactor-progress-reporting.md | 313 -------------------------- 1 file changed, 313 deletions(-) delete mode 100644 CLAUDE.refactor-progress-reporting.md diff --git a/CLAUDE.refactor-progress-reporting.md b/CLAUDE.refactor-progress-reporting.md deleted file mode 100644 index 367c500..0000000 --- a/CLAUDE.refactor-progress-reporting.md +++ /dev/null @@ -1,313 +0,0 @@ -# Refactoring Plan: Progress Reporting Architecture (refactor-progress-reporting branch) - -## Overview - -Refactor the worker's progress reporting to use symbolic messages instead of numeric percentages, with a dedicated mapper translating symbols to percentages for Faye publication. - -## Current Architecture - -``` -Service::AppraiseProject - ↓ (numeric percentages: 15, 25, 50, 85, 100) -JobReporter.progress_callback - ↓ (pass-through) -ProgressPublisher.publish - ↓ (HTTP POST) -Faye Server → Client -``` - -**Current Issues:** - -1. `Service::AppraiseProject` is tightly coupled to percentage values (15, 25, 50, etc.) -2. `JobReporter` is just a thin wrapper - doesn't add value beyond parsing job JSON -3. Percentage mapping logic is duplicated: - - `AppraisalMonitor::PHASES` hash in `presentation/values/progress_monitor.rb` - - Inline values in `Service::AppraiseProject` (15, 25, 40, 45, 50, 55, 85, 90, 100) - - `scale_clone_progress` method also has its own mapping -4. Progress stages are not clearly named - just raw numbers -5. `CloneMonitor` is marked as "legacy" but still exists - -## Proposed Architecture - -```text -Worker Controller - ↓ (deserializes job JSON, creates ProgressMapper) -Service::AppraiseProject - ↓ (symbolic progress: :started, :clone_receiving, :appraising, etc.) - ↓ (git output lines go through CloneMapper first) -ProgressMapper (in workers/infrastructure/faye/) - ↓ (maps symbols → percentages) -FayeServer (in workers/infrastructure/faye/) - ↓ (HTTP POST) -Faye Server → Client -``` - -**Key Changes:** - -1. **Service sends symbols**: `progress.call(:started)` instead of `progress.call(15)` -2. **CloneMapper parses git output**: Converts git clone lines → symbols (`:clone_receiving`, etc.) -3. **ProgressMapper is single source of truth**: All symbol → percentage mappings in one place -4. **FayeServer is pure infrastructure**: Just HTTP POST, no business logic -5. **JobReporter eliminated**: Responsibilities split between controller (parsing) and ProgressMapper (reporting) - -## Discussion Points - -### Benefits - -1. **Service is domain-focused**: Uses meaningful symbols like `:cloning`, `:appraising` -2. **Single source of truth**: All percentage mappings in one place (ProgressMapper) -3. **Testability**: Service tests don't need to know about percentages -4. **Flexibility**: Easy to change percentage distribution without touching service -5. **Clean architecture**: Proper layer separation (domain → presentation → infrastructure) - -### Layer Placement - -**Option A: Mapper in Presentation Layer** -- `workers/presentation/mappers/progress_mapper.rb` -- Aligns with current `progress_monitor.rb` location in presentation -- Translation from domain symbols to external format is a presentation concern - -**Option B: Mapper in Application Layer** -- `workers/application/requests/progress_mapper.rb` -- Keeps request handling together (JobReporter was a request object) -- Application layer coordinates between domain and infrastructure - -**Recommendation**: Option A - Presentation layer, because: -- Mapping domain concepts to external representation IS a presentation concern -- Similar to how Representers map entities to JSON -- Infrastructure (gateway) stays pure HTTP - -### Symbol Design - -**Current numeric stages in Service::AppraiseProject:** -```ruby -clone_repo: - 15 → STARTED - 50 → Skip clone (already exists) - 25, 35, 40, 45, 50 → Clone progress stages - -appraise_contributions: - 55 → Starting appraisal - 85 → Appraisal complete - -cache_result: - 90 → Caching - 100 → FINISHED -``` - -**Proposed symbols:** -```ruby -# Main phases -:started # 15 - Worker began processing -:clone_started # 20 - Beginning clone -:clone_receiving # 40 - Receiving objects from remote -:clone_resolving # 45 - Resolving deltas -:clone_done # 50 - Clone complete (or skipped) -:appraise_started # 55 - Beginning git blame analysis -:appraise_done # 85 - Analysis complete -:cache_started # 90 - Storing in Redis -:finished # 100 - All done -``` - -### Git Clone Progress Handling - -**Current approach**: `scale_clone_progress(line)` parses git output and maps keywords: -```ruby -{ 'Cloning' => 25, 'remote' => 35, 'Receiving' => 40, 'Resolving' => 45, 'Checking' => 50 } -``` - -**Options:** - -A. **Keep in Service**: Service parses git output, sends fine-grained symbols -B. **Move to Mapper**: Service sends `:clone_progress` with raw line, mapper parses -C. **Simplify**: Only report major phases, skip line-by-line git progress - -**Recommendation**: Option C - Simplify. The line-by-line progress adds complexity without significant UX benefit. Report: `:clone_started`, `:clone_done`. - -## Implementation Phases - -### Phase 1: Restructure Faye Infrastructure ✅ - -**Rename folder**: `workers/infrastructure/messaging/` → `workers/infrastructure/faye/` - -**Rename file**: `progress_publisher.rb` → `faye_server.rb` - -- [x] Rename class `ProgressPublisher` → `FayeServer` -- [x] Keep same interface: `publish(message)` -- [x] This is pure infrastructure - no logic changes - -**New file**: `workers/infrastructure/faye/progress_mapper.rb` - -- [x] Create `Appraiser::ProgressMapper` class -- [x] Define `PHASES` hash (single source of truth for all symbol → percentage mappings) -- [x] Include all phases: `:started`, `:clone_receiving`, `:clone_resolving`, `:clone_done`, `:appraising`, `:appraise_done`, `:caching`, `:finished` -- [x] Implement `map(symbol) → percentage` method -- [x] Accept FayeServer in constructor for dependency injection -- [x] Implement `report(symbol)` that maps and publishes via FayeServer - -**New file**: `workers/infrastructure/git/mappers/clone_mapper.rb` - -- [x] Create `CloneMapper` module -- [x] Parse git clone output lines into symbols -- [x] Map: `"Receiving..."` → `:clone_receiving`, `"Resolving..."` → `:clone_resolving`, etc. -- [x] Return symbol that ProgressMapper can translate to percentage - -### Phase 2: Update Service to Use Symbols ✅ - -**Modify**: `workers/application/services/appraise_project.rb` - -- [x] Change all `input[:progress].call(15)` → `input[:progress].call(:started)` -- [x] Remove `scale_clone_progress` method -- [x] Use CloneMapper to convert git output lines to symbols -- [x] Use symbols throughout: `:started`, `:cloning_receiving`, `:cloning_done`, `:appraising_started`, etc. -- [x] Update tests: Replace `scale_clone_progress` tests with `CloneMapper` tests - -### Phase 3: Update Worker Controller ✅ - -**Modify**: `workers/application/controllers/worker.rb` - -- [x] Move job JSON deserialization from JobReporter to controller -- [x] Create ProgressMapper with FayeServer and channel_id -- [x] Pass `progress_mapper.progress_callback` to service -- [x] Update `report_each_second` logic to use ProgressMapper with `:finished` symbol - -### Phase 4: Cleanup ✅ - -- [x] Delete `workers/application/requests/job_reporter.rb` -- [x] Delete `workers/presentation/values/progress_monitor.rb` - - `CloneMonitor` module (marked as legacy) - - `AppraisalMonitor` module (replaced by ProgressMapper) -- [x] `require_worker.rb` already updated in Phase 1 -- [x] Tests updated in Phase 2 (CloneMapper tests replace scale_clone_progress tests) - -## Open Questions - -1. ~~Should we keep fine-grained git clone progress, or simplify to just start/done?~~ **DECIDED: Keep fine-grained via CloneMapper** -2. ~~JobReporter: combine with ProgressMapper, or keep separate?~~ **DECIDED: Eliminate JobReporter, merge reporting into ProgressMapper** -3. ~~Should ProgressMapper be in presentation or application layer?~~ **DECIDED: Infrastructure (with FayeServer)** -4. ~~Keep AppraisalMonitor::PHASES as the source of truth, or define in ProgressMapper?~~ **DECIDED: ProgressMapper is the single source of truth** - -All open questions resolved! - ---- - -## Decisions Made - -### Decision 1: Infrastructure Gateway Naming - -- Rename `ProgressPublisher` → `FayeServer` -- File: `workers/infrastructure/messaging/progress_publisher.rb` → `faye_server.rb` -- **Rationale**: "Server" better describes the role as an infrastructure gateway to the Faye server - -### Decision 2: Faye Infrastructure Organization - -- Rename folder: `workers/infrastructure/messaging/` → `workers/infrastructure/faye/` -- Place both `FayeServer` (gateway) and `ProgressMapper` (mapper) in this folder -- Structure: - - ```text - workers/infrastructure/faye/ - ├── faye_server.rb # Gateway - HTTP POST to Faye - └── progress_mapper.rb # Mapper - symbols → percentages - ``` - -- **Rationale**: Faye is infrastructure; the mapper translates domain symbols for this specific infrastructure concern. Keeping gateway + mapper together follows the pattern in `workers/infrastructure/git/` (gateway + mappers) - -### Decision 4: Eliminate JobReporter - -- Delete `workers/application/requests/job_reporter.rb` -- Move job JSON deserialization to Worker controller (or service's first step) -- Move `report()`, `report_each_second()`, `progress_callback()` to ProgressMapper -- **Rationale**: JobReporter has no distinct responsibility: - - Its reporting methods belong in ProgressMapper - - Its JSON deserialization is trivial (3 lines) and can live in controller/service - -**Before:** -```text -Worker.perform(request_json) - ↓ -JobReporter.new(request_json, config) # parses + creates publisher - ↓ -Service::AppraiseProject.call(progress: job.progress_callback) -``` - -**After:** -```text -Worker.perform(request_json) - ↓ -Deserialize job (in controller or service step) - ↓ -ProgressMapper.new(config, channel_id) # created with FayeServer - ↓ -Service::AppraiseProject.call(progress: mapper.progress_callback) -``` - -### Decision 5: Symbol Naming Convention - -- Use `-ing` suffix for ongoing action phases: `cloning_*`, `appraising_*`, `caching_*` -- Symbols: `:started`, `:cloning_started`, `:cloning_remote`, `:cloning_receiving`, `:cloning_resolving`, `:cloning_done`, `:appraising_started`, `:appraising_done`, `:caching_started`, `:finished` -- **Rationale**: Consistent naming that describes ongoing actions; more readable and descriptive - -### Decision 3: Git Clone Progress with CloneMapper - -- Create `CloneMapper` to parse git clone output lines into symbols -- Location: `workers/infrastructure/git/mappers/clone_mapper.rb` (alongside other git mappers) -- `CloneMapper` converts git output → symbols (`:clone_receiving`, `:clone_resolving`, etc.) -- `ProgressMapper` remains the single source of truth for all symbol → percentage mappings -- Flow: - - ```text - Git clone output line (e.g., "Receiving objects: 50%...") - ↓ - CloneMapper.map(line) → :clone_receiving - ↓ - ProgressMapper.report(:clone_receiving) → 40% - ↓ - FayeServer.publish("40") - ``` - -- **Rationale**: Separates concerns - git parsing stays with git infrastructure, percentage mapping stays centralized in ProgressMapper - ---- - -## Session Log - -### Session 1 - -- Initial discussion of refactoring goals -- Created this planning document -- Reviewed current architecture: - - `workers/application/requests/job_reporter.rb` - thin wrapper around ProgressPublisher - - `workers/infrastructure/messaging/progress_publisher.rb` - HTTP POST to Faye - - `workers/application/services/appraise_project.rb` - uses numeric percentages - - `workers/presentation/values/progress_monitor.rb` - CloneMonitor (legacy) + AppraisalMonitor -- Identified issues: - - Duplicate percentage mappings in multiple places - - Service coupled to numeric values - - JobReporter adds little value -- **Status**: Initial planning, awaiting user feedback on proposed approach - ---- - -## Notes - -### Post-Implementation Cleanup - -After final implementation and testing: - -- [ ] Add `CLAUDE.refactor-progress-reporting.md` to `.gitignore` -- [ ] Move this file to `.claude/_archive/CLAUDE.refactor-progress-reporting.md` - -### Commit Practices - -- **Summarize changes before requesting commit permission** - provide brief summary by folder/file for user review BEFORE asking to commit -- **Separate concerns into distinct commits** - e.g., infrastructure rename vs service logic changes -- **User is author, Claude is co-author** - use `Co-Authored-By: Claude ` -- **Use conventional commit messages** - `feat:`, `fix:`, `refactor:`, `docs:`, etc. - - `refactor:` for internal changes (this refactoring) - - `feat:` only for changes to external API/service features - -### Implementation Practices - -- **Seek consent before moving to next phase** - summarize completed work, commit, then ask to proceed -- **Include coverage report in commits** - after successful tests, amend `coverage/.resultset.json` to coding commits