Allow ParseOptions as the second argument of getDocPage*() helpers#739
Allow ParseOptions as the second argument of getDocPage*() helpers#739
ParseOptions as the second argument of getDocPage*() helpers#739Conversation
getDocPage(), getDocPageSync(), and getDocPageAsync() silently misinterpreted a ParseOptions object as the args array when passed as the second argument. All three functions now accept ParseOptions as the second argument directly via new overloads and runtime Array.isArray() disambiguation. #480 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
#480 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The disambiguation logic dropped optionsParam whenever the second
argument was undefined, so getDocPage(parser, undefined, { annotations })
silently lost the annotations. Use a three-way condition: array → args;
non-null non-array → ParseOptions; undefined → fall through to the
third argument.
#480
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The new options-as-second-argument overloads required ParseOptions (non-optional), so callers forwarding an optional options variable (ParseOptions | undefined) got a type error. Make the parameter optional in all five new overloads. #480 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace two overload groups (options-only and args+options) with a single union parameter readonly string[] | ParseOptions per function. This makes the API fully composable: wrappers forwarding the union type no longer need casts or type guards. getDocPageSync and getDocPageAsync lose their overloads entirely; getDocPage drops from 6 overloads to 3 (one per mode). #480 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the usability and robustness of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #739 +/- ##
=======================================
Coverage 92.26% 92.27%
=======================================
Files 39 39
Lines 20375 20389 +14
Branches 5646 5656 +10
=======================================
+ Hits 18800 18814 +14
Misses 1530 1530
Partials 45 45 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue in getDocPage(), getDocPageSync(), and getDocPageAsync() where ParseOptions were misinterpreted when passed as the second argument. The functions have been updated to directly accept ParseOptions in this position, simplifying the process of passing annotations. New test cases validate this behavior, and CHANGES.md has been updated. The review feedback suggests refactoring getDocPageAsync and getDocPage to consolidate argument and option resolution logic, making the implementations more concise and maintainable. Additionally, getDocPageAsync should ensure synchronous results are wrapped in Promise.resolve() to adhere to the require-await lint rule.
|
@codex review |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis PR updates getDocPage, getDocPageSync, and getDocPageAsync so their second parameter accepts a union Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 docstrings
🧪 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/parser.ts`:
- Around line 879-907: getDocPageAsync currently calls getDocPageSyncImpl()
eagerly (via Promise.resolve(...)) and manually checks parser.$mode, which can
throw synchronously and violates the dispatchByMode guideline; refactor
getDocPageAsync to use dispatchByMode(parser, () => getDocPageSyncImpl(parser,
args, opts), () => getDocPageAsyncImpl(parser, args, opts)) so the sync path is
lazily invoked (or make the wrapper async and use dispatchByMode with functions)
and remove manual $mode checks and type assertions, mirroring the parse() /
suggest() pattern so all thrown errors become promise rejections.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3ac777e7-cbab-4b28-92a8-9549c6847aad
📒 Files selected for processing (3)
CHANGES.mdpackages/core/src/parser.test.tspackages/core/src/parser.ts
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/parser.ts`:
- Around line 971-995: The function getDocPage currently branches on
parser.$mode and uses type assertions; replace that manual branching with a call
to dispatchByMode(parser, handlers) so the mode-dispatched return types are
handled for you: call dispatchByMode with a sync handler that returns
getDocPageSyncImpl(parser as Parser<"sync",...,>, args, options) and an async
handler that returns getDocPageAsyncImpl(parser, args, options); ensure both the
array-argument and no-argument paths delegate into dispatchByMode (pass [] when
argsOrOptions is not an array) and remove the explicit parser.$mode checks and
type-asserting branches.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c19d3a6e-6957-4a6a-bf75-eebdb78e39b9
📒 Files selected for processing (3)
CHANGES.mdpackages/core/src/parser.test.tspackages/core/src/parser.ts
| export function getDocPage( | ||
| parser: Parser<Mode, unknown, unknown>, | ||
| args: readonly string[] = [], | ||
| argsOrOptions?: readonly string[] | ParseOptions, | ||
| options?: ParseOptions, | ||
| ): DocPage | undefined | Promise<DocPage | undefined> { | ||
| if (Array.isArray(argsOrOptions)) { | ||
| if (parser.$mode === "sync") { | ||
| return getDocPageSyncImpl( | ||
| parser as Parser<"sync", unknown, unknown>, | ||
| argsOrOptions, | ||
| options, | ||
| ); | ||
| } | ||
| return getDocPageAsyncImpl(parser, argsOrOptions, options); | ||
| } | ||
| const resolved = (argsOrOptions as ParseOptions | undefined) ?? options; | ||
| if (parser.$mode === "sync") { | ||
| return getDocPageSyncImpl( | ||
| parser as Parser<"sync", unknown, unknown>, | ||
| args, | ||
| options, | ||
| [], | ||
| resolved, | ||
| ); | ||
| } | ||
| return getDocPageAsyncImpl(parser, args, options); | ||
| return getDocPageAsyncImpl(parser, [], resolved); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer dispatchByMode() over manual mode checks.
The implementation uses manual parser.$mode === "sync" branching. Per coding guidelines, mode-based type returns should use dispatchByMode() from mode-dispatch.ts instead of manual mode checks with type assertions.
♻️ Suggested refactor using dispatchByMode
export function getDocPage(
parser: Parser<Mode, unknown, unknown>,
argsOrOptions?: readonly string[] | ParseOptions,
options?: ParseOptions,
): DocPage | undefined | Promise<DocPage | undefined> {
if (Array.isArray(argsOrOptions)) {
- if (parser.$mode === "sync") {
- return getDocPageSyncImpl(
- parser as Parser<"sync", unknown, unknown>,
- argsOrOptions,
- options,
- );
- }
- return getDocPageAsyncImpl(parser, argsOrOptions, options);
+ return dispatchByMode(
+ parser.$mode,
+ () =>
+ getDocPageSyncImpl(
+ parser as Parser<"sync", unknown, unknown>,
+ argsOrOptions,
+ options,
+ ),
+ () => getDocPageAsyncImpl(parser, argsOrOptions, options),
+ );
}
const resolved = (argsOrOptions as ParseOptions | undefined) ?? options;
- if (parser.$mode === "sync") {
- return getDocPageSyncImpl(
- parser as Parser<"sync", unknown, unknown>,
- [],
- resolved,
- );
- }
- return getDocPageAsyncImpl(parser, [], resolved);
+ return dispatchByMode(
+ parser.$mode,
+ () =>
+ getDocPageSyncImpl(
+ parser as Parser<"sync", unknown, unknown>,
+ [],
+ resolved,
+ ),
+ () => getDocPageAsyncImpl(parser, [], resolved),
+ );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/parser.ts` around lines 971 - 995, The function getDocPage
currently branches on parser.$mode and uses type assertions; replace that manual
branching with a call to dispatchByMode(parser, handlers) so the mode-dispatched
return types are handled for you: call dispatchByMode with a sync handler that
returns getDocPageSyncImpl(parser as Parser<"sync",...,>, args, options) and an
async handler that returns getDocPageAsyncImpl(parser, args, options); ensure
both the array-argument and no-argument paths delegate into dispatchByMode (pass
[] when argsOrOptions is not an array) and remove the explicit parser.$mode
checks and type-asserting branches.
Resolve args and options before the mode-based dispatch so each function has a single mode branch instead of duplicating it across the array/non-array paths. #739 (comment) #739 (comment) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where getDocPage(), getDocPageSync(), and getDocPageAsync() would misinterpret a ParseOptions object as an args array when passed as the second argument. The functions have been updated to correctly accept ParseOptions directly as the second argument, simplifying the process of passing annotations without needing an explicit empty args array. New test cases were added to validate this behavior across various scenarios, and the CHANGES.md file has been updated to document this fix and API enhancement. No feedback to provide.
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ 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.
♻️ Duplicate comments (1)
packages/core/src/parser.ts (1)
888-895:⚠️ Potential issue | 🟠 MajorDon't let sync parser exceptions escape
getDocPageAsync().
Promise.resolve(getDocPageSyncImpl(...))evaluates the sync path before the Promise exists, so exceptions fromparse()orgetDocFragments()still throw synchronously instead of rejecting. That breaks callers relying on.catch()from this always-Promise API.♻️ Proposed fix
if (parser.$mode === "sync") { - return Promise.resolve( - getDocPageSyncImpl( - parser as Parser<"sync", unknown, unknown>, - args, - opts, - ), - ); + try { + return Promise.resolve( + getDocPageSyncImpl( + parser as Parser<"sync", unknown, unknown>, + args, + opts, + ), + ); + } catch (error) { + return Promise.reject(error); + } }In JavaScript, does `Promise.resolve(fn())` convert a synchronous exception thrown by `fn()` into a rejected promise, or does the exception escape before the promise is created?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/parser.ts` around lines 888 - 895, The sync branch currently calls Promise.resolve(getDocPageSyncImpl(...)) which evaluates getDocPageSyncImpl synchronously and lets exceptions escape; change it to defer evaluation so sync exceptions become rejected promises—e.g., wrap the call in a microtask (use Promise.resolve().then(() => getDocPageSyncImpl(...))) or catch and return Promise.reject; update the branch that checks parser.$mode === "sync" and replace the current Promise.resolve(getDocPageSyncImpl(...)) pattern so getDocPageSyncImpl (the sync path) is invoked inside a .then callback (or try/catch -> Promise.reject) to ensure errors are delivered as rejected promises to callers of getDocPageAsync().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/core/src/parser.ts`:
- Around line 888-895: The sync branch currently calls
Promise.resolve(getDocPageSyncImpl(...)) which evaluates getDocPageSyncImpl
synchronously and lets exceptions escape; change it to defer evaluation so sync
exceptions become rejected promises—e.g., wrap the call in a microtask (use
Promise.resolve().then(() => getDocPageSyncImpl(...))) or catch and return
Promise.reject; update the branch that checks parser.$mode === "sync" and
replace the current Promise.resolve(getDocPageSyncImpl(...)) pattern so
getDocPageSyncImpl (the sync path) is invoked inside a .then callback (or
try/catch -> Promise.reject) to ensure errors are delivered as rejected promises
to callers of getDocPageAsync().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 202affc8-5d0b-48ba-bba3-31b5e5ba49f1
📒 Files selected for processing (1)
packages/core/src/parser.ts
1 participant
Summary
getDocPage(),getDocPageSync(), andgetDocPageAsync()accepted an optionalargsarray as their second parameter and an optionalParseOptionsas their third. Becauseargswas optional, it was natural to expectgetDocPage(parser, { annotations })to work, but the implementation always treated the second positional argument asargs, silently ignoring the annotations. Users had to writegetDocPage(parser, [], { annotations })instead.This PR replaces the separate
args?andoptions?parameters with a single union parameterargsOrOptions?: readonly string[] | ParseOptions, disambiguated at runtime viaArray.isArray(). The same pattern is already used byrunParser()in facade.ts. This change also reducesgetDocPage()'s overloads from six to three (one per mode), and eliminates overloads entirely forgetDocPageSync()andgetDocPageAsync().All of the following call shapes now work correctly at both the type level and runtime:
Fixes #480.
Test plan
getDocFragmentsreads annotations from state and embeds them in the footer(parser, undefined, { annotations })three-argument forwarding patternParseOptions | undefinedas the second argumentreadonly string[] | ParseOptionscompiles and worksmise testpasses across all runtimes (Deno, Node.js, Bun)