feat: add approve command - #3145
Draft
alepane21 wants to merge 2 commits into
Draft
Conversation
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Contributor
WalkthroughChangesProposal status update
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3145 +/- ##
===========================================
- Coverage 62.37% 45.34% -17.03%
===========================================
Files 262 148 -114
Lines 31003 14073 -16930
Branches 0 830 +830
===========================================
- Hits 19337 6381 -12956
+ Misses 10158 7690 -2468
+ Partials 1508 2 -1506
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cli/src/commands/proposal/commands/approve.ts (1)
7-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit command and action types.
Add a
Commandreturn type to the command factory.Define an interface for the action options.
Annotate
name,options, and the action return type.Proposed change
+interface ApproveProposalOptions { + federationGraph: string; + namespace: string; +} + -export default (opts: BaseCommandOptions) => { +export default (opts: BaseCommandOptions): Command => { ... - command.action(async (name, options) => { + command.action(async (name: string, options: ApproveProposalOptions): Promise<void> => {🤖 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 `@cli/src/commands/proposal/commands/approve.ts` around lines 7 - 17, Update the default command factory around the approve command to explicitly return Command. Define an interface for the action options matching the federationGraph and namespace options, then annotate the action’s name, options, and return type while preserving the existing command behavior.Source: Coding guidelines
🤖 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 `@cli/src/commands/proposal/commands/approve.ts`:
- Line 15: Require a namespace argument in the approve command by changing the
Commander option in cli/src/commands/proposal/commands/approve.ts:15 from an
optional value to a required <namespace> value, while preserving the default
behavior when the option is omitted; update
docs-website/cli/proposal/approve.mdx:26 from [namespace] to <namespace>; add a
regression test verifying parsing fails when --namespace has no value.
---
Nitpick comments:
In `@cli/src/commands/proposal/commands/approve.ts`:
- Around line 7-17: Update the default command factory around the approve
command to explicitly return Command. Define an interface for the action options
matching the federationGraph and namespace options, then annotate the action’s
name, options, and return type while preserving the existing command behavior.
🪄 Autofix
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
Run ID: 7171e895-d782-4f9a-83d9-b68a00409385
📒 Files selected for processing (7)
cli/src/commands/proposal/commands/approve.tscli/src/commands/proposal/index.tscli/test/approve-proposal.test.tsdocs-website/cli/proposal.mdxdocs-website/cli/proposal/approve.mdxdocs-website/concepts/proposals.mdxdocs-website/docs.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cli/src/commands/proposal/commands/update-status.ts (2)
7-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the status constant.
proposalStatusesis a module-level constant. Rename it toPROPOSAL_STATUSES.As per coding guidelines, constants use UPPER_SNAKE_CASE.
🤖 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 `@cli/src/commands/proposal/commands/update-status.ts` around lines 7 - 15, Rename the module-level proposalStatuses constant to PROPOSAL_STATUSES and update its references within parseProposalStatus, preserving the existing status validation behavior.Source: Coding guidelines
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit function types.
Add a
Commandreturn type to the exported command factory. Add parameter andPromise<void>return types to the action callback. Use an interface for the action options object.As per coding guidelines, TypeScript functions need explicit parameter and return types, and object shapes use interfaces.
Also applies to: 33-33
🤖 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 `@cli/src/commands/proposal/commands/update-status.ts` at line 18, Update the default command factory to explicitly return Command and type its opts parameter with the appropriate interface. Define an interface for the action options object, then annotate the action callback parameter with that interface and its return type as Promise<void>.Source: Coding guidelines
🤖 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 `@cli/src/commands/proposal/commands/update-status.ts`:
- Line 31: Require an explicit value for the namespace option by changing the
option declaration in the update-status command from an optional argument to a
required one, using the existing namespace option symbol. Update the
corresponding usage documentation in update-status.mdx to show the required
namespace argument.
---
Nitpick comments:
In `@cli/src/commands/proposal/commands/update-status.ts`:
- Around line 7-15: Rename the module-level proposalStatuses constant to
PROPOSAL_STATUSES and update its references within parseProposalStatus,
preserving the existing status validation behavior.
- Line 18: Update the default command factory to explicitly return Command and
type its opts parameter with the appropriate interface. Define an interface for
the action options object, then annotate the action callback parameter with that
interface and its return type as Promise<void>.
🪄 Autofix
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
Run ID: 08778819-cc9b-4b12-9ddd-2051f87710bb
📒 Files selected for processing (7)
cli/src/commands/proposal/commands/update-status.tscli/src/commands/proposal/index.tscli/test/update-proposal-status.test.tsdocs-website/cli/proposal.mdxdocs-website/cli/proposal/update-status.mdxdocs-website/concepts/proposals.mdxdocs-website/docs.json
🚧 Files skipped from review as they are similar to previous changes (2)
- docs-website/docs.json
- docs-website/cli/proposal.mdx
| 'The status to set. Allowed values: approved, closed.', | ||
| parseProposalStatus, | ||
| ); | ||
| command.option('-n, --namespace [string]', 'The namespace of the federated graph.', 'default'); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- relevant files ---'
fd -i 'update-status|package.json|package-lock|yarn.lock|pnpm-lock' . | head -80
printf '%s\n' '--- command and documentation references ---'
rg -n -C 8 --glob 'update-status.ts' --glob 'update-status.mdx' -- '--namespace|namespace' .
printf '%s\n' '--- Commander declarations ---'
rg -n -C 3 --glob 'package.json' --glob '*lock*' '"commander"|commander@' . | head -120
printf '%s\n' '--- namespace call sites and types ---'
rg -n -C 5 --glob '*.{ts,tsx,js,jsx}' 'updateStatus|namespace' cli/src | head -240Repository: wundergraph/cosmo
Length of output: 25824
🏁 Script executed:
#!/bin/bash
set -eu
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
curl -fsSL https://unpkg.com/commander@11.1.0/index.js -o "$tmpdir/index.js"
curl -fsSL https://unpkg.com/commander@11.1.0/lib/command.js -o "$tmpdir/command.js"
curl -fsSL https://unpkg.com/commander@11.1.0/lib/option.js -o "$tmpdir/option.js"
curl -fsSL https://unpkg.com/commander@11.1.0/package.json -o "$tmpdir/package.json"
printf '%s\n' '--- Commander option parsing source ---'
rg -n -C 6 'optional|required|true|argNot|parseOptions|value' "$tmpdir/command.js" "$tmpdir/option.js" | head -220
printf '%s\n' '--- standalone Commander 11.1.0 behavior probe ---'
cat > "$tmpdir/probe.mjs" <<'JS'
import { Command } from './index.js';
for (const optionSpec of ['-n, --namespace [string]', '-n, --namespace <namespace>']) {
for (const argv of [
['node', 'probe', '--namespace'],
['node', 'probe', '--namespace', 'production'],
]) {
const command = new Command();
command.exitOverride();
command.option(optionSpec, 'namespace', 'default');
try {
command.parse(argv);
console.log(JSON.stringify({
optionSpec,
argv: argv.slice(2),
namespace: command.opts().namespace,
namespaceType: typeof command.opts().namespace,
}));
} catch (error) {
console.log(JSON.stringify({
optionSpec,
argv: argv.slice(2),
error: error.code ?? error.message,
}));
}
}
}
JS
node "$tmpdir/probe.mjs"
printf '%s\n' '--- updateProposal declarations and tests ---'
rg -n -C 5 --glob '*.{ts,tsx}' 'updateProposal' cli/src cli/test cli/tests 2>/dev/null | head -180 || true
rg -n -C 4 --glob '*.{ts,tsx}' 'proposal update-status|update-status' cli | head -180 || trueRepository: wundergraph/cosmo
Length of output: 14478
🏁 Script executed:
#!/bin/bash
set -eu
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
mkdir "$tmpdir/commander"
curl -fsSL https://registry.npmjs.org/commander/-/commander-11.1.0.tgz | tar -xzf - -C "$tmpdir/commander" --strip-components=1
printf '%s\n' '--- standalone Commander 11.1.0 behavior probe ---'
cat > "$tmpdir/probe.cjs" <<'JS'
const { Command } = require('./commander');
for (const optionSpec of ['-n, --namespace [string]', '-n, --namespace <namespace>']) {
for (const argv of [
['node', 'probe', '--namespace'],
['node', 'probe', '--namespace', 'production'],
]) {
const command = new Command();
command.exitOverride();
command.option(optionSpec, 'namespace', 'default');
try {
command.parse(argv);
const namespace = command.opts().namespace;
console.log(JSON.stringify({
optionSpec,
argv: argv.slice(2),
namespace,
namespaceType: typeof namespace,
}));
} catch (error) {
console.log(JSON.stringify({
optionSpec,
argv: argv.slice(2),
error: error.code ?? error.message,
}));
}
}
}
JS
(
cd "$tmpdir"
node probe.cjs
)
printf '%s\n' '--- relevant Commander parser branches ---'
rg -n -C 12 'optional|presetArg|processOption|missing argument|arg ===|value = true' "$tmpdir/commander/lib/command.js" "$tmpdir/commander/lib/option.js" | head -260
printf '%s\n' '--- updateProposal declarations and namespace types ---'
rg -n -C 8 --glob '*.{ts,tsx}' 'updateProposal' cli | head -220 || true
rg -n -C 5 --glob '*.{ts,tsx}' 'namespace\??: string|namespace: string|namespace\?: string' cli/src cli | head -160 || trueRepository: wundergraph/cosmo
Length of output: 34369
Require a value for --namespace. Change [string] to <namespace>. Update docs-website/cli/proposal/update-status.mdx to match.
🤖 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 `@cli/src/commands/proposal/commands/update-status.ts` at line 31, Require an
explicit value for the namespace option by changing the option declaration in
the update-status command from an optional argument to a required one, using the
existing namespace option symbol. Update the corresponding usage documentation
in update-status.mdx to show the required namespace argument.
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 by CodeRabbit
New Features
Documentation
Tests
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.