fix: address review comments on PR #4823#1
Conversation
…oad worker routes - http.js: use native fetch for non-test envs to avoid axios adapter issues; add normalizeJsonPayload helper for consistent response parsing - retryer.js: normalize non-GraphQL message payloads (e.g. Bad credentials) into GraphQL-like error shape in both success and catch paths - repo.js: add missing imports and handle GraphQL errors from retryer response - worker.js: lazy-load route handlers so process.env is populated before module evaluation; replace switch with route-map lookup; guard process.env from being undefined - tests/pat-info.test.js: preserve NODE_ENV when resetting process.env in beforeAll so http.js continues to use axios mock adapter in tests
审阅者指南（在小型 PR 上折叠显示）审阅者指南通过以下方式落实 PR anuraghazra#4823 的审阅意见：修正排名测试中的浮点断言容差、在仓库获取逻辑中正确映射 GraphQL 错误类型，以及将卡片组件中的 JSON 导入恢复为基于 createRequire 的方式，以避免在当前 Node 运行时下出现 ESM SyntaxError。 fetchRepo 中更新后的 GraphQL 错误处理时序图sequenceDiagram
participant RepoConsumer
participant RepoFetcher as fetchRepo
participant GitHubAPI as GitHub_GraphQL_API
participant CustomErrorClass as CustomError
RepoConsumer->>RepoFetcher: fetchRepo(username, reponame)
RepoFetcher->>GitHubAPI: POST GraphQL repo query
GitHubAPI-->>RepoFetcher: HTTP response with data.errors
alt GraphQL_error_with_message
RepoFetcher->>RepoFetcher: extract message = res.data.errors[0].message
RepoFetcher->>RepoFetcher: extract type = res.data.errors[0].type || GRAPHQL_ERROR
RepoFetcher->>CustomErrorClass: new CustomError(message, type)
CustomErrorClass-->>RepoFetcher: error_instance
RepoFetcher-->>RepoConsumer: throw error_instance
else Other_error_without_message
RepoFetcher->>CustomErrorClass: new CustomError(HTTP_ERROR_MESSAGE, GRAPHQL_ERROR)
CustomErrorClass-->>RepoFetcher: error_instance
RepoFetcher-->>RepoConsumer: throw error_instance
end
使用 createRequire 导入 languageColors 的流程图flowchart TD
A[Module_startup] --> B[Import_createRequire_from_module]
B --> C[Call_createRequire_with_import_meta_url]
C --> D[Assign_result_to_require_function]
D --> E[Call_require_with_languageColors_json_path]
E --> F[Assign_export_to_languageColors]
F --> G[Use_languageColors_in_gist_or_wakatime_card]
文件级变更
使用提示与命令与 Sourcery 交互
自定义你的体验访问你的 控制面板 以：
获取帮助Original review guide in EnglishReviewer's guide (collapsed on small PRs)Reviewer's GuideImplements review feedback for PR anuraghazra#4823 by fixing floating-point assertion tolerance in rank tests, correctly mapping GraphQL error types in the repo fetcher, and reverting JSON imports in card components back to a createRequire-based approach to avoid ESM SyntaxError under the current Node runtime. Sequence diagram for updated GraphQL error handling in fetchReposequenceDiagram
participant RepoConsumer
participant RepoFetcher as fetchRepo
participant GitHubAPI as GitHub_GraphQL_API
participant CustomErrorClass as CustomError
RepoConsumer->>RepoFetcher: fetchRepo(username, reponame)
RepoFetcher->>GitHubAPI: POST GraphQL repo query
GitHubAPI-->>RepoFetcher: HTTP response with data.errors
alt GraphQL_error_with_message
RepoFetcher->>RepoFetcher: extract message = res.data.errors[0].message
RepoFetcher->>RepoFetcher: extract type = res.data.errors[0].type || GRAPHQL_ERROR
RepoFetcher->>CustomErrorClass: new CustomError(message, type)
CustomErrorClass-->>RepoFetcher: error_instance
RepoFetcher-->>RepoConsumer: throw error_instance
else Other_error_without_message
RepoFetcher->>CustomErrorClass: new CustomError(HTTP_ERROR_MESSAGE, GRAPHQL_ERROR)
CustomErrorClass-->>RepoFetcher: error_instance
RepoFetcher-->>RepoConsumer: throw error_instance
end
Flow diagram for languageColors import using createRequireflowchart TD
A[Module_startup] --> B[Import_createRequire_from_module]
B --> C[Call_createRequire_with_import_meta_url]
C --> D[Assign_result_to_require_function]
D --> E[Call_require_with_languageColors_json_path]
E --> F[Assign_export_to_languageColors]
F --> G[Use_languageColors_in_gist_or_wakatime_card]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses specific feedback from a previous review, focusing on enhancing the robustness and correctness of the application. It refines how JSON data is imported, improves the handling of GraphQL errors for better diagnostics, and fine-tunes test assertions to prevent flakiness related to floating-point arithmetic. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我在这里给出了一些高层次的反馈：
- 由于现在有多个模块都在对
languageColors.json使用createRequire这种变通方案，建议把这个导入集中到一个共享的 helper 中（例如common/languageColors.js），以避免重复，并把 Node 对 JSON 导入的处理集中到一个地方。 - 在
fetchRepo中，当访问res.data.errors[0].type时，最好使用可选链或条件保护，以避免在errors为空或缺失时出现潜在的运行时错误。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- Since multiple modules now use the `createRequire` workaround for `languageColors.json`, consider centralizing this import in a shared helper (e.g., `common/languageColors.js`) to avoid duplication and keep the Node JSON import handling in one place.
- In `fetchRepo`, it may be safer to use optional chaining or guards when accessing `res.data.errors[0].type` to avoid potential runtime errors if `errors` is empty or missing.帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've left some high level feedback:
- Since multiple modules now use the
createRequireworkaround forlanguageColors.json, consider centralizing this import in a shared helper (e.g.,common/languageColors.js) to avoid duplication and keep the Node JSON import handling in one place. - In
fetchRepo, it may be safer to use optional chaining or guards when accessingres.data.errors[0].typeto avoid potential runtime errors iferrorsis empty or missing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since multiple modules now use the `createRequire` workaround for `languageColors.json`, consider centralizing this import in a shared helper (e.g., `common/languageColors.js`) to avoid duplication and keep the Node JSON import handling in one place.
- In `fetchRepo`, it may be safer to use optional chaining or guards when accessing `res.data.errors[0].type` to avoid potential runtime errors if `errors` is empty or missing.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR correctly addresses previous review feedback, with appropriate changes to fix JSON imports, GraphQL error handling, and test precision. No security vulnerabilities were found. However, I've suggested a refactor in gist.js and wakatime.js to remove duplicated JSON import code. Additionally, there's a potential bug in src/fetchers/repo.js at line 85 where error handling could lead to a TypeError with an empty errors array; a check for res.data.errors.length > 0 is recommended.
| import { createRequire } from "module"; | ||
| const require = createRequire(import.meta.url); | ||
| const languageColors = require("../common/languageColors.json"); |
There was a problem hiding this comment.
This createRequire logic for importing JSON is duplicated in src/cards/wakatime.js. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider abstracting this into a shared utility function within a common module. This would centralize the logic for loading JSON files in a Node.js ESM-compatible way.
| import { createRequire } from "module"; | ||
| const require = createRequire(import.meta.url); | ||
| const languageColors = require("../common/languageColors.json"); |
There was a problem hiding this comment.
This createRequire logic for importing JSON is duplicated in src/cards/gist.js. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider abstracting this into a shared utility function within a common module. This would centralize the logic for loading JSON files in a Node.js ESM-compatible way.
BBleae
force-pushed
the
codex/pr-4714-followup
branch
from
727d211 to
4d72a71
Compare
|
Thanks @biplavbarua! All three of your changes have been taken — your commit was cherry-picked onto the rebased branch with authorship preserved (1fd5d22), which is why GitHub shows this PR as closed rather than merged. One important finding from testing your fix on this fork, which is also relevant for the upstream PR anuraghazra#4823 discussion:
Note that Meanwhile the bare JSON import you replaced was equally broken in the other direction: on Node ≥22 it throws The follow-up commit (4d72a71) switches to standard import attributes, which works on both targets: import languageColors from "../common/languageColors.json" with { type: "json" };plus For upstream anuraghazra#4823 (Vercel-only, no workerd) your The GraphQL error-type mapping fix and the float-tolerance test fix were taken as-is. Thanks again! |
2 participants
This PR addresses the recent review feedback on anuraghazra#4823.
calculateRank.test.jsrepo.jsgist.jsandwakatime.jsby reverting tocreateRequire.Summary by Sourcery
根据评审反馈，修正错误处理、JSON 导入以及测试预期。
Bug 修复：
createRequire恢复在 gist 和 Wakatime 卡片中以 Node 兼容方式加载语言颜色的 JSON。Original summary in English
Summary by Sourcery
Address review feedback by correcting error handling, JSON imports, and test expectations.
Bug Fixes: