Skip to content

⚡ Optimize GraphQL nested history nodes traversal #284

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
Open

⚡ Optimize GraphQL nested history nodes traversal #284

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
12 changes: 6 additions & 6 deletions src/lib/githubYearInReview.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,12 +196,12 @@ async function fetchCommitDatesForTopRepos(
const dates: string[] = [];

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) {
// @ts-expect-error Optional chaining through unknown type for performance
const historyNodes = response[`repo${i}`]?.defaultBranchRef?.target?.history?.nodes as Array<Record<string, unknown>> | undefined;
Comment on lines +199 to +200
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 22, 2026

Choose a reason for hiding this comment

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

P2 @ts-expect-error の理由が「for performance」と記載されていますが、実際にはTypeScriptが unknown 型に対するオプショナルチェーニングを許可しないためです。コメントを正確な説明に修正することを推奨します。

Suggested change
// @ts-expect-error Optional chaining through unknown type for performance
const historyNodes = response[`repo${i}`]?.defaultBranchRef?.target?.history?.nodes as Array<Record<string, unknown>> | undefined;
// @ts-expect-error: response values are typed as unknown; cast via optional chaining
const historyNodes = response[`repo${i}`]?.defaultBranchRef?.target?.history?.nodes as Array<Record<string, unknown>> | undefined;
Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/lib/githubYearInReview.ts
Line: 199-200

Comment:
`@ts-expect-error` の理由が「for performance」と記載されていますが、実際にはTypeScriptが `unknown` 型に対するオプショナルチェーニングを許可しないためです。コメントを正確な説明に修正することを推奨します。

```suggestion
            // @ts-expect-error: response values are typed as unknown; cast via optional chaining
            const historyNodes = response[`repo${i}`]?.defaultBranchRef?.target?.history?.nodes as Array<Record<string, unknown>> | undefined;
```

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

Comment on lines +199 to +200
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 22, 2026

Choose a reason for hiding this comment

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

medium

Using ts-expect-error and multiple type assertions can be avoided by using a single type assertion on the initial access. This improves maintainability and keeps the code clean while still benefiting from the performance of optional chaining. To ensure type safety and API clarity as per repository standards, consider defining a partial interface for the expected GraphQL response structure.

Suggested change
// @ts-expect-error Optional chaining through unknown type for performance
const historyNodes = response[`repo${i}`]?.defaultBranchRef?.target?.history?.nodes as Array<Record<string, unknown>> | undefined;
const historyNodes = (response['repo' + i] as any)?.defaultBranchRef?.target?.history?.nodes;
References
  1. Maintain explicit return types and ensure type safety to provide API clarity.

if (!historyNodes) continue;

for (let j = 0; j < historyNodes.length; j++) {
Comment on lines +200 to +203
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 22, 2026

Choose a reason for hiding this comment

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

P2 パフォーマンス改善の根拠が疑わしいです

PR説明では「~41.5%の実行時間改善」と主張していますが、この変更内容（オプショナルチェーニングへの置き換え・for...of からインデックス for への変更）は V8 などのモダンな JS エンジンではほぼ同等にコンパイル・最適化されます。ベンチマークがダミーデータ上の合成マイクロベンチマークであれば、実運用環境での改善を保証するものではありません。

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

Comment:
**パフォーマンス改善の根拠が疑わしいです**

PR説明では「~41.5%の実行時間改善」と主張していますが、この変更内容（オプショナルチェーニングへの置き換え・`for...of` からインデックス `for` への変更）は V8 などのモダンな JS エンジンではほぼ同等にコンパイル・最適化されます。ベンチマークがダミーデータ上の合成マイクロベンチマークであれば、実運用環境での改善を保証するものではありません。

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

const node = historyNodes[j];
const author = node?.author as Record<string, unknown> | undefined;
if (typeof author?.date === 'string') {
dates.push(author.date);
Expand Down
Loading