fetch previous comments

[bstanga]

No description provided.

[github-actions]

📖 Walkthrough

Enhanced the PR review functionality by implementing the ability to fetch and display existing comment chains in the code diff. Added incremental review support to only analyze new changes since the last review, and improved the loading message display with better commit range information.

Changes

File	Summary
src/diff.ts	Extended FileDiff and Hunk types to include comment chains. Added ReviewComment type and implemented generateCommentChains function to associate existing comments with code hunks. Updated diff formatting to display comment threads.
src/messages.ts	Added payload tags for storing review metadata, updated loading message to show commit range from base to head, and modified walkthrough message to include commit history in payload.
src/prompts.ts	Added instructions to avoid duplicating existing comments in review suggestions and updated example diff format to show comment thread structure.
src/pull_request.ts	Added support for incremental reviews by tracking reviewed commits, fetching review comments, and filtering files for review. Added automatic PR title updates when requested via @presubmitai mention.
dist/index.js	Generated distribution file from source changes.

---

autogenerated by presubmit.ai

[github-actions]

Review submitted

[github-actions]

Consider extracting the complex filter condition into a separate function for better readability and maintainability. For example:

function isTopLevelCommentInHunk(comment: ReviewComment, file: File, hunk: Hunk): boolean {
  const isInHunkRange = (line: number) => line <= hunk.endLine && line >= hunk.startLine;
  return !comment.in_reply_to_id && 
         comment.path === file.filename && 
         comment.body.length && 
         comment.line && 
         isInHunkRange(comment.line) &&
         (!comment.start_line || isInHunkRange(comment.start_line));
}

[github-actions]

The current JSON parsing approach could fail if the payload tags are malformed or missing. Consider adding more robust error handling and validation:

const extractPayload = (body?: string): { commits: string[] } => {
  const defaultPayload = { commits: [] };
  if (!body) return defaultPayload;
  
  const parts = body.split(PAYLOAD_TAG_OPEN);
  if (parts.length !== 2) return defaultPayload;
  
  const payloadParts = parts[1].split(PAYLOAD_TAG_CLOSE);
  if (payloadParts.length !== 2) return defaultPayload;
  
  try {
    return JSON.parse(payloadParts[0]);
  } catch {
    return defaultPayload;
  }
};

[github-actions]

Using some() inside a filter() can be inefficient for large arrays as it has O(n²) complexity. Consider using a Set for better performance:

const modifiedFiles = new Set(incrementalDiff.data.files?.map(f => f.filename));
filesToReview = filesToReview.filter(f => modifiedFiles.has(f.filename));

[github-actions]

Review submitted

[github-actions]

Consider adding a check at the beginning of the function to handle the case when reviewComments is empty or undefined. This would prevent unnecessary filtering operations and make the code more defensive. Example:

if (!reviewComments?.length) {
  return [];
}

[github-actions]

The current implementation might throw errors if PAYLOAD_TAG_OPEN or PAYLOAD_TAG_CLOSE are not found in the comment body, as split()[1] would return undefined. Consider adding more robust error handling:

const parts = overviewComment.body?.split(PAYLOAD_TAG_OPEN) || [];
if (parts.length < 2) {
  warning('Missing payload open tag');
  return { commits: [] };
}
const payloadParts = parts[1].split(PAYLOAD_TAG_CLOSE);
if (payloadParts.length < 1) {
  warning('Missing payload close tag');
  return { commits: [] };
}
const payload = JSON.parse(payloadParts[0] || '{}');

[github-actions]

The current implementation uses Array.some() inside a filter, which has O(n²) complexity. For better performance with large numbers of files, consider creating a Set of filenames first:

if (incrementalDiff?.data?.files) {
  const modifiedFiles = new Set(
    incrementalDiff.data.files.map(f => f.filename)
  );
  filesToReview = filesToReview.filter(f => 
    modifiedFiles.has(f.filename)
  );
}

[github-actions]

Before updating the PR title, consider validating that summary.title is not empty or undefined to avoid setting an invalid title. Also, the commented out code for updating the body should be removed if it's not being used. Add validation like:

