Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a31aecb435
ℹ️ 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".
Contributor
🚀 Preview DeploymentYour documentation preview is ready! Preview URL: https://pr-126.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 654b3cf |
luandro
added a commit
that referenced
this pull request
Feb 8, 2026
- Use Notion pages.retrieve API directly in fetchPage instead of invalid database query filter on non-existent "id" property - Add process tracking to job cancellation so DELETE handler actually kills running child processes and prevents status overwrite - Delete persisted job files during cleanup to prevent old jobs from reappearing after restart
luandro
changed the title
luandro
changed the title
luandro
added a commit
that referenced
this pull request
Feb 9, 2026
luandro
added a commit
that referenced
this pull request
Feb 9, 2026
- PRD.md: comprehensive review checklist with 16 tasks covering security, architecture, testing, and production readiness - PRD_DOCKER_IMAGE.md: specification for Docker Hub GitHub Action workflow with multi-platform support, path filtering, and PR preview tags
luandro
added a commit
that referenced
this pull request
Feb 9, 2026
- API_REVIEW.md: 16-task review plan for PR #126 with complexity levels and priority ordering. Covers security, architecture, reliability, and production readiness concerns - TEST_IMPROVEMENT.md: comprehensive test improvement plan addressing 20 failing tests, code duplication issues, and missing HTTP integration coverage
Contributor
🐳 Docker Image PublishedYour Docker image has been built and pushed for this PR. Image Reference: Platforms: linux/amd64, linux/arm64 TestingTo test this image: docker pull docker.io/communityfirst/comapeo-docs-api:pr-126
docker run -p 3001:3001 docker.io/communityfirst/comapeo-docs-api:pr-126Built with commit 654b3cf |
Contributor
Author
✅ PR #129 Merge Readiness Checklist (updated)Review findings / correctness
Docker / env completeness
Final validation
|
luandro
added a commit
that referenced
this pull request
Feb 13, 2026
- Use Notion pages.retrieve API directly in fetchPage instead of invalid database query filter on non-existent "id" property - Add process tracking to job cancellation so DELETE handler actually kills running child processes and prevents status overwrite - Delete persisted job files during cleanup to prevent old jobs from reappearing after restart
luandro
added a commit
that referenced
this pull request
Feb 13, 2026
- PRD.md: comprehensive review checklist with 16 tasks covering security, architecture, testing, and production readiness - PRD_DOCKER_IMAGE.md: specification for Docker Hub GitHub Action workflow with multi-platform support, path filtering, and PR preview tags
luandro
added a commit
that referenced
this pull request
Feb 13, 2026
- API_REVIEW.md: 16-task review plan for PR #126 with complexity levels and priority ordering. Covers security, architecture, reliability, and production readiness concerns - TEST_IMPROVEMENT.md: comprehensive test improvement plan addressing 20 failing tests, code duplication issues, and missing HTTP integration coverage
luandro
force-pushed
the
feat/notion-api-service
branch
from
f451b74 to
0d2004b
Compare
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: 1 New Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Previously Flagged Issues (from earlier reviews)The following issues were identified in earlier reviews:
Files Reviewed (multiple files)
|
luandro
added a commit
that referenced
this pull request
Feb 20, 2026
- Use Notion pages.retrieve API directly in fetchPage instead of invalid database query filter on non-existent "id" property - Add process tracking to job cancellation so DELETE handler actually kills running child processes and prevents status overwrite - Delete persisted job files during cleanup to prevent old jobs from reappearing after restart
luandro
force-pushed
the
feat/notion-api-service
branch
from
86c30bb to
a151429
Compare
luandro
added a commit
that referenced
this pull request
Feb 20, 2026
- PRD.md: comprehensive review checklist with 16 tasks covering security, architecture, testing, and production readiness - PRD_DOCKER_IMAGE.md: specification for Docker Hub GitHub Action workflow with multi-platform support, path filtering, and PR preview tags
luandro
added a commit
that referenced
this pull request
Feb 20, 2026
- API_REVIEW.md: 16-task review plan for PR #126 with complexity levels and priority ordering. Covers security, architecture, reliability, and production readiness concerns - TEST_IMPROVEMENT.md: comprehensive test improvement plan addressing 20 failing tests, code duplication issues, and missing HTTP integration coverage
luandro
force-pushed
the
feat/notion-api-service
branch
from
574c7e9 to
ba4e040
Compare
- Fix axios mock in imageProcessing.test.ts to return proper response objects - Fix node:fs mock to use proper factory functions - Fix typo: bun.lockb* -> bun.lock* in docker-publish-workflow.test.ts
…nc audit logging - Replace all sync node:fs calls in job-persistence.ts with async node:fs/promises equivalents (readFile, writeFile, appendFile, access, mkdir, stat, rename, unlink); add write-serialization mutex (logWritePromise) and waitForPendingWrites() for tests - Change jobOptionsSchema from .strip() to .strict() so unknown keys (e.g. typo drRun) are rejected rather than silently dropped - Fix audit.ts to use self-contained sync rotation (private rotateIfNeeded()) and synchronous appendFileSync, keeping log() fully synchronous for test compatibility; waitForPendingWrites() now returns Promise.resolve() as a compatibility no-op - Update all callers in job-tracker.ts and all test files to await the now-async job-persistence functions; fix Zod type narrowing in validation-schemas.test.ts Relates to: GitHub issue #150 (async-mutex follow-up tracked separately)
- Add loadPromise and waitForLoad() to JobTracker for async loading - Fix 6 tests that accessed jobs before persistence loading completed - Fix 2 tests expecting strict() schema to pass with unknown keys The JobTracker was calling loadPersistedJobs() without tracking the promise, causing race conditions when tests destroyed/recreated the tracker and immediately accessed jobs.
- Add envOverrides parameter to runJobProcess for workdir-specific paths - Pass CONTENT_PATH, I18N_PATH, IMAGES_PATH when running via runContentTask - Add detached: true and unref() to enable proper process group cleanup 🤖 Generated with [Qoder][https://qoder.com]
- Add SIGTERM -> SIGKILL escalation pattern matching job-executor.ts - Wait 5s after SIGTERM before escalating to SIGKILL - Add 1s fail-safe before rejecting promise - Track processExited to prevent double-rejection 🤖 Generated with [Qoder][https://qoder.com]
- Import fetchNotionBlocks to fetch page blocks - Import n2m (notion-to-md) for markdown conversion - Fetch blocks and convert to markdown string - Return content field with full markdown 🤖 Generated with [Qoder][https://qoder.com]
- Add unref: vi.fn() to mock child processes - Supports testing with detached: true spawn option 🤖 Generated with [Qoder][https://qoder.com]
…eout - Register abort controller as kill handler for fetch jobs so DELETE /jobs/:id properly cancels the running fetch via AbortSignal, which in turn triggers the SIGTERM→SIGKILL chain in runGenerationScript - Add detached: true to spawn in runGenerationScript so process group kills (process.kill(-pid)) work correctly and grandchildren (e.g. pngquant workers) are reaped on timeout or cancellation
…te header
- Remove in-process fetchAllNotionData preflight from runFetchJob; the
CLI script (notion-fetch-all) already fetches the full database, so
calling it twice doubled all Notion API requests and rate-limit exposure
- notion-fetch-all/index.ts now emits a JSON summary line to stdout
({candidateIds, pagesProcessed}) before exit; fetch-job-runner reads it
via extractLastJsonLine to obtain candidateIds and page count
- Remove the now-dead withTimeout helper from fetch-job-runner.ts
- Add missing WWW-Authenticate: Bearer header to both 401 response
branches in request-handler.ts, matching createAuthErrorResponse and
satisfying RFC 7235
- fix(api-server/openapi): use createJobRequestSchema (object) instead of createJobTypeSchema (string union) for POST /jobs request body schema - fix(api-server/content-repo): replace hardcoded "content"/"origin/content" strings in ensureRemoteContentBranchExists, pushContentBranchWithRetry, and verifyRemoteHeadMatchesLocal with config.contentBranch so GITHUB_CONTENT_BRANCH overrides are respected - fix(scripts/notion-count-pages): mirror applyFetchAllTransform logic from fetchAll.ts by using resolveChildrenByStatus instead of a direct status equality filter, so child pages of matching parents are counted correctly - fix(api-server/audit): convert synchronous file I/O (appendFileSync, statSync, renameSync, unlinkSync) to async operations via a sequential write queue; implement waitForPendingWrites() to drain the queue on shutdown
rmdirSync fails on non-empty directories; rmSync with recursive+force handles that case correctly. Also removes the now-unused rmdirSync import.
…ore job creation - Replace O(N) hash-per-key loop in authenticate() with a single SHA-256 hash computed once, then an O(1) Map.get() lookup against stored key hashes - Remove now-redundant verifyKey()/timingSafeEqual() helper; the Map lookup already confirms hash equality by construction - Add validateContentRepoConfig() to content-repo.ts to surface config errors at a defined boundary rather than as unhandled exceptions - Pre-validate content repo config in handleCreateJob() before acquiring locks or spawning processes, returning HTTP 500 with a descriptive message on failure - Restore process.env after each content-repo test to prevent env variable leakage between test cases
- Strip GitHub token from persisted jobs before writing to disk; empty string placeholder preserves the field type without leaking secrets - Replace hardcoded production IP in SETUP.md with a generic note to avoid exposing infrastructure details in the repository
- Route startup warning to stderr with a more descriptive message clarifying that all endpoints are publicly accessible - Add X-Auth-Disabled response header on non-public endpoints when running without credentials, so misconfiguration is visible to clients
Add cross-field superRefine validation so startup fails with a clear error when neither database identifier is provided, rather than proceeding silently with no content source configured.
Replace hardcoded /app/workspace/repo with ${WORKDIR:-/app/workspace/repo}
so git operations work when operators customize the WORKDIR path.
Add prominent WARNING comment in docker-compose.yml explaining that the API is fully open when no API_KEY_* variables are set. Co-authored-by: Junie <junie@jetbrains.com>
- fix: make ApiKeyAuth constructor private; add static resetInstance() for tests - fix: enforce MAX_REQUEST_SIZE via content-length header check (413 response) - fix: add PAYLOAD_TOO_LARGE to ErrorCode enum and getErrorCodeForStatus map - fix: add --watch flag to api:server:dev script for hot-reload - fix: add ALLOWED_ORIGINS to docker-compose.yml environment section - fix: expand api-validate.yml trigger paths to include api-server/** and package.json - chore: remove implementation-plan-fetch-ready.md from repo root
…English pages Add getEnglishTitle() to pageGrouping.ts and use it in generateBlocks.ts so that code.json entries are keyed by the English original title rather than a potentially non-English mainTitle, matching Docusaurus @translate expectations. Silently skips translation entries when no English page exists. Also fix createMockPageFamily() to pre-generate and link child IDs via subItems so groupPagesByLang can properly relate pages in tests.
luandro
force-pushed
the
feat/notion-api-service
branch
from
6efa332 to
2b62949
Compare
… fetch-ready runs
The gitleaks pre-commit hook is not available in CI environment, causing the workflow to fail. Using --no-verify is standard practice for automated commits in CI/CD pipelines.
Contributor
🧹 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
solves #120 but using a VPS instead of CloudFlare workers.
Summary
Testing