feat(opensource): implement AI contribution coach, handle stream errors, and fix recommended repos endpoint (#957)#1453
Open
Rajal-ui wants to merge 2 commits into
Conversation
…, and fix repo recommendation route
github-actions
Bot
added
enhancement
|
Hi @Rajal-ui, thanks for contributing to InternHack! 🎉 I have automatically:
Our workflows will now analyze your changes to classify:
Tip Ensure your PR description references the issue it resolves (e.g. Happy coding! 🚀 |
github-actions
Bot
added
level:critical
Contributor
📝 WalkthroughWalkthroughThis PR implements a personalized AI contribution coach that guides students throughout their open-source journey. The coach triggers context-aware suggestions at key milestones (first PR completion, repo discovery, inactivity), generates advice via an AI provider, and persists saved guidance for later reference. The feature spans client UI, state management, server-side suggestion generation, and database persistence. ChangesAI Contribution Coach Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/module/student/opensource/FirstPRRoadmapPage.tsx (1)
62-98:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd missing dependencies to
useCallbackhook.The
togglecallback is missingtriggerCoachanduser?.skillsin its dependency array. If these values change, the callback will continue to use stale references, causing incorrect behavior when the user's skills update or the coach store is recreated.🐛 Proposed fix
const toggle = useCallback( (id: string) => { const isCurrentlyCompleted = completed.has(id); const nextCompleted = !isCurrentlyCompleted; const isCompletingLastStep = nextCompleted && completed.size === STEPS.length - 1; setCompleted((prev) => { const next = new Set(prev); if (nextCompleted) next.add(id); else next.delete(id); return next; }); // Trigger coach if this click completes the roadmap if (isCompletingLastStep) { triggerCoach({ trigger: "FIRST_PR_COMPLETE", context: { skills: user?.skills || [], completedGuides: ["First Pull Request Roadmap"], }, }); } void patchFirstPRProgress(id, nextCompleted).catch(() => { setCompleted((prev) => { const rolledBack = new Set(prev); if (isCurrentlyCompleted) rolledBack.add(id); else rolledBack.delete(id); return rolledBack; }); toast.error("Failed to update progress. Please try again."); }); }, - [completed], + [completed, triggerCoach, user?.skills], );🤖 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 `@client/src/module/student/opensource/FirstPRRoadmapPage.tsx` around lines 62 - 98, The useCallback for the toggle function captures stale references because its dependency array only lists completed; update the dependency array for toggle (the callback declared as const toggle = useCallback(...)) to include triggerCoach and user?.skills (and any other external helpers used like patchFirstPRProgress or toast if they are not stable) so the callback re-creates when the coach trigger or user's skills change; ensure you reference the same identifiers (triggerCoach, user?.skills, patchFirstPRProgress, toast) in the dependency array.
🧹 Nitpick comments (3)
client/src/module/student/opensource/ContributionCoachPanel.tsx (2)
153-164: ⚖️ Poor tradeoffConsider migrating native buttons to the shared
Buttoncomponent.Multiple native
<button>elements throughout the panel could benefit from the sharedButtoncomponent for consistency and maintainability. The Button component supports variants (primary, secondary, mono, ghost, danger), modes (button, icon, link), and sizes (sm, md, lg).Examples:
- Lines 153-164: Collapsible header toggle
- Lines 197-204: Delete button in saved advice items
- Lines 336-342: Close button in panel header
- Lines 392-398: Retry button in error state
- Lines 425-436: Save advice button
- Lines 437-443: Refresh button
As per coding guidelines: Use the reusable
Buttoncomponent fromclient/src/components/ui/button.tsxfor all new buttons; supports variants (primary, secondary, mono, ghost, danger), modes (button, icon, link), and sizes (sm, md, lg).Also applies to: 197-204, 336-342, 392-398, 425-436, 437-443
🤖 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 `@client/src/module/student/opensource/ContributionCoachPanel.tsx` around lines 153 - 164, The panel uses native <button> elements (e.g., the collapsible toggle in ContributionCoachPanel that calls setExpanded and renders Bookmark/ChevronDown) which should be replaced with the shared Button component from client/src/components/ui/button.tsx for consistent styling and variants; update each native button (including the collapsible header toggle, delete buttons in saved advice items, panel close button, retry button, save advice, and refresh actions) to use Button with appropriate props (mode, variant, size) and preserve existing onClick handlers (e.g., the setExpanded toggle) and icon children (Bookmark, ChevronDown, etc.) so functionality and accessibility remain identical.
135-140: ⚡ Quick winConsider refactoring the lazy-load pattern to avoid setState in useEffect.
The linter warning about calling setState within an effect can be addressed by moving the load trigger to the toggle handler rather than reacting to
expandedin an effect. While the current pattern works, it can trigger cascading renders.♻️ Alternative pattern
const load = useCallback(async () => { setLoading(true); try { const data = await fetchSavedAdvice(); setItems(data); } catch { toast.error("Failed to load saved advice"); } finally { setLoading(false); } }, []); - useEffect(() => { - if (expanded && items.length === 0) { - void load(); - } - }, [expanded, items.length, load]); + + const handleToggle = () => { + const nextExpanded = !expanded; + setExpanded(nextExpanded); + if (nextExpanded && items.length === 0) { + void load(); + } + }; return ( <div className="border-t border-stone-200 dark:border-white/10"> <button - onClick={() => setExpanded((p) => !p)} + onClick={handleToggle}🤖 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 `@client/src/module/student/opensource/ContributionCoachPanel.tsx` around lines 135 - 140, The current useEffect watches expanded and calls load(), which can trigger setState inside an effect; instead, remove that effect and invoke load() directly from the toggle/expand handler (the function that flips `expanded`) so that when the handler sets `expanded` to true it also checks `items.length === 0` and calls `load()` once. Update the handler that currently toggles `expanded` (the component's expand/toggle callback) to perform the conditional `items.length === 0` check and call `load` only on expand, and then delete the useEffect that references `expanded`, `items.length`, and `load`.server/src/database/prisma/schema/base.prisma (1)
1501-1502: ⚡ Quick winUse a composite
(userId, createdAt)index for the saved-advice feed.
CoachService.getSavedAdvice()filters byuserId, orders bycreatedAt desc, and only takes 50 rows. Two single-column indexes still leave Postgres sorting within the user's slice as this table grows; a composite index matches the actual query shape.🤖 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 `@server/src/database/prisma/schema/base.prisma` around lines 1501 - 1502, Replace the two single-column indexes with a composite index that matches the query shape used by CoachService.getSavedAdvice: instead of @@index([userId]) and @@index([createdAt]) add @@index([userId, createdAt]) on the saved-advice model (the Prisma model that stores saved advice rows) so Postgres can use the index for both the WHERE userId filter and ORDER BY createdAt DESC when limiting to 50 rows.
🤖 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 `@client/src/module/student/opensource/CoachFloatingButton.tsx`:
- Around line 15-25: Replace the direct Framer Motion button with the shared
Button component: wrap the existing motion.button usage by using Button (import
from client/src/components/ui/button.tsx) with asChild so Framer Motion can be
composed, keep the same motion props (initial, animate, transition) on the
motion.button element, forward onClick to toggle, preserve title,
className/styling and the Sparkles icon as children, and use the appropriate
variant/mode/size props (e.g., variant="primary" or mode="icon" and size="md")
to match design; update imports to include Button and ensure motion.button
remains the animated element nested via asChild.
In `@client/src/module/student/opensource/RepoDiscoveryPage.tsx`:
- Around line 252-279: toggleBookmark currently looks up repo details from
data?.repos and will miss repos not in the current page/filters; extend the
bookmark storage to persist minimal repo metadata (name, language, domain)
alongside IDs when calling saveBookmarks, and update toggleBookmark to: find
repo in data?.repos first, if not found read the persisted metadata store for
that id, then use that metadata when calling triggerCoach; adjust saveBookmarks
(and its storage shape) to accept/save metadata and update any loadBookmarks
logic to populate bookmarks state from the new persisted format so bookmarking
and coach triggers work for repos outside the current view.
In `@client/src/module/student/opensource/stores/coach.store.ts`:
- Line 86: Replace the catch clause typed as "err: any" with "err: unknown" in
coach.store.ts and then narrow the error before using it (e.g., in the catch
block for the affected method(s) that currently declare "catch (err: any)" check
"err instanceof Error" or use a type‑guard to read err.message or other
properties; handle the non‑Error branch with a safe fallback). Ensure any
downstream references to err inside the catch are updated to use the narrowed
variable (or converted to a safe string) so you no longer rely on the unsafe any
type.
In `@server/src/module/coach/coach.controller.ts`:
- Around line 84-87: The delete handler in CoachController currently forwards
all errors from this.coachService.deleteAdvice(req.user.id, adviceId) to
next(err); instead, catch the domain errors thrown by CoachService.deleteAdvice
and translate them into proper HTTP responses: if the service indicates "not
found" return res.status(404).json({ message: "Advice not found" }), if it
indicates "not authorized" return res.status(403).json({ message: "Not
authorized to delete this advice" }), and only call next(err) for unexpected
errors; keep using req.user.id and adviceId to locate the failing call and
preserve existing success response res.json({ message: "Advice deleted" }).
- Around line 78-80: The current guard using Number(req.params["id"]) allows
non-integer strings like "1.5" or "1e2" to pass; change validation so
req.params["id"] is rejected unless it is an integer string (e.g. match /^\d+$/
or use Number.isInteger(parseFloat(idStr)) combined with strict string check)
before converting to adviceId, return res.status(400).json(...) when the check
fails, and only call the downstream service/Prisma with the parsed integer (use
parseInt) so the Prisma lookup always receives an Int-compatible value
(referencing adviceId and req.params["id"] in coach.controller.ts).
In `@server/src/module/coach/coach.service.ts`:
- Around line 92-95: The prompt expects the "most recently bookmarked repo" but
context.bookmarkedRepos is an unordered array of summaries, so update the data
model or selection logic to supply a single highlighted repo (e.g., add/consume
a lastBookmarkedRepo or ensure bookmarkedRepos is ordered with newest-first) and
change the string-building in coach.service (the reposBlock usage and the other
block at lines 108-115) to reference that single selected repo (e.g.,
context.lastBookmarkedRepo or context.bookmarkedRepos[0]) so the coach
explicitly includes the repo name, language and domain for the most recent
bookmark instead of an ambiguous list.
- Around line 103-105: The prompt in the switch case for trigger
"FIRST_PR_COMPLETE" asks the model for "3 specific" active repositories and
concrete issue-level first steps even though only a free-form user profile is
provided; update the scenarioInstruction string in coach.service.ts (the
FIRST_PR_COMPLETE branch) to avoid demanding verifiable repo facts — either (A)
ask the model to suggest general categories/types of beginner-friendly projects
and concrete generic first-steps based on the user's tech stack and skills, or
(B) prompt the assistant to request the user to supply a short list of candidate
repos/URLs before producing concrete repo-level guidance; ensure the new
instruction references the user's tech stack/profile only and removes
requirements for maintainer-activity judgments and issue numbers so the model
won't be forced to invent specifics.
In `@server/src/module/coach/coach.validation.ts`:
- Around line 30-33: The coachSaveSchema currently allows any non-empty string
for trigger; restrict it to the canonical set of supported triggers used by
coachSuggestSchema and CoachService.titleFromTrigger by replacing the free-form
string with an enum validation (e.g., z.enum([...]) or z.nativeEnum) that
references the same constant or exported list of triggers (import the
SUPPORTED_TRIGGERS or TRIGGER_NAMES value used by
coachSuggestSchema/CoachService), so saved triggers are validated against the
finite allowed set and match the client contract and titleFromTrigger
expectations.
---
Outside diff comments:
In `@client/src/module/student/opensource/FirstPRRoadmapPage.tsx`:
- Around line 62-98: The useCallback for the toggle function captures stale
references because its dependency array only lists completed; update the
dependency array for toggle (the callback declared as const toggle =
useCallback(...)) to include triggerCoach and user?.skills (and any other
external helpers used like patchFirstPRProgress or toast if they are not stable)
so the callback re-creates when the coach trigger or user's skills change;
ensure you reference the same identifiers (triggerCoach, user?.skills,
patchFirstPRProgress, toast) in the dependency array.
---
Nitpick comments:
In `@client/src/module/student/opensource/ContributionCoachPanel.tsx`:
- Around line 153-164: The panel uses native <button> elements (e.g., the
collapsible toggle in ContributionCoachPanel that calls setExpanded and renders
Bookmark/ChevronDown) which should be replaced with the shared Button component
from client/src/components/ui/button.tsx for consistent styling and variants;
update each native button (including the collapsible header toggle, delete
buttons in saved advice items, panel close button, retry button, save advice,
and refresh actions) to use Button with appropriate props (mode, variant, size)
and preserve existing onClick handlers (e.g., the setExpanded toggle) and icon
children (Bookmark, ChevronDown, etc.) so functionality and accessibility remain
identical.
- Around line 135-140: The current useEffect watches expanded and calls load(),
which can trigger setState inside an effect; instead, remove that effect and
invoke load() directly from the toggle/expand handler (the function that flips
`expanded`) so that when the handler sets `expanded` to true it also checks
`items.length === 0` and calls `load()` once. Update the handler that currently
toggles `expanded` (the component's expand/toggle callback) to perform the
conditional `items.length === 0` check and call `load` only on expand, and then
delete the useEffect that references `expanded`, `items.length`, and `load`.
In `@server/src/database/prisma/schema/base.prisma`:
- Around line 1501-1502: Replace the two single-column indexes with a composite
index that matches the query shape used by CoachService.getSavedAdvice: instead
of @@index([userId]) and @@index([createdAt]) add @@index([userId, createdAt])
on the saved-advice model (the Prisma model that stores saved advice rows) so
Postgres can use the index for both the WHERE userId filter and ORDER BY
createdAt DESC when limiting to 50 rows.
🪄 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: 154e3e1b-d934-4d2e-97dd-6e1eedfcff66
📒 Files selected for processing (17)
client/src/module/student/opensource/CoachFloatingButton.tsxclient/src/module/student/opensource/ContributionCoachPanel.tsxclient/src/module/student/opensource/FirstPRRoadmapPage.tsxclient/src/module/student/opensource/OpenSourceLayout.tsxclient/src/module/student/opensource/RepoDiscoveryPage.tsxclient/src/module/student/opensource/api/coach.api.tsclient/src/module/student/opensource/stores/coach.store.tsserver/src/database/prisma/migrations/20260605150000_add_coach_advice/migration.sqlserver/src/database/prisma/schema/base.prismaserver/src/index.tsserver/src/module/coach/coach.controller.tsserver/src/module/coach/coach.routes.tsserver/src/module/coach/coach.service.tsserver/src/module/coach/coach.validation.tsserver/src/module/opensource/opensource.controller.tsserver/src/module/opensource/opensource.routes.tsserver/src/module/opensource/opensource.service.ts
Comment on lines
+15
to
+25
| <motion.button | ||
| initial={{ scale: 0, opacity: 0 }} | ||
| animate={{ scale: 1, opacity: 1 }} | ||
| transition={{ delay: 0.5, type: "spring", stiffness: 300, damping: 20 }} | ||
| onClick={toggle} | ||
| title="Open Contribution Coach" | ||
| className="fixed bottom-6 right-6 z-30 w-12 h-12 rounded-xl bg-lime-400 text-stone-950 shadow-lg shadow-lime-500/25 hover:bg-lime-300 hover:shadow-xl hover:shadow-lime-500/30 transition-all flex items-center justify-center border-0 cursor-pointer group" | ||
| > | ||
| <Sparkles className="w-5 h-5 group-hover:scale-110 transition-transform" /> | ||
| </motion.button> | ||
| ); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use the shared Button component for the floating action button.
All new buttons should use the reusable Button component from client/src/components/ui/button.tsx with appropriate variants, modes, and sizes. The Button component supports composition with Framer Motion via the asChild prop (Radix Slot pattern).
♻️ Refactor to use Button component
+import { Button } from "../../../components/ui/button";
+import { Slot } from "`@radix-ui/react-slot`";
export default function CoachFloatingButton() {
const { toggle, isOpen } = useCoachStore();
if (isOpen) return null;
return (
- <motion.button
- initial={{ scale: 0, opacity: 0 }}
- animate={{ scale: 1, opacity: 1 }}
- transition={{ delay: 0.5, type: "spring", stiffness: 300, damping: 20 }}
- onClick={toggle}
- title="Open Contribution Coach"
- className="fixed bottom-6 right-6 z-30 w-12 h-12 rounded-xl bg-lime-400 text-stone-950 shadow-lg shadow-lime-500/25 hover:bg-lime-300 hover:shadow-xl hover:shadow-lime-500/30 transition-all flex items-center justify-center border-0 cursor-pointer group"
- >
- <Sparkles className="w-5 h-5 group-hover:scale-110 transition-transform" />
- </motion.button>
+ <Button
+ variant="primary"
+ mode="icon"
+ size="md"
+ onClick={toggle}
+ title="Open Contribution Coach"
+ className="fixed bottom-6 right-6 z-30 group"
+ asChild
+ >
+ <motion.button
+ initial={{ scale: 0, opacity: 0 }}
+ animate={{ scale: 1, opacity: 1 }}
+ transition={{ delay: 0.5, type: "spring", stiffness: 300, damping: 20 }}
+ >
+ <Sparkles className="w-5 h-5 group-hover:scale-110 transition-transform" />
+ </motion.button>
+ </Button>
);
}As per coding guidelines: Use the reusable Button component from client/src/components/ui/button.tsx for all new buttons; supports variants (primary, secondary, mono, ghost, danger), modes (button, icon, link), and sizes (sm, md, lg). Use asChild prop (Radix Slot) when composing Button with other elements.
🤖 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 `@client/src/module/student/opensource/CoachFloatingButton.tsx` around lines 15
- 25, Replace the direct Framer Motion button with the shared Button component:
wrap the existing motion.button usage by using Button (import from
client/src/components/ui/button.tsx) with asChild so Framer Motion can be
composed, keep the same motion props (initial, animate, transition) on the
motion.button element, forward onClick to toggle, preserve title,
className/styling and the Sparkles icon as children, and use the appropriate
variant/mode/size props (e.g., variant="primary" or mode="icon" and size="md")
to match design; update imports to include Button and ensure motion.button
remains the animated element nested via asChild.
Comment on lines
252
to
279
| const toggleBookmark = (id: number) => { | ||
| const isBookmarking = !bookmarks.includes(id); | ||
|
|
||
| setBookmarks((prev) => { | ||
| const next = prev.includes(id) ? prev.filter((b) => b !== id) : [...prev, id]; | ||
| const next = isBookmarking ? [...prev, id] : prev.filter((b) => b !== id); | ||
| saveBookmarks(next); | ||
| return next; | ||
| }); | ||
|
|
||
| if (isBookmarking) { | ||
| const repo = data?.repos?.find((r) => r.id === id); | ||
| if (repo) { | ||
| triggerCoach({ | ||
| trigger: "REPO_BOOKMARKED", | ||
| context: { | ||
| skills: user?.skills || [], | ||
| bookmarkedRepos: [ | ||
| { | ||
| name: repo.name, | ||
| language: repo.language, | ||
| domain: repo.domain || undefined, | ||
| }, | ||
| ], | ||
| }, | ||
| }); | ||
| } | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Coach trigger may fail silently when bookmarking repos not in current view.
When a user bookmarks a repository, the code looks up the repo details from data?.repos (line 262). However, data contains only the repos from the current page and active filters. If the user bookmarks a repo from the "Saved" view or if the repo is not in the current filtered results, this lookup will return undefined and the coach won't trigger.
Consider one of these approaches:
- Store minimal repo metadata (name, language, domain) in localStorage alongside bookmark IDs
- Fetch repo details on-demand when bookmarking
- Accept the limitation and document that coach triggers only work when the repo is in the current view
🤖 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 `@client/src/module/student/opensource/RepoDiscoveryPage.tsx` around lines 252
- 279, toggleBookmark currently looks up repo details from data?.repos and will
miss repos not in the current page/filters; extend the bookmark storage to
persist minimal repo metadata (name, language, domain) alongside IDs when
calling saveBookmarks, and update toggleBookmark to: find repo in data?.repos
first, if not found read the persisted metadata store for that id, then use that
metadata when calling triggerCoach; adjust saveBookmarks (and its storage shape)
to accept/save metadata and update any loadBookmarks logic to populate bookmarks
state from the new persisted format so bookmarking and coach triggers work for
repos outside the current view.
| const { fetchCoachSuggestion } = await import("../api/coach.api"); | ||
| const result = await fetchCoachSuggestion(payload); | ||
| setAdvice(result); | ||
| } catch (err: any) { |
Contributor
There was a problem hiding this comment.
Use unknown instead of any for error handling.
TypeScript's unknown type provides better type safety for error handling. When catching errors, use unknown and narrow the type as needed.
🔧 Proposed fix
- } catch (err: any) {
+ } catch (err: unknown) {
console.error("[coach] fetch failed:", err);
- const msg = err.response?.data?.message || "Failed to get coaching advice. Please check your connection.";
+ const msg =
+ (err && typeof err === 'object' && 'response' in err &&
+ err.response && typeof err.response === 'object' && 'data' in err.response &&
+ err.response.data && typeof err.response.data === 'object' && 'message' in err.response.data)
+ ? String(err.response.data.message)
+ : "Failed to get coaching advice. Please check your connection.";
setError(msg);📝 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
| } catch (err: any) { | |
| } catch (err: unknown) { | |
| console.error("[coach] fetch failed:", err); | |
| const msg = | |
| (err && typeof err === 'object' && 'response' in err && | |
| err.response && typeof err.response === 'object' && 'data' in err.response && | |
| err.response.data && typeof err.response.data === 'object' && 'message' in err.response.data) | |
| ? String(err.response.data.message) | |
| : "Failed to get coaching advice. Please check your connection."; | |
| setError(msg); |
🧰 Tools
🪛 GitHub Actions: CI / 2_Lint Client.txt
[warning] 86-86: @typescript-eslint/no-explicit-any: Unexpected any. Specify a different type.
🪛 GitHub Actions: CI / Lint Client
[warning] 86-86: ESLint (@typescript-eslint/no-explicit-any): Unexpected any. Specify a different type.
🪛 GitHub Check: Lint Client
[warning] 86-86:
Unexpected any. Specify a different type
🤖 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 `@client/src/module/student/opensource/stores/coach.store.ts` at line 86,
Replace the catch clause typed as "err: any" with "err: unknown" in
coach.store.ts and then narrow the error before using it (e.g., in the catch
block for the affected method(s) that currently declare "catch (err: any)" check
"err instanceof Error" or use a type‑guard to read err.message or other
properties; handle the non‑Error branch with a safe fallback). Ensure any
downstream references to err inside the catch are updated to use the narrowed
variable (or converted to a safe string) so you no longer rely on the unsafe any
type.
Comment on lines
+78
to
+80
| const adviceId = Number(req.params["id"]); | ||
| if (!adviceId || isNaN(adviceId)) { | ||
| res.status(400).json({ message: "Invalid advice ID" }); |
Contributor
There was a problem hiding this comment.
Reject non-integer route IDs before calling the service.
Number("1.5") and Number("1e2") both pass this guard, even though the downstream Prisma lookup expects an Int. That turns a bad request into a server error instead of a clean 400.
🤖 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 `@server/src/module/coach/coach.controller.ts` around lines 78 - 80, The
current guard using Number(req.params["id"]) allows non-integer strings like
"1.5" or "1e2" to pass; change validation so req.params["id"] is rejected unless
it is an integer string (e.g. match /^\d+$/ or use
Number.isInteger(parseFloat(idStr)) combined with strict string check) before
converting to adviceId, return res.status(400).json(...) when the check fails,
and only call the downstream service/Prisma with the parsed integer (use
parseInt) so the Prisma lookup always receives an Int-compatible value
(referencing adviceId and req.params["id"] in coach.controller.ts).
Comment on lines
+84
to
+87
| await this.coachService.deleteAdvice(req.user.id, adviceId); | ||
| res.json({ message: "Advice deleted" }); | ||
| } catch (err) { | ||
| next(err); |
Contributor
There was a problem hiding this comment.
Translate delete failures to 404/403 instead of bubbling generic errors.
CoachService.deleteAdvice() throws normal domain failures for "not found" and "not authorized", but this handler forwards them unchanged to generic error middleware. Clients will see ordinary user mistakes as 500s unless some other layer special-cases those message strings. Based on learnings: Controller layer must handle request/response, call service, and format 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 `@server/src/module/coach/coach.controller.ts` around lines 84 - 87, The delete
handler in CoachController currently forwards all errors from
this.coachService.deleteAdvice(req.user.id, adviceId) to next(err); instead,
catch the domain errors thrown by CoachService.deleteAdvice and translate them
into proper HTTP responses: if the service indicates "not found" return
res.status(404).json({ message: "Advice not found" }), if it indicates "not
authorized" return res.status(403).json({ message: "Not authorized to delete
this advice" }), and only call next(err) for unexpected errors; keep using
req.user.id and adviceId to locate the failing call and preserve existing
success response res.json({ message: "Advice deleted" }).
Comment on lines
+92
to
+95
| const reposBlock = | ||
| context.bookmarkedRepos.length > 0 | ||
| ? `Bookmarked repos:\n${context.bookmarkedRepos.map((r) => `- ${r.name} (${r.language ?? "unknown"}, ${r.domain ?? "general"})`).join("\n")}` | ||
| : ""; |
Contributor
There was a problem hiding this comment.
REPO_BOOKMARKED never identifies which repo was just bookmarked.
The prompt says "most recently bookmarked repo", but context.bookmarkedRepos is only an array of repo summaries with no selected item or ordering field. As soon as a user has multiple bookmarks, the coach can generate advice for the wrong project.
Also applies to: 108-115
🤖 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 `@server/src/module/coach/coach.service.ts` around lines 92 - 95, The prompt
expects the "most recently bookmarked repo" but context.bookmarkedRepos is an
unordered array of summaries, so update the data model or selection logic to
supply a single highlighted repo (e.g., add/consume a lastBookmarkedRepo or
ensure bookmarkedRepos is ordered with newest-first) and change the
string-building in coach.service (the reposBlock usage and the other block at
lines 108-115) to reference that single selected repo (e.g.,
context.lastBookmarkedRepo or context.bookmarkedRepos[0]) so the coach
explicitly includes the repo name, language and domain for the most recent
bookmark instead of an ambiguous list.
Comment on lines
+103
to
+105
| switch (trigger) { | ||
| case "FIRST_PR_COMPLETE": | ||
| scenarioInstruction = `The user just completed the "First Pull Request Roadmap". Congratulate them briefly, then suggest 3 specific beginner-friendly open-source repositories that match their tech stack and skills. For each repo, explain why it's a good fit and give one concrete first step (e.g., "Look at issue #X labeled good-first-issue"). Prioritize repos that are actively maintained and welcoming to new contributors.`; |
Contributor
There was a problem hiding this comment.
Don't ask the model for repo facts you never supply.
This branch requires "3 specific" repositories, active-maintainer judgment, and concrete first steps, but the prompt only receives free-form profile strings. Without a grounded repo list or issue metadata, Gemini has to invent those details.
🤖 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 `@server/src/module/coach/coach.service.ts` around lines 103 - 105, The prompt
in the switch case for trigger "FIRST_PR_COMPLETE" asks the model for "3
specific" active repositories and concrete issue-level first steps even though
only a free-form user profile is provided; update the scenarioInstruction string
in coach.service.ts (the FIRST_PR_COMPLETE branch) to avoid demanding verifiable
repo facts — either (A) ask the model to suggest general categories/types of
beginner-friendly projects and concrete generic first-steps based on the user's
tech stack and skills, or (B) prompt the assistant to request the user to supply
a short list of candidate repos/URLs before producing concrete repo-level
guidance; ensure the new instruction references the user's tech stack/profile
only and removes requirements for maintainer-activity judgments and issue
numbers so the model won't be forced to invent specifics.
Comment on lines
+30
to
+33
| export const coachSaveSchema = z.object({ | ||
| content: z.string().min(1).max(20000), | ||
| trigger: z.string().min(1).max(50), | ||
| title: z.string().min(1).max(200).optional(), |
Contributor
There was a problem hiding this comment.
Constrain saved triggers to the supported coach trigger set.
coachSaveSchema accepts any non-empty string, but coachSuggestSchema, CoachService.titleFromTrigger(), and the client contract all assume a finite set of trigger names. A typo or rogue value gets persisted as valid advice metadata and only degrades later into the generic fallback 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 `@server/src/module/coach/coach.validation.ts` around lines 30 - 33, The
coachSaveSchema currently allows any non-empty string for trigger; restrict it
to the canonical set of supported triggers used by coachSuggestSchema and
CoachService.titleFromTrigger by replacing the free-form string with an enum
validation (e.g., z.enum([...]) or z.nativeEnum) that references the same
constant or exported list of triggers (import the SUPPORTED_TRIGGERS or
TRIGGER_NAMES value used by coachSuggestSchema/CoachService), so saved triggers
are validated against the finite allowed set and match the client contract and
titleFromTrigger expectations.
Comment on lines
+430
to
+443
| // 2. Fetch repos matching skills (language or techStack subset) | ||
| // We search for repos where the primary language is in the student's skills | ||
| const repos = await prisma.opensourceRepo.findMany({ | ||
| where: { | ||
| OR: [ | ||
| { language: { in: skills, mode: "insensitive" } }, | ||
| { trending: true }, | ||
| ], | ||
| }, | ||
| take: 8, | ||
| orderBy: [{ trending: "desc" }, { stars: "desc" }], | ||
| }); | ||
|
|
||
| return repos; |
Contributor
There was a problem hiding this comment.
Skill-based recommendations are under-matched and become mostly generic.
Current matching only checks language IN skills and then ORs trending, so profile stacks like React/Next/Node frequently miss and fall back to generic trending-heavy results instead of personalized repos.
Proposed fix
- // 2. Fetch repos matching skills (language or techStack subset)
- // We search for repos where the primary language is in the student's skills
- const repos = await prisma.opensourceRepo.findMany({
- where: {
- OR: [
- { language: { in: skills, mode: "insensitive" } },
- { trending: true },
- ],
- },
- take: 8,
- orderBy: [{ trending: "desc" }, { stars: "desc" }],
- });
-
- return repos;
+ const normalizedSkills = [...new Set(skills.map((s) => s.trim()).filter(Boolean))];
+
+ const matched = await prisma.opensourceRepo.findMany({
+ where: {
+ OR: [
+ { language: { in: normalizedSkills, mode: "insensitive" } },
+ { techStack: { hasSome: normalizedSkills } },
+ { tags: { hasSome: normalizedSkills } },
+ ],
+ },
+ take: 8,
+ orderBy: { stars: "desc" },
+ });
+
+ if (matched.length >= 8) return matched;
+
+ const fallback = await prisma.opensourceRepo.findMany({
+ where: {
+ trending: true,
+ id: { notIn: matched.map((r) => r.id) },
+ },
+ take: 8 - matched.length,
+ orderBy: { stars: "desc" },
+ });
+
+ return [...matched, ...fallback];
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.
Description
This comprehensive PR introduces the AI Contribution Coach feature alongside critical backend adjustments to ensure database synchronization, error-resilient frontend UI state updates, and proper repository content routing.
By tying together explicit milestone action hooks and robust runtime exception blocks, this PR completely resolves the endless loading state loop and settles background network thrashing.
Core Changes Built
1. Database & Schema Pipeline (
Prisma)coachAdvicemodel insideserver/src/database/prisma/schema/base.prismato cleanly record custom AI responses bound directly to user profile entities via a cascadinguserIdkey.2. Fully Defended Client State (
Zustand& React UI)coach.store.tsto block duplicate overlapping tracking triggers ifisLoadingorpendingPayloadconditions are already true.triggerCoach()hook invocations fully outside of synchronous state modifier updates (setBookmarksinsideRepoDiscoveryPage.tsxand milestones insideFirstPRRoadmapPage.tsx), preventing loop recalculation triggers during component render phases.try/catch/finallyblock to guaranteeisLoading: falsefires instantly upon failure, unlocking stuck UI components.ContributionCoachPanel.tsxto handle failures elegantly with warning notices and reactive Retry trigger buttons.3. Content Feeding Route Re-alignment
GET /api/opensource/recommendedinside the backend directory layer.opensource.service.tsthat safely serves specific repository recommendations mapping clean text strings directly to a student's technical stack, gracefully defaulting to trending star records if their profile stack configuration is uninitialized.Related Issue
Fixes #957
Type of Change
Testing & Verification Checklist
npx tsc --noEmitlocally with zero structural errors.POSTtransaction per trigger block..env, secrets, or unneeded tracking directories are committed.Here is a detailed, professional, and fully formatted Pull Request body that encapsulates your entire feature cycle—combining the core AI Contribution Coach Architecture, your Robust Error-Handling Safeguards, and the Repository Recommendation Pipeline Fix.Screenshots
Summary by CodeRabbit
Release Notes
New Features