if (summary.title?.trim()) {
  await octokit.rest.pulls.update({
    ...context.repo,
    pull_number: pull_request.number,
    title: summary.title,
  });
} else {
  warning('Generated title is empty, skipping title update');
}

[bstanga]

Review complete

[bstanga]

The filter operation assumes reviewComments is always an array. Consider adding a null check or default value like reviewComments ?? [] to handle cases where reviewComments might be undefined or null, preventing potential runtime errors.

[bstanga]

The current implementation might throw errors if PAYLOAD_TAG_OPEN or PAYLOAD_TAG_CLOSE are not found in the string, as array indexing with [1] and [0] could return undefined. Consider using array destructuring with default values or additional checks to make this more robust:

const parts = overviewComment.body?.split(PAYLOAD_TAG_OPEN) || [];
const payloadStr = parts[1]?.split(PAYLOAD_TAG_CLOSE)[0] || "{}";
const payload = JSON.parse(payloadStr);

[bstanga]

The optional chaining on incrementalDiff.data.files in the some() method could be moved outside the filter to avoid potential undefined access. Consider restructuring as:

const incrementalFiles = incrementalDiff?.data?.files;
if (incrementalFiles) {
  filesToReview = filesToReview.filter((f) =>
    incrementalFiles.some((f2) => f2.filename === f.filename)
  );
}

[bstanga]

Consider defining an interface or type for the payload object structure. This would improve type safety and make it easier to maintain the code as the payload structure evolves:

interface WalkthroughPayload {
  commits: string[];
}

const payload: WalkthroughPayload = {
  commits: commits,
};

[github-actions]

Review submitted

[github-actions]

The filter operation assumes reviewComments is always an array. Consider adding a null check or default value to handle cases where reviewComments might be undefined or null:

const topLevelComments = (reviewComments || []).filter(...)

[github-actions]

The current implementation might throw if PAYLOAD_TAG_OPEN or PAYLOAD_TAG_CLOSE are not found in the string, as array access with index [1] or [0] could be undefined. Consider using a more robust parsing approach:

const parts = overviewComment.body?.split(PAYLOAD_TAG_OPEN) || [];
const payloadSection = parts[1] ? parts[1].split(PAYLOAD_TAG_CLOSE)[0] : '{}';
const payload = JSON.parse(payloadSection);

[github-actions]

This can be simplified using array destructuring with the at() method which handles empty arrays gracefully:

const lastCommitReviewed = commitsReviewed.at(-1) || null;

[github-actions]

Since you've already checked for incrementalDiff?.data?.files existence in the if condition, you don't need the optional chaining operator here. You can simplify to:

filesToReview = filesToReview.filter((f) =>
  incrementalDiff.data.files.some((f2) => f2.filename === f.filename)
);

[bstanga]

Review complete

[bstanga]

The type definition could be more precise about nullable fields. Consider using union types with null explicitly where applicable, and making required fields non-optional. For example:

type ReviewComment = {
  path: string;
  body: string;
  line: number | null;
  in_reply_to_id: number | null;
  id: number;
  start_line: number | null;
  user: {
    login: string;
  };
};

This makes it clearer which fields can actually be null versus undefined.

[bstanga]

This code can be simplified using array's at() method with optional chaining, which is more concise and handles empty arrays gracefully:

const lastCommitReviewed = commitsReviewed.at(-1) ?? null;

[bstanga]

Consider adding type safety to the payload parsing by defining an interface and validating the parsed data. This would prevent runtime errors from malformed data:

interface OverviewPayload {
  commits: string[];
}

function isValidPayload(data: unknown): data is OverviewPayload {
  return (
    typeof data === 'object' &&
    data !== null &&
    Array.isArray((data as OverviewPayload).commits) &&
    (data as OverviewPayload).commits.every(c => typeof c === 'string')
  );
}

try {
  const parsed = JSON.parse(/* ... */);
  if (!isValidPayload(parsed)) {
    warning('Invalid payload structure');
    commitsReviewed = [];
  } else {
    commitsReviewed = parsed.commits;
  }
}

[bstanga]

The filtering logic could be made more readable by extracting the file matching into a separate variable or function:

if (incrementalDiff?.data?.files) {
  const modifiedFiles = new Set(incrementalDiff.data.files.map(f => f.filename));
  filesToReview = filesToReview.filter(file => modifiedFiles.has(file.filename));
}

