test(adapters): enforce PR-39 activity fallback aliases#100
Merged
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds validation and tests for by-edited alias emission, updates GitHub lifecycle timestamp selection, expands Notion discovery with per-page schemas/examples and docs, modifies Notion resources and writeback routing/markdown parsing, and adjusts related verification and generation scripts. ChangesBy-edited alias support and validation
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/notion/src/__tests__/emit-auxiliary-files.test.ts (1)
570-709: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd collision test for by-edited alias subtree.
The coding guidelines require a collision test for each alias subtree. The by-edited alias was added in this PR but lacks a collision test. Two pages with the same
lastEditedTimedate (e.g.,'2026-05-12') should produce distinct by-edited alias paths via collision-suffix logic keyed on the page UUID.Suggest adding a test similar to the existing collision tests (lines 570-709) that verifies two pages edited on the same date produce non-colliding by-edited alias paths.
As per coding guidelines: "Each alias subtree needs a collision test".
📋 Proposed collision test structure
+ it('keeps distinct by-edited paths for same-date pages with different UUIDs', async () => { + // Two pages edited on the same date must produce distinct by-edited + // alias paths via `aliasShortId` keyed on the page UUID. + const pageAlpha = '11111111-1111-1111-1111-aaaaaaaaaaaa'; + const pageBeta = '22222222-2222-2222-2222-bbbbbbbbbbbb'; + const client = createClient(); + await emitNotionAuxiliaryFiles(client, { + workspaceId: 'ws-1', + pages: [ + { + id: pageAlpha, + title: 'Page A', + lastEditedTime: '2026-05-12T09:00:00.000Z', + }, + { + id: pageBeta, + title: 'Page B', + lastEditedTime: '2026-05-12T15:30:00.000Z', + }, + ], + }); + + const writtenPaths = new Set(client.writes.map((w) => w.path)); + const aliasA = notionByEditedAliasPath(PAGES_SCOPE, '2026-05-12', pageAlpha); + const aliasB = notionByEditedAliasPath(PAGES_SCOPE, '2026-05-12', pageBeta); + assert.notEqual(aliasA, aliasB); + assert.ok(writtenPaths.has(aliasA)); + assert.ok(writtenPaths.has(aliasB)); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/notion/src/__tests__/emit-auxiliary-files.test.ts` around lines 570 - 709, Add a collision test that mirrors the existing alias-subtree tests: call emitNotionAuxiliaryFiles with two page objects that share the same lastEditedTime (e.g., '2026-05-12') but have different UUIDs, then compute their by-edited alias paths using notionPageByEditedAliasPath and assert the two paths are not equal and that both paths were written by the test client (createClient and client.writes). Ensure the test uses distinct inline UUIDs whose trailing 8 hex differ so aliasShortId produces different suffixes and follow the same assert.notEqual / assert.ok(writtenPaths.has(...)) pattern as the other collision tests.
🧹 Nitpick comments (1)
scripts/digest-layout-contracts.mjs (1)
95-100: ⚡ Quick winAvoid hardcoding fixture dates in fallback contract needles.
Line 99 couples the contract to a specific date (
2026-05-12), which will cause false failures when test fixture dates change without behavior changes. Prefer a stable semantic needle for the alias pattern/assertion instead.Proposed change
{ provider: 'notion', resources: ['notion/pages'], layoutPrompt: 'src/layout-prompt.ts', emissionTest: 'src/__tests__/emit-auxiliary-files.test.ts', - emissionNeedles: ['notionByEditedAliasPath', 'canonicalBytes', '2026-05-12'], + emissionNeedles: ['notionByEditedAliasPath', 'canonicalBytes', 'by-edited/'], },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/digest-layout-contracts.mjs` around lines 95 - 100, The emissionNeedles array is hardcoded with a specific fixture date ('2026-05-12'), which couples tests to a calendar date; update the contract in scripts/digest-layout-contracts.mjs by removing the literal date from emissionNeedles and replacing it with a stable semantic needle (e.g., a date-placeholder or pattern token) that represents the alias date component used by the layout assertion; specifically, modify the emissionNeedles entry that currently includes '2026-05-12' (alongside 'notionByEditedAliasPath' and 'canonicalBytes') to use a durable token like 'ISO_DATE_PLACEHOLDER' or an alias-pattern token so the test asserts the date format/presence rather than a fixed day.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/notion/src/__tests__/emit-auxiliary-files.test.ts`:
- Around line 570-709: Add a collision test that mirrors the existing
alias-subtree tests: call emitNotionAuxiliaryFiles with two page objects that
share the same lastEditedTime (e.g., '2026-05-12') but have different UUIDs,
then compute their by-edited alias paths using notionPageByEditedAliasPath and
assert the two paths are not equal and that both paths were written by the test
client (createClient and client.writes). Ensure the test uses distinct inline
UUIDs whose trailing 8 hex differ so aliasShortId produces different suffixes
and follow the same assert.notEqual / assert.ok(writtenPaths.has(...)) pattern
as the other collision tests.
---
Nitpick comments:
In `@scripts/digest-layout-contracts.mjs`:
- Around line 95-100: The emissionNeedles array is hardcoded with a specific
fixture date ('2026-05-12'), which couples tests to a calendar date; update the
contract in scripts/digest-layout-contracts.mjs by removing the literal date
from emissionNeedles and replacing it with a stable semantic needle (e.g., a
date-placeholder or pattern token) that represents the alias date component used
by the layout assertion; specifically, modify the emissionNeedles entry that
currently includes '2026-05-12' (alongside 'notionByEditedAliasPath' and
'canonicalBytes') to use a durable token like 'ISO_DATE_PLACEHOLDER' or an
alias-pattern token so the test asserts the date format/presence rather than a
fixed day.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ded228ae-1fbd-4177-a120-8f48c12ee98c
📒 Files selected for processing (2)
packages/notion/src/__tests__/emit-auxiliary-files.test.tsscripts/digest-layout-contracts.mjs
khaliqgant
force-pushed
the
codex/pr39-adapters-complete
branch
from
474d404 to
b16c44d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/notion/src/writeback.ts (1)
89-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPer-page
...{pageId}.json/<id>.jsonroutes are declared but not routable.The resolver only matches terminal
.../{pageId}.jsonpaths, so writes to the newly declared.../{pageId}.json/<id>.jsonendpoints never resolve and throwNo Notion writeback rule matched. Add explicit route handling for the child-file form (for both database and standalone page variants) to keep discovery/resources/writeback consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/notion/src/writeback.ts` around lines 89 - 103, The resolver currently only matches terminal paths like /notion/databases/{dbId}/pages/{pageId}.json and /notion/pages/{pageId}.json; add explicit handling for the child-file form /notion/databases/{dbId}/pages/{pageId}.json/{childId}.json and /notion/pages/{pageId}.json/{childId}.json so those requests don't fall through to "No Notion writeback rule matched". Update the logic around the databasePageMatch and standalonePageMatch checks in classifyWrite/writeback path handling to also test for the child-file regex variants and route kinds ('create' and 'patch') and route them to the same builders (buildCreatePageWriteback for create, buildPagePropertiesWriteback for patch) using extractNotionId on the parent {pageId} and preserving content and isCanonicalStandalonePageSegment checks for standalone pages.
🧹 Nitpick comments (1)
packages/notion/src/__tests__/writeback.test.ts (1)
145-155: ⚡ Quick winAdd route tests for the new
...{pageId}.json/<id>.jsonwriteback form.This suite still doesn’t assert the newly introduced per-page child-file route shape, which is exactly where routing drift can happen. Please add database + standalone cases for that path form.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/notion/src/__tests__/writeback.test.ts` around lines 145 - 155, Add tests in writeback.test.ts that cover the new per-page child-file writeback shape by calling resolveWritebackRequest with the "{pageId}.json/<childId>.json" paths for both standalone pages and pages inside a database (e.g. invoke resolveWritebackRequest('/notion/pages/page-1/comments.json/child-1.json', ...) and resolveWritebackRequest('/notion/databases/db-1/pages/page-2/comments.json/child-1.json', ...)); assert the returned objects from resolveWritebackRequest (use new variables like pageChild and dbPageChild) map to the correct API endpoint and include the child id/page id in the payload or query as the implementation expects (refer to resolveWritebackRequest, and existing assertions for markdown, markdownObject, comment for guiding style).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/github/src/emit-auxiliary-files.ts`:
- Around line 1093-1100: The sort comparator currently calls Date.parse(...)
directly on candidates built with readNonEmptyString, which allows invalid
timestamps (Date.parse -> NaN) to corrupt ordering; before sorting, map each
candidate to a parsed numeric timestamp, filter out non-finite results using
Number.isFinite (the same defensive pattern used in Zendesk/Slack adapters),
then sort remaining finite timestamps descending and pick the first; update the
code surrounding the array of readNonEmptyString(...) values and the .sort(...)
call to perform parsing+filtering of finite numbers prior to sorting so only
valid dates are compared.
In `@packages/notion/src/resources.ts`:
- Around line 21-82: The resource entries that create directory-style records
under "/notion/databases/{databaseId}/pages/{pageId}.json/..." and
"/notion/pages/{pageId}/content.md/..." violate the meta.json rule; update the
affected resource objects (the ones with name "pages", "properties", "content",
and "comments" shown in the diff) to use directory paths ending in "meta.json"
(e.g. "/notion/pages/{pageId}/meta.json" and
"/notion/databases/{databaseId}/pages/{pageId}/meta.json"), adjust their
pathPattern and idPattern accordingly, and update each schema and createExample
references to the new "meta.json" directories so discovery/writeback contracts
stay consistent. Ensure flat entities (no children) remain as .json/.md files
while entities with children switch to the meta.json directory convention and
update any related discovery/writeback mappings that reference these resource
names.
In `@scripts/writeback-discovery-data.mjs`:
- Around line 322-335: The create endpoints using notionCommentProps() (see
endpoint calls that create comments like
'/notion/databases/{databaseId}/pages/{pageId}/comments.json' and
'/notion/pages/{pageId}/comments.json') currently allow empty objects because
notionCommentProps() only defines optional fields; update validation so the
request body cannot be `{}` — either add a non-optional required field to
notionCommentProps() or wrap/replace it for create routes with a schema that
enforces minProperties: 1 (or an equivalent required constraint) so the create
comment payload must contain at least one property.
- Around line 313-329: The patch endpoints erroneously mark properties as
required, blocking valid payloads that only update archived/icon/cover; update
the endpoint declarations (e.g.,
endpoint('/notion/databases/{databaseId}/pages/{pageId}.json', ...),
endpoint('/notion/databases/{databaseId}/pages/{pageId}/properties.json', ...),
endpoint('/notion/databases/{databaseId}/pages/{pageId}/comments.json', ...),
endpoint('/notion/pages/{pageId}.json', ...) ) to stop forcing required:
['properties'] — either remove that third-argument required array or replace it
with an empty array (or mark properties optional in notionPagePatchProps()) so
patches that only include archived/icon/cover succeed.
---
Outside diff comments:
In `@packages/notion/src/writeback.ts`:
- Around line 89-103: The resolver currently only matches terminal paths like
/notion/databases/{dbId}/pages/{pageId}.json and /notion/pages/{pageId}.json;
add explicit handling for the child-file form
/notion/databases/{dbId}/pages/{pageId}.json/{childId}.json and
/notion/pages/{pageId}.json/{childId}.json so those requests don't fall through
to "No Notion writeback rule matched". Update the logic around the
databasePageMatch and standalonePageMatch checks in classifyWrite/writeback path
handling to also test for the child-file regex variants and route kinds
('create' and 'patch') and route them to the same builders
(buildCreatePageWriteback for create, buildPagePropertiesWriteback for patch)
using extractNotionId on the parent {pageId} and preserving content and
isCanonicalStandalonePageSegment checks for standalone pages.
---
Nitpick comments:
In `@packages/notion/src/__tests__/writeback.test.ts`:
- Around line 145-155: Add tests in writeback.test.ts that cover the new
per-page child-file writeback shape by calling resolveWritebackRequest with the
"{pageId}.json/<childId>.json" paths for both standalone pages and pages inside
a database (e.g. invoke
resolveWritebackRequest('/notion/pages/page-1/comments.json/child-1.json', ...)
and
resolveWritebackRequest('/notion/databases/db-1/pages/page-2/comments.json/child-1.json',
...)); assert the returned objects from resolveWritebackRequest (use new
variables like pageChild and dbPageChild) map to the correct API endpoint and
include the child id/page id in the payload or query as the implementation
expects (refer to resolveWritebackRequest, and existing assertions for markdown,
markdownObject, comment for guiding style).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c808a076-7d49-4ee0-bb11-b05a3d526117
📒 Files selected for processing (27)
packages/github/src/__tests__/emit-auxiliary-files.test.tspackages/github/src/emit-auxiliary-files.tspackages/notion/discovery/notion/.adapter.mdpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}.json/.create.example.jsonpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}.json/.schema.jsonpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/comments.json/.create.example.jsonpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/comments.json/.schema.jsonpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/content.md/.create.example.jsonpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/content.md/.schema.jsonpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/properties.json/.create.example.jsonpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/properties.json/.schema.jsonpackages/notion/discovery/notion/pages/{pageId}.json/.create.example.jsonpackages/notion/discovery/notion/pages/{pageId}.json/.schema.jsonpackages/notion/discovery/notion/pages/{pageId}/comments.json/.create.example.jsonpackages/notion/discovery/notion/pages/{pageId}/comments.json/.schema.jsonpackages/notion/discovery/notion/pages/{pageId}/content.md/.create.example.jsonpackages/notion/discovery/notion/pages/{pageId}/content.md/.schema.jsonpackages/notion/discovery/notion/pages/{pageId}/properties.json/.create.example.jsonpackages/notion/discovery/notion/pages/{pageId}/properties.json/.schema.jsonpackages/notion/src/__tests__/emit-auxiliary-files.test.tspackages/notion/src/__tests__/writeback.test.tspackages/notion/src/resources.tspackages/notion/src/writeback.tsscripts/digest-layout-contracts.mjsscripts/verify-writeback-discovery.mjsscripts/writeback-discovery-data.mjsscripts/writeback-discovery-normalizer.mjs
✅ Files skipped from review due to trivial changes (12)
- packages/notion/discovery/notion/pages/{pageId}/comments.json/.create.example.json
- packages/notion/discovery/notion/pages/{pageId}/properties.json/.create.example.json
- packages/notion/discovery/notion/pages/{pageId}/content.md/.create.example.json
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/content.md/.create.example.json
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/properties.json/.create.example.json
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/properties.json/.schema.json
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}.json/.schema.json
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/content.md/.schema.json
- packages/notion/discovery/notion/pages/{pageId}/content.md/.schema.json
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/comments.json/.create.example.json
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}.json/.create.example.json
- packages/notion/discovery/notion/pages/{pageId}.json/.create.example.json
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/digest-layout-contracts.mjs
Comment on lines
+21
to
+82
| path: "/notion/databases/{databaseId}/pages/{pageId}.json", | ||
| pathPattern: /^\/notion\/databases\/[^\/]+\/pages\/[^\/]+\.json(?:\/[^\/]+(?:\.json)?)?$/, | ||
| idPattern: /^(?:[A-Za-z0-9_.~-]+(?:--|__))?(?:[0-9a-f]{32}|[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})$/i, | ||
| schema: "discovery/notion/databases/{databaseId}/pages/{pageId}.json/.schema.json", | ||
| createExample: "discovery/notion/databases/{databaseId}/pages/{pageId}.json/.create.example.json", | ||
| }, | ||
| { | ||
| name: "properties", | ||
| path: "/notion/databases/{databaseId}/pages/{pageId}/properties.json", | ||
| pathPattern: /^\/notion\/databases\/[^\/]+\/pages\/[^\/]+\/properties\.json(?:\/[^\/]+(?:\.json)?)?$/, | ||
| idPattern: /^(?:[A-Za-z0-9_.~-]+(?:--|__))?(?:[0-9a-f]{32}|[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})$/i, | ||
| schema: "discovery/notion/databases/{databaseId}/pages/{pageId}/properties.json/.schema.json", | ||
| createExample: "discovery/notion/databases/{databaseId}/pages/{pageId}/properties.json/.create.example.json", | ||
| }, | ||
| { | ||
| name: "content", | ||
| path: "/notion/databases/{databaseId}/pages/{pageId}/content.md", | ||
| pathPattern: /^\/notion\/databases\/[^\/]+\/pages\/[^\/]+\/content\.md(?:\/[^\/]+(?:\.json)?)?$/, | ||
| idPattern: /^(?:[A-Za-z0-9_.~-]+(?:--|__))?(?:[0-9a-f]{32}|[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})$/i, | ||
| schema: "discovery/notion/databases/{databaseId}/pages/{pageId}/content.md/.schema.json", | ||
| createExample: "discovery/notion/databases/{databaseId}/pages/{pageId}/content.md/.create.example.json", | ||
| }, | ||
| { | ||
| name: "comments", | ||
| path: "/notion/databases/{databaseId}/pages/{pageId}/comments.json", | ||
| pathPattern: /^\/notion\/databases\/[^\/]+\/pages\/[^\/]+\/comments\.json(?:\/[^\/]+(?:\.json)?)?$/, | ||
| idPattern: /^(?:[A-Za-z0-9_.~-]+(?:--|__))?(?:[0-9a-f]{32}|[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})$/i, | ||
| schema: "discovery/notion/databases/{databaseId}/pages/{pageId}/comments.json/.schema.json", | ||
| createExample: "discovery/notion/databases/{databaseId}/pages/{pageId}/comments.json/.create.example.json", | ||
| }, | ||
| { | ||
| name: "pages", | ||
| path: "/notion/pages/{pageId}.json", | ||
| pathPattern: /^\/notion\/pages\/[^\/]+\.json(?:\/[^\/]+(?:\.json)?)?$/, | ||
| idPattern: /^(?:[A-Za-z0-9_.~-]+(?:--|__))?(?:[0-9a-f]{32}|[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})$/i, | ||
| schema: "discovery/notion/pages/{pageId}.json/.schema.json", | ||
| createExample: "discovery/notion/pages/{pageId}.json/.create.example.json", | ||
| }, | ||
| { | ||
| name: "properties", | ||
| path: "/notion/pages/{pageId}/properties.json", | ||
| pathPattern: /^\/notion\/pages\/[^\/]+\/properties\.json(?:\/[^\/]+(?:\.json)?)?$/, | ||
| idPattern: /^(?:[A-Za-z0-9_.~-]+(?:--|__))?(?:[0-9a-f]{32}|[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})$/i, | ||
| schema: "discovery/notion/pages/{pageId}/properties.json/.schema.json", | ||
| createExample: "discovery/notion/pages/{pageId}/properties.json/.create.example.json", | ||
| }, | ||
| { | ||
| name: "content", | ||
| path: "/notion/pages/{pageId}/content.md", | ||
| pathPattern: /^\/notion\/pages\/[^\/]+\/content\.md(?:\/[^\/]+(?:\.json)?)?$/, | ||
| idPattern: /^(?:[A-Za-z0-9_.~-]+(?:--|__))?(?:[0-9a-f]{32}|[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})$/i, | ||
| schema: "discovery/notion/pages/{pageId}/content.md/.schema.json", | ||
| createExample: "discovery/notion/pages/{pageId}/content.md/.create.example.json", | ||
| }, | ||
| { | ||
| name: "comments", | ||
| path: "/notion/pages/{pageId}/comments.json", | ||
| pathPattern: /^\/notion\/pages\/[^\/]+\/comments\.json(?:\/[^\/]+(?:\.json)?)?$/, | ||
| idPattern: /^(?:[A-Za-z0-9_.~-]+(?:--|__))?(?:[0-9a-f]{32}|[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})$/i, | ||
| schema: "discovery/notion/pages/{pageId}/comments.json/.schema.json", | ||
| createExample: "discovery/notion/pages/{pageId}/comments.json/.create.example.json", | ||
| }, |
There was a problem hiding this comment.
Use meta.json for entity directories with child artifacts.
These new resource paths model child artifacts under .../{pageId}.json/... and .../content.md/..., which conflicts with the repository rule for directory-record naming. Please migrate these directory records to .../meta.json/...-style paths (and update discovery/writeback contracts consistently).
As per coding guidelines, "Entities with child artifacts MUST use directory records ending in meta.json; entities without child artifacts MUST use flat .json records".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/notion/src/resources.ts` around lines 21 - 82, The resource entries
that create directory-style records under
"/notion/databases/{databaseId}/pages/{pageId}.json/..." and
"/notion/pages/{pageId}/content.md/..." violate the meta.json rule; update the
affected resource objects (the ones with name "pages", "properties", "content",
and "comments" shown in the diff) to use directory paths ending in "meta.json"
(e.g. "/notion/pages/{pageId}/meta.json" and
"/notion/databases/{databaseId}/pages/{pageId}/meta.json"), adjust their
pathPattern and idPattern accordingly, and update each schema and createExample
references to the new "meta.json" directories so discovery/writeback contracts
stay consistent. Ensure flat entities (no children) remain as .json/.md files
while entities with children switch to the meta.json directory convention and
update any related discovery/writeback mappings that reference these resource
names.
Member
Author
There was a problem hiding this comment.
I reviewed this against the current branch and am intentionally not migrating Notion page records to meta.json in this PR. The existing Notion adapter already emits and tests /notion/pages/<page>.json and /notion/databases/<db>/pages/<page>.json as stable canonical paths, with content/comments/properties as sibling exact-file writeback sidecars. Changing those paths here would be a broader compatibility migration across emitted paths, tests, and consumers rather than review cleanup. I did tighten the still-valid validation findings in c73148e: PATCH schemas/runtime now accept properties/archived/icon/cover, comment schemas/runtime reject empty payloads, GitHub timestamp sorting filters invalid dates, and LAYOUT.md has explicit non-empty coverage.
There was a problem hiding this comment.
@khaliqgant, understood — the decision makes sense given that /notion/pages/<page>.json and /notion/databases/<db>/pages/<page>.json are already stable canonical emitted paths, and content/comments/properties are intentional exact-file sidecar writeback endpoints rather than nested directory records. Treating a breaking path migration as separate work is the right call here.
Would you like me to open a follow-up GitHub issue to track the meta.json migration for Notion page records so it doesn't get lost?
I'll record the intentional design for future reviews:
✏️ Learnings added
Learnt from: khaliqgant
Repo: AgentWorkforce/relayfile-adapters PR: 100
File: packages/notion/src/resources.ts:21-82
Timestamp: 2026-05-19T10:25:36.028Z
Learning: In the Notion adapter (`packages/notion/src/resources.ts`), `/notion/pages/{pageId}.json` and `/notion/databases/{databaseId}/pages/{pageId}.json` are intentionally stable canonical emitted paths. The `content.md`, `properties.json`, and `comments.json` under `{pageId}/` are exact-file sidecar writeback endpoints, NOT nested directory records. Migrating these to a `meta.json`-style directory convention would be a separate broad compatibility migration and should NOT be flagged as a violation in PR review.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
khaliqgant
force-pushed
the
codex/pr39-adapters-complete
branch
2 times, most recently
from
2046457 to
f89f2ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/notion/discovery/notion/databases/`{databaseId}/pages/{pageId}.json/.schema.json:
- Around line 5-7: The schema wrongly forces "properties" as required for
PATCH-style updates; remove "properties" from the "required" array (or clear the
"required" array) so fields like "archived", "icon", and "cover" can be updated
independently, and ensure the schema still validates those fields when present
(e.g., keep their definitions under the root object but do not make "properties"
mandatory).
In `@packages/notion/src/__tests__/layout-prompt.test.ts`:
- Around line 18-20: Add an explicit non-empty content assertion for the
LAYOUT.md emitter in the test: in
packages/notion/src/__tests__/layout-prompt.test.ts, locate the test that reads
`file.content` (the assertions using assert.match shown) and add a direct check
such as asserting `file.content.trim().length > 0` (e.g., with assert.ok or
assert.strictEqual) immediately after those assert.match calls so the test fails
if LAYOUT.md is empty; ensure you reference the same `file`/`file.content`
variable used in the existing assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 50583ef7-de55-4553-b91f-c7bf15d5e902
📒 Files selected for processing (33)
docs/writeback-spec-coverage.mdpackages/github/src/__tests__/emit-auxiliary-files.test.tspackages/github/src/emit-auxiliary-files.tspackages/notion/discovery/notion/.adapter.mdpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}.json/.create.example.jsonpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}.json/.schema.jsonpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/comments.json/.create.example.jsonpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/comments.json/.schema.jsonpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/content.md/.create.example.jsonpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/content.md/.schema.jsonpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/properties.json/.create.example.jsonpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/properties.json/.schema.jsonpackages/notion/discovery/notion/pages/{pageId}.json/.create.example.jsonpackages/notion/discovery/notion/pages/{pageId}.json/.schema.jsonpackages/notion/discovery/notion/pages/{pageId}/comments.json/.create.example.jsonpackages/notion/discovery/notion/pages/{pageId}/comments.json/.schema.jsonpackages/notion/discovery/notion/pages/{pageId}/content.md/.create.example.jsonpackages/notion/discovery/notion/pages/{pageId}/content.md/.schema.jsonpackages/notion/discovery/notion/pages/{pageId}/properties.json/.create.example.jsonpackages/notion/discovery/notion/pages/{pageId}/properties.json/.schema.jsonpackages/notion/src/__tests__/emit-auxiliary-files.test.tspackages/notion/src/__tests__/layout-prompt.test.tspackages/notion/src/__tests__/writeback.test.tspackages/notion/src/layout-prompt.tspackages/notion/src/layout.test.tspackages/notion/src/layout.tspackages/notion/src/resources.tspackages/notion/src/writeback.tsscripts/digest-layout-contracts.mjsscripts/generate-writeback-discovery.mjsscripts/verify-writeback-discovery.mjsscripts/writeback-discovery-data.mjsscripts/writeback-discovery-normalizer.mjs
✅ Files skipped from review due to trivial changes (10)
- packages/notion/discovery/notion/pages/{pageId}/content.md/.create.example.json
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}.json/.create.example.json
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/comments.json/.schema.json
- packages/notion/discovery/notion/pages/{pageId}/comments.json/.create.example.json
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/comments.json/.create.example.json
- docs/writeback-spec-coverage.md
- packages/notion/discovery/notion/pages/{pageId}/properties.json/.create.example.json
- packages/notion/src/layout-prompt.ts
- packages/notion/discovery/notion/pages/{pageId}.json/.create.example.json
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/content.md/.create.example.json
🚧 Files skipped from review as they are similar to previous changes (20)
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/properties.json/.create.example.json
- packages/notion/src/layout.test.ts
- packages/notion/src/layout.ts
- scripts/verify-writeback-discovery.mjs
- packages/notion/discovery/notion/pages/{pageId}/content.md/.schema.json
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/content.md/.schema.json
- packages/notion/discovery/notion/pages/{pageId}.json/.schema.json
- packages/github/src/tests/emit-auxiliary-files.test.ts
- scripts/writeback-discovery-normalizer.mjs
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/properties.json/.schema.json
- packages/notion/src/writeback.ts
- packages/notion/discovery/notion/pages/{pageId}/comments.json/.schema.json
- scripts/digest-layout-contracts.mjs
- packages/notion/src/tests/writeback.test.ts
- packages/notion/src/tests/emit-auxiliary-files.test.ts
- packages/github/src/emit-auxiliary-files.ts
- scripts/writeback-discovery-data.mjs
- packages/notion/discovery/notion/.adapter.md
- packages/notion/discovery/notion/pages/{pageId}/properties.json/.schema.json
- packages/notion/src/resources.ts
khaliqgant
force-pushed
the
codex/pr39-adapters-complete
branch
3 times, most recently
from
8f14d8f to
67b5c30
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
scripts/writeback-discovery-data.mjs (2)
315-331:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
propertiesis incorrectly mandatory for Notion patch routes.Line 315, Line 318, Line 327, and Line 330 describe patching
archived/icon/cover, butrequired: ['properties']rejects those valid payloads.Proposed fix
- endpoint('/notion/databases/{databaseId}/pages/{pageId}.json', 'Update Notion database page properties', 'Updates properties, archive state, icon, or cover for a page inside a Notion database.', ['properties'], notionPagePatchProps(), { + endpoint('/notion/databases/{databaseId}/pages/{pageId}.json', 'Update Notion database page properties', 'Updates properties, archive state, icon, or cover for a page inside a Notion database.', [], notionPagePatchProps(), { properties: { Status: { type: 'select', value: 'In progress' } }, - }), - endpoint('/notion/databases/{databaseId}/pages/{pageId}/properties.json', 'Update Notion database page properties file', 'Updates properties, archive state, icon, or cover through a database page properties sidecar.', ['properties'], notionPagePatchProps(), { + }, notionPagePatchRequirement()), + endpoint('/notion/databases/{databaseId}/pages/{pageId}/properties.json', 'Update Notion database page properties file', 'Updates properties, archive state, icon, or cover through a database page properties sidecar.', [], notionPagePatchProps(), { properties: { Status: { type: 'select', value: 'In progress' } }, - }), - endpoint('/notion/pages/{pageId}.json', 'Update Notion standalone page properties', 'Updates properties, archive state, icon, or cover for a standalone page.', ['properties'], notionPagePatchProps(), { + }, notionPagePatchRequirement()), + endpoint('/notion/pages/{pageId}.json', 'Update Notion standalone page properties', 'Updates properties, archive state, icon, or cover for a standalone page.', [], notionPagePatchProps(), { properties: { Status: { type: 'select', value: 'In progress' } }, - }), - endpoint('/notion/pages/{pageId}/properties.json', 'Update Notion standalone page properties file', 'Updates properties, archive state, icon, or cover through a standalone page properties sidecar.', ['properties'], notionPagePatchProps(), { + }, notionPagePatchRequirement()), + endpoint('/notion/pages/{pageId}/properties.json', 'Update Notion standalone page properties file', 'Updates properties, archive state, icon, or cover through a standalone page properties sidecar.', [], notionPagePatchProps(), { properties: { Status: { type: 'select', value: 'In progress' } }, - }), + }, notionPagePatchRequirement()),+function notionPagePatchRequirement() { + return { + anyOf: [ + { required: ['properties'] }, + { required: ['archived'] }, + { required: ['icon'] }, + { required: ['cover'] }, + ], + }; +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/writeback-discovery-data.mjs` around lines 315 - 331, The endpoint definitions for Notion patch routes currently mark properties as required (the fourth arg ['properties']) which wrongly rejects valid payloads that only set archived/icon/cover; for each endpoint call that uses notionPagePatchProps() — e.g. the endpoints with paths '/notion/databases/{databaseId}/pages/{pageId}.json', '/notion/databases/{databaseId}/pages/{pageId}/properties.json', '/notion/pages/{pageId}.json', and '/notion/pages/{pageId}/properties.json' — remove the ['properties'] required list (or replace it with an empty array) so properties is not mandatory while keeping notionPagePatchProps() as the schema provider. Ensure you update only the required-list argument and leave the rest of the endpoint(...) call intact.
324-337:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNotion comment-create schemas currently allow empty
{}payloads.Line 324 and Line 336 use
required: [], andnotionCommentProps()has only optional fields, so invalid empty bodies pass discovery validation.Proposed fix
- endpoint('/notion/databases/{databaseId}/pages/{pageId}/comments.json', 'Create Notion database page comment', 'Creates a Notion comment on a page inside a Notion database from comments.json.', [], notionCommentProps(), { + endpoint('/notion/databases/{databaseId}/pages/{pageId}/comments.json', 'Create Notion database page comment', 'Creates a Notion comment on a page inside a Notion database from comments.json.', [], notionCommentProps(), { text: 'Replace example comment body.', - }), + }, notionCommentRequirement()), ... - endpoint('/notion/pages/{pageId}/comments.json', 'Create Notion standalone page comment', 'Creates a Notion comment on a standalone page from comments.json.', [], notionCommentProps(), { + endpoint('/notion/pages/{pageId}/comments.json', 'Create Notion standalone page comment', 'Creates a Notion comment on a standalone page from comments.json.', [], notionCommentProps(), { text: 'Replace example comment body.', - }), + }, notionCommentRequirement()),+function notionCommentRequirement() { + return { + anyOf: [ + { required: ['text'] }, + { required: ['richText'] }, + ], + }; +}Also applies to: 985-991
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/writeback-discovery-data.mjs` around lines 324 - 337, The comment-create endpoints currently pass required: [] and notionCommentProps() only defines optional fields, allowing empty {} bodies; update the schema so a comment body must include the text field—either add 'text' to the endpoint calls' required array (e.g. change required: [] to required: ['text'] for the endpoint('/notion/.../comments.json' entries) or modify notionCommentProps() to mark text as required—ensure the validation schema references the text property as required so empty payloads are rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@scripts/writeback-discovery-data.mjs`:
- Around line 315-331: The endpoint definitions for Notion patch routes
currently mark properties as required (the fourth arg ['properties']) which
wrongly rejects valid payloads that only set archived/icon/cover; for each
endpoint call that uses notionPagePatchProps() — e.g. the endpoints with paths
'/notion/databases/{databaseId}/pages/{pageId}.json',
'/notion/databases/{databaseId}/pages/{pageId}/properties.json',
'/notion/pages/{pageId}.json', and '/notion/pages/{pageId}/properties.json' —
remove the ['properties'] required list (or replace it with an empty array) so
properties is not mandatory while keeping notionPagePatchProps() as the schema
provider. Ensure you update only the required-list argument and leave the rest
of the endpoint(...) call intact.
- Around line 324-337: The comment-create endpoints currently pass required: []
and notionCommentProps() only defines optional fields, allowing empty {} bodies;
update the schema so a comment body must include the text field—either add
'text' to the endpoint calls' required array (e.g. change required: [] to
required: ['text'] for the endpoint('/notion/.../comments.json' entries) or
modify notionCommentProps() to mark text as required—ensure the validation
schema references the text property as required so empty payloads are rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ca5b1e8-6607-45f5-a6a3-dde7aac8d52b
📒 Files selected for processing (38)
docs/writeback-spec-coverage.mdpackages/core/src/runtime/file-native-router.tspackages/github/src/__tests__/emit-auxiliary-files.test.tspackages/github/src/emit-auxiliary-files.tspackages/gitlab/discovery/gitlab/.adapter.mdpackages/gitlab/discovery/gitlab/projects/{projectPath}/issues/{issueIid}__{slug}/comments/.schema.jsonpackages/gitlab/discovery/gitlab/projects/{projectPath}/merge_requests/{mergeRequestIid}__{slug}/discussions/.schema.jsonpackages/notion/discovery/notion/.adapter.mdpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}.json/.create.example.jsonpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}.json/.schema.jsonpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/comments.json/.create.example.jsonpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/comments.json/.schema.jsonpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/content.md/.create.example.jsonpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/content.md/.schema.jsonpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/properties.json/.create.example.jsonpackages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/properties.json/.schema.jsonpackages/notion/discovery/notion/pages/{pageId}.json/.create.example.jsonpackages/notion/discovery/notion/pages/{pageId}.json/.schema.jsonpackages/notion/discovery/notion/pages/{pageId}/comments.json/.create.example.jsonpackages/notion/discovery/notion/pages/{pageId}/comments.json/.schema.jsonpackages/notion/discovery/notion/pages/{pageId}/content.md/.create.example.jsonpackages/notion/discovery/notion/pages/{pageId}/content.md/.schema.jsonpackages/notion/discovery/notion/pages/{pageId}/properties.json/.create.example.jsonpackages/notion/discovery/notion/pages/{pageId}/properties.json/.schema.jsonpackages/notion/src/__tests__/emit-auxiliary-files.test.tspackages/notion/src/__tests__/layout-prompt.test.tspackages/notion/src/__tests__/writeback-id-extraction.test.tspackages/notion/src/__tests__/writeback.test.tspackages/notion/src/layout-prompt.tspackages/notion/src/layout.test.tspackages/notion/src/layout.tspackages/notion/src/resources.tspackages/notion/src/writeback.tsscripts/digest-layout-contracts.mjsscripts/generate-writeback-discovery.mjsscripts/verify-writeback-discovery.mjsscripts/writeback-discovery-data.mjsscripts/writeback-discovery-normalizer.mjs
✅ Files skipped from review due to trivial changes (7)
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}.json/.create.example.json
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/content.md/.create.example.json
- packages/gitlab/discovery/gitlab/projects/{projectPath}/merge_requests/{mergeRequestIid}__{slug}/discussions/.schema.json
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/comments.json/.create.example.json
- docs/writeback-spec-coverage.md
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/properties.json/.create.example.json
- packages/notion/discovery/notion/.adapter.md
🚧 Files skipped from review as they are similar to previous changes (25)
- packages/notion/discovery/notion/pages/{pageId}.json/.create.example.json
- packages/notion/discovery/notion/pages/{pageId}/content.md/.create.example.json
- packages/notion/discovery/notion/pages/{pageId}/comments.json/.create.example.json
- packages/notion/src/tests/layout-prompt.test.ts
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/content.md/.schema.json
- packages/notion/src/tests/emit-auxiliary-files.test.ts
- packages/github/src/emit-auxiliary-files.ts
- packages/github/src/tests/emit-auxiliary-files.test.ts
- scripts/generate-writeback-discovery.mjs
- packages/notion/discovery/notion/pages/{pageId}/comments.json/.schema.json
- packages/notion/src/layout.ts
- packages/notion/discovery/notion/pages/{pageId}/properties.json/.schema.json
- packages/notion/src/layout-prompt.ts
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/comments.json/.schema.json
- scripts/digest-layout-contracts.mjs
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}.json/.schema.json
- packages/notion/discovery/notion/pages/{pageId}/properties.json/.create.example.json
- packages/notion/src/resources.ts
- packages/notion/src/layout.test.ts
- scripts/writeback-discovery-normalizer.mjs
- packages/notion/src/writeback.ts
- scripts/verify-writeback-discovery.mjs
- packages/notion/discovery/notion/pages/{pageId}/content.md/.schema.json
- packages/notion/discovery/notion/pages/{pageId}.json/.schema.json
- packages/notion/discovery/notion/databases/{databaseId}/pages/{pageId}/properties.json/.schema.json
khaliqgant
force-pushed
the
codex/pr39-adapters-complete
branch
from
67b5c30 to
c73148e
Compare
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.
Summary
Verification