feat(page): support native local image upload via official API#29
Conversation
|
(#28 should be merged first before this can be) |
Detect standalone local markdown image lines during page upload and sync, upload those files through the official Notion API, and substitute them back into the page in document order. Reject inline local image syntax instead of silently dropping it.
Send uploaded file parts with the correct MIME type, use the current Notion block insertion payload shape, and fail closed when local image uploads are attempted without a parent shared to the integration. This keeps page upload and sync working against the live API instead of relying on stale request contracts or leaving orphaned pages behind.
Use mime.FormatMediaType for the file part content disposition so quoted filenames are encoded safely while keeping the live upload path working against the current Notion API.
When set, standalone local image lines are silently stripped from the markdown instead of triggering the official API upload flow. Remote images are preserved. This lets users upload/sync documents with local image references even without an API token configured.
0xble
force-pushed
the
feat/page-native-local-image-upload-upstream
branch
from
4b6d20f to
d96b12d
Compare
|
@codex review |
![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: d96b12d0fe
ℹ️ 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".
--skip-local-images was reusing the upload-validation path, so a missing referenced file aborted the strip with a not-found error. Split validation out of the parser and wire strip mode to a non-validating helper. Inline or mixed-content local image syntax still errors so callers never silently drop characters around an inline image.
rollbackSyncedPage only short-circuited on nil or empty snapshots before. If GetPageMarkdown returned a truncated snapshot, rollback would have replayed the partial markdown back to the live page via replace_content and silently dropped content. Skip rollback when snapshot.Truncated is set and surface an explicit error so the caller still sees the original substitution failure and the fact that rollback was intentionally skipped.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce000bff0e
ℹ️ 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".
Replace the regex-based local-image scanner with a line-level parser that tracks fenced and indented code blocks and parses destinations with balanced parens and CommonMark backslash escapes. This prevents standalone image lines inside fenced or indented code samples from being treated as upload/strip targets and fixes "not found" errors for paths like ./diagram(1).png or ./diagram\(final\).png.
Drop the early-return that treated a blank snapshot markdown as "nothing to restore". An empty or whitespace-only snapshot is a valid prior page state, so rollback should still issue replace_content instead of leaving placeholder-filled content on the page when image substitution fails after a replace on a previously empty page. Route rollback through a pageUpdater interface so the behavior is covered by tests without a live MCP client.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ff612a759
ℹ️ 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".
Track `[`/`]` depth inside alt-text parsing so images with nested brackets like `![Architecture [v2]](./diagram.png)` are recognized as a single standalone image, and treat `//cdn.example.com/...` destinations as remote so protocol-relative URLs are not mistaken for local paths.
Previously substituteUploadedLocalImages silently returned success when pageID was empty but uploads were pending, leaving `NOTION_CLI_LOCAL_IMAGE_*` placeholders in the published page whenever CreatePage's fallback path could not extract an ID. Return an explicit error in that case so upload/sync surfaces the failure instead of reporting success with corrupted content.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 315f5317d3
ℹ️ 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".
Treat `\![...]` as literal text so documentation that shows image syntax doesn't trip the unsupported-inline-syntax error, and accept an optional whitespace-separated title (in `"..."`, `'...'`, or `(...)` form) after angle-bracketed destinations so valid markdown like `` parses correctly instead of being silently skipped.
page upload and create-mode page sync now verify --parent/--parent-db is set before prepareLocalImageUploads runs, so a missing-parent user error is raised up front instead of after local files have already been uploaded to Notion. Replaces requireLocalImageParent with a pre-upload checkLocalImageParent helper that scans markdown via FindStandaloneLocalImageLines.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d31965d973
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4f38245fa
ℹ️ 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".
Three fixes motivated by reviewer findings on the local image flow. runPageSync now scans for local images up front, fetches the page markdown snapshot, and fails fast when the snapshot is truncated. Previously we uploaded local images first and only noticed truncation during rollback, leaving every upload orphaned on the official API and the page stuck with placeholder paragraphs. The local image scanner masks inline code spans before looking for image syntax, so documentation text like backtick-- backtick no longer trips the "must appear on its own line" error and no longer needs a fenced or indented block to survive. The placeholder substitution keeps the original leading whitespace on the image line instead of collapsing to column zero, so images written as indented list continuations are not silently lifted out of their container before upload.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf87d08f77
ℹ️ 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".
scanStandaloneLocalImages preserves the whitespace that surrounded the image line (added with the indentation fix), so an input like " " now becomes " NOTION_CLI_LOCAL_IMAGE_*". The equality check in stripLocalImages only matched the bare placeholder token, so these indented lines survived intact and the command published the raw placeholder text instead of stripping the image. Match on the trimmed token so leading or trailing whitespace still strips correctly.
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ef067c452
ℹ️ 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".
runPageUpload and the create path of runPageSync previously uploaded local images via prepareLocalImageUploads before resolving --parent or --parent-db through the official API. An invalid parent reference returned an error only after bytes were already sent, leaving orphaned uploads behind on a command users expected to be a failed no-op. Resolve parent IDs first so any parent-resolution failure aborts before touching the file-upload path. Pre-resolved IDs are threaded into the CreatePage request so the resolution still only happens once.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fd1e95d36
ℹ️ 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".
When CreatePage succeeds but pageIDFromCreateResponse cannot extract an ID (empty resp.ID and unparseable resp.URL), substituteUploadedLocalImages returns a 'missing page ID' error and substituteOrCleanup then skips its TrashPage branch because pageID is empty, leaving a newly created page in the workspace with raw NOTION_CLI_LOCAL_IMAGE_* placeholders and no automatic recovery path. Pass the create URL through and, when the ID is missing with uploads pending, wrap the error with the create URL so the user can delete the orphan manually.
…tives isLocalDestination previously treated any destination whose second character was ':' as a Windows drive path, so valid single-letter URI schemes (for example 'a://example.com/img.png' or 'a:foo') were classified as filesystem paths. That caused 'local image not found' errors during sync and --skip-local-images incorrectly stripped those lines. Require the leading character to be an ASCII letter, require a path separator (or end-of-string) after the colon, and exclude 'x://...' authority-style URIs so one-letter schemes fall through to the existing scheme regex.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71d2800b54
ℹ️ 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".
UploadFile always sends mode=\"single_part\" but Notion's single-part upload limit is ~20MB; anything larger fails on the server with an opaque error. The client already knows the file size, so add an explicit SinglePartUploadMaxBytes constant and fail the upload early with a message that names the limit and notes that multi_part uploads are not yet supported. Full multipart support is out of scope for this PR and tracked as follow-up work.
parseMarkdownDestination stopped at the first '>' even when escaped, while the scanner's findDestinationEnd already walks past '\\>' the way CommonMark requires. That made inputs like '<./foo\\>bar.png>' parse as './foo\\' and fail local-file resolution. Walk character by character in the angle-bracket branch, skip escape sequences, and stop only at the first unescaped '>' so the two halves of the parser agree.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae15b56cc3
ℹ️ 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".
Two scanner fixes that together let standalone local images with quoted titles and escaped whitespace round-trip correctly: - parseMarkdownDestination already walked past '\\ ' so unbracketed destinations like './diagram\\ 1.png' parsed as one token, but unescapeMarkdownPunctuation only decoded ASCII punctuation; the backslash was left in the resolved path and resolveLocalPath failed to find the real file. Decode '\\ ' to ' ' during unescape. - findDestinationEnd closed the image span at the first unescaped ')' at depth 0, including ')' characters inside a quoted title like '![Alt](./img.png \"v1) draft\")'. Treat depth-0 whitespace as the destination terminator and hand off to skipOptionalTitleAndClose so a ')' inside the title can no longer be mistaken for the image's closing paren.
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ 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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38b68767e8
ℹ️ 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".
prepareLocalImageUploads fanned out up to four uploads in parallel, but each upload fires three sequential official-API calls (create, send, then poll), so even two in-flight uploads easily exceed Notion's documented ~3 req/sec average limit. Without a shared rate limiter or 429/Retry-After handling, multi-image sync hits HTTP 429 nondeterministically. Drop the concurrency limit to 1 so uploads proceed serially and stay under the documented budget; full rate-limit/backoff infrastructure can land separately.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1b03abf2b
ℹ️ 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".
The fence detector only looked for up to three leading spaces before checking for triple-backtick/tilde, so a quoted fence like '> \`\`\`md' never set the in-fence state. A subsequent quoted example line such as '> ' was then scanned as a real local image and hit the 'must appear on their own line' error path, failing page upload and page sync on ordinary documentation markdown. Treat any blockquote line (up to three spaces of indent then '>') as opaque to image scanning; blockquoted images already wouldn't round-trip cleanly through the substitute-placeholder path anyway.
|
Ready for this one to be merged @0xble? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce2ca0b049
ℹ️ 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".
|
Fixed the latest blockquote-indent edge case from the automated review. CI is green. Ready to merge. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 688d6ff70c
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d866ccf6e5
ℹ️ 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".
| func isBlockHTMLTag(name string) bool { | ||
| switch name { | ||
| case "address", "article", "aside", "base", "basefont", "blockquote", "body", "caption", "center", "col", "colgroup", "dd", "details", "dialog", "dir", "div", "dl", "dt", "fieldset", "figcaption", "figure", "footer", "form", "frame", "frameset", "h1", "h2", "h3", "h4", "h5", "h6", "head", "header", "hr", "html", "iframe", "legend", "li", "link", "main", "menu", "menuitem", "nav", "noframes", "ol", "optgroup", "option", "p", "param", "search", "section", "summary", "table", "tbody", "td", "tfoot", "th", "thead", "title", "tr", "track", "ul": | ||
| return true |
There was a problem hiding this comment.
The raw-HTML skip path only recognizes a fixed block-tag allowlist, so it misses CommonMark type-7 HTML blocks where a complete non-block tag appears by itself (for example <Warning> or <a href="..."> on its own line). In those documents, lines inside the HTML block are still scanned as markdown images, which can wrongly upload/strip local image syntax or raise the inline-syntax error even though that region should be treated as raw HTML text.
Useful? React with 👍 / 👎.
| case strings.HasPrefix(lower, "<script"), strings.HasPrefix(lower, "</script"): | ||
| return htmlBlockEndScript | ||
| case strings.HasPrefix(lower, "<pre"), strings.HasPrefix(lower, "</pre"): | ||
| return htmlBlockEndPre | ||
| case strings.HasPrefix(lower, "<style"), strings.HasPrefix(lower, "</style"): | ||
| return htmlBlockEndStyle |
There was a problem hiding this comment.
The opener checks use prefix matches like "<script", "<pre", and "<style" without validating a tag boundary, so tags such as <scriptx> or <prelude> are misclassified as literal-content HTML blocks. Because those block types only close on matching end tags, the scanner can incorrectly ignore the rest of the document (or until EOF), causing real local images later in the file to be skipped instead of uploaded/validated.
Useful? React with 👍 / 👎.
2 participants
Summary
page uploadandpage sync--skip-local-imagesflag topage uploadandpage syncso users can proceed without an API token by stripping local image lines from the markdownScreenshot
Why
This branch stacks on top of #28 and depends on that auth setup flow landing first. Until then, this PR includes those auth commits in its diff against
main.Token behavior
When
page uploadorpage syncdetects standalone local images in the markdown:NOTION_API_TOKENis set, images are uploaded via the official API.--skip-local-imagesis passed, local image lines are stripped and the page is created without them.Validation
go build ./...go test -v ./...golangci-lint runpage uploadwith standalone local imagespage synccreate and update with standalone local images--skip-local-imagesflag with and without API token