This approach is not only more readable but also more efficient as it avoids repeated array searches.

[bstanga]

Review complete

[bstanga]

Consider using the more modern at() method with optional chaining to simplify the code and make it more readable:

const lastCommitReviewed = commitsReviewed.at(-1) ?? null;

This approach is more concise and handles empty arrays gracefully.

[bstanga]

The current implementation performs an O(n²) operation by using some() inside filter(). Consider using a Set for better performance:

const modifiedFiles = new Set(incrementalDiff.data.files.map(f => f.filename));
filesToReview = filesToReview.filter(file => modifiedFiles.has(file.filename));

This approach reduces the time complexity to O(n) and makes the code more readable.

[bstanga]

Consider adding validation for the new title before updating:

1. Remove the commented out code for better maintainability
2. Add validation for the new title to prevent setting empty or invalid titles

if (pull_request.title.includes("@presubmitai") && summary.title?.trim()) {
  info(`title contains @presubmitai, so generating a new title`);
  await octokit.rest.pulls.update({
    ...context.repo,
    pull_number: pull_request.number,
    title: summary.title
  });
} else if (pull_request.title.includes("@presubmitai")) {
  warning('Generated title is empty or invalid, skipping title update');
}

[bstanga]

Review complete

[bstanga]

The API request for review comments should be wrapped in a try-catch block to handle potential failures gracefully. Consider adding error handling:

let reviewComments = [];
if (isIncrementalReview) {
  try {
    reviewComments = (await octokit.rest.pulls.listReviewComments({
      ...context.repo,
      pull_number: pull_request.number,
    })).data;
  } catch (error) {
    warning(`Failed to fetch review comments: ${error}`);
  }
}

[bstanga]

The commit comparison API request should be wrapped in a try-catch block to handle potential failures. Consider adding error handling:

let incrementalDiff = null;
if (lastCommitReviewed && lastCommitReviewed !== pull_request.head.sha) {
  try {
    incrementalDiff = await octokit.rest.repos.compareCommits({
      ...context.repo,
      base: lastCommitReviewed,
      head: pull_request.head.sha,
    });
  } catch (error) {
    warning(`Failed to compare commits: ${error}`);
  }
}

[bstanga]

The PR update API request should be wrapped in a try-catch block to handle potential failures. Also, remove the commented out code. Consider:

if (pull_request.title.includes("@presubmitai")) {
  info(`title contains @presubmitai, so generating a new title`);
  try {
    await octokit.rest.pulls.update({
      ...context.repo,
      pull_number: pull_request.number,
      title: summary.title,
    });
    info('Successfully updated PR title');
  } catch (error) {
    warning(`Failed to update PR title: ${error}`);
  }
}

[github-actions]

Review submitted

[github-actions]

Consider removing this debug console.log statement before merging to production. Debug logs should typically be removed from production code.

[github-actions]

The JSON parsing could throw an error if the payload is malformed. While there is a try-catch block, consider adding more specific error handling and validation of the parsed data structure. Also, the fallback '{}' should be moved inside the JSON.parse() call:

JSON.parse(overviewComment.body?.split(PAYLOAD_TAG_OPEN)[1].split(PAYLOAD_TAG_CLOSE)[0] ?? '{}');

[github-actions]

The optional chaining operator (?.) is used inconsistently here. Consider using it consistently throughout this block to prevent potential runtime errors:

if (incrementalDiff?.data?.files) {
  filesToReview = filesToReview.filter((f) =>
    incrementalDiff.data?.files?.some((f2) => f2.filename === f.filename)
  );
}

[github-actions]

The condition for filtering comments is complex and could be split into smaller, more readable functions. Consider extracting the conditions into a separate function with a descriptive name, like isValidTopLevelComment. This would improve readability and maintainability:

const isValidTopLevelComment = (comment: ReviewComment, file: File, hunk: Hunk) => (
  !comment.in_reply_to_id &&
  comment.path === file.filename &&
  comment.body.length &&
  // ... rest of conditions
);

const topLevelComments = reviewComments.filter(c => isValidTopLevelComment(c, file, hunk));