Skip to content

🧹 [refactor: extract graphQL fragment and deep nested loop logic] #352

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
is0692vs wants to merge 1 commit into
base: main
Choose a base branch
from
+39 −28
Open

🧹 [refactor: extract graphQL fragment and deep nested loop logic] #352

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
67 changes: 39 additions & 28 deletions src/lib/githubYearInReview.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,42 @@ function mergeTopRepository(data: NonNullable<YearInReviewResponse["user"]>["con
return top;
}


function buildRepoFragment(index: number): string {
Comment on lines 139 to +143

@greptile-apps greptile-apps Bot Jun 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 関数間の空行が2行連続になっています。通常のTypeScriptスタイルでは関数間は1行のみの空行が一般的です。

Suggested change
return top;
}
function buildRepoFragment(index: number): string {
return top;
}
function buildRepoFragment(index: number): string {
Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/lib/githubYearInReview.ts
Line: 139-143

Comment:
関数間の空行が2行連続になっています。通常のTypeScriptスタイルでは関数間は1行のみの空行が一般的です。

```suggestion
    return top;
}

function buildRepoFragment(index: number): string {
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

return `
repo${index}: repository(owner: $owner${index}, name: $name${index}) {
defaultBranchRef {
target {
... on Commit {
history(author: { id: $authorId }, since: $since, until: $until, first: 100) {
nodes {
author {
date
}
}
}
}
}
}
}
`;
}

function extractCommitDatesFromRepo(repoData: Record<string, unknown> | undefined): string[] {
const dates: string[] = [];
const defaultBranchRef = repoData?.defaultBranchRef as Record<string, unknown> | undefined;
const target = defaultBranchRef?.target as Record<string, unknown> | undefined;
const history = target?.history as Record<string, unknown> | undefined;
const historyNodes = (history?.nodes as Array<Record<string, unknown>> | undefined) || [];
for (const node of historyNodes) {
const author = node?.author as Record<string, unknown> | undefined;
if (typeof author?.date === 'string') {
dates.push(author.date);
}
}
return dates;
}
Comment on lines +163 to +176

@gemini-code-assist gemini-code-assist Bot Jun 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The current implementation of extractCommitDatesFromRepo relies on repetitive type assertions (as Record<string, unknown> | undefined) to traverse the nested GraphQL response. This is verbose, hard to maintain, and bypasses TypeScript's type safety.

By defining a dedicated interface for the expected GraphQL response structure and leveraging optional chaining alongside Array.isArray, we can make the extraction process much cleaner, safer, and more robust. Additionally, ensure that we maintain explicit return types for functions to ensure type safety and API clarity.

interface RepoCommitHistory {
    defaultBranchRef?: {
        target?: {
            history?: {
                nodes?: Array<{
                    author?: {
                        date?: string;
                    };
                }>;
            };
        };
    };
}

function extractCommitDatesFromRepo(repoData: unknown): string[] {
    const dates: string[] = [];
    const nodes = (repoData as RepoCommitHistory | undefined)?.defaultBranchRef?.target?.history?.nodes;
    if (Array.isArray(nodes)) {
        for (const node of nodes) {
            const date = node?.author?.date;
            if (typeof date === 'string') {
                dates.push(date);
            }
        }
    }
    return dates;
}
References
  1. Maintain explicit return types for functions in TypeScript to ensure type safety and API clarity.

Comment on lines +163 to +176

@greptile-apps greptile-apps Bot Jun 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 抽出関数の単体テストが見当たらない

PRの説明に「Added verification of extraction via tests」とありますが、差分に新しいテストファイルが含まれていません。buildRepoFragment と extractCommitDatesFromRepo はいずれも非公開関数のため、既存の fetchCommitDatesForTopRepos の統合テスト経由でしか検証されません。抽出したロジックの境界値（repoData が undefined、nodes が空配列、date フィールドが文字列以外の型など）を直接カバーするユニットテストを追加することで、将来の改修時の安全性が高まります。

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/lib/githubYearInReview.ts
Line: 163-176

Comment:
**抽出関数の単体テストが見当たらない**

PRの説明に「Added verification of extraction via tests」とありますが、差分に新しいテストファイルが含まれていません。`buildRepoFragment` と `extractCommitDatesFromRepo` はいずれも非公開関数のため、既存の `fetchCommitDatesForTopRepos` の統合テスト経由でしか検証されません。抽出したロジックの境界値（`repoData` が `undefined`、`nodes` が空配列、`date` フィールドが文字列以外の型など）を直接カバーするユニットテストを追加することで、将来の改修時の安全性が高まります。

How can I resolve this? If you propose a fix, please make it concise.


async function fetchCommitDatesForTopRepos(
authorId: string,
token: string,
Expand All @@ -163,23 +199,7 @@ async function fetchCommitDatesForTopRepos(
};

candidates.forEach((repo, index) => {
fragments.push(`
repo${index}: repository(owner: $owner${index}, name: $name${index}) {
defaultBranchRef {
target {
... on Commit {
history(author: { id: $authorId }, since: $since, until: $until, first: 100) {
nodes {
author {
date
}
}
}
}
}
}
}
`);
fragments.push(buildRepoFragment(index));
variables[`owner${index}`] = repo.repository.owner.login;
variables[`name${index}`] = repo.repository.name;
});
Expand All @@ -198,18 +218,9 @@ async function fetchCommitDatesForTopRepos(

for (let i = 0; i < candidates.length; i++) {
const repoData = response[`repo${i}`] as Record<string, unknown> | undefined;
const defaultBranchRef = repoData?.defaultBranchRef as Record<string, unknown> | undefined;
const target = defaultBranchRef?.target as Record<string, unknown> | undefined;
const history = target?.history as Record<string, unknown> | undefined;
const historyNodes = (history?.nodes as Array<Record<string, unknown>> | undefined) || [];
for (const node of historyNodes) {
const author = node?.author as Record<string, unknown> | undefined;
if (typeof author?.date === 'string') {
dates.push(author.date);
}
}
const repoDates = extractCommitDatesFromRepo(repoData);
dates.push(...repoDates);
}

return dates;
} catch (error) {
// Fallback to empty array on failure, matching original behavior somewhat
Expand Down
Loading