Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pr-review/src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ if (process.env.NODE_ENV === "development") {
// eslint-disable-next-line @typescript-eslint/no-for-in-array, no-restricted-syntax, guard-for-in
for (const key in actionYaml.inputs) {
const envKey = `INPUT_${key.toUpperCase()}`
const envValue = actionYaml.inputs[key].default
const envValue = actionYaml.inputs[key]?.default
if (envValue && !Object.keys(process.env).includes(envKey)) {
process.env[envKey] = envValue
}
Expand Down
69 changes: 38 additions & 31 deletions pr-review/src/hunk-reader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,43 +34,50 @@ export function resolveHunkReferencesInComments(comments: AiReview["comments"],
core.warning(`Could not find file for comment on ${comment.path}, start ${comment.start}, end ${comment.end}, ${comment.comment}, skipping.`)
} else {
const hunkChangeMap = currentFile.chunks.flatMap(hunk => hunk.changes.map(change => ({ change, hunk })))
let { change: startChange, hunk: startHunk } = hunkChangeMap[comment.start - 1] // eslint-disable-line prefer-const
let { change: endChange, hunk: endHunk } = hunkChangeMap[comment.end - 1] // eslint-disable-line prefer-const
const startEntry = hunkChangeMap[comment.start - 1]
const endEntry = hunkChangeMap[comment.end - 1]

if (!startHunk) {
core.warning(`Could not find hunk for comment on ${comment.path}, start ${comment.start}, end ${comment.end}, ${comment.comment}, skipping.`)
if (!startEntry || !endEntry) {
core.error(`Could not find hunk for comment on ${comment.path}, start ${comment.start}, end ${comment.end}, ${comment.comment}, skipping.`)
} else {
if (startHunk !== endHunk) endChange = startHunk.changes.at(-1)!
const { change: startChange, hunk: startHunk } = startEntry
let { change: endChange, hunk: endHunk } = endEntry // eslint-disable-line prefer-const
Comment on lines 36 to +44

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nested destructuring and multiple let/const statements make this block hard to follow. Extract the logic to find the start and end entries into a helper, and use early returns on missing data to reduce nesting. For instance:

function getHunkEntry(index: number) {
  const entry = hunkChangeMap[index];
  if (!entry) core.error(`Missing hunk entry at index ${index}`);
  return entry;
}

const startEntry = getHunkEntry(comment.start - 1);
const endEntry = getHunkEntry(comment.end - 1);
if (!startEntry || !endEntry) return; // skip

This moves validation early, flattens the control flow, and makes the main logic more linear.


const startSide = startChange.type !== "del" ? "RIGHT" : "LEFT"
const endSide = endChange.type !== "del" ? "RIGHT" : "LEFT"
if (!startHunk) {
core.warning(`Could not find hunk for comment on ${comment.path}, start ${comment.start}, end ${comment.end}, ${comment.comment}, skipping.`)
} else {
if (startHunk !== endHunk) endChange = startHunk.changes.at(-1)!

// get start line of the actual comment
let start: number
if (startChange.type === "normal") {
start = startChange.ln2
} else if (startChange.type === "add" || startChange.type === "del") {
start = startChange.ln
} else throw new Error(`Unknown change type.`)
const startSide = startChange.type !== "del" ? "RIGHT" : "LEFT"
const endSide = endChange.type !== "del" ? "RIGHT" : "LEFT"
Comment on lines -45 to +52

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're repeating the same pattern to compute a line number based on change.type. Refactor into a utility:

function getLineNumber(change: Change): number {
  switch (change.type) {
    case 'normal': return change.ln2;
    case 'add':
    case 'del': return change.ln;
    default: throw new Error(`Unknown change type: ${change.type}`);
  }
}

Then you can write:

const start = getLineNumber(startChange);
const end   = getLineNumber(endChange);

This removes repetition and centralizes error handling for unexpected change.type values.


// get end line of the actual comment
let end: number
if (endChange.type === "normal") {
end = endChange.ln2
} else if (endChange.type === "add" || endChange.type === "del") {
end = endChange.ln
} else throw new Error(`Unknown change type.`)
// make sure start and end are within the hunk
end = Math.min(end, endSide === "RIGHT" ? startHunk.newStart + startHunk.newLines - 1 : startHunk.oldStart + startHunk.oldLines - 1)
// get start line of the actual comment
let start: number
if (startChange.type === "normal") {
start = startChange.ln2
} else if (startChange.type === "add" || startChange.type === "del") {
start = startChange.ln
} else throw new Error(`Unknown change type.`)

result.push({
path: comment.path,
start_side: startSide !== endSide ? startSide : undefined, // only set start_side if it is a multi-line comment
side: startSide !== endSide ? endSide : startSide,
start_line: start !== end && start < end ? start : undefined, // only set start_line if it is a multi-line comment, start must be less than end
line: start !== end && start < end ? end : start,
body: comment.comment,
})
// get end line of the actual comment
let end: number
if (endChange.type === "normal") {
end = endChange.ln2
} else if (endChange.type === "add" || endChange.type === "del") {
end = endChange.ln
} else throw new Error(`Unknown change type.`)
// make sure start and end are within the hunk
end = Math.min(end, endSide === "RIGHT" ? startHunk.newStart + startHunk.newLines - 1 : startHunk.oldStart + startHunk.oldLines - 1)

result.push({
path: comment.path,
start_side: startSide !== endSide ? startSide : undefined, // only set start_side if it is a multi-line comment
side: startSide !== endSide ? endSide : startSide,
start_line: start !== end && start < end ? start : undefined, // only set start_line if it is a multi-line comment, start must be less than end
line: start !== end && start < end ? end : start,
body: comment.comment,
})
}
}
}
})
Expand Down
4 changes: 2 additions & 2 deletions pr-review/src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ export async function run(config: Config): Promise<void> {
const regex = new RegExp(`^${markerStart}\\s+<!-- (?<base>\\w+)\\.\\.\\.(?<head>\\w+) -->([\\s\\S]*?)${markerEnd}$`, "g")
previousReviews.forEach(comment => {
;[...(comment.body?.matchAll(regex) ?? [])].forEach(match => {
const commentBase = match.groups!.base
const commentHead = match.groups!.head
const commentBase = match.groups!.base!
const commentHead = match.groups!.head!
if (commentHead) base = commentHead !== head ? commentHead : commentBase
})
})
Expand Down
18 changes: 3 additions & 15 deletions pr-review/tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,19 +1,7 @@
{
"$schema": "https://json.schemastore.org/tsconfig",
"extends": "../tsconfig.json",
"compilerOptions": {
"target": "ES2022",
"module": "NodeNext",
"rootDir": "./src",
"moduleResolution": "NodeNext",
"baseUrl": "./",
"sourceMap": true,
"outDir": "./dist",
"noImplicitAny": true,
"esModuleInterop": true,
"forceConsistentCasingInFileNames": true,
"strict": true,
"skipLibCheck": true,
"newLine": "lf"
},
"exclude": ["./dist", "./node_modules", "./__tests__", "./coverage"]
"rootDir": "./src"
}
}
2 changes: 1 addition & 1 deletion pr-summary/src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ if (process.env.NODE_ENV === "development") {
// eslint-disable-next-line @typescript-eslint/no-for-in-array, no-restricted-syntax, guard-for-in
for (const key in actionYaml.inputs) {
const envKey = `INPUT_${key.toUpperCase()}`
const envValue = actionYaml.inputs[key].default
const envValue = actionYaml.inputs[key]?.default
if (envValue && !Object.keys(process.env).includes(envKey)) {
process.env[envKey] = envValue
}
Expand Down
4 changes: 2 additions & 2 deletions pr-summary/src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ export async function run(config: Config): Promise<void> {
const regex = new RegExp(`^${markerStart}\\s+<!-- (?<base>\\w+)\\.\\.\\.(?<head>\\w+) -->([\\s\\S]*?)${markerEnd}$`, "g")
comments.forEach(comment => {
;[...(comment.body?.matchAll(regex) ?? [])].forEach(match => {
const commentBase = match.groups!.base
const commentHead = match.groups!.head
const commentBase = match.groups!.base!
const commentHead = match.groups!.head!
if (commentHead) base = commentHead !== head ? commentHead : commentBase
})
})
Expand Down
18 changes: 3 additions & 15 deletions pr-summary/tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,19 +1,7 @@
{
"$schema": "https://json.schemastore.org/tsconfig",
"extends": "../tsconfig.json",
"compilerOptions": {
"target": "ES2022",
"module": "NodeNext",
"rootDir": "./src",
"moduleResolution": "NodeNext",
"baseUrl": "./",
"sourceMap": true,
"outDir": "./dist",
"noImplicitAny": true,
"esModuleInterop": true,
"forceConsistentCasingInFileNames": true,
"strict": true,
"skipLibCheck": true,
"newLine": "lf"
},
"exclude": ["./dist", "./node_modules"]
"rootDir": "./src"
}
}
22 changes: 22 additions & 0 deletions tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
{
"$schema": "https://json.schemastore.org/tsconfig",
"compilerOptions": {
"composite": true,
"target": "esnext",
"module": "nodenext",
"moduleResolution": "nodenext",
"baseUrl": "./",
"sourceMap": true,
"outDir": "./dist",
"noImplicitAny": true,
"esModuleInterop": true,
"forceConsistentCasingInFileNames": true,
"noUncheckedIndexedAccess": true,
"strict": true,
"skipLibCheck": true,
"newLine": "lf",
"erasableSyntaxOnly": true,
"rewriteRelativeImportExtensions": true
},
"exclude": ["./dist", "./node_modules", "./__tests__", "./coverage"]
}