feat: pull course in navbar and front page from airbase table instead…#1029
Conversation
WalkthroughThe codebase was refactored to replace static course data with dynamic fetching from an API. The homepage, navigation dropdowns, and footer now use a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NavLinks
participant useCourses
participant API
User->>NavLinks: Navigate to site
NavLinks->>useCourses: Call hook
useCourses->>API: Fetch /api/courses
API-->>useCourses: Return course data
useCourses-->>NavLinks: Provide courses and loading state
NavLinks->>User: Render dropdown (loading or links)
sequenceDiagram
participant User
participant CourseSection
participant useCourses
participant API
User->>CourseSection: View homepage
CourseSection->>useCourses: Call hook
useCourses->>API: Fetch /api/courses
API-->>useCourses: Return course data
useCourses-->>CourseSection: Provide courses and loading state
CourseSection->>User: Render featured and regular courses (or loading)
Assessment against linked issues
Poem
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
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (12)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (10)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
tolgadur
force-pushed
the
tolgadur/make-homepage-courses-dynamic
branch
2 times, most recently
from
4b52094 to
d29e35b
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Major architectural change to unify course data management by migrating from hardcoded constants to dynamic Airtable fetching across the homepage, navbar, and footer components.
- New
useCourseshook inapps/website/src/lib/hooks/useCourses.tshandles centralized course fetching and enrichment with manual properties - Modified
CourseSection.tsxto include loading states and null checks for empty courses - Consider adding Storybook stories for the modified components to document new dynamic behavior
- Potential performance impact from dynamic fetching vs. build-time optimization should be evaluated
- Data consistency questions regarding course descriptions and feature flags (
isNew,isFeatured) need resolution in Airtable
7 files reviewed, 5 comments
Edit PR Review Bot Settings | Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
apps/website/src/pages/_app.tsx (1)
44-45: Footer rendered with stale data on first paintWhile courses are loading the array is empty, so the footer initially shows no “Explore” links and then snaps in. Consider displaying a skeleton or keeping previously-fetched data in a global cache (React Context / SWR) to avoid layout shift.
apps/website/src/lib/hooks/useCourses.ts (1)
32-36: Resource fetch should be GET, not POSTRetrieving courses is idempotent – using
POSTis semantically incorrect and may surprise caches / observability tools. Switch toGETunless the endpoint truly requires a body.libraries/ui/src/Footer.tsx (1)
88-89: Guard against empty course listIf
courses.length === 0, the “Explore” column renders empty space.
Fallback links (e.g. to/courses) or conditional rendering will improve UX.-links={courses.map(...)} +links={courses.length ? courses.map(...) : [{ url: '/courses', label: 'All courses' }]}apps/website/src/components/Nav/_NavLinks.tsx (1)
38-47: Duplicate network request – consider central cache/context
useCoursesis now called in_app,NavLinks,CourseSection, etc.
Althoughaxios-hookshas rudimentary caching, each hook still sets up its own request/handlers. Moving the data into a React Context (populated once in_app) will eliminate redundant hooks and simplify loading states.Not urgent, but worth tracking before the number of consumers grows.
apps/website/src/components/homepage/CourseSection.test.tsx (1)
36-41: Type safety lost in mockeduseAxiosreturn tupleThe cast to
as UseAxiosResulthides tuple shape mismatches (only two non-null items supplied).
Populate the full[state, refetch, cancel]tuple to avoid silent test breakage when the library API changes.const axiosState = { data: mockData, loading: false, error: null }; return [axiosState, vi.fn(), vi.fn()] as UseAxiosResult;apps/website/src/components/homepage/CourseSection.tsx (2)
20-24: Fallback if noisFeaturedcourse is flaggedToday the whole section vanishes when no item is marked
isFeatured, which is a silent failure that may go unnoticed in production.
Consider defaulting to the first course instead, or at least logging a warning so the data team knows the flag is missing.
54-69:keyshould be stable and unique – use an id/slug instead oftitleUsing
titleas akeycan lead to duplicated keys if two courses share the same name or if a title changes between renders, forcing React to remount elements and lose state.- key={course.title} + key={course.id ?? course.slug ?? course.path}(Replace with whatever unique identifier the API provides.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
apps/website/src/components/Nav/__snapshots__/Nav.test.tsx.snapis excluded by!**/*.snapapps/website/src/components/homepage/__snapshots__/CourseSection.test.tsx.snapis excluded by!**/*.snaplibraries/ui/src/__snapshots__/Footer.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
apps/website/src/components/Nav/_NavLinks.tsx(3 hunks)apps/website/src/components/homepage/CourseSection.test.tsx(1 hunks)apps/website/src/components/homepage/CourseSection.tsx(3 hunks)apps/website/src/lib/hooks/useCourses.ts(1 hunks)apps/website/src/pages/_app.tsx(2 hunks)libraries/ui/src/Footer.tsx(3 hunks)libraries/ui/src/constants.ts(0 hunks)
💤 Files with no reviewable changes (1)
- libraries/ui/src/constants.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/website/src/lib/hooks/useCourses.ts (2)
apps/website/src/pages/api/courses/index.ts (1)
GetCoursesResponse(6-9)libraries/ui/src/constants.ts (1)
CourseType(1-1)
apps/website/src/components/homepage/CourseSection.test.tsx (1)
apps/website/src/pages/api/courses/index.ts (1)
GetCoursesResponse(6-9)
apps/website/src/components/Nav/_NavLinks.tsx (3)
apps/website/src/lib/hooks/useCourses.ts (1)
useCourses(31-63)apps/website/src/lib/routes.ts (1)
ROUTES(80-94)libraries/ui/src/Tag.tsx (1)
Tag(12-30)
🔇 Additional comments (1)
apps/website/src/pages/_app.tsx (1)
12-19: ```shell
#!/bin/bash
set -eLocate files with 'useCourses' in their filename
echo "Searching for files named useCourses:"
files=$(fd -t f -i useCourses || true)
if [ -z "$files" ]; then
echo "No files found with fd; falling back to find:"
files=$(find . -type f -iname 'useCourses')
fi
echo "Found files:"
echo "$files"Print contents of those files to inspect implementation
for file in $files; do
echo "---- Begin $file ----"
sed -n '1,200p' "$file"
echo "---- End $file ----"
done</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
Code generally looks fine - what does this look like when loading? (maybe attach a short screen recording/similar?) |
I think it looks fine imo. I attached a screen recording. Is the differing text between constants.tsx and the airtable db fine? (see PR description for more info). |
|
Maybe some kind of loading indicator would be good? Otherwise there's an unexpected jump on page load which isn't that great. I think moving the isNew/isFeatured logic into Airtable sounds sensible rather than hardcoding! |
Done. Have a look at the updated screenshare.
Could you give me rights to add airtable fields to the courses table? |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
libraries/ui/src/CourseCard.tsx (1)
99-105: Guard against emptycadenceto avoid rendering a blank tag.If the backend ever returns an empty string, the UI will render an empty
<Tag>which looks broken. A one-line guard keeps the UX intact.- <Tag className="course-card__type">{cadence}</Tag> + {cadence && ( + <Tag className="course-card__type">{cadence}</Tag> + )}
🧹 Nitpick comments (2)
libraries/ui/src/CourseCard.tsx (2)
18-20: Remove obsolete ESLint suppression and consider stricter typing forcadence.
cadenceis now actually consumed byCourseCard, therefore thereact/no-unused-prop-typessuppression is no longer needed.
Additionally, leaving the prop as plainstringinvites typos. A union such as'Self-paced' | 'Cohort-based' | 'Bootcamp'(or whatever the canonical values are) would give us free validation at compile-time.- // eslint-disable-next-line react/no-unused-prop-types - cadence: string, + cadence: + | 'Self-paced' + | 'Cohort-based' + | 'Bootcamp' + | string; // fallback for yet-unknown values
108-118: Drop the unusedcadenceprop forwarded toFeaturedCourseCard.
FeaturedCourseCarddoesn’t referencecadence; passing it adds noise and may trigger “unused prop” warnings further down. If you foresee future use, a TODO comment would be clearer.- cadence={cadence}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/website/src/components/Nav/_NavLinks.tsx(6 hunks)apps/website/src/components/homepage/CourseSection.tsx(3 hunks)apps/website/src/lib/api/db/tables.ts(5 hunks)apps/website/src/lib/hooks/useCourses.ts(1 hunks)apps/website/src/pages/_app.tsx(2 hunks)libraries/ui/src/CourseCard.tsx(4 hunks)libraries/ui/src/Footer.tsx(2 hunks)libraries/ui/src/constants.ts(0 hunks)
💤 Files with no reviewable changes (1)
- libraries/ui/src/constants.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/website/src/pages/_app.tsx
- apps/website/src/lib/hooks/useCourses.ts
- apps/website/src/components/homepage/CourseSection.tsx
- apps/website/src/lib/api/db/tables.ts
- libraries/ui/src/Footer.tsx
- apps/website/src/components/Nav/_NavLinks.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/website/src/components/homepage/CourseSection.test.tsx (1)
88-91: Consider enhancing snapshot test coverage.While the snapshot test is functional, consider whether it should also verify loading states since the component now handles asynchronous data fetching.
You could add a test for the loading state:
test('renders loading state correctly', () => { // Mock loading state vi.mocked(useAxios).mockReturnValueOnce([{ data: null, loading: true, error: null, }, null!, null!] as UseAxiosResult); const { container } = render(<CourseSection />); expect(container).toMatchSnapshot(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/website/src/components/homepage/CourseSection.test.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci
🔇 Additional comments (3)
apps/website/src/components/homepage/CourseSection.test.tsx (3)
1-12: LGTM! Clean imports and type definitions.The imports are well-organized and the
UseAxiosResulttype definition correctly captures the return type from axios-hooks for better type safety in the mock.
13-36: Excellent mock helper function design.The
mockCoursehelper function provides a comprehensive default course object with all required fields and allows flexible overrides. This is a clean pattern for test data generation that makes the tests more maintainable.
94-117: Good implementation of BEM class selector approach.The test correctly implements the BEM class selector strategy as recommended in past review comments. This approach is more future-proof than relying on text content that might change. The test coverage includes both featured and regular course cards with proper GA event verification.
tolgadur
force-pushed
the
tolgadur/make-homepage-courses-dynamic
branch
from
bcd505f to
4673439
Compare
… of constants.tsx minor bugfixes as suggested by greptile fix import issues add progress dots simplify useCourses hook move loading to footer add loading to navbar update snapshots update coursesection test nav tests
tolgadur
force-pushed
the
tolgadur/make-homepage-courses-dynamic
branch
from
4673439 to
19b8561
Compare
tarinrickett
left a comment
tarinrickett
left a comment
There was a problem hiding this comment.
are the screenshots updated to the latest changes? just to confirm updates like cadence are working as expected
Ah no I updated them now. sorry about that. I updated it now. Cadence works but the wording is a bit different. We would need to change that in the airtable. |
3 participants
Description
Previously we were pulling courses from both the airtable in the /courses path and from a constant.tsx that needs to be maintained manually. The values from the constant were used on the frontpage, the footer and the navigation bar. This PR unified this and pulls all from Airtable
Needed clarifications:
Issue
Fixes #746
Developer checklist
Screenshot
Screen recording when loading:
Screen.Recording.2025-06-18.at.15.00.44.mov