feat(api): replace api-notion-fetch with api-native fetch jobs#143
feat(api): replace api-notion-fetch with api-native fetch jobs#143luandro merged 12 commits intofeat/notion-api-servicefrom
Conversation
Sync content with origin/main before any generated-content commit in clean-content and translate-docs workflows. Replace workflow_run trigger in deploy-staging with push-based detection on content/main branches, adding bash logic to skip deploys when no relevant paths changed. Also fix fromJSON string workarounds in translate-docs with format().
💡 Codex ReviewProtected routes are authenticated before routing, but comapeo-docs/api-server/fetch-job-runner.ts Lines 361 to 364 in c5a09bb The fetch-ready flow derives Line 16 in c5a09bb The image build copies ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge OverviewThe PR implements API-native fetch jobs (
Files Reviewed (43 files)
Prior Issues StatusAll previously reported issues have been addressed:
|
🚀 Preview DeploymentYour documentation preview is ready! Preview URL: https://pr-143.comapeo-docs.pages.dev 🔄 Content: Regenerated 5 pages from Notion (script changes detected)
This preview will update automatically when you push new commits to this PR. Built with commit 32209e9 |
- Skip auth for OPTIONS requests to allow CORS preflight without credentials - Track ready-to-publish pages for status transitions before child replacement - Fix bun.lock pattern in Dockerfile (bun.lockb* → bun.lock*) Fixes P1: CORS preflight receiving 401 instead of 204 Fixes P1: Empty transitionCandidates when children replace parents Fixes P2: Missing lockfile in Docker builds
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ce70263f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Use config.contentBranch instead of hardcoded "content" in git fetch - Add withTimeout wrapper to enforce timeout during Notion fetch phase Fixes P1: prepareContentBranchForFetch fails when GITHUB_CONTENT_BRANCH is non-default Fixes P1: fetchAllNotionData ignores JOB_TIMEOUT_MS when Notion API stalls
…ests - Modified fetchAllNotionData to return candidateIds (original 'Ready to publish' IDs) - Updated fetch-job-runner to use candidateIds for transitions, ensuring parents are transitioned even if replaced by children - Fixed memory leak in withTimeout by clearing the timer - Stabilized persistence tests by adding waitForPendingWrites() before tracker destruction - Fixed out-of-sync API documentation validation tests - Prevented environment variable interference in auth tests by clearing keys in beforeEach - Allowed fallback values in GitHub Actions secret handling tests - Made log rotation tests async and properly awaited persistence calls
This addresses PR 143 review feedback by replacing UNKNOWN with SERVER_RESTART_ABORT, handling in-flight state gracefully, and documenting API fetching mechanics. Co-authored-by: Junie <junie@jetbrains.com>
…tion Co-authored-by: Junie <junie@jetbrains.com>
🧹 Preview Deployment CleanupThe preview deployment for this PR has been cleaned up. Preview URL was: Note: Cloudflare Pages deployments follow automatic retention policies. Old previews are cleaned up automatically. |
* ci(workflows): enforce content-branch sync and smarter deploy trigger Sync content with origin/main before any generated-content commit in clean-content and translate-docs workflows. Replace workflow_run trigger in deploy-staging with push-based detection on content/main branches, adding bash logic to skip deploys when no relevant paths changed. Also fix fromJSON string workarounds in translate-docs with format(). * feat(api): implement fetch-ready/fetch-all runtime contracts and safety semantics * ci(workflows): add api-validate workflow and remove legacy api-notion-fetch * docs(fetch): document api-native fetch model and rollout validation * fix(ci): harden api-validate auth handling * fix(ci): set content-repo env for api-validate workflow * fix(api-server): address codex review feedback - Skip auth for OPTIONS requests to allow CORS preflight without credentials - Track ready-to-publish pages for status transitions before child replacement - Fix bun.lock pattern in Dockerfile (bun.lockb* → bun.lock*) Fixes P1: CORS preflight receiving 401 instead of 204 Fixes P1: Empty transitionCandidates when children replace parents Fixes P2: Missing lockfile in Docker builds * fix(api-server): address more codex review feedback - Use config.contentBranch instead of hardcoded "content" in git fetch - Add withTimeout wrapper to enforce timeout during Notion fetch phase Fixes P1: prepareContentBranchForFetch fails when GITHUB_CONTENT_BRANCH is non-default Fixes P1: fetchAllNotionData ignores JOB_TIMEOUT_MS when Notion API stalls * fix(api-server): track ready-to-publish pages for status transitions correctly * fix(api-server): correct fetch-ready transition logic and stabilize tests - Modified fetchAllNotionData to return candidateIds (original 'Ready to publish' IDs) - Updated fetch-job-runner to use candidateIds for transitions, ensuring parents are transitioned even if replaced by children - Fixed memory leak in withTimeout by clearing the timer - Stabilized persistence tests by adding waitForPendingWrites() before tracker destruction - Fixed out-of-sync API documentation validation tests - Prevented environment variable interference in auth tests by clearing keys in beforeEach - Allowed fallback values in GitHub Actions secret handling tests - Made log rotation tests async and properly awaited persistence calls * fix(api-server): handle job aborts correctly on server restart This addresses PR 143 review feedback by replacing UNKNOWN with SERVER_RESTART_ABORT, handling in-flight state gracefully, and documenting API fetching mechanics. * fix(api-server): handle edge cases, enhance error handling and validation
* ci(workflows): enforce content-branch sync and smarter deploy trigger Sync content with origin/main before any generated-content commit in clean-content and translate-docs workflows. Replace workflow_run trigger in deploy-staging with push-based detection on content/main branches, adding bash logic to skip deploys when no relevant paths changed. Also fix fromJSON string workarounds in translate-docs with format(). * feat(api): implement fetch-ready/fetch-all runtime contracts and safety semantics * ci(workflows): add api-validate workflow and remove legacy api-notion-fetch * docs(fetch): document api-native fetch model and rollout validation * fix(ci): harden api-validate auth handling * fix(ci): set content-repo env for api-validate workflow * fix(api-server): address codex review feedback - Skip auth for OPTIONS requests to allow CORS preflight without credentials - Track ready-to-publish pages for status transitions before child replacement - Fix bun.lock pattern in Dockerfile (bun.lockb* → bun.lock*) Fixes P1: CORS preflight receiving 401 instead of 204 Fixes P1: Empty transitionCandidates when children replace parents Fixes P2: Missing lockfile in Docker builds * fix(api-server): address more codex review feedback - Use config.contentBranch instead of hardcoded "content" in git fetch - Add withTimeout wrapper to enforce timeout during Notion fetch phase Fixes P1: prepareContentBranchForFetch fails when GITHUB_CONTENT_BRANCH is non-default Fixes P1: fetchAllNotionData ignores JOB_TIMEOUT_MS when Notion API stalls * fix(api-server): track ready-to-publish pages for status transitions correctly * fix(api-server): correct fetch-ready transition logic and stabilize tests - Modified fetchAllNotionData to return candidateIds (original 'Ready to publish' IDs) - Updated fetch-job-runner to use candidateIds for transitions, ensuring parents are transitioned even if replaced by children - Fixed memory leak in withTimeout by clearing the timer - Stabilized persistence tests by adding waitForPendingWrites() before tracker destruction - Fixed out-of-sync API documentation validation tests - Prevented environment variable interference in auth tests by clearing keys in beforeEach - Allowed fallback values in GitHub Actions secret handling tests - Made log rotation tests async and properly awaited persistence calls * fix(api-server): handle job aborts correctly on server restart This addresses PR 143 review feedback by replacing UNKNOWN with SERVER_RESTART_ABORT, handling in-flight state gracefully, and documenting API fetching mechanics. * fix(api-server): handle edge cases, enhance error handling and validation
* ci(workflows): enforce content-branch sync and smarter deploy trigger Sync content with origin/main before any generated-content commit in clean-content and translate-docs workflows. Replace workflow_run trigger in deploy-staging with push-based detection on content/main branches, adding bash logic to skip deploys when no relevant paths changed. Also fix fromJSON string workarounds in translate-docs with format(). * feat(api): implement fetch-ready/fetch-all runtime contracts and safety semantics * ci(workflows): add api-validate workflow and remove legacy api-notion-fetch * docs(fetch): document api-native fetch model and rollout validation * fix(ci): harden api-validate auth handling * fix(ci): set content-repo env for api-validate workflow * fix(api-server): address codex review feedback - Skip auth for OPTIONS requests to allow CORS preflight without credentials - Track ready-to-publish pages for status transitions before child replacement - Fix bun.lock pattern in Dockerfile (bun.lockb* → bun.lock*) Fixes P1: CORS preflight receiving 401 instead of 204 Fixes P1: Empty transitionCandidates when children replace parents Fixes P2: Missing lockfile in Docker builds * fix(api-server): address more codex review feedback - Use config.contentBranch instead of hardcoded "content" in git fetch - Add withTimeout wrapper to enforce timeout during Notion fetch phase Fixes P1: prepareContentBranchForFetch fails when GITHUB_CONTENT_BRANCH is non-default Fixes P1: fetchAllNotionData ignores JOB_TIMEOUT_MS when Notion API stalls * fix(api-server): track ready-to-publish pages for status transitions correctly * fix(api-server): correct fetch-ready transition logic and stabilize tests - Modified fetchAllNotionData to return candidateIds (original 'Ready to publish' IDs) - Updated fetch-job-runner to use candidateIds for transitions, ensuring parents are transitioned even if replaced by children - Fixed memory leak in withTimeout by clearing the timer - Stabilized persistence tests by adding waitForPendingWrites() before tracker destruction - Fixed out-of-sync API documentation validation tests - Prevented environment variable interference in auth tests by clearing keys in beforeEach - Allowed fallback values in GitHub Actions secret handling tests - Made log rotation tests async and properly awaited persistence calls * fix(api-server): handle job aborts correctly on server restart This addresses PR 143 review feedback by replacing UNKNOWN with SERVER_RESTART_ABORT, handling in-flight state gracefully, and documenting API fetching mechanics. * fix(api-server): handle edge cases, enhance error handling and validation
Summary
fetch-ready/fetch-alljob model implemented inapi-server/api-notion-fetch.ymlremoved after smoke validationTesting
Commands run:
All tests passed (92 tests total).
Scripts run:
scripts/ci-validation/vps-fetch-smoke.sh- Created for VPS dry-run smoke validationReviewer notes
Residual risks
api-validate.ymlpost-PR should merge to feat/notion-api-service not mainGreptile Summary
Replaces the GitHub Actions workflow
api-notion-fetch.ymlwith API-nativefetch-readyandfetch-alljob types running inside the API server. This refactoring moves Notion fetch orchestration from CI workflows into the API service itself, providing deterministic single-process execution with proper timeout handling, git branch management, and status transitions.Major changes:
fetch-job-runner.tsimplements the core fetch-ready/fetch-all execution flow with timeout protection, exponential retry for Notion API calls, and automatic status transitions from "Ready to publish" to "Draft published"fetch-job-lock.tsprovides in-memory lock with TTL to prevent concurrent fetch jobscontent-repo.tsadds extensive git operations for content branch preparation, merge conflict handling, staging, committing, and push retry logicjob-tracker.tsnow tracks terminal state for fetch jobs and handles in-flight job recovery on server restartroutes/jobs.tsintegrates lock checking during job creation and returns simplified response schemas for fetch jobsapi-notion-fetch.ymlworkflow deletedapi-validate.ymlworkflow added for CI smoke testingDesign highlights:
origin/contentbranch prevents race conditionsorigin/mainintocontentbefore every fetch to ensure consistencyAbortSignalandwithTimeoutwrapperConfidence Score: 4/5
fetch-job-runner.tsthat should be addressed, but they don't block merging to the feature branch. The extensive test coverage and careful handling of git operations, retries, and status transitions demonstrate thorough engineering.api-server/fetch-job-runner.tsfor the two timeout-related issues already noted in previous commentsImportant Files Changed
Sequence Diagram
sequenceDiagram participant Client participant API as API Server participant Lock as Fetch Job Lock participant Tracker as Job Tracker participant Runner as Fetch Job Runner participant Notion as Notion API participant Git as Content Repo Client->>API: POST /jobs (fetch-ready) API->>Lock: isFetchJobLockHeld()? Lock-->>API: false API->>Tracker: createJob(fetch-ready) Tracker-->>API: jobId API->>Lock: tryAcquireFetchJobLock(jobId) Lock-->>API: true (acquired) API-->>Client: 202 {jobId, status: pending} API->>Runner: runFetchJob (async) Runner->>Notion: fetchAllNotionData (Ready to publish) Notion-->>Runner: pages + candidateIds Runner->>Git: prepareContentBranch (merge main->content) Git-->>Runner: remoteRef Runner->>Runner: runGenerationScript (spawn bun) Runner->>Git: copyGeneratedContent + stage Runner->>Git: hasStagedChanges? Git-->>Runner: true Runner->>Git: commit + push Git-->>Runner: commitHash Runner->>Notion: updatePageStatus (Draft published) Notion-->>Runner: success Runner->>Tracker: setTerminalState Runner->>Lock: releaseFetchJobLock(jobId) Client->>API: GET /jobs/{jobId} API->>Tracker: getJob(jobId) Tracker-->>API: job with terminal state API-->>Client: 200 {status: completed, pagesTransitioned, commitHash}Last reviewed commit: 32209e9