MOSIP-45245-Github Trakker add Apis to fetch user details in backend along with roles - #45
Open
kharodejayesh wants to merge 3 commits into
Open
MOSIP-45245-Github Trakker add Apis to fetch user details in backend along with roles#45kharodejayesh wants to merge 3 commits into
kharodejayesh wants to merge 3 commits into
Conversation
Signed-off-by: Jayesh Kharode <jayesh.kharode@technoforte.co.in>
WalkthroughThis PR adds GitHub user name and role persistence, role-aware admin and listing endpoints, role-filtered analytics, frontend filter/data wiring, and migration/startup changes for lookup-table initialization and deployment. ChangesUser Names and Roles Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant userRoleRoute
participant userRoleService
participant DB
Admin->>userRoleRoute: POST /admin/users/role {login, role, organization}
userRoleRoute->>userRoleService: setUserRole(...)
userRoleService->>DB: validate login, role, organization
userRoleService->>DB: lock active user_details row
userRoleService->>DB: deactivate old assignment
userRoleService->>DB: insert new active user_details row
userRoleService-->>userRoleRoute: formatted user response
userRoleRoute-->>Admin: success or validation error
sequenceDiagram
participant Admin
participant userNameSyncRoute
participant githubUserService
participant GitHubAPI
participant DB
Admin->>userNameSyncRoute: POST /admin/sync/user-names
userNameSyncRoute->>githubUserService: backfillMissingUserNames(limit)
githubUserService->>DB: select github_users with missing name
loop each user
githubUserService->>GitHubAPI: fetchGitHubUserName(login)
GitHubAPI-->>githubUserService: name
githubUserService->>DB: update github_users.name
githubUserService->>DB: ensure active user_details row
end
githubUserService-->>userNameSyncRoute: counts
userNameSyncRoute-->>Admin: status + result
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Trivy (0.69.3)Trivy execution failed: 2026-07-09T12:15:10Z FATAL Fatal error run error: fs scan error: scan error: scan failed: failed analysis: post analysis error: post analysis error: cloudformation scan error: fs filter error: fs filter error: walk error range error: stat github-activity-tracker/backend/doctor.config.json: no such file or directory: range error: stat github-activity-tracker/backend/doctor.config.json: no such file or directory Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
github-activity-tracker/backend/migrations/006_add_role_to_github_users.sql (1)
1-6: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider a DB-level CHECK constraint for role values.
Role validity is only enforced in the application layer (
isValidUserRole). Adding a CHECK constraint mirroring the allowed list would prevent drift if a future code path writes togithub_users.roledirectly.♻️ Proposed constraint
ALTER TABLE github_users ADD COLUMN IF NOT EXISTS role VARCHAR(50); + +ALTER TABLE github_users + ADD CONSTRAINT chk_github_users_role + CHECK (role IS NULL OR role IN ( + 'Developer', 'Tech Lead', 'Architect', 'Product Owner', + 'Leadership', 'QA Engineer', 'DevOps Engineer' + ));Note: keep this constraint in sync with
USER_ROLESinconfig/userRoles.jsif adopted.🤖 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-activity-tracker/backend/migrations/006_add_role_to_github_users.sql` around lines 1 - 6, The migration that adds github_users.role should also enforce allowed values at the database level instead of relying only on isValidUserRole in application code. Update the ALTER TABLE in 006_add_role_to_github_users.sql to add a CHECK constraint on role using the same allowed list as USER_ROLES in config/userRoles.js, and keep the constraint name explicit so future schema updates can manage it easily. Ensure the constraint matches the manually maintained role set and remains in sync with the application validation.github-activity-tracker/backend/services/reviewSyncService.js (1)
183-192: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider memoizing
userIdper reviewer across the PR/review pagination loop.Even though
profileNameavoids the extra API fetch here,upsertGitHubUserstill issues a DB upsert per review event rather than once per unique reviewer. For PRs with many reviews from a small set of reviewers, this is avoidable I/O. Same caching approach as suggested forcommitSyncService.jsapplies here.🤖 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-activity-tracker/backend/services/reviewSyncService.js` around lines 183 - 192, Memoize the result of upsertGitHubUser in reviewSyncService.js so the same reviewer only triggers one DB upsert across the PR/review pagination loop. Use a per-PR cache keyed by a stable reviewer identifier (for example githubUserId or login) around the code that builds profileName and assigns userId, and reuse the cached userId on subsequent review events instead of calling upsertGitHubUser again.github-activity-tracker/backend/services/prSyncService.js (1)
106-112: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSame redundant per-event name-resolution risk as commitSyncService.
upsertGitHubUseris invoked per-PR withoutname, so ifresolveGitHubUserNamefetches from the API whenevernameis falsy, repos with many PRs from the same authors incur repeated redundant fetches. See the analogous comment incommitSyncService.jsfor the caching approach and verification script.🤖 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-activity-tracker/backend/services/prSyncService.js` around lines 106 - 112, The per-PR call to upsertGitHubUser in prSyncService.js can repeatedly trigger resolveGitHubUserName for the same author when name is missing, causing redundant API lookups across many PRs. Apply the same caching approach used in commitSyncService: reuse previously resolved GitHub user names keyed by github_user_id/login before calling upsertGitHubUser, and pass the cached name so the resolver is not re-fetched for each event.github-activity-tracker/backend/routes/orgActivityRoute.js (1)
18-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared role-filter validation.
This validate-then-compute-
roleFilterblock is duplicated almost verbatim inorgSummaryRoute.js(lines 19-24). Extracting a small helper (e.g., inconfig/userRoles.js) would keep both routes in sync as role semantics evolve.♻️ Proposed shared helper
// config/userRoles.js function resolveRoleFilter(role) { if (role && role !== "all" && !isValidUserRole(role)) { return { error: "Invalid role value" }; } return { roleFilter: role && role !== "all" ? role : null }; } module.exports = { USER_ROLES, isValidUserRole, resolveRoleFilter };- if (role && role !== "all" && !isValidUserRole(role)) { - return res.status(400).json({ error: "Invalid role value" }); - } - - try { - const roleFilter = role && role !== "all" ? role : null; + const { error, roleFilter } = resolveRoleFilter(role); + if (error) { + return res.status(400).json({ error }); + } + + try { const data = await getOrgActivity(org_id, period, roleFilter);🤖 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-activity-tracker/backend/routes/orgActivityRoute.js` around lines 18 - 24, The role validation and roleFilter derivation in orgActivityRoute’s request handling are duplicated in orgSummaryRoute, so extract the shared logic into a small helper such as resolveRoleFilter in config/userRoles.js. Update both orgActivityRoute and orgSummaryRoute to call that helper for validating role and computing the normalized filter, returning the same 400 error shape when invalid so both routes stay in sync.github-activity-tracker/backend/services/orgActivityService.js (1)
26-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate role-filter SQL-builder logic across services.
This exact
if (role) { params.push(role); whereClauses.push(...) }block is repeated inorgSummaryService.js(lines 72-76). A tiny shared helper (e.g.,pushRoleFilter(params, whereClauses, role)) would avoid drift if the filter logic changes (e.g., case-insensitive matching later).🤖 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-activity-tracker/backend/services/orgActivityService.js` around lines 26 - 30, The role-filter SQL builder logic is duplicated in orgActivityService and orgSummaryService, so extract it into a small shared helper such as pushRoleFilter(params, whereClauses, role) and use that from the existing query-building code. Keep the behavior identical by preserving the current params.push(role) and whereClauses.push(...) flow, and update both service methods that build these filters so the logic lives in one place.
🤖 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 `@github-activity-tracker/backend/app.js`:
- Around line 51-52: The admin-only routes are mounted without protection, so
any client can reach the name sync and role update endpoints. Add the
appropriate authentication/authorization middleware before mounting
userNameSyncRoute and userRoleRoute in app, or wrap those route handlers with
the same admin guard used elsewhere, so only authorized admins can access them.
In `@github-activity-tracker/backend/routes/userNameSyncRoute.js`:
- Around line 11-28: The /admin/sync/user-names handler currently runs
backfillMissingUserNames() synchronously inside the request cycle, which can
block for a long time and time out. Move the work out of router.post('
/admin/sync/user-names') into a background job/worker (or queued task) and have
the route only enqueue the backfill and return an accepted/job status response.
Add a separate status endpoint or job-tracking mechanism so callers can check
progress; keep the existing backfillMissingUserNames function as the worker
logic rather than invoking it directly from the HTTP handler.
In `@github-activity-tracker/backend/services/githubUserService.js`:
- Around line 108-144: `backfillMissingUserNames` is doing an unbounded,
synchronous per-user backfill inside the admin sync flow, which can keep the
request open too long and abort the whole batch on a single failure. Update
`backfillMissingUserNames` (and its caller in the admin route) to process only a
bounded batch via a `limit`/pagination or move the work to an async background
job that returns immediately. Also wrap the per-user `fetchGitHubUserName` and
update logic in try/catch so one failed user doesn’t stop the remaining
backfill, while still returning partial progress.
In `@github-activity-tracker/backend/services/orgUsersService.js`:
- Around line 65-67: The org user lookup in orgUsersService is exposing
sensitive fields like name and role without any authorization check. Add an
access-control gate in the service method that builds this query (or in the org
routes before calling it) so only authenticated/authorized callers can reach the
name/role projection, and keep the response limited to permitted org members.
Use the orgUsersService query path and the mounted org route handler to locate
where to enforce the check.
---
Nitpick comments:
In `@github-activity-tracker/backend/migrations/006_add_role_to_github_users.sql`:
- Around line 1-6: The migration that adds github_users.role should also enforce
allowed values at the database level instead of relying only on isValidUserRole
in application code. Update the ALTER TABLE in 006_add_role_to_github_users.sql
to add a CHECK constraint on role using the same allowed list as USER_ROLES in
config/userRoles.js, and keep the constraint name explicit so future schema
updates can manage it easily. Ensure the constraint matches the manually
maintained role set and remains in sync with the application validation.
In `@github-activity-tracker/backend/routes/orgActivityRoute.js`:
- Around line 18-24: The role validation and roleFilter derivation in
orgActivityRoute’s request handling are duplicated in orgSummaryRoute, so
extract the shared logic into a small helper such as resolveRoleFilter in
config/userRoles.js. Update both orgActivityRoute and orgSummaryRoute to call
that helper for validating role and computing the normalized filter, returning
the same 400 error shape when invalid so both routes stay in sync.
In `@github-activity-tracker/backend/services/orgActivityService.js`:
- Around line 26-30: The role-filter SQL builder logic is duplicated in
orgActivityService and orgSummaryService, so extract it into a small shared
helper such as pushRoleFilter(params, whereClauses, role) and use that from the
existing query-building code. Keep the behavior identical by preserving the
current params.push(role) and whereClauses.push(...) flow, and update both
service methods that build these filters so the logic lives in one place.
In `@github-activity-tracker/backend/services/prSyncService.js`:
- Around line 106-112: The per-PR call to upsertGitHubUser in prSyncService.js
can repeatedly trigger resolveGitHubUserName for the same author when name is
missing, causing redundant API lookups across many PRs. Apply the same caching
approach used in commitSyncService: reuse previously resolved GitHub user names
keyed by github_user_id/login before calling upsertGitHubUser, and pass the
cached name so the resolver is not re-fetched for each event.
In `@github-activity-tracker/backend/services/reviewSyncService.js`:
- Around line 183-192: Memoize the result of upsertGitHubUser in
reviewSyncService.js so the same reviewer only triggers one DB upsert across the
PR/review pagination loop. Use a per-PR cache keyed by a stable reviewer
identifier (for example githubUserId or login) around the code that builds
profileName and assigns userId, and reuse the cached userId on subsequent review
events instead of calling upsertGitHubUser again.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ae17eb7-ea3b-4960-86c2-9c38ab710d4a
📒 Files selected for processing (18)
github-activity-tracker/backend/app.jsgithub-activity-tracker/backend/config/userRoles.jsgithub-activity-tracker/backend/migrations/005_add_name_to_github_users.sqlgithub-activity-tracker/backend/migrations/006_add_role_to_github_users.sqlgithub-activity-tracker/backend/routes/orgActivityRoute.jsgithub-activity-tracker/backend/routes/orgSummaryRoute.jsgithub-activity-tracker/backend/routes/userNameSyncRoute.jsgithub-activity-tracker/backend/routes/userRoleRoute.jsgithub-activity-tracker/backend/services/commitSyncService.jsgithub-activity-tracker/backend/services/githubUserService.jsgithub-activity-tracker/backend/services/leaderBoardService.jsgithub-activity-tracker/backend/services/orgActivityService.jsgithub-activity-tracker/backend/services/orgSummaryService.jsgithub-activity-tracker/backend/services/orgUsersService.jsgithub-activity-tracker/backend/services/prSyncService.jsgithub-activity-tracker/backend/services/reviewSyncService.jsgithub-activity-tracker/backend/services/userDetailsService.jsgithub-activity-tracker/backend/services/userRoleService.js
Signed-off-by: Jayesh Kharode <jayesh.kharode@technoforte.co.in>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@github-activity-tracker/backend/config/syncConfig.js`:
- Around line 11-12: The backfill throttle in backfillMissingUserNames is using
a duplicated local NAME_FETCH_DELAY_MS instead of the exported config constant,
so config changes won’t take effect. Update githubUserService.js to import
NAME_FETCH_DELAY_MS from syncConfig.js and use that symbol in the sleep() delay,
removing the local duplicate. Keep the reference tied to
backfillMissingUserNames so the throttling stays driven by the shared config.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6ab59fd-b898-4562-85a8-84c977f36967
📒 Files selected for processing (1)
github-activity-tracker/backend/config/syncConfig.js
Signed-off-by: Jayesh Kharode <jayesh.kharode@technoforte.co.in>
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
github-activity-tracker/frontend/src/App.tsx (1)
104-111: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
loginfield not populated in leaderboard mapping — subtitle always shows "— • —".
LeaderboardCardwas updated to show@{user.login}whenloginis available (Line 71 ofLeaderboardCard.tsx), but this mapping never setslogin. Sinceuser.loginis alwaysundefined, the subtitle falls back to${user.team} • ${user.project}which renders "— • —" for every entry.🔧 Proposed fix
const ranked = list.map((u: any) => ({ - name: u.login, + name: u.name || u.login, + login: u.login, team: "—", project: "—", prs: u.prs, reviews: u.reviews, total: u.score, }));If the API does not return a
namefield, usename: u.loginandlogin: u.loginso the subtitle shows@usernamebeneath the title.🤖 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-activity-tracker/frontend/src/App.tsx` around lines 104 - 111, The leaderboard mapping in App.tsx is not populating the login field, so LeaderboardCard always falls back to the team/project subtitle. Update the ranked object built from list.map to include login from the API user (alongside the existing name mapping) so LeaderboardCard can render @{user.login} instead of “— • —”. Use the list.map block and LeaderboardCard as the key symbols to locate the fix.
♻️ Duplicate comments (1)
github-activity-tracker/backend/services/githubUserService.js (1)
149-187: 🩺 Stability & Availability | 🟠 Major
backfillMissingUserNamesstill unbounded with no per-user error handling; caller'slimitparameter silently ignored.A previous review flagged the unbounded loop and lack of per-user try/catch. Additionally, the caller in
userNameSyncRoute.jspasses{ limit: req.body?.limit }, but the function signature accepts no parameters — the limit is silently ignored and all users are processed.🔧 Proposed fix: accept limit + per-user try/catch
-async function backfillMissingUserNames() { +async function backfillMissingUserNames({ limit } = {}) { const result = await pool.query( ` SELECT u.id, u.login, u.type, u.github_user_id FROM github_users u WHERE u.name IS NULL AND u.login IS NOT NULL AND (u.type IS NULL OR LOWER(u.type) <> 'bot') ORDER BY u.id + ${limit ? 'LIMIT $1' : ''} ` + , limit ? [limit] : [] ); let namesFetched = 0; + let errors = 0; for (const user of result.rows) { - const name = await fetchGitHubUserName(user.login); - - if (name) { - await pool.query( - ` - UPDATE github_users - SET name = $1, updated_at = CURRENT_TIMESTAMP - WHERE id = $2 - `, - [name, user.id] - ); - namesFetched += 1; + try { + const name = await fetchGitHubUserName(user.login); + + if (name) { + await pool.query( + ` + UPDATE github_users + SET name = $1, updated_at = CURRENT_TIMESTAMP + WHERE id = $2 + `, + [name, user.id] + ); + namesFetched += 1; + } + + await ensureActiveUserDetails(user.id); + } catch (err) { + console.error(`Failed to backfill user ${user.login}:`, err.message); + errors += 1; } - await ensureActiveUserDetails(user.id); - await sleep(NAME_FETCH_DELAY_MS); } return { users_checked: result.rows.length, names_fetched: namesFetched, + errors, }; }🤖 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-activity-tracker/backend/services/githubUserService.js` around lines 149 - 187, `backfillMissingUserNames` is still processing every matching user and ignores the caller’s `limit`, while also lacking per-user error isolation. Update `backfillMissingUserNames` in `githubUserService.js` to accept an options object with `limit`, apply that limit to the `SELECT` query, and handle each user inside a try/catch so one failure does not stop the whole backfill. Keep the existing behavior for successful updates and `ensureActiveUserDetails`, and make sure `userNameSyncRoute.js`’s `{ limit: req.body?.limit }` call is actually honored by the function signature.
🧹 Nitpick comments (3)
github-activity-tracker/backend/app.js (1)
66-83: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winInitialize lookup tables before
app.listen()to avoid serving requests against an uninitialized database.
app.listen(PORT, async () => { ... })starts accepting requests immediately, but the async callback (which runsensureLookupTables()) is not awaited by Express. On a fresh database or when runningnode app.jswithout prior migrations, any request hitting endpoints that queryuser_rolesororganizationswill fail during the initialization window. In the docker-compose path this is mitigated bynpm run migrate && node app.js, but the code should not rely on that external ordering.♻️ Proposed refactor
- app.listen(PORT, async () => { - try { - const result = await ensureLookupTables(); - - if (result.createdTables.length > 0) { - console.log(`Created tables: ${result.createdTables.join(', ')}`); - } - - if (result.rolesAdded > 0 || result.orgsAdded > 0) { - console.log(`Added from .env: ${result.rolesAdded} role(s), ${result.orgsAdded} organization(s)`); - } else if (result.createdTables.length === 0) { - console.log('Lookup tables already exist; no new roles or organizations to add.'); - } - } catch (error) { - console.error('Failed to initialize lookup tables:', error.message); - process.exit(1); - } + async function startServer() { + try { + const result = await ensureLookupTables(); + + if (result.createdTables.length > 0) { + console.log(`Created tables: ${result.createdTables.join(', ')}`); + } + + if (result.rolesAdded > 0 || result.orgsAdded > 0) { + console.log(`Added from .env: ${result.rolesAdded} role(s), ${result.orgsAdded} organization(s)`); + } else if (result.createdTables.length === 0) { + console.log('Lookup tables already exist; no new roles or organizations to add.'); + } + + app.listen(PORT, () => { + console.log(`Server running on port ${PORT}`); + }); + } catch (error) { + console.error('Failed to initialize lookup tables:', error.message); + process.exit(1); + } + } + + startServer();🤖 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-activity-tracker/backend/app.js` around lines 66 - 83, Move the `ensureLookupTables()` initialization out of the `app.listen()` callback in `app.js` so the lookup tables are created before the server starts accepting traffic. Update the startup flow around `ensureLookupTables`, `app.listen`, and the existing success/error logging so the app awaits initialization first, then calls `app.listen(PORT)` only after the database is ready; keep the current `console.log`/`console.error` behavior but run it during pre-listen startup.github-activity-tracker/backend/migrations/009_user_details_role_organization_ids.sql (1)
43-49: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider concurrent index creation/deletion for production safety.
The
DROP INDEX(line 47) andCREATE INDEX(lines 48-49) acquireACCESS EXCLUSIVEandSHARElocks respectively, blocking writes during execution. For a table with significant data, preferCONCURRENTLYvariants. Note thatCREATE INDEX CONCURRENTLYcannot run inside a transaction block, and the migration runner (pool.query(sql)) may wrap multi-statement SQL in an implicit transaction — so this may require runner-level changes to support non-transactional migrations.🤖 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-activity-tracker/backend/migrations/009_user_details_role_organization_ids.sql` around lines 43 - 49, The migration currently uses non-concurrent index drop/create operations in the user_details migration, which can block writes in production. Update the migration around the DROP INDEX and CREATE INDEX statements to use the concurrent index variants where supported, and make sure the migration runner for this SQL can execute non-transactional migrations since CREATE INDEX CONCURRENTLY cannot run inside a transaction. Use the user_details role_id and organization_id index statements as the primary places to adjust, and verify the migration flow supports this change end-to-end.Source: Linters/SAST tools
github-activity-tracker/frontend/src/components/TopNav.tsx (1)
200-226: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider passing organizations from
ApptoTopNavto eliminate duplicate fetching.Both
AppandTopNavindependently callfetchOrganizations()on mount, resulting in two identical API requests.Appalready fetches organizations to set the initialselectedOrg; it could pass the same data down toTopNavas a prop instead of havingTopNavre-fetch.This is an optional refactor — the current approach works correctly, but sharing the fetch would reduce redundant network calls and ensure both views stay in sync.
🤖 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-activity-tracker/frontend/src/components/TopNav.tsx` around lines 200 - 226, `TopNav` is redundantly fetching organizations even though `App` already loads them for the initial `selectedOrg`. Update the `TopNav` component to receive the organizations list from `App` via props instead of calling `fetchOrganizations()` on mount, and keep the existing `organization` selector rendering/behavior unchanged. Make sure the `App` to `TopNav` prop wiring uses the same organization data source so both views stay in sync and duplicate API requests are removed.
🤖 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 `@github-activity-tracker/.env.example`:
- Line 15: The USER_ROLES environment entry should be quoted because it contains
role names with spaces, which can be truncated by strict dotenv parsing. Update
the USER_ROLES value in the example env file to use quotes so downstream
consumers like defaultUserRoles.js still read the full comma-separated list from
process.env.USER_ROLES.
In `@github-activity-tracker/backend/.env.example`:
- Around line 6-7: The dotenv example keys are out of the expected order, which
triggers the env-file lint. Reorder the entries in the .env.example block so
GITHUB_TOKEN appears before RDS_DATABASE, keeping the rest of the example
unchanged and matching the repo’s required key ordering.
In `@github-activity-tracker/backend/app.js`:
- Around line 56-64: Mount the privileged admin routers behind the existing
adminAuth middleware in app.js: the userNameSyncRoute and userRoleRoute are
currently registered directly with app.use, so update their wiring so requests
must pass through adminAuth before reaching those /admin/* handlers. Keep the
other route mounts unchanged and use the route names userNameSyncRoute and
userRoleRoute to locate the affected registration block.
In
`@github-activity-tracker/backend/migrations/007_create_user_details_table.sql`:
- Around line 20-58: The migration in `007_create_user_details_table.sql` drops
`user_details.name` before the backfill block that reads it, so the data
migration in the `DO $$` section never runs. Reorder the statements so the
`UPDATE github_users ... FROM user_details` logic executes before any `DROP
COLUMN IF EXISTS name` on `user_details`, keeping the existing
`github_users`/`user_details` migration flow intact and preserving upgrade data.
In
`@github-activity-tracker/backend/migrations/008_backfill_user_details_active_from.sql`:
- Around line 3-41: The migration backdates too many user_details rows, which
can shift later role windows and overlap prior history. Update the backfill
logic in the SQL migration so only the earliest role assignment row per user is
backdated, using the existing user_details and activity_events lookups to
identify that single first role row. Keep later role assignment rows unchanged,
and ensure the updated_at touch only applies to the targeted earliest assignment
row.
In `@github-activity-tracker/backend/services/orgUsersService.js`:
- Around line 159-162: The assignments list is being fetched globally in
orgUsersService via fetchAssignments(role), so /orgs/:org_id/users can include
assignments from other orgs before paging. Update fetchAssignments to accept
orgId and scope the returned assignments to that organization before building
the activity map and paginating, then ensure the downstream mapping/pagination
path in the org users flow only operates on org-matched assignments.
- Around line 156-158: Clamp the pagination inputs in orgUsersService after
parsing so negative or zero values cannot pass through as valid page/limit
values. Update the logic around page and limit normalization to ensure both
values are at least 1 before they are used for slicing or metadata calculations.
Use the existing pagination handling in orgUsersService and the page/limit
variables as the place to apply the validation.
- Around line 51-59: The default activity sort in orgUsersService’s results.sort
callback ignores the requested direction because total_activity is always
compared descending. Update the sorting logic to honor sortOrder/direction when
sorting by total_activity, and keep the existing active/login tie-breakers in
orgUsersService’s sorting block.
In `@github-activity-tracker/backend/services/userRoleService.js`:
- Around line 103-108: The role/organization validation in userRoleService is
calling async checks without awaiting them, so the Promise objects are always
truthy and the specific error branches never run. Update the validation flow to
await isValidUserRole and isValidOrganization before the conditional checks,
keeping the existing error messages so userRoleRoute can return the intended 400
responses and allowed_roles/allowed_organizations payloads. Also ensure these
awaited calls remain inside the same validation path in userRoleService so no
fire-and-forget Promises are left behind.
In `@github-activity-tracker/backend/utils/userRoleSql.js`:
- Around line 3-6: The role filter in userRoleSql should not require ud.active =
true for time-based lookups, because it drops closed historical assignments that
still match e.created_at. Update the SQL fragment used by the role join/where
logic to rely on the assignment window (ud.active_from and ud.active_to) instead
of the active flag, so historical role rows are included when their window
contains the event time.
In `@github-activity-tracker/frontend/src/components/ActivityTable.tsx`:
- Around line 17-18: Remove the debug browser log from ActivityTable so raw
activity data is not emitted in production; delete the console.log added
alongside visibleActivities in the ActivityTable component and keep the
filtering logic unchanged.
In `@github-activity-tracker/frontend/src/components/TeamMembers.tsx`:
- Around line 133-135: The page-reset effect in TeamMembers is missing period in
its dependency list, so changing the period does not reset the table back to
page 1. Update the useEffect that calls setPage(1) to include period alongside
org, role, and project, so the reset runs whenever any filter affecting the
dataset changes.
- Line 109: Remove the debug console output from TeamMembers so the full API
response is not logged in production. Delete the console.log("API RESPONSE:",
data) statement in the TeamMembers component, keeping the API handling logic
intact and ensuring no other console-based debugging remains in that flow.
In `@github-activity-tracker/helm/gh-tracker-service/values.yaml`:
- Around line 246-247: The `extraEnvVars` entry in `values.yaml` is incorrectly
using the `gh-tracker-service.env` helper include, which makes the values file
invalid as pure YAML. Move the Helm template logic out of `values.yaml` and into
the template that consumes `extraEnvVars` (or replace it with a literal value),
keeping `values.yaml` data-only.
---
Outside diff comments:
In `@github-activity-tracker/frontend/src/App.tsx`:
- Around line 104-111: The leaderboard mapping in App.tsx is not populating the
login field, so LeaderboardCard always falls back to the team/project subtitle.
Update the ranked object built from list.map to include login from the API user
(alongside the existing name mapping) so LeaderboardCard can render
@{user.login} instead of “— • —”. Use the list.map block and LeaderboardCard as
the key symbols to locate the fix.
---
Duplicate comments:
In `@github-activity-tracker/backend/services/githubUserService.js`:
- Around line 149-187: `backfillMissingUserNames` is still processing every
matching user and ignores the caller’s `limit`, while also lacking per-user
error isolation. Update `backfillMissingUserNames` in `githubUserService.js` to
accept an options object with `limit`, apply that limit to the `SELECT` query,
and handle each user inside a try/catch so one failure does not stop the whole
backfill. Keep the existing behavior for successful updates and
`ensureActiveUserDetails`, and make sure `userNameSyncRoute.js`’s `{ limit:
req.body?.limit }` call is actually honored by the function signature.
---
Nitpick comments:
In `@github-activity-tracker/backend/app.js`:
- Around line 66-83: Move the `ensureLookupTables()` initialization out of the
`app.listen()` callback in `app.js` so the lookup tables are created before the
server starts accepting traffic. Update the startup flow around
`ensureLookupTables`, `app.listen`, and the existing success/error logging so
the app awaits initialization first, then calls `app.listen(PORT)` only after
the database is ready; keep the current `console.log`/`console.error` behavior
but run it during pre-listen startup.
In
`@github-activity-tracker/backend/migrations/009_user_details_role_organization_ids.sql`:
- Around line 43-49: The migration currently uses non-concurrent index
drop/create operations in the user_details migration, which can block writes in
production. Update the migration around the DROP INDEX and CREATE INDEX
statements to use the concurrent index variants where supported, and make sure
the migration runner for this SQL can execute non-transactional migrations since
CREATE INDEX CONCURRENTLY cannot run inside a transaction. Use the user_details
role_id and organization_id index statements as the primary places to adjust,
and verify the migration flow supports this change end-to-end.
In `@github-activity-tracker/frontend/src/components/TopNav.tsx`:
- Around line 200-226: `TopNav` is redundantly fetching organizations even
though `App` already loads them for the initial `selectedOrg`. Update the
`TopNav` component to receive the organizations list from `App` via props
instead of calling `fetchOrganizations()` on mount, and keep the existing
`organization` selector rendering/behavior unchanged. Make sure the `App` to
`TopNav` prop wiring uses the same organization data source so both views stay
in sync and duplicate API requests are removed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bbaba61b-3c6a-4cd2-9114-619e66e1822f
📒 Files selected for processing (55)
github-activity-tracker/.env.examplegithub-activity-tracker/backend/.env.examplegithub-activity-tracker/backend/app.jsgithub-activity-tracker/backend/config/defaultUserRoles.jsgithub-activity-tracker/backend/config/organizations.jsgithub-activity-tracker/backend/config/userRoles.jsgithub-activity-tracker/backend/db/initLookupTables.jsgithub-activity-tracker/backend/migrations/007_create_user_details_table.sqlgithub-activity-tracker/backend/migrations/008_backfill_user_details_active_from.sqlgithub-activity-tracker/backend/migrations/009_user_details_role_organization_ids.sqlgithub-activity-tracker/backend/migrations/runMigrations.jsgithub-activity-tracker/backend/routes/leaderBoardRoute.jsgithub-activity-tracker/backend/routes/orgActivityRoute.jsgithub-activity-tracker/backend/routes/orgSummaryRoute.jsgithub-activity-tracker/backend/routes/orgUsersRoute.jsgithub-activity-tracker/backend/routes/organizationsRoute.jsgithub-activity-tracker/backend/routes/userDetailsRoute.jsgithub-activity-tracker/backend/routes/userNameSyncRoute.jsgithub-activity-tracker/backend/routes/userRoleRoute.jsgithub-activity-tracker/backend/routes/userRolesRoute.jsgithub-activity-tracker/backend/scripts/runSync.jsgithub-activity-tracker/backend/services/githubUserService.jsgithub-activity-tracker/backend/services/leaderBoardService.jsgithub-activity-tracker/backend/services/orgActivityService.jsgithub-activity-tracker/backend/services/orgSummaryService.jsgithub-activity-tracker/backend/services/orgUsersService.jsgithub-activity-tracker/backend/services/organizationsService.jsgithub-activity-tracker/backend/services/userDetailsService.jsgithub-activity-tracker/backend/services/userRoleService.jsgithub-activity-tracker/backend/services/userRolesService.jsgithub-activity-tracker/backend/utils/userRoleSql.jsgithub-activity-tracker/deploy/gh-tracker-values.yamlgithub-activity-tracker/docker-compose.ymlgithub-activity-tracker/frontend/.env.examplegithub-activity-tracker/frontend/index.htmlgithub-activity-tracker/frontend/src/App.tsxgithub-activity-tracker/frontend/src/components/ActivityChart.tsxgithub-activity-tracker/frontend/src/components/ActivityItem.tsxgithub-activity-tracker/frontend/src/components/ActivityTable.tsxgithub-activity-tracker/frontend/src/components/ActivityTrend.tsxgithub-activity-tracker/frontend/src/components/DetailView.tsxgithub-activity-tracker/frontend/src/components/LeaderboardCard.tsxgithub-activity-tracker/frontend/src/components/TeamMembers.tsxgithub-activity-tracker/frontend/src/components/TopNav.tsxgithub-activity-tracker/frontend/src/components/UserActivityStats.tsxgithub-activity-tracker/frontend/src/components/UserActivityTable.tsxgithub-activity-tracker/frontend/src/components/UserProfile.tsxgithub-activity-tracker/frontend/src/lib/api.tsgithub-activity-tracker/frontend/src/lib/hooks.tsgithub-activity-tracker/frontend/src/lib/organizations.tsgithub-activity-tracker/frontend/src/lib/periods.tsgithub-activity-tracker/frontend/src/vite-env.d.tsgithub-activity-tracker/helm/gh-tracker-service/templates/_helpers.tplgithub-activity-tracker/helm/gh-tracker-service/templates/migrate-job.yamlgithub-activity-tracker/helm/gh-tracker-service/values.yaml
💤 Files with no reviewable changes (4)
- github-activity-tracker/frontend/src/vite-env.d.ts
- github-activity-tracker/frontend/.env.example
- github-activity-tracker/frontend/src/components/UserActivityTable.tsx
- github-activity-tracker/frontend/src/components/ActivityTrend.tsx
✅ Files skipped from review due to trivial changes (3)
- github-activity-tracker/backend/routes/userRolesRoute.js
- github-activity-tracker/frontend/index.html
- github-activity-tracker/backend/config/organizations.js
🚧 Files skipped from review as they are similar to previous changes (4)
- github-activity-tracker/backend/routes/orgActivityRoute.js
- github-activity-tracker/backend/routes/orgSummaryRoute.js
- github-activity-tracker/backend/routes/userNameSyncRoute.js
- github-activity-tracker/backend/services/orgSummaryService.js
|
|
||
| # Single source of truth for lookup tables (seeded into DB on migrate/start). | ||
| GITHUB_ORG=mosip,inji | ||
| USER_ROLES=Developer,Tech Lead,Architect,Product Owner,Leadership,QA Engineer,DevOps Engineer |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Quote the USER_ROLES value to prevent potential parsing issues.
The value contains spaces (e.g., "Tech Lead", "QA Engineer"). Without quotes, strict dotenv parsers may truncate at the first space, yielding Developer,Tech instead of the full list. The downstream defaultUserRoles.js reads process.env.USER_ROLES directly, so a truncation would silently produce an incomplete role list.
🔧 Proposed fix
-USER_ROLES=Developer,Tech Lead,Architect,Product Owner,Leadership,QA Engineer,DevOps Engineer
+USER_ROLES="Developer,Tech Lead,Architect,Product Owner,Leadership,QA Engineer,DevOps Engineer"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| USER_ROLES=Developer,Tech Lead,Architect,Product Owner,Leadership,QA Engineer,DevOps Engineer | |
| USER_ROLES="Developer,Tech Lead,Architect,Product Owner,Leadership,QA Engineer,DevOps Engineer" |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 15-15: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
🤖 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-activity-tracker/.env.example` at line 15, The USER_ROLES environment
entry should be quoted because it contains role names with spaces, which can be
truncated by strict dotenv parsing. Update the USER_ROLES value in the example
env file to use quotes so downstream consumers like defaultUserRoles.js still
read the full comma-separated list from process.env.USER_ROLES.
Source: Linters/SAST tools
Comment on lines
+6
to
+7
| GITHUB_TOKEN=your-github-token | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the dotenv keys in the expected order.
Move GITHUB_TOKEN before RDS_DATABASE; otherwise this example will keep tripping the repo’s env-file lint.
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 6-6: [UnorderedKey] The GITHUB_TOKEN key should go before the RDS_DATABASE key
(UnorderedKey)
🤖 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-activity-tracker/backend/.env.example` around lines 6 - 7, The dotenv
example keys are out of the expected order, which triggers the env-file lint.
Reorder the entries in the .env.example block so GITHUB_TOKEN appears before
RDS_DATABASE, keeping the rest of the example unchanged and matching the repo’s
required key ordering.
Source: Linters/SAST tools
Comment on lines
+56
to
64
| app.use(userNameSyncRoute); | ||
| app.use(userRoleRoute); | ||
| app.use(userRolesRoute); | ||
| app.use(organizationsRoute); | ||
| app.use(orgUsersRoute); | ||
| app.use(orgSummaryRoute); | ||
| app.use(userDetailsRoute); | ||
| app.use(orgActivityRoute); | ||
| app.use(leaderboardRoute); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how admin routes are mounted in app.js
cat -n github-activity-tracker/backend/app.js | sed -n '40,65p'
# Search for adminAuth middleware usage
rg -n 'adminAuth' github-activity-tracker/backend/app.js
# Check route files for any built-in auth guards
rg -n 'adminAuth|requireAuth|middleware' github-activity-tracker/backend/routes/userRoleRoute.js github-activity-tracker/backend/routes/userNameSyncRoute.jsRepository: mosip/mosip-labs
Length of output: 1282
🏁 Script executed:
#!/bin/bash
set -e
cat -n github-activity-tracker/backend/routes/userNameSyncRoute.js | sed -n '1,220p'
printf '\n---\n'
cat -n github-activity-tracker/backend/routes/userRoleRoute.js | sed -n '1,220p'
printf '\n---\n'
cat -n github-activity-tracker/backend/app.js | sed -n '1,140p'Repository: mosip/mosip-labs
Length of output: 9516
Mount the admin routes behind adminAuth. github-activity-tracker/backend/app.js:56-57 exposes userNameSyncRoute and userRoleRoute directly, and neither router applies its own auth guard, so the privileged /admin/* endpoints are reachable without the required middleware.
🤖 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-activity-tracker/backend/app.js` around lines 56 - 64, Mount the
privileged admin routers behind the existing adminAuth middleware in app.js: the
userNameSyncRoute and userRoleRoute are currently registered directly with
app.use, so update their wiring so requests must pass through adminAuth before
reaching those /admin/* handlers. Keep the other route mounts unchanged and use
the route names userNameSyncRoute and userRoleRoute to locate the affected
registration block.
Comment on lines
+20
to
+58
| ALTER TABLE user_details | ||
| DROP COLUMN IF EXISTS login, | ||
| DROP COLUMN IF EXISTS name, | ||
| DROP COLUMN IF EXISTS github_user_id; | ||
|
|
||
| CREATE INDEX IF NOT EXISTS idx_user_details_user_id ON user_details(user_id); | ||
| CREATE INDEX IF NOT EXISTS idx_user_details_role_id ON user_details(role_id); | ||
| CREATE INDEX IF NOT EXISTS idx_user_details_organization_id ON user_details(organization_id); | ||
| CREATE INDEX IF NOT EXISTS idx_user_details_active_from ON user_details(active_from); | ||
| CREATE INDEX IF NOT EXISTS idx_user_details_active_to ON user_details(active_to); | ||
|
|
||
| CREATE UNIQUE INDEX IF NOT EXISTS idx_user_details_one_active_per_user | ||
| ON user_details(user_id) | ||
| WHERE active = true; | ||
|
|
||
| -- GitHub profile name lives on github_users; assignments live in user_details. | ||
| ALTER TABLE github_users | ||
| ADD COLUMN IF NOT EXISTS name VARCHAR(255); | ||
|
|
||
| DO $$ | ||
| BEGIN | ||
| IF EXISTS ( | ||
| SELECT 1 | ||
| FROM information_schema.columns | ||
| WHERE table_schema = 'public' | ||
| AND table_name = 'user_details' | ||
| AND column_name = 'name' | ||
| ) THEN | ||
| UPDATE github_users u | ||
| SET | ||
| name = ud.name, | ||
| updated_at = CURRENT_TIMESTAMP | ||
| FROM user_details ud | ||
| WHERE ud.user_id = u.id | ||
| AND ud.active = true | ||
| AND ud.name IS NOT NULL | ||
| AND (u.name IS NULL OR u.name = ''); | ||
| END IF; | ||
| END $$; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Critical: user_details.name column is dropped before the data migration that uses it, causing silent data loss on upgrades.
Lines 20-23 execute DROP COLUMN IF EXISTS name on user_details, then lines 39-58 contain a DO $$ block that checks information_schema.columns for user_details.name to migrate it to github_users.name. Since the column was already dropped, the IF EXISTS check will always be false and the migration never runs. Any existing display names stored in user_details.name are permanently lost.
For fresh installs this is harmless (no data to migrate), but for upgrades from a schema where user_details had a name column, this is a data-loss bug.
Note: the github_users.role migration at lines 60-83 is correctly ordered — the DROP COLUMN IF EXISTS role at line 87 runs after the DO $$ block that uses it.
🐛 Proposed fix: move DROP COLUMN after the data migration
ALTER TABLE user_details
- DROP COLUMN IF EXISTS login,
- DROP COLUMN IF EXISTS name,
- DROP COLUMN IF EXISTS github_user_id;
+ DROP COLUMN IF EXISTS login,
+ DROP COLUMN IF EXISTS github_user_id;
CREATE INDEX IF NOT EXISTS idx_user_details_user_id ON user_details(user_id);
CREATE INDEX IF NOT EXISTS idx_user_details_role_id ON user_details(role_id);
CREATE INDEX IF NOT EXISTS idx_user_details_organization_id ON user_details(organization_id);
CREATE INDEX IF NOT EXISTS idx_user_details_active_from ON user_details(active_from);
CREATE INDEX IF NOT EXISTS idx_user_details_active_to ON user_details(active_to);
CREATE UNIQUE INDEX IF NOT EXISTS idx_user_details_one_active_per_user
ON user_details(user_id)
WHERE active = true;
-- GitHub profile name lives on github_users; assignments live in user_details.
ALTER TABLE github_users
ADD COLUMN IF NOT EXISTS name VARCHAR(255);
DO $$
BEGIN
IF EXISTS (
SELECT 1
FROM information_schema.columns
WHERE table_schema = 'public'
AND table_name = 'user_details'
AND column_name = 'name'
) THEN
UPDATE github_users u
SET
name = ud.name,
updated_at = CURRENT_TIMESTAMP
FROM user_details ud
WHERE ud.user_id = u.id
AND ud.active = true
AND ud.name IS NOT NULL
AND (u.name IS NULL OR u.name = '');
END IF;
END $$;
+-- Now safe to drop the name column after migration
+ALTER TABLE user_details
+ DROP COLUMN IF EXISTS name;
+
DO $$📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ALTER TABLE user_details | |
| DROP COLUMN IF EXISTS login, | |
| DROP COLUMN IF EXISTS name, | |
| DROP COLUMN IF EXISTS github_user_id; | |
| CREATE INDEX IF NOT EXISTS idx_user_details_user_id ON user_details(user_id); | |
| CREATE INDEX IF NOT EXISTS idx_user_details_role_id ON user_details(role_id); | |
| CREATE INDEX IF NOT EXISTS idx_user_details_organization_id ON user_details(organization_id); | |
| CREATE INDEX IF NOT EXISTS idx_user_details_active_from ON user_details(active_from); | |
| CREATE INDEX IF NOT EXISTS idx_user_details_active_to ON user_details(active_to); | |
| CREATE UNIQUE INDEX IF NOT EXISTS idx_user_details_one_active_per_user | |
| ON user_details(user_id) | |
| WHERE active = true; | |
| -- GitHub profile name lives on github_users; assignments live in user_details. | |
| ALTER TABLE github_users | |
| ADD COLUMN IF NOT EXISTS name VARCHAR(255); | |
| DO $$ | |
| BEGIN | |
| IF EXISTS ( | |
| SELECT 1 | |
| FROM information_schema.columns | |
| WHERE table_schema = 'public' | |
| AND table_name = 'user_details' | |
| AND column_name = 'name' | |
| ) THEN | |
| UPDATE github_users u | |
| SET | |
| name = ud.name, | |
| updated_at = CURRENT_TIMESTAMP | |
| FROM user_details ud | |
| WHERE ud.user_id = u.id | |
| AND ud.active = true | |
| AND ud.name IS NOT NULL | |
| AND (u.name IS NULL OR u.name = ''); | |
| END IF; | |
| END $$; | |
| ALTER TABLE user_details | |
| DROP COLUMN IF EXISTS login, | |
| DROP COLUMN IF EXISTS github_user_id; | |
| CREATE INDEX IF NOT EXISTS idx_user_details_user_id ON user_details(user_id); | |
| CREATE INDEX IF NOT EXISTS idx_user_details_role_id ON user_details(role_id); | |
| CREATE INDEX IF NOT EXISTS idx_user_details_organization_id ON user_details(organization_id); | |
| CREATE INDEX IF NOT EXISTS idx_user_details_active_from ON user_details(active_from); | |
| CREATE INDEX IF NOT EXISTS idx_user_details_active_to ON user_details(active_to); | |
| CREATE UNIQUE INDEX IF NOT EXISTS idx_user_details_one_active_per_user | |
| ON user_details(user_id) | |
| WHERE active = true; | |
| -- GitHub profile name lives on github_users; assignments live in user_details. | |
| ALTER TABLE github_users | |
| ADD COLUMN IF NOT EXISTS name VARCHAR(255); | |
| DO $$ | |
| BEGIN | |
| IF EXISTS ( | |
| SELECT 1 | |
| FROM information_schema.columns | |
| WHERE table_schema = 'public' | |
| AND table_name = 'user_details' | |
| AND column_name = 'name' | |
| ) THEN | |
| UPDATE github_users u | |
| SET | |
| name = ud.name, | |
| updated_at = CURRENT_TIMESTAMP | |
| FROM user_details ud | |
| WHERE ud.user_id = u.id | |
| AND ud.active = true | |
| AND ud.name IS NOT NULL | |
| AND (u.name IS NULL OR u.name = ''); | |
| END IF; | |
| END $$; | |
| -- Now safe to drop the name column after migration | |
| ALTER TABLE user_details | |
| DROP COLUMN IF EXISTS name; |
🧰 Tools
🪛 Squawk (2.59.0)
[warning] 21-21: Dropping a column may break existing clients.
(ban-drop-column)
[warning] 22-22: Dropping a column may break existing clients.
(ban-drop-column)
[warning] 23-23: Dropping a column may break existing clients.
(ban-drop-column)
🤖 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-activity-tracker/backend/migrations/007_create_user_details_table.sql`
around lines 20 - 58, The migration in `007_create_user_details_table.sql` drops
`user_details.name` before the backfill block that reads it, so the data
migration in the `DO $$` section never runs. Reorder the statements so the
`UPDATE github_users ... FROM user_details` logic executes before any `DROP
COLUMN IF EXISTS name` on `user_details`, keeping the existing
`github_users`/`user_details` migration flow intact and preserving upgrade data.
Comment on lines
+3
to
+41
| UPDATE user_details ud | ||
| SET | ||
| active_from = LEAST( | ||
| ud.active_from, | ||
| COALESCE( | ||
| (SELECT MIN(e.created_at) FROM activity_events e WHERE e.user_id = ud.user_id), | ||
| ud.active_from | ||
| ) | ||
| ), | ||
| updated_at = CURRENT_TIMESTAMP | ||
| WHERE EXISTS ( | ||
| SELECT 1 FROM activity_events e WHERE e.user_id = ud.user_id | ||
| ); | ||
|
|
||
| -- First role assignment rows should cover all prior activity, not only from assignment time. | ||
| UPDATE user_details current_ud | ||
| SET | ||
| active_from = LEAST( | ||
| current_ud.active_from, | ||
| COALESCE( | ||
| (SELECT MIN(e.created_at) FROM activity_events e WHERE e.user_id = current_ud.user_id), | ||
| current_ud.active_from | ||
| ), | ||
| COALESCE( | ||
| (SELECT MIN(h.active_from) FROM user_details h WHERE h.user_id = current_ud.user_id), | ||
| current_ud.active_from | ||
| ) | ||
| ), | ||
| updated_at = CURRENT_TIMESTAMP | ||
| WHERE current_ud.role_id IS NOT NULL | ||
| AND NOT EXISTS ( | ||
| SELECT 1 | ||
| FROM user_details prior_role | ||
| WHERE prior_role.user_id = current_ud.user_id | ||
| AND prior_role.role_id IS NOT NULL | ||
| AND prior_role.id <> current_ud.id | ||
| AND prior_role.active_to IS NOT NULL | ||
| AND prior_role.active_to <= current_ud.active_from | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Only backdate the first role assignment per user.
Line 3 backdates every user_details row before Line 18 tries to identify first role assignments. For users with multiple role-history rows, later assignments can be moved back to the earliest activity date, overlapping prior windows and causing role-filtered activity to be double-counted or misattributed. Restrict the migration to the earliest role assignment row per user, and leave later assignment windows unchanged.
🤖 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-activity-tracker/backend/migrations/008_backfill_user_details_active_from.sql`
around lines 3 - 41, The migration backdates too many user_details rows, which
can shift later role windows and overlap prior history. Update the backfill
logic in the SQL migration so only the earliest role assignment row per user is
backdated, using the existing user_details and activity_events lookups to
identify that single first role row. Keep later role assignment rows unchanged,
and ensure the updated_at touch only applies to the targeted earliest assignment
row.
Comment on lines
+3
to
+6
| ud.user_id = u.id | ||
| AND ud.active = true | ||
| AND ud.active_from <= e.created_at | ||
| AND (ud.active_to IS NULL OR e.created_at < ud.active_to)`; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include historical role assignment windows.
ud.active = true excludes prior role rows after they are closed with active_to, even when e.created_at falls inside that assignment window. That makes role-scoped analytics miss historical activity for previous roles.
Possible fix
function userAssignmentWindowSql() {
return `
ud.user_id = u.id
- AND ud.active = true
+ AND (ud.active = true OR ud.role_id IS NOT NULL)
AND ud.active_from <= e.created_at
AND (ud.active_to IS NULL OR e.created_at < ud.active_to)`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ud.user_id = u.id | |
| AND ud.active = true | |
| AND ud.active_from <= e.created_at | |
| AND (ud.active_to IS NULL OR e.created_at < ud.active_to)`; | |
| ud.user_id = u.id | |
| AND (ud.active = true OR ud.role_id IS NOT NULL) | |
| AND ud.active_from <= e.created_at | |
| AND (ud.active_to IS NULL OR e.created_at < ud.active_to)`; |
🤖 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-activity-tracker/backend/utils/userRoleSql.js` around lines 3 - 6, The
role filter in userRoleSql should not require ud.active = true for time-based
lookups, because it drops closed historical assignments that still match
e.created_at. Update the SQL fragment used by the role join/where logic to rely
on the assignment window (ud.active_from and ud.active_to) instead of the active
flag, so historical role rows are included when their window contains the event
time.
Comment on lines
+17
to
18
| const visibleActivities = activities.filter((activity) => activity.type !== 'commit'); | ||
| console.log('Activities received in UI:', activities); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove debug console.log before production.
console.log('Activities received in UI:', activities) is a debug artifact that logs raw API response data to the browser console. This should be removed before merge.
🧹 Proposed fix
const visibleActivities = activities.filter((activity) => activity.type !== 'commit');
- console.log('Activities received in UI:', activities);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const visibleActivities = activities.filter((activity) => activity.type !== 'commit'); | |
| console.log('Activities received in UI:', activities); | |
| const visibleActivities = activities.filter((activity) => activity.type !== 'commit'); |
🤖 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-activity-tracker/frontend/src/components/ActivityTable.tsx` around
lines 17 - 18, Remove the debug browser log from ActivityTable so raw activity
data is not emitted in production; delete the console.log added alongside
visibleActivities in the ActivityTable component and keep the filtering logic
unchanged.
| sortBy, | ||
| sortOrder, | ||
| ); | ||
| console.log("API RESPONSE:", data); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove debug console.log before production.
console.log("API RESPONSE:", data) logs the full API response (including user data) to the browser console. This should be removed before merge.
🧹 Proposed fix
);
- console.log("API RESPONSE:", data);
if (Array.isArray(data)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| console.log("API RESPONSE:", data); |
🤖 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-activity-tracker/frontend/src/components/TeamMembers.tsx` at line 109,
Remove the debug console output from TeamMembers so the full API response is not
logged in production. Delete the console.log("API RESPONSE:", data) statement in
the TeamMembers component, keeping the API handling logic intact and ensuring no
other console-based debugging remains in that flow.
Comment on lines
133
to
+135
| useEffect(() => { | ||
| setPage(1); | ||
| }, [org]); | ||
| }, [org, role, project]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add period to the page-reset useEffect dependency array.
When the user changes the period while on page 2+, the main fetch effect re-runs (period is in its deps) but the page is not reset to 1. This causes a fetch for a page number that may not exist in the new period's data, resulting in an empty table or incorrect pagination display.
🐛 Proposed fix
useEffect(() => {
setPage(1);
- }, [org, role, project]);
+ }, [org, role, project, period]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| setPage(1); | |
| }, [org]); | |
| }, [org, role, project]); | |
| useEffect(() => { | |
| setPage(1); | |
| }, [org, role, project, period]); |
🤖 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-activity-tracker/frontend/src/components/TeamMembers.tsx` around lines
133 - 135, The page-reset effect in TeamMembers is missing period in its
dependency list, so changing the period does not reset the table back to page 1.
Update the useEffect that calls setPage(1) to include period alongside org,
role, and project, so the reset runs whenever any filter affecting the dataset
changes.
Comment on lines
246
to
+247
| extraEnvVars: | ||
| - name: RDS_HOST | ||
| valueFrom: | ||
| configMapKeyRef: | ||
| key: rds-host | ||
| name: rds-config | ||
| - name: RDS_PORT | ||
| valueFrom: | ||
| configMapKeyRef: | ||
| key: rds-port | ||
| name: rds-config | ||
| - name: RDS_DATABASE | ||
| valueFrom: | ||
| configMapKeyRef: | ||
| key: rds-database | ||
| name: rds-config | ||
| - name: RDS_USER | ||
| valueFrom: | ||
| configMapKeyRef: | ||
| key: rds-username | ||
| name: rds-config | ||
| - name: RDS_PASSWORD | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: rds_password | ||
| name: rds-secret | ||
| - name: GITHUB_TOKEN | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: token | ||
| name: github-token | ||
| {{- include "gh-tracker-service.env" . | nindent 2 }} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify chart rendering after moving the include into a template.
helm template gh-tracker-service github-activity-tracker/helm/gh-tracker-service \
-f github-activity-tracker/helm/gh-tracker-service/values.yaml >/tmp/gh-tracker-rendered.yamlRepository: mosip/mosip-labs
Length of output: 1984
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the chart files and the `extraEnvVars` consumers.
git ls-files 'github-activity-tracker/helm/gh-tracker-service/**' | sort
printf '\n---\n'
rg -n --hidden --glob 'github-activity-tracker/helm/gh-tracker-service/**' 'extraEnvVars|include "gh-tracker-service.env"|values.yaml|templates/' github-activity-tracker/helm/gh-tracker-serviceRepository: mosip/mosip-labs
Length of output: 1955
Move the helper include out of values.yaml. values.yaml is data, not a Helm template, so {{- include "gh-tracker-service.env" . | nindent 2 }} makes the file invalid YAML and can break chart rendering. Put the include in the template that renders extraEnvVars, or keep this value literal.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 247-247: syntax error: expected the node content, but found '-'
(syntax)
🤖 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-activity-tracker/helm/gh-tracker-service/values.yaml` around lines 246
- 247, The `extraEnvVars` entry in `values.yaml` is incorrectly using the
`gh-tracker-service.env` helper include, which makes the values file invalid as
pure YAML. Move the Helm template logic out of `values.yaml` and into the
template that consumes `extraEnvVars` (or replace it with a literal value),
keeping `values.yaml` data-only.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit