Pr 56 resolved#57
Conversation
|
@Pranav00076 is attempting to deploy a commit to the Rishi Bhardwaj's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR adds GitHub organization data fetching infrastructure and updates the website to display real-time GitHub activity. A new Bash script authenticates with GitHub API using a ChangesGitHub Data Fetching and Website Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
index.html (2)
212-240: ⚡ Quick winConsider dynamically loading stats from
github_summary.jsonfor consistency.The contributor wall now dynamically fetches data from
github_summary.json, but the stats row remains hardcoded. This creates a maintenance burden where stats must be manually updated whenever the JSON is regenerated.Also, "Students Engaged: 63" doesn't correspond to any field in
github_summary.json(which hasproject_count,total_stars,contributor_count), making it unclear where this value originates.♻️ Suggested approach to dynamically load stats
// Add to the existing DOMContentLoaded fetch handler: fetch('github_summary.json') .then(res => res.json()) .then(data => { // Update stats (add appropriate IDs to the stat elements first) document.getElementById('stat-projects').textContent = data.project_count; document.getElementById('stat-contributors').textContent = data.contributor_count; document.getElementById('stat-stars').textContent = data.total_stars; // ... existing contributor wall code });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@index.html` around lines 212 - 240, Replace the hardcoded stats by fetching github_summary.json in the existing DOMContentLoaded handler and populate stat elements (add IDs stat-projects, stat-contributors, stat-stars to the three corresponding <div class="text-headline-md"> nodes); set stat-projects = data.project_count, stat-contributors = data.contributor_count, stat-stars = data.total_stars. Remove or rename the "Students Engaged" item (it has no corresponding field in github_summary.json) and ensure the fetch/update logic runs before rendering the contributor wall so counts stay consistent.
339-401: ⚡ Quick winProject cards lack links to repositories.
The project cards display repo information but don't link to the actual GitHub repositories. The upstream
github_summary.jsonincludeshtml_urlfor each repo, which could be used to make these clickable.♻️ Example: wrap project item in anchor
- <div class="flex items-start gap-4 p-2 hover:bg-surface-variant transition-colors border-l-2 border-transparent hover:border-primary"> + <a href="https://github.com/Demon-Die/CNTRL" target="_blank" class="flex items-start gap-4 p-2 hover:bg-surface-variant transition-colors border-l-2 border-transparent hover:border-primary"> <span class="material-symbols-outlined text-primary mt-1" data-icon="folder">folder</span> ... - </div> + </a>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@index.html` around lines 339 - 401, Wrap each project card's outer div (the element with class "flex items-start gap-4 p-2 hover:bg-surface-variant transition-colors border-l-2 border-transparent hover:border-primary") in an anchor that uses the repo's html_url from github_summary.json as href (and include target="_blank" rel="noopener noreferrer"); ensure the entire card (including the span with data-icon="folder" and the child div containing the project name like the element with class "text-on-surface font-label-mono text-sm") becomes clickable, and apply the same hover/focus styles to the anchor so visual behavior is preserved for all project items.github_summary.json (1)
1-404: Consider generating this file in CI/CD rather than committing it.
github_summary.jsonis a generated artifact that is committed to the repository. This approach has operational concerns:
- The file can become stale if the script isn't run regularly
- Git history tracks every data change, creating noise
- Manual regeneration is error-prone
Since the GitHub Pages workflow deploys the entire repository, consider adding a workflow step to run
fetch_github_data.shbefore deployment, ensuring the data is always fresh without committing the generated file.Example workflow addition:
- name: Generate GitHub summary run: bash fetch_github_data.sh env: GIT_DEMONDIE_ALL: ${{ secrets.GITHUB_TOKEN }}Then add
github_summary.jsonto.gitignore.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@github_summary.json` around lines 1 - 404, github_summary.json is a generated artifact committed to the repo; stop committing it and generate it in CI by running the existing fetch_github_data.sh during the Pages/deploy workflow. Remove github_summary.json from the repository (or revert the committed changes), add github_summary.json to .gitignore, and update the GitHub Actions workflow to add a step that runs bash fetch_github_data.sh (exposing GIT_DEMONDIE_ALL via secrets.GITHUB_TOKEN) before the deployment step so the file is freshly produced at build time.fetch_github_data.sh (2)
72-80: ⚡ Quick winConsider validating numeric variables before JSON interpolation.
If upstream API calls fail and numeric variables (
$PROJECT_COUNT,$TOTAL_STARS,$CONTRIBUTOR_COUNT) are empty or non-numeric, the generated JSON will be invalid. Adding validation would make the script more robust.🔍 Example validation
# Before the heredoc PROJECT_COUNT=${PROJECT_COUNT:-0} TOTAL_STARS=${TOTAL_STARS:-0} CONTRIBUTOR_COUNT=${CONTRIBUTOR_COUNT:-0}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fetch_github_data.sh` around lines 72 - 80, The numeric variables PROJECT_COUNT, TOTAL_STARS, and CONTRIBUTOR_COUNT used inside the heredoc that writes github_summary.json must be validated/normalized before interpolation: ensure each is set to a numeric default (e.g., 0) or coerced to an integer if empty/non-numeric, and fall back to 0 on invalid input so the generated JSON remains valid; update the script just prior to the heredoc to validate PROJECT_COUNT, TOTAL_STARS, and CONTRIBUTOR_COUNT (or replace non-numeric values) so the cat >> "$(dirname "$0")/github_summary.json" heredoc always receives valid numbers.
48-48: ⚡ Quick winConsider adding pagination for organization members.
The members API call fetches only the first 100 members without pagination. While the current organization has 16 members (well under the limit), adding pagination would future-proof the script as the organization grows.
♻️ Proposed fix
# Deduplicate contributors UNIQUE_CONTRIBS=$(echo "$ALL_CONTRIBS" | jq 'unique_by(.login)') # Fetch org members and ensure it's an array -ORG_MEMBERS_RAW=$(curl -s -H "Authorization: token $TOKEN" "https://api.github.com/orgs/$ORG/members?per_page=100") -ORG_MEMBERS=$(echo "$ORG_MEMBERS_RAW" | jq 'if type=="array" then . else [. ] end') +ORG_MEMBERS="[]" +PAGE=1 +while true; do + PAGE_MEMBERS=$(curl -s -H "Authorization: token $TOKEN" "https://api.github.com/orgs/$ORG/members?per_page=100&page=$PAGE") + COUNT=$(echo "$PAGE_MEMBERS" | jq length) + if [[ $COUNT -eq 0 ]]; then + break + fi + ORG_MEMBERS=$(printf '%s\n%s' "$ORG_MEMBERS" "$PAGE_MEMBERS" | jq -s 'add') + ((PAGE++)) +done # Merge contributors and org members, deduplicate again🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fetch_github_data.sh` at line 48, The curl that sets ORG_MEMBERS_RAW only requests one page (per_page=100) and misses additional members; replace that single-request assignment with a paginated fetch loop that iterates page=1..N, performing curl calls to "https://api.github.com/orgs/$ORG/members?per_page=100&page=$PAGE", stops when a page returns empty, and concatenates the results into ORG_MEMBERS_RAW (or uses a temporary accumulator variable) so all member pages are included; reference the existing ORG_MEMBERS_RAW assignment and the curl invocation to locate where to implement the loop and combine JSON pages (e.g., using jq or string/array appends).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fetch_github_data.sh`:
- Around line 21-25: The script currently assigns API output directly to REPOS
and then runs jq on it; add robust response validation after the curl call that
populates REPOS (and apply the same pattern to the other curl calls that
populate variables later) by: capturing the HTTP status and curl exit code,
failing early if curl failed or status is not 200/2xx, and verifying the
returned JSON is an array before computing PROJECT_COUNT, TOTAL_STARS and
creating REPOS_DATA; use the existing variable names REPOS, PROJECT_COUNT,
TOTAL_STARS, and REPOS_DATA and ensure any error paths log a clear message and
exit non-zero so downstream jq operations are not executed on error responses.
- Line 12: The current extraction for TOKEN using TOKEN and ENV_FILE is too
permissive and matches keys like GIT_DEMONDIE_ALL_STAGING; change the grep
pattern to match the exact variable name by anchoring the equals sign (e.g.,
grep -E "^GIT_DEMONDIE_ALL=") or otherwise parse only the key "GIT_DEMONDIE_ALL"
before splitting, then continue to cut and trim as before; update the TOKEN
assignment using the stricter match to ensure only the intended variable is
read.
- Line 21: The current assignment to REPOS only fetches the first page
(per_page=100); change it to paginate and accumulate all pages for the
organization: implement a loop that requests
"https://api.github.com/orgs/$ORG/repos?per_page=100&page=$PAGE" (using the
existing TOKEN/ORG), appends each page's JSON results into a combined
variable/array (REPOS), and stops when a page returns an empty array or when the
Link header indicates no "next" rel; use curl with headers or check response
length to detect termination. Ensure the final REPOS value contains the
concatenated JSON list (or merged array) so downstream processing works
unchanged.
In `@index.html`:
- Around line 352-356: The project card for the `.github` entry contains an
empty description div (the element with classes "text-on-surface-variant
text-code-sm font-code-sm mt-1"); fill that div with a concise description of
the `.github` project (or remove the empty div if no description is desired) so
the card matches other project entries and is not left blank. Ensure the text
content is short and styled consistently with the other project cards.
- Around line 497-515: The code assumes data.members exists and is an array when
processing the fetch result; add a defensive guard before iterating (e.g.,
verify Array.isArray(data.members) and default to an empty array or bail out
with a warning) to avoid a TypeError in the fetch success handler; also verify
the container element obtained by document.getElementById('contributors-list')
exists before appending, and log a clear warning (including err when relevant)
if data.members is missing or malformed—update the promise chain where
fetch(...).then(res => res.json()).then(data => { ... }) to perform these checks
before calling data.members.forEach.
- Around line 504-512: Replace the unsafe use of div.innerHTML that injects
member.login and member.avatar_url with safe DOM methods: create the elements
with document.createElement, set the img src via setAttribute (or .src) for
member.avatar_url, set the alt and text nodes via .textContent for member.login,
and then append children to div before container.appendChild(div); update the
block referencing div.innerHTML, member.login, member.avatar_url, and
container.appendChild(div) to use these safe setters to eliminate XSS risk.
---
Nitpick comments:
In `@fetch_github_data.sh`:
- Around line 72-80: The numeric variables PROJECT_COUNT, TOTAL_STARS, and
CONTRIBUTOR_COUNT used inside the heredoc that writes github_summary.json must
be validated/normalized before interpolation: ensure each is set to a numeric
default (e.g., 0) or coerced to an integer if empty/non-numeric, and fall back
to 0 on invalid input so the generated JSON remains valid; update the script
just prior to the heredoc to validate PROJECT_COUNT, TOTAL_STARS, and
CONTRIBUTOR_COUNT (or replace non-numeric values) so the cat >> "$(dirname
"$0")/github_summary.json" heredoc always receives valid numbers.
- Line 48: The curl that sets ORG_MEMBERS_RAW only requests one page
(per_page=100) and misses additional members; replace that single-request
assignment with a paginated fetch loop that iterates page=1..N, performing curl
calls to "https://api.github.com/orgs/$ORG/members?per_page=100&page=$PAGE",
stops when a page returns empty, and concatenates the results into
ORG_MEMBERS_RAW (or uses a temporary accumulator variable) so all member pages
are included; reference the existing ORG_MEMBERS_RAW assignment and the curl
invocation to locate where to implement the loop and combine JSON pages (e.g.,
using jq or string/array appends).
In `@github_summary.json`:
- Around line 1-404: github_summary.json is a generated artifact committed to
the repo; stop committing it and generate it in CI by running the existing
fetch_github_data.sh during the Pages/deploy workflow. Remove
github_summary.json from the repository (or revert the committed changes), add
github_summary.json to .gitignore, and update the GitHub Actions workflow to add
a step that runs bash fetch_github_data.sh (exposing GIT_DEMONDIE_ALL via
secrets.GITHUB_TOKEN) before the deployment step so the file is freshly produced
at build time.
In `@index.html`:
- Around line 212-240: Replace the hardcoded stats by fetching
github_summary.json in the existing DOMContentLoaded handler and populate stat
elements (add IDs stat-projects, stat-contributors, stat-stars to the three
corresponding <div class="text-headline-md"> nodes); set stat-projects =
data.project_count, stat-contributors = data.contributor_count, stat-stars =
data.total_stars. Remove or rename the "Students Engaged" item (it has no
corresponding field in github_summary.json) and ensure the fetch/update logic
runs before rendering the contributor wall so counts stay consistent.
- Around line 339-401: Wrap each project card's outer div (the element with
class "flex items-start gap-4 p-2 hover:bg-surface-variant transition-colors
border-l-2 border-transparent hover:border-primary") in an anchor that uses the
repo's html_url from github_summary.json as href (and include target="_blank"
rel="noopener noreferrer"); ensure the entire card (including the span with
data-icon="folder" and the child div containing the project name like the
element with class "text-on-surface font-label-mono text-sm") becomes clickable,
and apply the same hover/focus styles to the anchor so visual behavior is
preserved for all project items.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f7f29e4-287b-49e9-924d-0e8e9dc39bb3
📒 Files selected for processing (4)
.gitignorefetch_github_data.shgithub_summary.jsonindex.html
Summary by CodeRabbit
New Features
Chores