Added support to skip presubmit review via description tags #9

bstanga · 2024-11-18T09:27:08Z

No description provided.

github-actions · 2024-11-18T09:27:26Z

📖 Walkthrough

Added functionality to skip presubmit review when specific tags are present in the PR description. Users can now add tags like '@presubmit ignore', '@presubmit skip', '@presubmitai ignore', or '@presubmitai skip' to bypass the automated review process.

Changes

File	Summary
`src/pull_request.ts`	Implemented `shouldIgnorePullRequest` function to check for specific ignore/skip tags in PR descriptions. Added early return in `handlePullRequest` when skip conditions are met. Includes null-safe handling of PR body text.

autogenerated by presubmit.ai

github-actions

Review complete

github-actions · 2024-11-18T09:27:49Z

src/pull_request.ts

+function shouldIgnorePullRequest(pull_request: { body?: string }) {
+  const ignorePhrases = [
+    "@presubmit ignore",
+    "@presubmit: ignore",
+    "@presubmit skip",
+    "@presubmit: skip",
+    "@presubmitai ignore",
+    "@presubmitai: ignore",
+    "@presubmitai skip",
+    "@presubmitai: skip",
+  ];
+  for (const phrase of ignorePhrases) {
+    if (pull_request.body?.includes(phrase)) {
+      return true;
+    }
+  }
+  return false;
+}


The current implementation performs case-sensitive matching for ignore phrases. Consider making the matching case-insensitive to improve user experience. Users might type the commands in different cases (e.g., '@presubmit IGNORE' or '@presubmit: Skip'). You can achieve this by converting both the PR body and the phrases to lowercase before comparison:

function shouldIgnorePullRequest(pull_request: { body?: string }) { const ignorePhrases = [...]; // same phrases as before const bodyLower = pull_request.body?.toLowerCase(); for (const phrase of ignorePhrases) { if (bodyLower?.includes(phrase.toLowerCase())) { return true; } } return false; }

github-actions · 2024-11-18T09:27:49Z

src/pull_request.ts

@@ -32,6 +32,13 @@ export async function handlePullRequest() {

  const octokit = initOctokit(config.githubToken);

+  if (shouldIgnorePullRequest(pull_request)) {
+    info(
+      `ignoring pull request because of '@presubmitai ignore' in description`


The log message specifically mentions '@presubmitai ignore' but the function accepts multiple variations of the ignore command. Consider making the message more generic or including the actual phrase that was matched:

`ignoring pull request due to skip command in description` // or `ignoring pull request due to '${matchedPhrase}' in description`

github-actions · 2024-11-18T09:27:49Z

src/pull_request.ts

@@ -278,3 +285,22 @@ async function submitReview(
    );
  }
 }
+
+function shouldIgnorePullRequest(pull_request: { body?: string }) {


Consider adding JSDoc documentation to describe the function's purpose, parameters, and return value. This will improve code maintainability and help other developers understand the function's behavior:

/** * Checks if the pull request should be ignored based on special commands in its description. * @param pull_request - The pull request object containing the description * @param pull_request.body - The description text of the pull request * @returns {boolean} True if the PR should be ignored, false otherwise */ function shouldIgnorePullRequest(pull_request: { body?: string }) {

github-actions

Review complete

github-actions · 2024-11-18T09:30:32Z

src/pull_request.ts

+  const bodyLower = pull_request.body?.toLowerCase();
+


The optional chaining (?.) followed by toLowerCase() could lead to unexpected behavior. If pull_request.body is undefined, bodyLower will also be undefined. However, on line 300, you're calling includes() on bodyLower which could be undefined. Consider combining the optional chaining with nullish coalescing to provide a default empty string:

const bodyLower = (pull_request.body ?? '').toLowerCase();

This ensures bodyLower is always a string, making the subsequent includes() call safe.

Thanks, fixed

github-actions · 2024-11-18T09:30:32Z

src/pull_request.ts

+  const ignorePhrases = [
+    "@presubmit ignore",
+    "@presubmit: ignore",
+    "@presubmit skip",
+    "@presubmit: skip",
+    "@presubmitai ignore",
+    "@presubmitai: ignore",
+    "@presubmitai skip",
+    "@presubmitai: skip",
+  ];


The current array of ignore phrases could be generated programmatically to reduce repetition and make maintenance easier. Consider refactoring to use a more structured approach:

const commands = ['ignore', 'skip']; const prefixes = ['@presubmit', '@presubmitai']; const separators = ['', ': ']; const ignorePhrases = prefixes.flatMap(prefix => commands.flatMap(command => separators.map(sep => `${prefix}${sep}${command}`) ) );

This approach makes it easier to add new commands, prefixes, or separator variations in the future while reducing the chance of typos.

Not a good comment

github-actions

Review complete

github-actions · 2024-11-18T09:32:47Z

src/pull_request.ts

+    "@presubmitai skip",
+    "@presubmitai: skip",
+  ];
+  const bodyLower = (pull_request.body ?? "").toLowerCase();


While the nullish coalescing operator handles null and undefined, there's an unnecessary optional chaining operator (?.) used on line 300 when calling includes(). Since bodyLower is already guaranteed to be a string, you can remove the optional chaining:

if (bodyLower.includes(phrase.toLowerCase())) {

github-actions · 2024-11-18T09:32:47Z

src/pull_request.ts

+function shouldIgnorePullRequest(pull_request: { body?: string }) {
+  const ignorePhrases = [
+    "@presubmit ignore",
+    "@presubmit: ignore",
+    "@presubmit skip",
+    "@presubmit: skip",
+    "@presubmitai ignore",
+    "@presubmitai: ignore",
+    "@presubmitai skip",
+    "@presubmitai: skip",
+  ];
+  const bodyLower = (pull_request.body ?? "").toLowerCase();
+
+  for (const phrase of ignorePhrases) {
+    if (bodyLower?.includes(phrase.toLowerCase())) {
+      info(`ignoring pull request because of '${phrase}' in description`);
+      return true;
+    }
+  }
+  return false;
+}


Consider adding more detailed logging when a PR is ignored. It would be helpful to log the PR number/URL along with the ignore phrase for better traceability. For example:

info(`Ignoring pull request #${pull_request.number} (${pull_request.html_url}) due to '${phrase}' in description`);

This would make it easier to track which PRs are being skipped and why in the logs.

github-actions

Review complete

github-actions · 2024-11-18T09:34:33Z

src/pull_request.ts

@@ -278,3 +282,25 @@ async function submitReview(
    );
  }
 }
+
+function shouldIgnorePullRequest(pull_request: { body?: string }) {


Consider using a more specific type for the pull_request parameter. Instead of using an inline type with just the body property, you could define a proper interface or type that better represents the pull request object. This would make the code more maintainable and type-safe. For example:

interface PullRequest { body?: string; // other relevant properties } function shouldIgnorePullRequest(pull_request: PullRequest)

github-actions · 2024-11-18T09:34:33Z

src/pull_request.ts

+  const bodyLower = (pull_request.body ?? "").toLowerCase();
+
+  for (const phrase of ignorePhrases) {
+    if (bodyLower.includes(phrase.toLowerCase())) {
+      info(`ignoring pull request because of '${phrase}' in description`);
+      return true;
+    }
+  }


The function could be simplified using the some() array method, which would make the code more concise and functional. Here's a suggested refactoring:

return ignorePhrases.some(phrase => { const matches = bodyLower.includes(phrase.toLowerCase()); if (matches) { info(`ignoring pull request because of '${phrase}' in description`); } return matches; });

github-actions · 2024-11-18T09:34:33Z

src/pull_request.ts

+  const ignorePhrases = [
+    "@presubmit ignore",
+    "@presubmit: ignore",
+    "@presubmit skip",
+    "@presubmit: skip",
+    "@presubmitai ignore",
+    "@presubmitai: ignore",
+    "@presubmitai skip",
+    "@presubmitai: skip",
+  ];


Consider moving the ignore phrases to a configuration file. This would make it easier to maintain and modify the supported phrases without changing the code. You could create a config.ts file that exports these phrases:

// config.ts export const IGNORE_PHRASES = [ '@presubmit ignore', // ... other phrases ];

This approach would also make it easier to test and modify the supported phrases without touching the core logic.

bstanga

Review complete

bstanga · 2024-11-18T10:02:38Z

src/pull_request.ts

+  if (shouldIgnorePullRequest(pull_request)) {
+    return;
+  }


Consider adding a log message before returning to explicitly indicate that the PR review is being skipped. This would improve observability and make it clearer in logs why no review was performed. For example:

if (shouldIgnorePullRequest(pull_request)) { info('Skipping PR review due to ignore tag in description'); return; }

bstanga · 2024-11-18T10:02:38Z

src/pull_request.ts

+  const bodyLower = (pull_request.body ?? "").toLowerCase();
+


The PR body text should be trimmed before checking for ignore phrases. Without trimming, extra whitespace could prevent matching valid ignore tags. Consider:

const bodyLower = (pull_request.body ?? "").toLowerCase().trim();

bstanga · 2024-11-18T10:02:38Z

src/pull_request.ts

+function shouldIgnorePullRequest(pull_request: { body?: string }) {
+  const ignorePhrases = [
+    "@presubmit ignore",
+    "@presubmit: ignore",
+    "@presubmit skip",
+    "@presubmit: skip",
+    "@presubmitai ignore",
+    "@presubmitai: ignore",
+    "@presubmitai skip",
+    "@presubmitai: skip",
+  ];
+  const bodyLower = (pull_request.body ?? "").toLowerCase();
+
+  for (const phrase of ignorePhrases) {
+    if (bodyLower.includes(phrase.toLowerCase())) {
+      info(`ignoring pull request because of '${phrase}' in description`);
+      return true;
+    }
+  }
+  return false;
+}


This new functionality should be accompanied by unit tests to verify the behavior of shouldIgnorePullRequest(). Tests should cover various scenarios including:

Different ignore phrase formats

Case sensitivity

Empty PR description

PR description with whitespace around ignore phrases

PR description without any ignore phrases

bstanga

Review complete

bstanga · 2024-11-18T10:07:31Z

src/pull_request.ts

+  for (const phrase of ignorePhrases) {
+    if (bodyLower.includes(phrase.toLowerCase())) {


The toLowerCase() is being called on each phrase inside the loop, even though the phrases are constant strings. Consider converting the ignorePhrases array to lowercase when it's defined to avoid redundant string transformations:

const ignorePhrases = [ '@presubmit ignore', // ... other phrases ].map(phrase => phrase.toLowerCase()); // Then in the loop if (bodyLower.includes(phrase)) {

This way, the lowercase conversion happens only once per phrase instead of on every iteration.

bstanga · 2024-11-18T10:07:31Z

src/pull_request.ts

+  const ignorePhrases = [
+    "@presubmit ignore",
+    "@presubmit: ignore",
+    "@presubmit skip",
+    "@presubmit: skip",
+    "@presubmitai ignore",
+    "@presubmitai: ignore",
+    "@presubmitai skip",
+    "@presubmitai: skip",
+  ];


Instead of having a fixed list of phrases, consider using a regex pattern that would be more flexible and maintainable. This would also handle variations in whitespace more elegantly:

const IGNORE_REGEX = /^@presubmit(?:ai)?(?:\s*:\s*|\s+)(ignore|skip)$/im; function shouldIgnorePullRequest(pull_request: { body?: string }) { const body = pull_request.body ?? ""; const matches = body.split('\n').some(line => IGNORE_REGEX.test(line.trim())); if (matches) { info('ignoring pull request due to ignore/skip command in description'); } return matches; }

This approach:

Handles variations in whitespace more robustly

Matches commands only at the start of a line (preventing false positives)

Is more maintainable as it's a single pattern instead of multiple strings

Can be easily extended to support additional variations

bstanga

Review complete

bstanga · 2024-11-18T11:25:21Z

src/pull_request.ts

+  if (shouldIgnorePullRequest(pull_request)) {
+    return;
+  }


Consider adding a more detailed log message before returning to provide better context about why the PR review is being skipped. This would improve observability and debugging. For example:

if (shouldIgnorePullRequest(pull_request)) { info(`Skipping PR review due to ignore tag in description: ${pull_request.body}`); return; }

bstanga · 2024-11-18T11:25:21Z

src/pull_request.ts

@@ -278,3 +282,25 @@ async function submitReview(
    );
  }
 }
+
+function shouldIgnorePullRequest(pull_request: { body?: string }) {


Add JSDoc documentation to describe the function's purpose, parameters, and return value. This will improve code maintainability and help other developers understand the function's behavior:

/** * Checks if the pull request should be ignored based on special commands in its description. * @param pull_request - The pull request object containing the description * @param pull_request.body - The description text of the pull request * @returns {boolean} True if the PR should be ignored, false otherwise */ function shouldIgnorePullRequest(pull_request: { body?: string }) {

bstanga · 2024-11-18T11:25:21Z

src/pull_request.ts

+  const ignorePhrases = [
+    "@presubmit ignore",
+    "@presubmit: ignore",
+    "@presubmit skip",
+    "@presubmit: skip",
+    "@presubmitai ignore",
+    "@presubmitai: ignore",
+    "@presubmitai skip",
+    "@presubmitai: skip",
+  ];


The current array of ignore phrases is repetitive and could be generated programmatically to reduce duplication and improve maintainability. Consider using a more structured approach:

const commands = ['ignore', 'skip']; const prefixes = ['@presubmit', '@presubmitai']; const separators = ['', ': ']; const ignorePhrases = prefixes.flatMap(prefix => commands.flatMap(command => separators.map(sep => `${prefix}${sep}${command}`) ) );

This makes it easier to add new commands or variations while reducing the chance of typos.

bstanga · 2024-11-18T11:25:21Z

src/pull_request.ts

+    "@presubmitai skip",
+    "@presubmitai: skip",
+  ];
+  const bodyLower = (pull_request.body ?? "").toLowerCase();


The PR body text should be trimmed before checking for ignore phrases. Without trimming, extra whitespace could prevent matching valid ignore tags. Consider:

const bodyLower = (pull_request.body ?? "").toLowerCase().trim();

bstanga · 2024-11-18T11:25:22Z

src/pull_request.ts

+  for (const phrase of ignorePhrases) {
+    if (bodyLower.includes(phrase.toLowerCase())) {
+      info(`ignoring pull request because of '${phrase}' in description`);
+      return true;
+    }
+  }


The code could be simplified and made more efficient using the some() array method. Additionally, toLowerCase() is being called unnecessarily on each iteration for constant strings. Consider:

const ignorePhrases = [ '@presubmit ignore', // ... other phrases ].map(phrase => phrase.toLowerCase()); return ignorePhrases.some(phrase => { const matches = bodyLower.includes(phrase); if (matches) { info(`ignoring pull request because of '${phrase}' in description`); } return matches; });

bstanga

Pull request needs attention❗

Review Summary

Commits Considered (1)

1354a84: fixed potential undefined error

Files Processed (1)

src/pull_request.ts (2 hunks)

Actionable Comments (1)

Missing error handling for invalid pull request object: The function call shouldIgnorePullRequest(pull_request) assumes that pull_request is properly defined. Consider adding a null check to handle cases where the pull request object might be undefined:

if (!pull_request) {
  error('Pull request object is undefined');
  return;
}

if (shouldIgnorePullRequest(pull_request)) {
  return;
}

Non-critical Comments (2)

enhancement [297-304]

for (const phrase of ignorePhrases) {
  if (bodyLower.includes(phrase.toLowerCase())) {
    info(`ignoring pull request because of '${phrase}' in description`);
    return true;
  }
}```
The current implementation might match ignore phrases that appear in the middle of sentences or code blocks. Consider splitting the description into lines and checking each line separately to ensure the ignore phrase appears as a standalone command:

const lines = bodyLower.split('\n');
for (const phrase of ignorePhrases) {
  if (lines.some(line => line.trim() === phrase.toLowerCase())) {
    info(`ignoring pull request because of '${phrase}' in description`);
    return true;
  }
}

enhancement [287-296]

const ignorePhrases = [
  "@presubmit ignore",
  "@presubmit: ignore",
  "@presubmit skip",
  "@presubmit: skip",
  "@presubmitai ignore",
  "@presubmitai: ignore",
  "@presubmitai skip",
  "@presubmitai: skip",
];```
The current implementation only checks the PR description for ignore phrases. Consider extending the functionality to also check for these phrases in PR comments, allowing users to skip the review process after the PR has been created:

const ignorePhrases = [
  '@presubmit ignore',
  '@presubmit: ignore',
  // ... existing phrases ...
  // Consider adding these to a shared config
];

This would require fetching and checking PR comments as well.

autogenerated by presubmit.ai

bstanga · 2024-11-18T12:03:42Z

src/pull_request.ts

+  if (shouldIgnorePullRequest(pull_request)) {
+    return;
+  }


The function call shouldIgnorePullRequest(pull_request) assumes that pull_request is properly defined. Consider adding a null check to handle cases where the pull request object might be undefined:

if (!pull_request) { error('Pull request object is undefined'); return; } if (shouldIgnorePullRequest(pull_request)) { return; }

bstanga

🚨 Pull request needs attention.

Review Summary

Commits Considered (1)

1354a84: fixed potential undefined error

Files Processed (1)

src/pull_request.ts (2 hunks)

Actionable Comments (2)

src/pull_request.ts [35-37]
> possible bug: Missing error handling for undefined pull request object
src/pull_request.ts [297-297]
> possible issue: Missing string sanitization

Skipped Comments (1)

**src/pull_request.ts** [35-37]
> enhancement: Missing logging for skipped review

bstanga · 2024-11-18T12:33:05Z

src/pull_request.ts

+  if (shouldIgnorePullRequest(pull_request)) {
+    return;
+  }


The code assumes pull_request is properly defined. Consider adding a null check to handle cases where the pull request object might be undefined:

if (!pull_request) { error('Pull request object is undefined'); return; } if (shouldIgnorePullRequest(pull_request)) { return; }

bstanga · 2024-11-18T12:33:05Z

src/pull_request.ts

+    "@presubmitai skip",
+    "@presubmitai: skip",
+  ];
+  const bodyLower = (pull_request.body ?? "").toLowerCase();


The PR body text should be trimmed before checking for ignore phrases. Without trimming, extra whitespace at the start or end could prevent matching valid ignore tags. Consider:

const bodyLower = (pull_request.body ?? "").toLowerCase().trim();

github-actions

✅ LGTM!

Review Summary

Commits Considered (1)

dfa3904: Merge branch 'main' of https://github.com/presubmit/ai-reviewer into dev

Files Processed (1)

src/pull_request.ts (2 hunks)

Actionable Comments (0)

Skipped Comments (2)

src/pull_request.ts [321-321]

enhancement: "Consider handling multiline PR descriptions more robustly"
src/pull_request.ts [324-327]

enhancement: "Consider consolidating logging with return value"

add support to skip presubmit review with tag in descripion

d91d78c