dashboard-routing-fix#299
Open
CloudM22 wants to merge 1 commit into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🔒 Invite-Only AccessHi @CloudM22, the Think Throo AI code review features are currently invite-only. Your account ( Want access? Request access here. This check is temporary while we control usage during the early access phase. |
Reviewer's GuideRefactors the learning dashboard to use shared dashboard metadata and server-derived course stats, centralizing Sanity queries and wiring both dashboard routes through a common client component that computes progress from real lesson completion data. Sequence diagram for loading the learning dashboard with shared stats and progresssequenceDiagram
actor User
participant DashboardPage
participant getLearningDashboardStats
participant sanityClient
participant DashboardAltPage
participant courseProgressClient
User->>DashboardPage: request /dashboard or /learn/dashboard
DashboardPage->>getLearningDashboardStats: getLearningDashboardStats()
getLearningDashboardStats->>sanityClient: fetch(ARCHITECTURE_STATS_QUERY)
getLearningDashboardStats->>sanityClient: fetch(PRODUCTION_STATS_QUERY)
getLearningDashboardStats-->>DashboardPage: CourseStat[]
DashboardPage-->>User: HTML with DashboardAltPage(courseStats)
User->>DashboardAltPage: load client dashboard
loop for each course in dashboardCourses
DashboardAltPage->>courseProgressClient: getCompletedLessons(courseSlug)
courseProgressClient-->>DashboardAltPage: completed lesson rows
end
DashboardAltPage-->>User: render progress cards and links
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughThe PR refactors the learning dashboard to centralize Sanity data fetching. A new ChangesDashboard Server-Side Data Flow
Sequence DiagramsequenceDiagram
participant Browser
participant DashboardServer as DashboardPage (Server)
participant StatsHelper as getLearningDashboardStats
participant Sanity
participant Component as DashboardAlt (Client)
participant ProgressAPI as courseProgressClient
Browser->>DashboardServer: request dashboard
DashboardServer->>StatsHelper: await getLearningDashboardStats()
StatsHelper->>Sanity: fetch modules for both courses
Sanity-->>StatsHelper: module data
StatsHelper-->>DashboardServer: CourseStat[]
DashboardServer->>Browser: render DashboardAlt with courseStats prop
Component->>ProgressAPI: fetch completed lessons per course
ProgressAPI-->>Component: completedByCourse map
Component->>Component: compute overall/per-course progress
Component->>Browser: render dashboard with progress metrics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 docstrings
🧪 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. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Fetching per-course completion data in
DashboardAltPageviacourseProgressClient.getCompletedLessonson the client for every course can be expensive; consider moving this aggregation into a server-side helper (or extendinggetLearningDashboardStats) so the page can render with progress data from a single server call. - Since
getLearningDashboardStatsexposesfirstLessonHrefper module, consider using that (or a similar server-derived URL) for the "Start Learning" link instead of hard-codingpathindashboardCourses, so routing stays aligned with CMS data and avoids manual path duplication.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Fetching per-course completion data in `DashboardAltPage` via `courseProgressClient.getCompletedLessons` on the client for every course can be expensive; consider moving this aggregation into a server-side helper (or extending `getLearningDashboardStats`) so the page can render with progress data from a single server call.
- Since `getLearningDashboardStats` exposes `firstLessonHref` per module, consider using that (or a similar server-derived URL) for the "Start Learning" link instead of hard-coding `path` in `dashboardCourses`, so routing stays aligned with CMS data and avoids manual path duplication.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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: 2
🤖 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 `@apps/platform/components/DashboardAlt.tsx`:
- Around line 25-35: The current useEffect uses Promise.all over
dashboardCourses so a single rejected courseProgressClient.getCompletedLessons
call rejects the whole batch and leaves completedByCourse unset; change the
logic to use Promise.allSettled over the mapped promises (inside the useEffect),
then iterate the settled results to build entries for successful ones (and
optionally log failures) and finally call
setCompletedByCourse(Object.fromEntries(entries)) so partial successes populate
completedByCourse while individual failures are handled per-course.
In `@apps/platform/lib/learning-dashboard-stats.ts`:
- Around line 18-33: ARCHITECTURE_STATS_QUERY and PRODUCTION_STATS_QUERY
currently project slug and chapterSlug as Sanity slug objects but
SanityModuleRow and buildHref() expect string slugs; update both queries so the
module slug and chapterSlug use slug.current (same pattern already used for
lessonSlug) so they return string values instead of objects, ensuring fields
named "slug" and "chapterSlug" are projected as slug.current to avoid `[object
Object]` in generated URLs.
🪄 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: 5c044578-55b2-4f3e-89a8-118ca2cda9d5
📒 Files selected for processing (6)
apps/platform/app/(backend)/api/dashboard/stats/route.tsapps/platform/app/(platform)/dashboard/page.tsxapps/platform/app/(platform)/learn/dashboard/page.tsxapps/platform/components/DashboardAlt.tsxapps/platform/lib/course-data.tsapps/platform/lib/learning-dashboard-stats.ts
Comment on lines
+25
to
+35
| useEffect(() => { | ||
| Promise.all( | ||
| dashboardCourses.map(async (course) => { | ||
| const rows = await courseProgressClient.getCompletedLessons({ | ||
| courseSlug: course.courseSlug, | ||
| }) | ||
| return [course.courseSlug, rows.length] as const | ||
| }), | ||
| ) | ||
| .then((entries) => setCompletedByCourse(Object.fromEntries(entries))) | ||
| .catch(console.error) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does JavaScript Promise.all reject as soon as one input promise rejects, and does Promise.allSettled wait for all input promises to settle?
💡 Result:
Yes, both of your premises are correct. Promise.all follows a fail-fast behavior: it rejects immediately as soon as any of the input promises reject, with the reason of that first rejection [1][2]. It does not wait for the other pending promises to settle [3][4]. Promise.allSettled waits for all input promises to settle, regardless of whether they fulfill or reject [5][1]. Once all promises have settled, it fulfills with an array of objects describing the outcome of each promise [5].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/all
- 2: https://stackoverflow.com/questions/30362733/handling-errors-in-promise-all
- 3: https://stackoverflow.com/questions/42304394/why-does-javascripts-promise-all-not-run-all-promises-in-failure-conditions
- 4: https://www.reddit.com/r/node/comments/1dgfzje/promiseall_unhandled_rejections_dont_show_up/
- 5: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/allSettled
Use Promise.allSettled to prevent one failed course from zeroing the entire dashboard.
With Promise.all, a single rejected getCompletedLessons() call rejects the entire operation, leaving completedByCourse empty and falling back to 0% progress everywhere. Switch to Promise.allSettled to wait for all courses and preserve successful results while handling per-course failures individually.
🔧 Proposed fix
useEffect(() => {
- Promise.all(
+ Promise.allSettled(
dashboardCourses.map(async (course) => {
const rows = await courseProgressClient.getCompletedLessons({
courseSlug: course.courseSlug,
})
return [course.courseSlug, rows.length] as const
}),
)
- .then((entries) => setCompletedByCourse(Object.fromEntries(entries)))
- .catch(console.error)
+ .then((results) => {
+ const entries = results.flatMap((result, index) => {
+ if (result.status === "fulfilled") return [result.value]
+
+ console.error(
+ "Failed to load progress for",
+ dashboardCourses[index].courseSlug,
+ result.reason,
+ )
+ return []
+ })
+
+ setCompletedByCourse(Object.fromEntries(entries))
+ })
}, [])🤖 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 `@apps/platform/components/DashboardAlt.tsx` around lines 25 - 35, The current
useEffect uses Promise.all over dashboardCourses so a single rejected
courseProgressClient.getCompletedLessons call rejects the whole batch and leaves
completedByCourse unset; change the logic to use Promise.allSettled over the
mapped promises (inside the useEffect), then iterate the settled results to
build entries for successful ones (and optionally log failures) and finally call
setCompletedByCourse(Object.fromEntries(entries)) so partial successes populate
completedByCourse while individual failures are handled per-course.
Comment on lines
+18
to
+33
| const ARCHITECTURE_STATS_QUERY = ` | ||
| *[_type == "module" && "Codebase Architecture" in categories[]->title]{ | ||
| "slug": slug, | ||
| title, | ||
| "lessonCount": count(*[_type == "codebaseArchitecture" && ^._id in chapter[]->module[]._ref]), | ||
| "chapter": *[ | ||
| _type == "chapter" && references(^._id) && order == 1 | ||
| ][0]{ | ||
| "chapterSlug": slug, | ||
| "lesson": *[ | ||
| _type == "codebaseArchitecture" && references(^._id) && order == 1 | ||
| ][0]{ | ||
| "lessonSlug": slug.current | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether module/chapter slugs are Sanity slug fields and compare them with the current projections.
set -e
echo "== Schema fields =="
rg -n -C3 'name:\s*"module"|name:\s*"chapter"|name:\s*"slug"|type:\s*"slug"' --glob '!**/node_modules/**'
echo
echo "== Dashboard query projections =="
rg -n -C2 '"slug":\s*slug(\.current)?|"chapterSlug":\s*slug(\.current)?' apps/platform/lib/learning-dashboard-stats.tsRepository: ramu-narasinga/thinkthroo
Length of output: 1308
🏁 Script executed:
#!/bin/bash
# Find buildHref function definition
rg -n "buildHref" --glob '*.ts' --glob '*.tsx' -A 5 | head -40Repository: ramu-narasinga/thinkthroo
Length of output: 1051
🏁 Script executed:
#!/bin/bash
# Check the complete learning-dashboard-stats.ts file to see buildHref usage
wc -l apps/platform/lib/learning-dashboard-stats.tsRepository: ramu-narasinga/thinkthroo
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Read the complete file to understand the full context
cat -n apps/platform/lib/learning-dashboard-stats.tsRepository: ramu-narasinga/thinkthroo
Length of output: 3873
Use slug.current in both dashboard stats queries to match the SanityModuleRow type expectations.
The ARCHITECTURE_STATS_QUERY and PRODUCTION_STATS_QUERY queries project slug and chapterSlug as Sanity slug type objects, but SanityModuleRow expects these as strings (lines 55, 59). When these objects are passed to buildHref() and stringified in the template literal (line 70), they will serialize as [object Object], producing malformed URLs. The lessonSlug field already uses slug.current correctly; apply the same pattern to module and chapter slugs.
Fix required
const ARCHITECTURE_STATS_QUERY = `
*[_type == "module" && "Codebase Architecture" in categories[]->title]{
- "slug": slug,
+ "slug": slug.current,
title,
"lessonCount": count(*[_type == "codebaseArchitecture" && ^._id in chapter[]->module[]._ref]),
"chapter": *[
_type == "chapter" && references(^._id) && order == 1
][0]{
- "chapterSlug": slug,
+ "chapterSlug": slug.current,
"lesson": *[
_type == "codebaseArchitecture" && references(^._id) && order == 1
][0]{
"lessonSlug": slug.current
}
}
}
`;
const PRODUCTION_STATS_QUERY = `
*[_type == "module" && "Production Grade Projects" in categories[]->title]{
- "slug": slug,
+ "slug": slug.current,
title,
"lessonCount": count(*[_type == "productionGradeProjects" && ^._id in chapter[]->module[]._ref]),
"chapter": *[
_type == "chapter" && references(^._id) && order == 1
][0]{
- "chapterSlug": slug,
+ "chapterSlug": slug.current,
"lesson": *[
_type == "productionGradeProjects" && references(^._id) && order == 1
][0]{
"lessonSlug": slug.current
}
}
}
`;Also applies to lines 36–51 (PRODUCTION_STATS_QUERY).
🤖 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 `@apps/platform/lib/learning-dashboard-stats.ts` around lines 18 - 33,
ARCHITECTURE_STATS_QUERY and PRODUCTION_STATS_QUERY currently project slug and
chapterSlug as Sanity slug objects but SanityModuleRow and buildHref() expect
string slugs; update both queries so the module slug and chapterSlug use
slug.current (same pattern already used for lessonSlug) so they return string
values instead of objects, ensuring fields named "slug" and "chapterSlug" are
projected as slug.current to avoid `[object Object]` in generated URLs.
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 Sourcery
Wire the dashboard pages to a shared, data-driven dashboard component backed by real CMS statistics instead of hardcoded mock data.
New Features:
Bug Fixes:
Enhancements:
Summary by CodeRabbit