Replace manual batch loops with declarative MapTask from @workglow/* … - #60
Conversation
There was a problem hiding this comment.
Pull request overview
This PR upgrades several SEC ingestion/update tasks to use Workflow.map()/endMap() (from @workglow/task-graph v0.0.103) instead of hand-rolled batching/loop orchestration, aiming to reduce boilerplate and standardize concurrency behavior.
Changes:
- Replaced manual loop/batch patterns with
Workflow.map()in submissions, facts, forms, and quarterly index range tasks. - Introduced per-task concurrency limits via
map({ concurrencyLimit: ... })to replace prior batch sizes / parallelPromise.allpatterns. - Adjusted quarterly index range aggregation logic to work with mapped results rather than
TaskGraphresults.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/task/submissions/UpdateAllSubmissionsTask.ts |
Refactors update + initial processing loops to Workflow.map() with concurrency limits and a shared mapped handler. |
src/task/index/FetchQuarterlyIndexRangeTask.ts |
Replaces TaskGraph fan-out with Workflow.map() over computed quarter start dates and merges mapped outputs. |
src/task/forms/UpdateAllFormsTask.ts |
Replaces batched Promise.all processing of missing forms with Workflow.map({ concurrencyLimit: 10 }). |
src/task/forms/FetchAndStoreFormsTask.ts |
Fixes prior sequential workflow piping by mapping filings into ProcessAccessionDocFormTask with concurrency. |
src/task/facts/UpdateAllCompanyFactsTask.ts |
Refactors update + initial processing loops to Workflow.map() and adds a mapped fetch/store handler with concurrency limits. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -132,4 +106,5 @@ async function runWorkflow(wf: IWorkflow<any, any>, input: { cik: string; date: | |||
| } | |||
| ); | |||
There was a problem hiding this comment.
processed_facts is created with a success bool not null column (see src/util/db.ts), but this insert only provides cik and last_processed. On a fresh DB this will violate the NOT NULL constraint and the failure-handling path will throw, masking the original error. Include a $success value (likely false) in the insert, or update the table schema to match what the code writes.
| const needsUpating = query_all<{ | ||
| cik: string; | ||
| last_update: string; | ||
| last_processed: string; | ||
| }>(` | ||
| SELECT cik_last_update.cik, cik_last_update.last_update, processed_submissions.last_processed FROM cik_last_update | ||
| JOIN processed_submissions | ||
| ON cik_last_update.cik = processed_submissions.cik | ||
| WHERE cik_last_update.last_update > processed_submissions.last_processed | ||
| WHERE cik_last_update.last_update > processed_submissions.last_processed | ||
| ORDER BY cik_last_update.last_update DESC`); | ||
| const needsUpatingCount = needsUpating?.length ?? 0; | ||
|
|
There was a problem hiding this comment.
The variable name needsUpating looks like a typo (missing 'd' in 'Updating'). Since it’s used throughout this method, consider renaming it (and the associated count variables) to needsUpdating for clarity and to avoid propagating the misspelling.
| const needsUpating = query_all<{ | ||
| cik: string; | ||
| last_update: string; | ||
| last_processed: string; | ||
| }>(` | ||
| SELECT cik_last_update.cik, cik_last_update.last_update, processed_facts.last_processed FROM cik_last_update | ||
| JOIN processed_facts | ||
| ON cik_last_update.cik = processed_facts.cik | ||
| WHERE cik_last_update.last_update > processed_facts.last_processed | ||
| WHERE cik_last_update.last_update > processed_facts.last_processed | ||
| ORDER BY cik_last_update.last_update DESC`); | ||
| const needsUpatingCount = needsUpating?.length ?? 0; | ||
|
|
There was a problem hiding this comment.
The variable name needsUpating looks like a typo (missing 'd' in 'Updating'). Consider renaming it (and the derived needsUpatingCount) to needsUpdating/needsUpdatingCount to improve readability.
…v0.0.103 Leverage Workflow.map()/endMap() to replace hand-rolled TaskGraph loops, batched Promise.all patterns, and sequential pipe chains across 5 task files. This reduces boilerplate, fixes an unintended sequential execution bug in FetchAndStoreFormsTask, and standardizes concurrency control. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…v0.0.103
Leverage Workflow.map()/endMap() to replace hand-rolled TaskGraph loops, batched Promise.all patterns, and sequential pipe chains across 5 task files. This reduces boilerplate, fixes an unintended sequential execution bug in FetchAndStoreFormsTask, and standardizes concurrency control.